fix(sidebar/main): impl modal selector and fix logos and items#74
Conversation
📝 WalkthroughWalkthroughThis PR refreshes the platform branding from Canvas/Phase to December across auth, navigation, and UI components. It adds settings support to the notifications popover, introduces model selection to the prompt footer, and updates icon collections and styling throughout. ChangesPlatform Branding and Navigation Refresh
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
web/src/features/navigation/components/SidebarFooter.tsx (2)
78-79:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHardcoded personal email as a default user value.
'dev.chaitanyasonawane@gmail.com'is shipped as the defaultuserEmailand'phasehuman'as the defaultuserName. A real personal address baked into a public component is a PII/branding leak — it will display on any code path whereprofile?.emailis missing, including unauthenticated/loading states. Use a placeholder ("you@example.com" or empty string) and let upstream decide what to render.The same value is also hardcoded in
UserProfilePopover.tsxat the prop default; consider scrubbing both.
86-91:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winWiring
onSettings={onProfile}conflates settings with profile.The
NotificationsPopover"Settings" entry will invokeonProfile, which from the rest of this component appears to open a profile flow rather than a notification-settings flow. If that is a temporary placeholder, leave a TODO; if it is intentional, naming the callbackonSettingsfor an action that opens profile is going to be confusing for the next reader. Same applies to theonSettings={onProfile}onUserProfilePopoverandProfileCardModalabove.web/src/features/navigation/components/Sidebar.tsx (1)
11-11:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAvoid
anyon the publicuserprop.
user?: anydefeats the type system at the boundary betweenSidebarand its consumers. Define the actual shape (or import the existing user type) so downstream usage likeuser?.nameinSidebarFooteris checked. As per coding guidelines, "TypeScript must be strict; prefer Zod schemas for API boundaries".Suggested typing
-const Sidebar: React.FC<SidebarProps & { user?: any }> = ({ +type SidebarUser = { name?: string } +const Sidebar: React.FC<SidebarProps & { user?: SidebarUser }> = ({web/src/shared/components/ui/PromptFooter.tsx (1)
6-21:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftSelected model is never propagated to the submission path.
selectedModelis local state andonSubmittakes no arguments, so whichever model the user picks is dropped on the floor at send time. This makes the new selector decorative until the API is extended (e.g.onSubmit: (model: string) => voidor by lifting the state into the parent that owns the prompt request). Worth resolving in this PR or the feature is non-functional.Also applies to: 145-156
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: f510d46f-669a-4ba8-8437-48ee9d639720
📒 Files selected for processing (14)
web/src/features/auth/components/AuthModalAuthStep.tsxweb/src/features/auth/components/AuthModalOtpStep.tsxweb/src/features/home/components/HomeHeader.tsxweb/src/features/home/components/HomeHero.tsxweb/src/features/navigation/components/NotificationsPopover.tsxweb/src/features/navigation/components/Sidebar.tsxweb/src/features/navigation/components/SidebarFooter.tsxweb/src/features/navigation/components/SidebarHeader.tsxweb/src/features/navigation/components/SidebarNavItem.tsxweb/src/features/navigation/components/UserProfilePopover.tsxweb/src/features/profile/components/ProfileSettings.tsxweb/src/features/projects/components/ProjectListView.tsxweb/src/shared/components/ui/Icons.tsxweb/src/shared/components/ui/PromptFooter.tsx
💤 Files with no reviewable changes (1)
- web/src/features/projects/components/ProjectListView.tsx
| <a | ||
| href="https://x.com/phasehumans" | ||
| target="_blank" | ||
| rel="noopener noreferrer" | ||
| className="text-textMuted hover:text-textMain transition-colors flex items-center justify-center p-1" | ||
| > | ||
| <Icons.XLogo className="w-[15px] h-[15px]" /> | ||
| </a> | ||
| <a | ||
| href="https://github.com/phasehumans/phasehumans.dev" | ||
| href="https://github.com/phasehumans/december" | ||
| target="_blank" | ||
| rel="noopener noreferrer" | ||
| className="text-textMuted hover:text-textMain transition-colors flex items-center justify-center p-1" |
There was a problem hiding this comment.
Add accessible names to the social links.
Both anchors are icon-only, so assistive tech gets an unnamed control here. Add aria-labels (and optionally mark the SVGs aria-hidden) so the X and GitHub links are discoverable.
| setPosition({ | ||
| bottom: window.innerHeight - rect.top + 8, | ||
| left: rect.left - 260 + rect.width, // align to right or just fixed width. Wait, rect.left is the notification icon. Let's align left to something reasonable or just fixed width of 300. We will set left to rect.left - 250 so it opens to the left. | ||
| left: rect.left - 250, | ||
| width: 280, | ||
| }) |
There was a problem hiding this comment.
Nested menu position can desync from the popover.
Line 41 sets left: rect.left - 250, line 109 then clamps with Math.max(10, position.left), but the nested menu at line 130 uses the raw position.left + 296. When the anchor is near the left edge and the popover is shifted to 10, the menu still anchors off the unclamped value, so the visual relationship between popover and menu breaks. Also the menu's top (line 129) is computed from a hardcoded 308 (the popover's height), which silently breaks if the popover height ever changes.
Consider deriving the menu position from the popover's actual rect (e.g. measure popoverRef.current?.getBoundingClientRect() once it is mounted) instead of recomputing from constants.
Also applies to: 109-110, 128-131
| <button | ||
| onClick={() => setIsMenuOpen(!isMenuOpen)} | ||
| className="text-[#969593] hover:text-[#CBCACA] transition-colors outline-none" | ||
| > | ||
| <MoreHorizontal className="w-[18px] h-[18px]" /> | ||
| </button> |
There was a problem hiding this comment.
Icon-only "more" button lacks an accessible name and visible focus state.
The trigger only renders a MoreHorizontal glyph and uses outline-none — keyboard and screen reader users will not know what it does and will not see focus on Tab. Add an aria-label and either keep a focus ring or replace outline-none with a visible focus-visible: style.
Suggested change
- <button
- onClick={() => setIsMenuOpen(!isMenuOpen)}
- className="text-[`#969593`] hover:text-[`#CBCACA`] transition-colors outline-none"
- >
+ <button
+ type="button"
+ aria-label="Notification options"
+ aria-haspopup="menu"
+ aria-expanded={isMenuOpen}
+ onClick={() => setIsMenuOpen(!isMenuOpen)}
+ className="text-[`#969593`] hover:text-[`#CBCACA`] transition-colors focus-visible:ring-2 focus-visible:ring-white/20 rounded"
+ >
<MoreHorizontal className="w-[18px] h-[18px]" />
</button>📝 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.
| <button | |
| onClick={() => setIsMenuOpen(!isMenuOpen)} | |
| className="text-[#969593] hover:text-[#CBCACA] transition-colors outline-none" | |
| > | |
| <MoreHorizontal className="w-[18px] h-[18px]" /> | |
| </button> | |
| <button | |
| type="button" | |
| aria-label="Notification options" | |
| aria-haspopup="menu" | |
| aria-expanded={isMenuOpen} | |
| onClick={() => setIsMenuOpen(!isMenuOpen)} | |
| className="text-[`#969593`] hover:text-[`#CBCACA`] transition-colors focus-visible:ring-2 focus-visible:ring-white/20 rounded" | |
| > | |
| <MoreHorizontal className="w-[18px] h-[18px]" /> | |
| </button> |
| <button className="text-[#969593] hover:text-[#CBCACA] transition-colors"> | ||
| <button | ||
| onClick={() => setIsMenuOpen(!isMenuOpen)} | ||
| className="text-[#969593] hover:text-[#CBCACA] transition-colors outline-none" |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Tailwind CSS v4 outline-none vs outline-hidden behavior
💡 Result:
In Tailwind CSS v4: 1) outline-none - Sets outline-style: none (removes the focus outline entirely). [1] 2) outline-hidden - Hides the default browser focus outline, but preserves an outline in Windows/forced-colors mode for accessibility. [1] So the “behavior difference” you’re looking for is primarily about forced-colors/high-contrast mode support: use outline-hidden when you want the outline hidden normally but still visible in forced colors; use outline-none when you truly want it removed. [1] Upgrading note: Tailwind changed the meaning/naming between v3 and v4 because the old outline-none behavior was implemented via an invisible outline for forced-colors accessibility. In v4, that behavior was moved to outline-hidden, and outline-none now simply sets outline-style: none. [2][3]
Citations:
- 1: https://tailwindcss.com/docs/outline-style
- 2: Replace
outline-nonewithoutline-hidden, add newoutline-nonetailwindlabs/tailwindcss#14926 - 3: [v4] :focus-visible no longer overrides default outline for some reason tailwindlabs/tailwindcss#15152
🏁 Script executed:
# Find and read the NotificationsPopover.tsx file
find . -name "NotificationsPopover.tsx" -type fRepository: phasehumans/december
Length of output: 130
🏁 Script executed:
# Check Tailwind version in project configuration
fd -e 'package.json' | head -1 | xargs cat | jq '.devDependencies.tailwindcss, .dependencies.tailwindcss'Repository: phasehumans/december
Length of output: 46
🏁 Script executed:
# Read the specific lines from the file with context
if [ -f "web/src/features/navigation/components/NotificationsPopover.tsx" ]; then
sed -n '110,160p' "web/src/features/navigation/components/NotificationsPopover.tsx" | cat -n
fiRepository: phasehumans/december
Length of output: 3101
🏁 Script executed:
# Read package.json to find Tailwind version
cat package.json | jq '.devDependencies | .tailwindcss, .dependencies | .tailwindcss'Repository: phasehumans/december
Length of output: 74
🏁 Script executed:
# Check the entire NotificationsPopover.tsx file for focus-related styles
cat "web/src/features/navigation/components/NotificationsPopover.tsx" | grep -i "focus"Repository: phasehumans/december
Length of output: 46
🏁 Script executed:
# Search for focus-visible patterns in the codebase to see if there's a pattern
rg "focus-visible" web/src/features/navigation/ -A 2Repository: phasehumans/december
Length of output: 1956
Add focus indicators to match the established codebase pattern.
These buttons have outline-none but lack the focus-visible:ring-2 focus-visible:ring-white/20 styles used throughout the navigation components. Keyboard users need a visible focus indicator. Apply the same focus pattern as SidebarNavItem, SidebarProjectItem, and SidebarSectionHeader.
Also applies to: 139-139, 149-149
| <span className="font-medium text-[13px] transition-colors tracking-tight text-[#8F8E8D] group-hover:text-[#CBCACA] truncate"> | ||
| {/* @ts-ignore */} | ||
| {project.name || project.title} | ||
| </span> |
There was a problem hiding this comment.
@ts-ignore is masking a real type gap on project.
Suppressing the error means the next person who refactors project won't be told this code expects either name or title. Either widen the Project type to include title (or rename the field), or pick the canonical one and convert at the API boundary. As per coding guidelines, "TypeScript must be strict; prefer Zod schemas for API boundaries".
| DecemberLogo: (props: any) => ( | ||
| <svg | ||
| {...props} | ||
| viewBox="5 4 14 16" | ||
| fill="none" | ||
| stroke="currentColor" | ||
| strokeWidth="1" | ||
| strokeLinecap="round" | ||
| strokeLinejoin="round" | ||
| > |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's find and examine the Icons.tsx file
fd -t f "Icons.tsx"Repository: phasehumans/december
Length of output: 103
🏁 Script executed:
# Read the Icons.tsx file to check the line ranges mentioned
cat -n web/src/shared/components/ui/Icons.tsx | head -50Repository: phasehumans/december
Length of output: 2339
🏁 Script executed:
# Check lines 124-135 to see the pattern
cat -n web/src/shared/components/ui/Icons.tsx | sed -n '120,140p'Repository: phasehumans/december
Length of output: 838
🏁 Script executed:
# Check the ProfileSettings.tsx file and line 176
fd -t f "ProfileSettings.tsx" | head -5Repository: phasehumans/december
Length of output: 120
🏁 Script executed:
# Read ProfileSettings.tsx around line 176
cat -n web/src/features/profile/components/ProfileSettings.tsx | sed -n '170,185p'Repository: phasehumans/december
Length of output: 1269
🏁 Script executed:
# Search for DocsBook in Icons.tsx
rg -n "DocsBook" web/src/shared/components/ui/Icons.tsx -A 15Repository: phasehumans/december
Length of output: 640
🏁 Script executed:
# Check a few more of the mentioned line ranges to verify the pattern holds
cat -n web/src/shared/components/ui/Icons.tsx | sed -n '139,160p'Repository: phasehumans/december
Length of output: 886
🏁 Script executed:
# Check lines 288-299 as mentioned
cat -n web/src/shared/components/ui/Icons.tsx | sed -n '285,305p'Repository: phasehumans/december
Length of output: 788
🏁 Script executed:
# Check TypeScript configuration to verify strict mode requirement
fd -t f "tsconfig.json" | head -3Repository: phasehumans/december
Length of output: 117
🏁 Script executed:
# Check the web tsconfig for strict mode
cat web/tsconfig.jsonRepository: phasehumans/december
Length of output: 1007
🏁 Script executed:
# Also check the root tsconfig
cat tsconfig.jsonRepository: phasehumans/december
Length of output: 869
Move the spread to the end of attributes to allow consumer overrides.
Each icon component spreads props before hardcoded attributes like strokeWidth, width, and height. Since JSX attributes are evaluated left-to-right with later ones overriding earlier ones, the hardcoded values always win. This means strokeWidth={1.5} in ProfileSettings.tsx Line 176 is ignored because DocsBook hardcodes strokeWidth="2" after the spread. Reorder to <svg width="18" height="18" strokeWidth="2" {...props}> to let consumers override defaults.
Also applies to: 124-135, 139-150, 155-166, 170-181, 191-202, 206-217, 288-299
Additionally, replace (props: any) with a properly typed SVG prop interface. The repository enforces strict TypeScript across all .ts and .tsx files, and exported UI components should not use any at their boundaries.
| <path d="M53.4,51.4 C58.2,53.0 66.5,56.3 75.3,60.1 C80.6,62.4 84.8,64.3 85.4,64.8 C83.2,70.5 76.5,80.1 69.3,86.6 C64.2,91.3 54.1,95.5 45.4,94.2 C45.3,94.2 44.9,94.1 44.4,94.0 C45.8,80.7 48.0,66.5 53.4,51.4 Z" /> | ||
| <path d="M49.4,51.6 C44.8,64.5 42.1,77.5 40.5,91.9 C33.1,88.7 26.6,80.1 23.3,71.7 C21.4,66.9 22.0,62.8 23.9,59.5 C25.0,57.7 26.6,55.9 28.5,54.2 C35.4,53.9 42.4,53.0 49.4,51.6 Z" /> | ||
| <path d="M50.1,11.1 C52.2,18.0 52.4,31.7 51.5,45.7 C44.8,47.1 38.0,48.0 31.4,48.4 C32.1,43.2 34.0,37.8 37.0,32.3 C40.8,25.6 44.9,16.8 50.1,11.1 Z" /> | ||
| DecemberLogo: (props: any) => ( |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
wc -l web/src/shared/components/ui/Icons.tsxRepository: phasehumans/december
Length of output: 108
🏁 Script executed:
head -50 web/src/shared/components/ui/Icons.tsxRepository: phasehumans/december
Length of output: 1989
🏁 Script executed:
sed -n '120,130p; 135,145p; 150,160p; 165,175p; 185,195p; 200,210p; 285,295p' web/src/shared/components/ui/Icons.tsxRepository: phasehumans/december
Length of output: 2413
🏁 Script executed:
grep -n "type IconProps\|interface.*Props" web/src/shared/components/ui/Icons.tsxRepository: phasehumans/december
Length of output: 46
🏁 Script executed:
sed -n '10p; 124p; 139p; 155p; 170p; 191p; 206p; 288p' web/src/shared/components/ui/Icons.tsxRepository: phasehumans/december
Length of output: 315
🏁 Script executed:
grep -n "props: any" web/src/shared/components/ui/Icons.tsx | head -20Repository: phasehumans/december
Length of output: 761
Replace any props with a proper type across all icon components in this file.
The Icons object has multiple components using any for props, which bypasses TypeScript's type checking at a shared UI boundary. Use type IconProps = React.SVGProps<SVGSVGElement> to keep the API flexible without sacrificing type safety. This applies to all icon components in the file, not just the flagged lines (20+ occurrences exist).
The Zod suggestion in the original guideline applies to API boundaries (external APIs, data integration), not component props—the type alias approach above is the appropriate solution here.
Example refactor
+type IconProps = React.SVGProps<SVGSVGElement>
+
export const Icons = {
- DecemberLogo: (props: any) => (
+ DecemberLogo: (props: IconProps) => (
<svg
{...props}
viewBox="5 4 14 16"📝 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.
| DecemberLogo: (props: any) => ( | |
| type IconProps = React.SVGProps<SVGSVGElement> | |
| export const Icons = { | |
| DecemberLogo: (props: IconProps) => ( | |
| <svg | |
| {...props} | |
| viewBox="5 4 14 16" |
| useEffect(() => { | ||
| const handleClickOutside = (e: MouseEvent) => { | ||
| if (selectorRef.current && !selectorRef.current.contains(e.target as Node)) { | ||
| setIsModelSelectorOpen(false) | ||
| } | ||
| } | ||
| document.addEventListener('mousedown', handleClickOutside) | ||
| return () => document.removeEventListener('mousedown', handleClickOutside) | ||
| }, []) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Outside-click listener is always attached.
handleClickOutside is bound at mount and only unbound on unmount, so it runs on every document mousedown even when the dropdown is closed (most of the time). Gate the effect on isModelSelectorOpen so the listener only exists while the menu is open.
Suggested change
- useEffect(() => {
- const handleClickOutside = (e: MouseEvent) => {
- if (selectorRef.current && !selectorRef.current.contains(e.target as Node)) {
- setIsModelSelectorOpen(false)
- }
- }
- document.addEventListener('mousedown', handleClickOutside)
- return () => document.removeEventListener('mousedown', handleClickOutside)
- }, [])
+ useEffect(() => {
+ if (!isModelSelectorOpen) return
+ const handleClickOutside = (e: MouseEvent) => {
+ if (selectorRef.current && !selectorRef.current.contains(e.target as Node)) {
+ setIsModelSelectorOpen(false)
+ }
+ }
+ document.addEventListener('mousedown', handleClickOutside)
+ return () => document.removeEventListener('mousedown', handleClickOutside)
+ }, [isModelSelectorOpen])📝 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.
| useEffect(() => { | |
| const handleClickOutside = (e: MouseEvent) => { | |
| if (selectorRef.current && !selectorRef.current.contains(e.target as Node)) { | |
| setIsModelSelectorOpen(false) | |
| } | |
| } | |
| document.addEventListener('mousedown', handleClickOutside) | |
| return () => document.removeEventListener('mousedown', handleClickOutside) | |
| }, []) | |
| useEffect(() => { | |
| if (!isModelSelectorOpen) return | |
| const handleClickOutside = (e: MouseEvent) => { | |
| if (selectorRef.current && !selectorRef.current.contains(e.target as Node)) { | |
| setIsModelSelectorOpen(false) | |
| } | |
| } | |
| document.addEventListener('mousedown', handleClickOutside) | |
| return () => document.removeEventListener('mousedown', handleClickOutside) | |
| }, [isModelSelectorOpen]) |
| const models = [ | ||
| { id: 'Auto', name: 'Auto', desc: 'Best model for your task', icon: null }, | ||
| { | ||
| id: 'Claude 4.7 Opus', | ||
| name: 'Claude 4.7 Opus', | ||
| desc: "Anthropic's Most Capable Model", | ||
| icon: Icons.Claude, | ||
| }, | ||
| { | ||
| id: 'Claude 4.6 Sonnet', | ||
| name: 'Claude 4.6 Sonnet', | ||
| desc: "Anthropic's Latest Model", | ||
| icon: Icons.Claude, | ||
| }, | ||
| { id: 'GPT 5.5', name: 'GPT 5.5', desc: "OpenAI's Latest Model", icon: Icons.OpenAI }, | ||
| { | ||
| id: 'GPT 5.4 - 1M', | ||
| name: 'GPT 5.4 - 1M', | ||
| desc: '1 Million Context', | ||
| icon: Icons.OpenAI, | ||
| badge: 'Pro', | ||
| }, | ||
| { | ||
| id: 'Gemini 1.5 Pro', | ||
| name: 'Gemini 1.5 Pro', | ||
| desc: "Google's Latest Model", | ||
| icon: Icons.Gemini, | ||
| }, | ||
| { | ||
| id: 'DeepSeek V3', | ||
| name: 'DeepSeek V3', | ||
| desc: 'Powerful Open Source', | ||
| icon: Icons.Deepseek, | ||
| }, | ||
| ] |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Hoist models out of the render function.
The array is recreated on every render but never depends on props or state. Move it to module scope (or wrap in useMemo) so referential equality is stable and the component body stays lean. Also consider sourcing it from a config/API later — the UI already has desc and badge fields that are declared but unused, so the data shape is drifting from what the dropdown actually displays.
Suggested move
-export const PromptFooter: React.FC<PromptFooterProps> = ({
+const MODELS = [
+ { id: 'Auto', name: 'Auto', desc: 'Best model for your task', icon: null },
+ // ...rest
+] as const
+
+export const PromptFooter: React.FC<PromptFooterProps> = ({| <div className="relative" ref={selectorRef}> | ||
| <button | ||
| onClick={() => setIsModelSelectorOpen(!isModelSelectorOpen)} | ||
| className={cn( | ||
| 'flex items-center gap-2 px-2.5 py-1 rounded-full transition-colors outline-none bg-[#2B2A29]', | ||
| isModelSelectorOpen ? 'text-white' : 'text-[#8F8E8D] hover:text-white' | ||
| )} | ||
| > | ||
| <span className="text-[12px] font-medium">{selectedModelData.name}</span> | ||
| <Icons.ChevronDown | ||
| className={cn( | ||
| 'w-[11px] h-[11px] transition-transform', | ||
| isModelSelectorOpen ? 'rotate-180' : 'rotate-0' | ||
| )} | ||
| /> | ||
| </button> | ||
|
|
||
| {isModelSelectorOpen && ( | ||
| <div className="absolute top-[calc(100%+8px)] left-0 w-[240px] bg-[#1E1D1C] border border-white/[0.08] rounded-xl p-1 shadow-2xl z-50 flex flex-col gap-[2px]"> | ||
| {models.map((model) => { | ||
| const isSelected = selectedModel === model.id | ||
| return ( | ||
| <button | ||
| key={model.id} | ||
| onClick={() => { | ||
| setSelectedModel(model.id) | ||
| setIsModelSelectorOpen(false) | ||
| }} | ||
| className={cn( | ||
| 'flex items-center justify-between px-3 py-1.5 rounded-lg text-left transition-colors outline-none', | ||
| isSelected | ||
| ? 'bg-[#252422] text-[#D6D5D4]' | ||
| : 'text-[#8F8E8D] hover:bg-[#252422] hover:text-[#D6D5D4]' | ||
| )} | ||
| > | ||
| <span className="text-[13px] font-medium truncate"> | ||
| {model.name} | ||
| </span> | ||
| {isSelected && ( | ||
| <Icons.Check className="w-4 h-4 text-[#D6D5D4] shrink-0" /> | ||
| )} | ||
| </button> | ||
| ) | ||
| })} | ||
| </div> | ||
| )} | ||
| </div> |
There was a problem hiding this comment.
Model dropdown is missing combobox/listbox semantics and keyboard support.
The trigger has no aria-haspopup/aria-expanded, the option buttons have no role="option"/listbox container, there is no Up/Down arrow navigation, no Home/End, and Escape does not close the menu. Keyboard and screen-reader users effectively cannot use the picker. Either adopt a headless library (Radix Select, Headless UI Listbox) or add the ARIA roles and key handlers manually.
1 participant
Summary by CodeRabbit
Release Notes
New Features
Improvements
UI Updates