From 9acd33dd99999421572bcc2f1c4ad38be13704c0 Mon Sep 17 00:00:00 2001 From: rachit367 Date: Thu, 2 Jul 2026 15:15:19 +0530 Subject: [PATCH 1/3] fix(app): prevent dashboard edits from clobbering each other useUpdateDashboard had no optimistic cache update, so the dashboard read from the ['dashboards'] query cache stayed stale until refetch. Two consecutive setDashboard(produce(dashboard, ...)) calls both derived from the same pre-mutation snapshot, so the second PATCH overwrote the first and the earlier change was lost on reload. Optimistically update the dashboards cache in onMutate (rollback in onError, invalidate in onSettled) so a following produce reads the latest state. Fixes #2216 --- .changeset/fix-dashboard-mutation-race.md | 9 ++ .../__tests__/dashboard.optimistic.test.ts | 94 +++++++++++++++++++ packages/app/src/dashboard.ts | 19 +++- 3 files changed, 121 insertions(+), 1 deletion(-) create mode 100644 .changeset/fix-dashboard-mutation-race.md create mode 100644 packages/app/src/__tests__/dashboard.optimistic.test.ts diff --git a/.changeset/fix-dashboard-mutation-race.md b/.changeset/fix-dashboard-mutation-race.md new file mode 100644 index 0000000000..4d2b6dbb5a --- /dev/null +++ b/.changeset/fix-dashboard-mutation-race.md @@ -0,0 +1,9 @@ +--- +"@hyperdx/app": patch +--- + +Fix remote dashboard changes being silently lost when two edits are saved in +quick succession. `useUpdateDashboard` now optimistically updates the +`dashboards` query cache in `onMutate` (with rollback on error), so a following +`setDashboard` derives its update from the latest state instead of a stale +pre-mutation snapshot whose PATCH would overwrite the earlier change. diff --git a/packages/app/src/__tests__/dashboard.optimistic.test.ts b/packages/app/src/__tests__/dashboard.optimistic.test.ts new file mode 100644 index 0000000000..f14b1d8277 --- /dev/null +++ b/packages/app/src/__tests__/dashboard.optimistic.test.ts @@ -0,0 +1,94 @@ +jest.mock('../config', () => ({ IS_LOCAL_MODE: false })); +jest.mock('../api', () => ({ hdxServer: jest.fn() })); +jest.mock('@mantine/notifications', () => ({ + notifications: { show: jest.fn() }, +})); +jest.mock('nuqs', () => ({ + parseAsJson: jest.fn(), + useQueryState: jest.fn(), +})); + +type MutationConfig = { + onMutate?: (input: any) => any; + onError?: (error: unknown, input: any, context: any) => void; + onSettled?: () => void; +}; + +const mutationConfigs: MutationConfig[] = []; + +const queryClient = { + cancelQueries: jest.fn().mockResolvedValue(undefined), + invalidateQueries: jest.fn(), + getQueryData: jest.fn(), + setQueryData: jest.fn(), +}; + +jest.mock('@tanstack/react-query', () => ({ + useQuery: jest.fn(), + useMutation: jest.fn((cfg: MutationConfig) => { + mutationConfigs.push(cfg); + return { mutate: jest.fn(), mutateAsync: jest.fn() }; + }), + useQueryClient: jest.fn(() => queryClient), +})); +jest.mock('@/utils', () => ({ hashCode: jest.fn(() => 0) })); + +import { useUpdateDashboard } from '@/dashboard'; + +const captureUpdateConfig = ( + hookFactory: () => unknown = useUpdateDashboard, +): MutationConfig => { + hookFactory(); + return mutationConfigs[mutationConfigs.length - 1]; +}; + +beforeEach(() => { + mutationConfigs.length = 0; + queryClient.cancelQueries.mockClear(); + queryClient.invalidateQueries.mockClear(); + queryClient.getQueryData.mockReset(); + queryClient.setQueryData.mockReset(); +}); + +describe('useUpdateDashboard optimistic cache update', () => { + it('writes the pending change into the dashboards cache so a following read is not stale', async () => { + const cfg = captureUpdateConfig(); + const existing = [ + { id: 'a', name: 'A', tiles: [], tags: [], bordered: false }, + { id: 'b', name: 'B', tiles: [], tags: [] }, + ]; + queryClient.getQueryData.mockReturnValue(existing); + + await cfg.onMutate?.({ id: 'a', bordered: true }); + + expect(queryClient.cancelQueries).toHaveBeenCalledWith({ + queryKey: ['dashboards'], + }); + const updater = queryClient.setQueryData.mock.calls[0][1]; + const next = updater(existing); + expect(next.find((d: any) => d.id === 'a').bordered).toBe(true); + expect(next.find((d: any) => d.id === 'b')).toEqual(existing[1]); + }); + + it('rolls the cache back to the previous snapshot on error', async () => { + const cfg = captureUpdateConfig(); + const previousDashboards = [{ id: 'a', name: 'A', tiles: [], tags: [] }]; + queryClient.getQueryData.mockReturnValue(previousDashboards); + + const context = await cfg.onMutate?.({ id: 'a', name: 'changed' }); + cfg.onError?.(new Error('boom'), { id: 'a' }, context); + + expect(queryClient.setQueryData).toHaveBeenLastCalledWith( + ['dashboards'], + previousDashboards, + ); + }); + + it('invalidates dashboards after settling', () => { + const cfg = captureUpdateConfig(); + cfg.onSettled?.(); + expect(queryClient.invalidateQueries).toHaveBeenCalledWith({ + queryKey: ['dashboards'], + }); + }); +}); diff --git a/packages/app/src/dashboard.ts b/packages/app/src/dashboard.ts index 8e52d3813a..dd41662126 100644 --- a/packages/app/src/dashboard.ts +++ b/packages/app/src/dashboard.ts @@ -138,7 +138,24 @@ export function useUpdateDashboard() { json: normalized, }); }, - onSuccess: () => { + onMutate: async ( + dashboard: Partial & { id: Dashboard['id'] }, + ) => { + await queryClient.cancelQueries({ queryKey: ['dashboards'] }); + const previousDashboards = queryClient.getQueryData([ + 'dashboards', + ]); + queryClient.setQueryData(['dashboards'], current => + current?.map(d => (d.id === dashboard.id ? { ...d, ...dashboard } : d)), + ); + return { previousDashboards }; + }, + onError: (_error, _dashboard, context) => { + if (context?.previousDashboards) { + queryClient.setQueryData(['dashboards'], context.previousDashboards); + } + }, + onSettled: () => { queryClient.invalidateQueries({ queryKey: ['dashboards'] }); }, }); From 1136f3431fe39059ad9835a6ca30110a3bbf7604 Mon Sep 17 00:00:00 2001 From: rachit367 Date: Thu, 2 Jul 2026 20:54:04 +0530 Subject: [PATCH 2/3] test(app): strengthen dashboard optimistic-update coverage Address review feedback on the #2216 fix: - back the mock query client with an in-memory cache and add a test that drives two sequential onMutate calls, proving both edits survive instead of the second clobbering the first - assert setQueryData targets the ['dashboards'] key - cover the empty-cache path so onMutate no longer writes undefined (guarded in the implementation) --- .../__tests__/dashboard.optimistic.test.ts | 74 ++++++++++++++----- packages/app/src/dashboard.ts | 6 +- 2 files changed, 60 insertions(+), 20 deletions(-) diff --git a/packages/app/src/__tests__/dashboard.optimistic.test.ts b/packages/app/src/__tests__/dashboard.optimistic.test.ts index f14b1d8277..a4c3a813ce 100644 --- a/packages/app/src/__tests__/dashboard.optimistic.test.ts +++ b/packages/app/src/__tests__/dashboard.optimistic.test.ts @@ -16,11 +16,25 @@ type MutationConfig = { const mutationConfigs: MutationConfig[] = []; +// A minimal in-memory query cache so getQueryData/setQueryData actually +// share state, letting a test prove two sequential onMutate calls compose +// instead of clobbering each other. +const store = new Map(); +const keyOf = (key: unknown) => JSON.stringify(key); + const queryClient = { cancelQueries: jest.fn().mockResolvedValue(undefined), invalidateQueries: jest.fn(), - getQueryData: jest.fn(), - setQueryData: jest.fn(), + getQueryData: jest.fn((key: unknown) => store.get(keyOf(key))), + setQueryData: jest.fn((key: unknown, updater: unknown) => { + const current = store.get(keyOf(key)); + const next = + typeof updater === 'function' + ? (updater as (c: unknown) => unknown)(current) + : updater; + store.set(keyOf(key), next); + return next; + }), }; jest.mock('@tanstack/react-query', () => ({ @@ -42,46 +56,68 @@ const captureUpdateConfig = ( return mutationConfigs[mutationConfigs.length - 1]; }; +const seedCache = (dashboards: unknown[]) => + store.set(keyOf(['dashboards']), dashboards); + beforeEach(() => { mutationConfigs.length = 0; + store.clear(); queryClient.cancelQueries.mockClear(); queryClient.invalidateQueries.mockClear(); - queryClient.getQueryData.mockReset(); - queryClient.setQueryData.mockReset(); + queryClient.getQueryData.mockClear(); + queryClient.setQueryData.mockClear(); }); +const cached = () => store.get(keyOf(['dashboards'])) as any[]; + describe('useUpdateDashboard optimistic cache update', () => { - it('writes the pending change into the dashboards cache so a following read is not stale', async () => { + it('writes the pending change into the dashboards cache under the dashboards key', async () => { const cfg = captureUpdateConfig(); - const existing = [ + seedCache([ { id: 'a', name: 'A', tiles: [], tags: [], bordered: false }, { id: 'b', name: 'B', tiles: [], tags: [] }, - ]; - queryClient.getQueryData.mockReturnValue(existing); + ]); await cfg.onMutate?.({ id: 'a', bordered: true }); expect(queryClient.cancelQueries).toHaveBeenCalledWith({ queryKey: ['dashboards'], }); - const updater = queryClient.setQueryData.mock.calls[0][1]; - const next = updater(existing); - expect(next.find((d: any) => d.id === 'a').bordered).toBe(true); - expect(next.find((d: any) => d.id === 'b')).toEqual(existing[1]); + expect(queryClient.setQueryData.mock.calls[0][0]).toEqual(['dashboards']); + expect(cached().find(d => d.id === 'a').bordered).toBe(true); + expect(cached().find(d => d.id === 'b').name).toBe('B'); + }); + + it('composes two sequential edits so neither clobbers the other (#2216)', async () => { + const cfg = captureUpdateConfig(); + seedCache([{ id: 'a', name: 'A', tiles: [], tags: [], bordered: false }]); + + // Edit 1: toggle bordered. Edit 2 reads the already-updated cache. + await cfg.onMutate?.({ id: 'a', bordered: true }); + await cfg.onMutate?.({ id: 'a', name: 'Renamed' }); + + const dashboard = cached().find(d => d.id === 'a'); + expect(dashboard.bordered).toBe(true); + expect(dashboard.name).toBe('Renamed'); + }); + + it('leaves an empty cache untouched instead of writing undefined', async () => { + const cfg = captureUpdateConfig(); + // No seedCache: the query has not resolved yet. + await cfg.onMutate?.({ id: 'a', bordered: true }); + expect(store.get(keyOf(['dashboards']))).toBeUndefined(); }); it('rolls the cache back to the previous snapshot on error', async () => { const cfg = captureUpdateConfig(); - const previousDashboards = [{ id: 'a', name: 'A', tiles: [], tags: [] }]; - queryClient.getQueryData.mockReturnValue(previousDashboards); + const previous = [{ id: 'a', name: 'A', tiles: [], tags: [] }]; + seedCache(previous); const context = await cfg.onMutate?.({ id: 'a', name: 'changed' }); - cfg.onError?.(new Error('boom'), { id: 'a' }, context); + expect(cached()[0].name).toBe('changed'); - expect(queryClient.setQueryData).toHaveBeenLastCalledWith( - ['dashboards'], - previousDashboards, - ); + cfg.onError?.(new Error('boom'), { id: 'a' }, context); + expect(cached()).toEqual(previous); }); it('invalidates dashboards after settling', () => { diff --git a/packages/app/src/dashboard.ts b/packages/app/src/dashboard.ts index dd41662126..0a480d0aa0 100644 --- a/packages/app/src/dashboard.ts +++ b/packages/app/src/dashboard.ts @@ -146,7 +146,11 @@ export function useUpdateDashboard() { 'dashboards', ]); queryClient.setQueryData(['dashboards'], current => - current?.map(d => (d.id === dashboard.id ? { ...d, ...dashboard } : d)), + current + ? current.map(d => + d.id === dashboard.id ? { ...d, ...dashboard } : d, + ) + : current, ); return { previousDashboards }; }, From 3bc9c360e5092b5f94b78d40bdfa84eb72c7d3fb Mon Sep 17 00:00:00 2001 From: rachit367 Date: Thu, 2 Jul 2026 21:03:23 +0530 Subject: [PATCH 3/3] refactor(app): type the onError rollback setQueryData for consistency Match the typed setQueryData used in onMutate, per review. --- packages/app/src/dashboard.ts | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/packages/app/src/dashboard.ts b/packages/app/src/dashboard.ts index 0a480d0aa0..624da96091 100644 --- a/packages/app/src/dashboard.ts +++ b/packages/app/src/dashboard.ts @@ -156,7 +156,10 @@ export function useUpdateDashboard() { }, onError: (_error, _dashboard, context) => { if (context?.previousDashboards) { - queryClient.setQueryData(['dashboards'], context.previousDashboards); + queryClient.setQueryData( + ['dashboards'], + context.previousDashboards, + ); } }, onSettled: () => {