feat(phase5): Cluster B — Inter-Agency Coordination#64
Merged
Conversation
…lineAgencyAssistance Implements B.1 agency-assistance callable trio: - requestAgencyAssistanceCore: municipal_admin creates agency_assistance_requests doc + command_channel_thread in a transaction; idempotent via withIdempotency - acceptAgencyAssistanceCore: agency_admin accepts; idempotent on already-accepted - declineAgencyAssistanceCore: agency_admin declines with reason; closes associated thread All use withIdempotency, bantayogErrorToHttps wrapping, and onCall with region=asia-southeast1. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
- Query active agency admins for target agency and include their UIDs in participantUids (municipal admin + all agency admins) - Use municipalityId as label placeholder for requestedByMunicipality since a names lookup collection is out of Phase 5 scope Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…uestType enum Uppercasing and casting agencyId directly to requestType would crash or produce invalid data for arbitrary strings like "mdrr" or "mmda". Add a pre-transaction guard that throws BantayogError(INVALID_ARGUMENT) when the label is not in ['BFP','PNP','PCG','RED_CROSS','DPWH','OTHER']. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
agency_assistance_requests schema defines escalatedAt as z.number().int(), matching createdAt which is stored as a Firestore Timestamp (Unix ms). Writing a raw number instead of going through Timestamp.toMillis() causes downstream code that calls .toDate()/.toMillis() on the field to crash.
…reason Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…ssistanceQueuePage Adds a 5th test that clicks the Decline button, types a reason, and verifies mockCallable was invoked with requestId + reason. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
enterFieldMode: auth_time freshness check (4h), 12h expiry, writes field_mode_sessions doc. exitFieldMode: idempotent clear (isActive false). auth_time string→number coercion guard. Fixes: municipalityId omit pattern for exactOptionalPropertyTypes, threadDoc optional guard in decline path, schemaVersion incompatible with Omit, unnecessary async in test. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Real-time Firestore onSnapshot subscription to field_mode_sessions/{uid}.
60-second interval checks session expiry via Date.now() > expiresAt.
exitFnRef avoids stale closure on the exit callable.
Explicit type narrowing for snapshot data fields.
vi.hoisted() pattern for Firebase mock functions in tests.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Fetches municipality and barangay boundaries from Overpass API and writes: - municipality-boundaries.geojson (12 municipalities) - barangay-boundaries.geojson (~282 barangays) - boundary-geohash-set.ts (6-char geohash cells within 2km of municipal boundaries) Hybrid node+relation approach: 4 municipalities have boundary polygons (Daet, Paracale, Santa Elena, Vinzons); 8 use buffered centroid fallback (Capalonga, Basud, Jose Panganiban, Labo, Mercedes, San Lorenzo Ruiz, San Vicente, Talisay). Barangay parent municipalities are derived via turf.point-in-polygon against municipality polygons/centroids. Run: pnpm exec tsx scripts/extract-boundaries.ts Requires network access to Overpass API (overpass-api.de). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…rt sharing Adds the shareReport onCall function that allows municipal_admin or provincial_superadmin to share verified reports with other municipalities. Key design decisions: - Uses withIdempotency guard to prevent duplicate shares from retried calls - Plain array merge instead of Admin SDK FieldValue.arrayUnion (SDK incompatibility with client SDK Transaction in test harness) - All transaction reads precede writes (Firestore requirement) - Creates report_sharing doc, events subcollection entry, and command_channel_threads doc with threadType=border_share in a single transaction - Updates report_ops.visibility to scope=shared on successful share Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…ffer check, auto event log
…Panel with 50-msg limit
- Replace HttpsError throws with BantayogError in addCommandChannelMessageCore - Update wrapper catch block to match accept-dispatch pattern (instanceof check) - Fix test afterAll to use optional chaining to prevent crash when emulator not running - Update test assertions to use BantayogErrorCode values
… shape in addCommandChannelMessageCore - Reject suspended users (accountStatus !== 'active') before participant check - Validate participantUids is a non-null object before indexing to avoid runtime TypeError - Addresses code quality review findings for Task 9
…n and shared-data
![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
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 13 minutes and 52 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (23)
📒 Files selected for processing (34)
📝 WalkthroughWalkthroughAdds agency-assistance workflows, field-mode session management, command-channel messaging, border auto-share geospatial logic, dispatch responder refactors, admin sweep scheduling, frontend components/hooks/pages, schema updates, and extensive tests across callables, triggers, scheduled jobs, and Firestore rules. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 38
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
functions/lib/callables/dispatch-responder.js (1)
122-127: 🧹 Nitpick | 🔵 TrivialApply rollout gating before staging this callable change set.
Because this touches Cloud Functions behavior with existing traffic, run dev emulator deployment + full tests, then request explicit staging approval before rollout. Based on learnings, for changes to security rules, DB indexes, deployment config, auth flows, or Cloud Functions with existing traffic: show the diff, deploy to dev emulator first, run full test suite, then request explicit staging approval (and never deploy to prod in the same authoring session).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@functions/lib/callables/dispatch-responder.js` around lines 122 - 127, This change alters Cloud Functions behavior for dispatchResponder (the onCall export configuring region, enforceAppCheck, maxInstances, and secrets), so before staging the callable change set deploy the function to the local/dev emulator and run the full test suite against that emulator; capture test results and any runtime/security regressions, update the PR with the diff and test artifacts, then request explicit staging approval (do not deploy to production in this authoring session) and only after approval perform a staged rollout to staging and re-run tests before production.
🤖 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/__tests__/field-mode-store.test.ts`:
- Around line 54-69: The test only checks that some callable ran after the
timer; tighten it to assert the hook actually invoked the exitFieldMode callable
by verifying the specific callable invocation or payload. Update the test around
useFieldModeStore + mockHttpsCallable to assert the callable name/args match the
exitFieldMode call (or spy on the exported exitFieldMode wrapper used by the
hook) instead of just checking that mockHttpsCallable was called, referencing
mockOnSnapshot, useFieldModeStore, and exitFieldMode to locate the wiring to
change.
In `@apps/admin-desktop/src/components/CommandChannelPanel.tsx`:
- Line 41: Move the getFirestore() call out of the CommandChannelPanel render
path: instead of calling getFirestore() inside the component body where const db
= getFirestore() runs on every render, obtain the Firestore instance once (e.g.,
at module scope) or memoize it with useMemo and reuse that variable (db) in the
component; update references to db in CommandChannelPanel to use the single
cached instance so the function is not invoked per render.
- Around line 77-91: The handleSend function currently calls getFunctions() on
every send; move that call out of handleSend (e.g., initialize const functions =
getFunctions() at component scope or alongside other initializations) and reuse
it inside handleSend when calling httpsCallable; update the httpsCallable call
to use httpsCallable(functions, 'addCommandChannelMessage') and remove
getFunctions() from handleSend so the functions client is created once (keep
existing idempotencyKey, input trim, and error handling in handleSend).
- Around line 43-58: The snapshot effect in useEffect
(query/collection/onSnapshot) unnecessarily lists activeThreadId in its
dependency array causing re-subscription on thread switch; remove activeThreadId
from the dependency list and implement a one-time flag (e.g., a ref like
initialSelectionDoneRef) or similar local state to ensure setActiveThreadId is
called only the first time threads are loaded; keep reportId and db as
dependencies, continue calling setThreads inside the onSnapshot callback, and
use the ref to guard the logic that sets the first thread so switching
activeThreadId doesn’t recreate the listener.
In `@apps/admin-desktop/src/components/ReconnectBanner.tsx`:
- Around line 7-10: The banner in ReconnectBanner should be accessible and
defensive: mark the emoji span as decorative for assistive tech (e.g., set it to
be hidden from screen readers) and give the container an appropriate
status/alert semantic (e.g., role="status" or role="alert" / aria-live as
required). Also validate and normalize actionLabel before rendering in the
"Connect to {actionLabel}" span — trim whitespace and fall back to an explicit
default (e.g., "service" or "your account") if empty so you never render
"Connect to ". Update the relevant JSX elements (the emoji span, the container
div, and the action label variable/usage) in ReconnectBanner.tsx accordingly.
In `@apps/admin-desktop/src/hooks/useFieldModeStore.ts`:
- Around line 47-50: The interval callback in setInterval currently keeps
calling void exitFnRef.current?.() after expiry and will repeat every interval;
modify the logic in useFieldModeStore where setInterval is created (the interval
id `id` and `expiresAt` handling) to stop further calls by clearing the interval
once expiry is detected (call clearInterval(id)) or set a one-time timeout
instead; ensure the cleanup also nulls/clears the interval id and relies on
exitFnRef.current (or the exitFieldMode function) only once so duplicate
exitFieldMode invocations are prevented.
- Around line 18-19: When uid is falsy the code currently returns early leaving
previous field-mode state in the store; update the useFieldModeStore logic so
that when !uid you explicitly reset local state to its initial/default values
and unsubscribe any active listener instead of just returning. Locate the uid
check and the ref = doc(getFirestore(), 'field_mode_sessions', uid) usage in
useFieldModeStore, and replace the early return with calls to clear the
field-mode store (e.g., set state to initialState or null for mode/user) and
cleanup any subscription/unsubscribe handler tied to ref.
In `@apps/admin-desktop/src/pages/AgencyAssistanceQueuePage.test.tsx`:
- Around line 75-107: Update the tests to assert the specific httpsCallable
action name is invoked for each path: when clicking Accept in
AgencyAssistanceQueuePage assert mockCallable was called with the action name
'acceptAgencyAssistance' (alongside any payload/assertions already present), and
when submitting Decline assert mockCallable was called with the action name
'declineAgencyAssistance' and the payload containing requestId 'ar1' and reason
'Too far'; keep using mockCallable and the existing payload assertions but
add/replace checks to include the first argument being the expected function
name.
In `@apps/admin-desktop/src/pages/AgencyAssistanceQueuePage.tsx`:
- Around line 35-39: The useEffect in AgencyAssistanceQueuePage returns early
when agencyId is falsy but never clears the component's loading state, leaving
non-agency users stuck on "Loading…"; update the early-return branch in the
useEffect (inside the AgencyAssistanceQueuePage component) to call
setLoading(false) (or route/redirect away) before returning so that the loading
state is cleared for non-agency users.
In `@functions/lib/__tests__/rules/coordination.rules.test.js`:
- Around line 210-249: The two describe blocks testing
participant/non-participant read access are duplicated; consolidate the
duplicate scenarios by merging the describe titled
"command_channel_threads/messages — participant map key lookup" with the earlier
one (the tests using thread-1/msg-1) or parameterize the tests to run against
both fixtures instead of repeating identical it() cases; update the beforeAll
that creates the fixture documents (the setDoc calls creating 'thread-2'/'msg-2'
or reuse 'thread-1'/'msg-1') and remove the redundant describe or add a
clarifying comment if both fixtures must remain for distinct reasons.
In `@functions/lib/__tests__/rules/report-messages.rules.test.js`:
- Around line 41-44: Add a negative test that ensures an agency_admin whose
agencyId is NOT present in the report_ops.agencyIds is denied read access:
create an authed client like in the existing test but use staffClaims with an
agencyId not listed in the report's report_ops.agencyIds, then assert denial
with assertFails(getDoc(doc(...))) for the same document path
('reports','report-1','messages','msg-1')); reference existing helpers
staffClaims, authed, doc, getDoc, assertFails to mirror the positive case but
expect failure.
In `@functions/lib/__tests__/rules/report-notes.rules.test.js`:
- Around line 71-74: The test 'allows muni admin to read notes' only checks the
happy path; add a negative case by seeding a report_notes document for a
different municipality (e.g., municipalityId: 'other-muni') and then assert that
the daet-admin cannot read it. Use the admin client to write a note (via
collection(dbAdmin, 'report_notes') / addDoc or setDoc) with municipalityId !==
'daet', then in the authed env (const db = authed(env, 'daet-admin',
staffClaims(...))) call getDocs(collection(db, 'report_notes')) or getDoc for
that specific seeded doc and assertFails on the read attempt instead of
assertSucceeds. Ensure the seed runs before the assertion and clean up if
necessary.
In `@functions/lib/__tests__/rules/report-sharing.rules.test.js`:
- Around line 107-116: The test "a second share appends a second event without
overwriting first" is order-dependent; make it self-contained by creating both
event documents inside this test (use addDoc/collection with distinct payloads
or targetMunicipalityId values using validEvent as base) instead of relying on a
prior test, then read the events via env.withSecurityRulesDisabled and assert
the exact count equals 2 (use getDocs(collection(...)) and
expect(snap.size).toBe(2)); ensure you reference the same document path
'report_sharing','r-share-1','events' and keep using addDoc, validEvent, and
getDocs as in the current test.
In `@functions/lib/callables/dispatch-responder-validation.js`:
- Around line 2-8: getActorMunicipalityIds currently prefers
claims.municipalityId over claims.permittedMunicipalityIds and discards the
latter when both exist; change it to merge both sources into a single
deduplicated array so no valid municipality IDs are lost. Update the
getActorMunicipalityIds function to collect claims.municipalityId (if present)
and spread claims.permittedMunicipalityIds (if present), deduplicate the
combined list, and return the resulting array (or an empty array) while keeping
existing optional chaining/null checks (references:
deps.actor.claims.municipalityId, deps.actor.claims.permittedMunicipalityIds,
getActorMunicipalityIds).
In `@functions/lib/callables/dispatch-responder.js`:
- Around line 40-42: The current guard unconditionally throws a BantayogError if
deps.actor.claims.municipalityId is missing; change it so the error is only
thrown when there is neither a municipalityId claim nor a scoping
permittedMunicipalityIds list. Update the check around the
BantayogError(BantayogErrorCode.INVALID_ARGUMENT, 'municipalityId is required')
in dispatch-responder.js to allow missing deps.actor.claims.municipalityId when
permittedMunicipalityIds (the request or context variable that scopes access) is
present/non-empty, ensuring provincial-superadmin flows that rely on
permittedMunicipalityIds are accepted.
- Around line 61-70: The code reads the secret via process.env (const salt =
process.env.SMS_MSISDN_HASH_SALT) which may be undefined; replace that with the
bound secret object's .value() accessor (e.g., use SMS_MSISDN_HASH_SALT.value()
or the actual binding variable name created when you defined the secret) and
pass that value into buildSmsPayload (keeping the existing salt/null fallback
behavior).
In `@functions/lib/callables/enter-field-mode.js`:
- Around line 71-89: exitFieldMode passes the raw actor from requireAuth while
enterFieldMode rebuilds a shaped actor including auth_time, causing an
inconsistency; update exitFieldMode to mirror enterFieldMode by reconstructing
the actor object (using requireAuth result) to include the same custom
claims/auth_time shape before calling exitFieldModeCore so both callables pass a
uniform actor shape to exitFieldModeCore and other shared logic.
In `@functions/lib/callables/request-agency-assistance.js`:
- Around line 171-176: Rollout for changes to the callable Cloud Function
requestAgencyAssistance (and the other callables referenced at lines ~265-270
and ~377-382) must be gated: update the deployment process so the function is
first deployed to the local emulator and the full test suite is run, and block
any automatic push to staging/prod until explicit human approval is recorded;
implement this by adding an explicit emulator deployment step and test-run stage
in CI (e.g., "deploy-emulator" + "run-tests"), and require a manual
"staging-approve" gate before staging/prod deployment, and ensure deploy
scripts/CI check for an environment flag or approval token before allowing
production deploys.
- Around line 40-43: Validation normalizes agencyId to uppercase into
agencyLabel but other code paths still use the raw agencyId; normalize once at
the top (e.g., compute canonicalAgency = agencyId.toUpperCase() or similar) and
use that canonical value everywhere instead of raw agencyId (replace occurrences
used in participant lookup, authorization checks like accept/decline
comparisons, and any other references). Update references to
VALID_AGENCY_ASSISTANCE_TYPES checks to use canonicalAgency (or keep
agencyLabel) and ensure functions/methods that call/compare agency use the
canonical symbol rather than the original agencyId to avoid case mismatch bugs.
In `@functions/lib/scheduled/admin-operations-sweep.js`:
- Around line 26-28: The new scheduled function adminOperationsSweep should be
rolled out with staged safety gates: add a rollout gate (e.g., an env var or
feature flag checked at deploy/runtime) so onSchedule export of
adminOperationsSweep does not go to production until approved, and ensure
deployment workflow first deploys this function to the local emulator (or dev
project) and runs the full test suite that exercises adminOperationsSweepCore
and adminDb interactions; update CI to require a manual/staging approval step
before promoting to prod, and document the required validation steps (emulator
run, full tests, explicit staging approval) so production deployment is blocked
until those gates are completed.
- Around line 17-24: The loop awaits each escalation serially which can time out
under backlog; change the serial await of d.ref.update(...) to run updates
concurrently in controlled batches: for the toEscalate array, map each item to a
promise that calls d.ref.update({ escalatedAt: deps.now.toMillis() }) and then
logs via log(...), then await Promise.all(...) for each batch; implement
chunking (e.g., batch size 25–100) to limit concurrency and avoid overloading
the DB while ensuring all updates are awaited.
- Around line 11-15: The sweep query builds pendingAssistance by querying the
collection 'agency_assistance_requests' with equality on status and a range on
createdAt, which requires a composite collection-level index; add a composite
index entry to your Firestore indexes config (the firestore.indexes.json used by
your infra tooling) for collection "agency_assistance_requests" with fields:
status ASCENDING and createdAt DESCENDING so the query in
admin-operations-sweep.js (pendingAssistance query) will succeed without runtime
index errors.
In `@functions/lib/triggers/process-inbox-item.js`:
- Around line 76-83: The report-building logic currently pushes every
pendingMediaId/uploadId into mediaRefs even when the pending_media doc is
missing; update the code that constructs mediaRefs (where
uploadId/pendingMediaId is handled) to skip or filter out entries for which the
pending_media lookup returned no document (the branch that logs
INBOX_PENDING_MEDIA_MISSING should not add that id to mediaRefs), ensuring only
materialized attachments are persisted in report refs.
In `@functions/src/__tests__/callables/share-report.test.ts`:
- Around line 79-90: Add a test that asserts a caller with role
"municipal_admin" but missing the municipalityId claim is rejected; seed the
report (seedReportOps('r1','mercedes')), construct an actor object with role
'municipal_admin' and no municipalityId claim (similar shape to daetAdmin but
without municipalityId), call shareReportCore(adminDb, { reportId: 'r1',
targetMunicipalityId: 'labo', actor: <that actor>, idempotencyKey:
crypto.randomUUID(), now: Timestamp.fromMillis(ts) }) and expect the promise to
reject with { code: 'permission-denied' } to cover the malformed-claims
boundary.
In `@functions/src/__tests__/triggers/border-auto-share.test.ts`:
- Around line 13-15: The test is currently nondeterministic because it accepts
both snap.exists true/false and uses a point that may not be within 500m of the
stub boundary; update the test to pick a deterministic point guaranteed to be
within 500m of the stub boundary and assert the expected outcome strictly.
Specifically, change NEAR_BOUNDARY_LAT / NEAR_BOUNDARY_LNG (and recompute
NEAR_BOUNDARY_GEOHASH) to coordinates you know lie inside the 500m radius used
by the auto-share logic, and replace the permissive assertion that accepts both
outcomes with a strict assertion (e.g., expect(snap.exists).toBe(true) or false)
in the tests around border-auto-share.test.ts (also apply same fix to the other
related assertions in the 154-190 block) so the test fails when auto-share does
not run as expected.
In `@functions/src/callables/https-error.ts`:
- Around line 37-45: Add a rollout gate around the new callable auth semantics
in requireAuth by checking a deploy-time feature flag (e.g.,
process.env.ENABLE_NEW_AUTH or config.AuthRolloutStage) and emulator/runtime
indicator (e.g., process.env.FUNCTIONS_EMULATOR or a config flag) before
applying the new role-check logic; if the flag is disabled or not running in
emulator/staging, preserve the previous behavior (skip or use legacy role
handling) so production is unaffected. Update requireAuth to read the flag and
branch around the role validation (the claims.role check and permission-denied
throw) and ensure tests cover both gated and ungated paths; coordinate enabling
the flag only after running the emulator and full test suite and obtaining
explicit staging approval.
- Around line 37-45: The requireAuth function currently treats allowedRoles as
optional which permits any authenticated user when callers omit it; change the
API to force explicit authorization mode by making allowedRoles required (remove
the optional ? from the parameter) or replace it with an explicit mode
parameter, then update the logic in requireAuth to always validate against
allowedRoles (or check the explicit mode) and throw
HttpsError('permission-denied', ...) when the caller has not provided a valid
allowedRoles list; reference the requireAuth function, the allowedRoles
parameter, the role variable and the existing HttpsError throws when applying
the change.
In `@functions/src/callables/request-agency-assistance.ts`:
- Around line 167-186: The created agency assistance request object lacks the
required schemaVersion field; update the object passed to tx.set in
request-agency-assistance (the block that writes to requestRef using tx.set and
constructs an AgencyAssistanceRequestDoc) to include schemaVersion: 1 so new
documents match the seeded fixtures and the rest of the code expects; ensure the
satisfies type still omits only the listed response fields and that
schemaVersion is present on the created document.
- Around line 69-79: Normalize agencyId once (e.g., set a normalizedAgencyId =
agencyId.toUpperCase() or similar) and use that normalized value everywhere: for
validation against VALID_AGENCY_ASSISTANCE_TYPES, for user/agency lookups, and
when setting/ persisting targetAgencyId so casing is consistent; update all
references in this file that currently use the raw agencyId (including the
validation block using agencyLabel, the code paths that look up agency
admins/users, and where targetAgencyId is stored) to use the single normalized
variable.
In `@functions/src/callables/share-report.ts`:
- Around line 57-59: When checking municipal ownership in shareReport, tighten
the conditional to fail when a municipal admin's claim lacks a municipalityId;
instead of only comparing ops.municipalityId !== municipalityId when
municipalityId is defined, update the check so that if isMuniAdmin and
(municipalityId === undefined || ops.municipalityId !== municipalityId) you
throw the HttpsError('permission-denied', 'report belongs to a different
municipality'); reference the variables isMuniAdmin, municipalityId,
ops.municipalityId and the thrown HttpsError in the share-report handler.
- Around line 74-83: The current pre-check using
existingSnap/existing/mergedSharedWith is non-atomic and can race; move the
read/check/update logic into a Firestore transaction (use runTransaction) so you
read the report_sharing doc inside the transaction, check if
existing.includes(targetMunicipalityId), and only if not present compute
mergedSharedWith and perform the update plus any event/thread writes within the
same transaction; ensure you remove the external pre-check and use the
transaction's snapshot to decide whether to create new event/thread to prevent
double-writes.
- Around line 149-151: The catch block currently unsafely casts err to
bantayogErrorToHttps input; replace that with explicit type checks: if err is an
instance of HttpsError rethrow it unchanged, if err is an instance of
BantayogError map it via bantayogErrorToHttps and throw that, otherwise rethrow
the original err. Update the catch in share-report.ts to use instanceof checks
for HttpsError and BantayogError rather than the unsafe cast so existing
HttpsError codes/messages are preserved.
In `@functions/src/scheduled/admin-operations-sweep.ts`:
- Around line 27-35: The current code fetches pendingAssistance and then filters
in-memory by !d.data().escalatedAt which will not scale; replace the in-memory
filter by adding the escalatedAt predicate to the Firestore query (e.g., include
where('escalatedAt', '==', null) alongside the existing time/age predicate that
uses deps.now) so only non‑escalated docs are returned, then iterate the query
snapshot and call d.ref.update({ escalatedAt: deps.now.toMillis() }) and log as
before; if Firestore rejects the combined query, create the required composite
index (for createdAt/age + escalatedAt) to support this compound query.
In `@functions/src/triggers/border-auto-share.ts`:
- Around line 91-100: The auto-created 'border_share' thread is written with
participantUids: {} making it inaccessible; update the tx.set call that writes
the thread (threadRef, reportId, threadType: 'border_share') to populate
participantUids with the source and target municipality participant IDs (include
entries for the source and target participants using the unique IDs you already
have in scope such as sourceMuniId and targetMuniId, e.g. participantUids: {
[sourceMuniId]: true, [targetMuniId]: true } or the app's standard participant
key format), or alternatively skip creating the command thread for
auto-shares—prefer populating participantUids so both municipalities can
read/write the thread.
- Around line 121-128: The catch block that swallows failures importing
'@bantayog/shared-data' should at minimum log the exception so auto-share
regressions are visible; update the try/catch around the dynamic import (the
import('@bantayog/shared-data') block that sets boundaryGeohashSet) to catch the
error and emit a warning including the error object (use the existing logger if
available, e.g., functions.logger or processLogger, or fallback to console.warn)
while keeping the current fallback to new Set() so boundaryGeohashSet remains
defined.
- Around line 50-58: The code reads existingSnap/existingData and computes
alreadySharedWith once before iterating boundaries.features, causing later
updates to report_sharing.sharedWith and report_ops.visibility.sharedWith to
overwrite earlier auto-shares; instead, inside the loop (or within the
transaction that writes) re-read the current document or fetch the up-to-date
sharedWith field before each update and perform a union/merge (add missing
targetMuniId only if not present) so you append without clobbering; apply this
change where existingSnap/existingData/alreadySharedWith are used and to the
update logic that writes report_sharing.sharedWith and
report_ops.visibility.sharedWith (also cover similar code between lines 72-106).
In `@scripts/extract-boundaries.ts`:
- Around line 212-223: The code currently writes municipality-boundaries.geojson
via writeFileSync using muniCollection before verifying the expected
muniFeatures length; move the integrity check so the if (muniFeatures.length !==
12) guard runs before calling writeFileSync and console.log, and only write the
file (and log success) when the count equals 12 (and retain the existing error
log and process.exit(1) on failure); reference writeFileSync, muniCollection,
muniFeatures, and the 12-municipality guard to find where to relocate the write
operation.
- Around line 92-111: The relation polygon builder is silently failing because
Overpass queries use "out skel qt;" (no way.geometry) and the OverpassElement
type lacks node coords; update the two boundary queries to "out geom qt;" so
way.geometry is populated and add lat?: number and lon?: number to the
OverpassElement interface; also replace the manual ring-stitching logic in
buildPolygonFromRelation (the loop that maps way.geometry, closes each way, and
unshifts/pushes rings) with a robust multipolygon reconstruction using a library
such as osmtogeojson (convert the relation + member ways to GeoJSON via
osmtogeojson and return the resulting Polygon or MultiPolygon) to correctly
handle multi-way boundaries.
---
Outside diff comments:
In `@functions/lib/callables/dispatch-responder.js`:
- Around line 122-127: This change alters Cloud Functions behavior for
dispatchResponder (the onCall export configuring region, enforceAppCheck,
maxInstances, and secrets), so before staging the callable change set deploy the
function to the local/dev emulator and run the full test suite against that
emulator; capture test results and any runtime/security regressions, update the
PR with the diff and test artifacts, then request explicit staging approval (do
not deploy to production in this authoring session) and only after approval
perform a staged rollout to staging and re-run tests before production.
🪄 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: 895e9b4e-87cd-46a6-88e5-4182d900eaef
⛔ Files ignored due to path filters (39)
functions/lib/__tests__/callables/agency-assistance.test.d.ts.mapis excluded by!**/*.mapfunctions/lib/__tests__/callables/agency-assistance.test.js.mapis excluded by!**/*.mapfunctions/lib/__tests__/callables/field-mode.test.d.ts.mapis excluded by!**/*.mapfunctions/lib/__tests__/callables/field-mode.test.js.mapis excluded by!**/*.mapfunctions/lib/__tests__/rules/coordination.rules.test.js.mapis excluded by!**/*.mapfunctions/lib/__tests__/rules/field-mode-sessions.rules.test.d.ts.mapis excluded by!**/*.mapfunctions/lib/__tests__/rules/field-mode-sessions.rules.test.js.mapis excluded by!**/*.mapfunctions/lib/__tests__/rules/report-messages.rules.test.d.ts.mapis excluded by!**/*.mapfunctions/lib/__tests__/rules/report-messages.rules.test.js.mapis excluded by!**/*.mapfunctions/lib/__tests__/rules/report-notes.rules.test.d.ts.mapis excluded by!**/*.mapfunctions/lib/__tests__/rules/report-notes.rules.test.js.mapis excluded by!**/*.mapfunctions/lib/__tests__/rules/report-sharing.rules.test.js.mapis excluded by!**/*.mapfunctions/lib/__tests__/scheduled/admin-operations-sweep.test.d.ts.mapis excluded by!**/*.mapfunctions/lib/__tests__/scheduled/admin-operations-sweep.test.js.mapis excluded by!**/*.mapfunctions/lib/__tests__/sms-inbound.test.js.mapis excluded by!**/*.mapfunctions/lib/__tests__/triggers/process-inbox-item.test.js.mapis excluded by!**/*.mapfunctions/lib/callables/advance-dispatch.js.mapis excluded by!**/*.mapfunctions/lib/callables/decline-dispatch.js.mapis excluded by!**/*.mapfunctions/lib/callables/dispatch-responder-notify.d.ts.mapis excluded by!**/*.mapfunctions/lib/callables/dispatch-responder-notify.js.mapis excluded by!**/*.mapfunctions/lib/callables/dispatch-responder-validation.d.ts.mapis excluded by!**/*.mapfunctions/lib/callables/dispatch-responder-validation.js.mapis excluded by!**/*.mapfunctions/lib/callables/dispatch-responder-writes.d.ts.mapis excluded by!**/*.mapfunctions/lib/callables/dispatch-responder-writes.js.mapis excluded by!**/*.mapfunctions/lib/callables/dispatch-responder.d.ts.mapis excluded by!**/*.mapfunctions/lib/callables/dispatch-responder.js.mapis excluded by!**/*.mapfunctions/lib/callables/enter-field-mode.d.ts.mapis excluded by!**/*.mapfunctions/lib/callables/enter-field-mode.js.mapis excluded by!**/*.mapfunctions/lib/callables/request-agency-assistance.d.ts.mapis excluded by!**/*.mapfunctions/lib/callables/request-agency-assistance.js.mapis excluded by!**/*.mapfunctions/lib/firestore/sms-inbound-processor.js.mapis excluded by!**/*.mapfunctions/lib/index.d.ts.mapis excluded by!**/*.mapfunctions/lib/index.js.mapis excluded by!**/*.mapfunctions/lib/scheduled/admin-operations-sweep.d.ts.mapis excluded by!**/*.mapfunctions/lib/scheduled/admin-operations-sweep.js.mapis excluded by!**/*.mapfunctions/lib/triggers/dispatch-sms-outbox.js.mapis excluded by!**/*.mapfunctions/lib/triggers/process-inbox-item.d.ts.mapis excluded by!**/*.mapfunctions/lib/triggers/process-inbox-item.js.mapis excluded by!**/*.mappnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (64)
apps/admin-desktop/package.jsonapps/admin-desktop/src/__tests__/command-channel-panel.test.tsxapps/admin-desktop/src/__tests__/field-mode-store.test.tsapps/admin-desktop/src/components/CommandChannelPanel.tsxapps/admin-desktop/src/components/ReconnectBanner.tsxapps/admin-desktop/src/hooks/useFieldModeStore.tsapps/admin-desktop/src/pages/AgencyAssistanceQueuePage.test.tsxapps/admin-desktop/src/pages/AgencyAssistanceQueuePage.tsxapps/admin-desktop/src/routes.tsxapps/admin-desktop/src/services/callables.tsapps/admin-desktop/tsconfig.jsonapps/admin-desktop/vitest.config.tsfunctions/lib/__tests__/callables/agency-assistance.test.d.tsfunctions/lib/__tests__/callables/agency-assistance.test.jsfunctions/lib/__tests__/callables/field-mode.test.d.tsfunctions/lib/__tests__/callables/field-mode.test.jsfunctions/lib/__tests__/rules/coordination.rules.test.jsfunctions/lib/__tests__/rules/field-mode-sessions.rules.test.d.tsfunctions/lib/__tests__/rules/field-mode-sessions.rules.test.jsfunctions/lib/__tests__/rules/report-messages.rules.test.d.tsfunctions/lib/__tests__/rules/report-messages.rules.test.jsfunctions/lib/__tests__/rules/report-notes.rules.test.d.tsfunctions/lib/__tests__/rules/report-notes.rules.test.jsfunctions/lib/__tests__/rules/report-sharing.rules.test.jsfunctions/lib/__tests__/scheduled/admin-operations-sweep.test.d.tsfunctions/lib/__tests__/scheduled/admin-operations-sweep.test.jsfunctions/lib/__tests__/sms-inbound.test.jsfunctions/lib/__tests__/triggers/process-inbox-item.test.jsfunctions/lib/callables/dispatch-responder-notify.d.tsfunctions/lib/callables/dispatch-responder-notify.jsfunctions/lib/callables/dispatch-responder-validation.d.tsfunctions/lib/callables/dispatch-responder-validation.jsfunctions/lib/callables/dispatch-responder-writes.d.tsfunctions/lib/callables/dispatch-responder-writes.jsfunctions/lib/callables/dispatch-responder.d.tsfunctions/lib/callables/dispatch-responder.jsfunctions/lib/callables/enter-field-mode.d.tsfunctions/lib/callables/enter-field-mode.jsfunctions/lib/callables/request-agency-assistance.d.tsfunctions/lib/callables/request-agency-assistance.jsfunctions/lib/index.d.tsfunctions/lib/index.jsfunctions/lib/scheduled/admin-operations-sweep.d.tsfunctions/lib/scheduled/admin-operations-sweep.jsfunctions/lib/triggers/process-inbox-item.jsfunctions/package.jsonfunctions/src/__tests__/callables/agency-assistance.test.tsfunctions/src/__tests__/callables/command-channel.test.tsfunctions/src/__tests__/callables/field-mode.test.tsfunctions/src/__tests__/callables/share-report.test.tsfunctions/src/__tests__/scheduled/admin-operations-sweep.test.tsfunctions/src/__tests__/triggers/border-auto-share.test.tsfunctions/src/callables/add-command-channel-message.tsfunctions/src/callables/enter-field-mode.tsfunctions/src/callables/https-error.tsfunctions/src/callables/request-agency-assistance.tsfunctions/src/callables/share-report.tsfunctions/src/index.tsfunctions/src/scheduled/admin-operations-sweep.tsfunctions/src/triggers/border-auto-share.tspackage.jsonpackages/shared-data/package.jsonpackages/shared-validators/lib/sms.d.tsscripts/extract-boundaries.ts
💤 Files with no reviewable changes (1)
- functions/lib/tests/sms-inbound.test.js
Comment on lines
+92
to
+111
| for (const member of relation.members) { | ||
| if (member.type !== 'way') continue | ||
| const way = wayMap.get(member.ref) | ||
| if (!way?.geometry || way.geometry.length < 3) continue | ||
|
|
||
| const ring: Array<[number, number]> = way.geometry.map((p) => [p.lng, p.lat]) | ||
| if (ring[0]![0] !== ring[ring.length - 1]![0] || ring[0]![1] !== ring[ring.length - 1]![1]) { | ||
| ring.push([ring[0]![0], ring[0]![1]]) | ||
| } | ||
|
|
||
| if (member.role === 'outer') { | ||
| rings.unshift(ring) | ||
| } else { | ||
| rings.push(ring) | ||
| } | ||
| } | ||
|
|
||
| if (rings.length === 0) return null | ||
| if (rings.length === 1) return turf.polygon([rings[0]!]) as Feature<turf.Polygon> | ||
| return turf.multiPolygon([rings]) as Feature<turf.MultiPolygon> |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
QUERY='[out:json][timeout:120];area(3601504550)->.cn;relation(area.cn)["admin_level"="6"]["boundary"="administrative"];out body;>;out skel qt;'
JSON="$(curl -sG --data-urlencode "data=${QUERY}" https://overpass-api.de/api/interpreter)"
echo "Expect false for way geometry when using out skel:"
jq '[.elements[] | select(.type=="way") | has("geometry")] | any' <<<"$JSON"
echo "Expect true for node lat/lon keys:"
jq '[.elements[] | select(.type=="node") | (has("lat") and has("lon"))] | any' <<<"$JSON"
echo "Open outer-way count (non-closed segments, indicates stitching is required):"
jq '
.elements as $els
| ([ $els[] | select(.type=="relation") | .members[]? | select(.type=="way" and .role=="outer") | .ref ]) as $outer
| [ $els[] | select(.type=="way")
| select((.id as $id | $outer | index($id)) != null)
| select((.nodes[0] != .nodes[-1])) ] | length
' <<<"$JSON"Repository: Exc1D/bantayog-alert
Length of output: 241
🏁 Script executed:
wc -l scripts/extract-boundaries.tsRepository: Exc1D/bantayog-alert
Length of output: 98
🏁 Script executed:
cat -n scripts/extract-boundaries.ts | head -120Repository: Exc1D/bantayog-alert
Length of output: 5287
🏁 Script executed:
cat -n scripts/extract-boundaries.ts | sed -n '120,230p'Repository: Exc1D/bantayog-alert
Length of output: 5005
🏁 Script executed:
cat -n scripts/extract-boundaries.ts | sed -n '230,310p'Repository: Exc1D/bantayog-alert
Length of output: 3513
Ways lack geometry in out skel responses; current code silently skips all boundary relations.
The queries at lines 128-135 and 226-233 use out skel qt;, which does not provide way.geometry. Line 95 will skip all ways, causing buildPolygonFromRelation to return null for both municipality and barangay boundaries. The OverpassElement interface is also missing lat/lon node fields.
Required fixes:
- Change
out skel qt;toout geom qt;in both boundary queries (lines 134, 232) - Add
lat?: numberandlon?: numberto OverpassElement interface (line 53) - Relation polygon stitching (lines 92–111) also breaks on multi-way boundaries: closing each way independently creates separate closed loops rather than a single stitched ring. Use a dedicated library like
osmtogeojsonfor correct multipolygon reconstruction.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/extract-boundaries.ts` around lines 92 - 111, The relation polygon
builder is silently failing because Overpass queries use "out skel qt;" (no
way.geometry) and the OverpassElement type lacks node coords; update the two
boundary queries to "out geom qt;" so way.geometry is populated and add lat?:
number and lon?: number to the OverpassElement interface; also replace the
manual ring-stitching logic in buildPolygonFromRelation (the loop that maps
way.geometry, closes each way, and unshifts/pushes rings) with a robust
multipolygon reconstruction using a library such as osmtogeojson (convert the
relation + member ways to GeoJSON via osmtogeojson and return the resulting
Polygon or MultiPolygon) to correctly handle multi-way boundaries.
Admin Desktop: - Tighten field-mode-store test to assert exitFieldMode callable name - Move getFirestore()/getFunctions() to module scope in CommandChannelPanel - Fix useEffect dependency array to avoid re-subscription on thread switch - Add accessibility semantics and empty label guard to ReconnectBanner - Reset field-mode state when uid becomes falsy - Prevent duplicate exitFieldMode calls by clearing interval after expiry - Assert specific callable names in AgencyAssistanceQueuePage tests - Clear loading state for non-agency users Functions: - Merge municipalityId + permittedMunicipalityIds in dispatch-responder-validation - Allow provincial-superadmin flows with permittedMunicipalityIds in dispatch-responder - Use SMS_MSISDN_HASH_SALT.value() instead of process.env - Normalize agencyId once and use canonical value everywhere - Add schemaVersion to AgencyAssistanceRequestDoc schema and creation - Make requireAuth allowedRoles parameter required - Move share-report read/check into Firestore transaction - Tighten municipal admin ownership check in share-report - Fix error handling with explicit type checks in share-report - Add escalatedAt filter to admin-operations-sweep query - Batch agency assistance escalation updates concurrently - Populate participantUids in border-auto-share threads - Re-read sharing state inside transaction to prevent race/overwrite - Log shared-data import failures in border-auto-share - Normalize actor shape in exitFieldMode for consistency Tests: - Consolidate duplicate coordination rules test scenarios - Add negative agency admin test for report-messages rules - Add cross-municipality denial test for report-notes rules - Make report-sharing events test self-contained - Add missing municipalityId claim test for share-report - Use deterministic coordinates in border-auto-share tests Scripts: - Move integrity check before write in extract-boundaries - Fix Overpass queries to use out geom qt for way geometry - Fix node centroid extraction to use lat/lon properties
…ocSchema test fixtures
Contributor
There was a problem hiding this comment.
Actionable comments posted: 20
♻️ Duplicate comments (2)
apps/admin-desktop/src/__tests__/field-mode-store.test.ts (1)
25-25: 🧹 Nitpick | 🔵 TrivialConsider using a deterministic timestamp.
Date.now()is evaluated at module load time, which could cause subtle issues if the module is cached or if tests run over time boundaries. For test stability, prefer a fixed timestamp.♻️ Suggested fix
-const ts = Date.now() +const ts = 1700000000000 // fixed timestamp for test determinism🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/admin-desktop/src/__tests__/field-mode-store.test.ts` at line 25, The test uses a non-deterministic timestamp assigned to the ts constant (ts = Date.now()), which can cause flaky behavior; change this to a deterministic value by replacing ts with a fixed numeric timestamp (e.g., a hardcoded epoch like 1600000000000) or by mocking Date.now() via the test framework (e.g., jest.useFakeTimers()/jest.setSystemTime) so the field-mode-store.test.ts uses a stable timestamp for assertions and snapshot comparisons.functions/lib/__tests__/rules/report-sharing.rules.test.js (1)
95-121:⚠️ Potential issue | 🟡 MinorThis count assertion is still order-coupled.
The earlier
allows muni admin to write event to subcollectiontest already inserts one event under the same path, so this case can observe 3 documents instead of 2. Make the test isolate its own parent doc/subcollection, or clear the existing events before asserting the exact count.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@functions/lib/__tests__/rules/report-sharing.rules.test.js` around lines 95 - 121, The test "a second share appends a second event without overwriting first" is order-coupled because it uses the shared path ('report_sharing','r-share-1','events') which previous tests also write to; change the test to isolate its data by using a unique parent doc id (e.g., a new id instead of 'r-share-1') or ensure teardown/cleanup of that subcollection before asserting the count, then write two events with addDoc and assert the collection size via getDocs inside env.withSecurityRulesDisabled to expect exactly 2; update references in the test body where 'r-share-1' is used so the test no longer depends on earlier tests' writes.
🤖 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/components/CommandChannelPanel.tsx`:
- Around line 41-79: Reset the selection state and ensure the active thread is
valid when reportId changes: in the useEffect that queries
'command_channel_threads' reset initialSelectionDoneRef.current = false (or set
it based on reportId) and clear messages/activeThreadId via setMessages([]) and
setActiveThreadId(null) before subscribing; after receiving found, if current
activeThreadId is missing from found choose a replacement (e.g., first.id) via
setActiveThreadId(first.id) and mark initialSelectionDoneRef.current = true;
ensure setThreads(found) is still called so UI updates correctly. Use the
existing symbols initialSelectionDoneRef, setThreads, setActiveThreadId,
setMessages and the threads-query useEffect to implement this behavior.
In `@apps/admin-desktop/src/hooks/useFieldModeStore.ts`:
- Around line 17-27: Replace the deferred setTimeout pattern in the useEffect
when uid is falsy by calling the state setters synchronously: inside the if
(!uid) branch call setIsActive(false) and setExpiresAt(null) directly and remove
the setTimeout/clearTimeout cleanup; keep the existing cleanup return only if
you reintroduce any async side effects. This targets the useEffect containing
uid, setIsActive, and setExpiresAt in useFieldModeStore.ts.
- Around line 44-47: The httpsCallable calls inside useFieldModeStore's exit and
enter callbacks call getFunctions() instead of the pre-configured functions
instance; import the named export functions from your project's firebase module
and replace getFunctions() with that functions variable in both the exit and
enter callbacks (in the useFieldModeStore file, update the import to bring in
functions from the firebase module and use it when constructing httpsCallable).
In `@apps/admin-desktop/src/pages/AgencyAssistanceQueuePage.tsx`:
- Around line 35-72: Remove the deferred reset via setTimeout and instead clear
agency-scoped state synchronously at the top of the useEffect: call
setRequests([]) and setError(null) immediately when the effect runs; if there is
no agencyId return early after that. For the truthy branch, setLoading(true)
immediately before creating the query/onSnapshot subscription (and continue to
setLoading(false) in the snapshot/error handlers). Update the useEffect
references to agencyId, setRequests, setError, setLoading, query, and onSnapshot
accordingly.
In `@functions/lib/__tests__/rules/coordination.rules.test.js`:
- Around line 142-166: The test setup currently creates the fixture document
'req-agency-match' inside one specific it() block, causing other tests
(assertFails/assertSucceeds against doc(db, 'agency_assistance_requests',
'req-agency-match')) to depend on test order; move the seeding into a shared
setup (e.g., beforeAll or beforeEach) using env.withSecurityRulesDisabled to
create the doc in collection 'agency_assistance_requests' with the same fields
(requestedByMunicipality, targetAgencyId, requestType, requestedAt), or
alternatively seed the document at the start of each dependent it() so each test
(the agency admin positive/negative, superadmin positive, and citizen negative
cases) always has the fixture available regardless of ordering or failure.
In `@functions/lib/__tests__/rules/report-notes.rules.test.js`:
- Line 3: The test file imports vitest helpers but omits the expect helper used
later; update the import statement (the one that currently imports afterAll,
beforeAll, describe, it) to also import expect from 'vitest' so assertions like
expect(hasOtherMuniNote).toBe(false) work; modify the import line that declares
afterAll/beforeAll/describe/it to include expect.
- Around line 75-90: The test "denies muni admin from reading notes for a
different municipality" is asserting client-side filtering instead of rule-level
denial; update the test in report-notes.rules.test.js to either (A) assert rule
denial by attempting a direct document read of the Mercedes note using
authed(db, ...) and assertFails(getDoc(doc(db, 'report_notes',
'<id-of-mercedes-note>'))) to ensure Firestore rules block the read, or (B) if
rules intentionally allow broad reads, rename the test to reflect that it
verifies application-level filtering (e.g., "filters out notes for other
municipalities on client") and keep the current getDocs + client-side check;
reference the test function name and the collection 'report_notes' when making
the change.
In `@functions/lib/callables/request-agency-assistance.js`:
- Around line 320-333: The current code queries command_channel_threads (where
assistanceRequestId == requestId and threadType == 'agency_assistance') outside
the Firestore transaction and then calls tx.update(threadRef) inside, creating a
TOCTOU risk; to fix, perform the query/read inside the transaction by replacing
the outside db.collection(...).where(...).limit(1).get() with a transactional
read (use tx.get on the same Query) to obtain the thread document inside the
transaction, then conditionally call tx.update(threadRef, { closedAt: nowMs,
updatedAt: nowMs }) using that transactional snapshot (references: threadSnap,
threadRef, tx.update, assistanceRequestId, threadType,
db.collection('command_channel_threads')).
In `@functions/src/__tests__/triggers/border-auto-share.test.ts`:
- Around line 190-220: The test "does not re-trigger if report already shared
with that municipality" is missing the seeded report_private.exactLocation so it
may take the early-return path; update the test to seed the same exactLocation
fixture into the report_private document (same reportId 'r1') before calling
borderAutoShareCore so the code exercises the intended branch that checks
existing sharedWith; ensure the seeded doc key and fields match the fixture used
in other tests (report_private.exactLocation) and keep opsDoc and existing
report_sharing entry unchanged.
In `@functions/src/callables/add-command-channel-message.ts`:
- Around line 38-77: Authorization is currently checked using the
pre-transaction snapshot `threadSnap` and `participantUids`, which can be stale;
re-check membership inside the transaction by reading the thread document via
the transaction (use
`tx.get(db.collection('command_channel_threads').doc(threadId))`) before
applying `tx.set(...)` and `tx.update(...)` and throw a
`BantayogError(BantayogErrorCode.FORBIDDEN, 'caller is not a thread
participant')` (or equivalent) if the transaction-read thread no longer contains
the caller in its `participantUids`; ensure this check occurs inside the
`withIdempotency` callback before writing `msgRef` so the transaction aborts if
membership changed.
- Around line 84-100: The Zod parsing call requestSchema.parse(req.data) is
outside the try block in addCommandChannelMessage, so ZodError escapes the
BantayogError -> bantayogErrorToHttps normalization; move the parse into the try
block (or replace with requestSchema.safeParse and, on failure, throw an
HttpsError('invalid-argument', ...)) so all validation errors are caught and
converted consistently like other handlers (see patterns in advance-dispatch.ts
and verify-report.ts), ensuring BantayogError/bantayogErrorToHttps and the catch
block handle all thrown errors.
In `@functions/src/callables/request-agency-assistance.ts`:
- Around line 500-516: The thread lookup is performed outside the Firestore
transaction (threadSnap from db.collection('command_channel_threads')) while the
update is done via tx.update, which can fail if the document changes or is
deleted between operations; fix this by moving the query into the transaction
scope and using tx.get() to retrieve the matching thread doc(s) (or obtain the
document ref then tx.get(ref)) before calling tx.update on that ref (keep the
same fields closedAt/updatedAt). Ensure you still limit to the same where
clauses (assistanceRequestId, threadType) and handle the empty result case
inside the transaction to avoid update-on-missing-doc errors.
- Around line 104-112: The current outside-transaction query for agency admins
(agencyAdminsSnap, db, canonicalAgencyId) risks TOCTOU; move the query into the
Firestore transaction so reads are consistent by using the transaction's get
(e.g., inside your runTransaction callback call
tx.get(db.collection('users').where('role','==','agency_admin').where('agencyId','==',
canonicalAgencyId).where('accountStatus','==','active'))), compute
agencyAdminUids/participantUids from that tx.get result, and then proceed with
the rest of the transactional updates; alternatively, if you intentionally keep
the query outside for performance, change the comment to explain the trade-off
rather than stating transactions cannot read.
- Around line 322-327: Normalize the actor's agency ID before authorization
checks: when comparing actor.claims.agencyId to request.targetAgencyId in
request-agency-assistance.ts (the check inside the if that throws BantayogError)
convert the actor claim to the same canonical form used for targetAgencyId
(e.g., use the existing canonicalAgencyId utility or uppercase normalization)
and then perform the comparison; apply the identical normalization to the
comparison in declineAgencyAssistanceCore so both checks use the same canonical
representation.
In `@functions/src/scheduled/admin-operations-sweep.ts`:
- Around line 21-26: The query in scheduled/admin-operations-sweep.ts (variable
pendingAssistance) filters agency_assistance_requests by status, createdAt
(range) and escalatedAt but there's no composite index for escalatedAt; fix this
by adding a new composite index entry to firestore.indexes.json for collectionId
"agency_assistance_requests" with fields in order: {fieldPath: "status", order:
"ASC"}, {fieldPath: "createdAt", order: "ASC"}, {fieldPath: "escalatedAt",
order: "ASC"} (or the order your console recommends), so Firestore can execute
the .where('status', '==',
...).where('createdAt','<',...).where('escalatedAt','==',null) query
efficiently.
In `@functions/src/triggers/border-auto-share.ts`:
- Around line 95-98: The participantUids map is being populated with
municipality IDs (ownerMuniId, targetMuniId) instead of actual user UIDs,
causing authorization checks in add-command-channel-message.ts (which looks for
actor.uid in participantUids) to fail; update the code that builds
participantUids in border-auto-share.ts to include real user UIDs (for example
include [actor.uid]: true like share-report.ts does, or query and enumerate the
user UIDs for both municipalities and set each as true) so threads contain user
IDs rather than municipality IDs and callers pass the actor.uid check.
In `@scripts/extract-boundaries.ts`:
- Around line 254-266: The loop over muniFeatures currently skips features with
properties['_centroidOnly'] before running turf.booleanPointInPolygon, causing
many barangays to get parentMunicipalityId = 'unknown'; to fix, either remove
the check that ignores muniFeature.properties?.['_centroidOnly'] so
centroid-only polygons are included in the containment test (i.e., let
booleanPointInPolygon run for all muniFeatures), or keep the check but add a
warning via your logger when parentMunicipalityId remains 'unknown' after the
loop (log the barangay id/centroidCoord and note matched only centroid-only
municipalities if relevant); update the block that references muniFeature,
muniFeatures, centroidCoord, and parentMunicipalityId accordingly.
- Around line 104-113: The code incorrectly constructs a MultiPolygon by passing
rings as separate polygons so inner rings become independent polygons; instead,
group inner rings with their corresponding outer ring(s) so holes are nested
inside their outer polygon arrays and then call turf.polygon([outer, ...inners])
for a single polygon-with-holes or turf.multiPolygon([...polygonsWithHoles])
when multiple outer polygons exist. Update the logic that builds rings (check
member.role === 'outer' / 'inner') to assemble a data structure like
[[outerRing, innerRing1, innerRing2], [outer2]] and use turf.polygon(...) when
there is one outer group or turf.multiPolygon(...) with that grouped structure
for multiple outers; for complex relation stitching consider using osmtogeojson
as a fallback.
- Around line 296-300: ngeohash.decode(cell) returns an object {latitude,
longitude}, but the code uses array destructuring and a wrong type assertion;
update the loop that iterates over cells to destructure the returned object (use
latitude and longitude from ngeohash.decode), pass the coordinates to
turf.booleanPointInPolygon in [longitude, latitude] order, and then call
boundaryGeohashes.add(cell); remove the incorrect "as [number, number]"
assertion so the runtime uses the actual properties from ngeohash.decode.
- Around line 53-61: The OverpassElement interface and any geometry mapping use
the wrong longitude field name `lng`; update the interface
(OverpassElement.geometry) to use `lon: number` instead of `lng: number` and
change all consumers that build polygons (e.g., the mapping that does
way.geometry.map(p => [p.lng, p.lat])) to use `p.lon` (e.g., [p.lon, p.lat]) and
similarly replace any other occurrences of `lng` with `lon` to match Overpass
API output.
---
Duplicate comments:
In `@apps/admin-desktop/src/__tests__/field-mode-store.test.ts`:
- Line 25: The test uses a non-deterministic timestamp assigned to the ts
constant (ts = Date.now()), which can cause flaky behavior; change this to a
deterministic value by replacing ts with a fixed numeric timestamp (e.g., a
hardcoded epoch like 1600000000000) or by mocking Date.now() via the test
framework (e.g., jest.useFakeTimers()/jest.setSystemTime) so the
field-mode-store.test.ts uses a stable timestamp for assertions and snapshot
comparisons.
In `@functions/lib/__tests__/rules/report-sharing.rules.test.js`:
- Around line 95-121: The test "a second share appends a second event without
overwriting first" is order-coupled because it uses the shared path
('report_sharing','r-share-1','events') which previous tests also write to;
change the test to isolate its data by using a unique parent doc id (e.g., a new
id instead of 'r-share-1') or ensure teardown/cleanup of that subcollection
before asserting the count, then write two events with addDoc and assert the
collection size via getDocs inside env.withSecurityRulesDisabled to expect
exactly 2; update references in the test body where 'r-share-1' is used so the
test no longer depends on earlier tests' writes.
🪄 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: 6ff9eb17-aeff-4132-a90c-c235a487230d
⛔ Files ignored due to path filters (2)
packages/shared-validators/lib/coordination.d.ts.mapis excluded by!**/*.mappackages/shared-validators/lib/coordination.js.mapis excluded by!**/*.map
📒 Files selected for processing (25)
apps/admin-desktop/src/__tests__/field-mode-store.test.tsapps/admin-desktop/src/components/CommandChannelPanel.tsxapps/admin-desktop/src/components/ReconnectBanner.tsxapps/admin-desktop/src/hooks/useFieldModeStore.tsapps/admin-desktop/src/pages/AgencyAssistanceQueuePage.test.tsxapps/admin-desktop/src/pages/AgencyAssistanceQueuePage.tsxfunctions/lib/__tests__/rules/coordination.rules.test.jsfunctions/lib/__tests__/rules/report-messages.rules.test.jsfunctions/lib/__tests__/rules/report-notes.rules.test.jsfunctions/lib/__tests__/rules/report-sharing.rules.test.jsfunctions/lib/callables/dispatch-responder-validation.jsfunctions/lib/callables/dispatch-responder.jsfunctions/lib/callables/enter-field-mode.jsfunctions/lib/callables/request-agency-assistance.jsfunctions/src/__tests__/callables/share-report.test.tsfunctions/src/__tests__/triggers/border-auto-share.test.tsfunctions/src/callables/add-command-channel-message.tsfunctions/src/callables/request-agency-assistance.tsfunctions/src/callables/share-report.tsfunctions/src/scheduled/admin-operations-sweep.tsfunctions/src/triggers/border-auto-share.tspackages/shared-validators/lib/coordination.d.tspackages/shared-validators/lib/coordination.jspackages/shared-validators/src/coordination.tsscripts/extract-boundaries.ts
Comment on lines
+41
to
+79
| const initialSelectionDoneRef = useRef(false) | ||
| const [messages, setMessages] = useState<Message[]>([]) | ||
| const [input, setInput] = useState('') | ||
| const [error, setError] = useState<string | null>(null) | ||
|
|
||
| useEffect(() => { | ||
| const q = query(collection(db, 'command_channel_threads'), where('reportId', '==', reportId)) | ||
| return onSnapshot(q, (snap) => { | ||
| const found = snap.docs.map((d: QueryDocumentSnapshot) => ({ | ||
| id: d.id, | ||
| ...(d.data() as Omit<Thread, 'id'>), | ||
| })) | ||
| setThreads(found) | ||
| if (found.length > 0 && !initialSelectionDoneRef.current) { | ||
| const first = found[0] | ||
| if (first) { | ||
| setActiveThreadId(first.id) | ||
| initialSelectionDoneRef.current = true | ||
| } | ||
| } | ||
| }) | ||
| }, [reportId]) | ||
|
|
||
| useEffect(() => { | ||
| if (!activeThreadId) return | ||
| const q = query( | ||
| collection(db, 'command_channel_messages'), | ||
| where('threadId', '==', activeThreadId), | ||
| orderBy('createdAt', 'desc'), | ||
| limit(50), | ||
| ) | ||
| return onSnapshot(q, (snap) => { | ||
| setMessages( | ||
| snap.docs | ||
| .map((d: QueryDocumentSnapshot) => ({ id: d.id, ...(d.data() as Omit<Message, 'id'>) })) | ||
| .reverse(), | ||
| ) | ||
| }) | ||
| }, [activeThreadId]) |
Contributor
There was a problem hiding this comment.
Reset thread selection when reportId changes.
initialSelectionDoneRef never resets, so a mounted panel that switches from one report to another keeps the old activeThreadId and old message subscription. The new report can end up rendering with no active tab while still listening to the previous thread. Reset the ref/state on reportId changes, and if the current thread disappears from found, select a valid replacement.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/admin-desktop/src/components/CommandChannelPanel.tsx` around lines 41 -
79, Reset the selection state and ensure the active thread is valid when
reportId changes: in the useEffect that queries 'command_channel_threads' reset
initialSelectionDoneRef.current = false (or set it based on reportId) and clear
messages/activeThreadId via setMessages([]) and setActiveThreadId(null) before
subscribing; after receiving found, if current activeThreadId is missing from
found choose a replacement (e.g., first.id) via setActiveThreadId(first.id) and
mark initialSelectionDoneRef.current = true; ensure setThreads(found) is still
called so UI updates correctly. Use the existing symbols
initialSelectionDoneRef, setThreads, setActiveThreadId, setMessages and the
threads-query useEffect to implement this behavior.
Comment on lines
+95
to
+98
| participantUids: { | ||
| [ownerMuniId]: true, | ||
| [targetMuniId]: true, | ||
| }, |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how participantUids is used in command channel authorization
rg -nC5 'participantUids' functions/src/callables/add-command-channel-message.ts
# Check security rules for command_channel_threads
rg -nC5 'participantUids' firestore.rules 2>/dev/null || echo "No firestore.rules found"Repository: Exc1D/bantayog-alert
Length of output: 673
🏁 Script executed:
# Read border-auto-share.ts around lines 95-98
cat -n functions/src/triggers/border-auto-share.ts | sed -n '85,105p'Repository: Exc1D/bantayog-alert
Length of output: 790
🏁 Script executed:
# Read share-report.ts around line 111 to see the correct pattern
rg -nC10 'participantUids.*actor.uid' functions/src/callables/share-report.tsRepository: Exc1D/bantayog-alert
Length of output: 869
🏁 Script executed:
# Check the full context of participantUids assignment in border-auto-share.ts
rg -nB5 -A5 'participantUids.*ownerMuniId' functions/src/triggers/border-auto-share.tsRepository: Exc1D/bantayog-alert
Length of output: 46
🏁 Script executed:
# Check if there are other places where participantUids is populated or used
rg -n 'participantUids.*:' functions/src --type tsRepository: Exc1D/bantayog-alert
Length of output: 670
🏁 Script executed:
# Check if there's any transformation or special handling for municipality IDs
rg -n 'ownerMuniId\|targetMuniId' functions/src/triggers/border-auto-share.ts | head -20Repository: Exc1D/bantayog-alert
Length of output: 46
🏁 Script executed:
# Check how request-agency-assistance.ts populates participantUids
rg -nB3 -A3 'participantUids:' functions/src/callables/request-agency-assistance.tsRepository: Exc1D/bantayog-alert
Length of output: 482
🏁 Script executed:
# Check what ownerMuniId and targetMuniId variables are in border-auto-share.ts
rg -n 'ownerMuniId\s*=' functions/src/triggers/border-auto-share.ts | head -5Repository: Exc1D/bantayog-alert
Length of output: 122
🏁 Script executed:
# Check the function signature and initial variable declarations
sed -n '1,50p' functions/src/triggers/border-auto-share.tsRepository: Exc1D/bantayog-alert
Length of output: 1943
🏁 Script executed:
# Find where targetMuniId is defined
rg -n 'targetMuniId\s*=' functions/src/triggers/border-auto-share.ts | head -3Repository: Exc1D/bantayog-alert
Length of output: 187
participantUids must contain user UIDs, not municipality IDs — threads will be inaccessible.
Lines 95-98 populate participantUids with municipality IDs (ownerMuniId and targetMuniId), but the command channel authorization in add-command-channel-message.ts (line 50) checks whether the caller's UID exists as a key: if (!(participantUids as Record<string, boolean>)[actor.uid]).
Since only municipality IDs are stored, any user attempting to add a message will receive a FORBIDDEN error. Compare with the correct pattern in share-report.ts (line 117) and request-agency-assistance.ts (lines 194–195), both of which use actual user UIDs: participantUids: { [actor.uid]: true }.
Fix by including actual user UIDs in participantUids. If auto-shared threads should be accessible to all users in both municipalities, you'll need to identify which users should have access and populate their UIDs accordingly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@functions/src/triggers/border-auto-share.ts` around lines 95 - 98, The
participantUids map is being populated with municipality IDs (ownerMuniId,
targetMuniId) instead of actual user UIDs, causing authorization checks in
add-command-channel-message.ts (which looks for actor.uid in participantUids) to
fail; update the code that builds participantUids in border-auto-share.ts to
include real user UIDs (for example include [actor.uid]: true like
share-report.ts does, or query and enumerate the user UIDs for both
municipalities and set each as true) so threads contain user IDs rather than
municipality IDs and callers pass the actor.uid check.
Comment on lines
+53
to
+61
| interface OverpassElement { | ||
| type: string | ||
| id: number | ||
| tags?: Record<string, string> | ||
| geometry?: Array<{ lat: number; lng: number }> | ||
| members?: Array<{ type: string; ref: number; role: string }> | ||
| lat?: number | ||
| lon?: number | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify actual Overpass API response format for way geometry
curl -sG --data-urlencode 'data=[out:json][timeout:30];way(122690479);out geom;' \
https://overpass-api.de/api/interpreter | jq '.elements[0].geometry[0] | keys'Repository: Exc1D/bantayog-alert
Length of output: 85
🏁 Script executed:
head -n 110 scripts/extract-boundaries.ts | tail -n +53Repository: Exc1D/bantayog-alert
Length of output: 1835
Overpass API returns lon, not lng — polygon construction will fail.
The OverpassElement.geometry array uses lng (line 57), but Overpass API actually returns lon for longitude. This means way.geometry.map((p) => [p.lng, p.lat]) at line 99 will produce [undefined, lat] for every coordinate, breaking all polygon construction.
🐛 Proposed fix
interface OverpassElement {
type: string
id: number
tags?: Record<string, string>
- geometry?: Array<{ lat: number; lng: number }>
+ geometry?: Array<{ lat: number; lon: number }>
members?: Array<{ type: string; ref: number; role: string }>
lat?: number
lon?: number
}And update the mapping:
- const ring: Array<[number, number]> = way.geometry.map((p) => [p.lng, p.lat])
+ const ring: Array<[number, number]> = way.geometry.map((p) => [p.lon, p.lat])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/extract-boundaries.ts` around lines 53 - 61, The OverpassElement
interface and any geometry mapping use the wrong longitude field name `lng`;
update the interface (OverpassElement.geometry) to use `lon: number` instead of
`lng: number` and change all consumers that build polygons (e.g., the mapping
that does way.geometry.map(p => [p.lng, p.lat])) to use `p.lon` (e.g., [p.lon,
p.lat]) and similarly replace any other occurrences of `lng` with `lon` to match
Overpass API output.
Comment on lines
+104
to
+113
| if (member.role === 'outer') { | ||
| rings.unshift(ring) | ||
| } else { | ||
| rings.push(ring) | ||
| } | ||
| } | ||
|
|
||
| if (rings.length === 0) return null | ||
| if (rings.length === 1) return turf.polygon([rings[0]!]) as Feature<turf.Polygon> | ||
| return turf.multiPolygon([rings]) as Feature<turf.MultiPolygon> |
Contributor
There was a problem hiding this comment.
Incorrect MultiPolygon construction — inner rings become separate polygons instead of holes.
When a relation has outer and inner (hole) rings, line 113 creates multiPolygon([rings]) which places each ring as a separate polygon. GeoJSON MultiPolygon format requires holes to be in the same polygon array as their outer ring: [[[outer], [hole1], [hole2]], [[outer2]]].
Current code produces: [[[ring1]], [[ring2]], [[ring3]]] — three separate polygons.
Correct for polygon with holes: [[[outer], [inner1], [inner2]]] — one polygon with holes.
🐛 Proposed fix for single polygon with holes
if (rings.length === 0) return null
- if (rings.length === 1) return turf.polygon([rings[0]!]) as Feature<turf.Polygon>
- return turf.multiPolygon([rings]) as Feature<turf.MultiPolygon>
+ // First ring is outer (unshifted), rest are inner (holes)
+ // This creates a single polygon with holes, not a multipolygon
+ return turf.polygon(rings) as Feature<turf.Polygon>Note: For complex relations with multiple disjoint outer ways that need stitching, consider using a library like osmtogeojson for correct multipolygon reconstruction.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/extract-boundaries.ts` around lines 104 - 113, The code incorrectly
constructs a MultiPolygon by passing rings as separate polygons so inner rings
become independent polygons; instead, group inner rings with their corresponding
outer ring(s) so holes are nested inside their outer polygon arrays and then
call turf.polygon([outer, ...inners]) for a single polygon-with-holes or
turf.multiPolygon([...polygonsWithHoles]) when multiple outer polygons exist.
Update the logic that builds rings (check member.role === 'outer' / 'inner') to
assemble a data structure like [[outerRing, innerRing1, innerRing2], [outer2]]
and use turf.polygon(...) when there is one outer group or
turf.multiPolygon(...) with that grouped structure for multiple outers; for
complex relation stitching consider using osmtogeojson as a fallback.
Comment on lines
+254
to
+266
| for (const muniFeature of muniFeatures) { | ||
| if ( | ||
| muniFeature.properties?.['_centroidOnly'] || | ||
| !turf.booleanPointInPolygon( | ||
| centroidCoord, | ||
| muniFeature as Feature<turf.Polygon | turf.MultiPolygon>, | ||
| ) | ||
| ) { | ||
| continue | ||
| } | ||
| parentMunicipalityId = muniFeature.properties?.['municipalityId'] | ||
| break | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Barangays in centroid-only municipalities will have parentMunicipalityId: 'unknown'.
The point-in-polygon check at line 256-263 explicitly skips municipalities marked _centroidOnly. Since 8 of 12 Camarines Norte municipalities only have centroid buffers (per the file header), barangays in those areas won't match and will be assigned 'unknown' as their parent.
This may be intentional given OSM data limitations, but it affects ~2/3 of municipalities. Consider either:
- Including centroid-buffered municipalities in the check (they're valid polygons, just approximations)
- Adding a warning log when a barangay gets
'unknown'parent
♻️ Option 1: Include centroid-only in parent matching
for (const muniFeature of muniFeatures) {
- if (
- muniFeature.properties?.['_centroidOnly'] ||
- !turf.booleanPointInPolygon(
+ if (
+ !turf.booleanPointInPolygon(
centroidCoord,
muniFeature as Feature<turf.Polygon | turf.MultiPolygon>,
)
) {
continue
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/extract-boundaries.ts` around lines 254 - 266, The loop over
muniFeatures currently skips features with properties['_centroidOnly'] before
running turf.booleanPointInPolygon, causing many barangays to get
parentMunicipalityId = 'unknown'; to fix, either remove the check that ignores
muniFeature.properties?.['_centroidOnly'] so centroid-only polygons are included
in the containment test (i.e., let booleanPointInPolygon run for all
muniFeatures), or keep the check but add a warning via your logger when
parentMunicipalityId remains 'unknown' after the loop (log the barangay
id/centroidCoord and note matched only centroid-only municipalities if
relevant); update the block that references muniFeature, muniFeatures,
centroidCoord, and parentMunicipalityId accordingly.
Comment on lines
+296
to
+300
| for (const cell of cells) { | ||
| const [lat, lng] = ngeohash.decode(cell) as [number, number] | ||
| if (turf.booleanPointInPolygon([lng, lat], buffered)) { | ||
| boundaryGeohashes.add(cell) | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
ngeohash npm package decode function return type
💡 Result:
The decode function of the ngeohash npm package returns a JavaScript object with latitude and longitude keys containing the decoded coordinate values as numbers.
Citations:
- 1: https://www.npmjs.com/package/ngeohash
- 2: https://github.com/sunng87/node-geohash/blob/master/README.md
- 3: https://github.com/sunng87/node-geohash
🏁 Script executed:
cat -n scripts/extract-boundaries.ts | sed -n '290,305p'Repository: Exc1D/bantayog-alert
Length of output: 894
🏁 Script executed:
rg "ngeohash" scripts/extract-boundaries.ts -B 2 -A 2Repository: Exc1D/bantayog-alert
Length of output: 582
ngeohash.decode() returns an object, not an array — geohash filtering will fail.
The ngeohash.decode() function returns { latitude: number, longitude: number }, not a tuple. The array destructuring at line 297 will assign undefined to both lat and lng, causing all booleanPointInPolygon checks to fail. The type assertion as [number, number] masks the error but does not fix the runtime behavior.
🐛 Proposed fix
for (const cell of cells) {
- const [lat, lng] = ngeohash.decode(cell) as [number, number]
+ const { latitude: lat, longitude: lng } = ngeohash.decode(cell)
if (turf.booleanPointInPolygon([lng, lat], buffered)) {
boundaryGeohashes.add(cell)
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/extract-boundaries.ts` around lines 296 - 300, ngeohash.decode(cell)
returns an object {latitude, longitude}, but the code uses array destructuring
and a wrong type assertion; update the loop that iterates over cells to
destructure the returned object (use latitude and longitude from
ngeohash.decode), pass the coordinates to turf.booleanPointInPolygon in
[longitude, latitude] order, and then call boundaryGeohashes.add(cell); remove
the incorrect "as [number, number]" assertion so the runtime uses the actual
properties from ngeohash.decode.
…clineAgencyAssistanceCoreDeps interface - Add missing DeclineAgencyAssistanceCoreDeps interface - Add missing acceptAgencyAssistanceHandler function - Export acceptAgencyAssistance callable function - Fixes build error: TS2724 acceptAgencyAssistance not exported - Fixes build error: TS2552 DeclineAgencyAssistanceCoreDeps not found
…set-state-in-effect rule - CommandChannelPanel: defer setActiveThreadId and setMessages with queueMicrotask - useFieldModeStore: defer setIsActive and setExpiresAt with queueMicrotask - AgencyAssistanceQueuePage: defer all setState calls with queueMicrotask - Fixes CodeRabbit review comments about synchronous setState in effects
This was referenced Apr 25, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Implements all 5 features of Phase 5 Cluster B (Inter-Agency Coordination):
B.1 — Agency Assistance
requestAgencyAssistance,acceptAgencyAssistance,declineAgencyAssistancecallables with idempotency, participant authorization, and thread creationadminOperationsSweepscheduled function — escalates stale agency assistance requests pending >30minAgencyAssistanceQueuePageadmin UI — accept/decline with reason inputB.2 — Field Mode
enterFieldMode,exitFieldModecallables — 4h re-auth check, 12h session expiryuseFieldModeStorehook +ReconnectBannercomponent — 60s expiry check with cleanupB.3 — OSM Boundary Extraction
scripts/extract-boundaries.ts— fetches Camarines Norte boundaries from Overpass APIBOUNDARY_GEOHASH_SETfor fast boundary proximity checksB.4 — Report Sharing
shareReportcallable — manual cross-municipality sharing with event subcollectionborderAutoShareTrigger— auto-shares reports within 500m of municipal boundaries using geohash fast-reject + turf buffer checkB.5 — Command Channel
addCommandChannelMessagecallable — participant auth, body validation (≤2000 chars), atomic message write + thread updateCommandChannelPanelReact component — thread tabs, message history, 50-msg limit, error handlingTest Results
Rollback
Summary by CodeRabbit
New Features
Tests