fix(ui): coalesce viewport listener via microtask to avoid setState loop#242
fix(ui): coalesce viewport listener via microtask to avoid setState loop#242aldevv wants to merge 1 commit intomodem-dev:mainfrom
Conversation
OpenTUI emits 'change' synchronously from its slider sync. Combined with this pane's many useLayoutEffects that scroll the box from inside React's commit phase, calling setScrollViewport directly from the listener can run setState while React is committing, which downstream layout effects amplify into a render loop and trip React's max-update-depth guard. Coalesce listener events into a single microtask-deferred read so the setState is dispatched outside the emit call stack and repeated events between paints collapse into one update. Fixes modem-dev#233.
Greptile SummaryThis PR fixes a "Maximum update depth exceeded" React render loop in
Confidence Score: 4/5Safe to merge — the coalescing logic is correct and the root-cause analysis matches the described crash. The No files require special attention beyond Important Files Changed
Sequence DiagramsequenceDiagram
participant OT as OpenTUI scrollBar
participant HL as handleViewportChange
participant MT as queueMicrotask
participant RV as readViewport
participant R as React (setScrollViewport)
Note over OT,R: Before fix — setState during commit
OT->>HL: emit "change" (synchronously, inside commit)
HL->>RV: readViewport() [direct call]
RV->>R: setScrollViewport() — setState mid-commit — render loop
Note over OT,R: After fix — coalesced microtask
OT->>HL: emit "change" (event 1)
HL->>MT: "scheduled=true, queueMicrotask(fn)"
OT->>HL: emit "change" (event 2)
HL-->>HL: "scheduled=true, return (dropped)"
Note over MT: microtask checkpoint
MT->>RV: "scheduled=false, cancelled check, readViewport()"
RV->>R: setScrollViewport() — outside commit phase
Prompt To Fix All With AIFix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
src/ui/components/panes/DiffPane.tsx:359-365
The `scheduled = false` reset happens unconditionally before the `cancelled` guard. If `readViewport()` throws (e.g., the scrollbox is torn down between the microtask being queued and it running), `scheduled` is already `false` and a new event could queue a fresh microtask before the thrown exception propagates. Resetting `scheduled` only on the non-cancelled branch makes the intent clearer and keeps the flag consistent through the full life of the microtask.
```suggestion
queueMicrotask(() => {
if (cancelled) {
scheduled = false;
return;
}
scheduled = false;
readViewport();
});
```
Reviews (1): Last reviewed commit: "fix(ui): coalesce viewport listener via ..." | Re-trigger Greptile |
| queueMicrotask(() => { | ||
| scheduled = false; | ||
| if (cancelled) { | ||
| return; | ||
| } | ||
| readViewport(); | ||
| }); |
There was a problem hiding this comment.
The
scheduled = false reset happens unconditionally before the cancelled guard. If readViewport() throws (e.g., the scrollbox is torn down between the microtask being queued and it running), scheduled is already false and a new event could queue a fresh microtask before the thrown exception propagates. Resetting scheduled only on the non-cancelled branch makes the intent clearer and keeps the flag consistent through the full life of the microtask.
| queueMicrotask(() => { | |
| scheduled = false; | |
| if (cancelled) { | |
| return; | |
| } | |
| readViewport(); | |
| }); | |
| queueMicrotask(() => { | |
| if (cancelled) { | |
| scheduled = false; | |
| return; | |
| } | |
| scheduled = false; | |
| readViewport(); | |
| }); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/ui/components/panes/DiffPane.tsx
Line: 359-365
Comment:
The `scheduled = false` reset happens unconditionally before the `cancelled` guard. If `readViewport()` throws (e.g., the scrollbox is torn down between the microtask being queued and it running), `scheduled` is already `false` and a new event could queue a fresh microtask before the thrown exception propagates. Resetting `scheduled` only on the non-cancelled branch makes the intent clearer and keeps the flag consistent through the full life of the microtask.
```suggestion
queueMicrotask(() => {
if (cancelled) {
scheduled = false;
return;
}
scheduled = false;
readViewport();
});
```
How can I resolve this? If you propose a fix, please make it concise.
User-visible: the TUI crashes with React's "Maximum update depth exceeded" when scrolling fast or rapidly pressing next/prev hunk, especially with comments attached.
Steps to reproduce (on
main)hunk diff <range>on a multi-file diff with several hunks.hunk session comment apply.](next hunk) or scroll continuously for a few seconds.Error: Maximum update depth exceededand a stack trace ending atupdateSliderFromScrollState → onChange → handleViewportChange → updateViewport → setState.Fix: the scrollbar
changelistener now coalesces events throughqueueMicrotask, so the resultingsetScrollViewportcall isn't dispatched while a layout effect is already mid-commit. Initial mount still reads the viewport synchronously.No user-facing docs or workflows change.
Fixes #233.