Skip to content

Commit

Permalink
[Asset graph sidebar] Preserve path when selecting a node in the side…
Browse files Browse the repository at this point in the history
…bar (#16665)

## Summary & Motivation

There are 2 sources of truth for the selected node, one tracked by the
graph, and one tracked by the sidebar. The difference is that the one
tracked by the sidebar includes a `path` (eg. the exact path of the node
you selected) where as selecting a node from the graph doesn't have a
path (it's ambiguous which path you want, so we just select the first
path with a matching ID). When we sync the select node to the graph, the
graph syncs the selected node back to the sidebar (`lastSelectedNode` =
selected node from graph as source of truth, `selectedNode` = selected
node from sidebar as source of truth) and it ends up overwriting the
selected node from the sidebar which has the real path, causing the
selected node's path in the sidebar to potentially be different than the
one that was clicked. This pr adds a check to prevent that from
happening by checking if the node synced back from the graph has the
same ID as the selected node in the sidebar.

## How I Tested These Changes

locally
  • Loading branch information
salazarm authored Sep 20, 2023
1 parent 67480f2 commit 25f11ba
Showing 1 changed file with 22 additions and 18 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -225,7 +225,9 @@ export const AssetGraphExplorerSidebar = React.memo(
nextOpenNodes.add(locationName);
nextOpenNodes.add(locationName + ':' + groupName);
}
setSelectedNode({id: lastSelectedNode.id});
if (selectedNode?.id !== lastSelectedNode.id) {
setSelectedNode({id: lastSelectedNode.id});
}
return nextOpenNodes;
}
let path = lastSelectedNode.id;
Expand All @@ -249,10 +251,15 @@ export const AssetGraphExplorerSidebar = React.memo(
currentPath = `${currentPath}:${nodesInPath[i]}`;
nextOpenNodes.add(currentPath);
}
setSelectedNode({id: lastSelectedNode.id, path: currentPath});
if (selectedNode?.id !== lastSelectedNode.id) {
setSelectedNode({id: lastSelectedNode.id, path: currentPath});
}
return nextOpenNodes;
});
} else {
setSelectedNode(null);
}
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [
lastSelectedNode,
assetGraphData,
Expand Down Expand Up @@ -430,8 +437,7 @@ const Node = ({
const downstream = Object.keys(assetGraphData.downstream[node.id] ?? {});
const elementRef = React.useRef<HTMLDivElement | null>(null);

const [showDownstreamDialog, setShowDownstreamDialog] = React.useState(false);
const [showUpstreamDialog, setShowUpstreamDialog] = React.useState(false);
const [showParents, setShowParents] = React.useState(false);

function showDownstreamGraph() {
const path = JSON.parse(node.id);
Expand Down Expand Up @@ -469,22 +475,11 @@ const Node = ({
<>
{launchpadElement}
<UpstreamDownstreamDialog
title="Downstream assets"
assets={downstream}
isOpen={showDownstreamDialog}
close={() => {
setShowDownstreamDialog(false);
}}
selectNode={(e, id) => {
selectNode(e, id);
}}
/>
<UpstreamDownstreamDialog
title="Upstream assets"
title="Parent assets"
assets={upstream}
isOpen={showUpstreamDialog}
isOpen={showParents}
close={() => {
setShowUpstreamDialog(false);
setShowParents(false);
}}
selectNode={selectNode}
/>
Expand Down Expand Up @@ -562,6 +557,15 @@ const Node = ({
}}
/>
{upstream.length || downstream.length ? <MenuDivider /> : null}
{upstream.length > 1 ? (
<MenuItem
text={`View parents (${upstream.length})`}
icon="list"
onClick={() => {
setShowParents(true);
}}
/>
) : null}
{upstream.length ? (
<MenuItem
text="Show upstream graph"
Expand Down

1 comment on commit 25f11ba

@github-actions
Copy link

Choose a reason for hiding this comment

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

Deploy preview for dagit-core-storybook ready!

✅ Preview
https://dagit-core-storybook-8cj8mn41e-elementl.vercel.app

Built with commit 25f11ba.
This pull request is being automatically deployed with vercel-action

Please sign in to comment.