Refactor visible models settings by provider#25
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 94a16d6629
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| isVisible: boolean; | ||
| }) => void; | ||
| onSetAllVisibility: (args: { | ||
| providerName: string; |
There was a problem hiding this comment.
ProviderModelSectionProps.onSetAllVisibility currently accepts providerName: string, but the callback passed from VisibleModelsTab is setAllVisibility.mutate, whose mutation input requires providerName: ProviderName; this mismatch triggers TS2322 and breaks type-checking builds in environments that run TypeScript checks. Updating the prop signature to ProviderName (or a shared input type) keeps the refactor compile-safe and prevents invalid provider values from flowing into the mutation.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
This PR refactors the Visible Models settings UI to be organized by provider, making large model catalogs easier to navigate and manage.
Changes:
- Replaces the flat provider rendering with a per-provider
ProviderModelSectionusing a collapsible UI. - Adds per-provider sub-provider filtering and (for large lists) sub-provider search.
- Moves fetch/refresh controls and bulk show/hide actions into each provider header, and surfaces an API-key-present indicator for non-local providers.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| interface ProviderModelSectionProps { | ||
| provider: string; | ||
| providerModels: ModelConfig[]; | ||
| visibleModels: { modelId: string; isVisible: boolean }[] | undefined; | ||
| isFetchable: boolean; | ||
| isFetching: boolean; | ||
| onFetchModels: () => void; | ||
| onSetVisibility: (args: { | ||
| providerName: string; | ||
| modelId: string; | ||
| isVisible: boolean; | ||
| }) => void; | ||
| onSetAllVisibility: (args: { | ||
| providerName: string; | ||
| modelIds: string[]; | ||
| isVisible: boolean; | ||
| }) => void; | ||
| apiKeys: ApiKeys | undefined; |
There was a problem hiding this comment.
onSetAllVisibility is typed with providerName: string, but useSetAllProviderModelsVisible().mutate requires providerName: ProviderName (see ProviderVisibilityAPI). This makes onSetAllVisibility={setAllVisibility.mutate} fail type-checking (TS2322). Update the props to use ProviderName (and likely provider as ProviderName too) so the mutate function can be passed through without casts and invalid provider names can’t be passed in.
| isLocal || | ||
| canProceedWithProvider(provider, apiKeys ?? ({} as ApiKeys)).canProceed; |
There was a problem hiding this comment.
Redundant type assertion: apiKeys ?? ({} as ApiKeys) isn’t necessary because {} is assignable to ApiKeys (all fields are optional). Dropping the cast reduces noise and avoids implying that an unsafe conversion is needed.
| isLocal || | |
| canProceedWithProvider(provider, apiKeys ?? ({} as ApiKeys)).canProceed; | |
| isLocal || canProceedWithProvider(provider, apiKeys ?? {}).canProceed; |
|
/codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Summary
This change reorganizes the Visible Models settings UI around providers instead of presenting a flatter list. Each provider now renders in its own collapsible section, fetchable providers expose refresh controls inline, and the section header reflects whether the provider has an API key configured.
Within each provider section, the UI now supports sub-provider filtering and search so larger provider catalogs are easier to navigate. Bulk show and hide actions also live with the provider-specific model list, which keeps visibility management closer to the models being changed.
Validation
I checked the changed component with ESLint and TypeScript. ESLint passed for
src/ui/components/VisibleModelsTab.tsx. TypeScript currently fails withTS2322insrc/ui/components/VisibleModelsTab.tsx:394becausesetAllVisibility.mutateexpectsproviderName: ProviderNamewhile the prop type foronSetAllVisibilitystill allowsstring.Notes
The current PR captures the UI refactor as-is so it can be reviewed in your fork. The type mismatch above still needs to be fixed before this is ready to merge cleanly.