Outlook-safe rendering for visual email builder output#2978
Outlook-safe rendering for visual email builder output#2978MrJoky wants to merge 9 commits intoknadh:masterfrom
Conversation
Keep the last visual builder source/body snapshot when switching between visual and raw formats so unchanged visual content can round-trip without losing button styling or other builder-specific structure.
Move the visual email post-processing into a dedicated outlook module, add Outlook-safe wrappers for buttons, images, text blocks, and spacers, and add a local ESLint config for the email-builder TypeScript subproject so its TS/TSX files are parsed with the correct parser.
|
Thanks @MrJoky. This is a big PR, which is going to be very difficult for me to test and verify. Would you be able to share screenshots+HTML of e-mails post-processed via this rendering in Outlook and non-Outlook clients? |
|
@knadh how far are you with the review? |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Issues Found
Important (P2)
- [P2] Avoid invalid
width="auto"on presentation tables (frontend/email-builder/src/outlook.ts:231-238)transformImageBlocks()builds an inner table withwidth="auto", but the HTMLwidthattribute for<table>is only defined for numeric pixel/percentage values and some email clients (notably Outlook/Word) handle invalid values unpredictably (often collapsing or ignoring sizing), which can cause wrapped images to render at the wrong size or disappear. Consider omitting thewidthattribute entirely for the auto-sized case (or expressing it via CSS) instead of emittingwidth="auto".
- [P2] Add XML namespace for
o:OfficeDocumentSettings snippet (frontend/email-builder/src/utils.tsx:5-6)MSO_DOCUMENT_SETTINGSinjects<o:OfficeDocumentSettings>inside an<xml>block but does not declare theonamespace (commonlyxmlns:o="urn:schemas-microsoft-com:office:office"). In Outlook this block is used to control DPI/PNG handling; without a valid namespace the XML can be considered invalid and the settings may be ignored, undermining the intended Outlook rendering fix.
Minor (P3)
- [P3] Prevent
<head>injection regex from matching<header>(frontend/email-builder/src/utils.tsx:8-14)renderHtmlWithMeta()uses/<head([^>]*)>/ito inject meta/settings; ifpostProcessForOutlook()returns the original markup (e.g., whenDOMParseris unavailable in a non-browser context) and that markup lacks a real<head>tag, this regex can accidentally match<header>and inject meta tags into the body. A stricter pattern (eg, word boundary) and/or a fallback that creates a<head>element would avoid corrupting output in those environments.
Summary
Total issues: 0 critical, 2 important, 1 minor.
Overall Verdict
Status: Patch has blocking issues
Explanation: The PR’s Outlook post-processing is directionally useful, but the current diff contains a couple of HTML/email-client compatibility pitfalls (notably an invalid table width value and a likely-nonfunctional MSO document settings block) that can cause the generated emails to render incorrectly in some clients, including the target Outlook environment.
Review generated by Hodor (model: gpt-5.2)
Review Metrics — 14 turns, 18 tool calls, 6m 50s
- Tokens: in
25.8K| cached285.4K| out19.0K(total330.3K) - Cost:
$0.3610
There was a problem hiding this comment.
Issues Found
Critical (P0/P1)
- [P1] Don’t wrap button VML in
{{ Safe "..." }}when href/text has templates (frontend/email-builder/src/outlook.ts:324-335)buildBulletproofButton()wraps the Outlook-only VML and conditional-comment markers in a{{ Safe "..." }}template string, which turns the entire VML block into a literal string at Go-template render time; as a result, any nested template actions in the buttonhrefor text (for examplehref="{{ .SubscriberUUID }}",{{ TrackLink ... }}, or personalized label text) will not be evaluated for Outlook and will be sent as literal{{ ... }}inside the VMLhref/content, producing broken links/text specifically in Outlook while the non-MSO<a>still renders correctly.
Important (P2)
- [P2] Keep meta/head insertion working when
DOMParseris unavailable (frontend/email-builder/src/utils.tsx:8-14)renderHtmlWithMeta()now injects the viewport/MSO meta by replacing the opening<head>tag, butpostProcessForOutlook()explicitly returns the original markup whenDOMParseris undefined; in that case,renderToStaticMarkup()output may not contain a<head>(the previous implementation inserted one after<html>), so the regex replace becomes a no-op and the viewport/MSO settings silently disappear in non-DOM environments (eg, unit tests/SSR/CLI rendering).
Summary
Total issues: 1 critical, 1 important, 0 minor.
Overall Verdict
Status: Patch has blocking issues
Explanation: The Outlook post-processing introduces a functional regression for campaigns that use template variables/functions inside button URLs or labels: Outlook will receive unevaluated template actions inside the VML block. There is also a smaller regression where meta injection no longer works in environments without DOMParser despite the code explicitly supporting that path.
Review generated by Hodor (model: gpt-5.2)
Review Metrics — 19 turns, 21 tool calls, 4m 47s
- Tokens: in
23.5K| cached311.2K| out13.5K(total348.2K) - Cost:
$0.2849
There was a problem hiding this comment.
Issues Found
Critical (P0/P1)
- [P1] Prevent newlines inside generated
{{ Safe "..." }}templates (frontend/email-builder/src/outlook.ts:137-145)makeSafeTemplate()wraps raw Outlook conditional-comment/VML markup in a double-quoted Go-template string ({{ Safe "..." }}) but only escapes backslashes and quotes. If the campaign HTML is later reformatted in the UI (eg switching visual → raw/html triggers the existing beautifier that inserts line breaks around<...>substrings), that introduces literal newlines inside the quoted string, which Go templates generally fail to parse, breaking preview/sending for those campaigns. Use a raw (backtick) template literal, or escape\r?\nto\\n, or avoid embedding tag-like content inside a quoted template string.
Summary
Total issues: 1 critical, 0 important, 0 minor.
Overall Verdict
Status: Patch has blocking issues
Explanation: The Outlook post-processing introduces Go-template Safe wrappers around large HTML snippets, and the current escaping strategy is fragile: common editor workflows that beautify/rewrap HTML can introduce newlines that make the template invalid at render time. This can cause campaign preview/sending failures for visual-builder campaigns containing buttons.
Review generated by Hodor (model: gpt-5.2)
Review Metrics — 13 turns, 15 tool calls, 5m 55s
- Tokens: in
24.9K| cached208.9K| out17.0K(total250.8K) - Cost:
$0.3180
MrJoky
force-pushed
the
outlook-type-safe
branch
from
248e6c7 to
13fa55a
Compare
There was a problem hiding this comment.
Issues Found
Critical (P0/P1)
- [P1] Avoid embedding
<!--...-->inside{{ Safe "..." }}strings (frontend/email-builder/src/outlook.ts:331-339)buildBulletproofButton()inserts conditional-comment markers viamakeSafeTemplate('<!--[if mso]>')etc. BecausepostProcessForOutlook()builds a DOM withDOMParser/createContextualFragment, the raw<!--inside those template string literals will be interpreted as an actual HTML comment start before the Go template runs, causing the parser to swallow/reshape the surrounding{{ Safe ... }}and the VML block until the next-->. The serialized output can end up with malformed conditional comments (or VML commented out), so Outlook buttons may render incorrectly or disappear. This affects any button that hits the non-fullWidthbranch.
Summary
Total issues: 1 critical, 0 important, 0 minor.
Overall Verdict
Status: Patch has blocking issues
Explanation: The Outlook post-processor introduces a parsing/serialization issue for bulletproof buttons: the generated {{ Safe "<!--..." }} markers are not stable when the HTML is parsed as DOM, leading to malformed conditional-comment blocks in the final HTML. Other parts look reasonable, but this breaks a core feature of the PR under common button output.
Review generated by Hodor (model: gpt-5.2)
Review Metrics — 24 turns, 26 tool calls, 7m 7s
- Tokens: in
23.7K| cached491.3K| out22.4K(total537.4K) - Cost:
$0.4411
MrJoky
force-pushed
the
outlook-type-safe
branch
from
13fa55a to
d9ffd9a
Compare
There was a problem hiding this comment.
Issues Found
Critical (P0/P1)
- [P0] Fix malformed HTML injection in injectHeadContents() (
frontend/email-builder/src/utils.tsx:8-17)injectHeadContents()replaces the opening<head>/<html>tag with a string that omits the closing>(eg it produces<head...${contents}and<html...<head>). SincerenderHtmlWithMeta()always calls this, the visual builder output becomes invalid HTML and can break rendering/parsing in clients and downstream conversions.
- [P1] Reset visualSnapshot when the bound campaign content changes* (
frontend/src/components/Editor.vue:125-176)visualSnapshotBody/visualSnapshotSourceare stored as component state and used to decide whether to reusebodySource, but they’re only initialized indata()/mounted()and updated on local visual edits. If the parent replaces thevalueprop with a different campaign/content while reusing this Editor instance, the snapshot can refer to the previous campaign and incorrectly restore an unrelatedbodySourcewhen switching tovisual(data corruption risk if bodies match).
Summary
Total issues: 2 critical, 0 important, 0 minor.
Overall Verdict
Status: Patch has blocking issues
Explanation: The head injection helper currently generates malformed markup, which is likely to break the visual builder’s generated HTML everywhere it’s used. Additionally, the new snapshot state for visual format switching can persist across prop changes and restore the wrong visual source under component reuse.
Review generated by Hodor (model: gpt-5.2)
Review Metrics — 47 turns, 49 tool calls, 8m 52s
- Tokens: in
31.8K| cached1.34M| out23.5K(total1.40M) - Cost:
$0.6204
MrJoky
force-pushed
the
outlook-type-safe
branch
from
d9ffd9a to
b3ff72d
Compare
There was a problem hiding this comment.
Issues Found
Critical (P0/P1)
- [P1] Force full-width button styles instead of only appending (
frontend/email-builder/src/outlook.ts:316-321)buildBulletproofButton()treats a button as “full width” whenwidth:100%is present, but then usesappendMissingStyles()to adddisplay:block/text-align:center/border:.... If the anchor already has these properties set to different values (eg.display:inline-block), they won’t be overridden, so the “full width” rendering can silently fail and produce a non-stretched/incorrectly aligned button in Outlook and other clients. This should set/override the required properties (eg. viasetStyleValues) whenfullWidthis true.
Summary
Total issues: 1 critical, 0 important, 0 minor.
Overall Verdict
Status: Patch has blocking issues
Explanation: The Outlook post-processing introduces a logic error in the full-width button path where required CSS is only appended if missing, not enforced. This can cause real-world visual builder buttons styled with width:100% but an existing display value to render incorrectly.
Review generated by Hodor (model: gpt-5.2)
Review Metrics — 20 turns, 22 tool calls, 5m 52s
- Tokens: in
26.1K| cached403.2K| out16.9K(total446.2K) - Cost:
$0.3524
MrJoky
force-pushed
the
outlook-type-safe
branch
from
b3ff72d to
2855cd8
Compare
There was a problem hiding this comment.
Issues Found
Critical (P0/P1)
- [P1] Avoid rewriting all tables in visual-builder HTML output (
frontend/email-builder/src/outlook.ts:174-187)addTableDefaults()appliesrole="presentation"and forcescellpadding/cellspacing/borderdefaults on every<table>in the generated HTML. If a user uses the visual builder’s HTML block (or any block that emits semantic tables) and relies on browser/email-client defaults (or CSS) rather than explicit attributes, this post-processing will silently change rendering in all clients (not just Outlook) by collapsing spacing/borders. Consider scoping this to builder-layout tables only (eg. those already markedrole=presentation/ known wrappers) or skipping tables inside user-provided HTML blocks.
- [P1] Don’t force all images to display:block (breaks inline icons) (
frontend/email-builder/src/outlook.ts:200-217)hardenImages()sets every<img>todisplay:block(and removesvertical-align). This can break legitimate inline images (eg. small icons inside a text run, social icon rows, etc.), especially when users include custom HTML via the HTML block, causing unexpected line breaks/spacing across non-Outlook clients. If the intent is to harden builder-emitted image blocks only, scope the selector accordingly or preserve inline images when they are inside text/inline contexts.
Important (P2)
- [P2] Post-processing on every editor change may cause UI lag (
frontend/email-builder/src/utils.tsx:22-25)renderHtmlWithMeta()now runs a full DOM parse + multiple querySelector passes (postProcessForOutlook) for every document update (it’s used in the builder’s onChange pipeline). For large emails, this can noticeably slow typing/dragging in the visual editor compared to the prior O(1) string replace. If possible, defer Outlook post-processing to “export/save” time, debounce it, or make it opt-in for the HTML preview panel only.
Summary
Total issues: 2 critical, 1 important, 0 minor.
Overall Verdict
Status: Patch has blocking issues
Explanation: The Outlook post-processor is applied globally and can change the semantics/layout of user-authored tables and inline images (eg. via HTML blocks) in all clients, not just Outlook. Additionally, running a full DOM transformation on every visual editor update is likely to introduce noticeable performance regressions for larger templates.
Review generated by Hodor (model: gpt-5.2)
Review Metrics — 24 turns, 31 tool calls, 6m 18s
- Tokens: in
30.1K| cached531.2K| out18.6K(total579.8K) - Cost:
$0.4054
MrJoky
force-pushed
the
outlook-type-safe
branch
from
2855cd8 to
87d9f34
Compare
There was a problem hiding this comment.
Issues Found
Critical (P0/P1)
- [P1] Ensure Outlook post-processing is applied to saved builder output (
frontend/email-builder/src/App/TemplatePanel/HtmlPanel.tsx:8-11)HtmlPanelnow renders HTML withoutlook: true, but the actual HTML emitted to the campaign editor is still produced viarenderHtmlWithMeta(document, { rootBlockId: 'root' })(seeemail-builder/src/App/index.tsx), so users will see/copy an Outlook-post-processed version while the content that gets saved/sent remains unprocessed. This mismatch was introduced by the new opt-in flag and will likely make the PR’s “Outlook-safe rendering” ineffective in production unless the same option is used on the main output path (or the default behavior is adjusted).
Summary
Total issues: 1 critical, 0 important, 0 minor.
Overall Verdict
Status: Patch has blocking issues
Explanation: The PR adds Outlook-specific HTML post-processing, but it is only enabled in the HTML preview panel, not on the primary HTML output that is persisted/used by the campaign editor. As a result, the intended production behavior (Outlook-safe output) is not achieved and the UI can display HTML that differs from what is actually saved/sent.
Review generated by Hodor (model: gpt-5.2)
Review Metrics — 12 turns, 15 tool calls, 5m
- Tokens: in
16.2K| cached194.0K| out16.0K(total226.2K) - Cost:
$0.2860
MrJoky
force-pushed
the
outlook-type-safe
branch
from
87d9f34 to
595a6e1
Compare
There was a problem hiding this comment.
Issues Found
Critical (P0/P1)
- [P1] Preserve MSO conditional head block when post-processing visual HTML (
frontend/src/components/VisualEditor.vue:38-48)VisualEditor.vuerunspostProcessForOutlook(body)on the HTML coming from the email-builder, but that HTML already hasMSO_DOCUMENT_SETTINGSinjected in<head>(viarenderHtmlWithMeta). SincepostProcessForOutlook()parses withDOMParser, Outlook conditional comment blocks like<!--[if mso]>…<![endif]-->are typically consumed/rewritten as plain comments and the enclosed XML gets dropped, so the final emitted campaign HTML can lose<o:OfficeDocumentSettings>(and any other user-supplied MSO conditionals), reducing Outlook rendering compatibility. Consider re-injecting the MSO head block after post-processing, or moving the post-processing earlier (eg. generate the outlook-safe HTML directly in the email-builder before adding conditional comments).
Summary
Total issues: 1 critical, 0 important, 0 minor.
Overall Verdict
Status: Patch has blocking issues
Explanation: The PR’s Outlook post-processing is likely to strip or rewrite existing MSO conditional-comment blocks when it re-parses the generated HTML, which can remove the very Outlook-specific head settings the PR adds. This undermines Outlook compatibility for visual-editor output in a reproducible flow.
Review generated by Hodor (model: gpt-5.2)
Review Metrics — 20 turns, 25 tool calls, 4m 4s
- Tokens: in
22.7K| cached366.2K| out15.7K(total404.6K) - Cost:
$0.3232
MrJoky
force-pushed
the
outlook-type-safe
branch
from
595a6e1 to
098fc8a
Compare
There was a problem hiding this comment.
Issues Found
Critical (P0/P1)
- [P1] Preserve Go template quote fix after Outlook re-rendering (
frontend/src/components/VisualEditor.vue:37-50)VisualEditor.vuestill computestplto replace"inside{{ ... }}blocks, butemitVisualChange()then re-renders HTML viaEmailBuilder.renderHtmlWithMeta(..., outlook: true)and emits that result, discarding thetplfix. If a visual email contains Go template expressions with quoted strings (eg{{ if eq .Foo "bar" }}), the emitted body can end up with"again and break template evaluation at send/preview time; apply the same replacement toprocessedBody(or move this fix intorenderHtmlWithMeta).
Summary
Total issues: 1 critical, 0 important, 0 minor.
Overall Verdict
Status: Patch has blocking issues
Explanation: The Outlook post-processing integration in the visual editor bypasses an existing HTML fix for Go template expressions, which can lead to broken templates when visual content contains quoted strings. This is a functional regression for a subset of visual campaigns.
Review generated by Hodor (model: gpt-5.2)
Review Metrics — 18 turns, 17 tool calls, 3m 35s
- Tokens: in
22.7K| cached298.5K| out12.2K(total333.3K) - Cost:
$0.2621
MrJoky
force-pushed
the
outlook-type-safe
branch
from
098fc8a to
7594d8e
Compare
There was a problem hiding this comment.
Issues Found
Critical (P0/P1)
- [P1] Fix malformed tag injection when no exists (
frontend/email-builder/src/utils.tsx:13-18)injectHeadContents()replaces the opening<html ...>tag with<html$1><head>...but misses the closing>after the<html...portion, producing invalid markup like<html<head>...(or<html lang="en"<head>...). This path is taken whenever the renderer output has an<html>but no<head>(which is the common case in this builder), sorenderHtmlWithMeta(document, {rootBlockId: 'root'})can emit broken HTML and downstream clients/fallback paths will persist or display malformed content.
Summary
Total issues: 1 critical, 0 important, 0 minor.
Overall Verdict
Status: Patch has blocking issues
Explanation: The new head-injection helper can generate malformed HTML (<html<head>) when the input lacks a <head> tag, which is likely for the visual builder output when outlook is not enabled. This can break rendered/saved email HTML in production paths that call renderHtmlWithMeta without the outlook option.
Review generated by Hodor (model: gpt-5.2)
Review Metrics — 13 turns, 18 tool calls, 2m 26s
- Tokens: in
17.1K| cached164.6K| out8.9K(total190.7K) - Cost:
$0.1838
False positive: current implementation already emits |
|
@knadh all issues from hodor are solved, except last one which is not correct. |
|
https://testi.at/proj/bjkrf423cz67fdldio Current cross-platform test |
|
Thanks @MrJoky. I do find it wrong to pollute all markup for all clients just for outlook compatibility (which likely is a tiny % of recipients) :) I've pushed a commit that makes it optional by adding a UI toggle to the global pane in the editor. Makes sense? 
|
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
AH! That's an awesome idea! I like it. |
2 participants
This PR improves Outlook compatibility for emails created with the visual email builder.
It adds Outlook-oriented post-processing to the visual builder HTML output so that buttons, images, text blocks, and
spacers render more reliably in Outlook. It also fixes format switching in the campaign editor so that switching
between
visualandraw/htmldoes not discard the visual builder source for unchanged content.References: #2576
Not sure if this issue addresses the same problem #1231