feat(storage): add removeMany for batch object deletion#102
feat(storage): add removeMany for batch object deletion#102chandranilbakshi wants to merge 5 commits into
Conversation
WalkthroughChangesStorage batch deletion
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
![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/modules/storage.ts (1)
529-593: 🚀 Performance & Scalability | 🔵 Trivial | 🏗️ Heavy liftConsider bounding concurrency for large batch counts.
removeManysends all batches concurrently viaPromise.allSettled. For very large inputs (e.g., 100,000 keys → 100 simultaneous DELETEs), this could overwhelm the server or hit connection limits. A bounded concurrency pool (e.g., processing N batches at a time) would improve resilience without changing the public API.🤖 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/modules/storage.ts` around lines 529 - 593, Bound concurrency in removeMany instead of passing all batches directly to Promise.allSettled. Process batches through a small worker pool or chunked groups with a fixed concurrency limit, while preserving each batch’s result ordering so the existing success and failures aggregation remains 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/modules/storage.ts`:
- Around line 565-580: Guard the batch response iteration in removeMany by
handling a missing or non-array result.value.results before calling forEach.
Treat it as an empty batch or record appropriate per-key failures while
preserving outcomes already aggregated from other successful batches, instead of
allowing a TypeError to trigger the top-level catch path.
---
Nitpick comments:
In `@src/modules/storage.ts`:
- Around line 529-593: Bound concurrency in removeMany instead of passing all
batches directly to Promise.allSettled. Process batches through a small worker
pool or chunked groups with a fixed concurrency limit, while preserving each
batch’s result ordering so the existing success and failures aggregation remains
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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5a8443c7-b0af-4a9c-a510-4937b9e49ca8
📒 Files selected for processing (4)
README.mdSDK-REFERENCE.mdsrc/index.tssrc/modules/storage.ts
| result.value.results.forEach((deleteResult) => { | ||
| if (deleteResult.status === 'deleted') { | ||
| success.push(deleteResult.key); | ||
| return; | ||
| } | ||
| failures.push({ | ||
| key: deleteResult.key, | ||
| error: new InsForgeError( | ||
| deleteResult.status === 'notFound' | ||
| ? 'Object not found' | ||
| : (deleteResult.message ?? 'Failed to delete object'), | ||
| deleteResult.status === 'notFound' ? 404 : 500, | ||
| 'STORAGE_ERROR' | ||
| ), | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Guard against a missing results array in the batch response.
If the server returns a 2xx response without a results field (e.g., an empty body or a different schema version), result.value.results.forEach will throw a TypeError, causing the entire removeMany to fall into the catch block and return a top-level error — losing all per-key outcomes from other successful batches. Adding a defensive check preserves the aggregated result pattern.
🛡️ Proposed fix
result.value.results?.forEach((deleteResult) => {
if (deleteResult.status === 'deleted') {
success.push(deleteResult.key);
return;
}
failures.push({
key: deleteResult.key,
error: new InsForgeError(
deleteResult.status === 'notFound'
? 'Object not found'
: (deleteResult.message ?? 'Failed to delete object'),
deleteResult.status === 'notFound' ? 404 : 500,
'STORAGE_ERROR'
),
});
});📝 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.
| result.value.results.forEach((deleteResult) => { | |
| if (deleteResult.status === 'deleted') { | |
| success.push(deleteResult.key); | |
| return; | |
| } | |
| failures.push({ | |
| key: deleteResult.key, | |
| error: new InsForgeError( | |
| deleteResult.status === 'notFound' | |
| ? 'Object not found' | |
| : (deleteResult.message ?? 'Failed to delete object'), | |
| deleteResult.status === 'notFound' ? 404 : 500, | |
| 'STORAGE_ERROR' | |
| ), | |
| }); | |
| }); | |
| result.value.results?.forEach((deleteResult) => { | |
| if (deleteResult.status === 'deleted') { | |
| success.push(deleteResult.key); | |
| return; | |
| } | |
| failures.push({ | |
| key: deleteResult.key, | |
| error: new InsForgeError( | |
| deleteResult.status === 'notFound' | |
| ? 'Object not found' | |
| : (deleteResult.message ?? 'Failed to delete object'), | |
| deleteResult.status === 'notFound' ? 404 : 500, | |
| 'STORAGE_ERROR' | |
| ), | |
| }); | |
| }); |
🤖 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/modules/storage.ts` around lines 565 - 580, Guard the batch response
iteration in removeMany by handling a missing or non-array result.value.results
before calling forEach. Treat it as an empty batch or record appropriate per-key
failures while preserving outcomes already aggregated from other successful
batches, instead of allowing a TypeError to trigger the top-level catch path.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 4 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/modules/storage.ts">
<violation number="1" location="src/modules/storage.ts:540">
P2: Large deletes can fan out into an unbounded number of simultaneous DELETE requests, which can overwhelm browser connection pools or the storage API. Consider using a small concurrency limit while still batching keys at 1000 per request.</violation>
<violation number="2" location="src/modules/storage.ts:565">
P2: If the server returns a 2xx response without a `results` field (e.g., empty body or unexpected schema), `result.value.results.forEach(...)` will throw a `TypeError`. Because `settled.forEach` processes batches sequentially and this is inside a try/catch, the exception will short-circuit into the top-level `error` return — discarding per-key outcomes already collected from earlier successful batches.
Using optional chaining (`results?.forEach`) ensures a malformed batch response is silently skipped rather than crashing the entire aggregation.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| batches.push(paths.slice(index, index + DELETE_OBJECTS_MAX_KEYS)); | ||
| } | ||
|
|
||
| const settled = await Promise.allSettled( |
There was a problem hiding this comment.
P2: Large deletes can fan out into an unbounded number of simultaneous DELETE requests, which can overwhelm browser connection pools or the storage API. Consider using a small concurrency limit while still batching keys at 1000 per request.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/modules/storage.ts, line 540:
<comment>Large deletes can fan out into an unbounded number of simultaneous DELETE requests, which can overwhelm browser connection pools or the storage API. Consider using a small concurrency limit while still batching keys at 1000 per request.</comment>
<file context>
@@ -483,6 +512,85 @@ export class StorageBucket {
+ batches.push(paths.slice(index, index + DELETE_OBJECTS_MAX_KEYS));
+ }
+
+ const settled = await Promise.allSettled(
+ batches.map((keys) =>
+ this.http.delete<RemoveManyResponse>(
</file context>
| return; | ||
| } | ||
|
|
||
| result.value.results.forEach((deleteResult) => { |
There was a problem hiding this comment.
P2: If the server returns a 2xx response without a results field (e.g., empty body or unexpected schema), result.value.results.forEach(...) will throw a TypeError. Because settled.forEach processes batches sequentially and this is inside a try/catch, the exception will short-circuit into the top-level error return — discarding per-key outcomes already collected from earlier successful batches.
Using optional chaining (results?.forEach) ensures a malformed batch response is silently skipped rather than crashing the entire aggregation.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/modules/storage.ts, line 565:
<comment>If the server returns a 2xx response without a `results` field (e.g., empty body or unexpected schema), `result.value.results.forEach(...)` will throw a `TypeError`. Because `settled.forEach` processes batches sequentially and this is inside a try/catch, the exception will short-circuit into the top-level `error` return — discarding per-key outcomes already collected from earlier successful batches.
Using optional chaining (`results?.forEach`) ensures a malformed batch response is silently skipped rather than crashing the entire aggregation.</comment>
<file context>
@@ -483,6 +512,85 @@ export class StorageBucket {
+ return;
+ }
+
+ result.value.results.forEach((deleteResult) => {
+ if (deleteResult.status === 'deleted') {
+ success.push(deleteResult.key);
</file context>
Fermionic-Lyu
left a comment
Fermionic-Lyu
left a comment
There was a problem hiding this comment.
Solid, well-documented addition — but before merge I'd resolve one architectural issue (duplicated wire types) and one API-shape/naming issue, both of which point at a simpler implementation. The README remove('user-avatar.png') fix is correct.
Highlights (details inline):
-
Import the wire types from
@insforge/shared-schemasinstead of hand-declaring them. The server PR (InsForge#1627) already addedDeleteObjectResult,DeleteObjectsResponse,DELETE_OBJECTS_MAX_KEYS, andDELETE_OBJECT_FAILURE_MESSAGEtostorage-api.schema.ts. This module already importsStorageFileSchema/ListObjectsResponseSchemafrom there, and the dashboard'sdeleteObjects(which this PR mirrors) imports these too. The localRemoveManyResult/RemoveManyResponse/DELETE_OBJECTS_MAX_KEYSare duplicates that will drift. -
Prefer overloading
remove(path: string | string[])over a newremoveMany. This SDK mirrors Supabase Storage, whereremove(paths: string[])is the batch method — that's the convention users expect (and why the old README example was wrong). One method, one mental model, and the naming question disappears. -
Return shape doesn't match the SDK's own batch convention.
createSignedUrlsreturns a flat per-item array ({ path, signedUrl|null, error: string|null }). This returns a lossy split{ success: string[], failures: { key, error: Error }[] }that also renamespath→keyand swapserror: stringfor anErrorobject. Returning the server's{ results }directly is cleaner, mirrors the endpoint 1:1, and lets callers decide whethernotFoundis an error. -
No tests.
createSignedUrlshas a directly analogous unit test ("resolves each path independently; one failure does not fail the rest"). This method's branching (batch splitting at 1000, rejected-batch → all keys fail, per-keydeleted/notFound/failedmapping, empty short-circuit) needs the same coverage insrc/modules/__tests__/storage.test.tsbefore merge. -
Client-side batching + unbounded concurrency — the server schema enforces
.max(1000)on purpose; batching circumvents it and fans out unbounded. See inline.
| * Max object keys accepted per batch-delete request. Requests with more keys are | ||
| * split into multiple requests client-side. Mirrors the server-side limit. | ||
| */ | ||
| const DELETE_OBJECTS_MAX_KEYS = 1000; |
There was a problem hiding this comment.
These wire types are duplicates of @insforge/shared-schemas and should be imported, not redeclared. The server PR (InsForge#1627) added all of them to packages/shared-schemas/src/storage-api.schema.ts:
DELETE_OBJECTS_MAX_KEYS— this exact constantDELETE_OBJECT_FAILURE_MESSAGE— the'Failed to delete object'string you hardcode at line 575DeleteObjectResult— identical to yourRemoveManyResult({ key, status: 'deleted'|'notFound'|'failed', message? })DeleteObjectsResponse— identical to yourRemoveManyResponse({ results: [...] })
This module already imports StorageFileSchema/ListObjectsResponseSchema from @insforge/shared-schemas, and the dashboard's deleteObjects — which this PR mirrors — imports DELETE_OBJECTS_MAX_KEYS and DeleteObjectsResponse from there rather than redefining them. Bump the dep (currently ^1.1.58) to the version that ships these and import them; delete the local copies. Hand-maintained duplicates of a generated schema will silently drift when the server changes the shape.
There was a problem hiding this comment.
These wire types are duplicates of
@insforge/shared-schemasand should be imported, not redeclared. The server PR (InsForge#1627) added all of them topackages/shared-schemas/src/storage-api.schema.ts:
DELETE_OBJECTS_MAX_KEYS— this exact constantDELETE_OBJECT_FAILURE_MESSAGE— the'Failed to delete object'string you hardcode at line 575DeleteObjectResult— identical to yourRemoveManyResult({ key, status: 'deleted'|'notFound'|'failed', message? })DeleteObjectsResponse— identical to yourRemoveManyResponse({ results: [...] })This module already imports
StorageFileSchema/ListObjectsResponseSchemafrom@insforge/shared-schemas, and the dashboard'sdeleteObjects— which this PR mirrors — importsDELETE_OBJECTS_MAX_KEYSandDeleteObjectsResponsefrom there rather than redefining them. Bump the dep (currently^1.1.58) to the version that ships these and import them; delete the local copies. Hand-maintained duplicates of a generated schema will silently drift when the server changes the shape.
The shared-schemas source currently contains these exports and is versioned as 1.2.0, but npm currently only has 1.1.59, which does not include them. First we need to publish the package ver 1.2.0 then I proceed with this change right?
| * `success` holds the keys that were deleted; `failures` pairs each remaining | ||
| * key with the reason it wasn't (not found, delete failed, or a request error). | ||
| */ | ||
| export interface RemoveManyOutcome { |
There was a problem hiding this comment.
Result shape doesn't follow this SDK's own convention for batch operations. Compare createSignedUrls, the existing precedent:
Array<{ path: string; signedUrl: string | null; error: string | null }>Three divergences from that (and from the server's { results }):
- Split arrays vs flat list. The server returns
{ results: [{ key, status, message }] }andcreateSignedUrlsreturns a flat array; this reshapes into two arrays and collapses thenotFoundvsfaileddistinction into a stringly-typedError. keyvspath. The public API otherwise usespath/paths(remove(path),createSignedUrls(paths)). Usepathfor consistency.Errorobject vserror: string | null.createSignedUrlsexposes a message string, not anErrorinstance.
Cleanest option: return the server's per-key results directly, mirroring the endpoint 1:1 like every other method here:
Promise<StorageResponse<DeleteObjectsResponse>> // { data: { results }, error }Callers filter data.results by status and decide for themselves whether notFound counts as an error — rather than the SDK baking in notFound = failure. This also collapses three exported types down to the one imported from shared-schemas.
| * @returns `{ data: { success, failures }, error }`. `error` is non-null only | ||
| * for an unexpected top-level failure; per-key problems surface in `failures`. | ||
| */ | ||
| async removeMany(paths: string[]): Promise<StorageResponse<RemoveManyOutcome>> { |
There was a problem hiding this comment.
Prefer overloading remove over a separate removeMany. This SDK is a near-exact clone of Supabase Storage, where remove(paths: string[]) is the batch method — that's the shape users coming from Supabase expect, and it's exactly the assumption that made the old README remove(['x']) example look right. An overload removes the naming debate entirely and gives one mental model:
remove(path: string): Promise<StorageResponse<{ message: string }>>;
remove(paths: string[]): Promise<StorageResponse<DeleteObjectsResponse>>;(string keeps the existing single-object endpoint for backward compat; array hits the batch endpoint.)
If you do keep a separate method, removeMany is defensible (*Many is the Prisma/Mongoose convention) — but avoid deleteMany, which clashes with the remove verb this module already uses. Worth confirming: does the single-object DELETE .../objects/:key endpoint still need to exist, or can a one-element array go through the batch endpoint and let you drop it?
| batches.push(paths.slice(index, index + DELETE_OBJECTS_MAX_KEYS)); | ||
| } | ||
|
|
||
| const settled = await Promise.allSettled( |
There was a problem hiding this comment.
Client-side batching circumvents a deliberate server cap, and fans out unbounded. Two things:
- The server's
deleteObjectsRequestSchemaenforces.max(1000, 'Cannot delete more than 1000 objects at once'). This batching intentionally works around that hard limit — a product decision that arguably isn't the SDK's to make unilaterally. Every other method here is 1 call = 1 request; a thin client would pass keys straight through and surface the server's 400 for >1000. - If batching stays, this launches unbounded concurrent requests — 100k keys → 100 parallel DELETEs, 1M → 1000 — which can exhaust the browser connection pool and hammer the backend. Bound it with a small concurrency pool (~5–10 in flight).
| } | ||
|
|
||
| result.value.results.forEach((deleteResult) => { | ||
| if (deleteResult.status === 'deleted') { |
There was a problem hiding this comment.
Minor robustness: this assumes the body always has results. If a batch ever returns 204 (parseResponse → undefined) or an unexpected shape, result.value.results.forEach throws inside this callback, the outer catch fires, and the entire operation collapses to a top-level error — discarding sibling batches that already succeeded. Guard with (result.value?.results ?? []), or treat a malformed batch like a rejected one. (Moot if you switch to returning { results } directly.)
| }); | ||
|
|
||
| return { data: { success, failures }, error: null }; | ||
| } catch (error) { |
There was a problem hiding this comment.
error is null even when every key failed. Every other method in this module sets error on failure, so a caller following the pattern this SDK trains (if (error) …) will read a fully-failed batch as success. The pass-through { results } shape (see the RemoveManyOutcome comment) sidesteps this since each entry's status is self-describing; if you keep the aggregated shape, this asymmetry is worth calling out even more prominently in the docs.
2 participants
Closes #58
Summary
Adds
StorageBucket.removeMany()for deleting multiple objects in one call, and fixes the README's incorrectremove()usage.Backs the new
DELETE /api/storage/buckets/:bucketName/objectsendpoint (body{ keys: [...] }) from InsForge/InsForge#1627, which returns a per-key result ofdeleted | notFound | failed.Changes
removeMany(paths)— splits keys into batches of 1000 as there's a request cap of 1000 keys and deletes them concurrently viaPromise.allSettled, so there's no client-side cap on how many keys you pass. Mirrors the dashboard'sdeleteObjectsimplementation.RemoveManyResult,RemoveManyResponse(raw HTTP body), andRemoveManyOutcome(aggregated result), re-exported from the package root.Summary by cubic
Add
StorageBucket.removeMany()to delete many objects in one call with auto-batching and per-key results. Also fix READMEremove()example, export result types, and align code with existing style.New Features
DELETE /api/storage/buckets/:bucketName/objects(body{ keys: string[] }).removeMany(paths)batches up to 1000 keys per request and runs batches concurrently.{ data: { success: string[], failures: { key, error }[] }, error }.RemoveManyResult,RemoveManyResponse, andRemoveManyOutcome.Refactors
Written for commit bb7da6a. Summary will update on new commits.
Note
Add
removeManytoStorageBucketfor batch object deletionStorageBucket.removeMany(paths: string[])in storage.ts that deletes any number of keys via client-side batching (max 1000 per request, concurrent requests).{ success, failures }outcome where each key resolves independently; a rejected batch marks all its keys as failures.erroris only set for unexpected exceptions, not per-key failures.Macroscope summarized bb7da6a.
Summary by CodeRabbit
New Features
removeMany().Documentation