feat(ui): warm-tone three-tier sidebar separation + contrast bump (dark/light) - #847
Conversation
|
Warning Review limit reached
Your plan currently allows 1 review/hour. Refill in 30 minutes and 2 seconds. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more review capacity refills, 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 have higher rate limits than trial, open-source, and free plans. In all cases, review capacity refills continuously over time. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR migrates several components from --surface-base to --surface-raised, restructures dark-theme surface and ink/icon tokens (3-tone surface mapping), adds a dark-mode text-field input-wrapper override, and updates the PawWork theme overrides and picker test to match. ChangesSurface token palette consolidation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
There was a problem hiding this comment.
Code Review
This pull request updates the design system's color tokens and component styles, focusing on dark mode elevation and sidebar separation. It introduces a 3-tier surface system in dark mode, updates foreground and icon colors for better legibility, and adjusts the picker, popover, and text-field components to utilize these new tokens. Feedback was provided regarding the use of typography variables instead of hardcoded values and the removal of redundant CSS rules in the popover component.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@packages/ui/src/components/text-field.css`:
- Around line 233-243: The dark-mode selector applies
:not([data-disabled]):not([data-readonly]) to [data-slot="input-wrapper"] but
the disabled/readonly attributes live on the input root, so the guard is
ineffective; update the selectors used in both rules (the
:root[data-color-scheme="dark"] and the `@media` block) to place
:not([data-disabled]):not([data-readonly]) on the input root selector
([data-component="input"][data-variant="normal"]) instead of on
[data-slot="input-wrapper"] so the dark background (var(--surface-raised)) is
only applied when the input root is not disabled or readonly.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 52b2731d-f167-4c10-8813-a39a3480cdb9
📒 Files selected for processing (6)
packages/ui/src/components/picker.csspackages/ui/src/components/picker.test.tspackages/ui/src/components/popover.csspackages/ui/src/components/text-field.csspackages/ui/src/styles/theme.csspackages/ui/src/theme/themes/pawwork.json
Perf delta summaryComparator: pass
|
cc344d5 to
224a699
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/ui/src/components/text-field.css (1)
233-243:⚠️ Potential issue | 🟠 Major | ⚖️ Poor tradeoffGuard is scoped to the wrong element, so disabled/readonly can be overridden in dark mode.
On Lines 235 and 241,
:not([data-disabled]):not([data-readonly])is applied to[data-slot="input-wrapper"], but disabled/readonly state is set on the input root (see Lines 123-138). This makes the guard ineffective and allows the dark--surface-raisedbackground to override disabled/readonly wrapper backgrounds.🔧 Proposed fix
:root[data-color-scheme="dark"] - [data-component="input"][data-variant="normal"] - [data-slot="input-wrapper"]:not([data-disabled]):not([data-readonly]) { + [data-component="input"][data-variant="normal"]:not([data-disabled]):not([data-readonly]) + [data-slot="input-wrapper"] { background: var(--surface-raised); } `@media` (prefers-color-scheme: dark) { :root:not([data-color-scheme="light"]) - [data-component="input"][data-variant="normal"] - [data-slot="input-wrapper"]:not([data-disabled]):not([data-readonly]) { + [data-component="input"][data-variant="normal"]:not([data-disabled]):not([data-readonly]) + [data-slot="input-wrapper"] { background: var(--surface-raised); } }🤖 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 `@packages/ui/src/components/text-field.css` around lines 233 - 243, The dark-mode selector currently applies the disabled/readonly guards to [data-slot="input-wrapper"] which is wrong; the disabled/readonly attributes live on the input root ([data-component="input"]), so update both dark-mode rules to move the :not([data-disabled]):not([data-readonly]) predicates from [data-slot="input-wrapper"] to the root selector ([data-component="input"][data-variant="normal"]) so the guard correctly prevents the dark background (--surface-raised) from overriding disabled/readonly wrapper styles; adjust the selectors that reference [data-slot="input-wrapper"] accordingly so only enabled/readonly roots receive the dark background.
🤖 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.
Duplicate comments:
In `@packages/ui/src/components/text-field.css`:
- Around line 233-243: The dark-mode selector currently applies the
disabled/readonly guards to [data-slot="input-wrapper"] which is wrong; the
disabled/readonly attributes live on the input root ([data-component="input"]),
so update both dark-mode rules to move the
:not([data-disabled]):not([data-readonly]) predicates from
[data-slot="input-wrapper"] to the root selector
([data-component="input"][data-variant="normal"]) so the guard correctly
prevents the dark background (--surface-raised) from overriding
disabled/readonly wrapper styles; adjust the selectors that reference
[data-slot="input-wrapper"] accordingly so only enabled/readonly roots receive
the dark background.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: b4ff220a-5e55-47dc-bee6-5b741168750a
📒 Files selected for processing (6)
packages/ui/src/components/picker.csspackages/ui/src/components/picker.test.tspackages/ui/src/components/popover.csspackages/ui/src/components/text-field.csspackages/ui/src/styles/theme.csspackages/ui/src/theme/themes/pawwork.json
✅ Files skipped from review due to trivial changes (1)
- packages/ui/src/components/picker.css
Move the dark-mode `:not([data-disabled]):not([data-readonly])` guard from [data-slot="input-wrapper"] up to the input root selector. The disabled / readonly attributes are set on the root (see Lines 123-138), not on the wrapper, so the original guard was a no-op and the dark `--surface-raised` override would have hijacked the disabled / readonly cream background. Reported by coderabbitai on PR #847. Also clean up popover-item selected styling: replace the hardcoded `font-weight: 500` with `var(--font-weight-emphasis)` to match picker.css, and drop the redundant `color: var(--fg-strong)` — the base [data-slot="popover-item"] rule already sets it (popover.css:106). Reported by gemini-code-assist on PR #847.
Reverse the 2026-05-14 "three columns same color" dark lock by lifting `--sidebar` from canvas into its own middle tier; bump `--surface-raised` and dark ink to restore depth. Light `--sidebar` moves one notch to separate from cream. Dark (pawwork.json + theme.css dual track): - --sidebar #1a1917 → #232120 (chrome middle tier) - --surface-raised #262523 → #2d2a27 (~1.27:1 against canvas) - --fg-strong / --fg-base #ebe7e0 → #f4efe7 (+9 sRGB delta sharpen) - --fg-weak / --fg-weaker #7e7872 → #928c82 (~5.6:1, AA large-text) - --icon-strong #ebe7e0 → #f4efe7 - --icon-base / --icon-weak #7e7872 → #928c82 - Rewrite the main dark block comment from "2-tier collapse" to "3-tier". Light: - --sidebar #f9f9f9 → #f2f0ed (distinguish from cream #faf9f7) Explicitly unchanged: canvas, hairline 0.08, brand orange, R-B=3, font sizes, spacing, shadows, radii, light fg / icon, palette.ink (terminal seed, intentionally decoupled from chrome). Expected cascade (raising --surface-raised lifts every raised consumer): dialog, composer dock, sheet, menu, msg-user bubble, list-hover, command-palette body, kbd.
Mirror the token lift in the three floating-surface containers. With the new dark 3-tier system, the old --surface-base equals canvas; floating elements that stayed on canvas would read as "pressed in" instead of floating. Moving them to --surface-raised restores the depth cue. popover.css: - container: surface-base → surface-raised - item hover / focus-visible: bg-cream → row-hover-overlay (cream collapses to canvas in dark and would press the item in; row-hover-overlay is the spec hover token for popover family) - item active / selected: bg-cream → surface-interactive-base (aligns with DESIGN.md L622 popover-family active rule) - selected adds font-weight 500 + color var(--fg-strong) picker.css: - [data-picker-content] background: surface-base → surface-raised - picker.test.ts: rename the test and update the assertion to match. text-field.css: - New dark-only override on both tracks ([data-color-scheme="dark"] + @media (prefers-color-scheme: dark)) - Targets only the normal-variant wrapper when neither data-disabled nor data-readonly is set. - The :not() guard keeps disabled / readonly on --bg-cream (= canvas in dark); a normal input lifts to raised, and disabled sinks back to thread, which reads as a legitimate disabled state. - Light normal-variant keeps --surface-base = white, unchanged.
Move the dark-mode `:not([data-disabled]):not([data-readonly])` guard from [data-slot="input-wrapper"] up to the input root selector. The disabled / readonly attributes are set on the root (see Lines 123-138), not on the wrapper, so the original guard was a no-op and the dark `--surface-raised` override would have hijacked the disabled / readonly cream background. Reported by coderabbitai on PR #847. Also clean up popover-item selected styling: replace the hardcoded `font-weight: 500` with `var(--font-weight-emphasis)` to match picker.css, and drop the redundant `color: var(--fg-strong)` — the base [data-slot="popover-item"] rule already sets it (popover.css:106). Reported by gemini-code-assist on PR #847.
8b7c70c to
3e37a05
Compare
The dock card already gets card identity from the 1px hairline plus the sole-sanctioned dark drop shadow (0 4px 12px rgba(0,0,0,0.3)); the extra --surface-raised fill in dark mode added a third visual cue and made the dock pop ~17 sRGB above thread canvas, more than Codex Desktop or Claude Desktop. Letting the dock fall back to --bg-base (= canvas in dark = white in light) keeps both modes symmetric and reuses the same logic. dock-card.css: remove the dark override that set background-color to --surface-raised; the dark rule now only carries the box-shadow override. Light mode is untouched (it already uses --bg-base). prompt-input.tsx: the bottom fade gradient must match the dock fill so text fading behind the toolbar reads cleanly; switch the gradient source from --surface-raised to --bg-base on both modes. DESIGN.md L189 + L417 updated to record the decision: dock no longer sits in the raised tier, and the "sole sanctioned dark drop-shadow" rule survives independent of the fill choice.
Pure bg-base fallback (a27d182) made the dark dock invisible — the sole-sanctioned drop shadow vanishes into canvas and the 1px hairline alone cannot carry the card boundary. Move dock to sidebar tone in dark (matches Codex Desktop) while keeping it white in light (matches Claude Desktop and Codex light); introduce `--surface-dock` so the inline gradient mask in prompt-input.tsx stays aligned with dock fill across both modes without duplicating the dark override. theme.css: - light :root: `--surface-dock: #ffffff` (= bg-base in light) - dark explicit + @media: `--surface-dock: #232120` (= sidebar in dark) pawwork.json: mirror the alias in both light and dark overrides so theme-parity stays green. dock-card.css: `[data-dock="card"]` background switches to `var(--surface-dock)`; the dark override now only carries the sole-sanctioned drop shadow, no fill duplication. prompt-input.tsx: bottom fade gradient uses `var(--surface-dock)` so the fade-to-fill matches the dock card in both modes. DESIGN.md L189 + L417 updated: dock sits at the chrome-middle tier (Codex same-track), not in raised; `--surface-dock` documented as the single source of truth for dock fill.
…-9 sRGB Drop the dedicated `--surface-dock` token in favor of `--surface-raised`. Composer dock shares the same elevation role as dialog, popover, card, and textfield — all are "raised surfaces" with identical physical behavior (light: shadow on white; dark: one lightness tier above sidebar). The token was a single-use alias introduced to back an inline TSX gradient; that gradient now reads `--surface-raised` directly, removing a mode-dependent indirection. In dark, this lifts the dock from `#232120` (same as sidebar) to `#2D2A27` (raised). The previous two-tier dock-as-sidebar layout matched classic IDEs (VS Code, Cursor) but pulled the composer out of the chat-app convention where ChatGPT, Claude, Perplexity, and Gemini all lift the composer one tier above the thread. Three tiers now read as thread #1A1917 → sidebar #232120 → raised dock #2D2A27, uniform +9 sRGB steps. In light, sidebar moves from `#F2F0ED` (-15 sRGB vs thread) to `#F8F6F3` (-9 sRGB). The original tone sat at the heavy end of modern productivity apps (alongside Gemini at -15); the new value lands at the median alongside Notion (-9), Copilot, and Figma (-10). The -9 step mirrors the dark mode's +9 between every tier, giving the surface system uniform elevation steps across both modes. Composer dock in light remains visually identical (raised = #FFFFFF = thread) — shadow continues to express elevation per the asymmetric light/dark elevation convention. Verification: theme-parity 200 tests pass; bun snap app-shell + sidebar visually confirm both surfaces. Source rationale lives in docs/research/2026-05-23-dark-mode-composer-dock-layering.md and docs/research/2026-05-23-light-mode-sidebar-contrast.md (local-only).
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/ui/src/theme/themes/pawwork.json (1)
57-57:⚠️ Potential issue | 🔴 CriticalLight sidebar value doesn't match PR objectives.
Same issue as theme.css line 90: the PR objectives specify
#f2f0edbut the code has#f8f6f3. The two files are consistent with each other, but both differ from the documented target.🤖 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 `@packages/ui/src/theme/themes/pawwork.json` at line 57, The "sidebar" color value in pawwork.json is incorrect — update the "sidebar" key in pawwork.json from "`#f8f6f3`" to the PR-specified "`#f2f0ed`" and also update the matching sidebar color in the theme.css entry (the sidebar color declaration) so both files use "`#f2f0ed`" and remain consistent with the documented target.
🤖 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 `@packages/ui/src/styles/theme.css`:
- Line 90: The CSS variable --sidebar in packages/ui/src/styles/theme.css
currently uses `#f8f6f3` but the comment/spec mentions `#f2f0ed`; either update the
PR description/objectives to state that the intended light sidebar color is
`#f8f6f3` (and leave --sidebar and packages/ui/src/theme/themes/pawwork.json
`"sidebar": "`#f8f6f3`"` as-is) or change the theme values to the spec color by
updating --sidebar in theme.css and the `"sidebar"` value in
packages/ui/src/theme/themes/pawwork.json to `#f2f0ed` so both code and spec
align.
---
Duplicate comments:
In `@packages/ui/src/theme/themes/pawwork.json`:
- Line 57: The "sidebar" color value in pawwork.json is incorrect — update the
"sidebar" key in pawwork.json from "`#f8f6f3`" to the PR-specified "`#f2f0ed`" and
also update the matching sidebar color in the theme.css entry (the sidebar color
declaration) so both files use "`#f2f0ed`" and remain consistent with the
documented target.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 7ab75d3d-e70b-4e3e-b7f2-5941ad16b206
📒 Files selected for processing (7)
packages/ui/src/components/dock-card.csspackages/ui/src/components/picker.csspackages/ui/src/components/picker.test.tspackages/ui/src/components/popover.csspackages/ui/src/components/text-field.csspackages/ui/src/styles/theme.csspackages/ui/src/theme/themes/pawwork.json
✅ Files skipped from review due to trivial changes (1)
- packages/ui/src/components/picker.css
Update popover.stories.tsx MenuMatrix (light + dark wrappers) and WithSearch wrapper to use --surface-raised, matching the actual content background in popover.css. The hand-crafted story wrappers previously used --surface-base, which drifts in dark mode (canvas #1a1917 vs raised #2d2a27). Visual regressions on popover hover / active / selected states would have validated the wrong background tier.
Summary
Lift dark
--sidebarout of canvas into its own middle tier (#1a1917 → #232120), bump--surface-raised(#262523 → #2d2a27) and dark ink for legibility, and ease light--sidebarto its final value (#f9f9f9 → #f8f6f3, -9 sRGB vs thread). Composer dock collapses onto--surface-raised(the single elevation token shared with dialog / popover / card / textfield) — removing the dedicated--surface-dockalias. Migrate the three floating-surface containers (popover, picker, textfield normal-variant) onto--surface-raised; switch popover hover/focus-visible to--row-hover-overlayso they stop reading as "pressed in" against the new raised container.The surface system now reads with uniform ±9 sRGB steps across both modes:
#ffffff→ sidebar#f8f6f3(-9). Raised collapses to#ffffff(dock = thread by color, lifted by shadow per HIG/Fluent convention).#1a1917→ sidebar#232120(+9) → raised#2d2a27(+9). Three uniform tiers; dock joins raised tier so the composer lifts above sidebar in chat-app convention (matches ChatGPT / Claude / Gemini, departs from VS Code / Cursor).Unchanged: canvas, hairline 0.08, brand orange, R-B=3, font sizes, spacing, shadows, radii.
Iteration history
The PR went through three rounds of in-flight tuning after the initial commit, captured in successive commits on the branch:
1cb6c361c9) — light sidebar#f9f9f9 → #f2f0ed(-15 sRGB); dock not yet differentiated from sidebar in dark.ab7c431c8e) — introduced--surface-dockalias so the inline TSX gradient could share a cross-mode token; dock pinned to sidebar tone in dark (Codex IDE style, two tiers).6866b9fc57) — collapsed--surface-dockinto--surface-raised(one elevation role, no alias); promoted dock to raised tier in dark (#2d2a27, three tiers); eased light sidebar from#f2f0ed(-15) to#f8f6f3(-9). Research backing in local-onlydocs/research/2026-05-23-dark-mode-composer-dock-layering.mdanddocs/research/2026-05-23-light-mode-sidebar-contrast.md.Why
Dark sidebar / thread / titlebar shared a single canvas color (the 2026-05-14 "three columns same color" lock); the column boundary was carried only by the 0.08 hairline. Combined with
--fg-weakat a borderline ~4.7:1, the product felt flatter than Codex Desktop or Claude Desktop. After ruling out density, bubble, home-surface, and brand-coverage hypotheses against the actual code, the only verifiable difference was tone. Light--sidebar#f9f9f9against#ffffffthread had a 6-luma delta and almost collided with cream#faf9f7, so it gets the same one-notch lift — calibrated to#f8f6f3after the third-round research showed -15 sat at the heavy end (Gemini territory) while modern AI-chat peers (Notion, Copilot, Figma) cluster around -9 to -10.The full design rationale and the visual lock are in the local-only
docs/design/scratch/dark-color-tone-compare.html; the per-commit implementation plan is in the local-onlydocs/architecture/2026-05-23-dark-light-color-tier-rework.md. Both were converged across 10 rounds of multi-model code review (Claude + Codex) until Codex returned 0 P1 in the final round.Related Issue
None. The change is internal-review driven; no open issue tracks the sidebar tone specifically.
Review Focus
#232120against#1a1917is only ~9 sRGB delta (~1.06:1 perceived). Checksidebar-divider.pngand decide whether the lift is enough or needs to widen further.--surface-interactive-baseper DESIGN.md L622; selected additionally bumps tofont-weight: 500+--fg-strong. Active is differentiated only byopacity: 0.8. Confirm the two states still read distinct enough.:not()guard::not([data-disabled]):not([data-readonly])keeps the disabled / readonly path on--bg-cream(which collapses to canvas in dark). A normal input lifts to raised, and disabled sinks back to thread — the asymmetry is intentional (mirrors light's cream-tint disabled hint) but is counter-intuitive at first read.--sidebar: macOS titlebar left segment and the right-panel selected side tab both consume--sidebar(packages/app/src/index.css#L97-L106 for the titlebar gradient stops, #L130-L133 for the selected tab). They will track the new sidebar value (intended — same pattern as Codex Desktop / VS Code on macOS), butsnapis web Chromium only; reviewer needs to confirm the native titlebar viabun run dev:desktop.Risk Notes
Expected cascade (
--surface-raisedrise pulls every consumer up one notch): dialog, composer dock, sheet, menu, msg-user bubble, list-hover, command-palette body, and kbd all shift. This is a single-token side effect, not missed edits.Native check not run locally: macOS titlebar left-segment color is gated on Electron.
snapcannot reach it — reviewer must runbun run dev:desktopon their machine.Pre-existing dev test failures (5):
bun testreports 638 pass / 5 fail. All five reproduce onorigin/devaftergit stash: Button / IconButton size assertions,undefined-tokensflagging--border-active(referenced insession-turn.cssbut undefined intheme.css— an unrelated drift), andmessage-part-stale. None caused by this PR.Explicit out-of-scope follow-ups:
palette.inkterminal seed stays at#ebe7e0(terminal is an independent visual system).switch.cssdark thumb still hardcodes#ebe7e0at L125 and L129 — token-cleanup follow-up.colors.txt(Tailwind alias generator input) not edited because it would be overwritten on the next generator run.--fg-weak#8c817aagainst white is ~3.94:1 (AA borderline); held under the four-tier ink lock pending user feedback.--brand-primary-hover,--diff-add,--diff-deltoken reductions deferred (≥2 real callsites each).--border-activeundefined-token drift insession-turn.css(pre-existing).How To Verify
Screenshots or Recordings
Checklist
bug,enhancement,task,documentation. Type labels are author-added; the labeler bot does NOT assign them. Add the label in the GitHub UI, then tick this.app,ui,platform,harness,ci. The labeler bot assigns these on PR open based on changed paths. Confirm the bot's choice (or override if wrong), then tick this.P0,P1,P2,P3. The priority-triage bot suggests one on PR open. Confirm or override, then tick this.Pending,Approved by @<reviewer>, orNot required: <reason>(default isPending; "not required" is restricted to bot-authored low-risk PRs).dev, and my PR title and commit messages use Conventional Commits in English.Human Review Status
Pending
Summary by CodeRabbit