Skip to content
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

Server log out after delete #306

Merged
merged 1 commit into from
Jan 12, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 17 additions & 4 deletions apps/web/app/(app)/settings/DeleteSection.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,13 @@ import {
deleteAccountAction,
resetAnalyticsAction,
} from "@/utils/actions/user";
import { handleActionResult } from "@/utils/server-action";
import { logOut } from "@/utils/user";
import { isActionError } from "@/utils/error";
import { useStatLoader } from "@/providers/StatLoaderProvider";

export function DeleteSection() {
const { onCancelLoadBatch } = useStatLoader();

return (
<FormSection>
<FormSectionLeft
Expand Down Expand Up @@ -45,9 +48,19 @@ export function DeleteSection() {

if (!yes) return;

const result = await deleteAccountAction();
handleActionResult(result, "Account deleted!");
await logOut("/");
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}`,
},
);
Comment on lines +51 to +63
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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.

}}
>
Yes, delete my account
Expand Down
2 changes: 1 addition & 1 deletion apps/web/app/(app)/settings/WebhookSection.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import { RegenerateSecretButton } from "@/app/(app)/settings/WebhookGenerate";
export async function WebhookSection() {
const session = await auth();
const userId = session?.user.id;
if (!userId) throw new Error("Not authenticated");
if (!userId) return null;

const user = await prisma.user.findUnique({
where: { id: userId },
Expand Down
1 change: 0 additions & 1 deletion apps/web/app/(landing)/login/page.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@ export default async function AuthenticationPage({
searchParams?: Record<string, string>;
}) {
const session = await auth();
console.log("🚀 ~ session:", session?.user);
if (session?.user.email && !searchParams?.error) {
if (searchParams?.next) {
redirect(searchParams?.next);
Expand Down
3 changes: 2 additions & 1 deletion apps/web/app/(landing)/welcome-upgrade/page.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import { Pricing } from "@/app/(app)/premium/Pricing";
import { Footer } from "@/app/(landing)/home/Footer";
import { Loading } from "@/components/Loading";
import { WelcomeUpgradeNav } from "@/app/(landing)/welcome-upgrade/WelcomeUpgradeNav";
import { userCount } from "@/utils/config";

export default function WelcomeUpgradePage() {
return (
Expand All @@ -18,7 +19,7 @@ export default function WelcomeUpgradePage() {
Spend 50% less time on email
</h2>
<p className="mt-2 font-cal text-2xl text-gray-900 sm:text-3xl">
Join 9,000+ users that use Inbox Zero
Join {userCount} users that use Inbox Zero
<br />
to be more productive!
</p>
Expand Down
1 change: 1 addition & 0 deletions apps/web/app/api/auth/[...nextauth]/auth.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ const defaultAuthOptions = getAuthOptions();
export const {
handlers: { GET, POST },
auth,
signOut,
} = NextAuth((req) => {
if (req?.url) {
const url = new URL(req?.url);
Expand Down
15 changes: 13 additions & 2 deletions apps/web/providers/StatLoaderProvider.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -23,12 +23,14 @@ type Context = {
loadBefore: boolean;
showToast: boolean;
}) => Promise<void>;
onCancelLoadBatch: () => void;
};

const StatLoaderContext = createContext<Context>({
isLoading: false,
onLoad: async () => {},
onLoadBatch: async () => {},
onCancelLoadBatch: () => {},
});

export const useStatLoader = () => useContext(StatLoaderContext);
Expand Down Expand Up @@ -64,6 +66,7 @@ const statLoader = new StatLoader();

export function StatLoaderProvider(props: { children: React.ReactNode }) {
const [isLoading, setIsLoading] = useState(false);
const [stopLoading, setStopLoading] = useState(false);

const onLoad = useCallback(
async (options: { loadBefore: boolean; showToast: boolean }) => {
Expand All @@ -78,18 +81,26 @@ export function StatLoaderProvider(props: { children: React.ReactNode }) {
async (options: { loadBefore: boolean; showToast: boolean }) => {
const batchSize = 50;
for (let i = 0; i < batchSize; i++) {
if (stopLoading) break;
console.log("Loading batch", i);
await onLoad({
...options,
showToast: options.showToast && i === batchSize - 1,
});
}
setStopLoading(false);
},
[onLoad],
[onLoad, stopLoading],
);

const onCancelLoadBatch = useCallback(() => {
setStopLoading(true);
}, []);

return (
<StatLoaderContext.Provider value={{ isLoading, onLoad, onLoadBatch }}>
<StatLoaderContext.Provider
value={{ isLoading, onLoad, onLoadBatch, onCancelLoadBatch }}
>
{props.children}
</StatLoaderContext.Provider>
);
Expand Down
8 changes: 6 additions & 2 deletions apps/web/utils/actions/user.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
import { z } from "zod";
import { deleteContact as deleteLoopsContact } from "@inboxzero/loops";
import { deleteContact as deleteResendContact } from "@inboxzero/resend";
import { auth } from "@/app/api/auth/[...nextauth]/auth";
import { auth, signOut } from "@/app/api/auth/[...nextauth]/auth";
import prisma from "@/utils/prisma";
import { deleteInboxZeroLabels, deleteUserLabels } from "@/utils/redis/label";
import { deleteUserStats } from "@/utils/redis/stats";
Expand Down Expand Up @@ -66,10 +66,14 @@ export const deleteAccountAction = withActionInstrumentation(
}),
]);
} catch (error) {
console.error("Error while deleting account: ", error);
console.error("Error while deleting account:", error);
captureException(error, undefined, session.user.email);
}

try {
await signOut();
} catch (error) {}

Comment on lines +73 to +76
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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

await prisma.user.delete({ where: { email: session.user.email } });
},
);
Expand Down
Loading