Fix greenscreen theming in about/auth/scrollbars #1067
Merged
Conversation
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
github-actions
bot
added
the
maintainer:e2e:ok
Contributor
|
Caution Review failedThe pull request is closed. WalkthroughThe PR applies Colors theme tokens to CLI UI components (AboutBox, AuthDialog, ScrollableList, VirtualizedList) for backgroundColor and scrollbarThumbColor props, replacing hardcoded colors. Each component receives a corresponding unit test verifying theme prop values match the Colors module. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (8)
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 analyze the changes in this pull request to evaluate how well it addresses the issues and the overall quality of the implementation. LLxprt PR Review – PR #1067Issue Alignment |
![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
🤖 Fix all issues with AI agents
In @issues-697-908-910-investigation-report.md:
- Line 144: The markdown code span contains a leading space ("`
SemanticColors.border.default`") which violates MD038; edit the text to remove
the leading space so it reads "`SemanticColors.border.default`" (or replace the
whole span with "`Colors.Gray`" if that was intended), ensuring the inline code
spans have no leading/trailing spaces.
🧹 Nitpick comments (1)
packages/cli/src/ui/components/AuthDialog.theme.test.tsx (1)
38-54: Consider verifying the specific container Box, not just any Box.The test finds any Box with
backgroundColor === Colors.Background, but doesn't verify it's the main container. If AuthDialog renders multiple nested Boxes, this could pass even if the wrong element is themed.💡 Possible enhancement to verify the root Box specifically
You could verify the first Box with backgroundColor is the themed one, or add a test ID prop to distinguish the container:
it('sets the background color from the active theme', () => { const settings = new LoadedSettings( { settings: { ui: { customThemes: {} }, mcpServers: {} }, path: '' }, { settings: {}, path: '' }, { settings: {}, path: '' }, { settings: { ui: { customThemes: {} }, mcpServers: {} }, path: '' }, true, ); renderWithProviders(<AuthDialog onSelect={vi.fn()} settings={settings} />); - const themedBox = recordedBoxProps.find( - (entry) => entry.backgroundColor === Colors.Background, - ); + // Verify the first Box with backgroundColor is themed (likely the container) + const themedBox = recordedBoxProps.find( + (entry) => entry.backgroundColor !== undefined, + ); - expect(themedBox).toBeDefined(); + expect(themedBox?.backgroundColor).toBe(Colors.Background); });This verifies the first Box with any backgroundColor uses the theme color.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
issues-697-908-910-investigation-report.mdpackages/cli/src/ui/components/AboutBox.theme.test.tsxpackages/cli/src/ui/components/AboutBox.tsxpackages/cli/src/ui/components/AuthDialog.theme.test.tsxpackages/cli/src/ui/components/AuthDialog.tsxpackages/cli/src/ui/components/shared/ScrollableList.theme.test.tsxpackages/cli/src/ui/components/shared/ScrollableList.tsxpackages/cli/src/ui/components/shared/VirtualizedList.theme.test.tsxpackages/cli/src/ui/components/shared/VirtualizedList.tsx
🧰 Additional context used
🧬 Code graph analysis (8)
packages/cli/src/ui/components/shared/VirtualizedList.theme.test.tsx (1)
packages/cli/src/ui/colors.ts (1)
Colors(12-78)
packages/cli/src/ui/components/shared/ScrollableList.theme.test.tsx (2)
packages/cli/src/ui/contexts/ScrollProvider.tsx (1)
ScrollProvider(80-356)packages/cli/src/ui/colors.ts (1)
Colors(12-78)
packages/cli/src/ui/components/AboutBox.tsx (1)
packages/cli/src/ui/colors.ts (1)
Colors(12-78)
packages/cli/src/ui/components/shared/VirtualizedList.tsx (1)
packages/cli/src/ui/colors.ts (1)
Colors(12-78)
packages/cli/src/ui/components/AuthDialog.theme.test.tsx (4)
packages/cli/src/config/settings.ts (1)
LoadedSettings(339-467)packages/cli/src/test-utils/render.tsx (1)
renderWithProviders(127-149)packages/cli/src/ui/components/AuthDialog.tsx (1)
AuthDialog(26-194)packages/cli/src/ui/colors.ts (1)
Colors(12-78)
packages/cli/src/ui/components/AboutBox.theme.test.tsx (2)
packages/cli/src/ui/components/AboutBox.tsx (1)
AboutBox(26-162)packages/cli/src/ui/colors.ts (1)
Colors(12-78)
packages/cli/src/ui/components/shared/ScrollableList.tsx (1)
packages/cli/src/ui/colors.ts (1)
Colors(12-78)
packages/cli/src/ui/components/AuthDialog.tsx (1)
packages/cli/src/ui/colors.ts (1)
Colors(12-78)
🪛 markdownlint-cli2 (0.18.1)
issues-697-908-910-investigation-report.md
144-144: Spaces inside code span elements
(MD038, no-space-in-code)
⏰ 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). (6)
- GitHub Check: Test (ubuntu-latest, 24.x)
- GitHub Check: Test (macos-latest, 24.x)
- GitHub Check: Test (windows-latest, 24.x)
- GitHub Check: E2E Test (Linux) - sandbox:none
- GitHub Check: E2E Test (Linux) - sandbox:docker
- GitHub Check: Slow E2E - Win
🔇 Additional comments (11)
packages/cli/src/ui/components/AboutBox.tsx (1)
44-44: LGTM! Background theming applied correctly.The addition of
backgroundColor={Colors.Background}ensures the AboutBox respects the active theme, addressing issue #697 where the dialog previously displayed a white background instead of following the greenscreen theme.packages/cli/src/ui/components/AuthDialog.tsx (1)
149-149: LGTM! Auth dialog theming applied correctly.The
backgroundColor={Colors.Background}addition ensures the authentication dialog respects the greenscreen theme, fixing issue #908 where white and dark grey elements were displayed instead of theme-appropriate colors.packages/cli/src/ui/components/shared/VirtualizedList.tsx (1)
20-20: LGTM! Scrollbar theming correctly implemented.Replacing the hardcoded
'gray'string withColors.Graymakes the scrollbar thumb color theme-aware, addressing issue #910. The Colors.Gray getter dynamically retrieves the appropriate color from the active theme, ensuring consistency with the greenscreen aesthetic.Also applies to: 473-473
packages/cli/src/ui/components/shared/ScrollableList.tsx (1)
22-22: LGTM! Focus-aware scrollbar theming is well-implemented.The dynamic scrollbar color based on focus state (
Colors.Foregroundwhen focused,Colors.Graywhen not) provides excellent visual feedback and aligns with issue #910. This pattern helps users identify which list is currently active while maintaining theme consistency.Also applies to: 136-136
packages/cli/src/ui/components/AboutBox.theme.test.tsx (1)
1-60: LGTM! Theme test is well-structured.The test effectively validates that
AboutBoxappliesColors.Backgroundto its root container. The Box interception approach usingInstrumentedBoxis appropriate for verifying prop usage, and the dynamic import pattern aftervi.mockis the correct approach for ESM module mocking in Vitest.packages/cli/src/ui/components/AuthDialog.theme.test.tsx (1)
13-31: LGTM! Mock setup follows best practices.The instrumentation pattern correctly:
- Records Box props before the component renders
- Uses
importOriginalto preserve other ink exports- Performs dynamic imports after mocks are established
packages/cli/src/ui/components/shared/ScrollableList.theme.test.tsx (2)
17-42: LGTM! Comprehensive mock setup.The test correctly:
- Mocks VirtualizedList to capture the scrollbarThumbColor prop
- Includes the SCROLL_TO_ITEM_END export for module compatibility
- Mocks measureElement to avoid DOM measurement dependencies
47-74: LGTM! Test correctly validates focus-dependent theming.The test properly:
- Renders ScrollableList in both focus states
- Captures the scrollbarThumbColor passed to VirtualizedList
- Asserts the correct theme colors are used based on focus state
packages/cli/src/ui/components/shared/VirtualizedList.theme.test.tsx (3)
17-30: LGTM! Focused recording approach.The conditional recording (lines 20-24) filters out irrelevant Box instances, capturing only those with
scrollbarThumbColor. This makes the test more precise and reduces noise from other Box components.
32-46: LGTM! Mock dependencies properly stubbed.The mocks provide:
- InstrumentedBox to capture props
- Fixed dimensions from measureElement
- Minimal useBatchedScroll interface to satisfy VirtualizedList dependencies
51-72: LGTM! Test validates default theme color.The test correctly:
- Renders VirtualizedList without providing scrollbarThumbColor
- Verifies the default value is
Colors.Grayfrom the theme- Uses beforeEach to ensure clean state between tests
|
|
||
| 1. Replace `const scrollbarColor = hasFocus ? 'gray' : 'darkgray';` with theme-aware colors | ||
| 2. Use `Colors.Foreground` for focused state | ||
| 3. Use `Colors.Gray` or ` SemanticColors.border.default` for unfocused state |
Contributor
There was a problem hiding this comment.
Fix markdown formatting: remove leading space in code span.
Line 144 has a leading space inside the code span, triggering markdownlint rule MD038.
📝 Proposed fix
-3. Use `Colors.Gray` or ` SemanticColors.border.default` for unfocused state
+3. Use `Colors.Gray` or `SemanticColors.border.default` for unfocused state📝 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
| 3. Use `Colors.Gray` or ` SemanticColors.border.default` for unfocused state | |
| 3. Use `Colors.Gray` or `SemanticColors.border.default` for unfocused state |
🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
144-144: Spaces inside code span elements
(MD038, no-space-in-code)
🤖 Prompt for AI Agents
In @issues-697-908-910-investigation-report.md at line 144, The markdown code
span contains a leading space ("` SemanticColors.border.default`") which
violates MD038; edit the text to remove the leading space so it reads
"`SemanticColors.border.default`" (or replace the whole span with
"`Colors.Gray`" if that was intended), ensuring the inline code spans have no
leading/trailing spaces.
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. |
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
Testing
Fixes #697
Fixes #908
Fixes #910
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.