Refactored email editor into a standalone component#26796
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
Walkthrough
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
| if (stringified !== value) { | ||
| onChange(stringified); | ||
| } | ||
| onChange(JSON.stringify(data)); |
There was a problem hiding this comment.
I think this is fine, but why did it change?
| // Keep settings panel copy compact | ||
| '[&_[data-kg-settings-panel]_p]:!mb-0', | ||
| // Nested-editor (callout, etc.) fixes: align placeholder with text | ||
| // 1. Override placeholder font/size/line-height to match the <p> styles |
There was a problem hiding this comment.
nit: why are these comments removed?
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/admin-x-settings/src/components/settings/membership/member-emails/member-email-editor.tsx (1)
1-2: Avoid adding newadmin-x-design-systemUI here.This refactor introduces
LoadingIndicatorfrom@tryghost/admin-x-design-systeminto anapps/admin-x-settingscomponent. Please keep the new fallback inshadeor a local primitive so this change does not expand usage of the phased-out package.As per coding guidelines, "Use
shadedesign system for new components in Admin UI. Avoidadmin-x-design-systemas it is being phased out."Also applies to: 143-145
🤖 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/member-email-editor.tsx` around lines 1 - 2, The code imports and uses LoadingIndicator from `@tryghost/admin-x-design-system` (symbols: LoadingIndicator, useDesignSystem) which must be avoided; replace any usage of LoadingIndicator used as the Suspense fallback (and any other UI from admin-x-design-system introduced here) with a shade design-system primitive or a local lightweight spinner component (e.g., a Shade Spinner or a local LoadingFallback) and remove the LoadingIndicator import; ensure ErrorBoundary and Koenig-related imports remain unchanged but update the Suspense fallback to reference the new shade/local component instead of LoadingIndicator.
🤖 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/member-email-editor.tsx`:
- Around line 95-96: The component currently captures the incoming prop via
const initialEditorState = useRef(value) (and similarly at line ~150), which
makes the editor ignore later changes to value/formState.lexical; either rename
the prop to initialValue to signal it’s intentionally one-time, or keep the prop
name and update the editor when value changes by replacing the useRef pattern
with state/effect logic: use a state (or ref-backed setter) to hold the editor
value and add a useEffect that watches value (and/or formState.lexical) and
reinitializes or updates the editor state when value changes; update references
to initialEditorState and ensure the key-based remounting
(key={automatedEmail?.id || 'new'}) remains consistent with this behavior.
---
Nitpick comments:
In
`@apps/admin-x-settings/src/components/settings/membership/member-emails/member-email-editor.tsx`:
- Around line 1-2: The code imports and uses LoadingIndicator from
`@tryghost/admin-x-design-system` (symbols: LoadingIndicator, useDesignSystem)
which must be avoided; replace any usage of LoadingIndicator used as the
Suspense fallback (and any other UI from admin-x-design-system introduced here)
with a shade design-system primitive or a local lightweight spinner component
(e.g., a Shade Spinner or a local LoadingFallback) and remove the
LoadingIndicator import; ensure ErrorBoundary and Koenig-related imports remain
unchanged but update the Suspense fallback to reference the new shade/local
component instead of LoadingIndicator.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: acf7b332-119b-4a7e-b792-3ee0040df499
📒 Files selected for processing (4)
apps/admin-x-design-system/src/global/form/koenig-editor-base.tsxapps/admin-x-design-system/src/index.tsapps/admin-x-settings/src/components/settings/membership/member-emails/member-email-editor.tsxapps/admin-x-settings/src/components/settings/membership/member-emails/welcome-email-modal.tsx
| // Capture the initial value once — the editor owns its own state after mount | ||
| const initialEditorState = useRef(value); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether current callers treat MemberEmailEditor as a controlled component.
# Expected result: if callers pass a mutable `value` without forcing a remount,
# the current implementation can go stale.
rg -n -C4 '<MemberEmailEditor\b|value=\{|key=\{' --type tsxRepository: TryGhost/Ghost
Length of output: 85
🏁 Script executed:
#!/bin/bash
# Fix: use correct file type flag for rg
# Search for MemberEmailEditor usages to verify if it's treated as a controlled component
echo "=== Searching for MemberEmailEditor component usages ==="
rg 'MemberEmailEditor' -t ts -C 3
echo ""
echo "=== Checking for value prop assignments ==="
rg 'value\s*=\s*\{|value\s*=' -t ts -C 2 | grep -A 5 -B 5 MemberEmailEditor
echo ""
echo "=== Checking design system imports in the target file ==="
head -20 apps/admin-x-settings/src/components/settings/membership/member-emails/member-email-editor.tsxRepository: TryGhost/Ghost
Length of output: 4255
🏁 Script executed:
#!/bin/bash
# Get actual value prop usage in the caller
echo "=== Value prop usage in welcome-email-modal.tsx ==="
rg -n 'value=|value\s*:\s*' apps/admin-x-settings/src/components/settings/membership/member-emails/welcome-email-modal.tsx -A 2 -B 2
echo ""
echo "=== Full context around MemberEmailEditor instantiation ==="
rg -n '<MemberEmailEditor' apps/admin-x-settings/src/components/settings/membership/member-emails/welcome-email-modal.tsx -A 10 -B 2
echo ""
echo "=== LoadingIndicator usage in member-email-editor.tsx ==="
rg -n 'LoadingIndicator' apps/admin-x-settings/src/components/settings/membership/member-emails/member-email-editor.tsx -B 2 -A 2
echo ""
echo "=== Lines 95-96 and surrounding context in member-email-editor.tsx ==="
sed -n '90,155p' apps/admin-x-settings/src/components/settings/membership/member-emails/member-email-editor.tsx
echo ""
echo "=== Check for other MemberEmailEditor callers ==="
rg 'MemberEmailEditor' -t ts --type tsx 2>/dev/null | grep -v 'welcome-email-modal\|member-email-editor\.tsx:import\|export' | head -20Repository: TryGhost/Ghost
Length of output: 6351
value is now only an initial snapshot; caller mutations won't update the editor.
These lines capture the prop value once via useRef, ignoring any subsequent value changes from the caller. While the key-based remounting (key={automatedEmail?.id || 'new'}) resets the component when the email ID changes, it leaves the editor stale if formState.lexical mutates without an ID change. The prop interface suggests a controlled component, but the implementation is uncontrolled for value updates.
Rename this parameter to initialValue or subscribe to value changes and reinitialize the editor when needed.
Also applies to: 150
🤖 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/member-email-editor.tsx`
around lines 95 - 96, The component currently captures the incoming prop via
const initialEditorState = useRef(value) (and similarly at line ~150), which
makes the editor ignore later changes to value/formState.lexical; either rename
the prop to initialValue to signal it’s intentionally one-time, or keep the prop
name and update the editor when value changes by replacing the useRef pattern
with state/effect logic: use a state (or ref-backed setter) to hold the editor
value and add a useEffect that watches value (and/or formState.lexical) and
reinitializes or updates the editor state when value changes; update references
to initialEditorState and ensure the key-based remounting
(key={automatedEmail?.id || 'new'}) remains consistent with this behavior.
E2E Tests FailedTo view the Playwright test report locally, run: REPORT_DIR=$(mktemp -d) && gh run download 23004296265 -n playwright-report -D "$REPORT_DIR" && npx playwright show-report "$REPORT_DIR" |
🤖 Velo CI Failure AnalysisClassification: 🟠 SOFT FAIL
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/admin-x-settings/src/components/settings/membership/member-emails/member-email-editor.tsx (1)
1-2: Consider migrating away fromadmin-x-design-systemimports.This file imports
ErrorBoundary,LoadingIndicator,loadKoenig, anduseDesignSystemfrom@tryghost/admin-x-design-system. The coding guidelines indicate this package is being phased out in favor of@tryghost/shade. While this may be acceptable for now given these utilities may not yet haveshadeequivalents, consider tracking migration for future work.🤖 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/member-email-editor.tsx` around lines 1 - 2, This import block currently pulls ErrorBoundary, LoadingIndicator, loadKoenig, and useDesignSystem from `@tryghost/admin-x-design-system`; update the module usage by replacing these imports with equivalents from `@tryghost/shade` where available (or create a TODO comment to track migration if equivalents don't yet exist) — specifically check for and switch ErrorBoundary, LoadingIndicator, loadKoenig, and useDesignSystem usages in MemberEmailEditor (member-email-editor.tsx) to their `@tryghost/shade` counterparts, or add a clear TODO and centralized issue reference in the file to track migrating these symbols off admin-x-design-system.
🤖 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/member-email-editor.tsx`:
- Around line 1-2: This import block currently pulls ErrorBoundary,
LoadingIndicator, loadKoenig, and useDesignSystem from
`@tryghost/admin-x-design-system`; update the module usage by replacing these
imports with equivalents from `@tryghost/shade` where available (or create a TODO
comment to track migration if equivalents don't yet exist) — specifically check
for and switch ErrorBoundary, LoadingIndicator, loadKoenig, and useDesignSystem
usages in MemberEmailEditor (member-email-editor.tsx) to their `@tryghost/shade`
counterparts, or add a clear TODO and centralized issue reference in the file to
track migrating these symbols off admin-x-design-system.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6914e45a-1771-4ac3-a9b2-ca8545c669e6
📒 Files selected for processing (2)
apps/admin-x-settings/src/components/settings/membership/member-emails/member-email-editor.tsxapps/admin-x-settings/src/components/settings/membership/member-emails/welcome-email-modal.tsx
E2E Tests FailedTo view the Playwright test report locally, run: REPORT_DIR=$(mktemp -d) && gh run download 23006020400 -n playwright-report -D "$REPORT_DIR" && npx playwright show-report "$REPORT_DIR" |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #26796 +/- ##
==========================================
- Coverage 73.19% 73.19% -0.01%
==========================================
Files 1534 1534
Lines 121048 121065 +17
Branches 14636 14643 +7
==========================================
+ Hits 88601 88612 +11
- Misses 31415 31421 +6
Partials 1032 1032
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
🤖 Velo CI Failure AnalysisClassification: 🔴 HARD FAIL
|
3 participants
ref TryGhost/Koenig#1764
ref https://linear.app/ghost/issue/NY-1130/