Skip to content

fix(web): timeline no longer jumps up when a row is removed mid scroll - #13707

Open
santiago-ramos-02 wants to merge 1 commit into
pingdotgg:mainfrom
santiago-ramos-02:fix/legendlist-stale-scroll-target
Open

santiago-ramos-02 wants to merge 1 commit into
pingdotgg:mainfrom
santiago-ramos-02:fix/legendlist-stale-scroll-target

Conversation

@santiago-ramos-02

@santiago-ramos-02 santiago-ramos-02 commented Sep 25, 2026 •

Copy link
Copy Markdown

Fixes #13706

What Changed

One guard in T3 Code's existing @legendapp/list patch, added to every build (web, React Native, React Native Web, CommonJS and ESM). prepareMVCP now skips sizing the in-flight scroll target when that target's row no longer exists:

-      if (scrollingToViewPosition && scrollingToViewPosition > 0) {
+      if (scrollingToViewPosition && scrollingToViewPosition > 0 && targetId !== null && targetId !== void 0) {

pnpm-lock.yaml only changes the patch hash.

Why

scrollToEnd records the last index as scrollingTo. If data shrinks before that scroll completes, getId(state, scrollTarget) returns null because the index is past the end. The check then calls getItemSize with that null key, and setSize passes it to addTotalSize, which treats a null key as "set the total to this size". The list's total height collapses to one estimated row. It then relays out from scroll = 0 and estimates unmeasured rows at the average measured size. In a long thread that lands near the top.

The timeline hits this when a turn starts. The trailing live activity row is replaced, briefly removing a row, while the send's scrollToEnd is still running. Details and measurements are in #13706.

Skipping the size check is safe here. It only adjusts for a target row that changed size, and a removed row has no size to compare. The scroll still finishes at the new end.

UI Changes

A page that renders only a LegendList with the timeline's options (150 tall rows, estimatedItemSize={90}), served by the apps/web dev server so it uses T3 Code's patched 3.3.5 and build. It scrolls up, calls scrollToEnd({ animated: true }), and removes the last row 60ms later.

Before, the list lands on rows 0 and 1, about 25,000px from the end:

legendlist-before-fix.mp4

After, it ends at the end:

legendlist-after-fix.mp4

Checklist

  • This PR is small and focused
  • I explained what changed and why
  • I included before/after screenshots for any UI changes
  • I included a video for animation/interaction changes

Tested with the page above, before and after, removing the row at 60ms and 150ms. Also ran two controls: scrolling without removing ends at the end, and removing without a scroll in flight keeps the position. pnpm install --frozen-lockfile applies the patch cleanly. A plain app built with Rolldown-based Vite and pristine npm 3.3.5 reproduces it too; 3.4.0 does not. Moving to @legendapp/list 3.4.x would also fix this, but that means porting the existing patch, so this guard is the smaller fix until then.

Model: Claude Opus 5.5 (1M context), running in Claude Code inside T3 Code.

🤖 Generated with Claude Code

LegendList records the last index as its scroll target when scrollToEnd
runs. If data shrinks before that scroll completes, the visible-position
check sizes a target whose key is now null, and a null key resets the
list's total size. The list then relays out from scroll 0 with average
row sizes, which in a long thread lands near the top. The timeline hits
this when a turn starts and its trailing live row is replaced while the
send's scrollToEnd is still running.

Skip that size check when the target row no longer exists. The guard is
added to every build in T3 Code's existing @legendapp/list patch.

Fixes pingdotgg#13706

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@github-actions github-actions Bot added the vouch:unvouched PR author is not yet trusted in the VOUCHED list. label Sep 25, 2026
@github-actions github-actions Bot added the size:M 30-99 changed lines (additions + deletions). label Sep 25, 2026
macroscopeapp[bot]
macroscopeapp Bot previously approved these changes Sep 25, 2026
@macroscopeapp

macroscopeapp Bot commented Sep 25, 2026 •

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Approved at 98056ad

Macroscope's review found this PR approvable — The change is a contained fix to existing LegendList scroll handling, preventing a removed in-flight target from collapsing the list’s measured height. All module variants receive the same guard, and the lockfile change only records the updated patch hash.

Notes:

  • Diff unchanged. Approvability was decided on eligibility alone.

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

@coderabbitai

coderabbitai Bot commented Sep 25, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository: pingdotgg/t3code/.coderabbit.yaml

Review profile: CHILL

Plan: Advanced

Run ID: 6a29d779-aa02-4ec3-ab2b-a90efee54adc

📥 Commits

Reviewing files that changed from the base of the PR and between ed809f7 and 98056ad.

⛔ Files ignored due to path filters (1)
  • pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml
📒 Files selected for processing (1)
  • patches/@legendapp__list@3.3.5.patch

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


📝 Walkthrough

Walkthrough

The LegendList patch updates MVCP checks, DOM ordering, and anchored end-space handling. The PR summary also describes keyboard inset adjustments, inset-aware scroll positioning, and content-size animation handling.

Changes

LegendList behavior

Layer / File(s) Summary
Guard MVCP size checks
patches/@legendapp__list@3.3.5.patch
The MVCP size-change check now requires a non-null target ID across the generated builds.
Update DOM node ordering
patches/@legendapp__list@3.3.5.patch
Web and React DOM sorting use moveBefore when available, with insertion-method fallbacks.
Adjust anchored end space and padding
patches/@legendapp__list@3.3.5.patch
Anchored end space is capped by known-size bounds and updated when unresolved size shrinks. ScrollAdjust records the padding value read from the content node.

Priority: ➖ Normal

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix · Severity of issue fixed: Medium

Suggested reviewers: juliusmarminge

Merge Risk: ⚪ Minimal · up to 98056

The patch updates LegendList behavior, and the reviewed changes establish no actionable regression; it appears ready for normal checks.

Architecture Summary

Architecture risk: 🔵 Low · up to 98056

The change affects 1 system.

Changed systems: patches

Architecture concerns
No architecture-level concerns identified.

Review details

Systems and components

  • observed — patches (service) was modified; 1 changed file maps to changed impact.

Before / after behavior

  • observed — Modified behavior in patches/@legendapp__list@3.3.5.patch: The generated react-native.js patch metadata changes its index hash.
  • observed — Modified behavior in patches/@legendapp__list@3.3.5.patch: The MVCP size-change branch now runs only when targetId is neither null nor undefined; previously, a positive view position alone allowed the size lookup.
  • observed — Modified behavior in patches/@legendapp__list@3.3.5.patch: The generated react-native.mjs patch metadata changes its index hash.
  • observed — Modified behavior in patches/@legendapp__list@3.3.5.patch: The MVCP size-change branch now requires a non-null target ID, narrowing the previous positive-view-position-only condition.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Out of Scope Changes check ⚠️ Warning The raw change summary reports changes unrelated to issue #13706, including keyboard composer inset APIs, contentInsetStartAdjustment, initial-scroll and end-settle behavior, animated size handling,… Remove the unrelated keyboard, inset, animation, rendering, event, DOM-reordering, and ScrollAdjust changes from this PR, or provide evidence that each change is required to implement issue #13706.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed The PR implements the direct requirement in issue #13706. The prepareMVCP target-size checks now require a non-null target ID in the native, web, and React builds, which prevents sizing a removed sc…
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 0…
Title check ✅ Passed The title clearly identifies the web timeline jump caused by removing a row during scrolling. It accurately summarizes the primary bug fix.
Description check ✅ Passed The description includes the required What Changed, Why, UI Changes, and Checklist sections. It explains the root cause, scope, testing, and observed result in sufficient detail.
Full details: Out of Scope Changes check

Explanation

The raw change summary reports changes unrelated to issue #13706, including keyboard composer inset APIs, contentInsetStartAdjustment, initial-scroll and end-settle behavior, animated size handling, Reanimated behavior, native drag and momentum events, DOM moveBefore reordering, and ScrollAdjust padding tracking. Issue #13706 requires only the null-target guard that prevents the total-size collapse during an in-flight scrollToEnd. These additional changes are not connected to that fix. The PR summary also claims that the PR only adds the guard and updates the lockfile, which conflicts with the raw change summary; the reported unrelated changes must be removed or their connection to issue #13706 must be established.

  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR

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

@santiago-ramos-02

Copy link
Copy Markdown
Author

Update after more testing: this is a bug in the published @legendapp/list 3.3.5, not in T3 Code's patch. A plain app with pristine npm 3.3.5 jumps every time when built with Rolldown-based Vite (as apps/web is), and does not jump with esbuild-based Vite. 3.4.0 does not reproduce under the same setup. So upgrading to 3.4.x would also fix it, but this one-line guard is the smaller change while the existing 3.3.5 patch is in place. I updated the description and #13706 to match.

@juliusmarminge juliusmarminge added the macroscope-review Opt PRs made by unvouched contributors in for Macroscope review. Vouched contributors auto-reviews label Oct 1, 2026 — with ChatGPT Codex Connector
@macroscopeapp
macroscopeapp Bot dismissed their stale review October 1, 2026 08:01

Dismissing prior approval to re-evaluate 98056ad

This branch has not been deployed

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

Labels

macroscope-review Opt PRs made by unvouched contributors in for Macroscope review. Vouched contributors auto-reviews size:M 30-99 changed lines (additions + deletions). vouch:unvouched PR author is not yet trusted in the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: Timeline jumps far up right after sending in a long thread

2 participants