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

# Fresh ordinary saved-view selection review — 2026-09-23

Independent static correctness review of the local, uncommitted feature on
`codex/multiselect-9e8r6t-20260923`, based on
`b8685416f56e53e30bd11ecf93a54cea10ac8458`. Scope is ordinary saved views only,
against [the implementation contract](multiselect-contract-20260923.md).
State, Allocation, Process/Scenario adapters and aggregate model deletion are
known incomplete work and are not findings in this review. The pure core
movement planner has its [separate review](multiselect-core-review-20260923.md).

Reviewed the selection reducer, App adapter, canvas adapter and gesture, focus
owner, App/menu/view integration, selection and focus tests, and adjacent
selection/sync/route/navigation paths. The initial review did not edit production
or test source. The parent subsequently delegated the bounded correction below.
No test execution, builds, GUI operation, Gitea update or publication were
performed by this reviewer.

## Findings

### P2 — starting passive I/O loses Shift that was already held

`crates/oa-desktop/src/diagram/focus.rs:93–97` replaces the wrapped canvas tree
with `Tree::new` when suspension starts. This intentionally retires pointer
gestures, but also resets the newly added `State.modifiers`
(`diagram.rs:69–72`) to its default. The only normal assignment of a nonempty
modifier state is receipt of `ModifiersChanged` (`diagram.rs:615–617`), and
the toggle decision reads that retained state (`diagram/multiselect.rs:208`).
The wrapping view supplies `app.busy` as suspension (`views.rs:511–516`).

Concrete event path: select A; deliver `ModifiersChanged(SHIFT)` while a clean
Inspector field owns focus; start Save/export through a mouse action while
continuing to hold Shift; let busy become true and then false; click B without
an intervening modifier transition. The tree replacement discards Shift, so
the click follows the plain selection path and replaces A with B instead of
adding B. The same loss occurs if the canvas owned focus before I/O. No stale
model mutation is needed to trigger it.

The existing test
`mounted_suspended_modifiers_are_passive_and_survive_until_resumed_click`
(`app/diagram_focus_tests.rs:329`) sends Shift **after** setting busy and
settling, so it verifies a different ordering and cannot catch this path.

Retain passive modifier state across gesture retirement, or restore it from
an owner whose lifetime survives the canvas replacement before the first
resumed press. Keep the existing I/O pointer-retirement behavior. Add the
mounted replay with Shift delivered before suspension, no modifier events
during suspension/resumption, and assert A/B selection, unchanged model and
history, and no keyboard focus granted by modifier tracking. This finding is
established by the explicit state-reset path; runtime reproduction remains
for the owning session's affected gate.

No other new in-scope correctness finding was established.

## Rechecked protections

- `Selection::matches` combines document/surface/revision/epoch, membership,
  primary and monotonic generation. The App rejects stale operation envelopes
  before allocation and camera boundary side effects. Choose/Clear use context
  admission; movement, Hide, refusal and collapse require a captured snapshot.
- Full typed-set admission prevents silently dropping edges or ports from a
  group movement/Hide request. Exact visibility is rechecked at execution.
  Reconciliation now preserves surviving targets when the exact primary
  disappears, and does not substitute a new placement of the same semantic
  element. The Placement-properties identity correction is present in
  `legacy_target`.
- A selected-member press defers primary/Inspector changes. Its stationary
  release collapses even a mixed group; a threshold-crossing mixed drag
  refuses while retaining membership. Threshold crossing remains latched for
  cancellation, zero movement and return to origin. The latest
  `Press::viewport_matches` delegates to the underlying gesture, so its retired
  state cannot regain authority.
- Ordered pointer normalization precedes group handling. Group preview and
  release consume the same captured geometry and planner. Active groups
  consume keyboard input; Escape cancels before a subsequent Escape clears.
  Middle pan excludes group admission. Context, selection generation,
  viewport and window/pointer lifecycle changes retire the gesture.
- Context-menu opening/dismissal preserves membership; Add/Remove uses the
  captured target, and selected-member group menus expose aggregate Hide and
  explicit inspection/collapse. App guards prevent queued singleton Move,
  diagram removal or model Delete from acting on one member of a new group.
  The legacy Hide action redirects to the whole visible set.
- Ordinary passive modifiers are forwarded without acquiring keyboard focus,
  including while another mounted field owns focus. Mounted tests cover this
  primary requirement and unfocused command denial. The finding above concerns
  the separate tree-replacement lifetime.
- The earlier unconditional global reconciliation cost is removed. Boundary
  changes trigger reconciliation and projection reuses `document.view_index()`.
  No latency or scale-performance measurement is claimed here.

## Acceptance limits

The parent reports desktop-3 passed 1,254 tests with zero failures and six
ignored on its recorded 729-file input set, followed by the small
`Press::viewport_matches` change. That report was context for this review;
this reviewer did not rerun or independently certify it. The parent owns the
affected gate, core/native checker result, native interaction verification and
integration acceptance. This static review cannot establish actual Windows
focus behavior, visual clarity, save/reopen usability, or performance.

The review inspected `diagram/multiselect.rs` with SHA-256
`C8ADF20046176F620CAEF52DD8ED93EEDB349F72D8C9F50401AAA440848803B4`,
`diagram/focus.rs` with
`16AA9C6254BE7E30C67A71C0D5FE5FC763F40923912154209907D0FA17E75889`, and
`app/multiselect.rs` with
`D3CA4A2D593CB15CEDD40897BBCCF410111D9E54AD8ADFE9AD985B47611B0B0B`.
These identify local reviewed contents, not a published commit.

## Delegated correction after review

The parent authorized edits only to `diagram/focus.rs`,
`app/diagram_focus_tests.rs` and this note. The focus owner now stores passive
modifier state independently of the child canvas and marks it for restoration
when suspension replaces the child. Before the next admitted child event it
replays `ModifiersChanged` without acquiring keyboard focus. A genuine modifier
change updates that retained state and window unfocus clears it. The existing
subtree replacement, pointer retirement, suspended-input rejection and keyboard
ownership checks are preserved.

Two mounted regression tests were added in source:

- `mounted_shift_held_before_suspension_survives_without_another_modifier_event`
  holds Shift with Inspector focus before I/O, crosses busy/resume without a
  modifier transition, and verifies that the next click selects A/B without
  model/history/draft changes or passive focus acquisition.
- `mounted_shift_release_or_window_unfocus_during_suspension_does_not_restore_stale_shift`
  covers release and window-unfocus while busy, then another suspension cycle
  to prove the retained owner state was updated rather than restoring stale
  Shift. The resumed click must select B alone with unchanged model/history.

The correction and test source were statically inspected; the parent owns
compilation, execution and acceptance. The P2 above describes the pre-correction
path and remains the regression rationale, not a claim that the new source
still has that path.