# Code Review — Polling Flow & Results Screen **Scope:** Read-only review of frontend polling + result rendering. **Files reviewed:** - `apps/web/lib/use-inspection-polling.ts` - `apps/web/lib/api.ts` - `apps/web/lib/auth-context.tsx` - `apps/web/lib/uploaded-previews.ts` - `apps/web/lib/jwt.ts` - `apps/web/app/results/[id]/page.tsx` - `apps/web/app/inspect/page.tsx` - `apps/web/app/(app)/inspect/new/page.tsx` - `apps/web/middleware.ts` - `packages/ui/src/components/UploadDropzone.tsx` - `apps/web/components/ResultsTabs.tsx` > Note: there is **no** `apps/web/app/(app)/results/[id]/page.tsx`. Results > live only under `app/results/[id]/page.tsx`, which means the page is > *not* matched by `PROTECTED_PREFIXES` in `middleware.ts`. See **HIGH-3**. --- ## Overall impression The polling hook is overall solid — it has an `AbortController`, a `cancelledRef`, exponential backoff, a max-duration budget, a consecutive-error budget, and distinguishes "fatal" vs "transient" errors. Cross-tab refresh dedup in `api.ts` is thoughtful. The Tauri-style cleanup in `inspect/new/page.tsx` (abort on unmount) is well done. That said, there are real bugs around **stale `consecutiveErrors`/`currentInterval`** when the `retryNonce`/effect re-runs, a **silent token-refresh failure mode** where a follower tab returns the *old* access token, and a **known and still-unfixed `URL.createObjectURL` leak** in `UploadDropzone.tsx` (the comments claim it's revoked, but the hook is dead code). The list below is prioritized; the Top 5 section at the end is the order I would fix things in. --- ## CRITICAL ### CRITICAL-1 — `useFilePreviewUrls` is dead code; `FilePreview` leaks blob URLs on every render **Where:** `packages/ui/src/components/UploadDropzone.tsx:123-180` The `useFilePreviewUrls` hook (lines 123-139) does call `URL.revokeObjectURL` in a cleanup, *but* `FilePreview` is also imported / used in two places that mutate `files` array identity on every keystroke / state change in the parent (`apps/web/app/inspect/page.tsx`, `apps/web/app/(app)/inspect/new/page.tsx`). Concretely: - `inspect/page.tsx:33-46` rebuilds `files` (new array reference) via `setFiles((prev) => [...prev, ...incoming]...)` each `onFiles` call. - The dedup logic returns a **new array even when nothing changed** (e.g. user re-drops the same file). Every new identity invalidates the `useMemo([files])`, which calls `URL.createObjectURL` again for every surviving file, leaking the previous URLs until effect cleanup later. - The cleanup runs only when `urls` identity changes — but by that point a new `urls` already exists, so the cleanup fires for the *new* set, not the leaked previous set. (`useEffect`'s cleanup uses the *previous* closure, but the leak is across renders where memo recomputes — and because `[files]` identity gates it, blob URLs that were already recreated for the same `File` reference are unconditionally orphaned.) `docs/WEB_PERFORMANCE.md:133-147` and `docs/SECURITY_WEB_AUDIT.md:145-151` both flag this. The fix the docs describe was supposedly applied, but the current code path still allocates a fresh URL per memo recompute without deduping by `File` identity. **Why it matters:** 20 photos × ~5 MB = 100 MB of blob memory per leak cycle. Long sessions on `/inspect/new` can crash mobile Safari and degrade desktop Chrome (multi-100 MB). **Suggestion (DO NOT IMPLEMENT — review only):** - Cache URLs in a `Map` ref keyed by `File` identity, and only allocate for files newly added since the last render. Revoke when a `File` leaves the list **or** on unmount. - Add a unit test that asserts `URL.createObjectURL` call count equals `files.length` after N re-renders with stable file identities. --- ### CRITICAL-2 — Polling effect resets `consecutiveErrors`/`currentInterval` but state `attempts` is preserved across `retry()`, producing inconsistent UX **Where:** `apps/web/lib/use-inspection-polling.ts:87-103` `retry()` increments `retryNonce`, which re-runs the entire effect. Inside the effect: - `consecutiveErrors = 0` (line 107) and `currentInterval = intervalMs` (line 106) reset on every run — good. - But `setState((s) => ({ ... attempts: s.attempts ... }))` at line 100 preserves `attempts`. After several manual retries, `attempts` keeps climbing across resets while `consecutiveErrors` does not. That's not a bug per se, but combined with the `paused` flag rendering at `results/[id]/page.tsx`, the user sees the `pollAttempts` counter (line 328) growing without bound across retries. There is **no `paused` branch** rendered on the results page at all — when polling pauses (8 consecutive errors), the UI shows the `` *plus* the `` spinner forever. There is no "Check again" button as the hook's docstring (line 51) promises. **Why it matters:** The contract documented at `use-inspection-polling.ts:51` ("UI should offer a 'check again' button") is **not implemented** in `results/[id]/page.tsx`. Once paused, the user has no way to call `retry()`. They have to reload the tab, which dumps any optimistic state. **Suggestion:** - Wire `paused` and `retry` from the hook into the results page and render a "Check again" button (analogous to the `timedOut` branch at line 80). - Either persist `attempts` across retries (current behavior) and label it "Total checks" so the growing number is intentional, or reset it on `retry()` and label it "Checks this session." --- ### CRITICAL-3 — Cross-tab refresh follower can race and return a token *previousAccess === current → null* even when leader succeeds **Where:** `apps/web/lib/api.ts:160-225` In `runRefresh()`: 1. `previousAccess = getStoredAccessToken()` is read **before** `tryAcquireRefreshLock()`. 2. If we lose the lock, we call `waitForLeaderRefresh(previousAccess)`. 3. The leader in another tab calls `setStoredTokens(access_token, ...)`, which writes `localStorage` synchronously. **But** if the leader completes between steps 1 and 2 in our tab (very narrow window — but possible because `tryAcquireRefreshLock` does multiple `localStorage` reads), `previousAccess` may *already equal* the new token written by the leader, in which case the `waitForLeaderRefresh` poll never sees `current !== previousToken` and resolves `null` at the 8 s timeout. Equally, `storage` events do not fire in the *writing* tab — that's documented at line 178. But the follower polls every 200 ms, which means the **happy path takes up to 200 ms per request** of pure latency added to the 401 → refresh → retry path in the loser tab. **Why it matters:** Tabs lose refresh attempts → user gets logged out spuriously in multi-tab sessions. Symptom is intermittent and very hard to repro. **Suggestion:** - After acquiring/losing the lock, **re-read the access token** and compare against `previousAccess`. If they differ, the leader already finished — return the new token immediately, skip the wait loop. - Reduce the poll interval to 50 ms or use a `BroadcastChannel`. --- ## HIGH ### HIGH-1 — 401 retry inside axios interceptor does not propagate `signal` **Where:** `apps/web/lib/api.ts:99-110` When a 401 fires, the interceptor calls `instance.request(original)`. The `original` config carries the original `AbortSignal` (good), but if the caller already aborted (e.g. unmounted polling), the abort fires **before** `runRefresh` returns. `instance.request(original)` will then issue a network request with an already-aborted signal — axios will throw `CanceledError`, which `classifyApiError` maps to `cancelled`, which the polling hook silently swallows. Net effect: harmless, but a wasted refresh round-trip on every unmount that happens to coincide with a 401. More importantly: if the user navigates away during refresh, the leader tab can still complete `/auth/refresh` and write fresh tokens to `localStorage` for a session the user thinks is dead. Edge-y, but worth noting. **Suggestion:** Bail out of the 401 retry path if `original.signal?.aborted` before calling `runRefresh()`. --- ### HIGH-2 — `setUnauthorizedHandler(null)` cleanup in `AuthProvider` runs on every `pathname` change → ephemeral window where the handler is unset **Where:** `apps/web/lib/auth-context.tsx:54-68` The effect re-runs whenever `pathname` changes (React Router behavior in app dir is to keep `pathname` stable inside a segment, but transitions DO fire). The cleanup sets the handler to `null`, then the next effect sets a new closure. If a 401 lands in the millisecond gap (during route transition), `_onUnauthorized?.()` is a no-op and the user is left on the page with a cleared session and no redirect. **Why it matters:** Probably rare, but on slow devices or under React 18 concurrent rendering, this gap widens. The symptom is "I'm logged out but the app still shows me the protected page." **Suggestion:** - Either keep the handler permanently registered (use a stable handler that reads `pathname` from a ref), or compute the dependency more narrowly (only re-register if `router` identity changes, not `pathname`). --- ### HIGH-3 — `/results/[id]` is not in `PROTECTED_PREFIXES`, but the API call requires auth **Where:** `apps/web/middleware.ts:4-15`, `apps/web/app/results/[id]/page.tsx:27` `PROTECTED_PREFIXES` lists `/inspect` and `/inspect/new` but **not** `/results`. An unauthenticated user who navigates to `/results/` bypasses middleware, then `useInspectionPolling` fires `GET /api/v1/inspect/`, gets a 401, the axios interceptor tries `runRefresh()` (no refresh token in localStorage → returns `null`), clears tokens (already empty), calls `_onUnauthorized?.()`. The auth context *does* push to `/login` from the protected pages check at line 58-65 — but note the condition: it only redirects if pathname is NOT `/login`, `/register`, or `/`. **`/results/` passes this check**, so the user *does* get redirected — but only after a failed network call, a failed refresh, and one full re-render. Slow and noisy. Also: the FastAPI 401 detail bubbles up as the polling error message (`tHttp('401')`) and is briefly shown to the user before the redirect. **Suggestion:** Add `/results` to `PROTECTED_PREFIXES` so middleware short-circuits at the edge. --- ### HIGH-4 — Polling effect re-runs whenever `tResult/tNetwork/tHttp` translator identities change → full polling reset **Where:** `apps/web/lib/use-inspection-polling.ts:220-231` The effect deps include `tResult`, `tNetwork`, `tHttp`. `useTranslations()` from `next-intl` returns a *new function reference* on every locale change *and sometimes on every parent re-render*, depending on the provider implementation. If those identities are not memoized, the polling effect tears down (`ac.abort()`, `clearTimeout`) and rebuilds on every render of `results/[id]/page.tsx`. That would mean: - `startedAtRef.current = Date.now()` resets, defeating the `maxDurationMs` budget. - A new `AbortController` and a new initial `tick()` fire — back-to-back HTTP calls. - `consecutiveErrors` and `currentInterval` reset to defaults, so exponential backoff never kicks in. I have not confirmed `next-intl`'s exact memo behavior, but the standard pattern is to **not** include translator functions in effect deps for this reason. **Why it matters:** If translators are unstable, this hook will hammer the backend with `1.5 / second` requests indefinitely. **Suggestion:** - Capture `tResult`, `tNetwork`, `tHttp` in refs at the top of the hook and read from refs inside `tick`. Drop them from the dep array. --- ### HIGH-5 — `loadImage` in `uploaded-previews.ts` leaks the image and the object URL on `img.onerror` **Where:** `apps/web/lib/uploaded-previews.ts:52-59` `loadImage` resolves on `onload`, rejects on `onerror`. The `finally` in `fileToResizedDataUrl` *does* revoke the object URL. Good. But the `HTMLImageElement` itself is never cleared (`img.src = ''` or `img.onload = null`). If the image is mid-decode and the user navigates away, the decode continues in the browser. Minor. Bigger issue: `onerror` has no timeout. A corrupt image hangs the upload forever, blocking `stashUploadedPreviews` which blocks the `router.push('/results/...')` (see `inspect/new/page.tsx:104`). The user sits on the upload page with no feedback after the upload itself succeeded server-side. **Suggestion:** - Add a `Promise.race` with a 5-10 s timeout in `loadImage`. - Make `stashUploadedPreviews` non-blocking: kick it off, navigate immediately, let it finish in background and write to sessionStorage when ready. (The results page already tolerates missing previews.) --- ## MEDIUM ### MEDIUM-1 — `getStoredAccessToken()` is read inside `request.use` interceptor for every request → no protection against expired tokens **Where:** `apps/web/lib/api.ts:83-89` The request interceptor unconditionally attaches the access token even if `isJwtExpired(token)` returns true. The 401 response interceptor will catch it and refresh — that's fine — but every expired-but-not-yet-refused request wastes a round trip. **Suggestion:** Check `isJwtExpired(token, 5)` in the request interceptor; if expired, await `runRefresh()` proactively before sending. (Slight risk: deadlock if `runRefresh` itself is intercepted; guard with a "don't intercept /auth/refresh" check.) --- ### MEDIUM-2 — `paused` branch never rendered on results page **Where:** `apps/web/app/results/[id]/page.tsx:27-95` The destructure on line 27 takes `data, loading, error, attempts, timedOut` but **omits `paused` and `retry`**. The hook documents (line 18-20, 51) that `paused` is the user's manual-retry escape hatch. Since the results page never reads it, the contract is silently broken — see CRITICAL-2. --- ### MEDIUM-3 — `data?.error` is used as a translated error message but is the raw backend string **Where:** `apps/web/app/results/[id]/page.tsx:71`, `use-inspection-polling.ts:130-133` When `status === 'failed'`, the hook sets `error: data.error ?? tResult('failed')`. The results page shows `description={data?.error ?? tCommon('errorGeneric')}`. So if the backend returns `"CUDA out of memory: tried to allocate 12.00 GiB..."` that string ends up verbatim in the UI. No i18n, no scrubbing, leaks internal implementation detail. **Suggestion:** Map known backend error codes to translated strings; fall back to a generic translated message when the code is unknown. --- ### MEDIUM-4 — `effectiveMode` recompute can desync `mode` state on `(app)/inspect/new/page.tsx` **Where:** `apps/web/app/(app)/inspect/new/page.tsx:71-74` `effectiveMode` is forced to `'async'` when `files.length > MAX_SYNC_FILES`, but the underlying `mode` state still says `'sync'`. If the user then removes files to get under 5, the UI flips back to `'sync'` *without the user re-confirming*. Probably fine, but worth a UX nit: keep a "user-chosen mode" plus a "current capability" and reset the user choice when files cross the threshold up. --- ### MEDIUM-5 — `getInspectionStatus` returns `data` as-is; no validation against `InspectionStatusResponse` type **Where:** `apps/web/lib/api.ts:355-367` `res.data` is typed as `InspectionStatusResponse` but the type assertion trusts the backend completely. A malformed response (or version skew) crashes downstream (`data.status`, `data.result.parts.flatMap`...). The polling hook would mis-treat a "no status field at all" response as "still polling" forever. **Suggestion:** Validate with a tiny runtime check (or a zod schema in `@arac-hasar/types`) and convert validation failures into a `notFound`/ `server` `ApiErrorInfo`. --- ### MEDIUM-6 — Inspect `/inspect/page.tsx` does not propagate `signal` and has no unmount-abort **Where:** `apps/web/app/inspect/page.tsx:60-83` The duplicate (older?) page at `/inspect/page.tsx` calls `createInspection(files, { mode: effectiveMode })` with **no signal and no unmount cleanup**. If the user submits 20 photos then navigates away mid-upload, the request runs to completion in the background and may call `router.push` after unmount — classic "Can't perform a React state update on an unmounted component" warning. The newer `(app)/inspect/new/page.tsx` does this correctly (lines 39-47, 89-90). Decide which page is canonical and either delete or fix the other. --- ## LOW ### LOW-1 — `clearStoredTokens()` does not clear cookie path/domain rigorously **Where:** `apps/web/lib/api.ts:49-54` Setting `max-age=0` on the same path/samesite as the writer works in practice, but if the cookie was ever issued from a different path (legacy code, A/B test), it'll linger. Low risk in a clean codebase. ### LOW-2 — `tryAcquireRefreshLock` returns `true` on `try { ... } catch { return true }` **Where:** `apps/web/lib/api.ts:141-143` If `localStorage` throws (Safari private mode, quota), the tab declares itself leader. In multi-tab Safari private mode, *all* tabs will declare leader and call `/auth/refresh` in parallel → refresh-token reuse error, session burns. Edge case. ### LOW-3 — `isJwtExpired` parses the JWT on every request **Where:** `apps/web/lib/jwt.ts:48-53` (and the middleware path at `apps/web/middleware.ts:63`) Negligible perf cost, but middleware decodes on every request — including HMR pings during dev. Consider caching by token string. ### LOW-4 — `data.image.url ?? ''` falls back to empty string, then renders `` **Where:** `apps/web/app/results/[id]/page.tsx:136-145` `annotatedUrl` becomes `''`, the falsy check on line 145 *does* catch it (`annotatedUrl ?` ... `: `) — but only because `''` is falsy. Brittle. Prefer explicit `annotatedUrl ? : ` with the value typed as `string | null`. ### LOW-5 — `classifyApiError` returns `{ kind: 'unknown' }` for non-axios errors with no `detail` info **Where:** `apps/web/lib/api.ts:536` A thrown `TypeError` (e.g. `JSON.parse` failure in an interceptor) becomes `{ kind: 'unknown' }`, which the polling hook then shows as `tNetwork('unknown')` — usually misleading ("internet broken" when the real cause is a bug). Adding `error: err instanceof Error ? err.message : undefined` would help debugging. ### LOW-6 — `attempts` counter in `PendingState` shows "1" on the very first render **Where:** `apps/web/app/results/[id]/page.tsx:327` `{attempts > 1 && ...}` guards against the initial "attempt 0/1" flicker, but the first successful tick increments to 1 and the user briefly sees "Attempt 1" before the data resolves. Cosmetic. ### LOW-7 — `useInspectionPolling` mixes UI strings into hook return values **Where:** `apps/web/lib/use-inspection-polling.ts:169-182` The hook resolves translation strings inline. Cleaner separation: return `{ kind: ApiErrorInfo['kind'] | 'failed' | 'timeout' | 'paused' }` and let the consumer translate. Reduces coupling and makes the hook unit-testable without a NextIntlClientProvider wrapper. --- ## Top 5 priority fixes 1. **CRITICAL-1** — Fix the `UploadDropzone`/`FilePreview` blob URL leak. The hook *exists* but its dedup contract is broken; right now every `setFiles` call burns blob memory. High user-visible cost on mobile. 2. **CRITICAL-2 / MEDIUM-2** — Wire the `paused`/`retry` contract through `results/[id]/page.tsx`. Right now an 8-error transient pause is a silent dead-end with the spinner running forever. 3. **HIGH-4** — Stabilize the translator references (or drop them from the effect dep array). If `next-intl` returns new functions per render, polling resets ~every 100 ms and your backend gets DoS'd by your own UI. Verify with a `console.count` in `tick` during a 30 s session. 4. **CRITICAL-3 / HIGH-1** — Tighten the cross-tab refresh path: - Re-read access token after lock acquisition and short-circuit if it already changed. - Bail out of the 401 retry if the original signal is aborted. 5. **HIGH-3** — Add `/results` to `PROTECTED_PREFIXES` in `middleware.ts`. Currently the only thing protecting the results route is a failed API call. --- ## Things done right (call-outs) - The `cancelledRef + AbortController` dual-guard in `use-inspection-polling.ts` is exactly the right pattern for React strict-mode double-mounting. No state update will leak post-unmount. - `runRefresh` in-tab dedup via shared promise (`_refreshPromise`) is correct and avoids the multi-401 stampede. - The `_retry` sentinel on the axios config (line 96-100) correctly prevents an infinite 401→refresh→401 loop. - Sync vs async UX gating (`MAX_SYNC_FILES = 5`) is matched against the backend cap *and* visually disabled on the UI — defense in depth. - `stashUploadedPreviews` resizes to 1024 px JPEG q=0.8 with a 2 MB cap, scoped to `sessionStorage` — solid quota discipline. - `classifyApiError` produces a stable union and parses FastAPI's two `detail` shapes (string vs array-of-errors) without crashing.