-
Notifications
You must be signed in to change notification settings - Fork 46
Enable domain visualization on graph explorer #2810
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
c20f6cd
a2fda24
3f4550a
9e269dd
3c08504
99b998d
6730c02
f38e304
e4121e2
41cb0da
e0d3950
3e1e767
446d76c
e267c6d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,7 +6,7 @@ import { Flex, theme } from 'antd'; | |
| import { type MouseEvent, useCallback, useEffect, useState } from 'react'; | ||
|
|
||
| import { defaultFitViewOptions, NodeEditor } from '@/components/charts/node-editor/node-editor.tsx'; | ||
| import { CustomEdgeTypes } from '@/components/charts/node-editor/node-types.ts'; | ||
| import { CustomEdgeTypes, CustomNodeTypes } from '@/components/charts/node-editor/node-types.ts'; | ||
| import type { Node as GraphNode } from '@/store/api/services/generated/graphApi.ts'; | ||
| import { NodeType, useGetGraphDataQuery } from '@/store/api/services/generated/graphApi.ts'; | ||
| import { parseRegularNode } from '@/utils/node-parser.helper'; | ||
|
|
@@ -16,9 +16,49 @@ import { parseEdges } from '../explorer/utils'; | |
| import { Sidebar, type SidebarFilters } from './sidebar/sidebar'; | ||
| import { useNodeEditor } from './use-node-editor'; | ||
|
|
||
| function parseFullNodes(nodes: GraphNode[], setNodeId: (id: string) => void): Node[] { | ||
| const DOMAIN_COLORS = [ | ||
| { bg: 'rgba(59, 130, 246, 0.08)', border: 'rgba(59, 130, 246, 0.3)' }, // blue | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fine for now, but maybe this should be logged as tech debt as this both hardcodes the colors and will recycle them after 8 domains.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We already use antv from ant design, I believe there might be a colorpallette there. I will look into it and respond here again. |
||
| { bg: 'rgba(16, 185, 129, 0.08)', border: 'rgba(16, 185, 129, 0.3)' }, // green | ||
| { bg: 'rgba(245, 158, 11, 0.08)', border: 'rgba(245, 158, 11, 0.3)' }, // amber | ||
| { bg: 'rgba(139, 92, 246, 0.08)', border: 'rgba(139, 92, 246, 0.3)' }, // purple | ||
| { bg: 'rgba(236, 72, 153, 0.08)', border: 'rgba(236, 72, 153, 0.3)' }, // pink | ||
| { bg: 'rgba(20, 184, 166, 0.08)', border: 'rgba(20, 184, 166, 0.3)' }, // teal | ||
| { bg: 'rgba(249, 115, 22, 0.08)', border: 'rgba(249, 115, 22, 0.3)' }, // orange | ||
| { bg: 'rgba(99, 102, 241, 0.08)', border: 'rgba(99, 102, 241, 0.3)' }, // indigo | ||
| ]; | ||
|
|
||
| function parseFullNodes(nodes: GraphNode[], setNodeId: (id: string) => void, domainsEnabled: boolean): Node[] { | ||
| // Synthesize domain container nodes | ||
| const domainNodes: Node[] = []; | ||
| if (domainsEnabled) { | ||
| const domainMap = new Map<string, string>(); | ||
| for (const node of nodes) { | ||
| if (node.data.domain_id && node.data.domain && !domainMap.has(node.data.domain_id)) { | ||
| domainMap.set(node.data.domain_id, node.data.domain); | ||
| } | ||
| } | ||
| const sortedDomainIds = [...domainMap.keys()].sort(); | ||
| for (let i = 0; i < sortedDomainIds.length; i++) { | ||
| const domainId = sortedDomainIds[i]; | ||
| const color = DOMAIN_COLORS[i % DOMAIN_COLORS.length]; | ||
| domainNodes.push({ | ||
| id: domainId, | ||
| position: { x: 0, y: 0 }, | ||
| type: CustomNodeTypes.DomainNode, | ||
| draggable: true, | ||
| deletable: false, | ||
| data: { | ||
| id: domainId, | ||
| name: domainMap.get(domainId), | ||
| backgroundColor: color.bg, | ||
| borderColor: color.border, | ||
| }, | ||
| }); | ||
| } | ||
| } | ||
|
|
||
| // Parse regular nodes | ||
| return nodes | ||
| const regularNodes = nodes | ||
| .filter((node) => node.type !== NodeType.DomainNode) | ||
| .map((node) => { | ||
| let extra_attributes = {}; | ||
|
|
@@ -56,9 +96,10 @@ function parseFullNodes(nodes: GraphNode[], setNodeId: (id: string) => void): No | |
| default: | ||
| throw new Error(`Unknown node type: ${node.type}`); | ||
| } | ||
| // For now perma disable domains | ||
| return parseRegularNode(node, setNodeId, false, true, extra_attributes); | ||
| }); // Skip domain nodes for clarity and reduced clutter | ||
| return parseRegularNode(node, setNodeId, domainsEnabled, true, extra_attributes); | ||
| }); | ||
|
|
||
| return [...domainNodes, ...regularNodes]; | ||
| } | ||
|
|
||
| function applyHighlighting(nodes: Node[], edges: Edge[], selectedId: string | null) { | ||
|
|
@@ -83,19 +124,29 @@ function applyHighlighting(nodes: Node[], edges: Edge[], selectedId: string | nu | |
| } | ||
| }); | ||
|
|
||
| // Domain nodes should stay visible if any of their children are connected | ||
| const domainNodeIds = new Set(nodes.filter((n) => n.type === CustomNodeTypes.DomainNode).map((n) => n.id)); | ||
| const activeDomainIds = new Set( | ||
| nodes.filter((n) => n.parentId && connectedNodeIds.has(n.id)).map((n) => n.parentId as string), | ||
| ); | ||
|
|
||
| // Apply highlighting to nodes | ||
| const highlightedNodes = nodes.map((node) => ({ | ||
| ...node, | ||
| data: { | ||
| ...node.data, | ||
| dimmed: !connectedNodeIds.has(node.id), | ||
| }, | ||
| style: { | ||
| ...node.style, | ||
| opacity: connectedNodeIds.has(node.id) ? 1 : 0.3, | ||
| zIndex: connectedNodeIds.has(node.id) ? 10 : 1, | ||
| }, | ||
| })); | ||
| const highlightedNodes = nodes.map((node) => { | ||
| const isConnected = | ||
| connectedNodeIds.has(node.id) || (domainNodeIds.has(node.id) && activeDomainIds.has(node.id)); | ||
| return { | ||
| ...node, | ||
| data: { | ||
| ...node.data, | ||
| dimmed: !isConnected, | ||
| }, | ||
| style: { | ||
| ...node.style, | ||
| opacity: isConnected ? 1 : 0.3, | ||
| zIndex: isConnected ? 10 : 1, | ||
| }, | ||
| }; | ||
| }); | ||
|
|
||
| // Apply highlighting to edges | ||
| const highlightedEdges = edges.map((edge) => ({ | ||
|
|
@@ -141,7 +192,7 @@ export default function InternalFullExplorer() { | |
|
|
||
| const generateGraph = useCallback(async () => { | ||
| if (graph) { | ||
| const nodes = parseFullNodes(graph.nodes, setNodeId); | ||
| const nodes = parseFullNodes(graph.nodes, setNodeId, sidebarFilters.domainsEnabled); | ||
| const edges = parseEdges(graph.edges, token); | ||
|
|
||
| // Explicitly specify straight edge so it doesn't default to default edge (which is a Bézier curve) | ||
|
|
@@ -221,7 +272,6 @@ export default function InternalFullExplorer() { | |
| <Flex className={styles.nodeWrapper}> | ||
| <Sidebar | ||
| nodes={nodes} | ||
| setNodes={setNodes} | ||
| onFilterChange={setSidebarFilters} | ||
| sidebarFilters={sidebarFilters} | ||
| nodeId={nodeId} | ||
|
|
||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. tests should be placed in the tests folder
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No in front end testing, it is preferred to place it next to the component
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. :o that feels weird :D But okay! Learned something new |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,74 @@ | ||
| import { Position } from '@xyflow/react'; | ||
| import { describe, expect, it, vi } from 'vitest'; | ||
|
|
||
| import type { Node as GraphNode } from '@/store/api/services/generated/dataProductsApi'; | ||
| import { parseRegularNode, sharedAttributes } from '@/utils/node-parser.helper'; | ||
|
|
||
| // Minimal GraphNode factory — only fields the parser actually reads. | ||
| // `import type` is erased at compile time so it does not trigger module execution. | ||
| const graphNode = (domain_id: string | null = null, domain: string | null = null): GraphNode => ({ | ||
| id: 'node-1', | ||
| type: 'dataProductNode' as GraphNode['type'], | ||
| isMain: false, | ||
| data: { | ||
| id: 'node-1', | ||
| name: 'Test Product', | ||
| icon_key: 'ANALYTICS', | ||
| domain_id, | ||
| domain, | ||
| description: null, | ||
| link_to_id: null, | ||
| }, | ||
| }); | ||
|
|
||
| const noop = vi.fn(); | ||
|
|
||
| // --------------------------------------------------------------------------- | ||
| // parentId assignment (the core change this PR makes) | ||
| // --------------------------------------------------------------------------- | ||
|
|
||
| describe('sharedAttributes — domainsEnabled', () => { | ||
| it('sets parentId to domain_id when domainsEnabled=true and domain_id is present', () => { | ||
| const node = sharedAttributes(graphNode('dom-1', 'Finance'), noop, true, false); | ||
| expect(node.parentId).toBe('dom-1'); | ||
| }); | ||
|
|
||
| it('does not set parentId when domainsEnabled=false even if domain_id is present', () => { | ||
| const node = sharedAttributes(graphNode('dom-1', 'Finance'), noop, false, false); | ||
| expect(node.parentId).toBeUndefined(); | ||
| }); | ||
|
|
||
| it('does not set parentId when domain_id is null', () => { | ||
| const node = sharedAttributes(graphNode(null, null), noop, true, false); | ||
| expect(node.parentId).toBeUndefined(); | ||
| }); | ||
|
|
||
| it('always sets id, position, type from the input node', () => { | ||
| const node = sharedAttributes(graphNode('dom-1', 'Finance'), noop, true, false); | ||
| expect(node.id).toBe('node-1'); | ||
| expect(node.position).toEqual({ x: 0, y: 0 }); | ||
| expect(node.type).toBe('dataProductNode'); | ||
| }); | ||
| }); | ||
|
|
||
| // --------------------------------------------------------------------------- | ||
| // parseRegularNode passes extra_attributes through | ||
| // --------------------------------------------------------------------------- | ||
|
|
||
| describe('parseRegularNode', () => { | ||
| it('merges extra_attributes into data', () => { | ||
| const extra = { targetHandlePosition: Position.Left }; | ||
| const node = parseRegularNode(graphNode('dom-1', 'Finance'), noop, true, false, extra); | ||
| expect(node.data.targetHandlePosition).toBe('left'); | ||
| }); | ||
|
|
||
| it('preserves parentId set by sharedAttributes', () => { | ||
| const node = parseRegularNode(graphNode('dom-1', 'Finance'), noop, true, false, {}); | ||
| expect(node.parentId).toBe('dom-1'); | ||
| }); | ||
|
|
||
| it('exposes the domain name on node.data.domain', () => { | ||
| const node = parseRegularNode(graphNode('dom-1', 'Finance'), noop, true, false, {}); | ||
| expect(node.data.domain).toBe('Finance'); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am unsure what the tests add to be honest, the logic did not change? So unsure why they are required. Can you explain to me the reasoning for adding these?
The includes domain fields tests for example could just be the last 2 asserts that are added to
test_get_graph_data.