fix: isolate per-analyzer failures so one bad analyzer can't empty the Analyzers list (ERA-13536) - #1624
Conversation
…e list (ERA-13536) fetchAnalyzers built the analyzer list inside a single Promise.all, so a throw while processing any one analyzer rejected the whole batch and the Analyzers sidebar list rendered completely empty. Wrap each analyzer's processing in try/catch: on error, log a console.warn naming the analyzer and skip it (resolve null) instead of failing the entire fetch, and filter the nulls before the existing empty-features check. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR fault-isolates per-analyzer processing inside fetchAnalyzers so that a single analyzer with invalid spatial-group geometry/threshold configuration cannot cause the entire analyzers fetch to fail and leave the Analyzers sidebar empty.
Changes:
- Wrap per-analyzer async processing in
try/catch, logging a warning and returningnullon failure instead of rejecting the sharedPromise.all. - Filter out failed (
null) analyzers before dispatchingFETCH_ANALYZERS_SUCCESS. - Add an MSW-backed Jest test that reproduces the real-world negative-buffer proximity failure and asserts other analyzers still dispatch plus a single
console.warn.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
src/ducks/analyzers.js |
Prevents one analyzer-processing exception from rejecting the whole analyzers fetch; filters out failed analyzers before dispatch. |
src/ducks/analyzers.test.js |
Adds regression coverage ensuring valid analyzers still dispatch when one analyzer throws, and that a warning is logged for the failed analyzer. |
luixlive
left a comment
luixlive
left a comment
There was a problem hiding this comment.
I don't see a hard-blocker, but a few small improvements so I'll wait for a second pass to approve it.
Also, I know this fetcher wasn't implemented in this PR, but in the team we have this notion of the "Boy Scout Rule": Leave the campground cleaner than you found it. I think we could ask Claude to refactor the fetchAnalyzers action creator, since it's a giant block of hard-to-read code. We could define better variable names, break the logic with some helper methods, and probably even optimize the code!
| if (analyzer.analyzer_category === 'proximity') { | ||
| // isolate per-analyzer processing so one bad analyzer can't reject the | ||
| // whole Promise.all and wipe out the entire analyzer list | ||
| try { |
There was a problem hiding this comment.
Claude:
Try/catch isolates failures per-analyzer, not per spatial-group link, so one bad link still discards sibling links' valid geometry for that analyzer.
| // flatten the feature array - | ||
| const features = [].concat(...fetchedFeatures); | ||
| return { id: analyzer.id, name: analyzer.name, type: analyzer.analyzer_category, geojson: featureCollection(features) }; | ||
| } catch (error) { |
There was a problem hiding this comment.
Claude:
The new catch-all has no isCancel() guard, so a benign axios request cancellation is misreported as an analyzer failure.
| @@ -20,41 +20,48 @@ export const fetchAnalyzers = () => async (dispatch) => { | |||
| const { data: { data } } = await axios.get(ANALYZERS_API_URL, { params: { active: true } }); | |||
|
|
|||
| const analyzers = await Promise.all(data.map(async (analyzer) => { | |||
There was a problem hiding this comment.
This approach can be simplified using Promise.allSettled instead of a Promise.all with a manual implementation of error handling 😄
| name: 'Tourist Road Proximity - Line', | ||
| analyzer_category: 'proximity', | ||
| admin_href: 'http://localhost/admin/bad-proximity', | ||
| threshold_dist_meters: -1.0, |
There was a problem hiding this comment.
Claude:
The 'bad proximity' test fixture relies on an unasserted, version-specific turf.buffer() quirk (returning undefined for a negative buffer on a LineString) instead of an explicit, documented failure contract.
What
Fault-isolates per-analyzer processing in
fetchAnalyzersso a single failing analyzer can no longer wipe out the entire Analyzers list.Jira: ERA-13536
Why
fetchAnalyzersbuilds the analyzer list inside onePromise.all(data.map(async (analyzer) => {...})). If processing any one analyzer throws, the wholePromise.allrejects,FETCH_ANALYZERS_SUCCESSis never dispatched, and the Analyzers sidebar list stays at its empty initial state — with no user-facing error.This is happening on wildlifeactdev.pamdas.org: a proximity analyzer ("Tourist Road Proximity - Line") is configured with
threshold_dist_meters: -1.0over a line geometry. The proximity branch buffers the feature bythreshold / 1000km; negative-buffering a line returnsundefinedfrom turf, soproximityPoly.geometrythrows aTypeError— and that single bad analyzer made every analyzer (KPR Geofence Analyzer included) disappear from the list.Change
try/catch. On error,console.warn(name + id + error) and returnnullinstead of rejecting the sharedPromise.all.null(failed) analyzers before the existing empty-features check.threshold_dist_metersis a tracked follow-up, as is backend/admin validation.Tests
Added
src/ducks/analyzers.test.js(MSW +setupServer, matching the repo's duck-test pattern). It reproduces the real-world failure (a valid geofence, a valid proximity, and a negative-buffer line proximity) and asserts:console.warnfires naming the failed analyzer.🤖 Generated with Claude Code