Skip to content

fix(ui): coalesce viewport listener via microtask to avoid setState loop#242

Open
aldevv wants to merge 1 commit intomodem-dev:mainfrom
aldevv:fix/viewport-update-loop
Open

fix(ui): coalesce viewport listener via microtask to avoid setState loop#242
aldevv wants to merge 1 commit intomodem-dev:mainfrom
aldevv:fix/viewport-update-loop

Conversation

@aldevv
Copy link
Copy Markdown

@aldevv aldevv commented May 8, 2026

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)

  1. hunk diff <range> on a multi-file diff with several hunks.
  2. Optional but reliable: attach a couple of agent comments via hunk session comment apply.
  3. Hold ] (next hunk) or scroll continuously for a few seconds.
  4. The TUI exits with Error: Maximum update depth exceeded and a stack trace ending at updateSliderFromScrollState → onChange → handleViewportChange → updateViewport → setState.

Fix: the scrollbar change listener now coalesces events through queueMicrotask, so the resulting setScrollViewport call 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.

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-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented May 8, 2026

Greptile Summary

This PR fixes a "Maximum update depth exceeded" React render loop in DiffPane by coalescing OpenTUI change and viewport events into a single microtask-deferred readViewport call, preventing setScrollViewport from dispatching inside React's commit phase.

  • The scheduled flag collapses multiple synchronous event firings into one microtask per paint cycle, while cancelled guards against stale reads after the effect's cleanup runs.
  • The initial synchronous readViewport() call is preserved so first-paint geometry is still correct.

Confidence Score: 4/5

Safe to merge — the coalescing logic is correct and the root-cause analysis matches the described crash.

The scheduled/cancelled flags are managed correctly across effect re-runs and cleanup. The only observation is a minor flag-reset ordering inconsistency that has no practical impact since listeners are always removed before the cancelled path can be reached.

No files require special attention beyond DiffPane.tsx, which contains the entire change.

Important Files Changed

Filename Overview
src/ui/components/panes/DiffPane.tsx Adds microtask coalescing to the viewport-change listener to avoid setState-during-commit render loops; logic is correct with a minor ordering note on scheduled = false.

Sequence Diagram

sequenceDiagram
    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
Loading
Prompt To Fix All With AI
Fix 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

Comment on lines +359 to +365
queueMicrotask(() => {
scheduled = false;
if (cancelled) {
return;
}
readViewport();
});
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P2 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.

Suggested change
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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Maximum update depth exceeded under rapid viewport changes (scroll or hunk-jump)

1 participant