pcbjam/docs/features/ysync-review/09-miss-undo-not-collab-aware.md
Gergő Törcsvári 574284c486
feat(collab): local-ops-only undo — remote applies skip the undo stack (ysync miss 09)
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
2026-07-08 11:09:26 +02:00

3.7 KiB
Raw Blame History

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-845 and wasm/bindings/pcbnew_embind.cpp:806-809 / :886-888 — remote applies go through SCH_COMMIT / BOARD_COMMIT with Push( "Collaborative edit" ), i.e. they create ordinary undoable entries on the receiving editor's stack
  • web/standalone/src/wasm/collab/kicad-binding.ts:176-188 — the adopt path applies ALL doc roots as one wire batch → one giant commit
  • web/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:

  1. 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".
  2. 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.
  3. 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

  1. 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).
  2. 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.
  3. 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: ~35 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.