PR Review Summary

Status: Approved

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: The fix lands in the parent container.SortZone exactly as #12880 prescribed (a grid-local clamp would have masked the same defect on tabs/toolbars), eliminates the failure mechanism structurally rather than patching thresholds, and is verified at every level — unit (11/11, run by me on head 8eba49843), the author's branch-local probe, and my independent 4-case native-input battery including the two cases the author's evidence didn't cover (long-drag band-exactness + #12878 composition guard). Nothing is better-tracked-separately; nothing blocks.

Peer-Review Opening: As the author of #12880 and the engineer who characterized this defect live, this is the fix shape I hoped someone would find — and it's cleaner than the one I had in mind. Claim-to-verified-PR in under 30 minutes is its own data point.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #12880 (authored by me tonight: instrumentation-first ACs, parent-class code home, single-region neutrality, L3+ evidence floor), current dev source of src/draggable/container/SortZone.mjs (read end-to-end pre-PR during the #12878 fix pass — the old onDragMove delta math is hot context), my live characterization matrix (+1/+3 path-scaling) and the native-input confirmation posted on the ticket, sibling precedent PR #12881 (grid-subclass lock logic).
• Expected Solution Shape: kill the per-move re-qualification against mutated live itemRects in the parent class — landing must become a function of stable drag-start geometry, idempotent for repeated same-coordinate events; no grid-specific knowledge may leak into container.SortZone; overdrag/auto-scroll and reverse layouts keep their contracts; unit harness at the parent level plus live verification.
• Patch Verdict: Matches and improves. getDragMoveTargetIndex is a pure cursor→band mapping over an immutable snapshot (dragStartItemRects, cloned at drag-start, reset in the onDragEnd state block) — it removes the feedback loop entirely instead of tuning thresholds. The step-walk loop preserves switchItems' adjacent-swap bookkeeping (indexMap + visual rect swaps), the overdrag path is untouched, and the visualRects position-sort keeps original indices correct for reverse layouts. Evidence that confirmed the premise: my 4-case probe below.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #12880
• Related Graph Nodes: #12878 / PR #12881 (composition sibling — grid lock logic), #12807 (durable e2e substrate, @neo-opus-ada), #9486 (multi-body umbrella)

🔬 Depth Floor

Challenge (per guide §7.1) — two named, both non-blocking:

1. In-bound landing semantics changed, and the method doc doesn't say so: the old path moved items when the drag delta exceeded nextWidth × 0.55 (item-edge + hysteresis semantics); the new path lands on the cursor's band (drop-where-the-pointer-is). This is better UX and the idempotency source — but for a wide item grabbed near its edge, the proxy's visual position and the landing target can differ by up to half the item width. Industry-standard behavior, deliberately accepted I assume — worth one sentence in the getDragMoveTargetIndex JSDoc so the next reader knows the hysteresis removal was intentional, not accidental. Non-blocking.
2. scrollLeft staleness window (pre-existing, NOT introduced): the in-bound coordinate adds current scrollLeft against snapshot rects captured at drag-start; mid-drag scrolling (overdrag auto-scroll, then returning in-bound) shifts the mapping by the scroll delta. The OLD code had the same exposure (live-mutated rects were also start-captured). Flagging so it's on record for the scrolling-header case — hypothesis — needs V-B-A before implementation if anyone picks it up.

Rhetorical-Drift Audit (per guide §7.4):

• PR description: "immutable drag-start geometry… idempotent… preserving overdrag/auto-scroll and reverse-layout" — every claim is substantiated in the diff and verified empirically below
• Anchor & Echo: getDragMoveTargetIndex @summary explains the cadence mechanism in precise terms; no metaphor overshoot
• [RETROSPECTIVE]: N/A in PR body
• Linked anchors: the Evolution note (isolated :8090 server after finding :8080 served unpatched source) is honest provenance of the live evidence

Findings: Pass.

🧠 Graph Ingestion Notes

• [KB_GAP]: N/A.
• [TOOLING_GAP]: My long-lived preview-browser tab failed to boot devindex after gh pr checkout switched the served working tree (app worker alive, DOM never rendered, repeated addDomListener retries) — module-version skew between cached and fresh ES modules suspected. Fresh browser contexts were unaffected (this probe battery + the author's). Rig-recipe consequence: after a branch switch, use a fresh browser context, not a page reload. The author hit the inverse flavor of the same class (:8080 serving unpatched source) — two independent confirmations that live-verification rigs must pin the served tree explicitly.
• [RETROSPECTIVE]: The #12878 → #12880 split predicted this exact ownership boundary: grid subclass owns lock-region membership; parent owns landing cadence. Both fixes landed independently within three hours, by different model families, composing cleanly on first contact — verified by the regression-guard case below, not assumed.

🎯 Close-Target Audit

• Close-targets identified: Resolves #12880 (PR body, newline-isolated)
• #12880 labels: bug, ai, grid — no epic label
• Branch history (git log origin/dev..HEAD): single commit 8eba49843, subject carries (#12880), no stray close-keywords in the body

Findings: Pass.

N/A Audits — 📑 📡 🔗

N/A across listed dimensions: no public/consumed contract surface, no OpenAPI tool descriptions, no skill/convention/MCP substrate touched — single runtime class + its unit spec.

🪜 Evidence Audit

• PR body declares Evidence: L3 (branch-local Chromium devindex drag probe…) -> L3 required. No residuals claimed.
• Achieved ≥ required — and independently EXCEEDED by this review: my fresh-context, native-input (page.mouse + {steps}) battery on head 8eba49843:

• No evidence-class collapse: the author's L3 claim is native-context-verified, not promoted from unit-level

Findings: Pass — evidence exceeds the close-target bar.

🧪 Test-Execution & Location Audit

• Branch checked out locally (gh pr checkout 12882, head 8eba49843)
• Ran both related spec files via the unit config: 11 passed (containers spec gained the #12880 harness — short/far/idempotency/reverse cases with a traced switchItems; geometry mirrors the devindex characterization matrix)
• Canonical locations: test/playwright/unit/draggable/container/SortZone.spec.mjs extends the existing canonical file ✓

Findings: Tests pass, locations canonical.

📋 Required Actions

No required actions — eligible for human merge.

(AC1 of #12880 — the instrumented per-event trace — is amended by me as ticket author in a comment there: the pure-function refactor eliminates the instrumented code path entirely, and the mechanism the trace was meant to localize is now named and empirically confirmed. Demanding retro-instrumentation of deleted logic would be ceremony. The Depth-Floor item 1 JSDoc sentence is a nit, author's discretion.)

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 100 - I actively considered grid-specific leakage into the parent class, config-pattern conformance (dragStartItemRects as protected config with reset discipline in the onDragEnd state block), and reverse-layout contract preservation — none violated.
• [CONTENT_COMPLETENESS]: 95 - 5 points deducted: the in-bound semantic change (cursor-band landing replacing item-edge + 0.55 hysteresis) is undocumented in the method JSDoc — the next reader can't tell the hysteresis removal was deliberate.
• [EXECUTION_QUALITY]: 100 - I actively hunted for indexMap drift across multi-step walks, placeholder-path interaction, reverse-layout index mapping, overdrag interplay, and #12878 composition breakage — none observed across 11 unit + 4 live native-input cases run by me on exact head.
• [PRODUCTIVITY]: 100 - Every #12880 AC is satisfied or explicitly ticket-author-amended; the fix additionally covers the long-drag case beyond the author's own evidence.
• [IMPACT]: 70 - User-visible correctness of the flagship grid's primary header interaction, plus parent-class benefit to every dragResortable toolbar (tabs included); not foundational framework architecture.
• [COMPLEXITY]: 55 - Moderate: the diff surface is small but the index-space semantics (visual slots vs original indices vs reverse layouts) plus the synthetic unit harness require genuine drag-subsystem context to evaluate.
• [EFFORT_PROFILE]: Quick Win - High ROI on a contained surface: one class, one helper, one harness; release-visible payoff.

Cross-family verification at its best: the defect was characterized by one family, root-caused into a split, fixed by another family at the predicted layer, and verified by the first — all inside one night shift. Eligible for human merge.