handleRedo never call setPaths (re-opens #70 false-positive close) #89

New issue

Closed

opened 2026-09-14 14:52:44 +00:00 by Mark · 0 comments

Mark commented

2026-09-14 14:52:44 +00:00

Owner

Where: src/pages/Editor.tsx:408-418 (handleUndo, handleRedo).

Bug: handleUndo and handleRedo only adjust historyIndex; they never call setPaths(history[…]). The canvas consumes paths as a prop (EditorCanvas.tsx:16), so without a state update on paths, the canvas never re-renders.

// src/pages/Editor.tsx:408
const handleUndo = useCallback(() => {
  if (historyIndex > 0) {
    setHistoryIndex(historyIndex - 1);   // ← moves pointer only
    // ← MISSING: setPaths(deepClone(history[historyIndex - 1]));
  }
}, [historyIndex]);

// src/pages/Editor.tsx:414
const handleRedo = useCallback(() => {
  if (historyIndex < history.length - 1) {
    setHistoryIndex(historyIndex + 1);   // ← moves pointer only
    // ← MISSING: setPaths(deepClone(history[historyIndex + 1]));
  }
}, [history, historyIndex]);

The canUndo / canRedo flags computed at Editor.tsx:507-508 and :516-517 correctly reflect historyIndex, so the buttons enable/disable on time. But the canvas, which only re-renders when the paths prop changes, never updates. Visually: click Undo / press Ctrl+Z → button disables, nothing on canvas changes. Click Redo → button enables, nothing happens. Repeat 49 times → history is fully traversed, canvas still identical.

This was filed as #70 ("Editor undo/redo is non-functional (history never advances, no state snapshots taken)") on 2026-09-14 03:17 UTC and closed 23 minutes later (commit f68a671). That close was a false-positive: the commit only added two vi.mock(...) lines to Editor.test.tsx so the smoke test could mount <Editor /> (without AnimatePresence / ExportDialog mocks). It added no assertions on undo/redo behavior, no setPaths(history[…]) calls. The smoke test (Editor.test.tsx:45-65) only verifies that the layout zones render and that the filename snapshot round-trips — neither of which exercises undo/redo. So #70 was closed without the bug being fixed.

The pushHistory and setHistoryIndex plumbing was added correctly (Editor.tsx:184-194, plus the seven call sites at L256, L280, L293, L319, L351, L396, plus commitDigitizedResult analog), so history DOES advance now (history.length grows beyond 1 when the user mutates a node, and canUndo flips true on the next render). The new bug is purely in the read side — the undo/redo handlers don't restore paths from the snapshot at the new index.

Reproduction (any image uploaded + Confirm & Edit, or directly from /editor with the demo paths):

/editor opens with paths = DEMO_PATHS (or whatever the snapshot has).
Click a node in select_nodes mode, drag it ~10 px.
Press Ctrl+Z. Observe: nothing happens visually. (Optional: open React DevTools → Editor → history shows 5–10 entries from the drag, historyIndex decreased to 1 — but paths is still the post-drag state.)
Press Ctrl+Shift+Z. Same: pointer moves, canvas unchanged.

Severity: high. The Editor's primary safety net (undo) is missing. Users will lose work after any mutation. The buttons render enabled but are inert — classic "clickable but does nothing" class, same family as #58 (nav links) and #71 (Export PES / Settings).

Fix consistent with existing patterns: Mirror useWorkspace.ts:118-127 (pushHistory) which already does this correctly. The Worker should:

In handleUndo (Editor.tsx:408), after setHistoryIndex(historyIndex - 1):
```
setPaths(deepClone(history[historyIndex - 1]));
```
historyIndex here is the closure-captured value; the read order matters — capture the new index first, then read history[newIndex]. Cleanest: compute the new index, then call both setters with the computed value:
```
const handleUndo = useCallback(() => {
  setHistoryIndex((prev) => {
    if (prev <= 0) return prev;
    const newIndex = prev - 1;
    setPaths(deepClone(history[newIndex]));
    return newIndex;
  });
}, [history]);
```
(Same shape for handleRedo with newIndex = prev + 1 and the upper-bound guard.)
Add a regression test in src/pages/Editor.test.tsx that:
- mocks EditorCanvas with a prop-recorder that exposes the latest paths prop.
- calls handleUndo (via the keyboard shortcut mock firing onUndo).
- asserts the recorded paths matches the snapshot at the prior history index.
  The existing f68a671 mocks were insufficient — they let the smoke test mount but didn't actually exercise undo. The new test should fire the shortcut handler and assert the prop changed.

Cross-reference: #70 was closed false-positive on 2026-09-14; this issue is its re-open. The original commit's only contribution (AnimatePresence + ExportDialog mocks) is still useful — leave it. The fix is purely additive in the handler bodies.

**Where:** `src/pages/Editor.tsx:408-418` (`handleUndo`, `handleRedo`). **Bug:** `handleUndo` and `handleRedo` only adjust `historyIndex`; they never call `setPaths(history[…])`. The canvas consumes `paths` as a prop (`EditorCanvas.tsx:16`), so without a state update on `paths`, the canvas never re-renders. ```ts // src/pages/Editor.tsx:408 const handleUndo = useCallback(() => { if (historyIndex > 0) { setHistoryIndex(historyIndex - 1); // ← moves pointer only // ← MISSING: setPaths(deepClone(history[historyIndex - 1])); } }, [historyIndex]); // src/pages/Editor.tsx:414 const handleRedo = useCallback(() => { if (historyIndex < history.length - 1) { setHistoryIndex(historyIndex + 1); // ← moves pointer only // ← MISSING: setPaths(deepClone(history[historyIndex + 1])); } }, [history, historyIndex]); ``` The `canUndo` / `canRedo` flags computed at `Editor.tsx:507-508` and `:516-517` correctly reflect `historyIndex`, so the buttons enable/disable on time. But the canvas, which only re-renders when the `paths` prop changes, never updates. Visually: click Undo / press Ctrl+Z → button disables, nothing on canvas changes. Click Redo → button enables, nothing happens. Repeat 49 times → history is fully traversed, canvas still identical. **This was filed as #70 ("Editor undo/redo is non-functional (history never advances, no state snapshots taken)") on 2026-09-14 03:17 UTC and closed 23 minutes later (commit `f68a671`).** That close was a false-positive: the commit only added two `vi.mock(...)` lines to `Editor.test.tsx` so the smoke test could mount `<Editor />` (without `AnimatePresence` / `ExportDialog` mocks). It added no assertions on undo/redo behavior, no `setPaths(history[…])` calls. The smoke test (`Editor.test.tsx:45-65`) only verifies that the layout zones render and that the filename snapshot round-trips — neither of which exercises undo/redo. So #70 was closed without the bug being fixed. The `pushHistory` and `setHistoryIndex` plumbing was added correctly (Editor.tsx:184-194, plus the seven call sites at L256, L280, L293, L319, L351, L396, plus `commitDigitizedResult` analog), so history DOES advance now (history.length grows beyond 1 when the user mutates a node, and `canUndo` flips true on the next render). The new bug is purely in the read side — the undo/redo handlers don't restore `paths` from the snapshot at the new index. **Reproduction** (any image uploaded + Confirm & Edit, or directly from `/editor` with the demo paths): 1. `/editor` opens with `paths = DEMO_PATHS` (or whatever the snapshot has). 2. Click a node in `select_nodes` mode, drag it ~10 px. 3. Press Ctrl+Z. Observe: nothing happens visually. (Optional: open React DevTools → Editor → `history` shows 5–10 entries from the drag, `historyIndex` decreased to 1 — but `paths` is still the post-drag state.) 4. Press Ctrl+Shift+Z. Same: pointer moves, canvas unchanged. **Severity:** high. The Editor's primary safety net (undo) is missing. Users will lose work after any mutation. The buttons render enabled but are inert — classic "clickable but does nothing" class, same family as #58 (nav links) and #71 (Export PES / Settings). **Fix consistent with existing patterns:** Mirror `useWorkspace.ts:118-127` (`pushHistory`) which already does this correctly. The Worker should: 1. In `handleUndo` (Editor.tsx:408), after `setHistoryIndex(historyIndex - 1)`: ```ts setPaths(deepClone(history[historyIndex - 1])); ``` `historyIndex` here is the closure-captured value; the read order matters — capture the new index first, then read `history[newIndex]`. Cleanest: compute the new index, then call both setters with the computed value: ```ts const handleUndo = useCallback(() => { setHistoryIndex((prev) => { if (prev <= 0) return prev; const newIndex = prev - 1; setPaths(deepClone(history[newIndex])); return newIndex; }); }, [history]); ``` (Same shape for `handleRedo` with `newIndex = prev + 1` and the upper-bound guard.) 2. Add a regression test in `src/pages/Editor.test.tsx` that: - mocks `EditorCanvas` with a prop-recorder that exposes the latest `paths` prop. - calls `handleUndo` (via the keyboard shortcut mock firing `onUndo`). - asserts the recorded `paths` matches the snapshot at the prior history index. The existing `f68a671` mocks were insufficient — they let the smoke test mount but didn't actually exercise undo. The new test should fire the shortcut handler and assert the prop changed. **Cross-reference:** #70 was closed false-positive on 2026-09-14; this issue is its re-open. The original commit's only contribution (`AnimatePresence` + `ExportDialog` mocks) is still useful — leave it. The fix is purely additive in the handler bodies.