Skip to content

feat(data-modeling): integrate diagramming package COMPASS-9357 #6979

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 35 commits into
base: main
Choose a base branch
from

Conversation

mabaasit
Copy link
Contributor

@mabaasit mabaasit commented Jun 2, 2025

Integrated diagramming package in data-modeling to visualize the schema.

Preview
data-modeling-diagram.mov

Description

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 Jun 2, 2025
@mabaasit mabaasit added the no release notes Fix or feature not for release notes label Jun 2, 2025
@mabaasit
Copy link
Contributor Author

mabaasit commented Jun 2, 2025

i have no idea what exploded lock file

source: source.ns,
target: target.ns,
markerStart: 'one',
markerEnd: 'many',
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this a placeholder item?

Something that's become fairly clear over time is that we're likely going to want to work with concrete cardinalities if we can (i.e. number ranges rather than one or many)

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, actually it needs to be like that. I'll change this to reflect the store cardinalities. My assumption is that we will be having 1 or 2 as numeric representation for one and many respectively.

Copy link
Collaborator

Choose a reason for hiding this comment

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

So ... what we'll ultimately have is descriptions like "1 to 4–6" or something along those lines, at least if we ultimately want to give schema recommendations (still a goal for Compass to have that eventually), because it's become pretty clear that the numeric values of the relative cardinality ranges matter for those (you maybe be able to create an embedding of collections in a 1:5 case but not in a 1:200 case)

? 'Unknown'
: typeof field.bsonType === 'string'
? field.bsonType
: field.bsonType[0];
Copy link
Collaborator

Choose a reason for hiding this comment

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

This line is also some sort of TODO?

@addaleax
Copy link
Collaborator

addaleax commented Jun 2, 2025

i have no idea what exploded lock file

Looking through it, it mostly seems to be additions of genuine new dependencies?

Comment on lines 255 to 256
"configs/eslint-config-compass/node_modules/eslint": {
"version": "8.57.1",
Copy link
Collaborator

@gribnoysup gribnoysup Jun 2, 2025

Choose a reason for hiding this comment

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

Seems like something went really wrong when you installed a new dep, you now have two versions of eslint in package-lock, one of them is not matchin the required one. This is partially why package-lock blew up so much and why CI is failing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, i finally resolved this, thank you. I tried checking out lock file from main couple of times and was not sure what I am missing. Eventually from your other comment realized its origin that I am missing.

Copy link
Collaborator

@gribnoysup gribnoysup left a comment

Choose a reason for hiding this comment

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

Can you try running

git checkout origin/main -- package-lock.json
npm install 

on your branch? This gives a pretty different result for package-lock at least on my machine (without eslint being messed up)

// to React 18, but for the time being we will just resolve react to
// actual files ourselves to work around that
'react/jsx-runtime': require.resolve('react/jsx-runtime'),
react: require.resolve('react'),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Probably should add this to ignore in depcheckrc file, we do this already for another package that we just need to resolve

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done in bba2841

Copy link
Contributor Author

Choose a reason for hiding this comment

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

with support for cjs, i ended up removing this.

@mabaasit mabaasit marked this pull request as ready for review June 2, 2025 19:42
@mabaasit mabaasit marked this pull request as draft June 2, 2025 19:43
title={diagramLabel}
edges={edges}
nodes={nodes}
onEdgeClick={(evt, edge) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we'll be doing removal like this, but it's actually neat for now :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah i wanted to retain the current functionality so ended up adding this.

@mabaasit mabaasit marked this pull request as ready for review June 11, 2025 06:18
Comment on lines +46 to +47
minute=$(escapeLeadingZero "${ts[4]}")
second=$(escapeLeadingZero "${ts[5]}")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Small drive-by because CI failed for this PR in this run.

When hour is 00 and minutes < 10, the version is as yymmdd-dev.0.., which is invalid

Comment on lines 141 to 148
useEffect(() => {
if (
process.env.APP_ENV === 'webdriverio' ||
process.env.NODE_ENV === 'development'
) {
(window as any).diagramInstance = diagram;
}
}, [diagram]);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Instead of using global window and gating it to these very specific checks (that we really truly should try to avoid), let's do something similar to what we do in codeedior and attach the diagram to the relevant DOM node that we can find in e2e tests, something like:

<div
  data-testid="diagram-editor-container"
  ref=(ref => {
    if (ref) {
      ref.__diagram = diagram;
    }
  })

Comment on lines 21 to 37
async function getDiagramNodes(browser: CompassBrowser) {
return await browser.execute(() => {
// eslint-disable-next-line no-restricted-globals
if ('diagramInstance' in window) {
// eslint-disable-next-line no-restricted-globals
const diagramInstance = window.diagramInstance as DiagramInstance;
const nodes = diagramInstance.getNodes();
return nodes.map((node) => ({
id: node.id,
title: node.data.title,
}));
}
throw new Error(
'Diagram instance not found in the window object. Ensure the diagram is set for tests.'
);
});
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can't say I have the strongest preference, but these are rendering real DOM nodes on the screen and we do expect them to be visible (and so accessible through the normal wdio APIs). Are there any issues with testing them without relying on internals of the library? These are e2e tests after all

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Currently we are not positioning these on the viewport correctly (the nodes are randomly placed) and with virutalization enabled for diagramming, they many not be rendered. So I tried to use this approach.

I think for tests we may want to disable virtualization so that everything is rendered and we don't need to do this kinda stuff or even using ref to access diagram. I added COMPASS-9447 to export virtualization flag from diagramming package and once that's done, I can do a followup on this and clean it completely.

Let me know what you think about it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(maybe with failing tests on mac, i need to revisit this whole thing in this PR actually)

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think noticeably changing how rendering works there only for e2e tests is a good idea, this should stay as close to real app as possible.

For visualization we should make sure that elements are positioned inside the viewport by default, right? So maybe that's where we can test without using internals, otherwise using them is probably fine, I would encourage you to separate the cases where it makes sense to use one or another. Consider what we do for the editor, if we want to validate that user actually sees something we should be testing through UI and wdio feature that mimic user interactions, but if we want to validate that something inside the editor is set correctly (but not necessarily visible) accessing the internals to get the value is fine, I'd suggest to keep this sort of rule of thumb in mind.

Copy link
Contributor Author

@mabaasit mabaasit Jun 12, 2025

Choose a reason for hiding this comment

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

Correct they should by default. Now I am not sure how we are planning to do that (to position nodes correctly), or how we plan to deal with diagram with many nodes. But @xyflow/react does offer fitView option so that everything is rendered on the viewport (it changes zoom based on the content). With that in mind, I'll make that change so that content is visible.

Yeah, I dont think we want to test the internals, but rather what we expect user to see and for that I think we can do simple assertions without any access to reactflow instance.

Made change in bed5629 (#6979)

Copy link
Collaborator

@gribnoysup gribnoysup left a comment

Choose a reason for hiding this comment

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

The integration part itself looks good for the scope, I have some questions about how we're e2e testing this, but I'll also approve and maybe consider addressing these things separately if CI is green so that we can unblock other work

@mabaasit mabaasit requested a review from a team as a code owner June 12, 2025 15:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat no release notes Fix or feature not for release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants