Mark/Kimi_Agent_Brother_10_Digitizing_App

Fork 0

Editor's undo/redo doesn't truncate the redo stack when a new edit is made after undoing — user can re-apply the 'undone' action by pressing Redo, even though the model says it should be discarded #237

New issue

Closed

opened 2026-10-04 02:33:08 +00:00 by Mark · 1 comment

Mark commented

2026-10-04 02:33:08 +00:00

Owner

Finding

src/pages/Editor.tsx:463 pushHistory does:

const pushHistory = useCallback((snapshot: StitchPath[]) => {
  const deepSnapshot = JSON.parse(JSON.stringify(snapshot));
  setHistory((prev) => {
    const next = [...prev, deepSnapshot];
    const shiftCount = next.length - HISTORY_LIMIT;
    return shiftCount > 0 ? next.slice(shiftCount) : next;
  });
  setHistoryIndex((prev) => {
    return Math.min(prev + 1, HISTORY_LIMIT - 1);
  });
}, []);

The setHistory adds the new snapshot to the end of the array. But it does NOT truncate the redo stack. The standard undo/redo model is:

history = [...edits, currentEdit] where the array ends at the current edit
When the user undoes, the pointer moves back (currentEdit = history[historyIndex - 1])
When the user makes a NEW edit, the redo stack (everything after historyIndex) is discarded

The current implementation just appends the new edit and bumps the index. So the redo stack is preserved.

Repro

Edit 1 (add node) → history = [s0, s1], historyIndex = 1
Edit 2 (move node) → history = [s0, s1, s2], historyIndex = 2
Press Cmd+Z (undo) → historyIndex = 1, paths = s1
Edit 3 (move a different node) → history = [s0, s1, s2, s3], historyIndex = 3
Press Cmd+Shift+Z (redo) → historyIndex = 4, paths = s4 (wait — there's no s4; let me re-think)

OK, the actual sequence depends on what handleUndo does. Let me re-trace.

After step 3: history = [s0, s1, s2], historyIndex = 1, paths = s1.

After step 4: handleNodeDrag or similar is called. It calls pushHistory(currentPaths). currentPaths is s1 (the state from step 3). So the new entry in history is... a duplicate of s1? Or a forward move from s1 to a new state s3?

Actually, the model is "history stores snapshots" and the snapshot is the state BEFORE the change. So:

After Edit 1: history = [s0, s1] (s0 is the initial, s1 is after edit 1)
After Edit 2: history = [s0, s1, s2] (s2 is after edit 2)
After Undo: historyIndex = 1, paths = s1 (we're back to s1)
After Edit 3 (a new move): pushHistory(s1) → history = [s0, s1, s2, s1_new] where s1_new is a snapshot of paths at the time of the push, which IS s1.
After Redo: historyIndex = 2, paths = s2. Wait, that's going forward through the array, so it should be s2 — which is the "undone" edit 2 state.

So the user undid edit 2, then made edit 3, and now can redo "back" to s2. But s2 is the edit 2 state — which they thought they undid. The redo brings them to a state they thought was discarded.

This is the standard undo/redo bug when the redo stack isn't cleared.

Where

pushHistory: src/pages/Editor.tsx:463-475
The setHistory call: line 467 (setHistory((prev) => [...prev, deepSnapshot]))
The setHistoryIndex call: line 471-473 (bump index but don't truncate)

Suggested fix shape

In pushHistory, truncate the redo stack before appending:

const pushHistory = useCallback((snapshot: StitchPath[]) => {
  const deepSnapshot = JSON.parse(JSON.stringify(snapshot));
  setHistory((prev) => {
    // Truncate the redo stack: keep only entries up to and including historyIndex + 1
    const truncated = prev.slice(0, historyIndexRef.current + 1);
    const next = [...truncated, deepSnapshot];
    const shiftCount = next.length - HISTORY_LIMIT;
    return shiftCount > 0 ? next.slice(shiftCount) : next;
  });
  setHistoryIndex((prev) => {
    return Math.min(prev + 1, HISTORY_LIMIT - 1);
  });
}, []);

historyIndexRef is a ref that mirrors historyIndex (so the callback can read the freshest value without a dependency cycle):

const historyIndexRef = useRef(0);
useEffect(() => { historyIndexRef.current = historyIndex; }, [historyIndex]);

The fix: when pushHistory is called after some undos, drop everything after the current historyIndex before appending the new entry. This way the redo stack is gone and the user can't re-apply the "undone" action.

Severity

Medium. The bug is subtle but affects the user's mental model of undo/redo. Power users notice.

Discovered by

Code review (2026-10-02) of editor undo/redo flow.

#196 (open) — Editor MAX_HISTORY. Same history management code; the truncation fix here is a related bug.
The pattern of "snapshot before change, append to history, bump index" is standard; the missing piece is "truncate redo stack on new edit."
Figma/Illustrator/Inkscape all correctly discard the redo stack on a new edit; this is the standard behavior.

## Finding `src/pages/Editor.tsx:463` `pushHistory` does: ```ts const pushHistory = useCallback((snapshot: StitchPath[]) => { const deepSnapshot = JSON.parse(JSON.stringify(snapshot)); setHistory((prev) => { const next = [...prev, deepSnapshot]; const shiftCount = next.length - HISTORY_LIMIT; return shiftCount > 0 ? next.slice(shiftCount) : next; }); setHistoryIndex((prev) => { return Math.min(prev + 1, HISTORY_LIMIT - 1); }); }, []); ``` The `setHistory` adds the new snapshot to the end of the array. But it does NOT truncate the redo stack. The standard undo/redo model is: - `history = [...edits, currentEdit]` where the array ends at the current edit - When the user undoes, the pointer moves back (currentEdit = history[historyIndex - 1]) - When the user makes a NEW edit, the redo stack (everything after historyIndex) is discarded The current implementation just appends the new edit and bumps the index. So the redo stack is preserved. ## Repro 1. Edit 1 (add node) → history = [s0, s1], historyIndex = 1 2. Edit 2 (move node) → history = [s0, s1, s2], historyIndex = 2 3. Press Cmd+Z (undo) → historyIndex = 1, paths = s1 4. Edit 3 (move a different node) → history = [s0, s1, s2, s3], historyIndex = 3 5. Press Cmd+Shift+Z (redo) → historyIndex = 4, paths = s4 (wait — there's no s4; let me re-think) OK, the actual sequence depends on what `handleUndo` does. Let me re-trace. After step 3: history = [s0, s1, s2], historyIndex = 1, paths = s1. After step 4: handleNodeDrag or similar is called. It calls `pushHistory(currentPaths)`. `currentPaths` is `s1` (the state from step 3). So the new entry in history is... a duplicate of s1? Or a forward move from s1 to a new state s3? Actually, the model is "history stores snapshots" and the snapshot is the state BEFORE the change. So: - After Edit 1: history = [s0, s1] (s0 is the initial, s1 is after edit 1) - After Edit 2: history = [s0, s1, s2] (s2 is after edit 2) - After Undo: historyIndex = 1, paths = s1 (we're back to s1) - After Edit 3 (a new move): pushHistory(s1) → history = [s0, s1, s2, s1_new] where s1_new is a snapshot of `paths` at the time of the push, which IS s1. - After Redo: historyIndex = 2, paths = s2. Wait, that's going forward through the array, so it should be s2 — which is the "undone" edit 2 state. So the user undid edit 2, then made edit 3, and now can redo "back" to s2. But s2 is the edit 2 state — which they thought they undid. The redo brings them to a state they thought was discarded. This is the standard undo/redo bug when the redo stack isn't cleared. ## Where - `pushHistory`: `src/pages/Editor.tsx:463-475` - The setHistory call: line 467 (`setHistory((prev) => [...prev, deepSnapshot])`) - The setHistoryIndex call: line 471-473 (bump index but don't truncate) ## Suggested fix shape In `pushHistory`, truncate the redo stack before appending: ```ts const pushHistory = useCallback((snapshot: StitchPath[]) => { const deepSnapshot = JSON.parse(JSON.stringify(snapshot)); setHistory((prev) => { // Truncate the redo stack: keep only entries up to and including historyIndex + 1 const truncated = prev.slice(0, historyIndexRef.current + 1); const next = [...truncated, deepSnapshot]; const shiftCount = next.length - HISTORY_LIMIT; return shiftCount > 0 ? next.slice(shiftCount) : next; }); setHistoryIndex((prev) => { return Math.min(prev + 1, HISTORY_LIMIT - 1); }); }, []); ``` `historyIndexRef` is a ref that mirrors `historyIndex` (so the callback can read the freshest value without a dependency cycle): ```ts const historyIndexRef = useRef(0); useEffect(() => { historyIndexRef.current = historyIndex; }, [historyIndex]); ``` The fix: when pushHistory is called after some undos, drop everything after the current `historyIndex` before appending the new entry. This way the redo stack is gone and the user can't re-apply the "undone" action. ## Severity Medium. The bug is subtle but affects the user's mental model of undo/redo. Power users notice. ## Discovered by Code review (2026-10-02) of editor undo/redo flow. ## Related - #196 (open) — Editor MAX_HISTORY. Same history management code; the truncation fix here is a related bug. - The pattern of "snapshot before change, append to history, bump index" is standard; the missing piece is "truncate redo stack on new edit." - Figma/Illustrator/Inkscape all correctly discard the redo stack on a new edit; this is the standard behavior.

Mark added the

bug

triage-approved

labels

2026-10-04 02:33:09 +00:00

Mark referenced this issue from a commit

2026-10-04 07:48:05 +00:00

fix(#237): discard redo stack on new edit so Redo does not replay undone state

Mark commented

2026-10-04 07:48:20 +00:00

Author

Owner

Fixed in commit 65fff8a (origin/main).

pushHistory now slices prev to historyIndexRef.current + 1 before appending the new snapshot, so a new edit after undoing drops the redo stack — matching the standard undo/redo model used by Figma / Illustrator / Inkscape.

A historyIndexRef mirrors historyIndex so the stable useCallback can read the freshest value without taking historyIndex into its dep array (which would re-create the callback on every edit and cascade through every handler depending on it).

Regression test added to src/pages/Editor.test.tsx — "new edit after undo discards redo stack (#237)". Verified it fails without the fix and passes with it. Full suite: 1200/1200 tests pass, tsc clean, lint clean.

Fixed in commit 65fff8a (origin/main). pushHistory now slices prev to historyIndexRef.current + 1 before appending the new snapshot, so a new edit after undoing drops the redo stack — matching the standard undo/redo model used by Figma / Illustrator / Inkscape. A historyIndexRef mirrors historyIndex so the stable useCallback can read the freshest value without taking historyIndex into its dep array (which would re-create the callback on every edit and cascade through every handler depending on it). Regression test added to src/pages/Editor.test.tsx — "new edit after undo discards redo stack (#237)". Verified it fails without the fix and passes with it. Full suite: 1200/1200 tests pass, tsc clean, lint clean.

Mark closed this issue

2026-10-04 07:48:23 +00:00