Uh oh!
There was an error while loading. Please reload this page.
fix(react): memoise the scoped-style schema SchemaRenderer hands down - #6591
fix(react): memoise the scoped-style schema SchemaRenderer hands down#6591os-support-ai wants to merge 4 commits into
Conversation
…yle node Measures, through the real SchemaRenderer path, whether the `schema` object a downstream component receives keeps its identity across a parent re-render. Fails today for a node carrying a sized `responsiveStyles` breakpoint: the scope-class merge allocates a fresh object every render, so every downstream `[schema]` memo re-runs. Plain and `base`-only nodes stay stable (controls). Refs objectui#6270 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011SfZeFWrhGLHmfq61xbz4q
A node carrying `responsiveStyles` (ADR-0065) takes a branch that rebuilds the schema object to merge the scope class into `className`. That spread was not memoised, so it allocated a new object on every SchemaRenderer render even when the `evaluatedSchema` memo above it held — and every downstream renderer keyed on `[schema]` saw a fresh identity and re-ran. Computed in a memo keyed on `[evaluatedSchema, autoStyleId]`, hoisted above the early returns: the use site sits after `if (!evaluatedSchema) return null` and the `_hidden` return, so a useMemo written there would be a conditional hook and a node toggling hidden -> visible would crash on the hook count. Fixes objectui#6270 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011SfZeFWrhGLHmfq61xbz4q
Adds the pin for the card's second claim: a consuming renderer's OWN setState re-renders only its subtree, so it keeps the exact schema object SchemaRenderer handed it. That is what makes this a redundant-recompute cost and not a refetch loop. Green on both sides of the fix by design. Also replaces the module-level `let` the harness reassigned during render with real click events — that reassignment is a render side effect (`react-hooks/globals`, 3 errors), and in a file that measures render counts the instrument must not break the rule under test. Adds the changeset for the @object-ui/react behaviour change. Refs objectui#6270 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011SfZeFWrhGLHmfq61xbz4q
…force-update Reverse-verification caught this test contradicting its own doc comment: it claimed to be green on both sides of the fix, and went red on the pre-fix tree. Cause: it counted identities from mount. SchemaRenderer force-updates itself once after mounting (it re-checks ComponentRegistry for a lazily registered component), so mount alone yields two SchemaRenderer renders — and pre-fix those two already handed down two different schema objects. The test was folding that parent-render instability in and duplicating the fix-case test instead of pinning the independent property. Now measured from the last render before the first click, so it covers only re-renders the parent took no part in. Refs objectui#6270 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011SfZeFWrhGLHmfq61xbz4q
os-support-ai
commented
Aug 26, 2026
PM review — ACCEPTED on substance. ⛔ NOT enqueued: this head has no verdict yet. SubstanceAccepted. The three things this seat asked for are all present and all measured rather than asserted:
Both inherited claims were re-measured and both came back with corrections, both accepted: the exposure was ~1 day old, not long-standing ( ⛔ Both dispatch constraints verified against the file list: three files, none under The scope conflict — resolved, and not on this PRResolved on the card: this PR keeps ⛔ Why this is not going into the merge queueRead at head
The GitHub Actions farm has not been scheduled for this head at all. The Draft status is not the explanation: PRs #6587 and #6589 are both drafts and both have Actions runs on their heads right now. The two live candidates are a scheduling backlog and a push that landed without firing its trigger; this seat has established neither, so it is not acting on either. First step for whoever picks this up: re-read the head's check-suites. If the Actions suite is still absent, Generated by Claude Code |
✅ 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-support-ai
commented
Aug 26, 2026
Correction to my review comment above — the farm was not missing, it was late. Re-read at 16:56Z on the same head
Still in progress: So the reading was accurate when taken and the conclusion drawn from it was not: I offered two candidates, a scheduling backlog and a push that never fired its trigger, and the answer was neither in the shape I meant — it was ordinary scheduling latency of roughly ten minutes on an unchanged head. ⛔ Ignore the "first step" in my comment above. Do not run The actual next step is the ordinary one: wait for the six running checks, then judge on every check green rather than the required subset, on this head. This PR remains ACCEPTED on substance and ⛔ not enqueued. ⭐ Worth carrying into the lane's readings: zero scheduled checks is NOT MEASURED, and "not measured" is a statement about the clock as much as about the trigger. Re-read before diagnosing; an absent farm at T+26min was a full farm at T+37min. Generated by Claude Code |
os-support-ai
commented
Aug 26, 2026
✅ Cleared to land — review complete against the code tree. Blocked only on API quota.Supersedes the "not enqueued" hold in my two comments above. Nothing about the change is outstanding; the only thing missing is the mechanical flip-and-enqueue, which I could not perform. Verdict on the reviewed head
⛔ Entry qualification was judged on every check green, not the required subset. The code tree was read, not the reportReviewed the diff at
One thing the refactor could have silently changed, checked rather than assumed. The old branch tested Why it is not in the queue
Remaining steps for whoever picks this up — no further review needed:
⛔ If the head moves for any reason, this verdict expires with it and the checks must be re-read on the new head. Generated by Claude Code |
Fixes#6270
Verified at
bc5773fbb.The defect, re-measured
The card named
SchemaRenderer.tsx:1076; the line is now:1252(re-measured, not inherited):Confirmed: for a node taking the
scopeClassbranch this allocated a new object on everySchemaRendererrender, even when theevaluatedSchemamemo directly above it held. Measured through the realSchemaRendererpath with a probe registered in the realComponentRegistry:schemaidentities across 1 parent re-renderresponsiveStyles: { base: … }(control)responsiveStyles: { large: … }(fix case)The fix is a hoist, not a one-line
useMemoThe card's suggested shape — "memoize
schemaForComponenton[evaluatedSchema, mergedClassName]" — cannot be written at that line. The use site sits afterif (!evaluatedSchema) return null, the_hiddenreturn, and the unresolved-component returns. AuseMemothere is a conditional hook: a node toggling hidden → visible would call one more hook than the previous render and React would throw "Rendered more hooks than during the previous render."So the whole ADR-0065 scoped-styling computation moves into one
useMemokeyed on[evaluatedSchema, autoStyleId], hoisted above the early returns, returning{ scopeClass, scopedCss, mergedClassName, schemaForComponent }. That dep set is complete: every value is a pure function of the evaluated node (className,id,responsiveStyles) plus theuseIdfallback.The non-scoped branch keeps handing down
evaluatedSchemaitself, never a copy — a copy there would spread the same instability to every node in the tree.Anti-staleness is pinned, not assumed
Memoising an object that legitimately changes is this fix's failure mode, so four tests pin the opposite direction — a changed
className, a changed value, a changed breakpoint, and (the strongest) a live interpolated value with theschemaprop identity held constant, driven throughPageVariablesProvider. All four must deliver a new identity.Reverse verification
Direction predicted before running, then measured. Pre-fix
SchemaRenderer.tsxrestored from the pinned merge-base6a7893d57; mutation and restore both proven on disk (anchored grep counts in both directions +git hash-objectagainst the HEAD blob), never by an exit code.Exactly the two fix-direction assertions go red; every control stays green.
The first pass predicted 2 red and got 3 — the boundedness pin went red too, contradicting its own doc comment. Cause: it counted identities from mount, and
SchemaRendererforce-updates itself once after mounting (re-checkingComponentRegistryfor a lazily registered component), so mount alone yields two renders — which pre-fix already handed down two different objects. It was folding parent-render instability in and duplicating the fix-case test. Now measured from the last render before the first click, so it covers only re-renders the parent took no part in. Fixed inbc5773fbb; the run above is the corrected one.The two inherited claims, re-measured
1. "Not new exposure from #6018" — CONFIRMED, with a date correction.
[schema]bymapConfig538ed9246dataConfig2aa2c226amapConfig's[schema]keying predatesdataConfig's by one day, so #6018 genuinely only made a second memo share an existing exposure.mapConfigwas not keyed on[schema]at all, it was unmemoised. The exposure was ~1 day old when the card was filed, not ancient.Worth recording:
ObjectMap's own memo doc comments assert that "the identity that reaches this component is ALREADY stable across the renders that matter" and nameSchemaRenderer'sevaluatedSchemaas one of three callers handing over a memoised node. That sentence was false for scoped-style nodes until this PR. It is true now.2. "Bounded — it is NOT an infinite loop" — CONFIRMED, and now pinned as a test rather than an argument.
React re-renders only the subtree below the component that set state, so a consumer's own
setDatanever re-runsSchemaRenderer, and the consumer keeps the very object React last handed it. The fetch effect's other deps (schema.filter,schema.sort) are nested references a shallow spread preserves, so they were stable even pre-fix. Cost was one redundant refetch cycle per parent render, never runaway.a consumer re-rendering itself keeps the exact schema object it was handedfixes this as a property of the renderer; it is green on both sides of the fix and turns red only if something upstream starts re-renderingSchemaRendererin response to a consumer's state.SchemaRenderer's mount effect callsforceUpdate()once to re-check the registry, so every mount of a scoped node already handed its child two different schema objects. Still bounded; just more frequent than "on parent render" reads.The reproduction trap, pinned
hasResponsiveStylesrequireslarge/medium/small/xsmall;{ base: … }does not take the branch. The first test proves from the deliveredschema.classNamewhich fixture is on which side, so the trap cannot silently turn the suite into an assertion that cannot fail. Every zero reading in it has a positive control in the same query shape.Gates — each one's own verdict line
vitest run packages/react/ packages/plugin-map/Test Files 77 passed (77)/Tests 999 passed (999)vitest run --shard=1/4(whole repo)Test Files 529 passed (529)/Tests 6562 passed | 1 skipped (6563)vitest run packages/core/src/styling/ packages/plugin-detail/Test Files 111 passed (111)/Tests 1041 passed (1041)pnpm --filter @object-ui/react type-checktsc --noEmit && tsc -p tsconfig.test.json)eslint packages/react --format jsonnode scripts/check-changeset-presence.mjs✅ … declares 1 changeset(s)node scripts/check-changeset-no-major.mjs✅ No changeset declares a major bump.node scripts/check-control-bytes.mjs✅ OK (scanned 5444 tracked text file(s); skipped 85 binary)node scripts/check-self-import.mjs✅ No package names itself inside its own src/.check:vi-mock-specifiers/check:shell-escape-residue✅ OKExit codes captured before any pipe (
cmd > file 2>&1; EXIT=$?), never read off atail.Type-check coverage was verified, not assumed —
tsc -p tsconfig.test.json --listFilesconfirms bothSchemaRenderer.tsxand the new test file are in the checked set (positive control: an existing sibling test is too). Atype-checkthat excluded*.test.tswould have been a true statement about nothing.Declared narrowing.
pnpm lint(turbo run lint, whole repo) and shards 2–4 were not run: a single shard exceeded the 600s foreground cap. What replaces them, and why it excludes nothing this diff could move: the behaviour change reaches only nodes carryingresponsiveStyles, and the complete set of files mentioning it iscore/src/styling/scoped-styles.ts(+ its test),plugin-detail/src/synth/buildDefaultPageSchema.ts,react/src/SchemaRenderer.tsx, the new test, and twoindex.cssfiles — every one of those packages is in a green run above. For lint: the rooteslint.config.jsdeclares noparserOptions.project/projectService, so type-aware linting is off and no untouched file's verdict can move under this diff;packages/react(the only package with changed source) is fully linted at 0 errors. CI still runs the full farm.Scope note for the reviewer
plugin-mapand every downstream memo key were left untouched, per the dispatch order.dataConfig.provider,dataConfig.object) souseMemoreturns to being a pure optimisation — and the dispatch order I received explicitly forbids touching downstream renderers' memo keys. I followed the dispatch order and am flagging the conflict rather than picking a side. That work is unstarted and still worth doing:useMemois not a semantic guarantee, and the fetch effect's correctness currently rests on a cache React is allowed to discard. This PR makes the identity stable; it does not make it guaranteed.Generated by Claude Code