feat(data-modeling): add add field button to object in diagram COMPASS-9742

[Anemy]

COMPASS-9742

Adds some field editing e2e testing as well. Left two comments/questions for reviewers.

add.field.to.object.mp4

[Anemy]

I don't love two functions that do pretty much the same thing, but it seemed better than doing something like:

export const getFieldsFromSchema = ({
  jsonSchema,
  renderOptions,
}: {
  jsonSchema: MongoDBJSONSchema;
  renderOptions?: {
    highlightedFields: FieldPath[];
    selectedField?: FieldPath;
    onClickAddNestedField: (parentFieldPath: string[]) => void;
  };
}): NodeProps['fields'] => {

[gribnoysup]

Big disclaimer that I'm just sharing some thoughts on this. I think what you have here is probably good enough (at least for now for sure), but as it bothers you I thought I'd try to provide some guidance here:

You are right that having two separate functions like that is not great: there's now no easy way to make sure that what we get for calculation purposes is the same as what we get for rendering purposes. That's not the only solution for this problem though 🙂 I mentioned this before, it's all about how you compose this: you have "default" level here, just pure data that contains serializeable information that should be enough to render this, and then you have "rendering" level that takes this data (maybe more stuff that's only rendering related, like those callbacks) and maps this to UI. The latter doesn't even need to live outside of React code FWIW, but if needed the logic of this mapping can be encapsulated into a function too.

So to summarise something that would allow you to compose the code in a way that builds up on the "base" functions and adds UI on top of it would probably be the cleanest way to deal with this instead of keeping the "base" completely separate. You can even have more "layers" here, why not, it doesn't really all need to happen in just two steps. Something like this pseudo-code:

type Node = NodeProps /* & { any extra stuff we need as long as it doesn't conflict with NodeProps } */
type Field = NodeProps['fields'][number]

function getBaseNode(collection: Collection): Node;

function getBaseFields(collection: Collection): Field[];

function getBaseNodes(collections: Collection[]): Node[] {
  return collections.map(collection => {
    return {
      ...getBaseNode(collection),
      fields: getBaseFields(collection)
    }
  });
}

function getUINode(node: Node): Node

function getUIFields(fields: Field[]): Field[]

function getNodesForUI(nodes: Node[]) { // <-- notice that this takes Node in, assuming this is already base node processed collections coming in here
  return nodes.map(node => {
    return {
      ...getUINode(node),
      fields: getUIFields(fields)
    }
  })
}

This has a slight downside of iterating over fields multiple times, but I would be hesitant to consider this an issue unless we really see this performing badly and even if it does, now you have shared building blocks to build two separate functions, but share the logic, we also have memoization tools to help us deal with that.

[Copilot]

Pull Request Overview

This PR adds functionality to allow users to add fields to object types directly from the diagram view in the data modeling feature. The implementation includes both the UI components to display "add field" buttons on object fields and the corresponding E2E tests to validate the new functionality.

Key changes:

• Added "add field" button to object field types in the diagram
• Created new E2E test helper functions for diagram interaction
• Extended field editing functionality to support nested field creation

Reviewed Changes

Copilot reviewed 14 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	Added comprehensive E2E test for field editing and new drag helper function
packages/compass-e2e-tests/helpers/selectors.ts	Added selectors for new add field buttons and field type combobox
packages/compass-e2e-tests/helpers/commands/set-multi-combo-box-value.ts	New helper command for setting multiple combobox values
packages/compass-data-modeling/src/utils/schema.ts	Enhanced field name generation to support nested object paths
packages/compass-data-modeling/src/utils/nodes-and-edges.tsx	Added object field type component and nested field functionality
packages/compass-data-modeling/src/components/diagram/object-field-type.tsx	New component rendering object type with add field button
packages/compass-data-modeling/src/store/diagram.ts	Added action handler for creating nested fields
packages/compass-data-modeling/src/components/diagram-editor.tsx	Connected nested field creation to the diagram editor

---

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

[gribnoysup]

Big disclaimer that I'm just sharing some thoughts on this. I think what you have here is probably good enough (at least for now for sure), but as it bothers you I thought I'd try to provide some guidance here:

You are right that having two separate functions like that is not great: there's now no easy way to make sure that what we get for calculation purposes is the same as what we get for rendering purposes. That's not the only solution for this problem though 🙂 I mentioned this before, it's all about how you compose this: you have "default" level here, just pure data that contains serializeable information that should be enough to render this, and then you have "rendering" level that takes this data (maybe more stuff that's only rendering related, like those callbacks) and maps this to UI. The latter doesn't even need to live outside of React code FWIW, but if needed the logic of this mapping can be encapsulated into a function too.

So to summarise something that would allow you to compose the code in a way that builds up on the "base" functions and adds UI on top of it would probably be the cleanest way to deal with this instead of keeping the "base" completely separate. You can even have more "layers" here, why not, it doesn't really all need to happen in just two steps. Something like this pseudo-code:

type Node = NodeProps /* & { any extra stuff we need as long as it doesn't conflict with NodeProps } */
type Field = NodeProps['fields'][number]

function getBaseNode(collection: Collection): Node;

function getBaseFields(collection: Collection): Field[];

function getBaseNodes(collections: Collection[]): Node[] {
  return collections.map(collection => {
    return {
      ...getBaseNode(collection),
      fields: getBaseFields(collection)
    }
  });
}

function getUINode(node: Node): Node

function getUIFields(fields: Field[]): Field[]

function getNodesForUI(nodes: Node[]) { // <-- notice that this takes Node in, assuming this is already base node processed collections coming in here
  return nodes.map(node => {
    return {
      ...getUINode(node),
      fields: getUIFields(fields)
    }
  })
}

This has a slight downside of iterating over fields multiple times, but I would be hesitant to consider this an issue unless we really see this performing badly and even if it does, now you have shared building blocks to build two separate functions, but share the logic, we also have memoization tools to help us deal with that.

[gribnoysup]

I was testing that the next field name logic is working fine and managed to get into some weird state where if I rename the field and then without unfocusing first I click on the "add new field" button, the new field is created with the name "interact" in the diagram, but the drawer shows the correct one, I can't figure out where the "interact" part is even coming from

[gribnoysup]

Ohhh, I was rewatching the gif and it seems to replace the next field in the document that is a sibling of an object this is being added for

[Anemy]

ooo nice catch

[Anemy]

I'm thinking this is most likely caused by the same issue as COMPASS-9737 and COMPASS-9738. We're re-rendering while a state change is happening, and as we're supplying entirely new nodes to ReactFlow, which diagramming is using in an uncontrolled way, we have multiple race conditions starting to happen.

I'm working on a fix for those in that ticket, however I'm not sure yet if the fix will catch this issue. A user could initiate a rename field edit that renames a field that their subsequent click (i.e. add nested field) would conflict with, thus generating an edit that would be invalid after the state change they just made. For instance they could rename a field they are try to add a field to, which would create an edit on the old field name, as the visual state hasn't changed yet.

I'm thinking the first and catch all approach here is to for our error handling to disregard invalid edits.
We already have some rough validation for edits:

compass/packages/compass-data-modeling/src/store/diagram.ts

Line 571 in 7f8d06c

const { result: isValid, errors } = validateEdit(edit);

That's fairly general parsing, and we don't have as much in how the static model is impacted by the subsequent requests.
We could update how our error handling works around when an edit is applied. We would take into consideration that the edit's collection name, field path, or field type could be invalid or no longer exist. So before we actually apply the edit, without a change made, which is what it does in most situations currently, we instead would disregard or throw away the edit. I'm thinking for now we wouldn't notify the user, maybe a log though.
https://github.com/mongodb-js/compass/pull/7300/files#diff-e1ebded80983ea1ae9d6db540f7967e84df56cbe83888336b0a809a53a11d36bR10
Right now we just error (in this one case, there are a few more).

For instance right now we throw if the field path doesn't exist in getNewUnusedFieldName. Instead we update it to catch that error and log, and not apply the edit.

[nbbeeken]

Trying to get my compass review legs under me, read through and looks, good Sergey caught some more interesting things than I ^

curious about one small thing

[Anemy]

Seeing some of the screenshots are at 1024x663 so pretty much none of the diagram shows on those. Those tests are failing currently. Going to skip on those screens.