Fix profile button covered by native title bar buttons on Windows#2569
Fix profile button covered by native title bar buttons on Windows#2569imnasnainaec wants to merge 8 commits into
Conversation
The toolbar reserved a fixed 138px for Windows' caption-button cluster, but that guess doesn't hold under DPI scaling or OS text-size settings, so the profile button could render underneath the native (OS-drawn) close button. Reads the real reserved width live via the Window Controls Overlay API instead. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Adds the missing navigation-history/, scroll-groups/, title-bar/, and verse-navigation/ entries to the isolated E2E README's subdirectory list. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Session-URL: <session URL>
50b4f4e to
b5dcca9
Compare
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
…style prop Wrap the toolbar in a local div carrying the live-measured reserved-space padding instead of threading it through platform-bible-react's Toolbar component, avoiding an unnecessary shared-library API change. Suppresses Toolbar's own end-side border and padding when the wrapper takes over so the reservation isn't applied twice, and tunes a small breathing-room margin so content doesn't sit pixel-flush against the native buttons. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Condensed summary of Review summaryFixes the profile button rendering under Windows' native title-bar buttons at non-100% DPI scaling, via a live measurement from the Window Controls Overlay API. Purpose confirmed with the author; scope stayed tight. Findings: no critical issues. One important finding (stale generated-doc drift) is now moot — see below. One minor doc gap was fixed (missing e2e-test README entries). Three Devin AI comments were incorporated: two verified as real but non-currently-exploitable pre-existing limitations (RTL padding assumption, Windows-fullscreen static fallback), left as-is; the third (loss of breathing-room spacing) turned out to predict, in the abstract, a bug the author later found live. Considered and altered: the original implementation added an optional Reverted/fixed along the way: that rework surfaced three visual bugs found by the author through live testing, not static analysis — a stray border artifact, an over-large reserved gap, and (once those were fixed) a too-small one. All three are fixed and covered by tests. Design understanding: the author initially said they were "still reviewing it myself" when asked to explain the design, but went on to personally drive the architecture decision and correctly diagnose all three live bugs before I confirmed the causes — worth a brief check-in at the review meeting rather than reading either signal alone. Quality checks: typecheck, lint, and the full relevant test suite (30/30) are clean, including after the follow-up fixes and a merge from |
The wrapper div introduced for the live-measured reserved space only suppressed Toolbar's own end-side border, leaving the top/bottom border stopping short at Toolbar's narrower edge instead of continuing across the reserved strip. The wrapper now takes over Toolbar's border on all four sides (and Toolbar drops its own entirely) whenever it's carrying the live measurement, so the outline encloses the whole toolbar-plus- reserved-space region again, matching how it looked before the padding moved outside Toolbar's own box. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
katherinejensen00
left a comment
katherinejensen00
left a comment
There was a problem hiding this comment.
AI Code Review — PR #2569 (Live WCO Title-Bar Measurement)
18 findings across correctness, testing, and code quality. Key items (see inline comments for blocking/warning details):
Blocking
test.skip()insidebeforeAll()is a no-op — tests run on all platformspaddingInlineEndreserves space on the wrong physical side in RTL locales- Reserved-space strip has no
WebkitAppRegion— dead zone for window dragging
Warnings
WorkspaceUpdatingOverlayhardcodestop: 48, but wrappertw:borderadds 2 px- Redundant re-render on mount:
useStateinit + effect both allocate freshDOMRect - Hook stuck forever if
windowControlsOverlayisundefinedat first render
Suggestions (summary — no inline comments)
12. E2E test in new subdirectory — e2e-tests/CLAUDE.md says prefer a flat spec file directly in tests/isolated/ over a new folder. tests/isolated/title-bar-reserved-space.spec.ts would run without config changes; the current tests/isolated/title-bar/ subdirectory requires a named project registration in playwright.config.ts to be discovered.
13. Extract triple condition to named boolean — osPlatformToReserveSpaceFor !== undefined && osPlatformToReserveSpaceFor !== 'darwin' && windowControlsOverlayRect encodes 'Windows with a live overlay' but forces readers to parse three clauses. const isWindowsWithLiveOverlay = ... one line above makes the ternary self-documenting.
14. toolbarReservedSpaceStyle allocates a new object every render — when the live-measurement path is active, every re-render of PlatformBibleToolbar produces a fresh { paddingInlineEnd: N }. React re-renders the full Toolbar subtree even when N is unchanged. useMemo(() => ..., [windowControlsOverlayRect, osPlatformToReserveSpaceFor]) would prevent that.
15. windowControlsOverlay in the useEffect dep array is misleading — navigator.windowControlsOverlay is a stable platform property whose identity never changes. Listing it implies the effect might re-run on change; [] with an inline read would be more honest.
16. Fix at the wrong architectural depth — the PR overrides Toolbar's private classes (tw:border, tw:px-4) from the caller. If those defaults change in platform-bible-react, the suppressions silently break. Consider a reservedInlineEnd?: number prop on Toolbar or an optional pixel arg on getToolbarOSReservedSpaceClassName so the measurement lives inside one abstraction.
17. event.titlebarAreaRect declared but never consumed — WindowControlsOverlayGeometryChangeEvent.titlebarAreaRect is declared in the .d.ts, but updateRect calls getTitlebarAreaRect() again. Reading event.titlebarAreaRect avoids the extra method call and aligns with the declared type.
18. addEventListener missing third parameter — the real EventTarget.addEventListener accepts options?: boolean | AddEventListenerOptions. The narrow declaration prevents { once: true } and similar patterns. Same applies to removeEventListener.
| const { windowControlsOverlay } = navigator; | ||
|
|
||
| const [titlebarAreaRect, setTitlebarAreaRect] = useState<DOMRect | undefined>(() => | ||
| windowControlsOverlay?.visible ? windowControlsOverlay.getTitlebarAreaRect() : undefined, |
There was a problem hiding this comment.
Warning — redundant re-render guaranteed on every mount when WCO is visible.
The useState lazy initializer calls getTitlebarAreaRect() at mount → DOMRect A. Then useEffect fires and calls updateRect() → getTitlebarAreaRect() again → DOMRect B (a new object — browsers always construct a fresh DOMRect). React's Object.is comparison sees two different references and schedules an extra re-render of PlatformBibleToolbar even though the geometry is identical. Every cold start triggers one extra toolbar paint.
Fix: either remove the lazy initializer (accept one render with undefined before the effect fires) or skip the updateRect() call in the effect when the state was already set by the initializer.
There was a problem hiding this comment.
Claude:
Fair catch — one extra render on cold start. Not worth the added branching to avoid it, leaving as-is.
| * full screen). | ||
| */ | ||
| export function useWindowControlsOverlay(): DOMRect | undefined { | ||
| const { windowControlsOverlay } = navigator; |
There was a problem hiding this comment.
Warning — hook is structurally stuck if navigator.windowControlsOverlay is undefined at first render.
windowControlsOverlay is captured by destructuring at render time and placed in the dep array. If it is undefined at first render (Storybook, Vitest/jsdom, non-Electron host), the dep array locks in [undefined] and the effect never re-runs. There is no mechanism to watch navigator for property additions, so the hook permanently returns undefined in those environments.
In production Electron with titleBarOverlay: true the API is synchronously available, so this is not a live-app risk — but the hook silently fails in every non-Electron test/preview host, and unit tests must mock the API before render to exercise the live path.
There was a problem hiding this comment.
Claude:
Real, but only affects non-Electron test/preview hosts — in production the API is synchronously available at first render. Leaving as-is.
| } | ||
|
|
||
| const updateRect = () => | ||
| setTitlebarAreaRect( |
There was a problem hiding this comment.
Suggestion — consider reading event.titlebarAreaRect instead of calling getTitlebarAreaRect() again.
WindowControlsOverlayGeometryChangeEvent (declared in window-controls-overlay.d.ts) already carries titlebarAreaRect: DOMRect at fire time. Reading event.titlebarAreaRect in the handler avoids the extra method call and uses the snapshot that triggered the event, which is more semantically aligned with the declared type.
There was a problem hiding this comment.
Claude:
Reasonable, but re-invoking it is cheap and keeps the update path uniform with the initial read. Leaving as-is.
|
Tested on macOS.
They're the same. |
|
Tested in WSL Ubuntu 22.04: |
- Reserve space with physical paddingRight instead of the logical paddingInlineEnd, which would reserve the wrong side in RTL locales - Make the reserved-space wrapper a drag region again (WebkitAppRegion) so the window can still be dragged from that strip - Fix the e2e test.skip() no-op inside beforeAll by moving it to describe scope, so it actually skips on non-Windows - Drop the dead default export, a no-op setState, and return undefined in useWindowControlsOverlay; add the missing options param to the WindowControlsOverlay addEventListener/removeEventListener types - Fix a test comment that named the wrong quantity Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…WorkspaceUpdatingOverlay Swap the wrapper's tw:border for an equivalent inset box-shadow. The wrapper has no explicit height, so a real border added to its layout height whenever the live measurement was active, silently invalidating WorkspaceUpdatingOverlay's hardcoded top offset (a separate, unrelated component). A box-shadow paints the same outline without occupying layout space, so that offset stays correct in every case. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
imnasnainaec
left a comment
imnasnainaec
left a comment
There was a problem hiding this comment.
@merchako @irahopkinson Thanks for the non-Windows tests and screenshots!
@katherinejensen00 Thanks for the review!
All blocking items fixed. One warning fixed and the other two responded to. Most suggestions enacted.
@imnasnainaec reviewed all commit messages and made 13 comments.
Reviewable status: 0 of 7 files reviewed, 12 unresolved discussions (waiting on katherinejensen00).
| const triggerBox = await trigger.boundingBox(); | ||
| expect(triggerBox).not.toBeNull(); | ||
| if (!triggerBox || overlayRight === undefined) return; | ||
|
|
There was a problem hiding this comment.
It's for type-satisfaction.
| const { windowControlsOverlay } = navigator; | ||
|
|
||
| const [titlebarAreaRect, setTitlebarAreaRect] = useState<DOMRect | undefined>(() => | ||
| windowControlsOverlay?.visible ? windowControlsOverlay.getTitlebarAreaRect() : undefined, |
There was a problem hiding this comment.
Claude:
Fair catch — one extra render on cold start. Not worth the added branching to avoid it, leaving as-is.
| } | ||
|
|
||
| const updateRect = () => | ||
| setTitlebarAreaRect( |
There was a problem hiding this comment.
Claude:
Reasonable, but re-invoking it is cheap and keeps the update path uniform with the initial read. Leaving as-is.
|
The new e2e test was written analogously to the existing isolated tests. It still requires human execution/verification as part of review, since the isolated e2e files don't pass for me locally. |
| // OS controls area width 150px + 4px breathing room (RESERVED_SPACE_BREATHING_ROOM_PX) | ||
| expect(screen.getByTestId('toolbar-reserved-space-wrapper')).toHaveStyle({ | ||
| paddingInlineEnd: '154px', | ||
| paddingRight: '154px', |
There was a problem hiding this comment.
I think an AI may have hallucinated the need to switch from logical directions to physical directions.
Issue: This change assumes the window controls (minimize/maximize/close on Windows, the traffic lights on macOS) always sit on a fixed physical edge, and that the previous logical directionality was wrong. That breaks in RTL locales (Arabic, Hebrew), where the OS mirrors the window chrome:
- Windows places the caption buttons on the left in RTL. The Windows TitleBar docs state, "The layout is reversed when the FlowDirection is RightToLeft." You can see this in the Paratext 9 Arabic tutorial videos:
- macOS moves the traffic lights from the top-left to the top-right in RTL:
Because Platform.Bible renders its own title bar in the web layer (not native chrome), the correct tool here is CSS logical direction — dir plus inline-start/inline-end — which flips automatically with the document's text direction. Hard-coding physical left/right defeats that and produces a mirrored, non-native layout in RTL.
Why it matters:
- Chrome position is driven by text direction, not a fixed edge, on every OS we target.
- Applying a physical-direction constant uniformly ignores that each OS mirrors the title bar in RTL.
- The result is misaligned controls and a non-native feel in RTL locales.
Recommended fix:
- Revert to logical directions.
- Drive title-bar layout from the document/window text direction rather than a physical constant.
This applies to all logical→physical regressions in this PR.
4 participants
Summary
On Windows, the toolbar's
UserProfilePopovertrigger can render underneath the OS's native minimize/maximize/close buttons. The toolbar reserves space for those buttons (drawn by Chromium as a true overlay via Electron'stitleBarOverlay, seemain.ts) using a hardcoded guess of138px + 1rem. That guess only holds at 100% DPI with the standard three-button cluster — under display scaling or OS text-size settings the real reserved width can be larger, so the profile button ends up painted over by the native close button.Before:
Replaces the fixed guess with a live measurement from the standard Window Controls Overlay API (
navigator.windowControlsOverlay.getTitlebarAreaRect()), which Chromium keeps accurate across DPI/theme changes and updates via ageometrychangeevent. macOS's fixed-width traffic-lights padding is untouched.After:
Changes
src/renderer/window-controls-overlay.d.ts(new) — ambient types for the Window Controls Overlay API (not yet in TS's DOM lib).src/renderer/hooks/use-window-controls-overlay.hook.ts(new) — reads the live titlebar rect and updates ongeometrychange.src/renderer/components/platform-bible-toolbar.tsx— wraps<Toolbar>in a local<div>carrying the live-measuredpaddingInlineEndplus a small tuned breathing-room margin (RESERVED_SPACE_BREATHING_ROOM_PX = 4). SuppressesToolbar's own end-side border/padding (tw:border-e-0 tw:pe-0) when the wrapper is active, so reserved space isn't applied twice.lib/platform-bible-reactis untouched.platform-bible-toolbar.test.tsxcoverage, and a new Windows-gated e2e spec (e2e-tests/tests/isolated/title-bar/) asserting the profile button's bounding box stays inside the live overlay rect and that clicking it actually opens the popover.AI Involvement
Diagnosed and implemented with Claude Code (Sonnet 5): traced the bug to the hardcoded
138pxreservation, implemented thenavigator.windowControlsOverlay-based fix, and wrote the accompanying unit/e2e tests.Testing
tscclean on the main app andplatform-bible-reacttitle-bar-reserved-space.spec.ts) run against a live Windows instance — needs CDP connection to a running appRisk Level
Low — isolated to toolbar layout on Windows; falls back to the previous static class whenever the new API is unavailable or not yet resolved, so behavior on macOS/Linux and non-Electron test environments is unchanged.
Co-authored-by: Claude Sonnet 5 noreply@anthropic.com
Devin review: https://app.devin.ai/review/paranext/paranext-core/pull/2569
This change is