Skip to content

fix: restore wrap anchor with paint retries #134

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
benvinegar wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix: restore wrap anchor with paint retries #134

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 36 additions & 7 deletions src/ui/components/panes/DiffPane.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,40 @@ function resolveViewportRowAnchorTop(
return 0;
}

/** Retry a layout-sensitive scroll restore across a couple of paint cycles. */
function schedulePaintRetries(callback: () => void, frames = 2) {
const cancels: Array<() => void> = [];
const requestFrame =
typeof globalThis.requestAnimationFrame === "function"
? (next: () => void) => {
const frameId = globalThis.requestAnimationFrame(() => next());
return () => {
if (typeof globalThis.cancelAnimationFrame === "function") {
globalThis.cancelAnimationFrame(frameId);
}
};
}
: (next: () => void) => {
const timeout = setTimeout(next, 16);
return () => clearTimeout(timeout);
};

const scheduleNext = (remaining: number) => {
const cancel = requestFrame(() => {
callback();
if (remaining > 1) {
scheduleNext(remaining - 1);
}
});
cancels.push(cancel);
};

scheduleNext(frames);
return () => {
cancels.splice(0).forEach((cancel) => cancel());
};
Comment on lines +123 to +153
Copy link
Copy Markdown

@greptile-apps greptile-apps bot Mar 30, 2026

Choose a reason for hiding this comment

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

P2 Stale cancel handles accumulate in the cancels array

After a frame fires, its cancel handle stays in the cancels array even though calling cancelAnimationFrame on a frame that has already executed is a harmless no-op. With frames=2:

  1. scheduleNext(2) → cancels = [cancel1]
  2. Frame 1 fires → scheduleNext(1) → cancels = [cancel1, cancel2] (cancel1 is now stale)
  3. Frame 2 fires → cancels = [cancel1, cancel2] (both stale if cleanup runs here)

The cleanup calls both, but cancel1 and cancel2 become no-ops once their respective frames have fired. This is correct in all paths but results in unnecessary no-op calls. A minor defensive improvement would be to splice the cancel out of the array once its frame has run:

const scheduleNext = (remaining: number) => {
  const cancel = requestFrame(() => {
    const idx = cancels.indexOf(cancel);
    if (idx !== -1) cancels.splice(idx, 1);
    callback();
    if (remaining > 1) {
      scheduleNext(remaining - 1);
    }
  });
  cancels.push(cancel);
};

This is low priority since cancelAnimationFrame/clearTimeout on already-fired IDs is well-defined as a no-op in all major environments.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

}

/** Render the main multi-file review stream. */
export function DiffPane({
diffContentWidth,
Expand Down Expand Up @@ -505,18 +539,13 @@ export function DiffPane({
restoreViewportAnchor();
// The wrap-toggle anchor restore should win over the usual selection-following behavior.
suppressNextSelectionAutoScrollRef.current = true;
// Retry across a couple of repaint cycles so the restored top-row anchor sticks
// after wrapped row heights and viewport culling settle.
const retryDelays = [0, 16, 48];
const timeouts = retryDelays.map((delay) => setTimeout(restoreViewportAnchor, delay));
const cancelPaintRetries = schedulePaintRetries(restoreViewportAnchor, 2);

previousWrapLinesRef.current = wrapLines;
previousSectionMetricsRef.current = sectionMetrics;
previousFilesRef.current = files;

return () => {
timeouts.forEach((timeout) => clearTimeout(timeout));
};
return cancelPaintRetries;
}
}

Expand Down
Loading