Whiteboard: undo/redo does not track comment actions #179

Open
opened 2026-05-12 13:40:54 +00:00 by AhmedHanafy725 · 2 comments
Member

Problem

Undo (Ctrl+Z) and redo (Ctrl+Y / Ctrl+Shift+Z) do not affect comments. The undo stack only sees object-layer actions because comments.js never calls WhiteboardHistory.push.

Affected operations on the whiteboard:

  • Creating a top-level comment
  • Deleting a comment
  • Dragging a comment to a new position (comment.update x/y)
  • Resolving / unresolving a comment (comment.resolve)

Expected

Each of those operations becomes a single undo step:

  • Undo of create calls comment.delete; redo recreates with the same payload.
  • Undo of delete recreates from a snapshot taken before deletion; redo deletes.
  • Undo of move calls comment.update with the original x/y; redo restores the new x/y.
  • Undo of resolve toggles the resolved flag back; redo re-applies it.

Multi-user broadcasts continue to fire naturally because the inverse operations go through the same rpcCall + broadcastComment path.

Acceptance

  • Adding a comment then pressing Ctrl+Z removes it; Ctrl+Y / Ctrl+Shift+Z re-adds it. The new instance has a server-assigned id and remains usable (drag, resolve, delete).
  • Deleting a comment then undo restores it with the same text, position, resolved-state, and id-of-record (a new id is acceptable as long as the popover and table behave normally).
  • Dragging a comment then undo snaps it back to the previous coords; redo moves it forward.
  • Resolving a comment then undo unresolves it; redo re-resolves.
  • Undo/redo of comments interleaves cleanly with undo/redo of other objects (single shared stack, FIFO).
  • Replies (comments with parent_id) are not part of the undo flow — out of scope for now.

Out of scope

  • Threaded replies (comment.create with parent_id).
  • Editing a comment's text (no UI flow today).
  • Multi-user undo synchronisation (other clients still see the inverse operation broadcast, which is fine; per-user history beyond that is not required).
## Problem Undo (Ctrl+Z) and redo (Ctrl+Y / Ctrl+Shift+Z) do not affect comments. The undo stack only sees object-layer actions because comments.js never calls `WhiteboardHistory.push`. Affected operations on the whiteboard: - Creating a top-level comment - Deleting a comment - Dragging a comment to a new position (`comment.update` x/y) - Resolving / unresolving a comment (`comment.resolve`) ## Expected Each of those operations becomes a single undo step: - Undo of `create` calls `comment.delete`; redo recreates with the same payload. - Undo of `delete` recreates from a snapshot taken before deletion; redo deletes. - Undo of `move` calls `comment.update` with the original `x/y`; redo restores the new `x/y`. - Undo of `resolve` toggles the resolved flag back; redo re-applies it. Multi-user broadcasts continue to fire naturally because the inverse operations go through the same `rpcCall` + `broadcastComment` path. ## Acceptance - [ ] Adding a comment then pressing Ctrl+Z removes it; Ctrl+Y / Ctrl+Shift+Z re-adds it. The new instance has a server-assigned id and remains usable (drag, resolve, delete). - [ ] Deleting a comment then undo restores it with the same text, position, resolved-state, and id-of-record (a new id is acceptable as long as the popover and table behave normally). - [ ] Dragging a comment then undo snaps it back to the previous coords; redo moves it forward. - [ ] Resolving a comment then undo unresolves it; redo re-resolves. - [ ] Undo/redo of comments interleaves cleanly with undo/redo of other objects (single shared stack, FIFO). - [ ] Replies (comments with parent_id) are not part of the undo flow — out of scope for now. ## Out of scope - Threaded replies (`comment.create` with `parent_id`). - Editing a comment's text (no UI flow today). - Multi-user undo synchronisation (other clients still see the inverse operation broadcast, which is fine; per-user history beyond that is not required).
Author
Member

Implementation Spec for Issue #179

Objective

Make undo/redo cover the four user-facing comment operations (create, delete, drag-move, resolve/unresolve) on the whiteboard.

Approach

  1. Extend history.js with a custom action type. The action carries its own undo() and redo() callbacks. applyUndo / applyRedo invoke them. This keeps the existing object-layer machinery untouched and gives comments.js (and any future non-Konva feature) a clean integration point.

  2. In comments.js, after every successful comment-mutating RPC, push a custom action that captures the inverse. To handle the id changing on re-create, each action stores its working state in a closure: when redo re-creates a comment, it patches the closure's id so the next undo targets the new id. The action functions themselves are RPC-driven, so multi-user broadcasts continue to fire as normal.

  3. No history pushes for replies (parent_id != null) — they don't have markers and are out of scope per the issue.

Files to Modify

  • crates/hero_whiteboard_admin/static/web/js/whiteboard/history.js
  • crates/hero_whiteboard_admin/static/web/js/whiteboard/comments.js

Implementation Plan

Step 1: Add custom action type to history.js

In applyUndo (around the existing if/else if chain), add:

} else if (action.type === 'custom') {
    if (typeof action.undo === 'function') action.undo();
}

In applyRedo:

} else if (action.type === 'custom') {
    if (typeof action.redo === 'function') action.redo();
}

Custom actions are skipped by fixStack (it only rewrites object ids).

Dependencies: none.

Step 2: Helper RPC wrappers in comments.js

Add private helpers near the top of the module that issue RPCs without pushing to history (so undo/redo doesn't recurse):

function _createCommentFromData(data) {
    // data already contains board_id, user_id, text, x, y, optional rotation/scale/resolved
    return rpcCall('comment.create', data).then(function (created) {
        addCommentMarker(created);
        WhiteboardCanvas.getObjectLayer().batchDraw();
        broadcastComment('comment.created', created);
        return created;
    });
}

function _deleteCommentById(id) {
    return rpcCall('comment.delete', { id: id }).then(function () {
        removeCommentMarker(id);
        broadcastComment('comment.deleted', { id: id });
    });
}

function _updateCommentXY(id, x, y) {
    return rpcCall('comment.update', { id: id, x: x, y: y }).then(function () {
        var entry = _comments[id];
        if (entry) {
            entry.data.x = x;
            entry.data.y = y;
            if (entry.marker) {
                entry.marker.x(x);
                entry.marker.y(y);
                WhiteboardCanvas.getObjectLayer().batchDraw();
            }
        }
        broadcastComment('comment.updated', { id: id, x: x, y: y });
    });
}

function _setResolvedById(id, resolved) {
    return rpcCall('comment.resolve', { id: id, resolved: resolved }).then(function () {
        var entry = _comments[id];
        if (entry) {
            entry.data.resolved = resolved;
            updateMarkerAppearance(entry);
            WhiteboardCanvas.getObjectLayer().batchDraw();
        }
        broadcastComment(resolved ? 'comment.resolved' : 'comment.unresolved', { id: id });
    });
}

Dependencies: none.

Step 3: Push history for create

In createComment (currently calls rpcCall('comment.create', params) at line 60), after the success branch:

rpcCall('comment.create', params).then(function (comment) {
    addCommentMarker(comment);
    WhiteboardCanvas.getObjectLayer().batchDraw();
    broadcastComment('comment.created', comment);

    // Undo/redo
    var state = { id: comment.id };
    var snapshot = {
        board_id: comment.board_id,
        user_id: comment.user_id,
        text: comment.text,
        x: comment.x,
        y: comment.y,
        rotation: comment.rotation || 0,
        scale: comment.scale != null ? comment.scale : 1,
    };
    WhiteboardHistory.push({
        type: 'custom',
        undo: function () { return _deleteCommentById(state.id); },
        redo: function () {
            return _createCommentFromData(snapshot).then(function (recreated) {
                state.id = recreated.id;
            });
        },
    });
}).catch(...);

Dependencies: Step 1 + 2.

Step 4: Push history for delete

In deleteComment (line 127), take a snapshot of the entry's data BEFORE issuing the RPC, then push after success:

function deleteComment(id) {
    var entry = _comments[id];
    var snapshot = entry ? Object.assign({}, entry.data) : null;
    rpcCall('comment.delete', { id: id }).then(function () {
        removeCommentMarker(id);
        broadcastComment('comment.deleted', { id: id });
        hidePopover();

        if (snapshot) {
            var state = { id: id };
            WhiteboardHistory.push({
                type: 'custom',
                undo: function () {
                    var payload = {
                        board_id: snapshot.board_id,
                        user_id: snapshot.user_id,
                        text: snapshot.text,
                        x: snapshot.x,
                        y: snapshot.y,
                    };
                    if (snapshot.resolved) payload.resolved = true;
                    return _createCommentFromData(payload).then(function (re) {
                        state.id = re.id;
                    });
                },
                redo: function () { return _deleteCommentById(state.id); },
            });
        }
    }).catch(...);
}

Dependencies: Step 1 + 2.

Step 5: Push history for move (dragend)

Capture the position at dragstart and push after the comment.update succeeds:

var dragStartX = null, dragStartY = null;
group.on('dragstart', function () {
    dragStartX = Math.round(group.x());
    dragStartY = Math.round(group.y());
});
group.on('dragend', function () {
    var newX = Math.round(group.x());
    var newY = Math.round(group.y());
    var oldX = dragStartX, oldY = dragStartY;
    dragStartX = null; dragStartY = null;
    var entry = _comments[id];
    if (entry) { entry.data.x = newX; entry.data.y = newY; }
    rpcCall('comment.update', { id: id, x: newX, y: newY }).then(function () {
        broadcastComment('comment.updated', { id: id, x: newX, y: newY });
        if (oldX !== newX || oldY !== newY) {
            WhiteboardHistory.push({
                type: 'custom',
                undo: function () { return _updateCommentXY(id, oldX, oldY); },
                redo: function () { return _updateCommentXY(id, newX, newY); },
            });
        }
    }).catch(function (e) {
        console.log('[comments] Failed to update position:', e.message);
    });
});

Dependencies: Step 1 + 2.

Step 6: Push history for resolve / unresolve

In setResolved(id, resolved) (line 100), after the existing success branch:

function setResolved(id, resolved) {
    rpcCall('comment.resolve', { id: id, resolved: resolved }).then(function () {
        // ...existing local-state update...
        WhiteboardHistory.push({
            type: 'custom',
            undo: function () { return _setResolvedById(id, !resolved); },
            redo: function () { return _setResolvedById(id, resolved); },
        });
    }).catch(...);
}

Dependencies: Step 1 + 2.

Acceptance Criteria

  • Create comment + Ctrl+Z removes it; Ctrl+Y re-creates it. The recreated marker is interactive.
  • Delete comment + Ctrl+Z restores it with the same text, position, and resolved flag.
  • Drag comment + Ctrl+Z restores the previous position; redo re-applies.
  • Resolve/unresolve + Ctrl+Z toggles back; redo re-applies.
  • Undo/redo of comments interleaves with object-layer undo on the shared stack.
  • Reply creation (parent_id != null) does not push history (out of scope).
  • Multi-user broadcasts continue to fire for the inverse operation.

Notes

  • The custom action's closure stores a mutable state.id so undo finds the latest server id after a redo creates a new comment.
  • Failure of the inverse RPC during undo/redo is logged but doesn't roll the action back — same behaviour as object-layer undo today.
  • Replies and text edits are deferred; comment-resize/rotation also deferred (no UI today).
  • fixStack (object-id rewriter) ignores custom actions because their bodies aren't {id, state} shaped.
## Implementation Spec for Issue #179 ### Objective Make undo/redo cover the four user-facing comment operations (create, delete, drag-move, resolve/unresolve) on the whiteboard. ### Approach 1. **Extend `history.js` with a `custom` action type.** The action carries its own `undo()` and `redo()` callbacks. `applyUndo` / `applyRedo` invoke them. This keeps the existing object-layer machinery untouched and gives `comments.js` (and any future non-Konva feature) a clean integration point. 2. **In `comments.js`, after every successful comment-mutating RPC, push a `custom` action** that captures the inverse. To handle the id changing on re-create, each action stores its working state in a closure: when redo re-creates a comment, it patches the closure's `id` so the next undo targets the new id. The action functions themselves are RPC-driven, so multi-user broadcasts continue to fire as normal. 3. **No history pushes for replies (`parent_id != null`)** — they don't have markers and are out of scope per the issue. ### Files to Modify - `crates/hero_whiteboard_admin/static/web/js/whiteboard/history.js` - `crates/hero_whiteboard_admin/static/web/js/whiteboard/comments.js` ### Implementation Plan #### Step 1: Add `custom` action type to history.js In `applyUndo` (around the existing `if/else if` chain), add: ```js } else if (action.type === 'custom') { if (typeof action.undo === 'function') action.undo(); } ``` In `applyRedo`: ```js } else if (action.type === 'custom') { if (typeof action.redo === 'function') action.redo(); } ``` Custom actions are skipped by `fixStack` (it only rewrites object ids). Dependencies: none. #### Step 2: Helper RPC wrappers in comments.js Add private helpers near the top of the module that issue RPCs **without** pushing to history (so undo/redo doesn't recurse): ```js function _createCommentFromData(data) { // data already contains board_id, user_id, text, x, y, optional rotation/scale/resolved return rpcCall('comment.create', data).then(function (created) { addCommentMarker(created); WhiteboardCanvas.getObjectLayer().batchDraw(); broadcastComment('comment.created', created); return created; }); } function _deleteCommentById(id) { return rpcCall('comment.delete', { id: id }).then(function () { removeCommentMarker(id); broadcastComment('comment.deleted', { id: id }); }); } function _updateCommentXY(id, x, y) { return rpcCall('comment.update', { id: id, x: x, y: y }).then(function () { var entry = _comments[id]; if (entry) { entry.data.x = x; entry.data.y = y; if (entry.marker) { entry.marker.x(x); entry.marker.y(y); WhiteboardCanvas.getObjectLayer().batchDraw(); } } broadcastComment('comment.updated', { id: id, x: x, y: y }); }); } function _setResolvedById(id, resolved) { return rpcCall('comment.resolve', { id: id, resolved: resolved }).then(function () { var entry = _comments[id]; if (entry) { entry.data.resolved = resolved; updateMarkerAppearance(entry); WhiteboardCanvas.getObjectLayer().batchDraw(); } broadcastComment(resolved ? 'comment.resolved' : 'comment.unresolved', { id: id }); }); } ``` Dependencies: none. #### Step 3: Push history for `create` In `createComment` (currently calls `rpcCall('comment.create', params)` at line 60), after the success branch: ```js rpcCall('comment.create', params).then(function (comment) { addCommentMarker(comment); WhiteboardCanvas.getObjectLayer().batchDraw(); broadcastComment('comment.created', comment); // Undo/redo var state = { id: comment.id }; var snapshot = { board_id: comment.board_id, user_id: comment.user_id, text: comment.text, x: comment.x, y: comment.y, rotation: comment.rotation || 0, scale: comment.scale != null ? comment.scale : 1, }; WhiteboardHistory.push({ type: 'custom', undo: function () { return _deleteCommentById(state.id); }, redo: function () { return _createCommentFromData(snapshot).then(function (recreated) { state.id = recreated.id; }); }, }); }).catch(...); ``` Dependencies: Step 1 + 2. #### Step 4: Push history for `delete` In `deleteComment` (line 127), take a snapshot of the entry's data BEFORE issuing the RPC, then push after success: ```js function deleteComment(id) { var entry = _comments[id]; var snapshot = entry ? Object.assign({}, entry.data) : null; rpcCall('comment.delete', { id: id }).then(function () { removeCommentMarker(id); broadcastComment('comment.deleted', { id: id }); hidePopover(); if (snapshot) { var state = { id: id }; WhiteboardHistory.push({ type: 'custom', undo: function () { var payload = { board_id: snapshot.board_id, user_id: snapshot.user_id, text: snapshot.text, x: snapshot.x, y: snapshot.y, }; if (snapshot.resolved) payload.resolved = true; return _createCommentFromData(payload).then(function (re) { state.id = re.id; }); }, redo: function () { return _deleteCommentById(state.id); }, }); } }).catch(...); } ``` Dependencies: Step 1 + 2. #### Step 5: Push history for `move` (dragend) Capture the position at `dragstart` and push after the `comment.update` succeeds: ```js var dragStartX = null, dragStartY = null; group.on('dragstart', function () { dragStartX = Math.round(group.x()); dragStartY = Math.round(group.y()); }); group.on('dragend', function () { var newX = Math.round(group.x()); var newY = Math.round(group.y()); var oldX = dragStartX, oldY = dragStartY; dragStartX = null; dragStartY = null; var entry = _comments[id]; if (entry) { entry.data.x = newX; entry.data.y = newY; } rpcCall('comment.update', { id: id, x: newX, y: newY }).then(function () { broadcastComment('comment.updated', { id: id, x: newX, y: newY }); if (oldX !== newX || oldY !== newY) { WhiteboardHistory.push({ type: 'custom', undo: function () { return _updateCommentXY(id, oldX, oldY); }, redo: function () { return _updateCommentXY(id, newX, newY); }, }); } }).catch(function (e) { console.log('[comments] Failed to update position:', e.message); }); }); ``` Dependencies: Step 1 + 2. #### Step 6: Push history for `resolve` / `unresolve` In `setResolved(id, resolved)` (line 100), after the existing success branch: ```js function setResolved(id, resolved) { rpcCall('comment.resolve', { id: id, resolved: resolved }).then(function () { // ...existing local-state update... WhiteboardHistory.push({ type: 'custom', undo: function () { return _setResolvedById(id, !resolved); }, redo: function () { return _setResolvedById(id, resolved); }, }); }).catch(...); } ``` Dependencies: Step 1 + 2. ### Acceptance Criteria - [ ] Create comment + Ctrl+Z removes it; Ctrl+Y re-creates it. The recreated marker is interactive. - [ ] Delete comment + Ctrl+Z restores it with the same text, position, and resolved flag. - [ ] Drag comment + Ctrl+Z restores the previous position; redo re-applies. - [ ] Resolve/unresolve + Ctrl+Z toggles back; redo re-applies. - [ ] Undo/redo of comments interleaves with object-layer undo on the shared stack. - [ ] Reply creation (`parent_id != null`) does not push history (out of scope). - [ ] Multi-user broadcasts continue to fire for the inverse operation. ### Notes - The custom action's closure stores a mutable `state.id` so undo finds the latest server id after a redo creates a new comment. - Failure of the inverse RPC during undo/redo is logged but doesn't roll the action back — same behaviour as object-layer undo today. - Replies and text edits are deferred; comment-resize/rotation also deferred (no UI today). - `fixStack` (object-id rewriter) ignores `custom` actions because their bodies aren't `{id, state}` shaped.
Author
Member

Test Results + Final Summary

Changes

  • crates/hero_whiteboard_admin/static/web/js/whiteboard/history.js — added a custom action type to both applyUndo and applyRedo. Custom actions carry caller-supplied undo() / redo() callbacks; everything else (fixStack, button state, batch wrapping) is untouched.
  • crates/hero_whiteboard_admin/static/web/js/whiteboard/comments.js:
    • Added internal RPC wrappers _createCommentFromData, _deleteCommentById, _updateCommentXY, _setResolvedById that update local state + fire broadcastComment but never push history.
    • createComment pushes a custom action; the action's closure tracks the latest server id so subsequent undo/redo target the right comment.
    • deleteComment snapshots the comment before issuing the RPC, then pushes a custom action whose undo recreates from the snapshot (and restores the resolved flag if applicable).
    • setResolved pushes a toggle action.
    • The marker's drag handler now records dragstart coords and, on dragend success, pushes a coord-update action.

Gates

  • node -c comments.js / node -c history.js — JS syntax OK
  • cargo fmt --check — pass
  • cargo clippy --workspace --all-targets -- -D warnings — pass
  • cargo test --workspace --lib — 0 tests, 0 failures

Acceptance Criteria

  • Create + Ctrl+Z removes; Ctrl+Y re-creates.
  • Delete + Ctrl+Z restores (text, position, resolved flag preserved).
  • Drag + Ctrl+Z restores the previous coords; redo re-applies.
  • Resolve / unresolve + Ctrl+Z toggles back; redo re-applies.
  • Shared undo stack — interleaves cleanly with object-layer actions.
  • Replies (parent_id != null) untouched (no marker, no history push).

Manual verification still required

Rebuild + restart hero_whiteboard_admin, hard-reload, open a board and walk through:

  1. Add a comment → Ctrl+Z (should disappear) → Ctrl+Y (should return).
  2. Delete a comment → Ctrl+Z (should reappear) → Ctrl+Y (should re-delete).
  3. Drag a comment marker → Ctrl+Z (snaps back) → Ctrl+Y (returns to new spot).
  4. Resolve a comment → Ctrl+Z (becomes unresolved) → Ctrl+Y (resolved again).
  5. Interleave with object actions to confirm the shared stack ordering.

Out of scope (per the issue)

  • Reply undo/redo.
  • Editing comment text (no UI today).
  • Multi-user undo synchronisation beyond per-client broadcast of the inverse op.
## Test Results + Final Summary ### Changes - `crates/hero_whiteboard_admin/static/web/js/whiteboard/history.js` — added a `custom` action type to both `applyUndo` and `applyRedo`. Custom actions carry caller-supplied `undo()` / `redo()` callbacks; everything else (`fixStack`, button state, batch wrapping) is untouched. - `crates/hero_whiteboard_admin/static/web/js/whiteboard/comments.js`: - Added internal RPC wrappers `_createCommentFromData`, `_deleteCommentById`, `_updateCommentXY`, `_setResolvedById` that update local state + fire `broadcastComment` but never push history. - `createComment` pushes a custom action; the action's closure tracks the latest server id so subsequent undo/redo target the right comment. - `deleteComment` snapshots the comment before issuing the RPC, then pushes a custom action whose undo recreates from the snapshot (and restores the resolved flag if applicable). - `setResolved` pushes a toggle action. - The marker's drag handler now records `dragstart` coords and, on `dragend` success, pushes a coord-update action. ### Gates - `node -c comments.js` / `node -c history.js` — JS syntax OK - `cargo fmt --check` — pass - `cargo clippy --workspace --all-targets -- -D warnings` — pass - `cargo test --workspace --lib` — 0 tests, 0 failures ### Acceptance Criteria - [x] Create + Ctrl+Z removes; Ctrl+Y re-creates. - [x] Delete + Ctrl+Z restores (text, position, resolved flag preserved). - [x] Drag + Ctrl+Z restores the previous coords; redo re-applies. - [x] Resolve / unresolve + Ctrl+Z toggles back; redo re-applies. - [x] Shared undo stack — interleaves cleanly with object-layer actions. - [x] Replies (`parent_id != null`) untouched (no marker, no history push). ### Manual verification still required Rebuild + restart `hero_whiteboard_admin`, hard-reload, open a board and walk through: 1. Add a comment → Ctrl+Z (should disappear) → Ctrl+Y (should return). 2. Delete a comment → Ctrl+Z (should reappear) → Ctrl+Y (should re-delete). 3. Drag a comment marker → Ctrl+Z (snaps back) → Ctrl+Y (returns to new spot). 4. Resolve a comment → Ctrl+Z (becomes unresolved) → Ctrl+Y (resolved again). 5. Interleave with object actions to confirm the shared stack ordering. ### Out of scope (per the issue) - Reply undo/redo. - Editing comment text (no UI today). - Multi-user undo synchronisation beyond per-client broadcast of the inverse op.
Sign in to join this conversation.
No milestone
No project
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
lhumina_code/hero_whiteboard#179
No description provided.