feat: parallel batch upload pipeline with Web Worker encryption#416
Merged
Conversation
Entire-Checkpoint: 0d7d5d0837ce
Entire-Checkpoint: 716254411802
Entire-Checkpoint: b1291f2556d3
Entire-Checkpoint: 49f985289d91
Entire-Checkpoint: ecd18555ef8d
Entire-Checkpoint: 764a7c0631a9
…h event type - Install p-limit in @cipherbox/sdk for concurrency control - Add ExternalEncryptFn type and optional encryptFn parameter to sdkCore.uploadFile() - When encryptFn provided, skip internal AES encrypt/key-wrap (Web Worker offload path) - Internal encryption path unchanged (backward compatible) - fileKeyInternal only cleared in finally when internally generated (caller owns encryptFn key) - Add files:batchUploaded event variant to SdkEvent union - Export ExternalEncryptFn from @cipherbox/sdk-core - Add 2 unit tests verifying encryptFn behavior
…ol and tests - Add uploadFiles() method on CipherBoxClient with p-limit pool of 3 - Pipeline: encrypt+pin files concurrently, collect results, single folder publish - Re-read folder metadata before final publish to mitigate stale-children race (D-05) - Partial failure handling: publish successes only, surface errors for failures (D-09) - Per-file progress/complete/error callbacks for inline progress UI (D-06) - Emit files:batchUploaded and folder:updated events after successful publish - Re-wrap file keys for share recipients (best-effort, non-blocking) - Clear file keys in finally block for all successful uploads - Add 10 unit tests covering all batch orchestration behaviors - Existing uploadFile() method unchanged (D-04)
- Create 37-01-SUMMARY.md with execution results - Update STATE.md with phase 37 position and metrics - Update ROADMAP.md with plan progress
- Add encrypt.worker.ts with AES-GCM/CTR support and Transferable buffer transfers - Add EncryptionWorkerService wrapper with Promise-based API and correlation IDs - Provide ExternalEncryptFn factory via createEncryptFn() for SDK integration - Exclude Worker from main tsconfig (same pattern as decrypt-sw.ts)
… Worker encryption - Replace sequential uploadFile() loop with single uploadFiles() batch call for new files - Pass encryptFn from EncryptionWorkerService for off-main-thread encryption (D-07) - Wire per-file progress/error/complete callbacks to Zustand upload store - Add destroyEncryptionWorker() call to destroySdkClient() for cleanup on logout - Duplicate file handling path remains unchanged (uses old encrypt+upload for Replace dialog)
- Add 37-02-SUMMARY.md with execution results - Update STATE.md with plan progress and metrics - Update ROADMAP.md with phase 37 plan completion
Entire-Checkpoint: 1e204f35e21b
- Replace inline encryption result types in EncryptionWorkerService with ExternalEncryptFn from sdk-core (was defined 4 times) - Remove D-0X requirement reference comments from client.ts and useDropUpload.ts — replace with actual WHY context where needed - Clarify Worker defensive copy comment (explains security intent) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Entire-Checkpoint: 83710ddce5bb
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds Phase 37: a parallel batch upload pipeline — introduces SDK Changes
Sequence Diagram(s)sequenceDiagram
participant UI as Web UI (useDropUpload)
participant Worker as Encryption Worker
participant SDK as SDK (uploadFiles)
participant Core as SDK Core (uploadFile)
participant IPFS as IPFS/IPNS
participant Folder as Folder Metadata
UI->>UI: Pre-register files in upload store
UI->>UI: Read files into fileEntries array
UI->>Worker: createEncryptFn() → encryptFn
UI->>SDK: uploadFiles(folderIpnsName, fileEntries, callbacks, {encryptFn})
Note over SDK: Concurrency pool (max 3)
par Parallel encrypt+upload slots
SDK->>Worker: encrypt(data, userPublicKey, encryptionMode)
Worker->>Worker: Generate fileKey, IV
Worker->>Worker: AES-GCM/CTR encrypt
Worker->>Worker: Wrap key with userPublicKey
Worker-->>SDK: {ciphertext, wrappedKey, iv, fileKey} (transferable)
SDK->>Core: uploadFile(..., encryptFnResult)
Core->>IPFS: Upload ciphertext
Core->>Folder: Create file metadata
Core-->>SDK: {filePointer, cid}
SDK->>UI: onFileProgress(fileName, percent)
SDK->>UI: onFileComplete(fileName)
end
SDK->>Folder: loadFolderMetadata() (stale re-read)
SDK->>Folder: Merge successful FilePointers into children
SDK->>IPFS: updateFolderMetadataAndPublish (single publish)
IPFS-->>SDK: {sequenceNumber, publishedCid}
SDK->>SDK: Emit files:batchUploaded event
SDK-->>UI: Return {successes[], failures[]}
UI->>Worker: destroyEncryptionWorker() (on logout)
sequenceDiagram
participant Core as SDK Core uploadFile()
participant Crypto as Internal Crypto
participant Worker as ExternalEncryptFn
participant IPFS as IPFS
rect rgba(100,200,100,0.5)
Note over Core: Without encryptFn (existing path)
Core->>Crypto: generateFileKey()
Core->>Crypto: generateIv(encryptionMode)
Core->>Crypto: encryptAesGcm/Ctr()
Core->>Crypto: wrapKey(fileKey, userPublicKey)
Core->>Crypto: bytesToHex(wrappedKey, iv)
Core->>IPFS: Upload ciphertext
end
rect rgba(100,100,200,0.5)
Note over Core: With encryptFn (new path)
Core->>Worker: encryptFn({data, userPublicKey, encryptionMode})
Worker-->>Core: {ciphertext, wrappedKey, iv, fileKey}
Note over Core: Skip internal crypto and wrapping
Core->>IPFS: Upload ciphertext (from encryptFn)
end
Core->>Core: createFileMetadata(wrappedKeyHex, ivHex, originalSize, encryptedSize)
Core-->>Core: Return UploadResult with FilePointer
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
- Add uploadFiles() integration test to SDK integration suite (3-file batch, round-trip content verification, callbacks) - Add batch-upload.test.ts to sdk-e2e suite (5 tests: basic batch, round-trip, mixed sizes, progress callbacks, event emission) - Include encrypt.worker.ts in tsconfig.sw.json so CI type-checks it Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Entire-Checkpoint: 8811efb34830
Contributor
There was a problem hiding this comment.
Pull request overview
This PR introduces a parallelized, batch-oriented upload pipeline across the TypeScript SDK and web app by adding an SDK uploadFiles() method with bounded concurrency and a Web Worker–backed encryption path (via a pluggable encryptFn).
Changes:
- Add
CipherBoxClient.uploadFiles()(p-limit concurrency=3), with stale-metadata re-read and single folder publish for the batch. - Add
ExternalEncryptFn/encryptFnsupport to@cipherbox/sdk-core uploadFile()for worker/WASM/off-thread encryption. - Add web encryption worker + wrapper service, and rewire
useDropUploadto use batch uploads for new files while keeping the duplicate replacement flow.
Reviewed changes
Copilot reviewed 27 out of 29 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| pnpm-lock.yaml | Adds p-limit and its transitive deps to the lockfile. |
| packages/sdk/package.json | Adds p-limit dependency for SDK-side concurrency limiting. |
| packages/sdk/src/events.ts | Introduces files:batchUploaded SDK event payload. |
| packages/sdk/src/client.ts | Adds uploadFiles() implementation with concurrency pool + single folder publish. |
| packages/sdk/src/tests/upload-batch.test.ts | Adds unit tests for batch orchestration behavior. |
| packages/sdk-core/src/upload/index.ts | Adds ExternalEncryptFn type and encryptFn option to uploadFile(). |
| packages/sdk-core/src/index.ts | Exports ExternalEncryptFn from sdk-core public API. |
| packages/sdk-core/src/tests/upload.test.ts | Adds unit tests for encryptFn behavior in uploadFile(). |
| apps/web/tsconfig.json | Excludes the new encryption worker file from the main web TS compilation. |
| apps/web/src/workers/encrypt.worker.ts | Adds Web Worker implementation for AES-GCM/CTR + ECIES wrapping, using Transferables. |
| apps/web/src/services/encrypt-worker.service.ts | Adds main-thread service wrapper (correlation IDs, Promise API, lifecycle). |
| apps/web/src/lib/sdk-provider.ts | Ensures worker is terminated during SDK client teardown/logout. |
| apps/web/src/hooks/useDropUpload.ts | Uses client.uploadFiles() + worker-backed encryptFn for new files; duplicates remain old path. |
| .planning/phases/37-parallel-batch-upload-pipeline/37-VERIFICATION.md | Adds phase verification report artifact. |
| .planning/phases/37-parallel-batch-upload-pipeline/37-VALIDATION.md | Adds validation strategy artifact. |
| .planning/phases/37-parallel-batch-upload-pipeline/37-RESEARCH.md | Adds research notes for design decisions and pitfalls. |
| .planning/phases/37-parallel-batch-upload-pipeline/37-DISCUSSION-LOG.md | Adds discussion/audit log for alternatives and decisions. |
| .planning/phases/37-parallel-batch-upload-pipeline/37-CONTEXT.md | Adds phase context + requirements/decisions doc. |
| .planning/phases/37-parallel-batch-upload-pipeline/37-02-SUMMARY.md | Adds Plan 02 execution summary. |
| .planning/phases/37-parallel-batch-upload-pipeline/37-02-PLAN.md | Adds Plan 02 execution plan artifact. |
| .planning/phases/37-parallel-batch-upload-pipeline/37-01-SUMMARY.md | Adds Plan 01 execution summary. |
| .planning/phases/37-parallel-batch-upload-pipeline/37-01-PLAN.md | Adds Plan 01 execution plan artifact. |
| .planning/config.json | Formatting/whitespace adjustment. |
| .planning/STATE.md | Updates project phase/progress metadata to Phase 37. |
| .planning/ROADMAP.md | Adds Phase 37 section marked complete. |
| .planning/PROJECT.md | Updates “Last updated” line to reflect Phase 37 completion. |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/web/src/hooks/useDropUpload.ts`:
- Around line 169-177: Remove the trailing commas that are causing Prettier/CI
failures: delete the comma after encryptFn in the call where encryptFn is
passed, and delete the trailing comma after result.failures in the logger.warn
call; update the calls involving encryptFn and logger.warn (references:
encryptFn, result.failures, logger.warn) so they have no dangling commas.
- Around line 125-139: The current loop in useDropUpload.ts reads every File
into a Uint8Array and stores them in fileEntries, which can allocate huge
memory; instead, stop materializing data here—push the File (and its
name/type/uploadId) into the batch and defer calling file.arrayBuffer() until
inside the concurrency-limited worker or inside uploadFiles(); specifically,
replace creating {data: Uint8Array; ...} entries with a lightweight {file,
fileName: file.name, mimeType: file.type || 'application/octet-stream',
uploadId} and change the consumer (uploadFiles or the per-slot worker) to call
await file.arrayBuffer() where concurrency is controlled and cancellation is
rechecked using useUploadStore.getState().files.get(uploadId).
- Around line 128-132: The current flow only checks cancellation before calling
arrayBuffer() and never passes a per-file cancel token into the upload pipeline,
so canceled items can continue encrypting/pinning and be published; update the
upload flow by generating or retrieving a per-file AbortSignal/ cancellation
token (tied to uploadIdMap and useUploadStore.getState().files) and pass that
signal into client.uploadFiles() and any worker/SDK calls it triggers (and into
any file->arrayBuffer processing if abortable), then after client.uploadFiles()
completes, re-check useUploadStore.getState().files.get(uploadId) and skip
publishing results for files marked canceled; apply the same pattern for the
related batch code referenced around lines 146–170 to ensure canceled files
never get published.
- Around line 112-123: The try block that registers new files with
useUploadStore (via createUploadId and useUploadStore.getState().addFile) can
throw and currently only sets currentDupUploadId in the outer catch, leaving
those new-file rows stuck; fix by, inside each catch that can handle batch-level
failures (the catches around the newFiles path / arrayBuffer() / worker setup /
client.uploadFiles()), iterating the uploadIdMap created before addFile and
updating each registered row to a terminal error state using the upload store
(e.g., call the store's API to mark each uploadId as failed with the caught
error or error message), preserving the existing currentDupUploadId
behavior—apply the same pattern in the other mentioned catch sites (125-139,
146-170, 246-250).
In `@apps/web/src/services/encrypt-worker.service.ts`:
- Around line 29-32: Reformat the Worker constructor invocation in
encrypt-worker.service.ts to satisfy Prettier/ESLint: adjust the call to new
Worker(...) so the arguments (new URL('../workers/encrypt.worker.ts',
import.meta.url) and the options object { type: 'module' }) follow the project's
preferred line breaks/indentation; update the expression that assigns
this.worker to use the corrected formatting while keeping the same values and
using the existing symbols this.worker, new URL(..., import.meta.url), and {
type: 'module' } so only whitespace/line-breaks change.
- Around line 70-72: Combine the multi-line parameter declaration of the encrypt
method into a single-line signature: change the current signature in
encrypt(params: Parameters<ExternalEncryptFn>[0]): ReturnType<ExternalEncryptFn>
so the parameter and return type appear on one line (keep the method name
encrypt and types Parameters<ExternalEncryptFn>[0] and
ReturnType<ExternalEncryptFn> unchanged) to satisfy the requested formatting.
In `@apps/web/src/workers/encrypt.worker.ts`:
- Around line 69-72: The ESLint warning is caused by the inline cast/array
formatting in the self.postMessage call; update the call so the transferable
array and its cast are formatted to satisfy the project's lint rules — e.g.
break the array onto its own line and place the as Transferable[] cast cleanly
(or remove the unnecessary cast by ensuring ciphertext.buffer and
fileKeyCopy.buffer are typed as ArrayBuffer/Transferable). Modify the
self.postMessage(response, [ciphertext.buffer, fileKeyCopy.buffer] as
Transferable[]) expression (referencing self.postMessage, ciphertext.buffer,
fileKeyCopy.buffer) to use proper multiline/spacing or correct typings so ESLint
no longer flags it.
In `@packages/sdk/src/__tests__/upload-batch.test.ts`:
- Around line 277-287: The type assertion in the event handler uses "typeof
events[0]" without parentheses which ESLint/Prettier flags; update the assertion
in the client.on callback where events.push(e as typeof events[0]) is used so
that the typeof applies to the whole events symbol by wrapping it in parentheses
(i.e., use (typeof events)[0])—modify the events variable usage in the
client.on((e) => events.push(...)) line to use the corrected parenthesized type
assertion.
- Around line 323-332: The test needs the same formatting/type consistency fix
as other tests: make the on-handler use an explicit event type and use a
value-equality assertion for the bigint; change the client.on line to
client.on((e: { type: string; sequenceNumber?: bigint }) => events.push(e)); and
change the assertion from expect(folderEvent!.sequenceNumber).toBe(2n) to
expect(folderEvent!.sequenceNumber).toEqual(2n) so the sequenceNumber bigint is
compared correctly.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 94cda1d8-b603-4608-b10f-9bc3f691de98
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml,!**/pnpm-lock.yaml
📒 Files selected for processing (25)
.planning/PROJECT.md.planning/ROADMAP.md.planning/STATE.md.planning/config.json.planning/phases/37-parallel-batch-upload-pipeline/37-01-PLAN.md.planning/phases/37-parallel-batch-upload-pipeline/37-01-SUMMARY.md.planning/phases/37-parallel-batch-upload-pipeline/37-02-PLAN.md.planning/phases/37-parallel-batch-upload-pipeline/37-02-SUMMARY.md.planning/phases/37-parallel-batch-upload-pipeline/37-CONTEXT.md.planning/phases/37-parallel-batch-upload-pipeline/37-DISCUSSION-LOG.md.planning/phases/37-parallel-batch-upload-pipeline/37-RESEARCH.md.planning/phases/37-parallel-batch-upload-pipeline/37-VALIDATION.md.planning/phases/37-parallel-batch-upload-pipeline/37-VERIFICATION.mdapps/web/src/hooks/useDropUpload.tsapps/web/src/lib/sdk-provider.tsapps/web/src/services/encrypt-worker.service.tsapps/web/src/workers/encrypt.worker.tsapps/web/tsconfig.jsonpackages/sdk-core/src/__tests__/upload.test.tspackages/sdk-core/src/index.tspackages/sdk-core/src/upload/index.tspackages/sdk/package.jsonpackages/sdk/src/__tests__/upload-batch.test.tspackages/sdk/src/client.tspackages/sdk/src/events.ts
- upload-batch.test.ts: add missing `version: 'v2'` to FolderMetadata mock instead of casting as any - delegated-routing.client.spec.ts: replace `any` with `unknown` in jest.SpyInstance type parameter Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Entire-Checkpoint: ab9dedef364d
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Entire-Checkpoint: 7d1d62aeacc4
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #416 +/- ##
==========================================
+ Coverage 61.25% 61.91% +0.66%
==========================================
Files 133 133
Lines 9790 9986 +196
Branches 986 1034 +48
==========================================
+ Hits 5997 6183 +186
- Misses 3577 3587 +10
Partials 216 216
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
…rror cleanup - Capture data.length before encryptFn call to prevent Transferable buffer detachment from reporting size=0 in file metadata - Add buffer-detachment regression test in sdk-core upload tests - Wrap addFilePointerToFolder in try/catch so name collisions from concurrent uploads don't abort the entire batch publish - Add batch-level error handler in useDropUpload so thrown exceptions mark all non-complete rows as 'error' instead of leaving them stuck Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Entire-Checkpoint: 67608480976c
Add 10 new test cases covering previously uncovered paths: - addFilePointerToFolder collision handling (graceful + all-fail) - folder publish failure throw - batch IPNS publish rejection and partial failure events - non-Error rejection wrapping - BYO-IPFS external provider pinFn creation - custom pinFn passthrough - share re-wrap with shareCallbacks (success + failure) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Entire-Checkpoint: 8a402cb62b2b
Add 3 deferred ideas to CONTEXT.md and VERIFICATION.md: - Batch upload secondary pin warning events (BYO-IPFS) - AbortSignal support for in-flight batch cancellation - Lazy file reading within p-limit concurrency pool Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Entire-Checkpoint: 5d2ad59d88d7
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
packages/sdk/src/__tests__/upload-batch.test.ts (1)
244-317: Add a late-failure regression case for the completion callbacks.These tests only fail inside
sdkCore.uploadFile(). They never simulateaddFilePointerToFolder()orupdateFolderMetadataAndPublish()failing after per-file uploads resolve, so a prematureonFileComplete/ false-success regression would still pass. Add one case where uploads succeed but final registration or publish fails, and assert completion is not emitted.As per coding guidelines,
**/*.test.ts: Focus on test coverage, edge cases, and test quality.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/sdk/src/__tests__/upload-batch.test.ts` around lines 244 - 317, Add a test that simulates per-file uploads succeeding but a late failure in folder registration/publish to ensure onFileComplete isn't emitted prematurely: mock sdkCore.uploadFile to resolve normally, then make addFilePointerToFolder or updateFolderMetadataAndPublish throw (use the same helpers as in other tests, e.g., setupBatchMocks or vi.mocked for those functions), call client.uploadFiles with an onFileComplete spy and assert it was not called and that the batch event reports failures; reference uploadFiles, sdkCore.uploadFile, addFilePointerToFolder, updateFolderMetadataAndPublish, and onFileComplete when locating where to add the test.tests/sdk-e2e/src/suites/batch-upload.test.ts (1)
61-66: Make this round-trip test self-contained.It assumes
batch-a.txtwas created by the previousit(). Running this case in isolation, or after the first case fails, will fail even if download logic is correct. Seed the upload in this test or move the shared fixture upload intobeforeAll.As per coding guidelines,
**/*.test.ts: Focus on test coverage, edge cases, and test quality.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/sdk-e2e/src/suites/batch-upload.test.ts` around lines 61 - 66, This test depends on a prior test creating 'batch-a.txt' so make it self-contained by seeding the upload at the start of this spec: before calling getChild/downloadFromIpns/decodeText, upload a small file named 'batch-a.txt' using the existing client upload helper (e.g., ctx.client.upload... or the same helper used in other tests) so that getChild(ctx.client, ctx.rootIpnsName, 'batch-a.txt') will resolve; alternatively, move the shared upload fixture into a beforeAll that runs for the suite. Ensure you use the same ctx.rootIpnsName and ctx.rootFolderKey values and assert against the downloaded text as before.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/web/src/workers/encrypt.worker.ts`:
- Around line 41-70: The generated file key from generateFileKey() must be
zeroed on every exit path; wrap the main try block that uses fileKey
(encryptAesCtr/encryptAesGcm, wrapKey, and self.postMessage) with a try/finally
so clearBytes(fileKey) is always called even if encryptAes*(...), wrapKey(...),
or postMessage(...) throws; ensure you still create a fileKeyCopy for transfer
and only clear the original fileKey (not the copy) before sending or in the
finally, and reference the variables/file names fileKey, fileKeyCopy,
generateFileKey, clearBytes, encryptAesCtr, encryptAesGcm, wrapKey, and
self.postMessage when making the change.
In `@packages/sdk/src/client.ts`:
- Around line 894-909: The code currently calls callbacks?.onFileComplete
immediately after sdkCore.uploadFile resolves; instead defer emitting completion
until after the batch registration and publish succeed: only call
callbacks?.onFileComplete for entries in registeredSuccesses after
addFilePointerToFolder and updateFolderMetadataAndPublish complete successfully.
Locate the upload flow around sdkCore.uploadFile, remove or delay the immediate
callbacks?.onFileComplete(file.fileName) call, and move the notification logic
to the success branch that processes registeredSuccesses once
updateFolderMetadataAndPublish has returned without error.
---
Nitpick comments:
In `@packages/sdk/src/__tests__/upload-batch.test.ts`:
- Around line 244-317: Add a test that simulates per-file uploads succeeding but
a late failure in folder registration/publish to ensure onFileComplete isn't
emitted prematurely: mock sdkCore.uploadFile to resolve normally, then make
addFilePointerToFolder or updateFolderMetadataAndPublish throw (use the same
helpers as in other tests, e.g., setupBatchMocks or vi.mocked for those
functions), call client.uploadFiles with an onFileComplete spy and assert it was
not called and that the batch event reports failures; reference uploadFiles,
sdkCore.uploadFile, addFilePointerToFolder, updateFolderMetadataAndPublish, and
onFileComplete when locating where to add the test.
In `@tests/sdk-e2e/src/suites/batch-upload.test.ts`:
- Around line 61-66: This test depends on a prior test creating 'batch-a.txt' so
make it self-contained by seeding the upload at the start of this spec: before
calling getChild/downloadFromIpns/decodeText, upload a small file named
'batch-a.txt' using the existing client upload helper (e.g.,
ctx.client.upload... or the same helper used in other tests) so that
getChild(ctx.client, ctx.rootIpnsName, 'batch-a.txt') will resolve;
alternatively, move the shared upload fixture into a beforeAll that runs for the
suite. Ensure you use the same ctx.rootIpnsName and ctx.rootFolderKey values and
assert against the downloaded text as before.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2d72611e-9f56-455c-9277-0d313575cccb
📒 Files selected for processing (11)
apps/api/src/ipns/delegated-routing.client.spec.tsapps/web/src/hooks/useDropUpload.tsapps/web/src/services/encrypt-worker.service.tsapps/web/src/workers/encrypt.worker.tsapps/web/tsconfig.sw.jsonpackages/sdk-core/src/__tests__/upload.test.tspackages/sdk-core/src/upload/index.tspackages/sdk/src/__tests__/integration.test.tspackages/sdk/src/__tests__/upload-batch.test.tspackages/sdk/src/client.tstests/sdk-e2e/src/suites/batch-upload.test.ts
✅ Files skipped from review due to trivial changes (3)
- apps/api/src/ipns/delegated-routing.client.spec.ts
- packages/sdk-core/src/tests/upload.test.ts
- apps/web/tsconfig.sw.json
- Worker: add finally block to clear fileKey on any exit path (Thread 1) - sdk-core: clear external fileKey in catch if pinning/metadata throws, preventing key material leak when caller never receives UploadResult (Thread 4) - Extract shared EncryptRequest/EncryptResponse types to encrypt.types.ts to avoid importing from excluded worker file (Thread 5) - Fix ExternalProviderConfig shape in BYO-IPFS test — use authToken/protocol instead of accessToken (Thread 6) - Fix integration test to use correct CipherBoxClient constructor config and vault setup matching existing test pattern (Thread 7) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Entire-Checkpoint: e97b4b30fb46
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/web/src/services/encrypt-worker.service.ts (1)
84-86: Minor: Transferable uses underlying buffer, not the view.
params.data.buffertransfers the entire underlyingArrayBuffer, not just theUint8Arrayview. Ifparams.datawere a slice of a larger buffer (e.g.,new Uint8Array(bigBuffer, offset, length)), this would transfer more than intended and detach the entirebigBuffer.In current usage (data from
File.arrayBuffer()inuseDropUpload.ts), each file gets a fresh buffer, so this is safe. However, for defensive coding, consider validating that the view spans the full buffer:♻️ Optional defensive check
+ // Ensure data spans the full buffer (no extra bytes transferred) + const buffer = + params.data.byteOffset === 0 && params.data.byteLength === params.data.buffer.byteLength + ? params.data.buffer + : params.data.slice().buffer; // Create a copy if it's a view // Transfer the data buffer to worker (zero-copy, original becomes empty) - worker.postMessage(message, [params.data.buffer] as Transferable[]); + worker.postMessage(message, [buffer] as Transferable[]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/services/encrypt-worker.service.ts` around lines 84 - 86, The postMessage call currently transfers params.data.buffer which can detach an entire underlying ArrayBuffer even when params.data is a subview; update the handling around worker.postMessage (the message creation path that uses params.data and the worker.postMessage call) to defensively verify that params.data.byteOffset === 0 and params.data.byteLength === params.data.buffer.byteLength before transferring params.data.buffer, and if it is a subview instead either copy the relevant bytes into a new ArrayBuffer or transfer params.data.slice().buffer so only the intended bytes are sent; ensure the check is performed where params.data is prepared and used in worker.postMessage (and preserve using Transferable[] when transferring the full buffer).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/web/src/services/encrypt-worker.service.ts`:
- Around line 84-86: The postMessage call currently transfers params.data.buffer
which can detach an entire underlying ArrayBuffer even when params.data is a
subview; update the handling around worker.postMessage (the message creation
path that uses params.data and the worker.postMessage call) to defensively
verify that params.data.byteOffset === 0 and params.data.byteLength ===
params.data.buffer.byteLength before transferring params.data.buffer, and if it
is a subview instead either copy the relevant bytes into a new ArrayBuffer or
transfer params.data.slice().buffer so only the intended bytes are sent; ensure
the check is performed where params.data is prepared and used in
worker.postMessage (and preserve using Transferable[] when transferring the full
buffer).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d9f13af5-e7f5-44d1-a9c5-65dbd0613651
📒 Files selected for processing (8)
.planning/phases/37-parallel-batch-upload-pipeline/37-CONTEXT.md.planning/phases/37-parallel-batch-upload-pipeline/37-VERIFICATION.mdapps/web/src/services/encrypt-worker.service.tsapps/web/src/workers/encrypt.types.tsapps/web/src/workers/encrypt.worker.tspackages/sdk-core/src/upload/index.tspackages/sdk/src/__tests__/integration.test.tspackages/sdk/src/__tests__/upload-batch.test.ts
✅ Files skipped from review due to trivial changes (3)
- packages/sdk/src/tests/integration.test.ts
- apps/web/src/workers/encrypt.types.ts
- packages/sdk/src/tests/upload-batch.test.ts
If params.data is a Uint8Array subview (byteOffset > 0 or byteLength < buffer.byteLength), transferring .buffer would detach the entire underlying ArrayBuffer beyond the intended range. Copy to a new buffer in that case; transfer zero-copy when it's a full buffer. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Entire-Checkpoint: c913fcbc2591
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/web/src/services/encrypt-worker.service.ts`:
- Around line 88-89: Reformat the boolean expression that checks params.data
(the expression containing params.data.byteOffset === 0 &&
params.data.byteLength === params.data.buffer.byteLength) to satisfy
Prettier—either put the entire condition on a single line or split into multiple
lines with the logical operator at the end of each wrapped line (e.g., break
after && or place each comparison on its own line with && at the end) so the
linter/formatter no longer flags the line-break violation in
encrypt-worker.service.ts.
- Around line 69-93: In encrypt
(apps/web/src/services/encrypt-worker.service.ts) wrap the synchronous calls
that can throw—this.getWorker() and worker.postMessage(...)—in a try/catch so
that if an exception occurs you remove the pending entry for the generated id
and call its reject to avoid leaking pending promises; specifically, after
creating id and setting this.pending.set(id, {resolve,reject}), call getWorker()
and prepare message/transfer inside try, and in the catch remove
this.pending.delete(id) and call reject(err) (or reject with a descriptive
Error) before rethrowing or returning. Ensure the transfer buffer creation and
worker.postMessage remain inside the guarded block so no orphaned pending
entries remain on sync failures.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e2cac12f-85ec-40a0-936d-b4b2e0c1c97a
📒 Files selected for processing (1)
apps/web/src/services/encrypt-worker.service.ts
eciesjs (transitive dep of @cipherbox/crypto, used by wrapKey) calls Buffer.from() internally. The main thread has Buffer via Vite's resolve.alias, but Workers don't inherit these polyfills. Import Buffer from the 'buffer' package and assign to globalThis at Worker init. This was causing silent upload failures — files were pinned to IPFS but never registered in the folder because wrapKey threw inside the Worker, making all files fail in the p-limit pool. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Entire-Checkpoint: c9ea1788db66
There was a problem hiding this comment.
♻️ Duplicate comments (1)
apps/web/src/services/encrypt-worker.service.ts (1)
69-92:⚠️ Potential issue | 🟠 MajorWrap
getWorker()andpostMessage()in try-catch to prevent pending entry leak.If
getWorker()throws (e.g., Worker instantiation fails due to CSP or bundling issues) orpostMessage()throws synchronously (e.g., serialization failure), the pending map retains an orphaned entry that never resolves, causing a memory leak and a hung Promise.🛠️ Proposed fix
encrypt(params: Parameters<ExternalEncryptFn>[0]): ReturnType<ExternalEncryptFn> { return new Promise((resolve, reject) => { this.idCounter += 1; const id = `enc-${this.idCounter}-${Date.now()}`; this.pending.set(id, { resolve, reject }); + try { const worker = this.getWorker(); const message: EncryptRequest = { type: 'encrypt', id, data: params.data, userPublicKey: params.userPublicKey, encryptionMode: params.encryptionMode, }; const isFullBuffer = params.data.byteOffset === 0 && params.data.byteLength === params.data.buffer.byteLength; const transferData = isFullBuffer ? params.data : params.data.slice(); message.data = transferData; worker.postMessage(message, [transferData.buffer] as Transferable[]); + } catch (err) { + this.pending.delete(id); + reject(err instanceof Error ? err : new Error(String(err))); + } }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/services/encrypt-worker.service.ts` around lines 69 - 92, The encrypt method currently creates a pending entry (this.pending.set) before calling this.getWorker() and worker.postMessage, so if getWorker() or postMessage() throws the pending map will leak; wrap the calls to this.getWorker() and worker.postMessage(...) in a try-catch, and in the catch remove the pending entry (this.pending.delete(id)) and call reject(error) so the Promise resolves with an error; keep the id creation and pending.set as-is but ensure any synchronous failures clean up the pending map and reject the stored promise rather than leaving it orphaned.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@apps/web/src/services/encrypt-worker.service.ts`:
- Around line 69-92: The encrypt method currently creates a pending entry
(this.pending.set) before calling this.getWorker() and worker.postMessage, so if
getWorker() or postMessage() throws the pending map will leak; wrap the calls to
this.getWorker() and worker.postMessage(...) in a try-catch, and in the catch
remove the pending entry (this.pending.delete(id)) and call reject(error) so the
Promise resolves with an error; keep the id creation and pending.set as-is but
ensure any synchronous failures clean up the pending map and reject the stored
promise rather than leaving it orphaned.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 06edb215-9177-4dff-a0d3-6979bebdb400
📒 Files selected for processing (2)
apps/web/src/services/encrypt-worker.service.tsapps/web/src/workers/encrypt.worker.ts
… uploadFiles doc Wrap getWorker()/postMessage() in try/catch so a synchronous throw (terminated worker, DataCloneError) cleans up the pending entry and rejects the Promise instead of leaking. Update uploadFiles() JSDoc to clarify memory bounding only covers in-flight encryption/pinning work. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Entire-Checkpoint: 97b95bf6b43c
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
uploadFiles()batch method toCipherBoxClientwith p-limit(3) concurrency pool — encrypts and pins N files in parallel, re-reads folder metadata before publish to avoid stale-children races, and does a single folder IPNS publish instead of Nencrypt.worker.ts) with Transferable ArrayBuffer zero-copy transfers, wrapped byEncryptionWorkerServicewith Promise-based API and correlation IDsuseDropUploadto calluploadFiles()for new files with Worker-backedencryptFn, keeping the old single-file path for duplicate replacement dialogExternalEncryptFntype to sdk-core for pluggable encryption (Web Worker, WASM, etc.)encryptFnpathTest plan
pnpm --filter @cipherbox/sdk exec tsc --noEmit,pnpm --filter web exec tsc --noEmit)pnpm --filter @cipherbox/sdk exec vitest run src/__tests__/upload-batch.test.ts)pnpm --filter @cipherbox/sdk-core exec vitest run src/__tests__/upload.test.ts)🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Tests