feat: frontend API client, React scaffold, and Vite dev server#5
Conversation
- createApiClient factory with injected getToken/baseUrl/fetcher for full testability without Firebase or Vite imports at module level - Firebase Web SDK init with connectAuthEmulator in DEV mode - React 19 app scaffold (App.tsx, main.tsx, index.html) - vite-env.d.ts reference for import.meta.env types under Bazel tsc - 3 Node native test runner tests covering auth header, URL, and error - frontend_lib ts_project and vite_binary dev_server in BUILD.bazel - vite bins declaration in MODULE.bazel npm_translate_lock
|
The following review focuses on the robustness and maintainability of the new frontend infrastructure. Great work on the test coverage for the API client and the migration to static environment variable access for production compatibility. Overall, the foundation for the React frontend is solid, and the testing strategy for the API client is a great pattern for the rest of the project. |
- Read response.text() on non-ok and append to error message so server validation details survive to the caller - Return undefined (not parse JSON) on 204 or content-length: 0 to avoid "Unexpected end of JSON input" from queue-worker trigger endpoints - Add two tests covering the new paths
- new URL(path, baseUrl) avoids double-slash / missing-slash edge cases - requireEnv() throws a clear actionable error at startup if any VITE_FIREBASE_* var is missing, instead of cryptic Firebase SDK errors
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| bazel run //backend:dev_server | ||
|
|
||
| # Terminal 3 — Vite-powered React frontend (not yet implemented) | ||
| # Terminal 3 — Vite-powered React frontend (http://localhost:5173) |
There was a problem hiding this comment.
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.
| 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.
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.
|
|
||
| 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"], |
There was a problem hiding this comment.
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.
| export const auth = getAuth(app); | ||
|
|
||
| if (import.meta.env.DEV) { | ||
| connectAuthEmulator(auth, 'http://localhost:9099', { disableWarnings: true }); |
There was a problem hiding this comment.
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.
| const detail = await response.text().catch(() => ''); | ||
| throw new Error( | ||
| `HTTP ${response.status}${detail ? `: ${detail}` : ''}`, |
There was a problem hiding this comment.
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.
| ], | ||
| entry_point = "index.test.js", | ||
| ) | ||
|
|
There was a problem hiding this comment.
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.
1 participant
Summary
createApiClient— a pure factory function with injectedgetToken,baseUrl, andfetcherso the HTTP layer is fully testable without Firebase or Vite at module levelconnectAuthEmulatorinDEVmode;import.meta.envaccess is confined tofirebase.tsand app-level callersApp.tsx,main.tsx,index.html) andvite.config.tswith sourcemapsfrontend/src/vite-env.d.ts(/// <reference types="vite/client" />) so Bazel's tsc recognisesimport.meta.envtypesvitebins inMODULE.bazelnpm_translate_lockto work around peer-dep version qualifier preventing auto-generation; exposesvite_binary(name = "dev_server")infrontend/BUILD.bazelTest plan
bazel test //frontend/...— all 3 tests passbazel test //...— full suite greenbazel run //:format— no formatting changesbazel run //frontend:dev_server— Vite starts on :5173 (requires.env.localfrom.env.local.example)