build(log-viewer-webui): Migrate server codebase to TypeScript and update dependencies. #647
Conversation
…date dependencies.
WalkthroughThis pull request restructures the log viewer web UI by renaming tasks, updating file paths, and transitioning core components from JavaScript to TypeScript. It replaces legacy implementations with enhanced TypeScript alternatives, refines build and dependency configurations, introduces comprehensive type definitions and ESLint settings, and adjusts test cases. Additionally, new utility modules and improved error handling have been incorporated into the Fastify server’s startup and route management workflows. Changes
Sequence Diagram(s)sequenceDiagram
participant Main as main.ts
participant Env as env.ts
participant App as app.ts
participant Server as Fastify Instance
Main->>Env: parseEnvVars()
Env-->>Main: Return environment variables
Main->>App: Call app({ fastifyOptions, sqlDbUser, sqlDbPass })
App-->>Main: Return initialized Fastify instance
Main->>Server: instance.listen(host, port)
Server-->>Main: Server startup complete
sequenceDiagram
participant Client as Client
participant Server as Fastify Server
participant QHandler as Query Route Handler
participant DB as DbManager
participant S3 as S3Manager
Client->>Server: POST /query/extract-stream
Server->>QHandler: Route handling initiated
QHandler->>DB: getExtractedStreamFileMetadata(id, logEventIdx)
DB-->>QHandler: Return metadata or null
alt Metadata not found
QHandler->>DB: submitAndWaitForExtractStreamJob(details)
DB-->>QHandler: Return extraction result
end
alt S3Manager available
QHandler->>S3: getPreSignedUrl(s3UriString)
S3-->>QHandler: Return pre-signed URL
end
QHandler-->>Client: Respond with stream metadata (URL or local path)
✨ 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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.
|
|
||
| import app from "./app.js"; | ||
|
|
||
|
|
||
| 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.
| 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.
I think we can wrap the registration with our own Promise then we can await the promise.
Wrapping a non-awaited register() call into a Promise is a bad idea as well, since Fastify doesn't execute the registration before it listens.
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.
@junhaoliao see missed comment
| 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
# Conflicts: # components/log-viewer-webui/server/src/S3Manager.js # components/log-viewer-webui/server/src/routes/query.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
components/log-viewer-webui/server/src/S3Manager.ts (2)
53-58: Consider using type predicate for error handling.The error handling could be more TypeScript-idiomatic by using a type predicate function.
Consider this approach:
- } catch (error: unknown) { - if (false === error instanceof Error) { - throw error; - } - throw new Error(`Failed to generate pre-signed URL: ${error.message}`); + } catch (error: unknown) { + if (error instanceof Error) { + throw new Error(`Failed to generate pre-signed URL: ${error.message}`); + } + throw error; + }
66-74: Consider using a more specific type for the region parameter.The region parameter could benefit from a more specific type than
Nullable<string>.Consider using a union type of valid AWS regions:
type AWSRegion = 'us-east-1' | 'us-west-2' | /* other regions */; type Options = { region: Nullable<AWSRegion>; };components/log-viewer-webui/server/src/routes/query.ts (2)
6-6: Consider using a type-safe approach for settings import.The settings import could benefit from runtime type validation.
Consider using TypeBox to validate settings at runtime:
import settingsJson from "../../settings.json" with {type: "json"}; import {Type} from "@sinclair/typebox"; const SettingsSchema = Type.Object({ StreamTargetUncompressedSize: Type.Number(), StreamFilesS3PathPrefix: Type.String(), }); const settings = settingsJson as unknown; if (!Type.Check(SettingsSchema, settings)) { throw new Error('Invalid settings format'); }
20-28: Consider adding more specific constraints to the schema.The schema could be more restrictive to prevent invalid inputs.
Consider adding these constraints:
schema: { body: Type.Object({ extractJobType: Type.Enum(QUERY_JOB_TYPE), - logEventIdx: Type.Integer(), + logEventIdx: Type.Integer({ minimum: 0 }), streamId: Type.String({minLength: 1}), + }, { + additionalProperties: false }), },components/log-viewer-webui/server/package.json (1)
7-13: Consider adding type checking script.While build scripts are present, there's no dedicated type checking script.
Add a type checking script:
"scripts": { "build": "tsc --project tsconfig.json", "build:watch": "npm run build -- --watch", + "type-check": "tsc --noEmit", "lint:check": "npx eslint .", "lint:fix": "npm run lint:check -- --fix", "prod": "NODE_ENV=production node dist/src/main.js", "start": "NODE_ENV=development concurrently \"npm run build:watch\" \"nodemon dist/src/main.js\"",Taskfile.yml (1)
243-250: Consider adding TypeScript type checking to the build process.The build process could benefit from explicit type checking.
Add type checking before the build:
cd "server" rsync -a \ package.json package-lock.json \ "{{.OUTPUT_DIR}}/server/" + PATH="{{.G_NODEJS_22_BIN_DIR}}":$PATH npm run type-check PATH="{{.G_NODEJS_22_BIN_DIR}}":$PATH npm run build -- \ --outDir "{{.OUTPUT_DIR}}/server/dist"
📜 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 (8)
Taskfile.yml(6 hunks)components/clp-package-utils/clp_package_utils/scripts/start_clp.py(2 hunks)components/log-viewer-webui/server/package.json(2 hunks)components/log-viewer-webui/server/src/DbManager.ts(1 hunks)components/log-viewer-webui/server/src/S3Manager.ts(5 hunks)components/log-viewer-webui/server/src/app.ts(2 hunks)components/log-viewer-webui/server/src/routes/query.ts(1 hunks)components/log-viewer-webui/server/src/typings/common.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- components/log-viewer-webui/server/src/typings/common.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- components/log-viewer-webui/server/src/app.ts
- components/clp-package-utils/clp_package_utils/scripts/start_clp.py
- components/log-viewer-webui/server/src/DbManager.ts
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}`: - Prefer `false == ...
**/*.{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/S3Manager.ts
⏰ Context from checks skipped due to timeout of 90000ms (12)
- GitHub Check: ubuntu-focal-static-linked-bins
- GitHub Check: centos-stream-9-static-linked-bins
- GitHub Check: ubuntu-jammy-static-linked-bins
- GitHub Check: ubuntu-focal-dynamic-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-14, true)
- GitHub Check: build-macos (macos-13, false)
- GitHub Check: lint-check (ubuntu-latest)
- GitHub Check: build-macos (macos-13, true)
- GitHub Check: lint-check (macos-latest)
🔇 Additional comments (4)
components/log-viewer-webui/server/src/S3Manager.ts (2)
1-9: Well-organized imports!The imports are properly structured, with AWS SDK imports grouped together and TypeScript-specific imports separated.
11-14: Well-documented constant!The pre-signed URL expiry time constant is clearly defined and documented.
components/log-viewer-webui/server/package.json (1)
21-33:✅ Verification successful
Verify compatibility of dependency versions.
Some dependency versions seem to be from beyond my knowledge cutoff date.
Let's check the compatibility of these versions:
🌐 Web query:
What are the latest stable versions of @fastify/mongodb, @fastify/mysql, and @fastify/static that are compatible with Fastify 5?💡 Result:
The latest stable versions compatible with Fastify v5 are:
@fastify/mongodb
- Latest version:
9.0.2
Compatible via its MongoDB driver v6.x.x integration[4]. Requires a MongoDB connection URL and supports multiple named instances[4].@fastify/mysql
- Latest version:
5.0.2
Explicitly lists^5.xplugin versions as compatible with Fastify^5.xin its compatibility table[10]. Supports async/await and connection pooling[8][10].@fastify/static
- Latest version:
8.1.1
Matches Fastify v5 via its^8.xplugin line[3]. Includes updated security headers and performance optimizations for static file serving[3].
Key compatibility notes:
- All three plugins dropped support for Node.js <20 to align with Fastify v5’s requirements[5][9].
- Legacy packages (
fastify-mongodb,fastify-mysql,fastify-static) are deprecated and replaced by scoped@fastify/*equivalents[1][6][7].Upgrade using:
npm install @fastify/mongodb@9.0.2 @fastify/mysql@5.0.2 @fastify/static@8.1.1Citations:
- 1: https://www.npmjs.com/package/fastify-mongodb
- 2: https://github.com/fastify/fastify-mysql/releases
- 3: https://www.npmjs.com/package/@fastify/static
- 4: https://www.npmjs.com/package/@fastify/mongodb
- 5: https://fastify.io/docs/v5.1.x/Guides/Migration-Guide-V5/
- 6: https://www.npmjs.com/package/fastify-mysql?activeTab=versions
- 7: https://www.npmjs.com/package/fastify-static
- 8: https://github.com/fastify/fastify-mysql
- 9: https://openjsf.org/blog/fastifys-growth-and-success
- 10: https://www.npmjs.com/package/@fastify/mysql
Dependency Versions Confirmed for Fastify 5
The versions for @fastify/mongodb (9.0.2), @fastify/mysql (5.0.2), and @fastify/static (8.1.1) have been verified as the latest stable releases compatible with Fastify v5. No changes are necessary.
- Location:
components/log-viewer-webui/server/package.json(Lines 21-33)Taskfile.yml (1)
221-225: Well-organized TypeScript source files!The source files for the TypeScript migration are properly organized and included in the build process.
| resp.code(StatusCodes.BAD_REQUEST); | ||
| throw new Error(`Unable to extract stream with streamId=${streamId} at ` + | ||
| `logEventIdx=${logEventIdx}`); | ||
| } | ||
|
|
||
| streamMetadata = await fastify.dbManager.getExtractedStreamFileMetadata( | ||
| streamId, | ||
| logEventIdx | ||
| ); | ||
| if (null === streamMetadata) { | ||
| resp.code(StatusCodes.BAD_REQUEST); | ||
| throw new Error("Unable to extract stream with " + | ||
| `streamId=${streamId} at logEventIdx=${logEventIdx}`); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Extract duplicate error messages into constants.
The same error message is duplicated in two places.
Consider this refactor:
+const EXTRACT_ERROR_MESSAGE = (streamId: string, logEventIdx: number) =>
+ `Unable to extract stream with streamId=${streamId} at logEventIdx=${logEventIdx}`;
- throw new Error(`Unable to extract stream with streamId=${streamId} at ` +
- `logEventIdx=${logEventIdx}`);
+ throw new Error(EXTRACT_ERROR_MESSAGE(streamId, logEventIdx));
- throw new Error("Unable to extract stream with " +
- `streamId=${streamId} at logEventIdx=${logEventIdx}`);
+ throw new Error(EXTRACT_ERROR_MESSAGE(streamId, logEventIdx));📝 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.
| resp.code(StatusCodes.BAD_REQUEST); | |
| throw new Error(`Unable to extract stream with streamId=${streamId} at ` + | |
| `logEventIdx=${logEventIdx}`); | |
| } | |
| streamMetadata = await fastify.dbManager.getExtractedStreamFileMetadata( | |
| streamId, | |
| logEventIdx | |
| ); | |
| if (null === streamMetadata) { | |
| resp.code(StatusCodes.BAD_REQUEST); | |
| throw new Error("Unable to extract stream with " + | |
| `streamId=${streamId} at logEventIdx=${logEventIdx}`); | |
| } | |
| const EXTRACT_ERROR_MESSAGE = (streamId: string, logEventIdx: number) => | |
| `Unable to extract stream with streamId=${streamId} at logEventIdx=${logEventIdx}`; | |
| resp.code(StatusCodes.BAD_REQUEST); | |
| throw new Error(EXTRACT_ERROR_MESSAGE(streamId, logEventIdx)); | |
| } | |
| streamMetadata = await fastify.dbManager.getExtractedStreamFileMetadata( | |
| streamId, | |
| logEventIdx | |
| ); | |
| if (null === streamMetadata) { | |
| resp.code(StatusCodes.BAD_REQUEST); | |
| throw new Error(EXTRACT_ERROR_MESSAGE(streamId, logEventIdx)); | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
components/log-viewer-webui/server/src/typings/DbManager.ts (2)
22-44: Consider adding status transition documentation.While the status enum is well-defined, it would be beneficial to document the allowed status transitions (e.g., PENDING → RUNNING → SUCCEEDED/FAILED) to help prevent invalid state changes.
65-90: Consider adding runtime validation for configuration values.While the types provide compile-time safety, consider using a schema validation library (e.g., Zod, io-ts) to validate configuration values at runtime, especially for values loaded from environment variables or configuration files.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
components/log-viewer-webui/server/src/DbManager.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 (1)
- components/log-viewer-webui/server/src/DbManager.ts
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}`: - Prefer `false == ...
**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}: - Preferfalse == <expression>rather than!<expression>.
components/log-viewer-webui/server/src/typings/DbManager.ts
⏰ Context from checks skipped due to timeout of 90000ms (12)
- GitHub Check: ubuntu-jammy-static-linked-bins
- GitHub Check: ubuntu-focal-static-linked-bins
- GitHub Check: ubuntu-jammy-dynamic-linked-bins
- GitHub Check: centos-stream-9-static-linked-bins
- GitHub Check: ubuntu-focal-dynamic-linked-bins
- GitHub Check: centos-stream-9-dynamic-linked-bins
- GitHub Check: build-macos (macos-14, false)
- GitHub Check: lint-check (ubuntu-latest)
- GitHub Check: build-macos (macos-13, false)
- GitHub Check: lint-check (macos-latest)
- GitHub Check: build-macos (macos-13, true)
- GitHub Check: build (macos-latest)
🔇 Additional comments (3)
components/log-viewer-webui/server/src/typings/DbManager.ts (3)
1-21: LGTM! Well-structured type definitions with clear documentation.The job type definitions are well-organized and properly documented, with clear mapping to the Python counterpart.
46-63: LGTM! Excellent use of TypeScript features for type safety.The QueryJob interface effectively uses computed property names and enum values to ensure type safety and consistency between the table schema and TypeScript types.
92-104: LGTM! Well-organized exports following TypeScript best practices.The separation of type and value exports is clean and follows TypeScript best practices.
| promise: true, | ||
| connectionString: `mysql://${config.user}:${config.password}@${config.host}:` + | ||
| `${config.port}/${config.database}`, | ||
| }); |
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?
| * @throws {Error} If a pre-signed URL couldn't be generated. | ||
| */ | ||
| async getPreSignedUrl (s3UriString) { | ||
| async getPreSignedUrl (s3UriString: string) { |
There was a problem hiding this comment.
| async getPreSignedUrl (s3UriString: string) { | |
| async getPreSignedUrl (s3UriString: string): Promise<string> { |
There was a problem hiding this comment.
I think the typescript changes made readability worse.
- I asked chatGPT to simplify and it came up with this. Maybe just check it's okay. I'd also consider moving the parsing code out of main, then importing it, to keep main cleaner
import process from "node:process";
import { config as dotenvConfig } from "dotenv";
import app from "./app.js";
dotenvConfig({ path: [".env.local", ".env"] });
const ENV_TO_LOGGER = {
development: { transport: { target: "pino-pretty" } },
production: true,
test: false,
};
const parseEnvVars = () => {
const { NODE_ENV = "development", CLP_DB_USER, CLP_DB_PASS, HOST, PORT } = process.env;
if (!CLP_DB_USER || !CLP_DB_PASS || !HOST || !PORT) {
throw new Error("Missing required environment variables.");
}
return { NODE_ENV, CLP_DB_USER, CLP_DB_PASS, HOST, PORT: Number(PORT) };
};
const main = async () => {
const env = parseEnvVars();
const server = await app({
fastifyOptions: { logger: ENV_TO_LOGGER[env.NODE_ENV] },
sqlDbPass: env.CLP_DB_PASS,
sqlDbUser: env.CLP_DB_USER,
});
try {
await server.listen({ host: env.HOST, port: env.PORT });
} catch (e) {
console.error(e);
process.exit(1);
}
};
main().catch(e => {
console.error(e);
process.exit(1);
});
There was a problem hiding this comment.
I'd also consider moving the parsing code out of main, then importing it, to keep main cleaner
For now, i only move the env var related code into util/env.ts.
I feel that we type the parsed environment variables is still helpful in ensuring we are parsing the variables correctly and in avoiding type assertions when they're used in main().
There was a problem hiding this comment.
I dont know, id still look through it and try to clean it up a bit.
| "lint:fix": "npx eslint --fix --no-eslintrc --config package.json src", | ||
| "prod": "NODE_ENV=production node src/main.js", | ||
| "start": "NODE_ENV=development nodemon src/main.js", | ||
| "build": "tsc --project tsconfig.json", |
There was a problem hiding this comment.
in client side, we are using tsc -b. Do you think this is better? I checked online a bit, it seemed like build is more general, also i dont think neccesary to specify path
There was a problem hiding this comment.
Right, i was following this Fastify doc: https://fastify.dev/docs/latest/Reference/TypeScript/#getting-started
but it seems for performance we might always prefer tsc -b.
i updated the script to use tsc -b
| if (fastify.hasDecorator("s3Manager")) { | ||
| // eslint-disable-next-line @typescript-eslint/no-non-null-assertion | ||
| streamMetadata.path = await fastify.s3Manager!.getPreSignedUrl( |
There was a problem hiding this comment.
| if (fastify.hasDecorator("s3Manager")) { | |
| // eslint-disable-next-line @typescript-eslint/no-non-null-assertion | |
| streamMetadata.path = await fastify.s3Manager!.getPreSignedUrl( | |
| if (fastify.hasDecorator("s3Manager") && "undefined" !== typeof fastify.s3Manager) { | |
| streamMetadata.path = await fastify.s3Manager.getPreSignedUrl( |
|
|
||
| import app from "./app.js"; | ||
|
|
||
|
|
||
| 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.
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.
Maybe add a comment where you took this config from?
Also maybe add exclude for node modules
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
components/log-viewer-webui/server/src/utils/time.ts (1)
9-11: Consider defensive coding for negative input values.The function correctly implements a sleep utility, but it doesn't handle negative values for the seconds parameter, which would still result in an immediate timeout resolution.
const sleep = (seconds: number): Promise<void> => new Promise((resolve) => { - setTimeout(resolve, seconds * MILLIS_PER_SECOND); + const delay = Math.max(0, seconds) * MILLIS_PER_SECOND; + setTimeout(resolve, delay); });components/log-viewer-webui/server/src/utils/env.ts (2)
60-82: Check for missing environment variables early in development.
The mandatory variables are thoroughly validated here, which is good. Consider logging a more explicit message clarifying which file(s) were used when loading.env.localand.env.
89-94: Clarify console output for unrecognizedNODE_ENV.
The console message is helpful, but you may want to standardize logging using your logger rather thanconsole.logdirectly, ensuring consistent formatting.components/log-viewer-webui/server/src/typings/DbManager.ts (2)
9-12: Consider using explicit numeric assignment for enum members.
Although default ordinal values are acceptable, explicitly assigning integer values toQUERY_JOB_TYPEmembers may improve maintainability and backward compatibility.
51-56: Validate column name usage for table indexing.
Ensure these enumerated column names match your database schema exactly and that indexing or primary keys exist for performance-critical fields.components/log-viewer-webui/server/src/DbManager.ts (1)
135-163: Flatten repeated job configurations with a helper function.
The repeated structure forjobConfigin lines 151-163 may be extracted into a reusable helper, reducing duplication and clarifying logic for different job types.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
components/log-viewer-webui/server/src/DbManager.ts(1 hunks)components/log-viewer-webui/server/src/S3Manager.ts(4 hunks)components/log-viewer-webui/server/src/main.ts(1 hunks)components/log-viewer-webui/server/src/typings/DbManager.ts(1 hunks)components/log-viewer-webui/server/src/utils/env.ts(1 hunks)components/log-viewer-webui/server/src/utils/time.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- components/log-viewer-webui/server/src/main.ts
- components/log-viewer-webui/server/src/S3Manager.ts
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}`: - Prefer `false == ...
**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}: - Preferfalse == <expression>rather than!<expression>.
components/log-viewer-webui/server/src/utils/time.tscomponents/log-viewer-webui/server/src/utils/env.tscomponents/log-viewer-webui/server/src/DbManager.tscomponents/log-viewer-webui/server/src/typings/DbManager.ts
⏰ Context from checks skipped due to timeout of 90000ms (12)
- GitHub Check: ubuntu-jammy-dynamic-linked-bins
- GitHub Check: ubuntu-jammy-static-linked-bins
- GitHub Check: ubuntu-focal-static-linked-bins
- GitHub Check: ubuntu-focal-dynamic-linked-bins
- GitHub Check: centos-stream-9-dynamic-linked-bins
- GitHub Check: centos-stream-9-static-linked-bins
- GitHub Check: build-macos (macos-14, false)
- GitHub Check: build-macos (macos-14, true)
- GitHub Check: build-macos (macos-13, true)
- GitHub Check: build-macos (macos-13, false)
- GitHub Check: lint-check (macos-latest)
- GitHub Check: lint-check (ubuntu-latest)
🔇 Additional comments (9)
components/log-viewer-webui/server/src/utils/time.ts (3)
1-1: Good use of constants for improved readability.Using a named constant for the conversion factor makes the code more maintainable and readable.
3-8: Well-documented function with clear JSDoc.The JSDoc comment provides excellent documentation for the function, clearly explaining its purpose, parameter, and return value.
13-14: Appropriate named export.Using named exports is a good practice for utility functions, as it makes imports more explicit.
components/log-viewer-webui/server/src/utils/env.ts (2)
9-10: Ensure theNodeEnvtype covers all relevant runtime environments.
The type declaration is suitable for most Node.js applications, but confirm that other custom or staging environments do not exist. If additional staging environments are needed (e.g., "staging"), consider extending the type accordingly.
24-32: Validate correctness of logger configuration in production.
While settingproduction: trueis valid, ensure you have indeed disabled sensitive logging in production or configured pino accordingly. Overly verbose logs may impact performance or leak internal information.components/log-viewer-webui/server/src/typings/DbManager.ts (1)
14-20: Name consistency between code and references.
The documentation referencesQueryJobTypefrom another file. Confirm that the naming and usage are aligned across files to avoid confusion.components/log-viewer-webui/server/src/DbManager.ts (3)
34-34: Polling interval might be misinterpreted as half a millisecond.
This appears to be the same concern previously raised. If the intent is half a second, change the value to500.
88-97: Sanitize connection string parameters.
Same as past feedback. Ifconfig.user,config.password, or other fields contain special characters, the connection may fail or lead to unexpected behaviour. Consider encoding them.
212-248: Prevent infinite polling.
This loop could continue indefinitely if the job never transitions from a waiting state. Integrate a maximum retry limit or timeout to avoid a potential infinite loop.
…stify.hasDecorator("s3Manager")` check.
…oating-promises" and remove the `await`.
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)
36-39: Dev dependencies updated successfully.Pinning exact versions can improve reproducibility, but the current approach with caret versions helps maintain up-to-date libraries.
📜 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 (5)
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/app.test.ts(1 hunks)components/log-viewer-webui/server/src/routes/query.ts(1 hunks)components/log-viewer-webui/server/tsconfig.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- components/log-viewer-webui/server/src/app.test.ts
- components/log-viewer-webui/server/tsconfig.json
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}`: - Prefer `false == ...
**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}: - Preferfalse == <expression>rather than!<expression>.
components/log-viewer-webui/server/src/routes/query.ts
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: ubuntu-jammy-static-linked-bins
- GitHub Check: centos-stream-9-static-linked-bins
- GitHub Check: ubuntu-jammy-dynamic-linked-bins
- GitHub Check: centos-stream-9-dynamic-linked-bins
- GitHub Check: lint-check (ubuntu-latest)
🔇 Additional comments (10)
components/log-viewer-webui/server/eslint.config.mjs (4)
1-3: Imports look consistent with eslint-config-yscope.These imports align with the recommended approach of splitting configs into separate modules for maintainability.
6-12: Ignore patterns are properly declared.Excluding
dist/andnode_modules/is standard practice to ensure generated or external files are not linted.
17-28: Rule “new-cap” usage is coherent.Allowing capitalised TypeBox calls for “Type.Enum” and “Type.Integer” ensures the linter accommodates TypeBox’s recommended usage.
30-53: TypeScript rule set is well structured.Disabling
require-awaitfor Fastify plug-ins and enablingno-floating-promisesare sensible guidelines.components/log-viewer-webui/server/src/routes/query.ts (3)
1-16: Consistent type usage and code clarity.Your imports for
TypeBoxTypeProviderandTypedemonstrate strong type enforcement via TypeBox. The JSDoc on lines 11-16 provides helpful context for developers.
30-33: Condition check matches coding guidelines.Using
false === EXTRACT_JOB_TYPES.has(extractJobType)rather than a negation operator (!) adheres to the stated preference for literal comparisons.
65-71: Good fallback logic for stream path resolution.The run-time check for an existing
s3Managerensures graceful degradation if the S3 manager is unavailable.components/log-viewer-webui/server/package.json (3)
5-5: Migration to TypeScript entry point.Replacing “main”: “src/main.js” with “src/main.ts” is consistent with the new TypeScript codebase.
7-12: Build and start scripts well defined.Using “tsc -b” for project references and concurrently for development improves the TypeScript workflow and hot reloading.
19-33: Updated dependencies align with TypeScript migration.Adding “@fastify/type-provider-typebox” and “@sinclair/typebox” are key steps for typed schemas.
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