Phase 0 — foundation and safety net - #1
Merged
Conversation
Tier 2, pulled forward because there was nothing at all — no ErrorBoundary, no componentDidCatch anywhere — so any error thrown during render unmounted the whole tree and left a white page with no way back. Bad anywhere; worst on a project that gets demonstrated live. Mounted outermost, above the router and every provider, so a throw inside any of them is caught too. Verified in a browser: a render-time null dereference now shows a branded page matching NotFound, logs "[error-boundary] app:" with the component stack, and Go Home recovers to a working landing page. The stack confirms the nesting — ErrorBoundary > BrowserRouter > QueryClientProvider > AuthProvider. Go Home is a plain href rather than a Link: the router lives inside the boundary, so a client-side navigation would re-render the same broken tree. Try again clears the error in place, which recovers a transient cause without losing the session. The error message renders only in development — in production it can carry internals a user cannot act on. This is not a substitute for handling errors where they happen. Screens that fetch already distinguish "empty" from "failed" — BUG-16 and UX-12/13 were the opposite mistake. This is the net under the errors nobody predicted. openapi.d.ts regenerated for the extended Health shape. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XYeFzET7gtfvEKMs47kshi
The frontend compiled with `strict` off, so strictNullChecks and noImplicitAny were never enforced. The generated contract types carry real nullability -- sellerRating, reserveMet, attributes -- and nothing made callers handle it. Verified before enabling: `tsc --strict --noEmit` reported zero errors, so this is a guardrail with no existing debt to pay down. Two control runs with stricter flags (24 and 3 errors) confirmed the check was real and not a no-op. BV-060. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Qn51EX4kuSFMjKXNY5jVDA
The suite fell back to a production hostname and .env.test.example pointed at a Vercel deployment -- and there is one backend and one database behind any deployed frontend. So `npm run test:e2e` on a fresh checkout signed three real accounts in and fired a deliberate wrong-password login at production. Read-only in effect today: the register test submits an empty form, the rating test only opens the modal. But the failed login is a real auth event against a real account, and BV-002 adds rate limiting and lockout next -- at which point running the suite would lock out the account the rest of it signs in with. - Default BASE_URL is http://localhost:5173, with a webServer block so a local run starts the dev server itself. - A known production host throws unless ALLOW_PRODUCTION_E2E=1 is passed in the command. Same shape as backend/tests/setup.ts, which has had this guard since the shared-Redis incident. - The wrong-password test uses a throwaway address. - Local .env.test had its deployed BASE_URL commented out. Verified: deployed target refused; refused target passes with the override; default lists 58 tests against localhost. BV-061. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Qn51EX4kuSFMjKXNY5jVDA
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
🟡 Changes recommended
The production-host refusal guard in playwright.config.ts is currently case-sensitive and can be bypassed with mixed-case hostnames, undermining the safety net.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Phase 0 of the audit remediation plan: strengthens TypeScript type-safety and reduces operational risk in Playwright e2e by defaulting to local targets and adding a production-host safeguard, plus adds a top-level React error boundary for render-failure recovery.
Changes:
- Enable TypeScript
strictmode in both app and node tsconfig projects. - Add a top-level
ErrorBoundaryand wrap the entire app tree to prevent blank-screen failures on render errors. - Make Playwright e2e default to
http://localhost:5173, start/reuse the dev server automatically, and refuse known production hosts unless explicitly overridden; adjust.env.test.exampleand the wrong-password login test accordingly.
File summaries
| File | Description |
|---|---|
| tsconfig.node.json | Enables strict compiler checks for node-side TS (e.g., Vite config). |
| tsconfig.app.json | Enables strict compiler checks for the React app TS project. |
| src/types/openapi.d.ts | Extends the Health contract type with version/commit/dependency status fields. |
| src/components/ErrorBoundary.tsx | Introduces a React error boundary with a recovery UI. |
| src/App.tsx | Wraps the entire app (router + providers) in the new error boundary. |
| playwright.config.ts | Defaults e2e to localhost, adds production-host refusal gate, and starts local dev server for local runs. |
| e2e/auth.spec.ts | Avoids wrong-password attempts against a real account by using a throwaway email. |
| .env.test.example | Updates test env guidance to prefer local runs and document the production-host guard. |
Review details
- Files reviewed: 7/8 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+21
to
+23
| const PRODUCTION_HOSTS = /(vercel\.app|railway\.app|bidvault\.tech)/; | ||
| if (PRODUCTION_HOSTS.test(BASE_URL) && process.env.ALLOW_PRODUCTION_E2E !== '1') { | ||
| throw new Error( |
Comment on lines
60
to
+65
| export default function App() { | ||
| // ErrorBoundary sits outermost, above the router and every provider: a throw inside any | ||
| // of them would otherwise blank the page with nothing rendered to recover from. | ||
| return ( | ||
| <BrowserRouter> | ||
| <QueryClientProvider client={queryClient}> | ||
| <AuthProvider> | ||
| <ListingProvider> | ||
| <ToastProvider> | ||
| <RealtimeBridge /> | ||
| <ToastContainer /> | ||
| {/* Layout convention: page content max-width is max-w-5xl (1024px) for most screens. */} | ||
| {/* BuyerLiveBidding uses max-w-[1100px] due to two-column layout. Do not change these per-screen. */} | ||
| <Suspense | ||
| fallback={ | ||
| <div className="min-h-screen bg-bg flex items-center justify-center"> | ||
| <div className="w-8 h-8 rounded-full border-2 border-border border-t-primary animate-spin" /> | ||
| </div> | ||
| } | ||
| > | ||
| <Routes> | ||
| <Route path="/" element={<LandingPage />} /> | ||
| <Route path="/privacy" element={<PrivacyPolicy />} /> | ||
| <Route path="/terms" element={<TermsOfService />} /> | ||
| <Route path="/maintenance" element={<MaintenancePage />} /> | ||
| <ErrorBoundary area="app"> | ||
| <BrowserRouter> |
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
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.
Phase 0 of the audit remediation plan (
IMPLEMENTATION_PLAN.md). No user-visible behaviour changes.Pairs with subhanlone/bidvault-backend#1.
What's here
strictwas off in both project configs, sostrictNullChecksandnoImplicitAnywere never enforced — while the generated contract types carry real nullability (sellerRating,reserveMet,attributes). Verified before enabling:tsc --strict --noEmitreported zero errors, so this is a guardrail with no debt to pay down. Two control runs with stricter flags (24 and 3 errors) confirmed the check was real..env.test.examplepointed at a Vercel deployment — and there is one backend and one database behind any deployed frontend. Sonpm run test:e2eon a fresh checkout signed three real accounts in and fired a deliberate wrong-password login at production.Why BV-061 matters now rather than later
Read-only in effect today: the register test submits an empty form, the rating test only opens the modal. But the failed login is a real auth event against a real account — and BV-002 adds rate limiting and lockout next, at which point running the suite would lock out the account the rest of it signs in with.
Now defaults to
http://localhost:5173with awebServerblock, and throws on a known production host unlessALLOW_PRODUCTION_E2E=1is passed in the command. Same shape asbackend/tests/setup.ts, which has had that guard since the shared-Redis incident.Verification
🤖 Generated with Claude Code
https://claude.ai/code/session_01Qn51EX4kuSFMjKXNY5jVDA