fix(plugin-tree): derive ObjectTree expansion during render, not in a state mirror - #8745
Conversation
… state mirror Expansion lived in a `useState<Set<string>>(new Set())` that a passive `useEffect` keyed on `[roots, defaultExpandedDepth]` re-seeded from the forest, with rows computed as `flattenVisible(roots, expanded)`. The commit that first painted the table therefore still carried the previous, empty mirror: the root drew, its children did not, and a second commit drew the seeded-open forest. Probed in the DOM the sequence was `loading -> table:1rows -> table:2rows`; it is now `loading -> table:2rows`. State now holds only the user's own answers (a sparse id -> open/closed map written solely by chevron clicks) and the seed is computed from the forest during render. The two compose by one rule, stated on `resolveExpanded`: a new forest may re-seed, but a node the user deliberately opened or closed, and which is still in the forest, keeps the user's answer; every other node, a genuinely new one included, takes the seed. Storing the user's EDITS rather than the resolved set is the point, not an implementation detail: a set of open ids cannot tell "the user closed this" apart from "the seed never opened it", so re-seeding it either loses the user's collapse on every identity change of `roots` (which is what main did) or never opens a genuinely new subtree. Pinned in ObjectTree.expandedDerived-8666.test.tsx in two groups, because the frames pin alone is passed by "seed during render and ignore the overrides" -- an implementation strictly worse than the bug. No assertion in ObjectTree.contractEnvelope-6839.test.tsx was standing on the two-commit sequence and none changed; its header gains a note saying so. Part of #8666 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YBWFb5YgMU5dw8p2VKj16S
`defaultExpandedDepth` was documented as "Initial expansion depth" and nothing said what happens to it when the record set changes. It now seeds a set derived during render, and a node the user opened or closed keeps the user's answer across a re-seed while it is still in the forest -- which is the observable half of the change and belongs in the README next to the key it qualifies. Part of #8666 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YBWFb5YgMU5dw8p2VKj16S
github-actions
Bot
added
documentation
✅ 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
|
|
Contract review — accepted, flipped out of draft, auto-merge armed. No failures; shards, Type Check and Lint still running. ⭐ The most valuable thing in this PR is a correction to my own ruling. I framed override-loss as a hazard of the conversion — something a naive derivation would introduce. Measurement says The composition rule is right, and the reason it is right is the part to keep. Storing the user's edits as a sparse
Re-seeding such a set has exactly two outcomes and both are wrong — overwrite and the user's collapse dies on every identity change, or union/skip and a genuinely new subtree never opens. That is a real derivation, not a preference, and it is what forced The two caricatures did the job the ruling asked of them, and the asymmetry is the lesson:
Caricature B is caught by exactly one case, the new-subtree one — 58 of 59 package tests pass under an implementation that never re-seeds. That is "the vivid assertion is rarely the discriminating one" with a number attached, and it is why the second group had to exist: both frame pins pass under caricature A, measured rather than assumed. The DOM recorder is honest about its own resolution. A On the The out-of-scope filing is the other thing that earned its place. objectui#8743 — Also noted: catching the Generated by Claude Code |
2 participants
Fixes #8666
useStateof a SET OF STRING, and a READONLY MAP FROM STRING TO BOOLEAN.Two deliverables, and the second is the discriminating one
1 — no intermediate commit. Expansion lived in a
useStateholding a SET OF STRING that a passiveuseEffectkeyed on[roots, defaultExpandedDepth]re-seeded from the forest, with rows computed asflattenVisible(roots, expanded). The commit that first painted the table therefore still carried the previous, empty mirror: the root drew, its children did not, and a second commit drew the seeded-open forest.2 — a user's expansion survives a re-seed. State now holds only the answers the user gave by clicking a chevron — a sparse READONLY MAP FROM STRING TO BOOLEAN, id to open/closed — and the seed is computed from the forest during render. The composition rule, stated in the docblock on
resolveExpandedand in the package README:Why the user's EDITS are stored and not the expansion set
This is the part that decided the design, not an implementation detail. A set of open ids cannot tell "the user closed this node" apart from "the seed never opened it". So re-seeding such a set has exactly two outcomes and both are wrong: overwrite, and the user's collapse is lost on every identity change of
roots; or union/skip, and a genuinely new subtree never opens. Recording the user's edits separately is what makes both halves derivable at once.⇒ That also makes this a behaviour fix in the same direction, not a side effect. Before this change a re-seed overwrote the user's expansion, so any refetch, filter, or host re-render that reallocated the rows silently reopened every subtree the user had collapsed. That is measured below, not asserted: the override pins are red against the pre-fix component.
Measurement
The DOM commit sequence, before and after
Recorded by a
React.ProfilerwhoseonRenderreads the container on every commit of the tree's subtree; only consecutive identical snapshots collapse, so a one-row table and a two-row table can never merge.origin/main645087cd3loading -> table:1rows -> table:2rowsloading -> table:2rowsBoth arms of the frames pin record the same sequence. The second arm defers
find()by 50ms rather than 0:@testing-library'sasyncWrapperdrains one macrotask before returning, so a 0ms deferral resolves inside that drain window and would leave the ordering unforced.Every pin observed RED, with base-blob provenance
The pre-fix leg is the component restored from the base blob, proven by hash rather than by "I put the old code back". Each leg proved the mutation reached disk before the run, and proved the restore by
git diff HEADbeing empty afterwards:Per-case outcomes, from vitest's JSON reporter — never the text reporter, because a module-scope
TypeErrorreads there as a suite death rather than a failing assertion:defaultExpandedDepthacross a re-seedpackages/plugin-tree/⭐ Which pin catches which. Caricature A is the mutation the card warned about — it removes the intermediate commit and answers the same thing for every input, an implementation strictly worse than the bug. Both frames pins pass under it, which is the measured reason the second group has to exist; the three override cases catch it. Caricature B is caught by exactly one case, the new-subtree one: 58 of 59 tests in the package pass under it, both survives cases included, because a frozen seed leaves the user's answer alone. Caricature A also reds the pre-existing
ObjectTree.test.tsxtoggle test, which is corroboration, not the pin.The PR #8664 repair: nothing was standing on the two-commit sequence
⭐ Measured directly rather than reasoned about:
ObjectTree.contractEnvelope-6839.test.tsxis green in all four columns above — against the pre-fix component, against both caricatures, and against this branch. Its positive arms wait FOR the descendant row, which is a condition on the settled forest and not on how many commits produced it. No assertion in it changed.What did change there is prose: its header carries a MEASURED description of the race as a live property of
ObjectTree, and that statement is now false. Rather than quietly editing the analysis away, the header gains a dated note saying the mechanism was fixed here, that nothing in the file changed for it, and why the waits stay shaped as they are. The⛔ do not fold these back into one waitinstruction is untouched and still correct.Verification, and what is NOT MEASURED
pnpm exec vitest run packages/plugin-tree/from the repo ROOT with a path filter — 13 files, 59 tests, 0 failed, atecbd71bba. Neverpnpm --filter PKG exec vitest FILE, never a package cwd, never--no-inline-config.pnpm --filter @object-ui/plugin-tree run type-check— exit 0, after building the dependency closure (turbo run build --filter="@object-ui/plugin-tree^..."). ⭐ Confirmed bytsc --listFilesthat the new pin file is actually in the program (14 of the package's source files, the new one among them) rather than excluded — atype-checkthat skips tests is NOT MEASURED, not green.check:doc-snippets,check:readme-exports,check:doc-types,check:doc-fences,check:control-bytes,check:vi-mock-specifiers,check:vi-mock-inherit,check:unreferenced-sources,check-changeset-presence— all exit 0. The first two were run only after the scoped build they demand (96s); unbuilt they exit 2 / reportthe population COLLAPSED -- this run proves nothing, which is a precondition failure and not a verdict.pnpm --filter @object-ui/plugin-tree run lint— 0 errors. The warnings are allno-explicit-anyon test fixtures, the house convention in this package, plus one pre-existingexhaustive-depswarning on the record-fetch effect that is unchanged by this diff.clientWidth: 0, so the pins count nodes and readdata-depth, never the inlinepaddingLeftthe same element carries. Also not measured: whether the flash is perceptible on a slow device.Scope
⛔
ObjectKanbanis not folded in. Its prop-mirroredboardColumns(PR #8533) is the same shape and is a separate card; it was not touched and not measured here.check:doc-examplesis red onorigin/mainat645087cd3— an undeclared@examplefailure atpackages/types/src/zod/imported-defaults.ts:318, becausestripImportedDefaultsis not on a public entry so the gate injects no import for it. Zero files underpackages/typesare in this diff and the block is byte-identical at the base commit, so it is not caused by this change and is not repaired by it.Changeset
.changeset/8666-tree-expanded-derived.md,patchon@object-ui/plugin-tree. Required: this is a user-visible rendering change. Theskip-changesetlabel is a phantom in this repo and the empty-frontmatter exemption does not apply.majoris banned repo-wide and is not wanted here — no schema key changes anddefaultExpandedDepthmeans exactly what it meant.Session that produced this change:
https://claude.ai/code/session_01YBWFb5YgMU5dw8p2VKj16S(written as prose and as a code span, because footer links do not reliably survive a body edit).🤖 Generated with Claude Code
https://claude.ai/code/session_01YBWFb5YgMU5dw8p2VKj16S
Generated by Claude Code