fix: pre-populate memory onboarding step from existing settings#1992
fix: pre-populate memory onboarding step from existing settings#1992octo-patch wants to merge 1 commit intoAndyMik90:developfrom
Conversation
…AndyMik90#1940) The MemoryStep component in the onboarding wizard always initialized with hardcoded defaults (enabled=true, provider='ollama', empty Azure/Voyage keys), ignoring any previously saved configuration. When users re-ran the wizard via Settings, the memory step showed defaults instead of their saved settings, which could silently overwrite configuration on Save.
|
|
📝 WalkthroughWalkthroughUpdated the MemoryStep component to initialize its configuration state from persisted settings values instead of hardcoded defaults. The Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Poem
🚥 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 |
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| azureOpenaiEmbeddingDeployment: settings.memoryAzureEmbeddingDeployment || '', | ||
| voyageApiKey: settings.memoryVoyageApiKey || '', | ||
| voyageEmbeddingModel: settings.memoryVoyageEmbeddingModel || '', | ||
| googleApiKey: settings.globalGoogleApiKey || '', |
There was a problem hiding this comment.
Bug: The memory onboarding step incorrectly uses the global globalGoogleApiKey setting instead of the memory-specific memoryGoogleApiKey, leading to incorrect key persistence and retrieval for Google embeddings.
Severity: MEDIUM
Suggested Fix
Update MemoryStep.tsx to use the memoryGoogleApiKey setting for both reading the initial value and for saving the updated value. Specifically, change settings.globalGoogleApiKey to settings.memoryGoogleApiKey where it's read and written. Additionally, ensure the memory-service-factory.ts uses settings.memoryGoogleApiKey when creating the embedding configuration.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: apps/desktop/src/renderer/components/onboarding/MemoryStep.tsx#L33
Potential issue: The `MemoryStep.tsx` component incorrectly reads from and writes to the
`globalGoogleApiKey` setting instead of the intended `memoryGoogleApiKey`. This is
inconsistent with the handling of Azure and Voyage keys, which were correctly updated in
this PR to use their memory-specific settings. As a result, if a user configures a
Google API key for memory, it will overwrite the global key. When re-running the wizard,
the global key will be displayed, and the memory-specific key will be permanently
ignored, breaking the separation of concerns for memory-specific configurations.
Did we get this right? 👍 / 👎 to inform future reviews.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/desktop/src/renderer/components/onboarding/MemoryStep.tsx (1)
23-38:⚠️ Potential issue | 🟠 MajorAdd
useEffectto resync local config state when settings hydrate.On Line 23,
configis initialized fromsettingsonly once. SinceloadSettings()is async andOnboardingWizardrenders before completion,MemoryStepcan mount while store settings are still loading with defaults. The localconfigstate never updates when persisted settings arrive, causing Save & Continue to overwrite with defaults instead of pre-populating from existing configuration.Add a
useEffectdependency onsettingsto resyncconfigwhen the store updates:Suggested fix
+import { useEffect, useState } from 'react'; ... const [config, setConfig] = useState<MemoryPanelConfig>({ enabled: settings.memoryEnabled ?? true, embeddingProvider: settings.memoryEmbeddingProvider || 'ollama', openaiApiKey: settings.globalOpenAIApiKey || '', openaiEmbeddingModel: settings.memoryOpenaiEmbeddingModel || '', azureOpenaiApiKey: settings.memoryAzureApiKey || '', azureOpenaiBaseUrl: settings.memoryAzureBaseUrl || '', azureOpenaiEmbeddingDeployment: settings.memoryAzureEmbeddingDeployment || '', voyageApiKey: settings.memoryVoyageApiKey || '', voyageEmbeddingModel: settings.memoryVoyageEmbeddingModel || '', googleApiKey: settings.globalGoogleApiKey || '', googleEmbeddingModel: settings.memoryGoogleEmbeddingModel || '', ollamaBaseUrl: settings.ollamaBaseUrl || 'http://localhost:11434', ollamaEmbeddingModel: settings.memoryOllamaEmbeddingModel || 'qwen3-embedding:4b', ollamaEmbeddingDim: settings.memoryOllamaEmbeddingDim ?? 2560, }); + + useEffect(() => { + setConfig({ + enabled: settings.memoryEnabled ?? true, + embeddingProvider: settings.memoryEmbeddingProvider || 'ollama', + openaiApiKey: settings.globalOpenAIApiKey || '', + openaiEmbeddingModel: settings.memoryOpenaiEmbeddingModel || '', + azureOpenaiApiKey: settings.memoryAzureApiKey || '', + azureOpenaiBaseUrl: settings.memoryAzureBaseUrl || '', + azureOpenaiEmbeddingDeployment: settings.memoryAzureEmbeddingDeployment || '', + voyageApiKey: settings.memoryVoyageApiKey || '', + voyageEmbeddingModel: settings.memoryVoyageEmbeddingModel || '', + googleApiKey: settings.globalGoogleApiKey || '', + googleEmbeddingModel: settings.memoryGoogleEmbeddingModel || '', + ollamaBaseUrl: settings.ollamaBaseUrl || 'http://localhost:11434', + ollamaEmbeddingModel: settings.memoryOllamaEmbeddingModel || 'qwen3-embedding:4b', + ollamaEmbeddingDim: settings.memoryOllamaEmbeddingDim ?? 2560, + }); + }, [settings]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/components/onboarding/MemoryStep.tsx` around lines 23 - 38, MemoryStep initializes local state variable config from settings only once, so when loadSettings() hydrates asynchronously the prefilled values are never applied; add a useEffect in the MemoryStep component that watches the settings object and calls setConfig to resync the local config whenever settings changes (keep existing default fallbacks when copying values like enabled, embeddingProvider, openaiApiKey, ollamaBaseUrl, ollamaEmbeddingModel, etc.), so Save & Continue uses the hydrated store values instead of overwriting with defaults.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@apps/desktop/src/renderer/components/onboarding/MemoryStep.tsx`:
- Around line 23-38: MemoryStep initializes local state variable config from
settings only once, so when loadSettings() hydrates asynchronously the prefilled
values are never applied; add a useEffect in the MemoryStep component that
watches the settings object and calls setConfig to resync the local config
whenever settings changes (keep existing default fallbacks when copying values
like enabled, embeddingProvider, openaiApiKey, ollamaBaseUrl,
ollamaEmbeddingModel, etc.), so Save & Continue uses the hydrated store values
instead of overwriting with defaults.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1484a5c1-3533-4694-bbf4-921ba924e7aa
📒 Files selected for processing (1)
apps/desktop/src/renderer/components/onboarding/MemoryStep.tsx
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the MemoryStep component to initialize its configuration state using existing values from the settings store instead of hardcoded defaults. A suggestion was made to use lazy initialization for the useState hook to optimize performance by preventing the configuration object from being re-created on every render.
| const [config, setConfig] = useState<MemoryPanelConfig>({ | ||
| enabled: true, | ||
| embeddingProvider: 'ollama', | ||
| enabled: settings.memoryEnabled ?? true, | ||
| embeddingProvider: settings.memoryEmbeddingProvider || 'ollama', | ||
| openaiApiKey: settings.globalOpenAIApiKey || '', | ||
| openaiEmbeddingModel: settings.memoryOpenaiEmbeddingModel || '', | ||
| azureOpenaiApiKey: '', | ||
| azureOpenaiBaseUrl: '', | ||
| azureOpenaiEmbeddingDeployment: '', | ||
| voyageApiKey: '', | ||
| azureOpenaiApiKey: settings.memoryAzureApiKey || '', | ||
| azureOpenaiBaseUrl: settings.memoryAzureBaseUrl || '', | ||
| azureOpenaiEmbeddingDeployment: settings.memoryAzureEmbeddingDeployment || '', | ||
| voyageApiKey: settings.memoryVoyageApiKey || '', | ||
| voyageEmbeddingModel: settings.memoryVoyageEmbeddingModel || '', | ||
| googleApiKey: settings.globalGoogleApiKey || '', | ||
| googleEmbeddingModel: settings.memoryGoogleEmbeddingModel || '', |
There was a problem hiding this comment.
To avoid re-creating this configuration object on every render, you can use the lazy initialization form of useState. This is a minor performance optimization but is a good practice in React when the initial state requires some computation or object creation.
This ensures the object is created only once on the initial render, not on every render.
const [config, setConfig] = useState<MemoryPanelConfig>(() => ({
// ... all the properties
}));2 participants
Fixes #1940
Problem
The
MemoryStepcomponent in the onboarding wizard always initialized with hardcoded defaults, ignoring any previously saved configuration:enabledwas hardcoded totrueinstead of readingsettings.memoryEnabledembeddingProviderwas hardcoded to'ollama'instead of readingsettings.memoryEmbeddingProviderWhen users re-ran the wizard via Settings → Re-run Wizard, the memory step always showed defaults instead of their saved settings. Clicking "Save & Continue" would then silently overwrite their configuration with the defaults.
Solution
Initialize all config fields from the existing
settingsstore values, with appropriate fallbacks:Testing
Summary by CodeRabbit