ARCADIA-Studio/harness/multiselect-core-review-20260923.md

Independent simultaneous-movement core review — 2026-09-23

Scope: frozen local crates/oa-core/src/diagram_edit.rs, tests/group_movement.rs, and lib.rs export, against harness/multiselect-contract-20260923.md and base b8685416f56e53e30bd11ecf93a54cea10ac8458. This is a static correctness/design review, not a passing build, test execution, GUI or final-feature acceptance claim. No production files were edited and no Cargo command ran.

Reviewed file SHA-256 values:

• diagram_edit.rs: 7F307D64FE09947973871A05BC8D5DCA82B64B65A8E6B3AAED22D6EFC6497962
• group_movement.rs: BADEC10713B9E9961330A1A77710B6D0D75C27B482281B02BEA6868C52B8F994

Findings, in priority order

No verified blocking correctness defect in the pure geometry planner. There is one concrete scaling concern and two nonblocking coverage extensions. The latter are not claims that the implementation currently behaves incorrectly.

1. P2 — applying the emitted batch performs a full project-identity enumeration for every moved placement. diagram_edit.rs:74 emits one EditNode per placement. Its existing handler (session.rs:530–544) calls reidentify_tracked, and reidentify invokes Project::contains before the same-ID early return (session.rs:873–879). Project::contains builds the entire identities() vector (model.rs:1761–1781). Thus applying a group of M placements adds O(M × P) identity enumeration/allocation for P project records, although every placement retains its ID. This is a verified code-path cost, not a measured latency defect. It matters especially if a future preview applies the whole command on every pointer event. The planner itself computes parents once and has no repeated full-project clone. Recommended follow-up: measure representative large nested selections before choosing the preview path; if needed, skip identity remapping for an unchanged placement UUID after the handler's existing-node check, or apply validated geometry records through a preview path without full session execution. Do not widen this into unrelated transaction refactoring.

2. P3 — new-planner tests do not directly exercise projection-derived ancestry differing from semantic ancestry. Current nested coverage uses an Activity child (group_movement.rs:27, :148). projection::parents specifically replaces work ancestry with allocation grouping and makes Traceability placements flat (projection.rs:94–138). The planner correctly calls that authoritative helper, so no divergent implementation was found. A compact follow-up fixture should select an allocated performer plus a selected work descendant and assert all visually grouped work moves exactly once without changing allocations; an equivalent Traceability fixture should assert an unselected semantic child remains stationary. Existing release_routing.rs:403–494 covers portions of Allocation grouping through old MoveNode and route helpers, but not the new command planner's final whole-model result.

3. P3 — the new end-to-end preservation fixture uses default attachment/port-anchor data. The route fixture always has source_attachment: None and target_attachment: None (group_movement.rs:7–11); the typed-port case has no authored port_anchors (:221). Clone-and-change implementation preserves them by inspection, and label metadata is already exercised. Populate at least one manual edge attachment and one owner-port anchor, then retain the whole-project equality and source round-trip assertion. This would make the contract's authored attachment preservation observable rather than relying exclusively on inspection.

Correctness traced to authoritative behavior

• Admission checks the diagram, nonempty set, every exact placement membership, and finite delta before the zero-motion return (diagram_edit.rs:18–36). A foreign or missing selected placement cannot disappear into a partial move, including zero delta. Typed visual-target admission, visibility and stale document/revision checks remain adapter responsibilities; the helper intentionally receives placement IDs only.
• The closure in :38–50 traverses projection::parents with a deduplicating visited set. It is equivalent to the existing descendant_nodes ancestry relation for accepted project snapshots, includes folded placements because folding is not used to filter parents, and cannot double-translate overlapping selected roots. It correctly preserves Traceability's flat projection and Allocation's visual parenting by delegation.
• Route derivation in :51–52 uses the original view and the complete union of displacement entries. translated_routes resolves actual endpoint owner placements, including ports, requires both owners to move by equal delta, handles State Entry/Exit containing boundaries, and clones changed routes (projection.rs:169–224; projection/state.rs:119–144). One-ended routes remain exact; label offsets and attachment fractions are retained. This addresses the independent-root failure of sequential MoveNode calls.
• Each placement clone changes only x/y (diagram_edit.rs:55–78). Bounds match existing validation: finite values within MAX_COORDINATE, positive bounded dimensions (validation.rs:820–840). Translated route pins are checked before returning a batch. Partial command construction cannot mutate the immutable input or escape on error.
• EditNode with the unchanged ID retains provenance; SetEdgeRoute updates its existing entry in place, retaining unrelated route ordering (session.rs:749–755). Batch execution prepares one candidate, validates the final state, and publishes one history entry only if the candidate changed (session.rs:172–198, :250–269, :386–395). The planner's returned command-selection ID is the last changed record rather than the user's primary; its doc comment correctly warns adapters not to adopt it as primary selection.
• Desktop Document::execute performs source update before committing (document.rs:322–335), so using this existing command path preserves model/source atomicity. The planner does not itself enforce source-draft or focus guards and should not be treated as doing so. Existing Document::commit suppresses unchanged source bundles (:399–406); the new zero-motion test directly proves only the core Session contract, not a desktop gesture/source-draft integration.

Test review and evidence limits

The main sibling test (group_movement.rs:71) independently assigns three exact expected positions and the internal route pin, then compares the entire Project. It proves intent independently of translate_placements, rather than comparing a preview and commit that share the same bug. It also includes a useful negative control showing sequential old moves retain the original internal pin, one Undo/Redo, JSON round-trip, and source-update/recompile preservation with a comment. The typed-port test independently constructs its complete expected Project (:221).

The folded-overlap test partly compares two planner outputs, but also independently asserts the hidden child's exact position and unchanged routes (:148). The invalid-input test covers unknown diagram, empty/mixed/missing membership, nonfinite/overflow deltas, translated route overflow, no mutation, and zero history (:183). The State boundary test checks independent expected route coordinates when the boundary moves and unchanged pins when it does not (:269); it is intentionally narrower than a whole-project comparison.

The input immutability assertions are inherently weak by themselves because the helper takes &Project; their value is secondary to the complete expected-result assertions. No test-run result was inferred from reading test source. No GUI adapter, mixed target-set admission, allocation reassignment, Process/Scenario reorder, stale input, preview latency, or full feature completion was reviewed or accepted here.

Overall: the frozen foundation implements the selected pure-geometry contract coherently. Retain the pending performance check and targeted coverage extensions while proceeding to adapter work; neither a successful compile nor this review alone establishes interaction acceptance.