Skip to content

refactor(bindx): decompose store internals (memoized reachability → RelationStore split)#49

Open
matej21 wants to merge 19 commits into
fix/hasmany-remove-created-entity-leaks-orphan-createfrom
refactor/relationstore-decomposition
Open

refactor(bindx): decompose store internals (memoized reachability → RelationStore split)#49
matej21 wants to merge 19 commits into
fix/hasmany-remove-created-entity-leaks-orphan-createfrom
refactor/relationstore-decomposition

Conversation

@matej21

@matej21 matej21 commented Jun 23, 2026

Copy link
Copy Markdown
Member

Stacked on #48 (fix/hasmany-remove-created-entity-leaks-orphan-create). Base is that branch, not main — review/merge #48 first.

Decomposes the bindx store internals in 13 reviewable, test-paired steps. Each refactor/perf/feat commit is preceded or followed by a test commit that pins the behavior it touches.

What changes

  • Memoized reachability — computeReachableCreated() (run on every dirty check, ≈ keystroke) is cached behind a sum of monotonic sub-store mutation counters; pure field edits keep the cache warm.
  • RekeyOrchestrator — temp-id → server-id rekey centralized into one ordered component.
  • Pessimistic presentation primitive — handle display reads route through getPresentationSnapshot, removing the need to mutate the store to the server view during a pessimistic persist.
  • Delete pessimistic mutate-restore dance — the store is never reset to the server view; the server baseline is presented instead. On failure, edits/creates stay intact for a retry by construction.
  • Has-one model — RelationStore made authoritative for has-one; parent notification derived from relation edges (childToParents removed); has-many planned-additions collapsed into one map.
  • RelationStore decomposed into a composed HasOneStore + HasManyStore.

Rebase note

Rebased onto the current tip of the base branch (dfc1966, "address review findings"). One semantic conflict was resolved by hand in BatchPersister.ts: dfc1966's pessimistic partial-failure fix touched the same capture/restore machinery this stack deletes. Resolution — the dance is removed (per this stack's intent), while dfc1966's let result + success-gated sweepUnreachableCreated() are preserved (the conservative PERSIST-1 fix). The reachability memoization test was adapted to dfc1966's new no-created-snapshot early-out fast path (invalidation proven by the result, matching the sibling root-(un)register test).

Tests

Full suite green except:

  • 9 browser/playground tests — require the live playground on :15180 (environmental, not run in this checkout);
  • 1 pre-existing FormHasManyRelationScope > tracks dirty state from has-many relation failure that also fails on the pre-rebase tip.

Neither is introduced by this stack. Reachability + persistence (the rebase-affected areas) are fully green.

🤖 Generated with Claude Code

https://claude.ai/code/session_01PkeRcE9offiYmcy3c7fiuj

matej21 and others added 19 commits June 23, 2026 10:59
@matej21 @claude
perf(bindx): memoize reachability walk via sub-store mutation counters
b433c80 
computeReachableCreated() is an O(E+R) walk run on every dirty check and
every post-persist sweep, previously recomputed from scratch each time.

Each sub-store the walk reads (entity snapshots, meta, relations, roots)
now exposes a monotonic getMutationVersion() bumped only when
graph-relevant state changes — the entity key set / id index,
existsOnServer / isPersisting, the root set, and relation edges. Pure
value edits (setFieldValue/updateFields/...) and the per-render no-op
getOrCreate* calls do not bump any counter, so the hot path stays warm.

ReachabilityAnalyzer caches its result keyed on the sum of those counters.
The sum is strictly increasing, so an unchanged sum proves nothing
relevant changed and the cached set is returned without re-walking.

All RelationStore map writes are routed through writeRelation/writeHasMany
helpers so the bump cannot be missed; EntitySnapshotStore and
EntityMetaStore bump selectively to avoid invalidating on value-only edits;
RootRegistry bumps only on an actual change so the per-render
registerParentChild -> unregister no-op does not thrash the cache.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PkeRcE9offiYmcy3c7fiuj
@matej21 @claude
test(bindx): cover reachability memoization cache hit/invalidation
80016d4 
White-box tests drive ReachabilityAnalyzer directly and spy on
getLiveChildIds to assert the cache hits when nothing changes (incl.
across pure field edits) and misses on each graph-affecting mutation
type. A black-box test proves SnapshotStore propagates counter bumps
end-to-end through getAllDirtyEntities (no stale create set).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PkeRcE9offiYmcy3c7fiuj
@matej21 @claude
refactor(bindx): centralize temp-id rekey in RekeyOrchestrator
f3a1939 
The temp→persisted rekey was a 9-store fan-out hand-written inline in
SnapshotStore.mapTempIdToPersistedId, and the same temp→persisted fact
was stored twice in two formats: SnapshotStore.rekeyedEntities
("Type:tempId" → "Type:persistedId") and EntityMetaStore.tempToPersistedId
("Type:key" → persistedId).

Introduce RekeyOrchestrator as the single owner of temp→persisted
identity. Both key resolution (resolveKey/resolveId) and persisted-id
queries (getPersistedId/isNewEntity) now derive from its one map, so the
two duplicate maps are gone. EntityMetaStore sheds tempToPersistedId and
its getPersistedId/isNewEntity/mapTempIdToPersistedId API; the
exists-on-server flip folds into its rekey().

Each participating sub-store implements a uniform Rekeyable.rekey(ctx)
interface, and the orchestrator drives them in one explicit, documented
order (previously load-bearing but implicit in the inline sequence).
SubscriptionManager keeps its own closure-redirect chain, which tracks
relation-key prefixes and stale unsubscribe closures rather than entity
identity. clear() now also clears the redirect map (previously leaked).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PkeRcE9offiYmcy3c7fiuj
@matej21 @claude
test(bindx): cover RekeyOrchestrator resolution and ordering contract
010c7b7 
Pins resolveKey/resolveId/getPersistedId/isNewEntity across placeholder,
temp, and persisted ids; asserts rekey visits every participant exactly
once in order with a fully-derived context; and an end-to-end check that
SnapshotStore resolves a created entity after persist via the orchestrator.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PkeRcE9offiYmcy3c7fiuj
@matej21 @claude
feat(bindx): add pessimistic presentation primitive (inert)
ea009e0 
Introduces the read primitive that will let handles present the server
baseline during a pessimistic persist WITHOUT the current mutate-store-
to-server-then-restore dance. Inert for now: no consumer reads it yet
(wiring + removal of mutate-restore lands in the next PR).

- EntityMetaStore tracks a pessimisticInFlight set (a subset of the
  persisting set), set/cleared in the same setPersisting transition so the
  two cannot drift; it does not bump the reachability mutation counter
  since the flag drives presentation only.
- SnapshotStore.getPresentationSnapshot returns the canonical snapshot
  except while pessimistically in-flight, when it returns a frozen
  server-baseline view (data === serverData) built without mutating the
  store. Non-pessimistic entities are returned verbatim, so optimistic and
  not-persisting share one path.
- The SET_PERSISTING action carries an optional pessimistic flag;
  BatchPersister sets it when updateMode is pessimistic.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PkeRcE9offiYmcy3c7fiuj
@matej21 @claude
test(bindx): cover the pessimistic presentation primitive
1fe2a66 
Asserts presentation equals canonical without the flag, equals the server
baseline while pessimistically in-flight (canonical staying dirty, still
reported as an update), tracks optimistic vs pessimistic, restores on
clear, and that the baseline view is frozen and not aliased.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PkeRcE9offiYmcy3c7fiuj
@matej21 @claude
refactor(bindx): route handle display reads through presentation
241eb4a 
Field and entity value display now read getPresentationSnapshot, so a
pessimistically in-flight entity shows the server baseline. Dirty tracking
keeps reading the canonical snapshot, so a field stays correctly dirty
while its display shows the server value — the crucial split that lets the
next commit delete the mutate-store-to-server-then-restore dance.

- BaseHandle: getEntityData stays canonical (also used by has-many
  materialization); new getPresentationData for display.
- FieldHandle.value -> presentation; isDirty compares canonical vs server.
- EntityHandle.data -> presentation; getSnapshot/isDirty/serverData stay
  canonical.

Scope note: relation display (has-one/has-many) still presents canonical
state during a pessimistic in-flight window; presenting relations at the
server baseline is a follow-up, cleaner once the has-one snapshot fallback
is removed (keystone PR). Final post-persist states are unaffected.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PkeRcE9offiYmcy3c7fiuj
@matej21 @claude
refactor(bindx): delete pessimistic mutate-restore dance
311d56c 
With handle display reads now routed through getPresentationSnapshot, the
store no longer needs to be reset to the server view during a pessimistic
persist and restored afterward. Remove the whole capture/reset/restore
machinery:

- drop captureEntityStates, restoreEntityState and the CapturedEntityState
  type, and the per-update resetEntity + resetAllRelations block;
- success now commits the (still-dirty) canonical state as the new server
  baseline via the same path optimistic mode always used;
- failure leaves the entity dirty with no action — edits and created
  entities survive for a retry (P2) by construction, since the store was
  never mutated;
- remove the now-dead store methods getAllRelationsForEntity,
  getAllHasManyForEntity and restoreHasManyState.

The full pessimistic suite (commit-on-success, P2 preserve-edits and
preserve-create on failure, isPersisting timing, batch) passes unchanged.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PkeRcE9offiYmcy3c7fiuj
@matej21 @claude
test(bindx): pin parent-notification behavior before childToParents r…
feb12f6 
…ework

Subscriber call-count harness covering has-one/has-many parent re-render on
child change, the diamond (shared child notifies both parents), rekey
preserving subscriptions, and the known append-only childToParents leak
(disconnect still notifies the former parent) — the regression oracle for
the upcoming reverse-index notification rework.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PkeRcE9offiYmcy3c7fiuj
@matej21 @claude
refactor(bindx): make RelationStore authoritative for has-one
53f23e2 
Has-one relation state had two sources of truth: the RelationStore entry
AND the related object embedded in the parent's snapshot data. HasOneHandle
read from the store when an entry existed, else fell back to the embedded
snapshot — invisible to reachability-based create detection, which reads
membership exclusively from RelationStore.

Mirror the has-many path (HasManyListHandle.materializeEmbeddedItems): add
an idempotent ensureEntry() that materializes the relation entry from the
parent's embedded data on every relation-state read (relatedId/state/isDirty),
then drop the snapshot fallback so RelationStore is the single source of truth.

- Initial materialization uses the non-notifying getOrCreateRelation, deriving
  the server baseline from the parent's serverData so a freshly loaded relation
  is not dirty (currentId === serverId, state === serverState).
- A parent re-fetch (embedded reference changed) advances the server baseline
  only when the relation is not locally dirty, via hasEmbeddedDataChanged —
  child-snapshot propagation keeps sole ownership of the propagation slot in
  ensureRelatedEntitySnapshot, so the two paths never double-consume it.
- Local connect/disconnect, placeholders, and creating entries are never
  clobbered (detected as locally-dirty / id-mismatch).

A created child connected via a has-one now appears in reachability and
getAllDirtyEntities() as a create with no explicit setRelation, with no
analyzer change. Tests cover loaded-not-dirty materialization, the dropped
fallback, and the created-child-as-create case.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PkeRcE9offiYmcy3c7fiuj
@matej21 @claude
refactor(bindx): derive parent notification from relation edges; remo…
6ec3260 
…ve childToParents

Parent re-render propagation used a separate append-only childToParents registry
in SubscriptionManager, populated on every render via registerParentChild and
never cleaned on disconnect (a leak). RelationStore is now the single source of
truth for relation membership, so derive 'which parents reference this child'
from its live edges instead.

- Add RelationStore.getParentKeysForChild(childId): an on-demand scan of the
  live has-one/has-many edges, with liveness matching getLiveChildIds exactly.
  Chosen over an incremental reverse index: correct by construction (reads the
  same maps as the forward query), so nothing can drift on disconnect/rekey/clear.
- Inject it into SubscriptionManager via a small ParentKeyLookup interface,
  mirroring SnapshotVersionBumper. notifyEntitySubscribers derives parents from
  it, preserving the recursion + cycle guard and the parent snapshot bump.
- Remove childToParents, registerParentChild/unregisterParentChild bodies, and
  their migration in rekey(). SnapshotStore.registerParentChild keeps only the
  load-bearing roots.unregister(childKey) side effect; unregisterParentChild is
  deleted. Render-time callers (handles, ActionDispatcher) are unchanged and now
  trigger only the root-unregister half — the relation edge they already set up
  is the notification source.

A disconnected child no longer notifies its former parent (the leak is gone).

Tests: flip the harness disconnect scenario to the no-leak behavior; update the
notification harness diamond and the rekey/snapshotStore/actionDispatcher tests
to establish real relation edges (the new notification source) instead of bare
registerParentChild calls. Add getParentKeysForChild.test.ts with a randomized
cross-check proving the reverse query always agrees with getLiveChildIds.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PkeRcE9offiYmcy3c7fiuj
@matej21 @claude
refactor(bindx): collapse has-many planned-addition sets into one map
b205841 
Replace StoredHasManyState's plannedConnections: Set and
createdEntities: Set with a single plannedAdditions: Map<string,
'created' | 'connected'>. The map keys are exactly the old
plannedConnections; keys whose value is 'created' are exactly the old
createdEntities, making the "createdEntities ⊆ plannedConnections"
invariant structural and removing one mutable field (5→4).

add() records 'created', connect()/embedded-connect record 'connected'
without downgrading an existing 'created' entry. removeFromHasMany
branches on get(id) === 'created'. getLiveChildIds,
getParentKeysForChild, dirty tracking, commit, rekey, export/import and
computeDefaultOrderedIds updated to the new field. SnapshotStore
accessors keep their Set/boolean return types so external callers are
unaffected; MutationCollector emits create vs connect from the kind.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PkeRcE9offiYmcy3c7fiuj
@matej21 @claude
refactor(bindx): decompose RelationStore into composed HasOneStore + …
b667a18 
…HasManyStore

Split the ~900-line RelationStore into two focused collaborator classes behind
an unchanged public API:

- HasOneStore owns has-one relation state and its own mutation-version counter.
- HasManyStore owns has-many list state, computeDefaultOrderedIds, and its own
  mutation-version counter.
- relationKey.ts holds the shared parentKeyFromRelationKey helper.
- RelationStore is now a thin facade composing both: it sums the two mutation
  versions, unions getLiveChildIds/getParentKeysForChild/getDirtyRelations, and
  fans rekey/replaceEntityId/removeOwnedRelations/commit/reset/clear out to both
  sub-stores.

Public API and all import paths (StoredRelationState, StoredHasManyState,
HasManyRemovalType, HasManyAdditionKind, computeDefaultOrderedIds re-exported
from RelationStore.js) are preserved; consumers see no change. Behavior is
identical — pure structural move.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PkeRcE9offiYmcy3c7fiuj
@matej21 @claude
fix(bindx): make has-one refetch baseline advance non-notifying
d1b837a 
ensureEntry()/advanceServerBaselineOnRefetch runs on every has-one read,
including during React render. Its refetch branch wrote the new server
baseline via the notifying setRelation, calling subscribers mid-render and
violating the external-store contract. Add a skipNotify path to
SnapshotStore.setRelation and use it here — the parent re-fetch that produced
the new embedded reference already notified subscribers.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PkeRcE9offiYmcy3c7fiuj
@matej21 @claude
fix(bindx): guard connectExistingToHasMany against duplicate ordered id
3827de0 
connectExistingToHasMany appended to orderedIds unconditionally, so
re-materializing the same embedded connect reference surfaced the list item
twice. Mirror planHasManyConnection: only touch an explicit order and guard
with !includes. Also pin the load-bearing no-downgrade invariant (a connect
after a create on the same id must keep emitting a create) on both the
planHasManyConnection and connectExistingToHasMany paths.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PkeRcE9offiYmcy3c7fiuj
@matej21 @claude
test(bindx): pin reachability cache invalidation for has-one edges
74c501b 
The memoization suite covered only has-many edge changes. Add has-one
setRelation connect/disconnect cases and a direct getMutationVersion test
proving the facade counter sums both sub-stores — so dropping the has-one
term (a stale-cache create-detection bug) would be caught.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PkeRcE9offiYmcy3c7fiuj
@matej21 @claude
docs(bindx): document bare-id contract of getParentKeysForChild
544ba20 
The reverse parent lookup matches on a bare child id, deliberately relying on
the store-wide global-id-uniqueness invariant (the same one behind
EntitySnapshotStore idIndex/keyForId and the forward reachability walk).
Document this on getParentKeysForChild and getParentKeys, and pin it with a
test so a future type-aware change trips deliberately.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PkeRcE9offiYmcy3c7fiuj
@matej21 @claude
perf(bindx): back relation queries with a bidirectional edge index
8ec7582 
getParentKeysForChild and getLiveChildIds scanned every relation entry, so
parent-notification and the reachability walk were O(total relations) per call
— a per-edit cost that scaled with store size (benchmarked ~550 µs/edit at
4000 rows vs ~3 µs before the childToParents map was removed).

Introduce RelationEdgeIndex: a bidirectional, reference-counted index of live
parent<->child edges that each sub-store maintains through its single
write/delete chokepoint by diffing the previous against the next live set. Both
queries become O(degree) and the two directions stay consistent by construction
(the failure mode of the old append-only childToParents map). Liveness is now a
single predicate per sub-store, consumed by the chokepoint, instead of being
duplicated across the forward and reverse scans.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PkeRcE9offiYmcy3c7fiuj
@matej21 @claude
test(bindx): cover the relation edge index
0c662bb 
Pin the reference-counting and migration cases the bidirectional index
introduces — same parent reaching a child through several fields, id
replacement, owner rekey, bulk removal, commit/reset — alongside the existing
randomized forward/reverse cross-check.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PkeRcE9offiYmcy3c7fiuj
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@matej21