Skip to content

fix(bindx): emit the entity persist lifecycle events (#28) - #74

Closed
matej21 wants to merge 2 commits into
mainfrom
fix/entity-persist-events
Closed

fix(bindx): emit the entity persist lifecycle events (#28)#74
matej21 wants to merge 2 commits into
mainfrom
fix/entity-persist-events

Conversation

@matej21

@matej21 matej21 commented Aug 19, 2026

Copy link
Copy Markdown
Member

Fixes #28.

Problem

entity:persisting, entity:persisted and entity:persistFailed are declared in the event types, registrable via EntityHandle.intercept(...) / useOnEntityEvent(...), and documented in the hook's own JSDoc — and never fired. BatchPersister dispatches setPersisting(...) around each persist; that flows through ActionDispatcher into events/eventFactory.ts, whose createBeforeEvent / createAfterEvent switches have no case for SET_PERSISTING, so they return null and nothing is emitted.

It fails silently: registration succeeds, no warning, the side effect just never happens. There is a victim inside this repo — packages/bindx-editor/src/components/BlockEditor.tsx registers its orphaned-reference cleanup with useEntityBeforePersist(...)useInterceptEntity('entity:persisting', …), so removing a reference-backed block and saving leaves the reference row behind.

Approach

Emitted from BatchPersister, not from the action factory — option B in the issue. persistedId is only known after the server response, and the persist lifecycle is multi-step, while the factory mapping suits simple state changes. eventFactory.ts is deliberately untouched, so there is no double emission.

event where why there
entity:persisting after sortByDependencies + markInFlight, before buildMutations a before-save hook's store writes must be visible to the collector, so they land in the same save
entity:persisted / entity:persistFailed after the persist has settled listeners observe store.isPersisting === false rather than mid-flight state
entity:persistFailed (throw path) in the catch, error rethrown unchanged

entity:persisted carries the server id for creates (persistedId ?? entityId) and isNew.

Cancellation

A null from runInterceptors vetoes that one entity; the rest of the batch proceeds. This follows the existing precedent in ActionDispatcher.dispatch, where a null from an interceptor cancels that action. The vetoed entity is cleared from in-flight, never has its persisting flag set, stays dirty and retryable, and is counted in skippedCount (not failedCount) with an explanatory result. Batch success becomes false, preserving success === true ⇔ every result ok. No entity:persisted / entity:persistFailed is emitted for it.

Two structural changes worth calling out

  • markInFlight moved earlier, before the interceptor await. The hook pipeline introduces a suspension point into persistScope, so the batch must be claimed before it or the concurrent-persist re-entrancy guard no longer holds.
  • EventEmitter.hasInterceptors(...) added. runInterceptors is async, so awaiting it unconditionally inserts a microtask before the adapter call — which broke deduplication.test.ts > should skip entities already in-flight when called concurrently, a test that relies on the transaction being invoked in the same synchronous turn. Rather than weaken that test, a persist with no hooks registered now skips the pipeline entirely and keeps its original timing byte for byte.

Scope

entity:deleting / entity:deleted remain unemitted — the issue notes the same factory gap for DELETE_ENTITY, and it is a separate unit. Note that entities with changeType: 'delete' inside a persist batch do get persisting/persisted with isNew: false; that is the persist lifecycle, not the delete lifecycle.

Known gaps (not fixed here)

  • Nested inline creates get entity:persisting but no entity:persisted. An entity created inline inside a parent's mutation is in the batch, but its standalone mutation is filtered out, so it has no entry in the transaction results. Closing this means emitting from commitNestedResults / commitUnresolvedNestedEntities, and the latter has no server id at all.
  • The client-validation-error early return still runs before entity:persisting, so a normalisation hook cannot clear a client error to unblock its own save.
  • DirtyEntity.dirtyFields / dirtyRelations are captured before the hooks run and not refreshed. Field and relation data still land correctly (collectors read the store fresh), but assertNoRelationChanges uses the stale list, so a hook that dirties a relation in a collector-less setup is silently dropped instead of throwing.

Tests

The reporter's reproducer is cherry-picked with their authorship and left unmodified — it flips from failing to passing. Added: a hook whose store write lands in the same persist (the BlockEditor scenario, pinning the ordering requirement), an entity-scoped interceptor firing via the useInterceptEntity path, entity:persisted carrying the server id with isNew: true for a create, entity:persistFailed with the error, and a cancelling interceptor excluding one entity while a sibling still persists.

Verified: tests/unit/persistence + tests/cases 134 pass, tests/events.test.tsx 16 pass, full CI suite 1558 pass / 0 fail, typecheck clean.

Not verified

All tests use in-memory adapters and the sequential fallback path (MockAdapter has no persistTransaction). The BlockEditor cleanup is inferred rather than observed — the browser tests that would exercise it need a live playground.

The Browser Tests check is red for an unrelated known reason — CI installs agent-browser unpinned and the popover click behaviour changed in the 0.32.x line. The suite is 66/66 green locally on an older driver. Being fixed separately.

🤖 Generated with Claude Code

https://claude.ai/code/session_01GiniFfaE4gb5EuQpQ3Ncee

jonasnobile and others added 2 commits August 20, 2026 11:34
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
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.

entity:persisting / entity:persisted events never emitted (silent no-op)

2 participants