Skip to content

Conversation

@daniel-lxs
Copy link
Member

@daniel-lxs daniel-lxs commented Jul 7, 2025

Description

This PR is blocking #3999

This PR removes the legacy codebaseIndexOpenAiCompatibleModelDimension property and standardizes on the generic codebaseIndexEmbedderModelDimension property throughout the codebase.

Changes Made

1. Backend Configuration (src/services/code-index/config-manager.ts):

  • Removed migration logic that was converting the old property to the new one
  • Cleaned up the getConfig() method to only use the generic property

2. Message Types (src/shared/WebviewMessage.ts):

  • Removed the legacy codebaseIndexOpenAiCompatibleModelDimension property from the CodeIndexSettings interface
  • The interface now only contains the generic codebaseIndexEmbedderModelDimension property

3. Webview Message Handler (src/core/webview/webviewMessageHandler.ts):

  • Removed backward compatibility code that was setting both properties
  • Now only sets the generic codebaseIndexEmbedderModelDimension property

4. UI Component (webview-ui/src/components/chat/CodeIndexPopover.tsx):

  • Removed the legacy property from the component's interface and default settings
  • Removed backward compatibility logic in the initialization
  • Improved UI logic: The "Model Dimension" input field is now only shown for OpenAI-compatible providers where custom models might be used
  • The dimension field is hidden for OpenAI, Ollama, and Gemini providers since their model dimensions are auto-detected

5. Test Files:

  • Fixed duplicate property definitions in src/core/config/__tests__/importExport.spec.ts
  • Updated all test files to use the generic property name

Testing

  • ✅ All backend tests pass (config-manager.spec.ts, importExport.spec.ts)
  • ✅ All UI tests pass (510 tests passed, 1 skipped)
  • ✅ Type checking passes
  • ✅ Linting passes

Verification

The codebase now consistently uses only codebaseIndexEmbedderModelDimension for all providers, eliminating the technical debt from the old provider-specific naming convention.

Benefits

  1. Cleaner codebase: Removes unnecessary backward compatibility code
  2. Better UX: The UI now intelligently shows/hides the dimension field based on provider
  3. Maintainability: Single property name makes the code easier to understand and maintain
  4. Future-proof: New providers can use the same generic property without special handling

Important

Refactor to remove legacy codebaseIndexOpenAiCompatibleModelDimension and standardize on codebaseIndexEmbedderModelDimension across the codebase.

  • Configuration:
    • Removed codebaseIndexOpenAiCompatibleModelDimension from config-manager.ts.
    • Standardized on codebaseIndexEmbedderModelDimension for all providers.
  • Message Types:
    • Updated WebviewMessage.ts to use codebaseIndexEmbedderModelDimension.
  • UI Component:
    • Updated CodeIndexPopover.tsx to use codebaseIndexEmbedderModelDimension.
    • Improved UI logic to show/hide "Model Dimension" field based on provider.
  • Tests:
    • Updated tests in config-manager.spec.ts and service-factory.spec.ts to reflect property changes.
  • Misc:
    • Updated i18n files to reflect changes in error messages and configuration keys.

This description was created by Ellipsis for 167a9e8. You can customize this summary. It will automatically update as commits are pushed.

@daniel-lxs daniel-lxs requested review from cte, jr and mrubens as code owners July 7, 2025 22:27
@dosubot dosubot bot added the size:L This PR changes 100-499 lines, ignoring generated files. label Jul 7, 2025
@delve-auditor
Copy link

delve-auditor bot commented Jul 7, 2025

No security or compliance issues detected. Reviewed everything up to 167a9e8.

Security Overview
  • 🔎 Scanned files: 27 changed file(s)
Detected Code Changes
Change Type Relevant files
Refactor ► config-manager.ts
    Move embedding dimension to generic config
► service-factory.ts
    Update vector dimension handling
► modes.ts
    Simplify mode selection logic
Enhancement ► embeddings.json
    Add new translation strings for service factory errors
Bug Fix ► reminder.ts
    Remove default todo list message

Reply to this PR with @delve-auditor followed by a description of what change you want and we'll auto-submit a change to this PR to implement it.

@hannesrudolph hannesrudolph added the Issue/PR - Triage New issue. Needs quick review to confirm validity and assign labels. label Jul 7, 2025
@daniel-lxs daniel-lxs moved this from Triage to PR [Draft / In Progress] in Roo Code Roadmap Jul 8, 2025
@hannesrudolph hannesrudolph added PR - Draft / In Progress and removed Issue/PR - Triage New issue. Needs quick review to confirm validity and assign labels. labels Jul 8, 2025
…roperty

- Remove backward compatibility for codebaseIndexOpenAiCompatibleModelDimension
- Standardize on generic codebaseIndexEmbedderModelDimension property
- Remove migration logic from config-manager.ts
- Update WebviewMessage interface to remove legacy property
- Clean up webviewMessageHandler to only set generic property
- Update UI component to remove legacy property handling
- Improve UI logic to only show dimension field for OpenAI-compatible providers
- Update all related tests to use generic property

This simplifies the codebase by removing technical debt from the old provider-specific dimension property naming convention.
… fix test expectations

- Added validation in config-manager.ts to ensure codebaseIndexEmbedderModelDimension is a valid positive number
- Fixed error message formatting in service-factory.ts to include bold/underline formatting for 'OpenAI-Compatible'
- Updated tests to use config.modelDimension instead of openAiCompatibleOptions.modelDimension
- Added check for vectorSize <= 0 in addition to undefined check
- Fixed failing test that expected invalid modelDimension to be preserved
- Implementation correctly converts invalid dimension values to undefined
- Added missing geminiOptions field to expected test output
… ANSI escape codes

- Replace hardcoded error strings with translation function calls
- Remove ANSI escape codes from error messages as they don't render properly in all contexts
- Add translations for all 17 supported languages
- Addresses PR review feedback about error message formatting
@daniel-lxs daniel-lxs force-pushed the fix/remove-legacy-dimension-property branch from 1aefdb6 to 1ca4b5a Compare July 8, 2025 17:06
- Fixed missing closing braces in validation sections
- Ensured proper JSON structure across all language files
- All translation checks now pass
- Japanese: Use consistent term '埋め込み次元' (embedding dimension) instead of mixing with 'ベクター次元' (vector dimension)
- Turkish: Fix grammatical error by changing 'yapılandırma' to 'yapılandırması' (possessive form)
@daniel-lxs daniel-lxs moved this from PR [Draft / In Progress] to PR [Needs Review] in Roo Code Roadmap Jul 8, 2025
@mrubens mrubens merged commit c551e17 into main Jul 8, 2025
18 of 19 checks passed
@mrubens mrubens deleted the fix/remove-legacy-dimension-property branch July 8, 2025 18:43
@github-project-automation github-project-automation bot moved this from PR [Needs Review] to Done in Roo Code Roadmap Jul 8, 2025
@github-project-automation github-project-automation bot moved this from New to Done in Roo Code Roadmap Jul 8, 2025
@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Jul 8, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

lgtm This PR has been approved by a maintainer PR - Draft / In Progress size:L This PR changes 100-499 lines, ignoring generated files.

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

4 participants