feat: app-wide theming with presets, fonts, and contrast#21
feat: app-wide theming with presets, fonts, and contrast#21aaditagrawal merged 3 commits intomainfrom
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughAdds eight theming fields to AppSettings, a new themeConfig module to apply those settings to document CSS, wires theme application into startup and route-level effects, and marks the sidebar with a translucency class for styling. Changes
Sequence Diagram(s)sequenceDiagram
participant App as App (startup)
participant Settings as AppSettings Store
participant Theme as themeConfig
participant Doc as document.documentElement
App->>Settings: getAppSettingsSnapshot()
Settings-->>App: initialSettings
App->>Doc: applyAccentColorToDocument(initialSettings.accentColor)
App->>Theme: applyThemeConfigToDocument(initialSettings)
Theme->>Doc: set CSS vars / toggle classes (fonts, sizes, colors, contrast, translucency)
Note over Settings,Theme: At runtime
Settings->>Theme: settings change (uiFont, codeFont, sizes, colors, contrast, translucency)
Theme->>Doc: update CSS vars / classes accordingly
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 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 docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
apps/web/src/appSettings.ts (1)
107-109: Consider constraining numeric theme fields at schema boundary.
uiFontSize,codeFontSize, andcontrastare currently unbounded in persisted settings. Adding schema-level range checks would keep storage normalized and reduce downstream cleanup reliance.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/appSettings.ts` around lines 107 - 109, The persisted numeric theme fields uiFontSize, codeFontSize, and contrast are unbounded; update their Schema.Number pipelines in appSettings.ts (the uiFontSize, codeFontSize, contrast entries that currently use Schema.withConstructorDefault(() => Option.some(0))) to include explicit range constraints (e.g., font sizes constrained to a reasonable range like 8–48 and contrast to 0–100) by composing a min/max or refinement validator (Schema.min/Schema.max or Schema.refine/Schema.range depending on your schema lib) before withConstructorDefault so invalid values are rejected at the schema boundary and storage remains normalized.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/web/src/components/ui/sidebar.tsx`:
- Line 291: The JSX applies the class "sidebar-translucent" unconditionally but
no CSS exists and translucency must be enabled only when the root has
"theme-translucent"; add stylesheet rules scoped under ".theme-translucent
.sidebar-translucent" (not a global .sidebar-translucent) to define the required
translucent background, borders, and backdrop-filter styles so the sidebar
becomes translucent only when the theme toggled by themeConfig.ts (via
settings.translucency) adds "theme-translucent" to the root; update the relevant
CSS/SCSS file where other sidebar styles live to include these scoped rules
(reference: className using "sidebar-translucent" in the Sidebar component and
the theme toggle logic in themeConfig.ts).
In `@apps/web/src/themeConfig.ts`:
- Around line 23-31: In buildFontStack, the current space-check wraps
comma-separated font lists like "Inter, system-ui" in quotes, breaking the CSS
token; change the logic so that if the input trimmed string contains a comma you
treat it as an already-composed font list and do not quote it (simply return
`${trimmed}, ${fallback}`), otherwise keep the existing behavior that quotes
only single multi-word family names (use trimmed.includes(" ") &&
!trimmed.startsWith('"') && !trimmed.startsWith("'") to decide quoting). Ensure
you still append the fallback in all cases; reference the buildFontStack
function and the trimmed/quoted variables when making the change.
---
Nitpick comments:
In `@apps/web/src/appSettings.ts`:
- Around line 107-109: The persisted numeric theme fields uiFontSize,
codeFontSize, and contrast are unbounded; update their Schema.Number pipelines
in appSettings.ts (the uiFontSize, codeFontSize, contrast entries that currently
use Schema.withConstructorDefault(() => Option.some(0))) to include explicit
range constraints (e.g., font sizes constrained to a reasonable range like 8–48
and contrast to 0–100) by composing a min/max or refinement validator
(Schema.min/Schema.max or Schema.refine/Schema.range depending on your schema
lib) before withConstructorDefault so invalid values are rejected at the schema
boundary and storage remains normalized.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1e145813-67a8-44c4-a02d-e0e594a3ecb0
📒 Files selected for processing (5)
apps/web/src/appSettings.tsapps/web/src/components/ui/sidebar.tsxapps/web/src/main.tsxapps/web/src/routes/__root.tsxapps/web/src/themeConfig.ts
| > | ||
| <div | ||
| className="flex h-full w-full flex-col bg-sidebar group-data-[variant=floating]:rounded-lg group-data-[variant=floating]:border group-data-[variant=floating]:border-sidebar-border group-data-[variant=floating]:shadow-sm/5" | ||
| className="sidebar-translucent flex h-full w-full flex-col bg-sidebar group-data-[variant=floating]:rounded-lg group-data-[variant=floating]:border group-data-[variant=floating]:border-sidebar-border group-data-[variant=floating]:shadow-sm/5" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify selector scoping for translucent sidebar styles.
# Expected: `.sidebar-translucent` visual rules should be gated by `.theme-translucent` (or equivalent root state).
rg -n -C3 'theme-translucent|sidebar-translucent' --type=css --type=scss --type=ts --type=tsxRepository: aaditagrawal/t3code
Length of output: 91
🏁 Script executed:
#!/bin/bash
# Search for theme-translucent and sidebar-translucent patterns
# Try without --type=scss since rg doesn't recognize it
# First, find what CSS/style files exist
fd -e css -e scss -e less | head -20
# Then search for the patterns in source files
rg -n 'theme-translucent|sidebar-translucent' --type=css --type=ts --type=tsx 2>/dev/null || \
rg -n 'theme-translucent|sidebar-translucent' 2>/dev/nullRepository: aaditagrawal/t3code
Length of output: 579
🏁 Script executed:
#!/bin/bash
# Check size of index.css and read relevant sections
wc -l apps/web/src/index.css
# Search for sidebar-translucent CSS rules with context
rg -n -B2 -A5 'sidebar-translucent' apps/web/src/index.cssRepository: aaditagrawal/t3code
Length of output: 89
🏁 Script executed:
#!/bin/bash
# sidebar-translucent not found in index.css with pattern search
# Let's check if it's defined in Tailwind config or as a CSS layer
# Search for tailwind config files
fd -e config -e ts -e js | grep -i tailwind
# Search for sidebar-translucent in all files (not just CSS)
rg -l 'sidebar-translucent'
# Check if there's a `@layer` directive in index.css
rg -n '@layer|theme-translucent' apps/web/src/index.cssRepository: aaditagrawal/t3code
Length of output: 118
🏁 Script executed:
#!/bin/bash
# sidebar-translucent is in sidebar.tsx only - check if it's defined there via `@layer`
# Read the sidebar.tsx file to see the `@layer` definition
cat -n apps/web/src/components/ui/sidebar.tsx | head -100Repository: aaditagrawal/t3code
Length of output: 3684
🏁 Script executed:
#!/bin/bash
# Read the `@layer` definition section starting at line 47
cat -n apps/web/src/components/ui/sidebar.tsx | sed -n '47,120p'Repository: aaditagrawal/t3code
Length of output: 2682
🏁 Script executed:
#!/bin/bash
# Search more broadly for `@layer` in sidebar.tsx
rg -n '@layer' apps/web/src/components/ui/sidebar.tsx
# Also get context around the `@layer` definition
rg -n -A30 '@layer base' apps/web/src/components/ui/sidebar.tsxRepository: aaditagrawal/t3code
Length of output: 45
🏁 Script executed:
#!/bin/bash
# Let's search for sidebar-translucent more thoroughly
# It might be in a CSS file or defined via Tailwind `@apply`
rg -n 'sidebar-translucent' --type-list
# Search in all files including CSS
rg -rn 'sidebar-translucent' apps/web/src/
# Check the entire sidebar.tsx file length
wc -l apps/web/src/components/ui/sidebar.tsx
# Look for CSS/Tailwind definitions with broader search
rg -n 'sidebar' apps/web/src/index.css | head -20Repository: aaditagrawal/t3code
Length of output: 788
🏁 Script executed:
#!/bin/bash
# The file is 992 lines - read the section around line 291 where sidebar-translucent is used
cat -n apps/web/src/components/ui/sidebar.tsx | sed -n '285,310p'
# Also search for any `@layer` directives in the entire file
rg -n '@layer' apps/web/src/components/ui/sidebar.tsxRepository: aaditagrawal/t3code
Length of output: 1101
🏁 Script executed:
#!/bin/bash
# sidebar-translucent is applied but not defined via `@layer`
# Check if it's a Tailwind utility class by looking at tailwind config
fd -e config.ts -e tailwind.config.ts -e tailwind.config.js
# Also read the entire index.css to see all CSS definitions
cat apps/web/src/index.css | head -200Repository: aaditagrawal/t3code
Length of output: 6624
🏁 Script executed:
#!/bin/bash
# sidebar-translucent appears in markup but has no CSS definition
# Let's search for any reference to translucent (without sidebar prefix)
rg -n 'translucent' apps/web/src/
# Check if there's a tailwind.config file
fd tailwind.config
# Also check index.css for the complete file length and look for any dynamic class generation
tail -100 apps/web/src/index.cssRepository: aaditagrawal/t3code
Length of output: 2840
🏁 Script executed:
#!/bin/bash
# sidebar-translucent has no CSS definition in index.css
# Check if it's a Tailwind utility defined in config
cat apps/web/src/index.css | grep -n 'sidebar'
# Look for tailwind.config or theme definitions
fd -type f \( -name "tailwind.config*" -o -name "*.config.ts" \) | xargs grep -l 'sidebar-translucent' 2>/dev/null || echo "No tailwind config found with sidebar-translucent"
# Also check if there's a CSS-in-JS or other styling approach
rg -n 'sidebar-translucent' --type-list 2>/dev/null || rg -rn 'sidebar-translucent' apps/web/Repository: aaditagrawal/t3code
Length of output: 576
🏁 Script executed:
#!/bin/bash
# sidebar-translucent class exists in markup but has no CSS definition
# Let's check the themeConfig to understand the theme-translucent mechanism better
cat -n apps/web/src/themeConfig.ts | sed -n '90,110p'
# Also search for any CSS that uses theme-translucent selector
rg -n 'theme-translucent' apps/web/src/index.css
# Check if sidebar-translucent might be a planned/TODO style
rg -n 'sidebar' apps/web/src/index.cssRepository: aaditagrawal/t3code
Length of output: 557
Add CSS rules for sidebar-translucent gated by .theme-translucent.
The sidebar-translucent class is applied unconditionally in line 291, but has no CSS definition. When styling is added, it must be scoped so translucency only applies when the root element has the theme-translucent class (set conditionally in themeConfig.ts based on settings.translucency).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/components/ui/sidebar.tsx` at line 291, The JSX applies the
class "sidebar-translucent" unconditionally but no CSS exists and translucency
must be enabled only when the root has "theme-translucent"; add stylesheet rules
scoped under ".theme-translucent .sidebar-translucent" (not a global
.sidebar-translucent) to define the required translucent background, borders,
and backdrop-filter styles so the sidebar becomes translucent only when the
theme toggled by themeConfig.ts (via settings.translucency) adds
"theme-translucent" to the root; update the relevant CSS/SCSS file where other
sidebar styles live to include these scoped rules (reference: className using
"sidebar-translucent" in the Sidebar component and the theme toggle logic in
themeConfig.ts).
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/web/src/themeConfig.ts (1)
75-91: Extract duplicated color-override branches into a helper.The background/foreground override blocks are duplicated; a tiny helper would reduce drift risk and keep future changes consistent.
♻️ Proposed refactor
+function applyColorOverride( + root: CSSStyleDeclaration, + cssVar: "--theme-background-override" | "--theme-foreground-override", + value: string | null | undefined, +): void { + if (value && isValidAccentColor(value)) { + root.setProperty(cssVar, normalizeAccentColor(value)); + } else { + root.removeProperty(cssVar); + } +} + export function applyThemeConfigToDocument(settings: AppSettings): void { if (typeof document === "undefined") return; const root = document.documentElement.style; @@ - if (settings.backgroundColorOverride && isValidAccentColor(settings.backgroundColorOverride)) { - root.setProperty( - "--theme-background-override", - normalizeAccentColor(settings.backgroundColorOverride), - ); - } else { - root.removeProperty("--theme-background-override"); - } - if (settings.foregroundColorOverride && isValidAccentColor(settings.foregroundColorOverride)) { - root.setProperty( - "--theme-foreground-override", - normalizeAccentColor(settings.foregroundColorOverride), - ); - } else { - root.removeProperty("--theme-foreground-override"); - } + applyColorOverride(root, "--theme-background-override", settings.backgroundColorOverride); + applyColorOverride(root, "--theme-foreground-override", settings.foregroundColorOverride);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/themeConfig.ts` around lines 75 - 91, Extract the duplicated background/foreground override logic into a small helper function (e.g., applyColorOverride) that accepts the CSS variable name and the settings color value; inside the helper call isValidAccentColor and either call root.setProperty(varName, normalizeAccentColor(color)) or root.removeProperty(varName). Replace the two blocks that reference settings.backgroundColorOverride and settings.foregroundColorOverride with calls to applyColorOverride("--theme-background-override", settings.backgroundColorOverride) and applyColorOverride("--theme-foreground-override", settings.foregroundColorOverride) so the code centrally uses isValidAccentColor, normalizeAccentColor, root.setProperty and root.removeProperty.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/web/src/themeConfig.ts`:
- Around line 75-91: Extract the duplicated background/foreground override logic
into a small helper function (e.g., applyColorOverride) that accepts the CSS
variable name and the settings color value; inside the helper call
isValidAccentColor and either call root.setProperty(varName,
normalizeAccentColor(color)) or root.removeProperty(varName). Replace the two
blocks that reference settings.backgroundColorOverride and
settings.foregroundColorOverride with calls to
applyColorOverride("--theme-background-override",
settings.backgroundColorOverride) and
applyColorOverride("--theme-foreground-override",
settings.foregroundColorOverride) so the code centrally uses isValidAccentColor,
normalizeAccentColor, root.setProperty and root.removeProperty.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e197fcb3-611e-4fa2-a1e8-eba1d1c79d8b
📒 Files selected for processing (1)
apps/web/src/themeConfig.ts
2a3a059 to
c3cb2b2
Compare
Summary
Test plan
Summary by CodeRabbit
New Features
Style