Server log out after delete #306
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThis pull request introduces several modifications across different components of the web application. The changes primarily focus on enhancing error handling, user experience, and configuration management. Key updates include adding a cancellable batch loading mechanism in the Changes
Sequence DiagramsequenceDiagram
participant User
participant DeleteSection
participant deleteAccountAction
participant AuthModule
User->>DeleteSection: Initiate Account Deletion
DeleteSection->>deleteAccountAction: Call Deletion Action
deleteAccountAction-->>DeleteSection: Return Result
alt Deletion Successful
DeleteSection->>AuthModule: Sign Out
DeleteSection->>User: Show Success Toast
else Deletion Failed
DeleteSection->>User: Show Error Toast
end
Possibly related PRs
Poem
Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
apps/web/providers/StatLoaderProvider.tsx (1)
84-91: Consider adding a loading state resetThe batch loading cancellation is implemented correctly. However, consider resetting the
isLoadingstate when cancellation occurs to ensure UI consistency.const onLoadBatch = useCallback( async (options: { loadBefore: boolean; showToast: boolean }) => { const batchSize = 50; for (let i = 0; i < batchSize; i++) { - if (stopLoading) break; + if (stopLoading) { + setIsLoading(false); + break; + } console.log("Loading batch", i); await onLoad({ ...options, showToast: options.showToast && i === batchSize - 1, }); } setStopLoading(false); }, [onLoad, stopLoading], );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
apps/web/app/(app)/settings/DeleteSection.tsx(2 hunks)apps/web/app/(app)/settings/WebhookSection.tsx(1 hunks)apps/web/app/(landing)/login/page.tsx(0 hunks)apps/web/app/(landing)/welcome-upgrade/page.tsx(2 hunks)apps/web/app/api/auth/[...nextauth]/auth.ts(1 hunks)apps/web/providers/StatLoaderProvider.tsx(3 hunks)apps/web/utils/actions/user.ts(2 hunks)
💤 Files with no reviewable changes (1)
- apps/web/app/(landing)/login/page.tsx
🔇 Additional comments (10)
apps/web/app/api/auth/[...nextauth]/auth.ts (1)
12-12: LGTM! Export ofsignOutfunction aligns with PR objective.The addition of the
signOutexport enables proper user session cleanup after account deletion.apps/web/app/(app)/settings/WebhookSection.tsx (1)
11-11: LGTM! Improved error handling for unauthenticated state.Gracefully returning
nullinstead of throwing an error is a better approach, especially for handling post-deletion states where the user session may be invalid.apps/web/app/(landing)/welcome-upgrade/page.tsx (1)
7-7: Verify the relationship with PR objective.While the change to use a dynamic
userCountfrom config improves maintainability, it seems unrelated to the PR's objective of implementing server logout after account deletion. Consider splitting this change into a separate PR for better change tracking.Also applies to: 22-22
apps/web/app/(app)/settings/DeleteSection.tsx (1)
11-12: LGTM! Added necessary imports and hooks.The addition of
isActionErroranduseStatLoaderprovides proper error handling and stat loading management capabilities.Also applies to: 15-15
apps/web/providers/StatLoaderProvider.tsx (4)
26-26: LGTM: Context type and default value updatesThe new
onCancelLoadBatchmethod is properly typed and initialized with a no-op function as the default value.Also applies to: 33-33
69-69: LGTM: State management for cancellationThe
stopLoadingstate is correctly initialized as a boolean flag.
96-98: LGTM: Cancel handler implementationThe cancel handler is correctly implemented with proper dependency management using
useCallback.
101-103: LGTM: Context provider value updateThe provider value is correctly updated to include the new
onCancelLoadBatchmethod.apps/web/utils/actions/user.ts (2)
6-6: LGTM: Import additionThe
signOutfunction is correctly imported alongside the existingauthimport.
69-69: LGTM: Error logging improvementThe error logging format is improved for better readability.
| toast.promise( | ||
| async () => { | ||
| onCancelLoadBatch(); | ||
| const result = await deleteAccountAction(); | ||
| await logOut("/"); | ||
| if (isActionError(result)) throw new Error(result.error); | ||
| }, | ||
| { | ||
| loading: "Deleting account...", | ||
| success: "Account deleted!", | ||
| error: (err) => `Error deleting account: ${err.message}`, | ||
| }, | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider potential race conditions in the deletion flow.
The current implementation has a good flow with proper user feedback through toasts. However, there's a potential race condition between account deletion and logout. If the deletion succeeds but logout fails, the user might be left in an inconsistent state.
Consider wrapping both operations in a single try-catch:
toast.promise(
async () => {
onCancelLoadBatch();
- const result = await deleteAccountAction();
- await logOut("/");
- if (isActionError(result)) throw new Error(result.error);
+ try {
+ const result = await deleteAccountAction();
+ if (isActionError(result)) throw new Error(result.error);
+ await logOut("/");
+ } catch (error) {
+ // If deletion succeeded but logout failed, we should still consider it a success
+ // as the account is deleted and the user can manually logout
+ if (error.message.includes("deletion successful")) {
+ console.error("Account deleted but logout failed:", error);
+ return;
+ }
+ throw error;
+ }
},
{
loading: "Deleting account...",
success: "Account deleted!",
error: (err) => `Error deleting account: ${err.message}`,
},
);Committable suggestion skipped: line range outside the PR's diff.
| try { | ||
| await signOut(); | ||
| } catch (error) {} | ||
|
|
There was a problem hiding this comment.
Critical: Reorder signOut after user deletion
The current implementation calls signOut() before deleting the user from the database. This could lead to an inconsistent state if the database deletion fails. Consider moving the signOut call after the successful user deletion.
Also, the empty catch block should at least log the error to help with debugging.
- try {
- await signOut();
- } catch (error) {}
-
await prisma.user.delete({ where: { email: session.user.email } });
+
+ try {
+ await signOut();
+ } catch (error) {
+ console.error("Error during sign out after account deletion:", error);
+ // Don't rethrow as the account is already deleted
+ }Also applies to: 78-78
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Chores
Performance