From 4f8046d7e939ac7237aba114d9050cfbba0fbab9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Padez?= Date: Fri, 7 Aug 2026 14:22:26 +0000 Subject: [PATCH] test useDashboardState, and fix the revert it proved was inverted MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit the store every dashboard layout is written through had no tests. writing them found a live defect on its rollback: the guard asked "does the cache still hold what i wrote?" by reference, and setQueryData runs react query's structural sharing, which rebuilds an object rather than storing the one it was handed. verified against 5.101.4 — an object comes back !==, a string comes back ===. so the check was false for every container the store exists to hold: every layout, every config.agentName. a refused write kept its optimistic value while the toast said it had been rolled back, and the change vanished at the next reload. only primitives ever reverted, which is why it went unnoticed. replaced with a per-key write sequence, which asks the question the identity check meant to ask — has anything written this key since — and does not depend on identity at all. 14 tests: readValue's kind guard, the optimistic write and its updater composition, and five on revert including the object regression pin. also moves testing-library's cleanup into test-setup. it auto-registers afterEach at module import time, so bun attaches it to whichever file imports the library first and every later file silently gets none. adding this test file was enough to break fourteen assertions in DataTable.test.tsx, which does not import it. preload has no file scope, so registering there removes the ordering from the question. Co-Authored-By: Claude Opus 5 --- .../state/src/useDashboardState.test.tsx | 270 ++++++++++++++++++ src/workspaces/state/src/useDashboardState.ts | 44 ++- test-setup.ts | 19 ++ 3 files changed, 324 insertions(+), 9 deletions(-) create mode 100644 src/workspaces/state/src/useDashboardState.test.tsx diff --git a/src/workspaces/state/src/useDashboardState.test.tsx b/src/workspaces/state/src/useDashboardState.test.tsx new file mode 100644 index 00000000..91c3871b --- /dev/null +++ b/src/workspaces/state/src/useDashboardState.test.tsx @@ -0,0 +1,270 @@ +import type { ReactNode } from 'react'; +import { describe, expect, mock, test, beforeEach } from 'bun:test'; +import { QueryClient, QueryClientProvider } from '@tanstack/react-query'; +import { renderHook, act, waitFor } from '@testing-library/react'; + +/** + * The store every dashboard layout — and therefore every chat panel's `config.agentName` — is written + * through. It had no tests, and the two pieces of reasoning it carries are the ones that decide whether an + * unattended run is survivable: + * + * - `readValue` refuses a stored value whose *kind* disagrees with the default. One wrong-shaped jsonb + * row used to be `.children.map is not a function` during render, i.e. a white screen recoverable only + * by SQL. + * - `revert` rolls an optimistic write back **only if nothing has written that key since**. Writes to one + * key overlap freely (a window resize fires one per group), so an unconditional rollback would turn one + * failed write into two lost ones. + * + * Both are module-private, so everything here goes through the real hook. The only fakes are the transport + * (`useClient`), the identity (`useAuth`) and the toast — the logic under test is untouched. + * + * Writing these found a live defect on the second point: the guard was reference equality against the + * value written, and `setQueryData`'s structural sharing rebuilds objects, so it was false for every + * container — i.e. for every layout. Fixed by a per-key sequence number; the pin is the "refused OBJECT + * write" test below. + */ + +type Layout = { type: string; id: string; config?: { agentName: string } }; + +const patchCalls: Record[] = []; +let getResponse: () => Promise = async () => ({}); +let patchBehaviour: (body: Record) => Promise = async () => ({}); + +const toastErrors: { message: string; description?: string }[] = []; + +/** + * `mock.module` replaces a module *process-wide and permanently* — Bun has one module registry for the + * whole run, so a stub declared here is what every other test file gets too. Replacing the module wholesale + * is therefore a way to delete exports out from under files that never heard of this one: a bare + * `{ useClient }` stub took `getHeaders` away from `AppRegistry.test.ts`, which failed with a `SyntaxError` + * and no obvious connection to the change. + * + * So each stub spreads the real module and overrides exactly the one export it needs to fake. + */ +const realUseClient = await import('hooks/useClient'); +const realUseAuth = await import('hooks/useAuth'); +const realSonner = await import('sonner'); + +mock.module('hooks/useClient', () => ({ + ...realUseClient, + useClient: () => ({ + get: () => getResponse(), + patch: (_path: string, body: Record) => { + patchCalls.push(body); + return patchBehaviour(body); + }, + }), +})); + +mock.module('hooks/useAuth', () => ({ ...realUseAuth, useAuth: () => ({ isAuthenticated: true }) })); + +mock.module('sonner', () => ({ + ...realSonner, + toast: { + ...realSonner.toast, + error: (message: string, opts?: { description?: string }) => + toastErrors.push({ message, description: opts?.description }), + }, +})); + +const { useDashboardState, DASHBOARD_STATE_QUERY_KEY } = await import('./useDashboardState'); + +let queryClient: QueryClient; + +const wrapper = ({ children }: { children: ReactNode }) => ( + {children} +); + +const mount = (key: string, defaultValue: T) => + renderHook(() => useDashboardState(key, defaultValue), { wrapper }); + +const cache = () => queryClient.getQueryData>(DASHBOARD_STATE_QUERY_KEY) ?? {}; + +beforeEach(() => { + patchCalls.length = 0; + toastErrors.length = 0; + getResponse = async () => ({}); + patchBehaviour = async () => ({}); + queryClient = new QueryClient({ defaultOptions: { queries: { retry: false }, mutations: { retry: false } } }); +}); + +describe('readValue — a wrong-shaped row must not reach a consumer that traverses it', () => { + test('a stored array where an object is expected falls back to the default', async () => { + // The real case: a layout column that defaulted to `[]`. A LayoutNode consumer does `.children.map`. + getResponse = async () => ({ 'ws-layout-x': [] }); + const { result } = mount('ws-layout-x', { type: 'panel', id: 'root' }); + await waitFor(() => expect(result.current.isLoaded).toBe(true)); + expect(result.current.value).toEqual({ type: 'panel', id: 'root' }); + }); + + test('a stored object where an array is expected falls back to the default', async () => { + getResponse = async () => ({ 'panel-sessions': { a: 1 } }); + const { result } = mount('panel-sessions', []); + await waitFor(() => expect(result.current.isLoaded).toBe(true)); + expect(result.current.value).toEqual([]); + }); + + test('a container of the right kind is returned as stored, by reference', async () => { + const stored = { type: 'panel', id: 'stored', config: { agentName: 'scout' } }; + getResponse = async () => ({ 'ws-layout-x': stored }); + const { result } = mount('ws-layout-x', { type: 'panel', id: 'default' }); + await waitFor(() => expect(result.current.isLoaded).toBe(true)); + expect(result.current.value).toEqual(stored); + }); + + test('a wrong primitive is passed through — only containers are guarded', async () => { + // Deliberate: a wrong primitive is a cosmetic surprise, a wrong container is a crash. + getResponse = async () => ({ 'files/currentPath': 42 }); + const { result } = mount('files/currentPath', '/'); + await waitFor(() => expect(result.current.isLoaded).toBe(true)); + expect(result.current.value).toBe(42 as unknown as string); + }); + + test('a bad read is NOT written back — the server keeps the row until a real write repairs it', async () => { + getResponse = async () => ({ 'ws-layout-x': [] }); + const { result } = mount('ws-layout-x', { type: 'panel', id: 'root' }); + await waitFor(() => expect(result.current.isLoaded).toBe(true)); + expect(patchCalls).toEqual([]); + }); +}); + +describe('setValue — optimistic write, then persist', () => { + test('updates the cache immediately and PATCHes the same value', async () => { + getResponse = async () => ({ 'ws-layout-x': { type: 'panel', id: 'a' } }); + const { result } = mount('ws-layout-x', { type: 'panel', id: 'root' }); + await waitFor(() => expect(result.current.isLoaded).toBe(true)); + + act(() => result.current.setValue({ type: 'panel', id: 'b' })); + + expect(cache()['ws-layout-x']).toEqual({ type: 'panel', id: 'b' }); + expect(patchCalls).toEqual([{ 'ws-layout-x': { type: 'panel', id: 'b' } }]); + }); + + test('an updater composes on the cache, not on a captured render value', async () => { + // This is what stops two mutations inside one window from losing the first: every caller in + // WorkspaceView passes an updater, and the updater must see the previous write. + getResponse = async () => ({ counter: 0 }); + const { result } = mount('counter', 0); + await waitFor(() => expect(result.current.isLoaded).toBe(true)); + + act(() => { + result.current.setValue((n) => n + 1); + result.current.setValue((n) => n + 1); + result.current.setValue((n) => n + 1); + }); + + expect(cache().counter).toBe(3); + expect(patchCalls).toEqual([{ counter: 1 }, { counter: 2 }, { counter: 3 }]); + }); +}); + +describe('revert — a failed write must not roll back over a later successful one', () => { + test('a failed write is rolled back to the value it replaced, and says so', async () => { + getResponse = async () => ({ 'ws-layout-x': { type: 'panel', id: 'before' } }); + patchBehaviour = async () => { + throw new Error('boom'); + }; + const { result } = mount('ws-layout-x', { type: 'panel', id: 'root' }); + await waitFor(() => expect(result.current.isLoaded).toBe(true)); + + act(() => result.current.setValue({ type: 'panel', id: 'after' })); + await waitFor(() => expect(toastErrors.length).toBe(1)); + + expect(cache()['ws-layout-x']).toEqual({ type: 'panel', id: 'before' }); + expect(toastErrors[0]!.description).toContain('boom'); + }); + + test('a key that did not exist is deleted rather than reverted to undefined', async () => { + getResponse = async () => ({}); + patchBehaviour = async () => { + throw new Error('nope'); + }; + const { result } = mount('brand-new', 'seed'); + await waitFor(() => expect(result.current.isLoaded).toBe(true)); + // The seeding effect writes the default; let it fail and clear before the real assertion. + await waitFor(() => expect(toastErrors.length).toBeGreaterThan(0)); + toastErrors.length = 0; + queryClient.setQueryData(DASHBOARD_STATE_QUERY_KEY, {}); + + act(() => result.current.setValue('written')); + await waitFor(() => expect(toastErrors.length).toBe(1)); + + expect('brand-new' in cache()).toBe(false); + }); + + test('a LATER successful write survives an EARLIER failure — the whole point of the compare-and-swap', async () => { + getResponse = async () => ({ 'ws-layout-x': 'original' }); + let failNext = true; + patchBehaviour = async () => { + if (failNext) { + failNext = false; + throw new Error('first write refused'); + } + return {}; + }; + const { result } = mount('ws-layout-x', 'default'); + await waitFor(() => expect(result.current.isLoaded).toBe(true)); + + // Two writes in flight at once — a window resize fires one per group. The first is refused. + act(() => { + result.current.setValue('doomed'); + result.current.setValue('winner'); + }); + await waitFor(() => expect(toastErrors.length).toBe(1)); + + // Unconditional rollback would leave 'doomed' here, turning one lost write into two. + expect(cache()['ws-layout-x']).toBe('winner'); + }); + + test('a refused OBJECT write is rolled back too — the case reference equality could never catch', async () => { + // Regression pin. The guard used to be `cache[key] === attempted`, and `setQueryData` runs React + // Query's structural sharing, which rebuilds an object rather than storing the one it was handed. So + // the check was false for every container — every layout, every `config.agentName` — and a refused + // write kept its optimistic value while the toast claimed a rollback. Primitives passed, which is + // why it went unnoticed. If this ever fails again, check `writeSeq`, not the test. + getResponse = async () => ({ 'ws-layout-obj': { type: 'panel', id: 'before', config: { agentName: 'scout' } } }); + patchBehaviour = async () => { + throw new Error('refused'); + }; + const { result } = mount('ws-layout-obj', { type: 'panel', id: 'fallback' }); + await waitFor(() => expect(result.current.isLoaded).toBe(true)); + + act(() => result.current.setValue({ type: 'panel', id: 'after', config: { agentName: 'builder' } })); + await waitFor(() => expect(toastErrors.length).toBe(1)); + + expect(cache()['ws-layout-obj']).toEqual({ type: 'panel', id: 'before', config: { agentName: 'scout' } }); + }); + + test('the failure is surfaced, not swallowed — this store used to lose writes invisibly', async () => { + getResponse = async () => ({ 'ws-layout-x': 'original' }); + patchBehaviour = async () => { + throw new Error('server said no'); + }; + const { result } = mount('ws-layout-x', 'default'); + await waitFor(() => expect(result.current.isLoaded).toBe(true)); + + act(() => result.current.setValue('attempt')); + await waitFor(() => expect(toastErrors.length).toBe(1)); + + expect(toastErrors[0]!.message).toBe('Could not save your layout'); + expect(toastErrors[0]!.description).toContain('ws-layout-x'); + }); +}); + +describe('seeding — a missing key is written once, not on every mount', () => { + test('the default is seeded to the server when the key is absent', async () => { + getResponse = async () => ({}); + const { result } = mount('files/currentPath', '/'); + await waitFor(() => expect(patchCalls.length).toBe(1)); + expect(patchCalls[0]).toEqual({ 'files/currentPath': '/' }); + expect(result.current.value).toBe('/'); + }); + + test('an existing key is not re-seeded', async () => { + getResponse = async () => ({ 'files/currentPath': '/Pictures' }); + const { result } = mount('files/currentPath', '/'); + await waitFor(() => expect(result.current.isLoaded).toBe(true)); + expect(patchCalls).toEqual([]); + expect(result.current.value).toBe('/Pictures'); + }); +}); diff --git a/src/workspaces/state/src/useDashboardState.ts b/src/workspaces/state/src/useDashboardState.ts index 1e1b33d2..df8303fa 100644 --- a/src/workspaces/state/src/useDashboardState.ts +++ b/src/workspaces/state/src/useDashboardState.ts @@ -47,8 +47,9 @@ export function useDashboardState(key: string, defaultValue: T) { const currentState = queryClient.getQueryData(QUERY_KEY) ?? {}; if (!(key in currentState)) { queryClient.setQueryData(QUERY_KEY, { ...currentState, [key]: defaultValue }); + const seq = nextWriteSeq(key); persist(clientRef.current, key, defaultValue).catch((err: unknown) => - revert(queryClient, key, defaultValue, undefined, err), + revert(queryClient, key, seq, undefined, err), ); } }, [isSuccess, key, defaultValue, queryClient]); @@ -63,8 +64,9 @@ export function useDashboardState(key: string, defaultValue: T) { queryClient.setQueryData(QUERY_KEY, { ...currentState, [key]: newValue }); + const seq = nextWriteSeq(key); persist(clientRef.current, key, newValue).catch((err: unknown) => - revert(queryClient, key, newValue, key in currentState ? currentValue : undefined, err), + revert(queryClient, key, seq, key in currentState ? currentValue : undefined, err), ); }, [key, defaultValue, queryClient], @@ -81,6 +83,30 @@ export function useDashboardState(key: string, defaultValue: T) { let inFlightWrites = 0; let lastWriteAt = 0; +/** + * Per-key write sequence, and the reason `revert` cannot just look at the cache. + * + * It used to ask "does the cache still hold exactly what I wrote?" by reference — and for a container that + * question can never be true. `setQueryData` runs React Query's structural sharing, which rebuilds the + * object it stores rather than keeping the one it was handed (verified against @tanstack/react-query + * 5.101.4: an object value comes back `!==` what went in, a string comes back `===`). So the guard silently + * inverted for exactly the values this store exists to hold: every layout, every `config.agentName`. A + * refused write kept its optimistic value in the cache while the toast said it had been rolled back, and + * the change vanished at the next reload — the invisible loss the rollback was added to end. + * + * A sequence number asks the question the identity check meant to ask — "has anything written this key + * since?" — and does not depend on identity at all. Caveat, unchanged from before: if two writes to one key + * both fail, the later one reverts to what the earlier one put there. Recovering further would mean keeping + * a known-good value per key, and two consecutive refusals is a server that is telling you plainly. + */ +const writeSeq = new Map(); + +function nextWriteSeq(key: string): number { + const seq = (writeSeq.get(key) ?? 0) + 1; + writeSeq.set(key, seq); + return seq; +} + function persist(client: ReturnType, key: string, value: unknown): Promise { return patchState(client, { [key]: value }); } @@ -102,7 +128,7 @@ function patchState(client: ReturnType, patch: Record {})` in this family: a delete that failed left the dashboard * gone from the list and alive on the server, reappearing at the next reload with no hint why. * - * Recovery here is an invalidate rather than the hook's compare-and-swap revert. There is no single + * Recovery here is an invalidate rather than the hook's sequence-guarded revert. There is no single * previous value to swap back — the caller has already rewritten the roster and dropped keys from the * blob — and refetching is the only thing that makes the list agree with the server again. */ @@ -163,19 +189,19 @@ const warned = new Set(); * are not merely cosmetic — a chat panel's `config.agentName` is the address a peer agent is delivered * to, so a silently-dropped write leaves a panel that answers to a name it will not have tomorrow. * - * Reverts only if the cache still holds exactly what we wrote. Writes to one key overlap freely (a - * window resize fires one per group), and rolling back over a later successful write would turn one - * failure into two. + * Reverts only if nothing has written this key since. Writes to one key overlap freely (a window resize + * fires one per group), and rolling back over a later successful write would turn one failure into two. + * `seq` rather than the written value is deliberate and load-bearing — see `writeSeq` above. */ function revert( queryClient: ReturnType, key: string, - attempted: unknown, + seq: number, previous: unknown, err: unknown, ): void { - const state = queryClient.getQueryData(QUERY_KEY) ?? {}; - if (state[key] === attempted) { + if (writeSeq.get(key) === seq) { + const state = queryClient.getQueryData(QUERY_KEY) ?? {}; const next = { ...state }; if (previous === undefined) delete next[key]; else next[key] = previous; diff --git a/test-setup.ts b/test-setup.ts index 7fb6068e..edf81a9f 100644 --- a/test-setup.ts +++ b/test-setup.ts @@ -1,3 +1,4 @@ +import { afterEach } from 'bun:test'; import { GlobalWindow } from 'happy-dom'; const window = new GlobalWindow(); @@ -32,3 +33,21 @@ Object.assign(globalThis, { setInterval: window.setInterval.bind(window), clearInterval: window.clearInterval.bind(window), }); + +/** + * Unmount every rendered tree between tests — globally, because the library's own version of this is + * order-dependent in a way that produces a bewildering failure. + * + * `@testing-library/react` auto-registers `afterEach(cleanup)` at *module import* time. Bun evaluates a + * module once and attaches lifecycle hooks to whichever file is loading at that moment, so the file that + * happens to import the library first gets the cleanup and every later file silently gets none — its + * renders pile up in `document.body` and the next `screen.getAllByRole` sees the previous test's DOM. + * Adding one test file was enough to break fourteen assertions in a file it does not import, does not + * touch, and was passing on its own. + * + * Registering it here removes the ordering from the question: preload has no enclosing file scope, so this + * hook is global. The dynamic import is deliberate — `@testing-library/dom` builds `screen` from + * `document.body` while it initialises, so it must not load until the happy-dom globals above exist. + */ +const { cleanup } = await import('@testing-library/react'); +afterEach(cleanup);