From 80135a99ac289faccd4a0ec73021e9324158f833 Mon Sep 17 00:00:00 2001 From: Viktor Vaczi Date: Fri, 3 Jul 2026 12:43:55 +0200 Subject: [PATCH] docs(ysync-review): repro suite results + empirical findings (doc 16); plan 15 executed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit New doc 16: the full repro-suite map (per-bug unit/e2e paths with verified failure sites), the phase-C probe outcome, and four findings only the RUNNING system revealed: - F1: bug 03's sending half emits NOTHING — a child-only delete commit never triggers a flush at all (worse than the doc's predicted bare removed-wire); the GetWidth-assert tracer evidence and the fix implication. - F2: Firefox cannot host two kicad_editor tabs in one context (per-process wasm budget) — bug-01 two-tab repros are Chromium-only. - F3: headless emit WORKS on both pcbnew and eeschema — the legacy two-tab skip rationale and items-bridge localEdit omissions are stale. - F4: drift-detect is strictly ITEM-silent on the green path (no writer- formatting false positives). Cross-updates: 00 index + verdict note; 01/04/05/06/07 Verification sections gain their repro paths; 02 upgraded to runtime-CONFIRMED; 03 gains the F1 empirical correction; 11 (no v2 e2e coverage) CLOSED with a point-by-point status update — only the legacy retirement remains. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01DPfrVhfYgPPgtawjSssZfn --- docs/features/ysync-review/00-overview.md | 86 +++++++++ ...bug-first-tab-listener-never-registered.md | 97 ++++++++++ .../02-bug-footprint-blob-zeroes-pad-nets.md | 74 ++++++++ .../03-bug-child-removal-dangling-slot.md | 88 +++++++++ .../04-bug-lossy-change-detection.md | 94 ++++++++++ .../05-bug-rebaseline-swallows-local-edits.md | 75 ++++++++ ...6-bug-concurrent-seed-duplicates-layout.md | 83 +++++++++ .../07-bug-sheet-switch-stale-down-hook.md | 85 +++++++++ .../08-miss-layout-state-never-syncs.md | 70 ++++++++ .../09-miss-undo-not-collab-aware.md | 56 ++++++ .../ysync-review/10-miss-no-repair-path.md | 60 +++++++ .../11-miss-no-v2-e2e-coverage.md | 86 +++++++++ .../12-opt-hot-path-full-model-work.md | 60 +++++++ .../13-opt-parked-dirty-full-sheet-replace.md | 56 ++++++ .../14-opt-item-granularity-bandwidth.md | 60 +++++++ .../15-plan-repro-tests-and-v2-e2e.md | 168 ++++++++++++++++++ ...ro-suite-results-and-empirical-findings.md | 133 ++++++++++++++ 17 files changed, 1431 insertions(+) create mode 100644 docs/features/ysync-review/00-overview.md create mode 100644 docs/features/ysync-review/01-bug-first-tab-listener-never-registered.md create mode 100644 docs/features/ysync-review/02-bug-footprint-blob-zeroes-pad-nets.md create mode 100644 docs/features/ysync-review/03-bug-child-removal-dangling-slot.md create mode 100644 docs/features/ysync-review/04-bug-lossy-change-detection.md create mode 100644 docs/features/ysync-review/05-bug-rebaseline-swallows-local-edits.md create mode 100644 docs/features/ysync-review/06-bug-concurrent-seed-duplicates-layout.md create mode 100644 docs/features/ysync-review/07-bug-sheet-switch-stale-down-hook.md create mode 100644 docs/features/ysync-review/08-miss-layout-state-never-syncs.md create mode 100644 docs/features/ysync-review/09-miss-undo-not-collab-aware.md create mode 100644 docs/features/ysync-review/10-miss-no-repair-path.md create mode 100644 docs/features/ysync-review/11-miss-no-v2-e2e-coverage.md create mode 100644 docs/features/ysync-review/12-opt-hot-path-full-model-work.md create mode 100644 docs/features/ysync-review/13-opt-parked-dirty-full-sheet-replace.md create mode 100644 docs/features/ysync-review/14-opt-item-granularity-bandwidth.md create mode 100644 docs/features/ysync-review/15-plan-repro-tests-and-v2-e2e.md create mode 100644 docs/features/ysync-review/16-repro-suite-results-and-empirical-findings.md diff --git a/docs/features/ysync-review/00-overview.md b/docs/features/ysync-review/00-overview.md new file mode 100644 index 0000000..30c6456 --- /dev/null +++ b/docs/features/ysync-review/00-overview.md @@ -0,0 +1,86 @@ +# Yjs ⇄ KiCad sync review — overview and index + +Full-stack review (2026-07-02) of the collaborative-editing synchronization between the +Y.Doc and the internal state of eeschema / pcbnew. Scope read end to end: + +- Shared Slot model: `web/pcbjam-shared/src/{kicad-doc,kicad-y,items-wire,kicad-delta,sexpr}.ts` +- Runtime binding: `web/standalone/src/wasm/collab/{kicad-binding,sheet-manager,provider,drift-detect,index}.ts` +- App wiring: `web/standalone/src/components/WasmTool.tsx` +- C++ bridges: `wasm/bindings/{eeschema,pcbnew}_embind.cpp` +- Tests: `web/standalone/src/wasm/collab/*.test.ts`, `tests/kicad/*-collab.spec.ts`, + `tests/collab/browser-entry.ts` + +## How the sync works (context for every finding) + +The production path is the v2 "items" wire (ysync 0008). C++ keeps a per-uuid scalar +snapshot (`itemToJson`) as a diff baseline; a `BOARD_LISTENER` / `SCHEMATIC_LISTENER` +treats commits as "something changed" triggers, and a post-settle `flushDiff` +(CallAfter + COROUTINE) compares snapshots and emits each changed item as a full s-expr +blob. The TS side re-flattens each blob into uuid-keyed items with `parent` links +(`itemsWireToDelta`), diffs against the Y.Doc's `kdoc_items` map, and writes item-level +updates (`applyDeltaToY`). Remote Y events go the other way: `deltaFromYEvents` → +render the item's full subtree s-expr → `kicadCollabApplyItems`, which does an +idempotent remove-by-uuid + re-add through a real commit. Seeding is seed-once: the +first tab writes `fileToDoc(file)` into the empty room; joiners adopt the doc +wholesale. Drift detection periodically compares a scratch save against `yToDoc` and +reports (never repairs) divergence. + +The overall shape — post-settle diff, idempotent full-item upserts, echo suppression by +origin tag plus C++ rebaseline — is sound and a good fit for the asyncify/wasm +constraints. The findings below are the places where it breaks or leaks. + +## Verdict / suggested order of attack + +Every bug below has a runnable expected-fail reproduction test, and the v2 e2e port +(miss 11) is DONE — see [16](16-repro-suite-results-and-empirical-findings.md) for +the suite map and four empirical findings the running system added (notably: bug 03's +sending half emits NOTHING, not the bare removal the doc predicted). + +1. Fix [01](01-bug-first-tab-listener-never-registered.md) immediately (one line). +2. Then [02](02-bug-footprint-blob-zeroes-pad-nets.md) and + [03](03-bug-child-removal-dangling-slot.md) (small, contained, data-corrupting). +3. Fold [04](04-bug-lossy-change-detection.md) and + [05](05-bug-rebaseline-swallows-local-edits.md) into one change: blob-hash / + dirty-set change detection plus targeted rebaseline. This also delivers most of + [12](12-opt-hot-path-full-model-work.md). +4. [06](06-bug-concurrent-seed-duplicates-layout.md) and + [07](07-bug-sheet-switch-stale-down-hook.md) are race windows — narrow them after + the e2e port ([11](11-miss-no-v2-e2e-coverage.md)) gives regression cover. + +## Index + +### Bugs + +| # | File | One-liner | +|---|------|-----------| +| 01 | [01-bug-first-tab-listener-never-registered.md](01-bug-first-tab-listener-never-registered.md) | Fresh-room seeding tab never registers the C++ change listener → its edits never sync | +| 02 | [02-bug-footprint-blob-zeroes-pad-nets.md](02-bug-footprint-blob-zeroes-pad-nets.md) | Footprint blobs strip pad net codes → net data loss propagates to peers, doc, and files | +| 03 | [03-bug-child-removal-dangling-slot.md](03-bug-child-removal-dangling-slot.md) | Child-only deletion leaves a dangling `{item}` slot in the parent's Y body → renders throw | +| 04 | [04-bug-lossy-change-detection.md](04-bug-lossy-change-detection.md) | Change detection is a lossy scalar projection → rotations, field text edits, pad edits never sync | +| 05 | [05-bug-rebaseline-swallows-local-edits.md](05-bug-rebaseline-swallows-local-edits.md) | Global rebaseline after apply can silently drop concurrent local edits and receiver-side cleanup | +| 06 | [06-bug-concurrent-seed-duplicates-layout.md](06-bug-concurrent-seed-duplicates-layout.md) | Two clients seeding an empty room concurrently duplicate `kdoc_layout` → corrupt materialization | +| 07 | [07-bug-sheet-switch-stale-down-hook.md](07-bug-sheet-switch-stale-down-hook.md) | Sheet switch leaves `onItems` pointing at the old room → cross-room contamination window | + +### Design misses + +| # | File | One-liner | +|---|------|-----------| +| 08 | [08-miss-layout-state-never-syncs.md](08-miss-layout-state-never-syncs.md) | Non-item state (title block, settings, `lib_symbols`) only syncs at seed | +| 09 | [09-miss-undo-not-collab-aware.md](09-miss-undo-not-collab-aware.md) | Ctrl+Z reverts remote work and re-broadcasts it; adopt commits are undo bombs | +| 10 | [10-miss-no-repair-path.md](10-miss-no-repair-path.md) | Drift is detected and reported but never repaired | +| 11 | [11-miss-no-v2-e2e-coverage.md](11-miss-no-v2-e2e-coverage.md) | Two-tab e2e exercises the legacy wire; the production v2 stack has no end-to-end test | + +### Optimizations + +| # | File | One-liner | +|---|------|-----------| +| 12 | [12-opt-hot-path-full-model-work.md](12-opt-hot-path-full-model-work.md) | O(full-model) work (incl. zod) on every edit/apply/remote batch | +| 13 | [13-opt-parked-dirty-full-sheet-replace.md](13-opt-parked-dirty-full-sheet-replace.md) | Parked-dirty sheet rebind re-applies the whole sheet instead of the delta | +| 14 | [14-opt-item-granularity-bandwidth.md](14-opt-item-granularity-bandwidth.md) | Item-level body granularity ships the whole item per nudge; LWW drops concurrent property edits | + +### Plan & results + +| # | File | One-liner | +|---|------|-----------| +| 15 | [15-plan-repro-tests-and-v2-e2e.md](15-plan-repro-tests-and-v2-e2e.md) | The approved plan: repro tests for bugs 01–07 + the v2 e2e port | +| 16 | [16-repro-suite-results-and-empirical-findings.md](16-repro-suite-results-and-empirical-findings.md) | Plan 15 executed (2026-07-03): suite map, verified failure sites, empirical findings F1–F4 | diff --git a/docs/features/ysync-review/01-bug-first-tab-listener-never-registered.md b/docs/features/ysync-review/01-bug-first-tab-listener-never-registered.md new file mode 100644 index 0000000..371f21d --- /dev/null +++ b/docs/features/ysync-review/01-bug-first-tab-listener-never-registered.md @@ -0,0 +1,97 @@ +# Bug 01 — First-ever tab never registers the C++ change listener; the seeding session can't send + +**Severity:** critical (breaks the primary first-session flow for all three tools) +**Status:** open +**Fix size:** one line + +## Where + +- `web/standalone/src/wasm/collab/kicad-binding.ts:144-154` — the file-seed branch of `seed()` +- `wasm/bindings/eeschema_embind.cpp:578-594` (`ensureBridge`), called only from + `schCollabSnapshot` (`:909`) and `schCollabSnapshotItems` (`:957`) +- `wasm/bindings/pcbnew_embind.cpp:729-745` (`ensureBridge`), called only from + `pcbCollabSnapshot` (`:975`) and `pcbCollabSnapshotItems` (`:998`) + +## What happens + +The C++ `COLLAB_LISTENER` — the only thing that turns local commits into emits — is +registered lazily inside `ensureBridge()`, which is reached **only** through the two +snapshot entry points. On the JS side, `seed()` has three branches: + +- `editorMatchesDoc` branch → calls `bridge.snapshotItems()` explicitly "to BASELINE + the wasm differ" → listener registered ✓ +- adopt / editor-snapshot-seed branches → call `bridge.snapshotItems()` to read the + model → listener registered ✓ +- **file-seed branch** (`!ydocHasState(doc) && seedDoc`) → `docToY(seedDoc, …)` and + `return` — `snapshotItems()` is never called ✗ + +So in any **fresh room** (first-ever open of a project, both api and ydoc mode, all +three collab tools), the seeding tab has no C++ listener: + +1. Local edits never fire `scheduleFlush` → nothing is ever emitted → the seeder + *receives* peers' edits but *never sends* its own. +2. eeschema is worse: `OnSchSheetChanged` → `emitSheetChanged` lives on the same + listener, so sheet navigation never notifies JS, `SheetCollabManager.switchTo` is + never driven, and the C++ diff baseline is never re-scoped on navigation. + +On the *next* session the room has state, seed() takes the adopt branch, the listener +registers, and everything works — which is exactly why this is easy to miss manually. + +## Why tests don't catch it + +- The two-tab e2e specs (`tests/kicad/{eeschema,pcbnew,pl_editor}-collab.spec.ts`) + drive the **legacy** scalar path via `tests/collab/browser-entry.ts` (`startCollab`), + whose `seed()` always calls `bridge.snapshot()` → `ensureBridge()`. +- The vitest binding tests (`kicad-binding.test.ts`) use a fake JS bridge; the fake has + no "listener registration happens inside snapshotItems" side effect, so the file-seed + branch looks fine there. + +See [11-miss-no-v2-e2e-coverage.md](11-miss-no-v2-e2e-coverage.md). + +## Observable symptoms + +- First session after project creation: tab A's edits don't reach tab B, while B's + edits reach A (asymmetric sync). +- eeschema fresh project: navigating into a subsheet doesn't rebind rooms; edits on + child sheets aren't scoped/synced. +- Drift telemetry: the seeder's session ends with a `beforeunload` drift beacon + containing every edit of the session (the Y.Doc never received any of them). + +## Fix + +In the file-seed branch of `seed()` (kicad-binding.ts:144-154), after `docToY(...)`, +call `bridge.snapshotItems()` and discard the result — exactly the pattern the +`editorMatchesDoc` branch already uses: + +```ts +if (!ydocHasState(doc) && seedDoc) { + docToY(seedDoc, doc, ORIGIN); + try { + bridge.snapshotItems(); // register the C++ listener + baseline the differ + } catch (err) { + cwarn("seed: post-file-seed baseline failed", err); + } + return; +} +``` + +The returned snapshot is redundant (the Y.Doc was just seeded losslessly from the +file), but the call's side effects — `ensureBridge()` listener registration and +`rebaseline()` — are the whole point. + +Note: even without the baseline part, the TS layer would absorb a stale-empty baseline +(a full-model re-emit diffs to an empty `KicadDelta` against the already-seeded Y +items), so the *listener registration* is the load-bearing half of the fix. + +## Verification + +Two-tab e2e on the v2 wire with a genuinely fresh room id: tab A opens (file-seeds), +makes an edit, tab B must receive it. For eeschema, additionally navigate into a +subsheet on tab A and assert `[sheet]` switch logs / per-room scoping. + +Repro tests (2026-07-03, expected-fail until fixed — see +[16](16-repro-suite-results-and-empirical-findings.md)): units in +`web/standalone/src/wasm/collab/ysync-repros.test.ts` (the snapshotItems contract + +peer-never-receives, with a C++-faithful fake that gates emit on `ensureBridge`); +e2e in `tests/kicad/ysync-two-tab.spec.ts` (pcbnew + eeschema fresh-room; Chromium +only — finding F2 — verified on Chrome: A's move lands, B never converges). diff --git a/docs/features/ysync-review/02-bug-footprint-blob-zeroes-pad-nets.md b/docs/features/ysync-review/02-bug-footprint-blob-zeroes-pad-nets.md new file mode 100644 index 0000000..9ca9df9 --- /dev/null +++ b/docs/features/ysync-review/02-bug-footprint-blob-zeroes-pad-nets.md @@ -0,0 +1,74 @@ +# Bug 02 — Footprint blobs zero pad net codes; net data loss propagates to peers, the Y.Doc, and materialized files + +**Severity:** high (silent, converging data corruption on boards with nets) +**Status:** open — runtime-CONFIRMED 2026-07-03: the snapshot blob carries the pads +with `(net 1 "SIG")` stripped (repro below) + +## Where + +- `wasm/bindings/pcbnew_embind.cpp:275-322` — `blobForItem`, specifically the footprint + branch's `pad->SetNetCode( 0 )` loop (`:293-295`) +- Consumers: `flushDiff`'s `liftBlob` (`:593-618`), `pcbCollabSnapshotItems` (`:996-1019`), + `doApplyItems` (`:822-894`) + +## What happens + +`blobForItem` copies clipboard `SaveSelection`'s "make safe to transfer" steps for +footprints, including zeroing every pad's net code before `Format()`. That is correct +for pasting into a *foreign* board (net codes are per-board indices), but the collab +wire connects peers editing the *same* board with identity-by-uuid — nets should +survive. + +Concrete flow for a plain footprint **move** on the sender: + +1. `flushDiff` sees the footprint's scalar json changed → `liftBlob` → footprint blob + with **netless pads** goes out on the v2 wire (`wChanged`). +2. TS `itemsWireToDelta` re-flattens the blob; the pad items' bodies differ from the + Y.Doc's (which still has `(net N "NAME")` from the file seed) → pads are `updated` + → **the Y.Doc's pad bodies lose their nets**. +3. Peers receive the change, render the footprint from the (now netless) Y view, and + `doApplyItems` does remove+add — **the peer's board now has net-0 pads** on that + footprint. Ratsnest lines to it disappear; DRC connectivity changes. +4. The sender's own editor still has nets → the sender now permanently drifts from the + Y.Doc (drift-detect will report it forever). +5. Once the *peer* touches the same footprint, its (already netless) blob flows back + and the session converges on netless pads everywhere. +6. In `docSource: "ydoc"` mode the next open materializes the board from the doc + (`docToFile`) — **the author's file itself loses pad nets across a reload**. + +Also note: `pcbCollabSnapshotItems` (editor-snapshot seeding and adopt comparisons) +uses the same blob, so even the seed path bakes in the loss when the room is seeded +from the editor snapshot instead of the file. The file-seed path (`fileToDoc`) +preserves nets — until the first footprint edit destroys them. + +## Why the "safety" step doesn't apply here + +`MapNets` remapping only runs for the `(kicad_pcb …)` envelope parse +(`makeFromBlob`, `:346-368`); bare footprint blobs never get net remapping — they get +nothing, because the nets were already stripped at the source. Peers in a collab +session share the same net table lineage, so the paste-into-foreign-board rationale +doesn't hold. + +## Fix direction + +1. In `blobForItem`'s footprint branch, **drop the `SetNetCode(0)` loop** (keep the + mandatory-field uuid restore and `SetLocked(false)`). +2. Verify on the receiving side that pad `(net N "NAME")` tokens parse correctly + against the live board (`io.SetBoard(aBoard)` should resolve them). If net *codes* + can diverge between peers after local edits, remap by **name** on apply (the same + thing `MapNets` does for envelope boards) rather than trusting the code. +3. Add a regression test: seed a board with netted pads, move a footprint on tab A, + assert tab B's pad nets AND the Y.Doc pad bodies still carry the nets. + +## Repro + +`tests/kicad/ysync-repros-pcbnew.spec.ts` "footprint blob preserves pad nets" +(`test.fail`), with the green "footprint blob embeds its pad children" precondition +pinning that only the nets — not the pads — are missing. See +[16](16-repro-suite-results-and-empirical-findings.md). + +## Related + +- [04-bug-lossy-change-detection.md](04-bug-lossy-change-detection.md) — pad property + edits are separately invisible to the differ; this file is about the emit *payload*, + that one about the emit *trigger*. diff --git a/docs/features/ysync-review/03-bug-child-removal-dangling-slot.md b/docs/features/ysync-review/03-bug-child-removal-dangling-slot.md new file mode 100644 index 0000000..59e32ba --- /dev/null +++ b/docs/features/ysync-review/03-bug-child-removal-dangling-slot.md @@ -0,0 +1,88 @@ +# Bug 03 — Child-only deletions leave a dangling `{item}` slot in the parent's Y body + +**Severity:** high (poisons a room's render/materialize path; three-way divergence) +**Status:** open + +## Where + +- `web/pcbjam-shared/src/kicad-y.ts:152-181` — `applyDeltaToY` cleans up **root layout** + slots for removed items but never prunes `{item: uuid}` slots from a surviving + **parent's body** +- `wasm/bindings/pcbnew_embind.cpp:656-660` — `flushDiff`'s removed loop pushes raw + uuids with no `liftBlob` counterpart (adds/changes lift a touched child to a parent + re-blob; removals don't) +- `wasm/bindings/pcbnew_embind.cpp:831-844` — `doApplyItems` removed loop *skips* + items with `GetParentFootprint()` ("covered by its parent's replace/remove") +- `web/pcbjam-shared/src/kicad-doc.ts:205-207` — `renderItemInner` throws + `renderItem: missing item ` on a dangling reference + +## Trigger + +Delete a footprint **user field** (Footprint Properties → remove a field row) or a +footprint's user text child on the pcbnew board editor: + +1. The child uuid vanishes from `forEachTopItem`'s snapshot → it lands in `removed`. +2. The footprint's own scalar json (`itemToJson`: id/type/x/y/layer) is **unchanged**, + so no parent blob is emitted alongside — the wire is exactly + `{ removed: [childUuid] }`. + +## Consequences + +Three states diverge simultaneously: + +- **Y.Doc:** `applyDeltaToY` deletes the child item, but the parent footprint's body + still contains the `{item: childUuid}` slot. From now on: + - `renderItem(parent)` throws → `deltaToItemsWire` throws inside the `observeDeep` + callback on **any** later remote change touching that footprint, aborting the whole + batch's conversion (other items in the same transaction are dropped too). + - `docToFile` throws → ydoc-mode materialization fails (WasmTool catches it and falls + back to the API fetch, so opens survive, but the room can't be rendered). +- **Receiving editor:** the C++ removed loop skips the child (parent-footprint guard) + → the peer *keeps* the field. +- **Sending editor:** the field is gone. + +Partial mitigation that exists by accident: if the footprint is later modified, its +blob re-upserts the whole body (`upsertYItem` replaces `body` wholesale) and the +dangling slot disappears — the poison self-heals *only if* that footprint changes +again. + +eeschema is not currently exposed to this specific trigger because symbol fields are +not visited by its snapshot at all (they're invisible to the differ — see +[04-bug-lossy-change-detection.md](04-bug-lossy-change-detection.md)), so a field +deletion there simply doesn't emit. + +## Fix direction + +Two layers, both worth doing: + +1. **Emit side (root cause):** make removals lift like adds/changes do. In `flushDiff`, + when a removed uuid's baseline entry belonged to a footprint child whose parent + still exists, emit the parent's re-blob in `wChanged` instead of a bare child + removal (mirror `liftBlob`'s dedup via `wDone`). The parent's new body then carries + the correct child set end to end, and the C++ receiver's parent-replace covers the + deletion naturally. +2. **Shared-lib side (defense in depth):** in `applyDeltaToY`, when deleting an item + whose `parent` still exists in the items map, prune the `{item: uuid}` slot from the + parent's body (a body rewrite via `upsertYItem`-style set). This keeps the + "file recoverable from the Y.Doc alone" invariant unconditionally true, whatever a + future emitter sends. + +## Verification + +Unit test in `kicad-y`/`items-wire`: apply `{removed:[child]}` where the parent +survives; assert `renderItem(parent)` and `docToFile` still succeed and the parent body +no longer references the child. Integration: delete a footprint user field on tab A, +assert tab B loses the field and the room still materializes. + +Repro tests (2026-07-03): units in `web/pcbjam-shared/test/ysync-repros.test.ts` +(`renderItem: missing item` + `docToFile` through the dangling slot); e2e Y-half in +`tests/kicad/ysync-two-tab.spec.ts`, receiving + sending halves in +`tests/kicad/ysync-repros-pcbnew.spec.ts`. + +**Empirical correction (finding F1 in +[16](16-repro-suite-results-and-empirical-findings.md)):** the sending half is WORSE +than the Trigger section above predicts — headless, a child-only delete commit never +triggers a flush at all (not even the bare `{removed:[child]}` wire goes out; the +snapshot-tracer shows `flushDiff` never ran). The emit-side fix therefore starts one +layer earlier: make the listener see the child-removal commit, THEN lift it to the +parent re-blob. diff --git a/docs/features/ysync-review/04-bug-lossy-change-detection.md b/docs/features/ysync-review/04-bug-lossy-change-detection.md new file mode 100644 index 0000000..9363917 --- /dev/null +++ b/docs/features/ysync-review/04-bug-lossy-change-detection.md @@ -0,0 +1,94 @@ +# Bug 04 — Change detection is a lossy scalar projection; a whole class of edits silently never syncs + +**Severity:** high (broad, silent non-replication of everyday edits) +**Status:** open + +## Where + +- `wasm/bindings/eeschema_embind.cpp:121-180` — `itemToJson` (the diff key), `:286-297` + `snapshotItems` iterating `screen->Items()` only +- `wasm/bindings/pcbnew_embind.cpp:144-169` — `forEachTopItem` (what is iterated), + `:176-252` — `itemToJson` +- `flushDiff` in both files — the v2 blob emit fires **only** for uuids whose scalar + json differs from the baseline + +## The structural problem + +The v2 items wire carries lossless per-item s-expr blobs, but its **trigger** is still +the legacy scalar snapshot diff. `itemToJson` projects each item to a handful of +fields (id/type/x/y/layer + a few per-type extras). Any edit that doesn't change the +projection produces an empty diff → **no emit on either wire**, even though the +listener fired. Drift-detect eventually *reports* the divergence; nothing repairs it +([10-miss-no-repair-path.md](10-miss-no-repair-path.md)). + +## Known-missed edits + +eeschema: + +- **Symbol rotate / mirror** — `GetPosition()` unchanged → invisible. +- **Reference / value / any field text edit** — fields are not in `screen->Items()` + (they live inside the symbol), and the symbol's json carries no field text + (`SCH_SYMBOL` is not an `EDA_TEXT`) → invisible. This is arguably the most common + schematic edit after moving things. +- **Label / text rotation** — spins about the anchor; position unchanged → invisible. +- Stroke color and similar cosmetic properties not in the projection. + +pcbnew: + +- **Pad property edits** (size, shape, drill, net via pad dialog) — pads are + deliberately not visited by `forEachTopItem` → invisible. +- **Zone properties** (net, hatch, priority, fill settings) — only `Outline(0)` points + are compared → invisible. Holes / additional outlines are also outside the + projection. +- **Graphic shape endpoint drags** — `Drawings` items' json is position-only. Dragging + the *end* point of a segment leaves `GetPosition()` (the start) unchanged → + invisible. (When the *start* moves, the change IS detected and the v2 blob replace is + correct; the legacy `SetPosition` semantics would have translated instead of + reshaping, but the legacy wire is dead in production.) +- **Footprint rotation** syncs only *by accident*: the field children's absolute + positions move, which lifts the parent blob. A footprint whose fields sit exactly on + the rotation anchor would not sync its rotation. + +## Why fixing this properly is cheap + +Two ingredients already exist: + +1. The listener callbacks receive the **touched-item vectors** + (`OnSchItemsChanged(…, std::vector&)`, `OnBoardItemsChanged(…)`, + `OnBoardCompositeUpdate(…)`) — currently ignored ("the listener is just a + trigger"). Collect the uuids into a dirty set at trigger time. +2. The blob serializer (`itemBlob` / `blobForItem`) is the lossless comparison unit. + +Post-settle, instead of diffing the full scalar snapshot, for each **dirty root** +(child uuids lifted to their parent, as `liftBlob` already does) compare the current +blob — or a hash of it — against the last-emitted blob hash, and emit on mismatch. +This: + +- catches every serializer-visible property (rotation, field text, pad edits, zone + settings) by construction; +- keeps the post-settle convergence property (the blob is taken after cleanup); +- shrinks the per-edit cost from O(all items) to O(dirty items) — the main lever of + [12-opt-hot-path-full-model-work.md](12-opt-hot-path-full-model-work.md); +- lets the scalar snapshot/baseline machinery (and the legacy wire emit) retire. + +Removals still need the baseline uuid set (a disappeared uuid can't be blobbed); keep a +uuid→(parent, blob-hash) map as the baseline instead of uuid→json. + +Interaction with [05-bug-rebaseline-swallows-local-edits.md](05-bug-rebaseline-swallows-local-edits.md): +moving the baseline to uuid→hash makes the targeted post-apply rebaseline natural — +update hashes only for the uuids the apply touched. + +## Verification + +Per-tool e2e matrix of the missed edit list above (rotate symbol, edit value text, +edit pad size, drag shape endpoint, change zone net), asserting the peer converges and +drift-detect stays quiet. + +Repro matrix (2026-07-03, each `test.fail` — see +[16](16-repro-suite-results-and-empirical-findings.md)): +`tests/kicad/ysync-repros-pcbnew.spec.ts` (anchor-centred footprint rotation — a +second fixture footprint with every child ON the anchor, so the "syncs by accident" +escape hatch is closed; pad resize; gr_line endpoint drag) and +`tests/kicad/ysync-repros-eeschema.spec.ts` (symbol rotation, Value-field edit). Each +case proves the edit LANDED (save poll) before expecting the emit; runtime-confirmed: +every one lands and none emits. Zone-net is the one matrix row without a hook yet. diff --git a/docs/features/ysync-review/05-bug-rebaseline-swallows-local-edits.md b/docs/features/ysync-review/05-bug-rebaseline-swallows-local-edits.md new file mode 100644 index 0000000..afe4258 --- /dev/null +++ b/docs/features/ysync-review/05-bug-rebaseline-swallows-local-edits.md @@ -0,0 +1,75 @@ +# Bug 05 — Global rebaseline after a remote apply can swallow concurrent local edits and receiver-side cleanup + +**Severity:** medium-high (silent permanent peer divergence; probability scales with edit rate × remote traffic) +**Status:** open + +## Where + +- `wasm/bindings/eeschema_embind.cpp:726-731` (`doApply`) and `:846-849` + (`doApplyItems`) — `rebaseline()` at the end of every apply +- `wasm/bindings/pcbnew_embind.cpp:811-816`, `:889-894` — same +- `rebaseline()` itself: snapshots the **entire current model** into `g_baseline` + +## The race + +Applies and flushes are both `CallAfter` pending events on the same frame handler, so +they are FIFO. This ordering is realistic: + +1. A remote batch arrives; JS calls `kicadCollabApplyItems` → **apply queued**. +2. Before the pending queue drains, the user's in-flight edit commits (e.g. mouse-up of + a drag processed in the same frame). `SCH/BOARD_COMMIT::Push` fires the listener → + `scheduleFlush` → **flush queued behind the apply**. +3. Pending drain: **apply runs first**. `doApplyItems` applies the remote items, then + `rebaseline()` snapshots the whole model — **which already contains the user's local + edit**. +4. **flush runs**: diff(current, baseline) is empty. The local edit is never broadcast. + +The edit stays in the local editor but never reaches the Y.Doc or peers — permanent, +silent divergence. Drift-detect will eventually report it; nothing repairs it +([10-miss-no-repair-path.md](10-miss-no-repair-path.md)). + +## Second casualty: receiver-side cleanup + +The convergence argument for the post-settle diff is "the sender broadcasts its FINAL, +post-cleanup geometry; the receiver re-cleaning already-clean geometry is idempotent." +That holds when the receiver's surroundings are identical. When they are not — +concurrent local geometry, e.g. a remotely-moved wire now crossing the receiver's +junction — the receiver's `RecalculateConnections` produces genuinely *new* state +(splits/merges the sender never saw). Because `rebaseline()` runs after `Push`, that +cleanup is folded into the baseline and **never broadcast**: the receiver has split +wires, the sender doesn't, and no future diff will notice. + +## Fix + +Make the post-apply rebaseline **targeted** instead of global: update `g_baseline` +entries only for the uuids the apply actually added/changed/removed (recompute their +post-apply json/blob-hash; drop removed ones). Everything else the apply's `Push` +mutated as a side effect — receiver-side cleanup, and any concurrently committed local +edit — then still differs from the baseline and flushes as a normal local diff: + +- the swallowed-local-edit race disappears (the edit's uuids weren't touched by the + apply, so their baseline entries are still pre-edit); +- receiver-side cleanup is broadcast and both sides converge on it. Re-application on + the original sender is idempotent, so the echo is bounded (one extra hop, then empty + diffs). + +This composes with the blob-hash baseline proposed in +[04-bug-lossy-change-detection.md](04-bug-lossy-change-detection.md): with a +uuid→hash baseline, "targeted" is just updating the hashes for the applied uuids. + +## Verification + +Deterministic interleave test via the existing test hooks: queue an apply +(`kicadCollabApplyItems`) and a local `kicadCollabTestMoveFirst` such that the commit +lands between apply-enqueue and apply-run; assert the local move still reaches the +peer. For the cleanup half: two tabs, tab B draws a wire crossing where tab A is about +to move a wire; after A's move syncs, assert both tabs converge on the same segment +set (currently B's split stays local). + +Repro (2026-07-03): implemented exactly as the first half above in +`tests/kicad/ysync-repros-pcbnew.spec.ts` ("a local edit committed while a remote +apply is queued…", `test.fail`): one JS turn queues `TestMoveFirst` then +`ApplyItems(added footprint)` → FIFO drain [move, apply, flush] → runtime-confirmed +that both land and the moved uuid is never emitted. The receiver-side-cleanup half +remains un-reproduced (needs the two-tab wire-crossing setup). See +[16](16-repro-suite-results-and-empirical-findings.md). diff --git a/docs/features/ysync-review/06-bug-concurrent-seed-duplicates-layout.md b/docs/features/ysync-review/06-bug-concurrent-seed-duplicates-layout.md new file mode 100644 index 0000000..5232214 --- /dev/null +++ b/docs/features/ysync-review/06-bug-concurrent-seed-duplicates-layout.md @@ -0,0 +1,83 @@ +# Bug 06 — Concurrent first-seed duplicates `kdoc_layout`; corrupt materialization with no self-heal + +**Severity:** medium (race window is small but the damage is durable file corruption) +**Status:** open + +## Where + +- `web/pcbjam-shared/src/kicad-y.ts:79-94` — `docToY`: `layout.delete(0, length)` then + `layout.insert(0, doc.layout)` inside one transaction +- Seed decision points: `web/standalone/src/wasm/collab/kicad-binding.ts:127-175` + (`seed()` checks `ydocHasState` then seeds), `provider.ts:56-68` (BroadcastChannel + `settleMs`, default 300 ms), `index.ts:85-98` (`whenSynced()` → `seed()`) + +## The race + +Seed-vs-adopt is decided by a **check-then-act** on the client: after `whenSynced()`, +if `ydocHasState(doc)` is false, the client seeds. Two clients opening the same fresh +room concurrently can both observe "empty" before receiving each other's seed +transaction: + +- network providers (partykit / hocuspocus): window ≈ one server round-trip between + sync-complete and the peer's update arriving; +- BroadcastChannel: window ≈ the whole `settleMs` (300 ms) if both tabs open together. + +Two simultaneous `docToY` calls then merge as follows: + +- `kdoc_meta.root` — Y.Map LWW → converges fine. +- `kdoc_items` — same uuid keys, (typically) identical values → LWW per key → + converges fine. +- **`kdoc_layout` — breaks.** Each client's `delete` sees only its own (empty) view; + each `insert` is an independent CRDT op, and Y.Array keeps **both** sequences. The + merged layout holds two `(version …)`, two `(paper …)`, two `(lib_symbols …)`, and + **two `{item: uuid}` slots per root item**. + +## Consequences + +- `docToFile` renders every root item **twice** (each `{item}` slot resolves; the + per-render `seen` set only guards cycles within one path, not repeated slots) and + duplicates the preamble forms → the materialized file is invalid or at best + semantically doubled. +- ydoc-mode opens materialize this corruption directly. +- **Nothing heals it**: `ydocHasState` is now true, so no client ever re-seeds; + `applyDeltaToY` only appends/removes individual root slots, it never rewrites the + layout. The duplication is permanent for the room's lifetime. +- If the two seeders had *different* file versions (one stale), items also interleave + arbitrarily per-key — messier still, but the layout duplication alone is enough to + corrupt. + +## Fix directions (pick one) + +1. **Server-side seeding (cleanest):** the sync server creates/initializes the room + document from the stored file exactly once (it already owns the `.ydoc` blob); + clients never file-seed. Kills the race by construction, and also removes the + client's seed-authority special cases. +2. **Client-side seed arbitration:** write a random `seedNonce` into `kdoc_meta` inside + the same `docToY` transaction. After the transaction has round-tripped (next sync / + short settle), re-read the nonce: LWW means exactly one seeder "won". A loser that + sees a foreign nonce re-runs `docToY`? No — re-running re-inserts. The loser must + instead **retract its own layout inserts** (delete slots whose insertion client-id + is its own) or simply rewrite layout in a fresh transaction *after* observing the + winner's state (delete-all + insert is safe once only one client does it, so gate + the rewrite on "my nonce lost"). +3. **Read-side dedup (mitigation, not a fix):** `yToDoc` / `docToFile` could drop + duplicate `{item}` slots and duplicate preamble heads. This masks the corruption for + materialization but leaves the doc itself dirty; only worth doing as a safety net on + top of 1 or 2. + +Option 1 is recommended — it also solves stale-file double-seeding and removes the +`settleMs` heuristic from the BroadcastChannel path. + +## Verification + +Unit: two Y.Docs, `docToY` the same `KicadDoc` into both, sync updates both ways, +assert `docToFile` output equals the single-seed output (currently it doesn't — layout +doubles). Integration: open the same fresh project in two tabs simultaneously +(Promise.all in the harness) and assert the room materializes cleanly. + +Repro (2026-07-03): unit in `web/pcbjam-shared/test/ysync-repros.test.ts` (doubled +`{item}` slot + materialization ≠ single-seed, plus a green CRDT-determinism baseline +— both docs corrupt IDENTICALLY, which is why nothing downstream notices); e2e race +in `tests/kicad/ysync-two-tab.spec.ts` (Promise.all start, equal settleMs, +skip-guarded when the race happens not to fire — it fired on every observed run). +See [16](16-repro-suite-results-and-empirical-findings.md). diff --git a/docs/features/ysync-review/07-bug-sheet-switch-stale-down-hook.md b/docs/features/ysync-review/07-bug-sheet-switch-stale-down-hook.md new file mode 100644 index 0000000..6b742fb --- /dev/null +++ b/docs/features/ysync-review/07-bug-sheet-switch-stale-down-hook.md @@ -0,0 +1,85 @@ +# Bug 07 — Sheet switch leaves the DOWN hook pointing at the old room; cross-room contamination window + +**Severity:** medium (small window on warm rooms; a full network round-trip — or forever — on cold/failed switches) +**Status:** open + +## Where + +- `web/standalone/src/wasm/collab/kicad-binding.ts:191-195` — `KicadBinding.destroy()` + only calls `items.unobserveDeep(observer)`; the DOWN hook registered via + `bridge.onItems(...)` (→ `window.kicadCollab.onItems`, `moduleItemsBridge` + `:216-222`) is **never unregistered** +- `web/standalone/src/wasm/collab/sheet-manager.ts:158-203` — `doSwitch`: destroys the + old binding synchronously, then `await ensureRoom(sheetPath)` (network for a cold + room), then `bindKicadCollab` re-registers `onItems` +- UP-side mirror: `wasm/bindings/eeschema_embind.cpp:935-943` — a queued + `schCollabApplyItems` CallAfter applies to `aFrame->GetScreen()` — whatever sheet is + active **when it runs**, not when it was queued + +## The DOWN-side hole (main issue) + +Between `old.binding.destroy()` and `bindKicadCollab(room.doc, bridge)` there is an +async gap. During that gap `window.kicadCollab.onItems` still points at the **old +binding's closure**, which writes into the **old sheet's Y.Doc**. + +The C++ side has already rebaselined to the new screen (`OnSchSheetChanged` → +`rebaseline()` fires from `DisplayCurrentSheet` before the JS switch completes), so a +local edit in the gap emits a *new-sheet-scoped* diff — and the stale hook applies it +to the *old* room: + +- the old room's doc gains the new sheet's items (`applyDeltaToY` upserts them and + appends root layout slots); +- peers bound to the old room receive them and **add the wrong sheet's items to their + editor screens**; +- the old sheet's materialized file now contains foreign items. + +Window size: + +- **Warm room** (already in the pool): one microtask — tiny but nonzero. +- **Cold room** (`switchTo` before `connectAll` finished warming it): a full + provider connect + `whenSynced()` round-trip. +- **Failed switch** (`ensureRoom` throws — network down): `doSwitch` aborts with + `activePath = null` and **no retry**; the stale hook stays live indefinitely, and + every subsequent edit on the new sheet flows into the old room until the user + navigates again successfully. +- **Coalesced rapid navigation**: superseded switches are skipped + (`requestedPath !== sheetPath`), correctly — but the hook keeps pointing at the last + *bound* room, which may be several sheets back, until the final switch completes. + +## The UP-side mirror (smaller) + +An `applyItems` already queued into the C++ CallAfter pipeline before a navigation +lands on the **new** screen: `doApplyItems` resolves `existing` hierarchy-wide (fine) +but `commit.Add(item, aFrame->GetScreen())` targets the now-active sheet — items from +sheet A's room can be added to sheet B's screen. Sub-frame window; lower priority than +the DOWN side but the fix below covers it too. + +## Fix direction + +1. **Detach the DOWN hook in `destroy()`**: give the bridge an `offItems()` (or have + `bindKicadCollab` install a wrapper that checks a `destroyed` flag and drops — or + buffers — emits). Dropping is acceptable only if the C++ baseline is rolled back; + otherwise the edit silently never syncs (a mini version of bug 05). Better: + **buffer** emits while unbound and let the next `seed()`'s adopt/baseline pass + reconcile them (the adopt already reconciles editor↔doc wholesale, so buffered + emits can simply be discarded *after* a successful adopt-bind — the adopt reads the + editor's current truth). +2. **Generation-tag the apply path**: include the target sheet path (the room's file) + in the wire envelope and have `doApplyItems` verify it against + `currentScreen()->GetFileName()`, dropping mismatches. This closes both the UP-side + race and any residual DOWN-side echo. +3. **Retry / re-run failed switches**: on `ensureRoom` failure, keep `requestedPath` + and re-attempt (with backoff) instead of leaving the editor unbound. + +## Verification + +Sheet-manager unit test: destroy old binding, delay `ensureRoom` (fake provider), +fire `onItems` during the gap, assert the old doc did NOT change. Integration: +throttle the network, navigate to a cold subsheet and immediately draw a wire; assert +the wire lands in the new sheet's room only. + +Repro (2026-07-03): `web/standalone/src/wasm/collab/ysync-repros.test.ts` — 07a +(post-`destroy()` emit must not write into the doc) and 07b (REAL sheet-manager + +REAL kicad-binding + REAL yjs with only `connectKicadDoc` faked; the cold-switch gap +is held open and the stale hook's emit lands in the old sheet's doc). Both `it.fails`. +See [16](16-repro-suite-results-and-empirical-findings.md). diff --git a/docs/features/ysync-review/08-miss-layout-state-never-syncs.md b/docs/features/ysync-review/08-miss-layout-state-never-syncs.md new file mode 100644 index 0000000..6649c88 --- /dev/null +++ b/docs/features/ysync-review/08-miss-layout-state-never-syncs.md @@ -0,0 +1,70 @@ +# Design miss 08 — Non-item document state only syncs at seed; `lib_symbols` is a landmine for the symbol-libraries milestone + +**Severity:** design gap (silent divergence for settings edits; structural blocker later) +**Status:** open decision + +## Where + +- `web/pcbjam-shared/src/kicad-y.ts:79-94` — `kdoc_layout` is written by `docToY` + (seed) and only ever *appended to / pruned* by `applyDeltaToY` for root **items**; + non-item layout slots are never updated live +- `web/pcbjam-shared/src/items-wire.ts:85-117` — `unwrapWireItem` deliberately strips + the blob's envelope, including a symbol blob's `(lib_symbols …)` cache ("sender + context, not document content") +- `wasm/bindings/eeschema_embind.cpp:786-808` — `doApplyItems`' `findLib` fallback + chain: blob's own lib cache (stripped upstream) → live screen's cache → nullptr +- Drift flags: `web/standalone/src/wasm/collab/drift-detect.ts:110-115` + (`layoutChanged` / `metaChanged` are computed and reported — but nothing consumes + them beyond telemetry) + +## What doesn't sync (by design, today) + +Everything that lives in `layout` (non-uuid forms at the document root): + +- eeschema: title block, paper size/orientation, `(settings …)`, `lib_symbols` +- pcbnew: `(setup …)` (design rules), net declarations (`(net N "NAME")` at root), + layer table, title block / paper + +A peer editing the title block or board setup diverges silently from the room and +from other peers; drift-detect reports `layoutChanged` forever and nothing repairs it +([10-miss-no-repair-path.md](10-miss-no-repair-path.md)). Because ydoc-mode opens +materialize from the doc, the *author's own* settings edit is lost on the next reload +(the saved file went to the API, but the doc wins on open — see the save-vs-room note +in [10](10-miss-no-repair-path.md)). + +## The `lib_symbols` landmine + +Today symbol placement is blocked (no bundled symbol libraries), which masks this. +When that lands: + +1. Sender places a symbol → the emit's clipboard blob carries the symbol AND its + `(lib_symbols …)` definition (that's what `aForClipboard` Format does). +2. `unwrapWireItem` **strips** the `lib_symbols` envelope → the definition never + enters the Y.Doc. +3. Peers apply the symbol; `findLib` falls back to the live screen's cache — which + doesn't have the definition for a symbol the peer has never seen → `nullptr` → + symbol added without a `LIB_SYMBOL` (renders broken). +4. Worse, persistently: the room's `layout.lib_symbols` still has only the seed-time + definitions, so `docToFile` produces a schematic referencing a lib id it doesn't + contain — an invalid file. + +## Fix directions + +- **Short term (before symbol placement ships):** stop stripping `lib_symbols` on the + eeschema wire. Either merge the blob's definitions into a dedicated + `kdoc_libsymbols` Y.Map (keyed by lib id, LWW per definition — definitions are + content-addressed-ish and rarely conflict), or fold them into the layout's + `lib_symbols` slot on upsert. Mirror on apply: render the definitions into the wire + so `findLib`'s first branch works. +- **Layout state generally:** decide per class: + - *Settings/title block:* add a coarse "layout rev" sync — on local save (the + existing `onSave` hook) diff the saved file's layout against the doc's + (`docDelta` covers items; layout needs a slot-list compare, which + `drift-detect` already does) and write changed non-item slots into `kdoc_layout` + with LWW-at-slot-head granularity. Coarse but converging. + - *Net declarations (pcbnew):* must be kept in step if/when net-creating edits are + possible in the standalone; otherwise document explicitly that nets are + seed-frozen. +- **Minimum bar if deliberately deferred:** document the freeze and make drift-detect's + `layoutChanged` distinguish "expected class" (title block) from "unexpected" + (missing lib_symbols), so telemetry stays actionable. diff --git a/docs/features/ysync-review/09-miss-undo-not-collab-aware.md b/docs/features/ysync-review/09-miss-undo-not-collab-aware.md new file mode 100644 index 0000000..461212f --- /dev/null +++ b/docs/features/ysync-review/09-miss-undo-not-collab-aware.md @@ -0,0 +1,56 @@ +# 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 + +## 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`. diff --git a/docs/features/ysync-review/10-miss-no-repair-path.md b/docs/features/ysync-review/10-miss-no-repair-path.md new file mode 100644 index 0000000..e2bb1df --- /dev/null +++ b/docs/features/ysync-review/10-miss-no-repair-path.md @@ -0,0 +1,60 @@ +# Design miss 10 — Drift is detected and reported, never repaired; saves and the room can diverge with the room winning + +**Severity:** design gap (turns every other bug from "transient" into "permanent") +**Status:** open decision + +## Where + +- `web/standalone/src/wasm/collab/drift-detect.ts` — computes the exact editor↔doc + delta (`docDelta(ydocDoc, wasmDoc)` + `layoutChanged`/`metaChanged`) every N doc + updates and at `beforeunload`, then… `reportDrift()` (telemetry) only +- `web/standalone/src/wasm/save-flow.ts` + `WasmTool.tsx:553,795` — `onSave` routes the + saved bytes to the API/local disk; it does **not** reconcile the saved state into + the Y.Doc +- `web/standalone/src/components/WasmTool.tsx:248-289` — ydoc-mode open prefers the + room (`docToFile(yToDoc(doc))`) over the API copy whenever the room has state + +## The two halves + +**1. No self-heal.** Every divergence source in this review — apply failures (blob +parse errors are logged and skipped), the projection gaps +([04](04-bug-lossy-change-detection.md)), swallowed cleanup +([05](05-bug-rebaseline-swallows-local-edits.md)), skipped child removals +([03](03-bug-child-removal-dangling-slot.md)) — ends as a drift report and stays +diverged for the rest of the session. The ironic part: `computeDrift` already holds +the exact repair payload. `docDelta(ydocDoc, wasmDoc)` *is* the delta that would make +the doc match the editor; applying it via `applyDeltaToY` (tagged with the local +origin) is precisely what the DOWN path does for normal edits. + +**2. Save vs room divergence, room wins.** The editor's Save writes the *true* model +(including everything the sync missed) to the API. The room is a separate store. On +the next ydoc-mode open, the room is preferred — so edits that drifted but were +correctly **saved** get silently dropped in favor of the stale doc. The author loses +work they explicitly saved. (When materialization *fails* — e.g. bug 03's dangling +slot — the API fallback accidentally preserves data better than the healthy path.) + +## Fix directions + +1. **Self-heal from the drift check (cheap, high leverage).** When `computeDrift` + finds an item-level diff, don't just report — apply `body.diff` into the Y.Doc as a + local-origin transaction (editor is the source of truth for *local* drift by + definition: the wasm model is what the user sees). Guardrails: + - only auto-apply the item diff; report-only for `layoutChanged`/`metaChanged` + until [08](08-miss-layout-state-never-syncs.md) is decided; + - cap the auto-applied delta size (a huge diff means something structural broke — + report and stop rather than bulk-rewrite the room); + - keep the report either way, flagged `repaired: true`, so telemetry still shows + the underlying bug frequency. + This one change converts bugs 03/04/05's permanent divergence into a bounded lag + (≤ N doc updates or session end). +2. **Reconcile on save.** The `onSave` hook has the saved file text in MEMFS; run the + same `fileToDoc` → `docDelta` → `applyDeltaToY` reconciliation there. Save is the + user's explicit "this is the state I mean" signal — it's the natural sync barrier, + and it fixes the save-vs-room precedence problem at the same time (after a save, + the room *equals* the saved file, so "room wins on open" becomes harmless). +3. **Precedence tie-break on open (defense in depth).** ydoc-mode open could compare + the API copy's mtime/content against the room and at least *warn* (or prefer the + newer) when they disagree materially, instead of unconditionally trusting the room. + +Recommendation: do 2 first (save is low-frequency, zero perf risk, biggest +user-visible win), then 1 with the size cap. diff --git a/docs/features/ysync-review/11-miss-no-v2-e2e-coverage.md b/docs/features/ysync-review/11-miss-no-v2-e2e-coverage.md new file mode 100644 index 0000000..0623177 --- /dev/null +++ b/docs/features/ysync-review/11-miss-no-v2-e2e-coverage.md @@ -0,0 +1,86 @@ +# Design miss 11 — The production v2 stack has no end-to-end test; the two-tab e2e exercises the legacy wire + +**Severity:** process gap (root cause of why bug 01 shipped) +**Status:** CLOSED 2026-07-03 — the v2 port landed (see the status update at the +bottom and [16](16-repro-suite-results-and-empirical-findings.md)); only the legacy +retirement remains open + +## Where + +- `tests/collab/browser-entry.ts:8` — the collab e2e bundle exports the **legacy** + `startCollab` (scalar reconciler, `kicadCollabSnapshot`/`Apply`/`onDelta`, Y key + `"items"`) +- `tests/kicad/{eeschema,pcbnew,pl_editor}-collab.spec.ts` — the two-tab convergence + specs all drive that bundle +- Production runs the **v2** items path: `bindKicadCollab` + `SheetCollabManager` + (`web/standalone/src/wasm/collab/`), Y keys `kdoc_*`, C++ + `kicadCollabSnapshotItems`/`ApplyItems`/`onItems` +- v2 coverage today: vitest unit tests only (`kicad-binding.test.ts`, + `sheet-manager.test.ts`) against a **fake** JS bridge + +## Why this matters + +The fake-bridge unit tests validate the TS state machine but structurally cannot see +C++-side integration behavior: + +- listener registration living inside `snapshotItems()` + ([01-bug-first-tab-listener-never-registered.md](01-bug-first-tab-listener-never-registered.md) + — invisible to a fake by construction); +- blob content fidelity (pad net zeroing, + [02](02-bug-footprint-blob-zeroes-pad-nets.md)); +- which edits the C++ differ actually detects + ([04](04-bug-lossy-change-detection.md)); +- CallAfter/coroutine ordering races ([05](05-bug-rebaseline-swallows-local-edits.md), + [07](07-bug-sheet-switch-stale-down-hook.md)). + +Meanwhile the legacy path the e2e *does* cover is dead in production (nothing +registers `onDelta`; `WasmTool` binds `onItems` only). + +## What to build + +Port the two-tab convergence e2e to the v2 stack (the harness pattern already exists; +swap the bundle to export `startKicadCollab` / `attachKicadCollab`): + +1. **Fresh-room seed test** (would have caught bug 01): tab A opens with a room id + that has never existed, edits, tab B joins and must converge — **in that order** + (A seeds via the file-seed branch, then sends). +2. **Adopt test**: tab B opens with a cold never-saved copy, must adopt A's identity + (already covered in units; cheap to assert e2e). +3. **Edit matrix per tool** (grows with bug 04's fix): move, rotate symbol, edit value + text, footprint move with netted pads (assert nets survive — bug 02), delete a + footprint user field (bug 03), draw + delete items. +4. **eeschema sheet navigation**: two sheets, edits on both, navigate while the peer + edits; assert per-room scoping and catch-up on revisit (sheet-manager + + bug 07 regression). +5. Assert **drift-detect silence** at the end of every scenario — it's a free, + high-signal convergence oracle (`computeDrift() === null` means editor ≡ doc). + +Once these exist, retire the legacy bundle and specs together with the legacy wire +(they currently provide a false sense of coverage), or keep exactly one legacy spec +until the C++ `emit()`/scalar path is deleted. + +## Note on cadence + +Per project practice, run the new specs in all three engines (Firefox + Chromium + +WebKit) like the rest of the kicad e2e suite. + +## Status update (2026-07-03) + +Landed as `tests/kicad/ysync-two-tab.spec.ts` + +`tests/kicad/ysync-repros-{pcbnew,eeschema}.spec.ts` over the new +`apps/kicad/collab-bundle-v2.js` (`tests/collab/browser-entry-v2.ts`). Against the +build list above: + +1. **Fresh-room seed test** — done (pcbnew + eeschema, `test.fail` on bug 01; + Chromium-only per finding F2 in [16](16-repro-suite-results-and-empirical-findings.md)). +2. **Adopt test** — done, green (pl_editor divergent-uuid cold copy adopts the doc's + identity through the real C++ apply). +3. **Edit matrix** — landed as the bug-02/03/04 expected-fail repros; turns green + case-by-case as bug 04's fix lands. Zone-net still needs a hook. +4. **eeschema sheet navigation** — NOT ported (navigation isn't driven headless yet); + the mechanism is covered by the bug-07b unit repro instead. +5. **Drift-detect silence** — done on the green pl scenarios, ITEM-level (strictly + silent — finding F4); layout/meta flags stay informational until miss 08. + +Still open from this doc: retiring the legacy bundle + specs. Finding F3 (headless +emit works on BOTH tools) removes the last excuse for the legacy two-tab skips. diff --git a/docs/features/ysync-review/12-opt-hot-path-full-model-work.md b/docs/features/ysync-review/12-opt-hot-path-full-model-work.md new file mode 100644 index 0000000..955cded --- /dev/null +++ b/docs/features/ysync-review/12-opt-hot-path-full-model-work.md @@ -0,0 +1,60 @@ +# Optimization 12 — O(full-model) work on every edit, apply, and remote batch + +**Severity:** performance (fine on demo boards; seconds-per-edit territory at 5–10k items) +**Status:** open + +## The costs, per hot path + +**Every local commit (C++):** +- `flushDiff` runs `snapshotByUuid` — `itemToJson` for **every** item on the + board/screen (json allocation per item) — plus a full map compare against + `g_baseline` (`pcbnew_embind.cpp:546-668`, eeschema `:374-453`). +- The legacy scalar wire is still built and emitted alongside the v2 wire even though + production registers no `onDelta` listener (the json arrays are constructed + regardless; the `EM_ASM` no-ops). + +**Every remote apply (C++):** +- `rebaseline()` at the end of `doApply`/`doApplyItems` — another full-model + `snapshotByUuid`. + +**Every remote batch (TS):** +- The `observeDeep` handler builds `itemsView()` — `yToItem` for **every** item, each + going through a zod `.parse` of its entire body tree + (`kicad-binding.ts:82-88,116`). zod validation dominates; on a large board this is + by far the most expensive step, and it runs even for a single-item remote nudge. +- `deltaToItemsWire`'s `coveredByAncestor`/`renderItem` only need the delta's items + plus their ancestor chains and descendant subtrees — not the full view. +- `itemsWireToDelta` → `descendants()` rebuilds the full children index per wire item + (`items-wire.ts:63-83`) → O(n·m) for an m-item wire. + +**Drift check:** full scratch save + `fileToDoc` + `yToDoc` + `docDelta` — but only +every 50 doc updates and at unload; acceptable as designed. (The `beforeunload` check +is synchronous full-model work and will add visible tab-close latency on big boards — +worth a size guard, not a redesign.) + +## Fixes, in leverage order + +1. **Dirty-set + blob-hash diffing on the C++ side** — the same change bug + [04](04-bug-lossy-change-detection.md) needs for correctness. Collect touched-item + uuids from the listener callbacks (currently discarded), lift children to roots, + and post-settle compare only those roots' blob hashes against a uuid→hash baseline. + Replaces both full snapshots (flush AND rebaseline — see + [05](05-bug-rebaseline-swallows-local-edits.md) for the targeted-rebaseline tie-in) + and retires the scalar snapshot + legacy emit entirely. Per-edit cost drops from + O(board) to O(edit). +2. **Kill zod on the TS hot path.** `yToItem`'s schema parse guards against malformed + Y content, but the observer path re-validates the *entire* model on every batch. + Either: + - cache the materialized `KicadItem` per item Y.Map (WeakMap keyed by the Y.Map, + invalidated from the event's changed keys), so a batch only converts touched + items + the subtrees `renderItem` walks; or + - validate at trust boundaries only (wire parse already zod-validates; Y reads can + use a cheap structural cast) and keep zod for seed/materialize paths. +3. **Scope the view to the delta.** `deltaToItemsWire` needs `view` for ancestor + chains and descendant rendering; build it lazily (resolve `items.get(uuid)` on + demand) instead of materializing the whole map up front. +4. **Index children once per conversion.** Build the parent→children map once per + `itemsWireToDelta` call, not per wire item. + +Items 2–4 are contained TS changes; item 1 is the structural one and pairs with the +bug-04/05 fixes — do them as one piece of work. diff --git a/docs/features/ysync-review/13-opt-parked-dirty-full-sheet-replace.md b/docs/features/ysync-review/13-opt-parked-dirty-full-sheet-replace.md new file mode 100644 index 0000000..52c5c85 --- /dev/null +++ b/docs/features/ysync-review/13-opt-parked-dirty-full-sheet-replace.md @@ -0,0 +1,56 @@ +# Optimization 13 — Parked-dirty sheet rebind re-applies the entire sheet instead of the delta + +**Severity:** performance + UX (heavy on big sheets; creates the adopt undo-bomb) +**Status:** open + +## Where + +- `web/standalone/src/wasm/collab/sheet-manager.ts:188-191` — a sheet revisited after + any remote traffic while parked (`room.dirty`) is re-seeded with + `binding.seed(undefined, { editorMatchesDoc: false })` +- `web/standalone/src/wasm/collab/kicad-binding.ts:176-188` — that adopt branch + renders **ALL** doc roots (`renderItem` per root) and sends them as one + `applyItems` batch; C++ `doApplyItems` then does remove+add for **every** item on + the sheet, in one commit + +## What it costs + +A single remote edit to a parked sheet marks it dirty; the next `switchTo` then: + +- renders every root item's full subtree s-expr from the Y view (O(sheet)); +- ships one giant wire batch across embind; +- C++ parses every blob through the clipboard-paste path, removes and re-adds every + item, runs one `Push` with full connectivity/ERC recompute; +- produces **one undo entry containing the whole sheet** + ([09-miss-undo-not-collab-aware.md](09-miss-undo-not-collab-aware.md)) and resets + selection/view state for everything. + +The warm-pool design already pays the memory/connection cost to keep parked docs +current precisely so switches are cheap — then throws that away by re-applying +everything instead of the accumulated difference. + +## Fix + +The parked room already knows what changed — `startWatch` marks `dirty` on every +update. Two escalating options: + +1. **Accumulate the parked delta.** Instead of a boolean `dirty`, buffer the + uuid set of changed items while parked (the `update` event's transaction carries + changed keys via `Y.decodeUpdate`, or cheaper: attach the standard + `observeDeep` → `deltaFromYEvents` pipeline to the parked doc and accumulate into + a pending `KicadDelta`, coalescing per uuid). On rebind, convert just that delta + with `deltaToItemsWire` and apply it — identical code path to a live remote batch. +2. **Diff on rebind (no bookkeeping).** On rebind, parse `bridge.snapshotItems()` + (already called for baselining), convert with `itemsWireToDelta` against the doc + view, and apply only the resulting difference in the *editor* direction (doc + authority: added/updated from doc side, removed for editor-only items). This is a + generalization of the existing adopt that degrades gracefully — the empty diff + case becomes the current "clean revisit" baseline-only branch, and the code paths + unify. + +Option 2 is more robust (it also self-corrects any drift the parked bookkeeping might +miss) and reuses existing conversion machinery; its cost is one editor snapshot per +rebind, which the seed already pays today. + +Either way, the giant single-commit adopt shrinks to the real changed set, which also +shrinks the undo entry and the selection churn. diff --git a/docs/features/ysync-review/14-opt-item-granularity-bandwidth.md b/docs/features/ysync-review/14-opt-item-granularity-bandwidth.md new file mode 100644 index 0000000..a40fdad --- /dev/null +++ b/docs/features/ysync-review/14-opt-item-granularity-bandwidth.md @@ -0,0 +1,60 @@ +# Optimization 14 — Item-level body granularity: whole-item payloads per nudge, LWW drops concurrent property edits + +**Severity:** known/documented v1 tradeoff — recorded here so its costs are visible when prioritizing +**Status:** open (deliberate design decision; revisit trigger below) + +## Where + +- `web/pcbjam-shared/src/kicad-y.ts:23-26` — the documented v1 choice: an item's + `body` is ONE plain-JSON value → item-level merge; "deep Y types per slot + (field-level merge inside one item) are the later refinement" +- `upsertYItem` (`kicad-y.ts:60-72`) — any body change rewrites the whole body value + +## Costs as shipped + +1. **Bandwidth / update-log growth ∝ item size, not edit size.** Nudging a footprint + 1 mm re-writes its full flattened subtree into the Y.Doc: the update ships the + whole body JSON, and the sync server persists it in the room's update log until + compaction. For a 100-pad footprint that's kilobytes per nudge. (The flatten + already helps a lot — pads/fields/pins are separate items, so a *child* edit only + re-writes the child + the lifted parent body — but the parent body itself embeds + `{item}` refs plus all non-item slots, and pcbnew's `liftBlob` re-emits the parent + for every child change.) +2. **Last-writer-wins at item granularity.** Two peers concurrently editing two + *different properties* of the same item (one moves a text, the other edits its + string) resolve by Yjs LWW on `body` — one peer's edit is silently discarded. + Convergent, but lossy in exactly the case CRDTs are chosen for. The flatten means + this only bites *within* one item (concurrent pad edits on the same footprint are + fine — different items), which is why it's been acceptable so far. +3. Body comparison is `JSON.stringify` equality (`upsertYItem`, `sameKicadItem`) — + fine at current sizes; becomes part of the hot-path cost at scale + ([12-opt-hot-path-full-model-work.md](12-opt-hot-path-full-model-work.md)). + +## The refinement path (when justified) + +The `kicad-doc.ts` header already sketches it: map each `body`/`v` slot list to a +Y.Array of slot Y.Maps instead of one JSON value. + +- Concurrent different-slot edits merge instead of LWW-dropping. +- Updates ship only changed slots. +- The zod schema remains the post-merge structural check (this is *why* slot lists + were designed as uniform ordered arrays — the shape is already CRDT-ready). + +Costs to respect: + +- ordered-list CRDT semantics introduce interleaving anomalies for concurrent inserts + at the same position (slot order is file order — usually stable, so low risk); +- the conversion layer (`itemsWireToDelta` / `deltaToItemsWire` / `yToItem`) must + become slot-diff-aware — a meaningful rewrite of `kicad-y.ts`'s write path; +- per-slot Y overhead (item count × slot count Y structs) raises baseline doc size. + +## Revisit trigger + +Not worth doing speculatively. Revisit when either: + +- server-side room storage / bandwidth per session becomes a measured cost, or +- concurrent same-item edits become a real reported UX complaint (e.g. two people + routing in the same area fighting over one track's endpoints), or +- the [04](04-bug-lossy-change-detection.md)/[12](12-opt-hot-path-full-model-work.md) + rework lands — that change touches the same conversion layer, and doing the slot + refinement then amortizes the rewrite. diff --git a/docs/features/ysync-review/15-plan-repro-tests-and-v2-e2e.md b/docs/features/ysync-review/15-plan-repro-tests-and-v2-e2e.md new file mode 100644 index 0000000..f5ac853 --- /dev/null +++ b/docs/features/ysync-review/15-plan-repro-tests-and-v2-e2e.md @@ -0,0 +1,168 @@ +# Reproduction tests for ysync bugs 01–07 + v2 e2e coverage (miss 11) + +## Context + +The 2026-07-02 Yjs⇄KiCad sync review (docs: `../kicad-wasm-ysync-review/docs/features/ysync-review/`) +found 7 bugs, and found that the two-tab e2e exercises only the DEAD legacy scalar wire +(`tests/collab/browser-entry.ts` → `startCollab`), while production runs the v2 items wire +(`bindKicadCollab` / `startKicadCollab` + `kicadCollabSnapshotItems`/`ApplyItems`/`onItems`). +That coverage gap is why bug 01 shipped. + +This task: (a) a reproduction test for every bug 01–07, (b) port the two-tab e2e to the v2 +stack (miss 11). **Out of scope:** optimizations 12–14, misses 08–10, fixing the bugs +themselves, retiring the legacy specs. + +**Convention:** every repro test asserts the CORRECT behavior and is marked expected-fail +(vitest `it.fails`, playwright `test.fail()`) with a comment naming the bug doc. The suite +stays green; fixing a bug flips the test to "unexpected pass", forcing the marker's removal +— the repro becomes the regression test. Expected-fail e2e polls use short timeouts (6–8 s) +so they don't burn the clock. + +Key facts verification established (why this design works): +- `kicadCollabTestMoveFirst` AND `kicadCollabApplyItems` are both CallAfter-deferred + (pcbnew_embind.cpp:1098, :960), and `scheduleFlush` queues behind them (:686) → the bug-05 + swallow is deterministically reproducible from one JS turn: queue move, then apply → drain + order is [move, apply, flush] → apply's global rebaseline swallows the move → flush empty. +- pl_editor's emit is an eager `OnModify` hook (pl_editor_embind.cpp:72), NOT the lazy + `ensureBridge` listener → bug 01 does NOT manifest there; its e2e repro must run on + eeschema/pcbnew. pl_editor is the green-baseline tool for the v2 two-tab harness (its + local-edit emit is already proven headless in `items-bridge.spec.ts` PL `localEdit`). +- Both legacy two-tab `test.skip`s cite a rationale the code itself documents as stale + (eeschema-collab.spec.ts:111-113 note: "predated the dyncall-shim fix — apply now works"). + Whether ee/pcb local-edit **emit** works headless is unverified → Phase C probe decides + live-vs-fixme for the emit-dependent repros. + +## Per-bug repro matrix + +| Bug | Unit (vitest, no wasm) | E2E (playwright, real C++) | +|-----|------------------------|----------------------------| +| 01 listener never registered | binding + C++-faithful fake (emit gated on snapshotItems) — file-seed then local edit must reach peer | two-tab v2 fresh-room on eeschema+pcbnew: A file-seeds → A edits → B must receive | +| 02 blob zeroes pad nets | — (C++-only) | single-tab pcbnew: `snapshotItems()` footprint blob must contain `(net 1 "SIG")` on pads | +| 03 dangling child slot | `applyDeltaToY({removed:[child]})`, parent survives → `renderItem(parent)`/`docToFile` must work, parent body slot pruned | (a) receiving half: `ApplyItems({removed:[childUuid]})` → saved board must lose the fp_text (today: kept); (b) Y half: feed the same wire into the real binding via `window.kicadCollab.onItems` in the two-tab spec → `docToFile(yToDoc)` must not throw; (c) sending half (needs new hook): remove child → emitted wire must carry parent re-blob, not bare removal | +| 04 lossy change detection | — (C++-only) | single-tab emit matrix w/ control (needs new hooks): control move (detected) proves harness, then rotate / field-text / pad-size / endpoint-drag each must emit its uuid | +| 05 rebaseline swallows edits | — (CallAfter ordering is C++-only) | single-tab pcbnew: same-JS-turn `TestMoveFirst` then `ApplyItems`(unrelated item) → moved uuid must appear in captured `onItems` wire | +| 06 concurrent seed duplicates layout | two Y.Docs, `docToY` independently, exchange updates → `docToFile(yToDoc(a))` must equal single-seed output | two-tab v2: both tabs start simultaneously on a fresh room (equal settleMs) → materialized doc clean | +| 07 stale DOWN hook | (a) `destroy()` then fire the captured `onItems` cb → doc must be unchanged; (b) sheet-manager gap: real binding + real Y.Docs, delayed `connectKicadDoc` mock, fire `win.kicadCollab.onItems` in the gap → old sheet's doc unchanged | — (unit covers the mechanism; e2e navigation can't be driven headless today) | + +## Files + +### Phase A — vitest unit repros (no wasm, run immediately) + +1. **`web/pcbjam-shared/test/ysync-repros.test.ts`** (new) — bugs 03, 06 against + `src/kicad-y.ts` / `src/kicad-doc.ts`. Reuse the `sexprToItems`/`fileToDoc` fixture style + from `test/kicad-y.test.ts`. Bug 06 uses a footprint+preamble `KicadDoc`; assert both + `docToFile` equality and single `{item}` slot per root uuid. +2. **`web/standalone/src/wasm/collab/ysync-repros.test.ts`** (new) — bugs 01, 07. + - Bug 01: a C++-faithful `FakeEditor` variant (copy the shape from `kicad-binding.test.ts`, + add `snapshotCalls` counter + `emit` gated on `snapshotCalls > 0`, mirroring + `ensureBridge`). `bindA.seed(fileToDoc(file))` on an empty relayed pair → `edA.localUpsert` + → `it.fails(expect edB.store to have received it)`. Plus a direct + `expect(edA.snapshotCalls).toBeGreaterThan(0)` assertion (the one-line fix's contract). + - Bug 07a: `bindKicadCollab(doc, fake)`; `binding.destroy()`; fire the fake's captured + onItems cb with an `added` wire → `it.fails(expect items map empty)`. + - Bug 07b: `vi.mock("./index")` only (real `kicad-binding`, real yjs — note the standalone + vitest config's `dedupe: ["yjs"]`); manager `switchTo(a)` → `switchTo(b)` with a + deferred `connectKicadDoc` promise; during the gap fire `win.kicadCollab.onItems` with an + edit wire; `it.fails(expect sheet-a doc unchanged)`. + +### Phase B — v2 e2e harness + specs runnable on the CURRENT wasm build + +3. **`tests/collab/browser-entry-v2.ts`** (new) — bundles the v2 runtime: + `startKicadCollab` (from `web/standalone/src/wasm/collab/index`), wrapped as + `window.KicadCollabV2.start(mod, win, { room, settleMs, seedText? })` (BroadcastChannel + provider, `seedDoc: seedText ? fileToDoc(seedText) : undefined`, handle stored on + `window.__collabV2` for in-page Y assertions). Also export helpers for in-page asserts: + `fileToDoc`, `docToFile`, `yToDoc`, `docDelta`, `isEmptyKicadDelta`, and a pure + `driftReport(saveFnName, path)` that replicates `computeDrift`'s core from + `drift-detect.ts:94-118` using only `@pcbjam/shared` exports (don't import `drift-detect` + itself — it pulls `@/lib/api`). +4. **`tests/collab/build.mjs`** (edit) — second esbuild entry → `apps/kicad/collab-bundle-v2.js`. + `@pcbjam/shared` resolves to `web/pcbjam-shared/src/index.ts` (exports map). Its runtime + deps (`zod`, `@ts-rest/core`, `yjs`) must resolve in CI where `web/node_modules` is absent + (the reason legacy added `yjs` to tests devDeps) → add `zod` + `@ts-rest/core` to + `tests/package.json` devDependencies; keep `nodePaths: [tests/node_modules]`. +5. **`tests/kicad/ysync-two-tab.spec.ts`** (new) — the miss-11 port, harness pattern copied + from the legacy two-tab blocks (boot via the existing `bootAndOpen` shapes, `addScriptTag` + the v2 bundle, per-worker room ids): + - **pl_editor, green (harness validation):** fresh room, A seeds (file-seed via `seedText`), + `kicadCollabTestAddText` on A → B receives (poll B's save output); B-side adopt; end with + `driftReport === null` on both tabs. + - **pcbnew + eeschema fresh-room (bug 01 repro, `test.fail`):** same flow with + `TestMoveFirst` as A's edit → B must receive. Gated on the Phase C probe (fixme if the + harness can't drive emits at all). + - **concurrent seed (bug 06 repro, `test.fail`):** both tabs `KicadCollabV2.start` via + `Promise.all` on a fresh room, equal settleMs → assert in-page + `docToFile(yToDoc(doc))` equals the single-seed rendering (compute reference by seeding a + third, fresh Y.Doc locally in-page from the same `seedText`). + - **bug 03 Y-half (`test.fail`):** in the pl_editor or pcbnew session, manually invoke + `window.kicadCollab.onItems('{"removed":[""]}')` (simulating the C++ emit the + bug doc proves is sent) → assert `docToFile(yToDoc(doc))` still succeeds and the parent + body carries no dangling slot. +6. **`tests/kicad/ysync-repros-pcbnew.spec.ts`** (new) — single-tab, items-bridge.spec.ts + driving style (no JS bundle needed except where noted): + - **bug 02 (`test.fail`):** fixture = current SAMPLE_PCB + `(net 1 "SIG")` + 2 pads on the + footprint carrying `(net 1 "SIG")`; `snapshotItems()` → footprint blob must contain + `(net 1 "SIG")`. + - **bug 03 receiving half (`test.fail`):** `ApplyItems({removed:[FP1_TXT]})` → poll save → + fp_text must be gone (today the parent-footprint guard keeps it). + - **bug 05 (`test.fail`, gated on probe):** register onItems capture (+ `snapshotItems()` + first, to register the listener + baseline), same-JS-turn `TestMoveFirst(…)` then + `ApplyItems()` → moved uuid must appear in a captured wire + within the poll window. Control variant in the same file: `TestMoveFirst` alone → emit + arrives (proves the harness; this is also the pcbnew emit probe made permanent). +7. **`tests/playwright-kicad.config.ts`** (edit) — add the three new spec filenames to + `BIG_MODULE_SPECS` (lines 87-108) so CI runs them on chromium-ci only. +8. **`tests/README.md`** (edit, short) — document the v2 bundle, the repro-marker convention, + and the bug-doc cross-references. + +### Phase C — headless-emit probe (decision gate, ~10 min) + +Run the bug-05 control from item 6 (pcbnew) and an eeschema `TestMoveFirst`+onItems capture: +- **Emit works** → keep bug-01/04/05 e2e repros as live `test.fail`; leave the legacy two-tab + skips as they are (removing them is follow-up material, noted in README). +- **Emit genuinely dead headless** → convert the emit-dependent repros to `test.fixme` with + the probe result in the comment; the unit repros remain the executable evidence for bug 01. + +### Phase D — new C++ test hooks (wasm layer) + docker rebuild + +9. **`wasm/bindings/pcbnew_embind.cpp`** (edit) — following the `pcbCollabTestMoveFirst` + CallAfter+COROUTINE pattern (:1080-1109), add + register: + - `kicadCollabTestRemoveItem(uuid)` — commit.Remove + Push (bug 03 sending half), + - `kicadCollabTestRotateItem(uuid, deg)` — rotate about own anchor (bug 04), + - `kicadCollabTestSetPadSize(uuid, w, h)` — pad property edit (bug 04), + - `kicadCollabTestMoveEndpoint(uuid, dx, dy)` — move a segment/shape END point only (bug 04). +10. **`wasm/bindings/eeschema_embind.cpp`** (edit) — same pattern: + - `kicadCollabTestRemoveItem(uuid)`, + - `kicadCollabTestRotateItem(uuid)` — rotate a symbol in place (bug 04), + - `kicadCollabTestSetFieldText(uuid, text)` — set a symbol field's text (bug 04). +11. **`tests/kicad/ysync-repros-pcbnew.spec.ts`** (extend) + **`ysync-repros-eeschema.spec.ts`** + (new) — the bug-04 matrix, each test = control move (proves emit) + target edit + (`test.fail` that its uuid is emitted): pcbnew rotate/pad-size/endpoint; eeschema + rotate/field-text (eeschema fixture gains a real symbol: minimal `lib_symbols` Device:R + + placed `(symbol …)` with field uuids). Bug 03 sending half: `TestRemoveItem(FP1_TXT)` → + `test.fail(emitted wire carries the parent footprint re-blob, not a bare child removal)`. +12. Rebuild: `docker/build.sh` (full kicad build; hooks are embind-only). Then run Phase D specs. + +## Sequencing + +A (units, immediate) → B (harness + current-wasm specs) → C (probe, adjusts B/D markers) → +D (C++ hooks + rebuild + matrix specs). Each phase lands runnable on its own. + +## Verification + +- Units: `pnpm --filter @pcbjam/shared test` and `pnpm --filter test` (from + `web/`) — new files all green (`it.fails` semantics). +- Bundle: `cd tests && npm run build:collab` produces both bundles. +- E2E: from `tests/`: `npm run test:kicad` (firefox) and + `npx playwright test --config=playwright-kicad.config.ts --project=chromium ysync-…` for the + new specs; report FULL summaries (passed/failed/flaky/skipped + expected-failures) per + project convention. Check `tests/logs/kicad/` on anything unexpected. +- Existing suites must stay green: `items-bridge.spec.ts`, `roundtrip.spec.ts`, legacy collab + specs, existing vitest files. +- No screenshot pass needed (no render-path changes — build/test-layer only). + +## Follow-ups (out of scope, noted in README) + +- Un-skip/retire the legacy two-tab specs once the v2 port is trusted (per miss 11). +- Update each bug doc's Verification section in the `ysync-review` worktree with its repro + test path (docs live on the `ysync-review` branch, separate commit there). diff --git a/docs/features/ysync-review/16-repro-suite-results-and-empirical-findings.md b/docs/features/ysync-review/16-repro-suite-results-and-empirical-findings.md new file mode 100644 index 0000000..43e02c6 --- /dev/null +++ b/docs/features/ysync-review/16-repro-suite-results-and-empirical-findings.md @@ -0,0 +1,133 @@ +# Repro suite results + empirical findings (2026-07-03) + +**Status:** plan [15](15-plan-repro-tests-and-v2-e2e.md) executed in full — every bug +01–07 has a runnable reproduction test, and the v2 items wire has end-to-end coverage +(miss [11](11-miss-no-v2-e2e-coverage.md) closed). This doc records what the suite +looks like, what each repro's failure was verified to be, and four things the RUNNING +system revealed that code-reading (docs 01–07) did not predict. + +## The suite + +Convention: a repro asserts the CORRECT behavior and is marked expected-fail +(vitest `it.fails` / playwright `test.fail()`) with a comment naming the bug doc. The +suite stays green while a bug is open; fixing it flips the repro to "unexpected pass", +forcing the marker's removal — the repro becomes the regression test. Green companion +tests pin each repro's preconditions (boot, apply path, emit path, "the edit really +landed"), so an expected failure can only come from the bug itself. Every expected +failure below was verified (JSON-reporter pass) to fail at its DOCUMENTED assert. + +| Bug | Unit repro | E2E repro | Verified failure site | +|-----|-----------|-----------|----------------------| +| [01](01-bug-first-tab-listener-never-registered.md) | `web/standalone/src/wasm/collab/ysync-repros.test.ts` (C++-faithful fake: emit gated on `snapshotItems`; snapshot-call contract + peer-never-receives; green B→A asymmetry control) | `tests/kicad/ysync-two-tab.spec.ts` pcbnew + eeschema fresh-room | Chrome: A's move lands (`80…→82…`), B never converges; the 8 s peer poll | +| [02](02-bug-footprint-blob-zeroes-pad-nets.md) | — (C++-only) | `tests/kicad/ysync-repros-pcbnew.spec.ts` "footprint blob preserves pad nets" | blob shows the pads present but WITHOUT `(net 1 "SIG")` — runtime-confirmed | +| [03](03-bug-child-removal-dangling-slot.md) | `web/pcbjam-shared/test/ysync-repros.test.ts` (`renderItem: missing item fld-1` through the dangling slot; `docToFile` same) | Y-half: `ysync-two-tab.spec.ts` (room stops materializing); receiving half + sending half: `ysync-repros-pcbnew.spec.ts` | recv: fp_text still present after `removed:[child]` apply; send: **no parent re-blob AND no wire at all** (finding F1) | +| [04](04-bug-lossy-change-detection.md) | — (C++-only) | `ysync-repros-pcbnew.spec.ts` (anchor-centred fp rotation, pad resize, gr_line endpoint drag) + `ysync-repros-eeschema.spec.ts` (symbol rotation, Value-field edit) | every case: the "edit landed" save-poll is green, the emit poll receives NOTHING | +| [05](05-bug-rebaseline-swallows-local-edits.md) | — (CallAfter ordering is C++-only) | `ysync-repros-pcbnew.spec.ts` same-JS-turn `TestMoveFirst` + `ApplyItems` | apply landed, move landed, moved uuid never emitted | +| [06](06-bug-concurrent-seed-duplicates-layout.md) | `web/pcbjam-shared/test/ysync-repros.test.ts` (two offline `docToY` + merge → `[{item:'fp-1'},{item:'fp-1'}]`; materialization ≠ single-seed; green CRDT-determinism baseline) | `ysync-two-tab.spec.ts` Promise.all start, equal settleMs (skip-if-race-missed guard; the race fired on every observed run) | merged render ≠ single-seed render | +| [07](07-bug-sheet-switch-stale-down-hook.md) | `web/standalone/src/wasm/collab/ysync-repros.test.ts` 07a (post-`destroy()` emit still writes the doc) + 07b (REAL sheet-manager + REAL yjs, held-open cold `connectKicadDoc`; gap emit lands in the old room) | — (unit covers the mechanism) | doc gains `seg-ghost` / old room gains `wire-b` | + +v2 e2e coverage (miss 11) beyond the repros — all GREEN: + +- **Harness baseline (pl_editor two-tab, fresh room):** A file-seeds, `TestAddText` + edits flow A→B and B→A, and the drift-detect oracle is ITEM-silent on both tabs + (finding F4). pl_editor is the green tool because its emit is the eager `OnModify` + hook, not the lazily-registered listener — bug 01 does not gate it. +- **Adopt:** a joiner that cold-opened a divergent-uuid copy adopts the doc's + identity through the real C++ apply (doc uuid present, divergent uuid gone). +- **Headless emit probes** (phase C): green on BOTH pcbnew and eeschema (finding F3). + +Infrastructure: `tests/collab/browser-entry-v2.ts` → `apps/kicad/collab-bundle-v2.js` +(the production `connectKicadDoc` + `attachKicadCollab` stack; in-page +`renderActiveDoc` / `singleSeedRender` / `driftReport` helpers; `yjs` aliased to ONE +copy — the two web pnpm workspaces otherwise bundle two instanceof-incompatible +instances). Local-edit hooks added to the wasm layer: +`kicadCollabTest{RemoveItem,RotateItem}` (both tools, dispatched in the merged image), +`kicadCollabTest{SetPadSize,MoveEndpoint}` (pcbnew), `kicadCollabTestSetFieldText` +(eeschema) — all real commits via CallAfter + COROUTINE +(`wasm/bindings/{pcbnew,eeschema,kicad_editor}_embind.cpp`). + +Final verified state (firefox, fresh `kicad_editor` build; 3 ysync files + +items-bridge, roundtrip, the three legacy collab specs, save-hook): +**39 passed / 0 failed / 0 flaky / 5 skipped** — skips = the two F2 firefox guards, +the two pre-existing legacy two-tab skips, one pre-existing roundtrip fixme. + +## Empirical findings (what the running system added to the review) + +### F1 — Bug 03's sending half is WORSE than documented: a child-only delete emits NOTHING + +Doc 03 predicted (from reading `flushDiff`) that deleting a footprint child emits the +bare `{removed:[childUuid]}` wire. Empirically (headless, listener registered, +baseline seeded): a real `BOARD_COMMIT` remove of the fp_text child mutates the model +(the save loses the child) but **never triggers a flush at all** — no wire of any +shape goes out. + +Evidence: the benign `PCB_VIA::GetWidth called without a layer argument` assert fires +once per `snapshotByUuid` pass (`itemToJson` reads the via's width), making it a free +tracer for baseline/flush activity. The working move-probe log shows it twice +(baseline + flush); the child-delete log shows it exactly once (baseline only) — +`flushDiff` never ran, so the listener never fired for this commit shape. + +Implication for the fix: the emit-side repair is not just "lift removals to a parent +re-blob" — the listener has to SEE a child-only removal commit first. Whatever +`BOARD_COMMIT::Push` does with a child remove (roll-up to a parent modify, a +different notification path, or an early-out) needs a look before the lift can work. +The repro (`a child deletion goes out as the parent's re-blob`) asserts the correct +end state either way, so it covers both layers of the fix. + +### F2 — Firefox cannot host two `kicad_editor` tabs in one context + +The bug-01 two-tab repros boot the merged ~180 MB `kicad_editor` twice in one browser +context (BroadcastChannel requires same-context). On Firefox (ARM Mac, serial, +isolated run) the SECOND tab's `#canvas` never appears — the same per-content-process +SpiderMonkey wasm budget `tests/playwright-kicad.config.ts` documents for x86 CI, +reached at 2× on arm64. Chrome runs both tabs in ~17 s. + +Consequence: those two tests carry +`test.skip(project === "firefox", …)` and run on `chromium-ci` in CI (already routed +via `BIG_MODULE_SPECS`) and `--project=chromium` locally. Anything future that needs +two simultaneous board/schematic editors (multi-tab collab e2e, presence tests) has +the same constraint. pl_editor two-tab is unaffected (small separate bundle). + +### F3 — Headless emit WORKS on both pcbnew and eeschema; the legacy skip rationale is dead + +The phase-C probes (register listener via `snapshotItems`, real `TestMoveFirst` +commit, capture `window.kicadCollab.onItems`) are GREEN headless on both tools. So: + +- the `eeschema-collab.spec.ts` / `pcbnew-collab.spec.ts` two-tab `test.skip` + rationale ("harness can't drive the emit") is now disproven for BOTH halves — + apply was already known to work, and emit demonstrably works too. The legacy + two-tab specs could be un-skipped today (or better, retired with the legacy wire — + the follow-up miss 11 already names); +- `items-bridge.spec.ts`'s SCH/PCB `localEdit` omission ("emit unverifiable + headless", `:47` and `:236`) is stale — both ToolCfgs can gain a `localEdit` and + make the emit leg green there as well; +- every emit-dependent repro stays a LIVE `test.fail` — no fixme conversions were + needed anywhere. + +### F4 — The drift-detect oracle is strictly item-silent on the green path + +After the pl_editor two-tab session (file-seed + live edits both ways), the replicated +`computeDrift` core reports ZERO item drift on both tabs — the file-seeded Y bodies +byte-match what the editor's writer serializes, at least for the pl fixture. This was +a real risk (a writer that normalizes formatting would make drift-detect +false-positive on every file-seeded room) and it did NOT materialize. The layout/meta +halves are NOT asserted — non-item state only syncs at seed +([08](08-miss-layout-state-never-syncs.md)) and preamble formatting is +writer-normalized, so those flags stay informational until 08 lands. + +### Harness note + +The shared `testLogger` fixture only captures the DEFAULT page's console; two-tab +tests create pages via `context.newPage()`, so their `hasAbort(testLogger)` guard is +vacuous (true of the pre-existing legacy two-tab specs as well). Worth folding into +the fixture if two-tab coverage grows. + +## Follow-ups + +- Fix-order input: F1 moves bug 03's emit-side fix partly into listener/commit + territory — budget for that when scheduling the [00-overview](00-overview.md) + attack order. +- Retire (or un-skip, briefly) the legacy two-tab specs per miss 11 + F3. +- Add `localEdit` to items-bridge SCH/PCB ToolCfgs (F3). +- When any bug is fixed: remove its expected-fail marker; the repro becomes the + regression test.