-
Notifications
You must be signed in to change notification settings - Fork 2.6k
Adding requesty base url #6992
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
Adding requesty base url #6992
Conversation
adding locales fix fix fix
Co-authored-by: ellipsis-dev[bot] <65095814+ellipsis-dev[bot]@users.noreply.github.com>
Co-authored-by: ellipsis-dev[bot] <65095814+ellipsis-dev[bot]@users.noreply.github.com>
Co-authored-by: ellipsis-dev[bot] <65095814+ellipsis-dev[bot]@users.noreply.github.com>
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.
Thank you for your contribution! I've reviewed the changes and the implementation looks solid overall. The feature to add custom base URLs for the Requesty provider is well-implemented and follows existing patterns. I have a few suggestions that could improve the user experience and code robustness.
| <VSCodeTextField | ||
| value={apiConfiguration?.requestyBaseUrl || ""} | ||
| type="text" | ||
| onInput={handleInputChange("requestyBaseUrl")} |
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 URL validation for the custom base URL input. Currently, any string is accepted which could lead to runtime errors if an invalid URL is provided. You could add validation similar to other providers:
| onChange={(e: any) => { | ||
| const isChecked = e.target.checked === true | ||
| if (!isChecked) { | ||
| setApiConfigurationField("requestyBaseUrl", undefined) |
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.
Is it intentional that unchecking the checkbox immediately clears the base URL value? Users might accidentally uncheck it and lose their custom URL. Consider preserving the value and only clearing it when explicitly deleted by the user, or perhaps add a confirmation dialog?
| "User-Agent": `RooCode/${Package.version}`, | ||
| }, | ||
| }) | ||
| }) |
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.
While the test verifies that the custom base URL is passed to the OpenAI constructor, it would be valuable to add a test that actually makes an API call with the custom URL to ensure end-to-end functionality. This could help catch any issues with how the custom URL is used in practice.
| value={apiConfiguration?.requestyBaseUrl || ""} | ||
| type="text" | ||
| onInput={handleInputChange("requestyBaseUrl")} | ||
| placeholder={t("settings:providers.getRequestyBaseUrl")} |
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 showing the default URL as a placeholder to guide users. Instead of just the translation key, you could show:
This would help users understand the expected format.
| <VSCodeTextField | ||
| value={apiConfiguration?.requestyBaseUrl || ""} | ||
| type="text" | ||
| onInput={handleInputChange("requestyBaseUrl")} |
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 URL validation for the custom base URL input. Currently, any string is accepted which could lead to runtime errors if an invalid URL is provided. You could add validation similar to other providers to ensure the URL is valid before saving.
| onChange={(e: any) => { | ||
| const isChecked = e.target.checked === true | ||
| if (!isChecked) { | ||
| setApiConfigurationField("requestyBaseUrl", undefined) |
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.
Is it intentional that unchecking the checkbox immediately clears the base URL value? Users might accidentally uncheck it and lose their custom URL. Consider preserving the value and only clearing it when explicitly deleted by the user, or perhaps add a confirmation dialog?
| "User-Agent": `RooCode/${Package.version}`, | ||
| }, | ||
| }) | ||
| }) |
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.
While the test verifies that the custom base URL is passed to the OpenAI constructor, it would be valuable to add a test that actually makes an API call with the custom URL to ensure end-to-end functionality. This could help catch any issues with how the custom URL is used in practice.
| value={apiConfiguration?.requestyBaseUrl || ""} | ||
| type="text" | ||
| onInput={handleInputChange("requestyBaseUrl")} | ||
| placeholder={t("settings:providers.getRequestyBaseUrl")} |
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 showing the default URL as a placeholder to guide users. Instead of just the translation key, you could show the default URL (https://router.requesty.ai/v1) to help users understand the expected format.
#6982 + translations fix
Important
Adds optional
requestyBaseUrltorequestySchemaand updatesRequestyHandlerto use it, with UI and translation updates for custom base URL input.requestyBaseUrltorequestySchemainprovider-settings.tsas an optional string.RequestyHandlerinrequesty.tsto userequestyBaseUrlif provided, defaulting to "https://router.requesty.ai/v1".requesty.spec.tsto verify custom base URL usage.Requesty.tsxto include a checkbox and text field for custom base URL input.settings.json).This description was created by
for c06c491. You can customize this summary. It will automatically update as commits are pushed.