ARCADIA-Studio/harness/experiments/state-label-layout-20260924/source-review.md

#36 implementation review at 2af40c7

Read-only source review of 2af40c7e27ed9750b9585c07369dbf17a046a92d in .local/state-label-layout-20260924, confirmed clean after writers settled. This is a preliminary implementation review, not final acceptance. No independent compilation, GUI or runtime tests; parent owns execution. I authored the separate State canvas regression include earlier, so review of those tests is a consistency check, not independent authorship review.

Follow-up status: the P2 below is closed at source-review level by 1f471288284102f46f67075765568dbf5856474b, inspected in clean .local/state-label-hit-review-20260924 with exact parent 2af40c7e27ed9750b9585c07369dbf17a046a92d. Integration and execution remain parent-owned; no native acceptance is claimed. No new actionable finding in this bounded follow-up.

Actionable finding

P2 — Generic diagram surface paints State labels above nodes but still selects the covered node. This is a reachable surface: views.rs:403 includes dedicated State diagrams in the saved-view picker (only Scenario is excluded), constructs DiagramCanvas at470, and app.rs:3697 sends non-Scenario choices to Page::Diagram. The new generic painter defers State label rectangles/text until after nodes (diagram.rs:1491), matching their visible foreground position. However diagram.rs:331 context targeting, diagram.rs:999 ordinary press, and diagram/multiselect.rs:206 production group/Shift targeting still resolve nodes before labels. Additionally DiagramCanvas::hit_label at428 chooses nearest center rather than the last painted overlapping State label. The same authored label over a State therefore looks selectable but returns the obscured placement in this supported view; two overlapping State labels can choose the obscured one.

Use one dedicated-State-plan background hit helper in the relevant generic target paths, before ordinary node targets but after existing explicit selected-handle decisions, with reverse actual label paint order. Keep non-State label/node priority unchanged. Cover generic ordinary click, context target and Shift target at a fixed label-over-node point; cover overlapping State labels with the lower label selected. The existing generic detached_label_selects_connection_and_occluding_card_owns_its_hit test describes non-State behavior and should continue to pass. Alternatively explicitly remove/restrict this supported surface, but that would be a larger product change than the local target fix.

Repair constraint: an unconditional non-Shift handle return None in generic group_input would bypass mixed-selection refusal: the ordinary route-edit path has no StateCanvas-style singleton gate. Instead resolve a dedicated-State selected attachment/bend to Target::Edge(selected) before the State-label prepass, then use existing Shift/group admission. The existing singleton/nonmember fallthrough already reaches ordinary handle editing where allowed. A selected Edge+Placement group must remain captured/refused, and Shift on a handle must toggle that handle's exact Edge even when another label overlaps it. This challenges the proposed follow-up design; it is not an additional defect introduced in the reviewed commit.

Reviewed coherent behavior

• Core builds StateLabelLayout only for a StateTransition in a dedicated State projection. The raw global48 summary remains EdgeLabelGeometry.text; fixed title budget is computed from that raw excerpt before width measurement. LF/CRLF/CR and declared Unicode separators become explicit cells; blank/trailing cells survive; tab expansion is display-only. Title and summary positions share 12-unit cells, minimum24 height and five-unit horizontal padding.
• state_label::measure_line matches installed iced_graphics-0.14.0/src/geometry/text.rs::Text::draw_with: same Paragraph implementation, global font system, font/shaping/alignment defaults, font10, absolute12 and infinite bounds. Helper wrappingNone versus painter default wrapping cannot introduce width wrapping at infinite width for these already split lines. Paragraph advance plus padding still needs the declared native ink-containment checks; no universal Unicode claim follows from the source review.
• Production desktop routing consumers found by source search all call the measured wrapper, including ordinary/preview scene, Fit, State Fit, usage/focus, State bend setup, generic export and work/allocation geometry. Remaining direct core routing calls are tests. Measurement occurs before collision placement; core remains font-independent and derived plans are not serialized.
• Dedicated State hit order matches its opaque backgrounds and reverse stable edge paint order. Endpoint/bend checks and group admission remain at their existing input sites. Raw summary prefix, not normalized display lines, feeds typed-reference visibility, preventing CRLF byte-offset corruption. The State projection admits only non-Internal StateTransition edges, so the painter's plan-only branch does not suppress another supported label kind there.
• Settled SVG now emits the planned escaped lines and opaque backgrounds after nodes; the actual export test exercises this ordering, beyond the isolated string helper test. Non-State SVG label placement is preserved.
• The diff contains no semantic model/schema/serialization change and does not author node sizes, route fraction/offset or waypoints. Automatic label centers may change from corrected bounds as intended. Existing scene caching still avoids recomputation on unchanged hover/selection; shaping adds bounded work per retained line when routing actually occurs. No concrete performance blocker found in this bounded static review.

Test assumptions and remaining evidence

The new State canvas fixed-point fixture is defensible from source, rather than adjusted to returned geometry: B at(760,180),224x94 has Bottom0.5 attachment(872,274); the shape inset is zero at the side midpoint. Fraction0 and offset(-432,-14) give center(440,260). A's single entry row occupies y244..270. Thus (440,260) exercises covered row ownership, (440,272) the summary over the State header, and (200,260) the exposed row. Moving both B/C by(0,320) moves the anchor/label center by the same amount with the original authored offset; no waypoints require translation. The assertions deliberately fail if these declared assumptions cease to hold.

Those tests cover synthetic dedicated-canvas interactions and cancelled group preview; they do not establish native glyph containment, generic-canvas targeting, actual peer-fixture pixels, settled-move equality, or save/reopen/Undo. Parent's focused/full tests and native acceptance still need to supply their assigned evidence. The transient syntax/duplicate-tail states observed while writers were active were corrected before this pinned review and are not open findings.

Generic hit repair follow-up at 1f471288

Inspected the exact three-file commit diff and all230 lines of its new diagram/state_label_input_tests.rs; no source writes, compiler or product execution. hit_state_label now resolves only actually painted, nonempty State-plan rectangles in reverse stable paint order. Generic context targeting and ordinary click consult it before ports/nodes, while selected attachment/bend controls remain earlier. Selected label dragging requires that selected label to be the frontmost one; the lower selected label cannot steal the upper label's click. The existing nearest-center/non-State path stays intact.

Generic group input resolves a selected State attachment/bend to the exact Edge target before the label target and then passes through the existing Shift, singleton/nonmember, editability and mixed-group checks. It does not introduce the previously challenged unconditional handle fallthrough. For a selected Edge+Placement set, a handle press creates the existing unsupported group lifecycle and threshold release refuses; a singleton still falls through to the ordinary handle edit. Shift on the overlapped selected handle toggles that handle's Edge, while Shift away from handles toggles the front label. No global edge-first policy or non-State ordering change was introduced.

The added tests exercise the real generic Program path at three zooms/nonzero pan, plain/context selection above a node, lower-selected overlap, front-label drag, read-only selection, Shift label-versus-handle identity, selected bend continuation and mixed refusal. At half zoom the pointer is deliberately17 world units below the central bend, beyond its16-unit radius and still within the18-unit half-height; this is a valid independent separation of label and handle hit regions. Fixture setup uses routing only to author an offset placing both labels at the separately declared(200,150) target, not to choose the expected winning Edge. These tests remain unexecuted by this reviewer. Existing attachment tests still need their scheduled run; the new focused handle regression directly covers a bend rather than an attachment.