refactor(site): promote search to full-width sidebar nav item#25595
refactor(site): promote search to full-width sidebar nav item#25595tracyjohnsonux wants to merge 6 commits into
Conversation
Move the chat search button from a subtle icon next to the filter in the Chats header to a prominent full-width nav item below New Agent. The search bar shows a magnifying glass icon and Search label by default, with the keyboard shortcut badge and background appearing on hover/focus. Also pull the Chats header and filter row out of the scroll area so the scrollbar only covers the chat list content, and align the logo row with the nav item content inset.
|
/coder-agents-review |
There was a problem hiding this comment.
First-pass review (Netero). One P0, one P3. These are mechanical findings from the first-pass scanner; the full review panel has not yet reviewed this PR and will review after these findings are addressed.
The search button promotion and scroll-area restructure look solid. The Tooltip-to-inline-Kbd migration is a cleaner pattern for always-visible affordances.
"Since no Tooltip is rendered anymore, findByRole('tooltip') will time out and the story will deterministically fail." (Netero)
🤖 This review was automatically generated with Coder Agents.
Fix SearchDialogKeyboardShortcut story that asserted on a now-removed Tooltip; assert on the inline button content instead. Extract navItemBaseClassName from SettingsNavItem so the search button reuses the shared nav item layout, spacing, and transition classes.
|
/coder-agents-review |
There was a problem hiding this comment.
Panel review (round 2, 15 reviewers). Both R1 findings verified as fixed. Four new P3s, one Nit, one Note.
Clean work on the Netero fixes: the navItemBaseClassName extraction is the right granularity, and the story update is correct. The layout restructuring (header pinned, scroll area bounded with min-h-0 flex-1) is solid flex technique. The convergent finding from the panel is the mobile viewport regression, where the search button moved into the desktop-only nav zone.
"On mobile phones with the chat list showing, the search button is gone. A phone has no Ctrl key. The search affordance is simply gone on touch surfaces below sm." (Hisoka)
🤖 This review was automatically generated with Coder Agents.
- Add mobile-only search icon button (sm:hidden) in the Chats header so touch users retain a search affordance below the sm breakpoint. - Restore pt-5 on scroll content to preserve the 20px buffer above the mobile gradient mask. - Guard search button render on onOpenSearchDialog being defined so an undefined handler cannot produce a dead button. - Add getOSKey() assertion back to the story for full shortcut coverage. - Add doc comment on the exported navItemBaseClassName constant.
|
/coder-agents-review |
There was a problem hiding this comment.
Review blocked (round 3). Code changes in 6130e14 target 5 of 6 open findings, but all threads were resolved via GitHub UI without substantive author responses. The churn guard requires at least a brief acknowledgment per finding before the panel re-reviews.
Findings with code fixes but no response:
- DEREM-3 (P3): Mobile search button added (
sm:hiddenicon button next to Chats heading) - DEREM-4 (P3): Modifier key assertion restored (
toHaveTextContent("Ctrl")) - DEREM-5 (P3):
pt-5added to scroll content div, restoring gradient mask buffer - DEREM-7 (Nit): Doc comment added to
navItemBaseClassName - DEREM-8 (Note): Search button wrapped in
{onOpenSearchDialog && (...)}
Finding with no code change and no response:
- DEREM-6 (P3): JSX structural duplication with
NavItemContentremains
To unblock: reply on each thread with a brief note (even "fixed" or "intentionally not addressing, [reason]"), then request re-review. For DEREM-6 specifically, if the duplication is intentional, say so and why.
🤖 This review was automatically generated with Coder Agents.
|
/coder-agents-review |
There was a problem hiding this comment.
Panel review (round 4, 11 reviewers). Five R2 fixes verified. DEREM-6 (JSX duplication) contested by author and closed by panel (6/7 accept: the duplication is manageable at two consumers and navItemBaseClassName addresses the fragile portion). One P0, one P3, one Nit new.
The design work is solid. The responsive strategy (full-width nav item on desktop, icon button on mobile) is coherent, the scroll area restructuring is correct, and the prior fixes are all root-cause-level. CI is red due to formatting in commit 6130e14; run pnpm exec biome format --write on ChatsPanel.tsx.
"aria-label='Navigation' on a div is a no-op in conformant assistive technology. It presents one face (accessible landmark) and hides another (completely inert to AT)." (Hisoka)
🤖 This review was automatically generated with Coder Agents.
Run biome format to fix inconsistent JSX indentation. Replace div with nav element for the sidebar navigation container so aria-label is properly announced by assistive technology.
|
/coder-agents-review |
There was a problem hiding this comment.
Panel review (round 5, 8 reviewers). R4 fixes verified (formatting clean, nav landmark correct). DEREM-11 (focus-visible divergence) acknowledged as intentional. One new P3.
All prior findings resolved. The final fix commit (fdcf2ec) is clean: consistent formatting, correct semantic element, no regressions. The panel found one remaining coverage gap in the story file.
"Clicking the search button is the primary interaction. If onClick were removed from the button, every story would still pass." (Bisky)
🤖 This review was automatically generated with Coder Agents.
| @@ -777,11 +777,9 @@ export const SearchDialogKeyboardShortcut: Story = { | |||
| const body = within(document.body); | |||
| const searchButton = canvas.getByRole("button", { name: "Search chats" }); | |||
There was a problem hiding this comment.
P3 [DEREM-12] SearchDialogKeyboardShortcut finds the button and asserts its text content, then opens the dialog via userEvent.keyboard("{Control>}k{/Control}") through the keybinding hook. The button's onClick={onOpenSearchDialog} is never exercised. If onClick were removed from the <button> at ChatsPanel.tsx:348, every story still passes.
The PR promotes search from a subtle icon to a "prominent full-width nav item." Clicking it is the primary interaction. After line 782, add:
await userEvent.click(searchButton);
const searchInput = await body.findByRole("combobox", { name: "Search chats" });
await expect(searchInput).toHaveFocus();
await userEvent.keyboard("{Escape}");Then let the existing Ctrl+K tests continue. Five lines, proves the button works when clicked. (Bisky)
🤖
Move the chat search button from a subtle icon next to the filter in the Chats header to a prominent full-width nav item below New Agent. The search bar shows a magnifying glass icon and "Search" label by default, with the keyboard shortcut badge (
⌘ K/Ctrl K) and background appearing on hover/focus.Also pull the Chats header and filter row out of the scroll area so the scrollbar only covers the chat list content, and align the logo row with the nav item content inset.