fix(bindx-react): give useEntityList items a stable accessor identity (#64) - #73
Closed
matej21 wants to merge 3 commits into
Closed
fix(bindx-react): give useEntityList items a stable accessor identity (#64)#73matej21 wants to merge 3 commits into
matej21 wants to merge 3 commits into
Conversation
matej21
marked this pull request as draft
August 19, 2026 13:16
matej21
marked this pull request as ready for review
August 19, 2026 13:30
…useEntityList Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…#64) useEntityList rebuilt every item's EntityHandle on every store version bump, including items whose data did not change, so item accessor identity was unstable across renders. That defeats React.memo in list consumers — editing one item re-rendered every sibling's subtree — and it cascaded: a fresh root handle starts with an empty relationHandleCache, so every nested HasOne and HasMany handle and their per-item proxy caches were rebuilt too. Items are now cached per (entityType, entityId) for the hook's lifetime: one handle and one proxy per id, reused for the id's whole life, with ids no longer in the list evicted on rebuild. The cache is dropped whenever a handle construction input changes, notably selectionMeta. Identity is deliberately NOT a change signal. Making it one would require a total per-entity change signal, and EntitySnapshot.version is not one — notifyEntitySubscribers bumps the parents' versions but never the notified key's own, so errors, touched, scheduled-deletion and optimistic persisting flags never move it. Keying re-wraps on it looked right and silently broke memoized rows for all of those. Instead identity means identity, and change delivery is the subscription's job — the contract PR #56 established for accessors generally. The reproducer's memoized Row is amended to subscribe via useField, which is the contract it now tests. All seven assertions are byte-identical to the original; only the component and its props type changed. Known limit, documented on the cache: a membership change on a DESCENDANT relation does not reach a subscriber on the root item, because notifyRelationSubscribers does not walk up the parent chain. Such a row must subscribe to the owner of the relation it renders. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GiniFfaE4gb5EuQpQ3Ncee
…ear (#64) Review follow-up to the stable accessor identity. Once identity stops churning, anything memoized that does not subscribe goes silently stale. Two such places were reproduced, both of which worked before only because identity churn happened to re-render them. createComponent() with an implicit entity prop never subscribed. useRenderProps called useAccessor only for entity props carrying a selector, so .entity('author', schema.Author) — the mode whose whole point is implicit selection collection — got no subscription while ComponentImpl is memo-wrapped. Editing the entity left the component rendering the old value indefinitely. All declared entity props are now subscribed. The hook count stays constant: entityConfigs is fixed when buildComponent runs and never mutated, and a declared-but-unpassed prop still consumes exactly one slot through useAccessor's noop path. SnapshotStore.clear() notified global subscribers only, so a row subscribed exactly as the accessor contract prescribes kept rendering wiped data — and unlike the descendant-relation limitation, no subscription a consumer could write fixed it. clear() is the documented logout / teardown / schema-switch path. SubscriptionManager gains notifyAll(), which bumps the global version once and then dispatches to every entity, relation and global subscriber; it snapshots the registries first, since a subscriber may unsubscribe itself or a sibling while being notified. Registrations are deliberately not dropped — those components are still mounted and still own their unsubscribe closures. notify() semantics are untouched. Eviction is now tested. The earlier claim that a black-box test is vacuous was wrong: an id that leaves and re-enters the list must get a NEW accessor, which passes with the eviction loop and fails without it, entirely through the public hook. ItemAccessorCache moves to its own module — useEntityList.ts had grown past the file-size guideline — and a comment pins the invariant that the items array identity is deliberately unstable, since nothing in the DataGrid render chain subscribes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GiniFfaE4gb5EuQpQ3Ncee
matej21
force-pushed
the
fix/unstable-accessor-identity
branch
from
August 20, 2026 09:34
a4b15b6 to
a6f4f35
Compare
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
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 #64.
Problem
useEntityListrebuilt every item'sEntityHandleon every store version bump — including items whose data did not change — because the list cache is keyed on the globalstore.getVersion(). Item accessor identity was therefore unstable across renders, which defeatsReact.memoin list consumers: editing one item re-rendered every sibling's subtree. It also cascaded — a fresh root handle starts with an emptyrelationHandleCache, so every nestedHasOne/HasManyhandle and their per-item proxy caches were rebuilt too.Change
Items are cached per
(entityType, entityId)for the hook's lifetime: one handle and one proxy per id, reused for the id's whole life, with ids no longer in the list evicted on rebuild. The cache is dropped whenever a handle-construction input changes, notablyselectionMeta.Why identity is not a change signal
The first attempt made identity a per-entity change signal, re-wrapping the proxy when
EntitySnapshot.versionmoved. That was reviewed and rejected, because that version is not a total signal:notifyEntitySubscribersbumpsglobalVersionand the parents' snapshot versions, but never the notified key's own. Six staleness classes were reproduced against a memoized row —field.addError,field.touch(),scheduleForDeletion, optimisticsetPersisting, and descendant relation membership would all have been missed permanently. All of them work today via the global rebuild, so it would have been a regression, not merely an unfixed gap.So identity means identity, and change delivery is the subscription's job — the contract PR #56 established for accessors generally:
Re-review confirmed 5 of the 6 classes are resolved once the row subscribes.
Known limit (documented on the cache)
A membership change on a descendant relation does not reach a subscriber on the root item.
notifyRelationSubscribersnotifies the relation key and its owning entity but, unlikenotifyEntitySubscribers, does not walk up the parent chain. A memoized row renderingitem.profile.tagsmust subscribe to the owner of that relation —useAccessor(item.profile.tags)— not toitem.name.Related footgun, now load-bearing and called out in the doc comment:
useAccessor(item.profile)subscribes to the row itself, because a has-one ref reports its owner, not its target. Reach through to the nested relation or.$entity. The composed primitives (<HasMany>/<HasOne>) already resolve the right key, so JSX consumers get this for free.The amended reproducer
The reporter's reproducer is cherry-picked with their authorship, then amended in the fix commit — visible as a diff rather than folded in. Only the
Rowcomponent and its props type changed:Rownow readsuseField(item.name).valueinstead ofitem.name.valuefrom props, which is the contract it tests. All seven assertions are byte-identical to the original; nothing skipped or widened.Mutation-verified in an isolated copy of the repo: disabling identity reuse produces exactly 5 failures, both frozen tests among them, and frozen test 2 fails on the right assertion (
renderCounts['author-2']expected 1, received 2) — the original defect.Not verified
ItemAccessorCacheis private — exporting it purely for a test would either widen the public API or break the package boundary, so it stays private and this gap stays open honestly.HasManyListHandle.itemHandleCacheProxyonly ever inserts, and the root handle now survives the whole list, so paging a nested has-many retains a handle per nested id ever rendered. Same class already existed under<Entity>since refactor(bindx-react): stable EntityHandle identity (fine-grained reactivity) #56; this widens where it is reachable from. Worth its own issue.happy-domdoes not exercise React's concurrent scheduler;getSnapshotstill mutates the cache during render, so an abandoned render can evict entries the committed tree holds (consequence: identity churn, not wrong data).Verified:
tests/react/hooks/useEntityList/47 pass,tests/react326 pass, typecheck clean.🤖 Generated with Claude Code
https://claude.ai/code/session_01GiniFfaE4gb5EuQpQ3Ncee
Follow-up commit: the contract's consequences, fixed
An independent review reproduced two places that went silently stale once identity stopped churning. Both worked before only because identity churn happened to re-render them — so they are this PR's to fix, not pre-existing excuses.
1.
createComponent()with an implicit entity prop never subscribed.useRenderPropscalleduseAccessoronly for entity props carrying a selector, so.entity('author', schema.Author)— the mode whose entire point is implicit selection collection — got no subscription whileComponentImplismemo()-wrapped. Editing the entity left it rendering the old value indefinitely.All declared entity props are now subscribed. The hook count stays constant:
entityConfigsis fixed whenbuildComponentruns and never mutated, and a declared-but-unpassed prop still consumes exactly one slot throughuseAccessor's noop path. Mutation-verified by reverting the single line — the implicit test fails again, the explicit twin keeps passing.An earlier version of this description claimed "all 15 memoized components were enumerated and each subscribes or loads its own data." That was wrong.
2.
store.clear()left a correctly-subscribed row rendering wiped data. It notified global subscribers only, and unlike the descendant-relation limitation, no subscription a consumer could write fixed it.clear()is the documented logout / teardown / schema-switch path.SubscriptionManager.notifyAll()bumps the global version once, then dispatches to every entity, relation and global subscriber. It snapshots the registries before dispatching, because a subscriber may unsubscribe itself or a sibling while being notified. Registrations are deliberately not dropped — those components are still mounted and still own their unsubscribe closures.notify()semantics are untouched, andclear()was its only other caller.3. Eviction is now tested, and the earlier justification was wrong. This PR claimed a black-box test is "provably vacuous". The premise held — deleting the loop leaves the suite green — but the conclusion did not: an id that leaves and re-enters the list must get a new accessor. That test passes with the loop and fails without it, entirely through the public hook, with
ItemAccessorCachestill private.4.
ItemAccessorCachemoved to its own module (useEntityList.tshad grown past the ~300-line guideline), and a comment pins the invariant that theitemsarray identity is deliberately unstable — nothing in theDataGrid → DataViewContext → DataViewEachRow → renderCellchain subscribes, so stabilising that array too would make every cell go stale in one step.Known, deliberately not fixed
<If condition={cond…}>and<Switch>/<Case if={cond…}>are the same class of bug.If.tsx:56-57passesnulltouseFieldon the Condition-DSL path;Switch.tsx:122-133does the equivalent. They work today only because theconditionprop is freshly allocated on each parent render — luck, not contract. Any future memoization of that prop breaks them silently and nothing in the suite would catch it.notifyRelationSubscribersdoes not walk up the parent chain. Documented on the cache: such a row must subscribe to the owner of the relation it renders. NoteuseAccessor(item.profile)subscribes to the row, not the target, because a has-one ref reports its owner — reach through to the nested relation or.$entity.UnfetchedFieldErrortransient on a selection-widening render (listCacheRef's hit key ignores the selection) is unpinned and out of scope.HasManyListHandlecaches are insert-only and now live as long as the list: two maps, each retained handle carrying its own field and relation caches, so retention is a nesting subtree, andgetByIdretains ids never rendered. Bounded by ids ever seen and released on unmount — retention, not an unbounded leak. Worth its own issue.Branch gated in isolation: typecheck clean,
bun run test1537 pass / 0 fail,tests/repeater19 pass.