Repository navigation
fix(cli): eliminate banner duplication and drag flicker on resize/wake #8831
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 14 commits
d69f7aa
6bea8b7
0ba19b4
cb1297a
2515655
575ad26
50d755b
c3229b2
dad05dc
c4b5468
4f7b4ba
15d4be6
d2e01e8
7584697
78469b4
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 |
|---|---|---|
|
|
@@ -78,6 +78,7 @@ import { | |
| buildResumedHistoryItems, | ||
| expandCollapsedHistory, | ||
| } from './utils/resumeHistoryUtils.js'; | ||
| import { buildWakeRepaint } from './utils/terminal-resize-reflow.js'; | ||
| import { loadLowlight } from './utils/lowlightLoader.js'; | ||
| import { | ||
| getStickyTodos, | ||
|
|
@@ -624,6 +625,13 @@ interface AppContainerProps { | |
| initializationResult: InitializationResult; | ||
| initialUseVirtualViewport?: boolean; | ||
| extensionRefreshState?: ExtensionRefreshState; | ||
| /** | ||
| * VP wake/SIGCONT repaint: clear the viewport and replay the last frame | ||
| * (Ink skips unchanged-output redraws, so a bare clear would blank the | ||
| * screen). Absent under QWEN_CODE_LEGACY_RESIZE_ERASE: the VP wake path | ||
| * stays write-free (static remount bump only), matching pre-PR behavior. | ||
| */ | ||
| repaintViewport?: () => void; | ||
| } | ||
|
|
||
| /** | ||
|
|
@@ -639,8 +647,13 @@ const SHELL_WIDTH_FRACTION = 0.89; | |
| const SHELL_HEIGHT_PADDING = 10; | ||
|
|
||
| export const AppContainer = (props: AppContainerProps) => { | ||
| const { settings, config, initializationResult, initialUseVirtualViewport } = | ||
| props; | ||
| const { | ||
| settings, | ||
| config, | ||
| initializationResult, | ||
| initialUseVirtualViewport, | ||
| repaintViewport, | ||
| } = props; | ||
| const extensionRefreshState = useMemo( | ||
| () => props.extensionRefreshState ?? new ExtensionRefreshState(), | ||
| [props.extensionRefreshState], | ||
|
|
@@ -1287,15 +1300,16 @@ export const AppContainer = (props: AppContainerProps) => { | |
| }, []); | ||
|
|
||
| // In VP mode (ui.useTerminalBuffer) the React tree fully owns the visible | ||
| // region via ink 7 native overflow clipping. Writing clearTerminal / | ||
| // cursorTo+eraseDown would be a wasted flash and would also corrupt the | ||
| // in-app scroll position. The remount-key bump is also a near-no-op for | ||
| // VP: nothing in the VP render path is keyed by historyRemountKey, so | ||
| // keeping the bump is harmless because the startup-scoped VP decision | ||
| // is intentionally restart-only to match Ink's alternateScreen lifetime. | ||
| // The visible refresh in VP mode comes for free from the React tree | ||
| // re-reading `mergedHistory` / `allVirtualItems` on whatever state | ||
| // change triggered refreshStatic (Ctrl+O, model change, etc.). | ||
| // region via ink 7 native overflow clipping. The remount-key bump is | ||
| // write-free but not inert: one-shot <Static> output keyed by it (agent | ||
| // tab history in AgentChatContent) is only re-emitted on a bump. | ||
| // refreshStatic must stay write-free in VP: ordinary callers (Ctrl+O, | ||
| // model change, /clear, ...) get their visible refresh from the state | ||
| // change that triggered them, and replaying the pre-change frame would | ||
| // flash stale content. Only the wake/SIGCONT path (wakeRepaint below) does | ||
| // a physical clear-and-replay, because there the terminal buffer may be | ||
| // stale or rearranged while Ink both erases with a stale relative count | ||
| // and skips redraws whose output is unchanged. | ||
| const [useTerminalBuffer] = useState( | ||
| () => | ||
| initialUseVirtualViewport ?? | ||
|
|
@@ -1305,14 +1319,50 @@ export const AppContainer = (props: AppContainerProps) => { | |
| isInteractiveTerminal(), | ||
| ), | ||
| ); | ||
|
|
||
| // The VP post-shrink clear window (terminal-resize-reflow) wipes one-shot | ||
| // <Static> content from the viewport just like the wake path; pair it with | ||
| // the same remount bump so keyed statics (agent tab history) re-emit. | ||
| const prevTerminalWidthRef = useRef(terminalWidth); | ||
| useEffect(() => { | ||
| const prev = prevTerminalWidthRef.current; | ||
| prevTerminalWidthRef.current = terminalWidth; | ||
| if (useTerminalBuffer && terminalWidth < prev) { | ||
|
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] Three things about the new VP shrink-remount effect (R6-6 already covers the missing test, so not repeated here): (a) it bumps Suggested fix: bump only on the first shrink of a burst (when no timer is pending); gate on 中文说明新的 VP 缩窄 remount effect 有三个问题(R6-6 已覆盖缺测试,此处不重复):(a) 每次缩窄渲染都立即整体重发 static 历史——拖拽缩窄的每个宽度 tick 都触发一次,而窗口内每次重发又会被下一次
Collaborator
Author
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. Fixed in 936b1ad: (a) the immediate bump fires only on the first shrink of a burst (no timer pending); in-window ticks just reschedule the window-end re-bump; (b) the effect is gated on repaintViewport being present, which is undefined exactly under QWEN_CODE_LEGACY_RESIZE_ERASE (wrapper no-op), restoring pre-PR no-bump behavior there; (c) the delay is CLEAR_WINDOW_MS + 50 via the exported constant, no duplicated literal. |
||
| remountStaticHistory(); | ||
| } | ||
|
chiga0 marked this conversation as resolved.
|
||
| }, [terminalWidth, useTerminalBuffer, remountStaticHistory]); | ||
|
|
||
| const showScrollbar = settings.merged.ui?.showScrollbar ?? true; | ||
| const refreshStatic = useCallback(() => { | ||
| if (!useTerminalBuffer) { | ||
| stdout.write(ansiEscapes.clearTerminal); | ||
| } | ||
| // VP stays write-free for ordinary callers (/clear, model change, Ctrl+O, | ||
| // ...): replaying the pre-change frame would flash stale content. Their | ||
| // visible refresh comes from the state change that triggered them. The | ||
| // wake/SIGCONT path repaints separately via useWakeRepaint below. | ||
| remountStaticHistory(); | ||
| }, [useTerminalBuffer, remountStaticHistory, stdout]); | ||
|
|
||
| // Wake/SIGCONT: the terminal buffer may be stale or rearranged, and Ink | ||
| // both erases with a stale relative count and skips redraws whose output | ||
| // is unchanged — so VP repaints by replaying the last frame over a clean | ||
| // viewport (viewport-only: clearTerminal's 3J would destroy scrollback / | ||
| // Warp history) and bumps the static remount key so one-shot <Static> | ||
| // history (agent tabs) is re-emitted over the clear. Static mode uses the | ||
| // ordinary refreshStatic. Selection extracted (buildWakeRepaint) for unit | ||
| // coverage. | ||
| const wakeRepaint = useMemo( | ||
| () => | ||
| buildWakeRepaint({ | ||
| isVP: useTerminalBuffer, | ||
| repaintViewport, | ||
| refreshStatic, | ||
| remountStaticHistory, | ||
| }), | ||
| [useTerminalBuffer, repaintViewport, refreshStatic, remountStaticHistory], | ||
| ); | ||
|
|
||
| // Keep the static header in sync with model changes without polling. | ||
| // Ink's <Static> output is append-only, so model changes must explicitly | ||
| // clear and remount the static region to redraw the banner at the top. | ||
|
|
@@ -3402,7 +3452,7 @@ export const AppContainer = (props: AppContainerProps) => { | |
| // display sleep, Ctrl+Z → fg). The terminal's screen buffer is stale but | ||
| // Ink's frame-diff state still reflects the pre-sleep output, so the next | ||
| // render strands border characters on screen. | ||
| useWakeRepaint(refreshStatic); | ||
| useWakeRepaint(wakeRepaint); | ||
|
chiga0 marked this conversation as resolved.
|
||
|
|
||
| useEffect(() => { | ||
| if (ideNeedsRestart) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -35,6 +35,7 @@ import { | |
| pushKittyProtocolFlags, | ||
| } from './utils/kittyProtocolDetector.js'; | ||
| import { installTerminalRedrawOptimizer } from './utils/terminalRedrawOptimizer.js'; | ||
| import { installTerminalResizeReflow } from './utils/terminal-resize-reflow.js'; | ||
|
chiga0 marked this conversation as resolved.
|
||
| import { installSynchronizedOutput } from './utils/synchronizedOutput.js'; | ||
| import { | ||
| isInteractiveTerminal, | ||
|
|
@@ -164,6 +165,15 @@ export async function startInteractiveUI( | |
| isInteractiveTerminal(), | ||
| ); | ||
|
|
||
| // On width shrink the terminal reflows the printed frame into more physical | ||
| // rows than Ink's stale erase count (issue #8557); amplify the clear to the | ||
| // reflowed height. Installed before render() so the resize listener runs | ||
| // ahead of Ink's resized(). | ||
| const resizeReflow = | ||
|
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] No regression test drives a real terminal through the shrink path. The 38 unit tests validate the erase-amplification model against the wrapper's own reflow assumption — Failure scenario: the width model diverges from a real terminal's reflow, and the issue's exact symptom (duplicated/stacked transcript on shrink) returns on real terminals while all unit tests stay green. Suggested fix: commit a PTY/tmux-based regression in the integration harness that shrinks a real terminal mid-session and asserts a single reprint; at minimum, capture the byte-level shrink rig output as a fixture. 中文说明没有任何回归测试驱动真实终端走缩窄路径:38 个单测用 FakeStdout + 与生产代码同一套打包逻辑验证放大模型,模型与测试共享任何宽度计算错误;#8557 线程里作者自己已记录过 Warp 上的真实分歧(CJK 字体回退把
Collaborator
Author
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. Accepted as a known limitation, deferred: a PTY/tmux shrink regression would not share the width-model assumption, but the repo's interactive tmux harness is flaky-prone in CI and the byte-level rig would still bind to one terminal's reflow semantics (Warp's CJK fallback diverges from any app-side model, as documented in this thread). Mitigations in place: 38+ unit tests pin the wrapper contract, and the fix was author-verified on Ghostty and Warp (live-view duplication gone; Warp block-history snapshots remain platform behavior, documented). Tracking a real-terminal shrink regression as follow-up work; not blocking this PR. |
||
| process.stdout.isTTY && !config.getScreenReader() | ||
| ? installTerminalResizeReflow(process.stdout, { virtualViewport: useVP }) | ||
| : { restore: () => {}, repaint: () => {} }; | ||
|
|
||
| // Create wrapper component to use hooks inside render | ||
| const AppWrapper = () => { | ||
| const kittyProtocolStatus = useKittyKeyboardProtocol(); | ||
|
|
@@ -195,6 +205,7 @@ export async function startInteractiveUI( | |
| initializationResult={initializationResult} | ||
| initialUseVirtualViewport={useVP} | ||
| extensionRefreshState={options.extensionRefreshState} | ||
| repaintViewport={resizeReflow.repaint} | ||
| /> | ||
| </BackgroundTaskViewProvider> | ||
| </AgentViewProvider> | ||
|
|
@@ -313,6 +324,10 @@ export async function startInteractiveUI( | |
| if (useVP) { | ||
| process.stdout.setMaxListeners(stdoutMaxListeners); | ||
| } | ||
| // Unwind the stdout.write wrapper stack in LIFO order (resizeReflow is | ||
| // installed last / outermost); the identity-guarded restores silently | ||
| // no-op and leak wrappers otherwise. | ||
| resizeReflow.restore(); | ||
| restoreSynchronizedOutput(); | ||
|
chiga0 marked this conversation as resolved.
|
||
| restoreTerminalRedrawOptimizer(); | ||
| // If the ErrorBoundary caught a rendering error, echo it to stderr | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.