Repository navigation
fix(sidebar): map header drags to the nearest boundary slot instead of a dead zone - #8891
Conversation
…f a dead zone Fixes stablyai#8879
📝 WalkthroughWalkthroughSidebar drag previews now receive the measured scroll-content bottom and snap drops in expanded or interior section areas to the nearest header boundary. The shared preview logic handles midpoint tie-breaking and rejects positions below the rendered content end. Unit tests cover boundary selection and content limits, while the end-to-end manual-sort test verifies the resulting persisted project order. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 400c3872-a860-4aa7-a4eb-a9b87027b309
📒 Files selected for processing (8)
src/renderer/src/components/sidebar/project-group-header-drag.tssrc/renderer/src/components/sidebar/project-group-header-drop.test.tssrc/renderer/src/components/sidebar/project-group-header-drop.tssrc/renderer/src/components/sidebar/project-header-drag.tssrc/renderer/src/components/sidebar/project-header-drop.test.tssrc/renderer/src/components/sidebar/project-header-drop.tssrc/renderer/src/components/sidebar/worktree-sidebar-header-drop-preview.tstests/e2e/project-group-manual-sort.spec.ts
|
reviewing now |
…f a dead zone (stablyai#8891) * fix(sidebar): map header drags to the nearest boundary slot instead of a dead zone Fixes stablyai#8879 * fix(sidebar): bound header edge drops to measured content
Summary
Fixes #8879. Since 22d5989, dragging a project header or Project Group header only produced a drop indicator while the pointer was inside another header's own ~28px strip; over expanded section bodies — most of the sidebar — the preview returned
null, the indicator unmounted, and releasing was a silent no-op. Precise reordering with expanded sections was nearly impossible.computeWorktreeSidebarHeaderDropPreviewnow maps a pointer in a section body or interior gap to the nearest boundary slot (previous section's bottom vs next header's top, midpoint rule; ties resolve toward the next header). The indicator follows the pointer through the whole list, and drops land where the indicator shows —dropIndexis derived from the same slot that positions the line, for both header consumers.Guard against fabricated drops: the measured content bound (
container.scrollHeight, same coordinate space as the virtualizer-measured header tops) is applied up front to every branch, so a pointer below the actually-rendered list still cancels exactly like before. This also tightens the pre-existing edge-zone/final-boundary path — an overshoot final drop that lands below real content now cancels too (regression test added); that path's internal slot logic is otherwise unchanged. #6609's reorder-index fix (sibling reindexing) is untouched; that issue never asked for a dead zone — the same commit even acceptssectionBottom + gapfor the final boundary slot, so the interior dead zone was accidental scope.Out of scope (documented in #8879): the
Math.max(scrollTop, …)indicator clamp divergence, and replacing the estimator-based per-section boundaries with measured offsets (pre-existing, shared with the final-boundary path — see Notes).Screenshots
UI behavior change (drop indicator tracking). No recording attached: the semantics are pinned by the unit suites (including the short-list differential regression added in 0788d5ad8) and the rewritten e2e spec in CI.
Testing
pnpm lint— oxlint clean on all 8 touched files (one hook refactored to stay under the max-lines cap rather than bumping it); full lint chain deferred to CIpnpm typecheck— node, cli, and web tsconfigs all passpnpm test— all 17 sidebar drag/drop suites: 118 passed (focused trio: 49); full suite deferred to CIpnpm build/ e2e — deferred to CI (no quick single-spec e2e path; the rewritten spec's expected order is derived fromcommitProjectHeaderDragDrop's ungrouped path and is boundary-choice-independent at the drag target used)Test coverage: the three previous expect-
nullpins are flipped to nearest-boundary assertions; new cases pin both boundary choices across an interior gap and the midpoint tie; a content-bound group pins both drift directions of the estimated section end (overshoot: pointer below measured content →null; within measured content → still snaps), for both the project-header and project-group-header consumers; the pinned final-boundary tests and the in-band header behavior stay byte-identical.AI Review Report
Two adversarial Codex rounds (
gpt-5.6-sol, read-only sandbox). Round 1 surfaced a concern about the estimatedsectionBottom; adjudicated with a full evidence chain: the per-section boundary estimator (worktree-header-section-boundaries.ts) omits the virtualizer's 6px inter-row gap and measured sizes, but it is pre-existing shared infrastructure — the new snap consumes it byte-identically to two shipped paths, committeddropIndexis a pure row-model integer classified against measured header tops (drift-immune), and near boundaries the nearest-boundary choice self-corrects to measured geometry. Round 2 found one high-severity real defect in the first draft: with estimate overshoot, a pointer below the actually-rendered list could be classified "inside" and commit a reorder where the parent returnednull. Fixed exactly per the reviewer's recommendation — an authoritative measured content bound rejects such points before snapping — with tests for both drift directions and both consumers. Cross-platform: pure renderer pointer-geometry; no platform APIs, shortcuts, or paths touched.Security Audit
Renderer-only pointer-geometry logic. No input handling, IPC, command execution, path handling, auth, secrets, or dependency changes. The one security-adjacent failure mode — converting an outside-the-list release (cancel intent) into a persistent reorder — was caught in review and is now bounded by measured content height with regression tests.
Notes
[bravo, alpha, charlie]) encodes the new WYSIWYG semantics; the old test pinned the accidental no-op.