fix: checkpoint restore popover positioning issue (#8219) (#8220)

Co-authored-by: Roo Code <[email protected]>
Co-authored-by: daniel-lxs <[email protected]>

webview-ui/src/components/chat/checkpoints/CheckpointMenu.tsx

@@ -8,16 +8,32 @@ import { useRooPortal } from "@/components/ui/hooks"
 import { vscode } from "@src/utils/vscode"
 import { Checkpoint } from "./schema"
 
-type CheckpointMenuProps = {
+type CheckpointMenuBaseProps = {
 	ts: number
 	commitHash: string
 	currentHash?: string
 	checkpoint: Checkpoint
 }
+type CheckpointMenuControlledProps = {
+	open: boolean
+	onOpenChange: (open: boolean) => void
+}
+type CheckpointMenuUncontrolledProps = {
+	open?: undefined
+	onOpenChange?: undefined
+}
+type CheckpointMenuProps = CheckpointMenuBaseProps & (CheckpointMenuControlledProps | CheckpointMenuUncontrolledProps)
 
-export const CheckpointMenu = ({ ts, commitHash, currentHash, checkpoint }: CheckpointMenuProps) => {
+export const CheckpointMenu = ({
+	ts,
+	commitHash,
+	currentHash,
+	checkpoint,
+	open,
+	onOpenChange,
+}: CheckpointMenuProps) => {
 	const { t } = useTranslation()
-	const [isOpen, setIsOpen] = useState(false)
+	const [internalOpen, setInternalOpen] = useState(false)
 	const [isConfirming, setIsConfirming] = useState(false)
 	const portalContainer = useRooPortal("roo-portal")
 
@@ -25,6 +41,9 @@ export const CheckpointMenu = ({ ts, commitHash, currentHash, checkpoint }: Chec
 
 	const previousCommitHash = checkpoint?.from
 
+	const isOpen = open ?? internalOpen
+	const setOpen = onOpenChange ?? setInternalOpen
+
 	const onCheckpointDiff = useCallback(() => {
 		vscode.postMessage({
 			type: "checkpointDiff",
@@ -34,13 +53,23 @@ export const CheckpointMenu = ({ ts, commitHash, currentHash, checkpoint }: Chec
 
 	const onPreview = useCallback(() => {
 		vscode.postMessage({ type: "checkpointRestore", payload: { ts, commitHash, mode: "preview" } })
-		setIsOpen(false)
-	}, [ts, commitHash])
+		setOpen(false)
+	}, [ts, commitHash, setOpen])
 
 	const onRestore = useCallback(() => {
 		vscode.postMessage({ type: "checkpointRestore", payload: { ts, commitHash, mode: "restore" } })
-		setIsOpen(false)
-	}, [ts, commitHash])
+		setOpen(false)
+	}, [ts, commitHash, setOpen])
+
+	const handleOpenChange = useCallback(
+		(open: boolean) => {
+			setOpen(open)
+			if (!open) {
+				setIsConfirming(false)
+			}
+		},
+		[setOpen],
+	)
 
 	return (
 		<div className="flex flex-row gap-1">
@@ -49,15 +78,10 @@ export const CheckpointMenu = ({ ts, commitHash, currentHash, checkpoint }: Chec
 					<span className="codicon codicon-diff-single" />
 				</Button>
 			</StandardTooltip>
-			<Popover
-				open={isOpen}
-				onOpenChange={(open) => {
-					setIsOpen(open)
-					setIsConfirming(false)
-				}}>
+			<Popover open={isOpen} onOpenChange={handleOpenChange}>
 				<StandardTooltip content={t("chat:checkpoint.menu.restore")}>
 					<PopoverTrigger asChild>
-						<Button variant="ghost" size="icon">
+						<Button variant="ghost" size="icon" aria-label={t("chat:checkpoint.menu.restore")}>
 							<span className="codicon codicon-history" />
 						</Button>
 					</PopoverTrigger>
@@ -66,7 +90,7 @@ export const CheckpointMenu = ({ ts, commitHash, currentHash, checkpoint }: Chec
 					<div className="flex flex-col gap-2">
 						{!isCurrent && (
 							<div className="flex flex-col gap-1 group hover:text-foreground">
-								<Button variant="secondary" onClick={onPreview}>
+								<Button variant="secondary" onClick={onPreview} data-testid="restore-files-btn">
 									{t("chat:checkpoint.menu.restoreFiles")}
 								</Button>
 								<div className="text-muted transition-colors group-hover:text-foreground">
@@ -74,39 +98,50 @@ export const CheckpointMenu = ({ ts, commitHash, currentHash, checkpoint }: Chec
 								</div>
 							</div>
 						)}
-						<div className="flex flex-col gap-1 group hover:text-foreground">
+						{!isCurrent && (
 							<div className="flex flex-col gap-1 group hover:text-foreground">
-								{!isConfirming ? (
-									<Button variant="secondary" onClick={() => setIsConfirming(true)}>
-										{t("chat:checkpoint.menu.restoreFilesAndTask")}
-									</Button>
-								) : (
-									<>
-										<Button variant="default" onClick={onRestore} className="grow">
-											<div className="flex flex-row gap-1">
-												<CheckIcon />
-												<div>{t("chat:checkpoint.menu.confirm")}</div>
-											</div>
-										</Button>
-										<Button variant="secondary" onClick={() => setIsConfirming(false)}>
-											<div className="flex flex-row gap-1">
-												<Cross2Icon />
-												<div>{t("chat:checkpoint.menu.cancel")}</div>
-											</div>
+								<div className="flex flex-col gap-1 group hover:text-foreground">
+									{!isConfirming ? (
+										<Button
+											variant="secondary"
+											onClick={() => setIsConfirming(true)}
+											data-testid="restore-files-and-task-btn">
+											{t("chat:checkpoint.menu.restoreFilesAndTask")}
 										</Button>
-									</>
-								)}
-								{isConfirming ? (
-									<div className="text-destructive font-bold">
-										{t("chat:checkpoint.menu.cannotUndo")}
-									</div>
-								) : (
-									<div className="text-muted transition-colors group-hover:text-foreground">
-										{t("chat:checkpoint.menu.restoreFilesAndTaskDescription")}
-									</div>
-								)}
+									) : (
+										<>
+											<Button
+												variant="default"
+												onClick={onRestore}
+												className="grow"
+												data-testid="confirm-restore-btn">
+												<div className="flex flex-row gap-1">
+													<CheckIcon />
+													<div>{t("chat:checkpoint.menu.confirm")}</div>
+												</div>
+											</Button>
+											<Button variant="secondary" onClick={() => setIsConfirming(false)}>
+												<div className="flex flex-row gap-1">
+													<Cross2Icon />
+													<div>{t("chat:checkpoint.menu.cancel")}</div>
+												</div>
+											</Button>
+										</>
+									)}
+									{isConfirming ? (
+										<div
+											data-testid="checkpoint-confirm-warning"
+											className="text-destructive font-bold">
+											{t("chat:checkpoint.menu.cannotUndo")}
+										</div>
+									) : (
+										<div className="text-muted transition-colors group-hover:text-foreground">
+											{t("chat:checkpoint.menu.restoreFilesAndTaskDescription")}
+										</div>
+									)}
+								</div>
 							</div>
-						</div>
+						)}
 					</div>
 				</PopoverContent>
 			</Popover>

webview-ui/src/components/chat/checkpoints/CheckpointSaved.tsx

@@ -1,5 +1,6 @@
-import { useMemo } from "react"
+import { useMemo, useRef, useState, useEffect } from "react"
 import { useTranslation } from "react-i18next"
+import { cn } from "@/lib/utils"
 
 import { CheckpointMenu } from "./CheckpointMenu"
 import { checkpointSchema } from "./schema"
@@ -15,6 +16,37 @@ type CheckpointSavedProps = {
 export const CheckpointSaved = ({ checkpoint, ...props }: CheckpointSavedProps) => {
 	const { t } = useTranslation()
 	const isCurrent = props.currentHash === props.commitHash
+	const [isPopoverOpen, setIsPopoverOpen] = useState(false)
+	const [isClosing, setIsClosing] = useState(false)
+	const closeTimer = useRef<number | null>(null)
+
+	useEffect(() => {
+		return () => {
+			if (closeTimer.current) {
+				window.clearTimeout(closeTimer.current)
+				closeTimer.current = null
+			}
+		}
+	}, [])
+
+	const handlePopoverOpenChange = (open: boolean) => {
+		setIsPopoverOpen(open)
+		if (open) {
+			setIsClosing(false)
+			if (closeTimer.current) {
+				window.clearTimeout(closeTimer.current)
+				closeTimer.current = null
+			}
+		} else {
+			setIsClosing(true)
+			closeTimer.current = window.setTimeout(() => {
+				setIsClosing(false)
+				closeTimer.current = null
+			}, 200) // keep menu visible briefly to avoid popover jump
+		}
+	}
+
+	const menuVisible = isPopoverOpen || isClosing
 
 	const metadata = useMemo(() => {
 		if (!checkpoint) {
@@ -48,8 +80,16 @@ export const CheckpointSaved = ({ checkpoint, ...props }: CheckpointSavedProps)
 						"linear-gradient(90deg, rgba(0, 188, 255, .65), rgba(0, 188, 255, .65) 80%, rgba(0, 188, 255, 0) 99%)",
 				}}></span>
 
-			<div className="hidden group-hover:block h-4 -mt-2">
-				<CheckpointMenu {...props} checkpoint={metadata} />
+			{/* Keep menu visible while popover is open or briefly after close to prevent jump */}
+			<div
+				data-testid="checkpoint-menu-container"
+				className={cn("h-4 -mt-2", menuVisible ? "block" : "hidden group-hover:block")}>
+				<CheckpointMenu
+					{...props}
+					checkpoint={metadata}
+					open={isPopoverOpen}
+					onOpenChange={handlePopoverOpenChange}
+				/>
 			</div>
 		</div>
 	)

webview-ui/src/components/chat/checkpoints/__tests__/CheckpointSaved.spec.tsx

@@ -0,0 +1,141 @@
+// npx vitest run src/components/chat/checkpoints/__tests__/CheckpointSaved.spec.tsx
+
+vi.mock("@/components/ui", () => {
+	// Minimal UI primitives to ensure deterministic behavior in tests
+	return {
+		Button: ({ children, ...rest }: any) => <button {...rest}>{children}</button>,
+		StandardTooltip: ({ children }: any) => <>{children}</>,
+		Popover: ({ children, onOpenChange, open }: any) => {
+			lastOnOpenChange = onOpenChange
+			return (
+				<div data-testid="popover-root" data-open={open}>
+					{children}
+				</div>
+			)
+		},
+		PopoverTrigger: ({ children }: any) => <div data-testid="popover-trigger">{children}</div>,
+		PopoverContent: ({ children }: any) => <div data-testid="popover-content">{children}</div>,
+	}
+})
+
+import { render, waitFor, screen } from "@/utils/test-utils"
+import React from "react"
+import userEvent from "@testing-library/user-event"
+import { CheckpointSaved } from "../CheckpointSaved"
+
+// Capture onOpenChange from Popover to control open/close in tests
+let lastOnOpenChange: ((open: boolean) => void) | undefined
+
+const waitForOpenHandler = async () => {
+	await waitFor(() => {
+		// ensure Popover mock captured the onOpenChange handler before using it
+		expect(lastOnOpenChange).toBeTruthy()
+	})
+}
+
+describe("CheckpointSaved popover visibility", () => {
+	// Timers are controlled per-test to avoid interfering with i18n init
+	const baseProps = {
+		ts: 123,
+		commitHash: "abc123",
+		currentHash: "zzz999",
+		checkpoint: { from: "prev123", to: "abc123" } as Record<string, unknown>,
+	}
+
+	it("shows menu while popover is open and hides when closed", async () => {
+		const { getByTestId } = render(<CheckpointSaved {...baseProps} />)
+
+		const getMenu = () => getByTestId("checkpoint-menu-container") as HTMLElement
+
+		// Initially hidden (relies on group-hover)
+		expect(getMenu()).toBeTruthy()
+		expect(getMenu().className).toContain("hidden")
+
+		// Open via captured handler
+		await waitForOpenHandler()
+		lastOnOpenChange?.(true)
+
+		await waitFor(() => {
+			expect(getMenu().className).toContain("block")
+			expect(getMenu().className).not.toContain("hidden")
+		})
+
+		// Close via captured handler — menu remains visible briefly, then hides
+		lastOnOpenChange?.(false)
+
+		await waitFor(() => {
+			expect(getMenu().className).toContain("block")
+		})
+
+		await waitFor(() => {
+			expect(getMenu().className).toContain("hidden")
+		})
+	})
+
+	it("resets confirm state when popover closes", async () => {
+		const { getByTestId } = render(<CheckpointSaved {...baseProps} />)
+
+		// Open the popover
+		await waitForOpenHandler()
+		lastOnOpenChange?.(true)
+
+		// Enter confirm state
+		const restoreFilesAndTaskBtn = await waitFor(() => getByTestId("restore-files-and-task-btn"))
+		await userEvent.click(restoreFilesAndTaskBtn)
+
+		// Confirm warning should be visible
+		expect(getByTestId("checkpoint-confirm-warning")).toBeTruthy()
+
+		// Close popover -> confirm state should reset
+		lastOnOpenChange?.(false)
+
+		// Reopen
+		lastOnOpenChange?.(true)
+
+		// Confirm warning should be gone after reopening
+		await waitFor(() => {
+			expect(screen.queryByTestId("checkpoint-confirm-warning")).toBeNull()
+		})
+	})
+
+	it("closes popover after preview and after confirm restore", async () => {
+		const { getByTestId } = render(<CheckpointSaved {...baseProps} />)
+
+		const popoverRoot = () => getByTestId("popover-root")
+		const menuContainer = () => getByTestId("checkpoint-menu-container")
+
+		// Open
+		await waitForOpenHandler()
+		lastOnOpenChange?.(true)
+		await waitFor(() => {
+			expect(popoverRoot().getAttribute("data-open")).toBe("true")
+			expect(menuContainer().className).toContain("block")
+		})
+
+		// Click preview -> popover closes; menu remains briefly visible, then hides
+		await userEvent.click(getByTestId("restore-files-btn"))
+		await waitFor(() => {
+			expect(popoverRoot().getAttribute("data-open")).toBe("false")
+			expect(menuContainer().className).toContain("block")
+		})
+		await waitFor(() => {
+			expect(menuContainer().className).toContain("hidden")
+		})
+
+		// Reopen
+		lastOnOpenChange?.(true)
+		await waitFor(() => {
+			expect(popoverRoot().getAttribute("data-open")).toBe("true")
+		})
+
+		// Enter confirm and confirm restore -> popover closes; menu then hides
+		await userEvent.click(getByTestId("restore-files-and-task-btn"))
+		await userEvent.click(getByTestId("confirm-restore-btn"))
+		await waitFor(() => {
+			expect(popoverRoot().getAttribute("data-open")).toBe("false")
+		})
+		await waitFor(() => {
+			expect(menuContainer().className).toContain("hidden")
+		})
+	})
+})