Skip to content

Commit 25eeee7

Browse files
authored
e2e: fix flaky auto-scroll drag reachability check in notebook cell reordering (#14908)
### Summary Fixes flaky/failing "Drag-and-drop: auto-scroll when dragging in long notebook" e2e test by replacing a redundant, racy reachability check with the app's own drop-target signal. - `dragCellToPositionWithScroll` no longer re-derives "is the target reachable" from cell bounding boxes - Instead waits on the `drop-indicator` testid at the correct gap index, which reflects dnd-kit's real collision detection (`computeDropIndex`) - Removes `isTargetReachable`/`initialCheck`/`finalCheck` entirely (-44 lines) - Root cause found via triage: CI evidence showed dnd-kit had already resolved the drop while the test's separate geometry check disagreed and threw ### QA Notes @:web @:positron-notebooks
1 parent 0575c73 commit 25eeee7

2 files changed

Lines changed: 18 additions & 57 deletions

File tree

test/e2e/pages/notebooksPositron.ts

Lines changed: 13 additions & 57 deletions
Original file line numberDiff line numberDiff line change
@@ -56,6 +56,9 @@ export class PositronNotebooks extends Notebooks {
5656
sortableCellAtIndex = (index: number) => this.code.driver.currentPage.locator('.sortable-cell').nth(index);
5757
dragHandleAtIndex = (index: number) => this.sortableCellAtIndex(index).getByRole('button', { name: /Drag to reorder cell/i });
5858
dragZoneAtIndex = (index: number) => this.sortableCellAtIndex(index).locator('.cell-drag-zone');
59+
// One AddCellButtons per gap (including before the first and after the last cell), in DOM order by gap index
60+
addCellButtonsAtGap = (gapIndex: number) => this.code.driver.currentPage.locator('.positron-add-cell-buttons').nth(gapIndex);
61+
dropIndicatorAtGap = (gapIndex: number) => this.addCellButtonsAtGap(gapIndex).getByTestId('drop-indicator');
5962
moreActionsOption = (option: string) => this.code.driver.currentPage.locator('button.custom-context-menu-item', { hasText: option });
6063
runCellButtonAtIndex = (index: number) => this.cell.nth(index).getByRole('button', { name: 'Run Cell', exact: true });
6164
private executionOrderBadgeAtIndex = (index: number) => this.cell.nth(index).locator('.execution-order-badge');
@@ -587,71 +590,24 @@ export class PositronNotebooks extends Notebooks {
587590
? containerBox.y + containerBox.height * 0.85 // Near bottom edge
588591
: containerBox.y + containerBox.height * 0.15; // Near top edge
589592

590-
// Helper to check if target cell is visible and in a good drop position
591-
const isTargetReachable = async (): Promise<{ reachable: boolean; targetY?: number }> => {
592-
const targetCell = this.sortableCellAtIndex(toIndex);
593-
const targetBox = await targetCell.boundingBox();
594-
595-
if (!targetBox) {
596-
return { reachable: false };
597-
}
598-
599-
// Ensure target is sufficiently visible within container (not just peeking)
600-
const targetCenter = targetBox.y + targetBox.height / 2;
601-
const containerTop = containerBox.y + containerBox.height * 0.1;
602-
const containerBottom = containerBox.y + containerBox.height * 0.9;
603-
604-
if (targetCenter >= containerTop && targetCenter <= containerBottom) {
605-
// Target 75%/25% inside the cell because dnd-kit's collision
606-
// detection uses the vertical midpoint to decide above vs.
607-
// below placement. See: SortableCellList.tsx collisionDetection
608-
// callback (midY calculation).
609-
const dropY = scrollingDown
610-
? targetBox.y + targetBox.height * 0.75
611-
: targetBox.y + targetBox.height * 0.25;
612-
return { reachable: true, targetY: dropY };
613-
}
614-
615-
return { reachable: false };
616-
};
617-
618-
// This method is only used for real (non-no-op) moves, so
619-
// the drop indicator will always appear.
620-
const dropIndicator = this.code.driver.currentPage.getByTestId('drop-indicator');
593+
// The gap where dnd-kit will actually drop: after the target cell when
594+
// scrolling down, before it when scrolling up. This is the same index
595+
// AddCellButtons receives -- see PositronNotebookComponent.tsx.
596+
const targetGapIndex = scrollingDown ? toIndex + 1 : toIndex;
597+
// The drop indicator reflects dnd-kit's own collision detection
598+
// (computeDropIndex in sortableCellListLogic.ts), which is the
599+
// authoritative source of truth for reachability -- unlike a
600+
// bounding-box re-check, it can't disagree with the real drag state.
601+
const targetDropIndicator = this.dropIndicatorAtGap(targetGapIndex);
621602

622603
try {
623-
// First check if target is already visible (no scrolling needed)
624-
const initialCheck = await isTargetReachable();
625-
if (initialCheck.reachable && initialCheck.targetY !== undefined) {
626-
await this.code.driver.currentPage.mouse.move(startX, initialCheck.targetY, { steps: 10 });
627-
await expect(dropIndicator).toBeVisible({ timeout: 2000 });
628-
return;
629-
}
630-
631-
// Move to edge and wait for auto-scroll to bring target into view
632-
// Use polling with timeout instead of fixed iteration count
633604
await this.code.driver.currentPage.mouse.move(startX, edgeY, { steps: 5 });
634605

635606
await expect(async () => {
636607
// Keep cursor at edge to maintain auto-scroll
637608
await this.code.driver.currentPage.mouse.move(startX, edgeY, { steps: 2 });
638-
639-
const result = await isTargetReachable();
640-
if (!result.reachable) {
641-
throw new Error('Target not yet reachable');
642-
}
643-
return result;
609+
await expect(targetDropIndicator).toBeVisible({ timeout: 500 });
644610
}).toPass({ timeout: 15000, intervals: [100, 200, 300, 500] });
645-
646-
// Target is now reachable - get fresh position and drop
647-
const finalCheck = await isTargetReachable();
648-
if (finalCheck.reachable && finalCheck.targetY !== undefined) {
649-
await this.code.driver.currentPage.mouse.move(startX, finalCheck.targetY, { steps: 10 });
650-
await expect(dropIndicator).toBeVisible({ timeout: 2000 });
651-
return;
652-
}
653-
654-
throw new Error(`Could not reach target cell at index ${toIndex} via auto-scroll`);
655611
} finally {
656612
await this.code.driver.currentPage.mouse.up();
657613
}

test/e2e/tests/notebooks-positron/notebook-side-by-side.test.ts

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -113,6 +113,11 @@ test.describe('Notebook Side-by-Side Isolation', {
113113
const interpreterNameNb1 = /Untitled\-1.ipynb/;
114114
const interpreterNameNb2 = /Untitled\-2.ipynb/;
115115

116+
// The prior test starts kernels but only closes their editor tabs
117+
// (never shuts down the kernels), so clear those lingering sessions
118+
// before this test's own kernel selection/autostart timing checks.
119+
await sessions.deleteAll();
120+
116121
// Notebook 1: Create and start kernel
117122
await notebooksPositron.newNotebook();
118123
await notebooksPositron.kernel.select('Python');

0 commit comments

Comments
 (0)