diff --git a/.changeset/6797-refs-during-render-three-hooks.md b/.changeset/6797-refs-during-render-three-hooks.md new file mode 100644 index 000000000..5da684b22 --- /dev/null +++ b/.changeset/6797-refs-during-render-three-hooks.md @@ -0,0 +1,46 @@ +--- +'@object-ui/react': patch +--- + +`useETagCache`, `useGlobalUndo` and `useOffline` stop writing their config refs +during render (objectui#6797). + +Each of the three kept a "latest value" ref that was assigned in the render +body, one `react-hooks/refs` warning apiece on this base +(`useETagCache.ts:204`, `useGlobalUndo.ts:57`, `useOffline.ts:262` — all three +`Cannot update ref during render`, all three the WRITE only; unlike +`useSchemaPersistence` none of them also READ a ref during render). A ref +written in the render body is also written by renders React discards or +replays — StrictMode's double render, a Suspense retry, a concurrent +interruption — so a tree that never committed could publish its config to +callbacks that outlive it. + +The write moved to `useInsertionEffect` in all three, but that shape was chosen +per hook rather than carried over, because what each ref protects differs: + +- **`useETagCache`** — five resolved config scalars read by five `useCallback`s + with `[]` deps whose identity is part of the published result. Re-keying them + on the config values would have changed `fetchWithETag`'s identity whenever a + caller's `ttl` moved, re-firing consumer effects keyed on it, so the ref + stays. +- **`useGlobalUndo`** — the whole options bag. Every caller passes a fresh + inline literal with inline `onUndo` / `onRedo` closures, and the keydown + effect is keyed on `undo` / `redo`, so the ref is the only thing keeping + those two stable while still reaching the newest callbacks. +- **`useOffline`** — `config.sync`, read by one caller (`sync`) that is + *already* unstable (deps `[enabled, queue]`). Here the ref protects RETAINED + closures rather than an identity: the auto-sync effect deliberately captures + a `sync` and fires it 100ms later, and that closure must still see the newest + `batchSize`. Dropping the ref for a `syncConfig?.batchSize` dep would have + changed what that retained closure reads, so it was rejected. + +`useInsertionEffect` runs in the mutation phase — ahead of every layout effect, +ref attachment and paint — so the only window any of the three defers is the +render phase itself, where none of the affected callbacks is legally callable. +`useEffectEvent` would be the idiomatic answer but is React 19.2+, and this +package's peer range starts at React 18. + +**No behavioural change is claimed for callers that exist today**: reverting any +of the three implementations leaves the whole suite green, and the new pins pass +against the old code and the new code alike. They guard the next edit — each +file's discriminating pin fails under both `useEffect` and `useLayoutEffect`. diff --git a/packages/react/src/hooks/__tests__/useETagCache.configTiming.test.tsx b/packages/react/src/hooks/__tests__/useETagCache.configTiming.test.tsx new file mode 100644 index 000000000..fd5bdc56e --- /dev/null +++ b/packages/react/src/hooks/__tests__/useETagCache.configTiming.test.tsx @@ -0,0 +1,179 @@ +/** + * ObjectUI — useETagCache config-ref timing pins (objectui#6797) + * Copyright (c) 2024-present ObjectStack Inc. + * + * `useETagCache` kept its five resolved config values in a ref written in the + * RENDER BODY, which is what `react-hooks/refs` reported: + * + * packages/react/src/hooks/useETagCache.ts:204:3 + * react-hooks/refs Cannot update ref during render + * + * Who reads that ref, measured on this base: `isExpired` reads `.ttl`; + * `setEntry` reads `.maxEntries` / `.storage` / `.storagePrefix`; `removeEntry` + * and `clearCache` read `.storage` / `.storagePrefix`; `fetchWithETag` reads + * `.enabled`. Every one of them is a `useCallback` with `[]` deps, and their + * identity is part of the hook's published result — so the config has to reach + * them WITHOUT rebuilding them. Re-keying those callbacks on the config values + * instead would have changed `fetchWithETag`'s identity whenever a caller's + * `ttl` moved, re-firing any consumer effect keyed on it; that is an observable + * change, so the ref stays and only the WRITE moved. + * + * The write now happens in `useInsertionEffect`. Pin 2 is the discriminating + * one: `clearCache` is fully synchronous, so a child layout effect of the same + * commit reaching it fails under BOTH `useEffect` and `useLayoutEffect`. + */ + +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; +import { useLayoutEffect } from 'react'; +import { render, renderHook, act } from '@testing-library/react'; +import { useETagCache, type ETagCacheResult, type ETagCacheConfig } from '../useETagCache'; + +/** Seed the on-disk shape `clearLocalStorage(prefix)` walks: an index + entries. */ +function seedPrefix(prefix: string, url: string) { + localStorage.setItem(`${prefix}:index`, JSON.stringify([url])); + localStorage.setItem( + `${prefix}:${url}`, + JSON.stringify({ data: { v: prefix }, etag: 'W/"1"', url, timestamp: Date.now() }), + ); +} + +/** Parent holds the hook; the child reaches `clearCache` from its layout effect. */ +function Harness({ config, trigger }: { config: ETagCacheConfig; trigger: number }) { + const cache = useETagCache(config); + return ; +} + +function CommitPhaseCaller({ clearCache, trigger }: { clearCache: () => void; trigger: number }) { + useLayoutEffect(() => { + if (trigger > 0) clearCache(); + }, [trigger, clearCache]); + return null; +} + +/** The LRU map is module-scoped and shared across instances — reset it. */ +function resetSharedCache() { + const { result, unmount } = renderHook(() => useETagCache({ storage: 'memory' })); + act(() => { + result.current.clearCache(); + }); + unmount(); +} + +beforeEach(() => { + resetSharedCache(); + localStorage.clear(); +}); + +afterEach(() => { + vi.restoreAllMocks(); + vi.unstubAllGlobals(); +}); + +describe('useETagCache — config ref is refreshed in the commit, not in render (#6797)', () => { + // ---- pin 1: the newest config reaches the stable callbacks --------------- + it('clears the storage prefix of the LATEST committed render', () => { + seedPrefix('pfx-a', '/api/a'); + seedPrefix('pfx-b', '/api/b'); + + const { result, rerender } = renderHook( + ({ prefix }: { prefix: string }) => + useETagCache({ storage: 'localStorage', storagePrefix: prefix }), + { initialProps: { prefix: 'pfx-a' } }, + ); + + rerender({ prefix: 'pfx-b' }); + act(() => { + result.current.clearCache(); + }); + + expect(localStorage.getItem('pfx-b:index')).toBeNull(); + expect(localStorage.getItem('pfx-b:/api/b')).toBeNull(); + expect(localStorage.getItem('pfx-a:index')).not.toBeNull(); + }); + + // ---- pin 2: DISCRIMINATING — a child layout effect of the SAME commit ---- + it('has the swap in place before a child layout effect of the same commit clears', () => { + seedPrefix('hot-a', '/api/a'); + seedPrefix('hot-b', '/api/b'); + + const { rerender } = render( + , + ); + + act(() => { + rerender( + , + ); + }); + + expect(localStorage.getItem('hot-b:index')).toBeNull(); + expect(localStorage.getItem('hot-a:index')).not.toBeNull(); + }); + + // ---- pin 3: the identity the ref exists to protect ----------------------- + it('keeps the returned callbacks identical across config changes', () => { + const { result, rerender } = renderHook( + ({ ttl }: { ttl: number }) => useETagCache({ ttl, storagePrefix: `p-${ttl}` }), + { initialProps: { ttl: 1_000 } }, + ); + + const first: ETagCacheResult = result.current; + + rerender({ ttl: 2_000 }); + rerender({ ttl: 3_000 }); + + expect(result.current.fetchWithETag).toBe(first.fetchWithETag); + expect(result.current.clearCache).toBe(first.clearCache); + expect(result.current.invalidate).toBe(first.invalidate); + expect(result.current.invalidatePattern).toBe(first.invalidatePattern); + }); + + // ---- pin 4: the async reader (`isExpired` via fetchWithETag) sees it too -- + it('judges expiry against the ttl of the LATEST committed render', async () => { + let clock = 1_000_000; + vi.spyOn(Date, 'now').mockImplementation(() => clock); + + const fetchMock = vi.fn(async () => + new Response(JSON.stringify({ ok: true }), { + status: 200, + headers: { etag: 'W/"v1"', 'content-type': 'application/json' }, + }), + ); + vi.stubGlobal('fetch', fetchMock); + + const { result, rerender } = renderHook( + ({ ttl }: { ttl: number }) => useETagCache({ ttl, storage: 'memory' }), + { initialProps: { ttl: 60_000 } }, + ); + + // First call caches the entry (the response carries an etag). + await act(async () => { + await result.current.fetchWithETag('/api/ttl-probe'); + }); + + clock += 5_000; + // 5s have passed. Under the OLD ttl the entry is fresh; under the new one + // it is stale — and a stale entry must NOT send a revalidation header. + rerender({ ttl: 1_000 }); + + await act(async () => { + await result.current.fetchWithETag('/api/ttl-probe'); + }); + + expect(fetchMock).toHaveBeenCalledTimes(2); + const secondHeaders = new Headers( + (fetchMock.mock.calls[1] as unknown as [string, RequestInit])[1].headers, + ); + expect(secondHeaders.get('If-None-Match')).toBeNull(); + + // Control: with the ttl left alone the same entry DOES revalidate, so the + // assertion above is reading the ttl and not just a missing cache entry. + await act(async () => { + await result.current.fetchWithETag('/api/ttl-probe'); + }); + const thirdHeaders = new Headers( + (fetchMock.mock.calls[2] as unknown as [string, RequestInit])[1].headers, + ); + expect(thirdHeaders.get('If-None-Match')).toBe('W/"v1"'); + }); +}); diff --git a/packages/react/src/hooks/__tests__/useGlobalUndo.optionsTiming.test.tsx b/packages/react/src/hooks/__tests__/useGlobalUndo.optionsTiming.test.tsx new file mode 100644 index 000000000..732656180 --- /dev/null +++ b/packages/react/src/hooks/__tests__/useGlobalUndo.optionsTiming.test.tsx @@ -0,0 +1,175 @@ +/** + * ObjectUI — useGlobalUndo options-ref timing pins (objectui#6797) + * Copyright (c) 2024-present ObjectStack Inc. + * + * `useGlobalUndo` kept its whole `options` bag in a ref that was written in the + * RENDER BODY, which is what `react-hooks/refs` reported: + * + * packages/react/src/hooks/useGlobalUndo.ts:57:3 + * react-hooks/refs Cannot update ref during render + * + * Who reads that ref, measured on this base: `executeOp` reads + * `.dataSource` and `undo` / `redo` read `.onUndo` / `.onRedo` — all three are + * `useCallback`s whose identity must stay put, because the keydown effect is + * keyed on `undo` / `redo` and every in-repo caller passes a FRESH inline + * object literal with inline closures on every render (`AppContent.tsx`, + * `RecordDetailView.tsx`, `useConsoleActionRuntime.tsx`). So the ref is + * load-bearing and the fix could only move the WRITE, never remove the ref. + * + * The write now happens in `useInsertionEffect` — the mutation phase, ahead of + * every layout effect, ref attachment and paint. The pins below fix the + * behaviour that had to survive that move; pin 2 is the discriminating one and + * fails under BOTH `useEffect` and `useLayoutEffect`, because `executeOp` reads + * `optionsRef.current.dataSource` SYNCHRONOUSLY, before `undo`'s first `await`. + */ + +import { describe, it, expect, beforeEach, vi } from 'vitest'; +import { useLayoutEffect } from 'react'; +import { render, renderHook, act } from '@testing-library/react'; +import { globalUndoManager, type UndoableOperation } from '@object-ui/core'; +import { useGlobalUndo, type UseGlobalUndoOptions } from '../useGlobalUndo'; + +/** A `create` operation — undoing one dispatches `dataSource.delete`. */ +function createOp(id: string): UndoableOperation { + return { + id, + type: 'create', + objectName: 'account', + recordId: `rec_${id}`, + timestamp: Date.now(), + description: `created ${id}`, + undoData: { name: 'undo' }, + redoData: { name: 'redo' }, + }; +} + +function makeDataSource() { + return { + create: vi.fn(async () => ({})), + update: vi.fn(async () => ({})), + delete: vi.fn(async () => ({})), + }; +} + +/** Parent holds the hook; the child reaches `undo` from its own layout effect. */ +function Harness({ + options, + trigger, +}: { + options: UseGlobalUndoOptions; + trigger: number; +}) { + const ctl = useGlobalUndo(options); + return ; +} + +function CommitPhaseCaller({ undo, trigger }: { undo: () => Promise; trigger: number }) { + useLayoutEffect(() => { + if (trigger > 0) void undo(); + }, [trigger, undo]); + return null; +} + +beforeEach(() => { + globalUndoManager.clear(); +}); + +describe('useGlobalUndo — options ref is refreshed in the commit, not in render (#6797)', () => { + // ---- pin 1: the newest callbacks reach the next undo() ------------------- + it('routes undo through the options of the LATEST committed render', async () => { + const dsA = makeDataSource(); + const dsB = makeDataSource(); + const onUndoA = vi.fn(); + const onUndoB = vi.fn(); + + const { result, rerender } = renderHook( + ({ ds, onUndo }: { ds: ReturnType; onUndo: () => void }) => + // a fresh literal every render, exactly like every in-repo caller + useGlobalUndo({ dataSource: ds, onUndo }), + { initialProps: { ds: dsA, onUndo: onUndoA } }, + ); + + act(() => { + globalUndoManager.push(createOp('op1')); + }); + rerender({ ds: dsB, onUndo: onUndoB }); + + await act(async () => { + await result.current.undo(); + }); + + expect(dsB.delete).toHaveBeenCalledWith('account', 'rec_op1'); + expect(dsA.delete).not.toHaveBeenCalled(); + expect(onUndoB).toHaveBeenCalledTimes(1); + expect(onUndoA).not.toHaveBeenCalled(); + }); + + // ---- pin 2: DISCRIMINATING — a child layout effect of the SAME commit ---- + // `useEffect` lands after paint and `useLayoutEffect` runs bottom-up (so a + // CHILD's layout effect precedes the parent's). Only a mutation-phase write + // is already in place here. + it('has the swap in place before a child layout effect of the same commit calls undo', async () => { + const dsA = makeDataSource(); + const dsB = makeDataSource(); + + const { rerender } = render(); + + act(() => { + globalUndoManager.push(createOp('op2')); + }); + + await act(async () => { + rerender(); + }); + + expect(dsB.delete).toHaveBeenCalledWith('account', 'rec_op2'); + expect(dsA.delete).not.toHaveBeenCalled(); + }); + + // ---- pin 3: the identity the ref exists to protect ----------------------- + it('keeps undo/redo identity stable across renders that pass a new options literal', () => { + const { result, rerender } = renderHook( + ({ n }: { n: number }) => useGlobalUndo({ onUndo: () => void n, onRedo: () => void n }), + { initialProps: { n: 0 } }, + ); + + const firstUndo = result.current.undo; + const firstRedo = result.current.redo; + + rerender({ n: 1 }); + rerender({ n: 2 }); + + expect(result.current.undo).toBe(firstUndo); + expect(result.current.redo).toBe(firstRedo); + }); + + // ---- pin 4: redo reads the newest options too --------------------------- + it('routes redo through the options of the LATEST committed render', async () => { + const dsA = makeDataSource(); + const dsB = makeDataSource(); + const onRedoA = vi.fn(); + const onRedoB = vi.fn(); + + const { result, rerender } = renderHook( + ({ ds, onRedo }: { ds: ReturnType; onRedo: () => void }) => + useGlobalUndo({ dataSource: ds, onRedo }), + { initialProps: { ds: dsA, onRedo: onRedoA } }, + ); + + act(() => { + globalUndoManager.push(createOp('op3')); + }); + await act(async () => { + await result.current.undo(); + }); + + rerender({ ds: dsB, onRedo: onRedoB }); + await act(async () => { + await result.current.redo(); + }); + + expect(dsB.create).toHaveBeenCalledWith('account', { name: 'redo' }); + expect(onRedoB).toHaveBeenCalledTimes(1); + expect(onRedoA).not.toHaveBeenCalled(); + }); +}); diff --git a/packages/react/src/hooks/__tests__/useOffline.syncConfigTiming.test.tsx b/packages/react/src/hooks/__tests__/useOffline.syncConfigTiming.test.tsx new file mode 100644 index 000000000..97764f802 --- /dev/null +++ b/packages/react/src/hooks/__tests__/useOffline.syncConfigTiming.test.tsx @@ -0,0 +1,163 @@ +/** + * ObjectUI — useOffline sync-config ref timing pins (objectui#6797) + * Copyright (c) 2024-present ObjectStack Inc. + * + * `useOffline` kept `config.sync` in a ref written in the RENDER BODY, which is + * what `react-hooks/refs` reported: + * + * packages/react/src/hooks/useOffline.ts:262:3 + * react-hooks/refs Cannot update ref during render + * + * Who reads that ref, measured on this base: exactly ONE reader — + * `sync`, at `const batchSize = syncConfigRef.current?.batchSize ?? queue.length`. + * That makes this hook the odd one of the three: `sync` is NOT a stable + * callback (deps `[enabled, queue]`), so the ref is not protecting an identity + * the way the other two hooks' refs are. What it protects is RETAINED closures: + * a config-only change keeps the same `sync` alive, and the auto-sync effect + * deliberately captures one and fires it 100ms later. Pin 1 is that exact + * property — the ref's only job — and it is what rules out the alternative fix + * of dropping the ref and adding `syncConfig?.batchSize` to `sync`'s deps. + * + * The write now happens in `useInsertionEffect`. Pin 3 is the discriminating + * one: the `batchSize` read is SYNCHRONOUS, before `sync`'s first `await`, so a + * child layout effect of the same commit fails under BOTH `useEffect` and + * `useLayoutEffect`. + */ + +import { describe, it, expect, beforeEach } from 'vitest'; +import { useLayoutEffect, useRef } from 'react'; +import { render, renderHook, act, screen } from '@testing-library/react'; +import { useOffline, type OfflineConfig, type OfflineResult } from '../useOffline'; + +function queueN(result: { current: OfflineResult }, n: number) { + act(() => { + for (let i = 0; i < n; i += 1) { + result.current.queueMutation({ operation: 'create', resource: 'account', data: { i } }); + } + }); +} + +/** Parent holds the hook; the child reaches `sync` from its own layout effect. */ +function Harness({ config, trigger }: { config: OfflineConfig; trigger: number }) { + const offline = useOffline(config); + return ( + <> + {offline.pendingCount} + + + + ); +} + +function CommitPhaseCaller({ sync, trigger }: { sync: () => Promise; trigger: number }) { + // Fire EXACTLY once. `sync` drains the queue, which re-renders and hands this + // effect a new `sync` (it is keyed on `[enabled, queue]`), so an unguarded + // effect re-fires and drains the queue batch-by-batch until it is empty — the + // end state is then 0 whatever `batchSize` the first call read, and the pin + // measures nothing about timing. Measured: with no guard this test passed + // even with the ref write moved to `useEffect`. + const fired = useRef(false); + useLayoutEffect(() => { + if (trigger > 0 && !fired.current) { + fired.current = true; + void sync(); + } + }, [trigger, sync]); + return null; +} + +function Seeder({ + queueMutation, +}: { + queueMutation: OfflineResult['queueMutation']; +}) { + return ( +