Repository navigation
test(packets): stop the clamp E2E racing the table re-render - #70
Merged
Merged
Conversation
The Kpa-clawbot#1122 Details-row clamp E2E intermittently fails CI with "locator.scrollIntoViewIfNeeded: Element is not attached to the DOM". It blocked two unrelated PRs in one afternoon (#54 and #40, neither touching packets), and passed on a plain rerun both times. Cause: the step resolves the row locator, then awaits a page.evaluate() that fetches /api/packets/<hash> over the network, then calls scrollIntoViewIfNeeded() on that pre-fetch locator. The packets table live-updates, so the fetch is ample time for a re-render to replace the row. scrollIntoViewIfNeeded() is a one-shot action that throws on a detached node rather than re-resolving. click() already scrolls the target into view as part of its actionability checks, and it re-resolves the selector when the element detaches, so dropping the separate scroll removes the race without giving anything up. No assertion is changed — verified by diffing every assert()/waitForFunction line against master: identical, only shifted by the added comment. The suite passes 18/18 six consecutive times locally against a CI-faithful fixture (committed fixture + freshen-fixture.sh + both CI seed rows + migrate); the pre-fix code also passes 6/6 locally, so this is reasoning about the race the CI logs show, not a locally reproduced failure. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…creenshots Independent review caught two mistakes in the previous commit. The stated cause was wrong. There are no live updates to race: the reviewer measured zero WebSocket frames in 6s, packets.js has no interval, and the Go poller only broadcasts rows newer than the max ID captured at startup, so a static fixture pushes nothing. The locator is also created after the fetch, and Playwright locators are lazy, so 'a locator resolved before the fetch' described nothing real. The actual trigger, traced and measured: the one-shot theme-refresh. app.js fetches /api/config/theme, dispatches theme-changed, debounces 300ms, and packets.js re-runs renderTableRows(), which resets _lastVisibleStart and clears tbody.innerHTML — rows ready at +279ms, theme-refresh at +496ms, all 62 rows detached at +535ms. The background hop-resolution job re-renders the same way with unbounded latency. The comment now says this, so the next person does not go hunting for WebSocket traffic that is not there. Also: the previous commit accidentally included 619KB of Playwright screenshot output (git add -A swept up e2e-screenshots/*.png, which the test writes and which was never tracked before). Removed, and the directory is now gitignored so running the suite no longer dirties the working tree. The code change itself is unchanged and was independently confirmed: the reviewer reproduced the CI failure deterministically by firing a single theme-refresh 2-8ms into the action — pre-fix fails 4/4 with the exact CI message, post-fix passes 4/4 — and verified click() alone scrolls a virtualized row from top=2093 into view. 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
1 participant
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.
Why
The Kpa-clawbot#1122 Details-row clamp E2E fails CI intermittently with:
It blocked two unrelated PRs in one afternoon — #54 (My Mesh node link) and #40 (neighbor-graph filter), neither of which touches the packets page — and passed on a plain rerun both times. Left alone it will keep taxing every PR that goes through this pipeline.
Cause
In the
full message is shown when a long row is selectedstep the order is:const row = page.locator('#pktBody tr[data-hash="…"]')await page.evaluate(async … fetch('/api/packets/' + h) …)— a real network round trip, looping over candidate hashesawait row.scrollIntoViewIfNeeded()on the locator from step 1The packets table live-updates, so step 2 is ample time for a re-render to replace that row.
scrollIntoViewIfNeeded()is a one-shot action: it throws on a detached node instead of re-resolving.Fix
Drop the separate scroll.
click()already scrolls the target into view as part of its actionability checks and re-resolves the selector when the element detaches, so it waits the row out instead of failing.What this does not do
No assertion is changed. Verified by diffing every
assert(/waitForFunctionline against master — identical, only shifted by the added comment.Verification
tools/freshen-fixture.sh+ both CI seed rows +corescope-migrate).🤖 Generated with Claude Code