Fixed plaintext email content#27848
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:
Hidden review stack artifactWalkthroughThis PR adds the 🚥 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)
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
ghost/core/test/unit/server/services/email-service/email-renderer.test.js (1)
1658-1677: ⚡ Quick winAdd a positive plaintext assertion to avoid false positives.
This test only checks excluded characters; it should also verify expected preheader content is still present.
Proposed test hardening
it('excludes preheader spacing characters from plaintext', async function () { const post = createModel(basePost); const newsletter = createModel(baseNewsletter); const response = await emailRenderer.renderBody( post, newsletter, null, {} ); + assert( + response.plaintext.includes('Test plaintext for post'), + 'plaintext should retain preheader content' + ); + // These characters are in the spacing after the preheader, which should be excluded const FIGURE_SPACE = '\u2007'; const COMBINING_GRAPHEME_JOINER = '\u034F'; const SOFT_HYPHEN = '\u00AD';🤖 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 `@ghost/core/test/unit/server/services/email-service/email-renderer.test.js` around lines 1658 - 1677, The test currently only asserts excluded characters are not present; add a positive assertion to ensure expected preheader text is retained. After calling emailRenderer.renderBody(post, newsletter, null, {}), derive the expected preheader from the test fixtures (e.g. use post.custom_excerpt || post.excerpt || post.title) and assert that response.plaintext.includes(expectedPreheader) so the test verifies the preheader content exists as well as excluded characters are removed; update the test case around the existing assertions that reference response.plaintext, post, basePost, baseNewsletter, createModel and emailRenderer.renderBody.
🤖 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.
Nitpick comments:
In `@ghost/core/test/unit/server/services/email-service/email-renderer.test.js`:
- Around line 1658-1677: The test currently only asserts excluded characters are
not present; add a positive assertion to ensure expected preheader text is
retained. After calling emailRenderer.renderBody(post, newsletter, null, {}),
derive the expected preheader from the test fixtures (e.g. use
post.custom_excerpt || post.excerpt || post.title) and assert that
response.plaintext.includes(expectedPreheader) so the test verifies the
preheader content exists as well as excluded characters are removed; update the
test case around the existing assertions that reference response.plaintext,
post, basePost, baseNewsletter, createModel and emailRenderer.renderBody.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 64340bb3-7eeb-4f22-bf72-a3a6c8d5c91c
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (3)
ghost/core/core/server/services/email-rendering/partials/email-wrapper.hbsghost/core/package.jsonghost/core/test/unit/server/services/email-service/email-renderer.test.js
e370652 to
b6d2e68
Compare
aileen
left a comment
There was a problem hiding this comment.
Took me a while to connect the dots why adding the preheader-spacing class is the only change here, but seeing that the main change actually happened in the upstream dependency, made it clear.
Just one more snapshot that needs updating, otherwise LGTM
ref [ONC-1682](https://linear.app/ghost/issue/ONC-1682/plain-text-newsletter-encoding-artifacts-potential-deliverability) Remove the whitespace characters used only for presentation from the plaintext version of the email
b6d2e68 to
9cddaa1
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #27848 +/- ##
=======================================
Coverage 73.81% 73.81%
=======================================
Files 1519 1519
Lines 128187 128187
Branches 15357 15356 -1
=======================================
+ Hits 94621 94625 +4
+ Misses 32636 32632 -4
Partials 930 930
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:
|
ref ONC-1682
Remove the whitespace characters used only for presentation from the plaintext version of the email