Refactor useHistory hook: replace HistoryQuery with explicit parameters and add documentation
- Split useHistory interface to accept explicit parameters (page, pageSize, range, origin, jail, ip, source) instead of HistoryQuery object - Add comprehensive JSDoc for useHistory function - Update HistoryPage and tests to use new parameter structure - Move TaskList documentation from Tasks.md to Web-Development.md - Improve type safety with explicit TimeRange and BanOriginFilter types Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This commit is contained in:
@@ -1,31 +1,3 @@
|
||||
### 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**
|
||||
|
||||
@@ -328,6 +328,37 @@ export default BanTable;
|
||||
- Always include the correct dependency arrays in `useEffect`, `useMemo`, and `useCallback`. Disable the ESLint exhaustive-deps rule **only** with a comment explaining why.
|
||||
- Clean up side effects (subscriptions, timers, abort controllers) in the `useEffect` cleanup function.
|
||||
|
||||
### Object Parameters in Hooks (Reference Stability)
|
||||
|
||||
**When a hook accepts an object parameter, include it in dependency arrays only if it is guaranteed to be a stable reference.** If callers pass inline object literals, the object reference changes on every render, causing unnecessary re-fetches and potential infinite loops.
|
||||
|
||||
**Preferred solution:** Design hook signatures to accept individual **primitive parameters** instead of objects. This makes incorrect usage a compile-time error:
|
||||
|
||||
```ts
|
||||
// ❌ Footgun: query object causes infinite fetches if caller uses inline literals
|
||||
export function useHistory(query: HistoryQuery = {}): UseHistoryResult {
|
||||
const load = useCallback(() => { /* ... */ }, [query]);
|
||||
}
|
||||
|
||||
// Called like this (creates new object every render):
|
||||
const result = useHistory({ page: 1, jail: selectedJail });
|
||||
|
||||
// ✅ Safe: individual primitives can't be accidentally unstable
|
||||
export function useHistory(
|
||||
page: number = 1,
|
||||
pageSize: number = 50,
|
||||
jail?: string,
|
||||
): UseHistoryResult {
|
||||
const load = useCallback(() => { /* ... */ }, [page, pageSize, jail]);
|
||||
}
|
||||
|
||||
// Called like this (all primitives are stable):
|
||||
const result = useHistory(page, PAGE_SIZE, jailFilter);
|
||||
```
|
||||
|
||||
If refactoring to individual parameters is not feasible, document the constraint clearly in JSDoc and require callers to stabilize the reference using `useMemo`.
|
||||
|
||||
|
||||
```tsx
|
||||
// hooks/useBans.ts
|
||||
import { useState, useEffect } from "react";
|
||||
|
||||
Reference in New Issue
Block a user