feat: polish provider settings and picker behavior#48
Conversation
There was a problem hiding this comment.
Code Review
This pull request refactors the AI Providers settings page to introduce a sidebar with active accounts and inactive placeholders, adds a delete confirmation modal, and enhances the provider addition flow. In the chat interface, it implements automatic scrolling for the skill picker to keep the highlighted item in view. Feedback focuses on improving the maintainability of provider authentication logic and simplifying the scrolling implementation by using the native scrollIntoView API.
| const authModes: ProviderAccount['authMode'][] = vendor?.supportedAuthModes.length | ||
| ? vendor.supportedAuthModes | ||
| : vendorId === 'ollama' | ||
| ? ['local'] | ||
| : vendorId === 'openai' || vendorId === 'google' | ||
| ? ['oauth_browser', 'api_key'] | ||
| : typeInfo?.isOAuth | ||
| ? (typeInfo.supportsApiKey ? ['oauth_device', 'api_key'] : ['oauth_device']) | ||
| : ['api_key']; |
There was a problem hiding this comment.
This logic for determining authModes includes hardcoded fallbacks for specific providers. This could become a maintenance issue if provider definitions change or new ones are added. To improve maintainability, it would be better to rely on a single source of truth for provider capabilities, such as the vendor.supportedAuthModes property. If vendor can be undefined, perhaps we should ensure it's always populated from a consolidated provider registry.
| useEffect(() => { | ||
| if (!skillPickerVisible || filteredSlashItems.length === 0) { | ||
| return; | ||
| } | ||
|
|
||
| const container = skillPickerListRef.current; | ||
| const boundedHighlightedIndex = Math.min(highlightedSkillIndex, filteredSlashItems.length - 1); | ||
| const selectedItem = skillPickerItemRefs.current[boundedHighlightedIndex]; | ||
|
|
||
| if (!container || !selectedItem) { | ||
| return; | ||
| } | ||
|
|
||
| scrollElementIntoScrollableView(container, selectedItem); | ||
| }, [filteredSlashItems.length, highlightedSkillIndex, skillPickerVisible]); |
There was a problem hiding this comment.
This useEffect and the helper function scrollElementIntoScrollableView can be simplified by using the standard Element.scrollIntoView() method. This is more declarative and removes the need for manual scroll calculations and the skillPickerListRef.
Here's a suggested simplification for this useEffect block. You can then remove the scrollElementIntoScrollableView function and the skillPickerListRef.
useEffect(() => {
if (!skillPickerVisible || filteredSlashItems.length === 0) {
return;
}
const boundedHighlightedIndex = Math.min(highlightedSkillIndex, filteredSlashItems.length - 1);
const selectedItem = skillPickerItemRefs.current[boundedHighlightedIndex];
if (selectedItem) {
selectedItem.scrollIntoView({ block: 'nearest' });
}
}, [filteredSlashItems.length, highlightedSkillIndex, skillPickerVisible]);
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request updates AI provider metadata, including names and icons, and significantly refactors the provider settings UI to display both active accounts and inactive placeholders. It also introduces a deletion confirmation dialog and improves the chat input skill picker by ensuring highlighted items scroll into view. Feedback suggests refactoring a hardcoded name map in the OAuth manager to utilize central provider definitions, which would improve maintainability and prevent data duplication.
| const nameMap: Record<OAuthProviderType, string> = { | ||
| 'minimax-portal': 'MiniMax (Global)', | ||
| 'minimax-portal-cn': 'MiniMax (CN)', | ||
| 'minimax-portal-cn': 'MiniMax', |
There was a problem hiding this comment.
While this change correctly aligns the provider name, this nameMap duplicates information that is already present in PROVIDER_DEFINITIONS. To improve maintainability and avoid potential inconsistencies, consider refactoring to remove this map. You could instead retrieve the provider name directly from the definitions using getProviderDefinition from ./provider-registry when constructing nextAccount.
Summary:
Validation: