Skip to content

fix(web): keep queued transcript snaps reader-safe - #328

Merged
cursor[bot] merged 2 commits into
elie222:mainfrom
luinbytes:bugfix/transcript-follow-races
Aug 28, 2026
Merged

fix(web): keep queued transcript snaps reader-safe#328
cursor[bot] merged 2 commits into
elie222:mainfrom
luinbytes:bugfix/transcript-follow-races

Conversation

@luinbytes

@luinbytes luinbytes commented Aug 27, 2026

Copy link
Copy Markdown
Contributor

Follow-up to #324 for review comments that arrived after merge.

  • cancel queued refresh snaps when the reader moves before the frame runs
  • do not classify the first scroll event as downward without a baseline
  • initialize the baseline when pointer, touch, or upward-wheel interaction starts
  • cover both timing decisions with focused regression tests

Verification:

  • vitest run --root . apps/web/src (21 files, 123 tests)
  • tsc --noEmit -p apps/web/tsconfig.json
  • biome check on the three changed files

Summary by CodeRabbit

  • Bug Fixes
    • Improved transcript auto-scrolling after thread refreshes.
    • Prevented automatic snapping when the reader has moved away from the transcript’s end.
    • Improved detection of manual scrolling, including initial and upward scroll movement.
  • Tests
    • Added coverage for transcript scroll positioning and movement behavior.

@vercel

vercel Bot commented Aug 27, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
Project Deployment Actions Updated (UTC)
rakazo-www Skipped Skipped Aug 27, 2026 3:38pm

Request Review

@coderabbitai

coderabbitai Bot commented Aug 27, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 55c5a1d3-2e9d-4231-9897-a3adada7eb51

📥 Commits

Reviewing files that changed from the base of the PR and between 701a389 and 9406ba0.

📒 Files selected for processing (3)
  • apps/web/src/lib/transcript-scroll.test.ts
  • apps/web/src/lib/transcript-scroll.ts
  • apps/web/src/pages/Shell.tsx

Included review availability: Your plan provides up to 4 included reviews per hour; 2 remain after this review.


📝 Walkthrough

Walkthrough

Transcript refreshes now preserve the captured transcript element before snapping to the end. Shared helpers define frame-safe snapping and scroll-direction detection, with tests covering their behavior.

Changes

Transcript scroll behavior

Layer / File(s) Summary
Scroll-position helpers and validation
apps/web/src/lib/transcript-scroll.ts, apps/web/src/lib/transcript-scroll.test.ts
Updates transcriptCanSnapAfterFrame to require the same element and queued scroll position. Adds transcriptMovedDown and tests for both helpers.
Frame-safe transcript snapping
apps/web/src/pages/Shell.tsx
Adds a shared requestAnimationFrame snapping helper. refreshGroupThread and refreshThread use it before setting the captured transcript element to the end.
Transcript interaction tracking
apps/web/src/pages/Shell.tsx
Initializes lastScrollTop as null. Pointer, touch, and wheel handlers record the current position. The scroll handler uses transcriptMovedDown.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Merge Risk: ⚪ Minimal · up to 9406b

This change makes transcript scrolling safer around queued frame updates and initial direction detection, with focused regression coverage. No actionable merge-blocking risk remains after normal checks and review.

Sequence Diagram(s)

sequenceDiagram
  participant Refresh
  participant SnapHelper
  participant TranscriptElement
  Refresh->>SnapHelper: request transcript snap
  SnapHelper->>TranscriptElement: capture element and scrollTop
  SnapHelper->>TranscriptElement: validate element and queued target after frame
  SnapHelper->>TranscriptElement: set scrollTop to scrollHeight
Loading

Suggested reviewers: elie222

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 7 functions across 3 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: making queued transcript snaps safe for reader scroll behavior.
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.
  • Fix all pre-merge checks with AI

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.

❤️ Share

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

@greptile-apps

greptile-apps Bot commented Aug 27, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

The PR makes queued transcript snaps conditional on both DOM-element identity and unchanged scroll position, preventing delayed refresh frames from overriding reader movement or affecting a replacement transcript.

  • Adds a reader-safe animation-frame snap helper and applies it to thread refresh paths.
  • Initializes manual-scroll baselines from pointer, touch, and upward-wheel interactions.
  • Adds focused tests for replacement elements, movement during queued frames, and uninitialized scroll baselines.

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains.

Important Files Changed

Filename Overview
apps/web/src/lib/transcript-scroll.ts Adds element-identity and scroll-position validation for queued snaps and avoids classifying movement without an initialized baseline.
apps/web/src/lib/transcript-scroll.test.ts Adds deterministic regression coverage for reader movement, replacement transcript elements, and initial movement detection.
apps/web/src/pages/Shell.tsx Captures queued transcript ownership before scheduling and initializes movement baselines when manual interaction begins.

Reviews (2): Last reviewed commit: "fix(web): bind queued snap to transcript" | Re-trigger Greptile

Comment thread apps/web/src/pages/Shell.tsx Outdated
@github-actions

github-actions Bot commented Aug 27, 2026

Copy link
Copy Markdown

Playwright screenshots

Open screenshot gallery · Dashboard · CI run

Updated for commit 9406ba0.

@luinbytes

Copy link
Copy Markdown
Contributor Author

All late #324 findings and the follow-up ownership review are addressed. Fresh CI is green, Greptile reports 5/5 with no blocking issue, CodeRabbit generated no actionable comments, and all #328 review threads are resolved. Ready for maintainer merge.

@cursor
cursor Bot merged commit 7aba83d into elie222:main Aug 28, 2026
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant