feat(workspace): add delete workspace with cascade delete#285
Conversation
Summary of ChangesHello @l1ttps, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a critical feature allowing users to delete workspaces, along with all their dependent data, through a cascade deletion mechanism. The changes span both the backend API, where database entity relationships were updated for cascade deletion, and the frontend console, which now includes a dedicated UI component for initiating and confirming workspace deletions. This ensures data consistency and provides users with full control over their workspace lifecycle. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request implements cascade deletion for workspaces, so that when a workspace is deleted, all its related data is also removed from the database. This is achieved by adding onDelete: 'CASCADE' to various entity relationships in the backend and replacing the soft delete with a hard delete. On the frontend, the 'Archive' action is replaced with a 'Delete' action.
My review has identified a few areas for improvement:
- A regression in error handling in the asset groups page.
- An incorrect Tailwind CSS class in the edit workspace dialog.
- A redundant TypeORM option in the workspace entity.
- An opportunity to simplify code in the workspace service by removing unnecessary manual ID generation.
Overall, the changes are in the right direction to implement the desired feature.
| const assetGroups = data?.data ?? []; | ||
| const total = data?.total ?? 0; | ||
|
|
||
| if (!data && !isLoading) return <div>Error loading asset groups.</div>; |
There was a problem hiding this comment.
The explicit error handling for the case where data fetching fails has been removed. Now, if there's an error, the page will just show an empty table which might be confusing for the user. It would be better to restore some form of error handling to inform the user when something goes wrong. You can achieve this by using the isError flag from the useAssetGroupControllerGetAll hook.
| </Button> | ||
| <Dialog open={open} onOpenChange={setOpen}> | ||
| <DialogContent className="sm:max-w-[425px]"> | ||
| <DialogContent className="sm:max-w-106.25"> |
There was a problem hiding this comment.
The Tailwind CSS class sm:max-w-106.25 is not a standard Tailwind class and is likely a typo. The original value was sm:max-w-[425px]. To ensure consistent styling, please revert to the previous value or use a corresponding rem value like sm:max-w-[26.5rem].
| <DialogContent className="sm:max-w-106.25"> | |
| <DialogContent className="sm:max-w-[425px]"> |
| @OneToMany( | ||
| () => WorkspaceTarget, | ||
| (workspaceTarget) => workspaceTarget.workspace, | ||
| { onDelete: 'CASCADE' }, |
There was a problem hiding this comment.
The onDelete: 'CASCADE' option on a @OneToMany relationship has no effect on the database schema and can be misleading. The cascade behavior for deletions is defined on the @ManyToOne side of the relationship, which is correctly set in workspace-target.entity.ts. This option should be removed to avoid confusion.
| const newWorkspaceId = randomUUID(); | ||
|
|
There was a problem hiding this comment.
Part 1 of 3: Manually generating the UUID is unnecessary as the database is configured to do this automatically. This can be removed. Please see the subsequent comments for the rest of this refactoring.
| const newWorkspaceId = randomUUID(); | ||
|
|
||
| const newWorkspace = await this.repo.save({ | ||
| id: newWorkspaceId, |
There was a problem hiding this comment.
Part 2 of 3: This line can be removed as the database will generate the ID.
| await this.notificationsService.createNotification({ | ||
| recipients: [id], | ||
| scope: NotificationScope.USER, | ||
| workspaceId: newWorkspaceId, |
There was a problem hiding this comment.
Part 3 of 3: Since the manually generated ID is removed, you should use the ID from the created workspace object (newWorkspace.id).
| workspaceId: newWorkspaceId, | |
| workspaceId: newWorkspace.id, |
No description provided.