fix(ui): follow-ups to #255 — mobile group dead end, nodesEsc leak, test gaps (#259)

.github/workflows/deploy.yml

@@ -451,6 +451,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

public/nodes.js

@@ -582,6 +582,32 @@
 
   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';
+    }
+  }
+
+  // #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 = '<span>Select a node to view details</span>';
+    selectedKey = null;
+    history.replaceState(null, '', '#/nodes');
+    renderRows();
+  }
+
   let regionChangeHandler = null;
 
   function init(app, routeParam) {
@@ -606,13 +632,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 +1275,11 @@
   function destroy() {
     if (wsHandler) offWS(wsHandler);
     wsHandler = null;
+    // #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;
@@ -1664,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 = '<span>Select a node to view details</span>';
-          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) {

public/packets.js

@@ -2428,6 +2428,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/<hash>/<obs>, 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;
@@ -2439,6 +2451,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();
@@ -2447,7 +2462,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) {
@@ -2593,7 +2608,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;
@@ -4353,6 +4368,12 @@
       _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; },
     };
   }
 

test-all.sh

@@ -60,6 +60,7 @@ run test-url-state.js
 run test-node-adverts.js
 run test-issue-254-affinity-debug-toggle.js
 run test-issue-258-column-widths.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

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', () => {

test-issue-254-affinity-toggle-mobile-aria-e2e.js

@@ -13,8 +13,11 @@
  *    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 -- 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
@@ -126,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;
 
@@ -271,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(() => {});
@@ -286,7 +305,35 @@ 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
+    // (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);
+      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();
   }