Skip to content

Commit 14dd07b

Browse files
jacob314jacob314
authored
fix(cli): ensure dialogs stay scrolled to bottom in alternate buffer mode (#20527)
1 parent d7320f5 commit 14dd07b

6 files changed

Lines changed: 573 additions & 194 deletions

File tree

‎packages/cli/src/ui/components/shared/Scrollable.test.tsx‎

Lines changed: 17 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -6,20 +6,11 @@
66

77
import { renderWithProviders } from '../../../test-utils/render.js';
88
import { Scrollable } from './Scrollable.js';
9-
import { Text } from 'ink';
9+
import { Text, Box } from 'ink';
1010
import { describe, it, expect, vi, beforeEach } from 'vitest';
1111
import * as ScrollProviderModule from '../../contexts/ScrollProvider.js';
1212
import { act } from 'react';
13-
14-
vi.mock('ink', async (importOriginal) => {
15-
const actual = await importOriginal<typeof import('ink')>();
16-
return {
17-
...actual,
18-
getInnerHeight: vi.fn(() => 5),
19-
getScrollHeight: vi.fn(() => 10),
20-
getBoundingBox: vi.fn(() => ({ x: 0, y: 0, width: 10, height: 5 })),
21-
};
22-
});
13+
import { waitFor } from '../../../test-utils/async.js';
2314

2415
vi.mock('../../hooks/useAnimatedScrollbar.js', () => ({
2516
useAnimatedScrollbar: (
@@ -129,20 +120,26 @@ describe('<Scrollable />', () => {
129120
</Scrollable>,
130121
);
131122
await waitUntilReady2();
132-
expect(capturedEntry.getScrollState().scrollTop).toBe(5);
123+
await waitFor(() => {
124+
expect(capturedEntry?.getScrollState().scrollTop).toBe(5);
125+
});
133126

134127
// Call scrollBy multiple times (upwards) in the same tick
135128
await act(async () => {
136-
capturedEntry!.scrollBy(-1);
137-
capturedEntry!.scrollBy(-1);
129+
capturedEntry?.scrollBy(-1);
130+
capturedEntry?.scrollBy(-1);
138131
});
139132
// Should have moved up by 2 (5 -> 3)
140-
expect(capturedEntry.getScrollState().scrollTop).toBe(3);
133+
await waitFor(() => {
134+
expect(capturedEntry?.getScrollState().scrollTop).toBe(3);
135+
});
141136

142137
await act(async () => {
143-
capturedEntry!.scrollBy(-2);
138+
capturedEntry?.scrollBy(-2);
139+
});
140+
await waitFor(() => {
141+
expect(capturedEntry?.getScrollState().scrollTop).toBe(1);
144142
});
145-
expect(capturedEntry.getScrollState().scrollTop).toBe(1);
146143
unmount2();
147144
});
148145

@@ -191,10 +188,6 @@ describe('<Scrollable />', () => {
191188
keySequence,
192189
expectedScrollTop,
193190
}) => {
194-
// Dynamically import ink to mock getScrollHeight
195-
const ink = await import('ink');
196-
vi.mocked(ink.getScrollHeight).mockReturnValue(scrollHeight);
197-
198191
let capturedEntry: ScrollProviderModule.ScrollableEntry | undefined;
199192
vi.spyOn(ScrollProviderModule, 'useScrollable').mockImplementation(
200193
async (entry, isActive) => {
@@ -206,7 +199,9 @@ describe('<Scrollable />', () => {
206199

207200
const { stdin, waitUntilReady, unmount } = renderWithProviders(
208201
<Scrollable hasFocus={true} height={5}>
209-
<Text>Content</Text>
202+
<Box height={scrollHeight}>
203+
<Text>Content</Text>
204+
</Box>
210205
</Scrollable>,
211206
);
212207
await waitUntilReady();

‎packages/cli/src/ui/components/shared/Scrollable.tsx‎

Lines changed: 96 additions & 57 deletions
Original file line numberDiff line numberDiff line change
@@ -4,15 +4,9 @@
44
* SPDX-License-Identifier: Apache-2.0
55
*/
66

7-
import React, {
8-
useState,
9-
useEffect,
10-
useRef,
11-
useLayoutEffect,
12-
useCallback,
13-
useMemo,
14-
} from 'react';
15-
import { Box, getInnerHeight, getScrollHeight, type DOMElement } from 'ink';
7+
import type React from 'react';
8+
import { useState, useRef, useCallback, useMemo, useLayoutEffect } from 'react';
9+
import { Box, ResizeObserver, type DOMElement } from 'ink';
1610
import { useKeypress, type Key } from '../../hooks/useKeypress.js';
1711
import { useScrollable } from '../../contexts/ScrollProvider.js';
1812
import { useAnimatedScrollbar } from '../../hooks/useAnimatedScrollbar.js';
@@ -41,62 +35,101 @@ export const Scrollable: React.FC<ScrollableProps> = ({
4135
flexGrow,
4236
}) => {
4337
const [scrollTop, setScrollTop] = useState(0);
44-
const ref = useRef<DOMElement>(null);
38+
const viewportRef = useRef<DOMElement | null>(null);
39+
const contentRef = useRef<DOMElement | null>(null);
4540
const [size, setSize] = useState({
46-
innerHeight: 0,
41+
innerHeight: typeof height === 'number' ? height : 0,
4742
scrollHeight: 0,
4843
});
4944
const sizeRef = useRef(size);
50-
useEffect(() => {
45+
const scrollTopRef = useRef(scrollTop);
46+
47+
useLayoutEffect(() => {
5148
sizeRef.current = size;
5249
}, [size]);
5350

54-
const childrenCountRef = useRef(0);
55-
56-
// This effect needs to run on every render to correctly measure the container
57-
// and scroll to the bottom if new children are added.
58-
// eslint-disable-next-line react-hooks/exhaustive-deps
5951
useLayoutEffect(() => {
60-
if (!ref.current) {
61-
return;
52+
scrollTopRef.current = scrollTop;
53+
}, [scrollTop]);
54+
55+
const viewportObserverRef = useRef<ResizeObserver | null>(null);
56+
const contentObserverRef = useRef<ResizeObserver | null>(null);
57+
58+
const viewportRefCallback = useCallback((node: DOMElement | null) => {
59+
viewportObserverRef.current?.disconnect();
60+
viewportRef.current = node;
61+
62+
if (node) {
63+
const observer = new ResizeObserver((entries) => {
64+
const entry = entries[0];
65+
if (entry) {
66+
const innerHeight = Math.round(entry.contentRect.height);
67+
setSize((prev) => {
68+
const scrollHeight = prev.scrollHeight;
69+
const isAtBottom =
70+
scrollHeight > prev.innerHeight &&
71+
scrollTopRef.current >= scrollHeight - prev.innerHeight - 1;
72+
73+
if (isAtBottom) {
74+
setScrollTop(Number.MAX_SAFE_INTEGER);
75+
}
76+
return { ...prev, innerHeight };
77+
});
78+
}
79+
});
80+
observer.observe(node);
81+
viewportObserverRef.current = observer;
6282
}
63-
const innerHeight = Math.round(getInnerHeight(ref.current));
64-
const scrollHeight = Math.round(getScrollHeight(ref.current));
65-
66-
const isAtBottom =
67-
scrollHeight > innerHeight && scrollTop >= scrollHeight - innerHeight - 1;
68-
69-
if (
70-
size.innerHeight !== innerHeight ||
71-
size.scrollHeight !== scrollHeight
72-
) {
73-
setSize({ innerHeight, scrollHeight });
74-
if (isAtBottom) {
75-
setScrollTop(Math.max(0, scrollHeight - innerHeight));
83+
}, []);
84+
85+
const contentRefCallback = useCallback(
86+
(node: DOMElement | null) => {
87+
contentObserverRef.current?.disconnect();
88+
contentRef.current = node;
89+
90+
if (node) {
91+
const observer = new ResizeObserver((entries) => {
92+
const entry = entries[0];
93+
if (entry) {
94+
const scrollHeight = Math.round(entry.contentRect.height);
95+
setSize((prev) => {
96+
const innerHeight = prev.innerHeight;
97+
const isAtBottom =
98+
prev.scrollHeight > innerHeight &&
99+
scrollTopRef.current >= prev.scrollHeight - innerHeight - 1;
100+
101+
if (
102+
isAtBottom ||
103+
(scrollToBottom && scrollHeight > prev.scrollHeight)
104+
) {
105+
setScrollTop(Number.MAX_SAFE_INTEGER);
106+
}
107+
return { ...prev, scrollHeight };
108+
});
109+
}
110+
});
111+
observer.observe(node);
112+
contentObserverRef.current = observer;
76113
}
77-
}
78-
79-
const childCountCurrent = React.Children.count(children);
80-
if (scrollToBottom && childrenCountRef.current !== childCountCurrent) {
81-
setScrollTop(Math.max(0, scrollHeight - innerHeight));
82-
}
83-
childrenCountRef.current = childCountCurrent;
84-
});
114+
},
115+
[scrollToBottom],
116+
);
85117

86118
const { getScrollTop, setPendingScrollTop } = useBatchedScroll(scrollTop);
87119

88120
const scrollBy = useCallback(
89121
(delta: number) => {
90122
const { scrollHeight, innerHeight } = sizeRef.current;
91-
const current = getScrollTop();
92-
const next = Math.min(
93-
Math.max(0, current + delta),
94-
Math.max(0, scrollHeight - innerHeight),
95-
);
123+
const maxScroll = Math.max(0, scrollHeight - innerHeight);
124+
const current = Math.min(getScrollTop(), maxScroll);
125+
let next = Math.max(0, current + delta);
126+
if (next >= maxScroll) {
127+
next = Number.MAX_SAFE_INTEGER;
128+
}
96129
setPendingScrollTop(next);
97130
setScrollTop(next);
98131
},
99-
[sizeRef, getScrollTop, setPendingScrollTop],
132+
[getScrollTop, setPendingScrollTop],
100133
);
101134

102135
const { scrollbarColor, flashScrollbar, scrollByWithAnimation } =
@@ -107,10 +140,11 @@ export const Scrollable: React.FC<ScrollableProps> = ({
107140
const { scrollHeight, innerHeight } = sizeRef.current;
108141
const scrollTop = getScrollTop();
109142
const maxScroll = Math.max(0, scrollHeight - innerHeight);
143+
const actualScrollTop = Math.min(scrollTop, maxScroll);
110144

111145
// Only capture scroll-up events if there's room;
112146
// otherwise allow events to bubble.
113-
if (scrollTop > 0) {
147+
if (actualScrollTop > 0) {
114148
if (keyMatchers[Command.PAGE_UP](key)) {
115149
scrollByWithAnimation(-innerHeight);
116150
return true;
@@ -123,7 +157,7 @@ export const Scrollable: React.FC<ScrollableProps> = ({
123157

124158
// Only capture scroll-down events if there's room;
125159
// otherwise allow events to bubble.
126-
if (scrollTop < maxScroll) {
160+
if (actualScrollTop < maxScroll) {
127161
if (keyMatchers[Command.PAGE_DOWN](key)) {
128162
scrollByWithAnimation(innerHeight);
129163
return true;
@@ -140,21 +174,21 @@ export const Scrollable: React.FC<ScrollableProps> = ({
140174
{ isActive: hasFocus },
141175
);
142176

143-
const getScrollState = useCallback(
144-
() => ({
145-
scrollTop: getScrollTop(),
177+
const getScrollState = useCallback(() => {
178+
const maxScroll = Math.max(0, size.scrollHeight - size.innerHeight);
179+
return {
180+
scrollTop: Math.min(getScrollTop(), maxScroll),
146181
scrollHeight: size.scrollHeight,
147182
innerHeight: size.innerHeight,
148-
}),
149-
[getScrollTop, size.scrollHeight, size.innerHeight],
150-
);
183+
};
184+
}, [getScrollTop, size.scrollHeight, size.innerHeight]);
151185

152186
const hasFocusCallback = useCallback(() => hasFocus, [hasFocus]);
153187

154188
const scrollableEntry = useMemo(
155189
() => ({
156190
// eslint-disable-next-line @typescript-eslint/no-unsafe-type-assertion
157-
ref: ref as React.RefObject<DOMElement>,
191+
ref: viewportRef as React.RefObject<DOMElement>,
158192
getScrollState,
159193
scrollBy: scrollByWithAnimation,
160194
hasFocus: hasFocusCallback,
@@ -167,7 +201,7 @@ export const Scrollable: React.FC<ScrollableProps> = ({
167201

168202
return (
169203
<Box
170-
ref={ref}
204+
ref={viewportRefCallback}
171205
maxHeight={maxHeight}
172206
width={width ?? maxWidth}
173207
height={height}
@@ -183,7 +217,12 @@ export const Scrollable: React.FC<ScrollableProps> = ({
183217
based on the children's content. It also adds a right padding to
184218
make room for the scrollbar.
185219
*/}
186-
<Box flexShrink={0} paddingRight={1} flexDirection="column">
220+
<Box
221+
ref={contentRefCallback}
222+
flexShrink={0}
223+
paddingRight={1}
224+
flexDirection="column"
225+
>
187226
{children}
188227
</Box>
189228
</Box>

0 commit comments

Comments
 (0)