Whiteboard: selection-toolbar property edits bypass history, double-fire onUpdate, and batchDraw inconsistently #184

New issue

Open

opened 2026-05-13 12:01:23 +00:00 by AhmedHanafy725 · 1 comment

AhmedHanafy725 commented

2026-05-13 12:01:23 +00:00

Member

A. Property edits bypass history

Every picker handler in static/web/js/whiteboard/selection_toolbar.js — sticky color / font size / text color / alignment, text font/size/color/align, shape stroke/fill/strokeWidth/text-color/font-size/align, drawing stroke/width, frame title, kanban title / column color / add column / add card, mindmap title / direction / tree color, group name, webframe URL — calls WhiteboardSync.onUpdate(node) directly with no WhiteboardHistory.snapshotBefore + commitUpdate. Ctrl+Z does nothing for property changes.

B. Double `onUpdate` per mutation

Paths that call rerenderSticky/Text/Shape/Document also call WhiteboardSync.onUpdate(node) themselves. The rerender helpers already call onUpdate internally — every slider tick fires two RPCs.

C. Inconsistent `batchDraw()`

Some handlers batchDraw directly; some delegate via rerenderXxx; some omit both. Pattern is muddled.

Approach

Add a persistToolbarMutation(node) helper (commitUpdate + onUpdate). Two patterns:

A. uses rerenderXxx: snapshotBefore → mutate → rerenderXxx (batchDraw + onUpdate baked in) → commitUpdate.
B. manual: snapshotBefore → mutate → batchDraw → persistToolbarMutation.

Drop duplicate WhiteboardSync.onUpdate from rerenderXxx paths.

Acceptance

All picker handlers push one undo step per change.
onUpdate fires once per slider tick / picker click.
batchDraw semantics consistent (one call per handler).
Cancelled / same-value picks push nothing.
No regression to widgets already history-aware (kanban #182, mindmap/calendar/webframe #183).

### A. Property edits bypass history Every picker handler in `static/web/js/whiteboard/selection_toolbar.js` — sticky color / font size / text color / alignment, text font/size/color/align, shape stroke/fill/strokeWidth/text-color/font-size/align, drawing stroke/width, frame title, kanban title / column color / add column / add card, mindmap title / direction / tree color, group name, webframe URL — calls `WhiteboardSync.onUpdate(node)` directly with no `WhiteboardHistory.snapshotBefore` + `commitUpdate`. Ctrl+Z does nothing for property changes. ### B. Double `onUpdate` per mutation Paths that call `rerenderSticky/Text/Shape/Document` also call `WhiteboardSync.onUpdate(node)` themselves. The rerender helpers already call `onUpdate` internally — every slider tick fires two RPCs. ### C. Inconsistent `batchDraw()` Some handlers `batchDraw` directly; some delegate via `rerenderXxx`; some omit both. Pattern is muddled. ## Approach Add a `persistToolbarMutation(node)` helper (`commitUpdate + onUpdate`). Two patterns: - **A. uses `rerenderXxx`:** `snapshotBefore` → mutate → `rerenderXxx` (batchDraw + onUpdate baked in) → `commitUpdate`. - **B. manual:** `snapshotBefore` → mutate → `batchDraw` → `persistToolbarMutation`. Drop duplicate `WhiteboardSync.onUpdate` from rerenderXxx paths. ## Acceptance - [ ] All picker handlers push one undo step per change. - [ ] `onUpdate` fires once per slider tick / picker click. - [ ] `batchDraw` semantics consistent (one call per handler). - [ ] Cancelled / same-value picks push nothing. - [ ] No regression to widgets already history-aware (kanban #182, mindmap/calendar/webframe #183).

AhmedHanafy725 commented

2026-05-13 12:04:59 +00:00

Author

Member

Test Results + Final Summary

Changes

static/web/js/whiteboard/selection_toolbar.js:

Added persistToolbarMutation(node) helper (commitUpdate + onUpdate).
Wrapped every property-panel picker handler with snapshotBefore + either persistToolbarMutation (Pattern B: manual mutation + explicit batchDraw) or commitUpdate alone (Pattern A: paths that use WhiteboardObjects.rerenderXxx, which already emit onUpdate and batchDraw internally).
Dropped every duplicate WhiteboardSync.onUpdate(node) in Pattern A paths.
Webframe URL editor now calls WhiteboardWebframe.applyNewUrl(node, v) instead of updateUrl, so it inherits the history bracket added in #183 and the embeddable-check path.

Handlers wrapped

Sticky: color, font size, text color, alignment (4 sites).
Text: font family, font size, text color, alignment (4 sites).
Shape: stroke color, fill color, stroke width, text color, font size, alignment (6 sites).
Drawing: stroke color, stroke width (2 sites).
Frame: title editor (1 site).
Document: font family, font size, text color, bg color, border color, alignment (6 sites).
Kanban: title editor, per-column color popup, add column (3 sites).
Mindmap: title editor, direction select, tree color (3 sites).
Group: name editor (1 site).
Webframe: URL editor (delegates to applyNewUrl for history).

Left intentionally alone

Calendar view-mode select / Today button (already had explicit snapshotBefore + commitUpdate + onUpdate from #183 — equivalent to the new helper).
Calendar prev/next chevrons (delegate to WhiteboardCalendar.navigatePrev/Next which handle their own history).
Webframe Reload button (refresh action with no state change; no history entry needed).
Frame move-slide-up/down chevrons (delegate to WhiteboardFrames.moveFrameUp/Down — separate audit item #14).
Kanban Add Card button (delegates to WhiteboardKanban.addCardToFirstColumn — history from #182).
Mindmap Add Child / Expand/Collapse buttons (delegate to WhiteboardMindmap functions — history from #183).
Ungroup button (out of scope — audit #6).
Edit-text pencil buttons (just trigger inline editor; the editor itself doesn't mutate until commit).

Behaviour after fix

Every property picker is one undo step.
Cancelled / same-value picks push nothing (commitUpdate no-ops on identical before/after).
onUpdate fires exactly once per slider tick / picker click.
batchDraw semantics are consistent: either rerenderXxx handles it, or the manual handler calls it once before persistToolbarMutation.

Gates

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

Manual verification still required

Rebuild + restart hero_whiteboard_admin, hard-reload. For each object type, select it on a board and walk through every picker; confirm Ctrl+Z reverses each change.

## Test Results + Final Summary ### Changes `static/web/js/whiteboard/selection_toolbar.js`: - Added `persistToolbarMutation(node)` helper (`commitUpdate + onUpdate`). - Wrapped every property-panel picker handler with `snapshotBefore` + either `persistToolbarMutation` (Pattern B: manual mutation + explicit batchDraw) or `commitUpdate` alone (Pattern A: paths that use `WhiteboardObjects.rerenderXxx`, which already emit `onUpdate` and `batchDraw` internally). - Dropped every duplicate `WhiteboardSync.onUpdate(node)` in Pattern A paths. - Webframe URL editor now calls `WhiteboardWebframe.applyNewUrl(node, v)` instead of `updateUrl`, so it inherits the history bracket added in #183 and the embeddable-check path. ### Handlers wrapped - Sticky: color, font size, text color, alignment (4 sites). - Text: font family, font size, text color, alignment (4 sites). - Shape: stroke color, fill color, stroke width, text color, font size, alignment (6 sites). - Drawing: stroke color, stroke width (2 sites). - Frame: title editor (1 site). - Document: font family, font size, text color, bg color, border color, alignment (6 sites). - Kanban: title editor, per-column color popup, add column (3 sites). - Mindmap: title editor, direction select, tree color (3 sites). - Group: name editor (1 site). - Webframe: URL editor (delegates to `applyNewUrl` for history). ### Left intentionally alone - Calendar view-mode select / Today button (already had explicit `snapshotBefore` + `commitUpdate` + `onUpdate` from #183 — equivalent to the new helper). - Calendar prev/next chevrons (delegate to `WhiteboardCalendar.navigatePrev/Next` which handle their own history). - Webframe Reload button (refresh action with no state change; no history entry needed). - Frame move-slide-up/down chevrons (delegate to `WhiteboardFrames.moveFrameUp/Down` — separate audit item #14). - Kanban Add Card button (delegates to `WhiteboardKanban.addCardToFirstColumn` — history from #182). - Mindmap Add Child / Expand/Collapse buttons (delegate to `WhiteboardMindmap` functions — history from #183). - Ungroup button (out of scope — audit #6). - Edit-text pencil buttons (just trigger inline editor; the editor itself doesn't mutate until commit). ### Behaviour after fix - Every property picker is one undo step. - Cancelled / same-value picks push nothing (`commitUpdate` no-ops on identical before/after). - `onUpdate` fires exactly once per slider tick / picker click. - `batchDraw` semantics are consistent: either `rerenderXxx` handles it, or the manual handler calls it once before `persistToolbarMutation`. ### Gates - `node -c selection_toolbar.js` — JS syntax OK - `cargo fmt --check` — pass - `cargo clippy --workspace --all-targets -- -D warnings` — pass - `cargo test --workspace --lib` — 0 tests, 0 failures ### Manual verification still required Rebuild + restart `hero_whiteboard_admin`, hard-reload. For each object type, select it on a board and walk through every picker; confirm Ctrl+Z reverses each change.