build(log-viewer-webui): Migrate server codebase to TypeScript and update dependencies. #647
Conversation
…date dependencies.
WalkthroughThis pull request introduces modifications to the log viewer web UI server, transitioning from JavaScript to TypeScript and enhancing the organization of the project. Key changes include renaming tasks in Changes
Possibly Related PRs
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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.
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.
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
Bug Fixes
Refactors
Chores