63 lines
11 KiB
Plaintext
63 lines
11 KiB
Plaintext
# Placement relocation policy: independent correction review
|
|
|
|
Reviewed clean tracked c7065db76ecca3faa733ccb5f09b87fd0a0958be in C:/ar-nav-01a0d717. This is a policy/source review of a proposed correction, not an applied-patch review or runtime pass. No tracked edit, Cargo, GUI, utility execution or Gitea operation. Parent owns the active native process and subsequent implementation/evidence.
|
|
|
|
## Verified regression and recommendation
|
|
|
|
The owner-change exception is the appropriate narrow correction. A placement's explicit Diagram property requests relocation and should follow the accepted destination. An ordinary property edit should retain the originating Architecture canvas. The c706 implementation incorrectly merges these intents: app.rs:1010-1027 finds the accepted placement's destination and sets its layer/selection; 1034-1041 then overwrites the destination with the old Architecture surface. The existing test at tests.rs:907-934 is a valid regression oracle and should remain unchanged.
|
|
|
|
I read the retained repaired-workspace/workspace-tests.stdout, SHA256 af19ab9b3a9e7e7a19f99fdb8853faac75e0e072721fdae1effe961627377e98: its desktop result is 1552 passed, 1 failed, 7 ignored. The existing relocation test fails at line 920 with actual Oa versus expected Sa. This does not establish that later assertions failed, but the source explains why the successful relocation's destination is overwritten. My previous applied-source review missed this caller contract; its no-new-blocker conclusion did not establish full-suite acceptance. Retain that review and this counterevidence rather than retrospectively changing their scope.
|
|
|
|
## Required implementation boundary
|
|
|
|
1. Capture a pre-edit placement owner only for RecordKind::Node. Prefer the draft's original owning diagram, checked against the pre-edit Project's membership of draft.original. Do not use active_diagram as the original owner: inspection context and the inspected placement's actual ownership are different facts. This capture belongs after ordinary dirty-draft admission, before execution.
|
|
2. Keep the existing draft command/resulting-ID execution and all failure paths unchanged. Only after successful acceptance resolve the placement owner using the resulting placement UUID. Compare two actual resolved diagram IDs. A changed UUID within one owner is not relocation; relocation to another same-layer diagram is relocation just as cross-layer relocation is.
|
|
3. When and only when that owner changed, omit the newly introduced old-Architecture-surface restoration. Retain the existing accepted-record ownership lookup and destination/selection reconciliation. Reusing that lookup's actual node owner is simpler and less error-prone than introducing a second divergent routing rule. Do not execute another command, call select(), infer a preferred view, or restore an old surface after sync.
|
|
4. For all other successful drafts, retain the current surface-preservation policy, including deliberate active-diagram identity/layer handling. A missing pre/post owner must not itself be interpreted as relocation: require a pair of resolved owners before testing inequality. Do not use unwrap or arbitrary fallback diagram selection merely to enforce the new exception.
|
|
|
|
These conditions make the proposal an exception for a verified explicit model edit, not a broad return to following the selected object's semantic layer.
|
|
|
|
## Commands, identity and failure cases
|
|
|
|
forms.rs:675-702 constructs Node drafts as EditNode(original owner, original ID, materialized placement), then RelocateNode using the materialized resulting node ID only when destination differs. This ordering deliberately supports simultaneous UUID and destination changes. resulting_id at 713 supplies that same accepted UUID. Checking the original UUID after execution would miss this case; comparing draft.kind's diagram to raw text without checking the accepted owner would confuse requested and accepted state.
|
|
|
|
Core session.rs:566-620 edits the original placement, tracks reidentity, then finds and moves the resulting placement to the requested diagram. prepare_command at 186-214 works on a candidate and validates it before acceptance. Missing original/destination records, UUID collision, incompatible destination, or duplicate placement failures therefore stay on the existing failed transaction path. The new UI exception must not run on any of them. The port-owner repair wrapper does not rewrite Node/EditNode/RelocateNode batches. No new core command or identity-map API is needed.
|
|
|
|
A valid accepted Node command preserves a placement with the returned UUID in one owning diagram; EditNode and RelocateNode do not delete that record. Thus absence/ambiguous ownership is not a legitimate successful relocation outcome to fabricate a navigation target for. Keeping the conditional conservative avoids a new panic/fallback; any actual successful missing-owner state would be a separate violated invariant requiring investigation, not justification to weaken the acceptance checks.
|
|
|
|
Simultaneously changing a placement's element does not itself imply navigation. If its owner remains the same, preserve the canvas and let existing validity/visibility rules determine presentation. If its owner changes, follow that accepted owner and actual destination layer; do not derive destination from the replacement element's layer. The core may accept a same-layer ordinary destination that filters the moved element from display. Following the explicitly selected destination does not authorize changing its filters or adding duplicate placements to make the card visible.
|
|
|
|
NodePlacement (model.rs:1609) has alias, element, position, size, collapsed and port_anchors, but **no Name field**. Use alias/position/UUID for same-owner placement controls. The existing semantic relation/element Name tests remain separate controls of ordinary inspection retention.
|
|
|
|
## Queued clicks and typed boundaries
|
|
|
|
After accepted relocation, the existing inspect_canvas_target saved-surface tuple comparison and SelectNode surface/visibility check should refuse a different target captured on the old surface, retain the accepted edit and report the existing reselect message. They should not infer the clicked target's new identity or undo the relocation. Test these post-acceptance paths, not just direct Apply. The same-placement dirty SelectNode early return intentionally defers draft acceptance for a just-started gesture; do not remove that existing identity-edit/drag lifecycle to force this new test path.
|
|
|
|
Architecture restoration is already gated by Page::Diagram and is_architecture. Suppress that restoration for proven relocation; do not replace or broaden the prior Behavior state-diagram/scenario owner logic. Core tests already exercise state placement compatibility and atomic cross-machine movement with route repair. forms::tests::state_placement_form_uses_draft_destination_membership_without_changing_other_views checks incompatible typed placements and a coherent element/destination combination. Reuse those negative controls and the existing Behavior/placement identity tests. This proposal should not introduce new transitions between Architecture and typed representations.
|
|
|
|
## Observable gates
|
|
|
|
- Keep tests::placement_relocation_follows_cross_layer_traceability_view unchanged and rerun it. It checks the destination, layer, selected placement and removal from the original view, followed by an intentional destination-layer edit.
|
|
- Add an App-level simultaneous placement UUID plus destination edit, including a same-layer destination case. Assert accepted selected/resulting placement UUID, exact destination, full expected Project and source transaction, revision and one-step undo/redo. The existing forms::tests::moving_placement_and_changing_identity_is_one_lossless_undoable_edit establishes the command semantics but does not check App reconciliation.
|
|
- Same-owner alias/position and UUID-only edits must keep ordinary canvas context. Reuse placement_identity_edit_then_drag_and_diagram_identity_then_arrange as a control for identity/gesture behavior; retain the F1 cross-layer semantic Name Apply tests.
|
|
- Accept a pending relocation while clicking a different previously visible edge/port and while selecting a different card. Assert the relocation and destination remain accepted, the stale old target is not selected, and no second history/model/source change occurs. Place behavior assertions before exact status text to avoid the earlier status-first evidence ambiguity.
|
|
- Invalid/missing destination and conflicting UUID must preserve source, Project, revision/history and recoverable draft. Existing core negative cases cover transaction validation; at least one App invalid-draft control should ensure no navigation occurs before acceptance.
|
|
- Rerun the complete corrected workspace gate after scoped checks. The prior 20 canvas/5 Draw pass did not cover this independent pre-existing behavior. New source requires new build attribution for any subsequent native evidence; do not relabel the still-running c706 native process as testing the repair.
|
|
|
|
## Contract and alternatives
|
|
|
|
Amend the Properties contract and guide's unconditional retained-view sentence with a concrete exception: "Changing a placement's Diagram moves that placement and follows its accepted destination. Other property edits keep the current saved Architecture view; edits to that view's own identity or perspective still take effect." State that an earlier canvas click invalidated by the accepted move must be selected again. This preserves the explicit destination requirement already evidenced by the existing test; it is not a new product feature. Adjust the apply() comment that currently says Properties do not request navigation so it describes ordinary edits and the explicit relocation exception accurately.
|
|
|
|
Rejected: exempting every Node draft (would lose same-owner retention); switching only when semantic layer changes (misses same-layer destinations); trusting a dirty /diagram field (request is not accepted ownership); scanning a Command::Batch for a relocation variant (couples UI policy to command assembly and can overclassify no-op/failing requests); globally removing preservation (reintroduces F1); broad new navigation-policy abstractions or changes to core relocation (unnecessary for this demonstrated desktop reconciliation defect).
|
|
|
|
## Inspected hashes
|
|
|
|
- app.rs: f9a156caaf8d6651f73af7ae6cddbd6700564351ef117932339e6bd4515853b7.
|
|
- forms.rs: 7722645933a95c4a6b5c81a2752dda4270ff39d448f2362c51bd143c6585e706.
|
|
- tests.rs: 0552b7557b223bdf114ed37429940dc84e4c2830dd6ca30ca26744b29762311b.
|
|
- core/session.rs: c31ea79bf37a15a35c4b6b35c0c3ebf7c029ce0ec8213b3865308961bb3d9f52.
|
|
- core/model.rs: 05a6bd5eef14c77e7cafd2bc72eb3bb6b19f33c6d06184b4e7d21173a2684078.
|
|
- navigation-draft-contract-20261001.md: 51b9d2a5d53548e68a4a54e10b4bce7623523f4b605d1e368d089f62863827d1.
|
|
|
|
Recommendation: implement and test only the proven owner-change exception, with actual resulting-ID ownership and unchanged failure/typed policies. This recommendation supports a bounded candidate on source evidence; it is not an accepted repair or native pass.
|