Repository: mermaid-js/mermaid
File: packages/mermaid/src/dagre-wrapper/index.js (JavaScript)

Code before:
  const edgePaths = elem.insert('g').attr('class', 'edgePaths');
  const edgeLabels = elem.insert('g').attr('class', 'edgeLabels');
  const nodes = elem.insert('g').attr('class', 'nodes');

  // Insert nodes, this will insert them into the dom and each node will get a size. The size is updated
  // to the abstract node and is later used by dagre for the layout
  await Promise.all(
    graph.nodes().map(async function (v) {
      const node = graph.node(v);
      if (parentCluster !== undefined) {
        const data = JSON.parse(JSON.stringify(parentCluster.clusterData));
        // data.clusterPositioning = true;
        log.info('Setting data for cluster XXX (', v, ') ', data, parentCluster);
        graph.setNode(parentCluster.id, data);
        if (!graph.parent(v)) {
          log.trace('Setting parent', v, parentCluster.id);
          graph.setParent(v, parentCluster.id, data);
        }
      }
      log.info('(Insert) Node XXX' + v + ': ' + JSON.stringify(graph.node(v)));
      if (node && node.clusterNode) {
        // const children = graph.children(v);
        log.info('Cluster identified', v, node.width, graph.node(v));
        // node.graph.setGraph applies the graph configurations such as nodeSpacing to subgraphs as without this the default values would be used
        // We override the `rankdir` variable to allow for the subgraph's direction to override the graphs direction in the subgraph
        node.graph.setGraph({ ...graph.graph(), rankdir: node.graph.graph().rankdir });
        const o = await recursiveRender(
          nodes,
          node.graph,
          diagramtype,
          id,
          graph.node(v),
          siteConfig
        );
        const newEl = o.elem;
        updateNodeBounds(node, newEl);
        node.diff = o.diff || 0;
        log.info('Node bounds (abc123)', v, node, node.width, node.x, node.y);
        setNodeElem(newEl, node);

        log.warn('Recursive render complete ', newEl, node);
      } else {
        if (graph.children(v).length > 0) {
          // This is a cluster but not to be rendered recursively
          // Render as before
          log.info('Cluster - the non recursive path XXX', v, node.id, node, graph);
          log.info(findNonClusterChild(node.id, graph));
          clusterDb[node.id] = { id: findNonClusterChild(node.id, graph), node };
          // insertCluster(clusters, graph.node(v));
        } else {
          log.info('Node - the non recursive path', v, node.id, node);

Review comment (bug):
I think I've found the issue that was causing the screenshots of the E2E tests to fail.

The issue is that we were updating `marginy` for subgraphs to use their parent value, and that was causing the positioning to shift very slightly.

If we instead only copy over the `ranksep` (i.e. the [`rankSpacing`](https://mermaid.js.org/config/schema-docs/config-defs-flowchart-diagram-config.html#rankspacing)) and the `nodesep` (i.e. the [`nodeSpacing`](https://mermaid.js.org/config/schema-docs/config-defs-flowchart-diagram-config.html#nodespacing)), it seems to work fine.

In the real world, however, it should would slightly modify the subgraphs of anybody that has set `rankSpacing`/`nodeSpacing`, so IMO, this should be in a MINOR release, not a PATCH release.

```suggestion
        const { ranksep, nodesep } = graph.graph();
        node.graph.setGraph({
          ...node.graph.graph(),
          ranksep,
          nodesep,
        });
```

Code after:
  const edgePaths = elem.insert('g').attr('class', 'edgePaths');
  const edgeLabels = elem.insert('g').attr('class', 'edgeLabels');
  const nodes = elem.insert('g').attr('class', 'nodes');

  // Insert nodes, this will insert them into the dom and each node will get a size. The size is updated
  // to the abstract node and is later used by dagre for the layout
  await Promise.all(
    graph.nodes().map(async function (v) {
      const node = graph.node(v);
      if (parentCluster !== undefined) {
        const data = JSON.parse(JSON.stringify(parentCluster.clusterData));
        // data.clusterPositioning = true;
        log.info('Setting data for cluster XXX (', v, ') ', data, parentCluster);
        graph.setNode(parentCluster.id, data);
        if (!graph.parent(v)) {
          log.trace('Setting parent', v, parentCluster.id);
          graph.setParent(v, parentCluster.id, data);
        }
      }
      log.info('(Insert) Node XXX' + v + ': ' + JSON.stringify(graph.node(v)));
      if (node && node.clusterNode) {
        // const children = graph.children(v);
        log.info('Cluster identified', v, node.width, graph.node(v));
        // `node.graph.setGraph` applies the graph configurations such as nodeSpacing to subgraphs as without this the default values would be used
        // We override only the `ranksep` and `nodesep` configurations to allow for setting subgraph spacing while avoiding overriding other properties
        const { ranksep, nodesep } = graph.graph();
        node.graph.setGraph({
          ...node.graph.graph(),
          ranksep,
          nodesep,
        });
        const o = await recursiveRender(
          nodes,
          node.graph,
          diagramType,
          id,
          graph.node(v),
          siteConfig
        );
        const newEl = o.elem;
        updateNodeBounds(node, newEl);
        node.diff = o.diff || 0;
        log.info('Node bounds (abc123)', v, node, node.width, node.x, node.y);
        setNodeElem(newEl, node);

        log.warn('Recursive render complete ', newEl, node);
      } else {
        if (graph.children(v).length > 0) {
          // This is a cluster but not to be rendered recursively
          // Render as before
          log.info('Cluster - the non recursive path XXX', v, node.id, node, graph);