Conversation
Uses globalThis.crypto.subtle (Web Crypto API) so the hash runs correctly
in both the browser and Node.js 20+ without any require('node:crypto')
fallback, which broke under ESM/browser bundling.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Without await, hash was a Promise not a string, so every idempotency check would always miss (Promise !== stored hash string), making the guard a no-op for duplicate detection. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…l enforceAppCheck - Add rate limiting (30 req/min per uid) to match sibling callables - Replace direct dispatch.assignedTo.uid access with hasValidAssignedResponder guard to avoid TypeError when the field is missing or malformed - IdempotencyMismatchError now maps to already-exists instead of leaking an opaque functions/internal to the client - enforceAppCheck: process.env.NODE_ENV === 'production' so emulator runs without requiring a real App Check token Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Cover the declined status mirror path that was missing from the trigger test suite — review finding #14. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…atibility The emulator's source analysis follows package.json entrypoints. Without a JS build the emulator fails to load the package before any test runs. index.js is the hand-written JS shim; eslint.config.js ignores it from the TypeScript project service check (same pattern as firebase-messaging-sw.js). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ables - auth-provider: getIdTokenResult rejection was silently swallowed, leaving the app stuck on a loading screen; now sets claims=null and clears loading - await-auth-token: new helper that forces a fresh ID token via getIdToken(true) and waits for onIdTokenChanged to confirm the refresh propagated - useDeclineDispatch, useAcceptDispatch, useAdvanceDispatch: call awaitFreshAuthToken before every callable so stale tokens don't produce misleading auth failures; log errors via console.error for observability Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…gion - App Check: when VITE_USE_EMULATOR=true and no ReCaptcha site key is set, initialize with a CustomProvider that returns a synthetic emulator token; without this the callable returns FirebaseError: internal in emulator runs - getFunctions: add 'asia-southeast1' region to match callable definitions; without it the client targeted us-central1 causing a 404 which the SDK mapped to internal instead of not-found Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… error messages - useDispatch: onSnapshot error now logs with console.error (was silent); snapshot mapping errors log and set error state instead of crashing - DispatchDetailPage: auto-advance useEffect now includes dispatch?.status in the dep array so it does not fire on every re-render; add advanceError early-return guard so the page is not stuck after a failed auto-advance - DispatchDetailPage: fix RaceLossScreen dead-code check (was .message, now uses getFirebaseErrorCode to match .code correctly) - DispatchDetailPage: replace raw Firebase error codes shown to responder with human-readable messages - DispatchListPage: log callable errors with console.error Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- responder.spec.ts: 4 empty test bodies (accepts, advances, resolves) registered as passing trivially; converted to test.skip with empty impl - responder.spec.ts: add 'declines a pending dispatch with a reason' — logs in, navigates to the pending dispatch, submits a decline reason, and asserts the detail page transitions to the terminal state - responder-seed.ts: rewrite with correct firebase-admin usage; defaults FIRESTORE_EMULATOR_HOST to 127.0.0.1:8081; returns dispatchId + uid - e2e-tests/package.json: add @bantayog/shared-validators workspace dep Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- docs/reviews: multi-agent code review output for commits a56ca68 and 42544f1 - docs/superpowers/plans: step-by-step remediation plan from the review - docs/learnings: stale compiled functions binary is the first thing to check when FirebaseError: internal appears in E2E but unit tests pass - docs/progress: record post-remediation E2E pass (6/6) and root cause Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds guarded auth token refresh + an awaitFreshAuthToken helper; initializes App Check and sets Functions region; requires fresh tokens before client callables and strengthens error handling; introduces dispatch-presentation utilities; large suite of new tests across callables/triggers/rules/services; adds Admin SDK init and account-lifecycle callables; updates e2e fixtures, docs, and config. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Auth as FirebaseAuth
participant Functions
participant Firestore
Client->>Auth: awaitFreshAuthToken(auth)\n(call getIdToken(true) + subscribe onIdTokenChanged)
note right of Auth: token refresh requested\nresolve when onIdTokenChanged emits matching uid
Auth-->>Client: resolved User or null/error
Client->>Functions: call httpsCallable (with fresh token + App Check)
Functions->>Firestore: perform transactional updates (dispatch/report/etc.)
Firestore-->>Functions: commit result
Functions-->>Client: callable response or error (with code)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
…ort, function or class' Unused variable reportTypeSchema. Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
…ort, function or class' Unused variable reportTypeSchema. Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 12
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/responder-app/src/app/auth-provider.tsx`:
- Around line 23-34: The token-refresh promise chain starting at
u.getIdTokenResult(true) can overwrite state after an auth change; fix it by
capturing the current user's unique identity before calling getIdTokenResult
(e.g., const currentUid = u?.uid) and, when the promise
resolves/rejects/finally, verify the current auth user still matches that
identity before calling setClaims or setLoading; update the handlers in the
chain (the then, catch and finally blocks) to bail out if the user has changed
(or the component is unmounted) so stale results cannot override the new
session's state.
In `@apps/responder-app/src/app/await-auth-token.ts`:
- Around line 7-18: The Promise "refreshed" created around onIdTokenChanged can
hang if user.getIdToken(true) rejects; modify the logic so getIdToken(true) is
awaited with error handling that unsubscribes the onIdTokenChanged listener and
settles the "refreshed" promise (either resolve null or reject with the error)
instead of leaving it pending. Concretely, wrap the call to
user.getIdToken(true) in a try/catch (or attach .catch) and in the catch call
unsubscribe() and then resolve/refuse the outer promise; ensure the same
unsubscribe() is called in the existing onIdTokenChanged callback path so
unsubscribe is always invoked. Make sure to reference the existing "refreshed"
promise, "onIdTokenChanged(auth, ...)", "unsubscribe", and
"user.getIdToken(true)" symbols when applying the change.
In `@apps/responder-app/src/hooks/useAcceptDispatch.ts`:
- Around line 19-24: awaitFreshAuthToken(auth) can return null when
auth.currentUser is missing, so before calling the httpsCallable
'acceptDispatch' you must check the token result and abort locally if it's null
to avoid an unauthenticated network call; update useAcceptDispatch to capture
the return of awaitFreshAuthToken(auth), return/throw a local auth error (or
early return) when it is null, and only create/invoke the httpsCallable fn ({
dispatchId, idempotencyKey: keyRef.current }) when the token is present—apply
the same null-check guard to the other callable hooks added in this PR.
In `@apps/responder-app/src/hooks/useDispatch.ts`:
- Around line 34-59: The hardcoded schemaKeys array in useDispatch.ts is
fragile; replace it with a programmatic derivation from the shared Zod schema
(dispatchDocSchema) so the keys stay in sync: import dispatchDocSchema from
`@bantayog/shared-validators` and derive schemaKeys from dispatchDocSchema (e.g.,
using Object.keys(dispatchDocSchema.shape) or the schema's keyof utility and
cast to the inferred keys type), then remove the hardcoded list and add a short
comment referencing dispatchDocSchema to explain the source.
In `@apps/responder-app/src/pages/DispatchDetailPage.tsx`:
- Around line 116-120: The auto-advance useEffect currently calls
advance('acknowledged') only while dispatch?.status === 'accepted' and if that
call fails there's no way to surface a retry because the UI only shows the retry
button when dispatch.status === 'acknowledged'; change the flow so failures are
captured and surfaced via local state and the retry button is rendered based on
that state (not solely on dispatch.status). Concretely: wrap the
advance('acknowledged') call inside useEffect in a try/catch, set a local state
like ackError or ackFailed when the call throws, and update the retry button
rendering logic to show when ackFailed (and/or when dispatch?.status ===
'accepted' && ackFailed); wire the retry button to call advance('acknowledged')
again and clear ackFailed on success. Ensure useEffect still depends on
dispatch?.status and advance, and avoid mutating dispatch directly.
- Around line 197-203: The UI is wrongly mapping
getFirebaseErrorCode(advanceState.error) === 'functions/permission-denied' to
"Dispatch was cancelled…"; change the conditional so permission-denied displays
the real authorization error (use getActionErrorMessage(advanceState.error)) and
only render the explicit cancellation message when the backend sends a dedicated
cancel code (e.g., check for a specific cancel code such as
'functions/dispatch-cancelled' or 'dispatch/cancelled' from getFirebaseErrorCode
or another error field). Update the ternary in the render branch that reads
advanceState.error / getFirebaseErrorCode to stop treating
'functions/permission-denied' as a cancellation and instead fall back to
getActionErrorMessage unless the explicit cancel code is present.
In `@docs/reviews/2026-04-23-phase5-responder-mvp-review.md`:
- Around line 11-32: Add a language specifier to the markdown code fence that
contains the priority list (the block starting with "STOP-SHIP (before any
user-facing deploy):") to satisfy markdownlint MD040 and improve rendering;
change the opening fence from ``` to ```text or ```plaintext so the linter no
longer flags the block and the list renders consistently.
In `@e2e-tests/package.json`:
- Line 14: The lockfile is out of sync because the new dependency
"@bantayog/shared-validators" was added to e2e-tests/package.json but
pnpm-lock.yaml wasn't updated; run pnpm install from the repository root to
regenerate pnpm-lock.yaml (ensuring the new "workspace:*" entry is captured),
verify the lockfile changes, and commit the updated pnpm-lock.yaml so CI no
longer fails with ERR_PNPM_OUTDATED_LOCKFILE.
In `@functions/src/__tests__/triggers/dispatch-mirror-to-report.test.ts`:
- Around line 162-176: The test currently uses `.resolves.not.toThrow()` which
is invalid because `.toThrow()` expects a function; update the assertion in the
withAdminDb block that calls dispatchMirrorToReportCore so the promise is
awaited directly or asserted for a resolved value (e.g., `await
dispatchMirrorToReportCore({...})` or
`expect(dispatchMirrorToReportCore({...})).resolves.toBeDefined()`);
specifically change the call around dispatchMirrorToReportCore({ db, dispatchId,
beforeData: { status: 'pending' }, afterData: { status: 'accepted', reportId:
'nonexistent-report', correlationId: ... } }) to use a direct await or
`.resolves.toBeDefined()` instead of `.resolves.not.toThrow()`.
- Around line 10-11: Replace the process-global, order-dependent getApps()[0]
pattern with a named app instance: instead of using getApps()[0] ??
initializeApp(...), attempt to getApp('dispatch-mirror-test') (or wrap getApp in
try/catch) and if that fails call initializeApp({ projectId:
'dispatch-mirror-test' }, 'dispatch-mirror-test'); then pass that returned app
into getFirestore to set adminDb. This ensures the variables app and adminDb
(and functions getApps, initializeApp, getApp, getFirestore) always reference
the test-specific named app and avoids cross-test ordering flakiness.
In `@packages/shared-sms-parser/index.js`:
- Around line 93-104: The exported function parseInboundSms currently calls
body.trim() which will throw if body is null/undefined or not a string; add a
defensive guard at the start of parseInboundSms to validate that body is a
string (e.g., if typeof body !== 'string' or !body) and handle that case
consistently by returning the same "no match" response shape (confidence:
'none', parsed: null, candidates: [], autoReplyText: buildAutoReply('none'))
instead of proceeding to compute normalized/originalRest; reference
parseInboundSms, the normalized and originalRest locals, and buildAutoReply when
implementing the early-return guard.
- Line 6: Remove the unused dead constant reportTypeSchema from the file (or if
you intended to validate the looked-up report type, apply the schema to the
value returned from TYPE_SYNONYMS lookup by calling the enum validator on
reportType after you resolve it); specifically either delete the declaration
const reportTypeSchema = z.enum([...]) or replace its removal with a validation
step that runs reportTypeSchema.parse(reportType) (or safeParse) against the
resolved reportType variable so the schema is actually used.
🪄 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: c2df25ff-10aa-4ce8-b5c0-8d82d123d656
📒 Files selected for processing (25)
apps/responder-app/src/app/auth-provider.tsxapps/responder-app/src/app/await-auth-token.tsapps/responder-app/src/app/firebase.tsapps/responder-app/src/hooks/useAcceptDispatch.tsapps/responder-app/src/hooks/useAdvanceDispatch.tsapps/responder-app/src/hooks/useDeclineDispatch.tsapps/responder-app/src/hooks/useDispatch.tsapps/responder-app/src/pages/DispatchDetailPage.tsxapps/responder-app/src/pages/DispatchListPage.tsxdocs/learnings.mddocs/progress.mddocs/reviews/2026-04-23-phase5-responder-mvp-review.mddocs/superpowers/plans/2026-04-23-phase-5-responder-review-remediation.mde2e-tests/fixtures/responder-seed.tse2e-tests/package.jsone2e-tests/specs/responder.spec.tseslint.config.jsfunctions/src/__tests__/callables/decline-dispatch.test.tsfunctions/src/__tests__/triggers/dispatch-mirror-to-report.test.tsfunctions/src/callables/decline-dispatch.tsfunctions/src/idempotency/guard.tspackages/shared-sms-parser/index.jspackages/shared-sms-parser/package.jsonpackages/shared-validators/src/idempotency.test.tspackages/shared-validators/src/idempotency.ts
Comment on lines
116
to
120
| useEffect(() => { | ||
| if (dispatch?.status === 'accepted' && !advanceAttemptedRef.current) { | ||
| advanceAttemptedRef.current = true | ||
| if (dispatch?.status === 'accepted') { | ||
| void advance('acknowledged') | ||
| } | ||
| }, [dispatch?.status, advance]) |
Contributor
There was a problem hiding this comment.
The acknowledgement retry path is unreachable after the auto-advance fails.
advance('acknowledged') only runs while the dispatch is still accepted. If that call fails, the status never becomes acknowledged, so the retry button gated on dispatch.status === 'acknowledged' never renders and the responder has no recovery path.
🔁 Suggested fix
useEffect(() => {
if (dispatch?.status === 'accepted') {
void advance('acknowledged')
}
}, [dispatch?.status, advance])
@@
+ {dispatch.status === 'accepted' && advanceState.error && !advanceState.loading && (
+ <button
+ onClick={() => {
+ void advanceState.advance('acknowledged')
+ }}
+ >
+ Retry acknowledgement
+ </button>
+ )}
{dispatch.status === 'acknowledged' && !advanceState.loading && (
<>
<button
onClick={() => {
void advanceState.advance('en_route')
}}
>
Heading there
</button>
- {advanceState.error && (
- <button
- onClick={() => {
- void advanceState.advance('acknowledged')
- }}
- >
- Retry acknowledgement
- </button>
- )}
</>
)}Also applies to: 160-177
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/responder-app/src/pages/DispatchDetailPage.tsx` around lines 116 - 120,
The auto-advance useEffect currently calls advance('acknowledged') only while
dispatch?.status === 'accepted' and if that call fails there's no way to surface
a retry because the UI only shows the retry button when dispatch.status ===
'acknowledged'; change the flow so failures are captured and surfaced via local
state and the retry button is rendered based on that state (not solely on
dispatch.status). Concretely: wrap the advance('acknowledged') call inside
useEffect in a try/catch, set a local state like ackError or ackFailed when the
call throws, and update the retry button rendering logic to show when ackFailed
(and/or when dispatch?.status === 'accepted' && ackFailed); wire the retry
button to call advance('acknowledged') again and clear ackFailed on success.
Ensure useEffect still depends on dispatch?.status and advance, and avoid
mutating dispatch directly.
| "postinstall": "playwright install chromium" | ||
| }, | ||
| "devDependencies": { | ||
| "@bantayog/shared-validators": "workspace:*", |
Contributor
There was a problem hiding this comment.
Lockfile out of sync — CI is failing.
The pipeline reports ERR_PNPM_OUTDATED_LOCKFILE because pnpm-lock.yaml doesn't reflect the new @bantayog/shared-validators dependency. Run pnpm install at the repo root to regenerate the lockfile and commit it.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@e2e-tests/package.json` at line 14, The lockfile is out of sync because the
new dependency "@bantayog/shared-validators" was added to e2e-tests/package.json
but pnpm-lock.yaml wasn't updated; run pnpm install from the repository root to
regenerate pnpm-lock.yaml (ensuring the new "workspace:*" entry is captured),
verify the lockfile changes, and commit the updated pnpm-lock.yaml so CI no
longer fails with ERR_PNPM_OUTDATED_LOCKFILE.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
packages/shared-sms-parser/index.js (2)
4-24:⚠️ Potential issue | 🔴 CriticalBlocker: missing
FALLBACK_BARANGAYSdeclaration causes invalid JS syntax.Line 6 begins array elements without an owning declaration, which matches the syntax failure reported by CodeQL.
💡 Minimal fix
const require = createRequire(import.meta.url) +const FALLBACK_BARANGAYS = [ { name: 'Alcoy', municipality: 'Alcoy' }, { name: 'Bagasbas', municipality: 'Daet' }, { name: 'Baay', municipality: 'Labo' }, @@ { name: 'San', municipality: 'Jose Panganiban' }, { name: 'San Jose', municipality: 'Jose Panganiban' }, ]#!/bin/bash set -euo pipefail FILE="packages/shared-sms-parser/index.js" echo "== Top of file ==" sed -n '1,35p' "$FILE" echo echo "== Declaration check ==" rg -n "const FALLBACK_BARANGAYS\\s*=\\s*\\[" "$FILE" || trueExpected result: a
const FALLBACK_BARANGAYS = [declaration appears before the first{ name: ... }entry.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/shared-sms-parser/index.js` around lines 4 - 24, The file contains raw array entries starting with "{ name: ... }" with no variable declaration, causing invalid JS syntax; fix by adding a top-level declaration "const FALLBACK_BARANGAYS = [" immediately before the first "{ name: 'Alcoy', ... }" entry and ensure the array is properly closed with "]" (and a trailing semicolon) at the end of the listed items in packages/shared-sms-parser/index.js; also verify there are no duplicate FALLBACK_BARANGAYS declarations and add an export (e.g., export or module.exports) only if other code expects to import FALLBACK_BARANGAYS.
89-91:⚠️ Potential issue | 🟠 MajorGuard
parseInboundSmsinput type before calling.trim().
body.trim()will throw for non-string input, and this function is an external boundary.🛡️ Boundary guard
export function parseInboundSms(body) { + if (typeof body !== 'string') { + return { + confidence: 'none', + parsed: null, + candidates: [], + autoReplyText: buildAutoReply('none'), + } + } const normalized = body.trim().replace(/\s+/g, ' ').toUpperCase() const originalRest = body.trim().replace(/\s+/g, ' ')Based on learnings: “Applies to **/*.{js,ts,tsx,jsx} : Use defensive programming: validate external input at boundaries and never swallow errors with empty catch blocks.”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/shared-sms-parser/index.js` around lines 89 - 91, Guard the external boundary in parseInboundSms by validating the input before calling .trim(): check that the incoming body is a string (e.g., typeof body === 'string') and handle non-string cases explicitly (throw a clear TypeError or coerce to an empty string) so lines that compute normalized and originalRest (variables normalized and originalRest in parseInboundSms) never call .trim() on non-strings; update the function to perform this early check and return or throw accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/shared-sms-parser/index.js`:
- Around line 43-52: getBarangayGazetteer currently swallows all exceptions from
require('@bantayog/shared-data') which can hide real runtime errors; change the
catch to only treat module-missing as a fallback (e.g., inspect the caught error
and if error.code === 'MODULE_NOT_FOUND' or the message indicates missing
module, return FALLBACK_BARANGAYS), but for any other error rethrow or log it
(preserve the original error) so failures in loading/parsing the module are not
silently ignored; keep the existing behavior of returning FALLBACK_BARANGAYS
only for the absent-module case and reference getBarangayGazetteer,
require('@bantayog/shared-data'), and FALLBACK_BARANGAYS when making the change.
---
Duplicate comments:
In `@packages/shared-sms-parser/index.js`:
- Around line 4-24: The file contains raw array entries starting with "{ name:
... }" with no variable declaration, causing invalid JS syntax; fix by adding a
top-level declaration "const FALLBACK_BARANGAYS = [" immediately before the
first "{ name: 'Alcoy', ... }" entry and ensure the array is properly closed
with "]" (and a trailing semicolon) at the end of the listed items in
packages/shared-sms-parser/index.js; also verify there are no duplicate
FALLBACK_BARANGAYS declarations and add an export (e.g., export or
module.exports) only if other code expects to import FALLBACK_BARANGAYS.
- Around line 89-91: Guard the external boundary in parseInboundSms by
validating the input before calling .trim(): check that the incoming body is a
string (e.g., typeof body === 'string') and handle non-string cases explicitly
(throw a clear TypeError or coerce to an empty string) so lines that compute
normalized and originalRest (variables normalized and originalRest in
parseInboundSms) never call .trim() on non-strings; update the function to
perform this early check and return or throw accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: b42fdeba-25e0-4bef-a3ff-84ba67fb8a53
📒 Files selected for processing (1)
packages/shared-sms-parser/index.js
- await-auth-token: move getIdToken(true) inside Promise constructor to avoid hanging on rejection - auth-provider: add active flag + uid guard to prevent stale promise overwriting auth state - callable hooks: add null check on awaitFreshAuthToken before invoking httpsCallable - useDispatch: derive schemaKeys from dispatchDocSchema.shape instead of hardcoded list - DispatchDetailPage: show retry button when status=accepted+error; remove false permission-denied→"cancelled by admin" mapping - sms-parser: remove unused reportTypeSchema+z import; add body type guard; narrow getBarangayGazetteer catch to MODULE_NOT_FOUND - test: use named Firebase app instance; replace invalid .resolves.not.toThrow() with direct await - docs: add text language specifier to code fence (MD040) - lockfile: regenerated after @bantayog/shared-validators added to e2e-tests All verifications pass: typecheck, lint, and sms-parser tests. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/responder-app/src/hooks/useDispatch.ts`:
- Around line 94-95: Create the missing module
apps/responder-app/src/lib/dispatch-presentation.ts and export two functions
with the exact signatures expected by consumers: getResponderUiState(status:
DispatchStatus): ResponderUiState and getTerminalSurface(statusOrCode: string):
TerminalSurface; implement the logic per the Phase 5 MVP plan (mapping
DispatchStatus to ResponderUiState and mapping status/code strings to
TerminalSurface) and export any related type imports (DispatchStatus,
ResponderUiState, TerminalSurface) so useDispatch.ts and useOwnDispatches.ts can
import them without ESLint/type errors.
In `@apps/responder-app/src/pages/DispatchDetailPage.tsx`:
- Around line 80-93: getActionErrorMessage currently only checks Firebase error
codes via getFirebaseErrorCode, so client-side Errors thrown by hooks (e.g.,
message values 'auth_required' or 'resolutionSummary_required') fall through to
the generic message; update getActionErrorMessage to also inspect the
Error.message (and/or error.name) when getFirebaseErrorCode(error) is falsy and
map known client-side messages to user-friendly strings (handle 'auth_required'
-> 'You must be signed in to perform this action.' and
'resolutionSummary_required' -> 'Please provide a resolution summary before
completing this action.'), keeping the existing Firebase code checks and default
fallback intact.
🪄 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: bc2727bb-fa68-4eb1-af99-4069fec6c2b5
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (12)
apps/responder-app/src/app/auth-provider.tsxapps/responder-app/src/app/await-auth-token.tsapps/responder-app/src/hooks/useAcceptDispatch.tsapps/responder-app/src/hooks/useAdvanceDispatch.tsapps/responder-app/src/hooks/useDeclineDispatch.tsapps/responder-app/src/hooks/useDispatch.tsapps/responder-app/src/pages/DispatchDetailPage.tsxdocs/learnings.mddocs/progress.mddocs/reviews/2026-04-23-phase5-responder-mvp-review.mdfunctions/src/__tests__/triggers/dispatch-mirror-to-report.test.tspackages/shared-sms-parser/index.js
…etteer Fix CI failure on PR #60 by: - Update .gitignore to exclude only /lib/ at root, allow src/lib/ - Populate FALLBACK_BARANGAYS with all 282 barangays across 12 municipalities from PhilAtlas official PSGC data - Update SMS parser tests to use real barangay names Sources: - [Province of Camarines Norte | Philippine Statistics Authority](https://psa.gov.ph/classification/psgc/citimuni/0501600000) - [List of barangays of Camarines Norte – PhilAtlas](https://www.philatlas.com/lists/barangays-camarines-norte.html) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…rMessage Map client-side error messages thrown by hooks: - 'auth_required' → 'You must be signed in to perform this action.' - 'resolutionSummary_required' → 'Please provide a resolution summary before completing this action.' Keeps existing Firebase error code checks and generic fallback. Addresses CodeRabbit review comment about client-side errors falling through to generic message. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Add deny rule for responders updating reports directly. Regenerated via scripts/build-rules.ts to sync with latest schema. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This was referenced Apr 23, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Addresses all STOP-SHIP and BEFORE-NEXT-ITERATION findings from the multi-agent code review of commits
a56ca68and42544f1.declineDispatchgets rate limiting (30 req/min), a safehasValidAssignedResponderguard to prevent TypeError on missing field,IdempotencyMismatchErrornow maps toalready-existsinstead of opaqueinternal, andenforceAppCheckis gated onNODE_ENV=productionso the emulator doesn't require a real App Check tokencanonicalPayloadHashmade async using Web Crypto (globalThis.crypto.subtle); withoutawaitin the guard the hash was a Promise not a string, silently breaking duplicate detectionAuthProviderno longer swallowsgetIdTokenResultrejection (app was stuck loading); all callable hooks now callawaitFreshAuthTokenbefore invocation and log failures viaconsole.error.messageinstead of.code, so the race-loss screen was unreachabletest.skip; new decline smoke test exercises the full browser → callable → Firestore → onSnapshot loop (6 passed, 4 skipped)shared-sms-parsergets a JS entrypoint so the functions emulator can load the package;firebase.tsadds App CheckCustomProviderfor emulator runs and the correctasia-southeast1regionRoot Cause Discovery
The E2E decline test was failing with
FirebaseError: internalafter all client-side fixes were applied. Root cause:functions/lib/callables/decline-dispatch.jshad staleenforceAppCheck: true— the source had been changed but the functions were never rebuilt. Fix:pnpm --filter @bantayog/functions buildbeforefirebase emulators:exec. Added todocs/learnings.md.Test Plan
firebase emulators:exec --only firestore "pnpm --filter @bantayog/functions exec vitest run src/__tests__/callables/decline-dispatch.test.ts"— 18 passedpnpm --filter @bantayog/shared-validators test— 380 passedpnpm --filter @bantayog/responder-app typecheck && lint— PASSfirebase emulators:exec --project bantayog-alert-dev --only auth,functions,firestore,pubsub "playwright test specs/responder.spec.ts"— 6 passed, 4 skippedSummary by CodeRabbit
New Features
Bug Fixes
Documentation