new-log-viewer: Finish support for filtering by log-level and associated refactoring. #89
Conversation
WalkthroughThe changes in this pull request enhance 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
🧹 Outside diff range and nitpick comments (10)
new-log-viewer/src/typings/worker.ts (1)
104-104: LGTM: numPages property enhances pagination informationThe addition of the
numPagesproperty to thePAGE_DATAresponse inWorkerRespMapenhances the response structure by providing valuable information about the total number of pages. While not directly related to log level filtering, this change improves the overall pagination functionality.Consider updating the documentation or comments for the
WorkerRespMaptype to reflect this new property and its purpose in pagination.new-log-viewer/src/services/MainWorker.ts (1)
96-105: Consider adding a notification for the filter change.To improve the user experience, consider sending a notification to the renderer indicating that the log level filter has been successfully applied. This feedback will reassure users that their filter request has been processed correctly.
You can add a new
WORKER_RESP_CODEfor filter change notifications and send a response with this code after setting the filter. For example:postResp(WORKER_RESP_CODE.FILTER_CHANGED, { logLevelFilter: args.logLevelFilter, });Then, update the renderer to handle this new response code and display an appropriate notification to the user.
new-log-viewer/src/services/LogFileManager/utils.ts (3)
1-2: Ensure consistent import grouping and ordering.The import statements seem to be grouped inconsistently. Consider following a consistent pattern, such as:
- External dependencies
- Internal dependencies
- Type imports
Within each group, sort the imports alphabetically for better readability and maintainability.
Also applies to: 7-10
34-44: Consider renamingmatchingIdxtocursorIdxfor clarity.The name
matchingIdxdoesn't clearly convey that it represents the cursor position. Renaming it tocursorIdxwould make the intent more explicit and improve code readability.
119-140: Consider renaminggetMatchingLogEventNumtogetLogEventNumFromMatchingIdx.The current name
getMatchingLogEventNumdoesn't clearly convey that the function is converting a matching index to a log event number. Renaming it togetLogEventNumFromMatchingIdxwould make the purpose more apparent and improve code readability.new-log-viewer/src/services/LogFileManager/index.ts (4)
135-146: Ensure proper error handling and user feedback for log level filter failures.Consider providing more informative error messages to the user when setting the log level filter fails. Additionally, verify that the UI handles the error gracefully and provides appropriate feedback to the user.
194-194: Update the documentation for theloadPagemethod.The
loadPagemethod's documentation should be updated to reflect the new return value,numPages, and its description.
204-210: Consider adding a null check forfilteredLogEventMap.To avoid potential null pointer exceptions, consider adding a null check for
filteredLogEventMapbefore using it in thedecodeRangemethod.Apply this diff to add a null check:
const filteredLogEventMap = this.#decoder.getFilteredLogEventMap(); -const useFilter: boolean = null !== filteredLogEventMap; +const useFilter: boolean = filteredLogEventMap !== null && filteredLogEventMap !== undefined;
263-265: Update the documentation for the#getCursorDatamethod.The
#getCursorDatamethod's documentation should be updated to reflect the new return values (pageBeginIdx,pageEndIdx, andmatchingIdx) and their descriptions.new-log-viewer/src/contexts/StateContextProvider.tsx (1)
177-194: Consider adding JSDoc comments to document theupdateUrlIfModifiedfunction.The
updateUrlIfModifiedfunction plays a key role in keeping the URL in sync with the current log event number. To improve maintainability and make the code more self-explanatory, consider adding JSDoc comments to describe the function's purpose, parameters, and return value./** * Updates the URL hash parameters with the new log event number if it has changed. * * @param logEventNum The current log event number. * @param newLogEventNum The new log event number. * @returns Whether the log event number has changed. */ const updateUrlIfModified = (logEventNum: number, newLogEventNum: Nullable<number>): boolean => { // ... };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
- new-log-viewer/src/contexts/StateContextProvider.tsx (11 hunks)
- new-log-viewer/src/services/LogFileManager/index.ts (6 hunks)
- new-log-viewer/src/services/LogFileManager/utils.ts (4 hunks)
- new-log-viewer/src/services/MainWorker.ts (2 hunks)
- new-log-viewer/src/typings/worker.ts (5 hunks)
🔇 Additional comments (30)
new-log-viewer/src/typings/worker.ts (4)
3-6: LGTM: Import changes align with new functionalityThe addition of the
LogLevelFilterimport alongsideLOG_LEVELfrom thelogsmodule is consistent with the PR's objective of implementing log level filtering. This change supports the new functionality without introducing any issues.
60-60: LGTM: New SET_FILTER code supports filtering functionalityThe addition of the
SET_FILTERcode to theWORKER_REQ_CODEenum is directly aligned with the PR's objective of implementing log level filtering. This new worker request code is essential for the new filtering functionality.
84-88: LGTM: SET_FILTER request structure well-definedThe addition of the
SET_FILTERentry to theWorkerReqMaptype complements the new enum value and defines a clear structure for the filter-setting request. The inclusion ofcursor,logLevelFilter, and optionaldecoderOptionsprovides the necessary parameters for effective log level filtering.
39-39: LGTM: Nullable eventNum increases flexibilityThe change from
numbertoNullable<number>for theeventNumproperty allows for more flexible cursor handling. This modification is consistent with the PR objectives and doesn't introduce any immediate issues.To ensure proper null handling, please run the following script to check for appropriate null checks:
new-log-viewer/src/services/MainWorker.ts (4)
77-80: Excellent improvement to the user experience!Sending the page data immediately after loading the log file provides faster feedback to the user interface. This change enhances the responsiveness and usability of the application.
91-95: Consistent behaviour and improved user experience.Modifying the
LOAD_PAGEcase to send the page data back to the renderer aligns with the behaviour of theLOAD_FILEcase. This consistency improves the user experience by providing immediate feedback after loading a page.
96-105: New log level filtering functionality implemented correctly.The new
SET_FILTERcase in the switch statement correctly handles setting the log level filter:
- It checks if the
LOG_FILE_MANAGERis initialized and throws an error if not.- It calls the
setLogLevelFiltermethod with the providedlogLevelFilterfrom the request arguments.- It retains the ability to set formatter options if provided in the request arguments.
The code is structured correctly and follows the existing error handling pattern.
96-105: Verify if the cursor position is handled correctly when setting a filter.After setting the log level filter, the code loads the page using the provided
cursorargument. Ensure that this cursor position is appropriate and correctly positions the user at the expected location in the filtered log file.To verify the cursor position handling, you can add logging statements or breakpoints in the
LogFileManager.tsfile to inspect thecursorvalue and compare it with the expected position in the filtered log file. Pay attention to edge cases, such as filtering that results in an empty log file or a cursor position that exceeds the filtered log file's length.new-log-viewer/src/services/LogFileManager/utils.ts (5)
46-74: Excellent work on handling invalid log event numbers!The
getValidLogEventIdxfunction is a great addition to handle cases where the inputlogEventNummight be invalid due to being out of bounds or excluded by the current filter. Returning the nearest valid log event number is a smart approach.
89-97: Verify ifgetEventNumCursorDatahandles all edge cases.Please ensure that
getEventNumCursorDatacorrectly handles the following edge cases:
- When
logEventNumisnull.- When
logEventNumis greater thannumEvents.- When
logEventNumis excluded by the current filter.Consider adding unit tests to cover these scenarios and confirm the expected behaviour.
105-117: LGTM!The
getLastEventCursorDatafunction looks good. It correctly calculates the indexes for the last page and the last log event on that page.
142-160: Great job on calculating the new number of pages!The
getNewNumPagesfunction correctly determines the new page count based on the number of active events (considering filters) and the page size. This will be helpful in updating the UI when filters are applied or removed.
192-193: Ensure all exported functions are used.It appears that
getMatchingLogEventNumandgetNewNumPagesare exported but not used anywhere else in the codebase.Run the following script to verify the usage of these functions:
If these functions are not being used, consider removing them to keep the codebase clean and maintainable. If they are intended for future use, add a comment indicating so.
new-log-viewer/src/services/LogFileManager/index.ts (6)
268-275: Ensure proper handling of empty log events.Verify that the UI and other parts of the codebase handle the case when there are no log events (either due to filters or an empty log file) gracefully, displaying appropriate messages or placeholders.
Run the following script to verify the handling of empty log events:
#!/bin/bash # Description: Verify proper handling of empty log events. # Test 1: Check if the `LogFileManager` class handles empty log events correctly. ast-grep --lang typescript --pattern $'class LogFileManager { $$$ loadPage(cursor: CursorType): { $$$ if (0 === numEvents) { return {pageBeginIdx: 0, pageEndIdx: 0, matchingIdx: null}; } $$$ } $$$ }' # Test 2: Search for empty log event handling in the UI components. rg --type typescript $'(no|empty) (log|event)' new-log-viewer/src/components
235-241: Verify the correctness of the new page calculations.Ensure that the
getMatchingLogEventNumandgetNewNumPagesfunctions correctly calculate the matching log event number and the total number of pages based on the current filters and page size.Run the following script to verify the page calculations:
#!/bin/bash # Description: Verify the correctness of the new page calculations. # Test 1: Check if the `getMatchingLogEventNum` function handles null `matchingIdx` correctly. ast-grep --lang typescript --pattern $'function getMatchingLogEventNum(matchingIdx: Nullable<number>, filteredLogEventMap: Nullable<number[]>): number { $$$ if (null === matchingIdx) { $$$ } $$$ }' # Test 2: Check if the `getNewNumPages` function correctly calculates the number of pages. ast-grep --lang typescript --pattern $'function getNewNumPages(numEvents: number, pageSize: number, filteredLogEventMap: Nullable<number[]>): number { $$$ const numFilteredEvents = filteredLogEventMap?.length ?? numEvents; return Math.ceil(numFilteredEvents / pageSize); $$$ }'
281-291: Verify the correctness of the cursor data retrieval.Ensure that the
getPageNumCursorDataandgetEventNumCursorDatafunctions in the./utilsmodule correctly calculate the cursor data based on the provided arguments, taking into account the filtered log events when applicable.Run the following script to verify the cursor data retrieval:
#!/bin/bash # Description: Verify the correctness of the cursor data retrieval. # Test 1: Check if the `getPageNumCursorData` function correctly calculates the cursor data. ast-grep --lang typescript --pattern $'function getPageNumCursorData(pageNum: number, eventPositionOnPage: number, numEvents: number, pageSize: number): { $$$ const pageBeginIdx = (pageNum - 1) * pageSize; const pageEndIdx = Math.min(pageBeginIdx + pageSize, numEvents); const matchingIdx = pageBeginIdx + eventPositionOnPage; return {pageBeginIdx, pageEndIdx, matchingIdx}; $$$ }' # Test 2: Check if the `getEventNumCursorData` function correctly calculates the cursor data. ast-grep --lang typescript --pattern $'function getEventNumCursorData(eventNum: number, numEvents: number, pageSize: number, filteredLogEventMap: Nullable<number[]>): { $$$ const matchingIdx = filteredLogEventMap ? filteredLogEventMap.indexOf(eventNum) : eventNum; if (-1 === matchingIdx) { throw new Error(`Event number ${eventNum} not found in filtered log events.`); } const pageNum = Math.floor(matchingIdx / pageSize) + 1; return getPageNumCursorData(pageNum, matchingIdx % pageSize, numEvents, pageSize); $$$ }'
199-201: Verify that the#getCursorDatamethod is called with the correct arguments.Ensure that the
cursorargument passed to#getCursorDatais of the correct type (CursorType) and contains the expected properties based on the cursor code.Run the following script to verify the usage of
#getCursorData:
7-7: Verify thatLogLevelFilteris properly imported and used.Ensure that the
LogLevelFiltertype is correctly defined in the../../typings/logsmodule and that it is being used appropriately throughout the codebase.Run the following script to verify the usage of
LogLevelFilter:
22-23: Verify that the new utility functions are properly implemented.Ensure that the
getMatchingLogEventNumandgetNewNumPagesfunctions in the./utilsmodule are correctly implemented and tested.Run the following script to verify the implementation and usage of the utility functions:
new-log-viewer/src/contexts/StateContextProvider.tsx (11)
36-38: LGTM!The newly imported utility functions
findNearestLessThanOrEqualElementandisWithinBoundsfrom"../utils/data"will be helpful for handling log event numbers and page navigation.
39-39: LGTM!Importing the
clamputility function from"../utils/math"is a good addition for ensuring values stay within defined bounds.
80-80: LGTM!Initializing the
setLogLevelFiltermethod to an empty function in the default state is a good practice. It prevents unexpected behavior when the context is accessed before being properly set up.
212-213: LGTM!Initializing the
pageNumandnumPagesstates from the default values ensures a consistent starting point for pagination.
221-222: LGTM!Updating the
numPagesRefandpageNumRefrefs to match the current state values will keep them in sync for accurate pagination handling.
248-249: LGTM!Setting the
pageNumandnumPagesstates when receiving thePAGE_DATAresponse from the worker ensures that the context accurately reflects the current pagination state.
312-320: LGTM!Using
useEffectto synchronize thepageNumRefandnumPagesRefrefs with their respective state values ensures they stay up to date whenever the state changes.
368-384: LGTM!The updated logic for handling log event number changes is more robust and accurate. Using
isWithinBoundsto check if the clamped log event number is within the current page's range andfindNearestLessThanOrEqualElementto find the nearest valid log event number ensures that the URL is updated correctly and prevents unnecessary page loads.
61-61: Verify thesetLogLevelFiltermethod usage across the codebase.Adding the
setLogLevelFiltermethod to theStateContextTypeinterface is a key change that enables log level filtering functionality. However, it's crucial to ensure that this method is properly called and handled in all relevant parts of the codebase.#!/bin/bash # Description: Verify the usage of the `setLogLevelFilter` method. # Test 1: Search for calls to `setLogLevelFilter`. Expect: Relevant and correct usage. rg --type typescript -A 5 $'setLogLevelFilter' # Test 2: Search for the `LogLevelFilter` type usage. Expect: Consistent and correct usage. rg --type typescript -A 5 $'LogLevelFilter'
337-346: Verify the worker request payload for setting the log level filter.The
setLogLevelFiltermethod sends a request to the worker to update the log level filter. Double-check that the payload sent to the worker, specifically thecursorandlogLevelFilterproperties, are correctly structured and contain the expected values.#!/bin/bash # Description: Verify the worker request payload for setting the log level filter. # Test: Search for the `SET_FILTER` request code usage. Expect: Correct payload structure. rg --type typescript -A 10 $'WORKER_REQ_CODE.SET_FILTER'
14-14: Verify that theLogLevelFiltertype is defined correctly.Ensure that the
LogLevelFiltertype imported from"../typings/logs"accurately represents the structure and possible values for log level filtering. This is crucial for type safety and correct usage throughout the code.✅ Verification successful
Verified the
LogLevelFiltertype definition.The
LogLevelFiltertype innew-log-viewer/src/typings/logs.tsis correctly defined asNullable<LOG_LEVEL[]>, ensuring accurate type safety and proper representation for log level filtering.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the `LogLevelFilter` type definition. # Test: Search for the type definition. Expect: A clear and accurate type definition. rg --type typescript -A 5 $'type LogLevelFilter'Length of output: 184
Script:
#!/bin/bash # Description: Verify the `LogLevelFilter` type definition using ast-grep. # Test: Search for the type definition. Expect: A clear and accurate type definition. ast-grep --lang typescript --pattern $'type LogLevelFilter $_ = { $$$ }' .Length of output: 75
Script:
#!/bin/bash # Description: Search for any declarations of `LogLevelFilter` in TypeScript files. # Test: Look for type, interface, or enum declarations involving `LogLevelFilter`. rg --type ts '^(type|interface|enum)\s+LogLevelFilter\b' -A 5Length of output: 407
| const numEvents: number = filteredLogEventMap ? | ||
| filteredLogEventMap.length : | ||
| this.#numEvents; |
There was a problem hiding this comment.
Shall we name this.#numEvents as this.#numTotalEvents?
There was a problem hiding this comment.
I'm okay to do this. shall we rename in front-end as well? In front-end, it is always the total.
There was a problem hiding this comment.
Yeah, sounds like a good idea.
There was a problem hiding this comment.
for now I just changed to activeNumEvents in this function. There are many many instances of numEvents in the code.
Co-authored-by: Junhao Liao <[email protected]>
…rk into integrate
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (8)
new-log-viewer/src/services/LogFileManager/utils.ts (4)
34-44: LGTM: Function refactored for consistency.The
getPageNumCursorDatafunction has been successfully refactored to return zero-based indices, which aligns well with JavaScript/TypeScript array indexing. The implementation is now more straightforward and consistent with other functions in the file.Consider renaming
matchingIdxtomatchingEventIdxfor improved clarity, as it represents the index of a specific event within the page.
46-74: LGTM: New functiongetValidLogEventIdxadded.This new function effectively validates and adjusts log event numbers, handling both filtered and unfiltered cases. The logic is sound and covers edge cases appropriately.
Consider adding a brief comment explaining why
findNearestLessThanOrEqualElementis used, as it might not be immediately obvious to all readers why this specific operation is necessary.
119-141: LGTM: New functiongetMatchingLogEventNumadded with suggestion for improvement.This new function effectively converts the matching index to a log event number, handling both filtered and unfiltered cases. The logic is sound and the use of nullable types is appropriate.
Consider implementing the suggestion from the past review comment to improve readability:
return 1 + ( null !== filteredLogEventMap ? (filteredLogEventMap[matchingIdx] as number) : matchingIdx );This change makes the ternary operation more explicit and easier to understand.
144-161: LGTM: New functiongetNewNumPagesadded with suggestion for improvement.This new function effectively calculates the number of pages, handling both filtered and unfiltered cases. The logic is sound and provides the necessary functionality for pagination.
Please update the JSDoc parameter order to match the function signature:
/** * Gets the new number of pages. * * @param numEvents * @param filteredLogEventMap * @param pageSize * @return Page count. */This will resolve the static analysis warning and improve documentation accuracy.
🧰 Tools
🪛 GitHub Check: lint-check
[warning] 148-148:
Expected @param names to be "numEvents, filteredLogEventMap, pageSize". Got "numEvents, pageSize, filteredLogEventMap"new-log-viewer/src/contexts/StateContextProvider.tsx (4)
177-194: LGTM: New utility function for URL updatesThe
updateUrlIfModifiedfunction is a good addition that encapsulates the logic for updating the URL when the log event number changes. This improves code readability and maintainability.Consider adding a JSDoc comment to clearly document the function's purpose and parameters.
Here's a suggested JSDoc comment:
/** * Updates the URL if the log event number has changed. * @param logEventNum - The current log event number. * @param newLogEventNum - The new log event number to compare against. * @returns {boolean} - True if the URL was updated, false otherwise. */
212-213: LGTM: Improved pagination state managementThe addition of
numPagesandpageNumstate variables, along with their corresponding refs and synchronization effects, improves the management of pagination state. This approach allows for accessing the most up-to-date values in callbacks without triggering unnecessary re-renders.Consider adding comments to explain why both state and refs are used for these values, as it might not be immediately obvious to other developers.
Here's a suggested comment:
// We use both state and refs for numPages and pageNum: // - State triggers re-renders when these values change // - Refs allow access to the latest values in callbacks without causing re-rendersAlso applies to: 221-222, 312-320
337-346: LGTM with suggestion: Implementation of setLogLevelFilterThe
setLogLevelFilterfunction is implemented correctly and aligns with the PR objectives. It properly sends a worker request to update the log level filter.However, consider adding error handling or user feedback for the case when the worker is not initialized. This would improve the robustness of the function.
Here's a suggested improvement:
const setLogLevelFilter = (newLogLevelFilter: LogLevelFilter) => { if (null === mainWorkerRef.current) { console.error("Worker not initialized. Unable to set log level filter."); // Consider adding user feedback here, e.g., showing a notification return; } workerPostReq(mainWorkerRef.current, WORKER_REQ_CODE.SET_FILTER, { cursor: {code: CURSOR_CODE.EVENT_NUM, args: {eventNum: logEventNumRef.current}}, logLevelFilter: newLogLevelFilter, }); };
368-384: LGTM with suggestions: Improved URL updating logicThe new logic for updating the URL based on log event numbers is more robust and handles edge cases better. The use of
isWithinBoundsandfindNearestLessThanOrEqualElementfunctions improves the accuracy of URL updates.However, the complexity of this logic might make it harder to understand and maintain. Consider the following suggestions to improve readability:
- Extract the logic for finding the nearest log event number into a separate function.
- Add comments explaining the purpose of each step in the logic.
- Consider using early returns to reduce nesting and improve readability.
Here's a suggested refactor:
const findNearestLogEventNum = (logEventNums: number[], targetEventNum: number): number => { const nearestIdx = findNearestLessThanOrEqualElement(logEventNums, targetEventNum); return logEventNums[nearestIdx as number] as number; }; // In the useEffect hook: if (isWithinBounds(logEventNumsOnPage, clampedLogEventNum)) { // If the clamped log event number is within the current page, update URL if needed if (updateUrlIfModified(logEventNum, clampedLogEventNum)) { return; } // Find and update to the nearest log event number on the page const nearestLogEventNum = findNearestLogEventNum(logEventNumsOnPage, clampedLogEventNum); updateUrlIfModified(logEventNum, nearestLogEventNum); return; } // If the log event number is not on the current page, load a new page loadPageByCursor(mainWorkerRef.current, { code: CURSOR_CODE.EVENT_NUM, args: {eventNum: logEventNum}, });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- new-log-viewer/src/contexts/StateContextProvider.tsx (11 hunks)
- new-log-viewer/src/services/LogFileManager/utils.ts (4 hunks)
🧰 Additional context used
🪛 GitHub Check: lint-check
new-log-viewer/src/services/LogFileManager/utils.ts
[warning] 148-148:
Expected @param names to be "numEvents, filteredLogEventMap, pageSize". Got "numEvents, pageSize, filteredLogEventMap"
🔇 Additional comments (5)
new-log-viewer/src/services/LogFileManager/utils.ts (4)
1-10: LGTM: Import statements are appropriate.The new imports (FilteredLogEventMap, clampWithinBounds, findNearestLessThanOrEqualElement) are correctly added and align with the changes in the file. All imports appear to be used within the code.
89-97: LGTM: Function updated to support filtered log events.The
getEventNumCursorDatafunction has been successfully updated to work with filtered log events. The addition of thefilteredLogEventMapparameter and the use ofgetValidLogEventIdxensure that the function handles both filtered and unfiltered cases correctly.
112-117: LGTM: Function refactored for consistency.The
getLastEventCursorDatafunction has been successfully refactored to return zero-based indices, maintaining consistency with other functions in the file. The implementation remains correct for finding the last page and event.
193-194: LGTM: Export statements updated correctly.The new functions
getMatchingLogEventNumandgetNewNumPageshave been properly added to the exports, making them available for use in other parts of the application.new-log-viewer/src/contexts/StateContextProvider.tsx (1)
14-14: LGTM: New imports and interface updates for log level filteringThe new imports and interface changes are consistent with the PR objectives for implementing log level filtering. The
LogLevelFilterimport and the addition ofsetLogLevelFilterto theStateContextTypeinterface andSTATE_DEFAULTobject are appropriate for this feature.Also applies to: 36-38, 61-61, 80-80
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (5)
new-log-viewer/src/services/LogFileManager/utils.ts (3)
20-43: LGTM: Function refactored appropriately.The renaming of
getPageBoundariestogetPageNumCursorDataand the updated return type improve clarity and provide more relevant information. The logic for calculating page boundaries and the matching index is correct.Consider renaming
matchingIdxtoeventIdxorselectedEventIdxfor better clarity, as it represents the selected event's index based on theeventPositionOnPage.
47-72: LGTM: New function handles log event validation well.The
getValidLogEventIdxfunction effectively validates and adjusts log event numbers, handling both filtered and unfiltered cases. The logic is sound and covers edge cases appropriately.Consider adding an explicit check for
numEvents > 0at the beginning of the function to prevent potential division by zero or invalid index calculations.
142-160: LGTM: New function calculates page count correctly.The
getNewNumPagesfunction accurately calculates the number of pages based on the total number of events, filtered events, and page size. It handles both filtered and unfiltered cases appropriately.Consider using a ternary operator for a more concise expression:
const numActiveEvents = filteredLogEventMap ? filteredLogEventMap.length : numEvents;new-log-viewer/src/contexts/StateContextProvider.tsx (2)
177-210: LGTM: New utility function for URL updates, with minor documentation improvements neededThe
updateUrlIfEventOnPagefunction is a valuable addition that ensures the URL accurately reflects the current log event number. Its logic appears sound and aligns well with the PR objectives.However, there are some minor issues with the JSDoc comments:
- The
@paramforlogEventNumis missing in the JSDoc declaration.- The capitalization of
LogEventNumin the@paramdescription is inconsistent with the actual parameter name.To address these issues, please update the JSDoc comments as follows:
/** * If the new log event number is within the page boundaries, update the url with the nearest log * event and return true. If the new log event number is outside page boundaries, do not update and * return false. * * @param logEventNum - The log event number to check and potentially update in the URL. * @param logEventNumsOnPage - Array of log event numbers on the current page. * @return Whether the log event number is within page boundaries. */This will resolve the static analysis warnings and improve the documentation clarity.
🧰 Tools
353-362: LGTM: Implementation of log level filtering with a suggestion for error handlingThe
setLogLevelFilterfunction successfully implements the core functionality for applying log level filters, which is a key objective of this PR. It correctly utilizes the worker to perform the filtering, ensuring efficient processing.However, I have a minor suggestion to improve error handling:
const setLogLevelFilter = (newLogLevelFilter: LogLevelFilter) => { if (null === mainWorkerRef.current) { console.error("Unexpected null mainWorkerRef.current"); return; } workerPostReq(mainWorkerRef.current, WORKER_REQ_CODE.SET_FILTER, { cursor: {code: CURSOR_CODE.EVENT_NUM, args: {eventNum: logEventNumRef.current}}, logLevelFilter: newLogLevelFilter, }); };Adding a console.error message when
mainWorkerRef.currentis null would help with debugging if this unexpected state occurs. This addition maintains consistency with error handling patterns used elsewhere in the file.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- new-log-viewer/src/contexts/StateContextProvider.tsx (11 hunks)
- new-log-viewer/src/services/LogFileManager/index.ts (6 hunks)
- new-log-viewer/src/services/LogFileManager/utils.ts (4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- new-log-viewer/src/services/LogFileManager/index.ts
🧰 Additional context used
🔇 Additional comments (11)
new-log-viewer/src/services/LogFileManager/utils.ts (5)
1-10: LGTM: Import statements are appropriate.The new imports (
Nullable,FilteredLogEventMap,findNearestLessThanOrEqualElement) are correctly added and necessary for the new functionality. All imports appear to be used in the file.
Line range hint
76-95: LGTM: Function updated to support filtered log events.The
getEventNumCursorDatafunction has been successfully updated to handle filtered log events. The use ofgetValidLogEventIdxensures proper index calculation, and the logic for determining page boundaries remains correct.
97-115: LGTM: Minor updates maintain consistency.The
getLastEventCursorDatafunction has been updated to use the new return type (pageBeginIdx,pageEndIdx,matchingIdx), maintaining consistency with other functions. The logic for calculating the last event's data remains correct.
117-139: LGTM: New function correctly converts index to event number.The
getMatchingLogEventNumfunction effectively converts a matching index to a log event number, handling both filtered and unfiltered cases. The null check is appropriate, and the logic is sound.The suggested change from the previous review has been implemented, improving type safety in the return statement.
188-195: LGTM: Exports updated correctly.The export statements have been appropriately updated to include the new functions
getMatchingLogEventNumandgetNewNumPages. All necessary utility functions are now being exported for use in other parts of the application.new-log-viewer/src/contexts/StateContextProvider.tsx (6)
14-14: LGTM: Necessary imports and interface updates for log level filteringThe new imports and interface updates are consistent with the PR objectives. The addition of
LogLevelFilterimport and thesetLogLevelFiltermethod to theStateContextTypeinterface provide the foundation for implementing log level filtering functionality.Also applies to: 36-38, 61-61, 80-80
228-229: LGTM: Enhanced state management for paginationThe addition of
numPagesandpageNumstate variables, along with their corresponding refs, improves the component's ability to manage pagination state. The updates to thehandleMainWorkerRespfunction ensure that these state variables are properly updated when new page data is received from the worker.These changes contribute to a more robust pagination system, which is essential for effective log viewing and navigation.
Also applies to: 237-238, 264-265
328-336: LGTM: Ref synchronization for consistent state accessThe addition of these
useEffecthooks is a crucial improvement:useEffect(() => { pageNumRef.current = pageNum; }, [pageNum]); useEffect(() => { numPagesRef.current = numPages; }, [numPages]);These hooks ensure that
pageNumRefandnumPagesRefalways reflect the most current values ofpageNumandnumPages, respectively. This synchronization is particularly important when these refs are used within callback functions or event handlers that might otherwise capture outdated values due to closures.By keeping the refs in sync with the state, you're preventing potential bugs related to stale data and ensuring consistent behaviour throughout the component's lifecycle.
384-385: LGTM: Optimized page loading logicThe updated
useEffecthook for handlinglogEventNumchanges now incorporates the newupdateUrlIfEventOnPagefunction:if (updateUrlIfEventOnPage(clampedLogEventNum, logEventNumsOnPage)) { // Do not request a new page, if the log event can be found on the current page. return; }This enhancement significantly improves the efficiency of the component by:
- Avoiding unnecessary page loads when the target log event is already on the current page.
- Updating the URL to reflect the nearest log event number on the current page.
These optimizations contribute to a smoother user experience and reduced server load, aligning perfectly with the PR's objectives of enhancing the log viewer's functionality and performance.
427-428: LGTM: Updated context value with new pagination and filtering propertiesThe
StateContext.Providervalue prop has been correctly updated to include the new state variables and methods:value={{ // ...existing properties numPages: numPages, pageNum: pageNum, // ...existing methods setLogLevelFilter: setLogLevelFilter, }}These additions ensure that the new pagination state (
numPagesandpageNum) and the log level filtering functionality (setLogLevelFilter) are accessible to all child components consuming this context. This update is consistent with the changes made earlier in the file and is crucial for the new features to be utilized throughout the application.Also applies to: 433-433
Line range hint
1-438: Overall assessment: Successful implementation of log level filtering and pagination improvementsThis PR successfully achieves its main objectives:
- Integration of log level filtering functionality
- Improvements to pagination handling
- Optimization of page loading logic
The changes are well-implemented, with clear additions to the state management, worker communication, and context provision. The new utility function
updateUrlIfEventOnPageenhances URL management, contributing to a better user experience.Minor suggestions have been made for improving error handling and JSDoc comments, but these are small refinements to an otherwise solid implementation.
The code demonstrates good practices in React state management and shows careful consideration of performance optimizations. Great job on this contribution!
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (7)
new-log-viewer/src/services/LogFileManager/utils.ts (2)
56-81: getValidLogEventIdx function is a good additionThis new function effectively handles the validation and adjustment of log event indices, considering both filtered and unfiltered scenarios. The use of utility functions enhances code readability and maintainability.
Consider adding a brief comment explaining why the explicit cast on line 80 is necessary:
// Explicit cast is necessary because TypeScript's type inference doesn't recognize // that the result cannot be null due to the previous checks and clamping. return findNearestLessThanOrEqualElement(filteredLogEventMap, clampedLogEventIdx) as number;
Line range hint
125-178: getEmptyPage function and export updates look goodThe addition of the getEmptyPage function provides a consistent way to create empty page objects, which is beneficial for maintainability and reducing potential errors. The explicit return type definition enhances type safety.
The export list has been appropriately updated to include the new function.
For consistency with the emptyPage constant, consider using object property shorthand in the getEmptyPage function:
return { beginLineNumToLogEventNum: new Map(), cursorLineNum: 1, logEventNum: 0, logs: "", numPages: 1, pageNum: 1 };This makes the function's return value match the structure of the emptyPage constant more closely.
new-log-viewer/src/contexts/StateContextProvider.tsx (3)
14-14: LGTM! Consider adding JSDoc for the new imports.The new imports and the
updateUrlIfEventOnPageutility function are well-implemented and enhance the functionality of the component. The function effectively handles URL updates based on log event numbers within page boundaries.Consider adding JSDoc comments for the newly imported functions (
findNearestLessThanOrEqualElement,isWithinBounds,clamp) to improve code documentation and maintainability.Also applies to: 36-38, 177-210
228-229: LGTM! Consider combining the useEffect hooks.The addition of
numPagesandpageNumstate variables, along with their corresponding refs and synchronization hooks, is well-implemented. These changes provide the necessary state management for pagination.Consider combining the two useEffect hooks for
pageNumRefandnumPagesRefinto a single hook with multiple effects for better performance and readability:useEffect(() => { pageNumRef.current = pageNum; numPagesRef.current = numPages; }, [pageNum, numPages]);Also applies to: 237-238, 328-336
353-362: LGTM! Consider adding error handling.The
setLogLevelFilterfunction is well-implemented and consistent with the PR objectives for log level filtering. It correctly uses theworkerPostReqfunction to communicate with the worker and uses the current log event number from the ref.Consider adding error handling or logging for the case when
mainWorkerRef.currentis null:const setLogLevelFilter = (newLogLevelFilter: LogLevelFilter) => { if (null === mainWorkerRef.current) { console.error("Cannot set log level filter: main worker is not initialized"); return; } workerPostReq(mainWorkerRef.current, WORKER_REQ_CODE.SET_FILTER, { cursor: {code: CURSOR_CODE.EVENT_NUM, args: {eventNum: logEventNumRef.current}}, logLevelFilter: newLogLevelFilter, }); };new-log-viewer/src/services/LogFileManager/index.ts (2)
1-1: Sort imports to improve readabilityStatic analysis suggests sorting these imports to maintain consistency and readability. Please run autofix to organize them accordingly.
🧰 Tools
🪛 GitHub Check: lint-check
[warning] 1-1:
Run autofix to sort these imports!
[failure] 1-1:
'Nullable' is defined but never used
265-274: Clarify parameter documentation in '#getCursorData'Consider updating the JSDoc comment for the
#getCursorDatamethod to specify thatnumEventsrefers to the total number of events after filtering. This clarification will help prevent confusion about whethernumEventsis filtered or unfiltered.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- new-log-viewer/src/contexts/StateContextProvider.tsx (11 hunks)
- new-log-viewer/src/services/LogFileManager/index.ts (6 hunks)
- new-log-viewer/src/services/LogFileManager/utils.ts (3 hunks)
🧰 Additional context used
🪛 GitHub Check: lint-check
new-log-viewer/src/services/LogFileManager/index.ts
[warning] 1-1:
Run autofix to sort these imports!
[failure] 1-1:
'Nullable' is defined but never used
[failure] 22-22:
'getMatchingLogEventNum' is defined but never used
[failure] 23-23:
'getNewNumPages' is defined but never used
🪛 Biome
new-log-viewer/src/services/LogFileManager/utils.ts
[error] 20-20: expected
,but instead found;Remove ;
(parse)
[error] 22-22: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 21-21: Unexpected label.
Only loops should be labeled.
The use of labels for other statements is suspicious and unfamiliar.(lint/suspicious/noConfusingLabels)
🔇 Additional comments (10)
new-log-viewer/src/services/LogFileManager/utils.ts (4)
1-18: Import statements look goodThe new imports align well with the changes described in the PR summary. The addition of FilteredLogEventMap and BeginLineNumToLogEventNumMap types, along with the EVENT_POSITION_ON_PAGE enum, suggests improved type safety for the log level filtering functionality.
29-53: getPageNumCursorData function looks goodThe changes to this function align well with the PR objectives. The shift from log event numbers to indices is consistent and should improve the efficiency of log level filtering. The logic for calculating pageBeginIdx, pageEndIdx, and matchingIdx is correct and straightforward.
89-103: getEventNumCursorData function updates look goodThe changes to this function align well with the PR objectives. The addition of the filteredLogEventMap parameter and the use of getValidLogEventIdx improve the handling of filtered log events. The shift to returning indices instead of log event numbers is consistent with the overall changes in the file.
The use of the nullish coalescing operator (??) on line 100 is a good practice for handling potentially null input.
111-123: getLastEventCursorData function changes are appropriateThe updates to this function are consistent with the overall changes in the file, shifting from log event numbers to indices. The logic for calculating the indices remains correct and efficient. These changes should integrate well with the new log level filtering functionality.
new-log-viewer/src/contexts/StateContextProvider.tsx (4)
61-61: LGTM! StateContextType interface updated correctly.The addition of
setLogLevelFilterto theStateContextTypeinterface and the default state is consistent with the PR objectives. It follows the existing pattern for other methods in the interface, maintaining code consistency.Also applies to: 80-80
264-265: LGTM! handleMainWorkerResp updated correctly.The changes to the
handleMainWorkerRespfunction, specifically in the PAGE_DATA case, are correct and consistent with the new pagination state management. The addition ofsetNumPagesandsetPageNumensures that the pagination state is properly maintained when receiving page data from the worker.
384-385: LGTM! Improved handling of log event number updates.The changes to the useEffect hook for
logEventNumupdates are well-implemented. The new logic, including the use ofupdateUrlIfEventOnPage, ensures that the URL is updated correctly and that page changes are requested only when necessary. This improvement enhances the efficiency and accuracy of the log viewer's navigation.
427-428: LGTM! StateContext.Provider value updated correctly.The changes to the
StateContext.Providervalue are correct and consistent with the updates made to theStateContextTypeinterface. All new state variables (numPages,pageNum) and functions (setLogLevelFilter) are properly included in the context value, ensuring that they are available to consuming components.Also applies to: 433-433
new-log-viewer/src/services/LogFileManager/index.ts (2)
136-147: Method 'setLogLevelFilter' implemented correctlyThe
setLogLevelFiltermethod properly updates the decoder's log level filter with appropriate error handling.
199-207: Correctly handle cases with no matching eventsThe updated logic in
loadPageappropriately checks if the number of events after filtering is zero and returns an empty page accordingly.
| const emptyPage = { | ||
| beginLineNumToLogEventNum: new Map(); | ||
| cursorLineNum: 1, | ||
| logEventNum: matchingLogEventNum, | ||
| logs: messages.join(""), | ||
| umPages: newNumPages, | ||
| pageNum: newPageNum, | ||
| } |
There was a problem hiding this comment.
Fix syntax errors in emptyPage constant
There are several issues in the emptyPage constant definition:
- Line 20: Replace the semicolon with a comma.
- Line 22: The
matchingLogEventNumvariable is undefined. - Line 24: "umPages" should be "numPages".
- The object definition is missing a closing brace.
Here's the corrected version:
const emptyPage = {
beginLineNumToLogEventNum: new Map(),
cursorLineNum: 1,
logEventNum: 0, // Assuming 0 for an empty page
logs: "",
numPages: 1,
pageNum: 1,
};Please review and adjust the values as needed for your use case.
🧰 Tools
🪛 Biome
[error] 20-20: expected
,but instead found;Remove ;
(parse)
[error] 22-22: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 21-21: Unexpected label.
Only loops should be labeled.
The use of labels for other statements is suspicious and unfamiliar.(lint/suspicious/noConfusingLabels)
| @@ -1,8 +1,10 @@ | |||
| import {Nullable} from "../../typings/common"; | |||
There was a problem hiding this comment.
Remove unused import 'Nullable'
The import Nullable from "../../typings/common" is not used in this file and can be safely removed to clean up the code.
🧰 Tools
🪛 GitHub Check: lint-check
[warning] 1-1:
Run autofix to sort these imports!
[failure] 1-1:
'Nullable' is defined but never used
| getMatchingLogEventNum, | ||
| getNewNumPages, |
There was a problem hiding this comment.
Remove unused imports 'getMatchingLogEventNum' and 'getNewNumPages'
The functions getMatchingLogEventNum and getNewNumPages are imported but not used in this file. Please remove these unused imports to avoid confusion and potential issues.
🧰 Tools
🪛 GitHub Check: lint-check
[failure] 22-22:
'getMatchingLogEventNum' is defined but never used
[failure] 23-23:
'getNewNumPages' is defined but never used
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
new-log-viewer/src/contexts/StateContextProvider.tsx (2)
177-213: LGTM: New updateUrlIfEventOnPage function with minor style suggestionThe new
updateUrlIfEventOnPagefunction is well-implemented and handles the task of updating the URL when the log event number is within page boundaries. It makes good use of the newly imported utility functions.However, there's a minor style issue:
Add a blank line before the comment on line 193 to improve readability:
} + // Given `isWithinBounds` returns `true`, then `logEventNumsOnPage` must bound `logEventNum` and // `logEventNumsOnPage` cannot be empty. These assertions justify type casts at end of // function.🧰 Tools
🪛 GitHub Check: lint-check
[failure] 193-193:
Expected line before comment
356-365: LGTM: setLogLevelFilter function with optimization suggestionThe
setLogLevelFilterfunction is well-implemented and correctly sends a SET_FILTER request to the worker with the new log level filter.However, for optimization:
Consider memoizing this function with
useCallbackto prevent unnecessary re-renders:const setLogLevelFilter = useCallback((newLogLevelFilter: LogLevelFilter) => { if (null === mainWorkerRef.current) { return; } workerPostReq(mainWorkerRef.current, WORKER_REQ_CODE.SET_FILTER, { cursor: {code: CURSOR_CODE.EVENT_NUM, args: {eventNum: logEventNumRef.current}}, logLevelFilter: newLogLevelFilter, }); }, []);This change will ensure that the function reference remains stable across re-renders, potentially improving performance.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- new-log-viewer/src/contexts/StateContextProvider.tsx (11 hunks)
🧰 Additional context used
🪛 GitHub Check: lint-check
new-log-viewer/src/contexts/StateContextProvider.tsx
[failure] 193-193:
Expected line before comment
🔇 Additional comments (5)
new-log-viewer/src/contexts/StateContextProvider.tsx (5)
14-14: LGTM: Imports and interface updates for log level filteringThe new imports and interface changes are consistent with the addition of log level filtering functionality. The
LogLevelFiltertype is imported, utility functions are added, and theStateContextTypeinterface is updated accordingly. These changes lay the groundwork for implementing the log level filtering feature.Also applies to: 36-38, 61-61, 80-80
231-232: LGTM: State and ref additions for paginationThe new state variables
numPagesandpageNum, along with their corresponding refsnumPagesRefandpageNumRef, are well-integrated into the component. ThehandleMainWorkerRespfunction is appropriately updated to set these new state variables when receiving page data. These changes enhance the component's ability to manage pagination state effectively.Also applies to: 240-241, 267-268
331-339: LGTM: Ref synchronization with useEffectThe addition of these useEffect hooks is a good practice. They ensure that
pageNumRefandnumPagesRefare always synchronized with their corresponding state variablespageNumandnumPages. This prevents potential issues with stale values in callbacks that depend on these refs.
387-388: LGTM: Efficient handling of logEventNum updatesThe changes to this useEffect hook improve efficiency by using the new
updateUrlIfEventOnPagefunction. This prevents unnecessary page requests when the log event is already on the current page, which is a good optimization. The implementation is consistent with the newly added functionality and should improve the overall performance of the log viewer.
430-431: LGTM: Context value updatesThe additions to the StateContext.Provider value prop are consistent with the new state variables and functions implemented in the component. Including
numPages,pageNum, andsetLogLevelFilterin the context value makes these new pieces of state and functionality available to consumers of the context, which is essential for the proper functioning of the log level filtering and pagination features.Also applies to: 436-436
|
Made changes to naming of "active log events collections". Hopefully more clear |
Co-authored-by: kirkrodrigues <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (10)
new-log-viewer/src/typings/decoders.ts (1)
42-50: LGTM! Consider enhancing the documentation further.The new type
ActiveLogCollectionEventIdxis well-defined and serves its purpose. The documentation provides a clear explanation of its usage and context.However, to fully address the previous review comment and improve clarity, consider implementing the following suggestion:
/** * Index into the active log events collection. The active log events collection is either: - * - the filtered log events collection, if the log level filter is set; or - * - the unfiltered log events collection. + * - the filtered log events collection, if the log level filter is set; or + * - the unfiltered log events collection. * * NOTE: The filtered log events collection is currently represented using a `FilteredLogEventMap` * (so the index goes through a layer of indirection). */This change improves readability by using consistent formatting for the list items.
new-log-viewer/src/typings/worker.ts (1)
51-60: Consider renaming the 'matching' property for clarity.The
CursorDatatype is a good addition for enhancing cursor-related operations. However, the 'matching' property could be more descriptive.Consider renaming 'matching' to 'matchingEvent' or 'cursorMatchingEvent' to more clearly indicate its purpose:
type CursorData = { pageBegin: ActiveLogCollectionEventIdx; pageEnd: ActiveLogCollectionEventIdx; - matching: ActiveLogCollectionEventIdx; + matchingEvent: ActiveLogCollectionEventIdx; };new-log-viewer/src/services/LogFileManager/utils.ts (5)
23-45: LGTM! Function updated to support filtered log events.The changes to
getPageNumCursorDatalook good. The function now correctly calculates indices for filtered log events and returns aCursorDataobject.Consider updating the JSDoc to reflect the new parameter
numActiveEventsand the return typeCursorData. For example:/** * Gets the data for the `PAGE_NUM` cursor. * * @param pageNum - The page number * @param eventPositionOnPage - The position of the event on the page * @param numActiveEvents - The number of active events * @param pageSize - The size of each page * @return {CursorData} - The cursor data containing pageBegin, pageEnd, and matching indices */
49-77: LGTM! New function handles both filtered and unfiltered collections effectively.The
getActiveLogCollectionEventIdxfunction is well-implemented and correctly handles both filtered and unfiltered log collections. The use ofclampWithinBoundsandfindNearestLessThanOrEqualElementis appropriate for the filtered case.Consider adding a check for an empty
filteredLogEventMapto improve error handling. For example:if (filteredLogEventMap !== null && filteredLogEventMap.length === 0) { throw new Error("filteredLogEventMap cannot be empty"); }This would prevent potential issues if an empty filtered map is passed to the function.
80-102: LGTM! Function updated to support filtered log events.The changes to
getEventNumCursorDatalook good. The function now correctly usesgetActiveLogCollectionEventIdxto determine the matching index and returns aCursorDataobject with the appropriate indices.Consider updating the JSDoc to reflect the new parameters and return type. For example:
/** * Gets the data for the `EVENT_NUM` cursor. * * @param logEventNum - The log event number * @param numActiveEvents - The number of active events * @param pageSize - The size of each page * @param filteredLogEventMap - The map of filtered log events * @return {CursorData} - The cursor data containing pageBegin, pageEnd, and matching indices */
105-120: LGTM! Function updated to use indices instead of log event numbers.The changes to
getLastEventCursorDatalook good. The function now correctly calculates indices and returns aCursorDataobject, which is consistent with the new approach.Consider updating the JSDoc to reflect the new parameter and return type. For example:
/** * Gets the data for the `LAST` cursor. * * @param numActiveEvents - The number of active events * @param pageSize - The size of each page * @return {CursorData} - The cursor data containing pageBegin, pageEnd, and matching indices */
Line range hint
1-120: Overall, excellent improvements to support log level filtering.The changes in this file significantly enhance the log viewer's functionality by introducing support for filtered log events. The new
getActiveLogCollectionEventIdxfunction and the updates to existing functions (getPageNumCursorData,getEventNumCursorData, andgetLastEventCursorData) work together to provide a consistent approach to handling both filtered and unfiltered log collections.The code is well-structured, and the changes maintain consistency across functions. The use of
ActiveLogCollectionEventIdxandCursorDatatypes improves type safety and code readability.These changes align well with the PR objectives of integrating log level filtering into the jsonl decoder. The next step would be to connect these utility functions to the log level filter button in the UI, as mentioned in the PR description.
Consider creating unit tests for these utility functions to ensure their correctness and to facilitate future maintenance and refactoring.
new-log-viewer/src/contexts/StateContextProvider.tsx (2)
14-14: LGTM! New imports and utility function enhance log filtering capabilities.The new imports and the
updateUrlIfEventOnPagefunction are well-structured and support the log level filtering functionality. The function effectively updates the URL based on the current log event number.Consider adding a brief comment explaining the purpose of the
nearestIdxcalculation in theupdateUrlIfEventOnPagefunction to improve code readability.Also applies to: 36-38, 176-212
230-231: LGTM! New state variables and refs enhance pagination functionality.The addition of
numPagesandpageNumstate variables, along with their corresponding refsnumPagesRefandpageNumRef, is consistent with the improved pagination functionality. The use of refs allows for accessing the most up-to-date values in callbacks and effects without triggering unnecessary re-renders.For consistency, consider using the same naming convention for all refs. For example, you could rename
logEventNumReftologEventNumRefto match the pattern used fornumPagesRefandpageNumRef.Also applies to: 239-240
new-log-viewer/src/services/LogFileManager/index.ts (1)
212-214: Simplify error message formatting using template literalsFor better readability and consistency, use a single template literal to format the error message.
Apply this diff:
- throw new Error("Error occurred during decoding. " + - `pageBegin=${pageBegin}, ` + - `pageEnd=${pageEnd}`); + throw new Error(`Error occurred during decoding. pageBegin=${pageBegin}, pageEnd=${pageEnd}`);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
- new-log-viewer/src/contexts/StateContextProvider.tsx (11 hunks)
- new-log-viewer/src/services/LogFileManager/index.ts (5 hunks)
- new-log-viewer/src/services/LogFileManager/utils.ts (2 hunks)
- new-log-viewer/src/services/MainWorker.ts (1 hunks)
- new-log-viewer/src/typings/decoders.ts (2 hunks)
- new-log-viewer/src/typings/worker.ts (7 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- new-log-viewer/src/services/MainWorker.ts
🧰 Additional context used
🔇 Additional comments (16)
new-log-viewer/src/typings/decoders.ts (1)
115-115: LGTM! New type correctly exported.The
ActiveLogCollectionEventIdxtype is properly included in the export statement, ensuring it's available for use in other modules.new-log-viewer/src/typings/worker.ts (6)
1-9: LGTM: Import statements updated appropriately.The new imports align with the changes made in the file, introducing necessary types for enhanced type safety and log level filtering functionality.
74-74: LGTM: New SET_FILTER enum value added.The addition of the SET_FILTER enum value to WORKER_REQ_CODE is appropriate for supporting the new log level filtering functionality.
139-149: LGTM: EMPTY_PAGE_RESP constant added.The addition of the EMPTY_PAGE_RESP constant is a good practice:
- It provides a consistent structure for empty page responses.
- The use of Object.freeze() prevents accidental modifications.
- The default values are appropriate for an empty page scenario.
This constant will help maintain consistency across the codebase when dealing with empty page responses.
Line range hint
151-168: LGTM: Export statements updated appropriately.The changes to the export statements are correct and consistent with the new additions to the file:
- EMPTY_PAGE_RESP is now exported, making it available for use in other parts of the application.
- CursorData type is added to the type exports, allowing its use in other files.
These exports will enable the proper use of the new types and constants throughout the codebase.
112-114: LGTM: WorkerRespMap updated with nullable logEventNum and new numPages property.The changes enhance the flexibility of the response structure:
- Changing logEventNum to Nullable allows for cases where no log event number is available.
- The addition of numPages supports pagination functionality.
To ensure proper null handling for logEventNum, run:
#!/bin/bash # Search for usage of logEventNum to verify null checks rg --type typescript 'logEventNum' -A 5 -B 5Please review the results and ensure that null checks are implemented where necessary.
Line range hint
85-97: LGTM: WorkerReqMap updated appropriately.The changes to WorkerReqMap look good:
- Removal of decoderOptions from LOAD_PAGE aligns with previous discussions.
- Addition of SET_FILTER request type with cursor and logLevelFilter properties is appropriate for the new filtering functionality.
To ensure decoderOptions is not used elsewhere inappropriately, run:
✅ Verification successful
LGTM: Removal of
decoderOptionsfromLOAD_PAGEconfirmed.The verification confirms that
decoderOptionsis no longer used inLOAD_PAGErequests outside ofworker.ts. The changes are appropriate and do not introduce any issues.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining usage of decoderOptions in LOAD_PAGE requests rg --type typescript 'LOAD_PAGE.*decoderOptions' -g '!worker.ts'Length of output: 173
Script:
#!/bin/bash # Search for any remaining usage of decoderOptions in LOAD_PAGE requests excluding worker.ts rg --type ts 'LOAD_PAGE.*decoderOptions' --glob '!worker.ts'Length of output: 62
new-log-viewer/src/services/LogFileManager/utils.ts (1)
2-13: LGTM! Import statements updated appropriately.The new import statements are consistent with the changes made in the file and support the new functionality for filtered log events and cursor data.
new-log-viewer/src/contexts/StateContextProvider.tsx (5)
61-61: LGTM! StateContextType and STATE_DEFAULT updates align with new features.The addition of
setLogLevelFiltertoStateContextTypeandSTATE_DEFAULT, along with the new state variablesnumPagesandpageNum, are consistent with the introduction of log level filtering and improved pagination. The default implementation ofsetLogLevelFilteras a no-op function is a good practice for optional methods in the context.Also applies to: 80-80, 230-231
266-267: LGTM! handleMainWorkerResp now updates pagination state.The addition of
setNumPagesandsetPageNumcalls in theWORKER_RESP_CODE.PAGE_DATAcase ensures that the component's state is updated with the latest pagination information received from the worker. This change is consistent with the improved pagination functionality.
361-369: LGTM! New useEffect hooks ensure ref-state synchronization.The addition of useEffect hooks to synchronize
pageNumRefwithpageNumandnumPagesRefwithnumPagesis a good practice. These hooks ensure that the ref values are always up-to-date with their corresponding state values, which is crucial when using refs alongside state variables.
386-387: LGTM! Improved efficiency in handling logEventNum updates.The use of
updateUrlIfEventOnPagefunction to check if the log event is on the current page, along with the added comment, improves both the efficiency and readability of the code. This change avoids unnecessary page requests when the log event is already on the current page.
345-354: LGTM! setLogLevelFilter function implements log level filtering.The
setLogLevelFilterfunction correctly implements the log level filtering functionality by sending aSET_FILTERrequest to the worker.Consider the implications of using a fallback value of 1 for
logEventNumwhen it's null. This might not always be the desired behavior. It may be worth adding a comment explaining this choice or considering alternative approaches.To verify the impact of this fallback value, you could run the following script:
✅ Verification successful
LGTM! The
setLogLevelFilterfunction effectively implements log level filtering.The
setLogLevelFilterfunction correctly sends aSET_FILTERrequest to the worker and appropriately uses a fallback value of 1 forlogEventNumwhen it isnull.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for occurrences where logEventNum might be null # Test: Search for places where logEventNum is checked for null or undefined rg --type typescript 'logEventNum\s*(===?|!==?)\s*(null|undefined)' new-log-viewer/srcLength of output: 123
Script:
#!/bin/bash # Description: Check for occurrences where logEventNum might be null # Test: Search for places where logEventNum is checked for null or undefined rg --type ts 'logEventNum\s*(===?|!==?)\s*(null|undefined)' new-log-viewer/srcLength of output: 80
new-log-viewer/src/services/LogFileManager/index.ts (3)
6-6: Import of 'LogLevelFilter' is appropriateThe import statement correctly brings in
LogLevelFilterfrom"../../typings/logs", which is necessary for implementing log level filtering functionality.
Line range hint
189-236: Updates to 'loadPage' method enhance pagination functionalityThe modifications to the
loadPagemethod correctly implement pagination with log level filtering. The logic for calculatingnumActiveEvents, handling cases with zero events, and decoding the appropriate range is sound.
256-273: Proper adjustment of '#getCursorData' method with 'numActiveEvents'The addition of the
numActiveEventsparameter to#getCursorDataensures accurate cursor data retrieval based on the current filtering state. The changes integrate well with existing cursor handling logic.
| /** | ||
| * Sets the log level filter. | ||
| * | ||
| * @param logLevelFilter | ||
| * @throws {Error} If the log level filter couldn't be set. | ||
| */ | ||
| setLogLevelFilter (logLevelFilter: LogLevelFilter) { | ||
| const result = this.#decoder.setLogLevelFilter(logLevelFilter); | ||
| if (false === result) { | ||
| throw new Error("Failed to set log level filter for the decoder."); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Include 'logLevelFilter' value in error message for better debugging
To enhance the clarity of the error message when setting the log level filter fails, consider including the value of logLevelFilter. This provides more context and aids in debugging.
Apply this diff to improve the error message:
if (false === result) {
- throw new Error("Failed to set log level filter for the decoder.");
+ throw new Error(`Failed to set log level filter '${logLevelFilter}' for the decoder.`);
}📝 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.
| /** | |
| * Sets the log level filter. | |
| * | |
| * @param logLevelFilter | |
| * @throws {Error} If the log level filter couldn't be set. | |
| */ | |
| setLogLevelFilter (logLevelFilter: LogLevelFilter) { | |
| const result = this.#decoder.setLogLevelFilter(logLevelFilter); | |
| if (false === result) { | |
| throw new Error("Failed to set log level filter for the decoder."); | |
| } | |
| } | |
| /** | |
| * Sets the log level filter. | |
| * | |
| * @param logLevelFilter | |
| * @throws {Error} If the log level filter couldn't be set. | |
| */ | |
| setLogLevelFilter (logLevelFilter: LogLevelFilter) { | |
| const result = this.#decoder.setLogLevelFilter(logLevelFilter); | |
| if (false === result) { | |
| throw new Error(`Failed to set log level filter '${logLevelFilter}' for the decoder.`); | |
| } | |
| } |
There was a problem hiding this comment.
silly rabbit tricks are for kids. The log level filter is not printable. I think its okay as is
There was a problem hiding this comment.
@davemarco, understood. Thank you for the clarification.
(_/)
( •︵•)
/ \
✏️ Learnings added
Learnt from: davemarco
PR: y-scope/yscope-log-viewer#89
File: new-log-viewer/src/services/LogFileManager/index.ts:136-147
Timestamp: 2024-10-09T15:38:36.380Z
Learning: The `logLevelFilter` is not printable, so error messages should not include its value.
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.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (5)
new-log-viewer/src/services/LogFileManager/utils.ts (4)
23-45: LGTM! Function updated to support filtered log events.The changes to
getPageNumCursorDataimprove type safety and align with the new filtering functionality. The logic for calculating cursor data is correct and efficient.Consider updating the function documentation to include a brief description of the
CursorDatareturn type and its properties. This would enhance the clarity for other developers using this function.
49-75: LGTM! New function handles filtered and unfiltered log events effectively.The
getNearestActiveLogCollectionEventIdxfunction is well-implemented and correctly handles both filtered and unfiltered cases. The use of utility functions and the type assertion are appropriate.For improved clarity, consider adding a brief comment explaining why the type assertion is necessary on line 75. This would help other developers understand the reasoning behind this decision.
78-104: LGTM! Function updated to support filtered log events.The changes to
getEventNumCursorDataalign well with the new filtering functionality and improve type safety. The use ofgetNearestActiveLogCollectionEventIdxis appropriate and enhances the function's capabilities.For consistency with the parameter name, consider renaming the
logEventNumvariable tologEventIdxwhen passing it togetNearestActiveLogCollectionEventIdxon line 95. This would make the code slightly more intuitive:- logEventNum - 1, + logEventIdx - 1,
106-122: LGTM! Function updated to support filtered log events.The changes to
getLastEventCursorDataalign well with the new filtering functionality and improve type safety. The logic for calculating cursor data for the last event is correct and efficient.For consistency with other functions, consider adding the
filteredLogEventMapparameter to this function, even if it's not used in the current implementation. This would make the function signature more uniform withgetEventNumCursorData:const getLastEventCursorData = ( numActiveEvents: number, pageSize: number, filteredLogEventMap: FilteredLogEventMap ): CursorData => { // ... (existing implementation) };This change would make it easier to extend the function in the future if needed.
new-log-viewer/src/services/LogFileManager/index.ts (1)
252-253: Include documentation fornumActiveEventsparameterThe parameter
numActiveEventsin the method#getCursorDatalacks a description in the JSDoc comment. Adding a brief explanation will enhance code clarity and maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- new-log-viewer/src/services/LogFileManager/index.ts (5 hunks)
- new-log-viewer/src/services/LogFileManager/utils.ts (2 hunks)
- new-log-viewer/src/typings/worker.ts (7 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- new-log-viewer/src/typings/worker.ts
🧰 Additional context used
🔇 Additional comments (3)
new-log-viewer/src/services/LogFileManager/utils.ts (1)
2-13: LGTM! Import statements are consistent with new functionality.The new imports support the added functionality for handling filtered log events and cursor data. The changes are well-organized and improve the overall structure of the file.
new-log-viewer/src/services/LogFileManager/index.ts (2)
136-147: Implementation ofsetLogLevelFiltermethod is correctThe
setLogLevelFiltermethod correctly sets the log level filter with appropriate error handling.
189-198:loadPagemethod modifications are well-integratedThe updates to the
loadPagemethod effectively incorporate log level filtering and handle cases where there are no active events.
There was a problem hiding this comment.
For the PR title and commit body, how about:
new-log-viewer: Finish support for filtering by log-level and associated refactoring:
- Add new worker request for setting a log-level filter and handling the change in number of pages.
- Use log event indices rather than numbers in loadPage.
- Clarify the difference in indices into the complete log events collection and the filtered log events map.
- Remove unnecessary decoder options arg from `EXPORT_FILE` and `LOAD_PAGE` requests.
davemarco
changed the title
davemarco
changed the title
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 #77 #78 #79 #80 #81 #82 #83 #84
Description
This PR links all the previous filtering PRs (#76 + #79 + #81) together to provide real filtering functionality. All that is now needed is to link the log level filter button to the
setLogLevelFiltercall back in the state context.Requests/Response
Adds request and response for filtering similar to #63.
State Context
Modified logic in
logEventNumuseEffect to check if the new log event number is bounded by numbers on the current page, and if so, find the nearest log event and update the url. Takes advantage of methods from #81.Load page changes
Modified load page to use indexes instead of nums. I just found it confusing to work with nums when the values in the filtered array were indexes. Alot of the work on this PR was fixing off-by-one errors...
In addition,
getEventNumCursorDatanow takes advantage of methods in #81 to find the nearest log event when logs are filtered.Validation performed
Tested with some hard coded filters, and everything seemed good. Further validation can be performed when the button is linked.
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes