From 9ce06412cf0991e288fc5a789f25d3413f6ee244 Mon Sep 17 00:00:00 2001 From: Bruno Raimbault Date: Wed, 29 Jul 2026 14:16:05 +0200 Subject: [PATCH 1/2] feat: mark unsaved changes on the map title and guard against losing them Appends " - Edited" to the map title when the current map differs from what was last saved, mirroring the data-visualizer-app UX, and warns before navigating away or closing the tab with unsaved changes. Co-Authored-By: Claude Sonnet 5 --- src/components/app/App.jsx | 9 +- src/components/app/ConfirmLeaveModal.jsx | 47 +++++ src/components/app/FileMenu.jsx | 2 +- .../app/__tests__/ConfirmLeaveModal.spec.jsx | 25 +++ .../app/__tests__/useLoadMap.spec.js | 160 ++++++++++++++++++ src/components/app/useLoadMap.js | 25 ++- src/components/map/MapName.jsx | 12 +- src/components/map/__tests__/MapName.spec.jsx | 67 ++++++++ src/components/map/styles/MapName.module.css | 4 + src/hooks/__tests__/useMapDirty.spec.js | 85 ++++++++++ .../__tests__/useUnsavedChangesGuard.spec.js | 120 +++++++++++++ src/hooks/useMapDirty.js | 23 +++ src/hooks/useUnsavedChangesGuard.js | 53 ++++++ src/reducers/__tests__/savedMap.spec.js | 53 ++++++ src/reducers/index.js | 2 + src/reducers/savedMap.js | 16 ++ src/util/__tests__/mapDirty.spec.js | 122 +++++++++++++ src/util/favorites.js | 4 +- src/util/mapDirty.js | 18 ++ 19 files changed, 837 insertions(+), 10 deletions(-) create mode 100644 src/components/app/ConfirmLeaveModal.jsx create mode 100644 src/components/app/__tests__/ConfirmLeaveModal.spec.jsx create mode 100644 src/components/app/__tests__/useLoadMap.spec.js create mode 100644 src/components/map/__tests__/MapName.spec.jsx create mode 100644 src/hooks/__tests__/useMapDirty.spec.js create mode 100644 src/hooks/__tests__/useUnsavedChangesGuard.spec.js create mode 100644 src/hooks/useMapDirty.js create mode 100644 src/hooks/useUnsavedChangesGuard.js create mode 100644 src/reducers/__tests__/savedMap.spec.js create mode 100644 src/reducers/savedMap.js create mode 100644 src/util/__tests__/mapDirty.spec.js create mode 100644 src/util/mapDirty.js diff --git a/src/components/app/App.jsx b/src/components/app/App.jsx index 91a8f4ab13..0b9cdf7a1a 100644 --- a/src/components/app/App.jsx +++ b/src/components/app/App.jsx @@ -8,6 +8,7 @@ import DownloadSettings from '../download/DownloadSettings.jsx' import LayersPanel from '../layers/LayersPanel.jsx' import MapPosition from '../map/MapPosition.jsx' import AppMenu from './AppMenu.jsx' +import ConfirmLeaveModal from './ConfirmLeaveModal.jsx' import DetailsPanel from './DetailsPanel.jsx' import ModalContainer from './ModalContainer.jsx' import './App.css' @@ -28,7 +29,7 @@ const App = () => { ) }, []) - useLoadMap() + const { locationToConfirm, confirmLeave, cancelLeave } = useLoadMap() useLoadDataStore() useLayersLoader() @@ -69,6 +70,12 @@ const App = () => { )} + {locationToConfirm && ( + + )} ) } diff --git a/src/components/app/ConfirmLeaveModal.jsx b/src/components/app/ConfirmLeaveModal.jsx new file mode 100644 index 0000000000..937d84b146 --- /dev/null +++ b/src/components/app/ConfirmLeaveModal.jsx @@ -0,0 +1,47 @@ +import i18n from '@dhis2/d2-i18n' +import { + Modal, + ModalTitle, + ModalContent, + ModalActions, + Button, + ButtonStrip, +} from '@dhis2/ui' +import PropTypes from 'prop-types' +import React from 'react' + +const ConfirmLeaveModal = ({ onCancel, onConfirm }) => ( + + {i18n.t('Discard unsaved changes?')} + + {i18n.t( + 'Are you sure you want to leave this map? Any unsaved changes will be lost.' + )} + + + + + + + + +) + +ConfirmLeaveModal.propTypes = { + onCancel: PropTypes.func.isRequired, + onConfirm: PropTypes.func.isRequired, +} + +export default ConfirmLeaveModal diff --git a/src/components/app/FileMenu.jsx b/src/components/app/FileMenu.jsx index cfd1b6c188..3e4f769682 100644 --- a/src/components/app/FileMenu.jsx +++ b/src/components/app/FileMenu.jsx @@ -207,7 +207,7 @@ const FileMenu = ({ onFileMenuAction }) => { saveAsAlert.show({ msg: getSavedMessage(getMapName(name)) }) if (res.response.uid) { - history.push(`/${res.response.uid}`) + history.push(`/${res.response.uid}`, { isSaving: true }) } } } diff --git a/src/components/app/__tests__/ConfirmLeaveModal.spec.jsx b/src/components/app/__tests__/ConfirmLeaveModal.spec.jsx new file mode 100644 index 0000000000..65daad002d --- /dev/null +++ b/src/components/app/__tests__/ConfirmLeaveModal.spec.jsx @@ -0,0 +1,25 @@ +import { render, fireEvent, screen } from '@testing-library/react' +import React from 'react' +import ConfirmLeaveModal from '../ConfirmLeaveModal.jsx' + +describe('ConfirmLeaveModal', () => { + it('calls onCancel when the cancel button is clicked', () => { + const onCancel = jest.fn() + render() + + fireEvent.click(screen.getByTestId('confirm-leave-modal-option-cancel')) + + expect(onCancel).toHaveBeenCalledTimes(1) + }) + + it('calls onConfirm when the leave button is clicked', () => { + const onConfirm = jest.fn() + render() + + fireEvent.click( + screen.getByTestId('confirm-leave-modal-option-confirm') + ) + + expect(onConfirm).toHaveBeenCalledTimes(1) + }) +}) diff --git a/src/components/app/__tests__/useLoadMap.spec.js b/src/components/app/__tests__/useLoadMap.spec.js new file mode 100644 index 0000000000..68fcdb151c --- /dev/null +++ b/src/components/app/__tests__/useLoadMap.spec.js @@ -0,0 +1,160 @@ +import { renderHook, act, waitFor } from '@testing-library/react' +import history from '../../../util/history.js' +import { useLoadMap } from '../useLoadMap.js' + +const mockDispatch = jest.fn() +let mockMapState +let mockSavedMapState + +jest.mock('query-string', () => ({ + parse: jest.fn(() => ({})), +})) + +jest.mock('react-redux', () => ({ + useDispatch: () => mockDispatch, + useSelector: (selector) => + selector({ map: mockMapState, savedMap: mockSavedMapState }), +})) + +jest.mock('@dhis2/app-runtime', () => ({ + useDataEngine: () => ({ + query: jest.fn().mockResolvedValue({}), + mutate: jest.fn().mockResolvedValue({}), + }), +})) + +jest.mock('@dhis2/app-service-alerts', () => ({ + useAlert: () => ({ show: jest.fn() }), +})) + +jest.mock('../../cachedDataProvider/CachedDataProvider.jsx', () => ({ + useCachedData: () => ({ + systemSettings: { keyDefaultBaseMap: 'osm' }, + basemaps: [{ id: 'osm' }], + }), +})) + +jest.mock('../../../util/requests.js', () => ({ + fetchMap: jest.fn((args) => + Promise.resolve({ + id: args.id, + basemap: { id: 'osm' }, + mapViews: [], + }) + ), +})) + +jest.mock('../../../util/basemaps.js', () => ({ + getBasemapOrFallback: ({ id }) => ({ id }), +})) + +const dirtyMap = { id: 'map1', mapViews: [{ id: 'layer1', opacity: 0.5 }] } +const savedMap = { id: 'map1', mapViews: [{ id: 'layer1', opacity: 1 }] } + +describe('useLoadMap - unsaved changes guard', () => { + beforeEach(() => { + mockDispatch.mockClear() + mockMapState = savedMap + mockSavedMapState = savedMap + }) + + const mountOnMap = async (mapId) => { + history.replace(`/${mapId}`) + const hook = renderHook(() => useLoadMap()) + // Let the initial mount-time loadMap() resolve so previousParamsRef + // picks up this map id before we simulate further navigation. + await act(async () => { + await Promise.resolve() + await Promise.resolve() + }) + return hook + } + + it('shows a confirm dialog when dirty and switching to a different map', async () => { + mockMapState = dirtyMap + const { result } = await mountOnMap('map1') + + act(() => { + history.push('/map2') + }) + + expect(result.current.locationToConfirm).not.toBeNull() + }) + + it('does not gate navigation when the map is not dirty', async () => { + mockMapState = savedMap + const { result } = await mountOnMap('map1') + mockDispatch.mockClear() + + act(() => { + history.push('/map2') + }) + + expect(result.current.locationToConfirm).toBeNull() + await waitFor(() => expect(mockDispatch).toHaveBeenCalled()) + }) + + it('confirmLeave navigates to the pending location and clears it', async () => { + mockMapState = dirtyMap + const { result } = await mountOnMap('map1') + + act(() => { + history.push('/map2') + }) + expect(result.current.locationToConfirm).not.toBeNull() + + mockDispatch.mockClear() + await act(async () => { + result.current.confirmLeave() + await Promise.resolve() + await Promise.resolve() + }) + + expect(result.current.locationToConfirm).toBeNull() + expect(mockDispatch).toHaveBeenCalled() + }) + + it('cancelLeave clears the pending location without loading it', async () => { + mockMapState = dirtyMap + const { result } = await mountOnMap('map1') + + act(() => { + history.push('/map2') + }) + expect(result.current.locationToConfirm).not.toBeNull() + + mockDispatch.mockClear() + act(() => { + result.current.cancelLeave() + }) + + expect(result.current.locationToConfirm).toBeNull() + expect(mockDispatch).not.toHaveBeenCalled() + }) + + it('does not gate a Save As redirect even when dirty', async () => { + mockMapState = dirtyMap + const { result } = await mountOnMap('map1') + mockDispatch.mockClear() + + act(() => { + history.push('/map2', { isSaving: true }) + }) + + expect(result.current.locationToConfirm).toBeNull() + await waitFor(() => expect(mockDispatch).toHaveBeenCalled()) + }) + + it('does not gate re-opening the same map while dirty', async () => { + mockMapState = dirtyMap + const { result } = await mountOnMap('map1') + mockDispatch.mockClear() + + act(() => { + history.replace('/map1') + }) + + expect(result.current.locationToConfirm).toBeNull() + await waitFor(() => expect(mockDispatch).toHaveBeenCalled()) + }) +}) diff --git a/src/components/app/useLoadMap.js b/src/components/app/useLoadMap.js index d035e5a2e8..721cda8d2e 100644 --- a/src/components/app/useLoadMap.js +++ b/src/components/app/useLoadMap.js @@ -11,6 +11,7 @@ import { ALERT_CRITICAL, ALERT_MESSAGE_DYNAMIC, } from '../../constants/alerts.js' +import { useUnsavedChangesGuard } from '../../hooks/useUnsavedChangesGuard.js' import { CURRENT_AO_KEY } from '../../util/analyticalObject.js' import { dataStatisticsMutation } from '../../util/apiDataStatistics.js' import { getBasemapOrFallback } from '../../util/basemaps.js' @@ -89,6 +90,14 @@ export const useLoadMap = () => { [basemaps, defaultBasemap, dispatch, engine] ) + const { + locationToConfirm, + setLocationToConfirm, + isDirtyNow, + confirmLeave, + cancelLeave, + } = useUnsavedChangesGuard(loadMap) + useEffect(() => { loadMap(history.location) }, [loadMap]) @@ -122,11 +131,15 @@ export const useLoadMap = () => { ) const params = getHashUrlParams(location) - - if ( - action === 'REPLACE' || + const isSwitchingMap = previousParamsRef.current.mapId !== params.mapId - ) { + + if (isSwitchingMap && !location.state?.isSaving && isDirtyNow()) { + setLocationToConfirm(location) + return + } + + if (action === 'REPLACE' || isSwitchingMap) { loadMap(location) return } @@ -144,5 +157,7 @@ export const useLoadMap = () => { }) return () => unlisten?.() - }, [loadMap, dispatch]) + }, [loadMap, dispatch, isDirtyNow, setLocationToConfirm]) + + return { locationToConfirm, confirmLeave, cancelLeave } } diff --git a/src/components/map/MapName.jsx b/src/components/map/MapName.jsx index 9e098b21ff..2213682bf4 100644 --- a/src/components/map/MapName.jsx +++ b/src/components/map/MapName.jsx @@ -1,14 +1,24 @@ +import i18n from '@dhis2/d2-i18n' import React from 'react' import { useSelector } from 'react-redux' +import { useMapDirty } from '../../hooks/useMapDirty.js' import styles from './styles/MapName.module.css' const MapName = () => { const name = useSelector((state) => state.map.displayName) const downloadMode = useSelector((state) => state.ui.downloadMode) + const dirty = useMapDirty() return !downloadMode && name ? (
-
{name}
+
+ {name} + {dirty && ( + + {` - ${i18n.t('Edited')}`} + + )} +
) : null } diff --git a/src/components/map/__tests__/MapName.spec.jsx b/src/components/map/__tests__/MapName.spec.jsx new file mode 100644 index 0000000000..28e908bd42 --- /dev/null +++ b/src/components/map/__tests__/MapName.spec.jsx @@ -0,0 +1,67 @@ +import { render } from '@testing-library/react' +import React from 'react' +import { Provider } from 'react-redux' +import configureMockStore from 'redux-mock-store' +import { useMapDirty } from '../../../hooks/useMapDirty.js' +import MapName from '../MapName.jsx' + +jest.mock('../../../hooks/useMapDirty.js') + +const mockStore = configureMockStore() + +const renderWithState = ({ displayName, downloadMode = false }) => + render( + + + + ) + +describe('MapName', () => { + afterEach(() => { + jest.clearAllMocks() + }) + + it('renders nothing when there is no map name', () => { + useMapDirty.mockReturnValue(false) + + const { queryByTestId } = renderWithState({ displayName: undefined }) + + expect(queryByTestId('map-name')).toBeNull() + }) + + it('renders nothing in download mode', () => { + useMapDirty.mockReturnValue(false) + + const { queryByTestId } = renderWithState({ + displayName: 'My map', + downloadMode: true, + }) + + expect(queryByTestId('map-name')).toBeNull() + }) + + it('renders the name without a suffix when not dirty', () => { + useMapDirty.mockReturnValue(false) + + const { getByTestId, queryByTestId } = renderWithState({ + displayName: 'My map', + }) + + expect(getByTestId('map-name')).toHaveTextContent('My map') + expect(queryByTestId('map-name-edited')).toBeNull() + }) + + it('appends the "- Edited" suffix when dirty', () => { + useMapDirty.mockReturnValue(true) + + const { getByTestId } = renderWithState({ displayName: 'My map' }) + + expect(getByTestId('map-name-edited')).toHaveTextContent('Edited') + expect(getByTestId('map-name')).toHaveTextContent('My map - Edited') + }) +}) diff --git a/src/components/map/styles/MapName.module.css b/src/components/map/styles/MapName.module.css index 42c0d05c11..eed9da3fc9 100644 --- a/src/components/map/styles/MapName.module.css +++ b/src/components/map/styles/MapName.module.css @@ -19,3 +19,7 @@ padding: 6px var(--spacers-dp8); line-height: 17px; } + +.edited { + color: var(--colors-grey700); +} diff --git a/src/hooks/__tests__/useMapDirty.spec.js b/src/hooks/__tests__/useMapDirty.spec.js new file mode 100644 index 0000000000..bb710aa1da --- /dev/null +++ b/src/hooks/__tests__/useMapDirty.spec.js @@ -0,0 +1,85 @@ +import { renderHook, act } from '@testing-library/react' +import React from 'react' +import { Provider } from 'react-redux' +import { createStore } from 'redux' +import { changeLayerOpacity } from '../../actions/layers.js' +import { setMap } from '../../actions/map.js' +import rootReducer from '../../reducers/index.js' +import { useMapDirty } from '../useMapDirty.js' + +const savedMapConfig = { + id: 'abc', + name: 'My map', + basemap: { id: 'osm', opacity: 1, isVisible: true }, + mapViews: [{ id: 'layer1', opacity: 1, isVisible: true }], +} + +const renderWithStore = () => { + const store = createStore(rootReducer) + store.dispatch(setMap(savedMapConfig)) + + const { result } = renderHook(() => useMapDirty(), { + wrapper: ({ children }) => ( + {children} + ), + }) + + return { store, result } +} + +describe('useMapDirty', () => { + beforeEach(() => { + jest.useFakeTimers() + }) + + afterEach(() => { + jest.useRealTimers() + }) + + it('is false right after a map is loaded', () => { + const { result } = renderWithStore() + + act(() => { + jest.advanceTimersByTime(300) + }) + + expect(result.current).toBe(false) + }) + + it('becomes true after the debounce window once the map changes', () => { + const { store, result } = renderWithStore() + + act(() => { + store.dispatch(changeLayerOpacity('layer1', 0.5)) + }) + + // Still inside the debounce window + expect(result.current).toBe(false) + + act(() => { + jest.advanceTimersByTime(300) + }) + + expect(result.current).toBe(true) + }) + + it('only settles once after rapid successive changes (simulated slider drag)', () => { + const { store, result } = renderWithStore() + + act(() => { + for (let opacity = 0.9; opacity >= 0.5; opacity -= 0.1) { + store.dispatch(changeLayerOpacity('layer1', opacity)) + jest.advanceTimersByTime(50) // faster than the 300ms debounce + } + }) + + // The debounce window keeps getting reset, so it never fired yet + expect(result.current).toBe(false) + + act(() => { + jest.advanceTimersByTime(300) + }) + + expect(result.current).toBe(true) + }) +}) diff --git a/src/hooks/__tests__/useUnsavedChangesGuard.spec.js b/src/hooks/__tests__/useUnsavedChangesGuard.spec.js new file mode 100644 index 0000000000..a602bdd878 --- /dev/null +++ b/src/hooks/__tests__/useUnsavedChangesGuard.spec.js @@ -0,0 +1,120 @@ +import { renderHook, act, waitFor } from '@testing-library/react' +import history from '../../util/history.js' +import { useUnsavedChangesGuard } from '../useUnsavedChangesGuard.js' + +let mockMapState +let mockSavedMapState + +jest.mock('query-string', () => ({ + parse: jest.fn(() => ({})), +})) + +jest.mock('react-redux', () => ({ + useSelector: (selector) => + selector({ map: mockMapState, savedMap: mockSavedMapState }), +})) + +const dirtyMap = { id: 'map1', mapViews: [{ id: 'layer1', opacity: 0.5 }] } +const savedMap = { id: 'map1', mapViews: [{ id: 'layer1', opacity: 1 }] } + +describe('useUnsavedChangesGuard', () => { + beforeEach(() => { + mockMapState = savedMap + mockSavedMapState = savedMap + }) + + it('isDirtyNow reflects the current map vs savedMap comparison', () => { + mockMapState = dirtyMap + const { result } = renderHook(() => useUnsavedChangesGuard(jest.fn())) + + expect(result.current.isDirtyNow()).toBe(true) + }) + + it('isDirtyNow is false when the map matches the saved snapshot', () => { + const { result } = renderHook(() => useUnsavedChangesGuard(jest.fn())) + + expect(result.current.isDirtyNow()).toBe(false) + }) + + it('confirmLeave calls loadLocation with the pending location and clears it', () => { + const loadLocation = jest.fn() + const { result } = renderHook(() => + useUnsavedChangesGuard(loadLocation) + ) + + act(() => { + result.current.setLocationToConfirm({ pathname: '/map2' }) + }) + expect(result.current.locationToConfirm).toEqual({ + pathname: '/map2', + }) + + act(() => { + result.current.confirmLeave() + }) + + expect(loadLocation).toHaveBeenCalledWith({ pathname: '/map2' }) + expect(result.current.locationToConfirm).toBeNull() + }) + + it('confirmLeave is a no-op when there is nothing pending', () => { + const loadLocation = jest.fn() + const { result } = renderHook(() => + useUnsavedChangesGuard(loadLocation) + ) + + act(() => { + result.current.confirmLeave() + }) + + expect(loadLocation).not.toHaveBeenCalled() + }) + + it('cancelLeave clears the pending location and navigates back', async () => { + const { result } = renderHook(() => useUnsavedChangesGuard(jest.fn())) + + act(() => { + history.push('/before-cancel') + history.push('/map2') + }) + expect(history.location.pathname).toBe('/map2') + + act(() => { + result.current.setLocationToConfirm({ pathname: '/map2' }) + }) + + act(() => { + result.current.cancelLeave() + }) + + expect(result.current.locationToConfirm).toBeNull() + // history.back() navigates via the browser's popstate mechanism, + // which resolves asynchronously rather than within this act() call. + await waitFor(() => + expect(history.location.pathname).toBe('/before-cancel') + ) + }) + + it('prevents the tab from closing via beforeunload when dirty', () => { + mockMapState = dirtyMap + renderHook(() => useUnsavedChangesGuard(jest.fn())) + + const event = new Event('beforeunload', { cancelable: true }) + act(() => { + window.dispatchEvent(event) + }) + + expect(event.defaultPrevented).toBe(true) + }) + + it('does not block beforeunload when not dirty', () => { + renderHook(() => useUnsavedChangesGuard(jest.fn())) + + const event = new Event('beforeunload', { cancelable: true }) + act(() => { + window.dispatchEvent(event) + }) + + expect(event.defaultPrevented).toBe(false) + }) +}) diff --git a/src/hooks/useMapDirty.js b/src/hooks/useMapDirty.js new file mode 100644 index 0000000000..f48e376117 --- /dev/null +++ b/src/hooks/useMapDirty.js @@ -0,0 +1,23 @@ +import { useEffect, useRef, useState } from 'react' +import { useSelector } from 'react-redux' +import { isMapDirty } from '../util/mapDirty.js' + +const DEBOUNCE_MS = 300 + +export const useMapDirty = () => { + const map = useSelector((state) => state.map) + const savedMap = useSelector((state) => state.savedMap) + const [dirty, setDirty] = useState(false) + const timeoutRef = useRef() + + useEffect(() => { + clearTimeout(timeoutRef.current) + timeoutRef.current = setTimeout( + () => setDirty(isMapDirty(map, savedMap)), + DEBOUNCE_MS + ) + return () => clearTimeout(timeoutRef.current) + }, [map, savedMap]) + + return dirty +} diff --git a/src/hooks/useUnsavedChangesGuard.js b/src/hooks/useUnsavedChangesGuard.js new file mode 100644 index 0000000000..2937451abd --- /dev/null +++ b/src/hooks/useUnsavedChangesGuard.js @@ -0,0 +1,53 @@ +import i18n from '@dhis2/d2-i18n' +import { useCallback, useEffect, useRef, useState } from 'react' +import { useSelector } from 'react-redux' +import history from '../util/history.js' +import { isMapDirty } from '../util/mapDirty.js' + +export const useUnsavedChangesGuard = (loadLocation) => { + const mapRef = useRef() + mapRef.current = useSelector((state) => state.map) + const savedMapRef = useRef() + savedMapRef.current = useSelector((state) => state.savedMap) + + const [locationToConfirm, setLocationToConfirm] = useState(null) + + const isDirtyNow = useCallback( + () => isMapDirty(mapRef.current, savedMapRef.current), + [] + ) + + useEffect(() => { + const onBeforeUnload = (event) => { + if (isDirtyNow()) { + event.preventDefault() + // Required for triggering the unload confirmation dialog + // See: https://developer.mozilla.org/en-US/docs/Web/API/Window/beforeunload_event + event.returnValue = i18n.t('You have unsaved changes.') + } + } + + window.addEventListener('beforeunload', onBeforeUnload) + return () => window.removeEventListener('beforeunload', onBeforeUnload) + }, [isDirtyNow]) + + const confirmLeave = useCallback(() => { + if (locationToConfirm) { + loadLocation(locationToConfirm) + setLocationToConfirm(null) + } + }, [locationToConfirm, loadLocation]) + + const cancelLeave = useCallback(() => { + setLocationToConfirm(null) + history.back() + }, []) + + return { + locationToConfirm, + setLocationToConfirm, + isDirtyNow, + confirmLeave, + cancelLeave, + } +} diff --git a/src/reducers/__tests__/savedMap.spec.js b/src/reducers/__tests__/savedMap.spec.js new file mode 100644 index 0000000000..4906bc6b32 --- /dev/null +++ b/src/reducers/__tests__/savedMap.spec.js @@ -0,0 +1,53 @@ +import * as types from '../../constants/actionTypes.js' +import savedMap from '../savedMap.js' + +describe('savedMap reducer', () => { + it('defaults to null', () => { + expect(savedMap(undefined, { type: '@@INIT' })).toBe(null) + }) + + it('MAP_SET replaces state with the payload', () => { + const payload = { id: 'abc', name: 'My map', mapViews: [] } + + expect(savedMap({ id: 'old' }, { type: types.MAP_SET, payload })).toBe( + payload + ) + }) + + it('MAP_NEW resets to null', () => { + const state = { id: 'abc', name: 'My map', mapViews: [] } + + expect(savedMap(state, { type: types.MAP_NEW })).toBe(null) + }) + + it('MAP_PROPS_SET merges props into the current snapshot', () => { + const state = { id: 'abc', name: 'Old name', mapViews: [] } + + const result = savedMap(state, { + type: types.MAP_PROPS_SET, + payload: { name: 'New name', displayName: 'New name' }, + }) + + expect(result).toEqual({ + id: 'abc', + name: 'New name', + displayName: 'New name', + mapViews: [], + }) + }) + + it('MAP_PROPS_SET is a no-op when there is no saved snapshot yet', () => { + expect( + savedMap(null, { + type: types.MAP_PROPS_SET, + payload: { name: 'New name' }, + }) + ).toBe(null) + }) + + it('returns the same state for unrelated actions', () => { + const state = { id: 'abc', mapViews: [] } + + expect(savedMap(state, { type: types.DATA_TABLE_TOGGLE })).toBe(state) + }) +}) diff --git a/src/reducers/index.js b/src/reducers/index.js index dae6948282..b3720ded7a 100644 --- a/src/reducers/index.js +++ b/src/reducers/index.js @@ -11,6 +11,7 @@ import layerEdit from './layerEdit.js' import layerSources from './layerSources.js' import map from './map.js' import orgUnitProfile from './orgUnitProfile.js' +import savedMap from './savedMap.js' import ui from './ui.js' export default combineReducers({ @@ -24,6 +25,7 @@ export default combineReducers({ layerSources, map, orgUnitProfile, + savedMap, ui, feature, featureProfile, diff --git a/src/reducers/savedMap.js b/src/reducers/savedMap.js new file mode 100644 index 0000000000..3db47630e3 --- /dev/null +++ b/src/reducers/savedMap.js @@ -0,0 +1,16 @@ +import * as types from '../constants/actionTypes.js' + +const savedMap = (state = null, action) => { + switch (action.type) { + case types.MAP_NEW: + return null + case types.MAP_SET: + return action.payload + case types.MAP_PROPS_SET: + return state ? { ...state, ...action.payload } : state + default: + return state + } +} + +export default savedMap diff --git a/src/util/__tests__/mapDirty.spec.js b/src/util/__tests__/mapDirty.spec.js new file mode 100644 index 0000000000..fc39ad909a --- /dev/null +++ b/src/util/__tests__/mapDirty.spec.js @@ -0,0 +1,122 @@ +import { isMapDirty } from '../mapDirty.js' + +const baseMap = () => ({ + id: 'abc', + name: 'My map', + basemap: { id: 'osm', opacity: 1, isVisible: true }, + mapViews: [{ id: 'layer1', opacity: 1, isVisible: true }], +}) + +describe('isMapDirty', () => { + it('is false when there is no saved snapshot yet', () => { + expect(isMapDirty(baseMap(), null)).toBe(false) + }) + + it('is false for identical maps', () => { + expect(isMapDirty(baseMap(), baseMap())).toBe(false) + }) + + it('is true when a layer is added', () => { + const map = baseMap() + map.mapViews = [...map.mapViews, { id: 'layer2', opacity: 1 }] + + expect(isMapDirty(map, baseMap())).toBe(true) + }) + + it('is true when a layer is removed', () => { + const map = baseMap() + map.mapViews = [] + + expect(isMapDirty(map, baseMap())).toBe(true) + }) + + it('is true when the basemap changes', () => { + const map = baseMap() + map.basemap = { ...map.basemap, id: 'terrain' } + + expect(isMapDirty(map, baseMap())).toBe(true) + }) + + it('is true when a layer opacity changes', () => { + const map = baseMap() + map.mapViews = [{ ...map.mapViews[0], opacity: 0.5 }] + + expect(isMapDirty(map, baseMap())).toBe(true) + }) + + it('ignores ephemeral map-level fields', () => { + const map = baseMap() + map.coordinatePopup = { lat: 1, lng: 2 } + map.alerts = [{ message: 'oops' }] + + expect(isMapDirty(map, baseMap())).toBe(false) + }) + + it('ignores ephemeral layer-level fields', () => { + const map = baseMap() + map.mapViews = [ + { + ...map.mapViews[0], + isLoading: true, + coordinate: { lat: 1, lng: 2 }, + dataFilters: { field: 'x' }, + isExpanded: false, + alerts: [{ message: 'oops' }], + }, + ] + + expect(isMapDirty(map, baseMap())).toBe(false) + }) + + it('ignores ephemeral basemap fields', () => { + const map = baseMap() + map.basemap = { ...map.basemap, isExpanded: false } + + expect(isMapDirty(map, baseMap())).toBe(false) + }) + + it('ignores a client-only bounds field present on map but absent on savedMap', () => { + const map = { + ...baseMap(), + bounds: [ + [-1, -1], + [1, 1], + ], + } + + expect(isMapDirty(map, baseMap())).toBe(false) + }) + + it('ignores loader-added fields right after a fresh load (regression)', () => { + const map = baseMap() + map.mapViews = [ + { + ...map.mapViews[0], + isLoaded: true, + isLoading: false, + loadError: null, + data: [{ type: 'Feature' }], + periods: [{ id: '2024' }], + valuesByPeriod: { 2024: [1, 2, 3] }, + legend: { items: [] }, + alerts: [{ message: 'partial data' }], + }, + ] + + expect(isMapDirty(map, baseMap())).toBe(false) + }) + + it('still catches a real content change on a loaded layer', () => { + const map = baseMap() + map.mapViews = [ + { + ...map.mapViews[0], + isLoaded: true, + data: [{ type: 'Feature' }], + opacity: 0.3, + }, + ] + + expect(isMapDirty(map, baseMap())).toBe(true) + }) +}) diff --git a/src/util/favorites.js b/src/util/favorites.js index 520c919873..1be72539d5 100644 --- a/src/util/favorites.js +++ b/src/util/favorites.js @@ -10,7 +10,7 @@ import { } from '../constants/layers.js' // TODO: get latitude, longitude, zoom from map + basemap: 'none' -const validMapProperties = [ +export const validMapProperties = [ // 'basemap' and 'basemaps' removed — set exclusively by getBasemapPayload 'id', 'latitude', @@ -25,7 +25,7 @@ const validMapProperties = [ 'subscribers', ] -const validLayerProperties = [ +export const validLayerProperties = [ 'aggregationType', 'areaRadius', 'geometryCentroid', diff --git a/src/util/mapDirty.js b/src/util/mapDirty.js new file mode 100644 index 0000000000..ce268f8eb9 --- /dev/null +++ b/src/util/mapDirty.js @@ -0,0 +1,18 @@ +import { isEqual, pick } from 'lodash' +import { validLayerProperties, validMapProperties } from './favorites.js' + +// Loaders replace a layer's config with `{ ...config, ...loadedFields }` on +// every load, so compare only the saved-payload allowlist, not raw objects. +const layerFields = [...validLayerProperties, 'isVisible'] +const basemapFields = ['id', 'opacity', 'isVisible'] + +const stripLayer = (layer) => pick(layer, layerFields) + +const stripMap = (map) => ({ + ...pick(map, validMapProperties), + basemap: pick(map.basemap, basemapFields), + mapViews: map.mapViews.map(stripLayer), +}) + +export const isMapDirty = (map, savedMap) => + !!savedMap && !isEqual(stripMap(map), stripMap(savedMap)) From 75c664221b6208fb58da4e356089bc8fb74d064d Mon Sep 17 00:00:00 2001 From: Bruno Raimbault Date: Wed, 29 Jul 2026 14:47:27 +0200 Subject: [PATCH 2/2] fix: clear stale confirm-leave dialog after non-dialog navigation If the browser Back/Forward buttons revert a blocked navigation instead of the dialog's own Cancel/Leave buttons, the confirm-leave modal was never cleared -- it stayed on screen pointing at a stale location, and acting on it (Cancel or Leave) would navigate further than intended or desync the URL from the rendered map. Clear the pending confirm whenever the listener proceeds unblocked. Also stabilizes two useLoadMap test mocks (useDataEngine, useCachedData) that returned a fresh object per render, which destabilized loadMap's identity and caused its mount-effect to spuriously re-fire and mask this bug in the regression test. Co-Authored-By: Claude Sonnet 5 --- .../app/__tests__/useLoadMap.spec.js | 37 +++++++++++++++---- src/components/app/useLoadMap.js | 2 + 2 files changed, 31 insertions(+), 8 deletions(-) diff --git a/src/components/app/__tests__/useLoadMap.spec.js b/src/components/app/__tests__/useLoadMap.spec.js index 68fcdb151c..0360d37780 100644 --- a/src/components/app/__tests__/useLoadMap.spec.js +++ b/src/components/app/__tests__/useLoadMap.spec.js @@ -16,22 +16,26 @@ jest.mock('react-redux', () => ({ selector({ map: mockMapState, savedMap: mockSavedMapState }), })) +const mockEngine = { + query: jest.fn().mockResolvedValue({}), + mutate: jest.fn().mockResolvedValue({}), +} + jest.mock('@dhis2/app-runtime', () => ({ - useDataEngine: () => ({ - query: jest.fn().mockResolvedValue({}), - mutate: jest.fn().mockResolvedValue({}), - }), + useDataEngine: () => mockEngine, })) jest.mock('@dhis2/app-service-alerts', () => ({ useAlert: () => ({ show: jest.fn() }), })) +const mockCachedData = { + systemSettings: { keyDefaultBaseMap: 'osm' }, + basemaps: [{ id: 'osm' }], +} + jest.mock('../../cachedDataProvider/CachedDataProvider.jsx', () => ({ - useCachedData: () => ({ - systemSettings: { keyDefaultBaseMap: 'osm' }, - basemaps: [{ id: 'osm' }], - }), + useCachedData: () => mockCachedData, })) jest.mock('../../../util/requests.js', () => ({ @@ -157,4 +161,21 @@ describe('useLoadMap - unsaved changes guard', () => { expect(result.current.locationToConfirm).toBeNull() await waitFor(() => expect(mockDispatch).toHaveBeenCalled()) }) + + it('clears a pending confirm when the browser Back button reverts the navigation itself', async () => { + mockMapState = dirtyMap + const { result } = await mountOnMap('map1') + + act(() => { + history.push('/map2') + }) + expect(result.current.locationToConfirm).not.toBeNull() + + act(() => { + history.back() + }) + await waitFor(() => expect(history.location.pathname).toBe('/map1')) + + expect(result.current.locationToConfirm).toBeNull() + }) }) diff --git a/src/components/app/useLoadMap.js b/src/components/app/useLoadMap.js index 721cda8d2e..0d27689f27 100644 --- a/src/components/app/useLoadMap.js +++ b/src/components/app/useLoadMap.js @@ -139,6 +139,8 @@ export const useLoadMap = () => { return } + setLocationToConfirm(null) + if (action === 'REPLACE' || isSwitchingMap) { loadMap(location) return