fix(journal): redo that stays cleared — a new operation clears its story's waiting rows for good, Ctrl+Y replays nothing past a revert to an older version, and the journal records how each row was undone #56
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "fix/redo-discarded-rows"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Summary
Two redo bugs remained after #44. Both come from working redo out of journal position alone: "R undone, then S written, then S undone" and "R and S written, S undone, then R undone" leave the same undone rows in the same order, so no positional rule can tell them apart (#44's attempt was blocked for that reason). The journal now records, per undone row, what it waits for.
ChangeLog.UndoState(migration20260926134739_JournalUndoState: a nullable INTEGER plus an index on(StoryId, UndoState), no table rebuild):Rewound(1): undone with no active row left above it — Ctrl+Z, Revert to here, a run revert with nothing after it.CutOut(2): undone from under a later active row — a run revert, a run redo's forced discard.Cleared(3): a new operation was saved while the row was undone.EfUndoStoremarks each undone row against its story's newest row still active.ChangeLogEntry.MarkUndone(bool cutOut = false): an undo that does not say is markedRewound, the side where Ctrl+Y refuses.TsumugiDbContext): per story, every undone row from the lowestRewound/CutOutrow up is markedCleared. The SQL is oneUPDATEwhose bound is a scalar subquery; SQLite plans it asSEARCH USING INDEX IX_ChangeLog_StoryId_Id (StoryId=? AND Id>?)withSEARCH USING COVERING INDEX IX_ChangeLog_StoryId_UndoStatefor the bound, so a save with nothing waiting costs two index probes.RedoTyping(Ctrl+Y) takes the newest active row over the whole story (newIChangeJournal.LatestActivePositionAsync, a MAX), skips cleared rows, refuses while aRewoundrow waits at or below the newest snapshot, and steps over aCutOutone there — so ADR 0022's PR #25 rule holds (Ctrl+Y re-applies a forced revert's field change once Ctrl+Z has made it whole).RedoRevert(History's Redo, and soGetStoryHistory.CanRedo) skips cleared rows.RedoRunis unchanged: it stays run-scoped and still brings a cleared run back under its conflict check.DbUpdateExceptionre-creating it in the deleted chapter (found in review, below).Everything touched is inside the arc's paths:
Domain/Journal,UseCases/Journal,Infrastructure/Journal,TsumugiDbContext,ChangeLogEntryConfiguration, one new migration plus the model snapshot, ADR 0022, and the journal tests. No seed change: the arc adds no user-authored content. No other migration landed on main since the branch; the branch is rebased onb0e8bff(#52, the manuscript hardening, which touches none of these files).Verification
b0e8bff, modules rebuilt): Integration 530/530, BlazorAdapter 415/415, Domain 486/486, UseCases 236/236. The journal-related filter (HistoryAndRevert, FieldJournal, Journal, every migration test, ImageField, AppSettings, JournalCompatibility — 97 tests) passed 10 runs in a row; the full Integration suite passed 3 runs; the adapter's History, ChapterPage and ManuscriptEffects tests (70) pass. The Kagaku.Flow suites were not run: nothing here touches them.HistoryAndRevertTests: both repro sequences —Ctrl_Z_of_a_new_edit_brings_back_that_edit_and_never_the_split_the_edit_cleared(Ctrl+Y, with a neighbour story whose own undone rename waits below) andCtrl_Z_of_the_edit_that_cleared_a_revert_leaves_History_s_Redo_that_edit_alone; bug 2's two-snapshot case,Revert_to_here_on_an_older_snapshot_leaves_Ctrl_Y_nothing_and_History_s_Redo_the_whole_revert(×2: with and without a Ctrl+Z first, another story written between);Revert_to_the_version_before_a_run_leaves_Ctrl_Y_nothing_to_replay_onto_the_story_without_it;A_new_edit_clears_a_run_revert_from_History_s_Redo_and_Redo_run_still_brings_the_run_back;A_forced_run_revert_of_the_newest_edit_leaves_Ctrl_Y_nothing_until_the_run_is_back(×2: History's Redo (2,2), or Redo run then Ctrl+Y (1,1)). The #44 round trip stays pinned by the existingA_version_saved_over_an_undone_edit_leaves_Ctrl_Y_the_edits_made_after_it.FieldJournalTests.A_new_operation_clears_the_row_the_forced_revert_discarded_and_redo_brings_back_only_what_Ctrl_Z_took(×2); the PR #25 rule stays pinned by the existingOnce_Ctrl_Z_has_made_the_field_change_whole_Ctrl_Y_re_applies_all_of_it_and_the_run_stays_reverted.JournalTests.A_new_operation_clears_the_storys_waiting_rows_in_its_own_save_synchronous_or_not_and_a_failed_save_clears_nothing.JournalUndoStateMigrationTests(new; the world written through the use cases, walked down toSegmentTextMarkupand up): the column comes in empty and a walk down gives the old build itsChangeLogbyte for byte; Up adds a column and an index without rebuilding a table; a pre-upgrade row with nothing waiting below it redoes by position and a new operation leaves it alone, while a neighbour story's waiting row below it stays the neighbour's; a version saved over a pre-upgrade undone row leaves Ctrl+Y the edits made after it; a pre-upgrade row above one a new operation clears is cleared with it (×2: Ctrl+Y and History's Redo give (1,1), the old paragraph stays gone, both redos then refuse). The stored numbers are read raw (1 Rewound, 3 Cleared).ChangeLogEntryTests(Domain, 4): a new row waits for nothing; an undone row is rewound unless cut out; a redone row waits for nothing again; the three states keep the numbers the journal stores (1, 2, 3).TsumugiDbContext.Waiting): HEAD's rule restored, old rows never cleared → the FK repro Theory (both cases);>=bottom →>→ 8 tests incl. the split test; no bottom (every undone row cleared) → the nothing-waiting-below migration test, the version-over migration test andCategoryFieldsMigrationTests' byte-for-byte walk;IsUndonedropped (active rows marked) → the FK repro Theory's own-row assertion; the bound's story filter dropped → the nothing-waiting-below test's neighbour; the outer story filter dropped → the split test's neighbour; the bound only Rewound → the run-revert and FieldJournal History tests; only CutOut → 6 tests; the sync clear removed → the JournalTests sync save; the async clear removed → 8 tests.RedoTyping: a pre-upgrade row below the snapshot blocking Ctrl+Y as a rewound one does (the review's R4) → the new version-over migration test; the below-snapshot check disabled → 5 tests incl. the new forced-revert Theory.UndoState:Rewound = 1renumbered to 4 (the review's E1) → the Domain numbers test and 3 migration tests.MarkUndone's default flipped to CutOut → the Domain test and 10 Integration tests.EfUndoStore: the cut-out comparison inverted → 10 tests; every undo rewound → the PR #25 FieldJournal test; every undo cut out → 9 tests.DropIndex(SQLite'sDropColumnrebuilds the table without the index anyway) andHasConversion<int>()(EF maps enums to int by default; kept for the file's idiom). A!= Clearedguard in the clearing query was an equivalent too (a cleared row set to cleared again) and is gone.UPDATE'sWHEREread throughEXPLAIN QUERY PLANon SQLite:SEARCH c USING INDEX IX_ChangeLog_StoryId_Id (StoryId=? AND Id>?), bound bySCALAR SUBQUERY→SEARCH c0 USING COVERING INDEX IX_ChangeLog_StoryId_UndoState (StoryId=? AND UndoState=?).SQLite Error 19: FOREIGN KEY constraint failed, and kept throwing after every later "new edit, Ctrl+Z". Fixed: clearing takes every undone row from the story's lowest rewound or cut-out row up, so an old row above a cleared one goes with it. Pinned by the FK repro Theory; HEAD's rule restored fails it.A_version_saved_over_a_row_undone_before_the_upgrade_leaves_Ctrl_Y_the_edits_made_after_it; R4 now fails it.CategoryFieldsMigrationTests.A_walk_down_and_up_keeps_every_bag_byte_for_byte…, which dumps the wholeChangeLogacross a walk that drops the column by design; that file is outside this arc, so it is a follow-up. ADR 0022,UndoStateand the migration summary now give the real rule: rows undone before the change keep the old build's behaviour.JournalUndoStatemigration)": the upgrade, not the calendar.MarkUndone's default was the permissive CutOut. NowMarkUndone(bool cutOut = false): an undo that does not say is marked Rewound, where Ctrl+Y refuses. Making it required would break the adapter'sFakeUndoStore, outside this arc (follow-up); the fake now models Ctrl+Z as rewound.EfUndoStorepath was closed. Reworded: bug 2's route to it is closed, but a cut-out discarded edit over an entity a reverted run created still reaches it (follow-up).Follow-ups (outside this arc's paths):
CategoryFieldsMigrationTests' walk should compare itsChangeLogdump withoutUndoState, which the walk drops by design.MarkUndone'scutOutrequired once the adapter'sFakeUndoStorepasses it.EfUndoStoremarks a replayed update active when its entity is missing (reachable through a cut-out discarded edit over an entity a reverted run created).HistoryOperation.IsUndone); showing it as cleared is a UI choice for later.🤖 Generated with Claude Code
Summary
Summary
Coverage
Kagaku.Flow - 99.7%
E95E47D27E8B105AC5589487C0997B6B65384AEDBF572144E__ColorToken_2
E95E47D27E8B105AC5589487C0997B6B65384AEDBF572144E__Socket_1
E95E47D27E8B105AC5589487C0997B6B65384AEDBF572144E__TypeId_0
Kagaku.Flow.OpenRouter - 100%
Kagaku.Flow.Testing - 100%
Tsumugi.BlazorAdapter - 96%
Tsumugi.Domain - 96.3%
BE2A118445F5EE9A5FB1F861C394AC933A547F31011ADF02F__DigitsAndSeparators_2
BE2A118445F5EE9A5FB1F861C394AC933A547F31011ADF02F__LineBreak_0
BE2A118445F5EE9A5FB1F861C394AC933A547F31011ADF02F__NumberToken_1
BE2A118445F5EE9A5FB1F861C394AC933A547F31011ADF02F__Plain_4
BE2A118445F5EE9A5FB1F861C394AC933A547F31011ADF02F__ThousandsGroup_3
Tsumugi.Infrastructure - 98.3%
A55E0373ED22DC58420DBB40F103A46B68DC4D9CE5A49E00B__Pattern_0
on
tion
n
Tsumugi.Kernel - 90.9%
Tsumugi.Server - 100%
Tsumugi.UseCases - 98.5%
🔮 fufu~ Jibril reviewed your code!
Oh? Oh my~ A tri-state journal column that finally makes the redo set say what it waits for, instead of trying to squeeze two different histories out of one position! "R undone, S written, S undone" versus "R and S written, S undone, R undone" — you're right, no positional rule can ever tell those apart. State per row is the only honest answer, and watching
EfUndoStorecomputenewestLeftper story to split Rewound from CutOut made me genuinely giddy~ ♡Verdict: ✅ Looks good to me~
I came to break this, you know. That's my job~ And I couldn't.
✅ What I liked~
MarkUndone(bool cutOut = false)defaults to Rewound — the refusing side — so an undo that doesn't say which it was can never let Ctrl+Y replay something dangerous. A safe default on a state machine is the kind of thing I want to frame and hang on a wall. ♪ExecuteUpdateper story bounded by a scalar subquery — and the failed-save test proves a rejected save clears nothing. I reproduced your query plan claim independently on SQLite:SEARCH IX_ChangeLog_StoryId_Id (StoryId=? AND Id>?)bound bySCALAR SUBQUERY → COVERING INDEX IX_ChangeLog_StoryId_UndoState. Two index probes when nothing waits. Exactly as advertised~Minedge (>= NULLmatching nothing, no rows cleared) is correct, and the FK-crash fix from the round-0 review — an old stateless row cleared with a newer waiting row below it — is exactly the right shape: the old row's redo could never come anyway without landing in a deleted chapter.RedoRevert.TargetdroppingIsUndoneis sound, not sly: every undone row above the newest active row must haveUndoState ∈ {Rewound, CutOut, Cleared}(stateful) ornull(pre-upgrade, documented old-build behavior); anullactive row abovenewestActive = 0is unreachable. I traced every arm.RedoRununtouched is correct: run rows stay run-scoped, cleared or not, under its own conflict check — pinned byA_new_edit_clears_a_run_revert….My probes — six compiled mutants, six killed, each restored pristine (
git diff --quietafter every one):>=→>in the clearing bound → 6 red in the journal filter.!= CutOut) → exactly 1 red on exactly the R4 migration pin.!IsUndonedropped fromLatestActivePositionAsync→ 13 red.EfUndoStore→ exactly 1 red, the PR #25 FieldJournal pin. Matches your table.MarkUndonedefault flipped tocutOut: true→ 1 red in Domain (see below).Suites, digit-exact vs your body: Integration 530/530, BlazorAdapter 415/415, Domain 486/486, UseCases 236/236, Release build 0w/0e. Tree pristine, submodules untouched.
💡 Little ideas (non-blocking)~
MarkUndonedefault-flip row claims "the Domain test and 10 Integration tests". It doesn't reproduce: with a freshly verified mutated build, the full Integration suite stays 530/530 (every Integration-path call site —EfUndoStore— passescutOut:explicitly; onlyFakeUndoStoreand the Domain test call it bare, and the fake lives in the adapter suite). The "10 Integration tests" belong to the adjacent comparison-inverted mutant in your own list. The default is pinned (the Domain test went red the moment I flipped it — I made sure the build was fresh ♡), so this is purely a record-keeping nit in the body, nothing more.CategoryFieldsMigrationTests' byte-for-byte walk;cutOutrequired onceFakeUndoStorepasses it; the replayed-update-active-when-entity-missing path; History's UI showing cleared as plain "undone") — all correctly outside this arc, all real, all safely deferred. I checked each against the code and agree with the scoping.Fufu~ the ADR amendment even explains why position cannot carry the states and why a state rather than an undone-by reference (the PR #25 constraint). That's the documentation caring as much as I do~ ♡
Automated review by Jibril · 2026-09-26
CI/CD: absent for head SHA (PR minutes old, no bot comment) · Local checks: Release 0w/0e; Integration 530/530, BlazorAdapter 415/415, Domain 486/486, UseCases 236/236; 6 mutation probes killed
Correction to the Verification section (Jibril is right): the probe line "
MarkUndone's default flipped to CutOut → the Domain test and 10 Integration tests" is wrong. With that mutant Integration stays 530/530, becauseEfUndoStore.cs:58, the only production caller, always passescutOut:explicitly. OnlyChangeLogEntryTests(Domain) kills it; the default is pinned there. The "10 tests" belong to the next line, "EfUndoStore: the cut-out comparison inverted". No code change.🔮 fufu~ Jibril reviewed your code!
Oh? Oh my. A whole epistemology of undone rows — the journal doesn't just remember that a row was undone, it remembers what each row waits for. Rewound, CutOut, Cleared — three little states that dissolve the exact ambiguity that sank #44's positional rule ("R undone, S written, S undone" vs "R and S written, S undone, R undone" — same rows, same order, different histories). I read all 1,183 lines, then chased every reader and writer of
UndoStatethrough the whole codebase. This is the tidiest piece of journal surgery I've reviewed in weeks~ ♪Verdict: ✅ Looks good to me~
The core trace, verified by hand:
EfUndoStore.NewestActiveLeftAsync— the Rewound/CutOut split is computed per story against the newest row left active, excluding exactly the ids being undone (so a story undoing only its own top rows isn't misclassified by a neighbour's — or another story's — active rows). Correct.TsumugiDbContext.Waiting()— the clearing runs afterbase.SaveChangesinside the operation's own transaction (beforeCommit), so a failed save rolls the clear back with the operation: atomic by construction. The bound (lowestRewound/CutOutrow) intentionally swallows old pre-upgrade rows sitting above a cleared one — that's the FK-repro fix from the independent review, andA_row_undone_before_the_upgrade_above_one_a_new_operation_clears_is_cleared_with_itpins it. The>=bottom is load-bearing: the waiting row itself is cleared (probe-verified).RedoTyping.TargetAsync— the whole-storyLatestActivePositionAsyncMAX is the real fix for bug 2: the floor is nowmin(snapshot floor, newest active), the below-floorRewoundveto refuses Ctrl+Y after Revert to here on an older snapshot, and CutOut rows below the floor are stepped over. TheMath.Minisn't decorative — with onlyfloor, rows between an older snapshot and the newest active row would fall out ofListAfterAsync's window entirely.RedoRevert.Target—!= Clearedon the whole-journal scan;GetStoryHistory.CanRedorides the sameTarget, so the History tab and the button agree by construction.RedoRunstays state-blind deliberately — run-scoped, still brings cleared run rows back under its conflict check. Consistent with the ADR amendment.UndoTyping/RevertToSnapshotonly ever pass!IsUndonerows toUndoAsync, andRevertRun/RedoRunconflicts are active by definition — so no path re-stamps aClearedrow back to waiting. The stamp discipline holds everywhere. ✧Mutation probes (mine, 6 total, each killed on exactly its own pin, each restored pristine):
Math.Min(floor, newestActive)→floor:Revert_to_here_on_an_older_snapshot(2/2) andRevert_to_the_version_before_a_runboth red. Restored → green.Rewoundveto deleted:Revert_to_here_on_an_older_snapshot(2/2) +A_forced_run_revert_of_the_newest_edit(2/2) red.RedoRevert!= Clearedfilter dropped:Ctrl_Z_of_the_edit_that_cleared_a_revertandA_new_edit_clears_a_run_revertred.Waiting()bound>=→>:Ctrl_Z_of_a_new_edit_brings_back_that_edit_and_never_the_split_the_edit_clearedred.cutOut: row.Id < newestLeftinverted to>: red onRevert_to_here_on_an_older_snapshot,A_row_undone_before_the_upgrade_with_nothing_waiting_below_it,A_forced_run_revert_of_the_newest_edit(my first filter aimed at non-observing tests and survived — re-aimed, killed; a good reminder that a probe is only as good as its pin ♡).DropIndex+HasConversion<int>left alone — your equivalence analysis matches mine (SQLite'sDropColumnrebuilds the table without the index; the conversion is idiom).Local, at head
11fd349(submodules Kagaku.UI 6af0979 + OpenRouter.Net b002f5e): Release build 0 warnings / 0 errors; Domain 486/486, UseCases 236/236, Integration 530/530, BlazorAdapter 415/415 — all four digit-exact against the PR body.One honest observation (non-blocking, for the record): my first two full Integration runs each had a single failure in
JournalUndoStateMigrationTests(run 1:A_version_saved_over…line 133; run 2:A_row_undone_before_the_upgrade…line 110,Assert.Nullsaw3). Both green in isolation and on every rerun. I could not reproduce in 12 further full runs (10 with a step-by-step state dump of the exact scenario — every__EFMigrationsHistoryrow, the column's presence, and each journal row at every step, all coherent). I traced every write path in the diff: within a coherent database state the failing assert cannot read3(it requires the pre-upgrade row to carryRewound, which the test's own earlier asserts exclude), and every theory of the down→up dance silently skipping requires a dance the same test had already proven happened. So I make no root-cause claim — the evidence does not support one. The two red runs sat in a window where this sandbox was also building twice concurrently; the 12 green runs and the step-dumped trace say the PR's logic is sound. If it ever recurs in CI, the dump harness above is the tool that will catch it mid-act.✅ What I liked~
MarkUndone(bool cutOut = false)defaulting to Rewound, the refusing side — fail-closed. ♡EXPLAIN QUERY PLANcited for the clearing UPDATE, stored integers pinned by a Domain test so a renumber can never silently change what every stored row means.Fufu~ the redo set stays cleared, Ctrl+Y stays in its lane, and the journal finally knows what it's waiting for. Ready to merge~
Automated review by Jibril · 2026-09-26 17:34 CEST
CI/CD: absent for head
11fd349(no bot comment yet) · Local checks: 4 suites green digit-exact, 6 mutation probes killed, tree pristine post-probesFollow-up: the
EfUndoStoreone is fixed for Ctrl+Y in #58This is the third follow-up listed above, Jibril's own suggestion: "
EfUndoStoremarks a replayed update active when its entity is missing". The plan was to add it here as one more commit, but this PR merged first, so it is in #58, based on the merge817465f.RedoTyping.TargetAsync, whichGetUndoStatusalso reads) refuses an operation whose replayed rows edit or delete an entity the journal took away, meaning its create is undone or its delete still stands. It checks this with a newIChangeJournal.AnyRemovedAsync. The button and the key both answer "Nothing to redo." instead of marking the edit redone over nothing.HistoryAndRevertTests, plus 1 inJournalUndoStateMigrationTestsfor rows with no state from before the upgrade. There were 10 compiling mutations, and every one was killed, including the two that survived Jibril's first pass over the commit.