Skip to content

fix: [ENG-2345] serve webui SPA fallback when install path contains a dotfile#528

Merged
wzlng merged 2 commits intomainfrom
fix/ENG-2345
Apr 22, 2026
Merged

fix: [ENG-2345] serve webui SPA fallback when install path contains a dotfile#528
wzlng merged 2 commits intomainfrom
fix/ENG-2345

Conversation

@wzlng
Copy link
Copy Markdown
Collaborator

@wzlng wzlng commented Apr 22, 2026

res.sendFile() was called with an absolute path to dist/webui/index.html. send's default dotfiles: 'ignore' scans every component of the provided path, so global installs under ~/.nvm/... (or any parent starting with '.') returned 404 on hard reloads of SPA routes like /contexts. Pass 'root' so the dotfile check only applies to the relative 'index.html'.

… dotfile

res.sendFile() was called with an absolute path to dist/webui/index.html.
send's default dotfiles: 'ignore' scans every component of the provided
path, so global installs under ~/.nvm/... (or any parent starting with
'.') returned 404 on hard reloads of SPA routes like /contexts. Pass
 so the dotfile check only applies to the relative 'index.html'.
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Apr 22, 2026

Claude finished @wzlng's task in 1m 15s —— View job


Code Review — Score: 4/5

  • Read changed files and diff
  • Post inline comments
  • Post summary review

Summary

The fix is correct and well-targeted. res.sendFile(absolutePath) passes the full path through send's dotfile scanner, which flags any segment starting with . (including parent directories like .nvm). Splitting into res.sendFile('index.html', {root: webuiDistDir}) confines the check to just the filename, which never starts with a dot — exactly right.

The removal of the now-unused join import is clean.

What's good

  • Minimal, surgical change — the fix touches only the one line that caused the bug.
  • Tests reproduce the actual failure — spinning up a real HTTP server with a .nvm-containing path is a faithful reproduction of the issue and avoids mock fragility.
  • Integration test approach is appropriate — consistent with the repo's ES-module testing constraints noted in CLAUDE.md.

Issues found

Severity Location Issue
style test/…/webui-middleware.test.ts:68 as AddressInfo type assertion — violates CLAUDE.md; use a type guard instead (inline comment)
suggestion test/…/webui-middleware.test.ts:98 Missing regression test (non-dotfile path) and missing existsSync=false branch test (inline comment)
nitpick src/server/infra/webui/webui-middleware.ts:58 Comment is slightly over-explained; tighter one-liner suggested (inline comment)

The as AddressInfo assertion is the only item that technically conflicts with a stated CLAUDE.md rule. The test coverage gaps are suggestions rather than blockers — the two existing tests directly exercise the reported bug scenario. Ready to merge after the type-assertion fix.

Comment thread test/unit/infra/webui/webui-middleware.test.ts Outdated
Comment thread test/unit/infra/webui/webui-middleware.test.ts
Comment thread src/server/infra/webui/webui-middleware.ts Outdated
Replace 'as AddressInfo' cast with a type guard, tighten the SPA
fallback comment, and add tests for the normal install path and the
missing-webuiDistDir branch.
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Apr 22, 2026

Claude finished @wzlng's task in 1m 14s —— View job


Code Review — Score: 5/5

  • Read changed files and diff
  • Check if previous review issues were addressed
  • Post updated review

The follow-up commit (ac82f75) fully addresses all three items raised in the previous review. Here's the status of each:

Previous issue Status Detail
as AddressInfo type assertion ✅ Fixed Replaced with a type guard (if (!addr || typeof addr === 'string') throw ...) — now compliant with CLAUDE.md
Missing regression test (normal path) ✅ Fixed "should serve index.html for SPA routes on a normal install path" added
Missing existsSync=false branch test ✅ Fixed "should not register static or SPA routes when webuiDistDir does not exist" added
Over-explained comment ✅ Tightened 3-line comment concisely states the root option rationale and the ~/.nvm failure mode

Final state

src/server/infra/webui/webui-middleware.ts — Clean. join import removed, one-liner fix with a well-scoped comment explaining the non-obvious send dotfile behaviour.

test/unit/infra/webui/webui-middleware.test.ts — Four tests cover the full surface:

  1. SPA fallback with dotfile in install path (the reported bug)
  2. Static asset serving with dotfile in install path
  3. SPA fallback on a plain install path (regression guard)
  4. No routes registered when webuiDistDir is absent (existsSync branch)

realpathSync on the temp dir is a good touch — handles macOS /tmp/private/tmp symlink without extra complexity.

Ready to merge.

Copy link
Copy Markdown
Collaborator

@bao-byterover bao-byterover left a comment

Choose a reason for hiding this comment

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

LGTM

@wzlng wzlng merged commit 475253c into main Apr 22, 2026
6 checks passed
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