Improve theme surface token coverage#30
Merged
Merged
Conversation
|
Warning Review limit reached
More reviews will be available in 45 minutes and 49 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR introduces a new design token system for the web app by defining CSS variables (--app-chrome-control-, --app-metadata-, --app-control-icon-, --app-work-row-) in the root stylesheet, generating them across theme profiles, validating the generation with tests, and applying them across UI components to replace hardcoded Tailwind classes and older color tokens. ChangesTheme Surface Token System and Component Migration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
github-actions
Bot
added
size:L
vouch:trusted
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/web/src/index.css`:
- Around line 61-66: The CSS uses the background shorthand which breaks
transition-colors and triggers Stylelint; update the .sidebar-icon-button rule
and its hover variant (the selector .sidebar-icon-button) to use
background-color instead of background (both the base declaration currently
using var(--app-control-icon-bg, transparent) and the hover declaration using
var(--app-control-icon-hover-bg, ...)); keep the rest of the declaration order
(leave `@apply` and transition-colors intact) so the hover color change can be
animated and Stylelint warnings are resolved.
In `@docs/architecture/README.md`:
- Around line 18-25: Update the metadata table's "Last reviewed" field in
docs/architecture/README.md to a current date to reflect the added architecture
navigation; locate the "Last reviewed" metadata entry (the header metadata block
containing "Last reviewed") and change its value to today's review date so the
document's metadata remains accurate after edits to the table (also apply the
same update if there's a duplicate "Last reviewed" entry noted at line 32).
In `@docs/architecture/theme-surface-tokens.md`:
- Around line 34-37: The docs incorrectly state that chrome-control tokens are
derived only in buildProfileAppDepthVariables; update the source note in
theme-surface-tokens.md so it also documents that these tokens are emitted from
the Catppuccin branch in buildAppDepthVariables (in addition to
buildProfileAppDepthVariables), and reference the specific token family names
(`--app-chrome-control-*`) and the functions buildAppDepthVariables and
buildProfileAppDepthVariables so readers can trace generation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ffee8e80-e4e1-4e21-b392-997ed2415e9d
📒 Files selected for processing (13)
apps/web/src/components/BranchToolbar.tsxapps/web/src/components/ChatView.tsxapps/web/src/components/GitActionsControl.tsxapps/web/src/components/Sidebar.tsxapps/web/src/components/chat/ChatHeader.tsxapps/web/src/components/chat/MessageActionButton.tsxapps/web/src/components/chat/MessagesTimeline.tsxapps/web/src/index.cssapps/web/src/theme/theme.logic.test.tsapps/web/src/theme/theme.logic.tsdocs/architecture/README.mddocs/architecture/theme-surface-tokens.mddocs/testing/README.md
💤 Files with no reviewable changes (1)
- docs/testing/README.md
Comment on lines
+61
to
+66
| @apply items-center justify-center rounded-sm p-0.5 text-[var(--app-control-icon-fg,var(--muted-foreground))] transition-colors hover:text-[var(--app-control-icon-hover-fg,var(--foreground))] focus-visible:outline-none focus-visible:ring-1 focus-visible:ring-[color:var(--app-state-focus,var(--ring))]; | ||
| background: var(--app-control-icon-bg, transparent); | ||
| } | ||
|
|
||
| .sidebar-icon-button:hover { | ||
| background: var(--color-background-button-secondary-hover); | ||
| background: var(--app-control-icon-hover-bg, var(--color-background-button-secondary-hover)); |
There was a problem hiding this comment.
Use background-color here instead of background.
transition-colors will stop animating this hover change when the rule switches the background shorthand, and Stylelint is already flagging Line 62 for the declaration placement.
💡 Suggested fix
.sidebar-icon-button {
`@apply` items-center justify-center rounded-sm p-0.5 text-[var(--app-control-icon-fg,var(--muted-foreground))] transition-colors hover:text-[var(--app-control-icon-hover-fg,var(--foreground))] focus-visible:outline-none focus-visible:ring-1 focus-visible:ring-[color:var(--app-state-focus,var(--ring))];
- background: var(--app-control-icon-bg, transparent);
+
+ background-color: var(--app-control-icon-bg, transparent);
}
.sidebar-icon-button:hover {
- background: var(--app-control-icon-hover-bg, var(--color-background-button-secondary-hover));
+ background-color: var(--app-control-icon-hover-bg, var(--color-background-button-secondary-hover));
}📝 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
| @apply items-center justify-center rounded-sm p-0.5 text-[var(--app-control-icon-fg,var(--muted-foreground))] transition-colors hover:text-[var(--app-control-icon-hover-fg,var(--foreground))] focus-visible:outline-none focus-visible:ring-1 focus-visible:ring-[color:var(--app-state-focus,var(--ring))]; | |
| background: var(--app-control-icon-bg, transparent); | |
| } | |
| .sidebar-icon-button:hover { | |
| background: var(--color-background-button-secondary-hover); | |
| background: var(--app-control-icon-hover-bg, var(--color-background-button-secondary-hover)); | |
| `@apply` items-center justify-center rounded-sm p-0.5 text-[var(--app-control-icon-fg,var(--muted-foreground))] transition-colors hover:text-[var(--app-control-icon-hover-fg,var(--foreground))] focus-visible:outline-none focus-visible:ring-1 focus-visible:ring-[color:var(--app-state-focus,var(--ring))]; | |
| background-color: var(--app-control-icon-bg, transparent); | |
| } | |
| .sidebar-icon-button:hover { | |
| background-color: var(--app-control-icon-hover-bg, var(--color-background-button-secondary-hover)); | |
| } |
🧰 Tools
🪛 Stylelint (17.12.0)
[error] 62-62: Expected empty line before declaration (declaration-empty-line-before)
(declaration-empty-line-before)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/web/src/index.css` around lines 61 - 66, The CSS uses the background
shorthand which breaks transition-colors and triggers Stylelint; update the
.sidebar-icon-button rule and its hover variant (the selector
.sidebar-icon-button) to use background-color instead of background (both the
base declaration currently using var(--app-control-icon-bg, transparent) and the
hover declaration using var(--app-control-icon-hover-bg, ...)); keep the rest of
the declaration order (leave `@apply` and transition-colors intact) so the hover
color change can be animated and Stylelint warnings are resolved.
Comment on lines
+18
to
+25
| | If you are doing this | Start here | Then check | | ||
| | ----------------------------- | ------------------------------------------------------------------------------ | --------------------------------------------------------------------- | | ||
| | Getting oriented | [System Overview](system-overview.md) | [`../../AGENTS.md`](../../AGENTS.md) | | ||
| | Checking workspace boundaries | [Runtime Boundaries](runtime-boundaries.md) | [System Overview](system-overview.md) | | ||
| | Changing providers | [Provider Runtime Architecture](provider-runtime.md) | [`../../apps/server/AGENTS.md`](../../apps/server/AGENTS.md) | | ||
| | Changing server boundaries | [Server Architecture Migration Inventory](../server-architecture-migration.md) | [Testing Strategy](../testing/strategy.md) | | ||
| | Changing theme surface tokens | [Theme Surface Token Rules](theme-surface-tokens.md) | [Appearance Regression Testing](../testing/appearance-regressions.md) | | ||
| | Recording a durable decision | [ADR Index](../adr/README.md) | [Repo Governance](../governance/repo-governance.md) | |
There was a problem hiding this comment.
Update the metadata table's review date.
This adds new architecture navigation, so the Last reviewed field above should move forward with the edit as well. As per coding guidelines, "Keep metadata tables current when adding or substantially changing a document".
Also applies to: 32-32
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/architecture/README.md` around lines 18 - 25, Update the metadata
table's "Last reviewed" field in docs/architecture/README.md to a current date
to reflect the added architecture navigation; locate the "Last reviewed"
metadata entry (the header metadata block containing "Last reviewed") and change
its value to today's review date so the document's metadata remains accurate
after edits to the table (also apply the same update if there's a duplicate
"Last reviewed" entry noted at line 32).
Comment on lines
+34
to
+37
| | Family | Tokens | Use for | Source evidence | | ||
| | --------------- | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | ---------------------------------------------------------------------------------------------------------------------------- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | | ||
| | App surfaces | `--app-surface-canvas`, `--app-surface-sidebar`, `--app-surface-topbar`, `--app-surface-panel`, `--app-surface-card`, `--app-surface-card-header`, `--app-surface-composer`, `--app-surface-toolbar`, `--app-surface-toolbar-hover`, `--app-surface-toolbar-active`, `--app-surface-toolbar-border` | App shell depth, panels, cards, composer surfaces, and compact toolbar chrome | Derived in `apps/web/src/theme/theme.logic.ts`; required by `REQUIRED_APP_DEPTH_TOKENS` in `apps/web/src/theme/theme.logic.test.ts`; used by toolbar and git action classes. | | ||
| | Chrome controls | `--app-chrome-control-bg`, `--app-chrome-control-border`, `--app-chrome-control-fg`, `--app-chrome-control-hover-bg`, `--app-chrome-control-hover-fg`, `--app-chrome-control-active-bg` | Compact controls embedded in app chrome where background, border, foreground, hover, and active states need to move together | Derived in `buildProfileAppDepthVariables`; tested in the Catppuccin token expectations. | |
There was a problem hiding this comment.
Broaden the source note for chrome-control tokens.
These tokens are also emitted from the Catppuccin branch in buildAppDepthVariables, so documenting them as derived only in buildProfileAppDepthVariables is inaccurate. As per coding guidelines, "Keep docs source-grounded; do not invent commands, scripts, environment variables, or runtime behavior".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/architecture/theme-surface-tokens.md` around lines 34 - 37, The docs
incorrectly state that chrome-control tokens are derived only in
buildProfileAppDepthVariables; update the source note in theme-surface-tokens.md
so it also documents that these tokens are emitted from the Catppuccin branch in
buildAppDepthVariables (in addition to buildProfileAppDepthVariables), and
reference the specific token family names (`--app-chrome-control-*`) and the
functions buildAppDepthVariables and buildProfileAppDepthVariables so readers
can trace generation.
This was referenced Jun 1, 2026
Merged
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.
Summary
Verification
Summary by CodeRabbit
Style
Documentation
Tests