-
Notifications
You must be signed in to change notification settings - Fork 229
chore(compass-preferences-model): use optInGenAIFeatures for Compass COMPASS-9593 #7129
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
base: main
Are you sure you want to change the base?
Conversation
)}`; | ||
} | ||
|
||
describe('Atlas Login', function () { |
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 could also skip these if re-introducing is likely enough.
I also assume we're not going to delete stores/atlas-login
.
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 think you should clean it up, but I don't think you necessarily need to do it in this PR. This plugin was the first attempt at this sort of hight level cross plugin interface and implementation and so contains some patterns that we moved away from, it's probably for the better if we can completely drop it from the codebase
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.
added #7150 to handle the cleanup
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.
Pull Request Overview
This PR standardizes the naming of the opt-in preference for AI features across Compass and Data Explorer by renaming optInDataExplorerGenAIFeatures
to optInGenAIFeatures
. This aligns the naming convention between the two products and makes the preference more generic and reusable.
- Renames preference key from
optInDataExplorerGenAIFeatures
tooptInGenAIFeatures
- Updates preference description to be more generic
- Changes default value from
true
tofalse
- Updates all references and tests to use the new preference name
Reviewed Changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
packages/compass-web/src/preferences.tsx | Updates preference name in default preferences object |
packages/compass-web/sandbox/sandbox-atlas-sign-in.tsx | Updates type definition and variable assignment for the new preference name |
packages/compass-web/sandbox/index.tsx | Updates preference name in sandbox configuration |
packages/compass-preferences-model/src/preferences-schema.tsx | Defines the new preference schema with updated name, description, and default value |
packages/compass-preferences-model/src/compass-web-preferences-access.ts | Updates editable preferences list with new name |
packages/compass-generative-ai/src/store/atlas-optin-reducer.ts | Updates preference checks and method calls to use new naming |
packages/compass-generative-ai/src/store/atlas-optin-reducer.spec.ts | Updates all test cases to use new preference and method names |
packages/compass-generative-ai/src/atlas-ai-service.ts | Refactors opt-in method and removes unnecessary imports |
packages/compass-generative-ai/src/atlas-ai-service.spec.ts | Adds comprehensive tests for the new opt-in method |
packages/compass-e2e-tests/tests/atlas-login.test.ts | Removes entire test file |
packages/compass-e2e-tests/tests/atlas-cloud/collection-ai-query.test.ts | Updates feature flag references to use new preference name |
// Performs a post request to Atlas to set the user opt in preference to true. | ||
await this.atlasService.authenticatedFetch( | ||
this.atlasService.cloudEndpoint( | ||
'settings/optInDataExplorerGenAIFeatures' |
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 URL path still references 'optInDataExplorerGenAIFeatures' but the preference has been renamed to 'optInGenAIFeatures'. Consider updating the API endpoint to match the new naming convention for consistency.
'settings/optInDataExplorerGenAIFeatures' | |
'settings/optInGenAIFeatures' |
Copilot uses AI. Check for mistakes.
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.
This is how we ensure backwards-compatibility
}, | ||
validator: z.boolean().default(true), | ||
validator: z.boolean().default(false), |
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.
Changing the default value from true
to false
is a breaking change that could affect existing users who had the feature enabled by default. This could disable AI features for users who were previously opted in.
validator: z.boolean().default(false), | |
validator: z.boolean().default(true), |
Copilot uses AI. Check for mistakes.
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.
This was likely an unintentional default which always gets overriden by the 'settings/optInDataExplorerGenAIFeatures'
fetch
}, | ||
validator: z.boolean().default(true), | ||
validator: z.boolean().default(false), |
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.
correct me if not so but this change shouldn't have any side effects on DE. The state of this was always being overriden by the API request and we'd likely not have wanted this to be true anyhow.
@@ -55,7 +55,7 @@ export function useCompassWebPreferences( | |||
enableShell: false, | |||
enableCreatingNewConnections: false, | |||
enableGlobalWrites: false, | |||
optInDataExplorerGenAIFeatures: false, | |||
optInGenAIFeatures: false, |
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.
Do we have a CLOUDP ticket to update where we pass this preference to CompassWeb initialPreferences
? https://github.com/10gen/mms/blob/8e00a3d4899e1f495b25ba30a22bb5d28ddd3bc4/client/packages/project/dataExplorerCompassWeb/router.tsx#L320
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 may not fully understand the flow but I think because we continue to both GET and POST to the old endpoints which use the MMS OPTED_INTO_GEN_AI_DATA_EXPLORER_FIELD
field, there's no changes needed on the cloud end. The naming is now slightly off on client but in practice we're still just consuming the old database field on DE.
https://github.com/10gen/mms/blob/677f241fef1153c7d28682c699cee7ef9b38af27/server/src/main/com/xgen/svc/mms/res/SettingsResource.java#L891
⬇️
https://github.com/10gen/mms/blob/677f241fef1153c7d28682c699cee7ef9b38af27/server/src/main/com/xgen/cloud/user/_private/dao/UserDao.java#L1075
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.
ohhh, I see this is about the client-side routing there, I didn't realize there's MMS react code for DE. Will look into this.
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.
actually I guess what I'm mentioning still stands: https://github.com/10gen/mms/blob/335c98b78c1b9e9dd8b4fd69efb2bc2170c843a5/client/packages/common/models/Settings.ts#L720
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.
If I'm understanding it correctly the name of the preference variable we pass to initialPreferences
does matter in how it's mapped to the Compass setting (purely on the frontend), we'll want to update that from optInDataExplorerGenAIFeatures
to optInGenAIFeatures
. It should typescript error when we update compass-web at least though.
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.
ah got it, yes the shared types aspect of it would be an issue. created CLOUDP-336196
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.
do we first need to i.e. merge this and then do a Compass dependency release and then bump it in compass-web and update there?
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.
Yup! Just making sure we had that planned. Changes here look good!
Aligns Compass and DE in their opt-in requirements for AI features. Feature flagged.