Conversation
|
@liamhess is attempting to deploy a commit to the General Action Team on Vercel. A member of the Team first needs to authorize it. |
|
Hey @liamhess, thanks for taking this on. Is this already ready for review? |
It is now @arnestrickmann, looking forward to your review:) |
Greptile SummaryThis PR adds wheel-event forwarding so that scrolling in the empty whitespace on either side of the terminal pane (visible when the viewport is wider than Key changes:
Two minor polish issues were identified:
Confidence Score: 4/5
|
| Filename | Overview |
|---|---|
| src/renderer/hooks/useTerminalViewportWheelForwarding.ts | New hook that intercepts wheel events on the whitespace area surrounding the terminal and forwards them to the terminal's scroll API; guards against Ctrl+scroll, horizontal scrolling, and events already targeting the terminal container. |
| src/renderer/terminal/TerminalSessionManager.ts | Adds public scrollViewportFromWheelDelta with sub-line accumulation (wheelLineRemainder), pixel-to-line normalization, and DOM-clamped scrolling via xterm's scrollToLine; one edge case where the remainder is not cleared on direction reversal. |
| src/renderer/components/TerminalPane.tsx | Exports the new TerminalPaneHandle type (adds scrollViewportFromWheelDelta alongside existing focus) and wires it through useImperativeHandle; clean and backward-compatible. |
| src/renderer/components/ChatInterface.tsx | Updates terminalRef to TerminalPaneHandle, creates the forwarding handler, and attaches it via onWheelCapture on the terminal wrapper div — straightforward integration. |
| src/renderer/components/MultiAgentTask.tsx | Same TerminalPaneHandle + wheel-forwarding integration as ChatInterface; the single handler is correctly shared across variant containers since only the active variant can receive pointer events (visibility: hidden on inactive ones). |
Sequence Diagram
sequenceDiagram
participant User as User (whitespace scroll)
participant WrapperDiv as Wrapper Div (onWheelCapture)
participant Hook as useTerminalViewportWheelForwarding
participant TerminalPane as TerminalPane (ref)
participant TSM as TerminalSessionManager
participant XTerm as xterm.js
User->>WrapperDiv: WheelEvent (deltaY, deltaMode)
WrapperDiv->>Hook: onWheelCapture fires (capture phase)
Hook->>Hook: Guard: ctrlKey / deltaX dominant / data-terminal-container?
alt target inside [data-terminal-container]
Hook-->>WrapperDiv: return (xterm handles natively)
else whitespace area
Hook->>TerminalPane: scrollViewportFromWheelDelta(deltaY, deltaMode)
TerminalPane->>TSM: scrollViewportFromWheelDelta(deltaY, deltaMode)
TSM->>TSM: normalizeWheelDeltaToLines → lineDelta
TSM->>TSM: accumulate wheelLineRemainder → wholeLines
TSM->>TSM: scrollViewportByLineDelta(wholeLines)
TSM->>XTerm: terminal.scrollToLine(targetLine)
XTerm-->>TSM: done
TSM-->>TerminalPane: true (scrolled)
TerminalPane-->>Hook: true
Hook->>WrapperDiv: event.preventDefault()
end
Last reviewed commit: 038e95b
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const totalDelta = this.wheelLineRemainder + lineDelta; | ||
| const wholeLines = totalDelta > 0 ? Math.floor(totalDelta) : Math.ceil(totalDelta); | ||
| this.wheelLineRemainder = totalDelta - wholeLines; |
There was a problem hiding this comment.
Accumulated remainder not cleared on direction reversal
wheelLineRemainder is a running carry-over value that is never reset when the scroll direction changes. This means after a sustained scroll in one direction that stops on a fractional boundary, reversing direction will silently "burn through" up to one line's worth of accumulated carry before the terminal actually moves the other way.
For example, if wheelLineRemainder = 0.85 (scrolling down) and the user immediately starts scrolling up with a single-line pixel delta of -0.15:
totalDelta = 0.85 + (-0.15) = 0.70 → wholeLines = 0 (no movement)
The terminal won't respond until the carry is fully cancelled. Consider resetting the remainder when the incoming lineDelta has the opposite sign to wheelLineRemainder:
const totalDelta = this.wheelLineRemainder + lineDelta;could become:
// Reset carry on direction reversal to avoid an invisible "dead zone"
if ((this.wheelLineRemainder > 0 && lineDelta < 0) || (this.wheelLineRemainder < 0 && lineDelta > 0)) {
this.wheelLineRemainder = 0;
}
const totalDelta = this.wheelLineRemainder + lineDelta;
This is a UX polish issue rather than a correctness bug, but it can feel noticeably sluggish on trackpads where sub-line scroll events are very common.
| if (!Number.isFinite(event.deltaY) || event.deltaY === 0) return; | ||
| if (event.ctrlKey || Math.abs(event.deltaX) > Math.abs(event.deltaY)) return; |
There was a problem hiding this comment.
metaKey (Cmd+scroll) not guarded against
The hook correctly short-circuits for ctrlKey (pinch-zoom on some platforms) and for dominant-horizontal swipes, but metaKey (Cmd+scroll) is not checked. On macOS inside Electron, Cmd+scroll is sometimes used for zoom or OS-level gestures. Forwarding those events to the terminal scroll path will produce unexpected behaviour.
| if (!Number.isFinite(event.deltaY) || event.deltaY === 0) return; | |
| if (event.ctrlKey || Math.abs(event.deltaX) > Math.abs(event.deltaY)) return; | |
| if (event.ctrlKey || event.metaKey || Math.abs(event.deltaX) > Math.abs(event.deltaY)) return; |
2 participants
Summary
On wider screens the width of the chat interface is limited and then scrolling only works in the middle where the pty is actually rendered. This is a bit unintuitive, because visually there's no border/separation between the whitespace and the pty. Generally I think the nice fix would be to allow scrolling in the whitespace. But I understand if it's too much code / too hacky to forward the scroll for just a little usability improvement
Type of change
Mandatory Tasks
Checklist
pnpm run format)pnpm run lint)