fix(web): preserve selected language model#1152
fix(web): preserve selected language model#1152slumbi wants to merge 3 commits intosourcebot-dev:mainfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughReconciles stored, in-memory, and fallback language model selection; exposes a new Changes
Sequence Diagram(s)sequenceDiagram
participant Hook as useSelectedLanguageModel
participant Local as LocalStorage
participant Toolbar as ChatBoxToolbar
participant Selector as LanguageModelSelector
Hook->>Local: read "selectedLanguageModel" (parse & validate)
Hook-->>Hook: compute availableSelectedLanguageModel (match by key)
Hook-->>Hook: determine resolvedSelectedLanguageModel (available or fallback)
Toolbar->>Hook: request current model
Hook-->>Toolbar: return resolvedSelectedLanguageModel
Toolbar->>Selector: render with resolvedSelectedLanguageModel
Selector->>Hook: onChange -> setSelectedLanguageModel(new)
Hook->>Local: persist new selectedLanguageModel
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 1
🧹 Nitpick comments (1)
packages/web/src/features/chat/components/chatBox/languageModelSelector.tsx (1)
127-134: LGTM — explicitvaluepluskeywordscorrectly addresses the cmdk collision for similar model names.Pinning
valuetogetLanguageModelKey(model)prevents cmdk from auto-deriving (and potentially deduping) values from rendered text when display names look alike, andkeywordskeeps search ergonomics by matching on display name, model id, and provider.Optional nit: when
model.displayNameis missing,""is pushed intokeywords. cmdk handles this fine, but filtering empty strings keeps the keyword set tidy.♻️ Optional cleanup
- keywords={[ - model.displayName ?? "", - model.model, - model.provider, - ]} + keywords={[ + model.displayName, + model.model, + model.provider, + ].filter((kw): kw is string => Boolean(kw))}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/web/src/features/chat/components/chatBox/languageModelSelector.tsx` around lines 127 - 134, The keywords array passed to CommandItem includes an empty string when model.displayName is missing; update the keywords construction used in the CommandItem with value={getLanguageModelKey(model)} so it filters out falsy/empty values (e.g., only include model.displayName if truthy, plus model.model and model.provider) to avoid pushing "" into keywords while preserving search behavior; locate the CommandItem in languageModelSelector.tsx and adjust the keywords assembly around model.displayName, model.model, and model.provider to exclude empty strings.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/web/src/features/chat/useSelectedLanguageModel.ts`:
- Around line 37-41: The hook now returns resolvedSelectedLanguageModel (which
always yields a model) breaking consumers' falsy checks; change the hook
(useSelectedLanguageModel) to return the raw selectedLanguageModel from
useLocalStorage instead of resolvedSelectedLanguageModel so callers (chatBox.tsx
and chatThread.tsx) can still detect undefined, and keep
availableSelectedLanguageModel/resolvedSelectedLanguageModel only for internal
fallback display logic; alternatively, if implicit fallback is desired, update
the two consumers (chatBox.tsx and chatThread.tsx) to remove/adjust their
undefined checks and handle implicit selection explicitly.
---
Nitpick comments:
In `@packages/web/src/features/chat/components/chatBox/languageModelSelector.tsx`:
- Around line 127-134: The keywords array passed to CommandItem includes an
empty string when model.displayName is missing; update the keywords construction
used in the CommandItem with value={getLanguageModelKey(model)} so it filters
out falsy/empty values (e.g., only include model.displayName if truthy, plus
model.model and model.provider) to avoid pushing "" into keywords while
preserving search behavior; locate the CommandItem in languageModelSelector.tsx
and adjust the keywords assembly around model.displayName, model.model, and
model.provider to exclude empty strings.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ad11318a-0e3f-4226-b0b4-3ab4990f97ec
📒 Files selected for processing (2)
packages/web/src/features/chat/components/chatBox/languageModelSelector.tsxpackages/web/src/features/chat/useSelectedLanguageModel.ts
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/web/src/features/chat/useSelectedLanguageModel.ts (2)
12-23: Minor: unchecked cast on parsed JSON.
JSON.parse(storedValue) as LanguageModelInfowill silently accept any object shape persisted under"selectedLanguageModel". Practically harmless here because downstream matching viagetLanguageModelKey(...)will simply not find a match for a malformed shape and the resolution falls through tofallbackLanguageModel. Still, a minimal shape check (provider/model/displayNameare strings) would make the helper more defensible if other code paths start consuming the raw result.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/web/src/features/chat/useSelectedLanguageModel.ts` around lines 12 - 23, getStoredSelectedLanguageModel currently does an unchecked cast of JSON.parse(...) to LanguageModelInfo; update it to validate the parsed object's minimal shape before returning. After JSON.parse(storedValue) inside getStoredSelectedLanguageModel, assert that the result is an object and that result.provider, result.model, and result.displayName are all strings; only then return it cast as LanguageModelInfo, otherwise return undefined (preserving the existing try/catch behavior).
45-69: The suggested simplification is unsafe and would introduce bugs.The analysis misses a critical configuration detail:
initializeWithValue: falseat line 33. This setting intentionally prevents the hook from initializing with the stored value on first render, causingselectedLanguageModelto beundefinedon mount even if localStorage contains data. This design choice necessitates thegetStoredSelectedLanguageModel()call to handle hydration mismatches and recover the stored value whenlanguageModelsupdates.You're correct that the second branch (lines 61–65) checking
currentSelectedLanguageModelis redundant — the outer guard already established that the selected model is unavailable. However, collapsing the entire setter to justsetSelectedLanguageModel(fallbackLanguageModel)would eliminate the first branch's stored value recovery, breaking the fallback logic for the hydration case.A safer improvement would be to simplify only the redundant second branch while preserving the stored value logic:
♻️ Possible simplification
- if (selectedLanguageModel && !availableSelectedLanguageModel) { - setSelectedLanguageModel((currentSelectedLanguageModel) => { - const storedSelectedLanguageModel = getStoredSelectedLanguageModel(); - const availableStoredSelectedLanguageModel = storedSelectedLanguageModel && languageModels.find( - (model) => getLanguageModelKey(model) === getLanguageModelKey(storedSelectedLanguageModel) - ); - - if (availableStoredSelectedLanguageModel) { - return availableStoredSelectedLanguageModel; - } - - if (currentSelectedLanguageModel && languageModels.find( - (model) => getLanguageModelKey(model) === getLanguageModelKey(currentSelectedLanguageModel) - )) { - return currentSelectedLanguageModel; - } - - return fallbackLanguageModel; - }); - } + if (selectedLanguageModel && !availableSelectedLanguageModel) { + setSelectedLanguageModel((currentSelectedLanguageModel) => { + const storedSelectedLanguageModel = getStoredSelectedLanguageModel(); + const availableStoredSelectedLanguageModel = storedSelectedLanguageModel && languageModels.find( + (model) => getLanguageModelKey(model) === getLanguageModelKey(storedSelectedLanguageModel) + ); + + return availableStoredSelectedLanguageModel ?? fallbackLanguageModel; + }); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/web/src/features/chat/useSelectedLanguageModel.ts` around lines 45 - 69, The current effect in useSelectedLanguageModel must retain the stored-value recovery because initializeWithValue: false means selectedLanguageModel can be undefined on mount; therefore, in the setSelectedLanguageModel callback (used when selectedLanguageModel && !availableSelectedLanguageModel), remove the redundant check against currentSelectedLanguageModel but keep the getStoredSelectedLanguageModel lookup and fallback behavior — i.e., inside the callback look up availableStoredSelectedLanguageModel via getStoredSelectedLanguageModel() and languageModels.find, return that if found, otherwise return fallbackLanguageModel; keep references to availableSelectedLanguageModel, getStoredSelectedLanguageModel, setSelectedLanguageModel, languageModels, and fallbackLanguageModel to ensure hydration recovery remains intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/web/src/features/chat/useSelectedLanguageModel.ts`:
- Around line 12-23: getStoredSelectedLanguageModel currently does an unchecked
cast of JSON.parse(...) to LanguageModelInfo; update it to validate the parsed
object's minimal shape before returning. After JSON.parse(storedValue) inside
getStoredSelectedLanguageModel, assert that the result is an object and that
result.provider, result.model, and result.displayName are all strings; only then
return it cast as LanguageModelInfo, otherwise return undefined (preserving the
existing try/catch behavior).
- Around line 45-69: The current effect in useSelectedLanguageModel must retain
the stored-value recovery because initializeWithValue: false means
selectedLanguageModel can be undefined on mount; therefore, in the
setSelectedLanguageModel callback (used when selectedLanguageModel &&
!availableSelectedLanguageModel), remove the redundant check against
currentSelectedLanguageModel but keep the getStoredSelectedLanguageModel lookup
and fallback behavior — i.e., inside the callback look up
availableStoredSelectedLanguageModel via getStoredSelectedLanguageModel() and
languageModels.find, return that if found, otherwise return
fallbackLanguageModel; keep references to availableSelectedLanguageModel,
getStoredSelectedLanguageModel, setSelectedLanguageModel, languageModels, and
fallbackLanguageModel to ensure hydration recovery remains intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cdf813bb-1b8f-4eb6-a3db-7826c5d4f370
📒 Files selected for processing (2)
packages/web/src/features/chat/components/chatBox/chatBoxToolbar.tsxpackages/web/src/features/chat/useSelectedLanguageModel.ts
1 participant
Summary
Tests
Summary by CodeRabbit
New Features
Bug Fixes