refactor: implement refactor audit 2026-04-23 (P1-P3)#62
Merged
Conversation
…tor from Step2WhoWhere
…r and BarangaySelector
- Extract FALLBACK_BARANGAYS to data/fallback-barangays.ts - Extract storage error utilities to utils/storage-errors.ts - Extract useMunicipalityBarangays hook - Extract MunicipalitySelector, BarangaySelector, ContactFields components - Simplify Step2WhoWhere from 707 to 289 lines (-59%) - Add setLocationMethod to useGpsLocation return type Verification: typecheck, lint, test all PASS (137 passed, 2 todo) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…it items - P2 #7: Convert 14 implicit-any catch patterns to catch (err: unknown) - P2 #8: Replace console.log with console.info in shared-validators logging - P3 #9: Confirm no source any types remain in production code - P3 #10: Convert TODO to TICKET(BANTAYOG-PHASE6) - Fix remaining catch (replyErr) in sms-inbound-processor.ts
- P2 #6: Move AuthProvider and ProtectedRoute to @bantayog/shared-ui - AuthProvider accepts auth instance as prop, includes active guard and refreshClaims - ProtectedRoute accepts allowedRoles, requireActive, requireMunicipalityIdForRoles as props - Update admin-desktop and responder-app to consume shared components - Delete 4 duplicated auth files from apps - Add peer dependencies for firebase and react-router-dom
…ectedRoute - P2 #5: Add 11 tests covering AuthProvider (6) and ProtectedRoute (5) - AuthProvider: null user, authenticated user, signOut, refreshClaims, getIdTokenResult error, useAuth guard - ProtectedRoute: redirect to login, allowed role, denied role, requireActive deny, requireMunicipalityIdForRoles deny
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @Exc1D, your pull request is larger than the review limit of 150000 diff characters
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughConsolidates app-local auth and ProtectedRoute into Changes
Sequence Diagram(s)sequenceDiagram
participant App as Client App
participant Shared as `@bantayog/shared-ui`<br/>AuthProvider
participant Firebase as Firebase Auth
participant Token as getIdTokenResult
App->>Shared: Mount <AuthProvider auth={auth}>
Shared->>Firebase: onAuthStateChanged(auth)
Firebase-->>Shared: auth state (user|null)
alt user present
Shared->>Token: getIdTokenResult()
Token-->>Shared: token with claims
Shared-->>App: context ready (user, claims, loading=false)
App->>Shared: refreshClaims()
Shared->>Token: getIdTokenResult(true)
Token-->>Shared: refreshed claims
Shared-->>App: updated claims
else no user
Shared-->>App: loading=false, user=null, claims=null
end
sequenceDiagram
participant Client as SMS sender
participant Parser as Parser Module
participant Gazetteer as Gazetteer
participant Lev as Levenshtein
participant Auto as AutoReply
Client->>Parser: parseInboundSms("BANTAYOG <TYPE> <BARANGAY>")
Parser->>Gazetteer: getBarangayGazetteer()
Gazetteer-->>Parser: array of barangays
Parser->>Parser: normalize, extract type & token
alt exact match
Parser-->>Client: { confidence: "high", parsed: {...}, autoReplyText }
else fuzzy
Parser->>Lev: levenshtein(candidate, token)
Lev-->>Parser: distance
Parser->>Auto: buildAutoReply(confidence)
Auto-->>Parser: text
Parser-->>Client: { confidence: "medium"|"low", parsed/candidates, autoReplyText }
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![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: 17
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
functions/src/index.ts (1)
73-82:⚠️ Potential issue | 🟠 MajorUnsafe casts in
unknowncatch block can throw inside the error handler and mask original errors.Lines 74 and 81 use direct casts without type guards. If
errisnull,undefined, or a primitive value, accessing.codeor.messagewill throw, replacing the original error with a new one.Proposed fix
} catch (err: unknown) { - const code = (err as { code?: string }).code + const code = + typeof err === 'object' && err !== null && 'code' in err + ? (err as { code?: string }).code + : undefined if (code === 'MEDIA_REJECTED_MIME' || code === 'MEDIA_REJECTED_CORRUPT') { return } + const message = err instanceof Error ? err.message : String(err) log({ severity: 'ERROR', code: 'MEDIA_FINALIZE_FAILED', - message: `onMediaFinalize failed: ${(err as Error).message}`, + message: `onMediaFinalize failed: ${message}`, }) throw err }Per defensive programming guidelines: validate at the boundary and never swallow errors with unsafe type assertions that may themselves throw.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@functions/src/index.ts` around lines 73 - 82, The catch block in the onMediaFinalize flow is using unsafe casts on err (lines referencing the catch handling where err is read and logged) which can throw if err is null/undefined or a primitive; change it to defensively inspect err (use typeof checks or an isError-like guard) before accessing properties: safely derive code by checking err && typeof (err as any).code === 'string' and derive message via (err instanceof Error ? err.message : String(err || 'unknown error')), preserve the MEDIA_REJECTED_* early return logic using the safely-read code, and pass the safe message into the log call (the log invocation and MEDIA_FINALIZE_FAILED code should remain the same).functions/src/callables/dispatch-responder.ts (1)
165-171: 🧹 Nitpick | 🔵 TrivialDead code: the
typeof claims.role !== 'string'check is unreachable.Line 165 already validates that
claims.roleis exactly'municipal_admin'or'provincial_superadmin'using strict equality. If execution reaches line 169,claims.roleis guaranteed to be one of those string literals. The type check on lines 169-171 can never trigger.Either remove the redundant check, or reorder so the type check comes first if you want explicit string validation for clarity:
Option 1: Remove the redundant check
if (claims.role !== 'municipal_admin' && claims.role !== 'provincial_superadmin') { throw new HttpsError('permission-denied', 'municipal_admin or provincial_superadmin required') } if (claims.active !== true) throw new HttpsError('permission-denied', 'account is not active') - if (typeof claims.role !== 'string') { - throw new HttpsError('permission-denied', 'role is required') - }Option 2: Reorder to validate type first (if explicit check desired)
if (!claims) throw new HttpsError('unauthenticated', 'token required') + if (typeof claims.role !== 'string') { + throw new HttpsError('permission-denied', 'role is required') + } if (claims.role !== 'municipal_admin' && claims.role !== 'provincial_superadmin') { throw new HttpsError('permission-denied', 'municipal_admin or provincial_superadmin required') } if (claims.active !== true) throw new HttpsError('permission-denied', 'account is not active') - if (typeof claims.role !== 'string') { - throw new HttpsError('permission-denied', 'role is required') - }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@functions/src/callables/dispatch-responder.ts` around lines 165 - 171, The check "typeof claims.role !== 'string'" in the dispatch-responder permission block is unreachable because you already assert claims.role equals 'municipal_admin' or 'provincial_superadmin' earlier; remove the redundant type-check branch (the HttpsError thrown there) so only the role and active checks remain, or alternatively move the type validation to run before the role-equality checks (validate typeof claims.role === 'string' first, then test the specific values) — update the logic in the function containing claims.role and the HttpsError usage 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 `@apps/citizen-pwa/src/components/SubmitReportForm/ContactFields.tsx`:
- Around line 89-106: The Yes/No toggle buttons in the ContactFields component
only expose selection via CSS; add aria-pressed to both buttons so screen
readers get pressed-state semantics: set aria-pressed={anyoneHurt} on the "Yes"
button and aria-pressed={!anyoneHurt} on the "No" button (keep existing onClick
handlers and className logic intact so visual state and behavior remain
unchanged).
- Around line 40-71: Replace the plain <p> labels with explicit <label> elements
and associate them to the corresponding inputs by adding id attributes to the
inputs and htmlFor on the labels in ContactFields.tsx: change the "Your name"
<p> to a <label htmlFor="reporter-name"> and give the name input
id="reporter-name" (used by reporterName/onReporterNameChange/onNameErrorClear),
and change the "Phone number" <p> to a <label htmlFor="reporter-msisdn"> and
give the phone input id="reporter-msisdn" (used by
reporterMsisdn/onReporterMsisdnChange/onPhoneErrorClear); keep existing classes,
placeholders, required flags and error rendering intact.
In `@apps/citizen-pwa/src/components/SubmitReportForm/Step2WhoWhere.tsx`:
- Around line 80-83: The early return when locationMethod === 'manual' &&
!selectedMunicipalityId prevents proceeding but gives no UI feedback; add a
municipality selection validation error (e.g., municipalityError) when
selectedMunicipalityId is missing and pass that to MunicipalitySelector instead
of the GPS-specific locationError from useGpsLocation; update the logic that
computes canProceed/validation to consider municipalityError so the UI shows the
error message near MunicipalitySelector when the user tries to continue with no
municipality selected.
- Around line 219-224: The MunicipalitySelector is receiving the GPS-specific
locationError when locationMethod === 'manual', which shows a misleading GPS
error in the dropdown; update the code so MunicipalitySelector only gets
municipality-specific validation errors (or no error prop) by removing
error={locationError} from the MunicipalitySelector call in Step2WhoWhere and
either (a) implement and surface a municipality error from
useMunicipalityBarangays and pass that instead, or (b) omit the error prop
entirely if there is no dropdown validation; if you intend to show a GPS-failure
reason while in manual mode, add a separate prop (e.g., gpsNotice or
manualModeReason) and pass locationError to that prop rather than to
MunicipalitySelector.
In `@apps/citizen-pwa/src/hooks/useGpsLocation.test.ts`:
- Around line 60-90: Add a test case to cover GeolocationPositionError code 2 by
mirroring the existing tests for codes 1 and 3: in
apps/citizen-pwa/src/hooks/useGpsLocation.test.ts create an it('handles position
unavailable (code 2)') that sets mockGetCurrentPosition.mockImplementation to
call the error callback with { code: 2 } as GeolocationPositionError, then
renderHook(() => useGpsLocation()), await act(() =>
result.current.attemptGps()), and assert result.current.locationError equals
'Could not get location. Choose municipality manually.',
result.current.locationMethod is 'manual', and result.current.isLoading is
false.
In `@apps/citizen-pwa/src/hooks/useGpsLocation.ts`:
- Around line 49-58: The catch block in useGpsLocation.ts currently handles
GeolocationPositionError codes 1 and 3 but lets code 2 fall back to the generic
message; update the error handling in the catch (err: unknown) block to
explicitly handle code 2 (POSITION_UNAVAILABLE) by setting a clear message via
setLocationError (for example "Location unavailable. Choose municipality
manually.") and then call setLocationMethod('manual'), or if you prefer, add an
inline comment next to the existing code path that documents why code 2
intentionally uses the generic fallback; reference the existing error branch
that inspects (err as GeolocationPositionError).code and the functions
setLocationError and setLocationMethod to locate where to make the change.
In `@apps/citizen-pwa/src/hooks/useMunicipalityBarangays.ts`:
- Around line 19-20: The hook currently exports a raw setter
setSelectedBarangayId allowing callers to pick any id; change this to a
validated handler (e.g., setSelectedBarangay) that checks the id against the
current municipality's barangay list before applying it. Locate the exported
setSelectedBarangayId and the reset export in useMunicipalityBarangays.ts, make
the internal state setter private, and replace the exported setter with a
function that: returns early for undefined (or calls reset), verifies the id
exists in the current barangays collection (by comparing to barangay.id), and
only then updates state; if invalid, either ignore or call reset and optionally
surface an error/boolean result. Update the hook's returned type/signature to
expose only the validated setSelectedBarangay and keep reset as-is.
In `@docs/progress.md`:
- Line 5: The progress heading "Refactor Audit 2026-04-23 — Implementation
Continuation (2026-04-24)" uses a future date (2026-04-24); update that heading
so the continuation date matches the audit batch (e.g., change "(2026-04-24)" to
"(2026-04-23)" or remove the parenthetical date) to keep the timeline
consistent; edit the line that contains that exact heading text to reflect the
non-future date.
In `@functions/src/callables/dispatch-responder-validation.ts`:
- Around line 53-69: The current flow calls assertResponderOnShift before
verifying municipality ownership and existence checks, which can surface
INVALID_STATUS_TRANSITION instead of the intended NOT_FOUND or FORBIDDEN errors;
move the checks for deps.actor.claims.municipalityId (and throw
BantayogError(BantayogErrorCode.INVALID_ARGUMENT) if missing), and the existence
checks for reportSnap.exists and responderSnap.exists (throwing NOT_FOUND) to
run before calling assertResponderOnShift, then call
assertResponderOnShift(rtdb, deps.actor.claims.municipalityId,
deps.responderUid, ...) last so missing/unauthorized resources return the
correct error classes.
In `@functions/src/callables/dispatch-responder-writes.ts`:
- Around line 27-30: The current check allows whitespace-only phone strings to
pass; update the validation around consentData (where recipientMsisdn is
assigned) to trim and assert a non-empty phone value before proceeding: ensure
consentData?.phone is a string, compute const trimmedPhone =
consentData.phone.trim(), return null if trimmedPhone === '' and then set
recipientMsisdn = trimmedPhone; keep locale logic unchanged (locale =
consentData.locale === 'en' ? 'en' : 'tl') so downstream enqueue payloads never
receive blank/whitespace MSISDNs.
In `@packages/shared-sms-parser/lib/inbound.js`:
- Around line 1-155: The file contains a duplicated parser implementation (the
function parseInboundSms) which conflicts with the canonical parser in
parser.js; remove the duplicate body and instead delegate to or re-export the
single source of truth: import the existing parseInboundSms implementation from
the canonical module (parser) and export it from this module, ensuring any
symbols used by consumers (parseInboundSms, and any exported types/constants if
required) are preserved; update/remove any local references to the duplicate
helpers so this module just forwards to the shared implementation to avoid
divergence.
In `@packages/shared-sms-parser/src/gazetteer.ts`:
- Around line 315-325: Tighten the catch predicate so we only treat a true
"module not found" for the package `@bantayog/shared-data` as the fallback case:
keep the existing isModuleNotFound check but replace the loose
message.includes('@bantayog/shared-data') test (isSharedDataLoadFailure) with an
explicit match for the standard Node message for a missing module (e.g., test
that err is an Error and its message exactly equals or matches the pattern
"Cannot find module '@bantayog/shared-data'" using a strict equality or a regex
match), so that only that exact failure returns FALLBACK_BARANGAYS and all other
errors continue to be rethrown.
In `@packages/shared-sms-parser/src/levenshtein.ts`:
- Around line 20-22: The defensive if-check against undefined for deletion,
insertion, and substitution is misleading because Uint32Array returns 0 for OOB
reads, so remove the runtime check and instead add a concise comment above the
accesses (mentioning Uint32Array OOB → 0 and that loop/indices are
mathematically bounded) to satisfy reviewers and explain why
noUncheckedIndexedAccess is not a practical concern here; reference the
variables deletion, insertion, substitution used in the Levenshtein DP loop so
reviewers can find the change quickly.
- Around line 27-28: The nullish coalescing fallback on the Uint32Array lookup
(result ?? 0) is unreachable because dp[...] always yields a number; remove the
dead ?? 0 and either assert the index access is intentional with a non-null
assertion (use result = dp[m * (n + 1) + n]!) or keep the current expression but
add a short comment above the line explaining it exists only to satisfy
TypeScript's noUncheckedIndexedAccess rule; reference the variables dp, m, n and
the local variable result when making the change.
In `@packages/shared-sms-parser/src/parser.ts`:
- Around line 109-113: The current details extraction uses barangayIndex =
originalRest.toUpperCase().indexOf(barangayToken.toUpperCase()) which picks the
first occurrence and can be wrong if the token appears earlier; update the logic
in parser.ts (variables: originalRest, barangayToken, barangayIndex,
detailsStartIndex, details) to find the occurrence closest to the expected
details start — e.g., search for the last occurrence with lastIndexOf, or
iterate matches (using a case-insensitive regex with word boundaries) and pick
the match whose index + detailsStartIndex is within originalRest bounds or is
the first such match after any known prefix; then slice from that chosen index +
detailsStartIndex to compute details.
- Around line 151-160: The check for match === undefined inside the block
guarded by if (fuzzyMatches.length === 1) is unreachable; update the handling in
parse (or the function containing fuzzyMatches) to remove the redundant
undefined branch or use a non-null assertion when assigning const match =
fuzzyMatches[0] (e.g., const match = fuzzyMatches[0]!) so TypeScript knows it's
defined, and ensure you still return the expected structure (confidence, parsed,
candidates, autoReplyText via buildAutoReply) for the single-match path without
the unreachable conditional.
In `@packages/shared-ui/src/protected-route.tsx`:
- Around line 37-40: The authorization check in protected-route.tsx currently
treats whitespace-only municipalityId as valid; update the condition that uses
requireMunicipalityIdForRoles and claims?.municipalityId to reject values that
are only whitespace by trimming before checking length (e.g. replace the current
(typeof claims?.municipalityId !== 'string' || !claims.municipalityId) check
with a trimmed non-empty string check), so any municipalityId must be a string
and claims.municipalityId.trim().length > 0; ensure you modify the if branch
guarding the route (the block using requireMunicipalityIdForRoles and
claims?.municipalityId) accordingly and keep error handling defensive.
---
Outside diff comments:
In `@functions/src/callables/dispatch-responder.ts`:
- Around line 165-171: The check "typeof claims.role !== 'string'" in the
dispatch-responder permission block is unreachable because you already assert
claims.role equals 'municipal_admin' or 'provincial_superadmin' earlier; remove
the redundant type-check branch (the HttpsError thrown there) so only the role
and active checks remain, or alternatively move the type validation to run
before the role-equality checks (validate typeof claims.role === 'string' first,
then test the specific values) — update the logic in the function containing
claims.role and the HttpsError usage accordingly.
In `@functions/src/index.ts`:
- Around line 73-82: The catch block in the onMediaFinalize flow is using unsafe
casts on err (lines referencing the catch handling where err is read and logged)
which can throw if err is null/undefined or a primitive; change it to
defensively inspect err (use typeof checks or an isError-like guard) before
accessing properties: safely derive code by checking err && typeof (err as
any).code === 'string' and derive message via (err instanceof Error ?
err.message : String(err || 'unknown error')), preserve the MEDIA_REJECTED_*
early return logic using the safely-read code, and pass the safe message into
the log call (the log invocation and MEDIA_FINALIZE_FAILED code should remain
the same).
🪄 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: f0c14564-a7c2-468c-91f6-a05c91af0f5e
⛔ Files ignored due to path filters (36)
functions/lib/__tests__/callables/https-error.test.d.ts.mapis excluded by!**/*.mapfunctions/lib/__tests__/callables/https-error.test.js.mapis excluded by!**/*.mapfunctions/lib/__tests__/triggers/dispatch-mirror-to-report.test.js.mapis excluded by!**/*.mapfunctions/lib/firestore/sms-inbound-processor.d.ts.mapis excluded by!**/*.mapfunctions/lib/firestore/sms-inbound-processor.js.mapis excluded by!**/*.mapfunctions/lib/http/sms-inbound.d.ts.mapis excluded by!**/*.mapfunctions/lib/http/sms-inbound.js.mapis excluded by!**/*.mapfunctions/lib/services/fcm-send.d.ts.mapis excluded by!**/*.mapfunctions/lib/services/fcm-send.js.mapis excluded by!**/*.mapfunctions/lib/services/sms-providers/semaphore.d.ts.mapis excluded by!**/*.mapfunctions/lib/services/sms-providers/semaphore.js.mapis excluded by!**/*.mapfunctions/lib/triggers/on-media-finalize.d.ts.mapis excluded by!**/*.mapfunctions/lib/triggers/on-media-finalize.js.mapis excluded by!**/*.mappackages/shared-sms-parser/lib/__tests__/inbound.test.js.mapis excluded by!**/*.mappackages/shared-sms-parser/lib/auto-reply.d.ts.mapis excluded by!**/*.mappackages/shared-sms-parser/lib/auto-reply.js.mapis excluded by!**/*.mappackages/shared-sms-parser/lib/gazetteer.d.ts.mapis excluded by!**/*.mappackages/shared-sms-parser/lib/gazetteer.js.mapis excluded by!**/*.mappackages/shared-sms-parser/lib/inbound.d.ts.mapis excluded by!**/*.mappackages/shared-sms-parser/lib/inbound.js.mapis excluded by!**/*.mappackages/shared-sms-parser/lib/index.d.ts.mapis excluded by!**/*.mappackages/shared-sms-parser/lib/index.js.mapis excluded by!**/*.mappackages/shared-sms-parser/lib/levenshtein.d.ts.mapis excluded by!**/*.mappackages/shared-sms-parser/lib/levenshtein.js.mapis excluded by!**/*.mappackages/shared-sms-parser/lib/parser.d.ts.mapis excluded by!**/*.mappackages/shared-sms-parser/lib/parser.js.mapis excluded by!**/*.mappackages/shared-ui/lib/auth-provider.d.ts.mapis excluded by!**/*.mappackages/shared-ui/lib/auth-provider.test.d.ts.mapis excluded by!**/*.mappackages/shared-ui/lib/index.d.ts.mapis excluded by!**/*.mappackages/shared-ui/lib/protected-route.d.ts.mapis excluded by!**/*.mappackages/shared-ui/lib/protected-route.test.d.ts.mapis excluded by!**/*.mappackages/shared-validators/lib/logging.d.ts.mapis excluded by!**/*.mappackages/shared-validators/lib/logging.js.mapis excluded by!**/*.mappackages/shared-validators/lib/sms-templates.d.ts.mapis excluded by!**/*.mappackages/shared-validators/lib/sms-templates.js.mapis excluded by!**/*.mappnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (86)
apps/admin-desktop/src/App.tsxapps/admin-desktop/src/app/auth-provider.tsxapps/admin-desktop/src/app/protected-route.tsxapps/admin-desktop/src/pages/DispatchModal.tsxapps/admin-desktop/src/pages/TriageQueuePage.tsxapps/admin-desktop/src/routes.tsxapps/citizen-pwa/src/components/SubmitReportForm/BarangaySelector.test.tsxapps/citizen-pwa/src/components/SubmitReportForm/BarangaySelector.tsxapps/citizen-pwa/src/components/SubmitReportForm/ContactFields.test.tsxapps/citizen-pwa/src/components/SubmitReportForm/ContactFields.tsxapps/citizen-pwa/src/components/SubmitReportForm/MunicipalitySelector.test.tsxapps/citizen-pwa/src/components/SubmitReportForm/MunicipalitySelector.tsxapps/citizen-pwa/src/components/SubmitReportForm/Step2WhoWhere.tsxapps/citizen-pwa/src/components/SubmitReportForm/location-constants.tsapps/citizen-pwa/src/data/fallback-barangays.tsapps/citizen-pwa/src/hooks/useGpsLocation.test.tsapps/citizen-pwa/src/hooks/useGpsLocation.tsapps/citizen-pwa/src/hooks/useMunicipalityBarangays.tsapps/citizen-pwa/src/hooks/useReport.tsapps/citizen-pwa/src/lib/photoUpload.tsapps/citizen-pwa/src/useCitizenShell.tsapps/citizen-pwa/src/utils/storage-errors.tsapps/responder-app/src/App.tsxapps/responder-app/src/app/protected-route.tsxapps/responder-app/src/hooks/useRegisterFcmToken.tsapps/responder-app/src/pages/DispatchListPage.tsxapps/responder-app/src/routes.tsxapps/responder-app/src/services/fcm-client.tsdocs/learnings.mddocs/progress.mddocs/refactor-audit-2026-04-23.mddocs/superpowers/plans/2026-04-23-step2wherewhere-refactoring.mdeslint.config.jsfunctions/lib/__tests__/callables/https-error.test.d.tsfunctions/lib/__tests__/callables/https-error.test.jsfunctions/lib/__tests__/triggers/dispatch-mirror-to-report.test.jsfunctions/lib/firestore/sms-inbound-processor.jsfunctions/lib/http/sms-inbound.jsfunctions/lib/services/fcm-send.jsfunctions/lib/services/sms-providers/semaphore.jsfunctions/lib/triggers/on-media-finalize.jsfunctions/src/callables/advance-dispatch.tsfunctions/src/callables/decline-dispatch.tsfunctions/src/callables/dispatch-responder-notify.tsfunctions/src/callables/dispatch-responder-validation.tsfunctions/src/callables/dispatch-responder-writes.tsfunctions/src/callables/dispatch-responder.tsfunctions/src/firestore/sms-inbound-processor.tsfunctions/src/index.tsfunctions/src/triggers/dispatch-sms-outbox.tspackages/shared-sms-parser/lib/__tests__/inbound.test.jspackages/shared-sms-parser/lib/auto-reply.d.tspackages/shared-sms-parser/lib/auto-reply.jspackages/shared-sms-parser/lib/gazetteer.d.tspackages/shared-sms-parser/lib/gazetteer.jspackages/shared-sms-parser/lib/inbound.jspackages/shared-sms-parser/lib/index.d.tspackages/shared-sms-parser/lib/index.jspackages/shared-sms-parser/lib/levenshtein.d.tspackages/shared-sms-parser/lib/levenshtein.jspackages/shared-sms-parser/lib/parser.d.tspackages/shared-sms-parser/lib/parser.jspackages/shared-sms-parser/package.jsonpackages/shared-sms-parser/src/__tests__/inbound.test.tspackages/shared-sms-parser/src/auto-reply.tspackages/shared-sms-parser/src/gazetteer.tspackages/shared-sms-parser/src/index.tspackages/shared-sms-parser/src/levenshtein.tspackages/shared-sms-parser/src/parser.tspackages/shared-sms-parser/vitest.config.tspackages/shared-ui/lib/auth-provider.d.tspackages/shared-ui/lib/auth-provider.test.d.tspackages/shared-ui/lib/index.d.tspackages/shared-ui/lib/protected-route.d.tspackages/shared-ui/lib/protected-route.test.d.tspackages/shared-ui/package.jsonpackages/shared-ui/src/auth-provider.test.tsxpackages/shared-ui/src/auth-provider.tsxpackages/shared-ui/src/index.tspackages/shared-ui/src/protected-route.test.tsxpackages/shared-ui/src/protected-route.tsxpackages/shared-ui/vitest.config.tspackages/shared-validators/lib/logging.jspackages/shared-validators/lib/sms-templates.jspackages/shared-validators/src/logging.tspackages/shared-validators/src/sms-templates.ts
💤 Files with no reviewable changes (4)
- apps/admin-desktop/src/app/protected-route.tsx
- apps/responder-app/src/app/protected-route.tsx
- apps/admin-desktop/src/app/auth-provider.tsx
- eslint.config.js
Comment on lines
+27
to
+28
| const result = dp[m * (n + 1) + n] | ||
| return result ?? 0 |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Nullish coalescing fallback is unreachable.
Uint32Array elements are always number (never undefined), so result ?? 0 is dead code. The ?? 0 likely exists to satisfy TypeScript's noUncheckedIndexedAccess, which is fine, but a brief comment would clarify intent.
Clarify intent with a comment
const result = dp[m * (n + 1) + n]
- return result ?? 0
+ // ?? 0 satisfies noUncheckedIndexedAccess; Uint32Array guarantees a number here
+ return result ?? 0📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const result = dp[m * (n + 1) + n] | |
| return result ?? 0 | |
| const result = dp[m * (n + 1) + n] | |
| // ?? 0 satisfies noUncheckedIndexedAccess; Uint32Array guarantees a number here | |
| return result ?? 0 |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/shared-sms-parser/src/levenshtein.ts` around lines 27 - 28, The
nullish coalescing fallback on the Uint32Array lookup (result ?? 0) is
unreachable because dp[...] always yields a number; remove the dead ?? 0 and
either assert the index access is intentional with a non-null assertion (use
result = dp[m * (n + 1) + n]!) or keep the current expression but add a short
comment above the line explaining it exists only to satisfy TypeScript's
noUncheckedIndexedAccess rule; reference the variables dp, m, n and the local
variable result when making the change.
…w comments - dispatch-responder-validation: reorder checks so NOT_FOUND/FORBIDDEN surface before assertResponderOnShift - dispatch-responder-writes: add trim() + empty check for MSISDN - dispatch-responder: remove dead typeof claims.role !== 'string' check - index.ts: defensive err narrowing before accessing .code/.message - gazetteer.ts: tighten MODULE_NOT_FOUND to regex match - parser.ts: remove unreachable branch, fix details extraction with lastIndexOf - levenshtein.ts: fix lint/typecheck loop with ?? 0 narrowing - protected-route.tsx: add .trim().length > 0 for municipalityId
- Replace <p className="field-label"> with <label htmlFor="..."> for name and phone inputs; add matching id attributes - Add aria-pressed to Yes/No toggle buttons for screen reader state
- Step2WhoWhere: add municipalityError state, show error when user tries to continue without selecting a municipality in manual mode - Step2WhoWhere: pass municipalityError (not GPS locationError) to MunicipalitySelector for clearer semantics - useMunicipalityBarangays: replace raw setSelectedBarangayId with validated handleSelectBarangay that only accepts ids present in the current municipality's barangay list
…tion - Add explicit error message for GeolocationPositionError code 2 - Add test case covering code 2 alongside existing codes 1 and 3
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
packages/shared-sms-parser/src/gazetteer.ts (1)
309-330: 🧹 Nitpick | 🔵 TrivialError handling pattern is sound but regex could be stricter.
The error matching has been improved from the previous substring check. The combined check of
isModuleNotFound && isSharedDataLoadFailureis reasonable defensive coding. However, the regex/Cannot find module .@bantayog\/shared-data./uses.which matches any character (not just the quote). Consider using an explicit character class for the quote:-const isSharedDataLoadFailure = /Cannot find module .@bantayog\/shared-data./.test(message) +const isSharedDataLoadFailure = /Cannot find module ['"]@bantayog\/shared-data['"]/.test(message)This is minor since the
isModuleNotFoundcheck already gates this, but it would make the intent clearer and avoid any edge-case false positives from transitive dependency messages that happen to contain the package name.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/shared-sms-parser/src/gazetteer.ts` around lines 309 - 330, The regex in getBarangayGazetteer that tests the error message for "Cannot find module ..." is too permissive; tighten it to explicitly match the surrounding quote character so it only matches messages like "Cannot find module '...'" or "Cannot find module \"...\"". Update the isSharedDataLoadFailure test to use a character class for the quote (e.g., ['\"]) around `@bantayog`\/shared-data, leaving the rest of the catch logic (isModuleNotFound, returning FALLBACK_BARANGAYS, and rethrowing) unchanged and keep references to mod.BARANGAY_GAZETTEER logic intact.
🤖 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/citizen-pwa/src/hooks/useMunicipalityBarangays.ts`:
- Around line 34-37: handleSelectMunicipality currently accepts any truthy
muniId and commits it to state; instead validate the incoming muniId before
calling setSelectedMunicipalityId and clearing barangay state. Update
handleSelectMunicipality to verify the id is a valid municipality (e.g., matches
the expected id pattern and/or exists in the local municipalities list/lookup
used by this hook) and only then call setSelectedMunicipalityId(muniId) and
setSelectedBarangayId(undefined); if invalid, leave state unchanged or set an
explicit "none" value and optionally surface an error. Ensure you reference and
use the same municipalities collection or validator used elsewhere in this hook
so invalid ids cannot be forwarded to Step2WhoWhere or submitted payloads.
In `@functions/src/callables/dispatch-responder-validation.ts`:
- Around line 93-107: The code unsafely casts rawStatus to the literal
'verified' before validating it; instead, validate rawStatus against the allowed
status values (or use an appropriate type guard/enum) and only assign to from as
a typed status (e.g., 'verified' | other) after isValidReportTransition check
passes; update the block around rawStatus, from and the isValidReportTransition
call so you do not force-cast rawStatus to 'verified' — perform a runtime check
(e.g., allowedStatuses.includes(rawStatus) or a guard function) then set from =
rawStatus with the correct narrowed type, and keep throwing
BantayogError(BantayogErrorCode.INVALID_STATUS_TRANSITION, ...) when validation
fails.
In `@functions/src/callables/dispatch-responder-writes.ts`:
- Around line 39-51: The interface WriteDispatchDocsArgs uses the global
FirebaseFirestore.DocumentReference for dispatchRef, reportRef, reportEvRef and
dispatchEvRef; replace those type annotations with the imported
DocumentReference from 'firebase-admin/firestore' and add the corresponding
import at the top of the file. Update the four fields in WriteDispatchDocsArgs
(dispatchRef, reportRef, reportEvRef, dispatchEvRef) to use DocumentReference
and ensure the file imports DocumentReference (e.g., import { DocumentReference
} from 'firebase-admin/firestore') so the types are explicit and consistent.
In `@packages/shared-sms-parser/src/parser.ts`:
- Around line 103-107: When token1 is a municipality prefix
(MUNICIPALITY_PREFIXES.has(token1)) extend the logic to look for a third token
(e.g., token3 = tokens[3]) and try a three-word barangay first: build candidate
= token1 + ' ' + token2 + ' ' + token3, and if present set barangayToken =
candidate and detailsStartIndex = barangayToken.length; if the three-word
candidate is not available or token3 is missing, fall back to the existing
two-word combination (token1 + ' ' + token2) as before. Ensure you reference and
update the same variables used in the diff (tokens, token2, token3,
MUNICIPALITY_PREFIXES, barangayToken, detailsStartIndex) and prefer the longest
match to avoid leaving part of the barangay in extracted details.
---
Duplicate comments:
In `@packages/shared-sms-parser/src/gazetteer.ts`:
- Around line 309-330: The regex in getBarangayGazetteer that tests the error
message for "Cannot find module ..." is too permissive; tighten it to explicitly
match the surrounding quote character so it only matches messages like "Cannot
find module '...'" or "Cannot find module \"...\"". Update the
isSharedDataLoadFailure test to use a character class for the quote (e.g.,
['\"]) around `@bantayog`\/shared-data, leaving the rest of the catch logic
(isModuleNotFound, returning FALLBACK_BARANGAYS, and rethrowing) unchanged and
keep references to mod.BARANGAY_GAZETTEER logic 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: db61995f-978b-43b2-a112-8f1cf8328ea4
⛔ Files ignored due to path filters (6)
packages/shared-sms-parser/lib/gazetteer.js.mapis excluded by!**/*.mappackages/shared-sms-parser/lib/inbound.d.ts.mapis excluded by!**/*.mappackages/shared-sms-parser/lib/levenshtein.d.ts.mapis excluded by!**/*.mappackages/shared-sms-parser/lib/levenshtein.js.mapis excluded by!**/*.mappackages/shared-sms-parser/lib/parser.d.ts.mapis excluded by!**/*.mappackages/shared-sms-parser/lib/parser.js.mapis excluded by!**/*.map
📒 Files selected for processing (17)
apps/citizen-pwa/src/components/SubmitReportForm/ContactFields.tsxapps/citizen-pwa/src/components/SubmitReportForm/Step2WhoWhere.tsxapps/citizen-pwa/src/hooks/useGpsLocation.test.tsapps/citizen-pwa/src/hooks/useGpsLocation.tsapps/citizen-pwa/src/hooks/useMunicipalityBarangays.tsdocs/progress.mdfunctions/src/callables/dispatch-responder-validation.tsfunctions/src/callables/dispatch-responder-writes.tsfunctions/src/callables/dispatch-responder.tsfunctions/src/index.tspackages/shared-sms-parser/lib/gazetteer.jspackages/shared-sms-parser/lib/levenshtein.jspackages/shared-sms-parser/lib/parser.jspackages/shared-sms-parser/src/gazetteer.tspackages/shared-sms-parser/src/levenshtein.tspackages/shared-sms-parser/src/parser.tspackages/shared-ui/src/protected-route.tsx
- Add VALID_MUNICIPALITY_IDS Set from CAMARINES_NORTE_MUNICIPALITIES - Guard handleSelectMunicipality against invalid/out-of-set ids
… validation - Remove premature 'verified' cast before isValidReportTransition check - Pass rawStatus as ReportStatus to validate transition - Assign from = 'verified' as const only after validation passes
…estore - Replace global FirebaseFirestore.DocumentReference with explicit import - Add DocumentReference to existing type imports
… prefix - When token1 is a municipality prefix, prefer longest match: try token1 + token2 + token3 first, fall back to token1 + token2 - Prevents partial barangay names from leaking into details
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
functions/src/callables/dispatch-responder-validation.ts (1)
102-109: 🛠️ Refactor suggestion | 🟠 MajorMake the
'verified'precondition explicit before returningfrom: 'verified'.Line 102 validates transition permissibility, but Line 109 still hard-codes
fromto'verified'without an explicit runtime equality check. If additional* -> assignedtransitions are introduced later, this silently mislabelsfrom.Proposed minimal hardening
const to = 'assigned' as const - if (!isValidReportTransition(rawStatus as ReportStatus, to)) { + if (rawStatus !== 'verified' || !isValidReportTransition(rawStatus as ReportStatus, to)) { throw new BantayogError( BantayogErrorCode.INVALID_STATUS_TRANSITION, `Cannot dispatch from status ${rawStatus}`, ) } // After validation, we know rawStatus === 'verified'. const from = 'verified' as const🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@functions/src/callables/dispatch-responder-validation.ts` around lines 102 - 109, The code validates the transition with isValidReportTransition(rawStatus as ReportStatus, to) but then unconditionally sets from = 'verified'; make the precondition explicit by checking rawStatus at runtime (e.g., assert or if-check) that rawStatus === 'verified' before returning/assigning from, and throw a BantayogError (or similar) if it isn't; update the block around isValidReportTransition, rawStatus and the from constant to perform this explicit equality check to avoid silently mislabeling from when other transitions are added.
🤖 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/citizen-pwa/src/hooks/useMunicipalityBarangays.ts`:
- Around line 50-57: The select change handler in useMunicipalityBarangays
treats only undefined as a clear action so an emitted empty string ('') doesn't
clear the selection; update the handler that takes (id: string | undefined) to
treat '' as a clear by checking for falsy/empty id (e.g., if (!id) or id ===
''), call setSelectedBarangayId(undefined) and return; keep the existing
barangayOptions.some((b) => b.name === id) branch for valid ids.
In `@functions/src/callables/dispatch-responder-validation.ts`:
- Around line 18-23: Change assertResponderOnShift to accept a single object
param instead of four positional args: replace (rtdb: Database, municipalityId:
string, responderUid: string, message?: string) with a single options object
type { rtdb, municipalityId, responderUid, message? } and update the function
body to destructure those properties; update all call sites to pass an object
(e.g. { rtdb, municipalityId, responderUid, message }) and adjust any
tests/mocks accordingly so argument ordering mistakes are prevented while
preserving the same runtime behavior and default message.
In `@functions/src/callables/dispatch-responder-writes.ts`:
- Around line 24-30: The code builds an SMS payload without checking explicit
sms consent; before reading phone/locale (use consentSnap, consentData,
recipientMsisdn, locale) add a strict consent check (e.g., require
consentData?.smsConsent === true) and return null if consent is missing or false
so no SMS is prepared for opted-out users; keep the existing phone/locale
validations after that check and fail fast if consent is not granted.
In `@packages/shared-sms-parser/src/gazetteer.ts`:
- Around line 311-314: The code returns mod.BARANGAY_GAZETTEER after only
checking Array.isArray, which allows malformed items that cause
entry.name.toLowerCase() runtime errors; update the logic that reads
mod.BARANGAY_GAZETTEER to validate each item shape (e.g., ensure each item is an
object and has a non-empty string 'name' property and any other required fields
for BarangayEntry), filter out or reject invalid items, and only return the
array cast to BarangayEntry[] after validation; reference the existing symbols
mod, BARANGAY_GAZETTEER, BarangayEntry and the consumer usage
entry.name.toLowerCase() when implementing the checks and emit a warning/log for
dropped items rather than silently swallowing errors.
---
Duplicate comments:
In `@functions/src/callables/dispatch-responder-validation.ts`:
- Around line 102-109: The code validates the transition with
isValidReportTransition(rawStatus as ReportStatus, to) but then unconditionally
sets from = 'verified'; make the precondition explicit by checking rawStatus at
runtime (e.g., assert or if-check) that rawStatus === 'verified' before
returning/assigning from, and throw a BantayogError (or similar) if it isn't;
update the block around isValidReportTransition, rawStatus and the from constant
to perform this explicit equality check to avoid silently mislabeling from when
other transitions are added.
🪄 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: 84165254-de8d-4f08-8d18-f83a917877c1
⛔ Files ignored due to path filters (3)
packages/shared-sms-parser/lib/gazetteer.js.mapis excluded by!**/*.mappackages/shared-sms-parser/lib/parser.d.ts.mapis excluded by!**/*.mappackages/shared-sms-parser/lib/parser.js.mapis excluded by!**/*.map
📒 Files selected for processing (7)
apps/citizen-pwa/src/hooks/useMunicipalityBarangays.tsfunctions/src/callables/dispatch-responder-validation.tsfunctions/src/callables/dispatch-responder-writes.tspackages/shared-sms-parser/lib/gazetteer.jspackages/shared-sms-parser/lib/parser.jspackages/shared-sms-parser/src/gazetteer.tspackages/shared-sms-parser/src/parser.ts
…erified precondition
- Change assertResponderOnShift to accept single options object
{ rtdb, municipalityId, responderUid, message? }
- Update call sites in dispatch-responder-validation.ts and dispatch-responder.ts
- Add explicit rawStatus !== 'verified' check before returning from
…angays against gazetteer - Add isBarangayEntry type guard to validate shape before returning BARANGAY_GAZETTEER from shared-data - Move gazetteer lookup earlier in parser; build uppercase-name Set - Only accept 3-word municipality-prefix barangay if it exists in the gazetteer; fall back to 2-word otherwise
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
functions/src/callables/dispatch-responder.ts (1)
57-65:⚠️ Potential issue | 🟠 MajorRemove the pre-transaction shift read.
This call can still convert a missing or out-of-municipality responder into
INVALID_STATUS_TRANSITIONbeforevalidateDispatchTransaction()reaches itsNOT_FOUND/FORBIDDENchecks. The in-transaction recheck already covers the real race window, so keeping this preflight read regresses error classification.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@functions/src/callables/dispatch-responder.ts` around lines 57 - 65, Remove the pre-transaction shift read by deleting the call to assertResponderOnShift that runs before the transaction (the block using deps.actor.claims.municipalityId and deps.responderUid); rely on the in-transaction check inside validateDispatchTransaction to perform the shift / municipality validation so error classification remains correct (avoid converting missing/out-of-municipality responder into INVALID_STATUS_TRANSITION outside the transaction).
🤖 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/callables/dispatch-responder.ts`:
- Around line 34-35: The severity guard in isValidSeverity currently uses the
`in` operator which matches inherited keys; change the check to use
Object.hasOwn(DEADLINE_BY_SEVERITY, s) so only own properties are accepted.
Update the function isValidSeverity(s: unknown): s is keyof typeof
DEADLINE_BY_SEVERITY to return typeof s === 'string' &&
Object.hasOwn(DEADLINE_BY_SEVERITY, s), ensuring DEADLINE_BY_SEVERITY lookups
yield numeric values (used later when computing deadlineMs) rather than
inherited functions.
In `@packages/shared-sms-parser/src/parser.ts`:
- Around line 20-25: The ParsedFields interface currently exposes an unused
rawBarangay property; either remove rawBarangay from ParsedFields or, if you
intend it for future debugging, add a clear doc comment explaining its purpose
and ensure callers expect it. Locate the ParsedFields interface (symbol:
ParsedFields) and remove the rawBarangay declaration and any references, and
update parseInboundSms callers and tests accordingly; alternatively add a JSDoc
comment above rawBarangay explaining it is optional/debug-only and ensure
lint/tests acknowledge it.
---
Duplicate comments:
In `@functions/src/callables/dispatch-responder.ts`:
- Around line 57-65: Remove the pre-transaction shift read by deleting the call
to assertResponderOnShift that runs before the transaction (the block using
deps.actor.claims.municipalityId and deps.responderUid); rely on the
in-transaction check inside validateDispatchTransaction to perform the shift /
municipality validation so error classification remains correct (avoid
converting missing/out-of-municipality responder into INVALID_STATUS_TRANSITION
outside the transaction).
🪄 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: 586a275d-35d7-4ccf-9c07-714596541217
⛔ Files ignored due to path filters (4)
packages/shared-sms-parser/lib/gazetteer.d.ts.mapis excluded by!**/*.mappackages/shared-sms-parser/lib/gazetteer.js.mapis excluded by!**/*.mappackages/shared-sms-parser/lib/parser.d.ts.mapis excluded by!**/*.mappackages/shared-sms-parser/lib/parser.js.mapis excluded by!**/*.map
📒 Files selected for processing (8)
apps/citizen-pwa/src/hooks/useMunicipalityBarangays.tsfunctions/src/callables/dispatch-responder-validation.tsfunctions/src/callables/dispatch-responder-writes.tsfunctions/src/callables/dispatch-responder.tspackages/shared-sms-parser/lib/gazetteer.jspackages/shared-sms-parser/lib/parser.jspackages/shared-sms-parser/src/gazetteer.tspackages/shared-sms-parser/src/parser.ts
…for severity guard - Remove pre-transaction assertResponderOnShift call; rely on in-transaction check inside validateDispatchTransaction for correct error classification - Replace 'in' operator with Object.hasOwn in isValidSeverity to avoid matching inherited prototype properties - Remove unused rawBarangay field from ParsedFields interface and tests
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
packages/shared-sms-parser/src/parser.ts (1)
114-121:⚠️ Potential issue | 🟠 MajorDetails extraction is incorrect when the barangay name appears again in details.
Using
lastIndexOfpicks the final token occurrence, so inputs likeBANTAYOG FLOOD ANGAS flooding in Angas areacan truncate details to only the tail segment. This affects parsed incident context. (Also propagates topackages/shared-sms-parser/lib/parser.jsLine 84-Line 90.)Proposed fix (derive details by token position, not substring search)
- // Find the barangay token in originalRest using lastIndexOf to prefer the occurrence - // closest to where details would start (avoids matching an earlier instance of the token). - const barangayTokenUpper = barangayToken.toUpperCase() - const barangayIndex = originalRest.toUpperCase().lastIndexOf(barangayTokenUpper) - const details = - barangayIndex !== -1 && barangayIndex + detailsStartIndex < originalRest.length - ? originalRest.slice(barangayIndex + detailsStartIndex).trim() - : undefined + // Compute details from token offsets to avoid false matches when barangay repeats in details. + const restOriginal = originalRest.replace(/^BANTAYOG\b\s*/i, '') + const restOriginalTokens = restOriginal.split(/\s+/) + const barangayWordCount = barangayToken.split(/\s+/).length + const details = + restOriginalTokens.length > 1 + barangayWordCount + ? restOriginalTokens.slice(1 + barangayWordCount).join(' ').trim() + : undefined🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/shared-sms-parser/src/parser.ts` around lines 114 - 121, The current details extraction uses lastIndexOf and can pick a repeated barangay token inside the details; change it to locate the barangay token by token/word-boundary match and derive details from the match position rather than using lastIndexOf. Specifically, replace the barangayIndex lookup (barangayIndex = originalRest.toUpperCase().lastIndexOf(barangayTokenUpper)) with a regex-based search on originalRest.toUpperCase() using word boundaries (e.g., \b{barangayTokenUpper}\b) to find the correct match index (prefer the first match that represents the header token), then compute details using that match index + detailsStartIndex against originalRest.slice(...).trim() (keeping the existing variables barangayTokenUpper, originalRest, detailsStartIndex, and assigning to details).
🤖 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/callables/dispatch-responder.ts`:
- Around line 83-91: The callable reads process.env.SMS_MSISDN_HASH_SALT and
will silently skip SMS if it's missing; update the dispatchResponder deployment
to surface this secret by creating a Secret via defineSecret (name
SMS_MSISDN_HASH_SALT) and adding it to the dispatchResponder's secrets array
(the same place FCM_VAPID_PRIVATE_KEY is bound), or alternatively
document/ensure it is provided as a deploy-time environment variable; also
verify buildSmsPayload and the salt usage still work once the secret is bound.
- Around line 186-191: The current claims construction strips municipality
information for tokens that lack a string municipalityId (breaking
provincial_superadmin which supplies permittedMunicipalityIds); update the
claims assembly in dispatch-responder.ts to include permittedMunicipalityIds
when municipalityId is not a string (e.g., add
...(Array.isArray(claims.permittedMunicipalityIds) ? { permittedMunicipalityIds:
claims.permittedMunicipalityIds } : {})), and update the earlier authorization
validation (the block that currently rejects missing municipalityId) to accept
either a string municipalityId or a non-empty permittedMunicipalityIds array for
roles including provincial_superadmin so the request is not incorrectly
rejected.
In `@packages/shared-sms-parser/src/__tests__/inbound.test.ts`:
- Line 10: Add a regression test to inbound.test.ts that exercises the parser
with a message where the barangay token appears twice (e.g., "ANGAS flooding in
Angas area") and assert that result.parsed?.details preserves the full string
rather than being truncated; locate the existing assertion using
expect(result.parsed?.details).toBeUndefined() and add a new test case that
calls the same parser helper (the function used to produce result in this file)
with the repeated-barangay input and checks
expect(result.parsed?.details).toEqual("flooding in Angas area") or the full
expected details string to ensure repeated tokens don’t remove or truncate
details.
---
Duplicate comments:
In `@packages/shared-sms-parser/src/parser.ts`:
- Around line 114-121: The current details extraction uses lastIndexOf and can
pick a repeated barangay token inside the details; change it to locate the
barangay token by token/word-boundary match and derive details from the match
position rather than using lastIndexOf. Specifically, replace the barangayIndex
lookup (barangayIndex =
originalRest.toUpperCase().lastIndexOf(barangayTokenUpper)) with a regex-based
search on originalRest.toUpperCase() using word boundaries (e.g.,
\b{barangayTokenUpper}\b) to find the correct match index (prefer the first
match that represents the header token), then compute details using that match
index + detailsStartIndex against originalRest.slice(...).trim() (keeping the
existing variables barangayTokenUpper, originalRest, detailsStartIndex, and
assigning to details).
🪄 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: 3869c806-79a0-4f03-8805-741f545f4d5d
⛔ Files ignored due to path filters (3)
packages/shared-sms-parser/lib/__tests__/inbound.test.js.mapis excluded by!**/*.mappackages/shared-sms-parser/lib/parser.d.ts.mapis excluded by!**/*.mappackages/shared-sms-parser/lib/parser.js.mapis excluded by!**/*.map
📒 Files selected for processing (7)
functions/src/__tests__/sms-inbound.test.tsfunctions/src/callables/dispatch-responder.tspackages/shared-sms-parser/lib/__tests__/inbound.test.jspackages/shared-sms-parser/lib/parser.d.tspackages/shared-sms-parser/lib/parser.jspackages/shared-sms-parser/src/__tests__/inbound.test.tspackages/shared-sms-parser/src/parser.ts
💤 Files with no reviewable changes (1)
- functions/src/tests/sms-inbound.test.ts
…ncial_superadmin - parser.ts: Replace lastIndexOf with word-boundary regex to find first barangay occurrence, preventing truncation when barangay repeats in details - dispatch-responder.ts: Define SMS_MSISDN_HASH_SALT via defineSecret and add to onCall secrets array so Firebase injects it in production - dispatch-responder.ts + validation.ts: Support provincial_superadmin by accepting permittedMunicipalityIds array; use getActorMunicipalityIds() helper for unified municipality checks; use responder.municipalityId for RTDB shift lookup - inbound.test.ts: Add regression test for repeated barangay tokens
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Implements items from the refactor audit spec (
docs/refactor-audit-2026-04-23.md).P1 — High Risk
inbound.tsinto modules: Extractedgazetteer.ts,levenshtein.ts,auto-reply.ts, and renamed main parser toparser.ts. Eliminated all 6@ts-expect-errorcomments. Cleaned stalelib/inbound.jsbuild artifacts.dispatch-responder.ts: Split intodispatch-responder-validation.ts,dispatch-responder-notify.ts,dispatch-responder-writes.ts. AddedmunicipalityIdruntime validation and fixed spread-order bug invalidatedResponder.P2 — Structural Debt
catch (err)/catch (error)patterns across 10 production source files tocatch (err: unknown)/catch (error: unknown).console.logfrom production code: Replaced withconsole.infoinshared-validators/src/logging.ts(Cloud Functions structured logging transport).shared-ui: New genericAuthProvider(withactiveguard,refreshClaims,useCallbackmemoization) andProtectedRoute(configurable viaallowedRoles,requireActive,requireMunicipalityIdForRolesprops). Bothadmin-desktopandresponder-appupdated. Deleted 4 duplicated files.shared-ui: Added vitest + testing-library infrastructure and 11 tests forAuthProvider(6) andProtectedRoute(5).P3 — Cleanup
anytypes in source: Confirmedfcm-send.tsalready uses properBatchResponsetype. No sourceanytypes remain.TICKET(BANTAYOG-PHASE6)with deferral rationale.Verification
npx turbo run lint typecheck— PASS (25/25)pnpm --filter @bantayog/shared-sms-parser test— PASS (13/13)pnpm --filter @bantayog/shared-ui test— PASS (11/11)Rollback
To revert:
git revert HEAD~4..HEADon this branch, or restore the 4 deleted auth files frommain.Note: P2 #5 remaining gaps (admin-desktop, responder-app, shared-data, shared-types tests) are explicitly marked "Large (spread across sprints)" in the audit and are deferred.
Summary by CodeRabbit
New Features
Refactor
Tests
Documentation