Skip to content

Conversation

@shariqriazz
Copy link
Contributor

@shariqriazz shariqriazz commented Aug 24, 2025

  • Add support for up to 4 OpenRouter providers with automatic failover
  • Implement priority-based provider selection in UI with dynamic dropdowns
  • Add comprehensive test coverage for multi-provider functionality
  • Enable seamless switching between single and multi-provider modes
  • Ensure primary provider's context window and pricing is used for model info

Important

Adds multi-provider failover support for OpenRouter with UI updates and comprehensive test coverage, ensuring backward compatibility.

  • Behavior:
    • Adds support for up to 4 OpenRouter providers with automatic failover in openrouter.ts.
    • Implements priority-based provider selection in ApiOptions.tsx with dynamic dropdowns.
    • Ensures primary provider's context window and pricing is used for model info in useSelectedModel.ts.
  • Tests:
    • Adds comprehensive test coverage for multi-provider functionality in openrouter-multi-provider.spec.ts and useSelectedModel-multiProvider.spec.ts.
  • UI:
    • Updates ApiOptions.tsx and OpenRouter.tsx to enable seamless switching between single and multi-provider modes.
    • Adds UI elements for multi-provider selection and failover enablement.
  • Misc:
    • Keeps openRouterSpecificProvider for backward compatibility in provider-settings.ts.

This description was created by Ellipsis for f586d7e. You can customize this summary. It will automatically update as commits are pushed.

- Add support for up to 4 OpenRouter providers with automatic failover
- Implement priority-based provider selection in UI with dynamic dropdowns
- Add comprehensive test coverage for multi-provider functionality
- Enable seamless switching between single and multi-provider modes
- Ensure primary provider's context window and pricing is used for model info
@shariqriazz shariqriazz requested review from cte, jr and mrubens as code owners August 24, 2025 00:43
@dosubot dosubot bot added size:XL This PR changes 500-999 lines, ignoring generated files. enhancement New feature or request labels Aug 24, 2025
@shariqriazz shariqriazz marked this pull request as draft August 24, 2025 00:43
<Checkbox
checked={apiConfiguration?.openRouterFailoverEnabled ?? true}
onChange={handleInputChange("openRouterFailoverEnabled", noTransform)}>
Enable automatic failover
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use the translation function (e.g., t('settings:providers.openRouter.multiProvider.failoverEnabled')) for the checkbox label instead of the hardcoded string "Enable automatic failover".

Suggested change
Enable automatic failover
{t('settings:providers.openRouter.multiProvider.failoverEnabled')}

This comment was generated because it violated a code review rule: irule_C0ez7Rji6ANcGkkX.

{apiConfiguration?.openRouterFailoverEnabled ? (
<div key="multi-provider-mode">
<label className="block font-medium mb-2">
Multiple Providers (up to 4)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The multi-provider mode title is hardcoded as 'Multiple Providers (up to 4)'. Use a translatable string (e.g., t('settings:providers.openRouter.multiProvider.title')).

Suggested change
Multiple Providers (up to 4)
{t('settings:providers.openRouter.multiProvider.title')}

This comment was generated because it violated a code review rule: irule_C0ez7Rji6ANcGkkX.

key={`provider-${index}-${apiConfiguration?.openRouterFailoverEnabled}`}
className="mb-2">
<label className="block text-sm font-medium mb-1">
{index === 0
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Provider role labels ('Primary Provider', 'Secondary Provider', etc.) are hardcoded. These should be replaced with translation keys (e.g., t('settings:providers.openRouter.multiProvider.primaryProvider')) for consistency.

This comment was generated because it violated a code review rule: irule_C0ez7Rji6ANcGkkX.

Copy link
Contributor

@roomote roomote bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your contribution! I've reviewed the OpenRouter multi-provider failover implementation and found several issues that need attention before merging.

{apiConfiguration?.openRouterFailoverEnabled ? (
<div key="multi-provider-mode">
<label className="block font-medium mb-2">
Multiple Providers (up to 4)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing i18n translations here. These hardcoded English strings break internationalization:

  • "Multiple Providers (up to 4)"
  • "Primary Provider", "Secondary Provider", etc.
  • "Configure multiple providers in priority order..."

Consider using translation keys like:

Suggested change
Multiple Providers (up to 4)
<label className="block font-medium mb-2">
{t("settings:providers.openRouter.multiProvider.title")}
</label>

<Checkbox
checked={apiConfiguration?.openRouterFailoverEnabled ?? true}
onChange={handleInputChange("openRouterFailoverEnabled", noTransform)}>
Enable automatic failover
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These hardcoded strings should use i18n translation keys:

Suggested change
Enable automatic failover
{t("settings:providers.openRouter.multiProvider.failoverEnabled")}

and

Suggested change
Enable automatic failover
<div className="text-sm text-vscode-descriptionForeground mt-1">
{t("settings:providers.openRouter.multiProvider.failoverDescription")}
</div>

}

// This should never be reached, but just in case
throw lastError || new Error("All OpenRouter providers failed")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This error message doesn't provide useful context about which providers were tried or what errors occurred. Consider:

Suggested change
throw lastError || new Error("All OpenRouter providers failed")
const providersSummary = providers.join(', ');
throw lastError || new Error(`All OpenRouter providers failed (tried: ${providersSummary}). Last error: ${lastError?.message || 'Unknown'}`);

for (let providerIndex = 0; providerIndex < providers.length; providerIndex++) {
try {
const currentProvider = providers[providerIndex]
console.log(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider using a proper logging service or making these debug-only logs. Production console logs can impact performance and clutter user consoles:

Suggested change
console.log(
if (process.env.NODE_ENV === 'development') {
console.log(
`[OpenRouter] Attempting request with provider: ${currentProvider} (${providerIndex + 1}/${providers.length})`,
)
}

openRouterBaseUrl: z.string().optional(),
openRouterSpecificProvider: z.string().optional(),
openRouterSpecificProvider: z.string().optional(), // Keep for backward compatibility
openRouterProviders: z.array(z.string()).max(4).optional(), // New multi-provider support
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The 4-provider limit is a magic number. Consider defining it as a constant for better maintainability:

Suggested change
openRouterProviders: z.array(z.string()).max(4).optional(), // New multi-provider support
const MAX_OPENROUTER_PROVIDERS = 4;
// ...
openRouterProviders: z.array(z.string()).max(MAX_OPENROUTER_PROVIDERS).optional(),

const providers = (handler as any).getProvidersToUse()
expect(providers).toEqual(["multi1", "multi2"])
})
})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider adding integration tests that simulate actual API failures with mock responses. The current tests only verify the logic but don't test the actual failover behavior with real-world error scenarios.

/**
* Get the list of providers to use, supporting both new multi-provider and legacy single provider config
*/
private getProvidersToUse(): string[] {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider adding JSDoc comments to explain the purpose and parameters of these new methods. This would improve code maintainability:

Suggested change
private getProvidersToUse(): string[] {
/**
* Get the list of providers to use for requests, supporting both multi-provider and legacy configurations
* @returns Array of provider names in priority order
*/
private getProvidersToUse(): string[] {

"title": "OpenRouter Provider Routing",
"description": "OpenRouter routes requests to the best available providers for your model. By default, requests are load balanced across the top providers to maximize uptime. However, you can choose a specific provider to use for this model.",
"learnMore": "Learn more about provider routing"
},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good that you started adding i18n keys here, but the implementation is incomplete. You need to add all the missing keys mentioned in my other comments:

  • multiProvider.failoverEnabled
  • multiProvider.failoverDescription
  • The provider priority labels

Also ensure these are added to all other locale files.

@hannesrudolph hannesrudolph added the Issue/PR - Triage New issue. Needs quick review to confirm validity and assign labels. label Aug 24, 2025
@shariqriazz
Copy link
Contributor Author

@roomote-agent Don't be shy, you can address all the issues you have found then switch to translate and fix translations as well. make sure you do it in same PR and not create new branches

@roomote
Copy link
Contributor

roomote bot commented Aug 24, 2025

Hi @shariqriazz! I see the review comments and will address all the issues found:

  1. Fix missing i18n translations for hardcoded strings
  2. Add proper error context in failover error messages
  3. Make console logs development-only
  4. Define MAX_OPENROUTER_PROVIDERS constant
  5. Add JSDoc comments for new methods
  6. Add translations to all locale files

I'll work on these fixes and also ensure all translations are properly added across all locale files. Working on it now!

@roomote
Copy link
Contributor

roomote bot commented Aug 24, 2025

Hi @shariqriazz! ✅ All requested changes have been completed and pushed to the PR:

Changes Made:

🌐 i18n Fixes

  • ✅ Replaced all hardcoded strings with translation keys in OpenRouter.tsx and ApiOptions.tsx
  • ✅ Added missing translation keys to English locale file
  • ✅ Added translations to all 17 other locale files (ca, de, es, fr, hi, id, it, ja, ko, nl, pl, pt-BR, ru, tr, vi, zh-CN, zh-TW)

📝 Code Quality Improvements

  • ✅ Added MAX_OPENROUTER_PROVIDERS constant to avoid magic numbers
  • ✅ Improved error messages to include provider context and details
  • ✅ Made console logs development-only using process.env.NODE_ENV checks
  • ✅ Added comprehensive JSDoc comments to all new methods

✅ Testing

  • All tests passing locally:
    • openrouter-multi-provider.spec.ts: 15 tests passed
    • useSelectedModel-multiProvider.spec.ts: 4 tests passed
  • Translation validation script confirms all translations are complete

The changes maintain backward compatibility while addressing all the review feedback. The CI checks from the previous commit are showing failures, but those should be resolved with this new commit once the CI runs again.

@daniel-lxs daniel-lxs moved this from Triage to PR [Draft / In Progress] in Roo Code Roadmap Aug 25, 2025
@hannesrudolph hannesrudolph added PR - Draft / In Progress and removed Issue/PR - Triage New issue. Needs quick review to confirm validity and assign labels. labels Aug 25, 2025
@RuntimeFlash
Copy link

+1 required

@github-project-automation github-project-automation bot moved this from New to Done in Roo Code Roadmap Sep 23, 2025
@github-project-automation github-project-automation bot moved this from PR [Draft / In Progress] to Done in Roo Code Roadmap Sep 23, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request PR - Draft / In Progress size:XL This PR changes 500-999 lines, ignoring generated files.

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

3 participants