Skip to content

feat(files): preview HEIC photos in the file viewer - #6350

Merged
waleedlatif1 merged 6 commits into
stagingfrom
feat/heic-preview
Aug 7, 2026
Merged

feat(files): preview HEIC photos in the file viewer#6350
waleedlatif1 merged 6 commits into
stagingfrom
feat/heic-preview

Conversation

@waleedlatif1

Copy link
Copy Markdown
Collaborator

Summary

The agent can read HEIC since #6346, but the Files page still showed "Preview not available for .heic files". An <img> pointed at the serve route received the stored HEIF under X-Content-Type-Options: nosniff, which no browser outside Safari renders.

The serve route now resolves a JPEG derivative for HEIF bytes and caches it, so .heic uploads preview like any other image.

Design

  • Derivative is cached, keyed by the source's storage key. Workspace keys are regenerated on every content replacement, so the key is already a content version — using it avoids streaming the whole original just to hash it, and a replaced file naturally misses the stale entry.
  • Caching matters here in a way it did not for the vision path. A preview is re-fetched on every view and the WebAssembly decode costs ~1s for a phone photo; the vision path decodes once per agent read.
  • The original stays the stored object. Downloads and ?raw=1 serve it untouched, so this never changes what a user gets back.
  • A store failure does not fail the request. Unlike the compiled-doc store — whose serve path is load-only and cannot rebuild a missing artifact — a miss here is fully recoverable: the next read transcodes again. Failing would turn a cache problem into a broken image for bytes already rendered successfully.
  • compileDocumentIfNeeded becomes resolveServableBytes, since it now resolves images as well as generated documents.

Scope

.tif/.tiff stay download-only — nothing decodes those on either side, so previewing them would show a broken image rather than a picture. That exclusion is now the only one, and the comment says why.

Type of Change

  • New feature

Testing

Six tests on the resolver covering passthrough for non-HEIF, transcode-and-cache on a miss, cache hit without re-decoding, storage-key derivation (replaced content misses the old entry), image still served when caching fails, and null when the decode fails. Viewer categorisation tests updated. 681 tests, typecheck, lint, and check:api-validation pass.

Not verified in a browser — worth a look at an actual .heic in the Files page before merge.

Checklist

  • Code follows project style guidelines
  • Self-reviewed my changes
  • Tests added/updated and passing
  • No new warnings introduced
  • I confirm that I have read and agree to the terms outlined in the Contributor License Agreement (CLA)

The agent can read HEIC since #6346, but the Files page still showed 'Preview
not available' — an <img> pointed at the serve route got the stored HEIF under
nosniff, which no browser outside Safari renders.

The serve route now resolves a JPEG derivative for HEIF bytes, cached in the
artifact store and keyed by the source's storage key. Workspace keys are
regenerated on every content replacement, so the key is already a content
version and using it avoids streaming the original just to hash it. Caching
matters here in a way it did not for the vision path: a preview is re-fetched
on every view and the WASM decode costs roughly a second for a phone photo.

The original stays the stored object — downloads and raw=1 serve it untouched,
so this never changes what a user gets back.

compileDocumentIfNeeded becomes resolveServableBytes, since it now resolves
images as well as generated documents. .tif/.tiff stay download-only: nothing
decodes those on either side.
@vercel

vercel Bot commented Aug 6, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
Project Deployment Actions Updated (UTC)
docs Skipped Skipped Aug 7, 2026 12:30am

Request Review

@cursor

cursor Bot commented Aug 6, 2026

Copy link
Copy Markdown

PR Summary

Medium Risk
Changes authenticated and public file byte serving with server-side HEIF transcoding on preview traffic, which adds CPU/WASM cost and new caching behavior, though originals for download/raw are preserved and input/size guards were added.

Overview
Adds HEIC/HEIF image preview in the Files viewer (and related surfaces) by serving a cached JPEG derivative when callers request preview=1, while downloads and raw=1 still return the stored original.

The workspace /api/files/serve path refactors document resolution into resolveServableBytes with explicit ServeOptions (raw, preview, versioned). Document compilation stays on every non-raw read; HEIF transcoding runs only for preview requests. The public share content route accepts the same preview query and applies the derivative only there so shared downloads stay unchanged.

resolveServableImageBytes detects HEVC-coded HEIF (not AVIF), loads or creates a JPEG under an image-derivative/ cache key derived from the storage key, and does not fail the response if caching fails. heic.ts adds isHevcHeifContainer, shared brand sniffing, and a cap on ftyp box scanning on preview paths.

The UI treats .heic/.heif as image-previewable, ImagePreview builds URLs with preview=1 and falls back to UnsupportedPreview on decode errors, and UnsupportedPreview is centralized in preview-shared. FileContentUrlOptions.preview and mothership attachment thumbnails append preview=1 for images only.

Reviewed by Cursor Bugbot for commit 9165266. Configure here.

Comment thread apps/sim/lib/uploads/server/image-derivative.ts
@greptile-apps

greptile-apps Bot commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

The PR adds cached JPEG derivatives for HEVC-based HEIF previews while preserving original bytes for downloads and raw requests.

  • Adds derivative resolution and caching to authenticated and public file-serving routes.
  • Enables HEIC/HEIF image categorization and requests preview-specific URLs from image viewers.
  • Adds a client-side unsupported-preview fallback when decoding or transcoding fails.
  • Extends HEIF brand detection and adds resolver, viewer, and attachment-preview tests.

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains.

Important Files Changed

Filename Overview
apps/sim/lib/uploads/server/image-derivative.ts Adds storage-keyed loading, generation, and best-effort caching of JPEG derivatives for HEVC-based HEIF images.
apps/sim/app/api/files/serve/[...path]/route.ts Resolves image derivatives only for preview requests while retaining raw and download behavior.
apps/sim/app/api/files/public/[token]/content/route.ts Adds preview-only derivative resolution to public shared-file content responses.
apps/sim/app/workspace/[workspaceId]/files/components/file-viewer/image-preview.tsx Requests preview derivatives and replaces images that fail to load with the unsupported-preview state.
apps/sim/lib/uploads/server/heic.ts Separates HEVC-specific detection from broad HEIF detection and bounds compatible-brand scanning.
apps/sim/hooks/use-file-content-source.tsx Propagates the preview marker through authenticated and public file-content URLs.

Sequence Diagram

sequenceDiagram
  participant UI as ImagePreview
  participant Route as File serve route
  participant Cache as Derivative cache
  participant Decoder as HEIF decoder
  UI->>Route: "GET content?preview=1"
  Route->>Cache: Load derivative by storage key
  alt Cached JPEG exists
    Cache-->>Route: JPEG bytes
  else Cache miss
    Route->>Decoder: Transcode HEIF to JPEG
    alt Transcode succeeds
      Decoder-->>Route: JPEG bytes
      Route->>Cache: Store derivative
    else Transcode fails
      Decoder-->>Route: No derivative
      Route-->>UI: Original bytes
      UI->>UI: onError renders unsupported fallback
    end
  end
  Route-->>UI: Renderable response
Loading

Reviews (7): Last reviewed commit: "improvement(copilot): only ask for a pre..." | Re-trigger Greptile

Comment thread apps/sim/lib/uploads/server/image-derivative.ts
…n image

Five issues from review, all interlocking around one decision.

The derivative is now requested with preview=1 rather than suppressed with
raw=1. raw=1 would have corrupted generated-document downloads: every
non-markdown workspace download routes through the serve route and relies on
resolveServableDocBytes compiling stored source into the real binary. Opt-in
separates the three consumers cleanly — previews get the JPEG, downloads get
untouched stored bytes, and doc compilation stays unconditional.

- Public shares resolve the derivative too, with the same preview/download
  split; the viewer requests it, the download button does not.
- Split the brand predicate. isHeifContainer stays broad for the vision path,
  where it only runs after sharp has already failed. The serve path runs
  first, so it uses isHevcHeifContainer — an AVIF was costing a storage
  round-trip, a WASM load and a misleading warn per request.
- A derivative that cannot be produced (past the 20MB ceiling, or a decode
  failure) now falls back to 'Preview not available' instead of a broken
  image. UnsupportedPreview moved to preview-shared to avoid a module cycle.
- The chat composer chip requests the derivative, so HEIC attachments stop
  rendering as broken thumbnails.
@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

Pushed a83cfce addressing all four threads plus a fifth issue none of them flagged.

The fifth one was the most severe and it changed the design. triggerFileDownload routes every non-markdown workspace download through this serve route, and generated .docx/.xlsx/.pptx rely on resolveServableDocBytes compiling stored source into the real binary. The obvious fix — adding raw=1 to downloads so they skip the derivative — would have fixed HEIC and simultaneously corrupted every generated-document download, handing users source text named .docx.

So the derivative is now opt-in via preview=1 rather than opt-out via raw=1. That separates the three consumers of this route by intent, provably:

Consumer Flag Gets
ImagePreview, public viewer, chat composer chip preview=1 JPEG derivative
Download button none original bytes, uncorrupted
Generated office docs none still compiled — raw=1 never enters the picture

resolveServableBytes gates only the image branch on preview; doc compilation stays unconditional, so nothing about the existing document path moved.

Also fixed the chat composer chip (attachment-preview.ts), which had the same broken-thumbnail symptom from the same cause — it already pointed at this route, so it needed the flag and nothing else.

1302 tests pass, typecheck and check:api-validation clean. New tests pin the three things that would silently regress: AVIF left untouched with neither storage nor decoder invoked, isHevcHeifContainer brand coverage, and the image-error fallback. Each was verified to go red with its fix reverted.

Still not verified in a browser — worth loading an actual .heic in the Files page, a public share link, and the chat composer before merge.

@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@greptile

@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@cursor review

An overwrite preserves the storage key, which is what the parent keys this
component on, so only the URL version changes and it never remounts. The
previous bytes' outcome therefore stuck, leaving a replaced image parked on
'Preview not available' until something else forced a remount.

Reset on URL change during render rather than in an effect — this is derived
state, and an effect would render the stale outcome first.
@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@greptile

@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@cursor review

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

Reviewed by Cursor Bugbot for commit 76e496d. Configure here.

…d scan

- Content writes mint a new storage key, so the parent's key={file.key}
  already remounts ImagePreview; the render-phase reset was unreachable and
  made renames flash a loading overlay.
- Clamp the ftyp compatible-brand scan to a real box size. The declared size
  is attacker-controlled and this now runs on every preview request.
- UnsupportedPreview takes a primitive name so memo is load-bearing.
- Fix the hardcoded ? in the public preview URL builder.
@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@greptile

@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@cursor review

…bnails

A video has no derivative path, so preview=1 there only spent a brand sniff
per request. Adds the missing test coverage for the helper.
@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@greptile

@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@cursor review

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

Reviewed by Cursor Bugbot for commit 9165266. Configure here.

@waleedlatif1
waleedlatif1 merged commit ff3b422 into staging Aug 7, 2026
24 checks passed
@waleedlatif1
waleedlatif1 deleted the feat/heic-preview branch August 7, 2026 00:34
@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@greptile The 4/5 rests on a premise that is factually incorrect for this codebase: that a content replacement is written under the same storage key. It is not — every byte-changing write mints a brand-new key, so the parent's key={file.key} already remounts the component and the stuck-state scenario cannot occur.

Evidence:

  • workspace-file-manager.ts:152generateWorkspaceFileKey() returns workspace/{ws}/{Date.now()}-{randomBytes(8)}-{name}. It is never stable across two writes.
  • workspace-file-manager.ts:1314updateWorkspaceFileContent does const nextStorageKey = generateWorkspaceFileKey(...) before writing.
  • workspace-file-manager.ts:1471 — the upload-overwrite path deletes finalized.oldKey precisely because oldKey !== uploadResult.key. That branch only exists because the key changed.

I audited every update(workspaceFiles).set({... updatedAt ...}). The only writes that bump updatedAt while preserving key are rename, move-rename, soft-delete, restore, and folder moves — all pure metadata, zero byte change.

This is also why 76e496d0f was reverted in 6e1de2cff, not kept. That commit added the URL-tracking reset you're looking for; it was unreachable in production for the overwrite case, and in the one case it did fire (rename, identical bytes) it painted an opaque loading overlay over an already-rendered image — a regression. React's own guidance ranks "reset with a key" above "adjust state during render," and the parent is already doing the key reset.

An earlier reply on this PR did claim the fix landed in 76e496d0f; that reply was wrong on the mechanism and the commit has since been reverted. The current HEAD is correct.

Separately, this round did fix two real issues: the ftyp compatible-brand scan was bounded only by an attacker-declared box size (now capped at 512 bytes, with a test verified to fail without the cap), and preview=1 is no longer sent for video thumbnails.

@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@cursor review

@greptile-apps

greptile-apps Bot commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

You're right, and the evidence is conclusive. The premise behind that concern was wrong.

The three pointers in workspace-file-manager.tsgenerateWorkspaceFileKey() embedding Date.now() + randomBytes(8), updateWorkspaceFileContent calling it to produce nextStorageKey, and the upload-overwrite branch deleting oldKey — all confirm that a content write cannot reuse the previous storage key. The parent's key={file.key} remount therefore already handles the reset, exactly as the PR's own test documents at image-preview.test.tsx:51-65.

The earlier reply on this PR that claimed otherwise was incorrect on the mechanism, and the revert of 76e496d0f correctly removed something that was both unreachable for the overwrite case and a regression for rename.

The two real fixes you call out — the ftyp box scan cap (MAX_FTYP_BOX_BYTES = 512, verified by the new test that fails without it) and the preview=1 video omission — are both sound. Comment withdrawn.

Comment thread apps/sim/app/api/files/serve/[...path]/route.ts
@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@greptile

@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@greptile

@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@cursor review

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

Reviewed by Cursor Bugbot for commit 9165266. Configure here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant