-
Notifications
You must be signed in to change notification settings - Fork 3k
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 all 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,69 @@ 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. The | ||
| // window's CLEAR_VIEWPORT substitutes wipe the just-re-emitted statics on | ||
| // every in-window redraw, so bump again once the window closes. | ||
| const prevTerminalWidthRef = useRef(terminalWidth); | ||
| const shrinkRemountTimerRef = useRef<ReturnType<typeof setTimeout> | null>( | ||
| null, | ||
| ); | ||
| 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(); | ||
| if (shrinkRemountTimerRef.current) { | ||
| clearTimeout(shrinkRemountTimerRef.current); | ||
| } | ||
| // Slightly past CLEAR_WINDOW_MS (600) so the last window clear lands | ||
| // before the re-emit. | ||
| shrinkRemountTimerRef.current = setTimeout(remountStaticHistory, 650); | ||
| } | ||
|
chiga0 marked this conversation as resolved.
|
||
| }, [terminalWidth, useTerminalBuffer, remountStaticHistory]); | ||
| useEffect( | ||
| () => () => { | ||
| if (shrinkRemountTimerRef.current) { | ||
| clearTimeout(shrinkRemountTimerRef.current); | ||
| } | ||
| }, | ||
| [], | ||
| ); | ||
|
|
||
| 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 +3471,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(); | ||
|
Comment on lines
+327
to
+330
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] R6-7: The only production call site establishing the wrapper stack — the install gate ( If a full 中文说明[Suggestion] R6-7:建立 wrapper 栈的唯一生产调用点——安装门( 若完整的 — qwen3.8-max via Qwen Code /review (v0.21.9) |
||
| restoreSynchronizedOutput(); | ||
|
chiga0 marked this conversation as resolved.
|
||
| restoreTerminalRedrawOptimizer(); | ||
| // If the ErrorBoundary caught a rendering error, echo it to stderr | ||
|
|
||
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] R6-6: The new VP width-shrink remount effect has no test anywhere in the diff or the existing suite. The only resize-remount assertion is the #8004 test, which GROWS the width (80 → 100) in STATIC mode (
useTerminalBuffer: false) and asserts the key does not change — it cannot see this effect. — Concrete cost (mutation-verified):terminalWidth < prev→terminalWidth > prev, or deleting the effect, leaves every test green. The regression that would then ship uncaught: after a VP-mode terminal shrink, the reflow wrapper's2J+Hviewport clear wipes one-shot<Static>agent-tab history, and without the bump that history never re-emits until the next unrelated remount trigger — a visible-loss artifact of the same class this PR fixes. The wake path's identical bump IS covered (buildWakeRepaintunit tests); only the shrink path's is not.Add a case in the #8004 style with VP settings: deliver a width shrink (e.g. 100 → 80) via the same
resizeListenersmechanism and assertcapturedUIState.historyRemountKeyincremented; optionally a companion case asserting a grow or a static-mode shrink does not bump.中文说明
[Suggestion] R6-6:新增的 VP 宽度缩窄 remount effect 在 diff 与现有套件中均无测试。唯一的 resize-remount 断言是 #8004 测试,它在 STATIC 模式(
useTerminalBuffer: false)下把宽度增大(80 → 100)并断言 key 不变——无法覆盖该 effect。— 具体代价(已变异验证):把terminalWidth < prev改成terminalWidth > prev,或删除整个 effect,所有测试仍为绿。随后会无声合入的回归:VP 模式缩窄终端后,reflow wrapper 的2J+H视口清除会清掉一次性<Static>agent 标签页历史,若没有该自增,历史在下一次无关的 remount 触发之前永不重新发出——与本 PR 修复目标同类的可见丢失缺陷。wake 路径的相同自增已有覆盖(buildWakeRepaint单测);只有缩窄路径没有。按 #8004 风格补一个 VP 配置用例:通过同一
resizeListeners机制投递一次宽度缩窄(如 100 → 80),断言capturedUIState.historyRemountKey自增;可再加一个用例断言变宽或 static 模式缩窄不自增。— qwen3.8-max via Qwen Code /review (v0.21.9)