-
Notifications
You must be signed in to change notification settings - Fork 0
feat: frontend API client, React scaffold, and Vite dev server #5
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 all commits
bbccf50
bc6e6d8
404b635
7496086
bd6b13b
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,29 +1,81 @@ | ||
| load("@aspect_rules_js//js:defs.bzl", "js_test") | ||
| load("@aspect_rules_ts//ts:defs.bzl", "ts_project") | ||
| load("@npm_rulesjs//:vite/package_json.bzl", vite_bin = "bin") | ||
|
|
||
| ts_project( | ||
| name = "frontend", | ||
| srcs = glob( | ||
| ["*.ts"], | ||
| exclude = ["*.test.ts"], | ||
| exclude = [ | ||
| "*.test.ts", | ||
| "vite.config.ts", | ||
| ], | ||
| ), | ||
| declaration = True, | ||
| transpiler = "tsc", | ||
| tsconfig = "tsconfig.json", | ||
| visibility = ["//visibility:public"], | ||
|
Comment on lines
4
to
17
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. As noted in previous feedback, the frontend target (and the accompanying frontend/index.ts) appears to be a redundant shell now that the source logic has moved into src/ and frontend_lib. We should remove this target and the index.ts file to keep the Bazel build graph clean and avoid developer confusion. |
||
| ) | ||
|
|
||
| ts_project( | ||
| name = "frontend_lib", | ||
| srcs = glob( | ||
| [ | ||
| "src/**/*.ts", | ||
| "src/**/*.tsx", | ||
| ], | ||
| exclude = ["src/**/*.test.*"], | ||
| ), | ||
| declaration = True, | ||
| transpiler = "tsc", | ||
| tsconfig = "tsconfig.json", | ||
| visibility = ["//visibility:public"], | ||
| deps = [ | ||
| "//:node_modules/@types/react", | ||
| "//:node_modules/@types/react-dom", | ||
| "//:node_modules/firebase", | ||
| "//:node_modules/react", | ||
| "//:node_modules/react-dom", | ||
| "//:node_modules/vite", | ||
| ], | ||
| ) | ||
|
|
||
| ts_project( | ||
| name = "tests_lib", | ||
| srcs = ["index.test.ts"], | ||
| declaration = True, | ||
| transpiler = "tsc", | ||
| tsconfig = "tsconfig.json", | ||
| deps = ["//:node_modules/@types/node"], | ||
| deps = [ | ||
| ":frontend_lib", | ||
| "//:node_modules/@types/node", | ||
| ], | ||
| ) | ||
|
|
||
| js_test( | ||
| name = "test", | ||
| data = [":tests_lib"], | ||
| data = [ | ||
| ":frontend_lib", | ||
| ":tests_lib", | ||
| ], | ||
| entry_point = "index.test.js", | ||
| ) | ||
|
|
||
|
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. The dev_server target includes glob(["src/**"]) in its data. Since frontend_lib also depends on these files, this is slightly redundant, though it ensures Vite has access to the raw .tsx files for transformation. It's acceptable as-is but worth a note for future refactors of the frontend build rules. |
||
| vite_bin.vite_binary( | ||
| name = "dev_server", | ||
| args = [ | ||
| "--port=5173", | ||
| "--host", | ||
| ], | ||
| chdir = package_name(), | ||
| data = [ | ||
| "index.html", | ||
| "tsconfig.json", | ||
| "vite.config.ts", | ||
| ":frontend_lib", | ||
| "//:node_modules/@vitejs/plugin-react", | ||
| "//:node_modules/firebase", | ||
| "//:node_modules/react", | ||
| "//:node_modules/react-dom", | ||
| ] + glob(["src/**"]), | ||
| ) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,12 @@ | ||
| <!DOCTYPE html> | ||
| <html lang="en"> | ||
| <head> | ||
| <meta charset="UTF-8" /> | ||
| <meta name="viewport" content="width=device-width, initial-scale=1.0" /> | ||
| <title>Septima</title> | ||
| </head> | ||
| <body> | ||
| <div id="root"></div> | ||
| <script type="module" src="/src/main.tsx"></script> | ||
| </body> | ||
| </html> |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,95 @@ | ||
| import test from 'node:test'; | ||
| import assert from 'node:assert'; | ||
| import test from 'node:test'; | ||
| import { createApiClient } from './src/apiClient.js'; | ||
|
|
||
| test('attaches Authorization Bearer header', async () => { | ||
| let capturedInit: RequestInit | undefined; | ||
| const mockFetch = async ( | ||
| _url: string | URL | Request, | ||
| init?: RequestInit, | ||
| ): Promise<Response> => { | ||
| capturedInit = init; | ||
| return new Response(JSON.stringify({}), { | ||
| status: 200, | ||
| headers: { 'Content-Type': 'application/json' }, | ||
| }); | ||
| }; | ||
|
|
||
| const client = createApiClient( | ||
| async () => 'test-token', | ||
| 'http://localhost:8080', | ||
| mockFetch as typeof fetch, | ||
| ); | ||
| await client.post('/ping'); | ||
|
|
||
| const headers = capturedInit?.headers as Record<string, string>; | ||
| assert.strictEqual(headers['Authorization'], 'Bearer test-token'); | ||
| }); | ||
|
|
||
| test('sends POST to the correct URL', async () => { | ||
| let capturedUrl: string | URL | Request = ''; | ||
| const mockFetch = async ( | ||
| url: string | URL | Request, | ||
| _init?: RequestInit, | ||
| ): Promise<Response> => { | ||
| capturedUrl = url; | ||
| return new Response(JSON.stringify({}), { | ||
| status: 200, | ||
| headers: { 'Content-Type': 'application/json' }, | ||
| }); | ||
| }; | ||
|
|
||
| const client = createApiClient( | ||
| async () => 'token', | ||
| 'http://localhost:8080', | ||
| mockFetch as typeof fetch, | ||
| ); | ||
| await client.post('/ping'); | ||
| assert.strictEqual(capturedUrl, 'http://localhost:8080/ping'); | ||
| }); | ||
|
|
||
| test('throws on non-ok response with status code', async () => { | ||
| const mockFetch = async (): Promise<Response> => | ||
| new Response('', { status: 401 }); | ||
| const client = createApiClient( | ||
| async () => 'token', | ||
| 'http://localhost:8080', | ||
| mockFetch as typeof fetch, | ||
| ); | ||
| await assert.rejects( | ||
| () => client.post('/ping'), | ||
| (err: Error) => { | ||
| assert.match(err.message, /HTTP 401/); | ||
| return true; | ||
| }, | ||
| ); | ||
| }); | ||
|
|
||
| test('includes server error body in thrown error', async () => { | ||
| const mockFetch = async (): Promise<Response> => | ||
| new Response('Unauthorized: token expired', { status: 401 }); | ||
| const client = createApiClient( | ||
| async () => 'token', | ||
| 'http://localhost:8080', | ||
| mockFetch as typeof fetch, | ||
| ); | ||
| await assert.rejects( | ||
| () => client.post('/ping'), | ||
| (err: Error) => { | ||
| assert.match(err.message, /token expired/); | ||
| return true; | ||
| }, | ||
| ); | ||
| }); | ||
|
|
||
| test('Simple Test', () => { | ||
| assert.strictEqual(true, true); | ||
| test('returns undefined for 204 No Content', async () => { | ||
| const mockFetch = async (): Promise<Response> => | ||
| new Response(null, { status: 204 }); | ||
| const client = createApiClient( | ||
| async () => 'token', | ||
| 'http://localhost:8080', | ||
| mockFetch as typeof fetch, | ||
| ); | ||
| const result = await client.post('/trigger'); | ||
| assert.strictEqual(result, undefined); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| export function App() { | ||
| return <div>Septima</div>; | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,27 @@ | ||
| export function createApiClient( | ||
| getToken: () => Promise<string>, | ||
| baseUrl: string, | ||
| fetcher: typeof fetch = fetch, | ||
| ) { | ||
| return { | ||
| async post<T>(path: string, body?: unknown): Promise<T> { | ||
| const token = await getToken(); | ||
| const response = await fetcher(new URL(path, baseUrl).toString(), { | ||
|
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. The new URL(path, baseUrl) constructor has a well-known pitfall regarding leading and trailing slashes. If baseUrl is http://api/v1 (no trailing slash), v1 is dropped. If path is /ping (leading slash), the entire path of baseUrl is replaced by the root. Given our reliance on environment variables for baseUrl, this is highly likely to cause silent misconfigurations. We should either sanitize the inputs (e.g., baseUrl.replace(//$/, '') + '/' + path.replace(/^//, '')) or use a robust joining utility. |
||
| method: 'POST', | ||
| headers: { | ||
| Authorization: `Bearer ${token}`, | ||
| 'Content-Type': 'application/json', | ||
| }, | ||
| body: body != null ? JSON.stringify(body) : undefined, | ||
| }); | ||
| if (!response.ok) { | ||
| const detail = await response.text().catch(() => ''); | ||
| throw new Error( | ||
| `HTTP ${response.status}${detail ? `: ${detail}` : ''}`, | ||
|
Comment on lines
+18
to
+20
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. In the error path, response.text() is used to provide detail. If the server (or a proxy) returns a large HTML error page instead of a short string, the thrown Error message could become unwieldy. Consider truncating the detail string to a reasonable length (e.g., 200 characters) before including it in the error message. |
||
| ); | ||
| } | ||
| const text = await response.text(); | ||
| return (text ? JSON.parse(text) : undefined) as T; | ||
| }, | ||
| }; | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,32 @@ | ||
| import { initializeApp } from 'firebase/app'; | ||
| import { connectAuthEmulator, getAuth } from 'firebase/auth'; | ||
|
|
||
| function requireEnv(key: string, value: string | undefined): string { | ||
| if (!value) | ||
| throw new Error( | ||
| `Missing required env var: ${key}. Copy frontend/.env.local.example to frontend/.env.local and fill in the values.`, | ||
| ); | ||
| return value; | ||
| } | ||
|
|
||
| const firebaseConfig = { | ||
| apiKey: requireEnv( | ||
| 'VITE_FIREBASE_API_KEY', | ||
| import.meta.env.VITE_FIREBASE_API_KEY, | ||
| ), | ||
| authDomain: requireEnv( | ||
| 'VITE_FIREBASE_AUTH_DOMAIN', | ||
| import.meta.env.VITE_FIREBASE_AUTH_DOMAIN, | ||
| ), | ||
| projectId: requireEnv( | ||
| 'VITE_FIREBASE_PROJECT_ID', | ||
| import.meta.env.VITE_FIREBASE_PROJECT_ID, | ||
| ), | ||
| }; | ||
|
|
||
| export const app = initializeApp(firebaseConfig); | ||
| export const auth = getAuth(app); | ||
|
|
||
| if (import.meta.env.DEV) { | ||
| connectAuthEmulator(auth, 'http://localhost:9099', { disableWarnings: true }); | ||
|
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. The Firebase Auth emulator URL remains hardcoded to http://localhost:9099. While this is the default, our README.md already suggests using FIREBASE_AUTH_EMULATOR_HOST. We should make this configurable via a VITE_ environment variable to ensure developers in containerized or custom environments can connect their frontend to the local services. |
||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,9 @@ | ||
| import { StrictMode } from 'react'; | ||
| import { createRoot } from 'react-dom/client'; | ||
| import { App } from './App'; | ||
|
|
||
| createRoot(document.getElementById('root')!).render( | ||
| <StrictMode> | ||
| <App /> | ||
| </StrictMode>, | ||
| ); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| /// <reference types="vite/client" /> |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,9 @@ | ||
| import react from '@vitejs/plugin-react'; | ||
| import { defineConfig } from 'vite'; | ||
|
|
||
| export default defineConfig({ | ||
| plugins: [react()], | ||
| build: { | ||
| sourcemap: true, | ||
| }, | ||
| }); |
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.
The PR description still mentions "3 Node native test runner tests," but the implementation (and the previous audit) correctly identifies 5 tests in frontend/index.test.ts. Please update the summary to reflect the excellent additional coverage.