chore(data-modeling): refactor data modeling to calculate layout as part of the analysis COMPASS-9487 #7145
Conversation
| const actions = useContext(DrawerActionsContext); | ||
| const stableActions = useRef({ | ||
| openDrawer(id: string) { | ||
| openDrawer: (id: string) => { |
There was a problem hiding this comment.
Drive-by to make sure eslint is not warning on unbound methods (those are safe to call unbound, this just makes it clear)
| const diagramContainerRef = useRef<HTMLDivElement | null>(null); | ||
| const diagram = useDiagram(); | ||
| const [areNodesReady, setAreNodesReady] = useState(false); | ||
| const diagram = useRef(useDiagram()); |
There was a problem hiding this comment.
I was planning to debug weird flashing of the diagram on main more, but this was actually caused by effects that depend on diagram here and just removing applyLayout effect solved it. Jus to make sure we don't burn ourselves by this I'm making diagram hooks stable. The useDiagram hook returns a new object every render effectively causing every hook that depends on it to re-run every time, wrapping it in a ref works around that
There was a problem hiding this comment.
No big changes in these methods (I called out the only one below), just moved them out of diagram-editor.tsx to a separate file so that I can share these between UI and analysis action
| const selection: Record<string, string[] | null | undefined> = {}; | ||
| if (relationship?.[0].ns) { | ||
| selection[relationship[0].ns] = relationship[0].fields; | ||
| } | ||
| if (relationship?.[1].ns) { | ||
| selection[relationship[1].ns] = relationship[1].fields; | ||
| } |
There was a problem hiding this comment.
One small drive-by change here. Before you had to have both namespaces selected to have either of the fields to start showing up in UI, I adjusted it a bit to make sure that field selections start to show up as soon as possible when you have both namespace and field set
| // TODO: react-flow documentation suggests that we should be able to do this | ||
| // without unrelyable scheduling by calling the fitView after initial state | ||
| // is set, but for this we will need to make some changes to the diagramming | ||
| // package first | ||
| return rafraf(() => { | ||
| void diagram.current.fitView(); | ||
| }); |
There was a problem hiding this comment.
Will open a diagramming ticket for this
Having
applyLayoutas part of the UI logic instead of calculating it once during the initial processing of the diagram required a lot of disconnected code that was driven with hard to navigate React effects logic and extra state. As layout calculation doesn't really depend on actual DOM, we can make this logic part of the inital model generation removing the need for complicated effects from the code