Added sender details customization for welcome emails#27192
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:
WalkthroughAdds end-to-end support for editing and verifying shared sender fields for member welcome automated emails. Backend: new admin API endpoints ( 🚥 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 |
cmraible
force-pushed
the
NY-1164_welcome-email-sender-info
branch
from
8237b8c to
b6c9434
Compare
Widened newsletter-emails function parameter types to accept Pick<Newsletter> and removed redundant require redeclaration in welcome-emails test.
Collaborator
|
@CodeRabbit review |
Contributor
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (6)
ghost/core/test/integration/services/member-welcome-emails.test.js (1)
488-526: Consider resettingmemberWelcomeEmailService.apibeforeinit()for consistency.The test at line 452 resets
memberWelcomeEmailService.api = nullbefore callinginit(), but this test (line 489) callsinit()directly. While this may work due to test isolation, being explicit about the reset would be more consistent with the pattern used elsewhere in this file (e.g., lines 471-472, 539-540, 554-556).🔧 Suggested consistency improvement
it('uses automated sender overrides for test welcome emails', async function () { + memberWelcomeEmailService.api = null; memberWelcomeEmailService.init();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ghost/core/test/integration/services/member-welcome-emails.test.js` around lines 488 - 526, Reset memberWelcomeEmailService.api to null before calling memberWelcomeEmailService.init() in this test to match the pattern used elsewhere; locate the test function that currently calls memberWelcomeEmailService.init() (the spec using memberWelcomeEmailService.api.sendTestEmail) and add a line that sets memberWelcomeEmailService.api = null immediately before invoking memberWelcomeEmailService.init() so the service is initialized consistently.ghost/core/core/server/api/endpoints/utils/validators/input/automated_emails.js (1)
80-88: Consider validating email format for sender email fields.The
editSendersvalidator checks thatsender_emailandsender_reply_toare strings when provided, but doesn't validate they are valid email addresses. ThesendTestEmailvalidator (lines 101-105) usesvalidator.isEmail()for this purpose. Consider adding similar validation here to catch invalid emails early.💡 Suggested enhancement
editSenders(apiConfig, frame) { const senderName = frame.data.sender_name; const senderEmail = frame.data.sender_email; const senderReplyTo = frame.data.sender_reply_to; validateOptionalStringField(senderName, 'Sender name must be a string'); validateOptionalStringField(senderEmail, 'Sender email must be a string'); validateOptionalStringField(senderReplyTo, 'Reply-to email must be a string'); + + if (senderEmail && !validator.isEmail(senderEmail)) { + throw new ValidationError({ + message: 'Sender email must be a valid email address' + }); + } + + if (senderReplyTo && !validator.isEmail(senderReplyTo)) { + throw new ValidationError({ + message: 'Reply-to email must be a valid email address' + }); + } },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ghost/core/core/server/api/endpoints/utils/validators/input/automated_emails.js` around lines 80 - 88, The editSenders validator currently ensures sender_name, sender_email, and sender_reply_to are strings but does not validate email format; update editSenders to, after calling validateOptionalStringField for sender_email and sender_reply_to, also check their format using the same validator.isEmail() calls/pattern used in sendTestEmail so invalid addresses are rejected early (use the same error messages or similar descriptive errors) and ensure you reference editSenders, sender_email, sender_reply_to, validateOptionalStringField and validator.isEmail when locating and implementing the change.apps/admin-x-settings/src/components/settings/membership/member-emails.tsx (1)
163-206: Consider clearing the verification token from URL after processing.The verification effect runs whenever the component mounts with a
verifyEmailtoken in the URL. If the user navigates away and returns to the same URL (e.g., via browser history), the verification will be attempted again, likely failing with an expired/used token error.♻️ Suggested approach
useEffect(() => { if (!verifyEmailToken || !window.location.href.includes('memberemails')) { return; } const verify = async () => { try { const {meta: {email_verified: emailVerified} = {}} = await verifySenderUpdate({token: verifyEmailToken}); + + // Clear token from URL to prevent re-verification attempts + const url = new URL(window.location.href); + url.searchParams.delete('verifyEmail'); + window.history.replaceState({}, '', url.toString()); let title = 'Sender email verified'; // ... rest of success handling } catch (e) { + // Clear token from URL even on error + const url = new URL(window.location.href); + url.searchParams.delete('verifyEmail'); + window.history.replaceState({}, '', url.toString()); + let prompt = 'There was an error verifying your email address. Try again later.'; // ... rest of error handling } }; verify(); }, [handleError, verifyEmailToken, verifySenderUpdate]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/admin-x-settings/src/components/settings/membership/member-emails.tsx` around lines 163 - 206, After successful or failed verification in the useEffect's verify() (which calls verifySenderUpdate with verifyEmailToken), remove the verifyEmailToken from the URL so the effect won't re-run on navigation; update the browser URL using history.replaceState (or URLSearchParams on window.location.href) to strip the token/query parameter (or fragment) after handling the response (both in the try and catch paths), referencing verifyEmailToken, verifySenderUpdate and the verify() function so the token is cleared immediately after processing.apps/admin-x-settings/src/components/settings/membership/member-emails/welcome-email-customize-modal.tsx (1)
466-481: Sender hydration effect may cause stale closures with validation.The hydration effect runs when
automatedEmailsDatabecomes available and sets sender values. However, if the user has already started typing in the inputs before the data loads, their changes will be overwritten.Consider checking if any sender field has been modified before hydrating.
♻️ Suggested approach
useEffect(() => { if (senderInputsHydrated || automatedEmailsData === undefined) { return; } + // Don't overwrite user input if they've already started typing + const currentState = formState.generalSettings; + const hasUserInput = currentState.senderName || currentState.senderEmail || currentState.replyToEmail; + if (hasUserInput) { + setSenderInputsHydrated(true); + return; + } + setFormState(state => ({ ...state, generalSettings: { ...state.generalSettings, senderName: senderNameInput, senderEmail: senderEmailInput, replyToEmail: replyToEmailInput } })); setSenderInputsHydrated(true); }, [automatedEmailsData, replyToEmailInput, senderEmailInput, senderInputsHydrated, senderNameInput, setFormState]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/admin-x-settings/src/components/settings/membership/member-emails/welcome-email-customize-modal.tsx` around lines 466 - 481, The useEffect that hydrates sender fields (useEffect, senderInputsHydrated, automatedEmailsData) can overwrite user-typed values; update it to only write senderNameInput, senderEmailInput and replyToEmailInput into formState.generalSettings via setFormState if the current formState.generalSettings sender fields are empty/unchanged (i.e., not modified by the user) or match an initial default, otherwise skip hydration; read the current values from formState.generalSettings inside the effect (or use a functional setter/read) to avoid stale closures, then call setSenderInputsHydrated(true) only after performing the conditional merge.ghost/core/core/server/services/member-welcome-emails/service.js (2)
47-78: Consider documenting the transporter override pattern.The MagicLink is initialized with a no-op
sendMail(lines 49-51) that gets replaced in#sendEmailVerificationMagicLink. While functional, this pattern could confuse future maintainers. A brief comment explaining why the transporter is overridden per-call would improve clarity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ghost/core/core/server/services/member-welcome-emails/service.js` around lines 47 - 78, Add a brief explanatory comment above the MagicLink transporter.sendMail no-op to clarify that the noop is intentionally provided only as a placeholder because the transporter is overridden on a per-call basis in the `#sendEmailVerificationMagicLink` method; mention that this avoids creating a real transporter during construction and that the actual sendMail implementation is injected at send-time to allow dynamic transporter configuration and testing.
297-322: Transporter mutation is a minor code smell.Reassigning
this.#magicLinkService.transporteron each call (lines 300-313) works because the sequentialawaitin#sendSharedSenderVerificationsprevents internal race conditions. However, if this method were called concurrently from elsewhere, it could cause unexpected behavior. Consider passing the mailer callback toMagicLinkat construction or using a factory pattern.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ghost/core/core/server/services/member-welcome-emails/service.js` around lines 297 - 322, The code mutates this.#magicLinkService.transporter inside `#sendEmailVerificationMagicLink` which is a race-prone code smell; instead, modify MagicLink instantiation or its API so the mail-sending callback is injected immutably (e.g. pass a sendMail/mailFactory into the MagicLink constructor or expose a sendMagicLink option), and then call this.#magicLinkService.sendMagicLink({email, tokenData, sendMail: message => this.#mailer.send({...})}) rather than reassigning this.#magicLinkService.transporter; update callers such as `#sendSharedSenderVerifications` to use the new injection/factory API.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/admin-x-settings/src/components/settings/membership/member-emails.tsx`:
- Around line 163-206: After successful or failed verification in the
useEffect's verify() (which calls verifySenderUpdate with verifyEmailToken),
remove the verifyEmailToken from the URL so the effect won't re-run on
navigation; update the browser URL using history.replaceState (or
URLSearchParams on window.location.href) to strip the token/query parameter (or
fragment) after handling the response (both in the try and catch paths),
referencing verifyEmailToken, verifySenderUpdate and the verify() function so
the token is cleared immediately after processing.
In
`@apps/admin-x-settings/src/components/settings/membership/member-emails/welcome-email-customize-modal.tsx`:
- Around line 466-481: The useEffect that hydrates sender fields (useEffect,
senderInputsHydrated, automatedEmailsData) can overwrite user-typed values;
update it to only write senderNameInput, senderEmailInput and replyToEmailInput
into formState.generalSettings via setFormState if the current
formState.generalSettings sender fields are empty/unchanged (i.e., not modified
by the user) or match an initial default, otherwise skip hydration; read the
current values from formState.generalSettings inside the effect (or use a
functional setter/read) to avoid stale closures, then call
setSenderInputsHydrated(true) only after performing the conditional merge.
In
`@ghost/core/core/server/api/endpoints/utils/validators/input/automated_emails.js`:
- Around line 80-88: The editSenders validator currently ensures sender_name,
sender_email, and sender_reply_to are strings but does not validate email
format; update editSenders to, after calling validateOptionalStringField for
sender_email and sender_reply_to, also check their format using the same
validator.isEmail() calls/pattern used in sendTestEmail so invalid addresses are
rejected early (use the same error messages or similar descriptive errors) and
ensure you reference editSenders, sender_email, sender_reply_to,
validateOptionalStringField and validator.isEmail when locating and implementing
the change.
In `@ghost/core/core/server/services/member-welcome-emails/service.js`:
- Around line 47-78: Add a brief explanatory comment above the MagicLink
transporter.sendMail no-op to clarify that the noop is intentionally provided
only as a placeholder because the transporter is overridden on a per-call basis
in the `#sendEmailVerificationMagicLink` method; mention that this avoids creating
a real transporter during construction and that the actual sendMail
implementation is injected at send-time to allow dynamic transporter
configuration and testing.
- Around line 297-322: The code mutates this.#magicLinkService.transporter
inside `#sendEmailVerificationMagicLink` which is a race-prone code smell;
instead, modify MagicLink instantiation or its API so the mail-sending callback
is injected immutably (e.g. pass a sendMail/mailFactory into the MagicLink
constructor or expose a sendMagicLink option), and then call
this.#magicLinkService.sendMagicLink({email, tokenData, sendMail: message =>
this.#mailer.send({...})}) rather than reassigning
this.#magicLinkService.transporter; update callers such as
`#sendSharedSenderVerifications` to use the new injection/factory API.
In `@ghost/core/test/integration/services/member-welcome-emails.test.js`:
- Around line 488-526: Reset memberWelcomeEmailService.api to null before
calling memberWelcomeEmailService.init() in this test to match the pattern used
elsewhere; locate the test function that currently calls
memberWelcomeEmailService.init() (the spec using
memberWelcomeEmailService.api.sendTestEmail) and add a line that sets
memberWelcomeEmailService.api = null immediately before invoking
memberWelcomeEmailService.init() so the service is initialized consistently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d99e2d82-188b-4593-bd57-220ebd544c70
📒 Files selected for processing (17)
apps/admin-x-framework/src/api/automated-emails.tsapps/admin-x-settings/src/components/settings/email-design/email-preview.tsxapps/admin-x-settings/src/components/settings/membership/member-emails.tsxapps/admin-x-settings/src/components/settings/membership/member-emails/default-welcome-email-values.tsapps/admin-x-settings/src/components/settings/membership/member-emails/welcome-email-customize-modal.tsxapps/admin-x-settings/src/components/settings/membership/member-emails/welcome-email-modal.tsxapps/admin-x-settings/src/hooks/use-welcome-email-sender-details.tsapps/admin-x-settings/src/utils/newsletter-emails.tsapps/admin-x-settings/src/utils/welcome-email-sender-details.tsapps/admin-x-settings/test/acceptance/membership/member-welcome-emails.test.tsapps/admin-x-settings/test/unit/utils/welcome-email-sender-details.test.tsghost/core/core/server/api/endpoints/automated-emails.jsghost/core/core/server/api/endpoints/utils/validators/input/automated_emails.jsghost/core/core/server/services/member-welcome-emails/service.jsghost/core/core/server/web/api/endpoints/admin/routes.jsghost/core/test/e2e-api/admin/automated-emails.test.jsghost/core/test/integration/services/member-welcome-emails.test.js
cmraible
changed the title
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/admin-x-settings/test/acceptance/membership/member-welcome-emails.test.ts (1)
831-856: Assert that the query token reaches the verification endpoint.This test still passes if the component calls
PUT /automated_emails/verifications/without theverifyEmailvalue, because the mock always returns success. Adding a body assertion would cover the actual query-param plumbing.🧪 Suggested assertion
- await mockApi({page, requests: { + const {lastApiRequests} = await mockApi({page, requests: { ...globalDataRequests, ...newslettersRequest, browseConfig: {method: 'GET', path: '/config/', response: responseFixtures.config}, @@ const confirmation = page.getByTestId('confirmation-modal'); await expect(confirmation).toBeVisible(); await expect(confirmation).toContainText('Reply-to address verified'); await expect(page).toHaveURL(/#\/memberemails$/); + await expect.poll(() => lastApiRequests.verifyAutomatedEmailSenders?.body).toEqual({ + token: 'test-verification-token' + }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/admin-x-settings/test/acceptance/membership/member-welcome-emails.test.ts` around lines 831 - 856, The test currently mocks the PUT handler verifyAutomatedEmailSenders but never asserts the request body, so the verifyEmail query param might not be sent; update the test for the 'shows verification confirmation for memberemails verifyEmail token' case to capture/validate the PUT request body sent to verifyAutomatedEmailSenders and assert it contains the verifyEmail token 'test-verification-token' (e.g., ensure the handler or mockApi validation checks request.body.verifyEmail === 'test-verification-token'), while keeping the existing response behavior and other assertions.
🤖 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/admin-x-settings/src/components/settings/membership/member-emails.tsx`:
- Around line 139-145: The effect that reads verifyEmailToken and calls
verifySenderUpdate should be made idempotent: add a ref (e.g.,
submittedTokenRef) to track the token already submitted and check that
verifyEmailToken !== undefined and !== submittedTokenRef.current before calling
verifySenderUpdate; mark submittedTokenRef.current = verifyEmailToken
immediately on submit to prevent duplicate calls across remounts/StrictMode, and
add an isMounted ref or an abort/cleanup inside the useEffect to prevent showing
the modal after unmount (guard any modal/show logic by isMounted.current).
Update the effect that uses verifyEmailToken, verifySenderUpdate, handleError
and any modal state in member-emails.tsx to use these refs so the token is only
consumed once and no state updates occur after unmount.
---
Nitpick comments:
In
`@apps/admin-x-settings/test/acceptance/membership/member-welcome-emails.test.ts`:
- Around line 831-856: The test currently mocks the PUT handler
verifyAutomatedEmailSenders but never asserts the request body, so the
verifyEmail query param might not be sent; update the test for the 'shows
verification confirmation for memberemails verifyEmail token' case to
capture/validate the PUT request body sent to verifyAutomatedEmailSenders and
assert it contains the verifyEmail token 'test-verification-token' (e.g., ensure
the handler or mockApi validation checks request.body.verifyEmail ===
'test-verification-token'), while keeping the existing response behavior and
other assertions.
🪄 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: 30090423-100e-4a5d-89e7-660fb06c493b
📒 Files selected for processing (2)
apps/admin-x-settings/src/components/settings/membership/member-emails.tsxapps/admin-x-settings/test/acceptance/membership/member-welcome-emails.test.ts
apps/admin-x-settings/src/components/settings/membership/member-emails.tsx
Show resolved
Hide resolved
…ngs won't be saved
troyciesco
added
the
ok to merge for me
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/admin-x-settings/src/components/settings/membership/member-emails/welcome-email-customize-modal.tsx`:
- Around line 343-349: The form hydrations and save paths are running before
useBrowseAutomatedEmails() finishes (automatedEmailsData undefined), which
causes overwrites; fix by gating all initial hydration and any calls that rely
on existing automated emails on the browse query completing (i.e. only run
senderInputs hydration, ensureWelcomeEmailRows, and any code that uses
automatedEmails/existingBySlug when automatedEmailsData is not undefined), or
alternatively track form pristine state and only perform the
automatedEmails-derived initialization while the form is pristine; specifically
update logic around automatedEmails/automatedEmailsData, senderInputsHydrated,
ensureWelcomeEmailRows, addAutomatedEmail, editDesign and
editAutomatedEmailSenders so they early-return or defer until
automatedEmailsData is loaded (or until a isPristine flag indicates no user
edits) to avoid clobbering user changes.
- Around line 435-439: The domain comparison in the validation branch using
showSenderEmailInput and senderEmail currently does a case-sensitive check
(senderEmail.split('@')[1] !== senderEmailDomain); normalize both sides to
lower-case before comparing to avoid false negatives (e.g., User@Example.com).
Update the check in the validation block that sets validationErrors.senderEmail
so it compares senderEmail.split('@')[1].toLowerCase() against
senderEmailDomain.toLowerCase() (guarding for existence), leaving the rest of
the validator.isEmail flow unchanged.
🪄 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: 63995cdd-0456-4bfb-b0ca-dc6ae646e54e
📒 Files selected for processing (1)
apps/admin-x-settings/src/components/settings/membership/member-emails/welcome-email-customize-modal.tsx
...-settings/src/components/settings/membership/member-emails/welcome-email-customize-modal.tsx
Show resolved
Hide resolved
...-settings/src/components/settings/membership/member-emails/welcome-email-customize-modal.tsx
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
apps/admin-x-settings/src/components/settings/membership/member-emails.tsx (1)
165-225:⚠️ Potential issue | 🟠 MajorAdd unmount cleanup for async verification side effects.
The token de-dup guard is good, but the async
verify()branch can still execute modal/error side effects after unmount (for example, user navigates away before promise settles). Please add a cancellation cleanup and guard post-await UI/history actions.Suggested hardening
useEffect(() => { if (!verifyEmailToken || !window.location.href.includes('memberemails')) { return; } @@ submittedTokenRef.current = verifyEmailToken; + let cancelled = false; @@ const verify = async () => { try { const {meta: {email_verified: emailVerified} = {}} = await verifySenderUpdate({token: verifyEmailToken}); + if (cancelled) { + return; + } clearVerifyEmailFromRoute(); @@ NiceModal.show(ConfirmationModal, { @@ }); } catch (e) { + if (cancelled) { + return; + } let prompt = 'There was an error verifying your email address. Try again later.'; @@ handleError(e, {withToast: false}); } }; - verify(); + void verify(); + + return () => { + cancelled = true; + }; }, [handleError, verifyEmailToken, verifySenderUpdate]);#!/bin/bash set -euo pipefail FILE="$(fd -i 'member-emails.tsx' apps/admin-x-settings | head -n1)" echo "Inspecting effect body around verification in: $FILE" nl -ba "$FILE" | sed -n '160,235p' echo echo "Verifying whether the effect currently includes cleanup:" python - <<'PY' from pathlib import Path p = next(Path("apps/admin-x-settings").rglob("member-emails.tsx")) text = p.read_text() anchor = "useEffect(() => {" end = "}, [handleError, verifyEmailToken, verifySenderUpdate]);" s = text.find(anchor) e = text.find(end, s) segment = text[s:e] print("contains 'return () =>':", "return () =>" in segment) PY🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/admin-x-settings/src/components/settings/membership/member-emails.tsx` around lines 165 - 225, The effect can perform UI/history side effects after the component unmounts; inside the useEffect wrap the async work in a cancellation guard: add a local let cancelled = false and return a cleanup () => { cancelled = true }; in verify() check if (cancelled) return immediately after any await (both success and catch paths) before calling clearVerifyEmailFromRoute, NiceModal.show, or handleError; keep the token de-dup via submittedTokenRef but ensure any post-await side effects in verify() are skipped when cancelled.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@apps/admin-x-settings/src/components/settings/membership/member-emails.tsx`:
- Around line 165-225: The effect can perform UI/history side effects after the
component unmounts; inside the useEffect wrap the async work in a cancellation
guard: add a local let cancelled = false and return a cleanup () => { cancelled
= true }; in verify() check if (cancelled) return immediately after any await
(both success and catch paths) before calling clearVerifyEmailFromRoute,
NiceModal.show, or handleError; keep the token de-dup via submittedTokenRef but
ensure any post-await side effects in verify() are skipped when cancelled.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8968c36c-058f-4d36-8eae-6b672364c0f4
📒 Files selected for processing (1)
apps/admin-x-settings/src/components/settings/membership/member-emails.tsx
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
apps/admin-x-settings/src/components/settings/membership/member-emails/welcome-email-customize-modal.tsx (1)
343-360:⚠️ Potential issue | 🟠 MajorWait for sender data to settle before hydrating or saving.
automatedEmailsData === undefinedis still being treated like “no welcome emails exist”. If the design query wins the race, the modal becomes editable with fallback sender values; when the browse finishes, Line 471 overwrites those fields, and the same empty-array fallback can driveensureWelcomeEmailRows()into trying to recreate slugs that already exist.useWelcomeEmailSenderDetails()is also backed by another browse (apps/admin-x-settings/src/hooks/use-welcome-email-sender-details.ts:1-20), so these inputs are not necessarily stable just becauseautomatedEmailsDatais defined. Please gate hydration/save on a settled sender-details load state, or only hydrate while the form is still pristine.Also applies to: 372-387, 404-405, 466-481
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/admin-x-settings/src/components/settings/membership/member-emails/welcome-email-customize-modal.tsx` around lines 343 - 360, The modal hydrates and allows saving before sender data is fully settled, causing fallback values and duplicate-slug creation via ensureWelcomeEmailRows; change hydration and save gating so senderInputsHydrated is only set true when the welcome-sender details are fully loaded/stable (not merely automatedEmailsData !== undefined). Update the useWelcomeEmailSenderDetails call to return (or expose) a settled/isLoaded flag (or derive one from its internal browse hook), then only run the existing hydration logic that sets senderNameInput/senderEmailInput/replyToEmailInput and call ensureWelcomeEmailRows when that settled flag is true and the form is still pristine; likewise block the save paths (addAutomatedEmail/editDesign/editAutomatedEmailSenders) until settled to avoid racing writes.
🤖 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/admin-x-settings/src/components/settings/membership/member-emails/welcome-email-customize-modal.tsx`:
- Around line 405-419: The two-step save (editAutomatedEmailSenders followed by
editDesign using buildAutomatedEmailDesignPayload) can partially succeed and
currently treats any failure as a full save error; change the flow to handle
each mutation's errors separately: call
editAutomatedEmailSenders(senderPayload), catch errors from that call and set a
partial-success state (or call onSaveError with a specific partial-failure flag)
before proceeding to editDesign, then call editDesign(...) and catch its errors
independently so you can surface either "sender saved, design failed" or "design
saved, sender failed" messages (and only re-run the mutation that actually
failed on retry); also preserve and display sent_email_verification handling and
toast.info for the sender success path so verification emails are still
indicated when editAutomatedEmailSenders succeeded.
---
Duplicate comments:
In
`@apps/admin-x-settings/src/components/settings/membership/member-emails/welcome-email-customize-modal.tsx`:
- Around line 343-360: The modal hydrates and allows saving before sender data
is fully settled, causing fallback values and duplicate-slug creation via
ensureWelcomeEmailRows; change hydration and save gating so senderInputsHydrated
is only set true when the welcome-sender details are fully loaded/stable (not
merely automatedEmailsData !== undefined). Update the
useWelcomeEmailSenderDetails call to return (or expose) a settled/isLoaded flag
(or derive one from its internal browse hook), then only run the existing
hydration logic that sets senderNameInput/senderEmailInput/replyToEmailInput and
call ensureWelcomeEmailRows when that settled flag is true and the form is still
pristine; likewise block the save paths
(addAutomatedEmail/editDesign/editAutomatedEmailSenders) until settled to avoid
racing writes.
🪄 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: 9e56e2a3-a3c9-4fe9-8c6c-975b969823bf
📒 Files selected for processing (1)
apps/admin-x-settings/src/components/settings/membership/member-emails/welcome-email-customize-modal.tsx
...-settings/src/components/settings/membership/member-emails/welcome-email-customize-modal.tsx
Show resolved
Hide resolved
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 https://linear.app/ghost/issue/NY-1164/add-ability-to-edit-sender-info-using-existing-verification-flow
Summary
Adds the ability for admins to customize sender name, email, and reply-to address for welcome emails, shared across both free and paid welcome email types.
Changes
Backend
PUT /automated_emails/senders/endpoint to updatesender_name,sender_email, andsender_reply_toacross all welcome emails in a single callPUT /automated_emails/verifications/endpoint to verify pending email changes via single-use token (magic link)MemberWelcomeEmailServicenow uses per-automated-email sender overrides when sending, falling back to newsletter/default settingssender_reply_tochanges (uses existingSingleUseTokenProvider)Frontend
useEditAutomatedEmailSendersanduseVerifyAutomatedEmailSenderAPI hooksEmailPreviewcomponent updated to show From/Reply-to in envelope header, with optional recipient/subject line visibility propsverifyEmailquery param on load, shows confirmation modal after successful verificationTests