Harden ai-assistant scrolling tests against a scroll-settle race#5401
Open
habdelra wants to merge 1 commit into
Open
Harden ai-assistant scrolling tests against a scroll-settle race#5401habdelra wants to merge 1 commit into
habdelra wants to merge 1 commit into
Conversation
The "open a room" scroll assertions in the ai-assistant scrolling suite read the conversation's scroll position synchronously immediately after the room renders. The panel auto-scrolls to the newest message when the last message registers its scroller, and re-scrolls whenever that message's subtree mutates — so avatars, card pills, and markdown that finish rendering after the test runloop has otherwise settled shift the layout and briefly move the scroll position before the component corrects it. A single synchronous read races that correction and intermittently observes distanceFromBottom past the bottom threshold. Replace those point-in-time reads with a waitUntil poll that tolerates the late re-scroll, and report the exact scroll geometry (scrollHeight / clientHeight / scrollTop / distanceFromBottom) on timeout so a future failure is diagnosable instead of a bare "expected true". The two checks that verify enqueuing a streaming event does not synchronously jank the scroll stay synchronous by design. Co-Authored-By: Claude Opus 4.8 (1M context) <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
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.
The
Integration | ai-assistant-panel | scrollingsuite intermittently fails onscrolling stays at the bottom if a message is streaming inwithAI assistant is scrolled to bottomexpectedtrue, gotfalse(failing CI job).Root cause
The "open a room" assertions read the conversation's scroll position synchronously, immediately after the room renders. The panel auto-scrolls to the newest message when the last message registers its scroller, and re-scrolls whenever that message's subtree mutates (see the per-message
MutationObserverinai-assistant/message). Avatars, card pills, and markdown that finish rendering after the test runloop has otherwise settled shift the layout and briefly move the scroll position before the component re-scrolls to correct it. A single synchronous read races that correction and occasionally samples a position past the bottom threshold. The failing run shows this directly — the avatarset-background-imagehelper module 404s and retries at +100ms, i.e. late layout work landing aftersettled().Only the first assertion in the test can fail this way:
simulateRemoteMessagedelivers events viasetTimeout(0)and Glimmer batches renders, so the two assertions that follow asimulateRemoteMessagewith noawaitread stale DOM (still at the bottom) and pass trivially.Fix
Replace the point-in-time scroll reads in the "open a room" assertions with a
waitUntilpoll that tolerates the late re-scroll. The predicate is unchanged, so an already-settled panel resolves immediately; only the racing case now waits for the component's own correction to land. On timeout the helper reports the exact geometry (scrollHeight/clientHeight/scrollTop/distanceFromBottom/bottomThreshold) so a future failure is diagnosable instead of a bareexpected true.The same race exists in the sibling "open a room" checks (scroll-to-last, scroll-to-first-unread, and the unread-indicator click, which previously used a fixed 2s sleep), so they move to the same helper. The two checks that verify enqueuing a streaming event doesn't synchronously jank the scroll stay synchronous by design.
Verification
Test-only change;
eslintandember-tscare clean for the file. The underlying flake is timing- and CI-load-dependent and doesn't reproduce reliably locally, so the diagnostics are included as insurance: if the poll ever times out, the next failure will print the exact scroll geometry.