Fix undo/redo bug

[ecfairle]

Close #170. Forked from #174

[ecfairle]

I added onto @heruiwoniou 's PR since I have been using his branch for a while now in a project. There is a minor bug in his PR, that i believe has to do with the extra strokes being recorded on the canvas while the history is being updated. Which would explain why the undo/redo tests were failing.

I moved the history updating logic into the mouse-down / redo / undo / clear functions so there's no timing issues (basically record history on the "next" action instead of the "current" action).

Thanks for making this utility! Let me know if you have any questions or want me to make any changes.

[Copilot]

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (2)

packages/react-sketch-canvas/src/ReactSketchCanvas/index.tsx:174

• Resetting history to an empty array and setting historyPos to -1 may break subsequent undo/redo operations. Consider resetting to the initial state (e.g., history as [[]] and historyPos as 0) to maintain consistency.

setHistory([]);

packages/react-sketch-canvas/src/ReactSketchCanvas/index.tsx:199

• [nitpick] Incrementing historyPos before adding the current stroke state via addLastStroke might capture a stale currentPaths state. Reviewing the order of operations to ensure accurate history capture is recommended.

setHistoryPos(pos => pos + 1);

[vinothpandian]

@ecfairle Thank you for the PR. I swear I’m going to take this weekend to merge these PRs and release the new version!

Can you please look at the review comments from Copilot if you have time?

[vinothpandian]

This implementation is creating a race condition when undo or redo buttons are pressed quickly, especially on large codebases. I'm trying to fix this

[ecfairle]

Ah, right. It seems like the correct solution here is to refactor the logic for history-altering events as an event queue. I think that's closer to the underlying mental model for how a canvas should work. Anything else would be kind of hack imo. What do you think about doing that?

[vinothpandian]

Yes @ecfairle. That makes sense.

[ecfairle]

How does this look?

[Copilot]

Pull Request Overview

This PR fixes the undo/redo bug by adding support for loading canvas paths and refactoring the internal history management. Key changes include:

• Adding new props and UI for loading paths via a "Load Paths" button.
• Updating tests to cover undo/redo functionality with loaded paths and additional drawing scenarios.
• Refactoring ReactSketchCanvas to replace separate stacks with a unified history and an operation queue for canvas operations.

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File	Description
packages/tests/src/stories/WithUndoRedoButtons.tsx	Adds new loadPaths props, handler, and button for loading paths.
packages/tests/src/actions/undoRedo.spec.tsx	Updates tests to pass the new paths prop and adds tests for loadPaths, undo after clear, and combined draw-load actions.
packages/react-sketch-canvas/src/ReactSketchCanvas/index.tsx	Refactors canvas state management to use a unified history and operation queue for undo, redo, clear, and loadPaths operations.

[Copilot]

Referencing currentPaths directly here may risk using a stale state. Consider using a functional update or ensuring the latest state is accessed to avoid potential race conditions.

Suggested change

	setHistory(his => [...his.slice(0, historyPos+1), [...currentPaths, ...operation.payload!]]);
	setHistory(his => [...his.slice(0, historyPos+1), [...his[historyPos] || [], ...operation.payload!]]);

[Copilot]

[nitpick] Consider calling addLastStroke() before incrementing historyPos to ensure that the current canvas state is recorded accurately prior to state changes.

Suggested change

	setHistoryPos(pos => pos + 1);
	addLastStroke();
	addLastStroke();
	setHistoryPos(pos => pos + 1);