-
Notifications
You must be signed in to change notification settings - Fork 3k
fix(cli): enable footer text selection in VP mode #8329
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
620e982
bd1b424
70e9318
964c72a
fc08ff6
de89559
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 |
|---|---|---|
| @@ -0,0 +1,31 @@ | ||
| # Statusline text selection | ||
|
|
||
| ## Problem | ||
|
|
||
| Virtualized History enables terminal-wide mouse tracking, so the terminal cannot | ||
| provide native text selection. Qwen Code's application-level selection currently | ||
| accepts presses only inside the history viewport, leaving the footer/statusline | ||
| unselectable. | ||
|
|
||
| ## Design | ||
|
|
||
| Keep one selection controller and give it an ordered list of selectable frame | ||
| rectangles. The history viewport remains the primary rectangle. The default | ||
| layout passes a ref for the rendered footer through `Composer`, and | ||
| `MainContent` supplies its measured rectangle as the second target. | ||
|
|
||
| The controller records which rectangle owns a selection when the press starts. | ||
| Drag coordinates remain clamped to that rectangle, and frame/layout changes are | ||
| compared only within it. Input, dialogs, scrollbars, and other controls remain | ||
| outside the selectable targets, so their existing mouse behavior is unchanged. | ||
|
|
||
| This applies only to the existing Virtualized History path. Normal-buffer mode | ||
| continues to use terminal-native selection. | ||
|
|
||
| ## Verification | ||
|
|
||
| - Dragging within history still highlights and copies history text. | ||
| - Dragging within a multi-line footer highlights and copies footer text. | ||
| - Presses in the gap between the history and footer do not start a selection. | ||
| - Footer selection is cleared when its content or layout changes. | ||
| - A live Virtualized History session can copy visible statusline text. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,8 +6,10 @@ | |
|
|
||
| import { render } from 'ink-testing-library'; | ||
| import { render as inkRender } from 'ink'; | ||
| import type { DOMElement } from 'ink'; | ||
| import stripAnsi from 'strip-ansi'; | ||
| import { EventEmitter } from 'node:events'; | ||
| import { createRef, type RefObject } from 'react'; | ||
| import { act } from '@testing-library/react'; | ||
| import { describe, it, expect, vi, beforeEach } from 'vitest'; | ||
| import { Footer } from './Footer.js'; | ||
|
|
@@ -200,6 +202,7 @@ const renderAtLayoutWidth = ( | |
| columns: number, | ||
| uiState: UIState, | ||
| backgroundEntries: readonly DialogEntry[] = [], | ||
| containerRef?: RefObject<DOMElement | null>, | ||
| ) => { | ||
| useTerminalSizeMock.mockReturnValue({ columns, rows: 24 }); | ||
| let lastFrame = ''; | ||
|
|
@@ -231,10 +234,10 @@ const renderAtLayoutWidth = ( | |
| <BackgroundTaskViewStateContext.Provider | ||
| value={createBackgroundTaskState(backgroundEntries)} | ||
| > | ||
| <Footer /> | ||
| <Footer containerRef={containerRef} /> | ||
| </BackgroundTaskViewStateContext.Provider> | ||
| ) : ( | ||
| <Footer /> | ||
| <Footer containerRef={containerRef} /> | ||
| ); | ||
| const instance = inkRender( | ||
| <SettingsContext.Provider value={mockSettings}> | ||
|
|
@@ -280,6 +283,19 @@ describe('<Footer />', () => { | |
| }); | ||
| }); | ||
|
|
||
| it('attaches the selectable-region ref to its outer box', () => { | ||
| const containerRef = createRef<DOMElement>(); | ||
| const { unmount } = renderAtLayoutWidth( | ||
| 80, | ||
| createMockUIState(), | ||
| [], | ||
| containerRef, | ||
| ); | ||
|
|
||
| expect(containerRef.current).not.toBeNull(); | ||
| unmount(); | ||
|
Comment on lines
+295
to
+296
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] R1-3 (round 1, still stands — partially addressed): the added ref test asserts only that Suggested fix: pin the ref to the outer box — e.g. 中文说明R1-3(第 1 轮,仍然存在——仅部分解决):新增的 ref 测试只断言 — qwen3.8-max via Qwen Code /review (v0.21.9) |
||
| }); | ||
|
|
||
| it('passes the left-column width after a right pill reserves space', async () => { | ||
| const originalSandbox = process.env['SANDBOX']; | ||
| process.env['SANDBOX'] = 'qwen-code-docker'; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,8 +4,16 @@ | |
| * SPDX-License-Identifier: Apache-2.0 | ||
| */ | ||
|
|
||
| import { Box, Static } from 'ink'; | ||
| import { memo, useCallback, useEffect, useMemo, useRef, useState } from 'react'; | ||
| import { Box, Static, type DOMElement } from 'ink'; | ||
| import { | ||
| memo, | ||
| useCallback, | ||
| useEffect, | ||
| useMemo, | ||
| useRef, | ||
| useState, | ||
| type RefObject, | ||
| } from 'react'; | ||
| import type { HistoryItem, HistoryItemWithoutId } from '../types.js'; | ||
| import { | ||
| isHistoryItemVisibleAfterRestore, | ||
|
|
@@ -32,6 +40,7 @@ import { | |
| type ScrollableListRef, | ||
| } from './shared/ScrollableList.js'; | ||
| import { TextSelectionController } from '../selection/use-text-selection.js'; | ||
| import { measureElementPosition } from '../utils/measure-element-position.js'; | ||
|
|
||
| // Limit Gemini messages to a very high number of lines to mitigate performance | ||
| // issues in the worst case if we somehow get an enormous response from Gemini. | ||
|
|
@@ -114,7 +123,11 @@ const virtualKeyExtractor = (item: VpItem) => | |
| const virtualIsStaticItem = (item: VpItem) => | ||
| item.type === 'vp-banner' || item.id > 0; | ||
|
|
||
| export const MainContent = () => { | ||
| interface MainContentProps { | ||
| footerRef?: RefObject<DOMElement | null>; | ||
| } | ||
|
|
||
| export const MainContent = ({ footerRef }: MainContentProps) => { | ||
| const { version } = useAppContext(); | ||
| const uiState = useUIState(); | ||
| const { allExpanded: fullDetail } = useThoughtExpanded(); | ||
|
|
@@ -460,6 +473,11 @@ export const MainContent = () => { | |
| <TextSelectionController | ||
| isActive={!uiState.dialogsVisible} | ||
| getViewportRect={() => scrollRef.current?.getViewportRect() ?? null} | ||
| getAdditionalSelectableRects={() => | ||
| footerRef?.current | ||
| ? [measureElementPosition(footerRef.current)] | ||
| : [] | ||
|
DragonnZhang marked this conversation as resolved.
|
||
| } | ||
|
Comment on lines
+476
to
+480
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] R3-2: the production wiring that computes the footer's selectable rect has no end-to-end test — Suggested fix: add one integration-level test asserting 中文说明[Suggestion] R3-2:计算 footer 可选择矩形的生产接线没有任何端到端测试—— — qwen3.8-max via Qwen Code /review (v0.21.10) |
||
| getScrollState={() => | ||
| scrollRef.current?.getScrollState() ?? { | ||
| scrollTop: 0, | ||
|
|
||
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] R1-3 (round 1, still stands): the added ref test asserts only that
containerRef.currentis non-null, which passes no matter which element insideFootercarries the ref — the test name says "outer box" but the body only proves "some box". — Failure scenario: a future edit movesref={containerRef}from the outerwidth="100%"Box to an inner section box →measureElementPositionreturns a rect covering only that section, so part of the statusline silently becomes unselectable and drags clamp mid-footer, while this test stays green.(the outer box spans the full 80-column layout width; an inner section box would not)
中文说明
[Suggestion] R1-3(第 1 轮提出,仍然存在):新增的 ref 测试只断言
containerRef.current非空——无论 ref 挂在Footer内哪个元素上都会通过;测试名说的是 "outer box",但测试体只能证明 "某个 box"。— 失败场景:未来某次修改把ref={containerRef}从外层width="100%"Box 移到某个内部区块 box →measureElementPosition返回的矩形只覆盖该区块,状态栏的一部分会静默变得不可选择、拖拽会在 footer 中间被钳制,而本测试仍然全绿。建议断言几何:外层 box 在 80 列布局下计算宽度为 80,内部区块则不是。— qwen3.8-max via Qwen Code /review (v0.21.10)