Conversation
WalkthroughThis change adds a React-based members management interface gated by the "membersForward" feature flag. It extends the members API with new public types (labels, tiers, newsletters, subscriptions) and augments the Member type with additional fields. New client hooks provide infinite browsing and bulk member operations (edit, delete). A React members page, list, filters, actions, and several bulk-action modals are introduced, plus URL-backed filter state management. Routing and Ember templates are updated to conditionally route/render the React view while preserving the Ember fallback. Minor UI adjustments to filter popovers and a new lab flag are included. 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
E2E Tests FailedTo view the Playwright test report locally, run: REPORT_DIR=$(mktemp -d) && gh run download 21880026498 -n playwright-report -D "$REPORT_DIR" && npx playwright show-report "$REPORT_DIR" |
E2E Tests FailedTo view the Playwright test report locally, run: REPORT_DIR=$(mktemp -d) && gh run download 21884316502 -n playwright-report -D "$REPORT_DIR" && npx playwright show-report "$REPORT_DIR" |
weylandswart
force-pushed
the
port-members-list-from-ember-to-react
branch
from
7e0c093 to
8ce6c23
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #26334 +/- ##
=======================================
Coverage 72.77% 72.78%
=======================================
Files 1562 1562
Lines 120898 120905 +7
Branches 14558 14564 +6
=======================================
+ Hits 87984 87995 +11
+ Misses 31881 31874 -7
- Partials 1033 1036 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
E2E Tests FailedTo view the Playwright test report locally, run: REPORT_DIR=$(mktemp -d) && gh run download 21903576179 -n playwright-report -D "$REPORT_DIR" && npx playwright show-report "$REPORT_DIR" |
E2E Tests FailedTo view the Playwright test report locally, run: REPORT_DIR=$(mktemp -d) && gh run download 21904425147 -n playwright-report -D "$REPORT_DIR" && npx playwright show-report "$REPORT_DIR" |
E2E Tests FailedTo view the Playwright test report locally, run: REPORT_DIR=$(mktemp -d) && gh run download 21907494267 -n playwright-report -D "$REPORT_DIR" && npx playwright show-report "$REPORT_DIR" |
E2E Tests FailedTo view the Playwright test report locally, run: REPORT_DIR=$(mktemp -d) && gh run download 21944183858 -n playwright-report -D "$REPORT_DIR" && npx playwright show-report "$REPORT_DIR" |
1 similar comment
E2E Tests FailedTo view the Playwright test report locally, run: REPORT_DIR=$(mktemp -d) && gh run download 21944183858 -n playwright-report -D "$REPORT_DIR" && npx playwright show-report "$REPORT_DIR" |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In `@apps/admin-x-framework/src/api/members.ts`:
- Around line 214-224: The searchParams callback in useBulkEditMembers and
useBulkDeleteMembers can return an empty params object when called with {filter:
'', all: false}, causing server-side errors; update the searchParams
implementation in both functions (the searchParams: ({filter, all}) => { ... }
block) to validate inputs and throw an error if neither a non-empty filter (use
filter.trim() to treat whitespace as empty) nor all === true is provided, so
callers fail fast with a clear message instead of sending an invalid request.
In `@apps/posts/src/views/members/components/bulk-action-modals/delete-modal.tsx`:
- Around line 41-44: The handleExportBackup function doesn't handle rejections
from onExportBackup(), leaving the UI stuck in an exporting state; wrap the
await onExportBackup() call in a try/catch inside handleExportBackup, in the try
setBackupExported(true) as now, and in the catch reset any exporting flags
(e.g., setBackupExported(false) or the component's isExporting state) and
surface an error to the user (toast, setExportError state, or similar) so
failures clear the spinner and show feedback; reference handleExportBackup,
onExportBackup, and setBackupExported when making the changes.
In `@apps/posts/src/views/members/components/members-actions.tsx`:
- Around line 133-135: handleExportBackup currently awaits exportMembers(nql)
without handling failures, so an exception leaves DeleteModal stuck spinning;
wrap the call inside handleExportBackup in a try/catch and either return a
success boolean (true on success, false on failure) or rethrow a wrapped error
so the caller can distinguish failure. Then update DeleteModal to call
handleExportBackup and catch errors (instead of unconditionally calling
setBackupExported(true)) — on catch set the backup failure UI state (e.g.,
setBackupExported(false) and set an error flag/message) so the export button
shows an error rather than spinning forever. Ensure you reference
handleExportBackup, exportMembers, DeleteModal and setBackupExported when making
the change.
- Around line 78-131: The handlers (handleAddLabel, handleRemoveLabel,
handleUnsubscribe, handleDelete) currently call bulkEdit/bulkDelete with filter:
nql || '' and all: !nql which makes all=true when only a search term is active;
fix by incorporating the search term into the bulk payload: use filter: nql ||
searchQuery || '' and set all: !(nql || searchQuery) so operations apply only to
the NQL or current search results, and add searchQuery to each useCallback
dependency list (or alternatively hide the bulk UI unless nql is non-empty).
In `@apps/posts/src/views/members/hooks/use-members-filter-state.ts`:
- Around line 207-215: The generated NQL for the 'audience_feedback' case is
adding an unnecessary `:>0` prefix which makes `is`/`is_any_of` produce
contradictory filters (e.g.
`audience_feedback.score:>0+audience_feedback.score:0`); update the
'audience_feedback' branch (the switch case handling operator and pushing into
`parts`) to stop prepending `audience_feedback.score:>0+` and instead push a
direct equality segment like `audience_feedback.score:${value}` for `operator
=== 'is'` and `operator === 'is_any_of'` (preserving the existing negation
behavior for the else branch that pushes `audience_feedback.score:-${value}`).
In `@apps/posts/src/views/members/members.tsx`:
- Around line 83-106: MembersFilters is being unmounted/remounted when
hasFilters toggles because it's rendered in two different conditional branches;
keep a single MembersFilters instance to preserve its internal state by moving
the <MembersFilters filters={filters} onFiltersChange={setFilters} /> out of the
conditionals and control its visual placement with CSS (use filtersClassName and
hasFilters to apply a different class/positioning), or alternatively always
render it inside one container and toggle visibility via a className derived
from hasFilters; ensure you remove the duplicate MembersFilters render so only
the unique component instance (MembersFilters) remains and continue passing
filters and setFilters props as before.
🧹 Nitpick comments (8)
apps/posts/src/views/members/hooks/use-members-filter-state.ts (1)
81-92: Empty string''is silently skipped by the guard on Line 85.
!filter.values[0]istruefor'', so text filters where the user typed and then cleared the input would be silently dropped. This appears intentional and correct (no point queryingname:~''), but worth confirming that the UI prevents submitting filters with empty text values, otherwise the user could see a filter chip with no corresponding NQL clause.apps/admin/src/routes/members-route.tsx (1)
20-23: Consider a more polished Suspense fallback.A bare
<div>Loading...</div>may flash as unstyled text. If the codebase has a shared loading spinner or skeleton component, using it here would provide a smoother experience consistent with other lazy-loaded routes.apps/posts/src/views/members/components/bulk-action-modals/remove-label-modal.tsx (1)
27-89: Clean modal implementation.State reset on close, disabled button guards, and loading feedback are all handled well.
One minor note: if
labelsis empty, the select dropdown will be empty with no guidance to the user. Consider showing a message or disabling the entire dialog when there are no labels available.apps/posts/src/views/members/components/members-filters.tsx (1)
35-48: Duplicated paid+active tier filtering.The expression
tiers.filter(t => t.type === 'paid' && t.active)appears three times (lines 35, 40, 48). Consider extracting to a local variable for clarity.Suggested refactor
const labels = labelsData?.labels || []; const tiers = tiersData?.tiers || []; const newsletters = newslettersData?.newsletters || []; - const hasMultipleTiers = tiers.filter(t => t.type === 'paid' && t.active).length > 1; + const paidActiveTiers = tiers.filter(t => t.type === 'paid' && t.active); + const hasMultipleTiers = paidActiveTiers.length > 1; // Get filter configuration const filterFields = useMembersFilterConfig({ labels, - tiers: tiers.filter(t => t.type === 'paid' && t.active), + tiers: paidActiveTiers, newsletters: newsletters.filter(n => n.status === 'active'), offers: [], // TODO: Fetch offers if needed hasMultipleTiers, paidMembersEnabled, emailAnalyticsEnabled, // For now, use static options - can be enhanced with search later labelsOptions: labels.map(l => ({value: l.id, label: l.name})), - tiersOptions: tiers.filter(t => t.type === 'paid' && t.active).map(t => ({value: t.id, label: t.name})) + tiersOptions: paidActiveTiers.map(t => ({value: t.id, label: t.name})) });apps/posts/src/views/members/components/members-list.tsx (3)
31-35: Hardcoded'en-US'locale in date formatting.Ghost Admin is English-only so this works, but if internationalization is ever a goal, consider using
undefined(user's locale) or a shared locale constant.
37-59: Consider usingIntl.RelativeTimeFormatinstead of hand-rolled relative time.The native
Intl.RelativeTimeFormatAPI handles relative time formatting with proper pluralization and localization. The current implementation works but could be replaced with a ~10-line wrapper around the standard API, which would also handle edge cases (e.g., month/year boundaries) more accurately than thediffDays / 30anddiffDays / 365approximations.
137-144: Hash-based navigation to Ember member detail page is intentional for the migration, but worth documenting.
window.location.hashdirectly manipulates the URL, bypassing React Router. This is fine during the Ember-to-React transition but should be revisited when the member detail page is also ported.apps/posts/src/views/members/members.tsx (1)
36-45:searchParamsduplicates the defaults already defined inuseBrowseMembersInfinite.The hook's
defaultSearchParams(inapps/admin-x-framework/src/api/members.ts, lines 155-158) already specifiesinclude: 'labels,tiers',limit: '50', andorder: 'created_at desc'. Repeating them here means changes to the defaults would need to be mirrored in two places. Consider only passing thefilterparam and relying on the hook's defaults for the rest.Suggested simplification
- // Build search params for the API query - const searchParams: Record<string, string> = { - include: 'labels,tiers', - limit: '50', - order: 'created_at desc' - }; - - // Add NQL filter if present - if (nql) { - searchParams.filter = nql; - } + // Build search params for the API query (defaults for include, limit, order are in the hook) + const searchParams: Record<string, string> = nql ? {filter: nql} : {};
| searchParams: ({filter, all}) => { | ||
| const params: Record<string, string> = {}; | ||
| if (all) { | ||
| params.all = 'true'; | ||
| } else if (filter) { | ||
| params.filter = filter; | ||
| } | ||
| return params; | ||
| }, | ||
| invalidateQueries: {dataType} | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's find the members API routes/endpoints in Ghost
find ghost/core -name "*.js" -o -name "*.ts" | grep -i member | head -20Repository: TryGhost/Ghost
Length of output: 1483
🏁 Script executed:
# Search for bulk edit/delete endpoints in Ghost API
rg -l "bulk" ghost/core/core/server/api/ --type=js --type=ts | head -10Repository: TryGhost/Ghost
Length of output: 806
🏁 Script executed:
# Look for the members API routes
fd -e js -e ts "members" ghost/core/core/server/api/Repository: TryGhost/Ghost
Length of output: 661
🏁 Script executed:
# Search more broadly for bulk operations in the API
rg "bulkEdit|bulkDelete|bulkDestroy" ghost/core/core/server -A 5 -B 2 --type=js --type=ts | head -50Repository: TryGhost/Ghost
Length of output: 4983
🏁 Script executed:
# Read the members.js endpoint file to see how bulkEdit and bulkDestroy are implemented
cat -n ghost/core/core/server/api/endpoints/members.js | head -100Repository: TryGhost/Ghost
Length of output: 3520
🏁 Script executed:
# Check size and read the entire members.js endpoint file
wc -l ghost/core/core/server/api/endpoints/members.jsRepository: TryGhost/Ghost
Length of output: 109
🏁 Script executed:
# Search for bulkEdit and bulkDestroy in members.js
rg -n "bulkEdit|bulkDestroy" ghost/core/core/server/api/endpoints/members.js -A 30Repository: TryGhost/Ghost
Length of output: 2621
🏁 Script executed:
# Also check the members input validator to see if there's parameter validation
cat -n ghost/core/core/server/api/endpoints/utils/validators/input/members.js | head -150Repository: TryGhost/Ghost
Length of output: 234
🏁 Script executed:
# Find the members service implementation
fd -e js "members" ghost/core/core/server/services/ | grep -v test | grep -v node_modulesRepository: TryGhost/Ghost
Length of output: 531
🏁 Script executed:
# Look for the bulkDestroy and bulkEdit implementations in the members service
rg -n "bulkDestroy|bulkEdit" ghost/core/core/server/services/ -B 3 -A 20 --type=js | head -100Repository: TryGhost/Ghost
Length of output: 10125
🏁 Script executed:
# Find bulkDestroy and bulkEdit in members-api.js
rg -n "bulkDestroy|bulkEdit" ghost/core/core/server/services/members/members-api/members-api.js -B 3 -A 20Repository: TryGhost/Ghost
Length of output: 40
🏁 Script executed:
# Also check the base model for bulk operations to understand the behavior
cat -n ghost/core/core/server/models/base/plugins/bulk-operations.jsRepository: TryGhost/Ghost
Length of output: 10039
🏁 Script executed:
# Find the members API service
find ghost/core/core/server/services/members -name "*.js" | xargs grep -l "bulkDestroy\|bulkEdit" 2>/dev/nullRepository: TryGhost/Ghost
Length of output: 143
🏁 Script executed:
# Search in members-api.js for the members service
cat -n ghost/core/core/server/services/members/members-api/members-api.js | head -200Repository: TryGhost/Ghost
Length of output: 6862
🏁 Script executed:
# Find bulkDestroy and bulkEdit in member-repository.js
rg -n "bulkDestroy|bulkEdit" ghost/core/core/server/services/members/members-api/repositories/member-repository.js -B 5 -A 25Repository: TryGhost/Ghost
Length of output: 4309
🏁 Script executed:
# Get the complete context around lines 214-224 and 232-242 in members.ts
cat -n apps/admin-x-framework/src/api/members.ts | sed -n '200,250p'Repository: TryGhost/Ghost
Length of output: 1434
🏁 Script executed:
# Also check if there are usages of these hooks that might pass empty filter
rg -n "useBulkEditMembers|useBulkDeleteMembers" apps/ -B 3 -A 5Repository: TryGhost/Ghost
Length of output: 3378
🏁 Script executed:
# Check how bulkEdit and bulkDelete are called in members-actions.tsx
cat -n apps/posts/src/views/members/components/members-actions.tsx | sed -n '50,150p'Repository: TryGhost/Ghost
Length of output: 3871
🏁 Script executed:
# Look for where these mutations are called with parameters
rg -n "bulkEdit\(|bulkDelete\(" apps/posts/src/views/members/components/members-actions.tsx -B 2 -A 5Repository: TryGhost/Ghost
Length of output: 1061
🏁 Script executed:
# Check if there are other usages of these mutations besides members-actions.tsx
rg -n "useBulkEditMembers\|useBulkDeleteMembers" apps/ --type=ts --type=tsxRepository: TryGhost/Ghost
Length of output: 85
🏁 Script executed:
# Also check the createMutation function to understand how searchParams is used
fd -e ts -e tsx "createMutation" apps/admin-x-framework/ | head -5Repository: TryGhost/Ghost
Length of output: 40
🏁 Script executed:
# Check usages of the bulk mutations without type specification
rg -n "useBulkEditMembers|useBulkDeleteMembers" apps/ -B 2 -A 2Repository: TryGhost/Ghost
Length of output: 1997
🏁 Script executed:
# Find createMutation in admin-x-framework
fd "createMutation" apps/admin-x-framework/ --type fRepository: TryGhost/Ghost
Length of output: 40
🏁 Script executed:
# Search for createMutation definition in the codebase
rg -n "export.*createMutation|function createMutation" apps/admin-x-framework/ -B 2 -A 10Repository: TryGhost/Ghost
Length of output: 50370
🏁 Script executed:
# Check the complete context in members-actions.tsx to understand the logic
# The pattern shows: filter: nql || '', all: !nql
# This means: if nql exists, use it as filter and all=false
# if nql is empty/falsy, filter becomes '' and all becomes true
# So the actual problematic case is when nql is empty string ''
rg -n "nql" apps/posts/src/views/members/components/members-actions.tsx | head -20Repository: TryGhost/Ghost
Length of output: 566
🏁 Script executed:
# Check if there are other places where these hooks might be called with problematic params
rg -n "useBulkEditMembers\|useBulkDeleteMembers" apps/ --type js --type ts -A 10 | grep -E "filter|all" | head -40Repository: TryGhost/Ghost
Length of output: 40
🏁 Script executed:
# Look at the createMutation hook to see how searchParams is handled
cat -n apps/admin-x-framework/src/utils/api/hooks.ts | sed -n '184,220p'Repository: TryGhost/Ghost
Length of output: 2041
Add early validation to prevent bulk operations without filter or all flag.
In both useBulkEditMembers and useBulkDeleteMembers, the searchParams function checks if (filter) which is falsy for ''. If a caller passes {filter: '', all: false}, the request will have no filter or all query parameter, which the API rejects. This relies on server-side error handling rather than failing early on the client.
Throw an error if neither a non-empty filter nor the all flag is provided:
Proposed safeguard
searchParams: ({filter, all}) => {
const params: Record<string, string> = {};
if (all) {
params.all = 'true';
} else if (filter) {
params.filter = filter;
+ } else {
+ throw new Error('Bulk operation requires either a filter or all flag');
}
return params;
},Also applies to: 232-242
🤖 Prompt for AI Agents
In `@apps/admin-x-framework/src/api/members.ts` around lines 214 - 224, The
searchParams callback in useBulkEditMembers and useBulkDeleteMembers can return
an empty params object when called with {filter: '', all: false}, causing
server-side errors; update the searchParams implementation in both functions
(the searchParams: ({filter, all}) => { ... } block) to validate inputs and
throw an error if neither a non-empty filter (use filter.trim() to treat
whitespace as empty) nor all === true is provided, so callers fail fast with a
clear message instead of sending an invalid request.
apps/posts/src/views/members/components/bulk-action-modals/delete-modal.tsx
Show resolved
Hide resolved
| const handleExportBackup = useCallback(async () => { | ||
| await exportMembers(nql); | ||
| }, [nql]); |
There was a problem hiding this comment.
handleExportBackup doesn't catch errors, leaving the DeleteModal in a broken state.
If exportMembers throws, the promise rejection is unhandled here. In DeleteModal, setBackupExported(true) won't execute, and the export button won't show a failure state — it'll just silently stop spinning.
Suggested fix
const handleExportBackup = useCallback(async () => {
- await exportMembers(nql);
+ try {
+ await exportMembers(nql);
+ } catch (error) {
+ // eslint-disable-next-line no-console
+ console.error('Export backup failed:', error);
+ throw error; // Re-throw so DeleteModal knows it failed
+ }
}, [nql]);Then in DeleteModal, catch the error to show appropriate UI feedback rather than setting backupExported = true.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const handleExportBackup = useCallback(async () => { | |
| await exportMembers(nql); | |
| }, [nql]); | |
| const handleExportBackup = useCallback(async () => { | |
| try { | |
| await exportMembers(nql); | |
| } catch (error) { | |
| // eslint-disable-next-line no-console | |
| console.error('Export backup failed:', error); | |
| throw error; // Re-throw so DeleteModal knows it failed | |
| } | |
| }, [nql]); |
🤖 Prompt for AI Agents
In `@apps/posts/src/views/members/components/members-actions.tsx` around lines 133
- 135, handleExportBackup currently awaits exportMembers(nql) without handling
failures, so an exception leaves DeleteModal stuck spinning; wrap the call
inside handleExportBackup in a try/catch and either return a success boolean
(true on success, false on failure) or rethrow a wrapped error so the caller can
distinguish failure. Then update DeleteModal to call handleExportBackup and
catch errors (instead of unconditionally calling setBackupExported(true)) — on
catch set the backup failure UI state (e.g., setBackupExported(false) and set an
error flag/message) so the export button shows an error rather than spinning
forever. Ensure you reference handleExportBackup, exportMembers, DeleteModal and
setBackupExported when making the change.
| {!hasFilters && ( | ||
| <MembersFilters | ||
| filters={filters} | ||
| onFiltersChange={setFilters} | ||
| /> | ||
| )} | ||
| <MembersActions | ||
| canBulkDelete={canBulkDelete} | ||
| isFiltered={isFiltered} | ||
| memberCount={totalMembers} | ||
| nql={nql} | ||
| /> | ||
| </Header.ActionGroup> | ||
| </Header.Actions> | ||
|
|
||
| {/* When filters are active, show them in a row below */} | ||
| {hasFilters && ( | ||
| <div className={filtersClassName}> | ||
| <MembersFilters | ||
| filters={filters} | ||
| onFiltersChange={setFilters} | ||
| /> | ||
| </div> | ||
| )} |
There was a problem hiding this comment.
MembersFilters remounts when the first filter is added, potentially losing internal component state.
When hasFilters transitions from false to true, MembersFilters unmounts from the inline position (lines 83-88) and mounts in a new position (lines 99-106). React treats these as different component instances, so any internal state in Filters (e.g., open popover, partially entered filter) would be lost during the transition.
Consider using a single MembersFilters instance that is repositioned via CSS, or use a key prop to explicitly control instance identity.
Sketch: single instance with CSS-driven positioning
<Header.Actions>
<Header.ActionGroup>
- {/* When no filters, show filter button inline with other actions */}
- {!hasFilters && (
- <MembersFilters
- filters={filters}
- onFiltersChange={setFilters}
- />
- )}
<MembersActions
canBulkDelete={canBulkDelete}
isFiltered={isFiltered}
memberCount={totalMembers}
nql={nql}
/>
</Header.ActionGroup>
</Header.Actions>
- {/* When filters are active, show them in a row below */}
- {hasFilters && (
- <div className={filtersClassName}>
- <MembersFilters
- filters={filters}
- onFiltersChange={setFilters}
- />
- </div>
- )}
+ <div className={filtersClassName}>
+ <MembersFilters
+ filters={filters}
+ onFiltersChange={setFilters}
+ />
+ </div>🤖 Prompt for AI Agents
In `@apps/posts/src/views/members/members.tsx` around lines 83 - 106,
MembersFilters is being unmounted/remounted when hasFilters toggles because it's
rendered in two different conditional branches; keep a single MembersFilters
instance to preserve its internal state by moving the <MembersFilters
filters={filters} onFiltersChange={setFilters} /> out of the conditionals and
control its visual placement with CSS (use filtersClassName and hasFilters to
apply a different class/positioning), or alternatively always render it inside
one container and toggle visibility via a className derived from hasFilters;
ensure you remove the duplicate MembersFilters render so only the unique
component instance (MembersFilters) remains and continue passing filters and
setFilters props as before.
rob-ghost
left a comment
rob-ghost
left a comment
There was a problem hiding this comment.
Some comments that could probably be addressed now with the help of an agent, otherwise we can merge and iterate.
| const response = await fetch(`/ghost/api/admin/members/upload/?${params}`, { | ||
| method: 'GET', | ||
| credentials: 'include' | ||
| }); |
There was a problem hiding this comment.
Issue: We shouldn't be using a raw fetch and favour useFetchApi for consistency. If there's a reason to use raw fetch lets call it out with a comment.
| } catch (error) { | ||
| // eslint-disable-next-line no-console | ||
| console.error('Export failed:', error); |
There was a problem hiding this comment.
Issue: This swallows the error without any feedback to the user. The ember version shows a toast on failure.
If we intend to handle failure differently in React, or if we're waiting on Toast porting, we should add a TODO here explaining why we're handling the error.
| const searchParams: Record<string, string> = { | ||
| include: 'labels,tiers', | ||
| limit: '50', | ||
| order: 'created_at desc' | ||
| }; |
There was a problem hiding this comment.
Suggestion: This object is created on every render, not a big deal but I don't know if passing it to useBrowseMembersInfinite causes it to re-fetch un-necessarily.
We could use useMemo:
const searchParams = useMemo(() => {
const searchParams: Record<string, string> = {
include: 'labels,tiers',
limit: '50',
order: 'created_at desc'
};
if (nql) {
searchParams.filter = nql;
}
return searchParams;
}, [nql])| labels, | ||
| tiers: tiers.filter(t => t.type === 'paid' && t.active), | ||
| newsletters: newsletters.filter(n => n.status === 'active'), | ||
| offers: [], // TODO: Fetch offers if needed |
There was a problem hiding this comment.
Question: Is the lack of offers being tracked anywhere? Linear issue?
| function formatDate(dateString: string): string { | ||
| const date = new Date(dateString); | ||
| const options: Intl.DateTimeFormatOptions = {day: 'numeric', month: 'short', year: 'numeric'}; | ||
| return date.toLocaleDateString('en-US', options); | ||
| } |
There was a problem hiding this comment.
Suggestion: Shade provides formatDisplayDate() and formatTimestamp() we should use for consistency.
| /** | ||
| * Escape a string for NQL (escape single quotes) | ||
| */ | ||
| function escapeNqlString(value: string): string { | ||
| return '\'' + value.replace(/'/g, '\\\'') + '\''; | ||
| } | ||
|
|
||
| /** | ||
| * Map UI operator names to NQL operators | ||
| */ | ||
| function getFilterRelationOperator(relation: string): string { | ||
| const relationMap: Record<string, string> = { | ||
| 'is-less': '<', | ||
| 'is-or-less': '<=', | ||
| is: '', | ||
| 'is-not': '-', | ||
| 'is-greater': '>', | ||
| 'is-or-greater': '>=', | ||
| contains: '~', | ||
| 'does-not-contain': '-~', | ||
| 'starts-with': '~^', | ||
| 'ends-with': '~$', | ||
| // Shade filter operators (mapped to our internal names) | ||
| before: '<', | ||
| after: '>', | ||
| is_not: '-', | ||
| is_any_of: '', | ||
| is_not_any_of: '-', | ||
| greater_than: '>', | ||
| less_than: '<', | ||
| equals: '', | ||
| not_equals: '-' | ||
| }; | ||
|
|
||
| return relationMap[relation] ?? ''; | ||
| } |
There was a problem hiding this comment.
Suggestion(not-now): These are duplicated from Ember, along with some other utilities (escapeNqlString, getFilterRelationOperator, buildMemberNqlFilter, the filter option constants)
We probably want to centralise these into admin-x-framework or something at some point.
rob-ghost
force-pushed
the
port-members-list-from-ember-to-react
branch
from
c051041 to
75e7d1b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/posts/src/views/members/members.tsx`:
- Around line 36-51: The current useMemo for searchParams sets searchParams to
just {filter: nql} which replaces options.defaultSearchParams in
createInfiniteQuery's queryFn (so include, limit, order are lost); change
searchParams generation to merge the defaults with the filter when nql is
present (e.g. produce an object that spreads options.defaultSearchParams then
overrides/sets filter when nql exists) so useBrowseMembersInfinite/queryFn
receives a combined searchParams containing include, limit, order plus the new
filter; update the useMemo that creates searchParams and ensure types align with
createInfiniteQuery/queryFn expectations.
🧹 Nitpick comments (5)
apps/posts/src/views/members/hooks/use-members-filter-state.ts (1)
350-358: Potential stale closure in functionalsetFiltersupdates.When
actionis a function, it receives the memoizedfiltersfrom the last render rather than the latest URL state. IfsetFiltersis called twice in the same event loop before a re-render, the second call's functional updater will operate on stalefilters.This is unlikely to be a problem in practice for a filter UI, but worth noting if batch updates are ever needed.
apps/posts/src/views/members/hooks/use-members-filter-config.tsx (1)
54-65: Operator naming mismatch between config and shade filter component.The operators here use hyphenated names (
'is-not','is-less','starts-with', etc.) while the shadeFiltercomponent may emit underscored names ('is_not','is_any_of', etc.). The NQL builder inuse-members-filter-state.tsmaps both conventions, so this works, but it could become a maintenance burden.apps/admin-x-framework/src/api/members.ts (1)
148-181: TheisEndfield onMembersInfiniteResponseTypeappears unused.The
returnDatacomputesisEndbut consumers (e.g.,members.tsx) usehasNextPagefrom react-query's infinite query instead. Consider removing it to avoid dead code, or document its intended use case.apps/posts/src/views/members/components/members-actions.tsx (1)
22-47: Hardcoded API path for member export.The path
/ghost/api/admin/members/upload/is hardcoded. While this is common in Ghost's codebase, consider extracting it to a constant or deriving it from a shared API config to reduce maintenance burden if the path ever changes.apps/admin/src/routes/members-route.tsx (1)
20-24: Minor: bare "Loading..." fallback may be inconsistent with other routes.Other lazy-loaded routes in this app use
lazyComponentwhich likely provides a more polished loading state. Consider using a shared loading/spinner component for visual consistency.
| const searchParams = useMemo((): Record<string, string> | undefined => { | ||
| return nql ? {filter: nql} : undefined; | ||
| }, [nql]); | ||
|
|
||
| const { | ||
| data, | ||
| isError, | ||
| isFetching, | ||
| isFetchingNextPage, | ||
| isRefetching, | ||
| fetchNextPage, | ||
| hasNextPage | ||
| } = useBrowseMembersInfinite({ | ||
| searchParams, | ||
| keepPreviousData: true | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n "searchParams.*defaultSearchParams|createInfiniteQuery" apps/admin-x-framework/src/utils/api/hooks.ts -B 3 -A 8Repository: TryGhost/Ghost
Length of output: 5295
🏁 Script executed:
fd -t f "hooks.ts" apps/admin-x-framework/src/utils/api/Repository: TryGhost/Ghost
Length of output: 103
🏁 Script executed:
rg -n "useBrowseMembersInfinite|createInfiniteQuery.*members" apps/admin-x-framework/src --type ts -B 2 -A 10Repository: TryGhost/Ghost
Length of output: 1040
🏁 Script executed:
rg -n "browseMembersInfinite|defaultSearchParams.*label|include.*label" apps/admin-x-framework/src --type ts -A 3Repository: TryGhost/Ghost
Length of output: 335
Filtering loses include, limit, and order parameters — must merge with defaults instead of replacing them.
When nql is set, searchParams becomes {filter: nql}. In createInfiniteQuery (line 124), the queryFn uses pageParam || searchParams || options.defaultSearchParams — so when searchParams is provided, it completely overrides the defaults. This means filtered API requests lose include: 'labels,tiers', limit: '50', and order: 'created_at desc', breaking both data enrichment (missing labels/tiers) and sort order.
Merge with defaults:
Proposed fix
const searchParams = useMemo((): Record<string, string> | undefined => {
- return nql ? {filter: nql} : undefined;
+ if (!nql) {
+ return undefined;
+ }
+ return {
+ include: 'labels,tiers',
+ limit: '50',
+ order: 'created_at desc',
+ filter: nql
+ };
}, [nql]);🤖 Prompt for AI Agents
In `@apps/posts/src/views/members/members.tsx` around lines 36 - 51, The current
useMemo for searchParams sets searchParams to just {filter: nql} which replaces
options.defaultSearchParams in createInfiniteQuery's queryFn (so include, limit,
order are lost); change searchParams generation to merge the defaults with the
filter when nql is present (e.g. produce an object that spreads
options.defaultSearchParams then overrides/sets filter when nql exists) so
useBrowseMembersInfinite/queryFn receives a combined searchParams containing
include, limit, order plus the new filter; update the useMemo that creates
searchParams and ensure types align with createInfiniteQuery/queryFn
expectations.
This adds a React implementation of the members list view behind a labs flag. Includes infinite query API hooks, bulk operations support, and filter UI. The Ember implementation is preserved and active by default until the flag is enabled.
weylandswart
force-pushed
the
port-members-list-from-ember-to-react
branch
from
75e7d1b to
aea9c2d
Compare
2 participants
No ref
Draft PR to port our Members list from Ember to React. Accounts for filtering updates made on the Comments page and new components used for the newer React pages.