Merge pull request #52 from LIDSOL/fix-update-undo-redo

The undo/redo functionality has been modified to prevent duplicate ac…

Author: HansMarcus01

CONTRIBUTING.md

@@ -64,6 +64,42 @@ Please follow these steps to have your contribution considered by the maintainer
 - Ensure your code passes ESLint.
 - Make sure the code base is in English, this includes comments and variable names.
 
+## Undo/Redo: pushUndo vs setUndoStack
+
+When recording history, prefer the high-level helper `pushUndo(action)` from UI/components for simple append-and-clear behavior. `pushUndo` centralizes validation, deduplication and coalescing so the app maintains a sane undo/redo history.
+
+However, some modules implement the low-level undo/redo engine and must manipulate the stacks directly using `setUndoStack` / `setRedoStack`. Do NOT replace those internal usages with `pushUndo`.
+
+Files that intentionally use direct stack manipulation include (but are not limited to):
+
+- `src/components/EditorCanvas/Canvas.jsx`
+- `src/components/EditorHeader/ControlPanel.jsx`
+- `src/components/EditorHeader/Modal/Modal.jsx`
+- `src/components/Workspace.jsx`
+- `src/context/UndoRedoContext.jsx`
+
+Correct usage (UI-side append+clear):
+```js
+// Add a simple EDIT action and clear redo
+pushUndo({
+    action: Action.EDIT,
+    element: ObjectType.TABLE,
+    tid,
+    undo,
+    redo,
+    message,
+});
+```
+
+Incorrect to change internal code to this (keep low-level code as-is):
+```js
+// Internal undo engine — keep setUndoStack for low-level manipulation
+setUndoStack((prev) => prev.filter((_, i) => i !== prev.length - 1));
+setRedoStack((prev) => [...prev, action]);
+```
+
+If you're unsure which to use, prefer `pushUndo` for component-level changes and ask during code review for anything touching the stacks directly.
+
 ## Additional Notes
 
 ### Issue and Pull Request Labels
@@ -87,3 +123,4 @@ If you have any questions, please feel free to reach out to us through the follo
 ---
 
 Thank you for your contributions! ❤️
+

src/components/EditorCanvas/Area.jsx

@@ -203,7 +203,7 @@ function EditPopoverContent({ data }) {
   const [editField, setEditField] = useState({});
   const { setSaveState } = useSaveState();
   const { updateArea, deleteArea } = useAreas();
-  const { setUndoStack, setRedoStack } = useUndoRedo();
+  const { pushUndo } = useUndoRedo();
   const { t } = useTranslation();
 
   return (
@@ -216,11 +216,9 @@ function EditPopoverContent({ data }) {
           className="me-2"
           onChange={(value) => updateArea(data.id, { name: value })}
           onFocus={(e) => setEditField({ name: e.target.value })}
-          onBlur={(e) => {
-            if (e.target.value === editField.name) return;
-            setUndoStack((prev) => [
-              ...prev,
-              {
+            onBlur={(e) => {
+              if (e.target.value === editField.name) return;
+              pushUndo({
                 action: Action.EDIT,
                 element: ObjectType.AREA,
                 aid: data.id,
@@ -230,32 +228,26 @@ function EditPopoverContent({ data }) {
                   areaName: e.target.value,
                   extra: "[name]",
                 }),
-              },
-            ]);
-            setRedoStack([]);
-          }}
+              });
+            }}
         />
         <Popover
           content={
             <div className="popover-theme">
               <ColorPalette
                 currentColor={data.color}
                 onPickColor={(c) => {
-                  setUndoStack((prev) => [
-                    ...prev,
-                    {
-                      action: Action.EDIT,
-                      element: ObjectType.AREA,
-                      aid: data.id,
-                      undo: { color: data.color },
-                      redo: { color: c },
-                      message: t("edit_area", {
-                        areaName: data.name,
-                        extra: "[color]",
-                      }),
-                    },
-                  ]);
-                  setRedoStack([]);
+                  pushUndo({
+                    action: Action.EDIT,
+                    element: ObjectType.AREA,
+                    aid: data.id,
+                    undo: { color: data.color },
+                    redo: { color: c },
+                    message: t("edit_area", {
+                      areaName: data.name,
+                      extra: "[color]",
+                    }),
+                  });
                   updateArea(data.id, {
                     color: c,
                   });

src/components/EditorCanvas/Canvas.jsx

@@ -98,6 +98,12 @@ export default function Canvas() {
   const { notes, updateNote, addNote, deleteNote } = useNotes();
   const { layout } = useLayout();
   const { settings } = useSettings();
+  // IMPORTANT: this file implements the internal undo/redo machinery.
+  // Do NOT replace direct calls to `setUndoStack` or `setRedoStack` here with
+  // `pushUndo`. `setUndoStack` is intentionally used for low-level operations
+  // (popping, re-queuing, reconstructing complex actions) that must remain
+  // under explicit control. `pushUndo` is a higher-level helper for UI-side
+  // append+clear behavior and will not preserve the internal semantics.
   const { undoStack, redoStack, setRedoStack, setUndoStack, pushUndo } =
     useUndoRedo();
 
@@ -808,18 +814,14 @@ export default function Canvas() {
           }
         }
 
-        setUndoStack((prev) => [
-          ...prev,
-          {
-            action: Action.EDIT,
-            element: ObjectType.RELATIONSHIP,
-            rid: relationshipContextMenu.relationshipId,
-            undo: undoData,
-            redo: redoData,
-            message: `Swap direction for ${relationship.name}`,
-          },
-        ]);
-        setRedoStack([]);
+        pushUndo({
+          action: Action.EDIT,
+          element: ObjectType.RELATIONSHIP,
+          rid: relationshipContextMenu.relationshipId,
+          undo: undoData,
+          redo: redoData,
+          message: `Swap direction for ${relationship.name}`,
+        });
 
         setRelationships((prev) =>
           prev.map((e, idx) => {
@@ -867,28 +869,24 @@ export default function Canvas() {
           ? SubtypeRestriction.DISJOINT_TOTAL
           : undefined;
 
-        setUndoStack((prev) => [
-          ...prev,
-          {
-            action: Action.EDIT,
-            element: ObjectType.RELATIONSHIP,
-            rid: relationshipContextMenu.relationshipId,
-            undo: {
-              relationshipType: relationship.relationshipType,
-              cardinality: relationship.cardinality,
-              subtype: relationship.subtype,
-              subtype_restriction: relationship.subtype_restriction,
-            },
-            redo: {
-              relationshipType: newType,
-              cardinality: defaultCardinality,
-              subtype: isBecomingSubtype,
-              subtype_restriction: defaultSubtypeRestriction,
-            },
-            message: `Change type for ${relationship.name}`,
+        pushUndo({
+          action: Action.EDIT,
+          element: ObjectType.RELATIONSHIP,
+          rid: relationshipContextMenu.relationshipId,
+          undo: {
+            relationshipType: relationship.relationshipType,
+            cardinality: relationship.cardinality,
+            subtype: relationship.subtype,
+            subtype_restriction: relationship.subtype_restriction,
+          },
+          redo: {
+            relationshipType: newType,
+            cardinality: defaultCardinality,
+            subtype: isBecomingSubtype,
+            subtype_restriction: defaultSubtypeRestriction,
           },
-        ]);
-        setRedoStack([]);
+          message: `Change type for ${relationship.name}`,
+        });
 
         // When becoming subtype, convert existing FK fields to primary keys
         if (isBecomingSubtype && !wasSubtype) {
@@ -945,18 +943,14 @@ export default function Canvas() {
       const relationship =
         relationships[relationshipContextMenu.relationshipId];
       if (relationship) {
-        setUndoStack((prev) => [
-          ...prev,
-          {
-            action: Action.EDIT,
-            element: ObjectType.RELATIONSHIP,
-            rid: relationshipContextMenu.relationshipId,
-            undo: { cardinality: relationship.cardinality },
-            redo: { cardinality: newCardinality },
-            message: `Change cardinality for ${relationship.name}`,
-          },
-        ]);
-        setRedoStack([]);
+        pushUndo({
+          action: Action.EDIT,
+          element: ObjectType.RELATIONSHIP,
+          rid: relationshipContextMenu.relationshipId,
+          undo: { cardinality: relationship.cardinality },
+          redo: { cardinality: newCardinality },
+          message: `Change cardinality for ${relationship.name}`,
+        });
         setRelationships((prev) =>
           prev.map((e, idx) =>
             idx === relationshipContextMenu.relationshipId
@@ -1023,18 +1017,14 @@ export default function Canvas() {
           return; // Already has default name
         }
 
-        setUndoStack((prev) => [
-          ...prev,
-          {
-            action: Action.EDIT,
-            element: ObjectType.RELATIONSHIP,
-            rid: relationshipContextMenu.relationshipId,
-            undo: { name: relationship.name },
-            redo: { name: defaultName },
-            message: `Set default name for ${defaultName}`,
-          },
-        ]);
-        setRedoStack([]);
+        pushUndo({
+          action: Action.EDIT,
+          element: ObjectType.RELATIONSHIP,
+          rid: relationshipContextMenu.relationshipId,
+          undo: { name: relationship.name },
+          redo: { name: defaultName },
+          message: `Set default name for ${defaultName}`,
+        });
         updateRelationship(relationshipContextMenu.relationshipId, {
           name: defaultName,
         });
@@ -2540,16 +2530,6 @@ export default function Canvas() {
     setDragging({ element: ObjectType.NONE, id: -1, prevX: 0, prevY: 0 });
     setResizing({ element: ObjectType.NONE, id: -1, prevX: 0, prevY: 0 });
     if (panning.isPanning && didPan()) {
-      pushUndo({
-        action: Action.PAN,
-        undo: { x: panning.x, y: panning.y },
-        redo: transform.pan,
-        message: t("move_element", {
-          coords: `(${transform?.pan.x}, ${transform?.pan.y})`,
-          name: "diagram",
-        }),
-      });
-      setRedoStack([]);
       setSelectedElement((prev) => ({
         ...prev,
         element: ObjectType.NONE,

src/components/EditorCanvas/Note.jsx

@@ -31,7 +31,7 @@ export default function Note({ data, onPointerDown, onContextMenu }) {
   const { t } = useTranslation();
   const { setSaveState } = useSaveState();
   const { updateNote, deleteNote } = useNotes();
-  const { setUndoStack, setRedoStack } = useUndoRedo();
+  const { pushUndo } = useUndoRedo();
   const { selectedElement, setSelectedElement } = useSelect();
 
   const handleChange = (e) => {
@@ -48,21 +48,17 @@ export default function Note({ data, onPointerDown, onContextMenu }) {
     textarea.style.height = "0";
     textarea.style.height = textarea.scrollHeight + "px";
     const newHeight = textarea.scrollHeight + 16 + 20 + 4;
-    setUndoStack((prev) => [
-      ...prev,
-      {
-        action: Action.EDIT,
-        element: ObjectType.NOTE,
-        nid: data.id,
-        undo: editField,
-        redo: { content: e.target.value, height: newHeight },
-        message: t("edit_note", {
-          noteTitle: e.target.value,
-          extra: "[content]",
-        }),
-      },
-    ]);
-    setRedoStack([]);
+    pushUndo({
+      action: Action.EDIT,
+      element: ObjectType.NOTE,
+      nid: data.id,
+      undo: editField,
+      redo: { content: e.target.value, height: newHeight },
+      message: t("edit_note", {
+        noteTitle: e.target.value,
+        extra: "[content]",
+      }),
+    });
   };
 
   const edit = () => {
@@ -195,21 +191,17 @@ export default function Note({ data, onPointerDown, onContextMenu }) {
                           }
                           onBlur={(e) => {
                             if (e.target.value === editField.title) return;
-                            setUndoStack((prev) => [
-                              ...prev,
-                              {
-                                action: Action.EDIT,
-                                element: ObjectType.NOTE,
-                                nid: data.id,
-                                undo: editField,
-                                redo: { title: e.target.value },
-                                message: t("edit_note", {
-                                  noteTitle: e.target.value,
-                                  extra: "[title]",
-                                }),
-                              },
-                            ]);
-                            setRedoStack([]);
+                            pushUndo({
+                              action: Action.EDIT,
+                              element: ObjectType.NOTE,
+                              nid: data.id,
+                              undo: editField,
+                              redo: { title: e.target.value },
+                              message: t("edit_note", {
+                                noteTitle: e.target.value,
+                                extra: "[title]",
+                              }),
+                            });
                           }}
                         />
                         <Popover
@@ -226,21 +218,17 @@ export default function Note({ data, onPointerDown, onContextMenu }) {
                                     style={{ backgroundColor: c }}
                                     className="p-3 rounded-full mx-1"
                                     onClick={() => {
-                                      setUndoStack((prev) => [
-                                        ...prev,
-                                        {
-                                          action: Action.EDIT,
-                                          element: ObjectType.NOTE,
-                                          nid: data.id,
-                                          undo: { color: data.color },
-                                          redo: { color: c },
-                                          message: t("edit_note", {
-                                            noteTitle: data.title,
-                                            extra: "[color]",
-                                          }),
-                                        },
-                                      ]);
-                                      setRedoStack([]);
+                                      pushUndo({
+                                        action: Action.EDIT,
+                                        element: ObjectType.NOTE,
+                                        nid: data.id,
+                                        undo: { color: data.color },
+                                        redo: { color: c },
+                                        message: t("edit_note", {
+                                          noteTitle: data.title,
+                                          extra: "[color]",
+                                        }),
+                                      });
                                       updateNote(data.id, { color: c });
                                     }}
                                   >