From 6e251a009a36f0eceda60213f9540d0b44191a9b Mon Sep 17 00:00:00 2001 From: lrnselfreliance <33263708+lrnselfreliance@users.noreply.github.com> Date: Sat, 26 Sep 2026 16:55:42 -0600 Subject: [PATCH] app: map pins and subscriptions tables distinguish pending from empty (#732) * app: map pins and subscriptions tables distinguish pending from empty Both lists were initialised to [], which SortableTable renders as "loaded and empty". The Pins tab therefore showed "No pins. Right-click on the map to add one." until the pins arrived, and the Manage tab's subscriptions table showed "No results" until the catalog did. A non-OK response (the api helpers return undefined) or a thrown request left the same messages up permanently. Invariants now enforced and tested: * null = pending, undefined = fetch failed, [] = confirmed empty, so SortableTable shows its placeholder while pending and each empty message appears only once the server confirmed the list is empty. * A failed fetch renders an error in place of the table. * Only the newest request for each list may write state, since edits, deletes, subscribes and unsubscribes refetch while the mount request may still be in flight. Co-Authored-By: Claude Fable 5.1 * app: the regions preview waits for the catalog "View All Regions" stayed enabled while the catalog was pending or failed, and the modal received a fresh `catalog || []` on every render. Its map effect depends on that array, and ManageMap re-renders on every status poll, so an open preview tore its WebGL map down and rebuilt it every three seconds, and a failed catalog was previewed as zero regions over the error message. The button is disabled until the catalog is an array, and the modal is mounted only then, so it receives the state array itself. Co-Authored-By: Claude Fable 5.1 --------- Co-authored-by: Claude Fable 5.1 --- app/src/components/Map.js | 69 ++++++++++--- app/src/components/MapTables.test.js | 148 +++++++++++++++++++++++++++ 2 files changed, 203 insertions(+), 14 deletions(-) create mode 100644 app/src/components/MapTables.test.js diff --git a/app/src/components/Map.js b/app/src/components/Map.js index 6e4bd932..3c5ed0d5 100644 --- a/app/src/components/Map.js +++ b/app/src/components/Map.js @@ -1,4 +1,4 @@ -import React, {useEffect, useState} from "react"; +import React, {useEffect, useRef, useState} from "react"; import { APIButton, ErrorMessage, @@ -284,11 +284,15 @@ function MapCatalogRow({item, subscribedRegions, fetchData}) { function ManageMap() { const [files, setFiles] = useState(null); - const [catalog, setCatalog] = useState([]); + // null = pending, undefined = fetch failed, [] = no regions in the catalog. + const [catalog, setCatalog] = useState(null); const [subscriptions, setSubscriptions] = useState([]); const [allRegionsOpen, setAllRegionsOpen] = useState(false); const {status} = React.useContext(StatusContext); const searchBuilding = status?.flags?.map_search_building; + // Only the newest subscriptions request may write state: a subscribe or unsubscribe refetches + // while the mount request may still be in flight. + const subscriptionsGen = useRef(0); const fetchFiles = async () => { try { @@ -303,14 +307,24 @@ function ManageMap() { }; const fetchSubscriptionData = async () => { + const gen = ++subscriptionsGen.current; try { const data = await fetchMapSubscriptions(); + if (gen !== subscriptionsGen.current) { + return; + } if (data) { setCatalog(data.catalog || []); setSubscriptions(data.subscriptions || []); + } else { + // The api helper already toasted the non-OK response. + setCatalog(undefined); } } catch (e) { console.error(e); + if (gen === subscriptionsGen.current) { + setCatalog(undefined); + } } }; @@ -397,6 +411,8 @@ function ManageMap() { ; } + const catalogLoaded = Array.isArray(catalog); + /* * A fragment, not a `PageContainer`. `MapRoute` already provides the page chrome -- a * `wrolpi-stack` wrapper with its own top margin -- so a second one put this page a further @@ -419,18 +435,26 @@ function ManageMap() {
Map Subscriptions
- + {/* A pending or failed catalog is not "zero regions": the preview waits for one. The + modal is mounted only then, so it receives the state array itself and its map is + not rebuilt by a fresh `[]` on every status tick. */} +
- setAllRegionsOpen(false)}/> + {catalogLoaded && setAllRegionsOpen(false)}/>} - + {catalog === undefined + ? Could not fetch map subscriptions + : }
How to get map files
@@ -497,17 +521,29 @@ function PinEditRow({pin, onSave, onCancel}) { } function MapPins() { - const [pins, setPins] = useState([]); + // null = pending, undefined = fetch failed, [] = no pins. The empty row invites the user to + // add one, so it may only appear for a confirmed empty list. + const [pins, setPins] = useState(null); const [filter, setFilter] = useState(''); const [editingId, setEditingId] = useState(null); const navigate = useNavigate(); + // Only the newest pins request may write state: an edit or delete refetches while the mount + // request may still be in flight. + const pinsGen = useRef(0); const fetchPins = async () => { + const gen = ++pinsGen.current; try { const data = await getMapPins(); - if (data) setPins(data.pins || []); + if (gen === pinsGen.current) { + // The api helper returns undefined on a non-OK response. + setPins(data ? data.pins || [] : undefined); + } } catch (e) { console.error(e); + if (gen === pinsGen.current) { + setPins(undefined); + } } }; @@ -627,8 +663,13 @@ function MapPins() { No pins. Right-click on the map to add one. ; + if (pins === undefined) { + return Could not fetch map pins; + } + const lowerFilter = filter.toLowerCase(); - const filteredPins = lowerFilter + // Pending (null) passes through so SortableTable shows its placeholder. + const filteredPins = lowerFilter && pins ? pins.filter(p => (p.label || '').toLowerCase().includes(lowerFilter) || `${p.lat.toFixed(4)}, ${p.lon.toFixed(4)}`.includes(lowerFilter) diff --git a/app/src/components/MapTables.test.js b/app/src/components/MapTables.test.js new file mode 100644 index 00000000..efbf0242 --- /dev/null +++ b/app/src/components/MapTables.test.js @@ -0,0 +1,148 @@ +import React from 'react'; +import {act, screen} from '@testing-library/react'; +import {Route, Routes} from 'react-router'; +import {renderWithProviders as render} from '../test-utils'; +import {MapRoute} from './Map'; + +// What the map's Pins and Manage tabs show while their lists are still loading, when the +// fetch failed, and when the server confirmed the list is empty. The empty messages invite +// the user to act ("Right-click on the map to add one"), so they may only appear for a +// confirmed empty list. + +jest.mock('maplibre-gl', () => ({ + __esModule: true, + default: {Map: class { + }, addProtocol: jest.fn(), Marker: class { + }}, +})); +jest.mock('pmtiles', () => ({ + __esModule: true, Protocol: class { + tile = jest.fn(); + }, +})); +jest.mock('protomaps-themes-base', () => ({__esModule: true, default: () => []})); +jest.mock('./MapViewer', () => ({__esModule: true, default: () => null})); + +jest.mock('../api', () => ({ + ...jest.requireActual('../api'), + getMapFiles: jest.fn(), + fetchMapSubscriptions: jest.fn(), + getMapPins: jest.fn(), +})); + +const api = require('../api'); + +const pending = () => new Promise(() => { +}); +const NO_PINS = /No pins\. Right-click/; +const NO_RESULTS = /^No results$/; + +// The pins page renders one table per breakpoint through `Media`, so the real media provider is +// needed; the queries below tolerate either variant being in the tree. +const renderAt = (route) => render( + }/>, + {route, withMedia: true}, +); + +beforeEach(() => { + jest.clearAllMocks(); + // The media provider reads matchMedia; the global stub's implementation is reset between + // tests, so give it one here. Only the computer breakpoint (1024px) matches, so the + // provider reports a desktop viewport and the pins page renders its desktop table. + window.matchMedia = jest.fn().mockImplementation(query => ({ + matches: query.includes('1024'), media: query, onchange: null, + addListener: jest.fn(), removeListener: jest.fn(), + addEventListener: jest.fn(), removeEventListener: jest.fn(), dispatchEvent: jest.fn(), + })); + api.getMapFiles.mockResolvedValue({files: []}); + api.fetchMapSubscriptions.mockResolvedValue({catalog: [], subscriptions: []}); + api.getMapPins.mockResolvedValue({pins: []}); +}); + +describe('the Pins tab', () => { + test('pending: shows a placeholder, not "No pins"', () => { + api.getMapPins.mockReturnValue(pending()); + const {container} = renderAt('/map/pins'); + expect(container.querySelector('.wrolpi-placeholder')).toBeInTheDocument(); + expect(screen.queryAllByText(NO_PINS)).toHaveLength(0); + }); + + test('failed: shows an error, not "No pins"', async () => { + // The api helper returns undefined on a non-OK response. + api.getMapPins.mockResolvedValue(undefined); + renderAt('/map/pins'); + await act(async () => { + }); + expect(screen.getByText(/Could not fetch map pins/)).toBeInTheDocument(); + expect(screen.queryAllByText(NO_PINS)).toHaveLength(0); + }); + + test('empty: shows "No pins"', async () => { + renderAt('/map/pins'); + await act(async () => { + }); + expect(screen.getAllByText(NO_PINS).length).toBeGreaterThan(0); + }); + + test('loaded: lists the pins', async () => { + api.getMapPins.mockResolvedValue({pins: [{id: 1, lat: 40.1, lon: -111.2, label: 'Camp', color: '#f00'}]}); + renderAt('/map/pins'); + await act(async () => { + }); + expect(screen.getAllByText('Camp').length).toBeGreaterThan(0); + expect(screen.queryAllByText(NO_PINS)).toHaveLength(0); + }); +}); + +describe('the Manage tab subscriptions table', () => { + test('pending: shows a placeholder, not "No results"', async () => { + api.fetchMapSubscriptions.mockReturnValue(pending()); + const {container} = renderAt('/map/manage'); + await act(async () => { + }); + // The files table has loaded (empty); the subscriptions table is still pending. + expect(container.querySelector('.wrolpi-placeholder')).toBeInTheDocument(); + expect(screen.queryByText(NO_RESULTS)).not.toBeInTheDocument(); + }); + + test('failed: shows an error, not "No results"', async () => { + api.fetchMapSubscriptions.mockResolvedValue(undefined); + renderAt('/map/manage'); + await act(async () => { + }); + expect(screen.getByText(/Could not fetch map subscriptions/)).toBeInTheDocument(); + expect(screen.queryByText(NO_RESULTS)).not.toBeInTheDocument(); + }); + + test('empty: shows "No results"', async () => { + renderAt('/map/manage'); + await act(async () => { + }); + expect(screen.getByText(NO_RESULTS)).toBeInTheDocument(); + }); + + // The "View All Regions" preview draws the catalog; a pending or failed catalog must not be + // previewed as zero regions. + test('pending: the regions preview is disabled', async () => { + api.fetchMapSubscriptions.mockReturnValue(pending()); + renderAt('/map/manage'); + await act(async () => { + }); + expect(screen.getByRole('button', {name: 'View All Regions'})).toBeDisabled(); + }); + + test('failed: the regions preview is disabled', async () => { + api.fetchMapSubscriptions.mockResolvedValue(undefined); + renderAt('/map/manage'); + await act(async () => { + }); + expect(screen.getByRole('button', {name: 'View All Regions'})).toBeDisabled(); + }); + + test('loaded: the regions preview is enabled', async () => { + renderAt('/map/manage'); + await act(async () => { + }); + expect(screen.getByRole('button', {name: 'View All Regions'})).toBeEnabled(); + }); +});