docs/CODE_REVIEW_POLLING.md

# 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<File, string>` 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 `<ErrorBanner>` *plus* the `<PendingState>`
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/<some-id>`
bypasses middleware, then `useInspectionPolling` fires
`GET /api/v1/inspect/<id>`, 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/<id>` 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 `<img src="">`

**Where:** `apps/web/app/results/[id]/page.tsx:136-145`

`annotatedUrl` becomes `''`, the falsy check on line 145 *does* catch it
(`annotatedUrl ?` ... `: <fallback>`) — but only because `''` is falsy.
Brittle. Prefer explicit `annotatedUrl ? <Tabs/> : <Placeholder/>` 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.