-
Notifications
You must be signed in to change notification settings - Fork 239
feat(compass-collection): Connect schema redux state to modal and filter collections based on nesting depth - CLOUDP-342665 #7283
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.
Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.
| editViewName?: string; | ||
| sourcePipeline?: unknown[]; | ||
| onOpenMockDataModal: () => void; | ||
| hasData: boolean; |
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.
Rename hasData to hasSchemaAnalysisData and maxNestingDepth to something like analyzedSchemaDepth (see my other comment)
| return { | ||
| hasData: | ||
| schemaAnalysis.status === SCHEMA_ANALYSIS_STATE_COMPLETE && | ||
| schemaAnalysis.processedSchema && |
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] Remove the check for schemaAnalysis.processedSchema. The typing of schemaAnalysis dictates if the status is complete then processedSchema will always exist. This suggestion applies to schemaAnalysis.schemaMetadata as well
|
|
||
| const hasData = true; // TODO: CLOUDP-337090 | ||
| !sourceName && // sourceName indicates it's a view | ||
| maxNestingDepth < 4; // Filter out overly nested collections (4+ levels) |
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 name maxNestingDepth sounds like a configuration that should be holding the 4, but maxNestingDepth is computed from calculateSchemaDepth . It should be renamed to schemaDepth while the name maxNestingDepth should be re-purposed *(or renamed using "limit") to hold the magic value
edit*
...mpass-collection/src/components/collection-header-actions/collection-header-actions.spec.tsx
Outdated
Show resolved
Hide resolved
...mpass-collection/src/components/collection-header-actions/collection-header-actions.spec.tsx
Outdated
Show resolved
Hide resolved
|
|
||
| const hasData = true; // TODO: CLOUDP-337090 | ||
| !sourceName && // sourceName indicates it's a view | ||
| analyzedSchemaDepth < MAX_COLLECTION_NESTING_DEPTH; // Filter out overly nested collections |
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.
q - Do we care about observing when analyzedSchemaDepth < MAX_COLLECTION_NESTING_DEPTH ? If so, might be worth doing this check in the thunk and logging it 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.
Interesting thought. I can ask analytics
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.
In the meantime, if we do decide on this, how about we include this change as part of the analytics ticket?
|
|
||
| const hasData = true; // TODO: CLOUDP-337090 | ||
| !sourceName && // sourceName indicates it's a view | ||
| analyzedSchemaDepth < MAX_COLLECTION_NESTING_DEPTH; // Filter out overly nested collections |
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: max is an inclusive term so use <= and change to 3 instead
|
lgtm. did a line by line level review and tests seems consistent with the repo's code style. of course, deferring to |
|
|
||
| const hasData = true; // TODO: CLOUDP-337090 | ||
| !sourceName && // sourceName indicates it's a view | ||
| analyzedSchemaDepth <= MAX_COLLECTION_NESTING_DEPTH; // Filter out overly nested collections |
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.
Filters deeply nested collections: The Mock Data Generator button now only appears for collections with a maximum nesting depth of 3 levels or fewer. Collections with 4 or more levels of nested documents or arrays will not show the button
Have we considered instead keeping the button and having it disabled with a tooltip? As a user I could see it being confusing why its showing up on one collection and not another. Additionally if the button is showing and then vanishes.
I'm also a bit curious on this requirement, what is the cause of this limitation? Maybe we can mention that in a comment.
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.
Good question, this was something that I brought up to Designs as well, and where they decided to hide the button. (See Kevin's question about the same thing)
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.
Left a comment in that thread, I think we should bring it back up with design with the considerations. Right now this would be introducing some confusing UX for users and some UI that asynchronously disappears without indication.
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 the okay from design in the thread, not too sure on the text though, left a suggestion but feel free to interpret it in a way that makes sense to you!
At this time we are unable to generate mock data for collections with documents that have a deeply nested schema.
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.
Responded in the Figma thread with a few slight copy suggestions!
...es/compass-collection/src/components/collection-header-actions/collection-header-actions.tsx
Outdated
Show resolved
Hide resolved
| } | ||
| }; | ||
|
|
||
| void shouldRunSchemaAnalysis().then((shouldRun) => { |
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 want to cancel or skip the analysis if the tab is closed / plugin is deactivated and the cleanup method is called. One way other places in Compass do this is with an abort controller which is passed to methods that take it, like sample() in analyzeCollectionSchema(). We can use the addCleanup helper and abort then.
Here's an example:
| addCleanup(() => store.dispatch(cancelExportSchema())); |
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.
Nice! Thanks for also addressing the analysis being run without the experiment here!
Description
hasDatastate to collection-header-actions component to disable Mock Data Generator button for empty collectionsCollection Has Data - Button Displayed and Enabled
Empty Collection - Button Disabled
Button Opens Modal
Types of changes