Fix language settings and add model search functionality #1396
Conversation
- Replace hardcoded i18n.activate("en") with dynamic language loading
- Add LanguageInitializer component to read user preferences from config
- Support English and Korean languages with proper fallback
- Handle loading states and error conditions gracefully
- Validates language codes to prevent invalid configurations
Resolves TODO comment in main.tsx line 35
- Add SearchableModelSelect component with real-time filtering - Apply to "Others" section where users configure custom endpoints - Also apply to "OpenRouter" section for consistency - Show dynamic model count in search placeholder - Handle empty search results gracefully - Maintain accessibility with proper ARIA attributes - Use existing UI components (Command, Popover, Button) Resolves fastrepl#1386 - Users can now search through ~100 models instead of scrolling manually
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughAdds a reusable SearchableModelSelect UI (popover + searchable Command list) and replaces two model dropdowns in the LLM settings view with it. Separately, delays app render until user language is loaded by introducing a LanguageInitializer that activates i18n from saved config with a safe fallback. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant R as Renderer
participant LI as LanguageInitializer
participant Q as React Query
participant DB as dbCommands.getConfig
participant I18N as i18n
R->>LI: Mount
LI->>Q: query config.general
Q->>DB: getConfig()
DB-->>Q: { general: { display_language } } or error
Q-->>LI: data / error
alt data available & supported
LI->>I18N: activate(lang = en|ko)
I18N-->>LI: activated
LI-->>R: render children
else error or unsupported
LI->>I18N: activate(en)
I18N-->>LI: activated
LI-->>R: render children
end
sequenceDiagram
autonumber
participant U as User
participant SMS as SearchableModelSelect
participant Pop as Popover
participant Cmd as Command List
participant Form as Form Field
U->>SMS: Click combobox
SMS->>Pop: Open
U->>Cmd: Type search
Cmd-->>U: Filtered models / "No model found"
U->>Cmd: Select model
Cmd->>SMS: onValueChange(value)
SMS->>Form: field.onChange(value)
SMS->>Pop: Close
SMS-->>U: Selected model displayed with check
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
apps/desktop/src/main.tsx (2)
46-54: Normalize BCP 47 language tags (e.g., en-US → en) before activationSome configs store full locale tags; mapping to supported base languages avoids unnecessary fallback to English.
Apply this diff inside the effect:
useEffect(() => { - const displayLanguage = config.data?.general.display_language; - if (displayLanguage && (displayLanguage === "en" || displayLanguage === "ko")) { - i18n.activate(displayLanguage); - } else { - // Fallback to English for new users, invalid languages, or if config fails to load - i18n.activate("en"); - } + const raw = config.data?.general.display_language ?? ""; + const lang = raw.split("-")[0]?.toLowerCase(); // normalize 'en-US' -> 'en' + const supported = new Set(["en", "ko"]); + i18n.activate(supported.has(lang) ? lang : "en"); }, [config.data, config.error]);
56-59: Avoid a transient blank screen during language loadReturning null may show an empty window for slow disks or first run. Consider a tiny, non-translated skeleton (e.g., a centered spinner) to signal loading without adding i18n dependencies.
apps/desktop/src/components/settings/components/ai/llm-custom-view.tsx (3)
60-104: Searchable select: add i18n for user-visible strings and minor a11y polish
- Localize "Search … models…" and "No model found." to align with the new language initialization.
- Add an aria-label to the trigger button to improve screen-reader context.
Apply this diff within the component and import t from @lingui/macro at file top:
- <Button + <Button variant="outline" role="combobox" aria-expanded={open} + aria-label={value ? `Model: ${value}` : "Select model"} className="w-full justify-between" > {value || placeholder} <ChevronsUpDown className="ml-2 h-4 w-4 shrink-0 opacity-50" /> </Button> ... - <Command> - <CommandInput placeholder={`Search ${models.length} models...`} /> - <CommandEmpty>No model found.</CommandEmpty> + <Command> + <CommandInput placeholder={t`Search ${models.length} models...`} /> + <CommandEmpty> + <Trans>No model found.</Trans> + </CommandEmpty> <CommandGroup className="max-h-64 overflow-auto">Outside the selected lines, add at the top:
import { t, Trans } from "@lingui/macro";
249-256: React Hook Form watch usage is unconventional; prefer stable watched valuesCalling customForm.watch("…") inside the dependency array re-invokes watch on each render and is harder to reason about. Use watched values in render (or useWatch) and depend on those.
Refactor like this:
- const updateDebouncedValues = useDebouncedCallback( + const updateDebouncedValues = useDebouncedCallback( (apiBase: string, apiKey: string) => { setDebouncedApiBase(apiBase); setDebouncedApiKey(apiKey); }, - [], + [], 2000, ); - // Watch for form changes - useEffect(() => { - const apiBase = customForm.watch("api_base"); - const apiKey = customForm.watch("api_key"); - - updateDebouncedValues(apiBase || "", apiKey || ""); - }, [customForm.watch("api_base"), customForm.watch("api_key"), updateDebouncedValues]); + // Watch for form changes + const apiBase = customForm.watch("api_base"); + const apiKey = customForm.watch("api_key"); + useEffect(() => { + updateDebouncedValues(apiBase || "", apiKey || ""); + }, [apiBase, apiKey, updateDebouncedValues]);Alternatively, use useWatch:
// outside effect const { control } = customForm; const [apiBase, apiKey] = useWatch({ control, name: ["api_base", "api_key"] });Also applies to: 258-265
266-321: Network query: trim debug logs and tighten response typing
- Console logs will leak into production; remove or guard behind a dev flag.
- Avoid any in the mapper; narrow the JSON to the expected shape.
- console.log("onquery"); - console.log(url.toString()); + // tip: enable only during development to inspect the resolved models endpoint + // if (import.meta.env.DEV) console.debug("[models] GET", url.toString()); ... - const data = await response.json(); - - if (!data.data || !Array.isArray(data.data)) { + const data = (await response.json()) as { data?: Array<{ id?: string }> }; + if (!Array.isArray(data.data)) { throw new Error("Invalid response format"); } - const models = data.data - .map((model: any) => model.id) + const models = data.data + .map((m) => m?.id) .filter((id: string) => { const excludeKeywords = ["dall-e", "codex", "whisper"]; return !excludeKeywords.some(keyword => id.includes(keyword)); });
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
apps/desktop/src/components/settings/components/ai/llm-custom-view.tsx(5 hunks)apps/desktop/src/main.tsx(3 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/main.tsxapps/desktop/src/components/settings/components/ai/llm-custom-view.tsx
🧬 Code graph analysis (1)
apps/desktop/src/components/settings/components/ai/llm-custom-view.tsx (2)
packages/ui/src/components/ui/button.tsx (1)
Button(37-89)packages/ui/src/lib/utils.ts (1)
cn(4-6)
⏰ 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: cubic · AI code reviewer
🔇 Additional comments (5)
apps/desktop/src/main.tsx (2)
35-45: Good addition: language bootstrapping via React Query is clean and containedFetching config once and scoping the i18n activation behind this component keeps initialization concerns out of App. Retry=1 is a sensible default here.
166-177: Composition order looks rightDeferring I18nProvider render until after activation prevents flash-of-wrong-language. Wrapping App and Toaster here keeps all UI under the initialized locale.
apps/desktop/src/components/settings/components/ai/llm-custom-view.tsx (3)
53-58: Props shape is minimal and preciseThe narrow value/placeholder/onValueChange contract keeps the component generic and reusable.
594-599: Nice integration of SearchableModelSelect for OpenRouterThis improves discoverability on long OpenRouter lists without changing form contracts.
715-721: Good UX for “Others”: searchable when discoverable, input fallback otherwiseThe conditional keeps the flow smooth for endpoints that don’t expose /models.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
| }, [config.data, config.error]); | ||
|
|
||
| // Don't render children until language is initialized | ||
| if (config.isLoading) { |
There was a problem hiding this comment.
Rendering children as soon as isLoading is false can cause a brief flash in the wrong language since i18n.activate runs in useEffect after the initial render; delay rendering until activation completes.
Prompt for AI agents
Address the following comment on apps/desktop/src/main.tsx at line 57:
<comment>Rendering children as soon as isLoading is false can cause a brief flash in the wrong language since i18n.activate runs in useEffect after the initial render; delay rendering until activation completes.</comment>
<file context>
@@ -32,8 +32,34 @@ i18n.load({
ko: koMessages,
});
-// TODO: load language from user settings
-i18n.activate("en");
+// Language initialization component
+function LanguageInitializer({ children }: { children: ReactNode }) {
+ const config = useQuery({
+ queryKey: ["config", "general"],
</file context>
apps/desktop/src/components/settings/components/ai/llm-custom-view.tsx
Outdated
Show resolved
Hide resolved
apps/desktop/src/components/settings/components/ai/llm-custom-view.tsx
Outdated
Show resolved
Hide resolved
- Fix CSS variable to use var(--radix-popper-anchor-width) - Add internationalization to 'No model found' message - Improve accessibility with aria-label on model selector button
|
Based on the reviews from @coderabbitai and @cubic-dev-ai, I have implemented the following fixes: Implemented:
I think the following suggestions are not necessary:
|
|
Tip For best results, initiate chat on the files or code changes. @SashankBhamidi Thank you for implementing those accessibility and internationalization improvements! The CSS variable fix, I appreciate your thoughtful analysis of the other suggestions. Your reasoning makes sense:
Your implementation successfully addresses the core objectives from issue #1386 (searchable model selection) and the TODO for dynamic language loading. The changes are clean and maintain good UX without unnecessary complexity. Would you like me to create follow-up issues for any of the declined suggestions that might have value for future improvements, or are you satisfied with the current scope and implementation? |
|
Thanks @coderabbitai! Satisfied with current scope, no follow-up issues needed. |
|
Tip For best results, initiate chat on the files or code changes. @SashankBhamidi Perfect! You've done excellent work on this PR. The implementation is clean, addresses the core requirements effectively, and maintains good focus on the intended scope. Enjoy the improved model search functionality and dynamic language loading! 🐰 ( .. ) |
Resolved conflicts in apps/desktop/src/main.tsx
|
@yujonglee Hey, this PR has been open for over a month now. Just merged it with the latest main and resolved conflicts. Would appreciate a review when you get a chance. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
apps/desktop/src/main.tsx (1)
46-62: Gate rendering until i18n activation completes
Even with the loading guard, the first committed frame afterconfig.isLoadingflips tofalsestill renders using whatever locale was active before the query (typically English). Becausei18n.activateruns inuseEffect, the UI can momentarily flash in the wrong language before the activation-driven re-render kicks in. Please wait to render children until after you’ve runi18n.activatefor the resolved locale (e.g., set alanguageReadyflag in the same effect or switch touseLayoutEffectand gate on that state).function LanguageInitializer({ children }: { children: ReactNode }) { const config = useQuery({ @@ - // Don't render children until language is initialized - if (config.isLoading) { + const [languageReady, setLanguageReady] = useState(false); + + useEffect(() => { + if (config.isLoading) { + return; + } + const displayLanguage = config.data?.general.display_language; + const next = displayLanguage === "ko" ? "ko" : "en"; + if (i18n.locale !== next) { + i18n.activate(next); + } + setLanguageReady(true); + }, [config.isLoading, config.data, config.error]); + + if (!languageReady) { return null; }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/desktop/src/components/settings/components/ai/llm-custom-view.tsx(5 hunks)apps/desktop/src/main.tsx(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{js,ts,tsx,rs}
⚙️ CodeRabbit configuration file
**/*.{js,ts,tsx,rs}: 1. Do not add any error handling. Keep the existing one.
2. No unused imports, variables, or functions.
3. For comments, keep it minimal. It should be about "Why", not "What".
Files:
apps/desktop/src/main.tsxapps/desktop/src/components/settings/components/ai/llm-custom-view.tsx
🧬 Code graph analysis (1)
apps/desktop/src/components/settings/components/ai/llm-custom-view.tsx (2)
packages/ui/src/components/ui/button.tsx (1)
Button(37-89)packages/ui/src/lib/utils.ts (1)
cn(4-6)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/desktop/src/components/settings/components/ai/llm-custom-view.tsx (1)
79-79: Consider localizing the placeholder with variable interpolation.The placeholder text uses dynamic interpolation but isn't localized. For consistency with the broader i18n effort in this PR, consider using Lingui's
tmacro with variable placeholders.Example using the
tmacro with interpolation:+import { t } from "@lingui/core/macro"; + function SearchableModelSelect({ models, value, placeholder, onValueChange }: SearchableModelSelectProps) { const [open, setOpen] = useState(false); + const { i18n } = useLingui(); return ( <Popover open={open} onOpenChange={setOpen}> ... <PopoverContent className="w-[var(--radix-popper-anchor-width)] p-0"> <Command> - <CommandInput placeholder={`Search ${models.length} models...`} /> + <CommandInput placeholder={i18n._(t`Search ${models.length} models...`)} />Note: You'll also need to import
useLinguifrom@lingui/react/macro.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/desktop/src/components/settings/components/ai/llm-custom-view.tsx(5 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{js,ts,tsx,rs}
⚙️ CodeRabbit configuration file
**/*.{js,ts,tsx,rs}: 1. Do not add any error handling. Keep the existing one.
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/components/ai/llm-custom-view.tsx
🧬 Code graph analysis (1)
apps/desktop/src/components/settings/components/ai/llm-custom-view.tsx (4)
packages/ui/src/components/ui/popover.tsx (3)
Popover(85-85)PopoverTrigger(85-85)PopoverContent(85-85)packages/ui/src/components/ui/button.tsx (1)
Button(37-89)packages/ui/src/components/ui/command.tsx (5)
Command(200-200)CommandInput(204-204)CommandEmpty(202-202)CommandGroup(203-203)CommandItem(205-205)packages/ui/src/lib/utils.ts (1)
cn(4-6)
🔇 Additional comments (4)
apps/desktop/src/components/settings/components/ai/llm-custom-view.tsx (4)
6-22: LGTM!All imports are properly used, and the
Transimport from@lingui/react/macrofollows the correct Lingui v5 macro split convention. The previous review feedback has been addressed.
53-105: Well-structured component with proper accessibility.The
SearchableModelSelectcomponent correctly implements the searchable dropdown pattern using existing UI primitives. Previous review feedback has been properly addressed:
- CSS variable corrected to
var(--radix-popper-anchor-width)<Trans>wrapper added for "No model found"aria-labeladded to the button for screen readers
595-600: LGTM!The
SearchableModelSelectis correctly integrated into the OpenRouter form field with proper props and event handlers.
714-719: LGTM!The
SearchableModelSelectis correctly integrated into the Others form field, properly handling the dynamically fetched model list with appropriate props.
|
This PR conflicts with #1537 which removed i18n 2 days ago. My language initialization feature re-introduces it. Should I keep only the model search feature and drop language settings? |
Changes
i18n.activate("en")with dynamic loading from user configFixes
main.tsx:35- load language from user settingsImplementation
LanguageInitializercomponent readsconfig.general.display_languagewith fallback to EnglishSearchableModelSelectcomponent with real-time filtering for "Others" and "OpenRouter" sectionsSummary by cubic
Load app language from user settings and add search to model dropdowns. Improves localization and makes choosing from 100+ models fast.