fix: correct header card layout parsing based on DOM structure and classes#1774
Open
saschabuehrle wants to merge 2 commits intoTryGhost:mainfrom
Open
fix: correct header card layout parsing based on DOM structure and classes#1774saschabuehrle wants to merge 2 commits intoTryGhost:mainfrom
saschabuehrle wants to merge 2 commits intoTryGhost:mainfrom
Conversation
…asses (fixes TryGhost#1656) The header card HTML parser was incorrectly defaulting to split layout whenever an image was present, ignoring the actual HTML structure and classes that indicate the intended layout. This fix implements proper layout detection by: - Checking for explicit layout classes (kg-layout-split, kg-content-wide) - Analyzing DOM structure (picture as direct child = full, picture inside content = split) - Only falling back to split layout when structure is ambiguous Test cases added for: - Full/wide layout with kg-content-wide class - Split layout with kg-layout-split class - Full layout when picture is direct child of card Resolves the issue where programmatically created header cards via Admin API were incorrectly parsed as split layout despite having correct full/wide layout HTML structure.
WalkthroughThe V2 header parser was modified to determine layout from HTML classes and structure instead of defaulting to split when an image exists. It now checks for Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip CodeRabbit can use TruffleHog to scan for secrets in your code with verification capabilities.Add a TruffleHog config file (e.g. trufflehog-config.yml, trufflehog.yml) to your project to customize detectors and scanning behavior. The tool runs only when a config file is present. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/kg-default-nodes/test/nodes/header.test.js (1)
359-387:⚠️ Potential issue | 🔴 CriticalExisting test HTML structure will cause layout assertion to fail with the new parser logic.
The test at lines 359-387 has
<picture>as a direct child of the card div, but expectslayout === 'split'at line 379.With the new parser logic (lines 68-71 of header-parser.js):
hasSplitClass: false (nokg-layout-splitclass)isPictureInContent: false (picture is outside content)isPictureDirectChild: trueThe parser executes line 70-71, returning
layout = ''(full/wide), not'split', causing the test assertion to fail.Fix by moving the
<picture>inside the.kg-header-card-contentdiv to match the split layout structure:Suggested fix
const htmlstring = ` <div class="kg-card kg-header-card kg-v2 kg-style-accent" data-background-color="#abcdef"> - <picture><img class="kg-header-card-image" src="https://example.com/image.jpg" alt="" /></picture> <div class="kg-header-card-content"> + <picture><img class="kg-header-card-image" src="https://example.com/image.jpg" alt="" /></picture> <div class="kg-header-card-text kg-align-center">🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/kg-default-nodes/test/nodes/header.test.js` around lines 359 - 387, The test's HTML places the <picture> as a direct child of the card which, per the header parser's logic (variables hasSplitClass, isPictureInContent, isPictureDirectChild that determine layout), yields layout === '' instead of 'split'; update the test's htmlstring inside the "parses a header card V2" test so the <picture> element is moved inside the .kg-header-card-content (so isPictureInContent becomes true) thereby making the parser return layout === 'split' and satisfying the node.layout assertion.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/kg-default-nodes/test/nodes/header.test.js`:
- Around line 359-387: The test's HTML places the <picture> as a direct child of
the card which, per the header parser's logic (variables hasSplitClass,
isPictureInContent, isPictureDirectChild that determine layout), yields layout
=== '' instead of 'split'; update the test's htmlstring inside the "parses a
header card V2" test so the <picture> element is moved inside the
.kg-header-card-content (so isPictureInContent becomes true) thereby making the
parser return layout === 'split' and satisfying the node.layout assertion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8f16b448-abf1-4e0f-9240-4b047198e495
📒 Files selected for processing (2)
packages/kg-default-nodes/lib/nodes/header/parsers/header-parser.jspackages/kg-default-nodes/test/nodes/header.test.js
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
| if (hasSplitClass || isPictureInContent) { | ||
| layout = 'split'; | ||
| } else if (hasContentWideClass || isPictureDirectChild) { | ||
| layout = ''; // full/wide layout (empty string) |
There was a problem hiding this comment.
Parser returns empty string, node constructor coerces to 'full'
High Severity
The parser sets layout = '' for full/wide layouts, but HeaderNode's default for layout is 'full', and the constructor uses data[prop.name] || prop.default — since '' is falsy, '' || 'full' evaluates to 'full'. This means node.layout will be 'full', not ''. The new test assertions at lines 419 and 457 (node.layout.should.equal('')) will fail. The parser needs to return 'full' explicitly instead of ''.
Additional Locations (2)
| } else { | ||
| // Fallback to old behavior if structure is ambiguous | ||
| layout = 'split'; | ||
| } |
There was a problem hiding this comment.
Existing V2 parser test breaks with new logic
High Severity
The pre-existing "parses a header card V2" test (line 359 of the test file) has HTML where <picture> is a direct child of the card div and no kg-layout-split class is present. The new parser identifies isPictureDirectChild as true and returns layout = '' (→ 'full'), but that test asserts node.layout.should.equal('split'). This existing test will fail with the new parser logic.
…ayout structure The test was expecting layout='split' but had the picture element as a direct child of the card instead of inside the content container. Per the parser logic, when isPictureInContent is false but isPictureDirectChild is true, the layout defaults to empty string rather than 'split'. Moving the picture inside .kg-header-card-content makes isPictureInContent true, resulting in the expected 'split' layout.
|
You have used all of your free Bugbot PR reviews. To receive reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/kg-default-nodes/test/nodes/header.test.js`:
- Around line 404-459: Add two precedence tests to header.test.js that assert
explicit classes override DOM cues: (1) a case with class "kg-content-wide" on
the card while the <picture> is nested inside ".kg-header-card-content" and
expect node.layout === '' and backgroundImageSrc set; (2) a case with class
"kg-layout-split" on the card while the <picture> is a direct child of the card
and expect node.layout === 'split' and backgroundImageSrc set. These tests
should mirror existing test patterns (using createDocument,
$generateNodesFromDOM and editorTest) so they exercise the parser in
header-parser.js and catch class-precedence regressions. Ensure assertions check
nodes.length === 1 and validate node.layout and node.backgroundImageSrc.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 665ff049-a270-4438-8cd7-ef0e8826d725
📒 Files selected for processing (1)
packages/kg-default-nodes/test/nodes/header.test.js
Comment on lines
+404
to
+459
| it('parses full/wide layout when kg-content-wide class is present', editorTest(function () { | ||
| const htmlstring = ` | ||
| <div class="kg-card kg-header-card kg-v2 kg-width-full kg-content-wide" data-background-color="#000000"> | ||
| <picture><img class="kg-header-card-image" src="https://example.com/image.jpg" alt="" /></picture> | ||
| <div class="kg-header-card-content"> | ||
| <div class="kg-header-card-text kg-align-center"> | ||
| <h2 class="kg-header-card-heading" style="color: #FFFFFF;">Title</h2> | ||
| <p class="kg-header-card-subheading" style="color: #FFFFFF;">Subtitle</p> | ||
| </div> | ||
| </div> | ||
| </div>`; | ||
| const document = createDocument(htmlstring); | ||
| const nodes = $generateNodesFromDOM(editor, document); | ||
| nodes.length.should.equal(1); | ||
| const node = nodes[0]; | ||
| node.layout.should.equal(''); // full/wide layout is empty string | ||
| node.backgroundImageSrc.should.equal('https://example.com/image.jpg'); | ||
| })); | ||
|
|
||
| it('parses split layout when kg-layout-split class is present', editorTest(function () { | ||
| const htmlstring = ` | ||
| <div class="kg-card kg-header-card kg-v2 kg-layout-split kg-width-full"> | ||
| <div class="kg-header-card-content"> | ||
| <picture><img class="kg-header-card-image" src="https://example.com/image.jpg" alt="" /></picture> | ||
| <div class="kg-header-card-text kg-align-center"> | ||
| <h2 class="kg-header-card-heading">Title</h2> | ||
| <p class="kg-header-card-subheading">Subtitle</p> | ||
| </div> | ||
| </div> | ||
| </div>`; | ||
| const document = createDocument(htmlstring); | ||
| const nodes = $generateNodesFromDOM(editor, document); | ||
| nodes.length.should.equal(1); | ||
| const node = nodes[0]; | ||
| node.layout.should.equal('split'); | ||
| node.backgroundImageSrc.should.equal('https://example.com/image.jpg'); | ||
| })); | ||
|
|
||
| it('parses full layout when picture is direct child of card', editorTest(function () { | ||
| const htmlstring = ` | ||
| <div class="kg-card kg-header-card kg-v2 kg-width-full" data-background-color="#000000"> | ||
| <picture><img class="kg-header-card-image" src="https://example.com/image.jpg" alt="" /></picture> | ||
| <div class="kg-header-card-content"> | ||
| <div class="kg-header-card-text kg-align-center"> | ||
| <h2 class="kg-header-card-heading">Title</h2> | ||
| <p class="kg-header-card-subheading">Subtitle</p> | ||
| </div> | ||
| </div> | ||
| </div>`; | ||
| const document = createDocument(htmlstring); | ||
| const nodes = $generateNodesFromDOM(editor, document); | ||
| nodes.length.should.equal(1); | ||
| const node = nodes[0]; | ||
| node.layout.should.equal(''); // full layout is empty string | ||
| node.backgroundImageSrc.should.equal('https://example.com/image.jpg'); | ||
| })); |
There was a problem hiding this comment.
Add precedence tests for explicit classes vs DOM cues.
The new cases are good, but they still don’t assert the required precedence rule (“explicit classes win”). Please add conflict scenarios so regressions are caught:
kg-content-wide+pictureinside.kg-header-card-content→ expectlayout === ''kg-layout-split+pictureas direct child of card → expectlayout === 'split'
Without these, class-precedence bugs in packages/kg-default-nodes/lib/nodes/header/parsers/header-parser.js can still pass this suite.
Proposed test additions
+ it('prefers kg-content-wide over picture-in-content structure', editorTest(function () {
+ const htmlstring = `
+ <div class="kg-card kg-header-card kg-v2 kg-content-wide">
+ <div class="kg-header-card-content">
+ <picture><img class="kg-header-card-image" src="https://example.com/image.jpg" alt="" /></picture>
+ </div>
+ </div>`;
+ const document = createDocument(htmlstring);
+ const nodes = $generateNodesFromDOM(editor, document);
+ nodes.length.should.equal(1);
+ const node = nodes[0];
+ node.layout.should.equal('');
+ }));
+
+ it('prefers kg-layout-split over picture-direct-child structure', editorTest(function () {
+ const htmlstring = `
+ <div class="kg-card kg-header-card kg-v2 kg-layout-split">
+ <picture><img class="kg-header-card-image" src="https://example.com/image.jpg" alt="" /></picture>
+ <div class="kg-header-card-content"></div>
+ </div>`;
+ const document = createDocument(htmlstring);
+ const nodes = $generateNodesFromDOM(editor, document);
+ nodes.length.should.equal(1);
+ const node = nodes[0];
+ node.layout.should.equal('split');
+ }));📝 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.
Suggested change
| it('parses full/wide layout when kg-content-wide class is present', editorTest(function () { | |
| const htmlstring = ` | |
| <div class="kg-card kg-header-card kg-v2 kg-width-full kg-content-wide" data-background-color="#000000"> | |
| <picture><img class="kg-header-card-image" src="https://example.com/image.jpg" alt="" /></picture> | |
| <div class="kg-header-card-content"> | |
| <div class="kg-header-card-text kg-align-center"> | |
| <h2 class="kg-header-card-heading" style="color: #FFFFFF;">Title</h2> | |
| <p class="kg-header-card-subheading" style="color: #FFFFFF;">Subtitle</p> | |
| </div> | |
| </div> | |
| </div>`; | |
| const document = createDocument(htmlstring); | |
| const nodes = $generateNodesFromDOM(editor, document); | |
| nodes.length.should.equal(1); | |
| const node = nodes[0]; | |
| node.layout.should.equal(''); // full/wide layout is empty string | |
| node.backgroundImageSrc.should.equal('https://example.com/image.jpg'); | |
| })); | |
| it('parses split layout when kg-layout-split class is present', editorTest(function () { | |
| const htmlstring = ` | |
| <div class="kg-card kg-header-card kg-v2 kg-layout-split kg-width-full"> | |
| <div class="kg-header-card-content"> | |
| <picture><img class="kg-header-card-image" src="https://example.com/image.jpg" alt="" /></picture> | |
| <div class="kg-header-card-text kg-align-center"> | |
| <h2 class="kg-header-card-heading">Title</h2> | |
| <p class="kg-header-card-subheading">Subtitle</p> | |
| </div> | |
| </div> | |
| </div>`; | |
| const document = createDocument(htmlstring); | |
| const nodes = $generateNodesFromDOM(editor, document); | |
| nodes.length.should.equal(1); | |
| const node = nodes[0]; | |
| node.layout.should.equal('split'); | |
| node.backgroundImageSrc.should.equal('https://example.com/image.jpg'); | |
| })); | |
| it('parses full layout when picture is direct child of card', editorTest(function () { | |
| const htmlstring = ` | |
| <div class="kg-card kg-header-card kg-v2 kg-width-full" data-background-color="#000000"> | |
| <picture><img class="kg-header-card-image" src="https://example.com/image.jpg" alt="" /></picture> | |
| <div class="kg-header-card-content"> | |
| <div class="kg-header-card-text kg-align-center"> | |
| <h2 class="kg-header-card-heading">Title</h2> | |
| <p class="kg-header-card-subheading">Subtitle</p> | |
| </div> | |
| </div> | |
| </div>`; | |
| const document = createDocument(htmlstring); | |
| const nodes = $generateNodesFromDOM(editor, document); | |
| nodes.length.should.equal(1); | |
| const node = nodes[0]; | |
| node.layout.should.equal(''); // full layout is empty string | |
| node.backgroundImageSrc.should.equal('https://example.com/image.jpg'); | |
| })); | |
| it('parses full/wide layout when kg-content-wide class is present', editorTest(function () { | |
| const htmlstring = ` | |
| <div class="kg-card kg-header-card kg-v2 kg-width-full kg-content-wide" data-background-color="#000000"> | |
| <picture><img class="kg-header-card-image" src="https://example.com/image.jpg" alt="" /></picture> | |
| <div class="kg-header-card-content"> | |
| <div class="kg-header-card-text kg-align-center"> | |
| <h2 class="kg-header-card-heading" style="color: `#FFFFFF`;">Title</h2> | |
| <p class="kg-header-card-subheading" style="color: `#FFFFFF`;">Subtitle</p> | |
| </div> | |
| </div> | |
| </div>`; | |
| const document = createDocument(htmlstring); | |
| const nodes = $generateNodesFromDOM(editor, document); | |
| nodes.length.should.equal(1); | |
| const node = nodes[0]; | |
| node.layout.should.equal(''); // full/wide layout is empty string | |
| node.backgroundImageSrc.should.equal('https://example.com/image.jpg'); | |
| })); | |
| it('parses split layout when kg-layout-split class is present', editorTest(function () { | |
| const htmlstring = ` | |
| <div class="kg-card kg-header-card kg-v2 kg-layout-split kg-width-full"> | |
| <div class="kg-header-card-content"> | |
| <picture><img class="kg-header-card-image" src="https://example.com/image.jpg" alt="" /></picture> | |
| <div class="kg-header-card-text kg-align-center"> | |
| <h2 class="kg-header-card-heading">Title</h2> | |
| <p class="kg-header-card-subheading">Subtitle</p> | |
| </div> | |
| </div> | |
| </div>`; | |
| const document = createDocument(htmlstring); | |
| const nodes = $generateNodesFromDOM(editor, document); | |
| nodes.length.should.equal(1); | |
| const node = nodes[0]; | |
| node.layout.should.equal('split'); | |
| node.backgroundImageSrc.should.equal('https://example.com/image.jpg'); | |
| })); | |
| it('parses full layout when picture is direct child of card', editorTest(function () { | |
| const htmlstring = ` | |
| <div class="kg-card kg-header-card kg-v2 kg-width-full" data-background-color="#000000"> | |
| <picture><img class="kg-header-card-image" src="https://example.com/image.jpg" alt="" /></picture> | |
| <div class="kg-header-card-content"> | |
| <div class="kg-header-card-text kg-align-center"> | |
| <h2 class="kg-header-card-heading">Title</h2> | |
| <p class="kg-header-card-subheading">Subtitle</p> | |
| </div> | |
| </div> | |
| </div>`; | |
| const document = createDocument(htmlstring); | |
| const nodes = $generateNodesFromDOM(editor, document); | |
| nodes.length.should.equal(1); | |
| const node = nodes[0]; | |
| node.layout.should.equal(''); // full layout is empty string | |
| node.backgroundImageSrc.should.equal('https://example.com/image.jpg'); | |
| })); | |
| it('prefers kg-content-wide over picture-in-content structure', editorTest(function () { | |
| const htmlstring = ` | |
| <div class="kg-card kg-header-card kg-v2 kg-content-wide"> | |
| <div class="kg-header-card-content"> | |
| <picture><img class="kg-header-card-image" src="https://example.com/image.jpg" alt="" /></picture> | |
| </div> | |
| </div>`; | |
| const document = createDocument(htmlstring); | |
| const nodes = $generateNodesFromDOM(editor, document); | |
| nodes.length.should.equal(1); | |
| const node = nodes[0]; | |
| node.layout.should.equal(''); | |
| })); | |
| it('prefers kg-layout-split over picture-direct-child structure', editorTest(function () { | |
| const htmlstring = ` | |
| <div class="kg-card kg-header-card kg-v2 kg-layout-split"> | |
| <picture><img class="kg-header-card-image" src="https://example.com/image.jpg" alt="" /></picture> | |
| <div class="kg-header-card-content"></div> | |
| </div>`; | |
| const document = createDocument(htmlstring); | |
| const nodes = $generateNodesFromDOM(editor, document); | |
| nodes.length.should.equal(1); | |
| const node = nodes[0]; | |
| node.layout.should.equal('split'); | |
| })); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/kg-default-nodes/test/nodes/header.test.js` around lines 404 - 459,
Add two precedence tests to header.test.js that assert explicit classes override
DOM cues: (1) a case with class "kg-content-wide" on the card while the
<picture> is nested inside ".kg-header-card-content" and expect node.layout ===
'' and backgroundImageSrc set; (2) a case with class "kg-layout-split" on the
card while the <picture> is a direct child of the card and expect node.layout
=== 'split' and backgroundImageSrc set. These tests should mirror existing test
patterns (using createDocument, $generateNodesFromDOM and editorTest) so they
exercise the parser in header-parser.js and catch class-precedence regressions.
Ensure assertions check nodes.length === 1 and validate node.layout and
node.backgroundImageSrc.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Bug
Fixes #1656 — The header card HTML parser was incorrectly defaulting to split layout whenever an image was present, ignoring the actual HTML structure and classes that indicate the intended layout.
Fix
This PR implements proper layout detection by:
Testing
Added comprehensive test cases covering:
Impact
This resolves the issue where programmatically created header cards via the Admin API were incorrectly parsed as split layout despite having correct full/wide layout HTML structure. Now Ghost will properly maintain the intended layout when importing HTML cards.
Verification
The fix handles the exact HTML structure mentioned in the issue:
This will now correctly parse as full/wide layout instead of incorrectly defaulting to split.
Greetings, saschabuehrle
Note
Medium Risk
Changes HTML import behavior for v2 header cards, which could affect how existing content is interpreted during parsing. Scope is limited to layout detection and covered by new unit tests.
Overview
Fixes v2 header card HTML parsing so
layoutis derived from explicit classes (e.g.kg-layout-split,kg-content-wide) and the<picture>placement in the DOM, instead of defaulting tosplitwhenever an image exists.Adds import tests covering full/wide vs split layouts to prevent regressions when header card HTML is generated externally (e.g. via API).
Written by Cursor Bugbot for commit 5f5602b. This will update automatically on new commits. Configure here.