From 39230d17384cb99741aa8a0fe7044ca874b2e2f6 Mon Sep 17 00:00:00 2001 From: Brooklyn Nicholson Date: Mon, 13 Jul 2026 17:39:03 -0400 Subject: [PATCH] refactor(desktop): funnel zoom apply+notify so restore can't desync MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit alelpoan's fix (emit hermes:zoom:changed after restore) is correct, but the bug's root is duplication: setAndPersistZoomLevel and restorePersistedZoomLevel each independently did setZoomLevel + send, and restore forgot the send. Collapse both (and the lifecycle re-assert) into a single applyZoomLevel() helper in zoom.ts that always applies-then-notifies — the regression can't recur by omitting a send. Replace the source-grep test (which broke on main: the sibling source-assertion pet test it copied was refactored to a behavioral one, dropping the fs/path imports it relied on) with behavioral coverage of the funnel, matching zoom.ts's "unit-testable without booting a BrowserWindow" convention. Co-authored-by: alelpoan --- apps/desktop/electron/main.ts | 16 ++++---- apps/desktop/electron/zoom.test.ts | 60 ++++++++++++++++++------------ apps/desktop/electron/zoom.ts | 16 ++++++++ 3 files changed, 59 insertions(+), 33 deletions(-) diff --git a/apps/desktop/electron/main.ts b/apps/desktop/electron/main.ts index 3dcbd85a585e..e89e369bacee 100644 --- a/apps/desktop/electron/main.ts +++ b/apps/desktop/electron/main.ts @@ -4705,7 +4705,7 @@ function installPreviewShortcut(window) { // process owns setZoomLevel, so we mirror each change into localStorage and // read it back on did-finish-load to re-apply after reloads or crash recovery. import { - clampZoomLevel, + applyZoomLevel, installZoomReassertOnWindowEvents, percentToZoomLevel, ZOOM_STORAGE_KEY, @@ -4718,11 +4718,9 @@ function setAndPersistZoomLevel(window, zoomLevel) { return } - const next = clampZoomLevel(zoomLevel) - window.webContents.setZoomLevel(next) - // Keep any open settings UI in sync, including changes made via the - // keyboard shortcuts or the View menu. - window.webContents.send('hermes:zoom:changed', { level: next, percent: zoomLevelToPercent(next) }) + // Apply + notify in one funnel so the settings UI stays in sync, including + // changes made via the keyboard shortcuts or the View menu. + const next = applyZoomLevel(window.webContents, zoomLevel) window.webContents .executeJavaScript( `try { localStorage.setItem(${JSON.stringify(ZOOM_STORAGE_KEY)}, ${JSON.stringify(String(next))}) } catch {}` @@ -4744,9 +4742,9 @@ function restorePersistedZoomLevel(window) { return } - const level = clampZoomLevel(Number(stored)) - window.webContents.setZoomLevel(level) - window.webContents.send('hermes:zoom:changed', { level, percent: zoomLevelToPercent(level) }) + // Notify the renderer too — otherwise the Appearance UI Scale control + // can stay stuck at 100% even though the window zoom was restored. + applyZoomLevel(window.webContents, Number(stored)) }) .catch(error => rememberLog(`[zoom] restore failed: ${error?.message || error}`)) } diff --git a/apps/desktop/electron/zoom.test.ts b/apps/desktop/electron/zoom.test.ts index b158456dafe9..d0a1517d38e5 100644 --- a/apps/desktop/electron/zoom.test.ts +++ b/apps/desktop/electron/zoom.test.ts @@ -9,6 +9,7 @@ import assert from 'node:assert/strict' import { test } from 'vitest' import { + applyZoomLevel, clampZoomLevel, installZoomReassertOnWindowEvents, percentToZoomLevel, @@ -118,31 +119,42 @@ test('unknown window kinds default to chat (zoom enabled)', () => { assert.deepEqual(zoomWiringForWindowKind('unknown'), { zoom: true }) assert.deepEqual(zoomWiringForWindowKind(undefined), { zoom: true }) }) -// Source assertion, same pattern as the pet-overlay test above: verifies the -// fix for the UI Scale settings control drifting out of sync after a restart. -// restorePersistedZoomLevel correctly re-applies the persisted zoom level to -// the window, but must also notify the renderer — otherwise the renderer's -// $zoomPercent store (see store/zoom.ts), which only updates from zoom.get() -// (called once on load) and 'hermes:zoom:changed' events, can be left with a -// stale value if zoom.get() resolves before the restore finishes, leaving the -// Appearance settings UI Scale control showing the wrong preset even though -// the window's actual zoom level was correctly restored. -test('restorePersistedZoomLevel notifies the renderer, same as setAndPersistZoomLevel', () => { - const electronDir = path.dirname(fileURLToPath(import.meta.url)) - const source = fs.readFileSync(path.join(electronDir, 'main.ts'), 'utf8').replace(/\r\n/g, '\n') - const restoreStart = source.indexOf('function restorePersistedZoomLevel(window)') - assert.notEqual(restoreStart, -1, 'missing restorePersistedZoomLevel') +// The UI Scale settings control drifts out of sync after a restart when zoom +// is applied to the window but the renderer is never told: its $zoomPercent +// store (see store/zoom.ts) only updates from zoom.get() (once, on load) and +// 'hermes:zoom:changed' events. applyZoomLevel is the single funnel every zoom +// path (user set, restore-on-load, lifecycle re-assert) shares, so applying a +// level always notifies — the regression can't come back by forgetting a send. +function fakeWebContents() { + const calls: Array<[string, ...unknown[]]> = [] - const snippet = source.slice(restoreStart, restoreStart + 600) - const setZoomIndex = snippet.indexOf('window.webContents.setZoomLevel(level)') - const sendIndex = snippet.indexOf("window.webContents.send('hermes:zoom:changed'") + return { + calls, + setZoomLevel: (level: number) => calls.push(['setZoomLevel', level]), + send: (channel: string, payload: unknown) => calls.push(['send', channel, payload]) + } +} - assert.notEqual(setZoomIndex, -1, 'restorePersistedZoomLevel must apply the restored zoom level') - assert.notEqual( - sendIndex, - -1, - 'restorePersistedZoomLevel must notify the renderer after restoring zoom, or the UI Scale control can show a stale preset' - ) - assert.ok(sendIndex > setZoomIndex, 'the renderer notification must happen after the zoom level is applied') +test('applyZoomLevel applies the level then notifies the renderer', () => { + const wc = fakeWebContents() + const applied = applyZoomLevel(wc, 3) + + assert.equal(applied, 3) + assert.deepEqual(wc.calls, [ + ['setZoomLevel', 3], + ['send', 'hermes:zoom:changed', { level: 3, percent: zoomLevelToPercent(3) }] + ]) +}) + +test('applyZoomLevel clamps garbage before applying and notifying', () => { + const wc = fakeWebContents() + const applied = applyZoomLevel(wc, 999) + const clamped = clampZoomLevel(999) + + assert.equal(applied, clamped) + assert.deepEqual(wc.calls, [ + ['setZoomLevel', clamped], + ['send', 'hermes:zoom:changed', { level: clamped, percent: zoomLevelToPercent(clamped) }] + ]) }) diff --git a/apps/desktop/electron/zoom.ts b/apps/desktop/electron/zoom.ts index d4d769b7f636..baf8ff48b70e 100644 --- a/apps/desktop/electron/zoom.ts +++ b/apps/desktop/electron/zoom.ts @@ -32,6 +32,22 @@ export function percentToZoomLevel(percent) { return clampZoomLevel(Math.log(percent / 100) / Math.log(ZOOM_FACTOR_BASE)) } +/** + * Apply a clamped zoom level to a webContents AND notify the renderer, in that + * order. Every path that changes zoom (user action, restore-on-load, lifecycle + * re-assert) funnels through here so the settings UI Scale control can never + * drift from the actually-applied level — the bug where restore set the level + * but forgot to emit 'hermes:zoom:changed', leaving the control stuck at 100%. + * Returns the clamped level so callers can persist it. + */ +export function applyZoomLevel(webContents, level) { + const clamped = clampZoomLevel(level) + webContents.setZoomLevel(clamped) + webContents.send('hermes:zoom:changed', { level: clamped, percent: zoomLevelToPercent(clamped) }) + + return clamped +} + // Chromium on Windows can drop webContents zoom when a BrowserWindow is minimized // and restored. Re-apply the persisted level on these lifecycle transitions. export const ZOOM_REASSERT_WINDOW_EVENTS = ['show', 'restore']