new-log-viewer: Add event cursor, refactor state context, and redesign load page methods to support discontinuous log event numbers in preparation for log level filtering. #76
Conversation
WalkthroughThe changes involve a significant refactoring of the log viewer's context management and pagination handling. Key modifications include updating the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant NavigationBar
participant StateContext
participant LogFileManager
User->>NavigationBar: Clicks navigation button
NavigationBar->>StateContext: Calls loadPageByAction(navAction)
StateContext->>LogFileManager: Loads page data
LogFileManager-->>StateContext: Returns logs, logEventNum, pageNum
StateContext-->>NavigationBar: Updates state
NavigationBar-->>User: Displays updated page
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: 6
Outside diff range and nitpick comments (11)
new-log-viewer/src/services/fileManager/utils.ts (1)
14-26: LGTM: Function implementation is correct. Consider adding return type annotation.The
getRangefunction is well-implemented and correctly calculates the range of log events to decode. The logic is sound and includes proper boundary checking.Consider adding an explicit return type annotation to improve code readability:
const getRange = ( numEvents: number, beginLogEventIdx: number, pageSize: number -): [number, number] => { +): [number, number] => {new-log-viewer/src/components/MenuBar/NavigationBar.tsx (2)
20-20: Simplified context usage improves code clarityThe refactoring to use only the
loadPagefunction fromStateContextsimplifies the component's dependencies and aligns well with the PR objectives. This change should make the component more maintainable and easier to understand.However, to further improve code readability, consider using a more descriptive name for the destructured
loadPagefunction. For example:- const {loadPage} = useContext(StateContext); + const {loadPage: navigateToPage} = useContext(StateContext);This change would make the function's purpose more explicit within the context of the NavigationBar component.
25-25: Streamlined navigation handling supports future improvementsThe direct use of
loadPagewith theactionNameparameter simplifies the navigation logic and aligns well with the PR objectives. This change sets the groundwork for the future implementation of the log event cursor.To enhance type safety, consider using a type assertion for
actionNamethat's more specific thanACTION_NAME. This would ensure that only valid navigation actions are passed toloadPage:type NavigationAction = Extract<ACTION_NAME, ACTION_NAME.FIRST_PAGE | ACTION_NAME.PREV_PAGE | ACTION_NAME.NEXT_PAGE | ACTION_NAME.LAST_PAGE>; const handleNavButtonClick = (event: React.MouseEvent<HTMLButtonElement>) => { const {actionName} = event.currentTarget.dataset as { actionName: NavigationAction }; loadPage(actionName); };This change would prevent potential runtime errors if invalid action names are accidentally used.
new-log-viewer/src/typings/worker.ts (2)
89-91: The WorkerRespMap type update is correct, but consider reordering properties.The addition of logEventNum and pageNum properties to the WORKER_RESP_CODE.PAGE_DATA response type aligns with the PR objectives. These changes support the new approach for handling pagination and log event cursors, allowing the backend to return the correct page number and log event number.
Consider reordering the properties for better readability:
[WORKER_RESP_CODE.PAGE_DATA]: { beginLineNumToLogEventNum: BeginLineNumToLogEventNumMap, cursorLineNum: number, pageNum: number, logEventNum: number, logs: string },This order groups related properties together and follows a logical flow from page-level information to specific log data.
Line range hint
1-130: Overall, the changes to worker.ts are well-implemented and support the PR objectives.The modifications to types and enums in this file provide a solid foundation for implementing the log event cursor functionality. The introduction of the LOG_EVENT_ANCHOR enum, updates to the CursorArgMap and WorkerRespMap types, and the export statement changes are all consistent with the PR objectives.
These changes will enable better support for future log filtering and improve the handling of pagination, especially in cases where log events are sparse due to filtering. The modifications maintain backward compatibility while introducing new features, which is a good practice.
As you proceed with implementing the log event cursor functionality in subsequent PRs, ensure that the backend components (e.g., API endpoints, database queries) are updated to support these new types and provide the required information (logEventNum, pageNum) in the response.
new-log-viewer/src/components/Editor/index.tsx (1)
85-85: LGTM! Action handling simplified, but consider a minor improvement.The changes to
handleEditorCustomActionalign well with the PR objectives. Directly callingloadPagewithACTION_NAME.LAST_PAGEsimplifies the logic and prepares for future log filter support.Consider using the
actionNameparameter instead of hardcodingACTION_NAME.LAST_PAGE:- loadPage(ACTION_NAME.LAST_PAGE); + loadPage(actionName);This change would make the function more flexible and consistent with its signature.
Also applies to: 101-101
new-log-viewer/src/utils/actions.ts (2)
130-133: Log error message in default case of switch statement.In the default case of the switch statement, the function returns
[null, null]without logging any error or warning. Consider adding a log statement to indicate that an unknown action was received, which can aid in debugging.
69-76: Update documentation comment to accurately reflect return values.The function documentation states "Returns null if the action is not setup," but the function actually returns a tuple
[null, null]. Consider updating the comment to specify that it returns a tuple withnullvalues, improving clarity for future developers.new-log-viewer/src/services/fileManager/LogFileManager.ts (2)
175-177: Update method documentation to reflect new return valuesThe
loadPagemethod now returns additional propertieslogEventNumandpageNum. To maintain clarity for users of this method, please update the JSDoc comments to include these new return values and their descriptions.
5-22: Organize import statements for improved readabilityThe import statements include both external and internal modules. Consider grouping them logically—for example, external libraries first, followed by internal modules—and sorting them alphabetically within each group. This enhances readability and makes it easier to locate specific imports.
new-log-viewer/src/contexts/StateContextProvider.tsx (1)
255-259: Consider Additional Error Handling inloadPageWhen
newPageNumoranchorarenull, the function logs an error and returns. Consider providing more detailed error messages or handling to aid in debugging, especially if this condition could arise due to unexpectedactionvalues.Apply this diff to enhance the error message:
- console.error(`Error with page action ${action}.`); + console.error(`Error in loadPage: Invalid page number or anchor for action ${action}.`);
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (10)
- new-log-viewer/src/components/Editor/index.tsx (3 hunks)
- new-log-viewer/src/components/MenuBar/NavigationBar.tsx (2 hunks)
- new-log-viewer/src/components/MenuBar/PageNumInput.tsx (3 hunks)
- new-log-viewer/src/contexts/StateContextProvider.tsx (9 hunks)
- new-log-viewer/src/services/MainWorker.ts (1 hunks)
- new-log-viewer/src/services/fileManager/LogFileManager.ts (3 hunks)
- new-log-viewer/src/services/fileManager/utils.ts (1 hunks)
- new-log-viewer/src/typings/worker.ts (4 hunks)
- new-log-viewer/src/utils/actions.ts (2 hunks)
- new-log-viewer/src/utils/math.ts (0 hunks)
Files not reviewed due to no reviewable changes (1)
- new-log-viewer/src/utils/math.ts
Additional comments not posted (27)
new-log-viewer/src/services/fileManager/utils.ts (2)
1-3: LGTM: Import statements are appropriate.The import statements are relevant to the functionality of this file and correctly import the necessary types and utility functions.
54-57: LGTM: Export statements are correct.The export statements correctly export both utility functions
getRangeandloadFilefor use in other parts of the application.new-log-viewer/src/components/MenuBar/NavigationBar.tsx (2)
9-9: Good practice: Centralized action namesThe addition of the
ACTION_NAMEimport from a separate utility file is a positive change. This centralization of action names promotes consistency and makes maintenance easier across the application.
Line range hint
1-62: Overall, the changes improve code quality and align with PR objectivesThe modifications to the NavigationBar component successfully simplify its logic, reduce dependencies, and prepare it for future enhancements. The changes align well with the PR objectives of revising the state context and modifying page load methods.
Key improvements:
- Centralized action names
- Simplified context usage
- Streamlined navigation handling
These changes should make the code more maintainable and easier to extend with the planned log event cursor functionality. The minor suggestions provided in the review comments could further enhance code clarity and type safety.
new-log-viewer/src/components/MenuBar/PageNumInput.tsx (3)
12-12: LGTM: New import added for ACTION_NAME.The addition of this import is consistent with the changes made in the component. It's good practice to import only what's necessary.
39-39: Approve change: Updated loadPage function call with action type.The addition of
ACTION_NAME.SPECIFIC_PAGEas the first argument toloadPageimproves code clarity and aligns with the PR objectives. This change enhances the specificity of the page loading action.To ensure consistency, please run the following script to check if this change has been applied uniformly across the codebase:
#!/bin/bash # Description: Check for consistent usage of loadPage with ACTION_NAME.SPECIFIC_PAGE # Test: Search for loadPage function calls rg --type typescript --type tsx 'loadPage\s*\(' -A 2
69-71: Approve change: Updated useEffect dependency and input value assignment.The modification to use
pageNumdirectly in both the input value assignment and the useEffect dependency array simplifies the component's logic. This change is consistent with the removal ofadjustedPageNumas mentioned in the summary.To ensure
pageNumis correctly updated and used consistently, please run the following script:Verification successful
Approve change: Updated useEffect dependency and input value assignment.
The modification to use
pageNumdirectly in both the input value assignment and theuseEffectdependency array is consistent with its usage across the codebase. No issues were found regarding the updates and management ofpageNum.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for consistent usage and updates of pageNum # Test: Search for pageNum usage and updates rg --type typescript --type tsx 'pageNum' -A 2Length of output: 89
Script:
#!/bin/bash # Description: Check for consistent usage and updates of pageNum # Test: Search for pageNum usage and updates in TypeScript and TSX files rg --type ts --type tsx 'pageNum' -A 2Length of output: 66
Script:
#!/bin/bash # Description: Check for consistent usage and updates of pageNum # Test: Search for pageNum usage and updates in all files rg 'pageNum' -A 2Length of output: 4977
new-log-viewer/src/typings/worker.ts (4)
10-17: LGTM! The new LOG_EVENT_ANCHOR enum is well-defined.The new enum aligns with the PR objectives and provides a clear way to specify whether the log event number should be anchored to the top or bottom of the page. The naming is self-explanatory, and the JSDoc comment provides sufficient context.
23-23: The updated comment for PAGE_NUM is accurate and consistent.The modification to the comment for the PAGE_NUM cursor code accurately reflects the new functionality, mentioning both first and last events on a given page. This change is consistent with the introduction of the LOG_EVENT_ANCHOR enum and supports the new approach for log event cursors.
34-34: The CursorArgMap type update is correct and aligns with the new functionality.The addition of the logEventAnchor property to the PAGE_NUM cursor definition is consistent with the PR objectives and the introduction of the LOG_EVENT_ANCHOR enum. This change allows specifying whether the cursor should point to the first or last event on a page, which is crucial for the new log event cursor functionality.
117-117: The export statement update is correct.The addition of LOG_EVENT_ANCHOR to the export statement is necessary and consistent. This change ensures that the new enum is available for use in other parts of the application, which is crucial for implementing the new log event cursor functionality.
new-log-viewer/src/services/MainWorker.ts (1)
13-13: Approved: Import path updated to reflect new file structure.The change in the import path for LogFileManager is correct and reflects the restructuring of the project's file organization. This modification doesn't affect the functionality of the code directly.
To ensure consistency across the project, let's verify if other files importing LogFileManager have been updated accordingly:
If the first command returns any results, those files may need to be updated to use the new import path.
Verification successful
Fix Import Path Verification Script
There was an error in the previous verification script due to incorrect quote usage. Please run the corrected script below to ensure all import paths are consistent:
Fix Import Path Verification Script
Apologies for the errors in the previous script. Please run the corrected script below to ensure all import paths are consistent:
Fix Import Path Verification Script
Apologies for the continued errors in the verification script. Please run the corrected script below to ensure all import paths are consistent:
Adjust Import Path Verification Script
The previous script failed because
rgdoes not recognize the file typetypescript. Please run the corrected scripts below to ensure all import paths are consistent:If the above script still results in an unrecognized file type error, please run the following script without specifying the file type:
Import Path Consistency Verified
All import paths for
LogFileManagerhave been successfully updated to./fileManager/LogFileManager. No remaining imports from the old path were found.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining imports of LogFileManager from the old path # Test: Search for imports from the old path. Expect: No results. rg --type typescript 'from\s+[\'"]\.\/LogFileManager[\'"]' # Test: Search for imports from the new path. Expect: At least one result (this file). rg --type typescript 'from\s+[\'"]\.\/fileManager\/LogFileManager[\'"]'Length of output: 93
Script:
#!/bin/bash # Description: Check for any remaining imports of LogFileManager from the old path # Test: Search for imports from the old path. Expect: No results. rg --type typescript "from\s+['\"]\.\/LogFileManager['\"]" # Test: Search for imports from the new path. Expect: At least one result (this file). rg --type typescript "from\s+['\"]\.\/fileManager\/LogFileManager['\"]"Length of output: 381
Script:
#!/bin/bash # Description: Check for any remaining imports of LogFileManager from the old path # Test: Search for imports from the old path. Expect: No results. rg --type typescript 'from\s+["\']\.\/LogFileManager["\']' # Test: Search for imports from the new path. Expect: At least one result (this file). rg --type typescript 'from\s+["\']\.\/fileManager\/LogFileManager["\']'Length of output: 93
Script:
#!/bin/bash # Description: Check for any remaining imports of LogFileManager from the old path # Test: Search for imports from the old path. Expect: No results. rg --type typescript "from\\s+['\"]\\./LogFileManager['\"]" # Test: Search for imports from the new path. Expect: At least one result (this file). rg --type typescript "from\\s+['\"]\\./fileManager/LogFileManager['\"]"Length of output: 209
Script:
#!/bin/bash # Description: Check for any remaining imports of LogFileManager from the old path # Test 1: Search for imports from the old path. Expect: No results. rg --type ts "from\s+['\"]\./LogFileManager['\"]" # Test 2: Search for imports from the new path. Expect: At least one result (this file). rg --type ts "from\s+['\"]\./fileManager/LogFileManager['\"]"Length of output: 224
Script:
#!/bin/bash # Description: Check for any remaining imports of LogFileManager from the old path without specifying file type # Test 1: Search for imports from the old path. Expect: No results. rg "from\s+['\"]\./LogFileManager['\"]" # Test 2: Search for imports from the new path. Expect: At least one result (this file). rg "from\s+['\"]\./fileManager/LogFileManager['\"]"Length of output: 204
new-log-viewer/src/components/Editor/index.tsx (2)
65-65: LGTM! State management simplified, but clarification needed.The removal of
useEffecthooks forlogEventNumRefandnumEventsRefsynchronization, along with the simplified dependency array for thelineNumupdate, aligns well with the PR objectives. These changes suggest a move towards a more centralized state management approach.Could you please clarify the new state management approach? Specifically:
- How will the log event number be tracked without
logEventNumRef?- How will the total number of events be managed without
numEventsRef?- Are there any plans to implement a replacement for these refs in the upcoming log event cursor PR?
To help verify the removal of these refs, please run the following script:
#!/bin/bash # Description: Check for any remaining usage of logEventNumRef and numEventsRef # Test: Search for any occurrences of logEventNumRef and numEventsRef rg --type typescript 'logEventNumRef|numEventsRef' new-log-viewer/src/components/Editor/index.tsx
65-65: LGTM! Imports and context usage updated appropriately.The changes in imports and context usage align well with the PR objectives. The addition of
loadPagefromStateContextand the removal ofhandleActionimport reflect the shift in state management approach.Let's verify if there are any unused imports remaining:
new-log-viewer/src/utils/actions.ts (3)
3-6: Imports are correctly added.The added imports
STATE_DEFAULT,LOG_EVENT_ANCHOR, andclampare necessary and properly included.
96-103: Re-evaluate condition for unset current page number.The check
STATE_DEFAULT.pageNum === currentPageNummight not accurately detect whether the current page number is set. Ensure thatSTATE_DEFAULT.pageNumrepresents an unset state and that this condition properly handles all cases where the current page number is invalid.
145-145: ExportinggetPageNumCursorArgsfunction is appropriate.The new function
getPageNumCursorArgsis correctly exported for use in other modules.new-log-viewer/src/services/fileManager/LogFileManager.ts (2)
205-206: Verify the calculation of the new page numberThe
newPageNumis calculated usingbeginLogEventNumwith thegetChunkNumfunction. SincebeginLogEventNumstarts at1, ensure thatgetChunkNumcorrectly handles this value to compute the accurate page number without introducing off-by-one errors.
181-181: Ensure correct indexing in decoder decode methodWhen calling
this.#decoder.decode(beginLogEventNum - 1, endLogEventNum);, subtracting1frombeginLogEventNumcould result in a negative index ifbeginLogEventNumis0. Confirm thatbeginLogEventNumis always at least1to prevent passing a negative index, which could cause runtime errors or unexpected behaviour.new-log-viewer/src/contexts/StateContextProvider.tsx (8)
20-25: Approval of Added ImportsThe imported entities (
LOG_EVENT_ANCHOR,MainWorkerRespMessage,WORKER_REQ_CODE,WORKER_RESP_CODE,WorkerReq) are appropriate and necessary for the new functionalities introduced.
26-29: New Imports from "../utils/actions" ConfirmedThe import of
ACTION_NAMEandgetPageNumCursorArgsfrom"../utils/actions"is correct and aligns with their usage in the updated code.
54-54: Verify Non-NullablepageNumImplementationThe
pageNumproperty inStateContextTypehas been changed to a non-nullablenumber. Ensure that all initializations and usages ofpageNumthroughout the codebase correctly handle this change to prevent potentialundefinedornullreferences.
145-145: Initialization ofpageNumRefThe addition of
pageNumRefinitialized withSTATE_DEFAULT.pageNumis appropriate for maintaining the current page number reference.
171-179: Ensure Validity ofargs.logEventNumBefore Updating URLWhile updating
pageNumRefandbeginLineNumToLogEventNumRef, you're also updating the URL hash parameters withargs.logEventNum. Verify thatargs.logEventNumis always a valid number before using it to prevent potential inconsistencies in the URL state.
238-266:loadPageFunction Refactoring VerifiedThe
loadPagefunction has been successfully refactored to useactionandspecificPageNum. The use ofgetPageNumCursorArgsto computenewPageNumandanchorenhances the code's readability and maintainability. The null checks and error handling are appropriate.
262-266: Confirm Payload Structure forworkerPostReqEnsure that the payload sent to
workerPostReqmatches the expected structure, particularly thecursorobject. This will prevent any communication issues between the main thread and the worker.
368-371: ExportingSTATE_DEFAULTIncluding
STATE_DEFAULTin the exports allows other modules to access the default state configuration, which is useful for consistency across the application.
| #getCursorData (cursor: CursorType): { | ||
| beginLogEventNum: number, endLogEventNum: | ||
| number, | ||
| newLogEventNum: number} { | ||
| let beginLogEventIdx: number = 0; | ||
| let beginLogEventNum: number = 1; | ||
| let endLogEventNum: number = 0; | ||
| let newLogEventNum: number = 1; | ||
|
|
||
| if (0 === this.#numEvents) { | ||
| return { | ||
| beginLogEventNum: 1, | ||
| endLogEventNum: 0, | ||
| beginLogEventNum: beginLogEventNum, | ||
| endLogEventNum: endLogEventNum, | ||
| newLogEventNum: newLogEventNum, | ||
| }; | ||
| } | ||
|
|
||
| let beginLogEventIdx: number = 0; | ||
| const {code, args} = cursor; | ||
| if (CURSOR_CODE.PAGE_NUM === code) { | ||
| beginLogEventIdx = ((args.pageNum - 1) * this.#pageSize); | ||
| [beginLogEventNum, endLogEventNum] = getRange( | ||
| this.#numEvents, | ||
| beginLogEventIdx, | ||
| this.#pageSize, | ||
| ); | ||
| if (LOG_EVENT_ANCHOR.FIRST === args.logEventAnchor) { | ||
| newLogEventNum = beginLogEventNum; | ||
| } else { | ||
| newLogEventNum = endLogEventNum; | ||
| } |
There was a problem hiding this comment.
Use 'else if' to prevent multiple condition blocks from executing
In the #getCursorData method, the if conditions for CURSOR_CODE.PAGE_NUM and CURSOR_CODE.LAST_EVENT are separated by standalone if statements. If code equals CURSOR_CODE.PAGE_NUM and beginLogEventIdx > this.#numEvents, both condition blocks could execute, which might not be the intended behaviour. To ensure that only one condition block executes, consider changing the second if to an else if.
Apply this diff to modify the condition:
const {code, args} = cursor;
if (CURSOR_CODE.PAGE_NUM === code) {
beginLogEventIdx = ((args.pageNum - 1) * this.#pageSize);
[beginLogEventNum, endLogEventNum] = getRange(
this.#numEvents,
beginLogEventIdx,
this.#pageSize,
);
if (LOG_EVENT_ANCHOR.FIRST === args.logEventAnchor) {
newLogEventNum = beginLogEventNum;
} else {
newLogEventNum = endLogEventNum;
}
- }
- if (CURSOR_CODE.LAST_EVENT === code || beginLogEventIdx > this.#numEvents) {
+ } else if (CURSOR_CODE.LAST_EVENT === code || beginLogEventIdx > this.#numEvents) {
// Set to the first event of the last page
beginLogEventIdx = (getChunkNum(this.#numEvents, this.#pageSize) - 1) * this.#pageSize;
[beginLogEventNum, endLogEventNum] = getRange(
this.#numEvents,
beginLogEventIdx,
this.#pageSize,
);
newLogEventNum = endLogEventNum;
} else if (CURSOR_CODE.TIMESTAMP === code) {
throw new Error(`Unsupported cursor type: ${code}`);
}Committable suggestion was skipped due to low confidence.
There was a problem hiding this comment.
This has been refactored as part of later commit
There was a problem hiding this comment.
@davemarco, understood. Thanks for letting me know that this has been refactored in a later commit.
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
davemarco
changed the title
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (6)
new-log-viewer/src/services/fileManager/utils.ts (5)
21-33: LGTM: Function implementation is correct. Consider a minor readability improvement.The
getRangefunction is well-implemented and correctly calculates the range of log events to decode. It properly handles the conversion from index to event number and ensures the ending event number doesn't exceed the total number of events.For improved readability, consider using object destructuring in the return statement:
- return [beginLogEventNum, - endLogEventNum]; + return [beginLogEventNum, endLogEventNum];This minor change aligns with modern JavaScript conventions and improves code consistency.
43-59: LGTM: Function implementation is correct. Consider adding error handling for unsupported file source types.The
loadFilefunction is well-implemented and correctly handles both URL string and File object inputs. It addresses the concerns raised in the past review comments by using appropriate utility functions and type checking.To fully address the past review comments and improve error handling, consider adding a check for unsupported file source types:
const loadFile = async (fileSrc: FileSrcType) : Promise<{ fileName: string, fileData: Uint8Array }> => { let fileName: string; let fileData: Uint8Array; if ("string" === typeof fileSrc) { fileName = getBasenameFromUrlOrDefault(fileSrc); fileData = await getUint8ArrayFrom(fileSrc, () => null); } else if (fileSrc instanceof File) { fileName = fileSrc.name; fileData = new Uint8Array(await fileSrc.arrayBuffer()); + } else { + throw new Error("Unsupported file source type"); } return { fileName, fileData, }; };This addition ensures that the function throws an error for unsupported file source types, as mentioned in the JSDoc comment.
70-83: LGTM: Function implementation is correct. Consider a minor improvement for consistency.The
getPageNumCursorDatafunction is well-implemented and correctly calculates the range and new log event number using a page number cursor. It properly utilizes thegetRangefunction and handles thelogEventAnchorparameter.For consistency with other functions in this file, consider using object property shorthand in the return statement:
- return {beginLogEventNum, endLogEventNum, newLogEventNum}; + return {beginLogEventNum, endLogEventNum, newLogEventNum};This minor change aligns with modern JavaScript conventions and improves code consistency throughout the file.
93-103: LGTM: Function implementation is correct. Consider a minor improvement for consistency.The
getEventNumCursorDatafunction is well-implemented and correctly calculates the range and new log event number using an event number cursor. It properly clamps the log event number and utilizes thegetChunkNumandgetRangefunctions.For consistency with other functions in this file, consider removing the type annotation for
newLogEventNumas it's not necessary:- const newLogEventNum: number = validLogEventNum; + const newLogEventNum = validLogEventNum;This minor change improves code consistency throughout the file without affecting functionality.
112-120: LGTM: Function implementation is correct. Consider a minor improvement for consistency.The
getLastEventCursorDatafunction is well-implemented and correctly calculates the range and new log event number for the last event cursor. It properly utilizes thegetChunkNumandgetRangefunctions to determine the appropriate range.For consistency with other functions in this file, consider removing the type annotation for
newLogEventNumas it's not necessary:- const newLogEventNum: number = endLogEventNum; + const newLogEventNum = endLogEventNum;This minor change improves code consistency throughout the file without affecting functionality.
new-log-viewer/src/contexts/StateContextProvider.tsx (1)
Line range hint
124-154: Add Validation forargs.logEventNuminhandleMainWorkerRespWithin the
WORKER_RESP_CODE.PAGE_DATAcase ofhandleMainWorkerResp, it is assumed thatargs.logEventNumis always valid. To enhance robustness, consider adding validation to check thatargs.logEventNumis not null or undefined before updating the window URL hash parameters.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
- new-log-viewer/src/components/Editor/index.tsx (3 hunks)
- new-log-viewer/src/components/MenuBar/NavigationBar.tsx (2 hunks)
- new-log-viewer/src/components/MenuBar/PageNumInput.tsx (4 hunks)
- new-log-viewer/src/contexts/StateContextProvider.tsx (10 hunks)
- new-log-viewer/src/services/fileManager/LogFileManager.ts (3 hunks)
- new-log-viewer/src/services/fileManager/utils.ts (1 hunks)
- new-log-viewer/src/typings/worker.ts (3 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- new-log-viewer/src/components/MenuBar/NavigationBar.tsx
- new-log-viewer/src/components/MenuBar/PageNumInput.tsx
- new-log-viewer/src/services/fileManager/LogFileManager.ts
- new-log-viewer/src/typings/worker.ts
🔇 Additional comments not posted (8)
new-log-viewer/src/services/fileManager/utils.ts (1)
122-127: LGTM: Export statement is correct and complete.The export statement correctly includes all five utility functions defined in this file, making them available for use in other parts of the application.
new-log-viewer/src/components/Editor/index.tsx (3)
65-65: Changes align with PR objectivesThe removal of
handleActionand addition ofloadPageActionin the context usage aligns well with the PR objectives of modifying the state context and page load methods. This change supports the new approach for handling page loading and log event cursors.
85-85: Verify the intended behavior ofhandleEditorCustomActionThe function now directly calls
loadPageActionwith a hardcoded action nameACTION_NAME.LAST_PAGE. However, this seems to contradict the switch statement that includes other action names likeFIRST_PAGE,PREV_PAGE, andNEXT_PAGE.Please verify if this is the intended behavior or if it should use the
actionNameparameter passed to the function.If this is intentional, consider adding a comment explaining why only the
LAST_PAGEaction is being used.Additionally, the dependency array has been correctly updated to include
loadPageAction.Also applies to: 101-101
Line range hint
1-191: Verify impact of removed effects and refsThe removal of several
useEffecthooks and references tologEventNumRefandnumEventsRefaligns with the PR objectives of revising the state context. This simplification likely supports the new approach for handling log event cursors and pagination.However, please verify that no critical functionality has been lost in this refactoring. Ensure that:
- The component still updates correctly when the log event number changes.
- Pagination still works as expected without these refs.
- Any functionality previously handled by the removed effects is now managed appropriately elsewhere.
new-log-viewer/src/contexts/StateContextProvider.tsx (4)
72-72: AddloadPageActiontoSTATE_DEFAULTThe
STATE_DEFAULTnow includesloadPageAction. Ensure that any components or logic relying on the default state are updated to accommodate this addition.
332-335: Verify Imports Due to Updated Export StatementThe export statement now includes
STATE_DEFAULTalongsideStateContext. Verify that all modules importing fromStateContextProviderare updated if they need to importSTATE_DEFAULT.To find all import statements affected, run:
#!/bin/bash # Description: Find all files importing from `StateContextProvider`. rg --type ts "from.*StateContextProvider"
273-287:⚠️ Potential issueInclude
loadPageinuseEffectDependency ArrayIn the
useEffecthook starting at line 273, theloadPagefunction is used but not included in the dependency array. This could lead to issues ifloadPagechanges, as the effect would not re-run. IncludingloadPageensures the hook behaves as expected.Apply this diff to update the dependency array:
- ], [ + ], [ logEventNum, + loadPage, ]);Likely invalid or redundant comment.
50-50: Update Type Definition ofpageNumThe
pageNuminStateContextTypeis now defined asnumberinstead ofNullable<number>. Ensure this change is consistently reflected throughout the codebase, and thatpageNumis always assigned a valid number.To verify all usages of
pageNumand check for any potential issues, run:
new-log-viewer/src/typings/worker.ts
Outdated
| * Indicates whether the log event number should be anchored to the top or bottom of the page. | ||
| * Used as input for the page number cursor. |
There was a problem hiding this comment.
| * Indicates whether the log event number should be anchored to the top or bottom of the page. | |
| * Used as input for the page number cursor. | |
| * For a page requested by a `CURSOR_CODE`, this enum indicates which log event number (e.g., first | |
| * on page, or last on page) should be returned with the page. |
There was a problem hiding this comment.
I modified this a bit, since only true for page number cursor
new-log-viewer/src/typings/worker.ts
Outdated
| * Indicates whether the log event number should be anchored to the top or bottom of the page. | ||
| * Used as input for the page number cursor. | ||
| */ | ||
| enum LOG_EVENT_ANCHOR { |
There was a problem hiding this comment.
Anchor is not a bad name but at the same time, it feels a bit abstract. How about LOG_EVENT_IN_PAGE or LOG_EVENT_SELECTOR or PAGE_LOG_EVENT_SELECTOR?
There was a problem hiding this comment.
I tried EVENT_POSITION. let me know if you like better or prefer something else.
| @@ -240,17 +209,45 @@ const StateContextProvider = ({children}: StateContextProviderProps) => { | |||
| handleMainWorkerResp, | |||
| ]); | |||
|
|
|||
| const loadPage = (newPageNum: number) => { | |||
| const loadPage = useCallback(( | |||
There was a problem hiding this comment.
How about:
loadPage->loadPageByCursorloadPageAction->loadPageByAction
|
|
||
| const loadPageAction = useCallback(( | ||
| action: ACTION_NAME, | ||
| specificPageNum: Nullable<number> = null |
There was a problem hiding this comment.
Instead of passing specificPageNum separately, can we create a type like CursorType that includes args? I know we already have ActionType, so we'll probably need to rename that to something like UiActionType.
There was a problem hiding this comment.
I tried something similar to worker req/response type, but slightly more complex. See in actions.ts
| // TODO: When filter is added, this will need to find the <= log event num on the page. | ||
| // If it is not the current log event, it will need to update it. However, a new request | ||
| // is not necessary. |
There was a problem hiding this comment.
Shouldn't this be in Editor/index.tsx where we update the cursor's position?
There was a problem hiding this comment.
My understand of the code is probably worse than yours. Nonetheless here are my thoughts.
If a user is on a page with events [1,3,5]. and they type 4 in the url. We want the log event to update to 3. That is the purpose of this comment.
Perhaps the cursor position in Editor/index.tsx will work too? However, I have a feeling it only works for mouse events and not URL? I didn't read through editor code.
There was a problem hiding this comment.
In context of my upcoming review, yeah, we shouldn't move this comment.
| @@ -67,42 +64,84 @@ const EDITOR_ACTIONS : ActionType[] = [ | |||
| keyBindings: [monaco.KeyMod.CtrlCmd | monaco.KeyCode.KeyI], | |||
| }, | |||
| ]; | |||
| /* eslint-enable sort-keys */ | |||
There was a problem hiding this comment.
Yes - I didnt know why this was here when no other file has it?
It was more to remove something I thought was a random remnant. If it has a purpose, we can keep it.
There was a problem hiding this comment.
Gotcha. It's part of a pair with the other half being on line 26. So if we remove one, we should remove the other.
I thought disabling the rule was intentional to avoid sorting the keys of each object, but it turns out the rule in our config only enforces sorting if there're >= 5 keys, so we don't need the disabling.
There was a problem hiding this comment.
Oh okay. If someone changes config... I'll just add it back. I just thought random, but I guess has purpose.
There was a problem hiding this comment.
Also should me just move this whole file from utils to typings?
There was a problem hiding this comment.
Probably, but I think let's leave that to another PR or else this one will get too big.
Co-authored-by: kirkrodrigues <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
new-log-viewer/src/utils/actions.ts (1)
105-133: LGTM: Comprehensive action handling with a minor suggestion.The switch statement in
getPageNumCursorArgseffectively handles all relevant actions, using theclampfunction to ensure page numbers remain within valid bounds. TheLOG_EVENT_ANCHORis set appropriately for each action, which aligns well with the new pagination approach mentioned in the PR objectives.Consider adding a comment explaining why
PAGE_TOPandPAGE_BOTTOMactions are not handled in this function, as it might not be immediately clear to other developers why these actions from theACTION_NAMEenum are omitted.new-log-viewer/src/contexts/StateContextProvider.tsx (1)
224-247: New loadPageAction function enhances page navigationThe
loadPageActionfunction is a well-implemented higher-level interface for page loading based on actions. It aligns with the PR objectives of modifying page load methods and implementing a basic log event cursor.Key strengths:
- Utilizes
getPageNumCursorArgsfor calculatingnewPageNumandanchor.- Creates a
CursorTypeobject to encapsulate navigation information.- Delegates actual page loading to the
loadPagefunction.Suggestion for improvement:
Consider adding error handling or user feedback for cases wherenewPageNumoranchorare null, instead of just logging to the console.Consider implementing error handling:
if (null === newPageNum || null === anchor) { - console.error(`Error with page action ${action}.`); - - return; + const errorMessage = `Error with page action ${action}.`; + console.error(errorMessage); + // TODO: Implement user-facing error notification + // notifyUser(errorMessage); + return; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
- new-log-viewer/src/components/Editor/index.tsx (3 hunks)
- new-log-viewer/src/contexts/StateContextProvider.tsx (10 hunks)
- new-log-viewer/src/services/fileManager/utils.ts (1 hunks)
- new-log-viewer/src/typings/worker.ts (3 hunks)
- new-log-viewer/src/utils/actions.ts (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- new-log-viewer/src/components/Editor/index.tsx
- new-log-viewer/src/services/fileManager/utils.ts
- new-log-viewer/src/typings/worker.ts
🧰 Additional context used
📓 Learnings (1)
new-log-viewer/src/utils/actions.ts (2)
Learnt from: davemarco PR: y-scope/yscope-log-viewer#76 File: new-log-viewer/src/utils/actions.ts:84-139 Timestamp: 2024-09-25T21:13:37.250Z Learning: In the `getPageNumCursorArgs` function, variables `newPageNum` and `anchor` are always initialized when handling valid actions, so there is no risk of them being uninitialized.Learnt from: davemarco PR: y-scope/yscope-log-viewer#76 File: new-log-viewer/src/utils/actions.ts:105-133 Timestamp: 2024-09-25T21:12:28.732Z Learning: In the `actions.ts` file, the `PAGE_TOP` and `PAGE_BOTTOM` actions are not intended to be handled in the `getPageNumCursorArgs` function.
🔇 Additional comments (11)
new-log-viewer/src/utils/actions.ts (4)
3-6: LGTM: New imports are relevant and necessary.The added imports (STATE_DEFAULT, Nullable, LOG_EVENT_ANCHOR, and clamp) are appropriate for the new functionality introduced in this file. They provide necessary types and utilities for the
getPageNumCursorArgsfunction.
69-83: LGTM: Well-documented function signature.The function signature and JSDoc for
getPageNumCursorArgsare clear and informative. The parameters and return type are well-defined, and the documentation explains the purpose of the function and the meaning of null return values.
87-103: LGTM: Robust input validation.The input validation in
getPageNumCursorArgsis thorough and handles potential error cases well. It checks for a nullspecificPageNumwhen the action is SPECIFIC_PAGE and ensures that thecurrentPageNumis set. The error messages are clear, and the function returns null values as documented when validation fails.
145-145: LGTM: Appropriate export of new function.The
getPageNumCursorArgsfunction is correctly added to the exports, making it available for use in other parts of the application.new-log-viewer/src/contexts/StateContextProvider.tsx (7)
25-28: New imports align with PR objectivesThe addition of
ACTION_NAMEandgetPageNumCursorArgsimports from "../utils/actions" supports the implementation of the new page loading approach and log event cursor management. This change is consistent with the PR objectives.
72-72: Updated default state to match interface changesThe change from
loadPagetoloadPageActionin theSTATE_DEFAULTobject ensures consistency with the updatedStateContextTypeinterface. This modification maintains the integrity of the default state.
330-333: Improved module exportsThe addition of
STATE_DEFAULTto the export statement enhances the module's API by making this constant available for import in other modules. This change promotes code reuse and consistency across the application.
54-54: Updated loadPageAction method signatureThe renaming of
loadPagetoloadPageActionand the updated parameters (action: ACTION_NAME, specificPageNum?: Nullable<number>) align with the PR objective of modifying page load methods. This change allows for more flexible page loading based on actions.Verify that all calls to this method have been updated throughout the codebase:
#!/bin/bash # Search for any remaining calls to the old loadPage method rg --type typescript "loadPage\s*\(" src/ # Search for calls to the new loadPageAction method to ensure correct usage rg --type typescript "loadPageAction\s*\(" src/
146-150: Improved state and URL synchronizationThe changes in the
handleMainWorkerRespfunction enhance the synchronization between the application state and URL:
- Setting
pageNumRef.currentdirectly fromargs.pageNumensures accurate page number tracking.- Updating the URL hash params with
logEventNumfromargsmaintains consistency between the URL and the current log event.These modifications align with the PR objective of revising the state context upon receiving a page.
Verify the impact of these changes on state management:
#!/bin/bash # Search for other occurrences of pageNumRef updates to ensure consistency rg --type typescript "pageNumRef\.current\s*=" src/ # Check for other uses of updateWindowUrlHashParams to ensure proper usage rg --type typescript "updateWindowUrlHashParams" src/
209-222: New loadPage implementation supports cursor-based navigationThe new
loadPagefunction implementation with acursorparameter aligns with the PR objective of modifying page load methods. This change supports the new approach for handling log event numbers and pagination by using a cursor-based system.Key improvements:
- Use of
CursorTypefor flexible page loading.- Direct communication with the worker using
WORKER_REQ_CODE.LOAD_PAGE.This implementation provides a solid foundation for the future log event cursor functionality.
Verify the usage and integration of this new function:
#!/bin/bash # Search for calls to the new loadPage function rg --type typescript "loadPage\s*\(\s*\{" src/ # Check for other occurrences of WORKER_REQ_CODE.LOAD_PAGE to ensure consistent usage rg --type typescript "WORKER_REQ_CODE\.LOAD_PAGE" src/
50-50: Improved type safety for pageNumThe change from
Nullable<number>tonumberforpageNumenhances type safety and aligns with the PR objective of modifying the state context. This change ensures thatpageNumis always a valid number.Verify that this change doesn't introduce any issues in the codebase:
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (6)
new-log-viewer/src/utils/actions.ts (2)
65-77: LGTM: NavigationActionsMap type supports new pagination approach.The
NavigationActionsMaptype effectively structures the arguments for different navigation actions, aligning well with the PR objectives. It provides a clear interface for implementing the new pagination approach.A minor suggestion for consistency:
Consider adding trailing commas to all entries in the
NavigationActionsMaptype for consistency and easier future additions. For example:type NavigationActionsMap = { [ACTION_NAME.SPECIFIC_PAGE]: { specificPageNum: number, }, [ACTION_NAME.FIRST_PAGE]: null, [ACTION_NAME.PREV_PAGE]: null, [ACTION_NAME.NEXT_PAGE]: null, [ACTION_NAME.LAST_PAGE]: null, };
78-91: LGTM: NavigationAction type enhances type safety for navigation actions.The
NavigationActiontype effectively uses conditional types to create a union of action types with their respective arguments. This aligns well with the PR objectives by providing a type-safe structure for handling navigation actions.A minor suggestion for improved readability:
Consider breaking the
NavigationActiontype definition into multiple lines for better readability:type NavigationAction = { [T in keyof NavigationActionsMap]: NavigationActionsMap[T] extends object ? { code: T; args: NavigationActionsMap[T] } : { code: T }; }[keywise NavigationActionsMap];The updated exports ensure that the new types are available for use in other parts of the application, which is good for maintainability.
new-log-viewer/src/services/LogFileManager/utils.ts (1)
37-61: LGTM: The loadFile function is well-implemented, but could benefit from improved error handling.The function effectively handles both URL string and File object inputs, using appropriate utility functions for each case. However, consider enhancing error handling to provide more specific error messages for different failure scenarios. This would make debugging easier in case of issues.
For example:
if ("string" === typeof fileSrc) { try { fileName = getBasenameFromUrlOrDefault(fileSrc); fileData = await getUint8ArrayFrom(fileSrc, () => null); } catch (error) { throw new Error(`Failed to load file from URL: ${error.message}`); } } else { try { fileName = fileSrc.name; fileData = new Uint8Array(await fileSrc.arrayBuffer()); } catch (error) { throw new Error(`Failed to load File object: ${error.message}`); } }This change would provide more context about where the error occurred, making it easier to diagnose and fix issues.
new-log-viewer/src/contexts/StateContextProvider.tsx (3)
54-54: LGTM: StateContextType updates align with new navigation systemThe changes to
StateContextTypereflect the improved navigation system described in the PR objectives:
- Changing
pageNumto non-nullable ensures a page number is always available.- Renaming
loadPagetoloadPageByActionwith the newNavigationActionparameter supports the action-based navigation approach.These updates are consistent with the new cursor-based navigation system and will help prevent potential null-related issues.
Consider adding JSDoc comments to the
loadPageByActionmethod in the interface to describe the expected behaviour and parameters. This will improve code documentation and make it easier for other developers to understand and use this method correctly.Also applies to: 58-58
124-124: LGTM: New navigation system implementationThe changes to
StateContextProvidersuccessfully implement the new navigation system as described in the PR objectives:
- The
pageNumRefis now non-nullable, consistent with the interface changes.- New functions
loadPageByCursor,getPageNumCursor, andloadPageByActionprovide a robust and flexible navigation system.- The implementation supports different navigation actions and cursor-based loading, which will facilitate future log filtering features.
These changes significantly improve the log viewer's navigation capabilities and lay the groundwork for future enhancements.
In the
getPageNumCursorfunction, consider adding a default case to the switch statement that throws an error with a descriptive message. This would help catch any future additions to theACTION_NAMEenum that aren't handled:default: throw new Error(`Unhandled navigation action: ${navAction.code}`);This change would make the function more robust and easier to maintain in the future.
Also applies to: 213-284
302-321: LGTM: Improved logEventNum handlingThe revised logic for handling
logEventNumupdates aligns well with the new cursor-based navigation system:
- It checks if the current
logEventNumis on the current page before loading a new page.- It creates a new cursor object when necessary, supporting the cursor-based loading approach.
These changes improve efficiency and lay the groundwork for future log filtering implementation as mentioned in the PR objectives.
To improve code clarity, consider extracting the logic for creating the cursor into a separate function. This would make the useEffect hook easier to read and maintain. For example:
const createEventNumCursor = (logEventNum: number): CursorType => ({ code: CURSOR_CODE.EVENT_NUM, args: { logEventNum }, }); // In the useEffect hook if (!logEventNumsOnPage.includes(logEventNum)) { loadPageByCursor(createEventNumCursor(logEventNum)); }This change would make the code more modular and easier to understand at a glance.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
- new-log-viewer/src/components/Editor/index.tsx (3 hunks)
- new-log-viewer/src/components/MenuBar/NavigationBar.tsx (2 hunks)
- new-log-viewer/src/components/MenuBar/PageNumInput.tsx (4 hunks)
- new-log-viewer/src/contexts/StateContextProvider.tsx (10 hunks)
- new-log-viewer/src/services/LogFileManager/index.ts (3 hunks)
- new-log-viewer/src/services/LogFileManager/utils.ts (1 hunks)
- new-log-viewer/src/typings/worker.ts (4 hunks)
- new-log-viewer/src/utils/actions.ts (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- new-log-viewer/src/components/Editor/index.tsx
- new-log-viewer/src/components/MenuBar/NavigationBar.tsx
- new-log-viewer/src/components/MenuBar/PageNumInput.tsx
- new-log-viewer/src/typings/worker.ts
🔇 Additional comments (12)
new-log-viewer/src/utils/actions.ts (1)
Line range hint
7-14: LGTM: Updated ACTION_NAME enum aligns with PR objectives.The new actions added to the
ACTION_NAMEenum are consistent with the PR's goals of implementing a new approach for pagination and supporting the "log event anchor" concept. This change provides a solid foundation for the revised state context and page load methods.new-log-viewer/src/services/LogFileManager/utils.ts (7)
1-10: LGTM: Import statements are appropriate and concise.The import statements are well-organized and import only the necessary functions and types from various parts of the project. This promotes good code organization and modularity.
13-35: LGTM: The getRange function is well-implemented and documented.The function correctly calculates the range of log events to decode, handling edge cases appropriately. The use of Math.min ensures that the ending log event number doesn't exceed the total number of events. The JSDoc comment provides clear documentation of the function's purpose and parameters.
63-85: LGTM: The getPageNumCursorData function is well-implemented and efficient.The function effectively calculates the range and new log event number using the page number cursor. It makes good use of the previously defined getRange function, promoting code reuse. The handling of the eventPosition parameter to determine the newLogEventNum is correct and clear.
87-105: LGTM: The getEventNumCursorData function is well-implemented and handles edge cases.The function effectively calculates the range and new log event number using the event number cursor. The use of the clamp function ensures that the logEventNum is always within valid bounds, preventing potential errors. The getChunkNum function is used appropriately to determine the correct page for the given log event number.
107-122: LGTM: The getLastEventCursorData function is well-implemented for handling the last page.The function effectively calculates the range and new log event number for the last page of events. It correctly uses getChunkNum to determine the last chunk (page) number, ensuring that the calculation works for any number of events. Setting newLogEventNum to endLogEventNum is appropriate for the last page scenario.
124-129: LGTM: Exports are appropriate and well-organized.The export statements correctly expose the main utility functions for external use while keeping the internal getRange function private. This approach promotes good encapsulation and provides a clean interface for other parts of the application to interact with this module.
1-129: Overall, excellent implementation that aligns well with PR objectives.This new utils.ts file provides a robust set of utility functions that support the PR objectives, particularly in improving pagination handling and setting the groundwork for future log filtering. The functions are well-implemented, properly documented, and provide a solid foundation for the changes described in the PR summary.
The code demonstrates good practices such as:
- Clear and consistent function naming
- Proper error handling and edge case management
- Efficient use of utility functions to promote code reuse
- Clear separation of concerns among different functions
These utility functions will greatly facilitate the management and retrieval of log events, enhancing the functionality of the log viewer as intended by the PR.
new-log-viewer/src/contexts/StateContextProvider.tsx (4)
19-19: LGTM: New imports support the revised navigation systemThe addition of
EVENT_POSITION,ACTION_NAME, andNavigationActionimports aligns with the PR objectives to revise the state context and load page methods. These new types and constants are essential for implementing the improved navigation system.Also applies to: 26-29
76-76: LGTM: STATE_DEFAULT updated to match new interfaceThe renaming of
loadPagetoloadPageByActionin theSTATE_DEFAULTobject maintains consistency with the updatedStateContextTypeinterface. This change ensures that the default state object correctly implements the new navigation method.
332-333: LGTM: Standardized initial cursor creationThe updates to the initial cursor creation when loading a file are well-aligned with the new cursor-based navigation system:
- The use of
CURSOR_CODE.EVENT_NUMstandardizes the cursor creation process.- This approach ensures that the file is loaded at the correct position when a
logEventNumis available.These changes contribute to a more consistent and robust navigation system throughout the component.
363-366: Verify the necessity of exporting STATE_DEFAULTThe addition of
STATE_DEFAULTto the named exports provides other components with access to the default state values. This can be beneficial for testing or for components that need to reset to default values.Please confirm that
STATE_DEFAULTis actually used in other parts of the application. If it's not currently used, consider whether it's necessary to export it at this time. You can use the following script to check for its usage:If there are no results, you may want to reconsider exporting
STATE_DEFAULTuntil it's actually needed.✅ Verification successful
STATE_DEFAULT is not used outside StateContextProvider.tsx
The
STATE_DEFAULTexport does not appear to be utilized in other parts of the application. Consider removing it if it's not required.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for usage of STATE_DEFAULT in the codebase # Test: Search for STATE_DEFAULT usage outside this file rg --type typescript --type tsx -g '!**/StateContextProvider.tsx' 'STATE_DEFAULT'Length of output: 116
Script:
#!/bin/bash # Description: Check for usage of STATE_DEFAULT in the codebase # Test: Search for STATE_DEFAULT usage outside this file rg --type ts --type tsx -g '!**/StateContextProvider.tsx' 'STATE_DEFAULT'Length of output: 101
Script:
#!/bin/bash # Description: Check for usage of STATE_DEFAULT in the codebase # Test: Search for STATE_DEFAULT usage in .ts and .tsx files excluding StateContextProvider.tsx rg 'STATE_DEFAULT' -g '*.ts' -g '*.tsx' -g '!new-log-viewer/src/contexts/StateContextProvider.tsx'Length of output: 98
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
new-log-viewer/src/contexts/StateContextProvider.tsx (1)
Line range hint
181-299: LGTM: Improved page state managementThe changes to the
StateContextProvidercomponent enhance the management of page-related state:
- Using a ref for
pageNumcould potentially avoid unnecessary re-renders.- The new
loadPageByActionfunction effectively encapsulates the logic for handling navigation actions.- The update to
WORKER_RESP_CODE.PAGE_DATAhandling now includes updating the URL, which maintains consistency between the UI state and the URL.These changes improve the overall structure and maintainability of the component.
Consider adding a comment explaining why
pageNumis managed as a ref instead of state, as this decision might not be immediately obvious to other developers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- new-log-viewer/src/contexts/StateContextProvider.tsx (11 hunks)
🔇 Additional comments (6)
new-log-viewer/src/contexts/StateContextProvider.tsx (6)
Line range hint
1-58: LGTM: Import statements and type definitions updated appropriatelyThe new imports and type updates align well with the changes in the component's functionality. The
StateContextTypeinterface now includespageNumas a non-nullable number, which improves type safety.
92-97: LGTM: Type-safe worker communication function addedThe
workerPostReqfunction is a well-implemented type-safe wrapper for worker communication. It ensures that the correct arguments are passed for each worker request code, which will help prevent errors and improve code maintainability.
163-170: LGTM: Simplified page loading functionThe
loadPageByCursorfunction effectively simplifies the process of loading a page by cursor. It utilizes theworkerPostReqfunction, ensuring type safety in worker communication. The function is concise and focused, which enhances maintainability.
Line range hint
357-373: LGTM: Updated filePath effect with new cursor typeThe changes to the useEffect hook for filePath align well with the new cursor management approach. The logic for determining the initial cursor when loading a file is correct and consistent with the rest of the changes in the component.
Line range hint
375-394: LGTM: Updated StateContext.Provider valueThe replacement of
loadPagewithloadPageByActionin the context value is consistent with the earlier modifications. This change ensures that the new navigation action handling is available to consumers of the context, maintaining consistency throughout the component.
Line range hint
316-354: LGTM with suggestions: Comprehensive logEventNum update handlingThe new useEffect hook for handling logEventNum updates is well-implemented overall. It includes proper checks and logic for determining whether to update the URL or load a new page.
However, there are two points that need attention:
There's a TODO comment that should be addressed:
// TODO: After filter is added, will need find the largest <= log event number on the // current page. Once found, we update the log event number in the URL instead of sending // a new request since the page has not changed.The early return when
mainWorkerRef.currentis null could potentially lead to inconsistent state iflogEventNumhas changed but couldn't be processed due to the worker not being available.Consider updating the URL even if the worker is not available:
useEffect(() => { + const clampedLogEventNum = clamp(logEventNum, 1, numEvents); + + if (clampedLogEventNum !== logEventNum) { + updateWindowUrlHashParams({ + logEventNum: clampedLogEventNum, + }); + } + if (null === mainWorkerRef.current) { return; } // ... rest of the function }, [ numEvents, logEventNum, ]);This change ensures that the URL is always updated with the clamped logEventNum, even if the worker is not available to load the new page.
To address the TODO comment, we should verify if there's an existing issue or task for implementing log filtering. Can you check the project's issue tracker?
|
new-log-viewer: Add event cursor, refactor state context and redesign load page methods to support discontinuous log event numbers in preparation for log level filtering. |
Sgtm. |
davemarco
changed the title
kirkrodrigues
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
new-log-viewer/src/contexts/StateContextProvider.tsx (3)
84-154: LGTM with a minor suggestion: Improve error loggingThe new
workerPostReqandgetPageNumCursorfunctions are well-implemented and improve type safety and navigation logic.For
getPageNumCursor, consider enhancing the error logging in the default case:default: + console.error(`Invalid navigation action: ${JSON.stringify(navAction)}`); return null;This will provide more context if an unexpected navigation action is passed.
Line range hint
181-299: LGTM with a suggestion for future improvementThe changes in the
StateContextProvidercomponent align well with the PR objectives. The newpageNumRefandloadPageByActionfunction improve the state management for pagination.However, the component is becoming quite large and complex. While this is out of scope for the current PR, consider breaking it down into smaller, more focused components or custom hooks in the future. This will improve maintainability and make the code easier to test.
For example, you could create separate hooks for handling worker responses, managing pagination, and handling file loading. This would make the main component more concise and easier to understand.
Line range hint
310-367: LGTM with a suggestion for improved clarityThe changes to the
useEffecthooks are well-implemented and improve the handling of log event numbers and file loading. The new logic for clamping log event numbers and determining when to load a new page is more robust.To improve clarity, consider adding more comments to explain the logic in the
logEventNumuseEffect, particularly around the clamping and page loading decision:useEffect(() => { if (null === mainWorkerRef.current) { return; } if (URL_HASH_PARAMS_DEFAULT.logEventNum === logEventNum) { return; } const logEventNumsOnPage: number [] = Array.from(beginLineNumToLogEventNumRef.current.values()); const clampedLogEventNum = clamp(logEventNum, 1, numEvents); + // If the clamped log event number is on the current page, update the URL if necessary if (logEventNumsOnPage.includes(clampedLogEventNum)) { if (clampedLogEventNum !== logEventNum) { updateWindowUrlHashParams({ logEventNum: clampedLogEventNum, }); } return; } + // If the log event number is not on the current page, load a new page const cursor: CursorType = { code: CURSOR_CODE.EVENT_NUM, args: {eventNum: logEventNum}, }; loadPageByCursor(mainWorkerRef.current, cursor); }, [ numEvents, logEventNum, ]);These comments will help future developers understand the logic flow more easily.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- new-log-viewer/src/contexts/StateContextProvider.tsx (11 hunks)
🔇 Additional comments (3)
new-log-viewer/src/contexts/StateContextProvider.tsx (3)
Line range hint
1-29: LGTM: Imports and type definitions updated appropriatelyThe new imports and type definitions align well with the PR objectives. The addition of
ACTION_NAME,NavigationAction, andEVENT_POSITION_ON_PAGEsuggests a more structured approach to handling navigation actions, which should improve the overall design of the log viewer.
Line range hint
374-391: LGTM: StateContext.Provider value updated correctlyThe changes to the
StateContext.Providervalue are consistent with the earlier modifications to the component and interface. The newpageNumandloadPageByActionare correctly exposed to consumers of the context, which aligns with the PR objectives.
54-58: LGTM with a suggestion: Verify pageNum handlingThe changes to
StateContextTypelook good and align with the new navigation system. However, changingpageNumfromNullable<number>tonumberassumes that a page number will always be available.Please verify that
pageNumis properly initialized and handled throughout the codebase to avoid potential issues with undefined or zero values. Run the following script to check for potential issues:
| * Submits a `LOAD_PAGE` request to a worker. | ||
| * | ||
| * @param worker | ||
| * @param code | ||
| * @param args | ||
| * @param cursor | ||
| */ | ||
| const workerPostReq = <T extends WORKER_REQ_CODE>( | ||
| const loadPageByCursor = ( | ||
| worker: Worker, | ||
| code: T, | ||
| args: WorkerReq<T> | ||
| cursor: CursorType, | ||
| ) => { | ||
| worker.postMessage({code, args}); | ||
| workerPostReq(worker, WORKER_REQ_CODE.LOAD_PAGE, { | ||
| cursor: cursor, | ||
| decoderOptions: getConfig(CONFIG_KEY.DECODER_OPTIONS), | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding error handling
The loadPageByCursor function looks good overall, but it might benefit from some error handling. Consider adding a check for null worker:
const loadPageByCursor = (
worker: Worker,
cursor: CursorType,
) => {
+ if (worker == null) {
+ console.error("Worker is null. Cannot load page.");
+ return;
+ }
workerPostReq(worker, WORKER_REQ_CODE.LOAD_PAGE, {
cursor: cursor,
decoderOptions: getConfig(CONFIG_KEY.DECODER_OPTIONS),
});
};This will prevent potential runtime errors if the function is called before the worker is initialized.
📝 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.
| * Submits a `LOAD_PAGE` request to a worker. | |
| * | |
| * @param worker | |
| * @param code | |
| * @param args | |
| * @param cursor | |
| */ | |
| const workerPostReq = <T extends WORKER_REQ_CODE>( | |
| const loadPageByCursor = ( | |
| worker: Worker, | |
| code: T, | |
| args: WorkerReq<T> | |
| cursor: CursorType, | |
| ) => { | |
| worker.postMessage({code, args}); | |
| workerPostReq(worker, WORKER_REQ_CODE.LOAD_PAGE, { | |
| cursor: cursor, | |
| decoderOptions: getConfig(CONFIG_KEY.DECODER_OPTIONS), | |
| }); | |
| /** | |
| * Submits a `LOAD_PAGE` request to a worker. | |
| * | |
| * @param worker | |
| * @param cursor | |
| */ | |
| const loadPageByCursor = ( | |
| worker: Worker, | |
| cursor: CursorType, | |
| ) => { | |
| if (worker == null) { | |
| console.error("Worker is null. Cannot load page."); | |
| return; | |
| } | |
| workerPostReq(worker, WORKER_REQ_CODE.LOAD_PAGE, { | |
| cursor: cursor, | |
| decoderOptions: getConfig(CONFIG_KEY.DECODER_OPTIONS), | |
| }); | |
| }; |
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
Description
When the user changes the log event number, the current design "chunks" the log event number to calculate new page number. The new page number is sent in a request to the backend to get the new data.
This is problematic for log level filtering as the filtered log events are sparse, and cannot be "chunked". PR #63 resolves this issue by keeping page boundaries (smallest and largest log event number on page) in the front end, and can bound the new page number using the log event number.
This PR sets up another solution where a log event cursor is sent to the backend, and the backend will calculate the correct page. The backend will reply with the page number, and the closest log event number (user may send a filtered out log event number) to the front end. The idea of this logic is to reduce clutter in the front end.
This PR does 3 things.
1. Modify state context
The page number and log event number are now set when receiving a page. This is imperative for the log event cursor to work. The front-end must be able to receive, then set, a log event number, and a new page (could be any page).
2. Modifies page load methods (next, prev, etc...)
Adds a "log event anchor" to page number cursor. The anchor allows the front-end to request if it wants the new log event number to be the top or bottom of the page.
Based on the first change (Modify state context), the returned page data includes the new log event number and page number.
In the current implementation, it is obvious what the first log event on the next page will be. However, with a filter, it is not obvious. This PR does not add any new functionality re:page changes, it simply sets up future filtering PRs.
Note, the other option with filtered log events and page requests is to use a boundary; however, the goal of log event cursor is to abandon boundaries.
3. Implement basic log event cursor.
This PR adds a basic log event cursor. It will not work with filtered logs; however, it allows this PR to be merged without reducing existing functionality.
Validation performed
Tested basic functionality.
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
handleActionfunction to streamline navigation actions.Chores