fix(new-webui): Register React SPA routes in Fastify server; Fix path resolution for root directory. (fixes #1011; fixes #1014).#1015
Conversation
… resolution for `rootDirname`.
WalkthroughThe changes update directory paths in the server configuration and modify the static file serving logic in the server's routing. The static route handling now includes a broader catch-all for SPA routing, adjusts directory resolution, and changes the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Server
participant FileSystem
Client->>Server: Request static file (e.g., /streams/...)
Server->>FileSystem: Resolve file path (new root resolution)
FileSystem-->>Server: Return file
Server-->>Client: Serve static file
Client->>Server: Request unknown route (e.g., /some/spa/route)
Server->>FileSystem: Read index.html
FileSystem-->>Server: Return index.html
Server-->>Client: Serve index.html (SPA fallback)
Possibly related PRs
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (1)`**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}`: - Prefer `false == ` rather than `!`.
🧠 Learnings (2)📓 Common learningscomponents/log-viewer-webui/server/src/routes/static.ts (1)⏰ Context from checks skipped due to timeout of 90000ms (2)
🔇 Additional comments (2)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
components/log-viewer-webui/server/src/fastify-v2/plugins/external/env.ts (1)
62-69: 🧹 Nitpick (assertive)
override: truemay unexpectedly clobber real env varsSetting
override: truemeans values in.env*files will always win over variables already present in the process environment (e.g., those injected by Kubernetes / Docker secrets).
If that is not explicitly required, consider keeping the default (false) or driving it off a separate flag so operators can opt-in.- override: true, + override: process.env.CLP_DOTENV_OVERRIDE === "1",
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
components/log-viewer-webui/server/settings.json(1 hunks)components/log-viewer-webui/server/src/fastify-v2/plugins/external/env.ts(1 hunks)components/log-viewer-webui/server/src/routes/static.ts(4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}`: - Prefer `false == ` rather than `!`.
**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}: - Preferfalse == <expression>rather than!<expression>.
components/log-viewer-webui/server/src/fastify-v2/plugins/external/env.tscomponents/log-viewer-webui/server/src/routes/static.ts
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: lint-check (macos-latest)
- GitHub Check: lint-check (ubuntu-latest)
🔇 Additional comments (2)
components/log-viewer-webui/server/settings.json (1)
13-16: Confirm new path hierarchy is still valid after the rootDirname jump
ClientDir,LogViewerDir, andStreamFilesDirwere each pulled one level closer, whilerootDirnameinstatic.tsmoved two levels up. Please double-check that the resultingpath.resolve(rootDirname, <dir>)still points to the intended folders once the server is started from the production bundle – an off-by-one here will silently 404 every static asset.components/log-viewer-webui/server/src/routes/static.ts (1)
29-34: Root directory changed from “server” to “components” – verify packaging scriptsJumping four levels up (
../../../..) meansrootDirnameis now<repo>/components, not<repo>/components/log-viewer-webui/server.
This is fine as long as all paths insettings.jsonwere re-based accordingly, but it will break if any other module still resolves paths relative to the old location.
| await fastify.register(fastifyStatic, { | ||
| prefix: "/", | ||
| root: clientDir, | ||
| decorateReply: false, | ||
| decorateReply: true, | ||
| wildcard: false, |
There was a problem hiding this comment.
wildcard: false blocks JS/CSS asset delivery
With prefix: "/" and wildcard: false the plugin only serves the root path /.
Requests like /assets/index-1234.js (generated by Vite/React) will bypass the static handler and fall straight to 404.
Unless another plugin serves those files, flip the flag back:
- wildcard: false,
+ // allow hashed asset files under /
+ wildcard: true,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| await fastify.register(fastifyStatic, { | |
| prefix: "/", | |
| root: clientDir, | |
| decorateReply: false, | |
| decorateReply: true, | |
| wildcard: false, | |
| await fastify.register(fastifyStatic, { | |
| prefix: "/", | |
| root: clientDir, | |
| decorateReply: true, | |
| // allow hashed asset files under / | |
| wildcard: true, |
🤖 Prompt for AI Agents
In components/log-viewer-webui/server/src/routes/static.ts around lines 59 to
63, the fastifyStatic plugin is configured with wildcard: false and prefix: "/",
which causes requests for JS/CSS assets under paths like /assets/index-1234.js
to bypass the static handler and result in 404 errors. To fix this, change
wildcard to true so that all asset requests under the prefix are properly served
by the static plugin.
There was a problem hiding this comment.
@davemarco why we had it disabled? and do you know why the assets were still getting served even with it disabeld?
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
when i was looking how to get the log viewer connector served under streamFile, it seemed like u had to disable it to have SPA fallback. I think it still serves other files in the directory, as long as they are there at build time.
https://github.com/fastify/fastify-static?tab=readme-ov-file#wildcard
There was a problem hiding this comment.
There was a problem hiding this comment.
If false, it globs the filesystem for all defined files in the served folder (${root}//) and creates the necessary routes, but will not serve newly added files.
if i understand correctly, this means the routes to serve all static files are created at init time which makes the serving deterministic for assets files
now we are adding a catch all route to serve index.html on any misses
all seems good then
|
Is there a specific reason to modify path resolution for rootDirname? was it not working in the package? |
There was a problem hiding this comment.
what is the purpose of the decorate reply change?
There was a problem hiding this comment.
https://github.com/fastify/fastify-static?tab=readme-ov-file#disabling-reply-decorator
decorateReply is by default true in the fastify-static plugin; however, when we register the plugin multiple times to serve static files on different routes from different paths, only one instance of the registration can specify decorateReply.
since we will only be calling sendFile to send files on / to serve the React SPA client, we should set decorateReply on the / fastify-static registration instance. the previous decoration on the /streams registration instance was unnecessary and unused anyways.
There was a problem hiding this comment.
Are we not also sending files on the /streams directory?
There was a problem hiding this comment.
like i understand what it does, but not why we are changing
There was a problem hiding this comment.
Oh is it related to removing the client_dir change u made before?
There was a problem hiding this comment.
Are we not also sending files on the /streams directory?
we are
Oh is it related to removing the client_dir change u made before?
right. see below
like i understand what it does, but not why we are changing
we need to call reply.sendFile() to serve the index.html file for all requests on /*. we can only set decorateReply: true on a single registration of the fastify-static plugin, and this should be done only with
await fastify.register(fastifyStatic, {
prefix: "/",
root: clientDir,
decorateReply: true,
wildcard: false,
});
the paths were not resolving as expected in debug mode when i directly run see #1014 for the context |
…t routes; Serve index.html for all unmatched routes.
|
|
rootDirname (fixes #1011; fixes #1014).… resolution for root directory. (fixes y-scope#1011; fixes y-scope#1014). (y-scope#1015)
Description
server/settings.jsonin refactor(new-webui): Enhance server implementation with Fastify toolchain. #899 .rootDirname.Checklist
breaking change.
Validation performed
./stop-clp.sh log_viewer_webui.cd clp-package/var/data/streams; touch hellocd components/log-viewer-webui/client; npm run buildcd components/log-viewer-webui/yscope-log-viewer; npm run buildcd components/log-viewer-webui/server; cp .env .env.local. Then modified value ofCLP_DB_PASSin.env.localto matchclp-package/etc/credentials.yml. (need the fix in feat(new-webui): Enable configuration override in Fastify dotenv settings (fixes #1010). #1013 for values to be correctly overridden)npm run startSummary by CodeRabbit
Bug Fixes
Chores