fix(plugin-detail,components,plugin-list): re-key three fetch effects onto the primitives they read (#6697) - #6725
Merged
Conversation
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
|
… its output objectui#6697's ListView leg keyed the data-fetch effect on `JSON.stringify(expandFields)`. That is discard-immune, but it is not content-equivalent to what the effect reads, and it defeated objectui#4567's live-dependency pin: `buildExpandFields` collapses the collected set down to the relation roots, so a column change that leaves the expand set alone is invisible to the key — while the effect body still builds `$select` from `schema.columns` and the view bindings. Key on the memo's own INPUTS instead. They are props and state, so a discarded memo cache still cannot re-run the effect (what #6697 asked for), and every re-run the effect had before is kept (what #4567 ruled correct, with the identity stabilisation living at the producer). Adds the case that was missing locally: a column change that does NOT move `$expand` must still refetch, asserted on `$select`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CRJge11jso9TpXRWFt1Z49
This was referenced Aug 29, 2026
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
|
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 free
to 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 #6697
Item 3 triage verdict, first:
ListView'sexpandFieldsIS a memberThe card fenced item 3 as not confirmed ("
ListViewis large, central plumbing and a false positive here would be costly to chase"). Triaged against a positive/negative discard-proxy pin before the file was touched: confirmed, not a false positive. Against the pre-fix source the pin is RED —expected "vi.fn()" to be called 1 times, but got 2 times— one discard, one extradataSource.find.Two facts make
ListViewtriageable in isolation, both re-derived from the code rather than taken from the card:buildExpandFieldsreturns a FRESH array in every branch ([], a fresh collection, or a fresh.filter()result), so a recompute always moves the identity. There is no accidental stability to hide behind.expandFieldsandperms. Everything else is a prop, auseStatevalue, or a plain derived primitive. The discard proxy is scoped by marker so it discardsexpandFieldsalone and leavespermscached — which is what lets a failure nameexpandFieldsand nothing else.That second point produced a separate finding, filed as #6724 and NOT fixed here:
usePermissions()'s own memoised return is consumed by effect-dependency identity in the same effect. Same family, missed by the #6592 census because the memo lives in another package and arrives as an ordinary hook return.ListView's fetch path was checked against #6677 (in flight onplugin-grid's column derivation, withListViewas the fetching host) before this reading was claimed: different package, no shared file, and #6677 does not touch the$expandderivation or the fetch effect's dependency array.The change
useMemois a pure optimisation, not a correctness dependency: React may discard a cache and recompute even when the dependency array compares equal. All three factories here return a fresh value on every call, so each effect re-ran on a discard alone.plugin-detail/src/RelatedList.tsx— collection fetchdefaultSortSpec,listFilterNodedefaultSortKey,filterKeycomponents/src/renderers/layout/containers.tsx— count probeprobeTargets(aMap)probeKeyplugin-list/src/ListView.tsx— data fetchexpandFieldsexpandKeyOnly the re-run condition moves. Every effect body still reads the memoised value, and a genuine content change still refetches exactly as before — pinned by a negative control per member. The
useRef/ stable-identity hack stays out (#6018 ruling). Severity is Low, so this is not widened into a correctness refactor.Why the pins had to force the discard, not vary a prop
Item 1 was the live risk the dispatch flagged.
defaultSortSpec/listFilterNodeare ALREADY keyed onJSON.stringify-derived strings, so handing the component a freshdefaultSort/filterreference with equal content yields an EQUAL key and the memo simply keeps its cache — the trigger never fires. That is precisely howObjectCalendar's first pin in #6592 came out green pre-fix. The same is true for items 2 and 3 in different ways.So the pins replace
useMemoat the MODULE level and fire exactly one discard on demand. Two things had to be measured to get there, both recorded in the test headers:RelatedListandListViewreachuseMemothroughimport * as React from 'react'. Under Vite that namespace is a frozen[object Module]:vi.spyOn, plain assignment andObject.definePropertyall fail on it with "Cannot redefine property". Patching the separateimport React from 'react'interop default object succeeds and reaches nothing in those components. A first draft that patched only the default binding reported all four of its cases green against the unfixed source — an unfalsifiable pin, the exact failure this family already sprang once.provesTheProxyDiscriminatescanary that renders a probe through the SAME namespace binding the component uses, asserts caching still holds while armed-but-unfired, and asserts the identity moves after one discard. It is a permanent guard that the mechanism is live, not a one-off check.Red before, green after
Reverse-verified from the committed fix: all three source files reverted to
origin/mainin one leg, the mutation proved on disk (blob hash changed, plus a grep for the exact introduced/removed text), then restored viagit checkout HEAD --and proved byte-identical (hash match against the HEAD blob + emptygit diff HEAD).The 7 passing cases in the red leg are the three canaries and the four negative controls — they must stay green in both legs, and do.
One correction to the card
For item 2 the card calls the cost "an extra count-probe round trip". Measured while pinning it:
RelatedCountStore.fetchreturns the cached count as its first act and dedupes concurrent probes throughinflight, so re-probing a warm key issues NOdataSource.find. The wire cost of a discard is normally ZERO; the real observable is the redundantRelatedCountStore.fetchinvocation. The pin asserts both, so the distinction survives in the record.The same measurement corrected the pin itself: the store bumps its version when the first probe resolves, and
countsVersionis a dependency of the probe effect by design (#2269), so mount legitimately costs more than onefetchbefore it quiesces. The pin anchors on a RESTING count — anchoring on "called once" would have measured that designed re-probe and blamed it on the discard.Verification
All runs below are at
26b0e47da, the final commit, under the shared verify lock.pnpm exec vitest run --maxWorkers=2 packages/plugin-detail/ packages/plugin-list/—Test Files 164 passed (164),Tests 1745 passed (1745)pnpm exec vitest run --maxWorkers=2 packages/components/—Test Files 204 passed (204),Tests 1875 passed (1875)pnpm --filter @object-ui/components --filter @object-ui/plugin-detail --filter @object-ui/plugin-list run type-check— exit 0. Note the HYPHEN:typecheckmatches zero scripts in this repo and would exit 0 having run nothing. This script chainstsc --noEmit && tsc -p tsconfig.test.json, so the new test files ARE compiled — it caught five real type errors in the first draft of theRelatedListpin.check-vi-mock-specifiersOK (relevant: three newvi.mockcall sites) ·check-control-bytesOK ·lint coverage: 46/46 packages linted, 0 with outstanding errors·type-check coverage: 45/46 ... 41/41 packages compile their tests·changeset-presence/-no-major/-fixed/-overwriteOK ·phantom-depsOK ·self-importOK ·shell-escape-residueOK.check-eager-closure-budgetis NOT MEASURED, not red: it reports "No eager-closure report at apps/console/dist/eager-closure.json ... This is a broken gauge, not a passing budget" because the console was not built in this container. Nothing in this diff touches the console or adds an import.Lint was narrowed, and here is the evidence the narrowing excluded nothing. Instead of the whole farm, eslint ran on the six changed files:
6 files linted, 0 errors, 342 warnings(read from--format json, not from a summary line). The warnings are all pre-existing@typescript-eslint/no-explicit-anyand onereact-hooks/exhaustive-depswarning on an untouched memo. The narrowing is sound because this repo's eslint config declares noparserOptions.project/projectService— linting is not type-aware, so this diff cannot move the verdict on any file it does not contain. Also note this repo's lint is a per-packageeslint ., NOTeslint . --no-inline-config: running it with that flag surfaces tworeact-hooks/static-componentserrors inRelatedList.tsxthat its own inline suppressions legitimately silence, and both predate this branch.Changeset
patch, all three packages — scored as such because this is a behavioural bug fix with no API surface change, no new authorable key, and no change to what a correct schema renders; it removes a redundant round trip on a path the user cannot observe as data. It follows the two siblings this family already landed (#6698, #6701), which declared the same re-key aspatch.minorwould overstate it, andmajoris refused outright bycheck-changeset-no-major— this repo's fixed group of 39 packages tracks@objectstack's major and a major here would drag all of them.Generated by Claude Code
Generated by Claude Code