pcbjam/docs/features/ysync-review/17-fixes-bugs-01-07.md

Fixes for bugs 01–07 (2026-07-03)

Status: all seven review bugs FIXED and verified — every repro from plan 15 had its expected-fail marker removed and now runs as a plain regression test. Final state: ysync unit suites green (pcbjam-shared 98, standalone collab 37), ysync e2e 20/20 on chromium (two-tab incl. both bug-01 tools, concurrent seed, bare child removal), collab regression set (items-bridge, roundtrip, legacy collab specs, save-hook) 21 passed / 3 pre-existing skips on firefox.

The fixes were done in the review's suggested batches; fixing them also surfaced and fixed two bugs the review missed (F5, F6 below).

Batch 1 — bug 01 (one line, as advertised)

kicad-binding.ts seed()'s file-seed branch now calls bridge.snapshotItems() — ensureBridge() registers the C++ change listener, and the differ gets a baseline. See F5 for why the snapshot's result is now also used.

Batch 2 — bugs 02 + 03

• 02: dropped blobForItem's footprint pad->SetNetCode(0) loop (pcbnew_embind.cpp). KiCad 10 formats pad nets by NAME and the parser resolves names against the receiving board (creating the net if absent), so the doc's "remap by name on apply" concern is satisfied by the format itself. The repro's (net 1 "SIG") assertion was updated to accept the KiCad 10 name-only form.
• 03 (root cause, F1): BOARD_COMMIT::Push's CHT_REMOVE branch for footprint children never touched any listener vector — OnItemsCompositeUpdate was simply never called for a child-only delete. The kicad fork now pushes the parent footprint into itemsChanged there (a genuine upstream notification gap; candidate for upstreaming).
• 03 (emit): itemToJson carries a parent uuid; flushDiff's removal loop lifts a removed child whose parent survives to a parent re-blob on the v2 wire (wRemoved now diverges from the legacy removed).
• 03 (receive): doApplyItems' removed loop only skips a child uuid when its parent footprint is itself in the same wire's removed set; a BARE child removal now removes the child.
• 03 (Y defense): applyDeltaToY deep-prunes {item: uuid} slots from a surviving parent's body before deleting a child item — "file recoverable from the Y.Doc alone" holds regardless of emitter behavior.

Batch 3 — bugs 04 + 05 (one change, both tools)

• The COLLAB_LISTENERs now CAPTURE the touched items (uuid strings at callback time; children lifted to their root — GetParentFootprint() on pcbnew, the symbol/sheet/label/table parent chain on eeschema) into a g_dirty set.
• flushDiff emits every dirty root's v2 blob unconditionally (deduped against the scalar-diff emits). Rotations, field text edits, pad/zone property edits, endpoint drags — anything serializer-visible — now reach the wire. The full-model scalar scan REMAINS: it feeds the legacy wire and catches eeschema's post-Push connectivity cleanup, which never reaches listeners. A no-op commit costs one blob serialization; the TS layer drops unchanged bodies, so nothing echoes.
• 05: doApplyItems replaced the global rebaseline() with rebaselineTouched(applied uuids) (pcbnew also refreshes/erases the child entries via the baseline's parent field) and ends with scheduleFlush(). A local edit committed while the apply was queued keeps its pre-edit baseline and still emits; receiver-side cleanup broadcasts (idempotent on the sender).
• Global rebaseline() clears g_dirty (a sheet switch must not leak the old sheet's dirty marks into the new room).
• Legacy doApply keeps the global rebaseline (dead in production, apply-leg tests only).

Batch 4 — bugs 06 + 07

• 06: new shared primitive seedDocToY(doc, ydoc, origin, nonce) writes the seed nonce into kdoc_meta inside the seed transaction and returns a retractor that deletes exactly the layout slots that seed inserted (matched by insertion id — this client, this clock window — so slots appended by later edits survive; unit-covered). The binding's file-seed branch arbitrates via a kdoc_meta observer: if a foreign nonce wins the LWW merge, the loser retracts, leaving the winner's single clean layout. kdoc_items converges per key on its own. (Server-side seeding — fix direction 1 — remains the cleaner long-term design; this closes the race client-side without backend changes.)
• 07a: bindKicadCollab gates the DOWN hook with a destroyed flag — destroy() makes the binding inert both ways. Gap emits during a sheet switch are dropped; C++ has already rebaselined to the new sheet, so applying them to the old room was pure contamination.
• 07b: failed switchTo now retries with backoff (2s doubling to 30s, event-driven, reset on success) instead of leaving the editor unbound forever.
• Fix direction 2 (generation-tagging the apply envelope for the UP-side sub-frame window) was NOT done — still open, low priority.

New findings while verifying (beyond docs 01–16)

F5 — F4 was an artifact of bug 01; file-seeded bodies must be editor-normalized

Doc 16's F4 ("drift-detect is item-silent on the green path") only held because bug 01 left the differ with NO baseline: the seeder's first edit re-emitted the FULL model, silently overwriting the file-seeded Y bodies with the editor's serialization. With bug 01 fixed, the pl_editor baseline drifted on every file-seeded item ((name border) vs (name "border"), normalized geometry…). Fix: the file-seed branch now uses snapshotItems()' RESULT — item bodies are re-upserted in the editor's serialization (the steady-state form every future emit produces; also what makes upsertYItem's no-op skip effective). Meta + layout stay file-derived. Consequence: a room's materialization is editor-normalized, not file-verbatim — the bug-06 e2e was updated accordingly (convergence + no-duplication, not byte-equality with the raw file render).

F6 — the "asyncify-fragile envelope parse" was actually a layer-name bug

The 0008-era decision to keep track/via/zone/text APPLY off the v2 wire cited an "asyncify-fragile (kicad_pcb …) envelope parse". Root cause found while fixing the pcbnew bug-01 two-tab repro (whose apply leg it blocked): wrapInBoardEnvelope wrote the envelope's layer table with GetLayerName() (user-visible names), but the parser validates canonical names against the fixed layer hash — any board with a layer whose display name differs from its canonical one (every KiCad 10 board: "F.Courtyard" vs "F.CrtYd") threw Layer 'B.Courtyard' … is not in fixed layer hash. Fixed with LSET::Name(). makeFromBlob also no longer swallows parse errors — it logs [collab] pcbnew blob parse error: <IO_ERROR.What()>, which is how this was found. Bare track/via/zone/text changes now apply fine through the envelope (verified: via moves converge across two tabs); the items-bridge PCB config's "changed footprint only" scope note is stale and those types can gain coverage.

Follow-ups (unchanged from 16, minus what this closed)

• Misses 08–10 (non-item state sync, collab-aware undo, drift repair) and optimizations 12–14 (beyond what the dirty set already delivers on the emit hot path) remain open.
• Retire (or un-skip) the legacy two-tab specs; add localEdit + track/via changed coverage to items-bridge (F3 + F6).
• Bug 07 fix direction 2 (envelope generation tag) if presence/multi-sheet work widens the UP-side window.
• Upstream the board_commit.cpp child-removal notification.