Picker Escape failure source review

Scope: independent source-only review of frozen candidate 8ddfadf440b5c208b1adb6b211a97ba0c99cff04, branch codex/prerelease-integrated-956atf-20260924. Candidate working tree was clean when inspected. The reviewer performed no source edits, builds, tests, fetches, GUI launches, or Gitea writes. Parent owns integration; Codex-ytr83r authored the separate peer fix. The parent read this report, fetched and reviewed the exact fix, and incorporated it in 715036e132677dd2bc2fb37ed316d93dc9987aec; that successor adds only two harness records over the peer fix. Direct Rustfmt and whitespace checks pass. This report establishes source review, not runtime acceptance.

Finding: stale fixture coordinates target the selection strip

High-confidence source diagnosis of the reported assertion, not a runtime confirmation:

crates/oa-desktop/src/app/state_picker_escape_tests.rs:51-54 assumes the missing-element picker is immediately above the measured canvas with one 8px gap. It clicks (canvas.center_x(), canvas.y - 24) and later tries the option at canvas.y + 8.
The actual composition in crates/oa-desktop/src/behavior/state_view.rs:189-200 is missing_controls, then selection_controls(app), then the focused canvas, with 8px gaps.
missing_controls has fixed height 34 (state_view.rs:151); selection_controls has fixed height 56 (state_view.rs:248). Thus its strip occupies approximately [canvas.y - 64, canvas.y - 8]. The test's first click at canvas.y - 24 is in that strip, outside the picker. The picker slot instead begins at canvas.y - 106; its 12px text / 8px padding gives approximately 31.6px actual height.
The later option target is stale as well: for a downward-opening one-option menu, the option occupies approximately [canvas.y - 74.4, canvas.y - 42.8], not the canvas at canvas.y + 8. Fixing only the opening click will expose another failure at the placement assertion.
Local history reports 2a3b9ba8 Add exact State multiselection, atomic movement and placement Hide as the last change to state_view.rs, consistent with the composed candidate gaining an intervening selection strip.

The failing assertion (state_picker_escape_tests.rs:71-74) checks messages after Escape, but there is no positive precondition proving the opening click created an overlay. The failure therefore does not establish an open-picker ownership defect.

Why the reported message failure follows from that targeting error

Mounted::click sends cursor move, left press, and left release through the actual App UI (crates/oa-desktop/src/behavior/canvas_workspace/tests.rs:286-300). press_escape then delivers a separate keyboard batch with unavailable cursor and forwards the resulting event status through the real subscriptions (state_picker_escape_tests.rs:6-19). The unavailable cursor is not itself a problem: the picker Escape handler does not require a hovered pointer.

The picker opens only when the press cursor is inside its actual layout bounds (crates/vendor/iced_widget/src/pick_list.rs:476-490). With no open picker, an uncaptured Escape reaches the App subscription, which explicitly returns Message::Escape unless status is Captured (crates/oa-desktop/src/app.rs:1316-1321). That message is sufficient to fail .is_empty(). This is a predicted path from source; the failed run did not print its message vector, so its exact contents remain unverified.

Ownership source inspection

The inspected production paths appear consistent with the intended ownership for an actually opened picker:

pick_list.rs:719-736 creates the menu only while the shared open flag is true and passes that flag plus optional close message into close_on_escape.
crates/vendor/iced_widget/src/overlay/menu.rs:299-321 ignores stale dismissed overlays; on Escape it atomically clears the open flag, publishes only an optional close notification, captures the event, requests redraw, and returns. The State missing-element picker configures no on_close notification (state_view.rs:158-170), so its dismissal should produce no App message.
pick_list.rs:452-462 also handles open-then-Escape within a base-event batch, clearing the flag and capturing the event.
crates/oa-desktop/src/app/escape.rs:112-136 preflights current overlays for Escape and returns on capture before descending into the base tree; escape.rs:179-193 forwards overlay creation. The root is installed at app.rs:1255.
The diagram focus wrapper forwards its overlay (crates/oa-desktop/src/diagram/focus.rs:303-317), and the raw-input observer forwards overlay updates using the same shell (crates/oa-desktop/src/source_navigation/interaction.rs, ObservedOverlay::update). No dropped capture or missing overlay forwarding was found in these inspected paths.

This does not prove the complete runtime behavior. It means no production ownership change is justified by this assertion until the fixture positively opens the intended control.

Minimal fix and discriminating verification

Correct both opening and option targeting. A narrow diagnostic adjustment subtracts the added 64px (56px selection strip plus an additional 8px gap) from both old points: opening canvas.y - 88, option canvas.y - 56. Those are interior points under the current layout. Prefer durable measured geometry for the picker slot/overlay (for example, a stable identified container exposing actual picker bounds through the existing Geometry operation) instead of another unexplained magic offset.
Add a precondition proving the intended overlay exists before Escape; geometry or an observable overlay operation is stronger than an empty click-message vector, because a click on inert content also produces no message. If instrumentation is needed to diagnose, capture click/Escape statuses and actual messages.
Re-run the exact failing mounted test on the integrated Windows candidate after the peer fix. Retain its exact snapshot/bundle/revision/history, reopen/place, and Undo assertions. Do not weaken the empty-message assertion merely to make the stale-target fixture pass.
Recheck existing ownership tests in crates/oa-desktop/src/app/escape/tests.rs: picker_overlay_escape_closes_once_reopens_selects_and_retains_outside_click (line162), initially_closed_picker_handles_open_then_escape_in_one_runtime_batch (line259), dismissed_picker_ignores_a_queued_click_in_its_old_option_area (line287), and picker_overlay_precedes_focused_input_and_creation_source_drafts (line336), plus the desktop gate as appropriate.
If a positively confirmed open overlay still produces an App message, distinguish status: Ignored implicates overlay discovery/cache or event forwarding; Captured plus Message::Escape implicates a message published through base-tree or subscription ownership. The current evidence reaches neither conclusion.

Follow-up review: peer fix `a9cd364eac`

Read the fetched commit directly, comparing its full name-only delta and the two-file diff against 8ddfadf440b5c208b1adb6b211a97ba0c99cff04. No tests or builds run by this reviewer. Assessment: adequate scoped correction; no blocking source finding.

state_view.rs:190 wraps the existing 34px missing_controls slot in container(...).id("state-missing-picker"). The slot remains present whether an option exists or not; selection-controls and canvas sibling positions remain unchanged.
state_picker_escape_tests.rs:49-54 measures that container through existing mounted geometry, uses its center for opening, and derives the option point as bounds.y + bounds.height * 1.5. With the present 34px slot, picker height approximately 31.6px, one missing element, and fixed 1800x1100 viewport, opening y+17 is interior to the picker and option y+51 is interior to the downward menu approximately y+31.6..63.2. This removes dependence on intervening selection controls. It does retain the explicit fixture assumption that the one-option menu opens downward; this is reasonable for this large fixed viewport and not a general small-window menu geometry test.
state_picker_escape_tests.rs:13-17 requires the final event status to be Captured before forwarding subscriptions. Capture alone does not mathematically identify which widget captured the event. However, combined with the measured correct control, empty dismissal message vector, exact snapshot/source/selection/camera/revision/history checks, successful reopen followed by the actual StateDiagram(Event::Place) message, and exact Undo restoration, it supplies proportionate regression coverage for this fixture. An unrelated capture cannot be inferred from the inspected setup; separate overlay-open introspection need not block this small correction.
The existing preservation/reopen/Place/Undo assertions are retained without weakening. The new assertion improves diagnosis if targeting or forwarding regresses.

Wrapper impact: vendored container.rs:90-104 defaults to zero padding, left/top alignment, fluid sizing inherited from content, and no clipping. Its default Theme style is transparent (container.rs:560-574). It delegates tree tag/state/children/diff to its child (container.rs:235-249), forwards update using the same shell (container.rs:298-322), and forwards overlays (container.rs:372-388). Its layout adds a zero-padding layout node while retaining the child's fixed 34px content height. It does not insert an extra widget-state owner or add focus behavior. No expected change to slot reservation, canvas bounds, or focus-tree identity was found. This is a source conclusion; targeted/full State execution remains the required runtime confirmation.

Recommended acceptance: exact failing mounted test plus peer's full State suite, then parent-integrated desktop/workspace gates as planned. No extra production Escape changes or generalized measurement infrastructure are warranted by this diff.

9.8 KiB Raw Blame History