-
Notifications
You must be signed in to change notification settings - Fork 0
feat(phase-3a): citizen submission + triptych materialization #43
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 22 commits
918eac0
95c82c5
d6b87ab
73d5975
161ecaa
b3fbbef
4c0d19c
d64b335
63e0001
6ff1feb
9c415fe
0cd1c83
d96e4f1
93e0c6a
8fdfd1a
98e5481
06b8b18
a072b71
372f047
768b912
8de412c
97ada0c
e02c179
ad7fe3a
8affc8a
e5071fe
57e5e15
e78701e
6f57a0b
b276424
bba292a
9e52f48
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| 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() | ||
| }) | ||
| }) | ||
| 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 /> | ||
| } |
| 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> { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chain🌐 Web query:
💡 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 @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 -40Repository: 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 Line 23 casts 🤖 Prompt for AI Agents
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Outside scope —
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
🧠 Learnings used
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
🐇 🗒️ ✏️ Learnings added
🧠 Learnings used |
||
| 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
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
rg -nP --type=tsx 'httpsCallable|as LookupResult|safeParse|\\.parse\\(' apps/citizen-pwa/src/components/LookupScreen.tsxRepository: Exc1D/bantayog-alert Length of output: 136 🏁 Script executed: cat -n apps/citizen-pwa/src/components/LookupScreen.tsx | head -50Repository: Exc1D/bantayog-alert Length of output: 1744 🏁 Script executed: rg -n "httpsCallable" apps/citizen-pwa/src/components/LookupScreen.tsx -A 5 -B 2Repository: Exc1D/bantayog-alert Length of output: 559 🏁 Script executed: rg -n "as LookupResult" apps/citizen-pwa/src/components/LookupScreen.tsx -B 3 -A 3Repository: Exc1D/bantayog-alert Length of output: 339 🏁 Script executed: rg -n "LookupResult" apps/citizen-pwa/src/ -t tsx -t ts --max-count=20Repository: Exc1D/bantayog-alert Length of output: 91 🏁 Script executed: rg -n "safeParse\|\.parse" apps/citizen-pwa/src/components/LookupScreen.tsxRepository: Exc1D/bantayog-alert Length of output: 46 Add runtime validation for external API response before setting UI state. Line 23 casts untrusted Per defensive programming guidelines: validate external input at the boundary. 🤖 Prompt for AI Agents
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Outside scope — Same as above.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
🧠 Learnings used
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
( ´・ω・)ノ 🧠 Learnings used |
||
| } 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> | ||
| ) | ||
| } | ||
| 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
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chain🌐 Web query:
💡 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 ( 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 safePer coding guidelines: validate external input at boundaries and assume external data is broken. 🤖 Prompt for AI Agents
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Outside scope —
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
🧠 Learnings used
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Would you like me to open a follow-up GitHub issue to track adding runtime validation of ✏️ Learnings added
🧠 Learnings used |
||
| 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'll notify you when we can. For now, check back with your reference number via the{' '} | ||
| <Link to="/lookup">lookup page</Link>. | ||
| </p> | ||
| </section> | ||
| ) | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replace trivial smoke assertion with behavior verification.
Line 8 can still pass if
Apprenders nothing useful; this weakens regression protection.Suggested test change
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Outside scope —
App.test.tsxsmoke test improvements are PWA-level, not Phase 3a.There was a problem hiding this comment.
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 theApp.test.tsxbehavior-verification improvement so it isn't lost?🧠 Learnings used
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 theAppcomponent entirely throughAppRoutes, 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.tsxbehavior-verification (i.e., assertingAppRoutesdelegation) as a Phase 4 task? That way it won't slip through the cracks when the PWA testing pass begins.🧠 Learnings used