-
Notifications
You must be signed in to change notification settings - Fork 229
feat(compass-indexes): Add view support to indexes tab COMPASS-9667 #7182
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 adds view support to the indexes tab for MongoDB versions 8.0+ and 8.1+, enabling search index management on views with version-specific functionality. The implementation includes new banners, empty states, and conditional rendering based on MongoDB version and environment (Compass vs Data Explorer).
- Introduces version-specific banners and empty states for views with different MongoDB versions
- Adds conditional fetching and rendering of search indexes for readonly views
- Updates the indexes toolbar to support view-specific functionality and disable standard indexes tab for views
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.
Show a summary per file
File | Description |
---|---|
atlas-upgrade-cluster-link.ts |
New utility function to generate Atlas cluster upgrade links |
store.ts |
Updated search index fetching logic to support views on MongoDB 8.0+ |
search-indexes.ts |
Modified fetchIndexes to allow search index fetching for views on MongoDB 8.1+ |
view-version-incompatible-banners.tsx |
New component providing version-specific upgrade banners for views |
indexes.tsx |
Updated main component to handle view support with conditional rendering and banners |
indexes.spec.tsx |
Added test coverage for view support scenarios |
indexes-toolbar.tsx |
Enhanced toolbar with view-specific controls and automatic tab selection |
indexes-toolbar.spec.tsx |
Added tests for toolbar behavior with readonly views |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
packages/compass-indexes/src/components/indexes-toolbar/indexes-toolbar.tsx
Outdated
Show resolved
Hide resolved
...dexes/src/components/view-version-incompatible-banners/view-version-incompatible-banners.tsx
Outdated
Show resolved
Hide resolved
const mongoDBMajorVersion = parseFloat( | ||
serverVersion.split('.').slice(0, 2).join('.') | ||
); |
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.
We use semver package for handling mongodb version comparison, you can't really rely fully on the version here being fully parseable as a number (because it's not). There are some examples in the code here that already use semver for similar checks (see serverSupportsSearchIndexManagement
for example), please update your code to the similar pattern
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 me know if the way I changed this works! The majority of cases check 8.1+ but i had a few cases that differed so created a function that defaulted to 8.1+ but let me override which operator and comparison version.
useEffect(() => { | ||
// If the view is readonly, set the default tab to 'search-indexes' | ||
if (isReadonlyView && indexView !== 'search-indexes') { | ||
onIndexViewChanged('search-indexes'); // Update redux state | ||
} | ||
}, [indexView, isReadonlyView, onIndexViewChanged]); |
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.
Redux store owns this state and redux store reducer controls how exactly this state is set, this shouldn't be an effect in the view, please move this logic to the store
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 an initial state constant for views and modified the store initialization to have this when it's readOnly!
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.
General question: can you clarify why we are making a distinction in the verisons here for compass and for DE? They are ultimately connecting to the same clusters, so it doesn't look right to me that we are making a difference here between the two based on the server minor version
css, | ||
spacing, | ||
DropdownMenuButton, |
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.
Just a small note for the future, I'm not sure what's doing the re-ordering of the imports here, we don't have this style requirement in compass, but it makes reviewing this code way harder because it's not clear what exactly changed in the imports. If it's possible at all not to do, that would be very helpful 🙂
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, will keep this in mind. I think it's probably an intelllij setting I have set up that I can disable.
};*/ | ||
|
||
const MIN_VERSION_FOR_VIEW_SEARCH_COMPATIBILITY_COMPASS = '8.1.0'; | ||
export const compareVersionForViewSearchCompatibility = ( |
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 (see my other comment) we need so many different comparison methods, I think we should still keep the pattern of having focused, clearly named, check methods instead of a very generic compare
one, preferably explaining the reason why 🙂 Looking at current code, there is no way at all right now to say why exactly those arbitrary checks are used and this makes it very hard to maintain long term
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.
Got it! I'll introduce individual comparison methods/try to limit the comparisons I make.
Hi! So the reason is that in data explorer we are trying to provide cta for them to go to atlas search list and create indexes from there (The reason being is that we can create search indexes on atlas for 8.0+ while compass can only create indexes for 8.1+.) |
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.
Overall looks neat! Left a few notes, most importantly let's replace the button onclick handler with passing a href property
...dexes/src/components/view-version-incompatible-banners/view-version-incompatible-banners.tsx
Outdated
Show resolved
Hide resolved
@@ -162,8 +217,17 @@ export function Indexes({ | |||
} | |||
> | |||
<div className={indexesContainersStyles}> | |||
{!isReadonlyView && !enableAtlasSearchIndexes && ( | |||
<AtlasIndexesBanner | |||
{isReadonlyView && ( |
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.
Nit: doesn't need to addressed immediately, but I do think this code can be reworked a bit, at a glance it's now pretty tricky to read, there are overlapping states here that should never be rendered together, but the way the rendering part is written it's really hard to understand that without computing all the conditions in your head
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, i'll look into it again and see if I can make it more clear in a later pr.
@@ -78,6 +78,7 @@ | |||
"@mongodb-js/compass-workspaces": "^0.51.0", | |||
"@mongodb-js/mongodb-constants": "^0.12.2", | |||
"@mongodb-js/shell-bson-parser": "^1.2.0", | |||
"@mongodb-js/connection-info": "^0.17.1", |
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 usually should be a corresponding package-lock update. If you added this one manually, you can run npm install
on your machine to update the lockfile
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.
ran npm install and pushed!
Co-authored-by: Sergey Petushkov <[email protected]>
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.
Thanks for incorporating the requested changes, feel free to merge as soon as CI is green
@@ -43729,228 +43729,6 @@ | |||
"mocha": "^10.2.0" | |||
} | |||
}, | |||
"packages/bson-transpilers/node_modules/@typescript-eslint/eslint-plugin": { |
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.
Just wanted to note, this looks kinda unrelated, but this is okay to keep here in changes and merge: seems like this got in by accident with one of the recently merged PRs that was hanging in open state for too long, this is a harmless clean-up for package-lock (and we do have a CI task that validates that) even though it's not really related to your changes
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 someone gets to fixing this on main before you merge, this will probably cause a package-lock conflict, those are annoying to deal with, so the easiest thing to do is always to checkout package-lock from main and run install again instead of trying to resolve the conflict
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 very good to know, ty!
Description
Change indexes tab to support views. Figma
COMPASS-9667
compass_view_support_indexes.mov
Checklist
Motivation and Context
Open Questions
Dependents
Types of changes