feat: MBW Monitoring Dashboard v3 — GPS, streaming, performance & approval filters#33
Merged
akkaouim merged 6 commits intojjackson:labs-mainfrom Mar 2, 2026
Merged
Conversation
Add server-side filtering by Connect visit approval status (Approved, Pending, Rejected, Over Limit) across all dashboard tabs. Backend: - SQL backend filters via WHERE status IN (...) on ComputedVisitCache - Pipeline skips strict tolerance check when filters are present, reusing cached data instead of re-downloading from Connect - Filter-only changes read from cache instantly (no bust_cache flag) - views.py parses status_filter param, deepcopies config, injects filter Frontend: - Filter bar with toggle pills for each status (default: Approved only) - Apply/Reset buttons trigger server-side SSE reload - bustCacheRef distinguishes Refresh Data (bust=true) from Apply (bust=false) - sessionStorage persists filter state through OAuth redirects Docs: updated DOCUMENTATION.md, DASHBOARD_GUIDE.md, and Guide tab with filter bar documentation. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
% ≤1 Missed, % 4 Visits On Track, % 5 Visits Complete, and % 6 Visits Complete now count only eligible mothers (eligible_full_intervention_bonus = "1") instead of all mothers. Updates tooltips, Guide tab, and docs. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds end-to-end server-side Visit Status filtering: status_filter is parsed/validated in views, threaded into per-request pipeline configs, SQL cache reads now filter by status (and support list/single values), pipeline cache logic is filter-aware (populate unfiltered cache on miss, then read filtered), frontend persists/applies status filters and controls cache-busting; docs/metrics updated. Changes
Sequence DiagramsequenceDiagram
participant User as User / Frontend
participant FE as Frontend Template
participant View as MBWMonitoring View
participant Pipeline as Pipeline (streaming)
participant Cache as ComputedVisitCache
participant SQL as SQL Backend
User->>FE: Toggle statusFilter & Apply
FE->>FE: persist mbw_pending_filters (sessionStorage)
FE->>View: GET /start-stream?status_filter=...
View->>Pipeline: stream_analysis(pipeline_config with status_filter)
Pipeline->>Cache: check cache (expected_count=0 for filtered)
alt Cache Hit
Cache-->>Pipeline: return cached entries
Pipeline->>SQL: query cached rows with status_filter
SQL-->>Pipeline: filtered results
else Cache Miss
Pipeline->>SQL: build full (unfiltered) dataset
SQL-->>Cache: populate cache (unfiltered)
Pipeline->>Cache: read cached dataset with status_filter
Cache->>SQL: apply status filter on read
SQL-->>Pipeline: filtered results
end
Pipeline-->>View: stream filtered visits (include status_filter in meta)
View-->>FE: SSE payload with metadata
FE-->>User: render dashboard with filtered data
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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
🧹 Nitpick comments (1)
commcare_connect/workflow/templates/mbw_monitoring/template.py (1)
3590-3592: Avoid per-group rounding before computing the weighted total.Line 3590 rounds each subgroup contribution first, which can skew the final
% ≤1 Missed. Keep full precision per subgroup and round once at the end.📐 Suggested adjustment
- totalMissedNum += Math.round(r.pct_missed_1_or_less_visits * r.total_cases_eligible_at_registration / 100); + totalMissedNum += (r.pct_missed_1_or_less_visits * r.total_cases_eligible_at_registration / 100);🤖 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 3590 - 3592, The code is rounding each subgroup contribution when accumulating totalMissedNum which skews the weighted percent; change the accumulation in the loop that updates totalMissedNum to add the full-precision value (r.pct_missed_1_or_less_visits * r.total_cases_eligible_at_registration / 100) without Math.round, and only apply Math.round once when computing pctMissed (using totals.total_cases_eligible_at_registration to compute the final percentage); update references in the loop that use totalMissedNum, r.pct_missed_1_or_less_visits, r.total_cases_eligible_at_registration, and the pctMissed calculation to reflect this single final rounding.
🤖 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 104-113: The deserialized/session value _savedStatusFilter (and
instance.state?.status_filter) may be non-array and later array methods
(slice().sort(), indexOf) will throw; normalize these values when initializing
statusFilter and appliedStatusFilter by checking
Array.isArray(_savedStatusFilter) and
Array.isArray(instance.state?.status_filter) and falling back to ['approved']
(e.g. use a utility or inline expression that returns the value if
Array.isArray(...) else [value] if defined else ['approved']); also ensure the
JSON.parse try/catch only assigns _savedStatusFilter when the parsed value is an
array or convert it to an array safely so that references to statusFilter,
appliedStatusFilter, and operations in functions that call slice().sort() and
indexOf are always operating on arrays.
In `@commcare_connect/workflow/templates/mbw_monitoring/views.py`:
- Around line 421-427: The status_filter parsing silently drops
invalid/mixed-case tokens which can remove the filter entirely; change parsing
to be case-insensitive (call .lower() on tokens) and validate against
VALID_STATUSES, and if request.GET.get("status_filter") was provided but no
valid tokens remain, return a 400 Bad Request (or raise Django's
HttpResponseBadRequest) instead of treating it as an empty/unfiltered list;
update the logic around VALID_STATUSES, status_filter_raw and status_filter
accordingly and apply the same fix in the GPS detail path referenced by the
comment.
---
Nitpick comments:
In `@commcare_connect/workflow/templates/mbw_monitoring/template.py`:
- Around line 3590-3592: The code is rounding each subgroup contribution when
accumulating totalMissedNum which skews the weighted percent; change the
accumulation in the loop that updates totalMissedNum to add the full-precision
value (r.pct_missed_1_or_less_visits * r.total_cases_eligible_at_registration /
100) without Math.round, and only apply Math.round once when computing pctMissed
(using totals.total_cases_eligible_at_registration to compute the final
percentage); update references in the loop that use totalMissedNum,
r.pct_missed_1_or_less_visits, r.total_cases_eligible_at_registration, and the
pctMissed calculation to reflect this single final rounding.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
commcare_connect/labs/analysis/backends/sql/backend.pycommcare_connect/labs/analysis/pipeline.pycommcare_connect/workflow/templates/mbw_monitoring/DASHBOARD_GUIDE.mdcommcare_connect/workflow/templates/mbw_monitoring/DOCUMENTATION.mdcommcare_connect/workflow/templates/mbw_monitoring/followup_analysis.pycommcare_connect/workflow/templates/mbw_monitoring/template.pycommcare_connect/workflow/templates/mbw_monitoring/views.pyconfig/settings/local.py
… rounding - Normalize statusFilter init: Array.isArray guard on JSON.parse and snapshot state, coerce non-array values to [value], fall back to ['approved'] - Case-insensitive status_filter parsing (.lower()) in both SSE and GPS detail paths; return error when provided filter has no valid tokens - Remove intermediate Math.round in totals row accumulation loop to avoid ±2pp skew across subgroups - Update % Still Eligible color thresholds: ≥85% green, 50-84% yellow, <50% red Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
commcare_connect/workflow/templates/mbw_monitoring/template.py (1)
104-123:⚠️ Potential issue | 🟠 MajorMove status-filter hydration out of render and hard-normalize values.
Line 104-Line 123 currently reads/parses/removes
mbw_pending_filtersduring render, and_normalizeStatusFilterstill allows empty/invalid arrays through. That can silently dropstatus_filterfrom requests (Line 272 / Line 939) and make filter persistence brittle.🛠️ Proposed hardening
- var _savedStatusFilter = null; - try { - var _raw = sessionStorage.getItem('mbw_pending_filters'); - if (_raw) { - var _parsed = JSON.parse(_raw); - if (Array.isArray(_parsed)) _savedStatusFilter = _parsed; - sessionStorage.removeItem('mbw_pending_filters'); - } - } catch(e) {} - var _normalizeStatusFilter = function(val) { - if (Array.isArray(val)) return val; - if (val != null) return [val]; - return null; - }; - var _initFilter = _normalizeStatusFilter(_savedStatusFilter) - || _normalizeStatusFilter(instance.state?.status_filter) - || ['approved']; - var [statusFilter, setStatusFilter] = React.useState(_initFilter); - var [appliedStatusFilter, setAppliedStatusFilter] = React.useState(_initFilter); + var ALLOWED_STATUS_FILTERS = ['approved', 'pending', 'rejected', 'over_limit']; + var _normalizeStatusFilter = function(val) { + var arr = Array.isArray(val) ? val : (val != null ? [val] : []); + var filtered = arr.filter(function(v) { return ALLOWED_STATUS_FILTERS.indexOf(v) !== -1; }); + return filtered.length > 0 ? filtered : ['approved']; + }; + var _stateInitFilter = _normalizeStatusFilter(instance.state?.status_filter); + var [statusFilter, setStatusFilter] = React.useState(_stateInitFilter); + var [appliedStatusFilter, setAppliedStatusFilter] = React.useState(_stateInitFilter); + + React.useEffect(function() { + try { + var raw = sessionStorage.getItem('mbw_pending_filters'); + if (!raw) return; + var parsed = JSON.parse(raw); + var normalized = _normalizeStatusFilter(parsed); + setStatusFilter(normalized); + setAppliedStatusFilter(normalized); + } catch (e) { + // ignore malformed session value + } finally { + try { sessionStorage.removeItem('mbw_pending_filters'); } catch (e) {} + } + }, []);Also applies to: 272-274, 939-941, 2227-2233
🤖 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 104 - 123, The render currently parses/removes sessionStorage and weakly normalizes filters; move that work out of render by creating a single hydration routine (e.g., hydrateStatusFilter) that: reads sessionStorage.getItem('mbw_pending_filters'), JSON.parses safely, filters out non-string/empty values, and returns either a non-empty array or null; use that routine as the lazy initializer for React.useState (e.g., React.useState(() => hydrateStatusFilter() || hydrateFrom(instance.state?.status_filter) || ['approved'])) so _initFilter logic runs once and not on every render; perform the sessionStorage.removeItem('mbw_pending_filters') inside a useEffect that runs once after mount instead of during render; tighten _normalizeStatusFilter to reject empty/invalid arrays (filter falsy items and return null if result is empty) and apply the same normalization when initializing appliedStatusFilter and when reading instance.state.
🧹 Nitpick comments (2)
commcare_connect/workflow/templates/mbw_monitoring/views.py (1)
421-430: Consider extracting duplicated status filter parsing to a helper function.The status filter parsing logic at Lines 421-430 is duplicated at Lines 1008-1016 in
MBWGPSDetailView.get. Both use the sameVALID_STATUSESset and identical validation logic.Extracting to a module-level helper would reduce duplication and ensure consistent behavior:
♻️ Proposed refactor
Add at module level (after existing helper functions around line 115):
VALID_STATUS_FILTER_VALUES = {"approved", "pending", "rejected", "over_limit"} def _parse_status_filter(raw: str | None) -> tuple[list[str], str | None]: """Parse and validate status_filter query param. Returns (parsed_list, error_message). If error_message is not None, the caller should return an error response. """ if not raw: return [], None parsed = [ s.strip().lower() for s in raw.split(",") if s.strip().lower() in VALID_STATUS_FILTER_VALUES ] if not parsed: return [], "Invalid status_filter values" return parsed, NoneThen in both locations:
- VALID_STATUSES = {"approved", "pending", "rejected", "over_limit"} - status_filter_raw = request.GET.get("status_filter", "") - status_filter = [ - s.strip().lower() for s in status_filter_raw.split(",") - if s.strip().lower() in VALID_STATUSES - ] if status_filter_raw else [] - if status_filter_raw and not status_filter: - yield send_sse_event("Error", error="Invalid status_filter values") - return + status_filter, filter_error = _parse_status_filter(request.GET.get("status_filter")) + if filter_error: + yield send_sse_event("Error", error=filter_error) + returnAlso applies to: 1008-1016
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@commcare_connect/workflow/templates/mbw_monitoring/views.py` around lines 421 - 430, Extract the duplicated status-filter parsing into a module-level helper named VALID_STATUS_FILTER_VALUES and _parse_status_filter(raw: str | None) -> tuple[list[str], str | None]; implement _parse_status_filter to return ([], None) for empty input, the list of lowercased valid statuses when any match VALID_STATUS_FILTER_VALUES, or ([], "Invalid status_filter values") when raw was provided but no valid entries were found. Replace the inline parsing in the SSE view (the block using VALID_STATUSES and status_filter/status_filter_raw) and in MBWGPSDetailView.get with calls to _parse_status_filter and, on receiving a non-None error message, use the same error handling path (yield send_sse_event("Error", error=...) in the SSE view and the existing error return behavior in MBWGPSDetailView.get) so behavior remains identical.commcare_connect/workflow/templates/mbw_monitoring/template.py (1)
2159-2187: Add pressed-state semantics to the Visit Status toggles.The buttons are functionally toggles; expose state to assistive tech with
aria-pressed.♿ Suggested tweak
return <button key={opt.value} type="button" + aria-pressed={isActive} + aria-label={'Visit Status: ' + opt.label} onClick={function() {🤖 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 2159 - 2187, The Visit Status toggle buttons are missing pressed-state semantics for assistive tech; update the button elements (the map callback that renders each <button> using opt and isActive) to include an aria-pressed attribute set to the boolean isActive so screen readers know the toggle state, e.g., aria-pressed={isActive}; keep the existing onClick that calls setStatusFilter and do not change behavior—only add the aria-pressed attribute to the rendered button in the map where statusFilter and setStatusFilter are used.
🤖 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/DASHBOARD_GUIDE.md`:
- Around line 728-729: Update the “Eligible 5+” thresholds so both places use
the same rule set: change the earlier “Eligible 5+” section's thresholds
(currently ≥70/50-69/<50) to match the reference table values (≥85/50-84/<50);
locate the text labelled “Eligible 5+” in the document and replace the old
triplet with the new one so the section and the Green/Yellow/<50 table are
consistent.
In `@commcare_connect/workflow/templates/mbw_monitoring/DOCUMENTATION.md`:
- Around line 975-987: Update the earlier "Key Design Decisions" paragraph (the
one that currently claims filtering is client-side only) to state that filtering
is implemented both client-side and server-side: clarify that some filters (see
"Filtering" -> "Server-Side Filters") modify the SSE stream and trigger a server
query and SSE reload (e.g., Visit Status and App Version for GPS), while others
remain client-side and operate on already-fetched pipeline data; mention that
server-side filters require clicking "Apply" and that state is persisted in
sessionStorage.
In `@commcare_connect/workflow/templates/mbw_monitoring/template.py`:
- Around line 4267-4269: The "Color Coding Reference" still uses the old
thresholds (>=70 / 50–69 / <50); update the corresponding span elements to match
the new "% Still Eligible" thresholds used later by changing any occurrences of
"≥70" or "≥70" to "≥85" ("≥85") and "50–69" to "50–84" (preserve the
en-dash as "–" or –), e.g., update the span text that currently reads
"≥70 Green" and "50–69 Yellow" to "≥85 Green" and "50–84
Yellow" so both guide sections align.
---
Duplicate comments:
In `@commcare_connect/workflow/templates/mbw_monitoring/template.py`:
- Around line 104-123: The render currently parses/removes sessionStorage and
weakly normalizes filters; move that work out of render by creating a single
hydration routine (e.g., hydrateStatusFilter) that: reads
sessionStorage.getItem('mbw_pending_filters'), JSON.parses safely, filters out
non-string/empty values, and returns either a non-empty array or null; use that
routine as the lazy initializer for React.useState (e.g., React.useState(() =>
hydrateStatusFilter() || hydrateFrom(instance.state?.status_filter) ||
['approved'])) so _initFilter logic runs once and not on every render; perform
the sessionStorage.removeItem('mbw_pending_filters') inside a useEffect that
runs once after mount instead of during render; tighten _normalizeStatusFilter
to reject empty/invalid arrays (filter falsy items and return null if result is
empty) and apply the same normalization when initializing appliedStatusFilter
and when reading instance.state.
---
Nitpick comments:
In `@commcare_connect/workflow/templates/mbw_monitoring/template.py`:
- Around line 2159-2187: The Visit Status toggle buttons are missing
pressed-state semantics for assistive tech; update the button elements (the map
callback that renders each <button> using opt and isActive) to include an
aria-pressed attribute set to the boolean isActive so screen readers know the
toggle state, e.g., aria-pressed={isActive}; keep the existing onClick that
calls setStatusFilter and do not change behavior—only add the aria-pressed
attribute to the rendered button in the map where statusFilter and
setStatusFilter are used.
In `@commcare_connect/workflow/templates/mbw_monitoring/views.py`:
- Around line 421-430: Extract the duplicated status-filter parsing into a
module-level helper named VALID_STATUS_FILTER_VALUES and
_parse_status_filter(raw: str | None) -> tuple[list[str], str | None]; implement
_parse_status_filter to return ([], None) for empty input, the list of
lowercased valid statuses when any match VALID_STATUS_FILTER_VALUES, or ([],
"Invalid status_filter values") when raw was provided but no valid entries were
found. Replace the inline parsing in the SSE view (the block using
VALID_STATUSES and status_filter/status_filter_raw) and in MBWGPSDetailView.get
with calls to _parse_status_filter and, on receiving a non-None error message,
use the same error handling path (yield send_sse_event("Error", error=...) in
the SSE view and the existing error return behavior in MBWGPSDetailView.get) so
behavior remains identical.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
commcare_connect/workflow/templates/mbw_monitoring/DASHBOARD_GUIDE.mdcommcare_connect/workflow/templates/mbw_monitoring/DOCUMENTATION.mdcommcare_connect/workflow/templates/mbw_monitoring/template.pycommcare_connect/workflow/templates/mbw_monitoring/views.py
- Fix remaining Eligible 5+ thresholds (DASHBOARD_GUIDE overview section, template.py Overview guide and Color Coding Reference) to match new ≥85% green / 50-84% yellow / <50% red rule - Update DOCUMENTATION.md "Client-side filtering" → "Hybrid filtering" to reflect server-side Visit Status and App Version filters - Refactor statusFilter init: lazy useState initializer, useEffect for sessionStorage cleanup, tighter validation (reject non-string/falsy) - Add aria-pressed to Visit Status toggle buttons for screen readers - Extract _parse_status_filter helper with VALID_STATUS_FILTER_VALUES frozenset, replace inline parsing at SSE and GPS detail call sites Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/DASHBOARD_GUIDE.md`:
- Line 47: Update the sentence beginning "**Note**: Changing Visit Status does
**not** re-download data from Connect..." in DASHBOARD_GUIDE.md to a softened
statement that describes the typical/fast-path behavior: say that changing Visit
Status usually reuses cached data and applies the filter server-side (resulting
in fast responses), but acknowledge exceptions such as cold/expired cache or
when a forced refresh triggers re-download from Connect. Keep the original
context and clarity but replace the absolute "does not" with language like
"typically does not" and include the exceptions.
In `@commcare_connect/workflow/templates/mbw_monitoring/template.py`:
- Line 3584: The UI uses different threshold logic for the same metric family
(pct_still_eligible) — the new cell uses >=85 / 50–84 / <50 while the Overview
table ("Eligible 5+") still uses >=70 / >=50 — make them consistent by
centralizing the threshold logic: create or use a shared helper (e.g.,
getStatusColor or a THRESHOLDS constant) and update the Overview table coloring
logic that computes "Eligible 5+" to call that helper or use the same constants
so both row.pct_still_eligible renderings produce matching colors.
- Around line 104-124: _hydrateStatusFilter and _normalizeStatusFilter currently
accept any non-empty string from session/passed-in values; update both to
validate returned/normalized entries against an allowlist of permitted status
tokens (introduce or use an ALLOWED_STATUS_FILTERS array/set) and filter out
anything not present in that allowlist before returning; ensure both functions
return null when no allowed tokens remain. Reference the functions
_hydrateStatusFilter and _normalizeStatusFilter and apply the allowlist filter
to the intermediate "filtered" arrays.
In `@commcare_connect/workflow/templates/mbw_monitoring/views.py`:
- Around line 80-95: _parsestatus_filter currently silently drops invalid tokens
(e.g., "approved,foo"); change it to reject the whole input if any token is
invalid by splitting raw on commas, normalizing each token (strip().lower()),
and if any token is non-empty and not in VALID_STATUS_FILTER_VALUES return ([],
"Invalid status_filter values: <bad tokens>") or a generic error; otherwise
return the full list of normalized valid tokens and None. Update references in
callers that expect the old behavior only if needed; key symbols:
_parse_status_filter and VALID_STATUS_FILTER_VALUES.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
commcare_connect/workflow/templates/mbw_monitoring/DASHBOARD_GUIDE.mdcommcare_connect/workflow/templates/mbw_monitoring/DOCUMENTATION.mdcommcare_connect/workflow/templates/mbw_monitoring/template.pycommcare_connect/workflow/templates/mbw_monitoring/views.py
commcare_connect/workflow/templates/mbw_monitoring/DASHBOARD_GUIDE.md
Outdated
Show resolved
Hide resolved
…trict parsing - Soften DASHBOARD_GUIDE note: "typically does not re-download" with exceptions (cold cache, forced refresh) - Centralize Eligible 5+ color thresholds via ELIGIBLE_THRESHOLDS constant and getEligibleColor helper; update Overview and Performance table cells to use it (both now consistently 85/50) - Add ALLOWED_STATUS_FILTERS allowlist to _hydrateStatusFilter and _normalizeStatusFilter — reject tokens not in the allowlist - Make _parse_status_filter reject the entire input if any token is invalid (e.g. "approved,foo" → error naming "foo") Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
commcare_connect/workflow/templates/mbw_monitoring/DASHBOARD_GUIDE.md (2)
47-47: Soften the bold emphasis on "not" for clarity.The sentence uses "typically does not" with bold emphasis, then immediately lists exceptions. The bold "not" creates a stronger claim than the following exceptions support. Consider removing the bold emphasis or rephrasing to "usually reuses" to better align the wording with the nuanced behavior.
📝 Suggested rephrasing
-> **Note**: Changing Visit Status typically does **not** re-download data from Connect. The dashboard reuses its cached pipeline data and applies the filter server-side, so switching statuses is usually fast (seconds, not minutes). Exceptions include a cold or expired cache, or when a forced refresh (`?bust_cache=1`) triggers a full re-download. +> **Note**: Changing Visit Status usually reuses cached pipeline data and applies the filter server-side, so switching statuses is typically fast (seconds, not minutes). Exceptions include a cold or expired cache, or when a forced refresh (`?bust_cache=1`) triggers a full re-download.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@commcare_connect/workflow/templates/mbw_monitoring/DASHBOARD_GUIDE.md` at line 47, The bold emphasis on "not" makes the statement too strong given the listed exceptions; update the sentence in DASHBOARD_GUIDE.md (the line that currently reads "Changing Visit Status typically does **not** re-download data from Connect...") to a softer phrasing such as "Changing Visit Status usually reuses cached pipeline data and applies the filter server-side..." or remove the bolding so it reads "typically does not", ensuring the exceptions that follow remain accurate.
638-641: Clarify the relationship between "4 visits" and "3+ completed" threshold.Line 639 states "they should have completed their first 4 visits by now," but Line 640 counts "3+ total completed visits." While this 3-out-of-4 threshold is intentional (allowing some flexibility for "on track" status), the relationship could be clearer to readers who might expect 4 completed visits for "4 Visits On Track."
📝 Suggested clarification
**How it's calculated:** 1. Filter to eligible mothers whose Month 1 visit scheduled date is 5+ days ago (i.e., they should have completed their first 4 visits by now) -2. Count those with 3+ total completed visits +2. Count those with 3+ total completed visits (on track means at least 3 of the first 4 visits are completed) 3. **(with 3+ completed / total eligible with Month 1 due) × 100**🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@commcare_connect/workflow/templates/mbw_monitoring/DASHBOARD_GUIDE.md` around lines 638 - 641, Summary: The guide currently mentions "first 4 visits" but uses "3+ completed" in the metric, which can confuse readers; update the text around the phrases "4 visits" and "(with 3+ completed / total eligible with Month 1 due) × 100" to explicitly state that the metric measures mothers who have completed at least 3 of the first 4 scheduled visits (i.e., a 3-of-4 threshold) and that eligible mothers are those whose Month 1 visit scheduled date is 5+ days ago; replace or append the existing lines with a single clear sentence explaining that the numerator is mothers with ≥3 completed visits out of the first 4 and the denominator is all mothers eligible with Month 1 due, so the resulting percentage reflects the proportion considered "on track" under the 3-of-4 rule.
🤖 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 105-107: The global sessionStorage key 'mbw_pending_filters' is
unscoped and can bleed filters across runs; update the key usage in
_hydrateStatusFilter and all places that call
sessionStorage.getItem/setItem/removeItem for 'mbw_pending_filters' to include a
run-specific suffix (e.g. use a namespaced key like
"mbw_pending_filters:{instance.id}" or "mbw_pending_filters:{opportunity_id}"),
ensure the same namespacing logic is used for reads, writes and removals
(sessionStorage.getItem, sessionStorage.setItem, sessionStorage.removeItem) so
all operations consistently use the namespaced key to prevent cross-run filter
bleed.
---
Nitpick comments:
In `@commcare_connect/workflow/templates/mbw_monitoring/DASHBOARD_GUIDE.md`:
- Line 47: The bold emphasis on "not" makes the statement too strong given the
listed exceptions; update the sentence in DASHBOARD_GUIDE.md (the line that
currently reads "Changing Visit Status typically does **not** re-download data
from Connect...") to a softer phrasing such as "Changing Visit Status usually
reuses cached pipeline data and applies the filter server-side..." or remove the
bolding so it reads "typically does not", ensuring the exceptions that follow
remain accurate.
- Around line 638-641: Summary: The guide currently mentions "first 4 visits"
but uses "3+ completed" in the metric, which can confuse readers; update the
text around the phrases "4 visits" and "(with 3+ completed / total eligible with
Month 1 due) × 100" to explicitly state that the metric measures mothers who
have completed at least 3 of the first 4 scheduled visits (i.e., a 3-of-4
threshold) and that eligible mothers are those whose Month 1 visit scheduled
date is 5+ days ago; replace or append the existing lines with a single clear
sentence explaining that the numerator is mothers with ≥3 completed visits out
of the first 4 and the denominator is all mothers eligible with Month 1 due, so
the resulting percentage reflects the proportion considered "on track" under the
3-of-4 rule.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
commcare_connect/workflow/templates/mbw_monitoring/DASHBOARD_GUIDE.mdcommcare_connect/workflow/templates/mbw_monitoring/template.pycommcare_connect/workflow/templates/mbw_monitoring/views.py
- Namespace sessionStorage key with instance.id to prevent cross-run
filter bleed (mbw_pending_filters:{id} instead of global key)
- Soften DASHBOARD_GUIDE note: remove bold on "not" for re-download
- Clarify "% 4 Visits On Track" as a 3-of-4 threshold
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
commcare_connect/workflow/templates/mbw_monitoring/template.py (1)
141-143:⚠️ Potential issue | 🟡 MinorGuard
sessionStorage.removeItemin the mount effect.Line 142 is unguarded while other storage calls in this file are wrapped. In restricted storage contexts, this can throw and interrupt render.
🛠️ Suggested fix
React.useEffect(function() { - sessionStorage.removeItem(_statusFilterKey); + try { sessionStorage.removeItem(_statusFilterKey); } catch(e) {} }, []);🤖 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 141 - 143, The unguarded sessionStorage.removeItem call inside the React.useEffect mount effect can throw in restricted storage contexts; wrap the call in a safe-guard that checks for availability and catches errors: inside the React.useEffect where sessionStorage.removeItem(_statusFilterKey) is called, detect that sessionStorage is accessible (or feature-detect via typeof/sessionStorage in try/catch) and perform the removal inside a try/catch that swallows or logs errors so render is not interrupted by storage permission exceptions.
🤖 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 3629-3631: The aggregation can produce NaN when
r.pct_missed_1_or_less_visits is null/undefined; update the accumulation for
totalMissedNum to coerce missing values to 0 (e.g., use
Number(r.pct_missed_1_or_less_visits) || 0) before multiplying by
r.total_cases_eligible_at_registration, and keep the pctMissed calculation that
checks totals.total_cases_eligible_at_registration > 0; update the code around
totalMissedNum, r.pct_missed_1_or_less_visits, and pctMissed to apply this
guard.
---
Duplicate comments:
In `@commcare_connect/workflow/templates/mbw_monitoring/template.py`:
- Around line 141-143: The unguarded sessionStorage.removeItem call inside the
React.useEffect mount effect can throw in restricted storage contexts; wrap the
call in a safe-guard that checks for availability and catches errors: inside the
React.useEffect where sessionStorage.removeItem(_statusFilterKey) is called,
detect that sessionStorage is accessible (or feature-detect via
typeof/sessionStorage in try/catch) and perform the removal inside a try/catch
that swallows or logs errors so render is not interrupted by storage permission
exceptions.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
commcare_connect/workflow/templates/mbw_monitoring/DASHBOARD_GUIDE.mdcommcare_connect/workflow/templates/mbw_monitoring/template.py
Comment on lines
+3629
to
+3631
| totalMissedNum += r.pct_missed_1_or_less_visits * r.total_cases_eligible_at_registration / 100; | ||
| }); | ||
| var pctMissed = totals.total_cases > 0 ? Math.round(totalMissedNum / totals.total_cases * 100) : 0; | ||
| var pctMissed = totals.total_cases_eligible_at_registration > 0 ? Math.round(totalMissedNum / totals.total_cases_eligible_at_registration * 100) : 0; |
There was a problem hiding this comment.
Prevent NaN in total % ≤1 Missed aggregation.
If any row has null/undefined pct_missed_1_or_less_visits, Line 3629 can propagate NaN to the total row.
🛠️ Suggested fix
perf.forEach(function(r) {
- totalMissedNum += r.pct_missed_1_or_less_visits * r.total_cases_eligible_at_registration / 100;
+ var missedPct = typeof r.pct_missed_1_or_less_visits === 'number'
+ ? r.pct_missed_1_or_less_visits
+ : 0;
+ var eligibleDen = typeof r.total_cases_eligible_at_registration === 'number'
+ ? r.total_cases_eligible_at_registration
+ : 0;
+ totalMissedNum += (missedPct * eligibleDen) / 100;
});
var pctMissed = totals.total_cases_eligible_at_registration > 0 ? Math.round(totalMissedNum / totals.total_cases_eligible_at_registration * 100) : 0;🤖 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
3629 - 3631, The aggregation can produce NaN when r.pct_missed_1_or_less_visits
is null/undefined; update the accumulation for totalMissedNum to coerce missing
values to 0 (e.g., use Number(r.pct_missed_1_or_less_visits) || 0) before
multiplying by r.total_cases_eligible_at_registration, and keep the pctMissed
calculation that checks totals.total_cases_eligible_at_registration > 0; update
the code around totalMissedNum, r.pct_missed_1_or_less_visits, and pctMissed to
apply this guard.
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.
Summary
Summary by CodeRabbit
New Features
Behavior Changes
Documentation