574284c486Ctrl+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
63 lines
3.7 KiB
Markdown
63 lines
3.7 KiB
Markdown
# 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](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](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](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.
|