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/Editor.tsx b/src/diagram/Editor.tsx index 1313ca961..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; } }, []); @@ -658,7 +682,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 => { @@ -684,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)) { @@ -2016,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 ( @@ -2079,44 +2123,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/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) } ``` diff --git a/src/diagram/drawing/Canvas.tsx b/src/diagram/drawing/Canvas.tsx index fb35ce34a..dda349ea2 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, @@ -52,6 +54,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'; @@ -86,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', @@ -890,6 +894,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, @@ -2019,13 +2050,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/Flow.tsx b/src/diagram/drawing/Flow.tsx index 81620d102..32246baed 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; @@ -478,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, @@ -500,6 +514,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) => { @@ -529,7 +550,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 = { @@ -601,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), @@ -645,6 +684,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[], @@ -713,9 +783,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; @@ -772,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, @@ -1189,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. * @@ -1396,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, @@ -1505,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/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/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 ba735fe07..78c754501 100644 --- a/src/diagram/flow-attach.ts +++ b/src/diagram/flow-attach.ts @@ -42,19 +42,18 @@ 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. 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 @@ -341,28 +340,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, @@ -388,6 +395,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; @@ -460,3 +477,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/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/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/canvas-gestures-elements.test.tsx b/src/diagram/tests/canvas-gestures-elements.test.tsx index 479eb1f9b..954b6f566 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); @@ -260,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]; @@ -348,6 +386,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); @@ -410,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(); + }); +}); 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); + }); +}); diff --git a/src/diagram/tests/flow-attach.test.ts b/src/diagram/tests/flow-attach.test.ts index b2c0c6bad..08a8fdb5e 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, @@ -26,6 +27,12 @@ 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) ----- @@ -149,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', () => { @@ -422,13 +434,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 +454,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 +478,110 @@ 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('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: 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, [ @@ -718,3 +850,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); + }); +}); diff --git a/src/diagram/tests/flow-routing.test.ts b/src/diagram/tests/flow-routing.test.ts index cd908edb1..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). @@ -2740,4 +2790,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..f18398e67 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 @@ -1311,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', () => { 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