From 81ad3ef5ef96f5b7a14dd0812c5154ccb2bfd3b3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Padez?= Date: Fri, 7 Aug 2026 09:15:55 +0000 Subject: [PATCH] let two windows onto the same dashboards agree again MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The dashboard-state cache had staleTime: Infinity and there is no invalidateQueries anywhere in the repo, so it was fetched once per page load and never again: two windows diverged permanently and neither was ever told. It now refetches on focus — with three non-default guards, because this cache is optimistic and a refetch that started before an in-flight PATCH landed would overwrite the value we already showed. Never on mount (splitting a panel mounts a fresh consumer, which is exactly when a write is in flight), never on reconnect, and on focus only after a short quiet period with nothing in flight. The PATCH stopped assembling a full state blob it then returned to nobody — three SELECTs per splitter release, thrown away, and a caller that did read it would be reading state assembled before whatever concurrent write it raced. And the last three `.catch(() => {})` in this family are gone: dashboard create, rename and delete build their own multi-key patches and so bypass the hook. They now go through persistDashboardState, which keeps the in-flight bookkeeping honest and, on failure, invalidates rather than reverts — there is no single previous value to swap back once the roster has been rewritten, and a refetch is the only thing that makes the list agree with the server. A failed delete used to leave the dashboard gone from the list and alive on the server, reappearing at the next reload with no hint why. Co-Authored-By: Claude Opus 5 --- src/servers/api/dashboards/dashboards.ts | 6 +- .../src/apps/Dashboards/DashboardListApp.tsx | 4 +- .../src/apps/Dashboards/DashboardPreview.tsx | 8 +- src/workspaces/state/src/useDashboardState.ts | 85 ++++++++++++++++--- 4 files changed, 85 insertions(+), 18 deletions(-) diff --git a/src/servers/api/dashboards/dashboards.ts b/src/servers/api/dashboards/dashboards.ts index 2ed0907b..b0ec103b 100644 --- a/src/servers/api/dashboards/dashboards.ts +++ b/src/servers/api/dashboards/dashboards.ts @@ -121,6 +121,8 @@ dashboardsRouter.patch('/', async (ctx) => { return ctx.json({ error: `unknown dashboard-state key "${key}"` }, 400); } - const state = await getAllDashboardState(userId); - return ctx.json(state); + // Three SELECTs per splitter release, thrown away — no caller has ever read this response body, and one + // that did would be reading a blob assembled *before* the concurrent write it raced. The client + // converges by refetching on focus instead (`useDashboardState`). + return ctx.json({ ok: true }); }); diff --git a/src/workspaces/officerdev/src/apps/Dashboards/DashboardListApp.tsx b/src/workspaces/officerdev/src/apps/Dashboards/DashboardListApp.tsx index 1abd9b91..ebd98cae 100644 --- a/src/workspaces/officerdev/src/apps/Dashboards/DashboardListApp.tsx +++ b/src/workspaces/officerdev/src/apps/Dashboards/DashboardListApp.tsx @@ -6,7 +6,7 @@ import { useGlobal } from 'hooks/useGlobal'; import { useClient } from 'hooks/useClient'; import { generateSlug } from 'helpers/slug'; import { useUserState } from 'state/useUserState'; -import { useDashboardState } from 'state/useDashboardState'; +import { useDashboardState, persistDashboardState } from 'state/useDashboardState'; import type { DashboardDefinition } from '../../components/Workspace'; import { AlertDialog, @@ -90,7 +90,7 @@ export const DashboardListApp = () => { const currentState = queryClient.getQueryData>(['DASHBOARD_STATE']) ?? {}; const { [layoutKey]: _, ...rest } = currentState; queryClient.setQueryData(['DASHBOARD_STATE'], rest); - client.patch('/dashboards', { [layoutKey]: null }).catch(() => {}); + persistDashboardState(client, queryClient, { [layoutKey]: null }); setDeleting(null); }; diff --git a/src/workspaces/officerdev/src/apps/Dashboards/DashboardPreview.tsx b/src/workspaces/officerdev/src/apps/Dashboards/DashboardPreview.tsx index ce92281a..0feaa44f 100644 --- a/src/workspaces/officerdev/src/apps/Dashboards/DashboardPreview.tsx +++ b/src/workspaces/officerdev/src/apps/Dashboards/DashboardPreview.tsx @@ -5,7 +5,7 @@ import { useQueryClient } from '@tanstack/react-query'; import { useGlobal } from 'hooks/useGlobal'; import { useClient } from 'hooks/useClient'; import { useUserState } from 'state/useUserState'; -import { useDashboardState } from 'state/useDashboardState'; +import { useDashboardState, persistDashboardState } from 'state/useDashboardState'; import { WorkspaceLayout, WorkspaceView, createDefaultLayout } from '../../components/Workspace'; import type { LayoutNode, DashboardDefinition } from '../../components/Workspace'; import { Button } from '@/components/ui/button'; @@ -320,10 +320,10 @@ const CreatePanel = () => { const patch: Record = { [newLayoutKey]: wsLayout }; for (const k of oldKeys) patch[k] = null; - client.patch('/dashboards', patch).catch(() => {}); + persistDashboardState(client, queryClient, patch); } else { queryClient.setQueryData(['DASHBOARD_STATE'], { ...currentState, [newLayoutKey]: wsLayout }); - client.patch('/dashboards', { [newLayoutKey]: wsLayout }).catch(() => {}); + persistDashboardState(client, queryClient, { [newLayoutKey]: wsLayout }); } setEditingId(null); @@ -353,7 +353,7 @@ const CreatePanel = () => { setDashboards((prev) => [...prev, ws]); const currentState = queryClient.getQueryData>(['DASHBOARD_STATE']) ?? {}; queryClient.setQueryData(['DASHBOARD_STATE'], { ...currentState, [layoutKey]: wsLayout }); - client.patch('/dashboards', { [layoutKey]: wsLayout }).catch(() => {}); + persistDashboardState(client, queryClient, { [layoutKey]: wsLayout }); setName(''); setDescription(''); diff --git a/src/workspaces/state/src/useDashboardState.ts b/src/workspaces/state/src/useDashboardState.ts index b80725bb..1e1b33d2 100644 --- a/src/workspaces/state/src/useDashboardState.ts +++ b/src/workspaces/state/src/useDashboardState.ts @@ -5,7 +5,9 @@ import { useClient } from 'hooks/useClient'; import { useAuth } from 'hooks/useAuth'; import type { UserState } from './useSettings'; -const QUERY_KEY = ['DASHBOARD_STATE']; +export const DASHBOARD_STATE_QUERY_KEY = ['DASHBOARD_STATE']; + +const QUERY_KEY = DASHBOARD_STATE_QUERY_KEY; export function useDashboardState(key: string, defaultValue: T) { const client = useClient(); @@ -18,7 +20,22 @@ export function useDashboardState(key: string, defaultValue: T) { queryKey: QUERY_KEY, enabled: isAuthenticated, queryFn: () => client.get('/dashboards'), - staleTime: Infinity, + // `Infinity` meant this was fetched once per page load and never again, with no `invalidateQueries` + // anywhere in the repo — so two windows onto the same dashboards diverged permanently and neither was + // ever told. Refetching on focus is the cheapest thing that converges them: you come back to the tab + // and it catches up. + // + // The other three knobs are the safety around it, and they are not defaults. This cache is optimistic: + // a write updates it immediately and the PATCH follows. A refetch that started before that PATCH + // landed returns pre-write state and would overwrite the value we already showed — the same + // lost-update shape the layout updaters exist to prevent, and self-healing only until the next + // mutation composes on top of the stale tree. So: never on mount (splitting a panel mounts a fresh + // consumer, which is precisely when a write is in flight), never on reconnect, and on focus only when + // nothing has been written for a moment. + staleTime: 30_000, + refetchOnMount: false, + refetchOnReconnect: false, + refetchOnWindowFocus: () => inFlightWrites === 0 && Date.now() - lastWriteAt > 2_000, }); // Seed default to backend when key is missing after initial fetch @@ -30,9 +47,9 @@ export function useDashboardState(key: string, defaultValue: T) { const currentState = queryClient.getQueryData(QUERY_KEY) ?? {}; if (!(key in currentState)) { queryClient.setQueryData(QUERY_KEY, { ...currentState, [key]: defaultValue }); - clientRef.current - .patch('/dashboards', { [key]: defaultValue }) - .catch((err: unknown) => revert(queryClient, key, defaultValue, undefined, err)); + persist(clientRef.current, key, defaultValue).catch((err: unknown) => + revert(queryClient, key, defaultValue, undefined, err), + ); } }, [isSuccess, key, defaultValue, queryClient]); @@ -46,11 +63,9 @@ export function useDashboardState(key: string, defaultValue: T) { queryClient.setQueryData(QUERY_KEY, { ...currentState, [key]: newValue }); - clientRef.current - .patch('/dashboards', { [key]: newValue }) - .catch((err: unknown) => - revert(queryClient, key, newValue, key in currentState ? currentValue : undefined, err), - ); + persist(clientRef.current, key, newValue).catch((err: unknown) => + revert(queryClient, key, newValue, key in currentState ? currentValue : undefined, err), + ); }, [key, defaultValue, queryClient], ); @@ -58,6 +73,56 @@ export function useDashboardState(key: string, defaultValue: T) { return { key, value, setValue, isLoaded: isSuccess }; } +/** + * Every instance of this hook addresses one query key, so the write-in-flight bookkeeping the focus + * refetch reads is module-level rather than per-instance. `lastWriteAt` is stamped on both ends: the + * quiet period that has to elapse is measured from when the last write *settled*, not when it started. + */ +let inFlightWrites = 0; +let lastWriteAt = 0; + +function persist(client: ReturnType, key: string, value: unknown): Promise { + return patchState(client, { [key]: value }); +} + +function patchState(client: ReturnType, patch: Record): Promise { + inFlightWrites += 1; + lastWriteAt = Date.now(); + return client.patch('/dashboards', patch).finally(() => { + inFlightWrites -= 1; + lastWriteAt = Date.now(); + }); +} + +/** + * The same write, for the three callers that cannot use the hook. + * + * Creating, renaming and deleting a dashboard each move several keys at once — a rename PATCHes the new + * layout and nulls three old ones in one body — so they build the patch themselves and update the cache + * by hand. They were the last `.catch(() => {})` in this family: a delete that failed left the dashboard + * gone from the list and alive on the server, reappearing at the next reload with no hint why. + * + * Recovery here is an invalidate rather than the hook's compare-and-swap revert. There is no single + * previous value to swap back — the caller has already rewritten the roster and dropped keys from the + * blob — and refetching is the only thing that makes the list agree with the server again. + */ +export function persistDashboardState( + client: ReturnType, + queryClient: ReturnType, + patch: Record, +): void { + patchState(client, patch).catch((err: unknown) => { + const keys = Object.keys(patch) + .map((k) => `"${k}"`) + .join(', '); + console.error(`[dashboard-state] failed to persist ${keys}`, err); + toast.error('Could not save that change', { + description: `${keys} was not written — reloading from the server. ${err instanceof Error ? err.message : String(err)}`, + }); + void queryClient.invalidateQueries({ queryKey: QUERY_KEY }); + }); +} + /** * Read a stored value, or the caller's default when what is stored cannot be that value. *