Open
Conversation
Contributor
WalkthroughAdds a new Select component: a client-side React implementation built on Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📝 Coding Plan
Comment |
|
The latest updates on your projects. Learn more about Argos notifications ↗︎
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/eclipse/content/design-system/components/select.mdx`:
- Around line 665-678: The controlled example uses useState but doesn't import
it; add an import for useState from React at the top of the snippet (so the
example that declares const [value, setValue] = useState("option1"); works).
Locate the example using Select, SelectTrigger, SelectValue, SelectContent, and
SelectItem and add the missing import (e.g., import { useState } from 'react')
so copy-paste consumers won't get a ReferenceError.
- Around line 442-447: The docs list for the Select props is out of sync with
the component defaults: update the MDX documentation entries for the Select
props so that `align` default is "start" and `alignItemWithTrigger` default is
false to match the implementation in select.tsx (the component's `align =
"start"` and `alignItemWithTrigger = false` defaults); locate the `align` and
`alignItemWithTrigger` lines in the select.mdx props list and change their
described defaults accordingly while leaving the other prop descriptions
unchanged.
In `@packages/eclipse/src/components/select.tsx`:
- Line 60: The decorative <i> icons in the select component are currently
exposed to assistive tech; update each decorative icon element (e.g., the <i>
with className "fa-regular fa-chevron-down text-foreground-neutral-weak size-4
pointer-events-none" and the other similar <i> occurrences) to include
aria-hidden="true" so screen readers ignore them; locate these <i> elements in
the select component (packages/eclipse/src/components/select.tsx) and add
aria-hidden="true" to each decorative icon instance.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: cd983f93-1e50-4456-89e5-b8f9b0fee68d
📒 Files selected for processing (4)
apps/eclipse/content/design-system/components/meta.jsonapps/eclipse/content/design-system/components/select.mdxpackages/eclipse/src/components/index.tspackages/eclipse/src/components/select.tsx
Comment on lines
+442
to
+447
| - `side` - Position relative to trigger ("top" | "bottom" | "left" | "right", default: "bottom") | ||
| - `align` - Alignment relative to trigger ("start" | "center" | "end", default: "center") | ||
| - `sideOffset` - Distance from trigger in pixels (number, default: 4) | ||
| - `alignOffset` - Alignment offset in pixels (number, default: 0) | ||
| - `alignItemWithTrigger` - Align selected item with trigger (boolean, default: true) | ||
| - `className` - Additional CSS classes (string or function, optional) |
Contributor
There was a problem hiding this comment.
Align documented defaults with implementation.
Line [443] and Line [446] currently conflict with the component defaults in packages/eclipse/src/components/select.tsx (align = "start", alignItemWithTrigger = false).
Suggested patch
-- `align` - Alignment relative to trigger ("start" | "center" | "end", default: "center")
+- `align` - Alignment relative to trigger ("start" | "center" | "end", default: "start")
...
-- `alignItemWithTrigger` - Align selected item with trigger (boolean, default: true)
+- `alignItemWithTrigger` - Align selected item with trigger (boolean, default: false)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/eclipse/content/design-system/components/select.mdx` around lines 442 -
447, The docs list for the Select props is out of sync with the component
defaults: update the MDX documentation entries for the Select props so that
`align` default is "start" and `alignItemWithTrigger` default is false to match
the implementation in select.tsx (the component's `align = "start"` and
`alignItemWithTrigger = false` defaults); locate the `align` and
`alignItemWithTrigger` lines in the select.mdx props list and change their
described defaults accordingly while leaving the other prop descriptions
unchanged.
Comment on lines
+665
to
+678
| **Controlled (with value and onValueChange):** | ||
| ```tsx | ||
| const [value, setValue] = useState("option1"); | ||
|
|
||
| <Select value={value} onValueChange={setValue}> | ||
| <SelectTrigger> | ||
| <SelectValue /> | ||
| </SelectTrigger> | ||
| <SelectContent> | ||
| <SelectItem value="option1">Option 1</SelectItem> | ||
| <SelectItem value="option2">Option 2</SelectItem> | ||
| </SelectContent> | ||
| </Select> | ||
| ``` |
Contributor
There was a problem hiding this comment.
Controlled example is missing useState import.
Line [667] uses useState but the snippet doesn’t import it, so copy-paste users will hit an immediate error.
Suggested patch
+import { useState } from "react";
+
const [value, setValue] = useState("option1");📝 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
| **Controlled (with value and onValueChange):** | |
| ```tsx | |
| const [value, setValue] = useState("option1"); | |
| <Select value={value} onValueChange={setValue}> | |
| <SelectTrigger> | |
| <SelectValue /> | |
| </SelectTrigger> | |
| <SelectContent> | |
| <SelectItem value="option1">Option 1</SelectItem> | |
| <SelectItem value="option2">Option 2</SelectItem> | |
| </SelectContent> | |
| </Select> | |
| ``` | |
| import { useState } from "react"; | |
| const [value, setValue] = useState("option1"); | |
| <Select value={value} onValueChange={setValue}> | |
| <SelectTrigger> | |
| <SelectValue /> | |
| </SelectTrigger> | |
| <SelectContent> | |
| <SelectItem value="option1">Option 1</SelectItem> | |
| <SelectItem value="option2">Option 2</SelectItem> | |
| </SelectContent> | |
| </Select> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/eclipse/content/design-system/components/select.mdx` around lines 665 -
678, The controlled example uses useState but doesn't import it; add an import
for useState from React at the top of the snippet (so the example that declares
const [value, setValue] = useState("option1"); works). Locate the example using
Select, SelectTrigger, SelectValue, SelectContent, and SelectItem and add the
missing import (e.g., import { useState } from 'react') so copy-paste consumers
won't get a ReferenceError.
| {...props} | ||
| > | ||
| {children} | ||
| <i className="fa-regular fa-chevron-down text-foreground-neutral-weak size-4 pointer-events-none" /> |
Contributor
There was a problem hiding this comment.
Hide decorative icons from assistive technologies.
These <i> elements are visual-only. Add aria-hidden="true" so screen readers don’t announce them.
Suggested patch
- <i className="fa-regular fa-chevron-down text-foreground-neutral-weak size-4 pointer-events-none" />
+ <i
+ aria-hidden="true"
+ className="fa-regular fa-chevron-down text-foreground-neutral-weak size-4 pointer-events-none"
+ />
- <i className="fa-regular fa-check pointer-events-none" />
+ <i aria-hidden="true" className="fa-regular fa-check pointer-events-none" />
- <i className="fa-regular fa-chevron-up" />
+ <i aria-hidden="true" className="fa-regular fa-chevron-up" />
- <i className="fa-regular fa-chevron-down" />
+ <i aria-hidden="true" className="fa-regular fa-chevron-down" />Also applies to: 143-143, 180-180, 198-198
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/eclipse/src/components/select.tsx` at line 60, The decorative <i>
icons in the select component are currently exposed to assistive tech; update
each decorative icon element (e.g., the <i> with className "fa-regular
fa-chevron-down text-foreground-neutral-weak size-4 pointer-events-none" and the
other similar <i> occurrences) to include aria-hidden="true" so screen readers
ignore them; locate these <i> elements in the select component
(packages/eclipse/src/components/select.tsx) and add aria-hidden="true" to each
decorative icon instance.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
apps/eclipse/content/design-system/components/select.mdx (2)
665-678:⚠️ Potential issue | 🟡 MinorAdd the missing
useStateimport in the controlled snippet.Line [667] uses
useStatewithout importing it, so the snippet is not copy-paste runnable.Suggested patch
**Controlled (with value and onValueChange):** ```tsx +import { useState } from "react"; + const [value, setValue] = useState("option1");#!/bin/bash # Verify whether the controlled example includes a useState import near its usage. awk 'NR>=662 && NR<=676 {print NR ":" $0}' apps/eclipse/content/design-system/components/select.mdxExpected result: an
import { useState } from "react";line appears beforeconst [value, setValue] = useState("option1");.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/eclipse/content/design-system/components/select.mdx` around lines 665 - 678, The controlled example uses useState but doesn't import it; add an import statement importing useState from React (import { useState } from "react";) before the line that declares const [value, setValue] = useState("option1"); so the Select example (Select, SelectTrigger, SelectValue, SelectContent, SelectItem) is copy-paste runnable.
443-446:⚠️ Potential issue | 🟡 MinorCorrect the documented
SelectContentdefaults.Line [443] and Line [446] still document defaults that conflict with the component defaults, which can mislead consumers.
Suggested patch
- - `align` - Alignment relative to trigger ("start" | "center" | "end", default: "center") + - `align` - Alignment relative to trigger ("start" | "center" | "end", default: "start") ... - - `alignItemWithTrigger` - Align selected item with trigger (boolean, default: true) + - `alignItemWithTrigger` - Align selected item with trigger (boolean, default: false)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/eclipse/content/design-system/components/select.mdx` around lines 443 - 446, Update the SelectContent documentation so the prop default values match the actual component implementation: in the SelectContent docs (props: align, sideOffset, alignOffset, alignItemWithTrigger) change the documented defaults for `align` and `alignItemWithTrigger` to exactly match the defaults used by the SelectContent component (use the values from the SelectContent implementation/props). Ensure the docs list the same default strings/booleans as the component (refer to SelectContent's prop defaults for the correct values).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@apps/eclipse/content/design-system/components/select.mdx`:
- Around line 665-678: The controlled example uses useState but doesn't import
it; add an import statement importing useState from React (import { useState }
from "react";) before the line that declares const [value, setValue] =
useState("option1"); so the Select example (Select, SelectTrigger, SelectValue,
SelectContent, SelectItem) is copy-paste runnable.
- Around line 443-446: Update the SelectContent documentation so the prop
default values match the actual component implementation: in the SelectContent
docs (props: align, sideOffset, alignOffset, alignItemWithTrigger) change the
documented defaults for `align` and `alignItemWithTrigger` to exactly match the
defaults used by the SelectContent component (use the values from the
SelectContent implementation/props). Ensure the docs list the same default
strings/booleans as the component (refer to SelectContent's prop defaults for
the correct values).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a7c82757-9b4a-40ab-b509-343a3b0eb6d9
📒 Files selected for processing (1)
apps/eclipse/content/design-system/components/select.mdx
aidankmcalister
approved these changes
Mar 19, 2026
Contributor
|
Looks good, do you think we could add a subtil hover effect when hovering the select component ? |
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
3 participants
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.
Fixes #DR-7295
Summary by CodeRabbit
New Features
Documentation
Chores