Repository navigation
Conversation
Generated-by: Codex
jackwener
left a comment
There was a problem hiding this comment.
I reviewed exact head bba94e7fa88a56748ea85d74bebe67684337723c.
No P0–P2 findings. The change is appropriately scoped: the Lightbox close control continues to use Astryx's existing 12 px inset when the Window Controls Overlay reports no right-hand obstruction, and adds only the measured right-hand overlay width on the shipped left-to-right locales.
I exercised the real Image Thumbnails Storybook surface against the built Astryx 0.6.3 DOM. With a simulated 138 px Windows caption strip, the current rule moved the 32 px close button from x=1236–1268 to x=1098–1130, clearing a strip that began at x=1142. Overriding the rule back to the previous 12 px inset reproduced the overlap immediately. The zero-width path retained the original 12 px gap. The exact-head Storybook build and Biome check passed, and the hosted exact-head test check is successful.
The current main head is 0d19f242336a31e8aeb300372d3d3c896b1e99ef. A synthetic merge is clean as tree 2ad06c91d5b0989efe38c56d0da20a6aec854dad; the affected CSS blob is identical to the tested exact head.
One non-blocking test gap remains: the repository does not currently automate the nonzero-overlay assertion above. The existing attachment story exercises only the zero-width path, while the selector also depends on Astryx keeping the close button as the container's first button. Please consider extending that story or the browser smoke to open the Lightbox, inject a nonzero overlay width, and assert that the close control clears the simulated caption strip.
I did not run a native Windows Electron window in this review.
Automated review notice: This comment was posted by an automated review agent operated by jackwener. It is not an independent human review and does not replace one.
Astro-Han
left a comment
There was a problem hiding this comment.
Automated review notice: This comment was posted by an automated review agent. It is not an independent human review and does not replace one.
Reviewed bba94e7fa88a56748ea85d74bebe67684337723c (+13/-0, one stylesheet). I found no confirmed P0–P2 issue. This independently confirms the close-button geometry reported in the existing review, with an additional check of the shipped renderer's direction selector.
At apps/desktop/src/renderer/styles/astryx-mount.css:81–84, the close action adds the existing measured right-hand titlebar obstruction to Astryx's 12px spacing. I traced the actual Astryx 0.6.3 Lightbox DOM, the shared maka-tokens.css Window Controls Overlay calculation, and the main-window platform configuration. The first direct container button is the close action; gallery controls follow it.
Fresh dependencies/patches, the normal clean root test build, normal production renderer build, Storybook build, Biome and the static merge with current main 0d19f242336a31e8aeb300372d3d3c896b1e99ef passed. The PR does not change protocol code or its epoch. Exact-head test is successful.
In Chromium, I opened the real Image Thumbnails → dashboard.png preview and exercised close/reopen and Escape. With a supplied 138px obstruction, the close button ends at x=1130 in a 1280px viewport, leaving 12px before the reserved strip at x=1142; removing only the new rule moves it back to x=1268, inside that strip. The zero-obstruction case retains its 12px gap.
P3, test fidelity: Storybook's default CSS target transforms :dir(ltr) into a language-based selector. Setting dir=rtl while retaining an English document therefore produces a misleading 150px left gap in that build. The production renderer explicitly targets Chromium 150 and preserves :dir(ltr). Installing that exact emitted production rule on the same real component keeps the RTL close action 12px from the left with both zero and 138px right-hand obstructions. A future RTL regression test should use the production CSS target or account for this difference.
The positive obstruction was controlled test input, not a native Windows measurement. A bounded Linux Electron attempt did not produce a test window, so I did not verify native overlay acquisition, Windows caption-button hit testing, macOS traffic lights or a complete application session. Those platform checks remain outside this result.
Summary
Keep the desktop Lightbox close control clear of native Windows caption buttons by adding Maka’s existing measured Window Controls Overlay right-hand inset. The current spacing is preserved when that inset is zero; RTL keeps its close control on the left.
Fixes #6209
Verification
npm run lint,npm run format:check, both requiredknipworkspace checks, the head Storybook build, and staged commit checks passed. Remotetestandlabelchecks pass atbba94e7.npm run buildandnpm run typecheckfailed at unchangedapps/desktop/src/main/runtime-host-desktop-candidate.ts:971with TS2322 (assistant.allocate-workspaceis absent from the accepted operation union).UI evidence
Existing Storybook Image Thumbnails → dashboard.png Lightbox, Chrome on macOS, 1456 × 849, light theme, same fixture and locale. Before:
ff74d21; after:bba94e7. The base build limited only the story catalog to the existing attachment stories; its fixture and renderer CSS were unchanged.These captures cover the zero Window Controls Overlay inset path, where unchanged spacing is expected. They do not reproduce the native Windows caption-button overlap; original Windows overlay visual verification remains unperformed.
AI use
Tool(s) and scope: Codex implemented the styling, inspected its integration, ran the checks, and captured the Storybook UI evidence. The commit includes
Generated-by: Codex.Checklist
Does this PR entail a change in behavior?