From 6a64c1372579f262f1a7752bb4b770744257e626 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 10 Sep 2026 17:26:49 +0000 Subject: [PATCH 1/2] refactor(client): drop the dead loader-data guards in three screens MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The charge, business and contracts screens each wrapped `useLoaderData()` in a try/catch with a `react-hooks/rules-of-hooks` disable, 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. `contracts.tsx` goes further — its route declares no loader at all, so `loaderData` was always undefined there and every branch reading it was unreachable. Deleted rather than rewritten. `charge.tsx` also loses a mount effect that re-executed a query already unpaused under the identical condition. I had expected this to be costing a second request; it was not. urql dedupes the re-execution against the still-in-flight operation, which the new test establishes by counting what a scripted `fetch` actually receives. The effect was redundant, not expensive, and the test now holds that count at one across the change. The test drives a `createMemoryRouter`, not a `MemoryRouter`: `useLoaderData` throws outside a data router. That is the thing the try/catch was really guarding, and it is why removing it is safe only for components that are exclusively route elements — an earlier draft of this test used `MemoryRouter` and failed for exactly that reason. `useRouteLoaderData` is deliberately not used: no route in `router/config.tsx` declares an `id`, and it would need one. Step 5 of the urql quick-wins sequence. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01TbrGL3NndzRiEwJkwHxnbm --- .changeset/urql-loader-data-guards.md | 25 ++++++ .../screens/businesses/business.tsx | 10 +-- .../clients/contracts/contracts.tsx | 21 +---- .../screens/charges/__tests__/charge.test.tsx | 82 +++++++++++++++++++ .../src/components/screens/charges/charge.tsx | 21 +---- 5 files changed, 116 insertions(+), 43 deletions(-) create mode 100644 .changeset/urql-loader-data-guards.md create mode 100644 packages/client/src/components/screens/charges/__tests__/charge.test.tsx 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..425c0e29a7 --- /dev/null +++ b/packages/client/src/components/screens/charges/__tests__/charge.test.tsx @@ -0,0 +1,82 @@ +// @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, so every mount + // cost two round-trips — and with no cache exchange, nothing absorbed the + // second one. + 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; From 9a5483d113c5306736d3238266ed9d35ab6b7e19 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 12 Sep 2026 12:38:19 +0000 Subject: [PATCH 2/2] test(client): correct the charge-screen test's account of what it guards The comment still claimed the removed mount effect cost "two round-trips". It did not: urql dedupes the re-execution against the operation still in flight, so the scripted fetch saw one request before the change as well as after. I had corrected this in the changeset and PR description but left the comment saying the opposite, which would have taught the next reader the wrong reason for the cleanup. The assertion holds the count at one across the removal; it does not record a fix. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01TbrGL3NndzRiEwJkwHxnbm --- .../components/screens/charges/__tests__/charge.test.tsx | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/packages/client/src/components/screens/charges/__tests__/charge.test.tsx b/packages/client/src/components/screens/charges/__tests__/charge.test.tsx index 425c0e29a7..652fd7833f 100644 --- a/packages/client/src/components/screens/charges/__tests__/charge.test.tsx +++ b/packages/client/src/components/screens/charges/__tests__/charge.test.tsx @@ -52,10 +52,11 @@ afterEach(async () => { 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, so every mount - // cost two round-trips — and with no cache exchange, nothing absorbed the - // second one. + // 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);