From 4d63141ce6c15fe7ff72ad8cb5bf6124168413f6 Mon Sep 17 00:00:00 2001 From: dborup Date: Mon, 5 Oct 2026 16:03:31 +0200 Subject: [PATCH 1/7] test(packets,nodes): cover the #259 follow-ups to #255 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Tests first, red on master 3bb2cb89 where they describe a behaviour change. 1. test-packets.js: an expanded group across the 600 px breakpoint. At 390 px it must render no child rows, no `expanded` class and no down caret, while the hash stays in expandedHashes so the children return at 1400 px. _getRowCount must agree with the rendered rows on both sides (#424). 4 of the 7 new cases fail on master. 2. test-issue-259-nodes-esc-listener.js: the node page's document keydown handler (`nodesEsc`). After the router's destroy/init cycle for node A -> B -> C at most one listener may survive, Escape may write the hash once, and destroy() must leave none behind. 4 of 7 fail on master, where three listeners stack and Escape navigates three times. 3. test-issue-254-affinity-debug-toggle.js: three source assertions so the fast layer proves the loadFullNode template goes through renderAffinityDebugCard() and holds no inline on*= handler. Green on master by design — these close review finding F1, where the mutant that restores the old inline card passed the unit test 9/9. 4. test-issue-254-affinity-toggle-mobile-aria-e2e.js: press Space, not only Enter, on the 390 px group row (review finding F3). New E2E test-issue-259-mobile-expanded-group-e2e.js registered in deploy.yml next to the #254 line; the new unit file registered in test-all.sh. Relates to #259 Co-Authored-By: Claude Opus 5 --- .github/workflows/deploy.yml | 1 + test-all.sh | 1 + test-issue-254-affinity-debug-toggle.js | 39 ++++ ...sue-254-affinity-toggle-mobile-aria-e2e.js | 21 +- test-issue-259-mobile-expanded-group-e2e.js | 183 ++++++++++++++++++ test-issue-259-nodes-esc-listener.js | 182 +++++++++++++++++ test-packets.js | 95 +++++++++ 7 files changed, 520 insertions(+), 2 deletions(-) create mode 100644 test-issue-259-mobile-expanded-group-e2e.js create mode 100644 test-issue-259-nodes-esc-listener.js diff --git a/.github/workflows/deploy.yml b/.github/workflows/deploy.yml index 954d29a3a..15150e8bb 100644 --- a/.github/workflows/deploy.yml +++ b/.github/workflows/deploy.yml @@ -445,6 +445,7 @@ jobs: BASE_URL=http://localhost:13581 node test-issue-1128-packets-layout-e2e.js 2>&1 | tee -a e2e-output.txt BASE_URL=http://localhost:13581 node test-issue-189-group-caret-e2e.js 2>&1 | tee -a e2e-output.txt BASE_URL=http://localhost:13581 node test-issue-254-affinity-toggle-mobile-aria-e2e.js 2>&1 | tee -a e2e-output.txt + BASE_URL=http://localhost:13581 node test-issue-259-mobile-expanded-group-e2e.js 2>&1 | tee -a e2e-output.txt BASE_URL=http://localhost:13581 node test-issue-1128-multi-viewport-e2e.js 2>&1 | tee -a e2e-output.txt BASE_URL=http://localhost:13581 node test-issue-1136-live-region-e2e.js 2>&1 | tee -a e2e-output.txt BASE_URL=http://localhost:13581 node test-live-multibyte-only-e2e.js 2>&1 | tee -a e2e-output.txt diff --git a/test-all.sh b/test-all.sh index 9ce3bdbcc..921bbfa56 100755 --- a/test-all.sh +++ b/test-all.sh @@ -58,6 +58,7 @@ run test-important-links-byte-filter.js run test-url-state.js run test-node-adverts.js run test-issue-254-affinity-debug-toggle.js +run test-issue-259-nodes-esc-listener.js run test-perf-go-runtime.js run test-channel-psk-ux.js run test-channel-sidebar-layout.js diff --git a/test-issue-254-affinity-debug-toggle.js b/test-issue-254-affinity-debug-toggle.js index 7e388e0ae..9c98754ee 100644 --- a/test-issue-254-affinity-debug-toggle.js +++ b/test-issue-254-affinity-debug-toggle.js @@ -155,6 +155,45 @@ test('collapsed shows exactly one caret, caret-right', () => { assert.deepStrictEqual(caretsIn(html), ['caret-right'], 'no other caret in the card'); }); +// #259 (3): the cases above all exercise renderAffinityDebugCard() in isolation, +// so they stay green if the loadFullNode template keeps the old inline-onclick +// card and the renderer is merely exported. The review of #255 showed that +// mutant passing 9/9 here while only the E2E caught it. These three read the +// source so the fast layer catches it too. +console.log('\n=== #259 the node page actually uses the renderer ==='); + +const SRC = fs.readFileSync('public/nodes.js', 'utf8'); +const CARD_ID = 'id="node-affinity-debug"'; + +test('#259: the loadFullNode template interpolates ${renderAffinityDebugCard()}', () => { + assert( + SRC.includes('${renderAffinityDebugCard()}'), + 'public/nodes.js must render the card through the renderer, not inline markup' + ); +}); + +test('#259: the card markup lives only in renderAffinityDebugCard()', () => { + const n = SRC.split(CARD_ID).length - 1; + assert.strictEqual(n, 1, 'exactly one ' + CARD_ID + ' in public/nodes.js, found ' + n); + const body = /function renderAffinityDebugCard\(\)\s*\{[\s\S]*?\n \}/.exec(SRC); + assert(body, 'renderAffinityDebugCard() is defined in public/nodes.js'); + assert(body[0].includes(CARD_ID), 'that one occurrence is the renderer\'s own'); +}); + +test('#259: no inline on*= handler in the Affinity Debug card source', () => { + // A window around every occurrence of the card id, so a reinstated inline + // onclick on the heading is caught wherever in the file it sits. + let from = 0, i; + while ((i = SRC.indexOf(CARD_ID, from)) !== -1) { + const window_ = SRC.slice(Math.max(0, i - 200), i + 800); + const handlers = window_.match(/\son[a-z]+\s*=\s*["'`]/gi); + assert(!handlers, 'inline handler(s) near ' + CARD_ID + ': ' + JSON.stringify(handlers)); + from = i + CARD_ID.length; + } + const affinityOnclick = /onclick="[^"]*affinity/i.exec(SRC); + assert(!affinityOnclick, 'inline affinity handler: ' + (affinityOnclick && affinityOnclick[0])); +}); + console.log('\n=== #254 Affinity Debug card: toggle ==='); test('the toggle opens the body: aria-expanded true, caret-down', () => { diff --git a/test-issue-254-affinity-toggle-mobile-aria-e2e.js b/test-issue-254-affinity-toggle-mobile-aria-e2e.js index b85361ba6..7c6186cfa 100644 --- a/test-issue-254-affinity-toggle-mobile-aria-e2e.js +++ b/test-issue-254-affinity-toggle-mobile-aria-e2e.js @@ -13,8 +13,9 @@ * the "Seed grouped-packet row for #1486" step in deploy.yml): * - 1400 px: the #189 toggle, data-action="toggle-select" with aria-expanded; * - 390 px (touch): mobile-page-actions.js (#1461 #7) makes activation - * select, so the row is select-hash without aria-expanded, and a tap or - * Enter opens the detail sheet instead of expanding the group; + * select, so the row is select-hash without aria-expanded, and a tap, + * Enter or Space (#259 item 4) opens the detail sheet instead of + * expanding the group; * - resizing 1400 → 390 → 1400 re-renders the row for each side. * * Usage: BASE_URL=http://localhost:13581 node test-issue-254-affinity-toggle-mobile-aria-e2e.js @@ -288,6 +289,22 @@ async function openPackets(page) { const s = await rowState(page); assert(s && !s.expanded && s.children === 0, 'Enter did not expand the group: ' + JSON.stringify(s)); }); + + // #259 (4): the row handler treats Enter and Space the same + // (packets.js, the keydown branch); the original #254 E2E only pressed + // Enter, so a mutant that dropped Space went unnoticed here. + await step('390 px: Space on the focused row selects too, it does not expand', async () => { + await page.evaluate(() => { const s = document.getElementById('mobileDetailSheet'); if (s) s.classList.remove('open'); }); + await page.focus(ROW); + await page.keyboard.press('Space'); + await page.waitForFunction(() => { + const s = document.getElementById('mobileDetailSheet'); + return !!s && s.classList.contains('open'); + }, null, { timeout: 8000 }); + const s = await rowState(page); + assert(s && !s.expanded && s.children === 0, 'Space did not expand the group: ' + JSON.stringify(s)); + assert(isMobileRow(s), 'still the mobile row after Space: ' + JSON.stringify(s)); + }); await ctx.close(); } diff --git a/test-issue-259-mobile-expanded-group-e2e.js b/test-issue-259-mobile-expanded-group-e2e.js new file mode 100644 index 000000000..ad8aa04e7 --- /dev/null +++ b/test-issue-259-mobile-expanded-group-e2e.js @@ -0,0 +1,183 @@ +/** + * E2E (#259 item 1): a packet group expanded on desktop must not leave visible + * child rows behind when the layout crosses to the mobile breakpoint. + * + * At <= 600 px mobile-page-actions.js (#1461 #7) hides the expand column and + * makes activating a group row select it, and #255 renders the row as + * select-hash without aria-expanded. Before this fix the row still carried + * class="expanded" and its child rows stayed on screen with nothing left to + * collapse them: the dead end upstream #1461 #7 describes. + * + * The fix keeps the hash in expandedHashes but leaves the children out of the + * rendered slice while the mobile mode is active, so: + * - 1400 px: expanding shows the children and aria-expanded="true" (#248/#255); + * - 390 px: no child rows, no expanded class, no aria-expanded; + * - back at 1400 px: the children and the expanded state are back. + * + * Runs against the fixture's seeded 3-observation group (see the "Seed + * grouped-packet row for #1486" step in deploy.yml). + * + * Usage: BASE_URL=http://localhost:13581 node test-issue-259-mobile-expanded-group-e2e.js + * SCREENSHOT_DIR= also saves screenshots (390 px and 1400 px, light and dark). + */ +'use strict'; +const path = require('path'); +const { chromium } = require('playwright'); + +const BASE = process.env.BASE_URL || 'http://localhost:13581'; +const SHOTS = process.env.SCREENSHOT_DIR || ''; +const SEED_HASH = 'fae0c9e6d357a814'; +const ROW = `#pktBody tr[data-hash="${SEED_HASH}"]`; + +let passed = 0, failed = 0; +async function step(name, fn) { + try { await fn(); passed++; console.log(' ✓ ' + name); } + catch (e) { failed++; console.error(' ✗ ' + name + ': ' + e.message); } +} +function assert(c, m) { if (!c) throw new Error(m || 'assertion failed'); } + +async function shot(page, name) { + if (!SHOTS) return; + await page.waitForTimeout(400); + await page.screenshot({ path: path.join(SHOTS, '259-' + name + '.png'), fullPage: false }); +} + +// The rendered state of the seeded group row, plus whether its children are +// actually on screen (getClientRects, not just attached) and whether anything +// is left to collapse the group with. +async function rowState(page) { + return page.evaluate(([sel, hash]) => { + const tr = document.querySelector(sel); + if (!tr) return null; + const kids = [...document.querySelectorAll('#pktBody tr.group-child[data-parent-hash="' + hash + '"]')]; + const expandCell = tr.querySelector('td.col-expand'); + return { + action: tr.getAttribute('data-action'), + aria: tr.getAttribute('aria-expanded'), + expandedClass: tr.classList.contains('expanded'), + children: kids.length, + visibleChildren: kids.filter((k) => k.getClientRects().length > 0).length, + expandCellVisible: !!expandCell && expandCell.getClientRects().length > 0, + carets: expandCell ? [...expandCell.querySelectorAll('use')].map((u) => (u.getAttribute('href') || '').split('#')[1]) : [], + }; + }, [ROW, SEED_HASH]); +} + +async function waitRow(page, want) { + try { + await page.waitForFunction(([sel, w]) => { + const tr = document.querySelector(sel); + if (!tr) return false; + const action = tr.getAttribute('data-action'); + return w === 'desktop' ? action === 'toggle-select' : action === 'select-hash'; + }, [ROW, want], { timeout: 5000 }); + } catch (_) { /* reported below with the actual state */ } + const s = await rowState(page); + const action = want === 'desktop' ? 'toggle-select' : 'select-hash'; + assert(s && s.action === action, 'expected the ' + want + ' row, got ' + JSON.stringify(s)); + return s; +} + +async function openPackets(page) { + await page.goto(BASE + '/#/packets?hash=' + SEED_HASH + '&timeWindow=0', { waitUntil: 'domcontentloaded' }); + await page.waitForSelector(ROW, { timeout: 12000 }); + await page.waitForTimeout(300); +} + +async function expandAtDesktop(page) { + await waitRow(page, 'desktop'); + await page.click(ROW + ' td.col-expand'); + await page.waitForFunction((sel) => document.querySelector(sel).classList.contains('expanded'), ROW, { timeout: 8000 }); +} + +async function run(browser, theme) { + const ctx = await browser.newContext({ viewport: { width: 1400, height: 900 } }); + await ctx.addInitScript((t) => { localStorage.setItem('meshcore-theme', t); }, theme); + const page = await ctx.newPage(); + page.setDefaultTimeout(10000); + page.on('pageerror', (e) => console.error('[pageerror]', e.message)); + + await step(`[${theme}] 1400 px: expanding the group shows its children`, async () => { + await openPackets(page); + await expandAtDesktop(page); + const s = await rowState(page); + assert(s.aria === 'true', 'aria-expanded="true" on desktop: ' + JSON.stringify(s)); + assert(s.expandedClass, 'the row is marked expanded'); + assert(s.visibleChildren > 0, 'the children are on screen: ' + JSON.stringify(s)); + assert(s.carets.length === 1 && s.carets[0] === 'ph-caret-down', 'down caret while expanded: ' + JSON.stringify(s.carets)); + await shot(page, '1400-expanded-' + theme); + }); + + await step(`[${theme}] resize 1400 -> 390: no child row is left visible`, async () => { + await page.setViewportSize({ width: 390, height: 844 }); + await waitRow(page, 'mobile'); + const s = await rowState(page); + assert(s.visibleChildren === 0, 'no visible child rows on mobile: ' + JSON.stringify(s)); + assert(s.children === 0, 'the child rows are not rendered at all: ' + JSON.stringify(s)); + assert(!s.expandedClass, 'the row does not claim to be expanded: ' + JSON.stringify(s)); + assert(s.aria === null, 'and it announces no expanded state (#255)'); + assert(!s.expandCellVisible, 'the expand column is hidden at 390 px, as before'); + await shot(page, '390-after-resize-' + theme); + }); + + await step(`[${theme}] resize 390 -> 1400: the children and the state come back`, async () => { + await page.setViewportSize({ width: 1400, height: 900 }); + await waitRow(page, 'desktop'); + const s = await rowState(page); + assert(s.aria === 'true', 'the expansion was kept, not cleared: ' + JSON.stringify(s)); + assert(s.expandedClass, 'the row is marked expanded again'); + assert(s.visibleChildren > 0, 'the children are back: ' + JSON.stringify(s)); + assert(s.carets.length === 1 && s.carets[0] === 'ph-caret-down', 'down caret again: ' + JSON.stringify(s.carets)); + }); + + await step(`[${theme}] 1400 px: the group still collapses from the caret`, async () => { + await page.click(ROW + ' td.col-expand'); + await page.waitForFunction((sel) => !document.querySelector(sel).classList.contains('expanded'), ROW, { timeout: 8000 }); + const s = await rowState(page); + assert(s.aria === 'false' && s.children === 0, 'collapsed again: ' + JSON.stringify(s)); + assert(s.carets.length === 1 && s.carets[0] === 'ph-caret-right', 'right caret while collapsed: ' + JSON.stringify(s.carets)); + }); + + await ctx.close(); +} + +(async () => { + const browser = await chromium.launch({ + headless: true, + executablePath: process.env.CHROMIUM_PATH || undefined, + args: ['--no-sandbox', '--disable-gpu', '--disable-dev-shm-usage'], + }); + + console.log(`\n=== #259 expanded group across the mobile breakpoint, against ${BASE} ===`); + + await run(browser, 'light'); + await run(browser, 'dark'); + + // A touch context that starts at 390 px: the same group deep-linked and + // expanded by the URL must not render children there either. Reaching the + // row by hash keeps the state out of a desktop render entirely. + { + const ctx = await browser.newContext({ viewport: { width: 390, height: 844 }, isMobile: true, hasTouch: true }); + const page = await ctx.newPage(); + page.setDefaultTimeout(10000); + page.on('pageerror', (e) => console.error('[pageerror]', e.message)); + + await step('390 px touch: a tap selects and leaves no child rows behind', async () => { + await openPackets(page); + await waitRow(page, 'mobile'); + await page.tap(ROW + ' td.col-time'); + await page.waitForFunction(() => { + const s = document.getElementById('mobileDetailSheet'); + return !!s && s.classList.contains('open'); + }, null, { timeout: 8000 }); + const s = await rowState(page); + assert(s.visibleChildren === 0 && !s.expandedClass, 'no children after a tap: ' + JSON.stringify(s)); + await shot(page, '390-touch-sheet'); + }); + await ctx.close(); + } + + await browser.close(); + console.log(`\n${passed} passed, ${failed} failed`); + process.exit(failed === 0 ? 0 : 1); +})().catch((e) => { console.error(e); process.exit(1); }); diff --git a/test-issue-259-nodes-esc-listener.js b/test-issue-259-nodes-esc-listener.js new file mode 100644 index 000000000..62d925829 --- /dev/null +++ b/test-issue-259-nodes-esc-listener.js @@ -0,0 +1,182 @@ +/** + * #259 (2): the node page's Escape-to-go-back listener must not leak. + * + * public/nodes.js init() wires a document-level `keydown` handler (`nodesEsc`) + * for the full-screen node view. It used to be a fresh closure per init() and + * was only ever removed when Escape actually fired, so navigating node A -> B + * -> C left three live listeners on `document` and one Escape then wrote the + * same hash three times. + * + * This test drives the real nodes.js in a vm sandbox with a counting + * document.addEventListener / removeEventListener, runs the router's + * destroy/init cycle for three node pages and asserts: + * - at most one document `keydown` listener survives; + * - Escape writes location.hash exactly once; + * - destroy() leaves no document `keydown` listener behind; + * - the listener still works on a page opened after a destroy. + * + * Usage: node test-issue-259-nodes-esc-listener.js + */ +'use strict'; +const vm = require('vm'); +const fs = require('fs'); +const assert = require('assert'); + +let passed = 0, failed = 0; +function test(name, fn) { + try { fn(); passed++; console.log(' ✅ ' + name); } + catch (e) { failed++; console.log(' ❌ ' + name + ': ' + e.message); } +} + +// A sandbox that records every document-level listener and the hash writes. +function loadNodes() { + const noop = () => {}; + const docListeners = []; // { type, fn } + const hashWrites = []; + const pages = {}; + const els = new Map(); + function el(id) { + if (!els.has(id)) { + els.set(id, { + id, innerHTML: '', textContent: '', style: {}, isConnected: true, + classList: { add: noop, remove: noop, toggle: noop, contains: () => false }, + addEventListener: noop, removeEventListener: noop, + querySelector: () => null, querySelectorAll: () => [], + appendChild: noop, setAttribute: noop, getAttribute: () => null, + closest: () => null, focus: noop, scrollIntoView: noop, + }); + } + return els.get(id); + } + const location = { + _hash: '#/nodes/aa', + get hash() { return this._hash; }, + set hash(v) { hashWrites.push(v); this._hash = v; }, + search: '', pathname: '/', href: 'http://localhost/', + }; + const ctx = { + window: { + addEventListener: noop, removeEventListener: noop, dispatchEvent: noop, + innerWidth: 1400, innerHeight: 900, location, + }, + document: { + readyState: 'complete', + body: { classList: { add: noop, remove: noop, toggle: noop, contains: () => false } }, + createElement: () => el('created'), + head: { appendChild: noop }, + getElementById: (id) => el(id), + addEventListener: (type, fn) => { docListeners.push({ type, fn }); }, + removeEventListener: (type, fn) => { + const i = docListeners.findIndex((l) => l.type === type && l.fn === fn); + if (i >= 0) docListeners.splice(i, 1); + }, + querySelectorAll: () => [], + querySelector: () => null, + }, + console: { log: noop, warn: noop, error: noop, debug: noop }, + Date, Math, Array, Object, String, Number, JSON, RegExp, Error, TypeError, + Map, Set, Promise, URLSearchParams, parseInt, parseFloat, isNaN, isFinite, + encodeURIComponent, decodeURIComponent, + setTimeout: noop, clearTimeout: noop, setInterval: noop, clearInterval: noop, + fetch: () => Promise.resolve({ ok: true, json: () => Promise.resolve({}) }), + performance: { now: () => Date.now() }, + localStorage: { getItem: () => null, setItem: noop, removeItem: noop }, + location, + history: { replaceState: noop, pushState: noop }, + CustomEvent: class CustomEvent {}, + requestAnimationFrame: noop, + ROLE_COLORS: {}, ROLE_STYLE: {}, TYPE_COLORS: {}, + getNodeStatus: () => 'active', + getHealthThresholds: () => ({ staleMs: 1, degradedMs: 2, silentMs: 3 }), + timeAgo: () => '', truncate: (s) => s, escapeHtml: (s) => String(s == null ? '' : s), + payloadTypeName: () => '', payloadTypeColor: () => '', + registerPage: (name, page) => { pages[name] = page; }, + RegionFilter: { init: noop, onChange: () => noop, offChange: noop, getRegionParam: () => '' }, + debouncedOnWS: () => null, onWS: noop, offWS: noop, debounce: (fn) => fn, + api: () => Promise.resolve({}), invalidateApiCache: noop, + CLIENT_TTL: { nodeList: 1, nodeDetail: 1, nodeHealth: 1 }, + initTabBar: noop, getFavorites: () => [], favStar: () => '', bindFavStars: noop, + makeColumnsResizable: noop, + getHashParams: () => new URLSearchParams(''), + AreaFilter: { init: noop, onChange: () => noop, offChange: noop, getParam: () => '', get: () => null }, + }; + ctx.window.document = ctx.document; + vm.createContext(ctx); + vm.runInContext(fs.readFileSync('public/nodes.js', 'utf8'), ctx, { filename: 'public/nodes.js' }); + return { + page: pages.nodes, + app: el('app'), + keydownListeners: () => docListeners.filter((l) => l.type === 'keydown'), + hashWrites, + pressEscape: () => { + for (const l of docListeners.filter((x) => x.type === 'keydown').slice()) { + l.fn({ key: 'Escape', preventDefault: () => {}, stopPropagation: () => {} }); + } + }, + }; +} + +console.log('\n=== #259 node page: one Escape listener, not one per visit ==='); + +test('nodes.js registers a page with init and destroy', () => { + const s = loadNodes(); + assert(s.page, 'registerPage("nodes", …) ran'); + assert.strictEqual(typeof s.page.init, 'function', 'init'); + assert.strictEqual(typeof s.page.destroy, 'function', 'destroy'); +}); + +test('a single node page leaves exactly one document keydown listener', () => { + const s = loadNodes(); + s.page.init(s.app, 'aa'); + assert.strictEqual(s.keydownListeners().length, 1, 'got ' + s.keydownListeners().length); +}); + +test('navigating A -> B -> C does not stack listeners', () => { + const s = loadNodes(); + for (const pk of ['aa', 'bb', 'cc']) { + s.page.destroy(); // the router destroys the current page first + s.page.init(s.app, pk); + } + const n = s.keydownListeners().length; + assert(n <= 1, 'expected at most one keydown listener after three visits, got ' + n); +}); + +test('after A -> B -> C one Escape writes the hash exactly once', () => { + const s = loadNodes(); + for (const pk of ['aa', 'bb', 'cc']) { + s.page.destroy(); + s.page.init(s.app, pk); + } + s.hashWrites.length = 0; + s.pressEscape(); + assert.deepStrictEqual(s.hashWrites, ['#/nodes'], 'hash writes: ' + JSON.stringify(s.hashWrites)); +}); + +test('destroy() removes the Escape listener', () => { + const s = loadNodes(); + s.page.init(s.app, 'aa'); + s.page.destroy(); + assert.strictEqual(s.keydownListeners().length, 0, 'got ' + s.keydownListeners().length); + s.hashWrites.length = 0; + s.pressEscape(); + assert.deepStrictEqual(s.hashWrites, [], 'a destroyed page does not navigate: ' + JSON.stringify(s.hashWrites)); +}); + +test('Escape still goes back on a page opened after a destroy', () => { + const s = loadNodes(); + s.page.init(s.app, 'aa'); + s.page.destroy(); + s.page.init(s.app, 'bb'); + s.hashWrites.length = 0; + s.pressEscape(); + assert.deepStrictEqual(s.hashWrites, ['#/nodes'], 'hash writes: ' + JSON.stringify(s.hashWrites)); +}); + +test('the nodes list view adds no Escape listener', () => { + const s = loadNodes(); + s.page.init(s.app, null); + assert.strictEqual(s.keydownListeners().length, 0, 'the list view has no Escape handler'); +}); + +console.log(`\n${passed} passed, ${failed} failed`); +process.exit(failed === 0 ? 0 : 1); diff --git a/test-packets.js b/test-packets.js index e117c58a0..8a8810580 100644 --- a/test-packets.js +++ b/test-packets.js @@ -1267,6 +1267,101 @@ console.log('\n=== packets.js: group row action and aria-expanded by viewport (# }); } +// #259 (1): a group expanded on desktop must not leave visible child rows behind +// when the layout flips to the mobile mode, where the expand column is hidden +// and the row only selects — there would be no way to collapse it again. The +// hash stays in expandedHashes, so the children come back on desktop; the +// rendered row is the collapsed one while the mobile mode is active. +console.log('\n=== packets.js: an expanded group across the mobile breakpoint (#259) ==='); +{ + const ctx = loadPacketsSandbox(); + loadInCtx(ctx, 'public/mobile-page-actions.js'); + const api = ctx._packetsTestAPI; + const mkChild = (id, obs) => ({ + id, observer_id: obs, hash: 'grp259', raw_hex: 'aabbcc', payload_type: 0, + route_type: 0, decoded_json: '{}', path_json: '[]', timestamp: '2024-01-01T00:00:00Z' + }); + const group = { + hash: 'grp259', count: 3, latest: '2024-01-01T00:00:00Z', + observer_id: null, raw_hex: 'aabbcc', payload_type: 0, + route_type: 0, decoded_json: '{}', path_json: '[]', + observation_count: 3, observer_count: 3, + _children: [mkChild(1, '1'), mkChild(2, '2'), mkChild(3, '3')] + }; + const header = (html) => /]*>/.exec(html)[0]; + const rowClass = (html) => (/ (html.match(/ { + const prev = ctx.window.innerWidth; + ctx.window.innerWidth = w; + try { return fn(); } finally { ctx.window.innerWidth = prev; } + }; + + // _getRowCount only counts children in grouped mode; the hook lets the + // sandbox say so. Guarded so this file still runs against a tree without it. + if (typeof api._setDisplayGrouped === 'function') api._setDisplayGrouped(true); + api._setExpanded(group.hash, true); + + test('#259: at 1400 px the expanded group renders its children (#248 unchanged)', () => atWidth(1400, () => { + const html = api.buildGroupRowHtml(group); + assert.strictEqual(childRows(html), 3, 'three child rows: ' + childRows(html)); + assert(/\bexpanded\b/.test(rowClass(html)), 'the row is marked expanded: ' + rowClass(html)); + assert(header(html).includes('aria-expanded="true"'), header(html)); + assert(expandCell(html).includes(phIcon('caret-down')), 'down caret while expanded'); + })); + + test('#259: at 390 px the same expanded group renders no child rows', () => atWidth(390, () => { + const html = api.buildGroupRowHtml(group); + assert.strictEqual(childRows(html), 0, 'no visible children on mobile, got ' + childRows(html)); + })); + + test('#259: at 390 px the row does not claim the expanded class or caret', () => atWidth(390, () => { + const html = api.buildGroupRowHtml(group); + assert(!/\bexpanded\b/.test(rowClass(html)), 'no expanded class on mobile: ' + rowClass(html)); + assert(header(html).includes('data-action="select-hash"'), header(html)); + assert(!header(html).includes('aria-expanded'), 'still no aria-expanded on mobile'); + assert(!expandCell(html).includes(phIcon('caret-down')), 'no down caret on mobile'); + })); + + test('#259: _getRowCount matches the rendered rows on both sides of the breakpoint', () => { + atWidth(390, () => { + assert.strictEqual(api._getRowCount(group), 1, 'mobile: the group is one row'); + }); + atWidth(1400, () => { + assert.strictEqual(api._getRowCount(group), 4, 'desktop: the group plus three children'); + }); + }); + + test('#259: 600 px hides the children and 601 px shows them again', () => { + atWidth(600, () => { + assert.strictEqual(childRows(api.buildGroupRowHtml(group)), 0, 'at the breakpoint the children are hidden'); + }); + atWidth(601, () => { + assert.strictEqual(childRows(api.buildGroupRowHtml(group)), 3, 'just above it they are back'); + }); + }); + + test('#259: the expansion survives 1400 -> 390 -> 1400, it is not cleared', () => { + atWidth(390, () => { api.buildGroupRowHtml(group); }); + atWidth(1400, () => { + const html = api.buildGroupRowHtml(group); + assert.strictEqual(childRows(html), 3, 'the children are back on desktop: ' + childRows(html)); + assert(header(html).includes('aria-expanded="true"'), 'and the state is still expanded'); + }); + }); + + test('#259: a collapsed group is unaffected at either width', () => { + api._setExpanded(group.hash, false); + try { + for (const w of [390, 1400]) atWidth(w, () => { + const html = api.buildGroupRowHtml(group); + assert.strictEqual(childRows(html), 0, w + ' px: a collapsed group has no children'); + assert(!/\bexpanded\b/.test(rowClass(html)), w + ' px: ' + rowClass(html)); + }); + } finally { api._setExpanded(group.hash, true); } + }); +} + // Without mobile-page-actions.js there is no #1461 #7 redirect, so a group row // toggles at any width. console.log('\n=== packets.js: group row without mobile-page-actions.js (#254) ==='); From ffd211336b8a7df138f0457ef86a2740dd463059 Mon Sep 17 00:00:00 2001 From: dborup Date: Mon, 5 Oct 2026 16:04:29 +0200 Subject: [PATCH 2/7] fix(packets): an expanded group leaves no orphan children on mobile (#259) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A group expanded at desktop width kept `class="expanded"` and its child rows after the layout crossed to <= 600 px. There the expand column is hidden and the row only selects (#1461 #7, #255), so nothing was left to collapse it: the dead end upstream #1461 #7 describes, and finding F2 of the #255 review. Keep the hash in `expandedHashes` and leave the children out of the rendered slice while the mobile mode is active, rather than clearing the set on the flip. Two reasons: - the 600 px line is crossed in both directions by a phone rotating (390x844 is mobile, 844x390 is not), so clearing would discard the user's expansion on every rotation; suppressing the rows restores it on the way back; - the dead end also appears on a *first* render at a narrow width — the #866 deep link #/packets// adds the hash to `expandedHashes` before any row is built — which a mode-flip hook alone would not catch. One render-time helper, `groupIsExpandedInView()`, is used by both `buildGroupRowHtml` and `_getRowCount`, so the virtual-scroll row counts and the rendered rows cannot diverge (#424). The mode-flip resize handler now also invalidates the cached counts, because an expanded group's row count changes with the mode. Desktop rendering is untouched: carets, `aria-expanded` and `expandedHashes` behave exactly as in #248/#255. `_setDisplayGrouped` joins the existing test hooks so the unit sandbox can exercise `_getRowCount` in grouped mode. Relates to #259 Co-Authored-By: Claude Opus 5 --- public/packets.js | 20 ++++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-) diff --git a/public/packets.js b/public/packets.js index 826c3cd65..792d49a8b 100644 --- a/public/packets.js +++ b/public/packets.js @@ -2410,6 +2410,18 @@ return !!(window.MobilePageActions && window.MobilePageActions.isMobile()); } + // #259: a group expanded on desktop used to keep its child rows after the + // layout crossed to the mobile mode, where the expand column is hidden and + // the row only selects — nothing was left to collapse it with (the dead end + // upstream #1461 #7 describes). The hash stays in expandedHashes, so the + // children come back on desktop and a phone rotation does not throw the + // state away; it is only the rendered slice that leaves them out. This also + // covers the first render at a narrow width, e.g. the #866 deep link + // #/packets//, which expands the hash before any render. + function groupIsExpandedInView(hash) { + return expandedHashes.has(hash) && !groupRowSelectsOnActivate(); + } + // A group row's action and aria-expanded depend on that breakpoint, so a // resize that crosses it (e.g. a phone rotating) re-renders the visible rows. let _groupRowsSelect = false; @@ -2421,6 +2433,9 @@ if (selects === _groupRowsSelect) return; _groupRowsSelect = selects; if (!_displayGrouped) return; + // #259: crossing the breakpoint changes how many DOM rows an expanded + // group produces, so the cached per-entry counts are stale. + _invalidateRowCounts(); _lastVisibleStart = -1; _lastVisibleEnd = -1; renderVisibleRows(); @@ -2429,7 +2444,7 @@ // Build HTML for a single grouped packet row function buildGroupRowHtml(p, entryIdx = -1) { - const isExpanded = expandedHashes.has(p.hash); + const isExpanded = groupIsExpandedInView(p.hash); let headerObserverId = p.observer_id; let headerPathJson = p.path_json; if (_observerFilterSet && p._children?.length) { @@ -2575,7 +2590,7 @@ // Used by both row counting and renderVisibleRows to avoid divergence (#424). function _getRowCount(p) { if (!_displayGrouped) return 1; - if (!expandedHashes.has(p.hash) || !p._children) return 1; + if (!groupIsExpandedInView(p.hash) || !p._children) return 1; let childCount = p._children.length; if (_observerFilterSet) { childCount = p._children.filter(c => _observerFilterSet.has(String(c.observer_id))).length; @@ -4335,6 +4350,7 @@ _setPackets: function(p) { packets = p; }, _setFilter: function(k, v) { filters[k] = v; }, _setExpanded: function(hash, on) { if (on) expandedHashes.add(hash); else expandedHashes.delete(hash); }, + _setDisplayGrouped: function(on) { _displayGrouped = !!on; }, }; } From fdb5fe6fe33934d3d3541982bde6dfa6cf734f55 Mon Sep 17 00:00:00 2001 From: dborup Date: Mon, 5 Oct 2026 16:04:29 +0200 Subject: [PATCH 3/7] fix(nodes): one Escape listener per node page, not one per visit (#259) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `init()` added a fresh `nodesEsc` closure to `document` on every full-screen node-page init and only unhooked it when Escape actually fired. Navigating node A -> B -> C therefore left three live listeners, and one Escape wrote the same hash three times. Finding F4 of the #255 review. The handler is now a single module-level `_nodesEsc`, so a repeat `addEventListener` is a DOM no-op, and `destroy()` removes it — which covers the router's destroy/init cycle and the in-module `navigateToNode` path. Relates to #259 Co-Authored-By: Claude Opus 5 --- public/nodes.js | 25 ++++++++++++++++++------- 1 file changed, 18 insertions(+), 7 deletions(-) diff --git a/public/nodes.js b/public/nodes.js index 4928620e9..78b13223f 100644 --- a/public/nodes.js +++ b/public/nodes.js @@ -582,6 +582,15 @@ let directNode = null; // set when navigating directly to #/nodes/:pubkey + // #259: the full-screen node view's Escape-to-go-back handler. Module level + // so every init() registers the same reference and destroy() can remove it. + function _nodesEsc(e) { + if (e.key === 'Escape') { + document.removeEventListener('keydown', _nodesEsc); + location.hash = '#/nodes'; + } + } + let regionChangeHandler = null; function init(app, routeParam) { @@ -606,13 +615,12 @@ // would stack listeners. document.getElementById('nodeFullBody').addEventListener('click', onFullBodyClick); loadFullNode(directNode); - // Escape to go back to nodes list - document.addEventListener('keydown', function nodesEsc(e) { - if (e.key === 'Escape') { - document.removeEventListener('keydown', nodesEsc); - location.hash = '#/nodes'; - } - }); + // Escape to go back to nodes list. #259: one listener, not one per visit. + // It used to be a fresh closure per init() that only unhooked itself when + // Escape fired, so node A -> B -> C stacked three and one Escape wrote the + // same hash three times. _nodesEsc is a stable module-level reference, so + // a repeat add is a DOM no-op, and destroy() takes it off again. + document.addEventListener('keydown', _nodesEsc); return; } @@ -1250,6 +1258,9 @@ function destroy() { if (wsHandler) offWS(wsHandler); wsHandler = null; + // #259: the full-screen view's Escape handler is document-level, so it + // outlives the page's DOM unless it is removed here. + document.removeEventListener('keydown', _nodesEsc); closeDetailView(); if (regionChangeHandler) RegionFilter.offChange(regionChangeHandler); regionChangeHandler = null; From 0ed1a3df65575fc08b2aa58d9b69ca9623a72bf5 Mon Sep 17 00:00:00 2001 From: dborup Date: Mon, 5 Oct 2026 16:11:33 +0200 Subject: [PATCH 4/7] test(nodes): dedupe the #259 sandbox's listeners as the DOM does The counting document.addEventListener in the new test pushed every registration, so it reported three listeners even for a stable handler reference that the real DOM would have registered once. It now deduplicates on (type, handler), which makes the count the listener count rather than the number of add() calls. Both mutants are still killed: master's per-init closure stacks three listeners, and keeping the stable reference while dropping the destroy() removal fails the "destroy() removes the Escape listener" case. Relates to #259 Co-Authored-By: Claude Opus 5 --- test-issue-259-nodes-esc-listener.js | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/test-issue-259-nodes-esc-listener.js b/test-issue-259-nodes-esc-listener.js index 62d925829..0a86c89bb 100644 --- a/test-issue-259-nodes-esc-listener.js +++ b/test-issue-259-nodes-esc-listener.js @@ -7,8 +7,9 @@ * -> C left three live listeners on `document` and one Escape then wrote the * same hash three times. * - * This test drives the real nodes.js in a vm sandbox with a counting - * document.addEventListener / removeEventListener, runs the router's + * This test drives the real nodes.js in a vm sandbox whose + * document.addEventListener / removeEventListener record the live listeners and + * deduplicate on (type, handler) exactly as the DOM does. It runs the router's * destroy/init cycle for three node pages and asserts: * - at most one document `keydown` listener survives; * - Escape writes location.hash exactly once; @@ -65,7 +66,13 @@ function loadNodes() { createElement: () => el('created'), head: { appendChild: noop }, getElementById: (id) => el(id), - addEventListener: (type, fn) => { docListeners.push({ type, fn }); }, + // Deduplicated on (type, fn) exactly as the DOM does, so a stable + // handler reference registered twice counts once — the test measures + // real listener count, not add() calls. + addEventListener: (type, fn) => { + if (docListeners.some((l) => l.type === type && l.fn === fn)) return; + docListeners.push({ type, fn }); + }, removeEventListener: (type, fn) => { const i = docListeners.findIndex((l) => l.type === type && l.fn === fn); if (i >= 0) docListeners.splice(i, 1); From 8fd2be92229212846607f6a68f77bc568563d5e4 Mon Sep 17 00:00:00 2001 From: dborup Date: Mon, 5 Oct 2026 21:25:28 +0200 Subject: [PATCH 5/7] test(packets): make the mobile "does not expand" assertions bite (#259) Review F1 on #260: since groupIsExpandedInView(), a hash that *is* in expandedHashes renders at <= 600 px exactly like a collapsed row -- no `expanded` class, no child rows. The 390 px tap, Enter and Space steps asserted only on those two, so they could no longer fail on an expansion at that width: a mutant whose select-hash row toggles the group *and* opens the sheet passed every layer on this branch, while the same mutant on master's packets.js failed the #254 E2E twice. Two independent ways to see it again: - packets.js exports `_isExpanded(hash)` on the existing test API, so the steps read expandedHashes itself. A missing hook yields the string 'NO-HOOK', which fails the assertion rather than passing it. - both E2Es now resize back to 1400 px after the mobile activations and assert the group is still collapsed -- the render-level view, and what a user rotating a phone to landscape would actually see. The #259 E2E's 390 px resize step also asserts the other half of the fix at the state level: the expansion is kept in expandedHashes, not cleared. Review F4: the touch block's comment called ?hash= the #866 deep link. #866 is #/packets/?obs=, which does expand before the first render; ?hash= only filters. The comment now says which case the block covers and where the first-render-at-narrow-width case is covered. --- public/packets.js | 5 +++ ...sue-254-affinity-toggle-mobile-aria-e2e.js | 38 +++++++++++++++++-- test-issue-259-mobile-expanded-group-e2e.js | 35 +++++++++++++++-- 3 files changed, 71 insertions(+), 7 deletions(-) diff --git a/public/packets.js b/public/packets.js index 792d49a8b..16ddcd5fd 100644 --- a/public/packets.js +++ b/public/packets.js @@ -4350,6 +4350,11 @@ _setPackets: function(p) { packets = p; }, _setFilter: function(k, v) { filters[k] = v; }, _setExpanded: function(hash, on) { if (on) expandedHashes.add(hash); else expandedHashes.delete(hash); }, + // #259: at <= 600 px groupIsExpandedInView() renders a hash that *is* in + // expandedHashes exactly like a collapsed row, so the DOM alone can no + // longer tell "did not expand" from "expanded invisibly". The mobile E2E + // assertions read the set itself through this. + _isExpanded: function(hash) { return expandedHashes.has(hash); }, _setDisplayGrouped: function(on) { _displayGrouped = !!on; }, }; } diff --git a/test-issue-254-affinity-toggle-mobile-aria-e2e.js b/test-issue-254-affinity-toggle-mobile-aria-e2e.js index 7c6186cfa..a2a1276ad 100644 --- a/test-issue-254-affinity-toggle-mobile-aria-e2e.js +++ b/test-issue-254-affinity-toggle-mobile-aria-e2e.js @@ -15,7 +15,9 @@ * - 390 px (touch): mobile-page-actions.js (#1461 #7) makes activation * select, so the row is select-hash without aria-expanded, and a tap, * Enter or Space (#259 item 4) opens the detail sheet instead of - * expanding the group; + * expanding the group -- checked both through expandedHashes (the test + * hook, because at 390 px an expanded group renders exactly like a + * collapsed one) and by resizing back to 1400 px; * - resizing 1400 → 390 → 1400 re-renders the row for each side. * * Usage: BASE_URL=http://localhost:13581 node test-issue-254-affinity-toggle-mobile-aria-e2e.js @@ -127,19 +129,35 @@ async function affinityCard(browser, theme) { // --- 2. Packets group rows --------------------------------------------------- +// #259: since groupIsExpandedInView(), a hash in expandedHashes renders +// exactly like a collapsed row at <= 600 px -- no `expanded` class, no child +// rows. The DOM therefore cannot distinguish "did not expand" from "expanded +// invisibly", so the mobile steps below read expandedHashes itself through +// packets.js's test hook. `inExpandedHashes` is the string 'NO-HOOK' if the +// hook is gone, so a missing hook fails the assertion instead of passing it. async function rowState(page) { return page.evaluate(([sel, hash]) => { const tr = document.querySelector(sel); if (!tr) return null; + const api = window._packetsTestAPI; return { action: tr.getAttribute('data-action'), aria: tr.getAttribute('aria-expanded'), expanded: tr.classList.contains('expanded'), children: document.querySelectorAll('#pktBody tr.group-child[data-parent-hash="' + hash + '"]').length, + inExpandedHashes: api && typeof api._isExpanded === 'function' ? api._isExpanded(hash) : 'NO-HOOK', }; }, [ROW, SEED_HASH]); } +// "Activating the row selected it and did not expand the group": nothing +// visible, and nothing in expandedHashes either. +function assertNotExpanded(s, what) { + assert(s, what + ': no row'); + assert(!s.expanded && s.children === 0, what + ' left no expansion on screen: ' + JSON.stringify(s)); + assert(s.inExpandedHashes === false, what + ' did not expand the group: ' + JSON.stringify(s)); +} + const isDesktopRow = (s) => !!s && s.action === 'toggle-select' && s.aria === 'false'; const isMobileRow = (s) => !!s && s.action === 'select-hash' && s.aria === null; @@ -272,7 +290,7 @@ async function openPackets(page) { return !!s && s.classList.contains('open'); }, null, { timeout: 8000 }); const s = await rowState(page); - assert(s && !s.expanded && s.children === 0, 'the group did not expand: ' + JSON.stringify(s)); + assertNotExpanded(s, 'the tap'); assert(isMobileRow(s), 'still the mobile row after the re-render: ' + JSON.stringify(s)); await shot(page, 'packets-390-sheet'); await page.click('#mobileSheetClose').catch(() => {}); @@ -287,7 +305,7 @@ async function openPackets(page) { return !!s && s.classList.contains('open'); }, null, { timeout: 8000 }); const s = await rowState(page); - assert(s && !s.expanded && s.children === 0, 'Enter did not expand the group: ' + JSON.stringify(s)); + assertNotExpanded(s, 'Enter'); }); // #259 (4): the row handler treats Enter and Space the same @@ -302,9 +320,21 @@ async function openPackets(page) { return !!s && s.classList.contains('open'); }, null, { timeout: 8000 }); const s = await rowState(page); - assert(s && !s.expanded && s.children === 0, 'Space did not expand the group: ' + JSON.stringify(s)); + assertNotExpanded(s, 'Space'); assert(isMobileRow(s), 'still the mobile row after Space: ' + JSON.stringify(s)); }); + + // The same conclusion at the render level, independently of the hook: a + // group that one of the three activations above expanded silently shows up + // as soon as the layout goes back over the breakpoint -- which is what a + // user rotating the phone to landscape would see. + await step('back at 1400 px the group is still collapsed, so nothing expanded it', async () => { + await page.evaluate(() => { const s = document.getElementById('mobileDetailSheet'); if (s) s.classList.remove('open'); }); + await page.setViewportSize({ width: 1400, height: 900 }); + const s = await waitRow(page, isDesktopRow, 'desktop'); + assert(!s.expanded && s.children === 0, 'no child rows came back: ' + JSON.stringify(s)); + assert(s.inExpandedHashes === false, 'and the hash never entered expandedHashes: ' + JSON.stringify(s)); + }); await ctx.close(); } diff --git a/test-issue-259-mobile-expanded-group-e2e.js b/test-issue-259-mobile-expanded-group-e2e.js index ad8aa04e7..a538aa931 100644 --- a/test-issue-259-mobile-expanded-group-e2e.js +++ b/test-issue-259-mobile-expanded-group-e2e.js @@ -14,6 +14,11 @@ * - 390 px: no child rows, no expanded class, no aria-expanded; * - back at 1400 px: the children and the expanded state are back. * + * The touch block at the end checks the other direction: at 390 px an expanded + * group renders exactly like a collapsed one, so "activating the row did not + * expand it" is read off expandedHashes through packets.js's test hook and + * confirmed by resizing back over the breakpoint. + * * Runs against the fixture's seeded 3-observation group (see the "Seed * grouped-packet row for #1486" step in deploy.yml). * @@ -59,6 +64,11 @@ async function rowState(page) { visibleChildren: kids.filter((k) => k.getClientRects().length > 0).length, expandCellVisible: !!expandCell && expandCell.getClientRects().length > 0, carets: expandCell ? [...expandCell.querySelectorAll('use')].map((u) => (u.getAttribute('href') || '').split('#')[1]) : [], + // At <= 600 px a hash in expandedHashes renders exactly like a collapsed + // row, so "did not expand" has to be read off the set, not the DOM. + // 'NO-HOOK' if packets.js's test hook is gone, so its absence fails. + inExpandedHashes: window._packetsTestAPI && typeof window._packetsTestAPI._isExpanded === 'function' + ? window._packetsTestAPI._isExpanded(hash) : 'NO-HOOK', }; }, [ROW, SEED_HASH]); } @@ -116,6 +126,7 @@ async function run(browser, theme) { assert(s.children === 0, 'the child rows are not rendered at all: ' + JSON.stringify(s)); assert(!s.expandedClass, 'the row does not claim to be expanded: ' + JSON.stringify(s)); assert(s.aria === null, 'and it announces no expanded state (#255)'); + assert(s.inExpandedHashes === true, 'the expansion is kept in state, not cleared: ' + JSON.stringify(s)); assert(!s.expandCellVisible, 'the expand column is hidden at 390 px, as before'); await shot(page, '390-after-resize-' + theme); }); @@ -153,9 +164,14 @@ async function run(browser, theme) { await run(browser, 'light'); await run(browser, 'dark'); - // A touch context that starts at 390 px: the same group deep-linked and - // expanded by the URL must not render children there either. Reaching the - // row by hash keeps the state out of a desktop render entirely. + // A touch context that starts at 390 px, reached by ?hash= so the group is + // never rendered at desktop width first: activating the row must select it, + // and must neither render children nor put the hash into expandedHashes. + // + // ?hash= only filters -- it does not expand anything. The #866 deep link + // that *does* expand before the first render is #/packets/?obs=; + // the first-render-at-narrow-width case it produces is covered by the + // test-packets.js unit cases, which seed expandedHashes and render at 390 px. { const ctx = await browser.newContext({ viewport: { width: 390, height: 844 }, isMobile: true, hasTouch: true }); const page = await ctx.newPage(); @@ -172,8 +188,21 @@ async function run(browser, theme) { }, null, { timeout: 8000 }); const s = await rowState(page); assert(s.visibleChildren === 0 && !s.expandedClass, 'no children after a tap: ' + JSON.stringify(s)); + assert(s.inExpandedHashes === false, 'the tap did not expand the group: ' + JSON.stringify(s)); await shot(page, '390-touch-sheet'); }); + + // The same conclusion at the render level, without the hook: an expansion + // the tap made invisibly would be on screen as soon as the layout crosses + // back over the breakpoint. + await step('390 px touch -> 1400 px: the group is still collapsed', async () => { + await page.evaluate(() => { const s = document.getElementById('mobileDetailSheet'); if (s) s.classList.remove('open'); }); + await page.setViewportSize({ width: 1400, height: 900 }); + const s = await waitRow(page, 'desktop'); + assert(s.aria === 'false', 'aria-expanded="false" over the breakpoint: ' + JSON.stringify(s)); + assert(!s.expandedClass && s.children === 0, 'no children appeared: ' + JSON.stringify(s)); + assert(s.inExpandedHashes === false, 'and nothing entered expandedHashes: ' + JSON.stringify(s)); + }); await ctx.close(); } From 09ba33b499c22889c16104b31abfa17e0931eb00 Mon Sep 17 00:00:00 2001 From: dborup Date: Mon, 5 Oct 2026 21:25:44 +0200 Subject: [PATCH 6/7] test(nodes): cover the list view's Escape listener too (#259) Review F2 on #260: "the nodes list view adds no Escape listener" passed for the wrong reason. The list view does add one -- nodesPanelEsc in renderLeft() -- but only once the asynchronous loadNodes() resolves, and the case asserted synchronously before that. It stated something untrue about the page and could not fail. Replaced with cases that await the render (the sandbox's api() resolves immediately, so draining the microtask queue is enough) and then hold the list view to one listener. Review F3 is the leak they expose: - a region change re-runs loadNodes() -> renderLeft() inside one visit; - list -> node -> list -> node -> list across five router cycles; - destroy() must leave none behind. Supporting sandbox changes: loadNodes()'s catch is silent, so console.error is now recorded and every list-view case asserts it stayed empty -- a missing stub would otherwise read as "no listener". history.replaceState is recorded (nodesPanelEsc's observable effect), elements get a real Set-backed classList and a dataset/value, and #nodesRight starts .empty as the real markup renders it, so the handler's "only while a panel is open" guard is the browser's guard. Red on the parent commit: 2 listeners after two renders, 3 after the five cycles, 1 left after destroy(). --- test-issue-259-nodes-esc-listener.js | 147 ++++++++++++++++++++++++--- 1 file changed, 134 insertions(+), 13 deletions(-) diff --git a/test-issue-259-nodes-esc-listener.js b/test-issue-259-nodes-esc-listener.js index 0a86c89bb..f4be23692 100644 --- a/test-issue-259-nodes-esc-listener.js +++ b/test-issue-259-nodes-esc-listener.js @@ -7,6 +7,11 @@ * -> C left three live listeners on `document` and one Escape then wrote the * same hash three times. * + * The nodes *list* view has a sibling of the same class: `nodesPanelEsc` in + * renderLeft() (Escape closes the detail panel). renderLeft() runs on every + * load of the list -- a visit, a region change, a filter change -- so that one + * leaked one listener per render too. Both are covered here. + * * This test drives the real nodes.js in a vm sandbox whose * document.addEventListener / removeEventListener record the live listeners and * deduplicate on (type, handler) exactly as the DOM does. It runs the router's @@ -14,7 +19,13 @@ * - at most one document `keydown` listener survives; * - Escape writes location.hash exactly once; * - destroy() leaves no document `keydown` listener behind; - * - the listener still works on a page opened after a destroy. + * - the listener still works on a page opened after a destroy; + * - the list view's own Escape listener (`nodesPanelEsc`) does not stack + * across renders or visits, and destroy() removes it. + * + * The list-view cases await the asynchronous loadNodes() before counting: the + * listener is only registered once renderLeft() has run, so a synchronous + * assertion would pass no matter what the page does. * * Usage: node test-issue-259-nodes-esc-listener.js */ @@ -24,9 +35,21 @@ const fs = require('fs'); const assert = require('assert'); let passed = 0, failed = 0; -function test(name, fn) { - try { fn(); passed++; console.log(' ✅ ' + name); } - catch (e) { failed++; console.log(' ❌ ' + name + ': ' + e.message); } +const queue = []; +function test(name, fn) { queue.push([name, fn]); } +async function runQueue() { + for (const [name, fn] of queue) { + try { await fn(); passed++; console.log(' ✅ ' + name); } + catch (e) { failed++; console.log(' ❌ ' + name + ': ' + e.message); } + } +} + +// nodes.js's list view renders from the async loadNodes(); the sandbox's api() +// resolves immediately, so draining the microtask queue is enough to get the +// real renderLeft() to run. Nothing here depends on timers (the sandbox's are +// no-ops on purpose, so nothing fires behind the assertions). +async function settle() { + for (let i = 0; i < 50; i++) await Promise.resolve(); } // A sandbox that records every document-level listener and the hash writes. @@ -34,13 +57,24 @@ function loadNodes() { const noop = () => {}; const docListeners = []; // { type, fn } const hashWrites = []; + const replaceStateWrites = []; + const errors = []; // anything nodes.js's catch blocks log + let regionChange = null; // the list view's RegionFilter.onChange handler const pages = {}; const els = new Map(); function el(id) { if (!els.has(id)) { + // A real Set-backed classList, so nodesPanelEsc's "only act while a + // detail panel is open" guard behaves as it does in the browser. + const classes = new Set(); els.set(id, { id, innerHTML: '', textContent: '', style: {}, isConnected: true, - classList: { add: noop, remove: noop, toggle: noop, contains: () => false }, + dataset: {}, value: '', + classList: { + add: (c) => classes.add(c), remove: (c) => classes.delete(c), + toggle: (c, on) => (on ? classes.add(c) : classes.delete(c)), + contains: (c) => classes.has(c), + }, addEventListener: noop, removeEventListener: noop, querySelector: () => null, querySelectorAll: () => [], appendChild: noop, setAttribute: noop, getAttribute: () => null, @@ -80,7 +114,10 @@ function loadNodes() { querySelectorAll: () => [], querySelector: () => null, }, - console: { log: noop, warn: noop, error: noop, debug: noop }, + console: { log: noop, warn: noop, debug: noop, + // Surfaced by the list-view cases: loadNodes() swallows its own + // exceptions, so a missing stub would otherwise read as "no listener". + error: (...a) => { errors.push(a.map(String).join(' ')); } }, Date, Math, Array, Object, String, Number, JSON, RegExp, Error, TypeError, Map, Set, Promise, URLSearchParams, parseInt, parseFloat, isNaN, isFinite, encodeURIComponent, decodeURIComponent, @@ -89,7 +126,10 @@ function loadNodes() { performance: { now: () => Date.now() }, localStorage: { getItem: () => null, setItem: noop, removeItem: noop }, location, - history: { replaceState: noop, pushState: noop }, + history: { + replaceState: (_s, _t, url) => { replaceStateWrites.push(url); }, + pushState: noop, + }, CustomEvent: class CustomEvent {}, requestAnimationFrame: noop, ROLE_COLORS: {}, ROLE_STYLE: {}, TYPE_COLORS: {}, @@ -98,23 +138,40 @@ function loadNodes() { timeAgo: () => '', truncate: (s) => s, escapeHtml: (s) => String(s == null ? '' : s), payloadTypeName: () => '', payloadTypeColor: () => '', registerPage: (name, page) => { pages[name] = page; }, - RegionFilter: { init: noop, onChange: () => noop, offChange: noop, getRegionParam: () => '' }, + RegionFilter: { + init: noop, offChange: noop, getRegionParam: () => '', + onChange: (fn) => { regionChange = fn; return fn; }, + }, debouncedOnWS: () => null, onWS: noop, offWS: noop, debounce: (fn) => fn, api: () => Promise.resolve({}), invalidateApiCache: noop, CLIENT_TTL: { nodeList: 1, nodeDetail: 1, nodeHealth: 1 }, initTabBar: noop, getFavorites: () => [], favStar: () => '', bindFavStars: noop, makeColumnsResizable: noop, getHashParams: () => new URLSearchParams(''), - AreaFilter: { init: noop, onChange: () => noop, offChange: noop, getParam: () => '', get: () => null }, + AreaFilter: { + init: noop, onChange: noop, offChange: noop, getParam: () => '', + getAreaParam: () => '', getSelected: () => null, get: () => null, + }, + nodePassesGeoFilter: () => true, }; ctx.window.document = ctx.document; + // The real markup renders the right panel as class="panel-right empty", and + // nodesPanelEsc only acts while it is not .empty. Seed that, so the guard is + // the browser's guard and the cases have to open the panel to exercise it. + el('nodesRight').classList.add('empty'); vm.createContext(ctx); vm.runInContext(fs.readFileSync('public/nodes.js', 'utf8'), ctx, { filename: 'public/nodes.js' }); return { page: pages.nodes, app: el('app'), + panel: el('nodesRight'), keydownListeners: () => docListeners.filter((l) => l.type === 'keydown'), hashWrites, + replaceStateWrites, + errors, + // Re-runs the list view's load the way a region change does: one more + // renderLeft() inside the same visit. + regionChange: () => regionChange && regionChange(), pressEscape: () => { for (const l of docListeners.filter((x) => x.type === 'keydown').slice()) { l.fn({ key: 'Escape', preventDefault: () => {}, stopPropagation: () => {} }); @@ -179,11 +236,75 @@ test('Escape still goes back on a page opened after a destroy', () => { assert.deepStrictEqual(s.hashWrites, ['#/nodes'], 'hash writes: ' + JSON.stringify(s.hashWrites)); }); -test('the nodes list view adds no Escape listener', () => { +// --- the list view's own Escape listener (nodesPanelEsc) -------------------- +// +// The previous version of this block asserted that the list view adds no +// Escape listener at all. That passed for the wrong reason: it counted before +// the asynchronous loadNodes() -> renderLeft() had run. The list view does add +// one, and these cases await the render and hold it to one. + +test('the list view registers exactly one Escape listener once its load resolves', async () => { + const s = loadNodes(); + s.page.init(s.app, null); + await settle(); + assert.deepStrictEqual(s.errors, [], 'the list view rendered without errors'); + assert.strictEqual(s.keydownListeners().length, 1, + 'one list-view Escape listener, got ' + s.keydownListeners().length); +}); + +test('a second render inside the same visit does not add a second listener', async () => { + const s = loadNodes(); + s.page.init(s.app, null); + await settle(); + s.regionChange(); // a region change re-runs loadNodes() -> renderLeft() + await settle(); + assert.deepStrictEqual(s.errors, [], 'both renders ran without errors'); + const n = s.keydownListeners().length; + assert.strictEqual(n, 1, 'still one listener after two renders, got ' + n); +}); + +test('list -> node -> list -> node -> list does not stack listeners', async () => { + const s = loadNodes(); + for (const pk of [null, 'aa', null, 'bb', null]) { + s.page.destroy(); // the router destroys the current page first + s.page.init(s.app, pk); + await settle(); + } + assert.deepStrictEqual(s.errors, [], 'every render ran without errors'); + const n = s.keydownListeners().length; + assert.strictEqual(n, 1, 'the list view ends with one listener, got ' + n); +}); + +test('one Escape closes the list view\'s detail panel exactly once', async () => { + const s = loadNodes(); + for (const pk of [null, 'aa', null]) { + s.page.destroy(); + s.page.init(s.app, pk); + await settle(); + } + // nodesPanelEsc only acts while the right panel is not .empty, so open it. + s.panel.classList.remove('empty'); + s.replaceStateWrites.length = 0; + s.pressEscape(); + assert.deepStrictEqual(s.replaceStateWrites, ['#/nodes'], + 'one panel close, got ' + JSON.stringify(s.replaceStateWrites)); +}); + +test('destroy() removes the list view\'s Escape listener', async () => { const s = loadNodes(); s.page.init(s.app, null); - assert.strictEqual(s.keydownListeners().length, 0, 'the list view has no Escape handler'); + await settle(); + s.page.destroy(); + assert.strictEqual(s.keydownListeners().length, 0, + 'got ' + s.keydownListeners().length); + s.panel.classList.remove('empty'); + s.replaceStateWrites.length = 0; + s.pressEscape(); + assert.deepStrictEqual(s.replaceStateWrites, [], + 'a destroyed list view does not close anything: ' + JSON.stringify(s.replaceStateWrites)); }); -console.log(`\n${passed} passed, ${failed} failed`); -process.exit(failed === 0 ? 0 : 1); +runQueue().then(() => { + console.log(`\n${passed} passed, ${failed} failed`); + process.exit(failed === 0 ? 0 : 1); +}); From f6abeef1e330b134a41d797cfd5a1572c78158d9 Mon Sep 17 00:00:00 2001 From: dborup Date: Mon, 5 Oct 2026 21:25:44 +0200 Subject: [PATCH 7/7] fix(nodes): one list-view Escape listener, not one per render (#259) Review F3 on #260: nodesPanelEsc had the same leak class as the item-2 fix for _nodesEsc. It was a fresh named closure registered inside renderLeft(), and renderLeft() runs on every load of the list -- a visit, a region change, a filter change -- so the listeners stacked on document and nothing ever removed them. Same shape as the item-2 fix: _nodesPanelEsc is a module-level reference, so a repeat addEventListener is a DOM no-op, and destroy() takes it off alongside _nodesEsc. Behaviour is unchanged -- it still only acts while #nodesRight is not .empty. Pre-existing, but small and in the same file as item 2. --- public/nodes.js | 41 +++++++++++++++++++++++++---------------- 1 file changed, 25 insertions(+), 16 deletions(-) diff --git a/public/nodes.js b/public/nodes.js index 78b13223f..54c2c8523 100644 --- a/public/nodes.js +++ b/public/nodes.js @@ -591,6 +591,23 @@ } } + // #259: the list view's Escape-to-close-the-detail-panel handler, same class + // of leak. It used to be a fresh closure registered inside renderLeft(), and + // renderLeft() runs on every load of the list -- a visit, a region change, a + // filter change -- so the listeners stacked and were never removed. Module + // level means a repeat add is a DOM no-op and destroy() can take it off. + function _nodesPanelEsc(e) { + if (e.key !== 'Escape') return; + const panel = document.getElementById('nodesRight'); + if (!panel || panel.classList.contains('empty')) return; + closeDetailView(); + panel.classList.add('empty'); + panel.innerHTML = 'Select a node to view details'; + selectedKey = null; + history.replaceState(null, '', '#/nodes'); + renderRows(); + } + let regionChangeHandler = null; function init(app, routeParam) { @@ -1258,9 +1275,11 @@ function destroy() { if (wsHandler) offWS(wsHandler); wsHandler = null; - // #259: the full-screen view's Escape handler is document-level, so it - // outlives the page's DOM unless it is removed here. + // #259: both Escape handlers are document-level, so they outlive the + // page's DOM unless they are removed here -- _nodesEsc for the + // full-screen view, _nodesPanelEsc for the list view. document.removeEventListener('keydown', _nodesEsc); + document.removeEventListener('keydown', _nodesPanelEsc); closeDetailView(); if (regionChangeHandler) RegionFilter.offChange(regionChangeHandler); regionChangeHandler = null; @@ -1675,20 +1694,10 @@ tbody.addEventListener('keydown', handler); } - // Escape to close node detail panel - document.addEventListener('keydown', function nodesPanelEsc(e) { - if (e.key === 'Escape') { - const panel = document.getElementById('nodesRight'); - if (panel && !panel.classList.contains('empty')) { - closeDetailView(); - panel.classList.add('empty'); - panel.innerHTML = 'Select a node to view details'; - selectedKey = null; - history.replaceState(null, '', '#/nodes'); - renderRows(); - } - } - }); + // Escape to close node detail panel. #259: one listener, not one per + // render -- _nodesPanelEsc is a stable module-level reference, so this add + // is a no-op on every render after the first, and destroy() removes it. + document.addEventListener('keydown', _nodesPanelEsc); // #630: Close button for node detail panel (important for mobile full-screen overlay) document.getElementById('nodesRight').addEventListener('click', function(e) {