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) <noreply@anthropic.com>
This commit is contained in:
efiten
2026-09-03 09:41:36 +02:00
committed by GitHub
co-authored by Claude Opus 5
parent 9ae3387416
commit e2df9bbd3e
2 changed files with 42 additions and 1 deletions
+14 -1
View File
@@ -228,7 +228,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();
@@ -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');