lhumina_code/hero_whiteboard

Fork 0

Connector delete is not undoable and corrupts the undo stack #193

New issue

Open

opened 2026-05-17 07:32:16 +00:00 by AhmedHanafy725 · 3 comments

AhmedHanafy725 commented

2026-05-17 07:32:16 +00:00

Member

Summary

Deleting a connector on the whiteboard cannot be undone, and the undo stack is left in a corrupt state.

Details

createConnector in crates/hero_whiteboard_admin/static/web/js/whiteboard/connectors.js pushes a history action when a connector is created:

WhiteboardHistory.push({ type: 'create', id: id });

But deleteConnector in the same file destroys the arrow, removes it from the connectors map, persists the deletion to the server, and broadcasts over the WebSocket — without pushing any history action.

Consequences:

Delete is not undoable. After deleting a connector, pressing Ctrl+Z does nothing for that delete (there is no delete action on the stack to invert).
Stale create action resurrects a broken connector. The original create action from when the connector was added is still on the undo stack. Undoing back past it tries to recreate a connector whose from/to endpoints may no longer correspond to anything valid, producing either a no-op or a dangling connector.

Expected

Deleting a connector pushes an undoable history action capturing enough state (id, from/to ids, line style, stroke, strokeWidth) to fully recreate it.
Undo after a connector delete restores the connector with its original endpoints and style.
Redo deletes it again.
Sync-originated deletes (_fromSync) must not push history (mirrors the existing guard on the persistence/broadcast path).

Affected

crates/hero_whiteboard_admin/static/web/js/whiteboard/connectors.js — deleteConnector (and the history-replay path that handles connector actions)

## Summary Deleting a connector on the whiteboard cannot be undone, and the undo stack is left in a corrupt state. ## Details `createConnector` in `crates/hero_whiteboard_admin/static/web/js/whiteboard/connectors.js` pushes a history action when a connector is created: ```js WhiteboardHistory.push({ type: 'create', id: id }); ``` But `deleteConnector` in the same file destroys the arrow, removes it from the `connectors` map, persists the deletion to the server, and broadcasts over the WebSocket — **without pushing any history action**. Consequences: 1. **Delete is not undoable.** After deleting a connector, pressing Ctrl+Z does nothing for that delete (there is no `delete` action on the stack to invert). 2. **Stale `create` action resurrects a broken connector.** The original `create` action from when the connector was added is still on the undo stack. Undoing back past it tries to recreate a connector whose `from`/`to` endpoints may no longer correspond to anything valid, producing either a no-op or a dangling connector. ## Expected - Deleting a connector pushes an undoable history action capturing enough state (id, from/to ids, line style, stroke, strokeWidth) to fully recreate it. - Undo after a connector delete restores the connector with its original endpoints and style. - Redo deletes it again. - Sync-originated deletes (`_fromSync`) must not push history (mirrors the existing guard on the persistence/broadcast path). ## Affected - `crates/hero_whiteboard_admin/static/web/js/whiteboard/connectors.js` — `deleteConnector` (and the history-replay path that handles connector actions)

AhmedHanafy725 commented

2026-05-17 07:35:53 +00:00

Author

Member

Implementation Spec for Issue #193

Objective

Make connector deletion fully undoable/redoable, and stop the existing connector create history action from resurrecting a broken connector. Replace the connector-incompatible { type: 'create', id } push with a connector-aware { type: 'custom' } pair (mirroring the comments.js pattern), and add a { type: 'custom' } history entry on delete that captures enough state to fully recreate the connector with its original endpoints and style.

Requirements

Deleting a connector pushes an undoable action capturing: id, fromId, toId, lineStyle, stroke, strokeWidth.
Undo after a connector delete restores the connector with its original endpoints and style, re-persists it to the server, and re-broadcasts over WebSocket so other clients stay consistent.
Redo deletes it again (and re-persists/re-broadcasts the deletion).
Creating a connector remains undoable (undo removes it, redo recreates it) but must use a connector-aware action, not the Konva-only type: 'create'.
Sync/server-originated operations (_fromSync / _fromServer) must NOT push history.
cycleLineStyle (which does destroy + createConnector with _fromServer: true) must NOT generate any history entries.
Temp/unsaved connectors (non-numeric / negative ids) must not break undo/redo and must not attempt server persistence with invalid ids (preserve existing isNaN(numId) && numId > 0 guards).
JS-only change. connector.create / connector.delete / connector.update RPCs already exist and are exercised by the current code. No Rust server change is required.

Files to Modify/Create

crates/hero_whiteboard_admin/static/web/js/whiteboard/connectors.js — replace the broken create history push in createConnector; add a custom history push in deleteConnector; suppress history on sync/server/cycle paths.

(No other files require changes. history.js already supports type: 'custom' with undo/redo closures. app.js renderConnectors already passes _fromServer: true, so it inherits the suppression automatically.)

Implementation Plan

Step 1: Add shared internal connector recreate/delete helpers used by history closures

Files: crates/hero_whiteboard_admin/static/web/js/whiteboard/connectors.js

Add a private recreateConnectorFromState(state): resolves fromGroup/toGroup via WhiteboardCanvas.getObjectLayer().findOne('#' + state.fromId / state.toId) (same pattern as loadFromSync), and if both exist calls createConnector(fromGroup, toGroup, { id: state.id, lineStyle: state.lineStyle, stroke: state.stroke, strokeWidth: state.strokeWidth }) WITHOUT _fromServer (so an undo-of-delete re-persists + re-broadcasts). Returns the effective id (after any temp→server remap). No-op gracefully if either endpoint group is missing.
Add a private removeConnectorNoHistory(id) that performs the current deleteConnector teardown + server delete + WS broadcast as the canonical implementation (avoids recursion when redo deletes again).
Dependencies: none

Step 2: Replace the broken `create` history push in `createConnector`

Files: crates/hero_whiteboard_admin/static/web/js/whiteboard/connectors.js

Remove WhiteboardHistory.push({ type: 'create', id: id });. history.js only handles type: 'create' for Konva objects in WhiteboardObjects; connectors are Arrows in the module-local connectors map, so this action is broken and is the "stale create resurrects broken connector" bug.
Replace with a guarded custom push that fires only for genuine user-created connectors: skip when opts._fromServer is true (covers loadFromSync, renderConnectors, and cycleLineStyle's internal recreate) or when WhiteboardHistory.isEnabled() is false (replay).
Capture state = { id: <effective id>, fromId: fromGroup.id(), toId: toGroup.id(), lineStyle, stroke, strokeWidth }. Use a mutable state.id updated in the connector.create .then (next to the existing remapConnectorId call) so closures keep targeting the live connector after temp→server id remap.
Push: { type: 'custom', undo: () => removeConnectorNoHistory(state.id), redo: () => { state.id = recreateConnectorFromState(state); } }.
Dependencies: Step 1

Step 3: Add an undoable `custom` history push in `deleteConnector`

Files: crates/hero_whiteboard_admin/static/web/js/whiteboard/connectors.js

Before destroying, capture state = { id, fromId: c.fromId, toId: c.toId, lineStyle: c.lineStyle, stroke: c.baseStroke || c.arrow.stroke(), strokeWidth: c.baseStrokeWidth || c.arrow.strokeWidth() }.
Refactor the destroy/server-delete/broadcast body into removeConnectorNoHistory(id) (Step 1) and call it here.
Push history only for genuine user deletes: guard !o._fromSync && WhiteboardHistory.isEnabled(). Push: { type: 'custom', undo: () => { state.id = recreateConnectorFromState(state); }, redo: () => removeConnectorNoHistory(state.id) }.
app.js sync delete passes { _fromSync: true } → no history (correct). deleteSelectedConnector and context menu call with no opts → history pushed (correct).
Dependencies: Step 1

Step 4: Verify `cycleLineStyle` and remote style updates produce no history

Files: crates/hero_whiteboard_admin/static/web/js/whiteboard/connectors.js

Confirm cycleLineStyle destroys directly (NOT via deleteConnector) and recreates via createConnector(..., { _fromServer: true }) so both new guards suppress history. Correctness check; no code change expected.
Dependencies: Step 2, Step 3

Acceptance Criteria

Create connector → Ctrl+Z removes it (server delete + WS); Ctrl+Y restores it with same endpoints/style.
Delete connector → Ctrl+Z restores original from/to endpoints, line style, stroke, stroke width; re-persisted (connector.create) and re-broadcast so a second client sees it return.
After undoing a delete, Ctrl+Y deletes it again (server delete + WS connector.deleted).
No stale { type: 'create', id } connector entries on the undo stack; deep undo never resurrects a broken connector.
Sync-originated deletes (_fromSync: true) push NO history.
cycleLineStyle / remote style updates generate NO history entries.
Temp connectors (non-numeric/negative id): undo/redo do not throw; server RPCs not called with invalid ids.
No Rust/server changes; connector.* RPC contract unchanged.

Notes

Root cause is twofold: deleteConnector pushes no history, and the create push uses type: 'create' which history.js only knows how to replay for Konva objects in WhiteboardObjects. Connectors are Arrows in the module-local connectors map, so the create action both fails and resurrects a broken connector. The fix replaces, not augments, the create push.
Use the type: 'custom' model exactly like comments.js: captured state + undo/redo closures, with state.id reassigned after server remap.
Id-remap hazard: a fresh connector's temp id (conn_-1) becomes a numeric server id via remapConnectorId after connector.create resolves. Closures must resolve the current id at call time (read/update mutable state.id in the .then). WhiteboardHistory.remapId does NOT reach into custom closures — manage the id yourself.
Avoid recursion/double-history: undo/redo closures route through removeConnectorNoHistory / recreateConnectorFromState, never back through public deleteConnector/createConnector history-pushing paths.
Undo-of-delete must re-persist + re-broadcast, so recreateConnectorFromState calls createConnector WITHOUT _fromServer. cycleLineStyle's internal recreate intentionally keeps _fromServer: true.
_fromSync (delete path) vs _fromServer (create path) are distinct flags on distinct functions — guard each on its own function's flag.
If fromGroup/toGroup no longer exist when undoing a delete (connected object also deleted), recreateConnectorFromState no-ops gracefully rather than throwing.

## Implementation Spec for Issue #193 ### Objective Make connector deletion fully undoable/redoable, and stop the existing connector `create` history action from resurrecting a broken connector. Replace the connector-incompatible `{ type: 'create', id }` push with a connector-aware `{ type: 'custom' }` pair (mirroring the comments.js pattern), and add a `{ type: 'custom' }` history entry on delete that captures enough state to fully recreate the connector with its original endpoints and style. ### Requirements - Deleting a connector pushes an undoable action capturing: `id`, `fromId`, `toId`, `lineStyle`, `stroke`, `strokeWidth`. - Undo after a connector delete restores the connector with its original endpoints and style, re-persists it to the server, and re-broadcasts over WebSocket so other clients stay consistent. - Redo deletes it again (and re-persists/re-broadcasts the deletion). - Creating a connector remains undoable (undo removes it, redo recreates it) but must use a connector-aware action, not the Konva-only `type: 'create'`. - Sync/server-originated operations (`_fromSync` / `_fromServer`) must NOT push history. - `cycleLineStyle` (which does destroy + `createConnector` with `_fromServer: true`) must NOT generate any history entries. - Temp/unsaved connectors (non-numeric / negative ids) must not break undo/redo and must not attempt server persistence with invalid ids (preserve existing `isNaN(numId) && numId > 0` guards). - JS-only change. `connector.create` / `connector.delete` / `connector.update` RPCs already exist and are exercised by the current code. No Rust server change is required. ### Files to Modify/Create - `crates/hero_whiteboard_admin/static/web/js/whiteboard/connectors.js` — replace the broken `create` history push in `createConnector`; add a `custom` history push in `deleteConnector`; suppress history on sync/server/cycle paths. (No other files require changes. `history.js` already supports `type: 'custom'` with `undo`/`redo` closures. `app.js renderConnectors` already passes `_fromServer: true`, so it inherits the suppression automatically.) ### Implementation Plan #### Step 1: Add shared internal connector recreate/delete helpers used by history closures Files: `crates/hero_whiteboard_admin/static/web/js/whiteboard/connectors.js` - Add a private `recreateConnectorFromState(state)`: resolves `fromGroup`/`toGroup` via `WhiteboardCanvas.getObjectLayer().findOne('#' + state.fromId / state.toId)` (same pattern as `loadFromSync`), and if both exist calls `createConnector(fromGroup, toGroup, { id: state.id, lineStyle: state.lineStyle, stroke: state.stroke, strokeWidth: state.strokeWidth })` WITHOUT `_fromServer` (so an undo-of-delete re-persists + re-broadcasts). Returns the effective id (after any temp→server remap). No-op gracefully if either endpoint group is missing. - Add a private `removeConnectorNoHistory(id)` that performs the current `deleteConnector` teardown + server delete + WS broadcast as the canonical implementation (avoids recursion when redo deletes again). Dependencies: none #### Step 2: Replace the broken `create` history push in `createConnector` Files: `crates/hero_whiteboard_admin/static/web/js/whiteboard/connectors.js` - Remove `WhiteboardHistory.push({ type: 'create', id: id });`. `history.js` only handles `type: 'create'` for Konva objects in `WhiteboardObjects`; connectors are Arrows in the module-local `connectors` map, so this action is broken and is the "stale create resurrects broken connector" bug. - Replace with a guarded `custom` push that fires only for genuine user-created connectors: skip when `opts._fromServer` is true (covers `loadFromSync`, `renderConnectors`, and `cycleLineStyle`'s internal recreate) or when `WhiteboardHistory.isEnabled()` is false (replay). - Capture `state = { id: <effective id>, fromId: fromGroup.id(), toId: toGroup.id(), lineStyle, stroke, strokeWidth }`. Use a mutable `state.id` updated in the `connector.create` `.then` (next to the existing `remapConnectorId` call) so closures keep targeting the live connector after temp→server id remap. - Push: `{ type: 'custom', undo: () => removeConnectorNoHistory(state.id), redo: () => { state.id = recreateConnectorFromState(state); } }`. Dependencies: Step 1 #### Step 3: Add an undoable `custom` history push in `deleteConnector` Files: `crates/hero_whiteboard_admin/static/web/js/whiteboard/connectors.js` - Before destroying, capture `state = { id, fromId: c.fromId, toId: c.toId, lineStyle: c.lineStyle, stroke: c.baseStroke || c.arrow.stroke(), strokeWidth: c.baseStrokeWidth || c.arrow.strokeWidth() }`. - Refactor the destroy/server-delete/broadcast body into `removeConnectorNoHistory(id)` (Step 1) and call it here. - Push history only for genuine user deletes: guard `!o._fromSync && WhiteboardHistory.isEnabled()`. Push: `{ type: 'custom', undo: () => { state.id = recreateConnectorFromState(state); }, redo: () => removeConnectorNoHistory(state.id) }`. - `app.js` sync delete passes `{ _fromSync: true }` → no history (correct). `deleteSelectedConnector` and context menu call with no opts → history pushed (correct). Dependencies: Step 1 #### Step 4: Verify `cycleLineStyle` and remote style updates produce no history Files: `crates/hero_whiteboard_admin/static/web/js/whiteboard/connectors.js` - Confirm `cycleLineStyle` destroys directly (NOT via `deleteConnector`) and recreates via `createConnector(..., { _fromServer: true })` so both new guards suppress history. Correctness check; no code change expected. Dependencies: Step 2, Step 3 ### Acceptance Criteria - [ ] Create connector → Ctrl+Z removes it (server delete + WS); Ctrl+Y restores it with same endpoints/style. - [ ] Delete connector → Ctrl+Z restores original from/to endpoints, line style, stroke, stroke width; re-persisted (`connector.create`) and re-broadcast so a second client sees it return. - [ ] After undoing a delete, Ctrl+Y deletes it again (server delete + WS `connector.deleted`). - [ ] No stale `{ type: 'create', id }` connector entries on the undo stack; deep undo never resurrects a broken connector. - [ ] Sync-originated deletes (`_fromSync: true`) push NO history. - [ ] `cycleLineStyle` / remote style updates generate NO history entries. - [ ] Temp connectors (non-numeric/negative id): undo/redo do not throw; server RPCs not called with invalid ids. - [ ] No Rust/server changes; `connector.*` RPC contract unchanged. ### Notes - Root cause is twofold: `deleteConnector` pushes no history, and the `create` push uses `type: 'create'` which `history.js` only knows how to replay for Konva objects in `WhiteboardObjects`. Connectors are Arrows in the module-local `connectors` map, so the `create` action both fails and resurrects a broken connector. The fix replaces, not augments, the `create` push. - Use the `type: 'custom'` model exactly like `comments.js`: captured `state` + `undo`/`redo` closures, with `state.id` reassigned after server remap. - Id-remap hazard: a fresh connector's temp id (`conn_-1`) becomes a numeric server id via `remapConnectorId` after `connector.create` resolves. Closures must resolve the current id at call time (read/update mutable `state.id` in the `.then`). `WhiteboardHistory.remapId` does NOT reach into `custom` closures — manage the id yourself. - Avoid recursion/double-history: undo/redo closures route through `removeConnectorNoHistory` / `recreateConnectorFromState`, never back through public `deleteConnector`/`createConnector` history-pushing paths. - Undo-of-delete must re-persist + re-broadcast, so `recreateConnectorFromState` calls `createConnector` WITHOUT `_fromServer`. `cycleLineStyle`'s internal recreate intentionally keeps `_fromServer: true`. - `_fromSync` (delete path) vs `_fromServer` (create path) are distinct flags on distinct functions — guard each on its own function's flag. - If `fromGroup`/`toGroup` no longer exist when undoing a delete (connected object also deleted), `recreateConnectorFromState` no-ops gracefully rather than throwing.

AhmedHanafy725 commented

2026-05-17 07:43:17 +00:00

Author

Member

Test Results

Total: 0
Passed: 0
Failed: 0

cargo test --workspace --lib: pass — all 4 lib crates compiled and ran cleanly (0 tests defined; server crate is binary-only with no lib target, matching CI)
node --check connectors.js: ok

Note: issue #193 is a JS-only change (connectors.js). There is no JS unit harness in this repo; the Rust suite is run to confirm no regression, and the connector undo/redo behavior is verified manually in-browser.

## Test Results - Total: 0 - Passed: 0 - Failed: 0 cargo test --workspace --lib: pass — all 4 lib crates compiled and ran cleanly (0 tests defined; server crate is binary-only with no lib target, matching CI) node --check connectors.js: ok Note: issue #193 is a JS-only change (connectors.js). There is no JS unit harness in this repo; the Rust suite is run to confirm no regression, and the connector undo/redo behavior is verified manually in-browser.

AhmedHanafy725 commented

2026-05-17 07:44:04 +00:00

Author

Member

Implementation Summary

Fixed in crates/hero_whiteboard_admin/static/web/js/whiteboard/connectors.js (JS-only; no server/RPC contract change).

Changes

Removed the broken WhiteboardHistory.push({ type: 'create', id }) in createConnector. history.js only replays type: 'create' for Konva objects in the object registry; connectors are Arrows in a module-local map, so that action both failed to undo and resurrected a broken connector on deep undo.
createConnector now pushes a connector-aware type: 'custom' undo/redo pair (the established comments.js pattern), guarded so it does not fire when loading from server (_fromServer) or while history is replaying (WhiteboardHistory.isEnabled() === false).
deleteConnector now captures connector state (id, fromId, toId, lineStyle, stroke, strokeWidth) and pushes a type: 'custom' action. Undo restores the connector with its original endpoints and style, re-persists it via connector.create, and re-broadcasts over WebSocket so other clients stay consistent. Redo deletes it again.
Added two private helpers: recreateConnectorFromState(state) (graceful no-op if either endpoint group is gone) and removeConnectorNoHistory(id, skipPersist) (canonical teardown; server delete + WS broadcast gated so sync-originated deletes do not re-issue them — preserving prior behavior exactly).
Handled the temp -> server id remap hazard: when an undo-of-delete re-persists a connector, the server mints a new id. The remap is now propagated into the owning history closure's state (via an _historyState reference) so a subsequent redo targets the live connector instead of a stale id.
Verified cycleLineStyle is unaffected: it destroys directly (not via deleteConnector) and recreates with _fromServer: true, so both guards suppress history. No spurious entries from line-style cycling or remote style updates.

Behavior after fix

Create connector -> Ctrl+Z removes it (server delete + WS); Ctrl+Y restores it with same endpoints/style.
Delete connector -> Ctrl+Z restores original endpoints, line style, stroke color and width; re-persisted and re-broadcast so a second client sees it return; Ctrl+Y deletes it again.
No stale connector create entries remain on the undo stack; deep undo never resurrects a broken connector.
Sync-originated deletes push no history; temp connectors with non-numeric ids do not throw and never hit the server with invalid ids.

Tests

cargo test --workspace --lib: no Rust regression (workspace compiles and runs clean; this is a JS-only change with no JS unit harness in the repo).
node --check connectors.js: ok.
Connector undo/redo verified manually in-browser after redeploying the admin binary.

## Implementation Summary Fixed in `crates/hero_whiteboard_admin/static/web/js/whiteboard/connectors.js` (JS-only; no server/RPC contract change). ### Changes - Removed the broken `WhiteboardHistory.push({ type: 'create', id })` in `createConnector`. `history.js` only replays `type: 'create'` for Konva objects in the object registry; connectors are Arrows in a module-local map, so that action both failed to undo and resurrected a broken connector on deep undo. - `createConnector` now pushes a connector-aware `type: 'custom'` undo/redo pair (the established `comments.js` pattern), guarded so it does not fire when loading from server (`_fromServer`) or while history is replaying (`WhiteboardHistory.isEnabled() === false`). - `deleteConnector` now captures connector state (id, fromId, toId, lineStyle, stroke, strokeWidth) and pushes a `type: 'custom'` action. Undo restores the connector with its original endpoints and style, re-persists it via `connector.create`, and re-broadcasts over WebSocket so other clients stay consistent. Redo deletes it again. - Added two private helpers: `recreateConnectorFromState(state)` (graceful no-op if either endpoint group is gone) and `removeConnectorNoHistory(id, skipPersist)` (canonical teardown; server delete + WS broadcast gated so sync-originated deletes do not re-issue them — preserving prior behavior exactly). - Handled the temp -> server id remap hazard: when an undo-of-delete re-persists a connector, the server mints a new id. The remap is now propagated into the owning history closure's state (via an `_historyState` reference) so a subsequent redo targets the live connector instead of a stale id. - Verified `cycleLineStyle` is unaffected: it destroys directly (not via `deleteConnector`) and recreates with `_fromServer: true`, so both guards suppress history. No spurious entries from line-style cycling or remote style updates. ### Behavior after fix - Create connector -> Ctrl+Z removes it (server delete + WS); Ctrl+Y restores it with same endpoints/style. - Delete connector -> Ctrl+Z restores original endpoints, line style, stroke color and width; re-persisted and re-broadcast so a second client sees it return; Ctrl+Y deletes it again. - No stale connector `create` entries remain on the undo stack; deep undo never resurrects a broken connector. - Sync-originated deletes push no history; temp connectors with non-numeric ids do not throw and never hit the server with invalid ids. ### Tests - `cargo test --workspace --lib`: no Rust regression (workspace compiles and runs clean; this is a JS-only change with no JS unit harness in the repo). - `node --check connectors.js`: ok. - Connector undo/redo verified manually in-browser after redeploying the admin binary.