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 a new automated email design API module with types and read/edit hooks for Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
cmraible
changed the title
cmraible
force-pushed
the
chris-ny-1165-wiring-wire-up-the-customization-modal-to-the-api-for
branch
2 times, most recently
from
4f06eda to
0477591
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/admin-x-settings/src/components/settings/email-design/email-design-modal.tsx (1)
61-70:⚠️ Potential issue | 🟠 MajorPrevent Cmd/Ctrl+S from bypassing disabled/loading save state.
The button is disabled when
isLoading || saveDisabled, but the keyboard handler still callsonSavedirectly, allowing duplicate submits during in-flight saves.Proposed fix
useEffect(() => { const handleKeyDown = (e: KeyboardEvent) => { - if ((e.metaKey || e.ctrlKey) && e.key === 's') { - e.preventDefault(); - onSaveRef.current(); - } + if (!(e.metaKey || e.ctrlKey) || e.key !== 's') { + return; + } + if (!open || isLoading || okProps?.disabled) { + return; + } + e.preventDefault(); + onSaveRef.current(); }; window.addEventListener('keydown', handleKeyDown); return () => window.removeEventListener('keydown', handleKeyDown); -}, []); +}, [open, isLoading, okProps]);Also applies to: 113-114
🤖 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/email-design/email-design-modal.tsx` around lines 61 - 70, The keyboard handler (handleKeyDown used in the useEffect) currently calls onSaveRef.current() unconditionally, allowing Cmd/Ctrl+S to trigger saves even when isLoading or saveDisabled are true; update handleKeyDown to preventDefault then check the current save state and only call onSaveRef.current() when !isLoading && !saveDisabled (also guard that onSaveRef.current is defined). Apply the same conditional check to the other keyboard shortcut handler in this component (the similar call around lines 113-114) so keyboard-triggered saves respect the disabled/loading state.
🧹 Nitpick comments (4)
apps/admin-x-settings/src/components/settings/email-design/email-design-modal.tsx (1)
84-88: Consider replacing nested ternary with a small map/helper.This is readable now, but a mapping object would be easier to extend as button color variants grow.
🤖 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/email-design/email-design-modal.tsx` around lines 84 - 88, The nested ternary assigning saveButtonClassName from saveColor is fragile and hard to extend; replace it with a small mapping or helper function (e.g., a colorToClassMap object or getSaveButtonClass(saveColor)) that returns the class string for 'green', 'red', or a default undefined, and use that to set saveButtonClassName; update any references to saveColor/saveButtonClassName in the email-design-modal component accordingly so adding new variants only requires adding map entries or cases in the helper.apps/admin-x-framework/src/api/automated-email-design.ts (1)
32-32: Narrow the edit payload type to writable fields only.Current typing allows read-only/system-managed fields (
slug, timestamps) to be passed toPUT, which weakens compile-time safety.Proposed refactor
-export type EditAutomatedEmailDesign = Omit<Partial<AutomatedEmailDesign>, 'id'>; +export type EditAutomatedEmailDesign = Partial< + Omit<AutomatedEmailDesign, 'id' | 'slug' | 'created_at' | 'updated_at'> +>;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/admin-x-framework/src/api/automated-email-design.ts` at line 32, EditAutomatedEmailDesign currently allows system-managed/read-only fields; change its definition to only include writable fields by omitting read-only keys. Replace the current type alias (EditAutomatedEmailDesign = Omit<Partial<AutomatedEmailDesign>, 'id'>) with a type that omits id, slug and timestamp fields (for example: Partial<Omit<AutomatedEmailDesign, 'id' | 'slug' | 'createdAt' | 'updatedAt'>>), or explicitly Pick the writable properties from AutomatedEmailDesign; update the alias and any usages to use this narrowed type.apps/admin-x-settings/src/components/settings/membership/member-emails/welcome-email-customize-modal.tsx (2)
359-361: Replacevoidoperator with proper promise handling.SonarCloud flags the
voidoperator usage. Using.catch()or restructuring makes intent clearer.♻️ Proposed fix
const handleClose = useCallback(() => { - void modal.hide(); + modal.hide().catch(() => { + // Modal hide errors are non-critical + }); }, [modal]);Alternatively, if
modal.hide()never actually rejects andvoidis used purely to silence the floating promise lint, you can use a comment to clarify intent:const handleClose = useCallback(() => { + // Fire-and-forget: modal animation handles its own cleanup - void modal.hide(); + modal.hide(); }, [modal]);🤖 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 359 - 361, The handleClose callback currently uses the void operator to ignore the promise from modal.hide(); replace that with explicit promise handling: either make handleClose async and await modal.hide() inside a try/catch (or call modal.hide().catch(err => {/* log or ignore */})), or if modal.hide() is truly non-rejecting add an explanatory eslint-disable comment; update the handleClose function (referenced by name) to use one of these patterns so the floating promise is handled instead of using void.
179-201: Consider extracting nested ternary for readability.The nested ternary handling loading → error → content states is functional but harder to scan. Extracting to a helper function or using early returns would improve clarity.
♻️ Suggested refactor
+const SidebarContent: React.FC<{ + isLoading: boolean; + errorMessage?: string; + generalSettings: GeneralSettings; + onGeneralChange: (updates: Partial<GeneralSettings>) => void; + siteTitle: string | undefined; + emailDomain: string; +}> = ({isLoading, errorMessage, generalSettings, onGeneralChange, siteTitle, emailDomain}) => { + if (isLoading) { + return ( + <div className="flex flex-1 items-center justify-center"> + <LoadingIndicator size="md" /> + </div> + ); + } + if (errorMessage) { + return ( + <div className="flex flex-1 items-center justify-center px-6 text-center text-sm text-gray-700 dark:text-gray-300"> + {errorMessage} + </div> + ); + } + return ( + <> + <TabsContent className='min-h-0 flex-1 overflow-y-auto px-5 pb-5' value="general"> + <GeneralTab + emailDomain={emailDomain} + generalSettings={generalSettings} + siteTitle={siteTitle} + onGeneralChange={onGeneralChange} + /> + </TabsContent> + <TabsContent className='min-h-0 flex-1 overflow-y-auto px-5 pb-5' value="design"> + <DesignTab /> + </TabsContent> + </> + ); +};🤖 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 179 - 201, The JSX contains a nested ternary rendering isLoading → errorMessage → content which reduces readability; refactor by extracting the conditional rendering into a small helper (e.g. renderContent) or use early return branches inside the WelcomeEmailCustomizeModal component: check isLoading first and return the LoadingIndicator wrapper, then check errorMessage and return the error wrapper, otherwise return the TabsContent block with GeneralTab and DesignTab; reference the existing symbols isLoading, errorMessage, TabsContent, GeneralTab and DesignTab when moving the blocks so behavior remains identical.
🤖 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/email-design/color-picker-field.tsx`:
- Around line 17-28: normalizeColorValue currently maps all non-hex inputs to
'#ffffff', which loses semantic values like 'accent'; update normalizeColorValue
(in color-picker-field.tsx) to handle the 'accent' token explicitly (e.g., add a
case for 'accent' that returns 'accent' instead of '#ffffff') so accent-backed
fields from DEFAULT_EMAIL_DESIGN are preserved and not converted to white on
edit.
In `@e2e/tests/admin/settings/member-welcome-emails.test.ts`:
- Around line 173-230: Rename the three test titles to the lowercase "what is
tested - expected outcome" format by updating the string literals passed to the
test(...) calls: change 'saving design settings persists to the API' -> 'save
design settings - persists to api', 'saving general settings persists to the
API' -> 'save general settings - persists to api', and 'saved design settings
load when modal is reopened' -> 'saved design settings - loads when modal is
reopened'; keep the test bodies and identifiers (MemberWelcomeEmailsSection,
customizeModalFooterTextarea, saveCustomizeModal, etc.) unchanged so only the
human-readable test names are modified to match the guideline.
---
Outside diff comments:
In
`@apps/admin-x-settings/src/components/settings/email-design/email-design-modal.tsx`:
- Around line 61-70: The keyboard handler (handleKeyDown used in the useEffect)
currently calls onSaveRef.current() unconditionally, allowing Cmd/Ctrl+S to
trigger saves even when isLoading or saveDisabled are true; update handleKeyDown
to preventDefault then check the current save state and only call
onSaveRef.current() when !isLoading && !saveDisabled (also guard that
onSaveRef.current is defined). Apply the same conditional check to the other
keyboard shortcut handler in this component (the similar call around lines
113-114) so keyboard-triggered saves respect the disabled/loading state.
---
Nitpick comments:
In `@apps/admin-x-framework/src/api/automated-email-design.ts`:
- Line 32: EditAutomatedEmailDesign currently allows system-managed/read-only
fields; change its definition to only include writable fields by omitting
read-only keys. Replace the current type alias (EditAutomatedEmailDesign =
Omit<Partial<AutomatedEmailDesign>, 'id'>) with a type that omits id, slug and
timestamp fields (for example: Partial<Omit<AutomatedEmailDesign, 'id' | 'slug'
| 'createdAt' | 'updatedAt'>>), or explicitly Pick the writable properties from
AutomatedEmailDesign; update the alias and any usages to use this narrowed type.
In
`@apps/admin-x-settings/src/components/settings/email-design/email-design-modal.tsx`:
- Around line 84-88: The nested ternary assigning saveButtonClassName from
saveColor is fragile and hard to extend; replace it with a small mapping or
helper function (e.g., a colorToClassMap object or
getSaveButtonClass(saveColor)) that returns the class string for 'green', 'red',
or a default undefined, and use that to set saveButtonClassName; update any
references to saveColor/saveButtonClassName in the email-design-modal component
accordingly so adding new variants only requires adding map entries or cases in
the helper.
In
`@apps/admin-x-settings/src/components/settings/membership/member-emails/welcome-email-customize-modal.tsx`:
- Around line 359-361: The handleClose callback currently uses the void operator
to ignore the promise from modal.hide(); replace that with explicit promise
handling: either make handleClose async and await modal.hide() inside a
try/catch (or call modal.hide().catch(err => {/* log or ignore */})), or if
modal.hide() is truly non-rejecting add an explanatory eslint-disable comment;
update the handleClose function (referenced by name) to use one of these
patterns so the floating promise is handled instead of using void.
- Around line 179-201: The JSX contains a nested ternary rendering isLoading →
errorMessage → content which reduces readability; refactor by extracting the
conditional rendering into a small helper (e.g. renderContent) or use early
return branches inside the WelcomeEmailCustomizeModal component: check isLoading
first and return the LoadingIndicator wrapper, then check errorMessage and
return the error wrapper, otherwise return the TabsContent block with GeneralTab
and DesignTab; reference the existing symbols isLoading, errorMessage,
TabsContent, GeneralTab and DesignTab when moving the blocks so behavior remains
identical.
🪄 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: b3512f00-97eb-4323-b00e-d5ebc6fd55a5
📒 Files selected for processing (10)
apps/admin-x-framework/src/api/automated-email-design.tsapps/admin-x-settings/src/components/settings/email-design/color-picker-field.tsxapps/admin-x-settings/src/components/settings/email-design/email-design-modal.tsxapps/admin-x-settings/src/components/settings/email-design/header-image-field.tsxapps/admin-x-settings/src/components/settings/email-design/types.tsapps/admin-x-settings/src/components/settings/membership/member-emails/welcome-email-customize-modal.tsxe2e/helpers/pages/admin/settings/sections/member-welcome-emails-section.tse2e/tests/admin/settings/member-welcome-emails.test.tsghost/admin/app/services/state-bridge.jsghost/admin/tests/unit/services/state-bridge-test.js
apps/admin-x-settings/src/components/settings/email-design/color-picker-field.tsx
Outdated
Show resolved
Hide resolved
troyciesco
reviewed
Apr 2, 2026
...-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
troyciesco
reviewed
Apr 2, 2026
...-settings/src/components/settings/membership/member-emails/welcome-email-customize-modal.tsx
Outdated
Show resolved
Hide resolved
Loaded persisted design settings into the welcome email modal, saved changes back to the design endpoint, and added E2E coverage for the customization flow.
Matched the singleton design endpoint contract so the frontend no longer sends a fake id when saving customization settings.
Prevented successful automated email design saves from surfacing as frontend errors when React-only data types invalidate through the admin state bridge.
Mapped backend values such as light and transparent to valid hex colors so the design color picker can render saved settings without crashing.
Kept the customization modal open after save, matched the shared dirty-confirmation flow, and used the same toast and retry patterns as the newsletter editor.
cmraible
force-pushed
the
chris-ny-1165-wiring-wire-up-the-customization-modal-to-the-api-for
branch
from
0477591 to
27c0a86
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
apps/admin-x-settings/src/components/settings/email-design/color-picker-field.tsx (1)
18-24:⚠️ Potential issue | 🟠 Major
accentstill normalizes to white, which loses persisted semantic intent.On Line 22,
accentfalls into the default branch and becomes#ffffff. That can cause accent-backed fields to render/save as white after user interaction.Suggested fix
const normalizeColorValue = (value: string): string => { if (VALID_HEX.test(value)) { return value; } switch (value) { case 'light': case 'transparent': return '#ffffff'; + case 'accent': + // Map to the resolved accent hex used by this settings flow. + // (Use the same resolver/source of truth already used elsewhere.) + return resolvedAccentHex; default: return '#ffffff'; } };🤖 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/email-design/color-picker-field.tsx` around lines 18 - 24, The switch on the incoming value currently falls through to the default and maps 'accent' to white; add an explicit case for 'accent' in the switch (the same switch handling value) so it returns the intended accent color hex (or the theme ACCENT color constant) instead of '#ffffff', ensuring accent-backed fields preserve their semantic color rather than normalizing to white.
🤖 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/email-design/color-picker-field.tsx`:
- Around line 18-24: The switch on the incoming value currently falls through to
the default and maps 'accent' to white; add an explicit case for 'accent' in the
switch (the same switch handling value) so it returns the intended accent color
hex (or the theme ACCENT color constant) instead of '#ffffff', ensuring
accent-backed fields preserve their semantic color rather than normalizing to
white.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 66f17e50-0d82-4f74-a4c6-ceab2e1aa2f2
📒 Files selected for processing (10)
apps/admin-x-framework/src/api/automated-email-design.tsapps/admin-x-settings/src/components/settings/email-design/color-picker-field.tsxapps/admin-x-settings/src/components/settings/email-design/email-design-modal.tsxapps/admin-x-settings/src/components/settings/email-design/header-image-field.tsxapps/admin-x-settings/src/components/settings/email-design/types.tsapps/admin-x-settings/src/components/settings/membership/member-emails/welcome-email-customize-modal.tsxe2e/helpers/pages/admin/settings/sections/member-welcome-emails-section.tse2e/tests/admin/settings/member-welcome-emails.test.tsghost/admin/app/services/state-bridge.jsghost/admin/tests/unit/services/state-bridge-test.js
✅ Files skipped from review due to trivial changes (5)
- ghost/admin/app/services/state-bridge.js
- apps/admin-x-settings/src/components/settings/email-design/header-image-field.tsx
- ghost/admin/tests/unit/services/state-bridge-test.js
- apps/admin-x-framework/src/api/automated-email-design.ts
- apps/admin-x-settings/src/components/settings/email-design/email-design-modal.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/admin-x-settings/src/components/settings/membership/member-emails/welcome-email-customize-modal.tsx
ref #27070 Accent-backed button and link fields now resolve to the site accent color in the picker instead of rendering as white.
ref #27070 The new test titles now follow the lowercase what-is-tested to expected-outcome naming convention used elsewhere in the suite.
ref #27070 The customize modal now exposes section title color and the preview headings render with the configured value.
ref #27070 The welcome email UI now matches the existing regular link-style contract instead of presenting regular links as italic.
ref #27070 The API mapping and save payload now spread persisted design fields so future additions are preserved while preview-only fields stay local.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
e2e/tests/admin/settings/member-welcome-emails.test.ts (1)
20-44: Consider importingAutomatedEmailDesigntype from the framework package.The
AutomatedEmailDesignResponseinterface duplicates the shape defined inapps/admin-x-framework/src/api/automated-email-design.ts. Importing from the shared location would reduce maintenance burden if fields change.However, keeping it local in e2e tests is also reasonable for isolation and to avoid coupling test code to implementation details.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/tests/admin/settings/member-welcome-emails.test.ts` around lines 20 - 44, Replace the duplicated AutomatedEmailDesign type with the shared type from the framework: remove the local AutomatedEmailDesign interface declaration and import AutomatedEmailDesign from the framework package used by the repo, then update AutomatedEmailDesignResponse to reference the imported AutomatedEmailDesign (i.e., automated_email_design: AutomatedEmailDesign[]); keep the local AutomatedEmailDesignResponse interface if still needed for typing the API response.apps/admin-x-settings/test/unit/email-design/color-fields.test.tsx (1)
21-25: Consider using a more specific selector or test ID for the swatch element.
trigger.querySelector('span')relies on internal DOM structure. IfColorPickerFieldchanges its markup, these tests will break. Adding adata-testidto the swatch span would make the tests more resilient.Also applies to: 39-43
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/admin-x-settings/test/unit/email-design/color-fields.test.tsx` around lines 21 - 25, The test relies on fragile DOM traversal (trigger.querySelector('span')) to find the swatch; update the ColorPickerField implementation to add a stable data-testid attribute on the swatch element (e.g., data-testid="button-color-swatch") and change the tests in color-fields.test.tsx (the assertions around trigger and swatch) to select the swatch via screen.getByTestId('button-color-swatch') instead of querySelector; do the same for the other case referenced (lines 39-43) so tests use the new data-testid and remain resilient to markup changes.apps/admin-x-settings/src/components/settings/email-design/email-design-modal.tsx (1)
68-75: Consider consolidating color-to-className mapping.The conditional chain for
saveButtonClassNamehandles three cases but only two have custom classes. If more colors are added later, this could grow unwieldy.♻️ Optional: Extract to a mapping object
- const saveButtonClassName = saveColor === 'green' - ? 'bg-green text-white hover:bg-green/90' - : saveColor === 'red' - ? 'bg-destructive text-destructive-foreground hover:bg-destructive/90' - : undefined; + const colorClassMap: Record<string, string | undefined> = { + green: 'bg-green text-white hover:bg-green/90', + red: 'bg-destructive text-destructive-foreground hover:bg-destructive/90' + }; + const saveButtonClassName = colorClassMap[saveColor];🤖 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/email-design/email-design-modal.tsx` around lines 68 - 75, The saveButtonClassName logic uses a conditional chain on saveColor (derived from okProps.color) which will become hard to maintain as colors grow; replace the ternary chain by extracting a color→className mapping (e.g., a plain object or helper function) and look up saveColor to produce saveButtonClassName (falling back to undefined), updating the code that computes saveButtonClassName to use that map instead of the nested conditional.apps/admin-x-settings/src/components/settings/membership/member-emails/welcome-email-customize-modal.tsx (3)
254-266: Same type-safety concern in payload builder.The
Object.fromEntriespattern here has the same issue - TypeScript cannot verify the resulting object matchesEditAutomatedEmailDesign. Consider explicit field mapping or a helper type that enforces the shape.🤖 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 254 - 266, The payload builder buildAutomatedEmailDesignPayload uses Object.fromEntries which loses TypeScript guarantees for EditAutomatedEmailDesign; replace the dynamic fromEntries approach with an explicitly typed mapping: iterate state.designSettings and reduce into a typed accumulator of EditAutomatedEmailDesign (or construct an explicit object that picks each allowed design field), filtering with PREVIEW_ONLY_FIELDS and NON_DESIGN_FIELDS but ensuring the result is declared as EditAutomatedEmailDesign so the compiler can verify shape; ensure header_image, show_header_title, show_badge and footer_content are assigned as before from state.generalSettings and return the typed object.
239-252: Type assertion loses compile-time safety.The
Object.fromEntries(...) as PersistedEmailDesignSettingscast bypasses TypeScript's structural checking. If a field is missing fromapiDataorNON_DESIGN_FIELDSis misconfigured, the returned object may not actually satisfyPersistedEmailDesignSettings.Consider adding runtime validation or using a more explicit mapping that TypeScript can verify:
♻️ Suggested explicit mapping for type safety
export function mapApiToDesignSettings( apiData: PersistedEmailDesignSettings ): EmailDesignSettings { - const persistedDesign = Object.fromEntries( - Object.entries(apiData).filter(([key]) => !NON_DESIGN_FIELDS.has(key)) - ) as PersistedEmailDesignSettings; - return { - ...persistedDesign, + background_color: apiData.background_color, + title_font_category: apiData.title_font_category, + title_font_weight: apiData.title_font_weight, + body_font_category: apiData.body_font_category, + header_background_color: apiData.header_background_color, + section_title_color: apiData.section_title_color, + button_color: apiData.button_color, + button_style: apiData.button_style, + button_corners: apiData.button_corners, + link_color: apiData.link_color, + link_style: apiData.link_style, + image_corners: apiData.image_corners, + divider_color: apiData.divider_color, // Local-only fields not stored in the backend post_title_color: DEFAULT_EMAIL_DESIGN.post_title_color, title_alignment: DEFAULT_EMAIL_DESIGN.title_alignment }; }This trades some verbosity for compile-time guarantees that all required fields are present.
🤖 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 239 - 252, The current mapApiToDesignSettings uses Object.fromEntries casted to PersistedEmailDesignSettings which loses compile-time safety; replace the unsafe cast by explicitly mapping required design fields from apiData (or validating them at runtime) so TypeScript can verify shape — in mapApiToDesignSettings iterate the known design property names (those expected in PersistedEmailDesignSettings), pick them from apiData (or apply defaults from DEFAULT_EMAIL_DESIGN), and build a properly typed object before returning; reference NON_DESIGN_FIELDS to exclude non-design keys and ensure the returned EmailDesignSettings includes post_title_color and title_alignment set from DEFAULT_EMAIL_DESIGN if missing, or throw/return an error if required fields are absent.
298-305: Consider distinguishing between fetch errors and missing design data.The check
if (!design)could be true due to:
- Still loading (covered by
isLoading)- Fetch error (covered by
isError)- Unexpected empty response
The error message "Unable to load email design settings" is appropriate for case 2, but case 3 might warrant a different message. Currently this is fine since the API should always return design data, but consider logging case 3 separately for debugging.
🤖 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 298 - 305, In onSave, distinguish why design is falsy: check isLoading and isError first and handle those separately (use existing isLoading to avoid premature error, and when isError show the current 'Unable to load email design settings' toast with SAVE_ERROR_TOAST_ID and call setHasSaveError(true)); if neither isLoading nor isError but design is still missing, treat it as an unexpected empty response—use a different toast message and log/debug details (e.g. include state/context) before throwing a distinct Error to aid debugging; update references to onSave, design, isLoading, isError, SAVE_ERROR_TOAST_ID, and setHasSaveError accordingly.
🤖 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 359-363: The Retry button is disabled during fetch errors because
the component uses the combined isLoading || isError to drive the modal/button
loading state; create a distinct save-loading flag (e.g., isSaving) or change
the loading prop to only reflect the save operation (not initial fetch) so
retries remain clickable. Update the code that sets isLoading on the
Modal/button to use the save-specific flag (replace isLoading || isError with
isSaving or isLoading && !isError) and ensure the save handler (onOk/save
function referenced where modalOkProps and hasSaveError are used) toggles this
save-loading flag during the save request so modalOkProps (color/label) and
clickability behave correctly.
- Line 373: The modal's isLoading prop in welcome-email-customize-modal.tsx is
currently set to isLoading || isError which inadvertently disables the modal's
Save/Retry button (email-design-modal.tsx disables the button when isLoading is
true). Change the modal invocation to pass only the true loading state
(isLoading) and move explicit control of the button state into the
modalOkProps/okProps (e.g., set modalOkProps.okProps.disabled to isLoading ||
saveDisabled) so that fetch errors (isError) do not automatically disable the
Retry button but you still can disable the button during real loading or when
saveDisabled is true.
---
Nitpick comments:
In
`@apps/admin-x-settings/src/components/settings/email-design/email-design-modal.tsx`:
- Around line 68-75: The saveButtonClassName logic uses a conditional chain on
saveColor (derived from okProps.color) which will become hard to maintain as
colors grow; replace the ternary chain by extracting a color→className mapping
(e.g., a plain object or helper function) and look up saveColor to produce
saveButtonClassName (falling back to undefined), updating the code that computes
saveButtonClassName to use that map instead of the nested conditional.
In
`@apps/admin-x-settings/src/components/settings/membership/member-emails/welcome-email-customize-modal.tsx`:
- Around line 254-266: The payload builder buildAutomatedEmailDesignPayload uses
Object.fromEntries which loses TypeScript guarantees for
EditAutomatedEmailDesign; replace the dynamic fromEntries approach with an
explicitly typed mapping: iterate state.designSettings and reduce into a typed
accumulator of EditAutomatedEmailDesign (or construct an explicit object that
picks each allowed design field), filtering with PREVIEW_ONLY_FIELDS and
NON_DESIGN_FIELDS but ensuring the result is declared as
EditAutomatedEmailDesign so the compiler can verify shape; ensure header_image,
show_header_title, show_badge and footer_content are assigned as before from
state.generalSettings and return the typed object.
- Around line 239-252: The current mapApiToDesignSettings uses
Object.fromEntries casted to PersistedEmailDesignSettings which loses
compile-time safety; replace the unsafe cast by explicitly mapping required
design fields from apiData (or validating them at runtime) so TypeScript can
verify shape — in mapApiToDesignSettings iterate the known design property names
(those expected in PersistedEmailDesignSettings), pick them from apiData (or
apply defaults from DEFAULT_EMAIL_DESIGN), and build a properly typed object
before returning; reference NON_DESIGN_FIELDS to exclude non-design keys and
ensure the returned EmailDesignSettings includes post_title_color and
title_alignment set from DEFAULT_EMAIL_DESIGN if missing, or throw/return an
error if required fields are absent.
- Around line 298-305: In onSave, distinguish why design is falsy: check
isLoading and isError first and handle those separately (use existing isLoading
to avoid premature error, and when isError show the current 'Unable to load
email design settings' toast with SAVE_ERROR_TOAST_ID and call
setHasSaveError(true)); if neither isLoading nor isError but design is still
missing, treat it as an unexpected empty response—use a different toast message
and log/debug details (e.g. include state/context) before throwing a distinct
Error to aid debugging; update references to onSave, design, isLoading, isError,
SAVE_ERROR_TOAST_ID, and setHasSaveError accordingly.
In `@apps/admin-x-settings/test/unit/email-design/color-fields.test.tsx`:
- Around line 21-25: The test relies on fragile DOM traversal
(trigger.querySelector('span')) to find the swatch; update the ColorPickerField
implementation to add a stable data-testid attribute on the swatch element
(e.g., data-testid="button-color-swatch") and change the tests in
color-fields.test.tsx (the assertions around trigger and swatch) to select the
swatch via screen.getByTestId('button-color-swatch') instead of querySelector;
do the same for the other case referenced (lines 39-43) so tests use the new
data-testid and remain resilient to markup changes.
In `@e2e/tests/admin/settings/member-welcome-emails.test.ts`:
- Around line 20-44: Replace the duplicated AutomatedEmailDesign type with the
shared type from the framework: remove the local AutomatedEmailDesign interface
declaration and import AutomatedEmailDesign from the framework package used by
the repo, then update AutomatedEmailDesignResponse to reference the imported
AutomatedEmailDesign (i.e., automated_email_design: AutomatedEmailDesign[]);
keep the local AutomatedEmailDesignResponse interface if still needed for typing
the API response.
🪄 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: 572d8839-6acc-4e9e-9d3b-2214503ddb60
📒 Files selected for processing (12)
apps/admin-x-settings/src/components/settings/email-design/color-picker-field.tsxapps/admin-x-settings/src/components/settings/email-design/design-fields/button-color-field.tsxapps/admin-x-settings/src/components/settings/email-design/design-fields/link-color-field.tsxapps/admin-x-settings/src/components/settings/email-design/design-fields/link-style-field.tsxapps/admin-x-settings/src/components/settings/email-design/email-design-modal.tsxapps/admin-x-settings/src/components/settings/email-design/welcome-email-preview-content.tsxapps/admin-x-settings/src/components/settings/membership/member-emails/welcome-email-customize-modal.tsxapps/admin-x-settings/test/unit/email-design/color-fields.test.tsxapps/admin-x-settings/test/unit/email-design/design-payload.test.tsapps/admin-x-settings/test/unit/email-design/link-style.test.tsxapps/admin-x-settings/test/unit/email-design/section-title-color.test.tsxe2e/tests/admin/settings/member-welcome-emails.test.ts
✅ Files skipped from review due to trivial changes (1)
- apps/admin-x-settings/src/components/settings/email-design/design-fields/button-color-field.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/admin-x-settings/src/components/settings/email-design/color-picker-field.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
cmraible
commented
Apr 2, 2026
Comment on lines
+38
to
+39
| post_title_color: null, | ||
| title_alignment: 'center' |
Collaborator
Author
There was a problem hiding this comment.
These aren't used for welcome emails at all - tracking this as a cleanup issue here: https://linear.app/ghost/issue/NY-1204/clean-up-post-title-alignment-and-title-color-attributes
cmraible
commented
Apr 2, 2026
Comment on lines
+57
to
+60
| const PREVIEW_ONLY_FIELDS = new Set([ | ||
| 'post_title_color', | ||
| 'title_alignment' | ||
| ]); |
Collaborator
Author
There was a problem hiding this comment.
Follow up issue to clean up these unused fields here: https://linear.app/ghost/issue/NY-1204/clean-up-post-title-alignment-and-title-color-attributes
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/admin-x-settings/src/components/settings/email-design/email-design-modal.tsx (1)
91-98: Consider extracting the green button variant logic.The inline conditional for green color styling works but is slightly fragile if the design system changes. If this pattern is used elsewhere, consider creating a helper or extending the Button component's variants.
// Alternative: Extract to a variable for clarity const buttonVariant = okProps?.color === 'red' ? 'destructive' : 'default'; const greenOverride = okProps?.color === 'green' ? 'bg-green text-white hover:bg-green/90' : undefined;This is minor and can be deferred.
🤖 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/email-design/email-design-modal.tsx` around lines 91 - 98, Extract the inline green styling and variant logic for the Save Button in email-design-modal.tsx by creating a small helper (or local constants) that computes the Button props from okProps: determine the variant value (use okProps?.color === 'red' ? 'destructive' : 'default') and compute a greenOverride className when okProps?.color === 'green'; then pass those computed values into the Button component (replace the inline className and variant conditionals on Button with the new variables) so the logic is clearer and reusable for Button/okProps handling in the component.
🤖 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/email-design/email-design-modal.tsx`:
- Around line 91-98: Extract the inline green styling and variant logic for the
Save Button in email-design-modal.tsx by creating a small helper (or local
constants) that computes the Button props from okProps: determine the variant
value (use okProps?.color === 'red' ? 'destructive' : 'default') and compute a
greenOverride className when okProps?.color === 'green'; then pass those
computed values into the Button component (replace the inline className and
variant conditionals on Button with the new variables) so the logic is clearer
and reusable for Button/okProps handling in the component.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 844e9cf0-ca25-4e03-bccd-5f5735469939
📒 Files selected for processing (1)
apps/admin-x-settings/src/components/settings/email-design/email-design-modal.tsx
cmraible
added
the
ok to merge for me
addresses minor cleanup reported in sonar here https://sonarcloud.io/project/issues?sinceLeakPeriod=true&issueStatuses=OPEN%2CCONFIRMED&pullRequest=27070&id=TryGhost_Ghost&open=AZ1Lx1wAa9eWpxzqwdIj
|
troyciesco
approved these changes
Apr 2, 2026
cmraible
deleted the
chris-ny-1165-wiring-wire-up-the-customization-modal-to-the-api-for
branch
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-1165/wiring-wire-up-the-customization-modal-to-the-api-for-persistence
Summary
automated_emails/design/API so settings persistwindow.confirmwith anAlertDialogfor unsaved changeslight,transparent,accent) to hex for the color pickerI'd recommend reviewing this one commit at a time.