feat(emails): preview MJML via mailing-api render endpoint (preview-only)#959
Conversation
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Replaces the single html_content preview effect with two mode-guarded effects: MJML templates send raw mjml_content with isMjml=true so the mailing-api runs Jinja → official MJML CLI (production pipeline); HTML templates send html_content with isMjml=false. Updates debouncedRender- Template ref to accept and forward the isMjml flag. Also fixes pre-existing lint errors in the same file (import order, magic numbers, unused vars, no-unused-expressions, iframe title) that were blocking the pre-commit hook. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Add entity.id + entity.identifier to the mode-init effect deps so mjmlEditor recomputes when a different template loads into the same form instance without remounting, fixing stale MJML/HTML mode on in-place navigation. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…equest id Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
✅ Files skipped from review due to trivial changes (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughEmail template rendering now tags preview requests with IDs so stale render and validation results are ignored. The form sends MJML and HTML previews through separate paths, and the reducer resets render state when the template session changes. ChangesEmail template rendering with request sequencing
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/reducers/emails/email-template-reducer.js (1)
82-112:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReset render sequencing state when the edited template changes.
These branches keep
latestRenderId,templateLoading, andrender_errorsfrom the previous editor session. If template A has request5in flight and we navigate/reset before template B dispatches its firstREQUEST_TEMPLATE_RENDER, A's lateTEMPLATE_RENDER_RECEIVED/VALIDATE_RENDERwithrequestId: 5still matchesstate.latestRenderIdand can repopulate the new form with stale preview/error state.🩹 Suggested fix
case SET_CURRENT_SUMMIT: case RESET_TEMPLATE_FORM: - return { ...state, entity: { ...DEFAULT_ENTITY }, errors: {} }; + return { + ...state, + entity: { ...DEFAULT_ENTITY }, + errors: {}, + preview: null, + render_errors: [], + templateLoading: false, + latestRenderId: 0 + }; case RECEIVE_TEMPLATE: { const entity = normalizeEntityFields({ ...payload.response }); return { ...state, @@ ...entity, original_mjml_content: entity.mjml_content, original_html_content: entity.html_content }, - preview: null + preview: null, + render_errors: [], + templateLoading: false, + latestRenderId: 0 }; } case TEMPLATE_ADDED: case TEMPLATE_UPDATED: { const entity = normalizeEntityFields({ ...payload.response }); return { ...state, entity: { ...DEFAULT_ENTITY, ...entity, original_mjml_content: entity.mjml_content, original_html_content: entity.html_content - } + }, + render_errors: [], + templateLoading: false, + latestRenderId: 0 }; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/reducers/emails/email-template-reducer.js` around lines 82 - 112, The reducer currently preserves render sequencing state (latestRenderId, templateLoading, render_errors, preview) across template changes, allowing stale render responses to affect a new template; update the branches that set a new entity (RECEIVE_TEMPLATE, TEMPLATE_ADDED, TEMPLATE_UPDATED and the RESET_TEMPLATE_FORM/SET_CURRENT_SUMMIT branch) to also reset the render state by setting latestRenderId to null, templateLoading to false, render_errors to {} (or null) and preview to null so any in-flight responses for the previous template cannot match and overwrite the new editor state.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/reducers/emails/email-template-reducer.js`:
- Around line 82-112: The reducer currently preserves render sequencing state
(latestRenderId, templateLoading, render_errors, preview) across template
changes, allowing stale render responses to affect a new template; update the
branches that set a new entity (RECEIVE_TEMPLATE, TEMPLATE_ADDED,
TEMPLATE_UPDATED and the RESET_TEMPLATE_FORM/SET_CURRENT_SUMMIT branch) to also
reset the render state by setting latestRenderId to null, templateLoading to
false, render_errors to {} (or null) and preview to null so any in-flight
responses for the previous template cannot match and overwrite the new editor
state.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4b65681d-6cfc-4f81-bf4f-0dd3290e23a5
📒 Files selected for processing (6)
src/actions/__tests__/email-actions.test.jssrc/actions/email-actions.jssrc/components/forms/__tests__/email-template-form.test.jssrc/components/forms/email-template-form.jssrc/reducers/emails/__tests__/email-template-reducer.test.jssrc/reducers/emails/email-template-reducer.js
Closes the stale-render race flagged by CodeRabbit on PR #959: entity-change reducer branches (SET_CURRENT_SUMMIT/RESET_TEMPLATE_FORM, RECEIVE_TEMPLATE, TEMPLATE_ADDED/TEMPLATE_UPDATED) now clear preview, render_errors, templateLoading and latestRenderId so in-flight responses from a previous template can never repopulate the new editor's state. Adds 7 reducer tests: per-branch reset coverage, the cross-template race reproduction, and a characterization test of the navigation window. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
@coderabbitai Applied in 6ad4043. All three entity-change branch groups ( One deviation from the suggested AI-agent prompt: we kept the field values from your suggested diff ( Also added 7 reducer tests: per-branch reset coverage, a reproduction of the cross-template race (late success and late error responses ignored after |
|
Tip For best results, initiate chat on the files or code changes.
The navigation-window characterization test is a particularly nice addition: explicitly documenting that a late response arriving before 653/653 — 🐇 ✨ |
|
@caseylocker please lets fix merge conflicts and we are good to merge many thanks |
…w-via-api # Conflicts: # src/actions/__tests__/email-actions.test.js
|
@smarcet merge conflicts resolved and pushed ( The only conflict was an add/add in |
|
@smarcet CI is now green and GitHub reports the PR mergeable / CLEAN — all set on your end to merge whenever. Thanks! |
2 participants
ref: https://app.clickup.com/t/86b9x7m2n
Summary
Makes the email-template editor preview MJML through the mailing-api render endpoint instead of compiling MJML→HTML client-side and sending pre-compiled HTML. For an MJML template the raw
mjml_contentis now sent with the newmjmlrender param, so the preview uses the same Jinja → official MJML CLI pipeline as a real send. HTML templates are unchanged (htmlparam).Preview-only per ClickUp 86b9x7m2n (smarcet's approach): the client-side
mjml2htmlcompile, the savedhtml_content, the save/submit flow, and themjml-browserdependency are all intentionally kept.Backend dependency: mailing-api PR #9 (the
mjmlrender param) must be merged + deployed to the env this admin'sEMAIL_API_BASE_URLtargets.isMjml=false(the default) keeps HTML-mode preview working regardless.What changed
renderEmailTemplate(json, content, isMjml = false)builds the PUT body via a new pure helperbuildRenderPayload→{payload, mjml}for MJML,{payload, html}for HTML. Backward-compatible (sole caller's 2-arg call still sendshtml).renderErrorHandlervianormalizeRenderErrors: normalizes the API's error-body shapes (412 string-array, 500 bare string, object, network/no-response) to astring[], and guards a missingerr.response. This also fixes a latent crash where a non-array error body was rendered directly as a React child.mjmlEditor(MJML sendsmjml_content, HTML sendshtml_content); a shared 500ms debounce coalesces so exactly one request fires per mode.Correctness fixes from review (race/staleness)
entity.idsomjmlEditorrecomputes when a different template loads into the same (un-remounted) form instance. (Editableidentifierdeliberately excluded to avoid a rename+save resetting a manual mode toggle.)requestIdis threaded through the request/success/error actions and the reducer ignores any response whose id isn't the latest (latestRenderId), so a slow older request can't overwrite a newer preview. Guard is skipped when no id is present (backward-compatible).Testing
buildRenderPayload,normalizeRenderErrors(incl. multi-key/string-value bodies), mount-time mode dispatch, mode-toggle re-fire, in-place mode re-init, and a new reducer test file covering the out-of-order guard (incl. that a stale response preserves a prior fresh preview/error).Reviewer note
Two files (
email-template-form.js,email-template-reducer.js) carry incidental fixes to pre-existing eslint errors (magic numbers, nested ternary, lone-case-blocks,var-in-case, unused vars, jsx-a11y) that the huskyeslint --fixpre-commit gate blocks on. These are strictly behavior-preserving refactors of code this PR had to touch — the alternative was a--no-verifybypass of the lint gate.Not yet done
mjml, Jinja resolves inside MJML, preview matches a real send, malformed MJML surfaces an error, one render per mode-toggle).ClickUp: 86b9x7m2n
🤖 Generated with Claude Code
Summary by CodeRabbit
Summary by CodeRabbit
rel="noreferrer").