feat: add rule chaining support to routing engine#2319
Merged
Conversation
|
|
9 tasks
Collaborator
Author
This stack of pull requests is managed by Graphite. Learn more about stacking. |
Contributor
📝 WalkthroughSummary by CodeRabbit
WalkthroughRouting rules gain a boolean Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
🚥 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 |
Contributor
Confidence Score: 5/5
Important Files Changed
Reviews (13): Last reviewed commit: "feat: adds chaining to routing rules" | Re-trigger Greptile |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@plugins/governance/routing.go`:
- Around line 97-103: The loop builds iterCtx from routingCtx but never
refreshes iterCtx.BudgetAndRateLimitStatus, so chained evaluations see stale
metrics; before calling extractRoutingVariables(&iterCtx) recompute the
budget/rate-limit snapshot for the current provider/model
(currentProvider/currentModel) and assign it to iterCtx.BudgetAndRateLimitStatus
(the same logic used when initially populating
routingCtx.BudgetAndRateLimitStatus in main.go), then call
extractRoutingVariables; apply the same change in the other loop location that
also constructs iterCtx (the area around the second occurrence referencing
iterCtx/currentProvider/currentModel).
- Around line 97-98: The loop that advances routing decisions (the for loop
using chainStep) currently only checks for no-op steps and can loop forever
across repeating state cycles; update the logic in the routing loop (referencing
the for chainStep variable and the code that selects provider/model for the
current step) to record each visited state tuple (provider, model) in a set/map
and check that set before accepting a new step — if the tuple was seen before,
break and return an error/failed route; additionally add a small max-depth guard
(e.g., MAX_CHAIN_DEPTH) checked against chainStep to also break the loop as a
backstop; ensure the new checks are applied in the same block where the CEL
variables and next provider/model are chosen (the same region referenced also by
the reviewer for lines 213-227).
In `@ui/app/workspace/routing-rules/views/routingRuleSheet.tsx`:
- Around line 341-354: The Chain Rule Switch in routingRuleSheet.tsx (the Switch
component with id="chain_rule", props checked={chainRule} and onCheckedChange
calling setValue("chain_rule", ...)) needs a test id added: add a data-testid
attribute following the project convention
data-testid="routing-rule-chain-rule-switch" (or entity-element-qualifier
pattern) to the Switch element so automated tests can reliably select this new
interactive control.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2dc87fbf-668f-4f86-94d2-970e130896f9
📒 Files selected for processing (10)
docs/providers/routing-rules.mdxframework/configstore/clientconfig.goframework/configstore/migrations.goframework/configstore/tables/routing_rules.goplugins/governance/routing.gotransports/bifrost-http/handlers/governance.goui/app/workspace/routing-rules/views/routingRuleInfoSheet.tsxui/app/workspace/routing-rules/views/routingRuleSheet.tsxui/app/workspace/routing-rules/views/routingRulesTable.tsxui/lib/types/routingRules.ts
Pratham-Mishra04
force-pushed
the
03-26-feat_adds_chaining_to_routing_rules
branch
from
b978d56 to
cb70c9a
Compare
Merged
18 tasks
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
ui/app/workspace/routing-rules/views/routingRuleSheet.tsx (1)
349-353:⚠️ Potential issue | 🟡 MinorAdd a
data-testidto the new Chain Rule switch.This is a new interactive control, so it should expose a stable selector such as
routing-rule-chain-rule-switch.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/app/workspace/routing-rules/views/routingRuleSheet.tsx` around lines 349 - 353, Add a data-testid attribute to the Chain Rule Switch so tests can reliably select it: update the Switch component (id="chain_rule", checked={chainRule}, onCheckedChange={(checked) => setValue("chain_rule", checked)}) to include data-testid="routing-rule-chain-rule-switch" while keeping existing props (Switch, chainRule, setValue) unchanged.plugins/governance/routing.go (2)
97-103:⚠️ Potential issue | 🔴 CriticalDetect repeated chain states, not just no-op steps.
The convergence check only stops self-loops. A pair of chain rules that alternates
(provider, model)states will spin forever on the request path because every step changes state. Track visited(provider, model)tuples and keep a small max-depth backstop before advancing the loop.Also applies to: 218-227
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/governance/routing.go` around lines 97 - 103, The loop in routing.go that iterates with chainStep and builds iterCtx (setting Provider=currentProvider and Model=currentModel) only detects exact no-op self-loops and can spin on alternating states; modify the logic where extractRoutingVariables(&iterCtx) is invoked to track visited (provider, model) tuples (e.g., a map keyed by provider+model) and break the loop if a tuple is seen again, and also add a small max-depth cap (configurable constant) as a secondary backstop to stop after N iterations; apply the same change for the other similar loop around lines 218-227 to prevent infinite alternation.
97-103:⚠️ Potential issue | 🟠 MajorRefresh capacity metrics on each chained re-evaluation.
Only
iterCtx.ProvideranditerCtx.Modelare updated here.iterCtx.BudgetAndRateLimitStatusstill points at the snapshot built for the original provider/model, so chained rules usingbudget_used,tokens_used, orrequestevaluate stale values after the first rewrite.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/governance/routing.go` around lines 97 - 103, The loop updates iterCtx.Provider and iterCtx.Model but leaves iterCtx.BudgetAndRateLimitStatus pointing at the original snapshot, causing chained evaluations to see stale capacity metrics; before calling extractRoutingVariables(&iterCtx) refresh iterCtx.BudgetAndRateLimitStatus with a new capacity snapshot for the currentProvider/currentModel (i.e., recreate whatever BudgetAndRateLimitStatus object was originally built for routingCtx using the same helper/logic used on initial construction) so chained rules that reference budget_used, tokens_used or request get up-to-date values.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@framework/configstore/migrations.go`:
- Around line 5317-5345: The migration currently only adds the "chain_rule"
column but does not backfill the routing rules' config_hash, causing legacy rows
to appear changed; inside migrationAddChainRuleColumnToRoutingRules, after
successfully adding the "chain_rule" column (within the Migrate func), iterate
existing records in tables.TableRoutingRule using the tx (tx.WithContext(ctx))
and recompute each row's config_hash to include the new chain_rule value (treat
absent/null chain_rule as the default used by your hash logic), then persist the
updated config_hash (via
tx.Model(&tables.TableRoutingRule{}).Where(...).Update(...) or batched updates)
so legacy rows get the correct hash; ensure this operation runs within the same
transaction and respects errors (returning wrapped fmt.Errorf on failure) so
rollback still drops the column cleanly.
---
Duplicate comments:
In `@plugins/governance/routing.go`:
- Around line 97-103: The loop in routing.go that iterates with chainStep and
builds iterCtx (setting Provider=currentProvider and Model=currentModel) only
detects exact no-op self-loops and can spin on alternating states; modify the
logic where extractRoutingVariables(&iterCtx) is invoked to track visited
(provider, model) tuples (e.g., a map keyed by provider+model) and break the
loop if a tuple is seen again, and also add a small max-depth cap (configurable
constant) as a secondary backstop to stop after N iterations; apply the same
change for the other similar loop around lines 218-227 to prevent infinite
alternation.
- Around line 97-103: The loop updates iterCtx.Provider and iterCtx.Model but
leaves iterCtx.BudgetAndRateLimitStatus pointing at the original snapshot,
causing chained evaluations to see stale capacity metrics; before calling
extractRoutingVariables(&iterCtx) refresh iterCtx.BudgetAndRateLimitStatus with
a new capacity snapshot for the currentProvider/currentModel (i.e., recreate
whatever BudgetAndRateLimitStatus object was originally built for routingCtx
using the same helper/logic used on initial construction) so chained rules that
reference budget_used, tokens_used or request get up-to-date values.
In `@ui/app/workspace/routing-rules/views/routingRuleSheet.tsx`:
- Around line 349-353: Add a data-testid attribute to the Chain Rule Switch so
tests can reliably select it: update the Switch component (id="chain_rule",
checked={chainRule}, onCheckedChange={(checked) => setValue("chain_rule",
checked)}) to include data-testid="routing-rule-chain-rule-switch" while keeping
existing props (Switch, chainRule, setValue) unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b781bc5f-0e29-41a0-90ea-c1e59f87d9d1
📒 Files selected for processing (10)
docs/providers/routing-rules.mdxframework/configstore/clientconfig.goframework/configstore/migrations.goframework/configstore/tables/routing_rules.goplugins/governance/routing.gotransports/bifrost-http/handlers/governance.goui/app/workspace/routing-rules/views/routingRuleInfoSheet.tsxui/app/workspace/routing-rules/views/routingRuleSheet.tsxui/app/workspace/routing-rules/views/routingRulesTable.tsxui/lib/types/routingRules.ts
✅ Files skipped from review due to trivial changes (1)
- ui/app/workspace/routing-rules/views/routingRuleInfoSheet.tsx
🚧 Files skipped from review as they are similar to previous changes (5)
- framework/configstore/clientconfig.go
- ui/app/workspace/routing-rules/views/routingRulesTable.tsx
- ui/lib/types/routingRules.ts
- transports/bifrost-http/handlers/governance.go
- docs/providers/routing-rules.mdx
Pratham-Mishra04
force-pushed
the
03-26-feat_adds_chaining_to_routing_rules
branch
2 times, most recently
from
b2c094b to
c5a4a39
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
ui/app/workspace/config/pricing-config/page.tsx (1)
3-3: Prefer@/...alias import here for consistency.Please use the repository-standard alias path instead of a relative import.
♻️ Suggested change
-import ModelSettingsView from "../views/modelSettingsView" +import ModelSettingsView from "@/app/workspace/config/views/modelSettingsView"Based on learnings: In the UI codebase, prefer alias imports using
@/... over relative imports to maintain consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/workspace/config/pricing-config/page.tsx` at line 3, Replace the relative import of ModelSettingsView with the repository-standard alias import; locate the import statement that currently reads "import ModelSettingsView from \"../views/modelSettingsView\"" and change it to use the "@/..." alias path for ModelSettingsView so the module resolution matches other UI files and maintains consistency with the project's import conventions.ui/app/workspace/config/views/modelSettingsView.tsx (2)
163-176: Error message placement inconsistency for routing_chain_max_depth.The error message for
routing_chain_max_depth(line 176) is rendered outside the bordered container (lines 156-175), while the other fields have their error messages inside their respective containers. This creates visual inconsistency.♻️ Proposed fix to move error inside the container
{/* Routing Chain Max Depth */} <div className="flex items-center justify-between rounded-sm border p-4"> <div className="space-y-0.5"> <Label htmlFor="routing-chain-max-depth">Routing Chain Max Depth</Label> <p className="text-muted-foreground text-sm"> Maximum number of chained routing rule evaluations per request. Prevents infinite loops from circular rule definitions. </p> + {errors.routing_chain_max_depth && <p className="text-destructive text-sm">{errors.routing_chain_max_depth.message}</p>} </div> <Input id="routing-chain-max-depth" type="number" className={`w-24 ${errors.routing_chain_max_depth ? "border-destructive" : ""}`} data-testid="routing-chain-max-depth-input" {...register("routing_chain_max_depth", { required: "Routing chain max depth is required", min: { value: 1, message: "Must be at least 1" }, max: { value: 100, message: "Cannot exceed 100" }, valueAsNumber: true, })} /> </div> - {errors.routing_chain_max_depth && <p className="text-destructive text-sm">{errors.routing_chain_max_depth.message}</p>}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/workspace/config/views/modelSettingsView.tsx` around lines 163 - 176, The routing_chain_max_depth error paragraph is rendered outside the input's bordered container causing visual inconsistency; move the conditional rendering of errors.routing_chain_max_depth so the <p className="text-destructive text-sm">...</p> is inside the same parent container that wraps the Input (the block containing Input with id "routing-chain-max-depth") to match how other fields render their errors, ensuring you keep the same class names and the register call for routing_chain_max_depth unchanged.
65-85: Consider defensive handling for potentially undefinedclientConfig.The non-null assertion on
clientConfig!(line 76) assumesclient_configis always present whenbifrostConfigexists. While the submit button is disabled when!hasChanges(which checksbifrostConfig), there's still a theoretical edge case ifclient_configis undefined.Consider using a fallback or early return for safety.
♻️ Proposed defensive fix
const onSubmit = async (data: ModelSettingsFormData) => { + if (!bifrostConfig || !clientConfig) return; try { await updateCoreConfig({ - ...bifrostConfig!, + ...bifrostConfig, framework_config: { ...frameworkConfig, id: bifrostConfig?.framework_config.id || 0, pricing_url: data.pricing_datasheet_url, pricing_sync_interval: data.pricing_sync_interval_hours * 3600, }, client_config: { - ...clientConfig!, + ...clientConfig, routing_chain_max_depth: data.routing_chain_max_depth, }, }).unwrap();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/workspace/config/views/modelSettingsView.tsx` around lines 65 - 85, The onSubmit handler uses a non-null assertion on clientConfig when building the payload for updateCoreConfig; instead guard against undefined clientConfig (and bifrostConfig.framework_config) by either returning early with an error/toast if clientConfig is missing or by supplying a safe fallback object (e.g., {} or default routing values) before calling updateCoreConfig in onSubmit; update the payload construction in onSubmit to reference a local safeClientConfig and safeFrameworkConfig (derived from clientConfig and bifrostConfig.framework_config) so updateCoreConfig.unwrap() never receives a payload with client_config set to undefined.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@plugins/governance/main.go`:
- Around line 271-280: InitFromStore currently passes routingChainMaxDepth
(which may be nil if config or config.RoutingChainMaxDepth is nil) into
NewRoutingEngine causing failures; ensure a default is applied before the call
by setting routingChainMaxDepth to a local default value when config == nil or
config.RoutingChainMaxDepth == nil (same default used elsewhere), e.g. compute
an int value and assign its address to routingChainMaxDepth, then call
NewRoutingEngine(governanceStore, logger, routingChainMaxDepth); update the
block around the routingChainMaxDepth declaration and the NewRoutingEngine
invocation to use this non-nil default.
- Around line 157-162: The variable routingChainMaxDepth is left nil when config
or config.RoutingChainMaxDepth is nil, causing NewRoutingEngine to fail; fix
this by assigning a sensible default (e.g., 10) whenever config == nil or
config.RoutingChainMaxDepth == nil so routingChainMaxDepth is always a non-nil
*int; locate where routingChainMaxDepth is declared/assigned and if nil set it
to a pointer to the default value before calling NewRoutingEngine (refer to
routingChainMaxDepth, config.RoutingChainMaxDepth, and NewRoutingEngine).
---
Nitpick comments:
In `@ui/app/workspace/config/pricing-config/page.tsx`:
- Line 3: Replace the relative import of ModelSettingsView with the
repository-standard alias import; locate the import statement that currently
reads "import ModelSettingsView from \"../views/modelSettingsView\"" and change
it to use the "@/..." alias path for ModelSettingsView so the module resolution
matches other UI files and maintains consistency with the project's import
conventions.
In `@ui/app/workspace/config/views/modelSettingsView.tsx`:
- Around line 163-176: The routing_chain_max_depth error paragraph is rendered
outside the input's bordered container causing visual inconsistency; move the
conditional rendering of errors.routing_chain_max_depth so the <p
className="text-destructive text-sm">...</p> is inside the same parent container
that wraps the Input (the block containing Input with id
"routing-chain-max-depth") to match how other fields render their errors,
ensuring you keep the same class names and the register call for
routing_chain_max_depth unchanged.
- Around line 65-85: The onSubmit handler uses a non-null assertion on
clientConfig when building the payload for updateCoreConfig; instead guard
against undefined clientConfig (and bifrostConfig.framework_config) by either
returning early with an error/toast if clientConfig is missing or by supplying a
safe fallback object (e.g., {} or default routing values) before calling
updateCoreConfig in onSubmit; update the payload construction in onSubmit to
reference a local safeClientConfig and safeFrameworkConfig (derived from
clientConfig and bifrostConfig.framework_config) so updateCoreConfig.unwrap()
never receives a payload with client_config set to undefined.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 74b8a878-166b-44af-aaab-ea3d949d5887
📒 Files selected for processing (22)
docs/providers/routing-rules.mdxframework/configstore/clientconfig.goframework/configstore/migrations.goframework/configstore/rdb.goframework/configstore/tables/clientconfig.goframework/configstore/tables/routing_rules.goplugins/governance/main.goplugins/governance/routing.goplugins/governance/routing_test.gotransports/bifrost-http/handlers/config.gotransports/bifrost-http/handlers/governance.gotransports/bifrost-http/lib/config.gotransports/bifrost-http/server/plugins.goui/app/workspace/config/pricing-config/page.tsxui/app/workspace/config/views/modelSettingsView.tsxui/app/workspace/custom-pricing/page.tsxui/app/workspace/routing-rules/views/routingRuleInfoSheet.tsxui/app/workspace/routing-rules/views/routingRuleSheet.tsxui/app/workspace/routing-rules/views/routingRulesTable.tsxui/components/sidebar.tsxui/lib/types/config.tsui/lib/types/routingRules.ts
✅ Files skipped from review due to trivial changes (2)
- ui/app/workspace/custom-pricing/page.tsx
- framework/configstore/tables/routing_rules.go
🚧 Files skipped from review as they are similar to previous changes (8)
- ui/app/workspace/routing-rules/views/routingRuleInfoSheet.tsx
- framework/configstore/clientconfig.go
- ui/app/workspace/routing-rules/views/routingRuleSheet.tsx
- ui/app/workspace/routing-rules/views/routingRulesTable.tsx
- ui/lib/types/routingRules.ts
- docs/providers/routing-rules.mdx
- framework/configstore/migrations.go
- transports/bifrost-http/handlers/governance.go
Pratham-Mishra04
force-pushed
the
03-26-feat_adds_chaining_to_routing_rules
branch
from
c5a4a39 to
65f2ccc
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
plugins/governance/routing_test.go (1)
197-197: Consider centralizing test chain-depth setup to reduce repetition.These repeated
schemas.Ptr(10)literals are correct, but a shared test constant/helper would make future constructor changes less noisy.♻️ Suggested refactor
+const testRoutingChainMaxDepth = 10 + +func newTestRoutingEngine(t *testing.T, store *LocalGovernanceStore) *RoutingEngine { + t.Helper() + engine, err := NewRoutingEngine(store, NewMockLogger(), schemas.Ptr(testRoutingChainMaxDepth)) + require.NoError(t, err) + return engine +}Then replace per-test constructor blocks with
engine := newTestRoutingEngine(t, store).Also applies to: 210-210, 231-231, 282-282, 351-351, 415-415, 488-488, 511-511, 530-530, 582-582, 626-626, 662-662
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/governance/routing_test.go` at line 197, Multiple tests repeatedly pass schemas.Ptr(10) into NewRoutingEngine; create a small test helper to centralize the chain-depth setup (e.g., add a newTestRoutingEngine helper that takes testing.T and store and calls NewRoutingEngine(store, NewMockLogger(), schemas.Ptr(10))) and replace each per-test constructor call like NewRoutingEngine(store, NewMockLogger(), schemas.Ptr(10)) with a single call to newTestRoutingEngine(t, store) to reduce repetition and make future constructor or default depth changes easier.ui/app/workspace/routing-rules/views/routingRulesView.tsx (1)
74-77: ClearselectedRulewhen the info sheet closes to avoid stale state carryover.Not a blocker, but resetting selection on close avoids stale data being retained between opens.
♻️ Suggested change
+ const handleInfoSheetOpenChange = (open: boolean) => { + setInfoSheetOpen(open); + if (!open) setSelectedRule(null); + }; ... - <RoutingRuleInfoSheet rule={selectedRule} open={infoSheetOpen} onOpenChange={setInfoSheetOpen} /> + <RoutingRuleInfoSheet rule={selectedRule} open={infoSheetOpen} onOpenChange={handleInfoSheetOpenChange} />Also applies to: 134-134
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/workspace/routing-rules/views/routingRulesView.tsx` around lines 74 - 77, The info sheet close path currently only toggles setInfoSheetOpen(false) and leaves selectedRule set, which can cause stale data; update the close handlers (the code that calls setInfoSheetOpen(false) — e.g., the counterpart to handleRowClick and any close callbacks referenced around handleRowClick and the other occurrence at line ~134) to also call setSelectedRule(null) (or the appropriate empty state) so selectedRule is cleared whenever the info sheet is closed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@framework/configstore/clientconfig.go`:
- Around line 122-125: The condition around hashing RoutingChainMaxDepth
currently uses "> 0" which still includes the default value (10) and causes
default-value churn; change the check to only hash when the value differs from
the default (e.g., c.RoutingChainMaxDepth != 10 or, better, compare against the
package default constant if one exists such as DefaultRoutingChainMaxDepth),
leaving the hash.Write call for
"routingChainMaxDepth:"+strconv.Itoa(c.RoutingChainMaxDepth) unchanged so only
non-default values affect the config hash.
In `@ui/app/workspace/routing-rules/views/routingRuleInfoSheet.tsx`:
- Around line 162-165: The UI currently shows each target as 0% when the sum of
weights is zero, which is misleading; update the percentage calculation in
TargetCard (the weightPercent logic) to detect an all-zero total and instead
show uniform percentages (e.g., divide 100 by the number of targets and round)
so the sheet mirrors runtime uniform-random selection; apply the same fix to the
other places that compute/display percentages using the same pattern (the other
weightPercent-like calculations) so they also display equal shares when total
weight is zero.
- Around line 56-79: The CopyButton component renders an icon-only Button which
is missing an accessible name and the repo-required data-testid; update
CopyButton to provide an accessible label (e.g., add aria-label or include
visually-hidden text derived from the label prop) and add a data-testid
attribute following the pattern data-testid="<entity>-copy-button-<qualifier>"
on the Button element; apply the same fixes to the other icon-only copy controls
in this file (the other Copy button usages around the two similar blocks) so
every interactive copy control has an accessible name and the required
data-testid.
---
Nitpick comments:
In `@plugins/governance/routing_test.go`:
- Line 197: Multiple tests repeatedly pass schemas.Ptr(10) into
NewRoutingEngine; create a small test helper to centralize the chain-depth setup
(e.g., add a newTestRoutingEngine helper that takes testing.T and store and
calls NewRoutingEngine(store, NewMockLogger(), schemas.Ptr(10))) and replace
each per-test constructor call like NewRoutingEngine(store, NewMockLogger(),
schemas.Ptr(10)) with a single call to newTestRoutingEngine(t, store) to reduce
repetition and make future constructor or default depth changes easier.
In `@ui/app/workspace/routing-rules/views/routingRulesView.tsx`:
- Around line 74-77: The info sheet close path currently only toggles
setInfoSheetOpen(false) and leaves selectedRule set, which can cause stale data;
update the close handlers (the code that calls setInfoSheetOpen(false) — e.g.,
the counterpart to handleRowClick and any close callbacks referenced around
handleRowClick and the other occurrence at line ~134) to also call
setSelectedRule(null) (or the appropriate empty state) so selectedRule is
cleared whenever the info sheet is closed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bd39226d-237b-4791-9f22-0684aa06b2ea
📒 Files selected for processing (23)
docs/providers/routing-rules.mdxframework/configstore/clientconfig.goframework/configstore/migrations.goframework/configstore/rdb.goframework/configstore/tables/clientconfig.goframework/configstore/tables/routing_rules.goplugins/governance/main.goplugins/governance/routing.goplugins/governance/routing_test.gotransports/bifrost-http/handlers/config.gotransports/bifrost-http/handlers/governance.gotransports/bifrost-http/lib/config.gotransports/bifrost-http/server/plugins.goui/app/workspace/config/pricing-config/page.tsxui/app/workspace/config/views/modelSettingsView.tsxui/app/workspace/custom-pricing/page.tsxui/app/workspace/routing-rules/views/routingRuleInfoSheet.tsxui/app/workspace/routing-rules/views/routingRuleSheet.tsxui/app/workspace/routing-rules/views/routingRulesTable.tsxui/app/workspace/routing-rules/views/routingRulesView.tsxui/components/sidebar.tsxui/lib/types/config.tsui/lib/types/routingRules.ts
✅ Files skipped from review due to trivial changes (3)
- ui/app/workspace/config/pricing-config/page.tsx
- ui/app/workspace/custom-pricing/page.tsx
- ui/components/sidebar.tsx
🚧 Files skipped from review as they are similar to previous changes (12)
- transports/bifrost-http/server/plugins.go
- ui/lib/types/config.ts
- transports/bifrost-http/handlers/config.go
- framework/configstore/tables/routing_rules.go
- ui/lib/types/routingRules.ts
- ui/app/workspace/config/views/modelSettingsView.tsx
- ui/app/workspace/routing-rules/views/routingRuleSheet.tsx
- docs/providers/routing-rules.mdx
- ui/app/workspace/routing-rules/views/routingRulesTable.tsx
- transports/bifrost-http/lib/config.go
- transports/bifrost-http/handlers/governance.go
- framework/configstore/migrations.go
Pratham-Mishra04
force-pushed
the
03-26-feat_adds_chaining_to_routing_rules
branch
from
65f2ccc to
f711c09
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@framework/configstore/migrations.go`:
- Around line 5410-5464: The migration currently only backfills client config
hashes when !mg.HasColumn("routing_chain_max_depth"), which skips recompute if
the column already exists and can leave stale hashes; change the code so the
recompute loop that fetches tables.TableClientConfig, builds ClientConfig and
calls GenerateClientConfigHash() and tx.Model(&cc).Update("config_hash", ...)
runs unconditionally (or at least after ensuring the column exists) rather than
nested inside the mg.HasColumn check—mirror the
migrationAddChainRuleColumnToRoutingRules pattern: always perform the hash
recompute step using the existing tx.Find and update logic, keeping the
AddColumn call (mg.AddColumn) only when needed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a4a24645-2049-415f-ac14-5d6701dc2a8a
📒 Files selected for processing (23)
docs/providers/routing-rules.mdxframework/configstore/clientconfig.goframework/configstore/migrations.goframework/configstore/rdb.goframework/configstore/tables/clientconfig.goframework/configstore/tables/routing_rules.goplugins/governance/main.goplugins/governance/routing.goplugins/governance/routing_test.gotransports/bifrost-http/handlers/config.gotransports/bifrost-http/handlers/governance.gotransports/bifrost-http/lib/config.gotransports/bifrost-http/server/plugins.goui/app/workspace/config/pricing-config/page.tsxui/app/workspace/config/views/modelSettingsView.tsxui/app/workspace/custom-pricing/page.tsxui/app/workspace/routing-rules/views/routingRuleInfoSheet.tsxui/app/workspace/routing-rules/views/routingRuleSheet.tsxui/app/workspace/routing-rules/views/routingRulesTable.tsxui/app/workspace/routing-rules/views/routingRulesView.tsxui/components/sidebar.tsxui/lib/types/config.tsui/lib/types/routingRules.ts
✅ Files skipped from review due to trivial changes (6)
- framework/configstore/tables/clientconfig.go
- ui/app/workspace/config/pricing-config/page.tsx
- ui/app/workspace/custom-pricing/page.tsx
- ui/lib/types/config.ts
- framework/configstore/rdb.go
- ui/app/workspace/routing-rules/views/routingRuleInfoSheet.tsx
🚧 Files skipped from review as they are similar to previous changes (12)
- transports/bifrost-http/handlers/config.go
- transports/bifrost-http/server/plugins.go
- plugins/governance/routing_test.go
- ui/components/sidebar.tsx
- ui/lib/types/routingRules.ts
- transports/bifrost-http/lib/config.go
- ui/app/workspace/config/views/modelSettingsView.tsx
- ui/app/workspace/routing-rules/views/routingRulesTable.tsx
- transports/bifrost-http/handlers/governance.go
- framework/configstore/clientconfig.go
- docs/providers/routing-rules.mdx
- plugins/governance/routing.go
roroghost17
force-pushed
the
03-26-feat_adds_chaining_to_routing_rules
branch
from
f711c09 to
cc24e7c
Compare
10 tasks
roroghost17
force-pushed
the
03-26-feat_adds_chaining_to_routing_rules
branch
from
cc24e7c to
40a3b58
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
transports/bifrost-http/server/plugins.go (1)
176-181:⚠️ Potential issue | 🟠 MajorMirror the new depth wiring in
loadBuiltinPlugin().This only fixes the bulk bootstrap path.
InstantiatePlugin()still goes throughloadBuiltinPlugin()for single-plugin create/update, and that governance branch never injectsClientConfig.RoutingChainMaxDepth, so governance re-init can still see a nil or stale depth pointer there.🛠️ Suggested fix
case governance.PluginName: governanceConfig, err := MarshalPluginConfig[governance.Config](pluginConfig) if err != nil { return nil, fmt.Errorf("failed to marshal governance plugin config: %w", err) } + if governanceConfig == nil { + governanceConfig = &governance.Config{} + } + governanceConfig.RoutingChainMaxDepth = &bifrostConfig.ClientConfig.RoutingChainMaxDepth inMemoryStore := &GovernanceInMemoryStore{Config: bifrostConfig} return governance.Init(ctx, governanceConfig, logger, bifrostConfig.ConfigStore, bifrostConfig.GovernanceConfig, bifrostConfig.ModelCatalog, bifrostConfig.MCPCatalog, inMemoryStore)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@transports/bifrost-http/server/plugins.go` around lines 176 - 181, The governance.Config.RoutingChainMaxDepth pointer is not being set in loadBuiltinPlugin(), so InstantiatePlugin() (which calls loadBuiltinPlugin()) can re-init governance with a nil/stale depth; update loadBuiltinPlugin() to read s.Config.ClientConfig.RoutingChainMaxDepth and assign its address into the governance.Config (same way other ClientConfig fields are wired) so that governance initialization always receives the current RoutingChainMaxDepth when InstantiatePlugin() triggers plugin create/update.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ui/app/workspace/routing-rules/views/routingRulesTable.tsx`:
- Around line 141-189: The TableRow has onRowClick but a <tr> isn't
keyboard-focusable; move the click handler to a keyboard-accessible element by
wrapping the row details content inside a button in the first cell (the element
that currently renders rule.name and rule.description) and call onRowClick(rule)
from that button; ensure the button has the required data-testid (e.g.,
data-testid="routing-rule-row-open") and preserves visual styling/ARIA (keep the
existing truncate/title behavior and stopPropagation on the action cell buttons
so Edit/Delete still work); update references around sortedRules mapping,
onRowClick, and the first TableCell to implement this change.
In `@ui/lib/types/routingRules.ts`:
- Line 26: The runtime routing-rule schema is missing the new chain_rule field
so create/edit validation strips it; update the routing rule schemas (e.g.,
routingRuleSchema / createRoutingRuleSchema / editRoutingRuleSchema and any
routingRuleDefaults) to include chain_rule:boolean (default false) and ensure it
is passed through any pick/omit/transform steps used during validation or
serialization so the field survives the create/edit flows and reaches the API.
---
Outside diff comments:
In `@transports/bifrost-http/server/plugins.go`:
- Around line 176-181: The governance.Config.RoutingChainMaxDepth pointer is not
being set in loadBuiltinPlugin(), so InstantiatePlugin() (which calls
loadBuiltinPlugin()) can re-init governance with a nil/stale depth; update
loadBuiltinPlugin() to read s.Config.ClientConfig.RoutingChainMaxDepth and
assign its address into the governance.Config (same way other ClientConfig
fields are wired) so that governance initialization always receives the current
RoutingChainMaxDepth when InstantiatePlugin() triggers plugin create/update.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 604b199d-bb31-4164-8b5d-3f0623cb4505
📒 Files selected for processing (23)
docs/providers/routing-rules.mdxframework/configstore/clientconfig.goframework/configstore/migrations.goframework/configstore/rdb.goframework/configstore/tables/clientconfig.goframework/configstore/tables/routing_rules.goplugins/governance/main.goplugins/governance/routing.goplugins/governance/routing_test.gotransports/bifrost-http/handlers/config.gotransports/bifrost-http/handlers/governance.gotransports/bifrost-http/lib/config.gotransports/bifrost-http/server/plugins.goui/app/workspace/config/pricing-config/page.tsxui/app/workspace/config/views/modelSettingsView.tsxui/app/workspace/custom-pricing/page.tsxui/app/workspace/routing-rules/views/routingRuleInfoSheet.tsxui/app/workspace/routing-rules/views/routingRuleSheet.tsxui/app/workspace/routing-rules/views/routingRulesTable.tsxui/app/workspace/routing-rules/views/routingRulesView.tsxui/components/sidebar.tsxui/lib/types/config.tsui/lib/types/routingRules.ts
✅ Files skipped from review due to trivial changes (3)
- ui/app/workspace/config/pricing-config/page.tsx
- ui/app/workspace/custom-pricing/page.tsx
- ui/components/sidebar.tsx
🚧 Files skipped from review as they are similar to previous changes (14)
- framework/configstore/tables/clientconfig.go
- ui/lib/types/config.ts
- transports/bifrost-http/handlers/config.go
- plugins/governance/main.go
- framework/configstore/rdb.go
- ui/app/workspace/routing-rules/views/routingRulesView.tsx
- ui/app/workspace/routing-rules/views/routingRuleSheet.tsx
- transports/bifrost-http/lib/config.go
- transports/bifrost-http/handlers/governance.go
- ui/app/workspace/config/views/modelSettingsView.tsx
- plugins/governance/routing_test.go
- framework/configstore/migrations.go
- docs/providers/routing-rules.mdx
- plugins/governance/routing.go
Pratham-Mishra04
force-pushed
the
03-26-feat_adds_chaining_to_routing_rules
branch
from
40a3b58 to
4c9405a
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
ui/app/workspace/config/views/modelSettingsView.tsx (2)
155-177: Minor layout inconsistency: error message outside container.The error message for
routing_chain_max_depth(line 176) renders outside the bordered container, unlike other fields where errors appear inside. This creates visual inconsistency.🧹 Suggested fix to move error inside container
<div className="flex items-center justify-between rounded-sm border p-4"> <div className="space-y-0.5"> <Label htmlFor="routing-chain-max-depth">Routing Chain Max Depth</Label> <p className="text-muted-foreground text-sm"> Maximum number of chained routing rule evaluations per request. Prevents infinite loops from circular rule definitions. </p> + {errors.routing_chain_max_depth && <p className="text-destructive text-sm">{errors.routing_chain_max_depth.message}</p>} </div> <Input id="routing-chain-max-depth" type="number" className={`w-24 ${errors.routing_chain_max_depth ? "border-destructive" : ""}`} data-testid="routing-chain-max-depth-input" {...register("routing_chain_max_depth", { required: "Routing chain max depth is required", min: { value: 1, message: "Must be at least 1" }, max: { value: 100, message: "Cannot exceed 100" }, valueAsNumber: true, })} /> </div> -{errors.routing_chain_max_depth && <p className="text-destructive text-sm">{errors.routing_chain_max_depth.message}</p>}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/workspace/config/views/modelSettingsView.tsx` around lines 155 - 177, The error paragraph for errors.routing_chain_max_depth is placed outside the bordered container causing layout inconsistency; move the conditional JSX that renders <p className="text-destructive text-sm"> (the errors.routing_chain_max_depth.message) so it sits inside the same rounded-sm border/p-4 div that contains the Label and Input (i.e., include it alongside or immediately below the Input within that container) to match other field layouts and preserve styling consistency for the Routing Chain Max Depth field.
139-153: Missingdata-testidon pricing sync interval input.The
pricing-sync-intervalinput is missing adata-testidattribute. Other inputs in this form have them (e.g.,pricing-datasheet-url-input,routing-chain-max-depth-input).🧹 Suggested fix
<Input id="pricing-sync-interval" type="number" className={errors.pricing_sync_interval_hours ? "border-destructive" : ""} + data-testid="pricing-sync-interval-input" {...register("pricing_sync_interval_hours", {As per coding guidelines:
ui/**/*.{tsx,ts}: Add new interactive UI elements withdata-testidattributes following the pattern.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/workspace/config/views/modelSettingsView.tsx` around lines 139 - 153, The pricing sync interval Input (id="pricing-sync-interval", registered as "pricing_sync_interval_hours" and referenced by errors.pricing_sync_interval_hours) is missing a data-testid; add a data-testid attribute following the existing pattern (e.g., data-testid="pricing-sync-interval-input") to the <Input /> element so tests can reliably select it.ui/app/workspace/routing-rules/views/routingRuleInfoSheet.tsx (1)
170-170: Unusedindexprop in TargetCard.The
TargetCardfunction signature includesindexin its props destructuring, and it's passed at line 330, but theindexparameter is never used within the component body. Consider removing it to avoid confusion.Proposed fix
-function TargetCard({ target, index, total }: { target: RoutingRule["targets"][0]; index: number; total: number }) { +function TargetCard({ target, total }: { target: RoutingRule["targets"][0]; total: number }) {And at the call site:
-<TargetCard key={i} target={target} index={i} total={targets.length} /> +<TargetCard key={i} target={target} total={targets.length} />Also applies to: 330-330
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/workspace/routing-rules/views/routingRuleInfoSheet.tsx` at line 170, The TargetCard component declares and destructures an unused index prop which should be removed to avoid confusion; update the TargetCard signature to accept only { target, total } (keeping the existing type for target: RoutingRule["targets"][0]) and remove index from its props type, then remove the index argument at the call site where TargetCard(...) is invoked so only target and total are passed.
🤖 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/config/views/modelSettingsView.tsx`:
- Around line 155-177: The error paragraph for errors.routing_chain_max_depth is
placed outside the bordered container causing layout inconsistency; move the
conditional JSX that renders <p className="text-destructive text-sm"> (the
errors.routing_chain_max_depth.message) so it sits inside the same rounded-sm
border/p-4 div that contains the Label and Input (i.e., include it alongside or
immediately below the Input within that container) to match other field layouts
and preserve styling consistency for the Routing Chain Max Depth field.
- Around line 139-153: The pricing sync interval Input
(id="pricing-sync-interval", registered as "pricing_sync_interval_hours" and
referenced by errors.pricing_sync_interval_hours) is missing a data-testid; add
a data-testid attribute following the existing pattern (e.g.,
data-testid="pricing-sync-interval-input") to the <Input /> element so tests can
reliably select it.
In `@ui/app/workspace/routing-rules/views/routingRuleInfoSheet.tsx`:
- Line 170: The TargetCard component declares and destructures an unused index
prop which should be removed to avoid confusion; update the TargetCard signature
to accept only { target, total } (keeping the existing type for target:
RoutingRule["targets"][0]) and remove index from its props type, then remove the
index argument at the call site where TargetCard(...) is invoked so only target
and total are passed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5f6988be-640d-4578-bcc0-73af90222c3a
📒 Files selected for processing (25)
docs/providers/routing-rules.mdxframework/configstore/clientconfig.goframework/configstore/migrations.goframework/configstore/rdb.goframework/configstore/tables/clientconfig.goframework/configstore/tables/routing_rules.goplugins/governance/main.goplugins/governance/routing.goplugins/governance/routing_test.gotransports/bifrost-http/handlers/config.gotransports/bifrost-http/handlers/governance.gotransports/bifrost-http/lib/config.gotransports/bifrost-http/server/plugins.goui/app/workspace/config/pricing-config/page.tsxui/app/workspace/config/views/modelSettingsView.tsxui/app/workspace/custom-pricing/page.tsxui/app/workspace/routing-rules/views/routingRuleInfoSheet.tsxui/app/workspace/routing-rules/views/routingRuleSheet.tsxui/app/workspace/routing-rules/views/routingRulesTable.tsxui/app/workspace/routing-rules/views/routingRulesView.tsxui/components/sidebar.tsxui/lib/config/celFieldsRouting.tsui/lib/types/config.tsui/lib/types/routingRules.tsui/lib/types/schemas.ts
✅ Files skipped from review due to trivial changes (6)
- ui/app/workspace/config/pricing-config/page.tsx
- framework/configstore/tables/clientconfig.go
- ui/lib/config/celFieldsRouting.ts
- ui/app/workspace/custom-pricing/page.tsx
- framework/configstore/rdb.go
- framework/configstore/migrations.go
🚧 Files skipped from review as they are similar to previous changes (8)
- ui/lib/types/config.ts
- plugins/governance/routing_test.go
- plugins/governance/main.go
- ui/lib/types/routingRules.ts
- transports/bifrost-http/handlers/config.go
- transports/bifrost-http/lib/config.go
- plugins/governance/routing.go
- docs/providers/routing-rules.mdx
roroghost17
force-pushed
the
03-26-feat_adds_chaining_to_routing_rules
branch
from
4c9405a to
87e4dbb
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
ui/app/workspace/routing-rules/views/routingRulesTable.tsx (1)
141-142:⚠️ Potential issue | 🟠 MajorKeyboard users cannot access row click action.
The
onClickhandler on<TableRow>makes rows mouse-clickable, but<tr>elements are not keyboard-focusable by default. This means keyboard users cannot open the details sheet. The previous review flagged this and suggested moving the handler to a focusable element (like a button in the first cell).Consider wrapping the name/description content in a
<button>to ensure keyboard accessibility, or addingtabIndex={0}withonKeyDownhandling for Enter/Space activation.♿ Suggested approach
-<TableRow key={rule.id} className="hover:bg-muted/50 cursor-pointer transition-colors" onClick={() => onRowClick(rule)}> +<TableRow key={rule.id} className="hover:bg-muted/50 transition-colors"> <TableCell className="font-medium"> - <div className="flex flex-col gap-1"> + <button + type="button" + className="flex w-full flex-col gap-1 text-left bg-transparent border-none p-0 cursor-pointer" + onClick={() => onRowClick(rule)} + aria-label={`View details for ${rule.name}`} + data-testid={`routing-rule-row-${rule.id}-open-btn`} + > <span className="truncate max-w-xs">{rule.name}</span> {rule.description && ( <span data-testid="routing-rule-description" className="text-xs text-muted-foreground truncate max-w-xs">{rule.description}</span> )} - </div> + </button> </TableCell>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/workspace/routing-rules/views/routingRulesTable.tsx` around lines 141 - 142, The TableRow currently uses an onClick handler inside sortedRules.map which makes rows only mouse-clickable (TableRow / <tr> not keyboard-focusable); update the row to provide keyboard access by moving the activation to a focusable element (e.g., render a <button> inside the first cell that contains the name/description and invoke onRowClick(rule) from that button), or if you prefer keep the handler on the row add tabIndex={0} and an onKeyDown that triggers onRowClick(rule) for Enter/Space; ensure the visual styles remain the same and remove duplicate click handlers so activation is only via the accessible control.
🧹 Nitpick comments (3)
ui/app/workspace/routing-rules/views/routingRuleInfoSheet.tsx (3)
170-172: Remove unusedindexparameter fromTargetCard.The
indexparameter is declared in the function signature but never used within the component body. Either remove it or use it if intended for future purposes (e.g., display numbering).♻️ Suggested fix
-function TargetCard({ target, index, total }: { target: RoutingRule["targets"][0]; index: number; total: number }) { +function TargetCard({ target, total }: { target: RoutingRule["targets"][0]; total: number }) {And at the call site (line 330):
-<TargetCard key={i} target={target} index={i} total={targets.length} /> +<TargetCard key={i} target={target} total={targets.length} />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/workspace/routing-rules/views/routingRuleInfoSheet.tsx` around lines 170 - 172, The TargetCard component declares an unused index parameter; remove index from the TargetCard signature (change function TargetCard({ target, total }: { target: RoutingRule["targets"][0]; total: number }) ) and update all call sites that pass an index to stop supplying that argument (where TargetCard is rendered, remove the index prop/argument), leaving target and total intact; ensure any imports/utility calls like getProviderLabel remain unchanged.
353-365: Guard against invalid date strings in timestamp rendering.If
rule.created_atorrule.updated_atcontain invalid date strings,new Date(...)will produce anInvalid Dateobject, andformatDistanceToNowwill throw or display unexpected output. Consider adding defensive checks.🛡️ Suggested defensive approach
+function safeFormatDistance(dateStr: string): string { + const date = new Date(dateStr); + if (isNaN(date.getTime())) return "Unknown"; + return formatDistanceToNow(date, { addSuffix: true }); +} + // Then in the JSX: -{formatDistanceToNow(new Date(rule.created_at), { addSuffix: true })} +{safeFormatDistance(rule.created_at)}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/workspace/routing-rules/views/routingRuleInfoSheet.tsx` around lines 353 - 365, The timestamp rendering in routingRuleInfoSheet.tsx uses formatDistanceToNow(new Date(rule.created_at)) and similarly for rule.updated_at without validating the Date, so add defensive checks: validate the parsed Date (e.g., using date-fns isValid or checking !isNaN(date.getTime())) before calling formatDistanceToNow and render a safe fallback (like a placeholder "—" or "Invalid date") when the timestamp is invalid; update the two occurrences that reference rule.created_at and rule.updated_at and keep formatDistanceToNow only for valid Date objects.
97-106: Complex ternary chain reduces readability.The
bareKeyValueassignment spans multiple lines with nested ternaries and inline object creation. Consider extracting this to a small helper or simplifying with early returns.♻️ Suggested extraction
function parseBareKeyValue(isHeader: boolean, isParam: boolean, keyMatch: RegExpMatchArray | null, value: string) { if (keyMatch || !(isHeader || isParam) || !value) return null; const colonIdx = value.indexOf(":"); if (colonIdx === -1) return { key: value, val: "" }; return { key: value.slice(0, colonIdx), val: value.slice(colonIdx + 1) }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/workspace/routing-rules/views/routingRuleInfoSheet.tsx` around lines 97 - 106, Extract the complex ternary that computes bareKeyValue into a small helper (e.g., parseBareKeyValue) and replace the inline expression in routingRuleInfoSheet.tsx; the helper should accept isHeader, isParam, keyMatch and value, return null if keyMatch is present or not a header/param or value is falsy, return { key: value, val: "" } when no colon is found, and otherwise split at the first colon to return { key, val }; then set bareKeyValue = parseBareKeyValue(isHeader, isParam, keyMatch, value) and keep keyName = keyMatch?.[1] ?? bareKeyValue?.key and displayValue = bareKeyValue !== null ? bareKeyValue.val : value.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ui/app/workspace/config/views/modelSettingsView.tsx`:
- Around line 43-51: The useEffect that calls reset(...) when bifrostConfig
changes (the block referencing bifrostConfig, frameworkConfig, clientConfig, and
reset) should not unconditionally reset the form if the form is dirty; add a
guard to skip reset when hasChanges is true. Also prevent triggering
forcePricingSync while the form is dirty or an update is pending by disabling or
short-circuiting the forcePricingSync invocation when hasChanges || isLoading
(or when updateCoreConfig is pending); update the logic around the
forcePricingSync call (the action at lines referencing forcePricingSync) to
check hasChanges || isLoading before dispatching.
- Around line 111-127: The current pattern-based validation for the Input bound
to pricing_datasheet_url is too restrictive; replace the regex check in the
register call with a URL parse-based validator: remove the pattern option and in
validate.checkIfHttp implement parsing via the built-in URL constructor (try {
new URL(value) } catch { return "Please enter a valid URL." }) and then ensure
url.protocol is "http:" or "https:" (return "URL must start with http:// or
https://") and allow empty values to pass; update any error message strings
accordingly so signed object-store URLs, query strings, fragments, ports and
percent-encoded paths are accepted.
- Around line 139-149: The new numeric Input for pricing sync interval (the JSX
Input with id "pricing-sync-interval" and
register("pricing_sync_interval_hours", ...)) is missing a data-testid; add
data-testid="pricing-sync-interval-input" to that Input element so it follows
the ui tests naming convention and matches other interactive controls in this
view.
In `@ui/app/workspace/routing-rules/views/routingRulesTable.tsx`:
- Around line 174-185: Add data-testid attributes to the Edit and Delete Button
components in the routingRulesTable rendering block: for the Edit button (the
Button with onClick={() => onEdit(rule)}) add data-testid following the pattern
"<entity>-edit-button-<qualifier>" (e.g., "routing-rule-edit-button" or include
rule.id as qualifier like "routing-rule-edit-button-<rule.id>"), and for the
Delete button (the Button that calls setDeleteRuleId(rule.id)) add data-testid
like "<entity>-delete-button-<qualifier>" (e.g., "routing-rule-delete-button" or
"routing-rule-delete-button-<rule.id>"); ensure both attributes follow the
<entity>-<element>-<qualifier> guideline and are added to the respective Button
JSX elements near their aria-labels.
---
Duplicate comments:
In `@ui/app/workspace/routing-rules/views/routingRulesTable.tsx`:
- Around line 141-142: The TableRow currently uses an onClick handler inside
sortedRules.map which makes rows only mouse-clickable (TableRow / <tr> not
keyboard-focusable); update the row to provide keyboard access by moving the
activation to a focusable element (e.g., render a <button> inside the first cell
that contains the name/description and invoke onRowClick(rule) from that
button), or if you prefer keep the handler on the row add tabIndex={0} and an
onKeyDown that triggers onRowClick(rule) for Enter/Space; ensure the visual
styles remain the same and remove duplicate click handlers so activation is only
via the accessible control.
---
Nitpick comments:
In `@ui/app/workspace/routing-rules/views/routingRuleInfoSheet.tsx`:
- Around line 170-172: The TargetCard component declares an unused index
parameter; remove index from the TargetCard signature (change function
TargetCard({ target, total }: { target: RoutingRule["targets"][0]; total: number
}) ) and update all call sites that pass an index to stop supplying that
argument (where TargetCard is rendered, remove the index prop/argument), leaving
target and total intact; ensure any imports/utility calls like getProviderLabel
remain unchanged.
- Around line 353-365: The timestamp rendering in routingRuleInfoSheet.tsx uses
formatDistanceToNow(new Date(rule.created_at)) and similarly for rule.updated_at
without validating the Date, so add defensive checks: validate the parsed Date
(e.g., using date-fns isValid or checking !isNaN(date.getTime())) before calling
formatDistanceToNow and render a safe fallback (like a placeholder "—" or
"Invalid date") when the timestamp is invalid; update the two occurrences that
reference rule.created_at and rule.updated_at and keep formatDistanceToNow only
for valid Date objects.
- Around line 97-106: Extract the complex ternary that computes bareKeyValue
into a small helper (e.g., parseBareKeyValue) and replace the inline expression
in routingRuleInfoSheet.tsx; the helper should accept isHeader, isParam,
keyMatch and value, return null if keyMatch is present or not a header/param or
value is falsy, return { key: value, val: "" } when no colon is found, and
otherwise split at the first colon to return { key, val }; then set bareKeyValue
= parseBareKeyValue(isHeader, isParam, keyMatch, value) and keep keyName =
keyMatch?.[1] ?? bareKeyValue?.key and displayValue = bareKeyValue !== null ?
bareKeyValue.val : value.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 93a9ba97-7bc1-4fc3-a733-0e8fbf88b662
📒 Files selected for processing (25)
docs/providers/routing-rules.mdxframework/configstore/clientconfig.goframework/configstore/migrations.goframework/configstore/rdb.goframework/configstore/tables/clientconfig.goframework/configstore/tables/routing_rules.goplugins/governance/main.goplugins/governance/routing.goplugins/governance/routing_test.gotransports/bifrost-http/handlers/config.gotransports/bifrost-http/handlers/governance.gotransports/bifrost-http/lib/config.gotransports/bifrost-http/server/plugins.goui/app/workspace/config/pricing-config/page.tsxui/app/workspace/config/views/modelSettingsView.tsxui/app/workspace/custom-pricing/page.tsxui/app/workspace/routing-rules/views/routingRuleInfoSheet.tsxui/app/workspace/routing-rules/views/routingRuleSheet.tsxui/app/workspace/routing-rules/views/routingRulesTable.tsxui/app/workspace/routing-rules/views/routingRulesView.tsxui/components/sidebar.tsxui/lib/config/celFieldsRouting.tsui/lib/types/config.tsui/lib/types/routingRules.tsui/lib/types/schemas.ts
✅ Files skipped from review due to trivial changes (8)
- ui/app/workspace/custom-pricing/page.tsx
- ui/app/workspace/config/pricing-config/page.tsx
- framework/configstore/tables/clientconfig.go
- ui/lib/types/schemas.ts
- framework/configstore/tables/routing_rules.go
- framework/configstore/rdb.go
- ui/components/sidebar.tsx
- docs/providers/routing-rules.mdx
🚧 Files skipped from review as they are similar to previous changes (12)
- transports/bifrost-http/server/plugins.go
- plugins/governance/main.go
- ui/app/workspace/routing-rules/views/routingRuleSheet.tsx
- ui/lib/config/celFieldsRouting.ts
- plugins/governance/routing_test.go
- ui/lib/types/config.ts
- transports/bifrost-http/handlers/config.go
- ui/app/workspace/routing-rules/views/routingRulesView.tsx
- transports/bifrost-http/lib/config.go
- framework/configstore/migrations.go
- ui/lib/types/routingRules.ts
- plugins/governance/routing.go
Pratham-Mishra04
force-pushed
the
03-26-feat_adds_chaining_to_routing_rules
branch
from
87e4dbb to
2536c36
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Pratham-Mishra04
force-pushed
the
03-26-feat_adds_chaining_to_routing_rules
branch
from
2536c36 to
e4c3c3a
Compare
Pratham-Mishra04
force-pushed
the
03-26-feat_adds_chaining_to_routing_rules
branch
from
e4c3c3a to
8a27fe9
Compare
Contributor
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 (2)
ui/app/workspace/routing-rules/views/routingRulesTable.tsx (1)
253-257:⚠️ Potential issue | 🟡 MinorUse “Incoming …” here too, not “Any”.
The info sheet in this stack now treats blank target fields as “carry forward the current provider/model,” but this summary still says
"Any / Any model". Those two views are now describing different behavior for the same rule.🪄 Suggested fix
const label = [ - first.provider ? getProviderLabel(first.provider) : "Any", - first.model || "Any model", + first.provider ? getProviderLabel(first.provider) : "Incoming provider", + first.model || "Incoming model", ].join(" / ");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/workspace/routing-rules/views/routingRulesTable.tsx` around lines 253 - 257, Change the fallback text in the label construction so blank target fields show "Incoming" wording instead of "Any": in the block that computes label from targets[0] (variables first and label and function getProviderLabel), replace the fallbacks "Any" and "Any model" with "Incoming provider" (or "Incoming") and "Incoming model" respectively so the summary matches the info sheet's "carry forward the current provider/model" behavior.ui/app/workspace/routing-rules/views/routingRuleInfoSheet.tsx (1)
221-229:⚠️ Potential issue | 🟡 MinorDon’t feed the placeholder label into
RenderProviderIcon.When a fallback omits its provider segment,
providerbecomes"Incoming provider", but Line 228 still casts that display string toProviderIconType. That means provider-less fallbacks can take the wrong icon path instead of simply rendering no icon.🩹 Suggested fix
- const provider = parts[0] || "Incoming provider"; + const rawProvider = parts[0]; + const provider = rawProvider || "Incoming provider"; const model = parts.length > 1 ? parts.slice(1).join("/") : "Incoming model"; @@ - {provider && <RenderProviderIcon provider={provider as ProviderIconType} size="sm" className="h-3.5 w-3.5 shrink-0" />} + {rawProvider && ( + <RenderProviderIcon + provider={rawProvider as ProviderIconType} + size="sm" + className="h-3.5 w-3.5 shrink-0" + /> + )}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/workspace/routing-rules/views/routingRuleInfoSheet.tsx` around lines 221 - 229, The current code casts the display placeholder "Incoming provider" (variable provider) to ProviderIconType and passes it to RenderProviderIcon, causing wrong icons for provider-less fallbacks; change the icon rendering to only occur when the original provider segment exists (use parts[0] or a new providerRaw variable) and avoid casting the placeholder to ProviderIconType—i.e., keep provider/model fallback strings for display but gate the RenderProviderIcon call on the real provider segment (parts[0]) so icon rendering only happens for actual providers.
♻️ Duplicate comments (2)
ui/app/workspace/routing-rules/views/routingRulesTable.tsx (1)
142-149:⚠️ Potential issue | 🟠 MajorMake the row opener keyboard-accessible.
This stacked flow now relies on row click to open
RoutingRuleInfoSheet, but Line 142 binds that action to<TableRow>, which keyboard users cannot focus or activate. Move the opener into a real button in the first cell and give that control its owndata-testid.♿ Suggested fix
- <TableRow key={rule.id} className="hover:bg-muted/50 cursor-pointer transition-colors" onClick={() => onRowClick(rule)}> + <TableRow key={rule.id} className="hover:bg-muted/50 transition-colors"> <TableCell className="font-medium"> - <div className="flex flex-col gap-1"> + <button + type="button" + className="flex w-full flex-col gap-1 text-left" + onClick={() => onRowClick(rule)} + aria-label={`Open routing rule ${rule.name}`} + data-testid={`routing-rule-open-${rule.id}`} + > <span className="truncate max-w-xs">{rule.name}</span> {rule.description && ( <span data-testid="routing-rule-description" className="text-xs text-muted-foreground truncate max-w-xs">{rule.description}</span> )} - </div> + </button> </TableCell>As per coding guidelines,
ui/**/*.{tsx,ts}should “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/app/workspace/routing-rules/views/routingRulesTable.tsx` around lines 142 - 149, The TableRow currently uses onClick (onRowClick) making the opener inaccessible to keyboard users; move the click handler into a real <button> placed inside the first TableCell (wrap the rule.name and description), give that button a data-testid following the pattern (e.g., data-testid="routing-rule-open-button"), ensure the button is focusable and styled to match the row (full-width/transparent background/hover styles) and calls onRowClick(rule) (or opens RoutingRuleInfoSheet) on activation, and add an accessible label (aria-label or use the visible rule.name) so keyboard and screen-reader users can activate the opener.ui/app/workspace/config/views/modelSettingsView.tsx (1)
122-130:⚠️ Potential issue | 🟠 MajorReplace the URL regex validator (performance + correctness risk).
This regex is still vulnerable to expensive backtracking and remains overly restrictive for valid HTTP(S) URLs (query strings/fragments/signed URLs). Prefer
URLparsing + protocol validation instead.💡 Proposed fix
- <Input - id="pricing-datasheet-url" - type="text" + <Input + id="pricing-datasheet-url" + type="url" placeholder="https://example.com/pricing.json" data-testid="pricing-datasheet-url-input" {...register("pricing_datasheet_url", { - pattern: { - value: /^(https?:\/\/)?((localhost|(\d{1,3}\.){3}\d{1,3})(:\d+)?|([\da-z\.-]+)\.([a-z\.]{2,6}))([\/\w \.-]*)*\/?$/, - message: "Please enter a valid URL.", - }, - validate: { - checkIfHttp: (value) => { - if (!value) return true; - return value.startsWith("http://") || value.startsWith("https://") || "URL must start with http:// or https://"; - }, - }, + validate: (value) => { + if (!value) return true; + try { + const parsed = new URL(value); + return ["http:", "https:"].includes(parsed.protocol) || "URL must start with http:// or https://"; + } catch { + return "Please enter a valid URL."; + } + }, })}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/workspace/config/views/modelSettingsView.tsx` around lines 122 - 130, Remove the expensive/incorrect regex validator and replace it by using URL parsing in the validate checker: delete or stop using the "pattern" entry and update validate.checkIfHttp in modelSettingsView.tsx to accept empty values, then try new URL(value) inside a try/catch and ensure url.protocol is "http:" or "https:"; return true on success or the error string "URL must start with http:// or https://" (or a parse-failure message) on failure. This keeps validation in the validate.checkIfHttp function and avoids backtracking/regex issues while correctly allowing query strings/fragments/signed URLs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@plugins/governance/routing_test.go`:
- Around line 197-198: Add focused unit tests that directly exercise the routing
engine's chaining loop: construct the engine via NewRoutingEngine(...) and then
invoke the engine's evaluation loop (call the method that runs rule evaluation
in this codebase, e.g., engine.Evaluate/engine.Run/engine.ApplyRules as
appropriate) with controlled rules and inputs to assert four behaviors: (1)
chain_rule=true causes re-evaluation of dependent rules, (2) a terminal rule
halts further chaining, (3) convergence/no-op stops further iterations when no
state changes between iterations, and (4) max-depth cutoff stops after the
configured depth; create minimal mock rules or fixtures to trigger each case and
use assertions to check that re-evaluation, termination, convergence, and depth
limits occur as expected.
---
Outside diff comments:
In `@ui/app/workspace/routing-rules/views/routingRuleInfoSheet.tsx`:
- Around line 221-229: The current code casts the display placeholder "Incoming
provider" (variable provider) to ProviderIconType and passes it to
RenderProviderIcon, causing wrong icons for provider-less fallbacks; change the
icon rendering to only occur when the original provider segment exists (use
parts[0] or a new providerRaw variable) and avoid casting the placeholder to
ProviderIconType—i.e., keep provider/model fallback strings for display but gate
the RenderProviderIcon call on the real provider segment (parts[0]) so icon
rendering only happens for actual providers.
In `@ui/app/workspace/routing-rules/views/routingRulesTable.tsx`:
- Around line 253-257: Change the fallback text in the label construction so
blank target fields show "Incoming" wording instead of "Any": in the block that
computes label from targets[0] (variables first and label and function
getProviderLabel), replace the fallbacks "Any" and "Any model" with "Incoming
provider" (or "Incoming") and "Incoming model" respectively so the summary
matches the info sheet's "carry forward the current provider/model" behavior.
---
Duplicate comments:
In `@ui/app/workspace/config/views/modelSettingsView.tsx`:
- Around line 122-130: Remove the expensive/incorrect regex validator and
replace it by using URL parsing in the validate checker: delete or stop using
the "pattern" entry and update validate.checkIfHttp in modelSettingsView.tsx to
accept empty values, then try new URL(value) inside a try/catch and ensure
url.protocol is "http:" or "https:"; return true on success or the error string
"URL must start with http:// or https://" (or a parse-failure message) on
failure. This keeps validation in the validate.checkIfHttp function and avoids
backtracking/regex issues while correctly allowing query
strings/fragments/signed URLs.
In `@ui/app/workspace/routing-rules/views/routingRulesTable.tsx`:
- Around line 142-149: The TableRow currently uses onClick (onRowClick) making
the opener inaccessible to keyboard users; move the click handler into a real
<button> placed inside the first TableCell (wrap the rule.name and description),
give that button a data-testid following the pattern (e.g.,
data-testid="routing-rule-open-button"), ensure the button is focusable and
styled to match the row (full-width/transparent background/hover styles) and
calls onRowClick(rule) (or opens RoutingRuleInfoSheet) on activation, and add an
accessible label (aria-label or use the visible rule.name) so keyboard and
screen-reader users can activate the opener.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b91d8278-c261-4ea4-b006-e28d12582352
📒 Files selected for processing (25)
docs/providers/routing-rules.mdxframework/configstore/clientconfig.goframework/configstore/migrations.goframework/configstore/rdb.goframework/configstore/tables/clientconfig.goframework/configstore/tables/routing_rules.goplugins/governance/main.goplugins/governance/routing.goplugins/governance/routing_test.gotransports/bifrost-http/handlers/config.gotransports/bifrost-http/handlers/governance.gotransports/bifrost-http/lib/config.gotransports/bifrost-http/server/plugins.goui/app/workspace/config/pricing-config/page.tsxui/app/workspace/config/views/modelSettingsView.tsxui/app/workspace/custom-pricing/page.tsxui/app/workspace/routing-rules/views/routingRuleInfoSheet.tsxui/app/workspace/routing-rules/views/routingRuleSheet.tsxui/app/workspace/routing-rules/views/routingRulesTable.tsxui/app/workspace/routing-rules/views/routingRulesView.tsxui/components/sidebar.tsxui/lib/config/celFieldsRouting.tsui/lib/types/config.tsui/lib/types/routingRules.tsui/lib/types/schemas.ts
✅ Files skipped from review due to trivial changes (4)
- transports/bifrost-http/server/plugins.go
- framework/configstore/tables/clientconfig.go
- ui/app/workspace/custom-pricing/page.tsx
- framework/configstore/rdb.go
🚧 Files skipped from review as they are similar to previous changes (14)
- ui/app/workspace/config/pricing-config/page.tsx
- ui/lib/types/schemas.ts
- transports/bifrost-http/handlers/config.go
- framework/configstore/tables/routing_rules.go
- ui/lib/types/config.ts
- ui/components/sidebar.tsx
- ui/app/workspace/routing-rules/views/routingRuleSheet.tsx
- ui/lib/types/routingRules.ts
- ui/lib/config/celFieldsRouting.ts
- ui/app/workspace/routing-rules/views/routingRulesView.tsx
- transports/bifrost-http/lib/config.go
- framework/configstore/migrations.go
- docs/providers/routing-rules.mdx
- plugins/governance/routing.go
Pratham-Mishra04
force-pushed
the
03-26-feat_adds_chaining_to_routing_rules
branch
from
8a27fe9 to
6b68106
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
ui/app/workspace/routing-rules/views/routingRuleInfoSheet.tsx (2)
170-170: Remove unusedindexparameter.The
indexparameter is passed (line 330) but never used insideTargetCard. Remove it to avoid confusion.♻️ Proposed fix
-function TargetCard({ target, index, total }: { target: RoutingRule["targets"][0]; index: number; total: number }) { +function TargetCard({ target, total }: { target: RoutingRule["targets"][0]; total: number }) {And at line 330:
-<TargetCard key={i} target={target} index={i} total={targets.length} /> +<TargetCard key={i} target={target} total={targets.length} />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/workspace/routing-rules/views/routingRuleInfoSheet.tsx` at line 170, TargetCard's props include an unused index parameter; remove index from the TargetCard function signature and its props type (keep only target and total), and remove the corresponding argument where TargetCard is invoked (the place that currently passes index, e.g., the map/JSX at the call site that passes index as the second argument). Update any related type annotations or destructuring in TargetCard to only reference target and total.
101-106: Consider extracting the key-value parsing logic for clarity.The nested ternary on lines 102-103 correctly parses
key:valuestrings, but the logic is dense. Extracting it to a small helper would improve readability.♻️ Optional refactor
+function parseKeyValue(value: string): { key: string; val: string } | null { + if (!value) return null; + const colonIdx = value.indexOf(":"); + if (colonIdx >= 0) { + return { key: value.slice(0, colonIdx), val: value.slice(colonIdx + 1) }; + } + return { key: value, val: "" }; +} + function ConditionRow({ rule }: { rule: RuleType }) { // ... - const bareKeyValue = !keyMatch && (isHeader || isParam) && value - ? value.includes(":") ? { key: value.slice(0, value.indexOf(":")), val: value.slice(value.indexOf(":") + 1) } : { key: value, val: "" } - : null; + const bareKeyValue = !keyMatch && (isHeader || isParam) ? parseKeyValue(value) : null;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/workspace/routing-rules/views/routingRuleInfoSheet.tsx` around lines 101 - 106, Extract the dense key:value parsing into a small helper to improve readability: create a function (e.g., parseBareKeyValue or parseHeaderParamKeyValue) that takes the raw value string and returns { key, val } or null; replace the nested ternary that computes bareKeyValue with a call to this helper and keep the existing conditional check using isHeader/isParam and keyMatch (i.e., compute bareKeyValue = !keyMatch && (isHeader || isParam) ? parseBareKeyValue(value) : null), and then leave keyName and displayValue assignments unchanged to use the helper's result.
🤖 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/routing-rules/views/routingRuleInfoSheet.tsx`:
- Line 170: TargetCard's props include an unused index parameter; remove index
from the TargetCard function signature and its props type (keep only target and
total), and remove the corresponding argument where TargetCard is invoked (the
place that currently passes index, e.g., the map/JSX at the call site that
passes index as the second argument). Update any related type annotations or
destructuring in TargetCard to only reference target and total.
- Around line 101-106: Extract the dense key:value parsing into a small helper
to improve readability: create a function (e.g., parseBareKeyValue or
parseHeaderParamKeyValue) that takes the raw value string and returns { key, val
} or null; replace the nested ternary that computes bareKeyValue with a call to
this helper and keep the existing conditional check using isHeader/isParam and
keyMatch (i.e., compute bareKeyValue = !keyMatch && (isHeader || isParam) ?
parseBareKeyValue(value) : null), and then leave keyName and displayValue
assignments unchanged to use the helper's result.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9f1dc4af-14ee-495d-99e2-13d324fdd44a
📒 Files selected for processing (25)
docs/providers/routing-rules.mdxframework/configstore/clientconfig.goframework/configstore/migrations.goframework/configstore/rdb.goframework/configstore/tables/clientconfig.goframework/configstore/tables/routing_rules.goplugins/governance/main.goplugins/governance/routing.goplugins/governance/routing_test.gotransports/bifrost-http/handlers/config.gotransports/bifrost-http/handlers/governance.gotransports/bifrost-http/lib/config.gotransports/bifrost-http/server/plugins.goui/app/workspace/config/pricing-config/page.tsxui/app/workspace/config/views/modelSettingsView.tsxui/app/workspace/custom-pricing/page.tsxui/app/workspace/routing-rules/views/routingRuleInfoSheet.tsxui/app/workspace/routing-rules/views/routingRuleSheet.tsxui/app/workspace/routing-rules/views/routingRulesTable.tsxui/app/workspace/routing-rules/views/routingRulesView.tsxui/components/sidebar.tsxui/lib/config/celFieldsRouting.tsui/lib/types/config.tsui/lib/types/routingRules.tsui/lib/types/schemas.ts
✅ Files skipped from review due to trivial changes (5)
- ui/app/workspace/config/pricing-config/page.tsx
- ui/app/workspace/custom-pricing/page.tsx
- framework/configstore/rdb.go
- ui/app/workspace/routing-rules/views/routingRulesTable.tsx
- docs/providers/routing-rules.mdx
🚧 Files skipped from review as they are similar to previous changes (15)
- ui/lib/types/schemas.ts
- transports/bifrost-http/server/plugins.go
- framework/configstore/tables/routing_rules.go
- ui/app/workspace/routing-rules/views/routingRulesView.tsx
- ui/app/workspace/routing-rules/views/routingRuleSheet.tsx
- ui/components/sidebar.tsx
- ui/lib/types/routingRules.ts
- transports/bifrost-http/lib/config.go
- plugins/governance/main.go
- transports/bifrost-http/handlers/config.go
- ui/app/workspace/config/views/modelSettingsView.tsx
- transports/bifrost-http/handlers/governance.go
- framework/configstore/migrations.go
- plugins/governance/routing.go
- framework/configstore/clientconfig.go
Pratham-Mishra04
force-pushed
the
03-26-feat_adds_chaining_to_routing_rules
branch
from
6b68106 to
cb60334
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
ui/app/workspace/routing-rules/views/routingRulesTable.tsx (1)
141-190:⚠️ Potential issue | 🟠 MajorMake row-open interaction keyboard-accessible and add a dedicated row-open test id.
Line 142 attaches open behavior to
<TableRow>(<tr>), which is click-only for pointer users and not keyboard-focusable by default. This blocks keyboard users from opening rule details. Also, this new row-open interactive action does not have its owndata-testid.Suggested fix
- <TableRow key={rule.id} className="hover:bg-muted/50 cursor-pointer transition-colors" onClick={() => onRowClick(rule)}> + <TableRow key={rule.id} className="hover:bg-muted/50 transition-colors"> <TableCell className="font-medium"> - <div className="flex flex-col gap-1"> + <button + type="button" + className="flex w-full flex-col gap-1 text-left" + onClick={() => onRowClick(rule)} + aria-label={`Open routing rule ${rule.name}`} + data-testid={`routing-rule-open-${rule.id}`} + > <span className="truncate max-w-xs">{rule.name}</span> {rule.description && ( <span data-testid="routing-rule-description" className="text-xs text-muted-foreground truncate max-w-xs">{rule.description}</span> )} - </div> + </button> </TableCell>As per coding guidelines,
ui/**/*.{tsx,ts}should “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/app/workspace/routing-rules/views/routingRulesTable.tsx` around lines 141 - 190, The TableRow currently only supports pointer clicks via onRowClick(rule), which is not keyboard-accessible and lacks a dedicated test id; make the row focusable and activatable via keyboard by adding tabIndex={0}, role="button", and an onKeyDown handler that triggers onRowClick(rule) when Enter or Space is pressed, keep the existing onClick, and add a data-testid following the pattern (e.g., data-testid={`routing-rule-row-open-${rule.id}`}) to the TableRow so tests can target the row-open action; reference the TableRow element and the onRowClick handler when making these changes.
🧹 Nitpick comments (3)
ui/lib/utils/celConverterRouting.ts (2)
171-177: Same edge case applies here for keyValue fields.The same defensive guard suggested above for the
nulloperator should be applied to thenotNullcase for consistency.🛡️ Optional: Add defensive guard
if (operator === 'notNull') { const keyValuePair = parseKeyValue(String(value)); if (keyValuePair && keyValuePair.key) { return `${formatValue(keyValuePair.key, 'text')} in ${field}`; } + if (field === 'headers' || field === 'params') { + return ''; + } return `${field} != ""`; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/lib/utils/celConverterRouting.ts` around lines 171 - 177, The notNull branch currently assumes parseKeyValue(String(value)) returns an object with key; add the same defensive guard used for the null operator: check that keyValuePair exists and that keyValuePair.key is non-empty (or fallback to using the raw field) before returning `${formatValue(keyValuePair.key, 'text')} in ${field}`; if the guard fails, return the safe fallback `${field} != ""`. Update the operator === 'notNull' block that calls parseKeyValue, formatValue and references field to include this defensive check.
161-169: Edge case: empty value for keyValue fields produces semantically incorrect CEL.For string fields like
provider, the change to${field} == ""is correct sincehas()cannot be used with bare variable names in CEL.However, if a keyValue field (e.g.,
headers,params) reaches this fallback with an emptyvalue,parseKeyValue("")returnsnull, causing the code to generateheaders == "". This comparison of a map to an empty string is semantically incorrect.While UI validation via
inputType: "keyValue"should prevent empty keys in practice, consider adding a defensive check:🛡️ Optional: Add defensive guard for keyValue fields
if (operator === 'null') { const keyValuePair = parseKeyValue(String(value)); if (keyValuePair && keyValuePair.key) { return `!(${formatValue(keyValuePair.key, 'text')} in ${field})`; } + // Prevent invalid CEL for keyValue fields without a key + if (field === 'headers' || field === 'params') { + return ''; // or throw an error + } // has() requires a field selection (e.g. has(obj.field)) and cannot be used with bare // variable names. Plain string variables are always defined; "not set" means empty string. return `${field} == ""`; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/lib/utils/celConverterRouting.ts` around lines 161 - 169, In the operator === 'null' branch, when parseKeyValue(String(value)) returns null we currently fall back to `${field} == ""`, which is wrong for map-like keyValue fields (e.g., headers, params); add a defensive check: define a small set/array of known keyValue field names (e.g., KEY_VALUE_FIELDS) and if parseKeyValue returns null and field is in that set, return a CEL expression checking emptiness of the map (e.g., `${field}.size() == 0`) instead of comparing to an empty string; otherwise keep the existing `${field} == ""` behavior—update the logic around parseKeyValue, operator, formatValue and field references accordingly.framework/configstore/migrations.go (1)
5409-5469: Centralize the routing-chain default used in this backfill.This migration adds another literal
10, while the runtime stack already has its own default. Reusing a single configstore constant here and inClientConfigwould make future default changes much less likely to desync hash generation from runtime behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/configstore/migrations.go` around lines 5409 - 5469, Replace the hardcoded literal "10" in migrationAddRoutingChainMaxDepthColumn with the centralized default constant (e.g. DefaultRoutingChainMaxDepth) used by ClientConfig/runtime: update the depth assignment (where it currently sets depth = 10) to use that constant, import or reference the package/constant where DefaultRoutingChainMaxDepth is defined, and ensure ClientConfig.RoutingChainMaxDepth usage in the hash recompute remains consistent with that constant so future default changes stay in sync.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@ui/app/workspace/routing-rules/views/routingRulesTable.tsx`:
- Around line 141-190: The TableRow currently only supports pointer clicks via
onRowClick(rule), which is not keyboard-accessible and lacks a dedicated test
id; make the row focusable and activatable via keyboard by adding tabIndex={0},
role="button", and an onKeyDown handler that triggers onRowClick(rule) when
Enter or Space is pressed, keep the existing onClick, and add a data-testid
following the pattern (e.g., data-testid={`routing-rule-row-open-${rule.id}`})
to the TableRow so tests can target the row-open action; reference the TableRow
element and the onRowClick handler when making these changes.
---
Nitpick comments:
In `@framework/configstore/migrations.go`:
- Around line 5409-5469: Replace the hardcoded literal "10" in
migrationAddRoutingChainMaxDepthColumn with the centralized default constant
(e.g. DefaultRoutingChainMaxDepth) used by ClientConfig/runtime: update the
depth assignment (where it currently sets depth = 10) to use that constant,
import or reference the package/constant where DefaultRoutingChainMaxDepth is
defined, and ensure ClientConfig.RoutingChainMaxDepth usage in the hash
recompute remains consistent with that constant so future default changes stay
in sync.
In `@ui/lib/utils/celConverterRouting.ts`:
- Around line 171-177: The notNull branch currently assumes
parseKeyValue(String(value)) returns an object with key; add the same defensive
guard used for the null operator: check that keyValuePair exists and that
keyValuePair.key is non-empty (or fallback to using the raw field) before
returning `${formatValue(keyValuePair.key, 'text')} in ${field}`; if the guard
fails, return the safe fallback `${field} != ""`. Update the operator ===
'notNull' block that calls parseKeyValue, formatValue and references field to
include this defensive check.
- Around line 161-169: In the operator === 'null' branch, when
parseKeyValue(String(value)) returns null we currently fall back to `${field} ==
""`, which is wrong for map-like keyValue fields (e.g., headers, params); add a
defensive check: define a small set/array of known keyValue field names (e.g.,
KEY_VALUE_FIELDS) and if parseKeyValue returns null and field is in that set,
return a CEL expression checking emptiness of the map (e.g., `${field}.size() ==
0`) instead of comparing to an empty string; otherwise keep the existing
`${field} == ""` behavior—update the logic around parseKeyValue, operator,
formatValue and field references accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8881daa0-c2da-46c2-bc2f-4d6b0726c850
📒 Files selected for processing (26)
docs/providers/routing-rules.mdxframework/configstore/clientconfig.goframework/configstore/migrations.goframework/configstore/rdb.goframework/configstore/tables/clientconfig.goframework/configstore/tables/routing_rules.goplugins/governance/main.goplugins/governance/routing.goplugins/governance/routing_test.gotransports/bifrost-http/handlers/config.gotransports/bifrost-http/handlers/governance.gotransports/bifrost-http/lib/config.gotransports/bifrost-http/server/plugins.goui/app/workspace/config/pricing-config/page.tsxui/app/workspace/config/views/modelSettingsView.tsxui/app/workspace/custom-pricing/page.tsxui/app/workspace/routing-rules/views/routingRuleInfoSheet.tsxui/app/workspace/routing-rules/views/routingRuleSheet.tsxui/app/workspace/routing-rules/views/routingRulesTable.tsxui/app/workspace/routing-rules/views/routingRulesView.tsxui/components/sidebar.tsxui/lib/config/celFieldsRouting.tsui/lib/types/config.tsui/lib/types/routingRules.tsui/lib/types/schemas.tsui/lib/utils/celConverterRouting.ts
✅ Files skipped from review due to trivial changes (6)
- ui/app/workspace/config/pricing-config/page.tsx
- ui/app/workspace/custom-pricing/page.tsx
- ui/app/workspace/routing-rules/views/routingRuleSheet.tsx
- framework/configstore/tables/routing_rules.go
- framework/configstore/rdb.go
- ui/lib/types/routingRules.ts
🚧 Files skipped from review as they are similar to previous changes (16)
- transports/bifrost-http/server/plugins.go
- ui/lib/types/schemas.ts
- framework/configstore/tables/clientconfig.go
- ui/lib/types/config.ts
- transports/bifrost-http/handlers/config.go
- ui/app/workspace/routing-rules/views/routingRulesView.tsx
- ui/components/sidebar.tsx
- plugins/governance/main.go
- transports/bifrost-http/lib/config.go
- transports/bifrost-http/handlers/governance.go
- docs/providers/routing-rules.mdx
- ui/app/workspace/config/views/modelSettingsView.tsx
- plugins/governance/routing.go
- ui/app/workspace/routing-rules/views/routingRuleInfoSheet.tsx
- plugins/governance/routing_test.go
- ui/lib/config/celFieldsRouting.ts
akshaydeo
approved these changes
Apr 2, 2026
Contributor
Merge activity
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds rule chaining functionality to the routing engine, allowing routing rules to be composed together. When a rule has
chain_rule: true, the routing engine re-evaluates the full rule set using the resolved provider/model as the new context, enabling use cases like model alias normalization and tiered policy application.Changes
chain_ruleboolean field to routing rules database schema with migrationType of change
Affected areas
How to test
Create routing rules with chaining enabled to test the composition behavior:
Test chaining by creating two rules:
gpt-4togpt-4-turbogpt-4-turboto AzureSend a request with
model=gpt-4and verify it gets routed to Azure withgpt-4-turbo.Screenshots/Recordings
N/A - Backend feature with UI configuration support
Breaking changes
This is additive functionality with backward compatibility. Existing rules default to
chain_rule: false.Related issues
N/A
Security considerations
No security implications. The chaining feature operates within the existing routing rule evaluation framework and doesn't introduce new attack vectors.
Checklist
docs/contributing/README.mdand followed the guidelines