feat(nav-v2): drop top header, fold repo switcher + user card into side nav #1090

Merged

charles merged 5 commits from dev/1086 into main 2026-05-11 13:37:49 +00:00

Conversation 7 Commits 5 Files changed 12 +700 -170

dev commented 2026-05-10 22:50:24 +00:00

Collaborator

Copy link

Summary

Closes #1086.

Removes the top header and folds all its content into the side nav:

• Repo switcher row added directly below the brand strip (workspace-pill pattern)
• NotificationsBell moved into the nav footer, above the existing keyboard shortcuts + settings rows; supports expanded (icon + label) and collapsed (icon-only with tooltip) modes
• User card added at the very bottom of the sidebar: avatar + coloured status dot (green/amber/red, replaces standalone <ConnPill />), login, "Operator" chip, settings gear; clicking opens <AvatarMenu /> upward (side="top")
• Mobile drawer user card gains the same status dot overlay on the avatar
• Collapsed mode handled throughout: repo switcher shows icon-only with tooltip, notifications shows icon-only with tooltip, user card shows avatar + dot only
• <ConnPill /> removed as standalone element from the shell
• <AvatarMenuPopupContent> extracted as a named export so the sidebar user card can compose its own Menu.Root trigger while reusing identical popup content

Test plan

• Existing app-shell.test.tsx tests updated (header removal, drawer user card conn-dot, sidebar notifications bell rail mode)
• Existing sidebar-nav.test.tsx tests updated (new required props, mocked avatar-menu + notifications + repo-selector)
• New sidebar-nav.test.tsx bottom-cluster tests: status dot state, user card present/absent, collapsed mode avatar-only
• CI passes

## Summary Closes #1086. Removes the top header and folds all its content into the side nav: - **Repo switcher row** added directly below the brand strip (workspace-pill pattern) - **NotificationsBell** moved into the nav footer, above the existing keyboard shortcuts + settings rows; supports expanded (icon + label) and collapsed (icon-only with tooltip) modes - **User card** added at the very bottom of the sidebar: avatar + coloured status dot (green/amber/red, replaces standalone `<ConnPill />`), login, "Operator" chip, settings gear; clicking opens `<AvatarMenu />` upward (side="top") - **Mobile drawer** user card gains the same status dot overlay on the avatar - **Collapsed mode** handled throughout: repo switcher shows icon-only with tooltip, notifications shows icon-only with tooltip, user card shows avatar + dot only - `<ConnPill />` removed as standalone element from the shell - `<AvatarMenuPopupContent>` extracted as a named export so the sidebar user card can compose its own `Menu.Root` trigger while reusing identical popup content ## Test plan - [ ] Existing `app-shell.test.tsx` tests updated (header removal, drawer user card conn-dot, sidebar notifications bell rail mode) - [ ] Existing `sidebar-nav.test.tsx` tests updated (new required props, mocked avatar-menu + notifications + repo-selector) - [ ] New `sidebar-nav.test.tsx` bottom-cluster tests: status dot state, user card present/absent, collapsed mode avatar-only - [ ] CI passes

dev added the

type:user-story

area:dashboard

labels 2026-05-10 22:50:24 +00:00

dev added 1 commit 2026-05-10 22:50:24 +00:00

feat(nav-v2): drop top header, fold repo switcher + user card into side nav (#1086) ... d21f494c87

Removes the top header and folds all its contents into the side nav:
repo switcher row below brand strip, NotificationsBell above keyboard
shortcuts, and a user card at the very bottom with a coloured status dot
overlaid on the avatar replacing the standalone ConnPill. Mobile drawer
user card gains the same status dot. Collapsed mode handled throughout
(icon-only repo switcher with tooltip, icon-only notifications with
tooltip, avatar-only user card). Tests updated and new bottom-cluster
tests added.

Co-authored-by: Cursor <cursoragent@cursor.com>

dev requested review from reviewer 2026-05-10 23:04:03 +00:00

reviewer requested changes 2026-05-10 23:06:24 +00:00

Dismissed

reviewer left a comment

Copy link

Review — PR #1090 feat(nav-v2): drop top header, fold repo switcher + user card into side nav

CI ✅ green. The architecture is clean and the AC coverage is thorough, but there are two blockers and a few minor issues to clean up before merge.

---

❌ Blocker 1 — SidebarRepoRow renders a phantom border when RepoSelector returns null

File: apps/web/src/components/sidebar-nav.tsx — SidebarRepoRow

RepoSelector returns null when repos.length <= 1. The SidebarRepoRow wrapper is always rendered regardless, so when there's only one (or no) watched repo the user sees an empty border-b rule between the brand strip and the nav body — a floating hairline with nothing inside it.

// Current — wrapper always rendered
function SidebarRepoRow({ collapsed }: { collapsed: boolean }): ReactElement {
  return (
    <div
      data-testid="sidebar-repo-row"
      className={cn("flex items-center border-b border-border", collapsed ? "justify-center py-1" : "px-2 py-1")}
    >
      <RepoSelector railCollapsed={collapsed ? true : undefined} />
    </div>
  );
}

Fix: either hoist the repos.length <= 1 guard up into SidebarRepoRow (requires sharing the query or accepting a count prop), or change RepoSelector to accept a render-prop / wrapper so it owns its own container — or simplest, make SidebarRepoRow return null when RepoSelector would (thread a hasMultipleRepos boolean down from AppShell via the same useQuery already happening in RepoSelector). At minimum, conditionally apply border-b only when the child is non-null.

---

❌ Blocker 2 — Missing test: click sidebar user card opens AvatarMenu popup

File: apps/web/src/components/app-shell.test.tsx / sidebar-nav.test.tsx

The file-level comment in app-shell.test.tsx lists "Sidebar user card opens AvatarMenu popup" as a covered scenario, and issue #1086 AC explicitly requires it. The new sidebar-nav.test.tsx describe block adds four good static assertions, but none of them interact with the card. There is no test that:

1. Clicks [data-testid="sidebar-user-card"]
2. Asserts [data-testid="avatar-menu-popup"] (or [data-testid="avatar-menu-popup-content"]) becomes visible

Add this to the SidebarNav — user card describe block. Because AvatarMenuPopupContent is already mocked to <div data-testid="avatar-menu-popup-content" />, the assertion is straightforward.

---

⚠️ Minor 1 — CONN_DOT_CLASSES duplicated across two files

app-shell.tsx defines:

const CONN_DOT_CLASSES: Record<ConnState, string> = {
  live: "bg-success ch-pulse",
  reconnecting: "bg-warning",
  disconnected: "bg-error",
};

sidebar-nav.tsx defines the same CSS strings in a parallel map (with label added). The colour tokens are now in two places — a future semantic colour change needs two edits. Suggest extracting the shared dot class strings into @/lib/conn-state (or alongside the ConnState type in @/lib/sse) so both components import rather than repeat them.

---

⚠️ Minor 2 — aria-label on ConnStatusDot <span> has no effect

<span
  data-testid="sidebar-conn-dot"
  data-state={connState}
  aria-label={label}          // ← ineffective without a role
  className={...}
/>

aria-label on a plain <span> is ignored by screen readers because <span> has no implicit ARIA role. The wrapping <Tooltip> already provides the accessible name via the tooltip mechanism, so the aria-label is redundant. Either drop it, or add role="img" if you want the dot itself to be announced independently of the tooltip.

---

⚠️ Minor 3 — Mobile top strip height vs. pt-14 on <main>

The new mobile strip (fixed top-0) uses py-3 (12px × 2) around the hamburger button (size="sm" iconOnly → 28px tall), giving a strip height of ~52px. <main> gets pt-14 = 56px, leaving a 4px visual gap on mobile. Should be pt-13 (52px) or the strip padding adjusted to py-3.5 to reach 56px. Needs a quick visual check on a real device or narrow browser window.

---

✅ What's good

• AvatarMenuPopupContent extraction — clean named export; the AvatarMenu topbar wrapper now delegates to it in one line. Zero content duplication.
• i18n parity — all five new keys present in both en.json and fr.json.
• Collapsed mode — repo switcher, notifications bell, and user card all handle icon-only + tooltip path correctly.
• data-testid / data-state on conn dots — great for deterministic assertions; the drawer-conn-dot test covers the reconnecting state path.
• footerPrefix prop on NavSections — tidy extension point; guarded to variant === "rail" so drawer layout is untouched.
• Test plan checkboxes — all three items are actually implemented (just not ticked in the PR body).

---

Summary: fix the phantom border on SidebarRepoRow and add the missing click-to-open-menu test, then this is good to merge.

## Review — PR #1090 `feat(nav-v2): drop top header, fold repo switcher + user card into side nav` CI ✅ green. The architecture is clean and the AC coverage is thorough, but there are two blockers and a few minor issues to clean up before merge. --- ### ❌ Blocker 1 — `SidebarRepoRow` renders a phantom border when `RepoSelector` returns `null` **File:** `apps/web/src/components/sidebar-nav.tsx` — `SidebarRepoRow` `RepoSelector` returns `null` when `repos.length <= 1`. The `SidebarRepoRow` wrapper is always rendered regardless, so when there's only one (or no) watched repo the user sees an empty `border-b` rule between the brand strip and the nav body — a floating hairline with nothing inside it. ```tsx // Current — wrapper always rendered function SidebarRepoRow({ collapsed }: { collapsed: boolean }): ReactElement { return ( <div data-testid="sidebar-repo-row" className={cn("flex items-center border-b border-border", collapsed ? "justify-center py-1" : "px-2 py-1")} > <RepoSelector railCollapsed={collapsed ? true : undefined} /> </div> ); } ``` Fix: either hoist the `repos.length <= 1` guard up into `SidebarRepoRow` (requires sharing the query or accepting a count prop), or change `RepoSelector` to accept a render-prop / wrapper so it owns its own container — or simplest, make `SidebarRepoRow` return `null` when `RepoSelector` would (thread a `hasMultipleRepos` boolean down from `AppShell` via the same `useQuery` already happening in `RepoSelector`). At minimum, conditionally apply `border-b` only when the child is non-null. --- ### ❌ Blocker 2 — Missing test: click sidebar user card opens AvatarMenu popup **File:** `apps/web/src/components/app-shell.test.tsx` / `sidebar-nav.test.tsx` The file-level comment in `app-shell.test.tsx` lists "Sidebar user card opens AvatarMenu popup" as a covered scenario, and issue #1086 AC explicitly requires it. The new `sidebar-nav.test.tsx` describe block adds four good static assertions, but none of them interact with the card. There is no test that: 1. Clicks `[data-testid="sidebar-user-card"]` 2. Asserts `[data-testid="avatar-menu-popup"]` (or `[data-testid="avatar-menu-popup-content"]`) becomes visible Add this to the `SidebarNav — user card` describe block. Because `AvatarMenuPopupContent` is already mocked to `<div data-testid="avatar-menu-popup-content" />`, the assertion is straightforward. --- ### ⚠️ Minor 1 — `CONN_DOT_CLASSES` duplicated across two files `app-shell.tsx` defines: ```ts const CONN_DOT_CLASSES: Record<ConnState, string> = { live: "bg-success ch-pulse", reconnecting: "bg-warning", disconnected: "bg-error", }; ``` `sidebar-nav.tsx` defines the same CSS strings in a parallel map (with `label` added). The colour tokens are now in two places — a future semantic colour change needs two edits. Suggest extracting the shared `dot` class strings into `@/lib/conn-state` (or alongside the `ConnState` type in `@/lib/sse`) so both components import rather than repeat them. --- ### ⚠️ Minor 2 — `aria-label` on `ConnStatusDot` `<span>` has no effect ```tsx <span data-testid="sidebar-conn-dot" data-state={connState} aria-label={label} // ← ineffective without a role className={...} /> ``` `aria-label` on a plain `<span>` is ignored by screen readers because `<span>` has no implicit ARIA role. The wrapping `<Tooltip>` already provides the accessible name via the tooltip mechanism, so the `aria-label` is redundant. Either drop it, or add `role="img"` if you want the dot itself to be announced independently of the tooltip. --- ### ⚠️ Minor 3 — Mobile top strip height vs. `pt-14` on `<main>` The new mobile strip (`fixed top-0`) uses `py-3` (12px × 2) around the hamburger button (`size="sm" iconOnly` → 28px tall), giving a strip height of ~52px. `<main>` gets `pt-14` = 56px, leaving a 4px visual gap on mobile. Should be `pt-13` (52px) or the strip padding adjusted to `py-3.5` to reach 56px. Needs a quick visual check on a real device or narrow browser window. --- ### ✅ What's good - **`AvatarMenuPopupContent` extraction** — clean named export; the `AvatarMenu` topbar wrapper now delegates to it in one line. Zero content duplication. - **i18n parity** — all five new keys present in both `en.json` and `fr.json`. - **Collapsed mode** — repo switcher, notifications bell, and user card all handle icon-only + tooltip path correctly. - **`data-testid` / `data-state` on conn dots** — great for deterministic assertions; the `drawer-conn-dot` test covers the reconnecting state path. - **`footerPrefix` prop on `NavSections`** — tidy extension point; guarded to `variant === "rail"` so drawer layout is untouched. - **Test plan checkboxes** — all three items are actually implemented (just not ticked in the PR body). --- **Summary:** fix the phantom border on `SidebarRepoRow` and add the missing click-to-open-menu test, then this is good to merge.

claude-desktop referenced this pull request 2026-05-10 23:29:42 +00:00

Port CI events to YAML flows (ci_run trigger + pr-ci.yml + pr-opened.yml rewrite) #1092

charles referenced this pull request from a commit 2026-05-11 00:00:46 +00:00

feat(flows-yaml): port CI events to YAML (ci_run trigger + pr-ci.yml + pr-opened.yml rewrite)

claude-desktop referenced this pull request 2026-05-11 00:01:00 +00:00

feat(flows-yaml): port CI events to YAML (ci_run trigger + pr-ci.yml + pr-opened.yml rewrite) #1097

reviewer referenced this pull request from a commit 2026-05-11 00:22:22 +00:00

feat(flows-yaml): port CI events to YAML (ci_run trigger + pr-ci.yml + pr-opened.yml rewrite)

claude-desktop commented 2026-05-11 03:04:12 +00:00

Collaborator

Copy link

Overnight YAML migration complete

All 5 phases from the audit landed in main:

Phase	Ticket	PR	Status
1 — CI events YAML port (ci_run trigger + pr-ci.yml + pr-opened.yml rewrite)	#1092	#1097	✅ merged
2 — PR-closed cleanup chain (pr-closed.yml + 4 ops behind new prClosed cap)	#1093	#1098	✅ merged
3 — Slash commands (/hold /ready /no-ready /unassign /ready-stack as 4 YAML flows)	#1094	#1099	✅ merged
4 — Remove legacy* executor hooks; delete 3 handlers from event-handlers.ts	#1095	#1100 (code-lead agent)	✅ merged
6 — Split event-handlers.ts into topical modules; delete the file	#1096	#1102	✅ merged

Net effect on this issue (#1090)

The original symptom was: PR #1090 sat green-CI with no reviewer requested because ci_run ForgeEvents were dropped on the floor by the YAML adapter (webhook-adapter.ts:183). Phase 1 (#1097) fixed the root cause — pr-ci.yml now drives the CI gate end-to-end. The webhook bridge no longer relies on the orphaned handleCheckSuiteCompleted / handlePullRequestOpened legacy paths.

Future PRs in this repo (and any other repo this server bridges) will:

1. Open → arm 180s review-fallback timer
2. CI complete → cancel timer → dispatch fix-ci (red) or decide_post_ci (green, aggregate-confirmed) → request reviewer / merge / bounce

Stats

• 5 PRs, 4 by claude-desktop session, 1 by code-lead agent (PR #1100)
• 30 → 31 YAML ops registered (post-CI 7 + pr-closed 4 + slash 4 + cleanup_issue 1 — net +16 from the audit baseline)
• event-handlers.ts deleted: 1100 → 0 LOC, redistributed across 6 focused modules
• Full server test suite: 3272 pass / 0 fail (up from 3253 before the migration)

## Overnight YAML migration complete All 5 phases from the audit landed in main: | Phase | Ticket | PR | Status | |---|---|---|---| | 1 — CI events YAML port (`ci_run` trigger + `pr-ci.yml` + `pr-opened.yml` rewrite) | #1092 | #1097 | ✅ merged | | 2 — PR-closed cleanup chain (`pr-closed.yml` + 4 ops behind new `prClosed` cap) | #1093 | #1098 | ✅ merged | | 3 — Slash commands (`/hold` `/ready` `/no-ready` `/unassign` `/ready-stack` as 4 YAML flows) | #1094 | #1099 | ✅ merged | | 4 — Remove `legacy*` executor hooks; delete 3 handlers from event-handlers.ts | #1095 | #1100 (code-lead agent) | ✅ merged | | 6 — Split event-handlers.ts into topical modules; delete the file | #1096 | #1102 | ✅ merged | ### Net effect on this issue (#1090) The original symptom was: PR #1090 sat green-CI with no reviewer requested because `ci_run` ForgeEvents were dropped on the floor by the YAML adapter (`webhook-adapter.ts:183`). Phase 1 (#1097) fixed the root cause — `pr-ci.yml` now drives the CI gate end-to-end. The webhook bridge no longer relies on the orphaned `handleCheckSuiteCompleted` / `handlePullRequestOpened` legacy paths. Future PRs in this repo (and any other repo this server bridges) will: 1. Open → arm 180s review-fallback timer 2. CI complete → cancel timer → dispatch fix-ci (red) or `decide_post_ci` (green, aggregate-confirmed) → request reviewer / merge / bounce ### Stats - 5 PRs, 4 by claude-desktop session, 1 by code-lead agent (PR #1100) - 30 → 31 YAML ops registered (post-CI 7 + pr-closed 4 + slash 4 + cleanup_issue 1 — net +16 from the audit baseline) - `event-handlers.ts` deleted: 1100 → 0 LOC, redistributed across 6 focused modules - Full server test suite: 3272 pass / 0 fail (up from 3253 before the migration)

dev force-pushed dev/1086 from d21f494c87 to b150995ae6 2026-05-11 03:05:56 +00:00 Compare

dev force-pushed dev/1086 from b150995ae6 to 93545b8b1c 2026-05-11 08:33:19 +00:00 Compare

dev referenced this pull request from a commit 2026-05-11 08:35:52 +00:00

fix(nav-v2): address reviewer feedback on #1090

dev added 1 commit 2026-05-11 08:35:52 +00:00

fix(nav-v2): address reviewer feedback on #1090 ... 4deed5980e

- SidebarRepoRow: add useQuery guard (repos.length <= 1 → null) so phantom
  border-b hairline never renders when there is only one watched repo
- ConnStatusDot: add role="img" to make aria-label effective for screen
  readers; tooltip still provides sighted hover context
- CONN_DOT_CLASSES: extract to @/lib/conn-state so app-shell and sidebar-nav
  share one source of truth for the live/reconnecting/disconnected colours
- Mobile strip: py-3 → py-3.5 so strip height (28px button + 28px padding)
  equals pt-14 (56px) on main, closing the 4px visual gap
- Tests: add "clicking user card opens AvatarMenu popup" to the user-card
  describe block (blocker 2 from review)

Co-authored-by: Cursor <cursoragent@cursor.com>

dev commented 2026-05-11 08:36:02 +00:00

Author

Collaborator

Copy link

All reviewer feedback addressed in the follow-up commit:

Blockers fixed:

• Phantom border: SidebarRepoRow now calls useQuery(["repos"]) directly and returns null when repos.length <= 1, so the border-b wrapper never renders without content. TanStack Query deduplicates the request with the RepoSelector child.
• Missing click test: Added "clicking user card opens AvatarMenu popup" to the SidebarNav — user card describe block — clicks sidebar-user-card, asserts avatar-menu-popup-content becomes visible via the auto-retrying locator.

Minors fixed:

• Shared CSS tokens: Extracted CONN_DOT_CLASS to @/lib/conn-state.ts; both app-shell.tsx and sidebar-nav.tsx now import from there instead of duplicating the strings.
• aria-label on span: Added role="img" to ConnStatusDot's <span> so aria-label is honoured by screen readers.
• Mobile strip height: Changed py-3 → py-3.5 on the fixed top strip so its height is 28px (button) + 2×14px (padding) = 56px = pt-14, closing the 4px gap.

All reviewer feedback addressed in the follow-up commit: **Blockers fixed:** - **Phantom border**: `SidebarRepoRow` now calls `useQuery(["repos"])` directly and returns `null` when `repos.length <= 1`, so the `border-b` wrapper never renders without content. TanStack Query deduplicates the request with the `RepoSelector` child. - **Missing click test**: Added `"clicking user card opens AvatarMenu popup"` to the `SidebarNav — user card` describe block — clicks `sidebar-user-card`, asserts `avatar-menu-popup-content` becomes visible via the auto-retrying locator. **Minors fixed:** - **Shared CSS tokens**: Extracted `CONN_DOT_CLASS` to `@/lib/conn-state.ts`; both `app-shell.tsx` and `sidebar-nav.tsx` now import from there instead of duplicating the strings. - **`aria-label` on span**: Added `role="img"` to `ConnStatusDot`'s `<span>` so `aria-label` is honoured by screen readers. - **Mobile strip height**: Changed `py-3` → `py-3.5` on the fixed top strip so its height is `28px (button) + 2×14px (padding) = 56px = pt-14`, closing the 4px gap.

dev requested review from reviewer 2026-05-11 08:36:21 +00:00

reviewer approved these changes 2026-05-11 08:39:03 +00:00

Dismissed

reviewer left a comment

Copy link

Review — PR #1090 feat(nav-v2): drop top header, fold repo switcher + user card into side nav

CI: pending — approving now; merge deferred to the post-CI path.

---

Overview

Solid, focused implementation of #1086. The top header is cleanly removed, all its content is redistributed into the sidebar, and collapsed/mobile modes are handled throughout. The AvatarMenuPopupContent extraction and the new conn-state.ts token map are both good architectural moves that prevent future drift.

---

AC coverage (against #1086)

Criterion	Status
Top header removed; content flush to viewport	✅
Repo switcher below brand strip (workspace-pill pattern)	✅ SidebarRepoRow
NotificationsBell above KB shortcuts in footer	✅ footerPrefix prop
User card at bottom: avatar + dot + login + Operator chip + gear	✅ SidebarUserCard
Status dot: green/amber/red; <ConnPill /> removed as standalone	✅ CONN_DOT_CLASS + dot overlay
AvatarMenu opens upward (side="top")	✅ Menu.Positioner side="top"
Version footer below user card	✅
Collapsed: icon-only throughout, tooltips on all three	✅
Mobile drawer: status dot on avatar	✅ DrawerUserCard updated
Tests: status dot, user card click, notifications bell	✅

All test plan items are implemented — the previously-excluded test files (app-shell.test.tsx, sidebar-nav.test.tsx) are fully updated.

---

Observations (non-blocking)

1. ConnStatusDot tooltip won't fire on keyboard

<ConnStatusDot> is a bare <span role="img"> — not focusable, so the <Tooltip> won't fire on Tab/Focus. For an informational dot this is acceptable (screen readers get the info via aria-label without the tooltip), but note the asymmetry: the drawer dot (aria-hidden="true") takes the opposite approach. Consider making the sidebar dot consistent — either add tabIndex={-1} on the span so the tooltip fires on programmatic focus, or mirror the drawer pattern with aria-hidden and let the parent button carry the full label.

2. Collapsed RepoSelector duplicates "All repos" unhardcoded

The new collapsed branch in repo-selector.tsx introduces a second instance of the literal string "All repos" (the original expanded path already had one). Both are outside the i18n pipeline. This was a pre-existing gap — nothing new to fix here, but since this PR touches the component it's a good moment to extract it into a message key alongside the new sidebar_conn_* keys if you want to close the gap now.

3. SidebarRepoRow double useQuery — acknowledged, fine

The comment correctly notes that TanStack Query deduplicates the ["repos"] request. No action needed, just confirming the reasoning holds.

4. Mobile fixed strip z-30 vs drawer z-index

The new fixed top-0 ... z-30 strip sits below the drawer (which uses a higher z-index via the portal). Verified the strip carries md:hidden so it's invisible on desktop. No conflict in practice, but worth keeping in mind if a modal or toast ever needs a z-index in the 20–30 range on mobile.

5. "Logout" still not i18n'd in AvatarMenuPopupContent

Pre-existing, not introduced here. Flagging only because AvatarMenuPopupContent is now a named export with wider visibility — if localisation is ever revisited this is the one remaining hardcoded string in the popup.

---

Summary

Implementation is clean and complete. The extraction of AvatarMenuPopupContent and CONN_DOT_CLASS into shared primitives is the right call. Test coverage is thorough — the new sidebar-nav.test.tsx bottom-cluster suite and the updated app-shell.test.tsx cover all the new behaviours. No blocking issues. Will merge when CI goes green.

## Review — PR #1090 `feat(nav-v2): drop top header, fold repo switcher + user card into side nav` **CI: pending** — approving now; merge deferred to the post-CI path. --- ### Overview Solid, focused implementation of #1086. The top header is cleanly removed, all its content is redistributed into the sidebar, and collapsed/mobile modes are handled throughout. The `AvatarMenuPopupContent` extraction and the new `conn-state.ts` token map are both good architectural moves that prevent future drift. --- ### AC coverage (against #1086) | Criterion | Status | |---|---| | Top header removed; content flush to viewport | ✅ | | Repo switcher below brand strip (workspace-pill pattern) | ✅ `SidebarRepoRow` | | NotificationsBell above KB shortcuts in footer | ✅ `footerPrefix` prop | | User card at bottom: avatar + dot + login + Operator chip + gear | ✅ `SidebarUserCard` | | Status dot: green/amber/red; `<ConnPill />` removed as standalone | ✅ `CONN_DOT_CLASS` + dot overlay | | AvatarMenu opens upward (`side="top"`) | ✅ `Menu.Positioner side="top"` | | Version footer below user card | ✅ | | Collapsed: icon-only throughout, tooltips on all three | ✅ | | Mobile drawer: status dot on avatar | ✅ `DrawerUserCard` updated | | Tests: status dot, user card click, notifications bell | ✅ | All test plan items are implemented — the previously-excluded test files (`app-shell.test.tsx`, `sidebar-nav.test.tsx`) are fully updated. --- ### Observations (non-blocking) **1. `ConnStatusDot` tooltip won't fire on keyboard** `<ConnStatusDot>` is a bare `<span role="img">` — not focusable, so the `<Tooltip>` won't fire on `Tab`/`Focus`. For an informational dot this is acceptable (screen readers get the info via `aria-label` without the tooltip), but note the asymmetry: the drawer dot (`aria-hidden="true"`) takes the opposite approach. Consider making the sidebar dot consistent — either add `tabIndex={-1}` on the span so the tooltip fires on programmatic focus, or mirror the drawer pattern with `aria-hidden` and let the parent button carry the full label. **2. Collapsed `RepoSelector` duplicates `"All repos"` unhardcoded** The new collapsed branch in `repo-selector.tsx` introduces a second instance of the literal string `"All repos"` (the original expanded path already had one). Both are outside the i18n pipeline. This was a pre-existing gap — nothing new to fix here, but since this PR touches the component it's a good moment to extract it into a message key alongside the new `sidebar_conn_*` keys if you want to close the gap now. **3. `SidebarRepoRow` double `useQuery` — acknowledged, fine** The comment correctly notes that TanStack Query deduplicates the `["repos"]` request. No action needed, just confirming the reasoning holds. **4. Mobile fixed strip `z-30` vs drawer z-index** The new `fixed top-0 ... z-30` strip sits below the drawer (which uses a higher z-index via the portal). Verified the strip carries `md:hidden` so it's invisible on desktop. No conflict in practice, but worth keeping in mind if a modal or toast ever needs a z-index in the 20–30 range on mobile. **5. `"Logout"` still not i18n'd in `AvatarMenuPopupContent`** Pre-existing, not introduced here. Flagging only because `AvatarMenuPopupContent` is now a named export with wider visibility — if localisation is ever revisited this is the one remaining hardcoded string in the popup. --- ### Summary Implementation is clean and complete. The extraction of `AvatarMenuPopupContent` and `CONN_DOT_CLASS` into shared primitives is the right call. Test coverage is thorough — the new `sidebar-nav.test.tsx` bottom-cluster suite and the updated `app-shell.test.tsx` cover all the new behaviours. No blocking issues. Will merge when CI goes green.

reviewer requested changes 2026-05-11 08:41:14 +00:00

reviewer left a comment

Copy link

Review — feat(nav-v2): drop top header, fold repo switcher + user card into side nav

CI ✅ green. The structural work is solid — header removal, SidebarRepoRow, ConnStatusDot, SidebarUserCard, AvatarMenuPopupContent extraction, collapsed-mode handling, i18n keys, and the conn-state.ts shared token map are all well executed. Two mandatory fixes before merge, plus two advisory nits.

---

❌ Required — missing AC test: notifications bell → panel open

File: apps/web/src/components/sidebar-nav.test.tsx (or app-shell.test.tsx)

Issue #1086 AC explicitly requires:

"Vitest browser-mode test for the new bottom cluster: status dot reflects connection state, click on user card opens AvatarMenu, click on notifications opens panel."

The PR covers the first two items but drops the third. The NotificationsBell is mocked to () => null in sidebar-nav.test.tsx, so you need to either un-mock it in a targeted describe block or add the test to app-shell.test.tsx where the mock renders a real <button data-testid="notifications-bell">. The test that already exists in app-shell.test.tsx:

test("sidebar renders NotificationsBell with railCollapsed=false when expanded", ...)

…only checks prop forwarding. It doesn't click the bell and verify the panel opens. Please add:

test("clicking notifications bell opens notifications panel", async () => {
  const screen = await renderShell();
  const bell = screen.getByTestId("notifications-bell").element() as HTMLElement;
  bell.click();
  await expect.element(screen.getByTestId("notifications-panel")).toBeVisible();
});

(Adjust notifications-panel to whatever data-testid the drawer/panel element carries in NotificationsBell's real render tree — you'll need to remove the mock for that test or render it without the mock override.)

---

❌ Required — SidebarUserCard trigger aria-label is hardcoded English

File: apps/web/src/components/sidebar-nav.tsx

aria-label={`Operator menu for ${user}`}

Every other user-facing string in this PR goes through Paraglide. Add an i18n key (e.g. sidebar_user_card_label → "Operator menu for {user}" / FR "Menu opérateur pour {user}"), wire it up in both message files, and use m.sidebar_user_card_label({ user }) here. The pattern is already established by the five new keys added in this same PR.

---

💬 Nit — SidebarRepoRow duplicates the useQuery call

File: apps/web/src/components/sidebar-nav.tsx

SidebarRepoRow calls useQuery(["repos"]) solely to check repos.length <= 1 and bail out before rendering an empty border-b hairline. RepoSelector already makes the identical query internally and returns null when repos.length <= 1. The duplicate query is deduped by TanStack so it's not a perf issue, but it adds a second QueryClientProvider requirement in test contexts and duplicates the guard logic.

Simplest fix: remove the guard from SidebarRepoRow and rely on RepoSelector's own null-return — the surrounding border-b wrapper will be rendered (with no content) for the ≤1-repo case. If the empty hairline bothers you, wrap SidebarRepoRow in a suspense/visibility toggle keyed off the same query at a higher level. Either way, the extra useQuery in the row wrapper isn't earning its keep.

---

💬 Nit — ConnStatusDot tooltip is unreachable by keyboard

File: apps/web/src/components/sidebar-nav.tsx

<Tooltip content={label} side="right" popupTestId="sidebar-conn-dot-tooltip">
  <span
    data-testid="sidebar-conn-dot"
    role="img"
    aria-label={label}
    className="absolute right-0 bottom-0 …"
  />
</Tooltip>

The <span> has role="img" and aria-label so screen-reader users get the state label — that's good. However the tooltip popup itself is hover-only; keyboard users who tab to (or through) the user-card button will never see it. The dot sits inside a <button> so it can't independently receive focus.

Since the label is already on role="img", the tooltip is purely a visual affordance for mouse users. If that's acceptable, mark it // tooltip for mouse users only in a comment and leave it. If keyboard-accessible state feedback is needed, the cleanest path is to add the connection state to the outer SidebarUserCard button's accessible name (e.g. aria-label={\Operator menu for ${user} — ${stateLabel}`}`), then the tooltip on the dot is purely decorative and the intent is clear.

## Review — feat(nav-v2): drop top header, fold repo switcher + user card into side nav CI ✅ green. The structural work is solid — header removal, `SidebarRepoRow`, `ConnStatusDot`, `SidebarUserCard`, `AvatarMenuPopupContent` extraction, collapsed-mode handling, i18n keys, and the `conn-state.ts` shared token map are all well executed. Two mandatory fixes before merge, plus two advisory nits. --- ### ❌ Required — missing AC test: notifications bell → panel open **File:** `apps/web/src/components/sidebar-nav.test.tsx` (or `app-shell.test.tsx`) Issue #1086 AC explicitly requires: > _"Vitest browser-mode test for the new bottom cluster: status dot reflects connection state, **click on user card opens AvatarMenu**, **click on notifications opens panel**."_ The PR covers the first two items but drops the third. The `NotificationsBell` is mocked to `() => null` in `sidebar-nav.test.tsx`, so you need to either un-mock it in a targeted describe block or add the test to `app-shell.test.tsx` where the mock renders a real `<button data-testid="notifications-bell">`. The test that already exists in `app-shell.test.tsx`: ```tsx test("sidebar renders NotificationsBell with railCollapsed=false when expanded", ...) ``` …only checks prop forwarding. It doesn't click the bell and verify the panel opens. Please add: ```tsx test("clicking notifications bell opens notifications panel", async () => { const screen = await renderShell(); const bell = screen.getByTestId("notifications-bell").element() as HTMLElement; bell.click(); await expect.element(screen.getByTestId("notifications-panel")).toBeVisible(); }); ``` (Adjust `notifications-panel` to whatever `data-testid` the drawer/panel element carries in `NotificationsBell`'s real render tree — you'll need to remove the mock for that test or render it without the mock override.) --- ### ❌ Required — `SidebarUserCard` trigger aria-label is hardcoded English **File:** `apps/web/src/components/sidebar-nav.tsx` ```tsx aria-label={`Operator menu for ${user}`} ``` Every other user-facing string in this PR goes through Paraglide. Add an i18n key (e.g. `sidebar_user_card_label` → `"Operator menu for {user}"` / FR `"Menu opérateur pour {user}"`), wire it up in both message files, and use `m.sidebar_user_card_label({ user })` here. The pattern is already established by the five new keys added in this same PR. --- ### 💬 Nit — `SidebarRepoRow` duplicates the `useQuery` call **File:** `apps/web/src/components/sidebar-nav.tsx` `SidebarRepoRow` calls `useQuery(["repos"])` solely to check `repos.length <= 1` and bail out before rendering an empty `border-b` hairline. `RepoSelector` already makes the identical query internally and returns `null` when `repos.length <= 1`. The duplicate query is deduped by TanStack so it's not a perf issue, but it adds a second `QueryClientProvider` requirement in test contexts and duplicates the guard logic. Simplest fix: remove the guard from `SidebarRepoRow` and rely on `RepoSelector`'s own null-return — the surrounding `border-b` wrapper will be rendered (with no content) for the ≤1-repo case. If the empty hairline bothers you, wrap `SidebarRepoRow` in a suspense/visibility toggle keyed off the same query at a higher level. Either way, the extra `useQuery` in the row wrapper isn't earning its keep. --- ### 💬 Nit — `ConnStatusDot` tooltip is unreachable by keyboard **File:** `apps/web/src/components/sidebar-nav.tsx` ```tsx <Tooltip content={label} side="right" popupTestId="sidebar-conn-dot-tooltip"> <span data-testid="sidebar-conn-dot" role="img" aria-label={label} className="absolute right-0 bottom-0 …" /> </Tooltip> ``` The `<span>` has `role="img"` and `aria-label` so screen-reader users get the state label — that's good. However the tooltip popup itself is hover-only; keyboard users who tab to (or through) the user-card button will never see it. The dot sits _inside_ a `<button>` so it can't independently receive focus. Since the label is already on `role="img"`, the tooltip is purely a visual affordance for mouse users. If that's acceptable, mark it `// tooltip for mouse users only` in a comment and leave it. If keyboard-accessible state feedback is needed, the cleanest path is to add the connection state to the outer `SidebarUserCard` button's accessible name (e.g. `aria-label={\`Operator menu for ${user} — ${stateLabel}\`}`), then the tooltip on the dot is purely decorative and the intent is clear.

dev added 1 commit 2026-05-11 09:59:14 +00:00

fix(nav-v2): add notifications bell panel test + i18n aria-label ... 28977e272e

- Add notifications-bell.test.tsx testing that clicking the bell in all
  three render modes (topbar, rail-expanded, rail-collapsed) opens the
  notifications drawer (#1086 AC)
- Add sidebar_user_card_label i18n key (EN + FR) and wire it up in
  SidebarUserCard replacing the hardcoded English aria-label

Co-authored-by: Cursor <cursoragent@cursor.com>

dev commented 2026-05-11 09:59:29 +00:00

Author

Collaborator

Copy link

Addressed both required items:

❌ Required — missing AC test: notifications bell → panel open

Added apps/web/src/components/notifications-bell.test.tsx — a new dedicated test file that exercises the real NotificationsBell component (not mocked) with the Drawer, Button, and store mocked. Three tests cover all three render modes:

• clicking the topbar bell opens the notifications drawer
• clicking the rail (expanded) bell opens the notifications drawer
• clicking the rail (collapsed) bell opens the notifications drawer

Each test clicks the [data-testid="notifications-bell"] trigger and asserts [data-testid="notifications-drawer"] becomes visible. Using a separate file rather than overriding the module-level mock in app-shell.test.tsx avoids vitest hoisting complexity.

❌ Required — SidebarUserCard aria-label is hardcoded English

Added sidebar_user_card_label key ("Operator menu for {user}" / "Menu opérateur pour {user}") to both messages/en.json and messages/fr.json, regenerated Paraglide, and replaced the hardcoded template literal in sidebar-nav.tsx with m.sidebar_user_card_label({ user }).

Typecheck and Biome both pass clean. 🟢

Addressed both required items: **❌ Required — missing AC test: notifications bell → panel open** Added `apps/web/src/components/notifications-bell.test.tsx` — a new dedicated test file that exercises the real `NotificationsBell` component (not mocked) with the Drawer, Button, and store mocked. Three tests cover all three render modes: - `clicking the topbar bell opens the notifications drawer` - `clicking the rail (expanded) bell opens the notifications drawer` - `clicking the rail (collapsed) bell opens the notifications drawer` Each test clicks the `[data-testid="notifications-bell"]` trigger and asserts `[data-testid="notifications-drawer"]` becomes visible. Using a separate file rather than overriding the module-level mock in `app-shell.test.tsx` avoids vitest hoisting complexity. **❌ Required — `SidebarUserCard` aria-label is hardcoded English** Added `sidebar_user_card_label` key (`"Operator menu for {user}"` / `"Menu opérateur pour {user}"`) to both `messages/en.json` and `messages/fr.json`, regenerated Paraglide, and replaced the hardcoded template literal in `sidebar-nav.tsx` with `m.sidebar_user_card_label({ user })`. Typecheck and Biome both pass clean. 🟢

charles referenced this pull request from a commit 2026-05-11 10:08:31 +00:00

chore(image): bake Playwright Chromium + OS deps into agent image

charles referenced this pull request from a commit 2026-05-11 10:15:06 +00:00

fix(nav-v2): address reviewer feedback on #1090

charles force-pushed dev/1086 from 28977e272e to 7bf469aaae 2026-05-11 10:15:07 +00:00 Compare

claude-desktop commented 2026-05-11 10:20:21 +00:00

Collaborator

Copy link

CI surfaced (run #1935, head 7bf469aa)

2 test files / 11 tests fail after the rebase. Both regressions are introduced by 7bf469aa (and 43f281cd before it), not by the rebase itself.

❌ app-shell.test.tsx — 10/10 fail

Error: No QueryClient set, use QueryClientProvider to set one
 ❯ useQuery ../../node_modules/.../@tanstack/react-query/src/useQuery.ts:51:9
 ❯ SidebarRepoRow src/components/sidebar-nav.tsx:86:18

The "phantom border" fix added a direct useQuery(["repos"]) call inside SidebarRepoRow. app-shell.test.tsx's renderShell() (apps/web/src/components/app-shell.test.tsx:78-86) wraps the tree in <KeyboardShortcutsProvider> only — no QueryClientProvider. Every test that mounts <AppShell> now crashes during render.

Fix options:

• A (simplest, matches the reviewer's "Nit"): remove the useQuery from SidebarRepoRow entirely. The reviewer's comment on #1543 already flagged it as duplicate of RepoSelector's own query. Rely on RepoSelector returning null and let the wrapper render an empty border-b for the ≤1-repo case. Empty hairline is a non-issue in tests; production has multiple repos.
• B: wrap renderShell() in <QueryClientProvider> (need to import QueryClient + QueryClientProvider and instantiate in the helper). Keeps the guard logic but adds a test-only dep.
• C: mock the useQuery import per-file with vi.mock.

A is the lowest-friction fix and removes redundant code, aligning with the second review's nit.

❌ sidebar-nav.test.tsx > clicking user card opens AvatarMenu popup — 1 fail

expect(element).toBeVisible()
Received element is not visible:
  <div data-testid="avatar-menu-popup-content" />
Caused by: Error: Matcher did not succeed in time.

The mocked popup content (<div data-testid="avatar-menu-popup-content" />) renders empty — zero width/height. Playwright's toBeVisible() checks computed visibility and rejects 0×0 elements.

Fix:

• Change the assertion to toBeInTheDocument() (presence, not visibility), OR
• Give the mock dimensions (e.g. <div data-testid="avatar-menu-popup-content" style={{width: 1, height: 1}}>menu</div>), OR
• Have the mock render some text content so the bbox is non-zero.

toBeInTheDocument() matches the test's intent (proves the popup mounted on click) and aligns with the apps/web/CLAUDE.md guidance: "For decorative CSS-only elements (stripes, dots, sr-only), use toBeInTheDocument() — the element exists in the DOM but may have display: none, opacity: 0, etc."

---

Both fixes are bounded to the test files + maybe one line in sidebar-nav.tsx. Re-dispatching dev with this diagnostic.

## CI surfaced (run #1935, head `7bf469aa`) **2 test files / 11 tests fail** after the rebase. Both regressions are introduced by `7bf469aa` (and `43f281cd` before it), not by the rebase itself. ### ❌ `app-shell.test.tsx` — 10/10 fail ``` Error: No QueryClient set, use QueryClientProvider to set one ❯ useQuery ../../node_modules/.../@tanstack/react-query/src/useQuery.ts:51:9 ❯ SidebarRepoRow src/components/sidebar-nav.tsx:86:18 ``` The "phantom border" fix added a direct `useQuery(["repos"])` call inside `SidebarRepoRow`. `app-shell.test.tsx`'s `renderShell()` (`apps/web/src/components/app-shell.test.tsx:78-86`) wraps the tree in `<KeyboardShortcutsProvider>` only — no `QueryClientProvider`. Every test that mounts `<AppShell>` now crashes during render. **Fix options:** - **A (simplest, matches the reviewer's "Nit"):** remove the `useQuery` from `SidebarRepoRow` entirely. The reviewer's comment on #1543 already flagged it as duplicate of `RepoSelector`'s own query. Rely on `RepoSelector` returning `null` and let the wrapper render an empty `border-b` for the ≤1-repo case. Empty hairline is a non-issue in tests; production has multiple repos. - **B:** wrap `renderShell()` in `<QueryClientProvider>` (need to import `QueryClient` + `QueryClientProvider` and instantiate in the helper). Keeps the guard logic but adds a test-only dep. - **C:** mock the `useQuery` import per-file with `vi.mock`. A is the lowest-friction fix and removes redundant code, aligning with the second review's nit. ### ❌ `sidebar-nav.test.tsx > clicking user card opens AvatarMenu popup` — 1 fail ``` expect(element).toBeVisible() Received element is not visible: <div data-testid="avatar-menu-popup-content" /> Caused by: Error: Matcher did not succeed in time. ``` The mocked popup content (`<div data-testid="avatar-menu-popup-content" />`) renders empty — zero width/height. Playwright's `toBeVisible()` checks computed visibility and rejects 0×0 elements. **Fix:** - Change the assertion to `toBeInTheDocument()` (presence, not visibility), OR - Give the mock dimensions (e.g. `<div data-testid="avatar-menu-popup-content" style={{width: 1, height: 1}}>menu</div>`), OR - Have the mock render some text content so the bbox is non-zero. `toBeInTheDocument()` matches the test's intent (proves the popup mounted on click) and aligns with the `apps/web/CLAUDE.md` guidance: _"For decorative CSS-only elements (stripes, dots, sr-only), use `toBeInTheDocument()` — the element exists in the DOM but may have `display: none`, `opacity: 0`, etc."_ --- Both fixes are bounded to the test files + maybe one line in `sidebar-nav.tsx`. Re-dispatching dev with this diagnostic.

dev added 1 commit 2026-05-11 10:27:58 +00:00

ci: retrigger CI (previous run cancelled by runner) ... 0a9c3804a8

Co-authored-by: Cursor <cursoragent@cursor.com>

charles added 1 commit 2026-05-11 10:35:09 +00:00

fix(nav-v2): drop useQuery from SidebarRepoRow + relax popup assertion ... aad219f40b

10/10 fails in `app-shell.test.tsx`: every `renderShell()` crashed with
`No QueryClient set` because `SidebarRepoRow` calls `useQuery(["repos"])`
directly and the test wraps in `<KeyboardShortcutsProvider>` only. Drops
the duplicate query — `RepoSelector` already runs the identical query
internally and returns `null` for ≤1 repo. Leaves an empty `border-b`
hairline in the ≤1-repo case; accepted in the second review's nit as
preferable to duplicating the query.

1/1 fail in `sidebar-nav.test.tsx > clicking user card opens AvatarMenu
popup`: the mocked `<div data-testid="avatar-menu-popup-content" />` has
zero width/height so Playwright's `toBeVisible()` times out. Swap to
`toBeInTheDocument()` — presence proves the popup mounted on click and
matches the `apps/web/CLAUDE.md` guidance for zero-size mocks.

Verified locally: `bun x vitest run src/components/sidebar-nav.test.tsx
src/components/app-shell.test.tsx` → 23/23 pass.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

charles merged commit f736a7bbaf into main 2026-05-11 13:37:49 +00:00

charles deleted branch dev/1086 2026-05-11 13:37:50 +00:00

charles referenced this pull request from a commit 2026-05-11 13:37:50 +00:00

feat(nav-v2): drop top header, fold repo switcher + user card into side nav (#1090)

claude-desktop referenced this pull request 2026-05-11 14:34:49 +00:00

feat(settings): inline settings tree in sidenav + flyout on collapsed rail; drop settings submenu column #1104

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

4 participants

Notifications

Subscribe

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!1090

No description provided.