-
Notifications
You must be signed in to change notification settings - Fork 238
chore(my-queries): make feature flag ready for saving user data COMPASS-9657 #7264
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
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.
Pull Request Overview
This PR introduces a feature flag MY_QUERIES_DATA_EXPLORER
to control access to user query data features. The feature flag gates functionality related to saved queries, aggregations, recent queries autocomplete, and favorite queries/aggregations.
- Adds the new feature flag to preferences schema and default configurations
- Creates a dedicated hook
useMyQueriesFeature
for checking feature availability - Integrates the feature flag into UI components to conditionally enable My Queries functionality
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
packages/compass-web/src/preferences.tsx | Sets default value for MY_QUERIES_DATA_EXPLORER feature flag to false |
packages/compass-web/src/hooks/use-my-queries-feature.ts | Creates new hook to check if My Queries feature is enabled |
packages/compass-web/src/entrypoint.tsx | Imports and uses the new feature flag hook (for future implementation) |
packages/compass-web/sandbox/index.tsx | Adds feature flag configuration to sandbox environment |
packages/compass-sidebar/src/components/multiple-connections/navigation/navigation.tsx | Gates My Queries navigation item based on feature flag |
packages/compass-query-bar/src/components/query-bar.tsx | Conditionally enables saved queries functionality based on feature flag |
packages/compass-preferences-model/src/preferences-schema.tsx | Adds MY_QUERIES_DATA_EXPLORER to preferences type definitions and schema validation |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
packages/compass-sidebar/src/components/multiple-connections/navigation/navigation.tsx
Outdated
Show resolved
Hide resolved
onDebug, | ||
}); | ||
const preferencesAccess = useCompassWebPreferences(initialPreferences); | ||
const isMyQueriesEnabled = useMyQueriesFeature(); |
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.
lint will complain about unused variables, you can just leave this commented out together with the todo. we also try to add ticket numbers to todos, I don't think it'll be forgotten in this instance but it's a good habit
export function useMyQueriesFeature(): boolean { | ||
const cloudFeatureRolloutAccess = usePreference('cloudFeatureRolloutAccess'); | ||
return cloudFeatureRolloutAccess?.MY_QUERIES_DATA_EXPLORER ?? 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.
compass-web
is not the best place for the hook, check out the compass-preferences-model
package. there are similar hooks in the utils
.
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.
once moved, you can use the hook in the sidebar and query bar
enableDataModeling: true, | ||
cloudFeatureRolloutAccess: { | ||
MY_QUERIES_DATA_EXPLORER: false, // Disabled by default | ||
}, |
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 shouldn't be necessary
What happened, where did the changes in In the end, you can test your changes locally with Feel free to hit me on slack or schedule a pairing session |
description: { | ||
short: 'Save and manage favorite queries and aggregations', | ||
}, | ||
}, |
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.
so this part is a bit confusing, but this configuration and feature flags relate to compass, not DE. so adding it here with stage: 'development' adds it to these "feature preview" flags that we have in Compass. They are also turned off by default, which we don't want here.

So instead of adding it to FeatureFlags
, let's add it to the "other" section in UserConfigurablePreferences
and storedUserPreferencesProps
. Example you can follow is enableImportExport
(make sure to use default true, you can still override that for compass-web, but the default compass setting for this feature is on).
</div> | ||
</CollectionTabsProvider> | ||
</WorkspacesProvider> | ||
<PipelineStorageProvider value={pipelineStorage}> |
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.
placeholder until the implementation PR goes in (COMPASS-9565)
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.
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.
Let's add a TODO or note? Also fine as is as I doubt anyone else will be touching it.
Giving this some thought, we are constructing the file storage items here, it could lead to some unintended errors or something if we change the file storage implementation. Similarly compassFavoriteQueryStorageAccess
is only exported for testing. Should we leave these out until we have the web storage? Is there something we want them for now?
Slightly related, a total nit, and not something we follow in many places of Compass really:
https://react.dev/reference/react/useRef#avoiding-recreating-the-ref-contents
We could instead write the creation as:
// Create a simple pipeline storage instance for sandbox.
const pipelineStorageRef = useRef<CompassPipelineStorage | null>(null);
if (pipelineStorageRef.current === null) {
pipelineStorageRef.current = new CompassPipelineStorage();
}
to avoid calling the constructor of CompassPipelineStorage
on each render.
// TODO: remove direct check for storage existing, breaks single source of | ||
// truth rule and exposes services to UI, this breaks the rules for locators | ||
const isSavingAggregationsEnabled = !!usePipelineStorage(); | ||
const pipelineStorageAvailable = !!usePipelineStorage(); |
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 not mistaken we can now replace this check with the new flag. tagging @gribnoysup to confirm that the check was for compass-web and not some other case.
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.
Yep, should replace this with a preference if we have one now
// TODO: remove direct check for storage existing, breaks single source of | ||
// truth rule and exposes services to UI, this breaks the rules for locators | ||
const enableSavedAggregationsQueries = !!usePipelineStorage(); | ||
const pipelineStorageAvailable = !!usePipelineStorage(); |
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.
same as above
RecentQueryStorageProvider, | ||
PipelineStorageProvider, | ||
} from './provider'; | ||
|
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 provider being exported separately is actually intentional, this is to have a clear separation between the service and the provider (we even have a rule that restricts direct use of the service).
You can import these from @mongodb-js/my-queries-storage/provider
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.
Changes look good, left a couple small comments/questions
"@mongodb-js/connection-info": "^0.18.0", | ||
"@mongodb-js/mongodb-constants": "^0.14.0", | ||
"bson": "^6.10.1", | ||
"bson": "^6.10.4", |
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.
Is this related?
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.
oops taken out now
"@mongodb-js/compass-utils": "^0.9.12", | ||
"write-file-atomic": "^5.0.1", | ||
"zod": "^3.25.17" | ||
"zod": "^3.25.76" |
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.
Same q as the bson
dep, totally for my own understanding, happy to have drive by updates in prs, although it can make it easier if we have them in a separate one to make things simple if we ever need to revert.
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.
oops taken out now
Body, | ||
css, | ||
openToast, | ||
resetGlobalCSS, |
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.
Alphabetical 🙌 big fan.
</div> | ||
</CollectionTabsProvider> | ||
</WorkspacesProvider> | ||
<PipelineStorageProvider value={pipelineStorage}> |
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.
Let's add a TODO or note? Also fine as is as I doubt anyone else will be touching it.
Giving this some thought, we are constructing the file storage items here, it could lead to some unintended errors or something if we change the file storage implementation. Similarly compassFavoriteQueryStorageAccess
is only exported for testing. Should we leave these out until we have the web storage? Is there something we want them for now?
Slightly related, a total nit, and not something we follow in many places of Compass really:
https://react.dev/reference/react/useRef#avoiding-recreating-the-ref-contents
We could instead write the creation as:
// Create a simple pipeline storage instance for sandbox.
const pipelineStorageRef = useRef<CompassPipelineStorage | null>(null);
if (pipelineStorageRef.current === null) {
pipelineStorageRef.current = new CompassPipelineStorage();
}
to avoid calling the constructor of CompassPipelineStorage
on each render.
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.
lgtm 1 (once tests pass) !
…s into user-data-feature-flag
Description
Make feature flag to gate user queries. Once this is merged we will use this feature flag in the implementation PR
Checklist
Motivation and Context
Open Questions
Dependents
Types of changes