Undo every mutation, and keep backups on disk

[saforem2]

The bug

Undo was a single snapshot slot, and exactly one caller wrote to it:

// the only call site, in the "d" handler
m.captureDeleteSnapshot(m.board, n)
m.board.Delete(n.ID)

Every other mutation changed the board with no record. Pasting over a
note is the sharp edge:

case "ctrl+p":
    title, body := splitClipboardText(text)
    n.Title = title   // previous contents, gone
    n.Body = body
    m.saver.Touch()   // and 400ms later, gone from disk too

So ctrl+p on a focused note replaced its title and body outright, u
answered "nothing to undo", and the debounced save overwrote the only
copy on disk. There were no backups. The note was unrecoverable — which
is how this got reported.

In memory: a real undo stack

history.go holds a bounded stack of whole-workspace snapshots.
Workspace rather than Board because D deletes an entire board of notes,
and a board-level snapshot could not bring that back. A workspace is a
handful of flat structs, so a deep copy costs a few KB — much simpler
than a command/diff scheme, and it can't drift out of sync with the
mutation sites the way the single slot did.

All 30 mutation sites already funnelled through m.saver.Touch(), so
that was the natural seam. They now call m.mutate(label), which
snapshots and then touches. 50 steps, u undoes, ctrl+r redoes, and
the label shows in the toast (undo: paste into note).

Two cases needed thought rather than a mechanical swap:

• Editing snapshots once on entering the zoom-editor, so a session
  undoes as one step back to the pre-edit text — not zero steps, and not
  one per keystroke.
• Dragging snapshots at mouse-press. By release the note has
  already moved, so that is the only moment the pre-move position still
  exists.

Same-label pushes within 700ms coalesce, so a held arrow key costs one
entry rather than forty and cannot evict the state you actually want.

Deliberately outside undo: zoom, font, highlight, and background. They're
view state, and burying a real content change under six appearance steps
makes undo worse. Note that a snapshot is a whole-struct copy, so those
fields do ride along inside it — the restore carries the live values
across rather than the clone omitting them, keyed by GrainSeed since a
rename is itself undoable. (Review caught that I had claimed this
without implementing it; fixed in 1d9d0db.)

On disk: backups

Undo dies with the session. Each save that actually changes content now
copies the previous file aside first:

~/.local/share/redthread/backups/notes-20260814-160355.812734000.json

Newest 20 kept, stamped to nanosecond resolution — the debounce can fire
twice inside a millisecond, and a colliding name silently overwrites a
distinct version (50 rapid saves were leaving only 16 backups before
review caught this). Saves that would write identical bytes don't rotate,
so idling can't flush real history out of the ring. Rotation is
best-effort — a failure to back up never blocks the save itself, since a
full disk shouldn't cost you your live work on top of your history.

And a confirmation

ctrl+p over a note that already has text now asks: the footer reads
ctrl+p again to replace <title>, and only a second press within 3s
commits. Undo covers it regardless, but the confirmation is what you
actually see before the text disappears — which is the difference
between an inconvenience and the report that prompted this.

Testing

go test ./..., -race, go vet, gofmt all clean. Three new test
files: history semantics (deep copy, bounds, coalescing, redo
invalidation), the same paths driven through the model's real key
handlers, and backup rotation — including writing a backup back over
notes.json and loading it.

Reverting model.mutate to its pre-fix behavior fails six of them, so
they're testing the fix rather than describing it.

One note: a pre-existing data race

Saver.Touch runs SaveWorkspace on a time.AfterFunc goroutine, which
JSON-marshals the workspace while the UI thread mutates notes. go test -race flags it on current main too — I checked against unmodified code
before concluding it wasn't mine, so it is not introduced here and
not fixed here.

It is worth a small follow-up, though, and backups raise the stakes
slightly: a torn marshal can now be rotated into the backup ring. The fix
is either marshalling under a mutex the update loop also holds, or
snapshotting before handing off to the timer — cloneWorkspace from this
PR would do it. Happy to send that separately if you'd like.

This PR is independent of #3 (the light palette); they touch different
things and can merge in either order.

[Copilot]

Pull request overview

This PR addresses data-loss risk by introducing a real undo/redo stack that covers all workspace mutations (not just deletes) and by adding on-disk backup rotation for notes.json so prior states can be recovered across sessions.

Changes:

• Add a bounded workspace-snapshot history stack (undo/redo + coalescing) and integrate it via a centralized m.mutate(label) seam.
• Add best-effort backup rotation on save, keeping the newest 20 distinct prior versions.
• Add tests covering history semantics, integration via key handlers, and backup validity/rotation; update README to document undo/backups and redo.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
README.md	Documents multi-step undo/redo, paste confirmation, and backup recovery workflow.
internal/app/undo_integration_test.go	Integration tests that drive undo/redo via model key handlers and cover paste confirmation/coalescing.
internal/app/storage.go	Adds backup rotation/pruning and backup directory helper around SaveWorkspace.
internal/app/model.go	Replaces single-slot delete undo with history-backed undo/redo; centralizes mutation snapshotting; adds paste confirmation.
internal/app/history.go	New bounded snapshot-based undo/redo implementation with coalescing + deep clone helpers.
internal/app/history_test.go	Unit tests for history cloning, bounds, coalescing, redo invalidation, and round-trips.
internal/app/backup_test.go	Tests for backup creation, bounded rotation, unchanged-save behavior, and restore validity.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[saforem2]

Confirmed and fixed in 1d9d0db.

MoveActive rearranges w.Boards in place, and the call sat inside the if condition, so the snapshot recorded the already-reordered slice — undo was a straight no-op, not merely inaccurate.

Snapshot now happens first. That introduced a second-order problem: mutate would then fire even when the move was impossible (single board, or a wrap onto itself), burning an undo slot on nothing. Added Workspace.CanMoveActive(delta) so the caller can ask before committing.

TestUndoRestoresBoardOrder covers the reorder, TestNoOpBoardMoveDoesNotPushHistory the no-op. Reverting the ordering fails the first.

[saforem2]

Same fix as the } case above — both sites changed together in 1d9d0db.

[saforem2]

Correct, and worse than unreliable — that assertion could not fail. The wrong-behavior path clears pasteArmedUntil, so a ctrl+p that pasted immediately made !IsZero() false and the && short-circuited to a pass. I wrote a test that asserted nothing.

It also had no business depending on the system clipboard: absent in CI, and shared global state when present.

Rewritten in 1d9d0db. readClipboard is now a package var the tests stub, and the three paths are asserted separately:

• TestPasteOverANonEmptyNoteAsksFirst — first press changes nothing, arms the window, and shows the prompt
• TestSecondPasteReplacesAndIsUndoable — second press commits, and u gets the note back
• TestPasteIntoAnEmptyNoteDoesNotAsk — nothing to lose, so no nag

Replacing the guard condition with if false now fails the first one.

[saforem2]

Reproduced before fixing: 50 rapid saves with distinct content left only 16 backups. The rest collided on the filename and were silently overwritten — so the ring was quietly holding fewer versions than it claimed.

Fixed in 1d9d0db with 20060102-150405.000000000. Fixed-width, so lexical order still equals chronological order, which pruneBackups relies on when sorting to decide what to drop.

TestRapidSavesDoNotCollide asserts all 20 survive; TestBackupNamesSortChronologically pins the ordering property the pruner depends on.

[saforem2]

The best of the five — it caught a contradiction between the PR description and the code. Verified: with a note created, then a zoom out, u rewound the zoom too.

I kept the whole-struct copy rather than making cloneBoard selective. The snapshot being a complete copy is what makes it hard to get wrong; a clone that omits fields is a clone that silently drops the next field someone adds. The right place to express "view state is not undoable" is the restore, so applyRestoredWorkspace now carries the live zoom / font / highlight / background across.

One wrinkle worth naming: I first keyed the carry by board name, which broke immediately — renames are themselves undoable, so the match fails in exactly the case where the board is being restored. Keyed by GrainSeed instead, which is assigned once at creation and never changes.

Three tests in 1d9d0db: TestUndoLeavesViewStateAlone, TestUndoRenamePreservesViewState, TestViewStateChangesDoNotPushHistory.

[saforem2]

This one was inverted at the time — rotateBackup used millisecond precision then, and the README matched it. But the sibling comment about collisions was right, so the stamp is now nanosecond and the README did need updating.

Both moved together in 1d9d0db: the pattern reads notes-YYYYMMDD-HHMMSS.nnnnnnnnn.json and the worked example alongside it matches. TestBackupFilenameMatchesDocumentedPattern asserts the real filenames against that regex, so the docs cannot drift from the code again.

[saforem2]

@copilot the six review findings are addressed in 1d9d0db — four were real bugs (snapshot ordering on board reorder, view-state bleeding through undo, backup filename collisions, and a paste test whose assertion could not fail). Details are in the individual threads. Worth another look at the fixes if you're able.

[saforem2]

Same here — no rush.

Worth flagging the motivation for this one, since it is a data-loss bug rather than a feature: ctrl+p on a focused note replaced its title and body with the clipboard, u reported "nothing to undo" (truthfully — only the delete path ever recorded anything), and the 400ms debounce overwrote the only copy on disk. I lost a note to it, which is what prompted the PR.

So the two halves are: undo now covers every mutation rather than just deletes, and each content-changing save rotates the previous notes.json into backups/. ctrl+p over a non-empty note also asks before replacing.

If the whole thing is more than you want to take at once, the backup rotation in storage.go is about 60 lines and stands alone — happy to split that out as a minimal PR and drop the rest.

One thing I did not fix, noted in the description: Saver.Touch marshals the workspace on a timer goroutine while the UI thread mutates notes. go test -race flags it on main too, so it predates this work, but backups do raise the stakes slightly since a torn write can land in the ring. Happy to send that separately if you want it.

[saforem2]

Correction to something I said earlier in this PR: I reported the suite as -race clean, and it was not. Running the full package (rather than the filtered subsets I had been using) reproduced three data races, consistently.

They were in my tests, not the feature: pressKey arms the debounced saver, and its timer marshals the workspace on another goroutine ~400ms later — after the test has returned and while the next one is mutating. main is race-free because nothing there drives the saver from a test.

Fixed in a3536ea with a Saver.Cancel() that drops a pending write, called from newTestModel at teardown. Full suite is now clean across repeated runs.

The production race this was tripping over — SaveWorkspace marshalling on a time.AfterFunc goroutine while the update loop mutates notes — is still there and still out of scope for this PR, as noted in the description. Cancel() is a small piece of what a real fix needs, though.