Skip to content

Fix scrollbar click-drag on large diffs #120

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
benvinegar wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Fix scrollbar click-drag on large diffs #120

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
aefe483
Fix scrollbar click-drag on large diffs
benvinegar Mar 25, 2026
13bb7e5
Address review comments: fix division by zero and document coordinate…
benvinegar Mar 26, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
67 changes: 50 additions & 17 deletions src/ui/components/scrollbar/VerticalScrollbar.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import type { AppTheme } from "../../themes";

const HIDE_DELAY_MS = 2000;
const SCROLLBAR_WIDTH = 1;
const MIN_THUMB_HEIGHT = 2;

export interface VerticalScrollbarHandle {
show: () => void;
Expand All @@ -32,9 +33,10 @@ interface VerticalScrollbarProps {
export const VerticalScrollbar = forwardRef<VerticalScrollbarHandle, VerticalScrollbarProps>(
function VerticalScrollbar({ scrollRef, contentHeight, theme, height, onActivity }, ref) {
const [isVisible, setIsVisible] = useState(false);
const [isDragging, setIsDragging] = useState(false);
const [dragStartY, setDragStartY] = useState(0);
const [dragStartScroll, setDragStartScroll] = useState(0);
const [isDraggingState, setIsDraggingState] = useState(false);
const isDraggingRef = useRef(false);
const dragStartYRef = useRef(0);
const dragStartScrollRef = useRef(0);
const hideTimeoutRef = useRef<ReturnType<typeof setTimeout> | null>(null);

const show = useCallback(() => {
Expand All @@ -43,12 +45,12 @@ export const VerticalScrollbar = forwardRef<VerticalScrollbarHandle, VerticalScr
clearTimeout(hideTimeoutRef.current);
}
hideTimeoutRef.current = setTimeout(() => {
if (!isDragging) {
if (!isDraggingRef.current) {
setIsVisible(false);
}
}, HIDE_DELAY_MS);
onActivity?.();
}, [isDragging, onActivity]);
}, [onActivity]);

useImperativeHandle(ref, () => ({ show }), [show]);

Expand All @@ -67,7 +69,7 @@ export const VerticalScrollbar = forwardRef<VerticalScrollbarHandle, VerticalScr
// Calculate thumb metrics
const trackHeight = viewportHeight;
const scrollRatio = viewportHeight / contentHeight;
const thumbHeight = Math.max(SCROLLBAR_WIDTH, Math.floor(trackHeight * scrollRatio));
const thumbHeight = Math.max(MIN_THUMB_HEIGHT, Math.floor(trackHeight * scrollRatio));
const maxThumbY = trackHeight - thumbHeight;

const scrollTop = scrollRef.current?.scrollTop ?? 0;
Expand All @@ -79,31 +81,62 @@ export const VerticalScrollbar = forwardRef<VerticalScrollbarHandle, VerticalScr
if (event.button !== 0) return;

const currentScrollTop = scrollRef.current?.scrollTop ?? 0;
setIsDragging(true);
setDragStartY(event.y);
setDragStartScroll(currentScrollTop);
isDraggingRef.current = true;
setIsDraggingState(true);
dragStartYRef.current = event.y;
dragStartScrollRef.current = currentScrollTop;
show();
event.preventDefault();
event.stopPropagation();
};

const handleMouseDrag = (event: TuiMouseEvent) => {
if (!isDragging) return;
if (!isDraggingRef.current) {
return;
}

const deltaY = event.y - dragStartY;
const pixelsPerRow = maxThumbY / maxScroll;
const deltaY = event.y - dragStartYRef.current;
// Guard against division by zero when thumb fills track (maxThumbY = 0) or no scroll needed
const pixelsPerRow = maxThumbY > 0 && maxScroll > 0 ? maxThumbY / maxScroll : 1;
const scrollDelta = deltaY / pixelsPerRow;
const newScrollTop = Math.max(0, Math.min(maxScroll, dragStartScroll + scrollDelta));
const newScrollTop = Math.max(
0,
Math.min(maxScroll, dragStartScrollRef.current + scrollDelta),
);

scrollRef.current?.scrollTo(newScrollTop);
show();
event.preventDefault();
event.stopPropagation();
};

const handleTrackClick = (event: TuiMouseEvent) => {
if (event.button !== 0) return;

// Calculate where on the track was clicked
// Note: event.y is relative to the scrollbar container since the component
// is positioned at top: 0. If scrollbar position changes, this needs adjustment.
const clickY = event.y;

// If clicked above thumb, scroll up one viewport
// If clicked below thumb, scroll down one viewport
if (clickY < thumbY) {
const newScrollTop = Math.max(0, scrollTop - viewportHeight);
scrollRef.current?.scrollTo(newScrollTop);
} else if (clickY >= thumbY + thumbHeight) {
const newScrollTop = Math.min(maxScroll, scrollTop + viewportHeight);
scrollRef.current?.scrollTo(newScrollTop);
}
Comment on lines +119 to +129
Copy link

@greptile-apps greptile-apps bot Mar 25, 2026

Choose a reason for hiding this comment

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

P1 Potential coordinate frame mismatch

event.y is compared directly to thumbY, but thumbY is a track-relative offset (0 = top of the scrollbar track), while event.y may be an absolute terminal row coordinate.

If the scrollbar's parent container does not start at terminal row 0 (i.e. the diff viewer has a header row, or is embedded at any non-zero y offset), then clickY will be the absolute screen row while thumbY is relative to the track's top — making the above/below detection incorrect.

For example, if the scrollbar starts at absolute y=5 and the thumb is at thumbY=3 (relative):

  • Clicking at absolute y=7 (which is above the thumb at absolute y=8) gives 7 < 3 → false, so no scroll-up happens.

The drag handlers avoid this by using deltas (event.y - dragStartYRef.current), which cancel out any absolute offset. The track-click handler should do the same, or derive an element-relative y from the event.

Suggested change
const clickY = event.y;
// If clicked above thumb, scroll up one viewport
// If clicked below thumb, scroll down one viewport
if (clickY < thumbY) {
const newScrollTop = Math.max(0, scrollTop - viewportHeight);
scrollRef.current?.scrollTo(newScrollTop);
} else if (clickY >= thumbY + thumbHeight) {
const newScrollTop = Math.min(maxScroll, scrollTop + viewportHeight);
scrollRef.current?.scrollTo(newScrollTop);
}
const handleTrackClick = (event: TuiMouseEvent) => {
if (event.button !== 0) return;
// Use delta relative to the drag-start reference to stay in the same
// coordinate frame as thumbY (track-relative).
// For a simple click there is no prior mousedown, so we compute the
// click position relative to the track top via the scroll element.
const trackTop = scrollRef.current ? 0 : 0; // placeholder – derive from layout if available
const clickY = event.y - trackTop;
if (clickY < thumbY) {
const newScrollTop = Math.max(0, scrollTop - viewportHeight);
scrollRef.current?.scrollTo(newScrollTop);
} else if (clickY >= thumbY + thumbHeight) {
const newScrollTop = Math.min(maxScroll, scrollTop + viewportHeight);
scrollRef.current?.scrollTo(newScrollTop);
}
show();
event.preventDefault();
event.stopPropagation();
};

At minimum, add a comment documenting that this assumes the track's absolute y origin equals 0 (i.e. top: 0 is always relative to the terminal top), so a future layout change doesn't silently break track-click behaviour.


show();
event.preventDefault();
event.stopPropagation();
};

const handleMouseUp = (event?: TuiMouseEvent) => {
if (!isDragging) return;
setIsDragging(false);
if (!isDraggingRef.current) return;
isDraggingRef.current = false;
setIsDraggingState(false);
// Restart hide timer
if (hideTimeoutRef.current) {
clearTimeout(hideTimeoutRef.current);
Expand All @@ -128,7 +161,6 @@ export const VerticalScrollbar = forwardRef<VerticalScrollbarHandle, VerticalScr
width: SCROLLBAR_WIDTH,
height: trackHeight,
backgroundColor: theme.panel,
zIndex: 10,
}}
>
{/* Track background */}
Expand All @@ -141,6 +173,7 @@ export const VerticalScrollbar = forwardRef<VerticalScrollbarHandle, VerticalScr
height: trackHeight,
backgroundColor: theme.border,
}}
onMouseDown={handleTrackClick}
/>
{/* Thumb */}
<box
Expand All @@ -150,7 +183,7 @@ export const VerticalScrollbar = forwardRef<VerticalScrollbarHandle, VerticalScr
left: 0,
width: SCROLLBAR_WIDTH,
height: thumbHeight,
backgroundColor: isDragging ? theme.accent : theme.accentMuted,
backgroundColor: isDraggingState ? theme.accent : theme.accentMuted,
}}
onMouseDown={handleMouseDown}
onMouseDrag={handleMouseDrag}
Expand Down
208 changes: 208 additions & 0 deletions test/vertical-scrollbar.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -299,4 +299,212 @@ describe("Vertical scrollbar", () => {
});
}
});

test("thumb drag scrolls content", async () => {
// Create a file with many lines to ensure scrolling
const before = Array.from({ length: 100 }, (_, j) => `line${j + 1}`).join("\n");
const after = before.replace("line50", "line50modified");

const bootstrap: AppBootstrap = {
input: {
kind: "git",
staged: false,
options: { mode: "split" },
},
changeset: {
id: "drag-test",
sourceLabel: "repo",
title: "drag test",
files: [createDiffFile("drag", "src/drag.ts", before, after)],
},
initialMode: "split",
initialTheme: "midnight",
};

const setup = await testRender(<App bootstrap={bootstrap} />, {
width: 160,
height: 20, // Small viewport to force scrolling
});

try {
await flush(setup);
await act(async () => {
await Bun.sleep(100);
});

// Get initial frame - app centers on the hunk at line 50
const frame1 = setup.captureCharFrame();
expect(frame1).toContain("line50");

// Drag scrollbar thumb down (rightmost column is scrollbar at x=159, y ranges 0-19)
// Thumb should be at some position, drag it down to scroll
await act(async () => {
// Drag from top area of scrollbar down
await setup.mockMouse.drag(159, 2, 159, 10);
await flush(setup);
await Bun.sleep(100);
});

// After dragging down, we should see different content
const frame2 = setup.captureCharFrame();
expect(frame2).toBeTruthy();

// Drag back up
await act(async () => {
await setup.mockMouse.drag(159, 10, 159, 2);
await flush(setup);
await Bun.sleep(100);
});

const frame3 = setup.captureCharFrame();
expect(frame3).toBeTruthy();
} finally {
await act(async () => {
setup.renderer.destroy();
});
}
});

test("track click scrolls by one viewport", async () => {
// Create a file with many lines to ensure scrolling
const lines = Array.from({ length: 80 }, (_, j) => `line${String(j + 1).padStart(3, "0")}`);
const before = lines.join("\n");
const after = before.replace("line040", "line040modified");

const bootstrap: AppBootstrap = {
input: {
kind: "git",
staged: false,
options: { mode: "split" },
},
changeset: {
id: "track-click-test",
sourceLabel: "repo",
title: "track click test",
files: [createDiffFile("track", "src/track.ts", before, after)],
},
initialMode: "split",
initialTheme: "midnight",
};

const setup = await testRender(<App bootstrap={bootstrap} />, {
width: 160,
height: 15, // Viewport of 15 lines
});

try {
await flush(setup);
await act(async () => {
await Bun.sleep(100);
});

// Get initial content - app centers on the hunk at line 40
const frame1 = setup.captureCharFrame();
expect(frame1).toContain("line040");

// First scroll down a bit to make scrollbar visible and move thumb down
await act(async () => {
for (let i = 0; i < 5; i++) {
await setup.mockInput.pressArrow("down");
}
await flush(setup);
await Bun.sleep(100);
});

// Click on scrollbar track below thumb to page down
// Scrollbar is at rightmost column (x=159), click near bottom
await act(async () => {
await setup.mockMouse.click(159, 12);
await flush(setup);
await Bun.sleep(100);
});

const frame2 = setup.captureCharFrame();
// Should have scrolled down further after track click
expect(frame2).toBeTruthy();

// Click on scrollbar track above thumb to page up
await act(async () => {
await setup.mockMouse.click(159, 2);
await flush(setup);
await Bun.sleep(100);
});

const frame3 = setup.captureCharFrame();
// Should have scrolled back up
expect(frame3).toBeTruthy();
} finally {
await act(async () => {
setup.renderer.destroy();
});
}
});

test("handles edge case when content barely exceeds viewport", async () => {
// Create content that's just slightly larger than viewport
// This tests the division-by-zero guard in drag calculations
// Use the same pattern as other tests which work correctly
const before = Array.from(
{ length: 25 },
(_, j) => `export const line${String(j + 1).padStart(2, "0")} = ${j + 1};`,
).join("\n");
const after = before.replace("line08 = 8;", "line08 = 999; // modified");

const bootstrap: AppBootstrap = {
input: {
kind: "git",
staged: false,
options: { mode: "split" },
},
changeset: {
id: "edge-case-test",
sourceLabel: "repo",
title: "edge case test",
files: [createDiffFile("edge", "src/edge.ts", before, after)],
},
initialMode: "split",
initialTheme: "midnight",
};

const setup = await testRender(<App bootstrap={bootstrap} />, {
width: 160,
height: 15, // Small viewport to force scrolling (25 lines of content in 15-line viewport)
});

try {
await flush(setup);
await act(async () => {
await Bun.sleep(100);
});

// Verify app renders with the hunk visible - look for the modified line
const frame1 = setup.captureCharFrame();
expect(frame1).toContain("line08");

// Try to drag - should not crash with division by zero
await act(async () => {
await setup.mockMouse.drag(159, 0, 159, 5);
await flush(setup);
await Bun.sleep(100);
});

// App should still be responsive after drag attempt
const frame2 = setup.captureCharFrame();
expect(frame2).toBeTruthy();

// Try track click - should not crash
await act(async () => {
await setup.mockMouse.click(159, 10);
await flush(setup);
await Bun.sleep(100);
});

const frame3 = setup.captureCharFrame();
expect(frame3).toBeTruthy();
} finally {
await act(async () => {
setup.renderer.destroy();
});
}
});
});
Loading