Skip to content

fix(desktop): keep the find bar clear of the window controls - #80244

Closed
kiwiwyl wants to merge 2 commits into
NousResearch:mainfrom
kiwiwyl:fix/find-bar-window-controls
Closed

kiwiwyl wants to merge 2 commits into
NousResearch:mainfrom
kiwiwyl:fix/find-bar-window-controls

Conversation

@kiwiwyl

@kiwiwyl kiwiwyl commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

Problem

Pressing Ctrl+F opens the find-in-page bar at top: 8px on Windows, overlapping the native window controls (minimize/maximize/close) and the titlebar tool cluster.

Find bar overlapping the window controls

Impact: the find bar overlaps the main app window's close button (the × at the top-right corner) — a stray click near the bar's buttons can land on the close button instead, closing the entire Hermes window and losing the in-progress draft. This is a real user-facing footgun on every Windows install, not just a cosmetic issue.

Root cause

FindBar mounts outside the app-shell subtree that defines --titlebar-height, so the CSS variable fallback is what actually positions it. The fallback was 0px, pinning the bar onto the titlebar strip:

// before
'pointer-events-auto fixed right-4 top-[calc(var(--titlebar-height,0px)+0.5rem)] z-50',

Fix

Fall back to TITLEBAR_HEIGHT (34px), matching the existing pattern in floating-hud.ts and notifications.tsx:

// after
'pointer-events-auto fixed right-4 top-[calc(var(--titlebar-height,34px)+0.5rem)] z-50',

The bar now renders just below the titlebar band (top: 42px), clear of the window controls.

Verification

  • find-bar.test.tsx: 48/48 tests pass
  • Manually verified on Windows: Ctrl+F no longer overlaps min/max/close

FindBar mounts outside the app-shell subtree that defines
--titlebar-height, so the CSS var fallback is what actually positions
it. The fallback was 0px, pinning the bar at top: 8px — right on the
titlebar strip, overlapping the min/max/close controls (and the
titlebar tool cluster) on Windows. Align with floating-hud and
notifications, which already fall back to TITLEBAR_HEIGHT (34px), so
the bar lands just below the titlebar band.
@alt-glitch alt-glitch added type/bug Something isn't working comp/desktop Electron desktop app (apps/desktop/*) platform/windows Native Windows-specific behavior or breakage P3 Low — cosmetic, nice to have sweeper:risk-platform-windows Sweeper risk: may break or behave differently on native Windows labels Aug 6, 2026
@spfcraze

spfcraze commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

This was generated by AI during triage.

Summary:
This exact fallback edit — var(--titlebar-height,0px)var(--titlebar-height,34px) — is already present in open PR #73467, which changes the same find-bar.tsx line to the same 34px fallback.

Problems:

Solution:
Keeping the fallback edit in only one of the two PRs — with the other referencing it in its description — avoids a merge conflict on the same hunk.


Checked against 724b9d7 — the PR head when this was written.

@vollegrewar

Copy link
Copy Markdown
Contributor

Verified against current main (3139a30e5) — your root-cause analysis is correct and the fallback change does land. Confirming the mount structure precisely:

  • The main shell's --titlebar-height: 0px wrapper (src/app/contrib/controller.tsx:723) is passed as {children} and rendered inside ContribWiring's contents div (src/app/contrib/wiring.tsx:1010);
  • <FindBar /> renders at wiring.tsx:1043 — a sibling of that div — so the variable never reaches it, and the CSS fallback is indeed the effective value: top: 8px today, 42px with this PR, clear of the 34px native overlay strip on Windows.

(For the record: I filed #83192 claiming the opposite reading of the tree; I've corrected the issue body and closed it as covered by this PR.)

Optional suggestion: the current find-bar.test.tsx suite doesn't cover positioning — a small class-level assertion on the container's anchor (e.g. the fallback value) would have caught this overlap and guards against regressions.

@vollegrewar

Copy link
Copy Markdown
Contributor

Hi @teknium1 and @OutThisLife — could one of you take a look at this PR / approve the CI runs?

It has been ready since Aug 6: 2 commits, MERGEABLE, no conflicts, and the fix itself is verified (see my review above — the CSS fallback is the effective value since the bar mounts outside the --titlebar-height wrapper, so the bar moves from top: 8px — under the native window controls on Windows — to 42px).

The blocker is that CI never started: the check-suite for 724b9d7bc0c2 reports action_required, which looks like the first-contributor workflow approval gate (no prior merged PRs from this author). Approving the runs in the Actions tab would unblock it.

Happy to address any review feedback.

@teknium1

Copy link
Copy Markdown
Collaborator

This exact fix (0px → 34px --titlebar-height fallback, same rationale) just landed on main via #86746 — and you submitted it first, 8 days earlier. The duplicate sweep unfortunately happened post-merge instead of pre-build, so your branch didn't get cherry-picked as it should have. Full first-submitter credit to you for the correct diagnosis and fix; closing as implemented on main. Sorry we didn't catch your PR in time to merge it directly.

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 platform/windows Native Windows-specific behavior or breakage sweeper:risk-platform-windows Sweeper risk: may break or behave differently on native Windows type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants