-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Refactor file upload and toast notifications #472
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Replaces react-hot-toast with a custom toast system using @radix-ui/react-toast, updating all usages and adding new UI components for toast and dialog. Refactors file upload to use a two-step process: first generating an S3 upload URL, then adding the file to the database, and adds file deletion support with confirmation dialog and S3 cleanup. Updates Prisma schema, removes unused fields, and cleans up navigation and admin settings page.
Added a check to restrict users to 2 file uploads in the demo, with a new helper function and error handling. Updated navigation items, improved landing page content, and removed unused dependencies (react-hot-toast). Added @radix-ui/react-toast, updated testimonials, and made minor content and code improvements.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR refactors the toast notification system and file upload functionality to improve user experience and maintainability. It replaces react-hot-toast with a custom shadcn-based toast system and restructures file uploads into a two-step process for better error handling.
- Replaces react-hot-toast with custom shadcn toast components
- Refactors file upload to use a two-step process (generate URL, then add to database)
- Adds file deletion functionality with confirmation dialog and S3 cleanup
- Removes "Pricing" navigation item from authenticated user interface
Reviewed Changes
Copilot reviewed 29 out of 29 changed files in this pull request and generated 4 comments.
Show a summary per file
File | Description |
---|---|
template/app/src/hooks/use-toast.ts | New custom toast hook implementation replacing react-hot-toast |
template/app/src/components/ui/toast.tsx | New shadcn toast UI components |
template/app/src/components/ui/toaster.tsx | Toast provider component |
template/app/src/components/ui/dialog.tsx | New dialog components for confirmations |
template/app/src/file-upload/operations.ts | Refactored to split file creation into URL generation and database operations |
template/app/src/file-upload/s3Utils.ts | Added S3 file deletion utility |
template/app/src/file-upload/FileUploadPage.tsx | Updated to use new two-step upload process and file deletion |
template/app/src/file-upload/fileUploading.ts | Modified to accept pre-generated S3 credentials |
template/app/src/demo-ai-app/DemoAppPage.tsx | Updated error handling to use new toast system |
template/app/src/client/App.tsx | Added Toaster component to app root |
template/app/src/client/components/NavBar/constants.ts | Removed Pricing from demo navigation |
template/e2e-tests/tests/utils.ts | Fixed test navigation to use direct routing |
throw new HttpError(401); | ||
} | ||
|
||
await context.entities.File.findUniqueOrThrow({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The file ownership check queries the database but doesn't use the result. Consider combining this with the delete operation or using a more efficient approach to verify ownership before deletion.
await context.entities.File.findUniqueOrThrow({ | |
const deletedFile = await context.entities.File.delete({ |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is a decent suggestion. I will use this but change the order of deletion so it deletes the file from S3 only if the DB deletion is successful.
Refactors file deletion to delete the database record before attempting S3 deletion, ensuring the file is removed from the app even if S3 deletion fails. Adds error logging for failed S3 deletions to aid in manual cleanup. Also simplifies error handling in the file upload page and removes unused imports in the demo app page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't checked the open-saas
part yet, but there are some fixes needed in the template.
Added logic to decrement user credits or throw an error if out of credits in the AI demo app. Updated file upload operations to validate file existence in S3 before adding to the database, and implemented S3 file existence check utility. Minor UI and code improvements included.
@FranjoMindek made a couple changes based on your comments and replied to others :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some more changes and questions.
Updated toast action hover style and icon spacing for better UI consistency. Enhanced error handling in file deletion to display specific error messages. Refined credit/subscription error message in GPT response operation for clarity and removed redundant credit decrement logic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new upload logic seems okay.
A bit comments on some older stuff.
Could go nice with current PR.
Replaces error state management with toast notifications for file upload errors and success. Refactors file type validation to use a new ALLOWED_FILE_TYPES_CONST and type AllowedFileTypes. Updates validation logic to throw errors instead of returning error objects, and simplifies type handling across file upload modules.
Replaces the 'key' field with 's3Key' for file storage references throughout the codebase and database schema. Updates all related logic, types, and API contracts to use 's3Key'. Adds a scheduled job to delete old files from S3 and the database. Cleans up file type validation constants and improves consistency in file upload and download operations.
@FranjoMindek pushed some more requested fixes along with a couple cleanup items. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea for cleaning up older file data.
PR seems okay now, only got 1 question about package.json.diff
.
One meta comment: it would be better if this was split into two PRs: one for toast and the other for upload logic. I'll review the logic now - but I see Franjo did a few iterations himself, nice. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left some comments
+ const filesToDelete = await context.entities.File.findMany({ | ||
+ where: { | ||
+ createdAt: { | ||
+ lt: new Date(Date.now() - 1000 * 60 * 60 * 24 * 7), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd suggest we do
const last7Days = new Date(Date.now() - 1000 * 60 * 60 * 24 * 7);
So it's more clear what is the limit. Or even better:
const dayInMiliseconds = 1000 * 60 * 60 * 24;
const timeLimitToDeleteFiles = Date.now() - 7 * dayInMiliseconds;
Something along those lines.
+ for (const file of filesToDelete) { | ||
+ try { | ||
+ await deleteFileFromS3({ s3Key: file.s3Key }); | ||
+ } catch (err) { | ||
+ console.error(`Failed to delete S3 file with key ${file.s3Key}:`, err); | ||
+ } | ||
+ } | ||
+ | ||
+ const deletedFiles = await context.entities.File.deleteMany({ | ||
+ where: { | ||
+ id: { in: filesToDelete.map((file) => file.id) }, | ||
+ }, | ||
+ }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is fine, but it would be better done in a Prisma transaction:
try {
await prisma.$transaction(async (tx) => {
await tx.file.delete(...)
await deleteFileFromS3(...)
})
} catch (err) {
// If something went wrong with deleting the file from S3, the file won't be deleted from the database
}
I'm not sure if we this is too fancy @FranjoMindek
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We use prisma transactions elsewhere in the code so I think its fine to use it here too.
onClick={async () => { | ||
try { | ||
await deleteFile({ id: fileToDelete.id }); | ||
toast({ | ||
title: 'File deleted', | ||
description: ( | ||
<span> | ||
File <strong>{fileToDelete.name}</strong> deleted. | ||
</span> | ||
), | ||
}); | ||
allUserFiles.refetch(); | ||
} catch (error) { | ||
const errorMessage = error instanceof Error ? error.message : 'Error deleting file.'; | ||
toast({ | ||
title: 'Error', | ||
description: errorMessage, | ||
variant: 'destructive', | ||
}); | ||
} finally { | ||
setFileToDelete(null); | ||
} | ||
}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's move this into a proper function and not inline it, it's a lot of logic to be inline IMHO
const deletedFile = await context.entities.File.delete({ | ||
where: { | ||
id: args.id, | ||
user: { | ||
id: context.user.id, | ||
}, | ||
}, | ||
}); | ||
|
||
try { | ||
await deleteFileFromS3({ s3Key: deletedFile.s3Key }); | ||
} catch (error) { | ||
console.error(`S3 deletion failed. Orphaned file s3Key: ${deletedFile.s3Key}`, error); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Prisma transaction would be also useful here. If the s3 deletion fails, don't delete it from the database. @FranjoMindek is this an overkill to do here?
try { | ||
await s3Client.send(command); | ||
return true; | ||
} catch (error: any) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we do better than using any
here? Let's use instanceof
to check if unknown
is some class from S3.
@@ -0,0 +1,194 @@ | |||
"use client" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wasp doesn't need these for client components. It might prompt our users to think it's necessary. Let's remove it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whoops. Good catch.
Description
Fixes #438
Replaces react-hot-toast with a custom toast system using shadcn's toast. Also adds dialog. Refactors file upload to use a two-step process: first generating an S3 upload URL, then adding the file to the database, and adds file deletion support with confirmation dialog and S3 cleanup.
This was inspired by the
Pricing
nav item that was causing confusion. In the template, I think it makes sense to show it on the LP, but not once weve logged into the Demo App, since it is available from within theAccount Settings
user dropdown, or from the Ai scheduler toast if they run out of credits.fileup.mp4
Contributor Checklist