Skip to content
Merged
Show file tree
Hide file tree
Changes from 26 commits
Commits
Show all changes
32 commits
Select commit Hold shift + click to select a range
918eac0
feat(shared-validators): add municipalityLabel and correlationId to R…
claude Apr 18, 2026
95c82c5
feat(shared-validators): add publicRef, secretHash, correlationId to …
claude Apr 18, 2026
d6b87ab
feat(shared-validators): add tokenHash to ReportLookup and Municipali…
claude Apr 18, 2026
73d5975
feat(shared-validators): add state-machine transition tables (Task 4)
claude Apr 18, 2026
161ecaa
feat(shared-validators): add BantayogError and structured logEvent he…
claude Apr 18, 2026
b3fbbef
feat(infra): add firestore.rules template and codegen script (Task 6)
claude Apr 18, 2026
4c0d19c
fix(rules): correct reporterUid typo + add CI drift-check gate
claude Apr 18, 2026
d64b335
feat(functions): add requestUploadUrl callable
claude Apr 18, 2026
63e0001
feat(functions): add requestLookup callable
claude Apr 18, 2026
6ff1feb
feat(functions): add municipality lookup service with cold-start cache
claude Apr 18, 2026
9c415fe
feat(process-inbox-item): add inbox processing trigger and integratio…
claude Apr 18, 2026
0cd1c83
test(process-inbox-item): add failure-mode tests for schema invalid,
claude Apr 18, 2026
d96e4f1
feat(functions): add onMediaFinalize EXIF-strip + MIME-check trigger
claude Apr 18, 2026
93e0c6a
feat(phase-3a): integrate pending media migration into processInboxItem
claude Apr 18, 2026
8fdfd1a
feat(functions): pre-wire dormant onMediaRelocate for Phase 5
claude Apr 18, 2026
98e5481
feat(functions): scheduled inboxReconciliationSweep every 5 minutes
claude Apr 18, 2026
06b8b18
feat(citizen-pwa): scaffold routes and Firebase client init
claude Apr 18, 2026
a072b71
feat(citizen-pwa): submit-report orchestrator and SubmitReportForm
claude Apr 18, 2026
372f047
feat(terraform): Phase 3 monitoring module (metrics + alerts)
claude Apr 18, 2026
768b912
docs(phase-3a): extend rule-coverage gate, add pending_media rules, r…
claude Apr 18, 2026
8de412c
fix(phase-3a): resolve all critical integration test failures
claude Apr 18, 2026
97ada0c
fix(phase-3a): add replayed semantic, fix monitoring filters, accepta…
claude Apr 18, 2026
e02c179
merge: resolve conflict in reports.test.ts, keep Phase 3 + baseline t…
claude Apr 18, 2026
ad7fe3a
fix(phase-3a): address PR review comments from CodeRabbit, Sourcery, …
claude Apr 18, 2026
8affc8a
fix(citizen-pwa): add explicit firebase dependency to fix CI typechec…
claude Apr 18, 2026
e5071fe
fix(scripts): remove unused CAMARINES_NORTE_MUNICIPALITIES import
claude Apr 18, 2026
57e5e15
fix(citizen-pwa): handle geolocation errors with try-catch in SubmitR…
claude Apr 18, 2026
e78701e
fix(test): align test name with FORBIDDEN assertion in request-lookup…
claude Apr 18, 2026
6f57a0b
fix(https-error): make error code mapping exhaustive and fix media fi…
claude Apr 18, 2026
b276424
fix(logging): add event field for monitoring filter backward compat
claude Apr 18, 2026
bba292a
fix(idempotency): return fromCache to distinguish fresh vs replayed m…
claude Apr 18, 2026
9e52f48
fix(terraform): update error metric filter to capture v2 Cloud Run logs
claude Apr 18, 2026
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,13 @@ jobs:
- run: corepack prepare pnpm@${PNPM_VERSION} --activate
- run: pnpm install --frozen-lockfile
- run: pnpm exec tsx scripts/check-rule-coverage.ts
- run: pnpm exec tsx scripts/build-rules.ts
- name: Verify firestore.rules is not out of date
run: |
if ! git diff --exit-code -- infra/firebase/firestore.rules; then
echo "::error::firestore.rules is out of sync with scripts/build-rules.ts. Run 'pnpm exec tsx scripts/build-rules.ts' locally and commit."
exit 1
fi
build:
name: Build
Expand Down
4 changes: 3 additions & 1 deletion apps/citizen-pwa/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,10 @@
"@bantayog/shared-firebase": "workspace:*",
"@bantayog/shared-types": "workspace:*",
"@bantayog/shared-ui": "workspace:*",
"firebase": "^12.12.0",
"react": "^19.2.5",
"react-dom": "^19.2.5"
"react-dom": "^19.2.5",
"react-router-dom": "^7.14.1"
},
"devDependencies": {
"@testing-library/jest-dom": "^6.4.0",
Expand Down
73 changes: 4 additions & 69 deletions apps/citizen-pwa/src/App.test.tsx
Original file line number Diff line number Diff line change
@@ -1,75 +1,10 @@
import '@testing-library/jest-dom/vitest'
import { describe, it, expect, vi } from 'vitest'
import { render, screen } from '@testing-library/react'
import { describe, it, expect } from 'vitest'
import { render } from '@testing-library/react'
import { App } from './App.js'

const { useCitizenShell } = vi.hoisted(() => {
const defaultShellState = {
status: 'ready',
authState: 'signed-in',
appCheckState: 'active',
user: { uid: 'anon-123' },
minAppVersion: {
citizen: '0.1.0',
admin: '0.1.0',
responder: '0.1.0',
updatedAt: 1713350400000,
},
alerts: [
{
id: 'phase1-hello',
title: 'System online',
body: 'Citizen shell wired for Phase 1.',
severity: 'info',
publishedAt: 1713350400000,
publishedBy: 'phase-1-bootstrap',
},
],
error: null,
}
return {
useCitizenShell: vi.fn().mockReturnValue(defaultShellState),
}
})

vi.mock('./useCitizenShell.js', () => ({
useCitizenShell,
}))

describe('App', () => {
it('renders auth status, app version, and the hello-world alert feed', () => {
render(<App />)
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()
})

it('renders error message when status is error', () => {
useCitizenShell.mockReturnValueOnce({
status: 'error',
authState: 'signed-out',
appCheckState: 'failed',
user: null,
minAppVersion: null,
alerts: [],
error: 'Firebase initialization failed',
})
render(<App />)
expect(screen.getByText('Firebase initialization failed')).toBeInTheDocument()
})

it('renders signed-out state correctly', () => {
useCitizenShell.mockReturnValueOnce({
status: 'ready',
authState: 'signed-out',
appCheckState: 'pending',
user: null,
minAppVersion: null,
alerts: [],
error: null,
})
render(<App />)
expect(screen.getByText('signed-out')).toBeInTheDocument()
it('renders without throwing', () => {
expect(() => render(<App />)).not.toThrow()
Comment on lines +7 to +8
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Apr 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Replace trivial smoke assertion with behavior verification.

Line 8 can still pass if App renders nothing useful; this weakens regression protection.

Suggested test change
-import { describe, it, expect } from 'vitest'
+import { describe, it, expect, vi } from 'vitest'
 import { render } from '@testing-library/react'
 import { App } from './App.js'
+import { AppRoutes } from './routes.js'
+
+vi.mock('./routes.js', () => ({
+  AppRoutes: vi.fn(() => <div data-testid="app-routes" />),
+}))
 
 describe('App', () => {
-  it('renders without throwing', () => {
-    expect(() => render(<App />)).not.toThrow()
+  it('delegates rendering to AppRoutes', () => {
+    render(<App />)
+    expect(vi.mocked(AppRoutes)).toHaveBeenCalledTimes(1)
   })
 })

As per coding guidelines: **/*.{test,spec}.{js,ts,tsx,jsx}: “Write tests that verify the new code is actually invoked, not tests that pass trivially.”

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@apps/citizen-pwa/src/App.test.tsx` around lines 7 - 8, The current test
"renders without throwing" only checks that rendering doesn't crash (using
render(<App />)) which is too trivial; update the test to assert observable
behavior from the App component instead: import screen from
`@testing-library/react`, render(<App />) and then verify a meaningful element
produced by App (e.g. screen.getByRole('main') or screen.getByText('<expected
text>') or a specific test-id) and assert it is in the document
(expect(...).toBeInTheDocument()). Replace the trivial expect(() => render(<App
/>)).not.toThrow() with a concrete assertion that references the App render
output.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Outside scope — App.test.tsx smoke test improvements are PWA-level, not Phase 3a.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Exc1D, understood — I'll defer this to a dedicated PWA-quality pass. Would you like me to open a follow-up GitHub issue to track the App.test.tsx behavior-verification improvement so it isn't lost?


🧠 Learnings used
Learnt from: CR
Repo: Exc1D/bantayog-alert PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-17T16:23:10.558Z
Learning: Applies to **/*.{test,spec}.{js,ts,tsx,jsx} : Mock setup exceeding ~20 lines indicates the test is testing mocks, not behavior; consider integration test with emulators instead

Learnt from: CR
Repo: Exc1D/bantayog-alert PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-16T01:00:31.384Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : TDD means: write ONE failing test, RUN it, see it fail with a meaningful error, THEN implement. Write tests that verify the new code is actually invoked, not tests that pass trivially.

Learnt from: CR
Repo: Exc1D/bantayog-alert PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-16T01:00:31.384Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Mocks are a smell budget. If a unit test needs more than ~20 lines of mock setup, consider integration test with emulators instead.

Learnt from: CR
Repo: Exc1D/bantayog-alert PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-16T01:00:31.384Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Use `vi.hoisted` when a mock function needs per-test `mockImplementationOnce`/`mockRejectedValueOnce` to avoid cross-test contamination.

Learnt from: CR
Repo: Exc1D/bantayog-alert PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-17T16:23:10.558Z
Learning: Applies to **/*.{test,spec}.{js,ts,tsx,jsx} : Write failing tests first (red-green-refactor): write ONE failing test, RUN it, see it fail with meaningful error, THEN implement

Learnt from: CR
Repo: Exc1D/bantayog-alert PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-16T01:00:31.384Z
Learning: Applies to **/*.{ts,tsx,js,jsx} : VERIFY OR IT DIDN'T HAPPEN. Every edit must be followed by tests, linting, and type-checking. Never ignore warnings.

Learnt from: CR
Repo: Exc1D/bantayog-alert PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-17T16:23:10.558Z
Learning: Applies to **/*.{test,spec}.{js,ts,tsx,jsx} : Use `vi.hoisted` when a mock function needs per-test `mockImplementationOnce` or `mockRejectedValueOnce`

Learnt from: CR
Repo: Exc1D/bantayog-alert PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-17T16:23:10.558Z
Learning: When tests pass, confirm they actually ran the new code; a test that passes without invoking the change under test is worse than a failing test

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📝 Nitpick - acknowledged. The App.tsx change (simplified to AppRoutes) is Phase 3a scope; the test reduction is a consequence of that simplification. Full PWA test coverage is Phase 4 scope.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Exc1D, that context is helpful — noted that the test simplification is a deliberate consequence of routing the App component entirely through AppRoutes, and that behavioral coverage for the PWA layer is deferred to Phase 4.

Would you like me to open a follow-up GitHub issue to track App.test.tsx behavior-verification (i.e., asserting AppRoutes delegation) as a Phase 4 task? That way it won't slip through the cracks when the PWA testing pass begins.


🧠 Learnings used
Learnt from: CR
Repo: Exc1D/bantayog-alert PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-17T16:23:10.558Z
Learning: Applies to **/*.{test,spec}.{js,ts,tsx,jsx} : Mock setup exceeding ~20 lines indicates the test is testing mocks, not behavior; consider integration test with emulators instead

Learnt from: Exc1D
Repo: Exc1D/bantayog-alert PR: 43
File: functions/src/__tests__/callables/request-upload-url.test.ts:11-12
Timestamp: 2026-04-18T09:02:17.913Z
Learning: In `functions/src/__tests__/**/*.test.ts` (and similar test files in the Exc1D/bantayog-alert repo), `as any` casts and accompanying `// eslint-disable-next-line typescript-eslint/no-explicit-any` comments on Firebase SDK mock objects (e.g., `getSignedUrl`, `getStorage`, bucket/file stubs) are intentional and required for Firebase emulator compatibility. Do not flag these as violations of the no-`any` guideline; they are acceptable in test/mock contexts without a ticket reference.

Learnt from: CR
Repo: Exc1D/bantayog-alert PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-16T01:00:31.384Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Mocks are a smell budget. If a unit test needs more than ~20 lines of mock setup, consider integration test with emulators instead.

Learnt from: CR
Repo: Exc1D/bantayog-alert PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-16T01:00:31.384Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : TDD means: write ONE failing test, RUN it, see it fail with a meaningful error, THEN implement. Write tests that verify the new code is actually invoked, not tests that pass trivially.

Learnt from: CR
Repo: Exc1D/bantayog-alert PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-16T01:00:31.384Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Use `vi.hoisted` when a mock function needs per-test `mockImplementationOnce`/`mockRejectedValueOnce` to avoid cross-test contamination.

Learnt from: CR
Repo: Exc1D/bantayog-alert PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-16T01:00:31.384Z
Learning: Applies to **/*.{ts,tsx,js,jsx} : VERIFY OR IT DIDN'T HAPPEN. Every edit must be followed by tests, linting, and type-checking. Never ignore warnings.

Learnt from: CR
Repo: Exc1D/bantayog-alert PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-16T01:00:31.384Z
Learning: Applies to **/*.{ts,tsx,js,jsx} : SMALLEST SAFE CHANGE. Touch ≤3 files and write ≤50 lines per task unless explicitly asked to perform a wider refactor.

Learnt from: CR
Repo: Exc1D/bantayog-alert PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-17T16:23:10.558Z
Learning: When tests pass, confirm they actually ran the new code; a test that passes without invoking the change under test is worse than a failing test

Learnt from: Exc1D
Repo: Exc1D/bantayog-alert PR: 43
File: apps/citizen-pwa/src/services/firebase.ts:42-46
Timestamp: 2026-04-18T09:02:11.329Z
Learning: In `apps/citizen-pwa/src/services/firebase.ts`, the Cloud Functions region `'asia-southeast1'` passed to `getFunctions()` in the `fns()` helper is intentionally hardcoded for Phase 3 of the Bantayog Alert project. Do not flag this as a hardcoded value or suggest extracting it to an environment variable; it is a deliberate design decision for this phase.

})
})
52 changes: 2 additions & 50 deletions apps/citizen-pwa/src/App.tsx
Original file line number Diff line number Diff line change
@@ -1,53 +1,5 @@
import styles from './App.module.css'
import { useCitizenShell } from './useCitizenShell.js'
import { AppRoutes } from './routes.js'

export function App() {
const state = useCitizenShell()

return (
<main className={styles.page}>
<section className={styles.panel}>
<p className={styles.eyebrow}>Bantayog Alert</p>
<h1 className={styles.title}>Citizen Phase 1 shell</h1>
<p className={styles.summary}>
Pseudonymous sign-in, app health, and a hello-world alert feed.
</p>

<dl className={styles.meta}>
<div>
<dt>Status</dt>
<dd>{state.status}</dd>
</div>
<div>
<dt>Auth</dt>
<dd>{state.authState}</dd>
</div>
<div>
<dt>App Check</dt>
<dd>{state.appCheckState}</dd>
</div>
<div>
<dt>User UID</dt>
<dd>{state.user?.uid ?? 'unavailable'}</dd>
</div>
<div>
<dt>Minimum citizen version</dt>
<dd>{state.minAppVersion?.citizen ?? 'unavailable'}</dd>
</div>
</dl>

{state.error ? <p className={styles.error}>{state.error}</p> : null}

<div className={styles.feed}>
{state.alerts.map((alert) => (
<article key={alert.id} className={styles.alert}>
<h2>{alert.title}</h2>
<p>{alert.body}</p>
<span>{alert.severity}</span>
</article>
))}
</div>
</section>
</main>
)
return <AppRoutes />
}
72 changes: 72 additions & 0 deletions apps/citizen-pwa/src/components/LookupScreen.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,72 @@
import { useState } from 'react'
import { httpsCallable } from 'firebase/functions'
import { fns } from '../services/firebase.js'

interface LookupResult {
status: string
lastStatusAt: number
municipalityLabel: string
}

export function LookupScreen() {
const [publicRef, setPublicRef] = useState('')
const [secret, setSecret] = useState('')
const [result, setResult] = useState<LookupResult | null>(null)
const [error, setError] = useState<string | null>(null)

async function handleSubmit(e: React.SubmitEvent): Promise<void> {
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Apr 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🌐 Web query:

For React 19 TypeScript typings, is React.SubmitEventa valid type, or should form submit handlers useReact.FormEvent?

💡 Result:

For React 19 TypeScript typings, React.SubmitEvent is a valid type and is the recommended replacement for the deprecated React.FormEvent. Form submit handlers should use React.SubmitEvent or SubmitEventHandler instead of React.FormEvent. In @types/react for React 19 (starting v19.2.10), FormEvent is deprecated with the note: "FormEvent doesn't actually exist. You probably meant to use ChangeEvent, InputEvent, SubmitEvent, or just SyntheticEvent instead depending on the event type." SubmitEvent<T = Element> extends SyntheticEvent<T, NativeSubmitEvent> with target: EventTarget & HTMLFormElement. Examples: const handleSubmit = (e: React.SubmitEvent) => { e.preventDefault; // ... }; or const handleSubmit: React.SubmitEventHandler = (e) => { e.preventDefault; // ... };

FormEvent still works but triggers deprecation warnings, especially with linters like @typescript-eslint/no-deprecated-declarations. Official React docs do not specify TypeScript types but emphasize action prop over onSubmit for new form patterns.

Citations:


🏁 Script executed:

# Check if the file exists and read the relevant section
cat -n apps/citizen-pwa/src/components/LookupScreen.tsx | head -40

Repository: Exc1D/bantayog-alert

Length of output: 1457


🏁 Script executed:

# Check the React version in package.json to confirm which version is being used
cat package.json | jq '.dependencies.react, .devDependencies.react, .devDependencies["@types/react"]'

Repository: Exc1D/bantayog-alert

Length of output: 78


Validate external callable response before casting to LookupResult.

Line 23 casts res.data without validation. The response from httpsCallable is external data that must be validated at the boundary before casting. Add runtime validation (e.g., schema check or guard clause) to ensure the response matches the expected LookupResult structure, preventing type breakage if the server response is malformed or incomplete.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@apps/citizen-pwa/src/components/LookupScreen.tsx` at line 17, In
handleSubmit, don't directly cast external res.data to LookupResult; add a
runtime validation/guard that checks required fields and types on res.data
(e.g., ensure properties like id, name, and any expected subfields exist and
have correct types) before casting; return or throw an error / set an error
state if validation fails. Use a small helper/guard (e.g.,
isValidLookupResult(obj)) or a schema validator to centralize checks, and only
after it returns true assign/res.cast to LookupResult and proceed with existing
logic.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Outside scope — LookupScreen runtime validation is PWA-level.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Exc1D, understood — acknowledged and moving on. Feel free to track the runtime validation as a follow-up if desired.


🧠 Learnings used
Learnt from: CR
Repo: Exc1D/bantayog-alert PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-16T01:00:31.384Z
Learning: Applies to **/*.{ts,tsx,js,jsx} : Defensive Programming: Assume external input is malicious/broken. Validate at the boundary. Never swallow errors with an empty catch block.

Learnt from: CR
Repo: Exc1D/bantayog-alert PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-17T16:23:10.558Z
Learning: Applies to **/*.{js,ts,tsx,jsx} : Use defensive programming: validate external input at boundaries and never swallow errors with empty catch blocks

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📝 This is about API design - the current implementation is intentionally minimal (Phase 3a scope). The LookupScreen has no complex validation that would benefit from schema parsing. Accepted pattern for future complexity.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Exc1D, noted — the minimal cast in LookupScreen is intentional for Phase 3a, with runtime validation deferred to a later phase. I'll record this for future reviews.

🐇 🗒️


✏️ Learnings added
Learnt from: Exc1D
Repo: Exc1D/bantayog-alert PR: 43
File: apps/citizen-pwa/src/components/LookupScreen.tsx:17-17
Timestamp: 2026-04-18T09:22:20.784Z
Learning: In `apps/citizen-pwa/src/components/LookupScreen.tsx`, the `res.data` from `httpsCallable` is cast directly to `LookupResult` without runtime validation. This is intentional for Phase 3a (minimal scope). Runtime schema validation of the callable response is a deferred/accepted pattern for future complexity. Do not flag this as a missing boundary validation in the current phase.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.


🧠 Learnings used
Learnt from: CR
Repo: Exc1D/bantayog-alert PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-16T01:00:31.384Z
Learning: Applies to **/*.{ts,tsx,js,jsx} : Defensive Programming: Assume external input is malicious/broken. Validate at the boundary. Never swallow errors with an empty catch block.

Learnt from: CR
Repo: Exc1D/bantayog-alert PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-17T16:23:10.558Z
Learning: Applies to **/*.{js,ts,tsx,jsx} : Use defensive programming: validate external input at boundaries and never swallow errors with empty catch blocks

Learnt from: Exc1D
Repo: Exc1D/bantayog-alert PR: 43
File: functions/src/__tests__/callables/request-upload-url.test.ts:11-12
Timestamp: 2026-04-18T09:02:17.913Z
Learning: In `functions/src/__tests__/**/*.test.ts` (and similar test files in the Exc1D/bantayog-alert repo), `as any` casts and accompanying `// eslint-disable-next-line typescript-eslint/no-explicit-any` comments on Firebase SDK mock objects (e.g., `getSignedUrl`, `getStorage`, bucket/file stubs) are intentional and required for Firebase emulator compatibility. Do not flag these as violations of the no-`any` guideline; they are acceptable in test/mock contexts without a ticket reference.

Learnt from: Exc1D
Repo: Exc1D/bantayog-alert PR: 43
File: apps/citizen-pwa/src/components/SubmitReportForm.tsx:9-13
Timestamp: 2026-04-18T09:02:11.245Z
Learning: In `apps/citizen-pwa/src/components/SubmitReportForm.tsx`, the `randomPublicRef()` function uses `b % alphabet.length` (alphabet of 36 chars, 256 not evenly divisible by 36), which introduces slight modulo bias. This is intentional and acceptable — `publicRef` is a non-cryptographic, human-readable 8-character tracking reference, not used for security-sensitive purposes. Do not flag this as a bug or suggest rejection sampling.

Learnt from: Exc1D
Repo: Exc1D/bantayog-alert PR: 43
File: functions/src/services/geocode.ts:8-12
Timestamp: 2026-04-18T09:02:09.317Z
Learning: In `functions/src/services/geocode.ts`, the local `MunicipalityDoc` interface intentionally keeps `centroid` as optional (`centroid?: GeoPoint`) rather than importing `MunicipalityDoc` from `bantayog/shared-validators`, because the shared type requires `centroid` but Firestore documents in the `municipalities` collection may not always have it (e.g. legacy/incomplete entries). Do not suggest replacing this local interface with the shared type.

Learnt from: CR
Repo: Exc1D/bantayog-alert PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-16T01:00:31.384Z
Learning: Applies to **/*.{ts,tsx,js,jsx} : ASK, DON'T ASSUME. Ambiguity = stop and escalate to the user.

Learnt from: Exc1D
Repo: Exc1D/bantayog-alert PR: 43
File: apps/citizen-pwa/src/services/firebase.ts:42-46
Timestamp: 2026-04-18T09:02:11.329Z
Learning: In `apps/citizen-pwa/src/services/firebase.ts`, the Cloud Functions region `'asia-southeast1'` passed to `getFunctions()` in the `fns()` helper is intentionally hardcoded for Phase 3 of the Bantayog Alert project. Do not flag this as a hardcoded value or suggest extracting it to an environment variable; it is a deliberate design decision for this phase.

e.preventDefault()
setError(null)
setResult(null)
try {
const res = await httpsCallable(fns(), 'requestLookup')({ publicRef, secret })
setResult(res.data as LookupResult)
Comment on lines +22 to +23
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Apr 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
rg -nP --type=tsx 'httpsCallable|as LookupResult|safeParse|\\.parse\\(' apps/citizen-pwa/src/components/LookupScreen.tsx

Repository: Exc1D/bantayog-alert

Length of output: 136


🏁 Script executed:

cat -n apps/citizen-pwa/src/components/LookupScreen.tsx | head -50

Repository: Exc1D/bantayog-alert

Length of output: 1744


🏁 Script executed:

rg -n "httpsCallable" apps/citizen-pwa/src/components/LookupScreen.tsx -A 5 -B 2

Repository: Exc1D/bantayog-alert

Length of output: 559


🏁 Script executed:

rg -n "as LookupResult" apps/citizen-pwa/src/components/LookupScreen.tsx -B 3 -A 3

Repository: Exc1D/bantayog-alert

Length of output: 339


🏁 Script executed:

rg -n "LookupResult" apps/citizen-pwa/src/ -t tsx -t ts --max-count=20

Repository: Exc1D/bantayog-alert

Length of output: 91


🏁 Script executed:

rg -n "safeParse\|\.parse" apps/citizen-pwa/src/components/LookupScreen.tsx

Repository: Exc1D/bantayog-alert

Length of output: 46


Add runtime validation for external API response before setting UI state.

Line 23 casts untrusted res.data directly to LookupResult without runtime validation. If the API returns an object missing required properties (status, lastStatusAt, municipalityLabel), the type system won't catch it at runtime, causing silent shape drift and potential UI errors. Add a schema validator or type guard (e.g., using Zod, io-ts, or a custom guard function) before calling setResult.

Per defensive programming guidelines: validate external input at the boundary.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@apps/citizen-pwa/src/components/LookupScreen.tsx` around lines 22 - 23, The
response from httpsCallable(fns(), 'requestLookup') is untrusted but is cast
directly to LookupResult and passed to setResult; add runtime validation of
res.data (e.g., with Zod/io-ts or a custom type guard) to ensure it contains
required fields like status, lastStatusAt, and municipalityLabel before calling
setResult; if validation fails, handle it (log/report and do not call setResult
or set an error state) and keep references to the involved symbols
(requestLookup, res.data, LookupResult, setResult) so the check is placed
immediately before the existing setResult call.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Outside scope — Same as above.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Exc1D, understood — I'll drop this for the current PR. Would you like me to open a follow-up GitHub issue to track adding runtime validation (e.g., a Zod schema guard) for the requestLookup response in LookupScreen.tsx before it's passed to setResult?


🧠 Learnings used
Learnt from: CR
Repo: Exc1D/bantayog-alert PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-16T01:00:31.384Z
Learning: Applies to **/*.{ts,tsx,js,jsx} : Defensive Programming: Assume external input is malicious/broken. Validate at the boundary. Never swallow errors with an empty catch block.

Learnt from: CR
Repo: Exc1D/bantayog-alert PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-17T16:23:10.558Z
Learning: Applies to **/*.{js,ts,tsx,jsx} : Use defensive programming: validate external input at boundaries and never swallow errors with empty catch blocks

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📝 Acknowledged. For Phase 3a, the simple type assertion is acceptable since the backend callable contract is controlled. If validation is needed later, zod parsing can be added.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Exc1D, noted — I'll keep that in mind for future reviews of this file.

(  ´・ω・)ノ


🧠 Learnings used
Learnt from: CR
Repo: Exc1D/bantayog-alert PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-16T01:00:31.384Z
Learning: Applies to **/*.{ts,tsx,js,jsx} : Defensive Programming: Assume external input is malicious/broken. Validate at the boundary. Never swallow errors with an empty catch block.

Learnt from: CR
Repo: Exc1D/bantayog-alert PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-17T16:23:10.558Z
Learning: Applies to **/*.{js,ts,tsx,jsx} : Use defensive programming: validate external input at boundaries and never swallow errors with empty catch blocks

Learnt from: Exc1D
Repo: Exc1D/bantayog-alert PR: 43
File: functions/src/__tests__/callables/request-upload-url.test.ts:11-12
Timestamp: 2026-04-18T09:02:17.913Z
Learning: In `functions/src/__tests__/**/*.test.ts` (and similar test files in the Exc1D/bantayog-alert repo), `as any` casts and accompanying `// eslint-disable-next-line typescript-eslint/no-explicit-any` comments on Firebase SDK mock objects (e.g., `getSignedUrl`, `getStorage`, bucket/file stubs) are intentional and required for Firebase emulator compatibility. Do not flag these as violations of the no-`any` guideline; they are acceptable in test/mock contexts without a ticket reference.

Learnt from: Exc1D
Repo: Exc1D/bantayog-alert PR: 43
File: apps/citizen-pwa/src/components/SubmitReportForm.tsx:9-13
Timestamp: 2026-04-18T09:02:11.245Z
Learning: In `apps/citizen-pwa/src/components/SubmitReportForm.tsx`, the `randomPublicRef()` function uses `b % alphabet.length` (alphabet of 36 chars, 256 not evenly divisible by 36), which introduces slight modulo bias. This is intentional and acceptable — `publicRef` is a non-cryptographic, human-readable 8-character tracking reference, not used for security-sensitive purposes. Do not flag this as a bug or suggest rejection sampling.

Learnt from: CR
Repo: Exc1D/bantayog-alert PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-16T01:00:31.384Z
Learning: Applies to **/*.{ts,tsx,js,jsx} : ASK, DON'T ASSUME. Ambiguity = stop and escalate to the user.

Learnt from: Exc1D
Repo: Exc1D/bantayog-alert PR: 43
File: apps/citizen-pwa/src/services/firebase.ts:42-46
Timestamp: 2026-04-18T09:02:11.329Z
Learning: In `apps/citizen-pwa/src/services/firebase.ts`, the Cloud Functions region `'asia-southeast1'` passed to `getFunctions()` in the `fns()` helper is intentionally hardcoded for Phase 3 of the Bantayog Alert project. Do not flag this as a hardcoded value or suggest extracting it to an environment variable; it is a deliberate design decision for this phase.

} catch (e: unknown) {
setError(e instanceof Error ? e.message : 'lookup failed')
}
}

return (
<section aria-label="Report status lookup">
<h1>Check report status</h1>
<form
onSubmit={(e) => {
void handleSubmit(e)
}}
>
<label>
Reference
<input
value={publicRef}
onChange={(e) => {
setPublicRef(e.target.value)
}}
required
/>
</label>
<label>
Secret
<input
value={secret}
onChange={(e) => {
setSecret(e.target.value)
}}
required
/>
</label>
<button type="submit">Look up</button>
</form>
{error && <p role="alert">{error}</p>}
{result && (
<dl>
<dt>Status</dt>
<dd>{result.status}</dd>
<dt>Municipality</dt>
<dd>{result.municipalityLabel}</dd>
<dt>Last update</dt>
<dd>{new Date(result.lastStatusAt).toLocaleString()}</dd>
</dl>
)}
</section>
)
}
26 changes: 26 additions & 0 deletions apps/citizen-pwa/src/components/ReceiptScreen.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
import { useLocation, Link } from 'react-router-dom'

export function ReceiptScreen() {
const { state } = useLocation() as { state: { publicRef: string; secret: string } | null }
if (!state) return <p>No submission to display.</p>
Comment on lines +4 to +5
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Apr 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🌐 Web query:

In react-router-dom v7 + TypeScript, what is the recommended safe pattern to validate location.stateshape at runtime instead of asserting withas?

💡 Result:

In react-router-dom v7 with TypeScript, the recommended safe pattern to validate location.state shape at runtime (instead of using as type assertion) is to use a custom type guard function. This provides runtime validation while maintaining TypeScript type safety through type narrowing. Define your expected state interface and a type guard: interface MyState { userId: string; redirectPath: string; } function isMyState(state: unknown): state is MyState { return ( state != null && typeof state === 'object' && 'userId' in state && typeof (state as any).userId === 'string' && 'redirectPath' in state && typeof (state as any).redirectPath === 'string' ); } Then use it with useLocation: import { useLocation } from 'react-router'; function MyComponent { const location = useLocation; const state = location.state; if (isMyState(state)) { // TypeScript now knows state is MyState console.log(state.userId, state.redirectPath); // Safe to use } else { // Handle invalid/missing state console.log('No valid state found'); } } For more robust validation, integrate a schema library like Zod: import { z } from 'zod'; const MyStateSchema = z.object({ userId: z.string, redirectPath: z.string, }); type MyState = z.infer; function validateMyState(state: unknown): state is MyState { return MyStateSchema.safeParse(state).success; } This pattern is widely recommended in TypeScript/React Router discussions. Official docs confirm location.state types as State = any/unknown by default, with no built-in generic for useLocation or automatic validation, making manual runtime checks essential since state can be undefined (direct navigation) or arbitrary data. Note: location.state is unknown/any and often undefined, so always check existence first. Avoid global state typing hacks; use per-component guards for safety.

Citations:


🏁 Script executed:

fd -t f "ReceiptScreen.tsx" --exec cat -n {} \;

Repository: Exc1D/bantayog-alert

Length of output: 1040


Add runtime validation of router state shape before rendering receipt data.

Line 4 uses a type assertion (as) to cast useLocation() without runtime validation. This allows malformed state objects to pass the !state guard—for example, { state: { someField: "value" } } would be truthy but lack publicRef and secret, causing undefined values to render on lines 13 and 17.

Replace the assertion with a runtime type guard or schema validator (e.g., Zod):

const validateState = (state: unknown): state is { publicRef: string; secret: string } => {
  return (
    state != null &&
    typeof state === 'object' &&
    'publicRef' in state &&
    'secret' in state &&
    typeof (state as any).publicRef === 'string' &&
    typeof (state as any).secret === 'string'
  );
};

const { state: rawState } = useLocation();
if (!validateState(rawState)) return <p>No submission to display.</p>;
const state = rawState; // Now TypeScript knows state is safe

Per coding guidelines: validate external input at boundaries and assume external data is broken.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@apps/citizen-pwa/src/components/ReceiptScreen.tsx` around lines 4 - 5,
Replace the unsafe type assertion on useLocation in ReceiptScreen.tsx with a
runtime type guard: create a validateState(state: unknown): state is {
publicRef: string; secret: string } that checks non-null object shape and string
types for publicRef and secret, call const { state: rawState } = useLocation(),
return the "No submission to display." UI if validateState(rawState) is false,
and then assign const state = rawState so the rest of the component (where
publicRef and secret are used) is type-safe at runtime.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Outside scope — ReceiptScreen type guard is PWA-level.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Exc1D, understood — I'll defer this one. Would you like me to open a follow-up GitHub issue to track adding runtime validation of location.state in ReceiptScreen.tsx for a future PR?


🧠 Learnings used
Learnt from: CR
Repo: Exc1D/bantayog-alert PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-16T01:00:31.384Z
Learning: Applies to **/*.{ts,tsx,js,jsx} : Defensive Programming: Assume external input is malicious/broken. Validate at the boundary. Never swallow errors with an empty catch block.

Learnt from: CR
Repo: Exc1D/bantayog-alert PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-17T16:23:10.558Z
Learning: Applies to **/*.{js,ts,tsx,jsx} : Use defensive programming: validate external input at boundaries and never swallow errors with empty catch blocks

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📝 Acknowledged. ReceiptScreen is a placeholder with the 'as' cast for now. Runtime validation via pattern matching can be added when the receipt flow is expanded in a future phase.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Exc1D, noted — I'll remember that ReceiptScreen.tsx is a deliberate placeholder for the current phase and the as cast on useLocation() is deferred until the receipt flow is expanded.

Would you like me to open a follow-up GitHub issue to track adding runtime validation of location.state shape in ReceiptScreen.tsx?


✏️ Learnings added
Learnt from: Exc1D
Repo: Exc1D/bantayog-alert PR: 43
File: apps/citizen-pwa/src/components/ReceiptScreen.tsx:4-5
Timestamp: 2026-04-18T09:22:24.533Z
Learning: In `apps/citizen-pwa/src/components/ReceiptScreen.tsx`, the `as { state: { publicRef: string; secret: string } | null }` type assertion on `useLocation()` (Line 4) is intentional — `ReceiptScreen` is a placeholder component for Phase 3a. Runtime validation of `location.state` via a type guard or Zod schema is deferred until the receipt flow is expanded in a future phase. Do not flag this `as` cast as a missing runtime boundary check.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.


🧠 Learnings used
Learnt from: CR
Repo: Exc1D/bantayog-alert PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-16T01:00:31.384Z
Learning: Applies to **/*.{ts,tsx,js,jsx} : Defensive Programming: Assume external input is malicious/broken. Validate at the boundary. Never swallow errors with an empty catch block.

Learnt from: CR
Repo: Exc1D/bantayog-alert PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-17T16:23:10.558Z
Learning: Applies to **/*.{js,ts,tsx,jsx} : Use defensive programming: validate external input at boundaries and never swallow errors with empty catch blocks

Learnt from: Exc1D
Repo: Exc1D/bantayog-alert PR: 43
File: functions/src/__tests__/callables/request-upload-url.test.ts:11-12
Timestamp: 2026-04-18T09:02:17.913Z
Learning: In `functions/src/__tests__/**/*.test.ts` (and similar test files in the Exc1D/bantayog-alert repo), `as any` casts and accompanying `// eslint-disable-next-line typescript-eslint/no-explicit-any` comments on Firebase SDK mock objects (e.g., `getSignedUrl`, `getStorage`, bucket/file stubs) are intentional and required for Firebase emulator compatibility. Do not flag these as violations of the no-`any` guideline; they are acceptable in test/mock contexts without a ticket reference.

Learnt from: Exc1D
Repo: Exc1D/bantayog-alert PR: 43
File: apps/citizen-pwa/src/components/SubmitReportForm.tsx:9-13
Timestamp: 2026-04-18T09:02:11.245Z
Learning: In `apps/citizen-pwa/src/components/SubmitReportForm.tsx`, the `randomPublicRef()` function uses `b % alphabet.length` (alphabet of 36 chars, 256 not evenly divisible by 36), which introduces slight modulo bias. This is intentional and acceptable — `publicRef` is a non-cryptographic, human-readable 8-character tracking reference, not used for security-sensitive purposes. Do not flag this as a bug or suggest rejection sampling.

Learnt from: CR
Repo: Exc1D/bantayog-alert PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-17T16:23:10.558Z
Learning: Applies to **/*.{js,ts,tsx,jsx} : Never use `any`, `// ts-ignore`, or `TODO` without explicit permission or a ticket reference

Learnt from: CR
Repo: Exc1D/bantayog-alert PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-16T01:00:31.384Z
Learning: Applies to **/*.{ts,tsx,js,jsx} : Do not use `any`, `// ts-ignore`, or `TODO` without explicit permission or a ticket reference.

Learnt from: Exc1D
Repo: Exc1D/bantayog-alert PR: 43
File: apps/citizen-pwa/src/services/firebase.ts:42-46
Timestamp: 2026-04-18T09:02:11.329Z
Learning: In `apps/citizen-pwa/src/services/firebase.ts`, the Cloud Functions region `'asia-southeast1'` passed to `getFunctions()` in the `fns()` helper is intentionally hardcoded for Phase 3 of the Bantayog Alert project. Do not flag this as a hardcoded value or suggest extracting it to an environment variable; it is a deliberate design decision for this phase.

return (
<section aria-label="Submission receipt">
<h1>Report submitted</h1>
<p>Save these two values. You will need them to check status.</p>
<dl>
<dt>Reference</dt>
<dd>
<code>{state.publicRef}</code>
</dd>
<dt>Secret</dt>
<dd>
<code>{state.secret}</code>
</dd>
</dl>
<p>
We&apos;ll notify you when we can. For now, check back with your reference number via the{' '}
<Link to="/lookup">lookup page</Link>.
</p>
</section>
)
}
Loading
Loading