You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
PR Review: fix(kitchen-sink): serve built frontend from server
This PR adds SPA static-file serving to the kitchen-sink server: at startup it checks for a dist/ directory (emitted by a Vite build), mounts serveStatic to serve its assets, and wires a custom fallback handler that serves index.html for non-asset paths.
Bug — SPA fallback breaks routes with dots in path segments
The heuristic that treats any final path segment containing . as an "asset" returns 404 for SPA routes like /users/john.doe, /v1.0/dashboard, or any route with a dot in a path parameter — even though the SPA shell should be returned. Vite already fingerprints its output files (/assets/vendor-AbCd1234.js), so there is a simpler approach: serveStatic already calls next() when it cannot find a file. Replacing the entire hand-rolled fallback with the established pattern used elsewhere in this repo removes the heuristic entirely:
The 8-line custom handler (URL parsing, dot heuristic, readFileSync at startup) reimplements what serveStatic({ root: distDir, path: "/index.html" }) does natively. The one-liner form is used in every other server example in the repo (state-render, stream-render, etc.) and has no edge-case bugs. Recommend switching to the canonical form.
Simplification — new URL(c.req.url).pathname vs c.req.path
examples/kitchen-sink/src/server.ts:178
Hono's HonoRequest exposes c.req.path which is already the parsed pathname — no URL object allocation needed. (Resolved automatically if the SPA fallback is replaced as above.)
Minor convention — First comment line explains WHAT, not WHY
examples/kitchen-sink/src/server.ts:170
// Serve the built frontend when it is present. The Vite build emits `dist/`,
// which only exists in production images, so dev runs skip this branch.
Per CLAUDE.md: "Don't explain WHAT the code does." The first sentence is redundant with the code. The second sentence (the WHY) is the useful one — drop the first and keep: // The Vite build emits dist/ only in production images, so dev runs skip this branch.
Overall: The approach is sound — guard on existsSync, register static middleware before the SPA fallback. The main actionable item is replacing the hand-rolled fallback with serveStatic({ path: "/index.html" }) to fix the dot-in-path bug and eliminate ~8 lines of custom code.
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
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.
No description provided.