Reworked member filters on a shared filter core#26828
Conversation
|
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:
WalkthroughCentralized member filtering and search across the posts app: adds AST parsing, codecs (scalar/text/set/number), filter normalization, filter-query core, member-specific parsing/serialization, field definitions, query-param builders, resource-search filter, and buildMembersUrl. Introduces member filter UI field builders and hooks, refactors members filter state to be timezone-aware and to separate NQL vs search, updates members list/actions to propagate search and hasFilterOrSearch, and adds many unit tests. In admin-x-framework, bulk member mutations accepted optional filter/search and use a shared buildBulkMemberSearchParams helper. 🚥 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)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
E2E Tests FailedTo view the Playwright test report locally, run: REPORT_DIR=$(mktemp -d) && gh run download 23148563662 -n playwright-report -D "$REPORT_DIR" && npx playwright show-report "$REPORT_DIR" |
rob-ghost
left a comment
rob-ghost
left a comment
There was a problem hiding this comment.
My only feedback in the form of a PR you can accept / reject, entirely up to you! Approved regardless! 👍🏻
jonatansberg
force-pushed
the
ber-3414-rework-filters-for-members-only
branch
from
02ae53b to
75a96b6
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e3e5e1ff4b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (2)
apps/posts/src/views/members/member-query-params.ts (1)
22-34: Consider documenting the hardcoded'UTC'timezone.The
resolveFieldcall uses a hardcoded'UTC'timezone rather than the site's configured timezone. This appears intentional sincegetMemberActiveColumnsonly extracts metadata (activeColumn) and doesn't perform timezone-sensitive operations. A brief comment would clarify this is deliberate.💡 Suggested comment
export function getMemberActiveColumns(filters: FilterPredicate[]): ActiveColumn[] { const columns = new Map<string, ActiveColumn>(); for (const filter of filters) { + // Timezone is not used for activeColumn metadata extraction const activeColumn = resolveField(memberFields, filter.field, 'UTC')?.definition.metadata?.activeColumn; if (activeColumn) { columns.set(activeColumn.key, activeColumn); } } return Array.from(columns.values()); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/posts/src/views/members/member-query-params.ts` around lines 22 - 34, Add a brief inline comment in getMemberActiveColumns explaining the intentional use of the hardcoded 'UTC' timezone when calling resolveField(memberFields, filter.field, 'UTC') because this function only reads metadata (activeColumn) and does not perform timezone-sensitive operations; update the comment to mention that using the site/configured timezone is unnecessary here to avoid confusion for future maintainers.apps/posts/src/views/members/components/members-filters.tsx (1)
23-23: Consider simplifying theEMPTY_OFFERStype definition.The conditional type extraction works but is verbose. Since
buildOfferOptionsacceptsOffer[], an explicit type would be clearer:💡 Suggested simplification
-const EMPTY_OFFERS: typeof buildOfferOptions extends (offers: infer T) => unknown ? T : never = []; +const EMPTY_OFFERS: Parameters<typeof buildOfferOptions>[0] = [];Alternatively, import the
Offertype directly if available.🤖 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` at line 23, The EMPTY_OFFERS declaration uses a verbose conditional type; replace it with a simple explicit type annotation (e.g., Offer[] or the concrete array type returned/expected by buildOfferOptions) by importing the Offer type if necessary and changing the declaration of EMPTY_OFFERS to use that explicit type instead of the conditional typeof buildOfferOptions inference; update the symbol EMPTY_OFFERS in members-filters.tsx and ensure buildOfferOptions usage remains compatible.
🤖 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/views/filters/filter-codecs.ts`:
- Around line 54-64: serializeScalarValue currently only quotes strings with
whitespace, causing inputs like "foo+bar", "a,b", "x]" or "O'Brien" to be
emitted raw and break round-tripping for scalarCodec()/setCodec(). Update
serializeScalarValue to call escapeNqlString not only when config?.quoteStrings
or whitespace is present, but also when the string contains NQL metacharacters
(e.g. characters such as + , - , , , ] , [ , ' , " , :) so that any scalar or
set element that would parse differently is escaped; ensure this behavior
applies to values passed through setCodec() since it relies on
serializeScalarValue. Use the existing escapeNqlString helper and keep honoring
config?.quoteStrings.
In `@apps/posts/src/views/members/hooks/use-members-filter-state.ts`:
- Line 8: The SetFiltersAction functional overload on setFilters in
useMembersFilterState is unsafe because its updater is evaluated against the
render-time filters snapshot and drops sequential updates; remove the function
overload and only accept a concrete Filter[] (or, alternatively, resolve
updaters against the current URL-backed state before applying), update any
callsites that pass a function to instead read the current filters from the URL
state (or compute the new array) and pass an explicit Filter[] to setFilters,
and adjust related code in the same file (including the logic referenced around
lines 53-58) to stop relying on the callback form so updates are applied
deterministically.
- Around line 42-47: The code currently calls
parseMemberFilter(searchParams.get('filter'), timezone) before useBrowseSettings
resolves (timezone derived from getSiteTimezone(settingsData?.settings)), which
canonicalizes date predicates to UTC bounds too early; change the useMemo so it
only canonicalizes dates once the site timezone is stable (e.g., check
settingsData or settingsData.settings truthy) — either: (A) delay calling
parseMemberFilter with the timezone until settingsData is available (include
settingsData in the memo deps and return an un-canonicalized/raw parse or
undefined until then), or (B) call parseMemberFilter without the timezone until
settingsData is ready to avoid getDayBoundsInUtc normalization; also ensure any
callers that mutate the URL (setFilters, setSearch, clearFilters) guard against
running while settingsData is unresolved. Reference: useBrowseSettings,
getSiteTimezone, parseMemberFilter, setFilters, setSearch, clearFilters.
In `@apps/posts/src/views/members/member-fields.ts`:
- Around line 16-18: NUMBER_OPERATORS currently only lists
['is','is-greater','is-less'] so inclusive numeric predicates like 'is-or-less'
and 'is-or-greater' are missing and break representation of legacy filters
(e.g., email_count, email_opened_count, email_open_rate). Update the
NUMBER_OPERATORS constant in member-fields.ts to include the inclusive operators
('is-or-less' and 'is-or-greater') so it aligns with the shared numeric
pipeline; verify any switch/handler that consumes NUMBER_OPERATORS (e.g.,
predicate parsing/serialization paths) will accept these new tokens for fields
like email_count, email_opened_count, and email_open_rate.
In `@apps/posts/src/views/members/member-filter-query.ts`:
- Around line 177-183: The code currently only special-cases compound.operator
=== '$and' and falls through to dispatchSimpleNodes for other compounds (e.g.,
'$or'), which can silently drop clauses; modify the logic in the parse path that
uses getCompoundChildren(node) so that if compound exists and compound.operator
!== '$and' you either (a) preserve the original node by returning [node] so it
survives hydration, or (b) explicitly reject unsupported operators by throwing
an informative error (e.g., referencing compound.operator) instead of letting
dispatchSimpleNodes drop it; update the branch around getCompoundChildren,
parseMemberNode, and dispatchSimpleNodes accordingly to implement one of these
behaviors.
In `@apps/posts/src/views/members/member-route.ts`:
- Around line 9-15: The explicit filter option is being set then potentially
overwritten by query.filter during the generic query iteration; change the loop
in member-route.ts that iterates Object.entries(query ?? {}) to skip the
'filter' key (i.e., if (key === 'filter') continue) so the earlier
searchParams.set('filter', filter) always wins, and add a regression test that
supplies both filter and query.filter to ensure the canonical filter is
preserved in the generated members URL.
In `@apps/posts/src/views/members/members.tsx`:
- Around line 126-128: The empty-state message shown when hasFilterOrSearch is
true uses "No members match the current filter", which is incorrect when only
search is active; update the EmptyIndicator usage (the title prop passed to
EmptyIndicator in the members view) to a neutral string such as "No members
match the current criteria" so it accurately covers both filter and search-only
states (replace the existing title text wherever hasFilterOrSearch is used).
In `@apps/posts/src/views/PostAnalytics/Growth/growth.tsx`:
- Line 26: The navigateToMembers helper currently only forwards filter to
buildMembersUrl and so drops the postAnalytics context when navigating with
crossApp: true; update navigateToMembers (and the call-site that builds the URL)
to include the postAnalytics value (e.g., accept or capture a postAnalytics
param or pull it from scope) and pass it into buildMembersUrl alongside filter
so the generated URL retains postAnalytics before calling navigate({crossApp:
true}). Ensure you reference and update the navigateToMembers function and the
buildMembersUrl invocation to include the postAnalytics identifier.
---
Nitpick comments:
In `@apps/posts/src/views/members/components/members-filters.tsx`:
- Line 23: The EMPTY_OFFERS declaration uses a verbose conditional type; replace
it with a simple explicit type annotation (e.g., Offer[] or the concrete array
type returned/expected by buildOfferOptions) by importing the Offer type if
necessary and changing the declaration of EMPTY_OFFERS to use that explicit type
instead of the conditional typeof buildOfferOptions inference; update the symbol
EMPTY_OFFERS in members-filters.tsx and ensure buildOfferOptions usage remains
compatible.
In `@apps/posts/src/views/members/member-query-params.ts`:
- Around line 22-34: Add a brief inline comment in getMemberActiveColumns
explaining the intentional use of the hardcoded 'UTC' timezone when calling
resolveField(memberFields, filter.field, 'UTC') because this function only reads
metadata (activeColumn) and does not perform timezone-sensitive operations;
update the comment to mention that using the site/configured timezone is
unnecessary here to avoid confusion for future maintainers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 387b69a6-a45d-4a9d-9f01-29b75b3673cc
📒 Files selected for processing (40)
apps/admin-x-framework/src/api/members.tsapps/posts/src/nql-lang.d.tsapps/posts/src/views/PostAnalytics/Growth/growth.tsxapps/posts/src/views/PostAnalytics/Newsletter/newsletter.tsxapps/posts/src/views/filters/filter-ast.test.tsapps/posts/src/views/filters/filter-ast.tsapps/posts/src/views/filters/filter-codecs.test.tsapps/posts/src/views/filters/filter-codecs.tsapps/posts/src/views/filters/filter-normalization.test.tsapps/posts/src/views/filters/filter-normalization.tsapps/posts/src/views/filters/filter-query-core.test.tsapps/posts/src/views/filters/filter-query-core.tsapps/posts/src/views/filters/filter-types.test.tsapps/posts/src/views/filters/filter-types.tsapps/posts/src/views/filters/resolve-field.test.tsapps/posts/src/views/filters/resolve-field.tsapps/posts/src/views/members/components/bulk-action-modals/remove-label-modal.tsxapps/posts/src/views/members/components/bulk-action-modals/unsubscribe-modal.tsxapps/posts/src/views/members/components/members-actions.tsxapps/posts/src/views/members/components/members-filters.tsxapps/posts/src/views/members/components/members-header.tsxapps/posts/src/views/members/components/members-list-item.tsxapps/posts/src/views/members/components/members-list.tsxapps/posts/src/views/members/hooks/use-members-filter-config.tsxapps/posts/src/views/members/hooks/use-members-filter-state.test.tsxapps/posts/src/views/members/hooks/use-members-filter-state.tsapps/posts/src/views/members/hooks/use-resource-search.tsapps/posts/src/views/members/member-fields.test.tsapps/posts/src/views/members/member-fields.tsapps/posts/src/views/members/member-filter-query.test.tsapps/posts/src/views/members/member-filter-query.tsapps/posts/src/views/members/member-query-params.test.tsapps/posts/src/views/members/member-query-params.tsapps/posts/src/views/members/member-route.test.tsapps/posts/src/views/members/member-route.tsapps/posts/src/views/members/members.tsxapps/posts/src/views/members/use-member-filter-fields.test.tsapps/posts/src/views/members/use-member-filter-fields.tsapps/posts/vitest.config.tsghost/core/test/unit/server/lib/lexical.test.js
💤 Files with no reviewable changes (1)
- apps/posts/src/views/members/hooks/use-members-filter-config.tsx
jonatansberg
force-pushed
the
ber-3414-rework-filters-for-members-only
branch
from
e3e5e1f to
48ebfcf
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 48ebfcfe86
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Actionable comments posted: 3
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/members/hooks/use-resource-search.ts (1)
15-20:⚠️ Potential issue | 🟡 MinorBackslash characters are not escaped, which can break NQL parsing.
The function escapes single quotes but not backslashes. If a user searches for a string containing a trailing backslash (e.g.,
test\), the resulting filtertitle:~'test\'has\'interpreted as an escaped quote, leaving the string unterminated and causing a parse error.🔧 Proposed fix to escape backslashes
function buildFilter(baseFilter: string, search: string): string { if (!search) { return baseFilter; } - return `${baseFilter}+title:~'${search.replace(/'/g, '\\\'')}'`; + const escaped = search.replace(/\\/g, '\\\\').replace(/'/g, "\\'"); + return `${baseFilter}+title:~'${escaped}'`; }🤖 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-resource-search.ts` around lines 15 - 20, The buildFilter function currently only escapes single quotes but not backslashes, which breaks NQL when a search contains backslashes; update buildFilter to first escape backslashes (replace each "\" with "\\") and then escape single quotes (replace "'" with "\'") before interpolating into `${baseFilter}+title:~'...'`, ensuring both backslashes and quotes in the search string are properly escaped; reference the buildFilter function to locate and modify the string replacement logic.
♻️ Duplicate comments (6)
apps/posts/src/views/members/hooks/use-members-filter-state.ts (2)
8-8:⚠️ Potential issue | 🟡 MinorThe functional
setFiltersoverload is still non-atomic.
action(filters)runs against the memoizedfiltersfrom the current render, not the current URL state. TwosetFilters(prev => ...)calls in the same turn can therefore overwrite each other. Either drop the callback form or resolve it from freshsearchParamsbefore serializing.#!/bin/bash set -euo pipefail echo "Function-form setFilters callsites:" rg -nP --type=ts --type=tsx '\bsetFilters\s*\(\s*[A-Za-z_$][\w$]*\s*=>' apps/posts/src echo echo "useMembersFilterState consumers:" rg -nP --type=ts --type=tsx '\buseMembersFilterState\s*\(' apps/posts/src -C2Also applies to: 18-18, 53-58
🤖 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-members-filter-state.ts` at line 8, The SetFiltersAction supports a functional form but setFilters applies the function to the memoized filters from the render (filters) instead of resolving from the current URL state, making updates non-atomic; update useMembersFilterState so the functional overload is removed or (preferred) implemented atomically by deriving the latest filters from current searchParams inside setFilters: when setFilters is called with a function, first read and parse fresh searchParams (the same way useMembersFilterState does to produce filters), pass that fresh filters into the provided callback, then serialize and write the result back to searchParams; adjust SetFiltersAction/type definitions and the setFilters implementation to reference searchParams parsing/serializing helpers used elsewhere so two functional setFilters calls in the same tick no longer clobber each other (affects SetFiltersAction, setFilters, filters, and useMembersFilterState).
42-68:⚠️ Potential issue | 🔴 CriticalDon't canonicalize date filters until the site timezone is ready.
Before settings load,
getSiteTimezone(settingsData?.settings ?? [])supplies a fallback zone. Any URL date filter is then parsed and reserialized against that fallback, sosetFilters,setSearch, orclearFilterscan rewrite the deep link with different UTC day bounds before the real site timezone arrives. Please gate parse/serialize on resolved settings, or avoid serializing while the timezone is still provisional.🤖 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-members-filter-state.ts` around lines 42 - 68, The code currently parses and reserializes date filters using a provisional timezone before settings load; gate parsing/serializing on the resolved settings by checking settingsData (from useBrowseSettings) is defined. Specifically, only call getSiteTimezone and parseMemberFilter in the filters/search useMemo when settingsData is available (e.g., if settingsData === undefined return undefined or ''), and in setFilters, setSearch and clearFilters bail out (no-op) or queue until settingsData is loaded instead of calling toSearchParams when settingsData is undefined; update references to timezone, parseMemberFilter, toSearchParams, setFilters, setSearch, and clearFilters accordingly so no URL rewrite occurs using the fallback timezone.apps/posts/src/views/members/member-fields.ts (1)
15-16:⚠️ Potential issue | 🟠 MajorAdd the inclusive numeric operators to the schema.
email_count,email_opened_count, andemail_open_rateall useNUMBER_OPERATORS, but that list still omitsis-or-lessandis-or-greater. Hydrating<=or>=member filters then produces predicates the UI cannot represent or round-trip.Proposed fix
-const NUMBER_OPERATORS = ['is', 'is-greater', 'is-less'] as const; +const NUMBER_OPERATORS = ['is', 'is-greater', 'is-or-greater', 'is-less', 'is-or-less'] as const;Also applies to: 371-407
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/posts/src/views/members/member-fields.ts` around lines 15 - 16, NUMBER_OPERATORS currently omits inclusive operators causing filters for email_count, email_opened_count, and email_open_rate to be non-round-trip; update the NUMBER_OPERATORS constant (and any duplicate definitions of it) to include 'is-or-less' and 'is-or-greater' so numeric comparisons support <= and >=, ensuring any place referencing NUMBER_OPERATORS (e.g., member-fields' NUMBER_OPERATORS constant used by email_count/email_opened_count/email_open_rate) is updated consistently.apps/posts/src/views/members/member-filter-query.ts (1)
177-183:⚠️ Potential issue | 🟠 MajorDon't pass unmatched compounds to
dispatchSimpleNodes().When none of the member-specific matchers handle a compound node, this fallback forwards the whole node to
dispatchSimpleNodes(). Inapps/posts/src/views/filters/filter-query-core.ts, that helper returns[]for keys starting with$, so an unsupported$or/compound clause disappears during hydration and the next serialize broadens the query. Preserve or explicitly reject unsupported compounds here instead of silently dropping them.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/posts/src/views/members/member-filter-query.ts` around lines 177 - 183, The current branch forwards unmatched compound nodes to dispatchSimpleNodes() which causes unsupported compound operators (like $or) to be dropped; update the logic in the block that calls getCompoundChildren(node) so that if a compound exists but compound.operator !== '$and' you do not call dispatchSimpleNodes: either return the original node (e.g., return [node]) to preserve it through hydration, or explicitly throw/return a clear error for unsupported compound operators; modify the code around getCompoundChildren, parseMemberNode and dispatchSimpleNodes to implement one of these two behaviors (prefer explicit rejection with a descriptive error mentioning the compound.operator if you want to fail-fast).apps/posts/src/views/members/members.tsx (1)
126-128:⚠️ Potential issue | 🟡 MinorUse neutral empty-state copy for search-only states.
Line 128 still says “current filter”, but
hasFilterOrSearchis also true when onlysearchis active. A neutral title avoids misleading copy in that case.✏️ Suggested tweak
- <EmptyIndicator title="No members match the current filter"> + <EmptyIndicator title="No members match the current criteria">🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/posts/src/views/members/members.tsx` around lines 126 - 128, The empty-state title shown when hasFilterOrSearch is true uses "No members match the current filter", which is misleading when only search is active; update the EmptyIndicator title passed in that conditional (the JSX using hasFilterOrSearch and EmptyIndicator) to a neutral string such as "No members found" or "No members match your search or filters" so it accurately covers both filter and search-only states.apps/posts/src/views/filters/filter-codecs.ts (1)
54-64:⚠️ Potential issue | 🟠 MajorEscape non-safe scalar values before serializing NQL.
serializeScalarValue()still only quotes whitespace. Values likefoo+bar,a,b,-trial, orO'Brienare emitted raw and round-trip into a different filter, which breaks bothscalarCodec()andsetCodec().🛠️ Suggested fix
+const SAFE_UNQUOTED_NQL_VALUE = /^[A-Za-z0-9_./]+(?:-[A-Za-z0-9_./]+)*$/; + function serializeScalarValue(value: unknown, config?: CodecConfig): string { if (typeof value === 'string') { - if (config?.quoteStrings || /\s/.test(value)) { + if (config?.quoteStrings || !SAFE_UNQUOTED_NQL_VALUE.test(value)) { return escapeNqlString(value); } return value; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/posts/src/views/filters/filter-codecs.ts` around lines 54 - 64, serializeScalarValue currently only quotes strings containing whitespace, causing unsafe tokens like "foo+bar", "a,b", "-trial", or "O'Brien" to be emitted raw and break round-trips in scalarCodec() and setCodec(); update serializeScalarValue to call escapeNqlString whenever config?.quoteStrings is true OR the string contains any unsafe characters (e.g. whitespace, plus, comma, single-quote, or a leading hyphen) by replacing the /\s/ check with a regex that matches those characters or a leading hyphen (for example /[\s,+']|^-/) so all such values are escaped before returning.
🧹 Nitpick comments (1)
apps/posts/src/views/members/member-route.test.ts (1)
11-18: Add collision/omission regression cases for query normalization.Nice coverage for preserving extra params. Please also add tests for
query.filtercollision precedence and null/undefined query value omission to lock down expected behavior.💡 Suggested test additions
describe('buildMembersUrl', () => { @@ it('preserves additional query params alongside the canonical filter', () => { expect(buildMembersUrl({ filter: 'emails.post_id:post_123', query: { postAnalytics: 'post_123' } })).toBe('/members?filter=emails.post_id%3Apost_123&postAnalytics=post_123'); }); + + it('prefers canonical filter over query.filter', () => { + expect(buildMembersUrl({ + filter: 'status:paid', + query: { + filter: 'status:free' + } + })).toBe('/members?filter=status%3Apaid'); + }); + + it('omits null/undefined query values', () => { + expect(buildMembersUrl({ + filter: 'status:paid', + query: { + postAnalytics: undefined, + campaign: null, + source: 'growth' + } + })).toBe('/members?filter=status%3Apaid&source=growth'); + }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/posts/src/views/members/member-route.test.ts` around lines 11 - 18, Add two regression tests to member-route.test.ts around buildMembersUrl: one asserting that when callers pass a query object containing filter (e.g., query: {filter: 'x', ...}) the explicit top-level filter parameter (filter: 'y') wins or is used according to the normalization rule (create a test named like "query.filter collision prefers canonical filter" that verifies the expected precedence), and another asserting that query keys with null or undefined values are omitted from the resulting URL (create a test named like "omits null/undefined query values") by passing query entries set to null/undefined and expecting they do not appear in the built URL. Ensure both tests use buildMembersUrl and assert exact string equality of the generated URL to lock down behavior.
🤖 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/members.ts`:
- Around line 199-219: The helper buildBulkMemberSearchParams currently allows
all to be combined with filter or search which can create conflicting bulk
scopes; update buildBulkMemberSearchParams to treat all as mutually exclusive by
throwing an Error if all is true and either filter or search is provided, and
only set params.all when all is true alone; keep existing checks that require at
least one of all/filter/search and continue to populate params.filter and
params.search only when all is not set.
In `@apps/posts/src/views/members/use-member-filter-fields.ts`:
- Around line 402-410: The email_opened_count filter is being added whenever
emailAnalyticsEnabled is true even if open tracking is off; update the block in
use-member-filter-fields.ts so that createFieldConfig('email_opened_count') is
only pushed when emailTrackOpens is true (the same guard used for
createFieldConfig('email_open_rate')), i.e., add email_opened_count to
emailFields conditionally alongside email_open_rate using the emailTrackOpens
flag so open-based filters are hidden when opens are not tracked.
In `@apps/posts/src/views/PostAnalytics/Newsletter/newsletter.tsx`:
- Around line 80-85: Remove the unused postAnalytics query param from the
members navigation: update the navigateToMembers function so it calls
buildMembersUrl without adding the query.postAnalytics field (only provide the
filter and omit postAnalytics), i.e., remove the postAnalytics key from the
object passed to buildMembersUrl used by navigate/navigateToMembers; this keeps
navigation consistent with the members page state handled by
use-members-filter-state.ts which only reads filter and search.
---
Outside diff comments:
In `@apps/posts/src/views/members/hooks/use-resource-search.ts`:
- Around line 15-20: The buildFilter function currently only escapes single
quotes but not backslashes, which breaks NQL when a search contains backslashes;
update buildFilter to first escape backslashes (replace each "\" with "\\") and
then escape single quotes (replace "'" with "\'") before interpolating into
`${baseFilter}+title:~'...'`, ensuring both backslashes and quotes in the search
string are properly escaped; reference the buildFilter function to locate and
modify the string replacement logic.
---
Duplicate comments:
In `@apps/posts/src/views/filters/filter-codecs.ts`:
- Around line 54-64: serializeScalarValue currently only quotes strings
containing whitespace, causing unsafe tokens like "foo+bar", "a,b", "-trial", or
"O'Brien" to be emitted raw and break round-trips in scalarCodec() and
setCodec(); update serializeScalarValue to call escapeNqlString whenever
config?.quoteStrings is true OR the string contains any unsafe characters (e.g.
whitespace, plus, comma, single-quote, or a leading hyphen) by replacing the
/\s/ check with a regex that matches those characters or a leading hyphen (for
example /[\s,+']|^-/) so all such values are escaped before returning.
In `@apps/posts/src/views/members/hooks/use-members-filter-state.ts`:
- Line 8: The SetFiltersAction supports a functional form but setFilters applies
the function to the memoized filters from the render (filters) instead of
resolving from the current URL state, making updates non-atomic; update
useMembersFilterState so the functional overload is removed or (preferred)
implemented atomically by deriving the latest filters from current searchParams
inside setFilters: when setFilters is called with a function, first read and
parse fresh searchParams (the same way useMembersFilterState does to produce
filters), pass that fresh filters into the provided callback, then serialize and
write the result back to searchParams; adjust SetFiltersAction/type definitions
and the setFilters implementation to reference searchParams parsing/serializing
helpers used elsewhere so two functional setFilters calls in the same tick no
longer clobber each other (affects SetFiltersAction, setFilters, filters, and
useMembersFilterState).
- Around line 42-68: The code currently parses and reserializes date filters
using a provisional timezone before settings load; gate parsing/serializing on
the resolved settings by checking settingsData (from useBrowseSettings) is
defined. Specifically, only call getSiteTimezone and parseMemberFilter in the
filters/search useMemo when settingsData is available (e.g., if settingsData ===
undefined return undefined or ''), and in setFilters, setSearch and clearFilters
bail out (no-op) or queue until settingsData is loaded instead of calling
toSearchParams when settingsData is undefined; update references to timezone,
parseMemberFilter, toSearchParams, setFilters, setSearch, and clearFilters
accordingly so no URL rewrite occurs using the fallback timezone.
In `@apps/posts/src/views/members/member-fields.ts`:
- Around line 15-16: NUMBER_OPERATORS currently omits inclusive operators
causing filters for email_count, email_opened_count, and email_open_rate to be
non-round-trip; update the NUMBER_OPERATORS constant (and any duplicate
definitions of it) to include 'is-or-less' and 'is-or-greater' so numeric
comparisons support <= and >=, ensuring any place referencing NUMBER_OPERATORS
(e.g., member-fields' NUMBER_OPERATORS constant used by
email_count/email_opened_count/email_open_rate) is updated consistently.
In `@apps/posts/src/views/members/member-filter-query.ts`:
- Around line 177-183: The current branch forwards unmatched compound nodes to
dispatchSimpleNodes() which causes unsupported compound operators (like $or) to
be dropped; update the logic in the block that calls getCompoundChildren(node)
so that if a compound exists but compound.operator !== '$and' you do not call
dispatchSimpleNodes: either return the original node (e.g., return [node]) to
preserve it through hydration, or explicitly throw/return a clear error for
unsupported compound operators; modify the code around getCompoundChildren,
parseMemberNode and dispatchSimpleNodes to implement one of these two behaviors
(prefer explicit rejection with a descriptive error mentioning the
compound.operator if you want to fail-fast).
In `@apps/posts/src/views/members/members.tsx`:
- Around line 126-128: The empty-state title shown when hasFilterOrSearch is
true uses "No members match the current filter", which is misleading when only
search is active; update the EmptyIndicator title passed in that conditional
(the JSX using hasFilterOrSearch and EmptyIndicator) to a neutral string such as
"No members found" or "No members match your search or filters" so it accurately
covers both filter and search-only states.
---
Nitpick comments:
In `@apps/posts/src/views/members/member-route.test.ts`:
- Around line 11-18: Add two regression tests to member-route.test.ts around
buildMembersUrl: one asserting that when callers pass a query object containing
filter (e.g., query: {filter: 'x', ...}) the explicit top-level filter parameter
(filter: 'y') wins or is used according to the normalization rule (create a test
named like "query.filter collision prefers canonical filter" that verifies the
expected precedence), and another asserting that query keys with null or
undefined values are omitted from the resulting URL (create a test named like
"omits null/undefined query values") by passing query entries set to
null/undefined and expecting they do not appear in the built URL. Ensure both
tests use buildMembersUrl and assert exact string equality of the generated URL
to lock down behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 40d208a6-5595-4210-9995-bd2cdccd1584
📒 Files selected for processing (39)
apps/admin-x-framework/src/api/members.tsapps/posts/src/nql-lang.d.tsapps/posts/src/views/PostAnalytics/Growth/growth.tsxapps/posts/src/views/PostAnalytics/Newsletter/newsletter.tsxapps/posts/src/views/filters/filter-ast.test.tsapps/posts/src/views/filters/filter-ast.tsapps/posts/src/views/filters/filter-codecs.test.tsapps/posts/src/views/filters/filter-codecs.tsapps/posts/src/views/filters/filter-normalization.test.tsapps/posts/src/views/filters/filter-normalization.tsapps/posts/src/views/filters/filter-query-core.test.tsapps/posts/src/views/filters/filter-query-core.tsapps/posts/src/views/filters/filter-types.test.tsapps/posts/src/views/filters/filter-types.tsapps/posts/src/views/filters/resolve-field.test.tsapps/posts/src/views/filters/resolve-field.tsapps/posts/src/views/members/components/bulk-action-modals/remove-label-modal.tsxapps/posts/src/views/members/components/bulk-action-modals/unsubscribe-modal.tsxapps/posts/src/views/members/components/members-actions.tsxapps/posts/src/views/members/components/members-filters.tsxapps/posts/src/views/members/components/members-header.tsxapps/posts/src/views/members/components/members-list-item.tsxapps/posts/src/views/members/components/members-list.tsxapps/posts/src/views/members/hooks/use-members-filter-config.tsxapps/posts/src/views/members/hooks/use-members-filter-state.test.tsxapps/posts/src/views/members/hooks/use-members-filter-state.tsapps/posts/src/views/members/hooks/use-resource-search.tsapps/posts/src/views/members/member-fields.test.tsapps/posts/src/views/members/member-fields.tsapps/posts/src/views/members/member-filter-query.test.tsapps/posts/src/views/members/member-filter-query.tsapps/posts/src/views/members/member-query-params.test.tsapps/posts/src/views/members/member-query-params.tsapps/posts/src/views/members/member-route.test.tsapps/posts/src/views/members/member-route.tsapps/posts/src/views/members/members.tsxapps/posts/src/views/members/use-member-filter-fields.test.tsapps/posts/src/views/members/use-member-filter-fields.tsapps/posts/vitest.config.ts
💤 Files with no reviewable changes (1)
- apps/posts/src/views/members/hooks/use-members-filter-config.tsx
✅ Files skipped from review due to trivial changes (2)
- apps/posts/src/views/members/components/members-list-item.tsx
- apps/posts/src/views/filters/filter-codecs.test.ts
🚧 Files skipped from review as they are similar to previous changes (19)
- apps/posts/src/views/PostAnalytics/Growth/growth.tsx
- apps/posts/src/views/members/member-query-params.test.ts
- apps/posts/src/views/filters/filter-normalization.test.ts
- apps/posts/src/views/members/components/bulk-action-modals/unsubscribe-modal.tsx
- apps/posts/src/views/members/member-fields.test.ts
- apps/posts/src/views/members/member-filter-query.test.ts
- apps/posts/src/views/filters/filter-normalization.ts
- apps/posts/src/views/members/member-route.ts
- apps/posts/src/views/members/hooks/use-members-filter-state.test.tsx
- apps/posts/src/views/filters/filter-query-core.test.ts
- apps/posts/src/views/members/use-member-filter-fields.test.ts
- apps/posts/src/views/members/components/members-list.tsx
- apps/posts/src/views/filters/filter-ast.test.ts
- apps/posts/src/views/filters/filter-types.test.ts
- apps/posts/src/views/filters/resolve-field.ts
- apps/posts/src/views/filters/resolve-field.test.ts
- apps/posts/src/views/members/components/members-header.tsx
- apps/posts/src/views/filters/filter-ast.ts
- apps/posts/src/views/filters/filter-types.ts
There was a problem hiding this comment.
♻️ Duplicate comments (1)
apps/posts/src/views/members/member-filter-query.ts (1)
197-204:⚠️ Potential issue | 🟠 MajorUnsupported compound fallback still drops clauses instead of failing fast.
Line 203 still routes non-
$andcompounds intodispatchSimpleNodes, which can omit operator-only nodes. IfparseMemberFilteris called without the separate unsupported-check gate, hydration can broaden the filter silently.Proposed fix (explicitly reject unsupported compounds)
function parseMemberNode(node: AstNode, timezone: string): ParsedPredicate[] { for (const matcher of MEMBER_COMPOUND_MATCHERS) { const parsed = matcher(node); if (parsed) { return [parsed]; } } const compound = getCompoundChildren(node); - if (compound?.operator === '$and') { - return compound.children.flatMap(child => parseMemberNode(child, timezone)); + if (compound) { + if (compound.operator === '$and') { + return compound.children.flatMap(child => parseMemberNode(child, timezone)); + } + + throw new Error(`Unsupported compound operator in member filter parse: ${compound.operator}`); } return dispatchSimpleNodes([node], memberFields, timezone); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/posts/src/views/members/member-filter-query.ts` around lines 197 - 204, The current fallback routes non-'$and' compound nodes from getCompoundChildren(node) into dispatchSimpleNodes, which can drop operator-only clauses; instead, update parseMemberNode (or the surrounding logic that handles compound) to explicitly reject unsupported compound operators: if compound exists and compound.operator !== '$and', throw a clear error (e.g., "Unsupported compound operator: <operator>") rather than calling dispatchSimpleNodes; keep the existing behavior for operator === '$and' (flatMap parseMemberNode on children) and only call dispatchSimpleNodes for truly simple/non-compound nodes.
🧹 Nitpick comments (4)
apps/posts/src/views/members/hooks/use-members-filter-state.test.tsx (1)
7-15: Consider usingbeforeEach/afterEachfor settings mock state.The mutable
settingsDatavariable at module scope can cause test pollution if tests run in parallel or if a test fails before restoring the value. UsingbeforeEachto set up andafterEachto restore ensures consistent state.♻️ Suggested safer mock pattern
-let settingsData: {settings: Array<{key: string; value: string}>} | undefined = { - settings: [{key: 'timezone', value: 'UTC'}] -}; +const DEFAULT_SETTINGS = { + settings: [{key: 'timezone', value: 'UTC'}] +}; + +let settingsData: {settings: Array<{key: string; value: string}>} | undefined; + +beforeEach(() => { + settingsData = DEFAULT_SETTINGS; +});Then in the specific test that needs undefined settings:
it('preserves raw date filters while settings are unresolved', () => { settingsData = undefined; // ... test code ... - - settingsData = { - settings: [{key: 'timezone', value: 'UTC'}] - }; + // No need to restore - beforeEach handles it });🤖 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-members-filter-state.test.tsx` around lines 7 - 15, The module-scoped mutable settingsData and the vi.mock for useBrowseSettings can cause test pollution; change the mock setup so tests initialize settingsData in a beforeEach (e.g., set settingsData = {settings: [...]} or undefined as needed) and restore/reset it in an afterEach (or call vi.resetModules()/vi.clearAllMocks() there) rather than relying on a top-level variable, keeping the vi.mock('@tryghost/admin-x-framework/api/settings') implementation to read the runtime settingsData value so individual tests can safely override it.apps/posts/src/views/members/hooks/resource-search-filter.test.ts (1)
5-13: Consider adding test for standard search case.The tests cover empty search and escaping edge cases, but a basic test for a normal search term would improve coverage and serve as documentation.
💚 Suggested additional test
it('escapes trailing backslashes in search queries', () => { expect(buildResourceFilter('status:published', 'draft\\')).toBe('status:published+title:~\'draft\\\\\''); }); + + it('appends title filter for non-empty search', () => { + expect(buildResourceFilter('status:published', 'hello')).toBe('status:published+title:~\'hello\''); + }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/posts/src/views/members/hooks/resource-search-filter.test.ts` around lines 5 - 13, Add a basic test that verifies buildResourceFilter returns the base filter combined with a simple (non-escaping) search term: update apps/posts/src/views/members/hooks/resource-search-filter.test.ts to include an it block that calls buildResourceFilter('status:published', 'draft') and asserts it equals "status:published+title:~'draft'"; reference the buildResourceFilter function and the existing test file so the new test mirrors the style of the empty-search and trailing-backslash tests.apps/posts/src/views/members/hooks/resource-search-filter.ts (1)
3-9: Handle emptybaseFilteredge case.If
baseFilteris an empty string, the function will produce+title:~'search'which starts with an invalid+operator. Consider guarding against this or documenting the assumption thatbaseFilteris always non-empty.♻️ Suggested defensive handling
export function buildResourceFilter(baseFilter: string, search: string): string { if (!search) { return baseFilter; } + if (!baseFilter) { + return `title:~${escapeNqlString(search)}`; + } + return `${baseFilter}+title:~${escapeNqlString(search)}`; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/posts/src/views/members/hooks/resource-search-filter.ts` around lines 3 - 9, The buildResourceFilter function can produce an invalid leading '+' when baseFilter is empty; update buildResourceFilter to guard for empty or whitespace-only baseFilter (check baseFilter.trim() or falsy) and return only the title clause `${escapeNqlString(search)}` (i.e., `title:~...`) without a leading '+' in that case; otherwise keep the existing `${baseFilter}+title:~...` behavior so callers of buildResourceFilter and the escapeNqlString helper are handled correctly.apps/posts/src/views/members/member-fields.ts (1)
152-165: Unusual use ofoperatoras a data value.The
feedbackCodecusespredicate.operatoras the feedback score ('1'or'0') rather than as an actual operator. This works but is semantically confusing since operators are expected to describe comparisons (e.g.,is,is-not), not data values.Consider documenting this design choice or refactoring to use a more conventional pattern where the score is in
valuesand the operator is a standard comparison.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/posts/src/views/members/member-fields.ts` around lines 152 - 165, The serialize function in feedbackCodec is misusing predicate.operator as the feedback score; change the shape so the score is carried in predicate.values (e.g., values[0] = postId, values[1] = score) and treat predicate.operator as a real comparison token (validate it against expected operators like 'is'/'is-not' or '='/'!='), update feedbackCodec.serialize to read and validate postId from predicate.values[0] and score from predicate.values[1] (and return null if invalid), and adjust feedbackCodec.parse to produce that same shape so callers set the score in values rather than operator.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@apps/posts/src/views/members/member-filter-query.ts`:
- Around line 197-204: The current fallback routes non-'$and' compound nodes
from getCompoundChildren(node) into dispatchSimpleNodes, which can drop
operator-only clauses; instead, update parseMemberNode (or the surrounding logic
that handles compound) to explicitly reject unsupported compound operators: if
compound exists and compound.operator !== '$and', throw a clear error (e.g.,
"Unsupported compound operator: <operator>") rather than calling
dispatchSimpleNodes; keep the existing behavior for operator === '$and' (flatMap
parseMemberNode on children) and only call dispatchSimpleNodes for truly
simple/non-compound nodes.
---
Nitpick comments:
In `@apps/posts/src/views/members/hooks/resource-search-filter.test.ts`:
- Around line 5-13: Add a basic test that verifies buildResourceFilter returns
the base filter combined with a simple (non-escaping) search term: update
apps/posts/src/views/members/hooks/resource-search-filter.test.ts to include an
it block that calls buildResourceFilter('status:published', 'draft') and asserts
it equals "status:published+title:~'draft'"; reference the buildResourceFilter
function and the existing test file so the new test mirrors the style of the
empty-search and trailing-backslash tests.
In `@apps/posts/src/views/members/hooks/resource-search-filter.ts`:
- Around line 3-9: The buildResourceFilter function can produce an invalid
leading '+' when baseFilter is empty; update buildResourceFilter to guard for
empty or whitespace-only baseFilter (check baseFilter.trim() or falsy) and
return only the title clause `${escapeNqlString(search)}` (i.e., `title:~...`)
without a leading '+' in that case; otherwise keep the existing
`${baseFilter}+title:~...` behavior so callers of buildResourceFilter and the
escapeNqlString helper are handled correctly.
In `@apps/posts/src/views/members/hooks/use-members-filter-state.test.tsx`:
- Around line 7-15: The module-scoped mutable settingsData and the vi.mock for
useBrowseSettings can cause test pollution; change the mock setup so tests
initialize settingsData in a beforeEach (e.g., set settingsData = {settings:
[...]} or undefined as needed) and restore/reset it in an afterEach (or call
vi.resetModules()/vi.clearAllMocks() there) rather than relying on a top-level
variable, keeping the vi.mock('@tryghost/admin-x-framework/api/settings')
implementation to read the runtime settingsData value so individual tests can
safely override it.
In `@apps/posts/src/views/members/member-fields.ts`:
- Around line 152-165: The serialize function in feedbackCodec is misusing
predicate.operator as the feedback score; change the shape so the score is
carried in predicate.values (e.g., values[0] = postId, values[1] = score) and
treat predicate.operator as a real comparison token (validate it against
expected operators like 'is'/'is-not' or '='/'!='), update
feedbackCodec.serialize to read and validate postId from predicate.values[0] and
score from predicate.values[1] (and return null if invalid), and adjust
feedbackCodec.parse to produce that same shape so callers set the score in
values rather than operator.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 79b94f9b-cc0c-4f5f-bd22-1cf9f94954c6
📒 Files selected for processing (14)
apps/posts/src/views/PostAnalytics/Newsletter/newsletter.tsxapps/posts/src/views/filters/filter-codecs.test.tsapps/posts/src/views/filters/filter-codecs.tsapps/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-members-filter-state.test.tsxapps/posts/src/views/members/hooks/use-members-filter-state.tsapps/posts/src/views/members/hooks/use-resource-search.tsapps/posts/src/views/members/member-fields.test.tsapps/posts/src/views/members/member-fields.tsapps/posts/src/views/members/member-filter-query.tsapps/posts/src/views/members/member-route.test.tsapps/posts/src/views/members/member-route.tsapps/posts/src/views/members/members.tsx
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/posts/src/views/members/member-fields.test.ts
- apps/posts/src/views/members/member-route.ts
- apps/posts/src/views/members/member-route.test.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8017c07a5b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
ref https://linear.app/ghost/issue/BER-3414 Replaced the fragmented React filter implementation with the clean-room codec-based flow while leaving planning docs out of branch history
ref https://linear.app/ghost/issue/BER-3414 Replaced the codec fallback loop with explicit field parse aliases, shared the AST node type, removed duplicate predicate stamping, and tightened small consistency issues from review
ref https://linear.app/ghost/issue/BER-3414 Dropped shallow UI tests in favor of keeping coverage at the pure logic and hook layers, leaving UI behavior for the e2e suite
ref https://linear.app/ghost/issue/BER-3414 Reduced the repeated member compound matching loops so the member filter query stays explicit without carrying duplicated control flow.
ref https://linear.app/ghost/issue/BER-3414 Kept grouped retention options while scoping value translation to the offer field only
ref https://linear.app/ghost/issue/BER-3414 Removed duplicated operator label wiring from members and comments hydration hooks
ref https://linear.app/ghost/issue/BER-3414 Matched Ember's recursive compound parsing shape instead of reconstructing compounds from flattened leaves
ref https://linear.app/ghost/issue/BER-3414 Matched the recursive compound parsing model across comments and members and removed dead AST flattening helpers
…app/ghost/issue/BER-3414\nMoved subscribed legacy parsing fully into the member query layer and removed repeated hydration overrides in the member filter field hook
ref https://linear.app/ghost/issue/BER-3414 Cleared apps/posts lint fallout and added the local nql-lang declaration so the filter rewrite branch can build under CI's posts target.
ref https://linear.app/ghost/issue/BER-3414 Restored the comments surface to origin/main so the base branch only carries the shared filter core, the members rewrite, and the member-side API changes.
ref https://linear.app/ghost/issue/BER-3414 The previous revert commit inadvertently left Tailwind class reordering in comment components. This restores them to main so they produce zero diff and won't be linted in CI.
ref https://linear.app/ghost/issue/BER-3414 Moved canonicalizeClauses, normalizeMultiValue, getDayBoundsInUtc, and createOperatorOptions into their single consumer modules as unexported functions. Removed dead code normalizeOperator and deleted the filter-operator-options module. filter-normalization.ts now only exports escapeNqlString, which is the only genuinely shared utility.
ref https://linear.app/ghost/issue/BER-3414 Canonicalized analytics-to-members links to use the members route's filter query param directly instead of relying on Ember's filterParam aliasing. Restored scalar set-filter parsing and singleton offer serialization so existing member deep links like offer_redemptions:'id' continue to round-trip through the React filter pipeline.
ref https://linear.app/ghost/issue/BER-3414 Applied the Node 22 CI lint autofix to the newsletter analytics view on the base branch so the stacked branches stop inheriting this package-lint failure.
ref https://linear.app/ghost/issue/BER-3414 Members filters now sync immediately to URL state, so the codec and field schema need to preserve editable text and number values while restoring missing legacy member filter options.
ref https://linear.app/ghost/issue/BER-3414 Unsupported OR compounds now stay as raw member NQL so the filters UI does not partially represent queries it cannot edit safely.
ref https://linear.app/ghost/issue/BER-3414 The members filters state no longer exposes an unused functional setFilters overload, which removes a stale API path that could evaluate against stale URL-backed state.
ref https://linear.app/ghost/issue/BER-3414 The members empty state now uses neutral wording so it reads correctly for both filter and search-driven empty results.
ref https://linear.app/ghost/issue/BER-3414 The restored subscription status options now satisfy the posts field config types, and the touched posts views use the Tailwind class ordering required by the posts package lint.
ref https://linear.app/ghost/issue/BER-3414 Adjusted the members actions usage and unit test props so the rebased stack still passes Posts type-check and lint.
jonatansberg
force-pushed
the
ber-3414-rework-filters-for-members-only
branch
from
8017c07 to
929e525
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
apps/admin-x-framework/src/api/members.ts (1)
199-219:⚠️ Potential issue | 🟠 MajorMake
allmutually exclusive withfilter/searchin bulk params.Line 206 currently allows
allto be sent together withfilterand/orsearch(Lines 210-216). For bulk edit/delete this creates ambiguous scope and can widen destructive operations depending on backend precedence. Fail closed here.Proposed fix
function buildBulkMemberSearchParams({filter, search, all}: {filter?: string; search?: string; all?: boolean}) { + if (all && (filter || search)) { + throw new Error('Bulk operation cannot combine all with filter or search'); + } + if (!all && !filter && !search) { throw new Error('Bulk operation requires a filter, search, or all flag'); } const params: Record<string, string> = {}; if (all) { params.all = 'true'; + return params; } if (filter) { params.filter = filter; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/admin-x-framework/src/api/members.ts` around lines 199 - 219, The function buildBulkMemberSearchParams currently allows the all flag to be combined with filter/search; update it to fail when all is true alongside filter or search to make the options mutually exclusive. Inside buildBulkMemberSearchParams, add a guard that throws an Error if all && (filter || search) before building params, preserving the existing empty-check (if !all && !filter && !search) and then continue to set params.all, params.filter, params.search as before. Reference: buildBulkMemberSearchParams, and the parameters all, filter, search.apps/posts/src/views/members/use-member-filter-fields.ts (1)
402-410:⚠️ Potential issue | 🟡 Minor
email_opened_countshould be guarded byemailTrackOpens.The
email_opened_countfield is added wheneveremailAnalyticsEnabledis true, but it tracks email opens, which requires open tracking to be enabled. This is inconsistent withemail_open_rateandopened_emails.post_id, which are correctly guarded byemailTrackOpens.🔧 Suggested fix
if (emailAnalyticsEnabled) { const emailFields: FilterFieldConfig[] = [ - createFieldConfig('email_count'), - createFieldConfig('email_opened_count') + createFieldConfig('email_count') ]; if (emailTrackOpens) { - emailFields.push(createFieldConfig('email_open_rate')); + emailFields.push( + createFieldConfig('email_opened_count'), + createFieldConfig('email_open_rate') + ); }🤖 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 402 - 410, The email_opened_count field is currently added whenever emailAnalyticsEnabled is true but it depends on open tracking; modify the block inside the emailAnalyticsEnabled branch so that createFieldConfig('email_opened_count') is only pushed when emailTrackOpens is true (same guard used for createFieldConfig('email_open_rate') and opened_emails.post_id). Locate the emailFields array and move or wrap the createFieldConfig('email_opened_count') addition with an if (emailTrackOpens) check so both email_opened_count and email_open_rate are added only when emailTrackOpens is true.
🧹 Nitpick comments (2)
apps/posts/src/views/members/components/members-actions.tsx (1)
61-61: Consider memoizingmemberOperationParamsto stabilize callback references.
memberOperationParamsis recreated on every render, which causes all dependent callbacks (handleAddLabel,handleRemoveLabel,handleUnsubscribe,handleDelete) to be recreated even whennqlandsearchhaven't changed. This may trigger unnecessary re-renders in child components.♻️ Proposed fix using useMemo
+ const memberOperationParams = useMemo( + () => buildMemberOperationParams({nql, search}), + [nql, search] + ); - const memberOperationParams = buildMemberOperationParams({nql, search});Don't forget to add
useMemoto the React import at line 1.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/posts/src/views/members/components/members-actions.tsx` at line 61, memberOperationParams is recreated each render causing dependent callbacks (handleAddLabel, handleRemoveLabel, handleUnsubscribe, handleDelete) to be unstable; wrap the buildMemberOperationParams call in React.useMemo (or useMemo after importing it) so it only recomputes when nql or search change: replace const memberOperationParams = buildMemberOperationParams({nql, search}); with a useMemo that calls buildMemberOperationParams and lists [nql, search] as dependencies so the callbacks retain stable references.apps/posts/src/views/members/member-fields.ts (1)
152-165: Consider using semantic operator names for feedback codec.The
newsletter_feedbackfield uses'1'and'0'as operator names (line 453), which are unconventional. While this works functionally, using semantic names like'positive'and'negative'(or'like'and'dislike') would improve readability and maintainability.♻️ Optional refactor for clarity
const feedbackCodec: FilterCodec = { parse() { return null; }, serialize(predicate) { const postId = predicate.values[0]; - if (typeof postId !== 'string' || !postId || (predicate.operator !== '1' && predicate.operator !== '0')) { + if (typeof postId !== 'string' || !postId || (predicate.operator !== 'positive' && predicate.operator !== 'negative')) { return null; } - return [`(feedback.post_id:${escapeNqlString(postId)}+feedback.score:${predicate.operator})`]; + const score = predicate.operator === 'positive' ? '1' : '0'; + return [`(feedback.post_id:${escapeNqlString(postId)}+feedback.score:${score})`]; } };And update the field definition:
newsletter_feedback: { - operators: ['1', '0'], + operators: ['positive', 'negative'], ui: { label: 'Responded with feedback', type: 'select', searchable: true, placeholder: 'Select an email...', className: 'w-64', - defaultOperator: '1' + defaultOperator: 'positive' },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/posts/src/views/members/member-fields.ts` around lines 152 - 165, The feedbackCodec currently uses numeric operator strings ('1' and '0') in serialize to determine feedback.score; update it to use semantic operator names (e.g., 'positive' and 'negative' or 'like' and 'dislike') by changing the operator checks in feedbackCodec.serialize to accept and map those names to the corresponding score values, and update any related field definition (newsletter_feedback) to emit the new operator names instead of '1'/'0' so the codec and field remain consistent; adjust any consumer code that constructs predicates to use the new operator tokens.
🤖 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/views/members/hooks/use-members-filter-state.test.tsx`:
- Around line 7-10: The shared mutable variable settingsData is mutated during a
test which can leak state to later tests; fix by resetting or reinitializing
settingsData in a beforeEach (or creating an immutable factory and assigning
settingsData = createDefaultSettings() inside beforeEach) so every test gets a
fresh {settings: [{key: 'timezone', value: 'UTC'}]} instance; reference the
settingsData variable in use-members-filter-state.test.tsx and ensure any test
that mutates it does not rely on teardown to restore state.
---
Duplicate comments:
In `@apps/admin-x-framework/src/api/members.ts`:
- Around line 199-219: The function buildBulkMemberSearchParams currently allows
the all flag to be combined with filter/search; update it to fail when all is
true alongside filter or search to make the options mutually exclusive. Inside
buildBulkMemberSearchParams, add a guard that throws an Error if all && (filter
|| search) before building params, preserving the existing empty-check (if !all
&& !filter && !search) and then continue to set params.all, params.filter,
params.search as before. Reference: buildBulkMemberSearchParams, and the
parameters all, filter, search.
In `@apps/posts/src/views/members/use-member-filter-fields.ts`:
- Around line 402-410: The email_opened_count field is currently added whenever
emailAnalyticsEnabled is true but it depends on open tracking; modify the block
inside the emailAnalyticsEnabled branch so that
createFieldConfig('email_opened_count') is only pushed when emailTrackOpens is
true (same guard used for createFieldConfig('email_open_rate') and
opened_emails.post_id). Locate the emailFields array and move or wrap the
createFieldConfig('email_opened_count') addition with an if (emailTrackOpens)
check so both email_opened_count and email_open_rate are added only when
emailTrackOpens is true.
---
Nitpick comments:
In `@apps/posts/src/views/members/components/members-actions.tsx`:
- Line 61: memberOperationParams is recreated each render causing dependent
callbacks (handleAddLabel, handleRemoveLabel, handleUnsubscribe, handleDelete)
to be unstable; wrap the buildMemberOperationParams call in React.useMemo (or
useMemo after importing it) so it only recomputes when nql or search change:
replace const memberOperationParams = buildMemberOperationParams({nql, search});
with a useMemo that calls buildMemberOperationParams and lists [nql, search] as
dependencies so the callbacks retain stable references.
In `@apps/posts/src/views/members/member-fields.ts`:
- Around line 152-165: The feedbackCodec currently uses numeric operator strings
('1' and '0') in serialize to determine feedback.score; update it to use
semantic operator names (e.g., 'positive' and 'negative' or 'like' and
'dislike') by changing the operator checks in feedbackCodec.serialize to accept
and map those names to the corresponding score values, and update any related
field definition (newsletter_feedback) to emit the new operator names instead of
'1'/'0' so the codec and field remain consistent; adjust any consumer code that
constructs predicates to use the new operator tokens.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 14a774e3-de30-419d-9f16-1c4f42155277
📒 Files selected for processing (42)
apps/admin-x-framework/src/api/members.tsapps/posts/src/nql-lang.d.tsapps/posts/src/views/PostAnalytics/Growth/growth.tsxapps/posts/src/views/PostAnalytics/Newsletter/newsletter.tsxapps/posts/src/views/filters/filter-ast.test.tsapps/posts/src/views/filters/filter-ast.tsapps/posts/src/views/filters/filter-codecs.test.tsapps/posts/src/views/filters/filter-codecs.tsapps/posts/src/views/filters/filter-normalization.test.tsapps/posts/src/views/filters/filter-normalization.tsapps/posts/src/views/filters/filter-query-core.test.tsapps/posts/src/views/filters/filter-query-core.tsapps/posts/src/views/filters/filter-types.test.tsapps/posts/src/views/filters/filter-types.tsapps/posts/src/views/filters/resolve-field.test.tsapps/posts/src/views/filters/resolve-field.tsapps/posts/src/views/members/components/bulk-action-modals/remove-label-modal.tsxapps/posts/src/views/members/components/bulk-action-modals/unsubscribe-modal.tsxapps/posts/src/views/members/components/members-actions.tsxapps/posts/src/views/members/components/members-filters.tsxapps/posts/src/views/members/components/members-header.tsxapps/posts/src/views/members/components/members-list-item.tsxapps/posts/src/views/members/components/members-list.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-members-filter-config.tsxapps/posts/src/views/members/hooks/use-members-filter-state.test.tsxapps/posts/src/views/members/hooks/use-members-filter-state.tsapps/posts/src/views/members/hooks/use-resource-search.tsapps/posts/src/views/members/member-fields.test.tsapps/posts/src/views/members/member-fields.tsapps/posts/src/views/members/member-filter-query.test.tsapps/posts/src/views/members/member-filter-query.tsapps/posts/src/views/members/member-query-params.test.tsapps/posts/src/views/members/member-query-params.tsapps/posts/src/views/members/member-route.test.tsapps/posts/src/views/members/member-route.tsapps/posts/src/views/members/members.tsxapps/posts/src/views/members/use-member-filter-fields.test.tsapps/posts/src/views/members/use-member-filter-fields.tsapps/posts/test/unit/views/members/members-actions.test.tsxapps/posts/vitest.config.ts
💤 Files with no reviewable changes (1)
- apps/posts/src/views/members/hooks/use-members-filter-config.tsx
✅ Files skipped from review due to trivial changes (1)
- apps/posts/src/views/members/components/members-header.tsx
🚧 Files skipped from review as they are similar to previous changes (20)
- apps/posts/src/nql-lang.d.ts
- apps/posts/src/views/members/member-fields.test.ts
- apps/posts/src/views/PostAnalytics/Newsletter/newsletter.tsx
- apps/posts/src/views/members/member-route.ts
- apps/posts/src/views/filters/filter-normalization.ts
- apps/posts/src/views/members/hooks/resource-search-filter.ts
- apps/posts/src/views/members/components/bulk-action-modals/unsubscribe-modal.tsx
- apps/posts/src/views/members/components/members-list.tsx
- apps/posts/vitest.config.ts
- apps/posts/src/views/PostAnalytics/Growth/growth.tsx
- apps/posts/src/views/members/hooks/resource-search-filter.test.ts
- apps/posts/src/views/filters/filter-ast.ts
- apps/posts/src/views/members/member-filter-query.ts
- apps/posts/src/views/filters/filter-query-core.ts
- apps/posts/src/views/filters/filter-types.test.ts
- apps/posts/src/views/members/member-route.test.ts
- apps/posts/src/views/members/hooks/use-resource-search.ts
- apps/posts/src/views/members/use-member-filter-fields.test.ts
- apps/posts/src/views/members/components/members-list-item.tsx
- apps/posts/src/views/members/member-query-params.test.ts
apps/posts/src/views/members/hooks/use-members-filter-state.test.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 929e52535b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
ref https://linear.app/ghost/issue/BER-3414 Legacy Ember member date URLs encode UTC timestamps without a timezone suffix, so non-UTC sites parsed them as local dates and shifted saved segments.
ref https://linear.app/ghost/issue/BER-3414 Members views should block stale refetch interactions and fail closed on bulk delete when only preserved raw filters are available.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
apps/posts/src/views/members/members-view-state.test.ts (1)
4-16: Consider expanding test coverage.The current tests cover only two specific scenarios. Additional edge cases would strengthen confidence:
shouldShowMembersLoading: test whenisFetchingNextPageistrue(should returnfalse)canBulkDeleteMembers: test with filters containing restricted fields, test with filters containing only allowed fields🧪 Additional test cases
it('hides loading state during pagination fetches', () => { expect(shouldShowMembersLoading({ isFetching: true, isFetchingNextPage: true })).toBe(false); }); it('allows bulk delete when no restricted filters are present', () => { expect(canBulkDeleteMembers([{field: 'name'}])).toBe(true); }); it('disables bulk delete when a restricted filter field is present', () => { expect(canBulkDeleteMembers([{field: 'subscriptions.status'}])).toBe(false); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/posts/src/views/members/members-view-state.test.ts` around lines 4 - 16, Add the missing edge-case tests for shouldShowMembersLoading and canBulkDeleteMembers: add a test calling shouldShowMembersLoading with isFetching: true and isFetchingNextPage: true (and optionally other flags) and assert it returns false; add a test calling canBulkDeleteMembers with filters that contain only allowed fields (e.g., [{field: 'name'}]) and assert true; and add a test calling canBulkDeleteMembers with a filter containing a restricted field (e.g., 'subscriptions.status') and assert false; place these tests alongside existing ones in members-view-state.test.ts referencing the same helper functions shouldShowMembersLoading and canBulkDeleteMembers.apps/posts/src/views/members/members.tsx (1)
149-149: Inconsistent loading logic with centralized helper.The
isLoadingprop uses inline logicisFetching && !isFetchingNextPage, while lines 50-55 useshouldShowMembersLoadingfor the same purpose. Consider using the centralized helper here as well for consistency.- isLoading={isFetching && !isFetchingNextPage} + isLoading={shouldShowLoading}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/posts/src/views/members/members.tsx` at line 149, The isLoading prop currently uses inline logic (isFetching && !isFetchingNextPage) which duplicates logic already centralized in shouldShowMembersLoading; replace the inline expression with the centralized helper by passing shouldShowMembersLoading to the isLoading prop so the component uses the same loading decision as the rest of the file (ensure you reference the existing shouldShowMembersLoading variable instead of isFetching/isFetchingNextPage directly).
🤖 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/views/members/member-fields.ts`:
- Around line 138-156: The serialize function is interpolating ctx.params.slug
directly into NQL which can produce malformed queries for slugs with special
characters; update member-fields.ts to import and use the existing
escapeNqlString utility (as used in feedbackCodec) and substitute escapedSlug =
escapeNqlString(slug) before constructing the two return strings in serialize
(for both 'subscribed' and 'unsubscribed' branches), ensuring you apply the
escaped value in the `(newsletters.slug:...)` and `newsletters.slug:-...`
patterns.
In `@apps/posts/src/views/members/members-view-state.ts`:
- Around line 22-29: The function signature for shouldShowMembersLoading
currently only accepts {isFetching, isFetchingNextPage} but call sites
(members.tsx and the related test) pass additional properties isRefetching and
memberCount causing TypeScript excess property errors; either expand the
parameter type to include isRefetching?: boolean and memberCount?: number (and
update the function body only if those values should affect logic) or leave the
logic unchanged and update all call sites to pass only { isFetching,
isFetchingNextPage }; locate the function shouldShowMembersLoading and the call
sites in members.tsx and the test to apply the chosen fix consistently.
---
Nitpick comments:
In `@apps/posts/src/views/members/members-view-state.test.ts`:
- Around line 4-16: Add the missing edge-case tests for shouldShowMembersLoading
and canBulkDeleteMembers: add a test calling shouldShowMembersLoading with
isFetching: true and isFetchingNextPage: true (and optionally other flags) and
assert it returns false; add a test calling canBulkDeleteMembers with filters
that contain only allowed fields (e.g., [{field: 'name'}]) and assert true; and
add a test calling canBulkDeleteMembers with a filter containing a restricted
field (e.g., 'subscriptions.status') and assert false; place these tests
alongside existing ones in members-view-state.test.ts referencing the same
helper functions shouldShowMembersLoading and canBulkDeleteMembers.
In `@apps/posts/src/views/members/members.tsx`:
- Line 149: The isLoading prop currently uses inline logic (isFetching &&
!isFetchingNextPage) which duplicates logic already centralized in
shouldShowMembersLoading; replace the inline expression with the centralized
helper by passing shouldShowMembersLoading to the isLoading prop so the
component uses the same loading decision as the rest of the file (ensure you
reference the existing shouldShowMembersLoading variable instead of
isFetching/isFetchingNextPage directly).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ff021646-b860-42c5-a34c-8d1142e3963b
📒 Files selected for processing (5)
apps/posts/src/views/members/member-fields.tsapps/posts/src/views/members/member-filter-query.test.tsapps/posts/src/views/members/members-view-state.test.tsapps/posts/src/views/members/members-view-state.tsapps/posts/src/views/members/members.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/posts/src/views/members/member-filter-query.test.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5370206da8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
ref https://linear.app/ghost/issue/BER-3414 Date filters should stay raw until the site timezone is known, and the members loading helper should match its simplified non-pagination contract.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2a7c4d209f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
ref https://linear.app/ghost/issue/BER-3414 Unsupported member OR clauses should be discarded instead of preserved through the hook, while unresolved date filters still need to survive URL rewrites until the site timezone is known.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b4fb07b059
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| hasFilterOrSearch={hasFilterOrSearch} | ||
| memberCount={totalMembers} | ||
| nql={nql} | ||
| search={search} |
There was a problem hiding this comment.
Now that search is threaded into the members query and bulk/export actions, opening /members?search=... produces a genuinely scoped view, but this screen still derives its visible filter/reset state from filters.length only. On the current React members page there is no separate search control, so a search-only URL narrows the list and scopes destructive actions without showing any active chip/input or a way to clear that scope unless the result set is empty. The old members UX treated search as first-class state (ghost/admin/tests/acceptance/members/filter-test.js:1592-1626), so this becomes a confusing and easy-to-miss regression for shared/bookmarked search links.
Useful? React with 👍 / 👎.
| const compound = getCompoundChildren(node); | ||
|
|
||
| if (!compound || compound.children.length !== 2) { | ||
| return null; |
There was a problem hiding this comment.
subscribed:true|false member filters
The members API still accepts simple NQL like subscribed:true and subscribed:false (ghost/core/test/e2e-api/admin/members-newsletters.test.js:79-99), but this parser only recognizes the compound (subscribed...+email_disabled...) shapes. Because the simple-node fallback also has no subscribed parser, opening a URL such as /members?filter=subscribed:true now hydrates to filters=[]/nql=undefined, so the list and bulk actions silently fall back to the full audience instead of the subscribed segment.
Useful? React with 👍 / 👎.
ref https://linear.app/ghost/issue/BER-3414 Members filters now parse best-effort from the URL and canonicalize unsupported clauses away, while the page waits for site settings before hydrating timezone-sensitive date filters.
ref https://linear.app/ghost/issue/BER-3414 Best-effort member filter parsing now canonicalizes legacy subscribed booleans through the existing subscribed filter model.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: af3321313c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (activeNewsletters.length > 1) { | ||
| const newsletterFields: FilterFieldConfig[] = [ | ||
| createFieldConfig('subscribed', { | ||
| label: 'All newsletters', | ||
| options: [ |
There was a problem hiding this comment.
If a user opens a saved segment like ?filter=newsletters.slug:weekly+email_disabled:0 after the site is down to a single active newsletter, this branch stops registering any newsletters.{slug} field and only exposes the generic subscribed filter. useMembersFilterState() still parses and applies the newsletter-specific NQL, but Shade silently drops filters whose field config is missing (apps/shade/src/components/ui/filters.tsx:1849-1852), so the list/export/delete remain scoped with no visible chip or way to clear that constraint. The same hidden-scope problem also occurs for archived newsletters because members-filters.tsx filters them out before building the field map.
Useful? React with 👍 / 👎.
| const shouldDelayHydration = shouldDelayMembersDateFilterHydration(filterParam, Boolean(settingsData)); | ||
|
|
||
| if (shouldDelayHydration) { |
There was a problem hiding this comment.
For any members URL containing a date predicate, shouldDelayMembersDateFilterHydration() stays true until settingsData becomes truthy. If the settings request errors or never resolves, this component never reaches the normal members error/render path and just keeps showing the loading spinner, so bookmarked views like ?filter=created_at:<=... become unusable during a transient settings failure. This needs an isError/isLoading check or a fallback path instead of gating solely on Boolean(settingsData).
Useful? React with 👍 / 👎.
ref https://linear.app/ghost/issue/BER-3414 Hydrated newsletter filters now stay visible when their runtime field config would otherwise disappear, and date-filter loading only blocks while site settings are still actively loading.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b0951ac992
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (typeof node.subscribed === 'boolean') { | ||
| return { | ||
| field: 'subscribed', | ||
| operator: 'is', | ||
| values: [node.subscribed ? 'subscribed' : 'unsubscribed'] |
There was a problem hiding this comment.
subscribed:true|false URLs
Fresh evidence: matchSubscribedNode() now hydrates plain subscribed:true|false, but useMembersFilterState() immediately canonicalizes those predicates through subscribedCodec.serialize() into (subscribed:true+email_disabled:0) / (subscribed:false+email_disabled:0). The backend still treats raw subscribed as a pure newsletter-status alias (ghost/core/core/server/api/endpoints/utils/serializers/input/members.js:18-31), so opening an existing URL like ?filter=subscribed:false now silently narrows the segment by excluding email-disabled members before any list/export/delete action runs.
Useful? React with 👍 / 👎.
| return compound.children.flatMap(child => parseMemberNode(child, timezone)); | ||
| } | ||
|
|
||
| return dispatchSimpleNodes([node], memberFields, timezone); |
There was a problem hiding this comment.
The simple-node fallback only understands keys registered in memberFields, so backend-supported aliases such as ?filter=newsletters:weekly-newsletter and ?filter=tier:default-product fall through here and parse as no filters. On the old members route those raw aliases were still passed through to the API (ghost/admin/app/controllers/members.js:298-333), but the new hook rewrites an empty parse result to no filter, so the list and bulk actions broaden to the full audience. Ghost still accepts these aliases (ghost/core/core/server/models/member.js:61-84, ghost/core/test/e2e-api/admin/members.test.js:2707-2726).
Useful? React with 👍 / 👎.
jonatansberg
added
preview
ref https://linear.app/ghost/issue/BER-3414 Avoid invalid empty-string NQL by keeping incomplete text filters local until they can be serialized.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c7609201f6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| useEffect(() => { | ||
| if (currentQuery !== lastWrittenQueryRef.current) { | ||
| setDraftFilters(parsedFilters); | ||
| lastWrittenQueryRef.current = currentQuery; |
There was a problem hiding this comment.
When the page first falls back to UTC because settings data is absent but not loading (for example after a transient settings error) and later rerenders with the real site timezone, parsedFilters changes but this guard does not copy it into filters because the query string is unchanged. The next effect then serializes the stale UTC-derived day under the new timezone, rewriting filter= to the wrong date and retargeting the members list, export, and bulk actions on non-UTC sites.
Useful? React with 👍 / 👎.
4 participants
Summary
membersForwardrollout model unchangedWhy this change
The existing members filter stack had too many overlapping sources of truth across field configuration, query serialization, and runtime state. This branch consolidates that logic into a smaller shared core and uses it to drive the React members experience behind the existing
membersForwardrollout.The comments moderation rewrite is intentionally not included here. That work now lives in the follow-up stacked PR linked below so this PR can stand on its own as a mergeable members-focused change.
Follow-up PR
What changed
apps/posts/src/views/filters/for:searchhandling alongsidefilteruse-members-filter-configpath and updated the member actions/query-param plumbing to use the new state model@tryghost/nql-langtyping shim required by the new posts-side importsTest plan
yarn --cwd apps/posts tsc --noEmityarn --cwd apps/posts lintyarn --cwd apps/posts vitest run src/views/filters/filter-query-core.test.ts src/views/filters/filter-ast.test.ts src/views/filters/filter-codecs.test.ts src/views/filters/filter-operator-options.test.ts src/views/filters/filter-normalization.test.ts src/views/filters/filter-types.test.ts src/views/filters/resolve-field.test.ts src/views/members/member-query-params.test.ts src/views/members/member-fields.test.ts src/views/members/use-member-filter-fields.test.ts src/views/members/member-filter-query.test.ts src/views/members/hooks/use-members-filter-state.test.tsx