feat: add custom base URL support for Requesty provider #7337
Conversation
fixed URL construction for models and balance endpoints
Co-authored-by: ellipsis-dev[bot] <65095814+ellipsis-dev[bot]@users.noreply.github.com>
dosubot
bot
added
size:L
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
src/shared/utils/requesty.ts
Outdated
| if (type === "router") { | ||
| return baseUrl | ||
| } else { | ||
| return baseUrl.replace("router", type).replace("v1", "") |
There was a problem hiding this comment.
The replaceCname function uses simple string replacement (.replace) for both 'router' and 'v1'. While this works for the expected default URL, if a custom base URL contains these substrings in an unexpected location, it may yield unintended results. Consider using more precise matching (e.g. regex with boundaries) to ensure only the intended parts are replaced.
| return baseUrl.replace("router", type).replace("v1", "") | |
| return baseUrl.replace(/\brouter\b/, type).replace(/\/v1\b/, "") |
![roomote[bot]](https://avatars.githubusercontent.com/in/1546624?s=60&v=4){kind=small}
There was a problem hiding this comment.
Thank you for your contribution! I've reviewed the changes and found some issues that need attention. The implementation successfully addresses the core requirement of supporting custom base URLs for Requesty, but there are concerns about the URL transformation logic and missing test coverage.
Critical Issues:
-
URL transformation logic is fragile (
src/shared/utils/requesty.ts): ThereplaceCnamefunction uses simple string replacement that could fail with custom domains. For example,https://my-router.example.com/v1would incorrectly transform tohttps://my-app.example.com/when switching to "app" service. -
Missing unit tests: The new
toRequestyServiceUrlutility function has no test coverage. This is critical functionality that should be thoroughly tested.
Important Suggestions:
-
Parameter order change (
src/api/providers/fetchers/requesty.ts): The function signature was changed to(baseUrl?, apiKey?)which could be confusing since apiKey is more commonly the primary parameter. -
Hardcoded OAuth path (
webview-ui/src/components/settings/providers/Requesty.tsx): Isoauth/authorize?callback_url=guaranteed to be the same for all custom Requesty instances?
Minor Improvements:
-
Unused function (
webview-ui/src/oauth/urls.ts): ThegetRequestyAuthUrlfunction appears to be unused now. -
Missing JSDoc comments: The
toRequestyServiceUrlfunction would benefit from documentation.
| import { toRequestyServiceUrl } from "../../../shared/utils/requesty" | ||
|
|
||
| export async function getRequestyModels(apiKey?: string): Promise<Record<string, ModelInfo>> { | ||
| export async function getRequestyModels(baseUrl?: string, apiKey?: string): Promise<Record<string, ModelInfo>> { |
There was a problem hiding this comment.
Is there a specific reason for changing the parameter order to (baseUrl?, apiKey?) instead of keeping apiKey as the first parameter? Most other provider functions have apiKey as the primary parameter. This could be confusing for consistency.
| const callbackUrl = getCallbackUrl("requesty", uriScheme) | ||
| const baseUrl = toRequestyServiceUrl(apiConfiguration.requestyBaseUrl, "app") | ||
|
|
||
| const authUrl = new URL(`oauth/authorize?callback_url=${callbackUrl}`, baseUrl) |
There was a problem hiding this comment.
Is the OAuth path oauth/authorize?callback_url= guaranteed to be the same for all custom Requesty instances? If custom instances might have different OAuth endpoints, this should be configurable or documented.
hannesrudolph
added
Issue/PR - Triage
- Add dedicated test suite for toRequestyServiceUrl function - Test default behavior, custom URLs, service types, and edge cases - Improve robustness of URL handling with proper validation - Add JSDoc documentation for clarity - Address PR feedback about robustness and test coverage
github-project-automation
bot
moved this from PR [Needs Review]
to Done
in Roo Code Roadmap
Description
This PR adds support for custom base URLs in the Requesty provider, allowing users to configure their own Requesty instance endpoints.
Changes
toRequestyServiceUrlutility function to handle base URL resolutionRequestyHandlerto use custom base URL when providedgetRequestyModelsto accept base URL as first parameterRelated Issues
Replaces #7275 (couldn't push to fork)
Closes #7274
Test Procedure
Tests
✅ All tests passing locally
requesty.spec.tsto use valid URL formatmodelCache.spec.tsto match new parameter orderImportant
Adds custom base URL support for Requesty provider, updating URL handling, model fetching, and UI components.
toRequestyServiceUrlfunction inrequesty.tsfor custom base URL resolution.RequestyHandlerinrequesty.tsto use custom base URL if provided.getRequestyModelsinrequesty.tsto accept base URL as a parameter.ApiOptions.tsxandRequesty.tsxto support base URL configuration.requesty.spec.tsandmodelCache.spec.tsto match new function signatures.toRequestyServiceUrlinrequesty.spec.ts.GetModelsOptionsinapi.tsto includebaseUrlforrequestyprovider.This description was created by
for c5e918b. You can customize this summary. It will automatically update as commits are pushed.