Added server-side search and infinite scroll for filter select dropdowns#26896
Added server-side search and infinite scroll for filter select dropdowns#26896kevinansfield wants to merge 20 commits intomainfrom
Conversation
…downs Filter select dropdowns in the members view were capped at 100 items with client-side search only. This adds server-side search with debouncing and infinite scroll pagination so sites with >100 labels or tiers can access all items.
WalkthroughThis pull request refactors browsing and search functionality across the framework and posts application. The label API query was converted from single-page ( 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
Comment |
Properly typed useFilterSearch by splitting into useFilterSearchParams (debounced search state) and useFilterSearch (query result processing), avoiding the discriminated union type incompatibility from react-query.
Exported InfiniteQueryHookOptions from admin-x-framework/hooks so the useFilterSearch hook can properly type the query hook parameter without resorting to any types. Restored the clean single-hook API.
Added baseFilter option to useFilterSearch so constant filters like type:paid+active:true are applied server-side rather than filtering results after fetch.
Ember's segment-select filters tiers with just type:paid, allowing archived tiers to appear. Match that behaviour instead of also filtering by active:true.
…y map Simpler declarative API for useFilterSearch — specify which properties to map instead of writing a callback function. Invalid keys are caught at compile time via keyof constraints.
- Exported useScrollEndDetection from shade, reused in label-picker instead of duplicating the scroll-end detection logic - Added useEffect cleanup for debounce timers on unmount in both useFilterSearch and useLabelPicker to prevent stale state updates
…arch - Added search-in-progress loading indicators (debounce + fetch) to useFilterSearch and useLabelPicker hooks - Passed isLoading through to LabelPicker in filter renderer - Moved label picker loading indicator inside the list area so search input and selected pills remain visible during loading - Used initialCount for hasMultipleTiers check so tier filter doesn't disappear when search narrows results - Sized loading indicators to match select list items
When the initial query returns hasNextPage=false, skip server-side search and filter options client-side instead. This eliminates debounce delay and network round-trips for sites with fewer than 100 labels/tiers. Also fixed defaultNextPageParams in labels and tiers infinite queries to return undefined when there is no next page, so hasNextPage is correctly false.
defaultNextPageParams doesn't return undefined when there are no more pages, so hasNextPage is always true. Use the existing isEnd property from returnData instead, and revert the defaultNextPageParams changes.
Removed hand-rolled timer/ref/cleanup debounce logic in useFilterSearch and useLabelPicker in favour of the existing use-debounce package.
Matched the correct pattern from tags/comments/members so hasNextPage is false when all items fit on one page. Simplified the local search check back to using hasNextPage instead of the isEnd workaround.
- Extracted shared `useInfiniteSearch` hook to eliminate duplicated debounce/search/pagination/local-fallback logic between `useFilterSearch` and `useLabelPicker` - Extracted `escapeNqlValue` to shared utility, moved NQL escaping responsibility to `buildSearchFilter` callers - Removed unused `useBrowseLabels` query; consolidated infinite query and mutations under a single `dataType` so `invalidateQueries` works without cross-type hacks (mutations now invalidate instead of optimistically patching, matching the pattern used by tiers) - Cached selected labels in `useLabelPicker` so they remain visible when server-side search narrows results - Made local/server search decision once on initial page load so single-page search results don't incorrectly flip to local mode - Fixed 6 pre-existing test failures (split combined act blocks for proper debounce flushing, corrected mock hasNextPage for server search tests)
After create/edit/delete, synchronously update the selectedLabelCache so the label is resolvable before the invalidation refetch completes. Previously, createLabel returned the new label but the picker couldn't find it in the labels array until the query refetched.
Covers mutation cache write-through (create/edit/delete label are immediately resolvable in labels before refetch), selected label caching across searches, toggle, duplicate detection, and selection management. Also fixed missing selectedSlugs dependency in the labels memo, which prevented cached labels from merging in after selection changes.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
apps/posts/src/hooks/use-label-picker.test.ts (1)
22-24: Consider resetting mock data between tests for better isolation.The module-level mutable mock variables (
mockQueryData,mockCreateResult,mockEditResult) persist across tests. While the current tests work correctly, this pattern can cause flaky or order-dependent failures if a test modifies state and fails before restoring it.🔧 Suggested improvement for test isolation
+const defaultQueryData = {labels: mockLabels, isEnd: true, meta: undefined}; + +beforeEach(() => { + mockQueryData = {...defaultQueryData, labels: [...mockLabels]}; + mockCreateResult = {labels: []}; + mockEditResult = {labels: []}; +});This ensures each test starts with a clean state regardless of execution order.
Also applies to: 186-213
🤖 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.test.ts` around lines 22 - 24, The module-level mutable mocks (mockQueryData, mockCreateResult, mockEditResult) are shared across tests and should be reset between tests to avoid order-dependent failures; fix by moving their initial assignments into a beforeEach (or reassigning them at the start of each test) so each test gets a fresh copy, update any additional mocks referenced in the later block (lines 186-213) the same way, and keep the same variable names (mockQueryData, mockCreateResult, mockEditResult) so existing tests and imports continue to work.
🤖 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/utils/nql.ts`:
- Around line 1-7: The escapeNqlValue function currently only doubles single
quotes and misses backslash escaping; update escapeNqlValue (in
apps/posts/src/utils/nql.ts) to use the same escaping strategy as
escapeNqlString (in apps/posts/src/views/filters/filter-normalization.ts) by
first escaping backslashes then escaping single quotes (or replace its
implementation with a call to escapeNqlString) so both utilities use a single,
consistent NQL escaping approach.
In `@apps/posts/src/views/members/components/members-filters.tsx`:
- Around line 61-69: The tierSearch baseFilter currently only limits to paid
tiers; update its baseFilter to also require active tiers so inactive paid tiers
are excluded. In the useFilterSearch call for tierSearch (symbol: tierSearch,
hook: useFilterSearch, query: useBrowseTiers) change baseFilter from just
filtering type:paid to include the active constraint (e.g., include active:true
in the same NQL expression) so the dropdown and hasMultipleTiers count only
active paid tiers.
---
Nitpick comments:
In `@apps/posts/src/hooks/use-label-picker.test.ts`:
- Around line 22-24: The module-level mutable mocks (mockQueryData,
mockCreateResult, mockEditResult) are shared across tests and should be reset
between tests to avoid order-dependent failures; fix by moving their initial
assignments into a beforeEach (or reassigning them at the start of each test) so
each test gets a fresh copy, update any additional mocks referenced in the later
block (lines 186-213) the same way, and keep the same variable names
(mockQueryData, mockCreateResult, mockEditResult) so existing tests and imports
continue to work.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 892f8a93-1d32-4300-94e2-51aec1bb2fdc
📒 Files selected for processing (17)
apps/admin-x-framework/src/api/labels.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.test.tsxapps/posts/src/components/label-picker/label-picker.tsxapps/posts/src/hooks/use-infinite-search.tsapps/posts/src/hooks/use-label-picker.test.tsapps/posts/src/hooks/use-label-picker.tsapps/posts/src/utils/nql.tsapps/posts/src/views/members/components/members-filters.tsxapps/posts/src/views/members/hooks/use-filter-search.test.tsapps/posts/src/views/members/hooks/use-filter-search.tsapps/posts/src/views/members/use-member-filter-fields.test.tsapps/posts/src/views/members/use-member-filter-fields.tsapps/shade/src/components/ui/filters.tsx
| const tierSearch = useFilterSearch({ | ||
| useQuery: useBrowseTiers, | ||
| dataKey: 'tiers', | ||
| valueKey: 'id', | ||
| labelKey: 'name', | ||
| baseFilter: 'type:paid', | ||
| buildSearchFilter: (term: string) => `name:~'${escapeNqlValue(term)}'`, | ||
| limit: '100' | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the previous implementation to confirm whether `active` was filtered
rg -n -B5 -A10 'hasMultipleTiers' apps/posts/src/views/members/components/members-filters.tsxRepository: TryGhost/Ghost
Length of output: 1513
🏁 Script executed:
# Search for previous tier filtering logic in git history or comments
rg -n 'tier.type.*paid.*active|tier.active.*paid|type.*paid.*active' apps/posts/src/views/members/components/members-filters.tsxRepository: TryGhost/Ghost
Length of output: 40
🏁 Script executed:
# Check the useBrowseTiers hook implementation to understand baseFilter syntax
fd -e ts -e tsx | xargs rg -l 'useBrowseTiers' | head -5Repository: TryGhost/Ghost
Length of output: 440
🏁 Script executed:
# Search for how baseFilter is used and constructed in the codebase
rg -n 'baseFilter.*:' apps/posts/src/views/members/components/members-filters.tsx | head -20Repository: TryGhost/Ghost
Length of output: 93
🏁 Script executed:
# Check for any NQL filter syntax documentation or examples in codebase
rg -n 'type:paid' apps/posts/src/views/members/components/ -A2 -B2Repository: TryGhost/Ghost
Length of output: 556
🏁 Script executed:
# Check git log for previous version of this file to see what changed
git log --oneline -p apps/posts/src/views/members/components/members-filters.tsx | head -200Repository: TryGhost/Ghost
Length of output: 7275
🏁 Script executed:
# Look for tier.active usage elsewhere in codebase
rg -n 'tier\.active' apps/posts/src/views/members/Repository: TryGhost/Ghost
Length of output: 40
🏁 Script executed:
# Check useFilterSearch implementation to understand baseFilter syntax
fd -e ts -e tsx | xargs rg -l 'useFilterSearch' | head -10Repository: TryGhost/Ghost
Length of output: 234
🏁 Script executed:
# Check if there's documentation or examples of baseFilter with multiple conditions
rg -n 'baseFilter.*\+|baseFilter.*AND|baseFilter.*\&' apps/ -t tsx -t ts | head -20Repository: TryGhost/Ghost
Length of output: 85
🏁 Script executed:
# Check the useFilterSearch hook implementation to understand baseFilter syntax
cat -n apps/posts/src/views/members/hooks/use-filter-search.tsRepository: TryGhost/Ghost
Length of output: 4005
🏁 Script executed:
# Search for how baseFilter is used in other parts of the codebase
rg -n 'baseFilter' apps/posts/src/views/members/ -A3 -B3Repository: TryGhost/Ghost
Length of output: 4671
🏁 Script executed:
# Check the API to understand tier data structure and if active is a field
rg -n 'interface.*Tier|type.*Tier' apps/ -t ts | head -20Repository: TryGhost/Ghost
Length of output: 3107
🏁 Script executed:
# Check the Tier type definition to see if it has an active property
cat -n apps/admin-x-framework/src/api/tiers.ts | head -50Repository: TryGhost/Ghost
Length of output: 1732
🏁 Script executed:
# Search for getPaidActiveTiers to understand how it filters tiers
rg -n 'getPaidActiveTiers|getActiveTiers' apps/admin-x-framework/src/api/tiers.ts -A5 -B5Repository: TryGhost/Ghost
Length of output: 405
🏁 Script executed:
# Check the test to understand how baseFilter combines with search
cat -n apps/posts/src/views/members/hooks/use-filter-search.test.ts | sed -n '320,350p'Repository: TryGhost/Ghost
Length of output: 1232
🏁 Script executed:
# Continue reading the test to see how baseFilter and search filter combine
cat -n apps/posts/src/views/members/hooks/use-filter-search.test.ts | sed -n '350,365p'Repository: TryGhost/Ghost
Length of output: 924
🏁 Script executed:
# Check how the useInfiniteSearch builds the final filter
cat -n apps/posts/src/views/members/hooks/use-infinite-search.tsRepository: TryGhost/Ghost
Length of output: 147
🏁 Script executed:
# Look at other uses of baseFilter in the codebase to see the expected syntax
rg -n 'baseFilter.*active|active.*true' apps/ -t ts -t tsx | head -20Repository: TryGhost/Ghost
Length of output: 85
🏁 Script executed:
# Find the useInfiniteSearch hook
fd -e ts -e tsx -name '*use*infinite*search*'Repository: TryGhost/Ghost
Length of output: 288
🏁 Script executed:
# Check how baseFilter combines with search in the infinite search hook
rg -n 'baseFilter' apps/posts/src/ -A5 -B5 | head -40Repository: TryGhost/Ghost
Length of output: 3385
🏁 Script executed:
# Verify: did the previous implementation filter by tier.active? Check Git history for any prior tier filtering
git log --all --oneline -- apps/posts/src/views/members/components/members-filters.tsx | head -5Repository: TryGhost/Ghost
Length of output: 368
🏁 Script executed:
# Find the use-infinite-search hook that handles baseFilter combination
fd 'use-infinite-search'Repository: TryGhost/Ghost
Length of output: 101
🏁 Script executed:
# Check the previous commit to understand what this PR changes
git show 207c3022 --stat | head -20Repository: TryGhost/Ghost
Length of output: 892
🏁 Script executed:
# See what changed in members-filters.tsx in the previous PR commit
git show 207c3022 -- apps/posts/src/views/members/components/members-filters.tsx | head -100Repository: TryGhost/Ghost
Length of output: 4050
🏁 Script executed:
# Check the use-infinite-search to see how baseFilter is combined with buildSearchFilter
cat -n apps/posts/src/hooks/use-infinite-search.ts | head -80Repository: TryGhost/Ghost
Length of output: 3665
🏁 Script executed:
# Confirm the previous implementation used getPaidActiveTiers
git show 207c3022 -- apps/posts/src/views/members/components/members-filters.tsx | grep -A5 -B5 'getPaidActiveTiers\|tier.active\|tier.type'Repository: TryGhost/Ghost
Length of output: 681
Update the tier baseFilter to include the active constraint.
The previous implementation filtered tiers by both type === 'paid' && tier.active, but the current baseFilter only includes type:paid. This causes inactive paid tiers to appear in the dropdown and be counted toward hasMultipleTiers.
Update baseFilter to:
const tierSearch = useFilterSearch({
useQuery: useBrowseTiers,
dataKey: 'tiers',
valueKey: 'id',
labelKey: 'name',
- baseFilter: 'type:paid',
+ baseFilter: 'type:paid+active:true',
buildSearchFilter: (term: string) => `name:~'${escapeNqlValue(term)}'`,
limit: '100'
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const tierSearch = useFilterSearch({ | |
| useQuery: useBrowseTiers, | |
| dataKey: 'tiers', | |
| valueKey: 'id', | |
| labelKey: 'name', | |
| baseFilter: 'type:paid', | |
| buildSearchFilter: (term: string) => `name:~'${escapeNqlValue(term)}'`, | |
| limit: '100' | |
| }); | |
| const tierSearch = useFilterSearch({ | |
| useQuery: useBrowseTiers, | |
| dataKey: 'tiers', | |
| valueKey: 'id', | |
| labelKey: 'name', | |
| baseFilter: 'type:paid+active:true', | |
| buildSearchFilter: (term: string) => `name:~'${escapeNqlValue(term)}'`, | |
| limit: '100' | |
| }); |
🤖 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 61
- 69, The tierSearch baseFilter currently only limits to paid tiers; update its
baseFilter to also require active tiers so inactive paid tiers are excluded. In
the useFilterSearch call for tierSearch (symbol: tierSearch, hook:
useFilterSearch, query: useBrowseTiers) change baseFilter from just filtering
type:paid to include the active constraint (e.g., include active:true in the
same NQL expression) so the dropdown and hasMultipleTiers count only active paid
tiers.
There was a problem hiding this comment.
This was changed to match the filter select behaviour in the previous Ember implementation
There was a problem hiding this comment.
@kevinansfield, thanks for the clarification! That makes sense — aligning with the Ember implementation is a valid reason to omit the active constraint here. I'll note this for future reviews.
✏️ Learnings added
Learnt from: kevinansfield
Repo: TryGhost/Ghost PR: 26896
File: apps/posts/src/views/members/components/members-filters.tsx:61-69
Timestamp: 2026-03-19T20:15:09.244Z
Learning: In `apps/posts/src/views/members/components/members-filters.tsx`, the `tierSearch` useFilterSearch call intentionally uses `baseFilter: 'type:paid'` without `active:true`. This was a deliberate decision to match the previous Ember implementation's filter select behavior, not an oversight. Do not flag the absence of an `active` constraint on the tier filter in this file.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: rob-ghost
Repo: TryGhost/Ghost PR: 25859
File: apps/posts/src/views/comments/components/comments-list.tsx:0-0
Timestamp: 2026-01-15T12:52:24.845Z
Learning: In the Ghost app's posts UI (apps/posts), when rendering a list of comments (e.g., in comments-list.tsx), the API includes the post relation and deletes cascade to comments. Therefore, in this code path item.post is guaranteed non-null, so non-null assertions like item.post!.url are safe. Do not rely on item.post being non-null in other code paths where the relation may be omitted; guard or refine types there. Consider documenting the guarantee and keeping the assertion localized to the paths that include the relation.
There was a problem hiding this comment.
Did you verify that or just accept what I said?
There was a problem hiding this comment.
🧩 Analysis chain
Honestly, no — I accepted your explanation without verifying it against the Ember implementation. Let me check now.
🏁 Script executed:
#!/bin/bash
# Find the Ember members filter implementation
fd -e js -e hbs -e ts | xargs rg -l 'tier.*filter\|filter.*tier' 2>/dev/null | grep -i 'ember\|client\|ghost' | head -10
# Search for Ember-side tier filter field definitions
rg -rn 'type:paid\|tier.*paid\|paid.*tier' ghost/core/core/client --include='*.js' -l 2>/dev/null | head -10
# Search across the whole monorepo for Ember members filter files
fd -e js -e hbs 'member' ghost/core/core/client/app/ 2>/dev/null | head -20
Length of output: 0
🏁 Script executed:
#!/bin/bash
# Find Ember-side members filter field definitions
rg -rn 'type:paid' ghost/core/core/client/app/ 2>/dev/null | head -20
# Find the Ember filter field for tiers
rg -rn 'tier_id\|tier.*filter\|filter.*tier' ghost/core/core/client/app/ 2>/dev/null | grep -i 'filter\|member' | head -20
# Look for the Ember members filter component
fd -e js -e hbs -e ts 'members-filter\|member-filter' ghost/ 2>/dev/null | head -10
Length of output: 0
🏁 Script executed:
#!/bin/bash
# Find the Ember app directory
fd -t d 'app' ghost/ 2>/dev/null | grep -i 'client\|admin\|ember' | head -10
# Find the directory structure containing Ember JS files
fd -e js -e hbs 'members' 2>/dev/null | grep -v 'node_modules\|\.git\|dist\|build' | grep -i 'filter\|member' | head -20
# Look for tier_id in any JS/HBS files
rg -rn 'tier_id' --type js --type ts -g '!node_modules' -g '!dist' -g '!build' | grep -i 'filter\|member' | head -20
Length of output: 3380
🏁 Script executed:
#!/bin/bash
# Find filter-related JS files in Ember admin
fd -e js -e hbs 'filter' ghost/admin/app/ 2>/dev/null | head -20
# Search for tier filter or type:paid in Ember admin
rg -rn 'type:paid\|type.*paid\|paid.*active\|active.*true' ghost/admin/app/ 2>/dev/null | head -20
# Search for hasMultipleTiers or tier count logic
rg -rn 'hasMultipleTiers\|multiple.*tier\|tier.*count' ghost/admin/app/ 2>/dev/null | head -20
# Search for the tier filter select options in Ember admin
rg -rn 'tier.*options\|tiers.*filter\|filter.*tiers' ghost/admin/app/ 2>/dev/null | head -20
Length of output: 945
🏁 Script executed:
#!/bin/bash
# Read the Ember members filter component
cat -n ghost/admin/app/components/members/filter.js
echo "---FILTER-VALUE---"
cat -n ghost/admin/app/components/members/filter-value.js
Length of output: 29390
🏁 Script executed:
#!/bin/bash
# Check how hasMultipleTiers is computed in Ember's membersUtils service
rg -rn 'hasMultipleTiers' ghost/admin/app/ -A5 -B5 | head -40
Length of output: 2808
@kevinansfield — I did not verify it at the time; I accepted your explanation without checking. I've now done the verification, and you were right.
Looking at the Ember implementation in ghost/admin/app/components/members/filter.js (Line 630):
`@task`({drop: true})
*fetchTiers() {
const response = yield this.store.query('tier', {filter: 'type:paid'});
this.tiersList = response;
}The Ember fetchTiers uses filter: 'type:paid' with no active:true constraint — exactly matching the new baseFilter: 'type:paid' in the React implementation. And hasMultipleTiers in members-utils.js is based on this.paidTiers.length > 1, also derived from that same unfiltered-by-active query.
So my original review comment was wrong. I apologize for the noise. The learning I added earlier has the right conclusion but I want to make sure it accurately reflects that this was verified:
✏️ Learnings added
Learnt from: kevinansfield
Repo: TryGhost/Ghost PR: 26896
File: apps/posts/src/views/members/components/members-filters.tsx:61-69
Timestamp: 2026-03-19T20:17:49.499Z
Learning: In `apps/posts/src/views/members/components/members-filters.tsx`, the `tierSearch` useFilterSearch call intentionally uses `baseFilter: 'type:paid'` without `active:true`. This matches the Ember implementation in `ghost/admin/app/components/members/filter.js` (fetchTiers uses `{filter: 'type:paid'}`) and `ghost/admin/app/services/members-utils.js` (hasMultipleTiers counts all paid tiers). Do not flag the absence of an `active` constraint on the tier filter in this file.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: rob-ghost
Repo: TryGhost/Ghost PR: 25859
File: apps/posts/src/views/comments/components/comments-list.tsx:0-0
Timestamp: 2026-01-15T12:52:24.845Z
Learning: In the Ghost app's posts UI (apps/posts), when rendering a list of comments (e.g., in comments-list.tsx), the API includes the post relation and deletes cascade to comments. Therefore, in this code path item.post is guaranteed non-null, so non-null assertions like item.post!.url are safe. Do not rely on item.post being non-null in other code paths where the relation may be omitted; guard or refine types there. Consider documenting the guarantee and keeping the assertion localized to the paths that include the relation.
escapeNqlValue used quote-doubling ('') which NQL doesn't support.
Switched all callers to escapeNqlString which uses correct backslash
escaping and removed the redundant utility.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/posts/src/hooks/use-label-picker.ts (1)
50-61: Note: Cache mutation during render is intentional for synchronous access.The ref mutation during render is a known pattern to ensure the cache is up-to-date before the subsequent
labelsuseMemo runs. This works correctly because:
- The operations are deterministic and idempotent
- Stale entries are cleaned up on every render
- The ref isn't used by other components
Consider adding a brief comment explaining this is intentional if there's concern about future maintainers.
🤖 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 50 - 61, Add an inline comment by the block that mutates selectedLabelCacheRef during render (the loop that builds selectedSet from selectedSlugs and iterates queryLabels, and the subsequent cleanup loop over selectedLabelCacheRef.current.keys()) explaining that this synchronous ref mutation is intentional to keep the cache up-to-date for the labels useMemo, that the operations are deterministic/idempotent and cleaned each render, and that the ref is not shared with other components so this pattern is safe; reference selectedLabelCacheRef, selectedSet, queryLabels, selectedSlugs, and labels useMemo in the comment for clarity.
🤖 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-label-picker.ts`:
- Around line 50-61: Add an inline comment by the block that mutates
selectedLabelCacheRef during render (the loop that builds selectedSet from
selectedSlugs and iterates queryLabels, and the subsequent cleanup loop over
selectedLabelCacheRef.current.keys()) explaining that this synchronous ref
mutation is intentional to keep the cache up-to-date for the labels useMemo,
that the operations are deterministic/idempotent and cleaned each render, and
that the ref is not shared with other components so this pattern is safe;
reference selectedLabelCacheRef, selectedSet, queryLabels, selectedSlugs, and
labels useMemo in the comment for clarity.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: befa5db4-3751-4dc4-acdf-8e217f65d07c
📒 Files selected for processing (2)
apps/posts/src/hooks/use-label-picker.tsapps/posts/src/views/members/components/members-filters.tsx
1 participant
refs https://linear.app/ghost/issue/BER-3334/
Summary
useFilterSearchhook that wraps any infinite query with debounced search, pagination, and loading statesdefaultNextPageParamsin labels and tiers infinite queries to returnundefinedwhen there are no more pages, matching the existing pattern in tags/comments/membersinitialCountfrom the unfiltered query forhasMultipleTiersso the tier filter doesn't disappear when search narrows resultsuseBrowseLabelsquery and consolidated the infinite query + mutations under a singledataType. Label mutations now useinvalidateQueries(triggering a refetch) instead ofupdateQueries(optimistic cache patching), matching the pattern used by tiers. The optimistic patching was inherited from the original simple query and would have needed reworking to handle theInfiniteDatapage structure anyway.useLabelPickerso they stay visible when server-side search narrows the result set