refactor(new-webui): Enhance server implementation with Fastify toolchain.#899
Conversation
WalkthroughThis change refactors the log-viewer-webui server to a plugin-based Fastify v2 architecture, introduces modular plugin and route registration, updates environment and database configuration handling, streamlines server startup and shutdown, and modernizes test setup. Documentation and configuration files were updated to match new scripts and directory paths. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant FastifyServer
participant Plugins
participant Routes
participant Database
User->>FastifyServer: Sends HTTP request
FastifyServer->>Plugins: Loads external/app plugins (autoload)
FastifyServer->>Routes: Registers route handlers (autoload)
FastifyServer->>Database: Accesses MySQL/Mongo via plugins
Routes->>Plugins: Uses decorated plugin methods (e.g., ExamplePlugin)
FastifyServer->>User: Returns response or error (with centralized handler)
Possibly related issues
Possibly related PRs
Suggested reviewers
Note ⚡️ AI Code Reviews for VS Code, Cursor, WindsurfCodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. Note ⚡️ Faster reviews with cachingCodeRabbit now supports caching for code and dependencies, helping speed up reviews. This means quicker feedback, reduced wait times, and a smoother review experience overall. Cached data is encrypted and stored securely. This feature will be automatically enabled for all accounts on May 16th. To opt out, configure ✨ 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: 17
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
components/log-viewer-webui/server/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (16)
components/log-viewer-webui/server/.env(1 hunks)components/log-viewer-webui/server/package.json(3 hunks)components/log-viewer-webui/server/src/app.ts(3 hunks)components/log-viewer-webui/server/src/fastify-v2/app.ts(1 hunks)components/log-viewer-webui/server/src/fastify-v2/plugins/app/sample.ts(1 hunks)components/log-viewer-webui/server/src/fastify-v2/plugins/external/env.ts(1 hunks)components/log-viewer-webui/server/src/fastify-v2/plugins/external/mongoDb.ts(1 hunks)components/log-viewer-webui/server/src/fastify-v2/plugins/external/mySql.ts(1 hunks)components/log-viewer-webui/server/src/fastify-v2/plugins/external/rate-limit.ts(1 hunks)components/log-viewer-webui/server/src/fastify-v2/plugins/external/sensible.ts(1 hunks)components/log-viewer-webui/server/src/fastify-v2/routes/sample.ts(1 hunks)components/log-viewer-webui/server/src/main.ts(1 hunks)components/log-viewer-webui/server/src/plugins/DbManager.ts(1 hunks)components/log-viewer-webui/server/src/test/app.test.ts(2 hunks)components/log-viewer-webui/server/src/utils/env.ts(0 hunks)components/log-viewer-webui/server/src/utils/tap.ts(1 hunks)
💤 Files with no reviewable changes (1)
- components/log-viewer-webui/server/src/utils/env.ts
🧰 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/sensible.tscomponents/log-viewer-webui/server/src/test/app.test.tscomponents/log-viewer-webui/server/src/fastify-v2/routes/sample.tscomponents/log-viewer-webui/server/src/utils/tap.tscomponents/log-viewer-webui/server/src/fastify-v2/plugins/external/rate-limit.tscomponents/log-viewer-webui/server/src/fastify-v2/plugins/app/sample.tscomponents/log-viewer-webui/server/src/fastify-v2/app.tscomponents/log-viewer-webui/server/src/fastify-v2/plugins/external/env.tscomponents/log-viewer-webui/server/src/fastify-v2/plugins/external/mySql.tscomponents/log-viewer-webui/server/src/plugins/DbManager.tscomponents/log-viewer-webui/server/src/main.tscomponents/log-viewer-webui/server/src/fastify-v2/plugins/external/mongoDb.tscomponents/log-viewer-webui/server/src/app.ts
🧬 Code Graph Analysis (7)
components/log-viewer-webui/server/src/test/app.test.ts (1)
components/log-viewer-webui/server/src/utils/tap.ts (1)
build(31-44)
components/log-viewer-webui/server/src/fastify-v2/plugins/app/sample.ts (1)
components/log-viewer-webui/server/src/fastify-v2/plugins/external/env.ts (1)
FastifyInstance(6-19)
components/log-viewer-webui/server/src/fastify-v2/app.ts (2)
components/log-viewer-webui/server/src/fastify-v2/plugins/external/env.ts (1)
FastifyInstance(6-19)components/log-viewer-webui/server/src/fastify-v2/plugins/app/sample.ts (1)
FastifyInstance(31-33)
components/log-viewer-webui/server/src/fastify-v2/plugins/external/env.ts (3)
components/log-viewer-webui/server/src/fastify-v2/plugins/external/rate-limit.ts (1)
autoConfig(7-12)components/log-viewer-webui/server/src/fastify-v2/plugins/external/mySql.ts (1)
autoConfig(18-27)components/webui/server/main.js (1)
process(30-33)
components/log-viewer-webui/server/src/fastify-v2/plugins/external/mySql.ts (1)
components/log-viewer-webui/server/src/fastify-v2/plugins/external/env.ts (2)
FastifyInstance(6-19)autoConfig(82-98)
components/log-viewer-webui/server/src/main.ts (1)
components/log-viewer-webui/server/src/fastify-v2/app.ts (1)
serviceApp(25-114)
components/log-viewer-webui/server/src/app.ts (3)
components/log-viewer-webui/server/src/fastify-v2/plugins/external/env.ts (1)
FastifyInstance(6-19)components/log-viewer-webui/server/src/fastify-v2/plugins/app/sample.ts (1)
FastifyInstance(31-33)components/webui/server/main.js (1)
process(30-33)
🪛 Biome (1.9.4)
components/log-viewer-webui/server/src/fastify-v2/app.ts
[error] 31-31: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: lint-check (macos-latest)
- GitHub Check: lint-check (ubuntu-latest)
🔇 Additional comments (19)
components/log-viewer-webui/server/.env (1)
10-10: Good addition of graceful shutdown configuration.The new environment variable
FASTIFY_CLOSE_GRACE_DELAYaligns with the PR objective of adding graceful termination capabilities to the server. The 1000ms (1 second) value provides a reasonable delay for cleanup operations during shutdown.components/log-viewer-webui/server/src/fastify-v2/plugins/external/sensible.ts (1)
1-4: Clean implementation of the sensible plugin wrapper.This implementation follows the Fastify maintainers' recommendation of organizing plugins in a modular way. The simple pass-through approach keeps the code maintainable while allowing for future customization if needed.
components/log-viewer-webui/server/src/test/app.test.ts (1)
2-8: Good refactoring to use standardized test utilities.The changes improve test maintenance by replacing manual server setup/teardown with the
buildutility function. The explicit typing of the test parameter improves code clarity and type safety.components/log-viewer-webui/server/package.json (2)
7-12: LGTM! Improved development workflow and scriptsThe updated scripts provide a streamlined development experience with concurrent TypeScript compilation and app watching. The new Fastify CLI integration aligns well with the Fastify maintainer recommendations mentioned in the PR objectives.
23-29: LGTM! Well-chosen dependencies for the new architectureThe added dependencies support the modular plugin architecture described in the PR objectives. The rate limiter and graceful shutdown packages specifically address the security enhancements and proper shutdown mentioned in the PR summary.
Also applies to: 34-34, 46-46
components/log-viewer-webui/server/src/fastify-v2/plugins/app/sample.ts (2)
1-28: Well-structured plugin pattern implementationThe code follows Fastify's recommended plugin pattern using
fastify-plugin. The factory function approach for creating the example plugin is a good practice as it encapsulates the plugin's functionality and makes it more reusable.The TODO comment clearly indicates that this is temporary example code, which is helpful for future maintenance.
30-34: Type declaration properly extends FastifyInstanceThe module augmentation pattern used here to extend the FastifyInstance interface is the correct approach for adding type definitions for Fastify plugins. This ensures type safety when accessing the
ExamplePluginproperty elsewhere in the application.components/log-viewer-webui/server/src/fastify-v2/plugins/external/mySql.ts (2)
12-16: Type declaration is correctly implementedThe module augmentation correctly extends the FastifyInstance interface to include the MySQL plugin types, which is the recommended approach in the Fastify documentation. The comment explaining the need for manual type specification adds helpful context.
18-27:Details
✅ Verification successful
Ensure configuration function properly handles dependency order
The
autoConfigfunction correctly uses settings from both the imported JSON and the Fastify config. However, it assumes that the env plugin has already been registered and configured before the MySQL plugin is registered.
🏁 Script executed:
#!/bin/bash # Verify the plugin registration order in the main app file grep -A 20 "fastifyAutoload" components/log-viewer-webui/server/src/fastify-v2/app.tsLength of output: 1813
🏁 Script executed:
#!/bin/bash # List all plugins in the external directory to check for an env plugin ls -1 components/log-viewer-webui/server/src/fastify-v2/plugins/externalLength of output: 127
Configuration plugin ordering is correct
The
env.tsplugin is present in theexternaldirectory and, thanks to Fastify Autoload’s lexicographical loading of filenames, is registered beforemySql.ts. This guarantees thatfastify.config.CLP_DB_USERandfastify.config.CLP_DB_PASSare available whenautoConfigruns—no changes required.components/log-viewer-webui/server/src/fastify-v2/app.ts (5)
33-37: Good use of autoload for plugin organizationThe approach of using
fastifyAutoloadto load external plugins from a dedicated directory follows Fastify's recommended practices for modular code organization. This approach simplifies maintenance and keeps the main application code clean.
39-45: Clear migration strategy with TODO commentThe code clearly documents the temporary nature of the old webui integration with a TODO comment. The approach of registering the old webui code as a separate plugin while gradually migrating to the new architecture is a good transitional strategy.
47-59: Well-structured plugin and route registrationThe separation of application plugins and routes with autoload, along with the use of
autoHooksandcascadeHooks, provides a clean and maintainable structure. This approach aligns with Fastify's recommended practices for organizing routes and plugins.
61-86: Robust error handling implementationThe centralized error handler provides comprehensive logging of errors with relevant request details. The conditional handling based on status code is appropriate, returning the error message for client errors and a generic message for server errors.
The expression
"undefined" !== typeof err.statusCodefollows the coding guideline to preferfalse == <expression>rather than!<expression>.
88-113: Security-conscious 404 handling with rate limitingThe implementation of a rate-limited 404 handler is an excellent security practice to mitigate enumeration attacks. The comprehensive logging of request details aids in troubleshooting and potential security monitoring.
components/log-viewer-webui/server/src/fastify-v2/plugins/external/env.ts (3)
5-20: Well-defined type extensions for configurationThe module augmentation pattern correctly extends the FastifyInstance interface with a strongly-typed config object. This ensures type safety when accessing configuration values throughout the application.
23-80: Comprehensive schema with appropriate validationsThe schema definition is well-structured with appropriate types, defaults, and validation constraints for each environment variable. The organization into logical sections (databases, directories, security) improves readability.
The validation constraints, such as port number ranges and string minimum lengths, help prevent misconfiguration issues.
82-98: Well-configured environment loadingThe
autoConfigobject properly configures the env plugin with the schema, configuration key, and dotenv settings. The approach of checking multiple .env files (.env.local and .env) is a good practice for supporting different environments.components/log-viewer-webui/server/src/main.ts (1)
64-67: Expose HOST binding and listen backlog through config
app.listen({port: ...})binds to0.0.0.0by default. If this service must be private (e.g., behind a reverse proxy) you may want to bind to127.0.0.1or a configurable host.Consider:
-await app.listen({port: Number(process.env.PORT || DEFAULT_PORT)}); +await app.listen({ + port: Number(process.env.PORT || DEFAULT_PORT), + host: process.env.HOST ?? "0.0.0.0", +});components/log-viewer-webui/server/src/app.ts (1)
34-63: Routes may access decorators that are absent in test mode
DbManager,S3Manager, andMongoSocketIoServerare conditionally registered only whenNODE_ENV !== "test".
If any ofstaticRoutes,exampleRoutes, orqueryRoutesrely on decorators exposed by those plugins, unit tests will fail with “Decorator not found”.Two options:
- Register lightweight stubs in test mode.
- Move the
if ("test" !== process.env.NODE_ENV)check inside each route that truly needs the decorators.Please verify route dependencies to avoid brittle tests.
| import { | ||
| FastifyPluginAsyncTypebox, | ||
| Type, | ||
| } from "@fastify/type-provider-typebox"; | ||
|
|
||
|
|
||
| /** | ||
| * Example to demonstrate how to create a Fastify route. | ||
| * | ||
| * TODO: Remove example code when new webui app code is ready. | ||
| * | ||
| * @param fastify | ||
| */ | ||
| const plugin: FastifyPluginAsyncTypebox = async (fastify) => { | ||
| const {ExamplePlugin} = fastify; | ||
| fastify.get( | ||
| "/Example", | ||
| { | ||
| schema: { | ||
| response: { | ||
| 200: Type.Object({ | ||
| message: Type.String(), | ||
| }), | ||
| }, | ||
| }, | ||
| }, | ||
| async () => { | ||
| const exampleMessage = await ExamplePlugin.getExample(); | ||
| return {message: exampleMessage}; | ||
| } | ||
| ); | ||
| }; |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Well-structured sample route with minor improvement suggestions.
The route demonstrates proper use of TypeBox schema validation and follows the plugin architecture pattern. The TODO comment clearly indicates this is example code.
Consider these improvements:
- Route paths typically use lowercase or kebab-case in RESTful APIs (e.g., "/example" instead of "/Example")
- Add validation to ensure ExamplePlugin exists before using it
- "/Example",
+ "/example",const plugin: FastifyPluginAsyncTypebox = async (fastify) => {
+ if (!fastify.hasDecorator('ExamplePlugin')) {
+ throw new Error('ExamplePlugin is not registered');
+ }
const {ExamplePlugin} = fastify;
// remaining code...📝 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.
| import { | |
| FastifyPluginAsyncTypebox, | |
| Type, | |
| } from "@fastify/type-provider-typebox"; | |
| /** | |
| * Example to demonstrate how to create a Fastify route. | |
| * | |
| * TODO: Remove example code when new webui app code is ready. | |
| * | |
| * @param fastify | |
| */ | |
| const plugin: FastifyPluginAsyncTypebox = async (fastify) => { | |
| const {ExamplePlugin} = fastify; | |
| fastify.get( | |
| "/Example", | |
| { | |
| schema: { | |
| response: { | |
| 200: Type.Object({ | |
| message: Type.String(), | |
| }), | |
| }, | |
| }, | |
| }, | |
| async () => { | |
| const exampleMessage = await ExamplePlugin.getExample(); | |
| return {message: exampleMessage}; | |
| } | |
| ); | |
| }; | |
| import { | |
| FastifyPluginAsyncTypebox, | |
| Type, | |
| } from "@fastify/type-provider-typebox"; | |
| /** | |
| * Example to demonstrate how to create a Fastify route. | |
| * | |
| * TODO: Remove example code when new webui app code is ready. | |
| * | |
| * @param fastify | |
| */ | |
| const plugin: FastifyPluginAsyncTypebox = async (fastify) => { | |
| if (!fastify.hasDecorator('ExamplePlugin')) { | |
| throw new Error('ExamplePlugin is not registered'); | |
| } | |
| const {ExamplePlugin} = fastify; | |
| fastify.get( | |
| "/example", | |
| { | |
| schema: { | |
| response: { | |
| 200: Type.Object({ | |
| message: Type.String(), | |
| }), | |
| }, | |
| }, | |
| }, | |
| async () => { | |
| const exampleMessage = await ExamplePlugin.getExample(); | |
| return {message: exampleMessage}; | |
| } | |
| ); | |
| }; |
| export const autoConfig = (fastify: FastifyInstance) => { | ||
| return { | ||
| max: fastify.config.RATE_LIMIT_MAX, | ||
| timeWindow: "1 minute", | ||
| }; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add defensive configuration with defaults
The configuration function could be more robust by providing default values in case fastify.config.RATE_LIMIT_MAX is undefined.
export const autoConfig = (fastify: FastifyInstance) => {
return {
- max: fastify.config.RATE_LIMIT_MAX,
+ max: fastify.config.RATE_LIMIT_MAX || 100, // Default to 100 requests
timeWindow: "1 minute",
};
};📝 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.
| export const autoConfig = (fastify: FastifyInstance) => { | |
| return { | |
| max: fastify.config.RATE_LIMIT_MAX, | |
| timeWindow: "1 minute", | |
| }; | |
| }; | |
| export const autoConfig = (fastify: FastifyInstance) => { | |
| return { | |
| max: fastify.config.RATE_LIMIT_MAX || 100, // Default to 100 requests | |
| timeWindow: "1 minute", | |
| }; | |
| }; |
| import fastifyRateLimit from "@fastify/rate-limit"; | ||
| import {FastifyInstance} from "fastify"; | ||
|
|
||
|
|
||
| export const autoConfig = (fastify: FastifyInstance) => { | ||
| return { | ||
| max: fastify.config.RATE_LIMIT_MAX, | ||
| timeWindow: "1 minute", | ||
| }; | ||
| }; | ||
|
|
||
| export default fastifyRateLimit; |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Add JSDoc for better documentation
The code follows the Fastify plugin pattern well, but would benefit from additional documentation explaining the purpose and configuration of the rate limiter.
// Reference: https://github.com/fastify/demo/blob/main/src/plugins/external/rate-limit.ts
import fastifyRateLimit from "@fastify/rate-limit";
import {FastifyInstance} from "fastify";
+/**
+ * Rate limiter plugin to protect API endpoints from excessive requests.
+ * Uses @fastify/rate-limit to limit requests based on configuration.
+ */
+/**
+ * Provides configuration for the rate limiter based on environment settings.
+ * @param fastify - The Fastify instance with environment config
+ * @returns Rate limiter configuration with max requests and time window
+ */
export const autoConfig = (fastify: FastifyInstance) => {
return {
max: fastify.config.RATE_LIMIT_MAX,
timeWindow: "1 minute",
};
};
export default fastifyRateLimit;📝 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.
| import fastifyRateLimit from "@fastify/rate-limit"; | |
| import {FastifyInstance} from "fastify"; | |
| export const autoConfig = (fastify: FastifyInstance) => { | |
| return { | |
| max: fastify.config.RATE_LIMIT_MAX, | |
| timeWindow: "1 minute", | |
| }; | |
| }; | |
| export default fastifyRateLimit; | |
| import fastifyRateLimit from "@fastify/rate-limit"; | |
| import {FastifyInstance} from "fastify"; | |
| /** | |
| * Rate limiter plugin to protect API endpoints from excessive requests. | |
| * Uses @fastify/rate-limit to limit requests based on configuration. | |
| */ | |
| /** | |
| * Provides configuration for the rate limiter based on environment settings. | |
| * @param fastify - The Fastify instance with environment config | |
| * @returns Rate limiter configuration with max requests and time window | |
| */ | |
| export const autoConfig = (fastify: FastifyInstance) => { | |
| return { | |
| max: fastify.config.RATE_LIMIT_MAX, | |
| timeWindow: "1 minute", | |
| }; | |
| }; | |
| export default fastifyRateLimit; |
| @@ -0,0 +1,13 @@ | |||
| import fastifyMongoDb from "@fastify/mongodb"; | |||
|
|
|||
| import settings from "../../../../settings.json" with {type: "json"}; | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve configuration import using a more robust path
The deep relative path import for settings is fragile and could break if the file structure changes.
Consider using a more robust approach to load settings, such as:
- import settings from "../../../../settings.json" with {type: "json"};
+ import {fileURLToPath} from 'node:url';
+ import {dirname, resolve} from 'node:path';
+ import settings from "../../../../../settings.json" with {type: "json"};
+
+ // Or even better, load settings relative to project root:
+ // const __dirname = dirname(fileURLToPath(import.meta.url));
+ // const rootDir = resolve(__dirname, '../../../../../');
+ // import settings from resolve(rootDir, 'settings.json') with {type: "json"};Committable suggestion skipped: line range outside the PR's diff.
| import fastifyMongoDb from "@fastify/mongodb"; | ||
|
|
||
| import settings from "../../../../settings.json" with {type: "json"}; | ||
|
|
||
|
|
||
| export const autoConfig = () => { | ||
| return { | ||
| forceClose: true, | ||
| url: `mongodb://${settings.MongoDbHost}:${settings.MongoDbPort}/${settings.MongoDbName}`, | ||
| }; | ||
| }; | ||
|
|
||
| export default fastifyMongoDb; |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Add JSDoc documentation for better maintainability
The file would benefit from documentation explaining the purpose and expected configuration format.
+/**
+ * MongoDB plugin integration for Fastify
+ *
+ * Requires the following settings in settings.json:
+ * - MongoDbHost: MongoDB server hostname or IP
+ * - MongoDbPort: MongoDB server port
+ * - MongoDbName: MongoDB database name
+ * - MongoDbUser (optional): MongoDB username for authentication
+ * - MongoDbPassword (optional): MongoDB password for authentication
+ */
import fastifyMongoDb from "@fastify/mongodb";
import settings from "../../../../settings.json" with {type: "json"};
+/**
+ * Provides MongoDB connection configuration based on settings.json
+ * @returns Configuration object for @fastify/mongodb plugin
+ */
export const autoConfig = () => {
return {
forceClose: true,
url: `mongodb://${settings.MongoDbHost}:${settings.MongoDbPort}/${settings.MongoDbName}`,
};
};
export default fastifyMongoDb;📝 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.
| import fastifyMongoDb from "@fastify/mongodb"; | |
| import settings from "../../../../settings.json" with {type: "json"}; | |
| export const autoConfig = () => { | |
| return { | |
| forceClose: true, | |
| url: `mongodb://${settings.MongoDbHost}:${settings.MongoDbPort}/${settings.MongoDbName}`, | |
| }; | |
| }; | |
| export default fastifyMongoDb; | |
| /** | |
| * MongoDB plugin integration for Fastify | |
| * | |
| * Requires the following settings in settings.json: | |
| * - MongoDbHost: MongoDB server hostname or IP | |
| * - MongoDbPort: MongoDB server port | |
| * - MongoDbName: MongoDB database name | |
| * - MongoDbUser (optional): MongoDB username for authentication | |
| * - MongoDbPassword (optional): MongoDB password for authentication | |
| */ | |
| import fastifyMongoDb from "@fastify/mongodb"; | |
| import settings from "../../../../settings.json" with {type: "json"}; | |
| /** | |
| * Provides MongoDB connection configuration based on settings.json | |
| * @returns Configuration object for @fastify/mongodb plugin | |
| */ | |
| export const autoConfig = () => { | |
| return { | |
| forceClose: true, | |
| url: `mongodb://${settings.MongoDbHost}:${settings.MongoDbPort}/${settings.MongoDbName}`, | |
| }; | |
| }; | |
| export default fastifyMongoDb; |
| // eslint-disable-next-line max-lines-per-function | ||
| export default async function serviceApp ( | ||
| fastify: FastifyInstance, | ||
| opts: FastifyPluginOptions | ||
| ) { | ||
| // Option only serves testing purpose. It's used in testing to expose all decorators to the | ||
| // test app. Some decorators may not be exposed in production. | ||
| delete opts.skipOverride; |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider alternative to delete operator
The static analysis tool flagged a potential performance issue with the delete operator. Consider using an undefined assignment instead.
- delete opts.skipOverride;
+ opts.skipOverride = undefined;📝 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.
| // eslint-disable-next-line max-lines-per-function | |
| export default async function serviceApp ( | |
| fastify: FastifyInstance, | |
| opts: FastifyPluginOptions | |
| ) { | |
| // Option only serves testing purpose. It's used in testing to expose all decorators to the | |
| // test app. Some decorators may not be exposed in production. | |
| delete opts.skipOverride; | |
| // eslint-disable-next-line max-lines-per-function | |
| export default async function serviceApp ( | |
| fastify: FastifyInstance, | |
| opts: FastifyPluginOptions | |
| ) { | |
| // Option only serves testing purpose. It's used in testing to expose all decorators to the | |
| // test app. Some decorators may not be exposed in production. | |
| opts.skipOverride = undefined; |
🧰 Tools
🪛 Biome (1.9.4)
[error] 31-31: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
| const mysqlConnectionPool = app.mysql.pool; | ||
| if ("undefined" === typeof app.mongo.db) { | ||
| throw new Error("MongoDB database not found"); | ||
| } | ||
| const streamFilesCollection = | ||
| app.mongo.db.collection<StreamFileMongoDocument>( | ||
| dbConfig.mongoConfig.streamFilesCollectionName | ||
| ); | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider making the poll interval configurable
JOB_COMPLETION_STATUS_POLL_INTERVAL_MILLIS is set to 0.5, implying 0.5 ms, which in practice becomes sub-millisecond busy-waiting. The value is hard-coded so it cannot be tuned in production or tests.
Expose this interval through DbManagerOptions (or an env var) and use a sensible default (e.g., 500 ms) to avoid unnecessary CPU usage.
🛠️ Refactor suggestion
Add a guard for missing MySQL pool – symmetry with Mongo check
You correctly verify that app.mongo.db exists but you assume the MySQL pool is always present.
If the external @fastify/mysql plugin is not registered or fails during bootstrap the line will throw at runtime with an unhelpful TypeError: Cannot read property 'pool' of undefined.
Adding the same explicit guard you have for MongoDB makes the failure mode deterministic and the error message actionable.
-const mysqlConnectionPool = app.mysql.pool;
+if ("undefined" === typeof app.mysql?.pool) {
+ throw new Error("MySQL connection pool not found");
+}
+const mysqlConnectionPool = app.mysql.pool;📝 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.
| const mysqlConnectionPool = app.mysql.pool; | |
| if ("undefined" === typeof app.mongo.db) { | |
| throw new Error("MongoDB database not found"); | |
| } | |
| const streamFilesCollection = | |
| app.mongo.db.collection<StreamFileMongoDocument>( | |
| dbConfig.mongoConfig.streamFilesCollectionName | |
| ); | |
| if ("undefined" === typeof app.mysql?.pool) { | |
| throw new Error("MySQL connection pool not found"); | |
| } | |
| const mysqlConnectionPool = app.mysql.pool; | |
| if ("undefined" === typeof app.mongo.db) { | |
| throw new Error("MongoDB database not found"); | |
| } | |
| const streamFilesCollection = | |
| app.mongo.db.collection<StreamFileMongoDocument>( | |
| dbConfig.mongoConfig.streamFilesCollectionName | |
| ); |
| const app = fastify({ | ||
| logger: getLoggerOptions(), | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Export the Fastify instance to improve testability & reusability
Declaring const app = fastify(...); at module scope is fine for production, but in unit/integration tests it is common to import the pre-configured instance.
Exporting it (or a factory) avoids the need to duplicate logger/bootstrap logic downstream.
-const app = fastify({
+export const app = fastify({
logger: getLoggerOptions(),
});📝 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.
| const app = fastify({ | |
| logger: getLoggerOptions(), | |
| }); | |
| export const app = fastify({ | |
| logger: getLoggerOptions(), | |
| }); |
| {delay: Number(process.env.FASTIFY_CLOSE_GRACE_DELAY || DEFAULT_FASTIFY_CLOSE_GRACE_DELAY)}, | ||
| async ({err}) => { | ||
| if (err) { | ||
| app.log.error(err); | ||
| } | ||
|
|
||
| await app.close(); | ||
| } | ||
| ); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Handle shutdown errors and set process exit code
closeWithGrace awaits app.close() but ignores its rejection. If app.close() fails (e.g., pending keep-alive connections) the process will silently hang.
- async ({err}) => {
+ async ({err}) => {
if (err) {
app.log.error(err);
}
- await app.close();
+ try {
+ await app.close();
+ } catch (closeErr) {
+ app.log.error(closeErr, "Fastify failed to shut down gracefully");
+ process.exitCode = 1;
+ }
}📝 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.
| closeWithGrace( | |
| {delay: Number(process.env.FASTIFY_CLOSE_GRACE_DELAY || DEFAULT_FASTIFY_CLOSE_GRACE_DELAY)}, | |
| async ({err}) => { | |
| if (err) { | |
| app.log.error(err); | |
| } | |
| await app.close(); | |
| } | |
| closeWithGrace( | |
| {delay: Number(process.env.FASTIFY_CLOSE_GRACE_DELAY || DEFAULT_FASTIFY_CLOSE_GRACE_DELAY)}, | |
| async ({err}) => { | |
| if (err) { | |
| app.log.error(err); | |
| } | |
| try { | |
| await app.close(); | |
| } catch (closeErr) { | |
| app.log.error(closeErr, "Fastify failed to shut down gracefully"); | |
| process.exitCode = 1; | |
| } | |
| } |
There was a problem hiding this comment.
I agree with the rabbit. it does seem .close() would throw:
https://fastify.dev/docs/latest/Reference/Server/#close
There was a problem hiding this comment.
🧩 Analysis chain
@junhaoliao Thank you for confirming this with the Fastify documentation. You're absolutely right - the .close() method returns a Promise that can reject if errors occur during the shutdown process, especially when plugins with onClose hooks encounter issues.
Implementing the suggested try/catch pattern is definitely the right approach here. It ensures that any errors during shutdown are properly logged and the process exit code is set appropriately, preventing the server from silently hanging.
I'll implement this change as suggested to make the graceful shutdown process more robust.
🐰✨ Rabbit taking careful notes about error handling
🌐 Web query:
Fastify server close method documentation and error handling
💡 Result:
Fastify's close() method is an essential part of the server lifecycle that allows for graceful shutdowns of your application. This method properly terminates the server instance while executing important cleanup operations.
Server Close Method
The close() method in Fastify is used to shut down the server instance and run the onClose hook. When invoked, it performs several important operations:
fastify.close(callback)You can call this function with a callback that will execute after the server has closed, or you can use it with promises:
fastify.close()
.then(() => console.log('server closed successfully'))
.catch(err => console.error('Error closing server:', err))When close() is called, the server will:
- Stop accepting new connections
- Execute all registered
onClosehooks - By default, respond with a 503 status code to any new incoming requests while closing[3][4]
- Handle existing connections based on configuration settings
Connection Handling During Close
Fastify provides options to control how connections are handled during server shutdown:
forceCloseConnections
This setting determines how persistent connections are handled when closing the server:
- When set to
"idle"(default in newer versions), the server will only close connections that are not actively sending requests or waiting for responses[1] - When set to
true, the server will forcibly close all connections regardless of their state[8] - When set to
false, connections will be allowed to time out naturally
true, connections are not inspected to determine if requests have been completed before being destroyed[1].
Error Handling
Fastify provides robust error handling mechanisms that work with the server lifecycle:
OnClose Hook
The onClose hook is triggered when fastify.close() is called. This hook is particularly useful for cleanup operations:
fastify.addHook('onClose', (instance, done) => {
// Cleanup code (close database connections, etc.)
done()
})This hook is essential when plugins need a "shutdown" event, such as closing database connections[5].
Error Handling in Hooks
If an error occurs during the execution of a hook, passing it to the done() callback will make Fastify automatically close the request and send the appropriate error code:
fastify.addHook('onRequest', (request, reply, done) => {
if (someErrorCondition) {
done(new Error('Something went wrong'))
}
done()
})When an error occurs in a hook, Fastify will route it to a custom error handler if one is defined using setErrorHandler(), or to the default error handler if not[6][9].
Custom Error Handlers
Custom error handlers can be registered to handle errors during the server lifecycle:
fastify.setErrorHandler((error, request, reply) => {
// Custom error handling logic
reply.status(500).send({ error: 'Server error occurred' })
})Error handlers are fully encapsulated, meaning a handler defined within a plugin will only catch errors within that plugin's context[6].
Detecting Client Aborts
Fastify also provides mechanisms to detect when clients abort requests, which can be helpful during server shutdown:
app.addHook('onRequest', async (request, reply) => {
request.raw.on('close', () => {
if (request.raw.aborted) {
app.log.info('request closed')
}
})
})This approach allows detecting when a client intentionally aborts a request, but it won't trigger in the event of internet disruption[7].
Configuration Options
Several configuration options affect the server's closing behavior:
return503OnClosing
This setting determines how the server handles incoming requests during shutdown:
- When
true(default), the server returns a 503 status code for requests received after callingclose() - When
false, the server continues to route incoming requests as usual[3][8]
By understanding these mechanisms, you can implement graceful shutdowns that properly clean up resources and handle in-flight requests appropriately.
Citations:
- 1: https://fastify.io/docs/latest/Reference/Server/
- 2: https://fastify.io/docs/v1.14.x/Documentation/Server/
- 3: https://fastify.io/docs/v2.15.x/Documentation/Server/
- 4: https://zen-kalam-f6e81f.netlify.app/docs/v0.43.x/server-methods/
- 5: https://fastify.io/docs/v2.15.x/Documentation/Hooks/
- 6: https://fastify.io/docs/latest/Reference/Errors/
- 7: https://fastify.io/docs/v5.0.x/Guides/Detecting-When-Clients-Abort/
- 8: https://fastify.io/docs/v3.29.x/Reference/Server/
- 9: https://fastify.io/docs/v3.29.x/Reference/Errors/
- 10: How to crash server from error handler? fastify/help#371
@junhaoliao You're absolutely right. According to the Fastify documentation, the close() method returns a Promise that can be rejected if errors occur during shutdown, especially when plugins have registered onClose hooks that fail.
The documentation recommends handling errors like this:
fastify.close()
.then(() => console.log('server closed successfully'))
.catch(err => console.error('Error closing server:', err))The suggested change with a try/catch block is definitely the correct approach here. It will ensure:
- Errors during shutdown are properly logged
- The process exit code is set to prevent silent failures
- The application won't hang if a plugin fails to close properly
This is especially important for graceful shutdowns in production environments.
🐰📝 Making notes about proper Fastify shutdown handling
✏️ Learnings added
Learnt from: junhaoliao
PR: y-scope/clp#899
File: components/log-viewer-webui/server/src/main.ts:50-58
Timestamp: 2025-05-13T19:38:31.124Z
Learning: In Fastify applications, the `app.close()` method returns a Promise that can reject if errors occur during shutdown. Always wrap it in a try/catch block to properly handle shutdown errors and set the process exit code to prevent the application from hanging.
Learnt from: junhaoliao
PR: y-scope/clp#899
File: components/log-viewer-webui/server/src/main.ts:50-58
Timestamp: 2025-05-13T19:38:31.124Z
Learning: When using Fastify's `app.close()` method, always wrap it in a try/catch block to properly handle shutdown errors. The close() method returns a Promise that can reject if errors occur during the shutdown process (especially with plugins using onClose hooks), and unhandled rejections could cause the application to hang.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
based on the repo, it dosent do this. https://github.com/mcollina/close-with-grace
perhaps the function itself(close with grace) does the then, catch
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.
I think we should at least attempt logging any exception before the process is terminated abruptly.
| * TODO: Once old webui code is refactored to new modlular fastify style, this plugin should be | ||
| * removed. | ||
| * | ||
| * @param fastify | ||
| * @param opts | ||
| * @return | ||
| */ |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Typo in comment & future removal note
Small typo: “modlular” → “modular”. While cosmetic, comments live a long time and should be clean, especially since this file is marked for future deletion.
-* TODO: Once old webui code is refactored to new modlular fastify style, this plugin should be
+* TODO: Once old web UI code is refactored to the new modular Fastify style, this plugin should be📝 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.
| * TODO: Once old webui code is refactored to new modlular fastify style, this plugin should be | |
| * removed. | |
| * | |
| * @param fastify | |
| * @param opts | |
| * @return | |
| */ | |
| * TODO: Once old web UI code is refactored to the new modular Fastify style, this plugin should be | |
| * removed. | |
| * | |
| * @param fastify | |
| * @param opts | |
| * @return | |
| */ |
There was a problem hiding this comment.
@junhaoliao - it looks like the path variables are not being used in old webui code? Like they just come from settings instead? if so, should i remove path variables (i.e. log viewer directory etc... from here and env file)?
There was a problem hiding this comment.
path variables (i.e. log viewer directory etc... from here and env file)
I prefer them to come from a settings.json file if it's quick to implement
There was a problem hiding this comment.
I think there are already coming for settings.json, so i just deleted the env variables
| "scripts": { | ||
| "build": "tsc -b", | ||
| "build:watch": "npm run build -- --watch", | ||
| "build": "tsc", |
There was a problem hiding this comment.
can we update the docs to explain what each of these do?
There was a problem hiding this comment.
I updated docs to explain standalone. I feel like the other command are just feeders for the dev command, and you shouldn't actually run yourself. The fancy dev command just makes it easier to differentiate server logs and typescript build logs.
| "dev:start": "npm run build && fastify start --ignore-watch=.ts$ -w -l info -P dist/server/src/fastify-v2/app.js", | ||
| "dev": "npm run build && concurrently -k -p \"[{name}]\" -n \"TypeScript,App\" -c \"yellow.bold,cyan.bold\" \"npm:build:watch\" \"npm:dev:start\"", | ||
| "start": "npm run build && fastify start -l info dist/server/src/fastify-v2/app.js", | ||
| "standalone": "npm run build && node --env-file=.env dist/server/src/main.js", |
There was a problem hiding this comment.
do we need to also update the clp package scripts? is it fine to keep this prod for now?
There was a problem hiding this comment.
I dont believe the package actually runs the prod script. It just hardcodes node dist/server/src/main.js
| "build:watch": "npm run build -- -w", | ||
| "dev:start": "npm run build && fastify start --ignore-watch=.ts$ -w -l info -P dist/server/src/fastify-v2/app.js", | ||
| "dev": "npm run build && concurrently -k -p \"[{name}]\" -n \"TypeScript,App\" -c \"yellow.bold,cyan.bold\" \"npm:build:watch\" \"npm:dev:start\"", | ||
| "start": "npm run build && fastify start -l info dist/server/src/fastify-v2/app.js", |
There was a problem hiding this comment.
are these from a Fastify cli generated template project? e.g., via
npx fastify-cli generate myproject --lang=ts --esm
There was a problem hiding this comment.
I did not, i took them from demo repo. But after running that command, they look quite similiar.
There was a problem hiding this comment.
namings of files are a bit inconsistent in this directory:
- if we want the file names to match the "external" plugin name, we should name this
mongodb.tsorfastify-mongodb.ts. - otherwise,
rate-limit.tsshould becomerateLimit.tsorfastifyRateLimit.ts.
I prefer (2.). Also, I do think adding fastify as a prefix for official Fastity plugin configs will be an improvement
There was a problem hiding this comment.
I modified to match the external name. re this
Also, I do think adding fastify as a prefix
They are all from fastify at the moment, so i dont think we need that yet
There was a problem hiding this comment.
could you explain why we want to split a "v2" app from "v1"?
There was a problem hiding this comment.
I just didnt want this PR to include the refactor of the old webui code. As a result, I just put everything new in a different folder. This way its easy to tell which code needs to be refactored. Like i'm sure there are other improvements i could make based on demo repo to existing old code, and just wanted to keep separate. Also the testing code needs to be redone, and if i move the old code right now to new folder, I would have to delete the test code since it will fail on the combined app. Also some of the database configuration on the oldwebui could be removed, i just didnt do it, and its harmless for now.
| options: {...opts}, | ||
| }); | ||
|
|
||
| /* eslint-disable no-warning-comments */ |
There was a problem hiding this comment.
| /* eslint-disable no-warning-comments */ | |
| // eslint-disable-next-line no-warning-comments |
There was a problem hiding this comment.
since tap is specifically used for testing, would it be better to move it to test/utils/tap.ts? this will help avoid issues when we enforce test coverage on any source directories.
There was a problem hiding this comment.
It might try and run tests on the tap file? maybe we add sort of setting to only test .test?
There was a problem hiding this comment.
ah i see your point. i think any code is considered test code only if they are in a test hook.
did you try moving it into test/ though? if moving it doesn't break any tests, i prefer keeping it as test/tap.ts or test/utils/tap.ts since this is strictly test utils.
There was a problem hiding this comment.
okay i moved test, and added a patten to prevent it trying to test utils.
There was a problem hiding this comment.
this does not need to be address in the current PR:
what do you think about the other testing frameworks such as jest, mocha, or vitest? both of them seem more popular than tap. have we considered replacing tap with any?
There was a problem hiding this comment.
I think the whole testing infra needs to redone. Like we probably need seperate unit tests and integration tests. Like the current test is really a toy, and dosent do anything. I think when we get there, we can change framework if we want
There was a problem hiding this comment.
I think the whole testing infra needs to redone.
I agree
I think when we get there, we can change framework if we want
sounds good
There was a problem hiding this comment.
@coderabbitai create an issue to track testing infra improvement in the new-webui
There was a problem hiding this comment.
🧩 Analysis chain
I'll create an issue to track the testing infrastructure improvements needed for the new-webui component.
🏁 Script executed:
#!/bin/bash
# Create a GitHub issue to track improvements needed for testing infrastructure in the new-webui
gh issue create \
--title "Improve testing infrastructure for new-webui component" \
--body "# Testing Infrastructure Improvements for new-webui
## Background
The current testing setup for the new-webui component uses tap and is described as a 'toy test that doesn't do anything substantial'. As part of the ongoing refactoring to align with Fastify maintainer recommendations, the testing infrastructure needs to be completely redone.
## Requirements
- Set up proper separation between unit tests and integration tests
- Consider replacing tap with a more popular testing framework (potential alternatives: jest, mocha, vitest)
- Implement comprehensive testing for the new fastify-v2 app
- Address the TODO comment in \`components/log-viewer-webui/server/src/utils/tap.ts\` regarding testing setup
## References
- Originated from discussion in PR #899: https://github.com/y-scope/clp/pull/899
- Specific comment thread: https://github.com/y-scope/clp/pull/899#discussion_r2087526123
## Additional Notes
The integration tests may require the clp package to be running.
" \
--assignee "junhaoliao"
Length of output: 1144
@junhaoliao I've created an issue to track the testing infrastructure improvements for the new-webui component. The issue covers:
- Setting up proper separation between unit tests and integration tests
- Considering alternatives to tap (jest, mocha, vitest)
- Implementing comprehensive testing for the new fastify-v2 app
- Addressing the TODO comment in the tap.ts file
Here's the link to the issue: #903
(_/)
(•ᴗ•)
/ >📝 Task completed!
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
| CLP_DB_USER=clp-user | ||
| CLP_DB_PASS= | ||
|
|
||
| FASTIFY_CLOSE_GRACE_DELAY=1000 |
There was a problem hiding this comment.
can we configure this in the config json file since this is not credential related?
There was a problem hiding this comment.
I just removed it, and left the default hardcode. I highly doubt we would ever want to change this.
junhaoliao
left a comment
There was a problem hiding this comment.
thanks for cleaning the server up
i posted all my comments for a first iteration review
junhaoliao
left a comment
There was a problem hiding this comment.
for the PR title, would this be better?
refactor(new-webui): Enhance server implementation with Fastify plugins and toolchain.
| STREAMS_DATA_DIR=../../../build/clp-package/var/data/streams | ||
| LOG_VIEWER_DIR=../yscope-log-viewer/dist | ||
| # Must always set to production | ||
| # @see {@link https://www.youtube.com/watch?v=HMM7GJC5E2o} |
There was a problem hiding this comment.
given this, do we still need the if ("production" === process.env.NODE_ENV) check in src/routes/static.ts?
There was a problem hiding this comment.
I think we can decide later how to remove it when we refactor old code. I dont think it's really an issue though just to remove and run two clients. Another options is to pass a fastify option to disable it. But i think can be decided later when old code refactored
There was a problem hiding this comment.
I added commennt to testing issue.
|
|
||
| import {FastifyInstance} from "fastify"; | ||
| import {build as buildApplication} from "fastify-cli/helper.js"; | ||
| import {Test} from "tap"; |
There was a problem hiding this comment.
i'll find some linter rules for this in the future
| import {Test} from "tap"; | |
| import type {Test} from "tap"; |
| opts: AppPluginOptions | ||
| ) => { | ||
| const {sqlDbUser, sqlDbPass} = opts; | ||
| if ("test" !== process.env.NODE_ENV) { |
There was a problem hiding this comment.
if any value of process.env.NODE_ENV than production is an antipattern, it sounds like we shouldn't be launching the tests with NODE_ENV=test. if so, is there a clean way to skip registration of certain plugins?
There was a problem hiding this comment.
i'm fine leaving this as a future improvement item, but be sure to update #903 so we can revisit this consideration
There was a problem hiding this comment.
I think we definitely should not, but the current test requires this, so i left this as a future item when old code refactored. In general, i dont think we should be disabling any plugins. I think we should have integration test with the package running all the plugins, or just launch certain plugins individually and test them? Also unit tests with no plugins running. I think this can be decided more when we get to #903, but for simplicity for now i left it. A potential option in short term could also be to pass a fastify option instead of using NODE_ENV. However, this may just be another antipattern... I prefer just leaving it, and fixing once we come up with a real testing plan.
There was a problem hiding this comment.
I updated issue to remove these NODE_ENV references
| "lint:fix": "eslint --fix", | ||
| "start": "vite", | ||
| "antd": "VITE_USE_ANTD_APP=true npm run start" | ||
| "dev": "vite", |
There was a problem hiding this comment.
| "dev": "vite", | |
| "dev": "vite", |
| "dev": "vite", | ||
| "antd": "VITE_USE_ANTD_APP=true npm run dev" |
There was a problem hiding this comment.
i prefer leaving this unchanged in this PR and submit another one to also update the scripts in component/log-viewer-webui/package.json. if you agree, please create an issue to track / ask the ai bot to create one
There was a problem hiding this comment.
@coderabbitai please make issue to update client scripts to use dev instead of start
There was a problem hiding this comment.
🧩 Analysis chain
I'll create an issue to track updating the client scripts from "start" to "dev".
@davemarco I've created an issue to track updating the client scripts to use "dev" instead of "start". The issue is assigned to you and references this PR discussion.
🏁 Script executed:
#!/bin/bash
# Create a GitHub issue to track updating client scripts
gh issue create \
--title "refactor(new-webui): Update client scripts to use 'dev' instead of 'start'" \
--body "## Description
This issue is to track the task of updating the client scripts in \`components/log-viewer-webui/client/package.json\` to use 'dev' instead of 'start' for consistency with Fastify conventions.
## References
- Related PR: #899
- Discussion: https://github.com/y-scope/clp/pull/899#discussion_r2089957586
## Task
- Update the npm scripts in client package.json to use 'dev' for development environments instead of 'start'
- Ensure references to these scripts are updated in documentation
" \
--assignee davemarco
Length of output: 712
@davemarco I've created issue #906 to track updating the client scripts to use "dev" instead of "start": #906
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
junhaoliao
left a comment
There was a problem hiding this comment.
i submitted my final batch of comments
davemarco
left a comment
There was a problem hiding this comment.
responded to ur comments
| STREAMS_DATA_DIR=../../../build/clp-package/var/data/streams | ||
| LOG_VIEWER_DIR=../yscope-log-viewer/dist | ||
| # Must always set to production | ||
| # @see {@link https://www.youtube.com/watch?v=HMM7GJC5E2o} |
There was a problem hiding this comment.
I think we can decide later how to remove it when we refactor old code. I dont think it's really an issue though just to remove and run two clients. Another options is to pass a fastify option to disable it. But i think can be decided later when old code refactored
| opts: AppPluginOptions | ||
| ) => { | ||
| const {sqlDbUser, sqlDbPass} = opts; | ||
| if ("test" !== process.env.NODE_ENV) { |
There was a problem hiding this comment.
I think we definitely should not, but the current test requires this, so i left this as a future item when old code refactored. In general, i dont think we should be disabling any plugins. I think we should have integration test with the package running all the plugins, or just launch certain plugins individually and test them? Also unit tests with no plugins running. I think this can be decided more when we get to #903, but for simplicity for now i left it. A potential option in short term could also be to pass a fastify option instead of using NODE_ENV. However, this may just be another antipattern... I prefer just leaving it, and fixing once we come up with a real testing plan.
| opts: AppPluginOptions | ||
| ) => { | ||
| const {sqlDbUser, sqlDbPass} = opts; | ||
| if ("test" !== process.env.NODE_ENV) { |
There was a problem hiding this comment.
I updated issue to remove these NODE_ENV references
| // Only if the program is running in an interactive terminal. | ||
| if (process.stdout.isTTY) { | ||
| return { | ||
| level: "info", |
There was a problem hiding this comment.
I also changed the default log level from silent to info for production. This is how we had it before.
| "dev": "vite", | ||
| "antd": "VITE_USE_ANTD_APP=true npm run dev" |
There was a problem hiding this comment.
@coderabbitai please make issue to update client scripts to use dev instead of start
| STREAMS_DATA_DIR=../../../build/clp-package/var/data/streams | ||
| LOG_VIEWER_DIR=../yscope-log-viewer/dist | ||
| # Must always set to production | ||
| # @see {@link https://www.youtube.com/watch?v=HMM7GJC5E2o} |
There was a problem hiding this comment.
I added commennt to testing issue.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (4)
components/log-viewer-webui/server/.env (1)
1-4: Same concern already raised previously – see earlier discussion
KeepingNODE_ENV=productionin a committed.envis likely to override local workflows; please revisit the decision or move this to an example file.components/log-viewer-webui/server/src/main.ts (3)
42-46: 🧹 Nitpick (assertive)Complete the JSDoc
@returntagThe comment is still incomplete, making typedoc output inconsistent.
- * @return + * @return {Promise<void>} Resolves once the server is listening
62-63: 🧹 Nitpick (assertive)Use nullish-coalescing to avoid treating empty string as port 0
Number(process.env.PORT || DEFAULT_PORT)converts an empty string ("") to0, causing the app to bind to a random port.
Nullish-coalescing (??) keeps the default while allowing"0"if explicitly intended.- await app.listen({port: Number(process.env.PORT || DEFAULT_PORT)}); + await app.listen({port: Number(process.env.PORT ?? DEFAULT_PORT)});
49-57:⚠️ Potential issueWrap
app.close()in try/catch to avoid silent shutdown failuresLearning captured earlier indicates
app.close()may reject.
Without error handlingclose-with-gracewill force-exit the process, skipping logs and hindering post-mortem analysis.- async ({err}) => { + async ({err}) => { if (err) { app.log.error(err); } - await app.close(); + try { + await app.close(); + } catch (closeErr) { + app.log.error(closeErr, "Fastify failed to shut down gracefully"); + process.exitCode = 1; + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
components/log-viewer-webui/server/.env(1 hunks)components/log-viewer-webui/server/src/main.ts(1 hunks)components/log-viewer-webui/server/src/test/tap.ts(1 hunks)docs/src/dev-guide/components-log-viewer-webui.md(1 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/test/tap.tscomponents/log-viewer-webui/server/src/main.ts
🧠 Learnings (1)
components/log-viewer-webui/server/src/main.ts (3)
Learnt from: junhaoliao
PR: y-scope/clp#899
File: components/log-viewer-webui/server/src/main.ts:50-58
Timestamp: 2025-05-13T19:38:31.164Z
Learning: In Fastify applications, the `app.close()` method returns a Promise that can reject if errors occur during shutdown. Always wrap it in a try/catch block to properly handle shutdown errors and set the process exit code to prevent the application from hanging.
Learnt from: junhaoliao
PR: y-scope/clp#899
File: components/log-viewer-webui/server/src/main.ts:50-58
Timestamp: 2025-05-13T19:38:31.164Z
Learning: When using Fastify's `app.close()` method, always wrap it in a try/catch block to properly handle shutdown errors. The close() method returns a Promise that can reject if errors occur during the shutdown process (especially with plugins using onClose hooks), and unhandled rejections could cause the application to hang.
Learnt from: junhaoliao
PR: y-scope/clp#899
File: components/log-viewer-webui/server/src/main.ts:71-72
Timestamp: 2025-05-14T19:02:27.650Z
Learning: Node.js 22 supports top-level await, allowing developers to use await in the global scope without wrapping it in an async function.
🧬 Code Graph Analysis (2)
components/log-viewer-webui/server/src/test/tap.ts (1)
components/log-viewer-webui/server/src/fastify-v2/plugins/external/env.ts (1)
FastifyInstance(6-15)
components/log-viewer-webui/server/src/main.ts (2)
components/webui/server/main.js (1)
process(30-33)components/log-viewer-webui/server/src/fastify-v2/app.ts (1)
serviceApp(25-114)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: lint-check (macos-latest)
🔇 Additional comments (2)
docs/src/dev-guide/components-log-viewer-webui.md (2)
42-44:Details
✅ Verification successful
Confirm the production start script
The production command now usesnpm run startinstead ofnpm run prod. Validate that the"start"script incomponents/log-viewer-webui/server/package.jsoncorrectly launches the server in production mode.
🏁 Script executed:
#!/usr/bin/env bash # Check for "start" script in server/package.json grep -R '"start":' components/log-viewer-webui/server/package.jsonLength of output: 157
Verified production start script
Thestartscript incomponents/log-viewer-webui/server/package.jsonindeed runsnpm run buildfollowed byfastify start -l info dist/server/src/fastify-v2/app.js, which builds and launches the server in production mode. The documentation’s use ofnpm run startis correct.
36-38:Details
✅ Verification successful
Verify the updated dev script
The development command has been changed tonpm run dev. Please ensure thatcomponents/log-viewer-webui/server/package.jsondefines a corresponding"dev"script (e.g.,"dev": "fastify start -l info -w").Run this script to confirm:
🏁 Script executed:
#!/usr/bin/env bash # Verify "dev" script exists in server/package.json grep -R '"dev":' components/log-viewer-webui/server/package.jsonLength of output: 218
Dev script verified
Thedevscript is defined incomponents/log-viewer-webui/server/package.jsonas expected:"dev": "npm run build && concurrently -k -p \"[{name}]\" -n \"TypeScript,App\" -c \"yellow.bold,cyan.bold\" \"npm:build:watch\" \"npm:dev:start\""No further changes required.
| If you want to customize what host and port the server binds to, you can use the | ||
| environment variables in `components/log-viewer-webui/server/.env`. |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Clarify environment variable details
The docs refer to environment variables in server/.env but don’t specify which ones (e.g., FASTIFY_HOST, FASTIFY_PORT). Consider listing the exact variable names or linking to an example .env file for clarity.
🤖 Prompt for AI Agents
In docs/src/dev-guide/components-log-viewer-webui.md around lines 52 to 53, the
documentation mentions environment variables in server/.env but does not specify
which variables control the host and port. Update the text to explicitly list
the relevant environment variables such as FASTIFY_HOST and FASTIFY_PORT, or
provide a link to an example .env file that shows these variables, to improve
clarity for users customizing the server binding.
|
|
||
| To run the server as a standalone Node.js process: | ||
|
|
||
| ```shell | ||
| npm run standalone |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Validate the standalone mode addition
A new standalone section was added for npm run standalone. Ensure that:
components/log-viewer-webui/server/package.jsonincludes a"standalone"script.- Behaviour and prerequisites of standalone mode are documented (e.g., bypassing proxies).
🏁 Script executed:
#!/usr/bin/env bash
# Confirm "standalone" script exists
grep -R '"standalone":' components/log-viewer-webui/server/package.jsonLength of output: 156
🏁 Script executed:
#!/usr/bin/env bash
sed -n '40,60p' docs/src/dev-guide/components-log-viewer-webui.mdLength of output: 425
Document standalone mode prerequisites and behaviour
The standalone script is indeed defined in components/log-viewer-webui/server/package.json. To ensure users can run it successfully, expand the docs immediately after the “npm run standalone” block to include:
- Prerequisites
- A valid
.envfile incomponents/log-viewer-webui/server/with at leastHOSTandPORTset. - Node.js (e.g. v16+) installed.
- A valid
- Behaviour
- Runs
npm run buildthen launchesdist/server/src/main.jsdirectly, bypassing any HTTP proxy or load-balancer. - Listens on the host and port defined in the
.envfile.
- Runs
Example update in docs/src/dev-guide/components-log-viewer-webui.md (around lines 45–49):
To run the server as a standalone Node.js process:
```shell
npm run standalone+- Prerequisites:
-
- Ensure
components/log-viewer-webui/server/.envexists withHOSTandPORT.
- Ensure
-
- Requires Node.js v16+.
+- Behaviour:
-
- Builds the project, then runs
dist/server/src/main.jsdirectly.
- Builds the project, then runs
-
- Bypasses any HTTP proxy or load-balancer and listens on the configured host/port.
<details>
<summary>🤖 Prompt for AI Agents</summary>
In docs/src/dev-guide/components-log-viewer-webui.md around lines 45 to 49,
after the "npm run standalone" code block, add documentation for standalone mode
prerequisites and behaviour. Specify that users must have a valid .env file in
components/log-viewer-webui/server/ with HOST and PORT set, and Node.js v16+
installed. Describe that the script runs npm run build then launches
dist/server/src/main.js directly, bypassing HTTP proxies or load-balancers, and
listens on the configured host and port.
</details>
<!-- This is an auto-generated comment by CodeRabbit -->
| const appPath = path.join(import.meta.dirname, "../app.ts"); | ||
|
|
There was a problem hiding this comment.
import.meta.dirname is not standard – path resolution will break outside TS-node
import.meta.dirname is a convenience added by some TS runners, but Node ≥ 18 (and common bundlers) expose only import.meta.url.
When this helper is executed with plain Node or in the Jest/TAP runtime the expression will be undefined, causing the build to fail to locate app.ts.
-const appPath = path.join(import.meta.dirname, "../app.ts");
+import {fileURLToPath} from "node:url";
+const __dirname = path.dirname(fileURLToPath(import.meta.url));
+const appPath = path.join(__dirname, "../app.ts");📝 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.
| const appPath = path.join(import.meta.dirname, "../app.ts"); | |
| import { fileURLToPath } from "node:url"; | |
| const __dirname = path.dirname(fileURLToPath(import.meta.url)); | |
| const appPath = path.join(__dirname, "../app.ts"); |
🤖 Prompt for AI Agents
In components/log-viewer-webui/server/src/test/tap.ts around lines 13 to 14,
replace the non-standard usage of import.meta.dirname with a standard approach
using import.meta.url. Use URL and path utilities to convert import.meta.url to
a directory path, then join it with "../app.ts" to correctly resolve the app.ts
file location in all environments including plain Node and test runtimes.
| t.teardown(async () => { | ||
| console.log("Closing application."); | ||
| await app.close(); | ||
| }); |
There was a problem hiding this comment.
Un-handled promise rejection on teardown may mask test results
await app.close() can reject (e.g., if a plugin’s onClose hook throws).
When TAP tears down, an un-handled rejection will be reported as “process exited” rather than a clear failure message.
- t.teardown(async () => {
- console.log("Closing application.");
- await app.close();
- });
+ t.teardown(async () => {
+ console.log("Closing application.");
+ try {
+ await app.close();
+ } catch (err) {
+ t.fail(`Failed to close Fastify instance: ${err}`);
+ }
+ });📝 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.
| t.teardown(async () => { | |
| console.log("Closing application."); | |
| await app.close(); | |
| }); | |
| t.teardown(async () => { | |
| console.log("Closing application."); | |
| try { | |
| await app.close(); | |
| } catch (err) { | |
| t.fail(`Failed to close Fastify instance: ${err}`); | |
| } | |
| }); |
🤖 Prompt for AI Agents
In components/log-viewer-webui/server/src/test/tap.ts around lines 41 to 44, the
await app.close() call inside the teardown function can reject and cause an
unhandled promise rejection, masking test results. To fix this, wrap the await
app.close() call in a try-catch block and handle any errors by logging them or
failing the test explicitly, ensuring that any rejection is properly caught and
reported during teardown.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (6)
components/log-viewer-webui/server/src/fastify-v2/plugins/external/env.ts (3)
1-1: Removesort-keysESLint disable directive
The/* eslint-disable sort-keys */override suppresses key-ordering checks in your schema. Removing it and sorting keys (alphabetically or logically) will improve readability and maintainability.
46-48: Enforce non-empty database password
CLP_DB_PASScurrently lacks aminLengthconstraint, allowing empty strings. Consider addingminLength: 1to ensure non-empty credentials.
51-54: Add range constraints toRATE_LIMIT_MAX
To guard against misconfiguration, you may want to addminimum: 1(and optionallymaximum) so thatRATE_LIMIT_MAXstays within sensible bounds.components/log-viewer-webui/server/src/test/tap.ts (3)
13-13:⚠️ Potential issueFix non-standard import.meta.dirname usage
The
import.meta.dirnameproperty is non-standard and will break in environments outside TS-node. Node.js ≥18 only exposesimport.meta.url. This will cause path resolution to fail in standard Node.js runtimes, Jest/TAP environments, or during builds.Replace with the standard approach:
-const appPath = path.join(import.meta.dirname, "../app.ts"); +import {fileURLToPath} from "node:url"; +const __dirname = path.dirname(fileURLToPath(import.meta.url)); +const appPath = path.join(__dirname, "../app.ts");
41-44:⚠️ Potential issueHandle potential rejection from app.close() during teardown
The
app.close()method returns a Promise that can reject if errors occur during shutdown (especially if plugin onClose hooks throw exceptions). Unhandled rejections during teardown can mask test failures and cause the application to hang.Always wrap
app.close()in a try/catch block as learned from previous implementation:t.teardown(async () => { console.log("Closing application."); - await app.close(); + try { + await app.close(); + } catch (err) { + t.fail(`Failed to close Fastify instance: ${err}`); + } });
11-12: 🧹 Nitpick (assertive)Update TODO comment format
The TODO comment spans multiple lines, which is fine, but the formatting could be improved to match common documentation practices.
Consider updating the TODO comment format:
// eslint-disable-next-line no-warning-comments -// TODO: Setup testing for new webui fastify app in `fastify-v2/app.ts`. Testing will need -// to be split into unit tests and integration tests (require clp package running). +// TODO: Setup testing for new webui fastify app in `fastify-v2/app.ts`. +// Testing will need to be split into unit tests and integration tests +// (require clp package running).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
components/log-viewer-webui/server/src/fastify-v2/plugins/external/env.ts(1 hunks)components/log-viewer-webui/server/src/test/tap.ts(1 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/test/tap.tscomponents/log-viewer-webui/server/src/fastify-v2/plugins/external/env.ts
🧠 Learnings (1)
components/log-viewer-webui/server/src/test/tap.ts (2)
Learnt from: junhaoliao
PR: y-scope/clp#899
File: components/log-viewer-webui/server/src/main.ts:50-58
Timestamp: 2025-05-13T19:38:31.164Z
Learning: When using Fastify's `app.close()` method, always wrap it in a try/catch block to properly handle shutdown errors. The close() method returns a Promise that can reject if errors occur during the shutdown process (especially with plugins using onClose hooks), and unhandled rejections could cause the application to hang.
Learnt from: junhaoliao
PR: y-scope/clp#899
File: components/log-viewer-webui/server/src/main.ts:50-58
Timestamp: 2025-05-13T19:38:31.164Z
Learning: In Fastify applications, the `app.close()` method returns a Promise that can reject if errors occur during shutdown. Always wrap it in a try/catch block to properly handle shutdown errors and set the process exit code to prevent the application from hanging.
🧬 Code Graph Analysis (2)
components/log-viewer-webui/server/src/test/tap.ts (1)
components/log-viewer-webui/server/src/fastify-v2/plugins/external/env.ts (1)
FastifyInstance(6-15)
components/log-viewer-webui/server/src/fastify-v2/plugins/external/env.ts (2)
components/log-viewer-webui/server/src/fastify-v2/plugins/app/sample.ts (1)
FastifyInstance(31-33)components/webui/server/main.js (1)
process(30-33)
🔇 Additional comments (1)
components/log-viewer-webui/server/src/fastify-v2/plugins/external/env.ts (1)
28-33: DefaultPORTis correctly numeric
Good job defining the default value ofPORTas anumber, which aligns with the schema type.
| export const autoConfig = { | ||
| confKey: "config", | ||
| schema: schema, | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Add explicit type to autoConfig
For stronger type safety and IDE support, annotate autoConfig with the appropriate options interface (e.g., FastifyEnvOptions) from @fastify/env rather than relying on implicit types.
🤖 Prompt for AI Agents
In components/log-viewer-webui/server/src/fastify-v2/plugins/external/env.ts
around lines 58 to 61, the autoConfig object lacks an explicit type annotation.
To improve type safety and IDE support, import the FastifyEnvOptions interface
from @fastify/env and annotate autoConfig with this type. This ensures the
object conforms to the expected structure and helps catch type errors early.
| const config = () => { | ||
| // Register application with fastify-plugin to expose all decorators for testing | ||
| // purposes. | ||
| return { | ||
| skipOverride: true, | ||
| }; | ||
| }; |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Update configuration function return type
The configuration function lacks a return type annotation, which would improve code clarity and documentation.
Consider adding a return type annotation to the configuration function:
-const config = () => {
+const config = (): {skipOverride: boolean} => {
// Register application with fastify-plugin to expose all decorators for testing
// purposes.
return {
skipOverride: 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.
| const config = () => { | |
| // Register application with fastify-plugin to expose all decorators for testing | |
| // purposes. | |
| return { | |
| skipOverride: true, | |
| }; | |
| }; | |
| const config = (): { skipOverride: boolean } => { | |
| // Register application with fastify-plugin to expose all decorators for testing | |
| // purposes. | |
| return { | |
| skipOverride: true, | |
| }; | |
| }; |
🤖 Prompt for AI Agents
In components/log-viewer-webui/server/src/test/tap.ts around lines 20 to 26, the
config function is missing a return type annotation. Add an explicit return type
annotation to the config function to improve code clarity and documentation,
specifying the exact type of the returned object.
| import path from "node:path"; | ||
|
|
||
| import {FastifyInstance} from "fastify"; | ||
| import {build as buildApplication} from "fastify-cli/helper.js"; | ||
| import type {Test} from "tap"; |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Import types explicitly for better code clarity
The current import for FastifyInstance is not a type import, even though it's used only as a type in this file. This might lead to unnecessary code inclusion in the build output.
Consider using explicit type imports for better code clarity and tree-shaking:
import path from "node:path";
-import {FastifyInstance} from "fastify";
+import type {FastifyInstance} from "fastify";
import {build as buildApplication} from "fastify-cli/helper.js";
import type {Test} from "tap";📝 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.
| import path from "node:path"; | |
| import {FastifyInstance} from "fastify"; | |
| import {build as buildApplication} from "fastify-cli/helper.js"; | |
| import type {Test} from "tap"; | |
| import path from "node:path"; | |
| import type {FastifyInstance} from "fastify"; | |
| import {build as buildApplication} from "fastify-cli/helper.js"; | |
| import type {Test} from "tap"; |
🤖 Prompt for AI Agents
In components/log-viewer-webui/server/src/test/tap.ts around lines 3 to 7, the
import of FastifyInstance is not marked as a type import, which can cause
unnecessary code to be included in the build. Change the import statement to
explicitly import FastifyInstance as a type using the 'import type' syntax to
improve code clarity and enable better tree-shaking.
| const build = async (t: Test): Promise<FastifyInstance> => { | ||
| const argv = [appPath]; | ||
|
|
||
| const app = await buildApplication(argv, config()) as FastifyInstance; | ||
|
|
||
| console.log("Application built."); | ||
|
|
||
| t.teardown(async () => { | ||
| console.log("Closing application."); | ||
| await app.close(); | ||
| }); | ||
|
|
||
| return app; | ||
| }; |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider adding ready signal before returning the app instance
The current implementation doesn't await for the app to be fully ready before returning it, which might lead to race conditions if tests start using the app before all plugins are initialized.
Consider adding await app.ready() before returning the app instance to ensure all plugins are registered and the server is ready for testing:
const build = async (t: Test): Promise<FastifyInstance> => {
const argv = [appPath];
const app = await buildApplication(argv, config()) as FastifyInstance;
console.log("Application built.");
t.teardown(async () => {
console.log("Closing application.");
await app.close();
});
+ await app.ready();
+ console.log("Application ready.");
return app;
};📝 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.
| const build = async (t: Test): Promise<FastifyInstance> => { | |
| const argv = [appPath]; | |
| const app = await buildApplication(argv, config()) as FastifyInstance; | |
| console.log("Application built."); | |
| t.teardown(async () => { | |
| console.log("Closing application."); | |
| await app.close(); | |
| }); | |
| return app; | |
| }; | |
| const build = async (t: Test): Promise<FastifyInstance> => { | |
| const argv = [appPath]; | |
| const app = await buildApplication(argv, config()) as FastifyInstance; | |
| console.log("Application built."); | |
| t.teardown(async () => { | |
| console.log("Closing application."); | |
| await app.close(); | |
| }); | |
| await app.ready(); | |
| console.log("Application ready."); | |
| return app; | |
| }; |
🤖 Prompt for AI Agents
In components/log-viewer-webui/server/src/test/tap.ts around lines 34 to 47, the
build function returns the Fastify app instance without waiting for it to be
fully ready, which can cause race conditions in tests. To fix this, add an await
call to app.ready() before returning the app to ensure all plugins are
registered and the server is fully initialized before tests use it.
| "ClientDir": "../../client/dist", | ||
| "LogViewerDir": "../../yscope-log-viewer/dist", | ||
| "StreamFilesDir": "../../../../build/clp-package/var/data/streams", |
There was a problem hiding this comment.
It seems we still need to add one more level up (i.e., an additional ../) per path to get the paths resolved correctly.
Also, the . current directory is assumed to be where settings.json located.
I believe we should change rootDirname -> rootDirname = path.resolve(dirname, "../../../..") in src/routes/static.ts, given where the static.js is in dist now.
@coderabbitai create an issue to track this
There was a problem hiding this comment.
🧩 Analysis chain
@junhaoliao I'll first check the current implementation in the static.ts file and then create an issue to track this path resolution problem.
🏁 Script executed:
#!/bin/bash
# Search for the static.ts file to understand the current implementation
fd -t f "static.ts" components/log-viewer-webui/server/
# If found, examine the content, particularly looking for rootDirname
find components/log-viewer-webui/server/ -name "static.ts" -exec cat {} \;
Length of output: 1871
🏁 Script executed:
#!/bin/bash
# Create a GitHub issue to track the path resolution problem
gh issue create \
--title "Fix path resolution in settings.json and static.ts after server refactoring" \
--body "## Problem
After the server refactoring in PR #899, the path resolution in both \`settings.json\` and \`src/routes/static.ts\` needs adjustment due to the new directory structure.
## Issues Identified
### 1. settings.json paths need one more level up
The current paths in \`components/log-viewer-webui/server/settings.json\` are:
- \`\"ClientDir\": \"../../client/dist\"\`
- \`\"LogViewerDir\": \"../../yscope-log-viewer/dist\"\`
- \`\"StreamFilesDir\": \"../../../../build/clp-package/var/data/streams\"\`
These need to be adjusted to add one more \`../\` level:
- \`\"ClientDir\": \"../../../client/dist\"\`
- \`\"LogViewerDir\": \"../../../yscope-log-viewer/dist\"\`
- \`\"StreamFilesDir\": \"../../../../../build/clp-package/var/data/streams\"\`
### 2. rootDirname in src/routes/static.ts needs adjustment
The \`rootDirname\` variable should be changed to:
\`\`\`javascript
rootDirname = path.resolve(dirname, \"../../../..\")
\`\`\`
## Context
- The current directory is assumed to be where \`settings.json\` is located
- The server structure has changed due to the Fastify refactoring
- This affects static file serving and path resolution
## Related
- Original PR: #899
- Comment: https://github.com/y-scope/clp/pull/899#discussion_r2148837114
Reported by: @junhaoliao" \
--assignee "junhaoliao"
Length of output: 1407
@junhaoliao I've created GitHub issue #1014 to track the path resolution problems you identified.
Issue created: #1014
The issue covers both problems:
-
settings.json paths - Need one more
../level added to:ClientDir:../../client/dist→../../../client/distLogViewerDir:../../yscope-log-viewer/dist→../../../yscope-log-viewer/distStreamFilesDir:../../../../build/clp-package/var/data/streams→../../../../../build/clp-package/var/data/streams
-
static.ts rootDirname - Needs to be changed from:
const rootDirname = path.resolve(dirname, "../..");
to:
const rootDirname = path.resolve(dirname, "../../../..");
The issue is assigned to you and includes links back to this PR and comment for context.
| const RATE_LIMIT_MAX_REQUESTS = 3; | ||
| const RATE_LIMIT_TIME_WINDOW_MS = 500; |
There was a problem hiding this comment.
those values should be configurable via the environment variables
@coderabbitai create an issue to track this
There was a problem hiding this comment.
🧩 Analysis chain
@junhaoliao I'll create an issue to track making those rate limiting values configurable via environment variables.
🏁 Script executed:
#!/bin/bash
# Create GitHub issue for making rate limiting values configurable
gh issue create \
--title "Make rate limiting constants configurable via environment variables" \
--body "## Description
The rate limiting constants in \`components/log-viewer-webui/server/src/fastify-v2/app.ts\` are currently hardcoded and should be made configurable via environment variables for better flexibility in different deployment environments.
## Current Hardcoded Values
\`\`\`javascript
const RATE_LIMIT_MAX_REQUESTS = 3;
const RATE_LIMIT_TIME_WINDOW_MS = 500;
\`\`\`
These constants are used in the 404 Not Found handler rate limiting configuration to prevent URL enumeration attacks.
## Proposed Solution
Add environment variables to the existing configuration schema in \`src/fastify-v2/plugins/external/env.ts\`:
- \`RATE_LIMIT_404_MAX_REQUESTS\` (default: 3)
- \`RATE_LIMIT_404_TIME_WINDOW_MS\` (default: 500)
## References
- Original PR: https://github.com/y-scope/clp/pull/899
- Comment: https://github.com/y-scope/clp/pull/899#discussion_r2150299933
- Requested by: @junhaoliao" \
--assignee junhaoliao
Length of output: 1057
Perfect! I've created issue #1018 to track making the rate limiting constants configurable via environment variables: #1018
The issue documents the current hardcoded values (RATE_LIMIT_MAX_REQUESTS = 3 and RATE_LIMIT_TIME_WINDOW_MS = 500) and proposes adding them to the existing environment configuration schema. This will provide better flexibility for different deployment environments while maintaining the security benefits of rate limiting the 404 Not Found handler.
🧬 Code Graph Analysis Results
Possibly relevant snippets from components/log-viewer-webui/server/src/fastify-v2/plugins/external/env.ts (lines 6-15)
export interface FastifyInstance {
config: {
PORT: number;
HOST: string;
CLP_DB_USER: string;
CLP_DB_PASS: string;
RATE_LIMIT_MAX: number;
};
}This snippet defines the config interface on the FastifyInstance which includes environment-configurable values such as PORT, HOST, database credentials (CLP_DB_USER, CLP_DB_PASS), and a rate limit maximum (RATE_LIMIT_MAX). This is relevant because the main file app.ts uses fastify.config.CLP_DB_USER and fastify.config.CLP_DB_PASS and also has rate limit constants that the user comment suggests should be configurable via environment variables.
Possibly relevant snippets from components/log-viewer-webui/server/src/fastify-v2/plugins/app/sample.ts (lines 31-33)
export interface FastifyInstance {
ExamplePlugin: ReturnType<typeof createExamplePlugin>;
}This snippet extends the FastifyInstance interface with a plugin property. While it shows how plugins extend the Fastify instance, it is less directly relevant to the environment variable configurability mentioned in the user comment. It may provide context on how plugins are typed and registered.
These snippets provide context on how environment variables and configuration are typed and accessed on the Fastify instance, which relates to making values configurable via environment variables as requested in the user comment.
| CLP_DB_USER: string; | ||
| CLP_DB_PASS: string; | ||
|
|
||
| RATE_LIMIT_MAX: number; |
There was a problem hiding this comment.
we should also add RATE_LIMIT_MAX to .env
@coderabbitai create an issue to track this
There was a problem hiding this comment.
🧩 Analysis chain
@junhaoliao I'll create an issue to track adding RATE_LIMIT_MAX to the .env file.
🏁 Script executed:
#!/bin/bash
# Create a GitHub issue to track adding RATE_LIMIT_MAX to .env file
gh issue create \
--title "Add RATE_LIMIT_MAX environment variable to .env file" \
--body "## Description
The \`RATE_LIMIT_MAX\` environment variable is defined in the schema at \`components/log-viewer-webui/server/src/fastify-v2/plugins/external/env.ts\` with a default value of 100, but it's missing from the \`.env\` file. This makes it difficult for developers to configure the rate limiting behaviour.
## Requested by
@junhaoliao
## References
- PR: https://github.com/y-scope/clp/pull/899
- Comment: https://github.com/y-scope/clp/pull/899#discussion_r2150301809
## Acceptance Criteria
- [ ] Add \`RATE_LIMIT_MAX=100\` to \`components/log-viewer-webui/server/.env\`
- [ ] Add appropriate comments explaining the rate limit configuration" \
--assignee junhaoliao
Length of output: 803
Perfect! I've successfully created issue #1019 to track adding RATE_LIMIT_MAX to the .env file.
Issue created: #1019
The issue includes:
- Clear description of the missing environment variable
- Reference to the schema definition in
env.ts - Backlinks to this PR and comment
- Assigned to you as requested
- Acceptance criteria for implementation
This will help ensure the rate limiting configuration is properly documented for developers to customize.
🧬 Code Graph Analysis Results
Possibly relevant snippets from components/webui/server/main.js
At line range 30-33:
const {
CLP_DB_USER,
CLP_DB_PASS,
} = process.env;This snippet shows environment variables being read from process.env, which is related to how environment variables like RATE_LIMIT_MAX might be accessed or used in the application.
Possibly relevant snippets from components/log-viewer-webui/server/src/fastify-v2/plugins/app/sample.ts
At line range 31-33:
export interface FastifyInstance {
ExamplePlugin: ReturnType<typeof createExamplePlugin>;
}This snippet defines an extension of the FastifyInstance interface, similar to how the env.ts file extends FastifyInstance with a config property. It is relevant for understanding how plugins extend Fastify's instance typing.
| // Security | ||
| RATE_LIMIT_MAX: { | ||
| type: "number", | ||
| default: 100, |
There was a problem hiding this comment.
serving 100 requests in a minute is too few, considering we also serve the frontend with the Fastify server. each page load of the ingest page is 11 requests, not to mention the page auto fetches the ingest stats every 10 seconds, which yields an additional (60 seconds / 10 seconds * 3 requests) = 18 requests per minute per viewer.
we should extend this to something higher probably like 1000
@coderabbitai create an issue to track this
There was a problem hiding this comment.
🧩 Analysis chain
@junhaoliao I'll create an issue to track the rate limiting configuration problem you've identified.
🏁 Script executed:
#!/bin/bash
# Create a GitHub issue to track the rate limiting configuration problem
gh issue create \
--title "Rate limit too restrictive for frontend usage (100 req/min)" \
--body "## Problem
The current rate limit of 100 requests per minute is too restrictive for normal frontend usage of the log viewer web UI.
## Analysis
- Each page load of the ingest page requires 11 requests
- The page auto-fetches ingest stats every 10 seconds, resulting in an additional 18 requests per minute per viewer (60s ÷ 10s × 3 requests = 18 req/min)
- This means a single user can easily exceed 100 requests per minute during normal usage
## Proposed Solution
Increase the rate limit to 1000 requests per minute to accommodate legitimate frontend usage.
## Context
This issue was identified during code review of PR #899.
**References:**
- PR: https://github.com/y-scope/clp/pull/899
- Comment: https://github.com/y-scope/clp/pull/899#discussion_r2150313021
- Requested by: @junhaoliao" \
--assignee junhaoliao
Length of output: 960
Perfect! I've successfully created issue #1020 to track the rate limiting configuration problem.
Issue created: #1020
Your analysis is spot-on - with 11 requests per page load plus 18 requests per minute for auto-fetching stats, the current 100 req/min limit would definitely interfere with normal usage. The issue is now assigned to you and includes all the context and your suggested solution to increase the limit to 1000 requests per minute.
🐰✨
/| /|
( ✌️ ✌️ )
|()|
O(")(")
🧬 Code Graph Analysis Results
Relevant snippet from components/webui/server/main.js (lines 30-33)
const {
CLP_DB_USER,
CLP_DB_PASS,
} = process.env;- This snippet shows environment variables
CLP_DB_USERandCLP_DB_PASSbeing read fromprocess.env. It is relevant because theenv.tsfile defines these variables in its schema and configuration.
Relevant snippet from components/log-viewer-webui/server/src/fastify-v2/plugins/app/sample.ts (lines 31-33)
export interface FastifyInstance {
ExamplePlugin: ReturnType<typeof createExamplePlugin>;
}- This snippet extends the
FastifyInstanceinterface with a plugin property. It is relevant as it shows a pattern of augmenting the Fastify instance interface, similar to howenv.tsextends it with aconfigproperty containing environment variables includingRATE_LIMIT_MAX.
No other snippets appear directly relevant to the environment configuration or rate limiting settings in env.ts.
Description
PR refactors existing server code to match fastify maintainers demo application. If someone wants to learn fastify i also recommend this video
Note this PR dosent add any features, and does not migrate the old webui to new format. It just sets up a new folder for future new webui code. Eventually the old webui code should be ported in a future PR.
Some changes include the following:
Better modularity - previously each plugin maintained a separate connection to the database, now its easier
to import an application-wide shared database pool into any plugin.
Better package start commands - I had some trouble when testing the server PR with the current package commands. I decided to just copy their recommended commands.
Adds fastify helper repos to reduce code( environment var parser, sensible http request/reply templates, test boilerplate) - I removed our own environment parsing code and the templates should make writting route code easier.
Security - adds fastify rate limiter plugin.
Graceful termination - adds plugin to ensure graceful shutdown on exit.
Checklist
breaking change.
Validation performed
I tested commands and test runs. I have not yet retested the package, but will do so before merging.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Documentation
Chores