Files
ARCADIA-Studio/harness/release-behavior-canvas-phase1-review-20260911.md
T

77 lines
12 KiB
Markdown
Raw Blame History

This file contains ambiguous Unicode characters
This file contains Unicode characters that might be confused with other characters. If you think that this is intentional, you can safely ignore this warning. Use the Escape button to reveal them.
# Shared behavior canvas phase 1 — independent source review
Codex-956atf `/root/canvas_review`, 2026-09-11. **Two unresolved P2 source findings; execution remains pending.** Reviewed frozen `codex/behavior-canvas-20260911` at `e511c4153fb5bb484652434340e0e4c7000c93d0` plus the uncommitted isolated patch in `.local/behavior-canvas-worktree-20260911`. This is an independent review of the [source handoff](release-behavior-canvas-phase1-source-20260911.md), against the [ratified design](release-unified-behavior-canvas-design-20260911.md), not a review of an integrated root executable.
Read AGENTS, PROJECT_GOAL, current Gitea issue 1/2 comments, open PR inventory (empty), BehaviorCanvasPhase120260911 and root's separate scenario focus fix/review notes. Ownership: [issue 2/comment 325](http://217.177.34.108:3000/sorokin.oleg.edu/ARCADIA-Studio/issues/2#issuecomment-325). Only this new root report was written. No production edits, Cargo/rustc, renderer creation, native UI, install, restart, commit or subagent execution occurred. Read-only `git diff --check` passed; the existing CRLF normalization warning is not a test failure. Locations below refer to the isolated freeze and are relative to `crates/oa-desktop/src`.
## Findings
**C1 — P2: external camera/tool changes do not cancel an active pan.** `behavior/canvas_workspace/viewport.rs:76–85` resets Input only when document/owner/revision changes. It ignores epoch, retains `pan`, and stores no viewport bounds. The disabled-keyboard branch at line 140 clears only Space. CursorMoved at lines 168–172 resumes any retained pan without checking its original scope, tool, geometry or keyboard owner.
Concrete widget sequence, applicable to either production host: start a middle-button drag; while holding middle, left-click the toolbar Fit or a palette tool; move the pointer again before releasing middle. The toolbar/palette action changes camera/tool and canvas epoch, but the stable Viewport tree retains the old pan. The next movement is still applied as part of that old gesture. Toggling Focus canvas while middle remains held also changes viewport origin/size without invalidating the retained pan. This contradicts the design's cancellation on mode/tool/geometry changes.
The App adapters compound the missing boundary: `app/process_canvas.rs:52–59` and `app.rs:1455–1464` accept all Camera events with matching document/owner/revision, regardless of epoch. A retained Pan captured before Fit, or before A→B→Back to the same owner, is accepted afterward and changes the newly fitted/restored camera. This is a source-level delayed-event falsifier, not a claim that a particular native event-loop interleaving was executed. Relative deltas do need to survive their own camera geometry increments; that exception must be narrower than every external epoch change.
Correction direction: separate the lifetime of a camera gesture/input batch from geometry updates caused by that same gesture. Invalidate it for external tool/Fit/panel-owner/bounds changes, while folding legitimate ordered deltas. Do not simply reject every successive delta after its first epoch increment. Required regression: actual mounted press → real toolbar/palette action → move/release, plus retained old Camera input after Fit/caller return; assert no resumed movement and unchanged source/history. Preserve the existing zero-distance semantic-cancellation test.
**C2 — P2: palette and camera controls have no keyboard entry/activation path.** `behavior/canvas_workspace.rs:18–43,153–181` builds plain Iced Button<Text> controls and gives IDs only to surrounding containers. The installed primary source `iced_widget-0.14.2/src/button.rs:257–272` registers no focusable state; its update at lines 298–345 handles mouse/touch, not Enter/Space. The palette has no keyboard wrapper or alternate shortcut mapping. Viewport's keyboard handling at `viewport.rs:203–240` requires already-focused state and has no tool activation.
Concrete sequence: start a clean process or scenario workspace with keyboard input, Tab through available fields, and attempt to focus/activate Work/Action/Lifeline or Canvas/Fit. These buttons never join focus traversal and cannot be activated by Enter/Space. Even after pointer-focus on the canvas, its Tab operation skips the palette controls. Thus a synchronized outline and keyboard-reachable palette/canvas, explicitly required by the design, are not delivered by this phase. This is separate from deferred F2 and advanced domain manipulation.
Add a real focusable palette/control or an explicit keyboard command path, with visible focus and a defined entry/exit policy respecting the existing Inspector/Review owner. Verify from an initially unfocused actual App root without preparatory pointer input. Root's parent-Name I1/I2 union does not fix this different entry/activation gap.
## Source checks without an additional material counterexample
- Both production paths are wired: `App::view` → `views::view` Behavior → `behavior.rs:1031,1034` → `topology/view.rs:203` or `scenario/workspace.rs:90`. Each mounts a Fill Canvas in the shared Viewport. The shared host at `canvas_workspace.rs:187` retains the canvas as the first stack child; a bounded right editor overlays it, with independent scrolling and pinned actions. Selection/draft/source-child branches retain that canvas path.
- Draw translates by camera pan then scales (`topology.rs:1411`, `behavior.rs:2038`). Input subtracts viewport origin through position_in, then pan, then divides by scale (`topology/direct.rs:652`, `behavior.rs:1861`). Pending markers share that world transform. Viewport clips drawing to its bounds and uses the intersection with the inherited viewport for pointer admission. Ordered CursorMoved positions, rather than only the batch-final cursor position, drive press/release coordinates.
- Camera start synthesizes child Unfocused before publishing CancelGesture, including zero movement (`viewport.rs:162–165,246–258`). Semantic Scene/Graph state validates the complete context, tool and bounds and cancels on wheel, resize, hidden release and unfocus. The App semantic adapters recheck full context and typed destinations. The C1 gap concerns the parent camera gesture's own lifetime.
- Zoom math preserves the world point under the supplied local cursor and rejects nonfinite inputs. Explicit Fit resolves to a stored numerical scale and zero pan once; ordinary size/selection/editor changes do not automatically refit.
- Scenario body/label hits begin the existing parent-bound reorder; nearest slots in another operand are refused, not silently redirected (`behavior.rs:1698–1702,1901–1920,2001–2015`). `scenario.rs:1051` also validates the original parent and emits SetSteps. The prepared expected Project uses the exact reordered IDs, with full source snapshots before/Undo/Redo. No arbitrary scenario y coordinates or lane persistence were introduced.
- Scenario return clones transient presentation and retains only the independent outline's one-shot receipt. Process return captures document/use/qualified context/camera/expansions and validates the use before restoration. Camera/selection/palette operations do not build model commands. This inspection found no source/history write in those camera branches.
## Prepared tests and evidence limits
Exactly six new production mounted tests exist: two `production_process_canvas_*` in `topology/layout_tests.rs`, and four `production_scenario_*` / `production_empty_scenario_*` in `scenario/workspace/tests.rs`. Two shared tests cover relative math and ordered Viewport events. `canvas_workspace/tests.rs:51–55` constructs `iced::Renderer` through Headless and uses `expect` on backend failure. No missing-backend skip or private drag-state injection was found. The six mount actual App::view, not an alternative shell.
The shared helper's `batch` at lines 86–125 executes widget events and App updates but discards every returned Task. It never runs widget-operation chains or `draw`. Consequently:
- Arrow rejection while a raw draft exists establishes the keyboard-enabled guard, not actual exclusive focus or Tab traversal.
- The caller test establishes App camera restoration after direct OpenUse/Back dispatch, not pointer activation of the navigation controls.
- The invalid-destination test proves no Reorder message/command; it does not render or inspect the refusal caption.
- These tests cannot independently establish pixels, transformed clipping, readable text, popup/tooltip geometry, or real native font/input behavior merely because they initialize a WGPU renderer.
These are explicit gate limits, not grounds for weakening fixtures. Run the original meaningful assertions at 1100×700 and the wider size, 50/100/200%, with complete Project/source/history checks. Add real task execution for keyboard gates and rendered/native evidence for visible refusal/clipping. The existing source-freeze report's execution-pending language remains appropriate.
## Required union and remaining scope
Preserve root's [focus fix](release-scenario-focus-fix-20260911.md) and [independent focus review](release-scenario-focus-review-20260911.md). Exact union points: outer App::view live owner marker and scenario_focus_owner; guarded FormShown handler; separate form session versus parent mount state and Back/Use/Escape rearming; stamped Name ID; parent-only sensor key/payload; new `behavior/scenario/focus.rs`; all five scenario_form_focus regressions. The isolated branch deliberately still has the baseline global Name/session sensor and one old helper test. Replace that obsolete test with the union, not the other way around.
Root's compiler corrections (dynamic Id::from(String), chooser Send/'static bounds, explicit imports and test cursor initialization) must survive integration. The new `behavior.rs` removes obsolete Canvas ScrollReveal machinery while retaining outline restoration; do not re-add an old Observer field solely to satisfy stale root tests.
B4 remains ytr83r-owned and absent from this branch. On integration, both its clip rectangle and clipped child text must receive camera pan plus scale; retain its lifecycle/geometry/render oracles. No B4 replacement was created here.
Root owns the combined source freeze, compiler, all eight prepared canvas tests, the five focus gates, relevant existing direct/scenario/contextual-source suites and proportionate lint/build checks. A matching Windows build and supported Linux runtime need actual minimum-size input/visual acceptance. This phase does not close persistent process placement, scenario lane-x/source round trips, full process movement, message endpoint expansion, fragment wrap/explicit operand transfer, accessibility, issue #2, or PROJECT_GOAL.
## Reviewed fingerprints
SHA-256 of the isolated readback. The source report hash matches the author handoff. Root integration/formatting will need fresh provenance.
| Path below desktop src (except harness) | SHA-256 |
| --- | --- |
| `app.rs` | `8ABC1E7C5C2B58336AD6007F0BF2B34319A68975B82A2264B9A115DFEE40B1C0` |
| `app/process_canvas.rs` | `744A30A401BA00DBA889AD1165C0892B7828904AA1027017E4D5F3A31BC6C66A` |
| `behavior.rs` | `B1DED3DFA99DE40B73A9761E987354FF33EC877198DA76A0CE59217970EA9512` |
| `behavior/canvas_workspace.rs` | `BA55EFE8C0615FEE5D413376BA4F3BF4AD2A0D10F9497D50B05A6FE87A318C44` |
| `behavior/canvas_workspace/viewport.rs` | `5F5057848CA6A8EBEF8AD3DE30A60BDCC969CF4E66E4174056AC26328CF08531` |
| `behavior/canvas_workspace/tests.rs` | `1DD64F1FFD6B2DE638E18CC30323914FC9D4C10BAE6C43658351729BE7B98B2E` |
| `behavior/scenario.rs` | `30FA0ACF751D4EA111E8257257258446A6BC44FCEC97479490431B84959EA5F7` |
| `behavior/scenario/workspace.rs` | `236BCEDF96C1D4F780F1665C763D2E419A522D4923F6A75C8F6462801FE7BD50` |
| `behavior/scenario/workspace/tests.rs` | `40B2B0081619F2963DD1DFE034868C02E01C3310F94BF6BCD88931304A1E316D` |
| `behavior/topology.rs` | `3AE3F657142620CDD8B6317756459E3D6FBF4D87F5014854C145521705D55BCC` |
| `behavior/topology/view.rs` | `EF7511BAE20787D5BD8D693888282238A5CD1ED494265E9E286836C57EB4FE81` |
| `behavior/topology/direct.rs` | `4356B81FBA65819AEC31218002D279B4132397EA970086646EFFA98EE57EF7D0` |
| `behavior/topology/direct_tests.rs` | `D3CD44477BDEB31256BA52AA70383CEFC9805A8B172271D357884C95DDEF11D8` |
| `behavior/topology/layout_tests.rs` | `189AD49D116D4D64B20822C7B2982191BD9855FEC5751B85516BA3CD0F155551` |
| `behavior/topology/contextual.rs` | `5D0AC5B3E258FC70987A00EDE1E603681D99746596F2FCC46AD2B3667D65203A` |
| `harness/release-behavior-canvas-phase1-source-20260911.md` | `82C72802EFC8516F441117056FD4A22AD091CAF3302B41527EA02ADD7144BD96` |