Skip to content

fix(new-webui): Register React SPA routes in Fastify server; Fix path resolution for root directory. (fixes #1011; fixes #1014). #1015

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
junhaoliao merged 3 commits into from
Jun 16, 2025
Merged

fix(new-webui): Register React SPA routes in Fastify server; Fix path resolution for root directory. (fixes #1011; fixes #1014). #1015

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
369b391
fix(new-webui): Register React SPA routes in Fastify server; Fix path…
junhaoliao Jun 15, 2025
32eb36e
revert unrelated changes
junhaoliao Jun 15, 2025
b7da097
fix(static): Simplify static file serving by removing hardcoded clien…
junhaoliao Jun 16, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions components/log-viewer-webui/server/settings.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,9 @@
"MongoDbStreamFilesCollectionName": "stream-files",
"MongoDbSearchResultsMetadataCollectionName": "results-metadata",

"ClientDir": "../../client/dist",
"LogViewerDir": "../../yscope-log-viewer/dist",
"StreamFilesDir": "../../../../build/clp-package/var/data/streams",
"ClientDir": "../client/dist",
"LogViewerDir": "../yscope-log-viewer/dist",
"StreamFilesDir": "../../../build/clp-package/var/data/streams",
"StreamTargetUncompressedSize": 134217728,
"StreamFilesS3Region": null,
"StreamFilesS3PathPrefix": null,
Expand Down
10 changes: 6 additions & 4 deletions components/log-viewer-webui/server/src/routes/static.ts
View file
Open in desktop

@davemarco davemarco Jun 16, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

what is the purpose of the decorate reply change?

@junhaoliao junhaoliao Jun 16, 2025 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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.

@davemarco davemarco Jun 16, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Are we not also sending files on the /streams directory?

@davemarco davemarco Jun 16, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

like i understand what it does, but not why we are changing

@davemarco davemarco Jun 16, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Oh is it related to removing the client_dir change u made before?

@junhaoliao junhaoliao Jun 16, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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,
        });

Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ import settings from "../../settings.json" with {type: "json"};
const routes: FastifyPluginAsync = async (fastify) => {
const filename = fileURLToPath(import.meta.url);
const dirname = path.dirname(filename);
const rootDirname = path.resolve(dirname, "../..");
const rootDirname = path.resolve(dirname, "../../../..");

let streamFilesDir = settings.StreamFilesDir;
if (false === path.isAbsolute(streamFilesDir)) {
Expand All @@ -25,6 +25,7 @@ const routes: FastifyPluginAsync = async (fastify) => {
await fastify.register(fastifyStatic, {
prefix: "/streams",
root: streamFilesDir,
decorateReply: false,
});

let logViewerDir = settings.LogViewerDir;
Expand All @@ -48,12 +49,13 @@ const routes: FastifyPluginAsync = async (fastify) => {
await fastify.register(fastifyStatic, {
prefix: "/",
root: clientDir,
decorateReply: false,
decorateReply: true,
wildcard: false,
Comment on lines 49 to 53

@coderabbitai coderabbitai Bot Jun 15, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

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.

Suggested change
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.

@junhaoliao junhaoliao Jun 16, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@davemarco why we had it disabled? and do you know why the assets were still getting served even with it disabeld?

@coderabbitai coderabbitai Bot Jun 16, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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!

@davemarco davemarco Jun 16, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

@davemarco davemarco Jun 16, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

fastify/fastify-static#325

@junhaoliao junhaoliao Jun 16, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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

});

fastify.get("/streamFile", (_, reply) => {
reply.sendFile("index.html", clientDir);
// Serve index.html for all unmatched routes in the React Single Page Application (SPA).
fastify.get("/*", (_, reply) => {
reply.sendFile("index.html");
});
}
};
Expand Down