38 lines
9.0 KiB
Markdown
38 lines
9.0 KiB
Markdown
# Frozen behavior Delete candidate: independent acceptance review
|
|
|
|
Reviewed 2026-09-12 by `/root/behavior_delete_acceptance_review`, GPT-6 Astra high, read-only production review. Scope: the nine candidate files in `.local/behavior-delete-20260912`, branch `codex/behavior-delete-956atf-20260912`, uncommitted over `dc1506f`; implementation/design handoffs; applicable AGENTS and PROJECT_GOAL; surrounding dispatch, projection, focus, mounted harness and core deletion code. Root owns issue 15, integration, execution and delivery. This review did not use Gitea, Cargo, rustc, native UI or WSL and did not edit production files.
|
|
|
|
## Result
|
|
|
|
No blocking or high-severity implementation defect was confirmed by source review. The candidate is suitable for integration and executable acceptance work. This is not runtime or whole-product acceptance: candidate tests had not been executed by this reviewer, confirmation prose was deliberately awaiting parent integration, and the focused tests do not establish every required input/recovery workflow.
|
|
|
|
## Evidence supporting the implementation
|
|
|
|
- Exact identity: process Graph eligibility checks rendered node/edge IDs, rejects owner, and requires the ID's actual behavior owner (`behavior/topology/direct.rs:512`). App rechecks full semantic context, current selected ID, tool, modal/dirty state, projection eligibility and qualified-selection state (`app/process_canvas.rs:100`). Scenario uses actual scene box/line IDs plus owner membership (`behavior.rs:1193`, `behavior/scenario.rs:92`, `app.rs:1665`). This does not substitute referenced definitions or implied path endpoint IDs.
|
|
- Qualified process previews: the outer reference remains the App's selected UUID, but `has_qualified_selection` is checked before opening confirmation. The rejection explains how to open the shared definition or select the actual outer use. The qualified predicate resolves the owner/address against the current topology index. The mounted test uses a real preview coordinate and verifies a `SelectQualified` message before attempting Delete; it does not seed the qualified state.
|
|
- Capture and replay: `app/diagram_removal.rs:179` captures the derived owner after retiring the pre-modal canvas epoch. Confirmation requires the same owner at line 196 as well as the existing document/revision/epoch/page/layer snapshot and captured selection. Derived placement is consistently normalized to None in request, capture and validation (`:37`). Cancel/new-token replay and changed-owner/same-selection checks target the relevant lifetime risks.
|
|
- Semantic preservation: confirmation executes ordinary `Command::Delete` on the captured UUID. `oa-core/src/references.rs` follows owned children, incident/dependent relations and references into removed objects, rather than following an occurrence's reusable target outward. Lifeline removal clears participant references/bindings on retained occurrences. Call removal clears retained reply correlation. The candidate does not introduce a second deletion implementation.
|
|
- Input lifetime: nonrepeating plain/Shift Delete is admitted only for editable Select with no pending press or source. Middle/space pan blocks semantic keyboard forwarding. The new `focus_pending` immediately retires keyboard admission after viewport Tab/F6/Escape while keeping `is_focused` true for the deferred Collect/Apply operation. Real focus/unfocus clears it. This addresses the stale focused child during an Iced event batch without turning cursor presence into a keyboard ownership test.
|
|
- Harness validity: `canvas_workspace/tests.rs:155` builds the actual `App.view()` and calls pinned Iced `UserInterface.update` with the whole ordered event slice, then applies App messages and real widget operations. Pinned `iced_runtime-0.14.0/src/user_interface.rs` processes each base event before returning its messages, supporting the same-batch premise. Geometry probes calculate production projection coordinates; pointer tests assert the selected target afterward. Release followed by Delete in a subsequent build without a redraw is a meaningful check of stale child press state. Bundle compilation and workspace equality check the normal source/history path, although these checks still need execution.
|
|
|
|
## Acceptance gaps to close or explicitly retain
|
|
|
|
1. **Actual text ownership and keyboard recovery are not established by the two new focus tests.** `app/behavior_removal_tests.rs:253` chooses an arbitrary real text field and uses `FocusText` (`:255`, `:307`) to explicitly unfocus every other widget. That validly tests programmatic exclusive focus, but can conceal click/Tab ownership defects and does not identify the Inspector field. The boundary test (`:200`) checks lack of removal after Tab/F6/Escape but reacquires with pointer clicks; it does not assert that Tab reached a real control or that F6 can restore the viewport through the new pending flag. Suggested closure: select a real occurrence; click a named clean Inspector field, Delete at end-of-text, and assert no removal plus field ownership; then F6 and Delete with no pointer reacquisition. Separately assert the focused target after Tab, Shift+Tab and Escape, and keyboard-only F6 recovery.
|
|
2. **Remaining input variants are source-covered, not newly demonstrated.** The handoff correctly lists repeats, armed creation, message-source, connect/reconnect/reorder, contextual persisted links and legacy implied connectors as acceptance gaps. A small set of mounted cases should exercise these distinct mechanisms. For an implied connector, make the selected UUID and visible highlight explicit before and after clicking: unchanged selection of a previously selected occurrence must not be misreported as successful connector selection.
|
|
3. **Remaining scenario preservation variants are not all explicit in assertions.** The lifeline case proves messages are retained and sender/receiver references are cleared, but not scenario-use binding cleanup or action/state lifeline cleanup. The message case proves the reply survives but does not assert its `reply_to` was cleared. Core source supports those semantics; retain this distinction from executed behavior evidence.
|
|
4. **Confirmation prose and actual rendered recovery remain parent gates.** `diagram_removal::explanation` is intentionally not wired in the frozen candidate. Parent must use the captured object, inspect the rendered explanation for occurrence/link/lifeline/fragment cases, and inspect the incomplete-message state after lifeline removal. The supplied prose correctly distinguishes lifeline cleanup from deleting messages and reusable definitions.
|
|
|
|
These are verification gaps, not confirmed reproducible production defects. No new production fix is requested from this source review. Root's separate state relation-row visibility and F2 regression changes were outside this frozen candidate and must be included in its combined verification.
|
|
|
|
## Checks performed
|
|
|
|
Read scoped diffs, affected surrounding implementation, core cascade/reference cleanup, and installed Iced input/operation source. `git diff --check` passed in the frozen candidate worktree. A Git CRLF normalization warning appeared on an earlier diff read of `viewport.rs`; no content defect follows from that warning. No executable pass, GUI success, upstream coordination update or publication is claimed.
|
|
|
|
## Authorized evaluator follow-up
|
|
|
|
After the source review, parent authorized edits only to root `crates/oa-desktop/src/app/behavior_removal_tests.rs` to close acceptance gap 1. The frozen worker code is now `c2e651b455fbacda25d7840f6bbdaa79e594bcba` (parent reports CRLF normalization only), integrated as `49a4cb1`. Parent reports `12b2237` supplies confirmation explanation/rendered button tests and state row handling; root `2d9e9c1` adds interaction specification/help. Those later parent changes are not retroactively claimed as reviewed here.
|
|
|
|
The root test file now separately covers actual Inspector-name pointer focus and Escape followed by ordinary Tab entry, asserts actual focused widget geometry and `/name` binding via End + ShiftDelete while clean, then asserts F6 exclusively transfers focus to the viewport and permits subsequent Delete without another pointer event. The existing boundary test now checks completed Tab traversal to one real control and inverse ShiftTab return; F6/Escape exit and keyboard-only reentry; same-batch F6-entry/Delete refusal; and successful subsequent Delete. These expectations follow the existing production process/scenario keyboard tests: canvas Tab cycles canvas members, while Escape followed by ordinary Tab enters Inspector. The separate programmatic FocusText test remains and is not presented as evidence of pointer/Tab behavior.
|
|
|
|
Frozen root file SHA256: `EF09843A7DAEB91AFAB0B76E83C5679351A1635A84A26D80A194DEA744EA5874`. Scoped installed rustfmt (`--edition 2024 --config skip_children=true`) and scoped `git diff --check` passed. No Cargo, compiler, native, WSL, Gitea or Git/index mutation was run by this reviewer. Parent owns executable acceptance of this test-only follow-up; writing the tests does not close the runtime gap until they pass against the integrated implementation.
|