-
Notifications
You must be signed in to change notification settings - Fork 3k
feat(cli): VP mode — inline thought expand on click + auto-hiding scrollbar #6079
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
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 | ||||
|---|---|---|---|---|---|---|
|
|
@@ -61,7 +61,6 @@ import { DiffStatsDisplay } from './messages/DiffStatsDisplay.js'; | |||||
| import { GoalStatusMessage } from './messages/GoalStatusMessage.js'; | ||||||
| import { useSettings } from '../contexts/SettingsContext.js'; | ||||||
| import { useThoughtExpanded } from '../contexts/ThoughtExpandedContext.js'; | ||||||
| import { useThinkingViewer } from '../contexts/ThinkingViewerContext.js'; | ||||||
| import { useMouseEvents } from '../hooks/useMouseEvents.js'; | ||||||
| import type { MouseEvent } from '../utils/mouse.js'; | ||||||
| import { measureElementPosition } from '../utils/measure-element-position.js'; | ||||||
|
|
@@ -80,8 +79,12 @@ interface HistoryItemDisplayProps { | |||||
| sourceCopyIndexOffsets?: MarkdownSourceCopyIndexOffsets; | ||||||
| /** Force thinking blocks expanded (e.g. in SessionPreview). */ | ||||||
| thoughtExpanded?: boolean; | ||||||
| /** Aggregated text from this thought + its continuation items. */ | ||||||
| thinkingFullText?: string; | ||||||
| /** | ||||||
| * Head id of the thought group this item belongs to (the `gemini_thought` | ||||||
| * head id for both the head and its `gemini_thought_content` continuations). | ||||||
| * Used to expand/collapse the whole group as a unit on click. | ||||||
| */ | ||||||
| thoughtHeadId?: number; | ||||||
| } | ||||||
|
|
||||||
| /** | ||||||
|
|
@@ -91,32 +94,31 @@ interface HistoryItemDisplayProps { | |||||
| */ | ||||||
| const ClickableThinkMessage: React.FC<{ | ||||||
| text: string; | ||||||
| viewerText: string; | ||||||
| isPending: boolean; | ||||||
| expanded: boolean; | ||||||
| availableTerminalHeight?: number; | ||||||
| contentWidth: number; | ||||||
| durationMs?: number; | ||||||
| onToggle: () => void; | ||||||
| }> = ({ | ||||||
| text, | ||||||
| viewerText, | ||||||
| isPending, | ||||||
| expanded, | ||||||
| availableTerminalHeight, | ||||||
| contentWidth, | ||||||
| durationMs, | ||||||
| onToggle, | ||||||
| }) => { | ||||||
| const ref = useRef<DOMElement>(null); | ||||||
| const { openThinkingViewer } = useThinkingViewer(); | ||||||
| // Click-to-expand needs SGR mouse tracking. We do NOT pass `bypassVpGate`, so | ||||||
| // useMouseEvents enables it only in VP mode; in non-VP the click handler | ||||||
| // stays dormant and native terminal scrollback is preserved (the block still | ||||||
| // expands via Alt+T — the "option+t to expand" affordance it already shows). | ||||||
| const isActive = !isPending && !expanded; | ||||||
| const sanitizedViewerText = useMemo( | ||||||
| () => escapeAnsiCtrlCodes(viewerText), | ||||||
| [viewerText], | ||||||
| ); | ||||||
| // Click toggles the thought's inline expansion in place (it then scrolls | ||||||
| // with the conversation). Click needs SGR mouse tracking; useMouseEvents | ||||||
| // enables it only in VP mode (no `bypassVpGate`), so in non-VP the handler | ||||||
| // stays dormant and native scrollback is preserved — the block still toggles | ||||||
| // via Alt+T. Advertise "click" in the collapsed hint only in VP, where the | ||||||
| // click actually does something. | ||||||
| const settings = useSettings(); | ||||||
| const clickable = !!settings.merged.ui?.useTerminalBuffer; | ||||||
|
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.
The hook's effective gate is 中文
hook 的有效门控是 |
||||||
| const isActive = !isPending; | ||||||
|
|
||||||
| useMouseEvents( | ||||||
| useCallback( | ||||||
|
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] Users reading expanded thought text who click to focus or scroll within it will accidentally collapse the block they're reading. Consider either:
— qwen3.7-max via Qwen Code /review |
||||||
|
|
@@ -131,10 +133,10 @@ const ClickableThinkMessage: React.FC<{ | |||||
| row >= metrics.y && | ||||||
| row < metrics.y + metrics.height | ||||||
| ) { | ||||||
| openThinkingViewer({ text: sanitizedViewerText, durationMs }); | ||||||
| onToggle(); | ||||||
| } | ||||||
| }, | ||||||
| [openThinkingViewer, sanitizedViewerText, durationMs], | ||||||
| [onToggle], | ||||||
| ), | ||||||
| { isActive }, | ||||||
| ); | ||||||
|
|
@@ -148,6 +150,7 @@ const ClickableThinkMessage: React.FC<{ | |||||
| availableTerminalHeight={availableTerminalHeight} | ||||||
| contentWidth={contentWidth} | ||||||
| durationMs={durationMs} | ||||||
| clickable={clickable} | ||||||
| /> | ||||||
| </Box> | ||||||
| ); | ||||||
|
|
@@ -199,12 +202,22 @@ const HistoryItemDisplayComponent: React.FC<HistoryItemDisplayProps> = ({ | |||||
| availableTerminalHeightGemini, | ||||||
| sourceCopyIndexOffsets, | ||||||
| thoughtExpanded, | ||||||
| thinkingFullText, | ||||||
| thoughtHeadId, | ||||||
| }) => { | ||||||
| const marginTop = getHistoryItemMarginTop(item); | ||||||
|
|
||||||
| const contextThoughtExpanded = useThoughtExpanded(); | ||||||
| const resolvedThoughtExpanded = thoughtExpanded ?? contextThoughtExpanded; | ||||||
| const { | ||||||
| allExpanded, | ||||||
| expandedHeadIds, | ||||||
| toggle: toggleThought, | ||||||
| } = useThoughtExpanded(); | ||||||
| // A thought spans the `gemini_thought` head plus its trailing | ||||||
| // `gemini_thought_content` items; all of them key off the head id so one | ||||||
| // click expands the whole group. Continuations receive the head id via | ||||||
| // `thoughtHeadId`; the head itself falls back to its own id. | ||||||
| const thoughtGroupHeadId = thoughtHeadId ?? item.id; | ||||||
| const resolvedThoughtExpanded = | ||||||
| thoughtExpanded ?? (allExpanded || expandedHeadIds.has(thoughtGroupHeadId)); | ||||||
|
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] Test coverage gap: the new
The existing test ( Consider adding tests that exercise the — qwen3.7-max via Qwen Code /review
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. [Confirmed] Alt+T and per-thought click interact badly: clicking while
Two easy fixes: have the click handler treat 中文[已确认] Alt+T 与单条点击交互不良:
两个简单修法:点击处理器把 |
||||||
| const settings = useSettings(); | ||||||
| const showTimestamps = settings.merged.output?.showTimestamps === true; | ||||||
|
|
||||||
|
|
@@ -269,14 +282,14 @@ const HistoryItemDisplayComponent: React.FC<HistoryItemDisplayProps> = ({ | |||||
| {itemForDisplay.type === 'gemini_thought' && ( | ||||||
| <ClickableThinkMessage | ||||||
| text={itemForDisplay.text.trimEnd()} | ||||||
| viewerText={(thinkingFullText || itemForDisplay.text).trimEnd()} | ||||||
| isPending={isPending} | ||||||
| expanded={resolvedThoughtExpanded} | ||||||
| availableTerminalHeight={ | ||||||
| availableTerminalHeightGemini ?? availableTerminalHeight | ||||||
| } | ||||||
| contentWidth={contentWidth} | ||||||
| durationMs={itemForDisplay.durationMs} | ||||||
| onToggle={() => toggleThought(thoughtGroupHeadId)} | ||||||
| /> | ||||||
|
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] When the Guard the toggle when the prop overrides:
Suggested change
— qwen3.7-max via Qwen Code /review |
||||||
| )} | ||||||
| {itemForDisplay.type === 'gemini_thought_content' && ( | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -19,7 +19,7 @@ import { | |
| countMarkdownSourceBlocks, | ||
| type MarkdownSourceCopyIndexOffsets, | ||
| } from '../utils/MarkdownDisplay.js'; | ||
| import { buildThinkingFullTextMap } from '../utils/historyUtils.js'; | ||
| import { buildThoughtHeadIdMap } from '../utils/historyUtils.js'; | ||
| import { ScrollableList, SCROLL_TO_ITEM_END } from './shared/ScrollableList.js'; | ||
|
|
||
| // Limit Gemini messages to a very high number of lines to mitigate performance | ||
|
|
@@ -92,6 +92,7 @@ const virtualIsStaticItem = (item: HistoryItem) => item.id > 0; | |
| export const MainContent = () => { | ||
| const { version } = useAppContext(); | ||
| const uiState = useUIState(); | ||
| const showScrollbar = uiState.showScrollbar ?? true; | ||
|
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. Minor: the settingsSchema default, AppContainer ( 中文次要: settingsSchema 默认值、AppContainer( |
||
| const { | ||
| pendingHistoryItems, | ||
| terminalWidth, | ||
|
|
@@ -276,12 +277,12 @@ export const MainContent = () => { | |
| return map; | ||
| }, [historyItemsWithSourceCopyOffsets]); | ||
|
|
||
| const thinkingFullTextByItem = useMemo( | ||
| () => buildThinkingFullTextMap(visibleHistory), | ||
| const thoughtHeadIdByItem = useMemo( | ||
| () => buildThoughtHeadIdMap(visibleHistory), | ||
| [visibleHistory], | ||
| ); | ||
| const thinkingFullTextByItemRef = useRef(thinkingFullTextByItem); | ||
| thinkingFullTextByItemRef.current = thinkingFullTextByItem; | ||
| const thoughtHeadIdByItemRef = useRef(thoughtHeadIdByItem); | ||
| thoughtHeadIdByItemRef.current = thoughtHeadIdByItem; | ||
|
|
||
| const pendingSourceCopyOffsetsByIndex = useMemo( | ||
| () => | ||
|
|
@@ -359,7 +360,7 @@ export const MainContent = () => { | |
| isPending={false} | ||
| commands={uiState.slashCommands} | ||
| sourceCopyIndexOffsets={sourceCopyIndexOffsets} | ||
| thinkingFullText={thinkingFullTextByItemRef.current.get(item)} | ||
| thoughtHeadId={thoughtHeadIdByItemRef.current.get(item)} | ||
| /> | ||
| ); | ||
| }, | ||
|
|
@@ -398,6 +399,7 @@ export const MainContent = () => { | |
| initialScrollIndex={SCROLL_TO_ITEM_END} | ||
| isStaticItem={virtualIsStaticItem} | ||
| containerHeight={scrollContainerHeight} | ||
| showScrollbar={showScrollbar} | ||
| /> | ||
| <ShowMoreLines constrainHeight={uiState.constrainHeight} /> | ||
| </OverflowProvider> | ||
|
|
@@ -430,7 +432,7 @@ export const MainContent = () => { | |
| isPending={false} | ||
|
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] If the user expands a thought group by clicking the head, pending continuation items that stream in won't match ( Consider passing — qwen3.7-max via Qwen Code /review |
||
| commands={uiState.slashCommands} | ||
| sourceCopyIndexOffsets={sourceCopyIndexOffsets} | ||
| thinkingFullText={thinkingFullTextByItem.get(h)} | ||
| thoughtHeadId={thoughtHeadIdByItem.get(h)} | ||
| /> | ||
| ), | ||
| ), | ||
|
|
||
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] Test coverage gap:
ClickableThinkMessageis a new component introduced in this PR, but the click hit-testing logic is not tested. The existing test ('subscribes the click handler without bypassVpGate') only verifiesuseMouseEventswas called with{ isActive: true }— it never simulates a mouse event or asserts that (a) an in-bounds left-press firesonToggle, (b) an out-of-bounds click is ignored, or (c)isPending=truedisables the handler viaisActive=false.Since this is the core new interaction replacing the deleted
ThinkingViewer, consider adding a test that mocksmeasureElementPositionto return known bounds, feeds a synthetic{ name: 'left-press', col, row }event to the captureduseMouseEventscallback, and assertsonTogglefires only for in-bounds coordinates.— qwen3.7-max via Qwen Code /review