Ctrl+Z after a peer's edit no longer reverts (and re-broadcasts) the peer's work, and the adopt undo-bomb is gone: - doApply/doApplyItems (both editors) Push with SKIP_UNDO; the emit path is unaffected (suppression keys off s_applyingRemote, not undo). - With SKIP_UNDO no picker owns removed items — the bindings free them after Push (explicit removals + upsert's remove-before-re-add; fields excluded: CHT_REMOVE hides them, parent keeps ownership). Freeing stays out of the fork commit classes so DRC's SKIP_UNDO callers can't double-free. - Test hooks kicadCollabTestUndo/UndoDepth, registered per-editor AND in the kicad_editor dispatcher (merged image compiles out per-app registrations). - kicad pointer: eeschema UUID undo guard + SKIP_UNDO connectivity split + quiet stale-entry drop (ca8877324c). - tests/kicad/collab-undo.spec.ts: 5 scenarios (no undo entry from remote applies; selective undo; stranded replaced/deleted entries) — 5/5, plus collab/ysync regression 30 pass. - docs: ysync-review 20 fix record; 09 marked FIXED; overview indexed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019ejJEvS7ogef2o9gVTXjmp
3.7 KiB
Design miss 09 — Undo is not collab-aware: Ctrl+Z reverts peers' work and re-broadcasts it
Severity: design gap (converges, but with surprising and destructive UX) Status: FIXED 2026-07-07 — option 1 implemented, see 20-fix-miss09-collab-aware-undo.md
Where
wasm/bindings/eeschema_embind.cpp:722-724/:843-845andwasm/bindings/pcbnew_embind.cpp:806-809/:886-888— remote applies go throughSCH_COMMIT/BOARD_COMMITwithPush( "Collaborative edit" ), i.e. they create ordinary undoable entries on the receiving editor's stackweb/standalone/src/wasm/collab/kicad-binding.ts:176-188— the adopt path applies ALL doc roots as one wire batch → one giant commitweb/standalone/src/wasm/collab/sheet-manager.ts:188-191— parked-dirty rebind runs that adopt on every sheet revisit that saw remote traffic
What happens
Going through a real commit is the right call for model consistency (connectivity, ratsnest, ERC recompute like a UI edit) — but it has an unhandled consequence:
- Ctrl+Z reverts remote work. A remote apply is on the local undo stack. The user pressing undo after a peer's edit reverts the peer's change; the reversion is a normal local commit → it flushes → it propagates to everyone, including the original author. From the author's perspective their edit "randomly disappears".
- Adopt commits are undo bombs. The seed/adopt and the parked-dirty sheet rebind apply the entire sheet as one commit ("Collaborative edit (items)"). One Ctrl+Z after revisiting a sheet reverts the whole remote catch-up — potentially dozens of peers' edits — and broadcasts the stale sheet state to the room.
- Convergence is preserved (the system happily syncs the reverted state), which is exactly the problem: the damage replicates perfectly.
There is no loop risk (the reversion is applied on peers as a remote change and suppressed from re-emit), and redo behaves symmetrically. This is purely a policy/UX gap, not an algorithmic one — but it can destroy significant work with one keystroke, so it deserves an explicit decision rather than the current default.
Options
- Exclude remote applies from the undo stack. Both commit classes support pushing without undo (or the undo entry can be dropped after Push). Standard collaborative-editor semantics: undo is local-ops-only. This is the direction most products (Figma, Google Docs) take. Cost: KiCad's undo machinery assumes the stack mirrors model history; entries referencing items later replaced by remote applies must be invalidated or made resilient (item-by-uuid re-resolution at undo time — KiCad's PICKED_ITEMS_LIST holds pointers, so this needs care).
- Keep remote applies undoable but split adopt into per-item diffs (see 13-opt-parked-dirty-full-sheet-replace.md) so at least the bomb shrinks to the real delta. Doesn't fix (1), halves the blast radius.
- Minimum bar: name the undo entries distinctly (already done) and clear the undo stack on adopt (a full-sheet replace is a reasonable "history barrier"). Cheap, removes the worst case, keeps normal-sized remote entries undoable.
Recommendation: 3 now (one call at adopt time), 1 as the eventual model, evaluated
against how invasive pointer-invalidation is in PICKED_ITEMS_LIST.
Update 2026-07-07: option 1 feasibility researched in
19-undo-option1-feasibility.md — verdict: ~3–5
days; pcbnew undo already UUID-guards stale pointers, the core work is porting that
guard to eeschema plus two small SKIP_UNDO fork fixes (SCH connectivity gate,
removed-item leak). Option 3 becomes largely moot under it.