From 6c9d98978f20776365163511db35094cfa0f84ac Mon Sep 17 00:00:00 2001 From: Roo Code Date: Mon, 22 Sep 2025 17:19:52 +0000 Subject: [PATCH 1/3] fix: keep checkpoint menu visible when popover is open Fixes issue where the checkpoint restore popover would misposition when the mouse moved from the CheckpointSaved component to the popover itself. The menu now remains visible while the popover is open, preventing the anchor element from disappearing. Fixes #8219 --- .../src/components/chat/checkpoints/CheckpointMenu.tsx | 10 +++++++++- .../components/chat/checkpoints/CheckpointSaved.tsx | 7 ++++--- 2 files changed, 13 insertions(+), 4 deletions(-) diff --git a/webview-ui/src/components/chat/checkpoints/CheckpointMenu.tsx b/webview-ui/src/components/chat/checkpoints/CheckpointMenu.tsx index eba47699abc..683dde4c59f 100644 --- a/webview-ui/src/components/chat/checkpoints/CheckpointMenu.tsx +++ b/webview-ui/src/components/chat/checkpoints/CheckpointMenu.tsx @@ -13,9 +13,16 @@ type CheckpointMenuProps = { commitHash: string currentHash?: string checkpoint: Checkpoint + onPopoverOpenChange?: (open: boolean) => void } -export const CheckpointMenu = ({ ts, commitHash, currentHash, checkpoint }: CheckpointMenuProps) => { +export const CheckpointMenu = ({ + ts, + commitHash, + currentHash, + checkpoint, + onPopoverOpenChange, +}: CheckpointMenuProps) => { const { t } = useTranslation() const [isOpen, setIsOpen] = useState(false) const [isConfirming, setIsConfirming] = useState(false) @@ -54,6 +61,7 @@ export const CheckpointMenu = ({ ts, commitHash, currentHash, checkpoint }: Chec onOpenChange={(open) => { setIsOpen(open) setIsConfirming(false) + onPopoverOpenChange?.(open) }}> diff --git a/webview-ui/src/components/chat/checkpoints/CheckpointSaved.tsx b/webview-ui/src/components/chat/checkpoints/CheckpointSaved.tsx index b98f38b6d33..63d6870e974 100644 --- a/webview-ui/src/components/chat/checkpoints/CheckpointSaved.tsx +++ b/webview-ui/src/components/chat/checkpoints/CheckpointSaved.tsx @@ -1,4 +1,4 @@ -import { useMemo } from "react" +import { useMemo, useState } from "react" import { useTranslation } from "react-i18next" import { CheckpointMenu } from "./CheckpointMenu" @@ -15,6 +15,7 @@ type CheckpointSavedProps = { export const CheckpointSaved = ({ checkpoint, ...props }: CheckpointSavedProps) => { const { t } = useTranslation() const isCurrent = props.currentHash === props.commitHash + const [isPopoverOpen, setIsPopoverOpen] = useState(false) const metadata = useMemo(() => { if (!checkpoint) { @@ -48,8 +49,8 @@ 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%)", }}> -
- +
+
) From c606003166577b922e34b20ef673bb454cdaef50 Mon Sep 17 00:00:00 2001 From: daniel-lxs Date: Tue, 23 Sep 2025 15:05:33 -0500 Subject: [PATCH 2/3] fix(checkpoints): control Popover from parent to prevent anchor loss; add focused test for visibility logic (#8220) --- .../chat/checkpoints/CheckpointMenu.tsx | 35 +++++---- .../chat/checkpoints/CheckpointSaved.tsx | 6 +- .../__tests__/CheckpointSaved.spec.tsx | 71 +++++++++++++++++++ 3 files changed, 96 insertions(+), 16 deletions(-) create mode 100644 webview-ui/src/components/chat/checkpoints/__tests__/CheckpointSaved.spec.tsx diff --git a/webview-ui/src/components/chat/checkpoints/CheckpointMenu.tsx b/webview-ui/src/components/chat/checkpoints/CheckpointMenu.tsx index 683dde4c59f..cf244ee69c0 100644 --- a/webview-ui/src/components/chat/checkpoints/CheckpointMenu.tsx +++ b/webview-ui/src/components/chat/checkpoints/CheckpointMenu.tsx @@ -13,7 +13,8 @@ type CheckpointMenuProps = { commitHash: string currentHash?: string checkpoint: Checkpoint - onPopoverOpenChange?: (open: boolean) => void + open?: boolean + onOpenChange?: (open: boolean) => void } export const CheckpointMenu = ({ @@ -21,10 +22,11 @@ export const CheckpointMenu = ({ commitHash, currentHash, checkpoint, - onPopoverOpenChange, + 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") @@ -32,6 +34,9 @@ export const CheckpointMenu = ({ const previousCommitHash = checkpoint?.from + const isOpen = open ?? internalOpen + const setOpen = onOpenChange ?? setInternalOpen + const onCheckpointDiff = useCallback(() => { vscode.postMessage({ type: "checkpointDiff", @@ -41,13 +46,21 @@ export const CheckpointMenu = ({ 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) + setIsConfirming(false) + }, + [setOpen], + ) return (
@@ -56,13 +69,7 @@ export const CheckpointMenu = ({ - { - setIsOpen(open) - setIsConfirming(false) - onPopoverOpenChange?.(open) - }}> + @@ -81,7 +90,7 @@ export const CheckpointMenu = ({
{!isCurrent && (
-
@@ -89,39 +98,50 @@ export const CheckpointMenu = ({
)} -
+ {!isCurrent && (
- {!isConfirming ? ( - - ) : ( - <> - - - - )} - {isConfirming ? ( -
- {t("chat:checkpoint.menu.cannotUndo")} -
- ) : ( -
- {t("chat:checkpoint.menu.restoreFilesAndTaskDescription")} -
- )} + ) : ( + <> + + + + )} + {isConfirming ? ( +
+ {t("chat:checkpoint.menu.cannotUndo")} +
+ ) : ( +
+ {t("chat:checkpoint.menu.restoreFilesAndTaskDescription")} +
+ )} +
-
+ )}
diff --git a/webview-ui/src/components/chat/checkpoints/CheckpointSaved.tsx b/webview-ui/src/components/chat/checkpoints/CheckpointSaved.tsx index 9ecc6a4cc37..0172c1a344f 100644 --- a/webview-ui/src/components/chat/checkpoints/CheckpointSaved.tsx +++ b/webview-ui/src/components/chat/checkpoints/CheckpointSaved.tsx @@ -1,4 +1,4 @@ -import { useMemo, useState } from "react" +import { useMemo, useRef, useState, useEffect } from "react" import { useTranslation } from "react-i18next" import { cn } from "@/lib/utils" @@ -17,6 +17,36 @@ 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(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) { @@ -50,9 +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%)", }}> - {/* Keep menu visible while popover is open; otherwise rely on group-hover */} -
- + {/* Keep menu visible while popover is open or briefly after close to prevent jump */} +
+
) diff --git a/webview-ui/src/components/chat/checkpoints/__tests__/CheckpointSaved.spec.tsx b/webview-ui/src/components/chat/checkpoints/__tests__/CheckpointSaved.spec.tsx index 52c3d456881..3e1cd2a4652 100644 --- a/webview-ui/src/components/chat/checkpoints/__tests__/CheckpointSaved.spec.tsx +++ b/webview-ui/src/components/chat/checkpoints/__tests__/CheckpointSaved.spec.tsx @@ -1,25 +1,11 @@ // npx vitest run src/components/chat/checkpoints/__tests__/CheckpointSaved.spec.tsx -import { render, waitFor } from "@/utils/test-utils" -import React from "react" -import { CheckpointSaved } from "../CheckpointSaved" - -// Capture onOpenChange from Popover to control open/close in tests -let lastOnOpenChange: ((open: boolean) => void) | undefined - -vi.mock("@/components/ui", async () => { - const actual = await vi.importActual("@/components/ui") +vi.mock("@/components/ui", () => { + // Minimal UI primitives to ensure deterministic behavior in tests return { - ...actual, - Popover: ({ - children, - onOpenChange, - open, - }: { - children: React.ReactNode - open?: boolean - onOpenChange?: (open: boolean) => void - }) => { + Button: ({ children, ...rest }: any) => , + StandardTooltip: ({ children }: any) => <>{children}, + Popover: ({ children, onOpenChange, open }: any) => { lastOnOpenChange = onOpenChange return (
@@ -27,16 +13,28 @@ vi.mock("@/components/ui", async () => {
) }, - PopoverTrigger: ({ children }: { children: React.ReactNode }) => ( -
{children}
- ), - PopoverContent: ({ children }: { children: React.ReactNode }) => ( -
{children}
- ), + PopoverTrigger: ({ children }: any) =>
{children}
, + PopoverContent: ({ children }: any) =>
{children}
, } }) +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", @@ -45,15 +43,16 @@ describe("CheckpointSaved popover visibility", () => { } it("shows menu while popover is open and hides when closed", async () => { - const { container } = render() + const { getByTestId } = render() - const getMenu = () => container.querySelector("div.h-4.-mt-2") as HTMLElement + 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(() => { @@ -61,11 +60,82 @@ describe("CheckpointSaved popover visibility", () => { expect(getMenu().className).not.toContain("hidden") }) - // Close via captured handler + // 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() + + // 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() + + 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") + }) + }) })