From 21db35d2b965fbda2ddf5b419ac2646fcae05517 Mon Sep 17 00:00:00 2001 From: cesnimda Date: Tue, 10 Nov 2026 17:40:51 +0100 Subject: [PATCH] fix(perf): keep superseded company reads out of cache --- docs/performance/PERF-01_COMPANIES_CACHE.md | 9 +++ .../src/account-scoped-state.test.tsx | 72 +++++++++++++++++-- job-tracker-ui/src/hooks/useCompanies.ts | 8 +-- 3 files changed, 78 insertions(+), 11 deletions(-) create mode 100644 docs/performance/PERF-01_COMPANIES_CACHE.md diff --git a/docs/performance/PERF-01_COMPANIES_CACHE.md b/docs/performance/PERF-01_COMPANIES_CACHE.md new file mode 100644 index 00000000..4aec10f7 --- /dev/null +++ b/docs/performance/PERF-01_COMPANIES_CACHE.md @@ -0,0 +1,9 @@ +# PERF-01 Companies cache + +Base `fd524a4b9d55d671d4f9c93134057d42f9cd6373`. The browser's Companies cache is scoped to the current owner. A response that started before invalidation could finish after its replacement and overwrite the fresh list. A pre-logout response could also write after signing back in as the same account. Separately, a mounting consumer could publish its initial empty array as cached data before the network request finished, so another consumer could miss the shared result. + +Only the current in-flight response can now populate the cache. It must still match the active owner. The resource-state effect no longer writes a second copy into the shared cache. Existing readers, invalidation callers, request URL and server authorisation are unchanged. + +Before the fix, the three added cases failed on the stale post-edit list, a missed shared response and the stale pre-logout list. After the fix, the two simultaneous readers both received the shared result with exactly one request. The post-invalidation check used exactly two requests across the old and new generations, with no extra request on a warm mount. Re-login and account switching checks also passed. + +**Verification:** 16 tests across the account-scoped state, Edit Job revision and theme state suites passed. Targeted lint and TypeScript passed. The test worktree reused the installed dependency tree from the matching A3 lockfile. These are mock request counts, with no browser or production latency measurement. diff --git a/job-tracker-ui/src/account-scoped-state.test.tsx b/job-tracker-ui/src/account-scoped-state.test.tsx index aafe970e..4a76eaf6 100644 --- a/job-tracker-ui/src/account-scoped-state.test.tsx +++ b/job-tracker-ui/src/account-scoped-state.test.tsx @@ -1,11 +1,11 @@ import React from "react"; import "@testing-library/jest-dom"; -import { fireEvent, render, screen, waitFor } from "@testing-library/react"; +import { act, fireEvent, render, screen, waitFor } from "@testing-library/react"; import { api } from "./api"; -import { setAuthUserKey } from "./auth"; +import { clearAuthClientState, setAuthUserKey } from "./auth"; import SavedViewsMenu from "./components/SavedViewsMenu"; -import { useCompanies } from "./hooks/useCompanies"; +import { invalidateCompaniesCache, useCompanies } from "./hooks/useCompanies"; import { I18nProvider } from "./i18n/I18nProvider"; const mockedApi = api as jest.Mocked; @@ -15,10 +15,20 @@ function CompaniesProbe() { return
{companies.map((company) => company.name).join(", ") || "No companies"}
; } +function holdCompanyRead() { + let finish!: (value: { data: { id: number; name: string }[] }) => void; + const response = new Promise<{ data: { id: number; name: string }[] }>(resolve => { + finish = resolve; + }); + mockedApi.get.mockReturnValueOnce(response as ReturnType); + return (name: string) => finish({ data: [{ id: 1, name }] }); +} + describe("account-scoped browser state", () => { beforeEach(() => { window.localStorage.clear(); jest.clearAllMocks(); + invalidateCompaniesCache(); }); it("keeps saved views private to the account that created them", async () => { @@ -54,9 +64,63 @@ describe("account-scoped browser state", () => { expect(await screen.findByText("User A Company")).toBeInTheDocument(); mockedApi.get.mockResolvedValueOnce({ data: [{ id: 2, name: "User B Company" }] } as any); - setAuthUserKey("user-b", false); + act(() => setAuthUserKey("user-b", false)); await waitFor(() => expect(screen.getByText("User B Company")).toBeInTheDocument()); expect(screen.queryByText("User A Company")).not.toBeInTheDocument(); }); + + it("does not let a pre-edit company response restore stale cache", async () => { + setAuthUserKey("user-a", false); + const finishOldRead = holdCompanyRead(); + const oldConsumer = render(); + await waitFor(() => expect(mockedApi.get).toHaveBeenCalledTimes(1)); + + invalidateCompaniesCache(); + const finishFreshRead = holdCompanyRead(); + const freshConsumer = render(); + expect(mockedApi.get).toHaveBeenCalledTimes(2); + await act(async () => finishFreshRead("Current company")); + expect(freshConsumer.container).toHaveTextContent("Current company"); + + await act(async () => finishOldRead("Before the edit")); + oldConsumer.unmount(); + freshConsumer.unmount(); + const later = render(); + expect(later.container).toHaveTextContent("Current company"); + expect(mockedApi.get).toHaveBeenCalledTimes(2); + }); + + it("shares one pending company read between callers without treating initial empty state as cached", async () => { + setAuthUserKey("user-a", false); + const finishRead = holdCompanyRead(); + const first = render(); + const second = render(); + expect(mockedApi.get).toHaveBeenCalledTimes(1); + + await act(async () => finishRead("Shared company")); + expect(first.container).toHaveTextContent("Shared company"); + expect(second.container).toHaveTextContent("Shared company"); + expect(mockedApi.get).toHaveBeenCalledTimes(1); + }); + + it("does not restore a pending company response from before logout after signing back in", async () => { + setAuthUserKey("user-a", false); + const finishOldRead = holdCompanyRead(); + const oldConsumer = render(); + await waitFor(() => expect(mockedApi.get).toHaveBeenCalledTimes(1)); + + mockedApi.get.mockResolvedValueOnce({ data: [] } as Awaited>); + act(() => clearAuthClientState(false)); + await waitFor(() => expect(mockedApi.get).toHaveBeenCalledTimes(2)); + mockedApi.get.mockResolvedValueOnce({ data: [{ id: 1, name: "New session company" }] } as Awaited>); + act(() => setAuthUserKey("user-a", false)); + expect(await screen.findByText("New session company")).toBeInTheDocument(); + + await act(async () => finishOldRead("Previous session company")); + oldConsumer.unmount(); + const later = render(); + expect(later.container).toHaveTextContent("New session company"); + expect(mockedApi.get).toHaveBeenCalledTimes(3); + }); }); diff --git a/job-tracker-ui/src/hooks/useCompanies.ts b/job-tracker-ui/src/hooks/useCompanies.ts index bcab6ccb..0bcf7fd9 100644 --- a/job-tracker-ui/src/hooks/useCompanies.ts +++ b/job-tracker-ui/src/hooks/useCompanies.ts @@ -31,7 +31,7 @@ async function fetchCompanies(): Promise { const request = api .get('/companies') .then((r) => { - if (cacheOwner === owner && getAuthUserKey() === owner) cachedCompanies = r.data; + if (inflight === request && cacheOwner === owner && getAuthUserKey() === owner) cachedCompanies = r.data; return r.data; }) .finally(() => { @@ -61,12 +61,6 @@ export function useCompanies(): { deps: [cacheBust], }); - useEffect(() => { - if (!resource.error) { - cachedCompanies = resource.data; - } - }, [resource.data, resource.error]); - useEffect(() => { const reloadForAccount = () => { resetForOwner(getAuthUserKey());