-
Notifications
You must be signed in to change notification settings - Fork 0
release/v4.210.1 #25
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
release/v4.210.1 #25
Conversation
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.
🧪 PR Review is completed: Review of code indexing service updates. Identified unused constructor parameters in processors and a potential performance optimization for batch sizing.
⬇️ Low Priority Suggestions (1)
src/services/code-index/constants/index.ts (1 suggestion)
Location:
src/services/code-index/constants/index.ts(Lines 19-19)🟡 Performance
Issue:
BATCH_SEGMENT_THRESHOLDis set to 10, which is relatively low for batching embedding operations. Small batch sizes can lead to inefficient network usage and potential rate limiting.Fix: Consider increasing the threshold (e.g., to 50) to improve throughput, while ensuring
MAX_PENDING_BATCHESkeeps memory usage in check.Impact: Improved indexing performance and reduced API overhead
- export const BATCH_SEGMENT_THRESHOLD = 10 // Number of code segments to batch for embeddings/upserts + export const BATCH_SEGMENT_THRESHOLD = 50 // Number of code segments to batch for embeddings/upserts
| _batchSegmentThreshold?: number, | ||
| ) { | ||
| // Get the configurable batch size from VSCode settings, fallback to default | ||
| // If not provided in constructor, try to get from VSCode settings | ||
| if (batchSegmentThreshold !== undefined) { | ||
| this.batchSegmentThreshold = batchSegmentThreshold | ||
| } else { | ||
| try { | ||
| this.batchSegmentThreshold = vscode.workspace | ||
| .getConfiguration(Package.name) | ||
| .get<number>("codeIndex.embeddingBatchSize", BATCH_SEGMENT_THRESHOLD) | ||
| } catch { | ||
| // In test environment, vscode.workspace might not be available | ||
| this.batchSegmentThreshold = BATCH_SEGMENT_THRESHOLD | ||
| } | ||
| } | ||
| this.batchSegmentThreshold = BATCH_SEGMENT_THRESHOLD |
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.
🟡 Code Quality
Issue: The constructor accepts _batchSegmentThreshold but ignores it, hardcoding this.batchSegmentThreshold to the constant. This prevents configuration overrides (e.g., for testing) and makes the parameter misleading.
Fix: Use the passed parameter if available, falling back to the constant. Remove the _ prefix to indicate it is used.
Impact: Improves code correctness and testability
| _batchSegmentThreshold?: number, | |
| ) { | |
| // Get the configurable batch size from VSCode settings, fallback to default | |
| // If not provided in constructor, try to get from VSCode settings | |
| if (batchSegmentThreshold !== undefined) { | |
| this.batchSegmentThreshold = batchSegmentThreshold | |
| } else { | |
| try { | |
| this.batchSegmentThreshold = vscode.workspace | |
| .getConfiguration(Package.name) | |
| .get<number>("codeIndex.embeddingBatchSize", BATCH_SEGMENT_THRESHOLD) | |
| } catch { | |
| // In test environment, vscode.workspace might not be available | |
| this.batchSegmentThreshold = BATCH_SEGMENT_THRESHOLD | |
| } | |
| } | |
| this.batchSegmentThreshold = BATCH_SEGMENT_THRESHOLD | |
| batchSegmentThreshold?: number, | |
| ) { | |
| this.batchSegmentThreshold = batchSegmentThreshold ?? BATCH_SEGMENT_THRESHOLD |
| _batchSegmentThreshold?: number, | ||
| ) { | ||
| this.ignoreController = ignoreController || new RooIgnoreController(workspacePath) | ||
| if (ignoreInstance) { | ||
| this.ignoreInstance = ignoreInstance | ||
| } | ||
| // Get the configurable batch size from VSCode settings, fallback to default | ||
| // If not provided in constructor, try to get from VSCode settings | ||
| if (batchSegmentThreshold !== undefined) { | ||
| this.batchSegmentThreshold = batchSegmentThreshold | ||
| } else { | ||
| try { | ||
| this.batchSegmentThreshold = vscode.workspace | ||
| .getConfiguration(Package.name) | ||
| .get<number>("codeIndex.embeddingBatchSize", BATCH_SEGMENT_THRESHOLD) | ||
| } catch { | ||
| // In test environment, vscode.workspace might not be available | ||
| this.batchSegmentThreshold = BATCH_SEGMENT_THRESHOLD | ||
| } | ||
| } | ||
| this.batchSegmentThreshold = BATCH_SEGMENT_THRESHOLD |
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.
🟡 Code Quality
Issue: The constructor accepts _batchSegmentThreshold but ignores it, hardcoding this.batchSegmentThreshold to the constant. This prevents configuration overrides (e.g., for testing) and makes the parameter misleading.
Fix: Use the passed parameter if available, falling back to the constant. Remove the _ prefix to indicate it is used.
Impact: Improves code correctness and testability
| _batchSegmentThreshold?: number, | |
| ) { | |
| this.ignoreController = ignoreController || new RooIgnoreController(workspacePath) | |
| if (ignoreInstance) { | |
| this.ignoreInstance = ignoreInstance | |
| } | |
| // Get the configurable batch size from VSCode settings, fallback to default | |
| // If not provided in constructor, try to get from VSCode settings | |
| if (batchSegmentThreshold !== undefined) { | |
| this.batchSegmentThreshold = batchSegmentThreshold | |
| } else { | |
| try { | |
| this.batchSegmentThreshold = vscode.workspace | |
| .getConfiguration(Package.name) | |
| .get<number>("codeIndex.embeddingBatchSize", BATCH_SEGMENT_THRESHOLD) | |
| } catch { | |
| // In test environment, vscode.workspace might not be available | |
| this.batchSegmentThreshold = BATCH_SEGMENT_THRESHOLD | |
| } | |
| } | |
| this.batchSegmentThreshold = BATCH_SEGMENT_THRESHOLD | |
| batchSegmentThreshold?: number, | |
| ) { | |
| this.ignoreController = ignoreController || new RooIgnoreController(workspacePath) | |
| if (ignoreInstance) { | |
| this.ignoreInstance = ignoreInstance | |
| } | |
| this.batchSegmentThreshold = batchSegmentThreshold ?? BATCH_SEGMENT_THRESHOLD |
|
✅ Reviewed the changes: The changes correctly update the branding text across UI and tests. I've added a minor suggestion to improve the styling implementation in the React component. |
No description provided.