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. 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.

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.