Repository navigation
test(chat-virtual): hold the scrollbar's rest until the chat sees the scroll, and bring the output button into view before measuring (test-only) - #2081
Conversation
… scroll, and bring the output button into view before measuring (test-only) The scrollbar cases moved performance.now forward only around the synthetic scroll's dispatch. LegendList hands a scroll to onScroll inside the dispatch only while it settles a data change (its anchor lock lasts 300 ms), otherwise in an animation frame; on a loaded runner ChatView then read the real clock, took the press as fresh input and stopped following (#2022). Each case now awaits that frame before the clock is restored. The whole-output check read the row's position before clicking the output button. When rows above were measured late the button sat just below the fold, and Playwright's click scrolled it into view first; that scroll was counted as the output's. The button is now scrolled into view before the reading. Fixes #2022.
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Wait for scroll event delivery scripts/chat-virtual-browser-qa.ts |
The upward-input tests wait for an animation frame after scroll events, before the next input and after the scroll correction. |
Tool-output button positioning
| Layer / File(s) | Summary |
|---|---|
Position the button before measurement scripts/chat-virtual-browser-qa.ts |
The test scrolls the “more” button into view before measuring its row position and clicking it. |
Priority:
Estimated code review effort: 2 (Simple) | ~5 minutes
Change: Other · Severity of issue fixed: Low
Merge Risk: 
cc7c8
-
No actionable merge-blocking risk remains in the reviewed QA-script changes.
Pre-merge checks: 
-
Check name Status Explanation Resolution Linked Issues check Issue #2022requires the test to wait for the client-reported follow state after scrollbar input, with a bounded wait and no fixed sleep, before pushing the next revision. The change awaits one `reque…Add a bounded wait for the client-reported follow state after each scrollbar input, and complete that wait before the revision update. Keep the wait event- or state-driven and free of fixed sleeps. Out of Scope Changes check The change to call scrollIntoViewIfNeeded()before measuring the output row addresses a separate 390 px output-opening measurement race. Issue#2022concerns scrollbar follow-mode handling. No linke…Move the output-button measurement-race fix to a pull request with a matching linked issue, or link an active issue that requires this coding change. -
Check name Status Explanation Title check The title clearly describes both test-only fixes: preserving the patched clock through the scroll event and bringing the output button into view before measurement. It is specific and related to the m… Docstring Coverage No eligible functions found in the changed files. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Documentation-only edits and deletions within survi… Description check The pull request description is complete and follows the required template. It explains the two timing races, the resulting behavior, the implemented fixes, linked issues, and detailed validation resu… Full details: Title check
-
Explanation
The title clearly describes both test-only fixes: preserving the patched clock through the scroll event and bringing the output button into view before measurement. It is specific and related to the main changes, although somewhat long.
Full details: Docstring Coverage
-
Explanation
No eligible functions found in the changed files. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 0 functions across 1 files.
Full details: Description check
-
Explanation
The pull request description is complete and follows the required template. It explains the two timing races, the resulting behavior, the implemented fixes, linked issues, and detailed validation results. No changelog fragment is appropriately justified because product behavior does not change.
-
Full details: Linked Issues check
-
Explanation
Issue
#2022requires the test to wait for the client-reported follow state after scrollbar input, with a bounded wait and no fixed sleep, before pushing the next revision. The change awaits onerequestAnimationFrameafter the synthetic scroll. It does not wait for or verify the reported follow state. The timing fix may deliver the scroll before clock restoration, but it does not satisfy the stated acceptance criterion.Resolution
Add a bounded wait for the client-reported follow state after each scrollbar input, and complete that wait before the revision update. Keep the wait event- or state-driven and free of fixed sleeps.
Full details: Out of Scope Changes check
-
Explanation
The change to call
scrollIntoViewIfNeeded()before measuring the output row addresses a separate 390 px output-opening measurement race. Issue#2022concerns scrollbar follow-mode handling. No linked issue covers the output-button race, so this change is outside the linked issue scope.Resolution
Move the output-button measurement-race fix to a pull request with a matching linked issue, or link an active issue that requires this coding change.
-
Finishing Touches
-
📝 Generate docstrings
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
|
On CodeRabbit's two pre-merge warnings:
|
|
cc7c884, omo code reviewer: PASS (APPROVE). There are no HIGH or MEDIUM findings.
CodeRabbit reviewed this PR with no actionable comments, and CI is green. It is merged as a test-only PR on the omo review, per the merge pass's rule. |
Change
Test-only:
scripts/chat-virtual-browser-qa.tshad two timing races that fail it on a loaded CIrunner. Product code is unchanged, and so is what the script asserts.
Fixes #2022. Fixes #2086.
1. Scrollbar cases leave follow mode (#2022)
The scrollbar cases simulate a thumb that rested for 600 ms by moving
performance.nowforwardaround a synthetic upward scroll. The patch was removed right after
dispatchEvent("scroll"), onthe assumption that the chat reads the clock inside that dispatch.
LegendList only hands a scroll to its
onScrollprop during the dispatch while it is stillsettling a data change (its MVCP anchor lock lasts 300 ms,
isInMVCPActiveMode). Otherwise ithands the scroll over in an animation frame it requests (
useRafCoalescer). Once that window haspassed,
ChatView.onScrollruns a frame later with the real clock. The press is then a fewmilliseconds old, the 1 px reader allowance applies, and the 20 px move stops following:
only upward input stops following,1 !== 0, inscrollbar-unseen-release,scrollbar-blurredorscrollbar-held-still.Measured with
ChatView.onScrollinstrumented in a scratch checkout:patched: true,byReader: false), so the chat keeps following.patched: false,byReader: true), so thechat stops following.
The fix holds the patched clock until that frame has run: the case awaits one animation frame after
each synthetic scroll. The list's frame was requested first, so it runs first. The scroll that
returns the view to the end before each case is awaited the same way.
This is not a follow-mode bug (#2042 is a separate question). In real use the clock really does
move while the thumb rests, and the chat reads it correctly. Only the simulated rest was too short.
2.
opening output does not skip past the expanded work(390 px, #2086)In the 390 px failures, the rows above row 497 were measured after
reveal()returned, which leftthe row low in the view (top 642 px instead of ~360 px). Once the row is opened, its "Show the whole
output" button sits a few pixels below the 844 px fold. Playwright's
click()scrolls its targetinto view before clicking, so the view moved 5 px between the script's "before" reading and the
output opening, and that move was counted as the output's. Frame-by-frame traces of failing runs
show the 5 px scroll arriving during the click, after the row had fully laid out. Opening the output
itself moved nothing.
The script now brings the button into view (
scrollIntoViewIfNeeded()) before the "before"reading, so the click has nothing left to scroll. To confirm the cause, a scratch copy parks the
button 5 px below the fold right after the row opens:
opening output does not skip past the expanded workon the firstforced instance (1440 dark);
Validation
Loaded runs on the 32-core PC through
dori heavy, each copy pinned withtaskset(or with ChromeCPU-throttled through CDP
Emulation.setCPUThrottlingRate):scrollbar-unseen-release4,scrollbar-blurred4,scrollbar-held-still3)opening output…); with only fix 1, 4 of 30 failed and a traced run 2 of 24, allopening output…bun scripts/chat-virtual-browser-qa.tspasses idle (all 52 PASS lines).bun test scripts/conventions.test.tspasses (no new fixed sleep), and so doesbun run typecheck.bun run check fastpasses on this branch's head: report statussuccess, all seven steps ok,unit tests 7719 pass and 0 fail.
No changelog fragment: nothing users see changes.