Conversation
The actionability poll behind click(selector)/scrollTo(selector) took its first bounding-box sample synchronously and the second one after a single requestAnimationFrame. When the page-side evaluate task lands inside the same rendering frame as that first rAF callback (which headless Chrome lets happen between 'update animations' and rAF dispatch), both samples read the animation's from-keyframe, the element looks stable, and the click is dispatched before the animation has moved. Move the rAF to the top of the loop so every sample is taken from inside a rAF callback, and only accept a match when the rAF timestamp advanced, matching Playwright's _checkElementIsStable. Applies to both Chrome CDP and WKWebView backends. The animation test is reworked to restart the animation via evaluate() immediately before each click(), which reproduces the race on every iteration instead of the ~2% it previously hit in CI.
WalkthroughChangesAnimation-frame actionability
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 6:13 PM PT - Jul 22nd, 2026
❌ @robobun, your commit 921a5f7 has 2 failures in
🧪 To try this PR locally: bunx bun-pr 35173That installs a local version of the PR into your bun-35173 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — well-diagnosed flake fix that mirrors Playwright's _checkElementIsStable pattern; the inline test-duration nit is worth a follow-up but non-blocking.
Extended reasoning...
Overview
The PR fixes a recurring CI flake in click(selector)'s actionability check. The embedded JS polling loop in both ChromeBackend.h (CDP) and WebViewHost.cpp (WKWebView) is changed so every bounding-box sample is taken after an await requestAnimationFrame, and a match only counts when the rAF timestamp advanced (t !== lastT). Previously the first sample was synchronous and could land in the same rendering frame as the first rAF callback, making a just-started animation look stable at its from-keyframe. The Chrome test is rewritten to deterministically reproduce the pre-fix failure by restarting the animation via evaluate() immediately before each click().
Security risks
None. The change is confined to a page-side polling IIFE that reads getBoundingClientRect() and elementFromPoint; no new inputs, no escaping/serialization changes, no allocation or lifetime changes. The selector still goes through appendQuotedJSONString / NSDictionary args as before.
Level of scrutiny
Moderate. It's a small, focused logic change (~10 LOC across two embedded scripts) to a user-facing API's actionability predicate, backed by an instrumented trace, a cited reference implementation (Playwright), and a deterministic before/after test. The root cause is clearly explained and the fix is the minimal shape that addresses it. The one behavioral cost — one extra rAF frame (~16ms) of latency on static elements — is disclosed and acceptable for correctness.
Other factors
- Both sibling implementations (Chrome CDP and WKWebView) are updated identically, satisfying the "fix the whole class" rule.
- I traced the loop invariants: on iteration 0,
lastis undefined so the match can't fire;lastTis set unconditionally each iteration; the deadline check moved to after the rAF but still runs every iteration;lastis still reset toundefinedwhen the element disappears or leaves the viewport, so the stability window correctly restarts. - The only finding is a test-speed nit (10×200ms iterations ≈ 2.5–3s vs the ~1s budget). It's a fair point per REVIEW.md but doesn't affect correctness — CI passes
--timeout=90000and the wall-clock is dominated by Chrome's rAF cadence rather than debug-build slowdown. Worth trimming to ~5×100ms in a follow-up, but not a blocker. - No prior reviewer comments to address; no CODEOWNERS on this path.
There was a problem hiding this comment.
LGTM — the earlier test-speed concern was addressed in 3153dd1 (5×100ms, ~0.9s, verified still fails 10/10 without the src/ change). The remaining inline nit about porting the regression-proof test shape to webview.test.ts:553 is worth a follow-up but doesn't block: the WKWebView hunk is defensive symmetry with no observed flake, and the existing test still exercises the path.
Extended reasoning...
Overview
Fixes a recurring CI flake in click(selector)'s actionability check by moving every bounding-box sample inside a rAF callback and gating "stable" on t !== lastT so the two samples are provably from distinct rendering frames. Applied symmetrically to kActionabilityIIFE (ChromeBackend.h) and kActionabilityJS (WebViewHost.cpp). The Chrome test is rewritten to restart the animation via evaluate() immediately before each click(), which makes the pre-fix same-frame race deterministic per iteration.
Security risks
None. The changed code is a page-side injected JS string literal; the loop structure changed but no new user input is interpolated (selector still goes through appendQuotedJSONString / NSDictionary args). kScrollToIIFE/kScrollToJS are correctly left alone since they have no stability comparison.
Level of scrutiny
Medium. This is user-facing runtime behavior in shipped C++, but the diff is a tight rearrangement of an existing 15-line loop with a well-documented root cause (instrumented trace showing identical document.timeline.currentTime across two samples), and it mirrors Playwright's _checkElementIsStable. I traced the loop invariants: last is reset to undefined on element-missing / out-of-viewport, lastT is unconditionally updated each iteration, the deadline check still runs once per rAF, and the acknowledged ~16ms added latency on static elements is the only behavior change on the happy path.
Other factors
- My previous review's test-speed concern (10×200ms ≈ 2.5s) was addressed exactly as suggested; the author confirmed the trimmed test still fails 10/10 without the fix, satisfying the fails-for-the-right-reason requirement.
- The one open nit (WKWebView sibling test at
webview.test.ts:553still uses the old animation-in-initial-HTML shape) is a coverage gap on a defensive-symmetry change — the PR cites only Chrome flakes, and WKWebView'scallAsyncJavaScript:XPC scheduling differs from CDPRuntime.evaluate. Worth doing, not blocking. - No third sibling:
webview-chrome-ws.test.tshas no animation test, and thekScrollTo*scripts don't compare across frames.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/runtime/webview/ChromeBackend.h`:
- Around line 280-290: Shorten the comments describing the stable-element
predicate and regression tests to three lines or fewer while retaining only the
distinct-rAF invariant and essential behavior. Update ChromeBackend.h lines
280-290, WebViewHost.cpp lines 354-367, webview-chrome.test.ts lines 735-743,
and webview.test.ts lines 567-573; remove scheduling rationale and defer it to
PR documentation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9c79966b-76b2-47a5-8425-dad0a8fa7206
📒 Files selected for processing (4)
src/runtime/webview/ChromeBackend.hsrc/runtime/webview/WebViewHost.cpptest/js/bun/webview/webview-chrome.test.tstest/js/bun/webview/webview.test.ts
The leading await rAF meant click(selector) could never reject if the renderer was not producing frames (headless WKWebView without a display driver), regressing the WK injection-safe test which previously threw from querySelector before ever reaching rAF. Race the rAF wait against a setTimeout bound to the remaining deadline so the documented timeout option holds regardless of frame delivery, and move the deadline check ahead of querySelector so the first thing reached after the race is the throw. Comments tightened per review.
There was a problem hiding this comment.
LGTM — both earlier review points (test wall-clock, WKWebView test parity) are addressed in 3153dd1 and 0f7e19f.
What was reviewed:
- rAF-first +
t !== lastTloop in bothkActionabilityIIFEandkActionabilityJS—lastTis set unconditionally per iteration,laststill resets on not-found/out-of-viewport, so the match predicate is strictly tighter than before. - Deadline check moved after the leading rAF — still one check per iteration; only effect is the documented +1 frame latency on static elements.
- Chrome test at 5×100ms (~0.9s) and the ported
itRenderingWKWebView test — both use the evaluate()-driven restart the author verified fails deterministically without the src/ change.
Extended reasoning...
Overview
Fixes a recurring CI flake in webview-chrome.test.ts "click(selector) waits for animation to stop" by changing the page-injected actionability poll (in both src/runtime/webview/ChromeBackend.h and src/runtime/webview/WebViewHost.cpp) to take every bounding-box sample from inside a rAF callback and to require the rAF timestamp to have advanced before accepting a match. The two sibling tests (webview-chrome.test.ts and webview.test.ts) are rewritten to restart the animation via evaluate() immediately before each click(), which the author demonstrated fails 10/10 without the fix.
Security risks
None. The change is to two static JS string constants that run inside the page's JS context; no new user input flows in, and selector escaping is unchanged (still via appendQuotedJSONString / NSDictionary arguments).
Level of scrutiny
Moderate. This is a targeted flake fix with a clearly stated and instrumented root cause, modeled on Playwright's _checkElementIsStable. The loop restructuring is small: the rAF await moves from the bottom to the top of the loop body, a timestamp guard is added, and the deadline check stays once-per-iteration. I traced the state transitions for last/lastT across the not-found, out-of-viewport, hit-test-fail, and match branches — the new predicate is a strict tightening of the old one (adds t !== lastT), so no previously-rejected state can now pass. The only behavioural cost is one extra frame of latency for already-stable elements, which the PR calls out.
Other factors
I raised two concerns on earlier revisions: (1) the 10×200ms test exceeded the ~1s budget, and (2) the WKWebView test hadn't been ported to the regression-proof shape. Both were addressed (3153dd1 → 5×100ms ≈ 0.9s; 0f7e19f → webview.test.ts now uses the same restart-via-evaluate loop under itRendering, so a local macOS run guards the WebViewHost.cpp hunk). The bug-hunting pass on the final revision found nothing. The change is self-contained to the webview subsystem, applied identically to both sibling entry points, and the tests are structured to fail deterministically without the fix.
Same setTimeout race as the actionability loop, applied to the sibling
kScrollToIIFE/kScrollToJS so scrollTo(selector, {timeout}) also rejects
when the renderer is not producing frames. Pre-existing behaviour, not a
regression from this PR, but the two loops share the pattern.
There was a problem hiding this comment.
LGTM — all prior feedback (test wall-clock, WKWebView sibling test, scrollTo rAF guard) is addressed and the bug-hunt pass found nothing new.
What was reviewed:
- Traced the rAF/setTimeout race in both actionability and scrollTo scripts — double-resolve of
fis safe (Promise resolve is idempotent),clearTimeouton a fired timer is a no-op, and when the timer winst=undefinedsot !== lastTstays false and can't produce a spurious "stable". - Confirmed
lastT = truns on every loop path (including thelast = undefinedbranches) so the distinct-frame gate can't be bypassed by an intermediate not-actionable frame. - Verified all four
requestAnimationFramesites insrc/runtime/webview/now use the deadline-bound race; no remaining bare-rAF sibling. - Checked the test's
clientX=130arithmetic (left=100 + width/2=30,margin:0) and that the body-levelevent.clientXcapture asserts the dispatched coordinate independently of where#moveris when the event lands.
Extended reasoning...
Overview
The PR rewrites the four embedded page-side JS poll loops (kActionabilityIIFE/kActionabilityJS for click(selector) and kScrollToIIFE/kScrollToJS for scrollTo(selector)) in src/runtime/webview/ChromeBackend.h and src/runtime/webview/WebViewHost.cpp, plus rewrites the two "waits for animation to stop" tests in webview-chrome.test.ts and webview.test.ts. The C++/header changes are string-constant-content-only; no native control flow, allocation, or FFI surface changes.
Security risks
None. Selector escaping is unchanged (still appendQuotedJSONString on the Chrome side and arguments: NSDictionary on the WKWebView side); the injected scripts touch only performance.now, setTimeout, requestAnimationFrame, querySelector, getBoundingClientRect, and elementFromPoint — all pre-existing.
Level of scrutiny
Medium. This is a user-facing runtime API (Bun.WebView#click(selector)) whose timing semantics change: every selector click now waits at least one animation frame before its first sample, and "stable" now requires distinct rAF timestamps. That's a deliberate ~16 ms latency add on static elements, explicitly called out in the PR body and matching Playwright's _checkElementIsStable. I've reviewed the loop line-by-line: the setTimeout-vs-rAF race resolves the promise at most once meaningfully (second f() is a no-op), the timer path yields t=undefined so two consecutive timer-wins compare undefined !== undefined → false and cannot false-accept, and the deadline check now sits immediately after the wait so the documented timeout contract holds even when the renderer produces no frames.
Other factors
This is my fourth pass. The three earlier findings (test budget, WKWebView sibling test coverage, scrollTo sibling guard) were each fixed in dedicated commits and the threads are resolved. The one remaining thread is CodeRabbit's mechanical "comments must be ≤3 lines" nit, which robobun declined citing local file convention — the surrounding ChromeBackend.h/WebViewHost.cpp carry multi-paragraph design comments, and REVIEW.md's "match the exact file's local conventions" applies. The 4-line comments carry the load-bearing invariant (why rAF-first + t !== lastT, why the setTimeout race), which is exactly the durable non-obvious content REVIEW.md asks comments to keep. No CODEOWNERS entry for src/runtime/webview/. CI build #78135 is in flight for the head commit; the Chrome test is the one that flaked across the 11 cited builds, so a green run there is the real proof.
|
Diff is green: |
This branch has not been deployed
1 participant
Fixes the recurring flake in
test/js/bun/webview/webview-chrome.test.ts"chrome: click(selector) waits for animation to stop" (builds 76361, 76546, 76555, 76765, 77013, 77055, 77097, 77119, 77769, 77839, 78003 among others). The test and the behaviour were introduced in #28185.Cause
The actionability poll behind
click(selector)took its first bounding-box sample synchronously, then one more after a singleawait requestAnimationFrame, and accepted "stable" if they matched:Under headless Chrome, the
Runtime.evaluatetask that runs this IIFE can be scheduled inside the same rendering update as the first rAF callback, so the synchronous sample and the post-rAF sample read the identical animation state. For an element whose CSS animation just had its style applied, that state is the from-keyframe (left: 0), the loop returns immediately, and the click is dispatched before the slide has moved. The CI failure values (0,20.828125) are the element's position in the first two frames of the 80ms animation.Instrumented trace from a failing run:
Fix
Take every sample from inside a rAF callback and only accept a match when the rAF timestamp advanced, so "stable" means the same bounding box across two distinct rendering frames. The rAF wait races a
setTimeoutbound to the remaining deadline so the documentedtimeoutoption still rejects when the renderer is not producing frames (headless WKWebView without a display driver, where the loop previously relied on the firstquerySelectorrunning before rAF was ever reached):This is the same shape as Playwright's injected
_checkElementIsStable(rAF first, drop frames where the clock hasn't advanced, outer deadline race). Both the Chrome CDP and the WKWebView actionability scripts are updated. The extra leading rAF adds one frame (~16 ms) of latency toclick(selector)on a static element.Test
Both backend tests now apply the animation via
evaluate()immediately before eachclick(). Because the style change leaves the animation play-pending until the next rendering update, the pre-fix loop seesleft=0twice and clicks atclientX=30on essentially every iteration, while the fixed loop waits for the slide to settle atclientX=130. Asserting on the body-levelevent.clientXcaptures the coordinate the actionability check chose regardless of whether the element has already moved past it.no test proof · iteration 1 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/bun/webview/webview-chrome.test.ts test/js/bun/webview/webview.test.ts