Repository navigation
test(admin): wait for the deleted block's exit before reading rows - #4778
khoinguyenpham04 wants to merge 2 commits into
Conversation
The end-of-page delete test sampled the rows above for a fixed 450 ms that started before the Delete click. When the click reached the page late, as on loaded CI runners, the window closed before the deleted row had shrunk away, so the row list still held it. Sample until the deleted row is gone and nothing animates, so the check covers the whole exit however late it starts.
|
github-actions
Bot
added
review/needs-review
![emdashbot[bot]](https://avatars.githubusercontent.com/in/3255304?s=60&v=4){kind=small}
There was a problem hiding this comment.
The approach is sound. The sampler now stops once row "four" is gone and no animations are running, rather than after a fixed 450 ms that began before the Delete click. This addresses the slow-runner CI failure without weakening the position assertions, and only the test file changes. Both reviewers approved. One non-blocking suggestion: the new loop has no frame cap, so a delete that never takes effect would hang until the Vitest timeout instead of failing with a clear assertion. The until helper in the same file caps itself at 120 frames. Also, because the stop condition already requires "four" to be gone, the later rowKeys() assertion now only re-checks the order of the remaining rows.
Reviewed by anthropic/claude-opus-5-5 and openai/gpt-6.1-sol.
| if (now - start < ms) requestAnimationFrame(tick); | ||
| else resolve(); | ||
| if (done()) resolve(); | ||
| else requestAnimationFrame(tick); |
There was a problem hiding this comment.
[suggestion] Nothing bounds this loop. If done() never becomes true (the delete doesn't happen, or an animation never finishes), the test hangs until the Vitest timeout instead of failing quickly with a useful message. The until helper above stops after 120 frames. Consider a similar cap here, for example resolving after N frames and letting the assertions that follow report the failure:
if (done() || frames.length >= 300) resolve();
else requestAnimationFrame(tick);There was a problem hiding this comment.
Good catch, capped it at 300 frames in 86fd631, so a row that never goes now fails on the row-list check in a few seconds.
A regression that never removes the row now fails on the row-list assertion within a few seconds instead of hanging until the test timeout.
emdashbot
Bot
added
review/needs-review
There was a problem hiding this comment.
The follow-up is sound: the 300-frame cap addresses the earlier suggestion while retaining condition-based sampling and the existing position assertions. I checked the follow-up diff and surrounding sampler/call-site code; if deletion never completes, the row-list assertion can now report the failure rather than waiting indefinitely for the sampler. The prior finding is resolved, and I found no new issues in the changed code. Static review only; tests were not run.
Follow-up review by openai/gpt-6.1-sol, scoped to the changes since the last review.
🚀 Deploying Preview to Cloudflare 🚀Preview URL: https://noah-blocks-motion-test-flake.try.emdashcms.com, https://noah-blocks-motion-test-flake-emdash-playground.emdash-cms.workers.dev (commit 86fd631)This URL reflects your latest Preview deploymentPreview Deployments by commit
|
What does this PR do?
Fixes a timing flake in the admin browser test
BlocksField.undo.test.tsx> "Structure motion" > "leaves the rows above in place when the last block is deleted at the end of the page". It failed CI's Browser Tests job twice on unrelated PRs, each time the only failure:The test sampled the rows above for a fixed 450 ms, starting before the Delete click. On a loaded runner, the click can reach the page late, so the window closes before the deleted row has shrunk away and been removed. The row list then still holds it.
The sampler now runs until the deleted row is gone and nothing on the page is animating. The assertions are unchanged, and the sampled window now always covers the whole exit motion, however late it starts. Only the test file changes.
Type of change
Your connection to this change
Checklist
pnpm typecheckpassespnpm lintpassespnpm testpasses (or targeted tests for my change)pnpm formathas been runAI assistance disclosure
Screenshots / test output
Not applicable for screenshots.
Reproduction. CPU throttling alone (
Emulation.setCPUThrottlingRateat 4× and 20×, 10 runs each) did not reproduce it: throttling slows the page's own work, not the gap between the sampler starting and Playwright's click arriving. Holding the click back 250 ms inside the sample window, a stand-in for slow click delivery on a busy runner, made the old test fail with the same message as CI. The scratch edits used for this are not part of the PR.Verification.
expected [ 'one', 'two', 'three', 'four' ] to deeply equal [ 'one', 'two', 'three' ].pnpm --silent lint:json | jq '.diagnostics | length'returns 0;pnpm typecheckpasses.