fix(collab): one bad wire entry no longer discards the whole local-edit batch

The onItems handler let unwrapWireItem's throw unwind through embind into the
C++ emitter — a bare pageerror, the whole batch lost, and flushDiff already
rebaselined so the dropped items could never be re-sent. Field-seen case:
Update PCB from Schematic emitted 67 changed entries, one an item-less board
envelope (pcbnew writes nothing for a standalone footprint field); all 67 were
dropped and two new footprints existed only on the syncing tab.

The conversion now skips un-resolvable entries per-entry (shared items-wire
fix), every conversion site warns via warnSkip, and the handler body is
wrapped so nothing escapes into the C++ caller again. The tests/web spec
drives the real serializer's empty envelope through onItems and holds that a
good entry batched with it still reaches the peer (proven red before the fix).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019daWLdW5xrRhjUvCUWoSAe

tests/web/items-wire-batch-loss.spec.ts

@@ -0,0 +1,214 @@
+import { test, expect, type Page } from '@playwright/test';
+
+/**
+ * One un-unwrappable wire entry discards the WHOLE local-edit batch.
+ *
+ * Two tabs of the real app on the same board. An editor emits its local edits as
+ * an items-wire delta — `{added, changed, removed}`, each entry one item's full
+ * s-expr — which the binding converts with `itemsWireToDelta` and writes to the
+ * shared Y.Doc; the other tab renders that doc. `itemsWireToDelta` calls
+ * `unwrapWireItem` per entry inside a single loop, and `unwrapWireItem` THROWS on
+ * an entry it cannot resolve to exactly one uuid-bearing item form. The throw
+ * escapes the loop before `doc.transact`, so every OTHER entry in that message is
+ * lost with it — silently: the throw lands in the C++ caller and surfaces only as
+ * a bare pageerror.
+ *
+ * The unresolvable entry is real, and is not fabricated here. The emit side
+ * serializes a non-footprint item as `Format(item)` wrapped in a
+ * `(kicad_pcb … (layers …) <item>)` envelope, and KiCad's board writer emits
+ * NOTHING for a footprint field — `case PCB_FIELD_T: break;`
+ * (pcb_io_kicad_sexpr.cpp:411), correctly, because a field is written by its
+ * footprint's own writer and is never standalone board content. The result is an
+ * envelope with no item in it. This test reads that blob from the real serializer
+ * through `kicadCollabTestItemBlob` — the same function `blobForItem` calls — and
+ * asserts its shape before using it.
+ *
+ * Seen in the field: "Update PCB from Schematic" produced a 67-entry `changed`
+ * batch in which one entry was that empty envelope. All 67 were dropped, so two
+ * footprints the schematic had just added existed on the syncing tab and nowhere
+ * else — not in the room, not for any peer, and not after a reload.
+ *
+ * NOT covered here: the transient condition inside that sync's commit which lets
+ * a field reach the serializer unlifted in the first place (`noteDirty` and
+ * `liftBlob` both substitute a child's parent footprint, and every field on a
+ * settled board is properly parented). Pinning that down needs an instrumented
+ * wasm build. What this test holds is the consequence — where the damage is, and
+ * what any fix has to close: one bad entry must not take the others with it.
+ *
+ * Regression test for the fix: `itemsWireToDelta` now SKIPS an entry it cannot
+ * convert (reported via onSkip → console warn) instead of aborting the batch.
+ * Before the fix this failed exactly as described above.
+ */
+
+const SCOPE = 'default';
+const BOARD = `/${SCOPE}/projects/demo/demo.kicad_pcb`;
+
+/** Wire entries are `{sexpr, parent}`; the emit side sends parent null for roots. */
+type WireItem = { sexpr: string; parent: string | null };
+type Mod = {
+  kicadCollabSnapshotItems(): string;
+  kicadCollabTestItemBlob(uuid: string): string;
+};
+type W = { Module: Mod; kicadCollab: { onItems?: (json: string) => void } };
+
+async function bootBoard(page: Page, user: string): Promise<void> {
+  await page.goto(`${BOARD}?user=${user}`);
+  await expect(page.locator('#canvas')).toBeVisible({ timeout: 120000 });
+  await expect
+    .poll(() => page.title(), {
+      message: `${user}: board editor never reached the expected title`,
+      timeout: 120000,
+      intervals: [1000],
+    })
+    .toMatch(/demo — PCB Editor/i);
+  await page.waitForFunction(
+    () =>
+      typeof (window as unknown as Partial<W>).Module?.kicadCollabTestItemBlob === 'function',
+    null,
+    { timeout: 60000 },
+  );
+  // The binding owns the emit slot; without it there is nothing to feed.
+  await page.waitForFunction(
+    () => typeof (window as unknown as Partial<W>).kicadCollab?.onItems === 'function',
+    null,
+    { timeout: 60000 },
+  );
+}
+
+/** That footprint's own `(at …)` as this tab currently holds it. */
+async function positionOf(page: Page, uuid: string): Promise<string> {
+  return page.evaluate((id) => {
+    const snap = JSON.parse(
+      (window as unknown as W).Module.kicadCollabSnapshotItems(),
+    ) as { added: WireItem[] };
+    const blob = snap.added.find((w) => w.sexpr.includes(id));
+    // The footprint's own `(at …)` is the first in its blob — layer and uuid
+    // precede it, and every later one belongs to a child.
+    return blob?.sexpr.match(/\(at [^)]*\)/)?.[0] ?? '(absent)';
+  }, uuid);
+}
+
+/**
+ * Hand the binding a local-edit delta exactly as the C++ emit does.
+ *
+ * The handler runs synchronously inside `onItems`, so a throw comes straight back
+ * out; catching it in-page keeps the failure attributable to this message instead
+ * of surfacing as an unrelated pageerror.
+ */
+async function emit(page: Page, changed: WireItem[]): Promise<string> {
+  return page.evaluate((items) => {
+    try {
+      (window as unknown as W).kicadCollab.onItems!(
+        JSON.stringify({ added: [], changed: items, removed: [] }),
+      );
+      return 'no throw';
+    } catch (e) {
+      return String(e);
+    }
+  }, changed);
+}
+
+const moveTo = (sexpr: string, pos: string) => sexpr.replace(/\(at [^)]*\)/, `(at ${pos})`);
+
+/**
+ * A target position derived from where the item CURRENTLY sits, offset by a
+ * per-step delta. The room's doc persists across runs (and across the CI
+ * engine projects sharing one server stack), so absolute coordinates would
+ * leave a re-run moving an item onto itself — a vacuous no-change the final
+ * assertions cannot distinguish from a discarded batch.
+ */
+const bumped = (sexpr: string, dx: number, dy: number): string => {
+  const m = sexpr.match(/\(at (-?[\d.]+) (-?[\d.]+)/);
+  if (!m) throw new Error('item has no (at x y)');
+  return `${(parseFloat(m[1]!) + dx).toFixed(2)} ${(parseFloat(m[2]!) + dy).toFixed(2)}`;
+};
+
+test('a single un-unwrappable entry must not discard the rest of the batch', async ({
+  page,
+  context,
+}) => {
+  test.setTimeout(480000); // two full board boots
+
+  const alice = page;
+  await bootBoard(alice, 'alice');
+  const bob = await context.newPage();
+  await bootBoard(bob, 'bob');
+
+  // ── the payloads, all straight out of the editor ──────────────────────────
+  // Two footprints; the second one's field supplies the poisoned entry.
+  const picked = await alice.evaluate(() => {
+    const M = (window as unknown as W).Module;
+    const snap = JSON.parse(M.kicadCollabSnapshotItems()) as { added: WireItem[] };
+    const fps: Array<{ uuid: string; sexpr: string; field?: string }> = [];
+    for (const w of snap.added) {
+      if (!/^\s*\(footprint\b/.test(w.sexpr)) continue;
+      const uuid = w.sexpr.match(/\(uuid\s+"([^"]+)"\)/)?.[1];
+      // A field is a `(property …)` INSIDE the footprint, carrying its own uuid.
+      const field = w.sexpr.match(/\(property\s+"[^"]*"[^]*?\(uuid\s+"([^"]+)"\)/)?.[1];
+      if (uuid) fps.push({ uuid, sexpr: w.sexpr, field });
+    }
+    const fp1 = fps[0];
+    const fp2 = fps.find((f, i) => i > 0 && !!f.field);
+    if (!fp1 || !fp2) return null;
+    return { fp1, fp2, fieldBlob: M.kicadCollabTestItemBlob(fp2.field!) };
+  });
+  expect(picked, 'demo board should have two footprints, the second with a field').toBeTruthy();
+  const { fp1, fp2, fieldBlob } = picked!;
+
+  // The emit-side serializer, asked for a field on its own, yields an envelope
+  // with no item in it. This is the entry `unwrapWireItem` cannot resolve.
+  expect(fieldBlob, 'field blob is not empty text').toBeTruthy();
+  expect(fieldBlob, 'field blob is a board envelope').toContain('(kicad_pcb');
+  expect(fieldBlob, 'field blob carries the layer table').toContain('(layers');
+  expect(fieldBlob, 'field blob contains NO item — this is the defect').not.toContain('(uuid');
+
+  const fp2Before = await positionOf(bob, fp2.uuid);
+  expect(fp2Before, 'bob should already hold fp2').not.toBe('(absent)');
+
+  // ── 1. control: a lone footprint entry propagates ─────────────────────────
+  // Asserted as a CHANGE from what bob held, so it cannot pass vacuously if a
+  // previous run left the board at the target coordinates.
+  const fp1Before = await positionOf(bob, fp1.uuid);
+  expect(
+    await emit(alice, [{ sexpr: moveTo(fp1.sexpr, bumped(fp1.sexpr, 1.1, 2.2)), parent: null }]),
+  ).toBe('no throw');
+  await expect
+    .poll(() => positionOf(bob, fp1.uuid), {
+      message: 'bob never received a lone footprint entry — the harness is wrong, not the code',
+      timeout: 30000,
+      intervals: [500],
+    })
+    .not.toBe(fp1Before);
+  const fp1AfterControl = await positionOf(bob, fp1.uuid);
+
+  // ── 2. the defect: a good entry batched with the field entry ──────────────
+  // Deliberately NOT asserted on: today this returns the `unwrapWireItem: …
+  // found 0` throw, and once the conversion skips an entry it cannot convert it
+  // will return 'no throw'. Requiring either would pin the test to one side of
+  // the fix. The invariant is the one asserted at the end — the GOOD entry in
+  // this batch must reach the peer — so the outcome is recorded, not enforced.
+  const reproEmit = await emit(alice, [
+    { sexpr: moveTo(fp2.sexpr, bumped(fp2.sexpr, 3.3, 4.4)), parent: null },
+    { sexpr: fieldBlob, parent: null },
+  ]);
+  test.info().annotations.push({ type: 'repro emit', description: reproEmit });
+
+  // ── 3. fence: a later message arriving proves the earlier one is not merely
+  // in flight. No sleep, no flake — ordering does the waiting.
+  expect(
+    await emit(alice, [{ sexpr: moveTo(fp1.sexpr, bumped(fp1.sexpr, 5.5, 6.6)), parent: null }]),
+  ).toBe('no throw');
+  await expect
+    .poll(() => positionOf(bob, fp1.uuid), {
+      message: 'the fence message never arrived, so the negative assertion below cannot be trusted',
+      timeout: 30000,
+      intervals: [500],
+    })
+    .not.toBe(fp1AfterControl);
+
+  // The batch was discarded whole: fp2 never moved for the peer.
+  expect(
+    await positionOf(bob, fp2.uuid),
+    'fp2 moved in the same batch as the un-unwrappable entry and was discarded with it',
+  ).not.toBe(fp2Before);
+});

web/pcbjam-shared

@@ -1 +1 @@
-Subproject commit 9e269711deebbb7ed5fbb3679082c2b90224a188
+Subproject commit 29f6898c2ffdc0f6ab94bf7dbab790265c0b4a6c

web/standalone/src/wasm/collab/kicad-binding.ts

@@ -156,6 +156,14 @@ export function bindKicadCollab(
   const libDefs = (libId: string): string | undefined =>
     kicadLibSymbolsMap(doc).get(libId);
 
+  // A wire entry the conversion could not resolve to an item (typically the
+  // sender serializing an unlifted child → item-less board envelope). The
+  // conversion skips it so the rest of the batch survives; log it loudly —
+  // this line is also the breadcrumb for the still-open question of how a
+  // child reaches the sender's serializer unlifted.
+  const warnSkip = (w: { sexpr: string }, err: unknown): void =>
+    cwarn("wire entry skipped (un-resolvable):", err, w.sexpr.slice(0, 200));
+
   // DOWN: local editor change → Y.Doc
   bridge.onItems((json: string) => {
     if (readOnly) return; // viewer: local state never reaches the doc
@@ -167,20 +175,29 @@ export function bindKicadCollab(
       cwarn("⬇ onItems from wasm: UNPARSEABLE", err, json);
       return;
     }
-    const delta = itemsWireToDelta(wire, itemsView());
+    // This handler runs synchronously inside the C++ emit; a throw escaping it
-    // Library definitions the blob carried (a placed symbol's lib_symbols
+    // unwinds through embind as a bare pageerror AND discards the whole batch
-    // context — miss 08): store them alongside the items, same transaction.
+    // after the sender already rebaselined (the batch-loss bug). Entry-level
-    const defs = wireLibSymbols(wire);
+    // failures are already skipped inside the conversion; this catch is the
-    if (isEmptyKicadDelta(delta) && Object.keys(defs).length === 0) return;
+    // backstop for everything else.
-    clog("⬇ onItems (local edit):", {
+    try {
-      added: delta.added.length,
+      const delta = itemsWireToDelta(wire, itemsView(), warnSkip);
-      updated: delta.updated.length,
+      // Library definitions the blob carried (a placed symbol's lib_symbols
-      removed: delta.removed.length,
+      // context — miss 08): store them alongside the items, same transaction.
-    });
+      const defs = wireLibSymbols(wire);
-    doc.transact(() => {
+      if (isEmptyKicadDelta(delta) && Object.keys(defs).length === 0) return;
-      applyDeltaToY(doc, delta, ORIGIN);
+      clog("⬇ onItems (local edit):", {
-      upsertLibSymbolsToY(doc, defs, ORIGIN);
+        added: delta.added.length,
-    }, ORIGIN);
+        updated: delta.updated.length,
+        removed: delta.removed.length,
+      });
+      doc.transact(() => {
+        applyDeltaToY(doc, delta, ORIGIN);
+        upsertLibSymbolsToY(doc, defs, ORIGIN);
+      }, ORIGIN);
+    } catch (err) {
+      cwarn("⬇ onItems from wasm: batch failed to apply", err);
+    }
   });
 
   // UP: remote Y change → editor. The subscription + origin policy live HERE
@@ -287,7 +304,7 @@ export function bindKicadCollab(
       // and defeat upsertYItem's no-op skip. Meta + layout stay file-derived.
       try {
         const wire = parseItemsWireDelta(bridge.snapshotItems());
-        const local = itemsWireToDelta(wire, itemsView());
+        const local = itemsWireToDelta(wire, itemsView(), warnSkip);
         if (!isEmptyKicadDelta(local)) applyDeltaToY(doc, local, ORIGIN);
       } catch (err) {
         cwarn("seed: post-file-seed baseline failed", err);
@@ -311,7 +328,7 @@ export function bindKicadCollab(
         return;
       }
       // First tab, no file source: seed the shared doc from the editor model.
-      const local = itemsWireToDelta(wire, {});
+      const local = itemsWireToDelta(wire, {}, warnSkip);
       clog(`seed: doc empty → SEEDING from editor snapshot (${local.added.length} item(s))`);
       doc.transact(() => {
         applyDeltaToY(doc, local, ORIGIN);
@@ -328,8 +345,8 @@ export function bindKicadCollab(
     // adopt undo-bomb, miss 09) shrinks to the real changed set, and a clean
     // rebind degrades to baseline-only.
     const view = itemsView();
-    const editorDelta = itemsWireToDelta(wire, view); // editor state vs doc view
+    const editorDelta = itemsWireToDelta(wire, view, warnSkip); // editor state vs doc view
-    const editorUuids = wireItemUuids(wire);
+    const editorUuids = wireItemUuids(wire, warnSkip);
 
     // Doc authority, inverted per class:
     //  - doc-only ROOTS → add to the editor (their sexprs embed descendants;