Conversation
Add 'solarized-dark' to ThemeId union type, isValidThemeId guard, and THEMES registry using canonical Solarized Dark palette colors. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughAdds a new "solarized-dark" theme and registers it in the theme registry; updates ThemeId validation and a unit test to expect 18 themes. Adjusts pills' styling in SessionListItem and changes code-block rendering to compute theme-aware syntax styles. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip You can validate your CodeRabbit configuration file in your editor.If your editor has YAML language server, you can enable auto-completion and validation by adding |
Greptile SummaryThis PR adds the Solarized Dark theme, correctly wiring it through all required layers: the Two supporting changes accompany the new theme:
Key observations:
Confidence Score: 5/5
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["ThemeId union type\nsrc/shared/theme-types.ts"] -->|"'solarized-dark' added"| B["isValidThemeId guard\nsrc/shared/theme-types.ts"]
A --> C["THEMES record\nsrc/shared/themes.ts"]
C -->|"re-exported via"| D["src/renderer/constants/themes.ts"]
D -->|"consumed by"| E["SessionListItem.tsx\npill colors use accentText / textMain"]
D -->|"consumed by"| F["markdownConfig.ts\nthemedStyle overrides vscDarkPlus\nbase color + background"]
C --> G["themes.test.ts\ncount: 17 → 18"]
Last reviewed commit: 7dcc59e |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/renderer/components/SessionListItem.tsx (1)
281-289: Consider updating the search match pill for consistency.The content search match pill (line 284) still uses
theme.colors.accentfor both background and text color, while similar pills in this file were updated to use more readable text colors. This may have contrast issues on certain themes.Note that similar patterns exist in
Toast.tsx(lines 194-200) andShortcutsHelpModal.tsx(lines 84-87) per the relevant code snippets provided.♻️ Optional: Update for consistency
{searchResultInfo && searchResultInfo.matchCount > 0 && searchMode !== 'title' && ( <span className="flex items-center gap-1 px-1.5 py-0.5 rounded" - style={{ backgroundColor: theme.colors.accent + '20', color: theme.colors.accent }} + style={{ backgroundColor: theme.colors.accent + '30', color: theme.colors.accentText }} > <Search className="w-3 h-3" /> {searchResultInfo.matchCount} </span> )}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/SessionListItem.tsx` around lines 281 - 289, The search match pill in SessionListItem (rendering when searchResultInfo && searchResultInfo.matchCount > 0 && searchMode !== 'title') uses theme.colors.accent for both background and text causing contrast issues; update the inline style so the background remains theme.colors.accent + '20' but the text color uses a readable color (e.g., theme.colors.textPrimary or a dedicated on-accent color) instead of theme.colors.accent, and apply the same consistent change pattern to the similar pills in Toast.tsx and ShortcutsHelpModal.tsx to ensure consistent contrast across components.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/renderer/components/SessionListItem.tsx`:
- Around line 281-289: The search match pill in SessionListItem (rendering when
searchResultInfo && searchResultInfo.matchCount > 0 && searchMode !== 'title')
uses theme.colors.accent for both background and text causing contrast issues;
update the inline style so the background remains theme.colors.accent + '20' but
the text color uses a readable color (e.g., theme.colors.textPrimary or a
dedicated on-accent color) instead of theme.colors.accent, and apply the same
consistent change pattern to the similar pills in Toast.tsx and
ShortcutsHelpModal.tsx to ensure consistent contrast across components.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4148c7f4-cc2d-4273-8d34-6de7aea1eb27
📒 Files selected for processing (5)
src/__tests__/renderer/constants/themes.test.tssrc/renderer/components/SessionListItem.tsxsrc/renderer/utils/markdownConfig.tssrc/shared/theme-types.tssrc/shared/themes.ts
Same pattern — accent text on faint accent background. This is fine. The accent color ( No changes needed. These pills use a well-established pattern (accent-colored text with a faint accent wash) that provides adequate contrast across all themes, including Solarized Dark. The text color contrasts against the underlying dark background, not against the semi-transparent tint. |
|
Hey @chrisscott — thanks for this PR! The Solarized Dark theme and contrast improvements look great. There's a merge conflict in If you get a chance to rebase and fix that up, that'd be ideal. But if you're busy, we're happy to take care of it on our end — we'll make sure you get full credit for the work either way. Thanks again for the contribution! 🙏 |
- Darken border color (#586e75 → #2f4f56) so pill backgrounds contrast better with text - Brighten textMain (#839496 → #93a1a1) and accentText (#2aa198 → #5fddd5) for readable tag labels - Override SyntaxHighlighter base text color with theme's textMain so code blocks match inline code across all themes - Update pill styles in SessionListItem for better contrast - Bump theme count assertion from 17 → 18 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
chrisscott
force-pushed
the
fix/solarized-dark-contrast
branch
from
7dcc59e to
5514865
Compare
|
Thanks for the review @pedramamini! I've pushed up the requested changes. |
2 participants
Adds the Solarized Dark theme.
Test with dev server.
Summary by CodeRabbit
New Features
Style
Tests