Adding annotation in `FileDiff` + `Virtualizer` causes scroll jump

[jonatanklosko]

Inserting an annotation with many hunks above causes a scroll jump. This affects FileDiff, not CodeView.

Reproduction

Source: diffs-jump.tar.gz

The README.md includes agent notes on the alleged cause and possible solutions. I also paste it below:

Notes

Mechanism

• Changing the lineAnnotations array identity makes
  VirtualizedFileDiff.syncLineAnnotations call resetLayoutCache(),
  which wipes all measured heightDeltas for the file (here: 60
  annotation rows ≈ 28px each), even though annotations never renumber
  lines.
• The file re-renders at estimated line heights synchronously in the
  React commit (and forces a reflow mid-commit), so content above the
  viewport transiently shrinks by the forgotten corrections.
• Chromium's native scroll anchoring (overflow-anchor: auto, the
  default on the scroll container) "corrects" scrollTop against that
  transient layout. The virtualizer then re-measures only the rows near
  the new position, so nothing ever moves the scroll back.

Two independent changes each make the jump disappear, pointing at the
two halves of the bug:

• overflow-anchor: none on the scroll container — this is exactly
  what CodeView (advanced mode) does in setup(), plus its own
  anchor-capture/correction per render pass, which is why CodeView
  apps don't hit this. Simple mode never got that treatment.
• Preserving heightDeltas across annotation-only resets (they stay
  valid — annotation changes don't shift line indexes). Document edits
  (applyDocumentChange, the other reset site) need the keys remapped
  by the edit's line delta instead — the transform the editor already
  applies to annotations — plus the capturePendingLayoutAnchor
  bracket that advanced mode has and simple mode lacks, to absorb the
  residual error around the caret.

Demo

diffs-jump.mp4

[jonatanklosko]

The same issue is surfaced via a different code path when entering a newline in edit mode (also mentioned in the agent notes).

[amadeus]

Sorry for the delayed response, i've been out with a nasty illness but back and catching up.

overflow-anchor: none on the scroll container — this is exactly what CodeView (advanced mode) does in setup(), plus its own anchor-capture/correction per render pass, which is why CodeView apps don't hit this. Simple mode never got that treatment.

This is somewhat by design in that the Virtualizer tries to keep mostly hands off of the wrapper itself. But it's something we should document as a requirement because it will fight with the attempted scroll fixing.

Preserving heightDeltas across annotation-only resets (they stay valid — annotation changes don't shift line indexes). Document edits (applyDocumentChange, the other reset site) need the keys remapped by the edit's line delta instead — the transform the editor already applies to annotations — plus the capturePendingLayoutAnchor bracket that advanced mode has and simple mode lacks, to absorb the residual error around the caret.

regarding this, is this ALSO something that seemed required to fix the problem? or was it more of a one or the other type thing?

[jonatanklosko]

No worries, I hope you're doing well!

Two independent changes each make the jump disappear, pointing at the
two halves of the bug:

• overflow-anchor: none on the scroll container — this is exactly
  what CodeView (advanced mode) does in setup(), plus its own
  anchor-capture/correction per render pass, which is why CodeView
  apps don't hit this. Simple mode never got that treatment.
• Preserving heightDeltas across annotation-only resets (they stay
  valid — annotation changes don't shift line indexes). Document edits
  (applyDocumentChange, the other reset site) need the keys remapped
  by the edit's line delta instead — the transform the editor already
  applies to annotations — plus the capturePendingLayoutAnchor
  bracket that advanced mode has and simple mode lacks, to absorb the
  residual error around the caret.

It's a bit poorly worded, sorry. By "two halves of the bug" it means that both of those facts combined cause the bug, so addressing either should fix it independently. However, I think setting overflow-anchor: none is not a practical solution for the end user, since the anchoring behaviour is relevant for a lot of cases. CodeView doesn't need the anchoring since it it owns everything and fully takes care of the scroll behaviour. So an actually viable fix would (hopefully) be to do the things from point two. To clarify, it sounds as if the agent tried those changes, which iirc was not the case, it's just pointing out the relevant parts of the code.

[amadeus]

Yeah it gets a bit tricky if we allow overflow-anchor when attempting to scroll fix though. CodeView applies it purely BECAUSE it fully manages the scroll container. Virtualizer was mostly meant as a wrapper around a scroll view we don't entirely control, which makes this problem a lot harder.

We need to be able to handle scroll anchoring when dealing with our own diffs, but we also have to assume there could be other external things happening with the scroll view as well that might be out of our control. If anchoring is allowed, we can't know if we are fighting a developer's existing scroll fix or something the browser is doing (i.e. pinned to bottom scroll while agentic messages are coming in). Ideally we would never interrupt what the dev is trying to do. So not sure there's a viable path forward were we could effectively scroll anchor ourselves with the browser version enabled.

So at the very least, I think we need to add documentation about this (i'm actually surprised it never made it in, since it's always something i've assumed we had documented as a requirement for Virtualizer).

The secondary issue around resetting line cache though is something I can probably also separately look into, we might be doing something computationally a lot more aggressive than we need.

Also, a bit more tangential, but it's very helpful you are finding these edge cases. I would be curious about what sorta experiences are you trying to build? The reason is because I think contextually Virtualizer vs CodeView I've always architected as something to solve two separate problems.

CodeView was build as a very optimized, reasonably infinitely scalable code review surface. Virtualizer was intended more for scenarios like an agentic chat surface, that probably has some level of their own virtualization in place. A surface that has a lot of mixed content, some of it our File/Diff components, but also a lot of other stuff that we don't control.

Technically it can be used as a very large review surface, but as you're seeing in some of these cases, when you start stretching it to it's limits, it definitely falls apart at times.

All that is to say, knowing your longer term goals of what you're trying to build, can also help me to figure out things like messaging in our docs (when to use CodeView vs Virtualizer for example) or if there are use cases we aren't covering well and should try to optimize for.

[jonatanklosko]

Also, a bit more tangential, but it's very helpful you are finding these edge cases. I would be curious about what sorta experiences are you trying to build? The reason is because I think contextually Virtualizer vs CodeView I've always architected as something to solve two separate problems.

In our app we have a code review section, which is a list of files, however my understanding is that it strays too far from CodeView. Here's a short demo to give an idea:

diffs-code-review.mp4

So each file is its own card, and can consist of two separate diffs (unreviewed and reviewed). The file header is sticky, and then each of the subheaders (one or two) is also sticky).

[amadeus]

Is the idea here that you would have collapsible hunks essentially?

[jonatanklosko]

Not really, it's rather than a file can use two <FileDiff> components altogether. Here's the simplified shape:

<Virtualizer>
  {files.map((file) => (
    <div file-container>
      <div sticky-header>{file.name}</div>
      {file.unreviewed && (
        <div unreviewed-section>
          <div sticky-subheader>Unreviewed</div>
          <FileDiff fileDiff={file.unreviewed.fileDiff} />
        </div>
      )}
      {file.reviewed && (
        <div reviewed-section>
          <div sticky-subheader>Reviewed</div>
          <FileDiff fileDiff={file.reviewed.fileDiff} />
        </div>
      )}
    </div>
  ))}
</Virtualizer>

When a hunk is marked as reviewed, it actually causes both diffs to be recomputed.

There are a few other conditional elements that may impact layout, for example a notice box:

[amadeus]

Sorry, I don't think I was very clear.

From what i can see both in your originating video and follow up post, it looks like (and i am simplifying a bit here) you're trying to build the ability to collapse a hunk, to mark it as reviewed (this of course includes some extra custom UI and sticky positioning requirements, but for sake of discussion trying to point to the core API mismatch that we have for you at the moment).

The workarounds that you are going through right now to split that file into multiple pieces to manage the state/headers/etc is def not going to be compatible with CodeView.

Like, I don't love that you have to jump through all this extra orchestration of splitting up the file into different pieces and manage your own separate header wrapper, simply to render some custom UI and allow for a hunk to be collapsed.

But it's possible there's maybe more context that the example doesn't show? Maybe the collapsing is not based on hunks, but more so based on changed code lines or something? It does seem like you'd have to do a bunch of up front computation to pull this off, which I don't love, and would be nice to see if we could figure out a way to support this more natively in the future.

[jonatanklosko]

you're trying to build the ability to collapse a hunk, to mark it as reviewed

It's not really about collapsing, it really is two separate diffs per file.

Conceptually, for each file we have this:

• unreviewed - diff between staging and working tree: git diff -- /path/to/file.ts
• reviewed - diff between HEAD and staging: git diff --cached -- /path/to/file.ts

Then "Mark as reviewed" adds the hunk to staging and we recompute both diffs. In the straightforward case a hunk ends up "moving" from unreviewed diff to reviewed diff, though even then line numbers may not be the same.

Maybe this one gives a better idea:

diffs-code-review-2.mp4