80 lines
14 KiB
Markdown
80 lines
14 KiB
Markdown
# Independent production reference chooser review — 2026-09-11
|
||
|
||
Reviewer: Codex-956atf `/root/semantics_critic`. Scope: Inspector and active semantic Review scalar live-reference chooser; read-only production inspection. The reviewer did not run Cargo, the desktop application, or native input, and did not edit production, prior reports, or Git state. Root owns integration and execution; `behavior_audit` owns corrections. Scenario FormShown and canvas scrolling are separate work.
|
||
|
||
Status: the three concrete P2 findings below are corrected at source-review level after independent read-back. No remaining source blocker was found in the assigned chooser slice. The corrected host suite is still uncompiled/unexecuted at this review; this is not production or native acceptance. Gitea issue 1 comments 209 and 219 record ownership and the initial findings.
|
||
|
||
## Findings in the first production candidate
|
||
|
||
These locations refer to the formatted candidate inspected before the focus correction: `forms/reference_chooser/trigger.rs` SHA256 `529C36791272346C166064C02BB659FD0BE6C2A76376CC6C5D440B6BA22495F7`; `app/reference_authoring.rs` SHA256 `A8FEA3D252C01C849CE5B0ADE4FBBB9D3CF3A34E58C062FBEBF152710377F8E8`. Paths below are relative to `crates/oa-desktop/src` unless stated otherwise. They are source-derived falsifiers, not executed failure logs.
|
||
|
||
### K1 — P2: keyboard focus can enter a clipped field that cannot open
|
||
|
||
`forms/reference_chooser/trigger.rs:185` runs ordinary Iced focus traversal across all focusable descendants. `FromField`/`SetFocus` around lines 408–449 also select by traversal index alone. Neither reveals the selected field. The root `Measure` correctly clips reference anchors to nested viewport intersections at lines 357–391, and activation at line 236 publishes an Open only when such an anchor exists.
|
||
|
||
Minimal falsifier: put the reference trigger immediately below a 180-pixel form viewport; focus the last visible text input, press Tab, then Enter. The reference becomes focused below the viewport, but its anchor is `None`, so Enter publishes no chooser. Text inputs below the fold can likewise receive unseen typing. Escape/Tab return can select an offscreen neighbor. The original mounted scroll test snaps the reference into view before interaction, so it does not test this sequence.
|
||
|
||
The installed Iced 0.14 `widget/operation/focusable.rs` implements focus selection and traversal without using scroll geometry; there is no implicit reveal to complete this workflow. The author confirmed the defect. Resolution: select within the active form, minimally reveal the destination through its enclosing scrollables, and remeasure before activation; add a mounted test without a preparatory scroll.
|
||
|
||
### K2 — P2: fresh/boundary Tab loses form entry and can reach an inactive host
|
||
|
||
The same scope branch handles Tab only if `Count.focused` is true (`trigger.rs:185`). Iced `focus_next` unfocuses the last item and `focus_previous` unfocuses the first. Subsequent Tab sees no focused child and does nothing; a newly mounted unfocused form cannot enter its first field through this handler. Custom chooser return at `trigger.rs:430` also selects `None` beyond either boundary and clears all focus.
|
||
|
||
Minimal falsifiers: mount an unfocused form and press Tab; or focus its last field and press Tab twice; or close a chooser on its final field with Tab. No active field is recovered. Additionally, opening Review while Inspector retains focus leaves two mounted scopes: Iced `opaque` captures only mouse presses (`iced_widget-0.14.2/src/helpers.rs:660–682`), and `Stack` stops descending only after event capture (`stack.rs:231–263`). An unfocused Review can therefore let Tab reach the underlying Inspector. This is a real host-routing consequence of the same missing active entry policy, not a claim that an obsolete chooser passes the App's write guard.
|
||
|
||
The author confirmed the defect. Resolution: put keyboard routing at the stable root with the exact active form session, explicit first/last entry and boundary policy, and a distinction between a modal-owned Review form and an ordinary Inspector alongside other editable controls. Regression coverage must include an inactive mounted scope.
|
||
|
||
The correction must also prevent simultaneous inactive text focus: if modal entry focuses a Review reference trigger while an underlying Inspector text input stays focused, a character key unhandled by that trigger can pass through `Stack`/`opaque` into Inspector. Check retained Inspector raw text after modal entry, reference focus and character input, including chooser dismissal. Correct Tab routing alone does not prove general keyboard isolation.
|
||
|
||
### Additional ordering case required by the correction
|
||
|
||
An asynchronous Escape/Choose focus Task can be created while Inspector is active, then run after Review or another modal opens. The same Inspector field and generation can remain mounted underneath that modal. A traversal that only proves target existence can still focus the inactive host. Require current root ownership as well as exact field/session/generation before applying return focus. The original stale-focus test replaces the generation, so its target disappears and does not establish this stronger condition. The author has received this concrete test case.
|
||
|
||
### K3 — P2: the first focus correction disables a legitimate sibling Review input
|
||
|
||
The first correction's `Popup::update` calls `own_input` before every input while Review owns a Candidate form. Its `focus::exclusive` clears every focusable outside that form. This protects Inspector but also clears controls inside the same Review modal. A concrete existing control is `semantic_review/view.rs:333`, the required engineering rationale for explicit SA→OA technical-operation omission. It is mounted above `facts_view`, outside the Candidate detail scope around line 873.
|
||
|
||
Minimal falsifier: review such an Exchange occurrence in SA→OA direction, enable operation omission, select a Candidate facet, click its required rationale, then type a character. `own_input` unfocuses the rationale before dispatching the character, so its required text cannot be entered. The first correction's modal fixture contains only the reference trigger, so it cannot detect this regression. Resolution: distinguish the entire owning modal from the narrower field/form scope, clear only background focus, and add a mounted sibling-input regression. This finding was sent to the author and root at correction read-back before acceptance.
|
||
|
||
## Source checks with no counterexample found
|
||
|
||
- `app/reference_authoring.rs:52–89` checks Inspector versus exact active Review ownership and binds opening to document instance, project identity, document revision, page/layer, draft session, generation and live path. Unrelated modal, hosted, source-dirty, pending and busy states invalidate the chooser. This prevents the keyboard defects above from becoming authority to write an obsolete draft.
|
||
- `semantic_review/batch.rs:299–365` requires the current active row and Facts tab, checks the frozen project, rejects unaddressed writes and stops Open/Event messages before normal update/planning. `semantic_review/row.rs:529–545` requires the selected Candidate facet and authorized path roots, then resolves the correct scalar/group/attachment draft. The App validates again before mutation.
|
||
- `forms/reference_chooser/adapter.rs` derives candidates from the existing typed `reference_ids`, excludes nil and historical paths, retains invalid/missing/incompatible current text, and rechecks actual eligibility and optionality immediately before a scoped setter. Candidate metadata covers the actual `Project::identities` variants; the chooser does not introduce a second eligibility authority.
|
||
- The App's `Outcome::Browsed` branch at `reference_authoring.rs:140–144` restores only transient chooser state; it does not call the Review planner or the draft setter. Only explicit Choose/Clear uses `shape_scoped`, preserving unrelated raw inputs and constructor caches. Apply, accepted Project/source and history remain their existing separate boundary.
|
||
- The popup is persistently mounted above the ordinary view/context-menu wrapper and below hosted UI. Browser input is captured; the controller blocks global save/undo/open/new/source-navigation/guide shortcuts while open, and Escape closes the picker before its owning Review. Absolute row actions are invocation/result stamped; relative navigation/query ordering is intentional. Press and release must match the same pointer/action/stamp before a row write.
|
||
- The original root measurement traversal matches installed Iced's `Scrollable::operate` call order: each scrollable reports its offset immediately before child traversal. It accumulates nested offsets and viewport clipping with stack restoration. No coordinate contradiction was found by source inspection. The prepared mounted test establishes only a single already-scrolled case when executed; nested/clipped/full-app behavior still needs evidence.
|
||
|
||
## Evidence boundaries
|
||
|
||
The earlier 13/13 isolated chooser tests passed under root's test-only declaration and do not establish the new host adapter. The first production handoff adds six App tests and three retained-widget tests, initially unexecuted. They cover 1,000-entry eligibility, literal invalid sibling preservation, source/Project/Undo/Redo after Apply, invalid current/Clear, stale contexts, constructor/array changes, Review scalar/group/attachment behavior, banked rows and mounted opening/return/scrolling. The mounted renderer is a null text renderer with a purpose-built form, not the actual complete Inspector/Review view.
|
||
|
||
The correction must be compiled and its new focus/reveal tests executed by root, followed by the relevant existing forms/history/Review gates. Native acceptance remains necessary for actual font clipping, long details, 1100×700 layout, nested scrollers, modal priority, keyboard focus, gesture cancellation and measured 1,000-entry query/eligibility latency. This report does not turn authored tests, source reasoning, or the earlier isolated pass into production/native evidence.
|
||
|
||
## Corrected-source read-back
|
||
|
||
| Finding | Resolution independently inspected | Evidence still required |
|
||
| --- | --- | --- |
|
||
| K1: clipped focus | `trigger/focus.rs::Collect` retains enclosing scroll geometry, chooses the exact form destination, computes bounded reveal from inner viewport to outer viewport, then applies scrolling and focus. Root measurement runs again before Enter activation. | Execute the prepared fresh offscreen, one/two nested scroll, Enter, and return-after-scrolling-away test. The test starts at zero scroll and checks the reference really begins below the fold. |
|
||
| K2: entry, boundaries and ownership | Root Popup receives the current App form session. Fresh Tab/Shift+Tab enter first/last; boundaries cycle. Ordinary Inspector traversal respects another focused control. Exact asynchronous return requires both current root ownership and the original generation-scoped field. Review deactivates background input before ordinary input, including pointer opening and Outside dismissal. | Execute the bidirectional cycles, still-mounted old Inspector under actual Review ownership, character non-leak and Outside/no-focus-task tests. |
|
||
| K3: sibling rationale | A separate layout-neutral `trigger::modal` boundary wraps the entire `batch::State::view`, including header, rationale, facts and footer. Ordinary-input cleanup preserves everything inside that modal; Candidate form identity remains the narrower explicit Tab/return scope. | Execute the added SA→OA operation-omission rationale test and confirm the actual full view in native use. |
|
||
|
||
The final mounted rationale fixture loads a real operation-bearing SA occurrence, creates an OA counterpart, opens reverse Review, enables omission, and activates a Candidate parent facet. Its rationale is inside the modal but outside the Candidate form. It asserts every character emits the correct addressed rationale event, the final literal is `Engineering reason`, the rationale retains focus, and Candidate recovery, Inspector raw input, SourceBundle and Undo remain unchanged. The Inspector-under-Review and Outside fixtures likewise retain the actual old input and use real Stack/Opaque plus the reference adapter; they do not substitute a no-background modal.
|
||
|
||
The corrected host suite contains **14 prepared tests**: six App tests and eight mounted tests. None was run by this reviewer or the author at handoff. The separate earlier isolated **13/13** remains exactly that earlier executed checkpoint. Installed Iced Winit source also confirms that synchronous App updates rebuild user interfaces before returned widget operations (`iced_winit-0.14.0/src/lib.rs` around 1214–1251), supporting the new-generation/root-owner return design; it does not replace runtime acceptance.
|
||
|
||
Root can proceed with the compiled integration gates against this corrected candidate. A compile/test failure or native focus/geometry failure must be resolved before treating these findings as execution-closed. No redesign of typed eligibility, raw staging, semantic planning, or source/history contracts is indicated by this review.
|
||
|
||
Final correction hashes were independently read from disk and match the author's final freeze. Paths remain relative to `crates/oa-desktop/src`:
|
||
|
||
| File | SHA256 |
|
||
| --- | --- |
|
||
| `forms/reference_chooser/trigger.rs` | `4932CCE196DB20B7EC444C42AB01B3217D9B9AA0B9203D0E7466759B04722552` |
|
||
| `forms/reference_chooser/trigger/focus.rs` | `C59FC25096829B86632BE78979F54178D28653FB1E7447E5122FC5C1243456AE` |
|
||
| `forms/reference_chooser/widget.rs` | `D380A574F381C0AA6A5CEABC57F5879487623C816C120F2C3ADFE4C7737F03DC` |
|
||
| `app/reference_authoring.rs` | `81CFC1057082451C70E12DB3F5DCDE9FEC820F2B101592894B6CBAF132A5013C` |
|
||
| `app/reference_authoring/tests/mounted.rs` | `D50F89A1998DD6B37C841D63B33C1F72CD844FEB3CEA2F75C9294D3935416F9E` |
|
||
| `semantic_review/batch.rs` | `29F498265FC12C0B33B8186911ECC4D7C7DC806CC36BCBA51862436EAB1E12A1` |
|
||
|
||
These identify the inspected source before root formatting/build. They do not identify an executable. The author reports a clean `git diff --check`; the reviewer checked only the new review artifact for whitespace and preserved all frozen reports and production files.
|