Conversation
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (2)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThe email finalization flow now uses a new 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
ghost/core/core/server/services/email-rendering/finalize.js (1)
9-28: Ordering is correct; consider a brief comment on why juice runs between the two cheerio passes.The sequence (add
kg-card-figcaption→ juice → renamefigure/figcaptiontodiv) is deliberate: juice must run while the tag names still match thefigcaption/figureCSS selectors before they becomedivs. A short inline comment near line 15 noting this ordering dependency would help future maintainers avoid accidentally reordering the steps.Also minor: reassigning
html(parameter) and reusing$across two parses is fine but slightly reduces readability; separate names (withClass,$2) could make the two-phase nature more explicit. Non-blocking.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ghost/core/core/server/services/email-rendering/finalize.js` around lines 9 - 28, In finalizeHtml, add a short inline comment right before the juice(html, ...) call explaining that juice must run while elements are still <figure>/<figcaption> so CSS selectors are applied before we convert those tags to <div> (i.e., "run juice before renaming tags because selectors target figure/figcaption"); optionally, for clarity, consider renaming the intermediate variables (e.g., keep the original parsed DOM/HTML as withClass or $1 and the post-juice parse as $2) to make the two-phase processing around cheerio.load and juice more explicit.ghost/core/test/unit/server/services/member-welcome-emails/member-welcome-email-renderer.test.js (1)
905-931: LGTM — good coverage of the figure→div rewrite and caption styling.Assertions correctly validate both the structural change (no
figure/figcaptionremain) and that juice inlined the styles onto the renamed element via thekg-card-figcaptionclass before the rename. One small suggestion: thefont-family: -apple-systemcheck (Line 930) is brittle against unrelated changes to the sans-serif stack in the shared styles — asserting a substring likefont-family:presence, or matching the entire expected stack, may be more robust. Non-blocking.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ghost/core/test/unit/server/services/member-welcome-emails/member-welcome-email-renderer.test.js` around lines 905 - 931, The assertion checking the caption font is brittle: update the assertion on $caption.attr('style') in the test for MemberWelcomeEmailRenderer to avoid hardcoding "-apple-system"; either assert that the style string includes "font-family:" (i.e. presence of a font-family declaration) or compare against the full expected font stack if you want exact matching; modify the check that currently uses $caption.attr('style').includes('font-family: -apple-system') to one of these more robust alternatives so the test no longer fails on unrelated changes to the sans-serif stack.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@ghost/core/core/server/services/email-rendering/finalize.js`:
- Around line 9-28: In finalizeHtml, add a short inline comment right before the
juice(html, ...) call explaining that juice must run while elements are still
<figure>/<figcaption> so CSS selectors are applied before we convert those tags
to <div> (i.e., "run juice before renaming tags because selectors target
figure/figcaption"); optionally, for clarity, consider renaming the intermediate
variables (e.g., keep the original parsed DOM/HTML as withClass or $1 and the
post-juice parse as $2) to make the two-phase processing around cheerio.load and
juice more explicit.
In
`@ghost/core/test/unit/server/services/member-welcome-emails/member-welcome-email-renderer.test.js`:
- Around line 905-931: The assertion checking the caption font is brittle:
update the assertion on $caption.attr('style') in the test for
MemberWelcomeEmailRenderer to avoid hardcoding "-apple-system"; either assert
that the style string includes "font-family:" (i.e. presence of a font-family
declaration) or compare against the full expected font stack if you want exact
matching; modify the check that currently uses
$caption.attr('style').includes('font-family: -apple-system') to one of these
more robust alternatives so the test no longer fails on unrelated changes to the
sans-serif stack.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 172f211e-3313-4f4c-a5ee-b8669e24556d
⛔ Files ignored due to path filters (1)
ghost/core/test/integration/services/__snapshots__/member-welcome-emails-snapshot.test.js.snapis excluded by!**/*.snap
📒 Files selected for processing (2)
ghost/core/core/server/services/email-rendering/finalize.jsghost/core/test/unit/server/services/member-welcome-emails/member-welcome-email-renderer.test.js
| el.tagName = 'div'; | ||
| }); | ||
|
|
||
| return $.html(); |
There was a problem hiding this comment.
Using Cheerio has the side-effect of changing the way HTML entities are encoded. For example, © becomes ©. These are equivalent. That's part of why I recently changed the tests to be resilient to this.
| // Add a class to each figcaption so we can style them in the email. | ||
| let $ = cheerio.load(html, null, false); | ||
| $('figcaption').addClass('kg-card-figcaption'); |
There was a problem hiding this comment.
Lifted from the newsletter renderer:
Ghost/ghost/core/core/server/services/email-service/email-renderer.js
Lines 515 to 517 in 74bdef0
| $('figure, figcaption').each((_, el) => { | ||
| el.tagName = 'div'; | ||
| }); |
There was a problem hiding this comment.
Lifted from the newsletter renderer:
Ghost/ghost/core/core/server/services/email-service/email-renderer.js
Lines 544 to 545 in 74bdef0
closes https://linear.app/ghost/issue/NY-1233 Welcome emails use `<figure>` and `<figcaption>` for images and their captions, which some email clients don't support. That causes the styling to be messed up in those clients. [Newsletter rendering had already fixed this.][0] This patch fixes it the same way. In the long term, we should unify these renderers. In the short term, let's fix this bug. [0]: https://github.com/TryGhost/Ghost/blob/d26bfdb0b20f029a82709782804164d621848d3f/ghost/core/core/server/services/email-service/email-renderer.js#L514-L544
There was a problem hiding this comment.
🧹 Nitpick comments (1)
ghost/core/test/unit/server/services/member-welcome-emails/member-welcome-email-renderer.test.js (1)
905-930: Assert the replacements are actually<div>elements.This test verifies
figure/figcaptionare removed, but not that they were converted todivs. That leaves the stated behavior under-tested.Strengthen the assertion
const $ = cheerio.load(result.html); + const $imageCard = $('.kg-image-card'); assert.equal($('figure').length, 0); assert.equal($('figcaption').length, 0); + assert.equal($imageCard[0]?.tagName, 'div'); - const $caption = $('.kg-image-card .kg-card-figcaption'); + const $caption = $imageCard.find('.kg-card-figcaption'); + assert.equal($caption[0]?.tagName, 'div'); assert.equal($caption.text(), 'A caption'); assert($caption.attr('style').includes('text-align: center'), 'caption should be centered'); assert($caption.attr('style').includes('font-size: 13px'), 'caption should have 13px font');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ghost/core/test/unit/server/services/member-welcome-emails/member-welcome-email-renderer.test.js` around lines 905 - 930, Add an assertion that the image card nodes are actual div elements: in the test "uses <div>s with inline styles for image cards" (which uses lexicalRenderStub and MemberWelcomeEmailRenderer.render to produce result.html), explicitly check that the element with class .kg-image-card is a div (e.g. assert($('div.kg-image-card').length > 0 or assert($('.kg-image-card').is('div'))), and similarly ensure the caption node selector .kg-card-figcaption is a div, so the test verifies replacement to divs rather than just removal of figure/figcaption.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@ghost/core/test/unit/server/services/member-welcome-emails/member-welcome-email-renderer.test.js`:
- Around line 905-930: Add an assertion that the image card nodes are actual div
elements: in the test "uses <div>s with inline styles for image cards" (which
uses lexicalRenderStub and MemberWelcomeEmailRenderer.render to produce
result.html), explicitly check that the element with class .kg-image-card is a
div (e.g. assert($('div.kg-image-card').length > 0 or
assert($('.kg-image-card').is('div'))), and similarly ensure the caption node
selector .kg-card-figcaption is a div, so the test verifies replacement to divs
rather than just removal of figure/figcaption.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1a481bca-24a9-4628-832d-21f370ec9a41
⛔ Files ignored due to path filters (2)
ghost/core/test/e2e-api/admin/__snapshots__/automated-emails.test.js.snapis excluded by!**/*.snapghost/core/test/integration/services/__snapshots__/member-welcome-emails-snapshot.test.js.snapis excluded by!**/*.snap
📒 Files selected for processing (2)
ghost/core/core/server/services/email-rendering/finalize.jsghost/core/test/unit/server/services/member-welcome-emails/member-welcome-email-renderer.test.js
🚧 Files skipped from review as they are similar to previous changes (1)
- ghost/core/core/server/services/email-rendering/finalize.js
This comment was marked as resolved.
This comment was marked as resolved.
1 similar comment
This comment was marked as resolved.
This comment was marked as resolved.
|
troyciesco
left a comment
troyciesco
left a comment
There was a problem hiding this comment.
up to you if you care about Sonar's recommendation for replaceAll (I don't really, since replace matches the other email renderer)
2 participants
closes https://linear.app/ghost/issue/NY-1233
ref #27485
Welcome emails use
<figure>and<figcaption>for images and their captions, which some email clients don't support. That causes the styling to be messed up in those clients.Newsletter rendering had already fixed this. This patch fixes it the same way.
In the long term, we should unify these renderers. In the short term, let's fix this bug.