Add theme variants and stabilize markdown rendering - #1212
Conversation
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughAdds two built-in dark themes, expands theme validation, caches CodeFence and Mermaid output, improves mention-overlay selection and scroll synchronization, and clamps textarea sizing to computed CSS limits. ChangesTheme expansion
Renderer caching
Input overlay and sizing
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ 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 |
Greptile SummaryThis PR adds two built-in dark themes. The main changes are:
Confidence Score: 5/5This looks safe to merge after a small visual cleanup.
src/shared/themes.ts Important Files Changed
Reviews (1): Last reviewed commit: "feat(themes): add Deep Wine Red theme" | Re-trigger Greptile |
| border: '#06070a', | ||
| textMain: '#f2ebc0', | ||
| textDim: '#c7c2b3', | ||
| accent: '#1f2f63', |
There was a problem hiding this comment.
Indigo Accent Nearly Disappears
When the Indigo Blue theme is selected, accent is used for active theme UI like scrollbar highlights, but #1f2f63 is very close to the near-black bgMain value #010204. Highlighted controls can become hard to see in this theme, especially where accent is the only active-state indicator.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
|
@felipeggv Thanks for the contribution! Four new ultra dark variants is a nice addition, and the mechanics here are clean: you updated the One substantive issue before this can go in, and it's an extension of the point Greptile raised on
|
| Theme | accent | bgMain | accent vs bgMain |
|---|---|---|---|
olive-nights (existing precedent) |
#5b675b |
#0a0b0a |
3.32:1 |
indigo-blue |
#1f2f63 |
#010204 |
1.63:1 |
deep-wine-red |
#631f30 |
#040102 |
1.75:1 |
deep-purple |
#4f2673 |
#030106 |
1.84:1 |
yellow-dark-mustard |
#6b5314 |
#040301 |
2.82:1 |
WCAG 1.4.11 asks for 3:1 for non-text UI indicators. olive-nights, the closest existing theme in spirit, clears it at 3.32:1. All four new themes fall short.
This matters more than a typical palette nit because of where accent lands. src/renderer/hooks/ui/useThemeStyles.ts:80 maps the theme's accent straight to --accent-color, and the keyboard focus ring falls back to exactly that variable:
/* src/renderer/index.css:329 */
.focus-ring:focus-visible {
outline: 2px solid var(--focus-ring-color, var(--accent-color, rgba(99, 102, 241, 0.9)));
outline-offset: 2px;
}Maestro is a keyboard-first app, so a focus ring at 1.6:1 against a near-black background is effectively invisible. Users tabbing through the UI on these themes would lose track of where focus is.
Worth noting your text colors are all in great shape (textMain and textDim are 11:1 and up against bgMain, and accentForeground on accent is comfortable everywhere). It's specifically accent that needs lifting.
Here are hue-preserving values that clear 3:1, if useful as a starting point. These just scale luminance and keep your color identity intact:
| Theme | current | suggested | new ratio |
|---|---|---|---|
indigo-blue |
#1f2f63 |
#3a58b9 |
3.24:1 |
deep-wine-red |
#631f30 |
#a73451 |
3.22:1 |
yellow-dark-mustard |
#6b5314 |
#765b16 |
3.22:1 |
deep-purple |
#4f2673 |
#7e3db8 |
3.21:1 |
Feel free to pick your own values, these are only illustrative. The ask is just that accent reaches roughly 3:1 against bgMain so focus and active states stay visible. If you'd rather keep the accents deliberately deep for the aesthetic, the other option is to set an explicit brighter --focus-ring-color for these themes, but bumping accent is the simpler path and keeps the rest of the accent-driven UI (highlights, left borders) legible too.
Minor heads-up, no action needed
Both bots reviewed at commit 3ca0f7c ("Deep Wine Red"), before 990f1ad added the mustard and deep purple variants. That's why their summaries only mention two themes and four IDs. Your diff does correctly define all four, so nothing is actually missing, but the last commit went unreviewed by the bots. That's part of why I checked the full set above.
Once the accent contrast is addressed I'm happy to approve. Thanks again!
|
Thanks! Picked this up in a PR sweep but it conflicts with |
|
Two Markdown issues remain after the current changes:
I did not repeat the existing contrast and rebase feedback. |
|
@felipeggv Thanks again for sticking with this one - the caching work here is still the most valuable part of the PR, and nothing in 1. Needs a rebase on latest
|
Summary
Validation
git diff --checknpm test -- CodeFence.test.tsx MermaidRenderer.test.tsxnpm run lintnpx eslint src/renderer/components/CodeFence/CodeFence.tsx src/renderer/components/MermaidRenderer.tsx --no-warn-ignorednpx prettier --check src/renderer/components/CodeFence/CodeFence.tsx src/renderer/components/MermaidRenderer.tsx src/__tests__/renderer/components/CodeFence.test.tsx src/__tests__/renderer/components/MermaidRenderer.test.tsxvalidate:push:bun run format:check:all && bun run lint && bun run lint:eslint && bun run testSummary by CodeRabbit
max-height.