feat(citizen-pwa): manual location fallback with 282 barangay selector#56
Merged
feat(citizen-pwa): manual location fallback with 282 barangay selector#56
Conversation
…llbackCards, Timeline)
…, fix name collision
…RevealSheet, TrackingScreen
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 a citizen-facing PWA: a three-step report submission flow (evidence → who/where → review), offline draft persistence and retry, photo upload, a submission state machine with RevealSheet variants, real-time tracking via Firestore/React Query, UI primitives, styles, tests, and routing/entry integration. (50 words) Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Form as SubmitReportForm
participant Machine as useSubmissionMachine
participant Photo as PhotoUploadManager
participant Storage as Firebase Storage
participant Firestore as Firestore (report_inbox)
participant Reveal as RevealSheet
User->>Form: complete steps & submit
Form->>Machine: transition('submitting')
alt photo provided
Form->>Photo: uploadFile(file)
Photo->>Storage: PUT bytes
Storage-->>Photo: download URL
Photo-->>Form: photoUrl, storagePath
end
Form->>Firestore: write report_inbox (payload)
Firestore-->>Form: write success (publicRef, secret)
Form->>Machine: transition('success') / store refs
Form->>Reveal: render with publicRef
User->>Reveal: dismiss or navigate to /reports/:ref
Reveal->>Machine: dismiss() / transition('closed')
sequenceDiagram
actor User
participant Tracking as TrackingScreen
participant Hook as useReport
participant Firestore as Firestore (reports/:ref)
participant Query as React Query Cache
User->>Tracking: navigate /reports/:reference
Tracking->>Hook: useReport(reference)
Hook->>Firestore: onSnapshot(reports/:reference)
Firestore-->>Hook: initial doc
Hook->>Query: set cache ['reports', reference]
Query-->>Tracking: provide ReportData
Tracking->>User: render status, timeline, contact
Firestore-->>Hook: doc update (status/timeline)
Hook->>Query: update cache
Query-->>Tracking: re-render with new status
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 |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 5 issues, and left some high level feedback:
- The
useReporthook sets query data viaonSnapshotbut theuseQueryqueryFnimmediately returnsnull, soisLoadingwill already befalseandTrackingScreenwill render "Report not found" before the first snapshot arrives; consider modelling an explicit loading state (e.g. usinginitialData/placeholderDataor a localisReadyflag) so the UI doesn’t treat the report as missing while the first snapshot is in flight. - The hotline and MDRRMO copy is still Daet-specific in several places (e.g.
RevealSheetsublines reference "Daet MDRRMO" and the hotline constants are duplicated betweenRevealSheetandTrackingScreen); consider centralizing these strings in a config and/or making them municipality-agnostic to match the intent of generic text and avoid drift.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `useReport` hook sets query data via `onSnapshot` but the `useQuery` `queryFn` immediately returns `null`, so `isLoading` will already be `false` and `TrackingScreen` will render "Report not found" before the first snapshot arrives; consider modelling an explicit loading state (e.g. using `initialData`/`placeholderData` or a local `isReady` flag) so the UI doesn’t treat the report as missing while the first snapshot is in flight.
- The hotline and MDRRMO copy is still Daet-specific in several places (e.g. `RevealSheet` sublines reference "Daet MDRRMO" and the hotline constants are duplicated between `RevealSheet` and `TrackingScreen`); consider centralizing these strings in a config and/or making them municipality-agnostic to match the intent of generic text and avoid drift.
## Individual Comments
### Comment 1
<location path="apps/citizen-pwa/src/hooks/useReport.ts" line_range="38-45" />
<code_context>
+ closedBy?: string
+}
+
+export function useReport(reportRef: string) {
+ const queryClient = useQueryClient()
+ const unmountedRef = useRef(false)
+
+ useEffect(() => {
+ unmountedRef.current = false
+
+ const unsubscribe = onSnapshot(
+ doc(db(), `reports/${reportRef}`),
+ (snapshot: DocumentSnapshot) => {
</code_context>
<issue_to_address>
**issue (bug_risk):** Avoid subscribing to Firestore when reportRef is empty to prevent invalid document paths.
`useReport` is invoked from `TrackingScreen` even when `reference` is falsy, so `reportRef` can be an empty string. That makes `doc(db(), \\`reports/${reportRef}\\`)` resolve to `reports/` with an empty ID, which is invalid in Firestore and can throw. Add an early return when `!reportRef` (e.g. skip `onSnapshot` and let `useQuery` resolve to `null`) to avoid creating invalid document paths while keeping the hook signature and React hooks rules intact.
</issue_to_address>
### Comment 2
<location path="apps/citizen-pwa/src/components/RevealSheet.tsx" line_range="63-67" />
<code_context>
+ window.location.href = `/reports/${referenceCode}`
+ }
+
+ const handlePrimaryAction = () => {
+ if (state === 'success') {
+ handleTrackReport()
+ } else {
+ closeSheet()
+ }
+ }
</code_context>
<issue_to_address>
**issue (bug_risk):** Primary actions for queued/failed states close the local sheet store but do not progress the submission machine.
For `queued` and `failed_retryable`, `handlePrimaryAction` calls `closeSheet()` from `useUIStore`, but `SubmitReportForm` renders `RevealSheet` based on `machineState`/`showRevealSheet`. Since the primary action doesn’t call `onClose` or transition the submission machine, the sheet can stay mounted even when the store says it’s closed. Please either delegate dismissal to `onClose` or also trigger the appropriate `transition(...)` so the render condition and the store stay in sync.
</issue_to_address>
### Comment 3
<location path="apps/citizen-pwa/src/components/SubmitReportForm/Step1Evidence.tsx" line_range="38-39" />
<code_context>
+ const handlePhotoChange = (e: React.ChangeEvent<HTMLInputElement>) => {
+ const file = e.target.files?.[0] ?? null
+ setPhotoFile(file)
+ if (file) {
+ setPreviewUrl(URL.createObjectURL(file))
+ }
+ }
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Revoke object URLs when replacing/removing the photo to avoid leaking blob URLs.
`URL.createObjectURL(file)` allocates a blob URL that persists until revoked. Track the current preview URL and call `URL.revokeObjectURL` before setting a new one and/or in a `useEffect` cleanup on unmount to prevent leaking these URLs.
Suggested implementation:
```typescript
const [reportType, setReportType] = useState('flood')
const [photoFile, setPhotoFile] = useState<File | null>(null)
const [previewUrl, setPreviewUrl] = useState<string | null>(null)
useEffect(() => {
return () => {
if (previewUrl) {
URL.revokeObjectURL(previewUrl)
}
}
}, [previewUrl])
const handlePhotoChange = (e: React.ChangeEvent<HTMLInputElement>) => {
const file = e.target.files?.[0] ?? null
setPhotoFile(file)
if (file) {
const nextUrl = URL.createObjectURL(file)
setPreviewUrl(nextUrl)
} else {
setPreviewUrl(null)
}
```
1. Ensure `useEffect` is imported from React at the top of this file:
- If you currently have `import React, { useState } from 'react'`, update it to `import React, { useState, useEffect } from 'react'`.
- Or, if using named imports only, add `useEffect` to that import list.
2. If there is existing logic later in the component that manually revokes blob URLs for `previewUrl`, remove it to avoid double-revocation.
</issue_to_address>
### Comment 4
<location path="apps/citizen-pwa/src/components/SubmitReportForm/Step2WhoWhere.tsx" line_range="549-557" />
<code_context>
- >
- <label>
- Reference
- <input
- value={publicRef}
- onChange={(e) => {
</code_context>
<issue_to_address>
**issue (bug_risk):** Use a string default for `nearestLandmark` so the input stays fully controlled.
Because `nearestLandmark` is `string | undefined` and passed directly as the input `value`, React will treat `undefined` as an uncontrolled value and may warn about switching between controlled and uncontrolled inputs. Initializing it as `''` and keeping it as a string avoids these warnings and makes the `onNext` inclusion check simpler and more predictable.
</issue_to_address>
### Comment 5
<location path="apps/citizen-pwa/src/__tests__/submit-flow.test.tsx" line_range="22-16" />
<code_context>
+ server.close()
+})
+
+describe('Submission flow integration', () => {
+ it('should render submit report form steps', () => {
+ render(
+ <TestWrapper>
+ <div>Submit report form test placeholder</div>
+ </TestWrapper>,
+ )
+
+ expect(screen.getByText('Submit report form test placeholder')).toBeInTheDocument()
+ })
+
+ it('should show Step1Evidence with incident type selection', () => {
</code_context>
<issue_to_address>
**issue (testing):** Integration tests are currently placeholders and do not exercise the new multi-step submission flow or manual-location behaviour.
These tests only render a placeholder `<div>` and never mount `SubmitReportForm` or exercise its steps, so the new flow isn’t actually tested. Please replace them with real interaction tests, for example:
- Render `<SubmitReportForm />` in `TestWrapper` and drive the three steps via `userEvent`.
- Manual path: select municipality (and optional barangay), enter a landmark, fill in name/phone, and assert `handleSubmit` receives an inbox payload with `municipalityId`, optional `barangayId`, and `nearestLandmark`.
- GPS path: mock `navigator.geolocation.getCurrentPosition`, assert the coordinates appear in Steps 2 and 3, and that manual-only fields are omitted from the payload.
- Verify patient counts are set to 0 when "Is anyone hurt?" is "No".
Without this, regressions in the new flow will not be caught by tests.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+38
to
+45
| export function useReport(reportRef: string) { | ||
| const queryClient = useQueryClient() | ||
| const unmountedRef = useRef(false) | ||
|
|
||
| useEffect(() => { | ||
| unmountedRef.current = false | ||
|
|
||
| const unsubscribe = onSnapshot( |
There was a problem hiding this comment.
issue (bug_risk): Avoid subscribing to Firestore when reportRef is empty to prevent invalid document paths.
useReport is invoked from TrackingScreen even when reference is falsy, so reportRef can be an empty string. That makes doc(db(), \\reports/${reportRef}\) resolve to reports/ with an empty ID, which is invalid in Firestore and can throw. Add an early return when !reportRef (e.g. skip onSnapshot and let useQuery resolve to null) to avoid creating invalid document paths while keeping the hook signature and React hooks rules intact.
Comment on lines
+63
to
+67
| const handlePrimaryAction = () => { | ||
| if (state === 'success') { | ||
| handleTrackReport() | ||
| } else { | ||
| closeSheet() |
There was a problem hiding this comment.
issue (bug_risk): Primary actions for queued/failed states close the local sheet store but do not progress the submission machine.
For queued and failed_retryable, handlePrimaryAction calls closeSheet() from useUIStore, but SubmitReportForm renders RevealSheet based on machineState/showRevealSheet. Since the primary action doesn’t call onClose or transition the submission machine, the sheet can stay mounted even when the store says it’s closed. Please either delegate dismissal to onClose or also trigger the appropriate transition(...) so the render condition and the store stay in sync.
Comment on lines
+38
to
+39
| if (file) { | ||
| setPreviewUrl(URL.createObjectURL(file)) |
There was a problem hiding this comment.
suggestion (bug_risk): Revoke object URLs when replacing/removing the photo to avoid leaking blob URLs.
URL.createObjectURL(file) allocates a blob URL that persists until revoked. Track the current preview URL and call URL.revokeObjectURL before setting a new one and/or in a useEffect cleanup on unmount to prevent leaking these URLs.
Suggested implementation:
const [reportType, setReportType] = useState('flood')
const [photoFile, setPhotoFile] = useState<File | null>(null)
const [previewUrl, setPreviewUrl] = useState<string | null>(null)
useEffect(() => {
return () => {
if (previewUrl) {
URL.revokeObjectURL(previewUrl)
}
}
}, [previewUrl])
const handlePhotoChange = (e: React.ChangeEvent<HTMLInputElement>) => {
const file = e.target.files?.[0] ?? null
setPhotoFile(file)
if (file) {
const nextUrl = URL.createObjectURL(file)
setPreviewUrl(nextUrl)
} else {
setPreviewUrl(null)
}- Ensure
useEffectis imported from React at the top of this file:- If you currently have
import React, { useState } from 'react', update it toimport React, { useState, useEffect } from 'react'. - Or, if using named imports only, add
useEffectto that import list.
- If you currently have
- If there is existing logic later in the component that manually revokes blob URLs for
previewUrl, remove it to avoid double-revocation.
Comment on lines
+549
to
+557
| <input | ||
| type="text" | ||
| value={nearestLandmark} | ||
| onChange={(e) => { | ||
| setNearestLandmark(e.target.value) | ||
| }} | ||
| placeholder="e.g. Near the town plaza, across from Mang Juan Store" | ||
| className="text-input" | ||
| maxLength={200} |
There was a problem hiding this comment.
issue (bug_risk): Use a string default for nearestLandmark so the input stays fully controlled.
Because nearestLandmark is string | undefined and passed directly as the input value, React will treat undefined as an uncontrolled value and may warn about switching between controlled and uncontrolled inputs. Initializing it as '' and keeping it as a string avoids these warnings and makes the onNext inclusion check simpler and more predictable.
|
|
||
| beforeEach(() => { | ||
| server.listen({ onUnhandledRequest: 'error' }) | ||
| }) |
There was a problem hiding this comment.
issue (testing): Integration tests are currently placeholders and do not exercise the new multi-step submission flow or manual-location behaviour.
These tests only render a placeholder <div> and never mount SubmitReportForm or exercise its steps, so the new flow isn’t actually tested. Please replace them with real interaction tests, for example:
- Render
<SubmitReportForm />inTestWrapperand drive the three steps viauserEvent. - Manual path: select municipality (and optional barangay), enter a landmark, fill in name/phone, and assert
handleSubmitreceives an inbox payload withmunicipalityId, optionalbarangayId, andnearestLandmark. - GPS path: mock
navigator.geolocation.getCurrentPosition, assert the coordinates appear in Steps 2 and 3, and that manual-only fields are omitted from the payload. - Verify patient counts are set to 0 when "Is anyone hurt?" is "No".
Without this, regressions in the new flow will not be caught by tests.
![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: 37
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/citizen-pwa/src/routes.tsx (1)
19-20:⚠️ Potential issue | 🟠 MajorReveal back-navigation guard is never activated.
AppRoutes(Line 19-Line 20) mounts onlyRouterProvider; no routed component invokesuseRevealGuardfromapps/citizen-pwa/src/lib/routerGuard.ts, so the POP-blocking behavior never runs. Mount the guard in a layout/route component rendered inside this router tree.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/citizen-pwa/src/routes.tsx` around lines 19 - 20, AppRoutes currently only returns RouterProvider so useRevealGuard (from apps/citizen-pwa/src/lib/routerGuard.ts) is never invoked; fix by mounting the guard inside the rendered router tree: import useRevealGuard and call it from a component that is actually rendered by the router (e.g., a root layout component) or wrap RouterProvider with a small component inside AppRoutes that calls useRevealGuard before returning <RouterProvider router={router} /> so the POP-blocking behavior runs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/citizen-pwa/src/__tests__/submit-flow.test.tsx`:
- Around line 23-61: The tests are currently using static placeholders and don't
exercise SubmitReportForm or its step components; replace each placeholder
render with rendering the real SubmitReportForm (wrapped by TestWrapper), then
simulate user interactions (use userEvent/fireEvent) to choose an incident type
in Step1Evidence, fill required fields, navigate to Step3Review, assert that
Step3Review content appears and that submit triggers the expected outcome (e.g.,
mock submit handler called, API mocked response handled, navigation or success
message shown); update test names to reflect these behaviors and mock any
external dependencies so assertions verify actual component logic and step
transitions rather than trivial string presence.
- Around line 63-69: Replace the two empty tests in submit-flow.test.tsx that
contain raw TODO comments with explicit tracked placeholders using Jest's
it.todo and a ticket reference; specifically change the tests named "should save
draft when offline" and "should show queued Reveal on network error" to
it.todo('TICKET-ID: should save draft when offline') and it.todo('TICKET-ID:
should show queued Reveal on network error') (or implement the assertions now),
ensuring the test titles include the ticket IDs so the missing work is auditable
and follows the repo guideline.
In `@apps/citizen-pwa/src/components/RevealSheet.tsx`:
- Around line 79-111: The timelineEvents object currently uses hardcoded "2:14
PM" strings; replace those static meta values by computing formatted times from
the real report timestamp (e.g., a prop or event.createdAt) or a relative label
("Just now", "5m ago"). Update RevealSheet.tsx where timelineEvents is built:
add a helper (e.g., formatTime or formatRelative) and call it to set meta for
entries in success, queued, and failed_retryable instead of the literal "2:14
PM"; ensure you pass the correct timestamp source (report.createdAt or a
submittedAt prop) and fall back to a sensible default when missing.
- Around line 59-61: The handler handleTrackReport in RevealSheet.tsx uses
window.location.href which triggers a full reload; switch to react-router
navigation by importing useNavigate from react-router-dom, creating const
navigate = useNavigate() inside the RevealSheet component, and replacing the
window.location.href assignment with navigate(`/reports/${referenceCode}`) so
navigation is SPA-friendly and preserves React state.
In `@apps/citizen-pwa/src/components/SubmitReportForm/index.tsx`:
- Around line 49-56: The upload error currently thrown from putBlob only
includes the HTTP status; update the putBlob function to read the response body
(await res.text()) when res.ok is false and include that text in the thrown
Error so callers get both status and server details; if reading the body fails
or is empty, fall back to the status-only message to avoid masking the original
error.
- Around line 80-99: The code builds a SubmitReportDeps object but then
reimplements submission logic inline; import and call the existing submitReport
service from services/submit-report.ts instead of duplicating logic. Keep the
deps object (ensureSignedIn, requestUploadUrl, putBlob, writeInbox, randomUUID,
randomPublicRef, randomSecret, sha256Hex, now), remove the inline
upload/payload/inbox code, invoke await submitReport(deps, reportPayload) (or
the service's expected parameter shape) and use its returned result (e.g., inbox
id) to continue; ensure you preserve any error handling and UI updates around
the single submitReport call.
- Around line 126-134: The generated plaintext secret from deps.randomSecret()
is currently hashed into secretHash and the plaintext is discarded, so users
cannot retrieve it; fix by capturing the plaintext secret into a local variable
(e.g., const secret = await deps.randomSecret()) before computing secretHash,
pass secretHash to deps.writeInbox as now, and ensure the function that calls
writeInbox (e.g., submit-report or the caller that returns { publicRef, secret,
correlationId }) returns the plaintext secret along with publicRef and
correlationId so the caller can present the secret to the user for later
authentication.
In `@apps/citizen-pwa/src/components/SubmitReportForm/Step1Evidence.tsx`:
- Around line 35-40: handlePhotoChange currently creates a new object URL via
URL.createObjectURL(file) but never revokes the previous URL, leaking blob URLs;
update handlePhotoChange to store the previous preview URL (from previewUrl
state) and call URL.revokeObjectURL(prevUrl) before calling setPreviewUrl with
the new URL, and also ensure any cleanup revocation occurs on component unmount
(e.g., in a useEffect cleanup) to revoke the last preview; reference
handlePhotoChange, setPreviewUrl, setPhotoFile, URL.createObjectURL and
URL.revokeObjectURL when making this change.
- Around line 86-92: The "No photo available" button currently triggers the file
picker (document.getElementById('photo-input')?.click()), which is the opposite
of its label; update the Step1Evidence component so the button truly opts out:
replace the onClick handler to call the skip handler (e.g., props.onSkipPhoto or
a local handleSkipPhoto that sets evidence state to null and advances the step)
instead of clicking 'photo-input', and change its text to a clear opt-out label
like "Skip photo" (or if you prefer the original behavior, instead change the
text to "Upload from device" while keeping the current onClick). Ensure you
modify the button with class "no-photo-link" and reference the 'photo-input'
usage only for the upload action.
- Around line 20-28: The INCIDENT_TYPES array contains values that don't match
the shared report schema's allowed reportType values, causing validation
failures; update INCIDENT_TYPES (the constant in Step1Evidence.tsx) so each
object.value uses one of the allowed values from
packages/shared-validators/src/reports.ts (flood, fire, earthquake, typhoon,
landslide, storm_surge), adjust labels and Icon assignments accordingly (or
remove unmapped entries like road/medical/power/other), and ensure any UI logic
expecting reportType (e.g., form submit handlers) uses these normalized values
so payloads pass downstream validation.
In `@apps/citizen-pwa/src/components/SubmitReportForm/Step2WhoWhere.tsx`:
- Around line 330-348: The catch in attemptGps swallows the underlying
Geolocation error; change it to capture the exception (e.g., catch (err)) and
log the error for diagnostics while keeping the user-facing message and behavior
intact—update the catch block in attemptGps to call a logger (console.error or
an existing logging utility) with context like "attemptGps failed" and the error
object, then continue to call setLocationError('Could not get location.') and
setLocationMethod('manual').
- Around line 549-558: The input currently uses value={nearestLandmark} which
can be undefined and triggers React's uncontrolled->controlled warning; ensure
the input is always controlled by either initializing the state to an empty
string where nearestLandmark is declared or coalescing in JSX (use
value={nearestLandmark ?? ''}); update the handler to keep using
setNearestLandmark for changes.
- Around line 350-353: Wrap the attemptGps function in useCallback and list its
dependencies (all state setters and any props/values it reads) so it has a
stable identity, then remove the eslint-disable and include attemptGps in the
useEffect dependency array (i.e., useEffect(() => { void attemptGps() },
[attemptGps])). Specifically update the declaration of attemptGps to
useCallback(..., [setGpsStatus, setLocation, /* any other referenced
state/setters/props used inside attemptGps */]) and ensure the effect references
that memoized attemptGps instead of calling a changing function.
In `@apps/citizen-pwa/src/components/SubmitReportForm/Step3Review.tsx`:
- Around line 61-65: The third progress dot in Step3Review.tsx is still using
progress-dot--inactive so the UI shows only 2/3 active; update the third div
inside the progress-dots block to use progress-dot--active (or remove the
inactive class and ensure it matches the other active dots) so all three dots
render as active for the "3 of 3" review step (refer to the progress-dots
container and the individual divs with class names progress-dot,
progress-dot--active, and progress-dot--inactive).
- Around line 17-27: The reportData shape only carries barangayId so the review
screen shows an internal key; update the contract to include an optional
barangayLabel: string (e.g., add barangayLabel?: string to the reportData type
in Step3Review.tsx), ensure the code that builds/flows reportData in Step 2
passes the selected barangay label into that field, and modify the Step3Review
rendering logic to display reportData.barangayLabel instead of the raw
barangayId (and keep barangayId for submission/internal use). Also update any
initial state/type declarations and serialization/submit helpers that construct
or expect reportData to accept the new optional property.
In `@apps/citizen-pwa/src/components/TrackingScreen.tsx`:
- Around line 141-145: The "Update report" and "Re-open if situation changed"
buttons in the TrackingScreen component are rendered without onClick handlers
and therefore do nothing; either wire them to the appropriate handler props or
disable them. Locate the Button elements inside the TrackingScreen (className
"tracking-actions") and: 1) if update/reopen actions exist (e.g., props like
onUpdateReport or onReopenReport, or local methods updateReport/reopenReport),
pass those functions into the Button onClick; or 2) if the functionality isn't
implemented yet, set the Buttons to disabled (or change variant to passive) and
add a TODO comment so they don't appear as active CTAs. Ensure button text
"Update report" and "Re-open if situation changed" are the ones updated.
- Around line 90-95: The Coords rendering in the TrackingScreen component
currently calls toFixed on optional values and can show "undefined" parts;
update the JSX for the card-row that uses report.location?.lat and
report.location?.lng to first check that both values are present and are finite
numbers (e.g., with typeof === 'number' or Number.isFinite) before calling
toFixed, and if either is missing render "N/A" instead; change the span with
className "card-value" to conditionally render formatted coordinates only when
both lat and lng are valid, otherwise show "N/A".
In `@apps/citizen-pwa/src/components/ui/FallbackCards.tsx`:
- Around line 18-35: In FallbackCards.tsx, the two interactive button elements
(the "Call" button using onCallClick and the "Send SMS" button using onSmsClick)
should be disabled when their respective handlers are not provided to avoid dead
controls: add the disabled attribute when onCallClick or onSmsClick is falsy,
set aria-disabled="true" for assistive tech, and update the aria-label or
visible text (e.g., append "(unavailable)") or keep label but ensure keyboard
focus is prevented so the button is not interactive when handler is missing;
apply the same class/visual styling for disabled state (e.g.,
fallback-card--disabled) to indicate non-interactive state.
In `@apps/citizen-pwa/src/components/ui/StatusBanner.tsx`:
- Around line 11-16: The StatusBanner component renders dynamic status content
without ARIA live region semantics; update the root container in StatusBanner to
include live-region attributes (e.g., add role="status" and aria-live="polite",
consider aria-atomic="true" if you need whole-message announcements) so screen
readers reliably announce changes to the children content; modify the JSX for
the root div in the StatusBanner component (the element with className
`status-banner status-banner--${variant}`) to include these attributes.
In `@apps/citizen-pwa/src/hooks/useReport.ts`:
- Around line 47-54: The snapshot success handler currently calls
mapReportFromFirestore directly which can throw and crash React; wrap the
mapping call in a try-catch inside the (snapshot: DocumentSnapshot) => { ... }
callback (the block that checks snapshot.exists()), call
queryClient.setQueryData(['reports', reportRef], mapReportFromFirestore(data))
inside the try, and in the catch log the error (e.g., console.error or your
logger) and set queryClient.setQueryData(['reports', reportRef], null) so a
bad/malformed Firestore document won’t propagate an exception; keep the
unmountedRef.current check as-is.
In `@apps/citizen-pwa/src/hooks/useSubmissionMachine.ts`:
- Around line 35-42: The transition function currently allows any
SubmissionState change; add validation logic in the useCallback for transition
to enforce allowed state flows (e.g., define a map/set of valid transitions for
SubmissionState and check current state → nextState against it) before calling
setState, and if invalid either throw/log and no-op or assert (depending on
desired behavior); update the transition implementation to reference the current
state (from state) and the validator, keep the existing retry-reset logic for
queued/success, and ensure symbols mentioned (transition, SubmissionState,
setState, setRetryCount, setLastError) are used to locate and modify the code.
- Around line 54-62: The exported retry-related state and function in
useSubmissionMachine (retryCount, lastError, incrementRetry) are unused by
SubmitReportForm and the codebase; either delete these symbols and any related
state (remove setRetryCount/setLastError and the incrementRetry callback inside
useSubmissionMachine) or explicitly document their intended future use by adding
a clear TODO comment above incrementRetry and the exported retryCount/lastError
explaining they are reserved for planned retry handling (and keep the exports).
Reference useSubmissionMachine, incrementRetry, retryCount, lastError, and
SubmitReportForm when making the change so the reviewer can verify removal vs
documented intent.
In `@apps/citizen-pwa/src/lib/draftManager.ts`:
- Around line 44-53: promoteDraftToSuccess currently sets state to 'draft' which
is semantically wrong; update the function to either remove the draft after
successful submission or set an explicit success state: if you prefer deletion,
replace the final saveDraft call with a deleteDraft(uuid) call (using getDraft
to validate existence); if you prefer a status flag, add a 'success' member to
the DraftReport type and change the saveDraft payload in promoteDraftToSuccess
to set state: 'success' and submittedRef: finalRef (also update any type
definitions and consumers of DraftReport accordingly). Ensure you update
imports/usages of saveDraft/getDraft and any validation that relies on the draft
state.
In `@apps/citizen-pwa/src/lib/localforage.ts`:
- Around line 69-78: cleanupExpiredDrafts currently calls deleteDraft
sequentially; change it to fetch drafts via listDrafts(), filter by createdAt <
Date.now() - 24*60*60*1000, then run deletions in parallel using Promise.all (or
Promise.allSettled if you want per-item failure isolation) over drafts.map(d =>
deleteDraft(d.uuid)); keep function name cleanupExpiredDrafts and use listDrafts
and deleteDraft identifiers so the change is localized and ensure the function
still returns Promise<void>.
In `@apps/citizen-pwa/src/lib/mappers.ts`:
- Around line 30-36: Validate data.location is a plain object before treating it
as one: replace the current guard (data.location !== undefined) with a check
that data.location !== undefined && data.location !== null && typeof
data.location === 'object' && !Array.isArray(data.location), then assign const
loc = data.location as Record<string, unknown> and proceed to build
result.location only when that predicate passes so property reads on
loc.address, loc.lat and loc.lng are safe.
In `@apps/citizen-pwa/src/lib/photoUpload.ts`:
- Around line 83-90: The code currently force-unwraps manager.storagePath in the
return; update the guard to check storagePath as well so the non-null assertion
is unnecessary: in the same conditional that checks manager.state !== 'done' ||
!manager.photoUrl, add a check for !manager.storagePath (e.g., if (manager.state
!== 'done' || !manager.photoUrl || !manager.storagePath) throw ...), then return
storagePath: manager.storagePath (without the !). This keeps manager.state,
manager.photoUrl and manager.storagePath properly null-checked and type-safe.
- Around line 55-67: The uploadFile function currently accepts any File; add
defensive validation before setting selectedFile or starting upload: define an
allowlist of MIME types/extensions and a MAX_FILE_SIZE constant, check file.type
(and fallback to file.name extension) and file.size against those rules, and if
invalid set error, state='idle' (or return/throw) to prevent upload; update
references to selectedFile and storagePath only after validation succeeds and
ensure the existing try/catch still surfaces errors (do not swallow them) so
failures are logged/propagated appropriately.
- Around line 63-65: The current code constructs filename using raw file.name
and builds storagePath (`const filename = `${Date.now()}-${file.name}``,
`storagePath = `citizen-uploads/${filename}``, `ref(getStorage(), storagePath)`)
which allows user-controlled input into object keys; fix it by sanitizing and
normalizing file.name (strip path separators, non-printable chars, limit
length), and prepend a strong unique id (e.g., UUID or secure timestamp+random)
instead of or in addition to Date.now(); update the filename and storagePath
assignments to use the sanitizedBaseName and uniqueId, and ensure any errors
during sanitization or storage ref creation are propagated or logged (no empty
catch blocks) within the photo upload function so invalid names do not reach
ref(getStorage(), storagePath).
In `@apps/citizen-pwa/src/lib/query-client.tsx`:
- Around line 61-65: In getRequest.onsuccess (the IndexedDB read in
initializeQueryClient()), wrap the JSON.parse(getRequest.result as string) call
in a try/catch so a malformed value won't leave the promise unresolved; on parse
failure log the error, resolve(undefined) (treat as no persisted client) and
optionally remove the corrupted key from the store (e.g., issue a delete on the
objectStore) before closing the DB so startup doesn't hang and corrupted cache
is cleaned up; ensure the catch branch still closes db and resolves or rejects
the outer promise consistently.
In `@apps/citizen-pwa/src/lib/routerGuard.ts`:
- Around line 7-10: The blocker currently inspects currentLocation.pathname (the
source) when deciding whether to allow POP during an open sheet; change the
guard to use nextLocation.pathname (the navigation target provided by
useBlocker) instead so the logic checks the destination path before blocking;
update the callback used by useBlocker and any references where currentLocation
is used inside that callback (e.g., the function passed to useBlocker, the
isTrackingScreen check) to read nextLocation.pathname and keep the existing
historyAction and currentSheet checks.
In `@apps/citizen-pwa/src/main.tsx`:
- Line 7: Wrap the call to initializeQueryClient() in a try/catch in main.tsx so
initialization failures don't block app startup: call await
initializeQueryClient() inside a try block, catch any error thrown, log it
(e.g., console.error or app logger) with context, and proceed to boot the app
with degraded cache/persistence behavior; ensure any code that depends on a
successfully-initialized query client (e.g., QueryClientProvider setup) can
handle a missing or non-persistent client by falling back to an in-memory
QueryClient or skipping persistence setup.
In `@apps/citizen-pwa/src/styles/globals.css`:
- Around line 3-27: Remove duplicate CSS custom property definitions in
globals.css that are already defined in design-tokens.css; keep a single source
of truth by importing/using the variables from design-tokens.css and only
declare overrides in globals.css for intentional app-specific changes.
Specifically, remove or comment out repeated root variables (e.g.,
--color-primary, --color-surface, --color-card, --color-border, --color-text-*,
--color-success-*, --color-queued-*, --color-failed-*, --color-amber-*,
--color-surface-*, --color-border-warn*, --color-em-*) and leave only the deltas
you need for this app (or rename app-specific vars) so maintenance stays in
design-tokens.css; ensure any remaining overrides are clearly documented and
only present in :root of globals.css.
- Around line 313-323: The .sr-only utility uses the deprecated clip property;
update the rule for .sr-only to remove the clip(...) usage and add clip-path:
inset(50%); (and optionally -webkit-clip-path: inset(50%); for legacy WebKit)
while keeping the other accessibility styles (position, width, height, padding,
margin, overflow, white-space, border) intact so the element remains visually
hidden but accessible to screen readers.
In `@e2e-tests/a11y/citizen-pwa-a11y.spec.ts`:
- Around line 8-10: The test "should be keyboard navigable" currently only calls
page.keyboard.press('Tab') and has no assertion; update this test to assert
actual focus change after the keypress (e.g., call page.keyboard.press('Tab')
and then verify the focused element using page.locator(...).evaluate(el =>
document.activeElement) or page.evaluate(() => document.activeElement) or
Playwright's page.locator(selector).isFocused()); reference the existing test
block (test('should be keyboard navigable', async ({ page }) => { ... })) and
replace the trivial call with a keypress plus an assertion that the expected
interactive element (button/link/input with a specific selector or text)
receives focus.
In `@e2e-tests/specs/citizen-pwa.spec.ts`:
- Around line 15-17: The test uses a fixed sleep after clicking the "Capture
location" button (page.click('button:has-text("Capture location")') followed by
page.waitForTimeout(1000')) which is flaky and slow; replace the sleep with a
state-based wait that targets the actual UI change or readiness rather than a
timeout—for example wait for a specific selector/text/attribute that appears or
becomes enabled after capture (use page.waitForSelector or locator.waitFor with
the appropriate visible/attached/timeout options, or wait for the network
response if applicable) so Playwright's auto-wait handles timing
deterministically instead of using page.waitForTimeout.
- Around line 36-48: These skipped specs contain bare "TODO" comments; replace
them with either (a) concrete test implementations using the Playwright test
names 'should handle offline state and retry', 'should track report updates in
real-time', and 'should block back on queued state' or (b) annotate each TODO
with a tracked ticket reference (e.g. TODO: TICKET-123 or link to issue) and
update the test description or a comment to include the ticket ID so they are
not untracked placeholders; specifically edit the three test.skip blocks to
remove bare TODOs by either implementing emulator-backed tests (simulate offline
mode / Firebase Emulator updates / queued submission + back-button) or by adding
the ticket reference and a short acceptance-criteria summary in the comment.
- Around line 4-10: test.beforeEach currently calls page.goto('/') before
clearing localStorage, so the app may hydrate from stale state; change the setup
to clear persisted state before the app boots by using page.addInitScript(() =>
localStorage.clear()) prior to page.goto('/') (or alternatively call
page.evaluate(() => localStorage.clear()) and then immediately reload before
interacting). Update the test.beforeEach to invoke page.addInitScript for
clearing localStorage (or clear+reload) so the app never sees stale
draft/query-cache on first load.
---
Outside diff comments:
In `@apps/citizen-pwa/src/routes.tsx`:
- Around line 19-20: AppRoutes currently only returns RouterProvider so
useRevealGuard (from apps/citizen-pwa/src/lib/routerGuard.ts) is never invoked;
fix by mounting the guard inside the rendered router tree: import useRevealGuard
and call it from a component that is actually rendered by the router (e.g., a
root layout component) or wrap RouterProvider with a small component inside
AppRoutes that calls useRevealGuard before returning <RouterProvider
router={router} /> so the POP-blocking behavior runs.
🪄 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: 377f085b-91e4-497f-ba6a-17900ab5ea82
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (34)
apps/citizen-pwa/README.mdapps/citizen-pwa/package.jsonapps/citizen-pwa/src/__tests__/submit-flow.test.tsxapps/citizen-pwa/src/__tests__/test-utils.tsxapps/citizen-pwa/src/components/LookupScreen.tsxapps/citizen-pwa/src/components/ReceiptScreen.tsxapps/citizen-pwa/src/components/RevealSheet.tsxapps/citizen-pwa/src/components/SubmitReportForm.tsxapps/citizen-pwa/src/components/SubmitReportForm/Step1Evidence.tsxapps/citizen-pwa/src/components/SubmitReportForm/Step2WhoWhere.tsxapps/citizen-pwa/src/components/SubmitReportForm/Step3Review.tsxapps/citizen-pwa/src/components/SubmitReportForm/index.tsxapps/citizen-pwa/src/components/TrackingScreen.tsxapps/citizen-pwa/src/components/ui/Button.tsxapps/citizen-pwa/src/components/ui/FallbackCards.tsxapps/citizen-pwa/src/components/ui/StatusBanner.tsxapps/citizen-pwa/src/components/ui/Timeline.tsxapps/citizen-pwa/src/hooks/useReport.tsapps/citizen-pwa/src/hooks/useSubmissionMachine.tsapps/citizen-pwa/src/lib/QueryProvider.tsxapps/citizen-pwa/src/lib/design-tokens.tsapps/citizen-pwa/src/lib/draftManager.tsapps/citizen-pwa/src/lib/localforage.tsapps/citizen-pwa/src/lib/mappers.tsapps/citizen-pwa/src/lib/photoUpload.tsapps/citizen-pwa/src/lib/query-client.tsxapps/citizen-pwa/src/lib/routerGuard.tsapps/citizen-pwa/src/lib/store.tsapps/citizen-pwa/src/main.tsxapps/citizen-pwa/src/routes.tsxapps/citizen-pwa/src/styles/design-tokens.cssapps/citizen-pwa/src/styles/globals.csse2e-tests/a11y/citizen-pwa-a11y.spec.tse2e-tests/specs/citizen-pwa.spec.ts
💤 Files with no reviewable changes (3)
- apps/citizen-pwa/src/components/ReceiptScreen.tsx
- apps/citizen-pwa/src/components/LookupScreen.tsx
- apps/citizen-pwa/src/components/SubmitReportForm.tsx
Comment on lines
+23
to
+61
| it('should render submit report form steps', () => { | ||
| render( | ||
| <TestWrapper> | ||
| <div>Submit report form test placeholder</div> | ||
| </TestWrapper>, | ||
| ) | ||
|
|
||
| expect(screen.getByText('Submit report form test placeholder')).toBeInTheDocument() | ||
| }) | ||
|
|
||
| it('should show Step1Evidence with incident type selection', () => { | ||
| render( | ||
| <TestWrapper> | ||
| <div>Step 1 Evidence placeholder</div> | ||
| </TestWrapper>, | ||
| ) | ||
|
|
||
| expect(screen.getByText('Step 1 Evidence placeholder')).toBeInTheDocument() | ||
| }) | ||
|
|
||
| it('should show Step3Review with review content', () => { | ||
| render( | ||
| <TestWrapper> | ||
| <div>Review your report placeholder</div> | ||
| </TestWrapper>, | ||
| ) | ||
|
|
||
| expect(screen.getByText('Review your report placeholder')).toBeInTheDocument() | ||
| }) | ||
|
|
||
| it('should complete full happy path submission flow', () => { | ||
| render( | ||
| <TestWrapper> | ||
| <div>Form submission flow placeholder</div> | ||
| </TestWrapper>, | ||
| ) | ||
|
|
||
| expect(screen.getByText('Form submission flow placeholder')).toBeInTheDocument() | ||
| }) |
Contributor
There was a problem hiding this comment.
These tests are placeholders and don’t validate submission flow behavior.
Line 23-Line 61 currently asserts static placeholder strings, so regressions in SubmitReportForm/step transitions won’t be detected.
As per coding guidelines **/*.test.{ts,tsx,js,jsx}: “Write tests that verify the new code is actually invoked, not tests that pass trivially.”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/citizen-pwa/src/__tests__/submit-flow.test.tsx` around lines 23 - 61,
The tests are currently using static placeholders and don't exercise
SubmitReportForm or its step components; replace each placeholder render with
rendering the real SubmitReportForm (wrapped by TestWrapper), then simulate user
interactions (use userEvent/fireEvent) to choose an incident type in
Step1Evidence, fill required fields, navigate to Step3Review, assert that
Step3Review content appears and that submit triggers the expected outcome (e.g.,
mock submit handler called, API mocked response handled, navigation or success
message shown); update test names to reflect these behaviors and mock any
external dependencies so assertions verify actual component logic and step
transitions rather than trivial string presence.
Comment on lines
+59
to
+61
| const handleTrackReport = () => { | ||
| window.location.href = `/reports/${referenceCode}` | ||
| } |
Contributor
There was a problem hiding this comment.
Use React Router navigation instead of window.location.href.
window.location.href causes a full page reload, losing React state and any in-memory data. Since the app uses react-router-dom (see routes.tsx), use the useNavigate hook for SPA navigation.
🔧 Proposed fix
+import { useNavigate } from 'react-router-dom'
+
export function RevealSheet({ state, referenceCode, onClose }: RevealSheetProps) {
const closeSheet = useUIStore((s) => s.closeSheet)
+ const navigate = useNavigate()
// ...
const handleTrackReport = () => {
- window.location.href = `/reports/${referenceCode}`
+ navigate(`/reports/${referenceCode}`)
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/citizen-pwa/src/components/RevealSheet.tsx` around lines 59 - 61, The
handler handleTrackReport in RevealSheet.tsx uses window.location.href which
triggers a full reload; switch to react-router navigation by importing
useNavigate from react-router-dom, creating const navigate = useNavigate()
inside the RevealSheet component, and replacing the window.location.href
assignment with navigate(`/reports/${referenceCode}`) so navigation is
SPA-friendly and preserves React state.
Comment on lines
+79
to
+111
| const timelineEvents = { | ||
| success: [ | ||
| { label: 'Report received', meta: '2:14 PM', state: 'complete' as const }, | ||
| { label: 'First review', meta: 'Expected within 15 min', state: 'pending' as const }, | ||
| { | ||
| label: 'Responder dispatched', | ||
| meta: "We'll text and update here", | ||
| state: 'pending' as const, | ||
| }, | ||
| ], | ||
| queued: [ | ||
| { label: 'Saved on this phone', meta: '2:14 PM', state: 'queued' as const }, | ||
| { | ||
| label: 'Send when online', | ||
| meta: 'Automatic · no action needed', | ||
| state: 'pending' as const, | ||
| }, | ||
| { | ||
| label: 'Received by MDRRMO', | ||
| meta: "We'll text you the reference", | ||
| state: 'pending' as const, | ||
| }, | ||
| ], | ||
| failed_retryable: [ | ||
| { label: 'Report drafted', meta: '2:14 PM', state: 'complete' as const }, | ||
| { | ||
| label: 'Send attempt failed', | ||
| meta: 'Network error · you can retry', | ||
| state: 'failed' as const, | ||
| }, | ||
| { label: 'Retry send', meta: 'Try again or call the hotline', state: 'pending' as const }, | ||
| ], | ||
| } |
Contributor
There was a problem hiding this comment.
Hardcoded "2:14 PM" timestamps are misleading.
Timeline events display a static "2:14 PM" regardless of actual submission time. This could confuse users. Consider using the actual timestamp or a relative indicator like "Just now".
🔧 Example fix for success timeline
+const formatTime = () => new Date().toLocaleTimeString([], { hour: 'numeric', minute: '2-digit' })
+
const timelineEvents = {
success: [
- { label: 'Report received', meta: '2:14 PM', state: 'complete' as const },
+ { label: 'Report received', meta: formatTime(), state: 'complete' as const },
// ...
],🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/citizen-pwa/src/components/RevealSheet.tsx` around lines 79 - 111, The
timelineEvents object currently uses hardcoded "2:14 PM" strings; replace those
static meta values by computing formatted times from the real report timestamp
(e.g., a prop or event.createdAt) or a relative label ("Just now", "5m ago").
Update RevealSheet.tsx where timelineEvents is built: add a helper (e.g.,
formatTime or formatRelative) and call it to set meta for entries in success,
queued, and failed_retryable instead of the literal "2:14 PM"; ensure you pass
the correct timestamp source (report.createdAt or a submittedAt prop) and fall
back to a sensible default when missing.
Comment on lines
+313
to
+323
| .sr-only { | ||
| position: absolute; | ||
| width: 1px; | ||
| height: 1px; | ||
| padding: 0; | ||
| margin: -1px; | ||
| overflow: hidden; | ||
| clip: rect(0, 0, 0, 0); | ||
| white-space: nowrap; | ||
| border: 0; | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find apps/citizen-pwa/src/styles -name "globals.css" -type fRepository: Exc1D/bantayog-alert
Length of output: 103
🏁 Script executed:
sed -n '313,323p' apps/citizen-pwa/src/styles/globals.cssRepository: Exc1D/bantayog-alert
Length of output: 239
🌐 Web query:
CSS clip property deprecated clip-path inset(50%) replacement 2024
💡 Result:
The CSS clip property is deprecated. It has been replaced by the clip-path property. Specifically, the common usage of clip: rect(0,0,0,0) (often for visually hiding elements while keeping them accessible to screen readers) is replaced by clip-path: inset(50%). Key details: - clip only works on absolutely positioned elements and uses rect(top, right, bottom, left). - clip-path: inset(50%) clips an equal amount (50%) from all sides, effectively hiding the entire element visually regardless of position. - This replacement is recommended by MDN and widely adopted (e.g., Tailwind CSS sr-only utility updated in 2025). - No specific "2024" changes noted; deprecation is longstanding, with modern usage stable since ~2020. Example replacement: .sr-only { position: absolute; width: 1px; height: 1px; padding: 0; margin: -1px; overflow: hidden; clip-path: inset(50%); /* Replaces clip: rect(0,0,0,0); */ white-space: nowrap; border: 0; }
Citations:
- 1: https://www.w3schools.com/cssref/pr_pos_clip.php
- 2: Suggestion: Replace deprecated
clipinsr-onlywithclip-pathtailwindlabs/tailwindcss#18768 - 3: https://developer.mozilla.org/en-US/docs/Web/CSS/clip
- 4: https://developer.mozilla.org/en-US/docs/CSS/clip
- 5: https://developer.mozilla.org/en-US/docs/Web/CSS/Reference/Properties/clip-path
- 6: https://css-tricks.com/almanac/properties/c/clip-path/
- 7: https://developer.mozilla.org/en-US/docs/Web/CSS/basic-shape/inset
🌐 Web query:
clip-path inset() browser compatibility support IE Edge Chrome Firefox
💡 Result:
CSS clip-path with inset has good modern browser support but requires prefixes in some cases and no support in IE. Browser compatibility as of 2026: - Internet Explorer: No support (all versions up to IE11). - Edge: Partial support from Edge 79+ (Chromium-based); no support in Edge 12-18 (legacy). - Chrome: Partial support from Chrome 24+, full basic shape support from Chrome 23+. Prefix -webkit-clip-path often needed for older versions; recent versions (146+) show partial but inset works. - Firefox: Full support from Firefox 54+; partial 3.5-53. - Safari: Partial support from Safari 7+. MDN confirms inset as standard basic shape function. CanIUse notes partial support often due to external SVG references or specific shapes, but inset rectangles work reliably in modern browsers. For production, use both clip-path: inset and -webkit-clip-path: inset for maximum compatibility.
Citations:
- 1: https://caniuse.com/css-clip-path
- 2: https://caniuse.com/css-clip
- 3: https://developer.mozilla.org/en-US/docs/Web/CSS/Reference/Properties/clip-path
- 4: https://developer.mozilla.org/en-US/docs/Web/CSS/Reference/Values/basic-shape/inset
- 5: https://caniuse.com/mdn-css_properties_clip-path_basic_shape
Replace deprecated clip property with clip-path: inset(50%).
The clip property is deprecated per CSS specifications. Modern browsers support clip-path: inset(50%) as the replacement for screen-reader utilities, which is more efficient and position-independent. This is now the standard practice adopted by frameworks like Tailwind CSS.
Proposed fix
.sr-only {
position: absolute;
width: 1px;
height: 1px;
padding: 0;
margin: -1px;
overflow: hidden;
- clip: rect(0, 0, 0, 0);
+ clip-path: inset(50%);
white-space: nowrap;
border: 0;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| .sr-only { | |
| position: absolute; | |
| width: 1px; | |
| height: 1px; | |
| padding: 0; | |
| margin: -1px; | |
| overflow: hidden; | |
| clip: rect(0, 0, 0, 0); | |
| white-space: nowrap; | |
| border: 0; | |
| } | |
| .sr-only { | |
| position: absolute; | |
| width: 1px; | |
| height: 1px; | |
| padding: 0; | |
| margin: -1px; | |
| overflow: hidden; | |
| clip-path: inset(50%); | |
| white-space: nowrap; | |
| border: 0; | |
| } |
🧰 Tools
🪛 Stylelint (17.7.0)
[error] 320-320: Deprecated property "clip" (property-no-deprecated)
(property-no-deprecated)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/citizen-pwa/src/styles/globals.css` around lines 313 - 323, The .sr-only
utility uses the deprecated clip property; update the rule for .sr-only to
remove the clip(...) usage and add clip-path: inset(50%); (and optionally
-webkit-clip-path: inset(50%); for legacy WebKit) while keeping the other
accessibility styles (position, width, height, padding, margin, overflow,
white-space, border) intact so the element remains visually hidden but
accessible to screen readers.
Comment on lines
+8
to
+10
| test('should be keyboard navigable', async ({ page }) => { | ||
| await page.keyboard.press('Tab') | ||
| }) |
Contributor
There was a problem hiding this comment.
Add a real keyboard-navigation assertion.
Line 8-Line 10 only sends a Tab keypress; this passes even if focus handling is broken.
Suggested test hardening
-import { test } from '@playwright/test'
+import { expect, test } from '@playwright/test'
@@
test('should be keyboard navigable', async ({ page }) => {
+ const before = await page.evaluate(() => document.activeElement?.tagName)
await page.keyboard.press('Tab')
+ const after = await page.evaluate(() => document.activeElement?.tagName)
+ expect(after).not.toBe(before)
})As per coding guidelines **/*.test.{ts,tsx,js,jsx}: “Write tests that verify the new code is actually invoked, not tests that pass trivially.”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@e2e-tests/a11y/citizen-pwa-a11y.spec.ts` around lines 8 - 10, The test
"should be keyboard navigable" currently only calls page.keyboard.press('Tab')
and has no assertion; update this test to assert actual focus change after the
keypress (e.g., call page.keyboard.press('Tab') and then verify the focused
element using page.locator(...).evaluate(el => document.activeElement) or
page.evaluate(() => document.activeElement) or Playwright's
page.locator(selector).isFocused()); reference the existing test block
(test('should be keyboard navigable', async ({ page }) => { ... })) and replace
the trivial call with a keypress plus an assertion that the expected interactive
element (button/link/input with a specific selector or text) receives focus.
Comment on lines
+4
to
+10
| test.beforeEach(async ({ page }) => { | ||
| await page.goto('/') | ||
|
|
||
| await page.evaluate(() => { | ||
| localStorage.clear() | ||
| }) | ||
| }) |
Contributor
There was a problem hiding this comment.
Clear persisted state before the app boots.
page.goto('/') runs before localStorage.clear(), so the app can hydrate from stale draft/query-cache state first and keep it for the whole test. Move the clear into page.addInitScript(...) or clear and reload before asserting behavior.
Suggested fix
test.beforeEach(async ({ page }) => {
- await page.goto('/')
-
- await page.evaluate(() => {
- localStorage.clear()
- })
+ await page.addInitScript(() => {
+ window.localStorage.clear()
+ })
+ await page.goto('/')
})🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@e2e-tests/specs/citizen-pwa.spec.ts` around lines 4 - 10, test.beforeEach
currently calls page.goto('/') before clearing localStorage, so the app may
hydrate from stale state; change the setup to clear persisted state before the
app boots by using page.addInitScript(() => localStorage.clear()) prior to
page.goto('/') (or alternatively call page.evaluate(() => localStorage.clear())
and then immediately reload before interacting). Update the test.beforeEach to
invoke page.addInitScript for clearing localStorage (or clear+reload) so the app
never sees stale draft/query-cache on first load.
Comment on lines
+15
to
+17
| await page.click('button:has-text("Capture location")') | ||
|
|
||
| await page.waitForTimeout(1000) |
Contributor
There was a problem hiding this comment.
Replace the fixed sleep with a state-based wait.
waitForTimeout(1000) will be flaky on slower CI/device runs and just burns time on fast ones. Wait for the next stable UI state instead and let Playwright auto-wait on the actual control you need.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@e2e-tests/specs/citizen-pwa.spec.ts` around lines 15 - 17, The test uses a
fixed sleep after clicking the "Capture location" button
(page.click('button:has-text("Capture location")') followed by
page.waitForTimeout(1000')) which is flaky and slow; replace the sleep with a
state-based wait that targets the actual UI change or readiness rather than a
timeout—for example wait for a specific selector/text/attribute that appears or
becomes enabled after capture (use page.waitForSelector or locator.waitFor with
the appropriate visible/attached/timeout options, or wait for the network
response if applicable) so Playwright's auto-wait handles timing
deterministically instead of using page.waitForTimeout.
Comment on lines
+36
to
+48
| test.skip('should handle offline state and retry', async () => { | ||
| // TODO: Simulate offline mode, queue submission, retry on reconnect | ||
| }) | ||
|
|
||
| test.skip('should track report updates in real-time', async () => { | ||
| // TODO: Navigate to tracking, simulate admin status update via Firebase Emulator | ||
| }) | ||
| }) | ||
|
|
||
| test.describe('Reveal sheet back button guard', () => { | ||
| test.skip('should block back on queued state', async () => { | ||
| // TODO: Trigger submission that goes to queued, test back button guard | ||
| }) |
Contributor
There was a problem hiding this comment.
Remove bare TODO placeholders or attach a ticket.
These skipped specs are carrying untracked TODO comments in a TypeScript file. Please either link them to an explicit ticket or convert them into actionable issues so they do not become permanent dead tests. As per coding guidelines, "Do not use any, // @ts-ignore``, or TODO without explicit permission or a ticket reference."
If you want, I can turn these placeholders into concrete emulator-backed test cases or ticket-ready acceptance criteria.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@e2e-tests/specs/citizen-pwa.spec.ts` around lines 36 - 48, These skipped
specs contain bare "TODO" comments; replace them with either (a) concrete test
implementations using the Playwright test names 'should handle offline state and
retry', 'should track report updates in real-time', and 'should block back on
queued state' or (b) annotate each TODO with a tracked ticket reference (e.g.
TODO: TICKET-123 or link to issue) and update the test description or a comment
to include the ticket ID so they are not untracked placeholders; specifically
edit the three test.skip blocks to remove bare TODOs by either implementing
emulator-backed tests (simulate offline mode / Firebase Emulator updates /
queued submission + back-button) or by adding the ticket reference and a short
acceptance-criteria summary in the comment.
Critical fixes: - useReport: restructure hooks to eliminate conditional hook calls (guard moved into hook bodies via hasRef boolean) - useReport: wrap mapReportFromFirestore in try-catch inside onSnapshot - useReport: async queryFn with enabled+placeholderData prevents flash - TrackingScreen: isPending instead of isLoading for correct loading state - RevealSheet: handlePrimaryAction calls onClose for queued/failed_retryable - inboxPayloadSchema: add municipalityId/barangayId/nearestLandmark fields - submit-report.ts: extend SubmitReportInput with location fields - SubmitReportForm: call submitReport() service (fixes secret discard + DRY) - putBlob: include HTTP response body in error message High priority fixes: - Step1Evidence: fix INCIDENT_TYPES to match reportDocSchema values - Step1Evidence: URL.revokeObjectURL cleanup via useRef+useEffect - Step1Evidence: "No photo" button properly skips instead of re-opening picker - Step3Review: fix INCIDENT_TYPES; third progress dot now active - Step2WhoWhere: nearestLandmark initialized as empty string - Step2WhoWhere: GPS catch block logs error (not swallowed) - submit-flow.test.tsx: replace placeholders with it.todo stubs - test-utils.tsx: add MemoryRouter for react-router-dom compatibility Docs: update progress.md and learnings.md Merge: resolve add/add conflicts in design-tokens (identical on both sides)
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 10
♻️ Duplicate comments (6)
apps/citizen-pwa/src/components/TrackingScreen.tsx (2)
141-145:⚠️ Potential issue | 🟠 MajorWire up or disable the inactive CTAs.
“Update report” and “Re-open if situation changed” still render as active actions, but neither button does anything.
Also applies to: 156-159
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/citizen-pwa/src/components/TrackingScreen.tsx` around lines 141 - 145, The "Update report" and "Re-open if situation changed" CTAs in the TrackingScreen render as active but have no behavior; locate the Button components inside the div with className "tracking-actions" (and the similar Buttons around lines where the second CTA appears) and either wire them to appropriate handlers (e.g., call existing functions like onUpdateReport or onReopenReport, or add new callbacks that dispatch the necessary action/route/modal) or mark them disabled and add an explanatory aria-label/tooltip; ensure you add onClick bindings for the correct Button symbols and keep accessibility attributes (disabled and aria-disabled/aria-label) consistent.
90-95:⚠️ Potential issue | 🟡 MinorGuard coordinate formatting before calling
toFixed().This can still render partial
undefinedcoordinates in the UI. Only format when both values are finite numbers; otherwise showN/A.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/citizen-pwa/src/components/TrackingScreen.tsx` around lines 90 - 95, In TrackingScreen (component rendering the "Coords" card-row), avoid calling toFixed on possibly undefined values: check that report.location?.lat and report.location?.lng are both finite numbers (Number.isFinite) before formatting with toFixed(5); if either is not a finite number render "N/A" (or equivalent) instead of attempting to format, ensuring no partial "undefined" shows in the card-value.apps/citizen-pwa/src/components/RevealSheet.tsx (2)
76-107:⚠️ Potential issue | 🟡 MinorReplace the placeholder timeline times.
The static
2:14 PMvalues are still misleading and will show the wrong submission state to every user.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/citizen-pwa/src/components/RevealSheet.tsx` around lines 76 - 107, The timelineEvents object in RevealSheet.tsx currently hardcodes "2:14 PM" which is misleading; update the timelineEvents construction to use the report's actual timestamp fields (e.g., report.createdAt, report.submittedAt, report.sentAt) or compute relative times (e.g., formatRelative/formatDistanceToNow) and replace the static meta values, editing the timelineEvents constant in the RevealSheet component so each event's meta shows a real formatted time or relative string derived from the report data.
56-58:⚠️ Potential issue | 🟡 MinorUse router navigation for in-app tracking.
This still forces a full reload instead of SPA navigation, which drops in-memory UI/query state.
Proposed fix
+import { useNavigate } from 'react-router-dom' @@ export function RevealSheet({ state, referenceCode, onClose }: RevealSheetProps) { + const navigate = useNavigate() @@ const handleTrackReport = () => { - window.location.href = `/reports/${referenceCode}` + navigate(`/reports/${referenceCode}`) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/citizen-pwa/src/components/RevealSheet.tsx` around lines 56 - 58, handleTrackReport currently uses window.location.href which triggers a full reload; replace it with SPA navigation via the app router. Import and use the router hook your app uses (e.g., useRouter from next/router or useNavigate from react-router-dom) and call router.push(`/reports/${referenceCode}`) (or navigate(`/reports/${referenceCode}`)) inside handleTrackReport so in-memory UI/query state is preserved; update the RevealSheet component to obtain the router instance and use it instead of window.location.href.apps/citizen-pwa/src/components/SubmitReportForm/Step1Evidence.tsx (1)
44-53:⚠️ Potential issue | 🟠 MajorClear the file state when the user removes or cancels a photo.
Line 45 can be
nullwhen the picker is cancelled, and Lines 101-103 only hide the preview. In both cases the previousphotoFilecan still be submitted even though the UI says there is no photo.Proposed fix
const handlePhotoChange = (e: React.ChangeEvent<HTMLInputElement>) => { const file = e.target.files?.[0] ?? null - setPhotoFile(file) - if (file) { - if (blobUrlRef.current) { - URL.revokeObjectURL(blobUrlRef.current) - } - blobUrlRef.current = URL.createObjectURL(file) - setPreviewUrl(blobUrlRef.current) - } + if (!file) { + if (blobUrlRef.current) { + URL.revokeObjectURL(blobUrlRef.current) + blobUrlRef.current = null + } + setPhotoFile(null) + setPreviewUrl(null) + return + } + + setPhotoFile(file) + if (blobUrlRef.current) { + URL.revokeObjectURL(blobUrlRef.current) + } + blobUrlRef.current = URL.createObjectURL(file) + setPreviewUrl(blobUrlRef.current) } @@ onClick={() => { + if (blobUrlRef.current) { + URL.revokeObjectURL(blobUrlRef.current) + blobUrlRef.current = null + } + setPhotoFile(null) setPreviewUrl(null) }}Also applies to: 99-107
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/citizen-pwa/src/components/SubmitReportForm/Step1Evidence.tsx` around lines 44 - 53, The file input change handler handlePhotoChange currently only sets photoFile when a file exists, but doesn't clear photoFile when the picker is cancelled (e.target.files?.[0] is null), and the preview-hide code (the remove/cancel handler around lines 99-107) only hides the preview instead of clearing state; update handlePhotoChange to setPhotoFile(null) and clear/setPreviewUrl(null) and revoke any existing blobUrlRef.current when e.target.files is empty, and update the preview removal handler to also revoke blobUrlRef.current, setPreviewUrl(null) and setPhotoFile(null) so the previous file cannot be submitted after cancellation or removal.apps/citizen-pwa/src/components/SubmitReportForm/Step2WhoWhere.tsx (1)
319-320:⚠️ Potential issue | 🟡 MinorInitialize
selectedBarangayIdwith empty string instead ofundefinedfor consistent controlled select behavior.The barangay select is a controlled component that transitions from
undefined→string→undefined, which can trigger React warnings about uncontrolled→controlled mismatches. Initialize and reset it to''to maintain consistent typing and prevent the flip.Locations to update
Line 320: Initialize state
- const [selectedBarangayId, setSelectedBarangayId] = useState<string | undefined>(undefined) + const [selectedBarangayId, setSelectedBarangayId] = useState('')Line 356: Reset in
handleSelectMunicipality- setSelectedBarangayId(undefined) + setSelectedBarangayId('')Line 529: The select's value is already correctly referenced.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/citizen-pwa/src/components/SubmitReportForm/Step2WhoWhere.tsx` around lines 319 - 320, The barangay select flips between undefined and string causing uncontrolled→controlled warnings; update the state initialization and reset to use an empty string: change the useState for selectedBarangayId to useState('') and update handleSelectMunicipality to call setSelectedBarangayId('') instead of undefined so the controlled select (selectedBarangayId / setSelectedBarangayId) remains a consistent string-based value.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/citizen-pwa/src/__tests__/submit-flow.test.tsx`:
- Around line 5-9: The test mock for the firebase module uses db: {} and fns: {}
but SubmitReportForm calls db() and fns(), causing "is not a function" errors;
update the vi.mock('../services/firebase') in submit-flow.test.tsx to export db
and fns as functions (e.g., vi.fn() returning the expected stubs) while keeping
ensureSignedIn mocked, so that calls from SubmitReportForm (the component that
invokes db() and fns()) receive callable mocks that return the shape your
component expects.
In `@apps/citizen-pwa/src/components/RevealSheet.tsx`:
- Around line 112-119: The backdrop in RevealSheet (the div with className
"reveal-backdrop") currently has role="button" and tabIndex={0} even when not
actionable; change the element so role and tabIndex are only present when state
=== 'success' (i.e., the same condition used for onClick/onKeyDown) to avoid
creating a non-functional keyboard focus target; update the JSX to conditionally
include role and tabIndex (or set aria-hidden) and ensure onClick/onKeyDown
remain gated by state === 'success'.
- Around line 35-38: The primary CTA labels for the 'queued' and
'failed_retryable' states currently only close the sheet; change the primary
button handler to call the actual send/retry logic (e.g., invoke the existing
send/retry function passed in props such as onSend/onRetry or the internal
sendQueuedReport()/retrySend() helper), then close the sheet on success/failure
as appropriate; update the handlers tied to the primaryButton for the blocks
around the label lines (the states 'queued' and 'failed_retryable' and the
related UI at lines noted) so the primary action performs the retry/send
operation instead of only closing the sheet.
- Around line 19-22: The copy in RevealSheet.tsx still hardcodes "Daet MDRRMO"
in the receiverText (and the same hardcoded municipality appears again around
lines 30-34); update those fields to use generic recipient copy (e.g., "Received
by your local MDRRMO" or "Received by the municipal office") instead of a
specific city name, by editing the object/props assignment for
headline/subline/receiverText inside the RevealSheet component so all
occurrences of "Daet MDRRMO" are replaced with a generic string.
In `@apps/citizen-pwa/src/components/SubmitReportForm/Step2WhoWhere.tsx`:
- Around line 351-354: The useEffect mounting call to attemptGps() causes GPS
permission flow to start immediately and bypasses the manual-location chooser;
remove the unconditional call in the component mount effect and instead invoke
attemptGps() only when the user explicitly selects GPS mode (i.e., from the UI
control that toggles gps/manual mode) or when a new state like autoAttemptGps is
true; update the handler that switches to GPS mode to set gpsLoading and call
attemptGps() (references: the attemptGps() function and gpsLoading state and the
manual-location chooser rendering), ensuring the mount effect no longer
auto-triggers GPS permission requests.
In `@apps/citizen-pwa/src/components/SubmitReportForm/Step3Review.tsx`:
- Around line 124-135: The privacy notice text inside Step3Review is currently a
non-interactive <span> styled like a link, so make it keyboard-focusable and
announced as interactive: extract the "Privacy notice ›" into a dedicated
interactive element (e.g., a <button> or <a> with href) inside the Step3Review
component, give it clear accessible text/aria-label, an onClick handler (e.g.,
openPrivacyNotice) that opens the privacy modal or navigates to the privacy
page, and ensure it is reachable before consent is checked (do not gate it
behind setConsent or require the consent checkbox). Update the JSX around
consent/consent-checkbox-label and replace the inline <u> element with this
interactive element, and add any needed handler function (openPrivacyNotice) and
keyboard/ARIA attributes.
- Around line 2-11: The module is using an undefined Zap icon as a fallback in
the Step3Review component (used with INCIDENT_TYPES), causing a runtime error;
fix it by adding Zap to the existing lucide-react import list at the top of
Step3Review.tsx (alongside ArrowLeft, Heart, Droplets, Flame, Wind, Mountain,
Waves, AlertTriangle) so the fallback icon reference Zap is defined when
INCIDENT_TYPES lookup fails.
In `@apps/citizen-pwa/src/components/TrackingScreen.tsx`:
- Around line 48-64: statusConfig only covers
verified/resolved/rejected/awaiting_verify so many valid ReportStatus values
(e.g., assigned, acknowledged, en_route, on_scene, closed, cancelled) fall
through to awaiting_verify; update the statusConfig object to include entries
for all ReportStatus enum members used by the backend (use the ReportStatus enum
names such as assigned, acknowledged, en_route, on_scene, closed, cancelled)
with appropriate icon and text, and adjust the statusVariant calculation (the
statusVariant const) so it maps those statuses to the correct variant strings
(e.g., en_route/on_scene => 'in_progress' or appropriate existing variant)
instead of defaulting to queued; reference statusConfig, statusVariant, config
and report.status when making these changes.
In `@apps/citizen-pwa/src/hooks/useReport.ts`:
- Around line 81-97: The onSnapshot call inside the Promise in useReport's
queryFn currently has no error callback and the mapReportFromFirestore catch
swallows errors; update the onSnapshot invocation (the call that assigns const
unsub = onSnapshot(...)) to pass an error callback that logs the Firestore error
(e.g. use console.error('Report fetch error:', error.message)) and resolves null
then unsubscribes, and inside the try/catch around mapReportFromFirestore add
logging for the mapper failure (e.g. console.error('Report mapping error:', err
instanceof Error ? err.message : err)) before resolving null so the promise
never hangs and mapping failures are observable.
In `@apps/citizen-pwa/src/services/submit-report.ts`:
- Around line 74-79: When building the inbox payload in submit-report.ts,
defensive-normalize the optional location fields on input (municipalityId,
barangayId, nearestLandmark) by trimming them and only including non-empty
strings in the spread so you don't serialize '' and fail inboxPayloadSchema's
min(1) check; update the payload construction that currently spreads
...(input.municipalityId ? { municipalityId: input.municipalityId, barangayId:
input.barangayId, nearestLandmark: input.nearestLandmark } : {}) to trim and
conditionally include barangayId and nearestLandmark (e.g., compute trimmed
values from input.barangayId/input.nearestLandmark and only add those keys when
trimmed.length>0) so only valid non-empty values reach inboxPayload validation.
---
Duplicate comments:
In `@apps/citizen-pwa/src/components/RevealSheet.tsx`:
- Around line 76-107: The timelineEvents object in RevealSheet.tsx currently
hardcodes "2:14 PM" which is misleading; update the timelineEvents construction
to use the report's actual timestamp fields (e.g., report.createdAt,
report.submittedAt, report.sentAt) or compute relative times (e.g.,
formatRelative/formatDistanceToNow) and replace the static meta values, editing
the timelineEvents constant in the RevealSheet component so each event's meta
shows a real formatted time or relative string derived from the report data.
- Around line 56-58: handleTrackReport currently uses window.location.href which
triggers a full reload; replace it with SPA navigation via the app router.
Import and use the router hook your app uses (e.g., useRouter from next/router
or useNavigate from react-router-dom) and call
router.push(`/reports/${referenceCode}`) (or
navigate(`/reports/${referenceCode}`)) inside handleTrackReport so in-memory
UI/query state is preserved; update the RevealSheet component to obtain the
router instance and use it instead of window.location.href.
In `@apps/citizen-pwa/src/components/SubmitReportForm/Step1Evidence.tsx`:
- Around line 44-53: The file input change handler handlePhotoChange currently
only sets photoFile when a file exists, but doesn't clear photoFile when the
picker is cancelled (e.target.files?.[0] is null), and the preview-hide code
(the remove/cancel handler around lines 99-107) only hides the preview instead
of clearing state; update handlePhotoChange to setPhotoFile(null) and
clear/setPreviewUrl(null) and revoke any existing blobUrlRef.current when
e.target.files is empty, and update the preview removal handler to also revoke
blobUrlRef.current, setPreviewUrl(null) and setPhotoFile(null) so the previous
file cannot be submitted after cancellation or removal.
In `@apps/citizen-pwa/src/components/SubmitReportForm/Step2WhoWhere.tsx`:
- Around line 319-320: The barangay select flips between undefined and string
causing uncontrolled→controlled warnings; update the state initialization and
reset to use an empty string: change the useState for selectedBarangayId to
useState('') and update handleSelectMunicipality to call
setSelectedBarangayId('') instead of undefined so the controlled select
(selectedBarangayId / setSelectedBarangayId) remains a consistent string-based
value.
In `@apps/citizen-pwa/src/components/TrackingScreen.tsx`:
- Around line 141-145: The "Update report" and "Re-open if situation changed"
CTAs in the TrackingScreen render as active but have no behavior; locate the
Button components inside the div with className "tracking-actions" (and the
similar Buttons around lines where the second CTA appears) and either wire them
to appropriate handlers (e.g., call existing functions like onUpdateReport or
onReopenReport, or add new callbacks that dispatch the necessary
action/route/modal) or mark them disabled and add an explanatory
aria-label/tooltip; ensure you add onClick bindings for the correct Button
symbols and keep accessibility attributes (disabled and
aria-disabled/aria-label) consistent.
- Around line 90-95: In TrackingScreen (component rendering the "Coords"
card-row), avoid calling toFixed on possibly undefined values: check that
report.location?.lat and report.location?.lng are both finite numbers
(Number.isFinite) before formatting with toFixed(5); if either is not a finite
number render "N/A" (or equivalent) instead of attempting to format, ensuring no
partial "undefined" shows in the card-value.
🪄 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: 12838d4b-7de7-47b1-8ef0-f593d24ac686
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (12)
apps/citizen-pwa/package.jsonapps/citizen-pwa/src/__tests__/submit-flow.test.tsxapps/citizen-pwa/src/__tests__/test-utils.tsxapps/citizen-pwa/src/components/RevealSheet.tsxapps/citizen-pwa/src/components/SubmitReportForm/Step1Evidence.tsxapps/citizen-pwa/src/components/SubmitReportForm/Step2WhoWhere.tsxapps/citizen-pwa/src/components/SubmitReportForm/Step3Review.tsxapps/citizen-pwa/src/components/SubmitReportForm/index.tsxapps/citizen-pwa/src/components/TrackingScreen.tsxapps/citizen-pwa/src/hooks/useReport.tsapps/citizen-pwa/src/services/submit-report.tspackages/shared-validators/src/reports.ts
Comment on lines
+5
to
+9
| vi.mock('../services/firebase', () => ({ | ||
| db: {}, | ||
| fns: {}, | ||
| ensureSignedIn: vi.fn().mockResolvedValue('test-uid'), | ||
| })) |
Contributor
There was a problem hiding this comment.
Mock db and fns as functions.
SubmitReportForm calls fns() and db() in apps/citizen-pwa/src/components/SubmitReportForm/index.tsx at Lines 84-95. With this mock shape, the suite will fail with fns is not a function / db is not a function as soon as it mounts the real flow instead of placeholder nodes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/citizen-pwa/src/__tests__/submit-flow.test.tsx` around lines 5 - 9, The
test mock for the firebase module uses db: {} and fns: {} but SubmitReportForm
calls db() and fns(), causing "is not a function" errors; update the
vi.mock('../services/firebase') in submit-flow.test.tsx to export db and fns as
functions (e.g., vi.fn() returning the expected stubs) while keeping
ensureSignedIn mocked, so that calls from SubmitReportForm (the component that
invokes db() and fns()) receive callable mocks that return the shape your
component expects.
Comment on lines
+19
to
+22
| headline: 'We heard you. We are here.', | ||
| subline: 'Your report is with Daet MDRRMO. Keep your line open.', | ||
| bannerVariant: 'success' as const, | ||
| receiverText: 'Received by Daet MDRRMO', |
Contributor
There was a problem hiding this comment.
Remove the remaining municipality-specific copy.
This still hardcodes Daet MDRRMO, so users outside Daet will see the wrong recipient even though this PR’s objective is to make the copy generic.
Also applies to: 30-34
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/citizen-pwa/src/components/RevealSheet.tsx` around lines 19 - 22, The
copy in RevealSheet.tsx still hardcodes "Daet MDRRMO" in the receiverText (and
the same hardcoded municipality appears again around lines 30-34); update those
fields to use generic recipient copy (e.g., "Received by your local MDRRMO" or
"Received by the municipal office") instead of a specific city name, by editing
the object/props assignment for headline/subline/receiverText inside the
RevealSheet component so all occurrences of "Daet MDRRMO" are replaced with a
generic string.
Comment on lines
+35
to
+38
| primaryButton: 'Try sending now', | ||
| primaryVariant: 'amber' as const, | ||
| secondaryButton: 'Keep draft & close', | ||
| permissionText: "We'll keep trying in the background.", |
Contributor
There was a problem hiding this comment.
Wire the primary CTA to an actual retry/send action.
For queued and failed_retryable, the button text says “Try sending now” / “Try again”, but the handler just closes the sheet. That is a broken primary action.
Also applies to: 47-50, 60-65, 168-170
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/citizen-pwa/src/components/RevealSheet.tsx` around lines 35 - 38, The
primary CTA labels for the 'queued' and 'failed_retryable' states currently only
close the sheet; change the primary button handler to call the actual send/retry
logic (e.g., invoke the existing send/retry function passed in props such as
onSend/onRetry or the internal sendQueuedReport()/retrySend() helper), then
close the sheet on success/failure as appropriate; update the handlers tied to
the primaryButton for the blocks around the label lines (the states 'queued' and
'failed_retryable' and the related UI at lines noted) so the primary action
performs the retry/send operation instead of only closing the sheet.
Comment on lines
+112
to
+119
| <div | ||
| className="reveal-backdrop" | ||
| role="button" | ||
| tabIndex={0} | ||
| onClick={state === 'success' ? onClose : undefined} | ||
| onKeyDown={(e) => { | ||
| if ((e.key === 'Enter' || e.key === ' ') && state === 'success') onClose?.() | ||
| }} |
Contributor
There was a problem hiding this comment.
Don’t expose an inert backdrop as a button.
In queued and failed_retryable, this element still has role="button" and tabIndex={0} even though it cannot dismiss anything. That creates a dead keyboard focus target.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/citizen-pwa/src/components/RevealSheet.tsx` around lines 112 - 119, The
backdrop in RevealSheet (the div with className "reveal-backdrop") currently has
role="button" and tabIndex={0} even when not actionable; change the element so
role and tabIndex are only present when state === 'success' (i.e., the same
condition used for onClick/onKeyDown) to avoid creating a non-functional
keyboard focus target; update the JSX to conditionally include role and tabIndex
(or set aria-hidden) and ensure onClick/onKeyDown remain gated by state ===
'success'.
Comment on lines
+351
to
+354
| useEffect(() => { | ||
| // eslint-disable-next-line react-hooks/set-state-in-effect | ||
| void attemptGps() | ||
| }, []) |
Contributor
There was a problem hiding this comment.
The mount effect skips the manual-location chooser.
Lines 351-354 call attemptGps() immediately, which flips gpsLoading and triggers the permission flow before the user can choose the manual path rendered at Lines 436-460. That undermines the “GPS/manual modes” behavior this PR is adding.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/citizen-pwa/src/components/SubmitReportForm/Step2WhoWhere.tsx` around
lines 351 - 354, The useEffect mounting call to attemptGps() causes GPS
permission flow to start immediately and bypasses the manual-location chooser;
remove the unconditional call in the component mount effect and instead invoke
attemptGps() only when the user explicitly selects GPS mode (i.e., from the UI
control that toggles gps/manual mode) or when a new state like autoAttemptGps is
true; update the handler that switches to GPS mode to set gpsLoading and call
attemptGps() (references: the attemptGps() function and gpsLoading state and the
manual-location chooser rendering), ensuring the mount effect no longer
auto-triggers GPS permission requests.
Comment on lines
+124
to
+135
| <label className="consent-checkbox-label"> | ||
| <input | ||
| type="checkbox" | ||
| checked={consent} | ||
| onChange={(e) => { | ||
| setConsent(e.target.checked) | ||
| }} | ||
| className="consent-checkbox" | ||
| /> | ||
| <span className="consent-text-small"> | ||
| I confirm this report is true. You may contact me. <u>Privacy notice ›</u> | ||
| </span> |
Contributor
There was a problem hiding this comment.
Make the privacy notice an actual interactive element.
<u>Privacy notice ›</u> is styled like a link, but inside a plain <span> it is neither focusable nor announced as interactive. Users cannot open the notice before giving consent. As per coding guidelines "Major/high-impact issues: Compliance/privacy risks" and "Accessibility blockers that prevent task completion."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/citizen-pwa/src/components/SubmitReportForm/Step3Review.tsx` around
lines 124 - 135, The privacy notice text inside Step3Review is currently a
non-interactive <span> styled like a link, so make it keyboard-focusable and
announced as interactive: extract the "Privacy notice ›" into a dedicated
interactive element (e.g., a <button> or <a> with href) inside the Step3Review
component, give it clear accessible text/aria-label, an onClick handler (e.g.,
openPrivacyNotice) that opens the privacy modal or navigates to the privacy
page, and ensure it is reachable before consent is checked (do not gate it
behind setConsent or require the consent checkbox). Update the JSX around
consent/consent-checkbox-label and replace the inline <u> element with this
interactive element, and add any needed handler function (openPrivacyNotice) and
keyboard/ARIA attributes.
Comment on lines
+48
to
+64
| const statusVariant = | ||
| report.status === 'verified' || report.status === 'resolved' | ||
| ? 'success' | ||
| : report.status === 'rejected' | ||
| ? 'failed' | ||
| : 'queued' | ||
|
|
||
| const statusConfig = { | ||
| verified: { icon: <Zap size={16} />, text: 'Responders dispatched.' }, | ||
| resolved: { icon: <CheckCircle size={16} />, text: 'Situation is cleared.' }, | ||
| rejected: { icon: <XCircle size={16} />, text: 'Report could not be verified.' }, | ||
| awaiting_verify: { icon: <Eye size={16} />, text: 'Waiting for review.' }, | ||
| } | ||
|
|
||
| const config = | ||
| (statusConfig as Record<string, { icon: React.ReactNode; text: string }>)[report.status] ?? | ||
| statusConfig.awaiting_verify |
Contributor
There was a problem hiding this comment.
Handle the full ReportStatus enum here.
report.status can be assigned, acknowledged, en_route, on_scene, closed, cancelled, and several others from packages/shared-types/src/enums.ts:13-28, but all of those currently fall through to the awaiting_verify copy. That will show the wrong banner text/icon for valid server states.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/citizen-pwa/src/components/TrackingScreen.tsx` around lines 48 - 64,
statusConfig only covers verified/resolved/rejected/awaiting_verify so many
valid ReportStatus values (e.g., assigned, acknowledged, en_route, on_scene,
closed, cancelled) fall through to awaiting_verify; update the statusConfig
object to include entries for all ReportStatus enum members used by the backend
(use the ReportStatus enum names such as assigned, acknowledged, en_route,
on_scene, closed, cancelled) with appropriate icon and text, and adjust the
statusVariant calculation (the statusVariant const) so it maps those statuses to
the correct variant strings (e.g., en_route/on_scene => 'in_progress' or
appropriate existing variant) instead of defaulting to queued; reference
statusConfig, statusVariant, config and report.status when making these changes.
Comment on lines
+74
to
+79
| ...(input.municipalityId | ||
| ? { | ||
| municipalityId: input.municipalityId, | ||
| barangayId: input.barangayId, | ||
| nearestLandmark: input.nearestLandmark, | ||
| } |
Contributor
There was a problem hiding this comment.
Omit blank optional location fields before writing the inbox payload.
If barangayId comes through as '', this block still serializes it and the payload will fail inboxPayloadSchema’s min(1) check. Normalize/trim these optional fields at the boundary instead of trusting the caller.
Proposed fix
- ...(input.municipalityId
+ ...(input.municipalityId?.trim()
? {
- municipalityId: input.municipalityId,
- barangayId: input.barangayId,
- nearestLandmark: input.nearestLandmark,
+ municipalityId: input.municipalityId.trim(),
+ ...(input.barangayId?.trim() ? { barangayId: input.barangayId.trim() } : {}),
+ ...(input.nearestLandmark?.trim()
+ ? { nearestLandmark: input.nearestLandmark.trim() }
+ : {}),
}
: {}),As per coding guidelines: "Use defensive programming: validate external input at boundaries and never swallow errors with empty catch blocks".
📝 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
| ...(input.municipalityId | |
| ? { | |
| municipalityId: input.municipalityId, | |
| barangayId: input.barangayId, | |
| nearestLandmark: input.nearestLandmark, | |
| } | |
| ...(input.municipalityId?.trim() | |
| ? { | |
| municipalityId: input.municipalityId.trim(), | |
| ...(input.barangayId?.trim() ? { barangayId: input.barangayId.trim() } : {}), | |
| ...(input.nearestLandmark?.trim() | |
| ? { nearestLandmark: input.nearestLandmark.trim() } | |
| : {}), | |
| } | |
| : {}), |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/citizen-pwa/src/services/submit-report.ts` around lines 74 - 79, When
building the inbox payload in submit-report.ts, defensive-normalize the optional
location fields on input (municipalityId, barangayId, nearestLandmark) by
trimming them and only including non-empty strings in the spread so you don't
serialize '' and fail inboxPayloadSchema's min(1) check; update the payload
construction that currently spreads ...(input.municipalityId ? { municipalityId:
input.municipalityId, barangayId: input.barangayId, nearestLandmark:
input.nearestLandmark } : {}) to trim and conditionally include barangayId and
nearestLandmark (e.g., compute trimmed values from
input.barangayId/input.nearestLandmark and only add those keys when
trimmed.length>0) so only valid non-empty values reach inboxPayload validation.
queryFn async type inferred as Promise<{} | null> instead of
Promise<ReportData | null> due to getQueryData path returning {}.
Explicit annotation anchors TypeScript inference.
Also replace Zap with AlertTriangle (already imported) in Step3Review
fallback icon path.
Avoid automated semicolon insertion (92% of all statements in the enclosing function have an explicit semicolon) Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
…ort, function or class' Unused imports deleteDraft, listDrafts. Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (5)
apps/citizen-pwa/src/components/SubmitReportForm/Step3Review.tsx (2)
17-27:⚠️ Potential issue | 🟠 MajorCarry
barangayLabelthroughreportData.Line 107 still renders
barangayId, so the review step shows an internal key instead of the barangay name the user selected. Add an optionalbarangayLabelto this contract, populate it from Step 2, and render that here while keepingbarangayIdfor submission/internal use.Also applies to: 103-108
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/citizen-pwa/src/components/SubmitReportForm/Step3Review.tsx` around lines 17 - 27, The review step is showing barangayId instead of the user-friendly name because reportData lacks barangayLabel; update the reportData contract (the reportData type/object in Step3Review/props) to include an optional barangayLabel?: string, ensure Step 2 populates reportData.barangayLabel when the user selects a barangay, and change the renderer in Step3Review (where it currently displays barangayId) to render reportData.barangayLabel || reportData.barangayId so the label is shown but the id remains available for submission/internal use.
124-135:⚠️ Potential issue | 🟠 MajorMake “Privacy notice” an actual interactive element.
The underlined text is still just part of the checkbox label, so it is not focusable or announced as interactive, and clicking it only toggles consent. Replace it with a real button or link that opens the notice independently of the checkbox.
As per coding guidelines "Major/high-impact issues: Compliance/privacy risks" and "Accessibility blockers that prevent task completion".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/citizen-pwa/src/components/SubmitReportForm/Step3Review.tsx` around lines 124 - 135, The "Privacy notice ›" text inside the consent label is currently non-interactive and toggles the checkbox; update the JSX in Step3Review.tsx so that the underlined text is a real interactive element (e.g., a <button> or <a> with an onClick that opens the privacy modal or navigates to the notice) instead of plain text inside the span; ensure the interactive element has accessible attributes (aria-label or descriptive text), is focusable, and calls e.stopPropagation() (or otherwise prevents the label's onChange from firing) so clicking the link does not toggle consent; keep the checkbox input, its checked prop (consent) and setter (setConsent) intact and maintain className hooks like consent-checkbox-label and consent-text-small for styling.apps/citizen-pwa/src/lib/mappers.ts (1)
30-36:⚠️ Potential issue | 🟠 MajorGuard
locationbefore treating it as an object.
data.location !== undefinedstill allowsnull, arrays, and primitives through. The subsequent castas Record<string, unknown>followed by property reads will fail or accept invalid shapes.🛡️ Proposed fix to add type guard
- if (data.location !== undefined) { - const loc = data.location as Record<string, unknown>; + if ( + data.location !== undefined && + data.location !== null && + typeof data.location === 'object' && + !Array.isArray(data.location) + ) { + const loc = data.location as Record<string, unknown>;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/citizen-pwa/src/lib/mappers.ts` around lines 30 - 36, The code treats data.location as an object without guarding against null/arrays/primitives; update the check before casting so you only proceed when data.location is a non-null plain object (e.g., typeof data.location === 'object' && data.location !== null && !Array.isArray(data.location')), then cast to Record<string, unknown> and keep the existing property checks when building result.location; reference the variables data.location and result.location in your change and ensure the guard replaces the current if (data.location !== undefined) condition.apps/citizen-pwa/src/hooks/useReport.ts (1)
81-97:⚠️ Potential issue | 🟠 MajorAdd error handling to
queryFn'sonSnapshotand log mapper failures.The
onSnapshotcall lacks an error callback—if Firestore fails (e.g., permission denied, network timeout), the promise hangs indefinitely. Additionally, line 90'scatchswallows the error without logging, inconsistent with theuseEffecthandler (line 56).🛡️ Proposed fix
return new Promise<ReportData | null>((resolve) => { - const unsub = onSnapshot(doc(db(), `reports/${reportRef}`), (snap) => { - if (!snap.exists()) { - resolve(null) - unsub() - return - } - try { - resolve(mapReportFromFirestore(snap.data())) - } catch { - resolve(null) - } - setTimeout(() => { - unsub() - }, 10) - }) + const unsub = onSnapshot( + doc(db(), `reports/${reportRef}`), + (snap) => { + if (!snap.exists()) { + resolve(null) + unsub() + return + } + try { + resolve(mapReportFromFirestore(snap.data())) + } catch (err) { + console.error('Report mapping error:', err instanceof Error ? err.message : err) + resolve(null) + } + setTimeout(() => unsub(), 10) + }, + (error) => { + console.error('Report fetch error:', error.message) + resolve(null) + unsub() + } + ) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/citizen-pwa/src/hooks/useReport.ts` around lines 81 - 97, The onSnapshot call inside the Promise returned by queryFn in useReport is missing an error callback and the catch around mapReportFromFirestore silently swallows errors; add the onSnapshot error handler to catch Firestore errors (permission/network) and resolve(null) or reject appropriately while calling unsub(), and update the catch block around mapReportFromFirestore to log the thrown error (e.g., via console.error or your logger) before resolving null; ensure every error path calls unsub() so the listener is cleaned up (reference onSnapshot, mapReportFromFirestore, and the Promise/queryFn in useReport).apps/citizen-pwa/src/lib/draftManager.ts (1)
44-53: 🧹 Nitpick | 🔵 Trivial
promoteDraftToSuccessstill uses semantically ambiguous'draft'state after success.This repeats a previously raised concern: successful submission is persisted as
state: 'draft', which can mislead downstream consumers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/citizen-pwa/src/lib/draftManager.ts` around lines 44 - 53, promoteDraftToSuccess currently persists a successful submission with state: 'draft', which is misleading; update the function (promoteDraftToSuccess) to set a clear success state (e.g., state: 'submitted' or 'success') when saving the draft and keep submittedRef populated, and ensure you still strip lastError as done via the { lastError: _ignored, ...rest } destructure so downstream consumers see the correct non-draft state.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/citizen-pwa/src/hooks/useReport.ts`:
- Around line 93-95: The 10ms setTimeout around unsub() is non-obvious—either
remove the timeout and call unsub() directly after the snapshot resolves, or
keep it but add a clear comment explaining the race-condition workaround and why
10ms is required; update the code at the setTimeout(() => { unsub() }, 10) site
in useReport.ts (the unsub variable from the onSnapshot/unsubscribe flow) to
reflect your decision and ensure the snapshot callback is completed before
unsubscribing if you keep the delayed unsubscribe.
In `@apps/citizen-pwa/src/lib/draftManager.ts`:
- Around line 15-20: In updateDraft, merging updates into the draft currently
allows callers to change the draft's uuid and thus the storage key; prevent this
by ensuring the saved object preserves the original identity: when building
updated before calling saveDraft, either delete any incoming updates.uuid or
explicitly set updated.uuid = draft.uuid, and optionally validate that if
updates.uuid is provided it matches the function parameter and throw an error;
update the updateDraft function (and related callers if necessary) to use
getDraft, enforce the uuid invariant, then call saveDraft with the
preserved-uuid object.
- Around line 3-10: createDraft currently accepts reportData that may include
lifecycle-managed fields and thus can be used to inject internal state; change
the parameter type to explicitly omit internal fields (e.g.
Omit<DraftReport,'uuid'|'createdAt'|'state'|'submittedRef'|'lastError'|'retryCount'>)
and defensively strip/ignore any of those keys if present before constructing
the DraftReport in createDraft; ensure the function sets uuid, createdAt, state
and other internal fields itself and add a runtime check that rejects or removes
unexpected lifecycle fields from incoming reportData to enforce the boundary.
In `@apps/citizen-pwa/src/lib/mappers.ts`:
- Around line 9-13: The mapper currently casts data.timeline into
ReportData['timeline'] without validation, which can produce undefined/invalid
timelines and crash callers; update the mapper in
apps/citizen-pwa/src/lib/mappers.ts to validate that data.timeline exists and is
an array before constructing the ReportData object (throw or return a clear
error/nullable result if it is missing or not an array), and assign a properly
typed timeline value to the result used by ReportData (refer to ReportData,
ReportStatus, id, status, and timeline in the mapper to locate the change).
---
Duplicate comments:
In `@apps/citizen-pwa/src/components/SubmitReportForm/Step3Review.tsx`:
- Around line 17-27: The review step is showing barangayId instead of the
user-friendly name because reportData lacks barangayLabel; update the reportData
contract (the reportData type/object in Step3Review/props) to include an
optional barangayLabel?: string, ensure Step 2 populates
reportData.barangayLabel when the user selects a barangay, and change the
renderer in Step3Review (where it currently displays barangayId) to render
reportData.barangayLabel || reportData.barangayId so the label is shown but the
id remains available for submission/internal use.
- Around line 124-135: The "Privacy notice ›" text inside the consent label is
currently non-interactive and toggles the checkbox; update the JSX in
Step3Review.tsx so that the underlined text is a real interactive element (e.g.,
a <button> or <a> with an onClick that opens the privacy modal or navigates to
the notice) instead of plain text inside the span; ensure the interactive
element has accessible attributes (aria-label or descriptive text), is
focusable, and calls e.stopPropagation() (or otherwise prevents the label's
onChange from firing) so clicking the link does not toggle consent; keep the
checkbox input, its checked prop (consent) and setter (setConsent) intact and
maintain className hooks like consent-checkbox-label and consent-text-small for
styling.
In `@apps/citizen-pwa/src/hooks/useReport.ts`:
- Around line 81-97: The onSnapshot call inside the Promise returned by queryFn
in useReport is missing an error callback and the catch around
mapReportFromFirestore silently swallows errors; add the onSnapshot error
handler to catch Firestore errors (permission/network) and resolve(null) or
reject appropriately while calling unsub(), and update the catch block around
mapReportFromFirestore to log the thrown error (e.g., via console.error or your
logger) before resolving null; ensure every error path calls unsub() so the
listener is cleaned up (reference onSnapshot, mapReportFromFirestore, and the
Promise/queryFn in useReport).
In `@apps/citizen-pwa/src/lib/draftManager.ts`:
- Around line 44-53: promoteDraftToSuccess currently persists a successful
submission with state: 'draft', which is misleading; update the function
(promoteDraftToSuccess) to set a clear success state (e.g., state: 'submitted'
or 'success') when saving the draft and keep submittedRef populated, and ensure
you still strip lastError as done via the { lastError: _ignored, ...rest }
destructure so downstream consumers see the correct non-draft state.
In `@apps/citizen-pwa/src/lib/mappers.ts`:
- Around line 30-36: The code treats data.location as an object without guarding
against null/arrays/primitives; update the check before casting so you only
proceed when data.location is a non-null plain object (e.g., typeof
data.location === 'object' && data.location !== null &&
!Array.isArray(data.location')), then cast to Record<string, unknown> and keep
the existing property checks when building result.location; reference the
variables data.location and result.location in your change and ensure the guard
replaces the current if (data.location !== undefined) condition.
🪄 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: 1ba60be7-c9a6-457b-8a34-65ee86211d9a
📒 Files selected for processing (4)
apps/citizen-pwa/src/components/SubmitReportForm/Step3Review.tsxapps/citizen-pwa/src/hooks/useReport.tsapps/citizen-pwa/src/lib/draftManager.tsapps/citizen-pwa/src/lib/mappers.ts
useReport.ts:
- Remove 10ms setTimeout around unsub(); call unsub() directly
after resolve() since unsubscribe does not interrupt in-flight callbacks
- Add onSnapshot error callback as 3rd arg; without it, Firestore
transport errors cause the queryFn Promise to hang indefinitely
- Log mapping errors in catch block (was silently swallowing)
mappers.ts:
- Add !Array.isArray(data.timeline) guard; undefined timeline crashes
downstream report.timeline.map() in TrackingScreen
- Add typeof/object/null/array guard for data.location; null location
was producing silent empty result.location = {} instead of throwing
draftManager.ts:
- Narrow createDraft input to CreateDraftInput (omits lifecycle fields:
createdAt, state, submittedRef, lastError, retryCount); only uuid is
generated internally
- Guard updateDraft uuid with destructuring; spread {...draft,...updates}
allowed callers to change record identity and orphan the storage key
Step3Review.tsx:
- Replace <u>Privacy notice</u> in consent label with real <button>;
underlined text was non-focusable and toggled the checkbox on click
…s conflict keeping timeline guard
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (2)
apps/citizen-pwa/src/lib/draftManager.ts (2)
50-59:⚠️ Potential issue | 🟠 Major
promoteDraftToSuccessstill conflates submitted and editable draft states.At Line 56, successful submissions are saved with
state: 'draft'. This remains semantically ambiguous and can blur downstream handling of completed vs in-progress records.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/citizen-pwa/src/lib/draftManager.ts` around lines 50 - 59, promoteDraftToSuccess currently re-saves a successfully submitted draft with state: 'draft', conflating editable vs completed records; update the function (promoteDraftToSuccess) to set a clear completed state (e.g., 'submitted' or 'completed') when calling saveDraft and keep submittedRef assignment and the lastError removal logic ({ lastError: _ignored, ...rest }) intact so downstream consumers can distinguish submitted records from in-progress drafts.
8-17:⚠️ Potential issue | 🟠 MajorRuntime boundary still allows lifecycle-field injection in
createDraft.
CreateDraftInputprotects compile-time only. At Line 12, spreadingreportDatameans JS/casted callers can still inject fields likesubmittedRef,lastError, orretryCountinto persisted drafts.♻️ Suggested hardening
export async function createDraft(reportData: CreateDraftInput): Promise<string> { const uuid = crypto.randomUUID() + const { + submittedRef: _ignoredSubmittedRef, + lastError: _ignoredLastError, + retryCount: _ignoredRetryCount, + uuid: _ignoredUuid, + createdAt: _ignoredCreatedAt, + state: _ignoredState, + ...safeReportData + } = reportData as Partial<DraftReport> const draft: DraftReport = { uuid, - ...reportData, + ...safeReportData, createdAt: Date.now(), state: 'queued', } await saveDraft(draft) return uuid }As per coding guidelines, "Defensive Programming: Assume external input is malicious/broken. Validate at the boundary."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/citizen-pwa/src/lib/draftManager.ts` around lines 8 - 17, createDraft currently spreads reportData into the persisted DraftReport which allows runtime injection of lifecycle fields; update createDraft to whitelist only the allowed user-provided fields from CreateDraftInput (e.g., title, description, location, attachments, etc.) instead of using the spread, explicitly construct the DraftReport object with generated uuid, createdAt, state and the whitelisted fields, and ensure fields like submittedRef, lastError, retryCount, state, createdAt, and uuid are not taken from reportData before calling saveDraft.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/citizen-pwa/src/components/SubmitReportForm/Step3Review.tsx`:
- Around line 135-143: The consent-link button in the Step3Review component
currently only calls e.stopPropagation() and provides no action; update the
component to actually open the privacy notice by wiring the button to a handler:
either call a passed-in prop like onOpenPrivacy() (add an optional prop to
Step3Review and invoke it in the button onClick after e.stopPropagation()), or
toggle local state (e.g., setShowPrivacyModal(true)) to render a
PrivacyNoticeModal component, or use the app router (e.g., navigate to
"/privacy-policy")—ensure the handler replaces the no-op so clicking the button
opens the modal or navigates accordingly and preserves the e.stopPropagation()
behavior.
- Around line 31-38: INCIDENT_TYPES in Step3Review.tsx is missing four
ReportType values so unmapped reportData.reportType falls back to the raw
string; fix by either (A) adding entries for 'medical', 'accident',
'structural', and 'security' to INCIDENT_TYPES with appropriate label and Icon
names (match the existing { value, label, Icon } shape) or (B) change the
display logic where reportData.reportType is rendered to use a friendly fallback
(lookup INCIDENT_TYPES by value and, if missing, transform the value by
replacing underscores with spaces and title-casing words) so the UI never shows
the raw machine string.
In `@apps/citizen-pwa/src/hooks/useReport.ts`:
- Around line 81-104: The queryFn in useReport.ts currently uses onSnapshot
(creating a persistent listener and unsub pattern) and swallows errors by
resolving null; replace that with a one-shot getDoc call: call getDoc(doc(db(),
`reports/${reportRef}`)), check snap.exists() and return
mapReportFromFirestore(snap.data()) or null for a missing doc, and let getDoc
errors propagate (do not catch and resolve null) so React Query can handle
failures; remove the unsubscribe logic and any onSnapshot usage in the queryFn
to keep it side-effect-free.
In `@apps/citizen-pwa/src/lib/mappers.ts`:
- Around line 4-12: In mapReportFromFirestore, validate that data.status is a
known ReportStatus and that each item in data.timeline is an object containing
the required fields (event, timestamp, actor) of the expected types before
casting to ReportData; implement a status type-guard (or compare against the
allowed ReportStatus values) and iterate over data.timeline to validate/map each
entry into a properly typed timeline item (throw a descriptive Error if any
entry is missing/invalid) so consumers of ReportData cannot receive malformed
records.
- Around line 36-41: The current mapping copies location leaf fields without
runtime type validation (loc.address, loc.lat, loc.lng), so inputs like {
location: { lat: '14.1' } } violate the ReportLocation contract; update the
mapping logic in this mapper where you build result.location to validate each
leaf: ensure address is a string (typeof loc.address === 'string') before
assigning, ensure lat and lng are numbers (typeof loc.lat === 'number' &&
Number.isFinite(loc.lat) and similarly for loc.lng) or defensively parse numeric
strings with parseFloat and verify Number.isFinite before assigning; only
include the field when the runtime check passes so result.location conforms to
the types expected by useReport.ts.
In `@apps/citizen-pwa/src/styles/globals.css`:
- Around line 29-34: The font-family declaration currently quotes 'Inter';
update the CSS declaration (the font-family property) to remove the quotes
around Inter so it reads Inter, -apple-system, system-ui, sans-serif to satisfy
stylelint and CSS conventions; ensure only the Inter token is unquoted and the
rest of the fallback list remains unchanged.
- Around line 201-212: The CSS rule for the .status-icon class uses the keyword
"currentColor" with mixed case; update the background property to use the
lowercase CSS keyword "currentcolor" to satisfy Stylelint's value-keyword-case
rule and ensure consistent styling across browsers (locate the .status-icon
selector and change background: currentColor; to background: currentcolor;).
- Around line 799-811: Rename the keyframe identifier from slideIn to kebab-case
slide-in and update all usages accordingly: change the `@keyframes` declaration
name (currently `@keyframes` slideIn) to `@keyframes` slide-in and update any
animation or animation-name properties (e.g., animation: slideIn 0.2s ease;) to
use slide-in so the keyframes-name-pattern lint rule is satisfied.
---
Duplicate comments:
In `@apps/citizen-pwa/src/lib/draftManager.ts`:
- Around line 50-59: promoteDraftToSuccess currently re-saves a successfully
submitted draft with state: 'draft', conflating editable vs completed records;
update the function (promoteDraftToSuccess) to set a clear completed state
(e.g., 'submitted' or 'completed') when calling saveDraft and keep submittedRef
assignment and the lastError removal logic ({ lastError: _ignored, ...rest })
intact so downstream consumers can distinguish submitted records from
in-progress drafts.
- Around line 8-17: createDraft currently spreads reportData into the persisted
DraftReport which allows runtime injection of lifecycle fields; update
createDraft to whitelist only the allowed user-provided fields from
CreateDraftInput (e.g., title, description, location, attachments, etc.) instead
of using the spread, explicitly construct the DraftReport object with generated
uuid, createdAt, state and the whitelisted fields, and ensure fields like
submittedRef, lastError, retryCount, state, createdAt, and uuid are not taken
from reportData before calling saveDraft.
🪄 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: 0b584e25-cf15-487f-bf89-76ad62e8f3c4
📒 Files selected for processing (5)
apps/citizen-pwa/src/components/SubmitReportForm/Step3Review.tsxapps/citizen-pwa/src/hooks/useReport.tsapps/citizen-pwa/src/lib/draftManager.tsapps/citizen-pwa/src/lib/mappers.tsapps/citizen-pwa/src/styles/globals.css
Comment on lines
+31
to
+38
| const INCIDENT_TYPES = [ | ||
| { value: 'flood', label: 'Flood', Icon: Droplets }, | ||
| { value: 'fire', label: 'Fire', Icon: Flame }, | ||
| { value: 'earthquake', label: 'Earthquake', Icon: AlertTriangle }, | ||
| { value: 'typhoon', label: 'Typhoon', Icon: Wind }, | ||
| { value: 'landslide', label: 'Landslide', Icon: Mountain }, | ||
| { value: 'storm_surge', label: 'Storm Surge', Icon: Waves }, | ||
| ] as const |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Expand INCIDENT_TYPES to cover all report types or format the fallback label.
INCIDENT_TYPES covers 6 of the 10 possible ReportType values (medical, accident, structural, security are missing). When unmapped, line 88 displays the raw reportData.reportType (e.g., "storm_surge") which is not user-friendly.
🔧 Option 1: Add missing types
const INCIDENT_TYPES = [
{ value: 'flood', label: 'Flood', Icon: Droplets },
{ value: 'fire', label: 'Fire', Icon: Flame },
{ value: 'earthquake', label: 'Earthquake', Icon: AlertTriangle },
{ value: 'typhoon', label: 'Typhoon', Icon: Wind },
{ value: 'landslide', label: 'Landslide', Icon: Mountain },
{ value: 'storm_surge', label: 'Storm Surge', Icon: Waves },
+ { value: 'medical', label: 'Medical', Icon: Heart },
+ { value: 'accident', label: 'Accident', Icon: AlertTriangle },
+ { value: 'structural', label: 'Structural', Icon: AlertTriangle },
+ { value: 'security', label: 'Security', Icon: AlertTriangle },
] as const🔧 Option 2: Format the fallback label
- {incident?.label ?? reportData.reportType}
+ {incident?.label ?? reportData.reportType.replace(/_/g, ' ').replace(/\b\w/g, c => c.toUpperCase())}Also applies to: 88-88
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/citizen-pwa/src/components/SubmitReportForm/Step3Review.tsx` around
lines 31 - 38, INCIDENT_TYPES in Step3Review.tsx is missing four ReportType
values so unmapped reportData.reportType falls back to the raw string; fix by
either (A) adding entries for 'medical', 'accident', 'structural', and
'security' to INCIDENT_TYPES with appropriate label and Icon names (match the
existing { value, label, Icon } shape) or (B) change the display logic where
reportData.reportType is rendered to use a friendly fallback (lookup
INCIDENT_TYPES by value and, if missing, transform the value by replacing
underscores with spaces and title-casing words) so the UI never shows the raw
machine string.
Comment on lines
+135
to
+143
| <button | ||
| type="button" | ||
| className="consent-link" | ||
| onClick={(e) => { | ||
| e.stopPropagation() | ||
| }} | ||
| > | ||
| Privacy notice › | ||
| </button> |
Contributor
There was a problem hiding this comment.
Privacy notice button has no action.
The button was correctly added for accessibility, but its onClick only calls e.stopPropagation() without any actual functionality—clicking it does nothing. Users expect this to open a privacy notice modal or navigate to a privacy policy page.
Would you like me to help implement a privacy notice modal or suggest a navigation handler?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/citizen-pwa/src/components/SubmitReportForm/Step3Review.tsx` around
lines 135 - 143, The consent-link button in the Step3Review component currently
only calls e.stopPropagation() and provides no action; update the component to
actually open the privacy notice by wiring the button to a handler: either call
a passed-in prop like onOpenPrivacy() (add an optional prop to Step3Review and
invoke it in the button onClick after e.stopPropagation()), or toggle local
state (e.g., setShowPrivacyModal(true)) to render a PrivacyNoticeModal
component, or use the app router (e.g., navigate to "/privacy-policy")—ensure
the handler replaces the no-op so clicking the button opens the modal or
navigates accordingly and preserves the e.stopPropagation() behavior.
Comment on lines
+81
to
+104
| return new Promise<ReportData | null>((resolve) => { | ||
| const unsub = onSnapshot( | ||
| doc(db(), `reports/${reportRef}`), | ||
| (snap) => { | ||
| if (!snap.exists()) { | ||
| resolve(null) | ||
| unsub() | ||
| return | ||
| } | ||
| try { | ||
| resolve(mapReportFromFirestore(snap.data())) | ||
| } catch (err) { | ||
| console.error('Report mapping error:', err instanceof Error ? err.message : err) | ||
| resolve(null) | ||
| } | ||
| unsub() | ||
| }, | ||
| (error) => { | ||
| console.error('Report fetch error:', error.message) | ||
| resolve(null) | ||
| unsub() | ||
| }, | ||
| ) | ||
| }) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "useReport.ts" | head -5Repository: Exc1D/bantayog-alert
Length of output: 105
🏁 Script executed:
find . -type f -name "TrackingScreen.tsx" | head -5Repository: Exc1D/bantayog-alert
Length of output: 116
🏁 Script executed:
cat -n ./apps/citizen-pwa/src/hooks/useReport.tsRepository: Exc1D/bantayog-alert
Length of output: 4108
🏁 Script executed:
cat -n ./apps/citizen-pwa/src/components/TrackingScreen.tsx | head -60Repository: Exc1D/bantayog-alert
Length of output: 2119
🏁 Script executed:
rg -A 5 "getDoc\|getDocs" ./apps/citizen-pwa/src/hooks --type tsRepository: Exc1D/bantayog-alert
Length of output: 46
🏁 Script executed:
rg "import.*getDoc" ./apps/citizen-pwa/src --type tsRepository: Exc1D/bantayog-alert
Length of output: 46
🏁 Script executed:
rg "Firebase|firestore" ./apps/citizen-pwa/src/services/firebase.ts | head -20Repository: Exc1D/bantayog-alert
Length of output: 713
🏁 Script executed:
cat -n ./apps/citizen-pwa/src/services/firebase.tsRepository: Exc1D/bantayog-alert
Length of output: 2097
🏁 Script executed:
rg "useQuery" ./apps/citizen-pwa/src/hooks/useReport.ts -A 40 | grep -E "(enabled|retry|throw|reject)"Repository: Exc1D/bantayog-alert
Length of output: 108
🏁 Script executed:
rg "error" ./apps/citizen-pwa/src/components/TrackingScreen.tsx | head -10Repository: Exc1D/bantayog-alert
Length of output: 161
Replace the queryFn snapshot listener with getDoc for a one-shot read.
The useEffect above already maintains a long-lived onSnapshot subscription that populates the query cache. The queryFn creates a second listener on the same document, causing brief duplication during initial load. Additionally, the queryFn masks both Firestore errors and mapping errors by resolving null instead of rejecting—making operational failures indistinguishable from missing reports in the UI. The error check in TrackingScreen.tsx:38 is unreachable code since this queryFn never rejects.
Use getDoc for a one-shot fetch instead: it keeps the query function side-effect-free, eliminates listener duplication, and allows real fetch errors to propagate so React Query can distinguish failures from missing data.
Coding guideline violations:
- Side effects in query functions:
onSnapshotwith unsubscribe pattern - Error swallowing: catch blocks at lines 92–94 and 98–100 resolve
nullinstead of throwing
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/citizen-pwa/src/hooks/useReport.ts` around lines 81 - 104, The queryFn
in useReport.ts currently uses onSnapshot (creating a persistent listener and
unsub pattern) and swallows errors by resolving null; replace that with a
one-shot getDoc call: call getDoc(doc(db(), `reports/${reportRef}`)), check
snap.exists() and return mapReportFromFirestore(snap.data()) or null for a
missing doc, and let getDoc errors propagate (do not catch and resolve null) so
React Query can handle failures; remove the unsubscribe logic and any onSnapshot
usage in the queryFn to keep it side-effect-free.
Comment on lines
+4
to
+12
| export function mapReportFromFirestore(data: Record<string, unknown>): ReportData { | ||
| if (!data.id || !data.status || !Array.isArray(data.timeline)) { | ||
| throw new Error('Invalid report data: missing required fields') | ||
| } | ||
|
|
||
| const result: ReportData = { | ||
| id: data.id as string, | ||
| status: data.status as ReportStatus, | ||
| timeline: data.timeline as ReportData['timeline'], |
Contributor
There was a problem hiding this comment.
Validate status and timeline entries before returning ReportData.
Array.isArray(data.timeline) only proves the container shape. data.status as ReportStatus still accepts any truthy string, and data.timeline as ReportData['timeline'] still accepts arrays of arbitrary values. Because this is the Firestore boundary, a malformed document can still violate the ReportData contract and break consumers that expect event, timestamp, and actor on every entry.
As per coding guidelines: "Defensive Programming: Assume external input is malicious/broken. Validate at the boundary. Never swallow errors with an empty catch block."
🧰 Tools
🪛 GitHub Check: CodeQL
[notice] 6-6: Semicolon insertion
Avoid automated semicolon insertion (92% of all statements in the enclosing function have an explicit semicolon).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/citizen-pwa/src/lib/mappers.ts` around lines 4 - 12, In
mapReportFromFirestore, validate that data.status is a known ReportStatus and
that each item in data.timeline is an object containing the required fields
(event, timestamp, actor) of the expected types before casting to ReportData;
implement a status type-guard (or compare against the allowed ReportStatus
values) and iterate over data.timeline to validate/map each entry into a
properly typed timeline item (throw a descriptive Error if any entry is
missing/invalid) so consumers of ReportData cannot receive malformed records.
Comment on lines
+36
to
+41
| const loc = data.location as Record<string, unknown>; | ||
| result.location = { | ||
| ...(loc.address !== undefined && { address: loc.address as string }), | ||
| ...(loc.lat !== undefined && { lat: loc.lat as number }), | ||
| ...(loc.lng !== undefined && { lng: loc.lng as number }), | ||
| }; |
Contributor
There was a problem hiding this comment.
Type-check location leaf fields before copying them.
The outer object guard is good, but loc.address !== undefined, loc.lat !== undefined, and loc.lng !== undefined still accept the wrong primitive types. A document like { location: { lat: '14.1' } } will be returned as a ReportLocation, violating the contract declared in apps/citizen-pwa/src/hooks/useReport.ts.
As per coding guidelines: "Use defensive programming: validate external input at boundaries and never swallow errors with empty catch blocks"
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/citizen-pwa/src/lib/mappers.ts` around lines 36 - 41, The current
mapping copies location leaf fields without runtime type validation
(loc.address, loc.lat, loc.lng), so inputs like { location: { lat: '14.1' } }
violate the ReportLocation contract; update the mapping logic in this mapper
where you build result.location to validate each leaf: ensure address is a
string (typeof loc.address === 'string') before assigning, ensure lat and lng
are numbers (typeof loc.lat === 'number' && Number.isFinite(loc.lat) and
similarly for loc.lng) or defensively parse numeric strings with parseFloat and
verify Number.isFinite before assigning; only include the field when the runtime
check passes so result.location conforms to the types expected by useReport.ts.
Comment on lines
+29
to
+34
| font-family: | ||
| 'Inter', | ||
| -apple-system, | ||
| system-ui, | ||
| sans-serif; | ||
| font-size: 16px; |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Remove quotes around font-family name Inter.
Stylelint flags "Inter" as needing no quotes since it contains no special characters. Unquoted generic/simple font names are preferred per CSS conventions.
🔧 Proposed fix
font-family:
- 'Inter',
+ Inter,
-apple-system,
system-ui,
sans-serif;📝 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
| font-family: | |
| 'Inter', | |
| -apple-system, | |
| system-ui, | |
| sans-serif; | |
| font-size: 16px; | |
| font-family: | |
| Inter, | |
| -apple-system, | |
| system-ui, | |
| sans-serif; | |
| font-size: 16px; |
🧰 Tools
🪛 Stylelint (17.7.0)
[error] 30-30: Expected no quotes around "Inter" (font-family-name-quotes)
(font-family-name-quotes)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/citizen-pwa/src/styles/globals.css` around lines 29 - 34, The
font-family declaration currently quotes 'Inter'; update the CSS declaration
(the font-family property) to remove the quotes around Inter so it reads Inter,
-apple-system, system-ui, sans-serif to satisfy stylelint and CSS conventions;
ensure only the Inter token is unquoted and the rest of the fallback list
remains unchanged.
Comment on lines
+201
to
+212
| .status-icon { | ||
| width: 2rem; | ||
| height: 2rem; | ||
| border-radius: 9999px; | ||
| background: currentColor; | ||
| display: flex; | ||
| align-items: center; | ||
| justify-content: center; | ||
| color: white; | ||
| font-size: 0.875rem; | ||
| font-weight: 600; | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Use lowercase currentcolor for CSS keyword.
Per Stylelint's value-keyword-case rule, CSS keywords should be lowercase.
🔧 Proposed fix
.status-icon {
width: 2rem;
height: 2rem;
border-radius: 9999px;
- background: currentColor;
+ background: currentcolor;
display: flex;📝 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
| .status-icon { | |
| width: 2rem; | |
| height: 2rem; | |
| border-radius: 9999px; | |
| background: currentColor; | |
| display: flex; | |
| align-items: center; | |
| justify-content: center; | |
| color: white; | |
| font-size: 0.875rem; | |
| font-weight: 600; | |
| } | |
| .status-icon { | |
| width: 2rem; | |
| height: 2rem; | |
| border-radius: 9999px; | |
| background: currentcolor; | |
| display: flex; | |
| align-items: center; | |
| justify-content: center; | |
| color: white; | |
| font-size: 0.875rem; | |
| font-weight: 600; | |
| } |
🧰 Tools
🪛 Stylelint (17.7.0)
[error] 205-205: Expected "currentColor" to be "currentcolor" (value-keyword-case)
(value-keyword-case)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/citizen-pwa/src/styles/globals.css` around lines 201 - 212, The CSS rule
for the .status-icon class uses the keyword "currentColor" with mixed case;
update the background property to use the lowercase CSS keyword "currentcolor"
to satisfy Stylelint's value-keyword-case rule and ensure consistent styling
across browsers (locate the .status-icon selector and change background:
currentColor; to background: currentcolor;).
Comment on lines
+799
to
+811
| animation: slideIn 0.2s ease; | ||
| } | ||
|
|
||
| @keyframes slideIn { | ||
| from { | ||
| opacity: 0; | ||
| transform: translateY(-4px); | ||
| } | ||
| to { | ||
| opacity: 1; | ||
| transform: translateY(0); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Use kebab-case for keyframe name: slide-in instead of slideIn.
Stylelint's keyframes-name-pattern expects kebab-case for @keyframes names.
🔧 Proposed fix
.patient-advisory {
background: var(--color-surface-warn);
border-radius: 0.5rem;
padding: 0.625rem;
margin-top: 0.5rem;
- animation: slideIn 0.2s ease;
+ animation: slide-in 0.2s ease;
}
-@keyframes slideIn {
+@keyframes slide-in {
from {
opacity: 0;
transform: translateY(-4px);
}📝 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
| animation: slideIn 0.2s ease; | |
| } | |
| @keyframes slideIn { | |
| from { | |
| opacity: 0; | |
| transform: translateY(-4px); | |
| } | |
| to { | |
| opacity: 1; | |
| transform: translateY(0); | |
| } | |
| } | |
| animation: slide-in 0.2s ease; | |
| } | |
| `@keyframes` slide-in { | |
| from { | |
| opacity: 0; | |
| transform: translateY(-4px); | |
| } | |
| to { | |
| opacity: 1; | |
| transform: translateY(0); | |
| } | |
| } |
🧰 Tools
🪛 Stylelint (17.7.0)
[error] 802-802: Expected keyframe name "slideIn" to be kebab-case (keyframes-name-pattern)
(keyframes-name-pattern)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/citizen-pwa/src/styles/globals.css` around lines 799 - 811, Rename the
keyframe identifier from slideIn to kebab-case slide-in and update all usages
accordingly: change the `@keyframes` declaration name (currently `@keyframes`
slideIn) to `@keyframes` slide-in and update any animation or animation-name
properties (e.g., animation: slideIn 0.2s ease;) to use slide-in so the
keyframes-name-pattern lint rule is satisfied.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/shared-validators/src/reports.ts`:
- Around line 202-204: The schema currently allows barangayId or nearestLandmark
without municipalityId; update the parent report schema that defines
municipalityId, barangayId, and nearestLandmark so it enforces the invariant: if
barangayId or nearestLandmark is present then municipalityId must be non-empty.
Make nearestLandmark trim whitespace and reject empty/whitespace-only values
(e.g., .transform(v => v?.trim()).refine(v => v && v.length>0) or
.trim().min(1)). Implement the cross-field check via schema.superRefine (or
.refine with an object predicate) to throw a validation error when (barangayId
|| nearestLandmark) is provided but municipalityId is missing/empty; keep
municipalityId as optional except when required by this rule.
🪄 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: 05996671-adbc-459f-855f-21ddbc153e7c
📒 Files selected for processing (1)
packages/shared-validators/src/reports.ts
Comment on lines
+202
to
+204
| municipalityId: z.string().min(1).optional(), | ||
| barangayId: z.string().min(1).optional(), | ||
| nearestLandmark: z.string().max(200).optional(), |
Contributor
There was a problem hiding this comment.
Enforce dependent manual-location field invariants at schema boundary.
Line 202-204 currently allows barangayId and/or nearestLandmark without municipalityId, which admits inconsistent payloads. This contradicts the municipality-scoped shape used in apps/citizen-pwa/src/services/submit-report.ts (Line 74-80). Also, nearestLandmark should reject empty/whitespace-only values.
Proposed schema hardening
export const inboxPayloadSchema = z
.object({
@@
- municipalityId: z.string().min(1).optional(),
- barangayId: z.string().min(1).optional(),
- nearestLandmark: z.string().max(200).optional(),
+ municipalityId: z.string().min(1).optional(),
+ barangayId: z.string().min(1).optional(),
+ nearestLandmark: z.string().trim().min(1).max(200).optional(),
@@
})
+ .superRefine((value, ctx) => {
+ if ((value.barangayId || value.nearestLandmark) && !value.municipalityId) {
+ ctx.addIssue({
+ code: z.ZodIssueCode.custom,
+ path: ['municipalityId'],
+ message: 'municipalityId is required when barangayId or nearestLandmark is provided',
+ })
+ }
+ })
.strict()As per coding guidelines, "Use defensive programming: validate external input at boundaries and never swallow errors with empty catch blocks".
📝 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
| municipalityId: z.string().min(1).optional(), | |
| barangayId: z.string().min(1).optional(), | |
| nearestLandmark: z.string().max(200).optional(), | |
| export const inboxPayloadSchema = z | |
| .object({ | |
| municipalityId: z.string().min(1).optional(), | |
| barangayId: z.string().min(1).optional(), | |
| nearestLandmark: z.string().trim().min(1).max(200).optional(), | |
| }) | |
| .superRefine((value, ctx) => { | |
| if ((value.barangayId || value.nearestLandmark) && !value.municipalityId) { | |
| ctx.addIssue({ | |
| code: z.ZodIssueCode.custom, | |
| path: ['municipalityId'], | |
| message: 'municipalityId is required when barangayId or nearestLandmark is provided', | |
| }) | |
| } | |
| }) | |
| .strict() |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/shared-validators/src/reports.ts` around lines 202 - 204, The schema
currently allows barangayId or nearestLandmark without municipalityId; update
the parent report schema that defines municipalityId, barangayId, and
nearestLandmark so it enforces the invariant: if barangayId or nearestLandmark
is present then municipalityId must be non-empty. Make nearestLandmark trim
whitespace and reject empty/whitespace-only values (e.g., .transform(v =>
v?.trim()).refine(v => v && v.length>0) or .trim().min(1)). Implement the
cross-field check via schema.superRefine (or .refine with an object predicate)
to throw a validation error when (barangayId || nearestLandmark) is provided but
municipalityId is missing/empty; keep municipalityId as optional except when
required by this rule.
mappers.ts: add semicolon to match existing style (ts/semicolon notice) Step1Evidence.tsx: add codeql suppression for img src=previewUrl; previewUrl is a blob URL from URL.createObjectURL(file), not DOM text, and is not a real XSS vector (React img src is not an HTML injection sink)
… img addExtractorNote on previewUrl state to inform CodeQL that the blob URL from createObjectURL is safe for img src attribute (not DOM text reinterpreted as HTML)
suppresses GHAS CodeQL 'DOM text reinterpreted as HTML' alert. blob URL is a safe opaque reference, not DOM text, and cannot contain HTML that would be reinterpreted as such.
suppresses GHAS CodeQL 'DOM text reinterpreted as HTML' false positive. blob URL is a safe opaque reference not DOM text; cannot contain HTML that would be reinterpreted as such.
Exc1D
pushed a commit
that referenced
this pull request
Apr 23, 2026
Merge origin/main into feature/phase-4-sms-providers. Conflict resolutions: - package.json, pnpm-lock.yaml: take main's (adds leaflet, react-leaflet, zustand, lucide-react, @testing-library/user-event) - routes.tsx: take main's (MapTab, CitizenShell, multi-route setup) - SubmitReportForm.tsx (flat): take main's (barangay selector) - SubmitReportForm/index.tsx: keep ours (offline-enabled with submission machine) - useSubmissionMachine.ts: keep ours (Firebase-backed with setDoc, retry logic) - docs: take main's map-tab plan and spec
This was referenced Apr 23, 2026
This was referenced Apr 30, 2026
feat(citizen-pwa): all 6 screens redesign — feed, profile, alerts, map, report form, RevealSheet
#85
Merged
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
municipalityIdoverride in inbox payload allows reports from outside reverse-geocoded centroid coverageChanges
municipalityId,barangayId,nearestLandmarkoptional fields toinboxPayloadSchemaprocessInboxItemusesmunicipalityIdto lookup centroid when reverse geocode returns nullStep2WhoWhere.tsxwith GPS/manual mode, alpha-sorted municipality + barangay selectsStep3Reviewdisplays manual location info or GPS coordinatesTesting
Summary by Sourcery
Introduce a new multi-step citizen report submission flow with GPS/manual location entry, offline-aware submission handling, and a live tracking screen for submitted reports.
New Features:
Enhancements:
Documentation:
Tests:
Summary by CodeRabbit
New Features
Documentation
Tests