fix(cli): reduce main screen flicker#3584
Closed
chiga0 wants to merge 4 commits into
Closed
Conversation
Contributor
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. |
Contributor
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 reduces Ink TUI flicker on the main assistant streaming path by batching high-frequency stream updates, adds observability for terminal redraw behavior, and removes a redundant full-screen clear on /clear.
Changes:
- Added terminal redraw stats counters (writes/bytes/clears/optimizations) to the terminal redraw optimizer.
- Throttled streamed assistant content/thought updates in
useGeminiStreamwith forced flush boundaries for correctness. - Split out a remount-only static history refresh to avoid duplicate terminal clears on
/clear.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/cli/src/ui/utils/terminalRedrawOptimizer.ts | Adds redraw stats tracking and refactors erase-lines optimization to return counts. |
| packages/cli/src/ui/utils/terminalRedrawOptimizer.test.ts | Adds tests asserting the new redraw stats counters. |
| packages/cli/src/ui/hooks/useGeminiStream.ts | Buffers/throttles streamed content + thought updates and flushes at key boundaries. |
| packages/cli/src/ui/hooks/useGeminiStream.test.tsx | Adds fake-timer tests validating buffering/flush-on-cancel behavior. |
| packages/cli/src/ui/AppContainer.tsx | Introduces remount-only static history refresh and avoids redundant clearTerminal on /clear. |
| packages/cli/src/ui/AppContainer.test.tsx | Tests clear vs remount-only behavior and validates slash command refresh callback semantics. |
Comments suppressed due to low confidence (1)
packages/cli/src/ui/utils/terminalRedrawOptimizer.ts:171
- This wrapper incorrectly forwards
encodingOrCallbacktostream.write. When callers use the overloadwrite(chunk, cb),encodingOrCallbackis a function, but this code always passes it as theencodingargument, which can cause runtime failures (e.g., invalid/unknown encoding) and prevents the callback from being invoked correctly. Fix by forwarding overloads properly (e.g., detecttypeof encodingOrCallback === 'function'and calloriginalWrite.call(this, optimizedChunk, encodingOrCallback)), or by preserving the original arguments and only substitutingchunk.
return originalWrite.call(
this,
optimizedChunk as string | Uint8Array,
encodingOrCallback as BufferEncoding,
callback,
);
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+1154
to
1166
| 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.
Using Array.prototype.shift() in a loop is O(n) per operation because it re-indexes the array, which can become O(n²) if many events accumulate (e.g., if the event loop is busy and the 60ms flush is delayed). To keep the throttling path reliably cheap under load (the main goal of this PR), consider iterating with an index pointer and clearing the array at the end, or switching to a small deque/ring-buffer approach.
Comment on lines
708
to
713
| historyManager.addItem, | ||
| historyManager.clearItems, | ||
| historyManager.loadHistory, | ||
| refreshStatic, | ||
| remountStaticHistory, | ||
| toggleVimEnabled, | ||
| isProcessing, |
There was a problem hiding this comment.
This changes the callback passed into useSlashCommandProcessor from a 'clear + remount' behavior (refreshStatic) to a 'remount-only' behavior (remountStaticHistory). If the hook’s parameter is still conceptually/nominally refreshStatic (as suggested by the test mock naming), that becomes a contract mismatch and can lead to future callers accidentally assuming the terminal is cleared. Consider updating the hook API/name(s) to reflect the new semantics explicitly (e.g., separate parameters for clearAndRemount vs remountOnly, or rename the existing parameter to remountStaticHistory).
Collaborator
Author
|
Superseded by #3591, which consolidates the effective PR1-PR4 TUI flicker foundation changes onto the latest main as one reviewable PR. The new PR also documents what is fixed now and what remains for follow-up Closure-A/B/C work. |
5 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR implements the first focused slice of the TUI flicker work: reducing main-screen flicker on the normal assistant response path while keeping the change surface small and verifiable.
It intentionally does not take the large-output/detail-panel changes from #3013 as a base. Instead, #3013 was used only as prior exploration while this PR focuses on the PR-1 scope from the TUI optimization plan.
What Changed
1. Add terminal redraw observability
Adds lightweight counters to the existing terminal redraw optimizer:
stdoutWriteCountstdoutBytesclearTerminalCounteraseLinesOptimizedCountThese counters let follow-up PRs measure whether changes reduce write frequency and full-screen clears instead of relying only on visual inspection.
2. Throttle streamed assistant content/thought updates
useGeminiStreamnow buffers high-frequency streamed content and thought chunks and flushes them on a 60ms cadence.Forced flush boundaries are preserved for correctness:
This keeps the existing
findLastSafeSplitPoint()progressive promotion behavior intact while avoiding a React/Ink re-render for every tiny stream chunk.3. Avoid duplicate full-screen clearing on
/clearThe previous clear path could do both:
console.clear()viaclearScreen()stdout.write(ansiEscapes.clearTerminal)viarefreshStatic()This PR splits out a private
remountStaticHistory()helper and uses it only where the screen has already been cleared. That avoids the second full-screen clear while preserving/clearsemantics.Important Scope Boundaries
This PR deliberately leaves these areas unchanged:
The last point is intentional: Ink
<Static>is append-only. For already-rendered static history, remounting without clearing can duplicate old history or leave stale view output behind. Those paths need a separate renderer/static replacement strategy before removing clears safely.Expected Impact
/clearshould avoid a redundant second full-screen clear.Effect Comparison
/clearbehaviorValidation
Ran targeted tests:
cd packages/cli npx vitest run src/ui/utils/terminalRedrawOptimizer.test.ts src/ui/hooks/useGeminiStream.test.tsx src/ui/AppContainer.test.tsxResult:
Ran additional checks:
All passed.
Follow-ups
Planned follow-up PRs remain separate: