Fix blank UI bootstrapping and production static delivery#4
Fix blank UI bootstrapping and production static delivery#4ChrisAdamsdevelopment wants to merge 1 commit into
Conversation
Reviewer's GuideMounts the React frontend into the DOM, introduces a global Tailwind-based stylesheet for consistent dark-theme styling, and enhances the Node/Express server to support robust multi-origin CORS configuration and production static SPA delivery from the Vite dist bundle. Sequence diagram for CORS handling and SPA static deliverysequenceDiagram
participant Browser
participant ExpressApp
participant CORSConfig
participant StaticDist
participant SPAFallback
participant APIHandlers
Browser->>ExpressApp: GET /
ExpressApp->>StaticDist: Serve index.html from dist
StaticDist-->>Browser: index.html + assets
Browser->>Browser: Initialize JS bundle
Browser->>Browser: createRoot(rootElement).render(App)
Browser->>ExpressApp: GET /api/health
ExpressApp->>CORSConfig: Validate origin against configuredOrigins
CORSConfig-->>ExpressApp: Origin allowed or error
alt Origin allowed
ExpressApp->>APIHandlers: Handle /api/health
APIHandlers-->>Browser: { status: ok, time: ... }
else Origin blocked
ExpressApp-->>Browser: CORS error response
end
Browser->>ExpressApp: GET /non_api_route
ExpressApp->>SPAFallback: Route * (non /api)
SPAFallback-->>Browser: dist/index.html
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The global error-handling middleware currently appears before the new static file / SPA fallback routes, so any errors thrown in those handlers will bypass the centralized error handler; consider moving the error middleware to the very end of the middleware chain.
- The CORS origin callback now throws an Error for disallowed origins, which will surface as a generic 500 response via the global error handler; consider instead returning a controlled 4xx response (or using the
corspackage’s options callback style to signal rejection) so clients get a clearer CORS failure rather than an internal error.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The global error-handling middleware currently appears before the new static file / SPA fallback routes, so any errors thrown in those handlers will bypass the centralized error handler; consider moving the error middleware to the very end of the middleware chain.
- The CORS origin callback now throws an Error for disallowed origins, which will surface as a generic 500 response via the global error handler; consider instead returning a controlled 4xx response (or using the `cors` package’s options callback style to signal rejection) so clients get a clearer CORS failure rather than an internal error.
## Individual Comments
### Comment 1
<location path="server.js" line_range="128-132" />
<code_context>
+
app.use(cors({
- origin: ALLOWED_ORIGIN,
+ origin: (origin, cb) => {
+ if (!origin || configuredOrigins.includes('*') || configuredOrigins.includes(origin)) {
+ return cb(null, true);
+ }
+ return cb(new Error(`CORS blocked for origin: ${origin}`));
+ },
methods: ['GET', 'POST'],
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Consider returning `cb(null, false)` instead of an Error for disallowed CORS origins.
Using an Error here will likely produce a 500-style response and may hit the global error handler, instead of just denying the request. It can also expose internal error messages to clients depending on error serialization. Returning `cb(null, false)` ensures disallowed origins are treated as expected CORS denials rather than server errors.
Suggested implementation:
```javascript
app.use(cors({
origin: (origin, cb) => {
if (!origin || configuredOrigins.includes('*') || configuredOrigins.includes(origin)) {
return cb(null, true);
}
return cb(null, false);
},
methods: ['GET', 'POST'],
```
If you want to log blocked origins for observability without turning them into server errors, you can add a logging call (e.g. `console.warn` or your logger) before `cb(null, false)` within the same `origin` function.
</issue_to_address>
### Comment 2
<location path="server.js" line_range="556" />
<code_context>
app.use((err, req, res, _next) => {
</code_context>
<issue_to_address>
**issue (bug_risk):** New routes and static middleware are being registered after the global error handler, which can prevent errors from those routes from hitting this handler.
Express error handlers are order-dependent and should be registered last so any `next(err)` from routes or middleware reaches them. Please move this global error handler below the `dist` static middleware and `/api/health` route (including the catch‑all `app.get('*', ...)`) so those paths use the same centralized error handling.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| origin: (origin, cb) => { | ||
| if (!origin || configuredOrigins.includes('*') || configuredOrigins.includes(origin)) { | ||
| return cb(null, true); | ||
| } | ||
| return cb(new Error(`CORS blocked for origin: ${origin}`)); |
There was a problem hiding this comment.
suggestion (bug_risk): Consider returning cb(null, false) instead of an Error for disallowed CORS origins.
Using an Error here will likely produce a 500-style response and may hit the global error handler, instead of just denying the request. It can also expose internal error messages to clients depending on error serialization. Returning cb(null, false) ensures disallowed origins are treated as expected CORS denials rather than server errors.
Suggested implementation:
app.use(cors({
origin: (origin, cb) => {
if (!origin || configuredOrigins.includes('*') || configuredOrigins.includes(origin)) {
return cb(null, true);
}
return cb(null, false);
},
methods: ['GET', 'POST'],If you want to log blocked origins for observability without turning them into server errors, you can add a logging call (e.g. console.warn or your logger) before cb(null, false) within the same origin function.
| ); | ||
|
|
||
| const distPath = path.join(__dirname, 'dist'); | ||
| if (fs.existsSync(distPath)) { |
There was a problem hiding this comment.
issue (bug_risk): New routes and static middleware are being registered after the global error handler, which can prevent errors from those routes from hitting this handler.
Express error handlers are order-dependent and should be registered last so any next(err) from routes or middleware reaches them. Please move this global error handler below the dist static middleware and /api/health route (including the catch‑all app.get('*', ...)) so those paths use the same centralized error handling.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4fa578c004
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| origin: (origin, cb) => { | ||
| if (!origin || configuredOrigins.includes('*') || configuredOrigins.includes(origin)) { | ||
| return cb(null, true); | ||
| } | ||
| return cb(new Error(`CORS blocked for origin: ${origin}`)); | ||
| }, |
There was a problem hiding this comment.
Returning cb(new Error(...)) here turns a CORS mismatch into an application error, so requests from any origin not in configuredOrigins now fail with a 500 before reaching route handlers. This is a production regression for deployments that rely on defaults (e.g., serving dist from this same server while FRONTEND_URL remains http://localhost:5173), because browser POST calls include an Origin header and will be rejected as internal errors. CORS rejections should be handled as policy decisions (e.g., cb(null, false)) rather than server exceptions.
Useful? React with 👍 / 👎.
1 participant
Superseded by PR #8, which stabilized the root frontend build, React mount, Tailwind/global CSS, and backend static SPA serving from latest main.