mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-07-28 18:19:28 +00:00
refactor(desktop): funnel zoom apply+notify so restore can't desync
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 <alelpoan@proton.me>
This commit is contained in:
parent
9f7a3cb1f6
commit
39230d1738
3 changed files with 59 additions and 33 deletions
|
|
@ -4705,7 +4705,7 @@ function installPreviewShortcut(window) {
|
||||||
// process owns setZoomLevel, so we mirror each change into localStorage and
|
// 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.
|
// read it back on did-finish-load to re-apply after reloads or crash recovery.
|
||||||
import {
|
import {
|
||||||
clampZoomLevel,
|
applyZoomLevel,
|
||||||
installZoomReassertOnWindowEvents,
|
installZoomReassertOnWindowEvents,
|
||||||
percentToZoomLevel,
|
percentToZoomLevel,
|
||||||
ZOOM_STORAGE_KEY,
|
ZOOM_STORAGE_KEY,
|
||||||
|
|
@ -4718,11 +4718,9 @@ function setAndPersistZoomLevel(window, zoomLevel) {
|
||||||
return
|
return
|
||||||
}
|
}
|
||||||
|
|
||||||
const next = clampZoomLevel(zoomLevel)
|
// Apply + notify in one funnel so the settings UI stays in sync, including
|
||||||
window.webContents.setZoomLevel(next)
|
// changes made via the keyboard shortcuts or the View menu.
|
||||||
// Keep any open settings UI in sync, including changes made via the
|
const next = applyZoomLevel(window.webContents, zoomLevel)
|
||||||
// keyboard shortcuts or the View menu.
|
|
||||||
window.webContents.send('hermes:zoom:changed', { level: next, percent: zoomLevelToPercent(next) })
|
|
||||||
window.webContents
|
window.webContents
|
||||||
.executeJavaScript(
|
.executeJavaScript(
|
||||||
`try { localStorage.setItem(${JSON.stringify(ZOOM_STORAGE_KEY)}, ${JSON.stringify(String(next))}) } catch {}`
|
`try { localStorage.setItem(${JSON.stringify(ZOOM_STORAGE_KEY)}, ${JSON.stringify(String(next))}) } catch {}`
|
||||||
|
|
@ -4744,9 +4742,9 @@ function restorePersistedZoomLevel(window) {
|
||||||
return
|
return
|
||||||
}
|
}
|
||||||
|
|
||||||
const level = clampZoomLevel(Number(stored))
|
// Notify the renderer too — otherwise the Appearance UI Scale control
|
||||||
window.webContents.setZoomLevel(level)
|
// can stay stuck at 100% even though the window zoom was restored.
|
||||||
window.webContents.send('hermes:zoom:changed', { level, percent: zoomLevelToPercent(level) })
|
applyZoomLevel(window.webContents, Number(stored))
|
||||||
})
|
})
|
||||||
.catch(error => rememberLog(`[zoom] restore failed: ${error?.message || error}`))
|
.catch(error => rememberLog(`[zoom] restore failed: ${error?.message || error}`))
|
||||||
}
|
}
|
||||||
|
|
|
||||||
|
|
@ -9,6 +9,7 @@ import assert from 'node:assert/strict'
|
||||||
import { test } from 'vitest'
|
import { test } from 'vitest'
|
||||||
|
|
||||||
import {
|
import {
|
||||||
|
applyZoomLevel,
|
||||||
clampZoomLevel,
|
clampZoomLevel,
|
||||||
installZoomReassertOnWindowEvents,
|
installZoomReassertOnWindowEvents,
|
||||||
percentToZoomLevel,
|
percentToZoomLevel,
|
||||||
|
|
@ -118,31 +119,42 @@ test('unknown window kinds default to chat (zoom enabled)', () => {
|
||||||
assert.deepEqual(zoomWiringForWindowKind('unknown'), { zoom: true })
|
assert.deepEqual(zoomWiringForWindowKind('unknown'), { zoom: true })
|
||||||
assert.deepEqual(zoomWiringForWindowKind(undefined), { 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)')
|
// The UI Scale settings control drifts out of sync after a restart when zoom
|
||||||
assert.notEqual(restoreStart, -1, 'missing restorePersistedZoomLevel')
|
// 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)
|
return {
|
||||||
const setZoomIndex = snippet.indexOf('window.webContents.setZoomLevel(level)')
|
calls,
|
||||||
const sendIndex = snippet.indexOf("window.webContents.send('hermes:zoom:changed'")
|
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')
|
test('applyZoomLevel applies the level then notifies the renderer', () => {
|
||||||
assert.notEqual(
|
const wc = fakeWebContents()
|
||||||
sendIndex,
|
const applied = applyZoomLevel(wc, 3)
|
||||||
-1,
|
|
||||||
'restorePersistedZoomLevel must notify the renderer after restoring zoom, or the UI Scale control can show a stale preset'
|
assert.equal(applied, 3)
|
||||||
)
|
assert.deepEqual(wc.calls, [
|
||||||
assert.ok(sendIndex > setZoomIndex, 'the renderer notification must happen after the zoom level is applied')
|
['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) }]
|
||||||
|
])
|
||||||
})
|
})
|
||||||
|
|
|
||||||
|
|
@ -32,6 +32,22 @@ export function percentToZoomLevel(percent) {
|
||||||
return clampZoomLevel(Math.log(percent / 100) / Math.log(ZOOM_FACTOR_BASE))
|
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
|
// Chromium on Windows can drop webContents zoom when a BrowserWindow is minimized
|
||||||
// and restored. Re-apply the persisted level on these lifecycle transitions.
|
// and restored. Re-apply the persisted level on these lifecycle transitions.
|
||||||
export const ZOOM_REASSERT_WINDOW_EVENTS = ['show', 'restore']
|
export const ZOOM_REASSERT_WINDOW_EVENTS = ['show', 'restore']
|
||||||
|
|
|
||||||
Loading…
Add table
Add a link
Reference in a new issue