Conversation
|
Nice, it'd be pretty great to have! What's remaining to merge it? |
|
The MJML lib version that I tested this branch was broken, but it's improved since then. That dependency has to be updated and the implementation has to be tested fully. If you're able to do it, that would be great! |
|
I've updated the Someone has to test this out and help before I can merge it :) |
|
@knadh feel free to bump up to v0.11.0 : https://github.com/preslavrachev/gomjml/releases/tag/v0.11.0 |
|
@knadh I forked the entire repo, and will issue a separate PR to merge my |
b984dfd to
35d784c
Compare
|
@preslavrachev I've addressed this and force pushed to the
|
|
I've pulled down the There's an error message if you select "MJML" on the first page of the Campaign wizard. "Error compiling campaign body: error compiling MJML: failed to parse MJML: EOF". At that point, the campaign body is blank (of course). It should tolerate an empty body until I get to the second tab. |
|
@dfabulich amended the branch with a fix. Verified campaign creation and type conversion. |
There was a problem hiding this comment.
Issues Found
Critical (P0/P1)
- [P1] Apply template-func normalization to MJML body before mjml.Render (
models/campaigns.go:171-188)- In
CompileTemplate()the MJML path substitutesc.Bodyinto the base and runsmjml.Render()before the campaign body ever goes throughregTplFuncsrewriting (and thecontentsub-template is later set to""). This means shorthand template helper calls inside the campaign body (eg{{ TrackLink "..." }},{{ TrackView }}etc.) won't be rewritten to include dot context and can fail at render/send time even though non-MJML bodies get this normalization.
- In
- [P1] Fix content conversion paths involving MJML (returns non-MJML / can't handle partials) (
models/campaigns.go:272-297)ConvertContent()treatsmarkdown -> mjmlasmarkdown -> HTML(returning plain HTML but labeling it MJML), which will typically fail later when compiling/saving a MJML campaign because the result isn't a valid MJML document/fragment. Additionallymjml -> html|richtextrenders onlyc.Body, so it will error for the common case where the campaign body is a partial MJML fragment intended to be wrapped by a selected base MJML template (the frontend format-switch uses this endpoint).
Summary
Total issues: 2 critical, 0 important, 0 minor.
Overall Verdict
Status: Patch has blocking issues
Explanation: The PR adds MJML support, but the MJML compilation path skips existing template-helper normalization for the campaign body, and the new conversion routes involving MJML produce invalid output or fail for common template-wrapped MJML workflows. These issues are likely to break rendering/sending or editor format switching for real campaigns.
Review generated by Hodor (model: gpt-5.2)
Review Metrics — 35 turns, 34 tool calls, 4m 59s
- Tokens: in
20.9K| cached537.7K| out16.8K(total575.4K) - Cost:
$0.3652
|
In #3131 I've fixed the first P1 issue Hodor raised. Hodor's second P1 issue is quite confusing, but I don't think it's a real issue.
This is actually two issues:
I think the second issue is simply incorrect; I tested partial MJML with the provided "Sample MJML template" and it worked just fine, setting the campaign MJML to The gotcha is that embedding HTML or partial MJML inside an MJML template requires you to know/manage the current context element. The provided "Sample MJML template" embeds the email content in an <mjml>
<mj-body background-color="#F0F1F3">
<!-- Main Content Wrapper -->
<mj-section background-color="#fff" border-radius="5px" padding="30px">
<mj-column>
{{ template "content" . }}
</mj-column>
</mj-section>
</mj-body>
</mjml>If your partial MJML is just text like But Listmonk has no way of knowing how users will construct their templates. If, instead, the template had included an <mjml>
<mj-body background-color="#F0F1F3">
<!-- Main Content Wrapper -->
<mj-section background-color="#fff" border-radius="5px" padding="30px">
<mj-column>
<mj-text>
{{ template "content" . }}
</mj-text>
</mj-column>
</mj-section>
</mj-body>
</mjml>Arbitrary HTML renders fine inside an But a template like that would also be very confining. If you wanted to add more sections/columns to your MJML campaign, you'd first have to close out the tags that the template provided, and then reopen tags for the template to close, like this: Overall, it's not at all clear to me what a Listmonk "Sample MJML Template" should provide, but I suspect that it should either provide its own If the template only provides an Regardless, I don't think this is a P1 issue. With my PR #3131 merged, I think MJML support is ready to ship. |
There was a problem hiding this comment.
Issues Found
Critical (P0/P1)
- [P1] Validate subject/headers templates again in validateCampaignFields (
cmd/campaigns.go:720-737)validateCampaignFields()now compiles a freshly-constructedmodels.Campaigncontaining onlyBody/ContentType/TemplateBody, so template expressions inSubject,AltBody, and header values are no longer validated on create/update and will only fail later during send/preview. This is a regression from the previousc.CompileTemplate(...)call and can lead to campaigns being saved successfully but erroring at runtime when compiling templates.
- [P1] Fix TrackLink shorthand causing invalid MJML before mjml.Render (
models/campaigns.go:171-186)- MJML compilation runs
regTplFuncsreplacements and then callsmjml.Render()on the pre-template source; the@TrackLinkrewrite produces{{ TrackLink "..." . }}which commonly sits inside an already-quoted attribute (e.g.href="https://x@TrackLink"→href="{{ TrackLink "https://x" . }}"), yielding nested unescaped quotes that MJML/HTML parsers typically can't parse, so otherwise-valid MJML campaigns with tracked links can fail to compile.
- MJML compilation runs
Important (P2)
- [P2] Avoid treating MJML as HTML during format conversion in Editor.vue (
frontend/src/components/Editor.vue:160-232)- The editor now includes
'mjml'in the "HTML-like" branch (div.innerHTML+beautifyHTML), which can mutate MJML before sending it to the backend for MJML→HTML conversion; additionally the plain→mjml conversion path only inserts<br>tags (HTML fragment) while switching the type to MJML, which will typically produce invalid MJML and later compilation errors when saving/sending.
- The editor now includes
Summary
Total issues: 2 critical, 1 important, 0 minor.
Overall Verdict
Status: Patch has blocking issues
Explanation: The PR introduces MJML support but has regressions in campaign validation (subject/headers no longer template-validated) and likely MJML compilation failures for common tracked-link patterns due to quote rewriting prior to MJML parsing. The frontend also performs lossy/incorrect conversions into MJML that can lead to invalid saved content.
Review generated by Hodor (model: gpt-5.2)
Review Metrics — 19 turns, 23 tool calls, 6m 20s
- Tokens: in
25.8K| cached390.0K| out18.7K(total434.6K) - Cost:
$0.3752
No description provided.