Skip to content

test(bindx): pin the store's silent-write notification gaps - #80

Closed
matej21 wants to merge 1 commit into
mainfrom
test/store-notification-pins
Closed

test(bindx): pin the store's silent-write notification gaps#80
matej21 wants to merge 1 commit into
mainfrom
test/store-notification-pins

Conversation

@matej21

@matej21 matej21 commented Aug 20, 2026

Copy link
Copy Markdown
Member

Investigation only — no fix is included. Four SnapshotStore write paths were suspected of mutating state without notifying subscribers. This settles all four, two of them as non-issues.

method verdict
createEntity CONFIRMED — user-visible
clearAllServerErrors CONFIRMED — public action path only
sweepUnreachableCreated not a bug
unregisterRootEntity not a bug
unregisterParentChild does not exist

createEntity — the real one

this.setEntityData(entityType, id, data, false)   // notifies
this.setExistsOnServer(entityType, id, false)     // notifies
this.roots.register(this.getEntityKey(entityType, id))   // silent

The root registration is the write that makes the entity count as a create — the comment directly above it says so. Both notifications therefore carry the pre-registration value.

A save indicator built on store.subscribe + getAllDirtyEntities().length — the shape usePersist uses — renders 0 after mounting a plain <Entity create>, while store.getAllDirtyEntities() returns [{ changeType: 'create' }]. A bare store.notify() flips the DOM to 1, confirming it is purely a missing notification.

This method was originally written off as internal bookkeeping. It turned out to be the actual bug of the batch.

clearAllServerErrors

Its sibling clearAllErrors notifies for the identical write; this one does not, and store.getVersion() does not move either, so no consumer — entity, relation or global — can observe the clear.

Impact is narrower than it first looks, and the tests record why: both in-tree callers self-heal by accident of ordering. BatchPersister dispatches a notifying setPersisting(true) immediately before the clear, and mapServerErrors follows every clear with a notifying addFieldError. So this reaches users through the exported action only.

The two non-issues

  • sweepUnreachableCreated has no notify of its own, but its only mutation is removeEntity, whose last statement notifies entity subscribers, live parents and global subscribers. Sweeping nothing changes nothing.
  • unregisterRootEntity is silent, but both callers (Entity.tsx:206-207, useEntityList.ts:303-308) sweep on the very next line, and the set of entities whose observable state un-rooting changes is exactly what the sweep removes and notifies for. Un-rooting a persisted entity is inert anyway — ReachabilityAnalyzer.walk() seeds every existsOnServer entity as a root regardless of RootRegistry.

Recording these matters as much as the confirmations: both were on the suspect list, and neither needs a change.

Why test.failing instead of red reproducers

The two confirmed gaps are pinned with test.failing, which gives the xfail contract in both directions on bun 1.3.14:

  • bug present → the test passes, so the suite stays green and the pin can live on main
  • bug fixed → the run fails with this test is marked as failing but it passed. Remove .failing if tested behavior now works

That matters because #77 widens CI from an enumerated directory list to everything outside tests/browser. A permanently-red file would not be mergeable under that gate; a pin is, and it turns red by itself the day someone fixes the underlying bug.

Assertions are byte-identical to the versions that failed — only test(test.failing( and the titles changed.

Gates

bun test tests/unit/store/ 190 pass / 0 fail · bun test tests/react/storeNotifications/ 4 pass / 0 fail · typecheck clean.

Follow-ups this turned up

  1. SnapshotStore.createEntity — announce the root registration (notify after it, or register before setExistsOnServer so the last notification carries the finished value). This is a real user-facing bug and is worth its own PR.
  2. SnapshotStore.clearAllServerErrors — one line, mirroring clearAllErrors.
  3. registerParentChild has the same silent-tail-write shape (roots.unregister with no notify, and connect/add paths notify before calling it). Worth an audit.
  4. docs/issues/032-memory-leaks.md still prescribes wiring unregisterParentChild, which no longer exists.
  5. useEntityErrors is not re-exported from bindx-react's root, so apps cannot import it, and its getSnapshot allocates a fresh object per call — the React 18 "getSnapshot should be cached" hazard.

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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant