Add archived tiers to tier filter#28189
Open
jonatansberg wants to merge 1 commit into
Open
Conversation
Contributor
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 docstrings
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsStopped waiting for pipeline failures after 30000ms. One of your pipelines takes longer than our 30000ms fetch window to run, so review may not consider pipeline-failure results for inline comments if any failures occurred after the fetch window. Increase the timeout if you want to wait longer or run a 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}
Contributor
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
2 times, most recently
from
b8707cc to
01d87da
Compare
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
01d87da to
89509c1
Compare
Contributor
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
Comment on lines
+16
to
+22
| function toTierFilterOption(tier: Tier): FilterOption<string> { | ||
| return { | ||
| value: tier.id, | ||
| label: tier.active ? tier.name : `${tier.name}${ARCHIVED_TIER_LABEL_SUFFIX}`, | ||
| detail: tier.slug | ||
| }; | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check for translation usage in apps/posts
rg -nP "\bt\(\s*['\"]" apps/posts/src | head -50
fd -t f -e ts -e tsx . apps/posts/src | xargs rg -nl "useTranslation|`@doist/react-interpolate`|import.*\bt\b.*i18n" 2>/dev/null | head -20Repository: TryGhost/Ghost
Length of output: 40
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Locate the suffix constant definition/usages
rg -n "ARCHIVED_TIER_LABEL_SUFFIX" -S apps/posts/src
# 2) Inspect the actual implementation around the reported lines
sed -n '1,120p' apps/posts/src/hooks/filter-sources/use-tier-value-source.ts
# 3) Broader check: find i18n usage patterns in apps/posts/src (not just `t(`)
rg -n "useTranslation|`@doist/react-interpolate`|react-interpolate|i18n|next-i18next|formatMessage|intl|translate" apps/posts/src || true
# 4) Check for any `t('...')` usage and capture a few examples
rg -nP "\bt\(\s*['\"]" apps/posts/src | head -50 || true
# 5) If no `t(` usage, check for direct user-facing English strings near tiers/filters
rg -n "(archived|Archived|\(archived\)|filter.*tier|tier.*(archived|archived))" apps/posts/src || trueRepository: TryGhost/Ghost
Length of output: 8507
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Translation helper presence checks in apps/posts
rg -n --hidden -S "useTranslation|react-i18next|i18next|formatMessage|useIntl|next-intl|`@doist/react-interpolate`|react-interpolate|declare.*t\(|\bt\(\s*[`'\"]|\bIntl\." apps/posts/src || true
# Specifically check for any t() usage patterns (including template literals)
rg -nP "\bt\(\s*[`'\"].{0,200}\)" apps/posts/src || true
# Check for translation dictionaries / message IDs (common in this codebase style)
rg -n "messages|i18n\.[a-zA-Z_]+|translations|localized|locale" apps/posts/src || trueRepository: TryGhost/Ghost
Length of output: 4268
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== t('...') usage in apps/posts/src =="
rg -n "\bt\(\s*['\"]" apps/posts/src || true
echo "== translation helper usage (various) in apps/posts/src =="
rg -n "useTranslation\(|react-i18next|i18next|formatMessage|useIntl\(|next-intl|`@doist/react-interpolate`|react-interpolate" apps/posts/src || true
echo "== literal archived suffix usage in apps/posts/src =="
rg -n "(\(archived\)|archived\)|' \(archived\)'|\" \(archived\)\")" apps/posts/src || true
echo "== constant usage =="
rg -n "ARCHIVED_TIER_LABEL_SUFFIX" apps/posts/src || trueRepository: TryGhost/Ghost
Length of output: 607
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "posts\.tiers\.local" apps/posts/src || true
rg -n "posts\.tiers" apps/posts/src || true
rg -n "FilterOption<" apps/posts/src || true
rg -n "\.detail\b|\.label\b" apps/posts/src/views/filters apps/posts/src/views/members apps/posts/src/components apps/posts/src/hooks/filter-sources || trueRepository: TryGhost/Ghost
Length of output: 4121
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Look for other i18n patterns used in posts UI (not only t('...'))
rg -n "i18n=\{|\bi18n=\{|\bi18n:\s*\{|<.*\bi18n=" apps/posts/src || true
rg -n "from '.*i18n|from \".*i18n|from '.*translate|from \".*translate" apps/posts/src || true
rg -n "translate\(|formatMessage\(|intl\." apps/posts/src || trueRepository: TryGhost/Ghost
Length of output: 784
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '130,190p' apps/posts/src/components/label-picker/label-filter-renderer.tsx
sed -n '220,270p' apps/posts/src/views/members/use-member-filter-fields.tsRepository: TryGhost/Ghost
Length of output: 3173
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find where tierValueSource is consumed in the member filter fields hook
rg -n "tierValueSource" apps/posts/src/views/members/use-member-filter-fields.ts
# Inspect the surrounding logic (small targeted ranges)
sed -n '1,120p' apps/posts/src/views/members/use-member-filter-fields.ts
sed -n '120,240p' apps/posts/src/views/members/use-member-filter-fields.ts
sed -n '240,360p' apps/posts/src/views/members/use-member-filter-fields.ts
sed -n '360,520p' apps/posts/src/views/members/use-member-filter-fields.tsRepository: TryGhost/Ghost
Length of output: 16029
Localize the archived tier label suffix instead of hardcoding English
ARCHIVED_TIER_LABEL_SUFFIX is hardcoded to ' (archived)' and concatenated into FilterOption.label:
function toTierFilterOption(tier: Tier): FilterOption<string> {
return {
value: tier.id,
label: tier.active ? tier.name : `${tier.name}${ARCHIVED_TIER_LABEL_SUFFIX}`,
detail: tier.slug
};
}Because this string is constructed outside the app’s i18n layer, the “(archived)” part won’t be translated for localized users. Build the archived label via the app’s translation mechanism instead, using a single translatable string with interpolation (e.g. {name} (archived)).
🤖 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 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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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