Conversation
- Add ColorsTheme.DimComment and define it across built-in themes - Use explicit DimComment color for ThinkingBlock instead of Ink dimColor (avoids tmux/Linux background highlight) - Greenscreen: ensure black background and darker DimComment than #6a9955 - Add luminance-based test to assert DimComment is darker than Comment Refs: #829 Base: 0453210
github-actions
Bot
added
the
maintainer:e2e:ok
Contributor
WalkthroughIntroduces a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
Contributor
|
I'll review this PR which focuses on fixing the greenscreen thinking text styling issue on tmux. Let me analyze the changes systematically. LLxprt PR Review – PR #899Issue Alignment
Side Effects
Code Quality
Tests & Coverage
Verdict The fix appears to address the core issue but requires consideration of:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
packages/cli/src/ui/colors.ts(1 hunks)packages/cli/src/ui/components/messages/ThinkingBlockDisplay.test.tsx(2 hunks)packages/cli/src/ui/components/messages/ThinkingBlockDisplay.tsx(1 hunks)packages/cli/src/ui/themes/ansi-light.ts(1 hunks)packages/cli/src/ui/themes/ansi.ts(1 hunks)packages/cli/src/ui/themes/atom-one-dark.ts(1 hunks)packages/cli/src/ui/themes/ayu-light.ts(1 hunks)packages/cli/src/ui/themes/ayu.ts(1 hunks)packages/cli/src/ui/themes/dracula.ts(1 hunks)packages/cli/src/ui/themes/github-dark.ts(1 hunks)packages/cli/src/ui/themes/github-light.ts(1 hunks)packages/cli/src/ui/themes/googlecode.ts(1 hunks)packages/cli/src/ui/themes/green-screen.ts(2 hunks)packages/cli/src/ui/themes/no-color.ts(1 hunks)packages/cli/src/ui/themes/semantic-resolver.test.ts(6 hunks)packages/cli/src/ui/themes/semantic-tokens.test.ts(3 hunks)packages/cli/src/ui/themes/shades-of-purple.ts(1 hunks)packages/cli/src/ui/themes/theme-manager.test.ts(1 hunks)packages/cli/src/ui/themes/theme.test.ts(1 hunks)packages/cli/src/ui/themes/theme.ts(4 hunks)packages/cli/src/ui/themes/xcode.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Don't useany- Always specify proper types. Useunknownif the type is truly unknown and add proper type guards.
Do not useconsole.logorconsole.debug- Use the sophisticated logging system instead. Log files are written to ~/.llxprt/debug/
Fix all linting errors, including warnings aboutanytypes
Files:
packages/cli/src/ui/components/messages/ThinkingBlockDisplay.tsxpackages/cli/src/ui/themes/dracula.tspackages/cli/src/ui/components/messages/ThinkingBlockDisplay.test.tsxpackages/cli/src/ui/themes/shades-of-purple.tspackages/cli/src/ui/themes/atom-one-dark.tspackages/cli/src/ui/themes/theme.tspackages/cli/src/ui/themes/github-light.tspackages/cli/src/ui/themes/googlecode.tspackages/cli/src/ui/themes/ansi.tspackages/cli/src/ui/themes/ansi-light.tspackages/cli/src/ui/themes/semantic-tokens.test.tspackages/cli/src/ui/themes/xcode.tspackages/cli/src/ui/themes/theme-manager.test.tspackages/cli/src/ui/themes/no-color.tspackages/cli/src/ui/themes/github-dark.tspackages/cli/src/ui/themes/theme.test.tspackages/cli/src/ui/themes/ayu.tspackages/cli/src/ui/colors.tspackages/cli/src/ui/themes/ayu-light.tspackages/cli/src/ui/themes/semantic-resolver.test.tspackages/cli/src/ui/themes/green-screen.ts
🧬 Code graph analysis (2)
packages/cli/src/ui/components/messages/ThinkingBlockDisplay.tsx (2)
packages/cli/test-utils/ink-stub.ts (1)
Text(23-23)packages/cli/src/ui/colors.ts (1)
Colors(12-78)
packages/cli/src/ui/components/messages/ThinkingBlockDisplay.test.tsx (1)
packages/cli/src/ui/colors.ts (1)
Colors(12-78)
⏰ Context from checks skipped due to timeout of 270000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Test (ubuntu-latest, 24.x)
- GitHub Check: Test (macos-latest, 24.x)
- GitHub Check: Test (windows-latest, 24.x)
- GitHub Check: Slow E2E - Win
- GitHub Check: E2E Test (Linux) - sandbox:none
- GitHub Check: E2E Test (Linux) - sandbox:docker
- GitHub Check: E2E Test (macOS)
🔇 Additional comments (24)
packages/cli/src/ui/themes/github-light.ts (1)
23-23: LGTM!The
DimCommentcolor value is correctly added and is darker than the existingCommentcolor, aligning with the PR objective to provide a properly dimmed comment color for the thinking text display.packages/cli/src/ui/themes/no-color.ts (1)
23-23: LGTM!The empty
DimCommentvalue is consistent with the no-color theme's approach of using empty strings for all color properties.packages/cli/src/ui/themes/atom-one-dark.ts (1)
27-27: LGTM!The
DimCommentcolor is correctly added and is darker than theCommentcolor, providing the appropriate visual distinction for dimmed text in this dark theme.packages/cli/src/ui/themes/googlecode.ts (1)
23-23: LGTM!The
DimCommentcolor value is properly added and maintains the correct luminance relationship (darker thanComment), consistent with the PR's fix for thinking text display.packages/cli/src/ui/themes/green-screen.ts (2)
11-11: LGTM! Core fix for issue #829.Changing the background to true black (
#000000) directly addresses the tmux rendering issue where ANSI dim was being interpreted as a background attribute, causing the thinking text to appear permanently highlighted.
27-27: LGTM! Completes the fix for issue #829.The
DimCommentcolor (#4a7035) is appropriately darker than the normalCommentcolor (#6a9955), providing proper visual distinction for thinking text without relying on ANSI dimColor, which renders incorrectly on Linux/tmux.packages/cli/src/ui/themes/xcode.ts (1)
23-23: LGTM!The
DimCommentcolor is correctly defined and darker than theCommentcolor, maintaining the green color scheme while providing proper dimming for the Xcode theme.packages/cli/src/ui/themes/ansi-light.ts (1)
27-27: LGTM!The
DimCommenthex color value (#666666) is darker than the namedCommentcolor (gray, typically#808080), providing appropriate dimming for the ANSI light theme.packages/cli/src/ui/themes/shades-of-purple.ts (1)
28-28: LGTM!The
DimCommentcolor is correctly added with lower RGB values thanComment, providing proper dimming while maintaining the purple color scheme of this theme.packages/cli/src/ui/themes/github-dark.ts (1)
23-23: LGTM! DimComment color added appropriately.The DimComment color '#4f555d' is darker than the Comment color '#6A737D', which aligns with the intended use case for dimmed thinking text display.
packages/cli/src/ui/themes/theme-manager.test.ts (1)
45-45: LGTM! Test fixture updated correctly.The DimComment property is properly added to the test fixture to match the extended ColorsTheme interface.
packages/cli/src/ui/components/messages/ThinkingBlockDisplay.tsx (1)
59-59: LGTM! Core fix correctly implemented.Replacing
dimColorprop withColors.DimCommentdirectly addresses the environment-specific rendering issue where ANSI faint attributes behave inconsistently across terminals. This ensures consistent visual styling across macOS and Linux/tmux environments.packages/cli/src/ui/themes/ayu-light.ts (1)
23-23: LGTM! DimComment added correctly.The DimComment color is appropriately darker than the Comment color for this light theme.
packages/cli/src/ui/themes/semantic-tokens.test.ts (1)
109-109: LGTM! Test fixtures updated consistently.All custom theme test fixtures have been updated with appropriate DimComment values, ensuring test coverage for the new color token.
Also applies to: 142-142, 348-348
packages/cli/src/ui/themes/theme.test.ts (1)
30-30: LGTM! Test fixture properly extended.The validTheme test fixture correctly includes the DimComment property with an appropriate color value.
packages/cli/src/ui/themes/ayu.ts (1)
23-23: LGTM! DimComment color appropriately chosen.The DimComment color is darker than Comment, maintaining consistency with the theme's color hierarchy.
packages/cli/src/ui/colors.ts (1)
69-71: LGTM! Public API correctly extended.The DimComment getter follows the established pattern for exposing theme colors and enables ThinkingBlockDisplay to use Colors.DimComment. All color theme definitions (darkTheme, lightTheme, and all theme variants) have DimComment properly defined, and the implementation is type-safe with no linting violations.
packages/cli/src/ui/themes/ansi.ts (1)
27-27: LGTM! Appropriate dim color for ANSI theme.The hex color
#5a5a5aprovides a portable dim gray that avoids the ANSI faint attribute issues described in issue #829, while remaining visually appropriate for the ANSI dark theme.packages/cli/src/ui/themes/dracula.ts (1)
27-27: LGTM! Darker variant consistent with Dracula palette.The DimComment color
#4a5478is appropriately darker than Comment#6272a4and maintains the bluish tint characteristic of the Dracula theme.packages/cli/src/ui/themes/semantic-resolver.test.ts (1)
29-29: LGTM! Test fixtures properly updated.All theme test fixtures now include appropriate DimComment values consistent with their respective theme types and color palettes.
Also applies to: 81-81, 133-133, 185-185, 237-237, 262-262
packages/cli/src/ui/components/messages/ThinkingBlockDisplay.test.tsx (2)
18-19: LGTM! Import added for color testing.The Colors import is appropriately added to support the new DimComment color verification tests.
185-194: LGTM! Basic color existence checks are appropriate.The test correctly verifies that DimComment is defined, truthy, and a non-empty string.
packages/cli/src/ui/themes/theme.ts (2)
50-50: LGTM! Interface properly extended with required DimComment.The DimComment property is correctly added to the ColorsTheme interface as a required field.
However, note that Line 402 in
validateCustomThemedoesn't includeDimCommentin therequiredFieldsarray. SinceDimCommentis now a required property ofColorsThemeand theColors.DimCommentgetter (in packages/cli/src/ui/colors.ts) has no fallback, custom themes lacking this field will causeundefinedto be used in the UI.Consider adding
DimCommentto eitherrequiredFieldsorrecommendedFieldsinvalidateCustomTheme, or add a fallback in theColors.DimCommentgetter to handle backward compatibility with existing custom themes.
74-74: LGTM! Theme implementations correctly include DimComment.All three default theme implementations (lightTheme, darkTheme, ansiTheme) now properly define DimComment with appropriate values:
- lightTheme:
#006000(darker green)- darkTheme:
#4A4D5E(darker blue-gray)- ansiTheme:
#5a5a5a(darker gray, using hex to avoid ANSI escape issues)Also applies to: 93-93, 112-112
Comment on lines
+203
to
+238
| it('should have DimComment darker than Comment', () => { | ||
| // Helper to calculate relative luminance from hex color | ||
| const getRelativeLuminance = (hex: string): number => { | ||
| // Remove # if present | ||
| const cleanHex = hex.replace(/^#/, ''); | ||
|
|
||
| // Only handle 6-character hex colors | ||
| if (!/^[0-9A-Fa-f]{6}$/.test(cleanHex)) { | ||
| throw new Error(`Invalid hex color: ${hex}`); | ||
| } | ||
|
|
||
| // Parse RGB components | ||
| const r = parseInt(cleanHex.slice(0, 2), 16) / 255; | ||
| const g = parseInt(cleanHex.slice(2, 4), 16) / 255; | ||
| const b = parseInt(cleanHex.slice(4, 6), 16) / 255; | ||
|
|
||
| // Apply gamma correction | ||
| const gamma = (c: number) => | ||
| c <= 0.03928 ? c / 12.92 : Math.pow((c + 0.055) / 1.055, 2.4); | ||
|
|
||
| // Calculate relative luminance (WCAG formula) | ||
| return 0.2126 * gamma(r) + 0.7152 * gamma(g) + 0.0722 * gamma(b); | ||
| }; | ||
|
|
||
| // Basic check that both colors exist and are different | ||
| expect(Colors.DimComment).not.toBe(Colors.Comment); | ||
|
|
||
| // Ensure both colors are valid hex format | ||
| expect(Colors.Comment).toMatch(/^#[0-9A-Fa-f]{6}$/); | ||
| expect(Colors.DimComment).toMatch(/^#[0-9A-Fa-f]{6}$/); | ||
|
|
||
| // Calculate and compare luminance - DimComment should be darker | ||
| const commentLuminance = getRelativeLuminance(Colors.Comment); | ||
| const dimCommentLuminance = getRelativeLuminance(Colors.DimComment); | ||
| expect(dimCommentLuminance).toBeLessThan(commentLuminance); | ||
| }); |
Contributor
There was a problem hiding this comment.
Test assumes hex colors but ANSI theme uses named colors.
Lines 231-232 expect both Colors.Comment and Colors.DimComment to match the hex pattern /^#[0-9A-Fa-f]{6}$/. However, the ANSI theme defines Comment: 'gray' (a named color), causing this test to fail when the ANSI theme is active. Additionally, the comment on Line 199 mentions "greenscreen theme active," but the test doesn't actually set the active theme.
The getRelativeLuminance function implementation (lines 205-225) is correct and follows the WCAG standard.
🔎 Suggested fix options
Option 1: Make test theme-specific (recommended for greenscreen verification)
Set the greenscreen theme before running the luminance comparison:
it('should have DimComment darker than Comment', () => {
+ // Ensure we're testing the Green Screen theme specifically
+ themeManager.setTheme('Green Screen');
+
// Helper to calculate relative luminance from hex color
const getRelativeLuminance = (hex: string): number => {Option 2: Make test theme-agnostic
Handle both hex and named colors in the luminance test:
it('should have DimComment darker than Comment', () => {
// Helper to calculate relative luminance from hex color
const getRelativeLuminance = (hex: string): number => {
// Remove # if present
const cleanHex = hex.replace(/^#/, '');
- // Only handle 6-character hex colors
- if (!/^[0-9A-Fa-f]{6}$/.test(cleanHex)) {
- throw new Error(`Invalid hex color: ${hex}`);
- }
+ // Skip non-hex colors (ANSI named colors)
+ if (!/^[0-9A-Fa-f]{6}$/.test(cleanHex)) {
+ return -1; // Sentinel value for non-hex
+ }
// ... rest of function
};
// Basic check that both colors exist and are different
expect(Colors.DimComment).not.toBe(Colors.Comment);
- // Ensure both colors are valid hex format
- expect(Colors.Comment).toMatch(/^#[0-9A-Fa-f]{6}$/);
- expect(Colors.DimComment).toMatch(/^#[0-9A-Fa-f]{6}$/);
+ // Only compare luminance for hex colors
+ const commentIsHex = /^#[0-9A-Fa-f]{6}$/.test(Colors.Comment);
+ const dimCommentIsHex = /^#[0-9A-Fa-f]{6}$/.test(Colors.DimComment);
+
+ if (commentIsHex && dimCommentIsHex) {
- // Calculate and compare luminance - DimComment should be darker
- const commentLuminance = getRelativeLuminance(Colors.Comment);
- const dimCommentLuminance = getRelativeLuminance(Colors.DimComment);
- expect(dimCommentLuminance).toBeLessThan(commentLuminance);
+ const commentLuminance = getRelativeLuminance(Colors.Comment);
+ const dimCommentLuminance = getRelativeLuminance(Colors.DimComment);
+ expect(dimCommentLuminance).toBeLessThan(commentLuminance);
+ }
});Recommendation: Use Option 1 if this test is specifically for the Green Screen theme (as the comment suggests). Otherwise, use Option 2 to make the test work with all themes.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| it('should have DimComment darker than Comment', () => { | |
| // Helper to calculate relative luminance from hex color | |
| const getRelativeLuminance = (hex: string): number => { | |
| // Remove # if present | |
| const cleanHex = hex.replace(/^#/, ''); | |
| // Only handle 6-character hex colors | |
| if (!/^[0-9A-Fa-f]{6}$/.test(cleanHex)) { | |
| throw new Error(`Invalid hex color: ${hex}`); | |
| } | |
| // Parse RGB components | |
| const r = parseInt(cleanHex.slice(0, 2), 16) / 255; | |
| const g = parseInt(cleanHex.slice(2, 4), 16) / 255; | |
| const b = parseInt(cleanHex.slice(4, 6), 16) / 255; | |
| // Apply gamma correction | |
| const gamma = (c: number) => | |
| c <= 0.03928 ? c / 12.92 : Math.pow((c + 0.055) / 1.055, 2.4); | |
| // Calculate relative luminance (WCAG formula) | |
| return 0.2126 * gamma(r) + 0.7152 * gamma(g) + 0.0722 * gamma(b); | |
| }; | |
| // Basic check that both colors exist and are different | |
| expect(Colors.DimComment).not.toBe(Colors.Comment); | |
| // Ensure both colors are valid hex format | |
| expect(Colors.Comment).toMatch(/^#[0-9A-Fa-f]{6}$/); | |
| expect(Colors.DimComment).toMatch(/^#[0-9A-Fa-f]{6}$/); | |
| // Calculate and compare luminance - DimComment should be darker | |
| const commentLuminance = getRelativeLuminance(Colors.Comment); | |
| const dimCommentLuminance = getRelativeLuminance(Colors.DimComment); | |
| expect(dimCommentLuminance).toBeLessThan(commentLuminance); | |
| }); | |
| it('should have DimComment darker than Comment', () => { | |
| // Helper to calculate relative luminance from hex color | |
| const getRelativeLuminance = (hex: string): number => { | |
| // Remove # if present | |
| const cleanHex = hex.replace(/^#/, ''); | |
| // Skip non-hex colors (ANSI named colors) | |
| if (!/^[0-9A-Fa-f]{6}$/.test(cleanHex)) { | |
| return -1; // Sentinel value for non-hex | |
| } | |
| // Parse RGB components | |
| const r = parseInt(cleanHex.slice(0, 2), 16) / 255; | |
| const g = parseInt(cleanHex.slice(2, 4), 16) / 255; | |
| const b = parseInt(cleanHex.slice(4, 6), 16) / 255; | |
| // Apply gamma correction | |
| const gamma = (c: number) => | |
| c <= 0.03928 ? c / 12.92 : Math.pow((c + 0.055) / 1.055, 2.4); | |
| // Calculate relative luminance (WCAG formula) | |
| return 0.2126 * gamma(r) + 0.7152 * gamma(g) + 0.0722 * gamma(b); | |
| }; | |
| // Basic check that both colors exist and are different | |
| expect(Colors.DimComment).not.toBe(Colors.Comment); | |
| // Only compare luminance for hex colors | |
| const commentIsHex = /^#[0-9A-Fa-f]{6}$/.test(Colors.Comment); | |
| const dimCommentIsHex = /^#[0-9A-Fa-f]{6}$/.test(Colors.DimComment); | |
| if (commentIsHex && dimCommentIsHex) { | |
| // Calculate and compare luminance - DimComment should be darker | |
| const commentLuminance = getRelativeLuminance(Colors.Comment); | |
| const dimCommentLuminance = getRelativeLuminance(Colors.DimComment); | |
| expect(dimCommentLuminance).toBeLessThan(commentLuminance); | |
| } | |
| }); |
🤖 Prompt for AI Agents
In packages/cli/src/ui/components/messages/ThinkingBlockDisplay.test.tsx around
lines 203 to 238, the test assumes both Colors.Comment and Colors.DimComment are
6-character hex strings and fails when the ANSI theme uses named colors like
"gray"; also the test comment mentions the greenscreen theme but never activates
it. Fix by either (A) explicitly set the Greenscreen theme at the start of the
test (or mock active theme) so Colors.* are hex before running the luminance
assertions, or (B) make the assertions theme-agnostic by accepting named colors:
resolve named colors to hex (or skip the hex-regex checks and only compare
luminance after converting named colors to RGB) and ensure getRelativeLuminance
can accept both hex and common color names; choose A if this test is intended to
verify the greenscreen theme, otherwise implement B.
Contributor
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-24.x-ubuntu-latest' artifact from the main CI run. |
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
1 participant
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.
Fixes #829
Problem
Solution
Tests / Verification
Commit
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.