-
Notifications
You must be signed in to change notification settings - Fork 229
feat(compass-aggregations, compass-indexes): add confirm banner for edit pipeline and new banners COMPASS-9700 #7198
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
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.
Please use showConfirmation
method from @mongodb-js/compass-components
instead of this custom built confirmation modal logic
289879a
to
f5f1030
Compare
1f65dc7
to
894cb50
Compare
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 adds confirmation banners and validation for editing pipelines and creating search indexes on views. It introduces a new utility function to determine if a view's pipeline is compatible with search indexes and implements confirmation dialogs when modifying views that have existing search indexes.
- Adds
isPipelineSearchQueryable
utility function to validate view pipeline compatibility with search indexes - Implements confirmation dialogs when updating views with existing search indexes
- Disables search index creation buttons and shows warnings for incompatible view pipelines
Reviewed Changes
Copilot reviewed 16 out of 17 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
packages/compass-utils/src/view-search-queryable.ts | New utility function to check if a pipeline is search index compatible |
packages/compass-utils/src/view-search-queryable.spec.ts | Test coverage for the new utility function |
packages/compass-utils/src/index.ts | Exports the new utility function |
packages/compass-utils/package.json | Adds mongodb dependency for Document type |
packages/compass-indexes/src/modules/collection-stats.ts | Extends collection stats to include pipeline data |
packages/compass-indexes/src/components/view-version-incompatible-banners/ | Adds test ID to existing banner component |
packages/compass-indexes/src/components/search-indexes-table/ | Adds pipeline validation and disables buttons for incompatible views |
packages/compass-indexes/src/components/indexes/ | Implements new banner for pipeline incompatibility warnings |
packages/compass-indexes/src/components/indexes-toolbar/ | Disables create index button for incompatible pipelines |
packages/compass-indexes/package.json | Adds compass-utils dependency |
packages/compass-aggregations/src/modules/update-view.ts | Implements confirmation dialog when updating views with search indexes |
packages/compass-aggregations/src/modules/update-view.spec.ts | Test coverage for update view confirmation logic |
packages/compass-aggregations/src/modules/search-indexes.ts | Adds optional namespace parameter to fetchIndexes function |
Comments suppressed due to low confidence (1)
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
@@ -107,6 +112,25 @@ export const updateView = (): PipelineBuilderThunkAction<Promise<void>> => { | |||
getState(), | |||
pipelineBuilder | |||
); | |||
|
|||
// confirmation banner if indexes present | |||
await dispatch(fetchIndexes(viewNamespace)); |
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 dispatch will immediately resolve if you're still in the process of fetching the indexes, so the way it is currently implemented you can't rely on this fetch to ensure that indexes are fetched and stored in the state
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.
Based on what you're trying to do re-using the same action for this just generally doesn't sound correct to me. This will change the stored search indexes for the whole plugin state, not only for this one call, which I think is what you're trying to do here?
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 implemented a seperate method that just returns whether indexes exist for a certain namespace. Is this fine? I'm only using the search indexes to decide whether to display confirmation or not.
Description
When the user clicks edit pipeline, we will show a confirmation popup if they have search indexes. Popup will look different depending on whether the view is search queryable or not.
We also display banner if it is search queryable or not in indexes page.
confirmation.mov
Checklist
Motivation and Context
Open Questions
Dependents
Types of changes