Skip to content
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

Support custom providers #530

Merged
merged 9 commits into from
Apr 3, 2024
Merged

Support custom providers #530

merged 9 commits into from
Apr 3, 2024

Conversation

kliu57
Copy link
Collaborator

@kliu57 kliu57 commented Mar 27, 2024

Closes #503
Closes #440

Users are now able to add custom providers and the user settings modal now shows a table of providers.

image

Implemented functionalities:

  • Changed the display of the providers section to include a user-friendly and responsive table

Add Provider button:

  • Add custom providers by clicking "Add", typing info, clicking "Save" button
  • Users are allowed to add any provider that we currently support (openai, openrouter, and any future ones we implement - excluding FreeModelProvider) as well as any custom provider that is open ai compatible.
  • If the provider is not compatible, key or url is invalid, provider will not get added and user will see an error message.
  • Allowing the users to create multiples of the same provider (i.e. openai) since they may want to manage multiple keys.
  • Not allowing users to create FreeModelProvider since that does not have a key so you wouldn't need multiples of it.

Other buttons/fields:

  • Delete custom providers using "Delete" button (only custom ones can be deleted)
  • Change the current provider using "Set as Current Provider" button
  • Change the Api Key any of provider in the list by editing the Api Key field, automatically key validation will occur (error message will display if the key is invalid)

Code changes (only summary of important points):

PreferencesModal.tsx (core code is here)

  • focusedProvider state var tracks which provider row we are editing. This is what allows us to display the "Key is invalid" message on the correct corresponding row.
  • selectedProvider state var tracks which provider we have clicked the checkbox on
  • newCustomProvider stat var stores the data user typed in the new provider row

SmallRevealablePasswordInput.tsx
created this so I don't mess with the styles of RevealablePasswordInput.tsx since we still use that in Instructions page and we may bring back Instructions page someday


use-alert.ts

  • added two more types of toast alert in here since the existing types don't disappear fast enough for my liking

Had to make changes to OpenAiProvider.ts and OpenRouterProvider.ts
to take in a name param so that users will be able to have multiple Open AI or Open Router providers with different names


Had to add defaultModel member var to ChatCraftProvider class
to store the defaultModel, since for the custom providers users will add themselves we have no way of hardcoding the defaultModel. So we will query the first model from list of models and store it in this ChatCraftProvider member var.


Follow ups issues:

#524
#542
#548
#550
#551
#552
#553


How to test:

Currently I don't have any custom provider urls/keys to use for testing. If anyone has any please help me to test if those providers are open ai compatible.

You may add new providers using the providers we currently support:

https://api.openai.com/v1 (you can create multiple with diff names if you want to manage more than one api key)
https://openrouter.ai/api/v1 (you can create multiple with diff names if you want to manage more than one api key)
https://free-chatcraft-ai.deno.dev/api/v1 (not allowing them to create one since there is no api key so you would not need multiples)

@kliu57
Copy link
Collaborator Author

kliu57 commented Mar 27, 2024

Notes from team meeting:

  • when we create new provider, should validate the key, if not valid don't let it be created
  • just show the domain of the api url, when they hover over it show the whole url
  • In last column show a checkmark icon to specify which is the current provider in use
  • To change the current provider, have the user check the box on the left of the row and click "Set as Current Provider" button
  • If the user selects more than one provider and tries to set as default show an error message (N/A -> I decided to only allow users to check one box at a time)
  • If the user selects one or more providers that is our built in providers (ex: OpenAI) and tries to delete them then show an error message (N/A -> I decided to only allow users to check one box at a time)
  • Remove the GPT Model dropdown from the user settings (moved to follow up issue Update Settings model option to be per provider #524)
  • For now, for custom providers, grab all the models and choose the first one as the default model (done, but have a question which I will post below)

Future PRs:

  • Perhaps move this to a new modal
  • Perhaps add a column that allows the user to pick their "default model"
  • Perhaps have a way for us to store in localStorage the current in use model for each provider (for example if we are using gpt4vision with OpenAI, we switch to openRouter and switch back, we want to be back on gpt4vision) (right now we will always snap back to the default model since we do not store the current model for a specific provider anywhere)

@kliu57 kliu57 added this to the Release 1.7 milestone Mar 27, 2024
@kliu57 kliu57 self-assigned this Mar 27, 2024
Copy link

cloudflare-workers-and-pages bot commented Mar 30, 2024

Deploying chatcraft-org with  Cloudflare Pages  Cloudflare Pages

Latest commit: 6f05011
Status: ✅  Deploy successful!
Preview URL: https://a503f11c.console-overthinker-dev.pages.dev
Branch Preview URL: https://issue-503.console-overthinker-dev.pages.dev

View logs

@kliu57
Copy link
Collaborator Author

kliu57 commented Mar 30, 2024

@humphd I've completed this. Code could use some refactoring but I've implemented everything. I have a question about models though. I console logged all the models I got when I called queryModels on openAI and I got a list of 28 models. But in chatcraft production we already have displayed only 16 models. What is causing this difference? Are we filtering out the models somewhere, where and why?

[update] Question answered during team meeting - we are filtering out the models for openai deliberately

@kliu57 kliu57 marked this pull request as ready for review March 30, 2024 07:13
@mingming-ma mingming-ma self-requested a review March 30, 2024 14:02
src/hooks/use-alert.ts Outdated Show resolved Hide resolved
Copy link
Collaborator

@Amnish04 Amnish04 left a comment

Choose a reason for hiding this comment

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

@kliu57 This is a great improvement. Just left some comments/suggestions

src/components/SmallRevealablePasswordInput.tsx Outdated Show resolved Hide resolved
src/hooks/use-alert.ts Outdated Show resolved Hide resolved
src/lib/ChatCraftProvider.ts Outdated Show resolved Hide resolved
src/lib/providers/DefaultProvider/FreeModelProvider.ts Outdated Show resolved Hide resolved
src/components/PreferencesModal.tsx Show resolved Hide resolved
src/components/PreferencesModal.tsx Show resolved Hide resolved
src/components/PreferencesModal.tsx Show resolved Hide resolved
src/components/PreferencesModal.tsx Show resolved Hide resolved
src/components/PreferencesModal.tsx Outdated Show resolved Hide resolved
@kliu57
Copy link
Collaborator Author

kliu57 commented Mar 30, 2024

@Amnish04 made all the requested changes minus the ones you still need input from dave.

Copy link
Collaborator

@mingming-ma mingming-ma left a comment

Choose a reason for hiding this comment

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

I tested and it handles different situations very well!

image
I think click the Get key button to open a new tab is better. Just my thought and could be implemented in the follow up PR.

Update: Sorry that not notice current version can return back from the tab, so there is no need to change.

@Amnish04
Copy link
Collaborator

@kliu57 Another thing I noticed is that when you click on Add Provider and a new form appears

image

Closing the modal and returning does not make that go away. Maybe you should implement and call a resetProvidersForm function when the modal is closed

@mingming-ma mingming-ma removed this from the Release 1.7 milestone Mar 30, 2024
@mingming-ma mingming-ma added this to the Release 1.8 milestone Mar 30, 2024
@tarasglek
Copy link
Owner

tarasglek commented Mar 31, 2024

Katie, thank you for working on this.

Sorry to complain but I find this new ux really confusing. I don't think we need to show all the details when we aren't editing an item. Eg a simple list like

- [x] OpenAI
- [x] Provider1 | URL | Key
- [ ] Provider2

Might be easier to comprehend..Eg should only show details once we click on a provider.

Here is another example of a similar multi-network concept in crypo-land:
image

@humphd
Copy link
Collaborator

humphd commented Mar 31, 2024

This is me telling @kliu57 to do it as a table. Sorry we didn't have more clarity before you began, Katie.

Is it possible for us to do UI changes in a follow-up so that we can land the rest of the work in here? As far as I can tell, what you want is cosmetic.

@tarasglek
Copy link
Owner

Yeah always happy to land and iterate. lgtm for now :)

@kliu57
Copy link
Collaborator Author

kliu57 commented Apr 3, 2024

@kliu57 Another thing I noticed is that when you click on Add Provider and a new form appears

image

Closing the modal and returning does not make that go away. Maybe you should implement and call a resetProvidersForm function when the modal is closed

This is now fixed

@kliu57
Copy link
Collaborator Author

kliu57 commented Apr 3, 2024

@Amnish04 @mingming-ma Please review

Changes made:

  • Refactored password component into one -> PasswordInput.tsx (Removed SmallRevealablePasswordInput.tsx and RevealablePasswordInput.tsx)
  • If you add a new row for new provider and don't fill it out, when you close the modal and come back you will not see that new row
  • all alert toasts are now truncated to 200 chars
  • Get key from OpenRouter button renamed to Get OpenRouter key

Copy link
Collaborator

@mingming-ma mingming-ma left a comment

Choose a reason for hiding this comment

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

LGTM!


const paddingRight = size === "sm" ? "2.5rem" : "4.5rem";
const paddingLeft = size === "sm" ? "0.4rem" : undefined;
const inputSize = size === "sm" ? "sm" : "md";
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think we need this. We can use the size prop passed as is

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I created a follow up issue that we can use to discuss during next week's meeting.

#553

Comment on lines 4 to 5
type PasswordInputProps = ComponentPropsWithRef<typeof Input> & {
size?: "sm" | "md";
Copy link
Collaborator

Choose a reason for hiding this comment

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

InputProps already have size, we shouldn't override it.

Suggested change
type PasswordInputProps = ComponentPropsWithRef<typeof Input> & {
size?: "sm" | "md";
type PasswordInputProps =InputProps & {

Make sure to import InputProps from "@chakra-ui/react"

const paddingRight = size === "sm" ? "2.5rem" : "4.5rem";
const paddingLeft = size === "sm" ? "0.4rem" : undefined;
const inputSize = size === "sm" ? "sm" : "md";
const buttonSize = size === "sm" ? "xs" : "sm";
Copy link
Collaborator

Choose a reason for hiding this comment

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

You can rather add hideButtonSize prop to your props interface

Comment on lines 12 to 13
const paddingRight = size === "sm" ? "2.5rem" : "4.5rem";
const paddingLeft = size === "sm" ? "0.4rem" : undefined;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same here, the user of this component can pass these props explicitly and you can pass to Input directly.

@kliu57 kliu57 requested a review from Amnish04 April 3, 2024 19:38
Copy link
Collaborator

@Amnish04 Amnish04 left a comment

Choose a reason for hiding this comment

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

Looks good to me!

We can discuss #553 after landing this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Custom AI Completion Endpoint Support custom providers
5 participants