feat: apply initial layout COMPASS-9432 #7047

paula-stacho · 2025-06-20T11:33:27Z

Description

The new diagram is created at [-1, -1]
The editor uses applyLayout from the diagramming lib to position the nodes
The initial layout is saved to the store and storage, overriding the initial edit
Once the nodes are ready, the diagram is fit to view

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)

paula-stacho · 2025-06-20T16:47:32Z

The layouts work much better now with the dimensions fixed 🎉. But still not exactly as imagined. For example the screenshot is LEFT_RIGHT - I thought this would spread to more columns. STAR creates one column only. Are we doing something wrong or is this expected result? @lchans

gribnoysup · 2025-06-23T11:58:46Z

packages/compass-data-modeling/test/setup-store.tsx

+    expect(store.getState().diagram?.edits.current).to.have.lengthOf(1);
+  });
+  const renderResult = renderWithConnections(
+    <Provider store={store}>{component}</Provider>,


We have helpers for this in compass-testing-library, I strongly suggest you take a look at those. We added these specifically so that we don't need to spend time for similar setup all the time and so that making changes to the plugin internals is easier in the long run (otherwise every time we tinker with plugin system ALL tests might need to be updated accordingly). If those doesn't cover some case you are trying to cover here, please shout this out so that we can provide tools covering those and keep the benefits of a shared testing harness

@mabaasit FYI as you added some helpers here initially.

There was an issue i was facing when setting this up, but i don't remember exactly what it was (my guess is TestMongoDBInstanceManager props were not being picked up and it does not support multiple connections). As a result, I had to add testConnections in this file and mock connections implementation.

Updated the tests helpers in this file, haven't run into any problems here.

For now I avoided the problem with store not being fit for that view by replacing the error by empty result. Possibly something we could use in the future - to allow some preparation of the store before rendering the component. We could also provide initial store state but seems more natural (and easier to maintain) to get there with actions.

gribnoysup · 2025-06-23T12:30:34Z

packages/compass-data-modeling/src/components/diagram-editor.tsx

+      void applyInitialLayout(storedNodes);
+      return;
+    }
+    setNodes(storedNodes);


The state itself is not an effect here, this is state derived from model, only applying initial layout is an effect

So this used to be a memo, I changed it to effect so that unpositioned nodes can be ignored (setNodes is not called in that case). Is it too weird? I was considering having a memo + boolean 'areNodesReady' tag as an alternative

Yeah, it's almost always not a good idea to duplicate props to local component state through effects, especially when you have a clear way to derive this, so much so that React has a documentation for that:

When something can be calculated from the existing props or state, don’t put it in state. Instead, calculate it during rendering.

So nodes + areNodesReady + effect that depends on areNodesReady is definitely a cleaner way to do this.

There is honestly an argument to be made about moving this logic to the action itself as it doesn't really depend on UI (meaning actual DOM), is not really doing any application (more specifically it's not really an effect as compared to fitToView for example) just calculates the values for the state that is already in the store and so splitting this logic between React and Redux doesn't give us any meaningful separation of concerns here, while requiring us to jump through some unnecessary hoops to get the state in the right shape. This is a bit of splitting hairs thing, so I think it's fine if we keep it as is and just remove the unnecessary UI state from the equasion.

packages/compass-data-modeling/src/components/diagram-editor.tsx

gribnoysup · 2025-06-23T12:38:48Z

packages/compass-data-modeling/src/components/diagram-editor.tsx

+          'LEFT_RIGHT'
+        );
+        setNodes(positionedNodes);
+        onApplyInitialLayout(


We are supposed to also fit diagram into view after initial layout gets applied, right? I'm not seeing this happening at the moment

Thank you, added!

addaleax · 2025-06-25T13:58:33Z

packages/compass-data-modeling/src/components/diagram-editor.spec.tsx

+  },
+};
+(require.cache[require.resolve('@mongodb-js/diagramming')] as any).exports =
+  mockDiagramming;


I guess this isn't possible with something like sinon? Would it be possible/make sense to add a TODO to make the diagramming package itself allow mocking the relevant parts à la dependency injection?

It's a bit unfortunate to have this here, especially because it only targets a single test file right now, but all test files in the same mocha invocation will have these mocks in place

Ah. I was writing to explain that sinon wouldn't be enough because there was a problem with ESM/CJS interop. I actually spent some time trying to get a test setup that would import the Diagram correctly before I decided that mocks are fine as we cover the integration in e2e tests.
But as I was writing it hit me that even though the cause was the messed up import, no error was thrown until the imported piece was being used. So taking over the require was an overkill.
Tldr: actually sinon works, thanks for making me rethink!

packages/compass-data-modeling/src/components/diagram-editor.tsx

Co-authored-by: Anna Henningsen <[email protected]>

github-actions bot added the feat label Jun 20, 2025

paula-stacho added the feature flagged PRs labeled with this label will not be included in the release notes of the next release label Jun 20, 2025

paula-stacho force-pushed the COMPASS-9432 branch from 783340f to 710bb2e Compare June 20, 2025 14:51

paula-stacho force-pushed the COMPASS-9432 branch from 9f586d2 to 11bfe02 Compare June 20, 2025 16:48

paula-stacho marked this pull request as ready for review June 20, 2025 16:48

paula-stacho requested a review from a team as a code owner June 20, 2025 16:48

paula-stacho force-pushed the COMPASS-9432 branch from 036be24 to a4d96d8 Compare June 23, 2025 11:45

gribnoysup reviewed Jun 23, 2025

View reviewed changes

packages/compass-data-modeling/src/components/diagram-editor.tsx Outdated Show resolved Hide resolved

gribnoysup reviewed Jun 23, 2025

View reviewed changes

packages/compass-data-modeling/src/components/diagram-editor.tsx Outdated Show resolved Hide resolved

gribnoysup reviewed Jun 23, 2025

View reviewed changes

packages/compass-data-modeling/src/components/diagram-editor.tsx Outdated Show resolved Hide resolved

gribnoysup reviewed Jun 23, 2025

View reviewed changes

paula-stacho changed the title ~~feat(COMPASS-9432): apply initial layout~~ feat: apply initial layout COMPASS-9432 Jun 24, 2025

paula-stacho force-pushed the COMPASS-9432 branch 2 times, most recently from 2ef8837 to 235ab17 Compare June 25, 2025 11:51

addaleax approved these changes Jun 25, 2025

View reviewed changes

paula-stacho added 12 commits June 25, 2025 18:32

feat(COMPASS-9432): apply initial layout

bfbb2a5

store the layout

60760aa

update diagramming lib

1500d45

test and cleanup

6c4b800

wip test

2c7ebf8

fix test

b2abd6b

cleanup and another test

6d7a4a2

cleanup

8ceec28

update e2e

fce92dd

fitView, deps

e7c89d4

fixes

1baf122

use memo

c76a54d

paula-stacho and others added 5 commits June 25, 2025 18:32

update test helpers

536f8f7

fix naming

198685c

use nan

66534e7

Update packages/compass-data-modeling/src/components/diagram-editor.tsx

b019387

Co-authored-by: Anna Henningsen <[email protected]>

better mock

c14faac

paula-stacho force-pushed the COMPASS-9432 branch from bb093ab to c14faac Compare June 25, 2025 16:32

feat: apply initial layout COMPASS-9432 #7047

Are you sure you want to change the base?

feat: apply initial layout COMPASS-9432 #7047

Uh oh!

Conversation

paula-stacho commented Jun 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist

Motivation and Context

Open Questions

Dependents

Types of changes

Uh oh!

paula-stacho commented Jun 20, 2025

Uh oh!

gribnoysup Jun 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

paula-stacho commented Jun 20, 2025 •

edited

Loading

gribnoysup Jun 23, 2025 •

edited

Loading