Skip to content

feat(object-browser): delete files and prefixes in edit mode#403

Open
alukach wants to merge 5 commits into
mainfrom
worktree-file-delete
Open

feat(object-browser): delete files and prefixes in edit mode#403
alukach wants to merge 5 commits into
mainfrom
worktree-file-delete

Conversation

@alukach

@alukach alukach commented Jun 30, 2026

Copy link
Copy Markdown
Contributor

What

Adds a delete button to each row in the product file explorer, shown only in edit mode (when the user has fetched write credentials — the same signal the upload controls already use). Both files and prefixes/folders are deletable, each behind an AlertDialog confirmation popup ("Delete file" / "Delete folder … and everything inside it").

How

Delete runs client→proxy with the same per-user STS credentials the uploader uses — consistent with how uploads already work (not a server action):

  • src/lib/services/s3-upload.tsdeleteObject(key) and deletePrefix(prefix) on the existing per-scope proxy S3 client. deletePrefix lists recursively and batch-deletes (≤1000/request) with pagination.
  • src/components/features/uploader/UploadProvider.tsx — exposes both through the upload-manager context (reuses the scope→S3-service map already built for uploads).
  • src/components/features/products/object-browser/DirectoryRow.tsx — trash IconButtonAlertDialog confirm (mirrors the existing DeleteDataConnectionButton pattern). On confirm it deletes via the proxy and router.refresh()es the server-rendered listing.

Dependency

Actual deletion requires the data proxy worker to forward S3 DELETE/DeleteObjects verbs. The AWS role already grants s3:DeleteObject; verify the proxy HTTP service forwards these before relying on it in prod.

Testing

  • tsc -p tsconfig.typecheck.json → clean
  • next lint → clean (only the pre-existing getS3Service dep-array warning, matching the existing uploadFiles callback)
  • New s3-upload.test.ts — deleteObject key construction, deletePrefix pagination + batching, empty-page skip → 3 pass
  • Existing uploader tests → 5 pass

Not included

Visible error toast on delete failure (uses console.error, like the existing copy/upload paths) — add when users need failure feedback.

🤖 Generated with Claude Code

Add a delete button to each row in the product file explorer, shown only
in edit mode (when the user has fetched write credentials). Files and
prefixes/folders are both deletable, each behind an AlertDialog confirm.

Delete runs client->proxy with the same per-user STS credentials the
uploader uses: new deleteObject/deletePrefix methods on S3UploadService
(deletePrefix lists recursively and batch-deletes), exposed through the
upload-manager context, invoked from DirectoryRow with a router.refresh()
on success.

Depends on the data proxy forwarding S3 DELETE/DeleteObjects verbs.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@vercel

vercel Bot commented Jun 30, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
source-cooperative Ready Ready Preview, Comment Jun 30, 2026 6:16pm

Request Review

@claude

claude Bot commented Jun 30, 2026

Copy link
Copy Markdown
Contributor

Claude finished @alukach's task in 3m 34s —— View job


Review

  • Gather context (diff, repo guidelines)
  • Review s3-upload.ts (deleteObject/deletePrefix)
  • Review UploadProvider.tsx context wiring
  • Review DirectoryRow.tsx delete UI/confirmation flow
  • Review DirectoryList.tsx changes
  • Review tests
  • Post final verdict

✅ No blocking issues — safe to merge.

Walked the full chain: DirectoryRow.tsxUploadProvider.tsxS3UploadService.deletePrefix/deleteObject → tests. The concerns from the earlier review round on this PR (per-object delete instead of bucket-root DeleteObjects, optimistic removal with error surfacing, partial-delete warning copy, not reconciling deleted items against the eventually-consistent listing) are all already addressed in the current diff (commits 3e38201, 855629b, 1294ad0, 3841003).

Specifically verified as correct, not just plausible:

  • s3-upload.ts:120-143 (deletePrefix) — confirmed directory item.path always carries a trailing / end-to-end (S3 CommonPrefixes → server listing → FileNode.path), so the bare-string Prefix match in ListObjectsV2Command can't accidentally sweep up a sibling like foo-archive/ when deleting foo/.
  • DirectoryRow.tsx:315 (AlertDialog.Trigger without asChild) — already discussed in PR comments; Radix Themes forces asChild internally, so this is correctly a no-op.
  • DirectoryList.tsx:39-51deletedPaths is only added to on a confirmed delete and reset on prefix change, never reconciled against a fresh listing, which correctly avoids the "deleted item reappears" flicker from non-monotonic eventual consistency.
  • Pagination/batching in deletePrefix and the corresponding tests in s3-upload.test.ts are consistent and correct (continuation token handling, per-key deletes, empty-page short-circuit).

Minor, non-blocking observation:

  • UploadProvider.tsx:86-97 populates the s3Services map in a useEffect keyed off credential changes, while canEdit in DirectoryRow.tsx:86 flips true as soon as getCredentials(scope) succeeds. There's a narrow render window where the delete button is visible but getS3Service hasn't synced yet, so a click would throw "No S3 service available" (caught and surfaced via deleteError, not a crash). This mirrors the pre-existing uploadFiles pattern, so it isn't new to this PR — not blocking.

The data proxy routes by object key and has no bucket-root multi-object
delete endpoint, so DeleteObjects (POST /{bucket}?delete) 404s with
NoSuchBucket. Delete each listed key individually via DELETE
/{bucket}/{key} (the verb the proxy forwards), in small concurrent
batches to bound the request rate.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
After a large prefix delete, router.refresh() re-reads the listing through
the data proxy, whose LIST is eventually consistent after a bulk delete — so
the just-deleted prefix could still appear until a later manual refresh.

Hide a successfully-deleted item immediately (client-side) and keep it hidden
until the server listing no longer lists it (retainPresent prunes against
incoming objects, so a stale list can't resurrect it and a re-created path
isn't wrongly hidden). Still call router.refresh() to reconcile.

Also surface delete failures in the dialog instead of swallowing to console —
a partial failure on a large prefix otherwise looks identical to the
stale-listing symptom.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Per-object prefix deletion is non-atomic, so a mid-way failure leaves some
objects gone and the rest intact. Surface that in the error message for
directory deletes; single-file deletes stay all-or-nothing.

Addresses PR review feedback.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@alukach

alukach commented Jun 30, 2026

Copy link
Copy Markdown
Contributor Author

Thanks — addressed both:

1. AlertDialog.Trigger without asChild (DirectoryRow.tsx:310) — false positive, no change.

Radix Themes' AlertDialog.Trigger already forces asChild internally:

// @radix-ui/themes/dist/.../alert-dialog.js
const s = forwardRef(({ children, ...o }, i) =>
  createElement(r.Trigger, { ...o, ref: i, asChild: true }, requireReactElement(children)));

So there are no nested <button>s — the IconButton is the trigger button. And the dialog does open: Radix's Slot merges handlers child-first, and stopPropagation() doesn't set defaultPrevented, so it only blocks bubbling to the row's <Link> while Radix's open handler still fires. This matches observed behavior (prefix deletes have been working end-to-end). The existing DeleteDataConnectionButton/ProfileImageUpload triggers omit asChild for the same reason; adding it here would be a redundant no-op, so I left it as-is.

2. Partial-delete warning (s3-upload.ts deletePrefix) — applied (1294ad0).

Per-object prefix deletion is non-atomic, so a mid-way failure leaves S3 torn. The error message now warns about that — scoped to directory deletes, since a single-file delete is all-or-nothing:

const reason = error instanceof Error ? error.message : "request failed";
setDeleteError(
  item.isDirectory
    ? `Delete may be partial — ${reason}. Retry to remove remaining items.`
    : `Delete failed — ${reason}. Please retry.`
);

…sting

The proxy LIST is eventually consistent AND non-monotonic: after a delete it
can report the item gone, then a later read report it back. The previous logic
pruned a path from the optimistic-hide set the first time a listing omitted it,
so a subsequent stale read made the deleted directory briefly reappear.

Stop reconciling deletions against the listing. Keep a successfully-deleted
path hidden for the lifetime of the directory view and reset when the prefix
changes (navigating away) or on remount/refresh. We only add on a confirmed
delete, so the object really is gone. Removes the now-unused retainPresent
helper and its test.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant