mirror of
https://github.com/windmill-labs/windmill.git
synced 2026-08-24 00:00:46 +00:00
fix(frontend): address PR review feedback on flow layout
- Add max recursion depth guard (50) to layoutLevel to prevent stack overflow with malformed flow data - Log swallowed decrossOpt error as console.debug for debuggability - Initialize maxY to -Infinity for correctness with negative positions - Fix indentation artifacts in graphBuilder data objects Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This commit is contained in:
@@ -166,7 +166,8 @@ function runSugiyama(
|
||||
return [w + constants.gapH, h + constants.gapV] as readonly [number, number]
|
||||
})
|
||||
boxSize = layout(dag as any) as any
|
||||
} catch {
|
||||
} catch (e) {
|
||||
console.debug('[compoundLayout] decrossOpt failed, falling back to decrossTwoLayer:', e)
|
||||
const layout = sugiyama()
|
||||
.decross(decrossTwoLayer())
|
||||
.coord(coordCenter())
|
||||
@@ -229,11 +230,14 @@ function runSugiyama(
|
||||
* 5. Run sugiyama on the simplified graph
|
||||
* 6. Expand wrapper positions back to absolute positions
|
||||
*/
|
||||
const MAX_RECURSION_DEPTH = 50
|
||||
|
||||
function layoutLevel(
|
||||
nodeIds: string[],
|
||||
allNodes: Map<string, LayoutNode>,
|
||||
constants: LayoutConstants,
|
||||
childrenMap: Map<string, string[]>
|
||||
childrenMap: Map<string, string[]>,
|
||||
depth: number = 0
|
||||
): LayoutResult {
|
||||
const positions = new Map<string, { x: number; y: number }>()
|
||||
const nodeIdSet = new Set(nodeIds)
|
||||
@@ -246,6 +250,19 @@ function layoutLevel(
|
||||
}
|
||||
}
|
||||
|
||||
if (depth >= MAX_RECURSION_DEPTH) {
|
||||
console.warn('[compoundLayout] Max recursion depth reached, falling back to flat layout')
|
||||
const flatNodes = nodeIds.map((id) => {
|
||||
const n = allNodes.get(id)!
|
||||
return { id, parentIds: (n.parentIds ?? []).filter((pid) => nodeIdSet.has(pid)) }
|
||||
})
|
||||
const result = runSugiyama(flatNodes, constants)
|
||||
for (const [id, pos] of result.positions) {
|
||||
positions.set(id, pos)
|
||||
}
|
||||
return { positions, bbox: { width: result.width, height: result.height }, contentMinX: 0 }
|
||||
}
|
||||
|
||||
// Step 1: detect compound groups at this level
|
||||
const groups = detectGroups(nodeIdSet, allNodes, childrenMap)
|
||||
|
||||
@@ -305,7 +322,7 @@ function layoutLevel(
|
||||
const branchNodeIds = [branch.labelId, ...branch.innerIds]
|
||||
|
||||
// Find sub-groups within this branch
|
||||
const result = layoutLevel(branchNodeIds, allNodes, constants, childrenMap)
|
||||
const result = layoutLevel(branchNodeIds, allNodes, constants, childrenMap, depth + 1)
|
||||
|
||||
branchLayouts.push({
|
||||
labelId: branch.labelId,
|
||||
@@ -453,7 +470,7 @@ function layoutLevel(
|
||||
let minX = Infinity
|
||||
let maxX = -Infinity
|
||||
let minY = Infinity
|
||||
let maxY = 0
|
||||
let maxY = -Infinity
|
||||
for (const pos of positions.values()) {
|
||||
minX = Math.min(minX, pos.x - constants.nodeWidth / 2)
|
||||
maxX = Math.max(maxX, pos.x + constants.nodeWidth / 2)
|
||||
|
||||
@@ -650,7 +650,7 @@ export function graphBuilder(
|
||||
const startNode: NodeLayout = {
|
||||
id: `${module.id}-branch-0`,
|
||||
data: {
|
||||
id: module.id,
|
||||
id: module.id,
|
||||
branchIndex: -1,
|
||||
eventHandlers: eventHandlers,
|
||||
flowModuleState: extra.flowModuleStates?.[module.id],
|
||||
@@ -675,7 +675,7 @@ export function graphBuilder(
|
||||
const startNode: NodeLayout = {
|
||||
id: `${module.id}-branch-${branchIndex}`,
|
||||
data: {
|
||||
label: defaultIfEmptyString(branch.summary, `Branch ${branchIndex + 1}`),
|
||||
label: defaultIfEmptyString(branch.summary, `Branch ${branchIndex + 1}`),
|
||||
id: module.id,
|
||||
branchIndex: branchIndex,
|
||||
eventHandlers: eventHandlers,
|
||||
@@ -878,7 +878,7 @@ export function graphBuilder(
|
||||
const startNode: NodeLayout = {
|
||||
id: `${module.id}-branch-${branchIndex}`,
|
||||
data: {
|
||||
label: defaultIfEmptyString(branch.summary, 'Branch ' + (branchIndex + 1)),
|
||||
label: defaultIfEmptyString(branch.summary, 'Branch ' + (branchIndex + 1)),
|
||||
preLabel: branch.summary ? '' : branch.expr,
|
||||
id: module.id,
|
||||
branchIndex: branchIndex,
|
||||
@@ -921,7 +921,7 @@ export function graphBuilder(
|
||||
const startNode: NodeLayout = {
|
||||
id: startId,
|
||||
data: {
|
||||
label: `Start of subflow ${idWithoutPrefix}`,
|
||||
label: `Start of subflow ${idWithoutPrefix}`,
|
||||
id: startId,
|
||||
subflowId: module.id,
|
||||
eventHandlers: eventHandlers,
|
||||
@@ -949,7 +949,7 @@ export function graphBuilder(
|
||||
const endNode: NodeLayout = {
|
||||
id: endId,
|
||||
data: {
|
||||
label: `End of subflow ${idWithoutPrefix}`,
|
||||
label: `End of subflow ${idWithoutPrefix}`,
|
||||
id: endId,
|
||||
subflowId: module.id,
|
||||
eventHandlers: eventHandlers,
|
||||
|
||||
Reference in New Issue
Block a user