feat: replace direct AWS SDK uploads with pushduck#68
feat: replace direct AWS SDK uploads with pushduck#68abhay-ramesh wants to merge 1 commit intomichaelshimeles:mainfrom
Conversation
Greptile SummaryThis PR replaces the server-proxied Confidence Score: 4/5Safe to merge after fixing the window.location.reload() regression on the dismiss button One P1 finding: the X button calls window.location.reload() instead of dismissing the card, which is an observable UX regression. Remaining issues are P2 quality improvements that do not break the core upload flow. app/dashboard/upload/page.tsx line 311 — the dismiss button reload regression must be addressed before merge. Important Files Changed
Prompt To Fix All With AIThis is a comment left during a code review.
Path: app/dashboard/upload/page.tsx
Line: 311
Comment:
**"×" dismiss button reloads the page instead of removing the card**
The `onClick` handler was changed from `() => removeFile(file.id)` (which removed just that card from state) to `() => window.location.reload()` (which reloads the entire page). This is a hard regression: clicking "×" on any uploaded file now discards every upload in the gallery, resets all component state, and triggers a full page navigation — none of which a user expects from a dismiss/close button.
A better approach would be to track dismissed IDs in a `Set` via `useState` and filter them out of `uploadedFiles`, avoiding the full reload entirely.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: app/api/upload/route.ts
Line: 30-31
Comment:
**Unsanitized `file.name` used directly in the S3 object key**
`file.name` is inserted into the storage key without any sanitization. A filename containing `/` or `\` characters can produce misleading key structures in R2.
Consider stripping path-separator characters before embedding the name:
```ts
generateKey: ({ file, metadata }) => {
const safeName = file.name.replace(/[/\\]/g, "-");
return `uploads/${metadata.userId}/${Date.now()}-${safeName}`;
},
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: app/dashboard/upload/page.tsx
Line: 62-71
Comment:
**Success toast counts attempted files, not actually-succeeded files**
`fileArray.length` is the number of files that passed client-side validation, not the number successfully stored in R2. If pushduck resolves `uploadFiles` with some files marked `status: "error"` without rejecting the promise, the toast may overcount successes.
Consider counting only files with `status === "success"` to form the message.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "feat: replace direct AWS SDK uploads wit..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| size="sm" | ||
| variant="ghost" | ||
| onClick={() => removeFile(file.id)} | ||
| onClick={() => window.location.reload()} |
There was a problem hiding this comment.
The onClick handler was changed from () => removeFile(file.id) (which removed just that card from state) to () => window.location.reload() (which reloads the entire page). This is a hard regression: clicking "×" on any uploaded file now discards every upload in the gallery, resets all component state, and triggers a full page navigation — none of which a user expects from a dismiss/close button.
A better approach would be to track dismissed IDs in a Set via useState and filter them out of uploadedFiles, avoiding the full reload entirely.
Prompt To Fix With AI
This is a comment left during a code review.
Path: app/dashboard/upload/page.tsx
Line: 311
Comment:
**"×" dismiss button reloads the page instead of removing the card**
The `onClick` handler was changed from `() => removeFile(file.id)` (which removed just that card from state) to `() => window.location.reload()` (which reloads the entire page). This is a hard regression: clicking "×" on any uploaded file now discards every upload in the gallery, resets all component state, and triggers a full page navigation — none of which a user expects from a dismiss/close button.
A better approach would be to track dismissed IDs in a `Set` via `useState` and filter them out of `uploadedFiles`, avoiding the full reload entirely.
How can I resolve this? If you propose a fix, please make it concise.| generateKey: ({ file, metadata }) => | ||
| `uploads/${metadata.userId}/${Date.now()}-${file.name}`, |
There was a problem hiding this comment.
file.name used directly in the S3 object key
file.name is inserted into the storage key without any sanitization. A filename containing / or \ characters can produce misleading key structures in R2.
Consider stripping path-separator characters before embedding the name:
generateKey: ({ file, metadata }) => {
const safeName = file.name.replace(/[/\\]/g, "-");
return `uploads/${metadata.userId}/${Date.now()}-${safeName}`;
},Prompt To Fix With AI
This is a comment left during a code review.
Path: app/api/upload/route.ts
Line: 30-31
Comment:
**Unsanitized `file.name` used directly in the S3 object key**
`file.name` is inserted into the storage key without any sanitization. A filename containing `/` or `\` characters can produce misleading key structures in R2.
Consider stripping path-separator characters before embedding the name:
```ts
generateKey: ({ file, metadata }) => {
const safeName = file.name.replace(/[/\\]/g, "-");
return `uploads/${metadata.userId}/${Date.now()}-${safeName}`;
},
```
How can I resolve this? If you propose a fix, please make it concise.| try { | ||
| await uploadFiles(fileArray); | ||
| toast.success( | ||
| fileArray.length === 1 | ||
| ? `${fileArray[0].name} uploaded successfully` | ||
| : `${fileArray.length} files uploaded successfully`, | ||
| ); | ||
| } catch { | ||
| toast.error("Upload failed. Please try again."); | ||
| } |
There was a problem hiding this comment.
fileArray.length is the number of files that passed client-side validation, not the number successfully stored in R2. If pushduck resolves uploadFiles with some files marked status: "error" without rejecting the promise, the toast may overcount successes.
Consider counting only files with status === "success" to form the message.
Prompt To Fix With AI
This is a comment left during a code review.
Path: app/dashboard/upload/page.tsx
Line: 62-71
Comment:
**Success toast counts attempted files, not actually-succeeded files**
`fileArray.length` is the number of files that passed client-side validation, not the number successfully stored in R2. If pushduck resolves `uploadFiles` with some files marked `status: "error"` without rejecting the promise, the toast may overcount successes.
Consider counting only files with `status === "success"` to form the message.
How can I resolve this? If you propose a fix, please make it concise.1 participant
Summary
@aws-sdk/client-s3upload flow with pushduck — same Cloudflare R2 backend, cleaner abstractionapp/api/upload/route.ts— a type-safe pushduck router using thecloudflareR2provider, reusing the existing env vars (CLOUDFLARE_ACCOUNT_ID,R2_UPLOAD_IMAGE_*). Session auth is enforced viaauth.api.getSession()(better-auth), matching the pattern inapp/api/subscription/route.tsapp/dashboard/upload/page.tsxto usecreateUploadClientfrompushduck/client: real presigned-URL uploads with per-file progress, no simulated progress intervals, no manualFormDataconstructionlib/upload-image.tswith a@deprecatednotice pointing to the new router — the function itself is preserved so any other server-side callers still compilepushduck: "^0.4.0"topackage.jsonWhat changes for users
multipart/form-datato/api/upload-imagesetIntervalbetter-authsession required — returns 401 for unauthenticated requestsNo new env vars required
All existing R2 env vars are reused as-is:
Test plan
/dashboard/uploadNote
Replace direct AWS SDK uploads with pushduck in the image upload API and UI
app/api/upload/route.tsconfigured for Cloudflare R2, restricting uploads to images ≤10MB and prefixing stored files withuploads/{userId}/{timestamp}-{filename}. Requests without a valid session are rejected.app/dashboard/upload/page.tsxto use the typed pushduck client, replacing manualfetch('/api/upload-image')calls withuploadFiles()and showing live per-file progress from client hooks.lib/upload-image.tsas deprecated in favor of the new router.Macroscope summarized b6cf208.