fix(bindx): announce the writes that made the store go stale - #84
Closed
matej21 wants to merge 1 commit into
Closed
Conversation
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.
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.
Two
SnapshotStorewrite paths mutated observable state without notifying. #80 reproduced both withtest.failingpins; this fixes them, which turns those pins red (this test is marked as failing but it passed) and so also un-marks them into ordinary regression tests.createEntity— the user-visible oneThe root registration is the write that makes the entity count as a
create— the comment directly above it says so. Both notifications therefore carried the pre-registration value.Observable effect: mount a plain
<Entity create>and a save indicator built onstore.subscribe+getAllDirtyEntities().length— the shapeusePersistuses — renders0, whilegetAllDirtyEntities()returns onecreate.Reordered, not re-notified
Adding a notification after
roots.registerwould have worked too, and was rejected: it makesuseEntityListadding N drafts cost 3N notifications instead of 2N. The reorder costs nothing.The position is forced from both sides:
setEntityData—ReachabilityAnalyzer.walk()seeds a root onlyif (entitySnapshots.has(key)), so registering first would be inert, and would leave a dangling root if the snapshot write threw.setExistsOnServer, which then carries the final notification. Moving it last is observationally free:EntityMetaStore.existsOnServerdefaults an unknown key tofalse, so for a fresh entity that call is bookkeeping.roots.registerbumpsmutationVersion, invalidating the reachability memo, so the subscriber woken by that last notification recomputes and sees the create.Undo journal is unaffected: the pre-image is captured inside
setEntityData(journal?.recordEntity(key)) before any root write in either order, soexportEntityCell'sisRoot/present:falsecapture is identical. The per-kind write guard fuses snapshot + meta + roots into oneentitykind, already recorded by that same call.clearAllServerErrorsSilent, where its sibling
clearAllErrorsnotifies for the same kind of write, andgetVersion()did not move either — so no consumer, entity or global, could observe the clear.Both in-tree callers self-heal by accident of ordering (
BatchPersisterdispatches a notifyingsetPersisting(true)right before;mapServerErrorsfollows every clear with a notifyingaddFieldError), so this only ever reached users through the exported action. Their re-render count goes from 1 to 2 per persist attempt — not a per-keystroke path.Not fixed, recorded
Both siblings clear relation errors under the key prefix but notify only the entity, so a
subscribeToRelationconsumer still sees a stale value after either call. Fixing that asymmetry would change the notification counts these tests now pin, so it is a separate decision.The two other suspects from #80 stay untouched and their characterization tests are unchanged:
sweepUnreachableCreateddelegates to a notifyingremoveEntity, andunregisterRootEntity's callers sweep on the next line. Neither is a bug.Gates
bun test tests/unit/store/ tests/react/storeNotifications/— 194 pass / 0 failbun run typecheck— exit 0bun test --path-ignore-patterns='**/tests/browser/**'— 1751 pass / 0 fail / 154 files, identical totals before and after (the delta is only the 5 pins flipping red→green)Negative control: with the store fix reverse-applied and the markers still off, exactly the 5 un-marked tests fail. The two control and two characterization tests pass in both directions.
Comment headers in the four files were rewritten from "known-broken pin" to regression notes — they described
test.failingsemantics that no longer apply. No assertion, test name orexpectwas touched; verified by diffing non-comment lines and by the unchangedexpect()count (1959).