ARCADIA-Studio/harness/scenario-selection-evidence-20260924/reviews/source-review.md

Scenario selection adapter source review

Reviewed local commit 879eb1d5c8792a720433022e49f415d2c58ff72c against 47917dd, 2026-09-24. Scope: exact selection adapter, App admission/reconciliation, singleton spacing bridge, Scene input/paint and contextual menus. Read AGENTS.md, the multiple-selection contract, Scenario group policy, prior adapter critique, changed implementation and new tests. No Cargo, GUI, network or repository mutation. This review does not establish runtime acceptance or completion of issue 29. Group movement and aggregate removal are explicitly unfinished.

Findings

P2 — Clearing selection aborts the blank press's newly started pan

crates/oa-desktop/src/behavior.rs:2262-2268 publishes Clear without capturing the press, explicitly to share it with background panning. The containing behavior/canvas_workspace/viewport.rs:387-399 consequently starts state.pan and publishes Camera::CancelGesture. For a nonempty selection, app/scenario_multiselect.rs:114-125 advances both canvas_epoch and camera_epoch. The subsequent Viewport diff observes that changed camera scope and clears the newly started pan (viewport.rs:93-99; Scenario scope uses camera_epoch at behavior/scenario/workspace.rs:123). The camera packet emitted by that same press also carries the previous camera lifetime.

Concrete falsifier: in either Scenario representation, select a visible occurrence, then press blank canvas and drag without releasing. Selection should clear and the existing blank-pan gesture should move the camera. Source predicts selection clears but the first drag cannot continue after redraw; a second blank drag with already empty selection works. Existing Scene-only tests omit the parent Viewport/App/rediff composition and cannot establish this interaction.

Fix: retire stale selection/spacing captures without retiring the camera gesture legitimately initiated by that same background press, or admit clear-and-begin-pan as one coordinated operation. Do not merely capture Clear and remove background panning. Add a composed input regression that includes publication ordering and widget diff, then verify the native gesture.

P2 — Epoch retirement covers new selection events, not legacy singleton reconciliation

The new spacing bridge relies on generation changes advancing canvas_epoch (app/scenario_spacing.rs:24-26), but that happens only in scenario_selection_event (app/scenario_multiselect.rs:114-125). reconcile_scenario_multiselect can independently replace or clear membership (:35-45) without retirement. The deliberately retained singleton Scene path can emit legacy CanvasEvent::Select; App accepts it at app.rs:2444-2451 and select changes a same-owner selection without incrementing epochs (:1168-1219). The outer wrapper advances canvas_epoch on a selection change only if an active spacing gesture is actually canceled (:1631-1636); camera_boundary tracks the owner, not exact selected member (:1709-1723). Thus with no active spacing, same-owner singleton A-to-B-to-A changes selection generation but can preserve the captured spacing Context and camera_epoch.

Concrete App falsifier: start with saved-view singleton A; capture but do not deliver a valid spacing Begin. Deliver same-context legacy CanvasEvent::Select(B), then Select(A), with no active spacing and no model edits. Check that generation changed while Context/camera_epoch did not. Deliver the old Begin: scenario_spacing.rs:20-35 admits it because the live set is again singleton and no spacing token is active; its matching Finish can commit. Nonspacing Scenario boxes preserve an actual legacy selection route, so testing only Add/Remove through the new handler is not exhaustive. The new secondary-membership ABA test covers its new event path correctly but misses this path.

Fix: bind spacing Begin to the captured complete selection Snapshot, or make retirement cover every Scenario selection generation change, including legacy reconciliation. Preserve first-press singleton drag behavior while doing so. Add the legacy Select round-trip evaluator and verify that rejecting the old Begin leaves a newer live gesture, model, source, history and camera untouched. This is a remaining bridge/admission hole; the underlying legacy Select path predates this increment.

P3 — Ordinary workspace still labels only the primary, without the group's count

behavior/scenario/workspace.rs:348 still displays only Selected: <primary name>. Group count appears in the contextual menu, but no count was added to the normal Scenario workspace despite the contract's visible-count requirement (harness/multiselect-contract-20260923.md:34-35). Teal membership paint alone does not distinguish a two-item selection from a larger/off-screen group while property edits address only the primary.

Fix: show a compact visible count alongside the primary inspector label whenever the current Scenario snapshot contains a group. Native verification should include one selected item outside the viewport.

Bounded positive findings

The Scenario-specific envelope uses full representation/visit Context plus complete shared Snapshot; it rejects stale selection packets before shared navigation retirement, and bare generic selection packets are refused while Scenario is active. The representation-specific shared Scope is compatible with existing infrastructure. Actual presented targets, primary handling, passive Shift tracking, deferred group collapse, and whole-group refusal keep selection distinct from movement eligibility.

Menu Open validates before shared side effects, refreshes its context after epoch advancement, and captures selection. Invoke validates token/context/snapshot before dispatch; selection actions avoid ordinary implicit subject selection. No material menu-specific defect was found in this source review.

Preserving the original first-press singleton spacing/reorder path is a reasonable compatibility choice: unselected singleton hits retain their existing gesture, while an unselected hit replaces an already displayed group and coordinate-unavailable fallback remains selectable. Saved body/header spacing keeps precedence, and groups cannot silently apply spacing/reorder/removal to only their primary. That choice needs the bridge fix above rather than being reversed wholesale.

The new tests exercise membership/primary, representation round trips, stale new-event ABA, unsupported/bare packets, group refusal, secondary ABA spacing, pending edits, deferred collapse, threshold/refusal and menu lifetimes. I inspected their fixtures and assertions; I did not execute them. No source/history mutation was found in an admitted selection-only action. Complete native focus, first-press singleton drag, background camera behavior, selection visibility, and later group-operation preview remain separate acceptance work.

Delta review — local corrective changes after 879eb1d

Read the current uncommitted corrective source on 2026-09-24, without Git commands, Cargo or GUI execution. All three original findings are addressed at source level; no new material lifetime regression was found in this bounded delta. The findings above describe the reviewed commit, not the corrected working tree.

• Blank pan: the new selection handler now retires canvas_epoch only (app/scenario_multiselect.rs:129-132), leaving camera_epoch and camera_boundary_epoch unchanged. Existing Camera admission deliberately permits an older canvas epoch within the same camera lifetime. Therefore the Clear and parent CancelGesture emitted by one blank press remain compatible, while the Viewport no longer resets its pan merely because selection changed. Canvas-bound spacing and selection packets still become stale. This is the right separation of input lifetimes; native/composed verification remains necessary.
• Legacy selection ABA: reconciliation now captures a Snapshot against the already-current Context before changing membership and compares it with the resulting Snapshot (app/scenario_multiselect.rs:24,49-55). A changed existing selection cancels spacing and advances canvas_epoch. Because both snapshots use the same current Context, a changed revision/epoch alone does not repeatedly trigger retirement. An unchanged reconcile remains a no-op. The before-is-Some condition excludes initialization/scope changes; those lack an older matching selection scope or already change representation/visit, so no additional spacing-admission hole was identified there. The new legacy_singleton_selection_aba_retires_old_spacing_begin test covers both CanvasEvent::Select and Message::Select, checks restored exact singleton membership and changed Context, requires old Begin to be inert, then verifies a fresh Begin is accepted. The test is a suitable discriminator for the reported hole; I have not executed it.
• Count: the existing navigation row now always displays the exact snapshot count (behavior/scenario/workspace.rs:201-205), independently of opening Details. Details additionally distinguishes the primary properties target from group count (:355-358), and Select tooltip copy no longer promises currently unavailable group movement. This resolves the source-level omission; clipping/readability and offscreen-member comprehension still require visual evidence.

The corrective retirement remains compatible with the retained first-press singleton route: it happens when legacy selection actually changes, or after spacing Finish has already consumed its gesture, not automatically on a valid spacing Begin. Early exact packet admission and menu validation remain unchanged. No claim is made here about the separately owned mounted tests, their execution, native usability, operation preview, group movement/removal or issue 29 completion.

Delta review — retirement moved to the outer action boundary

The parent reports that the subsequent selection-scenario-2 run at the a0a source had 266 passing tests, two failures and one ignored test. It reports all three new mounted regressions, the corrected deletion guard fixture and legacy ABA passing. One failure was an obsolete context-retention expectation; the other demonstrated a real regression: the prior unconditional reconciliation retirement invalidated newly prepared RevealInDiagram work. These execution results are parent-reported, not independently rerun here. The earlier source-level assessment above missed this reveal-producing action path and is superseded for that fix.

Read the newest uncommitted corrective source immediately after that report. reconcile_scenario_multiselect again performs reconciliation only. App::update captures the full Scenario Context and Snapshot before the ordinary action (app.rs:1382-1384), then after reconciliation retires spacing and advances canvas_epoch only if the full Context is still identical and the Snapshot changed (:1650-1659). No new material defect was found in this bounded source delta.

This condition fits both relevant call paths. A legacy same-owner Select with no other retirement retains its original Context, changes the Snapshot and therefore receives the necessary retirement. An action that already changes document/revision/representation/visit/epoch invalidates older spacing by that Context change; it does not need another epoch bump merely because it also reconciles membership. In particular, diagram usage reveal prepares its new canvas_epoch before recording pending_diagram_reveal (app/diagram_usages.rs:456-457,491-499), so the changed Context now prevents the extra bump that stranded its viewport receipt. New exact selection events return through their own admission/retirement handler, and spacing Finish likewise owns its existing retirement; those early paths do not depend on this new tail condition. BehaviorControl delegates admitted actions through update; its existing camera/focus retirement remains separate.

The tail bump still leaves the camera lifetime intact for a same-context selection change, preserving the blank-pan correction. Scope initialization/navigation does not acquire an additional artificial retirement. This source reasoning supports the revised location but is not a passing result for the new pending fix. Re-run the revealed failing case and the legacy ABA/new mounted input cases on the exact corrected source; retain the failing a0 evidence as a regression finding, not acceptance. Native usability and group operation preview/movement/removal remain outside this review.

Final bounded source delta — ca1f18e to ed55b17

Inspected the exact read-only diff from ca1f18e to ed55b1781d86ec49af5d2588cbd06f7b878d9009: one file, crates/oa-desktop/src/behavior.rs, three insertions and six deletions. The change replaces self.selection.is_some() followed by repeated as_ref().expect(...) / as_ref()? with one if let Some((selection_context, snapshot)) = &self.selection binding. Both ScenarioGroupPress and blank Clear still use the selection envelope's Context and Snapshot, rather than the separate interaction Context. The None path, hit testing, modifiers, capture status, movement threshold and spacing/reorder fallthrough are unchanged. No behavioral or lifetime defect was found in this bounded source delta.

The parent reports that frozen ca1f18e passed 268 Scenario tests with zero failures and one ignored, plus 14 model_menu tests, against 809 unchanged proven inputs; strict lint then found only the unnecessary unwrap addressed by this commit. I did not independently inspect or execute those gate records. Final lint/format/build/Scenario/menu runs on ed55b17 are still pending at the time of this review. This delta review does not transfer ca1 runtime evidence to an unexecuted final gate or establish native/group-operation acceptance.