PR Review Summary

Status: Request Changes

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Request Changes
• Rationale: The ticket premise and local-capture repair shape are valid, but the exact head is not merge-safe: the newly adopted applyDeltas() promise rejects when its SharedWorker destination is already gone, and the current tests cannot detect either that rejection or a wrong proxy id. These are bounded in-place repairs, so Drop+Supersede and follow-up debt are both the wrong disposition.

Peer-Review Opening: The cancelled-timeout diagnosis is real, and capturing windowId plus the proxy id before destruction is the correct foundation. The continuation needs to own its terminal dispatch outcome as rigorously as it now owns the dispatch inputs.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: Issue #16623; the two-file changed surface; current dev implementations of DragZone#destroyDragProxy, core.Base#timeout, worker.App#applyDeltas, and worker.Base#promiseMessage; the global Neo.isDestroyed rejection filter; nearby drag teardown consumers; and Memory Core recall for prior DragZone cleanup work.
• Expected Solution Shape: Proxy removal must survive destruction of the owning zone while retaining the original window and proxy identity. It must not hardcode a specific consumer or treat every asynchronous failure as a closed-window no-op; test isolation must restore the global delta spy and prove the exact id plus terminal rejection behavior.
• Patch Verdict: Contradicts the complete expected shape. Lines 330–340 correctly localize dispatch inputs and survive the timeout cancellation, but the returned applyDeltas() promise is left unobserved; worker.Base#promiseMessage() rejects with undefined when the port is gone.
• Premise Coherence: Cohesive with verify-before-assert at the ticket level, but the PR-body no-op claim is falsified by the owning transport. N/A for the other organism-value surfaces; this is a scoped core lifecycle repair.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #16623
• Related Graph Nodes: Related: #16499, #16621, PR #16622
• Origin Session ID: 84d669f4-2271-4d6a-8878-45e8754be6b3

🔬 Depth Floor

Challenge OR documented search (per guide §7.1):

• Challenge: The body says dispatch to a window closed in the interim is a harmless no-op. Exact source shows the opposite: App#applyDeltas() returns promiseMessage(), and the SharedWorker closed-port branch calls bare reject(). On this head, a named actual-method probe observed one unhandledRejection with reason === undefined.

Rhetorical-Drift Audit (per guide §7.4):

• PR description: fail — the harmless-no-op and engine-wide guarantee claims overshoot the terminal promise behavior.
• Anchor & Echo summaries: the production comment accurately explains the instance-timeout cancellation mechanism.
• [RETROSPECTIVE] tag: N/A — none added.
• Linked anchors: issue and sibling links are consistent with the claimed lane.

Findings: Correct the no-op framing together with the behavior; prose-only narrowing would leave the runtime regression.

🧠 Graph Ingestion Notes

• [KB_GAP]: The missing contract is at the App-worker transport edge: a vanished SharedWorker port rejects the request promise; it is not a no-op.
• [TOOLING_GAP]: None. Managed exact-head GitHub state, source reads, Memory Core recall, and the isolated node falsifier all completed.
• [RETROSPECTIVE]: Moving cleanup beyond instance destruction creates a second ownership obligation: the detached continuation must own both its captured inputs and the terminal transport outcome.

N/A Audits — 📑 📡 🔗

N/A across listed dimensions: this patch does not introduce a public contract ledger surface, OpenAPI description, workflow convention, skill integration, or new architectural primitive.

🎯 Close-Target Audit

• Close-targets identified: #16623
• #16623 is an open leaf and is not epic-labeled.
• The Evidence Ladder close-target condition is not yet met: AC3 is explicitly residual, but the issue body does not carry the required L3-deferred operator-handoff annotation and Resolves would close it before the promised film log.

Findings: Keep the residual auditable before retaining the magic close keyword.

🪜 Evidence Audit

• PR body contains an Evidence: declaration.
• L2 covers AC1–2 only; AC3 remains an L3 visual receipt.
• The residual is named in the PR body, but the close-target issue lacks the corresponding [L3-deferred — operator handoff needed] annotation.
• The body distinguishes the unit witness from the later filmed receipt.
• No L1/L2 evidence is promoted to a completed visual claim.
• The staged parent film is correctly treated as post-merge validation rather than evidence from this unmerged head.

Findings: Evidence-to-close mismatch until AC3 is either witnessed at this head or formally preserved as an operator-gated residual.

🧪 Test-Evidence & Location Audit

• Execution evidence: all 14 required checks are green at 71b3865821a8dfa0c2c6cdaf5acf40c1e4f15268; the author also reports the focused RED/GREEN and 537 adjacent tests.
• Reviewer falsifier: isolated exact-head node probe for the closed-destination promise; result was {"unhandled":1,"observed":[{"reasonIsUndefined":true}]}.
• Test location: test/playwright/unit/draggable/DragZone.spec.mjs is canonical.

Findings: The suite is green but incomplete. The destroy-race case computes proxyId and discards the scenario return; the normal-path check compares the recorded id to itself. Both would pass if production removed an unrelated node.

📋 Required Actions

To proceed with merging, please address the following:

• Handle the closed-destination rejection from the returned Neo.applyDeltas() promise without swallowing genuine active-window delta failures, and add a regression witness that fails on the current unhandled-rejection behavior. Update the harmless-no-op / engine-wide prose to the behavior actually proven.
• Return or otherwise retain the created proxy id in the fixture and assert both teardown scenarios dispatch exactly one removeNode for that captured id; replace the self-referential id assertion.
• Reconcile Resolves #16623 with residual AC3: either provide the exact-head visual receipt before merge, or annotate the issue AC as [L3-deferred — operator handoff needed] with the promised post-merge log path; otherwise make the PR relationship non-closing until the receipt exists.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 82 - Correct class ownership and local capture preserve lifecycle boundaries, but the detached continuation omits the destination-loss contract at the App-worker boundary.
• [CONTENT_COMPLETENESS]: 68 - The timeout-race explanation is strong, but the body contains a falsified no-op claim and the residual evidence is not represented on the close target.
• [EXECUTION_QUALITY]: 45 - Required CI is green, yet an exact-head falsifier proves a new unhandled rejection and both tests miss the proxy-identity contract.
• [PRODUCTIVITY]: 55 - The patch restores the previously cancelled dispatch, but it does not yet deliver safe closed-window cleanup or complete AC evidence.
• [IMPACT]: 55 - The method is small but inherited by DragZone consumers, so its async lifecycle behavior has core-wide reach.
• [COMPLEXITY]: 34 - Two files and one promise chain are involved, with moderate hidden complexity at destruction and worker-port boundaries.
• [EFFORT_PROFILE]: Maintenance - A bounded lifecycle bug fix with targeted regression coverage, not a new subsystem.

The local-capture idea should remain; close the terminal promise and identity-evidence gaps around it.

[review-budget-managed]

• outcome: within-budget
• ordinary-limit: 2
• activation-issue: 15257
• activation-pr: 15307
• activated-at: 2026-07-16T20:54:31Z