Skip to content

test(web): timeline restore test no longer flakes on a late live-follow save - #22528

Closed
tris203 wants to merge 1 commit into
pingdotgg:mainfrom
tris203:chore/flake-messages-timeline-restore
Closed

tris203 wants to merge 1 commit into
pingdotgg:mainfrom
tris203:chore/flake-messages-timeline-restore

Conversation

@tris203

@tris203 tris203 commented Oct 20, 2026

Copy link
Copy Markdown
Contributor

Problem

MessagesTimeline.browser.test.tsx › "MessagesTimeline scroll position › restores a manual reading position after reopening and switching threads" fails intermittently with expected 600 to be less than or equal to 1 at the final switch back to the saved thread (#22324, item 5; also reported in #21968). The 600 is the test's wheel distance.

rememberTimelinePosition stores atEnd: liveFollowEnabled || position.atEnd. After reopening, the test rendered a bare MessagesTimeline with baseProps(), which has liveFollowEnabled: true and a no-op onManualNavigation. The restore effect calls onManualNavigation() for a manual position, but nothing turned live follow off. Once the restore settled, the next handleScroll frame saved the same offset as atEnd: true, so the final switch back scrolled to the end. The test only failed when that frame ran before it switched threads, which is why it passed on rerun.

ChatView wires onManualNavigation to setTimelineLiveFollowEnabled(false), so the app itself is not affected.

Change

The reopened timeline now renders through the test's existing Chat wrapper. The wrapper takes routeThreadKey and passes onManualNavigation={() => setLiveFollowEnabled(false)}, matching ChatView. The component is unchanged.

The fix proposed in #22324 (reusing the wrapper alone) isn't enough: the wrapper only turns follow off on wheel, and nothing wheels after the reopen.

Scope and approval

A test-only fix for one flaky test. It's item 5 of #22324, and @juliusmarminge invited picking up the remaining #22324 flakes. This PR fixes only that item.

Verification

  • Forced the timing with a 300ms wait after the reopened restore. Without the fix, the test fails every time with the CI assertion, and the stored entry reads {"atEnd":true,"scrollOffset":3174} with the expected offset of 3174. The wrapper without onManualNavigation fails the same way. With this change, it passes.
  • vp test run --project browser src/components/chat/MessagesTimeline.browser.test.tsx: 77/77 pass. The target test passed 10/10 repeated runs.
  • Reviewed by a separate agent (GPT-6.1-Sol, high reasoning), with no findings.

Investigated and written by Claude Opus 5.5 in Claude Code (T3 Code).

…erwrites the saved position

After reopening, the test rendered a bare MessagesTimeline whose
onManualNavigation was a no-op, so live follow stayed on. Once the
restore settled, the next scroll save stored the manual position as
atEnd: true, and the final switch back landed at the end. Render the
reopened timeline through the Chat wrapper and turn off live follow on
manual navigation, as ChatView does.
@github-actions github-actions Bot added vouch:trusted PR author is trusted by repo permissions or the VOUCHED list. size:XS 0-9 changed lines (additions + deletions). labels Oct 20, 2026
@macroscopeapp

macroscopeapp Bot commented Oct 20, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Approved at 8abc962

Macroscope's review found this PR approvable — This is a narrowly scoped browser-test-only flake fix confined to an ignored path, with no production runtime or product-default changes. Its impact is limited to test execution.

Notes:

  • No code objects were reviewed. Approvability was decided on eligibility alone.

You can add or adjust custom eligibility rules. Learn more.

@coderabbitai

coderabbitai Bot commented Oct 20, 2026

Copy link
Copy Markdown
Contributor

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Path: .coderabbit.config.ts
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: e882c2a1-73b9-414c-b6b9-104b47df4c83
📥 Commits

Reviewing files that changed from the base of the PR and between 5bcc90e and 8abc962.

📒 Files selected for processing: 1
  • apps/web/src/components/chat/MessagesTimeline.browser.test.tsx

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 8 remain after this review.

📝 Walkthrough

The thread-switching scroll restoration browser test now renders MessagesTimeline through a local Chat wrapper. The wrapper forwards an optional route thread key and disables live-following during wheel and manual navigation.

Changes: Thread scroll restoration

Layer / File(s) Summary
Configure the timeline test wrapper
apps/web/src/components/chat/MessagesTimeline.browser.test.tsx
The test adds a Chat wrapper that forwards routeThreadKey and disables live-following. The timeline factory uses this wrapper for reopen and thread-switch scenarios.

Priority: ⬇️ Low

Estimated code review effort: 1 (Trivial) | ~3 minutes

Change: Other

Suggested reviewers: t3dotgg

Merge Risk: ⚪ Minimal · up to 8abc9

The browser test now covers manual scroll restoration without introducing production changes or merge-blocking risk.

Pre-merge checks: Passed 4
Check name Status Explanation
Title check Passed The title is concise, specific, and accurately describes the test-only fix for intermittent timeline restoration failures caused by a late live-follow save.
Description check Passed The description includes all required sections. It clearly explains the intermittent failure, the test-only change, scope and approval context, and focused verification results.
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Comment @coderabbitai help to get the list of available commands.

@juliusmarminge

Copy link
Copy Markdown
Member

Note

Grok responding on behalf of Julius.

Thanks for tracking this down. #22557 just landed on main and deleted the "restores a manual reading position after reopening and switching threads" test this PR fixes, so there's nothing left for it to change. That also takes care of item 5 in #22324. Closing as superseded.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:XS 0-9 changed lines (additions + deletions). vouch:trusted PR author is trusted by repo permissions or the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants