AI_Assistant/pcbjam
Watch
1
0
Fork
You've already forked pcbjam
 0
Code Issues Pull requests Projects Releases 4 Packages Wiki Activity Actions 2
pcbjam/docs/features/ysync-review/00-overview.md

98 lines
7.1 KiB
Markdown
Raw Permalink Normal View History

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
2026-07-03 12:43:55 +02:00
# 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
fix(ysync): review bugs 01–07 — listener registration, pad nets, child removals, dirty-set emit, targeted rebaseline, seed arbitration, stale hook (doc 17) All seven ysync-review bugs fixed and verified; every repro's expected-fail marker removed (they now run as regression tests). Also fixes two bugs found while verifying (doc 17 F5/F6): file-seeded Y bodies are re-upserted in the editor's serialization (doc-16 F4 was an artifact of bug 01), and the 0008-era "asyncify-fragile envelope parse" was really wrapInBoardEnvelope emitting display layer names — canonical LSET::Name() fixes track/via/zone v2 applies; makeFromBlob now logs parse errors instead of swallowing them. Verified: shared 98, standalone collab 37, ysync e2e 20/20 (chromium), collab regression set 21 passed / 3 pre-existing skips (firefox). Bumps: kicad (board_commit child-removal listener notification), web/pcbjam-shared (slot prune + arbitrated seed). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JgThWXtdvrYLK47EDFoGdq
2026-07-03 18:08:37 +02:00
**UPDATE 2026-07-03: bugs 01–07 are all FIXED** — see
[17](17-fixes-bugs-01-07.md) for the fix record (done in the batch order below),
two additional findings (F5: doc 16's F4 was an artifact of bug 01, file-seeded
bodies are now editor-normalized; F6: the 0008 "asyncify-fragile envelope parse"
was a layer-name bug, now fixed), and the remaining follow-ups. Every repro
below now runs as a plain regression test. Misses 08–10 and opts 12–14 (beyond
what the dirty-set emit already delivers) remain open.
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
2026-07-03 12:43:55 +02:00
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 |
fix(ysync): review bugs 01–07 — listener registration, pad nets, child removals, dirty-set emit, targeted rebaseline, seed arbitration, stale hook (doc 17) All seven ysync-review bugs fixed and verified; every repro's expected-fail marker removed (they now run as regression tests). Also fixes two bugs found while verifying (doc 17 F5/F6): file-seeded Y bodies are re-upserted in the editor's serialization (doc-16 F4 was an artifact of bug 01), and the 0008-era "asyncify-fragile envelope parse" was really wrapInBoardEnvelope emitting display layer names — canonical LSET::Name() fixes track/via/zone v2 applies; makeFromBlob now logs parse errors instead of swallowing them. Verified: shared 98, standalone collab 37, ysync e2e 20/20 (chromium), collab regression set 21 passed / 3 pre-existing skips (firefox). Bumps: kicad (board_commit child-removal listener notification), web/pcbjam-shared (slot prune + arbitrated seed). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JgThWXtdvrYLK47EDFoGdq
2026-07-03 18:08:37 +02:00
| 17 | [17-fixes-bugs-01-07.md](17-fixes-bugs-01-07.md) | Bugs 01–07 fixed & verified (2026-07-03); findings F5–F6; remaining follow-ups |
feat(ysync): lib_symbols travel + layout save-sync (miss 08), TS hot-path opts (12), diff-on-rebind adopt (13) — doc 18 Miss 08A: the binding stores wire-carried (lib_symbols …) definitions in kdoc_libsymbols and prefixes them on apply wires — a joiner that never saw a symbol adopts it WITH its definition (new e2e ysync-libsymbols.spec.ts). Miss 08B: registerSaveHook gains onSavedText; WasmTool routes saved-file text to syncLayoutToY (per sheet room via the manager's syncLayoutFromSave, or the single-room doc) so title block / paper / setup edits converge instead of drifting. Opt 12 (TS half): zod off the observer hot path (yToItemUnchecked), children index built once per conversion. Opt 13: seed()'s adopt diffs the editor snapshot against the doc view and applies only the doc-authoritative difference — clean rebinds apply nothing, the adopt undo entry shrinks to the real changed set. Opt 14 deliberately deferred (doc 18). All TS-side; no wasm rebuild (the C++ blob/findLib sides already carried definitions). Verified: shared 107, standalone 79 (+2 known pre-existing wasm-assets), ysync e2e 21/21 chromium, collab regression 21/3-skip firefox. Bumps: web/pcbjam-shared (lib_symbols channel + syncLayoutToY + opts). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JgThWXtdvrYLK47EDFoGdq
2026-07-06 08:35:18 +02:00
| 18 | [18-miss08-opts-12-13.md](18-miss08-opts-12-13.md) | Miss 08 (lib_symbols + layout save-sync) + opts 12/13 implemented; opt 14 deferred (2026-07-03) |
docs(ysync-review): 19 — miss-09 option 1 (local-ops-only undo) feasibility research Verdict: ~3-5 days. pcbnew undo already UUID-guards stale pointers; core work = porting that guard to eeschema, splitting connectivity out of the SCH SKIP_UNDO gate, plugging the SKIP_UNDO removed-item leak, silencing the incomplete-undo modal. Cross-linked from 09 and the overview index. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019ejJEvS7ogef2o9gVTXjmp
2026-07-07 20:44:33 +02:00
| 19 | [19-undo-option1-feasibility.md](19-undo-option1-feasibility.md) | Miss-09 option 1 (local-ops-only undo) feasibility research (2026-07-07): ~3–5 days, eeschema UUID-guard port is the core |
feat(collab): local-ops-only undo — remote applies skip the undo stack (ysync miss 09) Ctrl+Z after a peer's edit no longer reverts (and re-broadcasts) the peer's work, and the adopt undo-bomb is gone: - doApply/doApplyItems (both editors) Push with SKIP_UNDO; the emit path is unaffected (suppression keys off s_applyingRemote, not undo). - With SKIP_UNDO no picker owns removed items — the bindings free them after Push (explicit removals + upsert's remove-before-re-add; fields excluded: CHT_REMOVE hides them, parent keeps ownership). Freeing stays out of the fork commit classes so DRC's SKIP_UNDO callers can't double-free. - Test hooks kicadCollabTestUndo/UndoDepth, registered per-editor AND in the kicad_editor dispatcher (merged image compiles out per-app registrations). - kicad pointer: eeschema UUID undo guard + SKIP_UNDO connectivity split + quiet stale-entry drop (ca8877324c). - tests/kicad/collab-undo.spec.ts: 5 scenarios (no undo entry from remote applies; selective undo; stranded replaced/deleted entries) — 5/5, plus collab/ysync regression 30 pass. - docs: ysync-review 20 fix record; 09 marked FIXED; overview indexed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019ejJEvS7ogef2o9gVTXjmp
2026-07-08 11:09:26 +02:00
| 20 | [20-fix-miss09-collab-aware-undo.md](20-fix-miss09-collab-aware-undo.md) | Miss 09 FIXED (2026-07-07): SKIP_UNDO remote applies + eeschema UUID guard + binding-owned removed-item lifetime; 5/5 e2e |
Reference in a new issue Copy permalink