Uh oh!
There was an error while loading. Please reload this page.
fix(react): stop writing config refs during render in useETagCache, useGlobalUndo and useOffline - #6815
Merged
Merged
Conversation
…ender The five resolved config scalars were assigned to `configRef` in the render body — one `react-hooks/refs` "Cannot update ref during render" on useETagCache.ts:204 — so a render React discarded or replayed still published its config to the five `useCallback([])` readers that outlive it. The ref itself is load-bearing: `isExpired`, `setEntry`, `removeEntry`, `clearCache` and `fetchWithETag` all have `[]` deps and their identity is part of the published result, so re-keying them on the config values would have changed `fetchWithETag`'s identity whenever a caller's `ttl` moved. Only the write moves, into `useInsertionEffect` — the mutation phase, ahead of every layout effect, ref attachment and paint. Four pins. The discriminating one drives the fully synchronous `clearCache` from a CHILD's layout effect in the same commit as the prefix change, so it fails under both `useEffect` and `useLayoutEffect`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CRJge11jso9TpXRWFt1Z49
… render `optionsRef.current = options` ran in the render body — one `react-hooks/refs` "Cannot update ref during render" on useGlobalUndo.ts:57 — so the options of a render React discarded or replayed still reached `executeOp`, `undo` and `redo`. All three in-repo callers (AppContent, RecordDetailView, useConsoleActionRuntime) pass a fresh inline literal with inline `onUndo` / `onRedo` closures on every render, 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. The write moves to `useInsertionEffect`. Four pins. The discriminating one calls `undo()` from a CHILD's layout effect in the same commit as the dataSource swap; `executeOp` reads `optionsRef.current.dataSource` synchronously, before `undo`'s first `await`, so the pin fails under both `useEffect` and `useLayoutEffect`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CRJge11jso9TpXRWFt1Z49
…n render `syncConfigRef.current = syncConfig` ran in the render body — one `react-hooks/refs` "Cannot update ref during render" on useOffline.ts:262. This hook is the odd one of the three: its ref has exactly ONE reader (`sync`, at `syncConfigRef.current?.batchSize`) and that reader is already unstable (deps `[enabled, queue]`). So the ref is not protecting an identity — it is protecting RETAINED closures: 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 change what the retained closure reads, so it was rejected and only the write moves, into `useInsertionEffect`. Four pins. The discriminating one syncs from a CHILD's layout effect in the same commit as the batchSize change; the `batchSize` read is synchronous, before `sync`'s first `await`, so it fails under both `useEffect` and `useLayoutEffect`. That caller fires exactly once on purpose: `sync` drains the queue, which re-renders and re-keys the effect, and an unguarded version re-fired until the queue emptied — reaching 0 whatever batchSize the first call had read. Measured: without the guard the pin passed even with the write moved to `useEffect`, so it was pinning nothing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CRJge11jso9TpXRWFt1Z49
…ooks Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CRJge11jso9TpXRWFt1Z49
Contributor
✅ Console Performance Budget
The eager closure is every chunk the entry reaches through static imports — what the browser fetches and parses before the app renders. The entry chunk on its own is a small fraction of it. 📦 Bundle Size Report
Size Limits
|
os-sales
marked this pull request as ready for review
August 29, 2026 22:15
Uh oh!
There was an error while loading. Please reload this page.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes#6797
Three
react-hooks/refswarnings, one apiece on three published hooks, eachCannot update ref during render. Each hook got its own reader-and-timinganalysis, its own shape decision and its own commit; the shapes converged, but
they were derived separately and the report below says where the three hooks
genuinely differ.
Measured on my own base, and the card's arithmetic corrected
Base
26896c689(not the card'sd06059f24). The three files have not moved:the card's line numbers are still exact.
react-hooks/refs26896c689)a77a00c2c)objectui#6796 has not merged, so
useSchemaPersistence's three warnings arestill present here and are still the three that remain after this branch
(
237:40,239:3,239:35— untouched, that file belongs to the other PR).Per hook, the count this branch owns:
useETagCache.ts:204:3useGlobalUndo.ts:57:3useOffline.ts:262:3The three new pin files add 3 to the file count and 0 warnings.
What is different from objectui#6796, measured rather than assumed
All three warnings here are the ref write only.
useSchemaPersistencealsohad two ref reads during render (
237:40,239:35), which is why its repairhad to change how the default adapter was held. Nothing of that kind applies
here: no reader of any of these three refs is reachable during render, so the
repair is strictly "move the write".
Each ref's readers, from a repo-wide grep of the ref identifier:
useETagCacheisExpired,setEntry,removeEntry,clearCache,fetchWithETag— alluseCallback([])useGlobalUndooptionsbagexecuteOp,undo,redouseOfflineconfig.syncsync, one siteWhy
useInsertionEffectfor each, decided per hookThe shared reason is the commit-phase window: every one of these readers is
handed to the consumer, and a consumer may legally call it from a layout effect,
an insertion effect or a ref callback — all of which run before passive effects,
and child layout effects run before the parent's. So
useEffectanduseLayoutEffectboth move a window that exists today, anduseInsertionEffect(mutation phase, ahead of every layout effect, ref attachment and paint) moves
only the render phase, where none of these functions is callable.
useEffectEventwould be idiomatic but is React 19.2+, and this package's peer range is
react: ^18.0.0 || ^19.0.0.What differs per hook is what the ref is protecting, which is what ruled out
the ref-free alternative in each case:
useETagCache— fiveuseCallback([])readers whose identity is part ofthe published result. Re-keying them on the config values is the tidiest React,
but it changes
fetchWithETag's identity whenever a caller'sttlmoves,re-firing any consumer effect keyed on it. Observable, so rejected.
useGlobalUndo— all three in-repo callers (AppContent.tsx:429,RecordDetailView.tsx:587,useConsoleActionRuntime.tsx:161) pass a freshinline literal with inline
onUndo/onRedoclosures on every render, andthe keydown effect is keyed on
undo/redo. Depending onoptionsdirectlywould re-register that listener every render. The ref is load-bearing.
useOffline— the odd one. Its single readersyncis already unstable(deps
[enabled, queue]), so the ref is not protecting an identity at all: itprotects retained closures. The auto-sync effect deliberately captures a
syncand fires it 100ms later, and that closure must still read the newestbatchSize. AsyncConfig?.batchSizedep would be a scalar and would notthrash, but it changes what that retained closure reads. Observable, so
rejected.
Pins
Three files, 4 pins each, one file per hook. Each file's discriminating pin
drives the hook's reader from a child's layout effect in the same commit as
the config change, which is the only window that separates the three candidate
shapes:
useETagCache.configTiming.test.tsx—clearCacheis fully synchronous, sothe prefix it clears is read straight out of the ref.
useGlobalUndo.optionsTiming.test.tsx—executeOpreadsoptionsRef.current.dataSourcesynchronously, beforeundo's firstawait.useOffline.syncConfigTiming.test.tsx— thebatchSizeread is synchronous,before
sync's firstawait.The remaining pins hold the properties the repair had to preserve: latest-value
delivery through the retained callbacks, and the callback identities themselves.
useOfflinecommit-phase caller was initially unguarded, sosyncdraining thequeue re-rendered, re-keyed the effect and re-fired it until the queue emptied —
reaching
0whateverbatchSizethe first call had read. It passed with thewrite moved to
useEffect, i.e. it was pinning nothing. It now fires exactlyonce, and fails on both legs.
Ablation, per hook
Implementation committed before every mutation. Each leg confirmed on disk by
comparing
git hash-objectagainst the HEAD blob (a mutation that did not changethe hash aborts the run), restored with
git checkout HEAD --on absolute pathsunder a
trap ... EXIT INT TERM, and each restore re-verified against the HEADblob.
packages/reacthas nodistand the pins import the hook by relativesource path, so no rebuild leg applies; legs B and C flipping results is itself
the evidence that the mutation reached the module Vitest loads.
useETagCacheuseGlobalUndouseOffline26896c689, new pins keptuseInsertionEffecttouseEffectuseInsertionEffecttouseLayoutEffectLeg A ran the whole
packages/reactsuite (64 files, 811 tests), once perhook, with that hook's implementation reverted and its pins kept.
Leg A is the honest headline, and it is the same answer for all three hooks:
no test fails when any of the three changes is reverted. The pins pass against
the old code and the new code alike, precisely because the timing was preserved.
For every one of these three hooks, warnings are the only thing that moves.
This is a lint-cleanliness change, not a bug fix; no user-visible break was
measured and none is claimed. What the old code additionally did — and this is
the defect the rule names — was perform the write on renders React discards or
replays, so a tree that never committed could publish its config to callbacks
that outlive it. The new pins guard the next edit (legs B and C), not a break
that exists today.
Verification
Union re-run after the final commit, at
a77a00c2c, tree clean:pnpm exec vitest run packages/react/—Test Files 64 passed (64),Tests 811 passed (811), including the 12 new pins.pnpm --filter '@object-ui/react' run type-check— pass. It runstsc --noEmit && tsc -p tsconfig.test.json;--listFilesconfirms all threeimplementations are in the first program and all three new pin files are in the
second, so the green covers the edits rather than merely coexisting with them.
pnpm exec eslint packages/react— 0 errors, 343 warnings (table above).check:control-bytes—OK (scanned 5651 tracked text file(s); skipped 85 binary).check:vi-mock-specifiers—OK.check:phantom-deps—Every in-scope import is declared by the package that publishes it.check:self-import—No package names itself inside its own src/.check:esm-specifiers—no un-ledgered package emits an extensionless relative specifier.check:readme-exportsis not measured here, not red: CI runsturbo run build --filter='./packages/*'before it (readme-exports.yml), andwithout that build all 334 findings are the same prerequisite —
its type entry ./dist/index.d.ts is not on disk -- run pnpm build first— inpackages this branch does not touch. Nothing in this diff adds or removes an
export or edits a README.
The lint run above is
packages/react, the only package this branch touches,rather than the repo-wide
pnpm lint; CI runs the farm exactly once regardless.That narrowing is a measurement rather than a gap: the population comes from
eslint's own config resolution, the count (129 files) is read from its
--format jsonoutput, andeslint.config.jsenables no type-aware linting —no
projectorprojectService,languageOptionscarries onlyecmaVersionand
globals— so every rule is single-file, and this diff cannot move theverdict on a file it does not contain.
Generated by Claude Code