Expanded test coverage for node renderers #23949
Conversation
|
Warning Rate limit exceeded@kevinansfield has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 24 minutes and 45 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (13)
WalkthroughThis set of changes removes all logic, feature flags, and configuration related to the "collectionsCard" feature across the codebase. This includes deleting the feature flag from configuration files, services, and the Koenig editor's card configuration, as well as eliminating all conditional code paths and methods in the Post model that depended on this flag. The corresponding test code that enabled or referenced the "collectionsCard" flag is also updated to remove such references. Additionally, several dependencies in ✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
kevinansfield
changed the title
![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
🔭 Outside diff range comments (1)
ghost/core/core/server/models/post.js (1)
1367-1373: Simplify bulkDestroy by removing dead code
Both branches invoke the samebulkDestroycall. Drop the conditional and return the call directly.Example refactor:
- bulkDestroy: async function bulkDestroy(ids, tableName, options) { - if (tableName === this.prototype.tableName) { - return ghostBookshelf.Model.bulkDestroy.call(this, ids, tableName, options); - } else { - return ghostBookshelf.Model.bulkDestroy.call(this, ids, tableName, options); - } - }, + bulkDestroy: function bulkDestroy(ids, tableName, options) { + return ghostBookshelf.Model.bulkDestroy.call(this, ids, tableName, options); + },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (22)
ghost/admin/app/components/koenig-lexical-editor.js(0 hunks)ghost/admin/app/services/feature.js(0 hunks)ghost/admin/package.json(2 hunks)ghost/core/core/server/models/post.js(2 hunks)ghost/core/core/server/services/koenig/node-renderers/callout-renderer.js(2 hunks)ghost/core/core/shared/labs.js(0 hunks)ghost/core/package.json(2 hunks)ghost/core/test/e2e-api/admin/pages.test.js(0 hunks)ghost/core/test/e2e-api/admin/posts-bulk.test.js(0 hunks)ghost/core/test/e2e-api/admin/posts.test.js(0 hunks)ghost/core/test/unit/server/services/koenig/node-renderers/call-to-action-renderer.test.js(3 hunks)ghost/core/test/unit/server/services/koenig/node-renderers/callout-renderer.test.js(2 hunks)ghost/core/test/unit/server/services/koenig/node-renderers/email-renderer.test.js(1 hunks)ghost/core/test/unit/server/services/koenig/node-renderers/embed-renderer.test.js(2 hunks)ghost/core/test/unit/server/services/koenig/node-renderers/file-renderer.test.js(1 hunks)ghost/core/test/unit/server/services/koenig/node-renderers/gallery-renderer.test.js(2 hunks)ghost/core/test/unit/server/services/koenig/node-renderers/html-renderer.test.js(2 hunks)ghost/core/test/unit/server/services/koenig/node-renderers/image-renderer.test.js(1 hunks)ghost/core/test/unit/server/services/koenig/node-renderers/product-renderer.test.js(2 hunks)ghost/core/test/unit/server/services/koenig/node-renderers/video-renderer.test.js(1 hunks)ghost/core/test/unit/server/services/koenig/test-utils/build-call-renderer.js(1 hunks)ghost/core/test/unit/server/services/koenig/test-utils/prettify-html.js(1 hunks)
💤 Files with no reviewable changes (6)
- ghost/core/core/shared/labs.js
- ghost/core/test/e2e-api/admin/posts-bulk.test.js
- ghost/core/test/e2e-api/admin/posts.test.js
- ghost/core/test/e2e-api/admin/pages.test.js
- ghost/admin/app/components/koenig-lexical-editor.js
- ghost/admin/app/services/feature.js
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Setup
🔇 Additional comments (28)
ghost/admin/package.json (1)
50-52: Verify the updated package versions exist and are compatible.The coordinated dependency updates for the Koenig editor packages look appropriate. These minor/patch version bumps align with the broader changes removing the collectionsCard feature.
#!/bin/bash # Check if the specified package versions exist on npm registry echo "Checking @tryghost/kg-clean-basic-html@4.2.6..." npm view @tryghost/kg-clean-basic-html@4.2.6 version echo "Checking @tryghost/kg-converters@1.1.6..." npm view @tryghost/kg-converters@1.1.6 version echo "Checking @tryghost/koenig-lexical@1.7.0..." npm view @tryghost/koenig-lexical@1.7.0 versionghost/core/test/unit/server/services/koenig/test-utils/prettify-html.js (1)
5-9: LGTM! Preserving HTML comments improves test utility functionality.Adding
removeComments: falseto the minifier options is a good enhancement for test utilities, allowing HTML comments to be preserved during prettification for better debugging and verification capabilities.ghost/core/package.json (2)
89-96: Coordinated dependency updates look good.The Koenig-related package updates are properly coordinated with the admin package.json changes and support the broader collectionsCard feature removal.
#!/bin/bash # Verify the updated kg-* package versions exist packages=( "@tryghost/kg-clean-basic-html@4.2.6" "@tryghost/kg-converters@1.1.6" "@tryghost/kg-default-cards@10.1.4" "@tryghost/kg-default-nodes@1.5.0" "@tryghost/kg-default-transforms@1.2.22" "@tryghost/kg-html-to-lexical@1.2.22" "@tryghost/kg-lexical-html-renderer@1.3.22" ) for package in "${packages[@]}"; do echo "Checking $package..." npm view "$package" version 2>/dev/null || echo "Package $package not found" done
222-222: Verify the optional dependency update.The
@tryghost/html-to-mobiledocupdate should be verified for compatibility.#!/bin/bash # Check the optional dependency version echo "Checking @tryghost/html-to-mobiledoc@3.2.7..." npm view @tryghost/html-to-mobiledoc@3.2.7 versionghost/core/test/unit/server/services/koenig/node-renderers/video-renderer.test.js (1)
88-115: Excellent test coverage additions for conditional rendering.The new test cases effectively cover important conditional rendering scenarios:
- Card width CSS class application
- Loop attribute inclusion
- Caption rendering with proper classes and structure
The tests follow established patterns and use appropriate assertions.
ghost/core/test/unit/server/services/koenig/node-renderers/file-renderer.test.js (1)
140-173: LGTM! Important security and edge case coverage.These test additions provide valuable coverage:
Minimal rendering test (lines 140-157): Ensures the component gracefully handles minimal data and applies appropriate styling (smaller icon height).
HTML escaping test (lines 159-173): Critical security test verifying XSS prevention through proper HTML escaping in all user-controlled fields.
The tests use appropriate assertions and follow established testing patterns.
ghost/core/test/unit/server/services/koenig/test-utils/build-call-renderer.js (1)
40-41: LGTM! Correct option merging order.This fix ensures that user-provided options properly override default options, which is the expected behavior. Previously, defaults were being spread after user options, causing them to override user preferences.
ghost/core/test/unit/server/services/koenig/node-renderers/email-renderer.test.js (2)
44-82: Excellent comprehensive placeholder testing.These test cases thoroughly cover the placeholder wrapping functionality, including valid patterns like
{variable}and{variable, "default"}, as well as edge cases with extra spaces. The negative test cases properly verify that invalid patterns (with spaces at beginning/end or multiple words without quotes) are not wrapped.
84-109: Good coverage for complex HTML structures.These tests verify proper handling of multiple paragraphs and the selective replacement of
<code>elements containing placeholders while preserving other<code>elements. This ensures the renderer correctly distinguishes between placeholder code and regular code content.ghost/core/test/unit/server/services/koenig/node-renderers/embed-renderer.test.js (2)
61-69: Good edge case coverage for missing metadata.This test ensures the renderer gracefully handles the case when metadata is explicitly
null, verifying that the HTML and caption are still properly rendered within the expected figure structure.
176-333: Comprehensive Twitter embed testing for email.This test thoroughly validates the complex table-based email rendering for Twitter embeds, including proper handling of tweet data, mentions, timestamps, and engagement metrics. The test data structure is realistic and the expected HTML output validates the correct email-specific rendering approach.
ghost/core/test/unit/server/services/koenig/node-renderers/callout-renderer.test.js (2)
41-59: Excellent edge case and security testing.These tests cover important scenarios:
- Rendering without emoji (graceful degradation)
- Fallback to white background for invalid/transparent colors
- Preservation of inline code with proper attributes and styles
- Sanitization by stripping disallowed HTML tags
The tests ensure both functionality and security of the callout renderer.
81-99: Good duplication of web tests for email target.The email rendering tests mirror the web tests, ensuring consistent behavior across both rendering targets. This is important for maintaining feature parity between web and email outputs.
ghost/core/test/unit/server/services/koenig/node-renderers/product-renderer.test.js (2)
82-153: Comprehensive product card rendering scenarios.These tests thoroughly cover different combinations of product elements (title, description, button, image) and verify that the renderer handles missing or empty content gracefully. The tests ensure proper HTML structure is maintained regardless of which elements are present.
240-432: Thorough email rendering test coverage.The email rendering tests cover important scenarios including:
- Proper image width/height handling
- Disabled star rating and button functionality
- Missing image handling
- Correct table-based HTML structure for email clients
These tests ensure the email renderer produces valid, accessible email HTML across different product card configurations.
ghost/core/core/server/services/koenig/node-renderers/callout-renderer.js (2)
3-3: Good refactoring to use tagged template literals.The switch to using tagged template literals improves code readability and maintainability. The declarative approach is cleaner than manual DOM manipulation.
Also applies to: 16-31
33-38: Well-structured sanitization helper function.The
cleanCalloutTextfunction properly centralizes the sanitization logic and includes an appropriate set of allowed HTML tags for callout formatting.ghost/core/test/unit/server/services/koenig/node-renderers/html-renderer.test.js (3)
3-3: Excellent use of prettification utilities for cleaner test assertions.The adoption of
assertPrettifiesToimproves test readability and makes snapshot comparisons more maintainable.Also applies to: 26-30
38-55: Good coverage of edge cases.The new tests for unclosed tags, HTML entities, and single-quote attributes ensure the renderer handles various HTML quirks correctly.
57-199: Comprehensive content visibility feature testing.The extensive test coverage for both old and new visibility settings ensures backward compatibility while supporting the new content visibility features. The tests properly verify gating wrappers, segment targeting, and conditional rendering for both web and email targets.
ghost/core/test/unit/server/services/koenig/node-renderers/call-to-action-renderer.test.js (2)
4-4: Good parameterized testing approach.Using the
LAYOUTSconstant to test multiple layout variations ensures consistent behavior across different CTA card layouts.Also applies to: 95-102
419-567: Thorough dark mode button color conversion testing.The comprehensive tests for black button conversion on dark backgrounds with various CTA background colors ensure proper visual contrast. Good coverage of edge cases including hex color variations (#000000, #000, black).
ghost/core/test/unit/server/services/koenig/node-renderers/image-renderer.test.js (3)
73-77: Good edge case handling for missing src.Testing that an empty span is rendered when src is missing prevents potential rendering errors.
117-281: Excellent comprehensive srcset attribute testing.The extensive test coverage for srcset generation covers all major scenarios including:
- Unsplash image handling
- Subdirectory support (both relative and absolute)
- Proper omission conditions (email target, missing dimensions, disabled srcsets)
- Size-based srcset filtering
This ensures responsive images work correctly across different configurations.
283-370: Thorough sizes attribute testing with proper media queries.The tests properly verify that the sizes attribute is:
- Added with correct media queries for different card widths
- Omitted when appropriate (missing srcset, full-width cards, small images)
- Using the correct breakpoints (720px for standard, 1200px for wide)
ghost/core/test/unit/server/services/koenig/node-renderers/gallery-renderer.test.js (3)
227-264: Good defensive programming with invalid image filtering.Testing that images without width/height dimensions are skipped prevents potential rendering issues and ensures only valid images are included in galleries.
266-307: Correct handling of image resizing constraints.The test properly verifies that local images respect the max width configuration while Unsplash images maintain their original dimensions. This ensures consistent behavior across different image sources.
694-765: Comprehensive email-specific image handling.The tests properly verify:
- Images are resized to fit 600px email template width
- Smaller images maintain their original dimensions
- Height is proportionally adjusted
- Correct image transformation URLs are used
- Fallback to original images when transformation isn't available
This ensures galleries render correctly in email clients.
| bulkEdit: async function bulkEdit(ids, tableName, options) { | ||
| if (tableName === this.prototype.tableName) { | ||
| const result = await ghostBookshelf.Model.bulkEdit.call(this, ids, tableName, options); | ||
|
|
||
| if (labs.isSet('collectionsCard')) { | ||
| // reset all page HTML so collection cards can be re-rendered with updated posts | ||
| // NOTE: we can't check for only published edits here as we don't have access to previous values | ||
| // to see if a previously published post has been unpublished, so we just reset all pages | ||
| const pageResetQuery = ghostBookshelf.knex.raw('UPDATE posts set html = NULL WHERE type = "page" AND lexical IS NOT NULL'); | ||
| await (options.transacting ? pageResetQuery.transacting(options.transacting) : pageResetQuery); | ||
| } | ||
|
|
||
| return result; | ||
| return await ghostBookshelf.Model.bulkEdit.call(this, ids, tableName, options); | ||
| } else { | ||
| return ghostBookshelf.Model.bulkEdit.call(this, ids, tableName, options); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove redundant conditional in bulkEdit
The if and else both call the same method (with only a difference in await usage), making the branch unnecessary. Simplify by returning the call directly.
Proposed diff:
- bulkEdit: async function bulkEdit(ids, tableName, options) {
- if (tableName === this.prototype.tableName) {
- return await ghostBookshelf.Model.bulkEdit.call(this, ids, tableName, options);
- } else {
- return ghostBookshelf.Model.bulkEdit.call(this, ids, tableName, options);
- }
- },
+ bulkEdit: function bulkEdit(ids, tableName, options) {
+ return ghostBookshelf.Model.bulkEdit.call(this, ids, tableName, options);
+ },📝 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.
| bulkEdit: async function bulkEdit(ids, tableName, options) { | |
| if (tableName === this.prototype.tableName) { | |
| const result = await ghostBookshelf.Model.bulkEdit.call(this, ids, tableName, options); | |
| if (labs.isSet('collectionsCard')) { | |
| // reset all page HTML so collection cards can be re-rendered with updated posts | |
| // NOTE: we can't check for only published edits here as we don't have access to previous values | |
| // to see if a previously published post has been unpublished, so we just reset all pages | |
| const pageResetQuery = ghostBookshelf.knex.raw('UPDATE posts set html = NULL WHERE type = "page" AND lexical IS NOT NULL'); | |
| await (options.transacting ? pageResetQuery.transacting(options.transacting) : pageResetQuery); | |
| } | |
| return result; | |
| return await ghostBookshelf.Model.bulkEdit.call(this, ids, tableName, options); | |
| } else { | |
| return ghostBookshelf.Model.bulkEdit.call(this, ids, tableName, options); | |
| } | |
| bulkEdit: function bulkEdit(ids, tableName, options) { | |
| return ghostBookshelf.Model.bulkEdit.call(this, ids, tableName, options); | |
| }, |
🤖 Prompt for AI Agents
In ghost/core/core/server/models/post.js around lines 1313 to 1318, the bulkEdit
function contains an if-else that calls the same method in both branches, making
the conditional redundant. Simplify by removing the if-else and directly
returning the awaited call to ghostBookshelf.Model.bulkEdit with the given
parameters.
no issue When node renderers were moved over from Koenig, the tests were set up with minimal cases to ensure the relevant renderer was running. This was fine because there were no substantial changes to renderers and tests for edge-cases still existed in Koenig. Now that we're planning on deleting the now-duplicated renderers from Koenig we'd like to ensure we're not dropping test coverage. - fixed issue with `assertPrettifiesTo()` test helper dropping comments - moved renderer tests over from Koenig, updated to work with Ghost's render and test setup - updated callout renderer to use a string template in place of manual DOM building
kevinansfield
force-pushed
the
expand-renderer-tests
branch
from
8fe3604 to
d41f45f
Compare
no issue
When node renderers were moved over from Koenig, the tests were set up with minimal cases to ensure the relevant renderer was running. This was fine because there were no substantial changes to renderers and tests for edge-cases still existed in Koenig. Now that we're planning on deleting the now-duplicated renderers from Koenig we'd like to ensure we're not dropping test coverage.
assertPrettifiesTo()test helper dropping comments