diff --git a/src/workspaces/state/src/useAuthoredLayout.test.ts b/src/workspaces/state/src/useAuthoredLayout.test.ts index c71b3d75..fd9f2943 100644 --- a/src/workspaces/state/src/useAuthoredLayout.test.ts +++ b/src/workspaces/state/src/useAuthoredLayout.test.ts @@ -1,5 +1,5 @@ import { describe, expect, test } from 'bun:test'; -import type { LayoutNode } from 'officerdev'; +import type { LayoutNode, LayoutPanel } from 'officerdev'; import { __testing } from './useAuthoredLayout'; const { collectSizes, applySizes } = __testing; @@ -12,7 +12,7 @@ const { collectSizes, applySizes } = __testing; * way it can be wrong is silent, because a layout with the wrong numbers in it still renders. */ -const panel = (id: string, appType: string): LayoutNode => ({ type: 'panel', id, appType }); +const panel = (id: string, appType: string): LayoutPanel => ({ type: 'panel', id, appType }); const group = (id: string, children: { node: LayoutNode; size: number }[]): LayoutNode => ({ type: 'group', @@ -127,3 +127,93 @@ describe('a repair that cannot stick must not be written', () => { expect(JSON.stringify(afterRepair)).toBe(JSON.stringify(current)); }); }); + +/** + * Zoom, which was silently deleted for as long as only sizes were grafted. + * + * The failure was invisible rather than merely unsaved: the round trip produced a tree identical to the + * one already held, so `setValue`'s no-write guard dropped it and the header went on reading 100%. These + * tests are the round trip, at the level where it went wrong. + */ +describe('zoom survives the graft', () => { + const { collectZooms, applyZooms, graft } = __testing; + + /** The user has zoomed the chat panel, as `setZoom` would leave it. */ + const zoomed = group('root', [ + { + node: group('lists', [ + { node: panel('hosts', 'proxy-manager-hosts'), size: 62 }, + { node: panel('certs', 'proxy-manager-certificates'), size: 38 }, + ]), + size: 66, + }, + { node: { ...panel('chat', 'proxy-manager-chat'), zoom: 1.2 }, size: 34 }, + ]); + + test('collects only the panels that carry one', () => { + expect(collectZooms(zoomed)).toEqual({ chat: 1.2 }); + expect(collectZooms(source)).toEqual({}); + }); + + test('grafts onto the author structure, which never carries zoom itself', () => { + const merged = graft(source, zoomed); + expect(collectZooms(merged)).toEqual({ chat: 1.2 }); + }); + + test('a zoom change is a real difference, so the no-write guard lets it through', () => { + const current = graft(source, source); + const next = graft(source, zoomed); + expect(JSON.stringify(next)).not.toBe(JSON.stringify(current)); + }); + + test('sizes and zoom graft together, neither eating the other', () => { + const resized = group('root', [ + { + node: group('lists', [ + { node: panel('hosts', 'proxy-manager-hosts'), size: 20 }, + { node: panel('certs', 'proxy-manager-certificates'), size: 80 }, + ]), + size: 66, + }, + { node: { ...panel('chat', 'proxy-manager-chat'), zoom: 0.8 }, size: 34 }, + ]); + const merged = graft(source, resized); + expect(collectSizes(merged)).toEqual({ root: [66, 34], lists: [20, 80] }); + expect(collectZooms(merged)).toEqual({ chat: 0.8 }); + }); + + /** + * `setZoom` deletes the field on reset rather than writing 1, so an absent id has to mean "no opinion" + * — otherwise Reset zoom would be unreachable on a screen whose author shipped a zoom of their own. + */ + test('a reset drops back to whatever the author said', () => { + const authored = group('root', [ + { node: { ...panel('a', 'x'), zoom: 1.4 }, size: 50 }, + { node: panel('b', 'y'), size: 50 }, + ]); + // Nothing stored: the author's 1.4 stands. + expect(collectZooms(graft(authored, authored))).toEqual({ a: 1.4 }); + // The user overrides it. + const overridden = group('root', [ + { node: { ...panel('a', 'x'), zoom: 0.9 }, size: 50 }, + { node: panel('b', 'y'), size: 50 }, + ]); + expect(collectZooms(graft(authored, overridden))).toEqual({ a: 0.9 }); + // The user resets — `setZoom` removed the field — and the author's value returns. + const reset = group('root', [ + { node: panel('a', 'x'), size: 50 }, + { node: panel('b', 'y'), size: 50 }, + ]); + expect(collectZooms(graft(authored, reset))).toEqual({ a: 1.4 }); + }); + + test('a panel id that no longer exists is simply not applied', () => { + const stale = group('root', [{ node: { ...panel('gone', 'x'), zoom: 2 }, size: 100 }]); + expect(collectZooms(graft(source, stale))).toEqual({}); + }); + + test('applyZooms returns the node by reference when nothing changes', () => { + const node = panel('a', 'x'); + expect(applyZooms(node, {})).toBe(node); + }); +}); diff --git a/src/workspaces/state/src/useAuthoredLayout.ts b/src/workspaces/state/src/useAuthoredLayout.ts index 977b6985..4395c45a 100644 --- a/src/workspaces/state/src/useAuthoredLayout.ts +++ b/src/workspaces/state/src/useAuthoredLayout.ts @@ -3,7 +3,9 @@ import { useCallback, useMemo } from 'react'; import { useDashboardState } from './useDashboardState'; /** - * A layout whose STRUCTURE belongs to whoever wrote the screen, and whose SIZES belong to whoever uses it. + * A layout whose STRUCTURE belongs to whoever wrote the screen, and whose VIEW belongs to whoever uses it. + * + * The view is sizes and zoom: the two things a `locked` screen still lets a person change. * * ── The bug this ends ── * @@ -20,18 +22,30 @@ import { useDashboardState } from './useDashboardState'; * first panel and SAVED, so the corruption survived every reload, every fix, and every restart. The * row had to be deleted from Postgres, which is not a thing a user can do. * + * ── The bug this ended a second time, and why zoom is here ── + * + * Sizes were the only thing grafted back, so `zoom` — which lives on the panel node, because the node is + * the thing already persisted — was collected by nothing and rebuilt from `source` every render. The + * result was a control that rendered, took the click, and discarded it: `setValue` re-derived a tree + * identical to the one it already had, the no-write guard below dropped it as a no-op, and the header + * went on reading 100%. Silent on every plugin screen, `/bots` and `/plugins`, and reported as "the zoom + * buttons do nothing". + * + * The lesson is the general one: anything the user owns has to be named here, because the default for a + * field this hook does not know about is not "passed through" — it is "deleted, quietly". + * * ── The fix, and why it is a shape rather than a rule ── * - * The structure comes from `source` on every render and is never read back from storage. Only sizes are + * The structure comes from `source` on every render and is never read back from storage. Only the view is * persisted. So a stale structure cannot exist, a repair has nothing to write itself into, and a plugin * that changes its layout reaches everybody on the next load — with no version to bump, no migration, and * nothing for an author to remember. * - * The user keeps the one thing that was theirs. + * The user keeps the things that were theirs. * * ── What is stored ── * - * `source` with the user's sizes applied — a valid layout, because the storage validates one on both the + * `source` with the user's view applied — a valid layout, because the storage validates one on both the * route and a CHECK constraint, and because a value that is always freshly derived from source cannot rot. * Its structure is written and never read. That asymmetry is the whole design and is why it is safe. * @@ -43,6 +57,7 @@ import { useDashboardState } from './useDashboardState'; */ type Sizes = Record; +type Zooms = Record; const isGroup = (node: LayoutNode): node is LayoutGroup => node.type === 'group'; @@ -54,6 +69,22 @@ function collectSizes(node: LayoutNode, into: Sizes = {}): Sizes { return into; } +/** + * Every panel's zoom, by panel id — and only where one is actually set. + * + * `setZoom` DELETES the field on a reset to 100% rather than writing 1, so an absent id means "the user + * has no opinion" and the author's value stands. That is what makes Reset zoom reachable on a screen + * whose author shipped a zoom of their own. + */ +function collectZooms(node: LayoutNode, into: Zooms = {}): Zooms { + if (!isGroup(node)) { + if (node.zoom !== undefined) into[node.id] = node.zoom; + return into; + } + for (const child of node.children) collectZooms(child.node, into); + return into; +} + /** * Put saved sizes back onto the author's structure. * @@ -78,22 +109,39 @@ function applySizes(node: LayoutNode, sizes: Sizes): LayoutNode { }; } +/** Put saved zoom back onto the author's structure. A panel id nobody saved keeps whatever `source` says. */ +function applyZooms(node: LayoutNode, zooms: Zooms): LayoutNode { + if (!isGroup(node)) { + const saved = zooms[node.id]; + if (saved === undefined || saved === node.zoom) return node; + return { ...node, zoom: saved }; + } + return { + ...node, + children: node.children.map((child) => ({ ...child, node: applyZooms(child.node, zooms) })), + }; +} + +/** The author's structure wearing the user's view. The only way either half of this hook builds a tree. */ +const graft = (source: LayoutNode, stored: LayoutNode): LayoutNode => + applyZooms(applySizes(source, collectSizes(stored)), collectZooms(stored)); + /** * Same shape as `useDashboardState`, so a screen swaps one call and changes nothing else. * * `setValue` accepts a whole layout because that is what `WorkspaceView` reports — a resize hands back the - * entire tree. Everything but its sizes is discarded here, which is precisely the point: the view is free - * to report a normalised, repaired, or otherwise rewritten structure and none of it can be stored. + * entire tree. Everything but the user's view is discarded here, which is precisely the point: the view is + * free to report a normalised, repaired, or otherwise rewritten structure and none of it can be stored. */ export function useAuthoredLayout(key: string, source: LayoutNode) { const stored = useDashboardState(key, source); - const value = useMemo(() => applySizes(source, collectSizes(stored.value)), [source, stored.value]); + const value = useMemo(() => graft(source, stored.value), [source, stored.value]); const setValue = useCallback( (update: LayoutNode | ((prev: LayoutNode) => LayoutNode)) => { const next = typeof update === 'function' ? update(value) : update; - const merged = applySizes(source, collectSizes(next)); + const merged = graft(source, next); /* A write that changes nothing is dropped, and this is not an optimisation — it is what stops an @@ -109,6 +157,10 @@ export function useAuthoredLayout(key: string, source: LayoutNode) { panel keys, so every render of `/browser` produced a repair. Shipping the sizes-only hook turned a silent, harmless fallback into a hard crash on that screen. + It is also why a field this hook forgets to graft is INVISIBLE rather than merely unsaved: the + change round-trips to the same tree, compares equal, and is dropped here. That is exactly how zoom + was lost. + Comparing the serialised value is enough — these trees are small, this runs only on a real change, and it is the same check `setValue` would otherwise force React Query to make against a new object. */ @@ -123,4 +175,4 @@ export function useAuthoredLayout(key: string, source: LayoutNode) { } /** The tree walking, exposed for its test. Not part of the hook's API — import the hook. */ -export const __testing = { collectSizes, applySizes }; +export const __testing = { collectSizes, applySizes, collectZooms, applyZooms, graft };