feat(phase6): native responder app — Capacitor, telemetry, field workflows, roster ops#68
Merged
feat(phase6): native responder app — Capacitor, telemetry, field workflows, roster ops#68
Conversation
…rceAppCheck, reason validation
…ate for push tap-through
…ilabilityOverride callables
![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:
📝 WalkthroughWalkthroughAdds Phase‑6 responder field functionality across client and backend: new Firebase callables (backup, SOS, witness report, unable-to-complete, roster actions, shift handoff), client hooks/pages for admin and responder flows, telemetry and push services, mobile Android/iOS platform scaffolding, updated validators and DB rules, and extensive tests and projections. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Responder App
participant Push as Push Service
participant Telemetry as Telemetry Service
participant RTDB as Realtime DB
participant Functions as Cloud Functions
participant Firestore as Firestore
Client->>Push: acquirePushToken()
Push-->>Client: { token } / { error }
Client->>Telemetry: startTracking(dispatchId)
Telemetry->>Client: onLocation(loc)
Client->>RTDB: write responder_locations/{uid} (telemetry)
Client->>Firestore: update responders/{uid}.lastTelemetryAt (merge)
Client->>Functions: httpsCallable(requestBackup{dispatchId, reason, idempotencyKey})
Functions->>Firestore: transaction: validate dispatch & actor, create backup_requests + admin_notifications
Firestore-->>Functions: commit
Functions-->>Client: { status: "requested", backupRequestId }
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
…om numbers from a cryptographically secure source' CodeQL / Creating biased random numbers from a cryptographically secure source Using modulo on a cryptographically secure random number produces biased results. Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
…sitions and availability field fix
![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: 51
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
apps/responder-app/android/gradlew.bat (1)
1-95:⚠️ Potential issue | 🟠 MajorConvert line endings to CRLF for this Windows batch script.
The file currently has LF-only line endings. Batch scripts require CRLF (
\r\n) line endings to function correctly on Windows; LF-only endings can break label/goto parsing on some Windows shells.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/responder-app/android/gradlew.bat` around lines 1 - 95, The gradlew.bat file uses LF-only line endings which can break Windows batch parsing (labels like :execute, :fail, :mainEnd); convert the file to CRLF line endings (\r\n) so the Windows cmd parser handles labels and GOTOs correctly. Open apps/responder-app/android/gradlew.bat and change its line endings to CRLF (use your editor’s “Normalize to CRLF” or run a dos2unix/unix2dos style conversion) and ensure no LF-only lines remain, then commit the updated file.packages/shared-validators/src/dispatches.test.ts (1)
54-67: 🧹 Nitpick | 🔵 TrivialRename the test case for clarity.
The title still says “Phase 3c: en_route + on_scene”, but Line 66 now validates
unable_to_completetoo.Proposed wording update
- it('accepts all valid status values (Phase 3c: en_route + on_scene)', () => { + it('accepts all valid dispatch status values including unable_to_complete', () => {As per coding guidelines, “Code that a tired engineer can understand at 2 AM during an incident — prioritize Correctness > Clarity > Minimal Change > Performance > Cleverness”.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/shared-validators/src/dispatches.test.ts` around lines 54 - 67, Rename the test case title to accurately reflect the set of statuses being validated: replace the string "accepts all valid status values (Phase 3c: en_route + on_scene)" in the test (the it(...) wrapper in dispatches.test.ts) with a clearer description that includes "unable_to_complete" (for example, "accepts all valid status values (includes en_route, on_scene, and unable_to_complete)"). Ensure only the human-readable test name is changed and the test body (the statuses const and assertions) remains unchanged.apps/responder-app/src/hooks/useRegisterFcmToken.ts (1)
22-27:⚠️ Potential issue | 🟡 MinorAdd validation guard for
responderDocPathat the hook boundary.The hook accepts
responderDocPathas a required string parameter but does not validate it before passing to Firebase'sdoc()at line 33. An empty or malformed path would fail at the Firebase layer with a generic error. Add an early guard to fail fast with a deterministic error, following defensive programming practices.Suggested patch
const register = useCallback(async (): Promise<FcmTokenResult> => { + if (!responderDocPath) { + return { token: null, error: 'missing_responder_doc_path' } + } + const result = await acquirePushToken()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/responder-app/src/hooks/useRegisterFcmToken.ts` around lines 22 - 27, Add a defensive guard in the useRegisterFcmToken hook to validate responderDocPath before calling Firebase APIs: inside the exported function useRegisterFcmToken (and/or at start of the register callback) check that responderDocPath is a non-empty string (and optionally matches expected format) and if invalid immediately return a deterministic error result or throw a clear Error instead of calling doc(), so that register (which calls acquirePushToken and later doc()) never forwards a malformed path to Firebase; update any returned FcmTokenResult to reflect this early failure.packages/shared-validators/lib/state-machines/dispatch-states.d.ts (1)
14-17:⚠️ Potential issue | 🟡 MinorDocstring missing new terminal state.
The comment lists terminal states but omits
unable_to_complete. Since this is a generated.d.tsfile, update the source JSDoc indispatch-states.tsto include the new terminal state for consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/shared-validators/lib/state-machines/dispatch-states.d.ts` around lines 14 - 17, Update the JSDoc in the source file dispatch-states.ts to include the new terminal state "unable_to_complete" in the terminal-states list so the generated declaration (dispatch-states.d.ts) matches the runtime/state machine; locate the top-of-file comment block that documents "Terminal states: resolved, declined, timed_out, cancelled, superseded" and add "unable_to_complete" to that list, then regenerate the .d.ts to ensure consistency.apps/admin-desktop/src/hooks/useEligibleResponders.ts (1)
76-80:⚠️ Potential issue | 🔴 CriticalThis listener reads the wrong RTDB path and will filter out all responders.
The responder app (Phase 6) writes to
responder_index/{uid}with metadata only ({ municipalityId, agencyId }), but this hook reads/responder_index/${municipalityId}expecting a municipality-scoped subtree containing shift flags. The path mismatch meansshift[r.uid]is alwaysundefined, so line 86 filters every responder out and returns an empty list.Either update the path to read per-UID responder metadata and match against the responder UID differently, or switch to reading responder
availabilityStatusfrom Firestore (which is already loaded in the responders state) instead of relying on a stale RTDB shift flag.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/admin-desktop/src/hooks/useEligibleResponders.ts` around lines 76 - 80, The RTDB listener in useEligibleResponders is reading the wrong path (/responder_index/${municipalityId}) which yields no shift flags and causes every responder to be filtered out; change the logic to either (A) read per-UID responder metadata by subscribing to /responder_index/{uid} entries and build a map keyed by uid (update the ref/onValue usage and keep using setShift to store Record<uid, {isOnShift: boolean}>), or (B) stop using RTDB shift flags and instead use the already-loaded responders state -> availabilityStatus to determine eligibility (remove the getDatabase/ref/onValue subscription and consult responders[].availabilityStatus where useEligibleResponders filters). Ensure references to getDatabase, ref, onValue, setShift, and useEligibleResponders are updated consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/admin-desktop/src/hooks/useAgencyAssistanceQueue.ts`:
- Around line 53-77: The snapshot handlers in useAgencyAssistanceQueue currently
cast raw Firestore doc.data() into AgencyAssistanceRequest[] and BackupRequest[]
(see the onSnapshot callbacks that build docs and call setRequests) which skips
validation; instead, replace the direct "as" casts with explicit
parsing/normalization: for each snapshot.doc map, run a validator/type-guard
function (e.g., isValidAgencyAssistanceRequest /
normalizeAgencyAssistanceRequest and isValidBackupRequest /
normalizeBackupRequest) that checks required fields, coerces types, supplies
safe defaults, and returns either a typed object or null; filter out or log
invalid docs (do not swallow errors), then setRequests with the cleaned array
and setLoading(false) as before. Ensure parsing code handles optional fields,
logs validation failures, and avoids throwing so the listener remains stable.
- Around line 37-46: When agencyId becomes undefined or changes we must clear
stale queue state and cancel any previous snapshot subscription so old requests
don't show; inside useAgencyAssistanceQueue (where you currently early-return
when !agencyId and call setLoading), also call setRequests([]) and
setBackupRequests([]) and cancel/unsubscribe the previous snapshot listener (the
unsubscribe returned by your snapshot setup) before starting a new one so the
hook resets immediately on logout/tenant switch.
In `@apps/admin-desktop/src/pages/AgencyAssistanceQueuePage.tsx`:
- Around line 72-113: The renderRequestActions function types the status
parameter as a plain string; change it to the actual request status union/type
from your request schema (e.g., RequestStatus or the exact union like 'pending'
| 'accepted' | 'declined') so comparisons like status !== 'pending' are
type-checked; update the function signature (renderRequestActions(reqId: string,
status: RequestStatus)) and any callers to pass the correctly typed status,
keeping existing logic around declineState, handleDecline, handleAccept and
handleDeclineSubmit/Cancel unchanged.
In `@apps/admin-desktop/src/pages/RosterPage.tsx`:
- Around line 155-172: Add a confirmation step before performing destructive
actions: wrap the calls to handleSuspend(r.uid) and handleRevoke(r.uid) behind a
confirmation prompt (e.g., window.confirm or your app's modal) so the action
only proceeds if the user explicitly confirms; keep the existing
disabled={actingUid === r.uid} checks and call handleSuspend or handleRevoke
only after confirmation to prevent accidental clicks on the Suspend and Revoke
buttons.
- Around line 79-90: handleBulkOverride lacks a loading flag so users can click
Apply multiple times; add a bulkLoading state (e.g., const [bulkLoading,
setBulkLoading] = useState(false)) or reuse an existing indicator, set
setBulkLoading(true) before calling bulkAvailabilityOverride and
setBulkLoading(false) in a finally block, keep existing success logic
(setSelectedUids, setBanner) and error handling (setBanner on catch), and then
use bulkLoading to disable the Apply button (disabled={bulkLoading}) so the
button is disabled while handleBulkOverride runs; reference handleBulkOverride,
bulkAvailabilityOverride, setSelectedUids, setBanner and the Apply button.
In `@apps/admin-desktop/src/services/callables.ts`:
- Around line 136-140: The payload for bulkAvailabilityOverride currently types
status as string which permits invalid values; change the status type to the
explicit union 'available' | 'unavailable' | 'off_duty' to match the backend and
the pattern used by suspendResponder/revokeResponder (update the
bulkAvailabilityOverride payload declaration and any related call sites or type
aliases referencing IdempotencyKey if needed so the union is enforced
throughout).
In
`@apps/responder-app/android/app/src/androidTest/java/com/getcapacitor/myapp/ExampleInstrumentedTest.java`:
- Line 24: The package assertion in the test uses a stale expected value; update
the assertEquals call that compares the expected package string to
appContext.getPackageName() so it matches the module's applicationId
("ph.gov.camnorte.bantayog.responder"). Locate the assertion using assertEquals
and appContext.getPackageName() in ExampleInstrumentedTest (the line shown in
the diff) and replace the hardcoded "com.getcapacitor.app" with the correct
applicationId string.
In `@apps/responder-app/android/app/src/main/AndroidManifest.xml`:
- Line 5: Update the AndroidManifest.xml to disable platform backups by changing
the application attribute android:allowBackup from "true" to "false" (modify the
android:allowBackup attribute in the <application> element), and optionally
verify or add android:fullBackupContent if needed for explicit control; rebuild
the app to ensure the manifest change is applied.
In `@apps/responder-app/android/app/src/main/res/xml/file_paths.xml`:
- Around line 3-4: The file_paths.xml currently exposes broad roots via
external-path name="my_images" path="." and cache-path name="my_cache_images"
path=".", which is too permissive; update the mappings to app-scoped directories
by replacing external-path with external-files-path (e.g., external-files-path
name="my_images" path="Pictures" or another app-specific subdir) and tighten
cache-path to a subdirectory (e.g., cache-path name="my_cache_images"
path="images"), ensuring you only expose the app's external-files and app cache
subfolders rather than the device root.
In
`@apps/responder-app/android/app/src/test/java/com/getcapacitor/myapp/ExampleUnitTest.java`:
- Around line 12-17: The file ExampleUnitTest contains the template method
addition_isCorrect which is not relevant to this PR; either delete the
ExampleUnitTest class entirely or replace the addition_isCorrect test with real
unit tests that exercise the responder-app public API introduced in this change
(update ExampleUnitTest.addition_isCorrect to call and assert behavior on the
actual classes/methods you added, e.g., the request handling or response
generation methods), ensuring assertions validate meaningful behavior rather
than 2+2.
In `@apps/responder-app/ios/App/App.xcodeproj/project.pbxproj`:
- Around line 108-113: The project lacks push capability wiring: add a
SystemCapabilities entry for the app target (the TargetAttributes block for
target id 504EC3031FED79650016851F) enabling com.apple.Push, add a
CODE_SIGN_ENTITLEMENTS build setting for the target pointing to an entitlements
file (e.g., AppName/AppName.entitlements), and ensure that entitlements plist
contains the aps-environment key set appropriately ("development" for debug or
"production" for release); update the same changes for the other target blocks
noted (lines 296-333) so APNs registration on device can succeed.
In `@apps/responder-app/src/hooks/useDispatch.ts`:
- Around line 126-132: The hook early-returns when the document doesn't exist
but forgets to clear the loading flag; inside the async fetch in useDispatch
(the block that awaits getDoc(doc(db, 'dispatches', dispatchId)) and checks if
(!snap.exists())), call setLoading(false) before
setDispatch(undefined)/setError(undefined) and return, and ensure any other
early-return or error path in the same async function (including catch/finally
around the getDoc call) also clears loading by calling setLoading(false) so
consumers are never left stuck in the loading state.
In `@apps/responder-app/src/hooks/useMarkDispatchUnableToComplete.ts`:
- Around line 38-42: The catch block in useMarkDispatchUnableToComplete
currently logs and sets state (setError) but does not re-throw, causing the
promise to resolve and callers to treat failures as success; update the catch in
the async function to (after console.error and setError) re-throw the original
error (preserving the Error instance when err instanceof Error, otherwise throw
a new Error(String(err))) so callers receive a rejected promise and can handle
the failure.
- Around line 21-37: The markUnableToComplete flow (in
useMarkDispatchUnableToComplete → markUnableToComplete) currently forwards
dispatchId to the backend without local validation; add a guard to verify
dispatchId is present and non-empty before calling
httpsCallable('markDispatchUnableToComplete'), and if missing setError(new
Error('dispatch_id_required')) and throw that error to stop execution; ensure
this validation occurs after trimming reason and before invoking
awaitFreshAuthToken and the httpsCallable call so malformed input is rejected at
the boundary rather than relying on backend errors.
In `@apps/responder-app/src/hooks/useRequestBackup.ts`:
- Around line 21-37: The request function currently sends dispatchId to the
backend without validating it; add an early guard in request (before calling
awaitFreshAuthToken or httpsCallable) that trims and validates dispatchId is
non-empty/whitespace, and if invalid create and set an Error (e.g.
'dispatch_id_required'), call setError(err), setLoading(false) and throw the
error to fail fast; update any existing error handling paths so the loading
state is cleared when this validation fails and ensure the subsequent call to
httpsCallable('requestBackup') only runs when dispatchId is valid.
- Around line 38-42: The catch block in useRequestBackup's request() currently
sets setError(...) but does not rethrow, causing callers awaiting request() to
believe it succeeded; update the catch to rethrow the original error (after
setting setError) so the promise rejects for callers (preserve err typing: if
err instanceof Error rethrow err, else rethrow new Error(String(err))). Ensure
request() still executes the finally block as before.
In `@apps/responder-app/src/hooks/useResponderAvailability.ts`:
- Around line 62-63: The handler in useResponderAvailability currently only
checks presence of reason but allows whitespace-only strings; update the
validation in the function (the block that checks newStatus !== 'available') to
normalize reason (e.g., reason = reason?.trim()) and require that trimmed reason
is non-empty before proceeding, throwing the same 'reason_required' error if
trimmedReason is empty or undefined; ensure any callers pass the normalized
value and avoid swallowing errors in surrounding try/catch blocks.
In `@apps/responder-app/src/hooks/useResponderTelemetry.ts`:
- Line 10: The APP_VERSION constant is hardcoded to '0.0.0' which breaks
telemetry; replace the hardcoded value by reading the real version from
build/runtime config (e.g., process.env.APP_VERSION or an injected config value)
where APP_VERSION is defined, e.g., in the useResponderTelemetry module;
implement fallback logic (unknown or empty) and ensure the module exports/uses
the resolved value so telemetry payloads carry the injected version instead of
the literal '0.0.0'.
- Line 38: The low-battery branch in useResponderTelemetry currently halves the
telemetry interval (return batteryLow ? Math.max(base / 2, 5_000) : base), which
increases write frequency; change the low-battery behavior to lengthen the
interval instead (e.g., multiply base by 2 and cap with Math.max or Math.min as
appropriate) so that when batteryLow is true the function returns a larger
interval than base; update the expression in useResponderTelemetry to something
like batteryLow ? Math.min(base * 2, SOME_UPPER_LIMIT) : base (or Math.max(base
* 2, MIN_INTERVAL)) to ensure safe bounds.
In `@apps/responder-app/src/hooks/useSubmitResponderWitnessedReport.ts`:
- Around line 56-60: In useSubmitResponderWitnessedReport's catch block, don't
swallow failures: after logging and normalizing the error into state via
setError (keeping the existing console.error and the instanceof Error branch),
rethrow the normalized Error so the promise rejects and callers/UI can react to
the failure; ensure you throw the same Error instance you passed to setError (or
a newly constructed Error(String(err)) when err is not an Error).
- Around line 25-33: In the submit function of
useSubmitResponderWitnessedReport, validate the external inputs (dispatchId
argument and payload.reportType, payload.description, payload.severity) up front
before calling geolocation or the cloud callable; ensure dispatchId is present
and well-formed (e.g., non-empty string/expected pattern) and required payload
fields are non-empty and within allowed values, and if validation fails setError
with a descriptive message and return early; also remove any empty catch blocks
in this hook (and in the related invocation path around lines 50-55) so that
thrown errors are logged/propagated to setError instead of being swallowed.
In `@apps/responder-app/src/hooks/useTriggerSOS.ts`:
- Around line 25-31: The catch block in the useTriggerSOS hook swallows errors
after normalizing them, causing trigger() to resolve even on failure; inside the
catch for trigger (in useTriggerSOS), after calling setError(...) and before the
finally block/setLoading(false), rethrow the normalized Error (the same instance
you pass to setError) so callers awaiting trigger() receive the rejection.
Ensure the throw happens after state updates so setError and setLoading
semantics remain unchanged.
In `@apps/responder-app/src/pages/BackupRequestPage.tsx`:
- Around line 20-22: The empty catch in the BackupRequestPage component is
swallowing errors; update the catch to accept the error (catch (err)) and handle
it explicitly: log the full error via console.error or processLogger, set a
local UI error state (add/use a state like error / setError in
BackupRequestPage) with a user-friendly message and the error detail, and/or
rethrow or pass the error to the existing hook error handler so failures surface
to the UI. Ensure you reference the same try block in BackupRequestPage and wire
the new error state into the component's rendering to show an error message to
the user.
- Around line 6-9: The page currently calls useRequestBackup(id ?? '') and later
navigates/submits even when id is missing; add an explicit guard for a missing
route id: if (!id) immediately return an "invalid route" UI or navigate to a
safe error page and prevent any submit/navigation logic from running. Keep the
useRequestBackup usage intact but update handlers (the submit/navigation code
that calls navigate('/dispatches/...') and any functions referencing request) to
check id first (e.g., return early or disable the submit button) so no empty IDs
are propagated; reference useParams id, useRequestBackup, navigate and the
submit/navigation handler(s) when making the changes.
In `@apps/responder-app/src/pages/DispatchListPage.tsx`:
- Around line 58-85: Extract the handoff logic from handleInitiateHandoff into a
new hook named useInitiateResponderHandoff that encapsulates the httpsCallable
call to initiateResponderHandoff, input validation, loading/error state, and
idempotencyKey generation; replace the inline implementation in DispatchListPage
by calling the hook (e.g., const { initiateHandoff, loading, error, reset } =
useInitiateResponderHandoff) and wire its methods to setHandoffTarget,
setHandoffReason, setShowHandoff as appropriate. Ensure the hook exports a
function (initiateHandoff or similar) that accepts { toUid, reason } and returns
a promise, manages internal loading/error state, and uses
httpsCallable(functions, 'initiateResponderHandoff') and crypto.randomUUID()
internally so unit tests can mock the hook instead of httpsCallable.
- Around line 44-56: In handleStatusChange, validate that reason.trim() is
non-empty when selectedStatus !== 'available' before calling setAvailability; if
trimmed reason is empty, setStatusError to a user-facing message and return
without calling setAvailability or changing setStatusSaving, ensuring you still
setStatusSaving(false) as needed; use the existing selectedStatus, reason,
setStatusError, setStatusSaving, setReason and call setReason('') only after a
successful setAvailability.
In `@apps/responder-app/src/pages/ResponderWitnessReportPage.tsx`:
- Around line 5-17: Replace the local REPORT_TYPES array with a single import
from the shared validators/schema so values stay in sync: remove the
REPORT_TYPES constant and import the canonical list exported by the shared
module (e.g., from shared-validators or the module that exports
reportDocSchema), then use that imported symbol wherever REPORT_TYPES was
referenced in ResponderWitnessReportPage (ensuring the imported symbol has the
same readonly/union typing expected by the component); if the shared module only
exports reportDocSchema, derive the allowed values from reportDocSchema and
export them from the shared module so this file can import a stable REPORT_TYPES
list.
- Around line 29-43: handleSubmit currently returns silently when required
fields (reportType, description, severity) are missing; add a validation error
state (e.g., validationError) and set it when those fields are invalid instead
of returning silently, then render validationError alongside the existing error
in the UI; update handleSubmit to set validationError (clear it on successful
submit or on input change) and keep using submit(...) and navigate(...) as
before so users see explicit feedback when reportType, description, or severity
are missing.
In `@apps/responder-app/src/pages/SosPage.tsx`:
- Around line 5-7: Guard against a missing route param by checking the id from
useParams before calling useTriggerSOS: if id is undefined or empty, either
navigate away (using navigate) to a safe route or render an error/placeholder
early in SosPage instead of passing an empty string into useTriggerSOS; update
the component to return the error/redirect state before invoking or relying on
trigger, loading, or error so useTriggerSOS always receives a valid id.
In `@apps/responder-app/src/routes.tsx`:
- Around line 26-28: Logs currently print raw foreground push payloads from
subscribeForegroundPush (payload) to the console via unsubscribeForeground,
which can leak sensitive data; change this to either: (a) wrap the console.warn
behind an environment/dev-mode check (e.g., only log when process.env.NODE_ENV
=== 'development' or a feature flag), or (b) sanitize the payload before logging
by extracting and logging only a minimal marker (e.g., payload.type or
payload.id and a redacted summary) instead of the entire object; update the
subscribeForegroundPush callback accordingly to use the chosen guard/sanitizer
and ensure unsubscribeForeground remains assigned.
In `@apps/responder-app/src/services/push-client.ts`:
- Around line 29-48: The acquirePushToken promise never removes the native
listeners and doesn't handle register() failures; update acquirePushToken to
store the listener handles returned by PushNotifications.addListener for
'registration' and 'registrationError', call their remove/unsubscribe methods
after resolveOnce runs to avoid leaks (mirror the pattern used in
subscribeForegroundPush and subscribeNotificationTap), and handle
PushNotifications.register() rejections by wiring its returned promise to reject
or timeout the acquirePushToken promise so it cannot remain pending if
register() fails.
In `@apps/responder-app/src/services/telemetry-client.ts`:
- Around line 19-33: The current implementation uses a single mutable
currentCallback so when startTracking is called twice the first subscriber is
lost; change to a subscriber collection (e.g., Set<(loc:
TelemetryLocation)=>void>) instead of currentCallback and have startTracking add
the provided onLocation to that Set (increment refCount and only start the
underlying watcher when the Set transitions from 0→1). Modify stopTracking to
remove the specific onLocation from the Set (or return an unsubscribe function
from startTracking that removes that callback) and only stop/clear watcherId and
webWatchId when the Set becomes empty (and decrement refCount). Update any logic
that references currentCallback (including the watcher callbacks around lines
91-96) to iterate over the subscriber Set and invoke each subscriber.
In `@functions/src/__tests__/callables/mark-dispatch-unable-to-complete.test.ts`:
- Around line 59-65: seedDispatchActive currently takes five positional
parameters which is brittle; refactor it to accept a single options object
(e.g., { env, dispatchId, reportId, responderUid, status }) or split into
smaller helpers so callers pass one argument and named properties; update
seedDispatchActive's signature and all test call sites in this file to
destructure the object (or use the new helpers) and ensure types/interfaces are
adjusted (RulesTestEnvironment and status typing) so compilation and behavior
remain identical.
In `@functions/src/__tests__/callables/request-backup.test.ts`:
- Around line 56-62: Refactor the seedDispatchActive helper to accept a single
typed object instead of five separate parameters: replace the signature of
seedDispatchActive(env, dispatchId, reportId, responderUid, status) with
seedDispatchActive({ env, dispatchId, reportId, responderUid, status }:
SeedDispatchActiveOpts) where SeedDispatchActiveOpts is a small interface/type;
update all call sites in tests to pass an object literal and adjust internal
references accordingly; keep function behavior unchanged and export/type the new
option type if reused elsewhere (refer to symbol seedDispatchActive and
introduce SeedDispatchActiveOpts).
In `@functions/src/__tests__/callables/submit-responder-witnessed-report.test.ts`:
- Around line 59-65: Refactor the seedDispatchActive function to accept a single
parameter object (e.g., params: { env: RulesTestEnvironment; dispatchId: string;
reportId: string; responderUid: string; status: string }) instead of five
separate parameters; update the function signature for seedDispatchActive and
destructure the fields inside the function body, adjust the type annotation to
Promise<void>, and then update all call sites in tests to pass a single object
literal with the same property names (env, dispatchId, reportId, responderUid,
status) so existing behavior remains unchanged.
In `@functions/src/__tests__/scheduled/project-responder-locations.test.ts`:
- Around line 12-18: The test currently creates a RulesTestEnvironment via
initializeTestEnvironment in beforeEach but never calls its cleanup method,
leaking resources; add an afterEach (or afterAll) that checks the created
testEnv and awaits testEnv.cleanup() (and optionally sets testEnv = undefined)
to ensure initializeTestEnvironment's resources are released; reference the
beforeEach/testEnv/initializeTestEnvironment usage and add the corresponding
await testEnv.cleanup() in an afterEach block to fix the leak.
In `@functions/src/callables/mark-dispatch-unable-to-complete.ts`:
- Around line 81-98: The transaction assumes reports/{id} exists and calls
tx.update(reportRef) even when reportSnap is missing; instead, after reading
reportSnap (the currentReportStatus check) validate existence and if missing
throw a domain error (e.g., NOT_FOUND or FAILED_PRECONDITION) before performing
any tx.update on reportRef so the transaction aborts with a stable, debuggable
error; update the logic around reportRef/reportSnap/currentReportStatus in
mark-dispatch-unable-to-complete to guard the tx.update(reportRef, ...) call and
return/throw early with the chosen error code and descriptive message.
In `@functions/src/callables/request-backup.ts`:
- Around line 81-108: The transaction currently writes to backup_requests and
admin_notifications but lacks an immutable event record; add a third tx.set to
write a durable event into a dispatch_events (or backup_request_events)
collection within the same transaction. Create a new doc (e.g.,
db.collection('dispatch_events').doc()) and store fields such as eventType:
'backup_requested' (or 'backup_request_created'), dispatchId, backupRequestId,
actorUid: actor.uid, agencyId/municipalityId from dispatch.assignedTo, reason,
createdAt: nowMs, and schemaVersion so the event trail can be reconstructed;
ensure you use the same tx variable and include the same identifiers
(backupRequestId, dispatchId, nowMs) as in the existing tx.set calls.
In `@functions/src/callables/responder-roster.ts`:
- Around line 123-152: The admin callables (suspendResponder, revokeResponder,
bulkAvailabilityOverride) currently only verify role via requireAuth but do not
block admins whose accounts are not active; add an account-status guard
immediately after obtaining the actor in each callable: check
actor.accountStatus (or actor.claims.accountStatus) equals 'active' and if not
throw an HttpsError('permission-denied', 'admin account not active') so
suspended/revoked admins cannot perform roster mutations; apply this same check
in the suspendResponder, revokeResponder and bulkAvailabilityOverride handlers
before any state mutation or core call.
- Around line 193-245: bulkAvailabilityOverrideCore currently ignores the
required idempotency key, so retries cause duplicate work and consume
rate-limit. Modify bulkAvailabilityOverrideCore to read deps.idempotencyKey and
wrap the core update logic inside the existing withIdempotency helper (e.g.,
return withIdempotency(db, actor.uid, deps.idempotencyKey, async () => {
...current body... })), ensuring the checkRateLimit, batch updates, logging, and
returned { updated } are executed only once per idempotency key; reference
bulkAvailabilityOverrideCore, withIdempotency, deps.idempotencyKey, and
checkRateLimit when making the change.
In `@functions/src/callables/responder-shift-handoff.ts`:
- Around line 107-109: The code currently checks fromData.isActive and
toData.isActive; replace this boolean check with the canonical accountStatus
check used elsewhere by verifying fromData.accountStatus === 'ACTIVE' and
toData.accountStatus === 'ACTIVE' inside the responder-shift-handoff flow (keep
the same return on failure), ensuring you reference fromData, toData, and
accountStatus rather than isActive so suspended or missing-boolean cases align
with roster/phase-6 logic.
In `@functions/src/scheduled/project-responder-locations.ts`:
- Around line 111-126: The current TTL cleanup scans the entire
shared_projection tree (projectionSnap / projections) which won't scale;
instead, add per-entry expiry at write time (set an expiresAt or lastSeenAt when
creating/updating a ProjectionEntry in the code paths that call write
operations) and change the cleanup in project-responder-locations.ts to only
scan municipalities that were touched recently (e.g., accept or read a
touchedMunicipalities set or derive it from writeOps context) or delete by
checking expiresAt on entries for those specific muniIds rather than iterating
over all entries; update the code that writes responder entries to set
TTL_MS-based expiresAt and modify the cleanup loop (which currently iterates
projectionSnap/projections and uses TTL_MS and writeOps) to only operate on the
touched muniIds or remove entries whose expiresAt <= now.
- Around line 89-94: The code currently sets lastSeenAt to now which refreshes
TTL incorrectly; change it to use the telemetry timestamp from the location
object (e.g., loc.timestamp or loc.ts) when assigning lastSeenAt in the
byMunicipality[...] object and use that same telemetry timestamp when computing
freshness so stale positions evict correctly; if the telemetry timestamp is
missing or invalid, fall back to now to avoid breaking existing records. Ensure
you update the assignment in the block that writes byMunicipality[muniId][uid]
and any related freshness calculation that relies on now so both use the
telemetry timestamp consistently.
In `@infra/firebase/database.rules.json`:
- Line 7: The current ".validate" only checks key presence; update the
validation expression for the telemetry node referenced by ".validate" to
enforce types and ranges: require newData.child('capturedAt').isNumber() (or
valid timestamp range), newData.child('lat').isNumber() and
newData.child('lat').val() between -90 and 90, newData.child('lng').isNumber()
and newData.child('lng').val() between -180 and 180,
newData.child('accuracy').isNumber() and >= 0,
newData.child('batteryPct').isNumber() and between 0 and 100,
newData.child('appVersion').isString(),
newData.child('telemetryStatus').isString() (or enum), and restrict
newData.child('motionState') to allowed string values (or a constrained object
shape) using explicit equality checks or matches; combine these boolean checks
with && in the existing ".validate" expression so malformed values are rejected
rather than only requiring keys.
In `@infra/firebase/firestore.rules`:
- Around line 248-250: The update rule currently only restricts which keys may
change but not their types/values; update the condition that uses uid() == rUid
and
request.resource.data.diff(resource.data).affectedKeys().hasOnly(['availabilityStatus','availabilityReason','updatedAt'])
to also validate each field: check availabilityStatus is one of the allowed enum
strings (e.g., request.resource.data.availabilityStatus in
['AVAILABLE','UNAVAILABLE',...]), ensure
request.resource.data.availabilityReason is a string (and optionally
length-limited), and ensure request.resource.data.updatedAt is a timestamp and
not older than the current resource.data.updatedAt (e.g.,
request.resource.data.updatedAt is timestamp && request.resource.data.updatedAt
>= resource.data.updatedAt); keep the existing hasOnly check and combine these
predicates with logical AND so only well-typed, allowed-value updates are
permitted for availabilityStatus, availabilityReason, and updatedAt.
In `@packages/shared-validators/lib/index.d.ts`:
- Around line 14-15: The barrel file is missing exports for the new responder
handoff validator and type; add exports for responderShiftHandoffDocSchema and
ResponderShiftHandoffDoc to the root declarations so the public API surfaces are
consistent—specifically, export the schema (responderShiftHandoffDocSchema) and
the type (ResponderShiftHandoffDoc) from the same module that defines them (the
responders module) alongside the existing responder exports.
In `@packages/shared-validators/lib/responders.js`:
- Around line 20-32: The responderTelemetryPayloadSchema currently allows
fractional/negative timestamps, out-of-range coordinates, and negative accuracy;
update the schema (responderTelemetryPayloadSchema) to enforce integer,
non-negative timestamps (capturedAt and optional receivedAt as
z.number().int().nonnegative()), constrain lat and lng to realistic bounds (lat
between -90 and 90, lng between -180 and 180) and require accuracy to be
non-negative and bounded if desired (e.g., z.number().nonnegative()), keep
batteryPct min/max, and then regenerate the TypeScript declarations so the
tightened contract becomes the new shared boundary.
In `@packages/shared-validators/lib/state-machines/dispatch-states.js`:
- Around line 32-41: Update the lifecycle/doc comment above the dispatch state
transition table to list "unable_to_complete" as a terminal state to match the
transitions array; locate the comment that enumerates terminal states for the
dispatch state machine (above the arrays containing accepted, acknowledged,
en_route, on_scene, resolved, etc.) and add "unable_to_complete" to that list so
documentation and the state definitions (e.g., accepted, acknowledged, en_route,
on_scene, resolved, declined, timed_out, cancelled, superseded,
unable_to_complete) are consistent.
In `@packages/shared-validators/src/coordination.ts`:
- Line 151: The `reason` schema currently uses z.string().max(1000) and allows
empty strings; update the `reason` field in the coordination schema to enforce a
non-empty value by adding a minimum length check (e.g., `.min(1).max(1000)`),
matching the validation style used for `fromUid`, `toUid`, `agencyId`, and
`municipalityId`.
In `@packages/shared-validators/src/responders.ts`:
- Around line 22-34: Tighten responderTelemetryPayloadSchema by enforcing
realistic bounds and integer timestamps: make capturedAt and receivedAt use
z.number().int().nonnegative(), constrain lat with z.number().min(-90).max(90),
constrain lng with z.number().min(-180).max(180), and require accuracy as
z.number().nonnegative() (optionally add a reasonable max if desired); keep
batteryPct, motionState, telemetryStatus and appVersion as is and preserve
.strict() so invalid shapes still fail validation.
---
Outside diff comments:
In `@apps/admin-desktop/src/hooks/useEligibleResponders.ts`:
- Around line 76-80: The RTDB listener in useEligibleResponders is reading the
wrong path (/responder_index/${municipalityId}) which yields no shift flags and
causes every responder to be filtered out; change the logic to either (A) read
per-UID responder metadata by subscribing to /responder_index/{uid} entries and
build a map keyed by uid (update the ref/onValue usage and keep using setShift
to store Record<uid, {isOnShift: boolean}>), or (B) stop using RTDB shift flags
and instead use the already-loaded responders state -> availabilityStatus to
determine eligibility (remove the getDatabase/ref/onValue subscription and
consult responders[].availabilityStatus where useEligibleResponders filters).
Ensure references to getDatabase, ref, onValue, setShift, and
useEligibleResponders are updated consistently.
In `@apps/responder-app/android/gradlew.bat`:
- Around line 1-95: The gradlew.bat file uses LF-only line endings which can
break Windows batch parsing (labels like :execute, :fail, :mainEnd); convert the
file to CRLF line endings (\r\n) so the Windows cmd parser handles labels and
GOTOs correctly. Open apps/responder-app/android/gradlew.bat and change its line
endings to CRLF (use your editor’s “Normalize to CRLF” or run a
dos2unix/unix2dos style conversion) and ensure no LF-only lines remain, then
commit the updated file.
In `@apps/responder-app/src/hooks/useRegisterFcmToken.ts`:
- Around line 22-27: Add a defensive guard in the useRegisterFcmToken hook to
validate responderDocPath before calling Firebase APIs: inside the exported
function useRegisterFcmToken (and/or at start of the register callback) check
that responderDocPath is a non-empty string (and optionally matches expected
format) and if invalid immediately return a deterministic error result or throw
a clear Error instead of calling doc(), so that register (which calls
acquirePushToken and later doc()) never forwards a malformed path to Firebase;
update any returned FcmTokenResult to reflect this early failure.
In `@packages/shared-validators/lib/state-machines/dispatch-states.d.ts`:
- Around line 14-17: Update the JSDoc in the source file dispatch-states.ts to
include the new terminal state "unable_to_complete" in the terminal-states list
so the generated declaration (dispatch-states.d.ts) matches the runtime/state
machine; locate the top-of-file comment block that documents "Terminal states:
resolved, declined, timed_out, cancelled, superseded" and add
"unable_to_complete" to that list, then regenerate the .d.ts to ensure
consistency.
In `@packages/shared-validators/src/dispatches.test.ts`:
- Around line 54-67: Rename the test case title to accurately reflect the set of
statuses being validated: replace the string "accepts all valid status values
(Phase 3c: en_route + on_scene)" in the test (the it(...) wrapper in
dispatches.test.ts) with a clearer description that includes
"unable_to_complete" (for example, "accepts all valid status values (includes
en_route, on_scene, and unable_to_complete)"). Ensure only the human-readable
test name is changed and the test body (the statuses const and assertions)
remains unchanged.
🪄 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: 9ea6108b-05cc-4b41-b338-8d945e606bc2
⛔ Files ignored due to path filters (48)
apps/responder-app/android/app/src/main/res/drawable-land-hdpi/splash.pngis excluded by!**/*.pngapps/responder-app/android/app/src/main/res/drawable-land-mdpi/splash.pngis excluded by!**/*.pngapps/responder-app/android/app/src/main/res/drawable-land-xhdpi/splash.pngis excluded by!**/*.pngapps/responder-app/android/app/src/main/res/drawable-land-xxhdpi/splash.pngis excluded by!**/*.pngapps/responder-app/android/app/src/main/res/drawable-land-xxxhdpi/splash.pngis excluded by!**/*.pngapps/responder-app/android/app/src/main/res/drawable-port-hdpi/splash.pngis excluded by!**/*.pngapps/responder-app/android/app/src/main/res/drawable-port-mdpi/splash.pngis excluded by!**/*.pngapps/responder-app/android/app/src/main/res/drawable-port-xhdpi/splash.pngis excluded by!**/*.pngapps/responder-app/android/app/src/main/res/drawable-port-xxhdpi/splash.pngis excluded by!**/*.pngapps/responder-app/android/app/src/main/res/drawable-port-xxxhdpi/splash.pngis excluded by!**/*.pngapps/responder-app/android/app/src/main/res/drawable/splash.pngis excluded by!**/*.pngapps/responder-app/android/app/src/main/res/mipmap-hdpi/ic_launcher.pngis excluded by!**/*.pngapps/responder-app/android/app/src/main/res/mipmap-hdpi/ic_launcher_foreground.pngis excluded by!**/*.pngapps/responder-app/android/app/src/main/res/mipmap-hdpi/ic_launcher_round.pngis excluded by!**/*.pngapps/responder-app/android/app/src/main/res/mipmap-mdpi/ic_launcher.pngis excluded by!**/*.pngapps/responder-app/android/app/src/main/res/mipmap-mdpi/ic_launcher_foreground.pngis excluded by!**/*.pngapps/responder-app/android/app/src/main/res/mipmap-mdpi/ic_launcher_round.pngis excluded by!**/*.pngapps/responder-app/android/app/src/main/res/mipmap-xhdpi/ic_launcher.pngis excluded by!**/*.pngapps/responder-app/android/app/src/main/res/mipmap-xhdpi/ic_launcher_foreground.pngis excluded by!**/*.pngapps/responder-app/android/app/src/main/res/mipmap-xhdpi/ic_launcher_round.pngis excluded by!**/*.pngapps/responder-app/android/app/src/main/res/mipmap-xxhdpi/ic_launcher.pngis excluded by!**/*.pngapps/responder-app/android/app/src/main/res/mipmap-xxhdpi/ic_launcher_foreground.pngis excluded by!**/*.pngapps/responder-app/android/app/src/main/res/mipmap-xxhdpi/ic_launcher_round.pngis excluded by!**/*.pngapps/responder-app/android/app/src/main/res/mipmap-xxxhdpi/ic_launcher.pngis excluded by!**/*.pngapps/responder-app/android/app/src/main/res/mipmap-xxxhdpi/ic_launcher_foreground.pngis excluded by!**/*.pngapps/responder-app/android/app/src/main/res/mipmap-xxxhdpi/ic_launcher_round.pngis excluded by!**/*.pngapps/responder-app/android/gradle/wrapper/gradle-wrapper.jaris excluded by!**/*.jarapps/responder-app/ios/App/App/Assets.xcassets/AppIcon.appiconset/AppIcon-512@2x.pngis excluded by!**/*.pngapps/responder-app/ios/App/App/Assets.xcassets/Splash.imageset/splash-2732x2732-1.pngis excluded by!**/*.pngapps/responder-app/ios/App/App/Assets.xcassets/Splash.imageset/splash-2732x2732-2.pngis excluded by!**/*.pngapps/responder-app/ios/App/App/Assets.xcassets/Splash.imageset/splash-2732x2732.pngis excluded by!**/*.pngpackages/shared-types/lib/enums.d.ts.mapis excluded by!**/*.mappackages/shared-validators/lib/coordination.d.ts.mapis excluded by!**/*.mappackages/shared-validators/lib/coordination.js.mapis excluded by!**/*.mappackages/shared-validators/lib/coordination.test.js.mapis excluded by!**/*.mappackages/shared-validators/lib/dispatches.d.ts.mapis excluded by!**/*.mappackages/shared-validators/lib/dispatches.js.mapis excluded by!**/*.mappackages/shared-validators/lib/dispatches.test.js.mapis excluded by!**/*.mappackages/shared-validators/lib/events.d.ts.mapis excluded by!**/*.mappackages/shared-validators/lib/index.d.ts.mapis excluded by!**/*.mappackages/shared-validators/lib/index.js.mapis excluded by!**/*.mappackages/shared-validators/lib/responders.d.ts.mapis excluded by!**/*.mappackages/shared-validators/lib/responders.js.mapis excluded by!**/*.mappackages/shared-validators/lib/responders.test.js.mapis excluded by!**/*.mappackages/shared-validators/lib/state-machines.test.js.mapis excluded by!**/*.mappackages/shared-validators/lib/state-machines/dispatch-states.d.ts.mapis excluded by!**/*.mappackages/shared-validators/lib/state-machines/dispatch-states.js.mapis excluded by!**/*.mappnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (119)
.gitignoreapps/admin-desktop/src/hooks/useAgencyAssistanceQueue.tsapps/admin-desktop/src/hooks/useEligibleResponders.tsapps/admin-desktop/src/hooks/useRosterManagement.tsapps/admin-desktop/src/pages/AgencyAssistanceQueuePage.test.tsxapps/admin-desktop/src/pages/AgencyAssistanceQueuePage.tsxapps/admin-desktop/src/pages/DispatchModal.tsxapps/admin-desktop/src/pages/RosterPage.tsxapps/admin-desktop/src/routes.tsxapps/admin-desktop/src/services/callables.tsapps/responder-app/android/.gitignoreapps/responder-app/android/app/.gitignoreapps/responder-app/android/app/build.gradleapps/responder-app/android/app/capacitor.build.gradleapps/responder-app/android/app/proguard-rules.proapps/responder-app/android/app/src/androidTest/java/com/getcapacitor/myapp/ExampleInstrumentedTest.javaapps/responder-app/android/app/src/main/AndroidManifest.xmlapps/responder-app/android/app/src/main/java/ph/gov/camnorte/bantayog/responder/MainActivity.javaapps/responder-app/android/app/src/main/res/drawable-v24/ic_launcher_foreground.xmlapps/responder-app/android/app/src/main/res/drawable/ic_launcher_background.xmlapps/responder-app/android/app/src/main/res/layout/activity_main.xmlapps/responder-app/android/app/src/main/res/mipmap-anydpi-v26/ic_launcher.xmlapps/responder-app/android/app/src/main/res/mipmap-anydpi-v26/ic_launcher_round.xmlapps/responder-app/android/app/src/main/res/values/ic_launcher_background.xmlapps/responder-app/android/app/src/main/res/values/strings.xmlapps/responder-app/android/app/src/main/res/values/styles.xmlapps/responder-app/android/app/src/main/res/xml/file_paths.xmlapps/responder-app/android/app/src/test/java/com/getcapacitor/myapp/ExampleUnitTest.javaapps/responder-app/android/build.gradleapps/responder-app/android/capacitor.settings.gradleapps/responder-app/android/gradle.propertiesapps/responder-app/android/gradle/wrapper/gradle-wrapper.propertiesapps/responder-app/android/gradlewapps/responder-app/android/gradlew.batapps/responder-app/android/settings.gradleapps/responder-app/android/variables.gradleapps/responder-app/capacitor.config.tsapps/responder-app/ios/.gitignoreapps/responder-app/ios/App/App.xcodeproj/project.pbxprojapps/responder-app/ios/App/App.xcodeproj/project.xcworkspace/xcshareddata/IDEWorkspaceChecks.plistapps/responder-app/ios/App/App/AppDelegate.swiftapps/responder-app/ios/App/App/Assets.xcassets/AppIcon.appiconset/Contents.jsonapps/responder-app/ios/App/App/Assets.xcassets/Contents.jsonapps/responder-app/ios/App/App/Assets.xcassets/Splash.imageset/Contents.jsonapps/responder-app/ios/App/App/Base.lproj/LaunchScreen.storyboardapps/responder-app/ios/App/App/Base.lproj/Main.storyboardapps/responder-app/ios/App/App/Info.plistapps/responder-app/ios/App/CapApp-SPM/.gitignoreapps/responder-app/ios/App/CapApp-SPM/Package.swiftapps/responder-app/ios/App/CapApp-SPM/README.mdapps/responder-app/ios/App/CapApp-SPM/Sources/CapApp-SPM/CapApp-SPM.swiftapps/responder-app/ios/debug.xcconfigapps/responder-app/package.jsonapps/responder-app/src/App.tsxapps/responder-app/src/hooks/useDispatch.tsapps/responder-app/src/hooks/useMarkDispatchUnableToComplete.tsapps/responder-app/src/hooks/useRegisterFcmToken.tsapps/responder-app/src/hooks/useRequestBackup.tsapps/responder-app/src/hooks/useResponderAvailability.tsapps/responder-app/src/hooks/useResponderTelemetry.tsapps/responder-app/src/hooks/useSubmitResponderWitnessedReport.tsapps/responder-app/src/hooks/useTriggerSOS.tsapps/responder-app/src/pages/BackupRequestPage.tsxapps/responder-app/src/pages/DispatchDetailPage.tsxapps/responder-app/src/pages/DispatchListPage.tsxapps/responder-app/src/pages/ResponderWitnessReportPage.tsxapps/responder-app/src/pages/SosPage.tsxapps/responder-app/src/routes.tsxapps/responder-app/src/services/push-client.tsapps/responder-app/src/services/telemetry-client.tsdocs/learnings.mddocs/progress.mde2e-tests/specs/responder-phase-6.spec.tsfunctions/src/__tests__/callables/accept-dispatch.test.tsfunctions/src/__tests__/callables/advance-dispatch.test.tsfunctions/src/__tests__/callables/mark-dispatch-unable-to-complete.test.tsfunctions/src/__tests__/callables/request-backup.test.tsfunctions/src/__tests__/callables/responder-shift-handoff.test.tsfunctions/src/__tests__/callables/submit-responder-witnessed-report.test.tsfunctions/src/__tests__/callables/trigger-sos.test.tsfunctions/src/__tests__/rtdb.rules.test.tsfunctions/src/__tests__/scheduled/project-responder-locations.test.tsfunctions/src/callables/mark-dispatch-unable-to-complete.tsfunctions/src/callables/request-backup.tsfunctions/src/callables/responder-roster.tsfunctions/src/callables/responder-shift-handoff.tsfunctions/src/callables/submit-responder-witnessed-report.tsfunctions/src/callables/trigger-sos.tsfunctions/src/index.tsfunctions/src/scheduled/project-responder-locations.tsfunctions/src/services/rate-limit.tsinfra/firebase/database.rules.jsoninfra/firebase/firestore.rulespackages/shared-types/lib/enums.d.tspackages/shared-types/src/enums.tspackages/shared-validators/lib/coordination.d.tspackages/shared-validators/lib/coordination.jspackages/shared-validators/lib/coordination.test.jspackages/shared-validators/lib/dispatches.d.tspackages/shared-validators/lib/dispatches.jspackages/shared-validators/lib/dispatches.test.jspackages/shared-validators/lib/events.d.tspackages/shared-validators/lib/index.d.tspackages/shared-validators/lib/index.jspackages/shared-validators/lib/responders.d.tspackages/shared-validators/lib/responders.jspackages/shared-validators/lib/responders.test.jspackages/shared-validators/lib/state-machines.test.jspackages/shared-validators/lib/state-machines/dispatch-states.d.tspackages/shared-validators/lib/state-machines/dispatch-states.jspackages/shared-validators/src/coordination.test.tspackages/shared-validators/src/coordination.tspackages/shared-validators/src/dispatches.test.tspackages/shared-validators/src/dispatches.tspackages/shared-validators/src/index.tspackages/shared-validators/src/responders.test.tspackages/shared-validators/src/responders.tspackages/shared-validators/src/state-machines.test.tspackages/shared-validators/src/state-machines/dispatch-states.ts
Comment on lines
+37
to
+46
| if (!agencyId) { | ||
| queueMicrotask(() => { | ||
| setLoading(false) | ||
| }) | ||
| return | ||
| } | ||
|
|
||
| queueMicrotask(() => { | ||
| setLoading(true) | ||
| }) |
Contributor
There was a problem hiding this comment.
Clear the previous queue state when the agency context changes.
If agencyId becomes undefined or switches from one agency to another, this hook keeps returning the old agency's requests/backupRequests until another snapshot arrives. That leaks stale cross-agency data into the UI during logout or tenant switches.
♻️ Proposed fix
if (!agencyId) {
- queueMicrotask(() => {
- setLoading(false)
- })
+ setRequests([])
+ setBackupRequests([])
+ setLoading(false)
return
}
- queueMicrotask(() => {
- setLoading(true)
- })
+ setRequests([])
+ setBackupRequests([])
+ setLoading(true)📝 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
| if (!agencyId) { | |
| queueMicrotask(() => { | |
| setLoading(false) | |
| }) | |
| return | |
| } | |
| queueMicrotask(() => { | |
| setLoading(true) | |
| }) | |
| if (!agencyId) { | |
| setRequests([]) | |
| setBackupRequests([]) | |
| setLoading(false) | |
| return | |
| } | |
| setRequests([]) | |
| setBackupRequests([]) | |
| setLoading(true) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/admin-desktop/src/hooks/useAgencyAssistanceQueue.ts` around lines 37 -
46, When agencyId becomes undefined or changes we must clear stale queue state
and cancel any previous snapshot subscription so old requests don't show; inside
useAgencyAssistanceQueue (where you currently early-return when !agencyId and
call setLoading), also call setRequests([]) and setBackupRequests([]) and
cancel/unsubscribe the previous snapshot listener (the unsubscribe returned by
your snapshot setup) before starting a new one so the hook resets immediately on
logout/tenant switch.
Comment on lines
+107
to
+109
| if (fromData.isActive !== true || toData.isActive !== true) { | ||
| return { success: false as const, errorCode: 'failed-precondition' } | ||
| } |
Contributor
There was a problem hiding this comment.
Check accountStatus instead of isActive.
The rest of the phase-6 flows and roster mutations key off accountStatus. Reading a separate isActive field here will reject valid responders if that boolean is absent, and it can let suspended users pass if the two fields drift.
Proposed fix
- if (fromData.isActive !== true || toData.isActive !== true) {
+ if (fromData.accountStatus !== 'active' || toData.accountStatus !== 'active') {
return { success: false as const, errorCode: 'failed-precondition' }
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@functions/src/callables/responder-shift-handoff.ts` around lines 107 - 109,
The code currently checks fromData.isActive and toData.isActive; replace this
boolean check with the canonical accountStatus check used elsewhere by verifying
fromData.accountStatus === 'ACTIVE' and toData.accountStatus === 'ACTIVE' inside
the responder-shift-handoff flow (keep the same return on failure), ensuring you
reference fromData, toData, and accountStatus rather than isActive so suspended
or missing-boolean cases align with roster/phase-6 logic.
Comment on lines
+111
to
+126
| // TTL cleanup: delete projection entries not updated in 90s | ||
| const projectionSnap = await database.ref('shared_projection').get() | ||
| const projections = (projectionSnap.val() ?? {}) as Record< | ||
| string, | ||
| Record<string, ProjectionEntry> | ||
| > | ||
|
|
||
| for (const [muniId, entries] of Object.entries(projections)) { | ||
| for (const [uid, entry] of Object.entries(entries)) { | ||
| if (typeof entry.lastSeenAt === 'number' && now - entry.lastSeenAt > TTL_MS) { | ||
| writeOps.push(database.ref(`shared_projection/${muniId}/${uid}`).remove()) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| await Promise.all(writeOps) |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
The 30-second full-tree TTL sweep will not scale.
Fetching the entire shared_projection tree on every run makes cost and latency O(total projected responders), even when only a few entries changed. Consider expiring per responder at write time, or limiting cleanup to touched municipalities instead of rescanning the whole tree.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@functions/src/scheduled/project-responder-locations.ts` around lines 111 -
126, The current TTL cleanup scans the entire shared_projection tree
(projectionSnap / projections) which won't scale; instead, add per-entry expiry
at write time (set an expiresAt or lastSeenAt when creating/updating a
ProjectionEntry in the code paths that call write operations) and change the
cleanup in project-responder-locations.ts to only scan municipalities that were
touched recently (e.g., accept or read a touchedMunicipalities set or derive it
from writeOps context) or delete by checking expiresAt on entries for those
specific muniIds rather than iterating over all entries; update the code that
writes responder entries to set TTL_MS-based expiresAt and modify the cleanup
loop (which currently iterates projectionSnap/projections and uses TTL_MS and
writeOps) to only operate on the touched muniIds or remove entries whose
expiresAt <= now.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/submit-responder-witnessed-report.ts`:
- Around line 226-227: The Zod validation result from
submitResponderWitnessedReportSchema is discarded; update the error path where
parsed.success is false to include validation details in the thrown HttpsError
(use the HttpsError third "details" parameter or append to the message). Extract
a clear message from parsed.error (e.g., parsed.error.errors[0].message or
parsed.error.flatten()) and pass that as the details or part of the message when
throwing new HttpsError('invalid-argument', ... , ... ) so clients receive the
validation reason.
- Line 126: The code mixes global crypto with imported crypto utilities: replace
the use of global crypto.randomUUID() when creating correlationId with an
imported randomUUID for consistency; add randomUUID to the existing imports from
'node:crypto' alongside randomBytes/createHash/randomInt and update the
correlationId assignment in submit-responder-witnessed-report (the variable
created as correlationId) to call randomUUID() directly.
- Around line 174-181: The tokenHash is computed from randomBytes(32) but the
raw lookup secret is never retained, so verification later (in
request-lookup.ts) cannot occur; fix by generating a lookupSecret (Buffer or hex
string) via randomBytes(32), compute tokenHash =
createHash('sha256').update(lookupSecret).digest('hex'), store tokenHash in the
report_lookup document (as currently done) and return the raw lookupSecret
(e.g., hex string) from the submit handler so callers can persist or present it;
update the function's return/interface to include lookupSecret and ensure any
code paths using tokenHash (e.g., verification in request-lookup.ts) expect the
secret to be provided by the creator.
In `@infra/firebase/firestore.rules`:
- Around line 58-70: The generated Firestore rules artifact (firestore.rules) is
out of sync with the rules builder (build-rules.ts): regenerate the rules using
the builder, verify the transitions lines for statuses like
'accepted'->'unable_to_complete',
'acknowledged'->'en_route'/'cancelled'/'superseded'/'unable_to_complete',
'en_route'->'on_scene'/'cancelled'/'superseded'/'unable_to_complete', and
'on_scene'->'resolved'/'cancelled'/'superseded'/'unable_to_complete' are
present, update the builder logic in build-rules.ts if it produced incorrect
output, then commit the regenerated firestore.rules artifact so CI passes.
🪄 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: 35d23fec-a45e-4ab5-8ac4-2264f23fa7c0
📒 Files selected for processing (2)
functions/src/callables/submit-responder-witnessed-report.tsinfra/firebase/firestore.rules
| const publicRef = generatePublicRef() | ||
| const nowMs = now.toMillis() | ||
| const municipalityId = dispatch.assignedTo.municipalityId | ||
| const correlationId = crypto.randomUUID() |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Minor inconsistency: Mix of imported and global crypto.
randomUUID() is accessed via global crypto, while other crypto functions (randomBytes, createHash, randomInt) are explicitly imported from node:crypto. For consistency and clarity, consider importing randomUUID as well.
♻️ Suggested fix
-import { randomBytes, createHash, randomInt } from 'node:crypto'
+import { randomBytes, createHash, randomInt, randomUUID } from 'node:crypto'Then on line 126:
- const correlationId = crypto.randomUUID()
+ const correlationId = randomUUID()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@functions/src/callables/submit-responder-witnessed-report.ts` at line 126,
The code mixes global crypto with imported crypto utilities: replace the use of
global crypto.randomUUID() when creating correlationId with an imported
randomUUID for consistency; add randomUUID to the existing imports from
'node:crypto' alongside randomBytes/createHash/randomInt and update the
correlationId assignment in submit-responder-witnessed-report (the variable
created as correlationId) to call randomUUID() directly.
Comment on lines
+174
to
+181
| tx.set(db.collection('report_lookup').doc(publicRef), { | ||
| publicTrackingRef: publicRef, | ||
| reportId, | ||
| tokenHash: createHash('sha256').update(randomBytes(32)).digest('hex'), | ||
| expiresAt: nowMs + 365 * 24 * 60 * 60 * 1000, | ||
| createdAt: nowMs, | ||
| schemaVersion: 1, | ||
| }) |
Contributor
There was a problem hiding this comment.
Critical: Lookup secret is lost and can never be verified.
The tokenHash is computed from randomBytes(32), but those random bytes (the lookup secret) are immediately discarded. According to request-lookup.ts, verification requires the original secret to hash and compare against tokenHash.
Since the secret is never returned or stored, the lookup mechanism for this report is permanently broken.
🐛 Proposed fix: Generate, store hash, and return the secret
+ const lookupSecret = randomBytes(32).toString('base64url')
tx.set(db.collection('report_lookup').doc(publicRef), {
publicTrackingRef: publicRef,
reportId,
- tokenHash: createHash('sha256').update(randomBytes(32)).digest('hex'),
+ tokenHash: createHash('sha256').update(lookupSecret).digest('hex'),
expiresAt: nowMs + 365 * 24 * 60 * 60 * 1000,
createdAt: nowMs,
schemaVersion: 1,
})
// ... later in return
- return { reportId, publicTrackingRef: publicRef }
+ return { reportId, publicTrackingRef: publicRef, lookupSecret }Note: The return type and interface will need corresponding updates to include lookupSecret.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@functions/src/callables/submit-responder-witnessed-report.ts` around lines
174 - 181, The tokenHash is computed from randomBytes(32) but the raw lookup
secret is never retained, so verification later (in request-lookup.ts) cannot
occur; fix by generating a lookupSecret (Buffer or hex string) via
randomBytes(32), compute tokenHash =
createHash('sha256').update(lookupSecret).digest('hex'), store tokenHash in the
report_lookup document (as currently done) and return the raw lookupSecret
(e.g., hex string) from the submit handler so callers can persist or present it;
update the function's return/interface to include lookupSecret and ensure any
code paths using tokenHash (e.g., verification in request-lookup.ts) expect the
secret to be provided by the creator.
Comment on lines
+226
to
+227
| const parsed = submitResponderWitnessedReportSchema.safeParse(request.data) | ||
| if (!parsed.success) throw new HttpsError('invalid-argument', 'malformed payload') |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider including Zod validation details in error.
The Zod validation errors are discarded, making it harder for clients to understand what's malformed. Consider including the first error message for easier debugging.
♻️ Include validation details
const parsed = submitResponderWitnessedReportSchema.safeParse(request.data)
- if (!parsed.success) throw new HttpsError('invalid-argument', 'malformed payload')
+ if (!parsed.success) {
+ throw new HttpsError('invalid-argument', parsed.error.issues[0]?.message ?? 'malformed payload')
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@functions/src/callables/submit-responder-witnessed-report.ts` around lines
226 - 227, The Zod validation result from submitResponderWitnessedReportSchema
is discarded; update the error path where parsed.success is false to include
validation details in the thrown HttpsError (use the HttpsError third "details"
parameter or append to the message). Extract a clear message from parsed.error
(e.g., parsed.error.errors[0].message or parsed.error.flatten()) and pass that
as the details or part of the message when throwing new
HttpsError('invalid-argument', ... , ... ) so clients receive the validation
reason.
…update dispatch-states JSDoc Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…isActive->accountStatus, report existence guard
…, narrow status type
…kage assertion, refactor seedDispatchActive to object params, add testEnv cleanup
… errors, fix battery interval inversion, APP_VERSION from env
…_TYPES, dev-only push log, push-client listener cleanup
…tus enum check and updatedAt type guard
Contributor
There was a problem hiding this comment.
Actionable comments posted: 16
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/responder-app/src/hooks/useDispatch.ts (1)
99-106: 🧹 Nitpick | 🔵 TrivialRedundant
setLoading(false)in catch block.Line 103 calls
setLoading(false), but thefinallyblock at line 105 already does this unconditionally. The duplicate call is harmless but unnecessary.♻️ Proposed cleanup
} catch (err: unknown) { console.error('[useDispatch] snapshot mapping failed:', err) setDispatch(undefined) setError(err instanceof Error ? err : new Error(String(err))) - setLoading(false) } finally { setLoading(false) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/responder-app/src/hooks/useDispatch.ts` around lines 99 - 106, In useDispatch's error handling, remove the redundant setLoading(false) call inside the catch block since the finally block already unconditionally calls setLoading(false); update the catch in the async block that currently logs the error and calls setDispatch, setError and setLoading so it only logs the error, sets dispatch to undefined and sets the error (keeping the existing err instanceof Error logic), leaving the finally block to handle setLoading; reference the useDispatch hook and the catch/finally surrounding setLoading.
♻️ Duplicate comments (3)
apps/responder-app/src/hooks/useSubmitResponderWitnessedReport.ts (1)
31-40:⚠️ Potential issue | 🟠 MajorStrengthen boundary validation before geolocation/callable invocation.
Current checks allow whitespace-only values and unconstrained enum strings. Tighten validation at Line 31-Line 40 so malformed input is rejected locally before expensive calls.
Proposed fix
- if (!dispatchId) { + const normalizedDispatchId = dispatchId.trim() + if (!normalizedDispatchId) { const err = new Error('dispatch_id_required') setError(err) throw err } - if (!payload.reportType || !payload.description || !payload.severity) { + const normalizedDescription = payload.description.trim() + const allowedSeverity = new Set(['low', 'medium', 'high']) + if ( + !payload.reportType || + !normalizedDescription || + !allowedSeverity.has(payload.severity) + ) { const err = new Error('required_fields_missing') setError(err) throw err }As per coding guidelines, "Validate external input at boundaries and never swallow errors with empty catch blocks; assume external input is malicious or broken."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/responder-app/src/hooks/useSubmitResponderWitnessedReport.ts` around lines 31 - 40, The boundary validation in useSubmitResponderWitnessedReport is too weak: strengthen the checks around dispatchId and payload fields by trimming string inputs and rejecting whitespace-only values, and validate payload.reportType against the allowed enum/whitelist rather than accepting any string; specifically, update the guard that currently checks dispatchId and payload.reportType/description/severity to (1) ensure dispatchId.trim() is non-empty, (2) ensure payload.description.trim() and payload.severity (if string) are non-empty after trim, and (3) verify payload.reportType using a dedicated validator or enum lookup (e.g., isValidReportType or RESPONDER_WITNESSED_REPORT_TYPES.includes(payload.reportType)) and throw/setError as before when validation fails so malformed input is rejected before geolocation/callable invocation.infra/firebase/database.rules.json (2)
12-14:⚠️ Potential issue | 🟠 Major
responder_indexis writable by any signed-in user and still doesn’t validate the full shape.This node is not just metadata:
responder_locationsreads use it for municipality/agency scoping, and the projection job uses it for routing. Allowing any authenticated user to self-write here with onlymunicipalityIdvalidation leaves both authorization and projection vulnerable to spoofed index data. Restrict writes to active responders and validateagencyIdalongsidemunicipalityId.As per coding guidelines, "Defensive programming: Assume external input is malicious or broken. Validate at the boundary."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@infra/firebase/database.rules.json` around lines 12 - 14, The responder_index rules currently allow any authenticated user to write with only municipalityId validated; tighten the .write and .validate logic so only an active responder can write and both agencyId and municipalityId are validated: update the ".write" condition to require auth != null, auth.uid === $uid, and that root.child('responders').child(auth.uid).child('active').val() === true (or the project’s canonical active-responder flag), and update ".validate" to require newData.hasChildren(['municipalityId','agencyId']) and ensure newData.child('municipalityId').isString() && newData.child('agencyId').isString() (and optionally restrict extra keys if needed) to prevent spoofing of responder_index used by responder_locations and projection jobs.
7-7:⚠️ Potential issue | 🟠 MajorTelemetry rules still accept arbitrary
motionStateandtelemetryStatusvalues.The shared contract in
packages/shared-validators/src/responders.ts:22-34constrains both fields to enums, but this rule only checks that they are strings. A hostile client can still write invalid states and bypass the contract you just tightened in the app/schema layer.As per coding guidelines, "Defensive programming: Assume external input is malicious or broken. Validate at the boundary."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@infra/firebase/database.rules.json` at line 7, The database rules currently only assert that motionState and telemetryStatus are strings; update the telemetry .validate expression to enforce the exact enum values defined in responders.ts (the enums for motionState and telemetryStatus) by replacing the loose string checks with explicit membership checks using newData.child('motionState').val() and newData.child('telemetryStatus').val() against the allowed values from responders.ts; keep all existing numeric/format checks (capturedAt, lat, lng, accuracy, batteryPct, appVersion) and ensure the new membership checks are written as boolean OR chains or an equivalent contains-style expression so the security rule rejects any value not in the responder enums.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/admin-desktop/src/hooks/useAgencyAssistanceQueue.ts`:
- Around line 37-45: The returned object in useAgencyAssistanceQueue.ts
currently casts raw.status to BackupRequest['status'] without validation; update
the mapping to validate raw.status against the actual allowed status values
(e.g., a const ALLOWED_STATUSES or by deriving the union from BackupRequest)
before casting and only accept it if ALLOWED_STATUSES.includes(raw.status as
string), otherwise fallback to 'pending'; reference the return mapping where
status is set and the symbols raw.status and BackupRequest['status'] to locate
and implement the check.
- Around line 113-115: The error callback in the subscription within
useAgencyAssistanceQueue currently swallows errors; update that anonymous error
handler (the () => { // Backup request errors are non-fatal }) to log the error
instead of ignoring it—use the project's logging utility if available (e.g.,
logger.error or similar) or console.error as a fallback, and include contextual
information (e.g., "Agency assistance queue backup request error") along with
the error object to aid debugging and observability.
- Around line 54-70: The initial state resets using queueMicrotask can race with
the later setLoading(true); make the resets synchronous: remove the
queueMicrotask wrappers around setError(null), setRequests([]),
setBackupRequests([]) and call them directly at the top of the useEffect;
likewise call setLoading(false) synchronously in the if (!agencyId) branch and
setLoading(true) synchronously before kicking off any async fetches. Update the
useEffect that references useEffect, agencyId, setError, setRequests,
setBackupRequests, and setLoading to eliminate the microtask timing and perform
state resets and loading toggles immediately.
In `@apps/admin-desktop/src/pages/AgencyAssistanceQueuePage.tsx`:
- Around line 175-196: The UI is calling renderRequestActions for backup items
(from filteredBackupRequests) but the action handlers handleAccept/handleDecline
call callables.acceptAgencyAssistance and callables.declineAgencyAssistance
which do not exist for backups; either add backup-specific callables (e.g.,
callables.acceptBackupRequest and callables.declineBackupRequest) and wire
handleAccept/handleDecline for backup items to use them, or change the UI to
branch on the request type inside renderRequestActions (or its caller) to call
the existing agency assistance callables only for agency requests and call the
new backup callables for backup requests; update any related function names
(renderRequestActions, handleAccept, handleDecline) and the callables service to
keep naming consistent.
In `@apps/admin-desktop/src/pages/RosterPage.tsx`:
- Around line 7-14: Extract the duplicated Freshness type and computeFreshness
function into a shared utility module (e.g., utils/freshness) and export both so
they can be reused; replace the local definitions in RosterPage.tsx and
useEligibleResponders.ts with imports from that new module, ensuring the
exported Freshness union/type and computeFreshness signature
(computeFreshness(lastTelemetryAt: number | null): Freshness) match the existing
usage and update any references to the symbol names accordingly.
- Line 35: bulkStatus is currently inferred as string, forcing an unsafe cast
later; change the useState call for bulkStatus to a discriminated union of the
allowed status literals (e.g. useState<'available'|'unavailable'|'on_leave'>(
'available' )) so TypeScript knows only valid statuses are possible, then remove
the `as` cast where bulkStatus is used (the place that reads from the select and
the usage around setBulkStatus / bulkStatus). Update references to bulkStatus
and setBulkStatus accordingly so no runtime behavior changes—only the state
typing.
In `@apps/responder-app/android/app/src/main/AndroidManifest.xml`:
- Around line 41-44: Add the missing ACCESS_BACKGROUND_LOCATION permission to
the manifest and implement a runtime permission flow in telemetry-client.ts so
BackgroundGeolocation.addWatcher() is only called after the user grants
background location: add the <uses-permission
android:name="android.permission.ACCESS_BACKGROUND_LOCATION" /> declaration to
AndroidManifest.xml, then in telemetry-client.ts before calling
BackgroundGeolocation.addWatcher() check for and request
ACCESS_BACKGROUND_LOCATION (and FINE/COARSE if needed), show rationale when
required, await the permission result, and only invoke
BackgroundGeolocation.addWatcher() when the background location permission is
granted; handle denied cases gracefully (skip watcher or show prompt) and
surface any errors.
In `@apps/responder-app/src/hooks/useResponderAvailability.ts`:
- Around line 14-18: The hook's type and mapping are out of sync with Firestore:
update the ResponderAvailabilityStatus union to include 'on_break' and modify
mapFromFirestore to return 'on_break' when raw === 'on_break' (keep the existing
mapping of 'on_duty' → 'available' and preserve other cases), so Firestore
values never map to null unexpectedly; also ensure any consumers of
ResponderAvailabilityStatus (the hook's return type) are adjusted to handle the
new 'on_break' value.
In `@apps/responder-app/src/hooks/useResponderTelemetry.ts`:
- Around line 96-97: Wrap the two writes so partial state is handled: perform
the RTDB write (set(ref(rtdb, `responder_locations/${uid}`), validated)) then
attempt the Firestore merge (setDoc(doc(db, 'responders', uid), {
lastTelemetryAt: now }, { merge: true })) inside a try-catch; on Firestore
failure log the error and the fact RTDB was updated (including uid and now) and
optionally retry or mark a retry queue/flag so the next cycle can reconcile—do
not attempt cross-service atomicity since RTDB and Firestore cannot be
transactional together.
In `@apps/responder-app/src/pages/ResponderWitnessReportPage.tsx`:
- Around line 34-36: The empty catch in the submission flow inside
ResponderWitnessReportPage swallows errors; update the catch to capture the
error (e.g., catch (err)) and either log it via the existing logger or set
component state to surface it to the user and/or rethrow so failures are
diagnosable; change the try/catch in the submission handler (the onSubmit/submit
function inside ResponderWitnessReportPage) to log the error
(processLogger.error or console.error) and/or call setSubmissionError(error) /
show an error toast so the failure is observable.
In `@apps/responder-app/src/pages/SosPage.tsx`:
- Around line 18-20: handleConfirm currently swallows all exceptions with an
empty catch block; replace the empty catch with catching the error (catch
(error)) and surface it (e.g., log it via console.error or your app logger
and/or set an error state or toast) while preserving existing UI behavior
referenced by the "error surfaced by hook" comment; update the catch in the
handleConfirm function so failures are observable and include the error object
in the log message for debugging.
In `@apps/responder-app/src/services/push-client.ts`:
- Around line 45-59: The addListener setup can race because
registrationHandle/registrationErrorHandle may be null when the event callback
calls remove(); to fix, capture listener handles before allowing the callbacks
to call resolveOnce: call PushNotifications.addListener twice and store the
returned Promise for each handle (e.g., registrationHandlePromise and
registrationErrorHandlePromise), then use Promise.all to await both promises and
assign registrationHandle and registrationErrorHandle before invoking
resolveOnce inside the registration and registrationError callbacks (or
alternatively restructure so the callbacks wait on those handle promises),
ensuring remove() always operates on the actual listener handles; reference the
existing symbols PushNotifications.addListener, registrationHandle,
registrationErrorHandle, resolveOnce, and remove.
In `@functions/src/callables/responder-roster.ts`:
- Around line 235-249: The loop currently performs individual reads for each uid
using responderRef = db.collection('responders').doc(uid) and snap = await
responderRef.get(), causing N round-trips; replace this with a batched read
using a whereIn query on the responders collection
(db.collection('responders').where('__name__', 'in', chunk)) to fetch up to 10
uids per chunk, validate existence and agencyId from the returned snapshots,
then use batch.update(...) for each matching doc (preserve
availability/updatedAt logic and updated counter); implement chunking over uids
to handle >10 items and retain the existing batch write flow.
In `@functions/src/callables/responder-shift-handoff.ts`:
- Around line 123-132: The tx.set call is writing createdAt and expiresAt as
Firestore Timestamps while the shared schema responderShiftHandoffDocSchema
expects integer milliseconds and the accept path already treats expiresAt as a
number (calls expiresAt.toMillis()); change the write in the code that sets
createdAt and expiresAt (the tx.set for existingRef) to store numeric
millisecond values (e.g., now.toMillis() and now.toMillis() + 30*60*1000) and
update any reads/parsing in this file (including the accept path that assumes
expiresAt has toMillis) to treat those fields as numbers per the shared schema
so both callable and shared schema stay consistent.
- Around line 80-90: The current idempotency path in the transaction that
computes handoffId then reads the existing document (existingRef / existing)
blindly returns success, which hides payload mismatches; update the
db.runTransaction handler in responder-shift-handoff.ts to load the persisted
document fields (e.g., reason and any other input fields you care about) from
existing.data(), compare them to the current input (input.reason, input.toUid,
actor.uid, etc.), and only treat it as a cache hit when they match; if they
differ, return a clear conflict/error result (or surface the mismatch) so
callers know the idempotency key was reused with a different payload—reuse the
same idempotency-check pattern used elsewhere in the codebase for consistency.
In `@infra/firebase/firestore.rules.template`:
- Around line 229-233: The Firestore rule for responder updates requires
request.resource.data.updatedAt to be a timestamp but the shared responder
validator (packages/shared-validators/src/responders.ts) models updatedAt as an
integer, causing a mismatch; change the rule in
infra/firebase/firestore.rules.template (the allow update block using uid(),
rUid, request.resource.data.diff(...).hasOnly(...), and
request.resource.data.updatedAt) to expect the same type as the shared validator
(e.g., replace the "is timestamp" check with the integer check used by your
validators such as "is int" or otherwise accept the numeric representation), or
alternatively update the shared validator/writers to use Firestore
timestamp—just ensure request.resource.data.updatedAt, the shared validator, and
any writers all use the same representation.
---
Outside diff comments:
In `@apps/responder-app/src/hooks/useDispatch.ts`:
- Around line 99-106: In useDispatch's error handling, remove the redundant
setLoading(false) call inside the catch block since the finally block already
unconditionally calls setLoading(false); update the catch in the async block
that currently logs the error and calls setDispatch, setError and setLoading so
it only logs the error, sets dispatch to undefined and sets the error (keeping
the existing err instanceof Error logic), leaving the finally block to handle
setLoading; reference the useDispatch hook and the catch/finally surrounding
setLoading.
---
Duplicate comments:
In `@apps/responder-app/src/hooks/useSubmitResponderWitnessedReport.ts`:
- Around line 31-40: The boundary validation in
useSubmitResponderWitnessedReport is too weak: strengthen the checks around
dispatchId and payload fields by trimming string inputs and rejecting
whitespace-only values, and validate payload.reportType against the allowed
enum/whitelist rather than accepting any string; specifically, update the guard
that currently checks dispatchId and payload.reportType/description/severity to
(1) ensure dispatchId.trim() is non-empty, (2) ensure payload.description.trim()
and payload.severity (if string) are non-empty after trim, and (3) verify
payload.reportType using a dedicated validator or enum lookup (e.g.,
isValidReportType or
RESPONDER_WITNESSED_REPORT_TYPES.includes(payload.reportType)) and
throw/setError as before when validation fails so malformed input is rejected
before geolocation/callable invocation.
In `@infra/firebase/database.rules.json`:
- Around line 12-14: The responder_index rules currently allow any authenticated
user to write with only municipalityId validated; tighten the .write and
.validate logic so only an active responder can write and both agencyId and
municipalityId are validated: update the ".write" condition to require auth !=
null, auth.uid === $uid, and that
root.child('responders').child(auth.uid).child('active').val() === true (or the
project’s canonical active-responder flag), and update ".validate" to require
newData.hasChildren(['municipalityId','agencyId']) and ensure
newData.child('municipalityId').isString() &&
newData.child('agencyId').isString() (and optionally restrict extra keys if
needed) to prevent spoofing of responder_index used by responder_locations and
projection jobs.
- Line 7: The database rules currently only assert that motionState and
telemetryStatus are strings; update the telemetry .validate expression to
enforce the exact enum values defined in responders.ts (the enums for
motionState and telemetryStatus) by replacing the loose string checks with
explicit membership checks using newData.child('motionState').val() and
newData.child('telemetryStatus').val() against the allowed values from
responders.ts; keep all existing numeric/format checks (capturedAt, lat, lng,
accuracy, batteryPct, appVersion) and ensure the new membership checks are
written as boolean OR chains or an equivalent contains-style expression so the
security rule rejects any value not in the responder enums.
🪄 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: 6353a4e8-5ccf-48ed-a8aa-1a90bc3d8d12
📒 Files selected for processing (38)
apps/admin-desktop/src/hooks/useAgencyAssistanceQueue.tsapps/admin-desktop/src/hooks/useEligibleResponders.tsapps/admin-desktop/src/hooks/useRosterManagement.tsapps/admin-desktop/src/pages/AgencyAssistanceQueuePage.tsxapps/admin-desktop/src/pages/RosterPage.tsxapps/admin-desktop/src/services/callables.tsapps/responder-app/android/app/src/androidTest/java/com/getcapacitor/myapp/ExampleInstrumentedTest.javaapps/responder-app/android/app/src/main/AndroidManifest.xmlapps/responder-app/android/app/src/main/res/xml/file_paths.xmlapps/responder-app/src/hooks/useDispatch.tsapps/responder-app/src/hooks/useMarkDispatchUnableToComplete.tsapps/responder-app/src/hooks/useRegisterFcmToken.tsapps/responder-app/src/hooks/useRequestBackup.tsapps/responder-app/src/hooks/useResponderAvailability.tsapps/responder-app/src/hooks/useResponderTelemetry.tsapps/responder-app/src/hooks/useSubmitResponderWitnessedReport.tsapps/responder-app/src/hooks/useTriggerSOS.tsapps/responder-app/src/pages/BackupRequestPage.tsxapps/responder-app/src/pages/DispatchListPage.tsxapps/responder-app/src/pages/ResponderWitnessReportPage.tsxapps/responder-app/src/pages/SosPage.tsxapps/responder-app/src/routes.tsxapps/responder-app/src/services/push-client.tsfunctions/src/__tests__/callables/mark-dispatch-unable-to-complete.test.tsfunctions/src/__tests__/callables/request-backup.test.tsfunctions/src/__tests__/callables/submit-responder-witnessed-report.test.tsfunctions/src/__tests__/scheduled/project-responder-locations.test.tsfunctions/src/callables/mark-dispatch-unable-to-complete.tsfunctions/src/callables/responder-roster.tsfunctions/src/callables/responder-shift-handoff.tsfunctions/src/scheduled/project-responder-locations.tsinfra/firebase/database.rules.jsoninfra/firebase/firestore.rulesinfra/firebase/firestore.rules.templatepackages/shared-validators/src/coordination.tspackages/shared-validators/src/dispatches.test.tspackages/shared-validators/src/responders.tspackages/shared-validators/src/state-machines/dispatch-states.ts
Comment on lines
+54
to
+70
| useEffect(() => { | ||
| queueMicrotask(() => { | ||
| setError(null) | ||
| setRequests([]) | ||
| setBackupRequests([]) | ||
| }) | ||
|
|
||
| if (!agencyId) { | ||
| queueMicrotask(() => { | ||
| setLoading(false) | ||
| }) | ||
| return | ||
| } | ||
|
|
||
| queueMicrotask(() => { | ||
| setLoading(true) | ||
| }) |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
State reset via queueMicrotask has subtle timing.
The queueMicrotask on lines 55-59 clears state, but this runs asynchronously after the effect body continues. If agencyId is falsy, the early return on line 65 happens before the microtask executes, which is fine. However, if agencyId is valid, setLoading(true) (line 69) may race with setRequests([]) (line 57) depending on microtask ordering.
Consider synchronous state reset for clarity:
♻️ Simpler synchronous reset
useEffect(() => {
- queueMicrotask(() => {
- setError(null)
- setRequests([])
- setBackupRequests([])
- })
+ setError(null)
+ setRequests([])
+ setBackupRequests([])
if (!agencyId) {
- queueMicrotask(() => {
- setLoading(false)
- })
+ setLoading(false)
return
}
- queueMicrotask(() => {
- setLoading(true)
- })
+ setLoading(true)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/admin-desktop/src/hooks/useAgencyAssistanceQueue.ts` around lines 54 -
70, The initial state resets using queueMicrotask can race with the later
setLoading(true); make the resets synchronous: remove the queueMicrotask
wrappers around setError(null), setRequests([]), setBackupRequests([]) and call
them directly at the top of the useEffect; likewise call setLoading(false)
synchronously in the if (!agencyId) branch and setLoading(true) synchronously
before kicking off any async fetches. Update the useEffect that references
useEffect, agencyId, setError, setRequests, setBackupRequests, and setLoading to
eliminate the microtask timing and perform state resets and loading toggles
immediately.
Responder app: - Remove redundant setLoading(false) in useDispatch catch - Strengthen boundary validation in useSubmitResponderWitnessedReport - Add on_break to ResponderAvailabilityStatus - Handle partial writes in useResponderTelemetry - Log errors in ResponderWitnessReportPage and SosPage - Fix push-client listener race with Promise.all - Add ACCESS_BACKGROUND_LOCATION to AndroidManifest - Handle NOT_AUTHORIZED in telemetry-client Admin desktop: - Validate backup request statuses in useAgencyAssistanceQueue - Log backup subscription errors - Branch renderRequestActions for agency vs backup requests - Extract shared freshness utility - Type bulkStatus as union in RosterPage Functions: - Batch responder reads with whereIn chunking in responder-roster - Store handoff timestamps as milliseconds in responder-shift-handoff - Strengthen idempotency payload check in responder-shift-handoff Rules: - Enforce telemetry enum values in database.rules.json - Tighten responder_index write/validate rules - Align firestore updatedAt type to int
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 (1)
packages/shared-validators/lib/dispatches.test.js (1)
46-58: 🧹 Nitpick | 🔵 TrivialAdd explicit transition tests for
unable_to_complete.This change validates enum acceptance, but this suite still doesn’t assert transition behavior into/from
unable_to_complete. Add focused checks to guard state-machine regressions.🧪 Suggested test additions
describe('DISPATCH_TRANSITIONS — 3c additions', () => { + it('allows active states → unable_to_complete', () => { + for (const from of ['accepted', 'acknowledged', 'en_route', 'on_scene']) { + expect(isValidDispatchTransition(from, 'unable_to_complete')).toBe(true); + } + }); + it('denies unable_to_complete → resolved (terminal)', () => { + expect(isValidDispatchTransition('unable_to_complete', 'resolved')).toBe(false); + }); it('allows acknowledged → en_route', () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/shared-validators/lib/dispatches.test.js` around lines 46 - 58, Add focused tests in packages/shared-validators/lib/dispatches.test.js that assert explicit state transitions into and out of 'unable_to_complete': write unit cases that verify which prior statuses are allowed to transition to 'unable_to_complete' and which subsequent transitions from 'unable_to_complete' are prohibited or allowed, using the project’s dispatch transition validator (the same helper used elsewhere in this file—e.g., the status transition validator/assertion helpers present in this test suite). Ensure you include both positive (allowed) and negative (disallowed) assertions so future state-machine changes will fail the tests if regressions occur.
♻️ Duplicate comments (6)
apps/admin-desktop/src/hooks/useAgencyAssistanceQueue.ts (1)
66-82:⚠️ Potential issue | 🟡 MinorMake the reset/loading updates synchronous.
These setters already run inside an effect; deferring them with
queueMicrotaskadds another ordering hop and can briefly leave stale queue data or the wrong loading state visible during agency switches/logout. This is the same timing issue raised earlier.♻️ Simpler reset path
useEffect(() => { - queueMicrotask(() => { - setError(null) - setRequests([]) - setBackupRequests([]) - }) + setError(null) + setRequests([]) + setBackupRequests([]) if (!agencyId) { - queueMicrotask(() => { - setLoading(false) - }) + setLoading(false) return } - queueMicrotask(() => { - setLoading(true) - }) + setLoading(true)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/admin-desktop/src/hooks/useAgencyAssistanceQueue.ts` around lines 66 - 82, The effect currently defers state resets and loading changes via queueMicrotask, which can momentarily expose stale data during agencyId changes; remove the queueMicrotask wrappers and call setError(null), setRequests([]), setBackupRequests([]), and setLoading(true/false) synchronously inside the useEffect body (including the early-return branch that sets setLoading(false)) so that state updates happen immediately when agencyId changes; update the effect that references setError, setRequests, setBackupRequests, setLoading and agencyId accordingly.functions/src/callables/responder-shift-handoff.ts (1)
20-30:⚠️ Potential issue | 🟠 MajorValidate the handoff document with the shared schema instead of type-casting it.
This file duplicates the handoff contract locally and then trusts Firestore data with
as ResponderShiftHandoff. That bypasses the existingresponderShiftHandoffDocSchemainpackages/shared-validators/src/coordination.ts, so malformedagencyId/municipalityId/expiresAtvalues can be written here and then silently mis-evaluated in the accept flow. Parse the payload beforetx.set(...), and parsesnap.data()on read.🧩 Suggested consolidation on the shared validator
-import { BantayogError, logDimension } from '@bantayog/shared-validators' +import { + BantayogError, + logDimension, + responderShiftHandoffDocSchema, +} from '@bantayog/shared-validators' - -interface ResponderShiftHandoff { - fromUid: string - toUid: string - agencyId: string - municipalityId: string - reason: string - status: 'pending' | 'accepted' | 'declined' - createdAt: number - expiresAt: number - schemaVersion: number -} ... - tx.set(existingRef, { + const handoffDoc = responderShiftHandoffDocSchema.parse({ fromUid: actor.uid, toUid: input.toUid, agencyId: fromData.agencyId, municipalityId: fromData.municipalityId, reason: input.reason, status: 'pending', createdAt: now.toMillis(), expiresAt: now.toMillis() + 30 * 60 * 1000, schemaVersion: 1, }) + tx.set(existingRef, handoffDoc) ... - const handoff = snap.data() as ResponderShiftHandoff | undefined - if (handoff === undefined) return { success: false, errorCode: 'not-found' } + const parsedHandoff = responderShiftHandoffDocSchema.safeParse(snap.data()) + if (!parsedHandoff.success) { + return { success: false, errorCode: 'failed-precondition' } + } + const handoff = parsedHandoff.dataAs per coding guidelines: "Validate external input at boundaries and never swallow errors with empty catch blocks; assume external input is malicious or broken."
Also applies to: 137-147, 185-186
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@functions/src/callables/responder-shift-handoff.ts` around lines 20 - 30, The code defines a local ResponderShiftHandoff interface and casts Firestore data with "as ResponderShiftHandoff", bypassing the shared responderShiftHandoffDocSchema; update the flow to validate incoming and stored documents using the shared validator: before calling tx.set(...) parse and validate the payload with responderShiftHandoffDocSchema (throw/return on validation failure) and on reads parse snap.data() with responderShiftHandoffDocSchema instead of casting; replace usages of the ResponderShiftHandoff cast with the validated result and ensure you do not swallow validation errors (apply same change for the other occurrences around lines noted: the accept flow and the other two spots).infra/firebase/database.rules.json (2)
12-14:⚠️ Potential issue | 🟠 Major
responder_indexwrites still are not restricted to active responder tokens.This path is used later to authorize telemetry reads, but the new rule only checks
auth.uid === $uidplusresponders/{uid}/active === true. It still accepts tokens withoutauth.token.role === 'responder'orauth.token.accountStatus === 'active', so a stale/non-responder session can keep mutating authorization-relevant index data.As per coding guidelines, “Defensive programming: Assume external input is malicious or broken. Validate at the boundary.”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@infra/firebase/database.rules.json` around lines 12 - 14, The responder_index ".write" rule currently only checks auth.uid and responders/{uid}/active, so update the ".write" condition to additionally require auth.token.role === 'responder' and auth.token.accountStatus === 'active' (keep the existing root.child('responders').child(auth.uid).child('active').val() === true check); this ensures only actively authenticated responder tokens can mutate the index—modify the ".write" expression where responder_index is defined and reference auth.token.role and auth.token.accountStatus alongside auth.uid and the responders/{uid}/active lookup.
7-7:⚠️ Potential issue | 🔴 CriticalThe
inoperator is not supported in Firebase Realtime Database security rules and will prevent deployment.Line 7 uses
val() in [...]for enum checks onmotionStateandtelemetryStatus. The official RTDB rules reference documents only these operators:+,-,*,/,%,===,!==,&&,||,!,>,<,>=,<=, and?. Theinoperator is not included.Replace these checks with equivalent comparisons using
||:
- For
motionState:(newData.child('motionState').val() === 'moving' || newData.child('motionState').val() === 'walking' || newData.child('motionState').val() === 'still' || newData.child('motionState').val() === 'unknown')- For
telemetryStatus:(newData.child('telemetryStatus').val() === 'active' || newData.child('telemetryStatus').val() === 'degraded' || newData.child('telemetryStatus').val() === 'stale' || newData.child('telemetryStatus').val() === 'offline')🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@infra/firebase/database.rules.json` at line 7, The security rule uses the unsupported `in` operator for enum checks and will fail deployment; replace the `... .val() in [...]` checks with explicit equality chains joined by `||` for both enums: update the `newData.child('motionState')` check to compare `.val()` to 'moving', 'walking', 'still', and 'unknown' using `===`/`||`, and similarly update the `newData.child('telemetryStatus')` check to compare `.val()` to 'active', 'degraded', 'stale', and 'offline' using `===`/`||`, keeping the rest of the `.validate` expression intact.infra/firebase/firestore.rules.template (1)
229-233:⚠️ Potential issue | 🔴 CriticalThis responder update rule now blocks both client write paths.
Line 233 requires
updatedAtto be an integer, butapps/responder-app/src/hooks/useResponderAvailability.tswritesserverTimestamp(), and Lines 230-231 no longer allowlastTelemetryAt, whichapps/responder-app/src/hooks/useResponderTelemetry.tsmerges intoresponders/{uid}. As written, responder availability updates and telemetry freshness updates from the client will both be denied.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@infra/firebase/firestore.rules.template` around lines 229 - 233, The rule for updating responders is too restrictive: it only allows the three keys ['availabilityStatus','availabilityReason','updatedAt'] and requires updatedAt to be an int, which blocks client calls that set serverTimestamp() and telemetry updates that set lastTelemetryAt. Modify the update rule (the block using uid(), rUid, and request.resource.data.diff(resource.data).affectedKeys()) to also allow 'lastTelemetryAt' in the allowed keys and relax the updatedAt type check to accept Firestore Timestamps (e.g., change "request.resource.data.updatedAt is int" to allow timestamp types or accept both int and timestamp), so both responder availability updates (which use serverTimestamp()) and telemetry merges (which write lastTelemetryAt) are permitted.apps/responder-app/src/services/telemetry-client.ts (1)
19-33:⚠️ Potential issue | 🟠 MajorFan out location updates per subscriber instead of overwriting a single callback.
startTracking()still keeps only the latestonLocation, so a second caller silently disconnects the first one.stopTracking()then has no way to remove the correct subscriber, because it doesn't track callback identity. This makes the shared/ref-counted API incorrect as soon as two consumers overlap.Suggested direction
-let refCount = 0 -let currentCallback: ((loc: TelemetryLocation) => void) | null = null +const subscribers = new Set<(loc: TelemetryLocation) => void>() let watcherId: string | null = null let webWatchId: number | null = null export async function startTracking( _dispatchId: string, onLocation: (loc: TelemetryLocation) => void, -): Promise<void> { - refCount++ - currentCallback = onLocation +): Promise<() => Promise<void>> { + subscribers.add(onLocation) - if (refCount > 1) { - // Already tracking; just updated callback - return + if (subscribers.size > 1) { + return async () => { + await stopTracking(onLocation) + } } // ... - currentCallback?.({ ... }) + for (const subscriber of subscribers) { + subscriber({ ... }) + } } -export async function stopTracking(): Promise<void> { - refCount = Math.max(0, refCount - 1) - if (refCount > 0) return +export async function stopTracking(onLocation: (loc: TelemetryLocation) => void): Promise<void> { + subscribers.delete(onLocation) + if (subscribers.size > 0) returnAlso applies to: 56-62, 82-88, 97-101
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/responder-app/src/services/telemetry-client.ts` around lines 19 - 33, startTracking/stopTracking currently use a single mutable currentCallback and refCount, which causes later callers to overwrite earlier subscribers and prevents correct removal; change the implementation to fan-out updates by storing subscribers in a Set or Map (e.g., subscribers: Set<(loc: TelemetryLocation)=>void> or Map<id,callback>), have startTracking add the provided onLocation to that collection (and return a subscriber id or token if needed), increment refCount only for bookkeeping, and have stopTracking remove the exact callback (or remove by id/token) and only clear watcherId/webWatchId when the subscribers collection becomes empty; update usages of currentCallback, watcherId, and webWatchId accordingly so location updates iterate over all subscribers rather than calling a single callback.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/admin-desktop/src/hooks/useAgencyAssistanceQueue.ts`:
- Around line 89-132: The loading/error state currently flips false when the
agency-assistance listener resolves while the backup listener may still be
pending or failed; update useAgencyAssistanceQueue to track readiness for both
listeners (e.g., add requestsReady and backupReady boolean states) and only call
setLoading(false) when both are true, setError when either onSnapshot error
occurs (or surface a dedicated backup error state via setBackupError) instead of
only logging in the backup onSnapshot error handler, and ensure unsubscribe and
unsubscribeBackup still get cleaned up; modify the agency-assistance snapshot
success handler and the backup snapshot success and error handlers to set their
respective ready flags (and setBackupRequests via parseBackupRequest) so the
component's loading/error gating reflects both subscriptions' status.
In `@apps/admin-desktop/src/hooks/useEligibleResponders.ts`:
- Around line 39-63: The snapshot handler is persisting a stale computed
freshness into state; instead store only the raw lastTelemetryAt (keep uid,
displayName, agencyId, availabilityStatus, lastTelemetryAt) in responders and
stop saving computeFreshness(...) here, then have UIs (e.g., DispatchModal)
derive freshness on render by calling
computeFreshness(responder.lastTelemetryAt) and using FRESHNESS_ORDER there for
sorting, or alternatively set up a short interval timer inside this hook that
recomputes freshness by remapping responders with
computeFreshness(lastTelemetryAt) and calling setResponders periodically so
freshness updates over time; update any code that used responder.freshness to
use derived computeFreshness(lastTelemetryAt) instead.
In `@apps/admin-desktop/src/pages/RosterPage.tsx`:
- Around line 123-152: The freshness labels stop updating because
computeFreshness(r.lastTelemetryAt) only runs on React rerenders; modify the
RosterPage component to drive periodic rerenders (e.g., useEffect that sets a
ticking state or calls a setTick via setInterval) so that responders.map
recomputes freshness every N seconds/minutes, and ensure you clear the interval
on unmount; update references around computeFreshness, responders.map, and
FRESHNESS_COLOR so the UI reflects new freshness values without relying solely
on external changes (alternatively, consume a server-provided freshness field if
available).
In `@apps/responder-app/src/hooks/useDispatch.ts`:
- Around line 117-150: The refresh function can apply stale responses after
dispatchId changes; to fix, capture the dispatchId at the start of the
useCallback (e.g., const currentId = dispatchId) and, after any await (including
after getDoc and inside catch/finally), bail out early if the live dispatchId
!== currentId so you don't call setDispatch/setError/setLoading for an outdated
request; apply these checks in the main success path (before setting dispatch),
in the catch block (before setError/setDispatch), and in finally (before
setLoading), referencing the refresh function and the setters setDispatch,
setError, setLoading.
In `@apps/responder-app/src/hooks/useResponderAvailability.ts`:
- Around line 68-76: The setDoc call in useResponderAvailability currently
writes updatedAt as serverTimestamp() (violates the rule expecting an integer)
and omits availabilityReason when trimmedReason is empty (which preserves the
old reason); update the payload in the setDoc call inside
useResponderAvailability so updatedAt is an integer timestamp (e.g., Date.now()
or Math.floor(Date.now()/1000) per your epoch convention) and set
availabilityReason explicitly to trimmedReason ?? null (or '' if your schema
prefers) to clear the field when switching back to available, keeping { merge:
true } behavior.
In `@apps/responder-app/src/hooks/useResponderTelemetry.ts`:
- Around line 62-119: The handleLocation callback currently only checks
cancelled before starting its async IIFE, so in-flight awaits (e.g., await
getBatteryPercentage()) and subsequent writes can still run after teardown;
update handleLocation to re-check the cancellation flag after any await points
and immediately before any RTDB/Firestore writes and before the fire-and-forget
responder_index write (i.e., after getBatteryPercentage resolves, after
responderTelemetryPayloadSchema.parse, and before calling set(ref(...)) and
setDoc(...)); if cancelled is true at any of those checkpoints, return early to
avoid persisting telemetry or launching background writes. Ensure all references
are to the same cancelled flag used in handleLocation so the early returns
reliably stop in-flight work.
In `@apps/responder-app/src/hooks/useSubmitResponderWitnessedReport.ts`:
- Around line 25-31: The submit function currently forwards the optional
photoUrl without validation; update submit (and the same logic used around the
later photo handling) to trim and normalize photoUrl, treat empty or
all-whitespace strings as undefined, validate it using a strict check (e.g.,
construct a URL and ensure protocol is http or https), and if invalid throw a
clear error instead of letting downstream calls fail or swallowing errors;
reference the submit function and the photoUrl variable so reviewers can find
and apply the normalization/validation at this boundary and replicate it for the
later photo handling code paths.
- Around line 81-83: In useSubmitResponderWitnessedReport.ts the callable
payload is sending the raw dispatchId which can contain whitespace; update the
call to fn so it passes normalizedDispatchId instead of dispatchId (i.e.,
replace the dispatchId property in the payload passed to fn with
normalizedDispatchId), ensuring you reference the existing normalizedDispatchId
variable in the useSubmitResponderWitnessedReport hook so backend dispatch
lookup uses the trimmed/normalized value.
In `@apps/responder-app/src/pages/ResponderWitnessReportPage.tsx`:
- Around line 19-31: Trim the description before performing the required-field
check in handleSubmit: create a trimmedDescription = description.trim() at the
top of the handler and use trimmedDescription in the validation check
(reportType, trimmedDescription, severity) and in the payload passed to submit
(description: trimmedDescription); keep the existing photoUrl trimming logic
as-is and clear/setValidationError the same way.
In `@apps/responder-app/src/pages/SosPage.tsx`:
- Line 30: SosPage.tsx currently renders raw error.message directly; change the
UI to display a user-facing, non-sensitive message (e.g., "An unexpected error
occurred. Please try again.") instead of error.message and keep the original
error for debugging (e.g., log it or send to monitoring), and wrap the visible
message in an ARIA live region (role="status" and aria-live="polite" or
aria-live="assertive" as appropriate) so assistive tech is notified; locate the
render that references the error variable (the line with {error && <p
...>{error.message}</p>}) and replace it with the sanitized message +
live-region attributes.
In `@apps/responder-app/src/services/push-client.ts`:
- Around line 107-110: The current code only checks typeof dispatchId ===
'string' before calling onTap; instead validate that dispatchId is a non-empty,
trimmed string and path-safe (e.g., reject values containing '/' or backslashes,
'.' segments like "..", or other unsafe chars) before invoking onTap in
push-client.ts. Update the validation around data and dispatchId to trim() then
test against an allowlist regex (for example alphanumeric plus - and _), and if
invalid do not call onTap — log a clear error or surface the error to the caller
rather than silently swallowing it. Ensure you reference the same variables
(data, dispatchId) and preserve calling onTap(dispatchId) only after the
stricter check passes.
- Around line 29-59: The Promise.all([...]) inside the async IIFE can reject and
leave the outer Promise pending; wrap the await Promise.all([...]) call in a
try/catch so any rejection calls the outer reject and performs cleanup.
Specifically, inside the IIFE surrounding PushNotifications.addListener calls
(references: PushNotifications.addListener, registrationHandle,
registrationErrorHandle, resolveOnce) catch errors from Promise.all and from the
rest of the IIFE, call reject(err instanceof Error ? err : new
Error(String(err))), and only call .remove() on
registrationHandle/registrationErrorHandle if they are defined to avoid
additional exceptions.
In `@functions/src/callables/responder-roster.ts`:
- Around line 97-100: The admin mutation updates use the old field name
"availability" which diverges from the new schema and responder hook that expect
"availabilityStatus"; update all tx.update calls in responder-roster.ts (e.g.,
the call using responderRef at the tx.update that sets
accountStatus/availability/updatedAt and the similar block around lines 248-250)
to write the canonical field "availabilityStatus" (set to 'off_duty' or the
appropriate value) instead of or in addition to "availability" so the
authoritative status is kept consistent across the system; ensure the same field
name is used wherever responder availability is updated.
In `@functions/src/callables/responder-shift-handoff.ts`:
- Around line 34-47: The input schemas (initiateSchema and acceptSchema) allow
values that contain '/' and only enforce min length, enabling Firestore path
traversal; update initiateSchema to forbid '/' in toUid (e.g., add a regex or
.refine to reject any slash) and update acceptSchema to validate handoffId
against the server-generated 20-character hex format (e.g., a regex like
/^[0-9a-fA-F]{20}$/) so only valid document IDs are accepted; additionally,
replace the local ResponderShiftHandoff cast by importing and using
responderShiftHandoffDocSchema from `@bantayog/shared-validators` to validate the
handoff document shape instead of unsafe casting.
In `@infra/firebase/firestore.rules.template`:
- Around line 229-233: The update rule must additionally enforce that when
request.resource.data.availabilityStatus is one of
['off_duty','on_break','unavailable'] the
request.resource.data.availabilityReason is a non-empty string, and it should
also require an active responder session exists for rUid (e.g. check
exists(/databases/$(database)/documents/responderSessions/$(rUid)) and that
get(...).data.active == true); update the allow update condition that currently
references uid(), rUid, request.resource.data.availabilityStatus,
request.resource.data.updatedAt and request.resource.data.diff(...) to include
these two checks so the rule mirrors the client invariant.
In `@packages/shared-validators/lib/coordination.js`:
- Line 143: The current schema field "reason" in coordination.js uses
z.string().min(1).max(1000) which accepts whitespace-only values; update the
"reason" validator to reject strings that are only whitespace by adding a
non-whitespace check (for example using z.string().refine(...) or
z.string().regex(...)) so that trimmed length must be > 0 and provide a clear
validation message like "reason cannot be empty or whitespace"; update the
schema definition for the reason symbol accordingly.
- Around line 145-146: The createdAt and expiresAt validators currently allow
negative integers; update the schema validators (the object containing createdAt
and expiresAt in coordination.js) to enforce non-negative integers by adding a
non-negative constraint (e.g., use z.number().int().nonnegative() or
z.number().int().min(0)) for both createdAt and expiresAt so timestamps cannot
be negative.
---
Outside diff comments:
In `@packages/shared-validators/lib/dispatches.test.js`:
- Around line 46-58: Add focused tests in
packages/shared-validators/lib/dispatches.test.js that assert explicit state
transitions into and out of 'unable_to_complete': write unit cases that verify
which prior statuses are allowed to transition to 'unable_to_complete' and which
subsequent transitions from 'unable_to_complete' are prohibited or allowed,
using the project’s dispatch transition validator (the same helper used
elsewhere in this file—e.g., the status transition validator/assertion helpers
present in this test suite). Ensure you include both positive (allowed) and
negative (disallowed) assertions so future state-machine changes will fail the
tests if regressions occur.
---
Duplicate comments:
In `@apps/admin-desktop/src/hooks/useAgencyAssistanceQueue.ts`:
- Around line 66-82: The effect currently defers state resets and loading
changes via queueMicrotask, which can momentarily expose stale data during
agencyId changes; remove the queueMicrotask wrappers and call setError(null),
setRequests([]), setBackupRequests([]), and setLoading(true/false) synchronously
inside the useEffect body (including the early-return branch that sets
setLoading(false)) so that state updates happen immediately when agencyId
changes; update the effect that references setError, setRequests,
setBackupRequests, setLoading and agencyId accordingly.
In `@apps/responder-app/src/services/telemetry-client.ts`:
- Around line 19-33: startTracking/stopTracking currently use a single mutable
currentCallback and refCount, which causes later callers to overwrite earlier
subscribers and prevents correct removal; change the implementation to fan-out
updates by storing subscribers in a Set or Map (e.g., subscribers: Set<(loc:
TelemetryLocation)=>void> or Map<id,callback>), have startTracking add the
provided onLocation to that collection (and return a subscriber id or token if
needed), increment refCount only for bookkeeping, and have stopTracking remove
the exact callback (or remove by id/token) and only clear watcherId/webWatchId
when the subscribers collection becomes empty; update usages of currentCallback,
watcherId, and webWatchId accordingly so location updates iterate over all
subscribers rather than calling a single callback.
In `@functions/src/callables/responder-shift-handoff.ts`:
- Around line 20-30: The code defines a local ResponderShiftHandoff interface
and casts Firestore data with "as ResponderShiftHandoff", bypassing the shared
responderShiftHandoffDocSchema; update the flow to validate incoming and stored
documents using the shared validator: before calling tx.set(...) parse and
validate the payload with responderShiftHandoffDocSchema (throw/return on
validation failure) and on reads parse snap.data() with
responderShiftHandoffDocSchema instead of casting; replace usages of the
ResponderShiftHandoff cast with the validated result and ensure you do not
swallow validation errors (apply same change for the other occurrences around
lines noted: the accept flow and the other two spots).
In `@infra/firebase/database.rules.json`:
- Around line 12-14: The responder_index ".write" rule currently only checks
auth.uid and responders/{uid}/active, so update the ".write" condition to
additionally require auth.token.role === 'responder' and
auth.token.accountStatus === 'active' (keep the existing
root.child('responders').child(auth.uid).child('active').val() === true check);
this ensures only actively authenticated responder tokens can mutate the
index—modify the ".write" expression where responder_index is defined and
reference auth.token.role and auth.token.accountStatus alongside auth.uid and
the responders/{uid}/active lookup.
- Line 7: The security rule uses the unsupported `in` operator for enum checks
and will fail deployment; replace the `... .val() in [...]` checks with explicit
equality chains joined by `||` for both enums: update the
`newData.child('motionState')` check to compare `.val()` to 'moving', 'walking',
'still', and 'unknown' using `===`/`||`, and similarly update the
`newData.child('telemetryStatus')` check to compare `.val()` to 'active',
'degraded', 'stale', and 'offline' using `===`/`||`, keeping the rest of the
`.validate` expression intact.
In `@infra/firebase/firestore.rules.template`:
- Around line 229-233: The rule for updating responders is too restrictive: it
only allows the three keys
['availabilityStatus','availabilityReason','updatedAt'] and requires updatedAt
to be an int, which blocks client calls that set serverTimestamp() and telemetry
updates that set lastTelemetryAt. Modify the update rule (the block using uid(),
rUid, and request.resource.data.diff(resource.data).affectedKeys()) to also
allow 'lastTelemetryAt' in the allowed keys and relax the updatedAt type check
to accept Firestore Timestamps (e.g., change "request.resource.data.updatedAt is
int" to allow timestamp types or accept both int and timestamp), so both
responder availability updates (which use serverTimestamp()) and telemetry
merges (which write lastTelemetryAt) are permitted.
🪄 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: 88a2a4e8-4e55-41d9-aeb7-77a62f56a166
⛔ Files ignored due to path filters (3)
packages/shared-validators/lib/coordination.js.mapis excluded by!**/*.mappackages/shared-validators/lib/dispatches.test.js.mapis excluded by!**/*.mappackages/shared-validators/lib/responders.js.mapis excluded by!**/*.map
📒 Files selected for processing (24)
apps/admin-desktop/src/hooks/useAgencyAssistanceQueue.tsapps/admin-desktop/src/hooks/useEligibleResponders.tsapps/admin-desktop/src/pages/AgencyAssistanceQueuePage.tsxapps/admin-desktop/src/pages/DispatchModal.tsxapps/admin-desktop/src/pages/RosterPage.tsxapps/admin-desktop/src/utils/freshness.tsapps/responder-app/android/app/src/main/AndroidManifest.xmlapps/responder-app/src/hooks/useDispatch.tsapps/responder-app/src/hooks/useResponderAvailability.tsapps/responder-app/src/hooks/useResponderTelemetry.tsapps/responder-app/src/hooks/useSubmitResponderWitnessedReport.tsapps/responder-app/src/pages/ResponderWitnessReportPage.tsxapps/responder-app/src/pages/SosPage.tsxapps/responder-app/src/services/push-client.tsapps/responder-app/src/services/telemetry-client.tsfunctions/src/callables/responder-roster.tsfunctions/src/callables/responder-shift-handoff.tsinfra/firebase/database.rules.jsoninfra/firebase/firestore.rules.templatepackages/shared-validators/lib/coordination.jspackages/shared-validators/lib/dispatches.test.jspackages/shared-validators/lib/responders.jspackages/shared-validators/lib/state-machines/dispatch-states.d.tspackages/shared-validators/lib/state-machines/dispatch-states.js
Comment on lines
+229
to
+233
| allow update: if uid() == rUid | ||
| && request.resource.data.diff(resource.data).affectedKeys() | ||
| .hasOnly(['availabilityStatus']); | ||
| .hasOnly(['availabilityStatus', 'availabilityReason', 'updatedAt']) | ||
| && request.resource.data.availabilityStatus in ['available', 'on_duty', 'off_duty', 'on_break', 'unavailable'] | ||
| && request.resource.data.updatedAt is int; |
Contributor
There was a problem hiding this comment.
Enforce the same availability invariants at the rule boundary.
A direct Firestore write can still set availabilityStatus to off_duty, on_break, or unavailable without a non-empty availabilityReason, and this rule does not require an active responder session at all. That bypasses the client guard in apps/responder-app/src/hooks/useResponderAvailability.ts.
As per coding guidelines, “Defensive programming: Assume external input is malicious or broken. Validate at the boundary.”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@infra/firebase/firestore.rules.template` around lines 229 - 233, The update
rule must additionally enforce that when
request.resource.data.availabilityStatus is one of
['off_duty','on_break','unavailable'] the
request.resource.data.availabilityReason is a non-empty string, and it should
also require an active responder session exists for rUid (e.g. check
exists(/databases/$(database)/documents/responderSessions/$(rUid)) and that
get(...).data.active == true); update the allow update condition that currently
references uid(), rUid, request.resource.data.availabilityStatus,
request.resource.data.updatedAt and request.resource.data.diff(...) to include
these two checks so the rule mirrors the client invariant.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
infra/firebase/firestore.rules (1)
1-2:⚠️ Potential issue | 🔴 CriticalCI blocker: Firestore rules remain out of sync with the builder.
The pipeline error indicates
firestore.rulesdoesn't match the output ofscripts/build-rules.ts. Although a past review comment was marked as addressed, the error persists. Please run:pnpm exec tsx scripts/build-rules.tsThen verify the generated output includes your changes and commit the result. If manual edits were applied after regeneration, those changes need to be made in
build-rules.tsinstead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@infra/firebase/firestore.rules` around lines 1 - 2, The committed firestore.rules file is out of sync with the generated output; run the generator (pnpm exec tsx scripts/build-rules.ts) to regenerate the rules, verify the resulting firestore.rules includes your intended changes, and commit that generated file; if you made manual edits directly to firestore.rules, move those edits into scripts/build-rules.ts so future runs produce the same output before committing.
♻️ Duplicate comments (1)
infra/firebase/firestore.rules (1)
254-256:⚠️ Potential issue | 🟠 MajorAdd type and length validation for
availabilityReason.The rule restricts which fields can be updated and validates
availabilityStatusandupdatedAt, butavailabilityReasonlacks type/length validation. A malicious client could write arbitrary data types or excessively long strings to this field.The validator schema (
packages/shared-validators/src/responders.ts) allowsavailabilityReason: z.string().max(500).optional().🔒 Suggested fix
allow update: if uid() == rUid && request.resource.data.diff(resource.data).affectedKeys() .hasOnly(['availabilityStatus', 'availabilityReason', 'updatedAt']) && request.resource.data.availabilityStatus in ['available', 'on_duty', 'off_duty', 'on_break', 'unavailable'] - && request.resource.data.updatedAt is int; + && request.resource.data.updatedAt is int + && ( + !('availabilityReason' in request.resource.data.diff(resource.data).affectedKeys()) + || request.resource.data.availabilityReason == null + || ( + request.resource.data.availabilityReason is string + && request.resource.data.availabilityReason.size() <= 500 + ) + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@infra/firebase/firestore.rules` around lines 254 - 256, The availabilityReason field lacks type/length checks in the update rule; update the rule that currently checks availabilityStatus and updatedAt to also require that either availabilityReason is omitted or it is a string no longer than 500 characters. Concretely, augment the existing boolean expression (the && chain that includes request.resource.data.availabilityStatus and request.resource.data.updatedAt) with a clause like: ( !request.resource.data.keys().hasAny(['availabilityReason']) || (request.resource.data.availabilityReason is string && request.resource.data.availabilityReason.size() <= 500) ), referencing the same availabilityReason/availabilityStatus/updatedAt symbols so the rule enforces z.string().max(500).optional() behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@infra/firebase/firestore.rules`:
- Around line 254-256: The responder-roster callables are writing the wrong
field name (`availability`) causing clients to read `availabilityStatus` as
undefined; update the two assignments in responder-roster.ts so they write
`availabilityStatus` instead of `availability` — specifically replace the
occurrence that sets `availability: 'off_duty'` with `availabilityStatus:
'off_duty'` and the occurrence that sets `availability: status` with
`availabilityStatus: status` so the callable writes match the Firestore rules
and client schema.
---
Outside diff comments:
In `@infra/firebase/firestore.rules`:
- Around line 1-2: The committed firestore.rules file is out of sync with the
generated output; run the generator (pnpm exec tsx scripts/build-rules.ts) to
regenerate the rules, verify the resulting firestore.rules includes your
intended changes, and commit that generated file; if you made manual edits
directly to firestore.rules, move those edits into scripts/build-rules.ts so
future runs produce the same output before committing.
---
Duplicate comments:
In `@infra/firebase/firestore.rules`:
- Around line 254-256: The availabilityReason field lacks type/length checks in
the update rule; update the rule that currently checks availabilityStatus and
updatedAt to also require that either availabilityReason is omitted or it is a
string no longer than 500 characters. Concretely, augment the existing boolean
expression (the && chain that includes request.resource.data.availabilityStatus
and request.resource.data.updatedAt) with a clause like: (
!request.resource.data.keys().hasAny(['availabilityReason']) ||
(request.resource.data.availabilityReason is string &&
request.resource.data.availabilityReason.size() <= 500) ), referencing the same
availabilityReason/availabilityStatus/updatedAt symbols so the rule enforces
z.string().max(500).optional() behavior.
🪄 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: a18e56d0-53fc-4efe-aa73-bb01f3b0839e
📒 Files selected for processing (1)
infra/firebase/firestore.rules
…onderShiftHandoffDocSchema export - Add .nonnegative() to createdAt/expiresAt in responderShiftHandoffDocSchema - Add unable_to_complete transition tests in dispatches.test.ts - Export responderShiftHandoffDocSchema and ResponderShiftHandoffDoc from index.ts - Rebuild lib/ outputs
…relaxation - Replace unsupported RTDB 'in' operator with explicit || chains for motionState/telemetryStatus - Add responder_index write guard requiring active responder role - Firestore: add lastTelemetryAt to responder self-update allowlist - Firestore: accept updatedAt as int or timestamp - Add availabilityReason presence validation when status != available - Regenerate firestore.rules from template
…a hardening - responder-roster: write availabilityStatus instead of legacy 'availability' in suspend/revoke and bulk override - responder-shift-handoff: import responderShiftHandoffDocSchema from shared-validators - responder-shift-handoff: add toUid slash traversal guard and handoffId hex regex - responder-shift-handoff: parse with schema on write, safeParse on read instead of casting - responder-shift-handoff: use number ms for createdAt/expiresAt with idempotency payload comparison
…etry subscriber model - push-client: wrap Promise.all addListener race in try/catch with rejectOnce guard - push-client: await PushNotifications.register() instead of void+catch - push-client: validate dispatchId with regex ^[a-zA-Z0-9_-]+$ in subscribeNotificationTap - telemetry-client: refactor from refCount/single-callback to Set-based subscribers - telemetry-client: return unsubscribe function on native path, fix refCount cleanup leaks - telemetry-client: delete subscriber from Set on setup failure to prevent leaks
…ut validation - useDispatch: add latestDispatchIdRef stale-request guard in refresh() - useResponderAvailability: write Date.now() integer timestamp, clear reason with deleteField() - useResponderTelemetry: add cancelled checkpoints after battery read, after parse, before RTDB set, before Firestore setDoc, before responder_index write - useSubmitResponderWitnessedReport: use normalized dispatchId in payload, validate photoUrl with new URL() and protocol check
- ResponderWitnessReportPage: trim description before required-field guard - SosPage: replace raw error.message with sanitized user-facing message - SosPage: add role=alert and aria-live=assertive to error paragraph
- useAgencyAssistanceQueue: track requestsReady + backupReady flags, setLoading(false) only when both listeners resolved - useAgencyAssistanceQueue: surface backup subscription errors into error state - useEligibleResponders: remove stale freshness field from state, sort by lastTelemetryAt directly
…ic re-evaluation - DispatchModal: compute freshness from lastTelemetryAt at render instead of reading stale persisted value - RosterPage: add 30-second interval tick to force re-render and re-evaluate freshness labels
This use of variable 'cancelled' always evaluates to false. Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
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
responderTelemetryPayloadSchema); motion-aware sampling (15s moving → 30s walking → 120s still); RTDB self-only write rules with ±60s timestamp plausibility; writesresponder_index/{uid}for projection jobshared_projection/{municipalityId}/{uid}with 100m grid rounding and 4-tier freshness (fresh/degraded/stale/offline + 90s TTL cleanup)submitResponderWitnessedReport,triggerSOS,requestBackup,markDispatchUnableToComplete— all idempotent with rate limiting and audit eventsNotificationRouterinside layout route souseNavigatehas router contextinitiateResponderHandoff,acceptResponderHandoff); availability management (available/unavailable/off_duty)suspendResponder,revokeResponder,bulkAvailabilityOverridecallables with idempotency and rate limiting; specialization tags in dispatch selectione2e-tests/specs/responder-phase-6.spec.tsReview-Pass Fixes (post-implementation)
suspendResponder,revokeResponder,bulkAvailabilityOverrideinresponder-roster.tsresponder_indexnever writtenresponder_index/{uid}on each location emittriggerSOScheckRateLimit(5/5min) before transactionclaims.activewrong fieldclaims.accountStatus !== 'active'useResponderTelemetryDispatchDetailPage;TelemetryProviderinApp.tsxis canonicalaccept-dispatch.test.tsandadvance-dispatch.test.tsenforceAppCheckhardcodedtrueprocess.env.NODE_ENV === 'production'reasonallowed in handoff.trim().min(1)to validationwindow.location.assignreloads pageNotificationRouterinto layout route insideRouterProviderinitiateResponderHandoffCore+acceptResponderHandoffCoreTest Plan
pnpm --filter @bantayog/shared-validators test— 227/227 passfirebase emulators:exec --only firestore,database,storage "pnpm --filter @bantayog/functions exec vitest run src/__tests__/callables/responder-shift-handoff.test.ts src/__tests__/callables/trigger-sos.test.ts src/__tests__/callables/request-backup.test.ts"— 20/20 passpnpm --filter @bantayog/functions lint && typecheck— cleanpnpm --filter @bantayog/responder-app lint && typecheck— cleanpnpm --filter @bantayog/admin-desktop lint && typecheck— cleanResidual Known Gaps (documented in progress.md)
APP_VERSIONis hardcoded'0.0.0'in telemetry hook — needs Vite env var wiring in a follow-upSummary by CodeRabbit
New Features
Bug Fixes
Documentation