Harden workspace upload failure handling#669
Conversation
📝 WalkthroughWalkthroughUpload limits are reduced from 200 MB to 100 MB across workspace validation, container services, and documentation. Workspace batch uploads now return per-file outcomes, report failures explicitly, support cancellation, and preserve callback errors. PostHog global browser error capture is removed. ChangesWorkspace upload behavior
PostHog provider composition
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant WorkspaceFileUploadProvider
participant runWorkspaceFileUploadBatch
participant uploadAcceptedFiles
participant uploadFileDirectlyToR2
participant PostHog
WorkspaceFileUploadProvider->>runWorkspaceFileUploadBatch: start accepted-file batch
runWorkspaceFileUploadBatch->>uploadAcceptedFiles: execute concurrent uploads
uploadAcceptedFiles->>uploadFileDirectlyToR2: upload file
uploadFileDirectlyToR2-->>uploadAcceptedFiles: success or error outcome
uploadAcceptedFiles-->>runWorkspaceFileUploadBatch: return per-file outcomes
runWorkspaceFileUploadBatch->>PostHog: capture first non-abort failure
runWorkspaceFileUploadBatch-->>WorkspaceFileUploadProvider: show success or upload error
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 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 |
|
React Doctor found no new issues. 🎉 Reviewed by React Doctor for commit |
|
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. |
![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: 133d7eef96
ℹ️ 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".
| const execFileAsync = promisify(execFile); | ||
| const port = 8080; | ||
| const maxInputBytes = 200 * 1024 * 1024; | ||
| const maxInputBytes = 100 * 1024 * 1024; |
There was a problem hiding this comment.
When a HEIC/HEIF workspace upload is near the advertised 100 MiB limit, intake accepts it because it compares File.size to workspaceFileUploadLimits.maxFileBytes, but this container compares the HTTP content-length against the same 100 MiB. The conversion client sends the file as multipart/form-data, so the boundary/header bytes make an otherwise valid 100 MiB file exceed this limit and fail during conversion instead of uploading successfully; either allow overhead here or validate the parsed file's size.
Useful? React with 👍 / 👎.
Greptile SummaryThis PR hardens workspace upload failure handling and lowers upload limits. The main changes are:
Confidence Score: 5/5This PR is safe to merge with low risk. Upload limit enforcement is consistent across client validation, server-side intake, converter services, and documentation. The upload worker rewrite preserves bounded concurrency and the reviewed tests cover failure classification and cancellation behavior. No review comments were identified. No files require special attention.
 T-Rex LogsWhat T-Rex did
Important Files Changed
Reviews (1): Last reviewed commit: "fix(uploads): cap workspace uploads at 1..." | Re-trigger Greptile |
![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
🧹 Nitpick comments (1)
src/features/workspaces/files/workspace-file-upload.test.ts (1)
34-50: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueClean up stubbed globals.
While Vitest isolates test environments per file by default, it is a best practice to clean up any globally stubbed objects (like
fetchin this case) after each test. This prevents potential state leakage if isolation settings change or if tests that require the original global state are added to this suite later.♻️ Proposed refactor to add `afterEach` cleanup
Update your import to include
afterEach:-import { beforeEach, describe, expect, it, vi } from "vitest"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest";And add the hook after
beforeEach:), ); }); + +afterEach(() => { + vi.unstubAllGlobals(); +}); describe("workspace file upload batch failures", () => {🤖 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/files/workspace-file-upload.test.ts` around lines 34 - 50, Add afterEach cleanup for the fetch stub created in beforeEach, using Vitest’s global-restoration mechanism so the original fetch is restored after every test. Update the Vitest import to include afterEach and keep the existing mocked response setup unchanged.
🤖 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/files/workspace-file-upload.ts`:
- Around line 181-192: Update settleWorkspaceFileUpload to preserve DOMException
values, especially fetch abort errors, when constructing the failed
WorkspaceFileUploadOutcome. Extend the existing error selection so DOMException
instances are returned unchanged, while retaining the current Error handling and
generic fallback for other values.
---
Nitpick comments:
In `@src/features/workspaces/files/workspace-file-upload.test.ts`:
- Around line 34-50: Add afterEach cleanup for the fetch stub created in
beforeEach, using Vitest’s global-restoration mechanism so the original fetch is
restored after every test. Update the Vitest import to include afterEach and
keep the existing mocked response setup unchanged.
🪄 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
Run ID: a36b3afd-ae5e-4694-9fd8-d7abcc69f6b5
📒 Files selected for processing (12)
containers/image-converter/server.mjscontainers/liteparse/server.mjsdocs/concepts/items.mdxdocs/guides/import-files.mdxsrc/features/workspaces/components/WorkspaceFileUploadProvider.tsxsrc/features/workspaces/files/workspace-file-upload.test.tssrc/features/workspaces/files/workspace-file-upload.tssrc/features/workspaces/model/workspace-file/limits.tssrc/features/workspaces/upload/workspace-upload-intake.test.tssrc/features/workspaces/upload/workspace-upload-intake.tssrc/features/workspaces/use-workspace-client-mutation-echo.tssrc/integrations/posthog/provider.tsx
💤 Files with no reviewable changes (1)
- src/integrations/posthog/provider.tsx
| async function settleWorkspaceFileUpload( | ||
| job: WorkspaceFileUploadJob, | ||
| ): Promise<WorkspaceFileUploadOutcome> { | ||
| try { | ||
| return { command: await uploadWorkspaceFile(job), ok: true }; | ||
| } catch (error) { | ||
| return { | ||
| error: error instanceof Error ? error : new Error("Unable to upload file."), | ||
| ok: false, | ||
| }; | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Preserve DOMException here. Wrapping anything that isn’t instanceof Error converts fetch aborts into a generic error, so isWorkspaceUploadAbortError no longer recognizes user cancellations and they get reported as failures.
🤖 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/files/workspace-file-upload.ts` around lines 181 -
192, Update settleWorkspaceFileUpload to preserve DOMException values,
especially fetch abort errors, when constructing the failed
WorkspaceFileUploadOutcome. Extend the existing error selection so DOMException
instances are returned unchanged, while retaining the current Error handling and
generic fallback for other values.
1 participant
What changed
AsyncQueuercallback bridge with a bounded worker poolWhy
Production telemetry showed direct browser-to-R2 upload failures being duplicated and then flattened into a generic error. The prior queue callbacks could also misclassify cache-update failures as upload failures. Cloudflare recommends single PUT for files around this size range; the 100 MB guardrail avoids the previous 200 MB reliability and memory mismatch.
Impact
Users receive the same batch toast behavior, upload errors retain useful sanitized context, cancellations stay out of error tracking, and files above 100 MB are rejected with a 413 before processing.
The separate staging R2 CORS drift was repaired directly in Cloudflare using the checked-in
config/r2/cors.staging.jsonpolicy and verified through Wrangler and the Cloudflare API.Validation
pnpm checkgit diff --checkNeed help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by CodeRabbit
Changes
Documentation