fix(react): stop writing the useSchemaPersistence adapter ref during render

.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.

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<string, unknown>) => Promise<string | null>;
+
+ 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 <Child token={token} save={persistence.save} />;
+ }
+
+ const view = render(<Host adapter={first} token="first" />);
+ await act(async () => {});
+
+ expect(log).toEqual(['first:from-layout-first']);
+
+ await act(async () => {
+ view.rerender(<Host adapter={second} token="second" />);
+ });
+
+ // 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();
+ });
+});

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<Error | null>(null);
  const [isDirty, setIsDirty] = useState(false);