From c6480b01e43563a288f56253cffc6ee64de3baa2 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 24 Sep 2026 09:11:46 +0000 Subject: [PATCH] fix: patch four correctness bugs found in state/plugin/API layer (rebased on main) - PluginManager: fix a race where disabling a layer while enablePlugin() was still resolving its awaits could restart polling and re-emit layerToggled(true), leaking a background fetch loop and a stale websocket subscription for a layer the UI showed as disabled. Fixed with a per-plugin toggle epoch that lets an in-flight enablePlugin detect it was superseded by a later toggle. - data-query service / region API route: clamp `limit` with a lower bound of 1 (matching searchEntities' existing clamp), so a negative limit can no longer produce a negative Array.slice bound and return an incorrect entity set. - favoritesSlice: revert the optimistic add/remove on a failed POST/DELETE and surface an error toast, following the same optimistic-flip-with-rollback pattern already used in alertsSlice. - dataSlice.getAllEntities: filter by the layer's enabled flag to match its own docstring and PluginManager.getAllEntities' semantics. Fix: chore/refactor+fix mix -> patch bump per project convention. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01FyQHTVUZs5PzRkYJa7Tq4h --- package.json | 2 +- src/app/api/v1/entities/region/route.ts | 2 +- src/core/plugins/PluginManager.test.ts | 17 +++++ src/core/plugins/PluginManager.ts | 22 ++++++ src/core/state/dataSlice.test.ts | 32 ++++++++- src/core/state/dataSlice.ts | 4 +- src/core/state/favoritesSlice.cloud.test.ts | 77 +++++++++++++++++++++ src/core/state/favoritesSlice.ts | 21 +++++- src/lib/data-query/service.test.ts | 17 +++++ src/lib/data-query/service.ts | 2 +- 10 files changed, 188 insertions(+), 8 deletions(-) create mode 100644 src/core/state/favoritesSlice.cloud.test.ts diff --git a/package.json b/package.json index cb8b80da..4692d171 100644 --- a/package.json +++ b/package.json @@ -1,7 +1,7 @@ { "name": "worldwideview", - "version": "2.68.25", + "version": "2.68.26", "private": true, "license": "EL-2.0", diff --git a/src/app/api/v1/entities/region/route.ts b/src/app/api/v1/entities/region/route.ts index d7895c93..4b7c2c38 100644 --- a/src/app/api/v1/entities/region/route.ts +++ b/src/app/api/v1/entities/region/route.ts @@ -62,7 +62,7 @@ export async function GET(request: NextRequest) { const pluginId = searchParams.get("pluginId") ?? undefined; const rawLimit = searchParams.get("limit"); const parsedLimit = rawLimit !== null ? parseInt(rawLimit, 10) : NaN; - const limit = Math.min(Number.isNaN(parsedLimit) ? 100 : parsedLimit, 1000); + const limit = Math.min(Math.max(Number.isNaN(parsedLimit) ? 100 : parsedLimit, 1), 1000); try { const result = await getEntitiesInRegion({ north, south, east, west, pluginId, limit }); diff --git a/src/core/plugins/PluginManager.test.ts b/src/core/plugins/PluginManager.test.ts index 1726b937..2ddf1f18 100644 --- a/src/core/plugins/PluginManager.test.ts +++ b/src/core/plugins/PluginManager.test.ts @@ -150,6 +150,23 @@ describe("PluginManager.enablePlugin", () => { }); }); +describe("PluginManager — enable/disable race", () => { + it("does not restart polling or re-announce enabled if disabled while enablePlugin was still awaiting", async () => { + const layerHandler = vi.fn(); + await pluginManager.registerPlugin(makePlugin({ id: "race-1" })); + + const enabling = pluginManager.enablePlugin("race-1"); + // Fast toggle-off before the enable's internal awaits resolve. + pluginManager.disablePlugin("race-1"); + dataBus.on("layerToggled", layerHandler); + + await enabling; + + expect(pluginManager.getPlugin("race-1")?.enabled).toBe(false); + expect(layerHandler).not.toHaveBeenCalledWith({ pluginId: "race-1", enabled: true }); + }); +}); + describe("PluginManager.disablePlugin", () => { it("clears entities and emits layerToggled(false) + dataUpdated([])", async () => { const layerHandler = vi.fn(); diff --git a/src/core/plugins/PluginManager.ts b/src/core/plugins/PluginManager.ts index 6e18bbac..936f2228 100644 --- a/src/core/plugins/PluginManager.ts +++ b/src/core/plugins/PluginManager.ts @@ -40,6 +40,11 @@ class PluginManager { private loadedManifests: Map = new Map(); private initialized = false; private configCacheMaxAge = 3600000; + // Bumped by every enablePlugin/disablePlugin call for a given plugin so + // an in-flight enablePlugin can detect it was superseded by a later + // toggle (e.g. a fast enable->disable) after resuming from an await, + // and bail out instead of re-enabling a plugin the user just disabled. + private toggleEpoch: Map = new Map(); /** * Initializes the PluginManager and prepares the persistent cache layer. @@ -216,10 +221,21 @@ class PluginManager { async enablePlugin(pluginId: string): Promise { const start = performance.now(); console.debug(`[PluginManager] enablePlugin called for ${pluginId}`); + const epoch = (this.toggleEpoch.get(pluginId) ?? 0) + 1; + this.toggleEpoch.set(pluginId, epoch); + // Ensure local manifest is fetched so we don't accidentally fall back to cloud if toggled too fast await fetchLocalEngineManifest(); console.debug(`[PluginManager] Manifest fetched for ${pluginId}. Took ${(performance.now() - start).toFixed(2)}ms`); + // A disablePlugin() (or a newer enablePlugin()) call may have landed + // while we were awaiting above (e.g. the user toggled the layer off + // before this resolved). Bail out so we don't re-enable a plugin the + // user just disabled, or clobber a newer toggle's state. + if (this.toggleEpoch.get(pluginId) !== epoch) { + return; + } + const managed = this.plugins.get(pluginId); if (!managed) { console.error(`[PluginManager] Plugin ${pluginId} not found in managed plugins`); @@ -236,6 +252,11 @@ class PluginManager { cached = await cacheLayer.getFromPersistent(pluginId); } + // Re-check after the second await for the same reason as above. + if (this.toggleEpoch.get(pluginId) !== epoch) { + return; + } + // If still enabled and we got cached data, emit it if (cached && managed.enabled) { managed.entities = cached; @@ -256,6 +277,7 @@ class PluginManager { */ disablePlugin(pluginId: string): void { console.debug(`[PluginManager] disablePlugin called for ${pluginId}`); + this.toggleEpoch.set(pluginId, (this.toggleEpoch.get(pluginId) ?? 0) + 1); const managed = this.plugins.get(pluginId); if (!managed) { console.error(`[PluginManager] Plugin ${pluginId} not found during disable`); diff --git a/src/core/state/dataSlice.test.ts b/src/core/state/dataSlice.test.ts index 4f68b37e..eda100c2 100644 --- a/src/core/state/dataSlice.test.ts +++ b/src/core/state/dataSlice.test.ts @@ -6,9 +6,11 @@ import { createStore, type StoreApi } from 'zustand/vanilla'; import type { GeoEntity } from '@/core/plugins/PluginTypes'; import { createDataSlice, DataSlice } from './dataSlice'; -// We need a mock store that includes selectedEntity to test the cross-slice update behavior +// We need a mock store that includes selectedEntity and layers to test the +// cross-slice update behavior and getAllEntities' enabled-flag filtering. interface MockAppStore extends DataSlice { selectedEntity: GeoEntity | null; + layers: Record; } describe('dataSlice', () => { @@ -20,6 +22,7 @@ describe('dataSlice', () => { return { ...dataSlice, selectedEntity: null, + layers: {}, }; }); }); @@ -70,10 +73,16 @@ describe('dataSlice', () => { expect(store.getState().entitiesByPlugin['plugin-b']).toEqual(mockEntitiesB); }); - it('gets all entities across plugins flattened', () => { + it('gets all entities across enabled plugins flattened', () => { const mockEntitiesA = [{ id: 'e1', pluginId: 'plugin-a' } as GeoEntity]; const mockEntitiesB = [{ id: 'e2', pluginId: 'plugin-b' } as GeoEntity]; + store.setState({ + layers: { + 'plugin-a': { enabled: true }, + 'plugin-b': { enabled: true }, + }, + }); store.getState().setEntities('plugin-a', mockEntitiesA); store.getState().setEntities('plugin-b', mockEntitiesB); @@ -83,4 +92,23 @@ describe('dataSlice', () => { expect(all).toContainEqual(mockEntitiesA[0]); expect(all).toContainEqual(mockEntitiesB[0]); }); + + it('excludes entities from disabled plugins', () => { + const mockEntitiesA = [{ id: 'e1', pluginId: 'plugin-a' } as GeoEntity]; + const mockEntitiesB = [{ id: 'e2', pluginId: 'plugin-b' } as GeoEntity]; + + store.setState({ + layers: { + 'plugin-a': { enabled: true }, + 'plugin-b': { enabled: false }, + }, + }); + store.getState().setEntities('plugin-a', mockEntitiesA); + store.getState().setEntities('plugin-b', mockEntitiesB); + + const all = store.getState().getAllEntities(); + + expect(all).toHaveLength(1); + expect(all).toContainEqual(mockEntitiesA[0]); + }); }); diff --git a/src/core/state/dataSlice.ts b/src/core/state/dataSlice.ts index da92fd0b..980fc5f1 100644 --- a/src/core/state/dataSlice.ts +++ b/src/core/state/dataSlice.ts @@ -42,6 +42,8 @@ export const createDataSlice: StateCreator = (set, }), getAllEntities: () => { const state = get(); - return Object.values(state.entitiesByPlugin).flat(); + return Object.entries(state.entitiesByPlugin) + .filter(([pluginId]) => state.layers?.[pluginId]?.enabled) + .flatMap(([, entities]) => entities); }, }); diff --git a/src/core/state/favoritesSlice.cloud.test.ts b/src/core/state/favoritesSlice.cloud.test.ts new file mode 100644 index 00000000..00ba55b6 --- /dev/null +++ b/src/core/state/favoritesSlice.cloud.test.ts @@ -0,0 +1,77 @@ +/* eslint-disable @typescript-eslint/no-explicit-any */ +import { + describe, it, expect, beforeEach, vi, afterEach +} from 'vitest'; +import { createStore, type StoreApi } from 'zustand/vanilla'; +import type { GeoEntity } from '@/core/plugins/PluginTypes'; +import { createFavoritesSlice, FavoritesSlice } from './favoritesSlice'; + +// isDemo=false so addFavorite/removeFavorite exercise the fetch (cloud/local +// edition) branch instead of the cookie branch. +vi.mock('@/core/edition', () => ({ + isDemo: false, +})); + +interface MockAppStore extends FavoritesSlice { + showErrorToast?: (message: string) => void; +} + +describe('favoritesSlice (cloud mode) — optimistic update rollback', () => { + let store: StoreApi; + let showErrorToast: ReturnType void>>; + + beforeEach(() => { + showErrorToast = vi.fn<(message: string) => void>(); + store = createStore((set, get, api) => ({ + ...createFavoritesSlice(set as any, get as any, api as any), + showErrorToast, + })); + }); + + afterEach(() => { + vi.restoreAllMocks(); + vi.unstubAllGlobals(); + }); + + const mockEntity = { id: 'entity-1', pluginId: 'plugin-a', label: 'Test Entity' } as GeoEntity; + + it('reverts the optimistic add and surfaces an error when the POST fails', async () => { + vi.stubGlobal('fetch', vi.fn().mockResolvedValue({ ok: false, status: 500 })); + + store.getState().addFavorite(mockEntity, 'Plugin A'); + expect(store.getState().favorites).toHaveLength(1); + + // Let the fetch promise's .then/.catch chain settle. + await vi.waitFor(() => { + expect(store.getState().favorites).toHaveLength(0); + }); + expect(showErrorToast).toHaveBeenCalled(); + }); + + it('keeps the optimistic add when the POST succeeds', async () => { + vi.stubGlobal('fetch', vi.fn().mockResolvedValue({ ok: true, status: 200 })); + + store.getState().addFavorite(mockEntity, 'Plugin A'); + await Promise.resolve(); + await Promise.resolve(); + + expect(store.getState().favorites).toHaveLength(1); + expect(showErrorToast).not.toHaveBeenCalled(); + }); + + it('reverts the optimistic remove and surfaces an error when the DELETE fails', async () => { + vi.stubGlobal('fetch', vi.fn().mockResolvedValue({ ok: true, status: 200 })); + store.getState().initFavorites([{ + id: 'entity-1', pluginId: 'plugin-a', label: 'Test Entity', pluginName: 'Plugin A', lastSeen: 1, + }]); + + vi.stubGlobal('fetch', vi.fn().mockResolvedValue({ ok: false, status: 500 })); + store.getState().removeFavorite('entity-1'); + expect(store.getState().favorites).toHaveLength(0); + + await vi.waitFor(() => { + expect(store.getState().favorites).toHaveLength(1); + }); + expect(showErrorToast).toHaveBeenCalled(); + }); +}); diff --git a/src/core/state/favoritesSlice.ts b/src/core/state/favoritesSlice.ts index 75ceef48..4bda4de1 100644 --- a/src/core/state/favoritesSlice.ts +++ b/src/core/state/favoritesSlice.ts @@ -85,12 +85,20 @@ export const createFavoritesSlice: StateCreator console.error("Failed to sync add favorite to DB:", e)); + }).then((res) => { + if (!res.ok) throw new Error(`Request failed with status ${res.status}`); + }).catch((e) => { + console.error("Failed to sync add favorite to DB:", e); + // Revert the optimistic update so UI state matches the backend. + set((s) => ({ favorites: s.favorites.filter((f) => f.id !== entity.id) })); + get().showErrorToast?.("Failed to save favorite. Please try again."); + }); } }, removeFavorite: (id) => { const state = get(); + const removed = state.favorites.find((f) => f.id === id); const newFavorites = state.favorites.filter((f) => f.id !== id); set({ favorites: newFavorites }); @@ -99,7 +107,16 @@ export const createFavoritesSlice: StateCreator console.error("Failed to sync remove favorite from DB:", e)); + }).then((res) => { + if (!res.ok) throw new Error(`Request failed with status ${res.status}`); + }).catch((e) => { + console.error("Failed to sync remove favorite from DB:", e); + // Revert the optimistic update so UI state matches the backend. + if (removed) { + set((s) => (s.favorites.some((f) => f.id === id) ? {} : { favorites: [...s.favorites, removed] })); + } + get().showErrorToast?.("Failed to remove favorite. Please try again."); + }); } }, }); diff --git a/src/lib/data-query/service.test.ts b/src/lib/data-query/service.test.ts index 75692e1c..b467571b 100644 --- a/src/lib/data-query/service.test.ts +++ b/src/lib/data-query/service.test.ts @@ -335,6 +335,23 @@ describe("getEntitiesInRegion", () => { expect(result.entities[0].id).toBe("e1"); }); + it("clamps a negative limit to the floor of 1 instead of producing a negative slice bound", async () => { + const entities = [ + makeEntity({ id: "e1", latitude: 51.5, longitude: -0.1, pluginId: "test-plugin" }), + makeEntity({ id: "e2", latitude: 51.6, longitude: -0.2, pluginId: "test-plugin" }), + ]; + mockEngineSnapshot("test-plugin", entities); + const result = await getEntitiesInRegion({ + north: 52, + south: 50, + east: 1, + west: -1, + pluginId: "test-plugin", + limit: -5, + }); + expect(result.entities).toHaveLength(1); + }); + it("respects pluginId filter", async () => { const entity = makeEntity({ id: "e1", latitude: 51.5, longitude: -0.1, pluginId: "specific-plugin" }); mockEngineSnapshot("specific-plugin", [entity]); diff --git a/src/lib/data-query/service.ts b/src/lib/data-query/service.ts index db8a9f8e..cd096623 100644 --- a/src/lib/data-query/service.ts +++ b/src/lib/data-query/service.ts @@ -250,7 +250,7 @@ export async function getEntitiesInRegion(bounds: RegionOptions): Promise