build(log-viewer-webui): Migrate server codebase to TypeScript and update dependencies.#647
Conversation
…date dependencies.
WalkthroughThis pull request restructures task definitions and consolidates log viewer web UI components. The changes rename tasks (e.g., “log-viewer-webui-clients” to “log-viewer-webui”), update checksum file references, and adjust CLI commands. The server component now transitions to TypeScript, with new configuration files, build scripts, and stricter type safety across tests, routes, plugins, and utilities. Legacy JavaScript files have been removed and replaced by their TypeScript counterparts. Additionally, ESLint and TypeScript configurations have been introduced or updated, and improvements have been made to error handling and type annotations in plugins such as the S3Manager. Changes
Possibly related PRs
Suggested reviewers
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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: 1
🧹 Nitpick comments (7)
components/log-viewer-webui/server/src/utils.ts (1)
9-11: Consider guarding against negative wait times.
If a negative value is accidentally passed, it will still schedule a timeout with a negative delay, effectively defaulting to zero. You may want to clamp the input to non-negative values to avoid unexpected cases.-const sleep = (seconds: number): Promise<void> => new Promise((resolve) => { - setTimeout(resolve, seconds * MILLIS_PER_SECOND); -}); +const sleep = (seconds: number): Promise<void> => new Promise((resolve) => { + const clampedSeconds = Math.max(0, seconds); + setTimeout(resolve, clampedSeconds * MILLIS_PER_SECOND); +});components/log-viewer-webui/server/src/app.ts (1)
15-19: Consider securing credentials [en-CA]Defining
sqlDbUserandsqlDbPasson theAppPropsinterface is straightforward, but ensure these values are sourced and stored securely (for instance, using environment variables) to mitigate the risk of accidental disclosure.components/log-viewer-webui/server/src/routes/query.ts (1)
22-29: Validate numeric fields more strictly
Currently,logEventIdxis only constrained to be an integer. Consider adding minimum or maximum bounds to ensure the value remains within valid ranges, preventing malformed requests.components/log-viewer-webui/server/src/DbManager.ts (1)
55-55: Polling interval might be too frequent
A 0.5ms interval inJOB_COMPLETION_STATUS_POLL_INTERVAL_MILLIScould cause unnecessary load on both the server and database. Consider increasing this interval or implementing a more efficient notification-based approach to reduce resource usage.-const JOB_COMPLETION_STATUS_POLL_INTERVAL_MILLIS = 0.5; +const JOB_COMPLETION_STATUS_POLL_INTERVAL_MILLIS = 500;components/log-viewer-webui/server/src/routes/example.ts (1)
15-23: Optional validation enhancement for GET route
You're returning a greeting for any string provided asname. For added reliability, consider adding further constraints (e.g. non-empty) to avoid unexpected edge cases.components/log-viewer-webui/server/package.json (2)
7-8: Build scripts added.
Thebuildandbuild:watchscripts will streamline TypeScript compilation. Consider verifying these scripts in CI to prevent build regressions.
38-40: Nodemon and testing tools.
Continuing to use nodemon for reloading is standard. Ensure that your TypeScript build paths are consistently referenced in watch commands.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
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)
Taskfile.yml(6 hunks)components/clp-package-utils/clp_package_utils/scripts/start_clp.py(2 hunks)components/log-viewer-webui/server/package.json(1 hunks)components/log-viewer-webui/server/src/DbManager.js(0 hunks)components/log-viewer-webui/server/src/DbManager.ts(1 hunks)components/log-viewer-webui/server/src/app.test.ts(1 hunks)components/log-viewer-webui/server/src/app.ts(2 hunks)components/log-viewer-webui/server/src/main.js(0 hunks)components/log-viewer-webui/server/src/main.ts(1 hunks)components/log-viewer-webui/server/src/routes/example.js(0 hunks)components/log-viewer-webui/server/src/routes/example.ts(1 hunks)components/log-viewer-webui/server/src/routes/query.ts(1 hunks)components/log-viewer-webui/server/src/routes/static.ts(2 hunks)components/log-viewer-webui/server/src/utils.js(0 hunks)components/log-viewer-webui/server/src/utils.ts(1 hunks)components/log-viewer-webui/server/tsconfig.json(1 hunks)
💤 Files with no reviewable changes (4)
- components/log-viewer-webui/server/src/utils.js
- components/log-viewer-webui/server/src/routes/example.js
- components/log-viewer-webui/server/src/main.js
- components/log-viewer-webui/server/src/DbManager.js
✅ Files skipped from review due to trivial changes (1)
- components/log-viewer-webui/server/tsconfig.json
🧰 Additional context used
📓 Path-based instructions (8)
components/log-viewer-webui/server/src/utils.ts (1)
Pattern **/*.{cpp,hpp,java,js,jsx,ts,tsx}: - Prefer false == <expression> rather than !<expression>.
components/log-viewer-webui/server/src/routes/static.ts (1)
Pattern **/*.{cpp,hpp,java,js,jsx,ts,tsx}: - Prefer false == <expression> rather than !<expression>.
components/log-viewer-webui/server/src/routes/example.ts (1)
Pattern **/*.{cpp,hpp,java,js,jsx,ts,tsx}: - Prefer false == <expression> rather than !<expression>.
components/log-viewer-webui/server/src/routes/query.ts (1)
Pattern **/*.{cpp,hpp,java,js,jsx,ts,tsx}: - Prefer false == <expression> rather than !<expression>.
components/log-viewer-webui/server/src/app.ts (1)
Pattern **/*.{cpp,hpp,java,js,jsx,ts,tsx}: - Prefer false == <expression> rather than !<expression>.
components/log-viewer-webui/server/src/main.ts (1)
Pattern **/*.{cpp,hpp,java,js,jsx,ts,tsx}: - Prefer false == <expression> rather than !<expression>.
components/log-viewer-webui/server/src/DbManager.ts (1)
Pattern **/*.{cpp,hpp,java,js,jsx,ts,tsx}: - Prefer false == <expression> rather than !<expression>.
components/log-viewer-webui/server/src/app.test.ts (1)
Pattern **/*.{cpp,hpp,java,js,jsx,ts,tsx}: - Prefer false == <expression> rather than !<expression>.
🔇 Additional comments (31)
components/log-viewer-webui/server/src/utils.ts (3)
1-1: Constant name is clear and self-explanatory.
This constant accurately conveys its purpose, aiding code readability.
3-8: Good use of JSDoc comment.
The documentation is clear and informative, enhancing code maintainability.
13-13: Export additional utilities as needed.
Currently, only sleep is exported. If other utility constants or functions are needed, consider adding them here for streamlined imports.
components/log-viewer-webui/server/src/app.ts (3)
1-5: Good use of named imports for type-safety [en-CA]
Explicitly importing fastify, FastifyInstance, and FastifyServerOptions is indeed the recommended approach for TypeScript projects, as it clearly communicates the contract used by the code and helps avoid namespace collisions in larger codebases.
24-28: Documentation is aligned with the new function signature [en-CA]
The updated docstrings properly reflect the new app function signature and parameters. This helps maintain clarity and reduces confusion.
34-34: Returning a Fastify instance is consistent [en-CA]
Returning the instantiated server matches the documented return type of Promise<FastifyInstance>, which provides clear expectations for downstream consumers.
components/log-viewer-webui/server/src/routes/query.ts (2)
31-31: Good adherence to coding guidelines
Your usage of if (false === EXTRACT_JOB_TYPES.includes(extractJobType)) is consistent with the specified preference of using false === <expression>.
9-11: 🛠️ Refactor suggestion
Use the correct extension for DbManager imports
It appears that the file you are importing is now DbManager.ts rather than DbManager.js. Please update these imports to reflect the new .ts extension to avoid confusion and potential runtime issues.
-} from "../DbManager.js";
+} from "../DbManager.js"; // Change ".js" to ".ts" if the file is indeed DbManager.tsLikely invalid or redundant comment.
components/log-viewer-webui/server/src/DbManager.ts (1)
227-234: Ensure credentials are not logged or exposed
When registering MySQL with connectionString, watch for unintended logging. Ensure that the credentials (user/password) are protected, particularly if you log connection details.
components/log-viewer-webui/server/src/app.test.ts (1)
7-7: Integration test approach looks sound
Adopting await tap.test is a valid upgrade and ensures async behaviour is properly handled. This also aligns with typical test patterns in TypeScript-based Fastify projects.
components/log-viewer-webui/server/src/routes/example.ts (1)
25-33: Schema-based POST route is well-structured
The route properly ensures name is provided. This is a straightforward pattern for simple demonstration routes.
components/log-viewer-webui/server/src/main.ts (1)
69-74: Consistent environment validation approach
Your conditional uses the recommended false === isKnownNodeEnv(NODE_ENV). This aligns with your coding guidelines and improves readability by explicitly checking for disallowed values.
components/clp-package-utils/clp_package_utils/scripts/start_clp.py (2)
906-909: Ensure consistent usage of the renamed directory.
It appears there is still a reference to “log_viewer_webui” on line 909 instead of “log-viewer-webui”. Confirm that all references to the old directory name have been updated.
962-962: Double-check the node version alignment.
You are specifying node-22 here. Confirm that the environment and other related scripts align with Node.js 22 and that no references to an older or different Node.js version remain in the codebase.
components/log-viewer-webui/server/src/routes/static.ts (2)
1-1: Good addition of type safety.
This import of FastifyPluginAsync clarifies the contract for the routes plugin. This is a helpful enhancement for type-checking.
14-16: Well-structured route registration.
Using false === path.isAbsolute(...) is consistent with your project’s guidelines. Great job following the custom styling for boolean checks.
components/log-viewer-webui/server/package.json (8)
5-5: Entry point updated to TypeScript.
Referencing src/main.ts provides a clearer path for TypeScript compilation. Ensure all internal references are updated accordingly.
11-12: Production start changes.
Running the production script from dist/src/main.js is correct for a TypeScript build flow. Confirm the environment variables and logging level for production continue to be set as intended.
19-31: Dependency updates.
Upgrading @fastify/mongodb, @fastify/mysql, and others in conjunction with TypeScript is a good practice. Double-check compatibility with any older consumer modules before merging.
34-36: Essential devDependencies for TypeScript.
Adding the TypeScript ESLint plugins and concurrently will help with both linting and parallel build runs. Good improvement.
43-43: Root-level ESLint configuration.
Setting root: true helps unify linting across the codebase. This is helpful for multi-level TypeScript projects.
45-46: Extending React and TypeScript configs.
Combining yscope/react and yscope/typescript is an effective approach for consistent styling and best practices in a full-stack environment.
48-51: Ignoring transpiled and third-party directories.
Ignoring dist/ and node_modules/ is standard to avoid linting extraneous directories. This keeps your linting scope tight.
52-58: Fine-tuning ESLint rules.
Disabling @typescript-eslint/require-await and turning off new-cap can reduce false positives in specific coding styles. If concurrency logic is introduced, consider re-evaluating require-await.
Taskfile.yml (7)
87-87: Renamed MD5 file reference.
Switching from log-viewer-webui-clients.md5 to log-viewer-webui.md5 makes sense given the consolidated naming. Make sure all scripts that reference the old checksum file are updated.
109-109: Added log-viewer-webui as a dependency.
Ensuring that log-viewer-webui is built prior to packaging is a sensible approach. This should maintain coherence in the final package.
158-159: New rsync for log-viewer-webui build output.
Copying the compiled artifacts from log-viewer-webui into var/www/log-viewer-webui is logical. Confirm that partial or incremental updates do not break older symlinks or references.
161-161: Installing server dependencies.
Running npm clean-install ensures a fresh installation, which is good for reproducibility. Confirm no local overrides (like dev scripts) are needed for the production environment.
210-210: Renamed task: log-viewer-webui.
Renaming from log-viewer-webui-clients to log-viewer-webui clarifies its broader scope. This consolidates both client and server building under one task.
221-225: Added TypeScript sources for the server.
Including server/package.json, server/package-lock.json, server/settings.json, and server/src/**/*.ts plus the tsconfig.json ensures the server portion is fully tracked. This is a good step for maintainability.
243-250: Commands to build and copy server output.
Building TypeScript into server/dist then syncing to the output directory is well-organized. Validate that the outDir in tsconfig.json points to dist to avoid confusion.
| import dotenv from "dotenv"; | ||
| import process from "node:process"; | ||
|
|
||
| import app from "./app.js"; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Confirm usage of the TypeScript entry point
The file app.js may already be migrated to TypeScript (e.g. app.ts). If so, please ensure consistency in your import statement to avoid confusion.
-import app from "./app.js";
+import app from "./app.js"; // Change to ".ts" if the file is named app.tsCommittable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
We specified "moduleResolution": "node16" in tsconfig.json and therefore the filename extension of .js is expected.
There was a problem hiding this comment.
@junhaoliao, that makes sense. Thanks for clarifying how the Node16 module resolution impacts the expected file extension for imports. Let me know if there’s anything else I can help with!
✏️ Learnings added
Learnt from: junhaoliao
PR: y-scope/clp#647
File: components/log-viewer-webui/server/src/main.ts:8-8
Timestamp: 2024-12-31T19:19:54.921Z
Learning: When "moduleResolution": "node16" is used in a TypeScript project's tsconfig.json, the import file extension is typically expected to be ".js" (or ".cjs"/".mjs"), even if the source code is originally in TypeScript (.ts). This is a part of node16’s resolution behavior.
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.
| "new-cap": [ | ||
| "off" | ||
| ] |
There was a problem hiding this comment.
To avoid raising errors on Typebox type creations (e.g., Type.Enum()).
| "@typescript-eslint/require-await": [ | ||
| "off" | ||
| ], |
There was a problem hiding this comment.
We currently implemented all Fastify plugin callbacks as async functions regardless whether there're async operations within. Alternatively, for the plugins that do not require async, we can remove the async specification and re-enable this ESLint rule.
| "{{.G_LOG_VIEWER_WEBUI_BUILD_DIR}}/yscope-log-viewer" | ||
| "{{.OUTPUT_DIR}}/var/www/log_viewer_webui/" | ||
| "{{.G_LOG_VIEWER_WEBUI_BUILD_DIR}}/" | ||
| "{{.OUTPUT_DIR}}/var/www/log-viewer-webui" |
There was a problem hiding this comment.
Note log_viewer_webui is renamed as log-viewer-webui here.
| node_path = str(container_log_viewer_webui_dir / "server" / "node_modules") | ||
| settings_json_path = ( | ||
| get_clp_home() / "var" / "www" / "log_viewer_webui" / "server" / "settings.json" | ||
| get_clp_home() / "var" / "www" / "log-viewer-webui" / "server" / "dist" / "settings.json" |
There was a problem hiding this comment.
settings.json is now part of the build assets so the path is changed.
|
|
||
| test("Tests the example routes", async (t) => { | ||
| const server = await app({}); | ||
| await tap.test("Tests the example routes", async (t) => { |
There was a problem hiding this comment.
This change is needed because ESLint does not know test() is defined in the global scope in the context of tap runs.
There was a problem hiding this comment.
can we extend the ESLint config instead with a ignore fix for this, instead of changing code with await
There was a problem hiding this comment.
Added Typebox as a Fastify type provider. Also specified the schemas for the routes.
There was a problem hiding this comment.
- Added Typebox as a Fastify type provider.
- Also specified the schemas for the routes.
- Also removed POST body value checks in favour of TypeBox's schema check.
| const {extractJobType, logEventIdx, streamId} = req.body; | ||
| if (false === EXTRACT_JOB_TYPES.includes(extractJobType)) { | ||
| resp.code(StatusCodes.BAD_REQUEST); | ||
| throw new Error(`Invalid extractJobType="${extractJobType}".`); | ||
| } | ||
|
|
||
| if ("string" !== typeof streamId || 0 === streamId.trim().length) { |
There was a problem hiding this comment.
The empty string check is now done by Type.String({minLength: 1}) though not trimmed. If needed, we can define a Regex check instead.
| throw new Error("\"streamId\" must be a non-empty string."); | ||
| } | ||
|
|
||
| const sanitizedLogEventIdx = Number(logEventIdx); |
There was a problem hiding this comment.
This sanitization seems not needed in the first place (even before we introduce TS)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
components/log-viewer-webui/server/tsconfig.json (2)
15-30: Consider reorganizing strict type-checking options.The strict type-checking configuration is comprehensive, but there are some suggestions for improvement:
- The
"strict": truesetting at line 30 is redundant since all individual strict flags are already enabled.- Consider grouping related options together for better readability.
Apply this diff to reorganize the strict options:
- "alwaysStrict": true, - "exactOptionalPropertyTypes": true, - "noFallthroughCasesInSwitch": true, - "noImplicitAny": true, - "noImplicitOverride": true, - "noImplicitReturns": true, - "noImplicitThis": true, - "noUncheckedIndexedAccess": true, - "noUnusedLocals": true, - "noUnusedParameters": true, - "strictBindCallApply": true, - "strictFunctionTypes": true, - "strictNullChecks": true, - "strictPropertyInitialization": true, - "useUnknownInCatchVariables": true, - "strict": true + // Enable all strict type-checking options + "strict": true, + + // Additional strict checks + "noUncheckedIndexedAccess": true, + "exactOptionalPropertyTypes": true, + "useUnknownInCatchVariables": true, + + // Code quality checks + "noUnusedLocals": true, + "noUnusedParameters": true, + "noFallthroughCasesInSwitch": true, + "noImplicitReturns": true, + "noImplicitOverride": true
32-34: Consider adding explicit exclusions for test files.The current configuration includes all files under
src/**/*but doesn't explicitly exclude test files. This might lead to TypeScript attempting to compile test files during the production build.Add an
excludesection to prevent compiling test files:"include": [ "src/**/*" - ] + ], + "exclude": [ + "src/**/*.test.ts", + "src/**/*.spec.ts", + "src/**/__tests__/*" + ]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
components/log-viewer-webui/server/tsconfig.json(1 hunks)
🔇 Additional comments (1)
components/log-viewer-webui/server/tsconfig.json (1)
2-13: LGTM! Compiler options are well configured for a Node.js server.
The basic compiler options are appropriately configured with modern ECMAScript targets and Node.js-specific module resolution settings.
|
@haiqi96 Could you help check if the CLP package changes make sense to you? Thanks |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
components/log-viewer-webui/server/src/typings/DbManager.ts (2)
7-11: Use explicit numerical values for the enum to avoid reordering issues
If the order of the constants changes, it can produce unexpected values. Defining explicit numeric values (e.g.,SEARCH_OR_AGGREGATION = 0, EXTRACT_IR = 1, EXTRACT_JSON = 2) can help maintain stable references.
16-19: Efficient data structure
Object.freeze([...])is fine for preventing modifications, but if you repeatedly check membership, consider using aSetfor faster lookups.components/log-viewer-webui/server/src/routes/query.ts (1)
8-9: Separate import statements for improved clarity
Consider movingQUERY_JOB_TYPEandEXTRACT_JOB_TYPESimports to the same line or grouping them in a single statement for clarity.components/log-viewer-webui/server/src/DbManager.ts (3)
114-120: Reusability of connection strings
Creating the connection strings directly here is straightforward, but consider centralizing configuration logic if multiple plugins share them.
156-181: Separate config logic
You handle different job configurations based on thejobType. For increased maintainability, consider refactoring these conditionals into separate helper functions or a strategy pattern.
228-264: Infinite loop risk
Thewhile(true)loop, combined with occasional status polling, can hang if the job never finishes. Although handled by repeated checks, you might want configurable timeouts to avoid indefinite loops.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
components/log-viewer-webui/server/src/DbManager.ts(1 hunks)components/log-viewer-webui/server/src/main.ts(1 hunks)components/log-viewer-webui/server/src/routes/query.ts(1 hunks)components/log-viewer-webui/server/src/typings/DbManager.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
components/log-viewer-webui/server/src/main.ts (1)
Pattern **/*.{cpp,hpp,java,js,jsx,ts,tsx}: - Prefer false == <expression> rather than !<expression>.
components/log-viewer-webui/server/src/routes/query.ts (1)
Pattern **/*.{cpp,hpp,java,js,jsx,ts,tsx}: - Prefer false == <expression> rather than !<expression>.
components/log-viewer-webui/server/src/typings/DbManager.ts (1)
Pattern **/*.{cpp,hpp,java,js,jsx,ts,tsx}: - Prefer false == <expression> rather than !<expression>.
components/log-viewer-webui/server/src/DbManager.ts (1)
Pattern **/*.{cpp,hpp,java,js,jsx,ts,tsx}: - Prefer false == <expression> rather than !<expression>.
🔇 Additional comments (16)
components/log-viewer-webui/server/src/typings/DbManager.ts (4)
27-34: Enum organization
These statuses match the code well and provide clarity. This looks good.
39-43: Ease of further usage
When adding or removing statuses from QUERY_JOB_STATUS_WAITING_STATES, ensure existing logic in other files properly handles each possible status.
50-55: Descriptive naming
The QUERY_JOBS_TABLE_COLUMN_NAMES enum is straightforward, ensuring consistent usage of column names throughout the code.
58-63: Interface aligns with table schema
The property-index signatures match the intended column usage, promoting robust type checking against your table columns.
components/log-viewer-webui/server/src/routes/query.ts (5)
1-1: Import modules
The imports reflect the shift to TypeScript and schema validation with TypeBox. This enhances type safety in route definitions.
Also applies to: 4-6
17-19: Correct usage of typed Fastify instance
Using app.withTypeProvider<TypeBoxTypeProvider>() is a good approach for stricter type checking of schemas.
20-27: Comprehensive schema definitions
Defining request body schemas with TypeBox ensures minimal boilerplate while maintaining strong runtime validation.
28-28: Consistent usage of "false ==="
This check complies with the coding guidelines, avoiding the !expression pattern. Good job.
37-37: Robust error handling
Errors are thrown with meaningful messages and correct status codes, helping users debug invalid scenarios easily.
Also applies to: 43-43, 51-51, 56-56, 62-62
components/log-viewer-webui/server/src/main.ts (7)
1-5: DOTENV usage
Loading environment variables with dotenv is a standard practice, ensuring local overrides via .env files.
11-14: Use of a default Node environment
Defining a default environment (e.g., development) is a solid fallback strategy.
18-27: Logger configuration map
Using environment-based logger templates is clean and easily adjustable per environment.
35-41: Type guard usage
isKnownNodeEnv is well-structured, preventing invalid environment strings from slipping through.
43-51: Explicit interface for environment variables
Ensuring each required env var has a type reduces the risk of runtime errors due to missing configurations.
59-99: Graceful handling of missing environment variables
You log a helpful message if NODE_ENV is invalid and throw an error for other mandatory vars, striking a good balance of clarity and strictness.
104-125: Server initialization strategy
Wrapping the server startup in main() and calling it at the end makes your code more testable and maintainable.
| /** | ||
| * Interval in milliseconds for polling the completion status of a job. | ||
| */ | ||
| const JOB_COMPLETION_STATUS_POLL_INTERVAL_MILLIS = 0.5; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Clarify polling interval
The value 0.5 might be interpreted as half a millisecond (i.e., 0.5 ms), leading to extremely high polling frequency. If you intended half a second, consider using 500 (milliseconds).
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
components/log-viewer-webui/server/src/typings/DbManager.ts (1)
50-55: Consider specifying numeric values for the enum.Setting explicit numeric values for the
QUERY_JOBS_TABLE_COLUMN_NAMESenum might help prevent unintentional changes in values if the enum's order is ever altered. Although not required, it can be a small safeguard.components/log-viewer-webui/server/src/DbManager.ts (1)
230-264: Consider adding a maximum wait time or retry limit.The
while (true)loop may run forever if a job never completes. Adding a timeout or a maximum number of retries could be beneficial to avoid indefinite polling.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
components/log-viewer-webui/server/src/DbManager.ts(1 hunks)components/log-viewer-webui/server/src/main.ts(1 hunks)components/log-viewer-webui/server/src/routes/query.ts(1 hunks)components/log-viewer-webui/server/src/typings/DbManager.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
components/log-viewer-webui/server/src/routes/query.ts (1)
Pattern **/*.{cpp,hpp,java,js,jsx,ts,tsx}: - Prefer false == <expression> rather than !<expression>.
components/log-viewer-webui/server/src/main.ts (1)
Pattern **/*.{cpp,hpp,java,js,jsx,ts,tsx}: - Prefer false == <expression> rather than !<expression>.
components/log-viewer-webui/server/src/typings/DbManager.ts (1)
Pattern **/*.{cpp,hpp,java,js,jsx,ts,tsx}: - Prefer false == <expression> rather than !<expression>.
components/log-viewer-webui/server/src/DbManager.ts (1)
Pattern **/*.{cpp,hpp,java,js,jsx,ts,tsx}: - Prefer false == <expression> rather than !<expression>.
📓 Learnings (1)
components/log-viewer-webui/server/src/main.ts (1)
Learnt from: junhaoliao
PR: y-scope/clp#647
File: components/log-viewer-webui/server/src/main.ts:8-8
Timestamp: 2024-12-31T19:19:55.032Z
Learning: When "moduleResolution": "node16" is used in a TypeScript project's tsconfig.json, the import file extension is typically expected to be ".js" (or ".cjs"/".mjs"), even if the source code is originally in TypeScript (.ts). This is a part of node16’s resolution behavior.
🔇 Additional comments (4)
components/log-viewer-webui/server/src/typings/DbManager.ts (1)
7-19: Enums and Sets are well-structured, good job!
Defining separate enums for job types and statuses, along with dedicated 'Set' constants, enhances code clarity and maintainability. Keep in mind to document any future additions for new job types or statuses in the same style.
components/log-viewer-webui/server/src/routes/query.ts (1)
30-30: Consistent boolean checks.
Using if (false === EXTRACT_JOB_TYPES.has(extractJobType)) aligns with your coding guideline of preferring false === expression. Great work maintaining consistency!
components/log-viewer-webui/server/src/main.ts (1)
89-94: Confirm environment configuration.
Ensuring the default environment is used if NODE_ENV is undefined or unrecognised is a neat approach. Please verify that the NODE_ENV is set correctly in your deployment environment, so you don't inadvertently run in "development" mode in production.
components/log-viewer-webui/server/src/DbManager.ts (1)
59-59: Clarify polling interval.
The value 0.5 implies a half-millisecond polling interval, which is extremely frequent and might degrade performance. If you intended half a second, use 500.
| - |- | ||
| cd "server" | ||
| rsync -a \ | ||
| package.json package-lock.json \ |
There was a problem hiding this comment.
stupid question, don't we need to sync "src" as well?
There was a problem hiding this comment.
not at all. How TypeScript compilation works is that the compiler performs type-checking then transpiles the TypeScript code into JavaScript code.
After we run npm run build to perform the compilation, the JS code would have been the dist folder and the sources stored in src can be thrown away.
…ig.mjs; change "new-cap" capIsNewExceptions.
davemarco
left a comment
There was a problem hiding this comment.
Partial review. Reviewed DbManager.ts
| promise: true, | ||
| connectionString: `mysql://${config.user}:${config.password}@${config.host}:` + | ||
| `${config.port}/${config.database}`, | ||
| }); |
There was a problem hiding this comment.
You removed some error handling here
There was a problem hiding this comment.
Right, that was intentional because the property initializations have been moved to the constructor.
As a general rule, it is highly recommended that you handle your errors in the next after or ready block, otherwise you will get them inside the listen callback.
I'll add the .after() callback back so it becomes clear from which the error is thrown.
Since we await the .register(), if an error is thrown, the location would have been pretty clear. On the other hand, using the .after(error) callback to catch and throw an error is trivial so i think we should avoid.
There was a problem hiding this comment.
I am fine with that as long .register(), actually throws. It may just silently have an error, and set app.mysql to null or underfined? If it's silent, it may be better to just throw exception?
There was a problem hiding this comment.
Right, errors are thrown with both syntaxes.
I just did some tests and think some of my previous assumptions are incorrect. Here're the correct ones:
The only difference between the two approaches is that if we use .after(err=>throw err;), in the console it will be clearly shown the error is thrown from DbManager. e.g.,
[1] file:///home/junhao/workspace/clp/components/log-viewer-webui/server/dist/src/DbManager.js:35
[1] throw err;
[1] ^
[1]
Otherwise, the error is still thrown but just without line information:
[1] Error: connect ECONNREFUSED 127.0.0.1:3306
[1] at PromisePool.query (/home/junhao/workspace/clp/components/log-viewer-webui/server/node_modules/mysql2/lib/promise/pool.js:36:22)
[1] at _createConnection (/home/junhao/workspace/clp/components/log-viewer-webui/server/node_modules/@fastify/mysql/index.js:73:10)
[1] at fastifyMysql (/home/junhao/workspace/clp/components/log-viewer-webui/server/node_modules/@fastify/mysql/index.js:13:3)
[1] at Plugin.exec (/home/junhao/workspace/clp/components/log-viewer-webui/server/node_modules/avvio/lib/plugin.js:125:28)
[1] at Boot._loadPlugin (/home/junhao/workspace/clp/components/log-viewer-webui/server/node_modules/avvio/boot.js:432:10)
[1] at process.processTicksAndRejections (node:internal/process/task_queues:82:21) {
[1] code: 'ECONNREFUSED',
[1] errno: -111,
[1] sql: undefined,
[1] sqlState: undefined,
[1] sqlMessage: undefined
[1] }
There was a problem hiding this comment.
In either approach, we can see the error is not code related and is caused by a failed SQL connection, but i guess it will be harder to debug it it's a different error.
I still think we should keep the type of #initMySql async so that we can avoid serving requests before the sql connection is ready. I think we can wrap the registration with our own Promise then we can await the promise.
There was a problem hiding this comment.
Apparently there're issues with Fastify's TS definitions. I got it to work with the async syntax by some typecasts.
There was a problem hiding this comment.
How about this. This was there is no type casting and I think should still work.
app.register(fastifyMysql, {
promise: true,
connectionString: `mysql://${config.user}:${config.password}@${config.host}:` +
`${config.port}/${config.database}`,
}).after
((err) => {
if (null !== err) {
throw err;
}
});
await app.ready();
There was a problem hiding this comment.
The await app.ready() blocks and the plugin registration times out:
[1] FastifyError [Error]: Plugin did not start in time: 'dbManagerPluginCallback-auto-0'. You may have forgotten to call 'done' function or to resolve a Promise
[1] at Timeout._onTimeout (/home/junhao/workspace/clp/components/log-viewer-webui/server/node_modules/avvio/lib/plugin.js:114:31)
[1] at listOnTimeout (node:internal/timers:573:17)
[1] at process.processTimers (node:internal/timers:514:7) {
[1] code: 'AVV_ERR_PLUGIN_EXEC_TIMEOUT',
[1] statusCode: 500,
[1] fn: <ref *1> [AsyncFunction: dbManagerPluginCallback] {
[1] default: [Circular *1],
[1] [Symbol(skip-override)]: true,
[1] [Symbol(fastify.display-name)]: 'dbManagerPluginCallback-auto-0',
[1] [Symbol(plugin-meta)]: { name: 'dbManagerPluginCallback-auto-0' }
[1] }
[1] }
i think chances is that we can't call await app.ready() while we're registering some plugin because app.ready()'s promise only resolves when ALL plugin registrations are done.
There was a problem hiding this comment.
Okay that makes sense.
This is my last attempt. Then i will just concede the type casting.
This should fix i think, and provide a good error message
try {
await app.register(fastifyMysql, {
promise: true,
connectionString: `mysql://${config.user}:${config.password}@${config.host}:` +
`${config.port}/${config.database}`,
});
} catch (e: unknown) {
let message = "Failed to init MySql";
if (e instanceof Error) {
message += `: ${e.message}`;
}
throw new Error(message);
}
| import {sleep} from "./utils.js"; | ||
|
|
||
|
|
||
| interface DbManagerOptions { |
There was a problem hiding this comment.
These should also be in typings
# Conflicts: # components/log-viewer-webui/package.json
…to match latest eslint-config-yscope.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (6)
components/log-viewer-webui/server/src/app.ts (2)
16-20: Add JSDoc comments to document the interface properties.Consider adding JSDoc comments to describe the purpose and requirements of each property in the
AppPropsinterface.+/** + * Configuration options for the Fastify application. + */ interface AppProps { + /** Fastify server configuration options */ fastifyOptions: FastifyServerOptions; + /** MySQL database username */ sqlDbUser: string; + /** MySQL database password */ sqlDbPass: string; }
Line range hint
39-51: Add validation for database credentials.Consider adding validation for the database credentials before passing them to the configuration object. This helps prevent runtime issues with invalid or missing credentials.
+ if (!sqlDbUser?.trim() || !sqlDbPass?.trim()) { + throw new Error("Database credentials cannot be empty"); + } + await server.register(DbManager, { mysqlConfig: { database: settings.SqlDbName,components/log-viewer-webui/server/package.json (1)
34-37: Consider pinning the eslint-config-yscope version.The development dependencies look good, but using
latestforeslint-config-yscopemight lead to unexpected breaking changes. Consider pinning to a specific version for better reproducibility.- "eslint-config-yscope": "latest", + "eslint-config-yscope": "^1.0.0", // Replace with actual versioncomponents/log-viewer-webui/server/eslint.config.mjs (1)
17-32: Consider extracting Type. exceptions to a shared configuration.*The configuration correctly disables
require-awaitfor Fastify routes and handles Type.* method capitalization. Consider moving theType.*exceptions to the sharedeslint-config-yscopepackage if these are used across multiple projects.components/log-viewer-webui/server/src/routes/query.ts (1)
38-63: Consider extracting error message templatesThe error handling logic is sound, but the error messages contain repeated strings. Consider extracting these into constants for better maintainability.
+const ERROR_MESSAGES = { + EXTRACT_FAILED: (streamId: string, logEventIdx: number) => + `Unable to extract stream with streamId=${streamId} at logEventIdx=${logEventIdx}`, + METADATA_NOT_FOUND: (streamId: string, logEventIdx: number) => + `Unable to find the metadata of extracted stream with streamId=${streamId} at logEventIdx=${logEventIdx}`, +}; if (null === extractResult) { resp.code(StatusCodes.BAD_REQUEST); - throw new Error("Unable to extract stream with " + - `streamId=${streamId} at logEventIdx=${logEventIdx}`); + throw new Error(ERROR_MESSAGES.EXTRACT_FAILED(streamId, logEventIdx)); }components/log-viewer-webui/server/src/DbManager.ts (1)
278-287: Move type declarations to a separate .d.ts fileThe Fastify type declarations should be moved to a separate declaration file for better organization and reusability.
Create a new file
types/fastify.d.ts:import { MySQLPromisePool } from "@fastify/mysql"; import { DbManager } from "../DbManager"; declare module "fastify" { interface FastifyInstance { mysql: MySQLPromisePool; dbManager: DbManager; } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
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 (10)
components/clp-package-utils/clp_package_utils/scripts/start_clp.py(2 hunks)components/log-viewer-webui/server/eslint.config.mjs(1 hunks)components/log-viewer-webui/server/package.json(1 hunks)components/log-viewer-webui/server/src/DbManager.ts(1 hunks)components/log-viewer-webui/server/src/app.ts(1 hunks)components/log-viewer-webui/server/src/main.ts(1 hunks)components/log-viewer-webui/server/src/routes/example.ts(1 hunks)components/log-viewer-webui/server/src/routes/query.ts(1 hunks)components/log-viewer-webui/server/src/routes/static.ts(1 hunks)components/log-viewer-webui/server/src/typings/DbManager.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- components/log-viewer-webui/server/src/routes/example.ts
- components/clp-package-utils/clp_package_utils/scripts/start_clp.py
- components/log-viewer-webui/server/src/routes/static.ts
- components/log-viewer-webui/server/src/typings/DbManager.ts
- components/log-viewer-webui/server/src/main.ts
🧰 Additional context used
📓 Path-based instructions (3)
components/log-viewer-webui/server/src/routes/query.ts (1)
Pattern **/*.{cpp,hpp,java,js,jsx,ts,tsx}: - Prefer false == <expression> rather than !<expression>.
components/log-viewer-webui/server/src/app.ts (1)
Pattern **/*.{cpp,hpp,java,js,jsx,ts,tsx}: - Prefer false == <expression> rather than !<expression>.
components/log-viewer-webui/server/src/DbManager.ts (1)
Pattern **/*.{cpp,hpp,java,js,jsx,ts,tsx}: - Prefer false == <expression> rather than !<expression>.
🔇 Additional comments (12)
components/log-viewer-webui/server/src/app.ts (2)
3-7: Well-structured imports with proper type definitions!The explicit import of Fastify types (
FastifyInstance,FastifyServerOptions) improves type safety and code clarity.
38-38: 🛠️ Refactor suggestionUpdate condition to follow coding guidelines.
According to the coding guidelines, prefer
false == <expression>rather than!<expression>.- if ("test" !== process.env.NODE_ENV) { + if (false == (process.env.NODE_ENV === "test")) {Likely invalid or redundant comment.
components/log-viewer-webui/server/package.json (3)
7-12: Well-structured TypeScript development workflow!The scripts are properly configured to handle TypeScript compilation, development watching, and production builds. The use of
concurrentlyfor development is a good practice to watch both TypeScript compilation and server changes.
19-31: Verify compatibility of dependency versions.Several dependencies have been updated to very recent versions. Please ensure compatibility:
@fastify/mongodb@9.0.2withfastify@5.2.1@types/node@22.10.7withtypescript@5.7.3✅ Verification successful
Dependencies are compatible with each other
The dependency versions in package.json are compatible:
@fastify/mongodb@9.0.2works correctly withfastify@5.2.1@types/node@22.10.7is compatible withtypescript@5.7.3- All other dependencies meet their peer requirements
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for potential compatibility issues # Test: Look for peer dependency warnings in package-lock.json fd "package-lock.json" -x jq -r '.packages | .[] | select(.peerDependencies != null) | .name + ": " + (.peerDependencies | tostring)'Length of output: 9221
5-5: Verify TypeScript configuration exists.The main entry point has been updated for TypeScript, which is correct. However, we should ensure that the referenced TypeScript configuration exists.
✅ Verification successful
TypeScript configuration is properly set up
The main entry point correctly references src/main.ts, which exists and is included in the TypeScript compilation configuration.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for TypeScript configuration # Test: Verify tsconfig.json exists and contains proper configuration fd -t f "tsconfig.json" -x cat {}Length of output: 876
Script:
#!/bin/bash # Verify the existence of main.ts in the src directory fd -p "src/main.ts$" --type fLength of output: 78
components/log-viewer-webui/server/eslint.config.mjs (3)
8-11: LGTM! Appropriate ignore patterns.The configuration correctly excludes build output and dependency directories from linting.
13-15: LGTM! Well-structured base configurations.The configuration properly extends common, TypeScript, and stylistic configurations using appropriate spread syntax.
37-37: LGTM! Clear and proper export.The configuration is correctly exported as the default export.
components/log-viewer-webui/server/src/routes/query.ts (3)
1-8: Well-structured TypeScript imports with type safety!The imports are well-organized, utilizing TypeBox for request validation and properly importing types. The JSON import correctly uses the new "with" assertion syntax.
16-27: Excellent use of TypeBox for request validation!The route handler is well-typed and uses TypeBox for request validation, which provides compile-time type safety and runtime validation. This is a significant improvement over manual validation.
29-32: Good error handling with proper boolean comparison!The error handling follows coding guidelines by using
false === expressionpattern and provides informative error messages.components/log-viewer-webui/server/src/DbManager.ts (1)
58-58:⚠️ Potential issueFix polling interval value
The value
0.5forJOB_COMPLETION_STATUS_POLL_INTERVAL_MILLISis misleading as it represents half a millisecond. If you intended half a second, use500.-const JOB_COMPLETION_STATUS_POLL_INTERVAL_MILLIS = 0.5; +const JOB_COMPLETION_STATUS_POLL_INTERVAL_MILLIS = 500;Likely invalid or redundant comment.
| import CommonConfig from "eslint-config-yscope/CommonConfig.mjs"; | ||
| import StylisticConfigArray from "eslint-config-yscope/StylisticConfigArray.mjs"; | ||
| import TsConfigArray from "eslint-config-yscope/TsConfigArray.mjs"; |
There was a problem hiding this comment.
💡 Codebase verification
Fix eslint-config-yscope version inconsistency
The package has different versions across the codebase:
- components/webui/linter: version 0.0.32
- components/log-viewer-webui/server: version "latest"
Please update the server's package.json to use the same fixed version (0.0.32) to ensure consistent linting rules across the project. Also verify that the imported config files exist in the installed package, as they weren't found in the current codebase.
🔗 Analysis chain
Verify the eslint-config-yscope package.
The configuration imports from eslint-config-yscope. Let's ensure this package is properly configured in the project.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify eslint-config-yscope package configuration
# Check if the package is listed in package.json
echo "Checking package.json for eslint-config-yscope..."
cat components/log-viewer-webui/server/package.json | jq '.dependencies["eslint-config-yscope"] // .devDependencies["eslint-config-yscope"]'
# Verify the imported modules exist
echo "Verifying imported modules..."
fd -t f "CommonConfig.mjs|StylisticConfigArray.mjs|TsConfigArray.mjs" node_modules/eslint-config-yscope
Length of output: 556
Script:
#!/bin/bash
# Check for package.json files and workspace configurations
echo "Checking for package.json files..."
fd package.json
echo -e "\nChecking for eslint config files..."
fd -e mjs -x grep -l "CommonConfig\|StylisticConfigArray\|TsConfigArray" {}
echo -e "\nChecking for workspace configurations..."
fd "pnpm-workspace.yaml|lerna.json|package.json" -d 1
Length of output: 675
Script:
#!/bin/bash
# Check linter package and its exports
echo "Checking linter package.json..."
cat components/webui/linter/package.json
echo -e "\nChecking linter package files..."
fd . components/webui/linter -t f -e mjs -e js
echo -e "\nChecking for CommonConfig.mjs content..."
fd . components/webui/linter -t f -e mjs -e js -x grep -l "CommonConfig"
Length of output: 742
| static async #initMySql (app: FastifyInstance, config: DbManagerOptions["mysqlConfig"]) { | ||
| await app.register(fastifyMysql, { | ||
| promise: true, | ||
| connectionString: `mysql://${config.user}:${config.password}@${config.host}:` + | ||
| `${config.port}/${config.database}`, | ||
| }); | ||
|
|
||
| return app.mysql.pool; | ||
| } |
There was a problem hiding this comment.
Add input sanitization for MySQL connection string
The connection string is built using template literals without sanitizing the config values. This could be vulnerable if the config values contain special characters.
+const sanitizeConnectionParam = (param: string): string => {
+ return encodeURIComponent(param);
+};
static async #initMySql (app: FastifyInstance, config: DbManagerOptions["mysqlConfig"]) {
await app.register(fastifyMysql, {
promise: true,
- connectionString: `mysql://${config.user}:${config.password}@${config.host}:` +
- `${config.port}/${config.database}`,
+ connectionString: `mysql://${sanitizeConnectionParam(config.user)}:${sanitizeConnectionParam(config.password)}@${sanitizeConnectionParam(config.host)}:${config.port}/${sanitizeConnectionParam(config.database)}`,
});📝 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.
| static async #initMySql (app: FastifyInstance, config: DbManagerOptions["mysqlConfig"]) { | |
| await app.register(fastifyMysql, { | |
| promise: true, | |
| connectionString: `mysql://${config.user}:${config.password}@${config.host}:` + | |
| `${config.port}/${config.database}`, | |
| }); | |
| return app.mysql.pool; | |
| } | |
| const sanitizeConnectionParam = (param: string): string => { | |
| return encodeURIComponent(param); | |
| }; | |
| static async #initMySql (app: FastifyInstance, config: DbManagerOptions["mysqlConfig"]) { | |
| await app.register(fastifyMysql, { | |
| promise: true, | |
| connectionString: `mysql://${sanitizeConnectionParam(config.user)}:${sanitizeConnectionParam(config.password)}@${sanitizeConnectionParam(config.host)}:${config.port}/${sanitizeConnectionParam(config.database)}`, | |
| }); | |
| return app.mysql.pool; | |
| } |
| static async #initMongo (app: FastifyInstance, config: DbManagerOptions["mongoConfig"]) | ||
| : Promise<{ | ||
| streamFilesCollection: StreamFilesCollection; | ||
| }> { | ||
| await app.register(fastifyMongo, { | ||
| forceClose: true, | ||
| url: `mongodb://${config.host}:${config.port}/${config.database}`, | ||
| }); | ||
| if ("undefined" === typeof app.mongo.db) { | ||
| throw new Error("Failed to initialize MongoDB plugin."); | ||
| } | ||
|
|
||
| return {streamFilesCollection: app.mongo.db.collection(config.streamFilesCollectionName)}; | ||
| } |
There was a problem hiding this comment.
Add input sanitization for MongoDB connection URL
The MongoDB URL is built using template literals without sanitizing the config values. This could be vulnerable if the config values contain special characters.
static async #initMongo (app: FastifyInstance, config: DbManagerOptions["mongoConfig"])
: Promise<{
streamFilesCollection: StreamFilesCollection;
}> {
await app.register(fastifyMongo, {
forceClose: true,
- url: `mongodb://${config.host}:${config.port}/${config.database}`,
+ url: `mongodb://${encodeURIComponent(config.host)}:${config.port}/${encodeURIComponent(config.database)}`,
});📝 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.
| static async #initMongo (app: FastifyInstance, config: DbManagerOptions["mongoConfig"]) | |
| : Promise<{ | |
| streamFilesCollection: StreamFilesCollection; | |
| }> { | |
| await app.register(fastifyMongo, { | |
| forceClose: true, | |
| url: `mongodb://${config.host}:${config.port}/${config.database}`, | |
| }); | |
| if ("undefined" === typeof app.mongo.db) { | |
| throw new Error("Failed to initialize MongoDB plugin."); | |
| } | |
| return {streamFilesCollection: app.mongo.db.collection(config.streamFilesCollectionName)}; | |
| } | |
| static async #initMongo (app: FastifyInstance, config: DbManagerOptions["mongoConfig"]) | |
| : Promise<{ | |
| streamFilesCollection: StreamFilesCollection; | |
| }> { | |
| await app.register(fastifyMongo, { | |
| forceClose: true, | |
| url: `mongodb://${encodeURIComponent(config.host)}:${config.port}/${encodeURIComponent(config.database)}`, | |
| }); | |
| if ("undefined" === typeof app.mongo.db) { | |
| throw new Error("Failed to initialize MongoDB plugin."); | |
| } | |
| return {streamFilesCollection: app.mongo.db.collection(config.streamFilesCollectionName)}; | |
| } |
| async #awaitJobCompletion (jobId: number) { | ||
| // eslint-disable-next-line @typescript-eslint/no-unnecessary-condition | ||
| while (true) { | ||
| let rows: QueryJob[]; | ||
| try { | ||
| const [queryResult] = await this.#mysqlConnectionPool.query<QueryJob[]>( | ||
| ` | ||
| SELECT ${QUERY_JOBS_TABLE_COLUMN_NAMES.STATUS} | ||
| FROM ${this.#queryJobsTableName} | ||
| WHERE ${QUERY_JOBS_TABLE_COLUMN_NAMES.ID} = ? | ||
| `, | ||
| jobId, | ||
| ); | ||
|
|
||
| rows = queryResult; | ||
| } catch (e: unknown) { | ||
| throw new Error(`Failed to query status for job ${jobId} - ${e?.toString()}`); | ||
| } | ||
|
|
||
| const [job] = rows; | ||
| if ("undefined" === typeof job) { | ||
| throw new Error(`Job ${jobId} not found in database.`); | ||
| } | ||
| const status = job[QUERY_JOBS_TABLE_COLUMN_NAMES.STATUS]; | ||
|
|
||
| if (false === QUERY_JOB_STATUS_WAITING_STATES.has(status)) { | ||
| if (QUERY_JOB_STATUS.CANCELLED === status) { | ||
| throw new Error(`Job ${jobId} was cancelled.`); | ||
| } else if (QUERY_JOB_STATUS.SUCCEEDED !== status) { | ||
| throw new Error(`Job ${jobId} exited with unexpected status=${status}: ` + | ||
| `${Object.keys(QUERY_JOB_STATUS)[status]}.`); | ||
| } | ||
| break; | ||
| } | ||
|
|
||
| await sleep(JOB_COMPLETION_STATUS_POLL_INTERVAL_MILLIS); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add maximum retry count to prevent infinite polling
The while(true) loop could potentially run indefinitely. Consider adding a maximum retry count and timeout mechanism.
+const MAX_POLL_RETRIES = 100; // 50 seconds with 500ms interval
+
async #awaitJobCompletion (jobId: number) {
+ let retries = 0;
// eslint-disable-next-line @typescript-eslint/no-unnecessary-condition
- while (true) {
+ while (retries < MAX_POLL_RETRIES) {
+ retries++;
let rows: QueryJob[];
// ... existing code ...
await sleep(JOB_COMPLETION_STATUS_POLL_INTERVAL_MILLIS);
}
+ throw new Error(`Job ${jobId} timed out after ${MAX_POLL_RETRIES} retries`);
}📝 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.
| async #awaitJobCompletion (jobId: number) { | |
| // eslint-disable-next-line @typescript-eslint/no-unnecessary-condition | |
| while (true) { | |
| let rows: QueryJob[]; | |
| try { | |
| const [queryResult] = await this.#mysqlConnectionPool.query<QueryJob[]>( | |
| ` | |
| SELECT ${QUERY_JOBS_TABLE_COLUMN_NAMES.STATUS} | |
| FROM ${this.#queryJobsTableName} | |
| WHERE ${QUERY_JOBS_TABLE_COLUMN_NAMES.ID} = ? | |
| `, | |
| jobId, | |
| ); | |
| rows = queryResult; | |
| } catch (e: unknown) { | |
| throw new Error(`Failed to query status for job ${jobId} - ${e?.toString()}`); | |
| } | |
| const [job] = rows; | |
| if ("undefined" === typeof job) { | |
| throw new Error(`Job ${jobId} not found in database.`); | |
| } | |
| const status = job[QUERY_JOBS_TABLE_COLUMN_NAMES.STATUS]; | |
| if (false === QUERY_JOB_STATUS_WAITING_STATES.has(status)) { | |
| if (QUERY_JOB_STATUS.CANCELLED === status) { | |
| throw new Error(`Job ${jobId} was cancelled.`); | |
| } else if (QUERY_JOB_STATUS.SUCCEEDED !== status) { | |
| throw new Error(`Job ${jobId} exited with unexpected status=${status}: ` + | |
| `${Object.keys(QUERY_JOB_STATUS)[status]}.`); | |
| } | |
| break; | |
| } | |
| await sleep(JOB_COMPLETION_STATUS_POLL_INTERVAL_MILLIS); | |
| } | |
| } | |
| const MAX_POLL_RETRIES = 100; // 50 seconds with 500ms interval | |
| async #awaitJobCompletion (jobId: number) { | |
| let retries = 0; | |
| // eslint-disable-next-line @typescript-eslint/no-unnecessary-condition | |
| while (retries < MAX_POLL_RETRIES) { | |
| retries++; | |
| let rows: QueryJob[]; | |
| try { | |
| const [queryResult] = await this.#mysqlConnectionPool.query<QueryJob[]>( | |
| ` | |
| SELECT ${QUERY_JOBS_TABLE_COLUMN_NAMES.STATUS} | |
| FROM ${this.#queryJobsTableName} | |
| WHERE ${QUERY_JOBS_TABLE_COLUMN_NAMES.ID} = ? | |
| `, | |
| jobId, | |
| ); | |
| rows = queryResult; | |
| } catch (e: unknown) { | |
| throw new Error(`Failed to query status for job ${jobId} - ${e?.toString()}`); | |
| } | |
| const [job] = rows; | |
| if ("undefined" === typeof job) { | |
| throw new Error(`Job ${jobId} not found in database.`); | |
| } | |
| const status = job[QUERY_JOBS_TABLE_COLUMN_NAMES.STATUS]; | |
| if (false === QUERY_JOB_STATUS_WAITING_STATES.has(status)) { | |
| if (QUERY_JOB_STATUS.CANCELLED === status) { | |
| throw new Error(`Job ${jobId} was cancelled.`); | |
| } else if (QUERY_JOB_STATUS.SUCCEEDED !== status) { | |
| throw new Error(`Job ${jobId} exited with unexpected status=${status}: ` + | |
| `${Object.keys(QUERY_JOB_STATUS)[status]}.`); | |
| } | |
| break; | |
| } | |
| await sleep(JOB_COMPLETION_STATUS_POLL_INTERVAL_MILLIS); | |
| } | |
| throw new Error(`Job ${jobId} timed out after ${MAX_POLL_RETRIES} retries`); | |
| } |
# Conflicts: # components/log-viewer-webui/server/package-lock.json # components/log-viewer-webui/server/package.json # components/log-viewer-webui/server/src/DbManager.js # components/log-viewer-webui/server/src/routes/query.ts
…ctory and update import paths.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
components/log-viewer-webui/server/src/routes/query.ts (1)
24-25: Consider validating the range of logEventIdx.Although you already enforce an integer type, you may want to confirm that this value is non-negative (or within an expected range) to avoid unexpected job submissions.
components/log-viewer-webui/server/src/plugins/DbManager.ts (1)
211-248: Add a maximum wait or timeout for job completion.The current polling loop never stops if the job never completes. Consider introducing a maximum number of iterations or a timeout to gracefully handle hang conditions.
components/log-viewer-webui/server/src/plugins/S3Manager.ts (1)
54-56: Add parentheses for clarity.The expression
false === error instanceof Errorcan be easier to parse with explicit grouping:- if (false === error instanceof Error) { + if (false === (error instanceof Error)) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
components/log-viewer-webui/server/src/app.ts(1 hunks)components/log-viewer-webui/server/src/plugins/DbManager.ts(1 hunks)components/log-viewer-webui/server/src/plugins/S3Manager.ts(4 hunks)components/log-viewer-webui/server/src/routes/query.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- components/log-viewer-webui/server/src/app.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/routes/query.tscomponents/log-viewer-webui/server/src/plugins/DbManager.tscomponents/log-viewer-webui/server/src/plugins/S3Manager.ts
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: centos-stream-9-static-linked-bins
- GitHub Check: ubuntu-jammy-dynamic-linked-bins
- GitHub Check: ubuntu-jammy-static-linked-bins
- GitHub Check: centos-stream-9-dynamic-linked-bins
- GitHub Check: ubuntu-jammy-lint
- GitHub Check: lint-check (ubuntu-latest)
- GitHub Check: build-macos (macos-15, true)
- GitHub Check: build-macos (macos-13, true)
- GitHub Check: build-macos (macos-15, false)
- GitHub Check: build-macos (macos-13, false)
- GitHub Check: build (macos-latest)
🔇 Additional comments (4)
components/log-viewer-webui/server/src/routes/query.ts (2)
49-62: Duplicate error message string.The same error message appears at lines 50‒51 and lines 60‒61. Extracting it into a shared constant can help reduce duplication and simplify maintenance.
17-75: Implementation appears sound.The route logic for stream extraction, validation, and final path resolution is clear and aligns well with the new TypeScript-based approach.
components/log-viewer-webui/server/src/plugins/DbManager.ts (1)
34-34: Verify the 0.5 ms polling interval.A 0.5 ms interval can cause high query overhead, potentially overloading the system. Please confirm you truly intend to poll the database every half-millisecond.
components/log-viewer-webui/server/src/plugins/S3Manager.ts (1)
34-36: Logic is acceptable.Your error handling aligns with the aim of throwing an exception when URL generation fails.
# Conflicts: # Taskfile.yml
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
components/log-viewer-webui/server/src/plugins/DbManager.ts (1)
30-34: Consider using an integer or a more descriptive constant name.
JOB_COMPLETION_STATUS_POLL_INTERVAL_MILLIS = 0.5;can be confusing, as 0.5 implies half a millisecond, which may be exceedingly frequent.-const JOB_COMPLETION_STATUS_POLL_INTERVAL_MILLIS = 0.5; +const JOB_COMPLETION_STATUS_POLL_INTERVAL_MILLIS = 500;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
Taskfile.yml(6 hunks)components/clp-package-utils/clp_package_utils/scripts/start_clp.py(2 hunks)components/log-viewer-webui/server/src/plugins/DbManager.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- components/clp-package-utils/clp_package_utils/scripts/start_clp.py
🧰 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/plugins/DbManager.ts
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: centos-stream-9-static-linked-bins
- GitHub Check: centos-stream-9-dynamic-linked-bins
- GitHub Check: ubuntu-jammy-static-linked-bins
- GitHub Check: ubuntu-jammy-dynamic-linked-bins
- GitHub Check: ubuntu-jammy-lint
- GitHub Check: build-macos (macos-15, true)
- GitHub Check: build-macos (macos-15, false)
- GitHub Check: build-macos (macos-14, true)
🔇 Additional comments (18)
Taskfile.yml (7)
93-93: No concerns with the newly referenced “log-viewer-webui.md5” file.This change aligns with the rename from "log-viewer-webui-clients" to "log-viewer-webui."
116-116: Good addition of the “log-viewer-webui” dependency.This ensures the package task includes the log viewer web UI build.
229-248: The new “log-viewer-webui” task definition looks coherent.These additions properly list the server sources to handle TypeScript compilation and keep tasks consistent. Nothing appears problematic in referencing the new lines (e.g., “server/package.json,” “server/src/**/*.ts,” etc.).
266-266: No issues with creating the output directory.The “mkdir -p” command ensures the directory exists prior to the build artifacts being copied.
270-271: Clean use of “–emptyOutDir –outDir.”Explicitly specifying these flags helps avoid leftover build artifacts.
279-282: The rsync step for “server” directory is straightforward.Copying “package.json” and “package-lock.json” ensures consistency between build and deployed paths.
167-169:Details
✅ Verification successful
Verify correctness of new output path.
Placing files under “var/www/log-viewer-webui” appears sensible; however, confirm it does not conflict with existing web server directories.
Please verify there is no path overlap by running:
Check for any unexpected subpaths that may conflict with the new “var/www/log-viewer-webui” location.
--- 🏁 Script executed: ```shell #!/bin/bash fd log-viewer-webui -t dLength of output: 55
Verified output directory: No Conflict Detected
The search revealed that the only instance of “log-viewer-webui” exists incomponents/log-viewer-webui/. This directory is clearly separate from your new output path at{{.OUTPUT_DIR}}/var/www/log-viewer-webui, ensuring no overlap with existing web server directories.components/log-viewer-webui/server/src/plugins/DbManager.ts (11)
1-15: Imports are well-organized.These external dependencies (MySQL pool, Mongo plugin, etc.) are declared clearly. No obvious version or security concerns.
17-28: Good usage of typed imports.Referencing your custom types from “../typings/DbManager.js” ensures strong type safety throughout.
37-68: The constructor logic looks solid.Assigning the MySQL and MongoDB handlers here centralizes the database connections for the class.
70-80: The “create” factory method is clear.Abstracting MySQL and Mongo initialization logic away from external callers promotes clarity in setup code.
81-101: MySQL initialization is nicely encapsulated.Using parameterized queries helps prevent SQL injection. The code checks for errors correctly.
102-126: Mongo initialization logic is sound.The error-handling for plugin registration is robust, and the collection reference is set properly.
129-185: Job submission approach is reasonable.Utilizing MsgPack for job configuration ensures a compact structure. The error catching logs issues without exposing sensitive data.
187-202: Metadata retrieval from MongoDB is straightforward.Query filters correctly check for message index range. No concurrency concerns evident.
251-260: Neat plugin callback.This registers the DbManager on the Fastify instance, making it accessible elsewhere.
262-271: Augmenting the FastifyInstance interface is properly done.Manually specifying the
mysqlproperty type matches the plugin usage.
274-275: Export statements are clear.Providing both the Fastify plugin and the
QUERY_JOB_TYPEenum is helpful for external usage.
| /** | ||
| * Waits for the job with the given ID to finish. | ||
| * | ||
| * @param jobId | ||
| * @throws {Error} If there's an error querying the job's status, the job is not found in the | ||
| * database, the job was cancelled, or it exited with an unexpected status. | ||
| */ | ||
| async #awaitJobCompletion (jobId: number) { | ||
| // eslint-disable-next-line @typescript-eslint/no-unnecessary-condition | ||
| while (true) { | ||
| let rows: QueryJob[]; | ||
| try { | ||
| const [queryResult] = await this.#mysqlConnectionPool.query<QueryJob[]>( | ||
| ` | ||
| SELECT ${QUERY_JOBS_TABLE_COLUMN_NAMES.STATUS} | ||
| FROM ${this.#queryJobsTableName} | ||
| WHERE ${QUERY_JOBS_TABLE_COLUMN_NAMES.ID} = ? | ||
| `, | ||
| jobId, | ||
| ); | ||
|
|
||
| rows = queryResult; | ||
| } catch (e: unknown) { | ||
| throw new Error(`Failed to query status for job ${jobId} - ${e?.toString()}`); | ||
| } | ||
|
|
||
| const [job] = rows; | ||
| if ("undefined" === typeof job) { | ||
| throw new Error(`Job ${jobId} not found in database.`); | ||
| } | ||
| const status = job[QUERY_JOBS_TABLE_COLUMN_NAMES.STATUS]; | ||
|
|
||
| if (false === QUERY_JOB_STATUS_WAITING_STATES.has(status)) { | ||
| if (QUERY_JOB_STATUS.CANCELLED === status) { | ||
| throw new Error(`Job ${jobId} was cancelled.`); | ||
| } else if (QUERY_JOB_STATUS.SUCCEEDED !== status) { | ||
| throw new Error(`Job ${jobId} exited with unexpected status=${status}: ` + | ||
| `${Object.keys(QUERY_JOB_STATUS)[status]}.`); | ||
| } | ||
| break; | ||
| } | ||
|
|
||
| await sleep(JOB_COMPLETION_STATUS_POLL_INTERVAL_MILLIS); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Polling logic for job completion is correct but watch out for potential CPU usage.
A “while(true)” loop with a minimal sleep can be intensive. Increasing the interval or using event-based callbacks may be more performant for large workloads.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
Taskfile.yml (2)
266-269: Client build command adjustment and line length
The client build commands now create the output directory and execute the build with--emptyOutDirand--outDirflags. However, line 269 exceeds 100 characters, impacting readability. Consider refactoring the command (perhaps by using a shell variable or breaking it into multiple lines) to address the static analysis warning.🧰 Tools
🪛 GitHub Check: lint-check (ubuntu-latest)
[failure] 269-269:
269:101 [line-length] line too long (110 > 100 characters)🪛 GitHub Actions: clp-lint
[warning] 269-269: line too long (110 > 100 characters)
277-278: Yscope-log-viewer build command review
The build command for yscope-log-viewer now directs its output to"{{.OUTPUT_DIR}}/yscope-log-viewer". Similar to the client build command, the line length is triggering a static analysis warning. Consider shortening or reformatting this command to enhance readability.🧰 Tools
🪛 GitHub Check: lint-check (ubuntu-latest)
[failure] 278-278:
278:101 [line-length] line too long (112 > 100 characters)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Taskfile.yml(6 hunks)
🧰 Additional context used
🪛 GitHub Check: lint-check (ubuntu-latest)
Taskfile.yml
[failure] 278-278:
278:101 [line-length] line too long (112 > 100 characters)
[failure] 269-269:
269:101 [line-length] line too long (110 > 100 characters)
🪛 GitHub Actions: clp-lint
Taskfile.yml
[warning] 269-269: line too long (110 > 100 characters)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: build-macos (macos-15, false)
- GitHub Check: build-macos (macos-13, false)
- GitHub Check: build-macos (macos-13, true)
- GitHub Check: ubuntu-jammy-lint
- GitHub Check: centos-stream-9-static-linked-bins
- GitHub Check: ubuntu-jammy-static-linked-bins
- GitHub Check: centos-stream-9-dynamic-linked-bins
- GitHub Check: ubuntu-jammy-dynamic-linked-bins
🔇 Additional comments (4)
Taskfile.yml (4)
116-116: Dependency renaming consistency check
The dependency"log-viewer-webui"has been updated to reflect the new module name. This change is consistent with previous feedback regarding the renaming of the component fromlog_viewer_webuitolog-viewer-webui.
166-170: Verify rsync and server build commands
The rsync command now copies build artefacts from"{{.G_LOG_VIEWER_WEBUI_BUILD_DIR}}/"to"{{.OUTPUT_DIR}}/var/www/log-viewer-webui", and the subsequent command navigates to the server directory to runnpm clean-install. Please double-check that these paths match the new TypeScript server build output and that the build process is fully in sync.
229-249: Inclusion of TypeScript server files in package sources
The sources list for thelog-viewer-webuitask now includes several server-side files (e.g."server/package.json","server/package-lock.json","server/settings.json","server/src/**/*.ts", and"server/tsconfig.json"). This ensures that the migrated TypeScript server is packaged correctly. Confirm that the glob patterns accurately capture all necessary files and that no extraneous files are included.
271-275: Server build command and file sync with TypeScript integration
The server build process now compiles TypeScript usingnpx tsc --outDir '{{.OUTPUT_DIR}}/server/dist', followed by an rsync ofpackage.jsonandpackage-lock.jsonto the output directory. Please ensure that yourtsconfig.jsonis configured properly and that these files are required in the final build output.
| OUTPUT_DIR: "{{.G_PACKAGE_BUILD_DIR}}" | ||
| sources: | ||
| - "{{.G_BUILD_DIR}}/log-viewer-webui-clients.md5" | ||
| - "{{.G_BUILD_DIR}}/log-viewer-webui.md5" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Checksum file reference updated
The source for the checksum file now uses "{{.G_BUILD_DIR}}/log-viewer-webui.md5", which aligns with the new naming convention for the component. Please verify that all tasks referencing the old filename have been updated accordingly.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
components/log-viewer-webui/server/src/utils/env.ts (1)
54-59:⚠️ Potential issueFix dotenv path configuration.
The
pathoption in dotenv's config expects a single string, not an array. This might not work as expected.- dotenvConfig({ - path: [ - ".env.local", - ".env", - ], - }); + // To load multiple files, call dotenvConfig separately for each file + dotenvConfig({ path: ".env.local" }); + dotenvConfig({ path: ".env" });Alternatively, consider using a library like dotenv-flow that supports multiple environment files.
🧹 Nitpick comments (5)
components/log-viewer-webui/server/eslint.config.mjs (1)
21-22: Fix comment formattingAdd a space after the comment slashes for consistency with coding standards.
- // TypeBox imports + // TypeBox importscomponents/log-viewer-webui/server/src/utils/env.ts (4)
39-40: Consider adding minLength constraint for CLP_DB_PASS.The
CLP_DB_USERfield has aminLength: 1constraint, butCLP_DB_PASSdoesn't have any constraints. For consistency and to ensure valid credentials, consider adding similar validation.- CLP_DB_PASS: Type.String(), + CLP_DB_PASS: Type.String({minLength: 1}),
43-43: Ensure PORT is properly parsed from environment variables.Environment variables are strings by default, but this validation expects a number. There might be type conversion issues if
process.env.PORTisn't properly converted before validation.- PORT: Type.Number({minimum: 1, maximum: 65535}), + PORT: Type.Integer({minimum: 1, maximum: 65535}),Then add type conversion in the parseEnvVars function:
// Before validation process.env.PORT = process.env.PORT ? String(parseInt(process.env.PORT, 10)) : undefined;
50-50: Complete the @return JSDoc annotation.The @return annotation is empty. Document what the function returns for better code documentation.
- * @return + * @return {Static<typeof EnvVars>} Object containing validated environment variables.
54-59: Add error handling for dotenv configuration.The
dotenvConfig()call doesn't check for configuration errors. Consider capturing and handling any potential errors during environment variable loading.- dotenvConfig({ - path: [ - ".env.local", - ".env", - ], - }); + try { + // To load multiple files, call dotenvConfig separately for each file + dotenvConfig({ path: ".env.local" }); + dotenvConfig({ path: ".env" }); + } catch (error) { + throw new Error(`Failed to load environment variables: ${error instanceof Error ? error.message : String(error)}`); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
components/log-viewer-webui/server/eslint.config.mjs(1 hunks)components/log-viewer-webui/server/src/main.ts(1 hunks)components/log-viewer-webui/server/src/utils/env.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- components/log-viewer-webui/server/src/main.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/utils/env.ts
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: ubuntu-jammy-static-linked-bins
- GitHub Check: ubuntu-jammy-dynamic-linked-bins
- GitHub Check: ubuntu-jammy-lint
- GitHub Check: centos-stream-9-static-linked-bins
- GitHub Check: centos-stream-9-dynamic-linked-bins
- GitHub Check: lint-check (ubuntu-latest)
🔇 Additional comments (6)
components/log-viewer-webui/server/eslint.config.mjs (6)
1-3: Fix eslint-config-yscope version inconsistencyThe package has different versions across the codebase:
- components/webui/linter: version 0.0.32
- components/log-viewer-webui/server: version "latest"
Please update the server's package.json to use the same fixed version (0.0.32) to ensure consistent linting rules across the project. Also verify that the imported config files exist in the installed package, as they weren't found in the current codebase.
8-11: LGTM: Directory ignores are properly configuredThe configuration correctly ignores the standard build and dependency directories.
13-15: LGTM: Proper configuration inheritanceGood practice to extend the common configurations for consistency across the codebase.
22-30: LGTM: Appropriate TypeBox exceptionsThe TypeBox type constructor exceptions for the new-cap rule are correctly defined.
40-51: LGTM: Proper floating promise configurationGood configuration of the no-floating-promises rule with appropriate exceptions for test functions.
52-56: LGTM: Appropriate fastify plugin handlingThe require-await rule is correctly disabled with a clear explanation about Fastify's async pattern requirements.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
Taskfile.yml (2)
272-277: Server Build Command for TypeScript Compilation
The updated server build command compiles TypeScript files usingnpx tsc --outDir '{{.OUTPUT_DIR}}/server/dist'and then synchronises thepackage.jsonandpackage-lock.jsonfiles via rsync. Ensure that any necessary static assets or runtime files are also incorporated into the build as needed.
278-280: Naming Consistency in yscope-log-viewer Build Output
The build command foryscope-log-viewerspecifies the output path as'{{.OUTPUT_DIR}}/yscope-logviewer', which omits the hyphen found in other parts of the configuration (e.g., the source folder"yscope-log-viewer/package.json"). For consistency, consider renaming the output directory to'{{.OUTPUT_DIR}}/yscope-log-viewer'.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Taskfile.yml(6 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: ubuntu-jammy-lint
- GitHub Check: centos-stream-9-dynamic-linked-bins
- GitHub Check: centos-stream-9-static-linked-bins
- GitHub Check: ubuntu-jammy-dynamic-linked-bins
- GitHub Check: ubuntu-jammy-static-linked-bins
- GitHub Check: lint-check (macos-latest)
🔇 Additional comments (4)
Taskfile.yml (4)
93-93: Checksum File Reference Updated
The checksum file reference is now updated to"{{.G_BUILD_DIR}}/log-viewer-webui.md5", which reflects the renaming from the previouslog-viewer-webui-clientsconvention to the newlog-viewer-webuinaming. This change is in line with the PR objectives.
116-116: Dependency Naming Update
The dependency for thepackagetask has been updated to"log-viewer-webui". This change ensures consistency with the renamed component path.
229-248: Inclusion of Server Component Sources for TypeScript Migration
In thelog-viewer-webuitask, several new source entries have been added:
server/package.jsonserver/package-lock.jsonserver/settings.jsonserver/src/**/*.tsserver/tsconfig.jsonThese additions reflect the migration of the server codebase to TypeScript. Please verify that the build process correctly compiles these TypeScript sources and that the output is properly utilized downstream.
268-271: Client Build Command Enhancement
The client build command now includes the--emptyOutDirflag, ensuring that the output directory is cleared before the build process begins. This helps prevent any residual files from affecting the build's integrity.
davemarco
left a comment
There was a problem hiding this comment.
Approved pending minor comments. Also please test it's not broken prior to merge
| await (app.register(fastifyMysql, { | ||
| promise: true, | ||
| connectionString: `mysql://${config.user}:${config.password}@${config.host}:` + | ||
| `${config.port}/${config.database}`, | ||
| }).after((err) => { | ||
| if ("undefined" !== typeof err && null !== err) { | ||
| throw err; | ||
| } | ||
| }) as unknown as Promise<void>); |
There was a problem hiding this comment.
I tested this should work. Gets rid of type casting
| await (app.register(fastifyMysql, { | |
| promise: true, | |
| connectionString: `mysql://${config.user}:${config.password}@${config.host}:` + | |
| `${config.port}/${config.database}`, | |
| }).after((err) => { | |
| if ("undefined" !== typeof err && null !== err) { | |
| throw err; | |
| } | |
| }) as unknown as Promise<void>); | |
| try { | |
| await app.register(fastifyMysql, { | |
| promise: true, | |
| connectionString: `mysql://${config.user}:${config.password}@${config.host}:` + | |
| `${config.port}/${config.database}`, | |
| }); | |
| } catch (e: unknown) { | |
| let message = "Failed to init MySql"; | |
| if (e instanceof Error) { | |
| message += `: ${e.message}`; | |
| } | |
| throw new Error(message); | |
| } |
There was a problem hiding this comment.
i vaguely remember trying this approach before, but i'm not sure why i didn't end up proposing this. Maybe it's because of the difference in the stack trace: e.g., these lines are not showing up with the current approach.
[1] Error: connect ECONNREFUSED 127.0.0.1:3306
[1] at PromisePool.query (/home/junhao/workspace/clp/components/log-viewer-webui/server/node_modules/mysql2/lib/promise/pool.js:36:22)
[1] at _createConnection (/home/junhao/workspace/clp/components/log-viewer-webui/server/node_modules/@fastify/mysql/index.js:73:10)
[1] at fastifyMysql (/home/junhao/workspace/clp/components/log-viewer-webui/server/node_modules/@fastify/mysql/index.js:13:3)
[1] at Plugin.exec (/home/junhao/workspace/clp/components/log-viewer-webui/server/node_modules/avvio/lib/plugin.js:125:28)
[1] at Boot._loadPlugin (/home/junhao/workspace/clp/components/log-viewer-webui/server/node_modules/avvio/boot.js:432:10)
...
[1] code: 'ECONNREFUSED',
[1] errno: -111,
[1] sql: undefined,
[1] sqlState: undefined,
[1] sqlMessage: undefined
let me see if we can replace the stack with the original error
There was a problem hiding this comment.
How about this
| await (app.register(fastifyMysql, { | |
| promise: true, | |
| connectionString: `mysql://${config.user}:${config.password}@${config.host}:` + | |
| `${config.port}/${config.database}`, | |
| }).after((err) => { | |
| if ("undefined" !== typeof err && null !== err) { | |
| throw err; | |
| } | |
| }) as unknown as Promise<void>); | |
| try { | |
| await app.register(fastifyMysql, { | |
| promise: true, | |
| connectionString: `mysql://${config.user}:${config.password}@${config.host}:` + | |
| `${config.port}/${config.database}`, | |
| }); | |
| } catch (e: unknown) { | |
| throw new Error( | |
| `Failed to init MySql: ${e instanceof Error ? | |
| e.message : | |
| JSON.stringify(e)}`, | |
| {cause: e} | |
| ); | |
| } |
which gives
[1] Error: Failed to init MySql: connect ECONNREFUSED 127.0.0.1:3306
[1] at #initMySql (file:///home/junhao/workspace/clp/components/log-viewer-webui/server/dist/src/plugins/DbManager.js:38:19)
[1] at process.processTicksAndRejections (node:internal/process/task_queues:95:5)
[1] at async DbManager.create (file:///home/junhao/workspace/clp/components/log-viewer-webui/server/dist/src/plugins/DbManager.js:20:37)
[1] at async dbManagerPluginCallback (file:///home/junhao/workspace/clp/components/log-viewer-webui/server/dist/src/plugins/DbManager.js:131:23) {
[1] [cause]: Error: connect ECONNREFUSED 127.0.0.1:3306
[1] at PromisePool.query (/home/junhao/workspace/clp/components/log-viewer-webui/server/node_modules/mysql2/lib/promise/pool.js:36:22)
[1] at _createConnection (/home/junhao/workspace/clp/components/log-viewer-webui/server/node_modules/@fastify/mysql/index.js:73:10)
[1] at fastifyMysql (/home/junhao/workspace/clp/components/log-viewer-webui/server/node_modules/@fastify/mysql/index.js:13:3)
[1] at Plugin.exec (/home/junhao/workspace/clp/components/log-viewer-webui/server/node_modules/avvio/lib/plugin.js:125:28)
[1] at Boot._loadPlugin (/home/junhao/workspace/clp/components/log-viewer-webui/server/node_modules/avvio/boot.js:432:10)
[1] at process.processTicksAndRejections (node:internal/process/task_queues:82:21) {
[1] code: 'ECONNREFUSED',
[1] errno: -111,
[1] sql: undefined,
[1] sqlState: undefined,
[1] sqlMessage: undefined
[1] }
[1] }
| await (app.register(fastifyMongo, { | ||
| forceClose: true, | ||
| url: `mongodb://${config.host}:${config.port}/${config.database}`, | ||
| }).after((err) => { | ||
| if ("undefined" !== typeof err && null !== err) { | ||
| throw err; | ||
| } | ||
| }) as unknown as Promise<void>); | ||
| if ("undefined" === typeof app.mongo.db) { | ||
| throw new Error("Failed to initialize MongoDB plugin."); | ||
| } |
There was a problem hiding this comment.
| await (app.register(fastifyMongo, { | |
| forceClose: true, | |
| url: `mongodb://${config.host}:${config.port}/${config.database}`, | |
| }).after((err) => { | |
| if ("undefined" !== typeof err && null !== err) { | |
| throw err; | |
| } | |
| }) as unknown as Promise<void>); | |
| if ("undefined" === typeof app.mongo.db) { | |
| throw new Error("Failed to initialize MongoDB plugin."); | |
| } | |
| try { | |
| await app.register(fastifyMongo, { | |
| forceClose: true, | |
| url: `mongodb://${config.host}:${config.port}/${config.database}`, | |
| }); | |
| } catch (e: unknown) { | |
| let message = "Failed to init Mongo"; | |
| if (e instanceof Error) { | |
| message += `: ${e.message}`; | |
| } | |
| throw new Error(message); | |
| } | |
| if ("undefined" === typeof app.mongo.db) { | |
| throw new Error("Failed to initialize MongoDB plugin."); | |
| } |
Co-authored-by: davemarco <83603688+davemarco@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
components/log-viewer-webui/server/package.json (1)
19-33: Review Dependency Updates and Placement.
The dependency versions for AWS SDK, Fastify plugins, and related packages have been updated to the desired versions. Notice that"typescript": "~5.7.3"is now listed under"dependencies". Typically, TypeScript is only required at build time, so consider moving it to"devDependencies"if it is not needed during runtime. Please verify this aligns with your project’s dependency strategy.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
components/log-viewer-webui/server/package.json(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: centos-stream-9-static-linked-bins
- GitHub Check: ubuntu-jammy-dynamic-linked-bins
- GitHub Check: ubuntu-jammy-static-linked-bins
- GitHub Check: build-macos (macos-15, false)
- GitHub Check: build-macos (macos-14, false)
- GitHub Check: build-macos (macos-14, true)
- GitHub Check: lint-check (ubuntu-latest)
- GitHub Check: lint-check (macos-latest)
- GitHub Check: build (macos-latest)
🔇 Additional comments (3)
components/log-viewer-webui/server/package.json (3)
5-5: Clarify the Main Entry Point.
The"main"field now points to"src/main.ts", which reflects the source file in TypeScript. Please verify that this configuration works with your module consumers and build pipelines, given that the runtime scripts (like"prod"and"start") use the compiled output (dist/src/main.js).
7-12: Validate Build and Development Scripts.
The scripts have been updated as follows:
"build": "tsc -b"correctly leverages the project’s build mode."build:watch": "npm run build -- --watch"ensures continuous integration during development."lint:check"and"lint:fix"now enforce strict linting with no warnings allowed.- Both
"prod"and"start"scripts appropriately reference the compiled file atdist/src/main.js.
Please confirm that these updates align with your deployment and development workflow expectations.
36-39: Verify Dev Dependency Versions.
The development dependencies have been updated—"concurrently","nodemon", and"tap"now have newer versions. Additionally, ensure that"eslint-config-yscope"remains compatible with the updated TypeScript ESLint configurations.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
components/log-viewer-webui/server/src/plugins/DbManager.ts (3)
161-173: Consider adding a default case for handling unexpected job typesThe current implementation only handles
EXTRACT_IRandEXTRACT_JSONjob types without a default case. If a new job type is added in the future but not handled here, thejobConfigvariable could remain undefined.if (QUERY_JOB_TYPE.EXTRACT_IR === jobType) { jobConfig = { file_split_id: null, msg_ix: logEventIdx, orig_file_id: streamId, target_uncompressed_size: targetUncompressedSize, }; } else if (QUERY_JOB_TYPE.EXTRACT_JSON === jobType) { jobConfig = { archive_id: streamId, target_chunk_size: targetUncompressedSize, }; + } else { + throw new Error(`Unsupported job type: ${jobType}`); }
188-192: Improve error handling to provide more contextThe current error handling simply logs the error and returns null, which could make it difficult for callers to understand what went wrong. Consider adding more context to the error message or returning a more descriptive error object.
} catch (e) { this.#fastify.log.error(e); - - return null; + + const errorMessage = e instanceof Error ? e.message : String(e); + this.#fastify.log.error(`Failed to submit or complete job: ${errorMessage}`); + return null; }
246-246: Use preferred boolean expression styleAccording to the coding guidelines, you should prefer
false == <expression>rather than!<expression>. However, the current implementation usesfalse ===which is more verbose than needed. Consider simplifying this to match the guideline.- if (false === QUERY_JOB_STATUS_WAITING_STATES.has(status)) { + if (!QUERY_JOB_STATUS_WAITING_STATES.has(status)) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
components/log-viewer-webui/server/src/plugins/DbManager.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/plugins/DbManager.ts
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: ubuntu-jammy-lint
- GitHub Check: ubuntu-jammy-static-linked-bins
- GitHub Check: centos-stream-9-static-linked-bins
- GitHub Check: centos-stream-9-dynamic-linked-bins
- GitHub Check: ubuntu-jammy-dynamic-linked-bins
- GitHub Check: build-macos (macos-14, false)
- GitHub Check: build-macos (macos-15, false)
- GitHub Check: build-macos (macos-14, true)
- GitHub Check: lint-check (ubuntu-latest)
- GitHub Check: lint-check (macos-latest)
- GitHub Check: build (macos-latest)
🔇 Additional comments (2)
components/log-viewer-webui/server/src/plugins/DbManager.ts (2)
214-258: Polling logic for job completion is correct but watch out for potential CPU usage.A "while(true)" loop with a minimal sleep can be intensive. Increasing the interval or using event-based callbacks may be more performant for large workloads.
1-285: Overall code structure and TypeScript migration is well doneThe overall migration to TypeScript is well implemented with proper type definitions, error handling, and class structure. The code follows good practices such as:
- Clear separation of concerns with distinct methods for different operations
- Proper error handling with informative messages
- Type safety through interfaces and type annotations
- Clean integration with the Fastify framework
These improvements will make the codebase more maintainable and robust.
| /** | ||
| * Interval in milliseconds for polling the completion status of a job. | ||
| */ | ||
| const JOB_COMPLETION_STATUS_POLL_INTERVAL_MILLIS = 0.5; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Check the polling interval value
The constant JOB_COMPLETION_STATUS_POLL_INTERVAL_MILLIS is set to 0.5, which seems very low for a polling interval in milliseconds. This could lead to excessive CPU usage when waiting for job completion.
Consider using a larger value (e.g., 500 for 500ms) to reduce system load while still providing responsive updates.
-const JOB_COMPLETION_STATUS_POLL_INTERVAL_MILLIS = 0.5;
+const JOB_COMPLETION_STATUS_POLL_INTERVAL_MILLIS = 500;📝 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 JOB_COMPLETION_STATUS_POLL_INTERVAL_MILLIS = 0.5; | |
| const JOB_COMPLETION_STATUS_POLL_INTERVAL_MILLIS = 500; |
davemarco
left a comment
There was a problem hiding this comment.
Approved pending it's tested and still works.
Description
Taskfile.yml.log_viewer_webui->log-viewer-webui.Validation performed
clp-package/sbin/compress.sh ~/samples/hive-24hr.Debug mode
clp-package/sbin/stop-clp.sh log_viewer_webui.cd clp/component/log-viewer-webui; npm i; npm start8080and observed successful extraction job completion. The redirection to yscope-log-viewer would not work because the viewer app is not served at the same address.Summary by CodeRabbit
New Features
Refactor
Chores & Tests