From 269bb8075fa46c7488f4d87a35bff7e8855d6e39 Mon Sep 17 00:00:00 2001 From: efiten Date: Thu, 3 Sep 2026 09:41:36 +0200 Subject: [PATCH 1/2] fix(#1925): stop theme-refresh from discarding the neighbor-graph filter (#1944) Closes #1925. This is the flake that failed #1942 and #1871, neither of which touches the feature. It is not a test problem. ## Cause Every page load fires exactly one delayed, full re-render of the active analytics tab: 1. `app.js` starts `/api/config/theme` without gating navigation on it, deliberately. 2. When it resolves, `_customizerV2.init()` runs `applyCSS()`, which dispatches `theme-changed`. 3. `app.js:1188` debounces that by 300 ms and dispatches `theme-refresh`. 4. `analytics.js:230` answered it with `renderTab(_currentTab)`. For the neighbor-graph tab step 4 is destructive. `renderTab` replaces `el.innerHTML`, so the role checkboxes are recreated with their defaults and companion is silently re-checked, and `_ngState` is rebuilt from the full 1400-node graph. The count is back over the 1000 limit, so `#ngSkipMsg` returns and the canvas is hidden. The re-entrancy epoch guard cannot prevent it. That guard stops a superseded `tick()` loop; this is a legitimate new top-level render pass that resets the very inputs the guard protects downstream of. **One mechanism produces both documented failure modes**, decided only by where that single re-render lands: | lands | result | |---|---| | before the first uncheck | harmless, test passes | | between an uncheck and the next `waitForFunction` poll | mode 1, the 15 s timeout | | after `waitForFunction` succeeded, before the follow-up `evaluate` | mode 2, "expected #ngSkipMsg gone again" (#1942) | Measured on an idle machine: the test's final evaluate at 542 ms, `theme-refresh` at 636 ms, `#ngSkipMsg` re-added at 667 ms. It passes locally by about 90 ms. On a loaded runner the test's Playwright round trips stretch while `theme-refresh` still lands at theme-fetch latency plus 300 ms, so it arrives mid-test. ## Fix On `theme-refresh`, restart the renderer instead of rebuilding the tab when the neighbor-graph tab is active and has state. Four lines. This stays theme-correct: node colors are read live per frame from `window.ROLE_COLORS`, role swatches use `.role-swatch--{role}` CSS tokens, stats and the skip message use CSS variables, and the one cached theme value, `_labelColor = cssVar('--text-primary')`, is re-read on restart at `analytics.js:3375`, inside `startGraphRenderer`. When `_ngState` is null it falls through to the old path. ## Verification Measured, not asserted: - **Deterministic reproduction** (hold `/api/config/theme` until just before the second filter-down, then stall 500 ms before the final evaluate; no synthetic events dispatched): **2 of 2 fail** on unmodified master with the exact #1942 message, **3 of 3 pass** with this fix. - The **unmodified** E2E test passes against the fixed build. - With the tab open and a filter applied, a `theme-refresh` leaves the filter intact, the canvas present, and produces no page errors. - The **regression test added here fails on unmodified master** with `theme-refresh reset the role filter (companion re-checked)` and passes with the fix. It dispatches the event directly, so it tests the cause instead of waiting for the race to appear. ## What this does not cover The same startup re-render silently discards user interaction in the first second or so on **any** analytics tab, not just this one. A user who clicks quickly after load loses that click. This change covers the neighbor-graph tab, because that is what #1925 is about and what is failing CI. The general fix, for example skipping the startup refresh when the effective config changed nothing, deserves its own issue rather than being smuggled in here. Side observation while tracing: `theme-changed` fires twice at startup, at about 311 ms and 323 ms. The debounce collapses them, so it is harmless, but it means the customizer pipeline runs twice. I did not identify the second dispatcher. Co-authored-by: Claude Opus 5 (1M context) (cherry picked from commit e2df9bbd3e5a28445eea42774713189af14e18af) Co-Authored-By: Claude Opus 5 --- public/analytics.js | 15 +++++++++++- test-issue-1758-ng-filter-rerenders-e2e.js | 28 ++++++++++++++++++++++ 2 files changed, 42 insertions(+), 1 deletion(-) diff --git a/public/analytics.js b/public/analytics.js index 846ca04bb..74ad68fa1 100644 --- a/public/analytics.js +++ b/public/analytics.js @@ -246,7 +246,20 @@ } // Re-render when distance unit or theme changes - _themeRefreshHandler = function () { renderTab(_currentTab); }; + _themeRefreshHandler = function () { + // #1925: never full-rebuild the neighbor-graph tab on theme-refresh. + // Every page load fires one theme-refresh ~300ms after + // /api/config/theme resolves (app.js dispatches 'theme-changed', then + // debounces 300ms). renderTab() here replaced el.innerHTML, which reset + // the role checkboxes to their defaults and rebuilt _ngState from the + // full graph, discarding any filtering the user had applied in the + // meantime. Restarting the renderer keeps the current filter state and + // still picks up the new theme: node colors are read live per frame + // from window.ROLE_COLORS, role swatches use CSS tokens, and the one + // cached value (_labelColor) is re-read on restart. + if (_currentTab === 'neighbor-graph' && _ngState) { startGraphRenderer(); return; } + renderTab(_currentTab); + }; window.addEventListener('theme-refresh', _themeRefreshHandler); loadAnalytics(); diff --git a/test-issue-1758-ng-filter-rerenders-e2e.js b/test-issue-1758-ng-filter-rerenders-e2e.js index 1a75fdb43..f51e323ea 100644 --- a/test-issue-1758-ng-filter-rerenders-e2e.js +++ b/test-issue-1758-ng-filter-rerenders-e2e.js @@ -204,6 +204,34 @@ function buildFixture() { if (reFiltered.canvasDisplay === 'none') fail('second filter-down: expected #ngCanvas visible again'); console.log(' ✓ second filter-down: render restored across repeated cycles'); + // --- #1925: a theme-refresh must not discard the applied filter. + // Every page load fires one 'theme-refresh' about 300ms after + // /api/config/theme resolves. It used to call renderTab(), which replaced + // el.innerHTML (resetting the role checkboxes to their defaults) and + // rebuilt _ngState from the full graph, putting #ngSkipMsg back. On an + // idle machine the test finished ~90ms before that landed; under CI load + // it landed mid-test, which is both failure modes of #1925. + // Dispatching the event directly makes that deterministic, so this asserts + // the cause rather than waiting for the race to show up. + await page.evaluate(() => window.dispatchEvent(new CustomEvent('theme-refresh'))); + await page.waitForTimeout(500); + const afterTheme = await page.evaluate(() => { + const canvas = document.getElementById('ngCanvas'); + const cb = document.querySelector('#ngRoleChecks input[data-role="companion"]'); + return { + hasSkip: !!document.getElementById('ngSkipMsg'), + canvasDisplay: canvas ? getComputedStyle(canvas).display : '(no canvas)', + companionChecked: cb ? cb.checked : '(no checkbox)', + }; + }); + if (afterTheme.companionChecked !== false) { + fail('theme-refresh reset the role filter (companion re-checked): ' + + JSON.stringify(afterTheme)); + } + if (afterTheme.hasSkip) fail('theme-refresh brought #ngSkipMsg back: ' + JSON.stringify(afterTheme)); + if (afterTheme.canvasDisplay === 'none') fail('theme-refresh hid #ngCanvas again'); + console.log(' ✓ theme-refresh preserves the applied filter (#1925)'); + await browser.close(); console.log('\nPASS: #1758 neighbor-graph filter re-render lifecycle holds'); From 2d26603bd09e1fb9aa4393e5b56d436cde491e5a Mon Sep 17 00:00:00 2001 From: Dennis Jakobsen Date: Sun, 20 Sep 2026 15:21:49 +0200 Subject: [PATCH 2/2] reviewfix(analytics): make the theme-refresh comment match what it fixes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Independent review verified the theme claim end-to-end (steady-state canvas paint sets are identical on the fast path and on a full rebuild, in both themes, and a live role-colour override still repaints without renderTab), so the guard itself stands. Two corrections to what the comment says about it: - It undersold the change. Measured, the old handler leaked one rAF render loop and one canvas listener set per theme-refresh: after ten spaced events the reverted build ran 720 canvas clears/sec across 12 loops, and a synchronous burst reached 1320/sec across 22 loops with 70 listeners. The new path stays at 60/sec, 1 loop, 7 listeners under both. That is a real performance fix, not just filter preservation, and it is now stated. - It overstated the coverage. _ngState is assigned only after the graph fetch resolves, while the checkboxes exist from the synchronous innerHTML before it, so a theme-refresh landing inside the fetch window still falls through to renderTab() and still discards a filter applied during the load — the very window the comment names. The reviewer reproduced this with a delayed fetch and confirmed it is byte-identical on a reverted build, i.e. pre-existing rather than introduced here. Closing it needs a load-in-flight sentinel with careful failure-path handling, which does not belong in this port; the comment now says so, and it is filed as follow-up work. Comment only — no behaviour change. The existing 6 assertions still pass, and reverting the guard still fails the #1925 step. Co-Authored-By: Claude Opus 5 --- public/analytics.js | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/public/analytics.js b/public/analytics.js index 30e0ead5a..7f6bfad8b 100644 --- a/public/analytics.js +++ b/public/analytics.js @@ -256,7 +256,17 @@ // meantime. Restarting the renderer keeps the current filter state and // still picks up the new theme: node colors are read live per frame // from window.ROLE_COLORS, role swatches use CSS tokens, and the one - // cached value (_labelColor) is re-read on restart. + // cached value (_labelColor) is re-read on restart. Measured: this also + // stops the old path leaking one rAF render loop and one canvas + // listener set per theme-refresh (a 10-event burst ran the force + // simulation at ~20x speed; it now stays flat). + // + // Not covered, pre-existing: _ngState is only assigned after the graph + // fetch resolves, while the checkboxes exist from the synchronous + // innerHTML before it. A theme-refresh landing inside that fetch window + // still falls through to renderTab() and still discards a filter + // applied during the load. Closing that needs a "load in flight" + // sentinel, which is deliberately left out of this port. if (_currentTab === 'neighbor-graph' && _ngState) { startGraphRenderer(); return; } renderTab(_currentTab); };