From 962b090708395aa995fbeff76137444cac711ae1 Mon Sep 17 00:00:00 2001 From: SandroMaglione Date: Mon, 14 Sep 2026 10:32:05 +0200 Subject: [PATCH] Preserve both endpoints when repairing chart routes --- .changeset/safe-chart-endpoints.md | 7 + packages/devtools/package.json | 1 + .../src/internal/browser/chart-layout.ts | 109 ++++++----- .../internal/browser/ChartEndpoints.test.ts | 182 ++++++++++++++++++ pnpm-lock.yaml | 3 + scripts/release-contract.test.mjs | 11 ++ 6 files changed, 261 insertions(+), 52 deletions(-) create mode 100644 .changeset/safe-chart-endpoints.md create mode 100644 packages/devtools/test/internal/browser/ChartEndpoints.test.ts diff --git a/.changeset/safe-chart-endpoints.md b/.changeset/safe-chart-endpoints.md new file mode 100644 index 00000000..43c60e54 --- /dev/null +++ b/.changeset/safe-chart-endpoints.md @@ -0,0 +1,7 @@ +--- +"@typeonce/effect-machine-devtools": patch +--- + +Fix devtools charts failing to render retry transitions into compound states. Route repairs preserve the direction and clearance at both ends of each transition, including straight routes that need a detour to reach a state header. + +Keep the devtools platform dependencies on the supported Effect prerelease so fresh installations can start the CLI without missing-module errors. diff --git a/packages/devtools/package.json b/packages/devtools/package.json index 276b787d..2623e529 100644 --- a/packages/devtools/package.json +++ b/packages/devtools/package.json @@ -34,6 +34,7 @@ "dependencies": { "@effect/platform-browser": "4.0.0-rc.112", "@effect/platform-node": "4.0.0-rc.112", + "@effect/platform-node-shared": "4.0.0-rc.112", "@typeonce/effect-machine": "workspace:^", "chokidar": "4.0.3", "elkjs": "0.12.0", diff --git a/packages/devtools/src/internal/browser/chart-layout.ts b/packages/devtools/src/internal/browser/chart-layout.ts index 4fdaeafc..b74f0806 100644 --- a/packages/devtools/src/internal/browser/chart-layout.ts +++ b/packages/devtools/src/internal/browser/chart-layout.ts @@ -896,35 +896,52 @@ const shortenTransitionRoute = ( return candidate } -const normalizeTerminalDirection = ( - edge: ChartEdge, +// Both repairs use a route oriented away from the endpoint being repaired. +// An attached opposite endpoint is a constraint, including on two-point routes +// where replacing the connection would otherwise replace both endpoint steps. +const normalizeEndpointDirection = ( points: ReadonlyArray, - nodes: ReadonlyMap, - allNodes: ReadonlyArray + node: LaidOutChartNode, + opposite: LaidOutChartNode | undefined, + obstacles: ReadonlyArray ): ReadonlyArray => { - if (edge.target === null || isSelfTransition(edge) || points.length < 2) return points - const target = nodes.get(edge.target) - if (target === undefined) return points - const rawEnd = points.at(-1)! - const side = endpointSide(rawEnd, target) - const end = pointOnNodeBoundary(rawEnd, target, side) - const prefix = points.slice(0, -1) - const previous = prefix.at(-1)! - const attached = compactPoints([...prefix, end]) - if (isOutwardStep(end, previous, side)) return attached - - const targetStub = outwardPoint(end, side, 18) - const obstacles = [...routeObstacles(edge, allNodes), routeNodeRect(target)] - return endpointConnections(previous, targetStub, routeNodeRect(target), side) - .filter((connection) => - !connection.slice(0, -1).some((point) => point.x === end.x && point.y === end.y) && - routeIsClear([...connection, end], obstacles) + const rawStart = points[0]! + const side = endpointStepSide(rawStart, points[1]!, node) + const start = pointOnNodeBoundary(rawStart, node, side) + const end = points.at(-1)! + const previous = points.at(-2)! + const oppositeSide = opposite === undefined ? undefined : endpointStepSide(end, previous, opposite) + const preserveOpposite = opposite !== undefined && oppositeSide !== undefined && + nodeBoundaryDistance(end, opposite) <= 0.5 && isOutwardStep(end, previous, oppositeSide) + const validEndpoints = (route: ReadonlyArray): boolean => + isOutwardStep(route[0]!, route[1]!, side) && chartRouteLength(route.slice(0, 2)) >= 9 && + (!preserveOpposite || + isOutwardStep(route.at(-1)!, route.at(-2)!, oppositeSide!) && chartRouteLength(route.slice(-2)) >= 9) + + const tail = points.slice(1) + const attached = compactPoints([start, ...tail]) + if (attached.length >= 2 && validEndpoints(attached)) return attached + + if (tail.length === 1 && preserveOpposite) { + tail.unshift(outwardPoint(end, oppositeSide!, 18)) + } + const stub = outwardPoint(start, side, 18) + const endpointObstacles = [ + ...obstacles, + routeNodeRect(node), + ...(preserveOpposite ? [routeNodeRect(opposite!)] : []) + ] + return endpointConnections(stub, tail[0]!, routeNodeRect(node), side) + .map((connection) => compactPoints([start, ...connection, ...tail.slice(1)])) + .filter((route) => + route.length >= 2 && validEndpoints(route) && + !route.slice(1).some((point) => point.x === start.x && point.y === start.y) && + routeIsClear(route, endpointObstacles) ) - .map((connection) => compactPoints([...prefix, ...connection.slice(1), end])) .sort((left, right) => chartRouteLength(left) - chartRouteLength(right))[0] ?? attached } -const normalizeSourceDirection = ( +const normalizeTransitionEndpoints = ( edge: ChartEdge, points: ReadonlyArray, nodes: ReadonlyMap, @@ -932,21 +949,14 @@ const normalizeSourceDirection = ( ): ReadonlyArray => { if (isSelfTransition(edge) || points.length < 2) return points const source = nodes.get(edge.source) - if (source === undefined || source.node.children.length > 0) return points - const rawStart = points[0]! - const side = endpointSide(rawStart, source) - const start = pointOnNodeBoundary(rawStart, source, side) - const tail = points.slice(1) - const next = tail[0]! - const attached = compactPoints([start, ...tail]) - if (isOutwardStep(start, next, side)) return attached - - const sourceStub = outwardPoint(start, side, 18) - const obstacles = [...routeObstacles(edge, allNodes), routeNodeRect(source)] - return endpointConnections(sourceStub, next, routeNodeRect(source), side) - .filter((connection) => routeIsClear([start, ...connection, ...tail.slice(1)], obstacles)) - .map((connection) => compactPoints([start, ...connection, ...tail.slice(1)])) - .sort((left, right) => chartRouteLength(left) - chartRouteLength(right))[0] ?? attached + const target = edge.target === null ? undefined : nodes.get(edge.target) + const obstacles = routeObstacles(edge, allNodes) + const fromSource = source === undefined || source.node.children.length > 0 + ? points + : normalizeEndpointDirection(points, source, target, obstacles) + return target === undefined + ? fromSource + : [...normalizeEndpointDirection([...fromSource].reverse(), target, source, obstacles)].reverse() } const headerDetour = ( @@ -1207,24 +1217,19 @@ const collectLayout = ( if (elkPoints === undefined) return [] const chartEdge = chartEdges.get(edge.id) if (chartEdge === undefined) return [] - const points = normalizeTerminalDirection( + const points = normalizeTransitionEndpoints( chartEdge, - normalizeSourceDirection( + avoidCompoundHeaders( chartEdge, - avoidCompoundHeaders( + (shortenRoutes ? shortenTransitionRoute : (_edge: ChartEdge, route: ReadonlyArray) => route)( chartEdge, - (shortenRoutes ? shortenTransitionRoute : (_edge: ChartEdge, route: ReadonlyArray) => route)( - chartEdge, - normalizeHierarchyRoute(chartEdge, elkPoints, nodesByPath, nodes, hierarchyLanes), - nodesByPath, - nodes, - directLanes - ), + normalizeHierarchyRoute(chartEdge, elkPoints, nodesByPath, nodes, hierarchyLanes), + nodesByPath, nodes, - hierarchyLanes + directLanes ), - nodesByPath, - nodes + nodes, + hierarchyLanes ), nodesByPath, nodes @@ -1700,7 +1705,7 @@ export const layoutChartWith = ( new ChartLayoutError({ cause: { failures, invalid }, message: - `ELK did not produce a safe layout for ${model.machineId} after ${layoutProfiles.length} deterministic attempts: ${detail}` + `Chart routing did not produce a safe layout for ${model.machineId} after ${layoutProfiles.length} deterministic attempts: ${detail}` }) ) } diff --git a/packages/devtools/test/internal/browser/ChartEndpoints.test.ts b/packages/devtools/test/internal/browser/ChartEndpoints.test.ts new file mode 100644 index 00000000..2536b7ae --- /dev/null +++ b/packages/devtools/test/internal/browser/ChartEndpoints.test.ts @@ -0,0 +1,182 @@ +import { assert, describe, it } from "@effect/vitest" +import { Machine } from "@typeonce/effect-machine" +import { Effect, Schema } from "effect" +import type { ElkNode } from "elkjs/lib/elk-api.js" +import { + type ChartPoint, + chartRouteLength, + layoutChart, + layoutChartWith, + validateChartLayout +} from "../../../src/internal/browser/chart-layout.js" +import { type ChartModel, makeChartModel } from "../../../src/internal/browser/chart-model.js" +import * as MachineDocument from "../../../src/MachineDocument.js" + +const copyButtonModel = (): ChartModel => { + const root = Machine.state({ + states: { + Idle: {}, + Working: { fields: { text: Schema.String }, states: { Clicked: {}, Copying: {} } }, + Copied: {}, + Failed: { fields: { message: Schema.String } } + } + }) + const targets = Machine.targets(root) + const machine = Machine.make({ + id: "CopyButton", + root, + events: Machine.events({ Copy: { text: Schema.String } }), + effects: { copy: (text: string) => text.length > 0 ? Effect.void : Effect.fail({ message: "failure" }) }, + timers: { clicked: "120 millis", confirmation: "2 seconds", errorFeedback: "1 second" } + }).handle({ + initial: { target: targets.root.Idle }, + states: { + Idle: { + on: { + Copy: { + target: targets.root.Working, + guard: ({ event }) => event.text.length > 0, + data: ({ event }) => ({ text: event.text }) + } + } + }, + Working: { + initial: { target: targets.root.Working.Clicked }, + invoke: { + src: "copy", + input: ({ state }) => state.text, + onDone: { target: targets.root.Copied }, + onFailure: { target: targets.root.Failed, data: ({ error }) => ({ message: error.message }) } + }, + states: { + Clicked: { invoke: { src: "clicked", onDone: { target: targets.root.Working.Copying } } }, + Copying: {} + } + }, + Copied: { invoke: { src: "confirmation", onDone: { target: targets.root.Idle } } }, + Failed: { + invoke: { src: "errorFeedback", onDone: { target: targets.root.Idle } }, + on: { + Copy: { + target: targets.root.Working, + guard: ({ event }) => event.text.length > 0, + data: ({ event }) => ({ text: event.text }) + } + } + } + } + }) + return makeChartModel(MachineDocument.make(machine)) +} + +const model = copyButtonModel() +const variants: ReadonlyArray = [ + ["original", model], + ["reversed declarations", { ...model, nodes: [...model.nodes].reverse() }], + ["reversed edges", { ...model, edges: [...model.edges].reverse() }], + ["long labels", { + ...model, + edges: model.edges.map((edge) => ({ ...edge, label: edge.label + " with additional feedback details" })) + }], + ["without failure feedback", { + ...model, + edges: model.edges.filter((edge) => !(edge.source === "Failed" && edge.target === "Idle")) + }], + ["without success feedback", { + ...model, + edges: model.edges.filter((edge) => !(edge.source === "Copied" && edge.target === "Idle")) + }] +] + +describe("chart route endpoints", () => { + it.each(variants)("routes CopyButton with %s", async (_name, model) => { + const layout = await Effect.runPromise(layoutChart(model)) + const repeated = await Effect.runPromise(layoutChart(model)) + assert.deepStrictEqual(validateChartLayout(model, layout).issues, []) + assert.deepStrictEqual(repeated, layout) + const retry = layout.edges.find((edge) => + edge.kind === "transition" && edge.edge.source === "Failed" && edge.edge.target === "Working" + ) + assert.isDefined(retry) + assert.isAtLeast(chartRouteLength(retry!.points.slice(0, 2)), 9) + assert.isAtLeast(chartRouteLength(retry!.points.slice(-2)), 9) + }) + + // Rotate the same geometry to exercise every side, then reverse the edge to + // exercise source repair and terminal repair through the full layout pipeline. + for (const turns of [0, 1, 2, 3]) { + for (const reverse of [false, true]) { + it(`preserves the opposite endpoint with ${turns} quarter turns and reverse=${reverse}`, async () => { + const rotate = (point: ChartPoint): ChartPoint => { + let result = point + for (let turn = 0; turn < turns; turn++) result = { x: 600 - result.y, y: result.x } + return result + } + const rect = (id: string, x: number, y: number, width: number, height: number): ElkNode => { + const start = rotate({ x, y }) + const end = rotate({ x: x + width, y: y + height }) + return { + id, + x: Math.min(start.x, end.x), + y: Math.min(start.y, end.y), + width: Math.abs(start.x - end.x), + height: Math.abs(start.y - end.y) + } + } + const points = [{ x: 250, y: 400 }, { x: 250, y: 300 }].map(rotate) + if (reverse) points.reverse() + const model: ChartModel = { + machineId: "endpoint-repair", + roots: ["A", "B"], + nodes: ["A", "B"].map((path) => ({ + path, + label: path, + type: "atomic", + parent: null, + children: [], + active: false, + initial: false, + activities: [] + })), + edges: [{ + id: "edge", + transitionId: "edge", + branchIds: ["edge"], + source: reverse ? "B" : "A", + target: reverse ? "A" : "B", + kind: "target", + label: "go", + accessibleLabel: "go", + badges: [], + trigger: { type: "event", event: "go" }, + activityKind: null, + reenter: false, + acceptance: "required" + }], + runtimeTargets: [], + initials: [] + } + const graph: ElkNode = { + id: "graph", + width: 600, + height: 600, + children: [rect("A", 200, 400, 180, 88), rect("B", 100, 100, 120, 200)], + edges: [{ + id: "edge", + sources: [model.edges[0]!.source], + targets: [model.edges[0]!.target!], + sections: [{ id: "section", startPoint: points[0]!, endPoint: points[1]! }] + }] + } + const layout = await Effect.runPromise(layoutChartWith(model, async () => graph)) + assert.deepStrictEqual(validateChartLayout(model, layout).issues, []) + const route = layout.edges[0]!.points + assert.isAtLeast(chartRouteLength(route.slice(0, 2)), 9) + assert.isAtLeast(chartRouteLength(route.slice(-2)), 9) + for (let index = 1; index < route.length; index++) { + assert.isTrue(route[index - 1]!.x === route[index]!.x || route[index - 1]!.y === route[index]!.y) + } + }) + } + } +}) diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 7daf57f5..addca482 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -57,6 +57,9 @@ importers: '@effect/platform-node': specifier: 4.0.0-rc.112 version: 4.0.0-rc.112(effect@4.0.0-rc.112)(redis@6.2.1) + '@effect/platform-node-shared': + specifier: 4.0.0-rc.112 + version: 4.0.0-rc.112(effect@4.0.0-rc.112) '@typeonce/effect-machine': specifier: workspace:^ version: link:../effect-machine diff --git a/scripts/release-contract.test.mjs b/scripts/release-contract.test.mjs index 4dfc962a..8785f125 100644 --- a/scripts/release-contract.test.mjs +++ b/scripts/release-contract.test.mjs @@ -32,3 +32,14 @@ test("all Effect Machine packages release with the same version", async () => { "all Effect Machine packages must remain in the same Changesets fixed group" ) }) + +test("devtools pins its platform packages to the supported Effect prerelease", async () => { + const devtools = await readJson("packages/devtools/package.json") + for (const name of ["@effect/platform-browser", "@effect/platform-node", "@effect/platform-node-shared"]) { + assert.equal( + devtools.dependencies[name], + devtools.peerDependencies.effect, + `${name} must match the supported Effect version` + ) + } +})