Skip to content

fix(diffs): keep scroll anchor stable when CodeView removes items - #1047

Merged
amadeus merged 2 commits into
pierrecomputer:mainfrom
fdarian:fix-codeview-reconcile-scroll-anchor
Aug 3, 2026
Merged

fix(diffs): keep scroll anchor stable when CodeView removes items#1047
amadeus merged 2 commits into
pierrecomputer:mainfrom
fdarian:fix-codeview-reconcile-scroll-anchor

Conversation

@fdarian

@fdarian fdarian commented Aug 3, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

This PR is fully authored by Opus 5 and Sonnet 5 using Claude Code harness.

Here's a message from Opus regarding this change

CodeView.reconcileItems reassigns this.items (and rebuilds idToItem)
without first calling capturePendingLayoutAnchor(), unlike setOptions,
which always captures one before a layout-affecting change. The next render's
getScrollAnchor() call has nothing pending, so it scans
renderState.firstIndex..lastIndex against the array reconcileItems just
reindexed. Every index at or after a removal now points at a different
logical item, read through .top/.height that recomputeLayout hasn't
refreshed yet, so the search either returns the wrong item or, once the
window is far enough into stale territory, finds nothing at all and leaves
scrollTop stuck at its old value while the content underneath it has moved.

Measured against a 355-file list, removing one file from the middle:

  • Unpatched: scrollTop stays at 400000 while content shifts underneath
    it, landing the viewport on an unrelated item; scrollHeight drops from
    738888 to 735704.
  • Patched: scrollTop moves 400000 → 398256, tracking the same anchor
    line, with scrollHeight shrinking by exactly the removed item's height
    plus gap in one update and no resettle.

I first hit this in fdarian/nisi, a diff
review app built on @pierre/diffs, via its "hide reviewed files" action
(removes a card from the rendered list) on a ~206-file list: scrollTop
collapsed to 0, scrollHeight dropped from 10088 to 502, then
resettled ~600-900ms later at 4956, on unrelated files.

Same-length updates are unaffected; this only hits removals and reorders.

The fix is one line: call capturePendingLayoutAnchor() in
reconcileItems, placed after the method's existing no-op early return
(if (firstDirtyIndex == null) return;) and before this.items = nextItems.
Placement matters: capturing unconditionally at the top of the method would
strand a pending anchor on the instance after every no-op setItems call,
since a same-length update that changes nothing returns early without
rendering, and render is what consumes/clears the pending anchor. Some later,
unrelated render would then "correct" scroll against that stale anchor. I hit
exactly that while testing the fix, so it's a real trap and not hypothetical.

Added a regression test in CodeView.scrollAnchoring.test.ts that scrolls
into a 60-item list, removes 10 earlier items in one setItems call, and
asserts the anchored item's scroll offset shifts by exactly the removed
items' combined height. It fails on main (scroll position stuck) and
passes with the fix.

Fixes a scroll-position bug in CodeView's virtualized list: removing or reordering items in the controlled items array destroys the user's scroll position instead of keeping their place.

This one-line patch was discovered after Opus-5-observed Sonnet 5 did three iterations to solve the following bug. As shown in the video, removing an item will break the scroll position:

before.mp4

Then here's after it added the patch:

after.mp4

Type of changes

  • Bug fix (non-breaking change which fixes an issue)
  • Refactoring (non-breaking change)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would change existing functionality)
  • Documentation update

Checklist

  • I have read the contributing guidelines
  • My code follows the code style of the project (moon run root:lint)
  • My code is formatted properly (moon run root:format)
  • I have updated the documentation accordingly (if applicable)
  • I have added tests to cover my changes (if applicable)
  • All new and existing tests pass (moonx diffs:test)

Related issues

Possibly related to #1030, which is the same symptom (scroll anchor failing
to recover) via a different trigger (markdown remount / transient estimated
heights) and a different code path; not claiming this fixes it.

@fdarian
fix(diffs): keep scroll anchor stable when CodeView removes items
419a48a 
Scroll partway down a CodeView list, then update the controlled
items array to drop or reorder an earlier entry (for example,
hiding an already-reviewed file). The viewport jumps to an
unrelated position instead of staying on the item that was on
screen.

reconcileItems reassigns this.items without first capturing a
scroll anchor, unlike setOptions, which always captures one before
a layout-affecting change. Without a pending anchor, the next
render's getScrollAnchor scans the old render window's indices
against the freshly reindexed items array, reading position data
that recomputeLayout has not refreshed yet, so the anchor it finds
is wrong or nonexistent.

Capture the anchor right before reassigning this.items, after the
method's existing no-op early return rather than unconditionally at
the top. A same-length update that changes nothing returns early
without rendering, and render is what consumes a pending anchor;
capturing before that check would leave a stale anchor on the
instance for a later, unrelated render to "correct" against.
@vercel

vercel Bot commented Aug 3, 2026

Copy link
Copy Markdown

@fdarian is attempting to deploy a commit to the Pierre Computer Company Team on Vercel.

A member of the Team first needs to authorize it.

ije
ije approved these changes Aug 3, 2026
View reviewed changes

@ije ije left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@amadeus

amadeus commented Aug 3, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Hi! Yes, we should def fix this, but i think there's a few more areas of improvements. Your fix will only work if the top of your file is in the viewport so it can anchor to an earlier file. I'm take a few moments to try and see if i can get it to be more stable there and not cause a scroll jump if you're scrolled to the bottom of the file and remove it.

(the reason it will break is because you'll capture a pending anchor for the deleted file, and then when it attempts to rectify it wont exist and everything will be broked, so i'm looking into a way to fix pending anchor capture to not include removed files)

@amadeus
Harden item deletion scroll anchoring
14c4400 
This updated implementation adds more robustness to scroll anchoring to
continue to work on the following scenarios:

* If the bottom of the item to be removed was visible in the viewport
  * Prevent anchoring to an element that will be removed
* If the removed item took up the full viewport
  * Attempt to anchor to the next item's top
@amadeus

amadeus commented Aug 3, 2026

Copy link
Copy Markdown
Member

Oki, I added a commit that hardens this a bit more.

So here's a before, that shows the existing fix, but also shows two other cases where the scroll anchoring breaks:

bugs.mp4

Basically if you delete an item who's bottom is fully in view, the scroll anchoring will break. And if you remove an item who takes up the FULL viewport, then you'll just be arbitrarily somewhere else.

The fixes in action:

fixed.mp4

Added a way to ensure we never anchor to an element that will be removed in the next render (this fixes the visible bottom item breaking scroll anchoring).

For items that fully take up the viewport, we just anchor the next item if it exists, to the top.

@vercel

vercel Bot commented Aug 3, 2026 •
edited
Loading

Copy link
Copy Markdown

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

Project Deployment Actions Updated (UTC)
pierre-docs-diffs Ready Ready Preview Aug 3, 2026 10:26pm
pierre-docs-diffshub Ready Ready Preview Aug 3, 2026 10:26pm
pierre-docs-trees Ready Ready Preview Aug 3, 2026 10:26pm
pierrejs-diff-demo Ready Ready Preview Aug 3, 2026 10:26pm

Request Review

@amadeus

amadeus commented Aug 3, 2026

Copy link
Copy Markdown
Member

@codex review

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Aug 3, 2026

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. 🚀

Reviewed commit: 14c4400258

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

@amadeus
amadeus merged commit 94b65d7 into pierrecomputer:main Aug 3, 2026
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ije ije ije approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@fdarian @amadeus @ije