From 9a659b636d713ee8fdfbdad41c58bb3d7c79e0d9 Mon Sep 17 00:00:00 2001 From: Ruben Fiszel Date: Thu, 28 May 2026 18:16:12 +0200 Subject: [PATCH] fix(frontend): prevent duplicate asset node ids crashing flow graph (#9367) --- .../graph/renderers/nodes/AssetNode.svelte | 86 +++++++++---------- .../graph/renderers/nodes/assetNode.test.ts | 51 +++++++++++ 2 files changed, 94 insertions(+), 43 deletions(-) create mode 100644 frontend/src/lib/components/graph/renderers/nodes/assetNode.test.ts diff --git a/frontend/src/lib/components/graph/renderers/nodes/AssetNode.svelte b/frontend/src/lib/components/graph/renderers/nodes/AssetNode.svelte index 50e4f9c87a..0d5a9a14ea 100644 --- a/frontend/src/lib/components/graph/renderers/nodes/AssetNode.svelte +++ b/frontend/src/lib/components/graph/renderers/nodes/AssetNode.svelte @@ -63,7 +63,7 @@ type: 'asset' as const, parentId: node.id, data: { asset, displayedAccessType: 'r' }, - id: `${node.id}-asset-in-${asset.kind}-${asset.path}`, + id: `${node.id}-asset-in-${asset.kind}-${asset.path}-${i}`, width: inputAssetWidth, position: { x: @@ -100,7 +100,7 @@ type: 'asset' as const, parentId: node.id, data: { asset, displayedAccessType: 'w' }, - id: `${node.id}-asset-out-${asset.kind}-${asset.path}`, + id: `${node.id}-asset-out-${asset.kind}-${asset.path}-${i}`, width: outputAssetWidth, position: { x: @@ -136,7 +136,7 @@ allAssetNodes.push(...(inputAssetNodes ?? []), ...(outputAssetNodes ?? [])) // If there are more than 3 assets, we create an overflow node - if (overflowedInputAssets.length) + if (overflowedInputAssets.length) { allAssetNodes.push({ type: 'assetsOverflowed', data: { overflowedAssets: overflowedInputAssets, displayedAccessType: 'r' }, @@ -148,14 +148,15 @@ y: READ_ASSET_Y_OFFSET } } satisfies Node & AssetsOverflowedN) - allAssetEdges.push({ - id: `${node.id}-assets-overflowed-in-edge`, - source: `${node.id}-assets-overflowed-in`, - target: node.id, - type: 'empty', - data: { class: '!opacity-35 dark:!opacity-20' } - }) - if (overflowedOutputAssets.length) + allAssetEdges.push({ + id: `${node.id}-assets-overflowed-in-edge`, + source: `${node.id}-assets-overflowed-in`, + target: node.id, + type: 'empty', + data: { class: '!opacity-35 dark:!opacity-20' } + }) + } + if (overflowedOutputAssets.length) { allAssetNodes.push({ type: 'assetsOverflowed', data: { overflowedAssets: overflowedOutputAssets, displayedAccessType: 'w' }, @@ -167,13 +168,14 @@ y: WRITE_ASSET_Y_OFFSET } } satisfies Node & AssetsOverflowedN) - allAssetEdges.push({ - id: `${node.id}-assets-overflowed-out-edge`, - source: node.id, - target: `${node.id}-assets-overflowed-out`, - type: 'empty', - data: { class: '!opacity-35 dark:!opacity-25' } - }) + allAssetEdges.push({ + id: `${node.id}-assets-overflowed-out-edge`, + source: node.id, + target: `${node.id}-assets-overflowed-out`, + type: 'empty', + data: { class: '!opacity-35 dark:!opacity-25' } + }) + } } let ret: ReturnType = { @@ -274,8 +276,8 @@ {#snippet text()} - Could not find resource - {/snippet} + Could not find resource + {/snippet} {:else if isSelected && assetCanBeExplored(data.asset, cachedResourceMetadata) && !$userStore?.operator}
@@ -291,29 +293,27 @@ {/if}
{#snippet text()} - - {#if usageCount !== undefined} - Used in {pluralize(usageCount, 'step')}
- {/if} - { - if (data.asset.kind === 'resource') - flowGraphAssetsCtx?.val.resourceEditorDrawer?.initEdit(data.asset.path) - }} - > - {data.asset.path} -
- - {formatAssetKind({ ...data.asset, metadata: cachedResourceMetadata })} - - - {/snippet} + {#if usageCount !== undefined} + Used in {pluralize(usageCount, 'step')}
+ {/if} + { + if (data.asset.kind === 'resource') + flowGraphAssetsCtx?.val.resourceEditorDrawer?.initEdit(data.asset.path) + }} + > + {data.asset.path} +
+ + {formatAssetKind({ ...data.asset, metadata: cachedResourceMetadata })} + + {/snippet} {/snippet} diff --git a/frontend/src/lib/components/graph/renderers/nodes/assetNode.test.ts b/frontend/src/lib/components/graph/renderers/nodes/assetNode.test.ts new file mode 100644 index 0000000000..14fd5e24e3 --- /dev/null +++ b/frontend/src/lib/components/graph/renderers/nodes/assetNode.test.ts @@ -0,0 +1,51 @@ +import { describe, it, expect, vi } from 'vitest' + +// Mock heavy transitive imports pulled in by AssetNode.svelte's instance script +vi.mock('monaco-editor', () => ({})) +vi.mock('$lib/components/meltComponents', () => ({ Tooltip: {} })) +vi.mock('../../../ExploreAssetButton.svelte', () => ({ + default: {}, + assetCanBeExplored: () => false +})) +vi.mock('$lib/components/icons/AssetGenericIcon.svelte', () => ({ default: {} })) +vi.mock('$lib/components/assets/AssetColumnBadges.svelte', () => ({ default: {} })) +vi.mock('./NodeWrapper.svelte', () => ({ default: {} })) + +import { computeAssetNodes } from './AssetNode.svelte' + +function nodeWithAssets(id: string, assets: any[]) { + return { id, position: { x: 0, y: 0 }, data: { assets } } +} + +describe('computeAssetNodes (WIN-1998)', () => { + it('produces unique node and edge ids when a module lists the same asset twice', () => { + // Two assets with identical kind+path (e.g. read twice, or r + rw) — both + // display as inputs. Before the fix these collided on the same node id and + // crashed SvelteFlow with `each_key_duplicate`. + const dup = { kind: 'resource', path: 'f/foo/bar', access_type: 'r' } + const { newAssetNodes, newAssetEdges } = computeAssetNodes([ + nodeWithAssets('moduleA', [{ ...dup }, { ...dup }]) + ]) + + const nodeIds = newAssetNodes.map((n) => n.id) + expect(new Set(nodeIds).size).toBe(nodeIds.length) + + const edgeIds = newAssetEdges.map((e) => e.id) + expect(new Set(edgeIds).size).toBe(edgeIds.length) + }) + + it('does not emit overflow edges when there is no overflow node (<=3 assets)', () => { + const { newAssetNodes, newAssetEdges } = computeAssetNodes([ + nodeWithAssets('moduleB', [{ kind: 'resource', path: 'f/a/x', access_type: 'r' }]) + ]) + + // No overflow node should be created for a single asset... + expect(newAssetNodes.some((n) => n.type === 'assetsOverflowed')).toBe(false) + // ...and therefore no dangling edge should reference a missing overflow node. + const nodeIdSet = new Set(newAssetNodes.map((n) => n.id).concat('moduleB')) + for (const e of newAssetEdges) { + expect(nodeIdSet.has(e.source as string)).toBe(true) + expect(nodeIdSet.has(e.target as string)).toBe(true) + } + }) +})