-
Notifications
You must be signed in to change notification settings - Fork 2.6k
fix: correct export/import of OpenAI Compatible codebase indexing set… #5383
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: correct export/import of OpenAI Compatible codebase indexing set… #5383
Conversation
src/core/config/importExport.ts
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The variable 'targetProviderName' is set but never used. Consider removing it to simplify the code.
daniel-lxs
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've conducted a thorough analysis of this PR and discovered a critical architectural issue that explains the root cause of all three linked issues (#5035, #4962, #5091).
Critical Finding: Storage Location Mismatch
There's a fundamental mismatch between where OpenAI Compatible settings are saved vs. where they're read:
1. Writing (src/core/webview/webviewMessageHandler.ts lines 1841-1842)
Settings are saved INSIDE the codebaseIndexConfig object:
const globalStateConfig = {
...currentConfig,
codebaseIndexOpenAiCompatibleBaseUrl: settings.codebaseIndexOpenAiCompatibleBaseUrl,
codebaseIndexOpenAiCompatibleModelDimension: settings.codebaseIndexOpenAiCompatibleModelDimension,
}
await updateGlobalState("codebaseIndexConfig", globalStateConfig)2. Reading (src/services/code-index/config-manager.ts lines 62-65)
The code tries to read them as SEPARATE global state keys:
const openAiCompatibleBaseUrl = this.contextProxy?.getGlobalState("codebaseIndexOpenAiCompatibleBaseUrl") ?? ""
const openAiCompatibleModelDimension = this.contextProxy?.getGlobalState("codebaseIndexOpenAiCompatibleModelDimension")Why This Causes All Three Issues
- #5035: Export reads from the wrong location (separate keys that don't exist)
- #4962: Settings "disappear" because they're saved in one place but read from another
- #5091: Indexing fails to start because config-manager can't find the settings
Current PR Assessment
While this PR does fix the export/import functionality, it's essentially implementing a workaround by:
- Reading from global state during export (which happens to work because it reads from the correct location)
- Duplicating settings to provider profiles during import
The complex provider mapping logic (lines 72-112) is trying to work around the core issue rather than fixing it.
Recommended Solution
Instead of this workaround, we should fix the root cause. Two options:
Option 1: Update config-manager.ts to read from the correct location:
const codebaseIndexConfig = this.contextProxy?.getGlobalState("codebaseIndexConfig") ?? {}
const openAiCompatibleBaseUrl = codebaseIndexConfig.codebaseIndexOpenAiCompatibleBaseUrl ?? ""
const openAiCompatibleModelDimension = codebaseIndexConfig.codebaseIndexOpenAiCompatibleModelDimensionOption 2: Update webviewMessageHandler.ts to save as separate keys:
await updateGlobalState("codebaseIndexOpenAiCompatibleBaseUrl", settings.codebaseIndexOpenAiCompatibleBaseUrl)
await updateGlobalState("codebaseIndexOpenAiCompatibleModelDimension", settings.codebaseIndexOpenAiCompatibleModelDimension)Code Quality Issues
- Unused variable (line 77):
targetProviderNameis assigned but never used - Empty catch block (line 227): Silently swallows errors without logging
Questions for Consideration
- Should we fix the root cause instead of implementing this workaround?
- If we keep the provider profile approach, what's the migration strategy for existing users?
- Why maintain duplicate state in both global state and provider profiles?
While this PR does solve the immediate symptoms, addressing the underlying storage mismatch would provide a cleaner, more maintainable solution.
eca9074 to
eeb5b0a
Compare
|
I've addressed the root cause issue identified in the review. The fix includes:
The settings now properly persist across restarts, display correctly in the UI, and are included in exports/imports as expected. All tests are passing. |
…gs storage mismatch - Updated config-manager to read settings from correct location (codebaseIndexConfig object) - Added OpenAI Compatible fields to official type definitions - Removed complex workaround code from import/export functions - Fixed empty catch block by adding error logging - Updated all unit tests to reflect new architecture - All tests now passing (82 total)
- Fixed storage mismatch in config-manager.ts where settings were read from incorrect top-level keys instead of nested codebaseIndexConfig object - Updated CodebaseIndexConfig type definition to include OpenAI Compatible fields - Removed complex workaround logic from import/export that is no longer needed - Fixed UI bug in CodeIndexPopover where saved settings were not displayed - Updated unit tests to match the corrected architecture This addresses Dan's review feedback about the root cause of the bug and ensures OpenAI Compatible provider settings are properly saved, displayed, and exported.
eeb5b0a to
51e153c
Compare
Related GitHub IssueCloses: #5035 Roo Code Task Context (Optional)No Roo Code task context for this PR. DescriptionThis PR addresses the review feedback and fixes identified issues for PR #5383. Key Changes:
Review Comments Addressed:
Root Cause Analysis: Test ProcedureTesting performed:
To verify these changes:
Test Environment:
Pre-Submission Checklist
Screenshots / VideosNo UI changes in this PR. Documentation Updates
Additional NotesAll review feedback has been addressed. The rebase process resolved the code quality issues mentioned in the review, and the merge conflicts have been properly resolved. The fix addresses the root cause of three related issues (#5035, #4962, #5091) by ensuring consistent storage location for OpenAI Compatible settings. Files Modified: Get in TouchDiscord: @MuriloFP |
daniel-lxs
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
fix: correct export/import of OpenAI Compatible codebase indexing settings (#5035)
Related GitHub Issue
Closes: #5035
Closes: #4962
Closes: #5091
Roo Code Task Context (Optional)
Description
This PR fixes the export/import functionality for OpenAI Compatible codebase indexing settings. The issue was that exported settings were missing the
codebaseIndexEmbedderModelDimensionproperty and contained an incorrectcodebaseIndexEmbedderBaseUrl.Key implementation details:
exportSettingsfunction to correctly fetch OpenAI Compatible settings from global state instead of provider profilescodebaseIndexEmbedderModelDimensionproperty to thecodebaseIndexConfigSchematype definitioncontextProxy.getGlobalStatecallsDesign choice: The fix maintains the existing architecture where OpenAI Compatible indexing settings are stored in global state rather than provider profiles, ensuring consistency with the current implementation.
Test Procedure
Automated Testing:
Manual Testing Steps:
codebaseIndexEmbedderBaseUrlandcodebaseIndexEmbedderModelDimensionTest Commands:
cd src npx vitest core/config/__tests__/importExport.spec.tsPre-Submission Checklist
Screenshots / Videos
Documentation Updates
Additional Notes
This fix ensures that users can properly export and import their OpenAI Compatible codebase indexing configurations, including custom base URLs and model dimensions. The bug was caused by the export function looking for these settings in the wrong location (provider profiles instead of global state).
I had some extra issues with this PR because the import/export settings were not properly updating the UI state and the OpenAI Compatible Provider specific fields... The tests were created to ensure that these work correctly.
Get in Touch
@MuriloFP
Important
Fixes export/import of OpenAI Compatible codebase indexing settings by ensuring correct handling of global state and provider-specific settings.
importExport.ts.codebaseIndexEmbedderModelDimensionis included in exports and imports.codebaseIndexEmbedderModelDimensiontocodebaseIndexConfigSchemaincodebase-index.ts.importExport.spec.tsto verify correct export/import of settings, including edge cases and provider mismatches.This description was created by
for 9d71d1a66aed9a8271d7f451e81db4ee6227261b. You can customize this summary. It will automatically update as commits are pushed.