fix(data): record redo op for column-removal updates#140
Merged
Conversation
Core store.update mutates its values argument, deleting keys whose value
is undefined so it can reuse the object as the smaller archetype's row
data. The transactional store recorded the redo op from that same object
after the mutation, so a column-removal update (`{ comp: undefined }`)
captured an empty `{}` redo op — making redo a no-op. delete -> undo ->
redo left the column in place.
Snapshot the redo values before handing the object to core.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
krisnye
added a commit
that referenced
this pull request
Jul 13, 2026
… the delete sentinel Undoing an update that added a column (pre-image undefined) recorded the undo op's value as the internal DELETE sentinel. On replay the sentinel string was written into the column verbatim instead of removing it — the mirror of the redo-of-column-removal bug fixed in #140 (FFP-98612), and blocking FFP-98613. Root cause: recorded ops carry DELETE to mean "remove this column", but the apply path handed op values straight to store.update, which wrote the string. Fix resolves DELETE -> undefined at the single point recorded ops re-enter a store (applyOperations), on a *copy* — the core store deletes undefined keys from the object it is given, so mutating the shared op would empty it and make a second undo/redo a no-op. Rollback now reuses applyOperations (removing the duplicate applyWriteOperations), so a failed transaction that added a column also rolls back to the no-column state. Tests: undo-of-column-ADD round-trips to undefined (incl. undo -> redo -> undo), and rollback of a failed column-ADD removes the column. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
krisnye
added a commit
that referenced
this pull request
Jul 13, 2026
…141) * fix(data): exclude nonPersistent resources from store serialization Store.toData() serialized every archetype unconditionally, including archetypes holding nonPersistent resources/entities (negative-ID space). Since entityLocationTableData only ever covers the persistent location table, this leaked nonPersistent resource values into snapshots even though the corresponding entities could never be restored to point at them. Filter nonPersistent archetypes out of archetypesData so nonPersistent resources correctly stay out of serialized state. Bump version to 0.9.77. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(data): preserve archetype ids when excluding nonPersistent data from serialization The previous commit dropped nonPersistent archetypes from Store.toData() entirely. Archetype ids are dense array indices stored by index in the persistent location table, so omitting a nonPersistent archetype that precedes a persistent one shifted every later id on reload, leaving persistent entities pointing at the wrong (or a missing) archetype. Instead keep every archetype's slot to preserve ids, but serialize nonPersistent (and deprecated ephemeral) archetypes as a data-free stub of component names. fromData recreates the empty archetype at the same id and restores rows only for persistent archetypes. Adds a regression test covering the id-aliasing case. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * refactor(data): redesign nonPersistent serialization; drop the ephemeral alias Replaces the stub-based fix with a cleaner, uniform serialization model and removes the dead `ephemeral` naming entirely (breaking change). Serialization: every archetype serializes as `{ componentNames, data? }`. `data` is present only for persistent archetypes; nonPersistent (negative-ID space) archetypes serialize identity only. This preserves the dense archetype id (referenced by value in the persistent location table) without persisting session-only rows, and collapses the read/write decisions to O(1): `components.has("nonPersistent")` on write and `if (data)` on read — no component scanning. Verified end-to-end through the serialize/deserialize codec (the real bug: a nonPersistent resource previously decoded to `undefined`). Drop `ephemeral`: removed the `ephemeral` schema flag, the `ephemeral` component alias, `Entity.isEphemeral`, and every `?? schema.ephemeral` fallback. Only `nonPersistent` remains. Renamed is-ephemeral.ts → is-non-persistent.ts and updated ECS/sync docs. Existing snapshots that relied on the old format or the ephemeral flag are not compatible. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * feat(data): version the ECS snapshot format and reject incompatible loads Adds ECS_SNAPSHOT_VERSION, stamped into every toData() snapshot and checked by fromData(), which now throws on a version mismatch instead of silently mis-reconstructing. This makes the nonPersistent/ephemeral format break explicit: legacy snapshots (no version field) and any future-version snapshot are rejected with a clear error. The version guard covers the whole db/store/core fromData chain; the one internal caller that builds a payload directly (data-persistence's finalizeEntityLocationTable) stamps the current version. The incremental journal format is otherwise unaffected. Documents the break in the README. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(data): warn and skip on incompatible snapshot instead of throwing fromData now logs a console.warn and returns without loading when the snapshot version does not match (legacy/unversioned or future), keeping the freshly-constructed state. Callers such as createStoragePersistenceService treat an unloadable snapshot as "no saved data" rather than surfacing an error. Updates the regression test and README accordingly. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(data): undo of a column-ADD removes the column instead of writing the delete sentinel Undoing an update that added a column (pre-image undefined) recorded the undo op's value as the internal DELETE sentinel. On replay the sentinel string was written into the column verbatim instead of removing it — the mirror of the redo-of-column-removal bug fixed in #140 (FFP-98612), and blocking FFP-98613. Root cause: recorded ops carry DELETE to mean "remove this column", but the apply path handed op values straight to store.update, which wrote the string. Fix resolves DELETE -> undefined at the single point recorded ops re-enter a store (applyOperations), on a *copy* — the core store deletes undefined keys from the object it is given, so mutating the shared op would empty it and make a second undo/redo a no-op. Rollback now reuses applyOperations (removing the duplicate applyWriteOperations), so a failed transaction that added a column also rolls back to the no-column state. Tests: undo-of-column-ADD round-trips to undefined (incl. undo -> redo -> undo), and rollback of a failed column-ADD removes the column. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
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
1 participant
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.
Description
When a component is removed with
t.update(entity, { comp: undefined })(the shape used by the orphan-delete / sever-parent-link model), the recorded redo operation was empty, so redo became a no-op. A delete → undo → redo cycle left the column in place — the item reappeared instead of being re-deleted. UNDO was correct; only REDO was broken.Root cause
Core
store.update(store/core/create-core.ts) mutates itsvaluesargument in place: for any key whose value isundefinedit records a component removal anddeletes the key, so it can reuse the same object as the new (smaller) archetype's row data.The transactional store passed that same object reference to
store.updateand then recorded the redo op from it after the mutation (create-transactional-store.ts). For a column-removal update the object was now{}, so the redo op capturedupdate(entity, {})— a no-op.Fix
Snapshot the redo values in the transactional store before handing the object to core.
replacedValues(undo) was already computed pre-mutation, so undo was unaffected.Tests
Added a red/green regression test in
create-transactional-store.test.ts(column removal via update-with-undefined):undefinedvalue (not{})Verified red before the fix, green after. Full
packages/dataECS suite (1075 tests) and monorepo typecheck/lint pass.🤖 Generated with Claude Code