fix(filter): Clear existing log level filter when opening a new file (fixes #133). #141
Conversation
WalkthroughThe pull request introduces modifications to the Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 (3)
src/components/StatusBar/LogLevelSelect/index.tsx (2)
169-179: Consider adding error handling for the backend filter operation.While the function effectively manages both UI state and backend filtering, it would be more robust with error handling for the backend operation. This ensures graceful degradation if the backend filtering fails.
const updateFilter = useCallback((logLevels: LOG_LEVEL[]) => { setSelectedLogLevels(logLevels); // Filter logs in backend worker. - filterLogs((0 === logLevels.length ? - null : - logLevels)); + try { + filterLogs((0 === logLevels.length ? null : logLevels)); + } catch (error) { + console.error('Failed to apply log level filter:', error); + // Consider adding user feedback here + } }, [ filterLogs, setSelectedLogLevels, ]);
211-212: Consider enhancing error handling with user feedback.While the function correctly handles the undefined value case, consider providing user feedback when errors occur.
if ("undefined" === typeof currentTarget.dataset.value) { - console.error("Unexpected undefined value for \"data-value\" attribute"); + const errorMessage = "Failed to apply log level filter due to invalid selection"; + console.error(errorMessage); + // Consider using a toast notification or similar UI feedback return; }src/contexts/StateContextProvider.tsx (1)
Line range hint
456-464: Consider adding error handling for null workerThe
filterLogsimplementation silently returns when the worker is null. Consider logging an error message for debugging purposes.const filterLogs = useCallback((filter: LogLevelFilter) => { if (null === mainWorkerRef.current) { + console.error("Unexpected null mainWorkerRef.current in filterLogs"); return; } setUiState(UI_STATE.FAST_LOADING); workerPostReq(mainWorkerRef.current, WORKER_REQ_CODE.SET_FILTER, { cursor: {code: CURSOR_CODE.EVENT_NUM, args: {eventNum: logEventNumRef.current ?? 1}}, logLevelFilter: filter, }); }, []);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
src/components/StatusBar/LogLevelSelect/index.tsx(4 hunks)src/contexts/StateContextProvider.tsx(6 hunks)
🔇 Additional comments (8)
src/components/StatusBar/LogLevelSelect/index.tsx (3)
152-152: Well-structured migration to context-based state management!
The shift from local state to context management is a solid architectural choice that enables centralized control of log level filters, which is crucial for implementing the filter reset functionality when opening new files.
196-200: Clean integration with the new updateFilter function!
The changes maintain the existing functionality while properly integrating with the new filtering mechanism. The sort operation ensures consistent ordering of log levels.
214-216: Effective implementation of filter clearing!
The simplified implementation properly integrates with the new filtering mechanism and supports the PR's objective of clearing filters.
src/contexts/StateContextProvider.tsx (5)
76-76: LGTM: Interface updates align with requirements
The additions to StateContextType interface properly define the contract for managing log level filtering:
selectedLogLevelsfor state trackingfilterLogsfor applying filterssetSelectedLogLevelsfor direct state updates
Also applies to: 81-81, 85-85
105-105: LGTM: Default state initialization is correct
The default values are properly initialized:
- Empty array for
selectedLogLevels - Null functions for
filterLogsandsetSelectedLogLevels
Also applies to: 109-109, 113-113
266-267: LGTM: State management implementation
The useState hook is correctly implemented for selectedLogLevels with proper typing.
416-416: LGTM: Filter reset implementation matches requirements
The loadFile function correctly resets selectedLogLevels to default state when opening a new file, which addresses the core issue from PR #133.
556-556: LGTM: Context value updates are complete
The StateContext.Provider value object correctly includes all new properties and methods.
Also applies to: 560-560, 564-564
There was a problem hiding this comment.
Have we considered handling this completely within <LogLevelSelect/>?
We can add an useEffect hook that subscribes to uiState changes. When it is UI_STATE.FILE_LOADING, we setSelectedLogLevels([]).
If the existing useEffect(()=>setLogLevelFilter()), [..., selectedLogLevels]) hook would be triggered when we set the empty array, we can add a ref boolean variable isClearingSelectedLogLevels to temporarily supress sending the log filter update request.
|
By the way, |
I'm happy to do this. It will add an extra render since we are setting in a hook vs. at the same time we set ui state file loading, but it does allow us to not lift up SelectedLogLevels, which is cleaner.
I think the updateFilter() method I added will still work, even after the first change. The updateFilter() method gets rid of the hook entirely. I think i prefer just getting rid of the hook, but let me know if you think use effect is cleaner. |
Yes I thought it was weird as well. I changed to filterLogs() in this PR? Does that work. I tried to make it sound more like loadPage() implying it is doing something in the backend. |
|
Sounds good. I agree the |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
src/components/StatusBar/LogLevelSelect/index.tsx (3)
157-157: Consider consolidating state managementWhile the change to use
filterLogsis good, consider movingselectedLogLevelsto the context to maintain a single source of truth for all log level related state.
Line range hint
186-218: Consider enhancing error handlingThe event handlers are well-refactored to use the centralized
updateFilter. However, consider adding error boundaries or try-catch blocks around the numeric conversions to handle potential invalid values gracefully.Example enhancement for handleOptionClick:
const handleOptionClick = useCallback((ev: React.MouseEvent) => { const currentTarget = ev.currentTarget as HTMLElement; if ("undefined" === typeof currentTarget.dataset.value) { console.error("Unexpected undefined value for \"data-value\" attribute"); return; } - const selectedValue = Number(currentTarget.dataset.value); + try { + const selectedValue = Number(currentTarget.dataset.value); + if (isNaN(selectedValue)) { + throw new Error("Invalid log level value"); + } + updateFilter(range({begin: selectedValue, end: 1 + MAX_LOG_LEVEL})); + } catch (error) { + console.error("Failed to process log level selection:", error); + } - updateFilter(range({begin: selectedValue, end: 1 + MAX_LOG_LEVEL})); }, [updateFilter]);
224-229: Consider adding a cleanup function to the effectThe effect successfully addresses the filter persistence issue. However, consider adding a cleanup function to handle potential race conditions during rapid file switches.
useEffect(() => { if (UI_STATE.FILE_LOADING === uiState) { setSelectedLogLevels([]); } + return () => { + // Cancel any pending state updates if the component unmounts + // or if the effect re-runs + }; }, [uiState]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
src/components/StatusBar/LogLevelSelect/index.tsx(5 hunks)src/contexts/StateContextProvider.tsx(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/contexts/StateContextProvider.tsx
🔇 Additional comments (3)
src/components/StatusBar/LogLevelSelect/index.tsx (3)
35-38: LGTM: Clean import organization
The new imports are properly organized and necessary for the UI state management functionality.
175-185: LGTM: Well-optimized filter update logic
The updateFilter implementation efficiently combines UI state updates with backend filtering. The null check for empty log levels is a good optimization.
224-229: Verify the fix across different file loading scenarios
While the implementation looks correct, please verify these scenarios:
- Rapid file switching
- Large files that take longer to load
- Drag and drop vs. regular file opening
✅ Verification successful
Let me gather more information about file loading implementation to verify the scenarios mentioned.
Let me search for more specific information about file loading state transitions and error handling.
Based on the gathered information, I can now provide a final response about the file loading scenarios.
The implementation correctly handles file loading state transitions
The code review shows robust handling of file loading scenarios:
- The UI state transitions properly handle both regular and drag-and-drop file loading
- Multiple loading states (FILE_LOADING, SLOW_LOADING, FAST_LOADING) are managed to handle different scenarios
- The LogLevelSelect component correctly clears selected levels during FILE_LOADING state
- Drag and drop implementation includes proper disabled state handling during loading
Key findings:
- File loading state is properly managed in StateContextProvider
- Drag and drop operations are properly guarded against concurrent operations
- State transitions ensure consistent UI updates across different loading methods
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for file loading related code to ensure consistent behaviour
rg -A 5 "FILE_LOADING" --type ts
Length of output: 2366
Script:
#!/bin/bash
# Search for drag and drop related file loading
rg -A 10 "onDrop" --type ts
# Search for file size handling
rg -A 5 "fileSizeInBytes|onDiskFileSizeInBytes" --type ts
# Search for file loading error handling
rg -A 5 "catch.*file" --type ts
Length of output: 9390
Script:
#!/bin/bash
# Search for file loading state transitions
ast-grep --pattern 'setUiState(UI_STATE.$_)'
# Search for handleDrop implementation
ast-grep --pattern 'const handleDrop = $_'
# Search for error handling during file loading
rg -A 10 "catch.*{" --type ts
Length of output: 2331
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
src/components/StatusBar/LogLevelSelect/index.tsx (2)
175-185: Consider adding documentation for null conversion.The implementation is clean and efficient. However, the conversion of an empty array to null for the backend could benefit from a brief comment explaining why this is necessary.
Add a comment above line 179 explaining the null conversion:
setSelectedLogLevels(logLevels); + // Convert empty array to null as backend expects null for no filter // Filter logs in backend worker. filterLogs((0 === logLevels.length ?
224-229: LGTM! Core fix implemented efficiently.The effect correctly implements the PR's main objective by clearing the log level filter when loading a new file, without triggering unnecessary backend requests.
Would you like me to help create a test case to verify this behaviour? We could test that:
- The filter is cleared when uiState changes to FILE_LOADING
- No backend request is made during this clearing process
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/components/StatusBar/LogLevelSelect/index.tsx(5 hunks)
🔇 Additional comments (2)
src/components/StatusBar/LogLevelSelect/index.tsx (2)
35-38: LGTM! Necessary imports added for UI state handling.
The new imports are properly organized and required for the UI state management functionality.
157-157: LGTM! Improved context usage with more descriptive naming.
The replacement of setLogLevelFilter with filterLogs better represents the action being performed and aligns with the PR's objectives.
| updateFilter(newSelectedLogLevels.sort((a, b) => a - b)); | ||
| }, [ | ||
| selectedLogLevels, | ||
| updateFilter, | ||
| ]); |
There was a problem hiding this comment.
💡 Codebase verification
Sorting log levels is unnecessary and can be removed
The log level filtering is implemented through a worker interface that simply accepts an array of log levels as a filter, with no indication that order matters. The filterLogs function in StateContextProvider passes the log levels directly to the worker without any ordering requirements.
- Remove the
.sort((a, b) => a - b)from line 202 insrc/components/StatusBar/LogLevelSelect/index.tsx - The order of log levels in the filter array doesn't affect the filtering functionality
🔗 Analysis chain
Verify the necessity of sorting log levels.
The implementation looks correct, but let's verify if sorting the log levels is necessary for the backend processing.
Also applies to: 217-218, 221-221
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for log level array usage in backend processing
rg -A 5 "logLevels?.sort"
# Search for order-dependent log level processing
ast-grep --pattern 'function $_(logLevels: $_) {
$$$
for ($_ of logLevels) {
$$$
}
$$$
}'
Length of output: 131
Script:
#!/bin/bash
# Search for log level filtering or processing patterns
rg -A 5 "logLevel.*filter"
# Search for any log level array handling
rg -A 5 "LogLevel\[]"
# Look for log level type definitions and interfaces
ast-grep --pattern 'type $_ = {
$$$
logLevel$_: $_
$$$
}'
# Search for log level comparisons or ordering
rg -A 5 "LogLevel.*compare|LogLevel.*order"
Length of output: 531
Script:
#!/bin/bash
# Search for the LogLevelSelect component's interface and state handling
rg -A 10 "interface.*LogLevelSelect"
# Look for filter state updates in the context
rg -A 10 "updateFilter.*LogLevel"
# Search for log level enum or type definition
ast-grep --pattern 'enum LogLevel {
$$$
}'
# Check how log levels are used in filtering
rg -A 10 "filterLogs|applyFilter.*logLevel"
Length of output: 6574
Co-authored-by: Junhao Liao <[email protected]>
davemarco
changed the title
Description
Log level filter was persisting across files.
Moreover, the filter was not actually applied to the new file. It just remained in UI selector.
I modified so now when you open a new file, the filter is cleared. I had to take some care so that the reset does not trigger a new filtering request to the backend. The reset only clears the selected levels in the UI, it does not actually send a filtering request.
fixes #133
Validation performed
Tested levels are cleared with new file. Tested i did not break filtering.
Summary by CodeRabbit
New Features
Bug Fixes
These updates enhance user experience by providing clearer and more efficient log level controls.