From 2d3f92de6e62d747e76831a462e5ee74385a4a03 Mon Sep 17 00:00:00 2001 From: dborup Date: Fri, 2 Oct 2026 15:11:29 +0200 Subject: [PATCH 1/4] test(packets): reproduce ?obs=/?viewPath= loss on detail URLs (#147) Red on master 727efca0: - test-issue-147-packets-url-detail-params.js (24 of 29 fail): runs the real buildPacketsQuery/updatePacketsUrl/Clear handler in vm. Cold load, every filter type and Clear Filters drop ?obs= (and ?viewPath= while the View Path modal is open); selectPacket, the observation-row click and the detail onClose write the hash themselves and drop the filter params. buildPacketsQuery() golden output (= master) and the "list view gets no obs" / "viewPath never invented" guards pass on master by design. - test-packet-path-map.js: closing the modal must drop ?viewPath=1 from a #/packets/ URL (1 new red, 2 guards green). - test-issue-147-packets-url-obs-e2e.js (7 of 7 fail): desktop + mobile Playwright; also shows that cold-load ?obs= never selects the observation (init() parses it from routeParam, which the router has already stripped of its query). test-clear-filters.js: its body-only extract of updatePacketsUrl() gets an unused `detail` parameter name so it keeps working once the function takes one; no assertion changed. New tests registered in test-all.sh and in the deploy.yml unit/E2E steps (test-packet-path-map.js was not run in CI yet). Relates to #147 Co-Authored-By: Claude Opus 5.5 --- .github/workflows/deploy.yml | 3 + test-all.sh | 1 + test-clear-filters.js | 8 +- test-issue-147-packets-url-detail-params.js | 312 ++++++++++++++++++++ test-issue-147-packets-url-obs-e2e.js | 153 ++++++++++ test-packet-path-map.js | 32 +- 6 files changed, 505 insertions(+), 4 deletions(-) create mode 100644 test-issue-147-packets-url-detail-params.js create mode 100644 test-issue-147-packets-url-obs-e2e.js diff --git a/.github/workflows/deploy.yml b/.github/workflows/deploy.yml index d3262947b..e9fd7554d 100644 --- a/.github/workflows/deploy.yml +++ b/.github/workflows/deploy.yml @@ -140,6 +140,8 @@ jobs: node test-packet-filter-time.js node test-issue-121-clear-filters-selection.js node test-clear-filters.js + node test-issue-147-packets-url-detail-params.js + node test-packet-path-map.js node test-confidence-indicator.js node test-1659-analytics-warmup.js node test-issue-120-distance-building.js @@ -503,6 +505,7 @@ jobs: BASE_URL=http://localhost:13581 node test-charts-fluid-1058-e2e.js 2>&1 | tee -a e2e-output.txt BASE_URL=http://localhost:13581 node test-slideover-1056-e2e.js 2>&1 | tee -a e2e-output.txt BASE_URL=http://localhost:13581 node test-issue-1692-packets-init-parallel-e2e.js 2>&1 | tee -a e2e-output.txt + BASE_URL=http://localhost:13581 node test-issue-147-packets-url-obs-e2e.js 2>&1 | tee -a e2e-output.txt BASE_URL=http://localhost:13581 node test-slideover-1168-munger-e2e.js 2>&1 | tee -a e2e-output.txt BASE_URL=http://localhost:13581 node test-logo-pulse-1173-e2e.js 2>&1 | tee -a e2e-output.txt BASE_URL=http://localhost:13581 node test-issue-1122-packets-filter-ux-e2e.js 2>&1 | tee -a e2e-output.txt diff --git a/test-all.sh b/test-all.sh index a46a53639..c537b218a 100755 --- a/test-all.sh +++ b/test-all.sh @@ -13,6 +13,7 @@ node test-packet-filter.js node test-packet-filter-ux.js node test-issue-121-clear-filters-selection.js node test-clear-filters.js +node test-issue-147-packets-url-detail-params.js node test-aging.js node test-issue-1065-gesture-hints-gates.js node test-frontend-helpers.js diff --git a/test-clear-filters.js b/test-clear-filters.js index 23be1a26d..6ca07eac5 100644 --- a/test-clear-filters.js +++ b/test-clear-filters.js @@ -143,7 +143,9 @@ function extractFunctionSource(src, marker) { } function extractUpdatePacketsUrl() { const src = fs.readFileSync(__dirname + '/public/packets.js', 'utf-8'); - const body = extractFunctionSource(src, 'function updatePacketsUrl()'); + // #147: updatePacketsUrl(detail) takes an optional detail argument; the + // tests below leave it undefined (the plain filter-change call). + const body = extractFunctionSource(src, 'function updatePacketsUrl('); // updatePacketsUrl() delegates button visibility to updateClearFiltersVisibility() const vis = extractFunctionSource(src, 'function updateClearFiltersVisibility()'); return vis + '\n' + body.substring(body.indexOf('{') + 1, body.length - 1); @@ -288,7 +290,7 @@ test('updatePacketsUrl shows clear button when time window != default', () => { const DEFAULT_TIME_WINDOW = 15; const fn = new Function( 'filters', 'savedTimeWindowMin', 'DEFAULT_TIME_WINDOW', - 'document', 'history', 'RegionFilter', 'buildPacketsQuery', 'location', + 'document', 'history', 'RegionFilter', 'buildPacketsQuery', 'location', 'detail', updateUrlBody ); fn(filters, savedTimeWindowMin, DEFAULT_TIME_WINDOW, @@ -302,7 +304,7 @@ test('updatePacketsUrl hides clear button when all filters default', () => { const filters = {}; const fn = new Function( 'filters', 'savedTimeWindowMin', 'DEFAULT_TIME_WINDOW', - 'document', 'history', 'RegionFilter', 'buildPacketsQuery', 'location', + 'document', 'history', 'RegionFilter', 'buildPacketsQuery', 'location', 'detail', updateUrlBody ); fn(filters, 15, 15, ctx.document, ctx.history, ctx.RegionFilter, () => '', ctx.location); diff --git a/test-issue-147-packets-url-detail-params.js b/test-issue-147-packets-url-detail-params.js new file mode 100644 index 000000000..c9e1765ba --- /dev/null +++ b/test-issue-147-packets-url-detail-params.js @@ -0,0 +1,312 @@ +/* test-issue-147-packets-url-detail-params.js — #147 + * + * updatePacketsUrl() rebuilds #/packets/?… from buildPacketsQuery(), + * which only knows filter params. The packet-detail params it does not own, + * ?obs= (selected observation) and ?viewPath=1 (View Path modal), must + * survive: on cold load (init() calls it to show the Clear button), on every + * filter change and on Clear Filters. + * + * Decision under test: ?obs= travels with the detail subpath; ?viewPath=1 + * describes the open View Path modal, so it is kept only while that modal + * (#packetPathModal) is open. (Closing the modal drops it from the address + * bar: see test-packet-path-map.js.) + * + * Runs the REAL buildPacketsQuery / updateClearFiltersVisibility / + * updatePacketsUrl and the real Clear Filters handler body from + * public/packets.js (plus the real public/url-state.js for sort) inside a + * vm.createContext sandbox, so the closure state they share (filters, + * savedTimeWindowMin, sort) is a sandbox global the tests can set. + */ +'use strict'; + +const vm = require('vm'); +const fs = require('fs'); +const path = require('path'); +const assert = require('assert'); + +console.log('--- test-issue-147-packets-url-detail-params.js ---'); + +let passed = 0, failed = 0; +function test(name, fn) { + try { fn(); passed++; console.log(` ✅ ${name}`); } + catch (e) { failed++; console.log(` ❌ ${name}: ${e.message}`); } +} + +const PACKETS_SRC = fs.readFileSync(path.join(__dirname, 'public/packets.js'), 'utf8'); +const URLSTATE_SRC = fs.readFileSync(path.join(__dirname, 'public/url-state.js'), 'utf8'); + +// Source of the block that opens at the first '{' after `marker`, through its +// matching '}'. With keepHead the text from marker to that '{' is included. +function extractBlock(src, marker, keepHead) { + const idx = src.indexOf(marker); + assert(idx !== -1, marker + ' not found in public/packets.js'); + const open = src.indexOf('{', idx + (keepHead ? 0 : marker.length)); + let depth = 0; + for (let i = open; i < src.length; i++) { + if (src[i] === '{') depth++; + else if (src[i] === '}' && --depth === 0) return keepHead ? src.substring(idx, i + 1) : src.substring(open + 1, i); + } + throw new Error('unbalanced braces after ' + marker); +} + +const URL_FUNCS = [ + 'function buildPacketsQuery(', + 'function updateClearFiltersVisibility()', + 'function updatePacketsUrl(', +].map((m) => extractBlock(PACKETS_SRC, m, true)).join('\n'); +const CLEAR_BODY = extractBlock(PACKETS_SRC, "if (clearBtn) clearBtn.addEventListener('click', function()", false); + +// opts.modalOpen: a #packetPathModal overlay is in the DOM. +function makeSandbox(startHash, opts) { + opts = opts || {}; + const urls = []; + const els = {}; + for (const id of ['clearFiltersBtn', 'fHash', 'fNode', 'fChannel', 'fTimeWindow', 'fMyNodes', + 'packetFilterInput', 'packetFilterError', 'packetFilterCount']) { + els[id] = { id, value: '', style: { display: 'none' }, classList: { add() {}, remove() {} } }; + } + if (opts.modalOpen) els.packetPathModal = { id: 'packetPathModal' }; + const store = {}; + const ctx = { + console, URLSearchParams, encodeURIComponent, decodeURIComponent, + String, Number, Array, Object, RegExp, JSON, Math, Set, Map, Error, + location: { hash: startHash }, + history: { replaceState(_s, _t, url) { urls.push(url); ctx.location.hash = url; } }, + document: { getElementById: (id) => els[id] || null }, + localStorage: { + getItem: (k) => (k in store ? store[k] : null), + setItem: (k, v) => { store[k] = String(v); }, + removeItem: (k) => { delete store[k]; }, + }, + __region: [], + window: {}, + }; + ctx.RegionFilter = { + getRegionParam: () => ctx.__region.join(','), + setSelected: (a) => { ctx.__region = a.slice(); }, + }; + ctx.globalThis = ctx; + vm.createContext(ctx); + vm.runInContext(URLSTATE_SRC, ctx, { filename: 'public/url-state.js' }); + ctx.URLState = ctx.window.URLState; + vm.runInContext( + 'var filters = {}; var savedTimeWindowMin = 15; var DEFAULT_TIME_WINDOW = 15;\n' + + "var _packetSortColumn = null; var _packetSortDirection = 'desc';\n" + + 'var _observerFilterSet = null; var selectedObservers = new Set(); var selectedTypes = new Set();\n' + + 'function buildObserverMenu() {} function updateObsTrigger() {} function buildTypeMenu() {}\n' + + 'function updateTypeTrigger() {} function loadPackets() {}\n' + + URL_FUNCS + '\nfunction __clearFilters() {' + CLEAR_BODY + '}', + ctx, { filename: 'packets.js (#147 extract)' }); + return { + ctx, urls, els, + set(code) { vm.runInContext(code, ctx); }, + update(detail) { + ctx.__detail = detail; + vm.runInContext(detail === undefined ? 'updatePacketsUrl()' : 'updatePacketsUrl(__detail)', ctx); + return ctx.location.hash; + }, + clear() { vm.runInContext('__clearFilters()', ctx); return ctx.location.hash; }, + query(tw, region, skipHash) { return ctx.buildPacketsQuery(tw, region, skipHash); }, + }; +} + +function params(hash) { return new URLSearchParams(String(hash).split('?')[1] || ''); } +function countParam(hash, name) { return params(hash).getAll(name).length; } + +const DETAIL = '#/packets/abc123'; + +// ---- Cold load: init() calls updatePacketsUrl() once (packets.js ~1984) ---- + +test('cold load #/packets/?obs=123 keeps ?obs=123', () => { + const s = makeSandbox(DETAIL + '?obs=123'); + s.set("filters.hash = 'abc123'"); // init() sets filters.hash from the route + assert.strictEqual(s.update(), DETAIL + '?obs=123'); +}); + +test('cold load #/packets/?obs=123&viewPath=1 (modal open) keeps both', () => { + const s = makeSandbox(DETAIL + '?obs=123&viewPath=1', { modalOpen: true }); + s.set("filters.hash = 'abc123'"); + assert.strictEqual(s.update(), DETAIL + '?obs=123&viewPath=1'); +}); + +test('cold load with URL filters keeps filters and ?obs=', () => { + const s = makeSandbox(DETAIL + '?timeWindow=60&observer=OBS1&obs=123'); + s.set("filters.hash = 'abc123'; filters.observer = 'OBS1'; savedTimeWindowMin = 60"); + assert.strictEqual(s.update(), DETAIL + '?timeWindow=60&observer=OBS1&obs=123'); +}); + +// ---- viewPath decision: kept only while its modal is open ---- + +test('viewPath: kept on a filter change while the View Path modal is open', () => { + const s = makeSandbox(DETAIL + '?obs=123&viewPath=1', { modalOpen: true }); + s.set("filters.hash = 'abc123'; filters.observer = 'OBS1'"); + const h = s.update(); + assert.strictEqual(params(h).get('observer'), 'OBS1'); + assert.strictEqual(params(h).get('obs'), '123'); + assert.strictEqual(params(h).get('viewPath'), '1', 'viewPath dropped although the modal is open: ' + h); +}); + +test('viewPath: dropped once the modal is closed (URL describes what is shown), obs kept', () => { + const s = makeSandbox(DETAIL + '?obs=123&viewPath=1', { modalOpen: false }); + s.set("filters.hash = 'abc123'; filters.observer = 'OBS1'"); + assert.strictEqual(s.update(), DETAIL + '?observer=OBS1&obs=123'); +}); + +test('viewPath: never invented when the URL did not have it', () => { + const s = makeSandbox(DETAIL + '?obs=123', { modalOpen: true }); + s.set("filters.hash = 'abc123'"); + assert.strictEqual(countParam(s.update(), 'viewPath'), 0); +}); + +// ---- Every filter type keeps ?obs= (and updates its own param) ---- + +const FILTER_CASES = [ + ['observer', "filters.observer = 'OBS1'", 'observer', 'OBS1'], + ['region', "__region = ['EU']", 'region', 'EU'], + ['hash (text filter, not the subpath)', "filters.hash = 'ffee01'", 'hash', 'ffee01'], + ['node', "filters.node = 'nodepubkey0123456789'", 'node', 'nodepubkey0123456789'], + ['channel', "filters.channel = 'public'", 'channel', 'public'], + ['filter expression', "filters._filterExpr = 'type == ADVERT && snr > 5'", 'filter', 'type == ADVERT && snr > 5'], + ['time window', 'savedTimeWindowMin = 60', 'timeWindow', '60'], + ['sort', "_packetSortColumn = 'snr'; _packetSortDirection = 'asc'", 'sort', 'snr:asc'], +]; +for (const [label, code, name, value] of FILTER_CASES) { + test(`filter change keeps ?obs=: ${label}`, () => { + const s = makeSandbox(DETAIL + '?obs=123'); + s.set(code); + const h = s.update(); + assert(h.startsWith(DETAIL + '?'), 'detail subpath lost: ' + h); + assert.strictEqual(params(h).get(name), value, `${name} not written: ${h}`); + assert.strictEqual(params(h).get('obs'), '123', '?obs= dropped: ' + h); + assert.strictEqual(countParam(h, 'obs'), 1, '?obs= duplicated: ' + h); + }); +} + +test('filter change keeps ?obs=: area (not a URL param; URL otherwise unchanged)', () => { + // AreaFilter.onChange -> updatePacketsUrl(); the area itself is not in the hash. + const s = makeSandbox(DETAIL + '?obs=123'); + assert.strictEqual(s.update(), DETAIL + '?obs=123'); +}); + +test('removing a filter keeps ?obs=', () => { + const s = makeSandbox(DETAIL + '?observer=OBS1&obs=123'); + s.set("filters.observer = 'OBS1'"); + s.update(); + s.set('filters.observer = undefined'); + assert.strictEqual(s.update(), DETAIL + '?obs=123'); +}); + +test('repeated updates are idempotent (obs stays once, last)', () => { + const s = makeSandbox(DETAIL + '?obs=123'); + s.set("filters.observer = 'OBS1'; savedTimeWindowMin = 60"); + const a = s.update(), b = s.update(), c = s.update(); + assert.strictEqual(a, DETAIL + '?timeWindow=60&observer=OBS1&obs=123'); + assert.strictEqual(b, a); + assert.strictEqual(c, a); +}); + +test('list view (#/packets, no detail) gets no ?obs=', () => { + const s = makeSandbox('#/packets?obs=123'); + s.set("filters.observer = 'OBS1'"); + assert.strictEqual(s.update(), '#/packets?observer=OBS1'); +}); + +// ---- Clear Filters (#121/#132): filter params go, detail subpath + obs stay ---- + +test('Clear Filters on a detail URL removes filter params, keeps subpath and ?obs=', () => { + const s = makeSandbox(DETAIL + '?timeWindow=60®ion=EU&observer=OBS1&obs=123'); + s.set("filters.hash = 'abc123'; filters.observer = 'OBS1'; savedTimeWindowMin = 60; __region = ['EU']"); + assert.strictEqual(s.clear(), DETAIL + '?obs=123'); + assert.strictEqual(s.els.clearFiltersBtn.style.display, 'none', 'Clear button still visible after clearing'); +}); + +test('Clear Filters on a detail URL drops viewPath=1 when the modal is closed', () => { + const s = makeSandbox(DETAIL + '?observer=OBS1&obs=123&viewPath=1', { modalOpen: false }); + s.set("filters.observer = 'OBS1'"); + assert.strictEqual(s.clear(), DETAIL + '?obs=123'); +}); + +test('Clear Filters on the list view still yields bare #/packets', () => { + const s = makeSandbox('#/packets?timeWindow=60&observer=OBS1'); + s.set("filters.observer = 'OBS1'; savedTimeWindowMin = 60"); + assert.strictEqual(s.clear(), '#/packets'); +}); + +// ---- buildPacketsQuery() stays filter-only, output unchanged ---- + +test('buildPacketsQuery() output is unchanged and never contains detail params', () => { + // Golden values = master 727efca0 output. The hash carries ?obs=&viewPath= + // and the modal is open, so a buildPacketsQuery() that reads them would differ. + const s = makeSandbox(DETAIL + '?obs=123&viewPath=1', { modalOpen: true }); + const q = (setup, tw, region, skipHash) => { + s.set('filters = {}; _packetSortColumn = null; _packetSortDirection = "desc";' + setup); + return s.query(tw, region, skipHash); + }; + assert.strictEqual(q('', 15, ''), ''); + assert.strictEqual(q('', 60, ''), '?timeWindow=60'); + assert.strictEqual(q('', 15, 'EU,US-W'), '?region=EU%2CUS-W'); + assert.strictEqual(q("filters.hash = 'abc123'", 15, ''), '?hash=abc123'); + assert.strictEqual(q("filters.hash = 'abc123'", 15, '', true), ''); + assert.strictEqual( + q("filters.hash = 'h'; filters.node = 'n'; filters.observer = 'o'; filters.channel = 'c'; filters._filterExpr = 'a b'; _packetSortColumn = 'snr'", 30, 'EU'), + '?timeWindow=30®ion=EU&hash=h&node=n&observer=o&channel=c&filter=a%20b&sort=snr'); + assert.strictEqual(q("_packetSortColumn = 'time'", 15, ''), ''); + assert.strictEqual(q("_packetSortColumn = 'time'; _packetSortDirection = 'asc'", 15, ''), '?sort=time%3Aasc'); +}); + +// ---- Detail writers: selecting a packet / observation, closing the detail ---- +// selectPacket() (also the cold-load auto-select), the observation-row click +// and the detail pane's onClose wrote #/packets/... themselves, dropping the +// filter params (and viewPath on cold load). They pass the new detail state +// to updatePacketsUrl({ subpath, obs }) instead. + +test('selecting a packet keeps the filter params and writes its ?obs=', () => { + const s = makeSandbox('#/packets?timeWindow=60&observer=OBS1'); + s.set("filters.observer = 'OBS1'; savedTimeWindowMin = 60"); + assert.strictEqual(s.update({ subpath: '/abc123', obs: '7' }), DETAIL + '?timeWindow=60&observer=OBS1&obs=7'); +}); + +test('cold-load auto-select of the same packet keeps ?viewPath=1 while the modal is open', () => { + const s = makeSandbox(DETAIL + '?obs=123&viewPath=1', { modalOpen: true }); + s.set("filters.hash = 'abc123'"); + assert.strictEqual(s.update({ subpath: '/abc123', obs: '123' }), DETAIL + '?obs=123&viewPath=1'); +}); + +test('selecting another packet carries neither the old ?obs= nor ?viewPath=1', () => { + const s = makeSandbox(DETAIL + '?obs=123&viewPath=1', { modalOpen: true }); + assert.strictEqual(s.update({ subpath: '/ffee01', obs: null }), '#/packets/ffee01'); +}); + +test('clicking another observation row switches ?obs= and keeps the filters', () => { + const s = makeSandbox(DETAIL + '?timeWindow=60&obs=1'); + s.set('savedTimeWindowMin = 60'); + assert.strictEqual(s.update({ subpath: '/abc123', obs: '2' }), DETAIL + '?timeWindow=60&obs=2'); +}); + +test('closing the detail drops subpath and detail params, keeps the filters', () => { + const s = makeSandbox(DETAIL + '?timeWindow=60&observer=OBS1&obs=1'); + s.set("filters.observer = 'OBS1'; savedTimeWindowMin = 60"); + assert.strictEqual(s.update({ subpath: '', obs: null }), '#/packets?timeWindow=60&observer=OBS1'); +}); + +// ---- One place: updatePacketsUrl() is the only #/packets hash writer ---- + +test('updatePacketsUrl() is the only history.replaceState() writer in packets.js', () => { + const writes = PACKETS_SRC.match(/history\.replaceState\(/g) || []; + assert.strictEqual(writes.length, 1, 'expected 1 replaceState call (in updatePacketsUrl), got ' + writes.length); + const fn = extractBlock(PACKETS_SRC, 'function updatePacketsUrl(', true); + assert(/history\.replaceState\(/.test(fn), 'the writer is not inside updatePacketsUrl()'); +}); + +test('buildPacketsQuery() is called only from updatePacketsUrl() (every caller covered)', () => { + const calls = PACKETS_SRC.match(/buildPacketsQuery\(/g) || []; + assert.strictEqual(calls.length, 2, 'expected the definition + one call, got ' + calls.length); + const fn = extractBlock(PACKETS_SRC, 'function updatePacketsUrl(', true); + assert(/buildPacketsQuery\(/.test(fn), 'updatePacketsUrl() no longer builds the query'); + assert(/updatePacketsUrl\(\);\s*\n\s*\n\s*\/\/ Filter event listeners/.test(PACKETS_SRC), + 'init() cold-load updatePacketsUrl() call moved; re-check #147 cold-load coverage'); +}); + +console.log(`\n${passed} passed, ${failed} failed`); +if (failed > 0) process.exit(1); +console.log('All tests passed ✅'); diff --git a/test-issue-147-packets-url-obs-e2e.js b/test-issue-147-packets-url-obs-e2e.js new file mode 100644 index 000000000..b4bb4f833 --- /dev/null +++ b/test-issue-147-packets-url-obs-e2e.js @@ -0,0 +1,153 @@ +/** + * E2E (#147): a packet-detail deep link #/packets/?obs= keeps its + * selected observation in the address bar on cold load, on filter changes + * and on Clear Filters, and reopening the copied URL in a new tab selects the + * same observation. ?viewPath=1 is kept while the View Path modal is open and + * dropped when it closes. + * + * The e2e fixture has no packet with two observations reachable through the + * API, so the packet-detail response is stubbed (context.route, so new tabs + * see it too) with one extra synthetic observation. That makes the chosen + * ?obs= distinguishable from the default (first) observation. + * + * Usage: BASE_URL=http://localhost:13581 node test-issue-147-packets-url-obs-e2e.js + */ +'use strict'; +const { chromium } = require('playwright'); + +const BASE = process.env.BASE_URL || 'http://localhost:13581'; +const SYN_OBS = '990000147'; + +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'); } +function hashParams(page) { + return page.evaluate(() => { + const h = location.hash; + return { hash: h, q: Object.fromEntries(new URLSearchParams(h.split('?')[1] || '')) }; + }); +} +async function waitHash(page, pred, what) { + try { + await page.waitForFunction(pred, null, { timeout: 8000 }); + } catch (_) { + throw new Error(what + ' — address bar: ' + (await page.evaluate(() => location.hash))); + } +} +async function currentObs(page) { + await page.waitForSelector(`.detail-obs-row[data-obs-id="${SYN_OBS}"]`, { timeout: 15000 }); + // Let init()'s auto-select finish before reading the selection. + await page.waitForTimeout(500); + return page.evaluate(() => { + const r = document.querySelector('.detail-obs-row.observation-current'); + return r ? r.dataset.obsId : null; + }); +} + +async function stubDetail(context, hash) { + await context.route((url) => url.pathname === '/api/packets/' + hash, async (route) => { + const upstream = await context.request.fetch(route.request()); + const data = await upstream.json(); + const base = (data.observations && data.observations[0]) || {}; + const extra = Object.assign({}, base, { id: Number(SYN_OBS), observer_name: 'e2e-147-observer', snr: -3.5, rssi: -111 }); + data.observations = (data.observations || []).concat([extra]); + await route.fulfill({ status: 200, contentType: 'application/json', body: JSON.stringify(data) }); + }); +} + +async function setTimeWindow(page, value) { + // Works on desktop and on the mobile layout (where the bar can be collapsed). + await page.evaluate((v) => { + const el = document.getElementById('fTimeWindow'); + el.value = v; + el.dispatchEvent(new Event('change', { bubbles: true })); + }, value); +} + +(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=== #147 packets detail URL keeps ?obs= / ?viewPath= against ${BASE} ===`); + + const list = await (await fetch(BASE + '/api/packets?limit=50&groupByHash=true')).json(); + const pkt = (list.packets || []).find((p) => p.hash && /^[0-9a-f]+$/.test(p.hash)); + if (!pkt) { console.error('no packet with a hash in the fixture'); process.exit(1); } + const hash = pkt.hash; + const DETAIL = `${BASE}/#/packets/${hash}`; + console.log(' packet ' + hash + ', synthetic observation ' + SYN_OBS); + + for (const vp of [ + { label: 'desktop', opts: { viewport: { width: 1400, height: 900 } } }, + { label: 'mobile', opts: { viewport: { width: 390, height: 844 }, isMobile: true, hasTouch: true } }, + ]) { + const context = await browser.newContext(vp.opts); + await context.addInitScript(() => { + try { localStorage.removeItem('meshcore-time-window'); localStorage.removeItem('meshcore-observer-filter'); } catch (_) {} + }); + await stubDetail(context, hash); + const page = await context.newPage(); + page.setDefaultTimeout(15000); + page.on('pageerror', (e) => console.error('[pageerror]', e.message)); + + await step(`${vp.label}: cold load ?obs= selects that observation and keeps ?obs=`, async () => { + await page.goto(`${DETAIL}?obs=${SYN_OBS}`, { waitUntil: 'load' }); + const cur = await currentObs(page); + assert(cur === SYN_OBS, 'selected observation is ' + cur + ', expected ' + SYN_OBS); + const { q, hash: h } = await hashParams(page); + assert(q.obs === SYN_OBS, 'obs missing after cold load: ' + h); + }); + + await step(`${vp.label}: changing the time window keeps ?obs= and adds ?timeWindow=`, async () => { + await setTimeWindow(page, '60'); + await waitHash(page, () => /[?&]timeWindow=60(&|$)/.test(location.hash), 'timeWindow=60 not written'); + const { q, hash: h } = await hashParams(page); + assert(q.obs === SYN_OBS, 'obs dropped by the filter change: ' + h); + }); + + if (vp.label === 'desktop') { + await step('desktop: copied URL opened in a new tab keeps observation and filter', async () => { + const url = page.url(); + const tab = await context.newPage(); + await tab.goto(url, { waitUntil: 'load' }); + const cur = await currentObs(tab); + assert(cur === SYN_OBS, 'new tab selected ' + cur + ', expected ' + SYN_OBS + ' (url ' + url + ')'); + const tw = await tab.evaluate(() => document.getElementById('fTimeWindow').value); + assert(tw === '60', 'new tab time window ' + tw); + const { q, hash: h } = await hashParams(tab); + assert(q.obs === SYN_OBS && q.timeWindow === '60', 'new tab URL ' + h); + await tab.close(); + }); + + await step('desktop: Clear Filters removes ?timeWindow=, keeps the detail and ?obs=', async () => { + await page.click('#clearFiltersBtn'); + await waitHash(page, () => !/timeWindow=/.test(location.hash), 'timeWindow still in URL after Clear'); + const { q, hash: h } = await hashParams(page); + assert(h.startsWith('#/packets/' + hash), 'detail subpath lost: ' + h); + assert(q.obs === SYN_OBS, 'obs dropped by Clear Filters: ' + h); + }); + + await step('desktop: ?viewPath=1 is kept while the modal is open and dropped when it closes', async () => { + await page.goto(`${DETAIL}?obs=${SYN_OBS}&viewPath=1`, { waitUntil: 'load' }); + await page.waitForSelector('#packetPathModal'); + await currentObs(page); + let p = await hashParams(page); + assert(p.q.viewPath === '1' && p.q.obs === SYN_OBS, 'cold load with modal open: ' + p.hash); + await page.keyboard.press('Escape'); + await waitHash(page, () => !/viewPath=/.test(location.hash), 'viewPath still in URL after closing the modal'); + p = await hashParams(page); + assert(p.q.obs === SYN_OBS, 'obs dropped when the modal closed: ' + p.hash); + }); + } + await context.close(); + } + + await browser.close(); + console.log(`\n${passed} passed, ${failed} failed`); + process.exit(failed ? 1 : 0); +})().catch((e) => { console.error(e); process.exit(1); }); diff --git a/test-packet-path-map.js b/test-packet-path-map.js index 7920e9cb6..06dd06940 100644 --- a/test-packet-path-map.js +++ b/test-packet-path-map.js @@ -142,7 +142,10 @@ function makeSandbox(apiImpl) { escapeHtml: (s) => String(s == null ? '' : s).replace(/&/g, '&').replace(//g, '>'), api: apiImpl, L: undefined, // Leaflet deliberately absent -- these tests only cover the no-plot-data / no-Leaflet paths. - location: { origin: 'https://stg.meshview.dk' }, + location: { origin: 'https://stg.meshview.dk', hash: '' }, + // Records replaceState() so the #147 tests can see the address bar. + history: { replaceState(_s, _t, url) { ctx.__replaced.push(url); ctx.location.hash = url; } }, + __replaced: [], }; ctx.window.copyToClipboard = (text, onDone) => { ctx.__copiedText = text; if (onDone) onDone(); }; vm.createContext(ctx); @@ -1195,6 +1198,33 @@ function makeSandbox(apiImpl) { } catch (e) { failed++; console.log(' ❌ shades each touched area on the map: polygon when drawn, rectangle fallback for bbox-only areas, both non-interactive: ' + e.message); } })(); + // #147: ?viewPath=1 on a #/packets/ URL describes this open modal. + // Closing it drops that one param (others verbatim) so a refresh or a + // copied address-bar link does not reopen a modal that is no longer shown. + async function viewPathCase(name, startHash, act, expectHash, expectWrites) { + try { + const ctx = makeSandbox(() => Promise.reject(new Error('boom'))); + ctx.location.hash = startHash; + await act(ctx); + assert.strictEqual(ctx.location.hash, expectHash); + assert.strictEqual(ctx.__replaced.length, expectWrites, 'replaceState calls: ' + JSON.stringify(ctx.__replaced)); + passed++; + console.log(' ✅ ' + name); + } catch (e) { failed++; console.log(' ❌ ' + name + ': ' + e.message); } + } + await viewPathCase('close() drops ?viewPath=1 from a #/packets/ URL, keeping the other params (#147)', + '#/packets/deadbeef?timeWindow=60&obs=123&viewPath=1', + async (ctx) => { await ctx.window.PacketPathMap.open('deadbeef'); ctx.window.PacketPathMap.close(); }, + '#/packets/deadbeef?timeWindow=60&obs=123', 1); + await viewPathCase('open() on a cold-loaded ?viewPath=1 link keeps it while the modal is shown (#147)', + '#/packets/deadbeef?obs=123&viewPath=1', + async (ctx) => { await ctx.window.PacketPathMap.open('deadbeef'); }, + '#/packets/deadbeef?obs=123&viewPath=1', 0); + await viewPathCase('close() leaves non-packets pages (e.g. analytics distance) untouched (#147)', + '#/analytics?tab=distance&viewPath=1', + async (ctx) => { await ctx.window.PacketPathMap.open('deadbeef'); ctx.window.PacketPathMap.close(); }, + '#/analytics?tab=distance&viewPath=1', 0); + console.log('\n════════════════════════════════════════'); console.log(` packet-path-map.js: ${passed} passed, ${failed} failed`); console.log('════════════════════════════════════════'); From d0a9f28ea6947987bd7628ed6e559ec48cd6b3bf Mon Sep 17 00:00:00 2001 From: dborup Date: Fri, 2 Oct 2026 15:24:06 +0200 Subject: [PATCH 2/4] fix(packets): keep ?obs=/?viewPath= on packet detail URLs (#147) updatePacketsUrl() is now the only writer of #/packets... and adds the packet-detail params that buildPacketsQuery() does not own after the filter params: - ?obs= stays with the detail subpath on cold load (init()), every filter change and Clear Filters. - ?viewPath=1 describes the open View Path modal, so it is kept only while #packetPathModal is open on the same packet; PacketPathMap.close() drops it from a #/packets/ URL (other params verbatim) so a refresh or a copied link does not reopen a closed modal. - selectPacket() (incl. the cold-load auto-select), the observation-row click and the SlideOver onClose pass the new detail state as updatePacketsUrl({ subpath, obs }) instead of writing the hash themselves, so they no longer drop the active filter params (or viewPath on cold load). - init() reads ?obs= from the hash query: the router strips the query from routeParam, so the old routeParam parse never ran and a cold-loaded ?obs= link always showed the first observation. buildPacketsQuery() is unchanged (golden test). Per call the work is a split of the current hash query and one getElementById: O(params), not O(packets), and no API calls. test-issue-147-packets-url-obs-e2e.js: catch a detail refetch still in flight when the context closes and unroute before closing it (a teardown race seen locally, 1 in 6 runs before; 5/5 green after). Relates to #147 Co-Authored-By: Claude Opus 5.5 --- public/packet-path-map.js | 19 ++++++++- public/packets.js | 58 ++++++++++++++++----------- test-issue-147-packets-url-obs-e2e.js | 18 ++++++--- 3 files changed, 64 insertions(+), 31 deletions(-) diff --git a/public/packet-path-map.js b/public/packet-path-map.js index 952ed72c5..e58c08569 100644 --- a/public/packet-path-map.js +++ b/public/packet-path-map.js @@ -80,9 +80,26 @@ if (e.key === 'Escape') close(); } + // A #/packets/?…&viewPath=1 URL describes this modal as open (#147). + // Once it closes, drop that one param (the others stay verbatim) so a + // refresh or a copied address-bar link does not reopen it. + function dropViewPathParam() { + if (typeof history === 'undefined' || !history.replaceState) return; + var h = String(location.hash || ''); + var q = h.indexOf('?'); + if (q < 0 || h.indexOf('#/packets/') !== 0) return; + var params = h.slice(q + 1).split('&'); + var kept = params.filter(function (p) { return p !== 'viewPath=1'; }); + if (kept.length === params.length) return; + history.replaceState(null, '', h.slice(0, q) + (kept.length ? '?' + kept.join('&') : '')); + } + function close() { var overlay = document.getElementById('packetPathModal'); - if (overlay) overlay.remove(); + if (overlay) { + overlay.remove(); + dropViewPathParam(); + } if (activeMap) { try { activeMap.remove(); } catch (e) { /* already gone */ } activeMap = null; diff --git a/public/packets.js b/public/packets.js index aa86bb18e..a9f28b542 100644 --- a/public/packets.js +++ b/public/packets.js @@ -799,15 +799,35 @@ cb.style.display = active ? '' : 'none'; } - function updatePacketsUrl() { - // Preserve any subpath after /packets (e.g. #/packets/). + // The only writer of #/packets... (#147). Filter params come from + // buildPacketsQuery; the packet-detail params it does not own are added + // after them: ?obs= (selected observation) while a detail subpath is open, + // and ?viewPath=1 only while its View Path modal is open on that same + // packet, so the URL keeps describing what is shown. + // detail (optional): { subpath: '/' or '', obs: id or null } when + // the caller changes the selection; without it the current subpath and + // ?obs= are kept (filter changes, Clear Filters, cold load). + function updatePacketsUrl(detail) { var cur = String(location.hash || ''); - var subpath = ''; + var qIdx = cur.indexOf('?'); + var curParams = qIdx < 0 ? [] : cur.slice(qIdx + 1).split('&'); var m = cur.match(/^#\/packets(\/[^?]*)?/); - if (m && m[1]) subpath = m[1]; + var curSubpath = (m && m[1]) || ''; + var subpath = detail ? detail.subpath : curSubpath; // Don't double-encode filters.hash when it's already the path segment. var skipHash = !!(filters.hash && subpath === '/' + filters.hash); - history.replaceState(null, '', '#/packets' + subpath + buildPacketsQuery(savedTimeWindowMin, RegionFilter.getRegionParam(), skipHash)); + var query = buildPacketsQuery(savedTimeWindowMin, RegionFilter.getRegionParam(), skipHash); + var keep = []; + if (subpath) { + if (detail) { + if (detail.obs) keep.push('obs=' + encodeURIComponent(detail.obs)); + } else { + keep = curParams.filter(function (p) { return /^obs=./.test(p); }); + } + if (subpath === curSubpath && curParams.indexOf('viewPath=1') !== -1 && document.getElementById('packetPathModal')) keep.push('viewPath=1'); + } + if (keep.length) query += (query ? '&' : '?') + keep.join('&'); + history.replaceState(null, '', '#/packets' + subpath + query); updateClearFiltersVisibility(); } @@ -1095,13 +1115,6 @@ try { renderTableRows(); } catch (e) { console.warn('[packets] hide-1byte re-render failed', e); } }); } - // Parse ?obs=OBSERVER_ID from routeParam - if (routeParam && routeParam.includes('?')) { - const qIdx = routeParam.indexOf('?'); - const qs = new URLSearchParams(routeParam.substring(qIdx)); - directObsId = qs.get('obs'); - routeParam = routeParam.substring(0, qIdx); - } // Detect route param type: "id/123" for direct packet, short hex for hash, long hex for node if (routeParam) { if (routeParam.startsWith('id/')) { @@ -1125,6 +1138,10 @@ if (_urlRegion) _pendingUrlRegion = _urlRegion; var _urlHash = _initUrlParams.get('hash'); if (_urlHash) filters.hash = _urlHash; + // ?obs= on #/packets/ selects that observation in + // the auto-opened detail below (#147: it used to be parsed from + // routeParam, which never carries the query). + directObsId = _initUrlParams.get('obs'); // Shareable "View Path" link (?viewPath=1) -- packet-path-map.js's // "Copy link" button builds #/packets/?viewPath=1, so a shared // link reopens the exact same modal instead of leaving the recipient @@ -1848,8 +1865,8 @@ if (filters.type) localStorage.setItem('meshcore-type-filter', filters.type); else localStorage.removeItem('meshcore-type-filter'); buildTypeMenu(); updateTypeTrigger(); - // Type is not in the URL, so leave the hash alone (updatePacketsUrl() - // would drop ?obs=/?viewPath=); only refresh the Clear button (#121). + // Type is not in the URL, so leave the hash alone; only refresh the + // Clear button (#121). updateClearFiltersVisibility(); renderTableRows(); }); @@ -3131,12 +3148,7 @@ async function selectPacket(id, hash, prefetchedData, obsRowId) { selectedId = id; selectedObservationId = obsRowId || null; - const obsParam = selectedObservationId ? `?obs=${selectedObservationId}` : ''; - if (hash) { - history.replaceState(null, '', `#/packets/${hash}${obsParam}`); - } else { - history.replaceState(null, '', `#/packets/${id}${obsParam}`); - } + updatePacketsUrl({ subpath: '/' + (hash || id), obs: selectedObservationId }); renderTableRows(); const isMobileNow = window.innerWidth <= 640; // #1168 review note: this branch is intentionally narrower than nodes.js / @@ -3169,7 +3181,7 @@ onClose: function () { selectedId = null; selectedObservationId = null; - history.replaceState(null, '', '#/packets'); + updatePacketsUrl({ subpath: '', obs: null }); renderTableRows(); } }); @@ -3559,9 +3571,7 @@ const obsId = row.dataset.obsId; selectedObservationId = obsId; // Update URL hash to reflect selected observation (deep linking) - const pktHash = pkt.hash || pkt.id; - const obsParam = obsId ? `?obs=${obsId}` : ''; - history.replaceState(null, '', `#/packets/${pktHash}${obsParam}`); + updatePacketsUrl({ subpath: '/' + (pkt.hash || pkt.id), obs: obsId }); renderDetail(panel, data, obsId); }); }); diff --git a/test-issue-147-packets-url-obs-e2e.js b/test-issue-147-packets-url-obs-e2e.js index b4bb4f833..ce9cabe90 100644 --- a/test-issue-147-packets-url-obs-e2e.js +++ b/test-issue-147-packets-url-obs-e2e.js @@ -49,12 +49,17 @@ async function currentObs(page) { async function stubDetail(context, hash) { await context.route((url) => url.pathname === '/api/packets/' + hash, async (route) => { - const upstream = await context.request.fetch(route.request()); - const data = await upstream.json(); - const base = (data.observations && data.observations[0]) || {}; - const extra = Object.assign({}, base, { id: Number(SYN_OBS), observer_name: 'e2e-147-observer', snr: -3.5, rssi: -111 }); - data.observations = (data.observations || []).concat([extra]); - await route.fulfill({ status: 200, contentType: 'application/json', body: JSON.stringify(data) }); + try { + const upstream = await context.request.fetch(route.request()); + const data = await upstream.json(); + const base = (data.observations && data.observations[0]) || {}; + const extra = Object.assign({}, base, { id: Number(SYN_OBS), observer_name: 'e2e-147-observer', snr: -3.5, rssi: -111 }); + data.observations = (data.observations || []).concat([extra]); + await route.fulfill({ status: 200, contentType: 'application/json', body: JSON.stringify(data) }); + } catch (_) { + // A refetch still in flight when the context closes; nothing to answer. + try { await route.abort(); } catch (__) { /* already closed */ } + } }); } @@ -144,6 +149,7 @@ async function setTimeWindow(page, value) { assert(p.q.obs === SYN_OBS, 'obs dropped when the modal closed: ' + p.hash); }); } + await context.unrouteAll({ behavior: 'ignoreErrors' }); await context.close(); } From 223a54ff1fcbacf695887c3138d09f2960aae76a Mon Sep 17 00:00:00 2001 From: dborup Date: Sat, 3 Oct 2026 08:36:38 +0200 Subject: [PATCH 3/4] test(packets): reproduce the #167 review findings (#147) Red on d0a9f28e: - Escape with the View Path modal open closed the detail pane (desktop) or the SlideOver (800 px) too; it must close only the modal. - The detail's View Path button did not write ?viewPath=1, so a reload did not reopen the modal. - The standalone #/packet/ page ignored ?obs= on load, and an observation click rewrote its URL to the packets list URL. - updatePacketsUrl() wrote the list URL on any route; open() on an open modal dropped ?viewPath=1; the modal did not name its packet. New guards (green on d0a9f28e; each catches a named mutant): an observation-row click and a SlideOver close keep the active filters in the URL (E2E, real clicks), viewPath only for a modal on the subpath's packet, and the default route is still written. The E2E closes the SlideOver with a real backdrop click: at 800 px its close button sits under the fixed top nav, so a real click on it hits the nav (pre-existing, CSS untouched). Relates to #147 Co-Authored-By: Claude Opus 5.5 --- test-issue-147-packets-url-detail-params.js | 98 +++++++++++-- test-issue-147-packets-url-obs-e2e.js | 145 +++++++++++++++++++- test-packet-path-map.js | 59 +++++++- 3 files changed, 283 insertions(+), 19 deletions(-) diff --git a/test-issue-147-packets-url-detail-params.js b/test-issue-147-packets-url-detail-params.js index c9e1765ba..681b6b32b 100644 --- a/test-issue-147-packets-url-detail-params.js +++ b/test-issue-147-packets-url-detail-params.js @@ -7,9 +7,14 @@ * filter change and on Clear Filters. * * Decision under test: ?obs= travels with the detail subpath; ?viewPath=1 - * describes the open View Path modal, so it is kept only while that modal - * (#packetPathModal) is open. (Closing the modal drops it from the address - * bar: see test-packet-path-map.js.) + * describes the open View Path modal, so it is in the URL exactly while that + * modal (#packetPathModal) is open on the packet of the subpath, also when + * it was opened with the detail's View Path button (#167 round 2). Closing + * the modal drops it from the address bar: see test-packet-path-map.js. + * + * Route guard (#167 round 2): updatePacketsUrl() writes only while the route + * is the packets list (#/packets…); the standalone #/packet/ page has its + * own writer, updatePacketPageUrl(), for #/packet/?obs=. * * Runs the REAL buildPacketsQuery / updateClearFiltersVisibility / * updatePacketsUrl and the real Clear Filters handler body from @@ -53,10 +58,13 @@ const URL_FUNCS = [ 'function buildPacketsQuery(', 'function updateClearFiltersVisibility()', 'function updatePacketsUrl(', -].map((m) => extractBlock(PACKETS_SRC, m, true)).join('\n'); + 'function updatePacketPageUrl(', +// A missing function fails the tests that call it, not the whole file. +].map((m) => (PACKETS_SRC.indexOf(m) === -1 ? '' : extractBlock(PACKETS_SRC, m, true))).join('\n'); const CLEAR_BODY = extractBlock(PACKETS_SRC, "if (clearBtn) clearBtn.addEventListener('click', function()", false); -// opts.modalOpen: a #packetPathModal overlay is in the DOM. +// opts.modalOpen: a #packetPathModal overlay is in the DOM, open on packet +// 'abc123' (true) or on the given hash (string). function makeSandbox(startHash, opts) { opts = opts || {}; const urls = []; @@ -65,7 +73,9 @@ function makeSandbox(startHash, opts) { 'packetFilterInput', 'packetFilterError', 'packetFilterCount']) { els[id] = { id, value: '', style: { display: 'none' }, classList: { add() {}, remove() {} } }; } - if (opts.modalOpen) els.packetPathModal = { id: 'packetPathModal' }; + if (opts.modalOpen) { + els.packetPathModal = { id: 'packetPathModal', dataset: { hash: typeof opts.modalOpen === 'string' ? opts.modalOpen : 'abc123' } }; + } const store = {}; const ctx = { console, URLSearchParams, encodeURIComponent, decodeURIComponent, @@ -106,6 +116,7 @@ function makeSandbox(startHash, opts) { return ctx.location.hash; }, clear() { vm.runInContext('__clearFilters()', ctx); return ctx.location.hash; }, + pageUrl(obs) { ctx.__obs = obs; vm.runInContext('updatePacketPageUrl(__obs)', ctx); return ctx.location.hash; }, query(tw, region, skipHash) { return ctx.buildPacketsQuery(tw, region, skipHash); }, }; } @@ -152,12 +163,21 @@ test('viewPath: dropped once the modal is closed (URL describes what is shown), assert.strictEqual(s.update(), DETAIL + '?observer=OBS1&obs=123'); }); -test('viewPath: never invented when the URL did not have it', () => { - const s = makeSandbox(DETAIL + '?obs=123', { modalOpen: true }); +test('viewPath: written while the modal is open on this packet (View Path button) (#167 r2)', () => { + // The button opens the modal, then calls updatePacketsUrl(). + const s = makeSandbox(DETAIL + '?timeWindow=60&obs=123', { modalOpen: 'abc123' }); + s.set("filters.hash = 'abc123'; savedTimeWindowMin = 60"); + assert.strictEqual(s.update(), DETAIL + '?timeWindow=60&obs=123&viewPath=1'); + assert.strictEqual(s.update(), DETAIL + '?timeWindow=60&obs=123&viewPath=1', 'not idempotent'); +}); + +test('viewPath: never written for a modal open on another packet', () => { + const s = makeSandbox(DETAIL + '?obs=123', { modalOpen: 'ffee01' }); s.set("filters.hash = 'abc123'"); assert.strictEqual(countParam(s.update(), 'viewPath'), 0); }); + // ---- Every filter type keeps ?obs= (and updates its own param) ---- const FILTER_CASES = [ @@ -289,13 +309,65 @@ test('closing the detail drops subpath and detail params, keeps the filters', () assert.strictEqual(s.update({ subpath: '', obs: null }), '#/packets?timeWindow=60&observer=OBS1'); }); -// ---- One place: updatePacketsUrl() is the only #/packets hash writer ---- +// ---- Route guard (#167 r2): the list URL is written only on #/packets ---- +// On the standalone #/packet/ page an observation click used to rewrite +// the address bar to #/packets/?&obs=… while the +// standalone page stayed on screen. A detail teardown after the route has +// changed (e.g. a SlideOver closed on the way to another page) must not +// rewrite the new page's URL either. + +test('route guard: no list-URL write on the standalone #/packet/ page', () => { + const s = makeSandbox('#/packet/abc123?obs=7'); + s.set("filters.observer = 'OBS1'; savedTimeWindowMin = 60"); + assert.strictEqual(s.update({ subpath: '/abc123', obs: '9' }), '#/packet/abc123?obs=7'); + assert.strictEqual(s.update(), '#/packet/abc123?obs=7'); + assert.strictEqual(s.urls.length, 0, 'replaceState called: ' + JSON.stringify(s.urls)); +}); + +test('route guard: no list-URL write once another page is shown (#/nodes)', () => { + const s = makeSandbox('#/nodes/abcdef'); + s.set("filters.observer = 'OBS1'"); + assert.strictEqual(s.update({ subpath: '', obs: null }), '#/nodes/abcdef'); + assert.strictEqual(s.urls.length, 0, 'replaceState called: ' + JSON.stringify(s.urls)); +}); + +test('route guard: the default route (empty hash) is the packets list and is written', () => { + const s = makeSandbox(''); + s.set("filters.observer = 'OBS1'"); + assert.strictEqual(s.update(), '#/packets?observer=OBS1'); +}); + +// ---- Standalone page writer: #/packet/?obs= (#167 r2) ---- +// The router strips the query before resolving #/packet/, so ?obs= fits +// its format; the standalone page reads it back on load. + +test('standalone page: an observation click writes #/packet/?obs=', () => { + const s = makeSandbox('#/packet/abc123'); + assert.strictEqual(s.pageUrl('7'), '#/packet/abc123?obs=7'); + assert.strictEqual(s.pageUrl('9'), '#/packet/abc123?obs=9'); + assert.strictEqual(s.pageUrl(null), '#/packet/abc123'); +}); + +test('standalone page: other params stay, ?obs= is encoded and appears once', () => { + const s = makeSandbox('#/packet/42?obs=1&embed=1'); + assert.strictEqual(s.pageUrl('a b'), '#/packet/42?embed=1&obs=a%20b'); +}); -test('updatePacketsUrl() is the only history.replaceState() writer in packets.js', () => { +test('standalone page writer leaves the packets list URL alone', () => { + const s = makeSandbox(DETAIL + '?timeWindow=60&obs=1'); + assert.strictEqual(s.pageUrl('7'), DETAIL + '?timeWindow=60&obs=1'); + assert.strictEqual(s.urls.length, 0, 'replaceState called: ' + JSON.stringify(s.urls)); +}); + +// ---- Two writers, each for its own route ---- + +test('history.replaceState() in packets.js only in updatePacketsUrl() and updatePacketPageUrl()', () => { const writes = PACKETS_SRC.match(/history\.replaceState\(/g) || []; - assert.strictEqual(writes.length, 1, 'expected 1 replaceState call (in updatePacketsUrl), got ' + writes.length); - const fn = extractBlock(PACKETS_SRC, 'function updatePacketsUrl(', true); - assert(/history\.replaceState\(/.test(fn), 'the writer is not inside updatePacketsUrl()'); + assert.strictEqual(writes.length, 2, 'expected 2 replaceState calls, got ' + writes.length); + for (const marker of ['function updatePacketsUrl(', 'function updatePacketPageUrl(']) { + const fn = extractBlock(PACKETS_SRC, marker, true); + assert.strictEqual((fn.match(/history\.replaceState\(/g) || []).length, 1, 'no writer inside ' + marker); + } }); test('buildPacketsQuery() is called only from updatePacketsUrl() (every caller covered)', () => { diff --git a/test-issue-147-packets-url-obs-e2e.js b/test-issue-147-packets-url-obs-e2e.js index ce9cabe90..0e335d654 100644 --- a/test-issue-147-packets-url-obs-e2e.js +++ b/test-issue-147-packets-url-obs-e2e.js @@ -2,8 +2,17 @@ * E2E (#147): a packet-detail deep link #/packets/?obs= keeps its * selected observation in the address bar on cold load, on filter changes * and on Clear Filters, and reopening the copied URL in a new tab selects the - * same observation. ?viewPath=1 is kept while the View Path modal is open and - * dropped when it closes. + * same observation. ?viewPath=1 is in the URL while the View Path modal is + * open, also when the detail's View Path button opened it, and is dropped + * when it closes. + * + * #167 round 2: + * - Escape closes only the top layer: the modal first (dropping viewPath, + * keeping the detail and ?obs=), the detail on the next Escape. + * - An observation-row click and the SlideOver close (641–1023 px) keep the + * active filters in the URL. + * - The standalone page #/packet/ writes #/packet/?obs= and + * reads it back on load, instead of the packets-list URL. * * The e2e fixture has no packet with two observations reachable through the * API, so the packet-detail response is stubbed (context.route, so new tabs @@ -47,6 +56,27 @@ async function currentObs(page) { }); } +// Load url as a new document, so no modal or selection from an earlier step +// carries over (goto alone is a same-document hash change). +async function fresh(page, url) { + await page.goto(url, { waitUntil: 'load' }); + await page.reload({ waitUntil: 'load' }); +} + +// The other (real) observation's id, from the rendered rows. +function otherObs(page) { + return page.evaluate((syn) => { + const r = Array.from(document.querySelectorAll('.detail-obs-row')).find((el) => el.dataset.obsId !== syn); + return r ? r.dataset.obsId : null; + }, SYN_OBS); +} +function detailPaneOpen(page) { + return page.evaluate(() => { + const r = document.getElementById('pktRight'); + return !!r && !r.classList.contains('empty') && !!r.querySelector('.detail-obs-row'); + }); +} + async function stubDetail(context, hash) { await context.route((url) => url.pathname === '/api/packets/' + hash, async (route) => { try { @@ -137,22 +167,129 @@ async function setTimeWindow(page, value) { assert(q.obs === SYN_OBS, 'obs dropped by Clear Filters: ' + h); }); - await step('desktop: ?viewPath=1 is kept while the modal is open and dropped when it closes', async () => { + await step('desktop: Escape closes only the View Path modal (drops viewPath, keeps detail + obs); a second Escape closes the detail', async () => { await page.goto(`${DETAIL}?obs=${SYN_OBS}&viewPath=1`, { waitUntil: 'load' }); await page.waitForSelector('#packetPathModal'); await currentObs(page); let p = await hashParams(page); assert(p.q.viewPath === '1' && p.q.obs === SYN_OBS, 'cold load with modal open: ' + p.hash); await page.keyboard.press('Escape'); + await page.waitForSelector('#packetPathModal', { state: 'detached' }); + await waitHash(page, () => !/viewPath=/.test(location.hash), 'viewPath still in URL after closing the modal'); + p = await hashParams(page); + assert(p.q.obs === SYN_OBS && p.hash.startsWith('#/packets/' + hash), 'detail URL changed when the modal closed: ' + p.hash); + assert(await detailPaneOpen(page), 'the first Escape also closed the detail pane'); + await page.keyboard.press('Escape'); + await page.waitForFunction(() => document.getElementById('pktRight').classList.contains('empty'), null, { timeout: 5000 }) + .catch(() => { throw new Error('the second Escape did not close the detail pane'); }); + // The URL after closing the detail pane itself is not asserted: a + // desktop detail close never wrote the URL (out of scope, see #180). + }); + + await step('desktop: View Path button writes ?viewPath=1 next to obs and filters; reload reopens it; closing drops it', async () => { + await fresh(page, `${DETAIL}?timeWindow=60&obs=${SYN_OBS}`); + assert(await currentObs(page) === SYN_OBS, 'observation not selected before opening View Path'); + await page.click('#pktRight [data-view-path]'); + await page.waitForSelector('#packetPathModal'); + await waitHash(page, () => /[?&]viewPath=1(&|$)/.test(location.hash), 'viewPath=1 not written by the View Path button'); + let p = await hashParams(page); + assert(p.hash.startsWith('#/packets/' + hash + '?') && p.q.obs === SYN_OBS && p.q.timeWindow === '60', 'button URL: ' + p.hash); + await page.reload({ waitUntil: 'load' }); + await page.waitForSelector('#packetPathModal'); + assert(await currentObs(page) === SYN_OBS, 'reload did not restore the observation'); + p = await hashParams(page); + assert(p.q.viewPath === '1' && p.q.obs === SYN_OBS && p.q.timeWindow === '60', 'after reload: ' + p.hash); + await page.click('#packetPathClose'); + await page.waitForSelector('#packetPathModal', { state: 'detached' }); await waitHash(page, () => !/viewPath=/.test(location.hash), 'viewPath still in URL after closing the modal'); p = await hashParams(page); - assert(p.q.obs === SYN_OBS, 'obs dropped when the modal closed: ' + p.hash); + assert(p.q.obs === SYN_OBS && p.q.timeWindow === '60', 'close dropped more than viewPath: ' + p.hash); + }); + + await step('desktop: clicking another observation row writes its ?obs= and keeps the filters', async () => { + await fresh(page, `${DETAIL}?timeWindow=60&obs=${SYN_OBS}`); + assert(await currentObs(page) === SYN_OBS, 'observation not selected before the click'); + const other = await otherObs(page); + assert(other, 'no second observation row'); + await page.click(`#pktRight .detail-obs-row[data-obs-id="${other}"]`); + await waitHash(page, () => /[?&]obs=/.test(location.hash) && !location.hash.includes('obs=990000147'), 'obs not switched'); + const p = await hashParams(page); + assert(p.hash.startsWith('#/packets/' + hash + '?'), 'detail subpath lost: ' + p.hash); + assert(p.q.obs === other && p.q.timeWindow === '60', 'observation click URL: ' + p.hash); + }); + + await step('desktop: standalone #/packet/?obs= shows that observation; a row click writes #/packet/?obs=', async () => { + // A fresh tab, so no observation selected on the packets page leaks in. + const tab = await context.newPage(); + tab.on('pageerror', (e) => console.error('[pageerror]', e.message)); + await tab.goto(`${BASE}/#/packet/${hash}?obs=${SYN_OBS}`, { waitUntil: 'load' }); + assert(await currentObs(tab) === SYN_OBS, 'standalone page ignored ?obs='); + const other = await otherObs(tab); + await tab.click(`.detail-obs-row[data-obs-id="${other}"]`); + await waitHash(tab, () => !location.hash.includes('obs=990000147'), 'obs not switched'); + const h = await tab.evaluate(() => location.hash); + assert(h === `#/packet/${hash}?obs=${other}`, 'standalone URL after the click: ' + h); + assert(await tab.evaluate(() => !document.getElementById('pktLeft')), 'not on the standalone page any more'); + await tab.reload({ waitUntil: 'load' }); + assert(await currentObs(tab) === other, 'reload of the standalone URL lost the observation'); + await tab.close(); }); } await context.unrouteAll({ behavior: 'ignoreErrors' }); await context.close(); } + // 641–1023 px: the detail opens in a SlideOver. + { + const context = await browser.newContext({ viewport: { width: 800, height: 900 } }); + await context.addInitScript(() => { + try { localStorage.removeItem('meshcore-time-window'); localStorage.removeItem('meshcore-observer-filter'); } catch (_) {} + }); + await stubDetail(context, hash); + const page = await context.newPage(); + page.setDefaultTimeout(15000); + page.on('pageerror', (e) => console.error('[pageerror]', e.message)); + const slideOverOpen = () => page.evaluate(() => !!(window.SlideOver && window.SlideOver.isOpen())); + + await step('tablet (800): closing the SlideOver with a filter writes the list URL and keeps the filter', async () => { + await page.goto(`${DETAIL}?timeWindow=60&obs=${SYN_OBS}`, { waitUntil: 'load' }); + assert(await currentObs(page) === SYN_OBS, 'observation not selected in the SlideOver'); + assert(await slideOverOpen(), 'detail did not open in a SlideOver'); + // A real click on the backdrop (left of the panel). The panel's × + // sits under the fixed top nav at this width, so a real click on it + // hits the nav instead (pre-existing layout issue, not #147). + await page.click('.slide-over-backdrop', { position: { x: 40, y: 450 } }); + await waitHash(page, () => !location.hash.startsWith('#/packets/'), 'detail subpath still in URL after the SlideOver closed'); + const p = await hashParams(page); + // The list is still filtered to the packet's hash (from the subpath), + // so the list URL carries it as ?hash=. + assert(p.hash.startsWith('#/packets?'), 'list URL: ' + p.hash); + assert(p.q.timeWindow === '60' && p.q.hash === hash && !('obs' in p.q), 'list URL: ' + p.hash); + }); + + await step('tablet (800): Escape closes only the View Path modal; the SlideOver stays until the next Escape', async () => { + await fresh(page, `${DETAIL}?timeWindow=60&obs=${SYN_OBS}`); + assert(await currentObs(page) === SYN_OBS, 'observation not selected in the SlideOver'); + await page.click('.slide-over-panel [data-view-path]'); + await page.waitForSelector('#packetPathModal'); + await waitHash(page, () => /[?&]viewPath=1(&|$)/.test(location.hash), 'viewPath=1 not written by the View Path button'); + await page.keyboard.press('Escape'); + await page.waitForSelector('#packetPathModal', { state: 'detached' }); + await waitHash(page, () => !/viewPath=/.test(location.hash), 'viewPath still in URL after closing the modal'); + assert(await slideOverOpen(), 'the first Escape also closed the SlideOver'); + let p = await hashParams(page); + assert(p.q.obs === SYN_OBS && p.q.timeWindow === '60', 'URL after closing the modal: ' + p.hash); + await page.keyboard.press('Escape'); + await page.waitForFunction(() => !window.SlideOver.isOpen(), null, { timeout: 5000 }) + .catch(() => { throw new Error('the second Escape did not close the SlideOver'); }); + p = await hashParams(page); + assert(p.hash.startsWith('#/packets?') && p.q.timeWindow === '60', 'list URL after the SlideOver closed: ' + p.hash); + }); + + await context.unrouteAll({ behavior: 'ignoreErrors' }); + await context.close(); + } + await browser.close(); console.log(`\n${passed} passed, ${failed} failed`); process.exit(failed ? 1 : 0); diff --git a/test-packet-path-map.js b/test-packet-path-map.js index 06dd06940..1b922eb4a 100644 --- a/test-packet-path-map.js +++ b/test-packet-path-map.js @@ -128,9 +128,20 @@ function makeSandbox(apiImpl) { }; return search(body); }, - addEventListener(type, fn) { (docListeners[type] = docListeners[type] || []).push(fn); }, - removeEventListener(type, fn) { if (docListeners[type]) docListeners[type] = docListeners[type].filter(f => f !== fn); }, + // docLog: the live document listeners with their capture flag (#167 r2: + // the modal's Escape handler must run in the capture phase). + addEventListener(type, fn, opts) { + (docListeners[type] = docListeners[type] || []).push(fn); + docLog.push({ type, fn, capture: opts === true || !!(opts && opts.capture) }); + }, + removeEventListener(type, fn, opts) { + if (docListeners[type]) docListeners[type] = docListeners[type].filter(f => f !== fn); + const capture = opts === true || !!(opts && opts.capture); + const i = docLog.findIndex(r => r.type === type && r.fn === fn && r.capture === capture); + if (i !== -1) docLog.splice(i, 1); + }, }; + const docLog = []; const ctx = { window: {}, document: doc, console, Math, String, JSON, Promise, Error, @@ -146,6 +157,7 @@ function makeSandbox(apiImpl) { // Records replaceState() so the #147 tests can see the address bar. history: { replaceState(_s, _t, url) { ctx.__replaced.push(url); ctx.location.hash = url; } }, __replaced: [], + __docLog: docLog, }; ctx.window.copyToClipboard = (text, onDone) => { ctx.__copiedText = text; if (onDone) onDone(); }; vm.createContext(ctx); @@ -1224,6 +1236,49 @@ function makeSandbox(apiImpl) { '#/analytics?tab=distance&viewPath=1', async (ctx) => { await ctx.window.PacketPathMap.open('deadbeef'); ctx.window.PacketPathMap.close(); }, '#/analytics?tab=distance&viewPath=1', 0); + // #167 r2: open() replaces a modal that is already open. That is not a + // close, so it must not drop ?viewPath=1 (open() used to call close()). + await viewPathCase('open() while the modal is already open keeps ?viewPath=1 (#167 r2)', + '#/packets/deadbeef?obs=123&viewPath=1', + async (ctx) => { + await ctx.window.PacketPathMap.open('deadbeef'); + await ctx.window.PacketPathMap.open('deadbeef'); + assert(ctx.document.getElementById('packetPathModal'), 'modal not open after the second open()'); + }, + '#/packets/deadbeef?obs=123&viewPath=1', 0); + + // #167 r2: packets.js writes ?viewPath=1 while the modal is open on the + // packet of the #/packets/ subpath, so the overlay names its packet. + await viewPathCase('the open modal records its packet hash (data-hash) (#167 r2)', + '#/packets/deadbeef', + async (ctx) => { + await ctx.window.PacketPathMap.open('deadbeef'); + const overlay = ctx.document.getElementById('packetPathModal'); + assert.strictEqual(overlay && overlay.dataset.hash, 'deadbeef'); + }, + '#/packets/deadbeef', 0); + + // #167 r2: Escape closes only the top layer. The modal's Escape handler + // runs in the capture phase and stops the event, so the packets detail + // pane's and the SlideOver's own Escape handlers (bubble phase on + // document) do not close as well. A second Escape reaches them. + await viewPathCase('Escape closes only the modal: capture-phase handler stops the event (#167 r2)', + '#/packets/deadbeef?obs=123&viewPath=1', + async (ctx) => { + await ctx.window.PacketPathMap.open('deadbeef'); + const keys = ctx.__docLog.filter(r => r.type === 'keydown'); + assert.strictEqual(keys.length, 1, 'keydown listeners: ' + keys.length); + assert.strictEqual(keys[0].capture, true, 'Escape handler is not in the capture phase'); + let stopped = 0; + keys[0].fn({ key: 'Escape', stopPropagation() { stopped++; } }); + assert.strictEqual(stopped, 1, 'Escape was not stopped'); + assert.strictEqual(ctx.document.getElementById('packetPathModal'), null, 'modal still open'); + assert.strictEqual(ctx.__docLog.filter(r => r.type === 'keydown').length, 0, 'keydown listener left behind'); + let stoppedAfter = 0; + keys[0].fn({ key: 'Escape', stopPropagation() { stoppedAfter++; } }); + assert.strictEqual(stoppedAfter, 0, 'a stray handler call with no modal open swallowed Escape'); + }, + '#/packets/deadbeef?obs=123', 1); console.log('\n════════════════════════════════════════'); console.log(` packet-path-map.js: ${passed} passed, ${failed} failed`); From 4b4163b38b28dd9b43d5067d0db2717c66aff133 Mon Sep 17 00:00:00 2001 From: dborup Date: Sat, 3 Oct 2026 08:38:43 +0200 Subject: [PATCH 4/4] fix(packets): Escape closes the top layer only; View Path button writes viewPath (#147) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review findings on #167: - Escape: the View Path modal's handler now runs in the capture phase and stops the event. With the modal open, Escape closes only the modal and drops ?viewPath=1; the detail pane (desktop) or the SlideOver (641–1023 px) and ?obs= stay until the next Escape. - View Path button: after opening the modal it calls updatePacketsUrl(), which now writes ?viewPath=1 while the modal is open on the packet of the #/packets/ subpath (the overlay records it in data-hash). A reload reopens the modal; closing drops the param. open() on an open modal replaces it without the close-side URL write, so viewPath stays. - Route guard: updatePacketsUrl() writes only while the packets list is the route (#/packets…, or the empty default route). - Standalone #/packet/: a new writer, updatePacketPageUrl(), writes #/packet/?obs= on an observation click (other params verbatim); the page reads ?obs= on load. The router strips the query before resolving the route, so this fits its format and the URL keeps describing what is shown. - The "only writer" comment names the other hash writers (packet-path-map.js dropViewPathParam, touch-gestures.js row actions). Relates to #147 Co-Authored-By: Claude Opus 5.5 --- public/packet-path-map.js | 36 +++++++++++++++++++-------- public/packets.js | 51 +++++++++++++++++++++++++++++++++------ 2 files changed, 69 insertions(+), 18 deletions(-) diff --git a/public/packet-path-map.js b/public/packet-path-map.js index e58c08569..d864822f0 100644 --- a/public/packet-path-map.js +++ b/public/packet-path-map.js @@ -76,13 +76,22 @@ var activeMap = null; + // Registered in the capture phase while the modal is open: Escape closes + // only this top layer. Stopping it keeps the layers below (the packets + // detail pane, a SlideOver), whose Escape handlers listen on document in + // the bubble phase, open until the next Escape (#167). function onKeydown(e) { - if (e.key === 'Escape') close(); + if (e.key === 'Escape' && document.getElementById('packetPathModal')) { + e.stopPropagation(); + close(); + } } // A #/packets/?…&viewPath=1 URL describes this modal as open (#147). // Once it closes, drop that one param (the others stay verbatim) so a - // refresh or a copied address-bar link does not reopen it. + // refresh or a copied address-bar link does not reopen it. This writes the + // hash next to packets.js's updatePacketsUrl(), which adds ?viewPath=1 while + // the modal is open on the packet of the #/packets/ subpath. function dropViewPathParam() { if (typeof history === 'undefined' || !history.replaceState) return; var h = String(location.hash || ''); @@ -94,17 +103,20 @@ history.replaceState(null, '', h.slice(0, q) + (kept.length ? '?' + kept.join('&') : '')); } - function close() { + // Removes the modal; true when one was open. + function removeModal() { var overlay = document.getElementById('packetPathModal'); - if (overlay) { - overlay.remove(); - dropViewPathParam(); - } + if (overlay) overlay.remove(); if (activeMap) { try { activeMap.remove(); } catch (e) { /* already gone */ } activeMap = null; } - document.removeEventListener('keydown', onKeydown); + document.removeEventListener('keydown', onKeydown, true); + return !!overlay; + } + + function close() { + if (removeModal()) dropViewPathParam(); } // A short prefix marking a node's role in tooltips -- purely a label, @@ -154,11 +166,15 @@ } async function open(hash) { - close(); // in case one's already open + // In case one's already open. Replacing it is not a close, so a + // ?viewPath=1 in the URL stays (#167). + removeModal(); var overlay = document.createElement('div'); overlay.id = 'packetPathModal'; overlay.className = 'modal-overlay'; + // The packet shown, for packets.js's ?viewPath=1 (#167). + overlay.dataset.hash = hash; overlay.innerHTML = '
' + '