chore(weave): combine op filter into filter panel #3816
Conversation
|
Preview this PR with FeatureBee: https://beta.wandb.ai/?betaVersion=3d58b53b5f64474c99d7f880ac19b7c41bde9719 |
WalkthroughThe changes integrate enhanced operation filtering into the Browse3 filter components. New properties are added to the filtering props of both the FilterBar and FilterPanel components (such as Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant CT as CallsTable
participant FP as FilterPanel
participant FB as FilterBar
participant UI as UI Elements
U->>CT: Initiates filter interaction
CT->>FP: Pass filtering props (including new operation filters)
FP->>FB: Forward filter configuration
U->>FB: Selects an operation filter option
FB->>FB: Triggers handleOperationChange
FB->>FP: Updates filter state via setFilter callback
FP->>CT: Reflects updated filtering settings
Possibly related PRs
Poem
✨ Finishing Touches
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
Documentation and Community
|
![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
🧹 Nitpick comments (6)
weave-js/src/components/PagePanelComponents/Home/Browse3/filters/FilterTagItem.tsx (2)
50-54: Consider using a more specific type for label.While the functionality is correct,
labelis typed asanywhich reduces type safety. Consider using a more specific type likeReact.ReactNode.- let label: any = `${field} ${operator}`; + let label: React.ReactNode = `${field} ${operator}`;
92-115: Consider extracting removeAction conditional logic for better readability.While the implementation is functionally correct, the conditional logic for the removeAction could be extracted to a variable for improved readability.
+ const removeActionElement = disableRemove ? ( + <></> + ) : ( + <RemoveAction + onClick={(e: any) => { + e.stopPropagation(); + onRemoveFilter(item.id); + }} + /> + ); return ( <div className={`flex items-center gap-1 rounded bg-moon-100 px-2 py-1 ${className}`}> <FilterTag label={ <> {label} <div className="ml-4">{value}</div> </> } - removeAction={ - disableRemove ? ( - <></> - ) : ( - <RemoveAction - onClick={(e: any) => { - e.stopPropagation(); - onRemoveFilter(item.id); - }} - /> - ) - } + removeAction={removeActionElement} /> </div> );weave-js/src/components/PagePanelComponents/Home/Browse3/filters/SelectField.tsx (1)
73-73: OverridinginnerIsDisabledtotrue.
Consider adding a clear comment to explain why you force-disable the field when no valid option is found, ensuring future maintainers understand this logic.weave-js/src/components/PagePanelComponents/Home/Browse3/filters/FilterBar.tsx (3)
238-245: Handlingdefault-operationinonRemoveFilter.
ResettingopVersionRefsto an empty array is a clear approach. Double-check that related UI elements also update to reflect this reset if triggered.Also applies to: 252-252
279-300:handleOperationChangefor op selection.
Implementation is straightforward. Consider adding a minor inline explanation to clarify the logic behind omittingopVersionRefsfor "all traces or calls."
496-509: Fallback filter row if none are present.
Providing a preset row is user-friendly. Ensure that any newly added filter triggers the correct re-renders.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
weave-js/src/components/PagePanelComponents/Home/Browse3/filters/FilterBar.tsx(11 hunks)weave-js/src/components/PagePanelComponents/Home/Browse3/filters/FilterPanel.tsx(2 hunks)weave-js/src/components/PagePanelComponents/Home/Browse3/filters/FilterTagItem.tsx(4 hunks)weave-js/src/components/PagePanelComponents/Home/Browse3/filters/SelectField.tsx(3 hunks)weave-js/src/components/PagePanelComponents/Home/Browse3/pages/CallsPage/CallsTable.tsx(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{js,jsx,ts,tsx}`: Focus on architectural and logical i...
**/*.{js,jsx,ts,tsx}: Focus on architectural and logical issues rather than style (assuming ESLint is in place).
Flag potential memory leaks and performance bottlenecks.
Check for proper error handling and async/await usage.
Avoid strict enforcement of try/catch blocks - accept Promise chains, early returns, and other clear error handling patterns. These are acceptable as long as they maintain clarity and predictability.
Ensure proper type usage in TypeScript files.
Look for security vulnerabilities in data handling.
Don't comment on formatting if prettier is configured.
Verify proper React hooks usage and component lifecycle.
Check for proper state management patterns.
weave-js/src/components/PagePanelComponents/Home/Browse3/filters/SelectField.tsxweave-js/src/components/PagePanelComponents/Home/Browse3/filters/FilterTagItem.tsxweave-js/src/components/PagePanelComponents/Home/Browse3/filters/FilterPanel.tsxweave-js/src/components/PagePanelComponents/Home/Browse3/pages/CallsPage/CallsTable.tsxweave-js/src/components/PagePanelComponents/Home/Browse3/filters/FilterBar.tsx
⏰ Context from checks skipped due to timeout of 90000ms (153)
- GitHub Check: WeaveJS Lint and Compile
- GitHub Check: Trace nox tests (3, 13, llamaindex)
- GitHub Check: Trace nox tests (3, 13, trace_server)
- GitHub Check: Trace nox tests (3, 13, trace)
- GitHub Check: Trace nox tests (3, 12, scorers)
- GitHub Check: Trace nox tests (3, 12, llamaindex)
- GitHub Check: Trace nox tests (3, 12, litellm)
- GitHub Check: Trace nox tests (3, 12, trace_server)
- GitHub Check: Trace nox tests (3, 12, trace)
- GitHub Check: Trace nox tests (3, 11, scorers)
- GitHub Check: Trace nox tests (3, 11, llamaindex)
- GitHub Check: Trace nox tests (3, 11, trace_server)
- GitHub Check: Trace nox tests (3, 11, trace)
- GitHub Check: Trace nox tests (3, 10, scorers)
- GitHub Check: Trace nox tests (3, 10, trace)
- GitHub Check: Trace nox tests (3, 9, scorers)
- GitHub Check: WeaveJS Lint and Compile
- GitHub Check: Trace nox tests (3, 13, llamaindex)
- GitHub Check: Trace nox tests (3, 13, trace_server)
- GitHub Check: Trace nox tests (3, 13, trace)
- GitHub Check: Trace nox tests (3, 12, scorers)
- GitHub Check: Trace nox tests (3, 12, llamaindex)
- GitHub Check: Trace nox tests (3, 12, litellm)
- GitHub Check: Trace nox tests (3, 12, trace_server)
- GitHub Check: Trace nox tests (3, 12, trace)
- GitHub Check: Trace nox tests (3, 11, scorers)
- GitHub Check: Trace nox tests (3, 11, llamaindex)
- GitHub Check: Trace nox tests (3, 11, trace_server)
- GitHub Check: Trace nox tests (3, 11, trace)
- GitHub Check: Trace nox tests (3, 10, scorers)
- GitHub Check: Trace nox tests (3, 10, trace)
- GitHub Check: Trace nox tests (3, 9, scorers)
- GitHub Check: WeaveJS Lint and Compile
- GitHub Check: Trace nox tests (3, 13, llamaindex)
- GitHub Check: Trace nox tests (3, 13, trace_server)
- GitHub Check: Trace nox tests (3, 13, trace)
- GitHub Check: Trace nox tests (3, 12, scorers)
- GitHub Check: Trace nox tests (3, 12, llamaindex)
- GitHub Check: Trace nox tests (3, 12, litellm)
- GitHub Check: Trace nox tests (3, 12, trace_server)
- GitHub Check: Trace nox tests (3, 12, trace)
- GitHub Check: Trace nox tests (3, 11, scorers)
- GitHub Check: Trace nox tests (3, 11, llamaindex)
- GitHub Check: Trace nox tests (3, 11, trace_server)
- GitHub Check: Trace nox tests (3, 11, trace)
- GitHub Check: Trace nox tests (3, 10, scorers)
- GitHub Check: Trace nox tests (3, 10, trace)
- GitHub Check: Trace nox tests (3, 9, scorers)
- GitHub Check: WeaveJS Lint and Compile
- GitHub Check: Trace nox tests (3, 13, llamaindex)
- GitHub Check: Trace nox tests (3, 13, trace_server)
- GitHub Check: Trace nox tests (3, 13, trace)
- GitHub Check: Trace nox tests (3, 12, scorers)
- GitHub Check: Trace nox tests (3, 12, llamaindex)
- GitHub Check: Trace nox tests (3, 12, litellm)
- GitHub Check: Trace nox tests (3, 12, trace_server)
- GitHub Check: Trace nox tests (3, 12, trace)
- GitHub Check: Trace nox tests (3, 11, scorers)
- GitHub Check: Trace nox tests (3, 11, llamaindex)
- GitHub Check: Trace nox tests (3, 11, trace_server)
- GitHub Check: Trace nox tests (3, 11, trace)
- GitHub Check: Trace nox tests (3, 10, scorers)
- GitHub Check: Trace nox tests (3, 10, trace)
- GitHub Check: Trace nox tests (3, 9, scorers)
- GitHub Check: WeaveJS Lint and Compile
- GitHub Check: Trace nox tests (3, 13, llamaindex)
- GitHub Check: Trace nox tests (3, 13, trace_server)
- GitHub Check: Trace nox tests (3, 13, trace)
- GitHub Check: Trace nox tests (3, 12, scorers)
- GitHub Check: Trace nox tests (3, 12, llamaindex)
- GitHub Check: Trace nox tests (3, 12, litellm)
- GitHub Check: Trace nox tests (3, 12, trace_server)
- GitHub Check: Trace nox tests (3, 12, trace)
- GitHub Check: Trace nox tests (3, 11, scorers)
- GitHub Check: Trace nox tests (3, 11, llamaindex)
- GitHub Check: Trace nox tests (3, 11, trace_server)
- GitHub Check: Trace nox tests (3, 11, trace)
- GitHub Check: Trace nox tests (3, 10, scorers)
- GitHub Check: Trace nox tests (3, 10, trace)
- GitHub Check: Trace nox tests (3, 9, scorers)
- GitHub Check: WeaveJS Lint and Compile
- GitHub Check: Trace nox tests (3, 13, llamaindex)
- GitHub Check: Trace nox tests (3, 13, trace_server)
- GitHub Check: Trace nox tests (3, 13, trace)
- GitHub Check: Trace nox tests (3, 12, scorers)
- GitHub Check: Trace nox tests (3, 12, llamaindex)
- GitHub Check: Trace nox tests (3, 12, litellm)
- GitHub Check: Trace nox tests (3, 12, trace_server)
- GitHub Check: Trace nox tests (3, 12, trace)
- GitHub Check: Trace nox tests (3, 11, scorers)
- GitHub Check: Trace nox tests (3, 11, llamaindex)
- GitHub Check: Trace nox tests (3, 11, trace_server)
- GitHub Check: Trace nox tests (3, 11, trace)
- GitHub Check: Trace nox tests (3, 10, scorers)
- GitHub Check: Trace nox tests (3, 10, trace)
- GitHub Check: Trace nox tests (3, 9, scorers)
- GitHub Check: WeaveJS Lint and Compile
- GitHub Check: Trace nox tests (3, 13, llamaindex)
- GitHub Check: Trace nox tests (3, 13, trace_server)
- GitHub Check: Trace nox tests (3, 13, trace)
- GitHub Check: Trace nox tests (3, 12, scorers)
- GitHub Check: Trace nox tests (3, 12, llamaindex)
- GitHub Check: Trace nox tests (3, 12, litellm)
- GitHub Check: Trace nox tests (3, 12, trace_server)
- GitHub Check: Trace nox tests (3, 12, trace)
- GitHub Check: Trace nox tests (3, 11, scorers)
- GitHub Check: Trace nox tests (3, 11, trace_server)
- GitHub Check: Trace nox tests (3, 11, trace)
- GitHub Check: Trace nox tests (3, 10, scorers)
- GitHub Check: Trace nox tests (3, 10, trace)
- GitHub Check: Trace nox tests (3, 9, scorers)
- GitHub Check: WeaveJS Lint and Compile
- GitHub Check: Trace nox tests (3, 13, llamaindex)
- GitHub Check: Trace nox tests (3, 13, trace_server)
- GitHub Check: Trace nox tests (3, 13, trace)
- GitHub Check: Trace nox tests (3, 12, scorers)
- GitHub Check: Trace nox tests (3, 12, llamaindex)
- GitHub Check: Trace nox tests (3, 12, trace_server)
- GitHub Check: Trace nox tests (3, 12, trace)
- GitHub Check: Trace nox tests (3, 11, scorers)
- GitHub Check: Trace nox tests (3, 11, trace_server)
- GitHub Check: Trace nox tests (3, 11, trace)
- GitHub Check: Trace nox tests (3, 10, scorers)
- GitHub Check: Trace nox tests (3, 10, trace)
- GitHub Check: Trace nox tests (3, 9, scorers)
- GitHub Check: WeaveJS Lint and Compile
- GitHub Check: Trace nox tests (3, 13, llamaindex)
- GitHub Check: Trace nox tests (3, 13, trace_server)
- GitHub Check: Trace nox tests (3, 13, trace)
- GitHub Check: Trace nox tests (3, 12, scorers)
- GitHub Check: Trace nox tests (3, 12, llamaindex)
- GitHub Check: Trace nox tests (3, 12, trace_server)
- GitHub Check: Trace nox tests (3, 12, trace)
- GitHub Check: Trace nox tests (3, 11, scorers)
- GitHub Check: Trace nox tests (3, 11, trace_server)
- GitHub Check: Trace nox tests (3, 11, trace)
- GitHub Check: Trace nox tests (3, 10, scorers)
- GitHub Check: Trace nox tests (3, 10, trace)
- GitHub Check: Trace nox tests (3, 9, scorers)
- GitHub Check: WeaveJS Lint and Compile
- GitHub Check: Trace nox tests (3, 13, llamaindex)
- GitHub Check: Trace nox tests (3, 13, trace_server)
- GitHub Check: Trace nox tests (3, 13, trace)
- GitHub Check: Trace nox tests (3, 12, scorers)
- GitHub Check: Trace nox tests (3, 12, llamaindex)
- GitHub Check: Trace nox tests (3, 12, trace_server)
- GitHub Check: Trace nox tests (3, 12, trace)
- GitHub Check: Trace nox tests (3, 11, scorers)
- GitHub Check: Trace nox tests (3, 11, trace_server)
- GitHub Check: Trace nox tests (3, 11, trace)
- GitHub Check: Trace nox tests (3, 10, scorers)
- GitHub Check: Trace nox tests (3, 10, trace)
- GitHub Check: Trace nox tests (3, 9, scorers)
🔇 Additional comments (21)
weave-js/src/components/PagePanelComponents/Home/Browse3/filters/FilterTagItem.tsx (4)
5-8: New imports support enhanced op filtering functionality.The added imports for
Icon,Tooltip,ALL_TRACES_OR_CALLS_REF_KEY, andopNiceNameproperly support the new operation filtering capabilities being integrated into the filter panel.Also applies to: 14-15
30-34: Adding optional className prop enhances component flexibility.The addition of an optional
classNameprop follows TypeScript best practices and allows for custom styling to be applied from parent components.
43-47: Component signature properly handles the new className prop.The updated component signature correctly destructures the new prop with a sensible default empty string value.
60-82: Implementation for operation filtering looks good.The special handling for the 'default-operation' filter item is well implemented:
- Uses tooltips to display operation information
- Properly parses operation names
- Disables removal for "All ops" filter
- Uses appropriate icon for visual clarity
The conditional logic is clear and handles all cases appropriately.
weave-js/src/components/PagePanelComponents/Home/Browse3/filters/SelectField.tsx (4)
30-30: Add optionalisDisabledprop.
This design is straightforward and enhances the component’s flexibility.
62-62: Prop destructuring looks good.
No issues identified; destructuring the optionalisDisabledprop is appropriate here.
68-68: Use of local variableinnerIsDisabled.
This approach is clean for toggling the disabled state. Just be mindful of scenarios where the parent might expect synchronous updates.
95-95: Prop usage forSelectis consistent.
PassinginnerIsDisabledto maintain the correct disabled state is well done.weave-js/src/components/PagePanelComponents/Home/Browse3/filters/FilterPanel.tsx (2)
11-12: New imports introduced.
These imports forWFHighLevelCallFilterandOpVersionSchemaalign with the expanded filtering functionality.
23-36: ExtendedFilterPanelPropsto handle op filtering.
Declaring additional props for new filter states (frozenFilter, filter, setFilter, etc.) ensures consistent usage throughout. The definitions look correct; no issues found.weave-js/src/components/PagePanelComponents/Home/Browse3/pages/CallsPage/CallsTable.tsx (3)
9-9: Imported new components from@mui/material.
This change is minor and has no apparent impact on functionality.
77-77: ImportingCallSchema.
This is a straightforward import. No concerns identified.
753-757: Passing new filter props toFilterPanel.
Linking the table’s filters with the panel is consistent with the design. Ensure that parent and child keep props in sync, but this looks good.weave-js/src/components/PagePanelComponents/Home/Browse3/filters/FilterBar.tsx (8)
5-7: Imports for new op filtering utilities and styled components.
These imports are organized and reflect the new filtering logic. No issues found.Also applies to: 13-13, 19-23
47-60: ExtendedFilterBarPropsfor operation filtering.
AddingfrozenFilter,filter,setFilter,selectedOpVersionOption, andopVersionOptionsbroadens the filter’s scope and keeps the design clean.
79-84: Destructuring newly added props.
Destructuring in the component signature captures the new filtering props. Looks consistent with the rest of the code.
314-397:renderOperationFilterto set operation row UI.
The approach is well-structured, usingAutocompleteto pick fromopVersionOptions. The usage of disabled fields to display “field” and “operator” is a clear, user-friendly design. No issues detected.
399-411: BuildingoperationItemfor default op filter handling.
This array logic ensures a consistent fallback if no references are present. The design is succinct.
426-426: CombiningoperationItemwith existing filters.
AppendingoperationItemat the front is a clean approach to ensure default filters are always shown first.
470-480: Default filters section block.
Clearly separates default from custom filters, improving UI. This structure is well-organized and easy to follow.
481-483: "AND" label for custom filters.
This small label helps clarify the logic chaining. No issues noted.
Description
TODO:
Testing
Summary by CodeRabbit