Consolidated filter search into unified useFilterSearch hook#26958
Open
kevinansfield wants to merge 7 commits intomainfrom
Open
Consolidated filter search into unified useFilterSearch hook#26958kevinansfield wants to merge 7 commits intomainfrom
kevinansfield wants to merge 7 commits intomainfrom
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 infinite and single-resource query hooks across APIs (e.g. 🚥 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)
Comment |
kevinansfield
force-pushed
the
filter-search-refactor
branch
from
6684148 to
53f1e9b
Compare
kevinansfield
commented
Mar 25, 2026
Comment on lines
80
to
83
| export const useBrowseOffersById = createQueryWithId<OffersResponseType>({ | ||
| dataType, | ||
| path: id => `/offers/${id}/` | ||
| }); |
Member
Author
There was a problem hiding this comment.
Aside: The useBrowseOffersById naming here is an inconsistency, all other APIs use get{Resource} with createQueryWithId
kevinansfield
force-pushed
the
filter-search-refactor
branch
from
53f1e9b to
fb3ae08
Compare
kevinansfield
commented
Mar 25, 2026
apps/posts/src/views/members/hooks/use-email-resource-search.ts
Outdated
Show resolved
Hide resolved
kevinansfield
commented
Mar 25, 2026
kevinansfield
commented
Mar 25, 2026
kevinansfield
commented
Mar 25, 2026
kevinansfield
force-pushed
the
filter-search-refactor
branch
2 times, most recently
from
ffb71ab to
91cf554
Compare
ref https://linear.app/ghost/issue/BER-3334/ - Shared `useFilterSearch` hook handles local/server search mode detection, debouncing, infinite scroll, and async resolution of selected values not on the current page - Comments and members filter views use `useFilterSearch` instead of separate scattered implementations - `createQueryWithId`-based getters (`getLabelBySlug`, `getNewsletter`, etc.) in admin-x-framework resolve display names for filter values outside the paginated window - `useLabelPicker` caches selected labels independently (mirroring shade's `cachedSelectedOptions` pattern) so label renames and search/remount cycles don't lose resolved names - Fixed `useDeleteLabel`'s cache updater crashing on infinite query data - Disabled cmdk's `shouldFilter` on filter dropdowns that use `onSearchChange` — cmdk v1 reorders DOM elements via `appendChild` during scored search and never restores the original order when the search is cleared
kevinansfield
force-pushed
the
filter-search-refactor
branch
from
91cf554 to
751cf4a
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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
apps/posts/src/views/members/components/bulk-action-modals/add-label-modal.tsx (1)
60-71:⚠️ Potential issue | 🟠 MajorBuild the confirmed ids from
picker.selectedLabels.This component now renders selection from
picker.selectedLabels, buthandleConfirmstill resolves ids frompicker.labels. Once the visible label list changes, the modal can submit only a subset of what the user still sees as selected.Suggested fix
const handleConfirm = () => { - const labelIds = picker.labels - .filter(l => selectedSlugs.includes(l.slug)) - .map(l => l.id); + const labelIds = picker.selectedLabels.map(l => l.id); if (labelIds.length > 0) { onConfirm(labelIds); } };🤖 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 60 - 71, handleConfirm currently builds the confirmed id list from picker.labels which may omit items when the visible labels change; update handleConfirm to derive confirmedIds from picker.selectedLabels instead (use picker.selectedLabels.map(l => l.id) or equivalent) and keep any existing filtering/validation logic. Locate the handleConfirm function in add-label-modal.tsx and replace references to picker.labels when constructing the submission payload so it uses picker.selectedLabels (and ensure it handles empty/null safely).apps/posts/src/views/members/components/bulk-action-modals/remove-label-modal.tsx (1)
91-99:⚠️ Potential issue | 🟠 MajorSubmit from
picker.selectedLabels, not the current option page.The picker now preserves selections outside the current result set, but
handleConfirmstill derives ids frompicker.labels. If the user selects a label and then changes the search or the label is renamed off the current page, this modal can silently drop that selection on confirm.Suggested fix
const handleConfirm = () => { - const labelIds = picker.labels - .filter(l => selectedSlugs.includes(l.slug)) - .map(l => l.id); + const labelIds = picker.selectedLabels.map(l => l.id); if (labelIds.length > 0) { onConfirm(labelIds); } };🤖 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 91 - 99, handleConfirm currently builds the submission list from picker.labels which only contains the currently loaded page, causing selections preserved in picker.selectedLabels to be dropped; update handleConfirm to derive ids (or full label objects) from picker.selectedLabels instead (or map selected ids to the corresponding label objects via availableLabels if you need full metadata). Locate handleConfirm in the modal component that renders LabelPicker and replace any usage of picker.labels for the final payload with picker.selectedLabels (or a stable lookup from availableLabels using picker.selectedLabels) so selections made off-page or after renames are honored.apps/posts/src/hooks/use-label-picker.ts (1)
124-127:⚠️ Potential issue | 🟡 MinorDuplicate name check may miss labels outside current search results.
isDuplicateNameonly checks againstlabels(the current filtered items fromlabelSearch.items). When searching, labels outside the current result set won't be checked, potentially allowing creation of duplicates.Consider using
labelSearch.allItemsor performing a server-side uniqueness check.🔧 Suggested fix using allItems
const isDuplicateName = useCallback((name: string, excludeId?: string) => { const normalized = name.trim().toLowerCase(); - return labels.some(l => l.name.toLowerCase() === normalized && l.id !== excludeId); - }, [labels]); + return labelSearch.allItems.some(l => l.name.toLowerCase() === normalized && l.id !== excludeId); + }, [labelSearch.allItems]);🤖 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 124 - 127, isDuplicateName currently checks only the filtered labels array (labels) and can miss duplicates outside the current search results; update isDuplicateName to check against the full set of known labels (e.g., labelSearch.allItems) instead of labels (or merge labels with labelSearch.allItems and dedupe) and keep the existing normalization + excludeId logic so comparisons use name.trim().toLowerCase() and ignore the excluded id; if server-side uniqueness is required, call the uniqueness endpoint from the same helper instead of relying solely on client-side arrays.
🧹 Nitpick comments (5)
apps/posts/test/unit/hooks/use-label-picker.test.ts (1)
49-203: Please add the search/remount regression case.This suite proves rename/delete updates, but it never drives
onSearchChangeor remounts the hook. The new behavior this refactor is protecting is preservingselectedLabelswhen the visible results change, so that regression can still slip through here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/posts/test/unit/hooks/use-label-picker.test.ts` around lines 49 - 203, Add a test that drives the onSearchChange flow and a remount to verify selectedLabels are preserved when visible results change: create a new it() under the existing suites that sets currentLabels with a selected item, renders useWrappedPicker(['slug']), then call result.current.onSearchChange('query that yields no results') (or simulate typing) and await the hook settling to assert selectedLabels still contains the previously selected label; next unmount and re-render the hook (remount) and assert selectedLabels persists; you can also reuse editLabel/deleteLabel in a variant where server.use updates currentLabels to return different GET results to simulate visibility changes. Target functions/props: useWrappedPicker, result.current.onSearchChange, result.current.selectedLabels, and use renderHookWithProviders to remount. Ensure queries use the same queryClient so cache/invalidation behavior matches the regression scenario.apps/posts/src/hooks/use-filter-search.ts (2)
197-202: Mutating ref during render can cause subtle bugs.Lines 197-202 mutate
resolvedForOptionsRef.currentdirectly during render. While refs are allowed to be read during render, writing to them during render can cause issues with React's concurrent features and may lead to torn reads.Consider moving this logic into a
useEffect:♻️ Safer pattern using useEffect
- // Append async-resolved item to the ref - if (resolvedRawItem) { - const resolvedValue = toOption(resolvedRawItem).value; - if (!resolvedForOptionsRef.current.some(existing => toOption(existing).value === resolvedValue)) { - resolvedForOptionsRef.current = [...resolvedForOptionsRef.current, resolvedRawItem]; - } - } + // Append async-resolved item to the ref + useEffect(() => { + if (resolvedRawItem) { + const resolvedValue = toOption(resolvedRawItem).value; + if (!resolvedForOptionsRef.current.some(existing => toOption(existing).value === resolvedValue)) { + resolvedForOptionsRef.current = [...resolvedForOptionsRef.current, resolvedRawItem]; + } + } + }, [resolvedRawItem, toOption]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/posts/src/hooks/use-filter-search.ts` around lines 197 - 202, The code mutates resolvedForOptionsRef.current during render (the block that checks resolvedRawItem, computes resolvedValue via toOption(resolvedRawItem).value, compares against resolvedForOptionsRef.current and appends resolvedRawItem), which can break concurrent rendering — move this mutation into a useEffect that depends on resolvedRawItem (and any values used by toOption) so the check-and-append runs after render; inside the effect compute resolvedValue = toOption(resolvedRawItem).value, bail if already present, and call set on resolvedForOptionsRef.current only from the effect to avoid in-render writes.
133-145: Ref assignment during render is intentional but fragile.The
baseAllItemsRefis assigned during render (lines 140-144) to capture the stable snapshot of non-search data. While this works, it relies on careful condition guards and could break with React concurrent features.Document this pattern or consider using
useEffectwith a guard to make the intent clearer.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/posts/src/hooks/use-filter-search.ts` around lines 133 - 145, The render-time assignment to baseAllItemsRef.current (in the use-filter-search hook) is fragile; move the mutation into a useEffect to make the intent explicit and safe with concurrent React: create a useEffect that depends on data, searchValue, debouncedSearch and useLocalSearch (or compute isNonSearchData inside the effect), and inside the effect check isNonSearchData and Array.isArray(data[dataKey]) before setting baseAllItemsRef.current to that array; alternatively add a concise inline comment above baseAllItemsRef explaining the deliberate render-time update if you prefer to keep it.apps/posts/src/components/label-picker/label-picker.tsx (1)
555-557: Consider adding accessible loading state.The spinner is purely visual. Screen reader users won't know labels are being fetched. Consider adding
aria-busyon the container or anaria-liveregion.♿ Optional accessibility improvement
<div className="flex items-center border-b px-3"> + <div aria-live="polite" className="sr-only"> + {isFetching ? 'Loading labels...' : ''} + </div> <LucideIcon.Search className="mr-2 size-4 shrink-0 opacity-50" />🤖 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 555 - 557, The visual spinner (LucideIcon.LoaderCircle) in the LabelPicker component doesn't expose loading state to assistive tech; update the container element in label-picker (the component rendering the {isFetching && <LucideIcon.LoaderCircle .../>} block) to include an accessible loading indicator by adding aria-busy={isFetching} and/or a visually-hidden live region (e.g., an element with role="status" and aria-live="polite" that announces "Loading labels..." when isFetching is true) so screen readers are informed while labels are being fetched.apps/posts/src/views/members/use-member-filter-fields.ts (1)
147-159: Clarify loading state logic.Line 157 sets
isLoadingto true only whenoptions.length === 0 && searchProps?.isLoading. This means the loading indicator won't show if there are stale options while fetching new ones. This might be intentional (to avoid flickering), but could also hide ongoing fetches from users.Consider whether
isFetchingshould also be exposed here for cases where you want to show a subtle loading indicator while refreshing.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/posts/src/views/members/use-member-filter-fields.ts` around lines 147 - 159, The current createSearchableFieldOverrides returns isLoading only when options are empty which hides refresh fetches; update the return to also surface an ongoing fetch flag by either (a) changing isLoading to reflect searchProps?.isLoading || (options.length === 0 && (searchProps?.isLoading ?? false)) or preferably (b) add a new property isFetching: searchProps?.isFetching ?? false (or similar name present on FilterSearchProps) so callers can show a subtle refresh indicator while options exist; modify createSearchableFieldOverrides (and FilterSearchProps type if needed) to return the additional isFetching property and ensure consumers use it for refresh 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/offers.ts`:
- Around line 63-67: The infinite hook useBrowseOffersInfinite is for a
non-paginated offers endpoint but does not explicitly stop pagination; update
the createInfiniteQuery call for useBrowseOffersInfinite to set
defaultNextPageParams to undefined so the generic getNextPageParam fallback
won't assume more pages. Locate the useBrowseOffersInfinite export and add
defaultNextPageParams: undefined alongside
dataType/path/defaultSearchParams/returnData to explicitly terminate pagination.
In `@apps/posts/src/views/members/components/members-filters.tsx`:
- Around line 131-148: postSearch and emailSearch don't receive the current
active ids so off-page selected posts/emails lose their labels; pass the active
ids into useFilterSearch the same way labels/tiers do so it can hydrate
backfilled options — for postSearch supply activeIds using postSearch (e.g.
activeIds: postSearch?.value ? [postSearch.value] : []) and for emailSearch
supply activeIds from emailSearch (or from the selection shape your code uses),
keeping useGetById: getPost as-is so the hook can fetch missing labels.
---
Outside diff comments:
In `@apps/posts/src/hooks/use-label-picker.ts`:
- Around line 124-127: isDuplicateName currently checks only the filtered labels
array (labels) and can miss duplicates outside the current search results;
update isDuplicateName to check against the full set of known labels (e.g.,
labelSearch.allItems) instead of labels (or merge labels with
labelSearch.allItems and dedupe) and keep the existing normalization + excludeId
logic so comparisons use name.trim().toLowerCase() and ignore the excluded id;
if server-side uniqueness is required, call the uniqueness endpoint from the
same helper instead of relying solely on client-side arrays.
In
`@apps/posts/src/views/members/components/bulk-action-modals/add-label-modal.tsx`:
- Around line 60-71: handleConfirm currently builds the confirmed id list from
picker.labels which may omit items when the visible labels change; update
handleConfirm to derive confirmedIds from picker.selectedLabels instead (use
picker.selectedLabels.map(l => l.id) or equivalent) and keep any existing
filtering/validation logic. Locate the handleConfirm function in
add-label-modal.tsx and replace references to picker.labels when constructing
the submission payload so it uses picker.selectedLabels (and ensure it handles
empty/null safely).
In
`@apps/posts/src/views/members/components/bulk-action-modals/remove-label-modal.tsx`:
- Around line 91-99: handleConfirm currently builds the submission list from
picker.labels which only contains the currently loaded page, causing selections
preserved in picker.selectedLabels to be dropped; update handleConfirm to derive
ids (or full label objects) from picker.selectedLabels instead (or map selected
ids to the corresponding label objects via availableLabels if you need full
metadata). Locate handleConfirm in the modal component that renders LabelPicker
and replace any usage of picker.labels for the final payload with
picker.selectedLabels (or a stable lookup from availableLabels using
picker.selectedLabels) so selections made off-page or after renames are honored.
---
Nitpick comments:
In `@apps/posts/src/components/label-picker/label-picker.tsx`:
- Around line 555-557: The visual spinner (LucideIcon.LoaderCircle) in the
LabelPicker component doesn't expose loading state to assistive tech; update the
container element in label-picker (the component rendering the {isFetching &&
<LucideIcon.LoaderCircle .../>} block) to include an accessible loading
indicator by adding aria-busy={isFetching} and/or a visually-hidden live region
(e.g., an element with role="status" and aria-live="polite" that announces
"Loading labels..." when isFetching is true) so screen readers are informed
while labels are being fetched.
In `@apps/posts/src/hooks/use-filter-search.ts`:
- Around line 197-202: The code mutates resolvedForOptionsRef.current during
render (the block that checks resolvedRawItem, computes resolvedValue via
toOption(resolvedRawItem).value, compares against resolvedForOptionsRef.current
and appends resolvedRawItem), which can break concurrent rendering — move this
mutation into a useEffect that depends on resolvedRawItem (and any values used
by toOption) so the check-and-append runs after render; inside the effect
compute resolvedValue = toOption(resolvedRawItem).value, bail if already
present, and call set on resolvedForOptionsRef.current only from the effect to
avoid in-render writes.
- Around line 133-145: The render-time assignment to baseAllItemsRef.current (in
the use-filter-search hook) is fragile; move the mutation into a useEffect to
make the intent explicit and safe with concurrent React: create a useEffect that
depends on data, searchValue, debouncedSearch and useLocalSearch (or compute
isNonSearchData inside the effect), and inside the effect check isNonSearchData
and Array.isArray(data[dataKey]) before setting baseAllItemsRef.current to that
array; alternatively add a concise inline comment above baseAllItemsRef
explaining the deliberate render-time update if you prefer to keep it.
In `@apps/posts/src/views/members/use-member-filter-fields.ts`:
- Around line 147-159: The current createSearchableFieldOverrides returns
isLoading only when options are empty which hides refresh fetches; update the
return to also surface an ongoing fetch flag by either (a) changing isLoading to
reflect searchProps?.isLoading || (options.length === 0 &&
(searchProps?.isLoading ?? false)) or preferably (b) add a new property
isFetching: searchProps?.isFetching ?? false (or similar name present on
FilterSearchProps) so callers can show a subtle refresh indicator while options
exist; modify createSearchableFieldOverrides (and FilterSearchProps type if
needed) to return the additional isFetching property and ensure consumers use it
for refresh UI.
In `@apps/posts/test/unit/hooks/use-label-picker.test.ts`:
- Around line 49-203: Add a test that drives the onSearchChange flow and a
remount to verify selectedLabels are preserved when visible results change:
create a new it() under the existing suites that sets currentLabels with a
selected item, renders useWrappedPicker(['slug']), then call
result.current.onSearchChange('query that yields no results') (or simulate
typing) and await the hook settling to assert selectedLabels still contains the
previously selected label; next unmount and re-render the hook (remount) and
assert selectedLabels persists; you can also reuse editLabel/deleteLabel in a
variant where server.use updates currentLabels to return different GET results
to simulate visibility changes. Target functions/props: useWrappedPicker,
result.current.onSearchChange, result.current.selectedLabels, and use
renderHookWithProviders to remount. Ensure queries use the same queryClient so
cache/invalidation behavior matches the regression scenario.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3358eb2b-0da9-4c31-904e-2e959880e04c
📒 Files selected for processing (33)
apps/admin-x-framework/src/api/labels.tsapps/admin-x-framework/src/api/newsletters.tsapps/admin-x-framework/src/api/offers.tsapps/admin-x-framework/src/api/pages.tsapps/admin-x-framework/src/api/posts.tsapps/admin-x-framework/src/api/tiers.tsapps/admin-x-framework/src/hooks.tsapps/admin-x-framework/src/utils/api/hooks.tsapps/posts/src/components/label-picker/label-filter-renderer.tsxapps/posts/src/components/label-picker/label-picker.tsxapps/posts/src/hooks/use-filter-search.tsapps/posts/src/hooks/use-label-picker.tsapps/posts/src/views/comments/comments.tsxapps/posts/src/views/comments/components/comments-filters.tsxapps/posts/src/views/comments/hooks/use-filter-options.tsapps/posts/src/views/comments/hooks/use-known-filter-values.tsapps/posts/src/views/comments/hooks/use-search-members.tsapps/posts/src/views/comments/hooks/use-search-posts.tsapps/posts/src/views/members/components/bulk-action-modals/add-label-modal.tsxapps/posts/src/views/members/components/bulk-action-modals/import-members/components/mapping-step.tsxapps/posts/src/views/members/components/bulk-action-modals/remove-label-modal.tsxapps/posts/src/views/members/components/members-filters.tsxapps/posts/src/views/members/hooks/resource-search-filter.test.tsapps/posts/src/views/members/hooks/resource-search-filter.tsapps/posts/src/views/members/hooks/use-post-resource-search.tsapps/posts/src/views/members/hooks/use-resource-search.tsapps/posts/src/views/members/use-member-filter-fields.test.tsapps/posts/src/views/members/use-member-filter-fields.tsapps/posts/test/unit/hooks/use-filter-options.test.tsxapps/posts/test/unit/hooks/use-filter-search.test.tsapps/posts/test/unit/hooks/use-label-picker.test.tsapps/posts/test/unit/views/members/import-members/modal.test.tsxapps/shade/src/components/ui/filters.tsx
💤 Files with no reviewable changes (9)
- apps/posts/src/views/comments/comments.tsx
- apps/posts/test/unit/hooks/use-filter-options.test.tsx
- apps/posts/src/views/comments/hooks/use-search-members.ts
- apps/posts/src/views/comments/hooks/use-known-filter-values.ts
- apps/posts/src/views/members/hooks/use-resource-search.ts
- apps/posts/src/views/members/hooks/resource-search-filter.ts
- apps/posts/src/views/members/hooks/resource-search-filter.test.ts
- apps/posts/src/views/comments/hooks/use-search-posts.ts
- apps/posts/src/views/comments/hooks/use-filter-options.ts
- Passed searchValue through FilterSearchProps so shade popovers sync search state on close/reopen instead of showing stale filtered results - Reset controlled search value when shade filter popovers close - Fixed isDuplicateName checking only search-filtered items instead of the full base snapshot (allItems) - Removed duplicate label resolution logic from useLabelPicker — now consumes resolvedItems from useFilterSearch instead of running a parallel fetch chain - Switched offers back to useBrowseOffers (non-infinite) since the API doesn't support pagination - Fixed unstable useCallback deps in usePostResourceSearch
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/posts/src/hooks/use-label-picker.ts (2)
125-140:⚠️ Potential issue | 🟡 MinoreditLabel looks up label from potentially filtered list.
Line 130 uses
labels(which islabelSearch.items) to find the old label. During an active search,itemsonly contains filtered results. If a user edits a label that isn't in the current search results,oldLabelwill beundefinedand the slug-swap logic (lines 134-138) won't execute.Consider using
labelSearch.allItemsinstead to ensure the label can be found regardless of current search state:🔧 Suggested fix
const editLabel = useCallback(async (id: string, name: string) => { const trimmed = name.trim(); if (!trimmed || isDuplicateName(trimmed, id)) { return; } - const oldLabel = labels.find(l => l.id === id); + const oldLabel = labelSearch.allItems.find(l => l.id === id); const result = await editLabelMutation({id, name: trimmed}); const updatedLabel = result?.labels?.[0];🤖 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 125 - 140, The editLabel callback currently looks up the old label using labels (which is labelSearch.items and may be filtered), causing oldLabel to be undefined during searches; change the lookup to use labelSearch.allItems so the original label is found regardless of search filters, then keep the existing slug-swap logic that checks oldLabel.slug !== updatedLabel.slug and uses selectedSlugsRef and onSelectionChange to replace the old slug with the new one; ensure the editLabel dependency array still includes labelSearch (or the appropriate allItems reference) so the hook updates correctly.
142-151:⚠️ Potential issue | 🟡 MinorSame issue: deleteLabel uses filtered list.
Line 143 also uses
labels(filtered items) to find the label. If the label being deleted isn't in the current search results, the selection cleanup (lines 145-149) won't happen.🔧 Suggested fix
const deleteLabel = useCallback(async (id: string) => { - const label = labels.find(l => l.id === id); + const label = labelSearch.allItems.find(l => l.id === id); await deleteLabelMutation(id);🤖 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 142 - 151, deleteLabel currently looks up the label in the filtered labels list, so if the deleted label is not in the current search results the selection cleanup won't run; change deleteLabel to resolve the label using the unfiltered/full labels source (e.g., an unfilteredLabels or labelsRef that holds all labels, or fetch the label by id before deleting) then call deleteLabelMutation(id) and, using that unfiltered label.slug, check selectedSlugsRef.current and call onSelectionChange with current.filter(s => s !== label.slug) when needed; ensure you reference the same deleteLabel function, selectedSlugsRef, labels (unfiltered variant) and onSelectionChange identifiers when implementing the fix.
♻️ Duplicate comments (1)
apps/posts/src/views/members/components/members-filters.tsx (1)
108-123:⚠️ Potential issue | 🟠 MajorMissing
activeValuesfor email search may cause label loss.Unlike
labelSearchandtierSearchwhich passactiveValues,emailSearchdoes not. This means if a saved member view references a post/email outside the first page of results, its display label won't be hydrated until the user searches for it.Consider passing the active email filter values:
🔧 Suggested fix
+ const activeEmailValues = getActiveFilterValues(filters, 'emails.post_id'); + const emailSearch = useFilterSearch({ useQuery: useBrowsePostsInfinite, dataKey: 'posts', serverSearchParams: term => ({ filter: term ? `${EMAIL_BASE_FILTER}+title:~${escapeNqlString(term)}` : EMAIL_BASE_FILTER, fields: 'id,title', order: 'published_at DESC' }), localSearchFilter: (posts, term) => posts.filter(p => p.title.toLowerCase().includes(term.toLowerCase())), limit: '25', toOption: p => ({value: p.id, label: p.title}), - useGetById: getPost + useGetById: getPost, + activeValues: activeEmailValues });🤖 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-filters.tsx` around lines 108 - 123, The emailSearch instance (created with useFilterSearch) is missing activeValues, so saved member views that reference emails/posts outside the initial page won't have their labels hydrated; update the emailSearch configuration to pass activeValues (same pattern as labelSearch/tierSearch) so the hook can fetch and hydrate option labels for values not present in the first page—locate the emailSearch declaration that uses EMAIL_BASE_FILTER, useBrowsePostsInfinite, and getPost and add an activeValues prop referencing the currently selected email filter values.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@apps/posts/src/hooks/use-label-picker.ts`:
- Around line 125-140: The editLabel callback currently looks up the old label
using labels (which is labelSearch.items and may be filtered), causing oldLabel
to be undefined during searches; change the lookup to use labelSearch.allItems
so the original label is found regardless of search filters, then keep the
existing slug-swap logic that checks oldLabel.slug !== updatedLabel.slug and
uses selectedSlugsRef and onSelectionChange to replace the old slug with the new
one; ensure the editLabel dependency array still includes labelSearch (or the
appropriate allItems reference) so the hook updates correctly.
- Around line 142-151: deleteLabel currently looks up the label in the filtered
labels list, so if the deleted label is not in the current search results the
selection cleanup won't run; change deleteLabel to resolve the label using the
unfiltered/full labels source (e.g., an unfilteredLabels or labelsRef that holds
all labels, or fetch the label by id before deleting) then call
deleteLabelMutation(id) and, using that unfiltered label.slug, check
selectedSlugsRef.current and call onSelectionChange with current.filter(s => s
!== label.slug) when needed; ensure you reference the same deleteLabel function,
selectedSlugsRef, labels (unfiltered variant) and onSelectionChange identifiers
when implementing the fix.
---
Duplicate comments:
In `@apps/posts/src/views/members/components/members-filters.tsx`:
- Around line 108-123: The emailSearch instance (created with useFilterSearch)
is missing activeValues, so saved member views that reference emails/posts
outside the initial page won't have their labels hydrated; update the
emailSearch configuration to pass activeValues (same pattern as
labelSearch/tierSearch) so the hook can fetch and hydrate option labels for
values not present in the first page—locate the emailSearch declaration that
uses EMAIL_BASE_FILTER, useBrowsePostsInfinite, and getPost and add an
activeValues prop referencing the currently selected email filter values.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d323b80d-d5aa-42d5-b58c-3df91d51a764
📒 Files selected for processing (9)
apps/admin-x-framework/src/api/offers.tsapps/posts/src/hooks/use-filter-search.tsapps/posts/src/hooks/use-label-picker.tsapps/posts/src/views/members/components/members-filters.tsxapps/posts/src/views/members/hooks/use-post-resource-search.tsapps/posts/src/views/members/use-member-filter-fields.test.tsapps/posts/src/views/members/use-member-filter-fields.tsapps/posts/test/unit/hooks/use-filter-search.test.tsapps/shade/src/components/ui/filters.tsx
✅ Files skipped from review due to trivial changes (2)
- apps/admin-x-framework/src/api/offers.ts
- apps/posts/src/hooks/use-filter-search.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/posts/src/views/members/hooks/use-post-resource-search.ts
- apps/shade/src/components/ui/filters.tsx
- Fixed Tailwind classname order lint errors in filters.tsx - Wrapped effectiveValues in useMemo to fix react-hooks/exhaustive-deps warning - Removed redundant client-side active tier filter - Added activeValues for post and email filter searches to hydrate saved views - Used selectedLabels instead of filtered labels in add/remove label modals - Used allItems instead of filtered items in editLabel/deleteLabel lookups - Made getActiveFilterValues accept multiple field names
Added apps/posts and apps/shade to scoped workspaces in lint-staged config so eslint runs from within each package directory, where eslint-plugin-tailwindcss can correctly resolve Tailwind v4 modules.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/hooks/use-label-picker.ts`:
- Around line 101-105: The uniqueness check uses labelSearch.allItems only;
instead compute a merged set of labels combining labelSearch.allItems,
labelSearch.items (current server/search results), and labelSearch.resolvedItems
(client-resolved off-page labels) and use that merged collection for
canCreateFromSearch and the other duplicate/slug-swap checks currently
referencing labelSearch.allItems; implement this by creating a deduplicated
array or Map keyed by id (or normalized name) and replace uses of
labelSearch.allItems in the functions (e.g., canCreateFromSearch and the
rename/delete slug logic) with the merged collection so off-page and
server-returned labels are considered.
In `@apps/posts/src/views/members/components/members-filters.tsx`:
- Around line 35-40: The toSearchProps adapter is narrowing away infinite-scroll
pagination and fetch state; update toSearchProps to forward the full
infinite-search contract instead of only onSearchChange, searchValue, and
isLoading — include and return onLoadMore, hasMore, isLoadingMore, and
isFetching (the same keys exposed by your useFilterSearch / useBrowse*Infinite
instances) so useMemberFilterFields and the members dropdown can show background
loading and fetch additional pages.
In `@apps/posts/src/views/members/hooks/use-post-resource-search.ts`:
- Around line 19-38: postSearch and pageSearch both receive the mixed
activeValues array so getPost may be called with page ids and getPage with post
ids; fix by making the activeValues type-aware before passing into each
useFilterSearch: either (A) split activeValues into post-only and page-only
arrays (e.g., postActiveValues, pageActiveValues) based on the item's
type/namespace and pass postActiveValues to the postSearch useFilterSearch and
pageActiveValues to the pageSearch useFilterSearch, or (B) supply a single
resolver that inspects an id's type and delegates to getPost or getPage
accordingly and pass that resolver into useFilterSearch; update references to
activeValues, postSearch, pageSearch, useFilterSearch, getPost, and getPage to
use the chosen approach.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 247d45e2-8e52-49ec-aab6-16c653baa6f4
📒 Files selected for processing (7)
apps/posts/src/hooks/use-filter-search.tsapps/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-filters.tsxapps/posts/src/views/members/hooks/use-post-resource-search.tsapps/shade/src/components/ui/filters.tsx
🚧 Files skipped from review as they are similar to previous changes (4)
- apps/posts/src/views/members/components/bulk-action-modals/remove-label-modal.tsx
- apps/posts/src/views/members/components/bulk-action-modals/add-label-modal.tsx
- apps/shade/src/components/ui/filters.tsx
- apps/posts/src/hooks/use-filter-search.ts
Comment on lines
+19
to
+38
| const postSearch = useFilterSearch({ | ||
| useQuery: useBrowsePostsInfinite, | ||
| dataKey: 'posts', | ||
| serverSearchParams: buildPublishedSearchParams, | ||
| localSearchFilter: titleFilter, | ||
| limit: '25', | ||
| toOption: p => ({value: p.id, label: p.title}), | ||
| useGetById: getPost, | ||
| activeValues | ||
| }); | ||
|
|
||
| const pageSearch = useFilterSearch({ | ||
| useQuery: useBrowsePagesInfinite, | ||
| dataKey: 'pages', | ||
| serverSearchParams: buildPublishedSearchParams, | ||
| localSearchFilter: titleFilter, | ||
| limit: '25', | ||
| toOption: p => ({value: p.id, label: p.title, detail: 'Page'}), | ||
| useGetById: getPage, | ||
| activeValues |
Contributor
There was a problem hiding this comment.
Don't pass the mixed signup/conversion id set into both resolvers.
activeValues here can contain both post ids and page ids, but both useFilterSearch instances receive the full array. That means a saved view containing a page will still try getPost(pageId), and a saved view containing a post will still try getPage(postId). At best this burns one failing lookup per opposite-type selection; at worst it prevents the selected option from hydrating cleanly. Use a single resolver that can read both resource types, or make the values type-aware before splitting them between the two hooks.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/posts/src/views/members/hooks/use-post-resource-search.ts` around lines
19 - 38, postSearch and pageSearch both receive the mixed activeValues array so
getPost may be called with page ids and getPage with post ids; fix by making the
activeValues type-aware before passing into each useFilterSearch: either (A)
split activeValues into post-only and page-only arrays (e.g., postActiveValues,
pageActiveValues) based on the item's type/namespace and pass postActiveValues
to the postSearch useFilterSearch and pageActiveValues to the pageSearch
useFilterSearch, or (B) supply a single resolver that inspects an id's type and
delegates to getPost or getPage accordingly and pass that resolver into
useFilterSearch; update references to activeValues, postSearch, pageSearch,
useFilterSearch, getPost, and getPage to use the chosen approach.
- Used merged label sources (allItems + items + resolvedItems) in use-label-picker for isDuplicateName, editLabel, and deleteLabel so off-page and server-searched labels are properly considered - Tracked failed ID resolutions in useFilterSearch to prevent the hook from getting stuck retrying unresolvable values (e.g. getPost called with a page ID)
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
apps/posts/src/hooks/use-label-picker.ts (1)
54-79: Consider extracting label resolution logic to reduce nesting depth.Static analysis flags the deeply nested code at lines 63-70. While this is a common React pattern, extracting the lookup logic into a helper function would improve readability and satisfy the linting rule.
♻️ Proposed refactor to reduce nesting
+ const resolveLabelBySlug = useCallback((slug: string, prevCache: Label[]) => { + return labelSearch.allItems.find(l => l.slug === slug) + || labelSearch.items.find(l => l.slug === slug) + || labelSearch.resolvedItems.find(l => l.slug === slug) + || prevCache.find(l => l.slug === slug) + || null; + }, [labelSearch.allItems, labelSearch.items, labelSearch.resolvedItems]); + useEffect(() => { if (selectedSlugs.length === 0) { setCachedSelectedLabels([]); return; } setCachedSelectedLabels((prev) => { - const result = selectedSlugs.map((slug) => { - // Check all available label sources from useFilterSearch - const fresh = labelSearch.allItems.find(l => l.slug === slug) - || labelSearch.items.find(l => l.slug === slug) - || labelSearch.resolvedItems.find(l => l.slug === slug); - if (fresh) { - return fresh; - } - // Fall back to previously cached - return prev.find(l => l.slug === slug) || null; - }).filter((l): l is Label => !!l); + const result = selectedSlugs + .map(slug => resolveLabelBySlug(slug, prev)) + .filter((l): l is Label => !!l); // Avoid unnecessary state updates if (result.length === prev.length && result.every((l, i) => l.slug === prev[i]?.slug && l.name === prev[i]?.name)) { return prev; } return result; }); - }, [selectedSlugs, labelSearch.items, labelSearch.allItems, labelSearch.resolvedItems]); + }, [selectedSlugs, resolveLabelBySlug]);🤖 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 54 - 79, Extract the label lookup logic inside the useEffect in use-label-picker.ts into a small helper function (e.g., resolveLabelBySlug) so the mapping callback passed to setCachedSelectedLabels is flat and not deeply nested; the helper should accept a slug, the three sources (labelSearch.allItems, labelSearch.items, labelSearch.resolvedItems) and a prev cache array and return either the fresh label or the cached fallback (or null). Replace the inline map/find block in the setCachedSelectedLabels updater with a call to resolveLabelBySlug for each slug and keep the existing filtering and equality-check logic unchanged to avoid behavior changes.apps/posts/src/hooks/use-filter-search.ts (1)
98-112: Consider usingNumber.parseIntinstead ofparseInt.Static analysis suggests using
Number.parseIntfor consistency with modern JavaScript conventions.♻️ Proposed fix
- setUseLocalSearch(items.length < parseInt(limit)); + setUseLocalSearch(items.length < Number.parseInt(limit, 10));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/posts/src/hooks/use-filter-search.ts` around lines 98 - 112, Replace the legacy global parseInt call inside the useEffect in use-filter-search.ts with Number.parseInt and include an explicit radix; specifically update the expression used when calling setUseLocalSearch (currently comparing items.length < parseInt(limit)) to use Number.parseInt(limit, 10) so the conversion is consistent and unambiguous; keep the surrounding logic in the same useEffect that checks useLocalSearch, isLoading, data and dataKey.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/posts/src/hooks/use-filter-search.ts`:
- Around line 98-112: Replace the legacy global parseInt call inside the
useEffect in use-filter-search.ts with Number.parseInt and include an explicit
radix; specifically update the expression used when calling setUseLocalSearch
(currently comparing items.length < parseInt(limit)) to use
Number.parseInt(limit, 10) so the conversion is consistent and unambiguous; keep
the surrounding logic in the same useEffect that checks useLocalSearch,
isLoading, data and dataKey.
In `@apps/posts/src/hooks/use-label-picker.ts`:
- Around line 54-79: Extract the label lookup logic inside the useEffect in
use-label-picker.ts into a small helper function (e.g., resolveLabelBySlug) so
the mapping callback passed to setCachedSelectedLabels is flat and not deeply
nested; the helper should accept a slug, the three sources
(labelSearch.allItems, labelSearch.items, labelSearch.resolvedItems) and a prev
cache array and return either the fresh label or the cached fallback (or null).
Replace the inline map/find block in the setCachedSelectedLabels updater with a
call to resolveLabelBySlug for each slug and keep the existing filtering and
equality-check logic unchanged to avoid behavior changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0ea12f8f-d256-4102-9919-10a521ab1ef7
📒 Files selected for processing (4)
.lintstagedrc.cjsapps/posts/src/hooks/use-filter-search.tsapps/posts/src/hooks/use-label-picker.tsapps/posts/test/unit/hooks/use-filter-search.test.ts
- Used Number.parseInt over parseInt - Merged duplicate imports from admin-x-framework modules - Extracted deriveSelectedLabels to reduce nesting in useLabelPicker - Reduced test duplication with shared defaultHookOptions and createServerModeData helpers - Used String.replaceAll over String.replace in label picker test
|
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (6)
apps/posts/src/hooks/use-filter-search.ts (3)
201-211: Mutating refs during render is a side effect that can cause issues.Lines 201-203 and 206-211 mutate
failedResolutionsRefandresolvedForOptionsRefdirectly during render. This violates React's expectation that the render phase is pure and side-effect-free, which can cause issues with Strict Mode double-rendering and concurrent features.♻️ Suggested approach using useEffect
- // Track failed resolutions so we skip them on future renders - if (missingValue && resolvedResult.isError) { - failedResolutionsRef.current.add(missingValue); - } - - // Append async-resolved item to the ref - if (resolvedRawItem) { - const resolvedValue = toOption(resolvedRawItem).value; - if (!resolvedForOptionsRef.current.some(existing => toOption(existing).value === resolvedValue)) { - resolvedForOptionsRef.current = [...resolvedForOptionsRef.current, resolvedRawItem]; - } - } + // Track failed resolutions so we skip them on future renders + useEffect(() => { + if (missingValue && resolvedResult.isError) { + failedResolutionsRef.current.add(missingValue); + } + }, [missingValue, resolvedResult.isError]); + + // Append async-resolved item to the ref + useEffect(() => { + if (resolvedRawItem) { + const resolvedValue = toOption(resolvedRawItem).value; + if (!resolvedForOptionsRef.current.some(existing => toOption(existing).value === resolvedValue)) { + resolvedForOptionsRef.current = [...resolvedForOptionsRef.current, resolvedRawItem]; + } + } + }, [resolvedRawItem, toOption]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/posts/src/hooks/use-filter-search.ts` around lines 201 - 211, The code mutates refs during render (failedResolutionsRef and resolvedForOptionsRef) — move those mutations into one or two useEffect hooks that run when the relevant values change (e.g., depend on missingValue and resolvedResult for failedResolutionsRef, and on resolvedRawItem for resolvedForOptionsRef); inside the effect, check resolvedResult.isError and add missingValue to failedResolutionsRef.current, and for resolvedRawItem compute const resolvedValue = toOption(resolvedRawItem).value and only push/assign into resolvedForOptionsRef.current if no existing entry satisfies toOption(existing).value === resolvedValue, ensuring all updates occur inside effects rather than during render.
111-111: Specify radix forNumber.parseInt.While
Number.parseIntdefaults to base 10 for strings without a prefix, explicitly specifying the radix improves clarity and prevents unexpected behavior if the string has leading zeros in older environments.✏️ Suggested fix
- setUseLocalSearch(items.length < Number.parseInt(limit)); + setUseLocalSearch(items.length < Number.parseInt(limit, 10));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/posts/src/hooks/use-filter-search.ts` at line 111, The call to Number.parseInt in setUseLocalSearch(items.length < Number.parseInt(limit)) omits the radix; update the parseInt call to include an explicit radix (e.g., Number.parseInt(limit, 10)) so parsing is deterministic. Locate the expression in the use-filter-search hook (referencing setUseLocalSearch and the limit variable) and replace Number.parseInt(limit) with Number.parseInt(limit, 10) (or use Number(limit) if appropriate for guaranteed base-10 conversion).
142-147: Mutating ref during render can cause issues with React concurrent features.Directly mutating
baseAllItemsRef.currentduring the render phase is a side effect that React's concurrent mode may execute multiple times. This pattern can lead to stale or inconsistent state when renders are interrupted and replayed.Consider moving this logic into a
useEffector using a different pattern:♻️ Suggested approach using useEffect
- // Only update base when we have non-search data. In server search mode, - // searchValue clears immediately but debouncedSearch lags behind — data still - // holds stale search results during that window, so guard against it. - const isNonSearchData = !searchValue && (useLocalSearch !== false || !debouncedSearch); - if (data && isNonSearchData) { - const arr = data[dataKey]; - if (Array.isArray(arr) && arr.length > 0) { - baseAllItemsRef.current = arr as ArrayItem<T, K>[]; - } - } + // Only update base when we have non-search data. In server search mode, + // searchValue clears immediately but debouncedSearch lags behind — data still + // holds stale search results during that window, so guard against it. + const isNonSearchData = !searchValue && (useLocalSearch !== false || !debouncedSearch); + useEffect(() => { + if (data && isNonSearchData) { + const arr = data[dataKey]; + if (Array.isArray(arr) && arr.length > 0) { + baseAllItemsRef.current = arr as ArrayItem<T, K>[]; + } + } + }, [data, dataKey, isNonSearchData]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/posts/src/hooks/use-filter-search.ts` around lines 142 - 147, The code mutates baseAllItemsRef.current during render (inside use-filter-search.ts) which is unsafe for React concurrent rendering; move that assignment into a side-effect: create a useEffect that runs when data, isNonSearchData, or dataKey change, check Array.isArray(data[dataKey]) and set baseAllItemsRef.current = arr inside the effect (or clear it when conditions fail) so the ref update occurs outside render and avoids replay/stale state issues.apps/posts/src/views/comments/components/comments-filters.tsx (1)
55-149: useMemo dependency includes entire objects, causing unnecessary recomputation.The dependency array
[memberSearch, postSearch]includes the entire return objects fromuseFilterSearch. Since these are new object references on each render, thefilterFieldsmemo recomputes every time, defeating its purpose.♻️ Destructure specific properties for stable dependencies
const filterFields: FilterFieldConfig[] = useMemo( () => [ { key: 'author', label: 'Author', type: 'select', icon: <LucideIcon.User className="size-4" />, options: memberSearch.options, isLoading: memberSearch.options.length === 0 && memberSearch.isLoading, onSearchChange: memberSearch.onSearchChange, searchValue: memberSearch.searchValue, // ... rest unchanged }, // ... rest unchanged ], - [memberSearch, postSearch] + [ + memberSearch.options, + memberSearch.isLoading, + memberSearch.onSearchChange, + memberSearch.searchValue, + postSearch.options, + postSearch.isLoading, + postSearch.onSearchChange, + postSearch.searchValue + ] );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/posts/src/views/comments/components/comments-filters.tsx` around lines 55 - 149, filterFields is being recomputed because useMemo depends on the whole memberSearch and postSearch objects; destructure only the stable properties used (options, isLoading, onSearchChange, searchValue) from memberSearch and postSearch (e.g. const { options: memberOptions, isLoading: memberLoading, onSearchChange: memberOnSearchChange, searchValue: memberSearchValue } = memberSearch and similarly for postSearch) and use those names inside the filterFields definition, then replace the dependency array with the specific properties ([memberOptions, memberLoading, memberOnSearchChange, memberSearchValue, postOptions, postLoading, postOnSearchChange, postSearchValue]) so memoization is stable.apps/posts/src/views/members/hooks/use-post-resource-search.ts (1)
46-49: Potential duplicate options when a resource exists in both posts and pages.The
optionsarray concatenatespostSearch.optionsandpageSearch.optionswithout deduplication. While posts and pages typically have distinct IDs, if there's any overlap or if both searches return resolved items for the sameactiveValue, duplicates could appear.♻️ Add deduplication if needed
const options = useMemo((): FilterOption<string>[] => { + const seen = new Set<string>(); + const result: FilterOption<string>[] = []; + for (const opt of [...postSearch.options, ...pageSearch.options]) { + if (!seen.has(opt.value)) { + seen.add(opt.value); + result.push(opt); + } + } + return result; - return [ - ...postSearch.options, - ...pageSearch.options - ]; }, [postSearch.options, pageSearch.options]);Alternatively, if IDs are guaranteed unique across posts/pages, a comment clarifying this would be helpful.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/posts/src/views/members/hooks/use-post-resource-search.ts` around lines 46 - 49, The combined options array built in the useMemo (const options = useMemo(...)) can produce duplicates by simply concatenating postSearch.options and pageSearch.options; update the logic to deduplicate by the unique identifier (e.g., option.id or option.value) — for example, merge the two arrays then filter using a Set or Map keyed by the id/value to keep the first occurrence — and ensure this change is applied where options is computed (referencing useMemo, postSearch.options, pageSearch.options); alternatively, if IDs are guaranteed unique across posts and pages, add a short inline comment next to that useMemo explaining the uniqueness guarantee.apps/posts/src/hooks/use-label-picker.ts (1)
76-83: Consider destructuring before useEffect to avoid unnecessary re-runs.The useEffect's dependency array directly references
labelSearch.items,labelSearch.allItems, andlabelSearch.resolvedItems. SincelabelSearchis a new object reference on each render, and these properties may also be new array references even when content is unchanged, this effect may run more frequently than intended.The
deriveSelectedLabelsfunction already handles reference equality internally (returningprevwhen unchanged), which mitigates the impact, but the effect still executes unnecessarily.♻️ Consider stabilizing references
If performance becomes a concern, you could either:
- Have
useFilterSearchreturn stable references (e.g., viauseMemo)- Use a ref to track the last-processed values and skip redundant calls
The current implementation is functional since
deriveSelectedLabelsreturnsprevwhen content is unchanged, avoiding cascading re-renders.🤖 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 76 - 83, Destructure the relevant properties from labelSearch before the effect so the dependency array uses stable references instead of accessing labelSearch.* directly; specifically, extract items, allItems, and resolvedItems (or derive stable memoized versions via useMemo in the same hook) and then update the useEffect to depend on selectedSlugs and those destructured/memoized variables while keeping the effect body using setCachedSelectedLabels and deriveSelectedLabels as-is.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/posts/src/hooks/use-filter-search.ts`:
- Around line 201-211: The code mutates refs during render (failedResolutionsRef
and resolvedForOptionsRef) — move those mutations into one or two useEffect
hooks that run when the relevant values change (e.g., depend on missingValue and
resolvedResult for failedResolutionsRef, and on resolvedRawItem for
resolvedForOptionsRef); inside the effect, check resolvedResult.isError and add
missingValue to failedResolutionsRef.current, and for resolvedRawItem compute
const resolvedValue = toOption(resolvedRawItem).value and only push/assign into
resolvedForOptionsRef.current if no existing entry satisfies
toOption(existing).value === resolvedValue, ensuring all updates occur inside
effects rather than during render.
- Line 111: The call to Number.parseInt in setUseLocalSearch(items.length <
Number.parseInt(limit)) omits the radix; update the parseInt call to include an
explicit radix (e.g., Number.parseInt(limit, 10)) so parsing is deterministic.
Locate the expression in the use-filter-search hook (referencing
setUseLocalSearch and the limit variable) and replace Number.parseInt(limit)
with Number.parseInt(limit, 10) (or use Number(limit) if appropriate for
guaranteed base-10 conversion).
- Around line 142-147: The code mutates baseAllItemsRef.current during render
(inside use-filter-search.ts) which is unsafe for React concurrent rendering;
move that assignment into a side-effect: create a useEffect that runs when data,
isNonSearchData, or dataKey change, check Array.isArray(data[dataKey]) and set
baseAllItemsRef.current = arr inside the effect (or clear it when conditions
fail) so the ref update occurs outside render and avoids replay/stale state
issues.
In `@apps/posts/src/hooks/use-label-picker.ts`:
- Around line 76-83: Destructure the relevant properties from labelSearch before
the effect so the dependency array uses stable references instead of accessing
labelSearch.* directly; specifically, extract items, allItems, and resolvedItems
(or derive stable memoized versions via useMemo in the same hook) and then
update the useEffect to depend on selectedSlugs and those destructured/memoized
variables while keeping the effect body using setCachedSelectedLabels and
deriveSelectedLabels as-is.
In `@apps/posts/src/views/comments/components/comments-filters.tsx`:
- Around line 55-149: filterFields is being recomputed because useMemo depends
on the whole memberSearch and postSearch objects; destructure only the stable
properties used (options, isLoading, onSearchChange, searchValue) from
memberSearch and postSearch (e.g. const { options: memberOptions, isLoading:
memberLoading, onSearchChange: memberOnSearchChange, searchValue:
memberSearchValue } = memberSearch and similarly for postSearch) and use those
names inside the filterFields definition, then replace the dependency array with
the specific properties ([memberOptions, memberLoading, memberOnSearchChange,
memberSearchValue, postOptions, postLoading, postOnSearchChange,
postSearchValue]) so memoization is stable.
In `@apps/posts/src/views/members/hooks/use-post-resource-search.ts`:
- Around line 46-49: The combined options array built in the useMemo (const
options = useMemo(...)) can produce duplicates by simply concatenating
postSearch.options and pageSearch.options; update the logic to deduplicate by
the unique identifier (e.g., option.id or option.value) — for example, merge the
two arrays then filter using a Set or Map keyed by the id/value to keep the
first occurrence — and ensure this change is applied where options is computed
(referencing useMemo, postSearch.options, pageSearch.options); alternatively, if
IDs are guaranteed unique across posts and pages, add a short inline comment
next to that useMemo explaining the uniqueness guarantee.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 255b1712-082e-4b40-bf43-62decbd61600
📒 Files selected for processing (7)
apps/posts/src/hooks/use-filter-search.tsapps/posts/src/hooks/use-label-picker.tsapps/posts/src/views/comments/components/comments-filters.tsxapps/posts/src/views/members/components/members-filters.tsxapps/posts/src/views/members/hooks/use-post-resource-search.tsapps/posts/test/unit/hooks/use-filter-search.test.tsapps/posts/test/unit/hooks/use-label-picker.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/posts/test/unit/hooks/use-label-picker.test.ts
Split search mode detection and active value resolution into separate hooks under use-filter-search/ for better composability.
|
Member
Author
|
There's an alternative approach in #26986 that tries to avoid the monolithic |
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.
ref https://linear.app/ghost/issue/BER-3334/
Summary
useFilterSearchhook handles local/server search mode detection, debouncing, infinite scroll, and async resolution of selected values not on the current pageuseFilterSearchinstead of separate scattered implementationscreateQueryWithId-based getters (getLabelBySlug,getNewsletter, etc.) in admin-x-framework resolve display names for filter values outside the paginated windowuseLabelPickercaches selected labels independently (mirroring shade'scachedSelectedOptionspattern) so label renames and search/remount cycles don't lose resolved namesuseDeleteLabel's cache updater crashing on infinite query datashouldFilteron filter dropdowns that useonSearchChange— cmdk v1 reorders DOM elements viaappendChildduring scored search and never restores the original order when the search is cleared