Skip to content

Commit 2cf355c

Browse files
authored
feat(editor): move admin settings onto TanStack Query (#7437)
# Description of Changes ## The problem `useAdminSettings` backs all 18 admin config sections. Each section fetched its own copy of its settings block, held it in hand-rolled loading/saving state, and refetched manually after every save. Three consequences: - **Duplicate fetching.** Four AI tabs all read the `aiEngine` block. Nothing was shared, so each open refetched it. - **Duplicated wiring.** All 18 sections carried the same effect to trigger the fetch, each one depending on a `fetchSettings` callback that would have refetched on every render had it ever become unstable. - **Console noise.** The hook made 11 `console.*` calls, four of them `JSON.stringify(settings, null, 2)` on **every fetch and every save** — admin configuration serialised into the console of every admin session. Every save also ended with a hand-written `await fetchSettings()`. Forget it in a new section and its pending badges silently go stale. ## The fix The hook uses TanStack Query, keyed on `sectionName`, so sections reading the same block share one fetch and one cache entry. The fetch gate moved into the hook. Sections used to write: ```ts const { settings, fetchSettings } = useAdminSettings({ sectionName: "legal" }); useEffect(() => { if (loginEnabled) fetchSettings(); }, [loginEnabled, fetchSettings]); ``` and now write: ```ts const { settings } = useAdminSettings({ sectionName: "legal", enabled: loginEnabled, }); ``` Saving is a mutation that invalidates the section on success, so the refetch is structural rather than something each section remembers. The delta computation and the save transformer are unchanged — that is domain logic, not fetching. `settings` is still an editable draft seeded from the server response, so forms behave exactly as before. ## Why it is better Measured against the previous implementation across identical scenarios. `commits` counts committed renders. | Scenario | Before | After | |---|---|---| | Open one section | 2 commits, 1 request | 2 commits, 1 request | | Browse the four AI tabs | 8 commits, 4 requests | **5 commits, 1 request** | | Edit and save | 4 commits, 2 requests | 4 commits, 2 requests | Committed renders are equal or better everywhere; browsing the AI tabs costs a quarter of the requests. The diff reads +449 / −303, but that includes a test file for a hook that had no tests: | | Added | Removed | Net | |---|---|---|---| | Production code (21 files) | 154 | 303 | **−149** | | Tests (1 file) | 295 | 0 | +295 | The 18 section files account for −133 of that: each drops an effect, a destructure and usually an import, and gains one `enabled:` line. The hook itself goes from 234 to 180 lines. `console.*` calls go from 11 to 0. ## Caching Settings inherit the client's 30s stale window rather than refetching on every mount, which is where the request saving comes from. Nothing inside a cached block is server-observed — the only live reads in these sections, `/api/v1/ai/health` and the tessdata language list, are separate calls outside this query. A block therefore only changes when another admin writes it. Two things bound the staleness: - Sections already held a single snapshot for as long as the modal stayed open, with no refetch on focus. 30s is shorter than that window, not longer. - `computeDelta` only emits fields whose draft differs from the baseline it was seeded from, so a stale baseline cannot produce a collateral write. The only race is two admins editing the same field, which is unchanged. Saving invalidates, so acting refreshes to current values. The blocks where a stale read would matter most — `security`, `premium`, `database` — are set once at deployment and effectively never edited concurrently. The block with the most cache reuse, `aiEngine`, is the least consequential. **Convention:** config blocks cache; observed state does not. A section that displays live server state inside its settings block should override `staleTime` locally. ## Testing 14 tests, covering the shared fetch, cache reuse across tab reopens, key separation between blocks, the `enabled` gate, delta-only saves, the empty-delta short circuit, post-save invalidation, pending-value display, and draft reseeding. Each was checked by breaking the implementation and confirming the suite fails: per-consumer query keys, sending the whole draft instead of the delta, dropping the post-save invalidate, reporting loaded while disabled, skipping the empty-delta short circuit, and reverting the stale window to zero. `task frontend:check` green. Two unrelated tests fail on this branch — `workbenchSession.test.ts` and `notificationActions.test.tsx` — and fail identically on `main`. ## Follow-ups The sections that fetch through services rather than this hook — Teams, TeamDetails, People, roughly 2,600 lines — are unchanged. Between them they share two reads (`getTeams` and `getUsers`, both used by all three) and carry ten distinct write operations, with no test coverage today. --- ## Primer: mutations `useQuery` is for reads. It caches, dedupes, and re-renders when data arrives. `useMutation` is for writes, where none of that applies — a write happens once, when the user asks. ```ts const save = useMutation({ mutationFn: (body) => putAdminSection("legal", body), onSuccess: () => queryClient.invalidateQueries({ queryKey }), }); save.mutate(body); // fire and forget await save.mutateAsync(body); // or await it save.isPending; // disable the button save.error; // show the failure ``` `isPending` and `error` replace the `useState` flag and `try/catch/finally` you would otherwise write around every save. After a write the cache holds stale data. Two ways to fix it: | | What it does | Use when | |---|---|---| | `invalidateQueries` | Marks the data stale so it refetches | The server may transform, queue or reject part of what you sent | | `setQueryData` | Writes your value into the cache, no request | The response tells you exactly what the server now holds | **Invalidate by default. Use `setQueryData` only when the response is authoritative.** This hook has to invalidate: the server can queue a settings change rather than applying it, returning it in a `_pending` block that the form renders as a badge. Writing the local draft into the cache would show a queued change as applied. Most mutations are not like that. A "rename a team" write, where the response is the new team, is a `setQueryData` case. One gotcha: `mutate` does not throw, `mutateAsync` does. An awaited `mutateAsync` without a `try/catch` is an unhandled rejection.
1 parent c57a2a4 commit 2cf355c

22 files changed

Lines changed: 476 additions & 315 deletions
Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,22 @@
1+
import apiClient from "@app/services/apiClient";
2+
3+
export async function fetchAdminSection<T>(sectionName: string): Promise<T> {
4+
const response = await apiClient.get<T>(
5+
`/api/v1/admin/settings/section/${sectionName}`,
6+
);
7+
return (response.data ?? {}) as T;
8+
}
9+
10+
export async function putAdminSection(
11+
sectionName: string,
12+
delta: unknown,
13+
): Promise<void> {
14+
await apiClient.put(`/api/v1/admin/settings/section/${sectionName}`, delta);
15+
}
16+
17+
/** Flat dotted-path settings, for sections that write outside their own block. */
18+
export async function putAdminSettings(
19+
settings: Record<string, unknown>,
20+
): Promise<void> {
21+
await apiClient.put("/api/v1/admin/settings", { settings });
22+
}
Lines changed: 313 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,313 @@
1+
import { describe, it, expect, vi, beforeEach } from "vitest";
2+
import { renderHook, waitFor, act } from "@testing-library/react";
3+
import { QueryClient, QueryClientProvider } from "@tanstack/react-query";
4+
import type { ReactNode } from "react";
5+
import { useAdminSettings } from "@app/hooks/useAdminSettings";
6+
import { qk } from "@app/query/keys";
7+
import {
8+
fetchAdminSection,
9+
putAdminSection,
10+
putAdminSettings,
11+
} from "@app/api/adminSettings";
12+
13+
vi.mock("@app/api/adminSettings", () => ({
14+
fetchAdminSection: vi.fn(),
15+
putAdminSection: vi.fn(),
16+
putAdminSettings: vi.fn(),
17+
}));
18+
19+
const mockFetch = vi.mocked(fetchAdminSection);
20+
const mockPutSection = vi.mocked(putAdminSection);
21+
const mockPutSettings = vi.mocked(putAdminSettings);
22+
23+
function makeWrapper() {
24+
const client = new QueryClient({
25+
defaultOptions: { queries: { retry: false } },
26+
});
27+
return ({ children }: { children: ReactNode }) => (
28+
<QueryClientProvider client={client}>{children}</QueryClientProvider>
29+
);
30+
}
31+
32+
describe("useAdminSettings", () => {
33+
beforeEach(() => {
34+
vi.clearAllMocks();
35+
mockFetch.mockResolvedValue({ appName: "Stirling" });
36+
mockPutSection.mockResolvedValue(undefined);
37+
mockPutSettings.mockResolvedValue(undefined);
38+
});
39+
40+
it("loads the section and seeds the editable draft", async () => {
41+
const { result } = renderHook(
42+
() => useAdminSettings({ sectionName: "general" }),
43+
{ wrapper: makeWrapper() },
44+
);
45+
46+
expect(result.current.loading).toBe(true);
47+
await waitFor(() => expect(result.current.loading).toBe(false));
48+
expect(result.current.settings).toEqual({ appName: "Stirling" });
49+
expect(mockFetch).toHaveBeenCalledWith("general");
50+
});
51+
52+
it("shares one fetch between sections reading the same block", async () => {
53+
const { result } = renderHook(
54+
() => ({
55+
a: useAdminSettings({ sectionName: "aiEngine" }),
56+
b: useAdminSettings({ sectionName: "aiEngine" }),
57+
c: useAdminSettings({ sectionName: "aiEngine" }),
58+
}),
59+
{ wrapper: makeWrapper() },
60+
);
61+
62+
await waitFor(() => expect(result.current.a.loading).toBe(false));
63+
expect(mockFetch).toHaveBeenCalledTimes(1);
64+
});
65+
66+
it("serves a reopened tab from cache within the stale window", async () => {
67+
const client = new QueryClient({
68+
defaultOptions: { queries: { retry: false, staleTime: 30_000 } },
69+
});
70+
const shared = ({ children }: { children: ReactNode }) => (
71+
<QueryClientProvider client={client}>{children}</QueryClientProvider>
72+
);
73+
74+
for (let i = 0; i < 4; i++) {
75+
const tab = renderHook(
76+
() => useAdminSettings({ sectionName: "aiEngine" }),
77+
{ wrapper: shared },
78+
);
79+
await waitFor(() => expect(tab.result.current.loading).toBe(false));
80+
tab.unmount();
81+
}
82+
83+
expect(mockFetch).toHaveBeenCalledTimes(1);
84+
});
85+
86+
it("keeps sections with different blocks apart", async () => {
87+
const { result } = renderHook(
88+
() => ({
89+
a: useAdminSettings({ sectionName: "general" }),
90+
b: useAdminSettings({ sectionName: "security" }),
91+
}),
92+
{ wrapper: makeWrapper() },
93+
);
94+
95+
await waitFor(() => expect(result.current.a.loading).toBe(false));
96+
expect(mockFetch).toHaveBeenCalledTimes(2);
97+
expect(mockFetch).toHaveBeenCalledWith("general");
98+
expect(mockFetch).toHaveBeenCalledWith("security");
99+
});
100+
101+
it("does not fetch while disabled, and reports itself unloaded", async () => {
102+
const { result } = renderHook(
103+
() => useAdminSettings({ sectionName: "general", enabled: false }),
104+
{
105+
wrapper: makeWrapper(),
106+
},
107+
);
108+
109+
expect(mockFetch).not.toHaveBeenCalled();
110+
// Sections gate their render on this; false would show an empty form.
111+
expect(result.current.loading).toBe(true);
112+
});
113+
114+
it("fetches when the gate opens", async () => {
115+
const { result, rerender } = renderHook(
116+
({ on }: { on: boolean }) =>
117+
useAdminSettings({ sectionName: "general", enabled: on }),
118+
{ wrapper: makeWrapper(), initialProps: { on: false } },
119+
);
120+
121+
expect(mockFetch).not.toHaveBeenCalled();
122+
rerender({ on: true });
123+
await waitFor(() => expect(result.current.loading).toBe(false));
124+
expect(mockFetch).toHaveBeenCalledTimes(1);
125+
});
126+
127+
it("sends only changed fields", async () => {
128+
mockFetch.mockResolvedValue({ appName: "Stirling", theme: "dark" });
129+
const { result } = renderHook(
130+
() =>
131+
useAdminSettings<{ appName: string; theme: string }>({
132+
sectionName: "general",
133+
}),
134+
{ wrapper: makeWrapper() },
135+
);
136+
await waitFor(() => expect(result.current.loading).toBe(false));
137+
138+
act(() => {
139+
result.current.setSettings({ appName: "Renamed", theme: "dark" });
140+
});
141+
await act(async () => {
142+
await result.current.saveSettings();
143+
});
144+
145+
expect(mockPutSection).toHaveBeenCalledWith("general", {
146+
appName: "Renamed",
147+
});
148+
});
149+
150+
it("skips the request when nothing changed", async () => {
151+
const { result } = renderHook(
152+
() => useAdminSettings({ sectionName: "general" }),
153+
{ wrapper: makeWrapper() },
154+
);
155+
await waitFor(() => expect(result.current.loading).toBe(false));
156+
157+
await act(async () => {
158+
await result.current.saveSettings();
159+
});
160+
161+
expect(mockPutSection).not.toHaveBeenCalled();
162+
});
163+
164+
it("refetches after a save so the _pending block is current", async () => {
165+
mockFetch.mockResolvedValue({ appName: "Stirling" });
166+
const { result } = renderHook(
167+
() => useAdminSettings<{ appName: string }>({ sectionName: "general" }),
168+
{
169+
wrapper: makeWrapper(),
170+
},
171+
);
172+
await waitFor(() => expect(result.current.loading).toBe(false));
173+
expect(mockFetch).toHaveBeenCalledTimes(1);
174+
175+
mockFetch.mockResolvedValue({
176+
appName: "Stirling",
177+
_pending: { appName: "Renamed" },
178+
});
179+
act(() => {
180+
result.current.setSettings({ appName: "Renamed" });
181+
});
182+
await act(async () => {
183+
await result.current.saveSettings();
184+
});
185+
186+
await waitFor(() => expect(mockFetch).toHaveBeenCalledTimes(2));
187+
await waitFor(() => expect(result.current.hasPendingChanges()).toBe(true));
188+
});
189+
190+
it("surfaces pending values in the draft and flags the field", async () => {
191+
mockFetch.mockResolvedValue({
192+
appName: "Stirling",
193+
_pending: { appName: "Queued" },
194+
});
195+
const { result } = renderHook(
196+
() => useAdminSettings<{ appName: string }>({ sectionName: "general" }),
197+
{
198+
wrapper: makeWrapper(),
199+
},
200+
);
201+
202+
await waitFor(() => expect(result.current.loading).toBe(false));
203+
// The draft shows the queued value, not the active one.
204+
expect(result.current.settings.appName).toBe("Queued");
205+
expect(result.current.isFieldPending("appName")).toBe(true);
206+
});
207+
208+
it("resets the draft when a fetch delivers new values", async () => {
209+
const client = new QueryClient({
210+
defaultOptions: { queries: { retry: false } },
211+
});
212+
const { result } = renderHook(
213+
() => useAdminSettings<{ appName: string }>({ sectionName: "general" }),
214+
{
215+
wrapper: ({ children }: { children: ReactNode }) => (
216+
<QueryClientProvider client={client}>{children}</QueryClientProvider>
217+
),
218+
},
219+
);
220+
await waitFor(() => expect(result.current.loading).toBe(false));
221+
222+
act(() => {
223+
result.current.setSettings({ appName: "Half-typed" });
224+
});
225+
expect(result.current.settings.appName).toBe("Half-typed");
226+
227+
mockFetch.mockResolvedValue({ appName: "From server" });
228+
await act(async () => {
229+
await client.invalidateQueries({ queryKey: qk.adminSection("general") });
230+
});
231+
232+
// A fetch is authoritative over the draft.
233+
await waitFor(() =>
234+
expect(result.current.settings.appName).toBe("From server"),
235+
);
236+
});
237+
238+
it("does not clobber an in-progress edit on re-render", async () => {
239+
const { result, rerender } = renderHook(
240+
() => useAdminSettings<{ appName: string }>({ sectionName: "general" }),
241+
{
242+
wrapper: makeWrapper(),
243+
},
244+
);
245+
await waitFor(() => expect(result.current.loading).toBe(false));
246+
247+
act(() => {
248+
result.current.setSettings({ appName: "Half-typed" });
249+
});
250+
rerender();
251+
rerender();
252+
253+
expect(result.current.settings.appName).toBe("Half-typed");
254+
});
255+
256+
it("routes transformer output to both endpoints", async () => {
257+
mockFetch.mockResolvedValue({ a: 1, b: 2 });
258+
const { result } = renderHook(
259+
() =>
260+
useAdminSettings<{ a: number; b: number }>({
261+
sectionName: "general",
262+
saveTransformer: (s) => ({
263+
sectionData: { a: s.a },
264+
deltaSettings: { "some.flat.path": s.b },
265+
}),
266+
}),
267+
{ wrapper: makeWrapper() },
268+
);
269+
await waitFor(() => expect(result.current.loading).toBe(false));
270+
271+
act(() => {
272+
result.current.setSettings({ a: 9, b: 8 });
273+
});
274+
await act(async () => {
275+
await result.current.saveSettings();
276+
});
277+
278+
expect(mockPutSection).toHaveBeenCalledWith("general", { a: 9 });
279+
expect(mockPutSettings).toHaveBeenCalledWith({ "some.flat.path": 8 });
280+
});
281+
282+
it("reports saving while the save is in flight", async () => {
283+
const { result } = renderHook(
284+
() => useAdminSettings<{ appName: string }>({ sectionName: "general" }),
285+
{
286+
wrapper: makeWrapper(),
287+
},
288+
);
289+
await waitFor(() => expect(result.current.loading).toBe(false));
290+
291+
let release: () => void = () => {};
292+
mockPutSection.mockReturnValueOnce(
293+
new Promise<void>((resolve) => {
294+
release = resolve;
295+
}),
296+
);
297+
298+
act(() => {
299+
result.current.setSettings({ appName: "Renamed" });
300+
});
301+
let done: Promise<void>;
302+
act(() => {
303+
done = result.current.saveSettings();
304+
});
305+
await waitFor(() => expect(result.current.saving).toBe(true));
306+
307+
await act(async () => {
308+
release();
309+
await done;
310+
});
311+
await waitFor(() => expect(result.current.saving).toBe(false));
312+
});
313+
});

0 commit comments

Comments
 (0)