feat(phase5): Cluster C — Broadcast + Intelligence#66
Merged
Conversation
…id token cleanup - Client: set hasFcmToken: true when registering a new FCM token - Server: clear hasFcmToken: false when all tokens are invalidated during cleanup - Schema: remove transform-derived hasFcmToken; make it an explicit stored field with default(false) - Tests: replace derived-behavior test with explicit-field preservation test Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…n on partial-invalid - Update existing partial-invalid test to assert hasFcmToken is NOT present - Add new test verifying hasFcmToken: false when all tokens are invalid Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Extend inboxPayloadSchema with optional followUpConsent boolean - Update processInboxItemCore to write followUpConsent onto report_sms_consent - Add 3 integration tests for municipalityId and followUpConsent fields - Add shared-validators tests for followUpConsent acceptance/rejection Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…rmc flow - mass_alert_requests rules: allow muni admin/superadmin create with status in ['queued', 'pending_ndrrmc_review', 'sent']; restrict update to superadmin only; preserve read scope for muni admin + superadmin. - Add 4 rules tests for mass_alert_requests creation boundaries. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ueueBroadcastSms - Extend SmsPurpose with 'mass_alert' and add TEMPLATES entry - Add renderBroadcastTemplate for broadcast messages (no publicRef) - Add enqueueBroadcastSms helper in send-sms.ts - Update shared-validators exports and rebuild lib/ Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…rwardToNDRRMC - Add four callable functions for mass alert system: - massAlertReachPlanPreview: counts FCM/SMS recipients, determines route - sendMassAlert: sends direct mass alert (creates doc + fires FCM) - requestMassAlertEscalation: creates pending NDRRMC review request - forwardMassAlertToNDRRMC: superadmin forwards pending request to NDRRMC - Add mass-alert.test.ts with 17 integration tests - Add evidencePack and forwardedBy optional fields to massAlertRequestDocSchema - Export callables from functions/src/index.ts Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…on, clean up mockCountOnDb spy
…ion CTA - Create MassAlertModal with message input, live GSM-7/UCS-2 encoding display, reach plan preview, direct send, and NDRRMC escalation CTA - Add 4 mass alert callable wrappers to callables.ts - Wire Mass Alert button into TriageQueuePage header with Escape dismissal - Fix usePendingHandoffs mocks in triage-queue and shift-handoff tests - Add @bantayog/shared-validators dependency to admin-desktop Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… per municipality
Reviewer's GuideImplements Phase 5 Cluster C by adding a mass alert broadcast pipeline (FCM + SMS) with NDRRMC escalation, daily Firestore analytics snapshots with a 7‑day admin dashboard, and tightening FCM token + SMS consent schemas and flows. Sequence diagram for direct mass alert send flowsequenceDiagram
actor MunicipalAdmin
participant TriageQueuePage
participant MassAlertModal
participant AdminCallables as AdminCallablesService
participant CF_MassAlert as CF_massAlertReachPlanPreview
participant CF_SendMass as CF_sendMassAlert
participant Firestore as Firestore
participant FcmMassSend as sendMassAlertFcm
participant Responders as RespondersDevices
MunicipalAdmin->>TriageQueuePage: Click "Mass Alert" button
TriageQueuePage->>MassAlertModal: Render MassAlertModal(municipalityId)
MunicipalAdmin->>MassAlertModal: Type alert message
MunicipalAdmin->>MassAlertModal: Click "Preview Reach"
MassAlertModal->>AdminCallables: massAlertReachPlanPreview(targetScope,message)
AdminCallables->>CF_MassAlert: HTTPS callable massAlertReachPlanPreview
CF_MassAlert->>Firestore: Query responders where hasFcmToken==true and municipalityId in scope count()
CF_MassAlert->>Firestore: Query report_sms_consent where followUpConsent==true and municipalityId in scope count()
CF_MassAlert-->>AdminCallables: reachPlan(route,fcmCount,smsCount,segmentCount,unicodeWarning)
AdminCallables-->>MassAlertModal: reachPlan
MassAlertModal-->>MunicipalAdmin: Show reach preview and route
MunicipalAdmin->>MassAlertModal: Click "Send Alert" (route == direct)
MassAlertModal->>AdminCallables: sendMassAlert(reachPlan,message,targetScope,idempotencyKey)
AdminCallables->>CF_SendMass: HTTPS callable sendMassAlert
CF_SendMass->>CF_MassAlert: massAlertReachPlanPreviewCore(targetScope,message,actor)
CF_MassAlert-->>CF_SendMass: serverPreview(reachPlan,route=direct)
CF_SendMass->>Firestore: Create mass_alert_requests doc(status="sent",estimatedReach,...)
CF_SendMass->>FcmMassSend: sendMassAlertFcm(municipalityIds,title,body,data)
FcmMassSend->>Firestore: Query responders where hasFcmToken==true and municipalityId in scope
FcmMassSend->>Responders: Batched FCM sendEachForMulticast(<=5000 tokens)
FcmMassSend-->>CF_SendMass: MassSendResult
CF_SendMass-->>AdminCallables: {requestId}
AdminCallables-->>MassAlertModal: {requestId}
MassAlertModal-->>MunicipalAdmin: Close modal
Sequence diagram for NDRRMC escalation and forwardingsequenceDiagram
actor MunicipalAdmin
actor ProvincialSuperadmin
participant MassAlertModal
participant AdminCallables as AdminCallablesService
participant CF_Escalate as CF_requestMassAlertEscalation
participant CF_Forward as CF_forwardMassAlertToNDRRMC
participant Firestore as Firestore
participant FcmMassSend as sendMassAlertFcm
MunicipalAdmin->>MassAlertModal: Compose message and preview reach
MassAlertModal-->>MunicipalAdmin: Route == ndrrmc_escalation
MunicipalAdmin->>MassAlertModal: Click "Request NDRRMC Escalation"
MassAlertModal->>AdminCallables: requestMassAlertEscalation(message,targetScope,evidencePack,idempotencyKey)
AdminCallables->>CF_Escalate: HTTPS callable requestMassAlertEscalation
CF_Escalate->>Firestore: Create mass_alert_requests doc(status="pending_ndrrmc_review",evidencePack,...)
CF_Escalate->>FcmMassSend: sendMassAlertFcm(municipalityIds,title="NDRRMC Escalation Request",body,data)
FcmMassSend->>Firestore: Query responders where hasFcmToken==true and municipalityId in scope
FcmMassSend-->>CF_Escalate: MassSendResult
CF_Escalate-->>AdminCallables: {requestId}
AdminCallables-->>MassAlertModal: {requestId}
MassAlertModal-->>MunicipalAdmin: Show escalation submitted
ProvincialSuperadmin->>AdminCallables: forwardMassAlertToNDRRMC(requestId,forwardMethod,ndrrrcRecipient)
AdminCallables->>CF_Forward: HTTPS callable forwardMassAlertToNDRRMC
CF_Forward->>Firestore: Get mass_alert_requests doc(requestId)
CF_Forward-->>CF_Forward: Validate status == pending_ndrrmc_review
CF_Forward->>Firestore: Update doc(status="forwarded_to_ndrrmc",forwardedAt,forwardedBy,forwardMethod,ndrrrcRecipient)
CF_Forward-->>AdminCallables: {success:true}
AdminCallables-->>ProvincialSuperadmin: Confirm forwarded
Sequence diagram for analytics snapshot writer and dashboardsequenceDiagram
participant Scheduler as CloudScheduler
participant CF_Analytics as analyticsSnapshotWriter
participant Firestore as Firestore
participant AdminDashboard as AnalyticsDashboardPage
participant FirestoreClient as ClientFirestore
Scheduler->>CF_Analytics: Trigger daily at 00:05 UTC
CF_Analytics->>CF_Analytics: Compute date and now
CF_Analytics->>Firestore: For each municipality in CAMARINES_NORTE_MUNICIPALITY_IDS
loop For each municipality
CF_Analytics->>Firestore: Count report_ops by status for municipality
CF_Analytics->>Firestore: Count report_ops by severity for municipality
CF_Analytics->>Firestore: Write analytics_snapshots/{date}/{municipalityId}/summary
end
CF_Analytics->>Firestore: Write analytics_snapshots/{date}/province/summary with aggregates
AdminDashboard->>FirestoreClient: useQuery activeCount
FirestoreClient->>Firestore: getCountFromServer(report_ops where municipalityId,status in ACTIVE_STATUSES)
Firestore-->>FirestoreClient: Count
FirestoreClient-->>AdminDashboard: activeCount
AdminDashboard->>FirestoreClient: useQuery snapshots
FirestoreClient->>Firestore: getDocs(analytics_snapshots ordered by __name__ desc limit 7)
Firestore-->>FirestoreClient: Snapshot docs
FirestoreClient-->>AdminDashboard: Last 7 day summaries
AdminDashboard-->>AdminDashboard: Render live count and SVG 7 day trend
Entity relationship diagram for mass alert and analytics dataerDiagram
RESPONDERS {
string uid
string agencyId
string municipalityId
string availabilityStatus
boolean isActive
string[] fcmTokens
boolean hasFcmToken
number createdAt
number updatedAt
number schemaVersion
}
REPORT_SMS_CONSENT {
string id
string municipalityId
boolean smsConsent
boolean followUpConsent
string locale
number createdAt
number schemaVersion
}
MASS_ALERT_REQUESTS {
string id
string requestedByMunicipality
string requestedByUid
string body
string targetType
string targetGeometryRef
string severity
number estimatedReach
string status
number createdAt
string evidencePack
string forwardedBy
number forwardedAt
string forwardMethod
string ndrrrcRecipient
number schemaVersion
}
ANALYTICS_SNAPSHOTS_SUMMARY {
string date
string municipalityId
json reportsByStatus
json reportsBySeverity
number generatedAt
number schemaVersion
}
RESPONDERS ||--o{ MASS_ALERT_REQUESTS : "requestedByUid"
REPORT_SMS_CONSENT }o--|| RESPONDERS : "same municipalityId"
MASS_ALERT_REQUESTS ||--o{ ANALYTICS_SNAPSHOTS_SUMMARY : "contributes_to_counts"
RESPONDERS ||--o{ ANALYTICS_SNAPSHOTS_SUMMARY : "counted_by_municipality"
Class diagram for core mass alert and analytics modulesclassDiagram
class MassAlertModal {
-string municipalityId
-string message
-ReachPlan reachPlan
-boolean loading
-string error
+MassAlertModal(municipalityId,onClose)
+handlePreview()
+handleSend()
+handleEscalate()
}
class CallablesService {
+massAlertReachPlanPreview(targetScope,message)
+sendMassAlert(reachPlan,message,targetScope,idempotencyKey)
+requestMassAlertEscalation(message,targetScope,evidencePack,idempotencyKey)
+forwardMassAlertToNDRRMC(requestId,forwardMethod,ndrrrcRecipient)
}
class MassAlertActor {
+string uid
+object claims
}
class MassAlertReachPlanPreviewCore {
+massAlertReachPlanPreviewCore(db,input,actor)
}
class SendMassAlertCore {
+sendMassAlertCore(db,input,actor)
}
class RequestMassAlertEscalationCore {
+requestMassAlertEscalationCore(db,input,actor)
}
class ForwardMassAlertToNDRRMCCore {
+forwardMassAlertToNDRRMCCore(db,input,actor)
}
class SendMassAlertFcmService {
+sendMassAlertFcm(db,opts) MassSendResult
}
class EnqueueBroadcastSmsService {
+enqueueBroadcastSms(db,tx,args) outboxId
}
class AnalyticsSnapshotWriterCore {
+analyticsSnapshotWriterCore(db,deps)
}
class AnalyticsDashboardPage {
+AnalyticsDashboardPage()
-useQueryActiveCount()
-useQuerySnapshots()
}
class ResponderDocSchema {
+boolean hasFcmToken
+string[] fcmTokens
}
class ReportSmsConsentDocSchema {
+string municipalityId
+boolean followUpConsent
}
class AnalyticsSnapshotDeps {
+string date
+number or Timestamp now
}
MassAlertModal --> CallablesService
MassAlertReachPlanPreviewCore --> MassAlertActor
SendMassAlertCore --> MassAlertActor
RequestMassAlertEscalationCore --> MassAlertActor
ForwardMassAlertToNDRRMCCore --> MassAlertActor
SendMassAlertCore --> SendMassAlertFcmService
RequestMassAlertEscalationCore --> SendMassAlertFcmService
EnqueueBroadcastSmsService --> ReportSmsConsentDocSchema
AnalyticsSnapshotWriterCore --> AnalyticsSnapshotDeps
AnalyticsDashboardPage --> AnalyticsSnapshotWriterCore
ResponderDocSchema --> SendMassAlertFcmService
ResponderDocSchema --> MassAlertReachPlanPreviewCore
ReportSmsConsentDocSchema --> MassAlertReachPlanPreviewCore
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
|
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 a mass-alert broadcast feature (UI + callables + backend services), a scheduled analytics snapshot writer and dashboard route, FCM/SMS broadcast sending and transactional token cleanup, schema and rule updates to support mass alerts and followUpConsent, and many new/updated tests across functions, packages, and apps. Changes
Sequence Diagram(s)sequenceDiagram
rect rgba(220,240,255,0.5)
participant Admin as Admin UI
participant Modal as MassAlertModal
participant Functions as Cloud Functions
participant Firestore as Firestore
participant FCM as FCM Service
end
Admin->>Modal: Compose message & click Preview
Modal->>Functions: massAlertReachPlanPreview(payload)
Functions->>Firestore: query responders & sms consent counts
Firestore-->>Functions: fcmCount, smsCount
Functions-->>Modal: { route, fcmCount, smsCount, unicodeWarning }
Modal->>Admin: Display preview
alt route == direct
Admin->>Modal: Click Send
Modal->>Functions: sendMassAlert(payload, idempotencyKey)
Functions->>Firestore: create mass_alert_request (sent)
Functions->>FCM: sendMassAlertFcm(municipalityIds, message)
FCM-->>Functions: { successCount, failureCount }
Functions-->>Modal: { requestId }
else route == ndrrmc_escalation
Admin->>Modal: Click Escalate
Modal->>Functions: requestMassAlertEscalation(payload, idempotencyKey)
Functions->>Firestore: create mass_alert_request (pending_ndrrmc_review)
Functions-->>Modal: { requestId }
end
sequenceDiagram
rect rgba(240,255,220,0.5)
participant Scheduler as Cloud Scheduler
participant Writer as analyticsSnapshotWriter
participant Firestore as Firestore
end
Scheduler->>Writer: daily trigger
Writer->>Firestore: query report_ops by status & municipality
Firestore-->>Writer: grouped counts
Writer->>Firestore: write analytics_snapshots/{date}/... summaries
Writer-->>Scheduler: log analytics.done
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- The mass-alert Firestore queries use
where('municipalityId', 'in', municipalityIds)withmunicipalityIdsallowed up to 12 items, but Firestore'sinoperator only supports up to 10 values; consider enforcing a max of 10 intargetScopeSchema(and/or splitting queries) to avoid runtime query errors. - The analytics snapshot writer stores per-municipality/province summaries in subcollections under
analytics_snapshots/{date}/{municipalityId}/summary, butAnalyticsDashboardPageis querying the rootanalytics_snapshotscollection and readingreportsByStatusoff the date docs, which are never written—consider querying the appropriate municipality/province subcollection instead.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The mass-alert Firestore queries use `where('municipalityId', 'in', municipalityIds)` with `municipalityIds` allowed up to 12 items, but Firestore's `in` operator only supports up to 10 values; consider enforcing a max of 10 in `targetScopeSchema` (and/or splitting queries) to avoid runtime query errors.
- The analytics snapshot writer stores per-municipality/province summaries in subcollections under `analytics_snapshots/{date}/{municipalityId}/summary`, but `AnalyticsDashboardPage` is querying the root `analytics_snapshots` collection and reading `reportsByStatus` off the date docs, which are never written—consider querying the appropriate municipality/province subcollection instead.
## Individual Comments
### Comment 1
<location path="functions/src/callables/mass-alert.ts" line_range="15" />
<code_context>
+const MAX_DIRECT_ROUTE = 5000
+
+const targetScopeSchema = z.object({
+ municipalityIds: z.array(z.string().min(1)).min(1).max(12),
+})
+
</code_context>
<issue_to_address>
**issue (bug_risk):** Target scope size can exceed Firestore `in` query limits, causing runtime errors.
This schema allows up to 12 `municipalityIds`, but Firestore `where('municipalityId', 'in', municipalityIds)` only supports up to 10 values. Passing 11–12 IDs will cause the preview query to fail at runtime. Please either cap the schema at `.max(10)` or split `municipalityIds` into chunks of ≤10 for multiple `in` queries and then aggregate the results.
</issue_to_address>
### Comment 2
<location path="functions/src/services/fcm-mass-send.ts" line_range="33-36" />
<code_context>
+ },
+): Promise<MassSendResult> {
+ // Query all responders with FCM tokens in scope
+ const snaps = await db
+ .collection('responders')
+ .where('hasFcmToken', '==', true)
+ .where('municipalityId', 'in', opts.municipalityIds)
+ .get()
+
</code_context>
<issue_to_address>
**issue:** Mass FCM sender also risks hitting Firestore `in` query 10-value limit.
This `where('municipalityId', 'in', opts.municipalityIds)` will fail if more than 10 IDs are passed. Either enforce `opts.municipalityIds.length <= 10` at the call site / schema, or handle it here by chunking into groups of ≤10, running multiple queries, and merging the token lists before sending FCM messages.
</issue_to_address>
### Comment 3
<location path="apps/admin-desktop/src/pages/AnalyticsDashboardPage.tsx" line_range="46" />
<code_context>
+ const { data: snapshots } = useQuery({
+ queryKey: ['analytics', 'snapshots', municipalityId],
+ queryFn: async () => {
+ const q = query(collection(db, `analytics_snapshots`), orderBy('__name__', 'desc'), limit(7))
+ const snap = await getDocs(q)
+ return snap.docs.map((d) => ({ date: d.id, ...d.data() }))
</code_context>
<issue_to_address>
**issue (bug_risk):** Analytics dashboard reads from root `analytics_snapshots` docs, which don’t align with how snapshots are written.
Snapshots are written under `analytics_snapshots/{date}/{municipalityId}/summary` and `analytics_snapshots/{date}/province/summary`, but this query reads only the top-level `analytics_snapshots` docs and expects fields like `reportsByStatus` there. That data won’t exist at this level, so the returned `snapshots` will be empty or incorrectly shaped. You’ll need to either query the `{date}/{municipalityId}/summary` / `province/summary` docs (e.g. find the latest dates, then fetch those summaries) or change the write path to denormalize the summary onto the top-level `analytics_snapshots/{date}` doc to match this query.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 18
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
functions/src/services/send-sms.ts (1)
50-57:⚠️ Potential issue | 🟠 MajorKeep
mass_alertout of the standard enqueue purpose gate.Line 56 allows
buildEnqueueSmsPayloadto accept'mass_alert', but that path renders throughrenderTemplate(publicRef template contract), not the broadcast renderer. This can produce malformed mass-alert bodies if called.💡 Proposed fix
const VALID_PURPOSES = new Set([ 'receipt_ack', 'verification', 'status_update', 'resolution', 'pending_review', - 'mass_alert', ])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@functions/src/services/send-sms.ts` around lines 50 - 57, VALID_PURPOSES currently includes 'mass_alert', which lets buildEnqueueSmsPayload accept mass alerts and route them through renderTemplate (publicRef template contract) instead of the broadcast renderer; remove 'mass_alert' from the VALID_PURPOSES set (or adjust the validation in buildEnqueueSmsPayload) so mass_alert is not permitted by that gate, and ensure mass_alert flows only through the broadcast renderer path (the broadcast-specific renderer function) or its dedicated enqueue flow.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/admin-desktop/src/__tests__/analytics-dashboard.test.tsx`:
- Around line 58-61: The test "scopes data to the caller's municipalityId for
muni admins" only checks rendered text; update it to assert the Firestore query
was scoped to municipalityId 'daet' by inspecting the mocks (e.g. verify
mockGetCountFromServer was called with a query built from collection/query/where
that includes where('municipalityId','==','daet')), or alternatively assert the
collection/query/where mock functions received the expected arguments; make this
check in the same test after rendering <AnalyticsDashboardPage /> with the
existing wrapper and before/after the screen.findByText assertion.
In `@apps/admin-desktop/src/__tests__/mass-alert-modal.test.tsx`:
- Around line 112-130: Update the two tests to assert on the actual arguments
passed to the mocks (not just call count): after triggering the send path in
MassAlertModal, assert mockSend was called with the expected payload (message
text, municipalityId and any metadata/state your UI sends); similarly, after
triggering the escalation CTA ensure mockEscalate (or
requestMassAlertEscalation) was invoked with the expected escalation payload
(including NDRRMC_PLAN-derived fields if mockPreview influences it). Use the
same mock names mockSend, mockEscalate and mockPreview to locate the tests and
add argument equality checks matching the component's request objects.
In `@apps/admin-desktop/src/pages/AnalyticsDashboardPage.tsx`:
- Around line 43-49: The query in useQuery (queryKey ['analytics','snapshots',
municipalityId]) is reading only analytics_snapshots/{date} docs but the writer
(analyticsSnapshotWriter) stores per-scope summaries at
analytics_snapshots/{date}/{scope}/summary, so reportsByStatus is missing; fix
by first querying the latest date documents as you do, then for each date id
fetch the scope summary document at collection path
analytics_snapshots/{date}/{municipalityId}/summary (or getDoc(doc(db,
'analytics_snapshots', date, municipalityId, 'summary'))), merge that summary
into each returned snapshot object (include reportsByStatus), and return the
array of snapshots with the nested summary data so components using
snapshots/reportsByStatus receive the correct values.
In `@apps/admin-desktop/src/pages/MassAlertModal.tsx`:
- Around line 52-57: The idempotencyKey is being regenerated on every send call
(crypto.randomUUID() inline) which can cause duplicate sends on retries; fix by
generating and storing a single stable idempotency key for the current draft in
the MassAlertModal component (e.g., a useRef or useState like
draftIdempotencyKey) when the modal/draft is initialized and only regenerate it
when the payload changes (message, reachPlan, municipalityId) or when the modal
is reset/closed, then pass that stored key into callables.sendMassAlert instead
of calling crypto.randomUUID() inline; apply the same change to the other send
location (lines ~71-76) to ensure consistent reuse across retries.
In `@apps/admin-desktop/src/pages/TriageQueuePage.tsx`:
- Around line 81-82: The modalOpen flag is becoming true even when massAlertOpen
is set without a municipalityId; update the logic so massAlertOpen cannot be
true unless municipalityId exists and reflect that in modalOpen: change where
massAlertOpen is set (the handler that opens the mass alert) to no-op or set
false when municipalityId is falsy, and change the modalOpen computation to
include massAlertOpen && !!municipalityId (e.g., const modalOpen =
!!dispatchForReportId || !!closeForReportId || handoffModalOpen ||
(massAlertOpen && !!municipalityId)); keep references to dispatchForReportId,
closeForReportId, handoffModalOpen and municipalityId to locate the changes.
In `@apps/admin-desktop/src/services/callables.ts`:
- Around line 74-125: The mass-alert callable types (used by
massAlertReachPlanPreview, sendMassAlert, requestMassAlertEscalation,
forwardMassAlertToNDRRMC) are duplicated here and in the backend; extract the
shared request/response interfaces (targetScope, reachPlan, evidencePack, and
the literal union types like 'direct'|'ndrrmc_escalation' and
'email'|'sms'|'portal') into a shared package/module, export them, and replace
the inline type literals in these functions with imports from that shared module
so both client and functions/src/callables/mass-alert.ts reference the same
canonical types; update imports and build configs accordingly to ensure both
packages compile against the shared types.
In `@docs/learnings.md`:
- Line 88: Change the wording at Line 88 from "SpyOn `mockImplementation` on
`collRef.where`" to use standard phrasing/casing—e.g. "Spy on `collRef.where`
and mock its implementation in Firestore admin SDK tests: the `.where` method
signature changed in firebase-admin v12+, so use `vi.spyOn(collRef, 'where' as
any)` to bypass TypeScript overload resolution that causes `TS2345: Target
signature provides too few arguments`."
In `@functions/src/callables/mass-alert.ts`:
- Around line 111-136: The code currently trusts client-supplied reach metrics
(input.reachPlan) when writing the mass_alert_requests document; instead, use
the server-computed reach plan from serverPreview (result of
massAlertReachPlanPreviewCore) or fail on mismatch: replace uses of
input.reachPlan.{fcmCount,smsCount} (and any estimatedReach derived from
input.reachPlan) with values from serverPreview.reachPlan, or compare
input.reachPlan to serverPreview.reachPlan and throw/return an error if they
differ before calling db.collection('mass_alert_requests').doc(requestId).set;
ensure the field estimatedReach is computed from serverPreview.reachPlan so
client cannot persist arbitrary numbers.
- Around line 126-153: The mass-alert flow currently marks mass_alert_requests
as sent and only calls sendMassAlertFcm, so SMS-consented recipients counted in
estimatedReach (set on the document) are never queued; update the code path
around the document write and subsequent sends (references: sendMassAlertFcm,
estimatedReach, mass_alert_requests, requestId) to also queue SMS sends when
input.reachPlan.smsCount > 0 by calling the existing SMS send/queue function (or
implement sendMassAlertSms) and await or handle its promise, and only set
status:'sent' after both FCM and SMS send/queue operations are initiated or mark
status per-channel (e.g., sent_fcm / queued_sms) and log/send errors from SMS
the same way FCM errors are handled. Ensure the mass_alert_requests document
reflects actual queued channels and errors so SMS audiences are not omitted.
- Around line 244-256: The status check and subsequent snap.ref.update() are not
atomic, allowing race conditions; wrap the read/check/update in a Firestore
transaction (db.runTransaction) or use an update precondition based on the
document version (e.g., precondition with snap.updateTime) so only one
concurrent actor can transition from 'pending_ndrrmc_review' to
'forwarded_to_ndrrmc'. Specifically, inside the transaction read the document
for requestId, verify it exists and status === 'pending_ndrrmc_review', then
perform the update setting status, forwardedAt, forwardedBy (actor.uid),
forwardMethod (input.forwardMethod), and ndrrrcRecipient
(input.ndrrrcRecipient); if the check fails, abort/throw to return the
appropriate error.
- Around line 51-71: The current code uses responder document counts
(fcmSnap.data().count) which undercounts FCM tokens because responders may have
multiple tokens; change the FCM counting logic to iterate responder documents
and sum the lengths of each responder.fcmTokens array (or the number of token
entries) to derive fcmTokenCount (use the existing symbol names fcmSnap -> fetch
responder docs, compute fcmTokenCount instead of fcmCount), and similarly ensure
smsCount counts phone entries (e.g., lengths of phone arrays or tokenized
entries) rather than document count; then compute total = fcmTokenCount +
smsCount and keep the route decision using MAX_DIRECT_ROUTE and municipalityIds
as before, capping the summed token counts early (e.g., stop summing once >
MAX_DIRECT_ROUTE) to avoid reading unnecessary data.
- Around line 210-214: The current call to sendMassAlertFcm with
input.targetScope.municipalityIds is sending the "review required" notification
to field responders (sendMassAlertFcm filters by responder municipality), but
this should go to provincial/NDRRMC reviewers who can act on
pending_ndrrmc_review; update the call to target the reviewer audience
instead—either invoke the FCM helper that filters by reviewer role (e.g., a
send-to-reviewers variant) or change the sendMassAlertFcm parameters to include
the reviewer role/region (province or reviewer role filter) so the notification
is delivered to NDRRMC/provincial reviewers rather than responders (reference:
sendMassAlertFcm, input.targetScope.municipalityIds, pending_ndrrmc_review).
In `@functions/src/services/fcm-mass-send.ts`:
- Around line 23-37: Add a defensive early guard in sendMassAlertFcm to check
opts.municipalityIds (handle undefined/null and empty array) before performing
the Firestore query: if the list is empty, short-circuit and return an empty
MassSendResult (i.e., no tokens processed) instead of running the
.where('municipalityId', 'in', ...) query; place this check immediately before
the code that constructs snaps so the function exits early when there are no
municipality IDs.
- Around line 39-55: The code currently appends raw fcmTokens from snaps.docs
into allTokens which can produce duplicates and then silently truncates at
MAX_BATCHES*TOKEN_BATCH_SIZE when slicing into batches (using TOKEN_BATCH_SIZE
and MAX_BATCHES), so deduplicate tokens after aggregation (e.g., use a Set to
build uniqueTokens from allTokens), compute totalUnique = uniqueTokens.size,
then limit to allowedCount = MAX_BATCHES * TOKEN_BATCH_SIZE and explicitly take
the first allowedCount tokens for batching; record droppedCount = totalUnique -
allowedCount and include that information in the returned result (or log it via
getMessaging/process logger) and ensure batch loop uses the deduplicated array
(not allTokens) and still updates successCount/failureCount/batchCount
accordingly.
In `@functions/src/services/fcm-send.ts`:
- Around line 94-101: The current update uses stale pre-send tokens and can
clear hasFcmToken incorrectly; wrap the removal and flag recompute in a
Firestore transaction: inside a transaction read the latest responder document
(adminDb.collection('responders').doc(uid)), compute the remaining tokens by
taking snapshot.data().fcmTokens and filtering out invalidTokens, then call
transaction.update on that doc with FieldValue.arrayRemove(...invalidTokens) and
set hasFcmToken to remaining.length > 0 so the flag is based on the latest
snapshot rather than the old tokens variable.
In `@functions/src/services/send-sms.ts`:
- Around line 103-109: EnqueueBroadcastSmsArgs currently types providerId as a
plain string which allows invalid IDs into sms_outbox; change
EnqueueBroadcastSmsArgs.providerId from string to the specific provider ID type
used elsewhere in this file (the typed provider contract declared near the top,
e.g., SmsProviderId or the exact exported type used in other SMS functions),
update the import/exports to reference that type, and adjust any callsites that
construct EnqueueBroadcastSmsArgs to pass the typed provider value so only
supported provider IDs are allowed.
In `@infra/firebase/firestore.rules`:
- Line 406: The update rule currently allows updates with only isSuperadmin(),
causing suspended superadmins to still modify mass alert requests; modify the
rule so it requires both isSuperadmin() && isActivePrivileged() (matching the
checks used at lines with similar rules) by updating the allow update condition
to include isActivePrivileged(), ensuring the same guard as other
superadmin-protected rules.
In `@packages/shared-validators/src/sms-templates.ts`:
- Around line 47-50: The mass_alert SMS templates (mass_alert.tl and
mass_alert.en) use an em dash (—) which forces UCS-2; replace the em dash with
GSM-7-safe punctuation such as a simple hyphen (-) or colon (:) and ensure
spacing remains correct (e.g., "ALERTO: {municipalityName} - {body}" or "ALERT:
{municipalityName} - {body}") so the templates remain GSM-7 friendly.
---
Outside diff comments:
In `@functions/src/services/send-sms.ts`:
- Around line 50-57: VALID_PURPOSES currently includes 'mass_alert', which lets
buildEnqueueSmsPayload accept mass alerts and route them through renderTemplate
(publicRef template contract) instead of the broadcast renderer; remove
'mass_alert' from the VALID_PURPOSES set (or adjust the validation in
buildEnqueueSmsPayload) so mass_alert is not permitted by that gate, and ensure
mass_alert flows only through the broadcast renderer path (the
broadcast-specific renderer function) or its dedicated enqueue flow.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 84ae635b-b1bb-4a90-9a03-313fbc2a3e15
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (35)
apps/admin-desktop/package.jsonapps/admin-desktop/src/__tests__/analytics-dashboard.test.tsxapps/admin-desktop/src/__tests__/mass-alert-modal.test.tsxapps/admin-desktop/src/__tests__/shift-handoff-modal.test.tsxapps/admin-desktop/src/__tests__/triage-queue.test.tsxapps/admin-desktop/src/pages/AnalyticsDashboardPage.tsxapps/admin-desktop/src/pages/MassAlertModal.tsxapps/admin-desktop/src/pages/TriageQueuePage.tsxapps/admin-desktop/src/routes.tsxapps/admin-desktop/src/services/callables.tsapps/responder-app/src/hooks/useRegisterFcmToken.tsdocs/learnings.mddocs/progress.mdfunctions/src/__tests__/callables/mass-alert.test.tsfunctions/src/__tests__/rules/mass-alert-requests.rules.test.tsfunctions/src/__tests__/services/fcm-send.test.tsfunctions/src/__tests__/triggers/analytics-snapshot-writer.test.tsfunctions/src/__tests__/triggers/process-inbox-item-prc2.test.tsfunctions/src/callables/mass-alert.tsfunctions/src/index.tsfunctions/src/scheduled/analytics-snapshot-writer.tsfunctions/src/services/fcm-mass-send.tsfunctions/src/services/fcm-send.tsfunctions/src/services/send-sms.tsfunctions/src/triggers/process-inbox-item.tsinfra/firebase/firestore.rulesinfra/firebase/firestore.rules.templatepackages/shared-data/src/index.tspackages/shared-validators/src/coordination.tspackages/shared-validators/src/index.tspackages/shared-validators/src/reports.test.tspackages/shared-validators/src/reports.tspackages/shared-validators/src/responders.test.tspackages/shared-validators/src/responders.tspackages/shared-validators/src/sms-templates.ts
Contributor
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 `@functions/src/__tests__/callables/mass-alert.test.ts`:
- Around line 377-392: The test currently only checks that sendMassAlertFcm was
called; strengthen it by asserting the call count and the key payload fields to
ensure correct routing and content. After invoking
requestMassAlertEscalationCore, assert mockFcm was called exactly once (or
expected number) and validate the first call's arguments include expected
title/body/data/targetScope/idempotencyKey (use sendMassAlertFcm mock calls or
toHaveBeenCalledWith to check specific fields), referencing sendMassAlertFcm and
requestMassAlertEscalationCore to find where to update the assertions. Ensure
you unwrap/match nested objects (e.g., targetScope.municipalityIds) rather than
only checking invocation.
- Around line 417-418: Rename the misspelled parameter ndrrrcRecipient to
ndrrmcRecipient everywhere to match the rest of the codebase: update the
parameter name in the forwardMassAlertToNDRRMCCore function signature, adjust
the callable validation/schema for forwardMassAlertToNDRRMC to expect
ndrrmcRecipient, change the admin-desktop service type definition to use
ndrrmcRecipient, and fix all related tests (including the failing assertions in
the mass-alert.test cases) to pass the corrected key. Ensure all usages, logs,
and forwards reference ndrrmcRecipient consistently.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 6df27835-8dfc-4f35-a865-ac41ad514365
📒 Files selected for processing (1)
functions/src/__tests__/callables/mass-alert.test.ts
Comment on lines
+377
to
+392
| it('FCMs provincial superadmins', async () => { | ||
| const { sendMassAlertFcm } = await import('../../services/fcm-mass-send.js') | ||
| const mockFcm = vi.mocked(sendMassAlertFcm) | ||
| mockFcm.mockClear() | ||
| await requestMassAlertEscalationCore( | ||
| adminDb, | ||
| { | ||
| message: 'Alert', | ||
| targetScope: { municipalityIds: ['daet'] }, | ||
| evidencePack: { linkedReportIds: [] }, | ||
| idempotencyKey: crypto.randomUUID(), | ||
| }, | ||
| muniAdminActor, | ||
| ) | ||
| expect(mockFcm).toHaveBeenCalled() | ||
| }) |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Strengthen the FCM escalation assertion to validate payload, not just invocation.
toHaveBeenCalled() alone can pass even if title/body/data routing regresses. Assert the call count and key payload fields to lock contract behavior.
Suggested test hardening
it('FCMs provincial superadmins', async () => {
const { sendMassAlertFcm } = await import('../../services/fcm-mass-send.js')
const mockFcm = vi.mocked(sendMassAlertFcm)
mockFcm.mockClear()
await requestMassAlertEscalationCore(
adminDb,
{
message: 'Alert',
targetScope: { municipalityIds: ['daet'] },
evidencePack: { linkedReportIds: [] },
idempotencyKey: crypto.randomUUID(),
},
muniAdminActor,
)
- expect(mockFcm).toHaveBeenCalled()
+ expect(mockFcm).toHaveBeenCalledTimes(1)
+ expect(mockFcm).toHaveBeenCalledWith(
+ adminDb,
+ expect.objectContaining({
+ municipalityIds: ['daet'],
+ title: 'NDRRMC Escalation Request',
+ data: expect.objectContaining({ type: 'escalation_review' }),
+ }),
+ )
})As per coding guidelines: "Write tests that verify the new code is actually invoked, not tests that pass trivially."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| it('FCMs provincial superadmins', async () => { | |
| const { sendMassAlertFcm } = await import('../../services/fcm-mass-send.js') | |
| const mockFcm = vi.mocked(sendMassAlertFcm) | |
| mockFcm.mockClear() | |
| await requestMassAlertEscalationCore( | |
| adminDb, | |
| { | |
| message: 'Alert', | |
| targetScope: { municipalityIds: ['daet'] }, | |
| evidencePack: { linkedReportIds: [] }, | |
| idempotencyKey: crypto.randomUUID(), | |
| }, | |
| muniAdminActor, | |
| ) | |
| expect(mockFcm).toHaveBeenCalled() | |
| }) | |
| it('FCMs provincial superadmins', async () => { | |
| const { sendMassAlertFcm } = await import('../../services/fcm-mass-send.js') | |
| const mockFcm = vi.mocked(sendMassAlertFcm) | |
| mockFcm.mockClear() | |
| await requestMassAlertEscalationCore( | |
| adminDb, | |
| { | |
| message: 'Alert', | |
| targetScope: { municipalityIds: ['daet'] }, | |
| evidencePack: { linkedReportIds: [] }, | |
| idempotencyKey: crypto.randomUUID(), | |
| }, | |
| muniAdminActor, | |
| ) | |
| expect(mockFcm).toHaveBeenCalledTimes(1) | |
| expect(mockFcm).toHaveBeenCalledWith( | |
| adminDb, | |
| expect.objectContaining({ | |
| municipalityIds: ['daet'], | |
| title: 'NDRRMC Escalation Request', | |
| data: expect.objectContaining({ type: 'escalation_review' }), | |
| }), | |
| ) | |
| }) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@functions/src/__tests__/callables/mass-alert.test.ts` around lines 377 - 392,
The test currently only checks that sendMassAlertFcm was called; strengthen it
by asserting the call count and the key payload fields to ensure correct routing
and content. After invoking requestMassAlertEscalationCore, assert mockFcm was
called exactly once (or expected number) and validate the first call's arguments
include expected title/body/data/targetScope/idempotencyKey (use
sendMassAlertFcm mock calls or toHaveBeenCalledWith to check specific fields),
referencing sendMassAlertFcm and requestMassAlertEscalationCore to find where to
update the assertions. Ensure you unwrap/match nested objects (e.g.,
targetScope.municipalityIds) rather than only checking invocation.
Comment on lines
+417
to
+418
| ndrrrcRecipient: 'ndrrmc@gov.ph', | ||
| }, |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify recipient field naming consistency for mass-alert forwarding contract.
# 1) Find both spellings with context across TS/TSX.
rg -nP -C3 '\bndrrrcRecipient\b|\bndrrmcRecipient\b' --type ts --type tsx
# 2) Inspect callable definition and schema for forward API input/output keys.
rg -nP -C4 'forwardMassAlertToNDRRMC|forwardMassAlertToNDRRMCCore|z\.object\(' functions/src/callables/mass-alert.ts
# 3) Inspect admin desktop caller payload fields (if present in this PR).
rg -nP -C4 'forwardMethod|ndrrr?mcRecipient|forwardMassAlertToNDRRMC' apps/admin-desktopRepository: Exc1D/bantayog-alert
Length of output: 2704
🏁 Script executed:
# Read the test file to see context around lines 417-418, 432-433, 440
cat -n functions/src/__tests__/callables/mass-alert.test.ts | sed -n '410,450p'Repository: Exc1D/bantayog-alert
Length of output: 1775
🏁 Script executed:
# Search for any comments or documentation about NDRRMC/NDRRRC in the codebase
rg -nP 'NDRR[MC]|ndrrr?mc' functions/src/callables/mass-alert.ts -A 2 -B 2Repository: Exc1D/bantayog-alert
Length of output: 2336
🌐 Web query:
NDRRMC Philippines disaster alert system
💡 Result:
The National Disaster Risk Reduction and Management Council (NDRRMC) of the Philippines oversees the country's disaster alert system, primarily through the National Disaster Risk Reduction and Management Operations Center (NDRRMOC). The core component is the Emergency Alert and Warning Messages (EAWM) system, governed by a Standard Operating Procedure (SOP) outlined in NDRRMC Memorandum No. 78 s. 2021. This system disseminates near real-time warnings to the public via free mobile alerts under Republic Act (RA) 10639, "The Free Mobile Disaster Alerts Act," and complements RA 10121, the Philippine DRRM Act of 2010, which mandates a national early warning and emergency alert system across broadcast, wireless, and landline media. Key elements include: - Triggered by hazard parameters from warning agencies (e.g., PAGASA for weather, PHIVOLCS for seismic/volcanic). - NDRRMOC as the central hub for monitoring, coordination, and issuance. - Integration with tools like PhilAWARE (multi-hazard early warning platform institutionalized in 2024 NDRP and NDRRMOC SOPG), HazardHunterPH (public hazard assessment), Project NOAH (flood/landslide forecasting), and emerging systems like Unified 911. - Uses alert levels (12 levels noted in NDRP 2024) and response clusters under the National Disaster Response Plan (NDRP) 2024 for coordinated action. - Updated procedures in NDRRMOC SOPG 2024 cover emergency alerts, evacuation, and multi-hazard response. The system emphasizes proactive, people-centered warnings to enable LGU actions, reducing disaster impacts. Official site: ndrrmc.gov.ph.
Citations:
- 1: https://www.un-spider.org/philippines-national-disaster-risk-reduction-and-management-council-ndrrmc
- 2: https://ndrrmc.gov.ph/attachments/article/4125/National_Disaster_Response_Plan_NDRP_2024.pdf
- 3: https://ndrrmc.gov.ph/attachments/article/3/NDRRMC_Memo_No_78_s_2021.pdf
- 4: https://www.pdc.org/typhoons-preparedness-in-the-philippines-why-monitoring-and-early-warning-matter/
- 5: https://www.scribd.com/document/856371336/Copy-of-NDRRMOC-SOPG-2024
- 6: https://hazardhunter.georisk.gov.ph/
- 7: https://www.pdc.org/philaware-proves-indispensable-to-responders/
- 8: https://www.ocd.gov.ph
- 9: https://ndrrmc.gov.ph/
- 10: https://en.wikipedia.org/wiki/National_Disaster_Risk_Reduction_and_Management_Council
- 11: https://dromic.dswd.gov.ph/ndrp/
- 12: https://ndrrmc.gov.ph/15-ndrrmc-issuances/4249-ndrrmc-memorandum-s-2024.html
Fix parameter name spelling: ndrrrcRecipient should be ndrrmcRecipient.
The codebase consistently uses triple-r spelling (ndrrrcRecipient) in the callable contract, but this is a typo. The official entity is NDRRMC (National Disaster Risk Reduction and Management Council), and all other references in the code correctly use double-r spelling: function names (forwardMassAlertToNDRRMC), status fields (pending_ndrrmc_review, forwarded_to_ndrrmc), email domain (ndrrmc@gov.ph), and log messages. Update the parameter name across:
forwardMassAlertToNDRRMCCorefunction signatureforwardMassAlertToNDRRMCcallable schema validation- Admin-desktop service type definition
- All test cases (lines 417, 432, 440)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@functions/src/__tests__/callables/mass-alert.test.ts` around lines 417 - 418,
Rename the misspelled parameter ndrrrcRecipient to ndrrmcRecipient everywhere to
match the rest of the codebase: update the parameter name in the
forwardMassAlertToNDRRMCCore function signature, adjust the callable
validation/schema for forwardMassAlertToNDRRMC to expect ndrrmcRecipient, change
the admin-desktop service type definition to use ndrrmcRecipient, and fix all
related tests (including the failing assertions in the mass-alert.test cases) to
pass the corrected key. Ensure all usages, logs, and forwards reference
ndrrmcRecipient consistently.
- mass-alert.ts: cap municipalityIds at 10 (Firestore in-limit), count individual FCM tokens not docs, use server-computed reachPlan, queue SMS broadcasts, remove wrong-audience escalation FCM, atomic transaction for forwardMassAlertToNDRRMCCore - fcm-mass-send.ts: guard empty municipalityIds, deduplicate tokens, chunk in-queries to 10, hard-cap refusal at 5000 tokens - fcm-send.ts: transactional hasFcmToken recompute from latest snapshot - send-sms.ts: type providerId as 'semaphore' | 'globelabs' union
- AnalyticsDashboardPage: read scope summaries at /{date}/{scope}/summary
instead of root docs that have no data
- MassAlertModal: stable idempotency keys via useRef, regenerated only
when message changes to prevent duplicate sends on retry
- TriageQueuePage: guard modalOpen and Mass Alert button when municipalityId
is absent to prevent blocked keyboard navigation without visible UI
- mass-alert.test: verify server-computed reach (not client input), verify escalation does NOT FCM responders, add SMS queueing test - fcm-send.test: mock runTransaction for hasFcmToken recompute tests - fcm-mass-send.test: new file — empty guard, dedup, chunking, hard-cap refusal, batching, error handling - analytics-dashboard.test: assert query scoping for municipalityId - mass-alert-modal.test: assert mock call arguments (message, scope)
- firestore.rules: add isActivePrivileged() to superadmin-only update rule for mass_alert_requests (prevents suspended superadmins) - firestore.rules.template: same fix for template consistency - sms-templates.ts: replace UCS-2 em dash with GSM-7-safe hyphen to reduce SMS segment count/cost for mass_alert broadcasts
- Change 'SpyOn' to 'Spy on' for standard phrasing/casing
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (2)
apps/admin-desktop/src/__tests__/analytics-dashboard.test.tsx (1)
58-65:⚠️ Potential issue | 🟡 MinorThis still doesn't verify municipality scoping.
getCountFromServerbeing called with some query object is not enough—the test will still pass if thewhere('municipalityId', '==', 'daet')filter disappears. Assert on the mockedwhere/queryarguments so the municipality constraint is part of the expectation. As per coding guidelines, "Write tests that verify the new code is actually invoked, not tests that pass trivially."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/admin-desktop/src/__tests__/analytics-dashboard.test.tsx` around lines 58 - 65, The test currently only checks that mockGetCountFromServer was called, not that it included the municipality filter; update the assertion to verify the actual query includes where('municipalityId', '==', <expectedId>) by inspecting the argument passed to mockGetCountFromServer (reference mockGetCountFromServer) and asserting the query contains the municipality constraint (either by asserting a mocked where helper was called with 'municipalityId' and the expected value, or by checking the queryArg object for the field/filter entry). Ensure you use the same expected municipality id used in the test render (e.g., 'daet' or the wrapper's municipalityId) and replace the loose expect(queryArg).toBeDefined() with a concrete assertion that the municipality filter exists.functions/src/callables/mass-alert.ts (1)
273-276: 🧹 Nitpick | 🔵 TrivialTODO tracks missing reviewer notification - consider creating an issue.
The TODO correctly identifies that escalation notifications should target
provincial_superadminreviewers, not responders. This is an important feature gap for the escalation workflow to function properly.Would you like me to open an issue to track implementing the reviewer notification channel?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@functions/src/callables/mass-alert.ts` around lines 273 - 276, The TODO notes missing reviewer notification for escalation; implement a separate notification path that queries users with role == 'provincial_superadmin' (and any NDRRMC reviewer roles) and sends them targeted FCMs instead of using sendMassAlertFcm which targets responders by municipality; add a new function (e.g. sendReviewerNotifications or sendEscalationFcm) that performs the DB query for users by role, builds reviewer-specific payloads, and calls the FCM send routine, then invoke this from the escalation flow in mass-alert.ts where the TODO is now so superadmins receive escalation alerts.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/admin-desktop/src/pages/AnalyticsDashboardPage.tsx`:
- Around line 78-95: The bars are currently capped with barH = Math.min(total,
70) which flattens values >70; compute the maximum total across snapshots first
(e.g., derive maxTotal from snapshots by mapping reportsByStatus -> totals and
using Math.max) and then scale each bar height proportionally to that max (barH
= maxTotal > 0 ? Math.round((total / maxTotal) * MAX_HEIGHT) : 0), keeping
MAX_HEIGHT as the current 70; update the mapping that produces barH and ensure
you guard against division by zero so days with zero totals render height 0.
In `@apps/admin-desktop/src/pages/MassAlertModal.tsx`:
- Around line 69-86: The escalation always sends an empty evidencePack; update
handleEscalate to pass real evidence by reading new/existing state fields (e.g.,
linkedReportIds, pagasaSignalRef, notes) instead of hardcoded { linkedReportIds:
[] } and wire those fields into the requestMassAlertEscalation call (keep
message, targetScope, and idempotencyKey the same). Add or use UI state (e.g.,
linkedReportIds state array, pagasaSignalRef string, notes string) and include
them in evidencePack: { linkedReportIds, pagasaSignalRef, notes } when calling
requestMassAlertEscalation in handleEscalate.
In `@apps/admin-desktop/src/pages/TriageQueuePage.tsx`:
- Around line 142-150: The button currently uses disabled={!canOpenMassAlert}
which makes the onClick branch that calls setBanner unreachable; instead remove
the disabled prop (or make the button enabled) and keep the onClick logic so
clicking when !canOpenMassAlert triggers setBanner('Mass Alert is only available
for municipality-scoped admins.') and clicking when canOpenMassAlert calls
setMassAlertOpen(true); update the button element containing canOpenMassAlert,
setBanner, and setMassAlertOpen accordingly so the informative banner is shown
on user click.
In `@functions/src/services/fcm-mass-send.ts`:
- Around line 38-55: The current aggregation flattens tokens into tokenSet
losing ownership, so instead build and populate a Map<string, string[]> (or
Map<string, Set<string>>) that maps each FCM token to its owner responder
document id while iterating in the loop that currently creates tokenSet (replace
tokenSet with this token→owner map in the block that reads responders and
fcmTokens); pass the flattened token list to sendEachForMulticast but keep the
token→owner map to translate per-token failures returned by sendEachForMulticast
back to responder IDs, then apply the same transactional cleanup pattern used in
functions/src/services/fcm-send.ts to remove invalid tokens and update
hasFcmToken per responder atomically.
In `@infra/firebase/firestore.rules.template`:
- Around line 379-386: The current allow create rule for this workflow/audit
collection (the allow create block that uses isActivePrivileged(),
isSuperadmin(), isMuniAdmin(), myMunicipality(), and allowedCreateStatus()) is
too permissive because it only checks role/scope and status; update it so
clients cannot inject arbitrary fields or pre-mark a request as sent: either (A)
deny client-side creates entirely (remove/replace the allow create path so only
callable/cloud functions can create documents), or (B) implement strict
server-side validation by whitelisting exact writable fields and constraining
status to pre-processing values inside allowedCreateStatus(), plus ensure
requestedByMunicipality equality check remains; choose one approach and apply it
to the allow create rule to prevent clients from setting arbitrary metadata or
terminal statuses.
In `@packages/shared-validators/src/sms-templates.ts`:
- Line 10: renderTemplate currently accepts all SmsPurpose values (including
'mass_alert' from SmsPurpose/RenderArgs) but only replaces {publicRef}, so
callers can pass purpose: 'mass_alert' and receive un-rendered placeholders;
update renderTemplate (the renderTemplate function referenced in this diff) to
either restrict its input to non-broadcast purposes or explicitly throw when
args.purpose === 'mass_alert'. Concretely: in renderTemplate, check the incoming
RenderArgs.purpose (or SmsPurpose) and if it equals 'mass_alert' throw a clear
error like "renderTemplate cannot render 'mass_alert' templates" (or narrow the
function's type signature to exclude 'mass_alert'), ensuring no code path
returns a template with unreplaced placeholders; apply the same change to the
analogous code block around lines 74-88 mentioned in the comment.
---
Duplicate comments:
In `@apps/admin-desktop/src/__tests__/analytics-dashboard.test.tsx`:
- Around line 58-65: The test currently only checks that mockGetCountFromServer
was called, not that it included the municipality filter; update the assertion
to verify the actual query includes where('municipalityId', '==', <expectedId>)
by inspecting the argument passed to mockGetCountFromServer (reference
mockGetCountFromServer) and asserting the query contains the municipality
constraint (either by asserting a mocked where helper was called with
'municipalityId' and the expected value, or by checking the queryArg object for
the field/filter entry). Ensure you use the same expected municipality id used
in the test render (e.g., 'daet' or the wrapper's municipalityId) and replace
the loose expect(queryArg).toBeDefined() with a concrete assertion that the
municipality filter exists.
In `@functions/src/callables/mass-alert.ts`:
- Around line 273-276: The TODO notes missing reviewer notification for
escalation; implement a separate notification path that queries users with role
== 'provincial_superadmin' (and any NDRRMC reviewer roles) and sends them
targeted FCMs instead of using sendMassAlertFcm which targets responders by
municipality; add a new function (e.g. sendReviewerNotifications or
sendEscalationFcm) that performs the DB query for users by role, builds
reviewer-specific payloads, and calls the FCM send routine, then invoke this
from the escalation flow in mass-alert.ts where the TODO is now so superadmins
receive escalation alerts.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 85786871-7274-4ce3-ae2f-990c643f6b68
📒 Files selected for processing (16)
apps/admin-desktop/src/__tests__/analytics-dashboard.test.tsxapps/admin-desktop/src/__tests__/mass-alert-modal.test.tsxapps/admin-desktop/src/pages/AnalyticsDashboardPage.tsxapps/admin-desktop/src/pages/MassAlertModal.tsxapps/admin-desktop/src/pages/TriageQueuePage.tsxdocs/learnings.mdfunctions/src/__tests__/callables/mass-alert.test.tsfunctions/src/__tests__/services/fcm-mass-send.test.tsfunctions/src/__tests__/services/fcm-send.test.tsfunctions/src/callables/mass-alert.tsfunctions/src/services/fcm-mass-send.tsfunctions/src/services/fcm-send.tsfunctions/src/services/send-sms.tsinfra/firebase/firestore.rulesinfra/firebase/firestore.rules.templatepackages/shared-validators/src/sms-templates.ts
Comment on lines
+69
to
+86
| const handleEscalate = () => { | ||
| setLoading(true) | ||
| void (async () => { | ||
| try { | ||
| await callables.requestMassAlertEscalation({ | ||
| message, | ||
| targetScope: { municipalityIds: [municipalityId] }, | ||
| evidencePack: { linkedReportIds: [] }, | ||
| idempotencyKey: escalateKeyRef.current, | ||
| }) | ||
| onClose() | ||
| } catch (err: unknown) { | ||
| setError(err instanceof Error ? err.message : 'Escalation failed') | ||
| } finally { | ||
| setLoading(false) | ||
| } | ||
| })() | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider allowing evidence attachment for escalations.
The evidencePack is hardcoded with empty linkedReportIds. For meaningful NDRRMC escalations, consider adding UI to link related reports or attach notes. The schema already supports pagasaSignalRef and notes fields.
Would you like me to draft a UI extension for evidence collection, or open an issue to track this enhancement?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/admin-desktop/src/pages/MassAlertModal.tsx` around lines 69 - 86, The
escalation always sends an empty evidencePack; update handleEscalate to pass
real evidence by reading new/existing state fields (e.g., linkedReportIds,
pagasaSignalRef, notes) instead of hardcoded { linkedReportIds: [] } and wire
those fields into the requestMassAlertEscalation call (keep message,
targetScope, and idempotencyKey the same). Add or use UI state (e.g.,
linkedReportIds state array, pagasaSignalRef string, notes string) and include
them in evidencePack: { linkedReportIds, pagasaSignalRef, notes } when calling
requestMassAlertEscalation in handleEscalate.
- firestore.rules + template: deny client creates on mass_alert_requests
(callables use Admin SDK; no client write path is intended). Remove
dead allowedCreateStatus() function.
- sms-templates.ts: export DirectSmsPurpose = Exclude<SmsPurpose,'mass_alert'>
and narrow RenderArgs.purpose to it, so renderTemplate() cannot receive
a broadcast purpose with un-renderable placeholders.
- send-sms.ts: adopt DirectSmsPurpose for EnqueueSmsArgs.purpose; remove
'mass_alert' from VALID_PURPOSES (broadcast uses enqueueBroadcastSms).
- fcm-mass-send.ts: replace tokenSet with Map<token,ownerIds> to preserve
ownership, collect invalid tokens from sendEachForMulticast responses, and
clean up stale fcmTokens/hasFcmToken per-responder after send.
- AnalyticsDashboardPage.tsx: normalize SVG bar heights against maxTotal
instead of capping at 70 so trend is meaningful when values exceed 70.
- MassAlertModal.tsx: wire pagasaSignalRef and notes into evidencePack for
NDRRMC escalation (schema already supported them; UI fields now expose them).
- TriageQueuePage.tsx: remove disabled={!canOpenMassAlert} from Mass Alert
button so the informative banner branch is reachable on click.
- analytics-dashboard.test.tsx: hoist mockWhere and assert the municipality
filter (where('municipalityId','==','daet')) is actually applied.
- shared-validators/lib: rebuild to include renderBroadcastTemplate and
DirectSmsPurpose in emitted declarations.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 10
♻️ Duplicate comments (1)
apps/admin-desktop/src/pages/MassAlertModal.tsx (1)
148-158:⚠️ Potential issue | 🟠 MajorRegenerate escalation idempotency key when evidence fields change.
evidencePackincludespagasaSignalRefandnotes(Lines 80-82), but changes in Lines 148-158 do not rotateescalateKeyRef. That can replay stale escalation payload under the same key.Proposed fix
<input id="pagasa-signal-ref" type="text" value={pagasaSignalRef} onChange={(e) => { setPagasaSignalRef(e.target.value) + escalateKeyRef.current = crypto.randomUUID() }} /> @@ <textarea id="escalation-notes" value={notes} onChange={(e) => { setNotes(e.target.value) + escalateKeyRef.current = crypto.randomUUID() }} rows={2} />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/admin-desktop/src/pages/MassAlertModal.tsx` around lines 148 - 158, The idempotency key (escalateKeyRef) must be regenerated whenever evidence fields change: update the onChange handlers for pagasaSignalRef (setPagasaSignalRef) and notes (setNotes) to also rotate the escalateKeyRef, or add a useEffect that watches evidencePack fields (pagasaSignalRef and notes) and sets a new escalateKeyRef (e.g., via the same key generator used elsewhere) whenever they change; ensure you reference and update the existing escalateKeyRef state so each edit yields a fresh idempotency key and prevents replay of stale escalation payloads.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/admin-desktop/src/__tests__/analytics-dashboard.test.tsx`:
- Around line 38-42: The beforeEach currently sets mockResolvedValue/Return for
mockGetCountFromServer, mockGetDocs and mockWhere but does not clear prior call
history, so add calls to clear the mocks' call history at the start of the
beforeEach (e.g., mockGetCountFromServer.mockClear(), mockGetDocs.mockClear(),
mockWhere.mockClear()) before reassigning mockResolvedValue/mockReturnValue so
assertions that inspect calls to mockWhere (and others) reflect only the current
test's activity; target the mocks by their identifiers mockGetCountFromServer,
mockGetDocs and mockWhere.
- Around line 40-41: The current tests stub mockGetDocs to always return an
empty docs array so the per-date summary/trend path is never exercised; update
the test to add one non-empty snapshot case by making
mockGetDocs.mockResolvedValue return an object with a non-empty docs array (each
doc should mimic the Firestore DocumentSnapshot shape with a data() method
returning the summary/date fields your component expects) and keep
mockWhere.mockReturnValue returning the query chain as before; then render the
AnalyticsDashboard component (or call the same test helper used elsewhere) and
add a snapshot assertion so the trend-fetch code path behind mockGetDocs and
mockWhere is actually executed and verified.
In `@apps/admin-desktop/src/pages/AnalyticsDashboardPage.tsx`:
- Around line 86-108: The current conditional shows "<p>No snapshot data
yet.</p>" whenever snapshots is falsy even while data is still being fetched;
update the render logic around snapshots (the ternary that checks snapshots &&
snapshots.length > 0) to also check a loading flag (e.g., isLoadingSnapshots or
similar) and only show the empty-state paragraph when loading is false and
snapshots is empty; if you don't already have a loading state, add one to the
component that tracks the fetch lifecycle and use it to render a loader or
nothing while snapshots are loading instead of the "No snapshot data yet."
message.
In `@functions/src/services/fcm-mass-send.ts`:
- Around line 43-47: The responders query in fcm-mass-send.ts currently filters
only by hasFcmToken and municipalityId (see db.collection('responders') and the
snaps variable); add an explicit active-filter by appending .where('isActive',
'==', true) to the query so deactivated responders don't receive mass alerts (or
alternatively, add a short comment above the query documenting the intentional
decision to include inactive responders if that is desired).
- Around line 123-138: The cleanup Promise.all over ownerToInvalidTokens can
reject if any db.runTransaction fails, which will abort all cleanups; change to
per-owner error handling by either wrapping each transaction in try/catch
(inside the map) or use Promise.allSettled to log failures without throwing, and
optionally collect failed ownerIds/tokens and surface them in MassSendResult so
callers (mass-alert.ts) can observe cleanup errors; refer to
ownerToInvalidTokens, db.runTransaction, FieldValue.arrayRemove and
MassSendResult when implementing the change.
In `@infra/firebase/firestore.rules.template`:
- Around line 381-384: The current rule allows client-side updates via "allow
update: if isSuperadmin() && isActivePrivileged()", which opens an unintended
client write path; either remove that client update allowance (change to "allow
update: if false") to force callable-only writes, or replace it with a strict
predicate that uses affectedKeys() and explicit status/field-transition checks
(e.g., verify only a small whitelist of keys can change and validate old/new
status transitions) while still requiring isSuperadmin() &&
isActivePrivileged(); reference the isSuperadmin(), isActivePrivileged(), and
affectedKeys() helpers when implementing the stricter predicate.
In `@packages/shared-validators/lib/coordination.js`:
- Around line 82-88: The evidencePack nested validator currently allows unknown
keys; update the zod schema for evidencePack (the z.object({...}) inside the
evidencePack property) to be strict so unknown keys cause validation failures:
replace the current z.object({...}) with z.object({...}).strict() while keeping
the outer .optional() intact (i.e., evidencePack:
z.object({...}).strict().optional()), ensuring malformed or extra fields are
rejected.
In `@packages/shared-validators/lib/reports.test.js`:
- Around line 361-374: Add a test that verifies followUpConsent: false is
accepted to ensure the schema enforces a boolean (not just truthy) — in the test
file where inboxPayloadSchema and basePayload are used, add an it() case similar
to the existing 'accepts followUpConsent boolean' that passes followUpConsent:
false along with a valid contact (e.g., contact: { phone: '+639171234567',
smsConsent: true }) and assert inboxPayloadSchema.parse(...) does not throw;
this ensures the schema (inboxPayloadSchema) accepts both true and false values.
In `@packages/shared-validators/lib/sms-templates.js`:
- Around line 53-61: The renderBroadcastTemplate function currently interpolates
args.vars.municipalityName and args.vars.body without guards; update
renderBroadcastTemplate to validate that args.vars exists and that
municipalityName and body are present and non-empty (e.g., not
undefined/null/empty string) before performing .replace; if validation fails,
throw an SmsTemplateError with a clear message referencing the missing field(s)
so callers can handle the error; keep the existing TEMPLATES.mass_alert lookup
and only perform interpolation after successful validation.
In `@packages/shared-validators/src/sms-templates.ts`:
- Around line 82-91: renderBroadcastTemplate currently only validates locale but
will accept empty municipalityName/body and produce bad SMS; update
renderBroadcastTemplate (and use BroadcastRenderArgs and SmsTemplateError) to
trim args.vars.municipalityName and args.vars.body, validate they are non-empty
after trimming, and throw a SmsTemplateError with a clear message (e.g.,
"Missing municipalityName" or "Missing body") if either is empty; then use the
trimmed values for the .replace() calls so replacements never insert blank
content.
---
Duplicate comments:
In `@apps/admin-desktop/src/pages/MassAlertModal.tsx`:
- Around line 148-158: The idempotency key (escalateKeyRef) must be regenerated
whenever evidence fields change: update the onChange handlers for
pagasaSignalRef (setPagasaSignalRef) and notes (setNotes) to also rotate the
escalateKeyRef, or add a useEffect that watches evidencePack fields
(pagasaSignalRef and notes) and sets a new escalateKeyRef (e.g., via the same
key generator used elsewhere) whenever they change; ensure you reference and
update the existing escalateKeyRef state so each edit yields a fresh idempotency
key and prevents replay of stale escalation payloads.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: a43dd432-cc7c-4b05-99cd-cad47a0e4ba9
⛔ Files ignored due to path filters (12)
packages/shared-validators/lib/coordination.d.ts.mapis excluded by!**/*.mappackages/shared-validators/lib/coordination.js.mapis excluded by!**/*.mappackages/shared-validators/lib/index.d.ts.mapis excluded by!**/*.mappackages/shared-validators/lib/index.js.mapis excluded by!**/*.mappackages/shared-validators/lib/reports.d.ts.mapis excluded by!**/*.mappackages/shared-validators/lib/reports.js.mapis excluded by!**/*.mappackages/shared-validators/lib/reports.test.js.mapis excluded by!**/*.mappackages/shared-validators/lib/responders.d.ts.mapis excluded by!**/*.mappackages/shared-validators/lib/responders.js.mapis excluded by!**/*.mappackages/shared-validators/lib/responders.test.js.mapis excluded by!**/*.mappackages/shared-validators/lib/sms-templates.d.ts.mapis excluded by!**/*.mappackages/shared-validators/lib/sms-templates.js.mapis excluded by!**/*.map
📒 Files selected for processing (22)
apps/admin-desktop/src/__tests__/analytics-dashboard.test.tsxapps/admin-desktop/src/pages/AnalyticsDashboardPage.tsxapps/admin-desktop/src/pages/MassAlertModal.tsxapps/admin-desktop/src/pages/TriageQueuePage.tsxfunctions/src/services/fcm-mass-send.tsfunctions/src/services/send-sms.tsinfra/firebase/firestore.rulesinfra/firebase/firestore.rules.templatepackages/shared-validators/lib/coordination.d.tspackages/shared-validators/lib/coordination.jspackages/shared-validators/lib/index.d.tspackages/shared-validators/lib/index.jspackages/shared-validators/lib/reports.d.tspackages/shared-validators/lib/reports.jspackages/shared-validators/lib/reports.test.jspackages/shared-validators/lib/responders.d.tspackages/shared-validators/lib/responders.jspackages/shared-validators/lib/responders.test.jspackages/shared-validators/lib/sms-templates.d.tspackages/shared-validators/lib/sms-templates.jspackages/shared-validators/src/index.tspackages/shared-validators/src/sms-templates.ts
Comment on lines
+82
to
+88
| evidencePack: z | ||
| .object({ | ||
| linkedReportIds: z.array(z.string()), | ||
| pagasaSignalRef: z.string().optional(), | ||
| notes: z.string().max(2000).optional(), | ||
| }) | ||
| .optional(), |
Contributor
There was a problem hiding this comment.
Harden evidencePack validation by making the nested object strict.
Line 82-Line 88 currently strips unknown keys inside evidencePack silently. For escalation evidence, that can mask malformed inputs instead of failing fast.
Suggested fix
evidencePack: z
.object({
linkedReportIds: z.array(z.string()),
pagasaSignalRef: z.string().optional(),
notes: z.string().max(2000).optional(),
})
+ .strict()
.optional(),As per coding guidelines: "Use defensive programming: validate external input at boundaries and never swallow errors with empty catch blocks."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| evidencePack: z | |
| .object({ | |
| linkedReportIds: z.array(z.string()), | |
| pagasaSignalRef: z.string().optional(), | |
| notes: z.string().max(2000).optional(), | |
| }) | |
| .optional(), | |
| evidencePack: z | |
| .object({ | |
| linkedReportIds: z.array(z.string()), | |
| pagasaSignalRef: z.string().optional(), | |
| notes: z.string().max(2000).optional(), | |
| }) | |
| .strict() | |
| .optional(), |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/shared-validators/lib/coordination.js` around lines 82 - 88, The
evidencePack nested validator currently allows unknown keys; update the zod
schema for evidencePack (the z.object({...}) inside the evidencePack property)
to be strict so unknown keys cause validation failures: replace the current
z.object({...}) with z.object({...}).strict() while keeping the outer
.optional() intact (i.e., evidencePack: z.object({...}).strict().optional()),
ensuring malformed or extra fields are rejected.
Comment on lines
+361
to
+374
| it('accepts followUpConsent boolean', () => { | ||
| expect(() => inboxPayloadSchema.parse({ | ||
| ...basePayload, | ||
| contact: { phone: '+639171234567', smsConsent: true }, | ||
| followUpConsent: true, | ||
| })).not.toThrow(); | ||
| }); | ||
| it('rejects non-boolean followUpConsent', () => { | ||
| expect(() => inboxPayloadSchema.parse({ | ||
| ...basePayload, | ||
| contact: { phone: '+639171234567', smsConsent: true }, | ||
| followUpConsent: 'yes', | ||
| })).toThrow(); | ||
| }); |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Add a positive test for followUpConsent: false to fully lock the boolean contract.
Right now the suite proves true passes and string fails; adding false prevents regressions that accidentally narrow this to a truthy/literal-true rule.
Suggested test addition
it('accepts followUpConsent boolean', () => {
@@
});
+ it('accepts followUpConsent=false', () => {
+ expect(() => inboxPayloadSchema.parse({
+ ...basePayload,
+ contact: { phone: '+639171234567', smsConsent: true },
+ followUpConsent: false,
+ })).not.toThrow();
+ });
it('rejects non-boolean followUpConsent', () => {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| it('accepts followUpConsent boolean', () => { | |
| expect(() => inboxPayloadSchema.parse({ | |
| ...basePayload, | |
| contact: { phone: '+639171234567', smsConsent: true }, | |
| followUpConsent: true, | |
| })).not.toThrow(); | |
| }); | |
| it('rejects non-boolean followUpConsent', () => { | |
| expect(() => inboxPayloadSchema.parse({ | |
| ...basePayload, | |
| contact: { phone: '+639171234567', smsConsent: true }, | |
| followUpConsent: 'yes', | |
| })).toThrow(); | |
| }); | |
| it('accepts followUpConsent boolean', () => { | |
| expect(() => inboxPayloadSchema.parse({ | |
| ...basePayload, | |
| contact: { phone: '+639171234567', smsConsent: true }, | |
| followUpConsent: true, | |
| })).not.toThrow(); | |
| }); | |
| it('accepts followUpConsent=false', () => { | |
| expect(() => inboxPayloadSchema.parse({ | |
| ...basePayload, | |
| contact: { phone: '+639171234567', smsConsent: true }, | |
| followUpConsent: false, | |
| })).not.toThrow(); | |
| }); | |
| it('rejects non-boolean followUpConsent', () => { | |
| expect(() => inboxPayloadSchema.parse({ | |
| ...basePayload, | |
| contact: { phone: '+639171234567', smsConsent: true }, | |
| followUpConsent: 'yes', | |
| })).toThrow(); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/shared-validators/lib/reports.test.js` around lines 361 - 374, Add a
test that verifies followUpConsent: false is accepted to ensure the schema
enforces a boolean (not just truthy) — in the test file where inboxPayloadSchema
and basePayload are used, add an it() case similar to the existing 'accepts
followUpConsent boolean' that passes followUpConsent: false along with a valid
contact (e.g., contact: { phone: '+639171234567', smsConsent: true }) and assert
inboxPayloadSchema.parse(...) does not throw; this ensures the schema
(inboxPayloadSchema) accepts both true and false values.
Comment on lines
+53
to
+61
| export function renderBroadcastTemplate(args) { | ||
| const purposeMap = TEMPLATES.mass_alert; | ||
| const template = purposeMap[args.locale]; | ||
| if (!template) { | ||
| throw new SmsTemplateError(`Unknown locale: ${args.locale}`); | ||
| } | ||
| return template | ||
| .replace('{municipalityName}', args.vars.municipalityName) | ||
| .replace('{body}', args.vars.body); |
Contributor
There was a problem hiding this comment.
Validate broadcast vars before interpolation.
Line 60 and Line 61 interpolate values without runtime guards. If municipalityName or body is empty/undefined at runtime, malformed public alerts can be sent.
Suggested fix
export function renderBroadcastTemplate(args) {
const purposeMap = TEMPLATES.mass_alert;
const template = purposeMap[args.locale];
if (!template) {
throw new SmsTemplateError(`Unknown locale: ${args.locale}`);
}
+ const municipalityName = args?.vars?.municipalityName?.trim();
+ const body = args?.vars?.body?.trim();
+ if (!municipalityName || !body) {
+ throw new SmsTemplateError('Missing municipalityName or body');
+ }
return template
- .replace('{municipalityName}', args.vars.municipalityName)
- .replace('{body}', args.vars.body);
+ .replace('{municipalityName}', municipalityName)
+ .replace('{body}', body);
}As per coding guidelines: "Use defensive programming: validate external input at boundaries and never swallow errors with empty catch blocks."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/shared-validators/lib/sms-templates.js` around lines 53 - 61, The
renderBroadcastTemplate function currently interpolates
args.vars.municipalityName and args.vars.body without guards; update
renderBroadcastTemplate to validate that args.vars exists and that
municipalityName and body are present and non-empty (e.g., not
undefined/null/empty string) before performing .replace; if validation fails,
throw an SmsTemplateError with a clear message referencing the missing field(s)
so callers can handle the error; keep the existing TEMPLATES.mass_alert lookup
and only perform interpolation after successful validation.
- mass-alert: regenerate idempotency key when evidence fields change - analytics-dashboard: add mockClear in beforeEach, add non-empty snapshot test - analytics-page: add loading state for snapshots during fetch - fcm-mass-send: add isActive filter, use Promise.allSettled for cleanup - firestore.rules: close client update path (allow update: if false) - coordination: add strict() to evidencePack schema - reports.test: add followUpConsent=false test case - sms-templates: validate and trim municipalityName/body before interpolation
Contributor
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/admin-desktop/src/__tests__/analytics-dashboard.test.tsx`:
- Around line 50-59: The test is stalling the wrong dependency: it mocks
mockGetCountFromServer but the test name says "snapshot data" so it never
exercises the snapshot-loading path; update the test to stall the snapshot fetch
mock used by AnalyticsDashboardPage (replace mockGetCountFromServer with the
snapshot-specific mock — e.g., mockGetSnapshotFromServer or whatever mock
corresponds to snapshot fetching in your test harness), ensure the rendered
component shows the snapshot loading state text, and rename the test to reflect
the mocked path (or alternatively, if you intend to test the page-level loading,
change the test name to "shows a loading state while analytics count is
fetching"). Ensure you target the AnalyticsDashboardPage snapshot-fetching
function and assert the snapshot-specific loading UI.
- Around line 19-28: The Firestore mock is missing doc() and getDoc() used by
AnalyticsDashboardPage and the test fixture documents lack an id field; add
mocked exports for doc and getDoc in the vi.mock replacement (matching the
existing mock helpers like mockGetDocs/mockGetCountFromServer) and update the
test snapshot/document fixture to include an id property so
AnalyticsDashboardPage can read d.id to build the reference path.
In `@apps/admin-desktop/src/pages/AnalyticsDashboardPage.tsx`:
- Around line 16-24: The project currently has two differing definitions of
"active" statuses causing inconsistent analytics: replace the local array
ACTIVE_STATUSES in AnalyticsDashboardPage.tsx with a shared constant (e.g.,
ACTIVE_REPORT_STATUSES) and import/use that same constant from the shared module
in apps/admin-desktop/src/hooks/useMuniReports.ts so both AnalyticsDashboardPage
and the useMuniReports hook reference the identical list; update exports where
you create ACTIVE_REPORT_STATUSES and adjust any import paths to use the unified
constant.
In `@apps/admin-desktop/src/pages/MassAlertModal.tsx`:
- Around line 53-54: The retry flows start new async requests with
setLoading(true) but never clear previous errors, so call setError(null)
immediately before each request begins (e.g., in the async IIFE that follows
setLoading(true) and likewise before the second request around lines that start
the escalate flow). Update the code paths that kick off send/escalate (identify
the async IIFE or functions wrapping setLoading(true) and the escalate/send
handlers) to call setError(null) just prior to setting loading and performing
the request so stale error state is cleared on retry.
- Around line 35-38: Trim and validate outbound text inputs at the UI boundary
before calling callables.massAlertReachPlanPreview (and the send/escalate
paths): ensure message is not empty or whitespace-only (e.g., message =
message?.trim() and reject if length === 0) and ensure notes is trimmed and
capped to the backend limit (e.g., if notes?.trim().length > 2000, show
validation error or truncate to 2000 chars) prior to invoking the RPC; also add
user-facing validation feedback and avoid empty catch blocks by logging or
surfacing errors in the preview/send/escalate handlers referenced around the
massAlertReachPlanPreview call and the related send/escalate functions.
In `@functions/src/services/fcm-mass-send.ts`:
- Around line 140-152: The current cleanup summary can overstate removals
because when failedCount > 0 you still log "Removed ..." using invalidTokens and
ownerToInvalidTokens; change the logging to accurately reflect partial failures:
inside the block where failedCount is computed and handled (variables/results:
failedCount, results), either (a) update the subsequent summary log to say
"Attempted to remove X invalid token(s) from Y responder(s); Z cleanup
transactions failed" or (b) split into two logs—one reporting successful
removals (compute successfulCount = total - failedCount) and one reporting
failures—so that the log messages involving invalidTokens and
ownerToInvalidTokens are not misleading.
- Around line 50-53: Validate the runtime shape of respDoc.data().fcmTokens
before treating it as string[]: check Array.isArray(tokens) and that every
element is a non-empty string (e.g., via tokens.every(t => typeof t === 'string'
&& t.trim() !== '')) and if the check fails skip/clean up that doc and log a
warning; apply the same validation before using currentTokens.filter(...) and in
the other similar block handling fcmTokens, and avoid swallowing errors—surface
or log exceptions instead of empty catches so malformed data doesn't produce
incorrect fanout or silent failures.
In `@infra/firebase/firestore.rules.template`:
- Around line 376-380: Add read-rule tests covering the new authorization path:
in functions/src/__tests__/rules/mass-alert-requests.rules.test.ts, add
assertions that access to read the mass-alert request document is allowed for an
active muni admin when myMunicipality() equals
resource.data.requestedByMunicipality (exercising isMassAlertMuniAdmin() and
isActivePrivileged()), allowed for an active superadmin (isSuperadmin() &&
isActivePrivileged()), and denied for inactive privileged accounts
(isActivePrivileged() false) even if muni admin or superadmin. Use the existing
test helpers/setup in that file to create request documents with
requestedByMunicipality set to match or not-match and add explicit
assertSucceeds/assertFails (or the repository’s equivalent) for each scenario.
Ensure you cover both matching and non-matching municipality cases so the
isMassAlertMuniAdmin() path is validated.
In `@packages/shared-validators/src/coordination.ts`:
- Line 88: The schema field linkedReportIds currently uses z.array(z.string())
which permits empty strings; update the validation to reject empty IDs by
changing it to z.array(z.string().min(1).trim()) (or at minimum
z.array(z.string().min(1))) so each item is non-empty and trimmed; locate the
linkedReportIds entry in the Zod schema in coordination.ts and replace the
validator accordingly, then run tests/type checks to ensure no other code
expects empty strings.
In `@packages/shared-validators/src/sms-templates.ts`:
- Around line 82-84: In renderBroadcastTemplate, validate and normalize
args.vars before calling .trim(): ensure args and args.vars exist, coerce
municipalityName and body to strings (e.g., fallback to empty string when
undefined/null or non-string) and then run the existing empty checks so
malformed input doesn't throw raw errors; update references to
args.vars.municipalityName and args.vars.body accordingly and throw the existing
SmsTemplateError where appropriate if values are empty after normalization.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 72552c6e-3d0b-4f9a-9b15-a1b794f0d186
📒 Files selected for processing (9)
apps/admin-desktop/src/__tests__/analytics-dashboard.test.tsxapps/admin-desktop/src/pages/AnalyticsDashboardPage.tsxapps/admin-desktop/src/pages/MassAlertModal.tsxfunctions/src/services/fcm-mass-send.tsinfra/firebase/firestore.rulesinfra/firebase/firestore.rules.templatepackages/shared-validators/src/coordination.tspackages/shared-validators/src/reports.test.tspackages/shared-validators/src/sms-templates.ts
Comment on lines
+82
to
+84
| export function renderBroadcastTemplate(args: BroadcastRenderArgs): string { | ||
| const municipalityName = args.vars.municipalityName.trim() | ||
| const body = args.vars.body.trim() |
Contributor
There was a problem hiding this comment.
Guard malformed vars before calling .trim().
This exported validator still throws a raw runtime error if a caller passes vars: undefined or non-string fields, so bad input can bypass SmsTemplateError. Normalize the nested values first, then reuse the existing empty checks.
Suggested hardening
export function renderBroadcastTemplate(args: BroadcastRenderArgs): string {
- const municipalityName = args.vars.municipalityName.trim()
- const body = args.vars.body.trim()
+ const municipalityName =
+ typeof args?.vars?.municipalityName === 'string' ? args.vars.municipalityName.trim() : ''
+ const body = typeof args?.vars?.body === 'string' ? args.vars.body.trim() : ''As per coding guidelines, "Defensive Programming: Assume external input is malicious/broken. Validate at the boundary. Never swallow errors with an empty catch block."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/shared-validators/src/sms-templates.ts` around lines 82 - 84, In
renderBroadcastTemplate, validate and normalize args.vars before calling
.trim(): ensure args and args.vars exist, coerce municipalityName and body to
strings (e.g., fallback to empty string when undefined/null or non-string) and
then run the existing empty checks so malformed input doesn't throw raw errors;
update references to args.vars.municipalityName and args.vars.body accordingly
and throw the existing SmsTemplateError where appropriate if values are empty
after normalization.
- analytics-dashboard: add doc/getDoc mocks, fix loading test stall - unify ACTIVE_REPORT_STATUSES in shared-types, update imports - MassAlertModal: clear error before retry, trim/validate inputs - fcm-mass-send: normalizeFcmTokens helper, fix cleanup log messaging - mass-alert-requests rules: add read-rule tests - coordination: linkedReportIds min(1) validation - sms-templates: add missing vars 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
2 participants
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
Implements Phase 5 PRE-C and Cluster C: mass alert broadcast system, SMS template infrastructure, and daily analytics snapshots.
PRE-C (Schema + Maintenance)
hasFcmTokenflag maintenance — settrueon token registration,falsewhen all tokens invalidreportSmsConsentDocSchemawithmunicipalityId+followUpConsent; updatedprocessInboxItemmaterializationmassAlertRequestDocSchemastatus enum (8 states: queued → ndrrmc flow); Firestore rules for NDRRMC escalation pathC.1 (Mass Alert Callables + UI)
mass_alertSMS template +renderBroadcastTemplate+enqueueBroadcastSms(nopublicRefrequirement)sendMassAlertFcm— batched FCM multicast (500-token batches, 5000 cap)massAlertReachPlanPreview,sendMassAlert,requestMassAlertEscalation,forwardMassAlertToNDRRMCMassAlertModal— reach preview, direct send, NDRRMC escalation CTAC.2 (Analytics Snapshot + Dashboard)
CAMARINES_NORTE_MUNICIPALITY_IDSin shared-dataanalyticsSnapshotWriterscheduled CF — dailycount()aggregate per municipality + province-wideAnalyticsDashboardPagewith React Query + inline SVG 7-day trend chart +/analyticsrouteTest plan
firebase emulators:exec— 31/31 function tests pass (PRE-C.2, rules, callables, analytics)npx turbo run lint typecheck— 26/26 cleanFiles changed
36 files, +2226 / -24 lines
Summary by Sourcery
Add mass alert broadcast capabilities, daily analytics snapshots, and related UI and schema updates for Phase 5 Cluster C.
New Features:
Bug Fixes:
Enhancements:
Documentation:
Tests:
Summary by CodeRabbit
New Features
Improvements
Tests
Documentation