fix: prevent scroll snapback when using Space/PgUp/PgDown by benvinegar · Pull Request #105 · modem-dev/hunk

benvinegar · 2026-03-24T15:46:49Z

Summary

Fixes scroll snapback issue where manually scrolling with Space, PageUp, PageDown, or the new navigation shortcuts (d/u/f/Shift+Space) would cause the view to snap back to the selected hunk.

Root Cause

The useLayoutEffect that scrolls the selected hunk into view was firing whenever selectedEstimatedScrollTop changed. During scrolling, this metric constantly recalculates due to a dependency chain:

50ms polling updates scrollViewport.top
This triggers visibleViewportFileIds to recalculate
Which triggers visibleAgentNotesByFile
Which triggers sectionMetrics to recalculate
Which changes selectedEstimatedScrollTop
The effect runs with retry timeouts [0, 16, 48]
Those timeouts fire and snap the view back even after manual scrolling!

The Fix

Added a ref to track prevSelectedAnchorId and only auto-scroll when the selection actually changes, not when layout metrics update during free scrolling.

const isSelectionChange = prevSelectedAnchorIdRef.current !== selectedAnchorId;
prevSelectedAnchorIdRef.current = selectedAnchorId;

if (!isSelectionChange) {
  return; // Don't scroll, user is freely scrolling
}

Testing

Open a diff with agent notes visible (triggers the bug scenario)
Scroll with Space, PageUp, PageDown
Scroll with new shortcuts: d (half down), u (half up), f (page down), Shift+Space (page up)
View should stay where you scroll (no snapback)

Typecheck & Tests

✅ TypeScript typecheck passes
✅ All 205 tests pass (6 skipped)
✅ Linting passes

greptile-apps · 2026-03-24T15:50:22Z

Greptile Summary

This PR addresses two distinct concerns: (1) a scroll snapback bug where Space/PgUp/PgDown and new keyboard shortcuts would snap the view back to the selected hunk during free scrolling, and (2) exposing TTY device path and tmux pane metadata in session registration so agents can better identify which terminal a live Hunk session is running in.

Key changes:

DiffPane.tsx: Adds a prevSelectedAnchorIdRef to the useLayoutEffect responsible for auto-scrolling, so it only fires when the selection actually changes rather than on every layout metric recalculation triggered by the 50ms viewport polling cycle.
sessionRegistration.ts: Detects the TTY device path via a synchronous spawnSync("tty") call and reads TMUX_PANE from the environment at session registration time.
types.ts / daemonState.ts / commands.ts: Plumbs the new tty and tmuxPane fields through the type system and into text/JSON session list output.
SKILL.md: Clarifies that coding agents should use hunk session * subcommands rather than trying to launch the interactive TUI directly.
Tests added for both the new metadata fields and the HunkDaemonState passthrough.

Confidence Score: 4/5

Safe to merge; the snapback fix is correct and the TTY metadata changes are additive and well-tested.
The core scrollback fix is logically sound — the selection-change guard correctly prevents the retry timeouts from re-firing during free scrolling. The one nuance is that selectedEstimatedScrollTop remaining in the dependency array means React still cleans up (and thus cancels) the 48ms retry timeout when polling fires at ~50ms, which could weaken scroll reliability for the scrollChildIntoView path (agent notes / line-wrap). This is a minor concern and does not affect the common path. The TTY/tmux changes are additive, optional, and fully covered by new tests.
src/ui/components/panes/DiffPane.tsx — specifically the interaction between the retry timeouts and the selectedEstimatedScrollTop dependency.

Important Files Changed

Filename	Overview
src/ui/components/panes/DiffPane.tsx	Adds a `prevSelectedAnchorIdRef` guard to the scroll-into-view `useLayoutEffect` to only auto-scroll on real selection changes. The fix correctly stops snapback, but `selectedEstimatedScrollTop` remaining in the dependency array means React will still cancel the 48ms retry timeout when the 50ms polling cycle fires, potentially weakening the retry mechanism for the `scrollChildIntoView` code path.
src/mcp/sessionRegistration.ts	Adds TTY device path detection via a synchronous `tty` subprocess call and reads `TMUX_PANE` from the environment. Safe with appropriate null/error guards; minor improvement of checking `result.error` explicitly would be cleaner.
src/mcp/types.ts	Adds optional `tty` and `tmuxPane` fields to both `HunkSessionRegistration` and `ListedSession` interfaces. Straightforward, no issues.
src/mcp/daemonState.ts	Passes `tty` and `tmuxPane` through from registration to listed session output. Minimal, correct change.
src/session/commands.ts	Adds optional TTY and tmux pane lines to both `formatListOutput` and `formatSessionOutput` using the conditional spread pattern already used throughout the file. Clean and consistent.
test/session-commands.test.ts	Adds four tests covering text/JSON output with and without TTY/tmux metadata. Good coverage of the new fields in list and get commands.
test/session-registration.test.ts	New test file verifying that `HunkDaemonState` correctly passes `tty` and `tmuxPane` through to listed sessions. Follows existing patterns; well-structured.
skills/hunk-review/SKILL.md	Clarifies that coding agents should use `hunk session *` commands instead of invoking the interactive TUI directly. Documentation-only change, clear improvement.

Sequence Diagram

sequenceDiagram
    participant User
    participant Keyboard as Keyboard / Space / PgDn
    participant DiffPane
    participant ScrollBox
    participant PollingTimer as 50ms Polling Timer

    Note over DiffPane: prevSelectedAnchorIdRef = null

    User->>Keyboard: Navigate to hunk B
    Keyboard->>DiffPane: selectedAnchorId changes → "hunk-B"
    DiffPane->>DiffPane: isSelectionChange = true<br/>prevSelectedAnchorIdRef = "hunk-B"
    DiffPane->>ScrollBox: scrollSelectionIntoView() (immediate)
    DiffPane->>DiffPane: schedule retries [0ms, 16ms, 48ms]

    User->>Keyboard: Press Space / PgDn (free scroll)
    Keyboard->>ScrollBox: Scroll viewport down
    Note over ScrollBox: selectedAnchorId unchanged = "hunk-B"

    PollingTimer->>DiffPane: scrollViewport.top updated
    DiffPane->>DiffPane: selectedEstimatedScrollTop recalculates
    DiffPane->>DiffPane: useLayoutEffect fires<br/>isSelectionChange = false → early return
    Note over DiffPane: ✅ No snapback — view stays where user scrolled

Comments Outside Diff (1)

src/ui/components/panes/DiffPane.tsx, line 344-356 (link)

Retry timeouts can be cancelled by the 50ms polling cycle

When a real selection change occurs, the effect sets up retry timeouts at 0ms, 16ms, and 48ms. However, selectedEstimatedScrollTop is still in the dependency array, and the PR description explains the 50ms polling cycle causes it to recalculate almost immediately. React runs the previous effect's cleanup before re-running the effect — so when metrics refresh at ~50ms, the cleanup cancels any still-pending timeouts (most critically the 48ms one).

For the common path (no wrap, no notes) this is benign since it uses a direct scrollTo. But for the scrollChildIntoView path (when wrapLines is true or agent notes are visible), the 48ms retry that's supposed to handle late layout settling will reliably be cancelled by the 50ms polling tick.

A safer approach would be to track selectedEstimatedScrollTop in a ref so the effect can always read the latest value without it needing to be a dependency:

const selectedEstimatedScrollTopRef = useRef(selectedEstimatedScrollTop);
selectedEstimatedScrollTopRef.current = selectedEstimatedScrollTop;

useLayoutEffect(() => {
  // ... isSelectionChange guard ...

  const scrollSelectionIntoView = () => {
    const scrollBox = scrollRef.current;
    if (!scrollBox) return;
    const estimatedScrollTop = selectedEstimatedScrollTopRef.current;
    if (!wrapLines && visibleAgentNotesByFile.size === 0 && estimatedScrollTop !== null) {
      const topPaddingRows = Math.max(2, Math.floor(scrollViewport.height * 0.25));
      scrollBox.scrollTo(Math.max(0, estimatedScrollTop - topPaddingRows));
      return;
    }
    scrollBox.scrollChildIntoView(selectedAnchorId);
  };

  scrollSelectionIntoView();
  const retryDelays = [0, 16, 48];
  const timeouts = retryDelays.map((delay) => setTimeout(scrollSelectionIntoView, delay));
  return () => timeouts.forEach(clearTimeout);
}, [scrollRef, scrollViewport.height, selectedAnchorId, visibleAgentNotesByFile.size, wrapLines]);
// selectedEstimatedScrollTop removed — read via ref inside the callback

This eliminates the dependency on selectedEstimatedScrollTop entirely, which means the effect no longer re-runs (and no longer cancels its own retries) when metrics update mid-scroll.

_{Reviews (1): Last reviewed commit: "fix: prevent scroll snapback when using ..." | Re-trigger Greptile}

greptile-apps · 2026-03-24T15:50:29Z

src/mcp/sessionRegistration.ts

+  try {
+    const result = spawnSync("tty", [], { stdio: ["inherit", "pipe", "pipe"] });
+    const name = result.stdout?.toString().trim();
+    return name && !name.startsWith("not a tty") ? name : undefined;
+  } catch {
+    return undefined;


spawnSync result error not checked before reading stdout

spawnSync does not throw on spawn failure (e.g. tty binary not found on PATH); instead it sets result.error. The code already handles a null/undefined result.stdout via optional chaining, so there is no crash risk — but on some stripped-down environments result.stdout may be an empty Buffer (not null) even when result.error is set, which means name would be "" and the function would return undefined correctly anyway. Still, explicitly checking result.error makes the intent clearer and more robust:

Suggested change

try {

const result = spawnSync("tty", [], { stdio: ["inherit", "pipe", "pipe"] });

const name = result.stdout?.toString().trim();

return name && !name.startsWith("not a tty") ? name : undefined;

} catch {

return undefined;

try {

const result = spawnSync("tty", [], { stdio: ["inherit", "pipe", "pipe"] });

if (result.error) {

return undefined;

}

const name = result.stdout?.toString().trim();

return name && !name.startsWith("not a tty") ? name : undefined;

} catch {

return undefined;

}

The useLayoutEffect that scrolls the selected hunk into view was firing whenever selectedEstimatedScrollTop changed, which happens during normal scrolling when agent notes are visible (metrics recalculate as viewport changes). Fix: Track previous selectedAnchorId and only auto-scroll when the selection actually changes, not when layout metrics update during free scrolling. Closes scroll snapback issue with Space, PageUp, PageDown keys.

greptile-apps bot reviewed Mar 24, 2026

View reviewed changes

benvinegar force-pushed the fix-scroll-snapback branch 3 times, most recently from 3ff90b2 to 09e9f38 Compare March 24, 2026 17:43

benvinegar force-pushed the fix-scroll-snapback branch from 09e9f38 to ab92fbf Compare March 24, 2026 17:50

benvinegar merged commit 1fe977c into main Mar 24, 2026
3 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: prevent scroll snapback when using Space/PgUp/PgDown#105

fix: prevent scroll snapback when using Space/PgUp/PgDown#105
benvinegar merged 1 commit intomainfrom
fix-scroll-snapback

benvinegar commented Mar 24, 2026

Uh oh!

greptile-apps bot commented Mar 24, 2026 •

edited

Loading

Comments Outside Diff (1)

Uh oh!

greptile-apps bot Mar 24, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

benvinegar commented Mar 24, 2026

Summary

Root Cause

The Fix

Testing

Typecheck & Tests

Uh oh!

greptile-apps bot commented Mar 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Greptile Summary

Confidence Score: 4/5

Important Files Changed

Sequence Diagram

Comments Outside Diff (1)

Uh oh!

greptile-apps bot Mar 24, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

greptile-apps bot commented Mar 24, 2026 •

edited

Loading