Add enhanced model selector with categories and thinking toggle#283
Add enhanced model selector with categories and thinking toggle#283AnthonyRonning wants to merge 1 commit intomasterfrom
Conversation
|
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. WalkthroughReplaces the flat model dropdown with a two-panel, category-driven ModelSelector (categories + Advanced all-models), adds per-model access checks and UpgradePrompt integration, syncs a new thinkingEnabled local state when selecting reasoning models, and adds Brain toggle buttons in UnifiedChat; thinkingEnabled/setThinkingEnabled exposed in local state. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI as "UnifiedChat / ModelSelector"
participant LocalState as "LocalStateContext"
participant Backend
User->>UI: Click Brain toggle
UI->>LocalState: setThinkingEnabled(true/false)
LocalState-->>UI: thinkingEnabled updated
UI->>Backend: Send message with active model + thinking flag
User->>UI: Open ModelSelector -> choose Category or Advanced model
UI->>LocalState: check access for selected model
alt access granted
UI->>LocalState: setActiveModel(model)
alt category == "reasoning"
UI->>LocalState: setThinkingEnabled(true)
end
else access denied
UI->>User: show UpgradePromptDialog
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (2)**/*.{ts,tsx,js,jsx}📄 CodeRabbit inference engine (CLAUDE.md)
Files:
**/*.{ts,tsx}📄 CodeRabbit inference engine (CLAUDE.md)
Files:
🧬 Code graph analysis (1)frontend/src/components/ModelSelector.tsx (1)
⏰ 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). (4)
Comment |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Greptile Overview
Greptile Summary
This PR implements a category-based model selector UI and adds thinking mode toggle for DeepSeek reasoning models. The implementation simplifies model selection with intuitive categories (Free, Quick, Reasoning, Math/Coding) and provides a sliding animation between category and advanced views.
Key Changes:
- Category-based model selection with icons and descriptions for better UX
- Thinking toggle (Brain icon) that switches between DeepSeek R1 (with thinking) and V3.1 Terminus (without thinking)
- State synchronization between
thinkingEnabledflag and model selection across components - Sliding menu UI with smooth transitions between category and advanced model views
- Access control checks integrated into category selection
Issues Found:
- Missing dependency
setThinkingEnabledin the useEffect dependency array atModelSelector.tsx:247- while React Context setters are stable and unlikely to cause issues, it should be included for correctness
Confidence Score: 4/5
- This PR is safe to merge with one minor fix to add the missing dependency
- The implementation is well-structured and the state synchronization logic is sound. The only issue is a missing dependency in the useEffect hook which violates React best practices but is unlikely to cause runtime issues since Context setters are stable. The UI changes are clean and the thinking toggle feature is properly implemented across both input areas.
- frontend/src/components/ModelSelector.tsx requires a minor fix to the useEffect dependency array
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| frontend/src/components/ModelSelector.tsx | 4/5 | Added category-based model selection UI with sliding menu, thinking toggle sync, and access control checks. Missing dependency in useEffect. |
| frontend/src/components/UnifiedChat.tsx | 5/5 | Added thinking toggle button that appears for DeepSeek reasoning models, switches between R1 and V3.1 Terminus. Implemented in two input areas. |
| frontend/src/state/LocalStateContext.tsx | 5/5 | Added thinkingEnabled state with default value false, getter/setter functions, and provider integration. Clean implementation. |
| frontend/src/state/LocalStateContextDef.ts | 5/5 | Added TypeScript definitions for thinkingEnabled boolean state and setThinkingEnabled function. Properly typed. |
Sequence Diagram
sequenceDiagram
participant User
participant UI as UnifiedChat UI
participant Toggle as Thinking Toggle
participant Selector as ModelSelector
participant State as LocalStateContext
Note over User,State: User Toggles Thinking Mode
User->>Toggle: Click thinking toggle
Toggle->>State: setThinkingEnabled(!current)
Toggle->>State: setModel(R1 or V3.1)
State->>State: Update thinkingEnabled state
State->>State: Update model state
State->>Selector: model change detected
Selector->>State: setThinkingEnabled(sync)
Note over Selector: useEffect syncs thinking<br/>state with model
Note over User,State: User Selects Reasoning Category
User->>Selector: Click "Reasoning" category
Selector->>State: Read thinkingEnabled
Selector->>Selector: Choose R1 or V3.1 based on state
Selector->>State: setModel(chosen model)
State->>State: Update model state
State->>Selector: model change detected
Selector->>State: setThinkingEnabled(sync)
Note over User,State: User Selects Model in Advanced View
User->>Selector: Select R1 or V3.1 directly
Selector->>State: setModel(selected)
State->>State: Update model state
State->>Selector: model change detected
Selector->>State: setThinkingEnabled(sync)
Selector->>UI: Update brain icon color
4 files reviewed, 1 comment
| useEffect(() => { | ||
| if (model === CATEGORY_MODELS.reasoning_on) { | ||
| setThinkingEnabled(true); | ||
| } else if (model === CATEGORY_MODELS.reasoning_off) { | ||
| setThinkingEnabled(false); | ||
| } | ||
| }, [model]); |
There was a problem hiding this comment.
syntax: missing setThinkingEnabled in dependency array
| useEffect(() => { | |
| if (model === CATEGORY_MODELS.reasoning_on) { | |
| setThinkingEnabled(true); | |
| } else if (model === CATEGORY_MODELS.reasoning_off) { | |
| setThinkingEnabled(false); | |
| } | |
| }, [model]); | |
| useEffect(() => { | |
| if (model === CATEGORY_MODELS.reasoning_on) { | |
| setThinkingEnabled(true); | |
| } else if (model === CATEGORY_MODELS.reasoning_off) { | |
| setThinkingEnabled(false); | |
| } | |
| }, [model, setThinkingEnabled]); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: frontend/src/components/ModelSelector.tsx
Line: 241:247
Comment:
**syntax:** missing `setThinkingEnabled` in dependency array
```suggestion
useEffect(() => {
if (model === CATEGORY_MODELS.reasoning_on) {
setThinkingEnabled(true);
} else if (model === CATEGORY_MODELS.reasoning_off) {
setThinkingEnabled(false);
}
}, [model, setThinkingEnabled]);
```
How can I resolve this? If you propose a fix, please make it concise.![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
frontend/src/components/UnifiedChat.tsx (1)
2776-2806: DRY the thinking toggle and improve accessibility
- Duplicate toggle logic appears in two places; extract to a small ThinkingToggle component or reuse a
handleThinkingTogglecallback to avoid divergence.- Add
aria-pressed={localState.thinkingEnabled}to reflect toggle state for screen readers.- <Button + <Button type="button" variant="ghost" size="sm" className="h-8 w-8 p-0" - onClick={() => { /* ... */ }} + onClick={handleThinkingToggle} + aria-pressed={localState.thinkingEnabled} aria-label={localState.thinkingEnabled ? "Disable thinking mode" : "Enable thinking mode"} >Optionally extract:
function ThinkingToggle({ visible }: { visible: boolean }) { if (!visible) return null; // render Button with Brain icon using handleThinkingToggle }frontend/src/state/LocalStateContext.tsx (2)
43-43: Persist thinkingEnabled across sessions (optional)Default false is fine, but users may expect the toggle to survive reloads. Consider reading/writing
thinkingEnabledto localStorage with try/catch.
300-303: Centralize model↔thinking sync in the provider (avoid scattered effects)Today, syncing is done in ModelSelector’s effect and here via manual toggles. To make it resilient, derive
thinkingEnabledfromsetModelwhen switching to/from the two DeepSeek models. This ensures any caller that changes the model keeps the flag in sync.Example:
// Inside setModel(...) const isR1 = aliasedModel === "deepseek-r1-0528"; const isV31 = aliasedModel === "deepseek-v31-terminus"; return { ...prev, model: aliasedModel, thinkingEnabled: isR1 ? true : isV31 ? false : prev.thinkingEnabled };This reduces hidden coupling with UI components. Based on coding guidelines.
frontend/src/components/ModelSelector.tsx (4)
39-101: Decouple MODEL_CONFIG into a shared constants moduleThis config is imported by other components (e.g., UnifiedChat). Move
MODEL_CONFIG(andgetModelTokenLimit) to a shared@/constants/models.tsto reduce cross-component coupling and circular‑import risk. Re-export here if convenient.
110-120: Strengthen typing for CATEGORY_MODELSDeclare as readonly to catch typos and enable literal inference.
-const CATEGORY_MODELS = { +const CATEGORY_MODELS = { free: "llama-3.3-70b", quick: "gpt-oss-120b", reasoning_on: "deepseek-r1-0528", reasoning_off: "deepseek-v31-terminus", math: "qwen3-coder-480b" -}; +} as const;Also applies to: 121-142
240-247: Effect deps and single source of truth for thinking sync
- Add
setThinkingEnabledto the dependency array to satisfy linting and future-proof closures.- Prefer centralizing this sync in the provider’s
setModel(see related comment in LocalStateContext.tsx) to avoid duplicate logic across components.-useEffect(() => { +useEffect(() => { if (model === CATEGORY_MODELS.reasoning_on) { setThinkingEnabled(true); } else if (model === CATEGORY_MODELS.reasoning_off) { setThinkingEnabled(false); } -}, [model]); +}, [model, setThinkingEnabled]);
289-326: Category selection: handle unavailable target models gracefullyIf the chosen target model isn’t present in
availableModels(e.g., endpoint didn’t return it), consider warning and falling back to the Free category instead of selecting a possibly unsupported ID. This avoids confusing errors at send‑time.Sketch:
const exists = availableModels.some((m) => m.id === targetModel); if (!exists) { console.warn(`Model ${targetModel} not available; falling back to Free`); setModel(CATEGORY_MODELS.free); return; } setModel(targetModel);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
frontend/src/components/ModelSelector.tsx(5 hunks)frontend/src/components/UnifiedChat.tsx(3 hunks)frontend/src/state/LocalStateContext.tsx(3 hunks)frontend/src/state/LocalStateContextDef.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx,js,jsx}: Use 2-space indentation, double quotes, and a 100-character line limit for formatting
Use camelCase for variable and function names
Use try/catch with specific error types for error handling
Files:
frontend/src/state/LocalStateContextDef.tsfrontend/src/components/UnifiedChat.tsxfrontend/src/components/ModelSelector.tsxfrontend/src/state/LocalStateContext.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use strict TypeScript typing and avoid
anywhen possible
Files:
frontend/src/state/LocalStateContextDef.tsfrontend/src/components/UnifiedChat.tsxfrontend/src/components/ModelSelector.tsxfrontend/src/state/LocalStateContext.tsx
🧬 Code graph analysis (1)
frontend/src/components/ModelSelector.tsx (3)
frontend/src/state/useLocalState.tsx (1)
useLocalState(4-10)frontend/src/components/ui/dropdown-menu.tsx (6)
DropdownMenu(170-170)DropdownMenuTrigger(171-171)DropdownMenuContent(172-172)DropdownMenuLabel(176-176)DropdownMenuSeparator(177-177)DropdownMenuItem(173-173)frontend/src/components/ui/button.tsx (1)
Button(62-62)
⏰ 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). (4)
- GitHub Check: build-linux
- GitHub Check: build-macos (universal-apple-darwin)
- GitHub Check: build-android
- GitHub Check: build-ios
🔇 Additional comments (4)
frontend/src/components/UnifiedChat.tsx (1)
38-40: No issues found with new icon importsImports look correct and consistent with usage below.
frontend/src/state/LocalStateContextDef.ts (2)
34-37: Types for thinkingEnabled look goodStrongly typed flag and setter added without using
any.
77-79: Context defaults are sensibleDefault
thinkingEnabled: falseand a no-op setter are appropriate.frontend/src/components/ModelSelector.tsx (1)
413-427: Minor: reset advanced panel state on close is correct
onOpenChangeresettingshowAdvancedprevents stale UI state.
| {/* Thinking toggle button - only visible when reasoning model is selected */} | ||
| {(localState.model === "deepseek-r1-0528" || | ||
| localState.model === "deepseek-v31-terminus") && ( | ||
| <Button | ||
| type="button" | ||
| variant="ghost" | ||
| size="sm" | ||
| className="h-8 w-8 p-0" | ||
| onClick={() => { | ||
| const newThinkingState = !localState.thinkingEnabled; | ||
| localState.setThinkingEnabled(newThinkingState); | ||
| // Switch between R1 (thinking) and V3.1 (no thinking) | ||
| localState.setModel( | ||
| newThinkingState ? "deepseek-r1-0528" : "deepseek-v31-terminus" | ||
| ); | ||
| }} | ||
| aria-label={ | ||
| localState.thinkingEnabled | ||
| ? "Disable thinking mode" | ||
| : "Enable thinking mode" | ||
| } | ||
| > | ||
| <Brain | ||
| className={`h-4 w-4 ${ | ||
| localState.thinkingEnabled | ||
| ? "text-purple-500" | ||
| : "text-muted-foreground" | ||
| }`} | ||
| /> | ||
| </Button> | ||
| )} |
There was a problem hiding this comment.
Prevent access-gating bypass when toggling thinking
This button unconditionally flips to DeepSeek R1/V3.1 even if the user’s plan lacks access. That’s inconsistent with ModelSelector’s gating and leads to 403s at send-time.
Add an access check before switching, and surface a clear prompt instead of silently swapping to a restricted model.
Example (shared handler + gating):
+ const handleThinkingToggle = useCallback(() => {
+ const newState = !localState.thinkingEnabled;
+ const targetModel = newState ? "deepseek-r1-0528" : "deepseek-v31-terminus";
+ const plan = localState.billingStatus?.product_name?.toLowerCase() || "";
+ const requiresPro = MODEL_CONFIG[targetModel]?.requiresPro || false;
+ const requiresStarter = MODEL_CONFIG[targetModel]?.requiresStarter || false;
+ const hasAccess =
+ !requiresPro && !requiresStarter
+ ? true
+ : requiresPro
+ ? plan.includes("pro") || plan.includes("max") || plan.includes("team")
+ : plan.includes("starter") || plan.includes("pro") || plan.includes("max") || plan.includes("team");
+
+ if (!hasAccess) {
+ setError("This model requires a higher plan. Open Model Selector to upgrade.");
+ return;
+ }
+ localState.setThinkingEnabled(newState);
+ localState.setModel(targetModel);
+ }, [localState, setError]);Then call handleThinkingToggle() here instead of duplicating the inline logic. Based on coding guidelines.
Committable suggestion skipped: line range outside the PR's diff.
|
@claude can we move the thinking toggle to as close to the dropdown box as possible? right now, from left to right, it seems to be ordered at the end, after the upload button and web search toggle. |
|
Claude finished @AnthonyRonning's task —— View job Moving thinking toggle closer to dropdownI've successfully moved the thinking toggle button to be positioned right after the model selector dropdown! Changes MadeReordered the toolbar buttons in both input areas (desktop and mobile views) in New order:
This makes the thinking toggle much more discoverable and logically groups it with the model selection since it controls model-specific behavior (switching between DeepSeek R1 with thinking and V3.1 Terminus without thinking). Tasks Completed
Branch: enhanced-model-selector | |
Deploying maple with
|
| Latest commit: |
23c72c3
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://095098b6.maple-ca8.pages.dev |
| Branch Preview URL: | https://enhanced-model-selector.maple-ca8.pages.dev |
There was a problem hiding this comment.
Greptile Overview
Greptile Summary
Adds category-based model selector with "Free", "Quick", "Reasoning", and "Math/Coding" options, plus an "Advanced" view for full model list. Implements thinking toggle for DeepSeek reasoning models.
Key Changes
- Simplified model selection UI with intuitive categories and icons
- Sliding menu animation between category and advanced views
- Added
thinkingEnabledstate to control reasoning output visibility - Brain icon toggle appears when DeepSeek R1 or V3.1 Terminus is selected
Issues Found
- Circular logic in thinking toggle - clicking toggle always switches between R1/V3.1, but if user manually selects a model from dropdown, toggle state becomes desynchronized with actual model
- Missing dependency in ModelSelector useEffect (already noted in previous comments)
Confidence Score: 3/5
- This PR has functional issues that affect UX but won't cause crashes
- Score reflects circular logic in thinking toggle that creates confusing user experience. The toggle always switches models, but manual model selection can desync toggle state from actual model. Additionally, missing dependency array in useEffect could cause stale closure issues.
- frontend/src/components/UnifiedChat.tsx needs logic fix for thinking toggle to avoid circular dependency
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| frontend/src/components/UnifiedChat.tsx | 3/5 | Added duplicate thinking toggle buttons (two locations) that control DeepSeek model switching. Logic has circular dependency issue where toggle always switches models. |
| frontend/src/components/ModelSelector.tsx | 4/5 | Implemented category-based model selection with sliding menu UI. Missing setThinkingEnabled in useEffect dependency array (already noted in previous comment). |
Sequence Diagram
sequenceDiagram
participant User
participant UnifiedChat
participant ModelSelector
participant LocalState
Note over User,LocalState: Category-based Model Selection
User->>ModelSelector: Click "Reasoning" category
ModelSelector->>LocalState: Check thinkingEnabled state
alt thinkingEnabled is true
ModelSelector->>LocalState: setModel("deepseek-r1-0528")
else thinkingEnabled is false
ModelSelector->>LocalState: setModel("deepseek-v31-terminus")
end
Note over User,LocalState: Thinking Toggle Interaction
User->>UnifiedChat: Click Brain icon toggle
UnifiedChat->>LocalState: newThinkingState = !thinkingEnabled
UnifiedChat->>LocalState: setThinkingEnabled(newThinkingState)
UnifiedChat->>LocalState: setModel(newThinkingState ? R1 : V3.1)
Note over User,LocalState: Model Change Sync (useEffect)
LocalState->>ModelSelector: model changed to R1 or V3.1
ModelSelector->>LocalState: Sync setThinkingEnabled(true/false)
Note over User,LocalState: Issue: Circular dependency when toggling
1 file reviewed, 1 comment
| onClick={() => { | ||
| const newThinkingState = !localState.thinkingEnabled; | ||
| localState.setThinkingEnabled(newThinkingState); | ||
| // Switch between R1 (thinking) and V3.1 (no thinking) | ||
| localState.setModel( | ||
| newThinkingState ? "deepseek-r1-0528" : "deepseek-v31-terminus" | ||
| ); | ||
| }} |
There was a problem hiding this comment.
logic: circular logic creates confusing UX - toggling thinking always changes model, but if user manually selects a model from ModelSelector, the toggle state and actual model become mismatched
for example:
- user on R1 (thinking=true) clicks toggle → switches to V3.1 (thinking=false) ✓
- user manually selects R1 from dropdown → R1 is active but thinking still shows false ✗
Prompt To Fix With AI
This is a comment left during a code review.
Path: frontend/src/components/UnifiedChat.tsx
Line: 2719:2726
Comment:
**logic:** circular logic creates confusing UX - toggling thinking always changes model, but if user manually selects a model from ModelSelector, the toggle state and actual model become mismatched
for example:
1. user on R1 (thinking=true) clicks toggle → switches to V3.1 (thinking=false) ✓
2. user manually selects R1 from dropdown → R1 is active but thinking still shows false ✗
How can I resolve this? If you propose a fix, please make it concise.
AnthonyRonning
force-pushed
the
enhanced-model-selector
branch
from
65d2a27 to
23c72c3
Compare
- Add category-based model selection (Free, Quick, Reasoning, Math/Coding) - Add thinking toggle for DeepSeek reasoning models - Implement sliding menu with advanced model view - Add icons and descriptions for better UX Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com> Move thinking toggle button next to model selector Reordered toolbar buttons so the thinking toggle (Brain icon) appears immediately after the ModelSelector dropdown instead of at the end. New order: ModelSelector → Thinking toggle → Attachments → Web search Co-authored-by: Anthony <AnthonyRonning@users.noreply.github.com> Fix infinite loop in ModelSelector useEffect The setThinkingEnabled dependency was causing an infinite re-render loop. Only the model should trigger this effect, not the setter function. Fix duplicate React keys in ModelSelector dropdown Add unique key prefixes to category menu items (category-*) and advanced submenu items (advanced-*) to prevent duplicate key warnings when both panels are rendered simultaneously. Deduplicate models in advanced submenu to fix duplicate key warnings Filter out duplicate model IDs before rendering to prevent React key conflicts when the same model appears multiple times in availableModels.
There was a problem hiding this comment.
Greptile Overview
Greptile Summary
Implements category-based model selection with a sliding menu UI and introduces a thinking toggle for DeepSeek reasoning models (R1 and V3.1 Terminus). The implementation uses R1 for "thinking enabled" mode and V3.1 for "thinking disabled" mode.
Key changes:
- Refactored ModelSelector to show category-based selection (Free, Quick, Reasoning, Math/Coding) with sliding animation to Advanced view
- Added
thinkingEnabledstate to LocalStateContext for toggling between R1 and V3.1 - Added Brain icon toggle button in UnifiedChat that switches models based on thinking state
- Maintains backward compatibility with advanced model selection
Issues found:
- State synchronization logic doesn't handle all edge cases (switching between reasoning and non-reasoning models leaves thinking state stale)
- Missing localStorage persistence for
thinkingEnabledcreates initialization mismatch with persisted model - The already-identified dependency array issue and circular logic concern from previous review remain valid
Confidence Score: 3/5
- This PR has functional logic issues that could cause UX confusion but won't break the app
- Score reflects state synchronization bugs that create inconsistent UI behavior - the thinking toggle and model state can become mismatched during initialization and when switching between model categories. While not critical runtime errors, these issues will cause user confusion. The refactoring is substantial and the category-based UI is a good improvement, but the state management needs refinement
- frontend/src/components/ModelSelector.tsx and frontend/src/state/LocalStateContext.tsx require attention for state synchronization issues
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| frontend/src/state/LocalStateContext.tsx | 4/5 | Added thinkingEnabled state management without localStorage persistence, which creates initialization mismatch with persisted model state |
| frontend/src/state/LocalStateContextDef.ts | 5/5 | Added type definitions for thinkingEnabled state - straightforward type additions |
| frontend/src/components/ModelSelector.tsx | 3/5 | Major refactor adding category-based UI with sliding menu. Contains state synchronization logic that may not handle all edge cases (switching between reasoning and non-reasoning models) |
| frontend/src/components/UnifiedChat.tsx | 4/5 | Added thinking toggle button in two locations (likely for different UI modes). Toggle logic directly couples state and model selection |
Sequence Diagram
sequenceDiagram
participant User
participant UnifiedChat
participant ModelSelector
participant LocalState
participant UseEffect
Note over User,UseEffect: Scenario 1: User clicks thinking toggle
User->>UnifiedChat: Click Brain toggle
UnifiedChat->>LocalState: setThinkingEnabled(!current)
UnifiedChat->>LocalState: setModel(newState ? R1 : V3.1)
LocalState-->>ModelSelector: model state updated
ModelSelector->>UseEffect: model changed (useEffect triggered)
UseEffect->>LocalState: setThinkingEnabled(based on model)
Note over UseEffect,LocalState: Redundant but harmless sync
Note over User,UseEffect: Scenario 2: User selects model from Advanced menu
User->>ModelSelector: Click R1 in Advanced menu
ModelSelector->>LocalState: setModel(R1)
LocalState-->>ModelSelector: model state updated
ModelSelector->>UseEffect: model changed (useEffect triggered)
UseEffect->>LocalState: setThinkingEnabled(true)
LocalState-->>UnifiedChat: thinkingEnabled updated
Note over UnifiedChat: Toggle shows correct state
Note over User,UseEffect: Scenario 3: User selects Reasoning category
User->>ModelSelector: Click Reasoning category
ModelSelector->>ModelSelector: Check current thinkingEnabled
ModelSelector->>LocalState: setModel(based on thinkingEnabled)
ModelSelector->>UseEffect: model changed (useEffect triggered)
UseEffect->>LocalState: setThinkingEnabled(based on model)
Note over User,UseEffect: Scenario 4: App initialization (BUG)
LocalState->>LocalState: Load model from localStorage (R1)
LocalState->>LocalState: Initialize thinkingEnabled=false
Note over LocalState: State mismatch!
LocalState-->>ModelSelector: Provide inconsistent state
ModelSelector->>UseEffect: Component mounts, model=R1
UseEffect->>LocalState: setThinkingEnabled(true)
Note over LocalState: Eventually synced
4 files reviewed, 2 comments
| })(), | ||
| availableModels: [llamaModel] as OpenSecretModel[], | ||
| hasWhisperModel: true, // Default to true to avoid hiding button during loading | ||
| thinkingEnabled: false, // Default to reasoning without thinking (V3.1) |
There was a problem hiding this comment.
logic: thinkingEnabled not persisted to localStorage, but model is (line 283). on app reload with R1 saved, model loads as "deepseek-r1-0528" but thinkingEnabled stays false until useEffect syncs - causes brief state mismatch
consider:
thinkingEnabled: (() => {
const savedModel = localStorage.getItem("selectedModel");
return savedModel === "deepseek-r1-0528";
})()Prompt To Fix With AI
This is a comment left during a code review.
Path: frontend/src/state/LocalStateContext.tsx
Line: 43:43
Comment:
**logic:** `thinkingEnabled` not persisted to localStorage, but `model` is (line 283). on app reload with R1 saved, `model` loads as "deepseek-r1-0528" but `thinkingEnabled` stays false until useEffect syncs - causes brief state mismatch
consider:
```typescript
thinkingEnabled: (() => {
const savedModel = localStorage.getItem("selectedModel");
return savedModel === "deepseek-r1-0528";
})()
```
How can I resolve this? If you propose a fix, please make it concise.| useEffect(() => { | ||
| if (model === CATEGORY_MODELS.reasoning_on) { | ||
| setThinkingEnabled(true); | ||
| } else if (model === CATEGORY_MODELS.reasoning_off) { | ||
| setThinkingEnabled(false); | ||
| } | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, [model]); |
There was a problem hiding this comment.
logic: useEffect doesn't reset thinkingEnabled when switching to non-reasoning models - if user goes from R1 → Llama → clicks Reasoning category, it uses stale thinking state
the check should cover all model changes:
| useEffect(() => { | |
| if (model === CATEGORY_MODELS.reasoning_on) { | |
| setThinkingEnabled(true); | |
| } else if (model === CATEGORY_MODELS.reasoning_off) { | |
| setThinkingEnabled(false); | |
| } | |
| // eslint-disable-next-line react-hooks/exhaustive-deps | |
| }, [model]); | |
| useEffect(() => { | |
| if (model === CATEGORY_MODELS.reasoning_on) { | |
| setThinkingEnabled(true); | |
| } else if (model === CATEGORY_MODELS.reasoning_off) { | |
| setThinkingEnabled(false); | |
| } | |
| // No else clause means thinkingEnabled persists across non-reasoning models | |
| }, [model, setThinkingEnabled]); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: frontend/src/components/ModelSelector.tsx
Line: 241:248
Comment:
**logic:** useEffect doesn't reset `thinkingEnabled` when switching to non-reasoning models - if user goes from R1 → Llama → clicks Reasoning category, it uses stale thinking state
the check should cover all model changes:
```suggestion
useEffect(() => {
if (model === CATEGORY_MODELS.reasoning_on) {
setThinkingEnabled(true);
} else if (model === CATEGORY_MODELS.reasoning_off) {
setThinkingEnabled(false);
}
// No else clause means thinkingEnabled persists across non-reasoning models
}, [model, setThinkingEnabled]);
```
How can I resolve this? If you propose a fix, please make it concise.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
frontend/src/components/ModelSelector.tsx (1)
240-248: Include setThinkingEnabled in dependency array.The missing
setThinkingEnableddependency was previously flagged and should be addressed. WhilesetThinkingEnabledis stable from context, React's exhaustive-deps rule requires all dependencies to be declared.Apply the suggested fix from the previous review:
useEffect(() => { if (model === CATEGORY_MODELS.reasoning_on) { setThinkingEnabled(true); } else if (model === CATEGORY_MODELS.reasoning_off) { setThinkingEnabled(false); } - // eslint-disable-next-line react-hooks/exhaustive-deps - }, [model]); + }, [model, setThinkingEnabled]);
🧹 Nitpick comments (1)
frontend/src/components/ModelSelector.tsx (1)
414-669: Consider accessibility enhancements for the sliding panel UI.The sliding panel animation works well visually, but could benefit from accessibility improvements:
- No ARIA attributes indicating which panel is currently visible
- No focus management when transitioning between main and advanced panels
- Screen readers may not announce the panel transition
Consider these enhancements:
<div className="relative"> {/* Main Category Menu */} <div className={`transition-transform duration-300 ease-in-out ${ showAdvanced ? "-translate-x-full" : "translate-x-0" }`} + aria-hidden={showAdvanced} > <DropdownMenuLabel>Select Model</DropdownMenuLabel> ... </div> {/* Advanced Submenu */} <div className={`absolute top-0 left-0 w-full max-h-[400px] flex flex-col transition-transform duration-300 ease-in-out ${ showAdvanced ? "translate-x-0" : "translate-x-full" }`} + aria-hidden={!showAdvanced} + role="menu" + aria-label="Advanced model selection" >You might also want to manage focus when the panel changes:
// Add a ref for the back button const backButtonRef = useRef<HTMLDivElement>(null); // Focus the back button when entering advanced mode useEffect(() => { if (showAdvanced && backButtonRef.current) { backButtonRef.current.focus(); } }, [showAdvanced]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
frontend/src/components/ModelSelector.tsx(5 hunks)frontend/src/components/UnifiedChat.tsx(3 hunks)frontend/src/state/LocalStateContext.tsx(3 hunks)frontend/src/state/LocalStateContextDef.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- frontend/src/state/LocalStateContextDef.ts
- frontend/src/state/LocalStateContext.tsx
- frontend/src/components/UnifiedChat.tsx
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx,js,jsx}: Use 2-space indentation, double quotes, and a 100-character line limit for formatting
Use camelCase for variable and function names
Use try/catch with specific error types for error handling
Files:
frontend/src/components/ModelSelector.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use strict TypeScript typing and avoid
anywhen possible
Files:
frontend/src/components/ModelSelector.tsx
🧬 Code graph analysis (1)
frontend/src/components/ModelSelector.tsx (1)
frontend/src/state/useLocalState.tsx (1)
useLocalState(4-10)
⏰ 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). (4)
- GitHub Check: build-android
- GitHub Check: build-linux
- GitHub Check: build-macos (universal-apple-darwin)
- GitHub Check: build-ios
🔇 Additional comments (4)
frontend/src/components/ModelSelector.tsx (4)
588-666: Advanced model list sorting looks comprehensive.The sorting implementation properly handles:
- Vision model prioritization when
chatHasImagesis true- Disabled and restricted model placement
- Proper access gating with upgrade prompts
- Conditional disabling for non-vision models in image chats
The logic is well-structured and handles edge cases appropriately.
One note: the sorting logic is quite complex (lines 597-627). If this grows further, consider extracting it into a separate helper function like
sortModelsByAvailability(models, chatHasImages, hasAccessToModel)for better testability and maintainability.
250-261: LGTM!The
getCurrentCategoryhelper correctly derives the display label based on the current model selection, with appropriate fallbacks for advanced model selections.
414-414: Good UX choice resetting showAdvanced on menu close.Resetting
showAdvancedwhen the dropdown closes ensures users always see the main category view when reopening the menu, providing a consistent experience.
290-326: Clarify the reasoning category model selection behavior.The reasoning category selection (lines 305-308) uses
thinkingEnabledto determine which model to select. While this isn't a circular dependency (the state flow is one-directional:thinkingEnabled→ model selection, then model changes syncthinkingEnabled), it does create a UX concern where selecting "Reasoning" can result in different models depending on the external toggle state. Users may expect consistent behavior when selecting the same category.Additionally, the thinking toggle is located in UnifiedChat.tsx (separate component), so positioning clarity depends on layout. Consider these alternatives to improve user clarity:
- Make reasoning always select one model (e.g., R1), with thinking toggle only controlling output format
- Split reasoning into two distinct categories: "Reasoning (with thinking)" and "Reasoning (fast)"
- Add a sub-indicator showing which reasoning variant is currently active
| const CATEGORY_MODELS = { | ||
| free: "llama-3.3-70b", | ||
| quick: "gpt-oss-120b", | ||
| reasoning_on: "deepseek-r1-0528", // R1 with thinking | ||
| reasoning_off: "deepseek-v31-terminus", // V3.1 without thinking | ||
| math: "qwen3-coder-480b" | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
Verify category models exist in availableModels.
The hardcoded model IDs in CATEGORY_MODELS are not validated against availableModels. If any of these models are unavailable from the API, category selection will fail silently or cause unexpected behavior.
Consider adding validation after fetching models:
setAvailableModels([...currentModels, ...newModels]);
+
+ // Verify category models are available
+ const missingModels = Object.entries(CATEGORY_MODELS)
+ .filter(([_, modelId]) => !availableModelsRef.current.some(m => m.id === modelId))
+ .map(([category, modelId]) => ({ category, modelId }));
+
+ if (missingModels.length > 0 && import.meta.env.DEV) {
+ console.warn("Missing category models:", missingModels);
+ }Run this script to check if these models are consistently available:
🏁 Script executed:
#!/bin/bash
# Verify CATEGORY_MODELS are present in backend model configurations
echo "Checking for category model references..."
rg -n --type=ts --type=js "deepseek-r1-0528|deepseek-v31-terminus|llama-3.3-70b|gpt-oss-120b|qwen3-coder-480b"Length of output: 1899
🏁 Script executed:
#!/bin/bash
# Search for availableModels definition and initialization in ModelSelector.tsx
echo "=== Searching for availableModels initialization and usage ==="
rg -n "availableModels\s*[:=]" frontend/src/components/ModelSelector.tsx -A 5
echo ""
echo "=== Checking for MODELS object definition ==="
rg -n "^const MODELS|^const CATEGORY_MODELS" frontend/src/components/ModelSelector.tsx -A 3 -B 1Length of output: 273
🏁 Script executed:
#!/bin/bash
# Find ModelSelector.tsx and examine availableModels usage
echo "=== Locating ModelSelector.tsx ==="
fd "ModelSelector.tsx" --type f
echo ""
echo "=== Searching for availableModels in context or state files ==="
fd -e tsx -e ts . frontend/src | head -20
echo ""
echo "=== Check LocalStateContext for model-related state ==="
fd "LocalStateContext" --type fLength of output: 1091
🏁 Script executed:
#!/bin/bash
# Examine ModelSelector.tsx structure and availableModels usage
echo "=== Check props and context usage (lines 1-160) ==="
head -160 frontend/src/components/ModelSelector.tsx | tail -80
echo ""
echo "=== Check MODELS and CATEGORY_MODELS definitions (lines 40-125) ==="
sed -n '40,125p' frontend/src/components/ModelSelector.tsx
echo ""
echo "=== Search for availableModels usage in entire file ==="
grep -n "availableModels" frontend/src/components/ModelSelector.tsx | head -15Length of output: 4786
🏁 Script executed:
#!/bin/bash
# Check LocalStateContext to see how availableModels is initialized
echo "=== Search for availableModels initialization in LocalStateContext ==="
grep -n "availableModels" frontend/src/state/LocalStateContext.tsx -B 2 -A 5 | head -50
echo ""
echo "=== Check if MODEL_CONFIG is used to initialize or populate availableModels ==="
grep -n "MODEL_CONFIG\|availableModels\|setAvailableModels" frontend/src/state/LocalStateContext.tsx | head -20
echo ""
echo "=== Check LocalStateContextDef for type definition ==="
grep -n "availableModels" frontend/src/state/LocalStateContextDef.ts -B 2 -A 2Length of output: 1836
🏁 Script executed:
#!/bin/bash
# Find where setAvailableModels is called and how it populates models
echo "=== Search for setAvailableModels calls across codebase ==="
grep -rn "setAvailableModels" frontend/src --include="*.ts" --include="*.tsx" | grep -v "^\s*//"
echo ""
echo "=== Check handleCategorySelect implementation (around line 290-326) ==="
sed -n '290,330p' frontend/src/components/ModelSelector.tsxLength of output: 1872
🏁 Script executed:
#!/bin/bash
# Find hasAccessToModel implementation
echo "=== Search for hasAccessToModel function ==="
grep -n "hasAccessToModel\s*=" frontend/src/components/ModelSelector.tsx -A 15 | head -40
echo ""
echo "=== Search setModel implementation in LocalStateContext ==="
grep -n "function setModel\|setModel:\s*(" frontend/src/state/LocalStateContext.tsx -A 5 | head -20
echo ""
echo "=== Check if MODEL_CONFIG is imported or used with availableModels filtering ==="
grep -n "MODEL_CONFIG\|availableModels.find" frontend/src/components/ModelSelector.tsx | head -15Length of output: 1905
🏁 Script executed:
#!/bin/bash
# Check where models are fetched from API and if CATEGORY_MODELS are guaranteed
echo "=== Check lines around 225 where setAvailableModels is called ==="
sed -n '200,240p' frontend/src/components/ModelSelector.tsx
echo ""
echo "=== Search for where newModels comes from ==="
sed -n '185,230p' frontend/src/components/ModelSelector.tsx | grep -A 30 "useEffect"Length of output: 1632
Add validation for CATEGORY_MODELS availability in availableModels.
The hardcoded CATEGORY_MODELS entries are not guaranteed to exist in the dynamically-populated availableModels array. If the API fetch fails or returns an incomplete model list, selecting these categories will call setModel() with a model not in availableModels, causing lookup failures downstream at line 397 and beyond.
Add a check after fetching models to warn about missing category models:
setAvailableModels([...currentModels, ...newModels]);
+
+ // Warn if category models are missing
+ const missingCategoryModels = Object.values(CATEGORY_MODELS).filter(
+ (modelId) => ![...currentModels, ...newModels].some((m) => m.id === modelId)
+ );
+ if (missingCategoryModels.length > 0 && import.meta.env.DEV) {
+ console.warn("Missing category models:", missingCategoryModels);
+ }Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In frontend/src/components/ModelSelector.tsx around lines 113 to 119, the
hardcoded CATEGORY_MODELS entries may not exist in the dynamically fetched
availableModels, which can cause downstream lookup failures; after the models
are fetched, validate each CATEGORY_MODELS value against availableModels and for
any missing model emit a clear warning (console.warn or use component state to
show a UI warning), and ensure you do not call setModel with a missing model —
fallback to a safe default (e.g., the first available model or null) or remove
that category from selection options; update the fetch handler to perform this
validation and adjust selection/fallback logic accordingly.
|
@claude something broke and the advanced submenu doesn't allow scroll anymore |
|
Claude finished @AnthonyRonning's task —— View job Fixed Advanced Submenu Scroll ✓Tasks Completed
Issue FoundThe advanced submenu container had conflicting overflow/height constraints that prevented scrolling. Fix AppliedRemoved The problem: The outer container's The solution: By removing these constraints from the outer container and letting only the inner div handle scroll behavior, the advanced menu now scrolls correctly. Verification
|
There was a problem hiding this comment.
Greptile Overview
Greptile Summary
Adds a categorized model selector UI (Free, Quick, Reasoning, Math/Coding) with a sliding menu for advanced model selection, plus a thinking toggle for DeepSeek reasoning models (R1 with thinking, V3.1 without).
Key Changes:
- Reorganized
ModelSelectorwith category-based selection and two-panel sliding UI - Added
thinkingEnabledstate toLocalStateContextto track reasoning mode - Introduced Brain icon toggle in
UnifiedChat(appears in two locations) to switch between R1 and V3.1 - Syncs model selection with thinking state via
useEffectinModelSelector
Issues Found:
- Previous comments identified critical state synchronization bugs between thinking toggle and model selection
thinkingEnablednot persisted to localStorage whilemodelis, causing brief state mismatch on reloaduseEffectdependency array hassetThinkingEnableddisabled via eslint-disable comment (line 247)- Circular logic between toggle button and model selector can cause UI state mismatches
Confidence Score: 2/5
- This PR has critical state synchronization bugs that will cause confusing UX
- Score of 2 reflects significant logic issues: circular state dependencies between thinking toggle and model selection create edge cases where UI state diverges from actual state. The
thinkingEnablednot being persisted causes reload issues, and the eslint-disabled dependency suggests awareness of problems. While the UI improvements are good, the state management bugs need resolution before merge. - Pay close attention to
frontend/src/components/UnifiedChat.tsx(circular toggle logic) andfrontend/src/state/LocalStateContext.tsx(missing localStorage persistence)
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| frontend/src/components/ModelSelector.tsx | 3/5 | Added category-based UI with sliding menu and thinking sync logic; has eslint-disabled dependency issue and potential stale state bug |
| frontend/src/components/UnifiedChat.tsx | 2/5 | Added thinking toggle buttons in two locations; circular logic causes toggle/model state mismatch when user manually selects model |
| frontend/src/state/LocalStateContext.tsx | 3/5 | Added thinkingEnabled state and setter; not persisted to localStorage causing brief mismatch on reload |
Sequence Diagram
sequenceDiagram
participant User
participant UnifiedChat
participant ModelSelector
participant LocalState
Note over User,LocalState: Category Selection Flow
User->>ModelSelector: Click "Reasoning" category
ModelSelector->>ModelSelector: Check thinkingEnabled state
alt thinkingEnabled = true
ModelSelector->>LocalState: setModel("deepseek-r1-0528")
else thinkingEnabled = false
ModelSelector->>LocalState: setModel("deepseek-v31-terminus")
end
LocalState->>ModelSelector: model updated
ModelSelector->>ModelSelector: useEffect triggers on model change
ModelSelector->>LocalState: setThinkingEnabled(true/false) based on model
Note over User,LocalState: Thinking Toggle Flow
User->>UnifiedChat: Click Brain icon toggle
UnifiedChat->>UnifiedChat: newThinkingState = !thinkingEnabled
UnifiedChat->>LocalState: setThinkingEnabled(newThinkingState)
UnifiedChat->>LocalState: setModel(R1 or V3.1)
LocalState->>ModelSelector: model updated
ModelSelector->>ModelSelector: useEffect triggers
ModelSelector->>LocalState: setThinkingEnabled (syncs state)
Note over User,LocalState: Manual Model Selection (Issue)
User->>ModelSelector: Click "Advanced" → Select R1 manually
ModelSelector->>LocalState: setModel("deepseek-r1-0528")
LocalState->>ModelSelector: model updated
ModelSelector->>ModelSelector: useEffect triggers
ModelSelector->>LocalState: setThinkingEnabled(true)
Note right of UnifiedChat: Toggle button still shows old state<br/>until re-render
1 file reviewed, no comments
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
frontend/src/components/ModelSelector.tsx (2)
219-226: Validate CATEGORY_MODELS availability after fetch; warn/fallback.Without validation, selecting a category can set an unavailable model. Add a check and warn in dev.
- // Merge with existing models (keeping the hardcoded one) - setAvailableModels([...currentModels, ...newModels]); + // Merge with existing models (keeping the hardcoded one) + const merged = [...currentModels, ...newModels]; + setAvailableModels(merged); + + // Dev warning if category models are not present + const missing = Object.values(CATEGORY_MODELS).filter( + (id) => !merged.some((m) => m.id === id) + ); + if (missing.length > 0 && import.meta.env.DEV) { + console.warn("Missing category models:", missing); + }
240-249: Two-way sync missing + dependency fix for thinking toggle.
- Add setThinkingEnabled to deps and drop the eslint-disable.
- Add a reverse effect so external brain toggle updates the active reasoning model.
- // Sync thinking toggle when model changes externally - useEffect(() => { + // Sync thinking toggle when model changes externally + useEffect(() => { if (model === CATEGORY_MODELS.reasoning_on) { setThinkingEnabled(true); } else if (model === CATEGORY_MODELS.reasoning_off) { setThinkingEnabled(false); } - // eslint-disable-next-line react-hooks/exhaustive-deps - }, [model]); + }, [model, setThinkingEnabled]);Place the reverse sync after hasAccessToModel:
+ // Keep model in sync when the brain toggle changes + useEffect(() => { + if (model === CATEGORY_MODELS.reasoning_on && !thinkingEnabled) { + if (hasAccessToModel(CATEGORY_MODELS.reasoning_off)) { + setModel(CATEGORY_MODELS.reasoning_off); + } + } else if (model === CATEGORY_MODELS.reasoning_off && thinkingEnabled) { + if (hasAccessToModel(CATEGORY_MODELS.reasoning_on)) { + setModel(CATEGORY_MODELS.reasoning_on); + } + } + }, [thinkingEnabled, model, setModel]);
🧹 Nitpick comments (4)
frontend/src/components/ModelSelector.tsx (4)
199-201: Case-insensitive filtering for “instruct”/“embed”.Avoid missing variants due to casing.
- if (model.id.includes("instruct") || model.id.includes("embed")) { + const idLc = model.id.toLowerCase(); + if (idLc.includes("instruct") || idLc.includes("embed")) { return false; }
592-596: Include unknown models in Advanced (use Coming Soon badge).You currently exclude models not in MODEL_CONFIG. Since getDisplayName handles unknowns, let them show up.
- .filter( - (m, index, self) => - MODEL_CONFIG[m.id] !== undefined && - self.findIndex((model) => model.id === m.id) === index - ) + .filter( + (m, index, self) => + self.findIndex((mm) => mm.id === m.id) === index + )
339-346: Simplify Llama detection to be case-insensitive.Small cleanup, fewer branches.
- // Llama models: no badges - if (modelId.includes("llama") || modelId.includes("Llama")) { + // Llama models: no badges + if (modelId.toLowerCase().includes("llama")) { return []; }
414-426: Place brain toggle adjacent to this button in the top bar.To meet the request, render the brain toggle immediately to the right of ModelSelector’s trigger in UnifiedChat (reorder controls), or expose an optional inline Brain toggle prop here and render it next to the Button.
I can draft a minimal patch in UnifiedChat to move the toggle right next to ModelSelector; confirm preferred approach.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/src/components/ModelSelector.tsx(5 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx,js,jsx}: Use 2-space indentation, double quotes, and a 100-character line limit for formatting
Use camelCase for variable and function names
Use try/catch with specific error types for error handling
Files:
frontend/src/components/ModelSelector.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use strict TypeScript typing and avoid
anywhen possible
Files:
frontend/src/components/ModelSelector.tsx
🧬 Code graph analysis (1)
frontend/src/components/ModelSelector.tsx (3)
frontend/src/state/useLocalState.tsx (1)
useLocalState(4-10)frontend/src/components/ui/dropdown-menu.tsx (6)
DropdownMenu(170-170)DropdownMenuTrigger(171-171)DropdownMenuContent(172-172)DropdownMenuLabel(176-176)DropdownMenuSeparator(177-177)DropdownMenuItem(173-173)frontend/src/components/ui/button.tsx (1)
Button(62-62)
⏰ 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). (4)
- GitHub Check: build-macos (universal-apple-darwin)
- GitHub Check: build-linux
- GitHub Check: build-android
- GitHub Check: build-ios
| let targetModel: string; | ||
| switch (category) { | ||
| case "free": | ||
| targetModel = CATEGORY_MODELS.free; | ||
| break; | ||
| case "quick": | ||
| targetModel = CATEGORY_MODELS.quick; | ||
| break; | ||
| case "reasoning": | ||
| targetModel = thinkingEnabled | ||
| ? CATEGORY_MODELS.reasoning_on | ||
| : CATEGORY_MODELS.reasoning_off; | ||
| break; | ||
| case "math": | ||
| targetModel = CATEGORY_MODELS.math; | ||
| break; | ||
| default: | ||
| return; | ||
| } | ||
|
|
||
| // Check access | ||
| if (!hasAccessToModel(targetModel)) { | ||
| const modelConfig = MODEL_CONFIG[targetModel]; | ||
| setSelectedModelName(modelConfig?.displayName || targetModel); | ||
| setUpgradeDialogOpen(true); | ||
| return; | ||
| } | ||
|
|
||
| setModel(targetModel); | ||
| }; |
There was a problem hiding this comment.
Guard selection when target model is unavailable.
Prevent setting a model not present in availableModels; bail early or fallback.
let targetModel: string;
switch (category) {
// ...
}
+ // Ensure target model exists locally
+ const exists = availableModelsRef.current?.some((m) => m.id === targetModel);
+ if (!exists) {
+ if (import.meta.env.DEV) console.warn("Target model not available:", targetModel);
+ return;
+ }
+
// Check access
if (!hasAccessToModel(targetModel)) {Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In frontend/src/components/ModelSelector.tsx around lines 297-326, the code
computes targetModel but does not verify it exists in availableModels before
proceeding; update the logic to first ensure targetModel is defined and present
in the availableModels list (e.g., availableModels.includes(targetModel)), and
if it is missing either bail early (return) or choose a safe fallback (like the
first available model for that category) before running the access check; then
continue with hasAccessToModel and the existing upgrade dialog behavior only for
validated targetModel.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
AnthonyRonning
force-pushed
the
enhanced-model-selector
branch
from
555061a to
23c72c3
Compare
There was a problem hiding this comment.
Greptile Overview
Greptile Summary
Adds a category-based model selector with Free, Quick, Reasoning, and Math/Coding options, plus a thinking toggle for DeepSeek reasoning models. The selector uses a sliding two-panel UI with smooth animations.
Key changes:
- Reorganized model selection from flat list to categorized menu
- Added
thinkingEnabledstate to toggle between R1 (with thinking) and V3.1 (without thinking) - Implemented sliding animation between category view and advanced "All Models" view
- Added Brain icon toggle buttons (duplicated in two toolbar locations) to control thinking mode
Issues found:
- State synchronization bugs between thinking toggle and model selection
- Missing localStorage persistence for thinking state causes reload inconsistency
- Circular logic in toggle handler creates confusing UX
- Missing dependency in useEffect hook
Confidence Score: 2/5
- Multiple state synchronization bugs will cause confusing UX and inconsistent behavior
- The PR has well-implemented UI components, but several logical bugs in state management create user-facing issues: (1) circular logic between toggle and model selector causes state mismatches, (2) missing localStorage persistence causes reload bugs, (3) incomplete thinking state reset for non-reasoning models, (4) missing dependency in useEffect. These bugs won't break the app but will cause confusing behavior.
- Pay close attention to
ModelSelector.tsxandUnifiedChat.tsxfor state synchronization logic
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| frontend/src/components/ModelSelector.tsx | 3/5 | Added category-based model selector with sliding menu UI. Has dependency array issue and doesn't reset thinking state for non-reasoning models. |
| frontend/src/components/UnifiedChat.tsx | 2/5 | Added thinking toggle buttons (duplicated in two locations). Circular logic between toggle and model selection causes UX confusion when models are manually selected. |
| frontend/src/state/LocalStateContext.tsx | 3/5 | Added thinkingEnabled state management. Missing localStorage persistence causes brief state mismatch on app reload when R1 model is saved. |
Sequence Diagram
sequenceDiagram
participant User
participant UnifiedChat
participant ModelSelector
participant LocalStateContext
participant localStorage
Note over User,localStorage: Category Selection Flow
User->>ModelSelector: Click "Reasoning" category
ModelSelector->>ModelSelector: handleCategorySelect("reasoning")
ModelSelector->>LocalStateContext: Check thinkingEnabled
alt thinkingEnabled = true
ModelSelector->>LocalStateContext: setModel("deepseek-r1-0528")
else thinkingEnabled = false
ModelSelector->>LocalStateContext: setModel("deepseek-v31-terminus")
end
LocalStateContext->>localStorage: Save model ID
ModelSelector->>ModelSelector: useEffect detects model change
ModelSelector->>LocalStateContext: setThinkingEnabled(true/false)
Note over User,localStorage: Thinking Toggle Flow
User->>UnifiedChat: Click Brain icon toggle
UnifiedChat->>LocalStateContext: Read thinkingEnabled
UnifiedChat->>LocalStateContext: setThinkingEnabled(!thinkingEnabled)
alt newThinkingState = true
UnifiedChat->>LocalStateContext: setModel("deepseek-r1-0528")
else newThinkingState = false
UnifiedChat->>LocalStateContext: setModel("deepseek-v31-terminus")
end
LocalStateContext->>localStorage: Save model ID
ModelSelector->>ModelSelector: useEffect syncs thinking state
Note over User,localStorage: App Reload Flow (Bug)
User->>UnifiedChat: Reload app
localStorage->>LocalStateContext: Load model = "deepseek-r1-0528"
Note right of LocalStateContext: thinkingEnabled = false (default)
Note right of LocalStateContext: State mismatch until useEffect runs
ModelSelector->>ModelSelector: useEffect fires
ModelSelector->>LocalStateContext: setThinkingEnabled(true)
Note right of LocalStateContext: State now synced
4 files reviewed, no comments
|
close in favor of #299 |
1 participant
Changes
UI Improvements
Technical Details
thinkingEnabledstate to LocalStateContextSummary by CodeRabbit