From a7be07cc51dee78cafdeba7dc2b83dbe1ccf743a Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 29 Aug 2026 14:43:58 +0000 Subject: [PATCH] fix(react): stop writing the useSchemaPersistence adapter ref during render MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The hook wrote `adapterRef.current` in the render body and read `defaultAdapter.current` there too, drawing three react-hooks/refs warnings (objectui#6745). A render React discards or replays still performed the write, so a save could be routed through an adapter from a render that never committed. The write moves to `useInsertionEffect` — the mutation phase, ahead of every layout effect in the tree, paint, and any event handler — so every call site that may legally invoke save/load/list/remove sees exactly what the old render-body write gave it. `useEffect` (after paint) and `useLayoutEffect` (a child's run before its parent's) would each have deferred the swap past a legal reader; a new pin holds that distinction. The default adapter moves from `useRef(createLocalStorageAdapter())` to `useMemo`, which also stops the factory running on every render. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01CRJge11jso9TpXRWFt1Z49 --- ...45-schema-persistence-ref-during-render.md | 43 +++++ ...SchemaPersistence.adapterTracking.test.tsx | 172 ++++++++++++++++++ .../react/src/hooks/useSchemaPersistence.ts | 46 ++++- 3 files changed, 256 insertions(+), 5 deletions(-) create mode 100644 .changeset/6745-schema-persistence-ref-during-render.md create mode 100644 packages/react/src/hooks/__tests__/useSchemaPersistence.adapterTracking.test.tsx diff --git a/.changeset/6745-schema-persistence-ref-during-render.md b/.changeset/6745-schema-persistence-ref-during-render.md new file mode 100644 index 000000000..b49c0ac21 --- /dev/null +++ b/.changeset/6745-schema-persistence-ref-during-render.md @@ -0,0 +1,43 @@ +--- +'@object-ui/react': patch +--- + +`useSchemaPersistence` no longer writes its adapter ref during render +(objectui#6745). + +The hook keeps the live adapter in a ref so `save`/`load`/`list`/`remove` can be +created once and still reach the newest adapter at call time. That ref was +written in the render body: + +``` +const defaultAdapter = useRef(createLocalStorageAdapter()); +const adapterRef = useRef(adapter ?? defaultAdapter.current); +adapterRef.current = adapter ?? defaultAdapter.current; // during render +``` + +which `react-hooks/refs` flags on three counts. A render React discards or +replays — StrictMode, a Suspense retry, a concurrent interruption — still +performed that write, so a save could be routed through an adapter belonging to +a render that never committed. + +The write now happens in `useInsertionEffect`, and the default adapter comes +from `useMemo` instead of a ref read during render. + +**Timing is preserved for every legal call site.** Insertion effects run in the +mutation phase — before every layout effect in the tree, before paint, and +before any event handler can fire — so a changed `adapter` prop is in place +before anything that may legally invoke these methods can observe it. This is +deliberately *not* `useEffect` (which lands after paint) or `useLayoutEffect` +(a child's layout effects run before its parent's); either would route a call +made earlier in the same commit to the previous adapter. The only window that +changed is a read during the render phase itself, which no legal consumer has: +`save`/`load`/`list`/`remove` are side effects and are never callable during +render. + +Also fixed in passing, on the same lines: `useRef(createLocalStorageAdapter())` +invoked the factory on **every** render and discarded all but the first result. +The `useMemo` runs it once. The adapter is a stateless facade over +`localStorage` and its identity is never exposed, so this is unobservable +beyond the saved work. + +No API, signature or observable behaviour change for any supported call site. diff --git a/packages/react/src/hooks/__tests__/useSchemaPersistence.adapterTracking.test.tsx b/packages/react/src/hooks/__tests__/useSchemaPersistence.adapterTracking.test.tsx new file mode 100644 index 000000000..d0c00dc60 --- /dev/null +++ b/packages/react/src/hooks/__tests__/useSchemaPersistence.adapterTracking.test.tsx @@ -0,0 +1,172 @@ +/** + * ObjectUI — useSchemaPersistence adapter-tracking pins (objectui#6745) + * Copyright (c) 2024-present ObjectStack Inc. + * + * The hook keeps the live adapter in a ref so that `save`/`load`/`list`/ + * `remove` can be created once (`[]` deps) and still reach the newest adapter + * at call time. That ref used to be written in the RENDER BODY: + * + * const adapterRef = useRef(adapter ?? defaultAdapter.current); + * adapterRef.current = adapter ?? defaultAdapter.current; // during render + * + * which `react-hooks/refs` flags: a render React discards or replays still + * performed the write, so a save could be routed through an adapter belonging + * to a render that never committed. + * + * Moving that write to `useEffect` would have been a BEHAVIOUR change, not a + * cleanup — the assignment would land after paint, so anything calling `save()` + * earlier in the same commit would reach the PREVIOUS adapter. It now lives in + * `useInsertionEffect`, which runs in the mutation phase: before every layout + * effect in the tree, before paint, and before any event handler can fire. + * + * These pins hold BOTH halves. Pin 2 is the one that discriminates: it fails + * against a `useEffect` write (fires after paint) and against a + * `useLayoutEffect` write (a child's layout effect runs before its parent's), + * and passes only while the write happens no later than the mutation phase. + */ + +import { describe, it, expect, beforeEach, vi } from 'vitest'; +import { useLayoutEffect } from 'react'; +import { render, renderHook, act } from '@testing-library/react'; +import { + useSchemaPersistence, + type SchemaPersistenceAdapter, +} from '../useSchemaPersistence'; + +const SCHEMA = { type: 'page:list' } as const; +const KEY = 'objectui-schema:design-1'; + +/** An adapter that records every id handed to `save`, tagged with its name. */ +function recordingAdapter(name: string, log: string[]): SchemaPersistenceAdapter { + return { + save: vi.fn(async (id: string) => { + log.push(`${name}:${id}`); + return id; + }), + load: vi.fn(async () => null), + list: vi.fn(async () => []), + delete: vi.fn(async () => {}), + }; +} + +beforeEach(() => { + localStorage.clear(); +}); + +describe('useSchemaPersistence — adapter tracking (#6745)', () => { + // ---- pin 1: a changed `adapter` prop is what the next save reaches -------- + it('routes a save through the adapter from the latest render, not the first', async () => { + const log: string[] = []; + const first = recordingAdapter('first', log); + const second = recordingAdapter('second', log); + + const hook = renderHook(({ adapter }) => useSchemaPersistence(adapter), { + initialProps: { adapter: first }, + }); + + await act(async () => { + await hook.result.current.save('before-swap', { ...SCHEMA }); + }); + + hook.rerender({ adapter: second }); + + await act(async () => { + await hook.result.current.save('after-swap', { ...SCHEMA }); + }); + + expect(log).toEqual(['first:before-swap', 'second:after-swap']); + expect(first.save).toHaveBeenCalledTimes(1); + expect(second.save).toHaveBeenCalledTimes(1); + }); + + // ---- pin 2: the swap is in place BEFORE that commit's layout effects ------ + // + // The child's layout effect runs inside the very commit that introduced the + // new adapter, and before the parent's own layout effects. Whatever it calls + // must already be routed to the new adapter — that is the timing the old + // render-body write gave, and the reason this write is an insertion effect. + it('has the new adapter in place before a child layout effect of the same commit', async () => { + const log: string[] = []; + const first = recordingAdapter('first', log); + const second = recordingAdapter('second', log); + + type Save = (id: string, schema: Record) => Promise; + + function Child({ save, token }: { save: Save; token: string }) { + // `save` is created once (`[]` deps) and never changes identity, so this + // fires exactly once per adapter — on mount, and again in the commit that + // swapped it. (Depending on a fresh closure instead re-fires on every + // commit, and `save`'s own `setLoading` then loops the test forever.) + useLayoutEffect(() => { + void save(`from-layout-${token}`, { ...SCHEMA }); + }, [token, save]); + return null; + } + + function Host({ adapter, token }: { adapter: SchemaPersistenceAdapter; token: string }) { + const persistence = useSchemaPersistence(adapter); + return ; + } + + const view = render(); + await act(async () => {}); + + expect(log).toEqual(['first:from-layout-first']); + + await act(async () => { + view.rerender(); + }); + + // Under a `useEffect` or `useLayoutEffect` write this second entry reads + // `first:from-layout-second` — the call is routed to the stale adapter. + expect(log).toEqual(['first:from-layout-first', 'second:from-layout-second']); + }); + + // ---- pin 3: the default localStorage adapter still works across renders --- + // + // The default adapter moved from `useRef(createLocalStorageAdapter())` (which + // re-ran the factory on every render and threw the result away) to a + // `useMemo`. It must still be the adapter a save lands in when no `adapter` + // prop is given, including after a re-render. + it('keeps persisting through the default adapter after a re-render', async () => { + const hook = renderHook(() => useSchemaPersistence()); + + hook.rerender(); + hook.rerender(); + + await act(async () => { + await hook.result.current.save('design-1', { ...SCHEMA }); + }); + + const raw = localStorage.getItem(KEY); + expect(raw).not.toBeNull(); + expect(JSON.parse(raw as string).schema).toEqual({ ...SCHEMA }); + }); + + // ---- pin 4: an explicit adapter takes over from the default -------------- + it('switches from the default adapter to an explicit one handed in later', async () => { + const log: string[] = []; + const explicit = recordingAdapter('explicit', log); + + const hook = renderHook( + ({ adapter }: { adapter?: SchemaPersistenceAdapter }) => useSchemaPersistence(adapter), + { initialProps: { adapter: undefined as SchemaPersistenceAdapter | undefined } }, + ); + + await act(async () => { + await hook.result.current.save('design-1', { ...SCHEMA }); + }); + expect(localStorage.getItem(KEY)).not.toBeNull(); + + hook.rerender({ adapter: explicit }); + + await act(async () => { + await hook.result.current.save('design-2', { ...SCHEMA }); + }); + + expect(log).toEqual(['explicit:design-2']); + // The explicit adapter does not write localStorage, so the default's entry + // is the only one there. + expect(localStorage.getItem('objectui-schema:design-2')).toBeNull(); + }); +}); diff --git a/packages/react/src/hooks/useSchemaPersistence.ts b/packages/react/src/hooks/useSchemaPersistence.ts index 71e9bc364..7ec02606b 100644 --- a/packages/react/src/hooks/useSchemaPersistence.ts +++ b/packages/react/src/hooks/useSchemaPersistence.ts @@ -6,7 +6,7 @@ * LICENSE file in the root directory of this source tree. */ -import { useState, useCallback, useRef, useMemo } from 'react'; +import { useState, useCallback, useRef, useMemo, useInsertionEffect } from 'react'; /** * Persistence adapter interface for schema save/load operations. @@ -233,10 +233,46 @@ function callableRefusalMessage(id: string, paths: string[]): string { export function useSchemaPersistence( adapter?: SchemaPersistenceAdapter, ): SchemaPersistenceResult { - const defaultAdapter = useRef(createLocalStorageAdapter()); - const adapterRef = useRef(adapter ?? defaultAdapter.current); - // Keep the ref up to date if the adapter prop changes - adapterRef.current = adapter ?? defaultAdapter.current; + // Built once per hook instance. `useRef(createLocalStorageAdapter())` ran the + // factory on EVERY render and discarded all but the first result; `useMemo` + // runs it once and — unlike a ref — needs no `.current` read during render. + // Re-creating it would be harmless either way: the adapter is a stateless + // facade over `localStorage`, so a fresh one behaves identically to the one + // it replaces, and nothing outside this hook ever sees its identity. + const defaultAdapter = useMemo(() => createLocalStorageAdapter(), []); + const resolvedAdapter = adapter ?? defaultAdapter; + + // The four callbacks below are created once (`[]` deps) and read the adapter + // at CALL time, so the latest adapter must reach them without changing their + // identity. That is the whole job of this ref. + // + // The write lives in `useInsertionEffect` — not in the render body, where it + // used to be, and not in `useEffect`/`useLayoutEffect`: + // + // - Render body: a render React discards or replays (StrictMode, a + // Suspense retry, a concurrent interruption) still performed the write, + // so a save could be routed through an adapter from a render that never + // committed. That is what `react-hooks/refs` flags. + // - `useEffect`: runs after paint, so a call made from any layout effect in + // the same commit would still reach the PREVIOUS adapter. + // - `useLayoutEffect`: a child's layout effects run BEFORE its parent's, so + // a child calling `save()` from its own layout effect would still see the + // previous adapter. + // + // Insertion effects run in the mutation phase — before every layout effect in + // the tree, before paint, and before any event handler can fire — so every + // call site that may legally invoke `save`/`load`/`list`/`remove` observes + // exactly what the old render-body write gave it. The single window that did + // change is a read during the render phase itself, which no legal consumer + // has: these calls are side effects and are never allowed during render. + // + // `useSchemaPersistence.adapterTracking.test.tsx` pins both halves — that a + // changed `adapter` prop is tracked, and that it is already in place by the + // time a child's layout effect runs in that same commit. + const adapterRef = useRef(resolvedAdapter); + useInsertionEffect(() => { + adapterRef.current = resolvedAdapter; + }, [resolvedAdapter]); const [loading, setLoading] = useState(false); const [error, setError] = useState(null); const [isDirty, setIsDirty] = useState(false);