Open
Conversation
ref https://linear.app/ghost/issue/BER-3334/ Moved async filter lookups behind field-owned value sources so filter UIs no longer carry search orchestration and the remaining comments, members, label picker, and stats flows can share a simpler contract.
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:
WalkthroughThis pull request introduces a ValueSource abstraction pattern for dynamic option loading across the application. It adds infinite query hooks and parameterized query functions to the API layer (labels, newsletters, pages, posts, tiers), implements new filter source hooks for members, labels, posts, tiers, and emails, and refactors existing filtering components to use ValueSource instead of managing static options and search state independently. The label picker is enhanced with controlled search capabilities and resolved label tracking. Related utility functions for filtering and merging options are introduced, and deprecated filter option utilities are removed. The shade UI filters component is updated to support the ValueSource interface. 🚥 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 |
github-actions
bot
added
the
migration
ref https://linear.app/ghost/issue/BER-3334/ Removed redundant assertions, tightened prop immutability, and simplified value resolution so the PR-specific Sonar findings in the new filter value source path are addressed without changing behavior.
![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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/posts/src/views/PostAnalytics/components/stats-filter.tsx (1)
159-174:⚠️ Potential issue | 🟡 MinorUnused function:
applySelectedPostFilter.This function is defined but never used in this file. The comment at line 309 confirms there's no 'post' filter in the PostAnalytics context. This appears to be dead code, possibly copied from the stats app version.
🗑️ Proposed removal
-const applySelectedPostFilter = (params: Record<string, string>, selectedValue?: string) => { - if (!selectedValue) { - return; - } - - if (selectedValue.startsWith('/')) { - params.pathname = selectedValue; - delete params.post_uuid; - return; - } - - params.post_uuid = selectedValue; - delete params.pathname; -}; -🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/posts/src/views/PostAnalytics/components/stats-filter.tsx` around lines 159 - 174, The file contains an unused dead function applySelectedPostFilter which is never referenced (and a nearby comment notes there is no 'post' filter in this context); remove the applySelectedPostFilter function declaration and any associated types/exports/imports exclusively used by it to eliminate dead code and simplify the module, leaving other filter-related helpers like buildFilterParams intact.
🧹 Nitpick comments (12)
apps/posts/src/hooks/filter-sources/use-email-post-value-source.ts (1)
32-39: Minor:buildQuotedListFilteris called twice.The filter is computed twice - once for the conditional check and once for the value. Consider computing it once and reusing.
♻️ Suggested refactor
+ const hydratedFilter = buildQuotedListFilter('id', selectedValues); const hydrated = useBrowsePosts({ enabled: selectedValues.length > 0, searchParams: { limit: '25', fields: 'id,title', - ...(buildQuotedListFilter('id', selectedValues) ? {filter: buildQuotedListFilter('id', selectedValues)} : {}) + ...(hydratedFilter ? {filter: hydratedFilter} : {}) } });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/posts/src/hooks/filter-sources/use-email-post-value-source.ts` around lines 32 - 39, The code calls buildQuotedListFilter('id', selectedValues) twice when constructing the searchParams for useBrowsePosts; compute the filter once into a local const (e.g. const idFilter = buildQuotedListFilter('id', selectedValues)) and then use idFilter in the conditional and the object value so useBrowsePosts only receives the filter once, keeping selectedValues, buildQuotedListFilter and useBrowsePosts as the referenced symbols to locate the change.apps/posts/src/hooks/filter-sources/use-post-resource-value-source.ts (1)
47-62: Minor:buildQuotedListFilteris called twice for each hydrated query.Same pattern as in
use-email-post-value-source.ts- the filter is computed twice per query. Consider computing once and reusing.♻️ Suggested refactor
+ const hydratedFilter = buildQuotedListFilter('id', selectedValues); const hydratedPostBrowse = useBrowsePosts({ enabled: selectedValues.length > 0, searchParams: { limit: '25', fields: 'id,title', - ...(buildQuotedListFilter('id', selectedValues) ? {filter: buildQuotedListFilter('id', selectedValues)} : {}) + ...(hydratedFilter ? {filter: hydratedFilter} : {}) } }); const hydratedPageBrowse = useBrowsePages({ enabled: selectedValues.length > 0, searchParams: { limit: '25', fields: 'id,title', - ...(buildQuotedListFilter('id', selectedValues) ? {filter: buildQuotedListFilter('id', selectedValues)} : {}) + ...(hydratedFilter ? {filter: hydratedFilter} : {}) } });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/posts/src/hooks/filter-sources/use-post-resource-value-source.ts` around lines 47 - 62, Compute the filter once and reuse it instead of calling buildQuotedListFilter('id', selectedValues) twice for each query: create a local const (e.g., const idFilter = buildQuotedListFilter('id', selectedValues)) before the useBrowsePosts/useBrowsePages calls, then use idFilter in the searchParams spread (e.g., ...(idFilter ? { filter: idFilter } : {})) for both hydratedPostBrowse and hydratedPageBrowse so the filter is evaluated only once and the code is clearer.apps/stats/src/views/Stats/components/stats-filter.tsx (1)
607-607: Minor: Function uses declaration syntax inconsistently.The
StatsFilterfunction at line 369 uses function declaration syntax but ends with a semicolon at line 607 (};), which is typically used for function expressions. This is valid JavaScript but stylistically inconsistent.♻️ Remove trailing semicolon
-}; +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/stats/src/views/Stats/components/stats-filter.tsx` at line 607, The StatsFilter function is declared using function declaration syntax but ends with a stray trailing semicolon (`};`); remove the trailing semicolon to make the declaration style consistent (or alternatively convert the function declaration to a function expression/arrow assigned to a const named StatsFilter if you prefer expression style). Locate the StatsFilter function and either delete the final semicolon after the closing brace or refactor the declaration into a const StatsFilter = (...) => { ... } form so syntax and style are consistent.apps/posts/src/hooks/filter-sources/use-newsletter-value-source.ts (1)
6-18: Unstable memoization due to recreated function reference.The
useNewsletterValueSourceOptionsfunction is recreated on every render, causing the outeruseMemoat line 15-18 to always recompute since it depends on[useNewsletterValueSourceOptions]. Additionally, theoptionsparameter is captured in the closure but not tracked in any dependency array.Consider wrapping the inner function with
useCallbackor restructuring to achieve stable references:♻️ Proposed fix using useCallback
export function useNewsletterValueSource(options: FilterOption<string>[] = []): ValueSource<string> { - const useNewsletterValueSourceOptions = ({query}: ValueSourceParams<string>): ValueSourceResult<string> => { + const useNewsletterValueSourceOptions = useCallback(({query}: ValueSourceParams<string>): ValueSourceResult<string> => { const visibleOptions = useMemo(() => filterOptionsByQuery(options, query), [options, query]); return { options: visibleOptions, isLoading: false }; - }; + }, [options]); return useMemo(() => ({ id: 'posts.newsletters.local', useOptions: useNewsletterValueSourceOptions }), [useNewsletterValueSourceOptions]); }Note: You'll need to add
useCallbackto the imports fromreact.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/posts/src/hooks/filter-sources/use-newsletter-value-source.ts` around lines 6 - 18, The inner function useNewsletterValueSourceOptions is recreated every render, causing the outer useMemo that returns { id: 'posts.newsletters.local', useOptions: useNewsletterValueSourceOptions } to always recompute; wrap useNewsletterValueSourceOptions in React's useCallback so it has a stable reference and include all captured values (e.g., options and filterOptionsByQuery) in its dependency array, then keep the outer useMemo dependent on that stable callback; also add useCallback to the React imports to avoid missing import errors.apps/posts/src/hooks/filter-sources/use-member-value-source.ts (2)
28-30: Simplify conditional filter construction.The conditional spread
...(buildQuotedListFilter('id', selectedValues) ? {filter: buildQuotedListFilter('id', selectedValues)} : {})callsbuildQuotedListFiltertwice. Consider computing once and spreading conditionally:♻️ Proposed simplification
+ const idFilter = buildQuotedListFilter('id', selectedValues); const hydrated = useBrowseMembers({ enabled: selectedValues.length > 0, searchParams: { limit: '100', - ...(buildQuotedListFilter('id', selectedValues) ? {filter: buildQuotedListFilter('id', selectedValues)} : {}) + ...(idFilter && {filter: idFilter}) } });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/posts/src/hooks/filter-sources/use-member-value-source.ts` around lines 28 - 30, Compute the result of buildQuotedListFilter('id', selectedValues') once and reuse it instead of calling it twice; e.g., assign its return to a local variable (e.g., filterValue) inside use-member-value-source.ts and then conditionally spread {filter: filterValue} only when filterValue is truthy, updating the object that currently contains limit: '100' and the spread expression.
47-50: Empty dependency array creates stale closure.The outer
useMemohas an empty dependency array[], butuseMemberValueSourceOptionsis defined inside the hook and captures no external state. However, this pattern is inconsistent with similar hooks (e.g.,useNewsletterValueSourcewhich includes the inner function in deps). The empty array here happens to work because the inner function has no dependencies that change, but it's fragile and inconsistent.For consistency and clarity, either:
- Move
useMemberValueSourceOptionsoutside the hook (if it doesn't need closure variables), or- Use
useCallbackfor the inner function and include it in the dependency array♻️ Suggested consistent pattern
export function useMemberValueSource(): ValueSource<string> { - const useMemberValueSourceOptions = ({query, selectedValues}: ValueSourceParams<string>): ValueSourceResult<string> => { + const useMemberValueSourceOptions = useCallback(({query, selectedValues}: ValueSourceParams<string>): ValueSourceResult<string> => { // ... existing implementation - }; + }, []); return useMemo(() => ({ id: 'posts.members.remote', useOptions: useMemberValueSourceOptions - }), []); + }), [useMemberValueSourceOptions]); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/posts/src/hooks/filter-sources/use-member-value-source.ts` around lines 47 - 50, The outer useMemo in use-member-value-source currently uses an empty dependency array while returning an inner function useMemberValueSourceOptions defined inside the hook, which is inconsistent and fragile; fix by converting useMemberValueSourceOptions into a stable reference (either move the function definition for useMemberValueSourceOptions outside the hook if it does not close over hook-local variables, or wrap it with useCallback inside the hook and then include that callback in the useMemo dependency array) so that the returned object from useMemo stays consistent with patterns like useNewsletterValueSource.apps/shade/test/unit/components/ui/filters.test.tsx (2)
107-109: Prefer.toBeInTheDocument()for DOM presence assertions.Using
.toBeDefined()on the result ofscreen.getByText()will never fail sincegetByTextthrows if the element isn't found. Consider usingtoBeInTheDocument()from@testing-library/jest-domfor clearer intent, or usequeryByTextif you want to test existence.♻️ Suggested change
await waitFor(() => { expect(screen.getAllByText('Published').length).toBeGreaterThan(1); - expect(screen.getByText('Draft')).toBeDefined(); + expect(screen.getByText('Draft')).toBeInTheDocument(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/shade/test/unit/components/ui/filters.test.tsx` around lines 107 - 109, Replace the ambiguous presence assertion using getByText in the test: instead of expecting screen.getByText('Draft') to be defined, assert presence explicitly by using jest-dom's matcher (e.g., expect(screen.getByText('Draft')).toBeInTheDocument()) or switch to queryByText and assert it is truthy; update the assertion in the test case in apps/shade/test/unit/components/ui/filters.test.tsx where screen.getByText('Draft') is used.
139-145: Consider using fake timers for deterministic delay testing.The manual
setTimeoutinsideact()makes the test timing-dependent. Using Vitest's fake timers would be more reliable and faster.♻️ Proposed fix using fake timers
it('resets the local query when the popover closes', async () => { + vi.useFakeTimers(); const useOptions = vi.fn(({query, selectedValues}: {query: string; selectedValues: string[]}) => ({ // ... existing mock })); render(<TestFilters valueSource={{id: 'status', useOptions}} />); openSelectedValuePopover(); const input = await screen.findByPlaceholderText('Search status...'); fireEvent.change(input, {target: {value: 'dra'}}); await waitFor(() => { expect(useOptions).toHaveBeenLastCalledWith({ query: 'dra', selectedValues: ['published'] }); }); const trigger = getSelectedValueTrigger(); fireEvent.click(trigger); - await act(async () => { - await new Promise<void>((resolve) => { - setTimeout(() => { - resolve(); - }, 250); - }); - }); + await act(() => { + vi.advanceTimersByTime(250); + }); fireEvent.click(trigger); const reopenedInput = await screen.findByPlaceholderText('Search status...'); expect((reopenedInput as HTMLInputElement).value).toBe(''); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/shade/test/unit/components/ui/filters.test.tsx` around lines 139 - 145, The test currently waits with a real setTimeout inside act() which is timing-dependent; replace that block with Vitest fake timers: call vi.useFakeTimers() before the wait, remove the setTimeout/promise and instead advance timers with vi.advanceTimersByTime(250) inside act() to flush pending timers, then restore timers with vi.useRealTimers(); update the test around the act(...) call (the block containing the setTimeout Promise) and ensure any async updates triggered by timers are awaited after advancing timers.apps/posts/src/views/PostAnalytics/components/stats-filter.tsx (2)
97-130: Significant code duplication withapps/stats/src/views/Stats/components/stats-filter.tsx.The utility functions
filterOptionsByQuery,mergeFilterOptions,asFilterValueSource, andbuildTinybirdOptionsare duplicated verbatim between this file and the stats app'sstats-filter.tsx. TheuseTinybirdFilterValueSourcehook is also nearly identical.Consider extracting these shared utilities to a common location (e.g.,
apps/posts/src/hooks/filter-sources/utils.tsor a shared package) to reduce maintenance burden and ensure consistent behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/posts/src/views/PostAnalytics/components/stats-filter.tsx` around lines 97 - 130, The four utility functions filterOptionsByQuery, mergeFilterOptions, asFilterValueSource plus buildTinybirdOptions and the useTinybirdFilterValueSource hook are duplicated and should be extracted to a single shared module; create a new utils file (e.g., filter-sources/utils) that exports filterOptionsByQuery, mergeFilterOptions, asFilterValueSource, buildTinybirdOptions and useTinybirdFilterValueSource, move the implementations there, update both posts and stats files to import these symbols, ensure any related types (FilterOption, ValueSource) are exported or re-exported from the shared module, and run typechecks/compile to fix any import/type issues.
255-258: Memoization dependencies may cause unnecessary recreations.The dependency array includes
useTinybirdFilterValueSourceOptions, but this function is recreated on every render since it's defined inline. This will cause theuseMemoto recompute on every render, defeating its purpose.This is the same pattern issue noted in other value source hooks in this PR.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/posts/src/views/PostAnalytics/components/stats-filter.tsx` around lines 255 - 258, The useMemo currently depends on a function defined inline (useTinybirdFilterValueSourceOptions) causing it to recompute every render; make the dependency stable by either moving the useTinybirdFilterValueSourceOptions definition out of the component (or into a useCallback) so its reference is stable, then keep the dependency array as [fieldKey, useTinybirdFilterValueSourceOptions]; alternatively remove the function from the dependency array and depend only on [fieldKey] after ensuring the function is stable—update the useMemo block that returns { id: `posts.post-analytics.${fieldKey}`, useOptions: useTinybirdFilterValueSourceOptions } accordingly.apps/posts/src/views/comments/components/comments-filters.tsx (1)
18-20: Consider extracting sharedasFilterValueSourcehelper.This type assertion helper is duplicated across multiple files (
comments-filters.tsx,stats-filter.tsxin both posts and stats apps). Consider moving it to a shared location (e.g., alongside the value source utilities inapps/posts/src/hooks/filter-sources/utils.ts) or exporting it from@tryghost/shadeto eliminate duplication.🤖 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 18 - 20, The local type-assertion helper asFilterValueSource is duplicated; extract it into a single shared helper and import it where needed: create/export a function named asFilterValueSource(ValueSource<string> -> ValueSource<unknown>) in the shared utilities (for example in the existing filter-sources utils module used by posts: the module containing other value source helpers) or add it to the public API of `@tryghost/shade`, then replace the inline definitions in comments-filters.tsx and stats-filter.tsx by importing that shared asFilterValueSource so all files use the same implementation and keep the type assertions identical.apps/shade/src/components/ui/filters.tsx (1)
1286-1311: Inconsistent controlled mode handling in popover vs inline mode.In the popover rendering path, selected item deselection (lines 1291-1300) uses
valuesdirectly and callsonChange, but doesn't check forfield.onValueChangelike the inline mode does (lines 1141-1153). If controlled mode is used with the popover, this could cause state inconsistencies sinceeffectiveValuesconsidersfield.valuebut the handler writes tovalues.Current usage doesn't trigger this (popover mode isn't used in controlled mode), but this could become a bug if usage patterns change.
💡 Consider aligning with inline mode handling
onSelect={() => { if (isMultiSelect) { - onChange(values.filter(v => v !== option.value) as T[]); + const next = effectiveValues.filter(v => v !== option.value) as T[]; + if (field.onValueChange) { + field.onValueChange(next); + } else { + onChange(next); + } } else { - onChange([] as T[]); + if (field.onValueChange) { + field.onValueChange([] as T[]); + } else { + onChange([] as T[]); + } } if (!isMultiSelect) { setOpen(false); handleClose(); } }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/shade/src/components/ui/filters.tsx` around lines 1286 - 1311, The popover selection handler in CommandItem uses raw values to compute deselection and calls onChange without honoring controlled-mode hooks (it uses values instead of effectiveValues and doesn't call field.onValueChange), which can desync in controlled scenarios; update the onSelect in the popover path to mirror the inline-mode logic: derive the new array from effectiveValues (or use values but then account for field.value), and if field.onValueChange exists call it with the updated value in addition to onChange, and keep the existing setOpen(false)/handleClose behavior for single-select; reference selectedOptions, CommandItem onSelect, isMultiSelect, onChange, values, effectiveValues, and field.onValueChange to locate where to apply the fix.
🤖 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 37-45: The mapped labels currently set id =
String(option.metadata?.id || option.value) which mixes slugs into IDs and can
break API calls (see editLabel/deleteLabel); update the mapping in the useMemo
that builds labels from labelSourceState.options so that id comes only from
option.metadata.id (e.g., set id to String(option.metadata.id) when present and
to null/undefined otherwise or add a separate flag like hasId), and ensure
downstream logic that calls editLabel/deleteLabel uses only labels with a real
metadata.id or fetches the real id first; reference the labels constant,
useMemo, labelSourceState.options and option.metadata?.id to locate and adjust
the logic.
---
Outside diff comments:
In `@apps/posts/src/views/PostAnalytics/components/stats-filter.tsx`:
- Around line 159-174: The file contains an unused dead function
applySelectedPostFilter which is never referenced (and a nearby comment notes
there is no 'post' filter in this context); remove the applySelectedPostFilter
function declaration and any associated types/exports/imports exclusively used
by it to eliminate dead code and simplify the module, leaving other
filter-related helpers like buildFilterParams intact.
---
Nitpick comments:
In `@apps/posts/src/hooks/filter-sources/use-email-post-value-source.ts`:
- Around line 32-39: The code calls buildQuotedListFilter('id', selectedValues)
twice when constructing the searchParams for useBrowsePosts; compute the filter
once into a local const (e.g. const idFilter = buildQuotedListFilter('id',
selectedValues)) and then use idFilter in the conditional and the object value
so useBrowsePosts only receives the filter once, keeping selectedValues,
buildQuotedListFilter and useBrowsePosts as the referenced symbols to locate the
change.
In `@apps/posts/src/hooks/filter-sources/use-member-value-source.ts`:
- Around line 28-30: Compute the result of buildQuotedListFilter('id',
selectedValues') once and reuse it instead of calling it twice; e.g., assign its
return to a local variable (e.g., filterValue) inside use-member-value-source.ts
and then conditionally spread {filter: filterValue} only when filterValue is
truthy, updating the object that currently contains limit: '100' and the spread
expression.
- Around line 47-50: The outer useMemo in use-member-value-source currently uses
an empty dependency array while returning an inner function
useMemberValueSourceOptions defined inside the hook, which is inconsistent and
fragile; fix by converting useMemberValueSourceOptions into a stable reference
(either move the function definition for useMemberValueSourceOptions outside the
hook if it does not close over hook-local variables, or wrap it with useCallback
inside the hook and then include that callback in the useMemo dependency array)
so that the returned object from useMemo stays consistent with patterns like
useNewsletterValueSource.
In `@apps/posts/src/hooks/filter-sources/use-newsletter-value-source.ts`:
- Around line 6-18: The inner function useNewsletterValueSourceOptions is
recreated every render, causing the outer useMemo that returns { id:
'posts.newsletters.local', useOptions: useNewsletterValueSourceOptions } to
always recompute; wrap useNewsletterValueSourceOptions in React's useCallback so
it has a stable reference and include all captured values (e.g., options and
filterOptionsByQuery) in its dependency array, then keep the outer useMemo
dependent on that stable callback; also add useCallback to the React imports to
avoid missing import errors.
In `@apps/posts/src/hooks/filter-sources/use-post-resource-value-source.ts`:
- Around line 47-62: Compute the filter once and reuse it instead of calling
buildQuotedListFilter('id', selectedValues) twice for each query: create a local
const (e.g., const idFilter = buildQuotedListFilter('id', selectedValues))
before the useBrowsePosts/useBrowsePages calls, then use idFilter in the
searchParams spread (e.g., ...(idFilter ? { filter: idFilter } : {})) for both
hydratedPostBrowse and hydratedPageBrowse so the filter is evaluated only once
and the code is clearer.
In `@apps/posts/src/views/comments/components/comments-filters.tsx`:
- Around line 18-20: The local type-assertion helper asFilterValueSource is
duplicated; extract it into a single shared helper and import it where needed:
create/export a function named asFilterValueSource(ValueSource<string> ->
ValueSource<unknown>) in the shared utilities (for example in the existing
filter-sources utils module used by posts: the module containing other value
source helpers) or add it to the public API of `@tryghost/shade`, then replace the
inline definitions in comments-filters.tsx and stats-filter.tsx by importing
that shared asFilterValueSource so all files use the same implementation and
keep the type assertions identical.
In `@apps/posts/src/views/PostAnalytics/components/stats-filter.tsx`:
- Around line 97-130: The four utility functions filterOptionsByQuery,
mergeFilterOptions, asFilterValueSource plus buildTinybirdOptions and the
useTinybirdFilterValueSource hook are duplicated and should be extracted to a
single shared module; create a new utils file (e.g., filter-sources/utils) that
exports filterOptionsByQuery, mergeFilterOptions, asFilterValueSource,
buildTinybirdOptions and useTinybirdFilterValueSource, move the implementations
there, update both posts and stats files to import these symbols, ensure any
related types (FilterOption, ValueSource) are exported or re-exported from the
shared module, and run typechecks/compile to fix any import/type issues.
- Around line 255-258: The useMemo currently depends on a function defined
inline (useTinybirdFilterValueSourceOptions) causing it to recompute every
render; make the dependency stable by either moving the
useTinybirdFilterValueSourceOptions definition out of the component (or into a
useCallback) so its reference is stable, then keep the dependency array as
[fieldKey, useTinybirdFilterValueSourceOptions]; alternatively remove the
function from the dependency array and depend only on [fieldKey] after ensuring
the function is stable—update the useMemo block that returns { id:
`posts.post-analytics.${fieldKey}`, useOptions:
useTinybirdFilterValueSourceOptions } accordingly.
In `@apps/shade/src/components/ui/filters.tsx`:
- Around line 1286-1311: The popover selection handler in CommandItem uses raw
values to compute deselection and calls onChange without honoring
controlled-mode hooks (it uses values instead of effectiveValues and doesn't
call field.onValueChange), which can desync in controlled scenarios; update the
onSelect in the popover path to mirror the inline-mode logic: derive the new
array from effectiveValues (or use values but then account for field.value), and
if field.onValueChange exists call it with the updated value in addition to
onChange, and keep the existing setOpen(false)/handleClose behavior for
single-select; reference selectedOptions, CommandItem onSelect, isMultiSelect,
onChange, values, effectiveValues, and field.onValueChange to locate where to
apply the fix.
In `@apps/shade/test/unit/components/ui/filters.test.tsx`:
- Around line 107-109: Replace the ambiguous presence assertion using getByText
in the test: instead of expecting screen.getByText('Draft') to be defined,
assert presence explicitly by using jest-dom's matcher (e.g.,
expect(screen.getByText('Draft')).toBeInTheDocument()) or switch to queryByText
and assert it is truthy; update the assertion in the test case in
apps/shade/test/unit/components/ui/filters.test.tsx where
screen.getByText('Draft') is used.
- Around line 139-145: The test currently waits with a real setTimeout inside
act() which is timing-dependent; replace that block with Vitest fake timers:
call vi.useFakeTimers() before the wait, remove the setTimeout/promise and
instead advance timers with vi.advanceTimersByTime(250) inside act() to flush
pending timers, then restore timers with vi.useRealTimers(); update the test
around the act(...) call (the block containing the setTimeout Promise) and
ensure any async updates triggered by timers are awaited after advancing timers.
In `@apps/stats/src/views/Stats/components/stats-filter.tsx`:
- Line 607: The StatsFilter function is declared using function declaration
syntax but ends with a stray trailing semicolon (`};`); remove the trailing
semicolon to make the declaration style consistent (or alternatively convert the
function declaration to a function expression/arrow assigned to a const named
StatsFilter if you prefer expression style). Locate the StatsFilter function and
either delete the final semicolon after the closing brace or refactor the
declaration into a const StatsFilter = (...) => { ... } form so syntax and style
are consistent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9bb08c6e-88cf-4193-8d59-379951499ee9
📒 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/pages.tsapps/admin-x-framework/src/api/posts.tsapps/admin-x-framework/src/api/tiers.tsapps/posts/src/components/label-picker/label-filter-renderer.tsxapps/posts/src/components/label-picker/label-picker.tsxapps/posts/src/hooks/filter-sources/use-email-post-value-source.tsapps/posts/src/hooks/filter-sources/use-label-value-source.tsapps/posts/src/hooks/filter-sources/use-member-value-source.tsapps/posts/src/hooks/filter-sources/use-newsletter-value-source.tsapps/posts/src/hooks/filter-sources/use-post-resource-value-source.tsapps/posts/src/hooks/filter-sources/use-tier-value-source.tsapps/posts/src/hooks/filter-sources/utils.tsapps/posts/src/hooks/use-label-picker.tsapps/posts/src/views/PostAnalytics/components/stats-filter.tsxapps/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/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/shade/src/components/ui/filters.tsxapps/shade/test/unit/components/ui/filters.test.tsxapps/stats/src/views/Stats/components/stats-filter.tsx
💤 Files with no reviewable changes (7)
- apps/posts/src/views/comments/hooks/use-search-members.ts
- apps/posts/src/views/comments/hooks/use-search-posts.ts
- apps/posts/src/views/comments/comments.tsx
- apps/posts/test/unit/hooks/use-filter-options.test.tsx
- apps/posts/src/views/comments/hooks/use-known-filter-values.ts
- apps/posts/src/views/comments/hooks/use-filter-options.ts
- apps/posts/src/views/members/hooks/use-resource-search.ts
ref https://linear.app/ghost/issue/BER-3334/ Reduced a small duplicated setup block in the new filter value source tests so the PR addresses low-risk Sonar duplication without dragging the broader stats cleanup into this branch.
ref https://linear.app/ghost/issue/BER-3334/ Stopped deriving label IDs from slugs in the value-source-backed picker so edit and delete actions only operate on real label IDs when the option metadata is present.
ref https://linear.app/ghost/issue/BER-3334/ Removed the stale default React import from the new filters unit test so the shade typecheck passes in CI after the test dedupe change.
ref https://linear.app/ghost/issue/BER-3334/ Dropped the unused newsletter value-source hook now that the members newsletter filters still build explicit slug-based fields and never adopted a searchable value-source path.
ref https://linear.app/ghost/issue/BER-3334/ Pulled the local-to-remote mode decision into a small shared hook so the label value source keeps the hybrid behavior without inlining the switching logic.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/posts/src/hooks/filter-sources/use-hybrid-source-mode.ts (1)
15-20: Mode can become stale after first resolution.At Line 15,
mode !== nullprevents recalculation permanently. Ifpagination/pageLimitchanges later (context switch, refetch with different metadata), this can return an outdated mode. Prefer deriving mode from current inputs (or reset when inputs change).Proposed refactor
-import {useEffect, useState} from 'react'; +import {useMemo} from 'react'; @@ export function useHybridSourceMode( pagination: PaginationLike | undefined, pageLimit: number ): 'local' | 'remote' | null { - const [mode, setMode] = useState<'local' | 'remote' | null>(null); - - useEffect(() => { - if (mode !== null || !pagination) { - return; - } - - setMode(pagination.total <= pageLimit || pagination.next === null ? 'local' : 'remote'); - }, [mode, pageLimit, pagination]); - - return mode; + return useMemo(() => { + if (!pagination) { + return null; + } + return pagination.total <= pageLimit || pagination.next === null ? 'local' : 'remote'; + }, [pagination?.total, pagination?.next, pageLimit]); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/posts/src/hooks/filter-sources/use-hybrid-source-mode.ts` around lines 15 - 20, The current effect in use-hybrid-source-mode prevents recalculation by returning early when mode !== null, causing mode to become stale if pagination or pageLimit change; remove that early return and instead compute a derivedMode from the current pagination and pageLimit (e.g., derivedMode = pagination.total <= pageLimit || pagination.next === null ? 'local' : 'remote') and call setMode(derivedMode) whenever pagination or pageLimit change (keep mode, pageLimit, pagination in the dependency array) or alternatively reset mode to null when inputs change so the effect can recompute; update the effect that references mode, setMode, pageLimit, and pagination accordingly.
🤖 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/filter-sources/use-hybrid-source-mode.ts`:
- Around line 15-20: The current effect in use-hybrid-source-mode prevents
recalculation by returning early when mode !== null, causing mode to become
stale if pagination or pageLimit change; remove that early return and instead
compute a derivedMode from the current pagination and pageLimit (e.g.,
derivedMode = pagination.total <= pageLimit || pagination.next === null ?
'local' : 'remote') and call setMode(derivedMode) whenever pagination or
pageLimit change (keep mode, pageLimit, pagination in the dependency array) or
alternatively reset mode to null when inputs change so the effect can recompute;
update the effect that references mode, setMode, pageLimit, and pagination
accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 70a356ac-b088-4187-b630-4087d11c50af
📒 Files selected for processing (2)
apps/posts/src/hooks/filter-sources/use-hybrid-source-mode.tsapps/posts/src/hooks/filter-sources/use-label-value-source.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/posts/src/hooks/filter-sources/use-label-value-source.ts
ref https://linear.app/ghost/issue/BER-3334/ Removed the remaining value source friction and dead API so filter consumers can use the shared contract directly while members labels reuse the same source path as the rest of the refactor.
kevinansfield
changed the title
ref https://linear.app/ghost/issue/BER-3334/ Tightened the filter source hooks by removing repeated selected-value filter construction, covering the hybrid mode decision with tests, and simplifying the tier source shape so its memoization is easier to follow.
ref https://linear.app/ghost/issue/BER-3334/ Made the hook test props explicitly typed so the posts TypeScript build no longer narrows rerender inputs from the initial literal values.
|
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/
What changed
shadefor dynamic select/multiselect filtersshadepostsshadecoverage for the value source pathWhy
This refactor moves async option lookup behind a simpler, more idiomatic boundary so the filter UI no longer needs to carry search orchestration state or strategy-specific behavior.