docs(ysync-review): repro suite results + empirical findings (doc 16); plan 15 executed
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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DPfrVhfYgPPgtawjSssZfn
This commit is contained in:
Viktor Vaczi
Gergő Törcsvári
parent
f9696ac503
commit
80135a99ac
17 changed files with 1431 additions and 0 deletions
86
docs/features/ysync-review/00-overview.md
Normal file
86
docs/features/ysync-review/00-overview.md
Normal file
|
|
@ -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 |
|
||||||
|
|
@ -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).
|
||||||
|
|
@ -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*.
|
||||||
|
|
@ -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 <uuid>` 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.
|
||||||
94
docs/features/ysync-review/04-bug-lossy-change-detection.md
Normal file
94
docs/features/ysync-review/04-bug-lossy-change-detection.md
Normal file
|
|
@ -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<SCH_ITEM*>&)`, `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.
|
||||||
|
|
@ -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).
|
||||||
|
|
@ -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).
|
||||||
|
|
@ -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).
|
||||||
|
|
@ -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.
|
||||||
56
docs/features/ysync-review/09-miss-undo-not-collab-aware.md
Normal file
56
docs/features/ysync-review/09-miss-undo-not-collab-aware.md
Normal file
|
|
@ -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`.
|
||||||
60
docs/features/ysync-review/10-miss-no-repair-path.md
Normal file
60
docs/features/ysync-review/10-miss-no-repair-path.md
Normal file
|
|
@ -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.
|
||||||
86
docs/features/ysync-review/11-miss-no-v2-e2e-coverage.md
Normal file
86
docs/features/ysync-review/11-miss-no-v2-e2e-coverage.md
Normal file
|
|
@ -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.
|
||||||
|
|
@ -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.
|
||||||
|
|
@ -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.
|
||||||
|
|
@ -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.
|
||||||
168
docs/features/ysync-review/15-plan-repro-tests-and-v2-e2e.md
Normal file
168
docs/features/ysync-review/15-plan-repro-tests-and-v2-e2e.md
Normal file
|
|
@ -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":["<childUuid>"]}')` (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(<unrelated segment change>)` → 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 <standalone> 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/<test-name>` 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).
|
||||||
|
|
@ -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.
|
||||||
Loading…
Reference in a new issue