SPIKE - DO NOT MERGE: Added updates and announcements preference toggle#28643
Draft
troyciesco wants to merge 1 commit into
Draft
SPIKE - DO NOT MERGE: Added updates and announcements preference toggle#28643troyciesco wants to merge 1 commit into
troyciesco wants to merge 1 commit into
Conversation
Contributor
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
github-actions
Bot
added
the
migration
|
| Command | Status | Duration | Result |
|---|---|---|---|
nx run ghost:test:ci:e2e |
❌ Failed | 30s | View ↗ |
nx run ghost:test:ci:legacy |
✅ Succeeded | 2m 14s | View ↗ |
nx build @tryghost/sodo-search |
✅ Succeeded | <1s | View ↗ |
nx build @tryghost/signup-form |
✅ Succeeded | <1s | View ↗ |
nx build @tryghost/announcement-bar |
✅ Succeeded | <1s | View ↗ |
nx build @tryghost/comments-ui |
✅ Succeeded | <1s | View ↗ |
nx build @tryghost/admin-toolbar |
✅ Succeeded | <1s | View ↗ |
nx build @tryghost/activitypub |
✅ Succeeded | 1s | View ↗ |
Additional runs (7) |
✅ Succeeded | ... | View ↗ |
💡 Dealing with memory or CPU issues? See memory and CPU details with the resource usage add-on ↗.
☁️ Nx Cloud last updated this comment at 2026-06-17 20:39:13 UTC
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/portal/src/actions.js (1)
562-568:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHandle empty newsletter arrays as valid updates.
Line 562 and Line 566 treat
newslettersas truthy/falsy, sonewsletters: []is ignored. That makes a valid “unsubscribe from all newsletters” update a no-op.Proposed fix
- const {newsletters, enableCommentNotifications, enableUpdatesAndAnnouncements} = data; - if (!newsletters && enableCommentNotifications === undefined && enableUpdatesAndAnnouncements === undefined) { + const {newsletters, enableCommentNotifications, enableUpdatesAndAnnouncements} = data; + const hasNewsletters = newsletters !== undefined; + if (!hasNewsletters && enableCommentNotifications === undefined && enableUpdatesAndAnnouncements === undefined) { return {}; } const updateData = {}; - if (newsletters) { + if (hasNewsletters) { updateData.newsletters = newsletters; }Also applies to: 572-574
🤖 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/portal/src/actions.js` around lines 562 - 568, The condition at line 562 and the check at line 566 treat empty arrays as falsy, which prevents empty newsletter arrays from being processed as valid unsubscribe operations. Replace the falsy/truthy checks for newsletters with explicit checks against undefined (using !== undefined) in both the early return condition and the updateData assignment block. Apply the same fix to the similar newsletter-related checks mentioned at lines 572-574 to ensure all empty array scenarios are handled correctly.
🧹 Nitpick comments (1)
ghost/core/core/server/services/mail/ghost-mailer.js (1)
78-81: Clarify header merge intent for maintainability.The current implementation allows
message.headersto override theSenderheader since the spread occurs afterSender: addresses.from. IfSendershould always be preserved, consider reversing the order:headers: { ...message.headers, Sender: addresses.from }Current callers (e.g., member welcome emails) only set
List-Unsubscribeheaders, so no override occurs in practice. However, making the intent explicit prevents accidentalSenderoverrides in future code.🤖 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 `@ghost/core/core/server/services/mail/ghost-mailer.js` around lines 78 - 81, The header merge in the ghost-mailer.js file currently sets Sender first and then spreads message.headers, which allows message.headers to accidentally override the Sender value. Reverse the order of the spread and the Sender property in the headers object so that Sender: addresses.from comes after the spread operator, ensuring the Sender header is always preserved and preventing unintended overrides by future callers.
🤖 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.
Outside diff comments:
In `@apps/portal/src/actions.js`:
- Around line 562-568: The condition at line 562 and the check at line 566 treat
empty arrays as falsy, which prevents empty newsletter arrays from being
processed as valid unsubscribe operations. Replace the falsy/truthy checks for
newsletters with explicit checks against undefined (using !== undefined) in both
the early return condition and the updateData assignment block. Apply the same
fix to the similar newsletter-related checks mentioned at lines 572-574 to
ensure all empty array scenarios are handled correctly.
---
Nitpick comments:
In `@ghost/core/core/server/services/mail/ghost-mailer.js`:
- Around line 78-81: The header merge in the ghost-mailer.js file currently sets
Sender first and then spreads message.headers, which allows message.headers to
accidentally override the Sender value. Reverse the order of the spread and the
Sender property in the headers object so that Sender: addresses.from comes after
the spread operator, ensuring the Sender header is always preserved and
preventing unintended overrides by future callers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5f2c9ba4-4ceb-4e6d-bdf7-5de42632eae1
⛔ Files ignored due to path filters (9)
ghost/core/test/e2e-api/admin/__snapshots__/comments.test.js.snapis excluded by!**/*.snapghost/core/test/e2e-api/admin/__snapshots__/member-commenting.test.js.snapis excluded by!**/*.snapghost/core/test/e2e-api/admin/__snapshots__/members-edit-subscriptions.test.js.snapis excluded by!**/*.snapghost/core/test/e2e-api/admin/__snapshots__/members-newsletters.test.js.snapis excluded by!**/*.snapghost/core/test/e2e-api/admin/__snapshots__/members.test.js.snapis excluded by!**/*.snapghost/core/test/e2e-api/members/__snapshots__/middleware.test.js.snapis excluded by!**/*.snapghost/core/test/e2e-api/members/__snapshots__/webhooks.test.js.snapis excluded by!**/*.snapghost/core/test/e2e-server/__snapshots__/click-tracking.test.js.snapis excluded by!**/*.snapghost/core/test/e2e-webhooks/__snapshots__/members.test.js.snapis excluded by!**/*.snap
📒 Files selected for processing (28)
apps/portal/src/actions.jsapps/portal/src/app.jsapps/portal/src/components/common/newsletter-management.jsapps/portal/src/components/pages/account-email-page.jsapps/portal/src/components/pages/unsubscribe-page.jsapps/portal/src/utils/api.jsghost/core/core/frontend/services/routing/controllers/unsubscribe.jsghost/core/core/server/api/endpoints/utils/serializers/output/members.jsghost/core/core/server/data/migrations/versions/6.46/2026-06-16-19-42-38-add-enable-updates-and-announcements-to-members.jsghost/core/core/server/data/schema/schema.jsghost/core/core/server/models/member.jsghost/core/core/server/services/automations/poll.tsghost/core/core/server/services/mail/ghost-mailer.jsghost/core/core/server/services/member-welcome-emails/email-templates/wrapper.hbsghost/core/core/server/services/member-welcome-emails/member-welcome-email-renderer.jsghost/core/core/server/services/member-welcome-emails/service.jsghost/core/core/server/services/members/members-api/repositories/member-repository.jsghost/core/core/server/services/members/middleware.jsghost/core/core/server/services/members/utils.jsghost/core/core/server/services/settings-helpers/settings-helpers.jsghost/core/test/e2e-api/members/middleware.test.jsghost/core/test/e2e-frontend/members.test.jsghost/core/test/integration/services/member-welcome-emails.test.jsghost/core/test/unit/server/data/schema/integrity.test.jsghost/core/test/unit/server/services/automations/poll.test.tsghost/core/test/unit/server/services/member-welcome-emails/member-welcome-email-renderer.test.jsghost/core/test/unit/server/services/members/utils.test.jsghost/core/test/unit/server/services/settings-helpers/settings-helpers.test.js
EvanHahn
reviewed
Jun 16, 2026
| <p>{t('Occasional updates from {siteTitle}', {siteTitle: site?.title})}</p> | ||
| </div> | ||
| <div style={{display: 'flex', alignItems: 'center'}}> | ||
| <Switch id="updates-and-announcements" onToggle={handleToggle} checked={isChecked} dataTestId="switch-input" /> |
Contributor
There was a problem hiding this comment.
question: Should we disable this while the value is changing?
troyciesco
force-pushed
the
NY-1314_email-updates-and-announcements
branch
2 times, most recently
from
1cab587 to
23104b5
Compare
troyciesco
changed the base branch from
main
to
NY-1314_enable-updates-announcements-migration
troyciesco
force-pushed
the
NY-1314_enable-updates-announcements-migration
branch
from
f13e585 to
6b3e937
Compare
troyciesco
force-pushed
the
NY-1314_email-updates-and-announcements
branch
from
23104b5 to
42004f6
Compare
Base automatically changed from
NY-1314_enable-updates-announcements-migration
to
main
June 17, 2026 20:19
troyciesco
force-pushed
the
NY-1314_email-updates-and-announcements
branch
from
42004f6 to
854343e
Compare
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
2 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.
closes NY-1314
TK
NOTE: can't merge as is, it has some migrations and is missing i18n