Lukas
8904e180d1
When save() encounters a 401 or 403 error, the HTTP client dispatches SESSION_EXPIRED_EVENT which triggers auth handling and navigation to login. However, setSaveError was called first, causing a brief flash of an 'Unauthorized' message before the redirect. Now, isAuthError(err) checks if the error is a 401/403 before setting saveError. Auth errors are rethrown without setting error state, allowing the auth handler to deal with session expiry cleanly without UX confusion. - Import isAuthError from api/client in useConfigItem hook - Check for auth errors in the save() catch block before setSaveError - Add tests for 401 and 403 error handling Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
115 lines
5.6 KiB
Markdown
115 lines
5.6 KiB
Markdown
### TASK-QUALITY-02 — `useConfigItem.save()` Briefly Shows Session-Expiry as Save Error
|
||
|
||
**Where found**
|
||
`frontend/src/hooks/useConfigItem.ts` lines 70–80. The `save()` function's catch block calls `setSaveError(err.message)` for all errors including `ApiError(401)` and `ApiError(403)`. The HTTP client layer dispatches `SESSION_EXPIRED_EVENT` on those status codes, which triggers auth handling, but `setSaveError` still runs first and may briefly display an "Unauthorized" or similar message before the navigation occurs.
|
||
|
||
**Goal**
|
||
Check for auth errors before setting save error state:
|
||
```ts
|
||
} catch (err: unknown) {
|
||
if (isAuthError(err)) throw err; // let auth handler deal with it
|
||
const message = err instanceof Error ? err.message : "Failed to save data";
|
||
setSaveError(message);
|
||
throw err;
|
||
}
|
||
```
|
||
|
||
**Possible traps and issues**
|
||
- Rethrowing auth errors is correct here since the caller might also have error handling. Confirm that all callers of `save()` handle the re-thrown auth error gracefully (typically by not doing anything — the session expiry flow handles navigation).
|
||
- Import `isAuthError` from `../api/client`.
|
||
|
||
**Docs changes needed**
|
||
None required.
|
||
|
||
**Why this is needed**
|
||
Briefly flashing "Unauthorized" in a form's save-error field is confusing UX when the correct outcome is a redirect to the login page.
|
||
|
||
---
|
||
|
||
### TASK-QUALITY-03 — `useHistory` Object Identity Dependency Footgun
|
||
|
||
**Where found**
|
||
`frontend/src/hooks/useHistory.ts`. The hook accepts a `query` object as a parameter and lists it directly in the `useCallback` dependency array for the internal `load` function. If a caller passes an inline object literal on every render (e.g. `useHistory({ page: 1, jail: selectedJail })`), `query` is a new reference every render, causing a new `load` callback, which causes `useEffect([load])` to fire, triggering an infinite re-fetch.
|
||
|
||
**Goal**
|
||
Document this constraint prominently in the hook's JSDoc and in `Docs/Web-Development.md`. Alternatively, change the hook to accept individual primitive parameters instead of an object, eliminating the reference-stability requirement:
|
||
```ts
|
||
export function useHistory(page: number, pageSize: number, jail?: string, ...): UseHistoryResult
|
||
```
|
||
This is the safest fix because it makes incorrect usage a compile-time error.
|
||
|
||
**Possible traps and issues**
|
||
- Changing the signature is a breaking change for all callers. Audit all call sites before changing the signature.
|
||
- The interim documentation fix (a clear JSDoc warning) is a lower-risk option if refactoring callers is out of scope.
|
||
|
||
**Docs changes needed**
|
||
Add a note to `Docs/Web-Development.md`: "Hooks that accept objects as parameters must either destructure to primitives internally or require the caller to provide a stable reference (e.g. via `useMemo`)."
|
||
|
||
**Why this is needed**
|
||
This is a silent footgun. The hook works correctly in all current call sites only because callers happen to use `useMemo` or stable state references. A future caller passing an inline literal will introduce an infinite re-fetch with no obvious diagnostic.
|
||
|
||
---
|
||
|
||
### TASK-QUALITY-04 — `pendingSaveRef as boolean` Redundant Cast in `useAutoSave`
|
||
|
||
**Where found**
|
||
`frontend/src/hooks/useAutoSave.ts`. The code contains `if (pendingSaveRef.current as boolean)` where `pendingSaveRef` is already typed as `React.MutableRefObject<boolean>`. The `as boolean` cast is redundant and suggests the author was uncertain about the type.
|
||
|
||
**Goal**
|
||
Remove the cast: `if (pendingSaveRef.current)`. Run TypeScript type-check to confirm no error is introduced.
|
||
|
||
**Possible traps and issues**
|
||
- None. This is a one-line cleanup.
|
||
|
||
**Docs changes needed**
|
||
None required.
|
||
|
||
**Why this is needed**
|
||
Redundant type assertions are noise that makes reviewers second-guess the type system. They also suppress TypeScript errors in cases where the cast is actually incorrect.
|
||
|
||
---
|
||
|
||
### TASK-QUALITY-05 — `console.warn` in `MapPage` Provides No User Feedback for Threshold Errors
|
||
|
||
**Where found**
|
||
`frontend/src/pages/MapPage.tsx` lines ~148–151:
|
||
```ts
|
||
useEffect(() => {
|
||
if (mapThresholdError) {
|
||
console.warn("Failed to load map color thresholds:", mapThresholdError);
|
||
}
|
||
}, [mapThresholdError]);
|
||
```
|
||
When the threshold fetch fails the map silently falls back to hardcoded defaults. The user has no indication that their custom thresholds are not being applied.
|
||
|
||
**Goal**
|
||
Replace the `console.warn` with a small inline `MessageBar` or tooltip near the map legend that indicates thresholds could not be loaded and defaults are in use. The `console.warn` should be removed from production-facing code.
|
||
|
||
**Possible traps and issues**
|
||
- The fallback behaviour (using hardcoded defaults) is correct and the map should still render. The notification should be non-blocking (not a modal or full-page error).
|
||
- If the threshold fetch failing is expected in certain deployment configurations (e.g. feature not configured), an info-level message rather than a warning may be more appropriate.
|
||
|
||
**Docs changes needed**
|
||
None required.
|
||
|
||
**Why this is needed**
|
||
`console.warn` is invisible to end users. If a custom threshold configuration is silently not applied, the map colour coding may be misleading with no indication of why.
|
||
|
||
---
|
||
|
||
### TASK-QUALITY-06 — `console.log` Leaked in `HistoryPage.test.tsx`
|
||
|
||
**Where found**
|
||
`frontend/src/pages/__tests__/HistoryPage.test.tsx` line 8. A `console.log` statement was left in the test file, likely from a debugging session.
|
||
|
||
**Goal**
|
||
Remove the `console.log` call.
|
||
|
||
**Possible traps and issues**
|
||
- None.
|
||
|
||
**Docs changes needed**
|
||
None required.
|
||
|
||
**Why this is needed**
|
||
Debug logs in test files pollute the test runner output and make it harder to spot real failures or warnings. |