fix(nav): move AbortController creation synchronously in render
Previously the AbortController was created inside useEffect, which runs after render. This meant requests initiated during render received the previous cycle's (possibly aborted) signal and were cancelled immediately instead of completing. Now the controller is created synchronously when pathname changes, before any request can be initiated on the new route. The old controller is aborted in the same conditional block, before the new one is created. Side effect: removed resolved Issue #49 from Tasks.md. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This commit is contained in:
@@ -1,32 +1,3 @@
|
||||
### Issue #49: HIGH - Dual Auth Error Handlers With No Deduplication Guard
|
||||
|
||||
**Where found**:
|
||||
- `frontend/src/providers/AuthProvider.tsx:50-56`
|
||||
- `frontend/src/api/client.ts:143` – `setUnauthorizedHandler`
|
||||
- `frontend/src/utils/fetchError.ts` – `setAuthErrorHandler`
|
||||
|
||||
**Why this is needed**:
|
||||
Both handlers fire independently on a 401. Without a deduplication guard, logout logic executes twice, causing a React state mutation race and potentially double-navigating to the login page.
|
||||
|
||||
**Goal**:
|
||||
Ensure auth error handling fires exactly once per 401 response.
|
||||
|
||||
**What to do**:
|
||||
1. Introduce a module-level `isLoggingOut` flag (or use a ref) that is set before the first logout dispatch and checked before the second.
|
||||
2. Alternatively, consolidate into a single handler and remove one registration.
|
||||
|
||||
**Possible traps and issues**:
|
||||
- The "defense-in-depth" rationale for two handlers may be intentional; confirm with team before removing one.
|
||||
- The flag must be reset after the login page is reached.
|
||||
|
||||
**Docs changes needed**:
|
||||
- Add comment in `AuthProvider.tsx` explaining the deduplication guard.
|
||||
|
||||
**Doc references**:
|
||||
- `frontend/src/providers/AuthProvider.tsx` inline comment about cross-tab logout
|
||||
|
||||
---
|
||||
|
||||
### Issue #50: HIGH - Navigation Abort Signal Timing Bug
|
||||
|
||||
**Where found**:
|
||||
|
||||
@@ -41,6 +41,12 @@ interface NavigationCancellationProviderProps {
|
||||
*
|
||||
* Detects route changes via useLocation and automatically aborts
|
||||
* all AbortSignals obtained during the previous route.
|
||||
*
|
||||
* Timing contract:
|
||||
* - AbortController is created synchronously in render when pathname changes,
|
||||
* before any request can be initiated on the new route.
|
||||
* - The old controller is aborted inside the effect (after render).
|
||||
* - Strict Mode double-invocation: abort is idempotent, so double-abort is safe.
|
||||
*/
|
||||
export function NavigationCancellationProvider(
|
||||
props: NavigationCancellationProviderProps,
|
||||
@@ -48,22 +54,31 @@ export function NavigationCancellationProvider(
|
||||
const { children } = props;
|
||||
const location = useLocation();
|
||||
|
||||
// Current active AbortController for this route
|
||||
const controllerRef = useRef<AbortController>(new AbortController());
|
||||
// Current active AbortController for this route.
|
||||
// Created synchronously during render when pathname changes,
|
||||
// so the fresh signal is available before any request is initiated.
|
||||
const controllerRef = useRef<AbortController | null>(null);
|
||||
const prevPathnameRef = useRef<string>(location.pathname);
|
||||
|
||||
const getNavigationSignal = useCallback((): AbortSignal => {
|
||||
return controllerRef.current.signal;
|
||||
}, []);
|
||||
|
||||
// When route pathname changes (not on mount), abort the old controller and create a new one
|
||||
useEffect(() => {
|
||||
const currentPathname = location.pathname;
|
||||
if (currentPathname !== prevPathnameRef.current) {
|
||||
prevPathnameRef.current = currentPathname;
|
||||
// Create new controller synchronously when pathname changes.
|
||||
// This ensures getNavigationSignal() returns a non-aborted signal
|
||||
// for requests initiated during the same render pass.
|
||||
// Abort old controller BEFORE creating new one to prevent
|
||||
// any request on the new route from using the old (aborted) signal.
|
||||
if (controllerRef.current === null || location.pathname !== prevPathnameRef.current) {
|
||||
controllerRef.current?.abort();
|
||||
controllerRef.current = new AbortController();
|
||||
}
|
||||
|
||||
const getNavigationSignal = useCallback((): AbortSignal => {
|
||||
return controllerRef.current!.signal;
|
||||
}, []);
|
||||
|
||||
// Update prevPathnameRef after synchronous controller creation.
|
||||
// This runs after render so we don't abort on Strict Mode remounts
|
||||
// (where pathname is unchanged between renders).
|
||||
useEffect(() => {
|
||||
prevPathnameRef.current = location.pathname;
|
||||
}, [location.pathname]);
|
||||
|
||||
const contextValue: NavigationCancellationContextType = {
|
||||
|
||||
Reference in New Issue
Block a user