chore(log-viewer-webui-client): Convert the codebase to TypeScript; Update dependencies. #645
Conversation
WalkthroughThis pull request represents a comprehensive migration of the log viewer web UI from JavaScript to TypeScript. The changes involve updating the project configuration, modifying import statements, and enhancing type safety across multiple components. The transformation includes updating the build process, dependency management, and introducing more robust type definitions for various application components, particularly focusing on query and loading state management. Changes
Sequence DiagramsequenceDiagram
participant Client as Web UI
participant API as Query Service
Client->>API: submitExtractStreamJob()
API-->>Client: Return ExtractStreamResp
Client->>Client: Update Loading State
Client->>Client: Handle Response/Errors
Possibly Related PRs
Suggested Reviewers
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: 0
🧹 Nitpick comments (4)
components/log-viewer-webui/client/src/index.tsx (1)
9-10: Consider gracefully handling potential null references.While the non-null assertion operator (
!) informs TypeScript that the element will never be null, it could still lead to runtime errors if the element does not actually exist. It might be safer to throw an error or provide a fallback to handle this scenario more robustly.Below is a sample approach:
-const root = createRoot(document.getElementById("root")!); +const rootElement = document.getElementById("root"); +if (false == rootElement) { + throw new Error("Root element not found in DOM"); +} +const root = createRoot(rootElement);components/log-viewer-webui/client/src/App.tsx (1)
10-10: Confirm JSDoc return annotation.Removing the explicit return type is often fine in TypeScript since return types can be inferred. However, please ensure that your documentation accurately reflects the function’s intended return structure.
components/log-viewer-webui/client/src/api/query.ts (1)
29-44: Enhance error handling and request cancellation.While
submitExtractStreamJobis straightforward, consider adding optional request cancellation (via Axios.cancelToken) or error handling to avoid unhandled promise rejections if the server or network fails.components/log-viewer-webui/client/src/ui/QueryStatus.tsx (1)
29-29: Document return values more explicitly.
Please clarify the returned type or structure to help future readers understand the component’s outcome.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
components/log-viewer-webui/client/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (12)
components/log-viewer-webui/client/package.json(2 hunks)components/log-viewer-webui/client/src/App.tsx(1 hunks)components/log-viewer-webui/client/src/api/query.js(0 hunks)components/log-viewer-webui/client/src/api/query.ts(1 hunks)components/log-viewer-webui/client/src/index.tsx(1 hunks)components/log-viewer-webui/client/src/typings/common.ts(1 hunks)components/log-viewer-webui/client/src/typings/query.js(0 hunks)components/log-viewer-webui/client/src/typings/query.ts(1 hunks)components/log-viewer-webui/client/src/ui/Loading.tsx(3 hunks)components/log-viewer-webui/client/src/ui/QueryStatus.tsx(3 hunks)components/log-viewer-webui/client/tsconfig.json(1 hunks)components/log-viewer-webui/client/webpack.config.js(4 hunks)
💤 Files with no reviewable changes (2)
- components/log-viewer-webui/client/src/api/query.js
- components/log-viewer-webui/client/src/typings/query.js
✅ Files skipped from review due to trivial changes (1)
- components/log-viewer-webui/client/src/typings/common.ts
🧰 Additional context used
📓 Path-based instructions (7)
components/log-viewer-webui/client/src/index.tsx (1)
Pattern **/*.{cpp,hpp,java,js,jsx,ts,tsx}: - Prefer false == <expression> rather than !<expression>.
components/log-viewer-webui/client/webpack.config.js (1)
Pattern **/*.{cpp,hpp,java,js,jsx,ts,tsx}: - Prefer false == <expression> rather than !<expression>.
components/log-viewer-webui/client/src/App.tsx (1)
Pattern **/*.{cpp,hpp,java,js,jsx,ts,tsx}: - Prefer false == <expression> rather than !<expression>.
components/log-viewer-webui/client/src/api/query.ts (1)
Pattern **/*.{cpp,hpp,java,js,jsx,ts,tsx}: - Prefer false == <expression> rather than !<expression>.
components/log-viewer-webui/client/src/ui/Loading.tsx (1)
Pattern **/*.{cpp,hpp,java,js,jsx,ts,tsx}: - Prefer false == <expression> rather than !<expression>.
components/log-viewer-webui/client/src/typings/query.ts (1)
Pattern **/*.{cpp,hpp,java,js,jsx,ts,tsx}: - Prefer false == <expression> rather than !<expression>.
components/log-viewer-webui/client/src/ui/QueryStatus.tsx (1)
Pattern **/*.{cpp,hpp,java,js,jsx,ts,tsx}: - Prefer false == <expression> rather than !<expression>.
🔇 Additional comments (48)
components/log-viewer-webui/client/src/App.tsx (2)
1-1: Confirm updated import path for CssVarsProvider.
The new import path @mui/joy is correct for the current version of MUI Joy. Please confirm that this is aligned with your package.json dependencies.
3-4: Ensure consistent usage of TypeScript imports.
Dropping the file extensions is appropriate given your tsconfig settings. Double-check that all references to these imports have been updated accordingly in the rest of the project.
components/log-viewer-webui/client/src/api/query.ts (4)
1-4: Validate Axios import for TypeScript usage.
Your imports from Axios are compatible with TypeScript. Ensure that your tsconfig includes the necessary type definitions for Axios to avoid runtime or type definition issues.
6-16: Enum usage for QUERY_JOB_TYPE.
Leveraging QUERY_JOB_TYPE in TypeScript helps prevent invalid job-type values. Confirm that calling code passes valid enum members.
9-16: Check interface fields align with backend schema.
The fields in ExtractStreamResp appear to match typical streaming extraction responses. Confirm with backend docs that the field names (begin_msg_ix, is_last_chunk, etc.) remain stable or forward-compatible.
46-46: Export structure is consistent with TypeScript norms.
Exporting submitExtractStreamJob from this file provides a clean API surface for your data-fetching logic. Good work keeping it readable and modular.
components/log-viewer-webui/client/src/typings/query.ts (6)
1-5: Numeric enum usage is valid.
Using numeric enums for QUERY_LOADING_STATE is fine. If you ever need more explicit debugging or logging, consider string enums or an equivalent mapping for greater clarity.
7-12: Filtering enum values to numeric types.
The approach of filtering out string enum keys from the Object.values(QUERY_LOADING_STATE) array is efficient. This logic is correct for numeric enums.
14-18: QUERY_JOB_TYPE validity.
Ensure these job types don’t overlap with existing or potential future ones. Numeric enums can inadvertently overlap if not carefully tracked, so consider a stable numbering scheme.
20-23: Interface definitions match usage.
QueryLoadingStateDescription is clearly structured. Nicely done on using a separate interface for clarity.
28-43: Providing descriptive labels is helpful.
QUERY_LOADING_STATE_DESCRIPTIONS nicely documents each enum’s purpose. This improves maintainability. Great usage of Object.freeze for immutability.
45-50: Exports are well-organized.
All relevant symbols are exported for external usage. This aligns with a neat and modular TypeScript codebase.
components/log-viewer-webui/client/webpack.config.js (7)
36-36: Entry point switched to TypeScript index.
Switching from index.jsx to index.tsx is consistent with the migration to TypeScript. Verify that your scripts and references in package.json also reflect this change.
37-39: Mode configuration is correct.
Using environment-based modes is standard practice. Ensure that your build process sets NODE_ENV correctly.
43-43: Refined test pattern for TS/TSX.
Updating the regex ensures proper handling of .ts/.tsx files. Confirm that .js or .jsx is no longer needed if you’ve fully migrated.
56-56: Added TypeScript Babel preset.
Including @babel/preset-typescript is crucial for transpiling TypeScript. Looks good.
73-87: SplitChunks for better performance.
Your splitChunks configuration ensures proper vendor chunking to boost caching and reduce bundle sizes. Good practice.
103-104: Extended resolve with TS/TSX.
Adding .ts/.tsx to resolve.extensions is correct for a TypeScript codebase. Confirm that any .jsx references are not needed.
109-109: Exporting config object.
Exporting the Webpack config object is a clean approach that simplifies usage. Nice move.
components/log-viewer-webui/client/src/ui/QueryStatus.tsx (8)
7-7: Implementation of isAxiosError is beneficial.
This import helps refine error handling for Axios requests, ensuring more accurate detection of Axios-related issues.
9-15: Imports for new type definitions appear streamlined.
Bringing in submitExtractStreamJob, Nullable, and the QUERY_LOADING_STATE constants from separate files fosters modularity and clarity.
32-35: Adoption of strong state typing looks solid.
Using generic types for useState clarifies the permissible values, reducing unintended usage.
57-57: Confirm logical intention of false === streamType in EXTRACT_JOB_TYPE.
While this meets the style preference for false == expression, the operator precedence may be confusing. Consider adding parentheses or using an explicit check (e.g. false == (streamType in EXTRACT_JOB_TYPE)) to convey your intent more clearly.
64-65: Sanitizing user input is prudent.
Using Number() ensures logEventIdx is converted to a numeric type. This helps avoid injection issues and fosters type consistency.
70-72: Updating state on job submission is accurate.
Transitioning to the WAITING state upon submission clarifies the user that a process is in progress.
76-79: Smooth transition to LOADING with correct offset usage.
Calculating innerLogEventNum ensures the log event index aligns with the server’s base offset. Redirection is correct, and usage of window.location.href is consistent with the new approach.
82-84: Comprehensive error handling.
Distinguishing Axios errors from general exceptions helps provide more accurate feedback to the user. Good approach.
components/log-viewer-webui/client/src/ui/Loading.tsx (9)
1-1: Neat import of React.
Explicitly importing React ensures consistent usage of JSX transformations under the TypeScript environment.
12-12: DefaultColorPalette usage is consistent.
Allows for a typed palette configuration, reducing the risk of undefined colour usage.
14-14: Nullable type fosters clarity for optional fields.
This approach clearly conveys which properties can be null, enhancing reliability.
16-19: Use of enumerated states simplifies logic.
Referencing QUERY_LOADING_STATE and related constants reduces magic strings.
24-31: LoadingStepProps interface definitions are straightforward.
These type annotations provide clarity for each required prop, reducing guesswork for future contributors.
49-50: Conditional colour assignment is intuitive.
The code clearly differentiates active and error states, enhancing readability.
84-87: LoadingProps interface is well-defined.
Specifying these types ensures consistent usage of currentState and errorMsg throughout the component.
97-100: Structured destructuring of props.
Explicitly typed parameters help TypeScript detect possible misuses at compile time.
101-103: Building steps array based on states improves maintainability.
Looping through known state values in QUERY_LOADING_STATE_VALUES helps ensure all defined states are handled.
components/log-viewer-webui/client/package.json (4)
5-5: Switching main entry to src/index.tsx is appropriate.
Reflects the recent TypeScript migration, ensuring the build references the correct entry point.
7-7: Upgrading the build script.
Using --config-node-env production aligns with the updated webpack CLI usage, which resolves deprecation in older versions.
16-35: Dependency updates facilitate TypeScript support.
Including @babel/preset-typescript plus updated Babel/webpack dependencies is crucial for the TypeScript build pipeline.
36-86: Dependencies reorganized for clarity and usage.
Promoting relevant libraries (e.g. react, axios) into dependencies ensures they are accessible in production. ESlint configuration is robust, with coverage for TypeScript files.
components/log-viewer-webui/client/tsconfig.json (8)
1-7: Inclusion/exclusion paths are clearly defined.
This separation ensures extraneous files such as node_modules do not affect compilation time or produce unnecessary errors.
8-35: Targeting ES2022 with DOM library is appropriate.
Allows usage of next-gen JavaScript features while supporting browser-based functionalities.
39-44: ESNext module system with bundler resolution is modern.
Harmonizes well with contemporary bundlers and ensures top-level await or other ESNext features can be leveraged.
50-58: Permitting JSON imports is beneficial.
This functionality can consolidate certain config or data definitions directly into the TypeScript code.
63-67: allowJs and checkJs help unify JS and TS.
This configuration is valuable during transitional phases, ensuring JS still gets type-checked where possible.
74-83: sourceMap and noEmit are well-chosen.
Emitting source maps aids debugging, while noEmit clarifies that compiled output is handled by the bundler.
100-106: Enabling isolated modules and ES module interop is standard.
Helps ensure each file is self-sufficient during transpilation, plus broadens import compatibility for third-party modules.
108-143: Strict type checking is thoroughly configured.
Enabling strict mode and measures such as noImplicitAny, strictNullChecks, and noUnusedLocals fosters maintainable, predictable code.
There was a problem hiding this comment.
Everything was taken from https://github.com/y-scope/yscope-log-viewer/blob/7e6073f7c996f55721f41088ac2d8506085981ad/tsconfig.json except noted below.
| @@ -0,0 +1,152 @@ | |||
| { | |||
| "include": [ | |||
| "src", | |||
There was a problem hiding this comment.
compared to the one in yscope-log-viewer, removed
"jest.config.ts",
"test",
| /* Specify a set of bundled library declaration files that describe the target runtime environment. */ | ||
| "lib": [ | ||
| "ES2022", | ||
| "DOM", |
There was a problem hiding this comment.
compared to the one in yscope-log-viewer, removed
"DOM.Iterable",
"WebWorker",
| ], | ||
| }, | ||
| }; | ||
|
|
||
| export default () => { |
There was a problem hiding this comment.
@kirkrodrigues I don't recall if we discussed this for #468 - Did we have to export a function and change the config.mode dynamically? Can we simply define the mode in the above config object and return the object directly?
There was a problem hiding this comment.
Yeah, that's my understanding based on this: https://webpack.js.org/configuration/configuration-types/#exporting-a-function
I think the old code was wrong since it should've been using the env argument to determine whether it was prod/dev rather than isProduction. Based on a glance at this, I guess we shouldn't be using process.env.NODE_ENV in a webpack config.
There was a problem hiding this comment.
Gotcha. Let me defer this after investigating the viability of turning the app into a Vite-React app.
| @@ -33,11 +33,14 @@ const config = { | |||
| devtool: isProduction ? | |||
| "source-map" : | |||
| "eval-source-map", | |||
| entry: path.resolve(dirname, "src", "index.jsx"), | |||
| entry: path.resolve(dirname, "src", "index.tsx"), | |||
| mode: isProduction ? | |||
There was a problem hiding this comment.
Note this was moved from the default-exported function.
|
Alternatively, shall we look into turning this into a Vite-React app? That way we don't have to manage the build configs on our own with webpack. |
We could, I guess. |
Description
Validation performed
clp-package/sbin/compress.sh ~/samples/hive-24hr.Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores
Style