Fixed members-forward filter gating parity#26932
Conversation
ref https://linear.app/ghost/issue/BER-3456/responded-with-feedback-filter-is-missing-in-members-forward React was hiding email filters behind config-only gates that Ember does not use, which removed the feedback filter and its operators from members-forward.
WalkthroughThis pull request refactors the member filter configuration system by replacing browse config-based feature flags with a settings-based approach. The changes remove 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/posts/src/views/members/use-member-filter-fields.test.ts (1)
74-88: Good test coverage for the new gating logic.The test correctly validates that the Email group and its fields (including
newsletter_feedback) are present whenemailFiltersEnabledis true.Consider adding a complementary test to verify the Email group is absent when
emailFiltersEnabled: false(or omitted), which would complete the boundary coverage for this gating behavior.📝 Suggested test case for completeness
it('hides the Email group when email sending is disabled', () => { const {result} = renderHook(() => useMemberFilterFields({ emailFiltersEnabled: false, siteTimezone: 'UTC' })); const emailGroup = result.current.find(group => group.group === 'Email'); expect(emailGroup).toBeUndefined(); });🤖 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.test.ts` around lines 74 - 88, Add a complementary test to ensure the Email group is hidden when email sending is disabled: in the same test file use-member-filter-fields.test.ts, add a test that calls renderHook(() => useMemberFilterFields({emailFiltersEnabled: false, siteTimezone: 'UTC'})) and asserts that result.current.find(group => group.group === 'Email') is undefined (or toBeUndefined()). This verifies the gating logic in useMemberFilterFields for the false/omitted emailFiltersEnabled case and completes boundary coverage.
🤖 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/components/members-filters.tsx`:
- Line 60: The emailFiltersEnabled computation incorrectly treats undefined
(when settingsData hasn't loaded) as enabled; update the logic around the
emailFiltersEnabled variable to first ensure settings are present/loaded (e.g.,
check settings or settingsData truthiness) or treat undefined as a safe default
('disabled') before calling getSettingValue, then only mark enabled when
getSettingValue(settings, 'editor_default_email_recipients') === a
non-'disabled' value; locate the emailFiltersEnabled declaration and adjust the
condition to guard against undefined settings using the getSettingValue call.
---
Nitpick comments:
In `@apps/posts/src/views/members/use-member-filter-fields.test.ts`:
- Around line 74-88: Add a complementary test to ensure the Email group is
hidden when email sending is disabled: in the same test file
use-member-filter-fields.test.ts, add a test that calls renderHook(() =>
useMemberFilterFields({emailFiltersEnabled: false, siteTimezone: 'UTC'})) and
asserts that result.current.find(group => group.group === 'Email') is undefined
(or toBeUndefined()). This verifies the gating logic in useMemberFilterFields
for the false/omitted emailFiltersEnabled case and completes boundary coverage.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ca0662ca-ae23-4806-a1eb-ac50590d092e
📒 Files selected for processing (3)
apps/posts/src/views/members/components/members-filters.tsxapps/posts/src/views/members/use-member-filter-fields.test.tsapps/posts/src/views/members/use-member-filter-fields.ts
| const settings = settingsData?.settings || []; | ||
| const paidMembersEnabled = getSettingValue<boolean>(settings, 'paid_members_enabled') === true; | ||
| const emailAnalyticsEnabled = configData?.config?.emailAnalytics === true; | ||
| const emailFiltersEnabled = getSettingValue<string>(settings, 'editor_default_email_recipients') !== 'disabled'; |
There was a problem hiding this comment.
Handle the case when settings haven't loaded yet.
When settingsData is still loading, getSettingValue returns undefined, and undefined !== 'disabled' evaluates to true. This could briefly show email filters before settings load, even if they should be hidden.
🛡️ Proposed fix
- const emailFiltersEnabled = getSettingValue<string>(settings, 'editor_default_email_recipients') !== 'disabled';
+ const editorDefaultEmailRecipients = getSettingValue<string>(settings, 'editor_default_email_recipients');
+ const emailFiltersEnabled = editorDefaultEmailRecipients !== undefined && editorDefaultEmailRecipients !== 'disabled';📝 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 emailFiltersEnabled = getSettingValue<string>(settings, 'editor_default_email_recipients') !== 'disabled'; | |
| const editorDefaultEmailRecipients = getSettingValue<string>(settings, 'editor_default_email_recipients'); | |
| const emailFiltersEnabled = editorDefaultEmailRecipients !== undefined && editorDefaultEmailRecipients !== 'disabled'; |
🤖 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 60, The
emailFiltersEnabled computation incorrectly treats undefined (when settingsData
hasn't loaded) as enabled; update the logic around the emailFiltersEnabled
variable to first ensure settings are present/loaded (e.g., check settings or
settingsData truthiness) or treat undefined as a safe default ('disabled')
before calling getSettingValue, then only mark enabled when
getSettingValue(settings, 'editor_default_email_recipients') === a
non-'disabled' value; locate the emailFiltersEnabled declaration and adjust the
condition to guard against undefined settings using the getSettingValue call.
![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: 088cef65e0
ℹ️ 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".
| const settings = settingsData?.settings || []; | ||
| const paidMembersEnabled = getSettingValue<boolean>(settings, 'paid_members_enabled') === true; | ||
| const emailAnalyticsEnabled = configData?.config?.emailAnalytics === true; | ||
| const emailFiltersEnabled = getSettingValue<string>(settings, 'editor_default_email_recipients') !== 'disabled'; |
There was a problem hiding this comment.
getSettingValue returns null when /settings/ has not resolved (or the key is missing), so null !== 'disabled' makes emailFiltersEnabled true by default. That means the Email filter group is exposed before settings are known, and on a failed settings fetch it can stay visible even when member email is actually disabled, allowing users to create filters that should be gated off. Gate this on loaded settings (or a non-null setting value) before enabling the Email group.
Useful? React with 👍 / 👎.
1 participant
This updates
members-forwardso the Email filter group follows the same availability rule as Ember, based on whether member email sending is enabled. It also removes the extra React-only feedback gate, soResponded with feedbackis available again with theMore like thisandLess like thisoperators whenever the Email filters are shown.I added focused hook tests to cover that gating behavior.
ref https://linear.app/ghost/issue/BER-3456/responded-with-feedback-filter-is-missing-in-members-forward
Tested with
yarn --cwd apps/posts vitest run src/views/members/use-member-filter-fields.test.tsandyarn --cwd apps/posts eslint src/views/members/use-member-filter-fields.ts src/views/members/components/members-filters.tsx src/views/members/use-member-filter-fields.test.ts.