From 72f64e9364e5f700b08380eefb020bef5012a384 Mon Sep 17 00:00:00 2001 From: Saarlandpower Date: Wed, 2 Sep 2026 09:50:22 +0200 Subject: [PATCH 1/2] fix(packets): observer filter hides multi-observer rows in grouped mode (#1748) (#1821) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes #1748. ## Root cause `renderTableRows()` in `public/packets.js` re-filtered the already server-filtered `/api/packets` (grouped) results client-side, comparing `filters.observer` against each row's `observer_id`. But that field is only the **representative** observer chosen for display — `QueryGroupedPackets` (`cmd/server/db.go:554-606`) picks the observation with the longest observed path via a `LEFT JOIN ... ORDER BY length(path_json) DESC LIMIT 1`, purely for display purposes. The server-side filter (`buildTransmissionWhere`, `cmd/server/db.go:725-790`) is already correct: it uses an `EXISTS` subquery over **all** observations of a transmission, so any row it returns was genuinely seen by at least one selected observer. The client then discarded rows *again*, checking only the representative's `observer_id`, with a fallback to `p._children` — which is `undefined` on initial page load (only fetched lazily on row-expand or when the observer-sort dropdown changes, see the `obsSortSel` change handler). Net effect: a multi-observer transmission stayed visible under an observer filter only when the filtered observer happened to also be the representative (longest-path) observer — which matches the reported "works only for whichever observer logged it first" behavior in dense meshes, where longest-path and earliest-seen correlate. ## Fix Skip the client-side observer re-filter entirely when `groupByHash` is active — the server's `EXISTS` filter is authoritative for grouped rows and needs no client-side correction. The flat/expanded-mode path (single-observation rows, each with its own exact `observer_id` from `buildPacketWhere`) keeps the existing children-aware filter unchanged, which already had test coverage under #537 for the case where `_children` is already populated. ## Tests Added 6 cases in `test-frontend-helpers.js` covering the specific gap #537's tests didn't reach — grouped mode with `_children` still `undefined` (the actual initial-load state that triggers this bug). New tests confirm: - A multi-observer row whose representative doesn't match the filter is kept in grouped mode (the core bug). - Grouped mode never re-filters client-side (trusts the server). - Flat mode behavior is unchanged (matches by own `observer_id`, falls back to already-loaded `_children`). Full run: `test-frontend-helpers.js` 631 passed / 2 failed (same 2 pre-existing `favStar` failures reproduce identically on unmodified `master` — unrelated). `test-packet-filter.js` 92/92. `test-aging.js` 18/18. Operator context: running CoreScope for SaarMesh (SaarLorLux, DE/FR/LU, 800+ nodes, 14 observers) — this was hiding a large share of traffic whenever filtering by a non-primary observer in our dense mesh. Co-Authored-By: Claude --------- Co-authored-by: Claude (cherry picked from commit 02c2338d649b11f27b5e2e008438fd25e22b9660) Co-Authored-By: Claude Opus 5 --- public/packets.js | 51 ++++++++++++++++++---- test-frontend-helpers.js | 91 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 134 insertions(+), 8 deletions(-) diff --git a/public/packets.js b/public/packets.js index fd157240a..ff8c80f5a 100644 --- a/public/packets.js +++ b/public/packets.js @@ -2775,6 +2775,47 @@ }); } + // applyObserverFilter decides which already-loaded packets remain visible + // under the current observer filter. Extracted into its own function + // (rather than left inline in renderTableRows) specifically so tests can + // exercise the real production logic instead of a hand-copied + // reimplementation — see #1748 PR review (kent-beck): a test that only + // checks a copy of this logic doesn't fail if this function regresses. + // + // #1748: In grouped mode, the server already filters transmissions + // correctly (buildTransmissionWhere emits an EXISTS subquery over ALL + // observations of the transmission, not just the displayed one — see + // cmd/server/db.go). Each row's `observer_id` here is only the + // *representative* observer chosen for display (longest observed path), + // which may legitimately differ from the observer that satisfied the + // filter. Re-filtering client-side against that single representative — + // with `_children` still unpopulated at this point (only fetched lazily + // on row-expand or observer-sort-change) — hid every multi-observer + // transmission whose representative happened not to be one of the + // selected observers. In practice this meant a transmission only stayed + // visible when the filtered observer was also the one with the longest + // path (which is why the report described it as "works only for + // whichever observer logged it first" in dense meshes, where + // longest-path and earliest-seen correlate). The server-side EXISTS + // filter is authoritative for grouped rows, so no client-side + // re-filtering is needed or correct here. + // + // Flat/expanded mode (groupByHash === false) has no such + // representative-vs-actual mismatch — buildPacketWhere filters each + // observation row by its own exact observer_id — but we keep the + // defensive re-filter for that path since it costs nothing and guards + // against any future flat-mode server change. + function applyObserverFilter(displayPackets, filters, groupByHash, hashOnly) { + if (hashOnly || !filters.observer) return displayPackets; + if (groupByHash) return displayPackets; + const obsIds = new Set(filters.observer.split(',')); + return displayPackets.filter(p => { + if (obsIds.has(p.observer_id)) return true; + if (p._children) return p._children.some(c => obsIds.has(String(c.observer_id))); + return false; + }); + } + async function renderTableRows() { const tbody = document.getElementById('pktBody'); if (!tbody) return; @@ -2817,14 +2858,7 @@ const types = filters.type.split(',').map(Number); displayPackets = displayPackets.filter(p => types.includes(p.payload_type)); } - if (!hashOnly && filters.observer) { - const obsIds = new Set(filters.observer.split(',')); - displayPackets = displayPackets.filter(p => { - if (obsIds.has(p.observer_id)) return true; - if (p._children) return p._children.some(c => obsIds.has(String(c.observer_id))); - return false; - }); - } + displayPackets = applyObserverFilter(displayPackets, filters, groupByHash, hashOnly); // Packet Filter Language const pfCount = document.getElementById('packetFilterCount'); @@ -4074,6 +4108,7 @@ buildFlatRowHtml, _calcVisibleRange, buildPacketsParams, + applyObserverFilter, 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 5dbd47e57..57373501d 100644 --- a/test-frontend-helpers.js +++ b/test-frontend-helpers.js @@ -3608,6 +3608,97 @@ console.log('\n=== packets.js: savedTimeWindowMin defaults ==='); }); } +// ===== Packets page: observer filter in grouped mode (issue #1748) ===== +{ + console.log('\n--- Observer filter — grouped vs. flat mode (#1748) ---'); + + // Load the REAL applyObserverFilter from packets.js via sandbox, rather + // than testing a hand-copied reimplementation of its logic. Per #1748 PR + // review (kent-beck): a test that only checks a copy doesn't fail if the + // actual production function regresses. Mirrors the same loadInCtx + // pattern used below for _calcVisibleRange. + const obsFilterCtx = makeSandbox(); + obsFilterCtx.registerPage = (name, handlers) => {}; + obsFilterCtx.onWS = () => {}; + obsFilterCtx.offWS = () => {}; + obsFilterCtx.api = () => Promise.resolve({}); + obsFilterCtx.window.getParsedPath = () => []; + obsFilterCtx.window.getParsedDecoded = () => ({}); + loadInCtx(obsFilterCtx, 'public/packets.js'); + const applyObserverFilter = obsFilterCtx.window._packetsTestAPI.applyObserverFilter; + + // A transmission the server correctly returned for observer "B" (it was + // one of several observers of this hash) but whose displayed + // "representative" observer is "A" (longest path) — exactly the shape + // QueryGroupedPackets returns. `_children` is undefined, matching the + // state on initial load (only fetched lazily on expand/obs-sort-change). + const groupedRowRepresentativeNotFiltered = { + hash: 'hash1', observer_id: 'A', observer_count: 3, _children: undefined, + }; + // A single-observer transmission whose only observer is NOT in the filter — + // must still be excluded (it genuinely wasn't observed by the filtered + // observer; the server wouldn't have returned it in the first place, but + // the client function must not accidentally let everything through). + const groupedRowGenuinelyExcludedByServer = { + hash: 'hash2', observer_id: 'C', observer_count: 1, _children: undefined, + }; + // A flat/expanded-mode row: single observation, own observer_id. + const flatRowMatching = { hash: 'hash3', observer_id: 'B' }; + const flatRowNonMatching = { hash: 'hash4', observer_id: 'A' }; + // A flat/expanded-mode row with already-loaded children, only one of + // which matches — the pre-existing children-aware fallback path. + const flatRowWithMatchingChild = { + hash: 'hash5', observer_id: 'A', + _children: [{ observer_id: 'A' }, { observer_id: 'B' }], + }; + + 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'); + }); + + test('grouped mode: does not need _children to keep a valid server-filtered row', () => { + // The defining regression: _children is undefined (not yet fetched), + // and observer_id (the representative) does not match — pre-fix this + // returned zero rows. + const result = applyObserverFilter([groupedRowRepresentativeNotFiltered], { observer: 'B' }, true, false); + assert.strictEqual(result.length, 1); + }); + + test('grouped mode: passes through rows unfiltered (server EXISTS filter is authoritative)', () => { + // The client no longer second-guesses the server in grouped mode at all — + // this row would never have been returned by the server if observer C + // weren't a match, so the client trusts it. + const result = applyObserverFilter([groupedRowGenuinelyExcludedByServer], { observer: 'B' }, true, false); + assert.strictEqual(result.length, 1, 'grouped mode does not re-filter; server already applied the correct filter'); + }); + + test('flat mode: keeps a row whose own observer_id matches', () => { + const result = applyObserverFilter([flatRowMatching], { observer: 'B' }, false, false); + assert.strictEqual(result.length, 1); + }); + + test('flat mode: excludes a row whose own observer_id does not match and has no children', () => { + const result = applyObserverFilter([flatRowNonMatching], { observer: 'B' }, false, false); + assert.strictEqual(result.length, 0); + }); + + test('flat mode: falls back to matching children when representative does not match', () => { + const result = applyObserverFilter([flatRowWithMatchingChild], { observer: 'B' }, false, false); + assert.strictEqual(result.length, 1, 'observer B is among the already-loaded children'); + }); + + test('hashOnly bypasses the observer filter entirely (pinned-hash view)', () => { + const result = applyObserverFilter([groupedRowGenuinelyExcludedByServer, flatRowNonMatching], { observer: 'B' }, true, true); + assert.strictEqual(result.length, 2, 'hashOnly must return every row unfiltered, matching renderTableRows()'); + }); + + test('no observer filter set: all rows pass through unchanged', () => { + const result = applyObserverFilter([groupedRowGenuinelyExcludedByServer, flatRowNonMatching], {}, false, false); + assert.strictEqual(result.length, 2); + }); +} + // ===== Packets page: virtual scroll infrastructure ===== { console.log('\nPackets page — virtual scroll:'); From 6d936daf074c7de029812e9b9df4738bdcbe254a Mon Sep 17 00:00:00 2001 From: Dennis Jakobsen Date: Sun, 20 Sep 2026 11:44:14 +0200 Subject: [PATCH 2/2] reviewfix(packets): refetch when the observer filter changes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Independent review found that skipping the client-side re-filter in grouped mode made the observer filter a complete no-op in the default view, which is worse than the bug being fixed: the dropdown handler was the only server-side filter handler that did not refetch, so it relied entirely on that client-side pass to narrow an already-loaded page. Reproduced by driving the real renderTableRows(): rows whose sole observer was someone else stayed visible under an observer filter until the next page load. The observer filter is a server-side filter (buildPacketsParams sends 'observer'), so changing it now calls loadPackets(), like hash, time window, group toggle, myNodes, region and channel already do. That makes the grouped early return correct rather than merely harmless, and also fixes the pre-existing converse: unticking back to All Observers now restores the wider set instead of leaving the page narrowed. Also corrects two comments that cited code paths which do not run (buildPacketWhere has no non-test callers; the usual grouped path is store.QueryGroupedPackets, with buildTransmissionWhere only as the older-than-oldestLoaded fallback — both scan all observations, so the conclusion stands), and closes three test gaps the reviewer found by mutation testing: the hashOnly case was asserted in grouped mode where the early return made it vacuous, the children fixture used only string ids so the String() coercion could be dropped unnoticed, and nothing pinned the refetch contract. All three mutants now fail. Co-Authored-By: Claude Opus 5 --- public/packets.js | 37 +++++++++++++++++++++++++++---------- test-frontend-helpers.js | 34 ++++++++++++++++++++++++++++++++-- 2 files changed, 59 insertions(+), 12 deletions(-) diff --git a/public/packets.js b/public/packets.js index ff8c80f5a..426425459 100644 --- a/public/packets.js +++ b/public/packets.js @@ -1785,7 +1785,18 @@ buildObserverMenu(); updateObsTrigger(); updatePacketsUrl(); - renderTableRows(); + // The observer filter is a SERVER-side filter (buildPacketsParams sends + // `observer`), so changing it must refetch — the same thing every other + // server-side filter handler here does (hash, time window, group toggle, + // myNodes, region, channel, clear-filters). It previously only + // re-rendered, relying on the client-side re-filter in + // applyObserverFilter to narrow the already-loaded page. That is exactly + // the re-filter grouped mode must not do (a grouped row's observer_id is + // only the representative observer), so without a refetch the filter + // would silently do nothing in grouped mode until the next page load. + // Refetching also fixes the converse: unticking back to "All Observers" + // now restores the wider set instead of leaving the page narrowed. + loadPackets(); }); // --- Type multi-select --- @@ -2782,10 +2793,16 @@ // reimplementation — see #1748 PR review (kent-beck): a test that only // checks a copy of this logic doesn't fail if this function regresses. // - // #1748: In grouped mode, the server already filters transmissions - // correctly (buildTransmissionWhere emits an EXISTS subquery over ALL - // observations of the transmission, not just the displayed one — see - // cmd/server/db.go). Each row's `observer_id` here is only the + // #1748: In grouped mode, the server already filters transmissions over + // ALL observations of a transmission, not just the displayed one. Both + // server paths do this: the usual in-memory one (store.QueryGroupedPackets + // → filterPackets / transmissionsForObserver, cmd/server/store.go, which + // scans every tx.Observations) and the SQL fallback used for windows older + // than oldestLoaded (buildTransmissionWhere's EXISTS subquery, + // cmd/server/db.go). Because it is a server-side filter, changing it + // refetches — see the observer multi-select handler above; without that + // refetch this early return would make the filter a no-op in grouped mode. + // Each row's `observer_id` here is only the // *representative* observer chosen for display (longest observed path), // which may legitimately differ from the observer that satisfied the // filter. Re-filtering client-side against that single representative — @@ -2800,11 +2817,11 @@ // filter is authoritative for grouped rows, so no client-side // re-filtering is needed or correct here. // - // Flat/expanded mode (groupByHash === false) has no such - // representative-vs-actual mismatch — buildPacketWhere filters each - // observation row by its own exact observer_id — but we keep the - // defensive re-filter for that path since it costs nothing and guards - // against any future flat-mode server change. + // Flat/expanded mode (groupByHash === false) lists individual observations, + // each carrying its own observer_id, so re-filtering them against the + // selected set is exact rather than representative-based. We keep that + // defensive re-filter: it costs nothing and guards against any future + // flat-mode server change. function applyObserverFilter(displayPackets, filters, groupByHash, hashOnly) { if (hashOnly || !filters.observer) return displayPackets; if (groupByHash) return displayPackets; diff --git a/test-frontend-helpers.js b/test-frontend-helpers.js index 7803e4d5a..a276ca443 100644 --- a/test-frontend-helpers.js +++ b/test-frontend-helpers.js @@ -3688,15 +3688,45 @@ console.log('\n=== packets.js: savedTimeWindowMin defaults ==='); assert.strictEqual(result.length, 1, 'observer B is among the already-loaded children'); }); - test('hashOnly bypasses the observer filter entirely (pinned-hash view)', () => { - const result = applyObserverFilter([groupedRowGenuinelyExcludedByServer, flatRowNonMatching], { observer: 'B' }, true, true); + // groupByHash MUST be false here: with it true the grouped early-return + // would return everything anyway, so the assertion would hold even with the + // hashOnly guard deleted and would pin nothing. Pinning a hash and then + // toggling "Group by Hash" off is a reachable state, and is exactly the + // case buildPacketsParams's hash short-circuit (which suppresses the + // `observer` param server-side) relies on. + test('hashOnly bypasses the observer filter entirely in FLAT mode (pinned-hash view)', () => { + const result = applyObserverFilter([flatRowNonMatching, { hash: 'hash6', observer_id: 'C' }], { observer: 'B' }, false, true); assert.strictEqual(result.length, 2, 'hashOnly must return every row unfiltered, matching renderTableRows()'); }); + // The children fallback coerces with String() because an observation's + // observer_id arrives as a number on some payloads while filters.observer is + // always a comma-joined string. A fixture with string ids only would make + // the coercion a no-op and let its removal go unnoticed. + test('flat mode: numeric child observer_id still matches the string filter', () => { + const numericChildRow = { hash: 'hash7', observer_id: 'A', _children: [{ observer_id: 7 }] }; + const result = applyObserverFilter([numericChildRow], { observer: '7' }, false, false); + assert.strictEqual(result.length, 1, 'String(c.observer_id) must bridge a numeric child id to the string filter'); + }); + test('no observer filter set: all rows pass through unchanged', () => { const result = applyObserverFilter([groupedRowGenuinelyExcludedByServer, flatRowNonMatching], {}, false, false); assert.strictEqual(result.length, 2); }); + + // The grouped early-return is only safe because changing the observer + // filter refetches from the server. If that handler ever stops calling + // loadPackets(), the filter silently becomes a no-op in grouped mode (the + // default view) until the next page load — the regression this pins. + test('the observer multi-select handler refetches instead of only re-rendering', () => { + const src = fs.readFileSync('public/packets.js', 'utf8'); + const handler = src.slice(src.indexOf("obsMenu.addEventListener('change'")); + const body = handler.slice(0, handler.indexOf('\n });')); + assert.ok(body.includes('loadPackets()'), + 'observer filter changes must refetch from the server (grouped rows are server-filtered)'); + assert.ok(!/\brenderTableRows\(\)/.test(body), + 'a bare renderTableRows() would only re-filter the already-loaded page'); + }); } // ===== Packets page: virtual scroll infrastructure =====