Use preview downscaling in fewer places#9678
Merged
AustinMroz merged 6 commits intomainfrom Mar 9, 2026
Merged
Conversation
dosubot
bot
added
the
size:S
Contributor
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughThis change removes cloud preview parameter injection from the image URL construction pipeline and introduces a dual thumbnail/preview URL approach to the AssetItem schema. The appendCloudResParam logic is stripped from multiple files, while thumbnail_url and preview_url fields replace the single preview_url field across mappers, utilities, and components. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 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 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 |
🎨 Storybook: ✅ Built — View Storybook |
🎭 Playwright: ✅ 556 passed, 0 failed · 2 flaky📊 Browser Reports
|
📦 Bundle: 4.57 MB gzip 🔴 +11 BDetailsSummary
Category Glance App Entry Points — 28.9 kB (baseline 28.9 kB) • ⚪ 0 BMain entry bundles and manifests
Status: 1 added / 1 removed Graph Workspace — 967 kB (baseline 967 kB) • ⚪ 0 BGraph editor runtime, canvas, workflow orchestration
Status: 1 added / 1 removed Views & Navigation — 72.4 kB (baseline 72.4 kB) • ⚪ 0 BTop-level views, pages, and routed surfaces
Status: 9 added / 9 removed Panels & Settings — 436 kB (baseline 436 kB) • ⚪ 0 BConfiguration panels, inspectors, and settings screens
Status: 10 added / 10 removed User & Accounts — 16.1 kB (baseline 16.1 kB) • ⚪ 0 BAuthentication, profile, and account management bundles
Status: 5 added / 5 removed Editors & Dialogs — 77.5 kB (baseline 77.5 kB) • ⚪ 0 BModals, dialogs, drawers, and in-app editors
Status: 2 added / 2 removed UI Components — 56.5 kB (baseline 56.5 kB) • ⚪ 0 BReusable component library chunks
Status: 5 added / 5 removed Data & Services — 2.77 MB (baseline 2.77 MB) • 🔴 +88 BStores, services, APIs, and repositories
Status: 14 added / 14 removed Utilities & Hooks — 56.8 kB (baseline 56.8 kB) • ⚪ 0 BHelpers, composables, and utility bundles
Status: 11 added / 11 removed Vendor & Third-Party — 8.88 MB (baseline 8.88 MB) • ⚪ 0 BExternal libraries and shared vendor chunks
Other — 8.04 MB (baseline 8.04 MB) • 🟢 -77 BBundles that do not match a named category
Status: 50 added / 50 removed |
dosubot
bot
added
size:M
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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)
src/platform/assets/components/MediaImageTop.vue (1)
6-11:⚠️ Potential issue | 🟠 MajorMismatch between preloaded URL and rendered img src.
useImagepreloads the cloud-resized URL (appendCloudResUrl(asset.src)), but the actual<img>tag at line 8 uses the rawasset.src. This causes:
- Two separate network requests (resized preview + full original)
- The full-size image still loads in cloud mode, defeating the optimization
- Potential error state inconsistencies if one URL fails but not the other
The
<img>src should use the same transformed URL asuseImage.🐛 Proposed fix
+<script setup lang="ts"> +import { computed } from 'vue' +import { useImage, whenever } from '@vueuse/core' + +import { appendCloudResUrl } from '@/platform/distribution/cloudPreviewUtil' +import type { AssetMeta } from '../schemas/mediaAssetSchema' + +const { asset } = defineProps<{ + asset: AssetMeta +}>() + +const emit = defineEmits<{ + 'image-loaded': [width: number, height: number] + view: [] +}>() + +const imageSrc = computed(() => + asset.src ? appendCloudResUrl(asset.src) : '' +) + +const { state, error, isReady } = useImage({ + src: imageSrc.value, + alt: asset.display_name || asset.name +}) +</script>And in the template:
<img v-if="!error" - :src="asset.src" + :src="imageSrc" :alt="asset.display_name || asset.name"Also applies to: 36-39
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/platform/assets/components/MediaImageTop.vue` around lines 6 - 11, The template's <img> is using the raw asset.src while useImage preloads the transformed URL via appendCloudResUrl(asset.src), causing duplicate requests and mismatch; update the <img> src binding to use the same transformed URL produced by useImage/appendCloudResUrl so the rendered image and preloaded image match (refer to useImage, appendCloudResUrl, and the <img> element currently bound to asset.src), and ensure the same change is applied for the second instance around lines 36-39 so error state and caching are consistent.
🧹 Nitpick comments (2)
src/platform/distribution/cloudPreviewUtil.ts (2)
21-24: Note: No media type check unlikeappendCloudResParam.
appendCloudResParamskips non-image files, butappendCloudResUrlappendsres=512unconditionally. This is fine since the function is only used in MediaImageTop.vue (image-specific), but the difference in behavior should be documented.📝 Optional: Add clarifying JSDoc
+/** + * Appends `&res=512` to the given URL when in cloud mode. + * Unlike appendCloudResParam, this does not check media type - + * caller is responsible for ensuring this is only used for images. + */ export function appendCloudResUrl(url: string): string {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/platform/distribution/cloudPreviewUtil.ts` around lines 21 - 24, The function appendCloudResUrl currently appends '&res=512' unconditionally while appendCloudResParam skips non-image files; update appendCloudResUrl to either perform the same image-type guard as appendCloudResParam or add a clarifying JSDoc comment stating this function is image-only and intended to be used only by image components (e.g., MediaImageTop.vue); reference appendCloudResUrl and appendCloudResParam and ensure the chosen fix documents or enforces that non-image URLs are not modified.
21-24: Consider more robust query string handling.
appendCloudResUrlunconditionally concatenates&res=512, which assumes the URL already contains query parameters. If a URL without query params is ever passed (e.g.,/view), this produces an invalid URL (/view&res=512instead of/view?res=512).Current usage with mapper-constructed URLs is safe, but this could break if the function is reused elsewhere.
♻️ Suggested improvement for robustness
export function appendCloudResUrl(url: string): string { if (!isCloud) return url - return url + '&res=512' + const separator = url.includes('?') ? '&' : '?' + return url + separator + 'res=512' }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/platform/distribution/cloudPreviewUtil.ts` around lines 21 - 24, appendCloudResUrl currently appends "&res=512" unconditionally which breaks URLs without query strings; update the function (and usage around isCloud) to detect whether the input url already has query parameters and prepend either "?" or "&" accordingly (or use the URL/URLSearchParams API to set/replace the "res" param robustly), ensuring existing res params are replaced if present.
🤖 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 `@src/platform/assets/components/MediaImageTop.vue`:
- Around line 6-11: The template's <img> is using the raw asset.src while
useImage preloads the transformed URL via appendCloudResUrl(asset.src), causing
duplicate requests and mismatch; update the <img> src binding to use the same
transformed URL produced by useImage/appendCloudResUrl so the rendered image and
preloaded image match (refer to useImage, appendCloudResUrl, and the <img>
element currently bound to asset.src), and ensure the same change is applied for
the second instance around lines 36-39 so error state and caching are
consistent.
---
Nitpick comments:
In `@src/platform/distribution/cloudPreviewUtil.ts`:
- Around line 21-24: The function appendCloudResUrl currently appends '&res=512'
unconditionally while appendCloudResParam skips non-image files; update
appendCloudResUrl to either perform the same image-type guard as
appendCloudResParam or add a clarifying JSDoc comment stating this function is
image-only and intended to be used only by image components (e.g.,
MediaImageTop.vue); reference appendCloudResUrl and appendCloudResParam and
ensure the chosen fix documents or enforces that non-image URLs are not
modified.
- Around line 21-24: appendCloudResUrl currently appends "&res=512"
unconditionally which breaks URLs without query strings; update the function
(and usage around isCloud) to detect whether the input url already has query
parameters and prepend either "?" or "&" accordingly (or use the
URL/URLSearchParams API to set/replace the "res" param robustly), ensuring
existing res params are replaced if present.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c52710c7-6c9c-4967-8bf9-f63286c5b734
📒 Files selected for processing (6)
src/extensions/core/imageCompare.tssrc/platform/assets/components/MediaImageTop.vuesrc/platform/assets/composables/media/assetMappers.tssrc/platform/assets/utils/outputAssetUtil.tssrc/platform/distribution/cloudPreviewUtil.tssrc/stores/nodeOutputStore.ts
💤 Files with no reviewable changes (2)
- src/extensions/core/imageCompare.ts
- src/stores/nodeOutputStore.ts
⚡ Performance Report
Raw data{
"timestamp": "2026-03-09T23:00:38.263Z",
"gitSha": "793f54df2fd1f6c9c2f7cb0d0e665734aaa7c73c",
"branch": "austin/real-previews",
"measurements": [
{
"name": "canvas-idle",
"durationMs": 2040.616,
"styleRecalcs": 123,
"styleRecalcDurationMs": 17.286999999999995,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 375.082,
"heapDeltaBytes": -3617044
},
{
"name": "canvas-idle",
"durationMs": 2027.356999999995,
"styleRecalcs": 123,
"styleRecalcDurationMs": 20.333000000000002,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 389.56800000000004,
"heapDeltaBytes": -3199884
},
{
"name": "canvas-idle",
"durationMs": 2028.6729999999693,
"styleRecalcs": 124,
"styleRecalcDurationMs": 20.892,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 390.421,
"heapDeltaBytes": -3508016
},
{
"name": "canvas-mouse-sweep",
"durationMs": 2027.9050000000325,
"styleRecalcs": 183,
"styleRecalcDurationMs": 52.91700000000001,
"layouts": 12,
"layoutDurationMs": 3.6809999999999996,
"taskDurationMs": 957.115,
"heapDeltaBytes": -3103172
},
{
"name": "canvas-mouse-sweep",
"durationMs": 1882.1389999999951,
"styleRecalcs": 167,
"styleRecalcDurationMs": 48.132,
"layouts": 12,
"layoutDurationMs": 3.261,
"taskDurationMs": 841.754,
"heapDeltaBytes": -3052540
},
{
"name": "canvas-mouse-sweep",
"durationMs": 2022.8159999999775,
"styleRecalcs": 179,
"styleRecalcDurationMs": 48.119,
"layouts": 12,
"layoutDurationMs": 3.198,
"taskDurationMs": 967.8489999999999,
"heapDeltaBytes": -2926368
},
{
"name": "dom-widget-clipping",
"durationMs": 616.0060000000271,
"styleRecalcs": 44,
"styleRecalcDurationMs": 13.287,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 394.549,
"heapDeltaBytes": 7860096
},
{
"name": "dom-widget-clipping",
"durationMs": 566.4669999999887,
"styleRecalcs": 43,
"styleRecalcDurationMs": 13.171000000000001,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 349.47,
"heapDeltaBytes": 7065744
},
{
"name": "dom-widget-clipping",
"durationMs": 547.2990000000095,
"styleRecalcs": 39,
"styleRecalcDurationMs": 12.749,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 338.731,
"heapDeltaBytes": 6939128
},
{
"name": "subgraph-dom-widget-clipping",
"durationMs": 568.127000000004,
"styleRecalcs": 71,
"styleRecalcDurationMs": 13.934999999999997,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 385.53600000000006,
"heapDeltaBytes": 15237876
},
{
"name": "subgraph-dom-widget-clipping",
"durationMs": 594.4079999999872,
"styleRecalcs": 74,
"styleRecalcDurationMs": 15.376000000000001,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 414.257,
"heapDeltaBytes": -8707364
},
{
"name": "subgraph-dom-widget-clipping",
"durationMs": 613.6720000000082,
"styleRecalcs": 74,
"styleRecalcDurationMs": 14.987,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 409.6810000000001,
"heapDeltaBytes": -8885124
},
{
"name": "subgraph-idle",
"durationMs": 1993.423000000007,
"styleRecalcs": 120,
"styleRecalcDurationMs": 19.205,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 380.535,
"heapDeltaBytes": -2856368
},
{
"name": "subgraph-idle",
"durationMs": 2005.5570000000102,
"styleRecalcs": 121,
"styleRecalcDurationMs": 18.151,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 368.89900000000006,
"heapDeltaBytes": -3623564
},
{
"name": "subgraph-idle",
"durationMs": 1996.7920000000277,
"styleRecalcs": 120,
"styleRecalcDurationMs": 18.493999999999996,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 370.92,
"heapDeltaBytes": -3677524
},
{
"name": "subgraph-mouse-sweep",
"durationMs": 1979.9539999999638,
"styleRecalcs": 171,
"styleRecalcDurationMs": 51.98800000000001,
"layouts": 16,
"layoutDurationMs": 4.2459999999999996,
"taskDurationMs": 945.438,
"heapDeltaBytes": -5189940
},
{
"name": "subgraph-mouse-sweep",
"durationMs": 1679.5740000000023,
"styleRecalcs": 154,
"styleRecalcDurationMs": 43.598,
"layouts": 16,
"layoutDurationMs": 4.161,
"taskDurationMs": 712.453,
"heapDeltaBytes": -5206716
},
{
"name": "subgraph-mouse-sweep",
"durationMs": 2007.840999999985,
"styleRecalcs": 173,
"styleRecalcDurationMs": 53.652,
"layouts": 16,
"layoutDurationMs": 4.258000000000001,
"taskDurationMs": 964.959,
"heapDeltaBytes": -5142468
}
]
} |
dosubot
bot
added
size:S
DrJKL
reviewed
Mar 9, 2026
|
|
||
| export function appendCloudResUrl(url: string): string { | ||
| if (!isCloud) return url | ||
| return url + '&res=512' |
Contributor
There was a problem hiding this comment.
The URLSearchParams piece is a guard for when the asset already has a res set in the URL sent by the server
Collaborator
Author
There was a problem hiding this comment.
Appreciate the catch. I had hoped to make this a smaller PR than reworking the assetsSchema to pass both.
Will start on the nicer fix.
DrJKL
approved these changes
Mar 9, 2026
AustinMroz
added
needs-backport
github-actions bot
pushed a commit
that referenced
this pull request
Mar 9, 2026
Thumbnail downscaling is currently being used in more places than it should be. - Nodes which display images will display incorrect resolution indicators <img width="255" height="372" alt="image" src="https://github.com/user-attachments/assets/674790b6-04c8-4db0-84c2-2fa2dbaf123d" /> <img width="255" height="372" alt="image" src="https://github.com/user-attachments/assets/1dbe751b-7462-4408-9236-9446b005f5fc" /> This is particularly confusing with output nodes, which claim the output is not of the intended resolution - The "Download Image" and "Open Image" context menu actions will incorrectly download the downscaled thumbnail. - The assets panel will incorrectly display the thumbnail resolution as the resolution of the output - The lightbox (zoom) of an image will incorrectly display a downscaled thumbnail. This PR is a quick workaround to staunch the major problems - Nodes always display full previews. - Resolution downscaling is applied on the assert card, not on the assetItem itself - Due to implementation, this means that asset cards will still incorrectly show the resolution of the thumbnail instead of the size of the full image. --------- Co-authored-by: GitHub Action <action@github.com> Co-authored-by: Alexander Brown <drjkl@comfy.org>
github-actions bot
pushed a commit
that referenced
this pull request
Mar 9, 2026
Thumbnail downscaling is currently being used in more places than it should be. - Nodes which display images will display incorrect resolution indicators <img width="255" height="372" alt="image" src="https://github.com/user-attachments/assets/674790b6-04c8-4db0-84c2-2fa2dbaf123d" /> <img width="255" height="372" alt="image" src="https://github.com/user-attachments/assets/1dbe751b-7462-4408-9236-9446b005f5fc" /> This is particularly confusing with output nodes, which claim the output is not of the intended resolution - The "Download Image" and "Open Image" context menu actions will incorrectly download the downscaled thumbnail. - The assets panel will incorrectly display the thumbnail resolution as the resolution of the output - The lightbox (zoom) of an image will incorrectly display a downscaled thumbnail. This PR is a quick workaround to staunch the major problems - Nodes always display full previews. - Resolution downscaling is applied on the assert card, not on the assetItem itself - Due to implementation, this means that asset cards will still incorrectly show the resolution of the thumbnail instead of the size of the full image. --------- Co-authored-by: GitHub Action <action@github.com> Co-authored-by: Alexander Brown <drjkl@comfy.org>
Member
|
@AustinMroz Successfully backported to #9682 |
Member
|
@AustinMroz Successfully backported to #9683 |
github-actions
bot
removed
the
needs-backport
AustinMroz
added a commit
that referenced
this pull request
Mar 9, 2026
Backport of #9678 to `cloud/1.41` Automatically created by backport workflow. ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-9683-backport-cloud-1-41-Use-preview-downscaling-in-fewer-places-31e6d73d3650816aac69c4d1f01fa35a) by [Unito](https://www.unito.io) Co-authored-by: AustinMroz <austin@comfy.org> Co-authored-by: GitHub Action <action@github.com> Co-authored-by: Alexander Brown <drjkl@comfy.org>
AustinMroz
added a commit
that referenced
this pull request
Mar 9, 2026
Backport of #9678 to `core/1.41` Automatically created by backport workflow. ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-9682-backport-core-1-41-Use-preview-downscaling-in-fewer-places-31e6d73d365081fb845ff568d2088070) by [Unito](https://www.unito.io) Co-authored-by: AustinMroz <austin@comfy.org> Co-authored-by: GitHub Action <action@github.com> Co-authored-by: Alexander Brown <drjkl@comfy.org>
DrJKL
added a commit
that referenced
this pull request
Mar 10, 2026
Thumbnail downscaling is currently being used in more places than it should be. - Nodes which display images will display incorrect resolution indicators <img width="255" height="372" alt="image" src="https://github.com/user-attachments/assets/674790b6-04c8-4db0-84c2-2fa2dbaf123d" /> <img width="255" height="372" alt="image" src="https://github.com/user-attachments/assets/1dbe751b-7462-4408-9236-9446b005f5fc" /> This is particularly confusing with output nodes, which claim the output is not of the intended resolution - The "Download Image" and "Open Image" context menu actions will incorrectly download the downscaled thumbnail. - The assets panel will incorrectly display the thumbnail resolution as the resolution of the output - The lightbox (zoom) of an image will incorrectly display a downscaled thumbnail. This PR is a quick workaround to staunch the major problems - Nodes always display full previews. - Resolution downscaling is applied on the assert card, not on the assetItem itself - Due to implementation, this means that asset cards will still incorrectly show the resolution of the thumbnail instead of the size of the full image. --------- Co-authored-by: GitHub Action <action@github.com> Co-authored-by: Alexander Brown <drjkl@comfy.org>
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
4 participants
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.
Thumbnail downscaling is currently being used in more places than it should be.
Nodes which display images will display incorrect resolution indicators
This is particularly confusing with output nodes, which claim the output is not of the intended resolution
The "Download Image" and "Open Image" context menu actions will incorrectly download the downscaled thumbnail.
The assets panel will incorrectly display the thumbnail resolution as the resolution of the output
The lightbox (zoom) of an image will incorrectly display a downscaled thumbnail.
This PR is a quick workaround to staunch the major problems