Repository navigation
fix(apollo-vertex): hide command separator from listbox semantics - #1390
ruudandriessen wants to merge 1 commit into
Conversation
cmdk's Command.Separator hardcodes role="separator" after spreading props, so it cannot be overridden. Inside the role="listbox" list this fails axe aria-required-children. cmdk 1.1.1 is the latest release and the issue is still open upstream (dip/cmdk#179); shadcn new-york-v4 uses the same primitive. CommandSeparator now renders its own div with role="none" and reproduces cmdk's visibility logic (hidden while searching unless alwaysRender) via useCommandState. The asChild prop from cmdk's primitive is no longer accepted. Vitest now resolves tsconfig paths so registry components that import other registry components can be tested.
Dependency License Review
License distribution
Excluded packages
|
There was a problem hiding this comment.
🟡 Changes recommended
Removing the public asChild prop breaks compatibility for existing consumers.
1 open finding
What changed in this PR
Fixes Apollo Vertex command-list accessibility by removing separator semantics from the listbox.
Changes:
- Replaces cmdk’s separator with a presentation-only element while preserving search visibility.
- Adds separator regression tests.
- Enables TypeScript path resolution in Vitest.
| File | Description |
|---|---|
| packages/apollo-vertex/vitest.config.ts | Enables TypeScript path resolution for tests. |
| packages/apollo-vertex/registry/command/command.tsx | Updates separator semantics and visibility handling. |
| packages/apollo-vertex/registry/command/command.test.tsx | Tests separator roles and search visibility. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| interface CommandSeparatorProps extends React.ComponentProps<"div"> { | ||
| alwaysRender?: boolean; | ||
| } |
📊 Coverage + size by packagePer-package coverage and bundle size on this PR. New-line coverage = of the source lines this PR adds or changes, the % hit by tests.
"Coverage" is each package's own |
Storybook visual diff✅ No stories are affected by this PR's changes; nothing to compare. Logs Updated (PT): Oct 16, 2026, 04:52:18 AM |
|
Closing: we want to keep the shadcn command component as upstream ships it. The separator role issue is in cmdk (dip/cmdk#179) and will be handled as a lintapple exception instead. |
Summary
axe
aria-required-childrenflags thecommandcomponent:CommandSeparatorrenders[role=separator]inside cmdk'srole="listbox"list, and a listbox may only ownoption/group.CommandSeparatornow renders its owndivwithrole="none"instead ofCommand.Separator. It keeps cmdk's visibility behaviour (hidden while a search is active unlessalwaysRenderis set) viauseCommandState. Classes anddata-slotare the same, so it looks identical.Upstream status
1.1.1(Mar 2025) is the latest release, and there are no newer commits that touch the separator.Command.Separatorsetsrole="separator"after spreading props, so you can't override it withrole.new-york-v4command component still wrapsCommandPrimitive.Separatorunchanged, so it has the same violation.command.tsxhas the same pattern and is out of scope here.Notes
CommandSeparatorPropsis now an explicit interface (divprops +alwaysRender). TheasChildprop inherited from cmdk's primitive is no longer accepted. Nothing in the repo used it.cmdk-separatorattribute is no longer rendered. Nothing in the repo selects on it.vitest.config.tsnow uses Vite'sresolve.tsconfigPathsinstead of a single@alias. Without it, registry components that import other registry components (@/components/ui/dialog) can't be loaded in tests.Test plan
registry/command/command.test.tsx: no[role=separator]in the DOM, the separator sits inside the listbox withrole="none", it's hidden while searching, and it's shown whenalwaysRenderis setaria-required-childrenrun: violation withCommand.Separator, none with the newCommandSeparatorpnpm typecheck,pnpm lint(oxlint type-aware),pnpm test,pnpm -w knip, package build, registry buildNo changeset: the repo doesn't use changesets.