Give every previewable asset a working preview URL - #15509
Conversation
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 50 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe asset API now resolves owner-visible preview paths in bulk. It constructs 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Comment |
019f9e3 to
5916f69
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 019f9e38da
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| def _has_previewable_content(asset: schemas.AssetData | None) -> bool: | ||
| if asset is None: | ||
| return False | ||
| mime = (asset.mime_type or "").split(";", 1)[0].strip().lower() | ||
| return mime.startswith(PREVIEWABLE_MIME_PREFIXES) |
There was a problem hiding this comment.
During the fast asset scan, records are deliberately created without metadata, so their asset.mime_type is None even when the filename is a previewable image, audio, or video. This check consequently removes preview URLs that the previous input/output path supplied, potentially permanently for fast-only or failed enrichment scans, although the /content endpoint itself already falls back to MIME detection from the reference name. Use the same filename fallback here before deciding the content is not previewable.
AGENTS.md reference: AGENTS.md:L20-L21
Useful? React with 👍 / 👎.
| if result.ref.preview_id: | ||
| preview_detail = get_asset_detail(result.ref.preview_id) | ||
| if preview_detail: | ||
| preview_url = _build_preview_url_from_view(preview_detail.tags, preview_detail.ref.user_metadata) | ||
| else: | ||
| preview_url = None | ||
| # A nominated preview is one whatever it holds, so no media check here. | ||
| preview_url = _build_preview_url(result.ref.preview_id) |
There was a problem hiding this comment.
When a nominated preview reference is subsequently soft-deleted, this branch still emits its content URL because soft deletion leaves the foreign key in place; /api/assets/{id}/content filters deleted references and therefore returns 404. The same mismatch occurs if a preview ID refers to a reference not visible to the requesting owner, since preview assignment only checks that the row exists. Preserve the previous availability check, or otherwise ensure the nominated reference is active and visible before returning a broken preview_url.
AGENTS.md reference: AGENTS.md:L359-L361
Useful? React with 👍 / 👎.
5916f69 to
c03f92e
Compare
|
Both P2s triaged. The mime fallback is fixed in c03f92e — previewability is now resolved the same way On verifying nominated previews: not doing that one; rationale is in the PR description. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔍 Cursor Review — Consolidated panel
Triggered by @synap5e.
Found 6 finding(s).
| Severity | Count |
|---|---|
| 🟡 Medium | 4 |
| 🟢 Low | 2 |
Panel: 8/8 reviewers contributed findings.
| else: | ||
| preview_url = None | ||
| # A nominated preview is one whatever it holds, so no media check here. | ||
| preview_url = _build_preview_url(result.ref.preview_id) |
There was a problem hiding this comment.
🟡 Medium — The preview_id branch now builds /api/assets/{preview_id}/content unconditionally, dropping the old get_asset_detail existence check. Since soft-delete only sets deleted_at and never clears inbound preview_id pointers, a parent whose nominated preview is soft-deleted or no longer owner-visible permanently advertises a URL that /content 404s, instead of falling back to preview_url = None as before; re-validate the preview reference before emitting its URL. Raised by 5 of 8 reviewers (claude-opus-4-8-thinking-max adversarial, gpt-5.6-sol-max adversarial, claude-opus-4-8-thinking-max edge-case, gpt-5.6-sol-max edge-case, kimi-k2.7-code edge-case).
| else: | ||
| return None | ||
| # Anything else has no visual form: a preview of its own bytes would just make the client download it all. | ||
| PREVIEWABLE_MIME_PREFIXES = ("image/", "video/", "audio/") |
There was a problem hiding this comment.
🟡 Medium — Advertising audio/video previews through /content swaps the old FileResponse for a manual streaming response that ignores HTTP Range requests. Native <video>/<audio> elements depend on byte ranges to seek and read tail metadata, so previewing large media now forces a full-file download and breaks seeking. Raised by 2 of 8 reviewers (gpt-5.6-sol-max adversarial, gpt-5.6-sol-max edge-case).
|
|
||
| def _build_preview_url(reference_id: str) -> str: | ||
| # Asking for inline does not weaken /content: it still forces dangerous types to download. | ||
| return f"/api/assets/{reference_id}/content?disposition=inline" |
There was a problem hiding this comment.
🟡 Medium — /api/assets/{id}/content is owner-scoped and derives the user from the Comfy-User header, which browser-native <img>/<video>/<audio> fetches cannot attach. Under --multi-user, previews for non-default users will 401/404, whereas the previous /api/view URLs did not require the header. Raised by 1 of 8 reviewers (gpt-5.6-sol-max adversarial).
|
|
||
| def _build_preview_url(reference_id: str) -> str: | ||
| # Asking for inline does not weaken /content: it still forces dangerous types to download. | ||
| return f"/api/assets/{reference_id}/content?disposition=inline" |
There was a problem hiding this comment.
🟡 Medium — Every preview now targets /content, which resolves via resolve_asset_for_download and commits a last_access_time update on each fetch, so rendering a list issues a DB write per thumbnail. When the list is sorted by last_access_time with offset pagination, those writes mutate the sort key mid-scroll and cause rows to be skipped or duplicated across pages. Raised by 2 of 8 reviewers (gpt-5.6-sol-max adversarial, gpt-5.6-sol-max edge-case).
| def _has_previewable_content(asset: schemas.AssetData | None) -> bool: | ||
| if asset is None: | ||
| return False | ||
| mime = (asset.mime_type or "").split(";", 1)[0].strip().lower() |
There was a problem hiding this comment.
🟢 Low — _has_previewable_content returns False whenever mime_type is NULL, so legacy/unenriched images with a renderable extension get no preview_url even though /content can serve them via its mimetypes.guess_type fallback. Such rows previously received an /api/view preview, so this silently regresses pre-enrichment assets. Raised by 1 of 8 reviewers (gpt-5.6-sol-max edge-case).
|
|
||
| def _build_preview_url(reference_id: str) -> str: | ||
| # Asking for inline does not weaken /content: it still forces dangerous types to download. | ||
| return f"/api/assets/{reference_id}/content?disposition=inline" |
There was a problem hiding this comment.
🟢 Low — reference_id is interpolated into the URL path without urllib.parse.quote(..., safe=''), dropping the encoding the old /api/view builder applied. It is not currently exploitable because ids and preview_ids are server-generated, reference-validated UUIDs (so the raised path-traversal/CSRF concern is unfounded), but restoring the encoding hardens the URL-construction boundary. Raised by 3 of 8 reviewers (claude-opus-4-8-thinking-max adversarial, gemini-3.1-pro adversarial, gemini-3.1-pro edge-case).
|
Changed approach in 1ee0050: the preview URL is now derived from the file's own path back onto
This also resolves the nominated-preview point I'd declined: resolving the preview's path means a deleted or non-visible preview now drops out and is no longer advertised. |
| """Build a /api/view preview URL from asset tags and user_metadata filename.""" | ||
| if not user_metadata: | ||
| # Anything else has no visual form: a preview of its own bytes would just make the client download it all. | ||
| PREVIEWABLE_MIME_PREFIXES = ("image/", "video/", "audio/") |
There was a problem hiding this comment.
are text and 3d omitted on purpose?
There was a problem hiding this comment.
Following up as promised: text/ is in as of 14158fc — a one-line addition to the previewable set, so a .txt/.md/.csv asset now gets a preview URL for that snippet component to fetch.
Two tests came with it: a text asset's preview URL serves its content, and a .html one is still forced to application/octet-stream + attachment when fetched, so widening the set doesn't let markup render inline in the app origin.
3d needs no change — nominated previews already cover it, and Media3DTop.vue is already written that way.
Thanks for catching the text case.
1ee0050 to
14158fc
Compare
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/assets/api/routes.py`:
- Around line 219-224: The _has_previewable_content fallback currently uses the
editable name; change it to infer MIME type from asset.ref.file_path, preferably
using its basename, while retaining stored mime_type precedence and existing
normalization. Add coverage where the asset name and file-path filename have
different extensions, ensuring preview eligibility follows the file path.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: d71fe2f8-ad59-4edc-86b3-8e10cc3d196a
📒 Files selected for processing (8)
app/assets/api/routes.pyapp/assets/database/queries/__init__.pyapp/assets/database/queries/asset_reference.pyapp/assets/services/__init__.pyapp/assets/services/asset_management.pytests-unit/assets_test/services/test_asset_response_loader_path.pytests-unit/assets_test/services/test_asset_response_preview_url.pytests-unit/assets_test/test_preview_url.py
📜 Review details
⏰ Context from checks skipped due to timeout. (9)
- GitHub Check: test (windows-2022)
- GitHub Check: test (macos-latest)
- GitHub Check: Run Pylint
- GitHub Check: test (windows-latest)
- GitHub Check: test (ubuntu-latest)
- GitHub Check: test
- GitHub Check: test (macos-latest)
- GitHub Check: test (ubuntu-latest)
- GitHub Check: Run Pylint
🧰 Additional context used
📓 Path-based instructions (5)
**/*
📄 CodeRabbit inference engine (AGENTS.md)
**/*: Keep changes small, direct, and limited to the narrowest necessary code path and smallest number of files.
Prefer practical fixes, minimal dependencies, and existing repository patterns; remove obsolete, dead, unreachable, or unused code.
Preserve existing APIs, node names, model-loading behavior, file layout, and workflow compatibility unless replacement is explicitly intended.
Core ComfyUI must not add outbound internet requests, telemetry, tracking, reporting, remote configuration, or background network activity. User-authorized model downloads are limited to the requested artifact and must exclude telemetry and unrelated metadata.
Files:
app/assets/services/__init__.pyapp/assets/database/queries/__init__.pyapp/assets/database/queries/asset_reference.pytests-unit/assets_test/services/test_asset_response_loader_path.pyapp/assets/services/asset_management.pytests-unit/assets_test/test_preview_url.pyapp/assets/api/routes.pytests-unit/assets_test/services/test_asset_response_preview_url.py
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Keep state and capability flags on the object that owns the behavior. Prefer explicit parent-owned attributes over probing child objects withgetattr; use child checks only when the child owns the delegated behavior.
Preserve shared method signatures, argument order, return shapes, side effects, and error behavior unless every affected caller and interface is intentionally updated.
Do not add unused compatibility parameters, flags, attributes, constructor options, or model-specific options to shared helpers; keep one-off behavior at the integration boundary.
Normalize third-party return conventions at integration boundaries so core code receives the expected type and shape; avoid undocumented caller-side unwrapping.
Do not addtorch.no_grad,torch.inference_mode, or inference-mode wrappers. Do not add model freeze/unfreeze toggles; only disable globally enabled inference mode when a training path requires gradients.
Remove inference-only training behavior such as dropout while preserving checkpoint and state-dict compatibility; usenn.Identitywhen deleting a module would alter keys or ordering.
Keep imports at module scope except established optional-backend probes or imports required to avoid cycles; avoid unnecessarytry/exceptblocks and use specific exceptions with useful fallbacks.
Do not add workarounds for unsupported library versions, especially PyTorch exception-and-float-cast retries, unless a comment names the exact versions still requiring them.
Let unsupported model formats, invalid quantization metadata, and bad states fail with clear errors instead of silently degrading output.
Match local style, keep comments sparse and useful, and remove comments that merely restate obvious code.
Treat dtype, device placement, VRAM use, and offloading as correctness concerns across CPU, CUDA, ROCm, MPS, DirectML, XPU, NPU, and low-VRAM environments.
Prefer existing ComfyUI and Comfy Kitchen operations, quantization helpers, cast/offload helpe...
Files:
app/assets/services/__init__.pyapp/assets/database/queries/__init__.pyapp/assets/database/queries/asset_reference.pytests-unit/assets_test/services/test_asset_response_loader_path.pyapp/assets/services/asset_management.pytests-unit/assets_test/test_preview_url.pyapp/assets/api/routes.pytests-unit/assets_test/services/test_asset_response_preview_url.py
**/*.{py,json}
📄 CodeRabbit inference engine (AGENTS.md)
Treat legacy combo,
io.Combo, andio.DynamicCombovalues affecting filesystem access as untrusted; revalidate them at load/save boundaries withfolder_paths, containment checks, or fixed allowlists.
Files:
app/assets/services/__init__.pyapp/assets/database/queries/__init__.pyapp/assets/database/queries/asset_reference.pytests-unit/assets_test/services/test_asset_response_loader_path.pyapp/assets/services/asset_management.pytests-unit/assets_test/test_preview_url.pyapp/assets/api/routes.pytests-unit/assets_test/services/test_asset_response_preview_url.py
**/*.{py,md,txt,json}
📄 CodeRabbit inference engine (AGENTS.md)
Keep warning and info messages short and actionable, remove noisy or misleading logging, and make documentation edits concise, factual, and tied to changed behavior.
Files:
app/assets/services/__init__.pyapp/assets/database/queries/__init__.pyapp/assets/database/queries/asset_reference.pytests-unit/assets_test/services/test_asset_response_loader_path.pyapp/assets/services/asset_management.pytests-unit/assets_test/test_preview_url.pyapp/assets/api/routes.pytests-unit/assets_test/services/test_asset_response_preview_url.py
**
⚙️ CodeRabbit configuration file
**: IMPORTANT: Only comment on issues directly introduced by this PR's code changes.
Treat AGENTS.md as mandatory repository policy, not optional style guidance.
Flag PR changes that violate AGENTS.md even when the code is otherwise functional.
In particular, enforce architecture boundaries, dtype/device/memory rules,
interface contracts, import style, no unnecessary try/except blocks, no inline
imports, no outbound internet paths in core ComfyUI, and narrow scoped fixes.
Prefer direct findings over suggestions when a rule is violated. Only ignore
AGENTS.md when it clearly conflicts with a newer explicit maintainer instruction
in the PR.
Do NOT flag pre-existing issues in code that was merely moved, re-indented,
de-indented, or reformatted without logic changes. If code appears in the diff
only due to whitespace or structural reformatting (e.g., removing awith:block),
treat it as unchanged. Contributors should not feel obligated to address
pre-existing issues outside the scope of their contribution.
Files:
app/assets/services/__init__.pyapp/assets/database/queries/__init__.pyapp/assets/database/queries/asset_reference.pytests-unit/assets_test/services/test_asset_response_loader_path.pyapp/assets/services/asset_management.pytests-unit/assets_test/test_preview_url.pyapp/assets/api/routes.pytests-unit/assets_test/services/test_asset_response_preview_url.py
🧠 Learnings (1)
📚 Learning: 2026-02-21T14:01:41.482Z
Learnt from: pythongosssss
Repo: Comfy-Org/ComfyUI PR: 12555
File: comfy_extras/nodes_glsl.py:719-724
Timestamp: 2026-02-21T14:01:41.482Z
Learning: In PyOpenGL, bare Python scalars can be accepted for 1-element array parameters by NumberHandler. This means you can pass an int/float directly to OpenGL texture deletion (e.g., glDeleteTextures(tex)) without wrapping in a list. Verify function-specific expectations and ensure types match what the OpenGL call expects; use explicit lists only when the API requires an array.
Applied to files:
app/assets/services/__init__.pyapp/assets/database/queries/__init__.pyapp/assets/database/queries/asset_reference.pytests-unit/assets_test/services/test_asset_response_loader_path.pyapp/assets/services/asset_management.pytests-unit/assets_test/test_preview_url.pyapp/assets/api/routes.pytests-unit/assets_test/services/test_asset_response_preview_url.py
🪛 ast-grep (0.45.1)
tests-unit/assets_test/test_preview_url.py
[info] 101-101: use jsonify instead of json.dumps for JSON output
Context: json.dumps(["models", "model_type:checkpoints", "unit-tests", scope])
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
🔇 Additional comments (8)
app/assets/database/queries/asset_reference.py (1)
1067-1087: LGTM!app/assets/database/queries/__init__.py (1)
31-31: LGTM!Also applies to: 105-105
app/assets/services/asset_management.py (1)
24-24: LGTM!Also applies to: 428-438
app/assets/services/__init__.py (1)
7-7: LGTM!Also applies to: 87-87
app/assets/api/routes.py (1)
5-5: LGTM!Also applies to: 36-36, 45-45, 212-218, 227-252, 348-351, 390-390, 521-521, 612-612, 642-642
tests-unit/assets_test/services/test_asset_response_loader_path.py (1)
46-46: LGTM!Also applies to: 70-70, 80-80
tests-unit/assets_test/services/test_asset_response_preview_url.py (1)
1-274: LGTM!tests-unit/assets_test/test_preview_url.py (1)
1-217: LGTM!
14158fc to
dd3b7dd
Compare
preview_url was assembled from a /api/view link whose type was chosen by matching the asset's tags against "input" then "output". Anything written anywhere else - temp above all, where preview nodes put their images - fell off the end of that chain and came back with no preview at all. Tags are user-editable, so removing one also silently destroyed the URL. Derive the URL from where the file actually sits instead. That covers every root /api/view serves, temp included, and no longer depends on tags or on a filename in user_metadata. A file outside those roots, or content no client can render from its own bytes, gets no preview URL rather than one that cannot work. Nominated previews are resolved a page at a time rather than per row, so a list costs one extra query however long it is. A preview that is soft-deleted or not visible to the caller drops out of that lookup and is no longer advertised.
dd3b7dd to
ee82765
Compare
2 participants
preview_urlon an asset response was built as a/api/viewlink whose type was chosen by matching the asset's tags againstinput, thenoutput. Anything ComfyUI writes anywhere else fell off the end of that chain and came back with no preview at all — most visibly images from preview nodes, which land in the temp directory and otherwise get a complete asset record. And because the choice ran off tags, and tags are user-editable, removing one silently destroyed the URL.This derives the preview from where the file actually sits, using the same path helper the response already uses for
display_name:input,output,temp) becomes the view type;filename, with any leading directories split out intosubfolder;That covers every root
/api/viewserves, temp included, and depends on neither tags nor afilenameinuser_metadata— which an API-created reference need not carry, so those had no preview before either.A file outside those roots gets no preview URL, which is the honest answer:
/api/viewhas no directory type that could address it. Neither does content a client cannot render from the bytes themselves — a checkpoint has no preview of its own, and pointing at its bytes would only make the client download the whole file to discover that. Images, video, audio and text do render, so those get a URL; a mesh does not, and reaches a preview the other way, throughpreview_id. Previewability follows the same resolution/api/viewitself uses: the stored mime type, falling back to the filename, so an asset recorded by a scan that skipped metadata extraction does not lose its preview.Why
/api/viewand not the asset content endpoint. The content endpoint resolves any reference regardless of directory, which makes it a tempting single answer, but it is a download endpoint and previews are not downloads./api/viewreturns aFileResponse, so previews keep byte-range seeking — native<video>and<audio>need it to seek and read tail metadata, and some browsers will not play a source without it. It also needs no user header, which matters because a browser fetching<img src>cannot attach one. And it records no access, so rendering a list of thumbnails performs no writes; undersort=last_access_timethose writes would otherwise mutate the sort key mid-scroll and make paginated rows skip or repeat.Nominated previews. When a reference nominates another as its preview, the preview's own path is used. Those are resolved a page at a time in a single query rather than one per row, so a long list costs one extra query, not one per asset. A nominated preview that has been deleted or is not visible to the caller drops out of that lookup and is simply not advertised, rather than yielding a URL that 404s.
Testing:
tests-unit/assets_test/services/test_asset_response_preview_url.py— URL derivation across all three view roots, subfolder splitting and encoding, independence from tags and fromuser_metadata, thepreview_idindirection, the mime fallback, and every case that should yield no URL (models, paths outside every root, no file path, no content).tests-unit/assets_test/test_preview_url.py— against a live server: the URL serves the asset's bytes, honours aRangerequest with a206, resolves with no user header, survives removing the tag that used to select the view type, follows a nominated preview, drops a soft-deleted one, and withholds a URL for model weights from both the detail and the list route.SELECTat 1, 50 and 500 rows, and none when no asset on the page nominates a preview.text/does not weaken the inline-content guard: a.htmlasset gets a preview URL and fetching it is still forced toapplication/octet-stream+attachment, asserted end to end.pytest tests-unitandruff check .green.