fix(packets): keep ?obs=/?viewPath= on packet detail URLs (#147)

.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

public/packet-path-map.js

@@ -76,18 +76,47 @@
 
   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();
+    }
   }
 
-  function close() {
+  // A #/packets/<hash>?…&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. 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/<hash> subpath.
+  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('&') : ''));
+  }
+
+  // Removes the modal; true when one was open.
+  function removeModal() {
     var overlay = document.getElementById('packetPathModal');
     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,
@@ -137,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 =
       '<div class="modal" style="max-width:min(92vw,700px);padding:16px">' +
         '<button type="button" id="packetPathClose" aria-label="Close" ' +
@@ -167,7 +200,7 @@
     overlay.addEventListener('click', function (e) { if (e.target === overlay) close(); });
     var closeBtn = document.getElementById('packetPathClose');
     if (closeBtn) closeBtn.addEventListener('click', close);
-    document.addEventListener('keydown', onKeydown);
+    document.addEventListener('keydown', onKeydown, true);
 
     // Shareable link: #/packets/<hash>?viewPath=1 -- packets.js's init()
     // (public/packets.js) checks for viewPath=1 and re-opens this exact

public/packets.js

@@ -799,18 +799,64 @@
     cb.style.display = active ? '' : 'none';
   }
 
-  function updatePacketsUrl() {
-    // Preserve any subpath after /packets (e.g. #/packets/<hash>).
+  // The only writer of the packets list URL #/packets… in packets.js (#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 while the View Path modal is open
+  // on that packet (cold-loaded link or the detail's View Path button), so
+  // the URL keeps describing what is shown.
+  // It writes only while the packets list is the route (#167): not on the
+  // standalone #/packet/<id> page (see updatePacketPageUrl) and not after a
+  // detail teardown that runs once another page is shown.
+  // Other writers of this hash, outside packets.js:
+  // - packet-path-map.js dropViewPathParam(): removes only viewPath=1 when
+  //   the modal closes; obs and the filter params stay verbatim.
+  // - touch-gestures.js row-action overlay: navigates (location.hash) to
+  //   #/packets?hash=<hash> or #/packets/<hash>, a new list query or a new
+  //   selection. That re-runs init(), whose cold-load call here writes the
+  //   restored filters back; the previous packet's ?obs= does not apply.
+  // detail (optional): { subpath: '/<hash|id>' 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 = '';
+    // The packets list route: #/packets, #/packets/…, #/packets?…, or no
+    // route at all (the router's default page).
+    if (cur !== '' && cur !== '#/' && !/^#\/packets(?:[/?]|$)/.test(cur)) return;
+    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); });
+      }
+      var pathModal = document.getElementById('packetPathModal');
+      if (pathModal && subpath === '/' + pathModal.dataset.hash) keep.push('viewPath=1');
+    }
+    if (keep.length) query += (query ? '&' : '?') + keep.join('&');
+    history.replaceState(null, '', '#/packets' + subpath + query);
     updateClearFiltersVisibility();
   }
 
+  // The standalone page's URL, #/packet/<id>?obs=<id> (#167): the router
+  // strips the query before resolving the route, and the page reads ?obs=
+  // back on load. Writes only on that route; other params stay verbatim.
+  function updatePacketPageUrl(obs) {
+    var m = String(location.hash || '').match(/^(#\/packet\/[^?]+)(?:\?(.*))?$/);
+    if (!m) return;
+    var params = (m[2] ? m[2].split('&') : []).filter(function (p) { return p && !/^obs=/.test(p); });
+    if (obs) params.push('obs=' + encodeURIComponent(obs));
+    history.replaceState(null, '', m[1] + (params.length ? '?' + params.join('&') : ''));
+  }
+
   let filtersBuilt = false;
   let _renderTimer = null;
   function scheduleRender() {
@@ -1095,13 +1141,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 +1164,10 @@
     if (_urlRegion) _pendingUrlRegion = _urlRegion;
     var _urlHash = _initUrlParams.get('hash');
     if (_urlHash) filters.hash = _urlHash;
+    // ?obs=<observation id> on #/packets/<hash> 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/<hash>?viewPath=1, so a shared
     // link reopens the exact same modal instead of leaving the recipient
@@ -1848,8 +1891,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 +3174,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 +3207,7 @@
         onClose: function () {
           selectedId = null;
           selectedObservationId = null;
-          history.replaceState(null, '', '#/packets');
+          updatePacketsUrl({ subpath: '', obs: null });
           renderTableRows();
         }
       });
@@ -3558,10 +3596,11 @@
       row.addEventListener('click', () => {
         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}`);
+        // Update URL hash to reflect selected observation (deep linking).
+        // This detail renders on the packets list and on the standalone
+        // #/packet/<id> page; each writer only writes on its own route.
+        updatePacketsUrl({ subpath: '/' + (pkt.hash || pkt.id), obs: obsId });
+        updatePacketPageUrl(obsId);
         renderDetail(panel, data, obsId);
       });
     });
@@ -3575,6 +3614,9 @@
     if (viewPathBtn) {
       viewPathBtn.addEventListener('click', () => {
         if (window.PacketPathMap) window.PacketPathMap.open(viewPathBtn.dataset.viewPath);
+        // The modal is in the DOM now: add ?viewPath=1 like a shared link
+        // has it, so a reload reopens it (#167). Closing drops it again.
+        updatePacketsUrl();
       });
     }
 
@@ -4183,7 +4225,10 @@
         container.innerHTML = `<div style="margin-bottom:16px"><a href="#/packets" style="color:var(--link-color);text-decoration:none">← Back to packets</a></div>`;
         const detail = document.createElement('div');
         container.appendChild(detail);
-        await renderDetail(detail, data);
+        // #/packet/<id>?obs=<id> selects that observation (#167); without
+        // it the first one, not one left over from the packets list.
+        selectedObservationId = getHashParams().get('obs');
+        await renderDetail(detail, data, selectedObservationId);
         app.innerHTML = '';
         app.appendChild(container);
       } catch (e) {

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

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);