Flow-editing follow-ups: Z-shape offset, attach-failure UX, serializer unification, dead-code removal

src/core/tests/datamodel.test.ts

@@ -932,6 +932,126 @@ describe('StockFlowView', () => {
   });
 });
 
+// These pin the fields the (now-deleted) diagram-local stockFlowViewToJson copy
+// silently dropped, so unifying every diagram caller onto this implementation
+// cannot regress them (issue #821, an #811-class field-drop cluster).
+describe('StockFlowView serialization completeness (#821)', () => {
+  it('preserves link polarity through a round-trip', () => {
+    for (const polarity of ['+', '-'] as const) {
+      const link: LinkViewElement = {
+        type: 'link',
+        uid: 4,
+        fromUid: 1,
+        toUid: 2,
+        arc: 30,
+        isStraight: false,
+        multiPoint: undefined,
+        polarity,
+        x: NaN,
+        y: NaN,
+        isZeroRadius: false,
+        ident: undefined,
+      };
+      const json = stockFlowViewToJson({
+        nextUid: 5,
+        elements: [link],
+        viewBox: { x: 0, y: 0, width: 0, height: 0 },
+        zoom: -1,
+        useLetteredPolarity: false,
+      });
+      const linkJson = json.elements[0] as JsonLinkViewElement;
+      expect(linkJson.polarity).toBe(polarity);
+      expect(linkViewElementFromJson(linkJson).polarity).toBe(polarity);
+    }
+  });
+
+  it('treats arc and multiPoint as mutually exclusive (arc wins, matching fromJson)', () => {
+    const link: LinkViewElement = {
+      type: 'link',
+      uid: 4,
+      fromUid: 1,
+      toUid: 2,
+      arc: 45,
+      isStraight: false,
+      multiPoint: [{ x: 1, y: 2, attachedToUid: undefined }],
+      polarity: undefined,
+      x: NaN,
+      y: NaN,
+      isZeroRadius: false,
+      ident: undefined,
+    };
+    const json = stockFlowViewToJson({
+      nextUid: 5,
+      elements: [link],
+      viewBox: { x: 0, y: 0, width: 0, height: 0 },
+      zoom: -1,
+      useLetteredPolarity: false,
+    });
+    const linkJson = json.elements[0] as JsonLinkViewElement;
+    expect(linkJson.arc).toBe(45);
+    expect(linkJson.multiPoints).toBeUndefined();
+  });
+
+  it('preserves useLetteredPolarity when set', () => {
+    const json = stockFlowViewToJson({
+      nextUid: 1,
+      elements: [],
+      viewBox: { x: 0, y: 0, width: 0, height: 0 },
+      zoom: -1,
+      useLetteredPolarity: true,
+    });
+    expect(json.useLetteredPolarity).toBe(true);
+    const restored = stockFlowViewFromJson(json, new Map());
+    expect(restored.useLetteredPolarity).toBe(true);
+  });
+
+  it('keeps a viewBox with a single non-zero dimension (OR, not AND)', () => {
+    const json = stockFlowViewToJson({
+      nextUid: 1,
+      elements: [],
+      viewBox: { x: 0, y: 0, width: 800, height: 0 },
+      zoom: -1,
+      useLetteredPolarity: false,
+    });
+    expect(json.viewBox).toEqual({ x: 0, y: 0, width: 800, height: 0 });
+  });
+
+  it('round-trips a Z-shape flow whose corner points are unattached (#819)', () => {
+    const flow: FlowViewElement = {
+      type: 'flow',
+      uid: 10,
+      name: 'Births',
+      ident: 'births',
+      var: undefined,
+      x: 50,
+      y: 200,
+      labelSide: 'center',
+      points: [
+        { x: 0, y: 200, attachedToUid: undefined },
+        { x: 50, y: 200, attachedToUid: undefined },
+        { x: 50, y: 100, attachedToUid: undefined },
+        { x: 100, y: 100, attachedToUid: 1 },
+      ],
+      isZeroRadius: false,
+    };
+    const json = stockFlowViewToJson({
+      nextUid: 11,
+      elements: [flow],
+      viewBox: { x: 0, y: 0, width: 0, height: 0 },
+      zoom: -1,
+      useLetteredPolarity: false,
+    });
+    const flowJson = json.elements[0] as JsonFlowViewElement;
+    // Unattached corners omit attachedToUid entirely; the attached corner keeps it.
+    expect(flowJson.points.map((p) => p.attachedToUid)).toEqual([undefined, undefined, undefined, 1]);
+    for (const p of flowJson.points.slice(0, 3)) {
+      expect('attachedToUid' in p).toBe(false);
+    }
+    const restored = flowViewElementFromJson(flowJson);
+    expect(restored.points.map((p) => p.attachedToUid)).toEqual([undefined, undefined, undefined, 1]);
+  });
+});
+
 describe('canBeModuleInput and isPublic', () => {
   it('should default canBeModuleInput and isPublic to false for Stock', () => {
     const json = { name: 'pop', inflows: [], outflows: [] };

src/diagram/CLAUDE.md

@@ -20,7 +20,6 @@ For build/test/lint commands, see [docs/dev/commands.md](/docs/dev/commands.md).
 - `group-movement.ts` -- Group manipulation and movement logic
 - `flow-attach.ts` -- Pure functional core for flow attach/reattach/create (`computeFlowAttachment`): the source/sink endpoint and `updateStockFlows` op builders extracted from `Editor.handleFlowAttach`. Returns the new elements, model ops, and selection; the Editor shell applies them.
 - `selection-logic.ts` -- Selection-set arithmetic (mouse-down/up selection decisions, deferred-single-select, reattachment override, pointer-state reset). The pure slice the Canvas interaction model composes.
-- `view-conversion.ts` -- View coordinate conversions
 - `arc-utils.ts` -- Arc geometry helpers (`radToDeg`, `degToRad`, arc math)
 - `keyboard-shortcuts.ts` -- Keyboard shortcut handling
 - `StaticDiagram.tsx` -- Static (non-interactive) diagram renderer

src/diagram/Editor.tsx

@@ -15,7 +15,6 @@ import { canonicalize } from '@simlin/core/canonicalize';
 
 import { Project as EngineProject } from '@simlin/engine';
 import type { JsonProjectPatch, JsonModelOperation, JsonSimSpecs, JsonArrayedEquation } from '@simlin/engine';
-import { stockFlowViewToJson } from './view-conversion';
 import {
   Project,
   Model,
@@ -37,6 +36,7 @@ import {
   auxToJson,
   moduleToJson,
   arrayedEquationToJson,
+  stockFlowViewToJson,
   type ModuleReference,
 } from '@simlin/core/datamodel';
 import { defined, exists, setsEqual } from '@simlin/core/common';
@@ -937,12 +937,14 @@ export const Editor = React.memo(function Editor(props: EditorProps): React.Reac
         const patch: JsonProjectPatch = {
           models: [{ name: modelName(), ops: [...result.ops] }],
         };
-        // On patch failure, commit the selection/creation flag but DO NOT
-        // update the view -- preserving the original behavior exactly.
-        if (!(await applyPatchOrReportError(patch, 'flow attach'))) {
-          setState({ selection, flowStillBeingCreated: inCreation });
-          return;
-        }
+        // Apply the model-level ops but do NOT bail on failure: the drawn flow
+        // must still be committed to the view (issue #820). The controller
+        // reports any failure via onError (the toast); the view update below
+        // runs regardless, matching handleCreateVariable/handleSelectionDelete.
+        // Discarding it here previously both lost the user's work AND left the
+        // just-created-flow name edit selecting a flow that was never committed
+        // to the view -- the handleEditingNameDone getElementByUid crash.
+        await applyPatchOrReportError(patch, 'flow attach');
       }
 
       await updateView({ ...view, nextUid: result.nextUid, elements: [...result.elements] }, { recordHistory: true });

src/diagram/LAYOUT.md

@@ -444,7 +444,7 @@ Flows connect stocks and clouds with specific constraints:
    - Maintain axis alignment
    - Constrain valve position within valid range
 3. Update cloud positions to follow flow movement
-4. Keep minimum 20px clearance from flow endpoints
+4. Keep minimum 10px clearance from flow endpoints (`VALVE_CLAMP_MARGIN`)
 
 ### Connector Rules
 
@@ -652,7 +652,7 @@ Each element type has specific hit testing:
 1. **Spacing**: Minimum 20px between element edges
 2. **Grid alignment**: Snap to 5px or 10px grid for cleaner layouts
 3. **Label clearance**: Account for label bounds when positioning
-4. **Flow clearance**: Keep valve at least 20px from connected elements
+4. **Flow clearance**: The valve clamps at least 10px (`VALVE_CLAMP_MARGIN`) from flow endpoints
 
 ### Layout Strategies
 1. **Hierarchical**: Arrange in levels (sources → stocks → sinks)

src/diagram/drawing/Canvas.tsx

@@ -2059,8 +2059,20 @@ export const Canvas = React.memo(function Canvas(props: CanvasProps): React.Reac
       return;
     }
 
-    const uid = only(latest.current.props.selection);
-    const element = getElementByUid(uid);
+    // Resolve the element being named through the NON-throwing lookup, mirroring
+    // the render-time guard (see the editingElement resolution). A failed
+    // flow-attach can leave the selection referencing a flow that was never
+    // committed to the view (issue #820); dereferencing it with the throwing
+    // getElementByUid wedged the editor in a repeated-exception loop. When the
+    // selection is not a single resolvable element there is nothing to commit --
+    // tear the editor down cleanly instead of crashing.
+    const selection = latest.current.props.selection;
+    const uid = selection.size === 1 ? only(selection) : undefined;
+    const element = uid !== undefined ? tryGetElementByUid(uid) : undefined;
+    if (uid === undefined || element === undefined) {
+      clearPointerState();
+      return;
+    }
     const oldName = displayName(defined((element as NamedViewElement).name));
 
     if (uid === inCreationUid) {