new-log-viewer: Add log query support in StateContextProvider.
#80
Conversation
WalkthroughThe pull request introduces enhancements to the log management system by adding a 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 (
|
|
The design was discussed offline. Some key ideas:
Let's get those implemented and we can discuss further. I can help with the UI implementations once #74 is merged. |
…to match naming, clean up jsdoc
Henry8192
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Outside diff range and nitpick comments (4)
new-log-viewer/src/utils/time.ts (1)
1-4: Improve function documentationThe current documentation is incomplete. Consider enhancing it as follows:
- Add a description of the function's purpose and behaviour.
- Complete the
@paramtag with details about thecallbackparameter.- Include a
@returnstag to explicitly state that the function doesn't return a value.Here's a suggested improvement:
/** * Defers the execution of a callback function to the next event loop cycle. * * @param callback - The function to be executed in the next event loop cycle. * @returns {void} */new-log-viewer/src/components/MenuBar/index.tsx (1)
35-35: LGTM: Search feature added to MenuBarThe changes to add the search feature to the
MenuBarcomponent look good:
- The
queryLogsfunction is correctly added to the destructured context.- The new search button is logically placed at the end of the menu bar.
- The
onClickhandler for the new button correctly uses thequeryLogsfunction.- The button uses the
SmallIconButtoncomponent, maintaining consistency with other buttons in the menu bar.Consider adding an
aria-labelto the new button to improve accessibility. For example:<SmallIconButton onClick={queryLogs} + aria-label="Search logs" > <SearchIcon/> </SmallIconButton>Also applies to: 82-86
new-log-viewer/src/typings/worker.ts (1)
76-82: Consider exportingChunkResultTypefor external useWhile
ChunkResultsis exported,ChunkResultTypemight also need to be exported if it's used elsewhere in the application. This ensures that all necessary types are accessible where required.You can modify the export statement as follows:
export type { BeginLineNumToLogEventNumMap, ChunkResults, + ChunkResultType, CursorType, FileSrcType, MainWorkerReqMessage, MainWorkerRespMessage, WorkerReq, WorkerResp, };new-log-viewer/src/services/MainWorker.ts (1)
46-48: Ensure non-blocking query processing and result aggregationTo improve responsiveness, consider processing query tasks non-blockingly by submitting them one-by-one using
setTimeout()or a similar mechanism. This allows the renderer to handle incoming requests between task executions. Also, ensure that the rendering component aggregates all results upon receiving each chunk to keep the user interface updated.Would you like assistance in refactoring the query implementation to align with these design recommendations?
Also applies to: 107-116
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
- new-log-viewer/src/components/MenuBar/index.tsx (3 hunks)
- new-log-viewer/src/contexts/StateContextProvider.tsx (6 hunks)
- new-log-viewer/src/services/LogFileManager.ts (7 hunks)
- new-log-viewer/src/services/MainWorker.ts (4 hunks)
- new-log-viewer/src/typings/worker.ts (4 hunks)
- new-log-viewer/src/utils/time.ts (1 hunks)
🧰 Additional context used
🪛 Biome
new-log-viewer/src/services/LogFileManager.ts
[error] 300-300: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (10)
new-log-viewer/src/utils/time.ts (2)
5-7: Implementation looks goodThe
deferfunction is well-implemented:
- It correctly uses
setTimeoutwith a 0ms delay to defer execution.- The function name accurately describes its purpose.
- The type annotation for the callback parameter is correct.
9-9: Export statement is correctThe
deferfunction is properly exported, allowing it to be imported and used in other modules.new-log-viewer/src/components/MenuBar/index.tsx (2)
15-15: LGTM: New import for SearchIconThe import for
SearchIconis correctly placed and necessary for the new search feature being added to theMenuBarcomponent.
35-35: Verify implementation of suggested query optimizationsThe changes in this file align with the PR objectives by adding the search feature to the
MenuBar. However, to ensure full compliance with the suggestions made by junhaoliao in the PR comments, we should verify the implementation of:
- The task submission strategy using
setTimeout()instead of a blocking while-loop.- The result aggregation in the rendering component.
To verify these implementations, we need to check the
queryLogsfunction and the component that handles the search results. Run the following script to locate these implementations:Please review the results of this script to ensure that the suggested optimizations have been implemented.
Also applies to: 82-86
new-log-viewer/src/typings/worker.ts (2)
44-44: Proper addition ofQUERY_LOGto request codesThe
QUERY_LOGentry is correctly added to theWORKER_REQ_CODEenum, maintaining alignment with existing naming conventions and structure.
51-52: Correct inclusion ofPAGE_DATAandCHUNK_RESULTin response codesThe new response codes
PAGE_DATAandCHUNK_RESULTare appropriately added to theWORKER_RESP_CODEenum, ensuring consistent communication protocols.new-log-viewer/src/services/MainWorker.ts (2)
46-48: FunctionchunkResultsHandleris correctly implementedThe
chunkResultsHandlerfunction properly posts the chunk results to the renderer. The implementation aligns with the expected behaviour.
81-82: Verify thatLogFileManager.createacceptschunkResultsHandlerEnsure that the
LogFileManager.createmethod is updated to acceptchunkResultsHandleras the fourth parameter. This is important to avoid runtime errors if the method signature does not match.Run the following script to confirm the method signature:
new-log-viewer/src/services/LogFileManager.ts (1)
260-279: Efficient implementation of asynchronous log searchGood work implementing
startQueryand using asynchronous chunk processing withdeferin#searchChunk. This aligns with the task submission strategy suggested to enhance responsiveness and allows the rendering process to handle incoming requests between tasks.new-log-viewer/src/contexts/StateContextProvider.tsx (1)
365-365: EnsurequeryLogsis properly passed to context consumersAdding
queryLogsto theStateContext.Providervalue allows context consumers to access this method. Ensure that components consuming this context are updated to utilize thequeryLogsfunction as needed.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
new-log-viewer/src/services/LogFileManager.ts (1)
Line range hint
58-88: LGTM: New fields and constructor updatesThe new private fields and constructor updates are well-aligned with the new query functionality. However, there's a minor optimization opportunity:
Consider removing the initial value of
#numEvents:- readonly #numEvents: number = 0; + readonly #numEvents: number;This field is set in the constructor, so the initial value of 0 is unnecessary.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- new-log-viewer/src/services/LogFileManager.ts (7 hunks)
- new-log-viewer/src/services/MainWorker.ts (4 hunks)
- new-log-viewer/src/typings/worker.ts (4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- new-log-viewer/src/services/MainWorker.ts
- new-log-viewer/src/typings/worker.ts
🧰 Additional context used
🔇 Additional comments (4)
new-log-viewer/src/services/LogFileManager.ts (4)
11-19: LGTM: New imports added for query functionalityThe new imports for
ChunkResultsanddeferare appropriate for the added query functionality.
115-127: LGTM: Updated create methodThe
createmethod has been appropriately updated to include thechunkResultsHandlerparameter, maintaining consistency with the constructor changes.
171-173: LGTM: New incrementQueryId methodThe
incrementQueryIdmethod is a straightforward and effective way to manage query IDs.
252-322: LGTM: New query functionality implementationThe new
startQueryand#searchChunkmethods effectively implement the log querying functionality as discussed in the PR comments. The use ofdeferin#searchChunkensures non-blocking execution, allowing for better responsiveness.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
new-log-viewer/src/components/MenuBar/index.tsx (1)
86-90: Enhance user feedback and accessibility for the search buttonThe search button is correctly implemented, but could benefit from some enhancements:
- Add a tooltip to explain the button's function.
- Include an aria-label for better accessibility.
- Consider adding visual feedback when a search is in progress.
Here's a suggested improvement:
<SmallIconButton onClick={handleSearchButtonClick} title="Search Logs" aria-label="Search Logs" disabled={isLoading} > {isLoading ? <CircularProgress size={24} /> : <SearchIcon />} </SmallIconButton>This change adds a tooltip, improves accessibility, and provides visual feedback during the search process.
new-log-viewer/src/contexts/StateContextProvider.tsx (1)
218-222: Simplify object property assignment using shorthand notationIn the
queryLogsfunction, you can use the object property shorthand to make the code more concise since the property names and variables are the same.Apply this diff to simplify the code:
workerPostReq(mainWorkerRef.current, WORKER_REQ_CODE.QUERY_LOG, { - searchString: searchString, - isRegex: isRegex, - isCaseSensitive: isCaseSensitive, + searchString, + isRegex, + isCaseSensitive, });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- new-log-viewer/src/components/MenuBar/index.tsx (3 hunks)
- new-log-viewer/src/contexts/StateContextProvider.tsx (7 hunks)
🧰 Additional context used
🔇 Additional comments (3)
new-log-viewer/src/components/MenuBar/index.tsx (2)
15-15: LGTM: SearchIcon import added correctlyThe import of SearchIcon is properly placed with other icon imports and is used in the component.
Line range hint
1-101: Overall assessment: Good implementation with room for improvementThe changes successfully implement the log query feature as described in the PR objectives. The code is well-structured and consistent with the existing codebase. However, there are opportunities to enhance the user experience and flexibility of the search feature:
- Parameterize the search function to allow for more dynamic queries.
- Improve error handling and user feedback during the search process.
- Enhance accessibility and provide more context for the search button.
These improvements would make the feature more robust and user-friendly.
new-log-viewer/src/contexts/StateContextProvider.tsx (1)
208-223:queryLogsfunction implementation looks goodThe
queryLogsfunction is correctly implemented usinguseCallbackand appropriately sends the request to the worker with the provided parameters.
Co-authored-by: Junhao Liao <junhao@junhao.ca>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (6)
new-log-viewer/src/utils/time.ts (1)
6-8: The implementation looks good. Consider adding a type check for robustness.The
deferfunction correctly usessetTimeoutto defer the execution of the callback function. The implementation is concise and effective.For added robustness, you might consider adding a type check to ensure
callbackFnis a function:const defer = (callbackFn: () => void) => { + if (typeof callbackFn !== 'function') { + throw new TypeError('callbackFn must be a function'); + } setTimeout(callbackFn, 0); };This change would provide clearer error messages if
deferis called with an invalid argument.new-log-viewer/src/services/LogFileManager/index.ts (5)
15-24: LGTM! Good use of imports and constants.The new imports and constants are well-organized and relevant to the added functionality. Using
SEARCH_CHUNK_SIZEfrom a config file is a good practice for maintainability.Consider adding a brief comment explaining the purpose of
SEARCH_CHUNK_SIZEin the config file for better documentation.
Line range hint
46-125: LGTM! Constructor and create method updated correctly.The changes to the constructor and
createmethod are well-implemented and consistent with the new query functionality. The use of private fields and the callback pattern for query results is appropriate.However, there's a minor documentation issue:
Please update the JSDoc for the constructor to include the
@paramforonQueryResults:/** * Private constructor for LogFileManager. This is not intended to be invoked publicly. * Instead, use LogFileManager.create() to create a new instance of the class. * * @param params * @param params.decoder * @param params.fileName * @param params.onDiskFileSizeInBytes * @param params.pageSize Page size for setting up pagination. * @param params.onQueryResults Callback function to handle query results. */🧰 Tools
279-305: LGTM! Well-implemented startQuery method.The
startQuerymethod is well-structured and handles different cases appropriately. The use of#queryIdto manage ongoing searches is a good practice.For improved readability, consider extracting the regex construction into a separate private method:
private constructSearchRegex(searchString: string, isRegex: boolean, isCaseSensitive: boolean): RegExp { const regexPattern = isRegex ? searchString : searchString.replace(/[.*+?^${}()|[\]\\]/g, "\\$&"); const regexFlags = isCaseSensitive ? "" : "i"; return new RegExp(regexPattern, regexFlags); }Then use it in
startQuery:const searchRegex = this.constructSearchRegex(searchString, isRegex, isCaseSensitive);This change would make the
startQuerymethod more concise and easier to understand.
307-355: LGTM! Well-implemented chunk-based search, but consider using setTimeout.The
#searchChunkAndScheduleNextmethod effectively implements chunk-based searching, which is great for handling large log files without blocking the main thread. The use ofqueryIdto manage ongoing searches is a good practice.However, consider replacing
deferwithsetTimeoutfor scheduling the next chunk search.setTimeoutis more widely supported and its behaviour is more predictable across different JavaScript environments:if (endSearchIdx < this.#numEvents) { setTimeout(() => { this.#searchChunkAndScheduleNext(queryId, endSearchIdx, searchRegex); }, 0); }This change will maintain the same non-blocking behaviour while using a more standard JavaScript function.
54-54: Add missing JSDoc @param for onQueryResults.To complete the documentation, please add the missing JSDoc @param for
params.onQueryResultsin the constructor:/** * Private constructor for LogFileManager. This is not intended to be invoked publicly. * Instead, use LogFileManager.create() to create a new instance of the class. * * @param params * @param params.decoder * @param params.fileName * @param params.onDiskFileSizeInBytes * @param params.pageSize Page size for setting up pagination. * @param params.onQueryResults Callback function to handle query results. */This will ensure that all parameters are properly documented, improving code maintainability.
🧰 Tools
🪛 GitHub Check: lint-check
[warning] 54-54:
Missing JSDoc @param "params.onQueryResults" declaration
📜 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/MainWorker.ts (4 hunks)
- new-log-viewer/src/utils/time.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- new-log-viewer/src/services/MainWorker.ts
🧰 Additional context used
📓 Learnings (1)
new-log-viewer/src/services/LogFileManager/index.ts (2)
Learnt from: Henry8192 PR: y-scope/yscope-log-viewer#80 File: new-log-viewer/src/services/LogFileManager/index.ts:305-321 Timestamp: 2024-10-10T20:26:31.573Z Learning: When a function like `#searchChunkAndScheduleNext` requires significant improvements, it's preferred to rewrite the entire function rather than making incremental changes.Learnt from: Henry8192 PR: y-scope/yscope-log-viewer#80 File: new-log-viewer/src/services/LogFileManager.ts:297-313 Timestamp: 2024-10-09T02:08:56.053Z Learning: In the `#searchChunk` method of `LogFileManager.ts`, checking `typeof match.index === "number"` correctly handles matches at index 0, ensuring matches at the beginning of the string are not skipped.
🔇 Additional comments (1)
new-log-viewer/src/utils/time.ts (1)
1-5: LGTM! The documentation is clear and concise.The JSDoc comment effectively describes the purpose of the
deferfunction and its parameter. The parameter namecallbackFnis consistent with the function signature, addressing a previous review comment.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
new-log-viewer/src/services/LogFileManager/index.ts (3)
40-50: LGTM! Good additions for query functionality.The changes to the class properties are well-aligned with the new query functionality. Making
#fileNamereadonly is a good practice for immutability. The new properties#queryIdand#onQueryResultsare necessary for managing queries.Consider using a more descriptive name for
#queryId, such as#currentQueryIdor#latestQueryId, to better convey its purpose.
280-306: LGTM! Well-implemented query initiation logic.The
startQuerymethod effectively implements the logic for starting a new query. It properly handles empty query strings and the case of no logs. The RegExp construction looks correct and accounts for both regex and non-regex queries.Consider moving the
this.#queryId++increment to the beginning of the method to ensure it's always incremented, even for empty queries. This would prevent potential issues with query ID management.
308-356: LGTM! Well-implemented chunk querying logic, with room for improvements.The
#queryChunkAndScheduleNextmethod effectively implements the core logic for querying log chunks. It properly handles query cancellation and chunk size using theQUERY_CHUNK_SIZEconstant.Consider the following improvements:
- Add explicit error handling for the
decodeRangecall to handle potential decoding failures.- Replace
deferwithsetTimeoutfor better cross-browser compatibility.- Simplify the nested ternary operator in the
matchResultcheck for better readability.Example for point 3:
if (null !== matchResult && "number" === typeof matchResult.index) { // Rest of the code }new-log-viewer/src/contexts/StateContextProvider.tsx (1)
303-318: LGTM! Consider clearing existing query results before starting a new queryThe implementation of
startQuerylooks good and correctly handles the case wheremainWorkerRef.currentis null. However, it might be beneficial to clear any existing query results before starting a new query to ensure a clean slate for each search.Consider adding the following line at the beginning of the function:
setQueryResults(new Map());This will reset the query results before initiating a new search, preventing any potential confusion with results from previous queries.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
- new-log-viewer/src/contexts/StateContextProvider.tsx (6 hunks)
- new-log-viewer/src/index.tsx (1 hunks)
- new-log-viewer/src/services/LogFileManager/index.ts (5 hunks)
- new-log-viewer/src/services/MainWorker.ts (4 hunks)
- new-log-viewer/src/typings/worker.ts (4 hunks)
- new-log-viewer/src/utils/config.ts (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- new-log-viewer/src/index.tsx
- new-log-viewer/src/services/MainWorker.ts
- new-log-viewer/src/typings/worker.ts
- new-log-viewer/src/utils/config.ts
🧰 Additional context used
📓 Learnings (1)
new-log-viewer/src/services/LogFileManager/index.ts (1)
Learnt from: Henry8192 PR: y-scope/yscope-log-viewer#80 File: new-log-viewer/src/services/LogFileManager/index.ts:305-321 Timestamp: 2024-10-10T20:26:31.573Z Learning: When a function like `#searchChunkAndScheduleNext` requires significant improvements, it's preferred to rewrite the entire function rather than making incremental changes.
🔇 Additional comments (6)
new-log-viewer/src/services/LogFileManager/index.ts (5)
15-22: LGTM! Good practice for importing configuration constants.The addition of
QueryResultsandQUERY_CHUNK_SIZEimports is appropriate for the new query functionality. ImportingQUERY_CHUNK_SIZEfrom a separate config file is a good practice for maintainability.
58-76: LGTM! Constructor updated correctly for query functionality.The addition of the
onQueryResultsparameter to the constructor and its assignment to the class property is consistent with the new query functionality. The JSDoc comment is properly updated to include the new parameter.
107-126: LGTM! Create method updated correctly.The
createmethod has been properly updated to include the newonQueryResultsparameter and pass it to the constructor. The JSDoc comment is also updated to reflect this change.
Line range hint
1-391: LGTM! Well-implemented query functionality with minor suggestions for improvement.The changes introduce a robust query feature to the LogFileManager class while maintaining the existing structure. The implementation is solid overall, with clear separation of concerns and good use of TypeScript features.
Summary of suggestions:
- Consider renaming
#queryIdto#currentQueryIdor#latestQueryIdfor clarity.- Move the
this.#queryId++increment instartQueryto ensure it's always incremented.- Improve error handling in
#queryChunkAndScheduleNextfordecodeRange.- Replace
deferwithsetTimeoutfor better cross-browser compatibility.- Simplify the nested ternary operator in the
matchResultcheck.These minor improvements aside, the pull request looks good and ready to be merged after addressing the integration verification points mentioned earlier.
Line range hint
1-391: Verify integration with UI and other components.The new query functionality looks well-implemented and self-contained. However, it's important to ensure that it's properly integrated with the rest of the application.
Please verify:
- The UI components correctly call the
startQuerymethod with appropriate parameters.- The
onQueryResultscallback is properly handled in the parent component to update the UI with search results.- The query functionality doesn't negatively impact the performance of other features, especially for large log files.
new-log-viewer/src/contexts/StateContextProvider.tsx (1)
Line range hint
1-487: Overall, the changes implement query functionality well with room for minor improvementsThe additions to the
StateContextProvidersuccessfully implement new query functionality. The changes are consistent across the interface definition, state initialization, and context provider. ThestartQueryfunction and the handling of query results are implemented correctly.There are a few areas for potential improvement:
- The
QUERY_RESULTcase inhandleMainWorkerRespcould be refactored for better readability and robustness.- Consider clearing existing query results before starting a new query in the
startQueryfunction.These minor adjustments would enhance the overall quality and maintainability of the code.
| case WORKER_RESP_CODE.QUERY_RESULT: | ||
| setQueryResults((v) => { | ||
| args.results.forEach((resultsPerPage, queryPageNum) => { | ||
| if (false === v.has(queryPageNum)) { | ||
| v.set(queryPageNum, []); | ||
| } | ||
| v.get(queryPageNum)?.push(...resultsPerPage); | ||
| }); | ||
|
|
||
| return v; | ||
| }); | ||
| break; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider refactoring the QUERY_RESULT case for clarity and robustness
The implementation of the QUERY_RESULT case is functional, but there are opportunities for improvement:
- Use a more descriptive variable name instead of
vfor better readability. - Consider simplifying the state update logic using
Mapmethods. - Add error handling for potential type mismatches or unexpected data structures.
Here's a suggested refactor:
case WORKER_RESP_CODE.QUERY_RESULT:
setQueryResults((prevResults) => {
const newResults = new Map(prevResults);
args.results.forEach((resultsPerPage, queryPageNum) => {
if (typeof queryPageNum !== 'number' || !Array.isArray(resultsPerPage)) {
console.error(`Invalid data structure for page ${queryPageNum}`);
return;
}
const existingResults = newResults.get(queryPageNum) || [];
newResults.set(queryPageNum, [...existingResults, ...resultsPerPage]);
});
return newResults;
});
break;This refactored version improves readability, simplifies the logic, and adds basic error checking.
Co-authored-by: Junhao Liao <junhao@junhao.ca>
There was a problem hiding this comment.
I verified the behaviour with a simple query button added in the menubar which invokes the startQuery handler provided by the state context provider, and the result look expected.
For the PR title, how about:
new-log-viewer: Add log query support in `StateContextProvider`.
StateContextProvider.
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
Description
Add
queryLogsinstateContextProvider.tsx, which posts a worker request codeQUERY_LOG.In
MainWorker.ts,QUERY_LOGtriggers.startQuery()of LOG_FILE_MANAGER.startQuerythen initializes the searchString using regex, and calls#searchChunk.searchChunksearches logs bySEARCH_CHUNK_SIZE, and sends results withchunkResultsHandler, which posts a worker respond codeCHUNK_RESULT.Validation performed
Click the search button, which triggers a query for keyword "scheduled". Observe the results printed out in console matches with original logs.
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Documentation