Skip to content

Commit 5d7c231

Browse files
aldevvbenvinegar
andauthored
fix(ui): coalesce viewport listener via microtask to avoid setState loop (#242)
Co-authored-by: Ben Vinegar <ben@benv.ca>
1 parent 83ef596 commit 5d7c231

2 files changed

Lines changed: 122 additions & 3 deletions

File tree

src/ui/AppHost.interactions.test.tsx

Lines changed: 93 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -283,6 +283,46 @@ function createCrossFileHunkNavigationBootstrap(): AppBootstrap {
283283
});
284284
}
285285

286+
/** Build the issue #233 stress fixture: many files, separated hunks, and visible notes. */
287+
function createRapidViewportLoopBootstrap(): AppBootstrap {
288+
const files = Array.from({ length: 10 }, (_, index) => {
289+
const fileIndex = index + 1;
290+
const start = fileIndex * 100 + 1;
291+
const beforeLines = createNumberedAssignmentLines(start, 90);
292+
const afterLines = [...beforeLines];
293+
294+
afterLines[0] = `export const line${String(start).padStart(2, "0")} = ${start + 1000};`;
295+
afterLines[30] = `export const line${String(start + 30).padStart(2, "0")} = ${start + 3000};`;
296+
afterLines[60] = `export const line${String(start + 60).padStart(2, "0")} = ${start + 6000};`;
297+
298+
const file = buildTestDiffFile({
299+
id: `rapid-${fileIndex}`,
300+
path: `rapid-${fileIndex}.ts`,
301+
before: lines(...beforeLines),
302+
after: lines(...afterLines),
303+
context: 3,
304+
});
305+
file.agent = {
306+
path: file.path,
307+
summary: `rapid ${fileIndex}`,
308+
annotations: [
309+
{ newRange: [start, start], summary: `note start ${fileIndex}` },
310+
{ newRange: [start + 30, start + 30], summary: `note middle ${fileIndex}` },
311+
{ newRange: [start + 60, start + 60], summary: `note late ${fileIndex}` },
312+
],
313+
};
314+
return file;
315+
});
316+
317+
return createTestVcsAppBootstrap({
318+
changesetId: "changeset:rapid-viewport",
319+
files,
320+
vcsOptions: { mode: "stack", agentNotes: true },
321+
initialMode: "stack",
322+
initialShowAgentNotes: true,
323+
});
324+
}
325+
286326
function createMouseScrollSelectionBootstrap(): AppBootstrap {
287327
const firstBeforeLines = createNumberedAssignmentLines(1, 12);
288328
const secondBeforeLines = Array.from(
@@ -442,6 +482,59 @@ function firstVisibleAddedLineNumber(frame: string) {
442482
}
443483

444484
describe("App interactions", () => {
485+
test("rapid hunk navigation and wheel scrolling do not recurse through viewport updates", async () => {
486+
const updateDepthErrors: string[] = [];
487+
const originalError = console.error;
488+
console.error = (...args: unknown[]) => {
489+
if (args.some((arg) => String(arg).includes("Maximum update depth exceeded"))) {
490+
updateDepthErrors.push(args.map(String).join(" "));
491+
}
492+
originalError(...args);
493+
};
494+
495+
const setup = await testRender(<AppHost bootstrap={createRapidViewportLoopBootstrap()} />, {
496+
width: 220,
497+
height: 12,
498+
});
499+
500+
try {
501+
await flush(setup);
502+
await flush(setup);
503+
await flush(setup);
504+
await flush(setup);
505+
506+
// Regression coverage for issue #233 / PR #242. This intentionally combines the inputs
507+
// that made the old React/OpenTUI feedback loop reproducible: stack layout, many hunks,
508+
// visible agent notes, repeated next-hunk jumps, and bursty wheel scrolling.
509+
for (let batch = 0; batch < 2; batch += 1) {
510+
await act(async () => {
511+
for (let index = 0; index < 6; index += 1) {
512+
await setup.mockInput.typeText("]");
513+
}
514+
});
515+
await flush(setup);
516+
await flush(setup);
517+
}
518+
519+
for (let batch = 0; batch < 2; batch += 1) {
520+
await act(async () => {
521+
for (let index = 0; index < 4; index += 1) {
522+
await setup.mockMouse.scroll(120, 7, "down");
523+
}
524+
});
525+
await flush(setup);
526+
await flush(setup);
527+
}
528+
529+
expect(updateDepthErrors).toEqual([]);
530+
} finally {
531+
console.error = originalError;
532+
await act(async () => {
533+
setup.renderer.destroy();
534+
});
535+
}
536+
}, 20_000);
537+
445538
test("keyboard shortcuts toggle notes, line numbers, and hunk metadata", async () => {
446539
const setup = await testRender(<AppHost bootstrap={createSingleFileBootstrap()} />, {
447540
width: 240,

src/ui/components/panes/DiffPane.tsx

Lines changed: 29 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -325,7 +325,10 @@ export function DiffPane({
325325
return;
326326
}
327327

328-
const updateViewport = () => {
328+
let cancelled = false;
329+
let scheduled = false;
330+
331+
const readViewport = () => {
329332
const nextTop = scrollBox.scrollTop ?? 0;
330333
const nextHeight = scrollBox.viewport.height ?? 0;
331334

@@ -342,16 +345,39 @@ export function DiffPane({
342345
);
343346
};
344347

348+
// OpenTUI emits `change` synchronously from inside its own slider sync, and other
349+
// useLayoutEffects in this pane scroll the box from inside React's commit phase.
350+
// Calling setScrollViewport directly from the listener can run setState while React
351+
// is already committing — which downstream layout effects can amplify into a render
352+
// loop and trip React's max-update-depth guard. Coalesce listener events into a
353+
// single microtask-deferred read so the setState is dispatched outside the emit
354+
// call stack and repeated events between paints collapse into one update.
345355
const handleViewportChange = () => {
346-
updateViewport();
356+
if (scheduled) {
357+
return;
358+
}
359+
scheduled = true;
360+
queueMicrotask(() => {
361+
if (cancelled) {
362+
scheduled = false;
363+
return;
364+
}
365+
366+
try {
367+
readViewport();
368+
} finally {
369+
scheduled = false;
370+
}
371+
});
347372
};
348373

349-
updateViewport();
374+
readViewport();
350375
scrollBox.verticalScrollBar.on("change", handleViewportChange);
351376
scrollBox.viewport.on("layout-changed", handleViewportChange);
352377
scrollBox.viewport.on("resized", handleViewportChange);
353378

354379
return () => {
380+
cancelled = true;
355381
scrollBox.verticalScrollBar.off("change", handleViewportChange);
356382
scrollBox.viewport.off("layout-changed", handleViewportChange);
357383
scrollBox.viewport.off("resized", handleViewportChange);

0 commit comments

Comments
 (0)