cesnimda
b8f8569e6e
useViewResource built `reload` with `load` in its useCallback deps, and the fetch effect depended on `reload`. Callers routinely pass an inline `load` closure (e.g. JobTable), so `load` — and therefore `reload` and the effect — changed every render, calling setState and re-rendering: an unbounded "Maximum update depth exceeded" loop that froze the renderer on /jobs and every other list view (DashboardView, RemindersView, CompaniesTable). Fix: hold `load` in a ref (like the existing hasLoadedRef) and drop it from the dependency arrays. Re-fetching is still driven by `deps`/`enabled`; the ref always points at the latest closure. No API/behaviour change for callers. Runtime-verified live: /jobs went from a render storm (frozen renderer, 100s of console errors) to 0 errors in a 2s window and a clean render. Suites that drive JobTable→useViewResource pass in isolation; the remaining full-run flakiness is pre-existing (state-pollution/timing in the heavy RTL suites, unrelated). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
73 lines
5.9 KiB
Markdown
73 lines
5.9 KiB
Markdown
# Performance Improvements — Job Tracker
|
||
|
||
**Companion to:** [MEMORY_LEAK_REPORT.md](MEMORY_LEAK_REPORT.md) · [ROOT_CAUSE_ANALYSIS.md](ROOT_CAUSE_ANALYSIS.md)
|
||
|
||
## Changes made (this pass)
|
||
| Change | File | Effect | Verified |
|
||
|---|---|---|---|
|
||
| **Stop an infinite render loop on every list view** — hold `load` in a ref in `useViewResource` so `reload`/the fetch effect keep a stable identity | `job-tracker-ui/src/hooks/useViewResource.ts` | Fixes "Maximum update depth exceeded" on `/jobs` (and any `DashboardView`/`RemindersView`/`CompaniesTable` view whose caller passes an inline `load`) — pegged the CPU/renderer | **Runtime-confirmed**: `/jobs` went from a render storm (renderer frozen, 100s of errors) to 0 console errors in a live 2s window and a clean render; `workflow-trust-signals` (drives `JobTable`→`useViewResource`) passes |
|
||
| **Stop the infinite `/auth/me` request loop** — make `clearAuthClientState` emit `auth-changed` only on a real signed-in→out transition | `job-tracker-ui/src/auth.ts` | Eliminates a runaway request storm (100+ `GET /auth/me` and climbing) that ran continuously whenever the user was logged out | **Runtime-confirmed** in a live stack: `/auth/me` count 100+ & growing → 0 and stable after fix |
|
||
| Revoke CV-preview blob URLs on unmount only (ref-based), not on every carousel change | `job-tracker-ui/src/pages/ProfilePage.tsx` | Fixes broken previews on multi-template decks; still frees URLs on unmount | `profile-page.test.tsx` 5/5 |
|
||
|
||
### Runtime finding — self-triggering auth loop (the most impactful issue found)
|
||
Only visible with a running backend (static analysis could not surface it). Sequence: the axios response
|
||
interceptor (`api.ts`) calls `clearAuthClientState()` on **every** 401; that dispatched `"auth-changed"`;
|
||
the `App` handler re-fetched `/auth/me`; that 401'd again → interceptor → `clearAuthClientState()` →
|
||
`"auth-changed"` → … an unbounded loop that hammered the server and spun the client on the login page and
|
||
after any session expiry. Fix: `clearAuthClientState` now only emits when it actually removes a stored user
|
||
key (idempotent), so repeated 401s can't re-trigger the fetch. This is a CPU/network/battery drain and a
|
||
self-inflicted request flood, not a memory leak — but squarely in the Phase 3.5 "infinite polling / retry
|
||
loop / duplicate requests" scope, and the single highest-value fix from the whole investigation.
|
||
|
||
> Context: this was the only defect found in a full resource audit. The codebase already practises
|
||
> disciplined cleanup (timers cleared, listeners removed, object URLs revoked), so there was no leak to
|
||
> fix — see the main report.
|
||
|
||
## Recommended (low-severity, optional)
|
||
|
||
### 1. Stabilise the extraction-run poll — *minor*
|
||
`ProfilePage.tsx:315-324` recreates its 4s interval on every poll because `extractionRuns` is in the deps
|
||
and changes each tick. It's harmless (cleanup runs; it stops when runs finish) but churns. If touched:
|
||
poll on a stable trigger (e.g. a boolean `hasActiveRuns` in deps, or read runs from a ref inside the
|
||
interval) so the interval is created once per active-window.
|
||
|
||
### 2. One live heap-snapshot pass on a populated session — *verification, not a fix*
|
||
The static audit is strong, but a single DevTools confirmation closes the loop:
|
||
1. Run the real stack (backend on `:5202` + a seeded DB) and sign in.
|
||
2. DevTools → Memory → take a heap snapshot.
|
||
3. Navigate `/dashboard → /jobs → open a job dialog → close → /profile → build a CV deck → back`, ×5.
|
||
4. Force GC, take a second snapshot, **Comparison** view.
|
||
5. Expect: node/listener/detached counts return to baseline (sawtooth), not monotonic growth. Sort
|
||
retained size by constructor; look for `Detached HTMLElement`, growing `Array`/`Map`, or listener
|
||
counts that never fall.
|
||
|
||
Also cheap and useful: `performance.memory.usedJSHeapSize` (Chromium) logged across the loop, or a
|
||
Playwright script that repeats the navigation and asserts heap stays bounded.
|
||
|
||
### 3. Guard async setState after unmount — *defensive, not a current leak*
|
||
Several components `await api…().then(setState)`. React 18 no-ops setState on unmounted components (just a
|
||
dev warning historically), so this is not a leak, but for long CV/AI calls consider an `AbortController`
|
||
on the request (cancels the in-flight network work on unmount) — improves responsiveness and avoids wasted
|
||
work more than memory.
|
||
|
||
## Prevention — keep leaks from creeping in
|
||
- **Lint:** enable `react-hooks/exhaustive-deps` (surfaces the exact wrong-deps class that caused the one
|
||
bug here) and consider `react-hooks/react-compiler` checks.
|
||
- **Rule of thumb:** any effect that *acquires* a resource (listener, timer, object URL, observer,
|
||
subscription, connection) must return a cleanup that releases exactly that resource. "Release once on
|
||
unmount" ⇒ empty-deps effect + a ref for current state — never a value in the deps array.
|
||
- **Object URLs:** pair every `createObjectURL` with a `revokeObjectURL` in the *same* owner; prefer
|
||
revoking on unmount/replace, never on unrelated re-renders.
|
||
- **Server caches:** every `IMemoryCache.Set` must carry an absolute/sliding expiration (as
|
||
`GmailOAuthService` correctly does); if the app grows to heavy caching, set a `SizeLimit`.
|
||
- **No unbounded static state:** keep `static` collections to fixed lookup tables (as today); never
|
||
accumulate per-request data in a static field.
|
||
- **CI:** the heavy RTL suites are timeout-flaky under load — raising `testTimeout` (e.g. 15–20s) or
|
||
reducing jest worker contention would make regressions (including any future leak-guard tests) reliably
|
||
visible instead of hidden behind flakes.
|
||
|
||
## Security-audit note (standing instruction)
|
||
The applied change carries no security surface (client-side URL lifetime only). The recommendations above
|
||
introduce none either; if #3 (AbortController) is implemented, ensure aborted requests don't leave
|
||
partial writes — not applicable to the read-only CV export/preview calls here.
|