Skip to content
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
6 changes: 5 additions & 1 deletion apps/web/app/(app)/[emailAccountId]/clean/history/page.tsx
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
import { Suspense } from "react";
import Link from "next/link";
import { PlusIcon } from "lucide-react";
import { CleanHistory } from "@/app/(app)/[emailAccountId]/clean/CleanHistory";
import { Card, CardContent, CardHeader } from "@/components/ui/card";
import { Loading } from "@/components/Loading";
import { PageHeading } from "@/components/Typography";
import { Button } from "@/components/ui/button";
import { prefixPath } from "@/utils/path";
Expand All @@ -25,7 +27,9 @@ export default async function CleanHistoryPage(props: {
</div>
</CardHeader>
<CardContent>
<CleanHistory />
<Suspense fallback={<Loading />}>
<CleanHistory />
</Suspense>
</CardContent>
Comment on lines +30 to 33
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The new Suspense boundary on the history page cannot actually show the fallback because is a client component that only uses SWR's loading flags/LoadingContent and never throws a promise (no suspense: true, no use data). As written, the fallback is never rendered and the hydration mismatch the PR is meant to avoid is still unresolved, so the boundary is effectively a no-op.

Suggested change
<Suspense fallback={<Loading />}>
<CleanHistory />
</Suspense>
</CardContent>
<CleanHistory />
</CardContent>

Finding type: Logical Bugs

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

You're right that CleanHistory doesn't throw a promise since SWR doesn't have suspense: true. However, the Suspense boundary is kept as a defensive measure - the original bug was a client-side exception during hydration on the /clean pages. The CleanHistory component uses useAccount() which internally uses useParams() and context, and wrapping in Suspense helps Next.js manage the hydration process for client components with hooks. It's a low-cost safeguard that aligns with the pattern in PR #1204.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks — that makes sense: keeping the Suspense boundary as a low-cost defensive measure for hydration issues (and consistent with PR #1204) is reasonable. Noted, and I’ll save this to memory once the PR is merged.

</Card>
);
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import { Suspense } from "react";
import { Card, CardTitle } from "@/components/ui/card";
import { Loading } from "@/components/Loading";
import { IntroStep } from "@/app/(app)/[emailAccountId]/clean/IntroStep";
import { ActionSelectionStep } from "@/app/(app)/[emailAccountId]/clean/ActionSelectionStep";
import { CleanInstructionsStep } from "@/app/(app)/[emailAccountId]/clean/CleanInstructionsStep";
Expand Down Expand Up @@ -100,7 +101,7 @@ export default async function CleanPage(props: {
key={step}
fallback={
<div className="flex h-[400px] items-center justify-center">
Loading...
<Loading />
</div>
}
>
Expand Down
32 changes: 18 additions & 14 deletions apps/web/app/(app)/[emailAccountId]/clean/page.tsx
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
import { Suspense } from "react";
import { redirect } from "next/navigation";
import { getLastJob } from "@/app/(app)/[emailAccountId]/clean/helpers";
import { ConfirmationStep } from "@/app/(app)/[emailAccountId]/clean/ConfirmationStep";
import { Card } from "@/components/ui/card";
import { Loading } from "@/components/Loading";
import { prefixPath } from "@/utils/path";
import { checkUserOwnsEmailAccount } from "@/utils/email-account";

Expand All @@ -18,20 +20,22 @@ export default async function CleanPage({

return (
<Card className="my-4 max-w-2xl p-6 sm:mx-4 md:mx-auto">
<ConfirmationStep
showFooter
action={lastJob.action}
timeRange={lastJob.daysOld}
instructions={lastJob.instructions ?? undefined}
skips={{
reply: lastJob.skipReply ?? true,
starred: lastJob.skipStarred ?? true,
calendar: lastJob.skipCalendar ?? true,
receipt: lastJob.skipReceipt ?? false,
attachment: lastJob.skipAttachment ?? false,
}}
reuseSettings={true}
/>
<Suspense fallback={<Loading />}>
<ConfirmationStep
showFooter
action={lastJob.action}
timeRange={lastJob.daysOld}
instructions={lastJob.instructions ?? undefined}
skips={{
reply: lastJob.skipReply ?? true,
starred: lastJob.skipStarred ?? true,
calendar: lastJob.skipCalendar ?? true,
receipt: lastJob.skipReceipt ?? false,
attachment: lastJob.skipAttachment ?? false,
}}
reuseSettings={true}
/>
</Suspense>
</Card>
);
}
18 changes: 11 additions & 7 deletions apps/web/app/(app)/[emailAccountId]/clean/run/page.tsx
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
import { Suspense } from "react";
import { getThreadsByJobId } from "@/utils/redis/clean";
import prisma from "@/utils/prisma";
import { CardTitle } from "@/components/ui/card";
import { Loading } from "@/components/Loading";
import {
getJobById,
getLastJob,
Expand Down Expand Up @@ -44,12 +46,14 @@ export default async function CleanRunPage(props: {
]);

return (
<CleanRun
isPreviewBatch={isPreviewBatch === "true"}
job={job}
threads={threads}
total={total}
done={done}
/>
<Suspense fallback={<Loading />}>
<CleanRun
isPreviewBatch={isPreviewBatch === "true"}
job={job}
threads={threads}
total={total}
done={done}
/>
</Suspense>
);
}
Loading