Skip to content

fix: serve built frontend in production node server#83

Merged
chitcommit merged 1 commit intomainfrom
fix/prod-static-serving
Apr 8, 2026
Merged

fix: serve built frontend in production node server#83
chitcommit merged 1 commit intomainfrom
fix/prod-static-serving

Conversation

@chitcommit
Copy link
Copy Markdown
Contributor

@chitcommit chitcommit commented Apr 8, 2026

Summary

  • The Hono dev migration (PR chore: migrate dev server to Hono, remove 15 legacy modules #81) broke npm start — the ASSETS stub always returned 404, so the built SPA was never served
  • Replace the stub with a real file server that reads dist/public/ and falls back to index.html for SPA routing when NODE_ENV=production
  • Dev mode unchanged (Vite handles frontend)

Test plan

  • npm run build && PORT=5015 NODE_ENV=production node dist/dev.js
  • curl localhost:5015/ → 200, HTML
  • curl localhost:5015/assets/*.css → 200, correct MIME
  • curl localhost:5015/dashboard → 200, SPA fallback
  • curl localhost:5015/health → 200, JSON
  • Dev mode (npm run dev) still returns 404 for / (Vite serves frontend)

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Bug Fixes
    • Improved static asset serving with proper MIME type detection and correct content-type headers.
    • Enhanced single-page application routing with fallback support for non-file routes.

The Hono dev migration (PR #81) broke `npm start` — the ASSETS stub
always returned 404, so the built SPA was never served. Replace the
stub with a real file server that reads dist/public/ and falls back
to index.html for SPA routing when NODE_ENV=production.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings April 8, 2026 04:41
@chatgpt-codex-connector
Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 8, 2026

@coderabbitai review

Please evaluate:

  • Security implications
  • Credential exposure risk
  • Dependency supply chain concerns
  • Breaking API changes

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Apr 8, 2026

📝 Walkthrough

Walkthrough

The server/dev.ts file's asset serving implementation was enhanced to conditionally serve static files from publicDir when running in production mode. It now includes MIME type inference, preloads index.html, and implements SPA fallback routing, while remaining a 404 stub in non-production environments.

Changes

Cohort / File(s) Summary
Asset Serving Enhancement
server/dev.ts
Updated stubAssets.fetch to serve static files from publicDir in production with MIME type detection and SPA fallback routing; remains a 404 stub in non-production environments.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

🐰 A dev server hops through the code,
Serving files down production's road,
With MIME types sniffed and HTML prepped,
When routes fall through, the index is stepped!
Thump thump — the assets now truly flow. 📦✨

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'fix: serve built frontend in production node server' directly and clearly summarizes the main change: enabling static asset serving for the built frontend in production mode, addressing the broken functionality from the Hono dev migration.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/prod-static-serving

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@cloudflare-workers-and-pages
Copy link
Copy Markdown

Deploying with  Cloudflare Workers  Cloudflare Workers

The latest updates on your project. Learn more about integrating Git with Workers.

Status Name Latest Commit Updated (UTC)
✅ Deployment successful!
View logs
chittyfinance 322712c Apr 08 2026, 04:42 AM

@claude
Copy link
Copy Markdown

claude bot commented Apr 8, 2026

Code Review

Good fix for the broken npm start regression from PR #81. The approach is sound — detecting production mode and switching from a 404 stub to a real file server. A few issues worth addressing before merging:


Critical: Path Traversal Vulnerability

const filePath = resolve(publicDir, url.pathname.replace(/^\//, ''));

resolve() does not constrain the result to publicDir. A request like GET /../../etc/passwd would produce a path outside dist/public/. Add a bounds check:

const filePath = resolve(publicDir, url.pathname.replace(/^\//, ''));
if (!filePath.startsWith(publicDir + '/') && filePath !== publicDir) {
  return new Response('Not found', { status: 404 });
}

Performance: Synchronous I/O on the Event Loop

readFileSync and statSync block the Node.js event loop. For a production server this means every static file request stalls all concurrent requests. Use the async variants:

import { readFile, stat } from 'node:fs/promises';

// in the fetch handler:
try {
  const stats = await stat(filePath);
  if (stats.isFile()) {
    const body = await readFile(filePath);
    // ...
  }
} catch {}

The top-level readFileSync for index.html is fine since it runs once at startup.


Missing Cache Headers

No cache-control header is set, so browsers will re-validate on every navigation. Assets in dist/public/assets/ typically have content-hashed filenames and can be cached aggressively:

const isAsset = url.pathname.startsWith('/assets/');
const headers = {
  'content-type': mime,
  'cache-control': isAsset ? 'public, max-age=31536000, immutable' : 'no-cache',
};

Minor: Silent catch {} Blocks

The empty catch on the startup readFileSync is fine for optional pre-loading, but it means a missing dist/public/index.html fails silently. A console.warn would save debugging time:

try {
  indexHtml = readFileSync(resolve(publicDir, 'index.html'), 'utf-8');
} catch (e) {
  console.warn('[ASSETS] index.html not found in', publicDir, '— SPA fallback disabled');
}

Minor: import.meta.dirname Fallback

import.meta.dirname ?? '.' falls back to CWD if undefined. Since this builds to dist/dev.js, running node dist/dev.js from the project root would make publicDir resolve to ./public (wrong) instead of dist/public/. This is fine as long as npm start sets up the environment correctly, but worth a comment.


Summary: The path traversal issue is the blocker — the rest are improvements. The fix is otherwise correct and well-scoped.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR restores npm start behavior after the Hono dev migration by making the Node dev/production entrypoint able to serve the built SPA from disk in production, while keeping dev behavior unchanged (Vite serves the frontend).

Changes:

  • Replaces the ASSETS 404 stub with a production file server for dist/public/ (SPA index.html fallback).
  • Adds MIME type detection for served static assets.
  • Keeps dev-mode behavior as a 404 stub for ASSETS.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@server/dev.ts`:
- Around line 13-14: The imports readFileSync, statSync, and resolve violate
edge-runtime rules; remove those Node core imports and replace their usage with
Web-compatible APIs: use new URL('./relative/path', import.meta.url) to resolve
paths instead of resolve, use fetch(...) to load file contents instead of
readFileSync, and derive metadata (size, mtime) from the Response or a dev HTTP
metadata endpoint instead of statSync; update any code referencing
readFileSync/statSync/resolve accordingly (search for readFileSync, statSync,
resolve in this module and replace with fetch + URL-based resolution and a small
metadata adapter).
- Around line 55-64: The file-serving code builds filePath from url.pathname
without checking for path traversal; update the handler that computes filePath
to validate the resolved path stays inside publicDir by resolving publicDir and
filePath (using realpath/resolve) and comparing (e.g., with path.relative or
startsWith) so if the resolved filePath is outside publicDir you return a
404/403 instead of reading the file; keep the existing statSync/readFileSync
logic but only run it after this containment check (refer to filePath, publicDir
and the async request handler).
- Line 46: The resolved publicDir is incorrect — change how publicDir is
computed so it points to the built assets at dist/public in production; update
the declaration that currently uses resolve(import.meta.dirname ?? '.',
'public') (and the publicDir variable used by the index read and file serving)
to resolve(import.meta.dirname ?? '.', 'dist', 'public') when isProduction is
true so production asset reads/serving use the correct directory.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 1e951623-fb96-4eda-9964-52d5f79befad

📥 Commits

Reviewing files that changed from the base of the PR and between d5399f1 and 322712c.

📒 Files selected for processing (1)
  • server/dev.ts

@chitcommit chitcommit enabled auto-merge (squash) April 8, 2026 06:08
@chitcommit chitcommit merged commit c9a946b into main Apr 8, 2026
23 checks passed
@chitcommit chitcommit deleted the fix/prod-static-serving branch April 8, 2026 06:09
chitcommit added a commit that referenced this pull request Apr 8, 2026
- Validate resolved file paths stay within publicDir (prevents traversal)
- Only return SPA fallback HTML for GET requests (POST/PUT → 404)

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
chitcommit added a commit that referenced this pull request Apr 8, 2026
fix: address PR #83 review — path traversal guard + SPA method check

- Validate resolved file paths stay within publicDir (prevents traversal)
- Only return SPA fallback HTML for GET requests (POST/PUT → 404)

Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants