feat: add ability to specify enabled preview features #686
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a new previewFeatures configuration option to enable non-GA functionality, replacing the previous approach of using voyageApiKey presence as a feature flag. Currently, the only preview feature is vectorSearch.
Key changes:
- New
previewFeaturesarray configuration option that accepts feature flags like"vectorSearch" - Refactored feature detection from checking
voyageApiKeypresence to checking thepreviewFeaturesarray - Updated tests to use the new configuration approach
Reviewed Changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| src/common/config.ts | Adds previewFeatures config option with enum validation and array handling |
| src/tools/tool.ts | Replaces FeatureFlags enum and isFeatureFlagEnabled() with isFeatureEnabled() using string literals |
| src/common/search/embeddingsProvider.ts | Updates vector search configuration check to include previewFeatures |
| src/tools/mongodb/metadata/collectionIndexes.ts | Updates feature check from FeatureFlags.VectorSearch to "vectorSearch" |
| src/tools/mongodb/create/createIndex.ts | Updates feature check from FeatureFlags.VectorSearch to "vectorSearch" |
| src/tools/mongodb/delete/dropIndex.ts | Updates feature check from FeatureFlags.VectorSearch to "vectorSearch" |
| tests/integration/tools/mongodb/read/aggregate.test.ts | Updates test config to use previewFeatures: ["vectorSearch"] |
| tests/integration/tools/mongodb/metadata/collectionIndexes.test.ts | Updates test config to use previewFeatures: ["vectorSearch"] |
| tests/integration/tools/mongodb/delete/dropIndex.test.ts | Updates test config to use previewFeatures: ["vectorSearch"] |
| tests/integration/tools/mongodb/create/createIndex.test.ts | Updates test assertions and config to use previewFeatures |
| tests/accuracy/aggregate.test.ts | Corrects typo from 'sci-fy' to 'sci-fi' |
| tests/accuracy/createIndex.test.ts | Updates test config to use previewFeatures: "vectorSearch" |
| tests/accuracy/createIndex.vectorSearchDisabled.test.ts | Updates test config to use previewFeatures: "" |
| tests/accuracy/dropIndex.test.ts | Updates test config to use previewFeatures: "vectorSearch" |
| tests/accuracy/dropIndex.vectorSearchDisabled.test.ts | Updates test config to use previewFeatures: "" |
| README.md | Adds documentation for the new previewFeatures configuration option |
| defaults: Partial<UserConfig>; | ||
| }): UserConfig { | ||
| const userConfig: UserConfig = { | ||
| const userConfig = { |
There was a problem hiding this comment.
The type annotation was removed from userConfig. This makes the code less maintainable as the return type is no longer explicitly documented at the declaration site. Consider restoring the type annotation: const userConfig: UserConfig = {
There was a problem hiding this comment.
[q] why are we removing this?
There was a problem hiding this comment.
It's already type asserted a few lines down, so I figured it's a bit redundant - const foo: Foo = { } as Foo is a bit repetitive. For context, we're not losing on type safety because we have the as UserConfig assertion anyway. That being said, since this is multiline, I can see how it might not be super obvious, so happy to bring it back if it's clearer.
Pull Request Test Coverage Report for Build 18780521612Details
💛 - Coveralls |
| ...DbOperationArgs, | ||
| indexName: z.string().nonempty().describe("The name of the index to be dropped."), | ||
| type: this.isFeatureFlagEnabled(FeatureFlags.VectorSearch) | ||
| type: this.isFeatureEnabled("vectorSearch") |
There was a problem hiding this comment.
the ultimate nit: why not keep vectorSearch as a const against typos or even maintainisVectorSerachFeatureEnabled at the same level as isFeatureEnabled?
another nit: btw I usually prefer to keeping a tiny file with this logic as it might grow and we could also add some unit tests just while we're here to keep it safe from future changes.
There was a problem hiding this comment.
"vectorSearch" is a const in a union type. So when calling isFeatureEnabled you can only provide one of the preview features we've defined in the previewFeatures enum in config.ts.
Not sure I fully understood the second half of your suggestion - what part of the logic would you suggest we move to a separate file? For context, the enum values are currently defined in config.ts along with the PreviewFeatures union type, while isFeatureEnabled is defined on the base tool type. We could move it to config.ts if that makes more sense, but I wanted to keep the user config as close to a data bag as possible, which is why I opted to define the isFeatureEnabled helper in the base tool.
There was a problem hiding this comment.
To add to this: enums in JavaScript are pretty terrible and it's best to avoid them.
https://jsdevspace.substack.com/p/shrink-typescript-code-replace-enums
Especially because of "erasable syntax" that can allow us to run TypeScript in Node directly:
https://www.totaltypescript.com/erasable-syntax-only
Probably something to introduce as a lint rule soon.
| defaults: Partial<UserConfig>; | ||
| }): UserConfig { | ||
| const userConfig: UserConfig = { | ||
| const userConfig = { |
There was a problem hiding this comment.
[q] why are we removing this?
Proposed changes
This change adds a
previewFeaturesconfig option that users can provide to enable non-GA functionality. For now, the only option isvectorSearch.