Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
296caca to
753ac5a
Compare
545776d to
5ce48bf
Compare
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe changes introduce a reusable CEL (Common Expression Language) rule builder component in a custom component library, refactor the routing-specific rule builder to delegate to this new base component, and enhance menu portal positioning controls across multiselect components through new optional props. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
5ce48bf to
f1baec5
Compare
753ac5a to
a73de82
Compare
f1baec5 to
ef6d5bd
Compare
880f3fc to
143accc
Compare
201520b to
9a8c883
Compare
3f7d339 to
aa43ff3
Compare
6589138 to
609a5b0
Compare
238798b to
0020a22
Compare
609a5b0 to
62dc2af
Compare
0020a22 to
35a939d
Compare
783b0f9 to
e68874c
Compare
52bf616 to
dadb0ae
Compare
02bc9a2 to
de54265
Compare
dadb0ae to
80c486c
Compare
80c486c to
4a1e15a
Compare
de54265 to
1c7f2fa
Compare
1c7f2fa to
8e42a3e
Compare
4a1e15a to
3716f01
Compare
8e42a3e to
d4adcd6
Compare
b1dc69a to
f9d280f
Compare
3a61ef7 to
b43ac59
Compare
f9d280f to
3e8b805
Compare
b43ac59 to
eb1189e
Compare
Confidence Score: 3/5Not safe to merge — the missing CSS file will break the build. A P0 build-breaking issue (missing CSS file in the new library location) blocks merge. A P1 incomplete portal support for the provider multi-select also needs to be addressed. The rest of the refactoring is structurally sound. ui/components/ui/custom/celBuilder/queryBuilderWrapper.tsx (missing CSS file) and ui/components/ui/custom/celBuilder/valueEditor.tsx (provider AsyncMultiSelect missing portal props). Important Files Changed
|
There was a problem hiding this comment.
🧹 Nitpick comments (3)
ui/app/workspace/mcp-logs/views/bifrost.code-workspace (1)
6-8: Avoid committing environment-specific sibling-repo paths in shared workspace config.
"../../../../../../bifrost-enterprise"is machine/repo-layout specific and will break for contributors who don’t have that exact sibling checkout. Prefer a workspace file template, or keep this in user-local VS Code config instead of tracked app files.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/workspace/mcp-logs/views/bifrost.code-workspace` around lines 6 - 8, Remove the hard-coded sibling repo path value ("../../../../../../bifrost-enterprise") from the workspace file and replace it with a non-committed solution: either convert this workspace entry into a template placeholder (e.g., a "path": "<LOCAL_BIFROST_ENTERPRISE_PATH>" comment) or remove the "path" entry entirely and document the expected local path in the repo README; alternatively move this setting into a user-only VS Code workspace/user settings so it is not tracked. Update the bifrost.code-workspace to not contain environment-specific paths and add a short README note explaining how contributors should set their local path.ui/components/ui/custom/celBuilder/valueEditor.tsx (1)
179-191: Consider passing menu portal props to provider AsyncMultiSelect for consistency.The
ModelMultiselectcomponents at lines 113-122 and 126-137 receivemenuPositionandmenuPortalTargetprops, but theAsyncMultiSelectfor provider selection (lines 179-191) does not. If the CEL builder is rendered inside a popover or dialog, this dropdown may also have positioning issues.♻️ Proposed fix
return ( <AsyncMultiSelect value={selectedOptions} onChange={handleMultiselectChange} defaultOptions={allOptions} isNonAsync={true} isClearable={false} placeholder="Select providers..." className="w-[360px]" triggerClassName="!shadow-none !border-border h-10" menuClassName="!z-[100] w-full cursor-pointer" + menuPosition={menuPosition} + menuPortalTarget={menuPortalTarget} /> );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/components/ui/custom/celBuilder/valueEditor.tsx` around lines 179 - 191, The AsyncMultiSelect used for provider selection (component AsyncMultiSelect in valueEditor.tsx) lacks the menuPosition and menuPortalTarget props which ModelMultiselect components receive, causing potential dropdown positioning issues inside popovers/dialogs; update the AsyncMultiSelect JSX to accept and pass through the same menuPosition and menuPortalTarget props (and any existing portal target prop variable used elsewhere) so its menu is rendered consistently with ModelMultiselect and positioned correctly when the CEL builder is inside overlays.ui/components/ui/custom/celBuilder/celRuleBuilder.tsx (1)
157-176: Adddata-testidto the Copy button for testability.Per coding guidelines, new interactive UI elements should have
data-testidattributes. The Copy button lacks one.♻️ Proposed fix
<Button variant="outline" size="sm" onClick={() => copy(celExpression)} disabled={!celExpression} className="gap-2" type="button" + data-testid="cel-builder-copy-btn" >As per coding guidelines:
ui/**/*.{tsx,ts}: Add new interactive UI elements withdata-testidattributes following the pattern:data-testid="<entity>-<element>-<qualifier>".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/components/ui/custom/celBuilder/celRuleBuilder.tsx` around lines 157 - 176, The Copy Button in celRuleBuilder.tsx (the Button rendering with onClick={() => copy(celExpression)} and text toggled by the copied variable) is missing a data-testid; add a data-testid attribute following the project pattern (e.g. data-testid="celrule-copy-button" or similar entity-element-qualifier) to that Button element so tests can target it reliably.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@ui/app/workspace/mcp-logs/views/bifrost.code-workspace`:
- Around line 6-8: Remove the hard-coded sibling repo path value
("../../../../../../bifrost-enterprise") from the workspace file and replace it
with a non-committed solution: either convert this workspace entry into a
template placeholder (e.g., a "path": "<LOCAL_BIFROST_ENTERPRISE_PATH>" comment)
or remove the "path" entry entirely and document the expected local path in the
repo README; alternatively move this setting into a user-only VS Code
workspace/user settings so it is not tracked. Update the bifrost.code-workspace
to not contain environment-specific paths and add a short README note explaining
how contributors should set their local path.
In `@ui/components/ui/custom/celBuilder/celRuleBuilder.tsx`:
- Around line 157-176: The Copy Button in celRuleBuilder.tsx (the Button
rendering with onClick={() => copy(celExpression)} and text toggled by the
copied variable) is missing a data-testid; add a data-testid attribute following
the project pattern (e.g. data-testid="celrule-copy-button" or similar
entity-element-qualifier) to that Button element so tests can target it
reliably.
In `@ui/components/ui/custom/celBuilder/valueEditor.tsx`:
- Around line 179-191: The AsyncMultiSelect used for provider selection
(component AsyncMultiSelect in valueEditor.tsx) lacks the menuPosition and
menuPortalTarget props which ModelMultiselect components receive, causing
potential dropdown positioning issues inside popovers/dialogs; update the
AsyncMultiSelect JSX to accept and pass through the same menuPosition and
menuPortalTarget props (and any existing portal target prop variable used
elsewhere) so its menu is rendered consistently with ModelMultiselect and
positioned correctly when the CEL builder is inside overlays.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 53f48c3b-bd7e-4f83-a9b8-71ca05da056b
📒 Files selected for processing (12)
ui/app/workspace/mcp-logs/views/bifrost.code-workspaceui/app/workspace/routing-rules/components/celBuilder/celRuleBuilder.tsxui/components/ui/asyncMultiselect.tsxui/components/ui/custom/celBuilder/actionButton.tsxui/components/ui/custom/celBuilder/celRuleBuilder.tsxui/components/ui/custom/celBuilder/combinatorSelector.tsxui/components/ui/custom/celBuilder/fieldSelector.tsxui/components/ui/custom/celBuilder/index.tsui/components/ui/custom/celBuilder/operatorSelector.tsxui/components/ui/custom/celBuilder/queryBuilderWrapper.tsxui/components/ui/custom/celBuilder/valueEditor.tsxui/components/ui/modelMultiselect.tsx
Merge activity
|

Summary
Refactored the CEL Rule Builder component to be reusable across different contexts by extracting it from routing-specific code into a shared UI component library.
Changes
ui/app/workspace/routing-rules/components/celBuilder/toui/components/ui/custom/celBuilder/to make them reusableCELRuleBuildercomponent that accepts field definitions, operators, and conversion functions as propsAsyncMultiSelectandModelMultiselectcomponents withmenuPortalTargetandmenuPositionprops for better portal control in different contextsType of change
Affected areas
How to test
Verify that the routing rules CEL builder continues to work as expected:
Test the routing rules page to ensure the CEL builder functionality remains intact and that multiselect components render properly in different contexts.
Screenshots/Recordings
N/A - This is a refactoring change that maintains existing functionality.
Breaking changes
Related issues
N/A
Security considerations
No security implications - this is a pure refactoring change that maintains existing functionality.
Checklist
docs/contributing/README.mdand followed the guidelines