fix(app): restore model picker hover and tooltip contrast#453
Conversation
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, 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 the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. 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 (8)
📝 WalkthroughWalkthroughModel picker visual regression fix removes ChangesModel Picker Tooltip & List Styling Accessibility Fix
Sequence DiagramsequenceDiagram
actor User
participant Picker as Model Picker Dialog
participant Tooltip as Model Tooltip
participant List as List Component
participant CSS as CSS Styles
User->>Picker: Open slash command
Picker->>List: Render list with items
List->>CSS: Apply list item styling
User->>List: Hover and click first row
List->>List: Mark item data-active="true"
CSS-->>List: Apply sunken background (not raised)
List->>Tooltip: Show tooltip for hovered item
Tooltip->>CSS: Apply text-13-regular (no text-fg-on-brand)
CSS-->>Tooltip: Render with inherited foreground color
Tooltip-->>User: Display with accessible contrast ratio ≥3
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~25 minutes 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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/app/e2e/models/model-picker-visual.spec.ts`:
- Line 38: The test uses a fragile selector row =
picker.locator('[data-slot="list-item"]').first() — replace the data-slot
selector with a stable semantic or component hook: update the picker.locator
call to use a role-based locator (e.g., getByRole / locator('role', { name: ...
})) or a data-component/data-action attribute (e.g.,
'[data-component="model-list-item"]') so tests target the visible semantic
element; modify the locator in the same statement (picker.locator(...).first())
and any related assertions to use the new selector.
In `@packages/app/src/components/model-picker-hotfix.test.ts`:
- Around line 16-18: The assertion that checks for "background:
var(--surface-sunken)" is too broad and can pass if that token appears outside
the active-row block; replace the loose expect(source).toContain("background:
var(--surface-sunken)") with a scoped regex assertion that ensures the
background declaration appears inside the &\[data-active="true"\] block (similar
to the existing negative regex). In other words, use expect(source).toMatch(...)
with a pattern like
&\[data-active="true"\]\s*\{[\s\S]*?background:\s*var\(--surface-sunken\) to
tightly scope the check to the active block.
🪄 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: e3af72ed-a441-48b8-a3cd-0c3037cf3d9c
📒 Files selected for processing (4)
packages/app/e2e/models/model-picker-visual.spec.tspackages/app/src/components/model-picker-hotfix.test.tspackages/app/src/components/model-tooltip.tsxpackages/ui/src/components/list.css
There was a problem hiding this comment.
Code Review
This pull request introduces visual regression tests for the model picker and updates UI components, specifically adjusting background colors for active list items and removing text color classes in tooltips. The review feedback recommends using contrast calculations for hover state visibility checks and increasing the tooltip contrast threshold to 4.5 to align with WCAG AA accessibility standards.
5fa6fd3 to
eab8cfc
Compare
eab8cfc to
9df9164
Compare
Summary
Restores visible hover state across the prompt input selectors and fixes model tooltip contrast after the #440 visual-token work.
Covered prompt surfaces:
Selectmenu item highlight/hover stateWhy
#440's token-layer slices remapped several menu/list hover backgrounds to
surface-raised, which is also the popover background. Hovered rows therefore had no visible state across multiple prompt controls, not just the model picker.The model tooltip also forced detail rows to
text-fg-on-brand, which can become unreadable when the inverse tooltip background changes across themes.This is a small hotfix so later #440 slices can continue from a working baseline instead of carrying the regression forward.
Related Issue
Follow-up to #440.
Human Review Status
Pending. A human should make the final merge decision after reviewing the final diff and verification evidence.
Review Focus
Please focus on whether the fix stays scoped to prompt selector hover/tooltip contrast, without pulling forward any deferred #440 visual rewrite work.
Risk Notes
Low. The
ListandSelecthover tokens are shared, so other list/select menu surfaces will now usesurface-sunkenfor active/highlighted rows instead of matching their raised container. That is intentional for visible state contrast.How To Verify
Screenshots or Recordings
No screenshot attached. The committed Playwright E2E opens the prompt model picker, workspace picker, and reasoning-effort picker; hovers menu rows; asserts hovered row backgrounds differ from popover backgrounds; and checks model tooltip foreground/background contrast is at least 3:1.
Checklist
dev, and my PR title and commit messages use Conventional Commits in English