test(#1923 follow-up): pin the packets window in the munger slide-over step - #1942
Merged
Merged
Conversation
…r slide-over step The Kpa-clawbot#1924 sweep pinned `?timeWindow=1440` in test-slideover-1056-e2e.js so its packets navigation tests the slide-over rather than the clock. This is one of the row-dependent packets navigations that sweep did not reach: it goes to bare `#/packets` and waits for `#pktTable tbody tr[data-action]` with an 8s budget, on the default client-side window of `since = now - 15 min`. The freshened fixture's newest rows age out of that window ~15 minutes after tools/freshen-fixture.sh runs. On two of the four master runs on 2026-09-02 (33678488159, 33684614144) this step executed at freshen+8:06 and freshen+10:13 -- margins of 6:54 and 4:47 -- and every test added ahead of it shrinks that. Reproduced by shifting the fixture 20 minutes back and starting the server on it: the unpinned step times out on line 125, the pinned one passes; on a fresh fixture the pinned step passes 3/3. The step runs at an 800px viewport, i.e. `isMobile` (packets.js:732, innerWidth <= 1024). The pin still works because the mobile cap (packets.js:736) only clamps the value read from localStorage; the URL param is applied afterwards (packets.js:1091-1094) and is not re-clamped. Not fixed here, to keep the change to one file: three other row-dependent packets navigations in the same job share the exposure -- test-gestures-1062-e2e.js and test-touch-gestures-coverage-e2e.js (bare `#/packets`, then a `#pktBody tr[data-hash]` wait), and the mobile branch of test-observer-iata-1188-e2e.js. The gesture tests also assert on the URL after in-app navigation, so a bare `?timeWindow=` query would leak into those assertions; observer-iata pins via localStorage today, which the mobile cap clamps, so it would need switching to the URL-param idiom this PR uses. Each is a separate file; happy to send those. Same idiom and value as Kpa-clawbot#1924, same caveat: the value must be > 0 because packets.js only reads the param under `_urlTimeWindow > 0`.
efiten
added a commit
that referenced
this pull request
Sep 3, 2026
…ter (#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>
Collaborator
|
Recycling this (close + reopen) to re-test it against current master. Nothing is wrong with your work and nothing here needs changing from you. Your Playwright failure was the neighbor-graph A plain re-run would not help, and that is measured rather than assumed: a re-run reuses the merge commit from the original run, so it would test your branch against the old master again. Close and reopen is what forces a fresh merge ref. Sorry for the noise on your notifications, and for the delay: your PR was red for days because of code that was not yours. |
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
2 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.
Follow-up to #1923/#1924 — one of the row-dependent packets navigations the pin sweep did not reach.
The gap
test-slideover-1168-munger-e2e.jsnavigates to bare#/packetsand waits for#pktTable tbody tr[data-action]with an 8 s budget, on the default client-side window (since = now − 15 min). It is one of the row-dependent packets navigations #1924 did not reach. Most are already immune: #1924 pinned?timeWindow=1440intest-slideover-1056-e2e.js,test-e2e-playwright.jssetsmeshcore-time-window=525600ingotoPackets()and in the #1791 Group-Data step, andtest-issue-1122/1128widen the window through the UI dropdown.Three other row-dependent packets navigations in the same job share the exposure and are not in this PR, to keep it to one file:
test-gestures-1062-e2e.jsandtest-touch-gestures-coverage-e2e.jsalso assert on the URL after in-app navigation, so a bare?timeWindow=query leaks into those assertions (#/packets?hash=…becomes#/packets?timeWindow=1440&hash=…); they want the localStorage window path.test-observer-iata-1188-e2e.jspins via localStorage, whichpackets.js:736clamps back to 15 min above 180 on a mobile viewport; switching it to the URL-param idiom this PR uses (which is applied after that clamp) would fix it, but it is a separate file.I can send those separately.
(This step itself runs at an 800px viewport —
isMobile, since the breakpoint isinnerWidth <= 1024— and the pin still works precisely because the URL param is read after the mobile clamp, atpackets.js:1091-1094, not from the clamped localStorage value.)Why it matters now, with numbers
On two of the four master runs of 2026-09-02 this step executed at freshen+8:06 (run 33678488159) and freshen+10:13 (run 33684614144) — margins of 6:54 and 4:47 before the fixture's newest rows age out of the window. Every test added ahead of it shrinks that. The suite as a whole is closer still: in run 33684614144 the third repetition of the #1616 flake-gate ran 21:48:21→21:48:45 = freshen+14:45→+15:09, i.e. already past the 15-minute mark — it survives only because of the #1924 pin.
Verification
Reproduced without waiting for the clock: shift the freshened fixture 20 minutes back (
first_seenandobservations.timestamp) and start the server on it —Same idiom and value as #1924, same caveat baked into the comment: the value must be > 0, because
packets.jsonly reads the param under_urlTimeWindow > 0, sotimeWindow=0silently keeps the default.One observation from the same investigation, offered separately from this PR:
tools/freshen-fixture.shcomputes its shift asnow − MAX(first_seen); if the max ever sits in the future, the offset goes negative andprintf('+%d seconds')produces the invalid'+-N seconds'. On the NOT NULLtransmissions.first_seenthat aborts the script (set -e); on the nullable columns (nodes.last_seen, observers, neighbor_edges)strftimesilently returns NULL. A one-line clamp to ≥0 would make it safe to run around an insert. Happy to send that separately if wanted.