Skip to content

fix(desktop): keep lightbox close clear of window controls - #6212

Open
Dante-dan wants to merge 1 commit into
apache:mainfrom
Dante-dan:fix/6209-lightbox-caption-inset
Open

Dante-dan wants to merge 1 commit into
apache:mainfrom
Dante-dan:fix/6209-lightbox-caption-inset

Conversation

@Dante-dan

@Dante-dan Dante-dan commented Oct 21, 2026 •

Copy link
Copy Markdown
Contributor

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 required knip workspace checks, the head Storybook build, and staged commit checks passed. Remote test and label checks pass at bba94e7.
  • npm run build and npm run typecheck failed at unchanged apps/desktop/src/main/runtime-host-desktop-candidate.ts:971 with TS2322 (assistant.allocate-workspace is 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.

Before After
Before: base browser Lightbox After: PR head browser Lightbox

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

  • No generative tool made a substantive contribution
  • Generative tooling made a substantive contribution

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

  • Tests cover the change and fail without it
  • Lint, format, typecheck and the affected suites pass locally

Does this PR entail a change in behavior?

  • Yes — described under Summary above
  • No

@github-actions github-actions Bot added the effort/S Under 100 readable lines label Oct 21, 2026
@Dante-dan
Dante-dan marked this pull request as ready for review October 21, 2026 10:47

@jackwener jackwener left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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 Astro-Han 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.

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.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/S Under 100 readable lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug(desktop): keep the Windows image-preview close button clear of native title-bar controls

3 participants