Skip to content

Commit cabff6a

Browse files
benvinegarbenvinegar
authored
Fix scrollbar click-drag on large diffs (#120)
1 parent ecdace9 commit cabff6a

2 files changed

Lines changed: 258 additions & 17 deletions

File tree

‎src/ui/components/scrollbar/VerticalScrollbar.tsx‎

Lines changed: 50 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ import type { AppTheme } from "../../themes";
1212

1313
const HIDE_DELAY_MS = 2000;
1414
const SCROLLBAR_WIDTH = 1;
15+
const MIN_THUMB_HEIGHT = 2;
1516

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

4042
const show = useCallback(() => {
@@ -43,12 +45,12 @@ export const VerticalScrollbar = forwardRef<VerticalScrollbarHandle, VerticalScr
4345
clearTimeout(hideTimeoutRef.current);
4446
}
4547
hideTimeoutRef.current = setTimeout(() => {
46-
if (!isDragging) {
48+
if (!isDraggingRef.current) {
4749
setIsVisible(false);
4850
}
4951
}, HIDE_DELAY_MS);
5052
onActivity?.();
51-
}, [isDragging, onActivity]);
53+
}, [onActivity]);
5254

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

@@ -67,7 +69,7 @@ export const VerticalScrollbar = forwardRef<VerticalScrollbarHandle, VerticalScr
6769
// Calculate thumb metrics
6870
const trackHeight = viewportHeight;
6971
const scrollRatio = viewportHeight / contentHeight;
70-
const thumbHeight = Math.max(SCROLLBAR_WIDTH, Math.floor(trackHeight * scrollRatio));
72+
const thumbHeight = Math.max(MIN_THUMB_HEIGHT, Math.floor(trackHeight * scrollRatio));
7173
const maxThumbY = trackHeight - thumbHeight;
7274

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

8183
const currentScrollTop = scrollRef.current?.scrollTop ?? 0;
82-
setIsDragging(true);
83-
setDragStartY(event.y);
84-
setDragStartScroll(currentScrollTop);
84+
isDraggingRef.current = true;
85+
setIsDraggingState(true);
86+
dragStartYRef.current = event.y;
87+
dragStartScrollRef.current = currentScrollTop;
8588
show();
8689
event.preventDefault();
8790
event.stopPropagation();
8891
};
8992

9093
const handleMouseDrag = (event: TuiMouseEvent) => {
91-
if (!isDragging) return;
94+
if (!isDraggingRef.current) {
95+
return;
96+
}
9297

93-
const deltaY = event.y - dragStartY;
94-
const pixelsPerRow = maxThumbY / maxScroll;
98+
const deltaY = event.y - dragStartYRef.current;
99+
// Guard against division by zero when thumb fills track (maxThumbY = 0) or no scroll needed
100+
const pixelsPerRow = maxThumbY > 0 && maxScroll > 0 ? maxThumbY / maxScroll : 1;
95101
const scrollDelta = deltaY / pixelsPerRow;
96-
const newScrollTop = Math.max(0, Math.min(maxScroll, dragStartScroll + scrollDelta));
102+
const newScrollTop = Math.max(
103+
0,
104+
Math.min(maxScroll, dragStartScrollRef.current + scrollDelta),
105+
);
97106

98107
scrollRef.current?.scrollTo(newScrollTop);
99108
show();
100109
event.preventDefault();
101110
event.stopPropagation();
102111
};
103112

113+
const handleTrackClick = (event: TuiMouseEvent) => {
114+
if (event.button !== 0) return;
115+
116+
// Calculate where on the track was clicked
117+
// Note: event.y is relative to the scrollbar container since the component
118+
// is positioned at top: 0. If scrollbar position changes, this needs adjustment.
119+
const clickY = event.y;
120+
121+
// If clicked above thumb, scroll up one viewport
122+
// If clicked below thumb, scroll down one viewport
123+
if (clickY < thumbY) {
124+
const newScrollTop = Math.max(0, scrollTop - viewportHeight);
125+
scrollRef.current?.scrollTo(newScrollTop);
126+
} else if (clickY >= thumbY + thumbHeight) {
127+
const newScrollTop = Math.min(maxScroll, scrollTop + viewportHeight);
128+
scrollRef.current?.scrollTo(newScrollTop);
129+
}
130+
131+
show();
132+
event.preventDefault();
133+
event.stopPropagation();
134+
};
135+
104136
const handleMouseUp = (event?: TuiMouseEvent) => {
105-
if (!isDragging) return;
106-
setIsDragging(false);
137+
if (!isDraggingRef.current) return;
138+
isDraggingRef.current = false;
139+
setIsDraggingState(false);
107140
// Restart hide timer
108141
if (hideTimeoutRef.current) {
109142
clearTimeout(hideTimeoutRef.current);
@@ -128,7 +161,6 @@ export const VerticalScrollbar = forwardRef<VerticalScrollbarHandle, VerticalScr
128161
width: SCROLLBAR_WIDTH,
129162
height: trackHeight,
130163
backgroundColor: theme.panel,
131-
zIndex: 10,
132164
}}
133165
>
134166
{/* Track background */}
@@ -141,6 +173,7 @@ export const VerticalScrollbar = forwardRef<VerticalScrollbarHandle, VerticalScr
141173
height: trackHeight,
142174
backgroundColor: theme.border,
143175
}}
176+
onMouseDown={handleTrackClick}
144177
/>
145178
{/* Thumb */}
146179
<box
@@ -150,7 +183,7 @@ export const VerticalScrollbar = forwardRef<VerticalScrollbarHandle, VerticalScr
150183
left: 0,
151184
width: SCROLLBAR_WIDTH,
152185
height: thumbHeight,
153-
backgroundColor: isDragging ? theme.accent : theme.accentMuted,
186+
backgroundColor: isDraggingState ? theme.accent : theme.accentMuted,
154187
}}
155188
onMouseDown={handleMouseDown}
156189
onMouseDrag={handleMouseDrag}

‎test/vertical-scrollbar.test.tsx‎

Lines changed: 208 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -299,4 +299,212 @@ describe("Vertical scrollbar", () => {
299299
});
300300
}
301301
});
302+
303+
test("thumb drag scrolls content", async () => {
304+
// Create a file with many lines to ensure scrolling
305+
const before = Array.from({ length: 100 }, (_, j) => `line${j + 1}`).join("\n");
306+
const after = before.replace("line50", "line50modified");
307+
308+
const bootstrap: AppBootstrap = {
309+
input: {
310+
kind: "git",
311+
staged: false,
312+
options: { mode: "split" },
313+
},
314+
changeset: {
315+
id: "drag-test",
316+
sourceLabel: "repo",
317+
title: "drag test",
318+
files: [createDiffFile("drag", "src/drag.ts", before, after)],
319+
},
320+
initialMode: "split",
321+
initialTheme: "midnight",
322+
};
323+
324+
const setup = await testRender(<App bootstrap={bootstrap} />, {
325+
width: 160,
326+
height: 20, // Small viewport to force scrolling
327+
});
328+
329+
try {
330+
await flush(setup);
331+
await act(async () => {
332+
await Bun.sleep(100);
333+
});
334+
335+
// Get initial frame - app centers on the hunk at line 50
336+
const frame1 = setup.captureCharFrame();
337+
expect(frame1).toContain("line50");
338+
339+
// Drag scrollbar thumb down (rightmost column is scrollbar at x=159, y ranges 0-19)
340+
// Thumb should be at some position, drag it down to scroll
341+
await act(async () => {
342+
// Drag from top area of scrollbar down
343+
await setup.mockMouse.drag(159, 2, 159, 10);
344+
await flush(setup);
345+
await Bun.sleep(100);
346+
});
347+
348+
// After dragging down, we should see different content
349+
const frame2 = setup.captureCharFrame();
350+
expect(frame2).toBeTruthy();
351+
352+
// Drag back up
353+
await act(async () => {
354+
await setup.mockMouse.drag(159, 10, 159, 2);
355+
await flush(setup);
356+
await Bun.sleep(100);
357+
});
358+
359+
const frame3 = setup.captureCharFrame();
360+
expect(frame3).toBeTruthy();
361+
} finally {
362+
await act(async () => {
363+
setup.renderer.destroy();
364+
});
365+
}
366+
});
367+
368+
test("track click scrolls by one viewport", async () => {
369+
// Create a file with many lines to ensure scrolling
370+
const lines = Array.from({ length: 80 }, (_, j) => `line${String(j + 1).padStart(3, "0")}`);
371+
const before = lines.join("\n");
372+
const after = before.replace("line040", "line040modified");
373+
374+
const bootstrap: AppBootstrap = {
375+
input: {
376+
kind: "git",
377+
staged: false,
378+
options: { mode: "split" },
379+
},
380+
changeset: {
381+
id: "track-click-test",
382+
sourceLabel: "repo",
383+
title: "track click test",
384+
files: [createDiffFile("track", "src/track.ts", before, after)],
385+
},
386+
initialMode: "split",
387+
initialTheme: "midnight",
388+
};
389+
390+
const setup = await testRender(<App bootstrap={bootstrap} />, {
391+
width: 160,
392+
height: 15, // Viewport of 15 lines
393+
});
394+
395+
try {
396+
await flush(setup);
397+
await act(async () => {
398+
await Bun.sleep(100);
399+
});
400+
401+
// Get initial content - app centers on the hunk at line 40
402+
const frame1 = setup.captureCharFrame();
403+
expect(frame1).toContain("line040");
404+
405+
// First scroll down a bit to make scrollbar visible and move thumb down
406+
await act(async () => {
407+
for (let i = 0; i < 5; i++) {
408+
await setup.mockInput.pressArrow("down");
409+
}
410+
await flush(setup);
411+
await Bun.sleep(100);
412+
});
413+
414+
// Click on scrollbar track below thumb to page down
415+
// Scrollbar is at rightmost column (x=159), click near bottom
416+
await act(async () => {
417+
await setup.mockMouse.click(159, 12);
418+
await flush(setup);
419+
await Bun.sleep(100);
420+
});
421+
422+
const frame2 = setup.captureCharFrame();
423+
// Should have scrolled down further after track click
424+
expect(frame2).toBeTruthy();
425+
426+
// Click on scrollbar track above thumb to page up
427+
await act(async () => {
428+
await setup.mockMouse.click(159, 2);
429+
await flush(setup);
430+
await Bun.sleep(100);
431+
});
432+
433+
const frame3 = setup.captureCharFrame();
434+
// Should have scrolled back up
435+
expect(frame3).toBeTruthy();
436+
} finally {
437+
await act(async () => {
438+
setup.renderer.destroy();
439+
});
440+
}
441+
});
442+
443+
test("handles edge case when content barely exceeds viewport", async () => {
444+
// Create content that's just slightly larger than viewport
445+
// This tests the division-by-zero guard in drag calculations
446+
// Use the same pattern as other tests which work correctly
447+
const before = Array.from(
448+
{ length: 25 },
449+
(_, j) => `export const line${String(j + 1).padStart(2, "0")} = ${j + 1};`,
450+
).join("\n");
451+
const after = before.replace("line08 = 8;", "line08 = 999; // modified");
452+
453+
const bootstrap: AppBootstrap = {
454+
input: {
455+
kind: "git",
456+
staged: false,
457+
options: { mode: "split" },
458+
},
459+
changeset: {
460+
id: "edge-case-test",
461+
sourceLabel: "repo",
462+
title: "edge case test",
463+
files: [createDiffFile("edge", "src/edge.ts", before, after)],
464+
},
465+
initialMode: "split",
466+
initialTheme: "midnight",
467+
};
468+
469+
const setup = await testRender(<App bootstrap={bootstrap} />, {
470+
width: 160,
471+
height: 15, // Small viewport to force scrolling (25 lines of content in 15-line viewport)
472+
});
473+
474+
try {
475+
await flush(setup);
476+
await act(async () => {
477+
await Bun.sleep(100);
478+
});
479+
480+
// Verify app renders with the hunk visible - look for the modified line
481+
const frame1 = setup.captureCharFrame();
482+
expect(frame1).toContain("line08");
483+
484+
// Try to drag - should not crash with division by zero
485+
await act(async () => {
486+
await setup.mockMouse.drag(159, 0, 159, 5);
487+
await flush(setup);
488+
await Bun.sleep(100);
489+
});
490+
491+
// App should still be responsive after drag attempt
492+
const frame2 = setup.captureCharFrame();
493+
expect(frame2).toBeTruthy();
494+
495+
// Try track click - should not crash
496+
await act(async () => {
497+
await setup.mockMouse.click(159, 10);
498+
await flush(setup);
499+
await Bun.sleep(100);
500+
});
501+
502+
const frame3 = setup.captureCharFrame();
503+
expect(frame3).toBeTruthy();
504+
} finally {
505+
await act(async () => {
506+
setup.renderer.destroy();
507+
});
508+
}
509+
});
302510
});

0 commit comments

Comments
 (0)