Enhance screen clearing and removing flicker on update #157
Conversation
WalkthroughThe changes enhance terminal output management by improving how content is cleared and written in the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant TerminalManager
participant startLiveMonitoring
participant renderLiveDisplay
User->>startLiveMonitoring: Trigger live monitoring
startLiveMonitoring->>TerminalManager: clearScreen()
startLiveMonitoring->>startLiveMonitoring: await small delay
alt No active session block
startLiveMonitoring->>TerminalManager: write("No active block")
else Active block found
startLiveMonitoring->>TerminalManager: clearScreen()
startLiveMonitoring->>startLiveMonitoring: await small delay
startLiveMonitoring->>renderLiveDisplay: Render block
renderLiveDisplay->>TerminalManager: write(block content)
end
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @DonDawsy, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request aims to significantly improve the user experience in terminal applications like Warp and iTerm by eliminating screen flicker during display updates. It achieves this by refining the underlying terminal clearing logic, introducing a mechanism to clear only the previously rendered content, and adding small delays to ensure terminal commands are processed smoothly.
Highlights
- Enhanced Terminal Clearing: The
TerminalManagernow leveragesnode:readlinefor more precise screen clearing (cursorTo,clearScreenDown) and introduces a newclearPreviousContentmethod. This new method clears only the previously rendered output by moving the cursor up and clearing lines, which significantly reduces visual flicker during updates. - Improved Write Operations: The
writemethod inTerminalManagerhas been updated to track the number of lines written (lastContentLines). This line count is crucial for the newclearPreviousContentfunctionality. Additionally, a newwriteRawmethod was introduced for writing text without affecting the line tracking, useful for control characters or non-content output. - Flicker Mitigation in Live Monitoring: Small, asynchronous delays (10ms) have been strategically added after
terminal.clearScreen()calls within thestartLiveMonitoringfunction. This provides terminal emulators (like Warp and iTerm) sufficient time to process the clear command before new content is written, directly addressing the reported flicker issue. A redundantclearScreencall was also removed fromrenderLiveDisplay.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/commands/_blocks.live.ts (2)
101-102: Consider using a more robust delay mechanism.The hardcoded 10ms delay may not be sufficient for all terminal emulators. Consider making this configurable or using a more sophisticated approach.
- // Small delay to ensure terminal processes the clear command - await new Promise(resolve => setTimeout(resolve, 10)); + // Small delay to ensure terminal processes the clear command + await delay(10);This maintains consistency with the
delayfunction already imported and used elsewhere in the code.
118-119: Duplicate delay implementation - consolidate for consistency.The same delay pattern is repeated here. Consider extracting this into a helper function or using the already imported
delayutility.- // Small delay to ensure terminal processes the clear command - await new Promise(resolve => setTimeout(resolve, 10)); + // Small delay to ensure terminal processes the clear command + await delay(10);This approach is more consistent with the existing codebase and provides better maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/_terminal-utils.ts(4 hunks)src/commands/_blocks.live.ts(2 hunks)
🧰 Additional context used
🪛 ESLint
src/commands/_blocks.live.ts
[error] 102-102: Unsafe return of a value of type error.
(ts/no-unsafe-return)
[error] 102-102: Unsafe call of a(n) error type typed value.
(ts/no-unsafe-call)
[error] 119-119: Unsafe return of a value of type error.
(ts/no-unsafe-return)
[error] 119-119: Unsafe call of a(n) error type typed value.
(ts/no-unsafe-call)
src/_terminal-utils.ts
[error] 66-66: Unsafe call of a(n) error type typed value.
(ts/no-unsafe-call)
[error] 66-66: Unsafe member access .cursorTo on an error typed value.
(ts/no-unsafe-member-access)
[error] 67-67: Unsafe call of a(n) error type typed value.
(ts/no-unsafe-call)
[error] 67-67: Unsafe member access .clearScreenDown on an error typed value.
(ts/no-unsafe-member-access)
[error] 77-77: Unexpected any value in conditional. An explicit comparison or type conversion is required.
(ts/strict-boolean-expressions)
[error] 77-77: Unsafe member access .isTTY on an error typed value.
(ts/no-unsafe-member-access)
[error] 79-79: Unsafe call of a(n) error type typed value.
(ts/no-unsafe-call)
[error] 79-79: Unsafe member access .write on an error typed value.
(ts/no-unsafe-member-access)
[error] 81-81: Unsafe call of a(n) error type typed value.
(ts/no-unsafe-call)
[error] 81-81: Unsafe member access .write on an error typed value.
(ts/no-unsafe-member-access)
[error] 117-117: Unsafe call of a(n) error type typed value.
(ts/no-unsafe-call)
[error] 117-117: Unsafe member access .write on an error typed value.
(ts/no-unsafe-member-access)
[error] 127-127: Unsafe call of a(n) error type typed value.
(ts/no-unsafe-call)
[error] 127-127: Unsafe member access .write on an error typed value.
(ts/no-unsafe-member-access)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Socket Security: Pull Request Alerts
🔇 Additional comments (6)
src/_terminal-utils.ts (6)
3-3: Good addition of readline import for better terminal control.Using the native
readlinemodule provides more reliable cursor positioning and screen clearing compared to raw ANSI escape sequences.
33-33: Line tracking property added for content management.The
lastContentLinesproperty enables precise tracking of written content for better clearing operations.
65-71: Improved screen clearing implementation with better reliability.The switch to using
readline.cursorToandreadline.clearScreenDownprovides more reliable screen clearing across different terminal emulators, which directly addresses the flicker issue.
73-84: New method for precise content clearing.The
clearPreviousContentmethod provides fine-grained control over clearing previously written content, which is essential for smooth updates without flicker.
114-121: Enhanced write method with line counting.The line counting logic correctly tracks newlines to support the new clearing functionality.
123-128: Useful addition of writeRaw method.The
writeRawmethod provides a way to write content without affecting line count tracking, which is useful for control sequences and other non-content output.
There was a problem hiding this comment.
Code Review
This pull request aims to fix screen flickering issues in certain terminals by changing the screen clearing mechanism. The changes look good and should address the problem. I've made a few suggestions to improve code consistency and remove potentially unused code.
Overall, a good set of changes to improve the user experience.
|
could you resolve conflicts? |
|
if you all done mention me!!!! |
There was a problem hiding this comment.
Actionable comments posted: 6
🔭 Outside diff range comments (1)
src/_terminal-utils.ts (1)
105-109: Remove leftover code from old implementation.This old implementation using
ansiEscapesshould be removed as it's replaced by the new readline-based approach.- // Only clear screen in TTY environments to prevent issues with non-interactive streams - this.stream.write(ansiEscapes.clearScreen); - this.stream.write(ansiEscapes.cursorTo(0, 0));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/_terminal-utils.ts(4 hunks)
🧰 Additional context used
🪛 ESLint
src/_terminal-utils.ts
[error] 55-55: Unsafe call of a(n) error type typed value.
(ts/no-unsafe-call)
[error] 55-55: Unsafe member access .cursorTo on an error typed value.
(ts/no-unsafe-member-access)
[error] 56-56: Unsafe call of a(n) error type typed value.
(ts/no-unsafe-call)
[error] 56-56: Unsafe member access .clearScreenDown on an error typed value.
(ts/no-unsafe-member-access)
[error] 66-66: Unexpected any value in conditional. An explicit comparison or type conversion is required.
(ts/strict-boolean-expressions)
[error] 66-66: Unsafe member access .isTTY on an error typed value.
(ts/no-unsafe-member-access)
[error] 68-68: Unsafe call of a(n) error type typed value.
(ts/no-unsafe-call)
[error] 68-68: Unsafe member access .write on an error typed value.
(ts/no-unsafe-member-access)
[error] 68-68: Unsafe call of a(n) error type typed value.
(ts/no-unsafe-call)
[error] 68-68: Unsafe member access .MOVE_UP on an error typed value.
(ts/no-unsafe-member-access)
[error] 70-70: Unsafe call of a(n) error type typed value.
(ts/no-unsafe-call)
[error] 70-70: Unsafe member access .write on an error typed value.
(ts/no-unsafe-member-access)
[error] 79-79: Unexpected any value in conditional. An explicit comparison or type conversion is required.
(ts/strict-boolean-expressions)
[error] 79-79: Unsafe member access .isTTY on an error typed value.
(ts/no-unsafe-member-access)
[error] 80-80: Unsafe call of a(n) error type typed value.
(ts/no-unsafe-call)
[error] 80-80: Unsafe member access .write on an error typed value.
(ts/no-unsafe-member-access)
[error] 80-80: Unsafe member access .CLEAR_LINE on an error typed value.
(ts/no-unsafe-member-access)
[error] 88-88: Unexpected any value in conditional. An explicit comparison or type conversion is required.
(ts/strict-boolean-expressions)
[error] 88-88: Unsafe member access .isTTY on an error typed value.
(ts/no-unsafe-member-access)
[error] 89-89: Unsafe call of a(n) error type typed value.
(ts/no-unsafe-call)
[error] 89-89: Unsafe member access .write on an error typed value.
(ts/no-unsafe-member-access)
[error] 89-89: Unsafe call of a(n) error type typed value.
(ts/no-unsafe-call)
[error] 89-89: Unsafe member access .MOVE_UP on an error typed value.
(ts/no-unsafe-member-access)
[error] 97-97: Unexpected any value in conditional. An explicit comparison or type conversion is required.
(ts/strict-boolean-expressions)
[error] 97-97: Unsafe member access .isTTY on an error typed value.
(ts/no-unsafe-member-access)
[error] 98-98: Unsafe call of a(n) error type typed value.
(ts/no-unsafe-call)
[error] 98-98: Unsafe member access .write on an error typed value.
(ts/no-unsafe-member-access)
[error] 98-98: Unsafe call of a(n) error type typed value.
(ts/no-unsafe-call)
[error] 98-98: Unsafe member access .MOVE_TO_COLUMN on an error typed value.
(ts/no-unsafe-member-access)
[error] 117-117: Unsafe call of a(n) error type typed value.
(ts/no-unsafe-call)
[error] 117-117: Unsafe member access .write on an error typed value.
(ts/no-unsafe-member-access)
[error] 127-127: Unsafe call of a(n) error type typed value.
(ts/no-unsafe-call)
[error] 127-127: Unsafe member access .write on an error typed value.
(ts/no-unsafe-member-access)
🔇 Additional comments (3)
src/_terminal-utils.ts (3)
3-3: Good addition for enhanced cursor control.Adding the readline import enables more precise terminal cursor positioning, which aligns well with the PR objective of reducing flicker.
116-121: Good enhancement for line tracking.The modification to track line counts is essential for the new
clearPreviousContent()functionality and supports the flicker reduction objective.
126-128: Good addition for raw output without line tracking.The
writeRaw()method provides a clean way to write content without affecting the line count tracking, which is useful for terminal control sequences.
| clearScreen(): void { | ||
| if (this.stream.isTTY) { | ||
|
|
||
| // Use readline to clear and reposition cursor | ||
| readline.cursorTo(this.stream, 0, 0); | ||
| readline.clearScreenDown(this.stream); | ||
| // Reset line counter for next content | ||
| this.lastContentLines = 0; | ||
| } | ||
| } |
There was a problem hiding this comment.
Critical issue: Leftover code creates duplication.
The method contains both new and old implementations. Lines 105-109 show the old ansiEscapes implementation that should be removed.
clearScreen(): void {
if (this.stream.isTTY) {
-
// Use readline to clear and reposition cursor
readline.cursorTo(this.stream, 0, 0);
readline.clearScreenDown(this.stream);
// Reset line counter for next content
this.lastContentLines = 0;
}
}Also remove the duplicate implementation at lines 105-109.
Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 ESLint
[error] 52-52: Unexpected any value in conditional. An explicit comparison or type conversion is required.
(ts/strict-boolean-expressions)
[error] 52-52: Unsafe member access .isTTY on an error typed value.
(ts/no-unsafe-member-access)
[error] 52-54: Block must not be padded by blank lines.
(style/padded-blocks)
[error] 55-55: Unsafe call of a(n) error type typed value.
(ts/no-unsafe-call)
[error] 55-55: Unsafe member access .cursorTo on an error typed value.
(ts/no-unsafe-member-access)
[error] 56-56: Unsafe call of a(n) error type typed value.
(ts/no-unsafe-call)
[error] 56-56: Unsafe member access .clearScreenDown on an error typed value.
(ts/no-unsafe-member-access)
🤖 Prompt for AI Agents
In src/_terminal-utils.ts around lines 51 to 60 and also lines 105 to 109, the
clearScreen method contains both the new readline-based implementation and the
old ansiEscapes implementation, causing duplication. Remove the old ansiEscapes
code block at lines 105 to 109 entirely to keep only the new readline-based
implementation and avoid redundant code.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/_terminal-utils.ts (2)
52-59: Remove duplicate clearScreen implementation.The method now uses readline correctly, but there's still leftover code at lines 104-108 that creates duplication and should be removed entirely.
88-88: Critical issue: Replace undefined TERMINAL_CONTROL with ANSI escape sequence.The
TERMINAL_CONTROL.MOVE_UP()reference is still undefined and will cause runtime errors.- this.stream.write(TERMINAL_CONTROL.MOVE_UP(lines)); + this.stream.write(`\u001B[${lines}A`);
🧹 Nitpick comments (2)
src/_terminal-utils.ts (2)
16-16: Fix indentation to match project style.The property declaration uses spaces instead of tabs, which doesn't match the project's indentation style.
- private lastContentLines = 0; + private lastContentLines = 0;
77-81: Fix indentation and formatting issues.The method has inconsistent indentation that doesn't match the project style.
-clearLine(): void { - if (this.stream.isTTY) { - this.stream.write('\u001B[2K'); - } -} + clearLine(): void { + if (this.stream.isTTY) { + this.stream.write('\u001B[2K'); + } + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/_terminal-utils.ts(4 hunks)
🧰 Additional context used
🪛 ESLint
src/_terminal-utils.ts
[error] 16-16: Expected indentation of 1 tab but found 2 spaces.
(style/indent)
[error] 54-54: Unsafe call of a(n) error type typed value.
(ts/no-unsafe-call)
[error] 54-54: Unsafe member access .cursorTo on an error typed value.
(ts/no-unsafe-member-access)
[error] 55-55: Unsafe call of a(n) error type typed value.
(ts/no-unsafe-call)
[error] 55-55: Unsafe member access .clearScreenDown on an error typed value.
(ts/no-unsafe-member-access)
[error] 65-65: Unexpected any value in conditional. An explicit comparison or type conversion is required.
(ts/strict-boolean-expressions)
[error] 65-65: Unsafe member access .isTTY on an error typed value.
(ts/no-unsafe-member-access)
[error] 67-67: Unsafe call of a(n) error type typed value.
(ts/no-unsafe-call)
[error] 67-67: Unsafe member access .write on an error typed value.
(ts/no-unsafe-member-access)
[error] 69-69: Unsafe call of a(n) error type typed value.
(ts/no-unsafe-call)
[error] 69-69: Unsafe member access .write on an error typed value.
(ts/no-unsafe-member-access)
[error] 77-77: Expected indentation of 1 tab but found 0.
(style/indent)
[error] 78-78: Expected indentation of 2 tabs but found 4 spaces.
(style/indent)
[error] 78-78: Unexpected any value in conditional. An explicit comparison or type conversion is required.
(ts/strict-boolean-expressions)
[error] 78-78: Unsafe member access .isTTY on an error typed value.
(ts/no-unsafe-member-access)
[error] 79-79: Expected indentation of 3 tabs but found 8 spaces.
(style/indent)
[error] 79-79: Unsafe call of a(n) error type typed value.
(ts/no-unsafe-call)
[error] 79-79: Unsafe member access .write on an error typed value.
(ts/no-unsafe-member-access)
[error] 80-80: Expected indentation of 2 tabs but found 4 spaces.
(style/indent)
[error] 81-81: Expected indentation of 1 tab but found 0.
(style/indent)
[error] 87-87: Unexpected any value in conditional. An explicit comparison or type conversion is required.
(ts/strict-boolean-expressions)
[error] 87-87: Unsafe member access .isTTY on an error typed value.
(ts/no-unsafe-member-access)
[error] 88-88: Unsafe call of a(n) error type typed value.
(ts/no-unsafe-call)
[error] 88-88: Unsafe member access .write on an error typed value.
(ts/no-unsafe-member-access)
[error] 88-88: Unsafe call of a(n) error type typed value.
(ts/no-unsafe-call)
[error] 88-88: Unsafe member access .MOVE_UP on an error typed value.
(ts/no-unsafe-member-access)
[error] 96-96: Unexpected any value in conditional. An explicit comparison or type conversion is required.
(ts/strict-boolean-expressions)
[error] 96-96: Unsafe member access .isTTY on an error typed value.
(ts/no-unsafe-member-access)
[error] 97-97: Unsafe call of a(n) error type typed value.
(ts/no-unsafe-call)
[error] 97-97: Unsafe member access .write on an error typed value.
(ts/no-unsafe-member-access)
[error] 116-116: Unsafe call of a(n) error type typed value.
(ts/no-unsafe-call)
[error] 116-116: Unsafe member access .write on an error typed value.
(ts/no-unsafe-member-access)
[error] 126-126: Unsafe call of a(n) error type typed value.
(ts/no-unsafe-call)
[error] 126-126: Unsafe member access .write on an error typed value.
(ts/no-unsafe-member-access)
🔇 Additional comments (3)
src/_terminal-utils.ts (3)
117-120: Approve the line tracking enhancement.The line counting logic correctly tracks newlines in written content, which is essential for the
clearPreviousContent()functionality. This addresses the core issue of preventing flicker during terminal updates.
122-127: Good addition of writeRaw method.The
writeRawmethod provides a clean way to write content without affecting line tracking, which is useful for control sequences and non-content output.
64-72: Verify the clearPreviousContent implementation.The method correctly implements the core functionality to prevent flicker by moving the cursor up and clearing content. However, ensure this works properly with the terminal emulators mentioned in the PR (Warp and iTerm).
#!/bin/bash # Description: Check if there are any test files that verify terminal behavior # Expected: Find tests related to terminal clearing functionality fd -e ts -e js | xargs rg -l "clearPreviousContent|TerminalManager" | head -10
|
in my environment it still causes flicker hmm |
There was a problem hiding this comment.
Pull Request Overview
This PR enhances the live monitoring display by improving how the terminal is cleared and refreshed to eliminate flicker.
- Introduces small delays after clearing the screen to ensure the terminal processes clear commands.
- Updates
TerminalManager.clearScreento usereadlineAPIs and track the number of written lines. - Adds new methods (
clearPreviousContent,clearLine,moveUp,moveToLineStart,writeRaw) to better control cursor movement and content clearing.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/commands/_blocks.live.ts | Added await delay(10) after each terminal.clearScreen() to prevent flicker. |
| src/_terminal-utils.ts | Switched screen-clearing logic to readline, added line-tracking and new cursor/clearing APIs. |
Comments suppressed due to low confidence (3)
src/_terminal-utils.ts:115
- Consider updating the JSDoc comment for
writeto mention that it now tracks line counts, to keep documentation in sync with behavior.
write(text: string): void {
src/_terminal-utils.ts:64
- There are no tests covering the new
clearPreviousContent,clearLine, and cursor movement methods; consider adding unit tests to verify their behavior.
clearPreviousContent(): void {
src/_terminal-utils.ts:77
- [nitpick] The indentation of this method is inconsistent with other class methods; aligning it will improve readability and maintain consistency.
clearLine(): void {
| if (activeBlock == null) { | ||
| terminal.clearScreen(); | ||
| // Small delay to ensure terminal processes the clear command | ||
| await delay(10); |
There was a problem hiding this comment.
Extract the 10ms magic number into a named constant (e.g., CLEAR_DELAY_MS) for clarity and easier future adjustments.
| await delay(10); | |
| await delay(CLEAR_DELAY_MS); |
|
I'm so sorry but in my env this pr is worth than the main |
|
I'm using m3 pro with ghostty, which is fast enough not to see flicker |
|
Thank you! we merged it!! |
2 participants
When using Warp or iTerm, ccusage will print new blocks instead of clearing the screen, causing flicker on update. This fixes that.
Summary by CodeRabbit