Merged
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
📝 WalkthroughWalkthroughThis PR enhances citizen report tracking and lookup across multiple layers: normalizing Firestore timestamps and synthesizing missing timeline data in the report mapper, adding local storage fallback for report lookups, canonicalizing legacy report types to modern values, and setting visibility classes and update timestamps during verification transitions. ESLint directives and documentation are also updated. ChangesReport Data, Timeline, & Visibility Management
LookupScreen Secret Normalization & Local Report Fallback
Report Type Canonicalization
Test & Documentation Refinements
Sequence DiagramsequenceDiagram
participant User as User
participant Browser as LookupScreen
participant LocalStorage as localForageReports
participant Firebase as Firebase
User->>Browser: Enter secret code
Browser->>Browser: normalizeSecretCode(secret)
Browser->>LocalStorage: loadReports()
LocalStorage-->>Browser: [local reports]
alt Local match found
Browser->>Browser: Find report with matching secret
Browser-->>User: Navigate to /reports/{publicRef}
else No local match
Browser->>Firebase: requestLookup(normalized secret)
Firebase-->>Browser: Server response (report or error)
Browser-->>User: Navigate to report or display error
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 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)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 0/1 reviews remaining, refill in 15 minutes and 7 seconds.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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/LookupScreen.test.tsx`:
- Around line 127-156: The test claims to verify "server lookup not ready" but
LookupScreen (local check in the component) reads local reports before calling
Firebase, so the vi.mocked(httpsCallable).mockImplementationOnce(...) is never
consumed; remove that dead mock from the test, update the test name to reflect
local-first behavior (e.g., "navigates to locally saved report when a local
match exists"), and add an explicit assertion
expect(vi.mocked(httpsCallable)).not.toHaveBeenCalled() to lock in the
local-first behavior; refer to LookupScreen component (local lookup logic) and
the httpsCallable mock and mockLoadReports in the test to locate the changes.
In `@apps/citizen-pwa/src/lib/mappers.ts`:
- Around line 37-59: mapReportFromFirestore currently accepts any string in
data.status and casts it to ReportStatus; validate data.status against the
allowed ReportStatus values before using or casting it (e.g., check membership
in the ReportStatus enum/union or a Set of valid statuses) and throw an error if
it’s not valid so invalid backend values fail fast; update the validation near
the existing typeof data.status check and ensure downstream uses (timeline
synthesis and the result.status assignment) only use the validated/cast
ReportStatus.
- Around line 56-60: The mapper currently collapses missing inline IDs into the
shared sentinel 'unknown' on the ReportData object; update the mapper that
builds "result: ReportData" (the function that consumes "data", "timeline", and
returns "result") to accept the Firestore document ID as an additional parameter
(e.g., docId) and set id = typeof data.id === 'string' ? data.id : docId; if
passing docId is not possible, replace the shared 'unknown' fallback with a
guaranteed-unique fallback (e.g., a generated UUID) so different Firestore docs
do not collide; ensure you update all call sites to provide the document ID when
available and keep ReportStatus and timeline assignment unchanged.
In `@apps/citizen-pwa/src/services/submit-report.test.ts`:
- Around line 141-158: Add a test that exercises the submitReport normalization
path: call submitReport (or the exported handler that triggers inbox writes)
with a payload where reportType is 'public_disturbance' and assert that
deps.writeInbox (the mocked inbox writer used in tests) was called with a
payload whose reportType === 'security'; reuse the same test setup/mocks used in
submit-report.test.ts (e.g., mockDraftStoreSave / deps.writeInbox) so the test
verifies the actual inbox write path rather than only createDraft's return
value.
In `@apps/citizen-pwa/src/services/submit-report.ts`:
- Around line 55-62: canonicalizeReportType currently maps the legacy
"public_disturbance" but then force-casts any other string to ReportType,
allowing typos/stale values to persist; change it to map "public_disturbance" →
"security" and then validate the incoming string against a whitelist/type-guard
(e.g., an isValidReportType function or REPORT_TYPES array) instead of using "as
ReportType"; if the value is not valid, reject it by throwing an Error (or
returning null/undefined) so callers cannot persist unsupported report types
(update callers to handle the rejection as needed).
In `@functions/src/__tests__/callables/verify-report.test.ts`:
- Around line 61-65: The test currently checks status and visibility but misses
asserting the new updatedAt write; update the assertions after calling
verifyReportCore (the value in result and the DB record fetched via
db.collection('reports').doc(reportId).get()) to confirm updatedAt was set—e.g.,
assert result.updatedAt exists/is a Timestamp (or number) and that
report.updatedAt exists and equals (or is consistent with) result.updatedAt (or
is greater than the previous timestamp), and add the same updatedAt assertion
for the other block around lines 94-99 so the test exercises the updatedAt
update path.
🪄 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: 0180345f-1145-4b5a-8733-95b64e73c478
📒 Files selected for processing (15)
apps/citizen-pwa/src/components/AlertsTab.test.tsxapps/citizen-pwa/src/components/LookupScreen.test.tsxapps/citizen-pwa/src/components/LookupScreen.tsxapps/citizen-pwa/src/components/ProfileTab.test.tsxapps/citizen-pwa/src/hooks/useOfflineQueueCount.test.tsapps/citizen-pwa/src/lib/__tests__/mappers.test.tsapps/citizen-pwa/src/lib/mappers.tsapps/citizen-pwa/src/pages/SettingsPage.test.tsxapps/citizen-pwa/src/services/callables.test.tsapps/citizen-pwa/src/services/submit-report.test.tsapps/citizen-pwa/src/services/submit-report.tsdocs/learnings.mddocs/progress.mdfunctions/src/__tests__/callables/verify-report.test.tsfunctions/src/callables/verify-report.ts
💤 Files with no reviewable changes (4)
- apps/citizen-pwa/src/pages/SettingsPage.test.tsx
- apps/citizen-pwa/src/hooks/useOfflineQueueCount.test.ts
- apps/citizen-pwa/src/services/callables.test.ts
- apps/citizen-pwa/src/components/AlertsTab.test.tsx
- LookupScreen: remove dead mock, add local-first assertion - mappers: validate ReportStatus whitelist, accept docId fallback - useReport: pass snapshot.id to mapReportFromFirestore - submit-report: whitelist validate canonicalizeReportType, throw on invalid - verify-report: return updatedAt in result, assert in tests
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
1 participant
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.
What changed
visibilityClass: public_alertablein the backend verification callable.lastStatusAtso the timeline/radar page renders from real report docs.Why
The previous wiring left verified reports stuck in the internal bucket, so the public feed never saw them after verification, and the tracking UI could not reliably render live report docs.
Validation
npx vitest run src/lib/__tests__/mappers.test.ts src/components/LookupScreen.test.tsx src/components/TrackingScreen.test.tsx src/components/ReportStatusPill.test.tsx src/hooks/useMyActiveReports.test.ts src/components/ProfileTab.test.tsx src/services/submit-report.test.tspnpm lintandpnpm typecheckinapps/citizen-pwapnpm typecheckinfunctionsfunctionsverify-report suite is still blocked by pre-existing rules-unit-testing harness issues (AdminTimestampseeding / permission-denied path).Summary by CodeRabbit
New Features
Bug Fixes