From 98cd74935a67881823c2efa822c5e588956a8707 Mon Sep 17 00:00:00 2001 From: "[._.]/ Adam Eivy" Date: Sun, 30 Aug 2026 21:07:41 +0000 Subject: [PATCH 1/5] fix: guard unsaved General settings (#5480) --- client/src/components/settings/GeneralTab.jsx | 44 ++++- .../components/settings/GeneralTab.test.jsx | 174 ++++++++++++++++++ 2 files changed, 214 insertions(+), 4 deletions(-) create mode 100644 client/src/components/settings/GeneralTab.test.jsx diff --git a/client/src/components/settings/GeneralTab.jsx b/client/src/components/settings/GeneralTab.jsx index f2a480f564..362aec0455 100644 --- a/client/src/components/settings/GeneralTab.jsx +++ b/client/src/components/settings/GeneralTab.jsx @@ -2,8 +2,10 @@ import { useState, useEffect, useMemo } from 'react'; import { Save } from 'lucide-react'; import toast from '../ui/Toast'; import FormField from '../ui/FormField'; +import UnsavedChangesConfirm from '../ui/UnsavedChangesConfirm'; import BrailleSpinner from '../BrailleSpinner'; import ThemePickerPanel from '../ThemePickerPanel'; +import useUnsavedChangesGuard from '../../hooks/useUnsavedChangesGuard'; import { getSettings, updateSettings } from '../../services/api'; // Coordinate inputs are free text so a partially-typed "-" or "37." isn't @@ -20,25 +22,39 @@ const parseCoord = (v) => { export function GeneralTab() { const [loading, setLoading] = useState(true); const [timezone, setTimezone] = useState(''); + const [savedTimezone, setSavedTimezone] = useState(null); const [saving, setSaving] = useState(false); const [lat, setLat] = useState(''); const [lon, setLon] = useState(''); + const [savedLocation, setSavedLocation] = useState(null); const [savingLocation, setSavingLocation] = useState(false); const detectedTz = Intl.DateTimeFormat().resolvedOptions().timeZone; const allTimezones = useMemo(() => Intl.supportedValuesOf?.('timeZone') ?? [], []); + const timezoneDirty = savedTimezone !== null && timezone !== savedTimezone; + const locationDirty = savedLocation !== null + && (lat !== savedLocation.lat || lon !== savedLocation.lon); + const dirty = timezoneDirty || locationDirty; + const routeGuard = useUnsavedChangesGuard(dirty); useEffect(() => { getSettings({ silent: true }) .then(settings => { - setTimezone(settings?.timezone || ''); - setLat(settings?.location?.lat != null ? String(settings.location.lat) : ''); - setLon(settings?.location?.lon != null ? String(settings.location.lon) : ''); + const nextTimezone = settings?.timezone || ''; + const nextLat = settings?.location?.lat != null ? String(settings.location.lat) : ''; + const nextLon = settings?.location?.lon != null ? String(settings.location.lon) : ''; + setTimezone(nextTimezone); + setSavedTimezone(nextTimezone); + setLat(nextLat); + setLon(nextLon); + setSavedLocation({ lat: nextLat, lon: nextLon }); }) .catch(() => toast.error('Failed to load settings')) .finally(() => setLoading(false)); }, []); const handleSaveLocation = async () => { + const submittedLat = lat; + const submittedLon = lon; // Both-or-neither: weather needs a full pair, and a half-set pair would // silently mix a custom value with the tool's default coordinate. if (isBlank(lat) !== isBlank(lon)) { @@ -62,6 +78,11 @@ export function GeneralTab() { setSavingLocation(true); try { await updateSettings({ location: { lat: parsedLat, lon: parsedLon } }, { silent: true }); + const nextLat = parsedLat === null ? '' : String(parsedLat); + const nextLon = parsedLon === null ? '' : String(parsedLon); + setSavedLocation({ lat: nextLat, lon: nextLon }); + setLat(current => current === submittedLat ? nextLat : current); + setLon(current => current === submittedLon ? nextLon : current); toast.success(parsedLat === null ? 'Location cleared' : `Location set to ${parsedLat}, ${parsedLon}`); } catch (err) { toast.error(err.message || 'Failed to save location'); @@ -71,6 +92,7 @@ export function GeneralTab() { }; const handleSave = async (tz) => { + const submittedTimezone = timezone; const tzToSave = tz || detectedTz; if (!tzToSave) { toast.error('Timezone is required.'); @@ -95,7 +117,8 @@ export function GeneralTab() { setSaving(true); try { await updateSettings({ timezone: tzToSave }, { silent: true }); - setTimezone(tzToSave); + setSavedTimezone(tzToSave); + setTimezone(current => current === submittedTimezone ? tzToSave : current); toast.success(`Timezone set to ${tzToSave}`); } catch (err) { toast.error(err.message || 'Failed to save timezone'); @@ -108,6 +131,13 @@ export function GeneralTab() { return (
+

Interface Theme

@@ -140,6 +170,9 @@ export function GeneralTab() { {saving ? 'Saving...' : 'Save'} + {timezoneDirty && ( + Unsaved changes + )} {!timezone && ( + {locationDirty && ( + Unsaved changes + )}
diff --git a/client/src/components/settings/GeneralTab.test.jsx b/client/src/components/settings/GeneralTab.test.jsx new file mode 100644 index 0000000000..cc67a58cd4 --- /dev/null +++ b/client/src/components/settings/GeneralTab.test.jsx @@ -0,0 +1,174 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest'; +import { act, fireEvent, render, screen, waitFor, within } from '@testing-library/react'; +import { createMemoryRouter, RouterProvider } from 'react-router'; + +vi.mock('../../services/api', () => ({ + getSettings: vi.fn(), + updateSettings: vi.fn(), +})); +vi.mock('../ui/Toast', () => ({ + default: Object.assign(vi.fn(), { + success: vi.fn(), + error: vi.fn(), + }), +})); +vi.mock('../ThemePickerPanel', () => ({ + default: () =>
Theme picker
, +})); + +import { getSettings, updateSettings } from '../../services/api'; +import { GeneralTab } from './GeneralTab'; + +const SETTINGS = { + timezone: 'UTC', + location: { lat: 37.7749, lon: -122.4194 }, +}; +const CONFIRM = 'Discard your unsaved General settings changes?'; + +const timezoneCard = () => screen.getByRole('heading', { name: 'Timezone' }).parentElement; +const locationCard = () => screen.getByRole('heading', { name: 'Location' }).parentElement; + +const renderTab = async () => { + const router = createMemoryRouter([ + { path: '/settings/general', element: }, + { path: '/settings/security', element:
Security settings
}, + ], { initialEntries: ['/settings/general'] }); + render(); + await screen.findByDisplayValue('UTC'); + return router; +}; + +const navigate = (router, to) => act(async () => { await router.navigate(to); }); + +beforeEach(() => { + vi.clearAllMocks(); + getSettings.mockResolvedValue(SETTINGS); + updateSettings.mockResolvedValue({}); +}); + +describe('GeneralTab unsaved changes', () => { + it('marks each edited section dirty, arms beforeunload, and clears when values are restored', async () => { + const add = vi.spyOn(window, 'addEventListener'); + const remove = vi.spyOn(window, 'removeEventListener'); + await renderTab(); + + fireEvent.change(screen.getByLabelText('Timezone (IANA)'), { + target: { value: 'America/New_York' }, + }); + expect(within(timezoneCard()).getByText('Unsaved changes')).toBeInTheDocument(); + expect(within(locationCard()).queryByText('Unsaved changes')).toBeNull(); + + fireEvent.change(screen.getByLabelText('Latitude (-90 to 90)'), { + target: { value: '40.7128' }, + }); + expect(within(locationCard()).getByText('Unsaved changes')).toBeInTheDocument(); + await waitFor(() => { + expect(add.mock.calls.some(([type]) => type === 'beforeunload')).toBe(true); + }); + + fireEvent.change(screen.getByLabelText('Timezone (IANA)'), { + target: { value: SETTINGS.timezone }, + }); + fireEvent.change(screen.getByLabelText('Latitude (-90 to 90)'), { + target: { value: String(SETTINGS.location.lat) }, + }); + expect(screen.queryByText('Unsaved changes')).toBeNull(); + await waitFor(() => { + expect(remove.mock.calls.some(([type]) => type === 'beforeunload')).toBe(true); + }); + }); + + it('keeps the current route and draft when navigation is canceled', async () => { + const router = await renderTab(); + fireEvent.change(screen.getByLabelText('Timezone (IANA)'), { + target: { value: 'America/New_York' }, + }); + + await navigate(router, '/settings/security'); + expect(screen.getByText(CONFIRM)).toBeInTheDocument(); + fireEvent.click(screen.getByRole('button', { name: 'Keep editing' })); + + await waitFor(() => expect(screen.queryByText(CONFIRM)).toBeNull()); + expect(router.state.location.pathname).toBe('/settings/general'); + expect(screen.getByLabelText('Timezone (IANA)')).toHaveValue('America/New_York'); + }); + + it('discards the draft and runs the parked Settings navigation', async () => { + const router = await renderTab(); + fireEvent.change(screen.getByLabelText('Longitude (-180 to 180)'), { + target: { value: '-74.006' }, + }); + + await navigate(router, '/settings/security'); + expect(screen.getByText(CONFIRM)).toBeInTheDocument(); + fireEvent.click(screen.getByRole('button', { name: 'Discard' })); + + expect(await screen.findByText('Security settings')).toBeInTheDocument(); + expect(router.state.location.pathname).toBe('/settings/security'); + }); + + it('advances only the successful section baseline', async () => { + const router = await renderTab(); + fireEvent.change(screen.getByLabelText('Timezone (IANA)'), { + target: { value: 'America/New_York' }, + }); + fireEvent.change(screen.getByLabelText('Latitude (-90 to 90)'), { + target: { value: '40.7128' }, + }); + + await act(async () => { + fireEvent.click(within(timezoneCard()).getByRole('button', { name: 'Save' })); + }); + expect(updateSettings).toHaveBeenCalledWith( + { timezone: 'America/New_York' }, + { silent: true }, + ); + expect(within(timezoneCard()).queryByText('Unsaved changes')).toBeNull(); + expect(within(locationCard()).getByText('Unsaved changes')).toBeInTheDocument(); + + await navigate(router, '/settings/security'); + expect(screen.getByText(CONFIRM)).toBeInTheDocument(); + fireEvent.click(screen.getByRole('button', { name: 'Keep editing' })); + + await act(async () => { + fireEvent.click(within(locationCard()).getByRole('button', { name: 'Save' })); + }); + expect(updateSettings).toHaveBeenLastCalledWith( + { location: { lat: 40.7128, lon: SETTINGS.location.lon } }, + { silent: true }, + ); + expect(screen.queryByText('Unsaved changes')).toBeNull(); + }); + + it('keeps a failed timezone save dirty and guarded', async () => { + updateSettings.mockRejectedValueOnce(new Error('timezone offline')); + const router = await renderTab(); + fireEvent.change(screen.getByLabelText('Timezone (IANA)'), { + target: { value: 'America/New_York' }, + }); + + await act(async () => { + fireEvent.click(within(timezoneCard()).getByRole('button', { name: 'Save' })); + }); + expect(within(timezoneCard()).getByText('Unsaved changes')).toBeInTheDocument(); + + await navigate(router, '/settings/security'); + expect(screen.getByText(CONFIRM)).toBeInTheDocument(); + }); + + it('keeps a failed location save dirty and guarded', async () => { + updateSettings.mockRejectedValueOnce(new Error('location offline')); + const router = await renderTab(); + fireEvent.change(screen.getByLabelText('Longitude (-180 to 180)'), { + target: { value: '-74.006' }, + }); + + await act(async () => { + fireEvent.click(within(locationCard()).getByRole('button', { name: 'Save' })); + }); + expect(within(locationCard()).getByText('Unsaved changes')).toBeInTheDocument(); + + await navigate(router, '/settings/security'); + expect(screen.getByText(CONFIRM)).toBeInTheDocument(); + }); +}); From 36bb17aaf9108532e3a966d00dc44898115aa10a Mon Sep 17 00:00:00 2001 From: "[._.]/ Adam Eivy" Date: Sun, 30 Aug 2026 21:16:04 +0000 Subject: [PATCH 2/5] fix: close unsaved settings guard races (#5480) --- client/src/components/settings/GeneralTab.jsx | 22 ++++++-- .../components/settings/GeneralTab.test.jsx | 52 ++++++++++++++++++- 2 files changed, 67 insertions(+), 7 deletions(-) diff --git a/client/src/components/settings/GeneralTab.jsx b/client/src/components/settings/GeneralTab.jsx index 362aec0455..753b5f07b1 100644 --- a/client/src/components/settings/GeneralTab.jsx +++ b/client/src/components/settings/GeneralTab.jsx @@ -34,6 +34,8 @@ export function GeneralTab() { const locationDirty = savedLocation !== null && (lat !== savedLocation.lat || lon !== savedLocation.lon); const dirty = timezoneDirty || locationDirty; + const hasDiscardableChanges = (timezoneDirty && !saving) + || (locationDirty && !savingLocation); const routeGuard = useUnsavedChangesGuard(dirty); useEffect(() => { @@ -48,7 +50,14 @@ export function GeneralTab() { setLon(nextLon); setSavedLocation({ lat: nextLat, lon: nextLon }); }) - .catch(() => toast.error('Failed to load settings')) + .catch(() => { + // The empty fields remain usable after a failed load, so treat the + // displayed fallback as the baseline instead of leaving edits outside + // dirty tracking. + setSavedTimezone(''); + setSavedLocation({ lat: '', lon: '' }); + toast.error('Failed to load settings'); + }) .finally(() => setLoading(false)); }, []); @@ -133,7 +142,7 @@ export function GeneralTab() {
setTimezone(e.target.value)} + disabled={saving} placeholder={detectedTz} - className="w-full sm:flex-1 sm:max-w-xs min-w-0 px-3 py-2 bg-port-bg border border-port-border rounded-lg text-white text-sm" + className="w-full sm:flex-1 sm:max-w-xs min-w-0 px-3 py-2 bg-port-bg border border-port-border rounded-lg text-white text-sm disabled:opacity-50" list="tz-list" /> {timezoneDirty && ( - Unsaved changes + + Unsaved changes + )} {!timezone && (
diff --git a/client/src/components/settings/GeneralTab.test.jsx b/client/src/components/settings/GeneralTab.test.jsx index c22277c8bd..5560c46625 100644 --- a/client/src/components/settings/GeneralTab.test.jsx +++ b/client/src/components/settings/GeneralTab.test.jsx @@ -102,12 +102,14 @@ describe('GeneralTab unsaved changes', () => { target: { value: 'America/New_York' }, }); expect(within(timezoneCard()).getByText('Unsaved changes')).toBeInTheDocument(); + expect(screen.getByRole('status', { name: 'Timezone has unsaved changes' })).toBeInTheDocument(); expect(within(locationCard()).queryByText('Unsaved changes')).toBeNull(); fireEvent.change(screen.getByLabelText('Latitude (-90 to 90)'), { target: { value: '40.7128' }, }); expect(within(locationCard()).getByText('Unsaved changes')).toBeInTheDocument(); + expect(screen.getByRole('status', { name: 'Location has unsaved changes' })).toBeInTheDocument(); await waitFor(() => { expect(add.mock.calls.some(([type]) => type === 'beforeunload')).toBe(true); }); @@ -227,7 +229,36 @@ describe('GeneralTab unsaved changes', () => { fireEvent.click(within(locationCard()).getByRole('button', { name: 'Save' })); expect(latitudeInput).toBeDisabled(); expect(longitudeInput).toBeDisabled(); + await navigate(router, '/settings/security'); + expect(screen.queryByText(CONFIRM)).toBeNull(); await act(async () => { locationSave.resolve({}); }); + expect(await screen.findByText('Security settings')).toBeInTheDocument(); + }); + + it('discards only the unsaved section while another section save is in flight', async () => { + const timezoneSave = deferred(); + updateSettings.mockReturnValueOnce(timezoneSave.promise); + const router = createMemoryRouter([ + { path: '/settings/:tab', element: }, + ], { initialEntries: ['/settings/old-tab'] }); + render(); + const timezoneInput = await screen.findByDisplayValue('UTC'); + const latitudeInput = screen.getByLabelText('Latitude (-90 to 90)'); + fireEvent.change(timezoneInput, { target: { value: 'America/New_York' } }); + fireEvent.change(latitudeInput, { target: { value: '40.7128' } }); + fireEvent.click(within(timezoneCard()).getByRole('button', { name: 'Save' })); + + await navigate(router, '/settings/general'); + expect(screen.getByText(CONFIRM)).toBeInTheDocument(); + fireEvent.click(screen.getByRole('button', { name: 'Discard' })); + + await waitFor(() => expect(router.state.location.pathname).toBe('/settings/general')); + expect(timezoneInput).toHaveValue('America/New_York'); + expect(timezoneInput).toBeDisabled(); + expect(latitudeInput).toHaveValue(String(SETTINGS.location.lat)); + await act(async () => { timezoneSave.resolve({}); }); + expect(timezoneInput).toHaveValue('America/New_York'); + expect(screen.queryByText('Unsaved changes')).toBeNull(); }); it('keeps a failed timezone save dirty and guarded', async () => {