Add archived tiers to tier filter#28189
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:
WalkthroughThe PR refactors useTierValueSource into a self-contained hook that calls useBrowseTiers for paid tiers (limit/filter), auto-paginates, builds grouped filter options (active first, archived labeled “(archived)”), and exposes a local ValueSource augmented with hasMultipleTiers. MembersFilters no longer fetches tiers directly and reads hasMultipleTiers from the hook. Unit tests were added to validate option ordering, searching, hasMultipleTiers logic (including pagination), initial-load behavior, and fetchNextPage control. 🚥 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)
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 |
jonatansberg
force-pushed
the
ber-3588-add-archived-tiers-to-tiers-filter
branch
3 times, most recently
from
0cecfde to
8e2a7b7
Compare
jonatansberg
changed the title
jonatansberg
force-pushed
the
ber-3588-add-archived-tiers-to-tiers-filter
branch
3 times, most recently
from
3e7a1b8 to
343c22d
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/posts/test/unit/views/members/member-query-params.test.ts (1)
29-49: ⚡ Quick winConsider adding a test case for multiple tiers to verify join behavior.
The implementation joins multiple tier names with
', ', but this test only covers a single tier. Adding a test with multiple tiers would ensure the join logic works correctly and that all tier names are displayed (not just the first).✨ Example test case for multiple tiers
+ it('joins multiple tier names with comma separator', () => { + const member = { + tiers: [ + { + id: 'tier-1', + name: 'Archived Gold', + slug: 'archived-gold', + active: false, + type: 'paid' + }, + { + id: 'tier-2', + name: 'Premium Silver', + slug: 'premium-silver', + active: true, + type: 'paid' + } + ] + } as Member; + + expect(getActiveColumnValue({ + key: 'tiers', + label: 'Tiers', + include: 'tiers' + }, member, 'UTC')).toEqual({ + text: 'Archived Gold, Premium Silver' + }); + });🤖 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/test/unit/views/members/member-query-params.test.ts` around lines 29 - 49, Add a unit test in member-query-params.test.ts that covers multiple tiers to verify the join behavior: create a Member object with tiers array containing multiple tier objects with distinct name fields, call getActiveColumnValue({key: 'tiers', label: 'Tiers', include: 'tiers'}, member, 'UTC') and assert the returned value.text equals the tier names joined with ", " (e.g., "Gold, Silver"); this ensures getActiveColumnValue correctly concatenates multiple tier names rather than only returning the first.
🤖 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.
Nitpick comments:
In `@apps/posts/test/unit/views/members/member-query-params.test.ts`:
- Around line 29-49: Add a unit test in member-query-params.test.ts that covers
multiple tiers to verify the join behavior: create a Member object with tiers
array containing multiple tier objects with distinct name fields, call
getActiveColumnValue({key: 'tiers', label: 'Tiers', include: 'tiers'}, member,
'UTC') and assert the returned value.text equals the tier names joined with ", "
(e.g., "Gold, Silver"); this ensures getActiveColumnValue correctly concatenates
multiple tier names rather than only returning the first.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2b7b3497-cdd0-4f87-beb5-013af4210600
📒 Files selected for processing (7)
apps/posts/src/hooks/filter-sources/use-tier-value-source.tsapps/posts/src/hooks/filter-sources/utils.tsapps/posts/src/views/members/components/members-filters.tsxapps/posts/test/unit/hooks/use-tier-value-source.test.tsxapps/posts/test/unit/views/members/member-query-params.test.tsapps/shade/src/components/patterns/filters.tsxe2e/tests/admin/members/tier-filter-search.test.ts
jonatansberg
force-pushed
the
ber-3588-add-archived-tiers-to-tiers-filter
branch
3 times, most recently
from
01d87da to
89509c1
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/posts/src/hooks/filter-sources/use-tier-value-source.ts (1)
53-65: ⚖️ Poor tradeoff
createLocalValueSourceis rebuilt on every render inuseTierValueSource.
useTierValueSourcecallscreateLocalValueSource(...)inside the hook body and returns a newtierValueSourceobject each render (newuseOptionsclosure identity).tierValueSourceis then part of theuseMemberFilterFieldsuseMemodependency list, so this can force recomputation of filter fields even when the underlying tier options didn’t change. Consider memoizing the created local value source (or the returnedtierValueSource) based onoptions/isLoadingTierOptions.🤖 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/hooks/filter-sources/use-tier-value-source.ts` around lines 53 - 65, useTierValueSource currently calls createLocalValueSource(...) on every render which yields a new useLocalTierValueSource closure identity and forces recomputation in downstream hooks; change it to memoize the created local value source (or the final returned tierValueSource) using React.useMemo so the identity is stable unless the inputs change — base the memo on options and isLoadingTierOptions (and hasMultipleTiers if it affects consumers) and keep the same shape (id, useItems, toOption) so useMemberFilterFields sees a stable reference unless the underlying tier data truly changes.
🤖 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/hooks/filter-sources/use-tier-value-source.ts`:
- Around line 16-22: toTierFilterOption currently appends the hardcoded
ARCHIVED_TIER_LABEL_SUFFIX to build FilterOption.label; change this to use the
app's i18n translator so the archived suffix is localized (e.g., use the
translation key with interpolation like "{name} (archived)"). Update the
toTierFilterOption implementation to call the translation function (or accept a
t/translate argument) and pass tier.name into the interpolated string instead of
concatenating ARCHIVED_TIER_LABEL_SUFFIX, and remove reliance on the hardcoded
ARCHIVED_TIER_LABEL_SUFFIX constant so labels are produced via i18n.
---
Nitpick comments:
In `@apps/posts/src/hooks/filter-sources/use-tier-value-source.ts`:
- Around line 53-65: useTierValueSource currently calls
createLocalValueSource(...) on every render which yields a new
useLocalTierValueSource closure identity and forces recomputation in downstream
hooks; change it to memoize the created local value source (or the final
returned tierValueSource) using React.useMemo so the identity is stable unless
the inputs change — base the memo on options and isLoadingTierOptions (and
hasMultipleTiers if it affects consumers) and keep the same shape (id, useItems,
toOption) so useMemberFilterFields sees a stable reference unless the underlying
tier data truly changes.
🪄 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: 7189a7e4-0c3b-4c28-bdac-f61b9bfecce9
📒 Files selected for processing (4)
apps/posts/src/hooks/filter-sources/use-tier-value-source.tsapps/posts/src/views/members/components/members-filters.tsxapps/posts/src/views/members/use-member-filter-fields.test.tsapps/posts/test/unit/hooks/use-tier-value-source.test.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/posts/src/views/members/components/members-filters.tsx
ref https://linear.app/ghost/issue/BER-3588/add-archived-tiers-to-tiers-filter Members can keep archived paid tiers, so the filter needs to preserve those options while separating them from active tiers.
jonatansberg
force-pushed
the
ber-3588-add-archived-tiers-to-tiers-filter
branch
from
89509c1 to
38338a1
Compare
rob-ghost
left a comment
rob-ghost
left a comment
There was a problem hiding this comment.
I'm not sure but I think there's a createGhostBrowseValueSource helper which would simplify the value source for tiers and remove the need to manually page through them.
ref https://linear.app/ghost/issue/BER-3588 - the tiers endpoint returns every match in a single response, so the manual fetchNextPage drain was dead code (isEnd is always true) and is replaced by a single browse fetch - reused the shared getActiveTiers/getArchivedTiers helpers instead of re-deriving active/archived tiers in the hook - returned {valueSource, hasMultipleTiers} so the value source no longer carries UI-gating state bolted on via an intersection type - dropped the inert limit param (the tiers service ignores limit and 'all' is deprecated) - added unit coverage for the loading and empty-data states
jonatansberg
force-pushed
the
ber-3588-add-archived-tiers-to-tiers-filter
branch
from
f68a295 to
702bddd
Compare
2 participants
Summary
useTierValueSource, matching the other member filter value sources.limit=all.Why
Members can retain archived paid tiers, but the members filter only exposed active paid tiers. That made archived-tier member segments impossible to select from the tier filter.
Impact
The Membership tier filter now shows active and archived paid tiers in separate sections. Member table values continue to display the actual tier name, including archived tier names.
Validation
pnpm --filter @tryghost/shade exec vitest run test/unit/components/patterns/filters.test.tsxpnpm --filter @tryghost/posts exec vitest run test/unit/hooks/use-tier-value-source.test.tsx test/unit/views/members/member-query-params.test.ts src/views/members/use-member-filter-fields.test.tspnpm --filter @tryghost/posts lint:codepnpm --filter @tryghost/posts lint:testpnpm --filter @tryghost/e2e lintpnpm --filter @tryghost/e2e test:typesgit diff --check origin/main..HEADNotes:
apps/posts/src/hooks/use-post-success-modal.ts.pnpm --filter @tryghost/e2e test tests/admin/members/tier-filter-search.test.ts, but this temp worktree's dev-mode Ghost container failed before tests ran becauseghost/corehas no localnode_modules(nodemon: not found).ref https://linear.app/ghost/issue/BER-3588/add-archived-tiers-to-tiers-filter