🐛 Fixed missing newsletter title heading - #29642
Conversation
WalkthroughThe email wrapper now renders the post title link inside a styled 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ast-grep (0.45.0)ghost/core/test/unit/server/services/email-service/email-renderer.test.jsast-grep timed out on this file 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.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@ghost/core/test/unit/server/services/email-service/email-renderer.test.js`:
- Around line 1565-1577: Update the test case “renders the post title as the
top-level heading” to assert that heading.find('a.post-title-link') contains
exactly one element, while retaining the existing text assertion for “Test
Post”.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 0acc42fd-1d2c-4b3b-8429-e0711162f42a
📒 Files selected for processing (2)
ghost/core/core/server/services/email-rendering/partials/email-wrapper.hbsghost/core/test/unit/server/services/email-service/email-renderer.test.js
| it('renders the post title as the top-level heading', async function () { | ||
| const post = createModel(basePost); | ||
| const newsletter = createModel(baseNewsletter); | ||
|
|
||
| const response = await emailRenderer.renderBody(post, newsletter, null, {}); | ||
| await validateHtml(response.html); | ||
|
|
||
| const $ = cheerio.load(response.html); | ||
| const heading = $('.post-title > table td > h1'); | ||
|
|
||
| assert.equal(heading.length, 1); | ||
| assert.equal(heading.find('a.post-title-link').text(), 'Test Post'); | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert that the title contains exactly one link.
The text assertion alone does not reject duplicate matching links. Add a cardinality assertion so the test fully validates the intended markup.
Proposed fix
assert.equal(heading.length, 1);
+ assert.equal(heading.find('a.post-title-link').length, 1);
assert.equal(heading.find('a.post-title-link').text(), 'Test Post');📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| it('renders the post title as the top-level heading', async function () { | |
| const post = createModel(basePost); | |
| const newsletter = createModel(baseNewsletter); | |
| const response = await emailRenderer.renderBody(post, newsletter, null, {}); | |
| await validateHtml(response.html); | |
| const $ = cheerio.load(response.html); | |
| const heading = $('.post-title > table td > h1'); | |
| assert.equal(heading.length, 1); | |
| assert.equal(heading.find('a.post-title-link').text(), 'Test Post'); | |
| }); | |
| it('renders the post title as the top-level heading', async function () { | |
| const post = createModel(basePost); | |
| const newsletter = createModel(baseNewsletter); | |
| const response = await emailRenderer.renderBody(post, newsletter, null, {}); | |
| await validateHtml(response.html); | |
| const $ = cheerio.load(response.html); | |
| const heading = $('.post-title > table td > h1'); | |
| assert.equal(heading.length, 1); | |
| assert.equal(heading.find('a.post-title-link').length, 1); | |
| assert.equal(heading.find('a.post-title-link').text(), 'Test Post'); | |
| }); |
🤖 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 1565 - 1577, Update the test case “renders the post title as the
top-level heading” to assert that heading.find('a.post-title-link') contains
exactly one element, while retaining the existing text assertion for “Test
Post”.
fixes TryGhost#29612 Newsletter titles were styled visually but lacked the semantic top-level heading required to expose their structure to assistive technology.
- the h1 wrapper reset its typography with `inherit`, which Outlook's Word rendering engine ignores, so the title would have fallen back to the default h1 size and weight in every Outlook client - replaced the inline resets with a `.post-title-heading` rule so juice inlines concrete px/weight values matching the existing `.post-title > table td` rule - regenerated the email snapshots; the only delta is the h1 wrapper
9913bb3 to
3663b00
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In
`@ghost/core/core/server/services/email-service/email-templates/partials/styles.hbs`:
- Around line 164-170: Update the mobile email styles for `.post-title-heading`
so its font size is 26px and line height is 1.1, both with `!important`,
overriding the later `table.body h1` rules while preserving the existing desktop
declarations and nested-link styling.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 72fa24bb-88fb-4f2b-ab57-579aa7e03581
⛔ Files ignored due to path filters (3)
ghost/core/test/e2e-api/admin/__snapshots__/email-previews.test.js.snapis excluded by!**/*.snapghost/core/test/integration/services/email-service/__snapshots__/batch-sending.test.js.snapis excluded by!**/*.snapghost/core/test/integration/services/email-service/__snapshots__/cards.test.js.snapis excluded by!**/*.snap
📒 Files selected for processing (3)
ghost/core/core/server/services/email-rendering/partials/email-wrapper.hbsghost/core/core/server/services/email-service/email-templates/partials/styles.hbsghost/core/test/unit/server/services/email-service/email-renderer.test.js
🚧 Files skipped from review as they are similar to previous changes (2)
- ghost/core/core/server/services/email-rendering/partials/email-wrapper.hbs
- ghost/core/test/unit/server/services/email-service/email-renderer.test.js
|
| Command | Status | Duration | Result |
|---|---|---|---|
nx run ghost:test:ci:integration |
✅ Succeeded | 3m 9s | View ↗ |
nx run ghost:test:integration |
✅ Succeeded | 3m 15s | View ↗ |
nx run ghost:test:legacy |
✅ Succeeded | 3m 8s | View ↗ |
nx run ghost:test:e2e |
✅ Succeeded | 2m 47s | View ↗ |
nx run ghost-monorepo:lint:boundaries |
✅ Succeeded | 22s | View ↗ |
nx run-many -t lint -p ghost,ghost-monorepo |
✅ Succeeded | 20s | View ↗ |
nx run-many -t test:unit -p ghost |
✅ Succeeded | 29s | View ↗ |
nx run @tryghost/admin:build |
✅ Succeeded | 7s | View ↗ |
nx run-many --target=build --projects=tag:publi... |
✅ Succeeded | 1s | View ↗ |
💡 Verify your cache is correct by running tasks in a sandbox. Read docs ↗
☁️ Nx Cloud last updated this comment at 2026-07-29 14:47:46 UTC
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #29642 +/- ##
==========================================
- Coverage 75.45% 75.41% -0.05%
==========================================
Files 1607 1607
Lines 141190 141198 +8
Branches 17465 17451 -14
==========================================
- Hits 106535 106483 -52
- Misses 33611 33644 +33
- Partials 1044 1071 +27
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
2 participants
Problem
Newsletter post titles are styled visually as headings, but the rendered email marks them up as a regular link. This hides the top-level document structure from assistive technology and fails WCAG 1.3.1.
Solution
Wrap the post title link in an
h1while resetting the heading's default typography so the existing email-table layout and title styling remain unchanged. Add a focused rendering test that validates the output HTML and semantic title structure.Fixes #29612
Test plan
pnpm exec vitest run test/unit/server/services/email-service/email-renderer.test.js -t 'renders the post title as the top-level heading'pnpm exec eslint test/unit/server/services/email-service/email-renderer.test.js --no-cachegit diff --checkPlease check your PR against these items: