Skip to content

Fix model picker#1202

Merged
cte merged 1 commit intomainfrom
cte/fix-model-picker
Feb 26, 2025
Merged

Fix model picker#1202
cte merged 1 commit intomainfrom
cte/fix-model-picker

Conversation

@cte
Copy link
Copy Markdown
Collaborator

@cte cte commented Feb 26, 2025

Description

Several fixes:

  • Properly load unbound models when selecting it as a provider.
  • Don't flash an error when you first load the settings page.
  • Don't highlight the model picker with a red border if the error isn't related to selecting a model.
  • Use tailwind in more places.
  • Move the "Thinking Budget" right below the model picker and move it into its own component.
  • Properly set the default model in the model picker but allow it to be cleared out.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

Checklist:

  • My code follows the patterns of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation

Additional context

Related Issues

Reviewers


Important

Fixes and enhances the model picker functionality, UI consistency, and error handling in the settings interface.

  • Behavior:
    • Properly load unbound models when selected as a provider in ClineProvider and ApiOptions.
    • Prevent error flash on initial settings page load in SettingsView.
    • Avoid red border highlight on model picker if error is unrelated to model selection in ApiOptions.
    • Set default model in model picker but allow clearing in ModelPicker.
  • UI/UX:
    • Use Tailwind CSS in more components for consistent styling.
    • Move "Thinking Budget" below model picker and into its own component ThinkingBudget.
  • Code Structure:
    • Refactor ApiErrorMessage to be more concise and reusable.
    • Simplify Dialog and AlertDialog components for better maintainability.
  • Validation:
    • Update validateApiConfiguration and validateModelId to include requesty provider checks.

This description was created by Ellipsis for 41e75bc. It will automatically update as commits are pushed.

@cte cte requested a review from mrubens as a code owner February 26, 2025 01:21
@changeset-bot
Copy link
Copy Markdown

changeset-bot bot commented Feb 26, 2025

🦋 Changeset detected

Latest commit: 41e75bc

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
roo-cline Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@dosubot dosubot bot added size:L This PR changes 100-499 lines, ignoring generated files. bug Something isn't working labels Feb 26, 2025
@cte cte force-pushed the cte/fix-model-picker branch from 275cf28 to 241be7f Compare February 26, 2025 04:32
@dosubot dosubot bot added size:XL This PR changes 500-999 lines, ignoring generated files. and removed size:L This PR changes 100-499 lines, ignoring generated files. labels Feb 26, 2025
@cte cte force-pushed the cte/fix-model-picker branch from 88883ea to 2e93a2e Compare February 26, 2025 06:28
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Repeated error messages (e.g. 'You must provide a valid API key.') appear for several providers. Consider extracting these strings into constants or a helper so that updates and translations are centralized.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Intentionally leaving these console.log in (but commented out) for a future refactor.

@dosubot dosubot bot added lgtm This PR has been approved by a maintainer size:XXL This PR changes 1000+ lines, ignoring generated files. and removed size:XL This PR changes 500-999 lines, ignoring generated files. labels Feb 26, 2025
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The class 'focus:outline-hidden' appears to be a typo. It is likely meant to be 'focus:outline-none' to properly remove the outline on focus.

Suggested change
<DialogPrimitive.Close className="ring-offset-background focus:ring-ring data-[state=open]:bg-accent data-[state=open]:text-muted-foreground absolute top-4 right-4 rounded-xs opacity-70 transition-opacity hover:opacity-100 focus:ring-2 focus:ring-offset-2 focus:outline-hidden disabled:pointer-events-none [&_svg]:pointer-events-none [&_svg]:shrink-0 [&_svg:not([class*='size-'])]:size-4">
<DialogPrimitive.Close className="ring-offset-background focus:ring-ring data-[state=open]:bg-accent data-[state=open]:text-muted-foreground absolute top-4 right-4 rounded-xs opacity-70 transition-opacity hover:opacity-100 focus:ring-2 focus:ring-offset-2 focus:outline-none disabled:pointer-events-none [&_svg]:pointer-events-none [&_svg]:shrink-0 [&_svg:not([class*='size-'])]:size-4">

@cte cte force-pushed the cte/fix-model-picker branch from 124e2c1 to 41e75bc Compare February 26, 2025 07:02
)}
{...props}>
{children}
<DialogPrimitive.Close className="ring-offset-background focus:ring-ring data-[state=open]:bg-accent data-[state=open]:text-muted-foreground absolute top-4 right-4 rounded-xs opacity-70 transition-opacity hover:opacity-100 focus:ring-2 focus:ring-offset-2 focus:outline-hidden disabled:pointer-events-none [&_svg]:pointer-events-none [&_svg]:shrink-0 [&_svg:not([class*='size-'])]:size-4">
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Typographical Issue: The class name 'focus:outline-hidden' on line 48 appears to be a typo. It likely should be 'focus:outline-none' to correctly remove the focus outline. Please update this class accordingly.

Suggested change
<DialogPrimitive.Close className="ring-offset-background focus:ring-ring data-[state=open]:bg-accent data-[state=open]:text-muted-foreground absolute top-4 right-4 rounded-xs opacity-70 transition-opacity hover:opacity-100 focus:ring-2 focus:ring-offset-2 focus:outline-hidden disabled:pointer-events-none [&_svg]:pointer-events-none [&_svg]:shrink-0 [&_svg:not([class*='size-'])]:size-4">
<DialogPrimitive.Close className="ring-offset-background focus:ring-ring data-[state=open]:bg-accent data-[state=open]:text-muted-foreground absolute top-4 right-4 rounded-xs opacity-70 transition-opacity hover:opacity-100 focus:ring-2 focus:ring-offset-2 focus:outline-none disabled:pointer-events-none [&_svg]:pointer-events-none [&_svg]:shrink-0 [&_svg:not([class*='size-'])]:size-4">

@cte cte merged commit bdc96a0 into main Feb 26, 2025
8 of 9 checks passed
@cte cte deleted the cte/fix-model-picker branch February 26, 2025 07:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working lgtm This PR has been approved by a maintainer size:XXL This PR changes 1000+ lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants