diff --git a/public/packets.js b/public/packets.js index fd157240a..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 --- @@ -2775,6 +2786,53 @@ }); } + // 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 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 — + // 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) 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; + 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 +2875,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 +4125,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 2668c05c6..a276ca443 100644 --- a/test-frontend-helpers.js +++ b/test-frontend-helpers.js @@ -3608,6 +3608,127 @@ 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'); + }); + + // 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 ===== { console.log('\nPackets page — virtual scroll:');