🎨 Added picture element support to img card#26397
Conversation
jloh
added
affects:editor
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThis change implements support for rendering modern image formats (AVIF and WebP) in the editor through HTML5 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 review |
✅ Actions performedReview triggered.
|
![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
🤖 Fix all issues with AI agents
In `@ghost/core/core/server/services/koenig/node-renderers/image-renderer.js`:
- Around line 115-146: The current code always creates a picture element and
appends img even when no <source> children are added; change the logic so
picture is only used when at least one valid source was produced by
MODERN_IMAGE_FORMATS. Do this by either deferring creation of picture until the
first successful formattedSrcset (check imageTransform.canTransformToFormat and
getSrcsetAttribute inside the loop) or by tracking a boolean (e.g.,
sourcesAdded) while iterating; after the loop, if sourcesAdded is false, leave
picture as null and do not append img to picture (keep img as the top-level
element), otherwise append img to picture as now. Ensure you reference the
existing symbols picture, MODERN_IMAGE_FORMATS,
imageTransform.canTransformToFormat, getSrcsetAttribute and img when making the
change.
🧹 Nitpick comments (5)
ghost/core/test/unit/server/services/koenig/node-renderers/image-renderer.test.js (1)
157-171: Good coverage of the opt-out paths.Tests for labs-flag-off and GIF scenarios correctly assert
<picture>is absent and<img>is still present. Consider adding a snapshot assertion for the flag-off case to ensure the full<img>output (with srcset, sizes, etc.) hasn't regressed — though the existing default-data snapshot test from pre-picture days would catch regressions too if run.ghost/core/core/server/services/koenig/render-utils/srcset-attribute.js (2)
23-39: Consider deduplicating the format URL construction.Both the
srcsetWidth === widthandsrcsetWidth <= widthbranches construct the identical format URL string. The only difference in the non-format case is usingsrcdirectly (exact match) vs. the sized path. This could be simplified:♻️ Proposed refactor to reduce duplication
srcsetWidths.forEach((srcsetWidth) => { - if (srcsetWidth === width) { - // use original image path if width matches exactly (avoids 302s from size->original) - // unless a specific output format was requested - if (format) { - srcs.push(`${imagesPath}/size/w${srcsetWidth}/format/${format}/${filename} ${srcsetWidth}w`); - } else { - srcs.push(`${src} ${srcsetWidth}w`); - } - } else if (srcsetWidth <= width) { - // avoid creating srcset sizes larger than intrinsic image width - if (format) { - srcs.push(`${imagesPath}/size/w${srcsetWidth}/format/${format}/${filename} ${srcsetWidth}w`); - } else { - srcs.push(`${imagesPath}/size/w${srcsetWidth}/${filename} ${srcsetWidth}w`); - } + if (srcsetWidth > width) { + return; + } + + if (format) { + // format-specific paths always go through the size/format endpoint + srcs.push(`${imagesPath}/size/w${srcsetWidth}/format/${format}/${filename} ${srcsetWidth}w`); + } else if (srcsetWidth === width) { + // use original image path if width matches exactly (avoids 302s from size->original) + srcs.push(`${src} ${srcsetWidth}w`); + } else { + srcs.push(`${imagesPath}/size/w${srcsetWidth}/${filename} ${srcsetWidth}w`); } });
48-58: Unsplashfmparameter persists across loop iterations.The
unsplashUrlobject is mutated inside theforEachloop. Whenformatis truthy,fmis set on the first iteration and stays set for subsequent ones — which happens to be correct sinceformatis constant within a single call. However, ifformatis ever falsy but the source URL already contains anfmparameter, that pre-existingfmwould leak into all srcset entries. This is unlikely with Unsplash URLs today, but for defensive correctness you coulddeletethe param whenformatis falsy.🛡️ Defensive fix
unsplashUrl.searchParams.set('w', srcsetWidth); if (format) { unsplashUrl.searchParams.set('fm', format); + } else { + unsplashUrl.searchParams.delete('fm'); }ghost/core/core/server/services/koenig/node-renderers/image-renderer.js (2)
12-20:isAnimatedImageis reallyisGif— name may mislead future maintainers.This function returns
truefor all.giffiles (including non-animated ones) andfalsefor other animated formats (APNG, animated WebP). The conservative "treat all GIFs as potentially animated" approach is sensible, but the name suggests it detects animation. A name likeisGifImageorisPotentiallyAnimatedImagewould be more precise.
1-2: Top-levelrequire('@tryghost/image-transform')— eager load is fine here.Previously,
image-transformwas only lazily loaded inlexical.js'scanTransformImageclosure. Importing it eagerly at module level means it loads when the renderer is first required, even ifpictureImageFormatsis disabled. This is acceptable since the renderer is on the critical path for content rendering anyway, but worth noting if the module has heavy initialization.
| if (shouldRenderPicture) { | ||
| picture = document.createElement('picture'); | ||
|
|
||
| MODERN_IMAGE_FORMATS.forEach((format) => { | ||
| if (!imageTransform.canTransformToFormat(format)) { | ||
| return; | ||
| } | ||
|
|
||
| const formattedSrcset = getSrcsetAttribute({ | ||
| src: node.src, | ||
| width: node.width, | ||
| options, | ||
| format | ||
| }); | ||
|
|
||
| if (!formattedSrcset) { | ||
| return; | ||
| } | ||
|
|
||
| const source = document.createElement('source'); | ||
| source.setAttribute('srcset', formattedSrcset); | ||
| source.setAttribute('type', `image/${format}`); | ||
|
|
||
| if (sizes) { | ||
| source.setAttribute('sizes', sizes); | ||
| } | ||
|
|
||
| picture.appendChild(source); | ||
| }); | ||
|
|
||
| picture.appendChild(img); | ||
| } |
There was a problem hiding this comment.
Clean implementation of the <picture> element generation.
The format iteration with per-format capability checks (canTransformToFormat), format-specific srcset generation, and graceful skipping when a format isn't supported is well-structured. The sizes attribute is correctly shared between <source> and <img> elements.
One minor note: if both canTransformToFormat checks fail (neither avif nor webp is supported), picture will be created but contain only the <img> child (line 145) with no <source> elements. This produces a valid but unnecessary <picture> wrapper. You might want to skip appending img to picture (and leave picture = null) if no sources were added.
🐛 Proposed fix to avoid empty `` wrapper
+ // only use picture element if we actually added format sources
+ if (!picture.querySelector('source')) {
+ picture = null;
+ } else {
picture.appendChild(img);
+ }Or alternatively, track whether any sources were added:
if (shouldRenderPicture) {
picture = document.createElement('picture');
+ let hasFormatSources = false;
MODERN_IMAGE_FORMATS.forEach((format) => {
// ...existing code...
picture.appendChild(source);
+ hasFormatSources = true;
});
- picture.appendChild(img);
+ if (hasFormatSources) {
+ picture.appendChild(img);
+ } else {
+ picture = null;
+ }
}🤖 Prompt for AI Agents
In `@ghost/core/core/server/services/koenig/node-renderers/image-renderer.js`
around lines 115 - 146, The current code always creates a picture element and
appends img even when no <source> children are added; change the logic so
picture is only used when at least one valid source was produced by
MODERN_IMAGE_FORMATS. Do this by either deferring creation of picture until the
first successful formattedSrcset (check imageTransform.canTransformToFormat and
getSrcsetAttribute inside the loop) or by tracking a boolean (e.g.,
sourcesAdded) while iterating; after the loop, if sourcesAdded is false, leave
picture as null and do not append img to picture (keep img as the top-level
element), otherwise append img to picture as now. Ensure you reference the
existing symbols picture, MODERN_IMAGE_FORMATS,
imageTransform.canTransformToFormat, getSrcsetAttribute and img when making the
change.
c9f1619 to
d39caed
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
ghost/core/core/server/services/koenig/node-renderers/image-renderer.js (1)
115-152:sourcesAddedguard correctly resolves the previously flagged empty<picture>wrapper concern.The
picture = nullfallback on line 150 ensures the<picture>element is only used when at least one<source>was successfully produced, which was the exact fix proposed in the prior review.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ghost/core/core/server/services/koenig/node-renderers/image-renderer.js` around lines 115 - 152, The previous empty <picture> wrapper issue is fixed by tracking whether any <source> was appended via the sourcesAdded flag inside the shouldRenderPicture branch; ensure the loop over MODERN_IMAGE_FORMATS uses imageTransform.canTransformToFormat(format) and getSrcsetAttribute(...) to decide to create a <source>, set its srcset/type/sizes, append it to picture and set sourcesAdded = true, then only append img to picture if sourcesAdded is true and otherwise set picture = null so no empty <picture> is returned.
🧹 Nitpick comments (1)
ghost/core/core/server/services/koenig/node-renderers/image-renderer.js (1)
12-16: RenameisAnimatedImageto accurately reflect its GIF-only scope.The function only checks for
.gifand its name implies broader animated-image detection (animated WebP and animated PNG are not caught). If an animated.webpwere ever used as a content image, it would be passed through the picture-element path and potentially re-encoded into AVIF/WebP — possibly stripping animation. Renaming toisGiforisGifImageremoves the ambiguity and makes the intent clear.♻️ Proposed rename
-function isAnimatedImage(url = '') { +function isGifImage(url = '') {- !isAnimatedImage(node.src) && + !isGifImage(node.src) &&🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ghost/core/core/server/services/koenig/node-renderers/image-renderer.js` around lines 12 - 16, Rename the function isAnimatedImage to a clear GIF-only name (e.g., isGifImage or isGif) to reflect that it only checks for '.gif'; update the function declaration and every reference/call site (including exports and imports) from isAnimatedImage to the new name, and run/update any tests or usages that rely on the old identifier so behavior remains unchanged. Ensure the implementation (parsing URL and comparing extension === '.gif') stays the same and adjust any JSDoc/comments to match the new name.
🤖 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/services/koenig/node-renderers/image-renderer.js`:
- Around line 93-99: The sizes string currently set in image-renderer.js
(variable sizes in the node renderer) lacks an explicit fallback for narrow
viewports; update the assignments that set sizes (the branches checking
node.cardWidth and node.width in the image renderer) to append an explicit
fallback of 100vw so the values become e.g. "(min-width: 720px) 720px, 100vw"
and "(min-width: 1200px) 1200px, 100vw"; also ensure the default/regular branch
sets sizes to "100vw" so every code path for sizes (including when
node.cardWidth is undefined/regular) has an explicit fallback.
---
Duplicate comments:
In `@ghost/core/core/server/services/koenig/node-renderers/image-renderer.js`:
- Around line 115-152: The previous empty <picture> wrapper issue is fixed by
tracking whether any <source> was appended via the sourcesAdded flag inside the
shouldRenderPicture branch; ensure the loop over MODERN_IMAGE_FORMATS uses
imageTransform.canTransformToFormat(format) and getSrcsetAttribute(...) to
decide to create a <source>, set its srcset/type/sizes, append it to picture and
set sourcesAdded = true, then only append img to picture if sourcesAdded is true
and otherwise set picture = null so no empty <picture> is returned.
---
Nitpick comments:
In `@ghost/core/core/server/services/koenig/node-renderers/image-renderer.js`:
- Around line 12-16: Rename the function isAnimatedImage to a clear GIF-only
name (e.g., isGifImage or isGif) to reflect that it only checks for '.gif';
update the function declaration and every reference/call site (including exports
and imports) from isAnimatedImage to the new name, and run/update any tests or
usages that rely on the old identifier so behavior remains unchanged. Ensure the
implementation (parsing URL and comparing extension === '.gif') stays the same
and adjust any JSDoc/comments to match the new name.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
ghost/core/test/e2e-api/admin/__snapshots__/config.test.js.snapis excluded by!**/*.snap
📒 Files selected for processing (4)
ghost/core/core/server/lib/lexical.jsghost/core/core/server/services/koenig/node-renderers/image-renderer.jsghost/core/core/server/services/koenig/render-utils/srcset-attribute.jsghost/core/core/shared/labs.js
🚧 Files skipped from review as they are similar to previous changes (3)
- ghost/core/core/shared/labs.js
- ghost/core/core/server/lib/lexical.js
- ghost/core/core/server/services/koenig/render-utils/srcset-attribute.js
| if (!node.cardWidth || node.cardWidth === 'regular') { | ||
| img.setAttribute('sizes', '(min-width: 720px) 720px'); | ||
| sizes = '(min-width: 720px) 720px'; | ||
| } | ||
|
|
||
| if (node.cardWidth === 'wide' && node.width >= 1200) { | ||
| img.setAttribute('sizes', '(min-width: 1200px) 1200px'); | ||
| sizes = '(min-width: 1200px) 1200px'; | ||
| } |
There was a problem hiding this comment.
sizes attribute is missing an explicit fallback for narrow viewports.
(min-width: 720px) 720px (and the wide-card variant) has no trailing fallback value. When no media condition matches (viewport < 720 px), the browser falls back to the intrinsic pixel dimensions of the image — which may not be the best fit for all devices. The browser-assumed 100vw default should be stated explicitly, which also keeps the markup self-documenting and avoids HTML-validator warnings.
🛠️ Proposed fix
- sizes = '(min-width: 720px) 720px';
+ sizes = '(min-width: 720px) 720px, 100vw';- sizes = '(min-width: 1200px) 1200px';
+ sizes = '(min-width: 1200px) 1200px, 100vw';📝 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 (!node.cardWidth || node.cardWidth === 'regular') { | |
| img.setAttribute('sizes', '(min-width: 720px) 720px'); | |
| sizes = '(min-width: 720px) 720px'; | |
| } | |
| if (node.cardWidth === 'wide' && node.width >= 1200) { | |
| img.setAttribute('sizes', '(min-width: 1200px) 1200px'); | |
| sizes = '(min-width: 1200px) 1200px'; | |
| } | |
| if (!node.cardWidth || node.cardWidth === 'regular') { | |
| sizes = '(min-width: 720px) 720px, 100vw'; | |
| } | |
| if (node.cardWidth === 'wide' && node.width >= 1200) { | |
| sizes = '(min-width: 1200px) 1200px, 100vw'; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ghost/core/core/server/services/koenig/node-renderers/image-renderer.js`
around lines 93 - 99, The sizes string currently set in image-renderer.js
(variable sizes in the node renderer) lacks an explicit fallback for narrow
viewports; update the assignments that set sizes (the branches checking
node.cardWidth and node.width in the image renderer) to append an explicit
fallback of 100vw so the values become e.g. "(min-width: 720px) 720px, 100vw"
and "(min-width: 1200px) 1200px, 100vw"; also ensure the default/regular branch
sets sizes to "100vw" so every code path for sizes (including when
node.cardWidth is undefined/regular) has an explicit fallback.
There was a problem hiding this comment.
About explicit 100vw fallback in sizes. This is technically correct from a spec-purity standpoint, but it's a pre-existing pattern. The current code on main already sets sizes="(min-width: 720px) 720px" without a fallback:
img.setAttribute('sizes', '(min-width: 720px) 720px');
The browser default when no media condition matches is 100vw, which is exactly the right behavior here. Changing this would affect existing (non-picture) output too, so it's out of scope for this PR and not something to hold up on.
There was a problem hiding this comment.
@vershwal, you're absolutely right—this is a pre-existing pattern and falls outside the scope of this PR. Since the browser defaults to 100vw when no media condition matches, the current behavior is functionally correct. Thank you for the clarification!
🧠 Learnings used
Learnt from: rob-ghost
Repo: TryGhost/Ghost PR: 25791
File: ghost/core/core/server/api/endpoints/member-comment-ban.js:64-68
Timestamp: 2026-01-08T10:26:38.700Z
Learning: In the Ghost API, endpoints rely on the serialization layer to prepare frame.data[docName] as a non-empty array before query() executes. Endpoints access frame.data[docName][0] directly (e.g., frame.data.comment_bans[0], frame.data.members[0], frame.data.posts[0]) without per-endpoint validation. This pattern is common across API endpoints. When maintaining or creating endpoints, avoid duplicating validation for frame.data[docName] and ensure the serializer guarantees the shape and non-emptiness. If you add a new endpoint that uses this frame.data[docName], follow the same assumption and avoid redundant checks unless there's a documented exception.
Learnt from: rob-ghost
Repo: TryGhost/Ghost PR: 26219
File: ghost/core/test/e2e-api/members-comments/comments.test.js:939-983
Timestamp: 2026-02-04T15:58:09.124Z
Learning: In Ghost core tests and code that interact with the Ghost comments API, count.replies is a backward-compatible alias for count.total_replies (all descendants via parent_id) and does not represent direct replies. The new field count.direct_replies returns tree-native direct reply counts. Reviewers should verify any code paths, tests, or API surface areas that rely on count.replies are preserved for compatibility, and consider updating or adding tests to cover count.direct_replies for direct counts. When updating or adding tests, ensure behavior is documented and that any assertions reflect the distinction between total (including descendants) and direct reply counts to avoid regressions in API consumer expectations.
d39caed to
59ff917
Compare
no ref - The picture element is widely supported now and lets the browser choose the best supported format for the browser in question - Testing to see how often browsers will actually pick a different image format when using the new card
jloh
force-pushed
the
jloh/picture-element
branch
from
59ff917 to
aeaa5b7
Compare
|
You have run out of free Bugbot PR reviews for this billing cycle. This will reset on April 3. To receive reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial. |
rob-ghost
left a comment
rob-ghost
left a comment
There was a problem hiding this comment.
Question: We have a gallery-renderer.js which produces an <img> with a srcset using the same setSrcsetAttribute function, but that renderer does not get the <picture> wrapping.
Is this intentional?
Nothing blocking! I think this can move forward regardless of the comments 🚀 🖌️
| function isAnimatedImage(url = '') { | ||
| try { | ||
| const parsedUrl = new URL(url, 'http://localhost'); | ||
| const extension = path.extname(parsedUrl.pathname).toLowerCase(); | ||
| return extension === '.gif'; | ||
| } catch (err) { | ||
| return false; | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Nitpick: If this is just checking for a .gif extension, we could use a simple regex instead of constructing a URL:
function isAnimatedImage(url) {
return /\.gif([?#]|$)/i.test(url); }Unless this function is doubling up as a URL validation function? In which case, I think we're being overly defensive here:
function isAnimatedImage(url) {
const extension = path.extname(new URL(url, 'http://localhost').pathname);
return extension.toLowerCase() === '.gif';
}| MODERN_IMAGE_FORMATS.forEach((format) => { | ||
| if (!imageTransform.canTransformToFormat(format)) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
Nitpick: Instead of doing this work in the body of a loop, we could find the right format and then do it outside:
const format = MODERN_IMAGE_FORMATS.find(imageTransform.canTransformToFormat)
if (format) {
// Append source
}This would flatten things a bit.
| const shouldRenderPicture = Boolean( | ||
| options.feature?.pictureImageFormats && | ||
| img.getAttribute('srcset') && | ||
| !isAnimatedImage(node.src) && |
There was a problem hiding this comment.
Question: Do we support animated WebP uploads? Since we're only checking for .gif we could be assigning an animated WebP image to <picture> which would then render it as non-animated.
| const imgAttributes = { | ||
| src: node.src, | ||
| width: node.width, | ||
| height: node.height | ||
| }; |
There was a problem hiding this comment.
Nitpick: This is only used inside the if (options.target !== 'email') { block, but its only used inside the block with setSrcsetAttribute(img, imgAttributes, options);.
It doesn't matter, might be a leftover from a previous iteration.
| if (format) { | ||
| unsplashUrl.searchParams.set('fm', format); | ||
| } |
There was a problem hiding this comment.
Nitpick: This is a bit of a rabbit hole, but we only pass format for <picture> nodes, and there's a guard for those isContentImage(node.src, options.siteUrl, options.imageBaseUrl) which only applies to uploaded images.
isContentImage return false for unsplash images, meaning this particular branch is unreachable, unless I'm mistaken!
ref #26397 - The picture element has now been released but didn't have a Private UI toggle which this adds
ref #26397 - The picture element has now been released but didn't have a Private UI toggle which this adds
3 participants
no ref