feat: per-project enterprise routing overrides - #2363
Merged
Merged
Conversation
Allow enterprise orgs to override hardcoded routing knobs per project: scoring weights, thresholds, retry/fallback, timeouts, and per-provider priorities. Mirrors guardrails pattern (DB row + enterprise-gated API CRUD + UI tab + SWR-cached gateway read). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR introduces a comprehensive enterprise routing configuration system that allows per-project customization of routing weights, thresholds, retry behavior, timeouts, metrics history windows, and provider priorities. Changes span the shared configuration model, database schema, API endpoints, gateway integration, provider-selection scoring, UI editor, and metrics history computation. ChangesEnterprise routing configuration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
Adds per-project (enterprise-only) routing configuration overrides, exposing adjustable routing weights/thresholds/retry/timeouts/provider priorities via a new DB table + API endpoints, and wiring the resolved config into gateway provider selection and timeout/retry behavior.
Changes:
- Introduces
routing_configDB table + relations + migration for storing per-project routing overrides. - Adds shared
routing-configresolver/defaults and uses it in the routing algorithm (getCheapestFromAvailableProviders) plus gateway loaders. - Adds API endpoints + UI “Routing” settings page to view/update/reset routing config (enterprise owners/admins).
Reviewed changes
Copilot reviewed 21 out of 27 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| pnpm-lock.yaml | Updates lockfile for new workspace dependency wiring. |
| packages/shared/src/routing-config.ts | Adds routing config types, defaults, and resolve/merge helpers. |
| packages/shared/src/routing-config.spec.ts | Unit tests for routing config resolution/merging behavior. |
| packages/shared/src/index.ts | Exposes routing-config exports from shared package entrypoint. |
| packages/shared/package.json | Adds ./routing-config export path. |
| packages/db/src/schema.ts | Adds routing_config table schema + JSONB columns/types. |
| packages/db/src/relations.ts | Adds project ↔ routingConfig relations. |
| packages/db/migrations/meta/_journal.json | Registers new migration in Drizzle journal. |
| packages/db/migrations/1779389372_black_ghost_rider.sql | Creates routing_config table + index + FK. |
| packages/actions/src/models.spec.ts | Adds tests ensuring providerPriorities=0 excludes providers, and thresholds overrides don’t break selection. |
| packages/actions/src/get-cheapest-from-available-providers.ts | Integrates resolved routing config into scoring, exploration rate, cache threshold, defaults, and provider priority disabling. |
| packages/actions/package.json | Adds @llmgateway/shared dependency. |
| ee/admin/src/lib/api/v1.d.ts | Regenerates OpenAPI client types to include routing-config endpoints. |
| apps/ui/src/lib/api/v1.d.ts | Regenerates OpenAPI client types to include routing-config endpoints. |
| apps/playground/src/lib/api/v1.d.ts | Regenerates OpenAPI client types to include routing-config endpoints. |
| apps/code/src/lib/api/v1.d.ts | Regenerates OpenAPI client types to include routing-config endpoints. |
| apps/ui/src/components/dashboard/dashboard-sidebar.tsx | Adds “Routing” item to project settings sidebar. |
| apps/ui/src/app/dashboard/[orgId]/[projectId]/settings/routing/page.tsx | Adds routing settings page entrypoint. |
| apps/ui/src/app/dashboard/[orgId]/[projectId]/settings/routing/_components/routing-config-client.tsx | Implements routing config editor UI and save/reset flows. |
| apps/gateway/src/videos/videos.ts | Loads resolved routing config per request and applies retry + provider selection overrides. |
| apps/gateway/src/lib/timeout-config.ts | Applies routing-config timeouts with override → env → default precedence. |
| apps/gateway/src/lib/routing-config-loader.ts | Adds SWR-cached loader to fetch/resolve project routing config from DB. |
| apps/gateway/src/chat/tools/retry-with-fallback.ts | Makes max retries configurable (falls back to default routing retry maxRetries). |
| apps/gateway/src/chat/chat.ts | Loads routing config and threads it through routing decisions, timeouts, and retry limits. |
| apps/api/src/routes/routing-config.ts | Adds CRUD + resolved/defaults routing-config API routes with enterprise + owner/admin gating (partial). |
| apps/api/src/routes/index.ts | Registers the routing-config routes. |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+33
to
+37
| function getExplorationRate(cfg: ResolvedRoutingConfig): number { | ||
| const rawExplorationRate = process.env.EXPLORATION_RATE; | ||
|
|
||
| if (rawExplorationRate === undefined || rawExplorationRate.trim() === "") { | ||
| return DEFAULT_EXPLORATION_RATE; | ||
| return cfg.thresholds.explorationRate; |
Comment on lines
508
to
512
| const weightSum = new Decimal(effectivePriceWeight) | ||
| .plus(UPTIME_WEIGHT) | ||
| .plus(THROUGHPUT_WEIGHT) | ||
| .plus(weights.uptime) | ||
| .plus(weights.throughput) | ||
| .plus(effectiveLatencyWeight) | ||
| .plus(effectiveCacheWeight); |
Comment on lines
+27
to
+29
| return ( | ||
| Number(process.env.GATEWAY_TIMEOUT_MS) || DEFAULT_ROUTING_TIMEOUTS.gatewayMs | ||
| ); |
Comment on lines
+74
to
+78
| .object({ | ||
| price: z.number().min(0).optional(), | ||
| imagePrice: z.number().min(0).optional(), | ||
| uptime: z.number().min(0).optional(), | ||
| throughput: z.number().min(0).optional(), |
| }, | ||
| }); | ||
|
|
||
| routingConfig.openapi(getDefaults, async (c) => { |
Comment on lines
+201
to
+205
| const role = teamData?.members.find((m) => m.userId === user?.id)?.role; | ||
| const canManage = | ||
| selectedOrganization?.plan === "enterprise" && | ||
| (role === "owner" || role === "admin"); | ||
|
|
Comment on lines
+197
to
+200
| const { selectedOrganization } = useDashboardNavigation(); | ||
| const { user } = useUser(); | ||
| const { data: teamData } = useTeamMembers(selectedOrganization?.id ?? ""); | ||
|
|
| return Object.keys(defaults.providerPriorities).sort(); | ||
| }, [defaults]); | ||
|
|
||
| if (!canManage) { |
![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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
apps/gateway/src/chat/chat.ts (1)
2122-2129:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReject explicitly requested providers that are disabled for the project.
This branch only uses
routingCfgto pick a region insideusedProvider. IfproviderPriorities[usedProvider]is0, the request still succeeds because we never filtersameProviderMappings, andbestRegionResult?.provider ?? eligibleMappings[0]falls back to the disabled mapping anyway. That lets direct provider requests bypass the per-project disable knob.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/gateway/src/chat/chat.ts` around lines 2122 - 2129, The code currently can select a provider even when project-specific providerPriorities[usedProvider] === 0 because eligibleMappings/sameProviderMappings aren't filtered; update the selection to explicitly reject providers disabled for the project by filtering out mappings whose mapping.provider has priority 0 before computing sameProviderMappings/eligibleMappings (or, when computing selectedMapping from bestRegionResult/eligibleMappings, ensure bestRegionResult?.provider and eligibleMappings[0] both have non-zero providerPriorities entries), and if no mapping remains for the requested usedProvider return an explicit error (or fall back to another allowed provider) so direct provider requests cannot bypass the per-project disable knob. Include references to routingCfg, usedProvider, providerPriorities, sameProviderMappings, eligibleMappings, bestRegionResult, and selectedMapping when making the change.packages/actions/src/get-cheapest-from-available-providers.ts (2)
506-524:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard against zero total weight before score normalization.
If routing overrides set all effective weights to
0,weightSumbecomes0and the subsequent divisions on Lines 514/516/518/521/523 can produce invalid scores.Suggested fix
const weightSum = new Decimal(effectivePriceWeight) .plus(weights.uptime) .plus(weights.throughput) .plus(effectiveLatencyWeight) .plus(effectiveCacheWeight); + if (weightSum.lte(0)) { + // Safe fallback: prioritize by effective price when weights are all zero/invalid + const priority = getEffectivePriority(providerScore.provider.providerId, cfg); + const priorityPenalty = new Decimal(1).minus(priority); + providerScore.score = priceScore.plus(priorityPenalty); + continue; + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/actions/src/get-cheapest-from-available-providers.ts` around lines 506 - 524, Guard against weightSum being zero before normalization: after computing weightSum (which uses effectivePriceWeight, weights.uptime, weights.throughput, effectiveLatencyWeight, effectiveCacheWeight) check if weightSum.isZero() and if so set weightSum to new Decimal(1) (or another safe nonzero fallback) before computing baseScore so the subsequent .div(weightSum) calls (used in baseScore calculation) never divide by zero; update the block that computes weightSum and baseScore (referencing weightSum and baseScore) to perform this guard.
33-52:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPer-project exploration override is being bypassed by env precedence.
On Line 34-51,
EXPLORATION_RATEis always applied beforecfg.thresholds.explorationRate. That makes a global env var override explicit project config, which conflicts with the documented precedence (override → env → default).Suggested fix
-function getExplorationRate(cfg: ResolvedRoutingConfig): number { +function getExplorationRate( + cfg: ResolvedRoutingConfig, + hasProjectRoutingConfig: boolean, +): number { + if (hasProjectRoutingConfig) { + return cfg.thresholds.explorationRate; + } const rawExplorationRate = process.env.EXPLORATION_RATE;- if (!isTestProcess() && Math.random() < getExplorationRate(cfg)) { + if ( + !isTestProcess() && + Math.random() < + getExplorationRate(cfg, options?.routingConfig !== undefined) + ) {Also applies to: 351-351
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/actions/src/get-cheapest-from-available-providers.ts` around lines 33 - 52, The getExplorationRate function currently applies EXPLORATION_RATE env var before the project config; change the precedence so cfg.thresholds.explorationRate (per-project override) is checked first, then process.env.EXPLORATION_RATE, then the default; validate whichever source is used (ensure Number.isFinite and between 0 and 1) and throw the same error format for invalid values, and apply the same fix to the duplicate logic noted around line ~351.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/api/src/routes/routing-config.ts`:
- Around line 220-267: The current read-then-write flow using
db.query.routingConfig.findFirst followed by db.update(...) or db.insert(...)
can race on concurrent PUTs and cause unique project_id insert failures; replace
it with a single atomic upsert (or a transaction with retry) keyed on projectId.
Concretely, remove the separate findFirst + conditional insert logic and use
your ORM's upsert mechanism (e.g.,
db.insert(tables.routingConfig).values({...}).onConflictDoUpdate({ target:
tables.routingConfig.projectId, set: { enabled: body.enabled ??
existingEquivalent, weights: body.weights ?? null, thresholds: body.thresholds
?? null, retry: body.retry ?? null, timeouts: body.timeouts ?? null,
providerPriorities: body.providerPriorities ?? null } }).returning()) so the
update-or-insert happens in one atomic statement (or wrap the read/update/insert
in a transaction with retry if upsert isn't available); keep the same field
mappings used in the current db.update and db.insert blocks.
In `@apps/gateway/src/chat/chat.ts`:
- Line 4864: The retry loops currently cap iterations with the constant
MAX_RETRIES which ignores the resolved per-project override passed as
routingCfg.retry.maxRetries; change the loop bounds to use the resolved cap
(e.g., use resolvedRetryCap or routingCfg.retry.maxRetries) instead of
MAX_RETRIES so the condition becomes retryAttempt <= routingCfg.retry.maxRetries
(or the local resolved variable) wherever the loop uses retryAttempt and
MAX_RETRIES (including the loops around retryAttempt checks at the locations
that call maxRetries: routingCfg.retry.maxRetries); ensure any local variable
used to store the resolved cap replaces references to MAX_RETRIES consistently.
In `@apps/gateway/src/lib/timeout-config.ts`:
- Around line 27-29: The getter currently returns
Number(process.env.GATEWAY_TIMEOUT_MS) || DEFAULT_ROUTING_TIMEOUTS.gatewayMs
which treats negative numbers as truthy; update the logic in timeout-config.ts
to parse GATEWAY_TIMEOUT_MS into a Number, check that it is > 0, and only return
it when positive, otherwise return DEFAULT_ROUTING_TIMEOUTS.gatewayMs (use the
same validation pattern as the other timeout getters). Reference
process.env.GATEWAY_TIMEOUT_MS and DEFAULT_ROUTING_TIMEOUTS.gatewayMs when
making the change.
In `@packages/actions/src/models.spec.ts`:
- Around line 1087-1088: Replace the dynamic imports of
"`@llmgateway/shared/routing-config`" with top-level static imports: add a
file-scope import for resolveRoutingConfig and buildProviderPriorityDefaults and
remove the await import(...) occurrences; update any test references using
resolveRoutingConfig or buildProviderPriorityDefaults (the other dynamic import
occurrence that mirrors this one) to use the top-level imported symbols instead
so TypeScript rules are satisfied.
In `@packages/shared/src/routing-config.ts`:
- Around line 157-165: getDefaultRoutingConfig currently returns the shared
mutable cachedDefaults object; change it to return a defensive copy so callers
can't mutate shared state. After computing or reading cachedDefaults in
getDefaultRoutingConfig, return a deep/shallow clone as appropriate (e.g.,
structuredClone or a deep-clone util) rather than the cached reference; keep
caching logic using cachedDefaults but always return the cloned object. Update
references to getDefaultRoutingConfig, cachedDefaults, resolveRoutingConfig, and
buildProviderPriorityDefaults accordingly so callers get an immutable copy.
---
Outside diff comments:
In `@apps/gateway/src/chat/chat.ts`:
- Around line 2122-2129: The code currently can select a provider even when
project-specific providerPriorities[usedProvider] === 0 because
eligibleMappings/sameProviderMappings aren't filtered; update the selection to
explicitly reject providers disabled for the project by filtering out mappings
whose mapping.provider has priority 0 before computing
sameProviderMappings/eligibleMappings (or, when computing selectedMapping from
bestRegionResult/eligibleMappings, ensure bestRegionResult?.provider and
eligibleMappings[0] both have non-zero providerPriorities entries), and if no
mapping remains for the requested usedProvider return an explicit error (or fall
back to another allowed provider) so direct provider requests cannot bypass the
per-project disable knob. Include references to routingCfg, usedProvider,
providerPriorities, sameProviderMappings, eligibleMappings, bestRegionResult,
and selectedMapping when making the change.
In `@packages/actions/src/get-cheapest-from-available-providers.ts`:
- Around line 506-524: Guard against weightSum being zero before normalization:
after computing weightSum (which uses effectivePriceWeight, weights.uptime,
weights.throughput, effectiveLatencyWeight, effectiveCacheWeight) check if
weightSum.isZero() and if so set weightSum to new Decimal(1) (or another safe
nonzero fallback) before computing baseScore so the subsequent .div(weightSum)
calls (used in baseScore calculation) never divide by zero; update the block
that computes weightSum and baseScore (referencing weightSum and baseScore) to
perform this guard.
- Around line 33-52: The getExplorationRate function currently applies
EXPLORATION_RATE env var before the project config; change the precedence so
cfg.thresholds.explorationRate (per-project override) is checked first, then
process.env.EXPLORATION_RATE, then the default; validate whichever source is
used (ensure Number.isFinite and between 0 and 1) and throw the same error
format for invalid values, and apply the same fix to the duplicate logic noted
around line ~351.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 34aef7e3-9455-42f4-a69e-a89523c648d9
⛔ Files ignored due to path filters (5)
apps/code/src/lib/api/v1.d.tsis excluded by!**/v1.d.tsapps/playground/src/lib/api/v1.d.tsis excluded by!**/v1.d.tsapps/ui/src/lib/api/v1.d.tsis excluded by!**/v1.d.tsee/admin/src/lib/api/v1.d.tsis excluded by!**/v1.d.tspnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (22)
apps/api/src/routes/index.tsapps/api/src/routes/routing-config.tsapps/gateway/src/chat/chat.tsapps/gateway/src/chat/tools/retry-with-fallback.tsapps/gateway/src/lib/routing-config-loader.tsapps/gateway/src/lib/timeout-config.tsapps/gateway/src/videos/videos.tsapps/ui/src/app/dashboard/[orgId]/[projectId]/settings/routing/_components/routing-config-client.tsxapps/ui/src/app/dashboard/[orgId]/[projectId]/settings/routing/page.tsxapps/ui/src/components/dashboard/dashboard-sidebar.tsxpackages/actions/package.jsonpackages/actions/src/get-cheapest-from-available-providers.tspackages/actions/src/models.spec.tspackages/db/migrations/1779389372_black_ghost_rider.sqlpackages/db/migrations/meta/1779389372_snapshot.jsonpackages/db/migrations/meta/_journal.jsonpackages/db/src/relations.tspackages/db/src/schema.tspackages/shared/package.jsonpackages/shared/src/index.tspackages/shared/src/routing-config.spec.tspackages/shared/src/routing-config.ts
- New RoutingContactSalesCard so non-enterprise orgs see routing copy instead of the guardrails copy that was being reused by mistake. - Default the per-project routing override row to enabled=false (DB default + API insert default + UI initial state) so that creating the row does not silently change routing behavior until the toggle is explicitly flipped on. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Collaborator
|
Merge conflict resolution verified. Conflicts were in |
The gateway reads routing config through swrWrap with a 4-hour stale TTL, which keeps routing working when the DB is down. Writes via the API go through the uncached `db` client (matching the guardrails pattern), so they did not bust the SWR mirror — the gateway would have served the prior value for up to 4 hours after a save. Explicitly invalidate by table name after each update / insert / reset. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
The provider-metrics aggregation in apps/worker (60-minute window with 10x/3x/1x tier weights) was hardcoded. Make these configurable per project as part of the routing config. When a project's history config matches the defaults, the gateway keeps using the cheap pre-aggregated columns the worker writes onto modelProviderMapping. When it differs, the gateway runs a per-request aggregation against modelProviderMappingHistory using the project's tier weights, cached in SWR by (history-config-hash, model-set) with a 30s TTL so concurrent requests with the same shape share one DB hit and the gateway stays warm if Postgres falls over. Bounds: windowMinutes <= 120, weights and tier minutes >= 0. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
… from history
The gateway now reads routing metrics directly from
model_provider_mapping_history on every request (cached by
(history-config-hash, model-set) for 30s and SWR-mirrored to survive
DB outages), so the precomputed columns on model_provider_mapping
that the worker was writing are dead weight.
Drop them:
- routingUptime / routingLatency / routingThroughput / routingTotalRequests
The worker stats job still writes the unweighted logsCount /
errorsCount / cachedCount / clientErrorsCount / gatewayErrorsCount /
upstreamErrorsCount / avgTimeToFirstToken{,ReasoningToken} columns
because those feed admin/UI displays, but its weighted aggregation
block is gone — that work is now done on-demand with per-project
tier weights.
Test harnesses (setRoutingMetrics) seed a single recent history row
that, when aggregated, reproduces the requested
uptime/latency/throughput. provider-metrics.spec was deleted (it
tested the now-removed pre-computed getters) and the worker spec's
routing-metric assertions were dropped.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
The API zod schema allows individual scoring weights to be set to 0 (z.number().min(0)), but the weighted-score path in getCheapestFromAvailableProviders then computes weightSum and divides by it. A project that zeroed out every weight would crash with DecimalError: Division by zero on the next request. Short-circuit before the loop: when the effective weight total is zero (which also covers the not-streaming case where latencyWeight is dropped, and the short-prompt case where cacheWeight is dropped), fall back to the existing price-only selection path. Per-provider priority overrides and priority=0 exclusion still apply via selectByPriceOnly. Adds a regression test in models.spec.ts. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Move GET /routing-config/defaults to GET /routing-config/config/{projectId}/defaults
so the same checkProjectEnterpriseAccess gate that protects the other
routing-config endpoints applies here too — enterprise plan + owner/admin
role on the project's organization. The endpoint was previously only
behind the global session-auth middleware, so any authenticated user
could fetch it; consistency with the rest of the feature is worth the
URL change.
UI fetches updated; openapi + fetch-client types regenerated.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Replace the findFirst-then-update-or-insert pattern with a single onConflictDoUpdate keyed on the unique projectId column. Concurrent PUTs from the same admin (double-click Save, multiple tabs, retrying scripts) used to race between the read and the insert and trip the unique constraint on project_id with a 500. Preserve the previous semantics: - Fields omitted from the body keep their existing value on update — done by only adding those keys to the conflict-update set when the body explicitly provided them, so Postgres leaves them untouched. - Fields explicitly set to null in the body clear the column. - A no-op body (no fields at all) maps to onConflictDoNothing and we refetch the existing row so the response shape stays identical. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
The streaming retry loop at chat.ts:4814 and the non-streaming loop at chat.ts:8481 were capped at the constant MAX_RETRIES (2), so projects that lowered or raised retry.maxRetries via routing config saw shouldRetryRequest correctly skip later attempts but the outer loop still spun up to two before exiting. Use routingCfg.retry.maxRetries for both loop bounds. MAX_RETRIES import is dropped from chat.ts since nothing else references it. videos.ts has no MAX_RETRIES-bounded loop — it relies on shouldRetryRequest (already maxRetries-aware via the option added earlier), so no change there. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
getGatewayTimeoutMs used `Number(env) || default`, which falls back for NaN but happily returns a negative number (`-5000` is truthy), which would then feed `AbortSignal.timeout(-5000)`. Match the streaming/plain getters: parse to a number and only return it if > 0, otherwise fall back to DEFAULT_ROUTING_TIMEOUTS.gatewayMs. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
The three new tests in `routing config overrides` were using
dynamic `await import("@llmgateway/shared/routing-config")` (one of
them with a nested dynamic import of `@llmgateway/db` for metricsKey).
Move resolveRoutingConfig and buildProviderPriorityDefaults to
top-level imports and drop the dynamic await blocks. metricsKey is
already imported at the top.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
The cache memoized the resolved defaults but handed the live reference to every caller — a single misbehaving caller mutating cfg.providerPriorities or cfg.weights would have poisoned every subsequent routing decision in the process. structuredClone the cached value before returning. Only caller today (the scoring function) just reads, so this is hardening, not a fix for a live bug. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…to enterprise-routing-overrides
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 (4)
apps/ui/src/app/dashboard/[orgId]/[projectId]/settings/routing/_components/routing-config-client.tsx (2)
238-242:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAvoid transient upsell rendering before role data resolves.
Line 307 can show
RoutingContactSalesCardfor authorized enterprise admins while team membership is still loading (roleis initially undefined). Gate the upsell behind an explicit membership-loading check so authorized users only see loading until access is known.Suggested fix
- const role = teamData?.members.find((m) => m.userId === user?.id)?.role; + const isTeamLoading = Boolean(selectedOrganization?.id) && !teamData; + const role = teamData?.members.find((m) => m.userId === user?.id)?.role; const canManage = selectedOrganization?.plan === "enterprise" && (role === "owner" || role === "admin"); @@ - if (!canManage) { + if (isTeamLoading || isLoading) { + return ( + <div className="flex flex-col"> + <div className="flex-1 space-y-4 p-4 pt-6 md:p-8"> + <div className="max-w-3xl mx-auto">Loading…</div> + </div> + </div> + ); + } + + if (!canManage) { return <RoutingContactSalesCard />; }Also applies to: 307-309
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/ui/src/app/dashboard/`[orgId]/[projectId]/settings/routing/_components/routing-config-client.tsx around lines 238 - 242, The upsell can render transiently because role is undefined while membership data loads; introduce an explicit membership-loading guard (e.g., isMembershipLoading = teamData === undefined || teamData.members === undefined) and only evaluate canManage or render RoutingContactSalesCard when loading is complete. Update the existing canManage logic (which currently uses role and selectedOrganization) to include !isMembershipLoading (or move the check to the JSX where RoutingContactSalesCard is rendered) so authorized enterprise admins see a loading state until teamData.members and role are resolved.
250-299:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftReplace effect-based loading with TanStack Query.
Line 250 performs API data fetching in
useEffectwith manual loading/cancellation state. This violates the UI data-fetching contract and bypasses query-layer behavior (cache/retry/invalidation consistency).As per coding guidelines,
apps/{ui,playground,code}/**/*.{ts,tsx}: "Do not use useEffect for data fetching in the UI; use TanStack Query for all data fetching and state management".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/ui/src/app/dashboard/`[orgId]/[projectId]/settings/routing/_components/routing-config-client.tsx around lines 250 - 299, Replace the useEffect-based fetch with a TanStack Query: create a useQuery (or two queries) that uses fetchClient.GET to load "/routing-config/config/{projectId}/defaults" and "/routing-config/config/{projectId}" (use projectId in the query key), set the query's enabled flag to canManage, and move the side-effects currently in the effect into the query's onSuccess and onError handlers—call setDefaults with defaultsRes.data and setState with the mapped row (enabled, weights, thresholds, retry, timeouts, history, providerPriorities) on success, and setError on failure; remove the cancelled flag and manual setIsLoading and instead derive loading state from the query status/isFetching and wire invalidation/retries to the query layer so fetchClient.GET is only used inside the query function(s).apps/worker/src/services/stats-calculator.ts (1)
993-1007:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRollup leaves stale counters for entities with no rows in the current 60-minute window.
These loops only update IDs present in
mappingAggregates. If a provider/model/mapping had prior non-zero counters but no current-window history, its counters are never reset and remain stale, which breaks the “last hour” semantics.💡 Suggested direction
+// Before/while applying aggregate updates, ensure entities missing from the +// current rollup are explicitly zeroed out (providers, models, mappings). +// Then apply non-zero/non-empty aggregate values.A practical implementation is to iterate all target IDs and use
agg ?? zeroAggwhen writing updates, instead of updating only keys present in the aggregate map.Also applies to: 1011-1025, 1031-1057
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/worker/src/services/stats-calculator.ts` around lines 993 - 1007, The rollup code only updates entries present in providerMap (and similarly for mappingAggregates/modelMap blocks), leaving previously non-zero counters unchanged when there are no rows in the current window; change the update loops (the one iterating providerMap and the similar loops handling mappingAggregates/modelMap) to iterate the full set of target IDs (all providers/models/mappings you intend to update) and use a default zero aggregation (e.g., zeroAgg) when agg is undefined (agg ?? zeroAgg) so you write zeros for counters when there is no current-window data; update the calls to database.update(...) where you currently use agg.* to instead derive values from (agg ?? zeroAgg) and ensure statsUpdatedAt/updatedAt are still set.apps/gateway/src/chat/chat.ts (1)
1998-2037:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix: fail closed when routing config disables providers (priority=0)
getCheapestFromAvailableProvidersfilters out providers withproviderPriorities[providerId] <= 0(priority0), and returnsnullwhen no eligible providers remain. The gateway then re-selects the first candidate anyway, bypassing that disable contract:
- In auto-routing:
if (cheapestResult) ... else { usedProvider = selectedProviders[0] ... }(≈1998-2037).- In direct-provider region selection:
selectedMapping = bestRegionResult?.provider ?? eligibleMappings[0](≈2213-2231).- In generic routing fallback:
else { usedProvider = routingCandidates[0] ... }(≈2783-2853).Change these
nullbranches to fail closed (or prefilter candidates topriority > 0before scoring), so priority0providers can’t be re-enabled via[0]fallbacks.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/gateway/src/chat/chat.ts` around lines 1998 - 2037, The routing logic currently re-enables providers that get filtered out by getCheapestFromAvailableProviders (priority <= 0) by falling back to first-array entries; update the branches that handle a null/undefined cheapest/best result to fail closed: where you see the pattern "if (cheapestResult) ... else { usedProvider = selectedProviders[0] ... }" (and analogous fallbacks using eligibleMappings[0] or routingCandidates[0] and the expression selectedMapping = bestRegionResult?.provider ?? eligibleMappings[0]), either pre-filter the candidate arrays (selectedProviders, eligibleMappings, routingCandidates) to remove any providerId with providerPriorities[providerId] <= 0 before scoring, or change the else/null branch to return an error/abort path (or set usedProvider/usedModel to undefined and propagate a failure) so a null result from getCheapestFromAvailableProviders/getBestRegionResult cannot be overridden by array[0]; locate uses around getCheapestFromAvailableProviders, collapseProvidersToBestRegionPerProvider, bestRegionResult, and routingCandidates to apply this change.
🧹 Nitpick comments (3)
packages/db/src/schema.ts (1)
1771-1778: ⚡ Quick winDuplicate
RoutingHistoryConfiginterface.This interface is identical to the one exported from
packages/shared/src/routing-config.ts. Consider importing from the shared package to avoid drift if the interface changes.+import type { RoutingHistoryConfig } from "`@llmgateway/shared/routing-config`"; + -export interface RoutingHistoryConfig { - windowMinutes?: number; - tier1Minutes?: number; - tier2Minutes?: number; - tier1Weight?: number; - tier2Weight?: number; - tier3Weight?: number; -}Note: If intentional (to avoid circular dependencies between db and shared packages), consider adding a comment explaining the duplication.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/db/src/schema.ts` around lines 1771 - 1778, The RoutingHistoryConfig interface in packages/db/src/schema.ts is a duplicate of the one in packages/shared/src/routing-config.ts; remove the duplicated declaration and import the shared type instead (replace the local export of RoutingHistoryConfig with an import from the shared package and re-export if needed) so changes stay centralized; if this duplication is intentional to avoid a circular dependency, add a clear comment above the interface explaining why it must remain duplicated and reference the original location (packages/shared/src/routing-config.ts) to prevent future accidental merges.packages/db/src/provider-metrics-history.ts (1)
97-165: 💤 Low valueRevisit stacking
swrWrapwith Drizzle.$withCache
.$withCacheandswrWrapcache the same SELECT result in two independent layers (Drizzle’s query cache + SWR’s fetcher/result cache). This isn’t inherently incorrect, but it can increase staleness and complexity if their TTLs/invalidation/revalidation behaviors aren’t aligned—consider using one as the source of truth or intentionally keeping both with matching expectations.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/db/src/provider-metrics-history.ts` around lines 97 - 165, The query is being cached twice (swrWrap around the async fetcher and Drizzle’s .$withCache on the cdb.select), which can cause divergent staleness; choose one caching layer and remove the other. Either remove the `.$withCache({ config: { ex: HISTORY_SWR_TTL_SECONDS } })` on the `cdb.select(...).from(modelProviderMappingHistory)...` so SWR (`swrWrap`) is the single source of truth, or remove `swrWrap` and let Drizzle’s `.$withCache` control TTLs (ensuring `HISTORY_SWR_TTL_SECONDS` is used consistently if you keep `.$withCache`); adjust code around `swrWrap`, `.$withCache`, `HISTORY_SWR_TTL_SECONDS`, and the `cdb.select`/`modelProviderMappingHistory` block accordingly.apps/gateway/src/lib/provider-metrics-for-routing.ts (1)
10-17: ⚡ Quick winRemove the long implementation-detail comment block.
This block restates code behavior and is likely to drift; the function name and types are already self-explanatory.
♻️ Suggested cleanup
-/** - * Returns the metrics map for the candidate (model, provider, region) - * combinations using the project's history window + tier weights when - * configured; otherwise the built-in defaults. Either way this runs the - * same on-demand weighted aggregation against - * model_provider_mapping_history, cached in SWR by (history-config-hash, - * model-set) for resilience and to keep concurrent requests cheap. - */ export async function getProviderMetricsForRouting(As per coding guidelines:
**/*.{ts,tsx}:No unnecessary code comments.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/gateway/src/lib/provider-metrics-for-routing.ts` around lines 10 - 17, Remove the long implementation-detail JSDoc block that begins with "/** Returns the metrics map for the candidate (model, provider, region) ..." — delete that verbose comment and either leave no comment or replace it with a single-line comment that matches the function name and signature (the function that produces the metrics map / weighted aggregation over model_provider_mapping_history). Ensure you do not change the function implementation or types, only remove the unnecessary explanatory block.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/gateway/src/test-utils/gateway-api-test-harness.ts`:
- Around line 188-240: The test harness seed function setRoutingMetrics() now
inserts into modelProviderMappingHistory but the harness reset path doesn't
clear that table; modify the harness reset function (the reset function in
apps/gateway/src/test-utils/gateway-api-test-harness.ts) to remove persisted
routing history by issuing a delete on tables.modelProviderMappingHistory (or
otherwise truncating it) so seeded rows from setRoutingMetrics() are cleared
between tests.
---
Outside diff comments:
In `@apps/gateway/src/chat/chat.ts`:
- Around line 1998-2037: The routing logic currently re-enables providers that
get filtered out by getCheapestFromAvailableProviders (priority <= 0) by falling
back to first-array entries; update the branches that handle a null/undefined
cheapest/best result to fail closed: where you see the pattern "if
(cheapestResult) ... else { usedProvider = selectedProviders[0] ... }" (and
analogous fallbacks using eligibleMappings[0] or routingCandidates[0] and the
expression selectedMapping = bestRegionResult?.provider ?? eligibleMappings[0]),
either pre-filter the candidate arrays (selectedProviders, eligibleMappings,
routingCandidates) to remove any providerId with providerPriorities[providerId]
<= 0 before scoring, or change the else/null branch to return an error/abort
path (or set usedProvider/usedModel to undefined and propagate a failure) so a
null result from getCheapestFromAvailableProviders/getBestRegionResult cannot be
overridden by array[0]; locate uses around getCheapestFromAvailableProviders,
collapseProvidersToBestRegionPerProvider, bestRegionResult, and
routingCandidates to apply this change.
In
`@apps/ui/src/app/dashboard/`[orgId]/[projectId]/settings/routing/_components/routing-config-client.tsx:
- Around line 238-242: The upsell can render transiently because role is
undefined while membership data loads; introduce an explicit membership-loading
guard (e.g., isMembershipLoading = teamData === undefined || teamData.members
=== undefined) and only evaluate canManage or render RoutingContactSalesCard
when loading is complete. Update the existing canManage logic (which currently
uses role and selectedOrganization) to include !isMembershipLoading (or move the
check to the JSX where RoutingContactSalesCard is rendered) so authorized
enterprise admins see a loading state until teamData.members and role are
resolved.
- Around line 250-299: Replace the useEffect-based fetch with a TanStack Query:
create a useQuery (or two queries) that uses fetchClient.GET to load
"/routing-config/config/{projectId}/defaults" and
"/routing-config/config/{projectId}" (use projectId in the query key), set the
query's enabled flag to canManage, and move the side-effects currently in the
effect into the query's onSuccess and onError handlers—call setDefaults with
defaultsRes.data and setState with the mapped row (enabled, weights, thresholds,
retry, timeouts, history, providerPriorities) on success, and setError on
failure; remove the cancelled flag and manual setIsLoading and instead derive
loading state from the query status/isFetching and wire invalidation/retries to
the query layer so fetchClient.GET is only used inside the query function(s).
In `@apps/worker/src/services/stats-calculator.ts`:
- Around line 993-1007: The rollup code only updates entries present in
providerMap (and similarly for mappingAggregates/modelMap blocks), leaving
previously non-zero counters unchanged when there are no rows in the current
window; change the update loops (the one iterating providerMap and the similar
loops handling mappingAggregates/modelMap) to iterate the full set of target IDs
(all providers/models/mappings you intend to update) and use a default zero
aggregation (e.g., zeroAgg) when agg is undefined (agg ?? zeroAgg) so you write
zeros for counters when there is no current-window data; update the calls to
database.update(...) where you currently use agg.* to instead derive values from
(agg ?? zeroAgg) and ensure statsUpdatedAt/updatedAt are still set.
---
Nitpick comments:
In `@apps/gateway/src/lib/provider-metrics-for-routing.ts`:
- Around line 10-17: Remove the long implementation-detail JSDoc block that
begins with "/** Returns the metrics map for the candidate (model, provider,
region) ..." — delete that verbose comment and either leave no comment or
replace it with a single-line comment that matches the function name and
signature (the function that produces the metrics map / weighted aggregation
over model_provider_mapping_history). Ensure you do not change the function
implementation or types, only remove the unnecessary explanatory block.
In `@packages/db/src/provider-metrics-history.ts`:
- Around line 97-165: The query is being cached twice (swrWrap around the async
fetcher and Drizzle’s .$withCache on the cdb.select), which can cause divergent
staleness; choose one caching layer and remove the other. Either remove the
`.$withCache({ config: { ex: HISTORY_SWR_TTL_SECONDS } })` on the
`cdb.select(...).from(modelProviderMappingHistory)...` so SWR (`swrWrap`) is the
single source of truth, or remove `swrWrap` and let Drizzle’s `.$withCache`
control TTLs (ensuring `HISTORY_SWR_TTL_SECONDS` is used consistently if you
keep `.$withCache`); adjust code around `swrWrap`, `.$withCache`,
`HISTORY_SWR_TTL_SECONDS`, and the `cdb.select`/`modelProviderMappingHistory`
block accordingly.
In `@packages/db/src/schema.ts`:
- Around line 1771-1778: The RoutingHistoryConfig interface in
packages/db/src/schema.ts is a duplicate of the one in
packages/shared/src/routing-config.ts; remove the duplicated declaration and
import the shared type instead (replace the local export of RoutingHistoryConfig
with an import from the shared package and re-export if needed) so changes stay
centralized; if this duplication is intentional to avoid a circular dependency,
add a clear comment above the interface explaining why it must remain duplicated
and reference the original location (packages/shared/src/routing-config.ts) to
prevent future accidental merges.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 3864e599-0d40-4e75-85b1-9b7efc5195db
⛔ Files ignored due to path filters (5)
apps/code/src/lib/api/v1.d.tsis excluded by!**/v1.d.tsapps/playground/src/lib/api/v1.d.tsis excluded by!**/v1.d.tsapps/ui/src/lib/api/v1.d.tsis excluded by!**/v1.d.tsee/admin/src/lib/api/v1.d.tsis excluded by!**/v1.d.tspnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (27)
apps/api/package.jsonapps/api/src/routes/routing-config.tsapps/gateway/src/chat/chat.tsapps/gateway/src/fallback.spec.tsapps/gateway/src/lib/provider-metrics-for-routing.tsapps/gateway/src/lib/routing-config-loader.tsapps/gateway/src/lib/timeout-config.tsapps/gateway/src/test-utils/gateway-api-test-harness.tsapps/gateway/src/videos/videos.tsapps/ui/src/app/dashboard/[orgId]/[projectId]/settings/routing/_components/routing-config-client.tsxapps/ui/src/app/dashboard/[orgId]/[projectId]/settings/routing/_components/routing-contact-sales-card.tsxapps/worker/src/services/stats-calculator.spec.tsapps/worker/src/services/stats-calculator.tspackages/actions/src/get-cheapest-from-available-providers.tspackages/actions/src/models.spec.tspackages/db/migrations/1779716905_dizzy_tigra.sqlpackages/db/migrations/meta/1779716905_snapshot.jsonpackages/db/migrations/meta/_journal.jsonpackages/db/src/index.tspackages/db/src/provider-metrics-history.tspackages/db/src/provider-metrics.spec.tspackages/db/src/provider-metrics.tspackages/db/src/relations.tspackages/db/src/schema.tspackages/shared/src/index.tspackages/shared/src/routing-config.spec.tspackages/shared/src/routing-config.ts
💤 Files with no reviewable changes (2)
- packages/db/src/provider-metrics.spec.ts
- packages/db/src/provider-metrics.ts
Comment on lines
+188
to
+240
| // Routing now reads metrics on-demand from | ||
| // model_provider_mapping_history (see packages/db/src/provider-metrics-history.ts). | ||
| // Seed a single recent history row whose unweighted aggregates | ||
| // produce the requested uptime/latency/throughput. | ||
| const totalRequests = metrics.totalRequests ?? 100; | ||
| const latency = metrics.latency ?? 100; | ||
| const throughput = metrics.throughput ?? 100; | ||
| const uptimeFraction = metrics.uptime / 100; | ||
| const errorRate = 1 - uptimeFraction; | ||
| const errorsCount = Math.round(totalRequests * errorRate); | ||
| const totalDurationMs = 1000; // arbitrary | ||
| const totalOutputTokens = Math.round( | ||
| (throughput * totalDurationMs) / 1000, | ||
| ); | ||
| const totalTimeToFirstToken = latency * totalRequests; | ||
| const minuteTimestamp = new Date(Math.floor(Date.now() / 60000) * 60000); | ||
|
|
||
| await db | ||
| .update(tables.modelProviderMapping) | ||
| .set({ | ||
| status: "active", | ||
| routingUptime: metrics.uptime, | ||
| routingLatency: metrics.latency ?? 100, | ||
| routingThroughput: metrics.throughput ?? 100, | ||
| routingTotalRequests: metrics.totalRequests ?? 100, | ||
| .insert(tables.modelProviderMappingHistory) | ||
| .values({ | ||
| modelId, | ||
| providerId, | ||
| modelProviderMappingId: `${modelId}::${providerId}`, | ||
| minuteTimestamp, | ||
| logsCount: totalRequests, | ||
| errorsCount, | ||
| clientErrorsCount: 0, | ||
| gatewayErrorsCount: 0, | ||
| upstreamErrorsCount: errorsCount, | ||
| cachedCount: 0, | ||
| totalOutputTokens, | ||
| totalDuration: totalDurationMs, | ||
| totalTimeToFirstToken, | ||
| totalTimeToFirstReasoningToken: 0, | ||
| }) | ||
| .where( | ||
| and( | ||
| eq(tables.modelProviderMapping.modelId, modelId), | ||
| eq(tables.modelProviderMapping.providerId, providerId), | ||
| ), | ||
| ); | ||
| .onConflictDoUpdate({ | ||
| target: [ | ||
| tables.modelProviderMappingHistory.modelProviderMappingId, | ||
| tables.modelProviderMappingHistory.minuteTimestamp, | ||
| ], | ||
| set: { | ||
| logsCount: totalRequests, | ||
| errorsCount, | ||
| clientErrorsCount: 0, | ||
| gatewayErrorsCount: 0, | ||
| upstreamErrorsCount: errorsCount, | ||
| cachedCount: 0, | ||
| totalOutputTokens, | ||
| totalDuration: totalDurationMs, | ||
| totalTimeToFirstToken, | ||
| totalTimeToFirstReasoningToken: 0, | ||
| }, | ||
| }); |
Contributor
There was a problem hiding this comment.
Clear seeded routing history in harness reset to prevent cross-test state bleed.
setRoutingMetrics() now persists rows in modelProviderMappingHistory, but the harness reset path does not clear that table. This can make routing-sensitive tests order-dependent.
💡 Suggested fix
async function resetGatewayTestData() {
await db.delete(tables.log);
+ await db.delete(tables.modelProviderMappingHistory);
await db.delete(tables.webhookDeliveryLog);
await db.delete(tables.videoJob);
await db.delete(tables.apiKey);
await db.delete(tables.providerKey);
await db.delete(tables.userOrganization);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/gateway/src/test-utils/gateway-api-test-harness.ts` around lines 188 -
240, The test harness seed function setRoutingMetrics() now inserts into
modelProviderMappingHistory but the harness reset path doesn't clear that table;
modify the harness reset function (the reset function in
apps/gateway/src/test-utils/gateway-api-test-harness.ts) to remove persisted
routing history by issuing a delete on tables.modelProviderMappingHistory (or
otherwise truncating it) so seeded rows from setRoutingMetrics() are cleared
between tests.
The Metrics History Window description still said "Non-default values run a per-project aggregation against the 1-minute history table instead of the worker-rolled-up global values." The worker-rolled-up columns are gone — every request always aggregates from history now. Trim the second sentence so the copy matches what the gateway actually does. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/ui/src/app/dashboard/[orgId]/[projectId]/settings/routing/_components/routing-config-client.tsx (1)
250-298: ⚡ Quick winPrefer TanStack Query over useEffect for data fetching.
This manual fetch pattern in
useEffecthandles cancellation but bypasses benefits of TanStack Query (caching, automatic refetching, loading/error states, deduplication). Additionally, the emptycatchblock on line 287 swallows error details.♻️ Suggested refactor using TanStack Query
import { useQuery } from "`@tanstack/react-query`"; // Replace the useEffect with: const { data: defaults, isLoading: defaultsLoading } = useQuery({ queryKey: ["routing-config-defaults", projectId], queryFn: () => fetchClient.GET("/routing-config/config/{projectId}/defaults", { params: { path: { projectId } }, }).then(res => res.data as DefaultsResponse), enabled: canManage, }); const { data: configData, isLoading: configLoading } = useQuery({ queryKey: ["routing-config", projectId], queryFn: () => fetchClient.GET("/routing-config/config/{projectId}", { params: { path: { projectId } }, }).then(res => res.data), enabled: canManage, }); const isLoading = defaultsLoading || configLoading;As per coding guidelines: "Do not use useEffect for data fetching in the UI; use TanStack Query for all data fetching and state management".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/ui/src/app/dashboard/`[orgId]/[projectId]/settings/routing/_components/routing-config-client.tsx around lines 250 - 298, The current useEffect data-fetching block (the async IIFE that calls fetchClient.GET for "/routing-config/config/{projectId}/defaults" and "/routing-config/config/{projectId}" and then calls setDefaults and setState) should be replaced with TanStack Query useQuery hooks: create one useQuery for the defaults (key ["routing-config-defaults", projectId]) and one for the config (key ["routing-config", projectId]), each using fetchClient.GET(...).then(res => res.data) as the queryFn and enabled: canManage; remove the cancelled flag and the try/catch in the effect, derive isLoading as the OR of both queries' isLoading and error from their error values, and when query data is available map it into setDefaults and setState (casting to DefaultsResponse and RoutingConfigState shapes) instead of manual fetch logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In
`@apps/ui/src/app/dashboard/`[orgId]/[projectId]/settings/routing/_components/routing-config-client.tsx:
- Around line 250-298: The current useEffect data-fetching block (the async IIFE
that calls fetchClient.GET for "/routing-config/config/{projectId}/defaults" and
"/routing-config/config/{projectId}" and then calls setDefaults and setState)
should be replaced with TanStack Query useQuery hooks: create one useQuery for
the defaults (key ["routing-config-defaults", projectId]) and one for the config
(key ["routing-config", projectId]), each using fetchClient.GET(...).then(res =>
res.data) as the queryFn and enabled: canManage; remove the cancelled flag and
the try/catch in the effect, derive isLoading as the OR of both queries'
isLoading and error from their error values, and when query data is available
map it into setDefaults and setState (casting to DefaultsResponse and
RoutingConfigState shapes) instead of manual fetch logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 7311ccf0-3567-4d31-9bbd-52b68b11a335
📒 Files selected for processing (1)
apps/ui/src/app/dashboard/[orgId]/[projectId]/settings/routing/_components/routing-config-client.tsx
The built-in DEFAULT_ROUTING_TIMEOUTS values are also the hard ceiling
the infra layer enforces (upstream proxies, load balancers). A project
override only makes sense as a *shorter* timeout — pushing it higher
would silently get capped at the infra layer anyway and create
confusing diagnostics.
Three-layer guard:
- Shared resolver: Math.min(value, DEFAULT_ROUTING_TIMEOUTS[key]) so a
hand-edited DB row can't escape the ceiling either.
- API: zod .max(DEFAULT_ROUTING_TIMEOUTS.{gatewayMs,streamingMs,plainMs})
on each field for early 400 with a clear message.
- UI: NumericFieldRow gets min/max props, the input shows a red error
message inline, and handleSave refuses to PUT if any value exceeds
its ceiling. Card copy updated to explain the relationship.
Added a routing-config.spec test that confirms the resolver clamps a
deliberately-too-large override down to the default.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
apps/ui/src/app/dashboard/[orgId]/[projectId]/settings/routing/_components/routing-config-client.tsx (1)
269-317: ⚖️ Poor tradeoffData fetching uses
useEffectinstead of TanStack Query.This component fetches data via
useEffectwith manual cancellation handling. Per coding guidelines, TanStack Query should be used for all data fetching and state management in frontend apps. TanStack Query would provide automatic caching, deduplication, background refetching, and cleaner error/loading states.Since this appears to be the initial implementation of this component, consider refactoring to use
useQueryfrom TanStack Query.♻️ Conceptual refactor with TanStack Query
import { useQuery } from "`@tanstack/react-query`"; // Replace the useEffect with: const { data: defaults, isLoading: defaultsLoading } = useQuery({ queryKey: ["routing-config-defaults", projectId], queryFn: () => fetchClient.GET("/routing-config/config/{projectId}/defaults", { params: { path: { projectId } }, }).then(res => res.data as DefaultsResponse), enabled: canManage, }); const { data: configRow, isLoading: configLoading } = useQuery({ queryKey: ["routing-config", projectId], queryFn: () => fetchClient.GET("/routing-config/config/{projectId}", { params: { path: { projectId } }, }).then(res => res.data), enabled: canManage, }); const isLoading = defaultsLoading || configLoading;As per coding guidelines: "Do not use useEffect for data fetching in the UI; use TanStack Query for all data fetching and state management."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/ui/src/app/dashboard/`[orgId]/[projectId]/settings/routing/_components/routing-config-client.tsx around lines 269 - 317, The current useEffect data-fetching block should be replaced with TanStack Query useQuery calls: create two queries using useQuery({ queryKey: ["routing-config-defaults", projectId], queryFn: () => fetchClient.GET("/routing-config/config/{projectId}/defaults", { params: { path: { projectId } } }).then(r => r.data as DefaultsResponse), enabled: canManage }) and useQuery({ queryKey: ["routing-config", projectId], queryFn: () => fetchClient.GET("/routing-config/config/{projectId}", { params: { path: { projectId } } }).then(r => r.data), enabled: canManage }); then derive isLoading from the queries' isLoading flags, setDefaults from the defaults query data and setState from the config query data (mapping to enabled, weights, thresholds, retry, timeouts, history, providerPriorities), and use the queries' error states instead of the manual try/catch, removing the cancelled flag and the useEffect block that references setIsLoading, setDefaults, setState, and setError.apps/api/src/routes/routing-config.ts (1)
330-337: 💤 Low valuePotential undefined response when row doesn't exist and body is empty.
If
onConflictDoNothingis used (emptyconflictSet) and the row already exists, it correctly falls back. However, if somehow the fallback query also returns nothing (e.g., concurrent delete),resultwould beundefinedandc.json(result)would returnnull. This is an unlikely edge case but the response type declaresroutingConfigRowSchema(non-nullable).Consider adding a guard or adjusting the response schema to allow
null.🛡️ Optional defensive guard
const result = row ?? (await db.query.routingConfig.findFirst({ where: { projectId: { eq: projectId } }, })); + + if (!result) { + throw new HTTPException(500, { message: "Failed to persist routing configuration" }); + } await invalidateRoutingConfigCache(); return c.json(result);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/routes/routing-config.ts` around lines 330 - 337, The code may return undefined for result (from row ?? db.query.routingConfig.findFirst(...)) causing c.json(result) to violate the non-null routingConfigRowSchema; add a defensive guard after the query: check if result is falsy and respond with an explicit not-found or error (for example use c.status(404).json({ message: 'Routing config not found' }) or throw a typed HTTP error) before calling invalidateRoutingConfigCache() and c.json(result); alternatively, if null is acceptable, update the response schema (routingConfigRowSchema) to allow null — modify the logic around result, db.query.routingConfig.findFirst, invalidateRoutingConfigCache, and c.json accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@apps/api/src/routes/routing-config.ts`:
- Around line 330-337: The code may return undefined for result (from row ??
db.query.routingConfig.findFirst(...)) causing c.json(result) to violate the
non-null routingConfigRowSchema; add a defensive guard after the query: check if
result is falsy and respond with an explicit not-found or error (for example use
c.status(404).json({ message: 'Routing config not found' }) or throw a typed
HTTP error) before calling invalidateRoutingConfigCache() and c.json(result);
alternatively, if null is acceptable, update the response schema
(routingConfigRowSchema) to allow null — modify the logic around result,
db.query.routingConfig.findFirst, invalidateRoutingConfigCache, and c.json
accordingly.
In
`@apps/ui/src/app/dashboard/`[orgId]/[projectId]/settings/routing/_components/routing-config-client.tsx:
- Around line 269-317: The current useEffect data-fetching block should be
replaced with TanStack Query useQuery calls: create two queries using useQuery({
queryKey: ["routing-config-defaults", projectId], queryFn: () =>
fetchClient.GET("/routing-config/config/{projectId}/defaults", { params: { path:
{ projectId } } }).then(r => r.data as DefaultsResponse), enabled: canManage })
and useQuery({ queryKey: ["routing-config", projectId], queryFn: () =>
fetchClient.GET("/routing-config/config/{projectId}", { params: { path: {
projectId } } }).then(r => r.data), enabled: canManage }); then derive isLoading
from the queries' isLoading flags, setDefaults from the defaults query data and
setState from the config query data (mapping to enabled, weights, thresholds,
retry, timeouts, history, providerPriorities), and use the queries' error states
instead of the manual try/catch, removing the cancelled flag and the useEffect
block that references setIsLoading, setDefaults, setState, and setError.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 55e90705-5493-4d36-8369-5576cf2e16d4
📒 Files selected for processing (4)
apps/api/src/routes/routing-config.tsapps/ui/src/app/dashboard/[orgId]/[projectId]/settings/routing/_components/routing-config-client.tsxpackages/shared/src/routing-config.spec.tspackages/shared/src/routing-config.ts
Main added stable preferred-provider routing (PR #2351) that keeps a project routed to the same provider for a model as long as it stays healthy and competitive. The three knobs that drive it (TTL, uptime threshold, score margin) plus a global on/off were env-var only. Surface them as a new "sticky" group on routing_config, the same way the other knobs are exposed: - DB jsonb column on routing_config - shared RoutingStickyConfig type + DEFAULT_ROUTING_STICKY (mirror preferred-provider.ts defaults exactly) + clampSticky (uptime threshold clamped to 0-100, ttl >= 1s, scoreMargin >= 0) - preferred-provider.ts now accepts an optional cfg arg; when given, it bypasses the env lookups and uses cfg.{ttlSeconds,uptimeThreshold, scoreMargin} directly. cfg-less callers still hit the existing env fallbacks so nothing changes for non-enterprise orgs. - chat.ts hysteresis block now skips entirely when routingCfg.sticky.enabled is false and threads cfg through to set/resolve calls. - API: new stickySchema (ttlSeconds <= 24h, uptime 0-100, scoreMargin 0-10), wired into PUT body + resolved + defaults endpoints. - UI: new "Sticky Routing" card with the same Enabled switch pattern as the top-level toggle, plus three NumericFieldRow rows. - Spec adds clamping + defaults coverage. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…to enterprise-routing-overrides
This was referenced Jun 3, 2026
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
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
routing_configtable (per-project) lets enterprise orgs override the hardcoded routing knobs: scoring weights (price / uptime / throughput / latency / cache / image-price), thresholds (cache prompt size, uptime penalty curve, defaults for missing metrics, exploration rate), retry / low-uptime fallback, request timeouts, and per-provider priorities (set to0to fully disable a provider for the project).ContactSalesCardupsell for non-enterprise orgs.override -> env var -> built-in default, so existing tests and deployments behave identically out of the box.Test plan
pnpm test:unit— 1589 passing, including newpackages/shared/src/routing-config.spec.tsand new override cases inpackages/actions/src/models.spec.tspnpm build— full monorepo build passes (api openapi regenerated, UI fetch-client types regenerated)pnpm format— clean/dashboard/{orgId}/{projectId}/settings/routing, setproviderPriorities[openai] = 0, confirm OpenAI is excluded from routing in the gateway; non-enterprise org sees the upsell card.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Refactor