-
Notifications
You must be signed in to change notification settings - Fork 244
feat(data-modeling): field sidebar COMPASS-9659 #7218
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
| @@ -0,0 +1,128 @@ | |||
| import React from 'react'; | |||
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.
taken from the collection-drawer content
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 implements a read-only field drawer component as part of a larger field sidebar feature. The implementation enables users to select fields in the data modeling diagram and view their properties and relationships in a side panel.
- Adds schema traversal utilities for navigating MongoDB JSON schemas
- Implements field selection functionality and drawer UI
- Refactors relationship sections for reuse between collections and fields
Reviewed Changes
Copilot reviewed 14 out of 15 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
data-model-with-relationships.json |
Updates test fixture schema structure for testing |
schema-traversal.tsx |
Adds utilities for traversing schemas and finding specific fields |
schema-traversal.spec.tsx |
Comprehensive tests for schema traversal functionality |
nodes-and-edges.tsx |
Refactors field extraction to use new traversal utilities |
nodes-and-edges.spec.tsx |
Updates tests to match new field data structure |
diagram.ts |
Adds field selection state management and actions |
data-model-storage.ts |
Defines field path types and edit operation schemas |
relationships-section.tsx |
Extracts reusable relationship section component |
field-drawer-content.tsx |
Implements field drawer with properties and relationships |
diagram-editor-side-panel.tsx |
Integrates field drawer into main side panel |
diagram-editor-side-panel.spec.tsx |
Tests for field drawer functionality |
collection-drawer-content.tsx |
Refactors to use shared relationship section |
diagram-editor.tsx |
Adds field click handling in diagram |
package.json |
Updates diagramming package dependency |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
packages/compass-data-modeling/src/components/drawer/field-drawer-content.tsx
Show resolved
Hide resolved
| </DMFormFieldContainer> | ||
|
|
||
| <DMFormFieldContainer> | ||
| <Combobox |
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.
I have this as a todo (on the PR, hoping to resolve it with this one), so far I have no idea why it's jumping like that
packages/compass-data-modeling/src/components/drawer/field-drawer-content.tsx
Outdated
Show resolved
Hide resolved
packages/compass-data-modeling/src/components/drawer/field-drawer-content.tsx
Outdated
Show resolved
Hide resolved
9621485 to
7078026
Compare
gribnoysup
left 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.
Some non-blocking nits, but otherwise looks good to me.
| children.push(field); | ||
| } | ||
| if (field.items) { | ||
| children.push((field.items as MongoDBJSONSchema).anyOf || field.items); |
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: seems a bit redundant to check for anyOf here as your traversal function will handle it anyway when you're going through the children (you're also not doing this in other cases where it can happen, so for consistency feels better better not to do it here either)
| children.push((field.items as MongoDBJSONSchema).anyOf || field.items); | |
| children.push(field.items); |
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.
What other cases do you have in mind?
In this case we go two levels in (field.items.anyOf), because that's where the children are (example { friends: ['peter', { name: 'John', alias: 'Johny' } ] } --> name and alias are the children of friends). Same happens in the other case where the children are variant.items which translates to field.anyOf[].items. It looks like it's skipping a step but the path only increases by one step.
I'm not sure if I'm explaining it well. You can also check out the an array of mixed items (including objects) test.
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, I actually missed that in your code and made a comment assuming it will cover anyOf as part of traversal, but it only covers it for fields of children. You're kinda artificially "skipping" a level here looking into the next row of items, this is not bad, but just sliiightly unconventional for recursive functions to work that way, you'd usually want them to go through the levels in a more uniformed manner applying the same rules to all calls, although again, not the worst thing for our specific scenario. In particular here it causes a bug where traversal doesn't happen if your top level jsonSchema is just anyOf (and this is a valid schema based on types).
I would suggest to move anyOf handling at the very top of your traverse, then you are "unwrapping" it before all the other code and you don't need to deal with it separately for fields and items, but also feel free to consider this a corner case, maybe let's just document that this functino doesn't work for this 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.
yeah I realised I'm making an implicit assumption that the first call is on a collection schema. In the update function I'm avoiding this assumption, perhaps I can refactor these two before moving them to the lib.
|
Note: The combobox jumps are a problem with the LG component, created https://jira.mongodb.org/browse/LG-5469 |

Description
This is getting big, so let's do it in steps:
This part includes:
traverseSchemawith a visitor is extracted from the existing codegetFieldFromSchemafollows a given path to get a fieldWe will need at least one similar method for making updates. I plan to move the methods to
@mongodb-js/mongodb-schema, but to speed things up and have easier time testing, I want to finish the functionality first and only move them once it's all settled.TODO:
Checklist
Motivation and Context
Open Questions
Dependents
Types of changes