fix(citizen-pwa): resolve QA critical + important issues, fix 4 failing tests#87
Merged
fix(citizen-pwa): resolve QA critical + important issues, fix 4 failing tests#87
Conversation
…appers requestDataExport and registerCitizen called await callable() with no error handling — Firebase errors would propagate as opaque internal errors. Wrap both in try/catch and rethrow descriptive messages with the original error as cause. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…restore ReportTimelineEvent.actor was typed as required string but Firestore data may omit it, violating the TS contract at runtime. Make actor optional in the interface and map timeline events individually in mapReportFromFirestore instead of bulk-casting the array. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…act variants - handleCopySecret: show 'Copy failed' message to user instead of only console.error; use ref-based timer with cleanup on unmount instead of void t anti-pattern - Extract REVEAL_VARIANTS to module-level constant (was recreated every render as 42-line inline object) - Extract GUARDIAN_BENEFITS checklist items to shared constant (was duplicated between RevealSheet and ProfileTab inline) - Derive guardianIcon via useMemo keyed on state Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…d signOut feedback - Extract statusMeta() and severityDotColor() to incident-meta.tsx as single source of truth (was duplicated in ProfileTab) - ProfileTab: remove local statusMeta/severityDot, import from utils - useBadges: wrap in useMemo to avoid recalculating every render - handleSignOut: show 'Sign out failed. Please try again.' on error instead of silently swallowing Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
charAt(0).toUpperCase() on 'all' produces 'A'. Add early return for the 'all' filter value. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
4 route tests asserted synchronously with getByText on lazy-loaded components wrapped in Suspense. The render completes before React resolves the lazy import, so only the spinner fallback is in the DOM. Switch to findByText (async, wraps waitFor) to wait for the component to mount after Suspense resolves. All 213 tests now pass (was 209 pass + 4 fail). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Reviewer's GuideImplements safer error handling for Firebase callables, improves RevealSheet UX and performance, centralizes incident status/severity metadata, adds memoization and error feedback in ProfileTab, makes timeline mapping tolerant of missing actor fields, fixes severity/all labeling, and updates lazy-loaded route tests to await rendered content, resolving multiple QA-critical and important issues plus four failing tests. Sequence diagram for RevealSheet copy-to-clipboard feedback and timer cleanupsequenceDiagram
actor User
participant RevealSheet
participant ClipboardAPI
participant Timer as CopyTimer
User->>RevealSheet: clickCopySecretCode()
RevealSheet->>ClipboardAPI: writeText(secretCode)
alt copy_success
ClipboardAPI-->>RevealSheet: resolved
RevealSheet->>RevealSheet: setCopied(true)
RevealSheet->>RevealSheet: setCopyError(false)
RevealSheet->>CopyTimer: clearTimeout(copyTimerRef)
RevealSheet->>CopyTimer: copyTimerRef = setTimeout(resetCopied,2000)
CopyTimer-->>RevealSheet: resetCopied()
RevealSheet->>RevealSheet: setCopied(false)
else copy_failure
ClipboardAPI-->>RevealSheet: rejected
RevealSheet->>RevealSheet: setCopyError(true)
RevealSheet->>CopyTimer: clearTimeout(copyTimerRef)
RevealSheet->>CopyTimer: copyTimerRef = setTimeout(resetCopyError,3000)
CopyTimer-->>RevealSheet: resetCopyError()
RevealSheet->>RevealSheet: setCopyError(false)
end
User-->>RevealSheet: navigates_away_or_sheet_unmounts
RevealSheet->>CopyTimer: clearTimeout(copyTimerRef) on_unmount
Sequence diagram for Firebase callables with enhanced error handlingsequenceDiagram
actor User
participant UI as CitizenPWAUI
participant Callables as CallablesService
participant FNS as FirebaseFunctions
participant CloudCallable as CloudFunction
User->>UI: triggerDataExport()
UI->>Callables: requestDataExport()
Callables->>FNS: httpsCallable(fns,requestDataExport)
FNS-->>Callables: DataExportCallable
Callables->>CloudCallable: call()
alt export_success
CloudCallable-->>Callables: void
Callables-->>UI: resolve()
else export_failure
CloudCallable-->>Callables: throws Error
Callables->>Callables: constructError("Data export request failed",cause)
Callables-->>UI: throw_wrapped_Error
UI-->>User: show_export_error_message
end
User->>UI: registerCitizen()
UI->>Callables: registerCitizen()
Callables->>FNS: httpsCallable(fns,registerCitizen)
FNS-->>Callables: RegisterCitizenCallable
Callables->>CloudCallable: call()
alt registration_success
CloudCallable-->>Callables: CitizenRegistrationResult
Callables-->>UI: CitizenRegistrationResult
UI-->>User: proceed_to_registered_state
else registration_failure
CloudCallable-->>Callables: throws Error
Callables->>Callables: constructError("Citizen registration failed",cause)
Callables-->>UI: throw_wrapped_Error
UI-->>User: show_registration_error_message
end
Class diagram for updated report timeline mapping and typesclassDiagram
class ReportTimelineEvent {
+string event
+number timestamp
+string actor
+string note
}
class ReportTimelineEventOptionalActor {
+string event
+number timestamp
+string actor
+string note
}
class ReportData {
+string id
+ReportStatus status
+ReportTimelineEventOptionalActor[] timeline
}
class mapReportFromFirestore {
+ReportData mapReportFromFirestore(data)
}
class ReportStatus {
}
ReportData --> ReportTimelineEventOptionalActor : timeline
mapReportFromFirestore --> ReportData : returns
class RawTimelineEvent {
+string event
+number timestamp
+string actor
+string note
}
class FirestoreRecord {
+string id
+string status
+RawTimelineEvent[] timeline
}
mapReportFromFirestore --> FirestoreRecord : reads
FirestoreRecord --> RawTimelineEvent : timeline
class OptionalActorMappingLogic {
+ReportTimelineEventOptionalActor map(evt)
}
OptionalActorMappingLogic --> RawTimelineEvent : input
OptionalActorMappingLogic --> ReportTimelineEventOptionalActor : output
ReportTimelineEventOptionalActor <|-- ReportTimelineEvent : was_strict_required_actor
Class diagram for incident metadata utilities and ProfileTab integrationclassDiagram
class ProfileTab {
+boolean signOutError
+ProfileTab()
+handleSignOut()
}
class ReportCard {
+ReportCard(report,onTap)
}
class MyReport {
+string id
+string reportType
+string status
+string severity
}
class incidentIcon {
+icon incidentIcon(type)
}
class incidentLabel {
+string incidentLabel(type)
}
class statusMeta {
+StatusMeta statusMeta(status)
}
class severityDotColor {
+string severityDotColor(severity)
}
class StatusMeta {
+string label
+string bg
+string color
}
class SEVERITY_COLORS {
+Record~string,string~ SEVERITY_COLORS
}
class useBadges {
+BadgeDef[] useBadges(reports)
}
class BadgeDef {
+string id
+string title
+string description
+boolean earned
}
class BADGE_DEFS {
+BadgeDef[] BADGE_DEFS
}
ProfileTab --> useBadges : uses
useBadges --> BADGE_DEFS : maps_from
useBadges --> BadgeDef : returns
ReportCard --> MyReport : report
ReportCard --> incidentIcon : uses
ReportCard --> incidentLabel : uses
ReportCard --> statusMeta : uses
ReportCard --> severityDotColor : uses
statusMeta --> StatusMeta : returns
severityDotColor --> SEVERITY_COLORS : reads
class FirebaseAuthService {
+signOut(auth)
}
ProfileTab --> FirebaseAuthService : calls_signOut
Class diagram for Firebase callables with error handlingclassDiagram
class CallablesService {
+requestDataExport()
+registerCitizen()
}
class FirebaseFunctions {
+fns()
+httpsCallable(functions,name)
}
class DataExportCallable {
+call()
}
class RegisterCitizenCallable {
+call()
}
class CitizenRegistrationResult {
+string uid
+string role
+string accountStatus
}
class Error {
+string message
+any cause
}
CallablesService --> FirebaseFunctions : uses_httpsCallable
CallablesService --> DataExportCallable : creates
CallablesService --> RegisterCitizenCallable : creates
DataExportCallable --> Error : may_throw
RegisterCitizenCallable --> Error : may_throw
CallablesService --> CitizenRegistrationResult : returns_from_registerCitizen
class requestDataExport {
+void requestDataExport()
}
class registerCitizen {
+CitizenRegistrationResult registerCitizen()
}
requestDataExport --> CallablesService : implemented_by
registerCitizen --> CallablesService : implemented_by
requestDataExport --> Error : wraps_and_rethrows
registerCitizen --> Error : wraps_and_rethrows
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
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 (6)
📝 WalkthroughWalkthroughThis pull request updates tests to use asynchronous assertions, extracts shared incident metadata utilities, refactors UI components to use those utilities with improved memoization and error handling, adjusts type definitions to make timeline actor optional, updates data mapping logic, and enhances error handling in service callables. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 29 minutes and 22 seconds.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 1 issue, and left some high level feedback:
- In
mapReportFromFirestore,data.timelineis assumed to be a non-empty array; consider safely handlingundefined/non-array values (e.g., defaulting to an empty array or validating the shape) to avoid runtime errors when the field is missing or malformed. - In
RevealSheet,signOutErrorandcopyErrorare set but never explicitly cleared on user re-try actions; you might want to reset these flags when the user re-attempts sign-out or copy to avoid stale error messages. - For the clipboard failure path in
RevealSheet, you now show a user-facing error but fully swallow the exception; preserving aconsole.error(or similar logging) in thecatchblock would help with diagnosing production issues without affecting UX.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `mapReportFromFirestore`, `data.timeline` is assumed to be a non-empty array; consider safely handling `undefined`/non-array values (e.g., defaulting to an empty array or validating the shape) to avoid runtime errors when the field is missing or malformed.
- In `RevealSheet`, `signOutError` and `copyError` are set but never explicitly cleared on user re-try actions; you might want to reset these flags when the user re-attempts sign-out or copy to avoid stale error messages.
- For the clipboard failure path in `RevealSheet`, you now show a user-facing error but fully swallow the exception; preserving a `console.error` (or similar logging) in the `catch` block would help with diagnosing production issues without affecting UX.
## Individual Comments
### Comment 1
<location path="apps/citizen-pwa/src/components/ProfileTab.tsx" line_range="289-296" />
<code_context>
/* eslint-enable react-hooks/set-state-in-effect */
}, [reports])
+ const [signOutError, setSignOutError] = useState(false)
+
const handleSignOut = async () => {
try {
await signOut(auth())
void navigate('/', { replace: true })
} catch {
- // sign out failure is non-critical; page will still function
+ setSignOutError(true)
}
}
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Sign-out error state is never cleared, so the error message can persist unnecessarily.
Once `signOutError` is set to `true`, it never resets, so the error message can persist across subsequent attempts. Consider clearing it at the start of each sign-out attempt:
```ts
const handleSignOut = async () => {
setSignOutError(false)
try {
await signOut(auth())
void navigate('/', { replace: true })
} catch {
setSignOutError(true)
}
}
```
This keeps the error state in sync with the latest attempt.
Suggested implementation:
```typescript
const handleSignOut = async () => {
setSignOutError(false)
try {
await signOut(auth())
void navigate('/', { replace: true })
} catch {
setSignOutError(true)
}
}
```
If `handleSignOut` appears multiple times (e.g., in tests or other components within the same file), apply the same pattern: clear `signOutError` (or the equivalent local error state) at the beginning of each sign-out attempt.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![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/ProfileTab.tsx`:
- Around line 600-602: The Sign-out error message rendered in ProfileTab.tsx
using the signOutError variable should be made announcement-friendly for screen
readers: update the error <p> element that displays when signOutError is truthy
to include live-region semantics (for example add role="alert" or
aria-live="assertive") so assistive tech will announce the failure; locate the
conditional that renders the <p> with "Sign out failed. Please try again." and
add the attribute to that element.
In `@apps/citizen-pwa/src/components/RevealSheet.tsx`:
- Around line 160-165: The catch block for the clipboard write currently sets
error state but doesn't reset the success flag, which can leave copied=true;
update the catch in RevealSheet.tsx to explicitly clear the success state by
calling setCopied(false) (before or alongside setCopyError(true)), keep the
existing timer logic using copyTimerRef and clearTimeout, and ensure any
existing timers are reset so the error UI is cleared after the same 3000ms
window.
- Line 106: Rename the boolean state variable `copyError` to a predicate-style
name like `hasCopyError` and update its setter from `setCopyError` to
`setHasCopyError`; modify the useState declaration (const [hasCopyError,
setHasCopyError] = useState(false)) and replace all references to `copyError`
and `setCopyError` (including the instances around lines 430-434) across the
component (JSX conditional rendering, event handlers, and any functions that
read or write this state) to the new names to preserve intent and consistency.
In `@apps/citizen-pwa/src/hooks/useReport.ts`:
- Line 12: The hook changed actor to optional (actor?: string) so update
consumers like the TrackingScreen component's timeline renderers to guard
against undefined; in the TrackingScreen.tsx code paths that interpolate e.actor
(the timeline renderer or any place referencing e.actor) wrap it with a
conditional/fallback formatter (e.g., show a default label or omit the actor
when undefined) or use a small helper formatter so the UI never renders
"undefined · …".
In `@apps/citizen-pwa/src/lib/mappers.ts`:
- Around line 12-17: The timeline mapping blindly casts Firestore payloads into
ReportData fields; update the timeline construction to validate and sanitize
each evt before including it in ReportData: ensure the parent timeline is an
array, then filter/map events by checking typeof evt.event === 'string' and
typeof evt.timestamp === 'number' (reject or skip entries that fail), and only
include actor and note if typeof evt.actor === 'string' and typeof evt.note ===
'string'. Locate the timeline mapping in the ReportData construction (the
timeline: (...) .map((evt) => ...) block) and replace the casts with explicit
type checks and safe filtering so invalid or null values are not propagated.
In `@apps/citizen-pwa/src/services/callables.ts`:
- Around line 23-24: The code currently trusts result.data from callable() and
casts it directly; instead add runtime validation before returning: check that
result and result.data exist and that result.data.uid, result.data.role, and
result.data.accountStatus are present and of the expected types (e.g., strings),
or use a small validator function (e.g., validateCallableResponse) or a schema
(zod) to assert the shape; if validation fails, throw a descriptive Error (or
return a safe fallback) rather than blindly casting, and update the return site
(the code around the await callable() call and the result variable) to use the
validated/parsed object.
🪄 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: 3e2b2206-f312-46e0-a7c8-804f94a19b0e
📒 Files selected for processing (8)
apps/citizen-pwa/src/App.routes.test.tsxapps/citizen-pwa/src/components/MapTab/index.tsxapps/citizen-pwa/src/components/ProfileTab.tsxapps/citizen-pwa/src/components/RevealSheet.tsxapps/citizen-pwa/src/hooks/useReport.tsapps/citizen-pwa/src/lib/mappers.tsapps/citizen-pwa/src/services/callables.tsapps/citizen-pwa/src/utils/incident-meta.tsx
ReportTimelineEvent.actor became optional in the previous commit. TrackingScreen passed it into a template literal without null-check, triggering @typescript-eslint/restrict-template-expressions. Default to 'system' when actor is absent. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The committed firestore.rules had three manual edits from earlier PRs
that were not reflected in the template:
1. Removed isCitizen() — report_inbox create uses request.auth != null
to allow anonymous (pseudonymous) citizens to submit
2. Added publicRef, secretHash, correlationId to hasOnly/hasAll
3. Added reporter self-read on reports/{id} via report_private lookup
Update the template to match so build-rules.ts output stays in sync.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- ProfileTab: clear signOutError before each attempt; add role="alert" - RevealSheet: rename copyError -> hasCopyError; reset copied on catch - mappers: validate timeline event field types at boundary - callables: validate registerCitizen response shape before returning Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Summary
Fixes 6 critical and 10 important issues identified by QA assessment, plus 4 pre-existing failing route tests.
Critical fixes (6)
callables.ts—requestDataExportandregisterCitizennow wrapawait callable()in try/catch with descriptive error rethrow (was bare await, no error boundary)RevealSheet.tsx— Clipboard failure now shows "Copy failed — please write it down" to user instead of onlyconsole.error; setTimeout timer ref properly cleared on unmount (wasvoid tanti-pattern)useReport.ts+mappers.ts—ReportTimelineEvent.actorchanged fromstringtostring?; mapper individually maps timeline events with safe optional fields instead of unsafe bulk castImportant fixes (10)
incident-meta.tsx— ExtractedstatusMeta()andseverityDotColor()as shared exports (single source of truth, was duplicated in ProfileTab)ProfileTab.tsx— Removed duplicated status/severity logic, imports fromincident-meta;useBadgesnow usesuseMemo;handleSignOutshows error feedback instead of silent empty catchRevealSheet.tsx— ExtractedREVEAL_VARIANTSto module-level constant (was 42-line object recreated every render);GUARDIAN_BENEFITSextracted to shared constant (was duplicated inline)MapTab/index.tsx—severityLabel('all')now returns"All"instead of"A"App.routes.test.tsx— 4 failing route tests fixed by usingfindByTextfor lazy-loaded Suspense routes (wasgetByTextwhich asserted before React resolved the lazy import)Test results
Commits (6, one per logical change)
1ec2fc9— callables try/catch error boundaries13a7ebf— timeline actor optional + safe mapper7a1b011— RevealSheet clipboard feedback + timer cleanup + extract variantsddc3bc3— deduplicate status/severity logic + fix useBadges + signOut feedback1c64b68— severityLabel('all') → 'All'8a98409— fix 4 failing lazy-loaded route testsSummary by Sourcery
Improve reliability and UX of the citizen PWA around reporting, account flows, and routing, while fixing flaky tests and centralizing shared incident metadata.
Bug Fixes:
Enhancements:
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Tests