Skip to content

fix(desktop): find bar positioning and overlay visibility - #72959

Closed
DavidMetcalfe wants to merge 5 commits into
NousResearch:mainfrom
DavidMetcalfe:fix/find-bar-positioning
Closed

fix(desktop): find bar positioning and overlay visibility#72959
DavidMetcalfe wants to merge 5 commits into
NousResearch:mainfrom
DavidMetcalfe:fix/find-bar-positioning

Conversation

@DavidMetcalfe

@DavidMetcalfe DavidMetcalfe commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

What does this PR do?

Fixes two UX issues with the find-in-page bar (Ctrl+F) added in #72235:

  1. Position: The find bar overlapped the titlebar system tools (layout, haptics, keybinds, settings gear). Replaced fixed right-4 with a computed offset that accounts for the titlebar tool cluster width, so the bar sits to the left of those icons.

  2. Overlay visibility: The find bar rendered behind full-screen overlays (Settings, Command Center, etc.) at z-50, invisible but active. Added an isOverlayView() guard matching the titlebar controls pattern, plus a keybind gate so Ctrl+F is a no-op on overlay routes. This also avoids collision with the upcoming Settings search bar (feat(desktop): settings search bar for quick setting discovery #69025).

Changes Made

  • apps/desktop/src/components/find-bar.tsx — replace right-4 with calc() offset from titlebar tools; add isOverlayView() guard
  • apps/desktop/src/app/hooks/use-keybinds.ts — gate view.findInPage keybind on overlay routes
  • apps/desktop/src/components/find-bar.test.tsx — add regression test for overlay guard (49/49 passing)

How to Test

  1. Open Hermes Desktop, press Ctrl+F — find bar should appear to the left of the titlebar icons, not overlapping them
  2. Open Settings, press Ctrl+F — find bar should NOT appear
  3. Close Settings — find bar should NOT reappear
  4. On a chat session, press Ctrl+F, type text — search should work normally

Checklist

Code

  • My commit messages follow Conventional Commits
  • I searched for existing PRs to make sure this isn't a duplicate
  • My PR contains only changes related to this fix
  • I've added tests for my changes (49/49 passing)
  • I've tested on my platform: macOS 26.5.2

Documentation & Housekeeping

  • N/A — no documentation changes needed
  • N/A — no config keys changed
  • N/A — no architecture changes
  • Considered cross-platform: CSS calc() uses existing wiring.tsx vars already tested on Windows/macOS
  • N/A — no tool behavior changes

@teknium1 teknium1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for addressing the find-bar routing and titlebar collision. The overlay-route guard follows the existing titlebar pattern and the current-main premise is verified.

Problems

  • apps/desktop/src/components/find-bar.tsx:163 uses --titlebar-tools-width, but that variable counts four controls in apps/desktop/src/app/contrib/wiring.tsx:955. The rendered cluster has four systemTools plus the always-present rightSidebarTool (apps/desktop/src/app/shell/titlebar-controls.tsx:259-262). With the 1.25rem control size in apps/desktop/src/styles.css:461, the proposed offset remains 0.75rem inside the actual cluster, so the layout fix still overlaps it.

Suggested changes

  • Make the shared titlebar-width calculation include all five static right-side buttons, or add the missing control width and gap to this offset; then verify the find bar clears the full cluster.
  • Add a keybind-level overlay test. The new component test confirms hidden rendering, but it intentionally leaves $findInPage.active true and does not exercise the new view.findInPage gate.

Automated hermes-sweeper review.

@@ -159,7 +160,7 @@ export function FindBar() {
return (
<div
className={cn(
'pointer-events-auto fixed right-4 top-[calc(var(--titlebar-height,0px)+0.5rem)] z-50',
'pointer-events-auto fixed right-[calc(var(--titlebar-tools-right,0.75rem)+var(--titlebar-tools-width,0px)+0.5rem)] top-[calc(var(--titlebar-height,0px)+0.5rem)] z-50',

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

--titlebar-tools-width currently accounts for four controls, but the fixed right cluster renders four systemTools plus rightSidebarTool. With 1.25rem controls and 0.25rem gaps, this offset is still 0.75rem inside the actual cluster. Please include the fifth control in the shared width calculation or in this offset.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@teknium1 Both findings verified against the code and addressed. Thanks for the arithmetic — it checked out exactly.

1. Width under-count — verified, fixed at the source.
SYSTEM_TOOL_COUNT = 4 × (1.25rem control + 0.25rem gap) = 96px, but the appControls cluster renders four systemTools plus the unconditional rightSidebarTool (titlebar-controls.tsx:262): 5 × 20px + 4 × 4px = 116px, anchored at right: 0.75rem. The find bar's right edge landed at 12 + 96 + 8 = 116px — 12px (0.75rem) inside the fifth button's span [108, 128]px, exactly your number.

Fixed in wiring.tsx (commit 4578b73a7): SYSTEM_TOOL_COUNT is now 5 with a comment naming all five buttons. Because the count is shared, this also corrects the two other consumers with the same one-button shortfall — the titlebar header's right padding (titlebarHeaderBaseClass in titlebar.ts) and the pane-cluster anchor (--shell-preview-toolbar-gap). I also found a hardcoded copy of the same 4-count in controller.tsx:794 (the contribution controller's titleBar.right slot) and fixed it to 5 with a keep-in-sync comment (commit a43933b13) — same bug class, sibling path.

Post-fix geometry: find bar right edge at 12 + 120 + 8 = 140px, clearing the cluster's left edge (128px) by 12px.

2. Keybind-level gate test — added.
New view.findInPage keybind gate suite in find-bar.test.tsx (commit 1156d53c2) mounts the real useKeybinds hook (theme context mocked) in a MemoryRouter and drives an actual mod+f keydown through the registered combo index:

  • /settings$findInPage.active stays false (gate suppresses)
  • /session/a$findInPage.active becomes true (normal path intact)

Mutation check: reverting the handler to 'view.findInPage': openFindBar makes exactly the /settings test fail while the chat-route test still passes — the test bites.

Verification: find-bar.test.tsx 51/51, shell+contrib 65/65, tsc --noEmit clean. Rebased onto current main (it had gone CONFLICTING; the only conflict was main's react-router import migration).

Note on citations: the review's line refs were against the pre-rebase base — on current main they're wiring.tsx:976/981, titlebar-controls.tsx:262, styles.css:471. Substance unchanged.

@teknium1 teknium1 added the sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users label Jul 30, 2026
Three fixes for the find-in-page bar (Ctrl+F):

1. Position: replace fixed right-4 with calc() that accounts for the
   titlebar tool cluster width, preventing visual overlap with the
   layout/haptics/keybinds/settings icons. Uses existing CSS vars from
   wiring.tsx (--titlebar-tools-right, --titlebar-tools-width).

   right-[calc(var(--titlebar-tools-right,0.75rem)+var(--titlebar-tools-width,0px)+0.5rem)]

2. Overlay guard: hide the find bar on full-screen overlay routes
   (agents, command-center, cron, profiles, settings, starmap,
   webhooks), matching the titlebar controls pattern. Prevents the
   bar from rendering behind overlays at z-50 and avoids collision
   with overlay-specific search surfaces (e.g. Settings search NousResearch#69025).

3. Keybind gate: suppress the view.findInPage keybind on overlay
   routes so Ctrl+F doesn't mutate store state with no visible effect.
   Defense-in-depth alongside the component guard.

Adds regression test asserting the find bar does not render on /settings.
@DavidMetcalfe
DavidMetcalfe force-pushed the fix/find-bar-positioning branch from 06255b1 to a43933b Compare August 5, 2026 06:08
…files

- use-keybinds.ts: sort @/app/routes after @/app/chat/close-tab and the
  right-sidebar imports (natural-asc)
- find-bar.test.tsx: sort @/app/hooks/use-keybinds before @/components and
  @/i18n imports; KeybindRuntimeDeps before useKeybinds in named imports
teknium1 added a commit that referenced this pull request Aug 15, 2026
KeybindRuntimeDeps grew archiveSelectedSession on main after #72959 was
opened; the salvaged keybind-gate harness needs the new field to typecheck.
teknium1 added a commit that referenced this pull request Aug 15, 2026
…ol-count

fix(desktop): count all five static titlebar buttons; find-bar overlay guard (salvage #72959)
@teknium1

Copy link
Copy Markdown
Contributor

Salvaged into #86847 (merged) — your SYSTEM_TOOL_COUNT undercount diagnosis was verified correct on current main (the static cluster renders 5 buttons: 4 systemTools + the always-present right-sidebar toggle) and landed with your authorship preserved across all cherry-picked commits, along with the overlay render guard and keybind gate. The find-bar horizontal offset half was dropped as obsoleted by #86746. Thanks @DavidMetcalfe!

@teknium1 teknium1 closed this Aug 15, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/desktop Electron desktop app (apps/desktop/*) P3 Low — cosmetic, nice to have sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants