Skip to content

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

Open
wants to merge 16 commits into
base: main
Choose a base branch
from

Conversation

paula-stacho
Copy link
Contributor

@paula-stacho paula-stacho commented Aug 13, 2025

Description

Adding a new collection flow.

  • Click the button in the toolbar
  • Drawer is opened and name input focused
  • Once the name is updated (on blur), the collection appears in the diagram and the form updates to a normal edit state

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

  • New tests and/or benchmarks are included
  • Documentation is changed or added
  • If this change updates the UI, screenshots/videos are added and a design review is requested
  • I have signed the MongoDB Contributor License Agreement (https://www.mongodb.com/legal/contributor-agreement)

Motivation and Context

  • Bugfix
  • New feature
  • Dependency update
  • Misc

Open Questions

Dependents

Types of changes

  • Backport Needed
  • Patch (non-breaking change which fixes an issue)
  • Minor (non-breaking change which adds functionality)
  • Major (fix or feature that would cause existing functionality to change)

@github-actions github-actions bot added the feat label Aug 13, 2025
@paula-stacho paula-stacho added the feature flagged PRs labeled with this label will not be included in the release notes of the next release label Aug 14, 2025
@paula-stacho paula-stacho marked this pull request as ready for review August 14, 2025 14:49
@Copilot Copilot AI review requested due to automatic review settings August 14, 2025 14:49
@paula-stacho paula-stacho requested a review from a team as a code owner August 14, 2025 14:49
@paula-stacho paula-stacho requested a review from Anemy August 14, 2025 14:50
Copy link
Contributor

@Copilot Copilot AI left a 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.

@Anemy Anemy changed the title feat: add collection COMPASS-9655 feat(data-modeling): add collection COMPASS-9655 Aug 14, 2025
@gribnoysup
Copy link
Collaborator

gribnoysup commented Aug 15, 2025

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)

@paula-stacho
Copy link
Contributor Author

paula-stacho commented Aug 15, 2025

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:

  • creating the collection immediately, as a <database>.new-collection-<number>
  • (optionally) the initial renaming will update the AddCollection edit, instead of creating a new edit

@gribnoysup
Copy link
Collaborator

gribnoysup commented Aug 15, 2025

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

(optionally) the initial renaming will update the AddCollection edit, instead of creating a new edit

Yeah, I think that's not a bad idea either if we want to try it out!

@Anemy
Copy link
Member

Anemy commented Aug 17, 2025

I like how we're now creating the collection immediately.
When we create the collection can we center it in the view port? We can use setCenter that useDiagram provides:
https://reactflow.dev/api-reference/types/react-flow-instance#setcenter
The export diagram model is already using the useDiagram for reference: https://github.com/mongodb-js/compass/blob/92d2dc7a0e0879b9f0ee5257336488b1e44dc120/packages/compass-data-modeling/src/components/export-diagram-modal.tsx#L68C19-L68C29

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']);
Copy link
Collaborator

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?)

Comment on lines 325 to 334
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,
};
};
Copy link
Collaborator

@gribnoysup gribnoysup Aug 19, 2025

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

Copy link
Collaborator

@gribnoysup gribnoysup Aug 19, 2025

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

@@ -193,6 +231,8 @@ export const diagramReducer: Reducer<DiagramState> = (
state.selectedItems,
action.edit
),
draftCollection:
Copy link
Collaborator

@gribnoysup gribnoysup Aug 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One weird thing about how this logic is currently wired together is that any edit will reset the "draft" state (made slightly worse / harder to track due to UI, not store, controling the logic for when the draft gets converted to real thing), meaning that it's relatively easy to get in a weird condition where draft in state is reset before first edit gets applied to it and it's not "draft" anymore (if your collection name is invalid, including empty, for example and you start doing something else on the screen).

This type of getting your state out of sync with reality usually indicates that we're not picking the correct way of storing this information, splitting it out from the actual source of truth, I wonder if this can be avoided if draft is just a metadata attached to the namespace in the model (an optional config flag on AddCollection maybe that we keep around?), that way we can make sure that we don't accidentally lose this state now and in the future

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm.. the point of this change was to have an actual namespace immediately. I'm not sure if we want to keep it as draft for too long - if the user first does a bunch of other edits and only then names the collection, then their history might as well reflect that - it would be weird if "undo" then skipped to the edits done before the naming.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we even need special draft handling then in the first place? If you didn't change the name, undo immediately removes it, if you did, it just gets you to a default one first, that's not really that different from what you're saying about other edits. I'm worried that this current implementation is disconnected in the code and there is no clear description to what's the expected behavior is, so either we should really carefully document it or just not spend time doing it if it's not a hard requirement from our perspective 🙂

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But also just to be clear, you can still achieve that behavior that you're describing withot disconnecting the "draft" state from the actual namespace, all this logic is basically just merging edits in stack in one very particular case (when you're action is rename edit and when the previous edit is create a draft), reducer is capable of doing this just fine just based on the current state and you have all this logic in one place

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, doing the latter now. I'm open to skipping the merging completely if @Anemy agrees, but now that it's only handled in the reducer maybe it's clear enough?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it should be, yeah!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think skipping it works, especially if it simplifies things. How it is now looks good too!

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a rule of thumb if we have a choice I'd suggest to err on the side of skipping stuff if things are simpler, even if we're otherwise can do something in the most idiomatic way, because there are always maintenance costs for adding more code 🙂

Copy link
Member

@Anemy Anemy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat feature flagged PRs labeled with this label will not be included in the release notes of the next release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants