new-log-viewer: Integrate latest clp-ffi-js with support for log-level filtering; Add log-level selector to StatusBar.
#77
Conversation
WalkthroughThe changes introduce a new React component, Changes
Possibly related PRs
Suggested reviewers
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 (
|
new-log-viewer/src/utils/data.ts
Outdated
| @@ -38,7 +38,45 @@ const getMapValueWithNearestLessThanOrEqualKey = <T>( | |||
| map.get(lowerBoundKey) as T; | |||
| }; | |||
|
|
|||
| /** | |||
| * Creates an array of numbers in a specified range [startNum, startNum + steps]. | |||
There was a problem hiding this comment.
This utility function was created with a reference to Python's build-in https://docs.python.org/3.12/library/stdtypes.html#range method.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (9)
new-log-viewer/src/components/StatusBar/LogLevelSelect/index.css (1)
1-4: LGTM! Consider adding a flex-wrap property.The use of flexbox for layout and the gap property for spacing between items is appropriate and efficient. The class name is specific and follows a clear naming convention.
Consider adding
flex-wrap: wrap;to ensure the content remains accessible on smaller screens:.log-level-select-render-value-box { display: flex; gap: 2px; + flex-wrap: wrap; }new-log-viewer/src/components/StatusBar/LogLevelSelect/LogLevelChip.css (1)
2-3: Consider adhering to the stylelint naming convention.The use of
stylelint-disable-next-linesuggests a conflict with the project's naming convention for custom properties. Instead of disabling the linter, consider renaming the custom property to follow the project's conventions.For example, you could use kebab-case:
- /* stylelint-disable-next-line custom-property-pattern */ - --Chip-radius: 0; + --chip-radius: 0;This change would maintain consistency with common CSS naming conventions and eliminate the need for the linter disable comment.
new-log-viewer/src/typings/logs.ts (1)
25-25: Correct implementation of MAX_LOG_LEVEL and updated exports.The implementation of
MAX_LOG_LEVELand the updated export statement are correct:
MAX_LOG_LEVELefficiently usesMath.max()with the spread operator.- The export statement correctly includes the new constants.
Consider adding a brief comment for
MAX_LOG_LEVELto improve code clarity, similar to the other constants:/** * The highest numeric value in the LOG_LEVEL enum. */ const MAX_LOG_LEVEL = Math.max(...LOG_LEVEL_VALUES);Also applies to: 33-34
new-log-viewer/public/index.html (1)
13-13: LGTM! Consider adding the crossorigin attribute for consistency.The addition of the Roboto Mono font is a good choice for a log viewer application, as it will provide a monospaced option for displaying log entries or code snippets. The placement of the new link tag is appropriate, following the existing Roboto font inclusion.
For consistency with the preconnect link above, you might consider adding the crossorigin attribute to the new link tag:
- <link rel="stylesheet" href="https://fonts.googleapis.com/css2?family=Roboto+Mono&display=swap"> + <link rel="stylesheet" crossorigin href="https://fonts.googleapis.com/css2?family=Roboto+Mono&display=swap">This change is optional but would maintain consistency across font-related links.
new-log-viewer/src/components/StatusBar/LogLevelSelect/LogLevelChip.tsx (2)
1-25: LGTM! Consider using an enum for LOG_LEVEL_COLOR_MAP keys.The imports and constant definitions look good. The use of
Object.freeze()forLOG_LEVEL_COLOR_MAPis a nice touch for immutability.Consider using an enum for the
LOG_LEVELkeys inLOG_LEVEL_COLOR_MAPto improve type safety and maintainability. For example:enum LOG_LEVEL { NONE = 'NONE', TRACE = 'TRACE', // ... other levels } const LOG_LEVEL_COLOR_MAP: Record<LOG_LEVEL, DefaultColorPalette> = Object.freeze({ [LOG_LEVEL.NONE]: "neutral", [LOG_LEVEL.TRACE]: "neutral", // ... other mappings });This change would make the code more robust against typos and easier to refactor in the future.
49-65: LGTM! Consider adding an aria-label for better accessibility.The render function is well-implemented, using MUI Joy UI components effectively. The use of
LOG_LEVEL_COLOR_MAPensures consistent styling across different log levels.To improve accessibility, consider adding an
aria-labelto the Chip component. This will provide more context for screen readers. For example:<Chip className={"log-level-chip"} color={LOG_LEVEL_COLOR_MAP[value]} variant={"outlined"} onClick={handleChipClick} aria-label={`${name} log level`} > {name[0]} </Chip>This change ensures that screen reader users get the full context of what the chip represents, rather than just the first letter of the log level.
new-log-viewer/src/utils/data.ts (1)
41-76: Well-implemented range function with room for minor optimizationThe
rangefunction is well-implemented and closely follows the behaviour of Python's built-inrangefunction. The error handling, parameter defaults, and logic for both positive and negative steps are correct and efficient.Consider a minor optimization to reduce code duplication:
- if (0 < step) { - for (let i = start; i < stop; i += step) { - result.push(i); - } - } else { - for (let i = start; i > stop; i += step) { - result.push(i); - } - } + const condition = step > 0 ? (i: number) => i < stop : (i: number) => i > stop; + for (let i = start; condition(i); i += step) { + result.push(i); + }This change reduces code duplication and makes the function more concise while maintaining its functionality.
new-log-viewer/src/components/StatusBar/LogLevelSelect/index.tsx (2)
39-55: Review type usage inhandleRenderValuefunctionThere appears to be type casting with
as stringforselectedOption.label. Ensure that the types forselectedOptionalign with the expected properties to avoid unnecessary type assertions. Refining the types can enhance type safety and code clarity.
57-67: Clarify conditional logic inhandleSelectChangeThe logic within
handleSelectChangediffers based on whether any log levels are selected, which might be unclear to future developers. Adding comments or refactoring the code for readability can improve maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
- new-log-viewer/public/index.html (1 hunks)
- new-log-viewer/src/components/StatusBar/LogLevelSelect/LogLevelChip.css (1 hunks)
- new-log-viewer/src/components/StatusBar/LogLevelSelect/LogLevelChip.tsx (1 hunks)
- new-log-viewer/src/components/StatusBar/LogLevelSelect/index.css (1 hunks)
- new-log-viewer/src/components/StatusBar/LogLevelSelect/index.tsx (1 hunks)
- new-log-viewer/src/components/StatusBar/index.tsx (2 hunks)
- new-log-viewer/src/typings/logs.ts (1 hunks)
- new-log-viewer/src/utils/data.ts (1 hunks)
🔇 Additional comments (10)
new-log-viewer/src/components/StatusBar/LogLevelSelect/LogLevelChip.css (1)
1-7: Overall assessment: Good styling choices with room for improvement.The CSS for the log level chip component makes appropriate choices for font family and weight, which will enhance readability and emphasis. However, there are a few areas that could be improved:
- The custom property naming could be adjusted to follow project conventions.
- The use of
!importantshould be reconsidered to improve maintainability.- Increasing selector specificity could resolve the need for
!important.Addressing these points will result in more robust and maintainable CSS for the log level chip component.
new-log-viewer/src/typings/logs.ts (2)
11-16: Well-implemented constant for log level names.The implementation of
LOG_LEVEL_NAMESis correct and follows good practices:
- Using
Object.freeze()ensures immutability.- The filter function correctly selects only string values from the enum.
- The constant is well-documented with a clear comment.
18-23: Well-implemented constant for log level values.The implementation of
LOG_LEVEL_VALUESis correct and consistent withLOG_LEVEL_NAMES:
- Using
Object.freeze()ensures immutability.- The filter function correctly selects only numeric values from the enum.
- The constant is well-documented with a clear comment.
new-log-viewer/src/components/StatusBar/index.tsx (3)
12-12: LGTM: Import statement for LogLevelSelect is correct.The import statement for the new LogLevelSelect component is properly formatted and uses the correct relative path. This aligns with the PR objective of adding a log level selector component to the StatusBar.
Line range hint
1-54: Overall, the changes look good and align with the PR objectives.The modifications to the StatusBar component are minimal and focused, successfully introducing the LogLevelSelect component as intended. The implementation is clean and doesn't introduce any apparent issues. These changes effectively contribute to the goal of adding a log level selector component to the StatusBar of the log viewer application.
41-41: LGTM: LogLevelSelect component added correctly.The LogLevelSelect component has been appropriately added to the StatusBar, positioned between the status message and the existing Button component. This implementation aligns with the PR objective of introducing a log level selector component within the StatusBar.
Could you please verify if the LogLevelSelect component requires any props? If it does, ensure they are properly passed. Here's a script to check the LogLevelSelect component definition:
✅ Verification successful
LGTM: LogLevelSelect component added correctly.
The LogLevelSelect component has been appropriately added to the StatusBar, positioned between the status message and the existing Button component. This implementation aligns with the PR objective of introducing a log level selector component within the StatusBar.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check the LogLevelSelect component definition for any required props # Test: Search for the LogLevelSelect component definition ast-grep --lang typescript --pattern 'const LogLevelSelect = ({ $$ }: $_) => { $$ }'Length of output: 86
Script:
#!/bin/bash # Description: Check the LogLevelSelect component definition for any required props using updated patterns # Search for function declaration of LogLevelSelect ast-grep --lang typescript --pattern 'function LogLevelSelect($_) { $$$ }' # Search for arrow function with explicit return type ast-grep --lang typescript --pattern 'const LogLevelSelect = ($_): $_ => { $$$ }' # Search for interface defining props for LogLevelSelect ast-grep --lang typescript --pattern 'interface LogLevelSelectProps { $$$ }'Length of output: 239
Script:
#!/bin/bash # Description: Further check the LogLevelSelect component definition for any required props # Search for class-based LogLevelSelect component ast-grep --lang typescript --pattern 'class LogLevelSelect extends React.Component<$_, $_> { $$$ }' # Search for default export of LogLevelSelect ast-grep --lang typescript --pattern 'export default function LogLevelSelect($_) { $$$ }' ast-grep --lang typescript --pattern 'export default LogLevelSelect' # Search for import statements of LogLevelSelect rg 'import\s+LogLevelSelect\s+from\s+["'\''].*["'\'']' --type typescriptLength of output: 374
Script:
#!/bin/bash # Description: Further check the LogLevelSelect component definition for any required props with corrected rg usage # Search for class-based LogLevelSelect component ast-grep --lang typescript --pattern 'class LogLevelSelect extends React.Component<$_, $_> { $$$ }' # Search for default export of LogLevelSelect ast-grep --lang typescript --pattern 'export default function LogLevelSelect($_) { $$$ }' ast-grep --lang typescript --pattern 'export default LogLevelSelect' # Search for import statements of LogLevelSelect without using --type rg 'import\s+LogLevelSelect\s+from\s+["'\''].*["'\'']' -g '*.ts' -g '*.tsx'Length of output: 439
new-log-viewer/src/components/StatusBar/LogLevelSelect/LogLevelChip.tsx (2)
43-47: LGTM! Well-structured component and event handler.The component declaration and
handleChipClickfunction are well-implemented. Good job on stopping event propagation and using the functional update form for the state change.
66-66: LGTM! Proper component export.The default export of the component is correct and follows common React practices.
new-log-viewer/src/utils/data.ts (1)
77-81: Correct export of the new range functionThe
rangefunction is correctly exported alongside the existing utility functions.new-log-viewer/src/components/StatusBar/LogLevelSelect/index.tsx (1)
85-93: Verify accessibility compliance of customindicatorcomponentCustomizing the
indicatorprop with icons and buttons is functional, but it's important to ensure that these custom elements meet accessibility guidelines. Verify that keyboard navigation, focus management, and ARIA attributes are appropriately handled.
| .log-level-select-render-value-box-label { | ||
| background-color: initial !important; | ||
| } |
There was a problem hiding this comment.
Consider avoiding the use of !important.
While the class name is specific and follows a consistent naming convention, the use of !important is generally discouraged as it can lead to specificity issues and make the CSS harder to maintain.
Consider increasing the specificity of the selector instead of using !important. For example:
-.log-level-select-render-value-box-label {
- background-color: initial !important;
-}
+.log-level-select-render-value-box .log-level-select-render-value-box-label {
+ background-color: initial;
+}This approach increases the specificity of the selector, potentially eliminating the need for !important. If this doesn't resolve the issue, consider reviewing the conflicting styles and adjusting the overall CSS structure.
📝 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.
| .log-level-select-render-value-box-label { | |
| background-color: initial !important; | |
| } | |
| .log-level-select-render-value-box .log-level-select-render-value-box-label { | |
| background-color: initial; | |
| } |
There was a problem hiding this comment.
The !important usage is to override the styles from JoyUI and therefore cannot be easily removed.
There was a problem hiding this comment.
@junhaoliao, understood. If !important is necessary to override the styles from JoyUI, then it's appropriate in this case.
／(=´ x `=)＼
✏️ Learnings added
Learnt from: junhaoliao
PR: y-scope/yscope-log-viewer#77
File: new-log-viewer/src/components/StatusBar/LogLevelSelect/index.css:6-8
Timestamp: 2024-09-26T08:04:40.476Z
Learning: In this codebase, using `!important` is acceptable when overriding styles from JoyUI components.
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-log-viewer/src/components/StatusBar/LogLevelSelect/LogLevelChip.css
Outdated
Show resolved
Hide resolved
new-log-viewer/src/components/StatusBar/LogLevelSelect/LogLevelChip.css
Outdated
Show resolved
Hide resolved
| interface LogLevelChipProps { | ||
| name: string, | ||
| value: LOG_LEVEL, | ||
|
|
||
| onSelectedLogLevelsChange: (func: (value: LOG_LEVEL[]) => LOG_LEVEL[]) => void, | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider simplifying the callback function type.
The props interface looks good overall, but the callback function type could be simplified for better readability.
Instead of:
onSelectedLogLevelsChange: (func: (value: LOG_LEVEL[]) => LOG_LEVEL[]) => void,Consider using:
onSelectedLogLevelsChange: (updater: (currentLevels: LOG_LEVEL[]) => LOG_LEVEL[]) => void,This change makes it clearer that the function is an updater function that takes the current levels and returns the new levels. It also improves readability without losing any type information.
There was a problem hiding this comment.
sg. Renamed as setter.
There was a problem hiding this comment.
@junhaoliao, glad to hear that you've renamed the parameter. Thank you!
(_/)
( ^_^ )
(")(")
🧠 Learnings used
Learnt from: junhaoliao
PR: y-scope/yscope-log-viewer#77
File: new-log-viewer/src/components/StatusBar/LogLevelSelect/LogLevelChip.css:6-6
Timestamp: 2024-09-26T08:05:11.421Z
Learning: When overriding JoyUI's default styling, it's acceptable to use `!important` in CSS in this project.
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
| {/* Add a dummy MenuItem to avoid the first Option receiving focus. */} | ||
| <MenuItem sx={{display: "none"}}/> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace hidden MenuItem with proper focus management
Using a hidden MenuItem to prevent the first Option from receiving focus is a workaround that might not be the most effective solution. Consider utilizing the listboxProps property or adjusting the Select component's focus settings to manage the focus behaviour more elegantly.
There was a problem hiding this comment.
This is a JoyUI specific bug and the MenuItem was added as a mitigation.
There was a problem hiding this comment.
@junhaoliao, understood. Thank you for the clarification regarding the JoyUI bug and the use of the MenuItem as a mitigation.
(*·ω·)つ☕
✏️ Learnings added
Learnt from: junhaoliao
PR: y-scope/yscope-log-viewer#77
File: new-log-viewer/src/components/StatusBar/LogLevelSelect/index.tsx:0-0
Timestamp: 2024-09-26T08:09:26.714Z
Learning: In JoyUI, a hidden `MenuItem` may be added to prevent the first `Option` from receiving focus due to a specific bug. This is an acceptable mitigation.
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-log-viewer/src/components/StatusBar/LogLevelSelect/index.tsx
Outdated
Show resolved
Hide resolved
| <Option | ||
| key={logLevelName} | ||
| value={index} | ||
| > | ||
| {logLevelName} | ||
| </Option> | ||
| ))} |
There was a problem hiding this comment.
Ensure Option values correspond to LOG_LEVEL constants
The Option components currently use the index as the value, assuming that the indices align with the LOG_LEVEL enum values. This could lead to issues if the order of LOG_LEVEL_NAMES changes. To prevent potential mismatches, consider mapping over the LOG_LEVEL constants directly.
Here's how you might adjust the code:
-{LOG_LEVEL_NAMES.map((logLevelName, index) => (
+{Object.values(LOG_LEVEL).map((logLevel) => (
<Option
- key={logLevelName}
- value={index}
+ key={logLevel}
+ value={logLevel}
>
- {logLevelName}
+ {LOG_LEVEL_NAMES[logLevel]}
</Option>
))}📝 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.
| <Option | |
| key={logLevelName} | |
| value={index} | |
| > | |
| {logLevelName} | |
| </Option> | |
| ))} | |
| {Object.values(LOG_LEVEL).map((logLevel) => ( | |
| <Option | |
| key={logLevel} | |
| value={logLevel} | |
| > | |
| {LOG_LEVEL_NAMES[logLevel]} | |
| </Option> | |
| ))} |
There was a problem hiding this comment.
Object.values(LOG_LEVEL) returns not only the values in enum LOG_LEVEL but also the key names, which means we will need to perform extra filtering here.
Also, LOG_LEVEL is a numerical enum with values starting from 0 and therefore index is safe to be used.
There was a problem hiding this comment.
@junhaoliao, thank you for the clarification. I understand now.
(・ω・)?
✏️ Learnings added
Learnt from: junhaoliao
PR: y-scope/yscope-log-viewer#77
File: new-log-viewer/src/components/StatusBar/LogLevelSelect/index.tsx:99-105
Timestamp: 2024-09-26T08:12:43.984Z
Learning: `LOG_LEVEL` is a numerical enum with values starting from 0, so using `index` in `LOG_LEVEL_NAMES.map((logLevelName, index) => ...)` is safe.
Learnt from: junhaoliao
PR: y-scope/yscope-log-viewer#77
File: new-log-viewer/src/components/StatusBar/LogLevelSelect/index.tsx:99-105
Timestamp: 2024-09-26T08:12:43.984Z
Learning: Using `Object.values(LOG_LEVEL)` returns both the enum's numeric values and key names, so extra filtering is needed to obtain only the numeric values.
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.
🧠 Learnings used
Learnt from: junhaoliao
PR: y-scope/yscope-log-viewer#77
File: new-log-viewer/src/components/StatusBar/LogLevelSelect/index.tsx:0-0
Timestamp: 2024-09-26T08:09:26.853Z
Learning: In JoyUI, a hidden `MenuItem` may be added to prevent the first `Option` from receiving focus due to a specific bug. This is an acceptable mitigation.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…nd above values on clicks.
… "Click to deselect" prompt to the tooltip.
new-log-viewer/tsconfig.json
Outdated
| /* Set the JavaScript language version for emitted JavaScript and include compatible library declarations. */ | ||
| /* Specify a set of bundled library declaration files that describe the target runtime environment. */ | ||
| "lib": [ | ||
| "ES2022", | ||
| "ES2023", |
There was a problem hiding this comment.
To use Array.toReversed().
|
Although this is nicer, I think the behaviour still needs some refinement to be more intuitive.
|
…button; add docs.
… to handle tooltips with left and right placements respectively.
|
Very cool. Can we change a few more things (changed my mind on a few things after trying it)?
|
Obviously if we go with this, we should change the tooltip to "LEVEL and below" |
Right, it was to align the icons since different log levels have different number of characters in their names. On the other hand, the tooltips are not displayed all at the same time so it's less obvious if the icons have shifted a bit.
Sure, let's change this to "LEVEL and below" then.
sg |
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (2)
new-log-viewer/src/components/StatusBar/LogLevelSelect/index.tsx (1)
1-38: Consider sorting imports for improved readabilityThe import statements are well-organized, but sorting them could further enhance readability and maintainability. Consider using an autofix tool to sort these imports automatically.
🧰 Tools
🪛 GitHub Check: lint-check
[warning] 1-1:
Run autofix to sort these imports!new-log-viewer/src/services/decoders/ClpIrDecoder.ts (1)
43-43: Add a blank line before the return statement for readabilityInserting a blank line before the
returnstatement improves code readability.🧰 Tools
🪛 GitHub Check: lint-check
[warning] 43-43:
Expected blank line before this statement
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
new-log-viewer/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (3)
- new-log-viewer/package.json (1 hunks)
- new-log-viewer/src/components/StatusBar/LogLevelSelect/index.tsx (1 hunks)
- new-log-viewer/src/services/decoders/ClpIrDecoder.ts (2 hunks)
🧰 Additional context used
📓 Learnings (2)
📓 Common learnings
Learnt from: junhaoliao PR: y-scope/yscope-log-viewer#77 File: new-log-viewer/src/components/StatusBar/LogLevelSelect/index.tsx:99-105 Timestamp: 2024-10-08T15:52:50.753Z Learning: `LOG_LEVEL` is a numerical enum with values starting from 0, so using `index` in `LOG_LEVEL_NAMES.map((logLevelName, index) => ...)` is safe.Learnt from: junhaoliao PR: y-scope/yscope-log-viewer#77 File: new-log-viewer/src/components/StatusBar/LogLevelSelect/index.tsx:99-105 Timestamp: 2024-09-26T08:12:44.589Z Learning: `LOG_LEVEL` is a numerical enum with values starting from 0, so using `index` in `LOG_LEVEL_NAMES.map((logLevelName, index) => ...)` is safe.Learnt from: junhaoliao PR: y-scope/yscope-log-viewer#77 File: new-log-viewer/src/components/StatusBar/LogLevelSelect/index.tsx:99-105 Timestamp: 2024-10-08T15:52:50.753Z Learning: Using `Object.values(LOG_LEVEL)` returns both the enum's numeric values and key names, so extra filtering is needed to obtain only the numeric values.Learnt from: junhaoliao PR: y-scope/yscope-log-viewer#77 File: new-log-viewer/src/components/StatusBar/LogLevelSelect/index.tsx:99-105 Timestamp: 2024-09-26T08:12:44.589Z Learning: Using `Object.values(LOG_LEVEL)` returns both the enum's numeric values and key names, so extra filtering is needed to obtain only the numeric values.new-log-viewer/src/components/StatusBar/LogLevelSelect/index.tsx (6)
Learnt from: junhaoliao PR: y-scope/yscope-log-viewer#77 File: new-log-viewer/src/components/StatusBar/LogLevelSelect/index.tsx:0-0 Timestamp: 2024-09-26T08:09:26.853Z Learning: In JoyUI, a hidden `MenuItem` may be added to prevent the first `Option` from receiving focus due to a specific bug. This is an acceptable mitigation.Learnt from: junhaoliao PR: y-scope/yscope-log-viewer#77 File: new-log-viewer/src/components/StatusBar/LogLevelSelect/index.tsx:0-0 Timestamp: 2024-10-08T15:52:50.753Z Learning: In JoyUI, a hidden `MenuItem` may be added to prevent the first `Option` from receiving focus due to a specific bug. This is an acceptable mitigation.Learnt from: junhaoliao PR: y-scope/yscope-log-viewer#77 File: new-log-viewer/src/components/StatusBar/LogLevelSelect/index.tsx:99-105 Timestamp: 2024-10-08T15:52:50.753Z Learning: `LOG_LEVEL` is a numerical enum with values starting from 0, so using `index` in `LOG_LEVEL_NAMES.map((logLevelName, index) => ...)` is safe.Learnt from: junhaoliao PR: y-scope/yscope-log-viewer#77 File: new-log-viewer/src/components/StatusBar/LogLevelSelect/index.tsx:99-105 Timestamp: 2024-09-26T08:12:44.589Z Learning: `LOG_LEVEL` is a numerical enum with values starting from 0, so using `index` in `LOG_LEVEL_NAMES.map((logLevelName, index) => ...)` is safe.Learnt from: junhaoliao PR: y-scope/yscope-log-viewer#77 File: new-log-viewer/src/components/StatusBar/LogLevelSelect/index.tsx:99-105 Timestamp: 2024-10-08T15:52:50.753Z Learning: Using `Object.values(LOG_LEVEL)` returns both the enum's numeric values and key names, so extra filtering is needed to obtain only the numeric values.Learnt from: junhaoliao PR: y-scope/yscope-log-viewer#77 File: new-log-viewer/src/components/StatusBar/LogLevelSelect/index.tsx:99-105 Timestamp: 2024-09-26T08:12:44.589Z Learning: Using `Object.values(LOG_LEVEL)` returns both the enum's numeric values and key names, so extra filtering is needed to obtain only the numeric values.
🪛 GitHub Check: lint-check
new-log-viewer/src/components/StatusBar/LogLevelSelect/index.tsx
[warning] 1-1:
Run autofix to sort these imports!
[failure] 198-198:
Expected newline between test and consequent of ternary expression
[failure] 198-198:
Expected literal to be on the left side of ===
[failure] 198-198:
Expected newline between consequent and alternate of ternary expression
[failure] 198-198:
Missing semicolon
[warning] 199-199:
React Hook useEffect has a missing dependency: 'setLogLevelFilter'. Either include it or remove the dependency arraynew-log-viewer/src/services/decoders/ClpIrDecoder.ts
[failure] 42-42:
Missing semicolon
[warning] 43-43:
Expected blank line before this statement
🔇 Additional comments (6)
new-log-viewer/package.json (1)
32-32: Verify compatibility with the updated clp-ffi-js versionThe clp-ffi-js dependency has been updated from ^0.1.0 to ^0.2.0. This change suggests a minor version update, which typically includes new features or improvements while maintaining backwards compatibility.
Please ensure that:
- The new version is compatible with your project's requirements.
- Any new features or changes in clp-ffi-js v0.2.0 have been accounted for in your codebase.
- The project's test suite passes with this new version.
Additionally, consider updating the changelog to reflect this dependency update.
To verify the impact of this change, you can run the following script:
new-log-viewer/src/components/StatusBar/LogLevelSelect/index.tsx (2)
117-138: LGTM: ClearFiltersOption implementationThe ClearFiltersOption component is well-implemented. It uses appropriate TypeScript typing, follows React best practices, and correctly utilizes the INVALID_LOG_LEVEL_VALUE for its value prop. Good job!
1-247: Overall implementation review: LogLevelSelectThe LogLevelSelect component and its subcomponents are well-implemented and align with the PR objectives. The code is structured logically, uses appropriate React hooks and MUI Joy components, and follows TypeScript best practices.
Key strengths:
- Clear separation of concerns with subcomponents (LogSelectOption, ClearFiltersOption).
- Proper use of React hooks for state management and side effects.
- Thoughtful UI design with tooltips and clear visual feedback.
Areas for improvement (addressed in previous comments):
- Performance optimizations (memoization of components and options).
- Minor accessibility enhancements.
- Dependency array fix in useEffect.
Overall, this is a solid implementation that fulfills the requirements of adding a log level selector to the StatusBar. With the suggested improvements, it will be even more robust and performant.
🧰 Tools
🪛 GitHub Check: lint-check
[warning] 1-1:
Run autofix to sort these imports!
[failure] 198-198:
Expected newline between test and consequent of ternary expression
[failure] 198-198:
Expected literal to be on the left side of ===
[failure] 198-198:
Expected newline between consequent and alternate of ternary expression
[failure] 198-198:
Missing semicolon
[warning] 199-199:
React Hook useEffect has a missing dependency: 'setLogLevelFilter'. Either include it or remove the dependency arraynew-log-viewer/src/services/decoders/ClpIrDecoder.ts (3)
38-38: Implementation ofgetFilteredLogEventMapis correctThe method now correctly returns the filtered log event map from the stream reader.
49-49: UsingdeserializeStream()simplifies event deserializationReplacing
deserializeRange(0, LOG_EVENT_FILE_END_IDX)withdeserializeStream()improves clarity and ensures all events are deserialized correctly.
63-63: PassinguseFiltertodecodeRangeenables conditional filteringThe method now correctly passes the
useFilterparameter, allowing for filtering during decoding.
| interface LogSelectOptionProps { | ||
| isChecked: boolean, | ||
| logLevelName: string, | ||
| logLevelValue: LOG_LEVEL, | ||
| onCheckboxClick: React.MouseEventHandler | ||
| onOptionClick: React.MouseEventHandler | ||
| } | ||
|
|
||
| /** | ||
| * Renders an <Option/> in the <LogLevelSelect/> for selecting some log level and/or the levels | ||
| * above it. | ||
| * | ||
| * @param props | ||
| * @param props.isChecked | ||
| * @param props.logLevelName | ||
| * @param props.logLevelValue | ||
| * @param props.onCheckboxClick | ||
| * @param props.onOptionClick | ||
| * @return | ||
| */ | ||
| const LogSelectOption = ({ | ||
| isChecked, | ||
| logLevelName, | ||
| logLevelValue, | ||
| onCheckboxClick, | ||
| onOptionClick, | ||
| }: LogSelectOptionProps) => { | ||
| return ( | ||
| <Option | ||
| data-value={logLevelValue} | ||
| key={logLevelName} | ||
| value={logLevelValue} | ||
| onClick={onOptionClick} | ||
| > | ||
| <ListItemDecorator> | ||
| <Tooltip | ||
| placement={"left"} | ||
| title={ | ||
| <Stack | ||
| alignItems={"center"} | ||
| direction={"row"} | ||
| > | ||
| {isChecked ? | ||
| <RemoveIcon/> : | ||
| <AddIcon/>} | ||
| {logLevelName} | ||
| </Stack> | ||
| } | ||
| > | ||
| <Checkbox | ||
| checked={isChecked} | ||
| size={"sm"} | ||
| value={logLevelValue} | ||
| onClick={onCheckboxClick}/> | ||
| </Tooltip> | ||
| </ListItemDecorator> | ||
| <Tooltip | ||
| placement={"left"} | ||
| title={ | ||
| <Stack | ||
| alignItems={"center"} | ||
| direction={"row"} | ||
| > | ||
| {logLevelName} | ||
| {" and below"} | ||
| </Stack> | ||
| } | ||
| > | ||
| <ListItemContent> | ||
| {logLevelName} | ||
| </ListItemContent> | ||
| </Tooltip> | ||
| </Option> | ||
| ); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance accessibility and optimize performance of LogSelectOption
The LogSelectOption component is well-structured, but consider the following improvements:
- Accessibility: Add an
aria-labelto the Checkbox for screen readers. - Performance: Memoize the component using React.memo to prevent unnecessary re-renders.
Here's a suggested implementation:
import React, { memo } from 'react';
const LogSelectOption = memo(({
isChecked,
logLevelName,
logLevelValue,
onCheckboxClick,
onOptionClick,
}: LogSelectOptionProps) => {
return (
<Option
data-value={logLevelValue}
key={logLevelName}
value={logLevelValue}
onClick={onOptionClick}
>
<ListItemDecorator>
<Tooltip
placement="left"
title={
<Stack alignItems="center" direction="row">
{isChecked ? <RemoveIcon /> : <AddIcon />}
{logLevelName}
</Stack>
}
>
<Checkbox
checked={isChecked}
size="sm"
value={logLevelValue}
onClick={onCheckboxClick}
aria-label={`Select ${logLevelName} log level`}
/>
</Tooltip>
</ListItemDecorator>
<Tooltip
placement="left"
title={
<Stack alignItems="center" direction="row">
{logLevelName} and below
</Stack>
}
>
<ListItemContent>{logLevelName}</ListItemContent>
</Tooltip>
</Option>
);
});
LogSelectOption.displayName = 'LogSelectOption';These changes will improve accessibility for screen reader users and potentially boost performance by reducing unnecessary re-renders.
| const handleCheckboxClick = useCallback((ev: React.MouseEvent<HTMLInputElement>) => { | ||
| ev.preventDefault(); | ||
| ev.stopPropagation(); | ||
|
|
||
| const target = ev.target as HTMLInputElement; | ||
| const value = Number(target.value) as LOG_LEVEL; | ||
| let newSelectedLogLevels: LOG_LEVEL[]; | ||
| if (selectedLogLevels.includes(value)) { | ||
| newSelectedLogLevels = selectedLogLevels.filter((logLevel) => logLevel !== value); | ||
| } else { | ||
| newSelectedLogLevels = [ | ||
| ...selectedLogLevels, | ||
| value, | ||
| ]; | ||
| } | ||
| setSelectedLogLevels(newSelectedLogLevels.sort((a, b) => a - b)); | ||
| }, [selectedLogLevels]); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize state updates in handleCheckboxClick
Consider using the functional update form of setSelectedLogLevels to optimize this function:
const handleCheckboxClick = useCallback((ev: React.MouseEvent<HTMLInputElement>) => {
ev.preventDefault();
ev.stopPropagation();
const target = ev.target as HTMLInputElement;
const value = Number(target.value) as LOG_LEVEL;
setSelectedLogLevels(prevSelected => {
const newSelected = prevSelected.includes(value)
? prevSelected.filter(logLevel => logLevel !== value)
: [...prevSelected, value];
return newSelected.sort((a, b) => a - b);
});
}, []);This approach ensures that we're always working with the most up-to-date state and reduces the risk of stale state issues.
| useEffect(() => { | ||
| setLogLevelFilter((selectedLogLevels.length === 0 ? null : selectedLogLevels)) | ||
| }, [selectedLogLevels]); |
There was a problem hiding this comment.
Fix missing dependency in useEffect hook
The useEffect hook is missing the setLogLevelFilter dependency. Add it to the dependency array to ensure the effect runs when this function changes:
useEffect(() => {
setLogLevelFilter(selectedLogLevels.length === 0 ? null : selectedLogLevels);
}, [selectedLogLevels, setLogLevelFilter]);This change addresses the warning from the static analysis tool and ensures that the effect runs when setLogLevelFilter changes.
🧰 Tools
🪛 GitHub Check: lint-check
[failure] 198-198:
Expected newline between test and consequent of ternary expression
[failure] 198-198:
Expected literal to be on the left side of ===
[failure] 198-198:
Expected newline between consequent and alternate of ternary expression
[failure] 198-198:
Missing semicolon
[warning] 199-199:
React Hook useEffect has a missing dependency: 'setLogLevelFilter'. Either include it or remove the dependency array
| return ( | ||
| <Select | ||
| className={"log-level-select"} | ||
| multiple={true} | ||
| renderValue={handleRenderValue} | ||
| size={"sm"} | ||
| value={selectedLogLevels} | ||
| variant={"soft"} | ||
| indicator={0 === selectedLogLevels.length ? | ||
| <KeyboardArrowUpIcon/> : | ||
| <Tooltip title={"Clear filters"}> | ||
| <IconButton | ||
| variant={"plain"} | ||
| onClick={handleSelectClearButtonClick} | ||
| > | ||
| <CloseIcon/> | ||
| </IconButton> | ||
| </Tooltip>} | ||
| placeholder={ | ||
| <Chip className={"log-level-select-render-value-box-label"}> | ||
| Log Level | ||
| </Chip> | ||
| } | ||
| slotProps={{ | ||
| listbox: { | ||
| className: "log-level-select-listbox", | ||
| placement: "top-end", | ||
| }, | ||
| }} | ||
| > | ||
| <ClearFiltersOption onClick={handleSelectClearButtonClick}/> | ||
| {LOG_LEVEL_NAMES.map((logLevelName, logLevelValue) => { | ||
| const checked = selectedLogLevels.includes(logLevelValue); | ||
| return ( | ||
| <LogSelectOption | ||
| isChecked={checked} | ||
| key={logLevelName} | ||
| logLevelName={logLevelName} | ||
| logLevelValue={logLevelValue} | ||
| onCheckboxClick={handleCheckboxClick} | ||
| onOptionClick={handleOptionClick}/> | ||
| ); | ||
| })} | ||
| </Select> | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider performance optimization for Select component
The Select component is re-rendering on every state change. To optimize performance, consider memoizing the options:
const memoizedOptions = useMemo(() => (
LOG_LEVEL_NAMES.map((logLevelName, logLevelValue) => {
const checked = selectedLogLevels.includes(logLevelValue);
return (
<LogSelectOption
isChecked={checked}
key={logLevelName}
logLevelName={logLevelName}
logLevelValue={logLevelValue}
onCheckboxClick={handleCheckboxClick}
onOptionClick={handleOptionClick}
/>
);
})
), [selectedLogLevels, handleCheckboxClick, handleOptionClick]);
// In the return statement
{memoizedOptions}This optimization prevents unnecessary re-renders of the options when the selected log levels haven't changed.
| console.error("Not implemented."); | ||
|
|
||
| return false; | ||
| this.#streamReader.filterLogEvents(logLevelFilter) |
There was a problem hiding this comment.
Missing semicolon at the end of the statement
A semicolon is required at the end of the statement to adhere to coding standards.
Apply this diff to fix the missing semicolon:
-this.#streamReader.filterLogEvents(logLevelFilter)
+this.#streamReader.filterLogEvents(logLevelFilter);📝 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.
| this.#streamReader.filterLogEvents(logLevelFilter) | |
| this.#streamReader.filterLogEvents(logLevelFilter); |
🧰 Tools
🪛 GitHub Check: lint-check
[failure] 42-42:
Missing semicolon
|
Merged main, linked button to state context filter callback, moved to new clp-ffi-js release |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
new-log-viewer/src/services/decoders/ClpIrDecoder.ts (3)
41-43: Implemented log level filtering, consider error handlingThe implementation now correctly applies the log level filter. However, consider the following suggestions:
- Add parameter validation for
logLevelFilter.- Implement error handling in case the filtering operation fails.
- Return a boolean based on the success of the operation rather than always returning
true.Here's a potential implementation:
setLogLevelFilter(logLevelFilter: LogLevelFilter): boolean { if (!logLevelFilter) { console.error('Invalid log level filter'); return false; } try { this.#streamReader.filterLogEvents(logLevelFilter); return true; } catch (error) { console.error('Error applying log level filter:', error); return false; } }
48-50: Improved stream deserialization, consider handling invalid eventsThe use of
this.#streamReader.deserializeStream()is an improvement. However, consider the following suggestions:
- Implement error handling for the
deserializeStreamcall.- Consider if setting
numInvalidEventsto 0 is always accurate. If possible, retrieve the actual count of invalid events from the stream reader.Here's a potential implementation:
build(): LogEventCount { try { const numValidEvents = this.#streamReader.deserializeStream(); const numInvalidEvents = this.#streamReader.getNumInvalidEvents(); // Assuming this method exists return { numValidEvents, numInvalidEvents }; } catch (error) { console.error('Error building log events:', error); return { numValidEvents: 0, numInvalidEvents: 0 }; } }
63-63: Enhanced decoding flexibility, consider error handlingThe addition of the
useFilterparameter enhances the flexibility of thedecodeRangemethod. However, consider implementing error handling to manage potential exceptions during the decoding process.Here's a potential implementation with error handling:
decodeRange( beginIdx: number, endIdx: number, useFilter: boolean ): Nullable<DecodeResultType[]> { try { return this.#streamReader.decodeRange(beginIdx, endIdx, useFilter); } catch (error) { console.error('Error decoding range:', error); return null; } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- new-log-viewer/src/components/StatusBar/LogLevelSelect/index.tsx (1 hunks)
- new-log-viewer/src/services/decoders/ClpIrDecoder.ts (2 hunks)
- new-log-viewer/src/typings/decoders.ts (0 hunks)
💤 Files with no reviewable changes (1)
- new-log-viewer/src/typings/decoders.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- new-log-viewer/src/components/StatusBar/LogLevelSelect/index.tsx
🧰 Additional context used
🔇 Additional comments (1)
new-log-viewer/src/services/decoders/ClpIrDecoder.ts (1)
37-37: LGTM: Simplified method implementationThe change to directly return the result of
this.#streamReader.getFilteredLogEventMap()is a good simplification. It removes any unnecessary processing and aligns well with the method's purpose.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
new-log-viewer/src/contexts/StateContextProvider.tsx (1)
Line range hint
345-354: LGTM with minor suggestionsThe implementation of
setLogLevelFilterusinguseCallbackis a good optimization. However, consider the following improvements:
Use the nullish coalescing operator (??) instead of logical OR (||) for the
eventNum:eventNum: logEventNumRef.current ?? 1Review the empty dependency array of
useCallback. If the function relies on any external values that could change over time, they should be included in the dependency array to prevent stale closures.
davemarco
changed the title
StatusBar.StatusBar.
References
new-log-viewer series: #45 #46 #48 #51 #52 #53 #54 #55 #56 #59 #60 #61 #62 #63 #66 #67 #68 #69 #70 #71 #72 #73 #74 #76
Description
Validation performed
Log Level ^in the Statusbar located in the bottom of the viewport.Observed all options higher than the "ERROR" level were selected.Observed all options higher than the "TRACE" level were selected.Summary by CodeRabbit
Summary by CodeRabbit
New Features
LogLevelSelectcomponent for selecting multiple log levels with a user-friendly dropdown interface.INVALID_LOG_LEVEL_VALUE,LOG_LEVEL_NAMES,LOG_LEVEL_VALUES, andMAX_LOG_LEVEL.Bug Fixes
Chores
clp-ffi-jsdependency for improved library features.