-
Notifications
You must be signed in to change notification settings - Fork 3k
fix(cli): fix thought viewer truncation, layout gaps, and choppy scrolling in VP mode #6002
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.
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
Changes from all commits
416bd95
6b91301
46f168d
83366e2
4fd13d3
e44654a
36d1f6b
a06a308
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,9 +5,10 @@ | |
| */ | ||
|
|
||
| import type { FC } from 'react'; | ||
| import { useState, useCallback, useEffect, useMemo } from 'react'; | ||
| import { useState, useCallback, useEffect, useMemo, useRef } from 'react'; | ||
| import { Box, Text } from 'ink'; | ||
| import { useTerminalSize } from '../hooks/useTerminalSize.js'; | ||
| import { useFrameCoalescedFlush } from '../hooks/use-frame-coalesced-flush.js'; | ||
| import { useKeypress, type Key } from '../hooks/useKeypress.js'; | ||
| import { useMouseEvents } from '../hooks/useMouseEvents.js'; | ||
| import type { MouseEvent } from '../utils/mouse.js'; | ||
|
|
@@ -17,6 +18,7 @@ import { t } from '../../i18n/index.js'; | |
| import { AlternateScreen } from './AlternateScreen.js'; | ||
| import type { ThinkingViewerData } from '../contexts/ThinkingViewerContext.js'; | ||
| import { THINKING_ICON } from './messages/ConversationMessages.js'; | ||
| import { wrapToVisualLines } from '../utils/textUtils.js'; | ||
| import { formatDuration } from '../utils/displayUtils.js'; | ||
|
|
||
| interface ThinkingViewerProps { | ||
|
|
@@ -33,14 +35,25 @@ export const ThinkingViewer: FC<ThinkingViewerProps> = ({ | |
| onClose, | ||
| useAlternateScreen = true, | ||
| }) => { | ||
| const { rows } = useTerminalSize(); | ||
| const { rows, columns } = useTerminalSize(); | ||
| const [scrollOffset, setScrollOffset] = useState(0); | ||
|
|
||
| const headerHeight = 2; | ||
| const footerHeight = 2; | ||
| const contentHeight = Math.max(rows - headerHeight - footerHeight, 1); | ||
|
|
||
| const lines = useMemo(() => data.text.split('\n'), [data.text]); | ||
| // The thought text is frequently a single long paragraph with no explicit | ||
| // newlines. Splitting on '\n' alone yields one logical line that, rendered | ||
| // with `wrap="truncate-end"`, collapsed to a single ellipsised row above an | ||
| // empty box (and `maxScroll` stayed 0, so it could not scroll). Pre-wrap to | ||
| // visual rows at the inner content width — border (1 each side) + paddingX | ||
| // (1 each side) = 4 columns — so scrolling and rendering operate on the same | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] Consider extracting a named constant (e.g., — qwen3.7-max via Qwen Code /review |
||
| // rows the user actually sees. | ||
| const contentWidth = Math.max(1, columns - 4); | ||
| const lines = useMemo( | ||
| () => wrapToVisualLines(data.text, contentWidth), | ||
| [data.text, contentWidth], | ||
| ); | ||
| const maxScroll = Math.max(0, lines.length - contentHeight); | ||
|
|
||
| useEffect(() => { | ||
|
|
@@ -54,6 +67,17 @@ export const ThinkingViewer: FC<ThinkingViewerProps> = ({ | |
| [maxScroll], | ||
| ); | ||
|
|
||
| // Coalesce wheel bursts to one update per frame, mirroring ScrollableList — | ||
| // each wheel event re-renders the modal, so an un-batched brisk spin stutters. | ||
| const pendingWheelDelta = useRef(0); | ||
| const { schedule: scheduleWheelFlush } = useFrameCoalescedFlush( | ||
| useCallback(() => { | ||
| const delta = pendingWheelDelta.current; | ||
| pendingWheelDelta.current = 0; | ||
| if (delta !== 0) scrollBy(delta); | ||
| }, [scrollBy]), | ||
| ); | ||
|
|
||
| useKeypress( | ||
| useCallback( | ||
| (key: Key) => { | ||
|
|
@@ -85,12 +109,14 @@ export const ThinkingViewer: FC<ThinkingViewerProps> = ({ | |
| useCallback( | ||
| (event: MouseEvent) => { | ||
| if (event.name === 'scroll-up') { | ||
| scrollBy(-WHEEL_LINES); | ||
| pendingWheelDelta.current -= WHEEL_LINES; | ||
| scheduleWheelFlush(); | ||
| } else if (event.name === 'scroll-down') { | ||
| scrollBy(WHEEL_LINES); | ||
| pendingWheelDelta.current += WHEEL_LINES; | ||
| scheduleWheelFlush(); | ||
| } | ||
| }, | ||
| [scrollBy], | ||
| [scheduleWheelFlush], | ||
| ), | ||
| { isActive: true }, | ||
| ); | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -11,6 +11,7 @@ import { | |||||||||||||
| type VirtualizedListRef, | ||||||||||||||
| type VirtualizedListProps, | ||||||||||||||
| } from './VirtualizedList.js'; | ||||||||||||||
| import { useFrameCoalescedFlush } from '../../hooks/use-frame-coalesced-flush.js'; | ||||||||||||||
| import { useKeypress, type Key } from '../../hooks/useKeypress.js'; | ||||||||||||||
| import { keyMatchers, Command } from '../../keyMatchers.js'; | ||||||||||||||
| import { useMouseEvents } from '../../hooks/useMouseEvents.js'; | ||||||||||||||
|
|
@@ -104,31 +105,81 @@ function ScrollableList<T>( | |||||||||||||
| // native scrollback. In VP mode the list owns the visible region, so route | ||||||||||||||
| // wheel ticks and scrollbar drags to the virtualized viewport. | ||||||||||||||
| const WHEEL_LINES_PER_TICK = 3; | ||||||||||||||
| const handleMouseEvent = useCallback((event: MouseEvent) => { | ||||||||||||||
| if (!virtualizedListRef.current) return; | ||||||||||||||
| if (event.name === 'left-release') { | ||||||||||||||
| isDraggingScrollbar.current = false; | ||||||||||||||
| return; | ||||||||||||||
| } | ||||||||||||||
| if (event.name === 'left-press') { | ||||||||||||||
| isDraggingScrollbar.current = | ||||||||||||||
| virtualizedListRef.current.hitTestScrollbar(event); | ||||||||||||||
| if (isDraggingScrollbar.current) { | ||||||||||||||
| virtualizedListRef.current.scrollToScrollbarRow(event.row); | ||||||||||||||
| } | ||||||||||||||
| return; | ||||||||||||||
| } | ||||||||||||||
| if (event.name === 'move' && isDraggingScrollbar.current) { | ||||||||||||||
| virtualizedListRef.current.scrollToScrollbarRow(event.row); | ||||||||||||||
|
|
||||||||||||||
| // Terminal mouse reporting emits one event per row the pointer crosses, so a | ||||||||||||||
| // brisk wheel spin or scrollbar drag fires a rapid burst. Applying each event | ||||||||||||||
| // synchronously forced one Ink reflow + terminal flush per event — the source | ||||||||||||||
| // of the "一顿一顿" stutter. Accumulate the intent in refs and let | ||||||||||||||
| // useFrameCoalescedFlush apply the latest at most once per frame. A drag is | ||||||||||||||
| // absolute (snap to the newest row); a wheel burst is relative (sum the | ||||||||||||||
| // ticks); a drag in the same window wins. | ||||||||||||||
| const pendingWheelDelta = useRef(0); | ||||||||||||||
| const pendingDragRow = useRef<number | null>(null); | ||||||||||||||
|
|
||||||||||||||
| const applyPendingScroll = useCallback(() => { | ||||||||||||||
| const list = virtualizedListRef.current; | ||||||||||||||
| const dragRow = pendingDragRow.current; | ||||||||||||||
| const wheelDelta = pendingWheelDelta.current; | ||||||||||||||
| pendingDragRow.current = null; | ||||||||||||||
| pendingWheelDelta.current = 0; | ||||||||||||||
| if (!list) return; | ||||||||||||||
| if (dragRow !== null) { | ||||||||||||||
| list.scrollToScrollbarRow(dragRow); | ||||||||||||||
| return; | ||||||||||||||
| } | ||||||||||||||
| if (event.name === 'scroll-up') { | ||||||||||||||
| virtualizedListRef.current.scrollBy(-WHEEL_LINES_PER_TICK); | ||||||||||||||
| } else if (event.name === 'scroll-down') { | ||||||||||||||
| virtualizedListRef.current.scrollBy(WHEEL_LINES_PER_TICK); | ||||||||||||||
| if (wheelDelta !== 0) { | ||||||||||||||
| list.scrollBy(wheelDelta); | ||||||||||||||
| } | ||||||||||||||
| }, []); | ||||||||||||||
|
|
||||||||||||||
| const { schedule: scheduleScrollFlush, cancel: cancelScrollFlush } = | ||||||||||||||
| useFrameCoalescedFlush(applyPendingScroll); | ||||||||||||||
|
|
||||||||||||||
| // Discard any queued wheel/drag intent and cancel an in-flight flush. Used | ||||||||||||||
| // when a scrollbar press takes over: without it, a wheel burst scheduled | ||||||||||||||
| // moments earlier would still fire and `scrollBy` the view away from the row | ||||||||||||||
| // the user just clicked. | ||||||||||||||
| const cancelPendingScroll = useCallback(() => { | ||||||||||||||
| pendingWheelDelta.current = 0; | ||||||||||||||
| pendingDragRow.current = null; | ||||||||||||||
| cancelScrollFlush(); | ||||||||||||||
| }, [cancelScrollFlush]); | ||||||||||||||
|
|
||||||||||||||
| const handleMouseEvent = useCallback( | ||||||||||||||
| (event: MouseEvent) => { | ||||||||||||||
| if (!virtualizedListRef.current) return; | ||||||||||||||
| if (event.name === 'left-release') { | ||||||||||||||
| isDraggingScrollbar.current = false; | ||||||||||||||
| return; | ||||||||||||||
| } | ||||||||||||||
| if (event.name === 'left-press') { | ||||||||||||||
| isDraggingScrollbar.current = | ||||||||||||||
| virtualizedListRef.current.hitTestScrollbar(event); | ||||||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] The fix is one line:
Suggested change
— qwen3.7-max via Qwen Code /review |
||||||||||||||
| if (isDraggingScrollbar.current) { | ||||||||||||||
| // A press should feel instant — apply now and drop any queued | ||||||||||||||
| // wheel/drag intent (and its timer) so a flush scheduled moments | ||||||||||||||
| // earlier can't yank the view off the clicked row. | ||||||||||||||
| cancelPendingScroll(); | ||||||||||||||
| virtualizedListRef.current.scrollToScrollbarRow(event.row); | ||||||||||||||
| } | ||||||||||||||
| return; | ||||||||||||||
| } | ||||||||||||||
| if (event.name === 'move' && isDraggingScrollbar.current) { | ||||||||||||||
| pendingDragRow.current = event.row; | ||||||||||||||
| scheduleScrollFlush(); | ||||||||||||||
| return; | ||||||||||||||
| } | ||||||||||||||
| if (event.name === 'scroll-up') { | ||||||||||||||
| pendingWheelDelta.current -= WHEEL_LINES_PER_TICK; | ||||||||||||||
| scheduleScrollFlush(); | ||||||||||||||
| } else if (event.name === 'scroll-down') { | ||||||||||||||
| pendingWheelDelta.current += WHEEL_LINES_PER_TICK; | ||||||||||||||
| scheduleScrollFlush(); | ||||||||||||||
| } | ||||||||||||||
| }, | ||||||||||||||
| [scheduleScrollFlush, cancelPendingScroll], | ||||||||||||||
| ); | ||||||||||||||
|
|
||||||||||||||
| useMouseEvents(handleMouseEvent, { isActive: hasFocus }); | ||||||||||||||
|
|
||||||||||||||
| // ScrollableList is a thin keyboard / mouse wrapper around VirtualizedList. | ||||||||||||||
|
|
||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Suggestion]
ThinkingViewer.test.tsxdoes not exist. The core bug fix — pre-wrapping thought text to visual lines instead of splitting on\n— has no regression test. The motivating bug (a single long paragraph collapsing to one ellipsised row) could silently reappear ifwrapToVisualLinesor thecontentWidthformula changes.Consider adding tests covering:
columns - 4yields small widthsdata.textcolumnsbetween renders)— qwen3.7-max via Qwen Code /review