Refactored preview handling in post content output#26887
Conversation
ref https://linear.app/ghost/issue/NY-1154/ - existing preview substitution was bolted-on to visibility gating, which was inappropriate; it is now its own step The intent of this change is to provide a pattern for subtituting content in the Preview pane in Admin > Post Editor. In this case, we substitute a user-specific iframe with a placeholder rather than using a real user's uuid and display a real user's content.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThe changes relocate Transistor embed preview-mode handling from the post-gating module into a new dedicated 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
ghost/core/core/server/api/endpoints/utils/serializers/output/utils/preview-rendering.js (2)
19-21: Redundant null check.The
if (attrs.html)check on line 19 is unnecessary since line 13 already confirmsattrs.htmlexists and contains the Transistor embed marker.♻️ Suggested simplification
-if (attrs.html) { - attrs.plaintext = htmlToPlaintext.excerpt(attrs.html); -} +attrs.plaintext = htmlToPlaintext.excerpt(attrs.html);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ghost/core/core/server/api/endpoints/utils/serializers/output/utils/preview-rendering.js` around lines 19 - 21, Remove the redundant guard around turning HTML into plaintext: since earlier code already verifies attrs.html contains the Transistor embed marker, delete the `if (attrs.html)` check and always assign `attrs.plaintext = htmlToPlaintext.excerpt(attrs.html)`; update any surrounding comments if present and ensure this change is made where attrs.html and htmlToPlaintext.excerpt are referenced so behavior remains identical without the unnecessary null check.
14-17: Regex may not handle whitespace between elements.The regex expects
</iframe><script>to be directly adjacent. If Ghost's HTML generation ever introduces whitespace or newlines between the iframe and script elements, this replacement would silently fail.Consider adding optional whitespace handling:
♻️ Suggested optional improvement
-/<iframe[^>]*data-kg-transistor-embed[^>]*><\/iframe><script>[\s\S]*?<\/script>(?:<noscript>[\s\S]*?<\/noscript>)?/g, +/<iframe[^>]*data-kg-transistor-embed[^>]*><\/iframe>\s*<script>[\s\S]*?<\/script>(?:\s*<noscript>[\s\S]*?<\/noscript>)?/g,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ghost/core/core/server/api/endpoints/utils/serializers/output/utils/preview-rendering.js` around lines 14 - 17, The current replacement regex in the attrs.html.replace call (which uses _buildTransistorPlaceholder()) assumes </iframe><script> are adjacent; update the regex to allow optional whitespace/newlines between the iframe and script tags (e.g., match </iframe>\s*<script> instead of </iframe><script>) so the replacement still works when HTML generation inserts spaces or newlines; ensure the rest of the pattern (the lazy [\s\S]*? parts and optional noscript group) remains intact and still targets the same _buildTransistorPlaceholder() replacement point.ghost/core/test/unit/api/canary/utils/serializers/output/utils/preview-rendering.test.js (1)
80-92: Consider adding test for missing excerpt.This test validates excerpt update when
attrs.excerptalready exists. Based on the current implementation, ifattrs.excerptis undefined, it won't be populated. Consider adding a test to document this behavior:it('does not create excerpt when none exists', function () { frame.isPreview = true; const attrs = { html: '<p>Some text</p><iframe src="https://example.com" data-kg-transistor-embed></iframe><script>x</script>', plaintext: 'old plaintext' // no excerpt }; previewRendering.forPost(attrs, frame); assert.equal(attrs.excerpt, undefined); });If the intent is to populate the excerpt when missing, both the implementation and this test should be updated accordingly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ghost/core/test/unit/api/canary/utils/serializers/output/utils/preview-rendering.test.js` around lines 80 - 92, Add a unit test that documents current behavior when attrs.excerpt is missing: create a new "it('does not create excerpt when none exists', ...)" test that sets frame.isPreview = true, builds attrs with html and plaintext but no excerpt, calls previewRendering.forPost(attrs, frame), and asserts that attrs.excerpt remains undefined; reference previewRendering.forPost, frame.isPreview and attrs.excerpt so the test targets the same behavior as the existing 'updates plaintext and excerpt after replacement' test.
🤖 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/api/endpoints/utils/serializers/output/utils/preview-rendering.js`:
- Around line 19-21: Remove the redundant guard around turning HTML into
plaintext: since earlier code already verifies attrs.html contains the
Transistor embed marker, delete the `if (attrs.html)` check and always assign
`attrs.plaintext = htmlToPlaintext.excerpt(attrs.html)`; update any surrounding
comments if present and ensure this change is made where attrs.html and
htmlToPlaintext.excerpt are referenced so behavior remains identical without the
unnecessary null check.
- Around line 14-17: The current replacement regex in the attrs.html.replace
call (which uses _buildTransistorPlaceholder()) assumes </iframe><script> are
adjacent; update the regex to allow optional whitespace/newlines between the
iframe and script tags (e.g., match </iframe>\s*<script> instead of
</iframe><script>) so the replacement still works when HTML generation inserts
spaces or newlines; ensure the rest of the pattern (the lazy [\s\S]*? parts and
optional noscript group) remains intact and still targets the same
_buildTransistorPlaceholder() replacement point.
In
`@ghost/core/test/unit/api/canary/utils/serializers/output/utils/preview-rendering.test.js`:
- Around line 80-92: Add a unit test that documents current behavior when
attrs.excerpt is missing: create a new "it('does not create excerpt when none
exists', ...)" test that sets frame.isPreview = true, builds attrs with html and
plaintext but no excerpt, calls previewRendering.forPost(attrs, frame), and
asserts that attrs.excerpt remains undefined; reference
previewRendering.forPost, frame.isPreview and attrs.excerpt so the test targets
the same behavior as the existing 'updates plaintext and excerpt after
replacement' test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a3a849e1-441c-4c27-93a1-3e9e06c01b90
📒 Files selected for processing (4)
ghost/core/core/server/api/endpoints/utils/serializers/output/mappers/posts.jsghost/core/core/server/api/endpoints/utils/serializers/output/utils/post-gating.jsghost/core/core/server/api/endpoints/utils/serializers/output/utils/preview-rendering.jsghost/core/test/unit/api/canary/utils/serializers/output/utils/preview-rendering.test.js
💤 Files with no reviewable changes (1)
- ghost/core/core/server/api/endpoints/utils/serializers/output/utils/post-gating.js
There was a problem hiding this comment.
Pull request overview
This PR refactors Transistor preview substitution out of post content gating and into a dedicated preview-rendering step, to support safer Admin editor previews.
Changes:
- Added a new
preview-renderingserializer utility to replace Transistor iframe/script/noscript with a static placeholder during preview requests. - Removed the preview-specific Transistor replacement logic from
post-gating. - Wired the new preview-rendering step into the posts output mapper and added unit tests.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| ghost/core/test/unit/api/canary/utils/serializers/output/utils/preview-rendering.test.js | Adds unit tests covering Transistor embed placeholder replacement behavior in preview mode. |
| ghost/core/core/server/api/endpoints/utils/serializers/output/utils/preview-rendering.js | Introduces a dedicated preview-only rendering transform for Transistor embeds and updates derived text fields. |
| ghost/core/core/server/api/endpoints/utils/serializers/output/utils/post-gating.js | Removes Transistor preview substitution logic now handled by the new preview-rendering utility. |
| ghost/core/core/server/api/endpoints/utils/serializers/output/mappers/posts.js | Invokes previewRendering.forPost as a separate step after gating for Content API responses. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
ghost/core/core/server/api/endpoints/utils/serializers/output/utils/preview-rendering.js
Show resolved
Hide resolved
ghost/core/core/server/api/endpoints/utils/serializers/output/utils/preview-rendering.js
Outdated
Show resolved
Hide resolved
ghost/core/test/unit/api/canary/utils/serializers/output/utils/preview-rendering.test.js
Show resolved
Hide resolved
EvanHahn
left a comment
EvanHahn
left a comment
There was a problem hiding this comment.
LGTM. Some non-blocking comments.
| _buildTransistorPlaceholder() | ||
| ); | ||
|
|
||
| if (attrs.html && Object.prototype.hasOwnProperty.call(attrs, 'plaintext')) { |
| assert.notEqual(attrs.plaintext, 'old plaintext'); | ||
| assert.notEqual(attrs.excerpt, 'old excerpt'); |
There was a problem hiding this comment.
nit: Could we test what we update these to?
|
|
||
| if (attrs.html && attrs.html.includes('data-kg-transistor-embed')) { | ||
| attrs.html = attrs.html.replace( | ||
| /<iframe[^>]*data-kg-transistor-embed[^>]*><\/iframe><script>[\s\S]*?<\/script>(?:<noscript>[\s\S]*?<\/noscript>)?/g, |
There was a problem hiding this comment.
thought: on one hand, this feels brittle. If the HTML structure ever changes, we might fail to do the replacement. For example, if two <script> tags are placed, or we decide to put <noscript> first, this will break. On the other hand, tests cover a lot of this, so maybe it's not an issue.
Feel free to ignore, but I think this could be made better by by parsing the HTML somehow.
nit: we could make this regex more reliable with a few small changes:
- allow whitespace inside
<iframe>and between tags - allow attributes for the
<script>tag (e.g.,type="text/javascript") - make it case insensitive
Possible improvement (I didn't test this):
| /<iframe[^>]*data-kg-transistor-embed[^>]*><\/iframe><script>[\s\S]*?<\/script>(?:<noscript>[\s\S]*?<\/noscript>)?/g, | |
| /<iframe[^>]*data-kg-transistor-embed[^>]*>\s*<\/iframe>\s*<script[^>]*>[\s\S]*?<\/script>\s*(?:<noscript>[\s\S]*?<\/noscript>)?/gi, |
| const htmlToPlaintext = require('@tryghost/html-to-plaintext'); | ||
| const localUtils = require('../../../index'); | ||
|
|
||
| function _buildTransistorPlaceholder() { |
There was a problem hiding this comment.
nit: Why isn't this just a hard-coded string constant?
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@ghost/core/core/server/api/endpoints/utils/serializers/output/utils/text-attrs.js`:
- Around line 8-14: The checks that gate recomputing derived text use truthiness
of attrs.html so an empty string leaves plaintext/excerpt stale; change those
guards to test for presence (e.g., Object.hasOwn(attrs, 'html') or attrs.html
!== undefined) so updates run when html is an empty string. Specifically, update
the first conditional that references Object.hasOwn(attrs, 'plaintext') &&
attrs.html to use Object.hasOwn(attrs, 'html') (or attrs.html !== undefined) and
update the second block so the computed plaintext uses
htmlToPlaintext.excerpt(attrs.html) when attrs.html is present (including the
empty string) rather than only when truthy.
In
`@ghost/core/test/unit/api/canary/utils/serializers/output/utils/preview-rendering.test.js`:
- Around line 129-130: The test dereferences matches.length after calling
attrs.html.match which can return null; update the assertion to guard against a
null match before checking length (e.g., coalesce matches to an empty array or
assert matches is truthy) so the test fails with an assertion instead of
throwing a TypeError; locate the call to attrs.html.match and the subsequent use
of matches.length and change the assertion to use a null-safe length check (for
example use (matches || []).length or assert.ok(matches && matches.length ===
2)).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: eafb22d2-9add-4c04-9bc4-3d1c75ab4ed2
📒 Files selected for processing (4)
ghost/core/core/server/api/endpoints/utils/serializers/output/utils/post-gating.jsghost/core/core/server/api/endpoints/utils/serializers/output/utils/preview-rendering.jsghost/core/core/server/api/endpoints/utils/serializers/output/utils/text-attrs.jsghost/core/test/unit/api/canary/utils/serializers/output/utils/preview-rendering.test.js
✅ Files skipped from review due to trivial changes (1)
- ghost/core/core/server/api/endpoints/utils/serializers/output/utils/post-gating.js
🚧 Files skipped from review as they are similar to previous changes (1)
- ghost/core/core/server/api/endpoints/utils/serializers/output/utils/preview-rendering.js
| if (Object.hasOwn(attrs, 'plaintext') && attrs.html) { | ||
| attrs.plaintext = htmlToPlaintext.excerpt(attrs.html); | ||
| } | ||
|
|
||
| if (!attrs.custom_excerpt && Object.hasOwn(attrs, 'excerpt')) { | ||
| const plaintext = attrs.plaintext || htmlToPlaintext.excerpt(attrs.html); | ||
| attrs.excerpt = plaintext.substring(0, 500); |
There was a problem hiding this comment.
Recompute derived text when attrs.html is '' (empty), not only truthy.
On Line 8, && attrs.html skips updates when HTML becomes an empty string, so plaintext/excerpt can stay stale after content replacement/removal.
💡 Proposed fix
function updateTextAttrs(attrs) {
- if (Object.hasOwn(attrs, 'plaintext') && attrs.html) {
+ if (Object.hasOwn(attrs, 'plaintext') && typeof attrs.html === 'string') {
attrs.plaintext = htmlToPlaintext.excerpt(attrs.html);
}
if (!attrs.custom_excerpt && Object.hasOwn(attrs, 'excerpt')) {
- const plaintext = attrs.plaintext || htmlToPlaintext.excerpt(attrs.html);
+ const plaintext = Object.hasOwn(attrs, 'plaintext') ? attrs.plaintext : htmlToPlaintext.excerpt(attrs.html || '');
attrs.excerpt = plaintext.substring(0, 500);
}
}📝 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.
| if (Object.hasOwn(attrs, 'plaintext') && attrs.html) { | |
| attrs.plaintext = htmlToPlaintext.excerpt(attrs.html); | |
| } | |
| if (!attrs.custom_excerpt && Object.hasOwn(attrs, 'excerpt')) { | |
| const plaintext = attrs.plaintext || htmlToPlaintext.excerpt(attrs.html); | |
| attrs.excerpt = plaintext.substring(0, 500); | |
| if (Object.hasOwn(attrs, 'plaintext') && typeof attrs.html === 'string') { | |
| attrs.plaintext = htmlToPlaintext.excerpt(attrs.html); | |
| } | |
| if (!attrs.custom_excerpt && Object.hasOwn(attrs, 'excerpt')) { | |
| const plaintext = Object.hasOwn(attrs, 'plaintext') ? attrs.plaintext : htmlToPlaintext.excerpt(attrs.html || ''); | |
| attrs.excerpt = plaintext.substring(0, 500); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@ghost/core/core/server/api/endpoints/utils/serializers/output/utils/text-attrs.js`
around lines 8 - 14, The checks that gate recomputing derived text use
truthiness of attrs.html so an empty string leaves plaintext/excerpt stale;
change those guards to test for presence (e.g., Object.hasOwn(attrs, 'html') or
attrs.html !== undefined) so updates run when html is an empty string.
Specifically, update the first conditional that references Object.hasOwn(attrs,
'plaintext') && attrs.html to use Object.hasOwn(attrs, 'html') (or attrs.html
!== undefined) and update the second block so the computed plaintext uses
htmlToPlaintext.excerpt(attrs.html) when attrs.html is present (including the
empty string) rather than only when truthy.
| const matches = attrs.html.match(/kg-transistor-placeholder/g); | ||
| assert.equal(matches.length, 2); |
There was a problem hiding this comment.
Guard against null from String.prototype.match before dereferencing length.
On Line 130, matches.length throws a TypeError if no match occurs, which obscures the intended assertion failure.
💡 Proposed fix
const matches = attrs.html.match(/kg-transistor-placeholder/g);
- assert.equal(matches.length, 2);
+ assert.equal(matches?.length ?? 0, 2);📝 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.
| const matches = attrs.html.match(/kg-transistor-placeholder/g); | |
| assert.equal(matches.length, 2); | |
| const matches = attrs.html.match(/kg-transistor-placeholder/g); | |
| assert.equal(matches?.length ?? 0, 2); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@ghost/core/test/unit/api/canary/utils/serializers/output/utils/preview-rendering.test.js`
around lines 129 - 130, The test dereferences matches.length after calling
attrs.html.match which can return null; update the assertion to guard against a
null match before checking length (e.g., coalesce matches to an empty array or
assert matches is truthy) so the test fails with an assertion instead of
throwing a TypeError; locate the call to attrs.html.match and the subsequent use
of matches.length and change the assertion to use a null-safe length check (for
example use (matches || []).length or assert.ok(matches && matches.length ===
2)).
ref https://linear.app/ghost/issue/NY-1154/ - existing preview substitution was bolted-on to visibility gating, which was inappropriate; it is now its own step The intent of this change is to provide a pattern for subtituting content in the Preview pane in Admin > Post Editor. In this case, we substitute a user-specific iframe with a placeholder rather than using a real user's uuid and display a real user's content.
ref https://linear.app/ghost/issue/NY-1154/
The intent of this change is to provide a pattern for subtituting content in the Preview pane in Admin > Post Editor. In this case, we substitute a user-specific iframe with a placeholder rather than using a real user's uuid and display a real user's content.