pcbjam/docs/features/ysync-review/04-bug-lossy-change-detection.md
Gergő Törcsvári ae5d41a3d7
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-06 08:57:57 +02:00

4.9 KiB

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: FIXED 2026-07-03 — see 17 (batch 3: listener dirty-set → unconditional v2 blob emit)

Where

  • wasm/bindings/eeschema_embind.cpp:121-180itemToJson (the diff key), :286-297 snapshotItems iterating screen->Items() only
  • wasm/bindings/pcbnew_embind.cpp:144-169forEachTopItem (what is iterated), :176-252itemToJson
  • 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).

Known-missed edits

eeschema:

  • Symbol rotate / mirrorGetPosition() 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 dragsDrawings 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;
  • 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: 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): 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.