ref(cmdk): Merge CMDKGroup and CMDKAction into single CMDKAction component#112563
ref(cmdk): Merge CMDKGroup and CMDKAction into single CMDKAction component#112563JonasBa merged 1 commit intojb/cmdk/jsx-pocfrom
Conversation
…onent A node becomes a group by virtue of having children registered under it, so the Group/Action split was an artificial distinction. A single CMDKAction now covers all cases: navigation (to), callbacks (onAction), async resource groups (resource + render-prop children), and plain parent groups (children only). Co-Authored-By: Claude Sonnet 4 <noreply@anthropic.com>
| const ref = CommandPaletteSlot.useSlotOutletRef(); | ||
| const key = CMDKCollection.useRegisterNode({display, keywords, resource, ref}); | ||
|
|
||
| const nodeData: CMDKActionData = | ||
| to === undefined | ||
| ? onAction === undefined | ||
| ? {display, keywords, ref, resource} | ||
| : {display, keywords, ref, onAction} |
There was a problem hiding this comment.
Bug: The CMDKAction component silently ignores the onAction prop if the to prop is also provided, due to a loss of type-level mutual exclusivity.
Severity: MEDIUM
Suggested Fix
Restore the type-level safety by using a discriminated union for the CMDKActionProps type. This will enforce that either to or onAction can be provided, but not both, preventing this issue at compile time. This was the behavior in the previous version of the component.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: static/app/components/commandPalette/ui/cmdk.tsx#L88-L94
Potential issue: The `CMDKAction` component's props interface was changed from a
discriminated union to a single interface with optional `to` and `onAction` props. This
removes the compile-time guarantee that only one can be provided. The component's
implementation prioritizes the `to` prop for navigation. If a developer provides both
`to` and `onAction` on the same component, the `onAction` callback will be silently
ignored, and only the navigation will occur. This is a type safety regression that could
lead to unexpected behavior where a developer's intended action does not execute.
Did we get this right? 👍 / 👎 to inform future reviews.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit c85059a. Configure here.
|
|
||
| /** | ||
| * Registers a leaf action node in the collection. | ||
| */ |
There was a problem hiding this comment.
Leaf actions unnecessarily subscribe to query state
Medium Severity
The merged CMDKAction unconditionally calls useCommandPaletteState() and useQuery() for every instance, including leaf actions that have no resource and no children. Previously, leaf CMDKAction was lightweight (register + return null). Now every leaf subscribes to the query context, causing all leaf actions to re-render on every keystroke in the palette. With dozens of static leaves plus dynamic ones (projects, settings), this is a meaningful regression in a typing-heavy interaction.
Reviewed by Cursor Bugbot for commit c85059a. Configure here.


Changes
Merges
CMDKGroupandCMDKActioninto a singleCMDKActioncomponent. The distinction between the two was artificial — a node becomes a group simply by having children registered under it, which is already documented in theCMDKActionDatatype comment. Keeping them separate contradicted that stated design intent.The merged
CMDKActioncovers all four cases:<CMDKAction display={...} to="/path/" /><CMDKAction display={...} onAction={fn} /><CMDKAction display={...}><CMDKAction .../></CMDKAction><CMDKAction display={...} resource={queryFn}>{data => ...}</CMDKAction>CMDKActionDataTo.tois widened fromstringtoLocationDescriptorto matchCommandPaletteAsyncResultand the existing callsites.