Repository navigation
port(upstream#1944): theme refresh keeps the neighbor-graph filter - #40
Merged
Merged
Conversation
…r-graph filter (Kpa-clawbot#1944) Closes Kpa-clawbot#1925. This is the flake that failed Kpa-clawbot#1942 and Kpa-clawbot#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" (Kpa-clawbot#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 Kpa-clawbot#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 Kpa-clawbot#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> (cherry picked from commit e2df9bb) Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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 Kpa-clawbot#1925 step. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Split out of #25 (commit
a80759aathere). This branch holds exactly one upstream change so it can be reviewed, tested and reverted on its own.Upstream
e2df9bbd3e5a28445eea42774713189af14e18afgit cherry-pick -xonto masterfda24ca5; upstream authorship kept, and the commit message carries the(cherry picked from commit …)line.public/analytics.js,test-issue-1758-ng-filter-rerenders-e2e.js.Problem
Every page load fires one
theme-refreshabout 300 ms after/api/config/themeresolves. The analytics handler then fully re-rendered the active tab. On the Neighbor Graph tab that reset the role checkboxes and rebuilt_ngStatefrom the full graph, discarding any filtering the user had already applied.Change
On
theme-refresh, the Neighbor Graph tab (when its state exists) only restarts the renderer (startGraphRenderer()), which keeps filter state and re-reads theme colours. Other tabs still re-render as before.Adaptation to this fork
None. The cherry-pick applied without conflicts and the changed lines are identical to upstream.
Dependencies and merge order
fda24ca5and needs no other PR from this split.TestPruneOldNeighborMetricsdeterministic). If test(ingestor): make neighbor metrics pruning deterministic #33 lands first, the expected CI failure named below disappears; nothing in this PR depends on it.Verification
Local run of the same commands as CI's “Go Build & Test” job (server tests with
-race), on this branch and on masterfda24ca5under the same conditions (same machine, run one after another):fda24ca5xss-gate-diffchannel-lib-testdecrypt-cli-build-testdockerfile-copy-invariantsdeclare -A), macOS has 3.2; identical on masterstaging-disk-monitorcss-vars-linttest-issue-1375-scope-stats-fetch.jsExactly oneapi('/scope-stats'call exists (the fixed loader) — found 2test-issue-1648-m4-emoji-scan.jsmap.js has 1 emoji/misc-icon hit(s):test-a11y-axe-routes-coverage.jsaxe ROUTES missing analytics tabs (issue #1706): areas, foreign-traffic, wardrivingtest-frontend-helpers.jsfavStar returns empty star for non-favorite: The expression evaluated to a falsy value:,favStar returns filled star for favorite: The expression evaluated to a falsy value:Baseline failures (fail identically on master; not introduced or changed here): see rows marked baseline failure, unchanged.
Browser validation (local, fixture DB, no staging/production): Local Go server (built from master) on the committed E2E fixture DB (freshened, migrated and seeded exactly like CI), serving this branch's
public/, compared side by side with the same server serving master'spublic/. Analytics → Neighbor Graph: unchecked the "repeater" role filter, then dispatchedtheme-refresh(what app.js fires ~300 ms after the theme config loads). master: the tab was rebuilt (new checkbox elements) and "repeater" was reset to checked, discarding the filter. this branch: same checkbox elements, "repeater" stays unchecked after the refresh.Not run:
test-issue-1758-ng-filter-rerenders-e2e.js(Playwright) have not run anywhere for this fork.eslint(not installed locally; CI installs it on the fly).-race/tests for modules this PR does not touch (unchanged code, identical to master).Expected GitHub CI: “Go Build & Test” is expected to fail on
TestPruneOldNeighborMetrics, which already fails on master (see #25's run). Downstream jobs (Playwright, image build) are therefore skipped. “Deploy Staging” and all GHCR publish steps only run onpushtomasterand cannot run for this PR.Two further ingestor tests have failed intermittently in this split's CI on branches whose
cmd/ingestortree is byte-identical to master (#27, #28), so they can also appear here without being caused by this change:TestBackfillTxLastSeen_ResolvesFromMaxObservationTimestamp: also reproduced locally on unmodified master.TestMQTTStallWatchdog_DisconnectedEscalationThrottled_1749: the suite flake that upstream test(ingestor): join the watchdog loop goroutine instead of only asking it to stop Kpa-clawbot/CoreScope#2003 (also split out of port(upstream): 26 clean upstream fixes — prune batching, /ws limits, observer liveness, watchdog race #25) addresses.GitHub CI result: run 34751052618 on
269bb807. Go Build & Test: failure; all downstream jobs incl. Deploy Staging skipped. Failed tests:TestPruneOldNeighborMetrics: fails on master, documented baseline🤖 Generated with Claude Code