-
Notifications
You must be signed in to change notification settings - Fork 229
feat(data-modeling): add collection COMPASS-9655 #7195
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 functionality to create new collections directly from the data modeling diagram editor. Users can click an "Add Collection" button in the toolbar to open a drawer with a focused name input field, and when the name is updated (on blur), the collection appears in the diagram and transitions to a normal edit state.
Key changes:
- Added an "Add Collection" button to the diagram editor toolbar
- Implemented collection creation flow with drawer UI that focuses the name input for new collections
- Added state management for collection creation with proper validation and positioning logic
Reviewed Changes
Copilot reviewed 13 out of 14 changed files in this pull request and generated 3 comments.
Show a summary per file
File | Description |
---|---|
packages/compass-e2e-tests/tests/data-modeling-tab.test.ts | Adds end-to-end test for the new collection creation workflow |
packages/compass-e2e-tests/helpers/selectors.ts | Updates selectors to support the new Add Collection button |
packages/compass-data-modeling/src/store/diagram.ts | Implements core state management for collection creation including new actions, reducers, and positioning logic |
packages/compass-data-modeling/src/store/diagram.spec.ts | Adds unit tests for the collection creation flow |
packages/compass-data-modeling/src/services/data-model-storage.ts | Extends the edit schema to support AddCollection operations |
packages/compass-data-modeling/src/components/icons/add-collection.tsx | Creates custom SVG icon for the Add Collection button |
packages/compass-data-modeling/src/components/drawer/diagram-editor-side-panel.tsx | Updates drawer to handle new collection state with optional IDs and appropriate labeling |
packages/compass-data-modeling/src/components/drawer/collection-drawer-content.tsx | Modifies collection form to support creation mode with auto-focus and disabled controls |
packages/compass-data-modeling/src/components/diagram-editor.tsx | Integrates collection creation with drawer opening functionality |
packages/compass-data-modeling/src/components/diagram-editor-toolbar.tsx | Adds the Add Collection button to the toolbar UI |
packages/compass-data-modeling/package.json | Updates diagramming library dependency for new positioning functionality |
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/collection-drawer-content.tsx
Outdated
Show resolved
Hide resolved
packages/compass-data-modeling/src/components/drawer/collection-drawer-content.tsx
Show resolved
Hide resolved
Feel free to tell me if you discussed this with design already and it's decided upon 😆 but I do find it a bit weird when playing with the branch locally that new collection doesn't appear immediately when adding it. Everything else we have in the view is immediately synced to the diagram and this specific action is not (but that might be just me) |
It's something we've discussed with Rhys, I'm actually not sure if the behaviour was reviewed by Ben. I don't find the delayed appearance of the collection on the diagram that weird, but tbh because of the problem with placement it's usually underneath the drawer for me 🙈 . We can check with Ben when he's back next week, it'll also be easier to reason about now that we have a prototype. I'd like to keep the creation+naming as one edit, but there are other ways to achieve that. One idea, connecting this with the other (missing id) point:
|
Like your suggestion of using a placeholder (new-collection-n) name! I think that's a pretty reasonable way to deal with that and should be easy to iterate on later
Yeah, I think that's not a bad idea either if we want to try it out! |
I like how we're now creating the collection immediately. |
packages/compass-data-modeling/src/components/drawer/collection-drawer-content.tsx
Outdated
Show resolved
Hide resolved
packages/compass-data-modeling/src/components/drawer/collection-drawer-content.tsx
Show resolved
Hide resolved
packages/compass-data-modeling/src/components/drawer/collection-drawer-content.tsx
Show resolved
Hide resolved
expect(selectedFields.collection1).to.deep.include(['prop2', 'prop2A']); | ||
expect(selectedFields.collection1).to.deep.include([ | ||
expect(selectedFields).to.have.property('db.collection1'); | ||
expect(selectedFields['db.collection1']).to.deep.include(['prop1']); |
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.
Probably not super actionable in the context of this PR, but it could be nice if all collection names were typed as `${string}.${string}`
instead of just string
. That may feel a bit silly, but it does clarify in particular there we don't need to worry about conflicts with built-in methods from Object.prototype
such as hasOwnProperty
(which, as far as I can tell, was a bug that got "accidentally" fixed by this PR?)
packages/compass-data-modeling/src/components/diagram-editor.tsx
Outdated
Show resolved
Hide resolved
943c31b
to
dd07568
Compare
export const useDrawerState = () => { | ||
const drawerToolbarContext = useDrawerToolbarContext(); | ||
const drawerState = useContext(DrawerStateContext); | ||
return { | ||
isOpen: | ||
drawerToolbarContext.isDrawerOpen && | ||
// the second check is a workaround, because LG doesn't set isDrawerOpen to false when it's empty | ||
drawerState.length > 0, | ||
}; | ||
}; |
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 we have to add this, we have to do more wiring to actually make it work in our paradigm, this hook will only work if you're using this INSIDE leafygreen layout provider (because that's the only place where useDrawerToolbarContext is available), but not inside our drawer provider, which will not be always true in our case. If you want to add a hook like that you have to first move this isDrawerOpen
state higher up to our provider and re-expose it from 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.
Should be easier when this is implemented and we can just manage this state on our own, but fot now this needs more logic from us. Also worth adding a test for this
packages/compass-data-modeling/src/components/drawer/collection-drawer-content.tsx
Outdated
Show resolved
Hide resolved
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!
71059bb
to
df0db50
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.
I had a few small comments here and there, but otherwise looks good to me
packages/compass-components/src/components/drawer-portal.spec.tsx
Outdated
Show resolved
Hide resolved
@@ -183,6 +188,35 @@ const DiagramContent: React.FunctionComponent<{ | |||
}); | |||
}, []); | |||
|
|||
// Center on a new collection when it is added | |||
const previouslyOpenedDrawer = useRef<boolean>(false); | |||
useEffect(() => { |
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 know I will never convince y'all to avoid useEffect usage as much as possible 😆 but I just have to mention that while yes, this is an effect of sorts, I think a better way to think about this as (at least as it is right now) a side-effect directly of calling the createNewCollection action and so can be just a function that is called right after the onAddCollectionClick
callback prop
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.
hmm, do we know that the new collection will already be there?
Description
Adding a new collection flow.
Note: The coordinates calculation doesn't work as expected, I am still trying to figure out why I'm getting different numbers in compass.
Note2: One todo left around the model.database addition, this might turn into a follow up
Screen.Recording.2025-08-14.at.16.48.06.mov
Checklist
Motivation and Context
Open Questions
Dependents
Types of changes