Added multiple-active-subscriptions members warning and filter#28232
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR adds end-to-end support for filtering members by multiple active Stripe customers: registers a private labs flag, validates and extracts the Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
kevinansfield
force-pushed
the
multiple-subs-filter-banner
branch
5 times, most recently
from
c973a4c to
36b2594
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: 36b2594879
ℹ️ 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/posts/src/views/members/multiple-active-stripe-customers.ts (1)
42-58: ⚡ Quick winConsider reducing redundant parsing calls.
The function calls
parseAccessibilityPreferences(accessibility)at line 47, then callsgetMultipleActiveStripeCustomersBannerPreference(accessibility)at line 48, which internally parses the same string again. While this doesn't affect correctness, it's inefficient.♻️ Suggested refactor to parse once
export function buildDismissedMultipleActiveStripeCustomersPreference( accessibility: string | null | undefined, dismissedCount: number, dismissedAt: string ): string { const preferences = parseAccessibilityPreferences(accessibility); - const currentBannerPreference = getMultipleActiveStripeCustomersBannerPreference(accessibility); + const currentBannerPreference = preferences.multipleActiveStripeCustomersBanner ?? {}; return JSON.stringify({ ...preferences, multipleActiveStripeCustomersBanner: { ...currentBannerPreference, dismissedCount, dismissedAt } }); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/posts/src/views/members/multiple-active-stripe-customers.ts` around lines 42 - 58, The function buildDismissedMultipleActiveStripeCustomersPreference is calling parseAccessibilityPreferences(accessibility) and then getMultipleActiveStripeCustomersBannerPreference(accessibility), which causes the accessibility string to be parsed twice; refactor to parse once by calling parseAccessibilityPreferences(accessibility) into a local variable and extract the existing multipleActiveStripeCustomersBanner preference from that parsed object (instead of calling getMultipleActiveStripeCustomersBannerPreference with the raw string), then merge dismissedCount/dismissedAt and JSON.stringify the result; update references to parseAccessibilityPreferences and getMultipleActiveStripeCustomersBannerPreference accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/posts/src/views/members/members.tsx`:
- Around line 312-314: The banner text in members.tsx uses a fixed verb "were
found" which breaks subject-verb agreement when
multipleActiveStripeCustomersCount === 1; update the JSX inside the div
rendering {formatNumber(multipleActiveStripeCustomersCount)} ... so the verb is
conditional (use "was found" when multipleActiveStripeCustomersCount === 1,
otherwise "were found") while keeping the existing pluralization for
"member/members" based on multipleActiveStripeCustomersCount.
---
Nitpick comments:
In `@apps/posts/src/views/members/multiple-active-stripe-customers.ts`:
- Around line 42-58: The function
buildDismissedMultipleActiveStripeCustomersPreference is calling
parseAccessibilityPreferences(accessibility) and then
getMultipleActiveStripeCustomersBannerPreference(accessibility), which causes
the accessibility string to be parsed twice; refactor to parse once by calling
parseAccessibilityPreferences(accessibility) into a local variable and extract
the existing multipleActiveStripeCustomersBanner preference from that parsed
object (instead of calling getMultipleActiveStripeCustomersBannerPreference with
the raw string), then merge dismissedCount/dismissedAt and JSON.stringify the
result; update references to parseAccessibilityPreferences and
getMultipleActiveStripeCustomersBannerPreference accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2fde2b30-2393-4796-a326-805aa2d2dddc
⛔ Files ignored due to path filters (1)
ghost/core/test/e2e-api/admin/__snapshots__/config.test.js.snapis excluded by!**/*.snap
📒 Files selected for processing (11)
apps/admin-x-settings/src/components/settings/advanced/labs/private-features.tsxapps/posts/src/views/members/components/members-actions.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/members.tsxapps/posts/src/views/members/multiple-active-stripe-customers.test.tsapps/posts/src/views/members/multiple-active-stripe-customers.tsghost/core/core/server/api/endpoints/utils/serializers/input/members.jsghost/core/core/server/models/member.jsghost/core/core/shared/labs.jsghost/core/test/e2e-api/admin/members.test.js
kevinansfield
changed the title
kevinansfield
force-pushed
the
multiple-subs-filter-banner
branch
2 times, most recently
from
77458d8 to
e9fc0ed
Compare
kevinansfield
force-pushed
the
multiple-subs-filter-banner
branch
3 times, most recently
from
bb35583 to
68294ab
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@apps/posts/src/views/members/hooks/use-sync-multiple-active-stripe-customers-banner-dismissal.ts`:
- Around line 42-55: The async editUser call in
use-sync-multiple-active-stripe-customers-banner-dismissal can update state
after unmount (through setOptimisticDismissedMultipleActiveStripeCustomersCount
in the .then/.catch), so add a mounted flag inside the surrounding useEffect:
set let isMounted = true at start, return a cleanup that sets isMounted = false,
and only call setOptimisticDismissedMultipleActiveStripeCustomersCount(...)
inside the .then and .catch if isMounted is still true; keep the existing
optimistic set before calling editUser and keep clearing it after the promise
only when mounted. This avoids state updates on an unmounted component while
preserving the optimistic flow around editUser and the
buildUserWithDismissedMultipleActiveStripeCustomersBanner logic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2a7c06b8-2716-4103-a994-83452d8c6074
⛔ Files ignored due to path filters (2)
ghost/core/test/e2e-api/admin/__snapshots__/config.test.js.snapis excluded by!**/*.snapghost/core/test/e2e-api/admin/__snapshots__/members.test.js.snapis excluded by!**/*.snap
📒 Files selected for processing (13)
apps/admin-x-settings/src/components/settings/advanced/labs/private-features.tsxapps/posts/src/views/members/components/members-actions.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-sync-multiple-active-stripe-customers-banner-dismissal.tsapps/posts/src/views/members/members.tsxapps/posts/src/views/members/multiple-active-stripe-customers.test.tsapps/posts/src/views/members/multiple-active-stripe-customers.tsghost/core/core/server/api/endpoints/utils/serializers/input/members.jsghost/core/core/server/models/member.jsghost/core/core/server/services/members/members-api/repositories/member-repository.jsghost/core/core/shared/labs.jsghost/core/test/e2e-api/admin/members.test.js
✅ Files skipped from review due to trivial changes (1)
- apps/admin-x-settings/src/components/settings/advanced/labs/private-features.tsx
🚧 Files skipped from review as they are similar to previous changes (10)
- apps/posts/src/views/members/hooks/use-members-filter-state.test.tsx
- ghost/core/core/shared/labs.js
- apps/posts/src/views/members/components/members-actions.tsx
- apps/posts/src/views/members/multiple-active-stripe-customers.test.ts
- ghost/core/core/server/api/endpoints/utils/serializers/input/members.js
- apps/posts/src/views/members/multiple-active-stripe-customers.ts
- ghost/core/test/e2e-api/admin/members.test.js
- apps/posts/src/views/members/members.tsx
- apps/posts/src/views/members/hooks/use-members-filter-state.ts
- ghost/core/core/server/models/member.js
Comment on lines
+42
to
+55
| setOptimisticDismissedMultipleActiveStripeCustomersCount(multipleActiveStripeCustomersCount); | ||
|
|
||
| editUser(buildUserWithDismissedMultipleActiveStripeCustomersBanner( | ||
| currentUser, | ||
| multipleActiveStripeCustomersCount, | ||
| multipleActiveStripeCustomersBannerPreference.dismissedAt ?? new Date().toISOString() | ||
| )).then(() => { | ||
| setOptimisticDismissedMultipleActiveStripeCustomersCount(null); | ||
| }).catch((error) => { | ||
| setOptimisticDismissedMultipleActiveStripeCustomersCount(null); | ||
| // This keeps the preference in sync opportunistically; failing to sync should not interrupt the member list. | ||
| // eslint-disable-next-line no-console | ||
| console.log('Unable to update multiple active Stripe customers banner dismissed count', error); | ||
| }); |
Contributor
There was a problem hiding this comment.
Missing cleanup for async operation could cause state updates on unmounted component.
The useEffect doesn't return a cleanup function to cancel the in-flight editUser call. If the component unmounts while the promise is pending, the .then() and .catch() handlers will still execute and call setOptimisticDismissedMultipleActiveStripeCustomersCount, triggering React warnings about setting state on an unmounted component.
Consider tracking mounted state or returning a cleanup function:
🛡️ Proposed fix using mounted flag
useEffect(() => {
+ let isMounted = true;
const dismissedCount = multipleActiveStripeCustomersBannerPreference.dismissedCount;
if (
!currentUser
|| optimisticDismissedMultipleActiveStripeCustomersCount !== null
|| isDismissingMultipleActiveStripeCustomersBanner
|| dismissedCount === undefined
|| !hasLoadedMultipleActiveStripeCustomersCount
|| multipleActiveStripeCustomersCount >= dismissedCount
) {
return;
}
setOptimisticDismissedMultipleActiveStripeCustomersCount(multipleActiveStripeCustomersCount);
editUser(buildUserWithDismissedMultipleActiveStripeCustomersBanner(
currentUser,
multipleActiveStripeCustomersCount,
multipleActiveStripeCustomersBannerPreference.dismissedAt ?? new Date().toISOString()
)).then(() => {
- setOptimisticDismissedMultipleActiveStripeCustomersCount(null);
+ if (isMounted) {
+ setOptimisticDismissedMultipleActiveStripeCustomersCount(null);
+ }
}).catch((error) => {
- setOptimisticDismissedMultipleActiveStripeCustomersCount(null);
- // This keeps the preference in sync opportunistically; failing to sync should not interrupt the member list.
- // eslint-disable-next-line no-console
- console.log('Unable to update multiple active Stripe customers banner dismissed count', error);
+ if (isMounted) {
+ setOptimisticDismissedMultipleActiveStripeCustomersCount(null);
+ // This keeps the preference in sync opportunistically; failing to sync should not interrupt the member list.
+ // eslint-disable-next-line no-console
+ console.log('Unable to update multiple active Stripe customers banner dismissed count', error);
+ }
});
+
+ return () => {
+ isMounted = false;
+ };
}, [📝 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.
Suggested change
| setOptimisticDismissedMultipleActiveStripeCustomersCount(multipleActiveStripeCustomersCount); | |
| editUser(buildUserWithDismissedMultipleActiveStripeCustomersBanner( | |
| currentUser, | |
| multipleActiveStripeCustomersCount, | |
| multipleActiveStripeCustomersBannerPreference.dismissedAt ?? new Date().toISOString() | |
| )).then(() => { | |
| setOptimisticDismissedMultipleActiveStripeCustomersCount(null); | |
| }).catch((error) => { | |
| setOptimisticDismissedMultipleActiveStripeCustomersCount(null); | |
| // This keeps the preference in sync opportunistically; failing to sync should not interrupt the member list. | |
| // eslint-disable-next-line no-console | |
| console.log('Unable to update multiple active Stripe customers banner dismissed count', error); | |
| }); | |
| useEffect(() => { | |
| let isMounted = true; | |
| const dismissedCount = multipleActiveStripeCustomersBannerPreference.dismissedCount; | |
| if ( | |
| !currentUser | |
| || optimisticDismissedMultipleActiveStripeCustomersCount !== null | |
| || isDismissingMultipleActiveStripeCustomersBanner | |
| || dismissedCount === undefined | |
| || !hasLoadedMultipleActiveStripeCustomersCount | |
| || multipleActiveStripeCustomersCount >= dismissedCount | |
| ) { | |
| return; | |
| } | |
| setOptimisticDismissedMultipleActiveStripeCustomersCount(multipleActiveStripeCustomersCount); | |
| editUser(buildUserWithDismissedMultipleActiveStripeCustomersBanner( | |
| currentUser, | |
| multipleActiveStripeCustomersCount, | |
| multipleActiveStripeCustomersBannerPreference.dismissedAt ?? new Date().toISOString() | |
| )).then(() => { | |
| if (isMounted) { | |
| setOptimisticDismissedMultipleActiveStripeCustomersCount(null); | |
| } | |
| }).catch((error) => { | |
| if (isMounted) { | |
| setOptimisticDismissedMultipleActiveStripeCustomersCount(null); | |
| // This keeps the preference in sync opportunistically; failing to sync should not interrupt the member list. | |
| // eslint-disable-next-line no-console | |
| console.log('Unable to update multiple active Stripe customers banner dismissed count', error); | |
| } | |
| }); | |
| return () => { | |
| isMounted = false; | |
| }; | |
| }, [ | |
| currentUser, | |
| multipleActiveStripeCustomersBannerPreference, | |
| multipleActiveStripeCustomersCount, | |
| optimisticDismissedMultipleActiveStripeCustomersCount, | |
| isDismissingMultipleActiveStripeCustomersBanner, | |
| hasLoadedMultipleActiveStripeCustomersCount, | |
| ]); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@apps/posts/src/views/members/hooks/use-sync-multiple-active-stripe-customers-banner-dismissal.ts`
around lines 42 - 55, The async editUser call in
use-sync-multiple-active-stripe-customers-banner-dismissal can update state
after unmount (through setOptimisticDismissedMultipleActiveStripeCustomersCount
in the .then/.catch), so add a mounted flag inside the surrounding useEffect:
set let isMounted = true at start, return a cleanup that sets isMounted = false,
and only call setOptimisticDismissedMultipleActiveStripeCustomersCount(...)
inside the .then and .catch if isMounted is still true; keep the existing
optimistic set before calling editUser and keep clearing it after the promise
only when mounted. This avoids state updates on an unmounted component while
preserving the optimistic flow around editUser and the
buildUserWithDismissedMultipleActiveStripeCustomersBanner logic.
3a24220 to
0862751
Compare
no issue This adds a labs-gated warning and exact members filter for identifying members with active subscriptions across multiple Stripe customers.
kevinansfield
force-pushed
the
multiple-subs-filter-banner
branch
from
0862751 to
59eb759
Compare
jonatansberg
requested changes
Jun 10, 2026
Comment on lines
+86
to
+90
| const preservedRawFilter = useMemo(() => { | ||
| return preserveMultipleActiveSubscriptionsFilter && filters.length === 0 && isMultipleActiveSubscriptionsFilter(filterParam) | ||
| ? filterParam | ||
| : undefined; | ||
| }, [filterParam, filters.length, preserveMultipleActiveSubscriptionsFilter]); |
Member
There was a problem hiding this comment.
This looks a bit fishy to me. I think we should have a general way to preserve additional filter parameters without coupling this to the multiple active subscriptions banner. Ideally, we would just have that banner link to the member list view with the appropriate filter query state, and the rest would be driven by the URL. No preserveMultipleActiveSubscriptionsFilter or preservedRawFilter shenanigans.
Member
Author
There was a problem hiding this comment.
we would just have that banner link to the member list view with the appropriate filter query state
Do you mean without having the filter in the URL? One of the main use-cases for the URL approach is to allow it to be bookmarked and linked to from our help docs.
| const preference = useMemo(() => { | ||
| return getMultipleActiveSubscriptionsBannerPreference(currentUser?.accessibility); | ||
| }, [currentUser?.accessibility]); | ||
| const dismissedCount = optimisticDismissedCount ?? preference.dismissedCount ?? 0; |
Member
There was a problem hiding this comment.
Would it make more sense to have something tied to a date rather than the count? Or should it just be if you dismiss the banner, then it's gone forever?
Member
Author
There was a problem hiding this comment.
The dismiss/re-show behaviour was discussed a fair bit. The behaviour we landed on for this initial version is that any time the count increases from the last known value we re-show the banner, although the data model should support time-based dismissals in future if we decide that's needed.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #28232 +/- ##
==========================================
+ Coverage 73.74% 73.76% +0.02%
==========================================
Files 1541 1541
Lines 132180 132279 +99
Branches 15784 15804 +20
==========================================
+ Hits 97476 97576 +100
+ Misses 33737 33714 -23
- Partials 967 989 +22
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
no ref The preserved-raw-filter mode added a second state path through the members filter hook that every consumer had to be aware of. Registering count.active_stripe_customers as a real field keeps everything on the standard parse/serialize pipeline: - the field is absent from the filter UI's field config so it never appears in the add-filter dropdown or as a pill, and the members page ignores it when deciding whether to show the filter bar - the codec only round-trips the exact count.active_stripe_customers:>1 form the API accepts; invalid values are dropped client-side and invalid combinations are left to the API to reject - bulk delete is now restricted via the existing restricted-fields list since the predicate is parsed rather than preserved as raw NQL
no ref Released the multipleSubsFilter labs flag to GA. Admins now see a banner on the members list when members have active subscriptions across multiple Stripe customers, with a one-click filter to view the affected members.
jonatansberg
approved these changes
Jun 10, 2026
jonatansberg
left a comment
jonatansberg
left a comment
Member
There was a problem hiding this comment.
Let's tackle the thing we discussed in the mob as a follow-up.
kevinansfield
added a commit
that referenced
this pull request
Jun 11, 2026
ref #28232 - the multiple-active-subscriptions filter shipped as a hidden filter field whose codec only round-trips the exact `count.active_stripe_customers:>1` form, with the members page filtering the predicate out of the visible filter bar — every new bare filter needs the same field registration, codec, UI exclusion, and bulk-delete listing - generalised the underlying filter state handling instead: the filter param is parsed once, split into its top-level AND clauses via the AST, and any clause the filter UI can't represent (unknown fields, OR groups, operators the field map doesn't advertise) is preserved through the URL, the `nql` output, and chip edits rather than silently dropped - added an AST→NQL serializer as the inverse of nql-lang's parser, since the library has no serializer of its own; it returns undefined for anything it can't faithfully round-trip so clauses are dropped rather than corrupted - bulk delete is disabled whenever unknown clauses are present because we can't vouch for the bulk-destroy endpoint supporting them
This was referenced Jun 11, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
ref https://linear.app/ghost/issue/BER-3717/
Summary