Add embedding model setting #100
Conversation
Related to #99 --- For more details, open the [Copilot Workspace session](https://copilot-workspace.githubnext.com/Royal-lobster/Syncia/issues/99?shareId=XXXX-XXXX-XXXX-XXXX).
|
|
|
WalkthroughThe pull request introduces an enhancement to the chat settings by adding support for selecting an embedding model. This modification spans across three files: Changes
Sequence DiagramsequenceDiagram
participant User
participant ChatSettings
participant Settings
participant VectorStore
User->>ChatSettings: Select Embedding Model
ChatSettings->>Settings: Update embeddingModel
Settings-->>VectorStore: Provide Selected Model
VectorStore->>VectorStore: Initialize with Model
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/components/Settings/Sections/ChatSettings.tsx (1)
Line range hint
1-188: Consider improving settings management architectureSuggestions for better architecture:
- Create a centralized settings validation layer
- Use TypeScript enums or constants for valid model types
- Separate chat and embedding model management
- Add error boundaries for settings-related failures
This will improve type safety and maintainability.
Example implementation:
// types/models.ts export enum EmbeddingModel { Ada002 = 'text-embedding-ada-002', // Add other models } // validation/settings.ts export function validateSettings(settings: Settings): ValidationResult { return { isValid: true, errors: [], // Add validation logic } } // hooks/useEmbeddingModels.ts export function useEmbeddingModels() { return { models: Object.values(EmbeddingModel).map(id => ({ id, name: formatModelName(id) })), // Add other embedding-specific logic } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/components/Settings/Sections/ChatSettings.tsx(1 hunks)src/config/settings/index.ts(2 hunks)src/lib/getMatchedContent.ts(2 hunks)
🔇 Additional comments (1)
src/config/settings/index.ts (1)
27-27: LGTM! Type definition and default value are consistent
The embeddingModel field follows the same pattern as other similar fields in the settings, maintaining type consistency.
Also applies to: 46-46
| @@ -3,6 +3,7 @@ import { RecursiveCharacterTextSplitter } from 'langchain/text_splitter' | |||
| import { MemoryVectorStore } from 'langchain/vectorstores/memory' | |||
| import { createSHA256Hash } from './createSHA256Hash' | |||
| import { readStorage, setStorage } from '../hooks/useStorage' | |||
| import { useSettings } from '../hooks/useSettings' | |||
There was a problem hiding this comment.
Critical: Invalid React hooks usage in non-React context
The useSettings hook is being used incorrectly:
- React hooks can only be used inside React components
- Hooks cannot be called inside regular functions like
getContextVectorStore
This will cause runtime errors.
Consider refactoring to pass the embedding model as a parameter:
-import { useSettings } from '../hooks/useSettings'
export const getMatchedContent = async (
query: string,
context: string,
apiKey: string,
baseURL: string,
+ embeddingModel?: string,
) => {
- const vectorStore = await getContextVectorStore(context, apiKey, baseURL)
+ const vectorStore = await getContextVectorStore(context, apiKey, baseURL, embeddingModel)
const retriever = vectorStore.asRetriever()
const relevantDocs = await retriever.getRelevantDocuments(query)
return relevantDocs.map((doc) => doc.pageContent).join('\n')
}
const getContextVectorStore = async (
context: string,
apiKey: string,
baseURL: string,
+ embeddingModel?: string,
) => {
- const [settings] = useSettings()
- const embeddingModel = settings.chat.embeddingModel || 'text-embedding-ada-002'
+ const modelName = embeddingModel || 'text-embedding-ada-002'
const embeddings = new OpenAIEmbeddings({
openAIApiKey: apiKey,
- modelName: embeddingModel,
+ modelName,
configuration: {
baseURL: baseURL,
},
})Also applies to: 25-26, 29-29
| <FieldWrapper | ||
| title="Embedding Model" | ||
| description="Choose between available embedding models" | ||
| row={true} | ||
| > | ||
| <select | ||
| value={chatSettings.embeddingModel || ''} | ||
| className="input cdx-w-44" | ||
| onChange={(e) => { | ||
| setSettings({ | ||
| ...settings, | ||
| chat: { | ||
| ...chatSettings, | ||
| embeddingModel: e.target.value, | ||
| }, | ||
| }) | ||
| }} | ||
| > | ||
| {models.map((model) => ( | ||
| <option key={model.id} value={model.id}> | ||
| {model.id} | ||
| </option> | ||
| ))} | ||
| </select> | ||
| </FieldWrapper> |
There was a problem hiding this comment.
Critical: Incorrect reuse of chat models for embeddings
The implementation has several issues:
- Chat models (like GPT-3.5, GPT-4) are different from embedding models (like text-embedding-ada-002)
- No validation to ensure selected model supports embeddings
- UI doesn't distinguish between chat and embedding models
This could lead to runtime errors if incompatible models are selected.
Consider maintaining a separate list of embedding models:
+const EMBEDDING_MODELS = [
+ { id: 'text-embedding-ada-002', name: 'Ada 002' },
+ // Add other supported embedding models
+]
<FieldWrapper
title="Embedding Model"
description="Choose between available embedding models"
row={true}
>
<select
value={chatSettings.embeddingModel || ''}
className="input cdx-w-44"
onChange={(e) => {
setSettings({
...settings,
chat: {
...chatSettings,
embeddingModel: e.target.value,
},
})
}}
>
- {models.map((model) => (
+ {EMBEDDING_MODELS.map((model) => (
<option key={model.id} value={model.id}>
- {model.id}
+ {model.name}
</option>
))}
</select>
</FieldWrapper>Committable suggestion skipped: line range outside the PR's diff.
Related to #99
For more details, open the Copilot Workspace session.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
embeddingModelin the settings configuration.