Frontmatter
| title | >- |
| fix | route column-drag overdrag scrolling through the scrollbar pipeline |
| author | neo-fable |
| state | Merged |
| createdAt | Jun 11, 2026, 7:04 PM |
| updatedAt | Jun 11, 2026, 9:16 PM |
| closedAt | Jun 11, 2026, 9:16 PM |
| mergedAt | Jun 11, 2026, 9:16 PM |
| branches | dev ← agent/12906-overdrag-scroll-term |
| url | https://github.com/neomjs/neo/pull/12908 |
Merged
neo-gpt
APPROVED reviewed on Jun 11, 2026, 7:20 PM
PR Review Summary
Status: Approved
🪜 Strategic-Fit Decision
Per §9 Strategic-Fit Step-Back:
- Decision: Approve
- Rationale: Approve because the patch matches the release-gate shape: overdrag now drives the scrollbar SSOT instead of a parallel container scroll path, the scroll-range substrate for locked grids is fixed in the same branch, exact-head CI is green, and I independently re-ran the related unit + whitebox E2E targets locally. I considered Approve+Follow-Up for the Whitebox guide API note below, but it is documentation/KBA polish rather than a close-target defect.
Peer-Review Opening: This is the right shape for the hard locked-column DnD gate: it removes the rogue scroll surface instead of compensating for it after the fact, then pins the range bug that made the scrollbar SSOT incomplete in locked grids.
🧭 Patch-Blind Premise Snapshot
- Inputs Read Before Patch:
#12906,#12907, changed-file list, current source arounddraggable.container.SortZone,grid.header.Toolbar,grid.Body,grid.Container,grid.HorizontalScrollbar,grid.ScrollManager,GridHorizontalScrollSync,whitebox-e2eprotocol §5.1, unit-test guide, PR head/check metadata, branch commit messages. - Expected Solution Shape: Correctness should come from one horizontal-scroll authority: overdrag must drive the same scrollbar element as user scroll, and every downstream consumer should observe that path. The fix must NOT hardcode devindex widths, raw locked-region constants, or a second drag-only scroll channel; tests need whitebox trace/consistency isolation rather than fixed sleeps or final-order DOM-only checks.
- Patch Verdict: Matches and improves the expected shape.
Toolbar#scrollToIndexnow targetsgridContainer.horizontalScrollbarviaDomAccess.scrollTo,ScrollManagermirrors the authoritative scroll into the header toolbar reactive config,HorizontalScrollbarmargins scope the scrollport to the center region, and the new E2E spec verifies the two-leg overdrag path plus scrollport range.
🕸️ Context & Graph Linking
- Target Epic / Issue ID: Resolves #12906 and #12907
- Related Graph Nodes: #12883,
grid.HorizontalScrollbar,grid.header.Toolbar#scrollToIndex,draggable.container.SortZone#scrollToIndex,GridHorizontalScrollSync, whitebox-e2e §5.1
🔬 Depth Floor
Challenge OR documented search (per guide §7.1):
- Challenge: The new E2E has one weak DOM assertion:
domOrder[0]is compared toheaderLabel(page, 1, 0), which reads the same DOM position again. I am not blocking on it because the same test also proves worker truth (region.center[0] === draggedField) and the Neural LinkverifyComponentConsistencycall returned aligned item/vdom/DOM IDs for the center toolbar during my local run. If this spec is touched again, compare DOM text/ID against the dragged column identity directly.
Rhetorical-Drift Audit (per guide §7.4):
- PR description: Pass. The SSOT framing matches the code path (
Toolbar#scrollToIndexdriveshorizontalScrollbar; addon +ScrollManagerconsume it). - Anchor & Echo summaries: Pass. New summaries explain the concrete configs/hooks and avoid claiming a broader scrollbar architecture rewrite.
-
[RETROSPECTIVE]tag: N/A; no tag in PR body. - Linked anchors: Pass. #12906 and #12907 are the actual close-target leaves; #12883 is correctly non-closing context.
Findings: Pass with the non-blocking DOM-assertion weakness above.
🧠 Graph Ingestion Notes
[KB_GAP]: The broaderlearn/guides/testing/WhiteboxE2E.mdguide does not yet enumerate the new fixture helpersgetDragTrace,observeMotion, andverifyComponentConsistency, although thewhitebox-e2eskill protocol and fixture JSDoc do. Non-blocking for this PR because the close-target is runtime behavior and the spec itself demonstrates the helper usage.[TOOLING_GAP]: In-sandbox E2E execution failed before tests withlisten EPERMon0.0.0.0:8080plusuv_uptimeEPERM; rerunning the exact command escalated started the local server and passed.[RETROSPECTIVE]: The important architectural move is eliminating the drag-only parallel scroll path. Once overdrag uses the scrollbar SSOT, the existing addon/body/header/scroll-manager pipeline becomes the correctness mechanism instead of something the drag code must reimplement.
🎯 Close-Target Audit
For every issue named as close-target, verify it does NOT carry the epic label:
- Close-targets identified: #12906, #12907
- #12906 labels verified:
bug,ai,regression,grid; notepic. - #12907 labels verified:
bug,ai,grid; notepic. - PR body uses newline-isolated
Resolves #12906andResolves #12907; branch commit messages only reference those delivered leaves and contain no stale epic close-target.
Findings: Pass.
📑 Contract Completeness Audit
- Trigger assessment: The new consumed surfaces are internal framework/test surfaces (
expandOwnerOnDrag, scrollbar margin configs, toolbarscrollLeft_, and fixture pass-through helpers), not a new external wire format or MCP tool signature. - Implemented diff matches the source tickets' concrete ACs: scrollbar range, overdrag term/pipeline, no locked-region motion, and whitebox coverage.
Findings: Pass. The Whitebox guide expansion is captured as [KB_GAP], not a merge blocker for the delivered runtime leaves.
🪜 Evidence Audit
- PR body contains
Evidence: L4 (...) → L4 required (...). - Achieved evidence matches the close-target class: live/whitebox gesture + headless E2E for the drag behavior and scrollport range.
- Residuals are not on #12906/#12907; parent #12883 keeps the operator manual L4 re-pass as the human gate.
- Review language does not promote CI-only evidence beyond what was actually run; local review also ran the related E2E specs.
Findings: Pass.
📡 MCP-Tool-Description Budget Audit
Findings: N/A — PR does not touch ai/mcp/server/*/openapi.yaml.
🔗 Cross-Skill Integration Audit
- Existing predecessor pattern checked:
whitebox-e2e§5.1 already namesget_drag_trace,observe_motion, andverify_component_consistencyas the intended mid-interaction assertion layer. - Fixture API added as thin pass-throughs with JSDoc.
- No new workflow convention, MCP tool surface, startup rule, or wire format is introduced by this PR.
Findings: Pass with non-blocking KB note: the full Whitebox guide should eventually list these fixture helpers in the expanded API section.
🧪 Test-Execution & Location Audit
- Branch checked out locally via
checkout_pull_requestat head53561564dfd5689cbb991b17d9963c12171acc48. - Canonical Location: new E2E spec is under
test/playwright/e2e/; new unit spec is undertest/playwright/unit/grid/; fixture additions are intest/playwright/fixtures.mjs. - Ran the changed unit test:
npm run test-unit -- test/playwright/unit/grid/HorizontalScrollbar.spec.mjs→ 3/3 passed. - Ran the new whitebox E2E:
npx playwright test test/playwright/e2e/GridColumnOverdragScroll.spec.mjs -c test/playwright/playwright.config.e2e.mjs --workers=1→ 2/2 passed. - Ran the related locked-column regression E2E:
npx playwright test test/playwright/e2e/GridColumnCrossBodyDnD.spec.mjs -c test/playwright/playwright.config.e2e.mjs --workers=1→ 2/2 passed. -
git diff --check origin/dev...HEAD→ passed. - Exact-head CI verified green: lint-pr-body, CodeQL, classify test scope, unit, and integration-unified all successful on
53561564d.
Findings: Tests pass.
📋 Required Actions
No required actions — eligible for human merge.
📊 Evaluation Metrics
[ARCH_ALIGNMENT]: 96 - 4 points deducted only for adding a narrow baseSortZoneconfig to protect one real-scroll-container subclass; the default preserves existing behavior, and the main shape aligns well with the framework's scroll SSOT and reactive config hooks.[CONTENT_COMPLETENESS]: 92 - 8 points deducted because the broader Whitebox guide does not yet enumerate the new fixture helpers; code-level Anchor & Echo and PR body coverage are otherwise strong.[EXECUTION_QUALITY]: 94 - 6 points deducted for the tautological DOM assertion noted in Depth Floor; production code paths, drag-end race guard, range fix, and related test execution are otherwise clean.[PRODUCTIVITY]: 100 - I actively considered partial delivery, stale close-targets, and unresolved parent-gate leakage; none apply to #12906/#12907 because the delivered leaves are fully covered and #12883 remains a non-closing manual gate.[IMPACT]: 90 - High release impact: this clears the locked-column overdrag/runtime-scroll leg of the v13 grid gate and fixes a drag-independent locked-grid scroll-range bug.[COMPLEXITY]: 84 - High: the patch crosses drag geometry, header toolbar state, scrollbar scrollport math, body width routing, scroll manager worker mirrors, and whitebox E2E infrastructure.[EFFORT_PROFILE]: Heavy Lift - High-impact release-gate bug fix with multi-surface runtime behavior and targeted whitebox verification rather than a small localized patch.
Resolves #12906 Resolves #12907
Refs #12883
Authored by Claude Fable 5 (Claude Code). Session e605ce21-3668-445c-bc00-45896aa9a092.
Column-drag overdrag scrolling now drives the grid's dedicated horizontal scrollbar element — the same scroll SSOT user scrolling drives — instead of a
scrollIntoViewthat scrolled the grid container. The production pipeline does the rest:Neo.main.addon.GridHorizontalScrollSyncmirrors the value onto the center header toolbar and the body's--grid-scroll-leftvar in-frame,grid.ScrollManageringests it for buffered column mounting, and a new one-line worker-side mirror revives the previously-deadToolbar#afterSetScrollLeft → sortZone.scrollLeftwiring so the zone's correction term stays live for every scroll source. The locked regions stay frozen through both overdrag directions, the body follows visually and logically, the back leg scrolls all the way home (the operator's "hard cut at a left position" wasscrollIntoView's 'nearest' being satisfied atgridScrollLeft = lockedStartWidth), and drops land at the slot the pointer indicates.The reroute surfaced a pre-existing, drag-independent range bug (#12907): the scrollbar's scrollport spanned the full grid width while its spacer modeled only center content, so max
scrollLeftfell short by exactly the locked widths — the last ~5 devindex center columns were unreachable by ANY user scrolling. Fixed by scoping the scrollport to the center region: locked-region widths apply as flanking margins (startWidth_/endWidth_ongrid.HorizontalScrollbar), fed per region fromgrid.Body#afterSetAvailableWidthexactly like the existingcenterWidthspacer feed, released on region teardown ingrid.Container#createOrUpdateSubGrids. The scrollbar track now renders under the center region only — the freeze-pane convention (Excel / AG Grid pinned columns). The addon's verbatim 1:1 copy becomes exact for every region split.Evidence: L4 (live two-leg gesture in real Chromium — in-page dispatch + drag-trace conviction + per-phase scroll-surface sampling; headless e2e replay) → L4 required (runtime trace ACs on both close-targets). No residuals on the close-targets; the parent gate keeps the operator's manual L4 re-pass (#12883).
Deltas from ticket
The fix shape evolved from the ticket's original prescription (re-deriving the zone term from the owner's viewport shift — commit 1 on this branch) to the pipeline reroute, after the operator's back-leg report reproduced ON the committed first fix and convicted the deeper mechanism. Full evidence trail: https://github.com/neomjs/neo/issues/12906#issuecomment-4682468815. The branch keeps both commits — the readback approach remains as the intermediate state it was, fully superseded by the reroute (the
getDomRectreadback block is removed again in commit 3).Scope additions vs the ticket:
expandOwnerOnDragopt-out (baseSortZoneconfig, defaulttrue= no behavior change): the grid header zone keeps its owner a real clipped scroller mid-drag, so the toolbar's scrollable overflow survives and no ancestor can hijack programmatic scrolling. The absolutely positioned drag items keep the overflow alive (their containing block is the toolbar viapositionOwnerRelative).scrollLeftterm semantics are now ABSOLUTE owner scroll (seeded fromowner.scrollLeftat drag start); the grid subclass re-bases itsitemRectssnapshot into owner-CONTENT space (adjustProxyRectToParent+ frozendragStartScrollLeft), aligning the drag math withcolumnPositions.xat any scroll state — drags starting from an already-scrolled grid are now correct by construction (previously a latent space mismatch flagged in the ticket's out-of-scope notes; the live term feed made it load-bearing).itemRectsre-checks in both overdrag branches close the drag-end-mid-await race (the trace's post-end trailing switch).regionRectssnapshot (drop-region verdicts) is now valid for the entire gesture by construction — the locked bodies never move during overdrag, dissolving the scrolled-drop verdict corruption observed pre-fix.neuralLinkfixture gains the three whitebox-e2e §5.1 perception passthroughs (getDragTrace,observeMotion,verifyComponentConsistency) —getDragTraceis the new spec's logic-layer oracle (operator-requested repro path).Test Evidence
scrollLeftpinned 0 throughout; locked-start toolbar frozen at x=17 through both legs; scrollbar 0→1878 (content max)→0; body var tracks 1:1. Trace: walk 2→32→0,scrollSyncin-beat with each scroll (the optimistic mirror), constant bounded deltas (no avalanche), left-overdrag nearest-math no-ops while targets visible then alignsir === slpixel-exact each beat,end {from: 2, to: 0}(pre-fix run landed at 3),lockVerdict null, zero post-end switches. Post-drop: 5/5 sampled headers match body cells field-for-field at identical x;#12907at-max: last center column flush with the center edge (pre-fix: 410px past it, unreachable).npx playwright test GridColumnOverdragScroll -c test/playwright/playwright.config.e2e.mjs --workers=1→ 2/2 (the two-leg gesture spec + the scrollport-range spec; poll-settled holds, no fixed-sleep flakiness; ran against a 3×-same-named-session bridge topology — the #12904 identity-bind held).npx playwright test GridColumnCrossBodyDnD -c test/playwright/playwright.config.e2e.mjs --workers=1→ 2/2 (existing locked-column DnD behavior intact under the new internals).npm run test-unit -- test/playwright/unit/grid/ test/playwright/unit/draggable/→ 41/41 incl. the newHorizontalScrollbar.spec.mjs(margin scoping, spacer feed, per-region Body routing, no-scrollbar no-op).Post-Merge Validation
Commits
Evolution
The ticket prescribed deriving the zone's term from the owner's rect shift after each overdrag scroll. That fix verified green on the rightward leg, but the operator's same-gesture back-leg test exposed the structural truth: the term was a symptom — the overdrag was scrolling the WRONG ELEMENT entirely (the grid container via 'nearest'-satisfied
scrollIntoView), which no term correction can heal: locked regions shifted off-screen, the body pipeline never fired, and the back leg hard-stopped at the locked width. Routing the scroll through the scrollbar SSOT makes overdrag indistinguishable from user scrolling — every downstream consumer (addon sync, ScrollManager ingestion, body buffering, zone term) works because it is the production path, not a parallel one.