fix: spurious 'Invalid layer identifier' for dynamically registered OpenSPP collection layers#1
Conversation
The invalid-layer check in layers-utils snapshotted LayerDefinitions keys once at mount, before OpenSPP collections are fetched and registered via registerDynamicLayer. Selecting a collection from the dynamic Collections browser then raised a spurious 'Invalid layer identifier(s)' notification even though the layer itself rendered correctly. Validate against the live LayerDefinitions map at validation time instead, via a getInvalidLayerIds helper next to registerDynamicLayer.
There was a problem hiding this comment.
Code Review
This pull request introduces the getInvalidLayerIds helper function to validate layer IDs against the live LayerDefinitions map, ensuring that dynamically registered layers are correctly recognized. The useLayers hook is updated to use this new helper, and comprehensive unit tests are added. Feedback suggests replacing the in operator with Object.prototype.hasOwnProperty.call to prevent potential issues with built-in prototype properties when processing untrusted URL inputs.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| * registerDynamicLayer are recognized as valid. | ||
| */ | ||
| export function getInvalidLayerIds(layerIds: string[]): string[] { | ||
| return layerIds.filter(layerId => !(layerId in LayerDefinitions)); |
There was a problem hiding this comment.
Using the in operator to check for keys in LayerDefinitions can be problematic if the layerId matches a built-in property on Object.prototype (e.g., toString, constructor, hasOwnProperty). Since layerIds are parsed from URL parameters, which are untrusted user inputs, it is safer to use Object.prototype.hasOwnProperty.call to ensure we only check for direct properties of the LayerDefinitions object.
| return layerIds.filter(layerId => !(layerId in LayerDefinitions)); | |
| return layerIds.filter(layerId => !Object.prototype.hasOwnProperty.call(LayerDefinitions, layerId)); |
Problem
Selecting a collection from the OpenSPP Reports > Collections browser shows an error notification —
Invalid layer identifier(s): openspp_MIS_DEMO_BENEFICIARY_DIST_adm0— even though the choropleth renders correctly.Root cause
layers-utils.tsxvalidated URL layer ids against a key list snapshotted once at mount:OpenSPP collections are fetched asynchronously and injected into
LayerDefinitionslater viaregisterDynamicLayer()(RootAccordionItems/index.tsx), so theiropenspp_<collectionId>ids are never in the frozen snapshot. The rendering pipeline reads the live map (works); only the validator used the stale copy (spurious error).Fix
Validate against the live
LayerDefinitionsmap at validation time, via agetInvalidLayerIdshelper placed next toregisterDynamicLayerinconfig/utils.ts. SinceregisterDynamicLayermutates a module-level object (invisible to React reactivity), memoizing its keys can never be correct — the check must read the object when it runs. TheinvalidLayersIdsmemo recomputes on everyurlLayerIdschange (i.e., whenever a layer is toggled), which happens strictly after registration in the click flow.Known limitation (pre-existing, out of scope)
Opening a shared URL that references a dynamic collection layer still races the async collections fetch: validation runs before
registerDynamicLayer, and nothing re-triggers it after registration. Fixing that needs registration to become reactive (e.g., a redux-tracked registry version) — a larger change than this notification bug warrants.Testing
frontend/src/config/dynamic-layers.test.ts(4 cases — unknown id invalid, static id valid, runtime-registered id valid, mixed list) written red first, green after the fix.yarn tsc --noEmitand eslint on touched files: identical to base-branch baseline (3 pre-existing TS errors in untouched files, 1 pre-existing prettier warning).Found during the QGIS/PRISM ↔ OpenSPP integration test session (baseline for the OpenSPP2#281 OGC migration).