Skip to content

Commit 3809e65

Browse files
committed
fix(web): apiFetch resilience + auth state guards + double-submit + notification abort
- api.apiFetch: catch network failures (DNS, offline, abort) and wrap as ApiError("network_error", 0) instead of letting the raw TypeError fall through every call site's instanceof check as an opaque server error. - api.apiFetch: parse body via text() + safeParseJson so non-JSON responses (proxy HTML error pages, empty 5xx bodies) no longer throw SyntaxError that bypasses the typed error contract. - auth.signOut: try/catch around supabase.auth.signOut(). The signOut call can reject when the refresh token is already revoked or the network is down; without the catch the user is stuck on a half-broken page. The window.location redirect runs unconditionally. - main.bootstrap: try/catch around exchangeCodeForSession. A bad/expired OAuth code threw and prevented initAuth() from running, leaving the app stuck in status: "loading" — every protected route's beforeLoad treats that as authorized and the user stared at a blank screen. - _authed: treat any status other than "authenticated" as not-yet-authorized in both beforeLoad and the layout. Render null until authenticated so we don't briefly render protected children with user === null. - useNotifications: AbortController per refetch — cancel the in-flight request when a new one starts (visibility-change-then-timer race) and on unmount. Without this, slow first responses can clobber fast second responses since both call setItems. - room-card / room-row: committingRef guard on the inline-rename commit(). Pressing Enter dispatches the PATCH and synchronously closes the input, which fires onBlur and would otherwise issue a duplicate PATCH with the same value. - delete-room-dialog: e.preventDefault() on the destructive AlertDialogAction so Radix doesn't close the dialog before confirm() runs — preserving the spinner state and keeping the confirmation visible if delete fails. Code review pass 5.
1 parent 6d2b47f commit 3809e65

8 files changed

Lines changed: 119 additions & 31 deletions

File tree

apps/web/src/components/notifications/use-notifications.ts

Lines changed: 15 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -13,21 +13,31 @@ export function useNotifications() {
1313
// on an unmounted component (React warning + wasted render). Set inside the
1414
// mount effect below; the closure here captures it.
1515
const mountedRef = useRef(true);
16+
// Aborts the in-flight refetch when a new one starts (visibility change
17+
// triggers an immediate refetch while the polling timer fired one moments
18+
// before). Without this, a slow first response can clobber a fast second
19+
// response since both call setItems.
20+
const ctrlRef = useRef<AbortController | null>(null);
1621

1722
async function refetch() {
1823
const token = useSession.getState().token;
1924
if (!token) return;
25+
ctrlRef.current?.abort();
26+
const ctrl = new AbortController();
27+
ctrlRef.current = ctrl;
2028
if (mountedRef.current) setLoading(true);
2129
try {
22-
const data = await apiFetch<ListNotificationsResponse>("/api/notifications");
23-
if (!mountedRef.current) return;
30+
const data = await apiFetch<ListNotificationsResponse>("/api/notifications", {
31+
signal: ctrl.signal,
32+
});
33+
if (!mountedRef.current || ctrl.signal.aborted) return;
2434
setItems(data.notifications);
2535
setUnreadCount(data.unreadCount);
2636
failCountRef.current = 0;
2737
} catch {
28-
if (mountedRef.current) failCountRef.current++;
38+
if (mountedRef.current && !ctrl.signal.aborted) failCountRef.current++;
2939
} finally {
30-
if (mountedRef.current) setLoading(false);
40+
if (mountedRef.current && !ctrl.signal.aborted) setLoading(false);
3141
}
3242
}
3343

@@ -79,6 +89,7 @@ export function useNotifications() {
7989
stopped = true;
8090
mountedRef.current = false;
8191
if (timeoutId) clearTimeout(timeoutId);
92+
ctrlRef.current?.abort();
8293
document.removeEventListener("visibilitychange", onVis);
8394
};
8495
}, []);

apps/web/src/components/rooms/delete-room-dialog.tsx

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -52,6 +52,11 @@ export function DeleteRoomDialog({ open, onOpenChange, slug }: Props) {
5252
<AlertDialogCancel>Cancel</AlertDialogCancel>
5353
<AlertDialogAction
5454
onClick={(e) => {
55+
// preventDefault stops Radix from closing the dialog on click.
56+
// Without it the dialog unmounts immediately, the spinner state
57+
// is never visible, and a failed delete loses the confirmation
58+
// affordance. confirm() closes the dialog on success itself.
59+
e.preventDefault();
5560
e.stopPropagation();
5661
confirm();
5762
}}

apps/web/src/components/rooms/room-card.tsx

Lines changed: 18 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -71,16 +71,27 @@ export function RoomCard({
7171
}
7272
}
7373

74+
// Guards against the Enter-then-blur double-submit: pressing Enter calls
75+
// commit(), which dispatches the PATCH and synchronously calls setEditing(false).
76+
// Unmounting the <input> fires onBlur, which would call commit() again
77+
// with the same value and issue a duplicate PATCH.
78+
const committingRef = useRef(false);
7479
async function commit() {
80+
if (committingRef.current) return;
81+
committingRef.current = true;
7582
const next = draft.trim();
76-
if (next !== (room.name ?? "")) {
77-
const res = await apiFetch<UpdateRoomResponse>(`/api/rooms/${room.slug}`, {
78-
method: "PATCH",
79-
body: { name: next || null },
80-
});
81-
updateRoom(res.room);
83+
try {
84+
if (next !== (room.name ?? "")) {
85+
const res = await apiFetch<UpdateRoomResponse>(`/api/rooms/${room.slug}`, {
86+
method: "PATCH",
87+
body: { name: next || null },
88+
});
89+
updateRoom(res.room);
90+
}
91+
setEditing(false);
92+
} finally {
93+
committingRef.current = false;
8294
}
83-
setEditing(false);
8495
}
8596

8697
return (

apps/web/src/components/rooms/room-row.tsx

Lines changed: 15 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -71,16 +71,24 @@ export function RoomRow({
7171
}
7272
}
7373

74+
// Same Enter-then-blur double-submit guard as room-title.tsx.
75+
const committingRef = useRef(false);
7476
async function commit() {
77+
if (committingRef.current) return;
78+
committingRef.current = true;
7579
const next = draft.trim();
76-
if (next !== (room.name ?? "")) {
77-
const res = await apiFetch<UpdateRoomResponse>(`/api/rooms/${room.slug}`, {
78-
method: "PATCH",
79-
body: { name: next || null },
80-
});
81-
updateRoom(res.room);
80+
try {
81+
if (next !== (room.name ?? "")) {
82+
const res = await apiFetch<UpdateRoomResponse>(`/api/rooms/${room.slug}`, {
83+
method: "PATCH",
84+
body: { name: next || null },
85+
});
86+
updateRoom(res.room);
87+
}
88+
setEditing(false);
89+
} finally {
90+
committingRef.current = false;
8291
}
83-
setEditing(false);
8492
}
8593

8694
return (

apps/web/src/lib/api.ts

Lines changed: 33 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -37,17 +37,34 @@ async function runRefresh(): Promise<{ ok: boolean }> {
3737
return { ok: false };
3838
}
3939

40+
function safeParseJson(text: string): unknown {
41+
try {
42+
return JSON.parse(text);
43+
} catch {
44+
return null;
45+
}
46+
}
47+
4048
export async function apiFetch<T>(path: string, opts: FetchOpts = {}): Promise<T> {
4149
const token = useSession.getState().token;
4250
const headers = new Headers(opts.headers);
4351
if (opts.body !== undefined) headers.set("Content-Type", "application/json");
4452
if (token) headers.set("Authorization", `Bearer ${token}`);
4553

46-
const res = await fetch(`${env.VITE_API_URL}${path}`, {
47-
...opts,
48-
headers,
49-
body: opts.body !== undefined ? JSON.stringify(opts.body) : undefined,
50-
});
54+
let res: Response;
55+
try {
56+
res = await fetch(`${env.VITE_API_URL}${path}`, {
57+
...opts,
58+
headers,
59+
body: opts.body !== undefined ? JSON.stringify(opts.body) : undefined,
60+
});
61+
} catch (err) {
62+
// Network failure (DNS, offline, CORS preflight, abort, etc.). Without
63+
// this branch the raw TypeError propagates as a non-ApiError and every
64+
// call site's `instanceof ApiError` check falls through to a generic
65+
// "server error" message with no signal it's a connectivity issue.
66+
throw new ApiError("network_error", "Network request failed", 0, err);
67+
}
5168

5269
if (res.status === 401 && !opts._retried) {
5370
if (!refreshInFlight) {
@@ -62,10 +79,18 @@ export async function apiFetch<T>(path: string, opts: FetchOpts = {}): Promise<T
6279
throw new ApiError("unauthorized", "Session expired", 401);
6380
}
6481
if (res.status === 204) return undefined as T;
65-
const json = await res.json();
82+
83+
// Body may be empty (some servers return no body on 5xx) or non-JSON (a
84+
// proxy returning an HTML error page). Parse defensively so the helper
85+
// always throws a typed ApiError instead of a SyntaxError from .json().
86+
const text = await res.text();
87+
const json = text ? safeParseJson(text) : null;
88+
6689
if (!res.ok) {
67-
const body = json as ErrorEnvelope;
68-
throw new ApiError(body.error.code, body.error.message, res.status, json);
90+
const body = json as ErrorEnvelope | null;
91+
const code = body?.error?.code ?? "server_error";
92+
const message = body?.error?.message ?? res.statusText ?? "Request failed";
93+
throw new ApiError(code, message, res.status, json ?? undefined);
6994
}
7095
return json as T;
7196
}

apps/web/src/lib/auth.ts

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -100,6 +100,14 @@ export async function linkProvider(provider: "github" | "google", next = "/setti
100100
}
101101

102102
export async function signOut() {
103-
await supabase.auth.signOut();
103+
// signOut() can reject when the refresh token has already been revoked
104+
// server-side or the network is unreachable. Either way, we want the user
105+
// to land on the public home and a fresh page load to clear any in-memory
106+
// session state — don't leave them on a half-broken settings page.
107+
try {
108+
await supabase.auth.signOut();
109+
} catch {
110+
// best-effort — local state is wiped by the redirect below
111+
}
104112
window.location.href = "/";
105113
}

apps/web/src/main.tsx

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,15 @@ async function bootstrap(root: HTMLElement) {
2828
const params = new URLSearchParams(window.location.search);
2929
const code = params.get("code");
3030
if (code) {
31-
await supabase.auth.exchangeCodeForSession(code);
31+
// A bad/expired code throws here. Previously that prevented initAuth()
32+
// from running and left the entire app stuck in status: "loading" — every
33+
// protected route's beforeLoad checks for "anonymous" so the user never
34+
// got redirected and stared at a blank screen.
35+
try {
36+
await supabase.auth.exchangeCodeForSession(code);
37+
} catch {
38+
// fall through to initAuth() so the app boots in anonymous mode
39+
}
3240
params.delete("code");
3341
const clean = params.toString();
3442
const next = window.location.pathname + (clean ? `?${clean}` : "");

apps/web/src/routes/_authed.tsx

Lines changed: 15 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -4,9 +4,14 @@ import { Outlet, createFileRoute, redirect, useNavigate } from "@tanstack/react-
44
import { useEffect } from "react";
55

66
export const Route = createFileRoute("/_authed")({
7+
// bootstrap() awaits initAuth() before rendering, so by the time the router
8+
// runs `status` is either "authenticated" or "anonymous" on the initial
9+
// load. The "loading" state only appears during an in-app session reset
10+
// (rare). Redirect on anything other than "authenticated" so we never
11+
// briefly render a protected child with `user === null`.
712
beforeLoad: ({ location }) => {
813
const { status } = useSession.getState();
9-
if (status === "anonymous") {
14+
if (status !== "authenticated") {
1015
throw redirect({ to: "/sign-in", search: { next: location.pathname } });
1116
}
1217
},
@@ -18,12 +23,19 @@ function AuthedLayout() {
1823
const status = useSession((s) => s.status);
1924
const navigate = useNavigate();
2025

26+
// Mid-session sign-out (token revoked, refresh failed) flips status away
27+
// from "authenticated" — push the user back to /sign-in immediately and
28+
// hold the outlet so we don't render a stale page in the meantime.
2129
useEffect(() => {
22-
if (status === "anonymous") {
23-
navigate({ to: "/sign-in", search: { next: "/dashboard" } });
30+
if (status !== "authenticated") {
31+
navigate({
32+
to: "/sign-in",
33+
search: { next: window.location.pathname || "/dashboard" },
34+
});
2435
}
2536
}, [status, navigate]);
2637

38+
if (status !== "authenticated") return null;
2739
return <Outlet />;
2840
}
2941

0 commit comments

Comments
 (0)