-
Notifications
You must be signed in to change notification settings - Fork 601
feat(settings): add inline rename support for custom providers #1217
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
Conversation
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
📝 WalkthroughWalkthroughThis PR adds inline editing functionality to provider names in the ModelProviderSettings component. Users can now edit provider names directly in both enabled and disabled sections, with support for saving on blur/Enter and canceling on Escape, while maintaining existing display and drag-and-drop interactions. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/renderer/settings/components/ModelProviderSettings.vue (2)
236-238: Consider explicit typing for better type safety.The
editInputRefcould benefit from explicit typing for improved IDE support and type checking.🔎 Suggested typing improvement
const editingProviderId = ref<string | null>(null) const editingName = ref('') -const editInputRef = ref<HTMLInputElement | null>(null) +const editInputRef = ref<HTMLInputElement>()
61-81: Consider extracting the editing UI to reduce duplication.This editing UI block (input field, pencil icon, and display logic) is duplicated between enabled and disabled provider sections (here and lines 123-143). Extracting it into a reusable component or template ref would improve maintainability.
💡 Suggested approach
Consider extracting the provider name display and editing logic into a separate component that can be reused in both sections:
<!-- ProviderNameEditor.vue --> <template> <input v-if="isEditing" ref="inputRef" v-model="localName" class="text-sm font-medium flex-1 min-w-0 bg-background border border-input rounded px-2 py-0.5 outline-none focus:ring-1 focus:ring-ring" :dir="dir" @blur="$emit('save', localName)" @keydown="handleKeydown" @click.stop /> <template v-else> <span class="text-sm font-medium flex-1" :dir="dir">{{ displayName }}</span> <Icon v-if="showEditIcon" icon="lucide:pencil" class="w-3.5 h-3.5 text-muted-foreground opacity-0 group-hover:opacity-60 hover:opacity-100! cursor-pointer shrink-0" @click="$emit('startEdit', $event)" /> </template> </template>Then use it in both sections with appropriate props and event handlers.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
resources/model-db/providers.jsonsrc/renderer/settings/components/ModelProviderSettings.vue
🧰 Additional context used
📓 Path-based instructions (9)
**/*.{ts,tsx,js,jsx,vue}
📄 CodeRabbit inference engine (CLAUDE.md)
Use English for logs and comments (Chinese text exists in legacy code, but new code should use English)
Files:
src/renderer/settings/components/ModelProviderSettings.vue
**/*.vue
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.vue: Use Vue 3 Composition API for all components instead of Options API
Use Tailwind CSS with scoped styles for component styling
Files:
src/renderer/settings/components/ModelProviderSettings.vue
src/renderer/**/*.vue
📄 CodeRabbit inference engine (CLAUDE.md)
src/renderer/**/*.vue: All user-facing strings must use i18n keys via vue-i18n for internationalization
Ensure proper error handling and loading states in all UI components
Implement responsive design using Tailwind CSS utilities for all UI components
src/renderer/**/*.vue: Use composition API and declarative programming patterns; avoid options API
Structure files: exported component, composables, helpers, static content, types
Use PascalCase for component names (e.g., AuthWizard.vue)
Use Vue 3 with TypeScript, leveraging defineComponent and PropType
Use template syntax for declarative rendering
Use Shadcn Vue, Radix Vue, and Tailwind for components and styling
Implement responsive design with Tailwind CSS; use a mobile-first approach
Use Suspense for asynchronous components
Use <script setup> syntax for concise component definitions
Prefer 'lucide:' icon family as the primary choice for Iconify icons
Import Icon component from '@iconify/vue' and use with lucide icons following pattern '{collection}:{icon-name}'
Files:
src/renderer/settings/components/ModelProviderSettings.vue
src/**/*
📄 CodeRabbit inference engine (.cursor/rules/project-structure.mdc)
New features should be developed in the
srcdirectory
Files:
src/renderer/settings/components/ModelProviderSettings.vue
src/renderer/**/*.{vue,js,ts}
📄 CodeRabbit inference engine (.cursor/rules/project-structure.mdc)
Renderer process code should be placed in
src/renderer(Vue 3 application)
Files:
src/renderer/settings/components/ModelProviderSettings.vue
src/renderer/**/*.{ts,tsx,vue}
📄 CodeRabbit inference engine (.cursor/rules/vue-shadcn.mdc)
src/renderer/**/*.{ts,tsx,vue}: Write concise, technical TypeScript code with accurate examples
Use descriptive variable names with auxiliary verbs (e.g., isLoading, hasError)
Avoid enums; use const objects instead
Use arrow functions for methods and computed properties
Avoid unnecessary curly braces in conditionals; use concise syntax for simple statementsVue 3 app code in
src/renderer/srcshould be organized intocomponents/,stores/,views/,i18n/,lib/directories with shell UI insrc/renderer/shell/
Files:
src/renderer/settings/components/ModelProviderSettings.vue
src/renderer/**
📄 CodeRabbit inference engine (.cursor/rules/vue-shadcn.mdc)
Use lowercase with dashes for directories (e.g., components/auth-wizard)
Files:
src/renderer/settings/components/ModelProviderSettings.vue
src/renderer/**/*.{ts,vue}
📄 CodeRabbit inference engine (.cursor/rules/vue-shadcn.mdc)
src/renderer/**/*.{ts,vue}: Use useFetch and useAsyncData for data fetching
Leverage ref, reactive, and computed for reactive state management
Use provide/inject for dependency injection when appropriate
Use Iconify/Vue for icon implementation
Files:
src/renderer/settings/components/ModelProviderSettings.vue
src/**/*.{ts,tsx,vue,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use Prettier with single quotes, no semicolons, and 100 character width
Files:
src/renderer/settings/components/ModelProviderSettings.vue
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-check (x64)
🔇 Additional comments (1)
src/renderer/settings/components/ModelProviderSettings.vue (1)
261-272: LGTM!The cancel and keyboard handling logic is clean and follows expected UX patterns.
| const saveEditingName = async () => { | ||
| if (!editingProviderId.value || !editingName.value.trim()) { | ||
| cancelEditingName() | ||
| return | ||
| } | ||
| const trimmedName = editingName.value.trim() | ||
| const providerId = editingProviderId.value | ||
| editingProviderId.value = null | ||
| await providerStore.updateProviderConfig(providerId, { name: trimmedName }) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add error handling for the async update operation.
The function clears the editing state before the async updateProviderConfig call completes. If the update fails, the user won't receive feedback, and the state will be inconsistent. Additionally, the function doesn't check if the name actually changed, potentially causing unnecessary updates.
🔎 Recommended improvements
const saveEditingName = async () => {
if (!editingProviderId.value || !editingName.value.trim()) {
cancelEditingName()
return
}
+
const trimmedName = editingName.value.trim()
const providerId = editingProviderId.value
+
+ // Find the provider to check if name changed
+ const provider = providerStore.providers.find(p => p.id === providerId)
+ if (provider && provider.name === trimmedName) {
+ // Name unchanged, just cancel editing
+ cancelEditingName()
+ return
+ }
+
- editingProviderId.value = null
- await providerStore.updateProviderConfig(providerId, { name: trimmedName })
+ try {
+ await providerStore.updateProviderConfig(providerId, { name: trimmedName })
+ editingProviderId.value = null
+ } catch (error) {
+ console.error('Failed to update provider name:', error)
+ // Optionally show user feedback here
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const saveEditingName = async () => { | |
| if (!editingProviderId.value || !editingName.value.trim()) { | |
| cancelEditingName() | |
| return | |
| } | |
| const trimmedName = editingName.value.trim() | |
| const providerId = editingProviderId.value | |
| editingProviderId.value = null | |
| await providerStore.updateProviderConfig(providerId, { name: trimmedName }) | |
| } | |
| const saveEditingName = async () => { | |
| if (!editingProviderId.value || !editingName.value.trim()) { | |
| cancelEditingName() | |
| return | |
| } | |
| const trimmedName = editingName.value.trim() | |
| const providerId = editingProviderId.value | |
| // Find the provider to check if name changed | |
| const provider = providerStore.providers.find(p => p.id === providerId) | |
| if (provider && provider.name === trimmedName) { | |
| // Name unchanged, just cancel editing | |
| cancelEditingName() | |
| return | |
| } | |
| try { | |
| await providerStore.updateProviderConfig(providerId, { name: trimmedName }) | |
| editingProviderId.value = null | |
| } catch (error) { | |
| console.error('Failed to update provider name:', error) | |
| // Optionally show user feedback here | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/renderer/settings/components/ModelProviderSettings.vue around lines 250
to 259, the saveEditingName function clears editing state before awaiting
providerStore.updateProviderConfig and does not check whether the name actually
changed or handle update errors; change the flow to: compare trimmedName to the
current provider name and return early if unchanged, set a "saving" flag (or
disable inputs), then await updateProviderConfig inside try/catch, only clear
editingProviderId/editingName on success, and on error restore previous editing
state and show user-visible feedback (toast/alert/inline error). Ensure the
saving flag is cleared in finally so UI is re-enabled even on failure.
add inline rename support for custom providers
CleanShot.2025-12-24.at.17.44.31.mp4
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.