feat(files): upload and safely extract ZIP archives - #6782
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
PR SummaryHigh Risk Overview Extraction pipeline is hardened in Concurrency and limits: distributed lease via Reviewed by Cursor Bugbot for commit 52b5c2f. Configure here. |
Greptile SummaryThe PR adds authenticated ZIP extraction, rollback-safe storage cleanup, multipart upload improvements, and Files-page extraction UI. One destination-path validation gap remains:
Confidence Score: 4/5The PR should not merge until final deduplicated intermediate-folder paths are validated before insertion. The reply states that destination paths are now fully validated before mutation, but intermediate folder conflicts still append suffixes after preflight without rebuilding the full path, allowing an over-limit folder path to be persisted. Files Needing Attention: apps/sim/lib/uploads/archive.ts and apps/sim/lib/uploads/contexts/workspace/workspace-file-folder-manager.ts
|
| Filename | Overview |
|---|---|
| apps/sim/lib/uploads/archive.ts | Implements bounded two-pass ZIP validation and extraction, but intermediate conflict suffixes can make final paths exceed limits after preflight. |
| apps/sim/lib/uploads/contexts/workspace/workspace-file-folder-manager.ts | Adds resolved-name validation for prepared roots, while intermediate folder deduplication still inserts names without validating the resulting full path. |
| apps/sim/lib/uploads/contexts/workspace/workspace-file-manager.ts | Coordinates rollback metadata deletion, storage accounting, and durable physical cleanup without an accepted blocking issue. |
| apps/sim/lib/uploads/contexts/workspace/workspace-file-storage-cleanup-outbox.ts | Implements idempotent storage cleanup with immediate processing and generic outbox retry support. |
| apps/sim/lib/workspace-files/application/extract-workspace-file.ts | Adds the authorized extraction use case, destination-root locking, lease handling, and batched notifications. |
| apps/sim/app/workspace/[workspaceId]/files/files.tsx | Adds ZIP upload recognition, extraction confirmation, feedback, and drag-overlay fixes without an accepted blocking issue. |
| apps/sim/lib/uploads/upload-session/service.ts | Improves local multipart selection and provider-part ordering without an accepted blocking issue. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[ZIP archive] --> B[Validate original entry paths]
B --> C[Resolve destination root]
C --> D[Materialize intermediate folders]
D --> E{Folder-name conflict?}
E -- No --> F[Create folder and upload files]
E -- Yes --> G[Append numeric suffix]
G --> H[Revalidate final full path]
H --> F
F --> I[Commit extraction]
F -- Failure --> J[Rollback metadata and accounting]
J --> K[Enqueue durable storage cleanup]
Comments Outside Diff (1)
-
apps/sim/lib/uploads/archive.ts, line 471-478 (link)Deduplicated paths bypass preflight
When an archive's intermediate folder conflicts with an existing sibling and its original path is near the 4,096-byte limit, folder creation appends a numeric suffix after preflight without validating the resulting full path. This can persist an over-limit folder path, causing subsequent path reconstruction and folder operations to throw
FolderHierarchyError.Knowledge Base Used: Upload sessions
Reviews (6): Last reviewed commit: "fix(files): only remap the deadline abor..." | Re-trigger Greptile
|
@cursor review |
There was a problem hiding this comment.
✅ 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 c779a15. Configure here.
|
@cursor review |
There was a problem hiding this comment.
✅ 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 f139e28. Configure here.
|
@cursor review |
There was a problem hiding this comment.
✅ 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 0d255a2. Configure here.
There was a problem hiding this comment.
Pull request overview
This PR adds an end-to-end ZIP extraction workflow for workspace files, including a new authenticated API endpoint, a Files-page UX flow to confirm extraction, and extraction hardening (path/item-limit validation, rollback semantics, and batched notifications). It also improves upload reliability for larger local files by preferring multipart uploads and ensures multipart completion is stable even when providers return parts out of order.
Changes:
- Add
POST /api/workspaces/[id]/files/[fileId]/extractbacked by a new authorized workspace-files use case and operation. - Harden archive extraction: validate destination paths and projected item counts before writing, use a distributed lease to prevent concurrent extracts, and improve rollback + durable storage cleanup via outbox.
- Improve upload-session behavior for local storage (proxy-safe multipart threshold) and sort provider-returned multipart parts before completion; update UI to accept/label ZIPs and prompt unzip confirmation.
Reviewed changes
Copilot reviewed 24 out of 24 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| scripts/check-api-validation-contracts.ts | Updates the API-contract audit baseline to reflect the newly added route. |
| apps/sim/lib/workspace-files/application/operations.ts | Registers a new files.extract_archive workspace operation with session-only write access. |
| apps/sim/lib/workspace-files/application/extract-workspace-file.ts | Implements the authorized extraction use case with idempotency-lease protection and batched notifications. |
| apps/sim/lib/workspace-files/application/extract-workspace-file.test.ts | Adds unit coverage for extraction behavior, authorization, leasing, and error cleanup. |
| apps/sim/lib/workspace-files/application/create-workspace-file.ts | Adds notifyWorkspaceChange plumbing to suppress per-file notifications during bulk operations. |
| apps/sim/lib/workspace-files/api/internal-error-policies.ts | Adds an internal error policy that maps ArchiveError to caller-safe status codes. |
| apps/sim/lib/workspace-files/api/internal-error-policies.test.ts | Tests archive-error → HTTP status mapping behavior. |
| apps/sim/lib/uploads/upload-session/service.ts | Adjusts transfer method selection for local uploads and returns sorted multipart parts for completion. |
| apps/sim/lib/uploads/upload-session/service.test.ts | Tests the local multipart threshold and out-of-order provider parts handling. |
| apps/sim/lib/uploads/contexts/workspace/workspace-file-storage-cleanup-outbox.ts | Introduces a durable outbox event/handler for deferred workspace object deletion. |
| apps/sim/lib/uploads/contexts/workspace/workspace-file-storage-cleanup-outbox.test.ts | Tests payload validation, ENOENT handling, and retry-on-failure behavior for cleanup outbox. |
| apps/sim/lib/uploads/contexts/workspace/workspace-file-storage-accounting.test.ts | Extends storage/accounting tests for purge rollback, durable cleanup, and notification suppression. |
| apps/sim/lib/uploads/contexts/workspace/workspace-file-manager.ts | Adds durable purge semantics for archive-created files and integrates cleanup outbox + notification suppression. |
| apps/sim/lib/uploads/contexts/workspace/workspace-file-folder-manager.ts | Adds exact-name/dedup hooks for folder creation and an “archive if empty” rollback helper. |
| apps/sim/lib/uploads/contexts/workspace/workspace-file-folder-manager.test.ts | Adds coverage for folder dedup behavior and empty-folder archival semantics. |
| apps/sim/lib/uploads/archive.ts | Enhances archive extraction with preflight path/item validation, rollback via purge + empty-folder archival, and batched notifications. |
| apps/sim/lib/uploads/archive.test.ts | Updates/extends extraction tests for notifications, rollback semantics, and item/path pre-validation. |
| apps/sim/lib/api/contracts/workspace-files.ts | Adds a shared route contract + response schema for the new extract endpoint. |
| apps/sim/hooks/queries/workspace-file-folders.ts | Adds a React Query mutation hook for extraction and invalidates file browser caches. |
| apps/sim/hooks/queries/workspace-file-folders.test.ts | Tests cache invalidation and error handling for the new extraction mutation hook. |
| apps/sim/app/workspace/[workspaceId]/files/files.tsx | Updates Files UI to accept/label ZIPs and adds an unzip confirmation modal flow. |
| apps/sim/app/api/workspaces/[id]/files/[fileId]/extract/route.ts | Adds the new internal JSON route adapter for archive extraction. |
| apps/sim/app/api/workspaces/[id]/files/[fileId]/extract/route.test.ts | Adds route-level tests for auth, input mapping, success response, and archive error mapping. |
| apps/sim/app/api/webhooks/outbox/process/route.ts | Registers the new workspace-file storage cleanup outbox handler with the outbox processor. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…r's option surface `maxMaterializedItems` was opt-in, so only the new unzip route bounded its output tree — the copilot `materialize_file` and `POST /api/tools/file/manage` extract paths had no cap on folder creation at all. An archive within MAX_ARCHIVE_ENTRIES can still imply far more folders than files, so the cap now defaults to MAX_WORKSPACE_FILE_BULK_AFFECTED_ITEMS and applies to all three callers. `materializedRootFolderCount` was a hand-maintained number that had to agree with what an opaque callback would create, and the callee could not check it; drift surfaced only as an over-limit archive slipping past the cap. It is now derived from whether `prepareRootFolder` ran, so the contract is just "the callback creates exactly one folder". Also single-sources the ArchiveError -> HTTP status map (it was copied into both the internal error policy and the tools route), drops IdempotencyService config that only `executeWithIdempotency` reads (the extraction lease uses atomicallyClaim/release, so no result is ever stored), hoists the duplicated predicates in purgeCreatedWorkspaceFile and archiveWorkspaceFileFolderIfEmpty so a lock and its write cannot diverge, and names UPLOAD_SESSION_LOCAL_PUT_MAX_BYTES rather than overloading the multipart part size as the local single-PUT ceiling. Adds coverage for the two guards nothing exercised: the re-validation of the segments `prepareRootFolder` actually returned, and the default cap applying with no caller opt-in. UI: the drop overlay used --surface-4 unconditionally, which renders grey over the light-mode canvas; matches the canonical overlay's --white/dark:--surface-4 and swaps arbitrary px type sizes for named tokens.
…lease Cursor Bugbot flagged two related holes, both rooted in the write loop being unbounded: 1. `maxDuration` is a Next.js route-segment config that serverless platforms enforce and self-hosted deployments do not. A slow extraction (up to 1000 sequential uploads) could therefore outrun the six-minute lease, and `IdempotencyService` reclaims an expired in-progress claim — so a second unzip of the same archive could start beside the first. 2. Nothing rolls back a process killed mid-pass-2, so a timeout stranded the destination folder and every file written so far. `decompressArchiveBufferToWorkspaceFiles` now takes an `AbortSignal` and checks it between entries in both passes, and the extraction use case supplies a 180s deadline. The abort unwinds through the existing all-or-nothing rollback, so the work stops on our terms with the tree cleaned up, well inside both the route's 300s budget and the 360s lease. That closes (1) outright — the holder can no longer outlive its lease on any platform — and converts (2) from a stranded partial tree into a clean rollback for the slow case that actually triggers it. A SIGKILL still cannot be caught; that needs a durable job and is out of scope here. The overrun surfaces as a caller-fixable 413 naming the archive rather than an opaque 500 from the raw DOMException.
|
@cursor review |
…ng rollback Two follow-ups on the budget deadline, both reported by Cursor Bugbot: `deadline.aborted` stays true for the rest of the request once the timer fires, so it cannot decide whether *this* error was the abort. An `ArchiveError` or storage failure thrown mid-entry after the timer fired was being relabelled as a timeout and returned as a 413, hiding the real cause. The catch now matches the thrown value against `deadline.reason` — `throwIfAborted()` throws exactly that object, so the check is identity-exact and cannot capture an unrelated failure. The message also claimed a rollback that has not necessarily happened: the budget covers the archive download too, so it can fire before the first write, when there is nothing to roll back. It now says the unzip was cancelled and claims nothing about what was written. Including the download in the budget is deliberate — the lease it has to fit inside starts earlier still — so the TSDoc says that rather than "the extraction itself".
|
@cursor review |
There was a problem hiding this comment.
✅ 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 52b5c2f. Configure here.
Summary
The Files page previously rejected ZIP uploads and provided no way to unpack an archive. This PR adds an end-to-end ZIP extraction flow and fixes safety, cleanup, notification, and upload reliability issues uncovered while building it.
Features
Upload ZIP files
.zipfiles through the file picker and drag-and-drop.Unzip files from the Files page
Create a predictable extracted folder
archive.zipinto a sibling folder namedarchive.Fixes and hardening
Prevent duplicate extraction
Validate the complete output before writing
Make failed extraction rollback safe
Make storage cleanup durable
Batch workspace notifications
Improve upload and drag-and-drop reliability
Test coverage
Added or updated Vitest coverage for: