charles/claude-hooks
1
0
Fork 0
Code Issues 10 Pull requests Projects Releases Packages 1 Wiki Activity Actions

feat(settings): inline settings tree in sidenav + flyout on collapsed rail #1109

Merged
reviewer merged 5 commits from dev/1104 into main 2026-05-11 18:19:44 +00:00
Conversation 5 Commits 5 Files changed 24 +634 -287
dev commented 2026-05-11 16:35:53 +00:00
Collaborator
Copy link

Summary

  • Extracts NAV_GROUPS into apps/web/src/lib/settings-nav.ts as a factory function getSettingsNavGroups() with NavItem/NavGroup types and helpers (groupContainsPath, activeGroupLabel, findActiveNavEntry)
  • Replaces the static Settings footer NavLink in nav-sections.tsx with a SettingsFooterItem that renders:
    • Expanded rail + /settings/*: SettingsInlineTree with collapsible accordion groups, active leaf highlighted, open groups seeded from the active route
    • Expanded rail + other routes: plain NavLink unchanged
    • Collapsed rail (any route): SettingsCollapsedFlyout (Base UI Menu anchored to the Settings icon; trigger is a design-system <Button> carrying aria-current="page" when the active route is /settings/*, with the same left-edge accent bar as the rest of the rail)
  • Drops SettingsGroupedNav left column from settings.tsx; content is now full-width with a SettingsBreadcrumbBar (Settings › Group › Leaf, separators rendered as <ChevronRight>) above the <Outlet>
  • Removes per-page <SettingsBreadcrumb> from every settings route + ai-provider-keys-section.tsx and deletes the component itself — the layout-level SettingsBreadcrumbBar is now the single breadcrumb source
  • Adds --no-sandbox/--disable-setuid-sandbox to Playwright launchOptions for container environments
  • Adds browser tests for inline tree default-open state, flyout open/content + leaf-click dismissal, and breadcrumb segments
  • Adds CHROMIUM_PATH + PLAYWRIGHT_BROWSERS_PATH to the test task's passThroughEnv in turbo.json so the host pre-push fallback to /usr/bin/chromium reaches vitest

Sub-route SettingsSideNav decision (AC documentation)

settings.agents.tsx and settings.service.tsx keep their own <SettingsSideNav> because their sub-trees are deeper than what fits in the top-level NAV_GROUPS:

  • agents sub-tree: agent types · per-agent secrets · config history · sessions · admin
  • service sub-tree: forge · ai-providers · container · watchdogs · design

Folding those into the main sidenav's NAV_GROUPS would either bloat the inline tree past the comfortable footer-section height or break the "top-level group / leaf" model the breadcrumb relies on. The layout-level SettingsBreadcrumbBar continues to provide the Settings › Agents › … outermost breadcrumb above their pages, while the in-page side nav handles the deeper navigation. Re-evaluating this two-level pattern is in scope for milestone #36 ("Settings unification").

Test plan

  • CI browser-mode tests pass (sidebar-nav.test.tsx inline tree + flyout, settings.test.tsx breadcrumb + no side-nav column)
  • Visual: expanded sidebar on /settings/appearance shows inline tree with the General group open by default and the Appearance leaf active; clicking another group's header expands it
  • Visual: collapsed rail on /settings/* shows the Settings icon highlighted (aria-current="page" + accent bar) and clicking it opens a flyout with the grouped tree
  • Visual: /settings/service shows breadcrumb Settings › Forges › Service config
  • No double breadcrumb on /settings/appearance, /settings/labels, /settings/repos, /settings/secrets, /settings/voice-input, /settings/language, /settings/agent-types, or the AI providers section
  • Mobile drawer still works (drawer renders inline tree on /settings/*)

Closes #1104

## Summary - Extracts `NAV_GROUPS` into `apps/web/src/lib/settings-nav.ts` as a factory function `getSettingsNavGroups()` with `NavItem`/`NavGroup` types and helpers (`groupContainsPath`, `activeGroupLabel`, `findActiveNavEntry`) - Replaces the static Settings footer `NavLink` in `nav-sections.tsx` with a `SettingsFooterItem` that renders: - **Expanded rail + `/settings/*`**: `SettingsInlineTree` with collapsible accordion groups, active leaf highlighted, open groups seeded from the active route - **Expanded rail + other routes**: plain `NavLink` unchanged - **Collapsed rail (any route)**: `SettingsCollapsedFlyout` (Base UI Menu anchored to the Settings icon; trigger is a design-system `<Button>` carrying `aria-current="page"` when the active route is `/settings/*`, with the same left-edge accent bar as the rest of the rail) - Drops `SettingsGroupedNav` left column from `settings.tsx`; content is now full-width with a `SettingsBreadcrumbBar` (`Settings › Group › Leaf`, separators rendered as `<ChevronRight>`) above the `<Outlet>` - Removes per-page `<SettingsBreadcrumb>` from every settings route + `ai-provider-keys-section.tsx` and deletes the component itself — the layout-level `SettingsBreadcrumbBar` is now the single breadcrumb source - Adds `--no-sandbox`/`--disable-setuid-sandbox` to Playwright `launchOptions` for container environments - Adds browser tests for inline tree default-open state, flyout open/content + leaf-click dismissal, and breadcrumb segments - Adds `CHROMIUM_PATH` + `PLAYWRIGHT_BROWSERS_PATH` to the `test` task's `passThroughEnv` in `turbo.json` so the host pre-push fallback to `/usr/bin/chromium` reaches vitest ## Sub-route `SettingsSideNav` decision (AC documentation) `settings.agents.tsx` and `settings.service.tsx` **keep** their own `<SettingsSideNav>` because their sub-trees are deeper than what fits in the top-level `NAV_GROUPS`: - **agents** sub-tree: agent types · per-agent secrets · config history · sessions · admin - **service** sub-tree: forge · ai-providers · container · watchdogs · design Folding those into the main sidenav's `NAV_GROUPS` would either bloat the inline tree past the comfortable footer-section height or break the "top-level group / leaf" model the breadcrumb relies on. The layout-level `SettingsBreadcrumbBar` continues to provide the `Settings › Agents › …` outermost breadcrumb above their pages, while the in-page side nav handles the deeper navigation. Re-evaluating this two-level pattern is in scope for milestone #36 ("Settings unification"). ## Test plan - [ ] CI browser-mode tests pass (`sidebar-nav.test.tsx` inline tree + flyout, `settings.test.tsx` breadcrumb + no side-nav column) - [ ] Visual: expanded sidebar on `/settings/appearance` shows inline tree with the **General** group open by default and the Appearance leaf active; clicking another group's header expands it - [ ] Visual: collapsed rail on `/settings/*` shows the Settings icon highlighted (`aria-current="page"` + accent bar) and clicking it opens a flyout with the grouped tree - [ ] Visual: `/settings/service` shows breadcrumb `Settings › Forges › Service config` - [ ] No double breadcrumb on `/settings/appearance`, `/settings/labels`, `/settings/repos`, `/settings/secrets`, `/settings/voice-input`, `/settings/language`, `/settings/agent-types`, or the AI providers section - [ ] Mobile drawer still works (drawer renders inline tree on `/settings/*`) Closes #1104
feat(settings): inline settings tree in sidenav + flyout on collapsed rail (#1104)
Some checks failed
qa / sql-layer-check (pull_request) Successful in 8s
Details
qa / dockerfile (pull_request) Successful in 9s
Details
qa / db-schema (pull_request) Successful in 15s
Details
qa / i18n-string-check (pull_request) Successful in 20s
Details
qa / qa-1 (pull_request) Failing after 3m27s
Details
qa / qa (pull_request) Failing after 0s
Details
ce0ada78e2 
- Extract NAV_GROUPS into apps/web/src/lib/settings-nav.ts as factory
  function (getSettingsNavGroups) with NavItem/NavGroup types and helpers
  groupContainsPath/activeGroupLabel/findActiveNavEntry
- Replace static Settings footer NavLink in nav-sections.tsx with
  SettingsFooterItem that renders:
  • Expanded + /settings/*: SettingsInlineTree with accordion groups
  • Expanded + other routes: plain NavLink unchanged
  • Collapsed (any route): SettingsCollapsedFlyout (Base UI Menu)
- Drop SettingsGroupedNav left column from settings.tsx; content is now
  full-width with a SettingsBreadcrumbBar (Settings › Group › Leaf)
  above the Outlet
- Add browser tests for inline tree active state, flyout open/content,
  and breadcrumb segments across four settings sub-routes
- Add --no-sandbox to Playwright launchOptions for container environments

Closes #1104

Co-authored-by: Cursor <cursoragent@cursor.com>
dev requested review from reviewer 2026-05-11 16:41:55 +00:00
reviewer requested changes 2026-05-11 16:47:43 +00:00
Dismissed
reviewer left a comment
Copy link

Review — PR #1109 feat(settings): inline settings tree in sidenav + flyout on collapsed rail

CI: ❌ failing — see below for root cause.

The overall architecture is clean: extracting NAV_GROUPS into a shared factory, the three-mode SettingsFooterItem switch, the Base UI Menu flyout, and the breadcrumb-only settings layout are all sound. One critical bug is causing CI to fail; a few AC gaps need resolution before merge.

🔴 Critical — fix required (CI failure root cause)

isRoot reference-equality bug in SettingsBreadcrumbBar (settings.tsx)

const groups = getSettingsNavGroups();            // call A — fresh objects
const active = findActiveNavEntry(pathname);       // internally calls getSettingsNavGroups() — call B — different objects

const group = active?.group ?? groups[0];          // active.group is from call B
const isRoot = item === group.items[0] && group === groups[0];
//                                         ↑ active.group (call B) === groups[0] (call A) → ALWAYS false

getSettingsNavGroups() returns new object literals on every invocation. group === groups[0] compares instances from two separate calls — it is always false. As a result, isRoot is always false, so /settings/ renders the three-level else-branch (Settings › General (link) › General (text)) instead of the intended two-level form (Settings › General). The test "root '/settings/' shows Settings › General (two levels)" expects settings-breadcrumb-group to be null — it won't be — this is almost certainly why CI is red.

Fix — either pass groups into findActiveNavEntry so both share the same call's references:

// settings-nav.ts
export function findActiveNavEntry(
  pathname: string,
  groups = getSettingsNavGroups(),
): { group: NavGroup; item: NavItem } | null { ... }

// SettingsBreadcrumbBar
const groups = getSettingsNavGroups();
const active = findActiveNavEntry(pathname, groups);  // same array → === works

Or compare by value instead of reference:

const isRoot = item.to === group.items[0].to && group.label === groups[0].label;

🟠 Should-fix — AC gaps

1. Double breadcrumb on agents and service sub-routes

settings.agents.tsx and settings.service.tsx both render their own <SettingsBreadcrumb> (the existing ← Settings / <page> component). The new layout-level SettingsBreadcrumbBar in settings.tsx renders above all /settings/* outlets — including these two. On /settings/agents/sessions a user will see two stacked breadcrumbs.

The AC requires: "Sub-routes updated: drop any in-page SettingsSideNav that now duplicates the sidenav tree, OR keep it only where the sub-route has its own deeper sub-tree — document the decision in the PR description." The same logic applies to their breadcrumbs. The PR description doesn't mention this at all. The sub-route SettingsSideNav uses are clearly justified (deeper sub-trees not in NAV_GROUPS), but the per-sub-layout SettingsBreadcrumb is now superseded by the layout bar. Those should be dropped from settings.agents.tsx and settings.service.tsx, or the decision documented if intentional.

2. Raw <button> in SettingsInlineTree group toggles

apps/web/CLAUDE.md states: "Never reach for a raw <button>. The only exceptions are the close affordance inside <Drawer> itself, third-party widgets that own their own button (Base UI's <Switch>, etc.), and the Toasts dismiss surface." The accordion group toggle buttons in SettingsInlineTree don't fall under any exception. Use <Button variant="ghost"> (or a matching ghost/nav variant sized appropriately).

3. Flyout test missing leaf-click navigation assertion

The AC test requirement is: "collapsed + click Settings icon → flyout appears with leaves; leaf click navigates + closes flyout." The test verifies the flyout opens and contains leaves but stops there — it doesn't assert the flyout closes after a leaf click or that navigation occurs. The Base UI Menu does close on item activation, but an explicit assertion guards against future regressions.

🟡 Non-blocking observations

Collapsed rail active state — SettingsCollapsedFlyout's trigger button carries aria-[current=page]:bg-accent/15 CSS but nothing ever sets aria-current on it. The old NavLink received this attribute via TanStack's activeProps. Users in collapsed mode won't see the Settings icon highlighted when on a settings route. Consider wrapping the trigger in a TanStack <Link> with activeProps (matching the pattern used by other rail icons) so the icon highlights correctly.

useLocation({ select }) any-cast duplication — the same useLocation({ select: (loc: any) => loc.pathname as string }) pattern now appears in both nav-sections.tsx and settings.tsx. A shared usePathname() hook in @/lib/use-pathname.ts would eliminate the repeated cast and the repeated biome-ignore comment.

AC checklist status

AC item Status
NAV_GROUPS extracted to settings-nav.ts as factory ✅
NavGroup/NavItem types co-located ✅
settings.tsx no longer renders SettingsGroupedNav ✅
Inline tree — all groups/leaves from shared data ✅
Inline tree — active leaf highlighted ✅
Non-settings route → plain link ✅
Group accordions default expanded, toggle on click ✅
Collapsed: Base UI Menu flyout, no new dependency ✅
Tooltip doesn't double-fire when flyout is open ✅
Content full-width, breadcrumb header added ✅
Breadcrumb segments are links ✅
--no-sandbox flags for Playwright in containers ✅
i18n keys for breadcrumb (en + fr) ✅
isRoot two-level breadcrumb at /settings/ ❌ bug
Sub-route breadcrumb duplication documented/resolved ❌ missing
Group toggles use <Button> primitive ❌ raw <button>
Flyout leaf-click navigation + close tested ❌ partial test
Mobile drawer inline tree (behaviour works via shared NavSections, but untested) ⚠️ no test
## Review — PR #1109 `feat(settings): inline settings tree in sidenav + flyout on collapsed rail` **CI: ❌ failing** — see below for root cause. The overall architecture is clean: extracting `NAV_GROUPS` into a shared factory, the three-mode `SettingsFooterItem` switch, the Base UI Menu flyout, and the breadcrumb-only settings layout are all sound. One critical bug is causing CI to fail; a few AC gaps need resolution before merge. --- ### 🔴 Critical — fix required (CI failure root cause) #### `isRoot` reference-equality bug in `SettingsBreadcrumbBar` (`settings.tsx`) ```ts const groups = getSettingsNavGroups(); // call A — fresh objects const active = findActiveNavEntry(pathname); // internally calls getSettingsNavGroups() — call B — different objects const group = active?.group ?? groups[0]; // active.group is from call B const isRoot = item === group.items[0] && group === groups[0]; // ↑ active.group (call B) === groups[0] (call A) → ALWAYS false ``` `getSettingsNavGroups()` returns new object literals on every invocation. `group === groups[0]` compares instances from two separate calls — it is always `false`. As a result, `isRoot` is always `false`, so `/settings/` renders the three-level else-branch (`Settings › General (link) › General (text)`) instead of the intended two-level form (`Settings › General`). The test `"root '/settings/' shows Settings › General (two levels)"` expects `settings-breadcrumb-group` to be null — it won't be — **this is almost certainly why CI is red**. **Fix** — either pass `groups` into `findActiveNavEntry` so both share the same call's references: ```ts // settings-nav.ts export function findActiveNavEntry( pathname: string, groups = getSettingsNavGroups(), ): { group: NavGroup; item: NavItem } | null { ... } ``` ```ts // SettingsBreadcrumbBar const groups = getSettingsNavGroups(); const active = findActiveNavEntry(pathname, groups); // same array → === works ``` Or compare by value instead of reference: ```ts const isRoot = item.to === group.items[0].to && group.label === groups[0].label; ``` --- ### 🟠 Should-fix — AC gaps #### 1. Double breadcrumb on agents and service sub-routes `settings.agents.tsx` and `settings.service.tsx` both render their own `<SettingsBreadcrumb>` (the existing `← Settings / <page>` component). The new layout-level `SettingsBreadcrumbBar` in `settings.tsx` renders above **all** `/settings/*` outlets — including these two. On `/settings/agents/sessions` a user will see two stacked breadcrumbs. The AC requires: *"Sub-routes updated: drop any in-page SettingsSideNav that now duplicates the sidenav tree, OR keep it only where the sub-route has its own deeper sub-tree — document the decision in the PR description."* The same logic applies to their breadcrumbs. The PR description doesn't mention this at all. The sub-route `SettingsSideNav` uses are clearly justified (deeper sub-trees not in `NAV_GROUPS`), but the per-sub-layout `SettingsBreadcrumb` is now superseded by the layout bar. Those should be dropped from `settings.agents.tsx` and `settings.service.tsx`, or the decision documented if intentional. #### 2. Raw `<button>` in `SettingsInlineTree` group toggles `apps/web/CLAUDE.md` states: *"Never reach for a raw `<button>`. The only exceptions are the close affordance inside `<Drawer>` itself, third-party widgets that own their own button (Base UI's `<Switch>`, etc.), and the Toasts dismiss surface."* The accordion group toggle buttons in `SettingsInlineTree` don't fall under any exception. Use `<Button variant="ghost">` (or a matching `ghost`/`nav` variant sized appropriately). #### 3. Flyout test missing leaf-click navigation assertion The AC test requirement is: *"collapsed + click Settings icon → flyout appears with leaves; **leaf click navigates + closes flyout**."* The test verifies the flyout opens and contains leaves but stops there — it doesn't assert the flyout closes after a leaf click or that navigation occurs. The Base UI Menu does close on item activation, but an explicit assertion guards against future regressions. --- ### 🟡 Non-blocking observations **Collapsed rail active state** — `SettingsCollapsedFlyout`'s trigger button carries `aria-[current=page]:bg-accent/15` CSS but nothing ever sets `aria-current` on it. The old `NavLink` received this attribute via TanStack's `activeProps`. Users in collapsed mode won't see the Settings icon highlighted when on a settings route. Consider wrapping the trigger in a TanStack `<Link>` with `activeProps` (matching the pattern used by other rail icons) so the icon highlights correctly. **`useLocation({ select })` any-cast duplication** — the same `useLocation({ select: (loc: any) => loc.pathname as string })` pattern now appears in both `nav-sections.tsx` and `settings.tsx`. A shared `usePathname()` hook in `@/lib/use-pathname.ts` would eliminate the repeated cast and the repeated `biome-ignore` comment. --- ### AC checklist status | AC item | Status | |---|---| | `NAV_GROUPS` extracted to `settings-nav.ts` as factory | ✅ | | `NavGroup`/`NavItem` types co-located | ✅ | | `settings.tsx` no longer renders `SettingsGroupedNav` | ✅ | | Inline tree — all groups/leaves from shared data | ✅ | | Inline tree — active leaf highlighted | ✅ | | Non-settings route → plain link | ✅ | | Group accordions default expanded, toggle on click | ✅ | | Collapsed: Base UI Menu flyout, no new dependency | ✅ | | Tooltip doesn't double-fire when flyout is open | ✅ | | Content full-width, breadcrumb header added | ✅ | | Breadcrumb segments are links | ✅ | | `--no-sandbox` flags for Playwright in containers | ✅ | | i18n keys for breadcrumb (en + fr) | ✅ | | isRoot two-level breadcrumb at `/settings/` | ❌ bug | | Sub-route breadcrumb duplication documented/resolved | ❌ missing | | Group toggles use `<Button>` primitive | ❌ raw `<button>` | | Flyout leaf-click navigation + close tested | ❌ partial test | | Mobile drawer inline tree (behaviour works via shared `NavSections`, but untested) | ⚠️ no test |
fix(settings): address reviewer feedback on #1104
Some checks failed
qa / sql-layer-check (pull_request) Successful in 12s
Details
qa / db-schema (pull_request) Successful in 13s
Details
qa / i18n-string-check (pull_request) Successful in 22s
Details
qa / dockerfile (pull_request) Successful in 22s
Details
qa / qa-1 (pull_request) Failing after 2m36s
Details
qa / qa (pull_request) Failing after 0s
Details
a349513a18 
- Fix isRoot value-equality bug in SettingsBreadcrumbBar: compare
  item.to/group.label instead of object identity (two getSettingsNavGroups()
  calls produce different instances, making === always false).  Root
  '/settings/' now correctly renders two-level breadcrumb.
- Drop SettingsBreadcrumb back-link from settings.agents.tsx and
  settings.service.tsx; the layout-level SettingsBreadcrumbBar supersedes it
  (SettingsSideNav sub-trees are kept — they cover deeper paths not in NAV_GROUPS).
- Replace raw <button> in SettingsInlineTree group accordions with
  <Button variant="ghost"> per the CLAUDE.md "no raw button" rule.
- Add flyout leaf-click closes-flyout test to sidebar-nav.test.tsx.

Remaining leaf-route double-breadcrumb (appearance, repos, etc.) is a
follow-up in milestone #36 — those routes have tests asserting the old
breadcrumb testIds which would need migrating.

Co-authored-by: Cursor <cursoragent@cursor.com>
dev reviewed 2026-05-11 16:57:32 +00:00
dev left a comment
Copy link

Changes addressing reviewer feedback

All four issues from the review have been addressed in commit a349513a:

🔴 Critical — isRoot reference-equality bug (fixed)

SettingsBreadcrumbBar now compares item.to === group.items[0]?.to && group.label === groups[0]?.label instead of object identity. Since getSettingsNavGroups() returns new object literals on each call, the === comparison was always false. /settings/ now correctly renders the two-level breadcrumb.

🟠 Double breadcrumb on agents and service sub-routes (fixed)

Dropped <SettingsBreadcrumb> from settings.agents.tsx and settings.service.tsx. The SettingsSideNav sub-trees are preserved — they cover deeper paths not in NAV_GROUPS. Note: the leaf routes (appearance, repos, labels, secrets, voice-input, language, agent-types) have the same double-breadcrumb pattern but have existing tests asserting the old breadcrumb test IDs. Those are deferred to milestone #36.

🟠 Raw <button> in SettingsInlineTree accordions (fixed)

Replaced with <Button variant="ghost" size="sm"> per the CLAUDE.md convention.

🟠 Flyout leaf-click test incomplete (fixed)

Added a third flyout test: "leaf click closes the flyout" — opens the flyout, clicks snav-appearance, then asserts settings-flyout is no longer in the DOM.

## Changes addressing reviewer feedback All four issues from the review have been addressed in commit `a349513a`: ### 🔴 Critical — isRoot reference-equality bug (fixed) `SettingsBreadcrumbBar` now compares `item.to === group.items[0]?.to && group.label === groups[0]?.label` instead of object identity. Since `getSettingsNavGroups()` returns new object literals on each call, the `===` comparison was always `false`. `/settings/` now correctly renders the two-level breadcrumb. ### 🟠 Double breadcrumb on agents and service sub-routes (fixed) Dropped `<SettingsBreadcrumb>` from `settings.agents.tsx` and `settings.service.tsx`. The `SettingsSideNav` sub-trees are preserved — they cover deeper paths not in `NAV_GROUPS`. Note: the leaf routes (`appearance`, `repos`, `labels`, `secrets`, `voice-input`, `language`, `agent-types`) have the same double-breadcrumb pattern but have existing tests asserting the old breadcrumb test IDs. Those are deferred to milestone #36. ### 🟠 Raw `<button>` in SettingsInlineTree accordions (fixed) Replaced with `<Button variant="ghost" size="sm">` per the CLAUDE.md convention. ### 🟠 Flyout leaf-click test incomplete (fixed) Added a third flyout test: `"leaf click closes the flyout"` — opens the flyout, clicks `snav-appearance`, then asserts `settings-flyout` is no longer in the DOM.
dev requested review from reviewer 2026-05-11 17:00:21 +00:00
reviewer requested changes 2026-05-11 17:08:29 +00:00
Dismissed
reviewer left a comment
Copy link

Review — PR #1109

CI is red and there's a behavioural gap in collapsed mode. Two changes needed before merge; the rest are minor.

🔴 Blocker 1 — CI is failing

The aggregate CI status for head SHA a349513 is failure. Must be green before merge. Please dig into the run log at https://forge.jacquin.app/charles/claude-hooks/actions/runs/1944 and share what's failing. A few candidates from reading the diff:

  • The flyout test drives btn.click() on a <button> rendered through <Menu.Trigger render={…}> wrapped in <Tooltip>. Depending on how the Tooltip component forward refs / clones its child, the Menu.Trigger's synthesised onClick might not reach the DOM button in the JSDOM/Playwright environment. If the Tooltip adds a wrapper element, getByTestId("nav-settings") still finds the button correctly, but the synthetic click fired by Base UI may not register without a proper pointer event sequence. Worth checking whether userEvent.click(btn) from @testing-library/user-event is needed instead of the raw .click() used in the other two flyout tests.
  • getSettingsNavGroups() is called at render time inside SettingsBreadcrumbBar and SettingsInlineTree. If the Paraglide runtime isn't fully initialised in the settings.test.tsx environment for the new file path, message calls return the key string rather than the translated value — the breadcrumb text-content assertions (toContain("General"), toContain("Forges")) would then fail.

🔴 Blocker 2 — Collapsed rail: Settings icon never shows active state

SettingsCollapsedFlyout renders a plain <button> — not a TanStack <Link>. It carries the aria-[current=page]:… classes but nothing ever sets aria-current="page" on it, so the icon stays visually inert on all /settings/* routes in collapsed mode.

Every other collapsed icon (board, flows, agents-live, etc.) goes through NavLink which wraps a TanStack <Link> and gets aria-current="page" via activeProps automatically. Visiting /settings/appearance with the rail collapsed now produces a rail where nothing is highlighted — a regression from the pre-PR state where the Settings link carried the active style via NavLink.

Fix: derive isOnSettings (same logic already in SettingsFooterItem) and conditionally apply the active class, or pass an aria-current prop directly:

// in SettingsCollapsedFlyout
const pathname = useLocation({ select: (loc: any) => loc.pathname as string });
const isActive = typeof pathname === "string" && (pathname === "/settings" || pathname.startsWith("/settings/"));

<button
  ...
  aria-current={isActive ? "page" : undefined}
  className={cn(
    "...",
    "aria-[current=page]:bg-accent/15 aria-[current=page]:text-text-primary",
    // collapsed bar cue — match the ::before rule from NavLink collapsed
    isActive && "before:absolute before:top-1.5 before:bottom-1.5 before:left-0 before:w-1 before:rounded-pill before:bg-accent before:content-['']",
  )}
>

🟡 AC gap — sub-route SettingsSideNav decision undocumented

The AC requires: "document the decision in the PR description" about whether per-page SettingsSideNav usage is kept or dropped.

From the diff, settings.agents.tsx and settings.service.tsx both keep their own SettingsSideNav for their deeper sub-trees (agent types / per-agent secrets / config history / sessions / admin; service forge / ai-providers / container / watchdogs / design). That's the right call — these sub-trees are not in the top-level NAV_GROUPS — but the PR description doesn't mention it. Please add a sentence or two explaining the decision so future reviewers understand the two-level nav isn't accidental.

🟡 Null-safety: groupFirstItem in SettingsBreadcrumbBar

// settings.tsx
const groupFirstItem = group.items[0];   // NavItem | undefined if noUncheckedIndexedAccess
...
to={groupFirstItem.to as any}            // potential runtime throw if items is empty

NavGroup.items is ReadonlyArray<NavItem>. Index access on a ReadonlyArray returns NavItem | undefined under noUncheckedIndexedAccess. Even without that flag, an empty group at runtime would throw here. Given that you guard !group || !item above, also guard !groupFirstItem or use groupFirstItem?.to ?? group.items[0]?.to ?? "/settings/":

const groupFirstItem = group.items[0];
if (!group || !item || !groupFirstItem) return null;

✅ What's good

  • Single source of truth — getSettingsNavGroups() in settings-nav.ts is cleanly separated as a factory (not a module-level constant), which correctly defers Paraglide locale resolution to render time. The comment explaining why is appreciated.
  • Flyout tooltip guard — {!open ? <Tooltip>…</Tooltip> : trigger} correctly prevents the tooltip from firing while the flyout is open.
  • Inline tree open-all default — new Set(groups.map(g => g.label)) is a clean way to start all groups expanded without any state initialisation logic.
  • Test coverage — good breadth across the three modes (expanded-on-settings, expanded-elsewhere, collapsed). The currentPath module-level variable approach with beforeEach/afterEach resets is sound.
  • i18n — both en.json and fr.json updated in the same commit. ✓
  • --no-sandbox Playwright flags — correct fix for containerised CI; the comment explaining why is clear.
  • Breadcrumb logic — the isRoot condition (item.to === group.items[0]?.to && group.label === groups[0]?.label) correctly produces two-level breadcrumb only at /settings/ and three-level everywhere else.

Summary: Two blocking items (CI red; collapsed active state regression) and two non-blocking clean-ups. Happy to re-review once CI is green and the active indicator is wired up.

## Review — PR #1109 CI is red and there's a behavioural gap in collapsed mode. Two changes needed before merge; the rest are minor. --- ### 🔴 Blocker 1 — CI is failing The aggregate CI status for head SHA `a349513` is `failure`. Must be green before merge. Please dig into the run log at https://forge.jacquin.app/charles/claude-hooks/actions/runs/1944 and share what's failing. A few candidates from reading the diff: - The flyout test drives `btn.click()` on a `<button>` rendered through `<Menu.Trigger render={…}>` **wrapped in `<Tooltip>`**. Depending on how the Tooltip component forward refs / clones its child, the Menu.Trigger's synthesised `onClick` might not reach the DOM button in the JSDOM/Playwright environment. If the Tooltip adds a wrapper element, `getByTestId("nav-settings")` still finds the button correctly, but the synthetic click fired by Base UI may not register without a proper pointer event sequence. Worth checking whether `userEvent.click(btn)` from `@testing-library/user-event` is needed instead of the raw `.click()` used in the other two flyout tests. - `getSettingsNavGroups()` is called at render time inside `SettingsBreadcrumbBar` and `SettingsInlineTree`. If the Paraglide runtime isn't fully initialised in the `settings.test.tsx` environment for the new file path, message calls return the key string rather than the translated value — the breadcrumb text-content assertions (`toContain("General")`, `toContain("Forges")`) would then fail. --- ### 🔴 Blocker 2 — Collapsed rail: Settings icon never shows active state `SettingsCollapsedFlyout` renders a plain `<button>` — not a TanStack `<Link>`. It carries the `aria-[current=page]:…` classes but nothing ever sets `aria-current="page"` on it, so the icon stays visually inert on all `/settings/*` routes in collapsed mode. Every other collapsed icon (board, flows, agents-live, etc.) goes through `NavLink` which wraps a TanStack `<Link>` and gets `aria-current="page"` via `activeProps` automatically. Visiting `/settings/appearance` with the rail collapsed now produces a rail where nothing is highlighted — a regression from the pre-PR state where the Settings link carried the active style via `NavLink`. Fix: derive `isOnSettings` (same logic already in `SettingsFooterItem`) and conditionally apply the active class, or pass an `aria-current` prop directly: ```tsx // in SettingsCollapsedFlyout const pathname = useLocation({ select: (loc: any) => loc.pathname as string }); const isActive = typeof pathname === "string" && (pathname === "/settings" || pathname.startsWith("/settings/")); <button ... aria-current={isActive ? "page" : undefined} className={cn( "...", "aria-[current=page]:bg-accent/15 aria-[current=page]:text-text-primary", // collapsed bar cue — match the ::before rule from NavLink collapsed isActive && "before:absolute before:top-1.5 before:bottom-1.5 before:left-0 before:w-1 before:rounded-pill before:bg-accent before:content-['']", )} > ``` --- ### 🟡 AC gap — sub-route `SettingsSideNav` decision undocumented The AC requires: *"document the decision in the PR description"* about whether per-page `SettingsSideNav` usage is kept or dropped. From the diff, `settings.agents.tsx` and `settings.service.tsx` both keep their own `SettingsSideNav` for their deeper sub-trees (agent types / per-agent secrets / config history / sessions / admin; service forge / ai-providers / container / watchdogs / design). That's the right call — these sub-trees are not in the top-level `NAV_GROUPS` — but the PR description doesn't mention it. Please add a sentence or two explaining the decision so future reviewers understand the two-level nav isn't accidental. --- ### 🟡 Null-safety: `groupFirstItem` in `SettingsBreadcrumbBar` ```ts // settings.tsx const groupFirstItem = group.items[0]; // NavItem | undefined if noUncheckedIndexedAccess ... to={groupFirstItem.to as any} // potential runtime throw if items is empty ``` `NavGroup.items` is `ReadonlyArray<NavItem>`. Index access on a `ReadonlyArray` returns `NavItem | undefined` under `noUncheckedIndexedAccess`. Even without that flag, an empty group at runtime would throw here. Given that you guard `!group || !item` above, also guard `!groupFirstItem` or use `groupFirstItem?.to ?? group.items[0]?.to ?? "/settings/"`: ```ts const groupFirstItem = group.items[0]; if (!group || !item || !groupFirstItem) return null; ``` --- ### ✅ What's good - **Single source of truth** — `getSettingsNavGroups()` in `settings-nav.ts` is cleanly separated as a factory (not a module-level constant), which correctly defers Paraglide locale resolution to render time. The comment explaining why is appreciated. - **Flyout tooltip guard** — `{!open ? <Tooltip>…</Tooltip> : trigger}` correctly prevents the tooltip from firing while the flyout is open. - **Inline tree open-all default** — `new Set(groups.map(g => g.label))` is a clean way to start all groups expanded without any state initialisation logic. - **Test coverage** — good breadth across the three modes (expanded-on-settings, expanded-elsewhere, collapsed). The `currentPath` module-level variable approach with `beforeEach`/`afterEach` resets is sound. - **i18n** — both `en.json` and `fr.json` updated in the same commit. ✓ - **`--no-sandbox` Playwright flags** — correct fix for containerised CI; the comment explaining why is clear. - **Breadcrumb logic** — the `isRoot` condition (`item.to === group.items[0]?.to && group.label === groups[0]?.label`) correctly produces two-level breadcrumb only at `/settings/` and three-level everywhere else. --- **Summary:** Two blocking items (CI red; collapsed active state regression) and two non-blocking clean-ups. Happy to re-review once CI is green and the active indicator is wired up.
fix(ci): exempt new sidenav/breadcrumb testids from settings-manifest drift lint
Some checks failed
qa / dockerfile (pull_request) Successful in 10s
Details
qa / i18n-string-check (pull_request) Successful in 12s
Details
qa / db-schema (pull_request) Successful in 17s
Details
qa / sql-layer-check (pull_request) Successful in 22s
Details
qa / qa-1 (pull_request) Failing after 4m11s
Details
qa / qa (pull_request) Failing after 0s
Details
6f956dab79 
The #1104 commit added six new data-testid="settings-*" attributes
(settings-inline-tree, settings-flyout, settings-breadcrumb,
settings-breadcrumb-root, settings-breadcrumb-leaf,
settings-breadcrumb-group) but did not register them in
NON_FIELD_SETTINGS_TESTIDS. The settings-manifest drift-lint test
(run via bun test, not vitest browser mode) caught this and failed CI.

Add the six IDs to NON_FIELD_SETTINGS_TESTIDS — they are structural
nav chrome, not addressable settings fields.

Co-authored-by: Cursor <cursoragent@cursor.com>
dev requested review from reviewer 2026-05-11 17:16:32 +00:00
reviewer requested changes 2026-05-11 17:19:29 +00:00
Dismissed
reviewer left a comment
Copy link

Review — feat(settings): inline settings tree in sidenav + flyout on collapsed rail

The architecture is solid and the three-mode pattern (collapsed flyout / expanded-on-settings inline tree / expanded-other plain link) is clean and well-documented. The extraction of settings-nav.ts as a locale-safe factory is the right call. One critical bug blocks merge; several smaller issues need addressing.

🔴 Critical — Double breadcrumb on most settings pages

SettingsBreadcrumbBar is injected at the layout level (settings.tsx → <Outlet/>), so it now appears above every /settings/* child page. But the old per-page SettingsBreadcrumb component was only removed from two routes (settings.agents.tsx, settings.service.tsx). Eight others still render their own breadcrumb inside the page content:

  • settings.agent-types.tsx — <SettingsBreadcrumb current="Agent types" />
  • settings.appearance.tsx — <SettingsBreadcrumb current={m.settings_appearance_heading()} />
  • settings.language.tsx — <SettingsBreadcrumb current={m.settings_language_heading()} />
  • settings.labels.tsx — <SettingsBreadcrumb current="Labels" />
  • settings.repos.tsx — <SettingsBreadcrumb current="Repositories" />
  • settings.secrets.tsx — <SettingsBreadcrumb current="Secrets" />
  • settings.voice-input.tsx — <SettingsBreadcrumb current="Voice input" />
  • features/service-config/ai-provider-keys-section.tsx — <SettingsBreadcrumb current="AI providers" />

Users on those pages will see two stacked breadcrumbs. Remove SettingsBreadcrumb from all of them (the component itself can be deleted once no callers remain, or kept for other uses if there are any).

🟠 Convention violations

1. Raw <button> in SettingsCollapsedFlyout (nav-sections.tsx)

const button = (
  <button type="button" aria-label={label} …>
    <Settings size={16} aria-hidden />
  </button>
);
const trigger = <Menu.Trigger render={button} />;

apps/web/CLAUDE.md is explicit: "Never reach for a raw <button>. The only exceptions are the close affordance inside <Drawer> itself, third-party widgets that own their own button (<Switch>, etc.), and the Toasts dismiss surface."

Menu.Trigger accepts a render prop — pass <Button variant="ghost" size="icon" …> the same way SettingsInlineTree uses <Button variant="ghost" size="sm"> for the group toggles. This keeps styling consistent too (the icon button used in collapsed mode currently re-implements hover/focus classes by hand instead of inheriting them from the primitive).

2. Unicode breadcrumb separator › (settings.tsx)

<span aria-hidden="true" className="text-text-dim">›</span>

apps/web/CLAUDE.md: "Never paste a Unicode glyph into a JSX expression — pick the matching lucide icon." Use <ChevronRight size={12} aria-hidden className="text-text-dim" /> (already imported in related components; ChevronDown is imported in this PR already).

3. Test clicks use raw .element().click() on enabled elements (sidebar-nav.test.tsx)

const btn = screen.getByTestId("nav-settings").element() as HTMLButtonElement;
btn.click();

Per apps/web/CLAUDE.md, .element().click() is the escape hatch only for disabled controls that Playwright refuses to drive. For enabled elements, use the locator API:

await screen.getByTestId("nav-settings").click();

This applies to all three flyout tests.

🟡 Quality issues

4. SettingsInlineTree opens all groups unconditionally

const [openGroups, setOpenGroups] = useState<ReadonlySet<string>>(
  () => new Set(groups.map((g) => g.label))
);

The removed SettingsGroupedNav was smarter — it seeded the initial open set with General + the group containing the active route. Opening every group by default is noisier as the tree grows. Consider seeding with just the active group (falling back to groups[0] when none matches), mirroring the old behaviour:

() => {
  const initial = new Set([groups[0]?.label ?? ""]);
  const active = activeGroupLabel(pathname);
  if (active) initial.add(active);
  return initial;
}

Note this also requires reading pathname inside the initializer — useLocation should be called before useState.

5. getSettingsNavGroups() called twice in SettingsBreadcrumbBar

const groups = getSettingsNavGroups();                     // call 1
const active = findActiveNavEntry(…);                      // call 2 — internally calls getSettingsNavGroups() again

findActiveNavEntry already accepts a pathname and returns the full { group, item } pair. Either pass groups into it (add an optional second arg) or derive group from active without the top-level getSettingsNavGroups() call. Minor perf, but avoids rebuilding the array twice per render.

6. Group toggle buttons lack an accessible label (SettingsInlineTree)

<Button … onClick={() => toggleGroup(group.label)} aria-expanded={isOpen}>
  <span>{group.label}</span>
  <ChevronDown … />
</Button>

The button's accessible name comes from the visible <span> text, which reads as e.g. "General" — screen readers won't know it's a toggle. Add aria-label={Toggle ${group.label} group} (or aria-controls pointing at the item list id) so the affordance is unambiguous.

🟢 Good

  • settings-nav.ts factory pattern correctly avoids frozen-locale strings at module init time.
  • The biome-ignore annotations are all justified and consistently explained.
  • --no-sandbox / --disable-setuid-sandbox addition is well-commented and appropriate for containerised CI.
  • i18n keys added in both en.json and fr.json. ✅
  • NON_FIELD_SETTINGS_TESTIDS updated correctly in settings-manifest.ts. ✅
  • test-router-stub.tsx useLocation upgrade is clean and backward-compatible. ✅
  • Test coverage for inline tree active state and flyout content is thorough. ✅

Summary of required changes before merge:

  1. Remove <SettingsBreadcrumb> from all 8 remaining settings pages (and ai-provider-keys-section.tsx).
  2. Replace raw <button> in SettingsCollapsedFlyout with <Button>.
  3. Replace Unicode › separators with <ChevronRight>.
  4. Convert .element().click() flyout test calls to locator .click().
  5. (Recommended) Seed SettingsInlineTree open-groups from the active route rather than opening all.
## Review — `feat(settings): inline settings tree in sidenav + flyout on collapsed rail` The architecture is solid and the three-mode pattern (`collapsed flyout / expanded-on-settings inline tree / expanded-other plain link`) is clean and well-documented. The extraction of `settings-nav.ts` as a locale-safe factory is the right call. One **critical bug** blocks merge; several smaller issues need addressing. --- ### 🔴 Critical — Double breadcrumb on most settings pages `SettingsBreadcrumbBar` is injected at the **layout level** (`settings.tsx` → `<Outlet/>`), so it now appears above every `/settings/*` child page. But the old per-page `SettingsBreadcrumb` component was only removed from **two** routes (`settings.agents.tsx`, `settings.service.tsx`). Eight others still render their own breadcrumb inside the page content: - `settings.agent-types.tsx` — `<SettingsBreadcrumb current="Agent types" />` - `settings.appearance.tsx` — `<SettingsBreadcrumb current={m.settings_appearance_heading()} />` - `settings.language.tsx` — `<SettingsBreadcrumb current={m.settings_language_heading()} />` - `settings.labels.tsx` — `<SettingsBreadcrumb current="Labels" />` - `settings.repos.tsx` — `<SettingsBreadcrumb current="Repositories" />` - `settings.secrets.tsx` — `<SettingsBreadcrumb current="Secrets" />` - `settings.voice-input.tsx` — `<SettingsBreadcrumb current="Voice input" />` - `features/service-config/ai-provider-keys-section.tsx` — `<SettingsBreadcrumb current="AI providers" />` Users on those pages will see two stacked breadcrumbs. Remove `SettingsBreadcrumb` from all of them (the component itself can be deleted once no callers remain, or kept for other uses if there are any). --- ### 🟠 Convention violations **1. Raw `<button>` in `SettingsCollapsedFlyout`** (`nav-sections.tsx`) ```tsx const button = ( <button type="button" aria-label={label} …> <Settings size={16} aria-hidden /> </button> ); const trigger = <Menu.Trigger render={button} />; ``` `apps/web/CLAUDE.md` is explicit: _"Never reach for a raw `<button>`. The only exceptions are the close affordance inside `<Drawer>` itself, third-party widgets that own their own button (`<Switch>`, etc.), and the Toasts dismiss surface."_ `Menu.Trigger` accepts a `render` prop — pass `<Button variant="ghost" size="icon" …>` the same way `SettingsInlineTree` uses `<Button variant="ghost" size="sm">` for the group toggles. This keeps styling consistent too (the icon button used in collapsed mode currently re-implements hover/focus classes by hand instead of inheriting them from the primitive). **2. Unicode breadcrumb separator `›`** (`settings.tsx`) ```tsx <span aria-hidden="true" className="text-text-dim">›</span> ``` `apps/web/CLAUDE.md`: _"Never paste a Unicode glyph into a JSX expression — pick the matching lucide icon."_ Use `<ChevronRight size={12} aria-hidden className="text-text-dim" />` (already imported in related components; `ChevronDown` is imported in this PR already). **3. Test clicks use raw `.element().click()` on enabled elements** (`sidebar-nav.test.tsx`) ```tsx const btn = screen.getByTestId("nav-settings").element() as HTMLButtonElement; btn.click(); ``` Per `apps/web/CLAUDE.md`, `.element().click()` is the escape hatch **only for disabled controls** that Playwright refuses to drive. For enabled elements, use the locator API: ```tsx await screen.getByTestId("nav-settings").click(); ``` This applies to all three flyout tests. --- ### 🟡 Quality issues **4. `SettingsInlineTree` opens all groups unconditionally** ```tsx const [openGroups, setOpenGroups] = useState<ReadonlySet<string>>( () => new Set(groups.map((g) => g.label)) ); ``` The removed `SettingsGroupedNav` was smarter — it seeded the initial open set with General + the group containing the active route. Opening every group by default is noisier as the tree grows. Consider seeding with just the active group (falling back to `groups[0]` when none matches), mirroring the old behaviour: ```tsx () => { const initial = new Set([groups[0]?.label ?? ""]); const active = activeGroupLabel(pathname); if (active) initial.add(active); return initial; } ``` Note this also requires reading `pathname` inside the initializer — `useLocation` should be called before `useState`. **5. `getSettingsNavGroups()` called twice in `SettingsBreadcrumbBar`** ```tsx const groups = getSettingsNavGroups(); // call 1 const active = findActiveNavEntry(…); // call 2 — internally calls getSettingsNavGroups() again ``` `findActiveNavEntry` already accepts a `pathname` and returns the full `{ group, item }` pair. Either pass `groups` into it (add an optional second arg) or derive `group` from `active` without the top-level `getSettingsNavGroups()` call. Minor perf, but avoids rebuilding the array twice per render. **6. Group toggle buttons lack an accessible label** (`SettingsInlineTree`) ```tsx <Button … onClick={() => toggleGroup(group.label)} aria-expanded={isOpen}> <span>{group.label}</span> <ChevronDown … /> </Button> ``` The button's accessible name comes from the visible `<span>` text, which reads as e.g. "General" — screen readers won't know it's a toggle. Add `aria-label={`Toggle ${group.label} group`}` (or `aria-controls` pointing at the item list `id`) so the affordance is unambiguous. --- ### 🟢 Good - `settings-nav.ts` factory pattern correctly avoids frozen-locale strings at module init time. - The `biome-ignore` annotations are all justified and consistently explained. - `--no-sandbox` / `--disable-setuid-sandbox` addition is well-commented and appropriate for containerised CI. - i18n keys added in both `en.json` and `fr.json`. ✅ - `NON_FIELD_SETTINGS_TESTIDS` updated correctly in `settings-manifest.ts`. ✅ - `test-router-stub.tsx` `useLocation` upgrade is clean and backward-compatible. ✅ - Test coverage for inline tree active state and flyout content is thorough. ✅ --- **Summary of required changes before merge:** 1. Remove `<SettingsBreadcrumb>` from all 8 remaining settings pages (and `ai-provider-keys-section.tsx`). 2. Replace raw `<button>` in `SettingsCollapsedFlyout` with `<Button>`. 3. Replace Unicode `›` separators with `<ChevronRight>`. 4. Convert `.element().click()` flyout test calls to locator `.click()`. 5. (Recommended) Seed `SettingsInlineTree` open-groups from the active route rather than opening all.
charles force-pushed dev/1104 from 6f956dab79
Some checks failed
qa / dockerfile (pull_request) Successful in 10s
Details
qa / i18n-string-check (pull_request) Successful in 12s
Details
qa / db-schema (pull_request) Successful in 17s
Details
qa / sql-layer-check (pull_request) Successful in 22s
Details
qa / qa-1 (pull_request) Failing after 4m11s
Details
qa / qa (pull_request) Failing after 0s
Details
to 4d5e8d79a2
All checks were successful
qa / sql-layer-check (pull_request) Successful in 11s
Details
qa / i18n-string-check (pull_request) Successful in 15s
Details
qa / dockerfile (pull_request) Successful in 16s
Details
qa / db-schema (pull_request) Successful in 19s
Details
qa / qa-1 (pull_request) Successful in 3m40s
Details
qa / qa (pull_request) Successful in 0s
Details
2026-05-11 18:08:51 +00:00 Compare
dev requested review from reviewer 2026-05-11 18:14:52 +00:00
reviewer approved these changes 2026-05-11 18:19:37 +00:00
reviewer left a comment
Copy link

Review — PR #1109 feat(settings): inline settings tree in sidenav + flyout on collapsed rail

CI: ✅ green  |  Mergeable: ✅  |  Verdict: ✅ APPROVED

AC coverage

AC Status Notes
NAV_GROUPS extracted to settings-nav.ts ✅ Factory getSettingsNavGroups() — locale-reactive, not a frozen constant
NavGroup / NavItem types co-located ✅
All consumers import from shared module ✅ / ⚠️ See note below on SettingsSideNav
Inline tree on /settings/* + expanded ✅ SettingsInlineTree with all 4 groups
Active leaf highlighted ✅ activeProps + aria-[current=page]:* CSS
Non-settings route → plain link ✅ SettingsFooterItem branch on isOnSettings
Group accordions toggle; default expanded; chevron ✅ openGroups state seeded to all labels
Collapsed rail → Base UI Menu flyout ✅ SettingsCollapsedFlyout using @base-ui-components/react/menu
Flyout positioned right of rail icon ✅ side="right" sideOffset={8}
Tooltip does not double-fire when flyout open ✅ !open ? <Tooltip>…</Tooltip> : trigger
Leaf click closes flyout ✅ Menu.Item render={<Link>} merges Base UI's close handler; confirmed by the "leaf click closes the flyout" browser test
SettingsGroupedNav dropped from settings.tsx ✅
Full-width content layout ✅ sm:flex-row dropped; single flex column
Breadcrumb header Settings › Group › Leaf ✅ SettingsBreadcrumbBar — correct 2-level at root, 3-level elsewhere
Breadcrumb segments are links ✅ Root → /settings/; group → first leaf of group
Old SettingsBreadcrumb component deleted ✅ All 9 consumers cleaned up
settings-manifest.ts updated ✅ 4 new testIds registered in NON_FIELD_SETTINGS_TESTIDS
i18n (en + fr) ✅ settings_breadcrumb_aria_label + settings_breadcrumb_root
Mobile drawer inline tree preserved ✅ SettingsFooterItem is in NavSections which feeds the drawer; collapsed=false in drawer → inline tree path
vitest.config.ts — --no-sandbox ✅ Container-safe Playwright launch
turbo.json — passThroughEnv ✅ CHROMIUM_PATH + PLAYWRIGHT_BROWSERS_PATH visible to test task

Test coverage

  • sidebar-nav.test.tsx: inline tree on /settings/appearance ✅; Appearance leaf active ✅; other leaves not active ✅; all 9 testIds present ✅; no tree on /board ✅; plain link present on non-settings route ✅; collapsed → flyout opens ✅; flyout contains leaves ✅; leaf click closes flyout ✅
  • settings.test.tsx: settings-side-nav absent from DOM ✅; outlet visible ✅; breadcrumb at root shows 2-level ✅; /settings/appearance → General › Appearance ✅; /settings/service → Forges › Service ✅; /settings/secrets → Data › Secrets ✅

Notes (non-blocking)

SettingsSideNav not updated to import from settings-nav.ts
The AC says "all current consumers import from the shared module." settings-side-nav.tsx isn't touched. This is intentional and justified: the SettingsSideNav component serves deeper sub-trees inside the agents and service-config pages (agent-types / instance tabs, container / AI-provider sections) — these items are not in NAV_GROUPS at all, so there's nothing to import. The AC was written before that distinction was clear. Decision documented in the PR description ✓. No action needed unless #36 later moves those sub-trees into the shared manifest.

Redundant active-state application in inline tree leaves
Each leaf <Link> carries both aria-[current=page]:bg-accent/15 aria-[current=page]:text-text-primary (CSS utility) and activeProps={{ className: "bg-accent/15 text-text-primary" }} (direct injection). Both paths land on the same Tailwind classes, so the duplication is harmless, but the CSS-utility variant is redundant given that activeProps already injects the class. Fine to leave; just worth knowing if you're debugging active-highlight specificity later.

isRoot reference-equality dependency
group === groups[0] works because findActiveNavEntry returns the same object reference from the groups array passed in — and the comment above it explicitly calls this out ("Pass the same groups instance"). Correct and documented ✓.

Solid execution. The factory pattern for locale-reactive nav data, the tooltip suppression on flyout open, and the clean test stubs (active-route-aware ActiveLink, useLocation select shim) are all well-thought-out. Merging.

## Review — PR #1109 `feat(settings): inline settings tree in sidenav + flyout on collapsed rail` **CI:** ✅ green &nbsp;|&nbsp; **Mergeable:** ✅ &nbsp;|&nbsp; **Verdict:** ✅ APPROVED --- ### AC coverage | AC | Status | Notes | |---|---|---| | `NAV_GROUPS` extracted to `settings-nav.ts` | ✅ | Factory `getSettingsNavGroups()` — locale-reactive, not a frozen constant | | `NavGroup` / `NavItem` types co-located | ✅ | | | All consumers import from shared module | ✅ / ⚠️ | See note below on `SettingsSideNav` | | Inline tree on `/settings/*` + expanded | ✅ | `SettingsInlineTree` with all 4 groups | | Active leaf highlighted | ✅ | `activeProps` + `aria-[current=page]:*` CSS | | Non-settings route → plain link | ✅ | `SettingsFooterItem` branch on `isOnSettings` | | Group accordions toggle; default expanded; chevron | ✅ | `openGroups` state seeded to all labels | | Collapsed rail → Base UI Menu flyout | ✅ | `SettingsCollapsedFlyout` using `@base-ui-components/react/menu` | | Flyout positioned right of rail icon | ✅ | `side="right" sideOffset={8}` | | Tooltip does not double-fire when flyout open | ✅ | `!open ? <Tooltip>…</Tooltip> : trigger` | | Leaf click closes flyout | ✅ | `Menu.Item render={<Link>}` merges Base UI's close handler; confirmed by the "leaf click closes the flyout" browser test | | `SettingsGroupedNav` dropped from `settings.tsx` | ✅ | | | Full-width content layout | ✅ | `sm:flex-row` dropped; single flex column | | Breadcrumb header `Settings › Group › Leaf` | ✅ | `SettingsBreadcrumbBar` — correct 2-level at root, 3-level elsewhere | | Breadcrumb segments are links | ✅ | Root → `/settings/`; group → first leaf of group | | Old `SettingsBreadcrumb` component deleted | ✅ | All 9 consumers cleaned up | | `settings-manifest.ts` updated | ✅ | 4 new testIds registered in `NON_FIELD_SETTINGS_TESTIDS` | | i18n (en + fr) | ✅ | `settings_breadcrumb_aria_label` + `settings_breadcrumb_root` | | Mobile drawer inline tree preserved | ✅ | `SettingsFooterItem` is in `NavSections` which feeds the drawer; `collapsed=false` in drawer → inline tree path | | `vitest.config.ts` — `--no-sandbox` | ✅ | Container-safe Playwright launch | | `turbo.json` — `passThroughEnv` | ✅ | `CHROMIUM_PATH` + `PLAYWRIGHT_BROWSERS_PATH` visible to test task | **Test coverage** - `sidebar-nav.test.tsx`: inline tree on `/settings/appearance` ✅; Appearance leaf active ✅; other leaves not active ✅; all 9 testIds present ✅; no tree on `/board` ✅; plain link present on non-settings route ✅; collapsed → flyout opens ✅; flyout contains leaves ✅; leaf click closes flyout ✅ - `settings.test.tsx`: `settings-side-nav` absent from DOM ✅; outlet visible ✅; breadcrumb at root shows 2-level ✅; `/settings/appearance` → `General › Appearance` ✅; `/settings/service` → `Forges › Service` ✅; `/settings/secrets` → `Data › Secrets` ✅ --- ### Notes (non-blocking) **`SettingsSideNav` not updated to import from `settings-nav.ts`** The AC says "all current consumers import from the shared module." `settings-side-nav.tsx` isn't touched. This is intentional and justified: the `SettingsSideNav` component serves *deeper sub-trees* inside the agents and service-config pages (agent-types / instance tabs, container / AI-provider sections) — these items are not in `NAV_GROUPS` at all, so there's nothing to import. The AC was written before that distinction was clear. Decision documented in the PR description ✓. No action needed unless #36 later moves those sub-trees into the shared manifest. **Redundant active-state application in inline tree leaves** Each leaf `<Link>` carries both `aria-[current=page]:bg-accent/15 aria-[current=page]:text-text-primary` (CSS utility) *and* `activeProps={{ className: "bg-accent/15 text-text-primary" }}` (direct injection). Both paths land on the same Tailwind classes, so the duplication is harmless, but the CSS-utility variant is redundant given that `activeProps` already injects the class. Fine to leave; just worth knowing if you're debugging active-highlight specificity later. **`isRoot` reference-equality dependency** `group === groups[0]` works because `findActiveNavEntry` returns the same object reference from the `groups` array passed in — and the comment above it explicitly calls this out ("Pass the same `groups` instance"). Correct and documented ✓. --- Solid execution. The factory pattern for locale-reactive nav data, the tooltip suppression on flyout open, and the clean test stubs (active-route-aware `ActiveLink`, `useLocation` select shim) are all well-thought-out. Merging.
Sign in to join this conversation.
Reviewers
No reviewers
reviewer
Labels
Clear labels   
area:agents
Agent types, pool scheduling, per-instance config

  
area:dashboard
Dashboard UI and observability surfaces

  
area:database
DB layer — schema, migrations, ORM, raw SQL

  
area:design
UI/UX mockup work — routes to designer agent

  
area:design-review
Design review dispatch — routes to design-reviewer agent

  
area:flows
Flow runner — YAML loader, executor, op registry, expression eval

  
area:infra
Deployment, isolation, containers, systemd units

  
area:meta
Tracking, scaffolding, project setup

  
area:security
Security — routes to reviewer-security (opus)

  
area:sessions
Session-id store, Claude SDK resume logic

  
area:webhook
Forgejo webhook routing and handlers

  
area:workdir
Clone cache, worktrees, git identity

  
security
Security-sensitive issue

  
type:bug
Bug

  
type:chore
Chore

  
type:meta
Tracking or decisions, not implementation work

  
type:user-story
User story

No labels
area:agents
area:dashboard
area:database
area:design
area:design-review
area:flows
area:infra
area:meta
area:security
area:sessions
area:webhook
area:workdir
security
type:bug
type:chore
type:meta
type:user-story
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
3 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
charles/claude-hooks!1109
No description provided.