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
This commit is contained in:
parent
cd68114cdc
commit
574284c486
8 changed files with 528 additions and 6 deletions
|
|
@ -95,3 +95,4 @@ sending half emits NOTHING, not the bare removal the doc predicted).
|
|||
| 17 | [17-fixes-bugs-01-07.md](17-fixes-bugs-01-07.md) | Bugs 01–07 fixed & verified (2026-07-03); findings F5–F6; remaining follow-ups |
|
||||
| 18 | [18-miss08-opts-12-13.md](18-miss08-opts-12-13.md) | Miss 08 (lib_symbols + layout save-sync) + opts 12/13 implemented; opt 14 deferred (2026-07-03) |
|
||||
| 19 | [19-undo-option1-feasibility.md](19-undo-option1-feasibility.md) | Miss-09 option 1 (local-ops-only undo) feasibility research (2026-07-07): ~3–5 days, eeschema UUID-guard port is the core |
|
||||
| 20 | [20-fix-miss09-collab-aware-undo.md](20-fix-miss09-collab-aware-undo.md) | Miss 09 FIXED (2026-07-07): SKIP_UNDO remote applies + eeschema UUID guard + binding-owned removed-item lifetime; 5/5 e2e |
|
||||
|
|
|
|||
|
|
@ -1,7 +1,8 @@
|
|||
# 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:** open decision
|
||||
**Status:** FIXED 2026-07-07 — option 1 implemented, see
|
||||
[20-fix-miss09-collab-aware-undo.md](20-fix-miss09-collab-aware-undo.md)
|
||||
|
||||
## Where
|
||||
|
||||
|
|
|
|||
|
|
@ -0,0 +1,68 @@
|
|||
# Fix 20 — Miss 09 implemented: local-ops-only undo (option 1)
|
||||
|
||||
**Status:** DONE, e2e-verified 2026-07-07 (5/5 firefox + collab/ysync regression suites)
|
||||
**Design:** [09-miss-undo-not-collab-aware.md](09-miss-undo-not-collab-aware.md) option 1,
|
||||
feasibility per [19-undo-option1-feasibility.md](19-undo-option1-feasibility.md)
|
||||
|
||||
Remote applies no longer land on the receiving editor's undo stack: Ctrl+Z after a
|
||||
peer's edit reverts your own last op, never the peer's, and the adopt "undo bomb" is
|
||||
gone (adopt creates no undo entry at all — 09's option 3 history-barrier is moot).
|
||||
|
||||
## What changed
|
||||
|
||||
### KiCad fork
|
||||
|
||||
- `eeschema/schematic_undo_redo.cpp` — **UUID stale-pointer guard** in
|
||||
`PutDataInPreviousState`, mirroring pcbnew's: every picker (except `DELETED`,
|
||||
`PAGESETTINGS`, `REPEAT_ITEM`, and the root sheet, which `ResolveItem` cannot see)
|
||||
is existence-checked via `SCHEMATIC::ResolveItem(uuid)`. Missing → picker dropped
|
||||
(`not_found` → `wxLogWarning`, no modal). Present-but-different-pointer (a remote
|
||||
apply replaced the object, same uuid) → the picker is **re-anchored**
|
||||
(`SetPickedItem`) and the restore targets the current live item. Without this,
|
||||
undoing an entry whose item a remote apply freed dereferenced a dangling pointer.
|
||||
- `eeschema/sch_commit.cpp` — `RecalculateConnections` moved **out** of the
|
||||
`!( aCommitFlags & SKIP_UNDO )` gate in `pushSchEdit`: a commit that skips undo
|
||||
still changes the model, and remote applies rely on the recalc (it was the whole
|
||||
point of applying through real commits). `SaveCopyInUndoList` stays gated.
|
||||
- `pcbnew/undo_redo.cpp` — the "Incomplete undo/redo operation" `wxMessageBox`
|
||||
downgraded to `wxLogWarning`: dropped entries are routine in a collab session,
|
||||
and a blocking modal would hang the wasm modal pump.
|
||||
|
||||
### wasm bindings
|
||||
|
||||
- `eeschema_embind.cpp` / `pcbnew_embind.cpp` — all four remote-apply Push sites
|
||||
(`doApply` + `doApplyItems` per editor) now push with **`SKIP_UNDO`**. Change
|
||||
detection is unaffected: emit suppression is keyed off `s_applyingRemote`, not
|
||||
undo entries.
|
||||
- **Removed-item ownership**: under `SKIP_UNDO` no undo picker takes ownership of
|
||||
removed items (and the commit deletes the clone image), so the bindings free the
|
||||
detached items after Push — both the explicit `removed[]` uuids and the upsert's
|
||||
remove-old-before-re-add. Fields are excluded (CHT_REMOVE hides them; they stay
|
||||
owned by their parent). Freeing stays in the binding layer, NOT the fork commit
|
||||
classes, because existing `SKIP_UNDO` callers (DRC marker flows) manage their
|
||||
removed items' lifetimes themselves — freeing in the commit would double-free.
|
||||
- Test hooks `kicadCollabTestUndo` (runs `ACTIONS::undo` on the main-loop/fiber
|
||||
stack) and `kicadCollabTestUndoDepth`, registered per-editor **and** in
|
||||
`kicad_editor_embind.cpp`'s dispatcher — the merged image compiles out the
|
||||
per-app `EMSCRIPTEN_BINDINGS` registrations, so a shared-name hook that is not
|
||||
in the dispatcher silently vanishes from kicad_editor (build even forces the
|
||||
relink; it's the registration that's conditional).
|
||||
|
||||
## Coverage — tests/kicad/collab-undo.spec.ts (5/5)
|
||||
|
||||
| Scenario | Editors |
|
||||
|---|---|
|
||||
| Remote apply adds no undo entry; undo reverts own op, peer's delete survives | eeschema + pcbnew |
|
||||
| Stranded CHANGED entry (remote replaced the item) — undo re-anchors by uuid, no crash | eeschema |
|
||||
| Stranded entry (remote deleted the item) — picker dropped quietly (log, not modal), no resurrect | eeschema + pcbnew |
|
||||
|
||||
Pre-fix, the stranded cases dereferenced freed memory (eeschema had no guard at
|
||||
all; pcbnew guarded existence but popped a blocking modal).
|
||||
|
||||
## Accepted semantics (per doc 19)
|
||||
|
||||
- Undo restores the full item image → a peer's concurrent edit to *another field of
|
||||
the same item* is clobbered (LWW, converges; same trade-off as opt 14 granularity).
|
||||
- Duplicate-KIID edge (local delete parked on undo + peer re-adds same uuid + undo)
|
||||
remains untested; the differ keys by uuid so it should reconcile — follow-up test.
|
||||
- Pre-existing local entries that a big adopt strands are dropped on first undo.
|
||||
Loading…
Reference in a new issue