Skip to content

fix(terminal): preserve viewport across output and restore#541

Open
snvtac wants to merge 1 commit into
cline:mainfrom
snvtac:snvtac/539-terminal-viewport-preservation
Open

fix(terminal): preserve viewport across output and restore#541
snvtac wants to merge 1 commit into
cline:mainfrom
snvtac:snvtac/539-terminal-viewport-preservation

Conversation

@snvtac

@snvtac snvtac commented Jun 25, 2026

Copy link
Copy Markdown

Summary

  • preserve the xterm viewport around terminal output writes, restore replay/reset, and fit/resize operations
  • keep bottom-pinned terminals pinned to the new bottom, while restoring scrolled-back terminals to their previous line with clamping
  • skip stale viewport restoration when the user scrolls or types during an async write/replay
  • add focused terminal manager coverage for output, restore replay, resize clamping, bottom-pinned output, and async user-interaction races

Validation

  • npm run check
  • npm --prefix web-ui run test
  • npm run build

Fixes #539

@greptile-apps

greptile-apps Bot commented Jun 25, 2026

Copy link
Copy Markdown

Greptile Summary

This PR adds viewport preservation to PersistentTerminal so that users scrolled back in the terminal history are not abruptly snapped to the bottom after new output arrives, a restore replay, or a container resize. A monotonic sequence counter (userViewportInteractionSequence) is used to detect user scroll/key interactions during async writes, ensuring that programmatic viewport restoration is skipped when the user's own actions have already changed the scroll position.

  • A TerminalViewportSnapshot (position + "at-bottom" flag + interaction sequence) is captured before every terminal.write, fitAddon.fit, and terminal.reset call, then restored in the async callback with clamping to the new buffer bounds.
  • A set of DOM event listeners (wheel, touchmove, pointerdown on .xterm-viewport) and a keydown hook in attachCustomKeyEventHandler increment the sequence counter to mark user-driven viewport changes that should not be overridden.
  • New focused tests cover output writes, restore replay, resize clamping, bottom-pinned behaviour, and async user-interaction races.

Confidence Score: 4/5

The change is well-scoped and safe to merge; the viewport-preservation logic is correct and the new tests validate all key scenarios including async user-interaction races.

The sequence-counter mechanism, capture/restore placement, and clamping math all hold up under careful tracing of async write, restore, and resize paths. The only flag is a non-passive pointerdown listener — a minor performance annotation missing that has no functional effect.

No files require special attention; the test file and implementation are consistent and well-matched.

Important Files Changed

Filename Overview
web-ui/src/terminal/persistent-terminal-manager.ts Adds viewport capture/restore around all async write, resize, and restore operations; logic is correct with one minor non-passive event listener.
web-ui/src/terminal/persistent-terminal-manager.test.ts New test file with thorough coverage of the viewport-preservation scenarios including async race conditions and resize clamping.

Sequence Diagram

%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
    participant IO as IO WebSocket
    participant PTM as PersistentTerminal
    participant XTerm as xterm Terminal
    participant User as User Input

    Note over PTM: enqueueTerminalWrite (normal output)
    IO->>PTM: message (data)
    PTM->>PTM: captureViewport() → snapshot
    PTM->>XTerm: terminal.write(data, callback)
    User-->>PTM: wheel/touchmove/keydown → seq++
    XTerm-->>PTM: write callback fires
    PTM->>PTM: restoreViewport(snapshot) — seq mismatch skips, else scrollToBottom or scrollToLine(clamped)

    Note over PTM: applyRestore (control restore message)
    PTM->>PTM: captureViewport() BEFORE reset → snapshot
    PTM->>XTerm: terminal.reset()
    PTM->>XTerm: terminal.resize(cols, rows)
    PTM->>XTerm: "terminal.write(snapshot, callback) preserveViewport=false"
    XTerm-->>PTM: write callback fires (no viewport restore)
    PTM->>PTM: restoreViewport(snapshot) — seq check guards against user interaction during replay

    Note over PTM: requestResize (ResizeObserver / mount RAF)
    PTM->>PTM: captureViewport() → snapshot
    PTM->>XTerm: fitAddon.fit() → baseY may shrink
    PTM->>PTM: restoreViewport(snapshot) — clamps to new baseY
Loading
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
    participant IO as IO WebSocket
    participant PTM as PersistentTerminal
    participant XTerm as xterm Terminal
    participant User as User Input

    Note over PTM: enqueueTerminalWrite (normal output)
    IO->>PTM: message (data)
    PTM->>PTM: captureViewport() → snapshot
    PTM->>XTerm: terminal.write(data, callback)
    User-->>PTM: wheel/touchmove/keydown → seq++
    XTerm-->>PTM: write callback fires
    PTM->>PTM: restoreViewport(snapshot) — seq mismatch skips, else scrollToBottom or scrollToLine(clamped)

    Note over PTM: applyRestore (control restore message)
    PTM->>PTM: captureViewport() BEFORE reset → snapshot
    PTM->>XTerm: terminal.reset()
    PTM->>XTerm: terminal.resize(cols, rows)
    PTM->>XTerm: "terminal.write(snapshot, callback) preserveViewport=false"
    XTerm-->>PTM: write callback fires (no viewport restore)
    PTM->>PTM: restoreViewport(snapshot) — seq check guards against user interaction during replay

    Note over PTM: requestResize (ResizeObserver / mount RAF)
    PTM->>PTM: captureViewport() → snapshot
    PTM->>XTerm: fitAddon.fit() → baseY may shrink
    PTM->>PTM: restoreViewport(snapshot) — clamps to new baseY
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
web-ui/src/terminal/persistent-terminal-manager.ts:219
The `pointerdown` listener for scrollbar detection is not marked `passive`. Unlike the `wheel` and `touchmove` listeners (which correctly use `passive: true`), omitting it here forces the browser to wait for the handler to return before dispatching scroll initiation, which can introduce input latency. Since `trackScrollbarViewportInteraction` never calls `event.preventDefault()`, marking it passive is safe.

```suggestion
		this.hostElement.addEventListener("pointerdown", this.trackScrollbarViewportInteraction, { capture: true, passive: true });
```

Reviews (1): Last reviewed commit: "fix(terminal): preserve viewport across ..." | Re-trigger Greptile

capture: true,
passive: true,
});
this.hostElement.addEventListener("pointerdown", this.trackScrollbarViewportInteraction, { capture: true });

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 The pointerdown listener for scrollbar detection is not marked passive. Unlike the wheel and touchmove listeners (which correctly use passive: true), omitting it here forces the browser to wait for the handler to return before dispatching scroll initiation, which can introduce input latency. Since trackScrollbarViewportInteraction never calls event.preventDefault(), marking it passive is safe.

Suggested change
this.hostElement.addEventListener("pointerdown", this.trackScrollbarViewportInteraction, { capture: true });
this.hostElement.addEventListener("pointerdown", this.trackScrollbarViewportInteraction, { capture: true, passive: true });
Prompt To Fix With AI
This is a comment left during a code review.
Path: web-ui/src/terminal/persistent-terminal-manager.ts
Line: 219

Comment:
The `pointerdown` listener for scrollbar detection is not marked `passive`. Unlike the `wheel` and `touchmove` listeners (which correctly use `passive: true`), omitting it here forces the browser to wait for the handler to return before dispatching scroll initiation, which can introduce input latency. Since `trackScrollbarViewportInteraction` never calls `event.preventDefault()`, marking it passive is safe.

```suggestion
		this.hostElement.addEventListener("pointerdown", this.trackScrollbarViewportInteraction, { capture: true, passive: true });
```

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.

Terminal viewport jumps when scrolled back after agent output / restore / resize

1 participant