Added label picker with inline editing to members list#26666
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds label-management APIs and UI: three new label mutation hooks (create, edit, delete); a reusable LabelPicker component and useLabelPicker hook for selection, creation, editing, and deletion; LabelFilterRenderer used in member filters; refactors AddLabelModal and RemoveLabelModal to multi-label flows with onConfirm accepting arrays of label IDs; adjusts members filter configuration and multiselect URL parsing/serialization; adds LabelsResponseType and MembersResponseType mappings to the state bridge. 🚥 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 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/edit-labels-from-filter
branch
2 times, most recently
from
24de33f to
57589c8
Compare
rob-ghost
force-pushed
the
feat/edit-labels-from-filter
branch
from
57589c8 to
7cccd83
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: 5
🧹 Nitpick comments (1)
apps/posts/src/views/members/components/bulk-action-modals/remove-label-modal.tsx (1)
34-40: Consider avoiding unbounded members+labels fetch on modal open.Line 38 uses
limit: 'all'withinclude: 'labels'. On large audiences this can produce a very heavy payload and slow modal interactions. Prefer a dedicated backend aggregate (distinct labels for current filter) or paginated aggregation strategy.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/posts/src/views/members/components/bulk-action-modals/remove-label-modal.tsx` around lines 34 - 40, The current useBrowseMembers call (see useBrowseMembers, membersData, limit: 'all', include: 'labels') performs an unbounded fetch of members plus labels which can be very heavy; replace this with a lightweight approach by either calling a new backend aggregate endpoint that returns distinct labels for the current filter (e.g., getDistinctLabels(nql) or similar) or modifying the client call to remove include: 'labels' and use a paginated request (add page/limit and iterate or fetch only needed page) and then aggregate labels incrementally; update the modal to use the distinct-labels endpoint or paginated aggregation instead of limit: 'all' to avoid large payloads and slow UI.
🤖 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/admin-x-framework/src/api/labels.ts`:
- Around line 60-63: The cache update function update: (_, currentData, id) => {
... } assumes currentData is always defined and will throw when currentData is
undefined; add a null-guard like the create/edit update handlers so you only
access current.labels when currentData exists — if currentData is falsy return
currentData (or currentData as-is) instead of accessing labels, otherwise return
{...current, labels: current.labels.filter(label => label.id !== id)}; reference
the update handler, the currentData variable, LabelsResponseType and the
current.labels.filter call when making the change.
In `@apps/posts/src/components/label-picker/label-picker.tsx`:
- Around line 38-44: EditRow currently always renders delete controls while edit
mode is enabled whenever onEdit is present, leading to a dead delete flow if
onDelete is omitted; update EditRow and the parent toggle logic so delete UI and
flows are only available when an onDelete handler exists. Concretely: in the
EditRow component (props defined by EditRowProps) guard rendering of the delete
button/confirmation and any calls into the delete flow with a runtime check for
props.onDelete (and don't call it if undefined), and adjust the parent that
flips edit mode (where it checks onEdit) to either require onDelete before
exposing delete-specific editing or hide the delete affordance when only onEdit
is supplied; ensure UI state (confirm dialogs, click handlers) only references
onDelete when it's defined.
In `@apps/posts/src/hooks/use-label-picker.ts`:
- Around line 78-87: Selection is updated optimistically in deleteLabel before
the deletion is confirmed, causing UI inconsistency if delete fails; fix by only
changing selection after a successful delete or by reverting on failure. Update
the deleteLabel function: call await deleteLabelMutation(id) inside a try/catch
and only call onSelectionChange(current.filter(s => s !== label.slug)) after the
await resolves successfully; alternatively, if you prefer optimistic UI, perform
the current selection change first but save the previous selection and on
rejection restore it in the catch and surface/log the error. Ensure you
reference deleteLabel, selectedSlugsRef.current, onSelectionChange, and
deleteLabelMutation when making the change.
- Line 26: The label picker is truncated by a hard-coded limit in the
useLabelPicker hook — update the call to useBrowseLabels (currently:
useBrowseLabels({searchParams: {limit: '100'}})) so it returns the full set (or
a configurable/paginated set) instead of only 100 items: either remove or make
the limit configurable via a prop or constant, or implement cursor-based
pagination/fetchAll inside useLabelPicker to iterate useBrowseLabels pages and
merge results before assigning labelsData; ensure duplicate checks use the
combined result and keep isLoading behavior consistent while fetching additional
pages.
In `@apps/posts/src/views/members/components/members-actions.tsx`:
- Around line 57-76: handleAddLabel currently runs per-label bulkEditAsync
sequentially and treats any error the same, causing mixed states with only a
generic failure message; change it to run per-label operations (e.g., map
labelIds to bulkEditAsync calls and use Promise.allSettled) so you can count
fulfilled vs rejected results, close the modal and show toast.success only for
full success, and show a partial-success toast when some labels applied (include
counts and any error messages from rejected results); update the catch/handling
to surface the actual error info instead of swallowing it and keep references to
handleAddLabel, bulkEditAsync, setShowAddLabelModal, and
toast.success/toast.error for locating the change.
---
Nitpick comments:
In
`@apps/posts/src/views/members/components/bulk-action-modals/remove-label-modal.tsx`:
- Around line 34-40: The current useBrowseMembers call (see useBrowseMembers,
membersData, limit: 'all', include: 'labels') performs an unbounded fetch of
members plus labels which can be very heavy; replace this with a lightweight
approach by either calling a new backend aggregate endpoint that returns
distinct labels for the current filter (e.g., getDistinctLabels(nql) or similar)
or modifying the client call to remove include: 'labels' and use a paginated
request (add page/limit and iterate or fetch only needed page) and then
aggregate labels incrementally; update the modal to use the distinct-labels
endpoint or paginated aggregation instead of limit: 'all' to avoid large
payloads and slow UI.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between 08056d2 and 57589c8c26b4d9ee7d8518ccd638efd31aad1fa6.
📒 Files selected for processing (11)
apps/admin-x-framework/src/api/labels.tsapps/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/hooks/use-label-picker.tsapps/posts/src/views/members/components/bulk-action-modals/add-label-modal.tsxapps/posts/src/views/members/components/bulk-action-modals/remove-label-modal.tsxapps/posts/src/views/members/components/members-actions.tsxapps/posts/src/views/members/hooks/use-members-filter-config.tsxapps/posts/src/views/members/hooks/use-members-filter-state.tsghost/admin/app/services/state-bridge.js
ref https://linear.app/tryghost/issue/BER-3342/ Replaced the simple Select dropdowns for label management with a searchable, multi-select label picker that supports inline renaming and deletion. Used in filter value cells, add-label bulk modal (with label creation), and remove-label bulk modal. Added label CRUD mutations to admin-x-framework.
rob-ghost
force-pushed
the
feat/edit-labels-from-filter
branch
from
7cccd83 to
60d19ea
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (5)
apps/admin-x-framework/src/api/labels.ts (1)
60-63:⚠️ Potential issue | 🟡 MinorGuard delete cache updates when query data is missing.
Line 62 assumes
currentDatais always present. If the labels cache is cold,current.labelswill throw during the delete mutation update.Suggested fix
update: (_, currentData, id) => { - const current = currentData as LabelsResponseType; - return {...current, labels: current.labels.filter(label => label.id !== id)}; + const current = currentData as LabelsResponseType | undefined; + return current && { + ...current, + labels: current.labels.filter(label => label.id !== id) + }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/admin-x-framework/src/api/labels.ts` around lines 60 - 63, The optimistic cache update in the update callback assumes currentData exists and accesses current.labels causing a crash on a cold cache; in the update function (update: (_, currentData, id) => { ... }) check that currentData is present and of type LabelsResponseType (or at least has labels) before filtering—if missing, return currentData or undefined to avoid throwing; use a guard (e.g., if (!currentData || !('labels' in currentData)) return currentData) and then perform the labels filter on the safely-typed/current variable.apps/posts/src/components/label-picker/label-picker.tsx (1)
38-44:⚠️ Potential issue | 🟡 MinorHide delete controls when no delete handler is provided.
Line 227 enables edit affordances based on
onEdit, but EditRow always renders delete controls (Line 143-150). IfonDeleteis omitted, users can enter a delete flow with no effective handler.Suggested fix
interface EditRowProps { label: Label; onSave: (id: string, name: string) => Promise<void>; onCancel: () => void; - onDelete: (id: string) => Promise<void>; + onDelete?: (id: string) => Promise<void>; isDuplicateName?: (name: string, excludeId?: string) => boolean; } @@ - <button - className="flex size-7 items-center justify-center rounded text-muted-foreground hover:bg-destructive/10 hover:text-destructive" - title="Delete" - type="button" - onClick={() => setShowDeleteConfirm(true)} - > - <LucideIcon.Trash2 className="size-3.5" /> - </button> + {onDelete && ( + <button + className="flex size-7 items-center justify-center rounded text-muted-foreground hover:bg-destructive/10 hover:text-destructive" + title="Delete" + type="button" + onClick={() => setShowDeleteConfirm(true)} + > + <LucideIcon.Trash2 className="size-3.5" /> + </button> + )}Also applies to: 143-150, 227-273
🤖 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-picker.tsx` around lines 38 - 44, EditRow currently always shows delete controls even though onDelete is optional in EditRowProps; update the EditRow component to conditionally render the delete button and any delete-confirmation UI only when the onDelete prop is provided, and guard any calls to onDelete (e.g., in the delete handler or confirmation flow) with a presence check before invoking; ensure the edit affordances logic (where onEdit gating happens) remains unchanged and that no-op or undefined onDelete will neither render the delete affordance nor trigger any delete flow in methods inside EditRow.apps/posts/src/views/members/components/members-actions.tsx (1)
57-76:⚠️ Potential issue | 🟠 MajorHandle partial failures for multi-label bulk operations.
Line 59-67 and Line 80-88 run multiple mutations, but any mid-sequence failure leaves partial application with only a generic error path. This makes outcomes unclear and can mislead users.
Suggested pattern (apply to add + remove handlers)
- try { - for (const labelId of labelIds) { - await bulkEditAsync({/* ... */}); - } - setShowAddLabelModal(false); - toast.success(labelIds.length > 1 ? 'Labels added successfully' : 'Label added successfully'); - } catch { - toast.error('Failed to add label', { - description: 'There was a problem applying this label. Please try again.' - }); - } + const results = await Promise.allSettled(labelIds.map(labelId => bulkEditAsync({ + filter: nql || '', + all: !nql, + action: {type: 'addLabel', meta: {label: {id: labelId}}} + }))); + + const succeeded = results.filter(r => r.status === 'fulfilled').length; + const failed = results.length - succeeded; + + if (failed === 0) { + setShowAddLabelModal(false); + toast.success(labelIds.length > 1 ? 'Labels added successfully' : 'Label added successfully'); + } else if (succeeded === 0) { + toast.error('Failed to add label', { + description: 'There was a problem applying this label. Please try again.' + }); + } else { + toast.error('Some labels could not be applied', { + description: `${succeeded} succeeded, ${failed} failed.` + }); + }Also applies to: 78-97
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/posts/src/views/members/components/members-actions.tsx` around lines 57 - 76, The multi-label handlers (e.g., handleAddLabel) currently await mutations in a loop and swallow errors into a generic catch, causing silent partial success; change to run the per-label requests while collecting results (use Promise.allSettled or capture per-iteration success/failure) so you can determine which labelIds succeeded vs failed, close the modal only for full success (or still close but surface partial results), and show a clear toast summarizing successes and failures (e.g., "3 labels applied, 1 failed: <reason>"); apply the same pattern to the remove-label handler so both add/remove handlers report per-label outcomes and include error info instead of a single generic error.apps/posts/src/hooks/use-label-picker.ts (2)
78-87:⚠️ Potential issue | 🟡 MinorOnly update selection after delete succeeds (or roll back on failure).
Line 80-84 mutates selection before Line 86 confirms deletion. If delete fails, the UI selection can become inconsistent.
Suggested fix
const deleteLabel = useCallback(async (id: string) => { const label = labels.find(l => l.id === id); - if (label) { - const current = selectedSlugsRef.current; - if (current.includes(label.slug)) { - onSelectionChange(current.filter(s => s !== label.slug)); - } - } await deleteLabelMutation(id); + if (label) { + const current = selectedSlugsRef.current; + if (current.includes(label.slug)) { + onSelectionChange(current.filter(s => s !== label.slug)); + } + } }, [deleteLabelMutation, labels, onSelectionChange]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/posts/src/hooks/use-label-picker.ts` around lines 78 - 87, The deleteLabel function updates selection before awaiting deleteLabelMutation which can leave UI inconsistent on failure; modify deleteLabel (and use selectedSlugsRef and onSelectionChange) to await deleteLabelMutation first, then update selection if the deleted label's slug is present, and wrap the await in try/catch to rollback or restore the original selection (using the captured current array) on error so selection only changes when deletion succeeds.
26-27:⚠️ Potential issue | 🟠 MajorAvoid truncating the label source to 100 items.
Line 26 hard-limits the picker dataset, so labels beyond the first 100 cannot be selected/edited/deleted, and duplicate checks become incomplete.
Suggested fix
- const {data: labelsData, isLoading} = useBrowseLabels({searchParams: {limit: '100'}}); + const {data: labelsData, isLoading} = useBrowseLabels({searchParams: {limit: 'all'}});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/posts/src/hooks/use-label-picker.ts` around lines 26 - 27, The label picker currently limits results by passing {searchParams: {limit: '100'}} to useBrowseLabels causing labelsData to be truncated; remove or change that hard limit so the picker fetches the full label set (or implement proper pagination/infinite loading) — update the call to useBrowseLabels (and any related code that relies on labelsData/labels) to stop passing limit: '100' and ensure labels = useMemo(() => labelsData?.labels || [], [labelsData]) operates on the complete dataset or a correctly paginated stream so duplicate checks and edit/delete can act on all labels.
🧹 Nitpick comments (1)
apps/posts/src/views/members/components/bulk-action-modals/remove-label-modal.tsx (1)
34-42: Consider the scale impact of fetching all filtered members with labels.Using
limit: 'all'+include: 'labels'to derive available labels can become expensive on large member sets. This is functionally right, but likely to be a hotspot as lists grow.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/posts/src/views/members/components/bulk-action-modals/remove-label-modal.tsx` around lines 34 - 42, The current call to useBrowseMembers with limit: 'all' and include: 'labels' (involving symbols useBrowseMembers, nql, include:'labels', limit:'all', membersData) will fetch every matching member and their labels and is likely to be a performance hotspot; instead implement a lightweight labels-only query or an aggregated endpoint: replace this broad fetch with a dedicated call that requests only unique labels (e.g., a new or existing fetchMemberLabels / members/labels aggregation) or paginate/limit the browse call to small batches and accumulate labels server-side, ensuring the UI code that reads membersData adapts to the new response shape and that isMembersLoading is wired to the new labels-only request.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@apps/admin-x-framework/src/api/labels.ts`:
- Around line 60-63: The optimistic cache update in the update callback assumes
currentData exists and accesses current.labels causing a crash on a cold cache;
in the update function (update: (_, currentData, id) => { ... }) check that
currentData is present and of type LabelsResponseType (or at least has labels)
before filtering—if missing, return currentData or undefined to avoid throwing;
use a guard (e.g., if (!currentData || !('labels' in currentData)) return
currentData) and then perform the labels filter on the safely-typed/current
variable.
In `@apps/posts/src/components/label-picker/label-picker.tsx`:
- Around line 38-44: EditRow currently always shows delete controls even though
onDelete is optional in EditRowProps; update the EditRow component to
conditionally render the delete button and any delete-confirmation UI only when
the onDelete prop is provided, and guard any calls to onDelete (e.g., in the
delete handler or confirmation flow) with a presence check before invoking;
ensure the edit affordances logic (where onEdit gating happens) remains
unchanged and that no-op or undefined onDelete will neither render the delete
affordance nor trigger any delete flow in methods inside EditRow.
In `@apps/posts/src/hooks/use-label-picker.ts`:
- Around line 78-87: The deleteLabel function updates selection before awaiting
deleteLabelMutation which can leave UI inconsistent on failure; modify
deleteLabel (and use selectedSlugsRef and onSelectionChange) to await
deleteLabelMutation first, then update selection if the deleted label's slug is
present, and wrap the await in try/catch to rollback or restore the original
selection (using the captured current array) on error so selection only changes
when deletion succeeds.
- Around line 26-27: The label picker currently limits results by passing
{searchParams: {limit: '100'}} to useBrowseLabels causing labelsData to be
truncated; remove or change that hard limit so the picker fetches the full label
set (or implement proper pagination/infinite loading) — update the call to
useBrowseLabels (and any related code that relies on labelsData/labels) to stop
passing limit: '100' and ensure labels = useMemo(() => labelsData?.labels || [],
[labelsData]) operates on the complete dataset or a correctly paginated stream
so duplicate checks and edit/delete can act on all labels.
In `@apps/posts/src/views/members/components/members-actions.tsx`:
- Around line 57-76: The multi-label handlers (e.g., handleAddLabel) currently
await mutations in a loop and swallow errors into a generic catch, causing
silent partial success; change to run the per-label requests while collecting
results (use Promise.allSettled or capture per-iteration success/failure) so you
can determine which labelIds succeeded vs failed, close the modal only for full
success (or still close but surface partial results), and show a clear toast
summarizing successes and failures (e.g., "3 labels applied, 1 failed:
<reason>"); apply the same pattern to the remove-label handler so both
add/remove handlers report per-label outcomes and include error info instead of
a single generic error.
---
Nitpick comments:
In
`@apps/posts/src/views/members/components/bulk-action-modals/remove-label-modal.tsx`:
- Around line 34-42: The current call to useBrowseMembers with limit: 'all' and
include: 'labels' (involving symbols useBrowseMembers, nql, include:'labels',
limit:'all', membersData) will fetch every matching member and their labels and
is likely to be a performance hotspot; instead implement a lightweight
labels-only query or an aggregated endpoint: replace this broad fetch with a
dedicated call that requests only unique labels (e.g., a new or existing
fetchMemberLabels / members/labels aggregation) or paginate/limit the browse
call to small batches and accumulate labels server-side, ensuring the UI code
that reads membersData adapts to the new response shape and that
isMembersLoading is wired to the new labels-only request.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between 57589c8c26b4d9ee7d8518ccd638efd31aad1fa6 and 7cccd83d4f4462b1fdeceddecaa5b10e6cabe328.
📒 Files selected for processing (11)
apps/admin-x-framework/src/api/labels.tsapps/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/hooks/use-label-picker.tsapps/posts/src/views/members/components/bulk-action-modals/add-label-modal.tsxapps/posts/src/views/members/components/bulk-action-modals/remove-label-modal.tsxapps/posts/src/views/members/components/members-actions.tsxapps/posts/src/views/members/hooks/use-members-filter-config.tsxapps/posts/src/views/members/hooks/use-members-filter-state.tsghost/admin/app/services/state-bridge.js
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/posts/src/components/label-picker/index.ts
- apps/posts/src/components/label-picker/label-filter-renderer.tsx
- ghost/admin/app/services/state-bridge.js
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
apps/posts/src/hooks/use-label-picker.ts (1)
26-27:⚠️ Potential issue | 🟠 MajorLabel query is truncated to 100 results.
The hard-coded
limit: '100'means labels beyond the first 100 cannot be selected, edited, or deleted. The duplicate name checks will also be incomplete for sites with more than 100 labels.Consider using
limit: 'all'to fetch the complete set:Suggested fix
- const {data: labelsData, isLoading} = useBrowseLabels({searchParams: {limit: '100'}}); + const {data: labelsData, isLoading} = useBrowseLabels({searchParams: {limit: 'all'}});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/posts/src/hooks/use-label-picker.ts` around lines 26 - 27, The label query in use-label-picker uses a hard-coded limit of '100' which truncates results; update the call to useBrowseLabels({searchParams: {limit: 'all'}}) (or otherwise pass a non-truncating value) so the full label set is returned, then keep using labelsData?.labels in the useMemo; ensure any downstream duplicate-name checks and edit/delete flows in useLabelPicker operate on the full labels array returned by useBrowseLabels.apps/posts/src/views/members/components/members-actions.tsx (1)
57-76:⚠️ Potential issue | 🟠 MajorPer-label bulk updates can fail partially without clear outcome handling.
The sequential
forloop (Lines 59-67) applies labels one at a time. If one succeeds and a subsequent one fails, the user sees only a generic "Failed to add label" message without knowing some labels were applied. This applies equally tohandleRemoveLabel(Lines 80-88).Consider using
Promise.allSettledto handle partial failures gracefully and provide accurate feedback about which operations succeeded or failed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/posts/src/views/members/components/members-actions.tsx` around lines 57 - 76, The current handleAddLabel (and similarly handleRemoveLabel) performs sequential per-label bulkEditAsync calls so a partial failure yields only a generic error; change these handlers to run parallel requests with Promise.allSettled for the array of labelIds (calling bulkEditAsync with the same payload per labelId), then inspect the settled results to compute how many succeeded vs failed and which labelIds failed, close the modal if at least one succeeded, and show a toast summarizing successes and failures (e.g., "X labels added, Y failed" with optional details) instead of the single generic error; update references to handleAddLabel, handleRemoveLabel and bulkEditAsync accordingly.
🧹 Nitpick comments (4)
apps/posts/src/views/members/components/members-actions.tsx (1)
99-114: Inconsistent async handling patterns.
handleUnsubscribeuses.then()/.catch()chaining (Lines 106-113) whilehandleAddLabelandhandleRemoveLabeluseasync/awaitwith try/catch. Consider usingasync/awaitconsistently for better readability:Suggested refactor
- const handleUnsubscribe = useCallback(() => { - bulkEditAsync({ + const handleUnsubscribe = useCallback(async () => { + try { + await bulkEditAsync({ filter: nql || '', all: !nql, action: { type: 'unsubscribe' } - }).then(() => { + }); setShowUnsubscribeModal(false); toast.success('Members unsubscribed successfully'); - }).catch(() => { + } catch { toast.error('Failed to unsubscribe members', { description: 'There was a problem unsubscribing these members. Please try again.' }); - }); + } }, [bulkEditAsync, nql]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/posts/src/views/members/components/members-actions.tsx` around lines 99 - 114, handleUnsubscribe is using Promise .then()/.catch() while sibling handlers (handleAddLabel, handleRemoveLabel) use async/await; convert handleUnsubscribe to an async function and use try/catch to await bulkEditAsync, then call setShowUnsubscribeModal(false) and toast.success on success and toast.error with description on failure to match the pattern used by handleAddLabel/handleRemoveLabel and keep error handling consistent for bulkEditAsync.apps/posts/src/views/members/components/bulk-action-modals/remove-label-modal.tsx (1)
70-77: Consider memoizinghandleConfirmwithuseCallback.Similar to
AddLabelModal,handleConfirmcould be memoized for consistency:Suggested fix
- const handleConfirm = () => { + const handleConfirm = useCallback(() => { const labelIds = picker.labels .filter(l => selectedSlugs.includes(l.slug)) .map(l => l.id); if (labelIds.length > 0) { onConfirm(labelIds); } - }; + }, [picker.labels, selectedSlugs, onConfirm]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/posts/src/views/members/components/bulk-action-modals/remove-label-modal.tsx` around lines 70 - 77, Memoize the RemoveLabelModal's handler by wrapping handleConfirm in React's useCallback to avoid recreating the function on each render; import useCallback and replace the current const handleConfirm = () => { ... } with const handleConfirm = useCallback(() => { const labelIds = picker.labels.filter(l => selectedSlugs.includes(l.slug)).map(l => l.id); if (labelIds.length > 0) onConfirm(labelIds); }, [picker.labels, selectedSlugs, onConfirm]) so it mirrors AddLabelModal's behavior and uses picker.labels, selectedSlugs and onConfirm as dependencies.apps/posts/src/views/members/components/bulk-action-modals/add-label-modal.tsx (1)
42-49: Consider memoizinghandleConfirmwithuseCallback.
handleConfirmis recreated on every render. While this doesn't cause issues with the currentonClickusage, memoizing it would be consistent withhandleOpenChangeand avoids unnecessary function allocations:Suggested fix
- const handleConfirm = () => { + const handleConfirm = useCallback(() => { const labelIds = picker.labels .filter(l => selectedSlugs.includes(l.slug)) .map(l => l.id); if (labelIds.length > 0) { onConfirm(labelIds); } - }; + }, [picker.labels, selectedSlugs, onConfirm]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/posts/src/views/members/components/bulk-action-modals/add-label-modal.tsx` around lines 42 - 49, The handleConfirm function is recreated every render; wrap it with React.useCallback to memoize it like handleOpenChange — reference the handleConfirm function that builds labelIds from picker.labels and selectedSlugs and calls onConfirm(labelIds). Use useCallback with a dependency array including picker.labels, selectedSlugs, and onConfirm so the callback updates when inputs change, and replace the original handleConfirm with this memoized version.apps/posts/src/hooks/use-label-picker.ts (1)
60-68: Created label is not automatically selected.When
createLabelsucceeds, it returns the new label but doesn't add it to the selection. Callers must explicitly toggle the label after creation. If this is intentional for flexibility, consider documenting it in the return type or adding an optionalautoSelectparameter.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/posts/src/hooks/use-label-picker.ts` around lines 60 - 68, createLabel currently returns the newly created Label but doesn't add it to the hook's selection; change createLabel(name: string) to accept an optional autoSelect boolean (default true), and after awaiting createLabelMutation and obtaining newLabel, if newLabel and autoSelect is true then add it to the hook's selection using the hook's selection updater (e.g., call toggleLabel(newLabel.id) or push into selectedLabels via the existing setter); keep the duplicate check (isDuplicateName) and unchanged createLabelMutation usage, and allow callers to pass autoSelect=false when they don't want automatic selection.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@apps/posts/src/hooks/use-label-picker.ts`:
- Around line 26-27: The label query in use-label-picker uses a hard-coded limit
of '100' which truncates results; update the call to
useBrowseLabels({searchParams: {limit: 'all'}}) (or otherwise pass a
non-truncating value) so the full label set is returned, then keep using
labelsData?.labels in the useMemo; ensure any downstream duplicate-name checks
and edit/delete flows in useLabelPicker operate on the full labels array
returned by useBrowseLabels.
In `@apps/posts/src/views/members/components/members-actions.tsx`:
- Around line 57-76: The current handleAddLabel (and similarly
handleRemoveLabel) performs sequential per-label bulkEditAsync calls so a
partial failure yields only a generic error; change these handlers to run
parallel requests with Promise.allSettled for the array of labelIds (calling
bulkEditAsync with the same payload per labelId), then inspect the settled
results to compute how many succeeded vs failed and which labelIds failed, close
the modal if at least one succeeded, and show a toast summarizing successes and
failures (e.g., "X labels added, Y failed" with optional details) instead of the
single generic error; update references to handleAddLabel, handleRemoveLabel and
bulkEditAsync accordingly.
---
Nitpick comments:
In `@apps/posts/src/hooks/use-label-picker.ts`:
- Around line 60-68: createLabel currently returns the newly created Label but
doesn't add it to the hook's selection; change createLabel(name: string) to
accept an optional autoSelect boolean (default true), and after awaiting
createLabelMutation and obtaining newLabel, if newLabel and autoSelect is true
then add it to the hook's selection using the hook's selection updater (e.g.,
call toggleLabel(newLabel.id) or push into selectedLabels via the existing
setter); keep the duplicate check (isDuplicateName) and unchanged
createLabelMutation usage, and allow callers to pass autoSelect=false when they
don't want automatic selection.
In
`@apps/posts/src/views/members/components/bulk-action-modals/add-label-modal.tsx`:
- Around line 42-49: The handleConfirm function is recreated every render; wrap
it with React.useCallback to memoize it like handleOpenChange — reference the
handleConfirm function that builds labelIds from picker.labels and selectedSlugs
and calls onConfirm(labelIds). Use useCallback with a dependency array including
picker.labels, selectedSlugs, and onConfirm so the callback updates when inputs
change, and replace the original handleConfirm with this memoized version.
In
`@apps/posts/src/views/members/components/bulk-action-modals/remove-label-modal.tsx`:
- Around line 70-77: Memoize the RemoveLabelModal's handler by wrapping
handleConfirm in React's useCallback to avoid recreating the function on each
render; import useCallback and replace the current const handleConfirm = () => {
... } with const handleConfirm = useCallback(() => { const labelIds =
picker.labels.filter(l => selectedSlugs.includes(l.slug)).map(l => l.id); if
(labelIds.length > 0) onConfirm(labelIds); }, [picker.labels, selectedSlugs,
onConfirm]) so it mirrors AddLabelModal's behavior and uses picker.labels,
selectedSlugs and onConfirm as dependencies.
In `@apps/posts/src/views/members/components/members-actions.tsx`:
- Around line 99-114: handleUnsubscribe is using Promise .then()/.catch() while
sibling handlers (handleAddLabel, handleRemoveLabel) use async/await; convert
handleUnsubscribe to an async function and use try/catch to await bulkEditAsync,
then call setShowUnsubscribeModal(false) and toast.success on success and
toast.error with description on failure to match the pattern used by
handleAddLabel/handleRemoveLabel and keep error handling consistent for
bulkEditAsync.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between 7cccd83d4f4462b1fdeceddecaa5b10e6cabe328 and 60d19ea.
📒 Files selected for processing (11)
apps/admin-x-framework/src/api/labels.tsapps/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/hooks/use-label-picker.tsapps/posts/src/views/members/components/bulk-action-modals/add-label-modal.tsxapps/posts/src/views/members/components/bulk-action-modals/remove-label-modal.tsxapps/posts/src/views/members/components/members-actions.tsxapps/posts/src/views/members/hooks/use-members-filter-config.tsxapps/posts/src/views/members/hooks/use-members-filter-state.tsghost/admin/app/services/state-bridge.js
🚧 Files skipped from review as they are similar to previous changes (4)
- apps/posts/src/views/members/hooks/use-members-filter-state.ts
- apps/admin-x-framework/src/api/labels.ts
- apps/posts/src/components/label-picker/label-picker.tsx
- apps/posts/src/components/label-picker/label-filter-renderer.tsx
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 3 potential issues.
Bugbot Free Tier Details
Your team is on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle for each member of your team.
To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.
Autofix Details
Bugbot Autofix prepared fixes for all 3 issues found in the latest run.
- ✅ Fixed: RemoveLabelModal fetches all members unbounded
- Removed the member-browse query from RemoveLabelModal so it no longer fetches all filtered members with labels before rendering.
- ✅ Fixed: Bulk actions NQL guard removed allowing all-member operations
- Reintroduced the NQL UI guard for bulk action controls/modals and made bulk edit/delete handlers require a filter instead of using all=true.
- ✅ Fixed: Sequential bulk label calls cause partial application on failure
- Changed multi-label add/remove flows to settle all requests and report partial success/failure counts instead of a misleading generic error on mid-sequence failure.
Or push these changes by commenting:
@cursor push 54f52a1a8e
Preview (54f52a1a8e)
diff --git a/apps/posts/src/views/members/components/bulk-action-modals/remove-label-modal.tsx b/apps/posts/src/views/members/components/bulk-action-modals/remove-label-modal.tsx
--- a/apps/posts/src/views/members/components/bulk-action-modals/remove-label-modal.tsx
+++ b/apps/posts/src/views/members/components/bulk-action-modals/remove-label-modal.tsx
@@ -7,14 +7,12 @@
DialogTitle
} from '@tryghost/shade';
import {LabelPicker} from '@src/components/label-picker';
-import {useBrowseMembers} from '@tryghost/admin-x-framework/api/members';
-import {useCallback, useMemo, useState} from 'react';
+import {useCallback, useState} from 'react';
import {useLabelPicker} from '@src/hooks/use-label-picker';
interface RemoveLabelModalProps {
open: boolean;
memberCount: number;
- nql?: string;
onOpenChange: (open: boolean) => void;
onConfirm: (labelIds: string[]) => void;
isLoading?: boolean;
@@ -23,43 +21,17 @@
export function RemoveLabelModal({
open,
memberCount,
- nql,
onOpenChange,
onConfirm,
isLoading = false
}: RemoveLabelModalProps) {
const [selectedSlugs, setSelectedSlugs] = useState<string[]>([]);
- // Fetch members matching the current filter to find which labels they have
- const {data: membersData, isLoading: isMembersLoading} = useBrowseMembers({
- searchParams: {
- ...(nql ? {filter: nql} : {}),
- include: 'labels',
- limit: 'all',
- fields: 'id'
- },
- enabled: open
- });
-
- // Extract unique label slugs from the filtered members
- const memberLabelSlugs = useMemo(() => {
- const slugs = new Set<string>();
- for (const member of membersData?.members || []) {
- for (const label of member.labels || []) {
- slugs.add(label.slug);
- }
- }
- return slugs;
- }, [membersData]);
-
const picker = useLabelPicker({
selectedSlugs,
onSelectionChange: setSelectedSlugs
});
- // Filter labels to only those assigned to the filtered members
- const availableLabels = useMemo(() => picker.labels.filter(l => memberLabelSlugs.has(l.slug)), [picker.labels, memberLabelSlugs]);
-
const handleOpenChange = useCallback((isOpen: boolean) => {
if (!isOpen) {
setSelectedSlugs([]);
@@ -87,8 +59,8 @@
<LabelPicker
isDuplicateName={picker.isDuplicateName}
- isLoading={picker.isLoading || isMembersLoading}
- labels={availableLabels}
+ isLoading={picker.isLoading}
+ labels={picker.labels}
selectedSlugs={picker.selectedSlugs}
onDelete={picker.deleteLabel}
onEdit={picker.editLabel}
diff --git a/apps/posts/src/views/members/components/members-actions.tsx b/apps/posts/src/views/members/components/members-actions.tsx
--- a/apps/posts/src/views/members/components/members-actions.tsx
+++ b/apps/posts/src/views/members/components/members-actions.tsx
@@ -55,20 +55,33 @@
}, [nql]);
const handleAddLabel = useCallback(async (labelIds: string[]) => {
- try {
- for (const labelId of labelIds) {
- await bulkEditAsync({
- filter: nql || '',
- all: !nql,
- action: {
- type: 'addLabel',
- meta: {label: {id: labelId}}
- }
- });
+ if (!nql) {
+ return;
+ }
+
+ const results = await Promise.allSettled(labelIds.map(labelId => bulkEditAsync({
+ filter: nql,
+ all: false,
+ action: {
+ type: 'addLabel',
+ meta: {label: {id: labelId}}
}
+ })));
+
+ const failedCount = results.filter(result => result.status === 'rejected').length;
+ const succeededCount = labelIds.length - failedCount;
+
+ if (failedCount === 0) {
setShowAddLabelModal(false);
toast.success(labelIds.length > 1 ? 'Labels added successfully' : 'Label added successfully');
- } catch {
+ return;
+ }
+
+ if (succeededCount > 0) {
+ toast.error('Some labels could not be added', {
+ description: `${succeededCount} added, ${failedCount} failed. Please try again.`
+ });
+ } else {
toast.error('Failed to add label', {
description: 'There was a problem applying this label. Please try again.'
});
@@ -76,20 +89,33 @@
}, [bulkEditAsync, nql]);
const handleRemoveLabel = useCallback(async (labelIds: string[]) => {
- try {
- for (const labelId of labelIds) {
- await bulkEditAsync({
- filter: nql || '',
- all: !nql,
- action: {
- type: 'removeLabel',
- meta: {label: {id: labelId}}
- }
- });
+ if (!nql) {
+ return;
+ }
+
+ const results = await Promise.allSettled(labelIds.map(labelId => bulkEditAsync({
+ filter: nql,
+ all: false,
+ action: {
+ type: 'removeLabel',
+ meta: {label: {id: labelId}}
}
+ })));
+
+ const failedCount = results.filter(result => result.status === 'rejected').length;
+ const succeededCount = labelIds.length - failedCount;
+
+ if (failedCount === 0) {
setShowRemoveLabelModal(false);
toast.success(labelIds.length > 1 ? 'Labels removed successfully' : 'Label removed successfully');
- } catch {
+ return;
+ }
+
+ if (succeededCount > 0) {
+ toast.error('Some labels could not be removed', {
+ description: `${succeededCount} removed, ${failedCount} failed. Please try again.`
+ });
+ } else {
toast.error('Failed to remove label', {
description: 'There was a problem removing this label. Please try again.'
});
@@ -97,9 +123,13 @@
}, [bulkEditAsync, nql]);
const handleUnsubscribe = useCallback(() => {
+ if (!nql) {
+ return;
+ }
+
bulkEditAsync({
- filter: nql || '',
- all: !nql,
+ filter: nql,
+ all: false,
action: {
type: 'unsubscribe'
}
@@ -114,9 +144,13 @@
}, [bulkEditAsync, nql]);
const handleDelete = useCallback(() => {
+ if (!nql) {
+ return;
+ }
+
bulkDelete({
- filter: nql || '',
- all: !nql
+ filter: nql,
+ all: false
}, {
onSuccess: () => {
setShowDeleteModal(false);
@@ -159,28 +193,32 @@
: 'Export all members'}
</DropdownMenuItem>
- <DropdownMenuSeparator />
- <DropdownMenuItem onClick={() => setShowAddLabelModal(true)}>
- <LucideIcon.Tags className="mr-2 size-4" />
- Add label to {memberCount.toLocaleString()} {memberCount === 1 ? 'member' : 'members'}
- </DropdownMenuItem>
- <DropdownMenuItem onClick={() => setShowRemoveLabelModal(true)}>
- <LucideIcon.Tag className="mr-2 size-4" />
- Remove label from {memberCount.toLocaleString()} {memberCount === 1 ? 'member' : 'members'}
- </DropdownMenuItem>
- <DropdownMenuItem onClick={() => setShowUnsubscribeModal(true)}>
- <LucideIcon.MailX className="mr-2 size-4" />
- Unsubscribe {memberCount.toLocaleString()} {memberCount === 1 ? 'member' : 'members'}
- </DropdownMenuItem>
- <DropdownMenuSeparator />
- <DropdownMenuItem
- className="text-destructive focus:text-destructive"
- disabled={!canBulkDelete}
- onClick={() => setShowDeleteModal(true)}
- >
- <LucideIcon.Trash2 className="mr-2 size-4" />
- Delete {memberCount.toLocaleString()} {memberCount === 1 ? 'member' : 'members'}
- </DropdownMenuItem>
+ {nql && (
+ <>
+ <DropdownMenuSeparator />
+ <DropdownMenuItem onClick={() => setShowAddLabelModal(true)}>
+ <LucideIcon.Tags className="mr-2 size-4" />
+ Add label to {memberCount.toLocaleString()} {memberCount === 1 ? 'member' : 'members'}
+ </DropdownMenuItem>
+ <DropdownMenuItem onClick={() => setShowRemoveLabelModal(true)}>
+ <LucideIcon.Tag className="mr-2 size-4" />
+ Remove label from {memberCount.toLocaleString()} {memberCount === 1 ? 'member' : 'members'}
+ </DropdownMenuItem>
+ <DropdownMenuItem onClick={() => setShowUnsubscribeModal(true)}>
+ <LucideIcon.MailX className="mr-2 size-4" />
+ Unsubscribe {memberCount.toLocaleString()} {memberCount === 1 ? 'member' : 'members'}
+ </DropdownMenuItem>
+ <DropdownMenuSeparator />
+ <DropdownMenuItem
+ className="text-destructive focus:text-destructive"
+ disabled={!canBulkDelete}
+ onClick={() => setShowDeleteModal(true)}
+ >
+ <LucideIcon.Trash2 className="mr-2 size-4" />
+ Delete {memberCount.toLocaleString()} {memberCount === 1 ? 'member' : 'members'}
+ </DropdownMenuItem>
+ </>
+ )}
</DropdownMenuContent>
</DropdownMenu>
@@ -192,36 +230,39 @@
</Button>
{/* Modals */}
- <AddLabelModal
- isLoading={isBulkEditing}
- memberCount={memberCount}
- open={showAddLabelModal}
- onConfirm={handleAddLabel}
- onOpenChange={setShowAddLabelModal}
- />
- <RemoveLabelModal
- isLoading={isBulkEditing}
- memberCount={memberCount}
- nql={nql}
- open={showRemoveLabelModal}
- onConfirm={handleRemoveLabel}
- onOpenChange={setShowRemoveLabelModal}
- />
- <UnsubscribeModal
- isLoading={isBulkEditing}
- memberCount={memberCount}
- open={showUnsubscribeModal}
- onConfirm={handleUnsubscribe}
- onOpenChange={setShowUnsubscribeModal}
- />
- <DeleteModal
- isLoading={isBulkDeleting}
- memberCount={memberCount}
- open={showDeleteModal}
- onConfirm={handleDelete}
- onExportBackup={handleExportBackup}
- onOpenChange={setShowDeleteModal}
- />
+ {nql && (
+ <>
+ <AddLabelModal
+ isLoading={isBulkEditing}
+ memberCount={memberCount}
+ open={showAddLabelModal}
+ onConfirm={handleAddLabel}
+ onOpenChange={setShowAddLabelModal}
+ />
+ <RemoveLabelModal
+ isLoading={isBulkEditing}
+ memberCount={memberCount}
+ open={showRemoveLabelModal}
+ onConfirm={handleRemoveLabel}
+ onOpenChange={setShowRemoveLabelModal}
+ />
+ <UnsubscribeModal
+ isLoading={isBulkEditing}
+ memberCount={memberCount}
+ open={showUnsubscribeModal}
+ onConfirm={handleUnsubscribe}
+ onOpenChange={setShowUnsubscribeModal}
+ />
+ <DeleteModal
+ isLoading={isBulkDeleting}
+ memberCount={memberCount}
+ open={showDeleteModal}
+ onConfirm={handleDelete}
+ onExportBackup={handleExportBackup}
+ onOpenChange={setShowDeleteModal}
+ />
+ </>
+ )}
</>
);
};
apps/posts/src/views/members/components/bulk-action-modals/remove-label-modal.tsx
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
apps/posts/src/components/label-picker/label-picker.tsx (1)
43-43:⚠️ Potential issue | 🟡 MinorDelete UI can become a no-op when
onDeleteis not provided.Line 231 enables edit mode based on
onEdit, and Lines 107-136 always render delete controls inEditRow. IfonDeleteis absent, Lines 250-253 skip deletion, so users can enter a delete flow that cannot complete.Suggested fix
interface EditRowProps { label: Label; onSave: (id: string, name: string) => Promise<void>; onCancel: () => void; - onDelete: (id: string) => Promise<void>; + onDelete?: (id: string) => Promise<void>; isDuplicateName?: (name: string, excludeId?: string) => boolean; } -const EditRow: React.FC<EditRowProps> = ({label, onSave, onCancel, onDelete, isDuplicateName}) => { +const EditRow: React.FC<EditRowProps> = ({label, onSave, onCancel, onDelete, isDuplicateName}) => { @@ - const handleDelete = async () => { - await onDelete(label.id); - }; + const handleDelete = async () => { + if (!onDelete) { + return; + } + await onDelete(label.id); + }; @@ - {showDeleteConfirm ? ( + {onDelete && showDeleteConfirm ? ( @@ - ) : ( + ) : ( <div className="flex items-center"> - <Button - className="h-6 gap-1 px-1.5 text-xs text-red hover:bg-red/5 hover:text-red" - size="sm" - variant="ghost" - onClick={() => setShowDeleteConfirm(true)} - > - Delete - </Button> + {onDelete && ( + <Button + className="h-6 gap-1 px-1.5 text-xs text-red hover:bg-red/5 hover:text-red" + size="sm" + variant="ghost" + onClick={() => setShowDeleteConfirm(true)} + > + Delete + </Button> + )}Also applies to: 107-136, 231-231, 250-253, 262-268
🤖 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-picker.tsx` at line 43, The delete UI is rendered even when onDelete is not provided, causing a no-op delete flow; update the LabelPicker props and rendering guards so delete controls and delete-mode activation only appear when onDelete exists: make onDelete optional in the prop type and add conditional checks around the EditRow rendering, the edit-mode toggle (where onEdit is used), and the delete action handlers (e.g., handleDelete/confirmDelete) so they early-return or disable UI when onDelete is undefined; ensure any code paths at the spots referenced (EditRow, the edit-mode enablement, and the deletion confirmation flow) check props.onDelete before showing buttons or attempting Promise<void> calls.
🤖 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-picker.tsx`:
- Around line 69-77: The async callbacks (e.g., handleSave) call await on
mutation props without error handling; wrap each await (handleSave and the other
async handlers at the mentioned ranges) in try/catch, set a component-level
error state (use setError) with the caught error message, and avoid calling
onCancel/closing the UI until the mutation succeeds; optionally set and clear a
loading flag around the await to disable inputs while the mutation runs.
- Around line 178-191: The icon-only edit button lacks an accessible name;
update the button in the LabelPicker component to include an aria-label (and
optional title) so screen readers can announce the action — e.g., add
aria-label={`Edit label`} (or a dynamic string if you want to reflect selection
like `Edit selected label`) to the <button> that uses onEditClick, keeping the
existing onClick behavior and visual icons (LucideIcon.Check, LucideIcon.Pencil)
unchanged.
---
Duplicate comments:
In `@apps/posts/src/components/label-picker/label-picker.tsx`:
- Line 43: The delete UI is rendered even when onDelete is not provided, causing
a no-op delete flow; update the LabelPicker props and rendering guards so delete
controls and delete-mode activation only appear when onDelete exists: make
onDelete optional in the prop type and add conditional checks around the EditRow
rendering, the edit-mode toggle (where onEdit is used), and the delete action
handlers (e.g., handleDelete/confirmDelete) so they early-return or disable UI
when onDelete is undefined; ensure any code paths at the spots referenced
(EditRow, the edit-mode enablement, and the deletion confirmation flow) check
props.onDelete before showing buttons or attempting Promise<void> calls.
weylandswart
requested changes
Mar 3, 2026
Contributor
There was a problem hiding this comment.
Made tweaks to what I could in 39bddb5.
A few more changes that I think are worth making.
- Clicking on save when in the label edit state seems to save the value while remaining in the editing state
- It’s not currently possible to edit a label when initially selecting a filter of type label (Before the filter is added)
- Once a label filter has been added, the label dropdown is not aligned with the left side of the dropdown trigger (Label name or x labels).
- The combobox should use Shade's Input or Select styles (Height, focus state, borders, colours, etc.)
- I get an error when trying to add or remove a label
Error: A mutation invalidating MembersResponseType succeeded in React Admin but there is no mapping to an Ember type. Add one to emberDataTypeMapping at E.onInvalidate (state-bridge.js:147:1) at onInvalidate (main.tsx:29:35) at Object.onSuccess (hooks.ts:198:13) at async members-actions.tsx:60:17 - Does the empty header part of the dropdown have to be there when the option to create a new label is shown? It looks a bit broken to me.
ref https://linear.app/tryghost/issue/BER-3342/ - Added loading states to inline edit/delete (save and delete buttons show progress, controls disabled during operations, failures keep the edit row open for retry) - Fixed popover alignment in filter cells by dynamically measuring the offset between trigger and parent container instead of hardcoding - Fixed empty space above "Create" option by conditionally rendering CommandGroup only when labels exist - Removed unnecessary comments from filter config
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
apps/posts/src/components/label-picker/label-picker.tsx (2)
255-263:⚠️ Potential issue | 🟠 MajorHandle create mutation failures explicitly.
The create path awaits
onCreatewithouttry/catch; rejected mutations can surface as unhandled UI errors and provide no retry feedback in-place.Proposed fix
const LabelListItems: React.FC<LabelListItemsProps> = ({ @@ }) => { const [editingLabelId, setEditingLabelId] = useState<string | null>(null); + const [createError, setCreateError] = useState(''); @@ const handleCreate = async () => { if (onCreate) { - const newLabel = await onCreate(search.trim()); - if (newLabel) { - onToggle(newLabel.slug); - } - onSearchClear?.(); + try { + setCreateError(''); + const newLabel = await onCreate(search.trim()); + if (newLabel) { + onToggle(newLabel.slug); + onSearchClear?.(); + } + } catch { + setCreateError('Failed to create label'); + } } }; @@ {showCreate && ( <CommandGroup className="[&_[cmdk-group-heading]]:hidden"> + {createError && <div className="px-2 py-1 text-xs text-destructive">{createError}</div>} <CommandItem disabled={isCreating} onSelect={handleCreate} >Also applies to: 310-313
🤖 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-picker.tsx` around lines 255 - 263, The create flow in handleCreate calls the async onCreate without error handling; wrap the await onCreate(search.trim()) call in a try/catch inside handleCreate (and the other create handler at the similar location) so rejected mutations are caught, log or surface the error (e.g., call an optional onCreateError/onError callback or processLogger/console.error), and only call onToggle(newLabel.slug) and onSearchClear() when creation succeeds; ensure the catch block prevents unhandled promise rejections and provides a way for the UI to display or retry the failed creation.
38-43:⚠️ Potential issue | 🟠 MajorGuard delete UI/flow when delete capability is not provided.
showEditis enabled fromonEditalone, butEditRowalways exposes delete actions. WhenonDeleteis absent upstream, the delete confirmation path can enter a busy/no-op state instead of completing cleanly.Proposed fix
interface EditRowProps { label: Label; onSave: (id: string, name: string) => Promise<void>; onCancel: () => void; - onDelete: (id: string) => Promise<void>; + onDelete?: (id: string) => Promise<void>; isDuplicateName?: (name: string, excludeId?: string) => boolean; } const EditRow: React.FC<EditRowProps> = ({label, onSave, onCancel, onDelete, isDuplicateName}) => { + const canDelete = !!onDelete; @@ const handleDelete = async () => { + if (!onDelete) { + return; + } setIsDeleting(true); try { await onDelete(label.id); } catch { setIsDeleting(false); setShowDeleteConfirm(false); } }; @@ - {showDeleteConfirm ? ( + {canDelete && showDeleteConfirm ? ( @@ - ) : ( + ) : ( <div className="flex items-center"> - <Button - className="h-6 gap-1 px-1.5 text-xs text-red hover:bg-red/5 hover:text-red" - disabled={isBusy} - size="sm" - variant="ghost" - onClick={() => setShowDeleteConfirm(true)} - > - Delete - </Button> + {canDelete && ( + <Button + className="h-6 gap-1 px-1.5 text-xs text-red hover:bg-red/5 hover:text-red" + disabled={isBusy} + size="sm" + variant="ghost" + onClick={() => setShowDeleteConfirm(true)} + > + Delete + </Button> + )} @@ - const showEdit = !!onEdit; + const showEdit = !!onEdit;Also applies to: 147-155, 253-254, 271-275, 287-293
🤖 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-picker.tsx` around lines 38 - 43, EditRow exposes delete UI even when onDelete is not provided, causing a stuck/no-op delete flow; update the EditRow component and any handlers invoked from onEdit/showEdit to guard deletion by checking props.onDelete (or the onDelete prop) before rendering delete buttons/confirmations and before calling it. Specifically, wrap the delete button/confirmation UI in a conditional (e.g., if (onDelete)) and make the delete handler short-circuit when onDelete is undefined (return early and close/cancel the edit state instead of setting busy). Ensure EditRowProps usage (and any callers relying on showEdit/onEdit) does not assume delete exists and that all paths that set deleting/busy state are only entered when onDelete is present.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@apps/posts/src/components/label-picker/label-picker.tsx`:
- Around line 255-263: The create flow in handleCreate calls the async onCreate
without error handling; wrap the await onCreate(search.trim()) call in a
try/catch inside handleCreate (and the other create handler at the similar
location) so rejected mutations are caught, log or surface the error (e.g., call
an optional onCreateError/onError callback or processLogger/console.error), and
only call onToggle(newLabel.slug) and onSearchClear() when creation succeeds;
ensure the catch block prevents unhandled promise rejections and provides a way
for the UI to display or retry the failed creation.
- Around line 38-43: EditRow exposes delete UI even when onDelete is not
provided, causing a stuck/no-op delete flow; update the EditRow component and
any handlers invoked from onEdit/showEdit to guard deletion by checking
props.onDelete (or the onDelete prop) before rendering delete
buttons/confirmations and before calling it. Specifically, wrap the delete
button/confirmation UI in a conditional (e.g., if (onDelete)) and make the
delete handler short-circuit when onDelete is undefined (return early and
close/cancel the edit state instead of setting busy). Ensure EditRowProps usage
(and any callers relying on showEdit/onEdit) does not assume delete exists and
that all paths that set deleting/busy state are only entered when onDelete is
present.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/posts/src/components/label-picker/label-picker.tsxapps/posts/src/views/members/hooks/use-members-filter-config.tsx
Member
🤖 Velo CI Failure AnalysisClassification: 🟠 SOFT FAIL
|
ref https://linear.app/tryghost/issue/BER-3342/ When editing a label name, the server may regenerate its slug. The filter selection state (URL params) stores slugs, so it needs to be updated when the slug changes — otherwise the renamed label appears deselected. Now editLabel checks the returned slug and swaps it in the selection if it differs from the original.
weylandswart
requested changes
Mar 4, 2026
Contributor
weylandswart
left a comment
weylandswart
left a comment
There was a problem hiding this comment.
- Save isn’t exiting the editing state for a label (The changes are persisted, though)
- The dropdown when bulk adding a label (And likely removing) isn’t scrollable
- I think, UX wise, each new label addition should close the dropdown
- The add/remove label bulk actions work, but I stay in the modal and I see a "Failed to remove label. There was a problem removing this label. Please try again.” toast.
- Labels are not editable when initially adding a label filter from the top right filter dropdown (There's no pencil icon on hover)
- If you add a label (From bulk), the existing searched-for text remains in the input
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.
ref https://linear.app/tryghost/issue/BER-3342/ Reverted workarounds from 2e67736 that were masking a non-issue (the Ember state bridge mappings are correct and don't throw). Restored proper error handling in bulk actions, simplified createLabel, and reverted EditRow to try/catch with loading state. Fixed the actual scroll bug: ComboboxPicker (used in add/remove label modals) no longer uses a Radix Popover portal. The portal placed the dropdown outside the Dialog's DOM subtree, causing Dialog's scroll-lock to block scrolling in the label list. Now renders the dropdown inline as an absolutely-positioned div within the Dialog's content.
weylandswart
approved these changes
Mar 4, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The members list filter and bulk-action modals used simple Select dropdowns for choosing labels, which limited users to picking one label at a time with no way to manage labels without leaving the context. This replaces those dropdowns with a searchable, multi-select label picker built on Popover + Command (cmdk) from Shade — the same proven pattern used by the existing filter options popovers.
The picker is driven by a
useLabelPickerhook that encapsulates all label CRUD operations (browse, create, edit, delete) via new mutations added to admin-x-framework. The component itself adapts to three contexts through prop presence rather than an explicit mode prop: the filter value cell renders as an inline popover with applied-label pills above the search, the add-label bulk modal includes a "Create" option for new labels, and the remove-label bulk modal scopes available labels to those actually assigned to the filtered members. In all contexts, labels can be renamed or deleted inline via edit/delete buttons that appear on hover, with confirmation for destructive actions.The Ember state bridge was updated with
LabelsResponseTypeandMembersResponseTypemappings so that mutation cache updates propagate correctly between the React and Ember query caches. The filter state serialization was also adjusted to handle multiselect fields with empty values, so a label filter row persists in the URL even before any labels are selected.ref https://linear.app/tryghost/issue/BER-3342/
Note
Medium Risk
Touches multiple user-facing flows (filters and bulk member operations) and introduces new label mutation paths with cache-update logic; issues could cause incorrect selections or label assignments/removals.
Overview
Replaces the members label dropdown UX with a new searchable, multi-select
LabelPickerused in both the members list label filter (inline popover renderer) and bulk add/remove label modals, including inline rename/delete and optional create-from-search.Adds label CRUD mutations to
admin-x-framework(useCreateLabel,useEditLabel,useDeleteLabel) with query-cache updates, introduces a shareduseLabelPickerhook to coordinate selection + mutations (including slug-change handling), and updates bulk member actions to apply multiple labels and to scope removable labels to those present on the filtered members.Improves filter URL handling by serializing empty multiselect values so newly-added filter rows persist, and updates the Ember state bridge mappings for
LabelsResponseType/MembersResponseTypeto support React-side cache update semantics.Written by Cursor Bugbot for commit c96b765. This will update automatically on new commits. Configure here.