fix: consolidate correctness and reactivity follow-ups - #85
Open
matej21 wants to merge 32 commits into
Open
Conversation
…eates (#70) buildSelectionFromOps unioned scalar fields across sibling create/update ops but kept nested relations in a Map keyed by field name, so the last sibling's shape won. Two blocks whose nested `button` creates carried different fields emitted a node selection covering only one of them, so the response could not be content-matched back to the other sibling: its nested creates kept their temp IDs while still being committed as existing on the server, and the next edit went out as an update keyed by `__temp_...`, which the API rejects. Nested payloads are now accumulated per field name across every sibling and fed back through buildSelectionFromOps, so the union is recursive by construction for both has-one and has-many, nested-in-nested included. The walker is rebuilt around one primitive (buildSelectionFromDataObjects) and is now cast-free; a latent Object.entries(null) crash on a present-but-null `data` is guarded by the new isRecord type guard. The selection tests live under tests/unit/ rather than tests/bindx-client/ because only tests/unit, tests/react and tests/cases are in the CI script. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GiniFfaE4gb5EuQpQ3Ncee
…70) Widening the mutation node selection also widened the content matcher: the selection is its input, so selecting a nested relation that last-wins previously dropped makes isCreateDataMatchingNode recurse into a subtree it used to skip. Any scalar the server does not echo back byte-identically — an ordinary datetime normalisation is enough — then failed the match for the entire parent create op, and the greedy loop discarded that sibling and every entity nested under it. A one-entity temp-ID leak became a three-entity one, silently, with success: true. extractNestedResultsFromNode's greedy first-fit loop is replaced by a pairing pass: first pair every op that has exactly one candidate row, removing that row and looping, since consuming a row often makes another op unique; then fall back to first-fit for ops that remain ambiguous; then, if exactly one op and one row are left unpaired, pair them. The uniqueness pass is what makes elimination sound. Bare elimination on top of the greedy loop would have widened a pre-existing bug: a subset payload steals its sibling's row today and the sibling ends up unmapped, but with plain elimination the sibling would instead be mis-mapped onto the subset's row — silent cross-wiring rather than a leak. Pairing the precise payload first removes that precondition, which also repairs the existing bug. isCreateDataMatchingNode is untouched: strict comparison is still the evidence, it is just no longer the sole arbiter. The matcher must not treat "cannot identify" as "discard". Elimination is deliberately capped at one op and one row; with two simultaneously unmatchable siblings the entities keep their temp IDs rather than being guessed at, which a test pins. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GiniFfaE4gb5EuQpQ3Ncee
ChangeRegistry.getDirtyEntities() ran a full-store scan — deepEqual of data vs serverData for every snapshot, plus a reachability walk and a per-entity dirtyFields/dirtyRelations pass. usePersist feeds it to useSyncExternalStore, which React runs synchronously inside every store notification, and list reorder helpers emit one notification per reindexed item. A single delete in a large sortable list therefore cost O(N*M) full scans before React rendered. The result is now memoized per store write version. The key is deliberately NOT getVersion(): that is the subscription manager's globalVersion, bumped only inside notifying paths, and several dirtiness-changing writes do not notify — createEntity registers its root AFTER its last notification, registerParentChild un-registers a root silently, commitAllRelations and resetAllRelations never notify, and refreshServerData can skip notifying. Keying on it would serve a stale empty dirty set to the very first read, which happens synchronously inside createEntity's own notification. Instead getDirtyVersion() sums monotonic sub-store mutation counters, the pattern ReachabilityAnalyzer already uses: a new dataWriteVersion on EntitySnapshotStore plus the meta mutation/editable counters, the relation counter and the root registry counter. A sum of monotonic counters is strictly increasing on any bump, so an unchanged sum proves no dirtiness-relevant write happened, independent of whether anything was notified. getDirtyEntitiesNotInFlight() keeps filtering on every call, since in-flight state changes without any store write. This is the memoization fix only. Coalescing the notification storm and incremental dirty tracking are separate; a delete in an N-item list still fires ~N notifications, each now O(1) instead of a full scan. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GiniFfaE4gb5EuQpQ3Ncee
#65) Review follow-up to the dirty-scan memo. The memo is only as correct as its key, and `dataWriteVersion++` was hand-maintained across 11 call sites in EntitySnapshotStore. A contributor adding a mutating method — or an early-return branch to an existing one — and forgetting the bump would break nothing loudly: the store would serve a stale dirty set for the rest of the session, and the user would see a dead Save button. Writes now go through writeSnapshot/deleteSnapshot, which own the bump, the same shape HasOneStore uses for writeRelation/deleteRelation. bumpVersion is the one documented bypass: it reuses the same data/serverData refs so it cannot change dirtiness, and it runs once per ancestor on every notification, so bumping there would keep the cache permanently cold. The chokepoint makes it hard to get wrong; the new guard test is what enforces it. It classifies every name on the prototype into mutating / non-mutating / internal and fails on anything unclassified, then asserts each mutating method moves the key and each non-mutating one does not — so a new method cannot be added without a deliberate decision about its bump. Also: hasDirtyEntities() shares the memo instead of running its own full scan; getDirtyVersion() is marked @internal and documents the constraint the memo now depends on — snapshot values must be replaced, never mutated in place, since createEntitySnapshot freezes only the top level. The memoization test no longer asserts that a mid-write read sees an empty dirty set. That pinned one of the known missing-notification bugs as expected behaviour and would have handed a red test to whoever fixes it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GiniFfaE4gb5EuQpQ3Ncee
…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
Both the EntityHandle.intercept('entity:persisting', ...) interceptor
and the useOnEntityEvent('entity:persisted', ...) hook are publicly
advertised (the hook's own JSDoc literally documents the persisted
example), but never fire at runtime. The events/eventFactory.ts
createBeforeEvent / createAfterEvent switch statements have no case
for SET_PERSISTING (the action BatchPersister dispatches around the
mutation), so the EventEmitter never receives a before/after event
to fan out.
The two new tests assert the canonical advertised usage and currently
fail with `Received length: 0`. The mutation itself succeeds — the
mock store reflects the new value — proving the gap is purely in the
event factory, not in the persist pipeline.
Same root cause exists for entity:persistFailed (failure path) and
entity:deleting / entity:deleted (DELETE_ENTITY action). Adding
failure injection to MockAdapter is out of scope here; mirroring the
fix once the persisting/persisted path lands is a one-line addition
in the BatchPersister catch branch and the eventFactory switch.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
entity:persisting, entity:persisted and entity:persistFailed were declared, registrable through EntityHandle.intercept / useOnEntityEvent, documented in the hook's own JSDoc — and never fired. BatchPersister dispatches SET_PERSISTING around each persist; that flows through ActionDispatcher into events/eventFactory.ts, whose switches have no case for it, so nothing was ever emitted. Consumers registering before-save normalisation or after-save invalidation silently did nothing. BlockEditor's orphaned-reference cleanup, registered via useEntityBeforePersist, is a victim inside this repo. The events are now emitted from BatchPersister rather than the action factory. persistedId is only known after the server response, and the persist lifecycle is multi-step — the factory mapping suits simple state changes, not this. eventFactory.ts is deliberately untouched, so there is no double emission. entity:persisting runs through the interceptor pipeline before mutations are built, so a hook's store writes land in the same save. Cancellation follows the ActionDispatcher precedent — a null from an interceptor vetoes that one entity, the rest of the batch proceeds, and the vetoed entity stays dirty and retryable, counted as skipped rather than failed. markInFlight now claims the batch before the interceptor await, so the re-entrancy guard still holds across the new suspension point. EventEmitter gains hasInterceptors() so a persist with no hooks registered skips the async pipeline entirely and keeps its original synchronous timing. Delete entries in a persist batch also get persisting/persisted. That is the persist lifecycle; entity:deleting / entity:deleted remain unemitted and are a separate unit. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GiniFfaE4gb5EuQpQ3Ncee
`bun run test` enumerated the directories it covered, so 20 test files never ran in CI: the bindx-form, bindx-uploader and bindx-generator package suites plus tests/bindx-client and tests/repeater. The has-many materialisation bug fixed in #76 sat failing in packages/bindx-form/tests the whole time, behind a green board. Use an ignore pattern instead of an allow-list, so a new test directory is covered by default and cannot silently drop out again. Drop test:all, which now differs only by also running tests/browser and fails without a live playground. Gate: 1531 tests across 129 files -> 1737 across 149, 0 fail.
<Switch> called useField inside a loop over its <Case> children, behind a rules-of-hooks eslint-disable and a comment asserting a stable count that nothing enforced. SwitchProps.children is ReactNode, so a conditionally rendered <Case> type-checks and then crashes React with "Rendered more hooks than during the previous render". Separately, a cond.* DSL condition subscribed to nothing: If and Case passed null to useField whenever the condition was a Condition object rather than a bare FieldRef, while evaluateCondition kept reading the values live. Inside a memoized subtree, where the parent's re-render does not reach the child, the branch never re-evaluated. Both are the same missing capability — subscribing to a number of refs that is not known at build time. useFields(refs) takes a single useSyncExternalStore subscription over N refs, so the hook count stays constant, and <Switch> and <If> now feed it their condition fields. It widens ref to accessor through overloads exactly as useAccessor already does, so no cast is involved. The repo has no eslint config, so the rules-of-hooks disable removed here was never enforced by anything — which is how the crash survived.
HasManyListHandle cached an EntityHandle and a proxy per item id and released neither: the whole file had no delete, no clear and no dispose. A paginated, filtered or repeatedly refetched has-many therefore grew for as long as the parent handle lived. The items getter now prunes keys missing from the presented id list, before the handles are resolved, so a still-listed item is never touched and keeps its identity by construction. Two guards go with it. Pruning is skipped while the parent persists, because the presented list hides planned additions and pruning against it would evict a just-added item and re-mint it seconds later. And lookups canonicalise a temp id through its persisted id, so a rekeyed item reuses one handle instead of minting a duplicate for the same entity. A rekeyed entry is evicted rather than migrated. EntityHandle.id returns the id the handle was constructed with, so a carried-over handle would keep reporting a dead temp id while its own $fields.id.value reported the real one. itemHandleCacheRaw goes in the same pass: it was written and never read anywhere in the repo. The proxy is the reference that keeps the handle alive, so caching the raw handle separately bought nothing.
Four SnapshotStore write paths were suspected of mutating state without notifying subscribers. Investigating each settles them: - createEntity - CONFIRMED, user-visible. roots.register() runs after setEntityData and setExistsOnServer have already notified, and it is the write that makes the entity count as a create. A save indicator built on store.subscribe + getAllDirtyEntities().length renders 0 while the store holds a dirty create. - clearAllServerErrors - CONFIRMED. Its sibling clearAllErrors notifies for the same write. Both in-tree callers happen to self-heal through ordering, so this reaches users through the exported action only. - sweepUnreachableCreated - NOT a bug. Its only mutation is removeEntity, which notifies. - unregisterRootEntity - NOT a bug. Both callers sweep on the next line, and the sweep notifies. A fourth suspect, unregisterParentChild, no longer exists: ae80e75 removed it. docs/issues/032-memory-leaks.md still prescribes wiring it. The two confirmed gaps are pinned with test.failing rather than left as red reproducers, so they can live on main. Bun reports such a test as passing while the bug is present, and fails the run the moment the behaviour is fixed, forcing the marker off. The characterization tests around them record why the other two suspects are non-issues. No fix is included - this commit establishes the evidence.
The repo had no eslint config at all, while the source carried `// eslint-disable-next-line react-hooks/rules-of-hooks` comments that nothing has ever enforced. One of them sat over a `useField` call inside a loop in <Switch>, under a comment asserting a "stable count" that was false; it crashes React as soon as a <Case> is conditionally rendered. Enable exactly two rules over packages/*/src and tests: rules-of-hooks as an error, exhaustive-deps as a warning. eslint-plugin-react-hooks v7 ships 28 rules (the React Compiler set); none of the others are turned on and no style preset is added, so the signal stays readable in a repo with no lint history. No CI job is added. `bun run lint` exits 1 today: 9 rules-of-hooks sites (7 genuine hazards, 2 provably-stable false positives) and 25 phantom errors from dead `@typescript-eslint/*` disable directives naming rules no config defines. Turning the gate on has to wait for those. Flat config is named .mjs because the root package.json has no "type": "module".
All three selection-collection sites in createRelationColumn invoked the
cell renderer against a collector proxy and threw the returned JSX away.
Selections were captured only as a side effect of property accesses on
the proxy, so a declarative renderer such as
{p => <HasMany field={p.tags}>{tag => <Field field={tag.name}/>}</HasMany>}
registered `tags` but never `tag.name` — the inner callback is owned by a
nested component and is never invoked during collection. Rows came back
with their nested fields missing, which forced consumers to write cells
imperatively as `p.tags.map(t => t.name.value)` purely so the proxy would
see the accesses.
Feed the renderer's return value through collectSelection() and merge the
result into the relation's own child SelectionScope. The scope matters:
the collected fields are relative to the related entity, so merging them
into the row-level selection would attach them to the wrong entity.
The merge is additive — <HasMany>/<HasOne> getSelection already register
into the child scope as a side effect of map()/$entity, and Field returns
null during collection, so nothing is double-counted. The existing
.map()-on-proxy pattern keeps working; it is guarded by a test.
buildLeaf's relatedSelection had the same defect. It feeds
extractScalarFieldNames, so a declaratively-rendered has-many stayed in
the list as a bare scalar and was handed to the fulltext filter handler
as a searchable path — a `contains` against a relation. It now carries
its nested selection and is correctly excluded.
Known gap, left alone: buildLeaf builds its proxy without a
schemaRegistry, which is not reachable from staticRender(props). At
nesting depth >= 2 a related field named like a collector built-in
(`value`, `items`, `length`, ...) resolves to the stub and is dropped.
Top level is immune - the root proxy uses an allowlist.
Consumers hand a selection-branded accessor to a helper typed for a
different selection and bridge the gap with `as unknown as`. The
diagnosis this was meant to fix - "the __selected brand is invariant" -
is wrong. `readonly __selected?: TSelected` is a readonly optional
property, i.e. an output position, and is already covariant: widening a
full accessor to a narrower selection compiles today.
What actually fails is the opposite direction, for three reasons, and
only one of them is the brand:
1. narrow -> full is rejected by the brand AND, independently, by
EntityFieldsRef/EntityFieldsAccessor, which are keyed on
`keyof TSelected` and so are missing the properties outright. It
SHOULD be rejected: EntityHandle.fields throws UnfetchedFieldError
for any field outside the selection, so widening in place is a real
runtime bug. Loosening the brand would legalise it.
2. A free `TSelected` in a generic helper cannot be resolved at all - no
variance annotation can fix an unknowable mapped-type key set.
3. TEntityName is invariant through FieldRefMeta.entityType, so a
`string`-named accessor does not flow into a literal-named parameter.
A second, independent cast generator.
So instead of changing an existing type, add two erased views:
EntityRefLike<TEntity> = EntityRefInterface<TEntity, unknown>
EntityAccessorLike<TEntity> = EntityRefLike<TEntity> & { $data }
They erase TSelected and TEntityName, keep __entityType as the
discriminator, and deliberately omit the field proxy. Use them in
parameter positions that need entity identity and the
selection-independent API. A receiver that reads fields must still
declare the selection it needs - that cast was hiding a bug.
Nothing existing is modified. `__selected` also turned out to be
load-bearing for inference, not just checking: replacing it with
`unknown` breaks selection inference across ~30 sites.
Two SnapshotStore paths mutated observable state without telling anyone. The pins added in the parent commit reproduced both; this removes them and turns the pins into ordinary regression tests. createEntity registered the create-root AFTER setEntityData and setExistsOnServer had both already notified - and the root registration is precisely the write that makes the entity count as a create. So both notifications carried the pre-registration value and a save indicator built on store.subscribe + getAllDirtyEntities().length rendered 0 while the store held a dirty create. Fixed by reordering rather than adding a notification, so the cost stays at two notifications per create instead of three: setEntityData -> roots.register -> setExistsOnServer The order is forced from both sides. The root must come after setEntityData, because ReachabilityAnalyzer.walk() seeds a root only if the snapshot exists, so registering first would be inert and would leave a dangling root if the snapshot write threw. And it must come before setExistsOnServer, which then carries the final notification. Moving setExistsOnServer(false) last is observationally free: EntityMetaStore defaults an unknown key to false. roots.register bumps mutationVersion, which invalidates the reachability memo, so the subscriber woken by that last notification recomputes and sees the create. The undo journal is unaffected: the pre-image is captured inside setEntityData before any root write in either order, and the per-kind write guard already fuses snapshot, meta and roots into one `entity` kind recorded by that same call. clearAllServerErrors was simply silent where its sibling clearAllErrors notifies for the same kind of write. Both in-tree callers self-heal by ordering, so this only ever reached users through the exported action. Known and unchanged: both siblings clear relation errors under the key prefix but notify only the entity, so a subscribeToRelation consumer still sees a stale value after either call.
matej21
marked this pull request as ready for review
August 20, 2026 14:45
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
3 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.
Summary
$connect(id)re-points sibling subscriptionsIntegration notes
This consolidates and supersedes #29, #71, #72, #73, #74, #77, #78, #79, #80, #81, #82, #83, and #84 so the fixes can be reviewed and merged as one unit.
Nested nullable
<HasOne>and$connect(id)sibling-subscription regressions are normal passing tests. Relation notifications now update the owning entity and propagate through its live ancestors while notifying global subscribers once.Review follow-ups prevent a vetoed nested create from being included in its parent's mutation, emit
entity:persistedorentity:persistFailedfor inline creates, and subscribe lazily collected interface props before the component's first runtime render.The lint gate reports 18
exhaustive-depswarnings and zero errors. Exhaustive dependency findings remain warnings by design.Verification
bun run typecheckbun run lint— 0 errors, 18 warningsbun run test— 1,865 passed across 173 filesBrowser Tests— passedgit diff --check origin/main...HEAD