Skip to content

feat: add icon option for pages/groups#18

Merged
adkah merged 1 commit into
masterfrom
ak-sidebar-icons
May 15, 2026
Merged

feat: add icon option for pages/groups#18
adkah merged 1 commit into
masterfrom
ak-sidebar-icons

Conversation

@adkah
Copy link
Copy Markdown
Contributor

@adkah adkah commented May 15, 2026

Adds an icon option to the group config type and page frontmatter.

@greptile-apps
Copy link
Copy Markdown

greptile-apps Bot commented May 15, 2026

Greptile Summary

This PR adds an optional icon field to page frontmatter and group config, threading the icon data from content collection entries through the sidebar tree builder and into the React sidebar component for rendering.

  • types.ts, schemas/docs.ts, content.ts, tree.ts, and site.ts consistently extend the existing method/methodMap pattern to add icon/iconMap with no regressions.
  • sidebar-tree-view.tsx renders icons via a TreeIcon helper that does a dynamic key lookup on the full icons export from lucide-react, which prevents tree-shaking and ships the entire icon library to the client.
  • breadcrumbs.ts accepts and threads iconMap through its call chain even though breadcrumb items ({ label, href }) never expose an icon field.

Confidence Score: 3/5

The backend data-flow changes are clean, but the icon rendering approach in the sidebar bundles the entire lucide-react icon set on the client side — this should be resolved before merging to a production docs site.

The plumbing across types.ts, content.ts, tree.ts, and site.ts is solid and mirrors established patterns. The sidebar-tree-view.tsx change introduces a dynamic icon lookup against the full icons export from lucide-react, meaning every icon in the library ends up in the client bundle regardless of how many are actually used. This is a meaningful regression for a documentation site where JS bundle size directly affects page load.

src/components/sidebar-tree-view.tsx needs the most attention — the icon import strategy should be revisited. src/bach/breadcrumbs.ts is worth a second look to decide whether iconMap belongs in its interface at all.

Important Files Changed

Filename Overview
src/components/sidebar-tree-view.tsx Adds icon rendering to sidebar nodes using icons from lucide-react; the dynamic key lookup bypasses tree-shaking and pulls the entire icon library into the client bundle.
src/bach/breadcrumbs.ts Threads iconMap through breadcrumb functions, but the return type { label, href } never surfaces icons — the parameter is only used internally for a findFirstHref call that doesn't need it.
src/bach/tree.ts Correctly threads iconMap through buildPages and buildSidebarTree, populating icon on both article and category nodes from config and entry data.
src/bach/types.ts Adds optional icon field to SidebarCategoryNode, SidebarArticleNode, GroupItem, and CollectionGroupItem — all clean, backward-compatible interface extensions.
src/bach/content.ts Mirrors the existing methodMap pattern to build iconMap from collection entries and returns it alongside other sidebar data.
src/bach/site.ts Propagates iconMap into SiteContext and passes it through buildSidebarTree and buildBreadcrumbs — straightforward plumbing change.
src/bach/schemas/docs.ts Adds icon: z.string().optional() to the docs frontmatter schema — minimal and consistent with the existing field pattern.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[Page/Group Config] -->|icon field| B[buildCollectionsSidebarData]
    B -->|iconMap| C[buildSidebarTree]
    C -->|iconMap| D[buildPages]
    D -->|SidebarCategoryNode.icon| E[sidebar-tree-view.tsx]
    D -->|SidebarArticleNode.icon| E
    E -->|name lookup| F[icons from lucide-react]
    F -->|Icon component| G[Rendered TreeIcon]

    H[Frontmatter icon field] -->|docsSchema| I[buildSidebarEntryMap]
    I -->|iconMap.set| B

    J[buildBreadcrumbs] -->|iconMap passed| K[searchPagesForBreadcrumbs]
    K -->|buildPages for firstChildHref only| L[findFirstHref]
    L -->|href string| M[Breadcrumb label+href]
    K -.->|icon unused in output| M
Loading

Comments Outside Diff (1)

  1. src/bach/breadcrumbs.ts, line 10-16 (link)

    P2 iconMap parameter unused in breadcrumb output

    searchPagesForBreadcrumbs (and buildBreadcrumbs) accept iconMap and thread it into a buildPages call, but the call is made solely to extract findFirstHref — icon data on the returned nodes is discarded. The breadcrumb return type { label: string; href: string }[] has no icon field, so the map is carried through the entire call chain without ever contributing to the output. Removing it would simplify both signatures and the site.ts callsite.

Reviews (1): Last reviewed commit: "feat: add icon option for pages/groups" | Re-trigger Greptile

Comment on lines +2 to +11
import { icons } from 'lucide-react'
import { Badge } from '@/components/ui/badge'
import { cn } from '@/lib/utils'
import { hasActiveChild, isPathActive } from '@/bach/nav'
import type { SidebarCategoryNode, SidebarNode } from '@/bach/types'

function TreeIcon({ name }: { name: string }) {
const Icon = icons[name as keyof typeof icons]
if (!Icon) return null
return <Icon className="h-4 w-4 shrink-0" />
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Entire icon library bundled due to dynamic key lookup

import { icons } from 'lucide-react' pulls in the entire icon map (~1400+ components). Because the lookup is a runtime string key (icons[name as keyof typeof icons]), bundlers like Vite/Rollup cannot tree-shake unused icons — every icon ships to the client even if only two or three are ever referenced. For a documentation site sidebar this could add hundreds of KB to the JS bundle. A common alternative is to use a lazy-load registry or restrict the allowed icon set to named imports that the bundler can prune.

@adkah adkah merged commit 4ee98e4 into master May 15, 2026
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants