-
Notifications
You must be signed in to change notification settings - Fork 2.5k
feat: Add Qdrant collection name field to codebase indexing settings #7729
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
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -367,6 +367,7 @@ describe("CodeIndexServiceFactory", () => { | |
| "http://localhost:6333", | ||
| 3072, | ||
| "test-key", | ||
| undefined, // collectionName | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The test correctly passes
Should we add test coverage for the new parameter? |
||
| ) | ||
| }) | ||
|
|
||
|
|
@@ -392,6 +393,7 @@ describe("CodeIndexServiceFactory", () => { | |
| "http://localhost:6333", | ||
| 768, | ||
| "test-key", | ||
| undefined, // collectionName | ||
| ) | ||
| }) | ||
|
|
||
|
|
@@ -417,6 +419,7 @@ describe("CodeIndexServiceFactory", () => { | |
| "http://localhost:6333", | ||
| 3072, | ||
| "test-key", | ||
| undefined, // collectionName | ||
| ) | ||
| }) | ||
|
|
||
|
|
@@ -449,6 +452,7 @@ describe("CodeIndexServiceFactory", () => { | |
| "http://localhost:6333", | ||
| modelDimension, // Should use model's built-in dimension, not manual | ||
| "test-key", | ||
| undefined, // collectionName | ||
| ) | ||
| }) | ||
|
|
||
|
|
@@ -480,6 +484,7 @@ describe("CodeIndexServiceFactory", () => { | |
| "http://localhost:6333", | ||
| manualDimension, // Should use manual dimension as fallback | ||
| "test-key", | ||
| undefined, // collectionName | ||
| ) | ||
| }) | ||
|
|
||
|
|
@@ -509,6 +514,7 @@ describe("CodeIndexServiceFactory", () => { | |
| "http://localhost:6333", | ||
| 768, | ||
| "test-key", | ||
| undefined, // collectionName | ||
| ) | ||
| }) | ||
|
|
||
|
|
@@ -578,6 +584,7 @@ describe("CodeIndexServiceFactory", () => { | |
| "http://localhost:6333", | ||
| 3072, | ||
| "test-key", | ||
| undefined, // collectionName | ||
| ) | ||
| }) | ||
|
|
||
|
|
@@ -603,6 +610,7 @@ describe("CodeIndexServiceFactory", () => { | |
| "http://localhost:6333", | ||
| 3072, | ||
| "test-key", | ||
| undefined, // collectionName | ||
| ) | ||
| }) | ||
|
|
||
|
|
@@ -627,6 +635,7 @@ describe("CodeIndexServiceFactory", () => { | |
| "http://localhost:6333", | ||
| 1536, | ||
| "test-key", | ||
| undefined, // collectionName | ||
| ) | ||
| }) | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -21,6 +21,7 @@ export class CodeIndexConfigManager { | |||||||||||||||||
| private mistralOptions?: { apiKey: string } | ||||||||||||||||||
| private vercelAiGatewayOptions?: { apiKey: string } | ||||||||||||||||||
| private qdrantUrl?: string = "http://localhost:6333" | ||||||||||||||||||
| private qdrantCollectionName?: string | ||||||||||||||||||
| private qdrantApiKey?: string | ||||||||||||||||||
| private searchMinScore?: number | ||||||||||||||||||
| private searchMaxResults?: number | ||||||||||||||||||
|
|
@@ -46,6 +47,7 @@ export class CodeIndexConfigManager { | |||||||||||||||||
| const codebaseIndexConfig = this.contextProxy?.getGlobalState("codebaseIndexConfig") ?? { | ||||||||||||||||||
| codebaseIndexEnabled: true, | ||||||||||||||||||
| codebaseIndexQdrantUrl: "http://localhost:6333", | ||||||||||||||||||
| codebaseIndexQdrantCollectionName: "", | ||||||||||||||||||
| codebaseIndexEmbedderProvider: "openai", | ||||||||||||||||||
| codebaseIndexEmbedderBaseUrl: "", | ||||||||||||||||||
| codebaseIndexEmbedderModelId: "", | ||||||||||||||||||
|
|
@@ -56,6 +58,7 @@ export class CodeIndexConfigManager { | |||||||||||||||||
| const { | ||||||||||||||||||
| codebaseIndexEnabled, | ||||||||||||||||||
| codebaseIndexQdrantUrl, | ||||||||||||||||||
| codebaseIndexQdrantCollectionName, | ||||||||||||||||||
| codebaseIndexEmbedderProvider, | ||||||||||||||||||
| codebaseIndexEmbedderBaseUrl, | ||||||||||||||||||
| codebaseIndexEmbedderModelId, | ||||||||||||||||||
|
|
@@ -75,6 +78,7 @@ export class CodeIndexConfigManager { | |||||||||||||||||
| // Update instance variables with configuration | ||||||||||||||||||
| this.codebaseIndexEnabled = codebaseIndexEnabled ?? true | ||||||||||||||||||
| this.qdrantUrl = codebaseIndexQdrantUrl | ||||||||||||||||||
| this.qdrantCollectionName = codebaseIndexQdrantCollectionName | ||||||||||||||||||
| this.qdrantApiKey = qdrantApiKey ?? "" | ||||||||||||||||||
| this.searchMinScore = codebaseIndexSearchMinScore | ||||||||||||||||||
| this.searchMaxResults = codebaseIndexSearchMaxResults | ||||||||||||||||||
|
|
@@ -148,6 +152,7 @@ export class CodeIndexConfigManager { | |||||||||||||||||
| mistralOptions?: { apiKey: string } | ||||||||||||||||||
| vercelAiGatewayOptions?: { apiKey: string } | ||||||||||||||||||
| qdrantUrl?: string | ||||||||||||||||||
| qdrantCollectionName?: string | ||||||||||||||||||
| qdrantApiKey?: string | ||||||||||||||||||
| searchMinScore?: number | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
@@ -168,6 +173,7 @@ export class CodeIndexConfigManager { | |||||||||||||||||
| mistralApiKey: this.mistralOptions?.apiKey ?? "", | ||||||||||||||||||
| vercelAiGatewayApiKey: this.vercelAiGatewayOptions?.apiKey ?? "", | ||||||||||||||||||
| qdrantUrl: this.qdrantUrl ?? "", | ||||||||||||||||||
| qdrantCollectionName: this.qdrantCollectionName ?? "", | ||||||||||||||||||
| qdrantApiKey: this.qdrantApiKey ?? "", | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
|
|
@@ -193,6 +199,7 @@ export class CodeIndexConfigManager { | |||||||||||||||||
| mistralOptions: this.mistralOptions, | ||||||||||||||||||
| vercelAiGatewayOptions: this.vercelAiGatewayOptions, | ||||||||||||||||||
| qdrantUrl: this.qdrantUrl, | ||||||||||||||||||
| qdrantCollectionName: this.qdrantCollectionName, | ||||||||||||||||||
| qdrantApiKey: this.qdrantApiKey, | ||||||||||||||||||
| searchMinScore: this.currentSearchMinScore, | ||||||||||||||||||
| }, | ||||||||||||||||||
|
|
@@ -270,6 +277,7 @@ export class CodeIndexConfigManager { | |||||||||||||||||
| const prevMistralApiKey = prev?.mistralApiKey ?? "" | ||||||||||||||||||
| const prevVercelAiGatewayApiKey = prev?.vercelAiGatewayApiKey ?? "" | ||||||||||||||||||
| const prevQdrantUrl = prev?.qdrantUrl ?? "" | ||||||||||||||||||
| const prevQdrantCollectionName = prev?.qdrantCollectionName ?? "" | ||||||||||||||||||
| const prevQdrantApiKey = prev?.qdrantApiKey ?? "" | ||||||||||||||||||
|
|
||||||||||||||||||
| // 1. Transition from disabled/unconfigured to enabled/configured | ||||||||||||||||||
|
|
@@ -308,6 +316,7 @@ export class CodeIndexConfigManager { | |||||||||||||||||
| const currentMistralApiKey = this.mistralOptions?.apiKey ?? "" | ||||||||||||||||||
| const currentVercelAiGatewayApiKey = this.vercelAiGatewayOptions?.apiKey ?? "" | ||||||||||||||||||
| const currentQdrantUrl = this.qdrantUrl ?? "" | ||||||||||||||||||
| const currentQdrantCollectionName = this.qdrantCollectionName ?? "" | ||||||||||||||||||
| const currentQdrantApiKey = this.qdrantApiKey ?? "" | ||||||||||||||||||
|
|
||||||||||||||||||
| if (prevOpenAiKey !== currentOpenAiKey) { | ||||||||||||||||||
|
|
@@ -342,7 +351,11 @@ export class CodeIndexConfigManager { | |||||||||||||||||
| return true | ||||||||||||||||||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Critical Issue: Missing restart detection for collection name changes. The Could we add this check around line 351?
Suggested change
|
||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| if (prevQdrantUrl !== currentQdrantUrl || prevQdrantApiKey !== currentQdrantApiKey) { | ||||||||||||||||||
| if ( | ||||||||||||||||||
| prevQdrantUrl !== currentQdrantUrl || | ||||||||||||||||||
| prevQdrantCollectionName !== currentQdrantCollectionName || | ||||||||||||||||||
| prevQdrantApiKey !== currentQdrantApiKey | ||||||||||||||||||
| ) { | ||||||||||||||||||
| return true | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
|
|
@@ -396,6 +409,7 @@ export class CodeIndexConfigManager { | |||||||||||||||||
| mistralOptions: this.mistralOptions, | ||||||||||||||||||
| vercelAiGatewayOptions: this.vercelAiGatewayOptions, | ||||||||||||||||||
| qdrantUrl: this.qdrantUrl, | ||||||||||||||||||
| qdrantCollectionName: this.qdrantCollectionName, | ||||||||||||||||||
| qdrantApiKey: this.qdrantApiKey, | ||||||||||||||||||
| searchMinScore: this.currentSearchMinScore, | ||||||||||||||||||
| searchMaxResults: this.currentSearchMaxResults, | ||||||||||||||||||
|
|
@@ -426,9 +440,10 @@ export class CodeIndexConfigManager { | |||||||||||||||||
| /** | ||||||||||||||||||
| * Gets the current Qdrant configuration | ||||||||||||||||||
| */ | ||||||||||||||||||
| public get qdrantConfig(): { url?: string; apiKey?: string } { | ||||||||||||||||||
| public get qdrantConfig(): { url?: string; collectionName?: string; apiKey?: string } { | ||||||||||||||||||
| return { | ||||||||||||||||||
| url: this.qdrantUrl, | ||||||||||||||||||
| collectionName: this.qdrantCollectionName, | ||||||||||||||||||
| apiKey: this.qdrantApiKey, | ||||||||||||||||||
| } | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -24,7 +24,7 @@ export class QdrantVectorStore implements IVectorStore { | |||||||||||||||||
| * @param workspacePath Path to the workspace | ||||||||||||||||||
| * @param url Optional URL to the Qdrant server | ||||||||||||||||||
| */ | ||||||||||||||||||
| constructor(workspacePath: string, url: string, vectorSize: number, apiKey?: string) { | ||||||||||||||||||
| constructor(workspacePath: string, url: string, vectorSize: number, apiKey?: string, collectionName?: string) { | ||||||||||||||||||
| // Parse the URL to determine the appropriate QdrantClient configuration | ||||||||||||||||||
| const parsedUrl = this.parseQdrantUrl(url) | ||||||||||||||||||
|
|
||||||||||||||||||
|
|
@@ -77,10 +77,17 @@ export class QdrantVectorStore implements IVectorStore { | |||||||||||||||||
| }) | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| // Generate collection name from workspace path | ||||||||||||||||||
| const hash = createHash("sha256").update(workspacePath).digest("hex") | ||||||||||||||||||
| // Use provided collection name or generate from workspace path | ||||||||||||||||||
| if (collectionName && collectionName.trim()) { | ||||||||||||||||||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice sanitization logic! However, could we add a comment explaining the regex pattern for future maintainers?
Suggested change
|
||||||||||||||||||
| // Sanitize the collection name to ensure it's valid for Qdrant | ||||||||||||||||||
| // Qdrant collection names must match: ^[a-zA-Z0-9_-]+$ | ||||||||||||||||||
| this.collectionName = collectionName.trim().replace(/[^a-zA-Z0-9_-]/g, "-") | ||||||||||||||||||
| } else { | ||||||||||||||||||
| // Generate collection name from workspace path (default behavior) | ||||||||||||||||||
| const hash = createHash("sha256").update(workspacePath).digest("hex") | ||||||||||||||||||
| this.collectionName = `ws-${hash.substring(0, 16)}` | ||||||||||||||||||
| } | ||||||||||||||||||
| this.vectorSize = vectorSize | ||||||||||||||||||
| this.collectionName = `ws-${hash.substring(0, 16)}` | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| /** | ||||||||||||||||||
|
|
||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -58,6 +58,7 @@ interface LocalCodeIndexSettings { | |
| // Global state settings | ||
| codebaseIndexEnabled: boolean | ||
| codebaseIndexQdrantUrl: string | ||
| codebaseIndexQdrantCollectionName?: string | ||
| codebaseIndexEmbedderProvider: EmbedderProvider | ||
| codebaseIndexEmbedderBaseUrl?: string | ||
| codebaseIndexEmbedderModelId: string | ||
|
|
@@ -181,6 +182,7 @@ export const CodeIndexPopover: React.FC<CodeIndexPopoverProps> = ({ | |
| const getDefaultSettings = (): LocalCodeIndexSettings => ({ | ||
| codebaseIndexEnabled: true, | ||
| codebaseIndexQdrantUrl: "", | ||
| codebaseIndexQdrantCollectionName: "", | ||
| codebaseIndexEmbedderProvider: "openai", | ||
| codebaseIndexEmbedderBaseUrl: "", | ||
| codebaseIndexEmbedderModelId: "", | ||
|
|
@@ -213,6 +215,7 @@ export const CodeIndexPopover: React.FC<CodeIndexPopoverProps> = ({ | |
| const settings = { | ||
| codebaseIndexEnabled: codebaseIndexConfig.codebaseIndexEnabled ?? true, | ||
| codebaseIndexQdrantUrl: codebaseIndexConfig.codebaseIndexQdrantUrl || "", | ||
| codebaseIndexQdrantCollectionName: codebaseIndexConfig.codebaseIndexQdrantCollectionName || "", | ||
| codebaseIndexEmbedderProvider: codebaseIndexConfig.codebaseIndexEmbedderProvider || "openai", | ||
| codebaseIndexEmbedderBaseUrl: codebaseIndexConfig.codebaseIndexEmbedderBaseUrl || "", | ||
| codebaseIndexEmbedderModelId: codebaseIndexConfig.codebaseIndexEmbedderModelId || "", | ||
|
|
@@ -1160,6 +1163,30 @@ export const CodeIndexPopover: React.FC<CodeIndexPopoverProps> = ({ | |
| )} | ||
| </div> | ||
|
|
||
| <div className="space-y-2"> | ||
| <label className="text-sm font-medium"> | ||
| {t("settings:codeIndex.qdrantCollectionNameLabel")} | ||
| </label> | ||
| <VSCodeTextField | ||
| value={currentSettings.codebaseIndexQdrantCollectionName || ""} | ||
| onInput={(e: any) => | ||
| updateSetting("codebaseIndexQdrantCollectionName", e.target.value) | ||
| } | ||
| placeholder={t("settings:codeIndex.qdrantCollectionNamePlaceholder")} | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Suggestion: The UI doesn't validate collection names before saving. While the backend sanitizes invalid characters, it might be better UX to warn users in the UI when they enter invalid characters. Could we add validation feedback similar to other fields? Also, is the placeholder text informative enough about valid characters? Maybe something like: "Enter collection name (letters, numbers, -, _ only)"? |
||
| className={cn("w-full", { | ||
| "border-red-500": formErrors.codebaseIndexQdrantCollectionName, | ||
| })} | ||
| /> | ||
| {formErrors.codebaseIndexQdrantCollectionName && ( | ||
| <p className="text-xs text-vscode-errorForeground mt-1 mb-0"> | ||
| {formErrors.codebaseIndexQdrantCollectionName} | ||
| </p> | ||
| )} | ||
| <p className="text-xs text-vscode-descriptionForeground mt-1"> | ||
| {t("settings:codeIndex.qdrantCollectionNameDescription")} | ||
| </p> | ||
| </div> | ||
|
|
||
| <div className="space-y-2"> | ||
| <label className="text-sm font-medium"> | ||
| {t("settings:codeIndex.qdrantApiKeyLabel")} | ||
|
|
||
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.
Important: Missing test coverage for collection name functionality.
While the test file has been updated to include
qdrantCollectionNamein assertions, there are no specific tests for:Would it make sense to add dedicated test cases for this new feature?