feat(phase-1): add identity spine infrastructure#41
Merged
Conversation
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
- Replace workspace config with explicit include/exclude - Add explicit vitest imports to App.test.tsx - Disable @typescript-eslint/no-unsafe-call for test files - Note: citizen-pwa tests run separately via cd apps/citizen-pwa && pnpm test Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Reviewer's GuideImplements Phase 1 "identity spine" across shared types/validators, Cloud Functions, Firestore rules tests, and the citizen PWA shell, including Firebase web bootstrap utilities, staff claim lifecycle functions, and a minimal alert + config surface wired end-to-end. Sequence diagram for citizen PWA Phase 1 shell bootstrapsequenceDiagram
actor Citizen as Citizen
participant CitizenPWA as "CitizenPWA_App"
participant UseCitizenShell as "useCitizenShell_hook"
participant SharedFirebase as "@bantayog_shared_firebase"
participant FirebaseAuth as "Firebase_Auth"
participant FirebaseAppCheck as "Firebase_AppCheck"
participant Firestore as "Firestore_DB"
Citizen->>CitizenPWA: Load / open app
CitizenPWA->>UseCitizenShell: Call useCitizenShell()
UseCitizenShell->>SharedFirebase: parseFirebaseWebEnv(import_meta_env)
SharedFirebase-->>UseCitizenShell: FirebaseWebEnv
UseCitizenShell->>SharedFirebase: createFirebaseWebApp(env)
SharedFirebase-->>UseCitizenShell: FirebaseApp
UseCitizenShell->>SharedFirebase: getFirebaseAuth(app)
SharedFirebase-->>UseCitizenShell: Auth
UseCitizenShell->>SharedFirebase: getFirebaseDb(app)
SharedFirebase-->>UseCitizenShell: Firestore
UseCitizenShell->>SharedFirebase: createAppCheck(app, env)
SharedFirebase-->>FirebaseAppCheck: initializeAppCheck
FirebaseAppCheck-->>UseCitizenShell: AppCheck active
UseCitizenShell->>UseCitizenShell: state.appCheckState = active
UseCitizenShell->>SharedFirebase: ensurePseudonymousSignIn(auth)
SharedFirebase->>FirebaseAuth: signInAnonymously()
FirebaseAuth-->>SharedFirebase: User
SharedFirebase-->>UseCitizenShell: User
UseCitizenShell->>UseCitizenShell: state.authState = signed-in
UseCitizenShell->>SharedFirebase: subscribeMinAppVersion(db, callback)
SharedFirebase->>Firestore: onSnapshot(system_config/min_app_version)
Firestore-->>SharedFirebase: MinAppVersionDoc
SharedFirebase-->>UseCitizenShell: callback(minAppVersion)
UseCitizenShell->>UseCitizenShell: state.minAppVersion = doc
UseCitizenShell->>SharedFirebase: subscribeAlerts(db, callback)
SharedFirebase->>Firestore: onSnapshot(alerts orderBy publishedAt desc limit 5)
Firestore-->>SharedFirebase: AlertDoc[]
SharedFirebase-->>UseCitizenShell: callback(alerts)
UseCitizenShell->>UseCitizenShell: state.alerts = alerts
UseCitizenShell->>UseCitizenShell: state.status = ready
CitizenPWA-->>Citizen: Render status, auth, app check, min version, alerts
Sequence diagram for staff claim lifecycle callable functionssequenceDiagram
actor Superadmin as Provincial_Superadmin
participant Client as Admin_Client
participant Callable as Callable_Endpoint
participant AccountLifecycle as account_lifecycle_ts
participant Validators as shared_validators_auth
participant CustomClaims as custom_claims_ts
participant AdminAuth as Firebase_AdminAuth
participant AdminDb as Firestore_AdminDb
Superadmin->>Client: Initiate setStaffClaims(uid, role, scopes)
Client->>Callable: HTTPS callable request
Callable->>AccountLifecycle: setStaffClaims(request)
AccountLifecycle->>AccountLifecycle: Check request.auth.token.role
AccountLifecycle->>Validators: setStaffClaimsInputSchema.parse(request.data)
Validators-->>AccountLifecycle: parsedInput or throw
AccountLifecycle->>CustomClaims: buildStaffClaims(parsedInput)
CustomClaims-->>AccountLifecycle: CustomClaims
AccountLifecycle->>AccountLifecycle: updatedAt = Date.now()
AccountLifecycle->>AdminAuth: setCustomUserClaims(uid, claims)
AccountLifecycle->>AdminDb: batch = adminDb.batch()
AccountLifecycle->>AdminDb: batch.set(active_accounts/uid, buildActiveAccountDoc())
AccountLifecycle->>AdminDb: batch.set(claim_revocations/uid, buildClaimRevocationDoc(reason=claims_updated))
AccountLifecycle->>AdminDb: batch.commit()
AdminDb-->>AccountLifecycle: commit success
AccountLifecycle-->>Callable: { uid, claims }
Callable-->>Client: Response
Superadmin->>Client: Initiate suspendStaffAccount(uid)
Client->>Callable: HTTPS callable request
Callable->>AccountLifecycle: suspendStaffAccount(request)
AccountLifecycle->>AccountLifecycle: Check request.auth.token.role
AccountLifecycle->>Validators: suspendStaffAccountInputSchema.parse(request.data)
Validators-->>AccountLifecycle: input
AccountLifecycle->>AdminDb: get(active_accounts/uid)
AdminDb-->>AccountLifecycle: snapshot
AccountLifecycle->>AdminDb: set(active_accounts/uid, accountStatus=suspended, updatedAt, merge)
AccountLifecycle->>AdminDb: set(claim_revocations/uid, buildClaimRevocationDoc(reason=suspended))
AdminDb-->>AccountLifecycle: writes committed
AccountLifecycle-->>Callable: { uid, status: suspended }
Callable-->>Client: Response
Entity relationship diagram for Phase 1 identity spine documentserDiagram
USER {
string uid
}
ACTIVE_ACCOUNT_DOC {
string uid
string role
string accountStatus
string municipalityId
string agencyId
string[] permittedMunicipalityIds
boolean mfaEnrolled
number lastClaimIssuedAt
number updatedAt
}
CLAIM_REVOCATION_DOC {
string uid
number revokedAt
string reason
}
MIN_APP_VERSION_DOC {
string citizen
string admin
string responder
number updatedAt
}
ALERT_DOC {
string title
string body
string severity
number publishedAt
string publishedBy
}
SYSTEM_CONFIG_MIN_APP_VERSION_DOC {
string id
}
ALERT_COLLECTION_DOC {
string id
}
USER ||--|| ACTIVE_ACCOUNT_DOC : has_active_account
USER ||--o{ CLAIM_REVOCATION_DOC : has_revocations
SYSTEM_CONFIG_MIN_APP_VERSION_DOC ||--|| MIN_APP_VERSION_DOC : wraps
ALERT_COLLECTION_DOC ||--|| ALERT_DOC : stores
USER ||--o{ ALERT_DOC : may_publish
Class diagram for Phase 1 identity spine types and validatorsclassDiagram
class CustomClaims {
+UserRole role
+MunicipalityId municipalityId
+AgencyId agencyId
+MunicipalityId[] permittedMunicipalityIds
+AccountStatus accountStatus
+boolean mfaEnrolled
+number lastClaimIssuedAt
+boolean breakGlassSession
}
class ActiveAccountDoc {
+UserUid uid
+UserRole role
+AccountStatus accountStatus
+MunicipalityId municipalityId
+AgencyId agencyId
+MunicipalityId[] permittedMunicipalityIds
+boolean mfaEnrolled
+number lastClaimIssuedAt
+number updatedAt
}
class ClaimRevocationDoc {
+UserUid uid
+number revokedAt
+string reason
}
class MinAppVersionDoc {
+string citizen
+string admin
+string responder
+number updatedAt
}
class AlertDoc {
+string title
+string body
+string severity
+number publishedAt
+string publishedBy
}
class setStaffClaimsInputSchema {
+string uid
+UserRole role
+string municipalityId
+string agencyId
+string[] permittedMunicipalityIds
+boolean mfaEnrolled
}
class suspendStaffAccountInputSchema {
+string uid
+string reason
}
class activeAccountSchema {
+string uid
+UserRole role
+AccountStatus accountStatus
+string municipalityId
+string agencyId
+string[] permittedMunicipalityIds
+boolean mfaEnrolled
+number lastClaimIssuedAt
+number updatedAt
}
class claimRevocationSchema {
+string uid
+number revokedAt
+string reason
}
class minAppVersionSchema {
+string citizen
+string admin
+string responder
+number updatedAt
}
class alertSchema {
+string title
+string body
+string severity
+number publishedAt
+string publishedBy
}
class FirebaseWebEnv {
+string apiKey
+string authDomain
+string projectId
+string appId
+string messagingSenderId
+string storageBucket
+string appCheckSiteKey
}
class FirebaseHelpers {
+createFirebaseWebApp(env FirebaseWebEnv) FirebaseApp
+createAppCheck(app FirebaseApp, env FirebaseWebEnv) AppCheck
+parseFirebaseWebEnv(source Record)
+getFirebaseAuth(app FirebaseApp) Auth
+ensurePseudonymousSignIn(auth Auth) User
+getFirebaseDb(app FirebaseApp) Firestore
+subscribeMinAppVersion(db Firestore, callback)
+subscribeAlerts(db Firestore, callback)
+getSessionTimeoutMs(role UserRole) number
}
CustomClaims --> ActiveAccountDoc : shapes
CustomClaims --> ClaimRevocationDoc : shapes
ActiveAccountDoc <.. activeAccountSchema : validated_by
ClaimRevocationDoc <.. claimRevocationSchema : validated_by
MinAppVersionDoc <.. minAppVersionSchema : validated_by
AlertDoc <.. alertSchema : validated_by
setStaffClaimsInputSchema --> CustomClaims : input_to_buildStaffClaims
suspendStaffAccountInputSchema --> ClaimRevocationDoc : input_for_suspension
FirebaseWebEnv <.. FirebaseHelpers : used_by
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds Phase‑1 identity infrastructure: shared Firebase client/helpers, App Check, pseudonymous auth, realtime Firestore subscriptions, Cloud Functions for staff claim lifecycle, Zod validators, tests, a bootstrap script, PWA shell consuming live shell state, updated security rules, and documentation updates. Changes
Sequence Diagram(s)sequenceDiagram
participant App as App Component
participant Hook as useCitizenShell
participant Shared as shared-firebase
participant AppCheck as App Check
participant Auth as Firebase Auth
participant DB as Firestore
participant User as User
App->>Hook: mount
Hook->>Shared: parse env & createFirebaseWebApp()
Shared-->>Hook: FirebaseApp
Hook->>Shared: getFirebaseAuth(), getFirebaseDb()
Shared-->>Hook: Auth, Firestore
Hook->>AppCheck: createAppCheck(app, env)
AppCheck-->>Hook: active / failed
Hook->>Auth: ensurePseudonymousSignIn()
alt has user
Auth-->>Hook: currentUser
else
Auth->>User: signInAnonymously()
User-->>Auth: credential
Auth-->>Hook: anon user
end
Hook->>DB: subscribeMinAppVersion()
DB-->>Hook: min version updates
Hook->>DB: subscribeAlerts()
DB-->>Hook: alerts list
Hook-->>App: shell state (status, authState, appCheckState, user, versions, alerts)
Note over Hook: on unmount -> unsubscribe listeners
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 2 issues, and left some high level feedback:
- In
suspendStaffAccount, thereasonfield fromsuspendStaffAccountInputSchemais parsed but not used;buildClaimRevocationDocis always called with'suspended', so either removereasonfrom the input schema or plumb it through to the revocation document. buildStaffClaimscurrently re-parses itsinputwithsetStaffClaimsInputSchema, even though the caller (setStaffClaims) already validated the data; consider changing the function to accept the parsed type (e.g.z.infer<typeof setStaffClaimsInputSchema>) to avoid double validation and make misuse harder.- The Zod schemas for
active_accounts/claim_revocationsand the corresponding TypeScript interfaces (ActiveAccountDoc,ClaimRevocationDoc) encode the same shape separately; consider deriving the TS types from the Zod schemas (or vice versa) to keep them in sync automatically.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `suspendStaffAccount`, the `reason` field from `suspendStaffAccountInputSchema` is parsed but not used; `buildClaimRevocationDoc` is always called with `'suspended'`, so either remove `reason` from the input schema or plumb it through to the revocation document.
- `buildStaffClaims` currently re-parses its `input` with `setStaffClaimsInputSchema`, even though the caller (`setStaffClaims`) already validated the data; consider changing the function to accept the parsed type (e.g. `z.infer<typeof setStaffClaimsInputSchema>`) to avoid double validation and make misuse harder.
- The Zod schemas for `active_accounts` / `claim_revocations` and the corresponding TypeScript interfaces (`ActiveAccountDoc`, `ClaimRevocationDoc`) encode the same shape separately; consider deriving the TS types from the Zod schemas (or vice versa) to keep them in sync automatically.
## Individual Comments
### Comment 1
<location path="apps/citizen-pwa/src/useCitizenShell.ts" line_range="53-56" />
<code_context>
+ useEffect(() => {
+ unmountedRef.current = false
+
+ const env = parseFirebaseWebEnv(import.meta.env)
+ const app = createFirebaseWebApp(env)
+ const db = getFirebaseDb(app)
+ const auth = getFirebaseAuth(app)
+
+ // Capture refs in local variables to avoid exhaustive-deps warning in cleanup
</code_context>
<issue_to_address>
**issue (bug_risk):** Guard parseFirebaseWebEnv / Firebase initialization errors to avoid crashing the component tree.
`parseFirebaseWebEnv` (and possibly `createFirebaseWebApp`) can throw synchronously if env vars are missing or malformed. That exception will currently escape the effect and crash the React tree instead of transitioning this hook to a controlled error state. Please wrap env parsing / app creation in a try/catch (similar to the App Check handling) and set the hook state to `status: 'error'` with a clear message so misconfigurations are surfaced without hard crashes.
</issue_to_address>
### Comment 2
<location path="packages/shared-validators/src/auth.ts" line_range="31-34" />
<code_context>
+ }
+ })
+
+export const suspendStaffAccountInputSchema = z.object({
+ uid: z.string().min(1),
+ reason: z.enum(['suspended', 'claims_updated', 'manual_refresh']),
+})
+
</code_context>
<issue_to_address>
**suggestion:** The `reason` field on suspendStaffAccount input is unused and overly permissive.
The parsed `reason` is never used; `suspendStaffAccount` always writes `reason: 'suspended'` to the revocation doc. This makes the `reason` field in `suspendStaffAccountInputSchema` dead and potentially misleading, especially with values like `'claims_updated'` and `'manual_refresh'`. Please either remove `reason` from the input schema, or, if callers should control it, pass it through to `buildClaimRevocationDoc` and constrain the enum to the actually supported values.
```suggestion
export const suspendStaffAccountInputSchema = z.object({
uid: z.string().min(1),
})
```
</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: 24
🤖 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/App.test.tsx`:
- Line 12: The mocked minAppVersion object in App.test.tsx is missing the
required updatedAt field from the MinAppVersionDoc contract; update the test
mock for minAppVersion to include an updatedAt property (e.g., an ISO timestamp
or Date) so the shape matches MinAppVersionDoc and any validators/type checks,
locating and editing the minAppVersion mock in App.test.tsx.
In `@apps/citizen-pwa/src/App.tsx`:
- Line 43: Replace the unstable key built from
`${alert.title}-${String(alert.publishedAt)}` with a stable unique identifier:
add `id` to the shared AlertDoc type (and any code that constructs/fetches
alerts) and use `alert.id` as the React key for the <article> (the mapping that
renders `alert` in App.tsx). Ensure all places that produce AlertDoc include the
id so list reconciliation uses this stable unique key instead of
title+timestamp.
In `@apps/citizen-pwa/src/useCitizenShell.ts`:
- Around line 103-112: The catch block currently forces appCheckState to
'failed' which can overwrite a prior successful App Check; update the error
handler in the .catch to preserve the current appCheckState instead of forcing
'failed' — use a functional setState update (referencing setState and
unmounted.current) that spreads initialState and then sets status: 'error' and
error: (error instanceof Error ? error.message : 'Pseudonymous sign-in failed')
while copying the existing appCheckState from the previous state
(prev.appCheckState) so the existing App Check result (set earlier around line
65) is not lost.
- Around line 79-101: Both subscription callbacks (subscribeMinAppVersion and
subscribeAlerts) redundantly set shared auth/status/user fields causing extra
re-renders and recreating user each time; update the code so
authentication-related state is set once (e.g., immediately after sign-in where
you currently set authState/user) and modify the subscribeMinAppVersion and
subscribeAlerts callbacks to only call setState to merge their specific slice
(minAppVersion or alerts) without touching status, authState, or user; ensure
cleanup.current.stopVersion and cleanup.current.stopAlerts remain
subscribed/unsubscribed but no longer overwrite the shared auth fields, and
preserve the unmounted.current guard around their specific updates.
In `@docs/progress.md`:
- Around line 7-16: The document incorrectly marks verification as complete
while the checklist shows a failing unit test and skipped emulator-backed rules
test; update the status string "**Status:** Verification complete (see findings
below)" to indicate "Verification incomplete / pending" and add a clear
remediation note referencing the checklist entries that failed/skipped (`pnpm
test` FAIL and `firebase emulators:exec ...` SKIP) requiring: re-running full
test suite until all pass, running Firestore rules tests against the local
emulator, and obtaining explicit staging approval before any prod deployment;
also add a short checklist item or inline instruction reminding authors to
deploy to the dev emulator, run the full suite, and get staging sign-off for
changes touching rules/auth/functions.
In `@functions/package.json`:
- Line 16: The current generic "deploy" npm script is a risky footgun; replace
it with explicit environment-targeted scripts and guards: remove or change the
"deploy" script that runs "firebase deploy --only functions" to instead provide
named scripts like "deploy:dev", "deploy:staging", and "deploy:prod" that
require an explicit project flag or PROJECT env (e.g., --project or
process.env.PROJECT) and add a confirmation step for "deploy:prod"; ensure
"deploy:dev" runs the emulator and the full test suite before deploying, and
make the plain "deploy" either disabled or echo a message instructing to use the
environment-specific scripts so accidental prod deploys are prevented.
In `@functions/scripts/bootstrap-phase1.ts`:
- Line 18: The top-level call to main() lacks explicit error handling; update
the bootstrap entry so any rejection from main() is caught, logged with
structured error details, and the process exits with a non-zero code.
Specifically, wrap the top-level invocation of main() (the symbol main) in a
promise rejection handler or an immediately-invoked async wrapper that catches
errors, calls your logger (e.g., processLogger.error or console.error) with the
error and context, and then calls process.exit(1) to ensure deterministic
failure reporting.
- Around line 8-15: The current seed writes call
adminDb.collection('system_config').doc('min_app_version').set(...) and then
individually await each adminDb.collection('alerts').doc(alert.id).set(alert)
which can leave a partially-applied Phase 1 state if any write fails; change
this to a single atomic batch: create a write batch (e.g., via adminDb.batch()),
add a batch.set for the system_config/min_app_version doc and batch.set for each
alerts doc inside the loop, then call batch.commit() once so all seed writes
succeed or fail together.
In `@functions/src/__tests__/firestore.rules.test.ts`:
- Around line 78-153: Add two tests inside the existing "phase 1 firestore
rules" describe block: (1) a self-read test for the claim_revocations collection
that uses testEnv.authenticatedContext('<uid>', {...}).firestore() and asserts
assertSucceeds(db.collection('claim_revocations').doc('<uid>').get()) and
assertFails for other users, and (2) a write-denial test for alerts that uses
any authenticatedContext or unauthenticatedContext and asserts
assertFails(db.collection('alerts').doc('someId').set({...})); place them
alongside the existing tests so they reference the same testEnv,
assertSucceeds/assertFails helpers and validate the self-only read for
claim_revocations and the rule that denies all writes to alerts.
- Around line 14-19: The path resolution for loading firestore.rules is tied to
process.cwd(); update the test setup that calls initializeTestEnvironment and
the readFileSync/resolve usage to derive the rules file path from the test file
location instead (use __dirname) so the rules file is found regardless of
working directory. Locate the block that calls initializeTestEnvironment and
replace the resolve(process.cwd(), '../infra/firebase/firestore.rules') usage
with a resolution based on __dirname (e.g., resolve(__dirname,
'../infra/firebase/firestore.rules') or path.join(__dirname, '..', 'infra',
'firebase', 'firestore.rules')) so readFileSync reads the correct file robustly.
Ensure the change only affects the path passed to readFileSync; keep
initializeTestEnvironment and readFileSync usage intact.
In `@functions/src/auth/account-lifecycle.ts`:
- Around line 54-61: suspendStaffAccount performs two separate awaits to
adminDb.collection('active_accounts').doc(input.uid).set(...) and
.collection('claim_revocations').doc(input.uid).set(...), which is non-atomic;
change this to use a Firestore write batch: create a batch, call batch.set(...)
for the active_accounts doc (merging current with accountStatus:'suspended' and
updatedAt: revokedAt) and batch.set(...) for the claim_revocations doc using
buildClaimRevocationDoc(input.uid, revokedAt, 'suspended'), then call
batch.commit() and await the single commit so both writes succeed or fail
together.
- Line 18: Replace direct Zod .parse calls with .safeParse for both
setStaffClaimsInputSchema and suspendStaffAccountInputSchema so validation
failures are handled explicitly; if safeParse returns success use the
parsed.data, otherwise throw an HttpsError('invalid-argument', 'Validation
failed', { issues: result.error.format() }) (or similar) to return a descriptive
client error instead of an internal exception—update the code around the
setStaffClaimsInputSchema.parse(...) assignment and the
suspendStaffAccountInputSchema.parse(...) use to perform this conditional check
and throw the HttpsError on validation failure.
In `@functions/src/auth/custom-claims.ts`:
- Around line 5-7: buildStaffClaims currently re-parses input with
setStaffClaimsInputSchema.parse even though setStaffClaims already passes a
parsed value; remove the redundant parse by changing buildStaffClaims to accept
the parsed type produced by setStaffClaimsInputSchema.parse (e.g., rename param
to parsedInput and update its TypeScript type to the schema's output type) and
remove the call to setStaffClaimsInputSchema.parse inside buildStaffClaims, then
update callers (notably setStaffClaims) to pass the parsed value directly;
alternatively, if you need standalone parsing, add a clear JSDoc comment on
buildStaffClaims stating it expects raw input and keep the parse—pick one
approach and apply it consistently.
- Around line 33-45: buildActiveAccountDoc currently always includes
municipalityId and agencyId (and other optional fields) even when undefined;
change the function to only add optional properties (municipalityId, agencyId,
permittedMunicipalityIds if null/undefined, mfaEnrolled, lastClaimIssuedAt) to
the returned object when they are defined to avoid writing undefined fields to
Firestore—construct the base object with required keys (uid, role,
accountStatus, updatedAt) and then conditionally assign each optional property
(using short-circuit spreads or if-blocks) so only present values are included.
In `@functions/src/bootstrap/phase1-seed.ts`:
- Around line 1-22: Annotate the buildPhase1SeedDocs function with an explicit
shared contract return type (e.g., SharedSeedDocs or Phase1SeedContract) by
importing that type and changing the signature to export function
buildPhase1SeedDocs(updatedAt: number): SharedSeedDocs; ensure the returned
object matches the contract shape (systemConfig.min_app_version and alerts array
items conform to the imported type names/fields) and adjust any property
names/structures to satisfy the type so shape drift is caught at compile time.
In `@packages/shared-firebase/src/app.ts`:
- Around line 20-25: createAppCheck should guard against duplicate
initialization the same way createFirebaseWebApp does: attempt to fetch an
existing App Check instance (use getAppCheck(app) in a try/catch or equivalent
existence check) and return it if present, otherwise call
initializeAppCheck(app, { provider: new
ReCaptchaV3Provider(env.appCheckSiteKey), isTokenAutoRefreshEnabled: true });
update createAppCheck to return the existing AppCheck when present to avoid
throwing on double initialization.
In `@packages/shared-firebase/src/env.test.ts`:
- Around line 6-17: The test currently only asserts projectId after calling
parseFirebaseWebEnv; strengthen it by asserting the full normalized output
matches all expected fields. Update the assertion for parseFirebaseWebEnv(...)
to expect the complete object containing apiKey, authDomain, projectId, appId,
messagingSenderId, storageBucket, and appCheckSiteKey (matching the values
passed in VITE_FIREBASE_*), e.g. replace the single-field toMatchObject with a
toEqual (or toMatchObject) asserting all those normalized property names
returned by parseFirebaseWebEnv.
In `@packages/shared-types/src/auth.ts`:
- Around line 15-25: The builder functions buildActiveAccountDoc and
buildClaimRevocationDoc currently accept uid: string but return objects typed as
ActiveAccountDoc/ClaimRevocationDoc which require uid: UserUid; fix by branding
the uid before returning (e.g., wrap the incoming uid with the project’s
branding helper, such as asUserUid(uid)) so the returned objects satisfy the
branded UserUid type, and ensure the helper is imported where used (or change
the function signatures to accept UserUid if callers already pass branded
values).
In `@packages/shared-types/src/config.ts`:
- Around line 10-16: AlertDoc currently lacks the alert identifier; add an id:
string property to the AlertDoc interface so the shape matches the seeded alerts
produced by the seed process (e.g., phase1-seed). Update any producers that
construct AlertDoc (places that create seeded alerts) to include the id and
adjust consumers/usage sites to expect and use AlertDoc.id (for stable UI keys
and type alignment).
In `@packages/shared-validators/src/auth.ts`:
- Around line 31-34: The schema suspendStaffAccountInputSchema defines a reason
but suspendStaffAccount ignores it and always uses 'suspended'; either remove
reason from the schema or pass input.reason into the revocation builder. Update
suspendStaffAccount to call buildClaimRevocationDoc(input.uid, revokedAt,
input.reason) so the validated value is used, or remove the reason property from
suspendStaffAccountInputSchema (z.enum([...])) if the operation will always use
'suspended'; ensure tests and callers are updated accordingly.
- Around line 3-11: Export accountStatusSchema and derive both schemas from the
canonical shared-types enums: import the UserRole and AccountStatus types from
'@bantayog/shared-types', declare const arrays of allowed values (e.g.
USER_ROLES and ACCOUNT_STATUSES) typed to satisfy readonly UserRole[] / readonly
AccountStatus[], then create and export userRoleSchema = z.enum(USER_ROLES) and
export accountStatusSchema = z.enum(ACCOUNT_STATUSES); update references to use
the exported schemas instead of the local literals.
In `@packages/shared-validators/src/config.ts`:
- Around line 3-8: The minAppVersionSchema currently allows any non-empty string
for citizen, admin, and responder which permits invalid tags like "latest";
update the schema for those fields (citizen, admin, responder) to enforce proper
semantic versioning by either using a strict semver regex (e.g.
major.minor.patch with optional prerelease/build) or, preferably, validating
with the semver library (e.g., import semver and use z.string().refine(v =>
semver.valid(v) !== null, { message: 'Invalid semver' })). Keep updatedAt as is
(number().int().nonnegative()) and update error messages accordingly.
In `@packages/shared-validators/src/phase1-auth.test.ts`:
- Around line 41-50: The test title says required fields are enforced but only
validates a valid payload; add explicit negative assertions to verify missing
fields fail: update the describe('claimRevocationSchema') test to include two
additional checks that call claimRevocationSchema.parse (or
claimRevocationSchema.safeParse) with payloads missing revokedAt and missing
reason and assert failure (e.g. expect(() =>
claimRevocationSchema.parse({...})).toThrow() or
expect(claimRevocationSchema.safeParse({...}).success).toBe(false)); keep the
existing valid-case assertion for completeness.
In `@README.md`:
- Around line 47-53: Update the "Phase 1 verification" checklist to add a final
explicit "staging approval" step: after running the listed commands (pnpm test,
pnpm --filter `@bantayog/functions` test:unit, pnpm --filter `@bantayog/functions`
test:rules, lint/typecheck/build) require a documented staging-approval item
that mandates deploying changes to the dev emulator, running the full test
suite, and obtaining explicit staging approval before any production deploy for
changes touching auth, security rules, DB indexes, deployment config, auth
flows, or Cloud Functions; reference the "Phase 1 verification" header and add a
bullet that outlines these exact requirements.
🪄 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: 202c89e1-d25a-4963-8dad-ffdbf52dfea5
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (41)
README.mdapps/citizen-pwa/package.jsonapps/citizen-pwa/src/App.module.cssapps/citizen-pwa/src/App.test.tsxapps/citizen-pwa/src/App.tsxapps/citizen-pwa/src/useCitizenShell.tsapps/citizen-pwa/tsconfig.jsonapps/citizen-pwa/vitest.config.tsdocs/progress.mdeslint.config.jsfunctions/package.jsonfunctions/scripts/bootstrap-phase1.tsfunctions/src/__tests__/firestore.rules.test.tsfunctions/src/__tests__/phase1-auth.test.tsfunctions/src/auth/account-lifecycle.tsfunctions/src/auth/custom-claims.tsfunctions/src/bootstrap/phase1-seed.tsfunctions/src/firebase-admin.tsfunctions/src/index.tsfunctions/vitest.config.tsinfra/firebase/firestore.rulespackages/shared-firebase/package.jsonpackages/shared-firebase/src/app.tspackages/shared-firebase/src/auth.tspackages/shared-firebase/src/env.test.tspackages/shared-firebase/src/env.tspackages/shared-firebase/src/firestore.tspackages/shared-firebase/src/index.tspackages/shared-firebase/vitest.config.tspackages/shared-types/src/auth.tspackages/shared-types/src/config.tspackages/shared-types/src/enums.tspackages/shared-types/src/index.tspackages/shared-validators/package.jsonpackages/shared-validators/src/alerts.tspackages/shared-validators/src/auth.tspackages/shared-validators/src/config.tspackages/shared-validators/src/index.tspackages/shared-validators/src/phase1-auth.test.tsvitest.config.tsvitest.workspace.ts
💤 Files with no reviewable changes (1)
- vitest.workspace.ts
|
|
||
| <div className={styles.feed}> | ||
| {state.alerts.map((alert) => ( | ||
| <article key={`${alert.title}-${String(alert.publishedAt)}`} className={styles.alert}> |
Contributor
There was a problem hiding this comment.
Use a stable unique alert key instead of title+timestamp concatenation.
Line 43 can collide when two alerts share the same title and publishedAt, which can produce incorrect list reconciliation.
Proposed direction
-<article key={`${alert.title}-${String(alert.publishedAt)}`} className={styles.alert}>
+<article key={alert.id} className={styles.alert}>(Requires AlertDoc to include id in shared types.)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/citizen-pwa/src/App.tsx` at line 43, Replace the unstable key built from
`${alert.title}-${String(alert.publishedAt)}` with a stable unique identifier:
add `id` to the shared AlertDoc type (and any code that constructs/fetches
alerts) and use `alert.id` as the React key for the <article> (the mapping that
renders `alert` in App.tsx). Ensure all places that produce AlertDoc include the
id so list reconciliation uses this stable unique key instead of
title+timestamp.
Comment on lines
+3
to
+11
| const userRoleSchema = z.enum([ | ||
| 'citizen', | ||
| 'responder', | ||
| 'municipal_admin', | ||
| 'agency_admin', | ||
| 'provincial_superadmin', | ||
| ]) | ||
|
|
||
| const accountStatusSchema = z.enum(['active', 'suspended', 'disabled']) |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider exporting accountStatusSchema and deriving from shared types.
-
accountStatusSchemais defined but not exported. Other modules may need to validate account status independently. -
userRoleSchemaduplicates theUserRoletype definition inpackages/shared-types/src/enums.ts. If the type changes, the schema must be manually kept in sync.
♻️ Proposed improvement for type derivation
import type { UserRole, AccountStatus } from '@bantayog/shared-types'
// Derive enum values from the type (requires listing them, but centralizes the source of truth)
const USER_ROLES = ['citizen', 'responder', 'municipal_admin', 'agency_admin', 'provincial_superadmin'] as const satisfies readonly UserRole[]
export const userRoleSchema = z.enum(USER_ROLES)
export const accountStatusSchema = z.enum(['active', 'suspended', 'disabled'] as const satisfies readonly AccountStatus[])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/shared-validators/src/auth.ts` around lines 3 - 11, Export
accountStatusSchema and derive both schemas from the canonical shared-types
enums: import the UserRole and AccountStatus types from
'@bantayog/shared-types', declare const arrays of allowed values (e.g.
USER_ROLES and ACCOUNT_STATUSES) typed to satisfy readonly UserRole[] / readonly
AccountStatus[], then create and export userRoleSchema = z.enum(USER_ROLES) and
export accountStatusSchema = z.enum(ACCOUNT_STATUSES); update references to use
the exported schemas instead of the local literals.
Comment on lines
+3
to
+8
| export const minAppVersionSchema = z.object({ | ||
| citizen: z.string().min(1), | ||
| admin: z.string().min(1), | ||
| responder: z.string().min(1), | ||
| updatedAt: z.number().int().nonnegative(), | ||
| }) |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Tighten version-string validation in the config schema.
Line 4-Line 6 currently allow any non-empty string. That permits malformed versions (e.g., "latest"), which can break version-gating logic later.
Proposed refactor
import { z } from 'zod'
+const semverPattern =
+ /^\d+\.\d+\.\d+(?:-[0-9A-Za-z-.]+)?(?:\+[0-9A-Za-z-.]+)?$/
+
export const minAppVersionSchema = z.object({
- citizen: z.string().min(1),
- admin: z.string().min(1),
- responder: z.string().min(1),
+ citizen: z.string().regex(semverPattern),
+ admin: z.string().regex(semverPattern),
+ responder: z.string().regex(semverPattern),
updatedAt: z.number().int().nonnegative(),
-})
+}).strict()As per coding guidelines: “Defensive Programming: Assume external input is malicious/broken. Validate at the boundary.”
📝 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
| export const minAppVersionSchema = z.object({ | |
| citizen: z.string().min(1), | |
| admin: z.string().min(1), | |
| responder: z.string().min(1), | |
| updatedAt: z.number().int().nonnegative(), | |
| }) | |
| const semverPattern = | |
| /^\d+\.\d+\.\d+(?:-[0-9A-Za-z-.]+)?(?:\+[0-9A-Za-z-.]+)?$/ | |
| export const minAppVersionSchema = z.object({ | |
| citizen: z.string().regex(semverPattern), | |
| admin: z.string().regex(semverPattern), | |
| responder: z.string().regex(semverPattern), | |
| updatedAt: z.number().int().nonnegative(), | |
| }).strict() |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/shared-validators/src/config.ts` around lines 3 - 8, The
minAppVersionSchema currently allows any non-empty string for citizen, admin,
and responder which permits invalid tags like "latest"; update the schema for
those fields (citizen, admin, responder) to enforce proper semantic versioning
by either using a strict semver regex (e.g. major.minor.patch with optional
prerelease/build) or, preferably, validating with the semver library (e.g.,
import semver and use z.string().refine(v => semver.valid(v) !== null, {
message: 'Invalid semver' })). Keep updatedAt as is
(number().int().nonnegative()) and update error messages accordingly.
Comment on lines
+41
to
+50
| describe('claimRevocationSchema', () => { | ||
| it('requires a revocation timestamp and reason', () => { | ||
| expect( | ||
| claimRevocationSchema.parse({ | ||
| uid: 'admin-1', | ||
| revokedAt: 1713350400000, | ||
| reason: 'suspended', | ||
| }), | ||
| ).toMatchObject({ reason: 'suspended' }) | ||
| }) |
Contributor
There was a problem hiding this comment.
claimRevocationSchema test does not verify the “requires” behavior it claims.
On Line 42, the title says required fields are enforced, but the case only validates a valid payload. Add explicit failure checks for missing revokedAt/reason.
✅ Suggested test completion
describe('claimRevocationSchema', () => {
it('requires a revocation timestamp and reason', () => {
- expect(
- claimRevocationSchema.parse({
- uid: 'admin-1',
- revokedAt: 1713350400000,
- reason: 'suspended',
- }),
- ).toMatchObject({ reason: 'suspended' })
+ expect(
+ claimRevocationSchema.parse({
+ uid: 'admin-1',
+ revokedAt: 1713350400000,
+ reason: 'suspended',
+ }),
+ ).toMatchObject({ reason: 'suspended' })
+
+ expect(() => claimRevocationSchema.parse({ uid: 'admin-1', reason: 'suspended' })).toThrow()
+ expect(() => claimRevocationSchema.parse({ uid: 'admin-1', revokedAt: 1713350400000 })).toThrow()
})
})📝 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
| describe('claimRevocationSchema', () => { | |
| it('requires a revocation timestamp and reason', () => { | |
| expect( | |
| claimRevocationSchema.parse({ | |
| uid: 'admin-1', | |
| revokedAt: 1713350400000, | |
| reason: 'suspended', | |
| }), | |
| ).toMatchObject({ reason: 'suspended' }) | |
| }) | |
| describe('claimRevocationSchema', () => { | |
| it('requires a revocation timestamp and reason', () => { | |
| expect( | |
| claimRevocationSchema.parse({ | |
| uid: 'admin-1', | |
| revokedAt: 1713350400000, | |
| reason: 'suspended', | |
| }), | |
| ).toMatchObject({ reason: 'suspended' }) | |
| expect(() => claimRevocationSchema.parse({ uid: 'admin-1', reason: 'suspended' })).toThrow() | |
| expect(() => claimRevocationSchema.parse({ uid: 'admin-1', revokedAt: 1713350400000 })).toThrow() | |
| }) | |
| }) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/shared-validators/src/phase1-auth.test.ts` around lines 41 - 50, The
test title says required fields are enforced but only validates a valid payload;
add explicit negative assertions to verify missing fields fail: update the
describe('claimRevocationSchema') test to include two additional checks that
call claimRevocationSchema.parse (or claimRevocationSchema.safeParse) with
payloads missing revokedAt and missing reason and assert failure (e.g. expect(()
=> claimRevocationSchema.parse({...})).toThrow() or
expect(claimRevocationSchema.safeParse({...}).success).toBe(false)); keep the
existing valid-case assertion for completeness.
Comment on lines
+47
to
+53
| ## Phase 1 verification | ||
|
|
||
| - `pnpm test` | ||
| - `pnpm --filter @bantayog/functions test:unit` | ||
| - `pnpm --filter @bantayog/functions test:rules` | ||
| - `pnpm lint && pnpm typecheck && pnpm build` | ||
|
|
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Add an explicit staging-approval step to this verification checklist.
Given this PR touches auth/function/security surface, include a final checklist item that requires explicit staging approval after emulator + full-suite validation.
Based on learnings: For changes to security rules, DB indexes, deployment config, auth flows, or Cloud Functions with existing traffic: show the diff, deploy to dev emulator first, run full test suite, then request explicit staging approval.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@README.md` around lines 47 - 53, Update the "Phase 1 verification" checklist
to add a final explicit "staging approval" step: after running the listed
commands (pnpm test, pnpm --filter `@bantayog/functions` test:unit, pnpm --filter
`@bantayog/functions` test:rules, lint/typecheck/build) require a documented
staging-approval item that mandates deploying changes to the dev emulator,
running the full test suite, and obtaining explicit staging approval before any
production deploy for changes touching auth, security rules, DB indexes,
deployment config, auth flows, or Cloud Functions; reference the "Phase 1
verification" header and add a bullet that outlines these exact requirements.
Adds @bantayog/shared-firebase, @testing-library/jest-dom, @testing-library/react, tsx 🤖 Generated with [Claude Code](https://claude.com/claude-code)
The explicit @testing-library/jest-dom import was removed during previous fix but is needed for TypeScript to recognize toBeInTheDocument matcher. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
apps/citizen-pwa/src/App.test.tsx (1)
13-13:⚠️ Potential issue | 🟡 MinorMocked
minAppVersionis missing theupdatedAtfield.The
minAppVersionmock does not fully match theMinAppVersionDoccontract, which requires anupdatedAtfield. This was previously flagged and remains unaddressed.🔧 Proposed fix
- minAppVersion: { citizen: '0.1.0', admin: '0.1.0', responder: '0.1.0' }, + minAppVersion: { + citizen: '0.1.0', + admin: '0.1.0', + responder: '0.1.0', + updatedAt: 1713350400000, + },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/citizen-pwa/src/App.test.tsx` at line 13, The test mock for minAppVersion is missing the required updatedAt field per the MinAppVersionDoc contract; update the mock object named minAppVersion in App.test.tsx to include an updatedAt property (ISO string or Date) so it matches MinAppVersionDoc shape used by the app and tests, e.g., add updatedAt: new Date().toISOString() (or the appropriate fixture timestamp) to the existing object.
🤖 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/App.test.tsx`:
- Around line 2-4: The test file currently imports from vitest twice (separate
imports for describe/it/expect and vi); consolidate them into a single import
statement that imports describe, it, expect and vi together (replace the
separate imports for describe/it/expect and vi with one import from 'vitest') to
improve readability and reduce duplicate module imports.
- Around line 31-34: The existing assertions in App.test.tsx use broad regexes
that can produce false positives; update the assertions that check for anon-123,
System online, 0.1.0, and signed-in to use more precise matching (e.g., exact
string matching via getByText with exact:true or exact string match) and/or
query by roles/textContent where appropriate; additionally add tests that mock
useCitizenShell returning status: 'error' with an error message to assert the
error text is rendered, and another test that mocks authState: 'signed-out' to
assert the signed-out UI is shown (use the useCitizenShell mock, render(<App
/>), and screen queries to locate the specific elements).
---
Duplicate comments:
In `@apps/citizen-pwa/src/App.test.tsx`:
- Line 13: The test mock for minAppVersion is missing the required updatedAt
field per the MinAppVersionDoc contract; update the mock object named
minAppVersion in App.test.tsx to include an updatedAt property (ISO string or
Date) so it matches MinAppVersionDoc shape used by the app and tests, e.g., add
updatedAt: new Date().toISOString() (or the appropriate fixture timestamp) to
the existing 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: 8c57cb41-74e1-4ef2-8681-24c4d0a866c6
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (1)
apps/citizen-pwa/src/App.test.tsx
…hell Wrap parseFirebaseWebEnv, createFirebaseWebApp, getFirebaseDb, getFirebaseAuth in try-catch to avoid crashing the component tree when env vars are missing or initialization fails. Now properly sets error state instead of throwing. Addresses code review feedback: bug_risk issue. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
apps/citizen-pwa/src/useCitizenShell.ts (1)
121-128:⚠️ Potential issue | 🟡 MinorPreserve the existing App Check result on sign-in failure.
If App Check succeeded earlier, Line 126 rewrites it to
'failed'for an unrelated auth error. Keep the currentappCheckStatewhen building the error state.🐛 Proposed fix
.catch((error: unknown) => { if (!unmounted.current) { - setState({ - ...initialState, - status: 'error', - appCheckState: 'failed', - error: error instanceof Error ? error.message : 'Pseudonymous sign-in failed', - }) + setState((current) => ({ + ...initialState, + status: 'error', + appCheckState: current.appCheckState, + error: error instanceof Error ? error.message : 'Pseudonymous sign-in failed', + })) } })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/citizen-pwa/src/useCitizenShell.ts` around lines 121 - 128, When handling the sign-in .catch, don't overwrite a previously successful App Check; preserve the current appCheckState instead of forcing 'failed'. Change the setState call inside the catch to use a functional update that retains the existing appCheckState (e.g., derive appCheckState from the previous state or current state) and only update status and error, while still spreading initialState and using unmounted.current and initialState as before; reference the setState call, initialState, and appCheckState to locate where to apply this change.
🤖 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/useCitizenShell.ts`:
- Around line 95-119: The callbacks passed to subscribeMinAppVersion and
subscribeAlerts currently set status: 'ready' unconditionally, which masks
Firestore error fallbacks that pass null/[]; update the handlers used in
ensurePseudonymousSignIn's then block so they only set status: 'ready' when the
snapshot is a confirmed success (e.g., check minAppVersion !== null before
setting ready in the subscribeMinAppVersion callback and check alerts !== null
(or Array.isArray(alerts) && alerts.length >= 0 but not the error sentinel)
before setting ready in the subscribeAlerts callback), or better: change
subscribeMinAppVersion/subscribeAlerts to surface an explicit success/error flag
and use that to set status:'ready' only on success; keep the user uid assignment
but avoid switching authState/status to 'ready' on error fallbacks and ensure
cleanup.current.stopVersion/stopAlerts remain unchanged.
---
Duplicate comments:
In `@apps/citizen-pwa/src/useCitizenShell.ts`:
- Around line 121-128: When handling the sign-in .catch, don't overwrite a
previously successful App Check; preserve the current appCheckState instead of
forcing 'failed'. Change the setState call inside the catch to use a functional
update that retains the existing appCheckState (e.g., derive appCheckState from
the previous state or current state) and only update status and error, while
still spreading initialState and using unmounted.current and initialState as
before; reference the setState call, initialState, and appCheckState to locate
where to apply this change.
🪄 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: d662ba79-89a9-4351-b8f0-26a6a22627ea
📒 Files selected for processing (1)
apps/citizen-pwa/src/useCitizenShell.ts
… state once - Use functional setState in sign-in .catch to preserve existing appCheckState - Set auth state once after sign-in, not redundantly in both subscription callbacks - Subscription callbacks now only update their specific slice (minAppVersion or alerts) Addresses code review feedback. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
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 `@apps/citizen-pwa/src/useCitizenShell.ts`:
- Around line 95-117: After ensurePseudonymousSignIn resolves, the code
currently registers Firestore listeners unconditionally which can run after the
component unmounted; update the .then() handler in useCitizenShell so you first
check unmounted.current and return early if true (i.e., do not call setState or
create subscriptions), and only after that assign cleanup.current.stopVersion =
subscribeMinAppVersion(db, ...) and cleanup.current.stopAlerts =
subscribeAlerts(db, ...); ensure all setState calls and subscription
registrations remain inside the same unmounted.current guard so subscriptions
are never created after unmount (references: ensurePseudonymousSignIn,
unmounted.current, cleanup.current.stopVersion, cleanup.current.stopAlerts,
subscribeMinAppVersion, subscribeAlerts, setState).
🪄 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: 2decfe3b-ee35-48a6-b872-87a1ab982859
📒 Files selected for processing (1)
apps/citizen-pwa/src/useCitizenShell.ts
Comment on lines
+95
to
+117
| void ensurePseudonymousSignIn(auth) | ||
| .then((user) => { | ||
| // Set auth state once immediately after sign-in | ||
| if (!unmounted.current) { | ||
| setState((current) => ({ | ||
| ...current, | ||
| status: 'ready', | ||
| authState: 'signed-in', | ||
| user: { uid: user.uid }, | ||
| })) | ||
| } | ||
|
|
||
| cleanup.current.stopVersion = subscribeMinAppVersion(db, (minAppVersion) => { | ||
| if (!unmounted.current) { | ||
| setState((current) => ({ ...current, minAppVersion })) | ||
| } | ||
| }) | ||
|
|
||
| cleanup.current.stopAlerts = subscribeAlerts(db, (alerts) => { | ||
| if (!unmounted.current) { | ||
| setState((current) => ({ ...current, alerts })) | ||
| } | ||
| }) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="apps/citizen-pwa/src/useCitizenShell.ts"
echo "Inspecting sign-in then-block and subscription registration:"
nl -ba "$file" | sed -n '95,120p'
echo
echo "Expected after fix: early guard 'if (unmounted.current) return' before subscribe* calls."
rg -n "if \\(unmounted\\.current\\) return|subscribeMinAppVersion|subscribeAlerts" "$file" -C2Repository: Exc1D/bantayog-alert
Length of output: 184
🏁 Script executed:
#!/bin/bash
file="apps/citizen-pwa/src/useCitizenShell.ts"
# Check file exists and get line count
if [ -f "$file" ]; then
total_lines=$(wc -l < "$file")
echo "File found. Total lines: $total_lines"
echo "---"
# Read lines 90-130 to see context around 95-117
sed -n '90,130p' "$file" | cat -n
else
echo "File not found"
fiRepository: Exc1D/bantayog-alert
Length of output: 1588
Avoid creating Firestore subscriptions after unmount.
The .then() handler executes unconditionally when the sign-in promise resolves, even if the component has unmounted. The if (!unmounted.current) guards on lines 99 and 105 only protect the setState calls inside subscription callbacks, not the subscription registration itself. This causes subscribeMinAppVersion() and subscribeAlerts() to create listeners after cleanup has already run, leaking active Firestore subscriptions.
♻️ Proposed fix
void ensurePseudonymousSignIn(auth)
.then((user) => {
+ if (unmounted.current) return
+
// Set auth state once immediately after sign-in
- if (!unmounted.current) {
- setState((current) => ({
- ...current,
- status: 'ready',
- authState: 'signed-in',
- user: { uid: user.uid },
- }))
- }
+ setState((current) => ({
+ ...current,
+ status: 'ready',
+ authState: 'signed-in',
+ user: { uid: user.uid },
+ }))
cleanup.current.stopVersion = subscribeMinAppVersion(db, (minAppVersion) => {
if (!unmounted.current) {
setState((current) => ({ ...current, minAppVersion }))
}
})
cleanup.current.stopAlerts = subscribeAlerts(db, (alerts) => {
if (!unmounted.current) {
setState((current) => ({ ...current, alerts }))
}
})
})🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/citizen-pwa/src/useCitizenShell.ts` around lines 95 - 117, After
ensurePseudonymousSignIn resolves, the code currently registers Firestore
listeners unconditionally which can run after the component unmounted; update
the .then() handler in useCitizenShell so you first check unmounted.current and
return early if true (i.e., do not call setState or create subscriptions), and
only after that assign cleanup.current.stopVersion = subscribeMinAppVersion(db,
...) and cleanup.current.stopAlerts = subscribeAlerts(db, ...); ensure all
setState calls and subscription registrations remain inside the same
unmounted.current guard so subscriptions are never created after unmount
(references: ensurePseudonymousSignIn, unmounted.current,
cleanup.current.stopVersion, cleanup.current.stopAlerts, subscribeMinAppVersion,
subscribeAlerts, setState).
…-template-expressions Template literal with number expression in JSX key requires explicit string conversion to avoid lint error. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…properly - suspendStaffAccount: pass input.reason (not 'suspended') to buildClaimRevocationDoc - buildStaffClaims: accept typed interface instead of re-parsing input Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (7)
apps/citizen-pwa/src/App.test.tsx (2)
2-4: 🧹 Nitpick | 🔵 TrivialConsolidate duplicate
vitestimports.Line 2 and Line 4 import from the same module; combine into one import for clarity.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/citizen-pwa/src/App.test.tsx` around lines 2 - 4, Combine the duplicate vitest imports into a single import: remove the separate import of vi and merge it with the existing import that brings in describe, it, and expect so you import { describe, it, expect, vi } from 'vitest' (update the import(s) where describe, it, expect, and vi are referenced).
36-39:⚠️ Potential issue | 🟡 MinorUse exact text assertions instead of broad regexes.
These regex checks can match unintended strings (especially
0.1.0, where.is wildcard), so this test can pass too easily.Suggested tightening
- expect(screen.getByText(/anon-123/)).toBeInTheDocument() - expect(screen.getByText(/System online/)).toBeInTheDocument() - expect(screen.getByText(/0.1.0/)).toBeInTheDocument() - expect(screen.getByText(/signed-in/)).toBeInTheDocument() + expect(screen.getByText('anon-123')).toBeInTheDocument() + expect(screen.getByText('System online')).toBeInTheDocument() + expect(screen.getByText('0.1.0')).toBeInTheDocument() + expect(screen.getByText('signed-in')).toBeInTheDocument()As per coding guidelines: "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/App.test.tsx` around lines 36 - 39, Replace the broad regex assertions in App.test.tsx with exact string assertions so the test matches only the intended text; specifically change the four calls that use screen.getByText(/.../) to use exact string matches (e.g., screen.getByText('anon-123'), screen.getByText('System online'), screen.getByText('0.1.0'), screen.getByText('signed-in')) so the test cannot be accidentally satisfied by unintended substrings or regex wildcards.apps/citizen-pwa/src/App.tsx (1)
44-44:⚠️ Potential issue | 🟡 MinorUse a truly stable unique key for alert items.
Line 44 can still collide (
publishedBy + publishedAtis not guaranteed unique), which risks incorrect list reconciliation. Prefer a dedicated immutablealert.idfrom the data model and use that as the React key.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/citizen-pwa/src/App.tsx` at line 44, The list key currently uses a derived key `${alert.publishedBy}-${String(alert.publishedAt)}` which can collide; update the mapping in App.tsx to use a stable immutable identifier (e.g., alert.id) instead of the derived string so React can correctly reconcile the list; locate where the alert is rendered (the element using key={`${alert.publishedBy}-${String(alert.publishedAt)}`}) and replace that expression with the dedicated unique field (alert.id), ensuring the data model provides that id.functions/src/auth/account-lifecycle.ts (3)
44-44: 🧹 Nitpick | 🔵 TrivialSame Zod validation concern applies here.
The
suspendStaffAccountInputSchema.parse()call has the same issue—validation failures will surface as internal errors rather thaninvalid-argumenterrors.♻️ Proposed fix
- const input = suspendStaffAccountInputSchema.parse(request.data) + const result = suspendStaffAccountInputSchema.safeParse(request.data) + if (!result.success) { + throw new HttpsError('invalid-argument', result.error.issues[0]?.message ?? 'Invalid input') + } + const input = result.data🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@functions/src/auth/account-lifecycle.ts` at line 44, The call to suspendStaffAccountInputSchema.parse(request.data) should be replaced with a safe parse pattern so Zod validation failures are mapped to an 'invalid-argument' error instead of surfacing as internal errors; use suspendStaffAccountInputSchema.safeParse(request.data), check result.success, and when false throw a firebase/functions HttpsError (code 'invalid-argument') with the validation error details (or a concise message) so the suspendStaffAccount handler returns a proper client error on bad input.
18-18: 🧹 Nitpick | 🔵 TrivialZod validation errors surface as generic internal errors to clients.
When
setStaffClaimsInputSchema.parse()throws aZodError, it propagates as an unhandled exception, resulting in a generic internal error for the caller. Consider usingsafeParseand throwing a descriptiveHttpsErrorwithinvalid-argumentcode.♻️ Proposed fix
- const parsed = setStaffClaimsInputSchema.parse(request.data) + const result = setStaffClaimsInputSchema.safeParse(request.data) + if (!result.success) { + throw new HttpsError('invalid-argument', result.error.issues[0]?.message ?? 'Invalid input') + } + const parsed = result.data🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@functions/src/auth/account-lifecycle.ts` at line 18, Replace the direct call to setStaffClaimsInputSchema.parse(request.data) with safe parsing: call setStaffClaimsInputSchema.safeParse(request.data), check the .success flag, and if false throw an HttpsError with code "invalid-argument" and include the Zod validation error messages (e.g., join issues) in the error details; keep using the same variable names (parsed or parsedResult) and ensure downstream code uses parsedResult.data when success is true so existing logic (in this file's account lifecycle handler) continues to work.
54-61:⚠️ Potential issue | 🟠 MajorNon-atomic writes risk data inconsistency.
Unlike
setStaffClaimswhich uses a batch commit,suspendStaffAccountperforms two separate writes. If the second write fails, the account will be marked suspended but no revocation record will exist. Apply the same batch pattern for consistency.🐛 Proposed fix using batch writes
const current = snapshot.data() ?? {} const revokedAt = Date.now() - await adminDb - .collection('active_accounts') - .doc(input.uid) - .set({ ...current, accountStatus: 'suspended', updatedAt: revokedAt }, { merge: true }) - await adminDb - .collection('claim_revocations') - .doc(input.uid) - .set(buildClaimRevocationDoc(input.uid, revokedAt, input.reason)) + const batch = adminDb.batch() + batch.set( + adminDb.collection('active_accounts').doc(input.uid), + { ...current, accountStatus: 'suspended', updatedAt: revokedAt }, + { merge: true }, + ) + batch.set( + adminDb.collection('claim_revocations').doc(input.uid), + buildClaimRevocationDoc(input.uid, revokedAt, input.reason), + ) + await batch.commit() return { uid: input.uid, status: 'suspended' }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@functions/src/auth/account-lifecycle.ts` around lines 54 - 61, suspendStaffAccount currently performs two separate writes which can leave data inconsistent if the second write fails; change it to use a single Firestore batch: create a WriteBatch (const batch = adminDb.batch()), call batch.set(...) for adminDb.collection('active_accounts').doc(input.uid) with the merged payload ({ ...current, accountStatus: 'suspended', updatedAt: revokedAt }, { merge: true }) and batch.set(...) for adminDb.collection('claim_revocations').doc(input.uid) with buildClaimRevocationDoc(input.uid, revokedAt, input.reason), then await batch.commit() so both writes succeed or both fail atomically.functions/src/auth/custom-claims.ts (1)
40-52: 🧹 Nitpick | 🔵 TrivialAvoid writing
undefinedfields to Firestore.When
municipalityIdoragencyIdareundefined, they're still included in the returned object. While Firestore handles this, it can cause confusion and inconsistent document shapes. Consider conditionally including these fields similar tobuildStaffClaims.♻️ Proposed improvement
export function buildActiveAccountDoc(uid: string, claims: CustomClaims, updatedAt: number) { - return { + const doc: Record<string, unknown> = { uid, role: claims.role, accountStatus: claims.accountStatus, - municipalityId: claims.municipalityId, - agencyId: claims.agencyId, permittedMunicipalityIds: claims.permittedMunicipalityIds ?? [], mfaEnrolled: claims.mfaEnrolled, lastClaimIssuedAt: claims.lastClaimIssuedAt, updatedAt, } + if (claims.municipalityId !== undefined) doc.municipalityId = claims.municipalityId + if (claims.agencyId !== undefined) doc.agencyId = claims.agencyId + return doc }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@functions/src/auth/custom-claims.ts` around lines 40 - 52, The returned object from buildActiveAccountDoc includes municipalityId and agencyId even when undefined; change buildActiveAccountDoc to conditionally add municipalityId and agencyId only when they are not undefined (similar to buildStaffClaims), so the Firestore document doesn't get explicit undefined fields; keep permittedMunicipalityIds defaulting to [] and still include role, accountStatus, mfaEnrolled, lastClaimIssuedAt, lastClaimIssuedAt, updatedAt and uid as before, but merge municipalityId and agencyId into the result object only when claims.municipalityId !== undefined / claims.agencyId !== undefined.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@functions/src/auth/account-lifecycle.ts`:
- Around line 39-64: suspendStaffAccount currently updates Firestore but doesn't
update the user's JWT custom claims, causing token/db state mismatch; call
adminAuth.setCustomUserClaims(input.uid, { accountStatus: 'suspended' }) before
writing to 'active_accounts' and 'claim_revocations' so tokens reflect
suspension, and ensure errors from setCustomUserClaims are handled (fail fast or
log and abort) to avoid partial updates; keep existing use of
buildClaimRevocationDoc and revoke timestamp (revokedAt) when ordering these
operations.
---
Duplicate comments:
In `@apps/citizen-pwa/src/App.test.tsx`:
- Around line 2-4: Combine the duplicate vitest imports into a single import:
remove the separate import of vi and merge it with the existing import that
brings in describe, it, and expect so you import { describe, it, expect, vi }
from 'vitest' (update the import(s) where describe, it, expect, and vi are
referenced).
- Around line 36-39: Replace the broad regex assertions in App.test.tsx with
exact string assertions so the test matches only the intended text; specifically
change the four calls that use screen.getByText(/.../) to use exact string
matches (e.g., screen.getByText('anon-123'), screen.getByText('System online'),
screen.getByText('0.1.0'), screen.getByText('signed-in')) so the test cannot be
accidentally satisfied by unintended substrings or regex wildcards.
In `@apps/citizen-pwa/src/App.tsx`:
- Line 44: The list key currently uses a derived key
`${alert.publishedBy}-${String(alert.publishedAt)}` which can collide; update
the mapping in App.tsx to use a stable immutable identifier (e.g., alert.id)
instead of the derived string so React can correctly reconcile the list; locate
where the alert is rendered (the element using
key={`${alert.publishedBy}-${String(alert.publishedAt)}`}) and replace that
expression with the dedicated unique field (alert.id), ensuring the data model
provides that id.
In `@functions/src/auth/account-lifecycle.ts`:
- Line 44: The call to suspendStaffAccountInputSchema.parse(request.data) should
be replaced with a safe parse pattern so Zod validation failures are mapped to
an 'invalid-argument' error instead of surfacing as internal errors; use
suspendStaffAccountInputSchema.safeParse(request.data), check result.success,
and when false throw a firebase/functions HttpsError (code 'invalid-argument')
with the validation error details (or a concise message) so the
suspendStaffAccount handler returns a proper client error on bad input.
- Line 18: Replace the direct call to
setStaffClaimsInputSchema.parse(request.data) with safe parsing: call
setStaffClaimsInputSchema.safeParse(request.data), check the .success flag, and
if false throw an HttpsError with code "invalid-argument" and include the Zod
validation error messages (e.g., join issues) in the error details; keep using
the same variable names (parsed or parsedResult) and ensure downstream code uses
parsedResult.data when success is true so existing logic (in this file's account
lifecycle handler) continues to work.
- Around line 54-61: suspendStaffAccount currently performs two separate writes
which can leave data inconsistent if the second write fails; change it to use a
single Firestore batch: create a WriteBatch (const batch = adminDb.batch()),
call batch.set(...) for adminDb.collection('active_accounts').doc(input.uid)
with the merged payload ({ ...current, accountStatus: 'suspended', updatedAt:
revokedAt }, { merge: true }) and batch.set(...) for
adminDb.collection('claim_revocations').doc(input.uid) with
buildClaimRevocationDoc(input.uid, revokedAt, input.reason), then await
batch.commit() so both writes succeed or both fail atomically.
In `@functions/src/auth/custom-claims.ts`:
- Around line 40-52: The returned object from buildActiveAccountDoc includes
municipalityId and agencyId even when undefined; change buildActiveAccountDoc to
conditionally add municipalityId and agencyId only when they are not undefined
(similar to buildStaffClaims), so the Firestore document doesn't get explicit
undefined fields; keep permittedMunicipalityIds defaulting to [] and still
include role, accountStatus, mfaEnrolled, lastClaimIssuedAt, lastClaimIssuedAt,
updatedAt and uid as before, but merge municipalityId and agencyId into the
result object only when claims.municipalityId !== undefined / claims.agencyId
!== undefined.
🪄 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: 3b0441f0-ea98-48d9-a994-c2c531521b56
📒 Files selected for processing (4)
apps/citizen-pwa/src/App.test.tsxapps/citizen-pwa/src/App.tsxfunctions/src/auth/account-lifecycle.tsfunctions/src/auth/custom-claims.ts
Comment on lines
+39
to
+64
| export const suspendStaffAccount = onCall(async (request) => { | ||
| if (request.auth?.token.role !== 'provincial_superadmin') { | ||
| throw new HttpsError('permission-denied', 'Only superadmins can suspend accounts.') | ||
| } | ||
|
|
||
| const input = suspendStaffAccountInputSchema.parse(request.data) | ||
| const snapshot = await adminDb.collection('active_accounts').doc(input.uid).get() | ||
|
|
||
| if (!snapshot.exists) { | ||
| throw new HttpsError('not-found', 'Active account record not found.') | ||
| } | ||
|
|
||
| const current = snapshot.data() ?? {} | ||
| const revokedAt = Date.now() | ||
|
|
||
| await adminDb | ||
| .collection('active_accounts') | ||
| .doc(input.uid) | ||
| .set({ ...current, accountStatus: 'suspended', updatedAt: revokedAt }, { merge: true }) | ||
| await adminDb | ||
| .collection('claim_revocations') | ||
| .doc(input.uid) | ||
| .set(buildClaimRevocationDoc(input.uid, revokedAt, input.reason)) | ||
|
|
||
| return { uid: input.uid, status: 'suspended' } | ||
| }) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for other places where accountStatus or setCustomUserClaims are modified
rg -n "accountStatus.*suspended|setCustomUserClaims" --type tsRepository: Exc1D/bantayog-alert
Length of output: 493
🏁 Script executed:
wc -l functions/src/auth/account-lifecycle.tsRepository: Exc1D/bantayog-alert
Length of output: 106
🏁 Script executed:
cat -n functions/src/auth/account-lifecycle.tsRepository: Exc1D/bantayog-alert
Length of output: 2525
🏁 Script executed:
fd -i custom-claims custom-claims.js functions/src/auth/Repository: Exc1D/bantayog-alert
Length of output: 179
🏁 Script executed:
cat -n functions/src/auth/custom-claims.jsRepository: Exc1D/bantayog-alert
Length of output: 131
🏁 Script executed:
cat -n functions/src/auth/custom-claims.tsRepository: Exc1D/bantayog-alert
Length of output: 2120
🏁 Script executed:
fd firestore.rulesRepository: Exc1D/bantayog-alert
Length of output: 142
🏁 Script executed:
rg -n "getUser|token.*refresh|accountStatus" --type ts functions/src/ | head -30Repository: Exc1D/bantayog-alert
Length of output: 1096
🏁 Script executed:
cat -n infra/firebase/firestore.rulesRepository: Exc1D/bantayog-alert
Length of output: 3170
🏁 Script executed:
cat -n functions/src/__tests__/firestore.rules.test.ts | head -150Repository: Exc1D/bantayog-alert
Length of output: 5307
🏁 Script executed:
rg -n "accountStatus" --type ts functions/src/ | grep -v test | grep -v custom-claims.ts | grep -v account-lifecycle.tsRepository: Exc1D/bantayog-alert
Length of output: 46
🏁 Script executed:
rg "token\.accountStatus|auth\.token.*accountStatus" --type tsRepository: Exc1D/bantayog-alert
Length of output: 46
Consider updating JWT claims on account suspension for consistency.
While Firestore security rules independently verify accountStatus from the database document via isActivePrivileged() (which checks the Firestore record, not JWT claims), the user's JWT token will still have accountStatus: 'active' until it expires or is refreshed. This creates a state mismatch between Auth claims and the actual account status in Firestore. Updating the custom claims here would ensure consistency and allow client-side code to rely on token state without fetching Firestore.
The proposed fix to call adminAuth.setCustomUserClaims() with the suspended status before the Firestore updates would be a reasonable quality improvement.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@functions/src/auth/account-lifecycle.ts` around lines 39 - 64,
suspendStaffAccount currently updates Firestore but doesn't update the user's
JWT custom claims, causing token/db state mismatch; call
adminAuth.setCustomUserClaims(input.uid, { accountStatus: 'suspended' }) before
writing to 'active_accounts' and 'claim_revocations' so tokens reflect
suspension, and ensure errors from setCustomUserClaims are handled (fail fast or
log and abort) to avoid partial updates; keep existing use of
buildClaimRevocationDoc and revoke timestamp (revokedAt) when ordering these
operations.
- Add id field to AlertDoc, use alert.id as React key in App.tsx - Fix progress.md: status now reflects incomplete verification with remediation note - App.test.tsx: single vitest import, exact text matching, added error-state and signed-out tests - functions/package.json: replace plain deploy with env-specific scripts (dev/staging/prod), plain deploy now prints guidance Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
This was referenced Apr 17, 2026
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
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Implements Phase 1 Infrastructure and Identity Spine as specified in the implementation plan.
Identity Contracts and Schemas
activeAccountSchema,claimRevocationSchema,setStaffClaimsInputSchema,suspendStaffAccountInputSchema)minAppVersionSchema,alertSchema)packages/shared-typesFirebase Web Bootstrap Package
createFirebaseWebApp,createAppCheck,ensurePseudonymousSignIn,subscribeAlerts,subscribeMinAppVersion)Phase 1 Firestore Rules
isAuthed(),isSuperadmin(),isActivePrivileged()system_config: authenticated read, superadmin+active writeactive_accounts/claim_revocations: self-only readClaims and Revocation Functions
setStaffClaims: superadmin callable to set custom claimssuspendStaffAccount: superadmin callable to suspend accountsCitizen PWA Shell
Test Commands
Verification Results
pnpm testpnpm --filter @bantayog/functions test:unitpnpm --filter @bantayog/functions test:rulespnpm typecheck && pnpm buildNote
Track A (Terraform staging infrastructure, Firebase wiring, seed execution) requires staging project access and is not included in this PR.
Summary by Sourcery
Introduce Phase 1 identity spine and citizen shell, including shared auth/config schemas, Firebase web bootstrap helpers, Cloud Functions for staff lifecycle, and basic alert + app version wiring across backend and PWA.
New Features:
Bug Fixes:
Enhancements:
Build:
Tests:
Summary by CodeRabbit