From 61dddb7e9345ba48b143195c85e2134fe947a39b Mon Sep 17 00:00:00 2001 From: efiten Date: Fri, 4 Sep 2026 15:03:08 +0200 Subject: [PATCH 1/3] fix(#1898, #1900): carry observer_id into replayed packets (#1957) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes #1898. Closes #1900. Two issues, one omission. With a region filter active, VCR replay on the Live map rendered **nothing at all** (#1898), and the Replay button on packet detail **silently did nothing** (#1900). ## Cause `packetMatchesRegion` (`public/live.js:80-92`) matches a packet group by looking up `packets[i].observer_id` in the observer roster map. A packet whose `observer_id` is null is skipped, and when none match it returns `false` and the caller drops the whole group (`live.js:3374`). `dbPacketToLive()` returned `observer` (the resolved name) but never `observer_id`. So every replayed packet was skipped, and every group was dropped. The Replay button had the same gap: both branches passed `obsName(o.observer_id)` and threw the id itself away. **The value was there the whole time.** The VCR builds its entries with `Object.assign({}, p, obs, ...)`, so the observation's `observer_id` is on the input, and the server has returned `observer_id`, `observer_name` and `observer_iata` per packet since `cmd/server/db.go:345-347`. Only the object literal dropped it. ## Fix Carry `observer_id`, and `observer_iata` alongside it so `obsIataBadgeHtml` (`live.js:102-108`) can use the direct field for replayed packets instead of falling back to the roster map. Three lines of behaviour, in two files. ## Verification Four regression tests in `test-live-region-filter.js`. **Three fail without the fix**, checked by reverting `live.js` and re-running: ``` ❌ #1898: dbPacketToLive carries observer_id through ❌ #1898: a replayed packet survives an active region filter ✅ #1898: dropping observer_id is what broke it (guards the regression) ❌ #1898: observer_iata is carried so the badge needs no roster lookup ``` The one that passes either way does so on purpose: it asserts a packet carrying **no** `observer_id` is still dropped, pinning the mechanism so a future change cannot make the filter match everything. That test's sandbox needed `getParsedDecoded` and `getParsedPath`. `live.js:14` captures those from `packet-helpers.js` at load time and the sandbox does not load it, so they are stubbed in the sandbox definition rather than assigned afterwards. Assigning later is too late for that capture, which cost me two attempts. Other suites unaffected: `test-live.js` 95 passed, `test-packet-filter.js` 99, `test-frontend-helpers.js` 656. `test-1110-live-filter.js` fails identically on unmodified master with `ERR_CONNECTION_REFUSED`; it is an E2E test needing a server on port 13581. ## Note Both issues were filed separately and neither names the other. They are the same root cause in sibling code paths, which is why they are fixed together rather than in two PRs. Co-authored-by: Claude Opus 5 (1M context) (cherry picked from commit 9488e9c31cc9b5d6c4e07ca6fdfb8266f9a407b8) Co-Authored-By: Claude Opus 5 --- public/live.js | 8 ++++++- public/packets.js | 9 ++++++-- test-live-region-filter.js | 43 +++++++++++++++++++++++++++++++++++++- 3 files changed, 56 insertions(+), 4 deletions(-) diff --git a/public/live.js b/public/live.js index 266cf7342..536ecf80b 100644 --- a/public/live.js +++ b/public/live.js @@ -856,7 +856,13 @@ resolved_path: pkt.resolved_path, _ts: new Date(pkt.timestamp || pkt.created_at).getTime(), decoded: { header: { payloadTypeName: typeName }, payload: raw, path: { hops } }, - snr: pkt.snr, rssi: pkt.rssi, observer: pkt.observer_name + snr: pkt.snr, rssi: pkt.rssi, observer: pkt.observer_name, + // #1898: the region filter matches on observer_id (packetMatchesRegion, + // line ~85). Without it every replayed packet has observer_id undefined, + // so the filter skips them all and drops the whole group. observer_iata + // is carried too so obsIataBadgeHtml does not have to fall back to the + // roster map for replayed packets. + observer_id: pkt.observer_id, observer_iata: pkt.observer_iata }; } diff --git a/public/packets.js b/public/packets.js index fd157240a..ac8911eee 100644 --- a/public/packets.js +++ b/public/packets.js @@ -3503,7 +3503,11 @@ id: o.id, hash: pkt.hash, raw: o.raw_hex || pkt.raw_hex, _ts: new Date(o.timestamp).getTime(), decoded: { header: { payloadTypeName: typeName }, payload: oDec, path: { hops: oPath } }, - snr: o.snr, rssi: o.rssi, observer: obsName(o.observer_id) + snr: o.snr, rssi: o.rssi, observer: obsName(o.observer_id), + // #1900: carry the id itself, not just the resolved name. The Live + // region filter matches on observer_id, so without it the replay + // silently renders nothing whenever a region is selected. + observer_id: o.observer_id, observer_iata: o.observer_iata }); } } else { @@ -3511,7 +3515,8 @@ id: pkt.id, hash: pkt.hash, raw: pkt.raw_hex, _ts: new Date(pkt.timestamp).getTime(), decoded: { header: { payloadTypeName: typeName }, payload: decoded, path: { hops: pathHops } }, - snr: pkt.snr, rssi: pkt.rssi, observer: obsName(pkt.observer_id) + snr: pkt.snr, rssi: pkt.rssi, observer: obsName(pkt.observer_id), + observer_id: pkt.observer_id, observer_iata: pkt.observer_iata }); } sessionStorage.setItem('replay-packet', JSON.stringify(replayPackets)); diff --git a/test-live-region-filter.js b/test-live-region-filter.js index cf914727e..5a5126368 100644 --- a/test-live-region-filter.js +++ b/test-live-region-filter.js @@ -14,7 +14,13 @@ function test(name, fn) { function makeSandbox() { const ctx = { - window: { addEventListener: () => {}, dispatchEvent: () => {}, devicePixelRatio: 1 }, + window: { + addEventListener: () => {}, dispatchEvent: () => {}, devicePixelRatio: 1, + // live.js:14 captures these at load time from packet-helpers.js, which + // this sandbox does not load. They must exist before live.js is evaluated. + getParsedDecoded: (p) => { try { return p && p.decoded_json ? JSON.parse(p.decoded_json) : {}; } catch (e) { return {}; } }, + getParsedPath: (p) => { try { return p && p.path_json ? JSON.parse(p.path_json) : []; } catch (e) { return []; } }, + }, document: { readyState: 'complete', createElement: () => ({ style:{}, classList:{add(){},remove(){},contains(){return false;}}, setAttribute(){}, addEventListener(){}, getContext: () => ({clearRect(){},fillRect(){},beginPath(){},arc(){},fill(){},scale(){},fillText(){}}) }), @@ -135,7 +141,42 @@ test('observer iata map can be updated and used by filter', () => { assert.strictEqual(fn([{observer_id:'newobs'}], { 'newobs': 'LAX' }, ['LAX']), true); }); +// --- #1898 / #1900: replayed packets must carry observer_id --- +// dbPacketToLive() used to return only observer (the resolved name), so every +// VCR-replayed packet had observer_id undefined. packetMatchesRegion skips a +// packet whose observer_id is null and returns false when none match, so with a +// region selected the replay rendered nothing at all, and the Replay button on +// packet detail silently no-opped. Both issues are the same omission. +const toLive = ctx.window._liveDbPacketToLive; +assert.ok(toLive, '_liveDbPacketToLive must be exposed'); + +test('#1898: dbPacketToLive carries observer_id through', () => { + const live = toLive({ id: 1, hash: 'h', timestamp: '2026-01-01T00:00:00Z', + observer_id: 'obs1', observer_name: 'Obs One' }); + assert.strictEqual(live.observer_id, 'obs1', + 'observer_id must survive; the region filter matches on it'); +}); + +test('#1898: a replayed packet survives an active region filter', () => { + const live = toLive({ id: 1, hash: 'h', timestamp: '2026-01-01T00:00:00Z', + observer_id: 'obs1', observer_name: 'Obs One' }); + assert.strictEqual(fn([live], { obs1: 'BRU' }, ['BRU']), true, + 'with observer_id carried the group matches and is rendered'); +}); + +test('#1898: dropping observer_id is what broke it (guards the regression)', () => { + assert.strictEqual(fn([{ observer: 'Obs One' }], { obs1: 'BRU' }, ['BRU']), false, + 'a packet with no observer_id is skipped, so the group is dropped'); +}); + +test('#1898: observer_iata is carried so the badge needs no roster lookup', () => { + const live = toLive({ id: 1, hash: 'h', timestamp: '2026-01-01T00:00:00Z', + observer_id: 'obs1', observer_iata: 'BRU' }); + assert.strictEqual(live.observer_iata, 'BRU'); +}); + console.log(`\n${'═'.repeat(40)}`); console.log(` live region filter tests: ${passed} passed, ${failed} failed`); + console.log(`${'═'.repeat(40)}\n`); if (failed > 0) process.exit(1); From 2ec954ff4d13cc0cc0ac858372967d2b6afcf4fd Mon Sep 17 00:00:00 2001 From: Dennis Jakobsen Date: Sun, 20 Sep 2026 19:50:37 +0200 Subject: [PATCH 2/3] test(packets): cover the replay half of the observer_id fix MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Mutation testing showed half this PR was unverified: removing observer_id from live.js's dbPacketToLive fails 2 of the 13 region-filter tests, but removing it from packets.js's replay mapping passed the entire suite. The only coverage lived in test-live-region-filter.js, which reaches live.js alone — so the #1900 half could have regressed silently. The mapping sat inline in the replay button's anonymous click handler, so nothing could call it. Extracted it as buildReplayPackets(pkt, data, ctx) and exported it through _packetsTestAPI, following the precedent applyObserverFilter set in #1748: test the real production mapping, not a re-implementation of it that cannot fail when the original does. The extraction is behaviour-neutral — all existing suites are unchanged. Two tests now pin both branches: a single-observation packet carries observer_id and observer_iata alongside the resolved name, and a multi-observer packet gives each observation its OWN observer_id rather than the transmission's representative. Removing observer_id from either branch now fails. One test detail worth naming: the assertions join the ids into a string instead of using deepStrictEqual. The array comes from the vm sandbox's realm, so its prototype differs from the test realm's and a strict deep compare fails on identical contents. Verified: live-region-filter 13/13, frontend-helpers 705 passed (up from 703; the 2 remaining failures are the pre-existing favStar ones that also fail on master), XSS gate 34/34, eslint 0 errors. Co-Authored-By: Claude Opus 5 --- public/packets.js | 69 ++++++++++++++++++++++++---------------- test-frontend-helpers.js | 37 +++++++++++++++++++++ 2 files changed, 78 insertions(+), 28 deletions(-) diff --git a/public/packets.js b/public/packets.js index 5b3d3e63d..5ad83cee5 100644 --- a/public/packets.js +++ b/public/packets.js @@ -2833,6 +2833,44 @@ }); } + // buildReplayPackets shapes the objects the VCR replay reads back out of + // sessionStorage. Extracted from the replay button's click handler — the + // same reason applyObserverFilter above was extracted (#1748 review): a + // test that re-implements this mapping cannot fail when the real one + // regresses, and this mapping has a history of dropping fields. + // + // #1900: observer_id must be carried, not just the resolved observer NAME. + // The Live region filter (packetMatchesRegion in live.js) matches on + // observer_id and skips any packet where it is null, returning false when + // none match — so omitting it made the replay render nothing at all + // whenever a region was selected. observer_iata rides along so the IATA + // badge does not have to fall back to the roster map. + function buildReplayPackets(pkt, data, ctx) { + const { typeName, decoded, pathHops, obsName } = ctx; + const obs = (data && data.observations) || []; + const replayPackets = []; + if (obs.length > 1) { + for (const o of obs) { + replayPackets.push({ + id: o.id, hash: pkt.hash, raw: o.raw_hex || pkt.raw_hex, + _ts: new Date(o.timestamp).getTime(), + decoded: { header: { payloadTypeName: typeName }, payload: getParsedDecoded(o), path: { hops: getParsedPath(o) } }, + snr: o.snr, rssi: o.rssi, observer: obsName(o.observer_id), + observer_id: o.observer_id, observer_iata: o.observer_iata + }); + } + } else { + replayPackets.push({ + id: pkt.id, hash: pkt.hash, raw: pkt.raw_hex, + _ts: new Date(pkt.timestamp).getTime(), + decoded: { header: { payloadTypeName: typeName }, payload: decoded, path: { hops: pathHops } }, + snr: pkt.snr, rssi: pkt.rssi, observer: obsName(pkt.observer_id), + observer_id: pkt.observer_id, observer_iata: pkt.observer_iata + }); + } + return replayPackets; + } + async function renderTableRows() { const tbody = document.getElementById('pktBody'); if (!tbody) return; @@ -3543,34 +3581,8 @@ const replayBtn = panel.querySelector('.replay-live-btn'); if (replayBtn) { replayBtn.addEventListener('click', () => { - // Build replay packets for ALL observations of this transmission - const obs = data.observations || []; - const replayPackets = []; - if (obs.length > 1) { - for (const o of obs) { - const oPath = getParsedPath(o); - const oDec = getParsedDecoded(o); - replayPackets.push({ - id: o.id, hash: pkt.hash, raw: o.raw_hex || pkt.raw_hex, - _ts: new Date(o.timestamp).getTime(), - decoded: { header: { payloadTypeName: typeName }, payload: oDec, path: { hops: oPath } }, - snr: o.snr, rssi: o.rssi, observer: obsName(o.observer_id), - // #1900: carry the id itself, not just the resolved name. The Live - // region filter matches on observer_id, so without it the replay - // silently renders nothing whenever a region is selected. - observer_id: o.observer_id, observer_iata: o.observer_iata - }); - } - } else { - replayPackets.push({ - id: pkt.id, hash: pkt.hash, raw: pkt.raw_hex, - _ts: new Date(pkt.timestamp).getTime(), - decoded: { header: { payloadTypeName: typeName }, payload: decoded, path: { hops: pathHops } }, - snr: pkt.snr, rssi: pkt.rssi, observer: obsName(pkt.observer_id), - observer_id: pkt.observer_id, observer_iata: pkt.observer_iata - }); - } - sessionStorage.setItem('replay-packet', JSON.stringify(replayPackets)); + sessionStorage.setItem('replay-packet', JSON.stringify( + buildReplayPackets(pkt, data, { typeName, decoded, pathHops, obsName }))); window.location.hash = '#/live'; }); } @@ -4131,6 +4143,7 @@ _calcVisibleRange, buildPacketsParams, applyObserverFilter, + buildReplayPackets, renderTableRows, _setPackets: function(p) { packets = p; }, _setFilter: function(k, v) { filters[k] = v; }, diff --git a/test-frontend-helpers.js b/test-frontend-helpers.js index a276ca443..9ecdac035 100644 --- a/test-frontend-helpers.js +++ b/test-frontend-helpers.js @@ -3652,6 +3652,43 @@ console.log('\n=== packets.js: savedTimeWindowMin defaults ==='); _children: [{ observer_id: 'A' }, { observer_id: 'B' }], }; + // --- #1900: the VCR replay mapping must carry observer_id --- + // Same rationale as applyObserverFilter above: exercise the real + // production mapping. Dropping observer_id here previously passed every + // test in the suite, because the only coverage lived in + // test-live-region-filter.js and only reached live.js's half of the fix. + const buildReplayPackets = obsFilterCtx.window._packetsTestAPI.buildReplayPackets; + const replayCtx = { + typeName: 'ADVERT', decoded: { t: 1 }, pathHops: ['AA'], + obsName: (id) => 'Name of ' + id, + }; + + test('#1900 replay carries observer_id for a single-observation packet', () => { + const out = buildReplayPackets( + { id: 5, hash: 'h5', raw_hex: 'CAFE', timestamp: '2026-01-01T00:00:00Z', observer_id: 'obs1', observer_iata: 'BRU' }, + { observations: [{ id: 9 }] }, replayCtx); + assert.strictEqual(out.length, 1); + assert.strictEqual(out[0].observer_id, 'obs1', 'the Live region filter matches on observer_id'); + assert.strictEqual(out[0].observer_iata, 'BRU'); + assert.strictEqual(out[0].observer, 'Name of obs1', 'the resolved name is still carried too'); + }); + + test('#1900 replay carries observer_id for every observation of a multi-observer packet', () => { + const out = buildReplayPackets( + { id: 5, hash: 'h5', raw_hex: 'CAFE', timestamp: '2026-01-01T00:00:00Z', observer_id: 'obs1' }, + { observations: [ + { id: 1, observer_id: 'obsA', observer_iata: 'BRU', timestamp: '2026-01-01T00:00:00Z' }, + { id: 2, observer_id: 'obsB', observer_iata: 'LAX', timestamp: '2026-01-01T00:00:01Z' }, + ] }, replayCtx); + assert.strictEqual(out.length, 2, 'one replay packet per observation'); + // Joined rather than deepStrictEqual: the array comes from the vm + // sandbox's realm, so its prototype differs and a strict deep compare + // fails on identical contents. + assert.strictEqual(out.map((p) => p.observer_id).join(','), 'obsA,obsB', + 'each observation must carry its OWN observer_id, not the transmission representative'); + assert.strictEqual(out.map((p) => p.observer_iata).join(','), 'BRU,LAX'); + }); + test('grouped mode: keeps a multi-observer row whose representative is not the filtered observer (#1748 core bug)', () => { const result = applyObserverFilter([groupedRowRepresentativeNotFiltered], { observer: 'B' }, true, false); assert.strictEqual(result.length, 1, 'server already guaranteed observer B saw this transmission'); From fa3be8061850d2db9d0dcf499c108735802bf08c Mon Sep 17 00:00:00 2001 From: Dennis Jakobsen Date: Sun, 20 Sep 2026 20:03:57 +0200 Subject: [PATCH 3/3] reviewfix(live): make the regression guard actually drive the mapping MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Independent review proved the buildReplayPackets extraction is behaviour-neutral — a differential harness ran master's verbatim inline block against the extracted function over 5000 randomized inputs (0-3 observations, nulls, garbage timestamps, malformed path_json/decoded_json) with 0 mismatches, comparing key order and the parse-cache side effects as well as values. Two findings fixed here. The port's own test named "#1898: dropping observer_id is what broke it (guards the regression)" never called dbPacketToLive. It hand-built { observer: 'Obs One' } and asserted on packetMatchesRegion alone, so it duplicated the existing "does not match when packet has no observer_id" test and survived the very mutation its name claims to guard. It now drives the real mapping and is named for what it actually checks: a source row without an observer_id yields a packet that the filter skips. The genuine regression guard is the sibling test, which does kill that mutation. Switched my multi-observer assertions from joined strings to assert.deepEqual. The loose form compares correctly across the vm sandbox's realm — which is why deepStrictEqual fails there on identical contents — and asserts more than a join. Also dropped a stale "line ~85" reference from a comment. Filed as follow-up, not fixed here: the "Multibyte only" gate six lines above the region gate has the identical defect. groupIsMultibyte reads raw_hex and route_type, and neither replay mapping emits either (they emit `raw`, and no route_type at all), so with that option enabled Replay still renders nothing. Pre-existing, opt-in, and untested because the existing E2E hand-builds packets with a literal raw_hex and bypasses both mappings. Verified: region-filter 13/13 and still kills the live.js mutation (11/2); frontend-helpers 705 passed and still kills both packets.js branches. Co-Authored-By: Claude Opus 5 --- public/live.js | 3 +-- test-frontend-helpers.js | 11 ++++++----- test-live-region-filter.js | 11 +++++++++-- 3 files changed, 16 insertions(+), 9 deletions(-) diff --git a/public/live.js b/public/live.js index 536ecf80b..4b9e4aed8 100644 --- a/public/live.js +++ b/public/live.js @@ -857,8 +857,7 @@ _ts: new Date(pkt.timestamp || pkt.created_at).getTime(), decoded: { header: { payloadTypeName: typeName }, payload: raw, path: { hops } }, snr: pkt.snr, rssi: pkt.rssi, observer: pkt.observer_name, - // #1898: the region filter matches on observer_id (packetMatchesRegion, - // line ~85). Without it every replayed packet has observer_id undefined, + // #1898: the region filter matches on observer_id (packetMatchesRegion). Without it every replayed packet has observer_id undefined, // so the filter skips them all and drops the whole group. observer_iata // is carried too so obsIataBadgeHtml does not have to fall back to the // roster map for replayed packets. diff --git a/test-frontend-helpers.js b/test-frontend-helpers.js index 9ecdac035..5658dc517 100644 --- a/test-frontend-helpers.js +++ b/test-frontend-helpers.js @@ -3681,12 +3681,13 @@ console.log('\n=== packets.js: savedTimeWindowMin defaults ==='); { id: 2, observer_id: 'obsB', observer_iata: 'LAX', timestamp: '2026-01-01T00:00:01Z' }, ] }, replayCtx); assert.strictEqual(out.length, 2, 'one replay packet per observation'); - // Joined rather than deepStrictEqual: the array comes from the vm - // sandbox's realm, so its prototype differs and a strict deep compare - // fails on identical contents. - assert.strictEqual(out.map((p) => p.observer_id).join(','), 'obsA,obsB', + // deepEqual, not deepStrictEqual: the array comes from the vm sandbox's + // realm, so its prototype differs from this one and a STRICT deep compare + // fails on identical contents ("same structure but not reference-equal"). + // The loose form compares across realms and is stronger than joining. + assert.deepEqual(out.map((p) => p.observer_id), ['obsA', 'obsB'], 'each observation must carry its OWN observer_id, not the transmission representative'); - assert.strictEqual(out.map((p) => p.observer_iata).join(','), 'BRU,LAX'); + assert.deepEqual(out.map((p) => p.observer_iata), ['BRU', 'LAX']); }); test('grouped mode: keeps a multi-observer row whose representative is not the filtered observer (#1748 core bug)', () => { diff --git a/test-live-region-filter.js b/test-live-region-filter.js index 5a5126368..403e4bbb2 100644 --- a/test-live-region-filter.js +++ b/test-live-region-filter.js @@ -164,8 +164,15 @@ test('#1898: a replayed packet survives an active region filter', () => { 'with observer_id carried the group matches and is rendered'); }); -test('#1898: dropping observer_id is what broke it (guards the regression)', () => { - assert.strictEqual(fn([{ observer: 'Obs One' }], { obs1: 'BRU' }, ['BRU']), false, +test('#1898: a source row without observer_id still yields an unmatched packet', () => { + // Drive the real mapping rather than hand-building the object: asserting on + // packetMatchesRegion alone only re-tests the filter (already covered by + // "does not match when packet has no observer_id" above) and cannot fail if + // dbPacketToLive stops carrying the field — which is the regression this is + // named for. + const live = toLive({ id: 1, hash: 'h', timestamp: '2026-01-01T00:00:00Z', observer_name: 'Obs One' }); + assert.strictEqual(live.observer_id, undefined, 'nothing to carry when the source row has no id'); + assert.strictEqual(fn([live], { obs1: 'BRU' }, ['BRU']), false, 'a packet with no observer_id is skipped, so the group is dropped'); });