Skip to content

Commit e3b9555

Browse files
committed
fix(admin/spa): wire AbortSignal through every read API + minor polish
Two converging review findings (Claude bot + Gemini Code Assist): 1. AbortSignal not plumbed through listTables / describeTable / listBuckets / describeBucket. useApiQuery created an AbortController and passed signal to the loader, but those four methods discarded it - so requests ran to completion after a component unmounted, wasting network and server connections. Now every read method mirrors cluster's signature (`signal?: AbortSignal`) and every page-level useApiQuery callsite passes it through. 2. The safe() wrapper in Dashboard.tsx was a no-op that received `signal` and never passed it on. The "404 doesn't cancel render" suppression it claimed to provide is already handled by SummaryCard testing `error?.status === 404 && pendingMessage`. Removed. Drive-by polish from the same reviews: - useApiQuery's effect deps no longer include `markUnauthorized`. Read it through markUnauthorizedRef so an AuthProvider remount (React Fast Refresh in dev, or any future wrapping change) does not invalidate every active query and trigger a fresh round-trip on every page. Mirrors the existing loaderRef pattern. - Login.tsx now trims `secretKey` in addition to `accessKey`. SigV4 secrets never contain whitespace, so trimming is safe and saves an operator from a cryptic 401 after pasting a key with a trailing newline. Modal accessibility (no focus trap, missing aria-modal / role=dialog) was flagged as low-priority polish for an internal operator tool. Tracked as a follow-up; not addressed here to keep this change focused on the functional bugs. Verified with `npm run build` and `npm run lint` (tsc --strict --noUnusedLocals --noUncheckedSideEffectImports), and `go build ./...`.
1 parent ca45c5e commit e3b9555

8 files changed

Lines changed: 31 additions & 24 deletions

File tree

web/admin/src/api/client.ts

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -202,18 +202,18 @@ export const api = {
202202
logout: () => apiFetch<void>("/auth/logout", { method: "POST" }),
203203
cluster: (signal?: AbortSignal) =>
204204
apiFetch<ClusterInfo>("/cluster", { signal }),
205-
listTables: (next_token?: string) =>
206-
apiFetch<DynamoTableList>("/dynamo/tables", { query: { next_token } }),
207-
describeTable: (name: string) =>
208-
apiFetch<DynamoTable>(`/dynamo/tables/${encodeURIComponent(name)}`),
205+
listTables: (next_token?: string, signal?: AbortSignal) =>
206+
apiFetch<DynamoTableList>("/dynamo/tables", { query: { next_token }, signal }),
207+
describeTable: (name: string, signal?: AbortSignal) =>
208+
apiFetch<DynamoTable>(`/dynamo/tables/${encodeURIComponent(name)}`, { signal }),
209209
createTable: (req: CreateTableRequest) =>
210210
apiFetch<DynamoTable>("/dynamo/tables", { method: "POST", body: req as unknown as Json }),
211211
deleteTable: (name: string) =>
212212
apiFetch<void>(`/dynamo/tables/${encodeURIComponent(name)}`, { method: "DELETE" }),
213-
listBuckets: (next_token?: string) =>
214-
apiFetch<S3BucketList>("/s3/buckets", { query: { next_token } }),
215-
describeBucket: (name: string) =>
216-
apiFetch<S3Bucket>(`/s3/buckets/${encodeURIComponent(name)}`),
213+
listBuckets: (next_token?: string, signal?: AbortSignal) =>
214+
apiFetch<S3BucketList>("/s3/buckets", { query: { next_token }, signal }),
215+
describeBucket: (name: string, signal?: AbortSignal) =>
216+
apiFetch<S3Bucket>(`/s3/buckets/${encodeURIComponent(name)}`, { signal }),
217217
createBucket: (req: CreateBucketRequest) =>
218218
apiFetch<S3Bucket>("/s3/buckets", { method: "POST", body: req as unknown as Json }),
219219
putBucketAcl: (name: string, acl: "private" | "public-read") =>

web/admin/src/lib/useApi.ts

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,13 @@ export function useApiQuery<T>(
2323
const [tick, setTick] = useState(0);
2424
const loaderRef = useRef(loader);
2525
loaderRef.current = loader;
26+
// markUnauthorizedRef mirrors loaderRef for the same reason: keep the
27+
// 401 reaction out of the effect's dep list so a transient identity
28+
// change in AuthProvider (e.g. React Fast Refresh in dev, or a future
29+
// wrapping change) doesn't invalidate every active query and trigger
30+
// a fresh network round-trip on every page.
31+
const markUnauthorizedRef = useRef(markUnauthorized);
32+
markUnauthorizedRef.current = markUnauthorized;
2633

2734
useEffect(() => {
2835
const ctrl = new AbortController();
@@ -40,7 +47,7 @@ export function useApiQuery<T>(
4047
if (cancelled) return;
4148
if (ctrl.signal.aborted) return;
4249
if (err instanceof ApiError) {
43-
if (err.status === 401) markUnauthorized();
50+
if (err.status === 401) markUnauthorizedRef.current();
4451
setError(err);
4552
} else {
4653
setError(new ApiError(0, "network_error", String(err)));
@@ -54,8 +61,10 @@ export function useApiQuery<T>(
5461
// The loader itself is intentionally NOT in the dep list: callers
5562
// pass an inline arrow and the explicit deps array models the real
5663
// input set. Also include `tick` so reload() forces a refetch.
64+
// markUnauthorized is read through markUnauthorizedRef so it does
65+
// not need to be a dep either.
5766
// eslint-disable-next-line react-hooks/exhaustive-deps
58-
}, [...deps, tick, markUnauthorized]);
67+
}, [...deps, tick]);
5968

6069
const reload = useCallback(() => setTick((t) => t + 1), []);
6170
return { data, error, loading, reload };

web/admin/src/pages/Dashboard.tsx

Lines changed: 4 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,11 @@
1-
import { ApiError, api } from "../api/client";
1+
import type { ApiError } from "../api/client";
2+
import { api } from "../api/client";
23
import { formatApiError, useApiQuery } from "../lib/useApi";
34

45
export function DashboardPage() {
56
const cluster = useApiQuery((signal) => api.cluster(signal), []);
6-
const tables = useApiQuery((signal) => safe(() => api.listTables(), signal), []);
7-
const buckets = useApiQuery((signal) => safe(() => api.listBuckets(), signal), []);
7+
const tables = useApiQuery((signal) => api.listTables(undefined, signal), []);
8+
const buckets = useApiQuery((signal) => api.listBuckets(undefined, signal), []);
89

910
return (
1011
<div className="space-y-6">
@@ -125,9 +126,3 @@ function SummaryCard({ label, value, loading, error, pendingMessage }: SummaryCa
125126
);
126127
}
127128

128-
// safe wraps a promise so a 404 from a not-yet-wired backend route
129-
// does not cancel the dashboard render. Other errors propagate.
130-
function safe<T>(fn: () => Promise<T>, signal?: AbortSignal): Promise<T> {
131-
if (signal?.aborted) return Promise.reject(new ApiError(0, "aborted", ""));
132-
return fn();
133-
}

web/admin/src/pages/DynamoDetail.tsx

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@ import { formatApiError, useApiQuery } from "../lib/useApi";
88
export function DynamoDetailPage() {
99
const { name = "" } = useParams<{ name: string }>();
1010
const { session } = useAuth();
11-
const detail = useApiQuery((_signal) => api.describeTable(name), [name]);
11+
const detail = useApiQuery((signal) => api.describeTable(name, signal), [name]);
1212
const [confirmDelete, setConfirmDelete] = useState(false);
1313
const [deleting, setDeleting] = useState(false);
1414
const [deleteError, setDeleteError] = useState<string | null>(null);

web/admin/src/pages/DynamoList.tsx

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@ import { formatApiError, useApiQuery } from "../lib/useApi";
77

88
export function DynamoListPage() {
99
const { session } = useAuth();
10-
const tables = useApiQuery((_signal) => api.listTables(undefined), []);
10+
const tables = useApiQuery((signal) => api.listTables(undefined, signal), []);
1111
const [open, setOpen] = useState(false);
1212
const writeAllowed = session?.role === "full";
1313

web/admin/src/pages/Login.tsx

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,10 @@ export function LoginPage() {
2525
e.preventDefault();
2626
setError(null);
2727
try {
28-
await login(accessKey.trim(), secretKey);
28+
// Trim both sides: SigV4 secret keys never contain whitespace, so
29+
// trimming is safe and saves an operator who pasted a key with
30+
// trailing newline from a cryptic 401.
31+
await login(accessKey.trim(), secretKey.trim());
2932
const from = (location.state as LocationState | null)?.from ?? "/";
3033
navigate(from, { replace: true });
3134
} catch (err) {

web/admin/src/pages/S3Detail.tsx

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@ import { formatApiError, useApiQuery } from "../lib/useApi";
88
export function S3DetailPage() {
99
const { name = "" } = useParams<{ name: string }>();
1010
const { session } = useAuth();
11-
const detail = useApiQuery((_signal) => api.describeBucket(name), [name]);
11+
const detail = useApiQuery((signal) => api.describeBucket(name, signal), [name]);
1212
const [aclBusy, setAclBusy] = useState(false);
1313
const [aclError, setAclError] = useState<string | null>(null);
1414
const [confirmDelete, setConfirmDelete] = useState(false);

web/admin/src/pages/S3List.tsx

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@ import { formatApiError, useApiQuery } from "../lib/useApi";
77

88
export function S3ListPage() {
99
const { session } = useAuth();
10-
const buckets = useApiQuery((_signal) => api.listBuckets(), []);
10+
const buckets = useApiQuery((signal) => api.listBuckets(undefined, signal), []);
1111
const [open, setOpen] = useState(false);
1212
const writeAllowed = session?.role === "full";
1313

0 commit comments

Comments
 (0)