diff --git a/frontend/src/__tests__/components/GitHubRestoreModal.test.tsx b/frontend/src/__tests__/components/GitHubRestoreModal.test.tsx index 1602b734c..77fa0e924 100644 --- a/frontend/src/__tests__/components/GitHubRestoreModal.test.tsx +++ b/frontend/src/__tests__/components/GitHubRestoreModal.test.tsx @@ -6,6 +6,7 @@ import { describe, it, expect, vi, beforeEach } from 'vitest'; import { screen, waitFor } from '@testing-library/react'; import userEvent from '@testing-library/user-event'; import { http, HttpResponse } from 'msw'; +import { QueryClient, useQuery, useQueryClient } from '@tanstack/react-query'; import { render } from '../utils'; import { server } from '../mocks/server'; import { GitHubRestoreModal } from '../../components/GitHubRestoreModal'; @@ -364,4 +365,202 @@ describe('GitHubRestoreModal', () => { expect(onClose).toHaveBeenCalled(); }); + + // Regression guard for the settings clobber found in manual testing (#2656). + // SettingsPage — which renders this modal — holds a `localSettings` copy of the + // form state and a debounced effect that PATCHes it back whenever the server + // copy differs. Refetching ['settings'] here therefore made the page overwrite + // the restore with the pre-restore values, silently, ~500 ms later. 75 of the + // ~80 keys in a real backup sit in that save payload, so a restore reported as + // "77 restored, 0 failed" left almost nothing behind. + describe('settings restore must not be undone by SettingsPage', () => { + /** Runs a restore returning `results`, leaving the modal on its summary. */ + async function restoreWith(results: Record, onClose = vi.fn()) { + server.use( + http.post('/api/v1/github-backup/restore', () => + HttpResponse.json({ + success: true, + message: 'Restored 77 item(s) from aaa1111', + log_id: 7, + ref: mockPreview.ref, + results, + }) + ) + ); + render(); + + const checkboxes = await waitFor(() => screen.getAllByRole('checkbox') as HTMLInputElement[]); + await userEvent.click(checkboxes[1]); + await userEvent.click(screen.getByRole('button', { name: /Restore$/ })); + await waitFor(() => screen.getByText('Restore from backup?')); + const confirmButtons = screen.getAllByRole('button', { name: /Restore$/ }); + await userEvent.click(confirmButtons[confirmButtons.length - 1]); + await waitFor(() => screen.getByText('Restored 77 item(s) from aaa1111')); + return onClose; + } + + /** Replaces window.location with a reload spy for the duration of a test. */ + function stubReload() { + const original = window.location; + const reload = vi.fn(); + Object.defineProperty(window, 'location', { + configurable: true, + value: { ...original, reload }, + }); + return { + reload, + restore: () => + Object.defineProperty(window, 'location', { configurable: true, value: original }), + }; + } + + /** + * Stands in for SettingsPage: hands the test the provider's QueryClient and + * keeps an observer on ['settings'] for as long as the modal is mounted, so + * the entry survives the test client's `gcTime: 0`. Disabled, because these + * tests write the cache directly rather than fetching it. + */ + function makeSettingsProbe(capture: (client: QueryClient) => void) { + return function SettingsProbe() { + capture(useQueryClient()); + useQuery({ queryKey: ['settings'], queryFn: async () => null, enabled: false }); + return null; + }; + } + + it('never invalidates the settings query', async () => { + const invalidate = vi.spyOn(QueryClient.prototype, 'invalidateQueries'); + await restoreWith({ settings: { restored: 77, skipped: 3, failed: 0, notes: [] } }); + + const keys = invalidate.mock.calls.map((c) => JSON.stringify(c[0]?.queryKey)); + // The caches this restore genuinely rewrites are still refreshed... + expect(keys).toContain(JSON.stringify(['spools'])); + // ...but ['settings'] must not be, or the page writes the old values back. + expect(keys).not.toContain(JSON.stringify(['settings'])); + invalidate.mockRestore(); + }); + + it('reloads instead of merely closing after a settings restore', async () => { + const loc = stubReload(); + try { + const onClose = await restoreWith({ + settings: { restored: 77, skipped: 3, failed: 0, notes: [] }, + }); + + const closeButtons = screen.getAllByRole('button', { name: 'Close' }); + await userEvent.click(closeButtons[closeButtons.length - 1]); + + expect(loc.reload).toHaveBeenCalled(); + // Closing in place would leave SettingsPage mounted and armed. + expect(onClose).not.toHaveBeenCalled(); + } finally { + loc.restore(); + } + }); + + // Not invalidating ['settings'] only closes the refetch *this* modal caused. + // The query still refetches on window focus and on reconnect — both default + // to true, and ['settings'] has other always-mounted observers — and a + // refetch landing while the result panel is up puts the restored values in + // the cache next to SettingsPage's pre-restore `localSettings`, which is + // precisely the state its debounced effect writes back. So the cache is + // pinned to the copy the page already agrees with until the exit reload. + it('pins the settings cache against a background refetch landing under the result panel', async () => { + const preRestore = { currency: 'EUR' }; + let client: QueryClient | null = null; + const Probe = makeSettingsProbe((c) => { + client = c; + }); + + server.use( + http.post('/api/v1/github-backup/restore', () => + HttpResponse.json({ + success: true, + message: 'Restored 77 item(s) from aaa1111', + log_id: 7, + ref: mockPreview.ref, + results: { settings: { restored: 77, skipped: 3, failed: 0, notes: [] } }, + }) + ) + ); + render( + <> + + + + ); + // The copy SettingsPage's form state was built from. + client!.setQueryData(['settings'], preRestore); + + const checkboxes = await waitFor(() => screen.getAllByRole('checkbox') as HTMLInputElement[]); + await userEvent.click(checkboxes[2]); + await userEvent.click(screen.getByRole('button', { name: /Restore$/ })); + await waitFor(() => screen.getByText('Restore from backup?')); + const confirmButtons = screen.getAllByRole('button', { name: /Restore$/ }); + await userEvent.click(confirmButtons[confirmButtons.length - 1]); + await waitFor(() => screen.getByText('Restored 77 item(s) from aaa1111')); + + // A focus/reconnect refetch lands the restored server values. + client!.setQueryData(['settings'], { currency: 'USD' }); + + // Pinned back, so SettingsPage sees no divergence and saves nothing. + expect(client!.getQueryData(['settings'])).toEqual(preRestore); + }); + + it('leaves the settings cache alone when settings were not restored', async () => { + let client: QueryClient | null = null; + const Probe = makeSettingsProbe((c) => { + client = c; + }); + server.use( + http.post('/api/v1/github-backup/restore', () => + HttpResponse.json({ + success: true, + message: 'Restored 4 item(s) from aaa1111', + log_id: 8, + ref: mockPreview.ref, + results: { spools: { restored: 4, skipped: 0, failed: 0, notes: [] } }, + }) + ) + ); + render( + <> + + + + ); + client!.setQueryData(['settings'], { currency: 'EUR' }); + + const checkboxes = await waitFor(() => screen.getAllByRole('checkbox') as HTMLInputElement[]); + await userEvent.click(checkboxes[1]); + await userEvent.click(screen.getByRole('button', { name: /Restore$/ })); + await waitFor(() => screen.getByText('Restore from backup?')); + const confirmButtons = screen.getAllByRole('button', { name: /Restore$/ }); + await userEvent.click(confirmButtons[confirmButtons.length - 1]); + await waitFor(() => screen.getByText('Restored 4 item(s) from aaa1111')); + + const fresh = { currency: 'USD' }; + client!.setQueryData(['settings'], fresh); + // No settings were touched, so there is nothing to protect and normal + // refetching must keep working. + expect(client!.getQueryData(['settings'])).toEqual(fresh); + }); + + it('closes normally when settings were not part of the restore', async () => { + const loc = stubReload(); + try { + const onClose = await restoreWith({ + spools: { restored: 4, skipped: 0, failed: 0, notes: [] }, + }); + + const closeButtons = screen.getAllByRole('button', { name: 'Close' }); + await userEvent.click(closeButtons[closeButtons.length - 1]); + + expect(onClose).toHaveBeenCalled(); + expect(loc.reload).not.toHaveBeenCalled(); + } finally { + loc.restore(); + } + }); + }); }); diff --git a/frontend/src/components/GitHubRestoreModal.tsx b/frontend/src/components/GitHubRestoreModal.tsx index 9fd302c4f..ef8079d95 100644 --- a/frontend/src/components/GitHubRestoreModal.tsx +++ b/frontend/src/components/GitHubRestoreModal.tsx @@ -1,4 +1,4 @@ -import { useEffect, useMemo, useState } from 'react'; +import { useCallback, useEffect, useMemo, useRef, useState } from 'react'; import { useMutation, useQuery, useQueryClient } from '@tanstack/react-query'; import { useTranslation } from 'react-i18next'; import { @@ -52,6 +52,10 @@ export function GitHubRestoreModal({ onClose }: GitHubRestoreModalProps) { const [overwriteExisting, setOverwriteExisting] = useState(false); const [showConfirm, setShowConfirm] = useState(false); const [result, setResult] = useState(null); + // The ['settings'] cache entry as it stood when the restore returned — i.e. + // the copy SettingsPage's form state was built from. Used to pin the cache + // while a settings restore result is on screen; see the effect below. + const pinnedSettings = useRef(undefined); const commitsQuery = useQuery({ queryKey: ['github-backup-commits'], @@ -87,30 +91,80 @@ export function GitHubRestoreModal({ onClose }: GitHubRestoreModalProps) { // above a message saying nothing had been restored. Only a real success // gets the panel; a failure keeps the form and shows the red block below. if (data.success) { + pinnedSettings.current = queryClient.getQueryData(['settings']); setResult(data); // A restore rewrites rows these caches hold. queryClient.invalidateQueries({ queryKey: ['spools'] }); queryClient.invalidateQueries({ queryKey: ['archives'] }); - queryClient.invalidateQueries({ queryKey: ['settings'] }); } // A failure that got as far as resolving the commit still writes a log row // (status "failed"), so refresh the history and status either way. queryClient.invalidateQueries({ queryKey: ['github-backup-logs'] }); queryClient.invalidateQueries({ queryKey: ['github-backup-status'] }); + // ['settings'] is deliberately NOT invalidated. SettingsPage — which + // renders this modal — keeps a `localSettings` copy of the form state and + // a debounced effect that PATCHes it back whenever the server copy + // differs. Refetching here makes that effect write the pre-restore values + // over everything we just restored, 500 ms later and silently: 75 of the + // ~80 keys in a backup are in its save payload. A reload is the only safe + // resync, so `closeModal` below forces one instead. }, onError: () => setShowConfirm(false), }); const isRestoring = restoreMutation.isPending; + // Leaving this modal mounted after a settings restore lets SettingsPage + // overwrite what was restored (see the onSuccess comment), so every exit path + // reloads rather than just closing. + const settingsRestored = Boolean(result && 'settings' in result.results); + const closeModal = useCallback(() => { + if (settingsRestored) { + window.location.reload(); + return; + } + onClose(); + }, [settingsRestored, onClose]); + + // Not invalidating ['settings'] above keeps the page from reverting a restore + // *we* triggered a refetch for, but it is not enough on its own: the query + // still refetches on window focus and on reconnect (both default to true, and + // ['settings'] has other always-mounted observers that trigger it), and any + // such refetch lands the restored values in the cache while SettingsPage's + // `localSettings` still holds the pre-restore ones — which is exactly the + // state its debounced effect PATCHes back. Tabbing away from the result panel + // for a minute and back was enough to lose the restore. + // + // The refetch cannot be prevented from here, so pin the cache instead: while + // the result panel is up, any fresh ['settings'] payload is written straight + // back to the copy the page already agrees with, so `hasChanges` stays false + // and nothing is saved over the restore. Every exit path reloads (above), and + // that reload is what resyncs the page for real. + useEffect(() => { + if (!settingsRestored) return; + const pinned = pinnedSettings.current; + if (pinned === undefined) return; + // Compared by value, not identity: react-query's structural sharing stores a + // copy rather than the object handed to setQueryData, so an identity check + // would never match its own write and would recurse until the stack blew. + const pinnedJson = JSON.stringify(pinned); + return queryClient.getQueryCache().subscribe((event) => { + if (event.type !== 'updated' || event.action.type !== 'success') return; + const key = event.query.queryKey; + if (!Array.isArray(key) || key.length !== 1 || key[0] !== 'settings') return; + if (JSON.stringify(event.query.state.data) === pinnedJson) return; + queryClient.setQueryData(['settings'], pinned); + }); + }, [settingsRestored, queryClient]); + // Close on Escape, except while a restore is in flight. useEffect(() => { const handleKeyDown = (e: KeyboardEvent) => { - if (e.key === 'Escape' && !isRestoring && !showConfirm) onClose(); + if (e.key === 'Escape' && !isRestoring && !showConfirm) closeModal(); }; window.addEventListener('keydown', handleKeyDown); return () => window.removeEventListener('keydown', handleKeyDown); - }, [onClose, isRestoring, showConfirm]); + }, [closeModal, isRestoring, showConfirm]); // Interrupting a restore mid-flight can leave a partly-applied category. useEffect(() => { @@ -174,7 +228,7 @@ export function GitHubRestoreModal({ onClose }: GitHubRestoreModalProps) { <>
e.stopPropagation()}> @@ -190,7 +244,7 @@ export function GitHubRestoreModal({ onClose }: GitHubRestoreModalProps) {