Skip to content

Commit b986f89

Browse files
faiyaz26Copilot
andcommitted
fix: remove flaky terminal scroll affordance
The scroll-to-bottom button was unreliable in practice and distracted from the terminal experience. Keep the underlying auto-scroll pause behavior and harden shared scroll state detection across the terminal stack. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent e794cbe commit b986f89

7 files changed

Lines changed: 105 additions & 62 deletions

File tree

frontend/src/components/TerminalView.tsx

Lines changed: 1 addition & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import { onCleanup, onMount, createEffect, createSignal, Show, untrack } from 'solid-js';
1+
import { onCleanup, onMount, createSignal, Show } from 'solid-js';
22
import { useTerminalPool } from '../hooks/useTerminalPool';
33
import { TerminalInstance } from './terminal/TerminalInstance';
44
import type { TerminalHandle } from '../types/terminal';
@@ -124,14 +124,6 @@ export function TerminalView(props: TerminalViewProps) {
124124
setShowNotificationPrompt(false);
125125
};
126126

127-
const scrollToBottom = () => {
128-
const h = handle();
129-
if (h) {
130-
h.terminal.scrollToBottom();
131-
h.autoScroll = true;
132-
}
133-
};
134-
135127
return (
136128
<div class="relative w-full h-full group">
137129
<Show
@@ -158,19 +150,6 @@ export function TerminalView(props: TerminalViewProps) {
158150
{(h) => <TerminalInstance handle={h()} />}
159151
</Show>
160152

161-
<Show when={handle() && !handle()!.autoScroll}>
162-
<button
163-
onClick={scrollToBottom}
164-
data-testid="scroll-to-bottom-button"
165-
class="absolute bottom-10 right-10 px-4 py-2 bg-zed-accent-blue text-white rounded-full shadow-xl flex items-center gap-2 text-sm font-semibold hover:bg-zed-accent-blue-hover transition-all animate-bounce-in z-20 cursor-pointer"
166-
>
167-
<svg class="w-4 h-4" fill="none" stroke="currentColor" viewBox="0 0 24 24" stroke-width="3">
168-
<path stroke-linecap="round" stroke-linejoin="round" d="M19 13l-7 7-7-7m14-8l-7 7-7-7" />
169-
</svg>
170-
Scroll to Bottom
171-
</button>
172-
</Show>
173-
174153
<Show when={showNotificationPrompt()}>
175154
<div class="absolute top-3 left-3 right-3 flex items-center gap-3 px-4 py-3 rounded-lg bg-zed-bg-overlay border border-zed-border-default shadow-lg animate-slide-down z-10">
176155
<svg class="w-4 h-4 text-zed-accent-blue shrink-0" fill="none" viewBox="0 0 24 24" stroke="currentColor">

frontend/src/components/__tests__/TerminalView.test.tsx

Lines changed: 50 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,8 @@ vi.stubGlobal('ResizeObserver', class {
2525
});
2626

2727
// Mock terminal and fit addon
28-
let scrollCallback: (() => void) | null = null;
28+
let scrollCallback: ((viewportY: number) => void) | null = null;
29+
let wheelCallback: ((event: WheelEvent) => boolean) | null = null;
2930
const mockTerminalInstance = {
3031
open: vi.fn(),
3132
focus: vi.fn(),
@@ -37,6 +38,9 @@ const mockTerminalInstance = {
3738
onData: vi.fn(() => ({ dispose: vi.fn() })),
3839
onTitleChange: vi.fn(() => ({ dispose: vi.fn() })),
3940
attachCustomKeyEventHandler: vi.fn(),
41+
attachCustomWheelEventHandler: vi.fn((cb) => {
42+
wheelCallback = cb;
43+
}),
4044
loadAddon: vi.fn(),
4145
write: vi.fn(),
4246
scrollToBottom: vi.fn(),
@@ -95,6 +99,8 @@ vi.mock('../../lib/terminal-utils', () => ({
9599
createFitAddon: () => ({ fit: vi.fn() }),
96100
loadAddons: vi.fn(),
97101
attachKeyHandlers: vi.fn(),
102+
isTerminalViewportAtBottom: (terminal: any, viewportY = terminal.buffer.active.viewportY) =>
103+
viewportY >= terminal.buffer.active.baseY,
98104
updateTerminalTheme: vi.fn(),
99105
}));
100106

@@ -107,6 +113,7 @@ describe('TerminalView Scrolling', () => {
107113
beforeEach(() => {
108114
vi.clearAllMocks();
109115
scrollCallback = null;
116+
wheelCallback = null;
110117
mockTerminalInstance.buffer.active.viewportY = 0;
111118
mockTerminalInstance.buffer.active.baseY = 0;
112119
mockTerminalInstance.buffer.active.length = 24;
@@ -131,27 +138,55 @@ describe('TerminalView Scrolling', () => {
131138
setMockHandle = s;
132139
});
133140

134-
it('shows "Scroll to Bottom" button when user scrolls up', async () => {
135-
// Start with autoScroll false so button can show
136-
const current = { ...mockHandle() };
137-
current.autoScroll = false;
138-
setMockHandle(current);
139-
141+
it('keeps the scroll-to-bottom button hidden when the terminal is manually scrolled up', async () => {
140142
render(() => <TerminalView laneId="lane-1" />);
141143

142-
// Wait for handle to be set
143144
await waitFor(() => expect(mockTerminalPool.acquire).toHaveBeenCalled());
145+
await waitFor(() => expect(scrollCallback).not.toBeNull());
146+
147+
expect(screen.queryByTestId('scroll-to-bottom-button')).not.toBeInTheDocument();
144148

145149
// Simulate user scrolling up
146-
mockTerminalInstance.buffer.active.baseY = 76;
150+
mockTerminalInstance.buffer.active.baseY = 76;
147151
mockTerminalInstance.buffer.active.viewportY = 0;
148152
mockTerminalInstance.buffer.active.length = 100;
149-
150-
// Trigger the scroll callback
151-
if (scrollCallback) scrollCallback();
152153

153-
// The button should now be visible
154-
const button = await screen.findByTestId('scroll-to-bottom-button');
155-
expect(button).toBeDefined();
154+
scrollCallback?.(0);
155+
156+
expect(mockHandle().autoScroll).toBe(false);
157+
expect(screen.queryByTestId('scroll-to-bottom-button')).not.toBeInTheDocument();
158+
});
159+
160+
it('still disables auto-scroll from xterm scroll events before buffer viewport state catches up', async () => {
161+
render(() => <TerminalView laneId="lane-1" />);
162+
163+
await waitFor(() => expect(mockTerminalPool.acquire).toHaveBeenCalled());
164+
await waitFor(() => expect(scrollCallback).not.toBeNull());
165+
166+
mockTerminalInstance.buffer.active.baseY = 76;
167+
mockTerminalInstance.buffer.active.viewportY = 76;
168+
mockTerminalInstance.buffer.active.length = 100;
169+
170+
scrollCallback?.(0);
171+
172+
expect(mockHandle().autoScroll).toBe(false);
173+
expect(screen.queryByTestId('scroll-to-bottom-button')).not.toBeInTheDocument();
174+
});
175+
176+
it('pauses auto-scroll as soon as the user wheels upward', async () => {
177+
render(() => <TerminalView laneId="lane-1" />);
178+
179+
await waitFor(() => expect(mockTerminalPool.acquire).toHaveBeenCalled());
180+
await waitFor(() => expect(wheelCallback).not.toBeNull());
181+
182+
expect(screen.queryByTestId('scroll-to-bottom-button')).not.toBeInTheDocument();
183+
184+
const shouldContinue = wheelCallback?.({
185+
deltaY: -1,
186+
} as WheelEvent);
187+
188+
expect(shouldContinue).toBe(true);
189+
expect(mockHandle().autoScroll).toBe(false);
190+
expect(screen.queryByTestId('scroll-to-bottom-button')).not.toBeInTheDocument();
156191
});
157192
});

frontend/src/components/terminal/TerminalInstance.tsx

Lines changed: 21 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77
import { onMount, onCleanup, createEffect } from 'solid-js';
88
import {
99
SharedTerminalInstance,
10+
isTerminalViewportAtBottom,
1011
updateTerminalTheme,
1112
themeManager
1213
} from '@codelane/shared';
@@ -40,12 +41,28 @@ export function TerminalInstance(props: TerminalInstanceProps) {
4041
if (!props.handle) return;
4142
const { terminal } = props.handle;
4243

44+
const setAutoScroll = (autoScroll: boolean) => {
45+
props.handle.autoScroll = autoScroll;
46+
};
47+
4348
// Sticky scroll detection
44-
const updateAutoScroll = () => {
45-
const buffer = terminal.buffer.active;
46-
props.handle.autoScroll = buffer.baseY + terminal.rows >= buffer.length;
49+
const updateAutoScroll = (viewportY = terminal.buffer.active.viewportY) => {
50+
const autoScroll = isTerminalViewportAtBottom(terminal, viewportY);
51+
setAutoScroll(autoScroll);
4752
};
48-
terminal.onScroll(updateAutoScroll);
53+
54+
// Pause follow-mode as soon as the user starts scrolling up so live output
55+
// cannot race the viewport back to the bottom before xterm emits onScroll.
56+
terminal.attachCustomWheelEventHandler((event) => {
57+
if (event.deltaY < 0 && props.handle.autoScroll) {
58+
setAutoScroll(false);
59+
}
60+
return true;
61+
});
62+
63+
const scrollDisposable = terminal.onScroll((viewportY) => updateAutoScroll(viewportY));
64+
updateAutoScroll();
65+
onCleanup(() => scrollDisposable.dispose());
4966
});
5067

5168
return (

frontend/src/services/TerminalPool.ts

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@
1010

1111
import { spawn, type PtyHandle } from './PortablePty';
1212
import { getLaneAgentConfig, checkCommandExists } from '../lib/settings-api';
13-
import { createTerminal, createFitAddon, attachKeyHandlers } from '../lib/terminal-utils';
13+
import { createTerminal, createFitAddon, attachKeyHandlers, isTerminalViewportAtBottom } from '../lib/terminal-utils';
1414
import { agentStatusManager } from './AgentStatusManager';
1515
import type { DetectableAgentType } from '../types/agentStatus';
1616
import type {
@@ -232,8 +232,7 @@ class TerminalPool {
232232
await pty.onData((data) => {
233233
// Check if we're at the bottom BEFORE writing. If the user has scrolled up,
234234
// we should respect that and not force a scroll to bottom.
235-
const buffer = terminal.buffer.active;
236-
const isAtBottom = buffer.baseY + terminal.rows >= buffer.length;
235+
const isAtBottom = isTerminalViewportAtBottom(terminal);
237236

238237
terminal.write(data);
239238

frontend/src/services/__tests__/TerminalPool.test.ts

Lines changed: 16 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -68,7 +68,7 @@ function createMockTerminal() {
6868
focus,
6969
element: null,
7070
buffer: {
71-
active: { baseY: 0, length: 24 },
71+
active: { baseY: 0, viewportY: 0, length: 24 },
7272
},
7373
hasSelection: () => false,
7474
options: {},
@@ -88,6 +88,8 @@ vi.mock('../../lib/terminal-utils', () => ({
8888
},
8989
createFitAddon: () => ({ fit: vi.fn() }),
9090
attachKeyHandlers: vi.fn(() => () => {}),
91+
isTerminalViewportAtBottom: (terminal: any, viewportY = terminal.buffer.active.viewportY) =>
92+
viewportY >= terminal.buffer.active.baseY,
9193
loadAddons: vi.fn(),
9294
updateTerminalTheme: vi.fn(),
9395
}));
@@ -188,10 +190,11 @@ describe('TerminalPool', () => {
188190

189191
expect(handle.autoScroll).toBe(true);
190192

191-
// Simulate user being scrolled up (baseY is 0, length is 100)
192-
terminal.buffer.active.baseY = 0;
193+
// Simulate user being scrolled up. `baseY` marks the viewport position when
194+
// fully scrolled down, while `viewportY` is where the user currently is.
195+
terminal.buffer.active.baseY = 76;
196+
terminal.buffer.active.viewportY = 0;
193197
terminal.buffer.active.length = 100;
194-
// 0 + 24 < 100, so we're scrolled up
195198

196199
// Simulate PTY output
197200
const data = new TextEncoder().encode('new output');
@@ -203,19 +206,18 @@ describe('TerminalPool', () => {
203206
});
204207

205208
describe('scroll position detection', () => {
206-
it('detects at-bottom when baseY + rows >= length', () => {
207-
// This tests the logic: buffer.baseY + terminal.rows >= buffer.length
209+
it('detects at-bottom when viewportY reaches baseY', () => {
208210
const cases = [
209-
{ baseY: 0, rows: 24, length: 24, expected: true }, // exactly at bottom
210-
{ baseY: 10, rows: 24, length: 34, expected: true }, // scrollback, at bottom
211-
{ baseY: 10, rows: 24, length: 35, expected: false }, // 1 line scrolled up
212-
{ baseY: 0, rows: 24, length: 50, expected: false }, // scrolled to top
213-
{ baseY: 100, rows: 24, length: 124, expected: true }, // large buffer, at bottom
214-
{ baseY: 100, rows: 24, length: 125, expected: false }, // large buffer, 1 line up
211+
{ baseY: 0, viewportY: 0, expected: true }, // no scrollback
212+
{ baseY: 10, viewportY: 10, expected: true }, // scrolled to bottom
213+
{ baseY: 10, viewportY: 9, expected: false }, // 1 line above bottom
214+
{ baseY: 76, viewportY: 0, expected: false }, // top of long scrollback
215+
{ baseY: 100, viewportY: 100, expected: true }, // large buffer, at bottom
216+
{ baseY: 100, viewportY: 50, expected: false }, // large buffer, midway up
215217
];
216218

217-
for (const { baseY, rows, length, expected } of cases) {
218-
const isAtBottom = baseY + rows >= length;
219+
for (const { baseY, viewportY, expected } of cases) {
220+
const isAtBottom = viewportY >= baseY;
219221
expect(isAtBottom).toBe(expected);
220222
}
221223
});

packages/shared/src/components/SharedTerminalInstance.tsx

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,12 @@
55
*/
66

77
import { onMount, onCleanup } from 'solid-js';
8-
import { loadAddons, attachKeyHandlers, type TerminalHandlers } from '../terminal/terminal-utils';
8+
import {
9+
loadAddons,
10+
attachKeyHandlers,
11+
isTerminalViewportAtBottom,
12+
type TerminalHandlers,
13+
} from '../terminal/terminal-utils';
914
import type { Terminal } from '@xterm/xterm';
1015
import type { FitAddon } from '@xterm/addon-fit';
1116

@@ -60,8 +65,7 @@ export function SharedTerminalInstance(props: SharedTerminalInstanceProps) {
6065
const r = containerRef.getBoundingClientRect();
6166
if (r.width < 1 || r.height < 1) return;
6267

63-
const buffer = terminal.buffer.active;
64-
const isAtBottom = buffer.baseY + terminal.rows >= buffer.length;
68+
const isAtBottom = isTerminalViewportAtBottom(terminal);
6569

6670
fitAddon.fit();
6771
props.onResizePty(terminal.cols, terminal.rows);
@@ -77,7 +81,6 @@ export function SharedTerminalInstance(props: SharedTerminalInstanceProps) {
7781
// Initial resize
7882
setTimeout(() => {
7983
safeFitAndResize();
80-
terminal.scrollToBottom();
8184
}, 100);
8285

8386
// Handle resize events

packages/shared/src/terminal/terminal-utils.ts

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,14 @@ export interface TerminalHandlers {
1515
onWriteClipboard?: (text: string) => Promise<void>;
1616
}
1717

18+
export function isTerminalViewportAtBottom(
19+
terminal: Pick<Terminal, 'buffer'>,
20+
viewportY = terminal.buffer.active.viewportY
21+
): boolean {
22+
const buffer = terminal.buffer.active;
23+
return viewportY >= buffer.baseY;
24+
}
25+
1826
/**
1927
* Creates a pre-configured xterm.js Terminal instance with current theme
2028
*/

0 commit comments

Comments
 (0)