diff --git a/.changeset/urql-loader-data-guards.md b/.changeset/urql-loader-data-guards.md new file mode 100644 index 0000000000..263edd460a --- /dev/null +++ b/.changeset/urql-loader-data-guards.md @@ -0,0 +1,25 @@ +--- +'@accounter/client': patch +--- + +Simplify how the charge, business and contracts screens read their route loader data. + +All three wrapped `useLoaderData()` in a `try/catch` with an +`// eslint-disable-next-line react-hooks/rules-of-hooks`, guarding against being rendered outside a +data router. None of them can be: each is only ever a lazy route element of `createBrowserRouter`, +and none has a JSX call site anywhere in the app. The guard is now removed and the disable with it. + +`contracts.tsx` goes further. Its route declares no loader at all — only `chargeLoader` and +`businessLoader` exist — so `loaderData` was always `undefined` there and every branch reading it was +unreachable. The variable, its import and the `|| loaderData` / `!!loaderData` branches are deleted +rather than rewritten. + +`charge.tsx` also drops a mount effect that re-executed a query already unpaused under the identical +condition. Contrary to what the removal might suggest, this was not costing a second request: urql +dedupes the re-execution against the still-in-flight operation, which a new test pins down by +counting the operations a scripted `fetch` actually receives. The effect was redundant, not +expensive, and the test now guards that the count stays at one. + +That test is written against `createMemoryRouter` rather than `MemoryRouter`, because +`useLoaderData` throws outside a data router — which is also why the `try/catch` existed and why it +is safe to remove only for components that are exclusively route elements. diff --git a/packages/client/src/components/screens/businesses/business.tsx b/packages/client/src/components/screens/businesses/business.tsx index b6d01cfeef..ac74fd2ff4 100644 --- a/packages/client/src/components/screens/businesses/business.tsx +++ b/packages/client/src/components/screens/businesses/business.tsx @@ -18,14 +18,8 @@ import { AccounterLoader } from '../../common/index.js'; export const BusinessScreen = (): ReactElement => { const { businessId } = useParams<{ businessId: string }>(); - // Try to get loader data (will be available when navigating via router) - let loaderData: BusinessScreenQuery | undefined; - try { - // eslint-disable-next-line react-hooks/rules-of-hooks - loaderData = useLoaderData() as BusinessScreenQuery; - } catch { - // No loader data - fallback to query - } + // Present when the route's loader ran; `undefined` on a route without one. + const loaderData = useLoaderData() as BusinessScreenQuery | undefined; // Only fetch if we don't have loader data const [{ data, fetching }, fetchBusiness] = useQuery({ diff --git a/packages/client/src/components/screens/businesses/clients/contracts/contracts.tsx b/packages/client/src/components/screens/businesses/clients/contracts/contracts.tsx index 68d123a566..3c1772ffbb 100644 --- a/packages/client/src/components/screens/businesses/clients/contracts/contracts.tsx +++ b/packages/client/src/components/screens/businesses/clients/contracts/contracts.tsx @@ -1,9 +1,8 @@ import { useContext, type ReactElement } from 'react'; -import { useLoaderData } from 'react-router-dom'; import { useQuery } from 'urql'; import { AccounterLoader } from '@/components/common/index.js'; import { ContractsTable } from '@/components/contracts/index.js'; -import { ContractsScreenDocument, type ContractsScreenQuery } from '@/gql/graphql.js'; +import { ContractsScreenDocument } from '@/gql/graphql.js'; import { UserContext } from '@/providers/index.js'; // eslint-disable-next-line @typescript-eslint/no-unused-expressions -- used by codegen @@ -20,28 +19,14 @@ export const ContractsScreen = (): ReactElement => { const { userContext } = useContext(UserContext); const adminId = userContext?.context.adminBusinessId; - // Try to get loader data (will be available when navigating via router) - let loaderData: ContractsScreenQuery | undefined; - try { - // eslint-disable-next-line react-hooks/rules-of-hooks - loaderData = useLoaderData() as ContractsScreenQuery; - } catch { - // No loader data - fallback to query - } - - // Only fetch if we don't have loader data - const [{ data, fetching }] = useQuery({ + const [{ data: contractsData, fetching: isLoading }] = useQuery({ query: ContractsScreenDocument, - pause: !adminId || !!loaderData, + pause: !adminId, variables: { adminId: adminId ?? '', }, }); - // Use loader data if available, otherwise use query data - const contractsData = loaderData || data; - const isLoading = !loaderData && fetching; - if (isLoading && !contractsData?.contractsByAdmin) { return ; } diff --git a/packages/client/src/components/screens/charges/__tests__/charge.test.tsx b/packages/client/src/components/screens/charges/__tests__/charge.test.tsx new file mode 100644 index 0000000000..652fd7833f --- /dev/null +++ b/packages/client/src/components/screens/charges/__tests__/charge.test.tsx @@ -0,0 +1,83 @@ +// @vitest-environment happy-dom + +import { act } from 'react'; +import { createRoot, type Root } from 'react-dom/client'; +import { RouterProvider, createMemoryRouter } from 'react-router-dom'; +import { Client, Provider, fetchExchange } from 'urql'; +import { afterEach, beforeEach, describe, expect, it } from 'vitest'; +import { Charge } from '../charge.js'; + +(globalThis as unknown as { IS_REACT_ACT_ENVIRONMENT: boolean }).IS_REACT_ACT_ENVIRONMENT = true; + +/** Builds an urql client backed by a scripted, request-recording fetch. */ +function makeClient(operations: string[]): Client { + const mockFetch = (async (_url: string, init: RequestInit) => { + const body = JSON.parse(String(init.body)) as { operationName: string }; + operations.push(body.operationName); + const payload = JSON.stringify({ data: { charge: null } }); + return { + status: 200, + ok: true, + headers: new Headers({ 'Content-Type': 'application/json' }), + text: async () => payload, + json: async () => JSON.parse(payload), + } as unknown as Response; + }) as unknown as typeof fetch; + + return new Client({ + url: 'http://localhost/graphql', + exchanges: [fetchExchange], + fetch: mockFetch, + // Keep every operation on POST so the harness can read the body. + preferGetMethod: false, + }); +} + +let container: HTMLDivElement; +let root: Root; + +beforeEach(() => { + container = document.createElement('div'); + document.body.append(container); + root = createRoot(container); +}); + +afterEach(async () => { + await act(async () => { + root.unmount(); + await Promise.resolve(); + }); + container.remove(); +}); + +describe('Charge screen', () => { + it('fetches the charge exactly once on mount', async () => { + // The screen used to pair an already-unpaused `useQuery` with a mount effect + // that re-executed it under the identical condition. That never cost a + // second request — urql dedupes the re-execution against the operation still + // in flight — so this count was already 1 before the effect was removed. The + // assertion is here to keep it that way, not to record a fix. + const operations: string[] = []; + const client = makeClient(operations); + + // A *data* router, not `MemoryRouter`: `useLoaderData` throws outside one, + // and in the app these screens are only ever reached as route elements of + // `createBrowserRouter`. The route deliberately declares no loader, which + // is what leaves `useLoaderData` undefined and lets the query run. + const router = createMemoryRouter( + [{ path: '/charges/:chargeId', element: }], + { initialEntries: ['/charges/charge-1'] }, + ); + + await act(async () => { + root.render( + + + , + ); + await Promise.resolve(); + }); + + expect(operations.filter(name => name === 'ChargeScreen')).toHaveLength(1); + }); +}); diff --git a/packages/client/src/components/screens/charges/charge.tsx b/packages/client/src/components/screens/charges/charge.tsx index ec2a9c50b0..e994e43799 100644 --- a/packages/client/src/components/screens/charges/charge.tsx +++ b/packages/client/src/components/screens/charges/charge.tsx @@ -1,4 +1,4 @@ -import { useEffect, type ReactElement } from 'react'; +import type { ReactElement } from 'react'; import { useLoaderData, useParams } from 'react-router-dom'; import { useQuery } from 'urql'; import { ChargesTable } from '@/components/charges/charges-table.js'; @@ -23,17 +23,10 @@ export const Charge = ({ chargeId }: Props): ReactElement => { const { chargeId: chargeIdFromUrl } = useParams<{ chargeId: string }>(); const id = chargeId || chargeIdFromUrl; - // Try to get loader data (will be available when navigating via router) - let loaderData: ChargeScreenQuery | undefined; - try { - // eslint-disable-next-line react-hooks/rules-of-hooks - loaderData = useLoaderData() as ChargeScreenQuery; - } catch { - // No loader data - component used outside router context (e.g., as child component) - } + // Present when the route's loader ran; `undefined` on a route without one. + const loaderData = useLoaderData() as ChargeScreenQuery | undefined; - // Only fetch if we don't have loader data and need to fetch (prop-based usage) - const [{ data, fetching }, fetchCharge] = useQuery({ + const [{ data, fetching }] = useQuery({ query: ChargeScreenDocument, pause: !id || !!loaderData, variables: { @@ -41,12 +34,6 @@ export const Charge = ({ chargeId }: Props): ReactElement => { }, }); - useEffect(() => { - if (id && !loaderData) { - fetchCharge(); - } - }, [id, loaderData, fetchCharge]); - // Use loader data if available, otherwise use query data const chargeData = loaderData || data; const isLoading = !loaderData && fetching;