Conversation
Contributor
📝 WalkthroughWalkthroughThe changes introduce a new React Query hook to dynamically fetch available models from a user-defined LLM endpoint, update related UI logic to use this new data source, and adjust how API key mutations are handled. Additionally, translation source references in English and Korean locale files are updated to reflect source code line changes. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
apps/desktop/src/components/settings/views/ai.tsx (1)
324-324: Remove console.log statement before production.This debugging statement should be removed as it adds unnecessary noise to the browser console and is not needed in production code.
- console.log("available models being loaded");apps/desktop/src/components/settings/components/ai/llm-custom-view.tsx (1)
611-639: Improve the model selection UI logicThe conditional rendering logic for the model selection works but could be more robust:
Consider adding error state handling and improving the loading experience:
<FormControl> {othersModels.isLoading && !field.value ? ( <div className="py-1 text-sm text-neutral-500"> <Trans>Loading available models...</Trans> </div> ) + : othersModels.isError + ? ( + <Input + {...field} + placeholder="Enter model name (unable to fetch models)" + /> + ) : othersModels.data && othersModels.data.length > 0 ? ( <Select value={field.value} onValueChange={field.onChange} > <SelectTrigger> <SelectValue placeholder="Select model" /> </SelectTrigger> <SelectContent> {othersModels.data.map((model) => ( <SelectItem key={model} value={model}> {model} </SelectItem> ))} </SelectContent> </Select> ) : ( <Input {...field} placeholder="Enter model name (endpoint has no discoverable models)" /> )} </FormControl>
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/desktop/src/components/settings/components/ai/llm-custom-view.tsx(5 hunks)apps/desktop/src/components/settings/views/ai.tsx(2 hunks)apps/desktop/src/locales/en/messages.po(16 hunks)apps/desktop/src/locales/ko/messages.po(16 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{js,ts,tsx,rs}
⚙️ CodeRabbit Configuration File
**/*.{js,ts,tsx,rs}: 1. No error handling.
2. No unused imports, variables, or functions.
3. For comments, keep it minimal. It should be about "Why", not "What".
Files:
apps/desktop/src/components/settings/views/ai.tsxapps/desktop/src/components/settings/components/ai/llm-custom-view.tsx
⏰ 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). (2)
- GitHub Check: ci (macos, macos-latest)
- GitHub Check: ci (windows, windows-latest)
🔇 Additional comments (4)
apps/desktop/src/components/settings/views/ai.tsx (1)
570-570: Good change for consistent API key handling.This ensures the mutation is always called with a string value, which provides better consistency and supports the new dynamic model fetching approach.
apps/desktop/src/locales/en/messages.po (1)
259-1561: Translation source references updated correctly.These line number updates maintain accurate mappings between translation strings and their source code locations after the recent refactoring. This is standard maintenance required by translation tooling.
apps/desktop/src/locales/ko/messages.po (1)
259-1453: LGTM - Automatic localization reference updatesThese are automatic updates to source code line number references in the Korean localization file, reflecting changes made to the TypeScript source files. No translation strings or logic have been modified - this is standard maintenance for keeping localization references accurate.
apps/desktop/src/components/settings/components/ai/llm-custom-view.tsx (1)
16-16: LGTM - React Query importThe useQuery import is correctly added for the new dynamic model fetching functionality.
Comment on lines
+147
to
+206
| // temporary fix for fetching models smoothly | ||
| const othersModels = useQuery({ | ||
| queryKey: ["others-direct-models", customForm.watch("api_base"), customForm.watch("api_key")?.slice(0, 8)], | ||
| queryFn: async (): Promise<string[]> => { | ||
| const apiBase = customForm.getValues("api_base"); | ||
| const apiKey = customForm.getValues("api_key"); | ||
|
|
||
| const url = new URL(apiBase); | ||
| url.pathname = "/v1/models"; | ||
|
|
||
| const headers: Record<string, string> = { | ||
| "Content-Type": "application/json", | ||
| }; | ||
|
|
||
| if (apiKey && apiKey.trim().length > 0) { | ||
| headers["Authorization"] = `Bearer ${apiKey}`; | ||
| } | ||
|
|
||
| const response = await fetch(url.toString(), { | ||
| method: "GET", | ||
| headers, | ||
| }); | ||
|
|
||
| if (!response.ok) { | ||
| throw new Error(`HTTP ${response.status}: ${response.statusText}`); | ||
| } | ||
|
|
||
| const data = await response.json(); | ||
|
|
||
| if (!data.data || !Array.isArray(data.data)) { | ||
| throw new Error("Invalid response format"); | ||
| } | ||
|
|
||
| const models = data.data | ||
| .map((model: any) => model.id) | ||
| .filter((id: string) => { | ||
| const excludeKeywords = ["dall-e"]; | ||
| return !excludeKeywords.some(keyword => id.includes(keyword)); | ||
| }); | ||
|
|
||
| return models; | ||
| }, | ||
| enabled: (() => { | ||
| const apiBase = customForm.watch("api_base"); | ||
| const apiKey = customForm.watch("api_key"); | ||
| const isLocal = apiBase?.includes("localhost") || apiBase?.includes("127.0.0.1"); | ||
|
|
||
| try { | ||
| // Only enable if URL looks complete (ends with common patterns) | ||
| const validEndings = ["/v1", "/v1/", ":11434/v1", ":8080/v1"]; | ||
| const looksComplete = validEndings.some(ending => apiBase?.endsWith(ending)); | ||
|
|
||
| return Boolean(apiBase && new URL(apiBase) && looksComplete && (isLocal || apiKey)); | ||
| } catch { | ||
| return false; | ||
| } | ||
| })(), | ||
| retry: 1, | ||
| refetchInterval: false, | ||
| }); |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion
Review the query implementation for potential improvements
The othersModels query implementation looks functional but has several areas that could be improved:
- Error handling: The query throws errors but doesn't provide user-friendly error states in the UI
- Security: The fetch request uses user-provided URLs without additional validation beyond basic URL parsing
- Performance: The query key includes a slice of the API key which could cause unnecessary re-fetches
Consider these improvements:
const othersModels = useQuery({
- queryKey: ["others-direct-models", customForm.watch("api_base"), customForm.watch("api_key")?.slice(0, 8)],
+ queryKey: ["others-direct-models", customForm.watch("api_base"), Boolean(customForm.watch("api_key"))],
queryFn: async (): Promise<string[]> => {
const apiBase = customForm.getValues("api_base");
const apiKey = customForm.getValues("api_key");
+ // Additional URL validation
+ const url = new URL(apiBase);
+ if (!['http:', 'https:'].includes(url.protocol)) {
+ throw new Error('Only HTTP and HTTPS protocols are allowed');
+ }
- const url = new URL(apiBase);
url.pathname = "/v1/models";
// ... rest of implementation
},
enabled: (() => {
// ... existing logic
})(),
retry: 1,
refetchInterval: false,
+ staleTime: 5 * 60 * 1000, // Cache for 5 minutes
});📝 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.
Suggested change
| // temporary fix for fetching models smoothly | |
| const othersModels = useQuery({ | |
| queryKey: ["others-direct-models", customForm.watch("api_base"), customForm.watch("api_key")?.slice(0, 8)], | |
| queryFn: async (): Promise<string[]> => { | |
| const apiBase = customForm.getValues("api_base"); | |
| const apiKey = customForm.getValues("api_key"); | |
| const url = new URL(apiBase); | |
| url.pathname = "/v1/models"; | |
| const headers: Record<string, string> = { | |
| "Content-Type": "application/json", | |
| }; | |
| if (apiKey && apiKey.trim().length > 0) { | |
| headers["Authorization"] = `Bearer ${apiKey}`; | |
| } | |
| const response = await fetch(url.toString(), { | |
| method: "GET", | |
| headers, | |
| }); | |
| if (!response.ok) { | |
| throw new Error(`HTTP ${response.status}: ${response.statusText}`); | |
| } | |
| const data = await response.json(); | |
| if (!data.data || !Array.isArray(data.data)) { | |
| throw new Error("Invalid response format"); | |
| } | |
| const models = data.data | |
| .map((model: any) => model.id) | |
| .filter((id: string) => { | |
| const excludeKeywords = ["dall-e"]; | |
| return !excludeKeywords.some(keyword => id.includes(keyword)); | |
| }); | |
| return models; | |
| }, | |
| enabled: (() => { | |
| const apiBase = customForm.watch("api_base"); | |
| const apiKey = customForm.watch("api_key"); | |
| const isLocal = apiBase?.includes("localhost") || apiBase?.includes("127.0.0.1"); | |
| try { | |
| // Only enable if URL looks complete (ends with common patterns) | |
| const validEndings = ["/v1", "/v1/", ":11434/v1", ":8080/v1"]; | |
| const looksComplete = validEndings.some(ending => apiBase?.endsWith(ending)); | |
| return Boolean(apiBase && new URL(apiBase) && looksComplete && (isLocal || apiKey)); | |
| } catch { | |
| return false; | |
| } | |
| })(), | |
| retry: 1, | |
| refetchInterval: false, | |
| }); | |
| // temporary fix for fetching models smoothly | |
| const othersModels = useQuery({ | |
| queryKey: [ | |
| "others-direct-models", | |
| customForm.watch("api_base"), | |
| Boolean(customForm.watch("api_key")) | |
| ], | |
| queryFn: async (): Promise<string[]> => { | |
| const apiBase = customForm.getValues("api_base"); | |
| const apiKey = customForm.getValues("api_key"); | |
| // Additional URL validation | |
| const url = new URL(apiBase); | |
| if (!['http:', 'https:'].includes(url.protocol)) { | |
| throw new Error('Only HTTP and HTTPS protocols are allowed'); | |
| } | |
| url.pathname = "/v1/models"; | |
| const headers: Record<string, string> = { | |
| "Content-Type": "application/json", | |
| }; | |
| if (apiKey && apiKey.trim().length > 0) { | |
| headers["Authorization"] = `Bearer ${apiKey}`; | |
| } | |
| const response = await fetch(url.toString(), { | |
| method: "GET", | |
| headers, | |
| }); | |
| if (!response.ok) { | |
| throw new Error(`HTTP ${response.status}: ${response.statusText}`); | |
| } | |
| const data = await response.json(); | |
| if (!data.data || !Array.isArray(data.data)) { | |
| throw new Error("Invalid response format"); | |
| } | |
| const models = data.data | |
| .map((model: any) => model.id) | |
| .filter((id: string) => { | |
| const excludeKeywords = ["dall-e"]; | |
| return !excludeKeywords.some(keyword => id.includes(keyword)); | |
| }); | |
| return models; | |
| }, | |
| enabled: (() => { | |
| const apiBase = customForm.watch("api_base"); | |
| const apiKey = customForm.watch("api_key"); | |
| const isLocal = | |
| apiBase?.includes("localhost") || apiBase?.includes("127.0.0.1"); | |
| try { | |
| const validEndings = ["/v1", "/v1/", ":11434/v1", ":8080/v1"]; | |
| const looksComplete = validEndings.some((ending) => | |
| apiBase?.endsWith(ending) | |
| ); | |
| return Boolean( | |
| apiBase && new URL(apiBase) && looksComplete && (isLocal || apiKey) | |
| ); | |
| } catch { | |
| return false; | |
| } | |
| })(), | |
| retry: 1, | |
| refetchInterval: false, | |
| staleTime: 5 * 60 * 1000, // Cache for 5 minutes | |
| }); |
🤖 Prompt for AI Agents
In apps/desktop/src/components/settings/components/ai/llm-custom-view.tsx around
lines 147 to 206, improve the othersModels query by removing the API key slice
from the queryKey to prevent unnecessary refetches and enhance security. Add
more robust validation for the apiBase URL to restrict it to allowed domains or
patterns before making the fetch call. Implement user-friendly error handling by
catching errors in the queryFn and setting an error state that can be displayed
in the UI instead of throwing raw errors. These changes will improve security,
performance, and user experience.
This was referenced Jul 31, 2025
Merged
Merged
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.