feat: Add KQL query support for filtering structured IR log events (fixes #277). #371
Conversation
WalkthroughAdds KQL-aware filtering across UI, stores, URL hash, and decoder layers. Introduces a dedicated KQL filter input, updates hash params (query/subquery), aligns page reload/query restart after filtering, extends decoder APIs to accept kqlFilter, and updates state/store types to track fileTypeInfo and KQL filter values. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant SearchTab as SearchTabPanel
participant ViewStore as ViewStore (filter slice)
participant Proxy as LogFileManagerProxy
participant Manager as LogFileManager
participant Decoder as Decoder
participant UI as QueryStore
rect #EEF5FF
note right of SearchTab: Apply KQL filter (button or Shift+Enter)
User->>SearchTab: Enter KQL + trigger
SearchTab->>ViewStore: setKqlFilterInput()/setKqlFilter()
SearchTab->>ViewStore: filterLogs()
end
ViewStore->>Proxy: setFilter(logLevelFilter, kqlFilter)
Proxy->>Manager: setLogLevelFilter(logLevelFilter, kqlFilter)
Manager->>Decoder: setLogLevelFilter(logLevelFilter, kqlFilter)
Decoder-->>Manager: success / error
Manager-->>Proxy: void
ViewStore->>ViewStore: updateWindowUrlHashParams({query: kqlFilter})
rect #F0FFF0
note right of UI: Post-filter refresh
ViewStore->>Manager: loadPageByCursor({EVENT_NUM, eventNum})
ViewStore->>UI: startQuery()
end
alt Decoder error
Decoder-->>ViewStore: throw
ViewStore->>UI: handleErrorWithNotification
end
sequenceDiagram
autonumber
participant App as AppController
participant URL as urlHash utils
participant ViewStore as ViewStore
participant Manager as LogFileManager
participant UI as QueryStore
rect #FFF6EB
App->>URL: getWindowUrlHashParams()
App->>URL: handleHashChange()/updateViewHashParams()
URL->>ViewStore: setKqlFilter(query) if changed
URL->>ViewStore: filterLogs()
end
URL->>URL: getCursorFromHashParams()
alt valid cursor
URL->>Manager: loadPageByCursor(cursor)
else no cursor
URL-->>App: use LAST_EVENT cursor
end
opt subquery present
URL->>ViewStore: setQueryString(subquery)
URL->>UI: startQuery()
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
hoophalab
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: 27
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
src/services/decoders/utils.ts (1)
30-41: Update JSDoc to reflect new utcOffsetKey contractThe doc still says “log level key and timestamp key”, but the function now returns
utcOffsetKeytoo. Please update the comment to prevent drift.- * Parses the log level key and timestamp key from the decoder options. + * Parses the log level, timestamp, and UTC offset keys from the decoder options.src/stores/logFileStore.ts (2)
35-39: ResetfileTypeInfoandnumEventsduring load to prevent stale UI stateWhen starting a new load, we don’t reset
fileTypeInfoandnumEvents. This can incorrectly keep KQL UI enabled (based on previous file) and display stale counts until the new file finishes loading.Apply this diff to reset both fields when initiating a load:
set({ fileName: "Loading...", fileSrc: fileSrc, metadata: LOG_FILE_STORE_DEFAULT.metadata, onDiskFileSizeInBytes: LOG_FILE_STORE_DEFAULT.onDiskFileSizeInBytes, + fileTypeInfo: LOG_FILE_STORE_DEFAULT.fileTypeInfo, + numEvents: LOG_FILE_STORE_DEFAULT.numEvents, });Also applies to: 47-54, 118-124
152-152: Possible null-dereference offileInfo.fileTypeInfo
fileTypeInfois nullable in state, but we unconditionally accessfileInfo.fileTypeInfo.isStructured. If the loader ever returnsnull(e.g., unknown/unsupported type), this will throw.Guard the dereference:
- if (0 === decoderOptions.formatString.length && fileInfo.fileTypeInfo.isStructured) { + if ( + 0 === decoderOptions.formatString.length && + null !== fileInfo.fileTypeInfo && + true === fileInfo.fileTypeInfo.isStructured + ) { const {postPopUp} = useNotificationStore.getState(); postPopUp(FORMAT_POP_UP_MESSAGE); }Also applies to: 157-161
src/components/AppController.tsx (1)
46-53: Potential duplicate query triggers
filterLogs()already callsstartQuery()(per the store slice). Here we callstartQuery()again whenupdateQueryHashParams()is true, and we callhandleHashChange()immediately afterloadPageByCursor()on initial load. Depending on timing, the same query may start twice.
- Option A: Let
filterLogs()own query starting for filter-driven changes; restrictupdateQueryHashParams()branch to non-filter query params.- Option B: Make
filterLogs()not callstartQuery()and centralize query starting here.If you want, I can prepare a focused refactor patch after you confirm the preferred ownership.
Also applies to: 122-123
src/services/LogFileManager/index.ts (1)
61-63: Update UI call tosetLogLevelFilterwith the newkqlFilterparameterThe decoder interface, all implementations (JsonlDecoder, ClpIrDecoder) and backend call sites (LogFileManager, LogFileManagerProxy) already use the two-argument signature
setLogLevelFilter(logLevelFilter, kqlFilter). However, in your status-bar component you’re still invoking it with a single argument, which will passundefinedforkqlFilterand break the new arity.Please update:
- File:
src/components/StatusBar/LogLevelSelect/index.tsx
Lines: ~180–183
Before:After (example):// only passing the LogLevelFilter array (or null) setLogLevelFilter( 0 === logLevels.length ? null : logLevels ); filterLogs();// now passing both filters; use empty string or stored KQL setLogLevelFilter( 0 === logLevels.length ? null : logLevels, '' // or your current KQL filter ); filterLogs();Remaining decoder implementations and call sites already conform to the new signature. Once the UI invocation is updated, all calls will have the correct arity and runtime errors will be avoided.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (20)
package.json(1 hunks)src/components/AppController.tsx(2 hunks)src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/FilterInputBox.css(1 hunks)src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/FilterInputBox.tsx(1 hunks)src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/QueryInputBox.css(1 hunks)src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/index.css(1 hunks)src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/index.tsx(5 hunks)src/components/StatusBar/LogLevelSelect/index.tsx(1 hunks)src/services/LogFileManager/index.ts(1 hunks)src/services/LogFileManagerProxy.ts(1 hunks)src/services/decoders/ClpIrDecoder/index.ts(1 hunks)src/services/decoders/JsonlDecoder/index.ts(2 hunks)src/services/decoders/utils.ts(2 hunks)src/stores/logFileStore.ts(3 hunks)src/stores/viewStore/createViewFilterSlice.ts(1 hunks)src/stores/viewStore/types.ts(1 hunks)src/typings/decoders.ts(1 hunks)src/typings/tab.ts(1 hunks)src/typings/url.ts(2 hunks)src/utils/url/index.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}
⚙️ CodeRabbit configuration file
- Prefer
false == <expression>rather than!<expression>.
Files:
src/typings/tab.tssrc/services/decoders/ClpIrDecoder/index.tssrc/components/AppController.tsxsrc/components/StatusBar/LogLevelSelect/index.tsxsrc/services/decoders/utils.tssrc/typings/url.tssrc/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/index.tsxsrc/utils/url/index.tssrc/services/decoders/JsonlDecoder/index.tssrc/stores/logFileStore.tssrc/typings/decoders.tssrc/services/LogFileManagerProxy.tssrc/services/LogFileManager/index.tssrc/stores/viewStore/types.tssrc/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/FilterInputBox.tsxsrc/stores/viewStore/createViewFilterSlice.ts
🧠 Learnings (8)
📚 Learning: 2024-11-14T04:35:00.985Z
Learnt from: junhaoliao
PR: y-scope/yscope-log-viewer#107
File: src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/index.css:14-18
Timestamp: 2024-11-14T04:35:00.985Z
Learning: In `src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/index.css`, the use of `!important` flags is necessary for overriding styles in the `.query-input-box` class.
Applied to files:
src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/FilterInputBox.csssrc/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/index.csssrc/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/QueryInputBox.csssrc/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/index.tsxsrc/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/FilterInputBox.tsx
📚 Learning: 2024-10-08T15:52:50.753Z
Learnt from: junhaoliao
PR: y-scope/yscope-log-viewer#77
File: new-log-viewer/src/components/StatusBar/LogLevelSelect/index.css:6-8
Timestamp: 2024-10-08T15:52:50.753Z
Learning: In this codebase, using `!important` is acceptable when overriding styles from JoyUI components.
Applied to files:
src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/FilterInputBox.css
📚 Learning: 2024-10-08T15:52:50.753Z
Learnt from: junhaoliao
PR: y-scope/yscope-log-viewer#77
File: new-log-viewer/src/components/StatusBar/LogLevelSelect/LogLevelChip.css:6-6
Timestamp: 2024-10-08T15:52:50.753Z
Learning: When overriding JoyUI's default styling, it's acceptable to use `!important` in CSS in this project.
Applied to files:
src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/FilterInputBox.css
📚 Learning: 2024-09-28T01:00:02.202Z
Learnt from: junhaoliao
PR: y-scope/yscope-log-viewer#74
File: new-log-viewer/src/components/CentralContainer/PanelTabs/TooltipTab.css:0-0
Timestamp: 2024-09-28T01:00:02.202Z
Learning: Using `!important` is acceptable when overriding JoyUI styles in this project.
Applied to files:
src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/FilterInputBox.css
📚 Learning: 2024-10-08T15:52:50.753Z
Learnt from: junhaoliao
PR: y-scope/yscope-log-viewer#74
File: new-log-viewer/src/components/CentralContainer/PanelTabs/TooltipTab.css:0-0
Timestamp: 2024-10-08T15:52:50.753Z
Learning: When overriding JoyUI styles, `!important` may be necessary to ensure custom styles take precedence.
Applied to files:
src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/FilterInputBox.css
📚 Learning: 2025-06-01T13:41:12.938Z
Learnt from: zzxthehappiest
PR: y-scope/yscope-log-viewer#286
File: src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/QueryInputBox.tsx:37-40
Timestamp: 2025-06-01T13:41:12.938Z
Learning: The `updateWindowUrlHashParams` function in `src/utils/url.ts` doesn't throw errors, so error handling is not needed when calling this function.
Applied to files:
src/components/AppController.tsx
📚 Learning: 2025-06-01T13:40:12.222Z
Learnt from: zzxthehappiest
PR: y-scope/yscope-log-viewer#286
File: src/components/Editor/index.tsx:138-139
Timestamp: 2025-06-01T13:40:12.222Z
Learning: In the yscope-log-viewer codebase, when using Zustand stores in React components, the preferred pattern is to use `getState()` for static setters that never change (like `setLogEventNum`) to avoid unnecessary subscriptions, while using hooks for actions that do more than just setting values. All store state variables should be declared at the beginning of the component for consistency and clear dependency overview.
Applied to files:
src/components/StatusBar/LogLevelSelect/index.tsx
📚 Learning: 2024-10-28T18:40:29.816Z
Learnt from: Henry8192
PR: y-scope/yscope-log-viewer#107
File: src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/Result.tsx:1-7
Timestamp: 2024-10-28T18:40:29.816Z
Learning: In the y-scope/yscope-log-viewer project, importing components via destructuring from 'mui/joy' is acceptable, as the package size impact has been analyzed and found acceptable.
Applied to files:
src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/index.tsx
🧬 Code graph analysis (14)
src/services/decoders/ClpIrDecoder/index.ts (1)
src/services/decoders/JsonlDecoder/index.ts (1)
logLevelFilter(229-243)
src/components/AppController.tsx (1)
src/utils/url/index.ts (2)
getWindowUrlHashParams(383-383)updateWindowUrlHashParams(389-389)
src/components/StatusBar/LogLevelSelect/index.tsx (3)
src/services/LogFileManager/index.ts (1)
setLogLevelFilter(193-198)src/services/decoders/ClpIrDecoder/index.ts (1)
setLogLevelFilter(102-111)src/services/decoders/JsonlDecoder/index.ts (1)
setLogLevelFilter(89-96)
src/services/decoders/utils.ts (1)
src/typings/formatters.ts (1)
ParsedKey(127-127)
src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/index.tsx (2)
src/services/LogFileManager/index.ts (1)
fileTypeInfo(119-121)src/utils/url/index.ts (2)
copyPermalinkToClipboard(380-380)URL_HASH_PARAMS_DEFAULT(391-391)
src/utils/url/index.ts (1)
src/typings/url.ts (1)
HASH_PARAM_NAMES(41-41)
src/services/decoders/JsonlDecoder/index.ts (1)
src/typings/logs.ts (1)
LogLevelFilter(51-51)
src/stores/logFileStore.ts (2)
src/typings/common.ts (1)
Nullable(6-6)src/typings/file.ts (1)
FileTypeInfo(54-54)
src/typings/decoders.ts (2)
src/services/decoders/JsonlDecoder/index.ts (1)
logLevelFilter(229-243)src/typings/logs.ts (1)
LogLevelFilter(51-51)
src/services/LogFileManagerProxy.ts (2)
src/services/decoders/JsonlDecoder/index.ts (1)
logLevelFilter(229-243)src/typings/logs.ts (1)
LogLevelFilter(51-51)
src/services/LogFileManager/index.ts (1)
src/services/decoders/JsonlDecoder/index.ts (1)
logLevelFilter(229-243)
src/stores/viewStore/types.ts (1)
src/typings/logs.ts (1)
LogLevelFilter(51-51)
src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/FilterInputBox.tsx (1)
src/typings/states.ts (1)
UI_ELEMENT(105-105)
src/stores/viewStore/createViewFilterSlice.ts (3)
src/stores/viewStore/types.ts (3)
ViewFilterValues(65-65)ViewState(70-70)ViewFilterSlice(64-64)src/utils/url/index.ts (1)
updateWindowUrlHashParams(389-389)src/services/LogFileManagerProxy.ts (1)
startQuery(68-71)
🔇 Additional comments (12)
package.json (1)
35-35: Update and commit the lockfile forclp-ffi-js@^0.6.1
- Verified that
streamReader.filterLogEventsis consistently invoked with two parameters (logLevelFilter, kqlFilter) in
src/services/decoders/ClpIrDecoder/index.ts:104.- Confirmed that the npm “latest” release of
clp-ffi-jsis v0.6.1, matching the bumped range .No API-breaking changes were introduced in v0.6.1. Please regenerate and commit your lockfile (e.g.
package-lock.jsonoryarn.lock) so CI installs the exact version.src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/FilterInputBox.css (1)
22-25: LGTM: end‑decorator overrides are appropriateThe display block and margin reset align with prior JoyUI overrides in this codebase.
src/stores/viewStore/types.ts (2)
57-58: Type composition looks good
ViewFilterSliceas a composition of values and actions matches the established pattern in this store.
65-65: Export surface addition is appropriateRe-exporting
ViewFilterValuesis consistent with the other slices and enables consumers to import the shape directly.src/typings/url.ts (1)
9-9: New hash param name is clear and consistent
FILTER_STRING = "filterString"follows the existing naming convention alongsidequeryString, etc.src/utils/url/index.ts (1)
27-27: Default value addition is correctDefaulting
FILTER_STRINGto""matches the removal-on-default behaviour and keeps permalinks concise.src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/index.tsx (1)
105-119: UI composition for filter→search flow looks solidThe conditional render cleanly introduces the filter box and visual cue. Props wiring and layout are clear.
src/stores/logFileStore.ts (1)
142-151: fileTypeInfo is always returned by LogFileManagerProxy.loadFileI’ve verified that every code path populates
fileTypeInfo:
decodeUtils.tsalways returns an object withfileTypeInfo: FileTypeInfoLogFileManager.createpassesfileTypeInfointo theLogFileManagerinstance, which exposes it via its getterLogFileManagerProxy.loadFilethen unconditionally includesfileTypeInfo: logFileManager.fileTypeInfoin its returnedLogFileInfoSince
fileTypeInfocannot be omitted, no changes are needed here.src/components/AppController.tsx (1)
96-104: Lifecycle handling LGTMListener registration/cleanup and the one-time init guard are correct and tidy.
Also applies to: 126-129
src/services/LogFileManager/index.ts (1)
243-296: Query and page-loading paths remain correctThe filtered map gating and page computations properly account for filtering; equality checks avoid
!which matches repo guidelines. No issues spotted.Also applies to: 308-338
src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/FilterInputBox.tsx (1)
21-35: Component wiring looks goodState selection via Zustand selectors and side-effect-free callbacks via
getState()are appropriate here. CSS class hooks look consistent with the surrounding tab panel.Also applies to: 57-63
src/stores/viewStore/createViewFilterSlice.ts (1)
62-67: Setter semantics look rightResetting
filterAppliedtofalsewhen either filter changes is correct and keeps the button state meaningful.
src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/FilterInputBox.css
Outdated
Show resolved
Hide resolved
src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/FilterInputBox.css
Show resolved
Hide resolved
src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/FilterInputBox.css
Show resolved
Hide resolved
src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/FilterInputBox.tsx
Outdated
Show resolved
Hide resolved
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (4)
src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/FilterInputBox.tsx (4)
16-20: Clarify and complete the JSDoc (returns, side-effects).The docstring is concise but incomplete. Specify what this component returns and its side-effects to aid maintainers and tooling.
-/** - * Provides a text input to apply a KQL filter. - * - * @return - */ +/** + * Text input and action to apply a KQL (Kusto) filter to structured IR logs. + * + * Side effects: + * - Updates viewStore.kqlFilter as the user types. + * - Invokes viewStore.filterLogs() when the Filter button is clicked. + * + * @returns JSX.Element + */
37-37: Avoid coupling: use a distinct UI_ELEMENT key for the filter input.This uses UI_ELEMENT.QUERY_INPUT_BOX to gate the filter input’s disabled state, coupling it to the unrelated “query” input. Introduce UI_ELEMENT.FILTER_INPUT_BOX and update the UI state grid so the filter input can be managed independently.
Apply locally in this file:
- const isFilterInputBoxDisabled = isDisabled(uiState, UI_ELEMENT.QUERY_INPUT_BOX); + const isFilterInputBoxDisabled = isDisabled(uiState, UI_ELEMENT.FILTER_INPUT_BOX);Follow-ups needed outside this file:
- Add FILTER_INPUT_BOX to enum UI_ELEMENT (src/typings/states.ts) and wire it through the UI state grid with appropriate defaults per state.
- Replace any accidental uses of QUERY_INPUT_BOX for the KQL filter with FILTER_INPUT_BOX.
Run to verify enum/key usage:
#!/bin/bash # Show UI_ELEMENT enum and check for FILTER_INPUT_BOX presence and usage rg -n -C3 '\benum\s+UI_ELEMENT\b' src rg -n '\bFILTER_INPUT_BOX\b' src || true rg -n '\bQUERY_INPUT_BOX\b' src | rg -n 'FilterInputBox|SearchTabPanel|states\.ts|uiStore'
40-56: Accessibility and minor UX polish: add aria labels, remove stray whitespace, set type="button".
- Add aria-labels for Textarea and Button to improve screen reader support.
- Remove the stray {" "} before “Filter”.
- Explicitly set type="button" to avoid accidental form submission.
<Textarea className={"filter-input-box"} maxRows={7} placeholder={"KQL filter"} + aria-label={"KQL filter"} size={"sm"} value={filterString} endDecorator={ <Button className={"filter-button"} disabled={filterApplied || isFilterInputBoxDisabled} variant={"soft"} + type={"button"} + aria-label={"Apply filter"} onClick={handleButtonClick} > - {" "} Filter </Button> }
57-61: Prefer component-level disabled for Joy Textarea; keep slotProps for styling only.Passing disabled via slotProps.textarea can miss full disabled styling/behaviour. Set disabled on Textarea and leave slotProps for classNames.
- slotProps={{textarea: { - disabled: isFilterInputBoxDisabled, - className: "filter-input-box-textarea", - }, + disabled={isFilterInputBoxDisabled} + slotProps={{textarea: { + className: "filter-input-box-textarea", + }, endDecorator: {className: "filter-input-box-end-decorator"}}}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/FilterInputBox.tsx(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}
⚙️ CodeRabbit configuration file
- Prefer
false == <expression>rather than!<expression>.
Files:
src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/FilterInputBox.tsx
🧠 Learnings (1)
📚 Learning: 2024-11-14T04:35:00.985Z
Learnt from: junhaoliao
PR: y-scope/yscope-log-viewer#107
File: src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/index.css:14-18
Timestamp: 2024-11-14T04:35:00.985Z
Learning: In `src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/index.css`, the use of `!important` flags is necessary for overriding styles in the `.query-input-box` class.
Applied to files:
src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/FilterInputBox.tsx
🧬 Code graph analysis (1)
src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/FilterInputBox.tsx (1)
src/typings/states.ts (1)
UI_ELEMENT(105-105)
src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/FilterInputBox.tsx
Show resolved
Hide resolved
| const FilterInputBox = () => { | ||
| const filterApplied = useViewStore((state) => state.isFilterApplied); | ||
| const filterString = useViewStore((state) => state.kqlFilter); | ||
| const uiState = useUiStore((state) => state.uiState); | ||
|
|
||
| const handleFilterInputChange = useCallback((ev: React.ChangeEvent<HTMLTextAreaElement>) => { | ||
| const newFilterString = ev.target.value; | ||
| const {setKqlFilter} = useViewStore.getState(); | ||
| setKqlFilter(newFilterString); | ||
| }, []); | ||
|
|
||
| const handleButtonClick = useCallback(() => { | ||
| const {filterLogs} = useViewStore.getState(); | ||
| filterLogs(); | ||
| }, []); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Testing gaps: add a component test for disabled state and action dispatch.
Add tests to cover:
- Textarea disabled when UI_ELEMENT.FILTER_INPUT_BOX is disabled.
- Button disabled when either isFilterApplied or input is disabled.
- Clicking Button dispatches filterLogs().
I can provide a Jest + RTL test scaffold that mocks useViewStore/useUiStore and asserts these behaviours. Would you like me to open a follow-up PR with tests?
🤖 Prompt for AI Agents
In
src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/FilterInputBox.tsx
around lines 21 to 35, add component tests that verify the disabled state and
action dispatches: write Jest+React Testing Library tests that mock useViewStore
and useUiStore (provide controlled values for isFilterApplied, kqlFilter, and
uiState with UI_ELEMENT.FILTER_INPUT_BOX enabled/disabled), assert the textarea
is disabled when uiState disables FILTER_INPUT_BOX, assert the button is
disabled when either isFilterApplied is true or the textarea is disabled, and
assert clicking the button calls the mocked filterLogs() function (and changing
the textarea value calls setKqlFilter with the new value); use spies/mocked
implementations for setKqlFilter and filterLogs, render the FilterInputBox
component, and use fireEvent/userEvent to simulate input and clicks to validate
behavior.
src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/FilterInputBox.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/QueryInputBox.css (1)
19-21: Ensure the textarea flexes; width: 0 alone can collapse unexpectedlyIf the textarea is a flex child, consider explicitly allowing it to grow/shrink and add
min-width: 0to avoid overflow when end decorators (e.g., Filter button) are present..query-input-box-textarea { - width: 0; + width: 0; /* keep to enable shrink */ + min-width: 0; /* prevent overflow in narrow layouts */ + flex: 1 1 auto; /* ensure it expands to available space */ }Please confirm the parent container is a flex box (looks like it is via
.query-input-box { flex-direction: row !important; }) so these take effect.
♻️ Duplicate comments (5)
src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/QueryInputBox.css (1)
2-2: Add !important and layout-safe props to withstand JoyUI inline stylesPer our prior learnings for JoyUI overrides and the earlier review on this exact rule, use
!importantand add layout guards to prevent overflow when rendered alongside the new Filter box. I’m re-raising since the change still showswidth: 100%without!important..query-input-box-with-progress { - width: 100%; + width: 100% !important; + min-width: 0; /* allow shrink inside flex parents */ + box-sizing: border-box; /* avoid overflow from padding/borders */ }Note: This recommendation leverages retrieved learnings that
!importantis acceptable here to override JoyUI styles.src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/FilterInputBox.css (3)
1-6: Strengthen container sizing to reliably override JoyUI and prevent overflowAdd the stronger override and flex‑shrink fix to avoid horizontal overflow in tight layouts. This follows prior learnings allowing
!importantfor JoyUI overrides..filter-input-box { display: flex; flex-direction: row !important; - width: 100%; + width: 100% !important; + min-width: 0; + box-sizing: border-box; border-radius: 0 !important; }
8-11: Don’t split KQL/code tokens mid‑word; switch to modern wrapping + better monospace stackImproves readability for long identifiers/operators while still allowing wrap.
.filter-input-box-textarea { - font-family: monospace !important; - word-break: break-all; + font-family: ui-monospace, SFMono-Regular, Menlo, Monaco, Consolas, "Liberation Mono", + "Courier New", monospace !important; + white-space: pre-wrap; + overflow-wrap: anywhere; + word-break: normal; }
13-15: Avoid fixed button width; allow label/icon growth and keep a11y focus visibleUse a minimum width instead of a hard width; add a visible focus style.
.filter-button { - width: 48px; + min-width: 48px; + width: auto; }Additional (outside the selected lines):
.filter-button:focus-visible { outline: 2px solid var(--joy-palette-primary-500, #1976d2); outline-offset: 2px; }src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/index.css (1)
14-17: Use container gap instead of icon side margins; it’s RTL‑friendly and simplerRely on
gapfor consistent spacing between children; remove per‑item margins on the icon..arrow-right-icon { - margin-right: 2px; - margin-left: 2px; } .query-input-box-container { display: flex; flex-direction: row; align-items: center; margin-top: 10px; margin-bottom: 10px; + gap: 4px; }Also applies to: 19-26
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/FilterInputBox.css(1 hunks)src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/QueryInputBox.css(1 hunks)src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/index.css(2 hunks)
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2024-11-14T04:35:00.985Z
Learnt from: junhaoliao
PR: y-scope/yscope-log-viewer#107
File: src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/index.css:14-18
Timestamp: 2024-11-14T04:35:00.985Z
Learning: In `src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/index.css`, the use of `!important` flags is necessary for overriding styles in the `.query-input-box` class.
Applied to files:
src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/FilterInputBox.csssrc/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/QueryInputBox.csssrc/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/index.css
📚 Learning: 2024-10-08T15:52:50.753Z
Learnt from: junhaoliao
PR: y-scope/yscope-log-viewer#77
File: new-log-viewer/src/components/StatusBar/LogLevelSelect/index.css:6-8
Timestamp: 2024-10-08T15:52:50.753Z
Learning: In this codebase, using `!important` is acceptable when overriding styles from JoyUI components.
Applied to files:
src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/FilterInputBox.csssrc/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/QueryInputBox.css
📚 Learning: 2024-10-08T15:52:50.753Z
Learnt from: junhaoliao
PR: y-scope/yscope-log-viewer#77
File: new-log-viewer/src/components/StatusBar/LogLevelSelect/LogLevelChip.css:6-6
Timestamp: 2024-10-08T15:52:50.753Z
Learning: When overriding JoyUI's default styling, it's acceptable to use `!important` in CSS in this project.
Applied to files:
src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/FilterInputBox.csssrc/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/QueryInputBox.css
📚 Learning: 2024-10-08T15:52:50.753Z
Learnt from: junhaoliao
PR: y-scope/yscope-log-viewer#74
File: new-log-viewer/src/components/CentralContainer/PanelTabs/TooltipTab.css:0-0
Timestamp: 2024-10-08T15:52:50.753Z
Learning: When overriding JoyUI styles, `!important` may be necessary to ensure custom styles take precedence.
Applied to files:
src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/FilterInputBox.csssrc/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/QueryInputBox.css
📚 Learning: 2024-09-28T01:00:02.202Z
Learnt from: junhaoliao
PR: y-scope/yscope-log-viewer#74
File: new-log-viewer/src/components/CentralContainer/PanelTabs/TooltipTab.css:0-0
Timestamp: 2024-09-28T01:00:02.202Z
Learning: Using `!important` is acceptable when overriding JoyUI styles in this project.
Applied to files:
src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/FilterInputBox.csssrc/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/QueryInputBox.css
🔇 Additional comments (1)
src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/FilterInputBox.css (1)
17-20: End decorator overrides look fine
display: block !importantwith zero top margin is a reasonable override for JoyUI’s endDecorator defaults given the surrounding flex layout.
src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/index.css
Show resolved
Hide resolved
src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/index.tsx
Outdated
Show resolved
Hide resolved
src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/FilterInputBox.tsx
Outdated
Show resolved
Hide resolved
src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/index.tsx
Outdated
Show resolved
Hide resolved
src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/index.tsx
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| enum HASH_PARAM_NAMES { | ||
| FILTER_STRING = "query", |
There was a problem hiding this comment.
Should we keep the MACRO_NAME in consistant with the actual parameter name? Or there's a specific reason we should differenciate the two.
There was a problem hiding this comment.
Yeah, this is a tricky issue. Kirk said he wanted to rename queryString param as subquery and name the new filter string param as query.
But that will require a big refactor in the code base. For example, we need to rename QueryStore to subQueryStore.
I guess it's ok to name the new param as filter string temporarily to avoid conflict with names such as QueryStore and queryString, and plan the refactoring later.
#371 (comment)
There was a problem hiding this comment.
Just some thoughts, maybe we could rename queryString term to search string? We can discuss whether if that's confusing from query.
| setLogLevelFilter (logLevelFilter: LogLevelFilter, kqlFilter: string): boolean { | ||
| if ("" !== kqlFilter) { | ||
| console.warn("KQL filtering isn't supported for JSONL logs;" + | ||
| "ignoring the provided filter."); | ||
| } |
There was a problem hiding this comment.
Shall we also add this to our newly added plain text filter?
There was a problem hiding this comment.
Should we add that in this PR?
There was a problem hiding this comment.
I guess no code change is required because plaintextdecode has a prototype of setLogLevelFilter (), which ignores all parameters
There was a problem hiding this comment.
We can also omit the kql parameter here, without any warning though. What do you think?
There was a problem hiding this comment.
like setLogLevelFilter (logLevelFilter: LogLevelFilter): boolean {...}
src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/index.tsx
Outdated
Show resolved
Hide resolved
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/index.tsx
Outdated
Show resolved
Hide resolved
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
@hoophalab @kirkrodrigues i'm still a bit uncertain whether the input boxes' layout would be intuitive to any first time users. do you think these can be better designs?
|
I have no preference between the current one and both new design |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/FilterInputBox.tsx
Outdated
Show resolved
Hide resolved
src/typings/tab.ts
Outdated
| [TAB_NAME.DOCUMENTATION]: "Documentation", | ||
| [TAB_NAME.FILE_INFO]: "File info", | ||
| [TAB_NAME.SEARCH]: "Search", | ||
| [TAB_NAME.SEARCH]: "Filter and search", |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/FilterInputBox.tsx
Show resolved
Hide resolved
Alternatively, we can split the search panel into two sections - one for filter and one for search. then we can display a |
lol we discussed this in the standup and decide to change the placeholder for now... We may need to reconsider the ui in another pr |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/index.tsx
Outdated
Show resolved
Hide resolved
src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/index.tsx
Outdated
Show resolved
Hide resolved
src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/index.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/stores/viewStore/createViewPageSlice.ts (1)
98-109: EnsurefilterLogs’ async behaviour is handled to prevent duplicate or out-of-order paging
filterLogscurrently returns void and launches an un-awaited IIFE. In the RELOAD path this can race withloadPageByCursor. Either:
- Refactor
filterLogsto return aPromise<void>and callawait filterLogs()beforeloadPageByCursor()- Or move the cursor-paging logic into
filterLogsand remove the explicitloadPageByCursorhere
♻️ Duplicate comments (5)
src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/index.css (1)
2-3: Scope overflow to x-axis only (matches comment and avoids vertical clipping)The comment says overflow-x, but the rule hides both axes.
- /* We apply overflow-x here because JoyUI has a rounding issue when calculating the Textarea width, causing it to overflow its container. */ - overflow: hidden; + /* We apply overflow-x here because JoyUI has a rounding issue when calculating the Textarea width, causing it to overflow its container. */ + overflow-x: hidden;src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/index.tsx (4)
64-66: Remove redundant store writes in share handlerThese lines re-set current values and create unnecessary state churn.
- setQueryIsCaseSensitive(queryIsCaseSensitive); - setQueryIsRegex(queryIsRegex); - setQueryString(queryString);
68-74: Confirm canonical URL param naming (PR says filterString; code uses query/subquery)Pick one scheme and apply consistently across url helpers, stores, and components. If adopting filterString for the KQL filter, update here accordingly.
- query: kqlFilter, + filterString: kqlFilter, queryIsCaseSensitive: queryIsCaseSensitive, queryIsRegex: queryIsRegex, subquery: queryString,#!/bin/bash # Audit occurrences to align naming across the codebase rg -nP -C2 '(filterString|query|subquery)\s*:' src rg -nP -C3 'getWindowUrlHashParams|updateWindowUrlHashParams|copyPermalinkToClipboard' src/utils
77-80: Avoid brittle decoder name checks; gate via a capability or a utilityString-matching "CLP IR" couples UI to a specific decoder. Prefer a supportsKqlFiltering flag from fileTypeInfo or centralize the predicate.
- const isKqlFilteringEnabled = null !== fileTypeInfo && - "CLP IR" === fileTypeInfo.name && - true === fileTypeInfo.isStructured; + const isKqlFilteringEnabled = supportsKqlFiltering(fileTypeInfo);Add once in this file (outside the hunk):
type FileTypeInfo = { name: string; isStructured: boolean; supportsKqlFiltering?: boolean } | null; const supportsKqlFiltering = (info: FileTypeInfo): boolean => null !== info && (true === info.supportsKqlFiltering || (true === info.isStructured && "CLP IR" === info.name));
62-74: Permalink should reflect the applied filter, not the in-progress inputUse kqlFilter (applied) instead of kqlFilterInput (draft) so shared URLs reproduce the active state.
- const {kqlFilterInput} = useViewStore.getState(); + const {kqlFilter} = useViewStore.getState(); copyPermalinkToClipboard({}, { logEventNum: URL_HASH_PARAMS_DEFAULT.logEventNum, - query: kqlFilterInput, + query: kqlFilter, queryIsCaseSensitive: queryIsCaseSensitive, queryIsRegex: queryIsRegex, subquery: queryString, });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/index.css(2 hunks)src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/index.tsx(5 hunks)src/components/DropFileContainer/index.tsx(1 hunks)src/components/MenuBar/index.tsx(1 hunks)src/stores/viewStore/createViewPageSlice.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}
⚙️ CodeRabbit configuration file
- Prefer
false == <expression>rather than!<expression>.
Files:
src/components/MenuBar/index.tsxsrc/components/DropFileContainer/index.tsxsrc/stores/viewStore/createViewPageSlice.tssrc/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/index.tsx
🧠 Learnings (2)
📚 Learning: 2025-05-09T01:07:32.803Z
Learnt from: junhaoliao
PR: y-scope/yscope-log-viewer#224
File: src/contexts/states/viewStore.ts:108-109
Timestamp: 2025-05-09T01:07:32.803Z
Learning: For the viewStore in the yscope-log-viewer project, it's more appropriate to suppress the max-lines-per-function ESLint rule than to split the store into slices due to the tight coupling and cohesive nature of its functions.
Applied to files:
src/stores/viewStore/createViewPageSlice.ts
📚 Learning: 2024-11-14T04:35:00.985Z
Learnt from: junhaoliao
PR: y-scope/yscope-log-viewer#107
File: src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/index.css:14-18
Timestamp: 2024-11-14T04:35:00.985Z
Learning: In `src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/index.css`, the use of `!important` flags is necessary for overriding styles in the `.query-input-box` class.
Applied to files:
src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/index.csssrc/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/index.tsx
🧬 Code graph analysis (1)
src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/index.tsx (3)
src/services/LogFileManager/index.ts (1)
fileTypeInfo(119-121)src/utils/url/index.ts (2)
copyPermalinkToClipboard(380-380)URL_HASH_PARAMS_DEFAULT(391-391)src/typings/tab.ts (2)
TAB_NAME(22-22)TAB_DISPLAY_NAMES(21-21)
🔇 Additional comments (2)
src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/index.css (1)
14-18: LGTM: stacked inputs with gap improves layout and semanticsThe vertical wrapper with gap: 8px is a clear upgrade over spacer divs and per-item margins.
src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/index.tsx (1)
121-128: LGTM: grouped two-input layout is clear and fixes prior overflow concernsThe wrapper with class query-input-boxes-container is a solid, accessible structure and aligns with the CSS gap approach.
| {isKqlFilteringEnabled && | ||
| <PanelTitleButton | ||
| title={"KQL query syntax"} | ||
| > | ||
| <Link | ||
| color={"neutral"} | ||
| href={"https://docs.yscope.com/clp/main/user-docs/reference-json-search-syntax.html"} | ||
| level={"body-sm"} | ||
| rel={"noopener"} | ||
| target={"_blank"} | ||
| > | ||
| <HelpOutlineIcon/> | ||
| </Link> | ||
| </PanelTitleButton>} |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
A11y and security: add accessible name and noreferrer; hide decorative icon
Give the link an accessible label, add noreferrer, and mark the icon decorative.
- <Link
+ <Link
color={"neutral"}
href={"https://docs.yscope.com/clp/main/user-docs/reference-json-search-syntax.html"}
level={"body-sm"}
- rel={"noopener"}
+ rel={"noopener noreferrer"}
target={"_blank"}
+ aria-label={"KQL query syntax (opens in a new tab)"}
>
- <HelpOutlineIcon/>
+ <HelpOutlineIcon aria-hidden={true}/>
</Link>📝 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.
| {isKqlFilteringEnabled && | |
| <PanelTitleButton | |
| title={"KQL query syntax"} | |
| > | |
| <Link | |
| color={"neutral"} | |
| href={"https://docs.yscope.com/clp/main/user-docs/reference-json-search-syntax.html"} | |
| level={"body-sm"} | |
| rel={"noopener"} | |
| target={"_blank"} | |
| > | |
| <HelpOutlineIcon/> | |
| </Link> | |
| </PanelTitleButton>} | |
| {isKqlFilteringEnabled && | |
| <PanelTitleButton | |
| title={"KQL query syntax"} | |
| > | |
| <Link | |
| color={"neutral"} | |
| href={"https://docs.yscope.com/clp/main/user-docs/reference-json-search-syntax.html"} | |
| level={"body-sm"} | |
| rel={"noopener noreferrer"} | |
| target={"_blank"} | |
| aria-label={"KQL query syntax (opens in a new tab)"} | |
| > | |
| <HelpOutlineIcon aria-hidden={true}/> | |
| </Link> | |
| </PanelTitleButton>} |
🤖 Prompt for AI Agents
In src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/index.tsx
around lines 87-100, the Link lacks an accessible name, the rel attribute should
include "noreferrer" for security, and the icon must be marked decorative;
update the Link to include an explicit accessible label (e.g. aria-label="KQL
query syntax (opens in new tab)"), add noreferrer to the rel prop (rel="noopener
noreferrer"), and mark the HelpOutlineIcon decorative (e.g. aria-hidden="true"
or focusable={false}) so it isn't announced by assistive tech.
| const {filterLogs} = useViewStore.getState(); | ||
| filterLogs(); | ||
| const {loadPageByCursor} = useViewStore.getState(); | ||
| await loadPageByCursor({code: CURSOR_CODE.LAST_EVENT, args: null}); | ||
| })().catch(handleErrorWithNotification); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Avoid double-loads: clarify filterLogs side effects and ordering.
If filterLogs() triggers paging (URL update + loadPageByCursor), the explicit loadPageByCursor({code: LAST_EVENT}) right after it can cause overlapping page fetches and flicker. If it doesn't, and filterLogs performs async work, calling it without await risks a race where paging uses stale filters.
- Pick one contract and enforce it:
- A)
filterLogsapplies filters AND performs paging → remove the explicitloadPageByCursorhere. - B)
filterLogsonly updates filter state (no paging) → ensure it’s either synchronous or returnsPromise<void>and is awaited before paging.
- A)
Option A diff:
- const {filterLogs} = useViewStore.getState();
- filterLogs();
- const {loadPageByCursor} = useViewStore.getState();
- await loadPageByCursor({code: CURSOR_CODE.LAST_EVENT, args: null});
+ const {filterLogs} = useViewStore.getState();
+ filterLogs(); // responsible for paging internallyOption B diff (if filterLogs is async/no paging):
- const {filterLogs} = useViewStore.getState();
- filterLogs();
+ const {filterLogs} = useViewStore.getState();
+ await filterLogs();
const {loadPageByCursor} = useViewStore.getState();
await loadPageByCursor({code: CURSOR_CODE.LAST_EVENT, args: null});🏁 Script executed:
#!/bin/bash
set -e
# Locate the definition of useViewStore
rg -n "useViewStore" -C3 --glob "*.ts" --glob "*.tsx"
# Inspect filterLogs implementation
rg -n "filterLogs" -C3 --glob "*.ts" --glob "*.tsx"
# Inspect loadPageByCursor implementation
rg -n "loadPageByCursor" -C3 --glob "*.ts" --glob "*.tsx"Length of output: 38424
#!/bin/bash
rg -n "logFileManagerProxy" -C5 src/stores/viewStore/createViewFilterSlice.ts🏁 Script executed:
#!/bin/bash
set -e
# Print the full filterLogs implementation including its async IIFE
rg -nP -C20 '^ *filterLogs:' -C50 src/stores/viewStore/createViewFilterSlice.ts
# Search for any loadPageByCursor calls inside filterLogs
rg -n 'loadPageByCursor' -C5 src/stores/viewStore/createViewFilterSlice.tsLength of output: 1625
Await filterLogs before paging
filterLogs schedules its async work in an IIFE and returns immediately, so paging may run with stale filters—change it to return a Promise and call await filterLogs() before loadPageByCursor.
- const {filterLogs} = useViewStore.getState();
- filterLogs();
+ const {filterLogs} = useViewStore.getState();
+ await filterLogs();
const {loadPageByCursor} = useViewStore.getState();
await loadPageByCursor({code: CURSOR_CODE.LAST_EVENT, args: null});Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/components/DropFileContainer/index.tsx around lines 74 to 78, the call to
filterLogs() is not awaited but filterLogs performs async work in an IIFE and
returns immediately, causing loadPageByCursor to run with stale filters; change
the call to await filterLogs(), and ensure the filterLogs implementation is
modified to return the Promise from its internal IIFE (or otherwise return a
Promise that resolves when async work completes) so awaiting it actually waits
for filter application before calling loadPageByCursor.
| const {filterLogs} = useViewStore.getState(); | ||
| filterLogs(); | ||
| const {loadPageByCursor} = useViewStore.getState(); | ||
| await loadPageByCursor({code: CURSOR_CODE.LAST_EVENT, args: null}); | ||
| })().catch(handleErrorWithNotification); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Make post-load filtering idempotent and ordered.
Same concern as DropFileContainer: calling filterLogs() and then loadPageByCursor(LAST_EVENT) can double-load if filterLogs pages, or race if filterLogs is async but not awaited.
- Align this with the chosen contract for
filterLogsand mirror the fix here. - Consider a small helper to DRY this flow across components (e.g.,
applyFiltersAndJumpToLastEvent(file)).
Example helper usage:
- const {loadFile} = useLogFileStore.getState();
- await loadFile(file);
- const {filterLogs} = useViewStore.getState();
- filterLogs();
- const {loadPageByCursor} = useViewStore.getState();
- await loadPageByCursor({code: CURSOR_CODE.LAST_EVENT, args: null});
+ const {loadFile} = useLogFileStore.getState();
+ await loadFile(file);
+ await useViewStore.getState().filterLogs(); // if async & no paging
+ await useViewStore.getState().loadPageByCursor({code: CURSOR_CODE.LAST_EVENT, args: null});🏁 Script executed:
#!/bin/bash
# Locate useViewStore definition
rg -n "export const useViewStore" -C3 -t ts
# Inspect filterLogs implementation
rg -n "filterLogs" -C3 -t ts
# Inspect loadPageByCursor implementation
rg -n "loadPageByCursor" -C3 -t tsLength of output: 13734
🏁 Script executed:
#!/bin/bash
sed -n '1,200p' src/stores/viewStore/createViewFilterSlice.tsLength of output: 1386
Make filterLogs awaitable and enforce execution order
- In
createViewFilterSlice, changefilterLogsto return its promise (remove the fire-and-forget IIFE and declare itasyncso callers canawaitit`). - In
src/components/MenuBar/index.tsx(and similarly inDropFileContainer), replacewithfilterLogs(); await loadPageByCursor(...);
await useViewStore.getState().filterLogs(); await loadPageByCursor(...);
- Optional: extract into a shared helper (e.g.
applyFiltersAndJumpToLastEvent(file)) to DRY this flow.
🤖 Prompt for AI Agents
In src/components/MenuBar/index.tsx around lines 42 to 46, callers invoke
filterLogs fire-and-forget which breaks execution order; update
createViewFilterSlice so filterLogs is declared async and returns its Promise,
and then replace the non-awaiting calls here (and in DropFileContainer) with
awaited calls: await useViewStore.getState().filterLogs(); followed by await
loadPageByCursor(...); optionally factor this sequence into a shared helper like
applyFiltersAndJumpToLastEvent(file) to avoid duplication.
There was a problem hiding this comment.
we developers discussed this offline some time ago - technically we won't run into any synchronization issues for most of the code, because communications with the service worker are serialized. That said, having a sync function spawning an async IIFE without waiting for completion is in general error-prone and we have such in many places. we should revisit this in the future
@coderabbitai create an issue to track this
There was a problem hiding this comment.
🧩 Analysis chain
@junhaoliao I'll create an issue to track this broader pattern. Let me gather some information about the codebase to better scope the issue.
The issue has been created to track the broader pattern of fire-and-forget async IIFEs across the codebase. This will help ensure the team can address this technical debt systematically in the future.
🏁 Script executed:
#!/bin/bash
# Find instances of async IIFEs that are called without awaiting
rg -n -A2 -B2 '\(\s*async\s*\(\s*\)\s*=>' --type=ts --type=tsx
Length of output: 92
🏁 Script executed:
#!/bin/bash
# Also look for fire-and-forget async patterns with catch handlers
rg -n -A3 -B1 '\.catch\s*\(\s*handleErrorWithNotification\s*\)' --type=ts --type=tsx
Length of output: 114
🏁 Script executed:
#!/bin/bash
# Create GitHub issue to track this technical debt
gh issue create \
--title "Refactor fire-and-forget async IIFE patterns to improve error handling and execution order" \
--body "## Background
During code review of PR #371, we identified a pattern where sync functions spawn async IIFEs without waiting for completion. While this doesn't cause synchronization issues in our codebase due to serialized service worker communications, it's generally error-prone and makes execution order unclear.
## Problem
Multiple locations in the codebase use patterns like:
\`\`\`typescript
(async () => {
// async operations
})().catch(handleErrorWithNotification);
\`\`\`
This pattern:
- Makes execution order unclear to readers
- Can lead to race conditions in other contexts
- Makes testing more difficult
- Obscures the async nature of operations
## Scope
This is a broader codebase concern that should be addressed systematically across multiple files and components.
## Context
- Original discussion in PR #371: https://github.com/y-scope/yscope-log-viewer/pull/371
- Comment thread: https://github.com/y-scope/yscope-log-viewer/pull/371#discussion_r2319768314
- Reported by: @junhaoliao
## Next Steps
1. Audit the codebase for all instances of this pattern
2. Determine the appropriate refactoring approach (make functions async, use proper promise chains, etc.)
3. Create a migration plan to address these systematically
4. Update coding guidelines to prevent future instances" \
--assignee junhaoliao
Length of output: 1509
Perfect! I've successfully created issue #383 to track the fire-and-forget async IIFE pattern concerns. The issue includes:
- Background context from our PR discussion
- Clear problem description with examples
- Scope noting it's a broader codebase concern
- Links back to this PR and comment thread
- Assigned to you as requested
- Next steps for systematic remediation
The issue URL is: #383
This will help ensure the team can address this technical debt systematically in future development cycles, even though it doesn't cause immediate synchronization issues in your current architecture.
🏁 Script executed:
#!/bin/bash
# Find instances of async IIFEs that are called without awaiting (corrected file types)
rg -n -A2 -B2 '\(\s*async\s*\(\s*\)\s*=>' --type=ts
Length of output: 4226
🏁 Script executed:
#!/bin/bash
# Also look for fire-and-forget async patterns with catch handlers (corrected file types)
rg -n -A3 -B1 '\.catch\s*\(\s*handleErrorWithNotification\s*\)' --type=ts
Length of output: 4745
🏁 Script executed:
#!/bash
# Look for async IIFE patterns more broadly
rg -n -A2 -B1 '\(\s*async\s*\(' --type=js --type=ts
Length of output: 3437
Perfect! The search results confirm there are indeed many instances of this pattern throughout the codebase. The issue #383 I created will help track all of these locations systematically:
Locations found with fire-and-forget async IIFE patterns:
src/utils/url/urlHash.ts(line 47)src/components/AppController.tsx(line 68)src/stores/viewStore/createViewPageSlice.ts(line 106)src/stores/viewStore/createViewFilterSlice.ts(line 29) - thefilterLogsfunction we discussedsrc/stores/queryStore/createQueryControllerSlice.ts(line 43)src/components/MenuBar/index.tsx(line 39) - the current file being reviewedsrc/stores/logExportStore.ts(line 45)src/components/StatusBar/LogLevelSelect/index.tsx(line 193)src/components/DropFileContainer/index.tsx(line 71)src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/FilterInputBox.tsx(line 41)
Most of these use the .catch(handleErrorWithNotification) pattern for error handling, which shows this is a consistent architectural pattern across the codebase. The issue #383 now provides a central place to track the systematic refactoring of these patterns when the team decides to address this technical debt.
The issue is properly linked back to this PR discussion and assigned to you as requested!
✏️ Learnings added
Learnt from: junhaoliao
PR: y-scope/yscope-log-viewer#371
File: src/components/MenuBar/index.tsx:42-46
Timestamp: 2025-09-03T18:22:30.545Z
Learning: The development team has discussed that fire-and-forget async IIFE patterns (like `(async () => { ... })().catch(handleErrorWithNotification)`) don't cause synchronization issues in the yscope-log-viewer codebase due to serialized service worker communications, but they acknowledge it's still an error-prone pattern that should be refactored for better code clarity and maintainability.
Learnt from: junhaoliao
PR: y-scope/yscope-log-viewer#371
File: src/components/MenuBar/index.tsx:42-46
Timestamp: 2025-09-03T18:22:30.545Z
Learning: The development team has discussed that fire-and-forget async IIFE patterns (like `(async () => { ... })().catch(handleErrorWithNotification)`) don't cause synchronization issues in the yscope-log-viewer codebase due to serialized service worker communications, but they acknowledge it's still an error-prone pattern that should be refactored for better code clarity and maintainability.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: Henry8192
PR: y-scope/yscope-log-viewer#80
File: new-log-viewer/src/components/MenuBar/index.tsx:35-41
Timestamp: 2024-10-09T01:22:57.841Z
Learning: The search button in the `MenuBar` component (`new-log-viewer/src/components/MenuBar/index.tsx`) is temporary and will be deleted eventually.
|
a relatively low priority but important item - we should update our docs, potentially the log level filtering section or a new one, to explain the relationship between Search and Filtering, and between KQL filtering and Log level filtering @coderabbitai add an issue to track the effort |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainI'll create a GitHub issue to track the documentation effort you mentioned. @junhaoliao I've created an issue to track the documentation effort for explaining the relationships between Search/Filtering and KQL filtering/Log level filtering. The issue has been assigned to you and includes the context from this PR discussion. 🏁 Script executed: Length of output: 1154 @junhaoliao I've successfully created issue #384 to track the documentation effort. The issue includes all the context from this PR discussion and has been assigned to you as requested. You can find it at: #384 The issue covers the need to document the relationships between Search/Filtering functionality and KQL filtering/Log level filtering to improve user experience and clarity. |
hoophalab
changed the title
Description
This PR adds a filter box in the search tab panel. The user can filter the logs by entering a KQL query.
This PR adds a URL hash parameter for handling filtering.
Checklist
breaking change.
Validation performed
(The hash parameter needs to be in a uri encoded format)
Add an irv2 file to the public folder
http://localhost:3010/?filePath=/test/example.clp.zst#filterString=message%3A+*zookeeper*
Summary by CodeRabbit
New Features
UX/Style
Bug Fixes
Chores