diff --git a/Docs/Tasks.md b/Docs/Tasks.md index a65addc..cc5bf3b 100644 --- a/Docs/Tasks.md +++ b/Docs/Tasks.md @@ -1,23 +1,3 @@ -## 22) Magic strings are scattered in frontend storage keys -- Where found: - - [frontend/src/providers/AuthProvider.tsx](frontend/src/providers/AuthProvider.tsx) - - [frontend/src/layouts/MainLayout.tsx](frontend/src/layouts/MainLayout.tsx) - - [frontend/src/providers/ThemeProvider.tsx](frontend/src/providers/ThemeProvider.tsx) -- Why this is needed: - - Repeated literals invite drift and typo regressions. -- Goal: - - Centralize user/session/local storage keys. -- What to do: - - Consolidate into a single constants module. -- Possible traps and issues: - - Existing tests may assume current literal values. -- Docs changes needed: - - Add storage key registry note. -- Doc references: - - [frontend/src/utils/constants.ts](frontend/src/utils/constants.ts) - ---- - ## 23) No global cancellation policy on route transitions - Where found: - [frontend/src/hooks](frontend/src/hooks) diff --git a/Docs/Web-Development.md b/Docs/Web-Development.md index 77c315f..1059198 100644 --- a/Docs/Web-Development.md +++ b/Docs/Web-Development.md @@ -291,6 +291,92 @@ export function useMyCustomData(options: MyOptions): MyResult - **Testability**: Base hook can be tested in isolation; custom effects are minimal and easy to test - **Maintainability**: Bug fixes to abort or error handling only need to happen once +### Request Lifecycle & Navigation-Aware Cancellation + +BanGUI provides a global cancellation mechanism that automatically aborts route-specific requests when the user navigates away. This prevents: +- Silent errors from responses arriving after component unmount +- Wasted bandwidth from now-irrelevant requests +- State inconsistencies between pages + +**How It Works:** + +The `NavigationCancellationProvider` (wraps the entire router) detects route changes and aborts all `AbortSignal`s obtained from `useNavigationAbortSignal()`. Each route gets its own set of signals that live for the duration of that route. + +**When to Use Navigation Signals:** + +Use `useNavigationAbortSignal()` for data fetches that are **specific to the current route**: +- Page-level data fetches (e.g., dashboard stats on the home page) +- User-initiated refetches on the current page +- Paginated lists, search results, filtered data + +**When NOT to Use:** + +Long-lived background tasks should manage their own lifecycle and NOT use navigation signals: +- Session validation (survives route changes) +- Service-level polling (e.g., server health checks) +- Background syncs that may take longer than a user's stay on a page +- Cross-route background operations + +**Usage Pattern:** + +```ts +// hooks/useDashboardData.ts +export function useDashboardData(): DashboardResult { + const signal = useNavigationAbortSignal(); // Gets signal for current route + + const { items: dashStats } = useListData({ + fetcher: (sig) => fetchDashboardStats(sig || signal), // Use both signals + selector: (res) => res.stats, + errorMessage: "Failed to load dashboard stats", + }); + + return { dashStats }; +} +``` + +When the user navigates away, the signal is automatically aborted, cancelling any in-flight dashboard stats requests. + +**Opt-Out for Long-Lived Tasks:** + +If a fetch should persist across route changes, do not use `useNavigationAbortSignal()`. Instead, manage your own `AbortController`: + +```ts +// Service-level polling — persists across route changes +export function useServerHealth(): HealthResult { + const [health, setHealth] = useState(null); + const controllerRef = useRef(new AbortController()); + + useEffect(() => { + const poll = async () => { + try { + const data = await fetchHealth(controllerRef.current.signal); + setHealth(data); + } catch (err) { + if (!(err instanceof DOMException && err.name === "AbortError")) { + console.error(err); + } + } + }; + + const interval = setInterval(poll, 5000); + void poll(); // First fetch immediately + + return () => { + clearInterval(interval); + controllerRef.current?.abort(); + }; + }, []); + + return { health }; +} +``` + +**Provider Configuration:** + +The `NavigationCancellationProvider` is automatically placed in `App.tsx` inside `BrowserRouter` but before `AuthProvider`. It wraps all routes (including setup and login) to ensure consistent cancellation behavior across the entire app. + +See `src/providers/PROVIDER_ORDER.md` for the full provider hierarchy and dependencies. + --- ## 4. Code Organization diff --git a/frontend/src/App.tsx b/frontend/src/App.tsx index df52d3f..16ee184 100644 --- a/frontend/src/App.tsx +++ b/frontend/src/App.tsx @@ -2,13 +2,14 @@ * Application root component. * * Provider order (see `src/providers/PROVIDER_ORDER.md` for detailed contract): - * 1. `ThemeProvider` — OUTERMOST; provides theme context to AppContents - * 2. `FluentProvider` — supplies Fluent UI theme and design tokens - * 3. `NotificationProvider` — provides notification service to all descendants - * 4. `ErrorBoundary` — catches catastrophic errors - * 5. `BrowserRouter` — enables client-side routing via React Router - * 6. `AuthProvider` — manages session state; validates on mount; uses useNavigate() - * 7. `TimezoneProvider` — INNERMOST (inside protected routes); fetches timezone after auth + * 1. `ThemeProvider` — OUTERMOST; provides theme context to AppContents + * 2. `FluentProvider` — supplies Fluent UI theme and design tokens + * 3. `NotificationProvider` — provides notification service to all descendants + * 4. `ErrorBoundary` — catches catastrophic errors + * 5. `BrowserRouter` — enables client-side routing via React Router + * 6. `NavigationCancellationProvider` — manages route-aware request cancellation + * 7. `AuthProvider` — manages session state; validates on mount; uses useNavigate() + * 8. `TimezoneProvider` — INNERMOST (inside protected routes); fetches timezone after auth * * CRITICAL: Provider order is order-sensitive. See PROVIDER_ORDER.md before refactoring. * @@ -37,6 +38,7 @@ import { darkTheme, lightTheme } from "./theme/customTheme"; import { AuthProvider } from "./providers/AuthProvider"; import { ThemeProvider, useThemeMode } from "./providers/ThemeProvider"; import { TimezoneProvider } from "./providers/TimezoneProvider"; +import { NavigationCancellationProvider } from "./providers/NavigationCancellationProvider"; import { NotificationProvider } from "./services/notificationService"; import { RequireAuth } from "./components/RequireAuth"; import { SetupGuard } from "./components/SetupGuard"; @@ -60,12 +62,13 @@ const BlocklistsPage = lazy(() => import("./pages/BlocklistsPage").then((m) => ( * Root application component — mounts providers and top-level routes. * * Provider stack (see PROVIDER_ORDER.md for detailed contract): - * - FluentProvider (2) — receives theme from useThemeMode() - * - NotificationProvider (3) — provides notification service - * - ErrorBoundary (4) — catches catastrophic errors - * - BrowserRouter (5) — enables routing - * - AuthProvider (6) — session validation; uses useNavigate() - * - TimezoneProvider (7) — inside protected routes only + * - FluentProvider (2) — receives theme from useThemeMode() + * - NotificationProvider (3) — provides notification service + * - ErrorBoundary (4) — catches catastrophic errors + * - BrowserRouter (5) — enables routing + * - NavigationCancellationProvider (6) — manages route-aware request cancellation + * - AuthProvider (7) — session validation; uses useNavigate() + * - TimezoneProvider (8) — inside protected routes only */ function AppContents(): React.JSX.Element { const { colorMode } = useThemeMode(); @@ -89,9 +92,11 @@ function AppContents(): React.JSX.Element { {/* 5. BrowserRouter — enables routing; required by AuthProvider's useNavigate() */} - }> - {/* 6. AuthProvider — validates session on mount; must be inside BrowserRouter */} - + {/* 6. NavigationCancellationProvider — manages route-aware request cancellation */} + + }> + {/* 7. AuthProvider — validates session on mount; must be inside BrowserRouter */} + {/* Setup wizard — always accessible; redirects to /login if already done */} + diff --git a/frontend/src/hooks/useNavigationAbortSignal.ts b/frontend/src/hooks/useNavigationAbortSignal.ts new file mode 100644 index 0000000..f6fb1c5 --- /dev/null +++ b/frontend/src/hooks/useNavigationAbortSignal.ts @@ -0,0 +1,53 @@ +/** + * Hook to subscribe to navigation-aware request cancellation. + * + * Returns an AbortSignal that is automatically aborted when the user + * navigates to a different route. Use this signal to cancel API requests + * that are specific to the current route and should not survive a navigation. + * + * Usage: + * const signal = useNavigationAbortSignal(); + * const { items } = useListData({ + * fetcher: (sig) => fetchBans(sig || signal), + * // ... + * }); + * + * When to use: + * - For page-level data fetches that should not persist across navigation + * - For user-initiated refetches on the current page + * - For paginated lists, search results, filters + * + * When NOT to use: + * - For long-lived background polls (use your own AbortController instead) + * - For service-level state syncs (e.g., session validation) + * - For actions that may take longer than a user interaction timeout + * + * Note: The signal may already be aborted at the time you check it, + * depending on timing. This is safe — fetchers should handle aborted + * signals gracefully by throwing/catching AbortError. + */ + +import { useContext } from "react"; +import { NavigationCancellationContext } from "../providers/NavigationCancellationContext"; + +/** + * Get an AbortSignal for the current route's request lifecycle. + * + * The returned signal will be aborted when the user navigates away. + * All requests using this signal will be automatically cancelled. + * + * @returns AbortSignal tied to the current route + * @throws Error if called outside NavigationCancellationProvider + */ +export function useNavigationAbortSignal(): AbortSignal { + const context = useContext(NavigationCancellationContext); + + if (!context) { + throw new Error( + "useNavigationAbortSignal must be used within NavigationCancellationProvider. " + + "Wrap your router with in App.tsx.", + ); + } + + return context.getNavigationSignal(); +} diff --git a/frontend/src/providers/NavigationCancellationContext.ts b/frontend/src/providers/NavigationCancellationContext.ts new file mode 100644 index 0000000..08433fe --- /dev/null +++ b/frontend/src/providers/NavigationCancellationContext.ts @@ -0,0 +1,56 @@ +/** + * Navigation-aware request cancellation context. + * + * Provides a global cancellation mechanism tied to route transitions. + * When the user navigates to a new route, all AbortSignals obtained from + * this context are automatically aborted, cancelling in-flight requests + * associated with the previous route. + * + * Long-lived background fetches (e.g., polling with long TTL) can opt-out + * by not using this context and instead managing their own lifecycle, + * or by checking the signal early in their lifecycle. + * + * Design notes: + * - Subscribers are notified immediately when navigation occurs + * - Multiple consumers can safely subscribe and get independent signals + * - Signals are generator functions to allow late binding + */ + +import { createContext } from "react"; + +/** + * Provides a fresh AbortSignal tied to the current route lifecycle. + * + * Each call returns a new AbortSignal. When the user navigates, + * all previously-returned signals are aborted. + */ +export interface NavigationCancellationContextType { + /** + * Get an AbortSignal for the current route's request lifecycle. + * + * The signal will be aborted automatically when the user navigates + * to a different route. This is ideal for route-specific data fetches + * that should not persist across page transitions. + * + * Example: + * const signal = useNavigationAbortSignal(); + * const { items } = useListData({ + * fetcher: (sig) => fetchBans(sig || signal), + * // ... + * }); + * + * @returns An AbortSignal that lives for the duration of the current route + */ + getNavigationSignal(): AbortSignal; +} + +/** + * React context for navigation-aware cancellation. + * + * Wrap the application with `NavigationCancellationProvider` to enable + * automatic request cancellation on route transitions. + */ +export const NavigationCancellationContext = + createContext(null); + +NavigationCancellationContext.displayName = "NavigationCancellation"; diff --git a/frontend/src/providers/NavigationCancellationProvider.tsx b/frontend/src/providers/NavigationCancellationProvider.tsx new file mode 100644 index 0000000..2ea3053 --- /dev/null +++ b/frontend/src/providers/NavigationCancellationProvider.tsx @@ -0,0 +1,79 @@ +/** + * Provider for navigation-aware request cancellation. + * + * Wraps route transitions and automatically aborts all active requests + * when the user navigates to a new route. This prevents state-update + * errors from responses arriving after the component unmounts and helps + * conserve bandwidth by cancelling now-irrelevant requests. + * + * Installation: + * Wrap the `` and route definitions with this provider. + * It must be inside BrowserRouter (to access useLocation) but can wrap + * the entire router or just the protected routes. + * + * Example: + * + * + * ... + * + * + * + * Behavior: + * - On mount: creates initial AbortController for route 1 + * - On route change: aborts previous controller, creates new one + * - Subscribers receive fresh signals for each route + * - Long-lived background tasks can opt-out by not using this context + */ + +import { useEffect, useRef, useCallback, ReactNode } from "react"; +import { useLocation } from "react-router-dom"; +import { + NavigationCancellationContext, + type NavigationCancellationContextType, +} from "./NavigationCancellationContext"; + +interface NavigationCancellationProviderProps { + children: ReactNode; +} + +/** + * Provider component for navigation-aware cancellation. + * + * Detects route changes via useLocation and automatically aborts + * all AbortSignals obtained during the previous route. + */ +export function NavigationCancellationProvider( + props: NavigationCancellationProviderProps, +): React.JSX.Element { + const { children } = props; + const location = useLocation(); + + // Current active AbortController for this route + const controllerRef = useRef(new AbortController()); + const prevPathnameRef = useRef(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; + controllerRef.current?.abort(); + controllerRef.current = new AbortController(); + } + }, [location.pathname]); + + const contextValue: NavigationCancellationContextType = { + getNavigationSignal, + }; + + return ( + + {children} + + ); +} + diff --git a/frontend/src/providers/PROVIDER_ORDER.md b/frontend/src/providers/PROVIDER_ORDER.md index 109e4f3..3e7eeb2 100644 --- a/frontend/src/providers/PROVIDER_ORDER.md +++ b/frontend/src/providers/PROVIDER_ORDER.md @@ -11,16 +11,17 @@ This document makes that order explicit, documents the rationale for each provid ## Provider Hierarchy (Outermost to Innermost) ``` -1. ThemeProvider (must be outermost — provides theme to AppContents) +1. ThemeProvider (must be outermost — provides theme to AppContents) └─ AppContents - 2. FluentProvider (must wrap all Fluent UI consumers) - 3. NotificationProvider (must wrap error boundaries) - 4. ErrorBoundary (top-level — catches catastrophic errors) - 5. NotificationContainer (renders notifications) - 6. BrowserRouter (enables routing) - 7. AuthProvider (provides auth context) - 8. Routes with SetupGuard & RequireAuth - 9. TimezoneProvider (wraps protected routes only) + 2. FluentProvider (must wrap all Fluent UI consumers) + 3. NotificationProvider (must wrap error boundaries) + 4. ErrorBoundary (top-level — catches catastrophic errors) + 5. NotificationContainer (renders notifications) + 6. BrowserRouter (enables routing) + 7. NavigationCancellationProvider (manages route-aware request cancellation) + 8. AuthProvider (provides auth context) + 9. Routes with SetupGuard & RequireAuth + 10. TimezoneProvider (wraps protected routes only) ``` --- @@ -109,7 +110,7 @@ This document makes that order explicit, documents the rationale for each provid **Why it must be here:** - Enables routing for the entire app -- Must wrap `AuthProvider` and route definitions +- Must wrap `NavigationCancellationProvider` and route definitions - Allows `useNavigate()` to work in `AuthProvider` (for logout redirects) **Dependencies:** ErrorBoundary (error recovery), FluentProvider (potential Fluent components in routes) @@ -118,7 +119,33 @@ This document makes that order explicit, documents the rationale for each provid --- -### 7. **AuthProvider** (Inside BrowserRouter) +### 7. **NavigationCancellationProvider** (Inside BrowserRouter, before AuthProvider) + +**Location in code:** Inside BrowserRouter but wrapping AuthProvider + +**Why it must be here:** +- Uses `useLocation()` from React Router to detect route changes +- Must be inside `BrowserRouter` to access routing context +- Must be outside `AuthProvider` so it can wrap all routes including setup and login +- Must be before `AuthProvider` so authentication requests can use the cancellation signals if needed + +**Dependencies:** BrowserRouter (uses `useLocation()` hook) + +**Initialization:** Synchronous (creates initial AbortController on mount) + +**Critical Contract:** +- When the user navigates to a different route (detected via `useLocation().pathname`), automatically aborts all AbortSignals obtained from the context +- Ensures page-level data fetches don't continue after navigation +- Long-lived background tasks (e.g., polling services) can opt-out by not using this context + +**Usage by Consumers:** +- Call `useNavigationAbortSignal()` to get a signal that lives for the duration of the current route +- Pass this signal to API functions that accept `signal?: AbortSignal` +- The signal will be automatically aborted on route change + +--- + +### 8. **AuthProvider** (Inside NavigationCancellationProvider) **Location in code:** Inside BrowserRouter routes @@ -142,7 +169,7 @@ This document makes that order explicit, documents the rationale for each provid --- -### 8. **TimezoneProvider** (Inside RequireAuth) +### 9. **TimezoneProvider** (Inside RequireAuth) **Location in code:** Wrapped around `MainLayout` within protected routes @@ -170,17 +197,21 @@ This document makes that order explicit, documents the rationale for each provid 2. **Moving FluentProvider outside AppContents:** - Cannot access `useThemeMode()` to determine theme -3. **Moving AuthProvider before BrowserRouter:** +3. **Moving NavigationCancellationProvider before BrowserRouter:** + - `useLocation()` would fail (hook outside routing context) + - Route changes wouldn't be detected + +4. **Moving AuthProvider before BrowserRouter or NavigationCancellationProvider:** - `useNavigate()` would fail (hook outside routing context) -4. **Moving TimezoneProvider before AuthProvider:** +5. **Moving TimezoneProvider before AuthProvider:** - Unauthenticated users could see timezone fetch attempts - API calls would fail without a valid session -5. **Moving ErrorBoundary after AuthProvider:** +6. **Moving ErrorBoundary after AuthProvider:** - Auth errors wouldn't be caught by the top-level boundary -6. **Moving NotificationProvider after ErrorBoundary:** +7. **Moving NotificationProvider after ErrorBoundary:** - Error boundary couldn't display error notifications --- @@ -227,5 +258,6 @@ These tests act as a regression suite: any refactor that violates the provider c | NotificationProvider | FluentProvider | No | none | | ErrorBoundary | NotificationProvider | No | N/A | | BrowserRouter | ErrorBoundary | No | none | -| AuthProvider | BrowserRouter | **Yes** | backend session validation | +| NavigationCancellationProvider | BrowserRouter | No | `useLocation()` for route detection | +| AuthProvider | NavigationCancellationProvider | **Yes** | backend session validation | | TimezoneProvider | RequireAuth | **Yes** | backend API (requires auth) | diff --git a/frontend/src/providers/__tests__/NavigationCancellationProvider.test.tsx b/frontend/src/providers/__tests__/NavigationCancellationProvider.test.tsx new file mode 100644 index 0000000..eda0350 --- /dev/null +++ b/frontend/src/providers/__tests__/NavigationCancellationProvider.test.tsx @@ -0,0 +1,131 @@ +/** + * Tests for NavigationCancellationProvider and useNavigationAbortSignal hook. + * + * Verifies that: + * - Signals are properly created and returned + * - The provider correctly tracks route changes + * - The hook throws when used outside the provider + */ + +import { describe, it, expect } from "vitest"; +import { renderHook, act } from "@testing-library/react"; +import { MemoryRouter, useNavigate, Routes, Route } from "react-router-dom"; +import { ReactNode } from "react"; +import { NavigationCancellationProvider } from "../NavigationCancellationProvider"; +import { useNavigationAbortSignal } from "../../hooks/useNavigationAbortSignal"; + +describe("NavigationCancellationProvider", () => { + function createWrapper(initialRoute = "/"): React.FC<{ children: ReactNode }> { + return function Wrapper({ children }: { children: ReactNode }) { + return ( + + {children} + + ); + }; + } + + it("should provide a context that returns AbortSignal instances", () => { + const wrapper = createWrapper(); + const { result } = renderHook(() => useNavigationAbortSignal(), { wrapper }); + + expect(result.current).toBeInstanceOf(AbortSignal); + }); + + it("should not abort signal on initial mount", () => { + const wrapper = createWrapper(); + const { result } = renderHook(() => useNavigationAbortSignal(), { wrapper }); + + expect(result.current.aborted).toBe(false); + }); + + it("should throw when used outside the provider", () => { + const wrapper = ({ children }: { children: ReactNode }) => ( + {children} + ); + + expect(() => { + renderHook(() => useNavigationAbortSignal(), { wrapper }); + }).toThrow("useNavigationAbortSignal must be used within NavigationCancellationProvider"); + }); + + it("should abort signal when route changes", () => { + const wrapper = ({ + children, + initialRoute = "/page1", + }: { + children: ReactNode; + initialRoute?: string; + }) => ( + + + + + + + + + ); + + const { result, rerender } = renderHook( + () => ({ + signal: useNavigationAbortSignal(), + navigate: useNavigate(), + }), + { + wrapper, + initialProps: { children:
, initialRoute: "/page1" }, + }, + ); + + const signal1 = result.current.signal; + expect(signal1.aborted).toBe(false); + + // Navigate to different route + act(() => { + result.current.navigate("/page2"); + }); + + rerender({ children:
, initialRoute: "/page2" }); + + const signal2 = result.current.signal; + + // Previous signal should be aborted + expect(signal1.aborted).toBe(true); + // New signal should not be aborted + expect(signal2.aborted).toBe(false); + // Signals should be different + expect(signal1).not.toBe(signal2); + }); + + it("should provide AbortSignal with proper properties", () => { + const wrapper = createWrapper(); + const { result } = renderHook(() => useNavigationAbortSignal(), { wrapper }); + + const signal = result.current; + + // Verify standard AbortSignal properties exist + expect(typeof signal.aborted).toBe("boolean"); + expect(typeof signal.addEventListener).toBe("function"); + expect(typeof signal.removeEventListener).toBe("function"); + }); + + it("should work with fetch API abort pattern", async () => { + const wrapper = createWrapper(); + renderHook(() => useNavigationAbortSignal(), { wrapper }); + + // Create a fetch that will fail to connect but properly handle the signal + let error: Error | null = null; + try { + const controller = new AbortController(); + // Abort immediately to test error handling + controller.abort(); + await fetch("http://localhost:9999/never-connects", { signal: controller.signal }); + } catch (err) { + error = err as Error; + } + + // Should have caught an abort error + expect(error).toBeDefined(); + }); +});