feat(images): normalize uploads with Cloudflare Images - #689
Conversation
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
Capy auto-review is paused for this organization because the usage-cycle auto-review limit has been reached. Increase the limit or turn it off in billing settings to resume automatic reviews. |
|
React Doctor found no new issues. 🎉 Reviewed by React Doctor for commit |
📝 WalkthroughWalkthroughThe image-converter container and conversion module are removed. Cloudflare Images now normalizes workspace and chat images into bounded JPEGs, with updated upload validation, storage, AI extraction, error handling, runtime bindings, tests, and loading attachment UI. ChangesImage normalization migration
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 (2)
src/features/workspaces/conversion/image-normalizer.ts (1)
139-151: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPreserve the original error for debugging.
translateImageNormalizationErrorsre-wraps unknown errors using onlyerror.message, discarding the original stack trace/cause. This makes it harder to distinguish transient Images API failures from genuine content errors when triaging production incidents.♻️ Suggested fix using ES2022 error `cause`
throw new ImageNormalizationError( error instanceof Error ? error.message : "Image normalization failed.", - ); + { cause: error }, + );(Requires
ImageNormalizationError/WorkspaceFileConversionErrorconstructors to forward an optionaloptions: ErrorOptionstosuper().)🤖 Prompt for 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. In `@src/features/workspaces/conversion/image-normalizer.ts` around lines 139 - 151, Update translateImageNormalizationErrors to preserve unknown errors as the cause when constructing ImageNormalizationError, rather than retaining only error.message. Ensure the ImageNormalizationError and any relevant WorkspaceFileConversionError constructors accept and forward optional ErrorOptions to super(), while leaving existing ImageNormalizationError instances unchanged.src/features/workspaces/conversion/image-normalizer.test.ts (1)
9-88: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the empty-output error path.
Tests cover size-limit fallback and generic error translation, but not the case where Images returns a zero-byte/empty body (the
createEmptyImageErrorpath in bothrequireSizedResponseBodyandreadStreamWithinLimit). This is one of the module's core correctness guarantees.🤖 Prompt for 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. In `@src/features/workspaces/conversion/image-normalizer.test.ts` around lines 9 - 88, Add tests in the “image normalizer” suite for empty Images output, covering both normalizeImageToJpeg and normalizeChatImageToJpeg so their requireSizedResponseBody/readStreamWithinLimit paths reject with ImageNormalizationError. Assert the rejection uses createEmptyImageError semantics and verifies the relevant Images binding invocation.
🤖 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.
Nitpick comments:
In `@src/features/workspaces/conversion/image-normalizer.test.ts`:
- Around line 9-88: Add tests in the “image normalizer” suite for empty Images
output, covering both normalizeImageToJpeg and normalizeChatImageToJpeg so their
requireSizedResponseBody/readStreamWithinLimit paths reject with
ImageNormalizationError. Assert the rejection uses createEmptyImageError
semantics and verifies the relevant Images binding invocation.
In `@src/features/workspaces/conversion/image-normalizer.ts`:
- Around line 139-151: Update translateImageNormalizationErrors to preserve
unknown errors as the cause when constructing ImageNormalizationError, rather
than retaining only error.message. Ensure the ImageNormalizationError and any
relevant WorkspaceFileConversionError constructors accept and forward optional
ErrorOptions to super(), while leaving existing ImageNormalizationError
instances unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ef929edd-4ff7-4b91-9d0f-c06619ae4ae0
📒 Files selected for processing (17)
containers/image-converter/Dockerfilecontainers/image-converter/server.mjssrc/features/workspaces/conversion/image-file-converter.tssrc/features/workspaces/conversion/image-normalizer.test.tssrc/features/workspaces/conversion/image-normalizer.tssrc/features/workspaces/extraction/providers/workers-ai-to-markdown.tssrc/features/workspaces/extraction/types.tssrc/features/workspaces/extraction/workspace-file-extraction-workflow.tssrc/features/workspaces/model/workspace-file/limits.tssrc/features/workspaces/upload/workspace-file-upload-storage.test.tssrc/features/workspaces/upload/workspace-file-upload-storage.tssrc/features/workspaces/upload/workspace-upload-intake.test.tssrc/features/workspaces/upload/workspace-upload-intake.tssrc/routes/api/v1/workspaces.$workspaceId.ai-threads.$threadId.attachments.tssrc/server.tsworker-configuration.d.tswrangler.jsonc
💤 Files with no reviewable changes (4)
- containers/image-converter/Dockerfile
- containers/image-converter/server.mjs
- src/server.ts
- src/features/workspaces/conversion/image-file-converter.ts
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| throw new ImageNormalizationError( | ||
| "Cloudflare Images could not produce a JPEG within the chat attachment limit.", | ||
| ); |
There was a problem hiding this comment.
When a valid chat image remains over the limit after both profiles, this throws ImageNormalizationError, which the route maps to 422 CONVERSION_FAILED. This replaces the previous 413 ATTACHMENT_TOO_LARGE response and tells users that conversion failed instead of explaining that the optimized image is still too detailed.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Artifacts
Repro: pre-change route output showing the HTTP 413 ATTACHMENT_TOO_LARGE contract
- The full command output behind this check.
- The full command output behind this check.
Repro: executable pre-change route contract test
- Evidence file captured while the check ran.
Repro: executable current route-level failing test with mocked Images transformations
- Evidence file captured while the check ran.
Repro: Cloudflare workers environment mock used by both route tests
- Evidence file captured while the check ran.
Repro: Vitest configuration for the pre-change route contract
- Evidence file captured while the check ran.
Repro: Vitest configuration for the current route reproduction
- Evidence file captured while the check ran.
| quality: 92, | ||
| }); | ||
|
|
||
| return requireSizedResponseBody(result.response(), createEmptyImageError); |
There was a problem hiding this comment.
When a HEIC or HEIF upload uses the real Images binding, result.response() exposes transformed output as a stream without a guaranteed Content-Length, but requireSizedResponseBody rejects any response lacking that header. Valid image uploads therefore fail as empty conversions before the JPEG can be stored in R2; the test masks this by adding the header manually.
Artifacts
- Evidence file captured while the check ran.
Repro: focused Vitest configuration used to execute the harness
- Evidence file captured while the check ran.
Repro: verbose failing run showing stream characteristics and the ImageNormalizationError stack
- The full error output from the failing run.
Greptile SummaryThis PR replaces the image-converter container with a shared Cloudflare Images normalization pipeline.
Confidence Score: 2/5This PR should not merge until workspace image normalization handles the Images binding’s streaming output correctly and preserves the attachment size-error contract. The primary HEIC conversion path can reject valid transformed output while determining its size, and chat images that cannot fit the normalized limit are now incorrectly reported as generic conversion failures. Files Needing Attention: src/features/workspaces/conversion/image-normalizer.ts and src/routes/api/v1/workspaces.$workspaceId.ai-threads.$threadId.attachments.ts
T-Rex LogsWhat T-Rex did
|
| Filename | Overview |
|---|---|
| src/features/workspaces/conversion/image-normalizer.ts | Adds shared streaming and bounded image normalization, but generic size-exhaustion errors regress the attachment API and unbounded normalization assumes an unavailable response size header. |
| src/routes/api/v1/workspaces.$workspaceId.ai-threads.$threadId.attachments.ts | Migrates chat uploads to the shared normalizer but no longer preserves the size-specific response when normalization cannot meet the attachment cap. |
| src/features/workspaces/upload/workspace-file-upload-storage.ts | Routes HEIC conversion through Cloudflare Images; the orchestration remains coherent but inherits the normalizer’s response-sizing failure. |
| src/features/workspaces/extraction/workspace-file-extraction-workflow.ts | Adds guarded repeatable R2 reads for multi-profile image extraction, validating both size and ETag. |
| src/features/workspaces/extraction/providers/workers-ai-to-markdown.ts | Reuses bounded normalization before passing image bytes to Workers AI. |
| src/features/workspaces/upload/workspace-upload-intake.ts | Enforces the new 20 MB limit for accepted workspace image formats before transformation. |
| wrangler.jsonc | Adds IMAGES bindings and consistently removes the retired image-converter container, namespace binding, and Durable Object class through a deletion migration. |
Reviews (1): Last reviewed commit: "fix(uploads): enforce image transformati..." | Re-trigger Greptile
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 17 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Use the Images binding byte-stream interface instead of response headers. Classify output limits for route-specific 413 responses and preserve causes.
Use one bounded 1024px JPEG profile for chat and image extraction. Consume the existing stream directly and remove the duplicate R2 read contract.
Raise the typed size error where bounded stream reading detects overflow. Remove the impossible nullable result from both normalization paths.
Render pending images in the same square tile used by the ready preview. Keep non-image attachment loading states in their compact horizontal layout.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@src/features/workspaces/components/ai-chat/AiChatAttachmentItem.tsx`:
- Around line 81-103: Update AiChatAttachmentItem’s ready-image handling so a
ready attachment without a URL cannot remain in the done/loading presentation.
Enforce a defined url in the FileAttachmentData ready contract, or explicitly
map this invalid state to the unavailable/error attachment state and
corresponding UI instead of rendering “Preparing …”.
In `@src/features/workspaces/conversion/image-normalizer.ts`:
- Around line 104-106: Update the overflow branch in the image normalization
flow around reader.cancel to treat cancellation as best-effort: prevent a
rejection from replacing the createImageOutputTooLargeError result, then always
throw the typed output-too-large error. Add a regression test using a stream
whose cancellation rejects and verify normalization still returns the
output_too_large/413 error through translateImageNormalizationErrors.
🪄 Autofix (Beta)
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e1dfbafe-a892-4cab-b82b-7a0eb785da72
📒 Files selected for processing (8)
src/features/workspaces/components/ai-chat/AiChatAttachmentItem.tsxsrc/features/workspaces/conversion/errors.tssrc/features/workspaces/conversion/image-normalizer.test.tssrc/features/workspaces/conversion/image-normalizer.tssrc/features/workspaces/extraction/providers/workers-ai-to-markdown.tssrc/features/workspaces/upload/workspace-file-upload-storage.tssrc/routes/api/v1/workspaces.$workspaceId.ai-threads.$threadId.attachments.tssrc/routes/api/v1/workspaces.$workspaceId.file-upload.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/routes/api/v1/workspaces.$workspaceId.ai-threads.$threadId.attachments.ts
- src/features/workspaces/extraction/providers/workers-ai-to-markdown.ts
- src/features/workspaces/upload/workspace-file-upload-storage.ts
| const imageUrl = data.status === "ready" ? data.url : undefined; | ||
|
|
||
| return ( | ||
| <> | ||
| <Attachment | ||
| className="cursor-zoom-in focus-within:ring-2" | ||
| className={imageUrl ? "cursor-zoom-in focus-within:ring-2" : undefined} | ||
| orientation="vertical" | ||
| size="default" | ||
| state={getAttachmentState(data)} | ||
| > | ||
| <AttachmentMedia variant="image"> | ||
| <img | ||
| alt={label} | ||
| className="size-full object-cover" | ||
| height={96} | ||
| src={data.url} | ||
| width={96} | ||
| /> | ||
| {imageUrl ? ( | ||
| <img | ||
| alt={label} | ||
| className="size-full object-cover" | ||
| height={96} | ||
| src={imageUrl} | ||
| width={96} | ||
| /> | ||
| ) : ( | ||
| <> | ||
| <Skeleton aria-hidden="true" className="size-full rounded-none bg-foreground/10" /> | ||
| <span className="sr-only">Preparing {label}</span> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Handle ready images that have no URL.
Line 81 allows the valid FileAttachmentData state { status: "ready", url: undefined }. It is then marked done but permanently renders “Preparing …”. Require url for the ready variant at the attachment contract, or render an unavailable/error state for this invariant violation.
🤖 Prompt for 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.
In `@src/features/workspaces/components/ai-chat/AiChatAttachmentItem.tsx` around
lines 81 - 103, Update AiChatAttachmentItem’s ready-image handling so a ready
attachment without a URL cannot remain in the done/loading presentation. Enforce
a defined url in the FileAttachmentData ready contract, or explicitly map this
invalid state to the unavailable/error attachment state and corresponding UI
instead of rendering “Preparing …”.
| if (totalBytes + value.byteLength > maxBytes) { | ||
| await reader.cancel("Image output exceeds the byte limit."); | ||
| throw createImageOutputTooLargeError(maxBytes); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate file =="
fd -a 'image-normalizer\.ts$' . || true
echo "== file outline =="
ast-grep outline src/features/workspaces/conversion/image-normalizer.ts --view compact || true
echo "== relevant lines =="
cat -n src/features/workspaces/conversion/image-normalizer.ts | sed -n '1,220p'
echo "== search related errors/usages =="
rg -n "createImageOutputTooLargeError|output_too_large|conversion_failed|reader\.cancel|Image output exceeds" src || trueRepository: ThinkEx-OSS/thinkex
Length of output: 6847
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== errors.ts =="
cat -n src/features/workspaces/conversion/errors.ts
echo "== route usages context =="
cat - n src/routes/api/v1/workspaces.$workspaceId.file-upload.ts | sed -n '330,370p' || true
cat -n src/routes/api/v1/workspaces.$workspaceId.ai-threads.$threadId.attachments.ts | sed -n '80,120p' || true
echo "== normalize tests context =="
cat -n src/features/workspaces/conversion/image-normalizer.test.ts | sed -n '1,180p'
echo "== deterministic JS probe: await reader.cancel(rejects) then throw behavior =="
node - <<'JS'
function translateImageNormalizationErrors(run) {
return run().catch(error => {
const createImageNormalizationError = (message, failure, options) => {
const err = new Error(message);
err.failure = failure || "conversion_failed";
err.cause = options?.cause;
err.name = "ImageNormalizationError";
return err;
};
if (error && error.name === "ImageNormalizationError") throw error;
throw createImageNormalizationError(
error instanceof Error ? error.message : "Image normalization failed.",
"conversion_failed",
{ cause: error }
);
});
}
async function currentPath() {
const reader = {
cancel(message) {
return Promise.reject(new Error(message));
}
};
await translateImageNormalizationErrors(async () => {
await reader.cancel("Image output exceeds the byte limit.");
throw { name: "ImageNormalizationError", failure: "output_too_large" };
});
}
currentPath().catch(error => {
console.log(JSON.stringify({
name: error.name,
failure: error.failure,
causeMessage: error.cause instanceof Error ? error.cause.message : error.cause,
}));
});
JSRepository: ThinkEx-OSS/thinkex
Length of output: 7126
Preserve the typed overflow error if cancellation fails.
reader.cancel() is awaited here, so a rejection is caught by translateImageNormalizationErrors and wrapped as conversion_failed instead of the intended output_too_large (413). Cancel best-effort, then throw output_too_large.
Proposed fix
if (totalBytes + value.byteLength > maxBytes) {
- await reader.cancel("Image output exceeds the byte limit.");
+ void reader.cancel("Image output exceeds the byte limit.").catch(() => undefined);
throw createImageOutputTooLargeError(maxBytes);
}Add a regression test with a stream whose cancellation rejects.
📝 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 (totalBytes + value.byteLength > maxBytes) { | |
| await reader.cancel("Image output exceeds the byte limit."); | |
| throw createImageOutputTooLargeError(maxBytes); | |
| if (totalBytes + value.byteLength > maxBytes) { | |
| void reader.cancel("Image output exceeds the byte limit.").catch(() => undefined); | |
| throw createImageOutputTooLargeError(maxBytes); |
🤖 Prompt for 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.
In `@src/features/workspaces/conversion/image-normalizer.ts` around lines 104 -
106, Update the overflow branch in the image normalization flow around
reader.cancel to treat cancellation as best-effort: prevent a rejection from
replacing the createImageOutputTooLargeError result, then always throw the typed
output-too-large error. Add a regression test using a stream whose cancellation
rejects and verify normalization still returns the output_too_large/413 error
through translateImageNormalizationErrors.
1 participant
Summary
Why
The previous path duplicated image conversion behavior across a container and several callers. A single Cloudflare Images implementation removes that operational surface, keeps transformed bytes out of Durable Object state, and makes size/profile behavior consistent.
Changes
IMAGESbinding for local, staging, and production environments.image-normalizerwith explicit output bounds and content-type handling.Testing
pnpm exec vitest run src/features/workspaces/conversion/image-normalizer.test.ts src/features/workspaces/upload/workspace-file-upload-storage.test.ts src/features/workspaces/upload/workspace-upload-intake.test.ts— 22 tests passed.pnpm check— all 650 files formatted; no warnings, lint errors, or type errors across 626 checked files.Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Summary by CodeRabbit
New Features
Bug Fixes
Tests