-
Notifications
You must be signed in to change notification settings - Fork 2.6k
fix: use custom base URL for all Requesty requests #7276
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
- Update OAuth URL generation to use custom base URL when provided - Fix API key button to use custom base URL for web interface - Update API key info endpoint to use custom base URL - Fix models endpoint to use custom base URL - Pass requestyBaseUrl through all necessary contexts Fixes #7274
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewing my own code is like grading my own homework - suspicious but necessary.
| import { parseApiPrice } from "../../../shared/cost" | ||
|
|
||
| export async function getRequestyModels(apiKey?: string): Promise<Record<string, ModelInfo>> { | ||
| export async function getRequestyModels(apiKey?: string, baseUrl?: string): Promise<Record<string, ModelInfo>> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new baseUrl parameter works, but there's no test coverage for this functionality. Could we add unit tests to verify the URL transformations work correctly with different subdomain patterns (router/api/app)?
| const cleanBaseUrl = baseUrl.replace(/\/$/, "") | ||
| // If the base URL contains 'router' or 'api', replace with 'app' for web interface | ||
| const appUrl = cleanBaseUrl | ||
| .replace(/router\.requesty/, "app.requesty") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This URL transformation logic is duplicated in RequestyBalanceDisplay.tsx and urls.ts. Would it make sense to extract this into a shared utility function like transformRequestyUrlToApp(baseUrl: string) to maintain consistency?
| // Use the base URL if provided, otherwise default to the standard API endpoint | ||
| const apiUrl = baseUrl ? baseUrl.replace(/\/$/, "") : "https://api.requesty.ai" | ||
| // Convert router.requesty.ai to api.requesty.ai for API calls | ||
| const cleanApiUrl = apiUrl.replace(/router\.requesty/, "api.requesty").replace(/app\.requesty/, "api.requesty") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens if someone uses a completely custom domain that doesn't follow the .requesty. pattern (e.g., https://my-requesty-instance.com)? The current regex replacements might not handle this case correctly.
|
|
||
| const url = "https://router.requesty.ai/v1/models" | ||
| // Use the base URL if provided, otherwise default to the router endpoint | ||
| const apiUrl = baseUrl ? baseUrl.replace(/\/$/, "") : "https://router.requesty.ai" |
There was a problem hiding this comment.
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 document the expected format of the baseUrl parameter and what transformations are applied. This would help future maintainers understand the URL handling logic.
| <VSCodeButtonLink | ||
| href="https://app.requesty.ai/api-keys" | ||
| href={(() => { | ||
| const baseUrl = apiConfiguration?.requestyBaseUrl || "https://app.requesty.ai" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's no validation that the custom base URL is a valid URL format. Invalid URLs could cause runtime errors. Consider adding basic URL validation before using the value.
|
Addressed by the author |
Description
This PR fixes an issue where Requesty's custom base URL was not being used for all requests. When a user configured a custom base URL for Requesty, some operations (like the "Get Requesty API Key" button and model fetching) would still use the default app.requesty.ai URL instead of the custom one.
Changes
Implementation Details
The implementation handles URL transformations between different subdomains:
Testing
Notes
While the implementation is functionally complete and addresses all requirements, adding specific unit tests for the new base URL functionality would be beneficial for long-term maintainability.
Fixes #7274
Important
Fixes Requesty requests to consistently use custom base URL across all functionalities, ensuring backward compatibility.
getRequestyModels()inrequesty.tsto acceptbaseUrland adjust API endpoint accordingly.getRequestyAuthUrl()inurls.tsto use custom base URL for OAuth.RequestyHandlerinrequesty.tsto passbaseUrlwhen fetching models.webviewMessageHandlerto passbaseUrlin model fetch options.Requesty.tsxto handle custom base URL input and display.RequestyBalanceDisplay.tsxto use custom base URL for settings link.useRequestyKeyInfo.tsto fetch key info using custom base URL.This description was created by
for 9314316. You can customize this summary. It will automatically update as commits are pushed.