Add extra caution to terminal color detection #963
Conversation
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (1)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including You can disable this status message by setting the 📝 WalkthroughWalkthroughThe pull request introduces modifications to several terminal-related shell scripts. A new logging function Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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
Documentation and Community
|
![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 (1)
rootfs/etc/profile.d/_07-term-mode.sh (1)
38-43: Robust Check for /dev/tty Read/Write AccessThe added check for both writable and readable
/dev/tty—along with informative trace messages—provides clear guidance when the terminal isn’t fully accessible. This helps prevent misinterpretation of terminal capabilities and informs the user how to address permission issues.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
rootfs/etc/profile.d/_07-term-mode.sh(5 hunks)rootfs/etc/profile.d/_10-colors.sh(2 hunks)rootfs/etc/profile.d/_40-preferences.sh(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build-and-push (debian)
🔇 Additional comments (11)
rootfs/etc/profile.d/_40-preferences.sh (1)
40-43: Variable Scope in _term_fold FunctionThe PR summary mentioned the removal of the
localkeyword from thecolsvariable declaration, which would change its scope to global. However, the current diff still showslocal cols. Please verify whether this change in variable scoping is intentional. If the intention is to allow other functions or subsequent code to accesscols, it may be valid—but if not, retaining local scoping could help prevent unintended side effects.rootfs/etc/profile.d/_07-term-mode.sh (8)
20-25: Addition of _terminal_trace Logging FunctionThe new
_terminal_tracefunction is well implemented. It provides early-stage logging (even when color functions aren’t yet available) by leveragingtputfor ANSI color control. This enhancement will help in debugging terminal operations during startup.
28-30: Enhanced Terminal Capability CheckIntroducing the use of
tput colorsto determine the number of supported colors (with a fallback to zero) makes the terminal capability check more robust. This change ensures that the script can gracefully handle environments where terminal color support is absent.
57-65: Fallback Output in _is_term_dark_mode FunctionThe early-return condition based on
GEODESIC_TERM_COLOR_AUTObeing set to "unsupported" or "disabled" is practical. Mapping the various flags (e.g.,-b,-m) to predetermined output values ensures that the function always returns a consistent and expected result when color detection isn’t viable. Please double-check that these fallbacks integrate seamlessly with downstream logic.
77-77: Trace Logging Before OSC Query ExecutionThe trace message
"Checking terminal color scheme..."right before issuing the OSC queries is a useful debugging aid. It clearly indicates when the script begins probing terminal colors.
84-89: Timeout Handling for OSC QueriesAdjusting the timeout duration dynamically—with a longer wait when
GEODESIC_TERM_COLOR_SIGNALis true—ensures that the script can accommodate slower terminal responses (such as when waking from sleep). The use ofIFSwith a custom delimiter to capture OSC responses is an effective way to parse the terminal’s reply.
102-104: Error Reporting for OSC Query FailureThe trace message issued when no response is received from the OSC queries provides clear diagnostic feedback. Defaulting to “light” mode in this scenario is historically consistent, but ensure that this fallback aligns with overall user expectations.
118-123: Review Error Message FormattingThe error output on line 121 appears to have misformatted command substitutions (e.g. using
$(tput set bold)with a subsequent literal($tput setaf 1)). Please verify that the intended ANSI escape sequences are rendered correctly. A small tweak in the formatting might be needed to ensure the message is both clear and correctly colored.🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 121-121: tput is referenced but not assigned (for output from commands, use "$(tput ...)" ).
(SC2154)
182-186: Graceful Handling of Empty Input in _srgb_to_luminanceChecking for an empty
$colorargument and logging this with_terminal_traceis a good safeguard. This defensive programming ensures that the luminance conversion doesn’t proceed with invalid input.rootfs/etc/profile.d/_10-colors.sh (2)
63-67: Enhanced Terminal Color Support DetectionRefactoring
_is_color_termto capture the number of colors supported viatput colorsimproves its detection accuracy. Combining this with a check on file descriptor-t 0ensures a better assessment of whether the terminal can display colors.
76-79: Robust Initialization of Color Defaults in _geodesic_tput_cache_initDefining a local variable
color_offand using a compound condition that retrieves the default colors viatput op—while verifying terminal query support with_verify_terminal_queries_are_supported—adds robustness to the initialization process. This ensures that if terminal color settings can’t be reliably obtained, the cache initialization will safely abort, prompting a fallback.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
rootfs/etc/profile.d/_07-term-mode.sh (3)
69-92: Enhanced OSC query handling in_is_term_dark_mode
Within the terminal query block, the function now:
- Leverages
_verify_terminal_queries_are_supportedto ensure the terminal is capable before proceeding.- Uses a dynamic timeout based on the value of
GEODESIC_TERM_COLOR_SIGNAL(with 30 seconds for signal-wakeup scenarios and 1 second otherwise).- Reads OSC responses for both foreground and background colors and aggregates exit codes correctly.
These changes improve robustness when terminals respond slowly or not at all.Consider defaulting the value of
GEODESIC_TERM_COLOR_SIGNAL(e.g.,+GEODESIC_TERM_COLOR_SIGNAL=${GEODESIC_TERM_COLOR_SIGNAL:-false}) to avoid potential issues if it is undefined.
96-115: Fallback and signal handler messaging in_is_term_dark_mode
The block handling conditions when the terminal does not respond is now clear and caters for both standard and signal-wakeup scenarios. The use ofprintffor detailed error output is appropriate. This fallback logic ensures a sensible default (light mode) is applied while informing the user about detection issues.You may consider extracting the fallback messaging and error reporting into a helper function to reduce complexity.
178-219: Improved defensive logging and luminance conversion in_srgb_to_luminance
The update to log a warning via_terminal_tracewhen the function is called with an empty argument is a good defensive practice. The inner functionnormalize_and_linearizeis used to process each hexadecimal component correctly, and the subsequent calculation of luminance is logically sound.Be aware that the use of a nested function (i.e.
normalize_and_linearize) makes this implementation Bash-specific; if portability beyond Bash is ever needed, consider extracting this to a separate helper.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
rootfs/etc/profile.d/_07-term-mode.sh(6 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build-and-push (debian)
🔇 Additional comments (4)
rootfs/etc/profile.d/_07-term-mode.sh (4)
20-25: Introduce centralized terminal tracing via_terminal_trace
The new function is a well-isolated way to log terminal-related debug messages, ensuring that color codes are only used when available. This helps reduce code duplication and improves maintainability.
27-47: Robust terminal capability check in_verify_terminal_queries_are_supported
The modifications correctly check for terminal color support by usingtput colorsand testing for a TTY. Note the intentional use of directecho(instead of_terminal_trace) in contexts where color output might not be supported. The logic to exportGEODESIC_TERM_COLOR_AUTO=unsupportedand later unset it (unless already "disabled") is clear and effective.
57-65: Early exit for unsupported or disabled color auto-detection in_is_term_dark_mode
The initial guard clause now correctly detects whenGEODESIC_TERM_COLOR_AUTOis set to "unsupported" or "disabled" and immediately returns appropriate default outputs based on the flags. This helps avoid unnecessary terminal queries when color support is already ruled out.
118-123: Error reporting for unexpected color format
The check for an unanticipated color format using parameter expansion is effective. The error message is formatted withtputto highlight the issue. This will help capture non-standard responses from terminals for future enhancements.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
demo.tape (1)
33-34: Consider making the installation instructions more robust.While the instructions are clearer now, consider these improvements:
- Add a note about potential terminal color detection issues
- Consider using a specific version tag instead of 'latest' for reproducibility
-Type "# After having run `DOCKER_BASE_TAG=latest make install`," Enter -Type "# simply run `make run` (or `geodesic`) to enter the interactive shell" Enter +Type "# First, install Geodesic (use specific version tag for reproducibility):" Enter +Type "# DOCKER_BASE_TAG=v1.x.x make install" Enter +Type "# Note: If you experience terminal color issues, try running with TERM=xterm-256color" Enter +Type "# Then, enter the interactive shell:" Enter +Type "# make run (or geodesic)" Enter
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
demo.tape(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: vhs
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.github/workflows/vhs.yaml (1)
49-50: Updated Preferences Directory Structure:
The workflow now creates a new directory at$HOME/.geodesic/defaultsand writes the preferences to$HOME/.geodesic/defaults/preferences. This reorganization helps reduce code duplication and improve configuration management. Please verify that any downstream scripts or processes relying on the old path ($HOME/.geodesic/preferences) are updated to use the new location.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/vhs.yaml(1 hunks)demo.tape(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- demo.tape
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: vhs
what
why