feat: GPS tab enhancements #32
Merged
akkaouim merged 3 commits intojjackson:labs-mainfrom Mar 2, 2026
Merged
Conversation
…atio, aggregate map - Make all GPS tab columns sortable (With GPS, Unique Cases, Max Revisit Dist.) - Rename columns to match Overview tab (Avg Case Dist → Revisit Dist., Max Case Dist → Max Revisit Dist.) - Add cases_with_revisits denominator "(N)" to Revisit Dist. in both GPS and Overview tabs - Add Meter/Visit column to GPS tab (with color coding) - Add Dist. Ratio column to both GPS and Overview tabs (revisit dist / meter per visit) - Add collapsible aggregate map showing all FLW visits with per-user HSL color coding and MarkerCluster - Make GPS drill-down visit table sortable (Date, Form, Entity, Revisit Dist., Status) - Fix sticky header displacement when aggregate map or drill-down expands - Extract all_coordinates in views.py for aggregate map data - Update Guide tab, DOCUMENTATION.md, and DASHBOARD_GUIDE.md Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
📝 WalkthroughWalkthroughAdds per‑FLW revisit counts, median meters‑per‑visit, a Dist. Ratio metric, renamed revisit distance fields, an aggregate "All Visits" GPS map, gps_location fallback logic, and wires these into analysis, views, serializers, templates, and docs. Changes
Sequence Diagram(s)sequenceDiagram
participant Views as Views (views.py)
participant GA as GPS Analysis (gps_analysis.py)
participant Ser as Serializers (serializers.py)
participant UI as Frontend (template.py)
participant Map as Aggregate Map UI
Views->>GA: send visits for analysis
GA->>GA: compute per‑FLW metrics (revisit distances, median_meters_per_visit, cases_with_revisits)
GA-->>Views: return FLWSummary objects with new fields
Views->>Views: extract all_coordinates from visits
Views->>Ser: hand off enhanced gps_data + flw_summaries
Ser-->>UI: JSON payload (includes cases_with_revisits, median_meters_per_visit, all_coordinates)
UI->>UI: compute dist_ratio for items, update gpsDetailSort state
UI->>Map: render aggregate map using all_coordinates and per‑FLW legend
Map->>Map: display color‑coded pins and clusters
UI->>UI: render tables with new Dist. Ratio column and sorting
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@commcare_connect/workflow/templates/mbw_monitoring/template.py`:
- Around line 577-583: The effect in React.useEffect accesses
gpsData.all_coordinates without guarding for gpsData being undefined, which can
throw during refresh/loading transitions; update the React.useEffect block (and
the similar usages around the gpsData references at the other spots mentioned)
to first check gpsData exists (or use a safe default like const coords =
(gpsData && gpsData.all_coordinates) || []) before using it, and ensure any
other reads of gpsData (the ones near lines referencing gpsData at 623-625 and
648) use the same guard or optional chaining so aggregateMapRef.current and
rendering logic do not crash when gpsData is undefined.
- Around line 630-635: The popup currently interpolates unescaped values into
HTML in the marker.bindPopup call (flwName, c.e, c.d), creating an XSS risk; fix
by escaping these data fields before composing the HTML (e.g., add and use a
small helper like escapeHtml(value) and apply it to flwName, c.e and c.d) or
build the popup using DOM text nodes instead of raw string concatenation, then
pass the safe string/element to marker.bindPopup so only the intentional flagged
span remains unescaped.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
commcare_connect/workflow/templates/mbw_monitoring/DASHBOARD_GUIDE.mdcommcare_connect/workflow/templates/mbw_monitoring/DOCUMENTATION.mdcommcare_connect/workflow/templates/mbw_monitoring/gps_analysis.pycommcare_connect/workflow/templates/mbw_monitoring/serializers.pycommcare_connect/workflow/templates/mbw_monitoring/template.pycommcare_connect/workflow/templates/mbw_monitoring/views.py
- Add null guard for gpsData.all_coordinates in aggregate map useEffect (prevents crash during refresh/loading transitions) - Add escapeHtml helper and apply to all three bindPopup calls (aggregate map, drill-down visit markers, drill-down mother markers) to prevent XSS from entity names, dates, or FLW display names Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
commcare_connect/workflow/templates/mbw_monitoring/template.py (1)
578-648:⚠️ Potential issue | 🟠 MajorTear down/recreate the aggregate map when its container disappears.
Line 583 and Line 586 can return without clearing
aggregateMapRef.current, and Line 648 does not track tab changes. This can leave Leaflet bound to a detached DOM node after refresh/tab-switch and break map rendering when returning.💡 Suggested fix
React.useEffect(function() { - if (!leafletReady || !showAggregateMap) { + var shouldShow = leafletReady && showAggregateMap && activeTab === 'gps'; + if (!shouldShow) { if (aggregateMapRef.current) { aggregateMapRef.current.remove(); aggregateMapRef.current = null; } + aggregateMarkersRef.current = null; return; } var coords = (gpsData && gpsData.all_coordinates) || []; - if (coords.length === 0) return; + if (coords.length === 0) { + if (aggregateMapRef.current) { aggregateMapRef.current.remove(); aggregateMapRef.current = null; } + aggregateMarkersRef.current = null; + return; + } var mapDiv = document.getElementById('aggregate-gps-map'); - if (!mapDiv) return; + if (!mapDiv) { + if (aggregateMapRef.current) { aggregateMapRef.current.remove(); aggregateMapRef.current = null; } + aggregateMarkersRef.current = null; + return; + } @@ -}, [leafletReady, showAggregateMap, gpsData, filterFlws]); +}, [leafletReady, showAggregateMap, activeTab, gpsData, filterFlws]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@commcare_connect/workflow/templates/mbw_monitoring/template.py` around lines 578 - 648, The effect can early-return without removing the Leaflet map, leaving aggregateMapRef.current bound to a detached DOM node; before every early return (the checks using leafletReady || showAggregateMap, coords.length === 0, and !mapDiv) ensure you call the same teardown used at the end: if (aggregateMarkersRef.current && aggregateMapRef.current) remove the layer and set aggregateMarkersRef.current = null; if (aggregateMapRef.current) call aggregateMapRef.current.remove() and set aggregateMapRef.current = null. Also ensure the effect's dependency array includes the tab/state that controls visibility (e.g., showAggregateMap or the parent tab identifier) so the cleanup runs on tab changes; update the effect dependencies (currently [leafletReady, showAggregateMap, gpsData, filterFlws]) to include any missing tab variable used to show/hide the map.
🧹 Nitpick comments (1)
commcare_connect/workflow/templates/mbw_monitoring/template.py (1)
1663-1673: Avoid mutating state-derived rows while computingdist_ratio.Line 1664 and Line 1669 mutate objects coming from
dashData-derived arrays during render. Prefer immutable derivation to avoid side effects and stale cross-view behavior.♻️ Suggested refactor
-// Compute dist_ratio for GPS and Overview (revisit_distance_km * 1000 / median_meters_per_visit) -filteredGpsFlws.forEach(function(g) { - g.dist_ratio = (g.avg_case_distance_km != null && g.median_meters_per_visit != null && g.median_meters_per_visit > 0) - ? Math.round(g.avg_case_distance_km * 1000 / g.median_meters_per_visit * 10) / 10 - : null; -}); -filteredOverview.forEach(function(f) { - f.dist_ratio = (f.revisit_distance_km != null && f.median_meters_per_visit != null && f.median_meters_per_visit > 0) - ? Math.round(f.revisit_distance_km * 1000 / f.median_meters_per_visit * 10) / 10 - : null; -}); +// Compute dist_ratio for GPS and Overview (revisit_distance_km * 1000 / median_meters_per_visit) +filteredGpsFlws = filteredGpsFlws.map(function(g) { + return Object.assign({}, g, { + dist_ratio: (g.avg_case_distance_km != null && g.median_meters_per_visit != null && g.median_meters_per_visit > 0) + ? Math.round(g.avg_case_distance_km * 1000 / g.median_meters_per_visit * 10) / 10 + : null + }); +}); +filteredOverview = filteredOverview.map(function(f) { + return Object.assign({}, f, { + dist_ratio: (f.revisit_distance_km != null && f.median_meters_per_visit != null && f.median_meters_per_visit > 0) + ? Math.round(f.revisit_distance_km * 1000 / f.median_meters_per_visit * 10) / 10 + : null + }); +});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@commcare_connect/workflow/templates/mbw_monitoring/template.py` around lines 1663 - 1673, The code mutates dashData-derived rows by assigning g.dist_ratio and f.dist_ratio inside filteredGpsFlws.forEach and filteredOverview.forEach; instead create new arrays (e.g., use filteredGpsFlws = filteredGpsFlws.map(g => ({...g, dist_ratio: computedValue})) and similarly for filteredOverview) so you derive dist_ratio immutably without mutating the original objects; keep the exact computation (use avg_case_distance_km and revisit_distance_km with median_meters_per_visit checks) but return new objects rather than assigning to g.dist_ratio or f.dist_ratio.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@commcare_connect/workflow/templates/mbw_monitoring/template.py`:
- Around line 578-648: The effect can early-return without removing the Leaflet
map, leaving aggregateMapRef.current bound to a detached DOM node; before every
early return (the checks using leafletReady || showAggregateMap, coords.length
=== 0, and !mapDiv) ensure you call the same teardown used at the end: if
(aggregateMarkersRef.current && aggregateMapRef.current) remove the layer and
set aggregateMarkersRef.current = null; if (aggregateMapRef.current) call
aggregateMapRef.current.remove() and set aggregateMapRef.current = null. Also
ensure the effect's dependency array includes the tab/state that controls
visibility (e.g., showAggregateMap or the parent tab identifier) so the cleanup
runs on tab changes; update the effect dependencies (currently [leafletReady,
showAggregateMap, gpsData, filterFlws]) to include any missing tab variable used
to show/hide the map.
---
Nitpick comments:
In `@commcare_connect/workflow/templates/mbw_monitoring/template.py`:
- Around line 1663-1673: The code mutates dashData-derived rows by assigning
g.dist_ratio and f.dist_ratio inside filteredGpsFlws.forEach and
filteredOverview.forEach; instead create new arrays (e.g., use filteredGpsFlws =
filteredGpsFlws.map(g => ({...g, dist_ratio: computedValue})) and similarly for
filteredOverview) so you derive dist_ratio immutably without mutating the
original objects; keep the exact computation (use avg_case_distance_km and
revisit_distance_km with median_meters_per_visit checks) but return new objects
rather than assigning to g.dist_ratio or f.dist_ratio.
…t_ratio - Extract teardown() helper in aggregate map useEffect and call it on all three early-return paths (leaflet not ready, no coordinates, no map div) to prevent detached DOM node leaks - Add activeTab to aggregate map useEffect deps so cleanup runs on tab switch - Use cleanup ref (aggregateMapRef.current) instead of local var in return fn - Replace mutating forEach with immutable .map + Object.assign for dist_ratio computation on filteredGpsFlws and filteredOverview Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@commcare_connect/workflow/templates/mbw_monitoring/template.py`:
- Around line 628-635: The loop in activeCoords.forEach computes flwName by
scanning gpsFlws for each point (and similarly elsewhere), causing O(points ×
flws) cost; to fix, build a one-time lookup map from gpsFlws keyed by username
(e.g., flwLookup = {}; gpsFlws.forEach(g => flwLookup[g.username] =
g.display_name || g.username)) and then replace the inner linear scan in
activeCoords.forEach (and the other occurrence) to do a constant-time
flwLookup[c.u] lookup for flwName, preserving existing fallbacks and reusing
flwColorMap as before.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
— sortable columns, aligned names, Dist. Ratio, aggregate map
Summary by CodeRabbit
New Features
Documentation
Bug Fixes