feat(window): persist UI zoom level across restarts#3017
Conversation
|
React Doctor found no issues in the changed files. 🎉 Reviewed by React Doctor for commit |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f473a6bf8f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Reviews (1): Last reviewed commit: "feat(window): persist UI zoom level acro..." | Re-trigger Greptile |
pauldambra
left a comment
There was a problem hiding this comment.
Note
🤖 Automated comment by QA Swarm — not written by a human
Multi-perspective review: paul-reviewer, xp-reviewer, security-audit (qa-team skipped — not installed in this repo). See inline comments.
|
Note 🤖 Automated comment by QA Swarm — not written by a human Multi-perspective review: paul-reviewer, xp-reviewer, security-audit (qa-team skipped — not installed in this repo) Verdict: 💬 APPROVE WITH NITSClean, focused change. No security or blocking correctness issues. One genuine bug worth fixing: the in-app-reload restore re-applies the zoom level captured at window-create time, so it doesn't actually preserve session zooming across a reload. Key findings
Convergence
Reviewer summaries
Automated by QA Swarm — not a human review |
Address qa-swarm review on #3017: - did-finish-load now reads the latest persisted zoomLevel from the store instead of the create-time snapshot, so zooming done during a session survives in-app reloads. - Clamp the zoom level (ZOOM_MIN/ZOOM_MAX) so a runaway accelerator can't persist an unusable zoom across restarts. - Extract the ZOOM_STEP constant in place of the repeated 0.5 literals. Generated-By: PostHog Code Task-Id: 20eca4a9-7e69-4258-b681-5b60caae5a47
The View-menu zoom items used Electron's built-in zoomIn/zoomOut/resetZoom roles, which only adjust Chromium's in-memory per-webContents zoom and expose no click hook to persist it. Zoom therefore reset to 100% on every relaunch. Store the zoom level alongside the existing window state, re-apply it on did-finish-load (covering fresh launches and in-app reloads), and persist both menu-driven and wheel/pinch zoom changes. Generated-By: PostHog Code Task-Id: 20eca4a9-7e69-4258-b681-5b60caae5a47
Address qa-swarm review on #3017: - did-finish-load now reads the latest persisted zoomLevel from the store instead of the create-time snapshot, so zooming done during a session survives in-app reloads. - Clamp the zoom level (ZOOM_MIN/ZOOM_MAX) so a runaway accelerator can't persist an unusable zoom across restarts. - Extract the ZOOM_STEP constant in place of the repeated 0.5 literals. Generated-By: PostHog Code Task-Id: 20eca4a9-7e69-4258-b681-5b60caae5a47
0f59fb8 to
0ba211e
Compare
There was a problem hiding this comment.
No showstoppers: all substantive reviewer concerns (reload-reset bug, missing zoom clamp, unnamed step constant) are addressed in the current diff. Remaining comments are low-severity style suggestions. The circular dep between menu.ts and window.ts is real but runtime-safe for function-only imports in a bundled Electron main process.
menu.ts importing saveZoomLevel from window.ts created a circular dependency (window.ts already imports buildApplicationMenu from menu.ts). saveZoomLevel only wraps windowStateStore.set, so it belongs beside the store; both menu.ts and window.ts now import it from utils/store. Resolves greptile's circular-dependency note on #3017. Generated-By: PostHog Code Task-Id: 20eca4a9-7e69-4258-b681-5b60caae5a47
New commits pushed (delta classified non_trivial_delta) — stamphog approval dismissed; re-review running automatically.
|
Note 🤖 Automated comment by PR Shepherd — not written by a human Triaged the automated review threads against the latest commits:
Deferred (your call): the broader "consolidate all zoom logic into a dedicated module" suggestion — the current split is small and clear, so left as-is.
|
There was a problem hiding this comment.
All substantive reviewer concerns (reload-reset bug, missing zoom clamp, unnamed step constant) are addressed in the current diff. The unresolved greptile comment describes an earlier version; the actual code reads zoom from the live store on reload, not a stale snapshot. Changes are correctly scoped to the Electron host layer.
There was a problem hiding this comment.
No showstoppers. The substantive reviewer concerns (reload-reset bug, missing clamp, unnamed constant) are all addressed in the current diff. The circular-dependency risk is gone because both files now import through store.ts. The change is correctly confined to the Electron host layer.
Problem
Zooming the UI with
CMD++/CMD+-/CMD+0worked during a session, but the zoom reset to 100% on every relaunch.The View-menu zoom items used Electron's built-in
zoomIn/zoomOut/resetZoomroles, which only adjust Chromium's in-memory per-webContentszoom and expose noclickhook to persist it. Nothing saved the level.Change
Give zoom the same treatment as the existing window-state persistence (bounds/maximized), all in
apps/code/src/main/:utils/store.ts— addzoomLeveltoWindowStateSchemaand thewindowStateStoredefaults (0= 100%).window.ts— read the saved level ingetSavedWindowState(); addsaveZoomLevel(); re-apply the level ondid-finish-load(covers fresh launches and in-app reloads, which reset Chromium's zoom); persist wheel/pinch zoom viazoom-changed.menu.ts— replace the built-in zoom roles with custom items backed by a sharedapplyZoom()helper that sets and persists the level. KeepsCMD++,CMD+=(hidden duplicate, matching the built-in role's dual accelerator),CMD+-, andCMD+0.Verification
pnpm --filter code typecheckpasses.zoomLevelis written to<userData>/window-state.json; survivesCMD+Shift+Rreload.Created with PostHog Code