From aba099f066eaf446af07cb52cbf0ffd7b185bc17 Mon Sep 17 00:00:00 2001 From: Bobby Powers Date: Mon, 29 Jun 2026 20:46:26 -0700 Subject: [PATCH 1/9] diagram: restore tool deselection on re-click and palette close The SpeedDial tool palette regressed two deselect gestures the editor once had: re-clicking the active tool no longer toggled it off, and closing the palette via the FAB left the tool selected. On load no tool is selected, and both gestures should return to that state. handleDialClick lost its selectedTool clearing in 5191a9b6 (a SpeedDial UX commit); restore it so closing the palette deselects the active tool. The tool-select handlers set the tool unconditionally; make them toggle so re-clicking the active tool deselects it. Selecting a different tool still switches normally. --- src/diagram/Editor.tsx | 31 ++-- .../tests/editor-tool-deselect.test.ts | 165 ++++++++++++++++++ 2 files changed, 180 insertions(+), 16 deletions(-) create mode 100644 src/diagram/tests/editor-tool-deselect.test.ts diff --git a/src/diagram/Editor.tsx b/src/diagram/Editor.tsx index 1313ca961..eb5ff16b0 100644 --- a/src/diagram/Editor.tsx +++ b/src/diagram/Editor.tsx @@ -658,7 +658,14 @@ export const Editor = React.memo(function Editor(props: EditorProps): React.Reac }; const handleDialClick = React.useCallback((_event: React.MouseEvent): void => { - setState((prev) => ({ dialOpen: !prev.dialOpen })); + // Toggle the palette open/closed. Closing it deselects the active tool so the + // canvas returns to plain selection mode -- matching the load-time state where + // no tool is selected (the clearing was dropped in 5191a9b6 and is restored + // here). Opening leaves any selected tool untouched. + setState((prev) => ({ + dialOpen: !prev.dialOpen, + selectedTool: prev.dialOpen ? undefined : prev.selectedTool, + })); }, []); const handleDialClose = React.useCallback((_e: React.SyntheticEvent, reason: CloseReason): void => { @@ -2079,44 +2086,36 @@ export const Editor = React.memo(function Editor(props: EditorProps): React.Reac setState({ selectedTool: undefined }); }, []); + // Clicking a tool selects it; clicking the already-active tool deselects it + // (toggle), returning to plain selection mode -- the load-time state. const handleSelectStock = React.useCallback((e: React.MouseEvent): void => { e.preventDefault(); e.stopPropagation(); - setState({ - selectedTool: 'stock', - }); + setState((prev) => ({ selectedTool: prev.selectedTool === 'stock' ? undefined : 'stock' })); }, []); const handleSelectFlow = React.useCallback((e: React.MouseEvent): void => { e.preventDefault(); e.stopPropagation(); - setState({ - selectedTool: 'flow', - }); + setState((prev) => ({ selectedTool: prev.selectedTool === 'flow' ? undefined : 'flow' })); }, []); const handleSelectAux = React.useCallback((e: React.MouseEvent): void => { e.preventDefault(); e.stopPropagation(); - setState({ - selectedTool: 'aux', - }); + setState((prev) => ({ selectedTool: prev.selectedTool === 'aux' ? undefined : 'aux' })); }, []); const handleSelectLink = React.useCallback((e: React.MouseEvent): void => { e.preventDefault(); e.stopPropagation(); - setState({ - selectedTool: 'link', - }); + setState((prev) => ({ selectedTool: prev.selectedTool === 'link' ? undefined : 'link' })); }, []); const handleSelectModule = React.useCallback((e: React.MouseEvent): void => { e.preventDefault(); e.stopPropagation(); - setState({ - selectedTool: 'module', - }); + setState((prev) => ({ selectedTool: prev.selectedTool === 'module' ? undefined : 'module' })); }, []); // Undo/redo is fully owned by the controller: it moves the undo cursor, diff --git a/src/diagram/tests/editor-tool-deselect.test.ts b/src/diagram/tests/editor-tool-deselect.test.ts new file mode 100644 index 000000000..93695ea76 --- /dev/null +++ b/src/diagram/tests/editor-tool-deselect.test.ts @@ -0,0 +1,165 @@ +/** + * @jest-environment jsdom + * + * Copyright 2026 The Simlin Authors. All rights reserved. + * Use of this source code is governed by the Apache License, + * Version 2.0, that can be found in the LICENSE file. + */ + +// Tool deselection behavior (regressed in 5191a9b6, which dropped the +// selectedTool clearing from handleDialClick): the user expects that +// (1) re-clicking the currently-selected tool deselects it, and +// (2) closing the tool palette (the FAB) deselects the active tool. +// On load no tool is selected; both gestures must return to that state. +// +// We assert against OBSERVABLE output: render a real , drive the +// SpeedDial's FAB and action buttons, and read each tool's `selected` flag +// (surfaced as data-selected on the mocked SpeedDialAction button). + +import { TextEncoder, TextDecoder } from 'util'; +Object.assign(globalThis, { TextEncoder, TextDecoder }); + +import * as React from 'react'; +import { act, fireEvent, render, screen } from '@testing-library/react'; + +import { ProjectController, type ProjectSnapshot } from '../project-controller'; + +// Mock SpeedDial so its FAB and action children are always rendered and +// queryable. The FAB carries aria-label "dial-fab" and fires the dial's +// onClick (handleDialClick); each action surfaces its title as aria-label and +// its `selected` flag as data-selected, and fires its onClick handler. +jest.mock('../components/SpeedDial', () => { + const react = jest.requireActual('react') as typeof import('react'); + return { + __esModule: true, + default: (p: { children?: React.ReactNode; onClick?: (e: unknown) => void }) => + react.createElement( + 'div', + null, + react.createElement('button', { + type: 'button', + 'aria-label': 'dial-fab', + onClick: (e: unknown) => p.onClick?.(e), + }), + p.children, + ), + SpeedDialAction: (p: { title: string; selected?: boolean; onClick?: (e: unknown) => void }) => + react.createElement('button', { + type: 'button', + 'aria-label': p.title, + 'data-selected': p.selected ? 'true' : 'false', + onClick: (e: unknown) => p.onClick?.(e), + }), + SpeedDialIcon: () => null, + }; +}); + +// Canvas mounts a ResizeObserver and reads SVG geometry jsdom lacks; this test +// exercises only the toolbar, so stub Canvas to a null renderer. +jest.mock('../drawing/Canvas', () => ({ + __esModule: true, + Canvas: () => null, + inCreationUid: -2, +})); + +import { Editor, type EditorProps } from '../Editor'; + +function makeSnapshot(): ProjectSnapshot { + const view = { + nextUid: 1, + elements: [], + viewBox: { x: 0, y: 0, width: 800, height: 600 }, + zoom: 1, + useLetteredPolarity: false, + }; + return { + project: { + name: 'test-project', + models: new Map([['main', { name: 'main', variables: new Map(), views: [view], loopMetadata: [], groups: [] }]]), + simSpecs: { start: 0, stop: 100, dt: { isReciprocal: false, value: 1 }, timeUnits: 'years' }, + }, + modelName: 'main', + projectVersion: 1, + projectGeneration: 0, + status: 'ok', + cachedErrors: { simError: undefined, modelErrors: [], varErrors: new Map(), unitErrors: new Map() }, + data: new Map(), + modelStack: [], + canUndo: false, + canRedo: false, + navResetSeq: 0, + } as unknown as ProjectSnapshot; +} + +function makeProps(overrides: Partial = {}): EditorProps { + return { + inputFormat: 'json', + initialProjectJson: '{}', + initialProjectVersion: 1, + name: 'test-project', + embedded: false, + readOnlyMode: false, + onSave: async () => 1, + ...overrides, + } as EditorProps; +} + +function renderEditor(props: EditorProps = makeProps()): void { + act(() => { + render(React.createElement(Editor, props)); + }); +} + +const toolSelected = (title: string): boolean => screen.getByLabelText(title).getAttribute('data-selected') === 'true'; + +describe('Editor tool deselection', () => { + beforeEach(() => { + jest.spyOn(ProjectController.prototype, 'getSnapshot').mockReturnValue(makeSnapshot()); + jest.spyOn(ProjectController.prototype, 'openInitialProject').mockResolvedValue(undefined); + jest.spyOn(ProjectController.prototype, 'dispose').mockResolvedValue(undefined); + jest.spyOn(ProjectController.prototype, 'scheduleSimRun').mockImplementation(() => {}); + jest.spyOn(ProjectController.prototype, 'subscribe').mockReturnValue(() => {}); + }); + + afterEach(() => { + jest.restoreAllMocks(); + }); + + test('no tool is selected on load', () => { + renderEditor(); + expect(toolSelected('Flow')).toBe(false); + expect(toolSelected('Stock')).toBe(false); + }); + + test('clicking a tool selects it; re-clicking the active tool deselects it', () => { + renderEditor(); + fireEvent.click(screen.getByLabelText('dial-fab')); // open the palette (real UX: tools only show when open) + fireEvent.click(screen.getByLabelText('Flow')); + expect(toolSelected('Flow')).toBe(true); + + fireEvent.click(screen.getByLabelText('Flow')); + expect(toolSelected('Flow')).toBe(false); + }); + + test('clicking a different tool switches selection (does not toggle off)', () => { + renderEditor(); + fireEvent.click(screen.getByLabelText('dial-fab')); // open the palette + fireEvent.click(screen.getByLabelText('Flow')); + expect(toolSelected('Flow')).toBe(true); + + fireEvent.click(screen.getByLabelText('Stock')); + expect(toolSelected('Stock')).toBe(true); + expect(toolSelected('Flow')).toBe(false); + }); + + test('closing the palette (FAB) deselects the active tool', () => { + renderEditor(); + // Open the dial, select a tool, then close the dial. + fireEvent.click(screen.getByLabelText('dial-fab')); // open + fireEvent.click(screen.getByLabelText('Flow')); // select + expect(toolSelected('Flow')).toBe(true); + + fireEvent.click(screen.getByLabelText('dial-fab')); // close -> deselect + expect(toolSelected('Flow')).toBe(false); + }); +}); From 059f38b02d20282beb9d48d401a15b4e300389a0 Mon Sep 17 00:00:00 2001 From: Bobby Powers Date: Mon, 29 Jun 2026 20:56:48 -0700 Subject: [PATCH 2/9] diagram: pin a new flow's sink to the stock edge, not its center Creating a flow by dragging its sink onto a stock attached the sink to the stock's center coordinates with a midpoint valve and no orthogonal routing (71752f3b), so the arrowhead sat behind the stock and the flow could be diagonal. That commit was working around an earlier zero-delta route that collapsed the sink onto the source column. Route the in-creation sink the same way an existing flow's endpoint reattaches (reattachEndpoint): place the target at the old sink point and move it to the stock via UpdateCloudAndFlow, which picks the axis from the drag direction, aligns the sink to the source's axis, and clips it to the stock face. The source stays fixed and the valve lands on the segment. A flow sink only ever targets a stock (isValidTarget gates the canvas to stocks), so the dead cloud-target write-back is dropped. --- src/diagram/flow-attach.ts | 36 ++++++++++------- src/diagram/tests/flow-attach.test.ts | 57 ++++++++++++++++++++++----- 2 files changed, 70 insertions(+), 23 deletions(-) diff --git a/src/diagram/flow-attach.ts b/src/diagram/flow-attach.ts index ba735fe07..3b95c9534 100644 --- a/src/diagram/flow-attach.ts +++ b/src/diagram/flow-attach.ts @@ -341,28 +341,36 @@ export function computeFlowAttachment( const lastPt = last(flow.points); if (lastPt.attachedToUid === fauxCloudTargetUid) { if (targetUid) { - // Attaching the new flow's sink to an existing stock/cloud. The + // Attaching the new flow's sink to an existing stock. Route it exactly + // the way an existing flow's endpoint reattaches (see reattachEndpoint): + // pin the sink to the stock's EDGE and keep the flow orthogonal. The // in-creation flow's points are degenerate -- both sit at the press // point, since the drag offset is applied only at render time and never - // committed back to the element. Routing that degenerate flow through - // UpdateCloudAndFlow with a zero delta collapses the sink onto the - // source's column (the zero-delta branch defaults to a vertical axis and - // sets sink.x = source.x), leaving the flow attached to the target by - // uid but drawn back at the press point. Instead snap the sink point's - // coordinates onto the target's center -- matching the element-centered - // point convention used everywhere else -- and re-center the valve so - // the created flow renders connected. + // committed back to the element -- so we drive UpdateCloudAndFlow from + // the current sink point: place the target at the old sink position, then + // move it to the stock by the resulting delta. UpdateCloudAndFlow picks + // the axis from that delta, aligns the sink to the source's axis, and + // clips it to the stock face (a degenerate flow stays straight). This + // replaces an earlier snap-to-center that drew the arrowhead behind the + // stock; a prior zero-delta route had instead collapsed the sink onto the + // source column, which is why the center snap was tried. const to = getUid(targetUid) as StockViewElement | CloudViewElement; - state.stockAttachingIdent = defined(to.ident); - const src = first(flow.points); + if (to.type === 'stock') { + state.stockAttachingIdent = defined(to.ident); + } + const oldSink = last(flow.points); flow = { ...flow, points: flow.points.map((pt) => - pt.attachedToUid === fauxCloudTargetUid ? { ...pt, x: to.x, y: to.y, attachedToUid: to.uid } : pt, + pt.attachedToUid === fauxCloudTargetUid ? { ...pt, attachedToUid: to.uid } : pt, ), - x: (src.x + to.x) / 2, - y: (src.y + to.y) / 2, }; + const sinkDelta = { x: oldSink.x - to.x, y: oldSink.y - to.y }; + const targetAtOldSink = { ...to, x: oldSink.x, y: oldSink.y } as StockViewElement | CloudViewElement; + // A flow sink only ever attaches to a stock (isValidTarget gates the + // canvas to stock targets), and a stock keeps its position in the view -- + // so we discard the routed endpoint and keep only the re-routed flow. + [, flow] = UpdateCloudAndFlow(targetAtOldSink, flow, sinkDelta); } else { let to: StockViewElement | CloudViewElement = { type: 'cloud' as const, diff --git a/src/diagram/tests/flow-attach.test.ts b/src/diagram/tests/flow-attach.test.ts index b2c0c6bad..3d59884e7 100644 --- a/src/diagram/tests/flow-attach.test.ts +++ b/src/diagram/tests/flow-attach.test.ts @@ -26,6 +26,7 @@ import { inCreationCloudUid as canvasInCreationCloudUid, inCreationUid as canvasInCreationUid, } from '../drawing/Canvas'; +import { StockWidth, StockHeight } from '../drawing/default'; // ----- fixture helpers (mirroring flow-routing.test.ts patterns) ----- @@ -422,13 +423,19 @@ describe('computeFlowAttachment', () => { expect(snkOp.inflows).toEqual([flowIdent]); }); - it('cloud source to target stock: sink attaches to the stock, no sink cloud created', () => { - // The exact UI scenario: flow tool, press on empty space (source cloud), - // drag the sink onto a stock, release. targetUid is the stock. + it('cloud source to target stock: sink pins to the stock EDGE, flow horizontal, valve on the segment', () => { + // The exact UI scenario, faithful to runtime: the flow tool stages a + // DEGENERATE in-creation flow (both points AND the valve at the press + // point, since the drag offset is applied only at render time and never + // committed back to the element); the user drags the sink onto a stock and + // releases (targetUid = the stock). The sink must land on the stock's EDGE + // (not its center -- the center put the arrowhead behind the stock, the + // 71752f3b regression), the flow must be orthogonal, and the valve must sit + // ON the source->sink segment (not at/behind the stock center). const sinkStock = makeStockEl(1, 'snk', 300, 100); - const flow = makeFlowEl(inCreationUid, 'new_flow', 150, 100, [ + const flow = makeFlowEl(inCreationUid, 'new_flow', 50, 100, [ { x: 50, y: 100, attachedToUid: inCreationCloudUid }, - { x: 280, y: 100, attachedToUid: fauxCloudTargetUid }, + { x: 50, y: 100, attachedToUid: fauxCloudTargetUid }, ]); const view = makeView([sinkStock], 5); const variables = varsOf(makeStockVar('snk')); @@ -436,13 +443,23 @@ describe('computeFlowAttachment', () => { const result = computeFlowAttachment(view, variables, params({ flow, targetUid: 1, inCreation: true })); const realFlow = result.elements.find((e) => e.type === 'flow') as FlowViewElement; + const sourcePt = realFlow.points[0]; const sinkPt = realFlow.points[realFlow.points.length - 1]; // the sink point attaches to the stock (uid 1), not a freshly-created cloud expect(sinkPt.attachedToUid).toBe(1); - // ...and its coordinates land on the stock so the flow renders connected, - // rather than collapsing back onto the source/press point. - expect(sinkPt.x).toBe(300); - expect(sinkPt.y).toBe(100); + // ...landing on the stock's LEFT edge (center.x - StockWidth/2), NOT its + // center: the source is to the left, so the flow enters the left face. + expect(sinkPt.x).toBeCloseTo(300 - StockWidth / 2); + expect(sinkPt.x).not.toBe(300); + // ...and the flow is horizontal: the sink shares the source's y. + expect(sinkPt.y).toBe(sourcePt.y); + // a straight (2-point) orthogonal segment + expect(realFlow.points.length).toBe(2); + // the valve sits strictly between the source and the (edge-clipped) sink, + // i.e. on the visible segment -- never at/behind the stock center. + expect(realFlow.x).toBeGreaterThan(sourcePt.x); + expect(realFlow.x).toBeLessThan(sinkPt.x); + expect(realFlow.y).toBe(sourcePt.y); // only the source cloud is materialized; no sink cloud expect(result.elements.filter((e) => e.type === 'cloud').length).toBe(1); // the stock gains the flow as an inflow @@ -450,6 +467,28 @@ describe('computeFlowAttachment', () => { expect(snkOp.inflows).toEqual([realFlow.ident]); }); + it('cloud source above a target stock: sink pins to the TOP edge and the flow is vertical', () => { + // Press above the stock, drag straight down onto it: the flow must route + // vertically and pin to the stock's top edge (center.y - StockHeight/2). + const sinkStock = makeStockEl(1, 'snk', 300, 200); + const flow = makeFlowEl(inCreationUid, 'new_flow', 300, 120, [ + { x: 300, y: 50, attachedToUid: inCreationCloudUid }, + { x: 300, y: 180, attachedToUid: fauxCloudTargetUid }, + ]); + const view = makeView([sinkStock], 5); + const variables = varsOf(makeStockVar('snk')); + + const result = computeFlowAttachment(view, variables, params({ flow, targetUid: 1, inCreation: true })); + + const realFlow = result.elements.find((e) => e.type === 'flow') as FlowViewElement; + const sourcePt = realFlow.points[0]; + const sinkPt = realFlow.points[realFlow.points.length - 1]; + expect(sinkPt.attachedToUid).toBe(1); + expect(sinkPt.y).toBeCloseTo(200 - StockHeight / 2); // top edge, not center + expect(sinkPt.y).not.toBe(200); + expect(sinkPt.x).toBe(sourcePt.x); // vertical + }); + it('stock source to empty space: faux target becomes a new cloud at fauxTargetCenter', () => { const srcStock = makeStockEl(1, 'src', 0, 100); const flow = makeFlowEl(inCreationUid, 'new_flow', 150, 100, [ From 4071a9fd6fc86da6875e025725722cfeb69a86bf Mon Sep 17 00:00:00 2001 From: Bobby Powers Date: Mon, 29 Jun 2026 21:36:09 -0700 Subject: [PATCH 3/9] diagram: grow a new flow's preview toward the cursor Dragging out a flow with the flow tool showed nothing while the mouse moved: the tool stages a degenerate flow (both points at the press point) and records the drag only as moveDelta, and applyGroupMovement translated that degenerate cloud-to-cloud flow rigidly, so it stayed zero-length and invisible until release. Derive the preview in deriveRenderState instead: a new pure helper, growInCreationFlow, routes the sink toward the cursor (over empty space) or onto a hovered stock's edge, keeping the source fixed and the flow orthogonal -- the same routing computeFlowAttachment uses on release, so the preview matches the committed result. applyGroupMovement's UpdateFlow also drags the source cloud to the cursor (its cloud-to-cloud case translates both clouds), so the source cloud is restored to its staged position after the override. Also fix a latent degenerate-axis bug in adjustFlows: with both points coincident the perpendicular component is zeroed upstream, and the signed 'd.x > d.y' axis pick then collapsed to 'd.x > 0' / '0 > d.y' -- correct for rightward/downward drags but leaving leftward/upward ones degenerate. Compare magnitudes instead. This guard runs only for fully degenerate (in-creation) flows, so non-degenerate routing is unchanged; the fix covers both the live preview and the commit path. --- src/diagram/drawing/Canvas.tsx | 28 ++++ src/diagram/drawing/Flow.tsx | 8 +- src/diagram/flow-attach.ts | 48 ++++++ .../tests/canvas-gestures-elements.test.tsx | 141 ++++++++++++++++++ src/diagram/tests/flow-attach.test.ts | 131 ++++++++++++++++ 5 files changed, 355 insertions(+), 1 deletion(-) diff --git a/src/diagram/drawing/Canvas.tsx b/src/diagram/drawing/Canvas.tsx index fb35ce34a..34aed3dbb 100644 --- a/src/diagram/drawing/Canvas.tsx +++ b/src/diagram/drawing/Canvas.tsx @@ -52,6 +52,7 @@ import { Connector, ConnectorProps, computeLinkCreationArc } from './Connector'; import { EditableLabel } from './EditableLabel'; import { Flow, flowBounds } from './Flow'; import { applyGroupMovement } from '../group-movement'; +import { growInCreationFlow } from '../flow-attach'; import { Group, groupBounds, GroupProps } from './Group'; import { Module, moduleBounds, moduleContains, ModuleProps } from './Module'; import { anyModuleHasModelReference } from '../module-warning'; @@ -890,6 +891,33 @@ export const Canvas = React.memo(function Canvas(props: CanvasProps): React.Reac selectionUpdates = new Map([...selectionUpdates, ...updatedElements]); } + // Grow the in-creation flow's live preview. The flow tool stages a degenerate + // flow (both points at the press point) and records the drag only as + // moveDelta; applyGroupMovement can't grow it (a cloud->cloud flow translates + // rigidly, so it stays zero-length and invisible). Route it here the way the + // commit (computeFlowAttachment) does: the sink follows the cursor, or snaps + // onto a hovered stock's edge, with the source fixed and the flow orthogonal, + // so the preview matches what releasing the drag will produce. + if (inCreationNow?.type === 'flow' && latest.current.moveDelta && isDraggingArrowhead(latest.current.interaction)) { + let previewTarget: StockViewElement | undefined; + for (const el of displayElements) { + if (el.type === 'stock' && isValidTarget(el)) { + previewTarget = el; + break; + } + } + const grown = growInCreationFlow(inCreationNow, defined(latest.current.moveDelta), previewTarget); + selectionUpdates = new Map(selectionUpdates); + selectionUpdates.set(inCreationUid, grown); + // applyGroupMovement's single-flow path runs UpdateFlow, whose cloud->cloud + // case rigidly translates BOTH clouds by moveDelta -- dragging the source + // cloud along to the cursor. Restore it to its staged position so the flow + // grows from a planted tail. (The faux sink cloud isn't rendered.) + if (inCreationCloudNow) { + selectionUpdates.set(inCreationCloudUid, inCreationCloudNow); + } + } + const derived: RenderDerivation = { displayElements, elementsByUid: r.elements, diff --git a/src/diagram/drawing/Flow.tsx b/src/diagram/drawing/Flow.tsx index 81620d102..edfb16063 100644 --- a/src/diagram/drawing/Flow.tsx +++ b/src/diagram/drawing/Flow.tsx @@ -529,7 +529,13 @@ function adjustFlows( }; if (inCreation) { - horizontal = d.x > d.y; + // The flow is degenerate (all points coincide), so its own orientation + // can't pick the axis -- recover it from the larger-magnitude component + // of the source->target delta. A signed `d.x > d.y` here silently left + // leftward/upward creations degenerate: the perpendicular component is + // zeroed upstream, so the signed test collapsed to `d.x > 0` / `0 > d.y`, + // which is wrong for the negative direction on each axis. + horizontal = Math.abs(d.x) > Math.abs(d.y); } const adjust = { diff --git a/src/diagram/flow-attach.ts b/src/diagram/flow-attach.ts index 3b95c9534..190b5bdb3 100644 --- a/src/diagram/flow-attach.ts +++ b/src/diagram/flow-attach.ts @@ -468,3 +468,51 @@ export function computeFlowAttachment( isCreatingNew, }; } + +/** + * Compute the GROWN geometry of the in-creation flow for the live drag preview. + * + * The flow tool stages a degenerate flow (both points at the press point) and + * records the drag only as `moveDelta` (= press - cursor, the Canvas + * convention); it never rewrites the staged flow's points. So the preview has + * to be derived here: route the sink toward the cursor (over empty space) or + * onto a hovered stock's edge, keeping the SOURCE fixed and the flow + * orthogonal. This mirrors what `computeFlowAttachment` produces on release, so + * the live preview matches the committed result. + * + * `target` is the valid stock under the cursor (or undefined over empty space). + * Pure: returns a new flow element; the caller swaps it into the render. + */ +export function growInCreationFlow( + flow: FlowViewElement, + moveDelta: CanvasPoint, + target: StockViewElement | CloudViewElement | undefined, +): FlowViewElement { + const sinkPt = last(flow.points); + if (target !== undefined) { + // Hovering a valid stock: pin the sink to its edge -- the same routing the + // commit (computeFlowAttachment) and reattachEndpoint use. Temporarily + // attach the sink to the target so UpdateCloudAndFlow can route and clip it; + // the real attachment is (re)computed on release. + const attached: FlowViewElement = { + ...flow, + points: flow.points.map((pt, i) => (i === flow.points.length - 1 ? { ...pt, attachedToUid: target.uid } : pt)), + }; + const sinkDelta = { x: sinkPt.x - target.x, y: sinkPt.y - target.y }; + const targetAtOldSink = { ...target, x: sinkPt.x, y: sinkPt.y } as StockViewElement | CloudViewElement; + return UpdateCloudAndFlow(targetAtOldSink, attached, sinkDelta)[1]; + } + // Over empty space: the sink follows the cursor. Move the faux sink cloud (the + // sink point's current attachment) from the press point by moveDelta; + // UpdateCloudAndFlow picks the axis from moveDelta and keeps the source fixed. + const sinkCloud: CloudViewElement = { + type: 'cloud', + uid: sinkPt.attachedToUid ?? fauxCloudTargetUid, + flowUid: flow.uid, + x: sinkPt.x, + y: sinkPt.y, + isZeroRadius: false, + ident: undefined, + }; + return UpdateCloudAndFlow(sinkCloud, flow, moveDelta)[1]; +} diff --git a/src/diagram/tests/canvas-gestures-elements.test.tsx b/src/diagram/tests/canvas-gestures-elements.test.tsx index 479eb1f9b..eb06e8798 100644 --- a/src/diagram/tests/canvas-gestures-elements.test.tsx +++ b/src/diagram/tests/canvas-gestures-elements.test.tsx @@ -28,6 +28,7 @@ import { renderCanvas, type CanvasHarness, } from './canvas-gesture-harness'; +import { CloudRadius, StockWidth } from '../drawing/default'; function lastSelection(fn: jest.Mock): number[] { const calls = fn.mock.calls; @@ -35,6 +36,42 @@ function lastSelection(fn: jest.Mock): number[] { return last ? [...(last[0] as Set)].sort((a, b) => a - b) : []; } +// Parse the rendered flow path's polyline points from its `d` attribute +// (e.g. "M100,200L180,200" -> [[100,200],[180,200]]). The inner flow path is +// drawn straight from the flow element's points, so this reflects the live +// geometry the user sees. NOTE: the line's final point is pulled back by a fixed +// glyph inset (finalAdjust) to leave room for the arrowhead, so use this for +// growth/orientation -- not for the exact endpoint (use arrowheadPoint for that). +function flowPoints(h: CanvasHarness): Array<[number, number]> { + const inner = h.query('.simlin-flow .simlin-inner'); + const d = inner?.getAttribute('d') ?? ''; + const nums = (d.match(/-?\d+(?:\.\d+)?/g) ?? []).map(Number); + const pts: Array<[number, number]> = []; + for (let i = 0; i + 1 < nums.length; i += 2) { + pts.push([nums[i], nums[i + 1]]); + } + return pts; +} + +// The flow arrowhead is drawn at the TRUE sink endpoint -- where the arrow +// actually points -- via transform="rotate(angle, x, y)". Extract (x, y). +function arrowheadPoint(h: CanvasHarness): [number, number] { + const head = h.query('.simlin-arrowhead-bg'); + const t = head?.getAttribute('transform') ?? ''; + const m = t.match(/rotate\([^,]+,\s*(-?\d+(?:\.\d+)?),\s*(-?\d+(?:\.\d+)?)\)/); + return m ? [Number(m[1]), Number(m[2])] : [NaN, NaN]; +} + +// Clouds render via transform="matrix(sx,0,0,sy, x-radius, y-radius)"; recover +// each cloud's center as (translateX + radius, translateY + radius). +function cloudCenters(h: CanvasHarness): Array<[number, number]> { + return h.queryAll('.simlin-cloud').map((el) => { + const t = el.getAttribute('transform') ?? ''; + const m = t.match(/matrix\([^,]+,[^,]+,[^,]+,[^,]+,\s*(-?\d+(?:\.\d+)?),\s*(-?\d+(?:\.\d+)?)\)/); + return m ? [Number(m[1]) + CloudRadius, Number(m[2]) + CloudRadius] : [NaN, NaN]; + }); +} + // A horizontal flow stock -> cloud, with the stock wired to the flow. function stockToCloudFlow(): CanvasHarness { const stock = makeStock(1, 'stock', 100, 100); @@ -348,6 +385,110 @@ describe('Canvas gestures: creation tools (checklist 12)', () => { }); }); +describe('Canvas gestures: flow tool live preview (a)', () => { + // As the user drags the flow tool, the in-creation flow must GROW toward the + // cursor as an orthogonal segment (it previously rendered as a zero-length, + // invisible path), and snap to the stock's edge when the cursor is over a + // valid stock. We assert on the rendered flow path geometry, not just presence. + + it('the in-creation flow grows orthogonally toward the cursor (not degenerate)', () => { + const stock = makeStock(1, 'pop', 300, 200); + const h = renderCanvas({ elements: [stock], selectedTool: 'flow' }); + h.clearMountCalls(); + + pointerDown(h.svg, 100, 200); // press empty space, level with the stock, to its left + pointerMove(h.svg, 180, 200, { buttons: 1 }); // drag right, NOT yet over the stock + + const line = flowPoints(h); + expect(line.length).toBeGreaterThanOrEqual(2); + const lineStart = line[0]; + const lineEnd = line[line.length - 1]; + // grew a visible length (was a zero-length, invisible path before the fix) + expect(Math.hypot(lineEnd[0] - lineStart[0], lineEnd[1] - lineStart[1])).toBeGreaterThan(50); + // horizontal (orthogonal): the line stays on the source's y + expect(lineEnd[1]).toBeCloseTo(lineStart[1]); + // the arrowhead (true endpoint) points right at the cursor + const head = arrowheadPoint(h); + expect(head[0]).toBeCloseTo(180); + expect(head[1]).toBeCloseTo(200); + }); + + it('the in-creation flow snaps to the stock edge when the cursor is over a stock', () => { + const stock = makeStock(1, 'pop', 300, 200); + const h = renderCanvas({ elements: [stock], selectedTool: 'flow' }); + h.clearMountCalls(); + + pointerDown(h.svg, 100, 200); // press level with the stock + pointerMove(h.svg, 180, 200, { buttons: 1 }); + pointerMove(h.svg, 295, 200, { buttons: 1 }); // cursor now over the stock (center 300) + + const head = arrowheadPoint(h); + // pinned to the stock's LEFT edge (300 - StockWidth/2 = 277.5), NOT the cursor + // (295) and NOT the stock center (300, which drew the arrowhead behind it) + expect(head[0]).toBeCloseTo(300 - StockWidth / 2); + expect(head[0]).not.toBeCloseTo(300); + expect(head[0]).toBeLessThan(295); // snapped back to the edge, left of the cursor + expect(head[1]).toBeCloseTo(200); // still horizontal + }); + + it('a dominant vertical drag grows a vertical in-creation flow', () => { + const h = renderCanvas({ elements: [], selectedTool: 'flow' }); + h.clearMountCalls(); + + pointerDown(h.svg, 200, 100); // press empty space + pointerMove(h.svg, 200, 220, { buttons: 1 }); // drag straight down + + const line = flowPoints(h); + const lineStart = line[0]; + const lineEnd = line[line.length - 1]; + expect(Math.abs(lineEnd[1] - lineStart[1])).toBeGreaterThan(50); // grew downward + // the arrowhead points straight down at the cursor (x unchanged, vertical) + const head = arrowheadPoint(h); + expect(head[0]).toBeCloseTo(200); + expect(head[1]).toBeCloseTo(220); + }); + + it('grows LEFTWARD toward the cursor (not degenerate)', () => { + const h = renderCanvas({ elements: [], selectedTool: 'flow' }); + h.clearMountCalls(); + + pointerDown(h.svg, 300, 200); // press empty space + pointerMove(h.svg, 220, 200, { buttons: 1 }); // drag LEFT + + const line = flowPoints(h); + expect(Math.abs(line[line.length - 1][0] - line[0][0])).toBeGreaterThan(50); // grew + const head = arrowheadPoint(h); + expect(head[0]).toBeCloseTo(220); // arrowhead at the cursor, to the left + expect(head[1]).toBeCloseTo(200); + }); + + it('grows UPWARD toward the cursor (not degenerate)', () => { + const h = renderCanvas({ elements: [], selectedTool: 'flow' }); + h.clearMountCalls(); + + pointerDown(h.svg, 200, 300); // press empty space + pointerMove(h.svg, 200, 220, { buttons: 1 }); // drag UP + + const head = arrowheadPoint(h); + expect(head[0]).toBeCloseTo(200); // vertical + expect(head[1]).toBeCloseTo(220); // arrowhead at the cursor, above + }); + + it('plants the source cloud at the tail, not on the arrowhead at the cursor', () => { + const h = renderCanvas({ elements: [], selectedTool: 'flow' }); + h.clearMountCalls(); + + pointerDown(h.svg, 100, 200); // press empty space -> source cloud planted here + pointerMove(h.svg, 180, 200, { buttons: 1 }); // drag right + + const centers = cloudCenters(h); + // the source cloud stays at the tail (the press point)... + expect(centers.some(([x, y]) => Math.abs(x - 100) < 1 && Math.abs(y - 200) < 1)).toBe(true); + // ...and does NOT ride along to the cursor/arrowhead at x=180 + expect(centers.some(([x]) => Math.abs(x - 180) < 1)).toBe(false); + }); +}); + describe('Canvas gestures: link/flow tool from a named element (checklist 13)', () => { it('link tool pressing a named element starts an inCreation link drag that attaches on release', () => { const a = makeAux(1, 'a', 100, 100); diff --git a/src/diagram/tests/flow-attach.test.ts b/src/diagram/tests/flow-attach.test.ts index 3d59884e7..40955bffb 100644 --- a/src/diagram/tests/flow-attach.test.ts +++ b/src/diagram/tests/flow-attach.test.ts @@ -17,6 +17,7 @@ import type { JsonModelOperation } from '@simlin/engine'; import { computeFlowAttachment, fauxCloudTargetUid, + growInCreationFlow, inCreationCloudUid, inCreationUid, type FlowAttachParams, @@ -489,6 +490,31 @@ describe('computeFlowAttachment', () => { expect(sinkPt.x).toBe(sourcePt.x); // vertical }); + it('degenerate creation onto a stock to the LEFT: edge-pinned, not degenerate', () => { + // Faithful to runtime: the in-creation flow is degenerate (both points at + // the press point). Releasing on a stock to the LEFT of the press must pin + // the sink to the stock's RIGHT edge -- a signed axis comparison in + // adjustFlows previously left this (and upward) degenerate on the commit + // path, not just the preview. + const sinkStock = makeStockEl(1, 'snk', 40, 200); // stock to the left of the press + const flow = makeFlowEl(inCreationUid, 'new_flow', 100, 200, [ + { x: 100, y: 200, attachedToUid: inCreationCloudUid }, + { x: 100, y: 200, attachedToUid: fauxCloudTargetUid }, + ]); + const view = makeView([sinkStock], 5); + const variables = varsOf(makeStockVar('snk')); + + const result = computeFlowAttachment(view, variables, params({ flow, targetUid: 1, inCreation: true })); + + const realFlow = result.elements.find((e) => e.type === 'flow') as FlowViewElement; + const sourcePt = realFlow.points[0]; + const sinkPt = realFlow.points[realFlow.points.length - 1]; + expect(sinkPt.attachedToUid).toBe(1); + expect(sinkPt.x).toBeCloseTo(40 + StockWidth / 2); // 62.5: right edge + expect(sinkPt.x).not.toBe(100); // not degenerate / stuck at the press point + expect(sinkPt.y).toBe(sourcePt.y); // horizontal + }); + it('stock source to empty space: faux target becomes a new cloud at fauxTargetCenter', () => { const srcStock = makeStockEl(1, 'src', 0, 100); const flow = makeFlowEl(inCreationUid, 'new_flow', 150, 100, [ @@ -757,3 +783,108 @@ describe('computeFlowAttachment', () => { }); }); }); + +describe('growInCreationFlow (live drag preview)', () => { + // The in-creation flow is degenerate while dragging: both points sit at the + // press point, and the drag is recorded only as moveDelta (= press - cursor). + function degenerateFlow(px: number, py: number): FlowViewElement { + return makeFlowEl(inCreationUid, 'new_flow', px, py, [ + { x: px, y: py, attachedToUid: inCreationCloudUid }, + { x: px, y: py, attachedToUid: fauxCloudTargetUid }, + ]); + } + + it('over empty space: the sink follows the cursor along a horizontal segment', () => { + const flow = degenerateFlow(100, 200); + // cursor 80px to the right of the press point -> moveDelta = press - cursor. + const grown = growInCreationFlow(flow, { x: -80, y: 0 }, undefined); + + expect(grown.points.length).toBe(2); + const src = grown.points[0]; + const sink = grown.points[grown.points.length - 1]; + // source stays at the press point + expect(src.x).toBe(100); + expect(src.y).toBe(200); + // sink reaches the cursor; flow is horizontal (sink shares source y) + expect(sink.x).toBeCloseTo(180); + expect(sink.y).toBe(200); + // the sink still references the faux cursor target during the preview + expect(sink.attachedToUid).toBe(fauxCloudTargetUid); + }); + + it('over empty space: a dominant vertical drag grows a vertical segment', () => { + const flow = degenerateFlow(100, 200); + // cursor 80px below -> dominant y component picks the vertical axis. + const grown = growInCreationFlow(flow, { x: 0, y: -80 }, undefined); + + const src = grown.points[0]; + const sink = grown.points[grown.points.length - 1]; + expect(sink.x).toBe(src.x); // vertical + expect(sink.y).toBeCloseTo(280); + }); + + it('over a stock: the sink snaps to the stock EDGE (not the cursor or center)', () => { + const flow = degenerateFlow(100, 200); + const stock = makeStockEl(1, 'snk', 300, 200); + // moveDelta is irrelevant once a target is hovered; the sink pins to the edge. + const grown = growInCreationFlow(flow, { x: -200, y: 0 }, stock); + + const src = grown.points[0]; + const sink = grown.points[grown.points.length - 1]; + expect(sink.attachedToUid).toBe(1); + expect(sink.x).toBeCloseTo(300 - StockWidth / 2); // left edge, not center (300) + expect(sink.x).not.toBe(300); + expect(sink.y).toBe(src.y); // horizontal + expect(src.x).toBe(100); // source unchanged + }); + + // The degenerate axis-recovery in adjustFlows must work in BOTH directions on + // each axis; a signed comparison there silently left LEFT/UP drags degenerate. + it('over empty space: the sink follows a LEFTWARD cursor (not degenerate)', () => { + const flow = degenerateFlow(100, 200); + // cursor 80px to the LEFT -> moveDelta = press - cursor = +80. + const grown = growInCreationFlow(flow, { x: 80, y: 0 }, undefined); + + const src = grown.points[0]; + const sink = grown.points[grown.points.length - 1]; + expect(sink.x).toBeCloseTo(20); // reached the cursor to the left + expect(sink.x).not.toBe(100); // not stuck at the press point + expect(sink.y).toBe(src.y); // horizontal + }); + + it('over empty space: the sink follows an UPWARD cursor (not degenerate)', () => { + const flow = degenerateFlow(100, 200); + // cursor 80px ABOVE -> moveDelta = press - cursor = +80 in y. + const grown = growInCreationFlow(flow, { x: 0, y: 80 }, undefined); + + const src = grown.points[0]; + const sink = grown.points[grown.points.length - 1]; + expect(sink.x).toBe(src.x); // vertical + expect(sink.y).toBeCloseTo(120); // reached the cursor above + expect(sink.y).not.toBe(200); // not stuck at the press point + }); + + it('over a stock to the LEFT: the sink snaps to the stock RIGHT edge', () => { + const flow = degenerateFlow(100, 200); + const stock = makeStockEl(1, 'snk', 40, 200); // stock to the left of the press + const grown = growInCreationFlow(flow, { x: 0, y: 0 }, stock); + + const sink = grown.points[grown.points.length - 1]; + expect(sink.attachedToUid).toBe(1); + expect(sink.x).toBeCloseTo(40 + StockWidth / 2); // 62.5: right edge, not the center + expect(sink.x).not.toBe(100); // not stuck at the press point + expect(sink.y).toBe(200); + }); + + it('over a stock ABOVE: the sink snaps to the stock BOTTOM edge', () => { + const flow = degenerateFlow(100, 200); + const stock = makeStockEl(1, 'snk', 100, 40); // stock above the press + const grown = growInCreationFlow(flow, { x: 0, y: 0 }, stock); + + const sink = grown.points[grown.points.length - 1]; + expect(sink.attachedToUid).toBe(1); + expect(sink.y).toBeCloseTo(40 + StockHeight / 2); // bottom edge, not the center + expect(sink.y).not.toBe(200); // not stuck at the press point + expect(sink.x).toBe(100); + }); +}); From 0829e9b7b165192d071bc63fdbcba959cc0b3454 Mon Sep 17 00:00:00 2001 From: Bobby Powers Date: Tue, 30 Jun 2026 22:34:26 -0700 Subject: [PATCH 4/9] doc: add flow-editing audit and fix LAYOUT.md drift LAYOUT.md still described flows as 2-point-only and used Immutable.js List types removed in 406a7005; the routing code has supported multi-segment flows, perpendicular-drag L conversion, interior segment drags, and same-side flow spreading for some time. The new audit doc records the June 2026 deep audit of the interactive flow-editing path (rename/stale-view corruption chain, name-editor commit semantics, orientation-classification robustness, group-movement duplication) and the decisions behind the fixes that follow in this branch, so the PR has a durable rationale. --- docs/README.md | 1 + docs/dev/flow-editing-audit-2026-06.md | 147 +++++++++++++++++++++++++ src/diagram/LAYOUT.md | 24 ++-- 3 files changed, 165 insertions(+), 7 deletions(-) create mode 100644 docs/dev/flow-editing-audit-2026-06.md diff --git a/docs/README.md b/docs/README.md index 7dc4837cd..ab607934f 100644 --- a/docs/README.md +++ b/docs/README.md @@ -19,6 +19,7 @@ - [dev/python.md](dev/python.md) -- Python (pysimlin) development standards - [dev/workflow.md](dev/workflow.md) -- Problem-solving philosophy and TDD workflow - [dev/frontend-audit-2026-06.md](dev/frontend-audit-2026-06.md) -- June 2026 frontend bug audit of src/app and src/diagram: findings, fix status, tracked follow-ups +- [dev/flow-editing-audit-2026-06.md](dev/flow-editing-audit-2026-06.md) -- June 2026 deep audit of interactive flow editing: rename/stale-view corruption chain, name-editor UX, routing robustness, group-movement dedup ## Project Management diff --git a/docs/dev/flow-editing-audit-2026-06.md b/docs/dev/flow-editing-audit-2026-06.md new file mode 100644 index 000000000..9e2faa916 --- /dev/null +++ b/docs/dev/flow-editing-audit-2026-06.md @@ -0,0 +1,147 @@ +# Flow-Editing Audit (June 2026) + +Deep audit of the interactive flow-editing path in `src/diagram` (the TypeScript +editor -- not the engine's auto-layout). Sources: full read of `drawing/Flow.tsx`, +`flow-attach.ts`, `group-movement.ts`, `drawing/Canvas.tsx`, +`drawing/canvas-interaction.ts`, the Editor/controller flow handlers, and the +flow test suites; plus live exploration against the dev server (the +`bpowers/fooz` scratch project). This document records what was found and which +fixes were chosen, so the PR built from it has a durable rationale. + +## P1: Correctness bugs found live + +### 1. Rename leaves the live view stale; the next edit persists the divergence + +`Editor.handleRename` is the only handler that mutates the view through a patch +op (`upsertView` bundled with `renameVariable`) instead of going through +`ProjectController.updateView`. The patch applies correctly in the engine and +the save is correct, but `updateProject`'s unconditional `preserveLiveView` +keeps the active model's (pre-rename) live view, so the on-screen label never +changes -- the rename looks like it silently failed. Any subsequent +geometry-editing gesture then round-trips that stale view back through +`updateView`, overwriting the engine's corrected view: the model says the new +ident, the view says the old one, and the divergence is saved. + +Observed end-to-end in `bpowers/fooz`: a flow rename left the model variable +`drain_` with a view element still named "New Flow 1"; clicking that flow's +valve crashes the editor on every page load (see finding 3). + +Fix: `handleRename` applies the content op (`renameVariable`) via +`applyPatchOrReportError` and commits the renamed view through +`updateView(updatedView, { recordHistory: true })` like every other discrete +edit, so the optimistic live view is the renamed one. Defense in depth: the +controller's `applyPatch` path now adopts the engine view (skips +`preserveLiveView`) when the patch it just applied contains an `upsertView` op +for the active model -- an explicit view op is newer user intent than the +optimistic live view by construction. + +### 2. Plain Enter does not commit a name edit + +`EditableLabel.handleKeyPress` committed only on ctrl/shift/alt+Enter; a plain +Enter inserted a newline into the (multi-line-capable) Slate label. Every +"create element, type name, press Enter" interaction therefore left the editor +open with a trailing newline staged. On click-away the name committed with the +embedded newline; `handleRename` escaped it (`replace('\n', '\\n')`, first +occurrence only) and `canonicalize` mapped the result to idents like `drain_`. + +Fix: plain Enter (and NumpadEnter) commits; shift+Enter inserts the newline +(standard convention, still supporting multi-line labels); Escape still +cancels. Name commits are sanitized: trailing/leading whitespace and blank +lines are trimmed, interior newlines are preserved as intentional line breaks, +and a commit that produces an empty name is treated as cancel (which for a +just-created flow keeps the existing delete-on-cancel semantics). + +### 3. A model/view divergence crashes the whole editor and cannot be repaired + +`Editor.getDetails` resolves the selected element's variable with `getOrThrow` +during render, so a view element whose variable is missing takes down the +entire editor via the ErrorBoundary. Because the details panel is ALSO the only +delete affordance, and there is no keyboard delete, a corrupt element cannot be +removed: clicking it to delete crashes first. (Same hardening class as the +dangling-ref rendering fixes #812/#817.) + +Fix: `getDetails` degrades gracefully (no panel + console.warn) when the +variable is missing, and keyboard deletion (finding 4) provides a +details-independent repair path. + +### 4. No keyboard delete; clouds and corrupt elements have no delete path at all + +The Editor's global keydown only handles undo/redo. Deletion requires opening +the variable-details panel -- which unnamed elements (clouds) do not have, and +which crashes for divergent elements (finding 3). + +Fix: Delete/Backspace deletes the current selection (guarded by +`isEditableElement` so text fields are unaffected, and inert while a name edit +is showing). Escape clears an armed creation tool, else the selection. + +## P2: Geometry robustness + +### 5. Slightly-diagonal legacy flows are misrouted + +All routing classification uses exact coordinate equality (`p1.y === p2.y`). +The pre-fix creation bugs left persisted flows that are a few pixels off-axis +(the fooz "New Flow" drifts 5.5px); imported models can carry the same. Such a +visually-horizontal flow classifies as *vertical* (its endpoint y's differ), so +dragging its stock routes an L the wrong way -- valve and label end up stacked +on the source cloud. + +Fix: orientation decisions in the routing paths (`computeFlowRoute` / +`getFlowAttachmentInfo` / `adjustFlows`) classify by the dominant axis of the +anchor-side segment instead of exact equality. Exact equality remains the +definition of *orthogonal* for rendering/normalization; dominance only breaks +ties for classification so near-axis data routes the way it looks. + +### 6. Flow creation leaves the source endpoint at the stock's center + +Dragging a new flow out of a stock stages the source point at the stock CENTER +and `computeFlowAttachment` routes only the sink, so the persisted source +endpoint sits at the center (hidden by z-order) instead of on the stock edge, +violating the LAYOUT.md attachment rule until the next stock drag re-pins it. + +Fix: at commit, creation routes the source point onto the stock edge facing the +sink (same `UpdateCloudAndFlow` routing the sink already gets). + +## P3: Duplication and dead weight (behavior-preserving cleanup) + +- `group-movement.ts` contains the cloud L-shape conversion three times + (`routeCloudEndpointFlow` plus two ~50-line copies inside + `routeUnselectedFlows`' source/sink loops, which are themselves + near-duplicates); the copies' `cloudIsSelected` guard is always true by + construction (the maps are keyed by selected endpoints). Extracted into one + helper; the source/sink loops parameterized. +- `preProcessSelectedFlows` returns a `sideEffects` array that is always + empty; `hasSelectedEndpoints` reduces to `selection.size > 0`; + `routeUnselectedFlows` takes two element iterables that every caller passes + identically; the deprecated `Point2D` alias. All removed/simplified. +- `processSelectedFlow` repeats the "proposed valve, clamp to new path" block + three times; extracted. +- `adjustFlows` (Flow.tsx) carries a `FIXME: reduce this duplication` -- + near-identical cloud/non-cloud valve-fraction math whose duplication caused + the #818 NaN guard to be needed twice. Unified. +- The creation sentinel UIDs are duplicated between `Canvas.tsx` and + `flow-attach.ts` with a "must stay in sync" comment and a pinning test; + moved to one shared module (`drawing/creation-sentinels.ts`) imported by + both. +- The straight-flow -> L-shape conversion exists independently in + `UpdateCloudAndFlow`, `UpdateFlow`, and `routeCloudEndpointFlow`; the + perpendicular-dominance threshold logic is shared now. + +## Deferred (tracked as issues, not in this PR) + +- Straight stock-to-stock flows cannot be offset at all: the perpendicular + valve-drag reroute requires a cloud endpoint, and segment drags require + interior segments. Offsetting needs a Z-shape (two corners), a new + capability. +- The failed-patch path in `handleFlowAttach` silently discards the drawn + flow (toast only); consider preserving the gesture or a clearer error + surface. +- `bpowers/fooz` still contains the pre-fix corrupt pair (model `drain_` / + view "New Flow 1") plus scratch elements; after this PR the corrupt flow can + be deleted in the UI (keyboard delete + non-crashing details). + +## Testing approach + +Controller-level reproduction tests (fake engine) pin finding 1; jsdom +component tests pin findings 2-4; pure-function tests pin findings 5-6 and all +P3 refactors (characterization tests written against current behavior before +each extraction). diff --git a/src/diagram/LAYOUT.md b/src/diagram/LAYOUT.md index c4dce7daf..785b52348 100644 --- a/src/diagram/LAYOUT.md +++ b/src/diagram/LAYOUT.md @@ -47,7 +47,7 @@ The main container for a system dynamics diagram with these properties: ```typescript { nextUid: number, // Next available UID for new elements - elements: List, // All visual elements in the view + elements: readonly ViewElement[], // All visual elements in the view viewBox: Rect, // Bounding box of the view zoom: number // Zoom factor (negative means auto-fit) } @@ -91,8 +91,8 @@ StockViewElement { y: number, // Center Y coordinate labelSide: LabelSide, // 'top'|'bottom'|'left'|'right'|'center' isZeroRadius: boolean, // Always false for stocks - inflows: List, // UIDs of inflowing flows (computed) - outflows: List // UIDs of outflowing flows (computed) + inflows: readonly UID[], // UIDs of inflowing flows (computed) + outflows: readonly UID[] // UIDs of outflowing flows (computed) } ``` @@ -128,7 +128,7 @@ FlowViewElement { x: number, // Valve center X y: number, // Valve center Y labelSide: LabelSide, - points: List, // Path points defining the flow + points: readonly Point[], // Path points defining the flow isZeroRadius: boolean // Always false for flows } @@ -143,13 +143,23 @@ Point { 1. **Must have at least 2 points** (source and destination) 2. **First point** connects to source (stock or cloud) 3. **Last point** connects to sink (stock or cloud) -4. **Intermediate points** define the path shape (currently only 2-point flows supported) +4. **Intermediate points** define the path shape. Multi-segment (L- and + Z-shaped) flows are fully supported: stocks moving off-axis produce a + corner point, endpoint drags with a dominant perpendicular component + convert a straight flow to an L, and interior segments of 3+ point flows + can be dragged perpendicular to their direction. `normalizeFlowPoints` + removes zero-length and colinear interior segments after every mutation so + segments strictly alternate horizontal/vertical. 5. **Flows are constrained** to horizontal or vertical segments when connected to stocks 6. **Stock connections**: - Endpoints snap to stock edges (±width/2 or ±height/2) - Must be at least 3px from corners + - Multiple flows attaching to the same stock side spread evenly along the + edge (n flows sit at i/(n+1) fractions; straight flows keep their + anchor-determined position but still reserve a slot) 7. **Cloud connections**: - - Attach at CloudRadius (13.5px) distance from cloud center + - The endpoint sits at the cloud center; the rendered path is retracted by + CloudRadius (13.5px) so the arrowhead lands on the cloud's edge - Clouds adjust position when dragged **Path Rendering**: @@ -272,7 +282,7 @@ LinkViewElement { toUid: number, // Destination element UID arc?: number, // Takeoff angle in radians (undefined = straight) isStraight: boolean, // Force straight line - multiPoint?: List // Multi-segment connectors (future/not implemented) + multiPoint?: readonly Point[] // Multi-segment connectors (future/not implemented) } ``` From 96653c5dbd97551c6d7d9384185242e88e2cfb3d Mon Sep 17 00:00:00 2001 From: Bobby Powers Date: Tue, 30 Jun 2026 22:49:32 -0700 Subject: [PATCH 5/9] diagram: adopt patch-carried views over the stale live view preserveLiveView unconditionally keeps the active model's live view on every updateProject rebuild, protecting newer optimistic pans/moves from older engine snapshots. But handleRename ships its view change as an upsertView op inside the rename patch rather than through updateView, so the live view it was 'protected' by was the stale pre-rename one: the rename applied correctly in the engine and the save, while the UI kept showing the old name -- and the next geometry edit round-tripped that stale view back into the engine, persisting a model/view divergence (a view element whose variable no longer exists; observed live as an editor-crashing corrupted project). applyPatchOrReportError now mirrors any primary-view upsertView op for the active model into the live project via the same optimistic step updateView uses, so an explicit view op -- which is newer user intent than the live view by construction -- wins the merge. updateView and queueViewUpdate call engine.applyPatch directly, so their own view patches are not double-applied. --- src/diagram/project-controller.ts | 37 +++++++++++++ src/diagram/tests/project-controller.test.ts | 56 ++++++++++++++++++++ 2 files changed, 93 insertions(+) diff --git a/src/diagram/project-controller.ts b/src/diagram/project-controller.ts index e127ac709..d84a1da63 100644 --- a/src/diagram/project-controller.ts +++ b/src/diagram/project-controller.ts @@ -35,6 +35,7 @@ import { projectFromJson, projectAttachData, findNonFiniteViewCoord, + stockFlowViewFromJson, } from '@simlin/core/datamodel'; import { defined, mapSet, setsEqual, toInt, uint8ArraysEqual, type Series } from '@simlin/core/common'; import { first, getOrThrow } from '@simlin/core/collections'; @@ -591,9 +592,45 @@ export class ProjectController { this.reportError(err.message ?? `Unknown error during ${label}`); return false; } + this.adoptPatchedViews(patch); return true; } + /** + * Mirror any primary-view upsert carried by a just-applied patch into the + * live project, exactly as updateView's optimistic step does. + * + * Why: preserveLiveView (see updateProject) always keeps the ACTIVE model's + * live view on refresh, protecting newer optimistic pans/moves from older + * engine snapshots. But a view arriving in an explicit upsertView op (e.g. + * a rename patching the variable and its view together) is newer user intent + * than the live view by construction -- without this mirror, the stale live + * view clobbers the patched one on refresh, the edit looks like a silent + * no-op, and the next geometry edit round-trips the stale view back into the + * engine, persisting a model/view divergence. + * + * Elements are re-linked against the CURRENT (pre-patch) variables; refs to + * variables the patch introduced resolve as undefined until the follow-up + * refreshFromEngine re-links them (same transient the optimistic paths + * already tolerate). + */ + private adoptPatchedViews(patch: JsonProjectPatch): void { + const variables = this.project?.models.get(this.modelName)?.variables; + if (!variables) { + return; + } + for (const model of patch.models ?? []) { + if (model.name !== this.modelName) { + continue; + } + for (const op of model.ops ?? []) { + if (op.type === 'upsertView' && op.payload.index === 0) { + this.applyOptimisticView(stockFlowViewFromJson(op.payload.view, variables)); + } + } + } + } + /** Round-trip the engine's serialized state back into `project` and schedule * a re-simulation. Called after a successful patch. */ async refreshFromEngine(): Promise { diff --git a/src/diagram/tests/project-controller.test.ts b/src/diagram/tests/project-controller.test.ts index b508a9a07..cb74f0eef 100644 --- a/src/diagram/tests/project-controller.test.ts +++ b/src/diagram/tests/project-controller.test.ts @@ -17,6 +17,7 @@ import type { JsonProject, ErrorDetail } from '@simlin/engine'; import { SimlinErrorKind } from '@simlin/engine'; import { ProjectController, MaxUndoSize, type ProjectSnapshot } from '../project-controller'; +import { stockFlowViewToJson } from '../view-conversion'; import { makeFakeEngine, makeControllerConfig, @@ -204,6 +205,61 @@ describe('ProjectController optimistic view updates', () => { await controller.dispose(); }); + it('adopts a view carried by a content patch instead of preserving the stale live view', async () => { + // Mirrors Editor.handleRename: one patch both renames the variable AND + // upserts the renamed view. preserveLiveView exists to protect newer + // OPTIMISTIC views from older engine snapshots, but a view arriving in an + // explicit upsertView op is newer user intent than the live view by + // construction. Before the fix, the live (pre-rename) view clobbered the + // patched one on refresh: the rename looked like a silent no-op, and the + // next geometry edit round-tripped the stale view back into the engine, + // persisting a model/view divergence (a view element whose variable no + // longer exists -- observed live as an editor-crashing corrupted project). + const auxEl = { type: 'aux', name: 'x', uid: 1, x: 10, y: 20, labelSide: 'right' }; + let currentJson = validProjectJson({ mainViewElements: [auxEl] }); + const engine = makeFakeEngine({ json: () => currentJson }); + const { config } = makeControllerConfig({ engine }); + const controller = new ProjectController(config); + await controller.openInitialProject(); + + const view = controller.getView() as StockFlowView; + const renamedView: StockFlowView = { + ...view, + elements: view.elements.map((el) => (el.uid === 1 && el.type === 'aux' ? { ...el, name: 'y' } : el)), + }; + + const patch = { + models: [ + { + name: 'main', + ops: [ + { type: 'renameVariable' as const, payload: { from: 'x', to: 'y' } }, + { type: 'upsertView' as const, payload: { index: 0, view: stockFlowViewToJson(renamedView) } }, + ], + }, + ], + }; + // After the patch, the engine's serialized state carries the rename. + currentJson = validProjectJson({ mainViewElements: [{ ...auxEl, name: 'y' }] }); + + expect(await controller.applyPatchOrReportError(patch, 'rename')).toBe(true); + + // The optimistic mirror lands synchronously, before any refresh: the UI + // must show the rename immediately. + const optimistic = controller.getView() as StockFlowView; + const optimisticNames = optimistic.elements.map((el) => ('name' in el ? el.name : undefined)); + expect(optimisticNames).toContain('y'); + expect(optimisticNames).not.toContain('x'); + + await controller.refreshFromEngine(); + + const after = controller.getView() as StockFlowView; + const names = after.elements.map((el) => ('name' in el ? el.name : undefined)); + expect(names).toContain('y'); + expect(names).not.toContain('x'); + await controller.dispose(); + }); + it('updateView records undo history only when recordHistory is set', async () => { // Discrete element/structure edits (create/delete/move/flow-attach/etc.) // funnel their final engine state through updateView and must each produce From 431772541b0010981d5e4344ba6d50d63c4e5994 Mon Sep 17 00:00:00 2001 From: Bobby Powers Date: Tue, 30 Jun 2026 22:50:17 -0700 Subject: [PATCH 6/9] diagram: fix name-edit commit semantics, add keyboard delete/escape Plain Enter in the inline name editor previously inserted a newline (only ctrl/shift/alt+Enter committed), so the universal 'type name, press Enter' interaction staged a trailing newline that a later click-away committed; handleRename's single-occurrence newline escape then canonicalized such names into garbage idents (drain_). Enter and NumpadEnter now commit, shift+Enter inserts the line break (labels stay multi-line capable), and Escape still cancels. Committed names are sanitized (per-line trim, blank lines dropped); an empty result is treated as cancel, which for a just-created flow keeps the existing delete-on-cancel behavior. The create path now encodes line breaks to the stored backslash-n form exactly like the rename path, and the rename path encodes every newline instead of only the first. The Editor's global keydown gains Delete/Backspace (delete the current selection; guarded by isEditableElement, readOnlyMode, and stdlib models) and Escape (disarm the active creation tool, else clear the selection). This is the only delete affordance for unnamed elements (clouds) -- the details panel is the sole other one and unnamed elements never open it. The details panel itself now degrades gracefully when the selected view element's variable is missing from the model (a divergent/corrupted project): previously a render-time getOrThrow took down the entire editor via the ErrorBoundary, which also made the corrupt element unremovable since the panel hosts the delete button. Together with keyboard delete this makes such projects repairable in the UI. --- src/diagram/Editor.tsx | 59 ++- src/diagram/drawing/Canvas.tsx | 16 +- src/diagram/drawing/EditableLabel.tsx | 18 +- src/diagram/drawing/common.ts | 21 ++ .../tests/canvas-gestures-elements.test.tsx | 6 +- ...nvas-gestures-name-edit-lifecycle.test.tsx | 27 +- src/diagram/tests/common.test.ts | 42 ++- src/diagram/tests/editable-label.test.tsx | 107 ++++++ src/diagram/tests/editor-keyboard.test.ts | 339 ++++++++++++++++++ 9 files changed, 614 insertions(+), 21 deletions(-) create mode 100644 src/diagram/tests/editable-label.test.tsx create mode 100644 src/diagram/tests/editor-keyboard.test.ts diff --git a/src/diagram/Editor.tsx b/src/diagram/Editor.tsx index eb5ff16b0..a8a10e4ed 100644 --- a/src/diagram/Editor.tsx +++ b/src/diagram/Editor.tsx @@ -40,7 +40,7 @@ import { type ModuleReference, } from '@simlin/core/datamodel'; import { defined, exists, setsEqual } from '@simlin/core/common'; -import { getOrThrow, only } from '@simlin/core/collections'; +import { only } from '@simlin/core/collections'; import { AuxIcon } from './AuxIcon'; import { Toast } from './ErrorToast'; @@ -57,7 +57,7 @@ import { ModuleDetails } from './ModuleDetails'; import { ErrorDetails } from './ErrorDetails'; import { ZoomBar } from './ZoomBar'; import { Canvas, inCreationUid } from './drawing/Canvas'; -import { Point, searchableName } from './drawing/common'; +import { encodeNameNewlines, Point, searchableName } from './drawing/common'; import { computeFlowAttachment } from './flow-attach'; import { applyGroupMovement } from './group-movement'; import { detectUndoRedo, isEditableElement } from './keyboard-shortcuts'; @@ -557,21 +557,45 @@ export const Editor = React.memo(function Editor(props: EditorProps): React.Reac // `latest`/`r`, never a stale render closure, so empty deps is correct. const handleKeyDown = React.useCallback((e: KeyboardEvent): void => { - const { props: p } = latest.current; - // Don't handle shortcuts in embedded mode or editable fields + const { props: p, state } = latest.current; + // Don't handle shortcuts in embedded mode or editable fields (text + // inputs, the equation editor, and the canvas's inline name editor are + // all contenteditable/inputs, so typing there never triggers these). if (p.embedded || isEditableElement(e.target)) { return; } const action = detectUndoRedo(e); - if (!action) { + if (action) { + const isEnabled = action === 'undo' ? isUndoEnabled() : isRedoEnabled(); + if (isEnabled) { + e.preventDefault(); + handleUndoRedo(action); + } return; } - const isEnabled = action === 'undo' ? isUndoEnabled() : isRedoEnabled(); - if (isEnabled) { - e.preventDefault(); - handleUndoRedo(action); + // Escape: disarm the active creation tool first; with no tool armed, + // clear the selection (which also closes the details panel). + if (e.key === 'Escape') { + if (state.selectedTool !== undefined) { + setState({ selectedTool: undefined }); + } else if (state.selection.size > 0) { + handleSelection(new Set()); + } + return; + } + + // Delete/Backspace: delete the current selection. This is the only + // delete affordance for unnamed elements (clouds) and for elements whose + // details panel cannot open, so it must not depend on the panel. + if (e.key === 'Delete' || e.key === 'Backspace') { + const readOnly = p.readOnlyMode || isStdlibModel(modelName()); + if (!readOnly && state.selection.size > 0) { + e.preventDefault(); + void handleSelectionDelete(); + } + return; } }, []); @@ -691,7 +715,10 @@ export const Editor = React.memo(function Editor(props: EditorProps): React.Reac const view = defined(getView()); const oldIdent = canonicalize(oldName); - newName = newName.replace('\n', '\\n'); + // Encode ALL line breaks to the stored backslash-n form -- the previous + // single-occurrence replace left raw newlines in multi-line names, which + // canonicalized into malformed idents. + newName = encodeNameNewlines(newName); const elements = view.elements.map((element: ViewElement) => { if (!isNamedViewElement(element)) { @@ -2023,7 +2050,17 @@ export const Editor = React.memo(function Editor(props: EditorProps): React.Reac const model = defined(getModel()); const ident = defined(namedElement.ident); - const variable = getOrThrow(model.variables, ident); + // A view element whose variable is missing means the model and view have + // diverged (corrupted data, or a transient during multi-step updates). + // Throwing here took the WHOLE editor down via the ErrorBoundary during + // render -- and since this panel is also the delete affordance, the + // corrupt element became unremovable. Degrade to no panel instead; the + // element stays selectable and keyboard-deletable. + const variable = model.variables.get(ident); + if (variable === undefined) { + console.warn(`variable details unavailable: no variable '${ident}' in model '${modelName()}'`); + return; + } if (variable.type === 'module') { return ( diff --git a/src/diagram/drawing/Canvas.tsx b/src/diagram/drawing/Canvas.tsx index 34aed3dbb..d9ccfdaec 100644 --- a/src/diagram/drawing/Canvas.tsx +++ b/src/diagram/drawing/Canvas.tsx @@ -42,8 +42,10 @@ import { calcViewBox, displayName, labelRadii, + encodeNameNewlines, plainDeserialize, plainSerialize, + sanitizeLabelInput, Point, Rect, screenToCanvasPoint, @@ -2047,13 +2049,23 @@ export const Canvas = React.memo(function Canvas(props: CanvasProps): React.Reac return; } + // A commit whose sanitized name is empty (all whitespace/blank lines) is a + // cancel, not a rename to "": for a just-created flow the recursive cancel + // path also deletes the flow, matching Escape. + const newName = sanitizeLabelInput(plainSerialize(defined(latest.current.editingName))); + if (newName === '') { + handleEditingNameDone(true); + return; + } + const uid = only(latest.current.props.selection); const element = getElementByUid(uid); const oldName = displayName(defined((element as NamedViewElement).name)); - const newName = plainSerialize(defined(latest.current.editingName)); if (uid === inCreationUid) { - latest.current.props.onCreateVariable({ ...element, name: newName } as ViewElement); + // Names persist line breaks as literal backslash-n (see displayName); + // the rename path encodes in Editor.handleRename, the create path here. + latest.current.props.onCreateVariable({ ...element, name: encodeNameNewlines(newName) } as ViewElement); } else { latest.current.props.onRenameVariable(oldName, newName); } diff --git a/src/diagram/drawing/EditableLabel.tsx b/src/diagram/drawing/EditableLabel.tsx index 815873e02..d0d71eaef 100644 --- a/src/diagram/drawing/EditableLabel.tsx +++ b/src/diagram/drawing/EditableLabel.tsx @@ -49,8 +49,22 @@ export const EditableLabel = React.memo(function EditableLabel(props: EditingLab e.stopPropagation(); }; + const isEnter = (code: string): boolean => code === 'Enter' || code === 'NumpadEnter'; + + // Standard editor convention: plain Enter commits, shift+Enter inserts a + // line break (labels are multi-line capable), Escape cancels. The commit + // fires on keyUp; keyDown must block Slate's default insertBreak for the + // commit case, otherwise the committed name picks up a trailing newline -- + // which canonicalized into garbage idents like `drain_` and made renames + // appear to silently fail. + const handleKeyDown = (e: React.KeyboardEvent): void => { + if (isEnter(e.code) && !e.shiftKey) { + e.preventDefault(); + } + }; + const handleKeyPress = (e: React.KeyboardEvent): void => { - if (e.code === 'Enter' && (e.ctrlKey || e.shiftKey || e.altKey)) { + if (isEnter(e.code) && !e.shiftKey) { e.stopPropagation(); onDone(false); } else if (e.code === 'Escape') { @@ -129,7 +143,7 @@ export const EditableLabel = React.memo(function EditableLabel(props: EditingLab onPointerUp={handlePointerUpDown} > - + ); diff --git a/src/diagram/drawing/common.ts b/src/diagram/drawing/common.ts index 6224cdf2b..412c16a0d 100644 --- a/src/diagram/drawing/common.ts +++ b/src/diagram/drawing/common.ts @@ -77,6 +77,27 @@ export const displayName = (name: string): string => { return name.replace(/\\n/g, '\n').replace(/_/g, ' '); }; +// Normalize a user-entered label before committing it as a variable name: +// trim each line, drop blank lines (leading, trailing, or interior), and keep +// intentional interior line breaks. An all-whitespace input collapses to '' +// -- callers treat an empty result as a cancelled edit rather than committing +// a nameless variable. +export const sanitizeLabelInput = (raw: string): string => { + return raw + .split('\n') + .map((line) => line.trim()) + .filter((line) => line.length > 0) + .join('\n'); +}; + +// Encode a display-form name for storage: variable names persist line breaks +// as a literal backslash-n (displayName above decodes them). Every newline is +// encoded -- a single-occurrence replace here once produced idents containing +// raw newlines for multi-line names. +export const encodeNameNewlines = (name: string): string => { + return name.replace(/\n/g, '\\n'); +}; + // Convert a display name to a single-line searchable format. // Handles both actual newlines (from XMILE parsing) and escaped newlines (from edits). export const searchableName = (name: string): string => { diff --git a/src/diagram/tests/canvas-gestures-elements.test.tsx b/src/diagram/tests/canvas-gestures-elements.test.tsx index eb06e8798..954b6f566 100644 --- a/src/diagram/tests/canvas-gestures-elements.test.tsx +++ b/src/diagram/tests/canvas-gestures-elements.test.tsx @@ -297,7 +297,8 @@ describe('Canvas gestures: creation tools (checklist 12)', () => { expect(editable).not.toBeNull(); act(() => { - fireEvent.keyUp(editable!, { code: 'Enter', shiftKey: true }); + fireEvent.keyDown(editable!, { code: 'Enter' }); + fireEvent.keyUp(editable!, { code: 'Enter' }); }); expect(h.callbacks.onCreateVariable).toHaveBeenCalledTimes(1); const created = h.callbacks.onCreateVariable.mock.calls[0][0]; @@ -551,7 +552,8 @@ describe('Canvas gestures: name editing (checklist 15)', () => { const editable = enterEditing(h); act(() => { // EditableLabel commits on Enter held with a modifier. - fireEvent.keyUp(editable, { code: 'Enter', shiftKey: true }); + fireEvent.keyDown(editable, { code: 'Enter' }); + fireEvent.keyUp(editable, { code: 'Enter' }); }); expect(h.callbacks.onRenameVariable).toHaveBeenCalledTimes(1); diff --git a/src/diagram/tests/canvas-gestures-name-edit-lifecycle.test.tsx b/src/diagram/tests/canvas-gestures-name-edit-lifecycle.test.tsx index 2fbe42c64..7a854d568 100644 --- a/src/diagram/tests/canvas-gestures-name-edit-lifecycle.test.tsx +++ b/src/diagram/tests/canvas-gestures-name-edit-lifecycle.test.tsx @@ -106,8 +106,10 @@ describe('Canvas name-edit lifecycle: just-created flow', () => { const editable = createFlowAndEnterNameEdit(h); act(() => { - // EditableLabel commits on Enter held with a modifier. - fireEvent.keyUp(editable, { code: 'Enter', shiftKey: true }); + // EditableLabel commits on plain Enter (keyDown is prevented so Slate + // doesn't insert a line break; keyUp performs the commit). + fireEvent.keyDown(editable, { code: 'Enter' }); + fireEvent.keyUp(editable, { code: 'Enter' }); }); // Committing renames the flow (uid 50 != inCreationUid, so it is an existing @@ -128,7 +130,8 @@ describe('Canvas name-edit lifecycle: just-created flow', () => { // 1. Create a flow and COMMIT its name. This clears the creatingFlow latch. const flowEditable = createFlowAndEnterNameEdit(h); act(() => { - fireEvent.keyUp(flowEditable, { code: 'Enter', shiftKey: true }); + fireEvent.keyDown(flowEditable, { code: 'Enter' }); + fireEvent.keyUp(flowEditable, { code: 'Enter' }); }); expect(h.callbacks.onRenameVariable).toHaveBeenCalledTimes(1); expect(h.callbacks.onDeleteSelection).not.toHaveBeenCalled(); @@ -155,4 +158,22 @@ describe('Canvas name-edit lifecycle: just-created flow', () => { expect(h.callbacks.onDeleteSelection).not.toHaveBeenCalled(); expect(h.callbacks.onRenameVariable).toHaveBeenCalledTimes(1); // only the flow commit }); + + it('shift+Enter does NOT commit (it inserts a line break instead)', () => { + const h = renderCanvas({ elements: [], selectedTool: 'flow' }); + installFlowMaterializer(h); + h.clearMountCalls(); + + const editable = createFlowAndEnterNameEdit(h); + + act(() => { + fireEvent.keyDown(editable, { code: 'Enter', shiftKey: true }); + fireEvent.keyUp(editable, { code: 'Enter', shiftKey: true }); + }); + + // No commit, no delete: the editor stays open for the second line. + expect(h.callbacks.onRenameVariable).not.toHaveBeenCalled(); + expect(h.callbacks.onDeleteSelection).not.toHaveBeenCalled(); + expect(h.query('[contenteditable]')).not.toBeNull(); + }); }); diff --git a/src/diagram/tests/common.test.ts b/src/diagram/tests/common.test.ts index 27042bcee..1cb5247a7 100644 --- a/src/diagram/tests/common.test.ts +++ b/src/diagram/tests/common.test.ts @@ -2,7 +2,14 @@ // Use of this source code is governed by the Apache License, // Version 2.0, that can be found in the LICENSE file. -import { calcViewBox, mergeBounds, Rect, searchableName } from '../drawing/common'; +import { + calcViewBox, + encodeNameNewlines, + mergeBounds, + Rect, + sanitizeLabelInput, + searchableName, +} from '../drawing/common'; describe('common', () => { describe('mergeBounds', () => { @@ -117,4 +124,37 @@ describe('common', () => { expect(searchableName('')).toBe(''); }); }); + + describe('sanitizeLabelInput', () => { + it('trims surrounding whitespace on a single-line name', () => { + expect(sanitizeLabelInput(' births ')).toBe('births'); + }); + + it('drops a trailing newline left by an accidental line break', () => { + expect(sanitizeLabelInput('drain\n')).toBe('drain'); + }); + + it('drops leading and interior blank lines but keeps intentional breaks', () => { + expect(sanitizeLabelInput('\nfraction of\n\n carrying capacity \n')).toBe('fraction of\ncarrying capacity'); + }); + + it('collapses all-whitespace input to the empty string (treated as cancel)', () => { + expect(sanitizeLabelInput(' \n \n\t')).toBe(''); + expect(sanitizeLabelInput('')).toBe(''); + }); + }); + + describe('encodeNameNewlines', () => { + it('encodes a single newline to a literal backslash-n', () => { + expect(encodeNameNewlines('a\nb')).toBe('a\\nb'); + }); + + it('encodes EVERY newline (the old single-occurrence replace missed the rest)', () => { + expect(encodeNameNewlines('a\nb\nc')).toBe('a\\nb\\nc'); + }); + + it('leaves names without newlines unchanged', () => { + expect(encodeNameNewlines('plain name')).toBe('plain name'); + }); + }); }); diff --git a/src/diagram/tests/editable-label.test.tsx b/src/diagram/tests/editable-label.test.tsx new file mode 100644 index 000000000..8f059b734 --- /dev/null +++ b/src/diagram/tests/editable-label.test.tsx @@ -0,0 +1,107 @@ +/** + * @jest-environment jsdom + * + * Copyright 2026 The Simlin Authors. All rights reserved. + * Use of this source code is governed by the Apache License, + * Version 2.0, that can be found in the LICENSE file. + */ + +// EditableLabel key handling: plain Enter commits (and its keyDown is +// prevented so Slate never inserts a line break for it), shift+Enter is the +// line-break gesture and must NOT commit, ctrl+Enter still commits (legacy +// chord), Escape cancels. This pins the standard-editor convention adopted +// after the audit found the inverted mapping (commit only on modifier+Enter) +// left every "type name, press Enter" edit staging a trailing newline. + +import * as React from 'react'; +import { render, fireEvent } from '@testing-library/react'; + +// jsdom does not implement isContentEditable, but slate-react's keyDown +// pipeline gates on ReactEditor.hasEditableTarget -> element.isContentEditable +// before forwarding to our onKeyDown. Without this polyfill the keyDown +// handler (which prevents Slate's default line-break on plain Enter) is +// unreachable in tests. (keyUp is passed through unwrapped, so it needs no +// polyfill.) +beforeAll(() => { + Object.defineProperty(HTMLElement.prototype, 'isContentEditable', { + configurable: true, + get(this: HTMLElement): boolean { + return this.getAttribute('contenteditable') === 'true'; + }, + }); +}); + +import { EditableLabel } from '../drawing/EditableLabel'; +import { plainDeserialize } from '../drawing/common'; + +function renderLabel(): { editable: Element; onDone: jest.Mock; onChange: jest.Mock } { + const onDone = jest.fn(); + const onChange = jest.fn(); + const value = plainDeserialize('label', 'some name'); + const { container } = render( + , + ); + const editable = container.querySelector('[contenteditable]'); + expect(editable).not.toBeNull(); + // Slate's composed onKeyDown only forwards to our handler when the editor + // believes it is focused; jsdom needs an explicit focus event for that. + fireEvent.focus(editable as Element); + return { editable: editable as Element, onDone, onChange }; +} + +describe('EditableLabel key handling', () => { + it('commits on plain Enter and prevents the default line-break insertion', () => { + const { editable, onDone } = renderLabel(); + + // fireEvent returns false when preventDefault was called. + const keyDownNotPrevented = fireEvent.keyDown(editable, { code: 'Enter' }); + expect(keyDownNotPrevented).toBe(false); + + fireEvent.keyUp(editable, { code: 'Enter' }); + expect(onDone).toHaveBeenCalledTimes(1); + expect(onDone).toHaveBeenCalledWith(false); + }); + + it('commits on NumpadEnter too', () => { + const { editable, onDone } = renderLabel(); + fireEvent.keyDown(editable, { code: 'NumpadEnter' }); + fireEvent.keyUp(editable, { code: 'NumpadEnter' }); + expect(onDone).toHaveBeenCalledWith(false); + }); + + it('does not commit on shift+Enter (the line-break gesture)', () => { + const { editable, onDone } = renderLabel(); + + // keyDown must NOT be prevented: Slate's default insertBreak adds the line. + const keyDownNotPrevented = fireEvent.keyDown(editable, { code: 'Enter', shiftKey: true }); + expect(keyDownNotPrevented).toBe(true); + + fireEvent.keyUp(editable, { code: 'Enter', shiftKey: true }); + expect(onDone).not.toHaveBeenCalled(); + }); + + it('still commits on ctrl+Enter (legacy chord)', () => { + const { editable, onDone } = renderLabel(); + fireEvent.keyDown(editable, { code: 'Enter', ctrlKey: true }); + fireEvent.keyUp(editable, { code: 'Enter', ctrlKey: true }); + expect(onDone).toHaveBeenCalledWith(false); + }); + + it('cancels on Escape', () => { + const { editable, onDone } = renderLabel(); + fireEvent.keyUp(editable, { code: 'Escape' }); + expect(onDone).toHaveBeenCalledTimes(1); + expect(onDone).toHaveBeenCalledWith(true); + }); +}); diff --git a/src/diagram/tests/editor-keyboard.test.ts b/src/diagram/tests/editor-keyboard.test.ts new file mode 100644 index 000000000..4721b902a --- /dev/null +++ b/src/diagram/tests/editor-keyboard.test.ts @@ -0,0 +1,339 @@ +/** + * @jest-environment jsdom + * + * Copyright 2026 The Simlin Authors. All rights reserved. + * Use of this source code is governed by the Apache License, + * Version 2.0, that can be found in the LICENSE file. + */ + +// Editor-level keyboard shortcuts beyond undo/redo, and the graceful +// details-panel degradation for a model/view divergence: +// +// 1. Delete/Backspace deletes the current selection -- the ONLY delete +// affordance for unnamed elements (clouds) and for elements whose details +// panel cannot open. +// 2. Escape disarms the active creation tool first, then clears selection. +// 3. Keys typed into editable fields never trigger these. +// 4. A selected view element whose variable is MISSING from the model (a +// corrupted/divergent project) must not crash the editor when the details +// panel opens; it degrades to no panel so the element can still be +// selected and keyboard-deleted (the repair path). + +import { TextEncoder, TextDecoder } from 'util'; +Object.assign(globalThis, { TextEncoder, TextDecoder }); + +import * as React from 'react'; +import { act, fireEvent, render, screen } from '@testing-library/react'; + +import type { StockFlowView, Variable } from '@simlin/core/datamodel'; + +import { ProjectController, type ProjectSnapshot } from '../project-controller'; +import type { CanvasProps } from '../drawing/Canvas'; + +// Mock SpeedDial as in editor-tool-deselect.test.ts so tools are queryable. +jest.mock('../components/SpeedDial', () => { + const react = jest.requireActual('react') as typeof import('react'); + return { + __esModule: true, + default: (p: { children?: React.ReactNode; onClick?: (e: unknown) => void }) => + react.createElement( + 'div', + null, + react.createElement('button', { + type: 'button', + 'aria-label': 'dial-fab', + onClick: (e: unknown) => p.onClick?.(e), + }), + p.children, + ), + SpeedDialAction: (p: { title: string; selected?: boolean; onClick?: (e: unknown) => void }) => + react.createElement('button', { + type: 'button', + 'aria-label': p.title, + 'data-selected': p.selected ? 'true' : 'false', + onClick: (e: unknown) => p.onClick?.(e), + }), + SpeedDialIcon: () => null, + }; +}); + +// Capture the live Canvas props so tests can drive selection callbacks and +// observe the selection the Editor passes back down. +let canvasProps: CanvasProps | undefined; +jest.mock('../drawing/Canvas', () => ({ + __esModule: true, + Canvas: (p: CanvasProps) => { + canvasProps = p; + return null; + }, + inCreationUid: -2, +})); + +// The details panel itself is not under test; render a marker so tests can +// assert presence/absence without pulling in the full VariableDetails tree. +jest.mock('../VariableDetails', () => ({ + __esModule: true, + VariableDetails: () => { + const react = jest.requireActual('react') as typeof import('react'); + return react.createElement('div', { 'data-testid': 'variable-details' }); + }, +})); + +import { Editor, type EditorProps } from '../Editor'; + +function makeView(): StockFlowView { + return { + nextUid: 20, + elements: [ + { + type: 'aux', + uid: 9, + name: 'some var', + ident: 'some_var', + var: undefined, + x: 100, + y: 100, + labelSide: 'right', + isZeroRadius: false, + }, + // A "ghost": present in the view, but its variable is missing from the + // model's variables map (the corrupted-project shape). + { + type: 'aux', + uid: 10, + name: 'ghost', + ident: 'ghost', + var: undefined, + x: 200, + y: 100, + labelSide: 'right', + isZeroRadius: false, + }, + ], + viewBox: { x: 0, y: 0, width: 800, height: 600 }, + zoom: 1, + useLetteredPolarity: false, + }; +} + +function makeSnapshot(): ProjectSnapshot { + const someVar: Variable = { + type: 'aux', + ident: 'some_var', + equation: { type: 'scalar', equation: '1' }, + documentation: '', + units: '', + gf: undefined, + canBeModuleInput: false, + isPublic: false, + aiState: undefined, + data: undefined, + errors: [], + unitErrors: [], + uid: 9, + } as unknown as Variable; + return { + project: { + name: 'test-project', + models: new Map([ + [ + 'main', + { + name: 'main', + variables: new Map([['some_var', someVar]]), + views: [makeView()], + loopMetadata: [], + groups: [], + }, + ], + ]), + simSpecs: { start: 0, stop: 100, dt: { isReciprocal: false, value: 1 }, timeUnits: 'years' }, + }, + modelName: 'main', + projectVersion: 1, + projectGeneration: 0, + status: 'ok', + cachedErrors: { simError: undefined, modelErrors: [], varErrors: new Map(), unitErrors: new Map() }, + data: new Map(), + modelStack: [], + canUndo: false, + canRedo: false, + navResetSeq: 0, + } as unknown as ProjectSnapshot; +} + +function makeProps(overrides: Partial = {}): EditorProps { + return { + inputFormat: 'json', + initialProjectJson: '{}', + initialProjectVersion: 1, + name: 'test-project', + embedded: false, + readOnlyMode: false, + onSave: async () => 1, + ...overrides, + } as EditorProps; +} + +function renderEditor(props: EditorProps = makeProps()): void { + act(() => { + render(React.createElement(Editor, props)); + }); +} + +const toolSelected = (title: string): boolean => screen.getByLabelText(title).getAttribute('data-selected') === 'true'; + +describe('Editor keyboard shortcuts', () => { + let applyPatchCalls: unknown[]; + let updateViewCalls: StockFlowView[]; + + beforeEach(() => { + canvasProps = undefined; + applyPatchCalls = []; + updateViewCalls = []; + jest.spyOn(ProjectController.prototype, 'getSnapshot').mockReturnValue(makeSnapshot()); + jest.spyOn(ProjectController.prototype, 'openInitialProject').mockResolvedValue(undefined); + jest.spyOn(ProjectController.prototype, 'dispose').mockResolvedValue(undefined); + jest.spyOn(ProjectController.prototype, 'scheduleSimRun').mockImplementation(() => {}); + jest.spyOn(ProjectController.prototype, 'subscribe').mockReturnValue(() => {}); + jest.spyOn(ProjectController.prototype, 'getEngine').mockReturnValue({} as never); + jest.spyOn(ProjectController.prototype, 'applyPatchOrReportError').mockImplementation(async (patch) => { + applyPatchCalls.push(patch); + return true; + }); + jest.spyOn(ProjectController.prototype, 'updateView').mockImplementation(async (view) => { + updateViewCalls.push(view); + }); + }); + + afterEach(() => { + jest.restoreAllMocks(); + }); + + function selectUid(uid: number): void { + act(() => { + canvasProps?.onSetSelection(new Set([uid])); + }); + } + + it('Delete removes the selected element (deleteVariable op + view update)', async () => { + renderEditor(); + selectUid(9); + + await act(async () => { + fireEvent.keyDown(document, { key: 'Delete' }); + }); + + expect(applyPatchCalls).toHaveLength(1); + expect(JSON.stringify(applyPatchCalls[0])).toContain('"deleteVariable"'); + expect(JSON.stringify(applyPatchCalls[0])).toContain('some_var'); + expect(updateViewCalls).toHaveLength(1); + expect(updateViewCalls[0].elements.some((el) => el.uid === 9)).toBe(false); + // The selection was cleared alongside. + expect(canvasProps?.selection.size).toBe(0); + }); + + it('Backspace deletes too', async () => { + renderEditor(); + selectUid(9); + + await act(async () => { + fireEvent.keyDown(document, { key: 'Backspace' }); + }); + + expect(updateViewCalls).toHaveLength(1); + }); + + it('Delete with no selection is a no-op', async () => { + renderEditor(); + + await act(async () => { + fireEvent.keyDown(document, { key: 'Delete' }); + }); + + expect(applyPatchCalls).toHaveLength(0); + expect(updateViewCalls).toHaveLength(0); + }); + + it('Delete in readOnlyMode is a no-op', async () => { + renderEditor(makeProps({ readOnlyMode: true })); + selectUid(9); + + await act(async () => { + fireEvent.keyDown(document, { key: 'Delete' }); + }); + + expect(applyPatchCalls).toHaveLength(0); + expect(updateViewCalls).toHaveLength(0); + }); + + it('Delete typed in an editable field does not delete the selection', async () => { + renderEditor(); + selectUid(9); + + const input = document.createElement('input'); + document.body.appendChild(input); + await act(async () => { + fireEvent.keyDown(input, { key: 'Delete' }); + }); + + expect(updateViewCalls).toHaveLength(0); + input.remove(); + }); + + it('Escape disarms the active tool before touching the selection', () => { + renderEditor(); + fireEvent.click(screen.getByLabelText('dial-fab')); + fireEvent.click(screen.getByLabelText('Flow')); + expect(toolSelected('Flow')).toBe(true); + selectUid(9); + + act(() => { + fireEvent.keyDown(document, { key: 'Escape' }); + }); + expect(toolSelected('Flow')).toBe(false); + // Selection untouched on the first Escape. + expect(canvasProps?.selection.has(9)).toBe(true); + + act(() => { + fireEvent.keyDown(document, { key: 'Escape' }); + }); + expect(canvasProps?.selection.size).toBe(0); + }); + + it('the ghost element (variable missing from the model) can be selected and keyboard-deleted', async () => { + // Before the hardening, selecting the ghost with the details panel open + // crashed the whole editor in render (getOrThrow on the missing variable), + // which ALSO made the element undeletable (the panel is the only other + // delete affordance). + renderEditor(); + selectUid(10); + + // Open the details panel the way Canvas does after a clean click. + act(() => { + canvasProps?.onShowVariableDetails(); + }); + + // No crash: the editor is still rendering (the canvas is alive) and no + // details panel appears for the ghost. + expect(canvasProps).toBeDefined(); + expect(screen.queryByTestId('variable-details')).toBeNull(); + + // The repair path: keyboard-delete the ghost. Its variable is missing, so + // no deleteVariable op is emitted -- but the view element must go. + await act(async () => { + fireEvent.keyDown(document, { key: 'Delete' }); + }); + expect(updateViewCalls).toHaveLength(1); + expect(updateViewCalls[0].elements.some((el) => el.uid === 10)).toBe(false); + }); + + it('the details panel still renders for a healthy variable', () => { + renderEditor(); + selectUid(9); + act(() => { + canvasProps?.onShowVariableDetails(); + }); + expect(screen.queryByTestId('variable-details')).not.toBeNull(); + }); +}); From f5990e19e84ae4e89d8268cbe4ed5b33f692c15a Mon Sep 17 00:00:00 2001 From: Bobby Powers Date: Tue, 30 Jun 2026 22:56:20 -0700 Subject: [PATCH 7/9] diagram: classify flow orientation by dominant axis when routing Routing decisions used exact coordinate equality (p1.y === p2.y) to classify a segment as horizontal or vertical. Pre-fix creation bugs persisted flows with a few pixels of cross-axis drift (a live specimen drifts 5.5px), and imported models can carry the same; under exact checks such a visually-horizontal flow classified as VERTICAL, so a stock drag routed a wrong-way L (down from the source cloud, into the stock's left edge, valve and label stacked on the cloud), a parallel endpoint drag read as perpendicular and rerouted into a bogus L, and adjustFlows updated only the wrong coordinate, visually detaching the flow from its dragged endpoint. computeFlowRoute, getFlowAttachmentInfo, UpdateCloudAndFlow, and adjustFlows now classify by the segment's dominant axis (ties count as horizontal). Exact equality remains the definition of orthogonal for rendering and normalizeFlowPoints; dominance only breaks the classification tie so near-axis data routes the way it looks. One group-movement expectation moved with the change: a genuinely diagonal (77.5px by 50px) flow now attaches its L via the bottom edge (perpendicular to its dominant horizontal) instead of the right edge the misclassification used to pick. --- src/diagram/drawing/Flow.tsx | 42 +++++++++++++++--- src/diagram/tests/flow-routing.test.ts | 55 ++++++++++++++++++++++++ src/diagram/tests/group-movement.test.ts | 11 +++-- 3 files changed, 98 insertions(+), 10 deletions(-) diff --git a/src/diagram/drawing/Flow.tsx b/src/diagram/drawing/Flow.tsx index edfb16063..a63a1f12d 100644 --- a/src/diagram/drawing/Flow.tsx +++ b/src/diagram/drawing/Flow.tsx @@ -29,6 +29,22 @@ import styles from './Flow.module.css'; type Side = 'left' | 'right' | 'top' | 'bottom'; +/** + * Classify a segment's working orientation for ROUTING decisions by its + * dominant axis. Exact coordinate equality stays the definition of + * "orthogonal" for rendering and normalization, but routing must also handle + * slightly-diagonal data -- pre-fix creation bugs persisted flows with a few + * pixels of cross-axis drift, and imported models can carry the same. Under + * exact-equality checks such a visually-horizontal flow classified as + * VERTICAL: stock drags routed a wrong-way L (valve and label stacked on the + * source cloud) and endpoint drags constrained/updated the wrong coordinate. + * Ties (a perfect 45-degree diagonal, only possible cloud-to-cloud) count as + * horizontal. + */ +function isDominantlyHorizontal(p1: IPoint, p2: IPoint): boolean { + return Math.abs(p2.y - p1.y) <= Math.abs(p2.x - p1.x); +} + /** * Returns a point on the edge of a stock. * @@ -140,7 +156,7 @@ function getFlowAttachmentInfo( const anchor = stockIsFirst ? lastPoint : firstPoint; const adjacentPoint = getStockAdjacentPoint(points, stockIsFirst); const anchorAdjacentPoint = getAnchorAdjacentPoint(points, stockIsFirst); - const originalFlowIsHorizontal = anchor.y === anchorAdjacentPoint.y; + const originalFlowIsHorizontal = isDominantlyHorizontal(anchorAdjacentPoint, anchor); let side: Side; let isStraight = false; @@ -148,7 +164,7 @@ function getFlowAttachmentInfo( // For 4+ point flows, use the existing segment orientation if (points.length >= 4) { const currentStockPoint = stockIsFirst ? firstPoint : lastPoint; - const isHorizontalSegment = currentStockPoint.y === adjacentPoint.y; + const isHorizontalSegment = isDominantlyHorizontal(currentStockPoint, adjacentPoint); if (isHorizontalSegment) { side = adjacentPoint.x > newStockCx ? 'right' : 'left'; @@ -299,7 +315,7 @@ export function computeFlowRoute( // This works for both 2-point (straight) and 3+ point (L-shaped) flows. // For L-shaped flows, the anchor-side segment preserves the original direction. const anchorAdjacentPoint = getAnchorAdjacentPoint(points, stockIsFirst); - const originalFlowIsHorizontal = anchor.y === anchorAdjacentPoint.y; + const originalFlowIsHorizontal = isDominantlyHorizontal(anchorAdjacentPoint, anchor); // For flows with 4+ points (imported or manually-edited multi-segment flows), // update both the attached endpoint and the adjacent corner to preserve @@ -313,7 +329,7 @@ export function computeFlowRoute( // We must preserve this orientation to avoid creating diagonal segments between // corner1 and corner2 when the stock moves far perpendicular to the segment. const currentStockPoint = stockIsFirst ? firstPoint : lastPoint; - const isHorizontalSegment = currentStockPoint.y === adjacentPoint.y; + const isHorizontalSegment = isDominantlyHorizontal(currentStockPoint, adjacentPoint); // Choose the attachment side based on the preserved orientation let side: Side; @@ -500,6 +516,13 @@ function adjustFlows( let horizontal = isHorizontal(flow); const vertical = isVertical(flow); const inCreation = horizontal && vertical; + if (!inCreation && !horizontal && !vertical && flow.points.length === 2) { + // Slightly-diagonal legacy data: neither exact test matched, so classify + // by the dominant axis (see isDominantlyHorizontal). Without this, a + // visually-horizontal drifted flow snapped the dragged endpoint's y but + // never updated its x, detaching the flow from its own endpoint. + horizontal = isDominantlyHorizontal(first(flow.points), last(flow.points)); + } let otherEnd: IPoint | undefined; const points = flow.points.map((point, i) => { @@ -719,9 +742,14 @@ export function UpdateCloudAndFlow( // Use the drag direction to determine the intended axis. const isDegenerate = seg.isHorizontal && seg.isVertical; - // For degenerate segments, determine axis from drag direction. - // For non-degenerate segments, use the existing segment orientation. - const treatAsHorizontal = isDegenerate ? Math.abs(moveDelta.x) > Math.abs(moveDelta.y) : seg.isHorizontal; + // For degenerate segments, determine axis from drag direction. For + // non-degenerate segments, use the segment's DOMINANT orientation (see + // isDominantlyHorizontal): an exact check misread a parallel drag of a + // slightly-diagonal legacy flow as perpendicular and rerouted it into a + // bogus L. + const treatAsHorizontal = isDegenerate + ? Math.abs(moveDelta.x) > Math.abs(moveDelta.y) + : isDominantlyHorizontal(seg.p1, seg.p2); const perpDelta = treatAsHorizontal ? moveDelta.y : moveDelta.x; const parDelta = treatAsHorizontal ? moveDelta.x : moveDelta.y; diff --git a/src/diagram/tests/flow-routing.test.ts b/src/diagram/tests/flow-routing.test.ts index cd908edb1..02f2c94b4 100644 --- a/src/diagram/tests/flow-routing.test.ts +++ b/src/diagram/tests/flow-routing.test.ts @@ -2740,4 +2740,59 @@ describe('Flow routing', () => { } }); }); + + describe('slightly-diagonal legacy flows classify by their dominant axis', () => { + // Real specimen: pre-fix flow creation persisted visually-horizontal flows + // whose endpoint y's drift a few pixels (5.5px in the wild case); imported + // models can carry the same. Exact-equality orientation checks classified + // such a flow as VERTICAL, so routing produced a wrong-way L (down from + // the cloud, into the stock's left edge, valve and label stacked on the + // cloud) and endpoint drags constrained/updated the wrong coordinate. + + it('computeFlowRoute treats a near-horizontal flow as horizontal (L via the top edge)', () => { + // Cloud anchor on the left, stock on the right, 5.5px of y-drift. + const stock = makeStock(stockUid, 565.6, 353); + const flow = makeFlow(flowUid, 426.5, 355.7, [ + { x: 310, y: 358.5, attachedToUid: cloudUid }, + { x: 543.1, y: 353, attachedToUid: stockUid }, + ]); + + // Drag the stock far downward: a horizontal flow must attach via a + // perpendicular (vertical) segment into the stock's TOP edge, with the + // corner carrying the anchor's y. + const newStockCy = 483; + const result = computeFlowRoute(flow, stock, 565.6, newStockCy); + + expect(result.points.length).toBe(3); + const corner = result.points[1]; + const stockPoint = result.points[2]; + expect(stockPoint.y).toBeCloseTo(newStockCy - StockHeight / 2); + expect(corner.x).toBeCloseTo(stockPoint.x); + expect(corner.y).toBeCloseTo(358.5); + }); + + it('UpdateCloudAndFlow slides a near-horizontal cloud drag along x (no bogus L, endpoint follows)', () => { + // Same drift, cloud end being dragged 50px leftward along the VISUAL + // axis. As "vertical", this parallel drag read as perpendicular: it + // rerouted into an L and never updated the endpoint's x (the cloud + // detached visually from its own flow). + const cloud = makeCloud(cloudUid, flowUid, 310, 358.5); + const flow = makeFlow(flowUid, 426.5, 355.7, [ + { x: 310, y: 358.5, attachedToUid: cloudUid }, + { x: 543.1, y: 353, attachedToUid: 99 }, + ]); + + const [movedCloud, newFlow] = UpdateCloudAndFlow(cloud, flow, { x: 50, y: 0 }); + + // Still straight, constrained to the flow's (horizontal) axis. + expect(newFlow.points.length).toBe(2); + expect(movedCloud.x).toBeCloseTo(260); + expect(movedCloud.y).toBeCloseTo(358.5); + // The dragged endpoint follows the cloud on x. + expect(newFlow.points[0].x).toBeCloseTo(260); + // The far endpoint is untouched. + expect(newFlow.points[1].x).toBeCloseTo(543.1); + expect(newFlow.points[1].y).toBeCloseTo(353); + }); + }); }); diff --git a/src/diagram/tests/group-movement.test.ts b/src/diagram/tests/group-movement.test.ts index fb5035b19..c6c9b1787 100644 --- a/src/diagram/tests/group-movement.test.ts +++ b/src/diagram/tests/group-movement.test.ts @@ -15,7 +15,7 @@ import { UID, } from '@simlin/core/datamodel'; -import { StockWidth } from '../drawing/Stock'; +import { StockHeight, StockWidth } from '../drawing/Stock'; import { applyGroupMovement } from '../group-movement'; // Helper functions to create test elements @@ -454,9 +454,14 @@ describe('Group Movement', () => { expect(newFlow1.points[0].x).toBe(150 + StockWidth / 2); expect(newFlow1.points[newFlow1.points.length - 1].x).toBe(250 - StockWidth / 2); - // Flow 2 should be routed (one endpoint moved, one fixed) + // Flow 2 should be routed (one endpoint moved, one fixed). It runs + // 77.5px right and 50px down, so it classifies as dominantly HORIZONTAL + // and its L attaches perpendicular to that: via the stock's bottom edge. + // (Before dominant-axis classification the exact-equality check misread + // this diagonal as vertical and attached via the right edge.) const newFlow2 = result.get(3) as FlowViewElement; - expect(newFlow2.points[0].x).toBe(150 + StockWidth / 2); // at new stock position + expect(newFlow2.points[0].x).toBe(150); // bottom-edge attach at new stock x + expect(newFlow2.points[0].y).toBe(100 + StockHeight / 2); expect(newFlow2.points[newFlow2.points.length - 1].x).toBe(200); // cloud unchanged // Both flows should have different y-coordinates on Stock A's edge From 870688b81dd16290ffd09408cccd4925538e54ae Mon Sep 17 00:00:00 2001 From: Bobby Powers Date: Tue, 30 Jun 2026 23:00:41 -0700 Subject: [PATCH 8/9] diagram: pin a new flow's source to the stock edge at commit A flow drawn out of a stock stages its source point at the press-time stock CENTER, and computeFlowAttachment routed only the dragged sink -- so the persisted source endpoint sat hidden under the stock body, violating the documented edge-attachment rule until the next stock drag re-pinned it (observed in live data: both endpoints of a stock's flows attached at its center). pinSourceToStockEdge moves only the dominant-axis coordinate of the source point onto the edge facing the sink (a straight flow stays straight) and re-clamps the valve to the shortened segment. It is applied once, at creation commit, after the sink is routed. The routing deliberately avoids UpdateCloudAndFlow's zero-delta straight path, whose cloud-branch valve math mirrors an off-center valve around the segment midpoint (part of the adjustFlows duplication flagged in the audit). --- src/diagram/drawing/Flow.tsx | 31 +++++++++++++++ src/diagram/flow-attach.ts | 12 +++++- src/diagram/tests/flow-attach.test.ts | 57 +++++++++++++++++++++++++++ 3 files changed, 99 insertions(+), 1 deletion(-) diff --git a/src/diagram/drawing/Flow.tsx b/src/diagram/drawing/Flow.tsx index a63a1f12d..77cca1357 100644 --- a/src/diagram/drawing/Flow.tsx +++ b/src/diagram/drawing/Flow.tsx @@ -674,6 +674,37 @@ function adjustFlows( }); } +/** + * Pin a flow's stock-attached SOURCE endpoint onto the stock edge facing the + * sink, preserving the flow's orientation (the perpendicular coordinate is + * untouched, so a straight flow stays straight) and re-clamping the valve to + * the resulting path. Used at flow-creation commit: the flow tool stages the + * source point at the press-time stock CENTER and only routes the sink, so + * without this the persisted source endpoint sits hidden under the stock body + * (violating the edge-attachment rule) until the next stock drag re-pins it. + */ +export function pinSourceToStockEdge(flow: FlowViewElement, stock: StockViewElement): FlowViewElement { + if (flow.points.length < 2) { + return flow; + } + const srcPt = first(flow.points); + const sinkPt = last(flow.points); + if (srcPt.attachedToUid !== stock.uid) { + return flow; + } + + const horizontal = isDominantlyHorizontal(srcPt, sinkPt); + const side: Side = horizontal ? (sinkPt.x >= stock.x ? 'right' : 'left') : sinkPt.y >= stock.y ? 'bottom' : 'top'; + const edge = getStockEdgePoint(stock.x, stock.y, side); + const newSrc: Point = horizontal ? { ...srcPt, x: edge.x } : { ...srcPt, y: edge.y }; + const points: readonly Point[] = [newSrc, ...flow.points.slice(1)]; + + const currentValve: IPoint = { x: flow.x, y: flow.y }; + const segments = getSegments(points); + const valve = clampToSegment(currentValve, findClosestSegment(currentValve, segments)); + return { ...flow, x: valve.x, y: valve.y, points }; +} + export function UpdateStockAndFlows( stockEl: StockViewElement, flows: readonly FlowViewElement[], diff --git a/src/diagram/flow-attach.ts b/src/diagram/flow-attach.ts index 190b5bdb3..a25144a0a 100644 --- a/src/diagram/flow-attach.ts +++ b/src/diagram/flow-attach.ts @@ -42,7 +42,7 @@ import { } from '@simlin/core/datamodel'; import type { JsonModelOperation } from '@simlin/engine'; -import { UpdateCloudAndFlow } from './drawing/Flow'; +import { pinSourceToStockEdge, UpdateCloudAndFlow } from './drawing/Flow'; // The drawing-layer Point is a bare {x, y} (no attachedToUid). cursorMoveDelta // and fauxTargetCenter are screen-space positions/deltas, not flow points, so // they use this type -- matching what Canvas passes to onMoveFlow. @@ -396,6 +396,16 @@ export function computeFlowAttachment( elements = [...elements, to]; } } + // A flow drawn OUT of a stock stages its source point at the stock's + // CENTER; now that the sink is routed, pin the source onto the facing + // edge so the persisted endpoint honors the edge-attachment rule (it + // otherwise hides under the stock body until the next stock drag). + if (sourceUid !== undefined && sourceUid !== inCreationCloudUid) { + const sourceEl = getUid(sourceUid); + if (sourceEl.type === 'stock') { + flow = pinSourceToStockEdge(flow, sourceEl); + } + } elements = [...elements, flow]; selection = new Set([flow.uid]); isCreatingNew = true; diff --git a/src/diagram/tests/flow-attach.test.ts b/src/diagram/tests/flow-attach.test.ts index 40955bffb..7430c6df8 100644 --- a/src/diagram/tests/flow-attach.test.ts +++ b/src/diagram/tests/flow-attach.test.ts @@ -515,6 +515,63 @@ describe('computeFlowAttachment', () => { expect(sinkPt.y).toBe(sourcePt.y); // horizontal }); + it('stock source: the source endpoint pins to the stock EDGE facing the sink, not its center', () => { + // Faithful to runtime: pressing the flow tool ON a stock stages the + // source point at the stock's CENTER, and only the sink is routed at + // commit -- so the persisted source endpoint hid under the stock body + // (violating the edge-attachment rule) until the next stock drag + // re-pinned it. The commit must pin the source to the facing edge. + const srcStock = makeStockEl(1, 'src', 100, 100); + const sinkStock = makeStockEl(2, 'snk', 400, 100); + const flow = makeFlowEl(inCreationUid, 'new_flow', 100, 100, [ + { x: 100, y: 100, attachedToUid: 1 }, + { x: 100, y: 100, attachedToUid: fauxCloudTargetUid }, + ]); + const view = makeView([srcStock, sinkStock], 5); + const variables = varsOf(makeStockVar('src'), makeStockVar('snk')); + + const result = computeFlowAttachment(view, variables, params({ flow, targetUid: 2, inCreation: true })); + + const realFlow = result.elements.find((e) => e.type === 'flow') as FlowViewElement; + const sourcePt = realFlow.points[0]; + const sinkPt = realFlow.points[realFlow.points.length - 1]; + // Source on the RIGHT edge of the source stock (the sink is to the right). + expect(sourcePt.attachedToUid).toBe(1); + expect(sourcePt.x).toBeCloseTo(100 + StockWidth / 2); + expect(sourcePt.x).not.toBe(100); + // Sink on the LEFT edge of the target stock; flow horizontal and straight. + expect(sinkPt.x).toBeCloseTo(400 - StockWidth / 2); + expect(sinkPt.y).toBe(sourcePt.y); + expect(realFlow.points.length).toBe(2); + // The valve sits on the visible segment between the two edges. + expect(realFlow.x).toBeGreaterThan(sourcePt.x); + expect(realFlow.x).toBeLessThan(sinkPt.x); + }); + + it('stock source dragged UP to empty space: the source pins to the TOP edge', () => { + const srcStock = makeStockEl(1, 'src', 100, 300); + const flow = makeFlowEl(inCreationUid, 'new_flow', 100, 300, [ + { x: 100, y: 300, attachedToUid: 1 }, + { x: 100, y: 300, attachedToUid: fauxCloudTargetUid }, + ]); + const view = makeView([srcStock], 5); + const variables = varsOf(makeStockVar('src')); + + // Release 150px above the press: the sink cloud materializes there. + const result = computeFlowAttachment( + view, + variables, + params({ flow, inCreation: true, fauxTargetCenter: { x: 100, y: 150 }, cursorMoveDelta: { x: 0, y: 150 } }), + ); + + const realFlow = result.elements.find((e) => e.type === 'flow') as FlowViewElement; + const sourcePt = realFlow.points[0]; + const sinkPt = realFlow.points[realFlow.points.length - 1]; + expect(sourcePt.attachedToUid).toBe(1); + expect(sourcePt.y).toBeCloseTo(300 - StockHeight / 2); // top edge, not center + expect(sourcePt.x).toBe(sinkPt.x); // vertical + }); + it('stock source to empty space: faux target becomes a new cloud at fauxTargetCenter', () => { const srcStock = makeStockEl(1, 'src', 0, 100); const flow = makeFlowEl(inCreationUid, 'new_flow', 150, 100, [ From b5f76847b98a0dc06df8d5fc24259c9c09cee70e Mon Sep 17 00:00:00 2001 From: Bobby Powers Date: Tue, 30 Jun 2026 23:19:25 -0700 Subject: [PATCH 9/9] diagram: dedup flow-editing routing helpers Behavior-preserving P3 cleanup of the interactive flow-editing code, per docs/dev/flow-editing-audit-2026-06.md. No runtime behavior changes; the diagram jest suite passes identically (plus new characterization tests). group-movement.ts: the cloud "straight-flow to L when moved diagonally" conversion appeared three times -- in routeCloudEndpointFlow and twice inside routeUnselectedFlows' near-duplicate source/sink loops. The two loops are now one helper parameterized on which end is the moved endpoint, reusing routeCloudEndpointFlow. Their per-endpoint `cloudIsSelected` guard was always true by construction (the maps are keyed only by selected endpoints), so the dead branch is removed. The repeated "propose valve at flow - delta, clamp to path" block is extracted as clampDraggedValve. Dead weight removed: the deprecated Point2D alias (unused outside the file), preProcessSelectedFlows' always-empty sideEffects return, the `hasSelectedEndpoints` expression that reduced to `selection.size > 0`, and routeUnselectedFlows' redundant second element iterable (every caller passed the same values twice, and it built two identical maps). Flow.tsx: the recurring getSegments -> findClosestSegment -> clampToSegment valve re-clamp is extracted as clampValveToClosestSegment and applied at the clean sites (both straight-to-L reroute branches plus computeFlowRoute and the segment-move path). The straight-to-L corner construction in UpdateCloudAndFlow vs UpdateFlow is intentionally NOT unified: the two model different endpoints (single cloud with a cloud var vs one-or-both clouds with a clouds array) and return different shapes, so forcing a shared corner builder would obscure both without removing real duplication. The adjustFlows FIXME is replaced with a comment explaining that the isCloud and non-cloud valve formulas differ deliberately (absolute mirror vs signed offset) and must not be merged; the non-cloud branch is noted as currently unreachable (sole caller passes isCloud=true; tracked in #822). Creation sentinel UIDs (inCreation/faux target/cloud) had duplicate definitions in Canvas.tsx and flow-attach.ts with a "keep in sync" comment and a pinning test. They now live in one module, drawing/creation-sentinels.ts, re-exported by both so every import path keeps resolving; the sync test asserts against the shared source. --- src/diagram/drawing/Canvas.tsx | 9 +- src/diagram/drawing/Flow.tsx | 46 +++- src/diagram/drawing/creation-sentinels.ts | 25 ++ src/diagram/flow-attach.ts | 13 +- src/diagram/group-movement.ts | 319 ++++++---------------- src/diagram/tests/flow-attach.test.ts | 22 +- src/diagram/tests/flow-routing.test.ts | 50 ++++ src/diagram/tests/group-movement.test.ts | 89 ++++++ 8 files changed, 303 insertions(+), 270 deletions(-) create mode 100644 src/diagram/drawing/creation-sentinels.ts diff --git a/src/diagram/drawing/Canvas.tsx b/src/diagram/drawing/Canvas.tsx index d9ccfdaec..dda349ea2 100644 --- a/src/diagram/drawing/Canvas.tsx +++ b/src/diagram/drawing/Canvas.tsx @@ -89,10 +89,11 @@ import { import styles from './Canvas.module.css'; -export const inCreationUid = -2; -export const fauxTargetUid = -3; -export const inCreationCloudUid = -4; -export const fauxCloudTargetUid = -5; +// The creation sentinel UIDs live in one module (drawing/creation-sentinels) +// and are re-exported here so existing `from '../drawing/Canvas'` imports keep +// resolving. See creation-sentinels.ts for what each marks. +export { inCreationUid, fauxTargetUid, inCreationCloudUid, fauxCloudTargetUid } from './creation-sentinels'; +import { inCreationUid, fauxTargetUid, inCreationCloudUid, fauxCloudTargetUid } from './creation-sentinels'; const fauxTarget: AuxViewElement = { type: 'aux', diff --git a/src/diagram/drawing/Flow.tsx b/src/diagram/drawing/Flow.tsx index 77cca1357..32246baed 100644 --- a/src/diagram/drawing/Flow.tsx +++ b/src/diagram/drawing/Flow.tsx @@ -494,9 +494,7 @@ export function computeFlowRoute( // Preserve valve position by clamping to the closest segment of the new L-shape const currentValve: IPoint = { x: flow.x, y: flow.y }; - const newSegments = getSegments(newPoints); - const closestSegment = findClosestSegment(currentValve, newSegments); - const clampedValve = clampToSegment(currentValve, closestSegment); + const clampedValve = clampValveToClosestSegment(currentValve, newPoints); return { ...flow, @@ -630,13 +628,25 @@ function adjustFlows( }; } - // FIXME: reduce this duplication + // The isCloud and non-cloud branches deliberately compute the valve + // differently and must NOT be unified. isCloud mirrors the valve around the + // segment: base = min(otherEnd, moved end) plus the ABSOLUTE scaled offset, + // so an off-center valve stays between the two ends even when the drag flips + // the segment orientation (the moved end crossing past otherEnd). The + // non-cloud branch offsets by the SIGNED scaled distance from otherEnd -- + // correct for a stock endpoint that keeps its side, but divergent from the + // cloud formula for off-center / sign-flip cases. The shared arithmetic (the + // raw fraction, `d`) is too small and entangled with these differing + // combinators to factor out without obscuring both formulas. + // + // NOTE: the non-cloud branch is currently unreachable -- adjustFlows' sole + // caller (UpdateCloudAndFlow, straight-flow parallel drag) always passes + // isCloud=true. It is retained as the general signed formula. if (isCloud) { // Guard the denominators against zero: for a vertical flow origStock.x === // otherEnd.x (and likewise y for a horizontal flow), which without the // `|| 1` divides by zero and yields a NaN/Infinity valve -- serialized to - // JSON null, that bricks the model (#818). The non-cloud branch below - // already guards this; the duplication kept the fix out of this copy. + // JSON null, that bricks the model (#818). const fraction = { x: flow.x === otherEnd.x ? 0.5 : (stock.x - otherEnd.x) / (origStock.x - otherEnd.x || 1), y: flow.y === otherEnd.y ? 0.5 : (stock.y - otherEnd.y) / (origStock.y - otherEnd.y || 1), @@ -837,9 +847,7 @@ export function UpdateCloudAndFlow( }; // Clamp valve to closest segment of new shape - const newSegments = getSegments(points); - const closestSeg = findClosestSegment(currentValve, newSegments); - const newValve = clampToSegment(currentValve, closestSeg); + const newValve = clampValveToClosestSegment(currentValve, points); flow = { ...flow, @@ -1254,6 +1262,18 @@ const VALVE_HIT_TOLERANCE = 5; // Margin from segment endpoints when clamping valve position const VALVE_CLAMP_MARGIN = 10; +/** + * Snap a valve onto the closest segment of a flow path -- the recurring + * getSegments -> findClosestSegment -> clampToSegment chain run wherever a + * flow's geometry changes (stock/cloud moves, segment drags, L-shape + * conversions). Assumes `points` has at least two points, which every caller + * guarantees, so `findClosestSegment` always has a segment to return. + */ +function clampValveToClosestSegment(valve: IPoint, points: readonly Point[]): IPoint { + const segments = getSegments(points); + return clampToSegment(valve, findClosestSegment(valve, segments)); +} + /** * Preserves the valve's fractional position when a segment changes. * @@ -1461,9 +1481,7 @@ export function UpdateFlow( // Dragging any segment can affect adjacent segments via shared corners, // so the valve's segment may have changed shape even if it wasn't the // segment being dragged. - const newSegments = getSegments(points); - const closestSeg = findClosestSegment(currentValve, newSegments); - const newValve = clampToSegment(currentValve, closestSeg); + const newValve = clampValveToClosestSegment(currentValve, points); flowEl = { ...flowEl, x: newValve.x, @@ -1570,9 +1588,7 @@ export function UpdateFlow( }); // Clamp valve to the closest segment of the new shape - const newSegments = getSegments(points); - const closestSeg = findClosestSegment(currentValve, newSegments); - const newValve = clampToSegment(currentValve, closestSeg); + const newValve = clampValveToClosestSegment(currentValve, points); flowEl = { ...flowEl, diff --git a/src/diagram/drawing/creation-sentinels.ts b/src/diagram/drawing/creation-sentinels.ts new file mode 100644 index 000000000..072b0f53a --- /dev/null +++ b/src/diagram/drawing/creation-sentinels.ts @@ -0,0 +1,25 @@ +// Copyright 2026 The Simlin Authors. All rights reserved. +// Use of this source code is governed by the Apache License, +// Version 2.0, that can be found in the LICENSE file. + +/** + * Sentinel UIDs used while a flow is being created or dragged. + * + * During flow creation the Canvas stages placeholder elements that do not yet + * exist in the persisted view; these negative UIDs mark them so the creation + * logic (Canvas rendering, `flow-attach`'s `computeFlowAttachment`) can + * recognize and later replace them with real, positive UIDs on commit. + * + * This is the single source of truth. `drawing/Canvas.tsx` and `flow-attach.ts` + * both re-export these so existing import paths keep resolving, and so the + * functional-core `flow-attach` module stays free of React/DOM imports. + */ + +/** The in-creation flow itself (replaced with a real uid on commit). */ +export const inCreationUid = -2; +/** The faux drag target under the cursor while reattaching an existing flow endpoint. */ +export const fauxTargetUid = -3; +/** The source cloud staged when a new flow is drawn out of empty space. */ +export const inCreationCloudUid = -4; +/** The faux sink target a new flow points at until it snaps to a stock/cloud. */ +export const fauxCloudTargetUid = -5; diff --git a/src/diagram/flow-attach.ts b/src/diagram/flow-attach.ts index a25144a0a..78c754501 100644 --- a/src/diagram/flow-attach.ts +++ b/src/diagram/flow-attach.ts @@ -48,13 +48,12 @@ import { pinSourceToStockEdge, UpdateCloudAndFlow } from './drawing/Flow'; // they use this type -- matching what Canvas passes to onMoveFlow. import type { Point as CanvasPoint } from './drawing/common'; -// Sentinel UIDs used by Canvas during flow creation. Duplicated here (rather -// than imported from Canvas.tsx) so this functional-core module stays free of -// React/DOM dependencies. They must stay in sync with the exports in -// drawing/Canvas.tsx. -export const inCreationUid = -2; -export const inCreationCloudUid = -4; -export const fauxCloudTargetUid = -5; +// Sentinel UIDs used by Canvas during flow creation, defined once in +// drawing/creation-sentinels and re-exported here so this functional-core +// module stays free of React/DOM dependencies while callers that import them +// from `../flow-attach` keep resolving. +export { inCreationUid, inCreationCloudUid, fauxCloudTargetUid } from './drawing/creation-sentinels'; +import { inCreationUid, inCreationCloudUid, fauxCloudTargetUid } from './drawing/creation-sentinels'; /** * Inputs to `computeFlowAttachment`, mirroring the arguments Canvas passes to diff --git a/src/diagram/group-movement.ts b/src/diagram/group-movement.ts index 5ace520ba..d2d302802 100644 --- a/src/diagram/group-movement.ts +++ b/src/diagram/group-movement.ts @@ -49,15 +49,27 @@ export interface MovementDelta { } /** - * @deprecated Use MovementDelta instead. Kept for backwards compatibility. + * Move a selected flow's valve by the drag delta and clamp it back onto the + * flow's (already re-routed) path. + * + * A dragged flow's valve should track the pointer -- flow.x/y shifted by the + * drag delta -- and then snap to the closest segment of the new geometry. The + * routing helpers (computeFlowRoute, the pre-processed pass) instead preserve + * the valve's OLD fractional position, which lags behind the drag; this + * re-clamps it. Returns undefined when the path has no segments (fewer than two + * points) so each caller keeps its own fallback for that unreachable case. */ -export type Point2D = MovementDelta; - -export interface GroupMovementResult { - /** Map from element UID to updated element (for elements that changed) */ - updatedElements: Map; - /** Additional elements to update (clouds updated via flow routing, etc.) */ - sideEffects: ViewElement[]; +function clampDraggedValve( + flow: FlowViewElement, + delta: MovementDelta, + points: readonly Point[], +): { x: number; y: number } | undefined { + const proposedValve = { x: flow.x - delta.x, y: flow.y - delta.y }; + const segments = getSegments(points); + if (segments.length === 0) { + return undefined; + } + return clampToSegment(proposedValve, findClosestSegment(proposedValve, segments)); } /** @@ -151,7 +163,7 @@ function routeCloudEndpointFlow( export function computePreRoutedOffsets( elements: Iterable, selectedStockUids: Set, - delta: Point2D, + delta: MovementDelta, isInSelection: (uid: UID | undefined) => boolean, ): Map { const preComputedOffsets = new Map(); @@ -225,12 +237,11 @@ export function preProcessSelectedFlows( elements: Iterable, selectedFlowUids: Set, preComputedOffsets: Map, - delta: Point2D, + delta: MovementDelta, isInSelection: (uid: UID | undefined) => boolean, getElementByUid: (uid: UID) => ViewElement | undefined, -): [Map, ViewElement[]] { +): Map { const preProcessedFlows = new Map(); - const sideEffects: ViewElement[] = []; for (const element of elements) { if (element.type !== 'flow') continue; @@ -266,7 +277,7 @@ export function preProcessSelectedFlows( } } - return [preProcessedFlows, sideEffects]; + return preProcessedFlows; } /** @@ -281,7 +292,7 @@ export function preProcessSelectedFlows( */ export function processSelectedFlow( flow: FlowViewElement, - delta: Point2D, + delta: MovementDelta, isInSelection: (uid: UID | undefined) => boolean, preProcessedFlows: Map, getElementByUid: (uid: UID) => ViewElement | undefined, @@ -319,20 +330,10 @@ export function processSelectedFlow( // Check if this flow was pre-processed (for multi-flow spacing preservation) const preProcessed = preProcessedFlows.get(flow.uid); if (preProcessed) { - // When the flow is selected, the valve should move with the drag delta, - // then be clamped to the new flow path. The pre-processed flow has the - // valve's fractional position preserved, which is wrong when selected. - const proposedValve = { - x: flow.x - delta.x, - y: flow.y - delta.y, - }; - const segments = getSegments(preProcessed.points); - if (segments.length > 0) { - const closestSegment = findClosestSegment(proposedValve, segments); - const clampedValve = clampToSegment(proposedValve, closestSegment); - return [{ ...preProcessed, x: clampedValve.x, y: clampedValve.y }, sideEffects]; - } - return [preProcessed, sideEffects]; + // The pre-processed flow preserved the valve's fractional position, which + // lags the drag when the flow itself is selected; re-clamp it to the path. + const clampedValve = clampDraggedValve(flow, delta, preProcessed.points); + return [clampedValve ? { ...preProcessed, x: clampedValve.x, y: clampedValve.y } : preProcessed, sideEffects]; } // Handle different endpoint types: stocks need computeFlowRoute, clouds need UpdateCloudAndFlow @@ -345,17 +346,10 @@ export function processSelectedFlow( // Use default offset of 0.5 (center) for selected flows let updatedFlow = computeFlowRoute(flow, sourceEndpoint, newStockCx, newStockCy, 0.5); - // When the flow is selected, the valve should move with the drag delta, - // then be clamped to the new flow path. computeFlowRoute preserves the - // valve's fractional position which is wrong when the flow is selected. - const proposedValve = { - x: flow.x - delta.x, - y: flow.y - delta.y, - }; - const segments = getSegments(updatedFlow.points); - if (segments.length > 0) { - const closestSegment = findClosestSegment(proposedValve, segments); - const clampedValve = clampToSegment(proposedValve, closestSegment); + // computeFlowRoute preserves the valve's fractional position, which lags + // the drag when the flow is selected; re-clamp it to the new path. + const clampedValve = clampDraggedValve(flow, delta, updatedFlow.points); + if (clampedValve) { updatedFlow = { ...updatedFlow, x: clampedValve.x, y: clampedValve.y }; } return [updatedFlow, sideEffects]; @@ -373,17 +367,10 @@ export function processSelectedFlow( // Use default offset of 0.5 (center) for selected flows let updatedFlow = computeFlowRoute(flow, sinkEndpoint, newStockCx, newStockCy, 0.5); - // When the flow is selected, the valve should move with the drag delta, - // then be clamped to the new flow path. computeFlowRoute preserves the - // valve's fractional position which is wrong when the flow is selected. - const proposedValve = { - x: flow.x - delta.x, - y: flow.y - delta.y, - }; - const segments = getSegments(updatedFlow.points); - if (segments.length > 0) { - const closestSegment = findClosestSegment(proposedValve, segments); - const clampedValve = clampToSegment(proposedValve, closestSegment); + // computeFlowRoute preserves the valve's fractional position, which lags + // the drag when the flow is selected; re-clamp it to the new path. + const clampedValve = clampDraggedValve(flow, delta, updatedFlow.points); + if (clampedValve) { updatedFlow = { ...updatedFlow, x: clampedValve.x, y: clampedValve.y }; } return [updatedFlow, sideEffects]; @@ -458,39 +445,24 @@ export function processSelectedFlow( } // Stock-to-stock flows: move valve but clamp to flow path - const proposedValve = { - x: flow.x - delta.x, - y: flow.y - delta.y, - }; - const segments = getSegments(pts); - if (segments.length > 0) { - const closestSegment = findClosestSegment(proposedValve, segments); - const clampedValve = clampToSegment(proposedValve, closestSegment); - return [ - { - ...flow, - x: clampedValve.x, - y: clampedValve.y, - }, - sideEffects, - ]; - } - return [ - { - ...flow, - x: proposedValve.x, - y: proposedValve.y, - }, - sideEffects, - ]; + const valve = clampDraggedValve(flow, delta, pts) ?? { x: flow.x - delta.x, y: flow.y - delta.y }; + return [{ ...flow, x: valve.x, y: valve.y }, sideEffects]; } } /** * Route unselected flows attached to selected endpoints. * - * @param elements All view elements (after pass 1 updates) - * @param originalElements Original view elements (before movement) + * Called after the selected elements have moved (pass 1). Each unselected flow + * whose source and/or sink endpoint is in the selection is re-routed to follow + * the moved endpoint(s): a flow with BOTH ends selected translates uniformly; + * otherwise the flow routes to its single moved endpoint -- stocks via + * `computeFlowRoute` (honoring the pre-computed spacing offset) and clouds via + * `routeCloudEndpointFlow`. + * + * @param elements All view elements at their ORIGINAL (pre-movement) positions. + * The routing helpers derive the endpoints' new positions from `delta`, so + * they need the originals rather than the already-moved copies. * @param selection Selected UIDs * @param preComputedOffsets Pre-computed offsets * @param delta The movement delta @@ -498,25 +470,18 @@ export function processSelectedFlow( */ export function routeUnselectedFlows( elements: Iterable, - originalElements: Iterable, selection: ReadonlySet, preComputedOffsets: Map, - delta: Point2D, + delta: MovementDelta, ): ViewElement[] { const updatedFlows: ViewElement[] = []; - // Build maps FIRST so we can reuse them (iterators can only be consumed once) const elementsMap = new Map(); for (const el of elements) { elementsMap.set(el.uid, el); } - const originalElementsMap = new Map(); - for (const el of originalElements) { - originalElementsMap.set(el.uid, el); - } - const getOriginalElement = (uid: UID) => originalElementsMap.get(uid); - // Collect flows grouped by their attached endpoint + // Collect flows grouped by whichever attached endpoint is selected. const flowsBySourceEndpoint = new Map(); const flowsBySinkEndpoint = new Map(); const bothEndsSelectedFlows: FlowViewElement[] = []; @@ -546,7 +511,7 @@ export function routeUnselectedFlows( } } - // Handle flows where both ends are selected: translate uniformly + // Flows with both ends selected translate uniformly. for (const element of bothEndsSelectedFlows) { const pts = element.points; const newPoints = pts.map((p) => ({ @@ -562,147 +527,31 @@ export function routeUnselectedFlows( }); } - // Update flows grouped by source endpoint using pre-computed offsets - for (const [endpointUid, flows] of flowsBySourceEndpoint) { - const movedEndpoint = elementsMap.get(endpointUid); - - if (movedEndpoint && movedEndpoint.type === 'stock') { - const originalStock = getOriginalElement(endpointUid) as StockViewElement; - const newStockCx = originalStock.x - delta.x; - const newStockCy = originalStock.y - delta.y; - for (const flow of flows) { - const offset = preComputedOffsets.get(flow.uid) ?? 0.5; - const updatedFlow = computeFlowRoute(flow, originalStock, newStockCx, newStockCy, offset); - updatedFlows.push(updatedFlow); - } - } else if (movedEndpoint && movedEndpoint.type === 'cloud') { - // For clouds, use UpdateCloudAndFlow for routing but honor full delta if cloud is selected - const originalCloud = getOriginalElement(endpointUid) as CloudViewElement | undefined; - if (originalCloud) { - const cloudIsSelected = selection.has(endpointUid); - const newCloudX = originalCloud.x - delta.x; - const newCloudY = originalCloud.y - delta.y; + // Route flows anchored to a single moved endpoint. `isSource` records which + // end of the flow the grouped endpoint is (source = first point, sink = last). + // Because the maps are keyed only by endpoints that ARE selected, the cloud + // path always honors the full delta -- the former per-endpoint `cloudIsSelected` + // guard was true by construction and has been removed. + const routeForEndpoint = (flowsByEndpoint: Map, isSource: boolean): void => { + for (const [endpointUid, flows] of flowsByEndpoint) { + const endpoint = elementsMap.get(endpointUid); + if (endpoint?.type === 'stock') { + const newStockCx = endpoint.x - delta.x; + const newStockCy = endpoint.y - delta.y; for (const flow of flows) { - let [, updatedFlow] = UpdateCloudAndFlow(originalCloud, flow, delta); - // If cloud is selected, ensure flow matches full delta position with proper orthogonal geometry - if (cloudIsSelected) { - const cloudPointIndex = - first(updatedFlow.points).attachedToUid === endpointUid ? 0 : updatedFlow.points.length - 1; - const cloudPoint = updatedFlow.points[cloudPointIndex]; - const otherPointIndex = cloudPointIndex === 0 ? updatedFlow.points.length - 1 : 0; - const otherPoint = updatedFlow.points[otherPointIndex]; - if (cloudPoint && otherPoint) { - // Check if the flow is 2-point straight and we'd create a diagonal - const needsLShape = - updatedFlow.points.length === 2 && - Math.abs(newCloudX - otherPoint.x) > MIN_DIAGONAL_DISTANCE && - Math.abs(newCloudY - otherPoint.y) > MIN_DIAGONAL_DISTANCE; - if (needsLShape) { - // Add intermediate point to create L-shape (horizontal then vertical) - const intermediateX = newCloudX; - const intermediateY = otherPoint.y; - const intermediatePoint: Point = { x: intermediateX, y: intermediateY, attachedToUid: undefined }; - const newCloudPoint: Point = { ...cloudPoint, x: newCloudX, y: newCloudY }; - // Cloud is source (index 0): cloud -> intermediate -> other - // Cloud is sink (index size-1): other -> intermediate -> cloud - const newPoints: readonly Point[] = - cloudPointIndex === 0 - ? [newCloudPoint, intermediatePoint, otherPoint] - : [otherPoint, intermediatePoint, newCloudPoint]; - updatedFlow = { ...updatedFlow, points: newPoints }; - // Re-clamp valve to the new path - const segments = getSegments(newPoints); - if (segments.length > 0) { - const closestSegment = findClosestSegment({ x: updatedFlow.x, y: updatedFlow.y }, segments); - const clampedValve = clampToSegment({ x: updatedFlow.x, y: updatedFlow.y }, closestSegment); - updatedFlow = { ...updatedFlow, x: clampedValve.x, y: clampedValve.y }; - } - } else { - const updatedPoints = arrayWith(updatedFlow.points, cloudPointIndex, { - ...cloudPoint, - x: newCloudX, - y: newCloudY, - }); - updatedFlow = { ...updatedFlow, points: updatedPoints }; - } - } - } - updatedFlows.push(updatedFlow); + const offset = preComputedOffsets.get(flow.uid) ?? 0.5; + updatedFlows.push(computeFlowRoute(flow, endpoint, newStockCx, newStockCy, offset)); } - } - } - } - - // Update flows grouped by sink endpoint using pre-computed offsets - for (const [endpointUid, flows] of flowsBySinkEndpoint) { - const movedEndpoint = elementsMap.get(endpointUid); - - if (movedEndpoint && movedEndpoint.type === 'stock') { - const originalStock = getOriginalElement(endpointUid) as StockViewElement; - const newStockCx = originalStock.x - delta.x; - const newStockCy = originalStock.y - delta.y; - for (const flow of flows) { - const offset = preComputedOffsets.get(flow.uid) ?? 0.5; - const updatedFlow = computeFlowRoute(flow, originalStock, newStockCx, newStockCy, offset); - updatedFlows.push(updatedFlow); - } - } else if (movedEndpoint && movedEndpoint.type === 'cloud') { - // For clouds, use UpdateCloudAndFlow for routing but honor full delta if cloud is selected - const originalCloud = getOriginalElement(endpointUid) as CloudViewElement | undefined; - if (originalCloud) { - const cloudIsSelected = selection.has(endpointUid); - const newCloudX = originalCloud.x - delta.x; - const newCloudY = originalCloud.y - delta.y; + } else if (endpoint?.type === 'cloud') { for (const flow of flows) { - let [, updatedFlow] = UpdateCloudAndFlow(originalCloud, flow, delta); - // If cloud is selected, ensure flow matches full delta position with proper orthogonal geometry - if (cloudIsSelected) { - const cloudPointIndex = - last(updatedFlow.points).attachedToUid === endpointUid ? updatedFlow.points.length - 1 : 0; - const cloudPoint = updatedFlow.points[cloudPointIndex]; - const otherPointIndex = cloudPointIndex === 0 ? updatedFlow.points.length - 1 : 0; - const otherPoint = updatedFlow.points[otherPointIndex]; - if (cloudPoint && otherPoint) { - // Check if the flow is 2-point straight and we'd create a diagonal - const needsLShape = - updatedFlow.points.length === 2 && - Math.abs(newCloudX - otherPoint.x) > MIN_DIAGONAL_DISTANCE && - Math.abs(newCloudY - otherPoint.y) > MIN_DIAGONAL_DISTANCE; - if (needsLShape) { - // Add intermediate point to create L-shape (horizontal then vertical) - const intermediateX = newCloudX; - const intermediateY = otherPoint.y; - const intermediatePoint: Point = { x: intermediateX, y: intermediateY, attachedToUid: undefined }; - const newCloudPoint: Point = { ...cloudPoint, x: newCloudX, y: newCloudY }; - // Cloud is source (index 0): cloud -> intermediate -> other - // Cloud is sink (index size-1): other -> intermediate -> cloud - const newPoints: readonly Point[] = - cloudPointIndex === 0 - ? [newCloudPoint, intermediatePoint, otherPoint] - : [otherPoint, intermediatePoint, newCloudPoint]; - updatedFlow = { ...updatedFlow, points: newPoints }; - // Re-clamp valve to the new path - const segments = getSegments(newPoints); - if (segments.length > 0) { - const closestSegment = findClosestSegment({ x: updatedFlow.x, y: updatedFlow.y }, segments); - const clampedValve = clampToSegment({ x: updatedFlow.x, y: updatedFlow.y }, closestSegment); - updatedFlow = { ...updatedFlow, x: clampedValve.x, y: clampedValve.y }; - } - } else { - const updatedPoints = arrayWith(updatedFlow.points, cloudPointIndex, { - ...cloudPoint, - x: newCloudX, - y: newCloudY, - }); - updatedFlow = { ...updatedFlow, points: updatedPoints }; - } - } - } - updatedFlows.push(updatedFlow); + updatedFlows.push(routeCloudEndpointFlow(endpoint, flow, delta, isSource).updatedFlow); } } } - } + }; + + routeForEndpoint(flowsBySourceEndpoint, true); + routeForEndpoint(flowsBySinkEndpoint, false); return updatedFlows; } @@ -756,8 +605,8 @@ export function processLinks( originalElements: Map, updatedElements: Map, selection: ReadonlySet, - delta: Point2D, - arcPoint?: Point2D, + delta: MovementDelta, + arcPoint?: MovementDelta, ): Map { const result = new Map(); @@ -919,7 +768,7 @@ export function applyGroupMovement(input: GroupMovementInput): GroupMovementOutp : new Map(); // Pre-process selected flows with one endpoint selected (stock endpoint only) - const [preProcessedFlows] = + const preProcessedFlows = selection.size > 1 ? preProcessSelectedFlows( originalElements.values(), @@ -929,7 +778,7 @@ export function applyGroupMovement(input: GroupMovementInput): GroupMovementOutp isInSelection, (uid) => originalElements.get(uid), ) - : [new Map()]; + : new Map(); // Process selected flows for (const uid of selection) { @@ -1003,18 +852,12 @@ export function applyGroupMovement(input: GroupMovementInput): GroupMovementOutp // Route unselected flows attached to selected endpoints. // This applies even for single-element selection (e.g., moving a single stock - // should route its attached flows). - // Note: We use originalElements.values() here because the input `elements` iterator - // was already consumed when building originalElements. - const hasSelectedEndpoints = selectedStockUids.size > 0 || selection.size > 0; - if (hasSelectedEndpoints) { - const routedFlows = routeUnselectedFlows( - originalElements.values(), - originalElements.values(), - selection, - preComputedOffsets, - delta, - ); + // should route its attached flows). routeUnselectedFlows needs the ORIGINAL + // endpoint positions (it derives the new ones from `delta`), so we pass + // originalElements.values() -- the input `elements` iterator was already + // consumed when building originalElements. + if (selection.size > 0) { + const routedFlows = routeUnselectedFlows(originalElements.values(), selection, preComputedOffsets, delta); for (const flow of routedFlows) { updatedElements.set(flow.uid, flow); } diff --git a/src/diagram/tests/flow-attach.test.ts b/src/diagram/tests/flow-attach.test.ts index 7430c6df8..08a8fdb5e 100644 --- a/src/diagram/tests/flow-attach.test.ts +++ b/src/diagram/tests/flow-attach.test.ts @@ -27,6 +27,11 @@ import { inCreationCloudUid as canvasInCreationCloudUid, inCreationUid as canvasInCreationUid, } from '../drawing/Canvas'; +import { + fauxCloudTargetUid as sharedFauxCloudTargetUid, + inCreationCloudUid as sharedInCreationCloudUid, + inCreationUid as sharedInCreationUid, +} from '../drawing/creation-sentinels'; import { StockWidth, StockHeight } from '../drawing/default'; // ----- fixture helpers (mirroring flow-routing.test.ts patterns) ----- @@ -151,12 +156,17 @@ function payloadOf(op: JsonModelOperation): { ident: string; inflows: string[]; } describe('computeFlowAttachment', () => { - // The flow-attach module re-declares the Canvas creation sentinels to stay - // free of React/DOM imports. Guard that the duplicates never drift. - it('keeps creation sentinel constants in sync with Canvas', () => { - expect(inCreationUid).toBe(canvasInCreationUid); - expect(inCreationCloudUid).toBe(canvasInCreationCloudUid); - expect(fauxCloudTargetUid).toBe(canvasFauxCloudTargetUid); + // The creation sentinels are defined once in drawing/creation-sentinels and + // re-exported by both flow-attach and Canvas. This guards that every import + // path stays wired to that single source (a trivial identity now, but it + // keeps the compatibility of both re-export paths under test). + it('keeps creation sentinel constants in sync across import paths', () => { + expect(inCreationUid).toBe(sharedInCreationUid); + expect(inCreationCloudUid).toBe(sharedInCreationCloudUid); + expect(fauxCloudTargetUid).toBe(sharedFauxCloudTargetUid); + expect(canvasInCreationUid).toBe(sharedInCreationUid); + expect(canvasInCreationCloudUid).toBe(sharedInCreationCloudUid); + expect(canvasFauxCloudTargetUid).toBe(sharedFauxCloudTargetUid); }); describe('sink reattach', () => { diff --git a/src/diagram/tests/flow-routing.test.ts b/src/diagram/tests/flow-routing.test.ts index 02f2c94b4..b0470cf76 100644 --- a/src/diagram/tests/flow-routing.test.ts +++ b/src/diagram/tests/flow-routing.test.ts @@ -2333,6 +2333,56 @@ describe('Flow routing', () => { }); }); + describe('UpdateCloudAndFlow - valve fraction on parallel cloud drag (adjustFlows isCloud branch)', () => { + // A parallel drag of a straight cloud flow constrains the cloud to the flow + // axis and re-places the valve via adjustFlows' isCloud branch, which + // preserves the valve's fractional position between the fixed other end and + // the moved cloud using base=min(otherEnd, cloud) + abs(fraction*d). These + // pin that formula's output (the ONLY reachable adjustFlows branch: its sole + // caller passes isCloud=true). + // Fixture: source point at x=100 (uid 1), cloud sink at x=200 (uid 3), flow + // horizontal at y=100; drag the cloud right by 20 (cloud -> x=220). + const makeParallelCase = (valveX: number) => { + const cloud = makeCloud(3, 30, 200, 100); + const flow = makeFlow(30, valveX, 100, [ + { x: 100, y: 100, attachedToUid: 1 }, + { x: 200, y: 100, attachedToUid: 3 }, + ]); + return UpdateCloudAndFlow(cloud, flow, { x: -20, y: 0 }); + }; + + it('scales an off-center valve toward the moved cloud', () => { + // fraction.x = (220-100)/(200-100) = 1.2; d.x = 175-100 = 75; + // valve.x = min(100,220) + |1.2*75| = 100 + 90 = 190. + const [newCloud, newFlow] = makeParallelCase(175); + expect(newFlow.points.length).toBe(2); + expect(newCloud.x).toBe(220); + expect(newFlow.x).toBe(190); + expect(newFlow.y).toBe(100); + }); + + it('scales a centered valve toward the moved cloud', () => { + // fraction.x = 1.2; d.x = 150-100 = 50; valve.x = 100 + |1.2*50| = 160. + const [, newFlow] = makeParallelCase(150); + expect(newFlow.x).toBe(160); + expect(newFlow.y).toBe(100); + }); + + it('leaves the valve in place for a zero-delta drag (identity)', () => { + // fraction.x = (200-100)/(200-100) = 1; d.x = 175-100 = 75; + // valve.x = 100 + |1*75| = 175 (unchanged). + const cloud = makeCloud(3, 30, 200, 100); + const flow = makeFlow(30, 175, 100, [ + { x: 100, y: 100, attachedToUid: 1 }, + { x: 200, y: 100, attachedToUid: 3 }, + ]); + const [newCloud, newFlow] = UpdateCloudAndFlow(cloud, flow, { x: 0, y: 0 }); + expect(newCloud.x).toBe(200); + expect(newFlow.x).toBe(175); + expect(newFlow.y).toBe(100); + }); + }); + describe('UpdateCloudAndFlow - degenerate flow creation', () => { // When a flow is first created, both endpoints are at the same position. // The segment is both horizontal AND vertical (zero length). diff --git a/src/diagram/tests/group-movement.test.ts b/src/diagram/tests/group-movement.test.ts index c6c9b1787..f18398e67 100644 --- a/src/diagram/tests/group-movement.test.ts +++ b/src/diagram/tests/group-movement.test.ts @@ -1316,6 +1316,95 @@ describe('Group Movement', () => { expect(middlePt.x).toBe(lastPt.x); }); }); + + describe('Sink cloud endpoint selected, attached flow not in selection', () => { + // Characterization of routeUnselectedFlows' sink-endpoint loop: the mirror + // of the "Cloud in selection" source-endpoint cases above, exercising a flow + // whose LAST point is the selected cloud. Pins the sink loop's L-shape and + // straight-through behavior before the two loops are unified. + it('should adjust flow when sink cloud moves parallel to flow direction', () => { + const stock = makeStock(1, 100, 100, [], [2]); + const cloud = makeCloud(3, 2, 200, 100); + const flow = makeFlow(2, 150, 100, [ + { x: 100 + StockWidth / 2, y: 100, attachedToUid: 1 }, + { x: 200, y: 100, attachedToUid: 3 }, + ]); + + const elements = new Map([ + [1, stock], + [2, flow], + [3, cloud], + ]); + + // Only select the sink cloud, move it right 50 (parallel to flow) + const selection = new Set([3]); + const delta = { x: -50, y: 0 }; + + const result = testApplyGroupMovement(elements, selection, delta); + + // Cloud should move right + expect((result.get(3) as CloudViewElement).x).toBe(250); + + // Flow stays 2-point straight, sink follows the cloud + const newFlow = result.get(2) as FlowViewElement; + expect(newFlow.points.length).toBe(2); + expect(newFlow.points[0].x).toBe(100 + StockWidth / 2); + expect(newFlow.points[newFlow.points.length - 1].attachedToUid).toBe(3); + expect(newFlow.points[newFlow.points.length - 1].x).toBe(250); + expect(newFlow.points[newFlow.points.length - 1].y).toBe(100); + }); + + it('should create L-shaped flow when sink cloud moves perpendicular to flow direction', () => { + const stock = makeStock(1, 100, 100, [], [2]); + const cloud = makeCloud(3, 2, 200, 100); + const flow = makeFlow(2, 150, 100, [ + { x: 100 + StockWidth / 2, y: 100, attachedToUid: 1 }, + { x: 200, y: 100, attachedToUid: 3 }, + ]); + + const elements = new Map([ + [1, stock], + [2, flow], + [3, cloud], + ]); + + // Only select the sink cloud, move it down 30 (perpendicular) + const selection = new Set([3]); + const delta = { x: 0, y: -30 }; + + const result = testApplyGroupMovement(elements, selection, delta); + + // Cloud should move down + const newCloud = result.get(3) as CloudViewElement; + expect(newCloud.x).toBe(200); + expect(newCloud.y).toBe(130); + + // Flow becomes L-shaped (3 points), staying orthogonal + const newFlow = result.get(2) as FlowViewElement; + expect(newFlow.points.length).toBe(3); + + // First point stays at the stock edge + expect(newFlow.points[0].attachedToUid).toBe(1); + expect(newFlow.points[0].x).toBe(100 + StockWidth / 2); + expect(newFlow.points[0].y).toBe(100); + + // Corner turns the L (at stock edge x, cloud's new y) + const corner = newFlow.points[1]; + expect(corner.attachedToUid).toBeUndefined(); + expect(corner.x).toBe(100 + StockWidth / 2); + expect(corner.y).toBe(130); + + // Last point at the cloud's new position + const lastPt = newFlow.points[2]; + expect(lastPt.attachedToUid).toBe(3); + expect(lastPt.x).toBe(200); + expect(lastPt.y).toBe(130); + + // Orthogonal segments: vertical then horizontal + expect(newFlow.points[0].x).toBe(corner.x); + expect(corner.y).toBe(lastPt.y); + }); + }); }); describe('Link arc adjustment during group movement', () => {