fix: Add guard to handle null decode results; Update log event query to use active event count (fixes #166). #167
Conversation
|
Warning Rate limit exceeded@davemarco has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 23 minutes and 15 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughThe pull request introduces changes to the 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 (
|
![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 (1)
src/services/LogFileManager/index.ts (1)
Line range hint
1-451: File exceeds maximum line limit.The file has 451 lines, exceeding the maximum allowed 450 lines. Consider removing one blank line to meet the limit.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/services/LogFileManager/index.ts(2 hunks)src/services/decoders/ClpIrDecoder.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
src/services/LogFileManager/index.ts (1)
Pattern **/*.{cpp,hpp,java,js,jsx,ts,tsx}: - Prefer false == <expression> rather than !<expression>.
src/services/decoders/ClpIrDecoder.ts (1)
Pattern **/*.{cpp,hpp,java,js,jsx,ts,tsx}: - Prefer false == <expression> rather than !<expression>.
📓 Learnings (1)
src/services/LogFileManager/index.ts (1)
Learnt from: Henry8192
PR: y-scope/yscope-log-viewer#80
File: new-log-viewer/src/services/LogFileManager/index.ts:305-321
Timestamp: 2024-11-10T16:46:58.543Z
Learning: When a function like `#searchChunkAndScheduleNext` requires significant improvements, it's preferred to rewrite the entire function rather than making incremental changes.
🪛 GitHub Actions: lint
src/services/LogFileManager/index.ts
[error] 451-451: File has too many lines (451). Maximum allowed is 450
🪛 GitHub Check: lint-check
src/services/decoders/ClpIrDecoder.ts
[failure] 107-107:
Unnecessary conditional, the types have no overlap
🔇 Additional comments (4)
src/services/decoders/ClpIrDecoder.ts (2)
104-109: LGTM! Proper null handling implementation.The explicit null check is a good defensive programming practice that prevents potential runtime errors from iterating over null results.
🧰 Tools
🪛 GitHub Check: lint-check
[failure] 107-107:
Unnecessary conditional, the types have no overlap
Line range hint
139-139: LGTM! Follows coding guidelines.The boolean comparison using
false === isJsonObject(fields)correctly follows the coding guidelines.🧰 Tools
🪛 GitHub Check: lint-check
[failure] 107-107:
Unnecessary conditional, the types have no overlapsrc/services/LogFileManager/index.ts (2)
378-386: LGTM! Robust handling of active events count.The implementation properly handles both filtered and unfiltered cases, with an appropriate early return when no active events are present.
Line range hint
388-405: LGTM! Improved chunk processing logic.The implementation correctly handles null decode results and accurately calculates progress based on active events.
davemarco
changed the title
| @@ -383,7 +383,17 @@ class LogFileManager { | |||
| // Current task no longer corresponds to the latest query in the LogFileManager. | |||
| return; | |||
| } | |||
| const chunkEndIdx = Math.min(chunkBeginIdx + QUERY_CHUNK_SIZE, this.#numEvents); | |||
|
|
|||
| const filteredLogEventMap = this.#decoder.getFilteredLogEventMap(); | |||
There was a problem hiding this comment.
Let's replace the this.#decoder.getFilteredLogEventMap() call in L401 with this const.
Co-authored-by: Junhao Liao <[email protected]>
Co-authored-by: Junhao Liao <[email protected]>
davemarco
changed the title
How about |
davemarco
changed the title
Description
clp-ffi-js:decode_range will print an error and return null if it is provided invalid args. This PR adds a guard, so if the result is null, the javascript wrapper decodeRange for structuredIR will not attempt to iterate over results, and return early.
Moreover, this was indirectly caused by invalid args being sent to decodeRange when querying. It does not check if there are no log events (like loadPage), and the calculation is based on the total log events, and not the amount of active events. This can lead to decodeRange being called with invalid args, and printing an error and returning null, leading to the exception, and potentially missing results with no exception in other cases.
This PR also fixes the args passed to decode_range when searching.
Validation performed
Tested there is no more exception. Tested that the range is based on active log events.
Summary by CodeRabbit
Summary by CodeRabbit
Release Notes
Bug Fixes
Performance
The updates focus on improving the robustness and precision of log file management and decoding processes.