fix(backup): don't let SettingsPage revert a restored settings category (#2656)

Restoring App Settings from the Backup tab silently reverted almost everything
it reported restoring.

GitHubRestoreModal invalidated ['settings'] on success. SettingsPage — which
renders the modal — keeps a `localSettings` copy of its form state alongside a
debounced effect that PATCHes it back whenever the server copy differs. The
refetch made the two differ, the effect cannot tell "server changed" from "user
edited", and 500 ms later it wrote the pre-restore values back over the restore.
75 of the ~80 keys in a backup sit in that save payload, so a restore reporting
"77 restored, 0 failed" left only the five auth/internal flags behind. The
backend was correct throughout: the same restore driven against the API with no
browser open applies cleanly.

Since the Restore button lives on the Settings page, this was the default path
rather than an edge case.

Fixed inside the modal rather than in SettingsPage: that debounce's own comments
show it was tuned to avoid resetting text fields mid-typing, and widening this
change into it risks that. So ['settings'] is no longer invalidated, and every
exit path (footer Close, header X, overlay click, Escape) now reloads instead of
closing when settings were among the restored categories, since leaving the page
mounted is what arms the overwrite. The ['spools'] and ['archives']
invalidations are unchanged — SettingsPage is the only page carrying this kind
of whole-payload auto-save.

The root cause is left for a follow-up: any future feature that writes settings
server-side will be reverted the same way.

Found by manual end-to-end testing against a private test repo, which also
confirmed the natural-key id remapping and the overwrite-off behaviour working
as designed.

Tests: 3 new frontend tests — the settings query is never invalidated, a
settings restore reloads rather than closing, and a non-settings restore still
closes normally. The first two fail against the pre-fix component. Full frontend
suite green (186 files / 2459 tests), i18n parity unchanged, eslint clean.

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<string, unknown>, 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(<GitHubRestoreModal onClose={onClose} />);
+
+      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(
+        <>
+          <Probe />
+          <GitHubRestoreModal onClose={vi.fn()} />
+        </>
+      );
+      // 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(
+        <>
+          <Probe />
+          <GitHubRestoreModal onClose={vi.fn()} />
+        </>
+      );
+      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();
+      }
+    });
+  });
 });

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<GitHubRestoreResponse | null>(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<unknown>(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) {
     <>
       <div
         className="fixed inset-0 bg-black/50 flex items-center justify-center z-50 p-4"
-        onClick={isRestoring ? undefined : onClose}
+        onClick={isRestoring ? undefined : closeModal}
       >
         <Card className="w-full max-w-lg" onClick={(e: React.MouseEvent) => e.stopPropagation()}>
           <CardContent className="p-0">
@@ -190,7 +244,7 @@ export function GitHubRestoreModal({ onClose }: GitHubRestoreModalProps) {
                 </div>
               </div>
               <button
-                onClick={onClose}
+                onClick={closeModal}
                 disabled={isRestoring}
                 aria-label={t('common.close')}
                 className="p-2 hover:bg-bambu-dark-tertiary rounded-lg transition-colors disabled:opacity-50"
@@ -364,7 +418,7 @@ export function GitHubRestoreModal({ onClose }: GitHubRestoreModalProps) {
                 <>
                   <span />
                   <div className="flex gap-3">
-                    <Button variant="secondary" onClick={onClose}>
+                    <Button variant="secondary" onClick={closeModal}>
                       {t('common.close')}
                     </Button>
                     <Button
@@ -381,7 +435,7 @@ export function GitHubRestoreModal({ onClose }: GitHubRestoreModalProps) {
                     {t('backup.restoreFromGit.selectedCount', { count: selectedCount })}
                   </span>
                   <div className="flex gap-3">
-                    <Button variant="secondary" onClick={onClose} disabled={isRestoring}>
+                    <Button variant="secondary" onClick={closeModal} disabled={isRestoring}>
                       {t('common.cancel')}
                     </Button>
                     <Button