Skip to content

fix: close undo ledger holes for axis deletes and glyph renames#121

Merged
kostyafarber merged 3 commits into
mainfrom
fix/undo-ledger-holes
Jul 2, 2026
Merged

fix: close undo ledger holes for axis deletes and glyph renames#121
kostyafarber merged 3 commits into
mainfrom
fix/undo-ledger-holes

Conversation

@kostyafarber

Copy link
Copy Markdown
Collaborator

Closes the highest-severity in-session data-loss bugs from the 2026-07 review: DeleteAxis and UpdateGlyph had no ledger steps — undo popped the entry, replayed nothing, and reported success, leaving the axis/old identity unrecoverable and consuming the next undo target. Failed replays also permanently dropped ledger entries.

  • AxisDeleted gains a ledger step carrying the full axis and per-source location valuesFont::remove_axis strips the axis's value from every source location (with SQLite cascading), so the axis alone was insufficient pre-state; undo restores locations FK-safely in the same transaction.
  • GlyphIdentityChanged gains a step (pre/post name + unicodes derived from the change record).
  • The _ => {} wildcard in ledger_steps is gone — the ten deliberately step-less variants are listed explicitly and documented, so any future FontChange variant is a compile error until its undo story is decided.
  • Failed replays hand the entry back (new Ledger::restore_undo/restore_redo), matching the doc comment's promised semantics; the font is untouched on failure, so retry is safe. Both paths have real injection tests (wipe store rows → replay fails → entry restored → repair → retry succeeds).
  • New Font::source_mut accessor in shift-font (mirrors layer_mut) — needed to restore stripped locations; flagged for review as an IR API addition.

Tests: full apply→undo→assert-pre→redo→assert-post matrix over all 20 FontIntent kinds against real SQLite (shift-workspace 24 → 44 tests), plus previously-untested MAX_ENTRIES trimming.

cargo test --workspace: 319 passed, 0 failed. clippy zero warnings.

🤖 Generated with Claude Code

ledger_steps had no arm for AxisDeleted or GlyphIdentityChanged, so
undoing a deleteAxis or updateGlyph popped the entry, replayed nothing,
and echoed success — the axis definition / old glyph identity was
unrecoverable and the next undo target was consumed.

- capture the full Axis (and its per-source location values, which
  Font::remove_axis strips) before a DeleteAxis applies
- derive the GlyphIdentity pre/post pair from the change record itself
- replace the ledger_steps wildcard with an explicit variant list so a
  new FontChange kind forces an undo decision at compile time
- add Font::source_mut so replay can restore stripped source locations
undo/redo popped the entry before replaying and dropped it on error, so
a transient store failure silently erased that history step. On a failed
replay the entry is now re-pushed onto the stack it came from and the
error propagates, matching the hand-back semantics the Ledger doc
comment promised.
Per-intent matrix: apply, then assert undo restores the exact pre state
and redo the exact post state (layer snapshots compare by equality).
Also covers failed-replay hand-back for undo and redo via a wiped store,
and MAX_ENTRIES trimming of the oldest ledger entry.
@kostyafarber kostyafarber merged commit 88d4ab6 into main Jul 2, 2026
10 checks passed
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