Uh oh!
There was an error while loading. Please reload this page.
Build useETagCache's config object once per hook instance, not once per render - #6885
Merged
Merged
Conversation
`useRef({ enabled, storage, storagePrefix, maxEntries, ttl })` evaluated that
literal on EVERY render and kept only the first result, so every later render
of every component using the hook allocated a five-key object that was thrown
away. The values now come from a `useMemo` keyed on the five of them, which is
also what the ref's `useInsertionEffect` write publishes.
Nothing observable changes: the object's identity is private to the hook, the
returned callbacks keep their `[]` deps and their identities, and every reader
still reads fields off `configRef.current`. A memo React chooses to discard is
harmless — it rebuilds an equal object, which is what every render used to do
unconditionally.
`useETagCache` has zero in-repo consumers, so no existing test would notice
either way. `useETagCache.configAlloc.test.tsx` measures the allocation
directly by recording the argument handed to `useRef` on each render: pin 1
fails on the previous code (three renders, three distinct objects) and pins
2a-2e keep a `useMemo(..., [])` over-fix from passing it by requiring a fresh
config, carrying the new value, per changed key.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013hfmP9hoMd3dJwTh85J4yBos-sam
commented
Aug 30, 2026
CollaboratorAuthor
Review — ACCEPTLanding gated on CI; the verdict on the diff is below. I checked the one thing the dispatch fenced, rather than taking the claimThe order said: "if the memoization changes WHEN anything re-runs, that is a semantics change, useInsertionEffect(()=>{configRef.current=config;});// ← no dependency arrayNo deps array means it ran on every commit before and still does. So the publish happens Also confirmed the diff needs no import change because The verification is the part worth keepingThe dispatch flagged that zero in-repo consumers means a green suite proves nothing, and the
Counts move by exactly the new file: 66/819 → 67/826. Lint's population is stated as measured Scope and grading
No human-floor or governed-surface concern: Generated by Claude Code |
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-sam
marked this pull request as ready for review
August 30, 2026 11:17
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
1 participant
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#6817
useETagCacheseeded its config ref with an inline object literal:useRefevaluates that argument on every render and reads it only to seedthe initial value, so every render after the first allocated a five-key object
that was thrown away — in every component using the hook, forever, to no effect.
This is the same pattern PR #6796 repaired in
useSchemaPersistence(
useRef(createLocalStorageAdapter())), and it takes the same shape that oneused: hoist the value into a
useMemoand hand the variable touseRef. Thedifference that earned this its own card is that clearing the
react-hooks/refswarning required the change there and does not here — sothis is the half of that class the lint rule structurally cannot see. The rule
is left alone deliberately: widening it is a gate change with its own
population, and the card does not ask for it.
What changed
packages/react/src/hooks/useETagCache.ts— the five resolved values now comefrom a
useMemokeyed on all five, which is also what the ref'suseInsertionEffectwrite publishes. The ref, its readers and the insertioneffect's scheduling are untouched.
Nothing observable moves. The object's identity is private to the hook: nothing
exports it, and every reader (
isExpired,setEntry,removeEntry,clearCache,fetchWithETag) reads fields offconfigRef.current. Thereturned callbacks keep their
[]deps and their identities. A memo Reactchooses to discard is harmless — it rebuilds an equal object, which is exactly
what every render used to do unconditionally.
The verification problem, and how it was answered
useETagCachehas zero in-repo consumers, so there is no call site toregress and no existing test that would notice either way. A green suite proves
nothing here on its own — the whole burden is on showing the allocation really
changed.
packages/react/src/hooks/__tests__/useETagCache.configAlloc.test.tsxmeasuresit directly. The config object has exactly one external observation point: the
argument handed to
useRefon each render. The file wrapsuseRef(delegatingto the real one, so the hook still runs on genuine React) and records that
argument.
useRefthe same object. Measured red on untouched
origin/mainbefore the fixwas written, then green after it, assertion unchanged.
useMemoon an empty dependency list wouldsatisfy pin 1 while freezing the config at its first render. One case per key
requires a fresh object carrying the new value when that key changes, so a
dependency list missing any one of the five fails here.
working would let the identity pins pass on an empty array.
useETagCache.configTiming.test.tsx, untouched and still green: the newestconfig reaches the stable callbacks, it is in place before a child layout
effect of the same commit, callback identity survives config changes, and
isExpiredjudges against the latestttl.Evidence
Suite,
pnpm exec vitest run packages/react/srcfrom the repo root (thecanonical invocation the config guard demands):
a04d7c60cb48f9Plus one file and seven tests, which is exactly the new pin file; no other file
moved.
Red-first, on the untouched source:
"Compared values have no visual difference" is the defect stated precisely:
equal content, different identity.
Reverse-verified from the commit, prediction stated first. Reverting only
useETagCache.tstoa04d7c6and leaving the pin untouched was predicted tofail at line 89 with that same
Object.ismessage, 1 failed / 6 passed. Thatis what happened. The mutation was proven on disk before the run
(
hash-object=f1cf647e, old literal present once, new form absent), andthe restore proven byte-exact after it:
hash-object=496af5bf=git rev-parse HEAD:packages/react/src/hooks/useETagCache.ts, andgit diff HEADempty. The script carried an EXIT/INT/TERM trap using absolutepaths. No build step is involved on either leg: the pin imports the hook by a
relative source specifier, so it reads the source file itself and no
distcan go stale under it.
Type-check —
pnpm --filter @object-ui/react type-check, echoingtsc --noEmit && tsc -p tsconfig.test.json, exit 0 after building thedependency closure.
--listFilesconfirms this is a real reading and not azero-match green: the source file appears in the
tsconfig.jsonprogram andboth test files appear in the
tsconfig.test.jsonprogram.Lint — the affected package's own
eslint ., exit 0, 132 files ineslint's own reported population (
--format json), 0 errors. All threetouched files are in that population.
useETagCache.tscarries exactly onewarning before and after —
react-hooks/set-state-in-effect, at line 227 onorigin/mainand line 241 here, the same pre-existingsetCacheSizecall inthe mount hydration effect, shifted by the added comment. The new test file
carries 0.
Narrowing declared: repo lint is
turbo run lintover every package; this ranthe affected package only.
eslint.config.jsconfigures no type-aware linting(no
parserOptions.project, noprojectService), so a diff confined topackages/reactcannot move the verdict on a file outside it. CI runs the fullfarm regardless.
Gates —
pnpm changeset:check(No changeset declares a major bump),pnpm check:control-bytes(OK (scanned 5737 tracked text file(s))), andpnpm check:vi-mock-specifiers(OK (... 498 carry a mock ...), up from 497,so the new mock is inside the scan and not skipped). All at
0cb48f9.Changeset
patchon@object-ui/react. Nothing a published consumer can observe changes— public shape, callback identities and the values read off the ref are all the
same, and the only difference is an allocation that no longer happens. Not
minor: there is no new capability. Notmajor, per the fixed-group rule.Scope
One card, one defect. #6818 is not addressed here — it is a stale-closure logic
gap, a different defect in the same directory, and it keeps its own card.
useSchemaPersistenceis not touched; PR #6796 already repaired it.Generated by Claude Code