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

Added logic for sorting the retry options #344

Merged
merged 6 commits into from
Jan 18, 2024
Merged

Added logic for sorting the retry options #344

merged 6 commits into from
Jan 18, 2024

Conversation

Rachit1313
Copy link
Collaborator

Fixes #316

@tarasglek
Copy link
Owner

The bottom menu needs sorting too.

image

Need to unify this sorting code somewhere. Maybe do it at the point of requesting list models endpoint?

@tarasglek
Copy link
Owner

Thanks for breaking this up into separate pull req

Copy link
Collaborator

@humphd humphd left a comment

Choose a reason for hiding this comment

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

Let's fix this at the source, so it works for every user of models, which we get from the useModels() hook:

If you change the way we do the sort there, it should mean we're good in all UI situations.

Copy link

cloudflare-workers-and-pages bot commented Jan 17, 2024

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: 4e0dc79
Status: ✅  Deploy successful!
Preview URL: https://241eb0a6.console-overthinker-dev.pages.dev
Branch Preview URL: https://sorting.console-overthinker-dev.pages.dev

View logs

@@ -54,7 +54,8 @@ export const ModelsProvider: FC<{ children: ReactNode }> = ({ children }) => {
isFetching.current = true;
try {
const models = await queryModels(apiKey).then((models) => {
models.sort();
// models.sort();
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 delete vs. comment-out code

@tarasglek
Copy link
Owner

image

Still not sorting bottom menu

@humphd
Copy link
Collaborator

humphd commented Jan 17, 2024

Given that we ultimately want the pretty names to be sorted, let's do that in the hook (i.e., convert the models to ChatCraftModel instances, then sort on the prettyName) so that all consumers will have this list in the right order.

@@ -54,8 +54,7 @@ export const ModelsProvider: FC<{ children: ReactNode }> = ({ children }) => {
isFetching.current = true;
try {
const models = await queryModels(apiKey).then((models) => {
// models.sort();
models.sort((a, b) => a.localeCompare(b));
models.sort();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Don't bother sorting here if you're going to sort again later.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

removed

@@ -74,7 +73,7 @@ export const ModelsProvider: FC<{ children: ReactNode }> = ({ children }) => {
}, [settings.apiUrl, settings.apiKey]);

const value = {
models: models || defaultModels,
models: models?.sort((a, b) => a.prettyModel.localeCompare(b.prettyModel)) || defaultModels,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do this above on line 60 so you cache the result in state vs. doing it for every consumer:

models.sort((a, b) => a.prettyModel.localeCompare(b.prettyModel))
setModels(models);

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done thanks.

@Rachit1313 Rachit1313 self-assigned this Jan 17, 2024
Copy link
Collaborator

@humphd humphd left a comment

Choose a reason for hiding this comment

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

Code change looks good to me. Get some of your colleagues to test the change at the UI level and/or put in some screenshots of it doing what you expect.

Copy link
Owner

@tarasglek tarasglek left a comment

Choose a reason for hiding this comment

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

good to land. this will be very helpful

@Rachit1313
Copy link
Collaborator Author

Code change looks good to me. Get some of your colleagues to test the change at the UI level and/or put in some screenshots of it doing what you expect.

Screenshot 2024-01-18 at 12 27 22 PM Screenshot 2024-01-18 at 12 27 29 PM

@Rachit1313 Rachit1313 merged commit 880373e into main Jan 18, 2024
4 checks passed
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.

sort models, move stuff to "retry with" a submenu
3 participants