Merged
Conversation
- Added new stories for Select component including WithGroups, WithLabel, SmallSize, DisabledItems, Controlled, Disabled, and Scrollable. - Updated Select component to support grouped items and labels. - Introduced AsyncCombobox component for dynamic searching with simulated API calls. - Created Command component with various subcomponents for command palette functionality. - Added Command stories demonstrating usage of Command, CommandDialog, and CommandGroups. - Improved Popover styling in PopoverContent for better visual consistency.
…ure and accessibility
…igger button handling
…ity and option grouping
…ive stories - Introduced SmartMultiSelect component for multi-selection with async capabilities and custom rendering options. - Added SmartSelect component for single selection with search functionality and grouping options. - Created detailed Storybook stories for both components showcasing various use cases including async search, custom labels, and error handling. - Updated index.ts to export new components and their types.
… functionality and improved styling
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Trigger as Trigger/Button
participant UI as SmartSelect/SmartMultiSelect
participant Cmd as Command (cmdk)
participant Async as Async Search
User->>Trigger: Click / type
Trigger->>UI: open + forward input
UI->>Cmd: render Command input/list
alt async mode
UI->>Async: debounce -> onSearch(query)
Async-->>UI: loading -> results / error
else sync mode
UI-->>UI: filter local options
end
UI->>Cmd: populate items (groups, create, empty)
Cmd-->>User: show list, accept selection/create
User->>Cmd: select item or submit create
Cmd->>UI: selection/create event
UI->>UI: update value(s), optionally select-on-create
UI-->>Trigger: update display, close popover
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 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: 3
🧹 Nitpick comments (2)
src/components/ui/smart-select.tsx (1)
340-342: Avoid unsafe type casting for event handlers.The cast
as unknown as React.MouseEventis a type safety workaround. Consider using a shared handler that works with both event types, or separate handlers.💡 Cleaner approach
+ const handleClearInteraction = (e: React.SyntheticEvent) => { + e.stopPropagation() + if ('preventDefault' in e) e.preventDefault() + onValueChange?.("") + } + <span role="button" tabIndex={-1} className="rounded-sm opacity-50 hover:opacity-100 cursor-pointer" - onClick={handleClear} + onClick={handleClearInteraction} onKeyDown={(e) => { - if (e.key === "Enter" || e.key === " ") handleClear(e as unknown as React.MouseEvent) + if (e.key === "Enter" || e.key === " ") handleClearInteraction(e) }} >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ui/smart-select.tsx` around lines 340 - 342, The onKeyDown handler is using an unsafe cast (as unknown as React.MouseEvent) when calling handleClear; instead update the handlers so types align: either overload or change handleClear's signature to accept both React.MouseEvent and React.KeyboardEvent (e.g., React.SyntheticEvent) or add a dedicated keyboard handler (e.g., handleClearKey) that calls the existing handleClear without casting; update the onKeyDown prop to call the new keyboard-specific handler and keep the click handler using handleClear to preserve correct event types and type safety around the handleClear symbol used in this component.src/components/ui/combobox.tsx (1)
240-257: Simplify the type definition for ComboboxChips.The type intersection
React.ComponentPropsWithRef<typeof ComboboxPrimitive.Chips> & ComboboxPrimitive.Chips.Propsis likely redundant.ComponentPropsWithRefalready extracts props from the component type, so the additional& ComboboxPrimitive.Chips.Propsmay be unnecessary.💡 Suggested simplification
const ComboboxChips = React.forwardRef< HTMLDivElement, - React.ComponentPropsWithRef<typeof ComboboxPrimitive.Chips> & - ComboboxPrimitive.Chips.Props + ComboboxPrimitive.Chips.Props >(({ className, ...props }, ref) => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ui/combobox.tsx` around lines 240 - 257, The prop type for ComboboxChips is overly verbose: remove the redundant intersection and use only React.ComponentPropsWithRef<typeof ComboboxPrimitive.Chips> as the forwardRef generic and parameter type (replace the current "React.ComponentPropsWithRef<typeof ComboboxPrimitive.Chips> & ComboboxPrimitive.Chips.Props" with React.ComponentPropsWithRef<typeof ComboboxPrimitive.Chips>). Update the ComboboxChips forwardRef declaration and its props destructuring accordingly while keeping the component implementation and ComboboxChips.displayName unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/ui/smart-multi-select.tsx`:
- Around line 298-304: The onKeyDown handler attached to the SmartMultiSelect
currently always calls e.stopPropagation(), which blocks Tab navigation; update
the handler (the inline onKeyDown and its use of handleDefaultCreate) to only
call e.stopPropagation() for keys that should be suppressed (e.g., when e.key
=== "Enter" or other non-Tab keys) and let Tab (and any navigation keys)
propagate normally; ensure Enter still calls e.preventDefault() and
handleDefaultCreate() while only stopping propagation for that case so keyboard
navigation is preserved.
In `@src/components/ui/smart-select.tsx`:
- Around line 286-292: The onKeyDown handler currently calls e.stopPropagation()
for all keys which breaks keyboard navigation; update the handler attached to
the SmartSelect component so it only calls e.preventDefault() and
e.stopPropagation() for the specific keys that require interception (e.g.,
"Enter" and optionally "Escape") and leaves other keys (Tab, Arrow keys, etc.)
to propagate normally; locate the onKeyDown handler and adjust the logic around
handleDefaultCreate() to conditionally stop propagation only when e.key ===
"Enter" (and add similar branches for "Escape" if needed).
In `@src/components/ui/SmartMultiSelect.stories.tsx`:
- Around line 491-532: The MultiComplexCreateForm collects a description but
doesn't include it when creating the new option; update the onClick handler that
calls onCreated inside MultiComplexCreateForm to pass the description (e.g.,
include a description property alongside value and label, using
description.trim() or the description state) so the created option matches
ComplexCreateForm's shape; locate the onClick that calls onCreated({ value:
name.toLowerCase(), label: name }) and extend the object to include description.
---
Nitpick comments:
In `@src/components/ui/combobox.tsx`:
- Around line 240-257: The prop type for ComboboxChips is overly verbose: remove
the redundant intersection and use only React.ComponentPropsWithRef<typeof
ComboboxPrimitive.Chips> as the forwardRef generic and parameter type (replace
the current "React.ComponentPropsWithRef<typeof ComboboxPrimitive.Chips> &
ComboboxPrimitive.Chips.Props" with React.ComponentPropsWithRef<typeof
ComboboxPrimitive.Chips>). Update the ComboboxChips forwardRef declaration and
its props destructuring accordingly while keeping the component implementation
and ComboboxChips.displayName unchanged.
In `@src/components/ui/smart-select.tsx`:
- Around line 340-342: The onKeyDown handler is using an unsafe cast (as unknown
as React.MouseEvent) when calling handleClear; instead update the handlers so
types align: either overload or change handleClear's signature to accept both
React.MouseEvent and React.KeyboardEvent (e.g., React.SyntheticEvent) or add a
dedicated keyboard handler (e.g., handleClearKey) that calls the existing
handleClear without casting; update the onKeyDown prop to call the new
keyboard-specific handler and keep the click handler using handleClear to
preserve correct event types and type safety around the handleClear symbol used
in this component.
🪄 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: 49f363bd-a9c0-49e0-96c9-fd9f583c754b
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (12)
package.jsonsrc/components/ui/Combobox.stories.tsxsrc/components/ui/Command.stories.tsxsrc/components/ui/Select.stories.tsxsrc/components/ui/SmartMultiSelect.stories.tsxsrc/components/ui/SmartSelect.stories.tsxsrc/components/ui/combobox.tsxsrc/components/ui/command.tsxsrc/components/ui/index.tssrc/components/ui/popover.tsxsrc/components/ui/smart-multi-select.tsxsrc/components/ui/smart-select.tsx
Comment on lines
+298
to
+304
| onKeyDown={(e) => { | ||
| if (e.key === "Enter") { | ||
| e.preventDefault() | ||
| handleDefaultCreate() | ||
| } | ||
| e.stopPropagation() | ||
| }} |
There was a problem hiding this comment.
Same stopPropagation accessibility concern as SmartSelect.
Blanket e.stopPropagation() prevents Tab navigation. Apply the same fix as suggested for SmartSelect.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/ui/smart-multi-select.tsx` around lines 298 - 304, The
onKeyDown handler attached to the SmartMultiSelect currently always calls
e.stopPropagation(), which blocks Tab navigation; update the handler (the inline
onKeyDown and its use of handleDefaultCreate) to only call e.stopPropagation()
for keys that should be suppressed (e.g., when e.key === "Enter" or other
non-Tab keys) and let Tab (and any navigation keys) propagate normally; ensure
Enter still calls e.preventDefault() and handleDefaultCreate() while only
stopping propagation for that case so keyboard navigation is preserved.
Comment on lines
+286
to
+292
| onKeyDown={(e) => { | ||
| if (e.key === "Enter") { | ||
| e.preventDefault() | ||
| handleDefaultCreate() | ||
| } | ||
| e.stopPropagation() | ||
| }} |
There was a problem hiding this comment.
Reconsider blanket stopPropagation() on keyDown.
Calling e.stopPropagation() for all key events prevents keyboard navigation (like Tab) from working properly, which impacts accessibility. Consider only stopping propagation for specific keys that need it (e.g., Enter, Escape).
🛠️ Suggested fix
onKeyDown={(e) => {
if (e.key === "Enter") {
e.preventDefault()
handleDefaultCreate()
+ e.stopPropagation()
}
- e.stopPropagation()
}}📝 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
| onKeyDown={(e) => { | |
| if (e.key === "Enter") { | |
| e.preventDefault() | |
| handleDefaultCreate() | |
| } | |
| e.stopPropagation() | |
| }} | |
| onKeyDown={(e) => { | |
| if (e.key === "Enter") { | |
| e.preventDefault() | |
| handleDefaultCreate() | |
| e.stopPropagation() | |
| } | |
| }} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/ui/smart-select.tsx` around lines 286 - 292, The onKeyDown
handler currently calls e.stopPropagation() for all keys which breaks keyboard
navigation; update the handler attached to the SmartSelect component so it only
calls e.preventDefault() and e.stopPropagation() for the specific keys that
require interception (e.g., "Enter" and optionally "Escape") and leaves other
keys (Tab, Arrow keys, etc.) to propagate normally; locate the onKeyDown handler
and adjust the logic around handleDefaultCreate() to conditionally stop
propagation only when e.key === "Enter" (and add similar branches for "Escape"
if needed).
Comment on lines
+491
to
+532
| }) { | ||
| const [name, setName] = useState(ctx.searchValue); | ||
| const [description, setDescription] = useState(""); | ||
| return ( | ||
| <div className="p-3 border-t border-border space-y-3"> | ||
| <p className="text-sm font-medium">Create new framework</p> | ||
| <div className="space-y-1.5"> | ||
| <Label className="text-xs">Name</Label> | ||
| <Input | ||
| value={name} | ||
| onChange={(e) => setName(e.target.value)} | ||
| placeholder="Framework name..." | ||
| autoFocus | ||
| onKeyDown={(e) => e.stopPropagation()} | ||
| /> | ||
| </div> | ||
| <div className="space-y-1.5"> | ||
| <Label className="text-xs">Description</Label> | ||
| <Input | ||
| value={description} | ||
| onChange={(e) => setDescription(e.target.value)} | ||
| placeholder="Short description..." | ||
| onKeyDown={(e) => e.stopPropagation()} | ||
| /> | ||
| </div> | ||
| <div className="flex gap-2 justify-end pt-1"> | ||
| <Button variant="outline" size="sm" onClick={() => ctx.clearSearch()}> | ||
| Cancel | ||
| </Button> | ||
| <Button | ||
| size="sm" | ||
| disabled={!name.trim()} | ||
| onClick={() => { | ||
| onCreated({ value: name.toLowerCase(), label: name }); | ||
| ctx.clearSearch(); | ||
| }} | ||
| > | ||
| Create | ||
| </Button> | ||
| </div> | ||
| </div> | ||
| ); |
There was a problem hiding this comment.
Description field value is collected but not used in created option.
The MultiComplexCreateForm captures a description state (line 493) and renders an input for it (lines 508-514), but the created option at lines 523-524 doesn't include the description:
onCreated({ value: name.toLowerCase(), label: name });This is inconsistent with ComplexCreateForm in SmartSelect.stories.tsx which correctly includes description in the created option.
🔧 Proposed fix
onClick={() => {
- onCreated({ value: name.toLowerCase(), label: name });
+ onCreated({
+ value: name.toLowerCase(),
+ label: name,
+ description: description || undefined,
+ });
ctx.clearSearch();
}}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/ui/SmartMultiSelect.stories.tsx` around lines 491 - 532, The
MultiComplexCreateForm collects a description but doesn't include it when
creating the new option; update the onClick handler that calls onCreated inside
MultiComplexCreateForm to pass the description (e.g., include a description
property alongside value and label, using description.trim() or the description
state) so the created option matches ComplexCreateForm's shape; locate the
onClick that calls onCreated({ value: name.toLowerCase(), label: name }) and
extend the object to include description.
…lect and SmartSelect components
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/components/ui/SmartMultiSelect.stories.tsx (1)
434-435: SamestopPropagation()accessibility issue in story demo.The
onKeyDown={(e) => e.stopPropagation()}on line 434 unconditionally blocks all key events including Tab. For consistency with the fix needed in the component, only stop propagation for keys that need it (e.g., Enter, Escape).🔧 Suggested fix
- onKeyDown={(e) => e.stopPropagation()} + onKeyDown={(e) => { + if (e.key === "Escape") e.stopPropagation(); + }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ui/SmartMultiSelect.stories.tsx` around lines 434 - 435, The story demo currently uses onKeyDown={(e) => e.stopPropagation()} which blocks all keyboard events (including Tab); update the onKeyDown handler in the SmartMultiSelect story so it only calls e.stopPropagation() for specific keys (e.g., when e.key is "Enter" or "Escape") and otherwise lets events propagate (so Tab and other navigation keys still work); locate the onKeyDown prop in SmartMultiSelect.stories.tsx and replace the unconditional stopPropagation with a conditional check for e.key values.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/ui/smart-select.tsx`:
- Around line 289-293: The input for the create form (the JSX <input> that uses
value={search} and onChange={(e) => setSearch(e.target.value)}) is missing an
accessible name; add an aria-label attribute to that input (e.g.,
aria-label="Search or create name" or similar descriptive text) so screen
readers can convey the field's purpose while keeping the existing value/search
and setSearch handlers intact.
- Around line 379-391: The create-toggle Button rendered when
hasCreateCapability is missing an accessible label; update the Button in the
SmartSelect component (the element using hasCreateCapability, setShowCreate,
showCreate and toggling between XIcon and PlusIcon) to include an appropriate
aria-label (e.g. "Close create form" when showCreate is true and "Open create
form" when false) so screen readers can convey the button's purpose; ensure the
aria-label updates based on showCreate to reflect the current action.
- Around line 347-358: The clear control is not keyboard focusable because it
uses span with tabIndex={-1} and an unsafe cast when calling handleClear;
replace the span with a semantic <button> (or set tabIndex={0}) so it is
reachable and operable via keyboard, remove role="button" and the onKeyDown type
coercion, and wire onClick to handleClear directly; also update handleClear's
signature if needed to accept the correct event type (e.g., React.MouseEvent |
React.KeyboardEvent) so no casts are required — reference the clear element
using the existing handleClear and XIcon identifiers and the surrounding
className attributes.
---
Nitpick comments:
In `@src/components/ui/SmartMultiSelect.stories.tsx`:
- Around line 434-435: The story demo currently uses onKeyDown={(e) =>
e.stopPropagation()} which blocks all keyboard events (including Tab); update
the onKeyDown handler in the SmartMultiSelect story so it only calls
e.stopPropagation() for specific keys (e.g., when e.key is "Enter" or "Escape")
and otherwise lets events propagate (so Tab and other navigation keys still
work); locate the onKeyDown prop in SmartMultiSelect.stories.tsx and replace the
unconditional stopPropagation with a conditional check for e.key values.
🪄 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: 97c133ed-0301-48e2-84de-e98b8fa099f0
📒 Files selected for processing (4)
src/components/ui/SmartMultiSelect.stories.tsxsrc/components/ui/SmartSelect.stories.tsxsrc/components/ui/smart-multi-select.tsxsrc/components/ui/smart-select.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/ui/SmartSelect.stories.tsx
Comment on lines
+289
to
+293
| <input | ||
| className="flex h-8 w-full rounded-md border border-input bg-transparent px-2 text-sm outline-none placeholder:text-muted-foreground focus-visible:border-ring focus-visible:ring-ring/50 focus-visible:ring-[3px]" | ||
| value={search} | ||
| onChange={(e) => setSearch(e.target.value)} | ||
| placeholder="Enter name..." |
There was a problem hiding this comment.
Add accessible label to the create form input.
The input lacks an accessible name for screen readers. Add an aria-label to convey its purpose.
🛠️ Suggested fix
<input
className="flex h-8 w-full rounded-md border border-input bg-transparent px-2 text-sm outline-none placeholder:text-muted-foreground focus-visible:border-ring focus-visible:ring-ring/50 focus-visible:ring-[3px]"
value={search}
onChange={(e) => setSearch(e.target.value)}
placeholder="Enter name..."
+ aria-label="New item name"
autoFocus📝 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
| <input | |
| className="flex h-8 w-full rounded-md border border-input bg-transparent px-2 text-sm outline-none placeholder:text-muted-foreground focus-visible:border-ring focus-visible:ring-ring/50 focus-visible:ring-[3px]" | |
| value={search} | |
| onChange={(e) => setSearch(e.target.value)} | |
| placeholder="Enter name..." | |
| <input | |
| className="flex h-8 w-full rounded-md border border-input bg-transparent px-2 text-sm outline-none placeholder:text-muted-foreground focus-visible:border-ring focus-visible:ring-ring/50 focus-visible:ring-[3px]" | |
| value={search} | |
| onChange={(e) => setSearch(e.target.value)} | |
| placeholder="Enter name..." | |
| aria-label="New item name" | |
| autoFocus |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/ui/smart-select.tsx` around lines 289 - 293, The input for the
create form (the JSX <input> that uses value={search} and onChange={(e) =>
setSearch(e.target.value)}) is missing an accessible name; add an aria-label
attribute to that input (e.g., aria-label="Search or create name" or similar
descriptive text) so screen readers can convey the field's purpose while keeping
the existing value/search and setSearch handlers intact.
Comment on lines
+347
to
+358
| <span | ||
| role="button" | ||
| tabIndex={-1} | ||
| className="rounded-sm opacity-50 hover:opacity-100 cursor-pointer" | ||
| onClick={handleClear} | ||
| onKeyDown={(e) => { | ||
| if (e.key === "Enter" || e.key === " ") handleClear(e as unknown as React.MouseEvent) | ||
| }} | ||
| > | ||
| <XIcon className="size-3.5" /> | ||
| <span className="sr-only">Clear</span> | ||
| </span> |
There was a problem hiding this comment.
Clear button is not keyboard accessible.
Using tabIndex={-1} with role="button" makes the clear button unreachable via keyboard navigation while still presenting it as an interactive element to assistive technologies. Additionally, the type coercion on line 353 is unsafe.
Consider using a proper <button> element or setting tabIndex={0}.
🛠️ Suggested fix
{showClear && value && (
- <span
- role="button"
- tabIndex={-1}
+ <button
+ type="button"
+ tabIndex={0}
className="rounded-sm opacity-50 hover:opacity-100 cursor-pointer"
onClick={handleClear}
- onKeyDown={(e) => {
- if (e.key === "Enter" || e.key === " ") handleClear(e as unknown as React.MouseEvent)
- }}
>
<XIcon className="size-3.5" />
<span className="sr-only">Clear</span>
- </span>
+ </button>
)}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/ui/smart-select.tsx` around lines 347 - 358, The clear control
is not keyboard focusable because it uses span with tabIndex={-1} and an unsafe
cast when calling handleClear; replace the span with a semantic <button> (or set
tabIndex={0}) so it is reachable and operable via keyboard, remove role="button"
and the onKeyDown type coercion, and wire onClick to handleClear directly; also
update handleClear's signature if needed to accept the correct event type (e.g.,
React.MouseEvent | React.KeyboardEvent) so no casts are required — reference the
clear element using the existing handleClear and XIcon identifiers and the
surrounding className attributes.
Comment on lines
+379
to
+391
| {hasCreateCapability && ( | ||
| <Button | ||
| variant="ghost" | ||
| size="icon-sm" | ||
| className="me-2 shrink-0" | ||
| onClick={() => setShowCreate((prev) => !prev)} | ||
| > | ||
| {showCreate ? ( | ||
| <XIcon className="size-4" /> | ||
| ) : ( | ||
| <PlusIcon className="size-4" /> | ||
| )} | ||
| </Button> |
There was a problem hiding this comment.
Add aria-label to the create toggle button.
The button toggles the create form but has no accessible label. Screen reader users won't understand its purpose.
🛠️ Suggested fix
<Button
variant="ghost"
size="icon-sm"
className="me-2 shrink-0"
onClick={() => setShowCreate((prev) => !prev)}
+ aria-label={showCreate ? "Cancel create" : "Create new item"}
+ aria-expanded={showCreate}
>📝 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
| {hasCreateCapability && ( | |
| <Button | |
| variant="ghost" | |
| size="icon-sm" | |
| className="me-2 shrink-0" | |
| onClick={() => setShowCreate((prev) => !prev)} | |
| > | |
| {showCreate ? ( | |
| <XIcon className="size-4" /> | |
| ) : ( | |
| <PlusIcon className="size-4" /> | |
| )} | |
| </Button> | |
| {hasCreateCapability && ( | |
| <Button | |
| variant="ghost" | |
| size="icon-sm" | |
| className="me-2 shrink-0" | |
| onClick={() => setShowCreate((prev) => !prev)} | |
| aria-label={showCreate ? "Cancel create" : "Create new item"} | |
| aria-expanded={showCreate} | |
| > | |
| {showCreate ? ( | |
| <XIcon className="size-4" /> | |
| ) : ( | |
| <PlusIcon className="size-4" /> | |
| )} | |
| </Button> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/ui/smart-select.tsx` around lines 379 - 391, The create-toggle
Button rendered when hasCreateCapability is missing an accessible label; update
the Button in the SmartSelect component (the element using hasCreateCapability,
setShowCreate, showCreate and toggling between XIcon and PlusIcon) to include an
appropriate aria-label (e.g. "Close create form" when showCreate is true and
"Open create form" when false) so screen readers can convey the button's
purpose; ensure the aria-label updates based on showCreate to reflect the
current action.
…o Storybook story exports
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
.storybook/babel-plugin-story-source.js (1)
36-44: Support wrapper components declared asconstarrow/function expressions.Line 36 only captures
function DemoWrapper() {}. Wrappers likeconst DemoWrapper = () => (...)are ignored, so no source gets injected for those stories.Suggested diff
if (stmt.isFunctionDeclaration()) { const name = stmt.node.id?.name; if (!name) continue; const { start, end } = stmt.node; if (start == null || end == null) continue; wrapperFunctions.set(name, fileSource.slice(start, end)); } + + if (stmt.isVariableDeclaration()) { + for (const d of stmt.get('declarations')) { + if (!d.isVariableDeclarator()) continue; + const id = d.get('id'); + const init = d.get('init'); + if (!id.isIdentifier()) continue; + if (!init.isArrowFunctionExpression() && !init.isFunctionExpression()) continue; + const { start, end } = d.node; + if (start == null || end == null) continue; + wrapperFunctions.set(id.node.name, fileSource.slice(start, end)); + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.storybook/babel-plugin-story-source.js around lines 36 - 44, The current logic only handles function declarations via stmt.isFunctionDeclaration() and misses wrappers declared as const/let/var with arrow or function expressions; update the block that builds wrapperFunctions to also handle stmt.isVariableDeclaration(): iterate stmt.node.declarations, for each VariableDeclarator check declarator.id?.name and that declarator.init is an ArrowFunctionExpression or FunctionExpression, then compute appropriate start/end (use declarator.init.start/end if available, falling back to declarator.start/end) and call wrapperFunctions.set(name, fileSource.slice(start, end)); keep the existing function-declaration branch (stmt.isFunctionDeclaration()) intact and add this variable-declaration handling alongside it..storybook/main.ts (1)
91-105: Broaden the story pre-loader match to align with discovered story extensions.At Line 14, Storybook includes
.stories.jsand.stories.ts, but Line 104 only pre-processes.stories.jsx|.stories.tsx. That leaves part of your stories without source injection/transforms.Suggested diff
- config.module?.rules?.unshift({ - test: /\.stories\.(jsx|tsx)$/, + config.module?.rules?.unshift({ + test: /\.stories\.(js|jsx|ts|tsx)$/, use: [babelLoaderForStories], enforce: "pre" as const, });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.storybook/main.ts around lines 91 - 105, The test for the Storybook pre-loader only matches .stories.jsx and .stories.tsx, so update the rule added via config.module?.rules?.unshift that uses babelLoaderForStories to match the full set of discovered story extensions (at least .stories.js, .stories.ts, .stories.jsx, .stories.tsx); modify the test regex in that rule (the value used in the config.module?.rules?.unshift call) to include js and ts as well so all story files receive the babel-plugin-story-source pre-processing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.storybook/babel-plugin-story-source.js:
- Around line 93-125: The code currently bails out when a story already has a
parameters property (hasParams) — instead, locate the existing parameters object
in init.node.properties (the objectProperty with key 'parameters'), and
merge/augment it: ensure it has a docs objectProperty and within that a source
objectProperty, then set/replace the source's code and language using the source
string from wrapperFunctions.get(wrapperName). Build the sourceObj (code +
language) and docsObj as needed but when parameters exists, update its
properties rather than returning early so you only add/overwrite
parameters.docs.source while preserving any other existing parameter keys.
---
Nitpick comments:
In @.storybook/babel-plugin-story-source.js:
- Around line 36-44: The current logic only handles function declarations via
stmt.isFunctionDeclaration() and misses wrappers declared as const/let/var with
arrow or function expressions; update the block that builds wrapperFunctions to
also handle stmt.isVariableDeclaration(): iterate stmt.node.declarations, for
each VariableDeclarator check declarator.id?.name and that declarator.init is an
ArrowFunctionExpression or FunctionExpression, then compute appropriate
start/end (use declarator.init.start/end if available, falling back to
declarator.start/end) and call wrapperFunctions.set(name,
fileSource.slice(start, end)); keep the existing function-declaration branch
(stmt.isFunctionDeclaration()) intact and add this variable-declaration handling
alongside it.
In @.storybook/main.ts:
- Around line 91-105: The test for the Storybook pre-loader only matches
.stories.jsx and .stories.tsx, so update the rule added via
config.module?.rules?.unshift that uses babelLoaderForStories to match the full
set of discovered story extensions (at least .stories.js, .stories.ts,
.stories.jsx, .stories.tsx); modify the test regex in that rule (the value used
in the config.module?.rules?.unshift call) to include js and ts as well so all
story files receive the babel-plugin-story-source pre-processing.
🪄 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: 55987055-8fe7-45d1-86d2-065b593f97c3
📒 Files selected for processing (2)
.storybook/babel-plugin-story-source.js.storybook/main.ts
Comment on lines
+93
to
+125
| // Skip stories that already define their own `parameters` | ||
| const hasParams = properties.some( | ||
| (p) => | ||
| p.isObjectProperty() && | ||
| p.get('key').isIdentifier({ name: 'parameters' }) | ||
| ); | ||
| if (hasParams) continue; | ||
|
|
||
| // Build: parameters: { docs: { source: { code, language } } } | ||
| const sourceCode = wrapperFunctions.get(wrapperName); | ||
|
|
||
| const sourceObj = t.objectExpression([ | ||
| t.objectProperty( | ||
| t.identifier('code'), | ||
| t.stringLiteral(sourceCode) | ||
| ), | ||
| t.objectProperty( | ||
| t.identifier('language'), | ||
| t.stringLiteral('tsx') | ||
| ), | ||
| ]); | ||
|
|
||
| const docsObj = t.objectExpression([ | ||
| t.objectProperty(t.identifier('source'), sourceObj), | ||
| ]); | ||
|
|
||
| const paramsObj = t.objectExpression([ | ||
| t.objectProperty(t.identifier('docs'), docsObj), | ||
| ]); | ||
|
|
||
| init.node.properties.push( | ||
| t.objectProperty(t.identifier('parameters'), paramsObj) | ||
| ); |
There was a problem hiding this comment.
Don’t bail out when parameters already exists; merge docs.source instead.
At Line 99, the plugin exits if parameters exists, so many valid stories won’t get injected source at all. This undermines the plugin’s core behavior.
Suggested diff
- const hasParams = properties.some(
- (p) =>
- p.isObjectProperty() &&
- p.get('key').isIdentifier({ name: 'parameters' })
- );
- if (hasParams) continue;
+ const paramsProp = properties.find(
+ (p) =>
+ p.isObjectProperty() &&
+ p.get('key').isIdentifier({ name: 'parameters' })
+ );
...
- const paramsObj = t.objectExpression([
- t.objectProperty(t.identifier('docs'), docsObj),
- ]);
-
- init.node.properties.push(
- t.objectProperty(t.identifier('parameters'), paramsObj)
- );
+ if (!paramsProp) {
+ const paramsObj = t.objectExpression([
+ t.objectProperty(t.identifier('docs'), docsObj),
+ ]);
+ init.node.properties.push(
+ t.objectProperty(t.identifier('parameters'), paramsObj)
+ );
+ continue;
+ }
+
+ const paramsVal = paramsProp.get('value');
+ if (!paramsVal.isObjectExpression()) continue;
+
+ const hasDocs = paramsVal.get('properties').some(
+ (p) =>
+ p.isObjectProperty() &&
+ p.get('key').isIdentifier({ name: 'docs' })
+ );
+ if (!hasDocs) {
+ paramsVal.node.properties.push(
+ t.objectProperty(t.identifier('docs'), docsObj)
+ );
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.storybook/babel-plugin-story-source.js around lines 93 - 125, The code
currently bails out when a story already has a parameters property (hasParams) —
instead, locate the existing parameters object in init.node.properties (the
objectProperty with key 'parameters'), and merge/augment it: ensure it has a
docs objectProperty and within that a source objectProperty, then set/replace
the source's code and language using the source string from
wrapperFunctions.get(wrapperName). Build the sourceObj (code + language) and
docsObj as needed but when parameters exists, update its properties rather than
returning early so you only add/overwrite parameters.docs.source while
preserving any other existing parameter keys.
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 by CodeRabbit
New Features
Bug Fixes
Documentation