fix(query): Restart log queries when case-sensitivity or regex button is toggled. #130
Conversation
|
Warning Rate limit exceeded@Henry8192 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 16 minutes and 8 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughThe changes in the Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/index.tsx (1)
61-65: Consider enhancing the query string validationWhile the implementation successfully fixes the option toggling issue, consider these improvements:
- Make the empty check more explicit
- Add string trimming to handle whitespace-only queries
- if ("" !== queryStringRef.current) { + const trimmedQuery = queryStringRef.current.trim(); + if (trimmedQuery.length > 0) { const isCaseSensitive = newOptions.includes(QUERY_OPTION.IS_CASE_SENSITIVE); const isRegex = newOptions.includes(QUERY_OPTION.IS_REGEX); - startQuery(queryStringRef.current, isRegex, isCaseSensitive); + startQuery(trimmedQuery, isRegex, isCaseSensitive); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (1)
src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/index.tsx(3 hunks)
🔇 Additional comments (3)
src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/index.tsx (3)
48-48: Good addition of queryStringRef!
The useRef hook is a suitable choice for maintaining the current query string value between renders, which is essential for the fix.
Line range hint 50-55: Effective query input handling implementation!
The function properly updates queryStringRef and maintains the existing search functionality while supporting the fix for option toggling.
Line range hint 48-65: Verify the fix resolves the original issue
The implementation appears to correctly address the core issue where queries weren't restarting when options were toggled. Let's verify this by checking for any similar patterns in the codebase that might need the same fix.
✅ Verification successful
Implementation is correct and unique to the search functionality
The implementation properly addresses the issue of restarting queries when options change. After examining the codebase, I found that:
- The only other
ToggleButtonGroupusage is inThemeSwitchToggle.tsx, which has a different purpose (theme switching) and doesn't require query persistence - The search functionality implementation is unique to the
SearchTabPanelcomponent and doesn't have similar patterns elsewhere that would need the same fix
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for similar toggle button implementations that might need the same fix
ast-grep --pattern 'ToggleButtonGroup {
$$$
onChange={$_}
$$$
}'
Length of output: 72
Script:
#!/bin/bash
# Search for similar patterns using ripgrep to find ToggleButtonGroup usage
rg -l "ToggleButtonGroup" --type tsx
# Then search for onChange handlers in those files
rg "onChange.*=.*=>" --type tsx -A 5
Length of output: 131
Script:
#!/bin/bash
# Search for ToggleButtonGroup usage in all files
rg "ToggleButtonGroup"
# Search for onChange handlers in all files
rg "onChange.*=.*=>" -A 5
Length of output: 1181
There was a problem hiding this comment.
Let's treat this task as a refactoring task as I believe with some interface redesigns, the reported issue could have been avoided.
src/typings/worker.ts -> src/typings/query.ts
- Let move all query-related types into a new file
src/typings/query.ts. - Let's extract the type of the args of
START_QUERYinto an interface namedQueryArgsand also move that intoquery.ts. - As we previously discussed,
QUERY_PROGRESS_INITshall be namedQUERY_PROGRESS_VALUE_MINandQUERY_PROGRESS_DONEshall be namedQUERY_PROGRESS_VALUE_MAXif you agree.
Change the positional arguments signatures into object parameter signatures
That will include
src/services/LogFileManager/index.ts:startQuerysrc/services/MainWorker.ts:LOG_FILE_MANAGER.startQuery(args);.- Also I don't think the arg type checks are necessary?
src/contexts/StateContextProvider.tsx:startQuery
Misc
You might need to fix the type QueryResultsType import in src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/ResultsGroup.tsx
src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/index.tsx
Outdated
Show resolved
Hide resolved
# Conflicts: # package-lock.json # src/contexts/StateContextProvider.tsx
# Conflicts: # src/contexts/StateContextProvider.tsx
@Henry8192, could we go over the items on this list that you disagree with? |
Co-authored-by: Junhao Liao <[email protected]>
…nto object parameter signatures
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
src/typings/query.ts (2)
1-2: Consider more descriptive constant namesThe constants could be more specific about their purpose in the query progress context.
-const QUERY_PROGRESS_VALUE_MIN = 0; -const QUERY_PROGRESS_VALUE_MAX = 1; +const QUERY_PROGRESS_PERCENTAGE_MIN = 0; +const QUERY_PROGRESS_PERCENTAGE_MAX = 100;
10-10: Add JSDoc comment for TextRange typeThe type would benefit from documentation explaining its purpose and the meaning of each number in the tuple.
+/** + * Represents a range of text indices in a log message. + * The tuple contains [startIndex, endIndex] where startIndex is inclusive + * and endIndex is exclusive. + */ type TextRange = [number, number];src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/index.tsx (1)
70-81: Consider debouncing query submissionsThe current implementation triggers a query on every keystroke, which could be resource-intensive for large logs. Consider adding debouncing to improve performance.
+import {debounce} from "lodash"; + const handleQuerySubmit = (newArgs: Partial<QueryArgs>) => { startQuery({ isCaseSensitive: getIsCaseSensitive(queryOptions), isRegex: getIsRegex(queryOptions), queryString: queryString, ...newArgs, }); }; +const debouncedQuerySubmit = debounce(handleQuerySubmit, 300); + const handleQueryInputChange = (ev: React.ChangeEvent<HTMLTextAreaElement>) => { setQueryString(ev.target.value); - handleQuerySubmit({queryString: ev.target.value}); + debouncedQuerySubmit({queryString: ev.target.value}); };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/index.tsx(4 hunks)src/contexts/StateContextProvider.tsx(5 hunks)src/services/LogFileManager/index.ts(2 hunks)src/services/MainWorker.ts(2 hunks)src/typings/query.ts(1 hunks)src/typings/worker.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/contexts/StateContextProvider.tsx
- src/typings/worker.ts
🔇 Additional comments (4)
src/services/MainWorker.ts (1)
127-127: Consider adding runtime type validation for query arguments
The removal of type checks before calling startQuery could lead to runtime issues if the args object is malformed. While TypeScript provides compile-time type safety, consider adding runtime validation for critical parameters.
src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/index.tsx (1)
41-58: Well-implemented utility functions following DRY principles
The implementation follows the suggested refactoring and improves code maintainability.
src/services/LogFileManager/index.ts (2)
9-12: LGTM: Good separation of query-related types
The relocation of query-related types to a dedicated query.ts file improves code organization and follows the single responsibility principle.
292-297: LGTM: Improved method signature using object parameters
The change to use a single QueryArgs object parameter improves maintainability and makes the method signature more flexible for future additions.
Co-authored-by: Junhao Liao <[email protected]>
Co-authored-by: Junhao Liao <[email protected]>
Henry8192
changed the title
Henry8192
changed the title
Description
Fixes #126.
Also updates
package-locks.jsonfor vulnerable packages.Validation performed
Search "Abc" without enabling any of the query options. Enable "caseSensitive" button, that restarts the search.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation