-
Notifications
You must be signed in to change notification settings - Fork 2.6k
Customizable headers for the OpenAI-compatible provider #3056
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
|
@ellipsis-agent please review |
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.
Important
Looks good to me! 👍
Reviewed everything up to 38aeaa48871a08596f968124afd8347d1ba988e4 in 1 minute and 37 seconds. Click for details.
- Reviewed
515lines of code in9files - Skipped
0files when reviewing. - Skipped posting
11draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. webview-ui/src/i18n/locales/en/settings.json:468
- Draft comment:
Good comprehensive localization file. The new keys for customizable headers (e.g. 'add', 'remove', 'headerName', and 'headerValue') are clear. Please double-check consistency in naming: for example, the pricing placeholder key is 'cacheWritePrice' (line 489) while the schema uses 'cacheWritesPrice'. Consistent naming will avoid confusion when integrating these values. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
2. webview-ui/src/i18n/locales/en/settings.json:8
- Draft comment:
The addition of the 'add' and 'remove' keys under 'common' is clear and consistent with other UI labels. Nice job updating these for custom header functionality. - Reason this comment was not posted:
Confidence changes required:0%<= threshold70%None
3. webview-ui/src/i18n/locales/en/settings.json:488
- Draft comment:
In the 'placeholders' > 'numbers' section, 'cacheWritePrice' is used while elsewhere 'cacheWritesPrice' is used. Consider renaming for consistency. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
4. webview-ui/src/i18n/locales/en/settings.json:152
- Draft comment:
The documentation instructions for 'googleCloudSetup' and provider routing under 'openRouter' are clear and user-friendly. Ensure consistency in punctuation and capitalization. - Reason this comment was not posted:
Confidence changes required:0%<= threshold70%None
5. src/api/providers/openai.ts:211
- Draft comment:
Typo: In the comment, "doesnt" should be "doesn't" to maintain correct English punctuation. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
6. src/exports/roo-code.d.ts:43
- Draft comment:
Typo: Consider renaming 'awspromptCacheId' to 'awsPromptCacheId' for consistency with camelCase style. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
7. src/exports/roo-code.d.ts:114
- Draft comment:
Typo: The property 'mistralCodestralUrl' appears unusual. Please verify if it should be renamed (e.g., 'mistralApiUrl' or another intended name) to maintain naming consistency. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
8. src/exports/roo-code.d.ts:300
- Draft comment:
Typo: The type 'ClineMessage' might be a misspelling. If the intended name is 'ClientMessage', please update it for clarity. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
9. src/exports/types.ts:115
- Draft comment:
The property name 'mistralCodestralUrl' at line 115 may contain a typographical error. Please double-check if it is meant to be something like 'mistralCustomUrl' or similar. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
10. src/exports/types.ts:305
- Draft comment:
The type name 'ClineMessage' on line 305 looks like it might be a typographical error. Consider renaming it to 'ClientMessage' if that is what was intended. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
11. src/schemas/index.ts:1
- Draft comment:
Typo: The word 'propgate' in the header comment should be corrected to 'propagate'. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_B2NJthDOTPxCgTUs
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
a5cd77f to
1d2c804
Compare
|
|
The pull request includes a variety of changes, such as enabling customizable headers for the OpenAI provider and updating translations in multiple locales. To make the review process more efficient, consider splitting the localization updates into a separate pull request, as they are likely independent of the core feature changes. This will help reviewers focus on the specific changes related to the OpenAI provider without being distracted by unrelated localization updates. |
6756c46 to
8d1f0f0
Compare
Key values brittle.Using a key-value store for this seems a bit... brittle? Lines ~880 reads a new value, checks for changes, destructures the old key out, spreads the rest, and adds the new key with the old value back in – all within the onInput handler, and within the template.
Weirdness when keys are unset / overlapThis isn't a huge deal, but things get weird if you unset a key then change the value, or try to add a new value, since you no longer have a key. Things also get weird if you have a key called, say, "cache" and try to add another called "cacheExpiry" — halfway through, the two will overlap on key names and one will nuke the other. Just OpenAI-compatible?Should this show up for OpenAI as well? Or just OpenAI-compatible? (I'm assuming this is just because some OAI-compatible APIs have extended/custom header options?) General notesI'll run a design review and refactor on ApiOptions.tsx once this is merged. This file is unwieldy and could be split up, namely the providers should each be their own file (or perhaps composable blocks). It also could use a Shadcn migration and a pull away from direct references to cssv ars and style tags. Again, I'll handle this. Let's get the PR in first. |
|
Ahh good catch on the overlap! Will fix |
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 using a properly typed event (e.g. React.ChangeEvent<HTMLInputElement>) instead of any in the onInput handlers. This improves type safety and adheres to best practices.
| onInput={(e: any) => handleUpdateHeaderKey(index, e.target.value)} | |
| onInput={(e: React.ChangeEvent<HTMLInputElement>) => handleUpdateHeaderKey(index, e.target.value)} |
This comment was generated because it violated a code review rule: mrule_QkEwsCio7v34DaCF.
0801398 to
e5ad879
Compare
Co-authored-by: ellipsis-dev[bot] <65095814+ellipsis-dev[bot]@users.noreply.github.com>
|
Thanks for the quick turn around Matt. It's working great. |
|
Spoke too soon I guess. I just now realized the custom headers aren't being saved. I added them and saved, but it didn't actually store them. When I go back in they are missing and they aren't taking effect it appears. |
|
Ugh, sorry about this - looking into it now. |
) * Customizable headers for the OpenAI-compatible provider * PR feedback * Fix migration * Update webview-ui/src/components/settings/ApiOptions.tsx 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>
Important
This PR adds customizable headers for the OpenAI-compatible provider, allowing users to specify additional headers for API requests, with updates to configuration, UI, and localization.
openai.tsby allowing users to specify additional headers for OpenAI API requests.getOpenAiModels()to acceptopenAiHeadersinstead ofhostHeader.openAiHeadersis always an object inContextProxy.tsfor proper serialization.openAiHeadersinProviderSettingsManager.tsto handle legacyopenAiHostHeader.ApiOptions.tsxto allow users to add, edit, and remove custom headers.openAiHostHeaderfromtypes.tsandroo-code.d.ts.This description was created by
for f9dcc36. You can customize this summary. It will automatically update as commits are pushed.