Skip to content

feat(node): serve dashboard from Node target - #488

Open
ceerRep wants to merge 1 commit into
Menci:mainfrom
ceerRep:pr/node-serve-dashboard
Open

feat(node): serve dashboard from Node target#488
ceerRep wants to merge 1 commit into
Menci:mainfrom
ceerRep:pr/node-serve-dashboard

Conversation

@ceerRep

@ceerRep ceerRep commented Aug 18, 2026

Copy link
Copy Markdown
Contributor

No description provided.

@Menci

Menci commented Aug 19, 2026

Copy link
Copy Markdown
Owner

I'm Claude Code, an agent working in this repository. Menci asked me to review this PR. Findings below; the first group is the one I'd act on before merging.

The gateway path set loses its invariant

static-web.ts introduces a new restatement of "every path the gateway owns" — gatewayPathPrefixes and gatewayPaths, hardcoded — but this PR removes a topology from gateway-paths_test.ts (Three hosting topologiesTwo) instead of adding the new one. The count reads as correct while the actual new duplicate is unguarded: PUBLIC_DATA_PLANE_ROUTES is no longer replayed through the topology self-hosters run.

That test exists because, in its own words, drift here "surfaces only as a 404 the SPA fallback served for a real gateway endpoint" — silent until a user hits it. A hardcoded list with no test and no cross-reference comment is exactly the shape it was written to prevent.

Concretely: static-web.ts L9-L41 should carry the same codesync comment its two siblings carry, and gateway-paths_test.ts should replay the route table through isGatewayPath as it used to through the nginx regexes.

Three stale references to the deleted file

  • wrangler.example.jsonc L49-L59 still names docker/nginx.conf, and still says "the set two other files restate" and "replays … through all three". Not touched by this PR.
  • pnpm-workspace.yaml L50-L52: "served by Workers Static Assets in production and by nginx in the docker-compose topology". Also untouched.
  • apps/web/gateway-paths.ts is updated to two topologies, but does not list the Node one this PR creates.

Behaviour

  • contentTypes is missing .ttf and .txt. The build emits codicon-<hash>.ttf and robots.txt; both now fall to application/octet-stream, which nginx's bundled mime.types covered. The .map entry shows this class of problem was already on your radar — these two are the remaining members of it.
  • isStaticAssetPath treats any path with a dot in its last segment as an asset, so such a URL 404s instead of serving the SPA shell. No current route hits it, but Cloudflare serves the SPA for those paths, so the two topologies now disagree, and a future route parameter containing a dot would break only on Node.
  • No ETag or Last-Modified on any response, while index.html is no-cache. Revalidation without a validator is a full re-download, so the document is re-fetched in its entirety on every load. nginx and Workers Static Assets both send a validator and answer 304.
  • readFile buffers each file whole per request. There are 192 sourcemaps in a build, the largest around 1.3 MiB; nginx streamed them.
  • /LICENSE was served by the web image (COPY LICENSE /usr/share/nginx/html/LICENSE) and is now gone, with no replacement.

Deployment

  • HOST now defaults to 127.0.0.1, where @hono/node-server previously bound every interface. The Dockerfile, compose file and Quadlet all set HOST=0.0.0.0, so the first-party paths are fine — but README's own pnpm run dev:node now binds loopback only. Worth stating as intentional if it is.
  • Dropping the floway-web image is a breaking upgrade. An existing Quadlet user keeps a floway-web.container with Pull=newer pointed at an image that stops being published. The systemd README covers fresh installs; there's no upgrade note telling them to remove that unit.

For disclosure: #494 touches docker/nginx.conf and will need rebasing onto this once it lands — its nginx half is superseded by the immutable handling you already have in responseFromFile.

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