Repository navigation
feat(desktop): remember page zoom and add a sidebar zoom control on macOS and Linux - #6335
sungyongcho wants to merge 3 commits into
Conversation
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. 🧰 Additional context used📚 Code guidelines (2)No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: lidge-jun/opencodex/.coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (23)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthroughThe dashboard adds persistent zoom management for macOS and Linux. It supports keyboard shortcuts, Ctrl+wheel, and sidebar controls. Windows continues to use browser-engine zoom. Tests and documentation cover the behavior and compatibility with older proxies. ChangesDesktop zoom
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Feature Sequence Diagram(s)sequenceDiagram
participant App
participant useDesktopZoom
participant desktopZoom
participant BrowserStorage
participant TauriWebview
App->>useDesktopZoom: initialize with managed status
useDesktopZoom->>desktopZoom: readSavedZoom
desktopZoom->>BrowserStorage: read ocx-desktop-zoom
useDesktopZoom->>desktopZoom: applyWebviewZoom
desktopZoom->>TauriWebview: invoke set_webview_zoom
useDesktopZoom->>desktopZoom: writeSavedZoom after zoom change
desktopZoom->>BrowserStorage: save ocx-desktop-zoom
Merge Risk: 🔵 Low · up to This adds remembered zoom and a sidebar zoom control on macOS and Linux. No concrete defects were found. Real-hardware zoom event ordering in a packaged app is unverified, so a quick manual check before release is advisable. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The change is limited to desktop presentation settings and uses the existing zoom permission. No introduced security vulnerability was established. Risk remains low rather than minimal because packaged-runtime permission enforcement has not been verified. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 44.44% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 18 functions across 19 files. (4 skipped: 4 unsupported.)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
✅ Deterministic PR hygiene checks passed. |
✅ READY
Review readiness checklist
✅ 4/4 boxes ticked. This pull request is already Ready for Review. Hygiene✅ Deterministic PR hygiene checks passed. |
|
Thanks for this. Maintainer review found a version-skew problem, so this stays open until zoom ownership is negotiated:
A small capability handshake (shell announces it delegates zoom; dashboard only takes over when it sees that) plus version-skew regression tests would resolve both. No security concerns found. |
0e362bd to
c3e5eae
Compare
|
Thanks, both cases are real, and I had the premise wrong. The dashboard comes from the runtime and the shell is the installed app, so they can differ: a declined takeover attaches to an older runtime, and an app update leaves the shell newer than a service that has not restarted yet. Fixed in 3e62096 (rebased onto the current
I am not attached to this approach. If you would prefer an explicit capability token from the shell (for example in the user agent), tell me and I will switch to that. Not verified: a packaged app, and the order in which real WebKitGTK fires |
c3e5eae to
3e62096
Compare
) Desktop page zoom reset across launches and clipped sidebar footer controls. Carry persisted macOS/Linux zoom and capture-phase events that preempt older shell hotkeys. Keep native shell defaults, Windows behavior, localization, and the existing IPC permission unchanged. Carries lidge-jun#6335 by @sungyongcho. Co-authored-by: sungyongcho <46742040+sungyongcho@users.noreply.github.com>
|
Superseded by the integration in #6487, with reviewed follow-up fixes in #6490 and Windows validation repairs in #6494/#6495, all merged into Desktop zoom persistence and sidebar controls were carried, including the capture-phase behavior and locale coverage. Packaged-user acceptance limits remain documented separately. Original carry commit: Closing this PR as superseded, not claiming that its original head was merged. Thank you for the contribution. |
2 participants
Summary
The desktop window forgot its page zoom every time the app restarted, and had no visible zoom control. On macOS and Linux the level came from Tauri's zoom-hotkey polyfill (#5737), which keeps it in
let zoomLevel = 1inside the injected script. That value restarts at 1 on every page load, so the level was never saved, and after the hop from the bootstrap page to the dashboard the first shortcut jumped from the real level to 120%.The dashboard now owns zoom on macOS and Linux:
gui/src/lib/desktop-zoom.ts(new): the same keys as before (Cmd on macOS, Ctrl on Linux, with+,-,0, plus Ctrl + wheel), 50%-300% in 10% steps on whole percents, the remembered level inlocalStorage, and the one call toplugin:webview|set_webview_zoomthatcapabilities/dashboard-zoom.jsonalready grants to the loopback dashboard. No new IPC permission is added.gui/src/use-desktop-zoom.ts(new): applies the remembered level when the dashboard starts, so a restart or a page navigation cannot leave the webview at a level the dashboard does not know, then follows the shortcuts and the control and saves each change. Touchpad pinch deltas accumulate before stepping instead of stepping on every event.gui/src/components/desktop-zoom-control.tsx(new) andApp.tsx: a- 130% +row in the sidebar footer beside the theme and language rows, shown only in the desktop shell on macOS and Linux. Clicking the percentage resets to 100%. Strings are in all ten locale files (French keeps "Zoom", added to the intentional-English list infr-localization.test.ts).desktop/. The shell keeps Tauri's zoom hotkeys on (an earlier revision of this PR switched them off on macOS and Linux; the review pointed out why that is wrong, see below). On Windows WebView2 keeps its native zoom, which is not remembered, and the sidebar control is hidden there.stopImmediatePropagation(): Tauri's polyfill listens onwindowin the bubble phase, so a dashboard that handles zoom is the only writer under any shell version, an older dashboard has no handler and the polyfill keeps working, and an older shell's polyfill is pre-empted by a newer dashboard. No handshake is involved. The legacymousewheelevent the polyfill uses is silenced as well and not counted, so one gesture is one step. Alt chords are handled like the polyfill handled them, so they cannot sneak a second write past the dashboard. If maintainers prefer an explicit capability handshake (for example a user-agent token the shell announces), that is a small change on top.gui/src/styles/sidebar-zoom.css(new, imported frommain.tsx; it is kept out ofstyles.css, which is under a file-size cap that only moves down): the control's styles, and on the desktop layout (761px and up).sidebar navis now the scrolling region, so the footer stays on screen. Without it the new control was unreachable exactly when it is needed: page zoom shrinks the CSS viewport (1100x720 becomes 846x554 at 130%), the sidebar is a fixed-height column that does not scroll, and the language, theme, zoom and proxy rows were pushed out of the window (screenshot 1). The narrow layout is untouched because its drawer already scrolls as a whole. Thepadding: 4px; margin: -4pxpair stops the scroll box from clipping focus rings and moves nothing.gui/design-system/components.mdrecords the contract.structure/desktop-shell.mdand a Zoom section indocs-site/.../guides/desktop-app.mddescribe the new ownership and what an app attached to an older proxy does (earlier behaviour: 20% steps, not remembered, no sidebar control).Screenshots of the real dashboard at the app's default window size, in both themes, at 100% and 130%, plus the clipped footer before the change, are attached below.
Verification
cd gui && bun run build,bun run lintandbun run lint:i18n: pass.gui/tests/desktop-zoom.test.ts(16 tests: steps and clamping, storage failures, key bindings per host, Alt chords, wheel accumulation) andgui/tests/desktop-zoom-dom.test.tsx(13 tests against a stand-in shell that records its commands: the remembered level is applied on start, Ctrl plus moves and saves it, Ctrl zero resets, the buttons step, the ceiling disables plus, unmanaged hosts are left alone, and six version-skew cases). The skew cases register a stand-in for the polyfill before the dashboard renders, the way the shell's injected script is registered, then check that a newer dashboard is the only writer for keys, Alt chords and Ctrl + wheel, that an older dashboard that does not manage zoom leaves the polyfill working, and that non-zoom keys and a plain wheel turn still reach every listener. Switching the listeners back to the bubble phase, and removing the propagation stop, each made three of them fail; both were restored. (My first version of these tests registered the stand-in after the dashboard, which let the bubble phase pass by accident; the registration order is what the tests now pin.)devat f86ad0a.cd gui && bun test --isolate tests: 2747 pass, 0 fail across 314 files. While developing, the first DOM test leftlocalStoragenon-writable for later files in a shared process and broke 14 of them; it now keeps its overrides writable.bun run structure:checkandbun run privacy:scan: pass.desktop/is identical todev, so there is no Rust to format or compile.npx react-doctor@0.9.11 --verbose --scope changed(the version the repo'sguiscript pins) againstdev: 19 files scanned, no issues. The CI run of the same check is waiting for a maintainer to approve workflows for a first-time contributor, so this local run is the only evidence for it so far.bun run typecheckpasses, and so do the two extratscruns in the CI Typecheck step (tests/tsconfig.doctor-service-memory-contract.jsonandscripts/ci/docker-smoke.ts). The size ratchet caughtgui/src/styles.cssgrowing past its cap, which is why the styles moved to a new file.bun run test: 34936 pass, 77 skip, 93 fail, all 93 in 17 files this PR does not touch (Lab, Kiro catalogue, serving runtimes, packaged startup probe, and a few CLI and Codex integration files). Rerunning those 17 files at thedevtip without this PR gives the same 93 failures in the same files, so they are not caused by this change. My machine has no standalone Bun, so the suite runs through the packagedocxbinary in Bun mode, which at least explains the packaged-probe failure (Script not found "cached"); I did not chase the rest.wheelagainst the legacymousewheelevent on real hardware (the code silences both and counts onlywheel, so it does not depend on the order, but I could only exercise it in happy-dom, whoseWheelEventdrops the modifier flags, so the tests setctrlKeyon the instance). The screenshots are the built dashboard opened in headless Chrome with the desktop shell's user agent and a stand-in for the shell's zoom command; page zoom was reproduced by shrinking the CSS viewport to 1100/z by 720/z with a device scale factor of z, which is what the webview does. They are not the packaged app. Windows is untouched.Checklist
Screenshots
The five screenshots are added to this description in the web editor, in this order:
1-before-dark-130-footer-clipped.png: 130% before the sidebar change. Only "English" is left of the footer; theme, zoom, proxy and GitHub are cut off.2-after-dark-100.png: 100%, dark. The zoom row sits between the theme and proxy rows.3-after-dark-130.png: 130%, dark. The menu scrolls and the whole footer, including- 130% +, stays visible.4-after-light-100.png: 100%, light.5-after-light-130.png: 130%, light.Review readiness checklist
This PR stays in draft until every box below is ticked. Tick all four boxes once the requirements are met:
Required local validation passed; commands, results, and any full-suite exception are documented.
I pushed my PR to a recent dev commit (at most 10 behind; a maintainer may still ask for the exact tip before merge).
I resolved all correct Codex and CodeRabbit findings.
My PR is ready for review.
Summary by CodeRabbit