Updated welcome email modal to utilize useForm#25660
Conversation
WalkthroughRefactors member email UI and tests. In member-email-editor the onChange now stringifies incoming data and only calls onChange when the string differs; useCallback deps updated. welcome-email-modal was migrated to a form-driven flow (useForm/useHandleError), added lexical empty-content validation, moved subject into form state, uses global settings to display sender and conditional Reply‑to, adds Ctrl/Cmd+S save shortcut, and changes dirty/save handling. Public props for WelcomeEmailModal now require Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📥 CommitsReviewing files that changed from the base of the PR and between ade31c42337bcd736a6754af58e495740030ac33 and d2ce740. 📒 Files selected for processing (4)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
🧰 Additional context used🧠 Learnings (2)📚 Learning: 2025-09-11T07:31:40.433ZApplied to files:
📚 Learning: 2025-10-30T17:13:26.190ZApplied to files:
🧬 Code graph analysis (1)apps/admin-x-settings/src/components/settings/membership/member-emails/welcome-email-modal.tsx (4)
🔇 Additional comments (5)
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 |
troyciesco
force-pushed
the
NY-856_welcome-email-modal-use-form
branch
from
74f6a90 to
a1e2e5a
Compare
apps/admin-x-settings/src/components/settings/membership/member-emails/member-email-editor.tsx
Show resolved
Hide resolved
apps/admin-x-settings/src/components/settings/membership/member-emails/welcome-email-modal.tsx
Show resolved
Hide resolved
apps/admin-x-settings/src/components/settings/membership/member-emails/welcome-email-modal.tsx
Outdated
Show resolved
Hide resolved
apps/admin-x-settings/src/components/settings/membership/member-emails/welcome-email-modal.tsx
Outdated
Show resolved
Hide resolved
apps/admin-x-settings/src/components/settings/membership/member-emails/welcome-email-modal.tsx
Show resolved
Hide resolved
apps/admin-x-settings/src/components/settings/membership/member-emails/welcome-email-modal.tsx
Show resolved
Hide resolved
apps/admin-x-settings/src/components/settings/membership/member-emails/welcome-email-modal.tsx
Show resolved
Hide resolved
apps/admin-x-settings/src/components/settings/membership/member-emails/welcome-email-modal.tsx
Show resolved
Hide resolved
apps/admin-x-settings/src/components/settings/membership/member-emails/welcome-email-modal.tsx
Show resolved
Hide resolved
React E2E Tests FailedTo view the Playwright test report locally, run: REPORT_DIR=$(mktemp -d) && gh run download 20041200217 -n playwright-report-react -D "$REPORT_DIR" && npx playwright show-report "$REPORT_DIR" |
Ember E2E Tests FailedTo view the Playwright test report locally, run: REPORT_DIR=$(mktemp -d) && gh run download 20041200217 -n playwright-report-ember -D "$REPORT_DIR" && npx playwright show-report "$REPORT_DIR" |
![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 (3)
apps/admin-x-settings/src/components/settings/membership/member-emails/welcome-email-modal.tsx (3)
59-82: useForm integration is solid; consider using more ofokPropsfor clearer Save UXThe useForm wiring (initialState,
onSave,onSaveError,onValidate) looks correct and thefakeWhenUnchanged: truecall on click nicely supports the “Saved” state even without edits.Two small refinements could improve consistency with the rest of the form system:
- The
Modal’sdirty={saveState === 'unsaved'}is straightforward, but you might want to confirm whether othersaveStatevalues (like a validation-error state, if any) should still mark the form as dirty.- The Save button currently ignores any
okProps.disabled, relying only onsaveState === 'saving'. IfuseFormsetsokProps.disabledwhen invalid or mid-save, consider:<Button color={okProps.color} disabled={okProps.disabled ?? saveState === 'saving'} label={okProps.label || 'Save'} onClick={async () => await handleSave({fakeWhenUnchanged: true})} />This would keep the visual/interaction behavior fully aligned with what
useFormintends to expose.Also applies to: 131-131, 175-179
108-121: Global Cmd/Ctrl+S handler is correct; consider matching click behaviorThe keydown effect is wired cleanly: it guards on
handleSave, adds/removes the listener correctly, and the dependency onhandleSavekeeps it from re‑adding on every render. This is a good fit for the shared useForm save flow.One detail to double‑check: the click handler calls
handleSave({fakeWhenUnchanged: true}), while the keyboard shortcut callshandleSave()with no options. IffakeWhenUnchangedaffects the “Saved” UX (e.g., showing a saved state even when nothing changed), you may want the shortcut to mirror the button:handleSave({fakeWhenUnchanged: true});so Cmd/Ctrl+S and clicking Save feel identical from the user’s perspective.
53-58: Sender/Reply-to display and form wiring align well with the new behaviorThe way you derive and display the sender details looks good:
senderEmail/replyToEmailfalling back todefaultEmailAddress, and hiding the Reply‑to row when those match, gives the intended “no duplicate reply‑to” behavior.- Using
automatedEmail?.sender_name || siteTitlefor the display name keeps things in sync with site defaults while preserving any existing override.- Subject and lexical fields are correctly wired to
formStateandupdateForm, and pair nicely with the updatedMemberEmailEditorthat only emits changes when the underlying JSON string actually changes.Minor robustness thought: if
defaultEmailAddressis ever absent or still loading, you might want a simple fallback (e.g., not rendering the angle‑bracketed email until it’s available) to avoid showing<undefined>, but that’s more of an edge case than a blocker.Also applies to: 123-124, 184-196, 197-207, 218-219
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between 5a8a9c0 and 551d91b76bd0cd9b49f937c869c959817d8caf8a.
📒 Files selected for processing (2)
apps/admin-x-settings/src/components/settings/membership/member-emails/member-email-editor.tsx(1 hunks)apps/admin-x-settings/src/components/settings/membership/member-emails/welcome-email-modal.tsx(5 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: sam-lord
Repo: TryGhost/Ghost PR: 25303
File: ghost/core/core/server/services/email-service/BatchSendingService.js:19-19
Timestamp: 2025-10-30T17:13:26.190Z
Learning: In ghost/core/core/server/services/email-service/BatchSendingService.js and similar files in the Ghost codebase, prefer using `{...options}` spread syntax without explicit guards like `...(options || {})` when spreading potentially undefined objects, as the maintainer prefers cleaner syntax over defensive patterns when the behavior is safe.
Learnt from: danielraffel
Repo: TryGhost/Ghost PR: 25366
File: apps/admin/src/layout/app-sidebar/NavHeader.tsx:13-23
Timestamp: 2025-11-06T05:35:41.162Z
Learning: In apps/admin/src/layout/app-sidebar/NavHeader.tsx, the React component dispatches a synthetic KeyboardEvent to trigger the Ember keymaster.js search modal shortcut. This approach is known to have cross-browser reliability issues but was deferred for architectural refactoring in a separate PR. The recommended fix is to expose a global function or custom DOM event from the Ember app instead of relying on synthetic keyboard events with keymaster.js.
📚 Learning: 2025-09-11T07:31:40.433Z
Learnt from: aileen
Repo: TryGhost/Ghost PR: 24879
File: apps/admin-x-settings/src/hooks/useLimiter.tsx:125-125
Timestamp: 2025-09-11T07:31:40.433Z
Learning: In apps/admin-x-settings/src/hooks/useLimiter.tsx, the useMemo dependency array intentionally includes the entire `config` object rather than just `config.hostSettings?.limits` to enforce re-renders when anything in the config changes, particularly for subscription/plan change scenarios.
Applied to files:
apps/admin-x-settings/src/components/settings/membership/member-emails/welcome-email-modal.tsx
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Unit tests (Node 22.18.0)
- GitHub Check: Admin-X Settings tests
- GitHub Check: Build & Push
🔇 Additional comments (1)
apps/admin-x-settings/src/components/settings/membership/member-emails/member-email-editor.tsx (1)
23-30: handleChange comparison tovaluecleanly avoids spurious dirty stateStringifying once and only calling
onChangewhenstringified !== valueis a good way to ignore Koenig’s initial onChange while still emitting real content changes. Dependency onvaluekeeps the comparison up to date, so this looks solid and aligns with the new useForm-based flow.
apps/admin-x-settings/src/components/settings/membership/member-emails/welcome-email-modal.tsx
Show resolved
Hide resolved
apps/admin-x-settings/src/components/settings/membership/member-emails/welcome-email-modal.tsx
Show resolved
Hide resolved
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-modal.tsx (1)
211-222: Lexical validation error is still not displayed to the user.The
errors.lexicalvalue is set during validation (line 77) but is never rendered in the UI. When a user tries to save with empty content, the save is blocked silently with no visual feedback explaining why.Consider adding an error message below the editor:
<MemberEmailEditor key={automatedEmail?.id || 'new'} nodes='DEFAULT_NODES' placeholder='Write your welcome email content...' singleParagraph={false} value={formState.lexical} onChange={lexical => updateForm(state => ({...state, lexical}))} /> + {errors.lexical && ( + <p className='mt-2 text-sm text-red'>{errors.lexical}</p> + )} </div>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between 551d91b76bd0cd9b49f937c869c959817d8caf8a and f85948dca06e4525005798a2912a6b0efaf6ba65.
📒 Files selected for processing (1)
apps/admin-x-settings/src/components/settings/membership/member-emails/welcome-email-modal.tsx(5 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: danielraffel
Repo: TryGhost/Ghost PR: 25366
File: apps/admin/src/layout/app-sidebar/NavHeader.tsx:13-23
Timestamp: 2025-11-06T05:35:41.162Z
Learning: In apps/admin/src/layout/app-sidebar/NavHeader.tsx, the React component dispatches a synthetic KeyboardEvent to trigger the Ember keymaster.js search modal shortcut. This approach is known to have cross-browser reliability issues but was deferred for architectural refactoring in a separate PR. The recommended fix is to expose a global function or custom DOM event from the Ember app instead of relying on synthetic keyboard events with keymaster.js.
📚 Learning: 2025-09-11T07:31:40.433Z
Learnt from: aileen
Repo: TryGhost/Ghost PR: 24879
File: apps/admin-x-settings/src/hooks/useLimiter.tsx:125-125
Timestamp: 2025-09-11T07:31:40.433Z
Learning: In apps/admin-x-settings/src/hooks/useLimiter.tsx, the useMemo dependency array intentionally includes the entire `config` object rather than just `config.hostSettings?.limits` to enforce re-renders when anything in the config changes, particularly for subscription/plan change scenarios.
Applied to files:
apps/admin-x-settings/src/components/settings/membership/member-emails/welcome-email-modal.tsx
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Admin-X Settings tests
- GitHub Check: Unit tests (Node 22.18.0)
- GitHub Check: Lint
- GitHub Check: Build & Push
🔇 Additional comments (6)
apps/admin-x-settings/src/components/settings/membership/member-emails/welcome-email-modal.tsx (6)
21-44: LGTM!The utility correctly handles the various empty states for Lexical content: null/undefined, JSON parse failures, empty children arrays, and the common case of a single empty paragraph node.
108-121: LGTM!The keyboard shortcut implementation is correct. The effect properly adds/removes the listener and includes
handleSavein the dependency array to avoid stale closure issues.
123-124: LGTM!The fallback logic correctly uses
defaultEmailAddresswhen the automated email doesn't have specific sender/reply-to values configured.
174-179: LGTM!The Save button correctly integrates with
okPropsfrom the form hook and provides sensible fallbacks.
189-196: LGTM!Conditionally hiding Reply-to when it matches the sender email reduces visual noise and follows the original static UI design.
197-208: LGTM!Subject validation errors are properly wired to the TextField component.
apps/admin-x-settings/src/components/settings/membership/member-emails/welcome-email-modal.tsx
Outdated
Show resolved
Hide resolved
React E2E Tests FailedTo view the Playwright test report locally, run: REPORT_DIR=$(mktemp -d) && gh run download 20041724283 -n playwright-report-react -D "$REPORT_DIR" && npx playwright show-report "$REPORT_DIR" |
Ember E2E Tests FailedTo view the Playwright test report locally, run: REPORT_DIR=$(mktemp -d) && gh run download 20041724283 -n playwright-report-ember -D "$REPORT_DIR" && npx playwright show-report "$REPORT_DIR" |
Ember E2E Tests FailedTo view the Playwright test report locally, run: REPORT_DIR=$(mktemp -d) && gh run download 20042071700 -n playwright-report-ember -D "$REPORT_DIR" && npx playwright show-report "$REPORT_DIR" |
React E2E Tests FailedTo view the Playwright test report locally, run: REPORT_DIR=$(mktemp -d) && gh run download 20042071700 -n playwright-report-react -D "$REPORT_DIR" && npx playwright show-report "$REPORT_DIR" |
React E2E Tests FailedTo view the Playwright test report locally, run: REPORT_DIR=$(mktemp -d) && gh run download 20042230592 -n playwright-report-react -D "$REPORT_DIR" && npx playwright show-report "$REPORT_DIR" |
Ember E2E Tests FailedTo view the Playwright test report locally, run: REPORT_DIR=$(mktemp -d) && gh run download 20042230592 -n playwright-report-ember -D "$REPORT_DIR" && npx playwright show-report "$REPORT_DIR" |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
e2e/helpers/pages/admin/settings/sections/member-welcome-emails-section.ts (1)
90-93: UpdatesaveWelcomeEmailto align with new “Save keeps modal open” behaviorThe helper still assumes that clicking Save closes the modal:
await this.modalSaveButton.click(); await this.welcomeEmailModal.waitFor({state: 'hidden'});Per the PR objective, Save now shows “Saved” instead of closing the modal, so this
waitFor({state: 'hidden'})is likely to hang/fail. It would be better to wait for a post-save UI signal (e.g., the “Saved” label) instead of modal disappearance, for example:async saveWelcomeEmail(): Promise<void> { - await this.modalSaveButton.click(); - await this.welcomeEmailModal.waitFor({state: 'hidden'}); + await this.modalSaveButton.click(); + await this.welcomeEmailModal + .getByRole('button', {name: 'Saved'}) + .waitFor({state: 'visible'}); }Adjust the guard to whichever “save complete” indicator the modal guarantees so tests don’t depend on old close-on-save behavior.
♻️ Duplicate comments (1)
apps/admin-x-settings/src/components/settings/membership/member-emails/welcome-email-modal.tsx (1)
21-44: Lexical “empty content” validation error is never surfaced to the user
onValidatecorrectly setserrors.lexicalwhenisEmptyLexical(state.lexical)is true, but the UI never reads or renderserrors.lexical. Onlyerrors.subjectis wired into the SubjectTextField:
isEmptyLexical+ lexical validation: lines 21–44, 76–78- Subject field error wiring: lines 200–207
MemberEmailEditorusage: lines 213–220This creates a broken UX: attempting to save an empty welcome email silently fails (validation blocks save) but shows no explanation near the editor.
Consider surfacing
errors.lexicalbelowMemberEmailEditor(or by styling the editor container) similar to how the subject hint is handled, e.g.:<MemberEmailEditor key={automatedEmail?.id || 'new'} nodes='DEFAULT_NODES' placeholder='Write your welcome email content...' singleParagraph={false} value={formState.lexical} onChange={lexical => updateForm(state => ({...state, lexical}))} /> + {errors.lexical && ( + <p className="mt-2 text-sm text-red"> + {errors.lexical} + </p> + )}Adjust styling to match existing form error patterns, but the key is to read
errors.lexicaland make the validation reason visible.Also applies to: 69-81, 213-220
🧹 Nitpick comments (3)
e2e/helpers/pages/admin/settings/sections/member-welcome-emails-section.ts (1)
27-27: Subject locator should avoid raw CSS selector and.first(); prefer semantic or test-id based targeting
this.modalSubjectInput = this.welcomeEmailModal.locator('input').first();relies on generic CSS and DOM order, which is fragile (e.g., if the Test dropdown input is rendered first) and conflicts with the e2e guideline to avoid CSS selectors in favor of semantic locators ordata-testid.Consider adding a dedicated test id to the subject field in the modal (e.g.,
data-testid="welcome-email-subject") and usinggetByTestIdhere instead:- this.modalSubjectInput = this.welcomeEmailModal.locator('input').first(); + this.modalSubjectInput = this.welcomeEmailModal.getByTestId('welcome-email-subject');This will be more stable and aligned with the e2e locator guidelines.
Based on learnings, expose stable public readonly locators using semantic or test-id based selectors.
apps/admin-x-settings/src/components/settings/membership/member-emails/welcome-email-modal.tsx (2)
59-68:onSaveassumesautomatedEmailis always defined despite optional prop type
WelcomeEmailModalPropsdeclaresautomatedEmail?: AutomatedEmail, butonSavespreads it with a non-null assertion cast:onSave: async (state) => { await editAutomatedEmail({...automatedEmail as AutomatedEmail, ...state}); },If this modal is ever used without an
automatedEmail(e.g., a “create new welcome email” path),...automatedEmailwill throw at runtime. Either:
- Make
automatedEmailrequired in the props for this modal, or- Guard in
onSave(and/or assert early) and decide what should happen when it’s missing (e.g., call a “create” API instead of edit, or bail with a clear error).Example minimal guard:
- onSave: async (state) => { - await editAutomatedEmail({...automatedEmail as AutomatedEmail, ...state}); - }, + onSave: async (state) => { + if (!automatedEmail) { + // Option A: throw, Option B: no-op, Option C: call a "create" path + throw new Error('automatedEmail is required to save a welcome email'); + } + await editAutomatedEmail({...automatedEmail, ...state}); + },Align this with how the modal is actually instantiated so the runtime invariant is explicit.
53-58: Guard against missing default email / title when rendering “From” and “Reply‑to”
senderEmailandreplyToEmailfall back todefaultEmailAddress, andsender_namefalls back tositeTitle:const [defaultEmailAddress] = getSettingValues<string>(settings, ['default_email_address']); const {localSettings} = useSettingGroup(); const [siteTitle] = getSettingValues(localSettings, ['title']) as string[]; const senderEmail = automatedEmail?.sender_email || defaultEmailAddress; const replyToEmail = automatedEmail?.sender_reply_to || defaultEmailAddress;If either setting is missing, the UI can end up rendering something like
From: <undefined>or a blank name, which is confusing in an email configuration modal.Worth adding a small safety fallback (e.g., to
currentUser?.email, or a placeholder like “(no address configured)”), and/or defensive defaults:-const senderEmail = automatedEmail?.sender_email || defaultEmailAddress; -const replyToEmail = automatedEmail?.sender_reply_to || defaultEmailAddress; +const senderEmail = automatedEmail?.sender_email || defaultEmailAddress || currentUser?.email || ''; +const replyToEmail = automatedEmail?.sender_reply_to || defaultEmailAddress || senderEmail;and optionally avoid rendering the angle‑bracketed part if
senderEmailis empty. This keeps the header robust even when settings are misconfigured.Also applies to: 123-125, 183-195
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between f85948dca06e4525005798a2912a6b0efaf6ba65 and f6f1a7488e550b04918062f5ee1a0dcb538ec4cb.
📒 Files selected for processing (3)
apps/admin-x-settings/src/components/settings/membership/member-emails/welcome-email-modal.tsx(5 hunks)e2e/helpers/pages/admin/settings/sections/member-welcome-emails-section.ts(1 hunks)e2e/tests/admin/settings/member-welcome-emails.test.ts(0 hunks)
💤 Files with no reviewable changes (1)
- e2e/tests/admin/settings/member-welcome-emails.test.ts
🧰 Additional context used
📓 Path-based instructions (3)
e2e/**/*.{ts,tsx}
📄 CodeRabbit inference engine (e2e/AGENTS.md)
Prefer less comments and give things clear names
Files:
e2e/helpers/pages/admin/settings/sections/member-welcome-emails-section.ts
e2e/helpers/pages/**/*.ts
📄 CodeRabbit inference engine (e2e/AGENTS.md)
e2e/helpers/pages/**/*.ts: Page Objects should be located inhelpers/pages/directory
Expose locators aspublic readonlyin Page Objects when used with assertions
Page Object methods should use semantic names (e.g.,login()instead ofclickLoginButton())
UsewaitFor()for guards in Page Objects, never useexpect()in page objects
Files:
e2e/helpers/pages/admin/settings/sections/member-welcome-emails-section.ts
e2e/**/*.{ts,js}
📄 CodeRabbit inference engine (AGENTS.md)
E2E tests should use Playwright with Docker container isolation
Files:
e2e/helpers/pages/admin/settings/sections/member-welcome-emails-section.ts
🧠 Learnings (5)
📚 Learning: 2025-11-24T17:29:43.865Z
Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: e2e/AGENTS.md:0-0
Timestamp: 2025-11-24T17:29:43.865Z
Learning: Applies to e2e/helpers/pages/**/*.ts : Expose locators as `public readonly` in Page Objects when used with assertions
Applied to files:
e2e/helpers/pages/admin/settings/sections/member-welcome-emails-section.ts
📚 Learning: 2025-11-24T17:29:43.865Z
Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: e2e/AGENTS.md:0-0
Timestamp: 2025-11-24T17:29:43.865Z
Learning: Applies to e2e/**/*.test.ts : Prefer semantic locators (`getByRole`, `getByLabel`, `getByText`) over test IDs and never use CSS selectors, XPath, nth-child, or class names
Applied to files:
e2e/helpers/pages/admin/settings/sections/member-welcome-emails-section.ts
📚 Learning: 2025-11-24T17:29:43.865Z
Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: e2e/AGENTS.md:0-0
Timestamp: 2025-11-24T17:29:43.865Z
Learning: Applies to e2e/**/*.test.ts : Use `getByTestId()` only when semantic locators are unavailable, and suggest adding `data-testid` to Ghost codebase when needed
Applied to files:
e2e/helpers/pages/admin/settings/sections/member-welcome-emails-section.ts
📚 Learning: 2025-11-24T17:29:43.865Z
Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: e2e/AGENTS.md:0-0
Timestamp: 2025-11-24T17:29:43.865Z
Learning: Applies to e2e/**/*.test.ts : Never use CSS/XPath selectors - only use semantic locators or data-testid
Applied to files:
e2e/helpers/pages/admin/settings/sections/member-welcome-emails-section.ts
📚 Learning: 2025-09-11T07:31:40.433Z
Learnt from: aileen
Repo: TryGhost/Ghost PR: 24879
File: apps/admin-x-settings/src/hooks/useLimiter.tsx:125-125
Timestamp: 2025-09-11T07:31:40.433Z
Learning: In apps/admin-x-settings/src/hooks/useLimiter.tsx, the useMemo dependency array intentionally includes the entire `config` object rather than just `config.hostSettings?.limits` to enforce re-renders when anything in the config changes, particularly for subscription/plan change scenarios.
Applied to files:
apps/admin-x-settings/src/components/settings/membership/member-emails/welcome-email-modal.tsx
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Unit tests (Node 22.18.0)
- GitHub Check: Admin-X Settings tests
- GitHub Check: Lint
- GitHub Check: Build & Push
🔇 Additional comments (1)
apps/admin-x-settings/src/components/settings/membership/member-emails/welcome-email-modal.tsx (1)
108-121: Global Cmd/Ctrl+S handler and Save button wiring look consistent withuseFormpatternThe
useEffectthat registers a windowkeydownlistener for Cmd/Ctrl+S and the Save button’sonClickboth delegate tohandleSave({fakeWhenUnchanged: true}), sharing the same behavior and respectingokProps/saveStatefor button state:
useEffectwithhandleSavedependency and cleanup: lines 108–121- Save button: lines 175–179
This matches the intended “Cmd+S saves without closing” behavior and plugs into the form framework cleanly. No issues spotted here.
Also applies to: 175-179
Ember E2E Tests FailedTo view the Playwright test report locally, run: REPORT_DIR=$(mktemp -d) && gh run download 20043424711 -n playwright-report-ember -D "$REPORT_DIR" && npx playwright show-report "$REPORT_DIR" |
React E2E Tests FailedTo view the Playwright test report locally, run: REPORT_DIR=$(mktemp -d) && gh run download 20043424711 -n playwright-report-react -D "$REPORT_DIR" && npx playwright show-report "$REPORT_DIR" |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
e2e/helpers/pages/admin/settings/sections/member-welcome-emails-section.ts (2)
26-32: Subject input locator may be brittle; prefer semantic locator if available
this.modalSubjectInput = this.welcomeEmailModal.locator('input').first();will break if another input is added before the subject field. If the subject input has an accessible label/placeholder or adata-testid, consider targeting that instead, e.g.:- this.modalSubjectInput = this.welcomeEmailModal.locator('input').first(); + this.modalSubjectInput = this.welcomeEmailModal.getByRole('textbox', {name: 'Subject'}); + // or use a specific data-testid if that’s what the modal exposes + // this.modalSubjectInput = this.welcomeEmailModal.getByTestId('welcome-email-subject');Adjust the locator to whatever semantic hook the modal actually exposes.
Based on learnings, prefer stable semantic locators over positional ones where possible.
11-17: Saved‑state locator aligns with new UX; just confirm it’s stable enough forwaitForAdding
modalSavedButtonand waiting for it after clicking Save matches the new “stay open & show Saved” behavior and keeps the guard nicely encapsulated insaveWelcomeEmail(). The only thing to double‑check is that the “Saved” state isn’t too transient; if the button quickly flips back to “Save”, thiswaitFor({state: 'visible'})could become flaky.If the “Saved” label is short‑lived in practice, consider making the success indicator more durable (e.g., keeping “Saved” visible for a minimum time or exposing a more stable attribute/test id) and point this locator at that. Otherwise, this change looks good.
Also applies to: 26-32, 92-95
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between f6f1a7488e550b04918062f5ee1a0dcb538ec4cb and 0964b3837d11f07f8f89c3372aedec6aa614db70.
📒 Files selected for processing (1)
e2e/helpers/pages/admin/settings/sections/member-welcome-emails-section.ts(3 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
e2e/**/*.{ts,tsx}
📄 CodeRabbit inference engine (e2e/AGENTS.md)
Prefer less comments and give things clear names
Files:
e2e/helpers/pages/admin/settings/sections/member-welcome-emails-section.ts
e2e/helpers/pages/**/*.ts
📄 CodeRabbit inference engine (e2e/AGENTS.md)
e2e/helpers/pages/**/*.ts: Page Objects should be located inhelpers/pages/directory
Expose locators aspublic readonlyin Page Objects when used with assertions
Page Object methods should use semantic names (e.g.,login()instead ofclickLoginButton())
UsewaitFor()for guards in Page Objects, never useexpect()in page objects
Files:
e2e/helpers/pages/admin/settings/sections/member-welcome-emails-section.ts
e2e/**/*.{ts,js}
📄 CodeRabbit inference engine (AGENTS.md)
E2E tests should use Playwright with Docker container isolation
Files:
e2e/helpers/pages/admin/settings/sections/member-welcome-emails-section.ts
🧠 Learnings (4)
📚 Learning: 2025-11-24T17:29:43.865Z
Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: e2e/AGENTS.md:0-0
Timestamp: 2025-11-24T17:29:43.865Z
Learning: Applies to e2e/helpers/pages/**/*.ts : Expose locators as `public readonly` in Page Objects when used with assertions
Applied to files:
e2e/helpers/pages/admin/settings/sections/member-welcome-emails-section.ts
📚 Learning: 2025-11-24T17:29:43.865Z
Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: e2e/AGENTS.md:0-0
Timestamp: 2025-11-24T17:29:43.865Z
Learning: Applies to e2e/**/*.test.ts : Prefer semantic locators (`getByRole`, `getByLabel`, `getByText`) over test IDs and never use CSS selectors, XPath, nth-child, or class names
Applied to files:
e2e/helpers/pages/admin/settings/sections/member-welcome-emails-section.ts
📚 Learning: 2025-11-24T17:29:43.865Z
Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: e2e/AGENTS.md:0-0
Timestamp: 2025-11-24T17:29:43.865Z
Learning: Applies to e2e/**/*.test.ts : Use `getByTestId()` only when semantic locators are unavailable, and suggest adding `data-testid` to Ghost codebase when needed
Applied to files:
e2e/helpers/pages/admin/settings/sections/member-welcome-emails-section.ts
📚 Learning: 2025-11-24T17:29:43.865Z
Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: e2e/AGENTS.md:0-0
Timestamp: 2025-11-24T17:29:43.865Z
Learning: Applies to e2e/**/*.test.ts : Never use CSS/XPath selectors - only use semantic locators or data-testid
Applied to files:
e2e/helpers/pages/admin/settings/sections/member-welcome-emails-section.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Admin-X Settings tests
- GitHub Check: Unit tests (Node 22.18.0)
- GitHub Check: Lint
- GitHub Check: Build & Push
React E2E Tests FailedTo view the Playwright test report locally, run: REPORT_DIR=$(mktemp -d) && gh run download 20044027739 -n playwright-report-react -D "$REPORT_DIR" && npx playwright show-report "$REPORT_DIR" |
cmraible
left a comment
cmraible
left a comment
There was a problem hiding this comment.
Nice! Definitely good call on useForm, that's my bad for not checking our existing patterns for form handling. Everything's working nicely for me locally, and I've confirmed that it fixes the issues mentioned in the PR description.
Left a couple suggestions that are worth looking at before merge, but they're all pretty minor so I'll go ahead and approve so you're not blocked getting this merged in the AM 🙌
apps/admin-x-settings/src/components/settings/membership/member-emails/welcome-email-modal.tsx
Show resolved
Hide resolved
apps/admin-x-settings/src/components/settings/membership/member-emails/welcome-email-modal.tsx
Show resolved
Hide resolved
apps/admin-x-settings/src/components/settings/membership/member-emails/welcome-email-modal.tsx
Outdated
Show resolved
Hide resolved
apps/admin-x-settings/src/components/settings/membership/member-emails/welcome-email-modal.tsx
Show resolved
Hide resolved
apps/admin-x-settings/src/components/settings/membership/member-emails/welcome-email-modal.tsx
Outdated
Show resolved
Hide resolved
troyciesco
force-pushed
the
NY-856_welcome-email-modal-use-form
branch
3 times, most recently
from
252fdf0 to
ade31c4
Compare
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-modal.tsx (1)
175-179: Consider delegating Save button disabled state entirely tookProps.
useFormalready computesokProps.disabled(including the saving state), sodisabled={okProps.disabled ?? saveState === 'saving'}is likely redundant and slightly duplicates that logic. You could simplify to:- disabled={okProps.disabled ?? saveState === 'saving'} + disabled={okProps.disabled}to keep all button-state rules centralized in
useForm.In TryGhost/Ghost’s admin-x-framework `useForm` hook, does `okProps.disabled` already cover the saving state, making extra `saveState === 'saving'` checks redundant?
🧹 Nitpick comments (2)
e2e/helpers/pages/admin/settings/sections/member-welcome-emails-section.ts (1)
11-17: Saved-state locator and save flow look good; consider a more semantic subject locatorThe new
modalSavedButtonlocator and the updatedsaveWelcomeEmail()flow (clickingSavethen waiting for theSavedbutton state) align well with the new UX where the modal stays open and shows a “Saved” state. Naming and usage are clear and consistent with the rest of the Page Object.One small robustness suggestion:
modalSubjectInput = this.welcomeEmailModal.locator('input').first();is still positional. If the modal ever gains another input before Subject, this could start targeting the wrong element. If possible, consider switching to a more semantic or stable locator (e.g., by label, placeholder, ordata-testid) to match how other locators in this class are defined. As per coding guidelines, this will make the tests less brittle over time.Also applies to: 26-31, 92-95
apps/admin-x-settings/src/components/settings/membership/member-emails/welcome-email-modal.tsx (1)
52-55: Sender/Reply‑To display and subject binding match the intended behavior; minor placeholder tweak possible.Deriving From/Reply‑to from
automatedEmailwith a fallback todefault_email_address, hiding Reply‑to when it matches From, and binding subject toformState.subjectall look correct and align with the PR description. One small UX improvement would be to avoid a potential"Welcome to undefined"placeholder ifsiteTitleis unavailable:- placeholder={`Welcome to ${siteTitle}`} + placeholder={`Welcome to ${siteTitle || 'your site'}`}Optional, but keeps the placeholder clean in edge cases.
Also applies to: 123-125, 184-196, 205-207
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between 5f35178035847c92640c09f2f83f6b50057f6bf4 and 252fdf082ebd5a3e8c1f6011f50e8b75687265cb.
📒 Files selected for processing (4)
apps/admin-x-settings/src/components/settings/membership/member-emails/member-email-editor.tsx(1 hunks)apps/admin-x-settings/src/components/settings/membership/member-emails/welcome-email-modal.tsx(4 hunks)e2e/helpers/pages/admin/settings/sections/member-welcome-emails-section.ts(3 hunks)e2e/tests/admin/settings/member-welcome-emails.test.ts(0 hunks)
💤 Files with no reviewable changes (1)
- e2e/tests/admin/settings/member-welcome-emails.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/admin-x-settings/src/components/settings/membership/member-emails/member-email-editor.tsx
🧰 Additional context used
📓 Path-based instructions (3)
e2e/**/*.{ts,tsx}
📄 CodeRabbit inference engine (e2e/AGENTS.md)
Prefer less comments and give things clear names
Files:
e2e/helpers/pages/admin/settings/sections/member-welcome-emails-section.ts
e2e/helpers/pages/**/*.ts
📄 CodeRabbit inference engine (e2e/AGENTS.md)
e2e/helpers/pages/**/*.ts: Page Objects should be located inhelpers/pages/directory
Expose locators aspublic readonlyin Page Objects when used with assertions
Page Object methods should use semantic names (e.g.,login()instead ofclickLoginButton())
UsewaitFor()for guards in Page Objects, never useexpect()in page objects
Files:
e2e/helpers/pages/admin/settings/sections/member-welcome-emails-section.ts
e2e/**/*.{ts,js}
📄 CodeRabbit inference engine (AGENTS.md)
E2E tests should use Playwright with Docker container isolation
Files:
e2e/helpers/pages/admin/settings/sections/member-welcome-emails-section.ts
🧠 Learnings (5)
📚 Learning: 2025-11-24T17:29:43.865Z
Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: e2e/AGENTS.md:0-0
Timestamp: 2025-11-24T17:29:43.865Z
Learning: Applies to e2e/helpers/pages/**/*.ts : Expose locators as `public readonly` in Page Objects when used with assertions
Applied to files:
e2e/helpers/pages/admin/settings/sections/member-welcome-emails-section.ts
📚 Learning: 2025-11-24T17:29:43.865Z
Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: e2e/AGENTS.md:0-0
Timestamp: 2025-11-24T17:29:43.865Z
Learning: Applies to e2e/**/*.test.ts : Prefer semantic locators (`getByRole`, `getByLabel`, `getByText`) over test IDs and never use CSS selectors, XPath, nth-child, or class names
Applied to files:
e2e/helpers/pages/admin/settings/sections/member-welcome-emails-section.ts
📚 Learning: 2025-11-24T17:29:43.865Z
Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: e2e/AGENTS.md:0-0
Timestamp: 2025-11-24T17:29:43.865Z
Learning: Applies to e2e/**/*.test.ts : Use `getByTestId()` only when semantic locators are unavailable, and suggest adding `data-testid` to Ghost codebase when needed
Applied to files:
e2e/helpers/pages/admin/settings/sections/member-welcome-emails-section.ts
📚 Learning: 2025-11-24T17:29:43.865Z
Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: e2e/AGENTS.md:0-0
Timestamp: 2025-11-24T17:29:43.865Z
Learning: Applies to e2e/**/*.test.ts : Never use CSS/XPath selectors - only use semantic locators or data-testid
Applied to files:
e2e/helpers/pages/admin/settings/sections/member-welcome-emails-section.ts
📚 Learning: 2025-09-11T07:31:40.433Z
Learnt from: aileen
Repo: TryGhost/Ghost PR: 24879
File: apps/admin-x-settings/src/hooks/useLimiter.tsx:125-125
Timestamp: 2025-09-11T07:31:40.433Z
Learning: In apps/admin-x-settings/src/hooks/useLimiter.tsx, the useMemo dependency array intentionally includes the entire `config` object rather than just `config.hostSettings?.limits` to enforce re-renders when anything in the config changes, particularly for subscription/plan change scenarios.
Applied to files:
apps/admin-x-settings/src/components/settings/membership/member-emails/welcome-email-modal.tsx
🧬 Code graph analysis (1)
apps/admin-x-settings/src/components/settings/membership/member-emails/welcome-email-modal.tsx (6)
apps/admin-x-framework/src/providers/routing-provider.tsx (1)
useRouting(188-190)apps/admin-x-framework/src/api/current-user.ts (1)
useCurrentUser(14-31)apps/admin-x-framework/src/api/automated-emails.ts (2)
useEditAutomatedEmail(41-50)AutomatedEmail(4-16)apps/admin-x-settings/src/components/providers/global-data-provider.tsx (1)
useGlobalData(75-75)apps/admin-x-framework/src/api/settings.ts (1)
getSettingValues(85-87)ghost/core/core/server/services/koenig/node-renderers/header-v1-renderer.js (1)
div(55-55)
🔇 Additional comments (3)
apps/admin-x-settings/src/components/settings/membership/member-emails/welcome-email-modal.tsx (3)
5-12: Imports and hook usage look consistent.All newly added imports (design system components, useForm/useHandleError, settings/global data hooks) are used and align with existing admin-x patterns.
20-43: Lexical/content validation and error surfacing are wired correctly.
isEmptyLexicalplus theonValidatechecks, border color switch, and redHintensure both subject and body must be non-empty and give clear feedback when Save is blocked, rather than failing silently.Also applies to: 66-78, 213-224
105-121: Cmd/Ctrl+S handler withuseRefis a solid pattern.Storing
handleSavein a ref and registering the keydown listener once (empty deps) avoids both stale closures and repeatedly adding/removing listeners on each keystroke.
apps/admin-x-settings/src/components/settings/membership/member-emails/welcome-email-modal.tsx
Show resolved
Hide resolved
troyciesco
force-pushed
the
NY-856_welcome-email-modal-use-form
branch
from
ade31c4 to
5f1f76a
Compare
troyciesco
force-pushed
the
NY-856_welcome-email-modal-use-form
branch
from
5f1f76a to
d2ce740
Compare
2 participants
ref https://linear.app/ghost/issue/NY-856
welcome-email-modal.tsxto utilize ouruseFormhook and fixes a few related issues in the processcmd + Snow saves changes, without closing modal (closes https://linear.app/ghost/issue/NY-853)