fix(cli): add TUI flicker foundation fixes#3591
Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR lays groundwork to reduce TUI flicker and repeated-output rerenders by improving terminal serialization, buffering/throttling streamed UI updates, and adding redraw/synchronized-output instrumentation.
Changes:
- Add terminal text serialization + optional wrapped-line unwrapping for stable comparisons.
- Reduce live shell viewport rerenders caused solely by soft-wrap reflow; add related tests.
- Add redraw counters and introduce synchronized terminal output support with allowlisting + tests.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/core/src/utils/terminalSerializer.ts | Adds serializeTerminalToText() and optional wrapped-line unwrapping for object serialization. |
| packages/core/src/utils/terminalSerializer.test.ts | Adds coverage for wrapped-line unwrapping and transcript text behavior. |
| packages/core/src/services/shellExecutionService.ts | Uses new terminal text serializer and compares ANSI output in a wrap-stable way to avoid redundant emits. |
| packages/core/src/services/shellExecutionService.test.ts | Adds tests for transcript/newline semantics and rerender suppression; updates mocks/helpers. |
| packages/cli/src/ui/utils/terminalRedrawOptimizer.ts | Tracks stdout writes/bytes and clear/erase optimization counters. |
| packages/cli/src/ui/utils/terminalRedrawOptimizer.test.ts | Adds assertions for the new redraw stats counters. |
| packages/cli/src/ui/utils/synchronizedOutput.ts | Introduces synchronized-output framing (BSU/ESU) with allowlist + stats. |
| packages/cli/src/ui/utils/synchronizedOutput.test.ts | Adds tests for support detection, framing, callback preservation, and composition with redraw optimizer. |
| packages/cli/src/ui/hooks/useGeminiStream.ts | Buffers/throttles content/thought stream updates and flushes on important events/cancel. |
| packages/cli/src/ui/hooks/useGeminiStream.test.tsx | Adds fake-timer tests for throttling and flush-on-cancel behavior. |
| packages/cli/src/ui/components/messages/ToolMessage.tsx | Pre-slices oversized string outputs by visual width/height to reduce Ink layout pressure. |
| packages/cli/src/ui/components/messages/ToolMessage.test.tsx | Adds tests for pre-slicing multi-line and single-line outputs and exact-fit behavior. |
| packages/cli/src/ui/AppContainer.tsx | Separates “remount-only” vs clearTerminal + remount refresh to avoid duplicate clears. |
| packages/cli/src/ui/AppContainer.test.tsx | Adds tests ensuring /clear avoids double clearTerminal while refreshStatic still clears. |
| packages/cli/src/gemini.tsx | Installs synchronized output framing alongside redraw optimizer and restores on exit. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| while (bufferedEvents.length > 0) { | ||
| const nextEvent = bufferedEvents.shift()!; | ||
|
|
||
| if (nextEvent.kind === 'content') { | ||
| let mergedContent = nextEvent.value; | ||
|
|
||
| while (bufferedEvents[0]?.kind === 'content') { | ||
| const queuedContent = bufferedEvents.shift(); | ||
| if (queuedContent?.kind !== 'content') { | ||
| break; | ||
| } | ||
| mergedContent += queuedContent.value; | ||
| } |
There was a problem hiding this comment.
flushBufferedStreamEvents()repeatedly usesArray.shift(), which is O(n) per call and can become O(n²) during heavy streaming (many buffered events within a throttle window). Consider iterating with an index (or swapping buffers: copy current array, then reset bufferedEvents.length = 0`) to process in O(n), and merge adjacent items during the single pass without shifting from the front.
| let newOutputComparison: AnsiOutput; | ||
| if (shellExecutionConfig.showColor) { | ||
| newOutput = serializeTerminalToObject(headlessTerminal); | ||
| newOutputComparison = serializeTerminalToObject( | ||
| headlessTerminal, | ||
| 0, | ||
| { unwrapWrappedLines: true }, | ||
| ); | ||
| } else { | ||
| const buffer = headlessTerminal.buffer.active; | ||
| const lines: AnsiOutput = []; | ||
| for (let y = 0; y < headlessTerminal.rows; y++) { | ||
| const line = buffer.getLine(buffer.viewportY + y); | ||
| const lineContent = line ? line.translateToString(true) : ''; | ||
| lines.push([ | ||
| { | ||
| text: lineContent, | ||
| bold: false, | ||
| italic: false, | ||
| underline: false, | ||
| dim: false, | ||
| inverse: false, | ||
| fg: '', | ||
| bg: '', | ||
| }, | ||
| ]); | ||
| } | ||
| newOutput = lines; | ||
| } | ||
|
|
||
| let lastNonEmptyLine = -1; | ||
| for (let i = newOutput.length - 1; i >= 0; i--) { | ||
| const line = newOutput[i]; | ||
| if ( | ||
| line | ||
| .map((segment) => segment.text) | ||
| .join('') | ||
| .trim().length > 0 | ||
| ) { | ||
| lastNonEmptyLine = i; | ||
| break; | ||
| } | ||
| newOutput = serializePlainViewportToAnsiOutput(headlessTerminal); | ||
| newOutputComparison = serializePlainViewportToAnsiOutput( | ||
| headlessTerminal, | ||
| true, | ||
| ); | ||
| } |
There was a problem hiding this comment.
Each live update currently serializes the viewport twice (raw + unwrap-for-comparison). On frequent PTY data this doubles the hottest-path work. If possible, consider deriving the comparison form from the already-produced output (e.g., post-process the raw AnsiOutput to merge wrapped lines when showColor is false, or refactor serializers to optionally return both representations in one pass) to cut CPU cost while keeping the wrap-stable comparison behavior.
| const sliced = sliceTextForMaxHeight( | ||
| displayData, | ||
| availableHeight, | ||
| childWidth, | ||
| ); |
There was a problem hiding this comment.
String pre-slicing walks the entire output (codepoints + width measurement). Called directly during render, this can become expensive if parent re-renders while displayData is large. Memoizing the slice (e.g., via useMemo keyed on displayData, availableHeight, and childWidth) would avoid repeated full scans when inputs are unchanged.
| for (let attempt = 0; attempt < 20; attempt++) { | ||
| const dataEvents = onOutputEventMock.mock.calls.filter( | ||
| ([event]) => event.type === 'data', | ||
| ); | ||
| if (dataEvents.length >= expectedCount) { | ||
| return; | ||
| } | ||
| await new Promise((resolve) => setTimeout(resolve, 0)); | ||
| } |
There was a problem hiding this comment.
If the expected number of data events never arrives, this helper silently returns after the loop, which can allow tests to proceed without actually meeting the condition (false positives). It should throw an error when the loop completes without reaching expectedCount so the test fails deterministically.
| for (let attempt = 0; attempt < 20; attempt++) { | |
| const dataEvents = onOutputEventMock.mock.calls.filter( | |
| ([event]) => event.type === 'data', | |
| ); | |
| if (dataEvents.length >= expectedCount) { | |
| return; | |
| } | |
| await new Promise((resolve) => setTimeout(resolve, 0)); | |
| } | |
| let observedCount = 0; | |
| for (let attempt = 0; attempt < 20; attempt++) { | |
| const dataEvents = onOutputEventMock.mock.calls.filter( | |
| ([event]) => event.type === 'data', | |
| ); | |
| observedCount = dataEvents.length; | |
| if (observedCount >= expectedCount) { | |
| return; | |
| } | |
| await new Promise((resolve) => setTimeout(resolve, 0)); | |
| } | |
| throw new Error( | |
| `Timed out waiting for ${expectedCount} data events; only received ${observedCount}.`, | |
| ); |
|
Local E2E validation completed for this PR branch. The evidence files are retained locally and are intentionally not linked here with local absolute paths. I will attach or summarize shareable comparison media separately if needed. Results:
Review finding calibration:
|
wenshao
left a comment
There was a problem hiding this comment.
No issues found. LGTM! ✅ — gpt-5.5 via Qwen Code /review
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
|
I will add gif and video as soon. |
tmux end-to-end verificationRan a verification harness against the PR-branch source via A.
|
…#125) Backport of the core foundation pieces from upstream QwenLM#3591 (TUI flicker foundation fixes). Two interventions on stdout, both no-op outside a TTY or under a screen reader: 1. terminalRedrawOptimizer (from upstream QwenLM#3381 → QwenLM#3591): wraps stdout.write to collapse Ink's per-line {ERASE_LINE, CURSOR_UP_ONE} sequences into a single {CURSOR_UP_N, erase_at_each, CURSOR_UP_N, CURSOR_LEFT}. Eliminates the scrollback-bouncing during streaming renders. Bypass via PROTO_LEGACY_ERASE_LINES=1. 2. synchronizedOutput (from QwenLM#3591): wraps each render frame in BSU/ESU escape codes (\\e[?2026h / \\e[?2026l) on supporting terminals (Kitty, WezTerm, iTerm) so Ink frames are committed atomically. Terminal allowlist with auto-detect; opt-out via PROTO_DISABLE_SYNCHRONIZED_OUTPUT=1, force on via PROTO_FORCE_SYNCHRONIZED_OUTPUT=1. Both installed in startInteractiveUI() before the Ink render call; both restored in the registerCleanup callback. Deferred from upstream QwenLM#3591 (too entangled with our fork's useGeminiStream.ts / ToolMessage.tsx divergence; revisit later): - main-stream event buffering with flush timer - tool output pre-slicing by visual height - shell soft-wrap-only rerender suppression The two installed pieces cover the user-visible streaming flicker case directly; the deferred pieces are about huge-tool-output and narrow-terminal soft-wrap edge cases. Co-authored-by: Automaker <automaker@localhost> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…, shell wraps, Alacritty) (#127) * fix(core): preserve shell transcript across narrow wraps * fix(core): suppress soft-wrap-only shell rerenders * fix(core): compare default shell output by logical wraps * feat(cli): add Alacritty + Ghostty to synchronized output allowlist + docs Auto-detect Alacritty (ALACRITTY_WINDOW_ID, TERM=alacritty[-direct]) and Ghostty (TERM_PROGRAM=ghostty, GHOSTTY_RESOURCES_DIR). Both support DEC mode 2026 — Alacritty since v0.14 (Jan 2024), Ghostty since launch. Docs: README "Environment variable overrides" table now lists the three flicker-related env vars (PROTO_LEGACY_ERASE_LINES, PROTO_FORCE_SYNCHRONIZED_OUTPUT, PROTO_DISABLE_SYNCHRONIZED_OUTPUT) and adds a "TUI flicker mitigation" section explaining the two interventions and the auto-detected terminal allowlist. Tests: 5 new entries in the parametrized auto-detect test. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(cli): pre-slice tool output by visual height before Ink layout Backport of upstream QwenLM#3591 (commits 172df3d + 9ac3c11). Adds sliceTextForMaxHeight() to ToolMessage.tsx that walks tool output by code-point and tracks visual width per terminal column, slicing to maxHeight before the string enters Ink's <Text wrap="wrap">. Without this, a single 160k-character JSON/base64 line never gets sliced (lines.length === 1 <= maxHeight), but Ink still computes layout over the full unbounded string — the source of the "Ink hangs on huge tool output" symptom. The hidden line count is passed to MaxSizedBox via additionalHiddenLinesCount so the "+ N hidden lines" indicator stays accurate. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(cli): throttle streamed Content/Thought events with 60ms flush timer Backport of upstream QwenLM#3591 (commit b4428f3). Wraps the for-await event loop with a per-stream buffered queue that coalesces consecutive Content (or Thought) events into a single React state update. Without this, every streamed token chunk triggered an Ink redraw — the main source of streaming flicker. - bufferedEvents queue + flushTimer (STREAM_UPDATE_THROTTLE_MS = 60). - flushBufferedStreamEvents() merges consecutive same-kind events before dispatching to handleContentEvent / handleThoughtEvent. - All non-Content/Thought events flush immediately to preserve order. - Retry path discards buffered partial content from the failed attempt. - try/finally guarantees a final flush on stream end or throw. - flushBufferedStreamEventsRef exposes the flusher to cancelOngoingRequest so partial output still commits to history when the user hits Esc. Adapted from upstream: - Our fork removed dualOutput.startAssistantMessage/processEvent calls. - Our HookSystemMessage handler treats the message as content (Ralph Loop iteration info) rather than upstream's stop-hook history item; we flush before processing to preserve ordering. - Upstream's UserPromptSubmitBlocked / StopHookLoop / dualOutput helpers don't exist in our fork — handled implicitly via the exhaustive switch. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: 秦奇 <gary.gq@alibaba-inc.com> Co-authored-by: Automaker <automaker@localhost> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(cli): reduce main screen flicker * fix(cli): pre-slice large tool text output * fix(cli): slice tool output by visual height * fix(core): preserve shell transcript across narrow wraps * fix(core): suppress soft-wrap-only shell rerenders * fix(core): compare default shell output by logical wraps * fix(cli): gate synchronized terminal output --------- Co-authored-by: 秦奇 <gary.gq@alibaba-inc.com>
Summary
This PR supersedes the stacked PRs #3584, #3586, #3587 and #3588 by rebasing the effective implementation onto the latest
mainas one reviewable foundation PR.It intentionally focuses on the foundation layer for TUI flicker and repeated-output mitigation. It does not claim to fully close every TUI flicker / long-output / detail-panel issue; the remaining work is called out below.
What this PR fixes
clearTerminalwrites on already-cleared/clearpaths while preserving conservativerefreshStatic()behavior for Static replacement cases.Text wrapas unbounded visual rows.Related issue coverage
Strong candidate for closure after validation:
Regression / reference coverage:
Partially mitigated but not fully closed by this PR:
refreshStatic()replacement paths can still clear on resize / view switch / settings / compact merge.Local E2E validation
Validation was run locally against this PR branch, not only through unit/type/lint checks. The evidence files are retained locally and are intentionally not linked from this PR.
Validated scenarios:
doneCount=1,failCount=0,shellCompletedCount=1,pass=true.Effect comparison
Before/after comparison media will be attached separately if needed. Do not rely on local absolute paths in the PR description.
Scenarios to compare:
Validation
cd packages/cli && npx vitest run src/ui/AppContainer.test.tsx src/ui/hooks/useGeminiStream.test.tsx src/ui/utils/terminalRedrawOptimizer.test.ts src/ui/utils/synchronizedOutput.test.ts src/ui/components/messages/ToolMessage.test.tsxcd packages/core && npx vitest run src/services/shellExecutionService.test.ts src/utils/terminalSerializer.test.tsnpm run typecheck --workspace=packages/clinpm run typecheck --workspace=packages/corenpm run lint --workspace=packages/clinpm run lint --workspace=packages/coregit diff --check origin/main..HEADRemaining planned work
Follow-up closure PRs should be grouped as:
Closure-A: managed viewport + Static replacement, to remove replacement-stylerefreshStatic()full-screen clear paths.Closure-B: bounded detail + virtualized long output, covering subagent detail, tool result budgeting, markdown-heavy output, and scroll/readback.Closure-C: shell E2E + terminal frame renderer, covering JetBrains / Windows / cmux / tmux / SSH validation and runtime terminal probing.