feat(settings): 9router-style Routing Strategy card + sticky parity#6678
feat(settings): 9router-style Routing Strategy card + sticky parity#6678SeaXen wants to merge 4 commits into
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a Routing Strategy configuration interface to achieve 9router parity, enabling global and per-provider overrides for account round-robin, sticky limits, and combo rotation. The reviewer feedback highlights several critical and high-severity issues that need to be addressed: a potential data-loss bug in the provider strategy saving logic where other provider strategies could be wiped out on failure, a missing import of clampStickyRoundRobinTargetLimit in rrState.ts, and UX issues in the input fields where clamping values on onChange prevents natural typing. Additionally, the feedback suggests catching errors in useEffect hooks and using optional chaining to prevent runtime errors.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| const res = await fetch("/api/settings", { cache: "no-store" }); | ||
| const data = res.ok ? await res.json() : {}; |
There was a problem hiding this comment.
If the GET request to /api/settings fails or returns a non-OK response, res.ok is false and data defaults to {}. This means current (which holds all other provider strategies) will also be empty {}. When the subsequent PATCH request is sent, it will write { providerStrategies: { [providerKey]: override } }, which will completely wipe out all other provider strategies in the database.
To prevent this critical data loss, you should throw an error and abort the save operation if the GET request fails.
| const res = await fetch("/api/settings", { cache: "no-store" }); | |
| const data = res.ok ? await res.json() : {}; | |
| const res = await fetch("/api/settings", { cache: "no-store" }); | |
| if (!res.ok) throw new Error("Failed to fetch current settings"); | |
| const data = await res.json(); |
| if (perComboLimit !== undefined && perComboLimit !== null) { | ||
| return clampStickyRoundRobinTargetLimit(perComboLimit); | ||
| } | ||
| const comboSticky = settings?.comboStickyRoundRobinLimit; | ||
| if (comboSticky !== undefined && comboSticky !== null) { | ||
| return clampStickyRoundRobinTargetLimit(comboSticky); | ||
| } | ||
| return clampStickyRoundRobinTargetLimit(settings?.stickyRoundRobinLimit); |
There was a problem hiding this comment.
The function clampStickyRoundRobinTargetLimit is used in resolveComboStickyRoundRobinLimit but is not imported or defined in this file. This will cause a compilation or runtime error. Please ensure it is imported, or if it is defined in open-sse/services/combo.ts, move its definition to rrState.ts (or a shared utility file) to avoid a circular dependency between combo.ts and rrState.ts.
| if (nextStrategy === "round-robin" && nextSticky !== "") { | ||
| override.stickyRoundRobinLimit = Number(nextSticky) || 3; | ||
| } |
There was a problem hiding this comment.
The Zod schema in settingsSchemas.ts enforces stickyRoundRobinLimit: z.number().int().min(1).max(1000). If a user types or pastes an invalid value (e.g., 0 or a negative number), Number(nextSticky) || 3 could result in an invalid value being sent to the server, causing a Zod validation error (400 Bad Request) and failing to save. We should clamp the value on the client side before saving.
| if (nextStrategy === "round-robin" && nextSticky !== "") { | |
| override.stickyRoundRobinLimit = Number(nextSticky) || 3; | |
| } | |
| if (nextStrategy === "round-robin" && nextSticky !== "") { | |
| const val = parseInt(nextSticky, 10); | |
| override.stickyRoundRobinLimit = Math.min(10, Math.max(1, isNaN(val) ? 3 : val)); | |
| } |
| const data = await fetch("/api/settings", { cache: "no-store" }).then((r) => r.json()); | ||
| const override = ((data.providerStrategies || {}) as Record<string, ProviderStrategyOverride>)[ |
There was a problem hiding this comment.
Use optional chaining when accessing providerStrategies on data to prevent potential runtime TypeErrors if data is null, undefined, or not an object.
| const data = await fetch("/api/settings", { cache: "no-store" }).then((r) => r.json()); | |
| const override = ((data.providerStrategies || {}) as Record<string, ProviderStrategyOverride>)[ | |
| const data = await fetch("/api/settings", { cache: "no-store" }).then((r) => r.ok ? r.json() : null); | |
| const override = ((data?.providerStrategies || {}) as Record<string, ProviderStrategyOverride>)[ | |
| providerKey | |
| ]; |
| type RoutingSettings = { | ||
| fallbackStrategy?: string; | ||
| stickyRoundRobinLimit?: number; | ||
| comboStrategy?: string; | ||
| comboStickyRoundRobinLimit?: number; | ||
| }; |
There was a problem hiding this comment.
Update the RoutingSettings type definition to allow string values for the limit fields. This is necessary to support storing the raw string input in the state while the user is typing, before it is parsed and clamped on blur.
| type RoutingSettings = { | |
| fallbackStrategy?: string; | |
| stickyRoundRobinLimit?: number; | |
| comboStrategy?: string; | |
| comboStickyRoundRobinLimit?: number; | |
| }; | |
| type RoutingSettings = { | |
| fallbackStrategy?: string; | |
| stickyRoundRobinLimit?: number | string; | |
| comboStrategy?: string; | |
| comboStickyRoundRobinLimit?: number | string; | |
| }; |
| onChange={(e) => { | ||
| const num = Math.min(10, Math.max(1, parseInt(e.target.value, 10) || 3)); | ||
| setSettings((prev) => ({ ...prev, stickyRoundRobinLimit: num })); | ||
| }} | ||
| onBlur={() => | ||
| run(async () => { | ||
| const limit = settings.stickyRoundRobinLimit ?? 3; | ||
| const updated = await patchSettings({ stickyRoundRobinLimit: limit }); | ||
| setSettings((prev) => ({ | ||
| ...prev, | ||
| stickyRoundRobinLimit: updated.stickyRoundRobinLimit ?? limit, | ||
| })); | ||
| }) | ||
| } |
There was a problem hiding this comment.
Clamping and defaulting the value in the onChange handler makes it extremely difficult for users to type or edit the input. For example, if a user presses backspace to clear the input, parseInt("", 10) returns NaN, which immediately defaults to 3, preventing them from clearing the input.
Instead, allow the state to hold the raw string value during typing, and only clamp/validate on onBlur when saving.
| onChange={(e) => { | |
| const num = Math.min(10, Math.max(1, parseInt(e.target.value, 10) || 3)); | |
| setSettings((prev) => ({ ...prev, stickyRoundRobinLimit: num })); | |
| }} | |
| onBlur={() => | |
| run(async () => { | |
| const limit = settings.stickyRoundRobinLimit ?? 3; | |
| const updated = await patchSettings({ stickyRoundRobinLimit: limit }); | |
| setSettings((prev) => ({ | |
| ...prev, | |
| stickyRoundRobinLimit: updated.stickyRoundRobinLimit ?? limit, | |
| })); | |
| }) | |
| } | |
| onChange={(e) => { | |
| setSettings((prev) => ({ ...prev, stickyRoundRobinLimit: e.target.value })); | |
| }} | |
| onBlur={() => | |
| run(async () => { | |
| const limit = Math.min(10, Math.max(1, parseInt(String(settings.stickyRoundRobinLimit), 10) || 3)); | |
| const updated = await patchSettings({ stickyRoundRobinLimit: limit }); | |
| setSettings((prev) => ({ | |
| ...prev, | |
| stickyRoundRobinLimit: updated.stickyRoundRobinLimit ?? limit, | |
| })); | |
| }) | |
| } |
| onChange={(e) => { | ||
| const num = Math.min(100, Math.max(1, parseInt(e.target.value, 10) || 1)); | ||
| setSettings((prev) => ({ ...prev, comboStickyRoundRobinLimit: num })); | ||
| }} | ||
| onBlur={() => | ||
| run(async () => { | ||
| const limit = settings.comboStickyRoundRobinLimit ?? 1; | ||
| const updated = await patchSettings({ comboStickyRoundRobinLimit: limit }); | ||
| setSettings((prev) => ({ | ||
| ...prev, | ||
| comboStickyRoundRobinLimit: updated.comboStickyRoundRobinLimit ?? limit, | ||
| })); | ||
| }) | ||
| } |
There was a problem hiding this comment.
Similar to the stickyRoundRobinLimit input, clamping and defaulting the value in onChange prevents users from clearing the input or typing freely. Let's defer the clamping and validation to onBlur.
| onChange={(e) => { | |
| const num = Math.min(100, Math.max(1, parseInt(e.target.value, 10) || 1)); | |
| setSettings((prev) => ({ ...prev, comboStickyRoundRobinLimit: num })); | |
| }} | |
| onBlur={() => | |
| run(async () => { | |
| const limit = settings.comboStickyRoundRobinLimit ?? 1; | |
| const updated = await patchSettings({ comboStickyRoundRobinLimit: limit }); | |
| setSettings((prev) => ({ | |
| ...prev, | |
| comboStickyRoundRobinLimit: updated.comboStickyRoundRobinLimit ?? limit, | |
| })); | |
| }) | |
| } | |
| onChange={(e) => { | |
| setSettings((prev) => ({ ...prev, comboStickyRoundRobinLimit: e.target.value })); | |
| }} | |
| onBlur={() => | |
| run(async () => { | |
| const limit = Math.min(100, Math.max(1, parseInt(String(settings.comboStickyRoundRobinLimit), 10) || 1)); | |
| const updated = await patchSettings({ comboStickyRoundRobinLimit: limit }); | |
| setSettings((prev) => ({ | |
| ...prev, | |
| comboStickyRoundRobinLimit: updated.comboStickyRoundRobinLimit ?? limit, | |
| })); | |
| }) | |
| } |
| useEffect(() => { | ||
| load(); | ||
| }, [load]); |
There was a problem hiding this comment.
The load function is asynchronous and can throw errors (e.g., if the network request fails). Calling it inside useEffect without catching errors will result in an unhandled promise rejection.
| useEffect(() => { | |
| load(); | |
| }, [load]); | |
| useEffect(() => { | |
| load().catch(console.error); | |
| }, [load]); |
|
Addressed gemini-code-assist review in latest push:
Semgrep already passing on prior commit. |
…o sticky override (diegosouzapw#6678) Add a Routing Strategy settings card (Settings -> Routing) surfacing account round-robin/sticky-limit knobs plus a new combo-level sticky round-robin (comboStickyRoundRobinLimit), and a per-provider account-routing override (providerStrategies) wired into getProviderCredentials() ahead of the global fallback strategy. Rebased onto release/v3.8.47 (credit-preserving reconstruction: unrelated package.json/electron/proxyDispatcher drift from the PR's stale base was dropped, only the author's own 12 files were re-applied). Split ProviderAccountRoutingCard/RoutingStrategyCard into smaller hook+subcomponent pieces to stay under the frozen complexity/file-size gates; rebaselined ProviderDetailPageClient.tsx/auth.ts's frozen file-size caps for the small additive growth. Co-authored-by: diegosouzapw <8016841+diegosouzapw@users.noreply.github.com>
eac4136 to
e9c2d75
Compare
…ap.testFiles (diegosouzapw#6678) CI's Fast Quality Gates -> check:mutation-test-coverage --strict flagged the new test as missing from stryker.conf.json's tap.testFiles (it covers the mutated module open-sse/services/combo/rrState.ts). Adds the single entry, alphabetized next to the existing combo-rr-fallback-advance-948.test.ts. Co-authored-by: diegosouzapw <8016841+diegosouzapw@users.noreply.github.com>
…er-routing-strategy-ui
Co-authored-by: diegosouzapw <8016841+diegosouzapw@users.noreply.github.com>
Problem
Users comparing 9router Profile → Routing Strategy could not find equivalent controls in OmniRoute (sticky round-robin was buried under advanced combo defaults).
Solution
comboStrategy,comboStickyRoundRobinLimit,providerStrategiesproviderStrategieshonored inauth.ts; combo sticky usescomboStickyRoundRobinLimitbefore global account stickyHow to verify
Tests
tests/unit/combo-rr-sticky-9router.test.tssettings-ui-layout-static.test.tsupdated for card order