feat(phase-3c): responder loop end-to-end

[Exc1D]

Summary

• Responder PWA: Accept dispatch, advance status flow (acknowledged → en_route → on_scene → resolved), race-loss UX, FCM push notification service worker
• Backend callables: acceptDispatch, advanceDispatch, cancelDispatch, closeReport (Phase 3c state machine: pending→accepted→acknowledged→en_route→on_scene→resolved→closed)
• Triggers: dispatchMirrorToReport mirrors dispatch state back to report, dispatchTimeoutSweep atomic re-dispatch on timeout, onMediaFinalize EXIF stripping fix
• E2E tests: Playwright skeleton + acceptance tests for citizen lookup, admin dispatch, responder loop, race-loss
• Bug fixes: Idempotency guard now returns fromCache flag, PII scrubbing via scrubbedDescription field, adversarial audit proof-of-concept tests

Test plan

• pnpm test — 142 tests passing
• pnpm lint — pass (pre-existing test lint issues deferred)
• pnpm typecheck — pass
• pnpm build — pass

Summary by CodeRabbit

• New Features

  • Admin: cancel dispatches from additional lifecycle stages and close reports via modals; triage panel can launch close-flow.
  • Responder PWA: dispatch list links to a dispatch detail page, cancelled screen, status progression controls, background push (service worker) and FCM token registration; new hooks for accept/advance/subscribe/real-time dispatch.

• Bug Fixes

  • Improved real-time dispatch list/status sync, idempotent backend operations, and race-condition safety.

• Tests

  • Large Playwright E2E suites and many unit/integration tests covering dispatch lifecycle, race conditions, FCM, and callables.

• Documentation

  • Phase 3c runbooks, verification checklists, VAPID rotation guide, and implementation/readiness notes added.

[sourcery-ai]

Sorry @Exc1D, your pull request is larger than the review limit of 150000 diff characters

[coderabbitai]

Warning

Rate limit exceeded

@Exc1D has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 2 minutes and 31 seconds before requesting another review.

Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 2 minutes and 31 seconds.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 7dd07c53-ebf9-4152-ae2b-840f6972ea67

📥 Commits

Reviewing files that changed from the base of the PR and between 89d83e6 and ae54957.

📒 Files selected for processing (28)

• apps/responder-app/public/firebase-messaging-sw.js
• apps/responder-app/src/App.tsx
• apps/responder-app/src/hooks/useAcceptDispatch.ts
• apps/responder-app/src/hooks/useAdvanceDispatch.ts
• apps/responder-app/src/hooks/useRegisterFcmToken.ts
• apps/responder-app/src/pages/DispatchDetailPage.tsx
• apps/responder-app/src/services/fcm-client.ts
• docs/learnings.md
• docs/progress.md
• e2e-tests/specs/admin.spec.ts
• e2e-tests/specs/full-loop.spec.ts
• e2e-tests/specs/race-loss.spec.ts
• e2e-tests/specs/responder.spec.ts
• e2e-tests/tsconfig.json
• functions/src/__tests__/callables/advance-dispatch.test.ts
• functions/src/__tests__/callables/cancel-dispatch.test.ts
• functions/src/__tests__/callables/close-report.test.ts
• functions/src/__tests__/helpers/seed-factories.ts
• functions/src/__tests__/rules/coordination.rules.test.ts
• functions/src/__tests__/rules/responder-direct-writes.rules.test.ts
• functions/src/__tests__/services/fcm-send.test.ts
• functions/src/callables/accept-dispatch.ts
• functions/src/callables/https-error.ts
• functions/src/triggers/dispatch-timeout-sweep.ts
• infra/firebase/firestore.rules
• packages/shared-validators/src/dispatches.ts
• packages/shared-validators/src/index.ts
• scripts/phase-3c/acceptance.ts

📝 Walkthrough

Walkthrough

Implements Phase 3c: adds responder UI/routes/hooks and FCM (service worker, client helpers, registration), admin cancel/close modals, new callables/triggers for dispatch lifecycle (accept/advance/close/cancel/mirror/timeout), Firestore rule changes, shared-types/state-machine updates, extensive tests, Playwright E2E suite, and supporting docs/scripts.

Changes

Cohort / File(s)	Summary
Responder PWA: pages & routes apps/responder-app/src/pages/DispatchDetailPage.tsx, apps/responder-app/src/pages/DispatchListPage.tsx, apps/responder-app/src/pages/CancelledScreen.tsx, apps/responder-app/src/routes.tsx	Adds dispatch detail & cancelled pages, links from list to /dispatches/:dispatchId, and renders status-driven UI flows including accept/advance actions.
Responder PWA: hooks & FCM apps/responder-app/src/hooks/*, apps/responder-app/src/services/fcm-client.ts, apps/responder-app/src/App.tsx, apps/responder-app/public/firebase-messaging-sw.js, apps/responder-app/package.json, apps/responder-app/vite.config.ts	Adds hooks: useDispatch, useOwnDispatches change, useAcceptDispatch, useAdvanceDispatch, useRegisterFcmToken; FCM client (acquireFcmToken, subscribeForeground), SW, App FCM setup, and workspace dependency.
Admin desktop: modals & wiring apps/admin-desktop/src/pages/CancelDispatchModal.tsx, apps/admin-desktop/src/pages/CloseReportModal.tsx, apps/admin-desktop/src/pages/ReportDetailPanel.tsx, apps/admin-desktop/src/pages/TriageQueuePage.tsx, apps/admin-desktop/src/services/callables.ts	Adds CancelDispatch and CloseReport modal components, wires Close flow from ReportDetailPanel/TriageQueuePage, and exposes callables.closeReport.
Cloud Functions: callables functions/src/callables/*.ts	Adds idempotent callables: acceptDispatch (rate-limited), advanceDispatch (strict transitions, resolutionSummary enforcement), closeReport (auth/municipality checks, idempotency), updates to cancelDispatch (expanded allowed states, idempotency payload changes), dispatchResponder (FCM warnings), verifyReport enhancements, and requireAuth helper.
Cloud Functions: triggers & services functions/src/triggers/dispatch-mirror-to-report.ts, functions/src/triggers/dispatch-timeout-sweep.ts, functions/src/triggers/on-media-finalize.ts, functions/src/services/fcm-send.ts	Adds dispatch→report mirror trigger & core, scheduled timeout sweep for pending dispatches, media finalize robustness (buffer/size checks), and FCM send service with secret, retry and invalid-token cleanup.
Functions exports & tests functions/src/index.ts, functions/src/__tests__/**	Re-exports new callables/triggers; adds extensive unit/integration/adversarial tests covering callables, triggers, rules, services, and schema validation.
Firestore rules & template infra/firebase/firestore.rules, infra/firebase/firestore.rules.template	Updates responder transition sets (en_route/on_scene), tightens responder update field restrictions, denies responder writes to reports, switches assignment checks to assignedTo.uid, and adds helper functions for allowed fields.
Shared types & validators packages/shared-types/src/enums.ts, packages/shared-types/src/states.ts, packages/shared-validators/src/*	Replaces in_progress with en_route/on_scene; restructures dispatch transitions to a mapping; adds advanceDispatchRequestSchema, updates dispatchToReportState mapping, and adjusts related tests/exports.
E2E tests & workspace e2e-tests/*, pnpm-workspace.yaml, eslint.config.js	Adds e2e-tests workspace, Playwright configs (including staging), many spec files (admin/citizen/responder/full-loop/race-loss), tsconfig, scripts, and excludes SW file from ESLint processing.
Docs & scripts docs/*, scripts/phase-3c/*, scripts/build-rules.ts	Phase 3c docs, VAPID rotation runbook, acceptance and precondition verification scripts, and build-rules parser updates to support record-format transitions.

Sequence Diagram(s)

sequenceDiagram
    participant Responder as Responder (PWA)
    participant SW as Service Worker
    participant Functions as Cloud Functions
    participant Firestore as Firestore DB
    participant Admin as Admin (Desktop)

    Responder->>SW: register service worker / request token
    SW-->>Responder: FCM token
    Responder->>Functions: register FCM token (update responder.fcmTokens)
    Functions->>Firestore: update responder doc

    Responder->>Firestore: onSnapshot(dispatches query)
    Firestore-->>Responder: dispatch docs

    Responder->>Functions: call acceptDispatch(dispatchId, idempotencyKey)
    Functions->>Firestore: transaction: set dispatch accepted + event
    Firestore-->>Responder: onSnapshot update

    Responder->>Functions: call advanceDispatch(to)
    Functions->>Firestore: transaction: update dispatch status/timestamps + event
    Firestore->>Functions: onDocumentWritten -> dispatchMirrorToReport
    Functions->>Firestore: update report status / write report_event

    Admin->>Functions: call cancelDispatch / closeReport
    Functions->>Firestore: transaction updates (dispatch/report) + events

Loading

Estimated code review effort

🎯 5 (Critical) | ⏱️ ~120 minutes

Possibly related PRs

• feat(admin): Phase 3b — Admin Triage Dispatch #44: strong overlap—modifies admin dispatch/cancellation UI and backend cancel/dispatch logic.
• feat(phase-2): data model and Firestore security rules foundation #42: related to idempotency, dispatch/report callables and triggers, and shared-type/state-machine changes.
• feat(phase-3a): citizen submission + triptych materialization #43: related to rules-generation and state-machine artifacts (scripts/build-rules.ts and firestore.rules.template).

Poem

🐰 I hopped through code and fields,
I left my footprints in dispatching yields.
From pending leaps to on_scene bounds,
Idempotent carrots keep the rounds.
Phase 3c blooms — I nibbled all the mounds.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch feature/phase-3c-responder-loop-e2e

[coderabbitai]

Actionable comments posted: 54

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@apps/admin-desktop/src/pages/CancelDispatchModal.tsx`:
- Around line 80-86: The message in CancelDispatchModal.tsx incorrectly displays
REASONS (cancellation reasons) instead of the list of statuses from which a
dispatch can be cancelled; update the JSX in the render (the paragraph near
currentStatus) to use the correct cancellable-status list (e.g.,
CANCELLABLE_STATUSES or a new constant like
['pending','accepted','acknowledged','en_route','on_scene']) instead of REASONS,
and join them the same way ({CANCELLABLE_STATUSES.slice(0,-1).join(', ')} or
{CANCELLABLE_STATUSES[CANCELLABLE_STATUSES.length-1]}) so the message reads
“…only X, Y or Z from pending/accepted/acknowledged/en_route/on_scene.” Ensure
the constant name you choose is added/ exported where appropriate and used in
the CancelDispatchModal component.
- Around line 61-74: The radio labels currently render raw enum values from
REASONS (e.g., responder_unavailable); create a mapping from CancelReason to a
human-friendly string (e.g., REASON_LABELS: Record<CancelReason,string>) and use
that mapping when rendering the label text instead of the raw value in the JSX
where REASONS.map is used; keep the existing input props (name, value, checked,
onChange calling setReason) and reference the mapped string (REASON_LABELS[r])
for display so accessibility and state handling (reason, setReason) remain
unchanged.

In `@apps/admin-desktop/src/pages/CloseReportModal.tsx`:
- Around line 16-29: The confirm() handler generates a new idempotencyKey on
every submit and only clears submitting on error; change to use a stable
per-modal-session idempotency key (e.g., create an idempotencyKey via useRef or
useState when the modal opens and pass that into callables.closeReport as
idempotencyKey) so retries remain idempotent, and move setSubmitting(false) into
a finally block (and reset/rotate the idempotency key when the modal is closed
or reopened) to guarantee UI state is restored regardless of success or failure.

In `@apps/responder-app/public/firebase-messaging-sw.js`:
- Around line 34-36: The empty catch in
apps/responder-app/public/firebase-messaging-sw.js is swallowing initialization
errors; change the anonymous catch to catch (e) and log the error (e.g.,
console.error('Firebase SW initialization failed:', e)) so configuration/init
failures are visible when failing in the service worker; keep the rest of the
block unchanged (do not rethrow unless you want the SW to fail hard).

In `@apps/responder-app/src/App.tsx`:
- Around line 13-21: The useEffect should defensively check for service worker
support and stop swallowing registration failures: only attempt
navigator.serviceWorker.register('/firebase-messaging-sw.js') if typeof
navigator !== 'undefined' && 'serviceWorker' in navigator, and in the catch
block pass the caught error to your logging mechanism (e.g., console.error or
your app logger) with a clear message including context (e.g., "ServiceWorker
registration failed for /firebase-messaging-sw.js") instead of an empty catch;
keep the existing .then(() => register()) behavior on success so register() is
only called after successful SW registration.

In `@apps/responder-app/src/hooks/useAcceptDispatch.ts`:
- Line 9: The current idempotency key stored in keyRef (const keyRef =
useRef(crypto.randomUUID())) is only generated once and can be reused across
different dispatchId values; update keyRef whenever dispatchId changes by adding
an effect that sets keyRef.current = crypto.randomUUID() (or regenerate the
idempotency key via a memo tied to dispatchId) so the idempotency key used in
the send/submit logic (where keyRef is read) is unique per dispatchId change.

In `@apps/responder-app/src/hooks/useAdvanceDispatch.ts`:
- Around line 29-34: The hook useAdvanceDispatch currently only sets error and
loading (using setError and setLoading) but doesn't expose the operation result
or an explicit success flag; add a piece of state (e.g., result/data) and/or a
boolean success state, set result inside the try block from the awaited response
and set success = true there (set success = false in the catch), and include
these new values in the hook's returned object so callers can consume the
response or a clear success indicator instead of inferring from !error &&
!loading.

In `@apps/responder-app/src/hooks/useDispatch.ts`:
- Around line 38-60: The effect currently leaves a stale error when dispatchId
changes; update the useEffect tied to dispatchId so you clear error when
starting a new load: call setError(undefined) when dispatchId is falsy
(alongside setDispatch(undefined) and setLoading(false)) and also
clear/setError(undefined) right before creating the onSnapshot subscription (and
optionally setLoading(true)) so previous errors aren’t shown while the new
dispatch loads; locate the useEffect that references dispatchId, onSnapshot,
setDispatch, setLoading and setError to make these small updates.

In `@apps/responder-app/src/hooks/useRegisterFcmToken.ts`:
- Around line 27-30: Replace the nonstandard own-property check with the
conventional in-operator: in the useRegisterFcmToken hook, change the detection
that sets swContainer (currently using Object.hasOwn(navigator,
'serviceWorker')) to use "'serviceWorker' in navigator" and then assign
navigator.serviceWorker or null as before so swContainer and subsequent logic
(token/error returns) work across browsers.
- Around line 39-47: The current useRegisterFcmToken hook uses updateDoc(doc(db,
responderDocPath), ...) which will throw if the responder document does not
exist; change this to an upsert by calling setDoc on the same doc reference with
the fcmTokens and fcmTokenRegisteredAt fields and the option { merge: true } (or
detect the not-found error from updateDoc and then call setDoc) so the token
registration succeeds even when the responder document hasn't been created yet;
update references to updateDoc in useRegisterFcmToken accordingly and keep
doc(db, responderDocPath) and arrayUnion/serverTimestamp usage the same.

In `@apps/responder-app/src/pages/DispatchDetailPage.tsx`:
- Around line 93-101: The outer error wrapper in DispatchDetailPage renders a
<p> around acceptError but RaceLostBanner itself returns a <p>, causing invalid
nested paragraphs; update the conditional rendering for acceptError so the
wrapper is a non-paragraph element (e.g., a <div> or <span> with the same style)
or modify RaceLostBanner to return a fragment/inline element instead—locate the
acceptError rendering in DispatchDetailPage and the RaceLostBanner component and
change one of them so you no longer render a <p> inside another <p>.
- Around line 52-63: The hooks useAcceptDispatch and useAdvanceDispatch are
being called with dispatch?.dispatchId ?? '' which passes an empty string before
the dispatch loads; update the call sites to pass undefined when dispatchId is
not available (e.g., useAcceptDispatch(dispatch?.dispatchId) and
useAdvanceDispatch(dispatch?.dispatchId)) and modify the hook implementations to
early-return/no-op when the id is undefined (or handle undefined internally) so
they don't initialize API calls or side-effects for an empty id; ensure the hook
signatures accept id?: string and that consumers like accept, advance,
accepting, advanceLoading, acceptError and advanceError remain stable when id is
undefined.

In `@apps/responder-app/src/services/fcm-client.ts`:
- Around line 34-37: The code calls getToken(messaging, { vapidKey:
import.meta.env.VITE_FCM_VAPID_PUBLIC_KEY as string, serviceWorkerRegistration
}) without validating the VAPID key; add a defensive check that reads
import.meta.env.VITE_FCM_VAPID_PUBLIC_KEY into a local variable, verify it is a
non-empty string (throw or return a clear, early error/log if missing), then
pass that validated variable as vapidKey to getToken so failures surface with a
descriptive message; update any callers of the module to handle the new error
path if necessary (refer to getToken, messaging, serviceWorkerRegistration, and
VITE_FCM_VAPID_PUBLIC_KEY).

In `@apps/responder-app/vite.config.ts`:
- Line 3: The import 'resolve' from the 'path' module is unused; remove the
unused import symbol (the "resolve" import) from the top-level import statement
in vite.config.ts so the file no longer imports resolve, and then run the
linter/build to confirm no unused-import warnings remain.

In `@docs/phase-3c-fix-session-summary.md`:
- Around line 52-55: Update the test-count string under the "6. **Updated
tests**" summary: replace the hard-coded "127 tests pass" text with the correct
"142 tests pass" (or make it consistent with the PR summary) so the line under
the Updated tests block reads "142 tests pass with new state machine"; ensure
the other bullets ("10 states", "21 transitions") remain unchanged.

In `@docs/phase-3c-IMPLEMENTATION-README.md`:
- Around line 40-74: Update the fenced code blocks under the "Test Accounts",
"Responder Document", and "Feature Flag" sections to include a language
specifier (use "text") and ensure there's a blank line before and after each
triple-backtick block; also mark the VAPID Secrets block with "bash" and add
surrounding blank lines so the "VAPID Secrets" section follows the same
formatting conventions.

In `@docs/progress.md`:
- Around line 242-255: The header "Playwright e2e specs (unblocked — SSL issue
resolved)" is misleading given the "Known remaining blockers" table; update the
header and nearby summary to explicitly state "Local E2E unblocked — staging E2E
still blocked (SSL/domain issues)" or similar, and add a parenthetical or brief
sentence clarifying that local tests run fine but staging requires
BASE_URL/hosting/Firebase fixes; ensure the "Known remaining blockers" table
remains and optionally add a one-line note linking it to the staging status so
readers understand the distinction between local vs staging E2E.

In `@docs/runbooks/fcm-vapid-rotation.md`:
- Around line 106-119: Remove the incorrect firebase.json example and replace it
with instructions to set the frontend public VAPID key via environment files
(e.g., .env.staging, .env.production) using the VITE_FCM_VAPID_PUBLIC_KEY
variable, and to store the private VAPID key in GCP Secret Manager and reference
it in Cloud Functions via defineSecret() (FCM_VAPID_PRIVATE_KEY); update the
text to explain that Firebase Hosting has no hosting.env config and show the
two-corner approach (frontend .env for VITE_FCM_VAPID_PUBLIC_KEY, backend secret
via defineSecret()/Secret Manager for FCM_VAPID_PRIVATE_KEY).

In `@e2e-tests/specs/admin.spec.ts`:
- Around line 48-58: The three placeholder tests in admin.spec.ts — the tests
named "verifies a pending report", "rejects a pending report with reason", and
"dispatches a verified report to a responder" — have empty bodies and should be
converted to explicit skipped/todo tests so they don't pass silently; replace
each empty test(() => { /* comment */ }) with either test.skip('verifies a
pending report', async () => { /* keep comment about required seeded report */
}) or test.todo('verifies a pending report') (and do the same for the other two
test names) so the intent is explicit in CI and the comments about required
seeded data are preserved.

In `@e2e-tests/specs/full-loop.spec.ts`:
- Around line 22-45: These tests have empty bodies and therefore pass trivially;
update each placeholder test in full-loop.spec.ts (the tests with titles
"citizen submits → admin verifies → responder accepts → progresses → resolves",
"admin rejects report after citizen submission", and "admin cancels dispatch
after responder accepts") to use test.skip(...) or test.todo(...) instead of an
empty async function so the intent is explicit in test reports; replace the
async () => { /* ... */ } bodies with either test.skip('title', async () => { /*
optional note */ }) or test.todo('title') for each of the three tests
(references: the test.describe('full responder loop') block and the three
test('...') declarations).

In `@e2e-tests/specs/race-loss.spec.ts`:
- Around line 17-50: The tests inside the test.describe('race condition
handling') block are empty and will pass silently; instead mark each incomplete
test (e.g., the tests titled "acceptDispatch is idempotent — concurrent accepts
are safe", "responder sees cancelled screen when admin cancels after accept",
"dispatch list reflects status changes via onSnapshot", and "overdue dispatch
shows deadline warning") with test.skip(...) or test.fixme(...) to explicitly
indicate they are deferred; update the calls in the same file
(race-loss.spec.ts) to replace the empty test(...) invocations with
test.skip(...) or test.fixme(...) so CI reports them as skipped/fixed rather
than passed.

In `@e2e-tests/specs/responder.spec.ts`:
- Around line 40-64: Several placeholder tests in responder.spec.ts (the tests
titled "shows empty state when no dispatches", "shows active dispatches when
available", "accepts a pending dispatch", "advances from acknowledged to
en_route", "advances from en_route to on_scene", "resolves a dispatch from
on_scene", "cancelled dispatch shows cancelled screen") contain empty bodies and
should not pass trivially — either implement real assertions/flow or mark them
skipped; to quickly address the review, replace each empty test body with a skip
annotation (use Playwright's test.skip or test.fixme) referencing the same test
title so CI records them as skipped, or alternatively implement the full test
logic: set up the required authenticated responder session or seeded dispatch
state, drive the UI actions (accept/acknowledge/en_route/on_scene/cancel), and
assert the expected UI/state transitions in the corresponding test functions
(e.g., the tests named above).

In `@e2e-tests/tsconfig.json`:
- Around line 1-11: Remove the invalid top-level "name" property from
tsconfig.json (it looks like a package.json leftover); leave the
"compilerOptions" block as-is (e.g., target, module, moduleResolution, strict,
noEmit, lib) so TypeScript reads the file normally and avoid confusion from the
unsupported "name" field.

In `@functions/src/__tests__/adversarial-audit.test.ts`:
- Around line 94-126: Rename the test title to remove the misleading "BUG `#1`"
prefix and make it describe the enforced constraint; update the spec in the
it(...) call (currently named 'BUG `#1`: Redispatch Deadlock — cannot dispatch new
responder if report is "assigned"') to something like 'rejects dispatch of a
second responder while report status is "assigned"' so it clearly documents
expected behavior when calling dispatchResponderCore with reportId
'report-deadlock' and responderUid 'responder-b' while status is 'assigned'.

In `@functions/src/__tests__/callables/advance-dispatch.test.ts`:
- Line 1: The eslint-disable header at the top of advance-dispatch.test.ts
includes unused rules; open the file and edit the leading comment to remove the
unused directives `@typescript-eslint/no-unsafe-call` and
`@typescript-eslint/no-unsafe-return` so the remaining disable list only contains
rules actually violated (e.g., `@typescript-eslint/no-explicit-any`,
no-unsafe-member-access, no-unsafe-argument, no-unsafe-return if applicable),
leaving the file-level directive accurate; save and run lint to confirm no
warnings remain.

In `@functions/src/__tests__/callables/cancel-dispatch.test.ts`:
- Around line 31-34: The afterAll block contains an unnecessary truthiness guard
around testEnv which triggers the no-unnecessary-condition lint rule; remove the
redundant if statement and call testEnv.cleanup() directly inside afterAll
(i.e., replace the guarded call with a direct await testEnv.cleanup()), keeping
the same async teardown behavior for the testEnv resource referenced in this
file.

In `@functions/src/__tests__/callables/close-report.test.ts`:
- Around line 141-150: The query orders events by 'at' descending so the most
recent event is first, but the test takes the last element; update the test to
assert against the first/most-recent event instead of eventData[eventData.length
- 1] — e.g., read events.docs[0] or eventData[0] (replace the variable last) and
assert it matches { from: 'resolved', to: 'closed' } in the close-report.test.ts
test where events, eventData, and last are used.
- Around line 26-30: Remove the redundant truthy check in the afterAll teardown:
since testEnv is always assigned in beforeAll, replace the conditional block in
the afterAll handler so it directly calls await testEnv.cleanup() (referencing
the testEnv variable and the existing afterAll function in this test file) to
satisfy the linter.
- Line 4: Remove the unused FieldValue import from the import statement that
currently reads "import { Timestamp, FieldValue } from
'firebase-admin/firestore'"; keep only the used symbol(s) (e.g., "Timestamp") so
the import becomes "import { Timestamp } from 'firebase-admin/firestore'".

In `@functions/src/__tests__/callables/close-report.unit.test.ts`:
- Around line 4-55: Add two edge-case tests to the close-report.unit.test suite
that exercise reportId bounds on closeReportRequestSchema: one test should
assert that parsing fails when reportId is an empty string (expect(() =>
closeReportRequestSchema.parse({ reportId: '', idempotencyKey:
'550e8400-e29b-41d4-a716-446655440000', closureSummary: 'Resolved.'
})).toThrow()), and another should assert that parsing fails when reportId
exceeds 128 characters (use 'x'.repeat(129) for reportId and expect toThrow).
Place these new it(...) cases alongside the existing tests for completeness.

In `@functions/src/__tests__/helpers/seed-factories.ts`:
- Around line 331-339: The test factory narrows the dispatch `status` union in
seedDispatch and omits valid lifecycle values like `timed_out` and `superseded`;
update the typing for `status` in the seedDispatch factory (or replace the
inline union) to include all valid dispatch statuses (e.g., add 'timed_out' and
'superseded' and any other statuses defined by the application) or, preferably,
import and use the shared Dispatch status type/enum (e.g., DispatchStatus) so
the factory always stays consistent with the production type.

In `@functions/src/__tests__/rules/responder-direct-writes.rules.test.ts`:
- Around line 84-104: In the test "it('denies acknowledged → cancelled
(responder cannot cancel)')" replace the seeded field responderUid with the
assignedTo object shape used elsewhere (e.g. set the document to { status:
'acknowledged', assignedTo: { uid: 'resp-1' }, municipalityId: 'daet' }) so the
seeding matches the rules that check assignedTo.uid; update the setDoc call that
creates 'dispatches/d-3' accordingly while keeping the rest of the test (authed
env 'resp-1' and the subsequent setDoc merge to set status: 'cancelled')
unchanged.
- Around line 62-82: The test seeds the dispatch with responderUid which doesn't
match the rules' check resource.data.assignedTo.uid == uid(), so update the
seeded document in the test (the setDoc call that writes to 'dispatches/d-2' in
responder-direct-writes.rules.test.ts) to include the correct structure:
assignedTo: { uid: 'resp-1' } (and keep other fields like status and
municipalityId), so the later authed setDoc(doc(authedDb, 'dispatches/d-2'), {
status: 'resolved' }, { merge: true }) exercise fails for the intended
transition logic rather than a missing field; no change to
unauthenticatedContext usage is needed.

In `@functions/src/__tests__/services/fcm-send.test.ts`:
- Around line 75-84: The test intentionally casts the Firebase SDK mock return
value to any, but ESLint flags the `as any` on the FieldValue.arrayRemove mock;
add a linter disable comment immediately above the mockReturnValue call to
silence this (use `// eslint-disable-next-line
`@typescript-eslint/no-explicit-any``) so the line `const arrayRemoveSpy =
vi.spyOn(FieldValue, 'arrayRemove').mockReturnValue('array_remove_mock' as any)`
in the sendFcmToResponder test is allowed while leaving the rest of the test
(e.g., checks against mockUpdate and arrayRemoveSpy) unchanged.

In `@functions/src/callables/accept-dispatch.ts`:
- Around line 62-64: The code dereferences d.assignedTo.uid without validating
that assignedTo and its uid exist; update the check in the accept-dispatch flow
(where snap, d, deps.actor.uid, BantayogError and BantayogErrorCode are used) to
first guard that d.assignedTo is an object and d.assignedTo.uid is a non-empty
string, and if those are missing throw a controlled BantayogError (e.g.
BantayogErrorCode.CONFLICT with a message like "Dispatch unassigned or
malformed"); only compare d.assignedTo.uid to deps.actor.uid after those
validations, and keep the existing FORBIDDEN throw when the uid exists but does
not match.
- Around line 33-54: The rate-limit check is run before idempotency, causing
retries with the same idempotencyKey to be rate-limited instead of returning the
cached result; change the flow to call withIdempotency(...) first (the call that
uses key `acceptDispatch:${deps.actor.uid}:${deps.idempotencyKey}` and payload
`idempotentPayload`) and only invoke checkRateLimit(...) if the idempotency call
returns fromCache === false (i.e., first execution), using the same rate-limit
key `accept::${deps.actor.uid}` and parameters; ensure you still throw
BantayogError(BantayogErrorCode.RATE_LIMITED, ...) with retryAfterSeconds from
the rl result when the fresh path is rate-limited.

In `@functions/src/callables/advance-dispatch.ts`:
- Around line 106-107: The callable advanceDispatch is missing a region and will
default to us-central1; update the onCall() invocation for advanceDispatch to
include the region option matching the client (set region: 'asia-southeast1') in
the options object passed to onCall({ ... }), so the exported advanceDispatch
uses the asia-southeast1 region and becomes reachable by Phase 3c responder
clients.
- Around line 73-85: The patch object currently sets lastStatusAt to a Firestore
Timestamp (now) which must be an epoch ms number; update the creation of patch
(inside the advance-dispatch handler where patch, now, and to are defined) so
lastStatusAt is assigned now.toMillis() (matching
acknowledgedAt/enRouteAt/onSceneAt/resolvedAt) to keep the field type consistent
with client expectations and the DispatchDoc shape.

In `@functions/src/callables/close-report.ts`:
- Around line 76-78: The municipality ownership check currently compares
reportData.municipalityId !== deps.actor.claims.municipalityId unconditionally,
which fails for provincial_superadmin (their claims.municipalityId is
undefined); update the check in close-report.ts (the block that throws
BantayogError with FORBIDDEN) so it only runs when deps.actor.claims.role ===
'municipal_admin' (i.e., scope the municipalityId comparison to
municipal_admins); apply the same pattern to the identical checks in
verify-report.ts and reject-report.ts so provincial_superadmin calls bypass the
municipalityId comparison while municipal_admins are still validated.

In `@functions/src/callables/https-error.ts`:
- Around line 42-46: The code casts claims.role to a string and throws an
unclear HttpsError when it's missing; update the validation in the callable
handler to explicitly check that request.auth?.token?.role exists and is a
non-empty string before casting, and throw a distinct HttpsError (e.g.,
'unauthenticated' or 'invalid-argument') with a clear message like "missing role
claim" when absent; only then compare the validated role variable against
allowedRoles and throw the existing permission-denied HttpsError when the role
is present but not allowed.

In `@functions/src/services/fcm-send.ts`:
- Line 59: The empty catch blocks in fcm-send.ts (around the catch after the try
that sends the FCM and the second catch) are swallowing errors; update both
catches to accept the error (e) and log it for observability using the existing
logging facility (e.g., console.error or the module's logger) including context
like function name (e.g., sendFcm / sendPushNotification) and relevant
payload/recipient identifiers, then continue to return/resolve as before so the
function still does not throw. Ensure you reference the exact catch locations
surrounding the FCM send logic and add concise log messages that include the
error object for debugging.
- Around line 51-73: Extract the construction of the FCM message into a single
reusable object before the try/catch so you don't duplicate building it in both
the initial attempt and the retry: create the msg object (using tokens,
notification: { title, body } and optional data) once, then call getMessaging()
and await messaging.sendEachForMulticast(msg) inside the try and reuse the same
msg in the retry catch that currently repeats the construction; preserve
existing behavior of pushing 'fcm_network_error' into warnings and returning {
warnings } on second failure, and keep result assignment to the same variable.

In `@functions/src/triggers/dispatch-mirror-to-report.ts`:
- Around line 105-115: The guard allowing mirroring should require an exact
match of currentDispatchId to dispatchId; change the condition in the
dispatch-mirror logic from the weak check that only skips when currentDispatchId
is truthy and different to a strict inequality so that any mismatch or absence
blocks mirroring. Update the if condition that currently reads around
currentDispatchId and dispatchId to use if (currentDispatchId !== dispatchId) so
only the active dispatch (exact match) can proceed with mirror logic; leave the
existing logger.info(...) and return behavior intact.

In `@functions/src/triggers/dispatch-timeout-sweep.ts`:
- Around line 19-51: The current sweep builds a single Firestore write batch
(db.batch()) and may exceed Firestore's 500-op limit (each timeout uses 2 ops)
and lacks error handling on batch.commit(); change the logic in
dispatch-timeout-sweep.ts to accumulate operations in batches of at most 500
writes (i.e., commit every 250 timeouts) by creating a new batch when the op
count nears 500, keep using the same update and batch.set patterns (referencing
batch.update, batch.set, evRef, crypto.randomUUID(), timedOutCount), and wrap
each batch.commit() in try/catch to log failures and continue processing
remaining documents so a failed commit doesn’t abort the whole sweep.

In `@functions/src/triggers/on-media-finalize.ts`:
- Around line 33-47: Check the object's size from its Cloud Storage metadata
before calling file.download() (use the file.getMetadata()/metadata.size or the
SDK equivalent) and reject if it exceeds MAX_SIZE so you never buffer the whole
object; only call file.download() and destructure downloadResult => buf when the
size check passes. When rejecting due to size, delete the file as you already do
and return a distinct status (e.g., 'rejected_size') instead of 'rejected_mime'
so callers/tests can detect size rejections. Keep the existing log entry but
include the size and input.objectName, and avoid swallowing errors—let
download/file.delete errors propagate or log them explicitly.

In `@infra/firebase/firestore.rules`:
- Around line 49-68: The validResponderTransition function currently allows the
responder transition (from == 'pending' && to == 'timed_out'); remove that
clause so responders cannot set timed_out manually—edit validResponderTransition
to delete the `(from == 'pending' && to == 'timed_out')` disjunct and leave all
other allowed transitions unchanged (this enforces that only the system timeout
sweep in dispatch-timeout-sweep.ts can set timed_out).

In `@packages/shared-validators/src/dispatches.ts`:
- Around line 69-76: The advanceDispatchRequestSchema currently accepts any
dispatchStatusSchema for the "to" field; narrow this at the validator boundary
by defining a new schema (e.g., advanceTargetStatusSchema) that enumerates only
the allowed responder-progression/advance-flow targets and replace the "to:
dispatchStatusSchema" entry with "to: advanceTargetStatusSchema" inside
advanceDispatchRequestSchema; ensure the new schema is exported/used
consistently and that invalid statuses are rejected by the validator rather than
accepted downstream.

In `@scripts/phase-3c/acceptance.ts`:
- Around line 202-206: The second call to acceptFn (creating acceptData2) is
using a new hardcoded idempotency key so it won't hit the idempotency cache;
change the acceptFn invocation to reuse the original idempotency key used in the
first acceptDispatch call (e.g., pass the same idempotencyKey variable or
constant rather than '1111...'), updating the acceptFn call site that constructs
acceptData2 so it uses that original idempotencyKey value.
- Around line 215-240: The test should drive status changes via the callable
advanceDispatch rather than directly updating Firestore; replace the adminDb
updates inside dispatchTransition with calls to the existing advanceDispatch
callable (using dispatchId, the desired nextStatus, and extraFields), await its
response, then read back with readDispatch/readReport to run the same checks;
ensure you pass the required resolutionSummary when transitioning to 'resolved'
and preserve any timing fields (acknowledgedAt, enRouteAt, onSceneAt,
resolvedAt) in extraFields so the callable sees them and idempotency/guards/auth
are exercised.
- Around line 128-143: The calls to adminAuth.setCustomUserClaims for ADMIN_UID
and RESPONDER_UID assume those Auth users already exist; create the users first
using adminAuth.createUser(...) for each UID (e.g., create user with uid:
ADMIN_UID and uid: RESPONDER_UID and minimal required fields) before calling
adminAuth.setCustomUserClaims and adminAuth.createCustomToken; if you want
idempotence on repeated runs, check for existence or catch the "user-not-found"
error and create the user there, then proceed to setCustomUserClaims and
createCustomToken.
- Around line 163-188: The script currently stays signed in as admin
(signInWithCustomToken(webAuth, adminToken)) but calls the responder-only
function acceptDispatch; before calling acceptDispatch invoke
signInWithCustomToken(webAuth, responderToken) so the caller is the responder,
then call httpsCallable(webFunctions, 'acceptDispatch') as shown; after
acceptDispatch, if subsequent functions like closeReport or cancelDispatch are
admin-only, switch back by calling signInWithCustomToken(webAuth, adminToken)
before invoking those functions. Ensure you use the existing responderToken and
adminToken variables and keep idempotencyKey usage as-is.
- Around line 147-159: The web SDK is always initialized with emulator-only
credentials causing staging to fail; update the webInitApp call so it uses
environment-specific config when EMU is false (use real staging API
key/authDomain/other fields derived from env vars or PROJECT_ID) and only fall
back to the emulator values when EMU is true; locate the initialization around
webInitApp/webGetAuth/webGetFunctions and the EMU check and modify the config
selection so staging supplies valid apiKey/authDomain/appId (e.g.,
STAGING_API_KEY, STAGING_AUTH_DOMAIN or production-like values) while leaving
connectAuthEmulator and connectFunctionsEmulator calls unchanged for EMU=true.

In `@scripts/phase-3c/PRECONDITION_VERIFICATION.md`:
- Line 14: The "Tests passing" count in the document is stale ("4. ✅ **Tests
passing** — All 127 tests pass with new state machine"); update that line to the
current test count (replace 127 with 142 to match the PR summary) and verify no
other occurrences of "127 tests" remain in PRECONDITION_VERIFICATION.md or
related docs so the test count is consistent across the repo.
- Around line 97-105: The heredoc using `firebase firestore:main` is invalid;
replace it with a short Node.js script that uses the Firebase Admin SDK:
initialize the SDK (admin.initializeApp()), get Firestore via admin.firestore(),
then call collection('system_config').doc('features').set(...) with
dispatch_mirror_enabled: true and updatedAt: admin.firestore.Timestamp.now();
run this script with node (or, for local testing, use the Firestore emulator
REST API) to perform the same operation.

🪄 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: 2d3fdb57-2371-4b4e-8b45-92ca87edab66

📥 Commits

Reviewing files that changed from the base of the PR and between aca5ceb and e786e17.

⛔ Files ignored due to path filters (1)

• pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml

📒 Files selected for processing (80)

• apps/admin-desktop/src/pages/CancelDispatchModal.tsx
• apps/admin-desktop/src/pages/CloseReportModal.tsx
• apps/admin-desktop/src/pages/ReportDetailPanel.tsx
• apps/admin-desktop/src/pages/TriageQueuePage.tsx
• apps/admin-desktop/src/services/callables.ts
• apps/responder-app/package.json
• apps/responder-app/public/firebase-messaging-sw.js
• apps/responder-app/src/App.tsx
• apps/responder-app/src/hooks/useAcceptDispatch.ts
• apps/responder-app/src/hooks/useAdvanceDispatch.ts
• apps/responder-app/src/hooks/useDispatch.ts
• apps/responder-app/src/hooks/useOwnDispatches.ts
• apps/responder-app/src/hooks/useRegisterFcmToken.ts
• apps/responder-app/src/pages/CancelledScreen.tsx
• apps/responder-app/src/pages/DispatchDetailPage.tsx
• apps/responder-app/src/pages/DispatchListPage.tsx
• apps/responder-app/src/routes.tsx
• apps/responder-app/src/services/fcm-client.ts
• apps/responder-app/vite.config.ts
• docs/learnings.md
• docs/phase-3c-IMPLEMENTATION-README.md
• docs/phase-3c-fix-session-summary.md
• docs/progress.md
• docs/runbooks/fcm-vapid-rotation.md
• e2e-tests/fixtures/.gitkeep
• e2e-tests/package.json
• e2e-tests/playwright.config.ts
• e2e-tests/playwright.staging.config.ts
• e2e-tests/specs/.gitkeep
• e2e-tests/specs/admin.spec.ts
• e2e-tests/specs/citizen.spec.ts
• e2e-tests/specs/full-loop.spec.ts
• e2e-tests/specs/race-loss.spec.ts
• e2e-tests/specs/responder.spec.ts
• e2e-tests/tsconfig.json
• eslint.config.js
• functions/src/__tests__/adversarial-audit.test.ts
• functions/src/__tests__/callables/accept-dispatch.test.ts
• functions/src/__tests__/callables/accept-dispatch.unit.test.ts
• functions/src/__tests__/callables/advance-dispatch.test.ts
• functions/src/__tests__/callables/cancel-dispatch.test.ts
• functions/src/__tests__/callables/close-report.test.ts
• functions/src/__tests__/callables/close-report.unit.test.ts
• functions/src/__tests__/helpers/seed-factories.ts
• functions/src/__tests__/rules/dispatch-mirror.rules.test.ts
• functions/src/__tests__/rules/responder-direct-writes.rules.test.ts
• functions/src/__tests__/services/fcm-send.test.ts
• functions/src/__tests__/triggers/dispatch-mirror-to-report.test.ts
• functions/src/__tests__/triggers/dispatch-mirror-to-report.unit.test.ts
• functions/src/callables/__tests__/close-report.unit.test.ts
• functions/src/callables/accept-dispatch.ts
• functions/src/callables/advance-dispatch.ts
• functions/src/callables/cancel-dispatch.ts
• functions/src/callables/close-report.ts
• functions/src/callables/dispatch-responder.ts
• functions/src/callables/https-error.ts
• functions/src/callables/verify-report.ts
• functions/src/index.ts
• functions/src/services/fcm-send.ts
• functions/src/triggers/dispatch-mirror-to-report.ts
• functions/src/triggers/dispatch-timeout-sweep.ts
• functions/src/triggers/on-media-finalize.ts
• infra/firebase/firestore.rules
• infra/firebase/firestore.rules.template
• packages/shared-types/src/enums.ts
• packages/shared-types/src/states.ts
• packages/shared-validators/src/dispatches.test.ts
• packages/shared-validators/src/dispatches.ts
• packages/shared-validators/src/errors-and-logging.test.ts
• packages/shared-validators/src/index.ts
• packages/shared-validators/src/state-machines.test.ts
• packages/shared-validators/src/state-machines/__tests__/dispatch-to-report.test.ts
• packages/shared-validators/src/state-machines/dispatch-states.ts
• packages/shared-validators/src/state-machines/dispatch-to-report.ts
• packages/shared-validators/src/state-machines/report-states.ts
• pnpm-workspace.yaml
• scripts/build-rules.ts
• scripts/phase-3c/PRECONDITION_VERIFICATION.md
• scripts/phase-3c/acceptance.ts
• scripts/phase-3c/verify-preconditions.ts

[coderabbitai]

🧹 Nitpick | 🔵 Trivial

Consider humanizing reason labels for better UX.

The radio button labels display raw enum values (e.g., responder_unavailable), which aren't user-friendly. Consider adding a display label:

const REASON_LABELS: Record<CancelReason, string> = {
  responder_unavailable: 'Responder unavailable',
  duplicate_report: 'Duplicate report',
  admin_error: 'Administrative error',
  citizen_withdrew: 'Citizen withdrew report',
}

// In JSX:
<label key={r}>
  <input ... />
  {REASON_LABELS[r]}
</label>

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@apps/admin-desktop/src/pages/CancelDispatchModal.tsx` around lines 61 - 74,
The radio labels currently render raw enum values from REASONS (e.g.,
responder_unavailable); create a mapping from CancelReason to a human-friendly
string (e.g., REASON_LABELS: Record<CancelReason,string>) and use that mapping
when rendering the label text instead of the raw value in the JSX where
REASONS.map is used; keep the existing input props (name, value, checked,
onChange calling setReason) and reference the mapped string (REASON_LABELS[r])
for display so accessibility and state handling (reason, setReason) remain
unchanged.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Confusing error message: shows cancellation reasons instead of allowed statuses.

The explanatory message incorrectly references REASONS (cancellation reasons like responder_unavailable) when it should reference the cancellable statuses:

// Current (confusing):
{REASONS.slice(0, -1).join(', ')} or {REASONS[REASONS.length - 1]} from pending/...

// Shows: "responder_unavailable, duplicate_report, admin_error or citizen_withdrew from pending/..."

🐛 Proposed fix

       ) : (
         <p>
-          Dispatch in <strong>{currentStatus}</strong> cannot be cancelled (only{' '}
-          {REASONS.slice(0, -1).join(', ')} or {REASONS[REASONS.length - 1]} from{' '}
-          pending/accepted/acknowledged/en_route/on_scene).
+          Dispatch in <strong>{currentStatus}</strong> status cannot be cancelled.
+          Cancellation is only available for dispatches in pending, accepted,
+          acknowledged, en_route, or on_scene status.
         </p>
       )}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@apps/admin-desktop/src/pages/CancelDispatchModal.tsx` around lines 80 - 86,
The message in CancelDispatchModal.tsx incorrectly displays REASONS
(cancellation reasons) instead of the list of statuses from which a dispatch can
be cancelled; update the JSX in the render (the paragraph near currentStatus) to
use the correct cancellable-status list (e.g., CANCELLABLE_STATUSES or a new
constant like ['pending','accepted','acknowledged','en_route','on_scene'])
instead of REASONS, and join them the same way
({CANCELLABLE_STATUSES.slice(0,-1).join(', ')} or
{CANCELLABLE_STATUSES[CANCELLABLE_STATUSES.length-1]}) so the message reads
“…only X, Y or Z from pending/accepted/acknowledged/en_route/on_scene.” Ensure
the constant name you choose is added/ exported where appropriate and used in
the CancelDispatchModal component.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Use a stable idempotency key per modal session and reset state in finally.

Generating a new key on every submit (Line 22) makes retries non-idempotent from the client perspective. Also, setSubmitting(false) should be in finally for consistent UI recovery.

Suggested fix

-import { useState } from 'react'
+import { useRef, useState } from 'react'
@@
   const [summary, setSummary] = useState('')
   const [submitting, setSubmitting] = useState(false)
+  const idempotencyKeyRef = useRef(crypto.randomUUID())
@@
       await callables.closeReport({
         reportId,
-        idempotencyKey: crypto.randomUUID(),
+        idempotencyKey: idempotencyKeyRef.current,
         ...(trimmed ? { closureSummary: trimmed } : {}),
       })
       onClose()
     } catch (err: unknown) {
       onError(err instanceof Error ? err.message : 'Close failed')
+    } finally {
       setSubmitting(false)
     }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@apps/admin-desktop/src/pages/CloseReportModal.tsx` around lines 16 - 29, The
confirm() handler generates a new idempotencyKey on every submit and only clears
submitting on error; change to use a stable per-modal-session idempotency key
(e.g., create an idempotencyKey via useRef or useState when the modal opens and
pass that into callables.closeReport as idempotencyKey) so retries remain
idempotent, and move setSubmitting(false) into a finally block (and reset/rotate
the idempotency key when the modal is closed or reopened) to guarantee UI state
is restored regardless of success or failure.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Consider batch size limits and error handling for robustness.

Two observations for resilience:

1. Batch size limit: Firestore batches support max 500 operations. Each timeout uses 2 operations (update + event), so the sweep can handle at most 250 timeouts per run. While pending dispatches are typically few, a backlog (e.g., after an outage) could exceed this.

2. Missing error handling: If batch.commit() fails, the sweep exits silently without logging the failure.

🛡️ Suggested improvement

+    const BATCH_LIMIT = 250 // 500 ops / 2 ops per dispatch
+
     for (const doc of snap.docs) {
+      if (timedOutCount >= BATCH_LIMIT) {
+        log({ severity: 'WARN', code: 'DISPATCH_TIMEOUT_SWEEP', message: 'Batch limit reached', data: { limit: BATCH_LIMIT } })
+        break
+      }
       // ... existing logic
     }

     if (timedOutCount > 0) {
-      await batch.commit()
+      try {
+        await batch.commit()
+      } catch (err) {
+        log({ severity: 'ERROR', code: 'DISPATCH_TIMEOUT_SWEEP', message: 'Batch commit failed', data: { error: String(err) } })
+        throw err
+      }
       log({ ... })
     }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@functions/src/triggers/dispatch-timeout-sweep.ts` around lines 19 - 51, The
current sweep builds a single Firestore write batch (db.batch()) and may exceed
Firestore's 500-op limit (each timeout uses 2 ops) and lacks error handling on
batch.commit(); change the logic in dispatch-timeout-sweep.ts to accumulate
operations in batches of at most 500 writes (i.e., commit every 250 timeouts) by
creating a new batch when the op count nears 500, keep using the same update and
batch.set patterns (referencing batch.update, batch.set, evRef,
crypto.randomUUID(), timedOutCount), and wrap each batch.commit() in try/catch
to log failures and continue processing remaining documents so a failed commit
doesn’t abort the whole sweep.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Enforce the size limit before file.download(), and surface it as its own result.

This branch still buffers the entire object before Line 40, so the new 50 MB cap will not prevent OOM on oversized uploads. It also returns rejected_mime, which hides the new rejection mode from callers/tests. Check the Cloud Storage object size first, then download only if it passes, and return a distinct status for size failures. As per coding guidelines "Use defensive programming: validate external input at boundaries and never swallow errors with empty catch blocks".

💡 Safer direction

 export interface OnMediaFinalizeInput {
   bucket: { file(name: string): FileHandle }
   objectName: string
+  objectSizeBytes: number
   now?: () => number
   writePending: (doc: {
     uploadId: string
     storagePath: string
     strippedAt: number
     mimeType: string
   }) => Promise<void>
 }
 
 export interface OnMediaFinalizeResult {
-  status: 'accepted' | 'rejected_mime'
+  status: 'accepted' | 'rejected_mime' | 'rejected_size'
 }
 
 export async function onMediaFinalizeCore(
   input: OnMediaFinalizeInput,
 ): Promise<OnMediaFinalizeResult> {
   if (!input.objectName.startsWith('pending/')) {
     return { status: 'accepted' }
   }
   const file = input.bucket.file(input.objectName)
+  const MAX_SIZE = 50 * 1024 * 1024
+  if (input.objectSizeBytes > MAX_SIZE) {
+    await file.delete()
+    log({
+      severity: 'WARNING',
+      code: 'MEDIA_REJECTED_SIZE',
+      message: `Deleted oversized upload (${String(input.objectSizeBytes)} bytes): ${input.objectName}`,
+    })
+    return { status: 'rejected_size' }
+  }
   const downloadResult = await file.download()
   if (!downloadResult) throw new Error('download returned undefined')
   const buf = downloadResult[0]
-
-  const MAX_SIZE = 50 * 1024 * 1024
-  if (buf.length > MAX_SIZE) {
-    await file.delete()
-    log({
-      severity: 'WARNING',
-      code: 'MEDIA_REJECTED_SIZE',
-      message: `Deleted oversized upload (${String(buf.length)} bytes): ${input.objectName}`,
-    })
-    return { status: 'rejected_mime' }
-  }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@functions/src/triggers/on-media-finalize.ts` around lines 33 - 47, Check the
object's size from its Cloud Storage metadata before calling file.download()
(use the file.getMetadata()/metadata.size or the SDK equivalent) and reject if
it exceeds MAX_SIZE so you never buffer the whole object; only call
file.download() and destructure downloadResult => buf when the size check
passes. When rejecting due to size, delete the file as you already do and return
a distinct status (e.g., 'rejected_size') instead of 'rejected_mime' so
callers/tests can detect size rejections. Keep the existing log entry but
include the size and input.objectName, and avoid swallowing errors—let
download/file.delete errors propagate or log them explicitly.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Inconsistent test count.

The documentation states "127 tests pass" but the PR summary mentions "142 tests passing." Update to reflect the current count to avoid confusion.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@scripts/phase-3c/PRECONDITION_VERIFICATION.md` at line 14, The "Tests
passing" count in the document is stale ("4. ✅ **Tests passing** — All 127 tests
pass with new state machine"); update that line to the current test count
(replace 127 with 142 to match the PR summary) and verify no other occurrences
of "127 tests" remain in PRECONDITION_VERIFICATION.md or related docs so the
test count is consistent across the repo.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🌐 Web query:

firebase cli firestore set document command syntax

💡 Result:

There is no "firebase firestore set document" command in the Firebase CLI for setting or creating documents in production Cloud Firestore. The Firebase CLI provides firestore:delete for deleting documents/collections, but no equivalent set/create command. Official documentation confirms only management commands like firestore:locations, firestore:databases:create/update/delete, firestore:indexes, and firestore:delete (sources 3,4,5,27,28,29,30,31). For production data import, use gcloud firestore export/import via Cloud Storage (requires Blaze plan), e.g.: gcloud firestore import gs://[BUCKET]/[EXPORT_PREFIX]/ (sources 10,11,13). The old firebase data:set command was for Realtime Database, not Firestore (source 19). To populate data, use Firebase Admin SDKs in a script (source 2). For local development, use Firebase Local Emulator Suite: start with "firebase emulators:start --only firestore", connect app via connectFirestoreEmulator, or use REST API e.g. curl -X PATCH "http://localhost:8080/v1/projects/[PROJECT]/databases/(default)/documents/[PATH]?updateMask=" -d '{"fields": {...}}' (sources 22,23,24,25). No direct CLI set command even for emulator.

Citations:

• 1: https://stackoverflow.com/questions/65574000/is-it-possible-to-create-firestore-collections-documents-through-firebase-cli
• 2: https://firebase.google.com/docs/cli?hl=en
• 3: https://firebase.google.com/docs/cli
• 4: https://firebase-dot-devsite-v2-prod.appspot.com/docs/cli
• 5: https://docs.cloud.google.com/sdk/gcloud/reference/firestore/import
• 6: https://firebase.google.com/docs/firestore/manage-data/export-import
• 7: https://firebase.blog/posts/2015/11/the-firebase-cli-now-with-database_82
• 8: https://firebase.google.com./docs/firestore/manage-data/delete-data
• 9: https://cloud.google.com/firestore/docs/manage-data/delete-data
• 10: https://firebase.google.com/docs/firestore/manage-data/delete-data?authuser=0
• 11: https://firebase.google.com./docs/firestore/enterprise/delete-data-native
• 12: https://firebase-dot-devsite-v2-prod.appspot.com/docs/firestore/manage-data/delete-data

---

Replace this Firebase command with a valid approach.

The firebase firestore:main command with heredoc syntax is not a valid Firebase CLI command. Firebase CLI provides only firestore:delete, firestore:indexes, and database management commands—no document set/create commands exist. Replace with a Node.js script using Firebase Admin SDK:

const admin = require('firebase-admin');
admin.initializeApp();
admin.firestore().collection('system_config').doc('features').set({
  dispatch_mirror_enabled: true,
  updatedAt: admin.firestore.Timestamp.now()
});

Or use the Firestore emulator REST API for local testing.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@scripts/phase-3c/PRECONDITION_VERIFICATION.md` around lines 97 - 105, The
heredoc using `firebase firestore:main` is invalid; replace it with a short
Node.js script that uses the Firebase Admin SDK: initialize the SDK
(admin.initializeApp()), get Firestore via admin.firestore(), then call
collection('system_config').doc('features').set(...) with
dispatch_mirror_enabled: true and updatedAt: admin.firestore.Timestamp.now();
run this script with node (or, for local testing, use the Firestore emulator
REST API) to perform the same operation.

[coderabbitai]

Review continued from previous batch...

[coderabbitai]

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

# First, let's examine the full context of the advance-dispatch.ts file around the reported lines
cat -n functions/src/callables/advance-dispatch.ts | head -100

Repository: Exc1D/bantayog-alert

Length of output: 4030

---

🏁 Script executed:

# Search for other timestamp field usage in dispatch-related files
rg "(acceptedAt|lastStatusAt|acknowledgedAt|enRouteAt|onSceneAt|resolvedAt)" -A 2 -B 2 --type ts

Repository: Exc1D/bantayog-alert

Length of output: 23633

---

🏁 Script executed:

# Look for type definitions or schema for dispatch documents
fd -e ts -e tsx | xargs grep -l "acceptedAt\|lastStatusAt" | head -10

Repository: Exc1D/bantayog-alert

Length of output: 615

---

🏁 Script executed:

# Find and examine the complete DispatchDoc schema definition
grep -n "DispatchDoc\|interface.*Dispatch" packages/shared-validators/src/dispatches.ts -A 50 | head -100

Repository: Exc1D/bantayog-alert

Length of output: 126

---

🏁 Script executed:

# Also check if there's a type definition elsewhere
rg "type DispatchDoc|interface DispatchDoc" --type ts -A 30

Repository: Exc1D/bantayog-alert

Length of output: 4904

---

🏁 Script executed:

# Check what the accept-dispatch callable does with acceptedAt
cat -n functions/src/callables/accept-dispatch.ts

Repository: Exc1D/bantayog-alert

Length of output: 5327

---

🏁 Script executed:

# Get the complete dispatchDocSchema definition
cat -n packages/shared-validators/src/dispatches.ts | head -150

Repository: Exc1D/bantayog-alert

Length of output: 3254

---

🏁 Script executed:

# Also check what happens when Timestamp objects are written to Firestore
grep -n "statusUpdatedAt\|lastStatusAt\|acceptedAt" packages/shared-validators/src/dispatches.ts -B 2 -A 2

Repository: Exc1D/bantayog-alert

Length of output: 285

---

🏁 Script executed:

# Check how Firestore Timestamp objects are serialized when read by clients
# and verify if the client code really expects numbers
rg "lastStatusAt|acceptedAt" apps/responder-app -A 2 -B 2

Repository: Exc1D/bantayog-alert

Length of output: 417

---

🏁 Script executed:

# Verify how the data is actually read and used in the responder app
grep -n "useDispatch\|DispatchDoc" apps/responder-app/src/hooks/useDispatch.ts -A 20 | tail -40

Repository: Exc1D/bantayog-alert

Length of output: 1238

---

🏁 Script executed:

# Check if lastStatusAt or acceptedAt are used elsewhere in the schema or if they're dynamically added
rg "lastStatusAt|acceptedAt" packages/shared-validators/src/ -B 3 -A 3

Repository: Exc1D/bantayog-alert

Length of output: 1223

---

🏁 Script executed:

# Verify what happens when a Firestore Timestamp is read from Firestore in the client
# and check how the actual data type mismatch would manifest
cat -n apps/responder-app/src/hooks/useDispatch.ts | head -70

Repository: Exc1D/bantayog-alert

Length of output: 2233

---

🏁 Script executed:

# Search for how lastStatusAt is written elsewhere to see the pattern
rg "lastStatusAt\s*:" -B 2 -A 2 --type ts functions/src/callables functions/src/triggers

Repository: Exc1D/bantayog-alert

Length of output: 5394

---

🏁 Script executed:

# Check if there's any data transformation happening when reading from Firestore
rg "toMillis\(\)|getTime\(\)" apps/responder-app --type ts -B 2 -A 2 | head -50

Repository: Exc1D/bantayog-alert

Length of output: 46

---

🏁 Script executed:

# Get complete view of dispatchDocSchema to confirm lastStatusAt and acceptedAt are missing
cat -n packages/shared-validators/src/dispatches.ts | grep -A 30 "export const dispatchDocSchema"

Repository: Exc1D/bantayog-alert

Length of output: 1488

---

🏁 Script executed:

# Check how Firestore SDK handles Timestamp objects when reading data
# and verify if this causes actual runtime issues
rg "Timestamp" apps/responder-app/src --type ts -B 2 -A 2 | head -40

Repository: Exc1D/bantayog-alert

Length of output: 2577

---

Convert lastStatusAt to epoch milliseconds for consistency with client expectations and the pattern used for other lifecycle timestamps.

Lines 76: lastStatusAt: now should be lastStatusAt: now.toMillis(). The client expects this field as a number (epoch milliseconds), matching the schema pattern for acknowledgedAt, enRouteAt, onSceneAt, and resolvedAt. Writing a Firestore Timestamp object instead creates a type mismatch that circumvents static type safety (the client code casts with as DispatchDoc).

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@functions/src/callables/advance-dispatch.ts` around lines 73 - 85, The patch
object currently sets lastStatusAt to a Firestore Timestamp (now) which must be
an epoch ms number; update the creation of patch (inside the advance-dispatch
handler where patch, now, and to are defined) so lastStatusAt is assigned
now.toMillis() (matching acknowledgedAt/enRouteAt/onSceneAt/resolvedAt) to keep
the field type consistent with client expectations and the DispatchDoc shape.

[coderabbitai]

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

# First, let's read the acceptance.ts file to understand the full context
head -n 200 scripts/phase-3c/acceptance.ts | tail -n 100

Repository: Exc1D/bantayog-alert

Length of output: 3694

---

🏁 Script executed:

# Also search for how EMU is defined and used in this file
rg -n "EMU" scripts/phase-3c/acceptance.ts -B 2 -A 2

Repository: Exc1D/bantayog-alert

Length of output: 744

---

🏁 Script executed:

# Check if there's environment-specific config handling elsewhere
rg -n "env.*staging\|staging.*env\|--env" scripts/phase-3c/acceptance.ts

Repository: Exc1D/bantayog-alert

Length of output: 46

---

🏁 Script executed:

# Read the beginning of acceptance.ts to understand its purpose and any documentation
head -n 50 scripts/phase-3c/acceptance.ts

Repository: Exc1D/bantayog-alert

Length of output: 1823

---

🏁 Script executed:

# Search for where this script is invoked or documented
rg -n "acceptance.ts\|phase-3c" --type ts --type json --type md

Repository: Exc1D/bantayog-alert

Length of output: 46

---

The documented staging path cannot authenticate with hardcoded emulator Web SDK settings.

The script header explicitly documents staging support (--env=staging), but lines 147-159 initialize the Web SDK with apiKey: 'emulator-api-key' and authDomain: 'localhost' regardless of environment. When --env=staging is passed, EMU becomes false and emulator connections are skipped, but the Web SDK credentials remain invalid for staging. Firebase authentication and Cloud Functions calls will fail because the credentials do not match the staging project.

The Web SDK initialization must use environment-specific config (e.g., actual staging apiKey and authDomain) when --env=staging is active.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@scripts/phase-3c/acceptance.ts` around lines 147 - 159, The web SDK is always
initialized with emulator-only credentials causing staging to fail; update the
webInitApp call so it uses environment-specific config when EMU is false (use
real staging API key/authDomain/other fields derived from env vars or
PROJECT_ID) and only fall back to the emulator values when EMU is true; locate
the initialization around webInitApp/webGetAuth/webGetFunctions and the EMU
check and modify the config selection so staging supplies valid
apiKey/authDomain/appId (e.g., STAGING_API_KEY, STAGING_AUTH_DOMAIN or
production-like values) while leaving connectAuthEmulator and
connectFunctionsEmulator calls unchanged for EMU=true.

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@apps/responder-app/vite.config.ts`:
- Around line 8-12: The define object in vite.config.ts is empty so
import.meta.env values won't be injected and files in /public
(firebase-messaging-sw.js) are copied raw; fix by either populating the define
block in vite.config.ts with mappings for your VITE_FIREBASE_* env vars (e.g.,
map VITE_FIREBASE_API_KEY, VITE_FIREBASE_PROJECT_ID, etc. into define) so the SW
can read them at runtime, or move firebase-messaging-sw.js out of /public into
/src (so Vite processes it and import.meta.env is available) and update any
registration paths accordingly; ensure you update references to
import.meta.env.VITE_FIREBASE_* in firebase-messaging-sw.js to match the chosen
approach.

🪄 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: 2ee25907-68f1-4bd2-9b8d-42df2e20d1c7

📥 Commits

Reviewing files that changed from the base of the PR and between e786e17 and abb0549.

📒 Files selected for processing (1)

• apps/responder-app/vite.config.ts

[coderabbitai]

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "Inspect Vite config changed block:"
sed -n '1,140p' apps/responder-app/vite.config.ts

echo
echo "Inspect service worker env usage:"
sed -n '1,160p' apps/responder-app/public/firebase-messaging-sw.js

echo
echo "Locate import.meta.env usage in service worker and define usage in config:"
rg -n -C2 'import\.meta\.env|VITE_FIREBASE_|define\s*:' apps/responder-app/vite.config.ts apps/responder-app/public/firebase-messaging-sw.js

Repository: Exc1D/bantayog-alert

Length of output: 3745

---

🏁 Script executed:

# Check if there are multiple vite configs or build scripts
fd -e config -e conf | grep -E '(vite|build|webpack|tsconfig)'

Repository: Exc1D/bantayog-alert

Length of output: 46

---

🏁 Script executed:

# Check for any other service worker files or related build logic
find . -name '*service*worker*' -o -name '*sw.js' | head -20

Repository: Exc1D/bantayog-alert

Length of output: 116

---

🏁 Script executed:

# Look for package.json build scripts that might clarify SW handling
fd package.json -x cat {} \; | grep -A 10 '"scripts"'

Repository: Exc1D/bantayog-alert

Length of output: 3815

---

🏁 Script executed:

# Check if there are any environment variable setup files or .env examples
fd -e env -e env.example | head -20

Repository: Exc1D/bantayog-alert

Length of output: 46

---

🏁 Script executed:

# Check responder-app's specific vite config and build output
cat -n apps/responder-app/vite.config.ts

Repository: Exc1D/bantayog-alert

Length of output: 578

---

🏁 Script executed:

# Check if there's any additional build configuration or plugin handling for the SW
rg -n 'firebase.*sw|service.*worker|public' apps/responder-app/vite.config.ts

Repository: Exc1D/bantayog-alert

Length of output: 203

---

🏁 Script executed:

# Look for any runtime loading logic or how the SW is registered
rg -n 'registerServiceWorker|navigator.serviceWorker|import.*sw' apps/responder-app/src --type ts --type tsx -A 3

Repository: Exc1D/bantayog-alert

Length of output: 91

---

🏁 Script executed:

# Check if maybe the define block has content we're missing
cat -n apps/responder-app/vite.config.ts | sed -n '1,20p'

Repository: Exc1D/bantayog-alert

Length of output: 578

---

🏁 Script executed:

# Check if responder-app package.json has any vite-related plugins or SW handling
cat apps/responder-app/package.json

Repository: Exc1D/bantayog-alert

Length of output: 858

---

🏁 Script executed:

# Search for any vite plugins that might handle service workers
rg -n 'plugin|worker|sw' apps/responder-app/ --glob='*.json' --glob='*.ts' -A 2 -B 2

Repository: Exc1D/bantayog-alert

Length of output: 2425

---

🏁 Script executed:

# Look for any build output or dist structure hints
fd -e 'dist' -o -e 'build' --max-depth 3 apps/responder-app/

Repository: Exc1D/bantayog-alert

Length of output: 180

---

🏁 Script executed:

# Look for how the service worker is registered/loaded
rg -n 'register|/firebase-messaging-sw' apps/responder-app/src -A 3

Repository: Exc1D/bantayog-alert

Length of output: 2054

---

🏁 Script executed:

# Check the index.html to see how the app is bootstrapped and if SW is loaded
cat apps/responder-app/index.html

Repository: Exc1D/bantayog-alert

Length of output: 446

---

🏁 Script executed:

# Search for any documentation or comments about the SW setup in the codebase
rg -n 'import.meta.env|VITE_FIREBASE' apps/responder-app/ -B 2 -A 2

Repository: Exc1D/bantayog-alert

Length of output: 2821

---

🏁 Script executed:

# Verify Vite's handling by checking how main.tsx accesses env vars vs how SW does
cat -n apps/responder-app/src/main.tsx

Repository: Exc1D/bantayog-alert

Length of output: 433

---

🏁 Script executed:

# Check if there's any build-time file processing for the SW
find apps/responder-app -name '*.config.*' -o -name 'Makefile' -o -name 'build.*'

Repository: Exc1D/bantayog-alert

Length of output: 136

---

🏁 Script executed:

# Check capacitor config to see if it affects SW handling
cat apps/responder-app/capacitor.config.ts

Repository: Exc1D/bantayog-alert

Length of output: 320

---

🏁 Script executed:

# Verify if there's a tsconfig or any other build config that might pre-process the SW
cat apps/responder-app/tsconfig.json 2>/dev/null | head -30

Repository: Exc1D/bantayog-alert

Length of output: 233

---

🏁 Script executed:

# Check if there's any post-build or build-time script that might process the SW file
rg -n 'firebase-messaging-sw|public.*dist|copy.*sw' --type json --type ts --type js

Repository: Exc1D/bantayog-alert

Length of output: 524

---

🏁 Script executed:

# Verify the actual structure of what Vite would build
cat apps/responder-app/vite.config.ts

Repository: Exc1D/bantayog-alert

Length of output: 487

---

🏁 Script executed:

# Check if maybe there's environment file loading at the app level that's supposed to work
rg -n 'loadEnv|process.env|\.env' apps/responder-app/src -B 2 -A 2

Repository: Exc1D/bantayog-alert

Length of output: 987

---

🏁 Script executed:

# Final check: verify if Vite's dev server actually processes /public files
# by looking at Vite 8.0.8 documentation or seeing if there's any special handling
rg -n 'vite|webpack|build' apps/responder-app/.env* --type text 2>/dev/null || echo "No .env files found"

Repository: Exc1D/bantayog-alert

Length of output: 83

---

🏁 Script executed:

# Check the entire app structure to understand how env vars are supposed to flow
cat -n apps/responder-app/public/firebase-messaging-sw.js | head -30

Repository: Exc1D/bantayog-alert

Length of output: 1062

---

Service worker env vars won't be injected — define is empty and /public files skip Vite's build pipeline.

The define object at lines 8–11 is empty, so no Firebase config values are mapped. More critically, because the service worker is in /public, Vite copies it directly without processing, meaning import.meta.env.VITE_FIREBASE_* references in firebase-messaging-sw.js will remain undefined at runtime. The config initialization will fail silently (caught by the try-catch on line 24).

To fix this, either populate the define block with the actual env vars or move the service worker to /src so it's processed by Vite.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@apps/responder-app/vite.config.ts` around lines 8 - 12, The define object in
vite.config.ts is empty so import.meta.env values won't be injected and files in
/public (firebase-messaging-sw.js) are copied raw; fix by either populating the
define block in vite.config.ts with mappings for your VITE_FIREBASE_* env vars
(e.g., map VITE_FIREBASE_API_KEY, VITE_FIREBASE_PROJECT_ID, etc. into define) so
the SW can read them at runtime, or move firebase-messaging-sw.js out of /public
into /src (so Vite processes it and import.meta.env is available) and update any
registration paths accordingly; ensure you update references to
import.meta.env.VITE_FIREBASE_* in firebase-messaging-sw.js to match the chosen
approach.

[coderabbitai]

Actionable comments posted: 3

♻️ Duplicate comments (7)

functions/src/__tests__/callables/close-report.test.ts (2)

26-30: ⚠️ Potential issue | 🟡 Minor

Remove the redundant teardown conditional to satisfy lint.

Line 27 is flagged as an unnecessary condition (testEnv is always assigned in beforeAll), which leaves the lint check failing.

🔧 Suggested fix

 afterAll(async () => {
-  if (testEnv) {
-    await testEnv.cleanup()
-  }
+  await testEnv.cleanup()
 })

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@functions/src/__tests__/callables/close-report.test.ts` around lines 26 - 30,
The afterAll teardown currently guards testEnv with an unnecessary conditional;
remove the redundant if check and directly call await testEnv.cleanup() inside
the afterAll callback since testEnv is always assigned in beforeAll (update the
afterAll block that references testEnv and cleanup to call testEnv.cleanup()
unconditionally).

---

141-150: ⚠️ Potential issue | 🟡 Minor

Select the most recent event when ordering by at descending.

With orderBy('at', 'desc'), Line 149 currently reads the oldest record (eventData[eventData.length - 1]). Use the first element to assert the latest close transition.

🔧 Suggested fix

-    const last = eventData[eventData.length - 1]
+    const last = eventData[0]
     expect(last).toMatchObject({ from: 'resolved', to: 'closed' })

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@functions/src/__tests__/callables/close-report.test.ts` around lines 141 -
150, The test reads report_events ordered by 'at' descending but then selects
the last element (eventData[eventData.length - 1]), which returns the oldest
event; update the assertion to select the first element instead (e.g., use
eventData[0]) so the test checks the most recent transition from 'resolved' to
'closed' for the collection query stored in the events variable and mapped into
eventData (replace uses of the last variable accordingly).

scripts/phase-3c/acceptance.ts (5)

136-142: ⚠️ Potential issue | 🟠 Major

Switch to the responder identity before acceptDispatch.

The script signs in as the admin at Line 163 and never swaps identities, so Line 185 invokes a responder flow with the wrong principal. The responder token minted at Line 142 is also discarded, so this happy-path check becomes a permission test instead.

🔧 Minimal fix

-  await adminAuth.createCustomToken(RESPONDER_UID)
+  const responderToken = await adminAuth.createCustomToken(RESPONDER_UID)
   check('Responder custom token minted', true, RESPONDER_UID)
@@
+  await signInWithCustomToken(webAuth, responderToken)
   const acceptFn = httpsCallable(webFunctions, 'acceptDispatch')

Also applies to: 163-188

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@scripts/phase-3c/acceptance.ts` around lines 136 - 142, The script mints a
responder token (adminAuth.createCustomToken with RESPONDER_UID) but never
switches identities before calling acceptDispatch, so the accept flow runs as
admin; after creating the responder custom token and setting claims via
adminAuth.setCustomUserClaims(RESPONDER_UID), sign in as the responder (use the
created custom token to initialize/authenticate the client used by
acceptDispatch) so acceptDispatch runs with the responder principal; ensure any
subsequent checks that expect admin identity re-authenticate back to admin if
needed.

---

147-152: ⚠️ Potential issue | 🔴 Critical

The documented staging path still uses emulator-only Web SDK config.

When --env=staging is passed, Lines 156-159 are skipped, but the app is still initialized with apiKey: 'emulator-api-key' and authDomain: 'localhost'. signInWithCustomToken() and callable invocations will not authenticate against staging with those values.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@scripts/phase-3c/acceptance.ts` around lines 147 - 152, The app is always
initialized with emulator-only config in the webInitApp call (apiKey:
'emulator-api-key', authDomain: 'localhost'), causing staging auth to fail;
change the initialization so webInitApp uses real staging values when
--env=staging by selecting the correct config object (e.g., a STAGING config or
env-derived values) instead of the emulator constants. Locate the webInitApp
invocation and replace the hard-coded emulator apiKey/authDomain/appId with a
conditional or config lookup that returns emulator values only for emulator runs
and the actual staging API key/authDomain/appId (and PROJECT_ID) when env ===
'staging' so signInWithCustomToken() and callable functions authenticate against
staging correctly.

---

215-240: ⚠️ Potential issue | 🟠 Major

Advance the lifecycle via advanceDispatch, not direct Firestore writes.

These writes only exercise dispatchMirrorToReport. They bypass advanceDispatch auth, transition guards, idempotency, and resolved-state requirements, so this script is not actually validating the Phase 3c responder loop end to end.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@scripts/phase-3c/acceptance.ts` around lines 215 - 240, The test currently
mutates Firestore directly via dispatchTransition (which calls
adminDb.collection(...).update) and thus bypasses the real lifecycle logic;
replace those direct updates with calls to the canonical advanceDispatch
function so auth, guards, idempotency and resolved-state logic run (e.g., call
advanceDispatch(dispatchId, nextStatus, extraFields) instead of writing via
adminDb), keep the same Timestamp fields passed as extraFields, remove the
adminDb.collection(...).doc(dispatchId).update call inside dispatchTransition,
and keep assertions using readDispatch/readReport to validate end-to-end
behavior and dispatchMirrorToReport effects.

---

128-142: ⚠️ Potential issue | 🟠 Major

Create the Auth users before calling setCustomUserClaims().

setCustomUserClaims() requires an existing Auth user. On a clean emulator or staging project, Line 128 or Line 136 will fail before the acceptance flow even starts.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@scripts/phase-3c/acceptance.ts` around lines 128 - 142, The failure occurs
because adminAuth.setCustomUserClaims(...) is called for ADMIN_UID and
RESPONDER_UID before those Auth users exist; before calling
adminAuth.setCustomUserClaims in this file, ensure each user exists (e.g., call
adminAuth.getUser(ADMIN_UID) and create with adminAuth.createUser({uid:
ADMIN_UID, ...}) if not found, then call adminAuth.setCustomUserClaims for
ADMIN_UID, and do the same for RESPONDER_UID) so that setCustomUserClaims and
createCustomToken succeed; update the flow around adminAuth.setCustomUserClaims,
adminAuth.createCustomToken, ADMIN_UID and RESPONDER_UID accordingly.

---

202-206: ⚠️ Potential issue | 🟠 Major

Reuse the original idempotency key for the replay check.

acceptDispatchCore keys idempotency by acceptDispatch:${uid}:${idempotencyKey} in functions/src/callables/accept-dispatch.ts:48-65. Passing a different UUID here re-enters normal business logic instead of proving the cached path.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@scripts/phase-3c/acceptance.ts` around lines 202 - 206, The test is using a
different UUID for the second accept call so it bypasses the idempotency cache;
change the idempotencyKey passed to the replay acceptFn call (currently the
hardcoded '11111111-1111-1111-1111-111111111111' used to produce acceptData2) to
reuse the original idempotency key used on the first accept call (the same value
that acceptDispatchCore keys as acceptDispatch:${uid}:${idempotencyKey}), i.e.
reference the existing idempotencyKey variable/constant from the earlier accept
invocation instead of creating a new UUID so the cached path is exercised.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@functions/src/__tests__/callables/close-report.test.ts`:
- Around line 66-77: The test asserts a PERMISSION_DENIED error but
closeReportCore uses the core error code BantayogErrorCode.FORBIDDEN for
cross-municipality denial; update the assertion in the test (the expect of
closeReportCore) to expect BantayogErrorCode.FORBIDDEN (or the string
'FORBIDDEN') so it matches the thrown error from closeReportCore and the core
contract.

In `@scripts/phase-3c/acceptance.ts`:
- Around line 41-43: The current EMU assignment treats any --env value other
than the literal "staging" as emulator, which allows typos; change the parsing
to explicitly validate the --env argument: extract the envArg from process.argv
(the code that computes EMU), accept only a whitelist of allowed values (e.g.,
"staging" and "emulator"/"local" or whatever project supports), set EMU = true
only when the validated env equals the emulator value and set PROJECT_ID based
on the validated env (or throw/exit with a clear error when an unsupported value
is provided), so replace the current negative check with explicit validation and
a clear error path referencing the EMU constant and the code that computes
PROJECT_ID.
- Line 154: The webFunctions client is defaulting to us-central1 causing
callable failures; change the call to webGetFunctions in acceptance.ts to pin
the region used for Phase 3 by passing the deployed region (e.g.,
"asia-southeast1") so webFunctions = webGetFunctions(webApp, "asia-southeast1")
and ensure this region matches the backend service setup used in
apps/citizen-pwa/src/services/firebase.ts (or import the shared region constant
if available).

---

Duplicate comments:
In `@functions/src/__tests__/callables/close-report.test.ts`:
- Around line 26-30: The afterAll teardown currently guards testEnv with an
unnecessary conditional; remove the redundant if check and directly call await
testEnv.cleanup() inside the afterAll callback since testEnv is always assigned
in beforeAll (update the afterAll block that references testEnv and cleanup to
call testEnv.cleanup() unconditionally).
- Around line 141-150: The test reads report_events ordered by 'at' descending
but then selects the last element (eventData[eventData.length - 1]), which
returns the oldest event; update the assertion to select the first element
instead (e.g., use eventData[0]) so the test checks the most recent transition
from 'resolved' to 'closed' for the collection query stored in the events
variable and mapped into eventData (replace uses of the last variable
accordingly).

In `@scripts/phase-3c/acceptance.ts`:
- Around line 136-142: The script mints a responder token
(adminAuth.createCustomToken with RESPONDER_UID) but never switches identities
before calling acceptDispatch, so the accept flow runs as admin; after creating
the responder custom token and setting claims via
adminAuth.setCustomUserClaims(RESPONDER_UID), sign in as the responder (use the
created custom token to initialize/authenticate the client used by
acceptDispatch) so acceptDispatch runs with the responder principal; ensure any
subsequent checks that expect admin identity re-authenticate back to admin if
needed.
- Around line 147-152: The app is always initialized with emulator-only config
in the webInitApp call (apiKey: 'emulator-api-key', authDomain: 'localhost'),
causing staging auth to fail; change the initialization so webInitApp uses real
staging values when --env=staging by selecting the correct config object (e.g.,
a STAGING config or env-derived values) instead of the emulator constants.
Locate the webInitApp invocation and replace the hard-coded emulator
apiKey/authDomain/appId with a conditional or config lookup that returns
emulator values only for emulator runs and the actual staging API
key/authDomain/appId (and PROJECT_ID) when env === 'staging' so
signInWithCustomToken() and callable functions authenticate against staging
correctly.
- Around line 215-240: The test currently mutates Firestore directly via
dispatchTransition (which calls adminDb.collection(...).update) and thus
bypasses the real lifecycle logic; replace those direct updates with calls to
the canonical advanceDispatch function so auth, guards, idempotency and
resolved-state logic run (e.g., call advanceDispatch(dispatchId, nextStatus,
extraFields) instead of writing via adminDb), keep the same Timestamp fields
passed as extraFields, remove the adminDb.collection(...).doc(dispatchId).update
call inside dispatchTransition, and keep assertions using
readDispatch/readReport to validate end-to-end behavior and
dispatchMirrorToReport effects.
- Around line 128-142: The failure occurs because
adminAuth.setCustomUserClaims(...) is called for ADMIN_UID and RESPONDER_UID
before those Auth users exist; before calling adminAuth.setCustomUserClaims in
this file, ensure each user exists (e.g., call adminAuth.getUser(ADMIN_UID) and
create with adminAuth.createUser({uid: ADMIN_UID, ...}) if not found, then call
adminAuth.setCustomUserClaims for ADMIN_UID, and do the same for RESPONDER_UID)
so that setCustomUserClaims and createCustomToken succeed; update the flow
around adminAuth.setCustomUserClaims, adminAuth.createCustomToken, ADMIN_UID and
RESPONDER_UID accordingly.
- Around line 202-206: The test is using a different UUID for the second accept
call so it bypasses the idempotency cache; change the idempotencyKey passed to
the replay acceptFn call (currently the hardcoded
'11111111-1111-1111-1111-111111111111' used to produce acceptData2) to reuse the
original idempotency key used on the first accept call (the same value that
acceptDispatchCore keys as acceptDispatch:${uid}:${idempotencyKey}), i.e.
reference the existing idempotencyKey variable/constant from the earlier accept
invocation instead of creating a new UUID so the cached path is exercised.

🪄 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: d898fa06-2d31-42b7-9145-a2ef348476eb

📥 Commits

Reviewing files that changed from the base of the PR and between abb0549 and 89d83e6.

📒 Files selected for processing (2)

• functions/src/__tests__/callables/close-report.test.ts
• scripts/phase-3c/acceptance.ts

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Use the core error code (FORBIDDEN) for cross-municipality denial.

Line 76 expects PERMISSION_DENIED, but closeReportCore throws BantayogErrorCode.FORBIDDEN for municipality mismatch. This assertion will fail against the core contract.

🔧 Suggested fix

-    ).rejects.toMatchObject({ code: 'PERMISSION_DENIED' })
+    ).rejects.toMatchObject({ code: 'FORBIDDEN' })

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@functions/src/__tests__/callables/close-report.test.ts` around lines 66 - 77,
The test asserts a PERMISSION_DENIED error but closeReportCore uses the core
error code BantayogErrorCode.FORBIDDEN for cross-municipality denial; update the
assertion in the test (the expect of closeReportCore) to expect
BantayogErrorCode.FORBIDDEN (or the string 'FORBIDDEN') so it matches the thrown
error from closeReportCore and the core contract.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Validate --env explicitly.

Anything except the literal staging value falls into emulator mode here, so a typo like --env=stagin silently runs the wrong path. Reject unsupported values up front instead of inferring them from a negative check. As per coding guidelines, "Use defensive programming: validate external input at boundaries".

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@scripts/phase-3c/acceptance.ts` around lines 41 - 43, The current EMU
assignment treats any --env value other than the literal "staging" as emulator,
which allows typos; change the parsing to explicitly validate the --env
argument: extract the envArg from process.argv (the code that computes EMU),
accept only a whitelist of allowed values (e.g., "staging" and
"emulator"/"local" or whatever project supports), set EMU = true only when the
validated env equals the emulator value and set PROJECT_ID based on the
validated env (or throw/exit with a clear error when an unsupported value is
provided), so replace the current negative check with explicit validation and a
clear error path referencing the EMU constant and the code that computes
PROJECT_ID.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

# First, let's read the acceptance.ts file around line 154
cat -n scripts/phase-3c/acceptance.ts | sed -n '140,170p'

Repository: Exc1D/bantayog-alert

Length of output: 1242

---

🏁 Script executed:

# Also check the firebase.ts file mentioned in learnings to understand the setup
cat -n apps/citizen-pwa/src/services/firebase.ts | sed -n '30,60p'

Repository: Exc1D/bantayog-alert

Length of output: 979

---

🏁 Script executed:

# Search for webGetFunctions usage to understand the pattern
rg "webGetFunctions" -B 2 -A 2

Repository: Exc1D/bantayog-alert

Length of output: 2194

---

Pin the callable client to the deployed region.

webGetFunctions(webApp) defaults to us-central1, but Phase 3 functions are deployed to asia-southeast1. Without specifying the region, callable invocations on line 164 would fail in production. This must match the backend service setup in apps/citizen-pwa/src/services/firebase.ts.

🔧 Minimal fix

-  const webFunctions = webGetFunctions(webApp)
+  const webFunctions = webGetFunctions(webApp, 'asia-southeast1')

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@scripts/phase-3c/acceptance.ts` at line 154, The webFunctions client is
defaulting to us-central1 causing callable failures; change the call to
webGetFunctions in acceptance.ts to pin the region used for Phase 3 by passing
the deployed region (e.g., "asia-southeast1") so webFunctions =
webGetFunctions(webApp, "asia-southeast1") and ensure this region matches the
backend service setup used in apps/citizen-pwa/src/services/firebase.ts (or
import the shared region constant if available).