Migrated label and offer filters to MultiSelectCombobox#27044
Merged
Migrated label and offer filters to MultiSelectCombobox#27044
Conversation
Contributor
WalkthroughAdds a generic 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
rob-ghost
force-pushed
the
feat/offers-filters-multiselect-combobox
branch
from
ea1937a to
c0aaee4
Compare
![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: 2
🧹 Nitpick comments (6)
e2e/AGENTS.md (1)
30-31: Make the “run from e2e/ directory” step explicit in the command block.The text says to run from
e2e/, but the snippet doesn’t includecd e2e, which makes copy/paste less reliable.📌 Suggested doc tweak
# Terminal 2: Run e2e tests from the e2e/ directory +cd e2e yarn test # Run all tests🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/AGENTS.md` around lines 30 - 31, Update the command block in AGENTS.md so it explicitly shows changing into the e2e directory before running tests (i.e., add a separate command entry for `cd e2e` immediately before `yarn test`) to make the “run from e2e/ directory” step copy/paste-safe; edit the snippet containing the two-line block (currently showing only `yarn test`) to include the `cd e2e` command and a brief comment if desired.apps/posts/src/components/label-picker/edit-row.tsx (2)
65-73: Consider surfacing delete errors to the user.Similar to save, when
onDeletethrows, the error is silently caught. Users may not know why deletion failed.♻️ Proposed enhancement
const handleDelete = async () => { setIsDeleting(true); try { await onDelete(label.id); - } catch { + } catch (e) { setIsDeleting(false); setShowDeleteConfirm(false); + setError('Failed to delete. Please try again.'); } };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/posts/src/components/label-picker/edit-row.tsx` around lines 65 - 73, handleDelete currently swallows errors from onDelete, leaving users unaware when deletion fails; update handleDelete to catch the thrown error and surface it to the user (e.g., via an error state or toast) while still resetting UI flags. Specifically, in the handleDelete function, wrap await onDelete(label.id) in try/catch that captures the error and calls the component's error reporting mechanism (or set a local error state) and then calls setIsDeleting(false) and setShowDeleteConfirm(false) as appropriate; ensure the catch includes the error value so you can log or display a meaningful message instead of silently ignoring it.
38-51: Consider surfacing save errors to the user.When
onSavethrows,isSavingis reset but no error feedback is shown to the user. They may not understand why the save didn't complete.♻️ Proposed enhancement for error feedback
const handleSave = async () => { const validationError = validate(name); if (validationError) { setError(validationError); return; } setIsSaving(true); try { await onSave(label.id, name.trim()); onCancel(); - } catch { + } catch (e) { setIsSaving(false); + setError('Failed to save. Please try again.'); } };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/posts/src/components/label-picker/edit-row.tsx` around lines 38 - 51, The save handler handleSave currently resets isSaving on failure but provides no user-facing message; update the catch block in handleSave to capture the thrown error (e.g., catch (err)), setIsSaving(false), and call setError with a meaningful message (use err.message if available or a fallback like "Failed to save label") so users see why onSave(label.id, name.trim()) failed; keep the existing onCancel call only on success.apps/shade/src/components/ui/multi-select-combobox.tsx (1)
1-16: Missing Storybook documentation file.As per coding guidelines, atomic UI components in
src/components/ui/*must have a corresponding*.stories.tsxfile for Storybook documentation. Consider addingmulti-select-combobox.stories.tsxto document the component's various states and configurations.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/shade/src/components/ui/multi-select-combobox.tsx` around lines 1 - 16, Add a Storybook file named multi-select-combobox.stories.tsx to document the MultiSelectCombobox component (from multi-select-combobox.tsx); the story should export a default meta object and several named stories (Default, WithLoading, WithSelectedValues, Disabled, ManyOptions) that import the component, pass representative props/args (options list, isLoading, disabled, selected values), and demonstrate keyboard/selection behavior so reviewers can interactively verify visuals and states; ensure stories use controls/args where appropriate and export both the default meta and each story as named exports.apps/shade/src/index.ts (1)
21-21: Export placement breaks alphabetical ordering.The new export is between
flagandform, butmulti-select-comboboxshould come afterloading-indicatoralphabetically. This is a minor nit for consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/shade/src/index.ts` at line 21, The new re-export export * from './components/ui/multi-select-combobox' is placed between existing exports for "flag" and "form" which breaks the file's alphabetical ordering; move the export for multi-select-combobox so it appears after the export for loading-indicator (i.e., place export * from './components/ui/multi-select-combobox' in the correct alphabetical position among the other exports in apps/shade/src/index.ts) to restore consistent ordering.e2e/helpers/pages/admin/members/members-forward-page.ts (1)
96-109: CSS selectors used instead of semantic locators.Per coding guidelines, E2E tests should "Never use CSS/XPath selectors - only semantic locators or data-testid". Lines 97, 101, and 108 use CSS attribute selectors:
[data-edit-row] input[data-slot="filter-item"]button:not([data-slot="filters-remove"])While
data-*attributes are better than class names, the guideline prefersdata-testidspecifically or semantic locators (roles, labels). Consider usinggetByTestId()for these if possible.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/helpers/pages/admin/members/members-forward-page.ts` around lines 96 - 109, Replace the raw CSS attribute selectors in editLabelInput, getFilterItem, and openFilterValue with semantic/testid locators: use page.getByTestId(...) for elements that expose data-testid (e.g., replace '[data-edit-row] input' with page.getByTestId("edit-row").getByRole("textbox") or similar), replace page.locator('[data-slot="filter-item"]') in getFilterItem with page.getByTestId("filter-item").filter({ hasText: fieldName }), and in openFilterValue find the value button via semantic roles/testing ids (e.g., use filterItem.getByRole("button").filter({ hasNot: filterItem.getByTestId("filters-remove") }) or select the last role("button") that is not the remove/testid) so tests rely on getByTestId/getByRole/getByLabel instead of CSS selectors while preserving the existing methods editLabelInput, getFilterItem, and openFilterValue.
🤖 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/shade/src/components/ui/multi-select-combobox.tsx`:
- Around line 135-142: The conditional call to valueSource.useOptions violates
the Rules of Hooks (valueSource, useOptions, sourceState, resolvedOptions). Fix
by either (A) ensuring valueSource is always non-null and then call
valueSource.useOptions(...) unconditionally so the hook call count never
changes, or (B) move the hook call out of this component and pass the resulting
sourceState (or already-resolved options) into this component as a prop (e.g.,
accept a sourceState or options prop instead of calling valueSource.useOptions
here). Implement one of these two approaches so useOptions is not invoked
conditionally.
In `@e2e/helpers/pages/admin/members/members-forward-page.ts`:
- Around line 86-94: Escape user-provided text before constructing the RegExp in
selectMultiselectOption to prevent ReDoS and ensure literal matching (i.e.,
sanitize value by escaping regex metacharacters before using new RegExp in
selectMultiselectOption), and replace the CSS attribute locator in
searchMultiselectOptions (the '[cmdk-input]' selector) with a more semantic or
stable locator such as a role or data-testid (e.g., use getByRole or a
data-testid attribute) so tests avoid brittle attribute selectors.
---
Nitpick comments:
In `@apps/posts/src/components/label-picker/edit-row.tsx`:
- Around line 65-73: handleDelete currently swallows errors from onDelete,
leaving users unaware when deletion fails; update handleDelete to catch the
thrown error and surface it to the user (e.g., via an error state or toast)
while still resetting UI flags. Specifically, in the handleDelete function, wrap
await onDelete(label.id) in try/catch that captures the error and calls the
component's error reporting mechanism (or set a local error state) and then
calls setIsDeleting(false) and setShowDeleteConfirm(false) as appropriate;
ensure the catch includes the error value so you can log or display a meaningful
message instead of silently ignoring it.
- Around line 38-51: The save handler handleSave currently resets isSaving on
failure but provides no user-facing message; update the catch block in
handleSave to capture the thrown error (e.g., catch (err)), setIsSaving(false),
and call setError with a meaningful message (use err.message if available or a
fallback like "Failed to save label") so users see why onSave(label.id,
name.trim()) failed; keep the existing onCancel call only on success.
In `@apps/shade/src/components/ui/multi-select-combobox.tsx`:
- Around line 1-16: Add a Storybook file named multi-select-combobox.stories.tsx
to document the MultiSelectCombobox component (from multi-select-combobox.tsx);
the story should export a default meta object and several named stories
(Default, WithLoading, WithSelectedValues, Disabled, ManyOptions) that import
the component, pass representative props/args (options list, isLoading,
disabled, selected values), and demonstrate keyboard/selection behavior so
reviewers can interactively verify visuals and states; ensure stories use
controls/args where appropriate and export both the default meta and each story
as named exports.
In `@apps/shade/src/index.ts`:
- Line 21: The new re-export export * from
'./components/ui/multi-select-combobox' is placed between existing exports for
"flag" and "form" which breaks the file's alphabetical ordering; move the export
for multi-select-combobox so it appears after the export for loading-indicator
(i.e., place export * from './components/ui/multi-select-combobox' in the
correct alphabetical position among the other exports in
apps/shade/src/index.ts) to restore consistent ordering.
In `@e2e/AGENTS.md`:
- Around line 30-31: Update the command block in AGENTS.md so it explicitly
shows changing into the e2e directory before running tests (i.e., add a separate
command entry for `cd e2e` immediately before `yarn test`) to make the “run from
e2e/ directory” step copy/paste-safe; edit the snippet containing the two-line
block (currently showing only `yarn test`) to include the `cd e2e` command and a
brief comment if desired.
In `@e2e/helpers/pages/admin/members/members-forward-page.ts`:
- Around line 96-109: Replace the raw CSS attribute selectors in editLabelInput,
getFilterItem, and openFilterValue with semantic/testid locators: use
page.getByTestId(...) for elements that expose data-testid (e.g., replace
'[data-edit-row] input' with page.getByTestId("edit-row").getByRole("textbox")
or similar), replace page.locator('[data-slot="filter-item"]') in getFilterItem
with page.getByTestId("filter-item").filter({ hasText: fieldName }), and in
openFilterValue find the value button via semantic roles/testing ids (e.g., use
filterItem.getByRole("button").filter({ hasNot:
filterItem.getByTestId("filters-remove") }) or select the last role("button")
that is not the remove/testid) so tests rely on getByTestId/getByRole/getByLabel
instead of CSS selectors while preserving the existing methods editLabelInput,
getFilterItem, and openFilterValue.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1fc1fadb-4c5c-4fc1-bce8-b955723de13c
📒 Files selected for processing (13)
apps/posts/src/components/label-picker/edit-row.tsxapps/posts/src/components/label-picker/index.tsapps/posts/src/components/label-picker/label-filter-renderer.tsxapps/posts/src/components/label-picker/label-picker.tsxapps/posts/src/views/members/components/member-table-chrome.tsxapps/posts/src/views/members/components/members-list.tsxapps/shade/src/components/ui/multi-select-combobox.tsxapps/shade/src/index.tse2e/AGENTS.mde2e/helpers/environment/service-managers/ghost-manager.tse2e/helpers/pages/admin/members/members-forward-page.tse2e/scripts/run-playwright-host.she2e/tests/admin/members-forward/multiselect-filters.test.ts
rob-ghost
force-pushed
the
feat/offers-filters-multiselect-combobox
branch
4 times, most recently
from
3b667db to
0e84d17
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
apps/shade/src/components/ui/multi-select-combobox.tsx (1)
1-16: Missing Storybook file for the component.As per coding guidelines, atomic UI components in
src/components/ui/*must have a corresponding*.stories.tsxfile for Storybook documentation. Please addmulti-select-combobox.stories.tsxalongside this file.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/shade/src/components/ui/multi-select-combobox.tsx` around lines 1 - 16, Add a Storybook story file named multi-select-combobox.stories.tsx next to multi-select-combobox.tsx that documents the MultiSelectCombobox component (the exported component in that file) and demonstrates common states (default, loading with Loader2, selected values using FilterOption/ValueSource types, and empty state using CommandEmpty). Ensure the story imports the component from './multi-select-combobox', uses controls/args for props, shows the Check icon selection state, and includes accessibility/interaction examples for CommandInput/CommandList usage so the component is visible and testable in Storybook.apps/posts/src/components/label-picker/label-filter-renderer.tsx (1)
55-109: Consider extracting stable picker callbacks to reduce re-renders.The
renderItemcallback depends onpicker, which is a new object reference on every render. This causesrenderItemto be recreated frequently, potentially triggering unnecessary re-renders in theMultiSelectCombobox.♻️ Suggested optimization
Extract only the specific functions you need from the picker:
+ const {isDuplicateName, deleteLabel, editLabel} = picker; + const renderItem = useCallback(({option, isSelected, onSelect}: RenderItemProps<string>) => { // ... existing code using isDuplicateName, deleteLabel, editLabel directly - }, [editingLabelId, picker]); + }, [editingLabelId, isDuplicateName, deleteLabel, editLabel]);Apply the same pattern to
renderFooterby extractingcanCreateFromSearch,isCreating,createLabel, andtoggleLabel.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/posts/src/components/label-picker/label-filter-renderer.tsx` around lines 55 - 109, renderItem currently lists picker in its dependency array which forces recreation whenever the picker object identity changes; instead destructure the exact members you use from picker (e.g. const { deleteLabel, editLabel, isDuplicateName } = picker) and reference those named bindings in renderItem's deps so renderItem no longer depends on the entire picker object; apply the same pattern to renderFooter by extracting canCreateFromSearch, isCreating, createLabel, and toggleLabel from picker and using those specific bindings in renderFooter's dependency array.
🤖 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/posts/src/components/label-picker/edit-row.tsx`:
- Around line 38-51: The catch blocks in handleSave (and similar handlers like
any onDelete caller) silently swallow exceptions; update the catch to capture
the thrown error and surface it (either re-throw it to let the parent/mutation
layer handle it or set an inline error) rather than doing nothing: in handleSave
wrap the catch as catch (err) { setIsSaving(false); setError(err?.message ??
'Save failed'); /* or rethrow err */ } and apply the same pattern to the delete
handler so UI state is reset and the user receives feedback.
---
Nitpick comments:
In `@apps/posts/src/components/label-picker/label-filter-renderer.tsx`:
- Around line 55-109: renderItem currently lists picker in its dependency array
which forces recreation whenever the picker object identity changes; instead
destructure the exact members you use from picker (e.g. const { deleteLabel,
editLabel, isDuplicateName } = picker) and reference those named bindings in
renderItem's deps so renderItem no longer depends on the entire picker object;
apply the same pattern to renderFooter by extracting canCreateFromSearch,
isCreating, createLabel, and toggleLabel from picker and using those specific
bindings in renderFooter's dependency array.
In `@apps/shade/src/components/ui/multi-select-combobox.tsx`:
- Around line 1-16: Add a Storybook story file named
multi-select-combobox.stories.tsx next to multi-select-combobox.tsx that
documents the MultiSelectCombobox component (the exported component in that
file) and demonstrates common states (default, loading with Loader2, selected
values using FilterOption/ValueSource types, and empty state using
CommandEmpty). Ensure the story imports the component from
'./multi-select-combobox', uses controls/args for props, shows the Check icon
selection state, and includes accessibility/interaction examples for
CommandInput/CommandList usage so the component is visible and testable in
Storybook.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d25e24d5-a8cb-464b-9fe2-1c6568652bdf
📒 Files selected for processing (13)
apps/posts/src/components/label-picker/edit-row.tsxapps/posts/src/components/label-picker/index.tsapps/posts/src/components/label-picker/label-filter-renderer.tsxapps/posts/src/components/label-picker/label-picker.tsxapps/posts/src/views/members/components/member-table-chrome.tsxapps/posts/src/views/members/components/members-list.tsxapps/shade/src/components/ui/multi-select-combobox.tsxapps/shade/src/index.tse2e/AGENTS.mde2e/helpers/environment/service-managers/ghost-manager.tse2e/helpers/pages/admin/members/members-forward-page.tse2e/scripts/run-playwright-host.she2e/tests/admin/members-forward/multiselect-filters.test.ts
✅ Files skipped from review due to trivial changes (7)
- apps/posts/src/views/members/components/member-table-chrome.tsx
- apps/shade/src/index.ts
- apps/posts/src/views/members/components/members-list.tsx
- apps/posts/src/components/label-picker/index.ts
- e2e/scripts/run-playwright-host.sh
- e2e/AGENTS.md
- e2e/tests/admin/members-forward/multiselect-filters.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/posts/src/components/label-picker/label-picker.tsx
rob-ghost
force-pushed
the
feat/offers-filters-multiselect-combobox
branch
from
0e84d17 to
b1bac32
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/posts/src/components/label-picker/label-filter-renderer.tsx (1)
145-153: Trigger button lacks accessible name when showing count.When multiple labels are selected, the trigger shows "X labels" which is informative, but screen readers won't know which labels are selected. Consider adding an
aria-labelthat lists the selected labels or indicates this opens a label selection popover.♿ Proposed accessibility improvement
<PopoverTrigger asChild> <button ref={triggerRef} + aria-label={picker.resolvedSelectedLabels.length > 1 + ? `${picker.resolvedSelectedLabels.length} labels selected: ${picker.resolvedSelectedLabels.map(l => l.name).join(', ')}` + : undefined} className="flex size-full items-center truncate text-left text-sm" type="button" > {triggerText} </button> </PopoverTrigger>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/posts/src/components/label-picker/label-filter-renderer.tsx` around lines 145 - 153, The PopoverTrigger button (button using triggerRef and showing triggerText) lacks an accessible name when it displays counts; update the trigger to include an aria-label that exposes the selected labels or a clear summary (e.g., "3 labels selected: bug, frontend, urgent") so screen readers know which labels are active and that this opens the label picker; compute the aria string from the current selected labels (from the component's selectedLabels/selection state) and fall back to a generic label like "Open label selector" when none are selected, and also add aria-haspopup="listbox" (or "dialog" if you render a dialog) to the same button for improved semantics.
🤖 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/posts/src/components/label-picker/label-filter-renderer.tsx`:
- Around line 55-82: The renderItem/EditRow block needs to surface errors from
picker.editLabel and picker.deleteLabel so users see why an action failed;
update the onDelete and onSave handlers passed into EditRow to catch rejections
and forward an error message (e.g., via a new onError prop or by calling a
shared UI notifier/toast) instead of silently resetting editingLabelId, and only
clear editingLabelId after a successful operation; reference renderItem,
editingLabelId, EditRow, picker.editLabel and picker.deleteLabel when making
these changes.
---
Nitpick comments:
In `@apps/posts/src/components/label-picker/label-filter-renderer.tsx`:
- Around line 145-153: The PopoverTrigger button (button using triggerRef and
showing triggerText) lacks an accessible name when it displays counts; update
the trigger to include an aria-label that exposes the selected labels or a clear
summary (e.g., "3 labels selected: bug, frontend, urgent") so screen readers
know which labels are active and that this opens the label picker; compute the
aria string from the current selected labels (from the component's
selectedLabels/selection state) and fall back to a generic label like "Open
label selector" when none are selected, and also add aria-haspopup="listbox" (or
"dialog" if you render a dialog) to the same button for improved semantics.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 86cab940-b211-447e-aa8d-daf1bfdce074
📒 Files selected for processing (4)
apps/posts/src/components/label-picker/edit-row.tsxapps/posts/src/components/label-picker/index.tsapps/posts/src/components/label-picker/label-filter-renderer.tsxapps/posts/src/components/label-picker/label-picker.tsx
✅ Files skipped from review due to trivial changes (1)
- apps/posts/src/components/label-picker/index.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/posts/src/components/label-picker/edit-row.tsx
Comment on lines
+55
to
+82
| const renderItem = useCallback(({option, isSelected, onSelect}: RenderItemProps<string>) => { | ||
| const labelId = String(option.metadata?.id ?? ''); | ||
|
|
||
| if (editingLabelId === labelId) { | ||
| const label: Label = { | ||
| id: labelId, | ||
| name: option.label, | ||
| slug: String(option.value), | ||
| created_at: '', | ||
| updated_at: '' | ||
| }; | ||
| return ( | ||
| <EditRow | ||
| key={labelId} | ||
| isDuplicateName={picker.isDuplicateName} | ||
| label={label} | ||
| onCancel={() => setEditingLabelId(null)} | ||
| onDelete={async (id) => { | ||
| await picker.deleteLabel(id); | ||
| setEditingLabelId(null); | ||
| }} | ||
| onSave={async (id, name) => { | ||
| await picker.editLabel(id, name); | ||
| setEditingLabelId(null); | ||
| }} | ||
| /> | ||
| ); | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify EditRow only uses id, name, slug from the Label object
ast-grep --pattern $'EditRow'
rg -n 'created_at|updated_at' apps/posts/src/components/label-picker/edit-row.tsxRepository: TryGhost/Ghost
Length of output: 694
🏁 Script executed:
cat apps/posts/src/components/label-picker/edit-row.tsxRepository: TryGhost/Ghost
Length of output: 5148
🏁 Script executed:
cat apps/posts/src/components/label-picker/label-filter-renderer.tsx | head -100Repository: TryGhost/Ghost
Length of output: 3715
🏁 Script executed:
cat apps/posts/src/components/label-picker/label-filter-renderer.tsx | tail -n +100Repository: TryGhost/Ghost
Length of output: 3168
Add error feedback when label save/delete operations fail.
The EditRow component catches errors from picker.editLabel and picker.deleteLabel but doesn't display any user feedback—the operation button simply returns to normal state without explaining why the action failed. Consider showing an error message to the user when these async operations reject.
The reconstructed Label object with empty strings for created_at and updated_at is safe; EditRow only uses id and name.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/posts/src/components/label-picker/label-filter-renderer.tsx` around
lines 55 - 82, The renderItem/EditRow block needs to surface errors from
picker.editLabel and picker.deleteLabel so users see why an action failed;
update the onDelete and onSave handlers passed into EditRow to catch rejections
and forward an error message (e.g., via a new onError prop or by calling a
shared UI notifier/toast) instead of silently resetting editingLabelId, and only
clear editingLabelId after a successful operation; reference renderItem,
editingLabelId, EditRow, picker.editLabel and picker.deleteLabel when making
these changes.
Tests cover selecting, deselecting, searching, editing, and deleting labels via the multiselect combobox filter, plus offer filter selection and search. Also fixed sticky header z-index values to avoid overlapping filter popovers and improved e2e docs/error messages for dev mode setup.
New reusable combobox component supporting multi-select with search, async option loading, and customizable rendering for filter UIs
The inline label filter used a custom InlinePopover/InlineList implementation that duplicated logic already available in Shade's MultiSelectCombobox. This replaces it with the shared component, extracts EditRow to its own file, and adds onSearchChange support to MultiSelectCombobox for external search sync.
rob-ghost
force-pushed
the
feat/offers-filters-multiselect-combobox
branch
from
b1bac32 to
0f076fb
Compare
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
e2e/helpers/pages/admin/members/members-forward-page.ts (2)
104-106: Consider using a more specific selector or data-testid.The
[data-edit-row] inputselector is reasonable, but for improved stability, consider adding a specificdata-testidto the input element itself (e.g.,data-testid="edit-label-input") rather than relying on the structural relationship.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/helpers/pages/admin/members/members-forward-page.ts` around lines 104 - 106, The selector for editLabelInput is brittle because it uses a structural query ('[data-edit-row] input'); update the test to target a stable attribute instead by changing the page.locator call in the editLabelInput getter to use a specific test id on the input (e.g., data-testid="edit-label-input") and ensure the corresponding input in the application/template adds that data-testid; reference the editLabelInput getter and the page.locator('[data-edit-row] input') expression when making the change.
112-117: Complex selector may be fragile if DOM structure changes.The
button:not([data-slot="filters-remove"]).last()selector relies on button ordering. If the filter item's button arrangement changes, this could break. Consider adding adata-slot="filter-value"or similar attribute to the value button for more targeted selection.♻️ Proposed approach
If the component can be updated:
- await filterItem.locator('button:not([data-slot="filters-remove"])').last().click(); + await filterItem.locator('[data-slot="filter-value"]').click();This would require adding
data-slot="filter-value"to the value button in the component implementation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/helpers/pages/admin/members/members-forward-page.ts` around lines 112 - 117, The selector in openFilterValue (in members-forward-page.ts) is fragile because it relies on button order inside getFilterItem; change the component to add a stable attribute (e.g., data-slot="filter-value") to the value button and update openFilterValue to click that button (use filterItem.locator('[data-slot="filter-value"]').click()); if changing the component isn’t possible, replace the order-dependent selector with a clearer DOM query (e.g., target a specific class, aria-label, or data attribute on the value button) inside openFilterValue to avoid relying on button position.apps/posts/src/components/label-picker/label-filter-renderer.tsx (1)
111-135: Footer create flow handles edge cases correctly.The
renderFootercallback properly:
- Trims the search input before checking
canCreateFromSearch- Returns
nullwhen creation shouldn't be shown- Disables the button while
picker.isCreating- Toggles the new label's slug after creation and clears search
One minor note: if
createLabelthrows, the error isn't surfaced to the user, similar to the edit/delete handlers. TheclearSearch()will still be called even on failure.🛡️ Optional: Only clear search on success
onClick={async () => { - const newLabel = await picker.createLabel(searchInput.trim()); - if (newLabel) { - picker.toggleLabel(newLabel.slug); + try { + const newLabel = await picker.createLabel(searchInput.trim()); + if (newLabel) { + picker.toggleLabel(newLabel.slug); + } + clearSearch(); + } catch { + // Error handling is managed by mutation hooks } - clearSearch(); }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/posts/src/components/label-picker/label-filter-renderer.tsx` around lines 111 - 135, Wrap the async createLabel call inside renderFooter in a try/catch so failures are handled like the edit/delete handlers: call await picker.createLabel(searchInput.trim()) inside try, then on success call picker.toggleLabel(newLabel.slug) and clearSearch(); in catch log or surface the error (e.g., console.error or call an existing picker.onError/picker.setError if available) and avoid clearing the search on failure so the user can retry; ensure picker.isCreating is still respected (no change to that flag here).apps/posts/src/components/label-picker/edit-row.tsx (1)
75-88: Consider addingaria-invalidfor accessibility.The input displays an error message below it when validation fails, but the input itself doesn't indicate its invalid state to assistive technologies.
♿ Proposed accessibility improvement
<input ref={inputRef} className="h-7 w-full rounded border border-border bg-background px-2 text-sm outline-hidden focus:ring-1 focus:ring-ring disabled:opacity-50" disabled={isBusy} type="text" value={name} + aria-invalid={!!error} + aria-describedby={error ? 'edit-row-error' : undefined} onChange={(e) => { setName(e.target.value); setError(''); }} onKeyDown={handleKeyDown} /> -{error && <span className="text-xs text-destructive">{error}</span>} +{error && <span id="edit-row-error" className="text-xs text-destructive">{error}</span>}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/posts/src/components/label-picker/edit-row.tsx` around lines 75 - 88, The input in the EditRow component should expose its invalid state to assistive tech: add aria-invalid={!!error} to the input and, when an error exists, add aria-describedby pointing to the error message element’s id so screen readers associate the message with the field; ensure the error message DOM (the element updated via setError) has a stable id (e.g., derived from the component or a prop) that matches the aria-describedby. Target the input using the existing identifiers (inputRef, name, setName, setError, handleKeyDown, isBusy) and update the error rendering to include the matching id.
🤖 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/shade/src/components/ui/multi-select-combobox.tsx`:
- Around line 51-88: Create a Storybook file named
multi-select-combobox.stories.tsx that documents the MultiSelectCombobox
component: export default metadata for the component, add stories demonstrating
default multi-select behavior, searchable mode, async valueSource usage,
maxSelections and single-select (isMultiSelect: false) cases, and a story
showing custom renderItem/header/footer and i18n overrides; import and use the
MultiSelectCombobox component (and its props interface MultiSelectComboboxProps)
and mock options/valueSource handlers so controls and actions work in Storybook.
- Around line 259-265: getCommandItemValue currently returns undefined for
selected items which breaks cmdk keyboard navigation; change getCommandItemValue
(and any callers) to always return a non-undefined string (remove the isSelected
branch and parameter) by concatenating option.label and option.detail (e.g.,
label + " - " + detail when present), and ensure the CommandItem value prop uses
this string for both selected and unselected items so keyboard navigation and
filtering work reliably.
---
Nitpick comments:
In `@apps/posts/src/components/label-picker/edit-row.tsx`:
- Around line 75-88: The input in the EditRow component should expose its
invalid state to assistive tech: add aria-invalid={!!error} to the input and,
when an error exists, add aria-describedby pointing to the error message
element’s id so screen readers associate the message with the field; ensure the
error message DOM (the element updated via setError) has a stable id (e.g.,
derived from the component or a prop) that matches the aria-describedby. Target
the input using the existing identifiers (inputRef, name, setName, setError,
handleKeyDown, isBusy) and update the error rendering to include the matching
id.
In `@apps/posts/src/components/label-picker/label-filter-renderer.tsx`:
- Around line 111-135: Wrap the async createLabel call inside renderFooter in a
try/catch so failures are handled like the edit/delete handlers: call await
picker.createLabel(searchInput.trim()) inside try, then on success call
picker.toggleLabel(newLabel.slug) and clearSearch(); in catch log or surface the
error (e.g., console.error or call an existing picker.onError/picker.setError if
available) and avoid clearing the search on failure so the user can retry;
ensure picker.isCreating is still respected (no change to that flag here).
In `@e2e/helpers/pages/admin/members/members-forward-page.ts`:
- Around line 104-106: The selector for editLabelInput is brittle because it
uses a structural query ('[data-edit-row] input'); update the test to target a
stable attribute instead by changing the page.locator call in the editLabelInput
getter to use a specific test id on the input (e.g.,
data-testid="edit-label-input") and ensure the corresponding input in the
application/template adds that data-testid; reference the editLabelInput getter
and the page.locator('[data-edit-row] input') expression when making the change.
- Around line 112-117: The selector in openFilterValue (in
members-forward-page.ts) is fragile because it relies on button order inside
getFilterItem; change the component to add a stable attribute (e.g.,
data-slot="filter-value") to the value button and update openFilterValue to
click that button (use
filterItem.locator('[data-slot="filter-value"]').click()); if changing the
component isn’t possible, replace the order-dependent selector with a clearer
DOM query (e.g., target a specific class, aria-label, or data attribute on the
value button) inside openFilterValue to avoid relying on button position.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0b1eeed8-f36e-4c55-8838-169ad0dc09c3
📒 Files selected for processing (11)
apps/posts/src/components/label-picker/edit-row.tsxapps/posts/src/components/label-picker/index.tsapps/posts/src/components/label-picker/label-filter-renderer.tsxapps/posts/src/components/label-picker/label-picker.tsxapps/shade/src/components/ui/multi-select-combobox.tsxapps/shade/src/index.tse2e/AGENTS.mde2e/helpers/environment/service-managers/ghost-manager.tse2e/helpers/pages/admin/members/members-forward-page.tse2e/scripts/run-playwright-host.she2e/tests/admin/members-forward/multiselect-filters.test.ts
✅ Files skipped from review due to trivial changes (3)
- apps/shade/src/index.ts
- e2e/AGENTS.md
- e2e/tests/admin/members-forward/multiselect-filters.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/posts/src/components/label-picker/index.ts
- e2e/scripts/run-playwright-host.sh
- apps/posts/src/components/label-picker/label-picker.tsx
Comment on lines
+51
to
+88
| export interface MultiSelectComboboxProps<T = unknown> { | ||
| /** All available options (used for static/client-filtered mode) */ | ||
| options?: FilterOption<T>[]; | ||
| /** Currently selected values */ | ||
| values: T[]; | ||
| /** Called when selection changes */ | ||
| onChange: (values: T[]) => void; | ||
| /** Async option source — when provided, options prop is ignored and search is server-side */ | ||
| valueSource?: ValueSource<string>; | ||
| /** Whether the search input is shown (default true) */ | ||
| searchable?: boolean; | ||
| /** Whether options are loading (static mode only) */ | ||
| isLoading?: boolean; | ||
| /** Max number of selected values */ | ||
| maxSelections?: number; | ||
| /** Whether this is a multiselect (default true) */ | ||
| isMultiSelect?: boolean; | ||
| /** Auto-close callback — called when the combobox wants to close (e.g., single-select after picking) */ | ||
| onClose?: () => void; | ||
| /** Whether to auto-close after each selection */ | ||
| autoCloseOnSelect?: boolean; | ||
| /** CSS class applied to the outermost wrapper */ | ||
| className?: string; | ||
| /** CSS class applied to the search input */ | ||
| searchInputClassName?: string; | ||
| /** Whether cmdk should client-filter (default: true when no valueSource, false otherwise) */ | ||
| shouldFilter?: boolean; | ||
| /** Called when search input changes (for external sync, e.g., server-side filtering) */ | ||
| onSearchChange?: (value: string) => void; | ||
| /** i18n strings */ | ||
| i18n?: Partial<MultiSelectComboboxI18n>; | ||
| /** Custom item renderer — replaces the default option row */ | ||
| renderItem?: (props: RenderItemProps<T>) => React.ReactNode; | ||
| /** Render prop for content above the search input */ | ||
| header?: (props: HeaderRenderProps<T>) => React.ReactNode; | ||
| /** Render prop for content below the option list */ | ||
| footer?: (props: FooterRenderProps) => React.ReactNode; | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if a stories file exists for this component
fd -e tsx -e ts "multi-select-combobox.stories" apps/shade/src/components/ui/Repository: TryGhost/Ghost
Length of output: 1775
🏁 Script executed:
#!/bin/bash
# Check if a stories file exists for this component
fd -e tsx -e ts "multi-select-combobox.stories" apps/shade/src/components/ui/Repository: TryGhost/Ghost
Length of output: 40
🏁 Script executed:
#!/bin/bash
# Check if the component file exists
fd -e tsx "multi-select-combobox" apps/shade/src/components/ui/Repository: TryGhost/Ghost
Length of output: 112
Add Storybook documentation file.
The component requires a multi-select-combobox.stories.tsx file alongside it. Per coding guidelines for atomic UI components in apps/shade/src/components/ui/*, each component must have a corresponding Storybook documentation file.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/shade/src/components/ui/multi-select-combobox.tsx` around lines 51 - 88,
Create a Storybook file named multi-select-combobox.stories.tsx that documents
the MultiSelectCombobox component: export default metadata for the component,
add stories demonstrating default multi-select behavior, searchable mode, async
valueSource usage, maxSelections and single-select (isMultiSelect: false) cases,
and a story showing custom renderItem/header/footer and i18n overrides; import
and use the MultiSelectCombobox component (and its props interface
MultiSelectComboboxProps) and mock options/valueSource handlers so controls and
actions work in Storybook.
Comment on lines
+259
to
+265
| const getCommandItemValue = (option: FilterOption<T>, isSelected: boolean): string | undefined => { | ||
| if (isSelected) { | ||
| return undefined; | ||
| } | ||
| const detail = option.detail ? ` - ${option.detail}` : ''; | ||
| return option.label + detail; | ||
| }; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how CommandItem handles undefined value prop in other components
rg -n "value=\{undefined\}|value={undefined}" apps/shade/src/components/ui/ --type tsx
rg -A 5 "CommandItem" apps/shade/src/components/ui/command.tsx | head -50Repository: TryGhost/Ghost
Length of output: 1775
🏁 Script executed:
#!/bin/bash
# First, locate and examine the multi-select-combobox.tsx file
find . -name "multi-select-combobox.tsx" -type f 2>/dev/null | head -5Repository: TryGhost/Ghost
Length of output: 114
🏁 Script executed:
#!/bin/bash
# Search for CommandItem usage in the ui components directory to understand patterns
rg -n "CommandItem" apps/shade/src/components/ui/ --type tsx -A 3 | head -100Repository: TryGhost/Ghost
Length of output: 85
🏁 Script executed:
#!/bin/bash
# Check the actual implementation around lines 259-265
if [ -f "apps/shade/src/components/ui/multi-select-combobox.tsx" ]; then
sed -n '250,280p' apps/shade/src/components/ui/multi-select-combobox.tsx
fiRepository: TryGhost/Ghost
Length of output: 1421
🏁 Script executed:
#!/bin/bash
# Check the command.tsx file to understand CommandItem implementation
cat -n apps/shade/src/components/ui/command.tsx | head -150Repository: TryGhost/Ghost
Length of output: 6136
🏁 Script executed:
#!/bin/bash
# Search for cmdk imports in the codebase
rg -n "from.*cmdk|import.*cmdk" apps/shade/src/components/ui/ -A 2 | head -50Repository: TryGhost/Ghost
Length of output: 293
🏁 Script executed:
#!/bin/bash
# Search for other uses of CommandItem with value prop to see patterns
rg -n "CommandItem" apps/shade/src/components/ui/multi-select-combobox.tsx -B 3 -A 8Repository: TryGhost/Ghost
Length of output: 1777
🏁 Script executed:
#!/bin/bash
# Check package.json to see cmdk version being used
cat apps/shade/package.json | grep -A 2 -B 2 "cmdk"Repository: TryGhost/Ghost
Length of output: 225
🏁 Script executed:
#!/bin/bash
# Search for any comments or documentation about CommandItem value behavior
rg -n "value.*undefined|undefined.*value|keyboard.*navigation|cmdk" apps/shade/src/components/ui/multi-select-combobox.tsx -B 2 -A 2Repository: TryGhost/Ghost
Length of output: 555
🏁 Script executed:
#!/antml/bash
# Look for similar patterns in other components using CommandItem with value
rg -n "CommandItem" apps/shade/src/components/ -B 2 -A 6 | grep -A 6 -B 2 "value="Repository: TryGhost/Ghost
Length of output: 3057
🏁 Script executed:
#!/bin/bash
# Look at the selection handlers to understand the intent
rg -n "handleSelectSelected|handleSelectUnselected|handleSelect|handleDeselect" apps/shade/src/components/ui/multi-select-combobox.tsx -B 2 -A 5 | head -80Repository: TryGhost/Ghost
Length of output: 2425
🏁 Script executed:
#!/bin/bash
# Search for any tests or comments about this behavior
rg -n "selected.*keyboard|keyboard.*select|cmdk.*filter|undefined.*reason" apps/shade/src/components/ui/ -i -B 2 -A 2Repository: TryGhost/Ghost
Length of output: 650
🏁 Script executed:
#!/bin/bash
# Check if there's a shouldFilter or similar configuration that relates to this
sed -n '1,150p' apps/shade/src/components/ui/multi-select-combobox.tsx | grep -A 10 -B 10 "shouldFilter"Repository: TryGhost/Ghost
Length of output: 2589
🏁 Script executed:
#!/bin/bash
# Look for the complete render logic to understand item grouping
sed -n '310,340p' apps/shade/src/components/ui/multi-select-combobox.tsxRepository: TryGhost/Ghost
Length of output: 1507
🏁 Script executed:
#!/bin/bash
# Check if there's documentation or README about this component
find apps/shade -name "*.md" -o -name "README*" | xargs grep -l "multi-select\|MultiSelect" 2>/dev/nullRepository: TryGhost/Ghost
Length of output: 42
🏁 Script executed:
#!/bin/bash
# Look for any stories/examples that might show intended behavior
find apps/shade -name "*multi-select*.stories.tsx" -o -name "*combobox*.stories.tsx" 2>/dev/null | head -5Repository: TryGhost/Ghost
Length of output: 40
🌐 Web query:
cmdk CommandItem value undefined keyboard navigation filtering
💡 Result:
The issue with cmdk CommandItem where value is undefined, affecting keyboard navigation and filtering, typically occurs when the value prop is not explicitly provided or when item content is dynamic (e.g., containing icons or async-loaded data). Cmdk requires a stable, unique value string for each CommandItem for proper filtering, sorting, selection, and keyboard navigation (ArrowUp/Down, Enter). Key points from official docs and issues: - Value is optional but inferred from textContent if omitted. However, for non-plain-text children or dynamic content, you MUST provide an explicit value prop to avoid undefined/inconsistent behavior. - Values are automatically trimmed and lowercased internally for matching. - Without a stable value, filtering may fail (e.g., backspace not triggering re-filter), keyboard selection skips items, or first item not highlighted in dynamic lists. Fixes: 1. Always set value explicitly, e.g.: Dynamic content Use item.id or a slug as value. 2. For async/dynamic lists, add key={value} to items and consider shouldFilter={false} on Command + manual filtering if needed. 3. Ensure structure: Command > CommandInput > CommandList > CommandGroup > CommandItem. CommandList is required in cmdk v1+. 4. For keyboard nav without input focused, ensure Command root is focusable. Common errors like "undefined is not iterable" are from missing CommandList or cmdk v1 breaking changes (fix: data-[disabled=true] in styles). Examples work without value due to plain text inference, but production code needs explicit values for robustness.
Citations:
- 1: https://www.mintlify.com/dip/cmdk/components/item
- 2: https://www.mintlify.com/dip/cmdk/components/command
- 3: https://github.com/dip/cmdk
- 4: cmdk based components require code to account for value generation shadcn-ui/ui#326
- 5: fix: add value prop to commandItem to fix command search shadcn-ui/ui#1522
- 6: The first item is not selected by default when
Command.Listcontent is dynamic dip/cmdk#280 - 7: CommandItem not updating on asynchronous changes dip/cmdk#267
Provide explicit value for all CommandItems to ensure keyboard navigation works.
The value prop in cmdk's CommandItem is essential for proper keyboard navigation (arrow keys) and filtering. When getCommandItemValue returns undefined for selected items, cmdk cannot reliably navigate or filter those items, potentially causing keyboard users to skip items or experience broken navigation.
Since the component already separates selected and unselected items into distinct groups with proper selection handlers (handleSelectSelected/handleSelectUnselected), the value prop distinction is unnecessary. Provide an explicit value for all items:
const getCommandItemValue = (option: FilterOption<T>): string => {
const detail = option.detail ? ` - ${option.detail}` : '';
return option.label + detail;
};
This ensures cmdk can properly handle keyboard navigation while maintaining the existing toggle-on-click behavior.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/shade/src/components/ui/multi-select-combobox.tsx` around lines 259 -
265, getCommandItemValue currently returns undefined for selected items which
breaks cmdk keyboard navigation; change getCommandItemValue (and any callers) to
always return a non-undefined string (remove the isSelected branch and
parameter) by concatenating option.label and option.detail (e.g., label + " - "
+ detail when present), and ensure the CommandItem value prop uses this string
for both selected and unselected items so keyboard navigation and filtering work
reliably.
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.
The existing label filter in the members list used a custom inline popover with hand-rolled search, selection, and edit/delete UI. The offer filter had a similar but separate implementation. Both filters needed multi-select support for the new filter bar, and duplicating the interaction logic across each filter type was becoming difficult to maintain.
This PR introduces a
MultiSelectComboboxcomponent in Shade that encapsulates the full multi-select dropdown pattern: searchable option list with selected/unselected grouping, async value source support, custom item rendering, and extensible header/footer slots. The label filter renderer now composes this shared component instead of maintaining its own Command-based popover internals. The offer filter gets multi-select for free through the same component. TheEditRowfor inline label editing was extracted into its own file and the now-unusedInlinePopoverandInlineListsub-components were removed fromlabel-picker.tsx, significantly reducing its size.The z-index values on the sticky member table header were also lowered to prevent them from overlapping the new filter popovers.
E2E tests cover both label and offer multi-select filter workflows, including selecting/deselecting labels, searching within the combobox, and inline label editing and deletion from the filter popover.