Conversation
…r 3b lifecycle states
…RECONDITION, NOT_FOUND)
…ve + negative cases
…i, not-verified, off-shift
Runtime code writes `assignedTo: { uid, agencyId, municipalityId }` but the
schema had flat top-level `responderId`, `municipalityId`, `agencyId` fields.
All actual dispatch documents would fail validation against the old schema.
Updated both schema and tests to use the nested `assignedTo` object shape.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
dispatchResponderCore checks `responder.isActive !== true` and
getEligibleResponders queries `.where('isActive', '==', true)`, but the
schema had no isActive field — only availabilityStatus (different
semantics). Actual responder documents would fail schema validation.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
onSnapshot had no error callback — Firestore errors were silently discarded,
leaving the UI with stale data. In a disaster alert system, a responder not
knowing their dispatch list is broken could cost lives.
Now returns { rows, error } matching the pattern in useMuniReports.
DispatchListPage surfaces the error to the user.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…ying on cast Replace unsafe `report.status as 'awaiting_verify'` cast with an explicit runtime check. The prior code would throw a generic "invalid transition" error for any non-awaiting_verify source state, including terminal states like 'new'. Now throws a specific error clearly indicating the precondition. Also remove now-unused isValidReportTransition import. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…mapping Previously rejectReport threw BantayogErrorCode.INVALID_STATUS_TRANSITION when rejecting a non-awaiting_verify report, but the test and spec expected FAILED_PRECONDITION. Both codes map to HTTP 'failed-precondition' so the client-side behavior was correct, but the error code was wrong. Changes: - Add FAILED_PRECONDITION to BantayogErrorCode enum (shared-validators) - Map FAILED_PRECONDITION → 'failed-precondition' in BANTAYOG_TO_HTTPS_CODE - Change rejectReportCore to throw FAILED_PRECONDITION (not INVALID_STATUS_TRANSITION) when report is not in awaiting_verify state - Update test assertion in reject-report.test.ts to match actual code (was already correct; FAILED_PRECONDITION is the actual enum value) - Update errors-and-logging.test.ts count from 18 → 19 (added FAILED_PRECONDITION) Note: cancel-dispatch.test.ts lines 122 and 162 check 'FAILED_PRECONDITION' and 'INVALID_STATUS_TRANSITION' respectively — these match the actual implementation (line 74 throws FAILED_PRECONDITION, line 81 throws INVALID_STATUS_TRANSITION). No changes needed there. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Reviewer's GuideImplements Phase 3b admin triage + dispatch: new callable functions for verifying/rejecting reports and creating/cancelling dispatches with idempotency, rate limiting, muni scoping and logging; shared validators and rate-limit/responder-eligibility services; admin desktop triage UI and responder app dispatch list wired to these callables and Firestore/RTDB; infra metric for dispatch.created; plus seed factories, tests, scripts, and docs to cover the new flows. Sequence diagram for admin verify and dispatch flowsequenceDiagram
actor Admin
participant AdminDesktop
participant FirebaseAuth
participant Firestore
participant RTDB
participant verifyReportCF as verifyReport_callable
participant dispatchResponderCF as dispatchResponder_callable
participant ResponderApp
Admin->>AdminDesktop: Open TriageQueuePage
AdminDesktop->>FirebaseAuth: onAuthStateChanged
FirebaseAuth-->>AdminDesktop: Admin user with claims (role, municipalityId)
AdminDesktop->>Firestore: onSnapshot reports where municipalityId and status in queue
Firestore-->>AdminDesktop: Queue of reports
Admin->>AdminDesktop: Click report row
AdminDesktop->>Firestore: onSnapshot reports/{reportId}, report_ops/{reportId}
Firestore-->>AdminDesktop: ReportDetailPanel data
Admin->>AdminDesktop: Click Verify (status new)
AdminDesktop->>verifyReportCF: verifyReport(reportId, idempotencyKey)
verifyReportCF->>Firestore: rate_limits verifyReport key check
Firestore-->>verifyReportCF: Rate limit ok
verifyReportCF->>Firestore: Transaction load reports/{reportId}
Firestore-->>verifyReportCF: status new
verifyReportCF->>Firestore: Update status awaiting_verify
verifyReportCF->>Firestore: Insert report_events new to awaiting_verify
verifyReportCF-->>AdminDesktop: { status awaiting_verify, reportId }
Firestore-->>AdminDesktop: onSnapshot status awaiting_verify
Admin->>AdminDesktop: Click Verify again (status awaiting_verify)
AdminDesktop->>verifyReportCF: verifyReport(reportId, new idempotencyKey)
verifyReportCF->>Firestore: Rate limit check
verifyReportCF->>Firestore: Transaction load report status awaiting_verify
verifyReportCF->>Firestore: Update status verified, verifiedBy, verifiedAt
verifyReportCF->>Firestore: Insert report_events awaiting_verify to verified
verifyReportCF-->>AdminDesktop: { status verified, reportId }
Firestore-->>AdminDesktop: onSnapshot status verified
Admin->>AdminDesktop: Click Dispatch
AdminDesktop->>RTDB: onValue responder_index/{municipalityId}
AdminDesktop->>Firestore: onSnapshot responders where municipalityId and isActive true
RTDB-->>AdminDesktop: On shift flags
Firestore-->>AdminDesktop: Eligible responders
Admin->>AdminDesktop: Pick responder and confirm
AdminDesktop->>dispatchResponderCF: dispatchResponder(reportId, responderUid, idempotencyKey)
dispatchResponderCF->>Firestore: rate_limits dispatchResponder key check
dispatchResponderCF->>RTDB: Get responder_index municipality shift
dispatchResponderCF->>Firestore: Transaction load reports/{reportId}, responders/{responderUid}
Firestore-->>dispatchResponderCF: Report status verified, responder isActive true
dispatchResponderCF->>RTDB: Recheck responder_index for TOCTOU
RTDB-->>dispatchResponderCF: isOnShift true
dispatchResponderCF->>Firestore: Create dispatches/{dispatchId} pending with acknowledgementDeadlineAt
dispatchResponderCF->>Firestore: Update reports/{reportId} status assigned, currentDispatchId
dispatchResponderCF->>Firestore: Insert report_events verified to assigned
dispatchResponderCF->>Firestore: Insert dispatch_events null to pending
dispatchResponderCF-->>AdminDesktop: { dispatchId, status pending, reportId }
Firestore-->>AdminDesktop: onSnapshot report status assigned
ResponderApp->>FirebaseAuth: signInWithEmailAndPassword
FirebaseAuth-->>ResponderApp: user with role responder
ResponderApp->>Firestore: onSnapshot dispatches where assignedTo.uid and status in active set
Firestore-->>ResponderApp: List including new pending dispatch
ER diagram for reports, dispatches, responders, and moderation dataerDiagram
REPORTS {
string reportId PK
string municipalityId
string municipalityLabel
string status
string severityDerived
string source
string currentDispatchId
string verifiedBy
number createdAt
number lastStatusAt
number verifiedAt
}
REPORT_OPS {
string reportId PK
number verifyQueuePriority
string assignedMunicipalityAdmins
}
REPORT_PRIVATE {
string reportId PK
string reporterUid
string rawDescription
}
REPORT_EVENTS {
string eventId PK
string reportId FK
string from
string to
string actor
string actorRole
string correlationId
number at
}
MODERATION_INCIDENTS {
string incidentId PK
string reportId FK
string reason
string notes
string actor
string actorRole
string correlationId
number at
}
RESPONDERS {
string uid PK
string municipalityId
string agencyId
string displayName
boolean isActive
}
DISPATCHES {
string dispatchId PK
string reportId FK
string status
string assignedTo_uid
string assignedTo_agencyId
string assignedTo_municipalityId
string dispatchedBy
string dispatchedByRole
number dispatchedAt
number lastStatusAt
number acknowledgementDeadlineAt
}
DISPATCH_EVENTS {
string eventId PK
string dispatchId FK
string reportId FK
string from
string to
string actor
string actorRole
string reason
string correlationId
number at
}
RATE_LIMITS {
string key PK
number timestamps
number updatedAt
}
RTDB_RESPONDER_INDEX {
string municipalityId PK
string responderUid PK
boolean isOnShift
}
REPORTS ||--|| REPORT_OPS : has_ops
REPORTS ||--|| REPORT_PRIVATE : has_private
REPORTS ||--o{ REPORT_EVENTS : has_events
REPORTS ||--o{ MODERATION_INCIDENTS : has_moderation
REPORTS ||--o{ DISPATCHES : has_dispatches
DISPATCHES ||--o{ DISPATCH_EVENTS : has_events
RESPONDERS ||--o{ DISPATCHES : may_be_assigned
RTDB_RESPONDER_INDEX ||--|| RESPONDERS : mirrors_shift
RATE_LIMITS ||..|| REPORTS : controls_verify
RATE_LIMITS ||..|| DISPATCHES : controls_dispatch
Class diagram for callables, core dependencies, and shared servicesclassDiagram
class VerifyReportCoreDeps {
+string reportId
+string idempotencyKey
+VerifyReportActor actor
+Timestamp now
}
class VerifyReportActor {
+string uid
+string role
+string municipalityId
+boolean active
}
class VerifyReportResult {
+string status
+string reportId
}
class RejectReportCoreDeps {
+string reportId
+string reason
+string notes
+string idempotencyKey
+SimpleActor actor
+Timestamp now
}
class DispatchResponderCoreDeps {
+string reportId
+string responderUid
+string idempotencyKey
+SimpleActor actor
+Timestamp now
}
class CancelDispatchCoreDeps {
+string dispatchId
+string reason
+string idempotencyKey
+SimpleActor actor
+Timestamp now
}
class SimpleActor {
+string uid
+string role
+string municipalityId
}
class RateLimitCheck {
+string key
+number limit
+number windowSeconds
+Timestamp now
+Timestamp updatedAt
}
class RateLimitResult {
+boolean allowed
+number remaining
+number retryAfterSeconds
}
class EligibleResponderServiceModel {
+string uid
+string displayName
+string agencyId
+string municipalityId
}
class VerifyReportFunction {
+verifyReportCore(db Firestore, deps VerifyReportCoreDeps) VerifyReportResult
}
class RejectReportFunction {
+rejectReportCore(db Firestore, deps RejectReportCoreDeps) VerifyReportResult
}
class DispatchResponderFunction {
+dispatchResponderCore(db Firestore, rtdb Database, deps DispatchResponderCoreDeps) DispatchResult
}
class CancelDispatchFunction {
+cancelDispatchCore(db Firestore, deps CancelDispatchCoreDeps) DispatchCancelResult
}
class RateLimitService {
+checkRateLimit(db Firestore, check RateLimitCheck) RateLimitResult
}
class ResponderEligibilityService {
+getEligibleResponders(db Firestore, rtdb Database, municipalityId string, agencyId string) EligibleResponderServiceModel[]
}
class DispatchResult {
+string dispatchId
+string status
+string reportId
}
class DispatchCancelResult {
+string dispatchId
+string status
}
VerifyReportCoreDeps --> VerifyReportActor
RejectReportCoreDeps --> SimpleActor
DispatchResponderCoreDeps --> SimpleActor
CancelDispatchCoreDeps --> SimpleActor
VerifyReportFunction --> VerifyReportCoreDeps
VerifyReportFunction --> RateLimitService
RejectReportFunction --> RejectReportCoreDeps
RejectReportFunction --> RateLimitService
DispatchResponderFunction --> DispatchResponderCoreDeps
DispatchResponderFunction --> RateLimitService
DispatchResponderFunction --> ResponderEligibilityService
CancelDispatchFunction --> CancelDispatchCoreDeps
CancelDispatchFunction --> RateLimitService
RateLimitService --> RateLimitCheck
RateLimitService --> RateLimitResult
ResponderEligibilityService --> EligibleResponderServiceModel
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 Phase 3b admin triage and dispatch: Firebase client/init + App Check and emulator wiring, auth providers and route protection, admin/responder React pages and realtime hooks, new callable functions (verify/reject/dispatch/cancel) with idempotency and rate-limiting, RTDB support, tests, validator updates, infra index, docs and scripts. Changes
Sequence Diagram(s)sequenceDiagram
participant Admin as Admin User
participant AdminApp as Admin Desktop App
participant Auth as AuthProvider
participant Functions as Cloud Functions
participant Firestore as Firestore
participant RTDB as Realtime DB
Admin->>AdminApp: sign in (email/password)
AdminApp->>Auth: signInWithEmailAndPassword
Auth->>Firestore: getIdToken / custom claims
Firestore-->>Auth: claims
Auth-->>AdminApp: authenticated
AdminApp->>Firestore: query reports (municipality + status)
Firestore-->>AdminApp: report list
Admin->>AdminApp: click Verify
AdminApp->>Functions: callables.verifyReport(reportId, idempotencyKey)
Functions->>Firestore: read/validate/update report & write report_events
Firestore-->>Functions: commit
Functions-->>AdminApp: { status, reportId }
Admin->>AdminApp: open Dispatch modal
AdminApp->>RTDB: read /responder_index/{municipality}
RTDB-->>AdminApp: shift states
AdminApp->>Functions: callables.dispatchResponder(reportId, responderUid, idempotencyKey)
Functions->>RTDB: verify on-shift
RTDB-->>Functions: confirmed
Functions->>Firestore: create dispatch, update report, write events
Firestore-->>Functions: commit
Functions-->>AdminApp: { dispatchId, status, reportId }
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![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 6 issues, and left some high level feedback:
- dispatchResponderCore assumes
severityDerivedis one oflow|medium|high, but your data model and seed helpers mentioncriticalas well; consider extendingDEADLINE_BY_SEVERITYor adding a safe fallback so acriticalseverity doesn’t produce an undefined/NaN acknowledgement deadline. - In cancelDispatchCore you treat non-pending states as a 3b-only restriction but surface them as INVALID_STATUS_TRANSITION; if you intend this to behave as a business precondition (per your docs/tests), consider using FAILED_PRECONDITION there so callers can distinguish "wrong lifecycle" from "invalid state machine edge."
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- dispatchResponderCore assumes `severityDerived` is one of `low|medium|high`, but your data model and seed helpers mention `critical` as well; consider extending `DEADLINE_BY_SEVERITY` or adding a safe fallback so a `critical` severity doesn’t produce an undefined/NaN acknowledgement deadline.
- In cancelDispatchCore you treat non-pending states as a 3b-only restriction but surface them as INVALID_STATUS_TRANSITION; if you intend this to behave as a business precondition (per your docs/tests), consider using FAILED_PRECONDITION there so callers can distinguish "wrong lifecycle" from "invalid state machine edge."
## Individual Comments
### Comment 1
<location path="infra/terraform/modules/monitoring/phase-3/main.tf" line_range="74-83" />
<code_context>
+resource "google_logging_metric" "dispatch_created" {
+ name = "${var.env}-bantayog-dispatch-created"
+ description = "Count of dispatches created via dispatchResponder"
+ filter = "resource.type=\"cloud_function\" AND jsonPayload.event=\"dispatch.created\" OR resource.type=\"cloud_run_revision\" AND jsonPayload.event=\"dispatch.created\""
+ metric_descriptor {
+ metric_kind = "DELTA"
+ value_type = "INT64"
+ unit = "1"
+ labels {
+ key = "municipality_id"
+ value_type = "STRING"
+ description = "Municipality the dispatch was created in"
+ }
+ }
+ label_extractors = {
+ "municipality_id" = "EXTRACT(jsonPayload.municipalityId)"
+ }
+}
</code_context>
<issue_to_address>
**issue (bug_risk):** Clarify log filter precedence and align label extractor with emitted payload shape
Two points:
1) The filter currently relies on implicit `AND`/`OR` precedence. To avoid misclassification if you ever change the combinations, add parentheses to make the intended grouping explicit.
2) `municipality_id` is extracted from `jsonPayload.municipalityId`, but the `dispatch.created` log payload doesn’t appear to expose `municipalityId` at that path. This will yield empty labels. Either include `municipalityId` in the `logEvent` data for `dispatch.created`, or update the extractor to match the actual field location in the log (e.g., under `jsonPayload.data.*`).
</issue_to_address>
### Comment 2
<location path="apps/responder-app/src/app/firebase.ts" line_range="12-13" />
<code_context>
+
+let _app: FirebaseApp | undefined
+
+export function getFirebaseApp(): FirebaseApp {
+ _app ??= initializeApp({ projectId: PROJECT_ID })
+ return _app
+}
</code_context>
<issue_to_address>
**issue (bug_risk):** Using `initializeApp({ projectId })` will break production web usage
For the web client SDK, `initializeApp({ projectId })` only works with emulators; in production it requires the full config (apiKey, authDomain, etc.) and will otherwise fail to initialize. Here `USE_EMULATOR` only switches emulator connections, not the app config.
Please use a full config object (e.g., from Vite env vars as in the admin-desktop app) and use `USE_EMULATOR` solely to toggle emulator connections so the same build works for both local and real Firebase.
</issue_to_address>
### Comment 3
<location path="apps/responder-app/src/hooks/useOwnDispatches.ts" line_range="17-20" />
<code_context>
+ const [rows, setRows] = useState<OwnDispatchRow[]>([])
+ const [error, setError] = useState<string | null>(null)
+ useEffect(() => {
+ if (!uid) {
+ return
+ }
+ const q = query(
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Clear stale dispatch data when the responder logs out or UID disappears
When `uid` becomes falsy (e.g. on sign-out), this effect exits without clearing `rows` or `error`, so the previous user’s dispatches stay visible until remount.
Consider resetting local state when `uid` is falsy:
```ts
if (!uid) {
setRows([])
setError(null)
return
}
```
so the UI immediately reflects the unauthenticated state.
```suggestion
useEffect(() => {
if (!uid) {
setRows([])
setError(null)
return
}
```
</issue_to_address>
### Comment 4
<location path="functions/src/__tests__/callables/cancel-dispatch.test.ts" line_range="27-36" />
<code_context>
+describe('cancelDispatchCore (3b branches)', () => {
</code_context>
<issue_to_address>
**suggestion (testing):** Extend cancelDispatchCore tests to cover NOT_FOUND and non-current dispatch scenarios
These tests exercise the main 3b flows, but two key branches in `cancelDispatchCore` are still untested:
- When the dispatch document is missing, `cancelDispatchCore` should throw with `code: 'NOT_FOUND'`. Please add a test calling it with a random `dispatchId` and asserting this error.
- When `report.currentDispatchId !== dispatchId`, the report status and `currentDispatchId` should not change and no `report_events` entry should be written. A test that seeds such a report + pending dispatch and verifies those invariants would protect against reverting unrelated dispatches.
Optionally, you could also add a `NOT_FOUND` case for a missing report (dispatch references a non‑existent `reportId`).
Suggested implementation:
```typescript
await testEnv.clearFirestore()
})
describe('cancelDispatchCore edge cases', () => {
it('throws NOT_FOUND when dispatch does not exist', async () => {
const db = testEnv.unauthenticatedContext().firestore() as any
const randomDispatchId = 'non-existent-dispatch'
await expect(
cancelDispatchCore(db, {
dispatchId: randomDispatchId,
// add any other required fields for cancelDispatchCore here (e.g. auth context)
}),
).rejects.toMatchObject({ code: 'NOT_FOUND' })
})
it('does not modify report or write events when canceling non-current dispatch', async () => {
const db = testEnv.unauthenticatedContext().firestore() as any
// Seed a report and two pending dispatches so that currentDispatchId !== dispatchId being cancelled
const { reportId } = await seedReportAtStatus(db, 'assigned', { municipalityId: 'daet' })
const { dispatchId: currentDispatchId } = await seedDispatch(db, {
reportId,
responderUid: 'r-current',
municipalityId: 'daet',
status: 'pending',
})
const { dispatchId: otherDispatchId } = await seedDispatch(db, {
reportId,
responderUid: 'r-other',
municipalityId: 'daet',
status: 'pending',
})
// Make report point at the "current" dispatch
await db.collection('reports').doc(reportId).update({
currentDispatchId,
status: 'assigned',
})
// Capture initial state
const beforeReportSnap = await db.collection('reports').doc(reportId).get()
const beforeReportData = beforeReportSnap.data()
const beforeEventsSnap = await db
.collection('report_events')
.where('reportId', '==', reportId)
.get()
// Attempt to cancel the non-current dispatch
await cancelDispatchCore(db, {
dispatchId: otherDispatchId,
// add any other required fields for cancelDispatchCore here (e.g. auth context)
})
const afterReportSnap = await db.collection('reports').doc(reportId).get()
const afterReportData = afterReportSnap.data()
const afterEventsSnap = await db
.collection('report_events')
.where('reportId', '==', reportId)
.get()
// Report status and currentDispatchId should not change
expect(afterReportData).toMatchObject(beforeReportData!)
// No new report_events entries should be written
expect(afterEventsSnap.size).toBe(beforeEventsSnap.size)
})
// Optionally, add a NOT_FOUND-for-report case once cancelDispatchCore's behavior is confirmed
})
describe('cancelDispatchCore (3b branches)', () => {
```
1. Ensure `cancelDispatchCore` is imported into this test file (e.g. from `../../src/callables/cancel-dispatch` or the appropriate module).
2. Fill in any additional parameters required by `cancelDispatchCore` in the two new test calls (e.g. auth/user context, municipalityId, etc.) so the function can execute without throwing unrelated errors.
3. If your schema uses different collection names or field names (e.g. `reportsV2`, `reportEvents`), update the `db.collection(...)` calls and field keys (`currentDispatchId`, `status`, `reportId`) accordingly.
4. If there are existing helpers for reading report events (e.g. `getReportEventsForReport(db, reportId)`), you may prefer to use them instead of raw `collection('report_events').where(...)` queries to stay consistent with the rest of the test suite.
5. If `cancelDispatchCore` returns a structured error instead of throwing, adapt the first test assertion from `rejects.toMatchObject` to the pattern used elsewhere in this test file (for example, checking a returned `error` object).
</issue_to_address>
### Comment 5
<location path="functions/src/__tests__/services/responder-eligibility.test.ts" line_range="20-29" />
<code_context>
+describe('getEligibleResponders', () => {
</code_context>
<issue_to_address>
**suggestion (testing):** Add tests for responders with no shift entry and the empty-result case
To fully cover `getEligibleResponders`, consider adding:
- **No shift entry in RTDB**: Seed an active responder in Firestore but omit the RTDB `/responder_index/...` node, and assert they are not returned, matching the current “RTDB is source of truth for on‑shift” behavior.
- **Empty result**: Add a case where no responders match (e.g., municipality or agency with none) and assert `result.length === 0`, to lock in behavior when filtering and sorting yield no results.
Suggested implementation:
```typescript
describe('getEligibleResponders', () => {
it('excludes active responders that have no on-shift entry in RTDB', async () => {
await testEnv.withSecurityRulesDisabled(async (ctx) => {
const db = ctx.firestore() as unknown as Firestore
const rtdb = ctx.database() as unknown as Database
// Active responder in municipality/agency but NO RTDB shift entry
await seedResponderDoc(db, {
uid: 'r-no-shift',
municipalityId: 'daet',
agencyId: 'bfp-daet',
isActive: true,
})
// Another responder that is actually on shift to prove the filter works
await seedResponderDoc(db, {
uid: 'r-on-shift',
municipalityId: 'daet',
agencyId: 'bfp-daet',
isActive: true,
})
await rtdb.ref('/responder_index/daet/bfp-daet/r-on-shift').set({
uid: 'r-on-shift',
status: 'on_shift',
})
const result = await getEligibleResponders({
firestore: db,
database: rtdb,
municipalityId: 'daet',
agencyId: 'bfp-daet',
})
const uids = result.map((r) => r.uid)
expect(uids).toContain('r-on-shift')
expect(uids).not.toContain('r-no-shift')
})
})
it('returns an empty array when no responders match the filters', async () => {
await testEnv.withSecurityRulesDisabled(async (ctx) => {
const db = ctx.firestore() as unknown as Firestore
const rtdb = ctx.database() as unknown as Database
// Seed responders in a different municipality/agency
await seedResponderDoc(db, {
uid: 'r-other',
municipalityId: 'vinzons',
agencyId: 'bfp-vinzons',
isActive: true,
})
await rtdb.ref('/responder_index/vinzons/bfp-vinzons/r-other').set({
uid: 'r-other',
status: 'on_shift',
})
const result = await getEligibleResponders({
firestore: db,
database: rtdb,
municipalityId: 'daet',
agencyId: 'bfp-daet',
})
expect(result).toHaveLength(0)
})
})
it('returns only active responders in the target municipality who are on shift', async () => {
```
These edits assume:
1. `getEligibleResponders` is imported into this test file and accepts an options object with `{ firestore, database, municipalityId, agencyId }` (or a compatible signature). Adjust the call shape if your actual function signature differs.
2. `seedResponderDoc` writes responder metadata to Firestore but does not implicitly create RTDB entries; RTDB shift entries are explicitly created via `rtdb.ref('/responder_index/...').set(...)`. If the RTDB path or payload shape differs in your codebase, update the `/responder_index/...` paths and data objects accordingly.
3. If your existing tests use different municipality/agency IDs or a factory/helper for RTDB seeding, mirror those conventions (e.g., reuse a `seedShiftEntry` helper instead of direct `rtdb.ref(...).set(...)`).
4. Place these new `it` blocks alongside the existing ones within the same `describe('getEligibleResponders', ...)` block, preserving any surrounding setup/teardown logic or shared helpers that are present in parts of the file not shown here.
</issue_to_address>
### Comment 6
<location path="docs/learnings.md" line_range="366" />
<code_context>
Commit `6546a0e` ("fix validators: cap pendingMediaIds at 20") was made by a subagent and appeared in `git log` but wasn't in the worktree's HEAD. It was on `main`. After `git rebase main`, it appeared at the correct position in history. Always rebase worktrees onto main before starting implementation to avoid this confusion.
+
+### `seedReportAtStatus` uses Firebase Admin Timestamp, incompatible with RulesTestContext
+
+`seedReportAtStatus` (seed-factories.ts) uses `firebase-admin/firestore` `Timestamp.now()` which is incompatible with RulesTestEnvironment's `withSecurityRulesDisabled` context (uses JS SDK). Error: `FirebaseError: Function DocumentReference.set() called with invalid data. Unsupported field value: a custom Timestamp object`. Fix: write inline seeding with numeric `ts` timestamps (like other rules tests) instead of calling `seedReportAtStatus`.
</code_context>
<issue_to_address>
**suggestion (typo):** Heading uses `RulesTestContext` while the body refers to `RulesTestEnvironment`
Please update the heading to use `RulesTestEnvironment` so it matches the term used in the explanation below and avoids confusion.
```suggestion
### `seedReportAtStatus` uses Firebase Admin Timestamp, incompatible with RulesTestEnvironment
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+74
to
+83
| filter = "resource.type=\"cloud_function\" AND jsonPayload.event=\"dispatch.created\" OR resource.type=\"cloud_run_revision\" AND jsonPayload.event=\"dispatch.created\"" | ||
| metric_descriptor { | ||
| metric_kind = "DELTA" | ||
| value_type = "INT64" | ||
| unit = "1" | ||
| labels { | ||
| key = "municipality_id" | ||
| value_type = "STRING" | ||
| description = "Municipality the dispatch was created in" | ||
| } |
There was a problem hiding this comment.
issue (bug_risk): Clarify log filter precedence and align label extractor with emitted payload shape
Two points:
-
The filter currently relies on implicit
AND/ORprecedence. To avoid misclassification if you ever change the combinations, add parentheses to make the intended grouping explicit. -
municipality_idis extracted fromjsonPayload.municipalityId, but thedispatch.createdlog payload doesn’t appear to exposemunicipalityIdat that path. This will yield empty labels. Either includemunicipalityIdin thelogEventdata fordispatch.created, or update the extractor to match the actual field location in the log (e.g., underjsonPayload.data.*).
Comment on lines
+12
to
+13
| export function getFirebaseApp(): FirebaseApp { | ||
| _app ??= initializeApp({ projectId: PROJECT_ID }) |
There was a problem hiding this comment.
issue (bug_risk): Using initializeApp({ projectId }) will break production web usage
For the web client SDK, initializeApp({ projectId }) only works with emulators; in production it requires the full config (apiKey, authDomain, etc.) and will otherwise fail to initialize. Here USE_EMULATOR only switches emulator connections, not the app config.
Please use a full config object (e.g., from Vite env vars as in the admin-desktop app) and use USE_EMULATOR solely to toggle emulator connections so the same build works for both local and real Firebase.
Comment on lines
+17
to
+20
| useEffect(() => { | ||
| if (!uid) { | ||
| return | ||
| } |
There was a problem hiding this comment.
suggestion (bug_risk): Clear stale dispatch data when the responder logs out or UID disappears
When uid becomes falsy (e.g. on sign-out), this effect exits without clearing rows or error, so the previous user’s dispatches stay visible until remount.
Consider resetting local state when uid is falsy:
if (!uid) {
setRows([])
setError(null)
return
}so the UI immediately reflects the unauthenticated state.
Suggested change
| useEffect(() => { | |
| if (!uid) { | |
| return | |
| } | |
| useEffect(() => { | |
| if (!uid) { | |
| setRows([]) | |
| setError(null) | |
| return | |
| } |
Comment on lines
+27
to
+36
| describe('cancelDispatchCore (3b branches)', () => { | ||
| it('cancels a pending dispatch and reverts report to verified', async () => { | ||
| const db = testEnv.unauthenticatedContext().firestore() as any | ||
| const { reportId } = await seedReportAtStatus(db, 'assigned', { municipalityId: 'daet' }) | ||
| const { dispatchId } = await seedDispatch(db, { | ||
| reportId, | ||
| responderUid: 'r1', | ||
| municipalityId: 'daet', | ||
| status: 'pending', | ||
| }) |
There was a problem hiding this comment.
suggestion (testing): Extend cancelDispatchCore tests to cover NOT_FOUND and non-current dispatch scenarios
These tests exercise the main 3b flows, but two key branches in cancelDispatchCore are still untested:
- When the dispatch document is missing,
cancelDispatchCoreshould throw withcode: 'NOT_FOUND'. Please add a test calling it with a randomdispatchIdand asserting this error. - When
report.currentDispatchId !== dispatchId, the report status andcurrentDispatchIdshould not change and noreport_eventsentry should be written. A test that seeds such a report + pending dispatch and verifies those invariants would protect against reverting unrelated dispatches.
Optionally, you could also add a NOT_FOUND case for a missing report (dispatch references a non‑existent reportId).
Suggested implementation:
await testEnv.clearFirestore()
})
describe('cancelDispatchCore edge cases', () => {
it('throws NOT_FOUND when dispatch does not exist', async () => {
const db = testEnv.unauthenticatedContext().firestore() as any
const randomDispatchId = 'non-existent-dispatch'
await expect(
cancelDispatchCore(db, {
dispatchId: randomDispatchId,
// add any other required fields for cancelDispatchCore here (e.g. auth context)
}),
).rejects.toMatchObject({ code: 'NOT_FOUND' })
})
it('does not modify report or write events when canceling non-current dispatch', async () => {
const db = testEnv.unauthenticatedContext().firestore() as any
// Seed a report and two pending dispatches so that currentDispatchId !== dispatchId being cancelled
const { reportId } = await seedReportAtStatus(db, 'assigned', { municipalityId: 'daet' })
const { dispatchId: currentDispatchId } = await seedDispatch(db, {
reportId,
responderUid: 'r-current',
municipalityId: 'daet',
status: 'pending',
})
const { dispatchId: otherDispatchId } = await seedDispatch(db, {
reportId,
responderUid: 'r-other',
municipalityId: 'daet',
status: 'pending',
})
// Make report point at the "current" dispatch
await db.collection('reports').doc(reportId).update({
currentDispatchId,
status: 'assigned',
})
// Capture initial state
const beforeReportSnap = await db.collection('reports').doc(reportId).get()
const beforeReportData = beforeReportSnap.data()
const beforeEventsSnap = await db
.collection('report_events')
.where('reportId', '==', reportId)
.get()
// Attempt to cancel the non-current dispatch
await cancelDispatchCore(db, {
dispatchId: otherDispatchId,
// add any other required fields for cancelDispatchCore here (e.g. auth context)
})
const afterReportSnap = await db.collection('reports').doc(reportId).get()
const afterReportData = afterReportSnap.data()
const afterEventsSnap = await db
.collection('report_events')
.where('reportId', '==', reportId)
.get()
// Report status and currentDispatchId should not change
expect(afterReportData).toMatchObject(beforeReportData!)
// No new report_events entries should be written
expect(afterEventsSnap.size).toBe(beforeEventsSnap.size)
})
// Optionally, add a NOT_FOUND-for-report case once cancelDispatchCore's behavior is confirmed
})
describe('cancelDispatchCore (3b branches)', () => {- Ensure
cancelDispatchCoreis imported into this test file (e.g. from../../src/callables/cancel-dispatchor the appropriate module). - Fill in any additional parameters required by
cancelDispatchCorein the two new test calls (e.g. auth/user context, municipalityId, etc.) so the function can execute without throwing unrelated errors. - If your schema uses different collection names or field names (e.g.
reportsV2,reportEvents), update thedb.collection(...)calls and field keys (currentDispatchId,status,reportId) accordingly. - If there are existing helpers for reading report events (e.g.
getReportEventsForReport(db, reportId)), you may prefer to use them instead of rawcollection('report_events').where(...)queries to stay consistent with the rest of the test suite. - If
cancelDispatchCorereturns a structured error instead of throwing, adapt the first test assertion fromrejects.toMatchObjectto the pattern used elsewhere in this test file (for example, checking a returnederrorobject).
Comment on lines
+20
to
+29
| describe('getEligibleResponders', () => { | ||
| it('returns only active responders in the target municipality who are on shift', async () => { | ||
| await testEnv.withSecurityRulesDisabled(async (ctx) => { | ||
| const db = ctx.firestore() as unknown as Firestore | ||
| const rtdb = ctx.database() as unknown as Database | ||
| await seedResponderDoc(db, { | ||
| uid: 'r1', | ||
| municipalityId: 'daet', | ||
| agencyId: 'bfp-daet', | ||
| isActive: true, |
There was a problem hiding this comment.
suggestion (testing): Add tests for responders with no shift entry and the empty-result case
To fully cover getEligibleResponders, consider adding:
-
No shift entry in RTDB: Seed an active responder in Firestore but omit the RTDB
/responder_index/...node, and assert they are not returned, matching the current “RTDB is source of truth for on‑shift” behavior. -
Empty result: Add a case where no responders match (e.g., municipality or agency with none) and assert
result.length === 0, to lock in behavior when filtering and sorting yield no results.
Suggested implementation:
describe('getEligibleResponders', () => {
it('excludes active responders that have no on-shift entry in RTDB', async () => {
await testEnv.withSecurityRulesDisabled(async (ctx) => {
const db = ctx.firestore() as unknown as Firestore
const rtdb = ctx.database() as unknown as Database
// Active responder in municipality/agency but NO RTDB shift entry
await seedResponderDoc(db, {
uid: 'r-no-shift',
municipalityId: 'daet',
agencyId: 'bfp-daet',
isActive: true,
})
// Another responder that is actually on shift to prove the filter works
await seedResponderDoc(db, {
uid: 'r-on-shift',
municipalityId: 'daet',
agencyId: 'bfp-daet',
isActive: true,
})
await rtdb.ref('/responder_index/daet/bfp-daet/r-on-shift').set({
uid: 'r-on-shift',
status: 'on_shift',
})
const result = await getEligibleResponders({
firestore: db,
database: rtdb,
municipalityId: 'daet',
agencyId: 'bfp-daet',
})
const uids = result.map((r) => r.uid)
expect(uids).toContain('r-on-shift')
expect(uids).not.toContain('r-no-shift')
})
})
it('returns an empty array when no responders match the filters', async () => {
await testEnv.withSecurityRulesDisabled(async (ctx) => {
const db = ctx.firestore() as unknown as Firestore
const rtdb = ctx.database() as unknown as Database
// Seed responders in a different municipality/agency
await seedResponderDoc(db, {
uid: 'r-other',
municipalityId: 'vinzons',
agencyId: 'bfp-vinzons',
isActive: true,
})
await rtdb.ref('/responder_index/vinzons/bfp-vinzons/r-other').set({
uid: 'r-other',
status: 'on_shift',
})
const result = await getEligibleResponders({
firestore: db,
database: rtdb,
municipalityId: 'daet',
agencyId: 'bfp-daet',
})
expect(result).toHaveLength(0)
})
})
it('returns only active responders in the target municipality who are on shift', async () => {These edits assume:
getEligibleRespondersis imported into this test file and accepts an options object with{ firestore, database, municipalityId, agencyId }(or a compatible signature). Adjust the call shape if your actual function signature differs.seedResponderDocwrites responder metadata to Firestore but does not implicitly create RTDB entries; RTDB shift entries are explicitly created viartdb.ref('/responder_index/...').set(...). If the RTDB path or payload shape differs in your codebase, update the/responder_index/...paths and data objects accordingly.- If your existing tests use different municipality/agency IDs or a factory/helper for RTDB seeding, mirror those conventions (e.g., reuse a
seedShiftEntryhelper instead of directrtdb.ref(...).set(...)). - Place these new
itblocks alongside the existing ones within the samedescribe('getEligibleResponders', ...)block, preserving any surrounding setup/teardown logic or shared helpers that are present in parts of the file not shown here.
|
|
||
| Commit `6546a0e` ("fix validators: cap pendingMediaIds at 20") was made by a subagent and appeared in `git log` but wasn't in the worktree's HEAD. It was on `main`. After `git rebase main`, it appeared at the correct position in history. Always rebase worktrees onto main before starting implementation to avoid this confusion. | ||
|
|
||
| ### `seedReportAtStatus` uses Firebase Admin Timestamp, incompatible with RulesTestContext |
There was a problem hiding this comment.
suggestion (typo): Heading uses RulesTestContext while the body refers to RulesTestEnvironment
Please update the heading to use RulesTestEnvironment so it matches the term used in the explanation below and avoids confusion.
Suggested change
| ### `seedReportAtStatus` uses Firebase Admin Timestamp, incompatible with RulesTestContext | |
| ### `seedReportAtStatus` uses Firebase Admin Timestamp, incompatible with RulesTestEnvironment |
![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: 41
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/responder-app/package.json (1)
1-29:⚠️ Potential issue | 🟠 MajorUnresolved responder-app build failure is a merge blocker.
CI is currently red for
@bantayog/responder-app(tsc --noEmit && vite build), so this should not merge until that job is green.As per coding guidelines: "VERIFY OR IT DIDN'T HAPPEN. Every edit must be followed by tests, linting, and type-checking. Never ignore warnings."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/responder-app/package.json` around lines 1 - 29, CI shows the build for `@bantayog/responder-app` failing (the "build" script running "tsc --noEmit && vite build"); run the same locally (npm run typecheck and npm run build) to capture TypeScript errors and Vite build failures, fix all reported type errors in the src code or adjust types/exports in the shared packages referenced (symbols: package.json "build", "typecheck", and dependencies "@bantayog/shared-types"/"@bantayog/shared-ui"), add any missing `@types` or devDependencies needed, and re-run the scripts until "tsc --noEmit && vite build" completes cleanly before merging.
🤖 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/app/auth-provider.tsx`:
- Around line 39-50: The onAuthStateChanged listener in useEffect must handle
getIdTokenResult rejections and avoid applying stale async results: when auth
state changes, ensure you setClaims(undefined) and setLoading(false) immediately
for null users; for non-null users, capture an identifier (e.g., currentUid =
u.uid) or a cancelled flag scoped to this effect before calling
u.getIdTokenResult(), and inside the then/catch/finally only call
setClaims/setLoading if the identifier/flag still matches (user hasn't changed);
also add a .catch to handle rejection and a .finally to guarantee
setLoading(false) runs even on error; keep unsubscribe cleanup as-is to cancel
the listener.
In `@apps/admin-desktop/src/app/firebase.ts`:
- Around line 28-31: The current console.warn on missing VITE_RECAPTCHA_SITE_KEY
allows startups in non-emulator environments; change the branch that handles
missing VITE_RECAPTCHA_SITE_KEY (the block that currently calls
console.warn('[firebase] VITE_RECAPTCHA_SITE_KEY not set — App Check disabled.
DO NOT USE IN PRODUCTION.')) to fail fast in production by throwing a
descriptive Error (or calling process.exit(1)) when not running under the
emulator, e.g., check the existing emulator flag/condition and if emulator mode
is false and VITE_RECAPTCHA_SITE_KEY is absent, throw new Error with a clear
message mentioning VITE_RECAPTCHA_SITE_KEY and App Check so deployments cannot
start without the key.
In `@apps/admin-desktop/src/hooks/useEligibleResponders.ts`:
- Around line 18-19: The hook useEligibleResponders leaves stale state when
municipalityId is falsy; update both effects that early-return on
(!municipalityId) to explicitly clear responders and shift by calling
setResponders([]) and setShift(null) (or the existing reset values) before
returning so previous municipality data is not preserved; target the useEffect
blocks that reference municipalityId, responders, shift, setResponders and
setShift and add the clear calls in the early-exit paths.
In `@apps/admin-desktop/src/hooks/useMuniReports.ts`:
- Around line 19-23: The hook useMuniReports currently sets error on failures
but never clears it; update the logic so setError(null) is called both when
municipalityId is falsy (near the branch that calls setRows([]) and
setLoading(false)) and after successful snapshot/fetch handling (where you call
setRows and setLoading on success). Locate uses of municipalityId, setRows,
setLoading and the error state setter (setError) and add calls to clear the
error on boundary change and immediately after successful data processing to
avoid stale errors remaining visible.
In `@apps/admin-desktop/src/hooks/useReportDetail.ts`:
- Around line 27-30: When clearing state because reportId is falsy and when a
successful listener/snapshot updates state, also clear the error state; update
the block in useReportDetail where you do setReport(null) and setOps(null) to
also call setError(null), and likewise add setError(null) inside the
listener/snapshot success handler (the function that sets report and ops around
the code referenced at 41-53) so recovered snapshots remove prior errors.
- Line 37: The current object construction in useReportDetail uses ({ reportId:
s.id, ...(s.data() as Partial<ReportDetail>) } as ReportDetail) which lets the
Firestore document fields overwrite reportId; change the spread order so the
canonical s.id wins by spreading s.data() first and then setting reportId (e.g.,
build the object from ...(s.data() as Partial<ReportDetail>) and then reportId:
s.id), or explicitly remove reportId from the spread before composing the
object—update the expression in useReportDetail that references s.data() and
s.id accordingly so reportId cannot be overridden.
In `@apps/admin-desktop/src/pages/TriageQueuePage.tsx`:
- Around line 27-40: Validate the prompt input instead of force-casting: check
the `reason` string returned by prompt() against the allowed set
('obviously_false','duplicate','test_submission','insufficient_detail') before
calling `callables.rejectReport`; if invalid, show an error/abort. Pass the
validated reason to `callables.rejectReport` (still generate `idempotencyKey`
with `crypto.randomUUID()`), and on success ensure you clear or update the
success banner/UI state consistently. Also replace the empty catch with proper
error logging/notification so exceptions from the async block are surfaced.
In `@apps/responder-app/src/app/auth-provider.tsx`:
- Around line 20-31: The onAuthStateChanged callback currently calls
u.getIdTokenResult() without error handling which can leave the app stuck in
loading; update the handler around u.getIdTokenResult() (the promise returned in
the onAuthStateChanged callback) to add a .catch() that sets claims to null (via
setClaims(null)) on failure and a .finally() that always calls
setLoading(false), ensuring both success and error paths clear the loading state
and reset claims when appropriate.
- Line 2: The package.json for the responder app is missing the firebase runtime
dependency but the code imports onAuthStateChanged and the User type from
'firebase/auth' (see auth-provider.tsx), causing TypeScript to resolve User as
any; add "firebase": "^12.12.0" to the responder-app dependencies in
package.json (matching admin-desktop and citizen-pwa), run npm/yarn install, and
re-run the build so the import of onAuthStateChanged and the User type are
correctly resolved.
In `@apps/responder-app/src/app/firebase.ts`:
- Around line 23-35: The emulator connections are started with non-blocking
dynamic imports causing a race where db/auth/functions/rtdb may be used before
connectFirestoreEmulator, connectAuthEmulator, connectFunctionsEmulator, and
connectDatabaseEmulator finish; fix this by performing the imports and connector
calls synchronously at module initialization (e.g., await the dynamic imports or
use static imports) inside the USE_EMULATOR branch so that
connectFirestoreEmulator(db,...), connectAuthEmulator(auth,...),
connectFunctionsEmulator(functions,...), and connectDatabaseEmulator(rtdb,...)
complete before any exported services are used.
- Line 20: The exported Cloud Functions client is created with getFunctions(app)
which defaults to us-central1; update the exported const functions to call
getFunctions(app, "asia-southeast1") so the client targets the deployed region.
Locate the exported symbol functions and the getFunctions call in the file
(using the app variable) and replace the no-arg call with the region-aware call
matching the pattern used in other modules.
In `@apps/responder-app/src/app/protected-route.tsx`:
- Around line 1-4: The file is using React.ReactNode in the ProtectedRoute
component but never imports React, causing type errors under
verbatimModuleSyntax; update the imports to import ReactNode directly from
'react' (e.g., add "import { ReactNode } from 'react'") and change the component
signature to use that type if necessary; locate the ProtectedRoute function and
the top import block (where Navigate and useLocation are imported) and add the
ReactNode import to match the pattern used in auth-provider.tsx.
In `@apps/responder-app/src/hooks/useOwnDispatches.ts`:
- Around line 18-20: When uid becomes falsy in useOwnDispatches, reset the
hook's local state instead of returning early with stale values; inside the
existing early-return branch where you check `if (!uid) return`, call the state
setters to clear previous data (e.g., `setRows([])`, `setError(null or
undefined)`, and `setLoading(false)` as appropriate) so `rows` and `error` don't
keep showing prior-session dispatches; refer to the hook's state setters
(`setRows`, `setError`, `setLoading`) and `uid` in useOwnDispatches to implement
this change.
In `@docs/learnings.md`:
- Line 370: Update the heading text "ESLint `no-explicit-any` requires combined
disable comment for multiple rules on same line" to improve grammar by inserting
an article: change it to something like "ESLint `no-explicit-any` requires a
combined disable comment for multiple rules on the same line" (or "must be
combined into a single disable comment for multiple rules on the same line");
edit the heading string in docs/learnings.md to reflect this corrected wording.
In `@docs/progress.md`:
- Around line 78-92: The doc shows conflicting status: the heading "## Phase 3b
Admin Triage + Dispatch (Complete)" contradicts the Verification table where
Steps 3–5 are marked PENDING and another section states "In Progress"; change
the canonical status to reflect that the phase is still in progress (e.g.,
update the heading to "(In Progress)" or a neutral "Pending verification") and
ensure the Verification table entries for Steps 3–5 remain PENDING until those
emulator/staging/manual acceptance gates pass; also make the same consistent
update where the duplicate status appears (the other section referenced around
the same Phase 3b content) so both the heading and the verification block use
the same single canonical status and avoid misleading release gating.
In `@docs/runbooks/phase-3b-verify-and-dispatch.md`:
- Around line 163-171: The rollback snippet uses CommonJS require() which breaks
in ESM projects; update the inline Node invocation in the firebase
emulators:exec example to an ESM-compatible form (use dynamic import or run node
with --input-type=module and call initializeApp() then getFirestore()) or add a
short note that the snippet requires CommonJS; specifically replace
require('firebase-admin/firestore')/getFirestore usage with an ESM
import/initializeApp/getFirestore flow (or use dynamic
import('firebase-admin/firestore')) and ensure async/await is handled for the
db.collection(...).update calls.
In `@functions/src/__tests__/callables/dispatch-responder.test.ts`:
- Around line 16-24: The test suite sets up a TestEnvironment with beforeEach
using initializeTestEnvironment and assigns it to testEnv but never tears it
down; add an afterAll that calls testEnv.cleanup() to release resources. Locate
the beforeEach block that calls initializeTestEnvironment and add an
afterAll(async () => { await testEnv.cleanup() }) so the TestEnvironment created
in testEnv is cleaned up after all tests.
In `@functions/src/__tests__/callables/reject-report.test.ts`:
- Around line 8-15: Tests create a RulesTestEnvironment per test but never call
cleanup, causing leaked handles; add an afterEach that awaits testEnv.cleanup()
to properly tear down the environment created by initializeTestEnvironment and
avoid flaky CI—use the existing testEnv variable inside the new afterEach(async
() => { if (testEnv) await testEnv.cleanup(); }) so cleanup runs even if tests
fail.
In `@functions/src/__tests__/callables/verify-report.test.ts`:
- Around line 15-25: Add an afterAll cleanup that calls testEnv.cleanup() to
tear down the Firebase test environment initialized by initializeTestEnvironment
in this test file; specifically, after the existing beforeEach block that sets
testEnv (the variable assigned from initializeTestEnvironment in the
beforeEach), add an afterAll(async () => { await testEnv.cleanup() }) to prevent
resource leaks.
In `@functions/src/__tests__/helpers/seed-factories.ts`:
- Around line 296-301: seedResponderDoc currently sets createdAt and updatedAt
with new Date(), which is inconsistent with other seed helpers; change the
values for createdAt and updatedAt in seedResponderDoc to use Timestamp.now() so
they match seedReportAtStatus and seedDispatch (update the assignments for
createdAt and updatedAt within the seedResponderDoc object).
- Around line 210-211: The two type imports "Firestore" and "Database" are
located mid-file; move "import type { Firestore } from
'firebase-admin/firestore'" and "import type { Database } from
'firebase-admin/database'" to the top of the file with the other imports so they
appear before any function definitions (keep them as type-only imports),
ensuring they remain after the existing initial import block and preserve
ESLint/TS ordering conventions in seed-factories.ts.
In `@functions/src/__tests__/rules/admin-onsnapshot.rules.test.ts`:
- Around line 36-44: The test suite creates a RulesTestEnvironment in the
beforeEach via initializeTestEnvironment assigned to testEnv but never cleans it
up; add an afterAll hook that calls await testEnv.cleanup() to release resources
and avoid port conflicts (i.e., add afterAll(async () => { await
testEnv.cleanup() }) corresponding to the existing
beforeEach/testEnv/initializeTestEnvironment usage).
In `@functions/src/__tests__/services/responder-eligibility.test.ts`:
- Around line 10-18: The test suite creates a TestEnvironment via
initializeTestEnvironment() in beforeEach but never calls cleanup, which can
leak handles; add a teardown that calls testEnv.cleanup() after tests (e.g., in
afterEach or afterAll) to ensure the TestEnvironment is disposed — reference the
testEnv variable and initializeTestEnvironment() used in the beforeEach block
and invoke testEnv.cleanup() in the appropriate teardown hook.
In `@functions/src/callables/cancel-dispatch.ts`:
- Around line 70-82: The code first verifies
CANCELLABLE_FROM_STATES.includes(from) and then redundantly calls
isValidDispatchTransition(from, to); remove the second validation block that
calls isValidDispatchTransition and throws BantayogError (the if
(!isValidDispatchTransition(...)) branch) so that only the explicit
CANCELLABLE_FROM_STATES check remains; keep the existing variables (from, to)
and the BantayogError throw for the cancellable-states check to preserve
behavior.
In `@functions/src/callables/dispatch-responder.ts`:
- Around line 112-119: The code incorrectly asserts const from = report.status
as 'verified' which misrepresents the runtime value; update the
dispatch-responder logic to stop the misleading type cast and either remove the
"as 'verified'" annotation so from uses the actual type of report.status, or add
an explicit runtime check (e.g., if (report.status !== 'verified') throw new
BantayogError(...)) before calling isValidReportTransition; make sure to
reference and keep using isValidReportTransition(from, to) and the existing
BantayogError/BantayogErrorCode when rejecting invalid states.
- Around line 78-88: The RTDB read (shiftSnap via rtdb.ref) inside the Firestore
transaction is not atomic with the Firestore commit; add a compensating
post-commit validation: after the Firestore transaction that creates the
dispatch completes, re-read `/responder_index/.../${deps.responderUid}` and if
isOnShift !== true then immediately call the existing cancelDispatch flow (or
update the newly-created dispatch to a cancelled state) and log the discrepancy;
keep the original RTDB pre-check for TOCTOU mitigation but ensure the
post-commit check (using the same rtdb.ref path and the cancelDispatch routine)
guarantees we don't leave a dispatch assigned to an off-shift responder.
In `@functions/src/callables/reject-report.ts`:
- Line 8: The import for bantayogErrorToHttps uses a .js extension while other
relative imports in this module do not; update the import statement for
bantayogErrorToHttps (the import line referencing './https-error.js') to match
the project's convention by removing the .js extension (i.e., import from
'./https-error'), or alternately add .js to all other relative imports in this
file so they are consistent—pick one approach and apply it consistently across
the module.
In `@functions/src/callables/verify-report.ts`:
- Around line 82-103: The code computes the next state with an explicit state
machine (variables from/to) and then redundantly calls
isValidReportTransition(from, to); remove the redundant validation by either (A)
keeping the explicit mapping (the if/else that sets to) and deleting the
subsequent isValidReportTransition check and its BantayogError throw, or (B)
replace the explicit mapping with a single call to isValidReportTransition to
determine/validate the transition and throw BantayogError with
BantayogErrorCode.INVALID_STATUS_TRANSITION when it returns false; update the
error context to include reportId and from/to as appropriate (refer to symbols
from, to, isValidReportTransition, BantayogError,
BantayogErrorCode.INVALID_STATUS_TRANSITION).
- Around line 4-14: Remove the duplicate import of logDimension and normalize
the relative import extension: keep a single import of logDimension from
'@bantayog/shared-validators' (with isValidReportTransition, BantayogError,
BantayogErrorCode, type ReportStatus) and delete the redundant import at the
bottom; change the import of bantayogErrorToHttps from './https-error.js' to use
the same extension style as the other relative imports (e.g., './https-error')
so imports are consistent and non-duplicated, and then run a quick build to
ensure no path/type errors.
In `@functions/src/firebase-admin.ts`:
- Around line 4-10: The RTDB admin client export (rtdb) is added directly; wrap
its initialization behind a controlled rollout gate so it only initializes in
non-production contexts unless explicitly enabled — check
getApps()/initializeApp usage and the rtdb export and add a guard that reads a
config/ENV flag (e.g. ENABLE_RTDB_ADMIN or NODE_ENV === 'test' /
FIREBASE_EMULATOR) before calling getDatabase(app); also add a clear comment and
require running the emulator + full test suite and an explicit staging approval
before enabling the flag in staging/production.
In `@functions/src/services/rate-limit.ts`:
- Around line 20-33: Validate the incoming RateLimitCheck params before entering
the transaction: ensure limit and windowSeconds are positive integers (>0) (use
Number.isFinite and >0, optionally Math.floor/coerce), and if they are invalid
throw a descriptive Error (e.g., "Invalid rate limit config: limit/windowSeconds
must be > 0") so the caller can handle it; perform this check using the function
parameters (key, limit, windowSeconds, now, updatedAt) before calling
db.runTransaction in checkRateLimit so the transaction math never runs with bad
values.
In `@functions/src/services/responder-eligibility.ts`:
- Around line 33-41: The mapping callback that builds responder objects
inconsistently handles missing fields: displayName and agencyId are null-coerced
but municipalityId is cast directly; change the municipalityId extraction in the
.map callback (the code that uses doc.data() and returns
uid/displayName/agencyId/municipalityId) to defensively coerce like the others
(e.g., use String(data.municipalityId ?? '') or equivalent) so malformed docs
don’t yield undefined or throw; update only the municipalityId line to mirror
displayName/agencyId handling.
In `@infra/terraform/modules/monitoring/phase-3/main.tf`:
- Line 74: The filter in the monitoring module is using jsonPayload.event but
the dispatch responder emits the field as code; update the filter string in
main.tf (the filter variable that currently contains
resource.type="cloud_function" AND jsonPayload.event="dispatch.created" ...) to
use jsonPayload.code="dispatch.created" instead so it matches the logEvent
emission in functions/src/callables/dispatch-responder.ts (the logEvent call
around line 181) and stays consistent with other metrics in the module.
- Around line 71-73: Update the google_logging_metric resource
google_logging_metric.dispatch_created to use var.environment (not var.env) for
the name, add project = var.project_id to match other metrics, and change the
log filter discriminator from jsonPayload.event to jsonPayload.code; set the
code value to the canonical string used for dispatch creation logs (verify the
exact code value for dispatch.created in the logs and use that string, e.g.,
"DISPATCH_CREATED" if that is the canonical code).
In `@packages/shared-validators/src/dispatches.ts`:
- Around line 36-40: The nested assignedTo schema (defined as assignedTo:
z.object({...})) is not strict by default; update the assignedTo definition to
use z.object({...}).strict() so unknown keys in the nested object are rejected,
ensuring per-level boundary validation; locate the assignedTo z.object and
append .strict() to it (keeping surrounding top-level .strict() intact).
In `@packages/shared-validators/src/errors-and-logging.test.ts`:
- Around line 14-16: The test "has 19 named error codes" currently only asserts
the enum length; update it to also assert the presence of the newly introduced
member on BantayogErrorCode by adding an explicit membership assertion (e.g.,
check that Object.values(BantayogErrorCode) contains the new enum member or that
BantayogErrorCode has the specific property name) — reference the
BantayogErrorCode symbol and replace <NEW_MEMBER_NAME> with the actual enum key
added.
In `@packages/shared-validators/src/responders.ts`:
- Line 11: You added a new required field isActive in the responder schema which
requires a formal migration: create a runbook at
docs/runbooks/migrations/<YYYY-MM-DD>-responder.md describing old/new schema,
trigger compat matrix, backfill strategy, rollback and monitoring; increment the
responder schemaVersion constant where the schema is defined; implement branched
read/write handling so code that reads responders accepts both documents without
isActive and with isActive (e.g., add compatibility logic around the schema
validator or in the functions that load responders), add a scheduled backfill
job to set isActive on existing documents, run the migration in the dev emulator
and test suite, then get staging approval before deploying to production.
In `@scripts/phase-3b/acceptance.ts`:
- Line 4: Remove the unused import getFunctions from the top of the file (the
import statement "import { getFunctions } from 'firebase-admin/functions'")
since it's never referenced; simply delete that named import or the entire
import line if nothing else is imported from that module to satisfy the linter
and avoid unused-import warnings.
- Around line 86-88: When EMU is true the script only calls
connectAuthEmulator(webAuth, ...) but never connects the Functions emulator, so
callable functions still hit production; add a call to the Functions emulator
inside the same EMU block (e.g., invoke connectFunctionsEmulator with your
Functions instance—referencing the symbol used in this file such as webFunctions
or functions—and the local host/port for the emulator like localhost and 5001),
and ensure the connectFunctionsEmulator import is present so local callable
invocations target the emulator.
- Around line 82-84: webInitApp is called with only appId causing incomplete
Firebase initialization; update the webInitApp call to include at least
projectId and apiKey (keep appId:'demo'), then call webGetAuth(webApp) as before
and initialize functions with webGetFunctions(webApp, { region:
'asia-southeast1' }) so the client targets the correct region; finally, when EMU
is true, call connectFunctionsEmulator(webFunctions, 'localhost', 5001) (or
equivalent) to connect to the local emulator so function invocations work
locally.
In `@scripts/phase-3b/bootstrap-test-responder.ts`:
- Around line 48-54: Replace the brittle substring check in the catch block that
tests err.message with a check against the Firebase error code: inspect err.code
(e.g., compare to 'auth/uid-already-exists') instead of
err.message.includes('already'), so in the catch for the user creation flow (the
catch handling the variable err in scripts/phase-3b/bootstrap-test-responder.ts)
log '[bootstrap] auth user already exists' when err.code ===
'auth/uid-already-exists' and rethrow otherwise; use a safe cast or type guard
(e.g., treat err as any or check typeof err === 'object' && 'code' in err) so
you don’t rely on message parsing.
---
Outside diff comments:
In `@apps/responder-app/package.json`:
- Around line 1-29: CI shows the build for `@bantayog/responder-app` failing (the
"build" script running "tsc --noEmit && vite build"); run the same locally (npm
run typecheck and npm run build) to capture TypeScript errors and Vite build
failures, fix all reported type errors in the src code or adjust types/exports
in the shared packages referenced (symbols: package.json "build", "typecheck",
and dependencies "@bantayog/shared-types"/"@bantayog/shared-ui"), add any
missing `@types` or devDependencies needed, and re-run the scripts until "tsc
--noEmit && vite build" completes cleanly before merging.
🪄 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: 5eba9209-9259-4239-98c7-c995d7b0f799
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (55)
apps/admin-desktop/package.jsonapps/admin-desktop/src/App.tsxapps/admin-desktop/src/app/auth-provider.tsxapps/admin-desktop/src/app/firebase.tsapps/admin-desktop/src/app/protected-route.tsxapps/admin-desktop/src/hooks/useEligibleResponders.tsapps/admin-desktop/src/hooks/useMuniReports.tsapps/admin-desktop/src/hooks/useReportDetail.tsapps/admin-desktop/src/main.tsxapps/admin-desktop/src/pages/DispatchModal.tsxapps/admin-desktop/src/pages/LoginPage.tsxapps/admin-desktop/src/pages/ReportDetailPanel.tsxapps/admin-desktop/src/pages/TriageQueuePage.tsxapps/admin-desktop/src/routes.tsxapps/admin-desktop/src/services/callables.tsapps/responder-app/package.jsonapps/responder-app/src/App.tsxapps/responder-app/src/app/auth-provider.tsxapps/responder-app/src/app/firebase.tsapps/responder-app/src/app/protected-route.tsxapps/responder-app/src/hooks/useOwnDispatches.tsapps/responder-app/src/main.tsxapps/responder-app/src/pages/DispatchListPage.tsxapps/responder-app/src/pages/LoginPage.tsxapps/responder-app/src/routes.tsxdocs/learnings.mddocs/progress.mddocs/runbooks/phase-3b-verify-and-dispatch.mdfunctions/src/__tests__/callables/cancel-dispatch.test.tsfunctions/src/__tests__/callables/dispatch-responder.test.tsfunctions/src/__tests__/callables/reject-report.test.tsfunctions/src/__tests__/callables/verify-report.test.tsfunctions/src/__tests__/helpers/seed-factories.tsfunctions/src/__tests__/rules/admin-onsnapshot.rules.test.tsfunctions/src/__tests__/rules/dispatches.rules.test.tsfunctions/src/__tests__/services/rate-limit.test.tsfunctions/src/__tests__/services/responder-eligibility.test.tsfunctions/src/callables/cancel-dispatch.tsfunctions/src/callables/dispatch-responder.tsfunctions/src/callables/https-error.tsfunctions/src/callables/reject-report.tsfunctions/src/callables/verify-report.tsfunctions/src/firebase-admin.tsfunctions/src/index.tsfunctions/src/services/rate-limit.tsfunctions/src/services/responder-eligibility.tsinfra/firebase/firestore.indexes.jsoninfra/terraform/modules/monitoring/phase-3/main.tfpackages/shared-validators/src/dispatches.test.tspackages/shared-validators/src/dispatches.tspackages/shared-validators/src/errors-and-logging.test.tspackages/shared-validators/src/errors.tspackages/shared-validators/src/responders.tsscripts/phase-3b/acceptance.tsscripts/phase-3b/bootstrap-test-responder.ts
Comment on lines
+39
to
+50
| useEffect(() => { | ||
| const unsubscribe = onAuthStateChanged(auth, (u) => { | ||
| setUser(u) | ||
| if (u) { | ||
| void u.getIdTokenResult().then((tok) => { | ||
| setClaims({ | ||
| role: tok.claims.role as string | undefined, | ||
| municipalityId: tok.claims.municipalityId as string | undefined, | ||
| active: tok.claims.active === true, | ||
| } as AdminClaims) | ||
| setLoading(false) | ||
| }) |
Contributor
There was a problem hiding this comment.
Handle token-fetch failures and stale async resolution in auth state listener.
If getIdTokenResult() rejects, loading may never flip to false. Also, async completion can race after auth state changes and set stale claims.
Proposed fix
useEffect(() => {
const unsubscribe = onAuthStateChanged(auth, (u) => {
setUser(u)
if (u) {
- void u.getIdTokenResult().then((tok) => {
- setClaims({
- role: tok.claims.role as string | undefined,
- municipalityId: tok.claims.municipalityId as string | undefined,
- active: tok.claims.active === true,
- } as AdminClaims)
- setLoading(false)
- })
+ void u
+ .getIdTokenResult()
+ .then((tok) => {
+ if (auth.currentUser?.uid !== u.uid) return
+ setClaims({
+ role: tok.claims.role as string | undefined,
+ municipalityId: tok.claims.municipalityId as string | undefined,
+ active: tok.claims.active === true,
+ } as AdminClaims)
+ })
+ .catch(() => {
+ setClaims(null)
+ })
+ .finally(() => {
+ if (auth.currentUser?.uid === u.uid) setLoading(false)
+ })
} else {
setClaims(null)
setLoading(false)
}
})📝 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
| useEffect(() => { | |
| const unsubscribe = onAuthStateChanged(auth, (u) => { | |
| setUser(u) | |
| if (u) { | |
| void u.getIdTokenResult().then((tok) => { | |
| setClaims({ | |
| role: tok.claims.role as string | undefined, | |
| municipalityId: tok.claims.municipalityId as string | undefined, | |
| active: tok.claims.active === true, | |
| } as AdminClaims) | |
| setLoading(false) | |
| }) | |
| useEffect(() => { | |
| const unsubscribe = onAuthStateChanged(auth, (u) => { | |
| setUser(u) | |
| if (u) { | |
| void u | |
| .getIdTokenResult() | |
| .then((tok) => { | |
| if (auth.currentUser?.uid !== u.uid) return | |
| setClaims({ | |
| role: tok.claims.role as string | undefined, | |
| municipalityId: tok.claims.municipalityId as string | undefined, | |
| active: tok.claims.active === true, | |
| } as AdminClaims) | |
| }) | |
| .catch(() => { | |
| setClaims(null) | |
| }) | |
| .finally(() => { | |
| if (auth.currentUser?.uid === u.uid) setLoading(false) | |
| }) | |
| } else { | |
| setClaims(null) | |
| setLoading(false) | |
| } | |
| }) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/admin-desktop/src/app/auth-provider.tsx` around lines 39 - 50, The
onAuthStateChanged listener in useEffect must handle getIdTokenResult rejections
and avoid applying stale async results: when auth state changes, ensure you
setClaims(undefined) and setLoading(false) immediately for null users; for
non-null users, capture an identifier (e.g., currentUid = u.uid) or a cancelled
flag scoped to this effect before calling u.getIdTokenResult(), and inside the
then/catch/finally only call setClaims/setLoading if the identifier/flag still
matches (user hasn't changed); also add a .catch to handle rejection and a
.finally to guarantee setLoading(false) runs even on error; keep unsubscribe
cleanup as-is to cancel the listener.
Comment on lines
+28
to
+31
| } else { | ||
| console.warn( | ||
| '[firebase] VITE_RECAPTCHA_SITE_KEY not set — App Check disabled. DO NOT USE IN PRODUCTION.', | ||
| ) |
Contributor
There was a problem hiding this comment.
Fail fast in production when App Check key is missing.
In non-emulator mode, missing VITE_RECAPTCHA_SITE_KEY only logs a warning and continues without App Check. That weakens abuse protection if misconfigured deployment reaches prod.
🔧 Proposed guard
} else {
- console.warn(
- '[firebase] VITE_RECAPTCHA_SITE_KEY not set — App Check disabled. DO NOT USE IN PRODUCTION.',
- )
+ if (import.meta.env.PROD) {
+ throw new Error(
+ '[firebase] Missing VITE_RECAPTCHA_SITE_KEY in production; refusing to start without App Check.',
+ )
+ }
+ console.warn('[firebase] VITE_RECAPTCHA_SITE_KEY not set — App Check disabled in non-prod.')
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/admin-desktop/src/app/firebase.ts` around lines 28 - 31, The current
console.warn on missing VITE_RECAPTCHA_SITE_KEY allows startups in non-emulator
environments; change the branch that handles missing VITE_RECAPTCHA_SITE_KEY
(the block that currently calls console.warn('[firebase] VITE_RECAPTCHA_SITE_KEY
not set — App Check disabled. DO NOT USE IN PRODUCTION.')) to fail fast in
production by throwing a descriptive Error (or calling process.exit(1)) when not
running under the emulator, e.g., check the existing emulator flag/condition and
if emulator mode is false and VITE_RECAPTCHA_SITE_KEY is absent, throw new Error
with a clear message mentioning VITE_RECAPTCHA_SITE_KEY and App Check so
deployments cannot start without the key.
Comment on lines
+19
to
+23
| if (!municipalityId) { | ||
| setRows([]) | ||
| setLoading(false) | ||
| return | ||
| } |
Contributor
There was a problem hiding this comment.
Reset error on boundary change and successful snapshots.
The hook sets error on failures but never clears it on success (or when municipalityId becomes undefined). This can leave the page stuck showing stale errors.
Proposed fix
useEffect(() => {
if (!municipalityId) {
setRows([])
+ setError(null)
setLoading(false)
return
}
+ setError(null)
setLoading(true)
@@
(snap) => {
setRows(
@@
)
+ setError(null)
setLoading(false)
},Also applies to: 34-50
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/admin-desktop/src/hooks/useMuniReports.ts` around lines 19 - 23, The
hook useMuniReports currently sets error on failures but never clears it; update
the logic so setError(null) is called both when municipalityId is falsy (near
the branch that calls setRows([]) and setLoading(false)) and after successful
snapshot/fetch handling (where you call setRows and setLoading on success).
Locate uses of municipalityId, setRows, setLoading and the error state setter
(setError) and add calls to clear the error on boundary change and immediately
after successful data processing to avoid stale errors remaining visible.
Comment on lines
+27
to
+30
| if (!reportId) { | ||
| setReport(null) | ||
| setOps(null) | ||
| return |
Contributor
There was a problem hiding this comment.
Reset error on report switch and successful snapshots.
error is only ever appended/set on failures. It should be cleared when reportId changes to empty and when listeners recover successfully.
Proposed fix
useEffect(() => {
if (!reportId) {
setReport(null)
setOps(null)
+ setError(null)
return
}
+ setError(null)
const u1 = onSnapshot(
doc(db, 'reports', reportId),
(s) => {
@@
)
+ setError(null)
},
@@
(s) => {
setOps(s.exists() ? (s.data() as ReportOps) : null)
+ setError(null)
},Also applies to: 41-53
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/admin-desktop/src/hooks/useReportDetail.ts` around lines 27 - 30, When
clearing state because reportId is falsy and when a successful listener/snapshot
updates state, also clear the error state; update the block in useReportDetail
where you do setReport(null) and setOps(null) to also call setError(null), and
likewise add setError(null) inside the listener/snapshot success handler (the
function that sets report and ops around the code referenced at 41-53) so
recovered snapshots remove prior errors.
Comment on lines
+14
to
+16
| it('has 19 named error codes', () => { | ||
| const codes = Object.values(BantayogErrorCode) | ||
| expect(codes).toHaveLength(18) | ||
| expect(codes).toHaveLength(19) |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider hardening this assertion with an explicit member check.
The count change is correct, but adding a direct assertion for the newly introduced code makes this test more robust.
♻️ Suggested test hardening
it('has 19 named error codes', () => {
const codes = Object.values(BantayogErrorCode)
expect(codes).toHaveLength(19)
+ expect(codes).toContain(BantayogErrorCode.FAILED_PRECONDITION)
})🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/shared-validators/src/errors-and-logging.test.ts` around lines 14 -
16, The test "has 19 named error codes" currently only asserts the enum length;
update it to also assert the presence of the newly introduced member on
BantayogErrorCode by adding an explicit membership assertion (e.g., check that
Object.values(BantayogErrorCode) contains the new enum member or that
BantayogErrorCode has the specific property name) — reference the
BantayogErrorCode symbol and replace <NEW_MEMBER_NAME> with the actual enum key
added.
| displayCode: z.string().min(1), | ||
| specialisations: z.array(z.string()).default([]), | ||
| availabilityStatus: z.enum(['on_duty', 'off_duty', 'on_break', 'unavailable']), | ||
| isActive: z.boolean(), |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n packages/shared-validators/src/responders.tsRepository: Exc1D/bantayog-alert
Length of output: 814
🏁 Script executed:
# Search for migration files related to responders
fd -t f -i '*migrat*' --exec grep -l 'isActive\|responder' {} \;Repository: Exc1D/bantayog-alert
Length of output: 351
🏁 Script executed:
# Search for any backfill or data migration scripts
rg -t ts -t js 'isActive.*responder|responder.*isActive|backfill' --Repository: Exc1D/bantayog-alert
Length of output: 152
🏁 Script executed:
# Check if there's a Cloud Functions or server-side code handling this
fd -t f -i '*responder*' | head -20Repository: Exc1D/bantayog-alert
Length of output: 354
🏁 Script executed:
# Retry with proper glob syntax for migration files
fd --glob '**/*migrat*' -t fRepository: Exc1D/bantayog-alert
Length of output: 97
🏁 Script executed:
# Search for responder factory, seed, or initialization functions
rg -t ts -t js 'export.*responder|createResponder|responderFactory' -A 3Repository: Exc1D/bantayog-alert
Length of output: 2030
🏁 Script executed:
# Check Firestore initialization or seed scripts
fd --glob '**/seed*' -o --glob '**/init*' -t fRepository: Exc1D/bantayog-alert
Length of output: 180
🏁 Script executed:
# Look for test fixtures that might show expected document structure
rg -t ts 'responderDocSchema|ResponderDoc' -B 2 -A 5Repository: Exc1D/bantayog-alert
Length of output: 10870
🏁 Script executed:
cat -n functions/src/__tests__/helpers/seed-factories.ts | head -100Repository: Exc1D/bantayog-alert
Length of output: 4084
🏁 Script executed:
cat -n docs/runbooks/schema-migration.mdRepository: Exc1D/bantayog-alert
Length of output: 3366
🏁 Script executed:
# Check if there's any default/fallback handling for isActive
rg 'isActive.*\?' -t ts -A 2 -B 2Repository: Exc1D/bantayog-alert
Length of output: 46
🏁 Script executed:
# Check for any migration or backfill Cloud Functions
fd --glob '**/migration*' --glob '**/backfill*' -o --glob '**/backfill*' -t fRepository: Exc1D/bantayog-alert
Length of output: 180
🏁 Script executed:
# Get full seedResponderDoc implementation
rg -A 50 'export async function seedResponderDoc' functions/src/__tests__/helpers/seed-factories.tsRepository: Exc1D/bantayog-alert
Length of output: 1352
🏁 Script executed:
# Check if schemaVersion has been incremented for responders
rg 'schemaVersion' packages/shared-validators/src/responders.tsRepository: Exc1D/bantayog-alert
Length of output: 111
🏁 Script executed:
# Look for migration plan documents
fd --glob 'docs/runbooks/migrations/*' -t fRepository: Exc1D/bantayog-alert
Length of output: 46
🏁 Script executed:
# Check for any existing migration progress tracking
fd --glob '**/migrations/*' -t f | head -20Repository: Exc1D/bantayog-alert
Length of output: 46
Schema migration protocol required: isActive is a new required field without backfill plan.
Per docs/runbooks/schema-migration.md Stage 1, adding a required field without a default to an already-populated collection triggers the full migration protocol. Existing responder documents lack isActive, so:
- The strict schema will reject legacy reads/writes
- Dispatch eligibility filtering (
isActive !== true) silently drops untagged responders
Before deployment:
- Write a migration plan in
docs/runbooks/migrations/<YYYY-MM-DD>-responder.mdcovering: old/new schema, trigger compat matrix, backfill strategy, rollback, monitoring - Increment
schemaVersionin the responder schema - Implement branched read paths accepting both old and new versions during the 30-day migration window
- Schedule a backfill function to update existing documents
- Deploy to dev emulator, test full suite, then request explicit staging approval
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/shared-validators/src/responders.ts` at line 11, You added a new
required field isActive in the responder schema which requires a formal
migration: create a runbook at
docs/runbooks/migrations/<YYYY-MM-DD>-responder.md describing old/new schema,
trigger compat matrix, backfill strategy, rollback and monitoring; increment the
responder schemaVersion constant where the schema is defined; implement branched
read/write handling so code that reads responders accepts both documents without
isActive and with isActive (e.g., add compatibility logic around the schema
validator or in the functions that load responders), add a scheduled backfill
job to set isActive on existing documents, run the migration in the dev emulator
and test suite, then get staging approval before deploying to production.
Comment on lines
+86
to
+88
| if (EMU) { | ||
| connectAuthEmulator(webAuth, 'http://localhost:9099', { disableWarnings: true }) | ||
| } |
Contributor
There was a problem hiding this comment.
Connect Functions emulator when in EMU mode.
The script connects the Auth emulator but doesn't connect the Functions emulator. Without this, callable invocations will attempt to reach production/deployed functions instead of the local emulator.
🐛 Proposed fix
+import { connectFunctionsEmulator } from 'firebase/functions'
...
if (EMU) {
connectAuthEmulator(webAuth, 'http://localhost:9099', { disableWarnings: true })
+ connectFunctionsEmulator(webFunctions, 'localhost', 5001)
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/phase-3b/acceptance.ts` around lines 86 - 88, When EMU is true the
script only calls connectAuthEmulator(webAuth, ...) but never connects the
Functions emulator, so callable functions still hit production; add a call to
the Functions emulator inside the same EMU block (e.g., invoke
connectFunctionsEmulator with your Functions instance—referencing the symbol
used in this file such as webFunctions or functions—and the local host/port for
the emulator like localhost and 5001), and ensure the connectFunctionsEmulator
import is present so local callable invocations target the emulator.
Comment on lines
+48
to
+54
| } catch (err: unknown) { | ||
| if (err instanceof Error && err.message.includes('already')) { | ||
| console.log('[bootstrap] auth user already exists') | ||
| } else { | ||
| throw err | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Fragile error detection: check error code instead of message substring.
Checking err.message.includes('already') is brittle—the Firebase Admin SDK provides structured error codes. The auth/uid-already-exists code is stable across SDK versions.
🛡️ Proposed fix using error code
} catch (err: unknown) {
- if (err instanceof Error && err.message.includes('already')) {
+ if (
+ err instanceof Error &&
+ 'code' in err &&
+ (err as { code: string }).code === 'auth/uid-already-exists'
+ ) {
console.log('[bootstrap] auth user already exists')
} else {
throw err
}
}📝 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
| } catch (err: unknown) { | |
| if (err instanceof Error && err.message.includes('already')) { | |
| console.log('[bootstrap] auth user already exists') | |
| } else { | |
| throw err | |
| } | |
| } | |
| } catch (err: unknown) { | |
| if ( | |
| err instanceof Error && | |
| 'code' in err && | |
| (err as { code: string }).code === 'auth/uid-already-exists' | |
| ) { | |
| console.log('[bootstrap] auth user already exists') | |
| } else { | |
| throw err | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/phase-3b/bootstrap-test-responder.ts` around lines 48 - 54, Replace
the brittle substring check in the catch block that tests err.message with a
check against the Firebase error code: inspect err.code (e.g., compare to
'auth/uid-already-exists') instead of err.message.includes('already'), so in the
catch for the user creation flow (the catch handling the variable err in
scripts/phase-3b/bootstrap-test-responder.ts) log '[bootstrap] auth user already
exists' when err.code === 'auth/uid-already-exists' and rethrow otherwise; use a
safe cast or type guard (e.g., treat err as any or check typeof err === 'object'
&& 'code' in err) so you don’t rely on message parsing.
Comment on lines
+82
to
+84
| const webApp = webInitApp({ appId: 'demo' }) | ||
| const webAuth = webGetAuth(webApp) | ||
| const webFunctions = webGetFunctions(webApp) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Firebase web SDK initializeApp minimum required config fields
💡 Result:
The Firebase web SDK's initializeApp function accepts a FirebaseOptions config object with no TypeScript-required fields—all properties are optional. However, in practice, a minimal valid configuration typically requires at least the following fields obtained from the Firebase console when registering a web app: - apiKey: string (required for most API calls) - authDomain: string (for Authentication) - projectId: string (identifies the project) - storageBucket: string (for Cloud Storage) - messagingSenderId: string (for Cloud Messaging) - appId: string (unique app identifier, essential for app association) The complete standard config object includes these plus optional fields like databaseURL and measurementId. Example minimal config: const firebaseConfig = { apiKey: "AIza...", authDomain: "project.firebaseapp.com", projectId: "project", storageBucket: "project.appspot.com", messagingSenderId: "123456789", appId: "1:123456789:web:abcdef123456" }; const app = initializeApp(firebaseConfig); Note: In specific environments like Firebase App Hosting, initializeApp can be called with no arguments if FIREBASE_WEBAPP_CONFIG is set. For general web use, provide the full config from the console.
Citations:
- 1: https://firebase.google.com/docs/web/setup
- 2: https://firebase.google.com/docs/reference/js/app.md
- 3: https://firebase.google.com/docs/reference/js/app.firebaseoptions
- 4: Firebase InitializeCommand Recognition problem firebase/firebase-js-sdk#3949
🏁 Script executed:
# First, locate and read the acceptance.ts file
find . -name "acceptance.ts" -type f | head -20Repository: Exc1D/bantayog-alert
Length of output: 129
🏁 Script executed:
# Search for webInitApp and webGetFunctions definitions
rg -B2 -A8 "function webInitApp|export.*webInitApp|const webInitApp" --type tsRepository: Exc1D/bantayog-alert
Length of output: 46
🏁 Script executed:
# Search for webGetFunctions definitions
rg -B2 -A8 "function webGetFunctions|export.*webGetFunctions|const webGetFunctions" --type tsRepository: Exc1D/bantayog-alert
Length of output: 46
🏁 Script executed:
# Read the acceptance.ts file
cat -n ./scripts/phase-3b/acceptance.ts | head -100Repository: Exc1D/bantayog-alert
Length of output: 4254
🏁 Script executed:
# Search for imports of webInitApp, webGetAuth, webGetFunctions
rg "webInitApp|webGetAuth|webGetFunctions" --type ts -B2 -A2Repository: Exc1D/bantayog-alert
Length of output: 1153
🏁 Script executed:
# Search for web* functions more broadly - might be from SDK
rg "export.*web(Init|Get)" --type tsRepository: Exc1D/bantayog-alert
Length of output: 46
🏁 Script executed:
# Search for other usages of webGetFunctions or getFunctions in web SDK context
rg "getFunctions\(|getAuth\(|initializeApp\(" --type ts apps/citizen-pwa -B2 -A3Repository: Exc1D/bantayog-alert
Length of output: 450
🏁 Script executed:
# Check if there's any Firebase config or setup related to emulator in web SDK
rg "connectAuthEmulator|connectFunctionsEmulator|EMULATOR" --type tsRepository: Exc1D/bantayog-alert
Length of output: 2673
🏁 Script executed:
# Check if acceptance.ts continues and calls connectFunctionsEmulator anywhere
cat -n ./scripts/phase-3b/acceptance.ts | sed -n '80,150p'Repository: Exc1D/bantayog-alert
Length of output: 3222
🏁 Script executed:
# Search for connectFunctionsEmulator in entire acceptance.ts
rg "connectFunctionsEmulator" ./scripts/phase-3b/acceptance.tsRepository: Exc1D/bantayog-alert
Length of output: 46
🏁 Script executed:
# Check how admin-desktop and responder-app set up initializeApp for comparison
cat -n ./apps/admin-desktop/src/app/firebase.tsRepository: Exc1D/bantayog-alert
Length of output: 2275
Incomplete Firebase web SDK initialization and missing emulator setup.
Three issues here:
webInitApp({ appId: 'demo' })is missing required configuration fields. Provide at leastprojectIdandapiKeyto initialize the app properly.webGetFunctions(webApp)doesn't specify theasia-southeast1region. The backend Cloud Functions are deployed to that region, so the client must target it.- Missing
connectFunctionsEmulator()call in emulator mode. WhenEMUis true, you must connect the Functions client to the local emulator (on port 5001) or Cloud Function invocations will fail.
🐛 Proposed fix
const webApp = webInitApp({
+ apiKey: 'demo-key',
appId: 'demo',
+ projectId: PROJECT_ID,
})
const webAuth = webGetAuth(webApp)
- const webFunctions = webGetFunctions(webApp)
+ const webFunctions = webGetFunctions(webApp, 'asia-southeast1')
if (EMU) {
connectAuthEmulator(webAuth, 'http://localhost:9099', { disableWarnings: true })
+ // Import connectFunctionsEmulator at the top
+ connectFunctionsEmulator(webFunctions, 'localhost', 5001)
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/phase-3b/acceptance.ts` around lines 82 - 84, webInitApp is called
with only appId causing incomplete Firebase initialization; update the
webInitApp call to include at least projectId and apiKey (keep appId:'demo'),
then call webGetAuth(webApp) as before and initialize functions with
webGetFunctions(webApp, { region: 'asia-southeast1' }) so the client targets the
correct region; finally, when EMU is true, call
connectFunctionsEmulator(webFunctions, 'localhost', 5001) (or equivalent) to
connect to the local emulator so function invocations work locally.
…ort, function or class'
remove import { getFunctions } from 'firebase-admin/functions'
Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
scripts/phase-3b/acceptance.ts (1)
81-87:⚠️ Potential issue | 🟠 MajorWire the Functions client for both region and emulator mode.
Line 83 currently uses default region, and Line 85-87 only connects Auth emulator. In EMU mode this can still call deployed functions; in non-EMU mode it can target the wrong region.
🐛 Proposed fix
-import { httpsCallable, getFunctions as webGetFunctions } from 'firebase/functions' +import { httpsCallable, getFunctions as webGetFunctions, connectFunctionsEmulator } from 'firebase/functions' @@ - const webApp = webInitApp({ appId: 'demo' }) + const webApp = webInitApp({ + appId: 'demo', + projectId: PROJECT_ID, + apiKey: process.env.FIREBASE_WEB_API_KEY ?? 'demo-key', + }) const webAuth = webGetAuth(webApp) - const webFunctions = webGetFunctions(webApp) + const webFunctions = webGetFunctions(webApp, 'asia-southeast1') @@ if (EMU) { connectAuthEmulator(webAuth, 'http://localhost:9099', { disableWarnings: true }) + connectFunctionsEmulator(webFunctions, 'localhost', 5001) }Firebase JS SDK v9+: For callable functions deployed outside us-central1, is getFunctions(app, "<region>") required? In emulator mode, is connectFunctionsEmulator(functions, host, port) required to avoid hitting deployed functions?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/phase-3b/acceptance.ts` around lines 81 - 87, The Functions client is currently created with the default region and never wired to the emulator; update the initialization to explicitly request the deployed region via getFunctions(webApp, <region>) (use your deployed region constant or same region used elsewhere) instead of the default, and when EMU is true call connectFunctionsEmulator(webFunctions, 'localhost', 5001) (or the emulator host/port you run) after creating webFunctions so callable functions hit the emulator rather than deployed instances; update references around webGetFunctions and webFunctions accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/phase-3b/acceptance.ts`:
- Around line 71-77: The script calls adminAuth.setCustomUserClaims(adminUid,
...) and then adminAuth.createCustomToken(adminUid) but will fail if the user
with uid adminUid doesn't exist; modify the flow to first check for or fetch the
user (using adminAuth.getUser or getUserByEmail) and if not found create the
user with adminUid (and reasonable defaults like displayName/email if needed),
then call adminAuth.setCustomUserClaims(adminUid, {...}) and
adminAuth.createCustomToken(adminUid); ensure errors from getUser/createUser are
handled or surfaced so the acceptance run is deterministic.
---
Duplicate comments:
In `@scripts/phase-3b/acceptance.ts`:
- Around line 81-87: The Functions client is currently created with the default
region and never wired to the emulator; update the initialization to explicitly
request the deployed region via getFunctions(webApp, <region>) (use your
deployed region constant or same region used elsewhere) instead of the default,
and when EMU is true call connectFunctionsEmulator(webFunctions, 'localhost',
5001) (or the emulator host/port you run) after creating webFunctions so
callable functions hit the emulator rather than deployed instances; update
references around webGetFunctions and webFunctions accordingly.
🪄 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: d92fa6dc-8f16-4842-b541-c20dcf35b6f2
📒 Files selected for processing (1)
scripts/phase-3b/acceptance.ts
Comment on lines
+71
to
+77
| const adminUid = 'daet-admin-test-01' | ||
| await adminAuth.setCustomUserClaims(adminUid, { | ||
| role: 'municipal_admin', | ||
| municipalityId: 'daet', | ||
| active: true, | ||
| }) | ||
| const adminCustomToken = await adminAuth.createCustomToken(adminUid) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does firebase-admin Auth setCustomUserClaims(uid, claims) require that the user already exists? What error is returned when uid does not exist?
💡 Result:
Yes, firebase-admin Auth.setCustomUserClaims(uid, claims) requires that the user already exists in Firebase Authentication. It operates on an existing user record. When the uid does not exist, it returns an error with code auth/user-not-found and message "There is no user record corresponding to the provided identifier." (or similar phrasing like "There is no existing user record corresponding to the provided identifier."). This is confirmed by the official Firebase Admin Authentication API Errors documentation listing auth/user-not-found for operations requiring an existing user, and real-world examples where setCustomUserClaims fails with this exact error and message when the user does not exist.
Citations:
- 1: https://firebase.google.com/docs/auth/admin/errors
- 2: https://firebase.google.com./docs/auth/admin/errors
- 3: https://firebase-dot-devsite-v2-prod.appspot.com/docs/auth/admin/errors
- 4: https://stackoverflow.com/questions/77825916/unable-to-set-custom-claims-firebase-nodejs-sdk
- 5: Custom claims can't be set on a user made with makeUserRecord firebase/firebase-tools#3083
🏁 Script executed:
cat -n scripts/phase-3b/acceptance.ts | sed -n '60,90p'Repository: Exc1D/bantayog-alert
Length of output: 1259
🏁 Script executed:
rg -n "daet-admin-test-01|createUser|getUser" scripts/phase-3b/acceptance.tsRepository: Exc1D/bantayog-alert
Length of output: 106
🏁 Script executed:
head -n 70 scripts/phase-3b/acceptance.ts | tail -n 30Repository: Exc1D/bantayog-alert
Length of output: 878
🏁 Script executed:
head -n 40 scripts/phase-3b/acceptance.tsRepository: Exc1D/bantayog-alert
Length of output: 1591
🏁 Script executed:
wc -l scripts/phase-3b/acceptance.tsRepository: Exc1D/bantayog-alert
Length of output: 98
🏁 Script executed:
cat -n scripts/phase-3b/acceptance.ts | sed -n '1,120p'Repository: Exc1D/bantayog-alert
Length of output: 4979
🏁 Script executed:
cat -n scripts/phase-3b/acceptance.ts | sed -n '120,209p'Repository: Exc1D/bantayog-alert
Length of output: 3762
Ensure the admin user exists before setting custom claims.
setCustomUserClaims() will fail with auth/user-not-found if the user account doesn't already exist, making this acceptance script non-deterministic and dependent on external state.
🐛 Proposed fix
const adminUid = 'daet-admin-test-01'
+ try {
+ await adminAuth.getUser(adminUid)
+ } catch (err: unknown) {
+ const code = (err as { code?: string }).code
+ if (code === 'auth/user-not-found') {
+ await adminAuth.createUser({ uid: adminUid })
+ } else {
+ throw err
+ }
+ }
await adminAuth.setCustomUserClaims(adminUid, {
role: 'municipal_admin',
municipalityId: 'daet',
active: true,
})🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/phase-3b/acceptance.ts` around lines 71 - 77, The script calls
adminAuth.setCustomUserClaims(adminUid, ...) and then
adminAuth.createCustomToken(adminUid) but will fail if the user with uid
adminUid doesn't exist; modify the flow to first check for or fetch the user
(using adminAuth.getUser or getUserByEmail) and if not found create the user
with adminUid (and reasonable defaults like displayName/email if needed), then
call adminAuth.setCustomUserClaims(adminUid, {...}) and
adminAuth.createCustomToken(adminUid); ensure errors from getUser/createUser are
handled or surfaced so the acceptance run is deterministic.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
functions/src/callables/cancel-dispatch.ts (1)
80-82:⚠️ Potential issue | 🔴 Critical
pending -> cancelledis currently impossible here.
CANCELLABLE_FROM_STATESadmitspending, but the shared dispatch state machine does not definepending -> cancelled, so this branch throws on every valid cancellation attempt. That makes the success path unreachable until the shared transition table is updated or this second check is removed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@functions/src/callables/cancel-dispatch.ts` around lines 80 - 82, The branch throws because isValidDispatchTransition forbids pending -> cancelled even though CANCELLABLE_FROM_STATES includes pending; either add the pending->cancelled transition to the shared dispatch state machine or bypass this second validation here: in cancel-dispatch.ts, in the block that calls isValidDispatchTransition(from as DispatchStatus, to) (and currently throws BantayogError with BantayogErrorCode.INVALID_STATUS_TRANSITION), remove the redundant check or special-case allowing to === DispatchStatus.CANCELLED when from is in CANCELLABLE_FROM_STATES so legitimate cancellations succeed while preserving other transition validations.
🤖 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/responder-app/src/hooks/useOwnDispatches.ts`:
- Around line 33-43: In useOwnDispatches, the code unsafely casts Firestore
fields to Timestamp and coerces strings; instead perform runtime boundary
validation matching the schema: validate that data.dispatchedAt and optional
data.acknowledgementDeadlineAt are integers (epoch ms) using
typeof/Number.isInteger and only assign those numeric values to
OwnDispatchRow.dispatchedAt/acknowledgementDeadlineAt (do not cast to firebase
Timestamp), and remove the unnecessary String(...) around data.reportId and
data.status (assign as strings after asserting typeof === 'string'); update the
mapping in the snap.docs.map block and return/skip rows when validation fails.
In `@functions/src/__tests__/callables/cancel-dispatch.test.ts`:
- Around line 83-95: The test assertions for cancelDispatchCore use the wrong
domain error codes; update the expectations so that when calling
cancelDispatchCore(...) with an actor from another municipality the test asserts
rejects.toMatchObject({ code: 'FORBIDDEN' }) (use the domain-level FORBIDDEN
from cancelDispatchCore), and when attempting to cancel a dispatch that is no
longer pending assert rejects.toMatchObject({ code: 'FAILED_PRECONDITION' })
instead of INVALID_STATUS_TRANSITION; adjust the other similar assertions in the
same file (the second block around the later lines) to match these domain error
codes as well.
In `@functions/src/callables/cancel-dispatch.ts`:
- Around line 45-50: The idempotency payload currently includes the transient
deps.now, so withIdempotency(key:
`cancelDispatch:${deps.actor.uid}:${deps.idempotencyKey}`, payload: deps, ...)
will change on every call and break retries; update the call sites that pass
payload: deps (including the call in cancelDispatch and the other occurrences
around the other handlers) to pass a stable payload object that excludes
deps.now (or only includes deterministic fields used to compute idempotency),
e.g., construct a payload containing actor.uid, idempotencyKey, dispatch id and
other invariant request data but not the Timestamp.now value, so retries with
the same idempotencyKey will hit the cache and not be rate-limited.
In `@functions/src/callables/dispatch-responder.ts`:
- Around line 122-125: The code currently casts report.severityDerived to keyof
typeof DEADLINE_BY_SEVERITY which can falsely assert an invalid string is a
valid key; instead, read the raw value (report.severityDerived ?? 'medium'),
validate it against the keys of DEADLINE_BY_SEVERITY (e.g., with a
hasOwnProperty or "in" check) and only then narrow or cast to keyof typeof
DEADLINE_BY_SEVERITY; finally use that validated severity to compute deadlineMs
from DEADLINE_BY_SEVERITY (falling back to DEADLINE_BY_SEVERITY.high or 'medium'
if validation fails). Use the identifiers report.severityDerived, severity,
DEADLINE_BY_SEVERITY, and deadlineMs to locate and update the logic.
- Around line 46-52: The idempotency payload currently includes deps (which
contains deps.now, a Timestamp), causing the payload hash to change on retry;
update the withIdempotency call in dispatch-responder (the payload passed to
withIdempotency) to exclude the transient deps.now value — e.g., construct a
payload object that omits deps.now (or copy deps and delete the now field) and
pass that as payload so the idempotency key/hash remains stable across retries
while keeping the same key
(`dispatchResponder:${deps.actor.uid}:${deps.idempotencyKey}`) and other fields.
---
Duplicate comments:
In `@functions/src/callables/cancel-dispatch.ts`:
- Around line 80-82: The branch throws because isValidDispatchTransition forbids
pending -> cancelled even though CANCELLABLE_FROM_STATES includes pending;
either add the pending->cancelled transition to the shared dispatch state
machine or bypass this second validation here: in cancel-dispatch.ts, in the
block that calls isValidDispatchTransition(from as DispatchStatus, to) (and
currently throws BantayogError with
BantayogErrorCode.INVALID_STATUS_TRANSITION), remove the redundant check or
special-case allowing to === DispatchStatus.CANCELLED when from is in
CANCELLABLE_FROM_STATES so legitimate cancellations succeed while preserving
other transition validations.
🪄 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: ee08316e-5191-40fe-8a0d-19694d6f1bda
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (5)
apps/responder-app/package.jsonapps/responder-app/src/hooks/useOwnDispatches.tsfunctions/src/__tests__/callables/cancel-dispatch.test.tsfunctions/src/callables/cancel-dispatch.tsfunctions/src/callables/dispatch-responder.ts
Comment on lines
+33
to
+43
| snap.docs.map((d) => { | ||
| const data = d.data() | ||
| const row: OwnDispatchRow = { | ||
| dispatchId: d.id, | ||
| reportId: String(data.reportId), | ||
| status: String(data.status), | ||
| dispatchedAt: data.dispatchedAt as Timestamp, | ||
| } | ||
| if (data.acknowledgementDeadlineAt) { | ||
| row.acknowledgementDeadlineAt = data.acknowledgementDeadlineAt as Timestamp | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "1) Source of truth for dispatch document field types"
rg -n "dispatchedAt: z\\.number\\(\\)\\.int\\(\\)|acknowledgementDeadlineAt: z\\.number\\(\\)\\.int\\(\\)|assignedTo: z\\.object" packages/shared-validators/src/dispatches.ts
echo
echo "2) Current hook assumptions/coercions"
rg -n "String\\(data\\.reportId\\)|String\\(data\\.status\\)|as Timestamp|acknowledgementDeadlineAt" apps/responder-app/src/hooks/useOwnDispatches.tsRepository: Exc1D/bantayog-alert
Length of output: 624
🏁 Script executed:
# Read the useOwnDispatches.ts file to see full context
head -60 apps/responder-app/src/hooks/useOwnDispatches.tsRepository: Exc1D/bantayog-alert
Length of output: 1739
🏁 Script executed:
# Read the dispatch schema to confirm field types
head -60 packages/shared-validators/src/dispatches.tsRepository: Exc1D/bantayog-alert
Length of output: 2157
🏁 Script executed:
# Check if there's any validation applied to the Firestore query result
rg -A 5 -B 5 "setRows" apps/responder-app/src/hooks/useOwnDispatches.tsRepository: Exc1D/bantayog-alert
Length of output: 1000
Fix boundary validation and correct timestamp field types.
The hook receives dispatch data from Firestore where the schema defines dispatchedAt and acknowledgementDeadlineAt as z.number().int() (epoch milliseconds), yet the code casts them to Firebase Timestamp type without validation. Additionally, String(data.reportId) and String(data.status) are unnecessary coercions since the schema already validates these as strings. Apply runtime validation at the boundary to match the schema and eliminate unsafe type assertions.
Proposed hardening diff
export interface OwnDispatchRow {
dispatchId: string
reportId: string
status: string
- dispatchedAt: Timestamp
- acknowledgementDeadlineAt?: Timestamp
+ dispatchedAt: number
+ acknowledgementDeadlineAt?: number
}
@@
setRows(
snap.docs.map((d) => {
const data = d.data()
+ if (
+ typeof data.reportId !== 'string' ||
+ typeof data.status !== 'string' ||
+ typeof data.dispatchedAt !== 'number'
+ ) {
+ throw new Error(`Invalid dispatch payload for document ${d.id}`)
+ }
const row: OwnDispatchRow = {
dispatchId: d.id,
- reportId: String(data.reportId),
- status: String(data.status),
- dispatchedAt: data.dispatchedAt as Timestamp,
+ reportId: data.reportId,
+ status: data.status,
+ dispatchedAt: data.dispatchedAt,
}
- if (data.acknowledgementDeadlineAt) {
- row.acknowledgementDeadlineAt = data.acknowledgementDeadlineAt as Timestamp
+ if (typeof data.acknowledgementDeadlineAt === 'number') {
+ row.acknowledgementDeadlineAt = data.acknowledgementDeadlineAt
}
return row
}),
)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/responder-app/src/hooks/useOwnDispatches.ts` around lines 33 - 43, In
useOwnDispatches, the code unsafely casts Firestore fields to Timestamp and
coerces strings; instead perform runtime boundary validation matching the
schema: validate that data.dispatchedAt and optional
data.acknowledgementDeadlineAt are integers (epoch ms) using
typeof/Number.isInteger and only assign those numeric values to
OwnDispatchRow.dispatchedAt/acknowledgementDeadlineAt (do not cast to firebase
Timestamp), and remove the unnecessary String(...) around data.reportId and
data.status (assign as strings after asserting typeof === 'string'); update the
mapping in the snap.docs.map block and return/skip rows when validation fails.
Comment on lines
+83
to
+95
| await expect( | ||
| cancelDispatchCore(db, { | ||
| dispatchId, | ||
| reason: 'responder_unavailable', | ||
| idempotencyKey: crypto.randomUUID(), | ||
| actor: { | ||
| uid: 'admin-1', | ||
| claims: staffClaims({ role: 'municipal_admin', municipalityId: 'daet' }), | ||
| }, | ||
| now: Timestamp.now(), | ||
| }), | ||
| ).rejects.toMatchObject({ code: 'PERMISSION_DENIED' }) | ||
| }) |
Contributor
There was a problem hiding this comment.
These expected error codes don't match cancelDispatchCore.
This suite calls the core function directly, so the cross-municipality case should assert the domain code FORBIDDEN, not the HTTPS-layer PERMISSION_DENIED. Also, once a dispatch is no longer pending, the core exits through the pending-only guard first, so the second cancel should expect FAILED_PRECONDITION, not INVALID_STATUS_TRANSITION.
Suggested assertion fix
- ).rejects.toMatchObject({ code: 'PERMISSION_DENIED' })
+ ).rejects.toMatchObject({ code: 'FORBIDDEN' })
...
- ).rejects.toMatchObject({ code: 'INVALID_STATUS_TRANSITION' })
+ ).rejects.toMatchObject({ code: 'FAILED_PRECONDITION' })Also applies to: 211-223
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@functions/src/__tests__/callables/cancel-dispatch.test.ts` around lines 83 -
95, The test assertions for cancelDispatchCore use the wrong domain error codes;
update the expectations so that when calling cancelDispatchCore(...) with an
actor from another municipality the test asserts rejects.toMatchObject({ code:
'FORBIDDEN' }) (use the domain-level FORBIDDEN from cancelDispatchCore), and
when attempting to cancel a dispatch that is no longer pending assert
rejects.toMatchObject({ code: 'FAILED_PRECONDITION' }) instead of
INVALID_STATUS_TRANSITION; adjust the other similar assertions in the same file
(the second block around the later lines) to match these domain error codes as
well.
Comment on lines
+45
to
+50
| const { result } = await withIdempotency( | ||
| db, | ||
| { | ||
| key: `cancelDispatch:${deps.actor.uid}:${deps.idempotencyKey}`, | ||
| payload: deps, | ||
| now: () => deps.now.toMillis(), |
Contributor
There was a problem hiding this comment.
Retries with the same idempotency key are not actually safe.
withIdempotency() hashes the full payload, and payload: deps includes now, which changes on every call because the wrapper passes a fresh Timestamp.now(). A client retrying the same request after a timeout will therefore miss the cached result or hit an idempotency mismatch, and the pre-idempotency rate-limit check can throttle that retry before the cache is consulted.
Also applies to: 165-175, 191-191
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@functions/src/callables/cancel-dispatch.ts` around lines 45 - 50, The
idempotency payload currently includes the transient deps.now, so
withIdempotency(key: `cancelDispatch:${deps.actor.uid}:${deps.idempotencyKey}`,
payload: deps, ...) will change on every call and break retries; update the call
sites that pass payload: deps (including the call in cancelDispatch and the
other occurrences around the other handlers) to pass a stable payload object
that excludes deps.now (or only includes deterministic fields used to compute
idempotency), e.g., construct a payload containing actor.uid, idempotencyKey,
dispatch id and other invariant request data but not the Timestamp.now value, so
retries with the same idempotencyKey will hit the cache and not be rate-limited.
Comment on lines
+46
to
+52
| const { result } = await withIdempotency( | ||
| db, | ||
| { | ||
| key: `dispatchResponder:${deps.actor.uid}:${deps.idempotencyKey}`, | ||
| payload: deps, | ||
| now: () => deps.now.toMillis(), | ||
| }, |
Contributor
There was a problem hiding this comment.
Idempotency payload includes now timestamp, breaking retry semantics.
The payload: deps includes deps.now (a Timestamp). On retry, line 234 generates a fresh Timestamp.now(), so the payload hash will differ from the original request. Per the withIdempotency contract (context snippet 1), this throws IdempotencyMismatchError instead of returning the cached result.
This defeats the purpose of idempotency—clients cannot safely retry the same dispatch request.
🐛 Proposed fix: exclude timestamp from idempotency payload
const { result } = await withIdempotency(
db,
{
key: `dispatchResponder:${deps.actor.uid}:${deps.idempotencyKey}`,
- payload: deps,
+ payload: {
+ reportId: deps.reportId,
+ responderUid: deps.responderUid,
+ idempotencyKey: deps.idempotencyKey,
+ actorUid: deps.actor.uid,
+ municipalityId: deps.actor.claims.municipalityId,
+ },
now: () => deps.now.toMillis(),
},🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@functions/src/callables/dispatch-responder.ts` around lines 46 - 52, The
idempotency payload currently includes deps (which contains deps.now, a
Timestamp), causing the payload hash to change on retry; update the
withIdempotency call in dispatch-responder (the payload passed to
withIdempotency) to exclude the transient deps.now value — e.g., construct a
payload object that omits deps.now (or copy deps and delete the now field) and
pass that as payload so the idempotency key/hash remains stable across retries
while keeping the same key
(`dispatchResponder:${deps.actor.uid}:${deps.idempotencyKey}`) and other fields.
Comment on lines
+122
to
+125
| const severity = ((report.severityDerived as string | null | undefined) ?? | ||
| 'medium') as keyof typeof DEADLINE_BY_SEVERITY | ||
| // eslint-disable-next-line @typescript-eslint/no-unnecessary-condition | ||
| const deadlineMs = DEADLINE_BY_SEVERITY[severity] ?? DEADLINE_BY_SEVERITY.high |
Contributor
There was a problem hiding this comment.
Type assertion on severity may mislead maintainers.
If severityDerived is an unexpected string (e.g., "unknown"), the cast as keyof typeof DEADLINE_BY_SEVERITY is incorrect. The fallback on line 125 handles this gracefully, but the type assertion creates a false sense of type safety.
♻️ Suggested improvement: explicit validation
- const severity = ((report.severityDerived as string | null | undefined) ??
- 'medium') as keyof typeof DEADLINE_BY_SEVERITY
- // eslint-disable-next-line `@typescript-eslint/no-unnecessary-condition`
- const deadlineMs = DEADLINE_BY_SEVERITY[severity] ?? DEADLINE_BY_SEVERITY.high
+ const rawSeverity = (report.severityDerived as string | null | undefined) ?? 'medium'
+ const severity = rawSeverity in DEADLINE_BY_SEVERITY
+ ? (rawSeverity as keyof typeof DEADLINE_BY_SEVERITY)
+ : 'high'
+ const deadlineMs = DEADLINE_BY_SEVERITY[severity]📝 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
| const severity = ((report.severityDerived as string | null | undefined) ?? | |
| 'medium') as keyof typeof DEADLINE_BY_SEVERITY | |
| // eslint-disable-next-line @typescript-eslint/no-unnecessary-condition | |
| const deadlineMs = DEADLINE_BY_SEVERITY[severity] ?? DEADLINE_BY_SEVERITY.high | |
| const rawSeverity = (report.severityDerived as string | null | undefined) ?? 'medium' | |
| const severity = rawSeverity in DEADLINE_BY_SEVERITY | |
| ? (rawSeverity as keyof typeof DEADLINE_BY_SEVERITY) | |
| : 'high' | |
| const deadlineMs = DEADLINE_BY_SEVERITY[severity] |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@functions/src/callables/dispatch-responder.ts` around lines 122 - 125, The
code currently casts report.severityDerived to keyof typeof DEADLINE_BY_SEVERITY
which can falsely assert an invalid string is a valid key; instead, read the raw
value (report.severityDerived ?? 'medium'), validate it against the keys of
DEADLINE_BY_SEVERITY (e.g., with a hasOwnProperty or "in" check) and only then
narrow or cast to keyof typeof DEADLINE_BY_SEVERITY; finally use that validated
severity to compute deadlineMs from DEADLINE_BY_SEVERITY (falling back to
DEADLINE_BY_SEVERITY.high or 'medium' if validation fails). Use the identifiers
report.severityDerived, severity, DEADLINE_BY_SEVERITY, and deadlineMs to locate
and update the logic.
…tyId clears Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/shared-validators/src/state-machines/report-states.ts (1)
77-83:⚠️ Potential issue | 🔴 Critical
pending → cancelledhere exposes a server-only transition to client-writable rules.Line 81 propagates into Firestore rules and allows responder-side status mutation to
cancelled, which can bypasscancelDispatchcallable side effects (report reversion + event consistency).Safer direction (split transition sets)
-export const DISPATCH_TRANSITIONS: readonly [DispatchStatus, DispatchStatus][] = [ +// Used by Firestore rules (responder-direct only) +export const DISPATCH_RESPONDER_TRANSITIONS: readonly [DispatchStatus, DispatchStatus][] = [ ['accepted', 'acknowledged'], ['acknowledged', 'in_progress'], ['in_progress', 'resolved'], - ['pending', 'cancelled'], ['pending', 'declined'], ] as const + +// Used by server callables +export const DISPATCH_SERVER_TRANSITIONS: readonly [DispatchStatus, DispatchStatus][] = [ + ...DISPATCH_RESPONDER_TRANSITIONS, + ['pending', 'cancelled'], +] as const🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/shared-validators/src/state-machines/report-states.ts` around lines 77 - 83, DISPATCH_TRANSITIONS currently includes the server-only transition ['pending', 'cancelled'], which leaks a server-side-only state change into client-writable rules; split the transitions into two clearly named sets (e.g., DISPATCH_CLIENT_TRANSITIONS and DISPATCH_SERVER_TRANSITIONS or DISPATCH_TRANSITIONS and DISPATCH_SERVER_ONLY_TRANSITIONS), remove ['pending','cancelled'] from the client-writable set while keeping it in the server-only set, and update any consumers (security rule generator or validators that reference DISPATCH_TRANSITIONS) to use the appropriate set so clients cannot mutate status to 'cancelled' directly via the client-writable transition list.infra/firebase/firestore.rules (1)
1-356:⚠️ Potential issue | 🟠 MajorCodegen drift is blocking CI.
firestore.rulesis currently not synchronized with the generator output; CI is already failing on this. Regenerate and commit the updated rules artifact before merge.Suggested verification/fix commands
pnpm exec tsx scripts/build-rules.ts git diff -- infra/firebase/firestore.rules🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@infra/firebase/firestore.rules` around lines 1 - 356, The CI failure is due to infra/firebase/firestore.rules being out of sync with the codegen; regenerate the rules with the generator script (scripts/build-rules.ts) so the helper functions (e.g. isAuthed, isSuperadmin, adminOf) and all match blocks reflect the current spec, then commit the updated infra/firebase/firestore.rules artifact; verify by running the generator and checking git diff before pushing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@infra/firebase/firestore.rules`:
- Around line 1-356: The CI failure is due to infra/firebase/firestore.rules
being out of sync with the codegen; regenerate the rules with the generator
script (scripts/build-rules.ts) so the helper functions (e.g. isAuthed,
isSuperadmin, adminOf) and all match blocks reflect the current spec, then
commit the updated infra/firebase/firestore.rules artifact; verify by running
the generator and checking git diff before pushing.
In `@packages/shared-validators/src/state-machines/report-states.ts`:
- Around line 77-83: DISPATCH_TRANSITIONS currently includes the server-only
transition ['pending', 'cancelled'], which leaks a server-side-only state change
into client-writable rules; split the transitions into two clearly named sets
(e.g., DISPATCH_CLIENT_TRANSITIONS and DISPATCH_SERVER_TRANSITIONS or
DISPATCH_TRANSITIONS and DISPATCH_SERVER_ONLY_TRANSITIONS), remove
['pending','cancelled'] from the client-writable set while keeping it in the
server-only set, and update any consumers (security rule generator or validators
that reference DISPATCH_TRANSITIONS) to use the appropriate set so clients
cannot mutate status to 'cancelled' directly via the client-writable transition
list.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: b83e12f9-52d6-482a-b01f-af6c16e295a2
📒 Files selected for processing (3)
apps/admin-desktop/src/routes.tsxinfra/firebase/firestore.rulespackages/shared-validators/src/state-machines/report-states.ts
… to 5 Added ['pending','cancelled'] transition in prior commit raised the count from 4 to 5. Update the spec-count test to match. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
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 `@apps/admin-desktop/src/hooks/useEligibleResponders.ts`:
- Around line 31-35: The client fallback defaults in useEligibleResponders.ts
currently set displayName to String(data.displayName ?? d.id) and agencyId to
String(data.agencyId ?? 'unknown'); change these to match the backend
eligibility contract by defaulting both to empty strings (i.e., use
data.displayName ?? '' and data.agencyId ?? '') so sorting and rendered labels
align with functions/src/services/responder-eligibility.ts; update the object
assigned to out[d.id] (uid, displayName, agencyId) accordingly.
- Around line 48-51: The RTDB snapshot is being cast unchecked and stored via
setShift in the useEligibleResponders hook; instead validate snapVal's shape
before committing: check that snapVal is an object (not null/array), iterate its
keys and ensure each value is an object with a boolean isOnShift (filter or
coerce invalid entries), and only then call setShift with the cleaned/typed
record; also wrap parsing/validation in try/catch and log any errors rather than
swallowing them so malformed payloads don’t corrupt state (refer to the onValue
callback, snapVal, and setShift).
🪄 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: c85a664e-a53d-46f4-a083-59001702c5f9
📒 Files selected for processing (1)
apps/admin-desktop/src/hooks/useEligibleResponders.ts
Comment on lines
+31
to
+35
| out[d.id] = { | ||
| uid: d.id, | ||
| displayName: String(data.displayName ?? d.id), | ||
| agencyId: String(data.agencyId ?? 'unknown'), | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Align fallback normalization with backend eligibility contract.
Line [33]-Line [34] default to uid / 'unknown', but functions/src/services/responder-eligibility.ts defaults to empty strings. This can cause client/server divergence in sorting and rendered labels for missing profile fields.
Proposed fix
out[d.id] = {
uid: d.id,
- displayName: String(data.displayName ?? d.id),
- agencyId: String(data.agencyId ?? 'unknown'),
+ displayName: String(data.displayName ?? ''),
+ agencyId: String(data.agencyId ?? ''),
}📝 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
| out[d.id] = { | |
| uid: d.id, | |
| displayName: String(data.displayName ?? d.id), | |
| agencyId: String(data.agencyId ?? 'unknown'), | |
| } | |
| out[d.id] = { | |
| uid: d.id, | |
| displayName: String(data.displayName ?? ''), | |
| agencyId: String(data.agencyId ?? ''), | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/admin-desktop/src/hooks/useEligibleResponders.ts` around lines 31 - 35,
The client fallback defaults in useEligibleResponders.ts currently set
displayName to String(data.displayName ?? d.id) and agencyId to
String(data.agencyId ?? 'unknown'); change these to match the backend
eligibility contract by defaulting both to empty strings (i.e., use
data.displayName ?? '' and data.agencyId ?? '') so sorting and rendered labels
align with functions/src/services/responder-eligibility.ts; update the object
assigned to out[d.id] (uid, displayName, agencyId) accordingly.
Comment on lines
+48
to
+51
| const unsub = onValue(node, (s) => { | ||
| const snapVal = s.val() | ||
| setShift(snapVal !== null ? (snapVal as Record<string, { isOnShift: boolean }>) : {}) | ||
| }) |
Contributor
There was a problem hiding this comment.
Validate RTDB snapshot shape before committing to state.
Line [50] trusts untyped RTDB payload via direct cast. This skips boundary validation and can store malformed entries.
Proposed fix
const unsub = onValue(node, (s) => {
const snapVal = s.val()
- setShift(snapVal !== null ? (snapVal as Record<string, { isOnShift: boolean }>) : {})
+ if (!snapVal || typeof snapVal !== 'object') {
+ setShift({})
+ return
+ }
+ const normalized: Record<string, { isOnShift: boolean }> = {}
+ for (const [uid, value] of Object.entries(snapVal as Record<string, unknown>)) {
+ if (value && typeof value === 'object' && (value as { isOnShift?: unknown }).isOnShift === true) {
+ normalized[uid] = { isOnShift: true }
+ }
+ }
+ setShift(normalized)
})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
| const unsub = onValue(node, (s) => { | |
| const snapVal = s.val() | |
| setShift(snapVal !== null ? (snapVal as Record<string, { isOnShift: boolean }>) : {}) | |
| }) | |
| const unsub = onValue(node, (s) => { | |
| const snapVal = s.val() | |
| if (!snapVal || typeof snapVal !== 'object') { | |
| setShift({}) | |
| return | |
| } | |
| const normalized: Record<string, { isOnShift: boolean }> = {} | |
| for (const [uid, value] of Object.entries(snapVal as Record<string, unknown>)) { | |
| if (value && typeof value === 'object' && (value as { isOnShift?: unknown }).isOnShift === true) { | |
| normalized[uid] = { isOnShift: true } | |
| } | |
| } | |
| setShift(normalized) | |
| }) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/admin-desktop/src/hooks/useEligibleResponders.ts` around lines 48 - 51,
The RTDB snapshot is being cast unchecked and stored via setShift in the
useEligibleResponders hook; instead validate snapVal's shape before committing:
check that snapVal is an object (not null/array), iterate its keys and ensure
each value is an object with a boolean isOnShift (filter or coerce invalid
entries), and only then call setShift with the cleaned/typed record; also wrap
parsing/validation in try/catch and log any errors rather than swallowing them
so malformed payloads don’t corrupt state (refer to the onValue callback,
snapVal, and setShift).
This was referenced Apr 19, 2026
5 tasks
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
3 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
new → awaiting_verify → verifiedwith idempotency, event log, and municipality-gated rulesawaiting_verify → cancelled_false_reportwith structured moderation incident recordverifiedTest plan
pnpm test— 127 tests passingpnpm lint— cleanpnpm typecheck— cleanpnpm build— 10 apps/packages builtscripts/phase-3b/acceptance.tsagainst emulator (manual acceptance gate)Summary by Sourcery
Implement admin triage and dispatch flow across backend callables, admin desktop UI, responder app, and monitoring, including verification, rejection, dispatch, and cancellation of reports.
New Features:
Bug Fixes:
Enhancements:
Documentation:
Tests:
Summary by CodeRabbit
New Features
Tests
Documentation