Skip to content

Commit 35791d0

Browse files
roomote[bot]roomotedaniel-lxs
authored
fix: checkpoint restore popover positioning issue (RooCodeInc#8219) (RooCodeInc#8220)
Co-authored-by: Roo Code <[email protected]> Co-authored-by: daniel-lxs <[email protected]>
1 parent 0682629 commit 35791d0

File tree

3 files changed

+264
-48
lines changed

3 files changed

+264
-48
lines changed

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

Lines changed: 80 additions & 45 deletions
Original file line numberDiff line numberDiff line change
@@ -8,23 +8,42 @@ import { useRooPortal } from "@/components/ui/hooks"
88
import { vscode } from "@src/utils/vscode"
99
import { Checkpoint } from "./schema"
1010

11-
type CheckpointMenuProps = {
11+
type CheckpointMenuBaseProps = {
1212
ts: number
1313
commitHash: string
1414
currentHash?: string
1515
checkpoint: Checkpoint
1616
}
17+
type CheckpointMenuControlledProps = {
18+
open: boolean
19+
onOpenChange: (open: boolean) => void
20+
}
21+
type CheckpointMenuUncontrolledProps = {
22+
open?: undefined
23+
onOpenChange?: undefined
24+
}
25+
type CheckpointMenuProps = CheckpointMenuBaseProps & (CheckpointMenuControlledProps | CheckpointMenuUncontrolledProps)
1726

18-
export const CheckpointMenu = ({ ts, commitHash, currentHash, checkpoint }: CheckpointMenuProps) => {
27+
export const CheckpointMenu = ({
28+
ts,
29+
commitHash,
30+
currentHash,
31+
checkpoint,
32+
open,
33+
onOpenChange,
34+
}: CheckpointMenuProps) => {
1935
const { t } = useTranslation()
20-
const [isOpen, setIsOpen] = useState(false)
36+
const [internalOpen, setInternalOpen] = useState(false)
2137
const [isConfirming, setIsConfirming] = useState(false)
2238
const portalContainer = useRooPortal("roo-portal")
2339

2440
const isCurrent = currentHash === commitHash
2541

2642
const previousCommitHash = checkpoint?.from
2743

44+
const isOpen = open ?? internalOpen
45+
const setOpen = onOpenChange ?? setInternalOpen
46+
2847
const onCheckpointDiff = useCallback(() => {
2948
vscode.postMessage({
3049
type: "checkpointDiff",
@@ -34,13 +53,23 @@ export const CheckpointMenu = ({ ts, commitHash, currentHash, checkpoint }: Chec
3453

3554
const onPreview = useCallback(() => {
3655
vscode.postMessage({ type: "checkpointRestore", payload: { ts, commitHash, mode: "preview" } })
37-
setIsOpen(false)
38-
}, [ts, commitHash])
56+
setOpen(false)
57+
}, [ts, commitHash, setOpen])
3958

4059
const onRestore = useCallback(() => {
4160
vscode.postMessage({ type: "checkpointRestore", payload: { ts, commitHash, mode: "restore" } })
42-
setIsOpen(false)
43-
}, [ts, commitHash])
61+
setOpen(false)
62+
}, [ts, commitHash, setOpen])
63+
64+
const handleOpenChange = useCallback(
65+
(open: boolean) => {
66+
setOpen(open)
67+
if (!open) {
68+
setIsConfirming(false)
69+
}
70+
},
71+
[setOpen],
72+
)
4473

4574
return (
4675
<div className="flex flex-row gap-1">
@@ -49,15 +78,10 @@ export const CheckpointMenu = ({ ts, commitHash, currentHash, checkpoint }: Chec
4978
<span className="codicon codicon-diff-single" />
5079
</Button>
5180
</StandardTooltip>
52-
<Popover
53-
open={isOpen}
54-
onOpenChange={(open) => {
55-
setIsOpen(open)
56-
setIsConfirming(false)
57-
}}>
81+
<Popover open={isOpen} onOpenChange={handleOpenChange}>
5882
<StandardTooltip content={t("chat:checkpoint.menu.restore")}>
5983
<PopoverTrigger asChild>
60-
<Button variant="ghost" size="icon">
84+
<Button variant="ghost" size="icon" aria-label={t("chat:checkpoint.menu.restore")}>
6185
<span className="codicon codicon-history" />
6286
</Button>
6387
</PopoverTrigger>
@@ -66,47 +90,58 @@ export const CheckpointMenu = ({ ts, commitHash, currentHash, checkpoint }: Chec
6690
<div className="flex flex-col gap-2">
6791
{!isCurrent && (
6892
<div className="flex flex-col gap-1 group hover:text-foreground">
69-
<Button variant="secondary" onClick={onPreview}>
93+
<Button variant="secondary" onClick={onPreview} data-testid="restore-files-btn">
7094
{t("chat:checkpoint.menu.restoreFiles")}
7195
</Button>
7296
<div className="text-muted transition-colors group-hover:text-foreground">
7397
{t("chat:checkpoint.menu.restoreFilesDescription")}
7498
</div>
7599
</div>
76100
)}
77-
<div className="flex flex-col gap-1 group hover:text-foreground">
101+
{!isCurrent && (
78102
<div className="flex flex-col gap-1 group hover:text-foreground">
79-
{!isConfirming ? (
80-
<Button variant="secondary" onClick={() => setIsConfirming(true)}>
81-
{t("chat:checkpoint.menu.restoreFilesAndTask")}
82-
</Button>
83-
) : (
84-
<>
85-
<Button variant="default" onClick={onRestore} className="grow">
86-
<div className="flex flex-row gap-1">
87-
<CheckIcon />
88-
<div>{t("chat:checkpoint.menu.confirm")}</div>
89-
</div>
90-
</Button>
91-
<Button variant="secondary" onClick={() => setIsConfirming(false)}>
92-
<div className="flex flex-row gap-1">
93-
<Cross2Icon />
94-
<div>{t("chat:checkpoint.menu.cancel")}</div>
95-
</div>
103+
<div className="flex flex-col gap-1 group hover:text-foreground">
104+
{!isConfirming ? (
105+
<Button
106+
variant="secondary"
107+
onClick={() => setIsConfirming(true)}
108+
data-testid="restore-files-and-task-btn">
109+
{t("chat:checkpoint.menu.restoreFilesAndTask")}
96110
</Button>
97-
</>
98-
)}
99-
{isConfirming ? (
100-
<div className="text-destructive font-bold">
101-
{t("chat:checkpoint.menu.cannotUndo")}
102-
</div>
103-
) : (
104-
<div className="text-muted transition-colors group-hover:text-foreground">
105-
{t("chat:checkpoint.menu.restoreFilesAndTaskDescription")}
106-
</div>
107-
)}
111+
) : (
112+
<>
113+
<Button
114+
variant="default"
115+
onClick={onRestore}
116+
className="grow"
117+
data-testid="confirm-restore-btn">
118+
<div className="flex flex-row gap-1">
119+
<CheckIcon />
120+
<div>{t("chat:checkpoint.menu.confirm")}</div>
121+
</div>
122+
</Button>
123+
<Button variant="secondary" onClick={() => setIsConfirming(false)}>
124+
<div className="flex flex-row gap-1">
125+
<Cross2Icon />
126+
<div>{t("chat:checkpoint.menu.cancel")}</div>
127+
</div>
128+
</Button>
129+
</>
130+
)}
131+
{isConfirming ? (
132+
<div
133+
data-testid="checkpoint-confirm-warning"
134+
className="text-destructive font-bold">
135+
{t("chat:checkpoint.menu.cannotUndo")}
136+
</div>
137+
) : (
138+
<div className="text-muted transition-colors group-hover:text-foreground">
139+
{t("chat:checkpoint.menu.restoreFilesAndTaskDescription")}
140+
</div>
141+
)}
142+
</div>
108143
</div>
109-
</div>
144+
)}
110145
</div>
111146
</PopoverContent>
112147
</Popover>

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

Lines changed: 43 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
1-
import { useMemo } from "react"
1+
import { useMemo, useRef, useState, useEffect } from "react"
22
import { useTranslation } from "react-i18next"
3+
import { cn } from "@/lib/utils"
34

45
import { CheckpointMenu } from "./CheckpointMenu"
56
import { checkpointSchema } from "./schema"
@@ -15,6 +16,37 @@ type CheckpointSavedProps = {
1516
export const CheckpointSaved = ({ checkpoint, ...props }: CheckpointSavedProps) => {
1617
const { t } = useTranslation()
1718
const isCurrent = props.currentHash === props.commitHash
19+
const [isPopoverOpen, setIsPopoverOpen] = useState(false)
20+
const [isClosing, setIsClosing] = useState(false)
21+
const closeTimer = useRef<number | null>(null)
22+
23+
useEffect(() => {
24+
return () => {
25+
if (closeTimer.current) {
26+
window.clearTimeout(closeTimer.current)
27+
closeTimer.current = null
28+
}
29+
}
30+
}, [])
31+
32+
const handlePopoverOpenChange = (open: boolean) => {
33+
setIsPopoverOpen(open)
34+
if (open) {
35+
setIsClosing(false)
36+
if (closeTimer.current) {
37+
window.clearTimeout(closeTimer.current)
38+
closeTimer.current = null
39+
}
40+
} else {
41+
setIsClosing(true)
42+
closeTimer.current = window.setTimeout(() => {
43+
setIsClosing(false)
44+
closeTimer.current = null
45+
}, 200) // keep menu visible briefly to avoid popover jump
46+
}
47+
}
48+
49+
const menuVisible = isPopoverOpen || isClosing
1850

1951
const metadata = useMemo(() => {
2052
if (!checkpoint) {
@@ -48,8 +80,16 @@ export const CheckpointSaved = ({ checkpoint, ...props }: CheckpointSavedProps)
4880
"linear-gradient(90deg, rgba(0, 188, 255, .65), rgba(0, 188, 255, .65) 80%, rgba(0, 188, 255, 0) 99%)",
4981
}}></span>
5082

51-
<div className="hidden group-hover:block h-4 -mt-2">
52-
<CheckpointMenu {...props} checkpoint={metadata} />
83+
{/* Keep menu visible while popover is open or briefly after close to prevent jump */}
84+
<div
85+
data-testid="checkpoint-menu-container"
86+
className={cn("h-4 -mt-2", menuVisible ? "block" : "hidden group-hover:block")}>
87+
<CheckpointMenu
88+
{...props}
89+
checkpoint={metadata}
90+
open={isPopoverOpen}
91+
onOpenChange={handlePopoverOpenChange}
92+
/>
5393
</div>
5494
</div>
5595
)

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

Lines changed: 141 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,141 @@
1+
// npx vitest run src/components/chat/checkpoints/__tests__/CheckpointSaved.spec.tsx
2+
3+
vi.mock("@/components/ui", () => {
4+
// Minimal UI primitives to ensure deterministic behavior in tests
5+
return {
6+
Button: ({ children, ...rest }: any) => <button {...rest}>{children}</button>,
7+
StandardTooltip: ({ children }: any) => <>{children}</>,
8+
Popover: ({ children, onOpenChange, open }: any) => {
9+
lastOnOpenChange = onOpenChange
10+
return (
11+
<div data-testid="popover-root" data-open={open}>
12+
{children}
13+
</div>
14+
)
15+
},
16+
PopoverTrigger: ({ children }: any) => <div data-testid="popover-trigger">{children}</div>,
17+
PopoverContent: ({ children }: any) => <div data-testid="popover-content">{children}</div>,
18+
}
19+
})
20+
21+
import { render, waitFor, screen } from "@/utils/test-utils"
22+
import React from "react"
23+
import userEvent from "@testing-library/user-event"
24+
import { CheckpointSaved } from "../CheckpointSaved"
25+
26+
// Capture onOpenChange from Popover to control open/close in tests
27+
let lastOnOpenChange: ((open: boolean) => void) | undefined
28+
29+
const waitForOpenHandler = async () => {
30+
await waitFor(() => {
31+
// ensure Popover mock captured the onOpenChange handler before using it
32+
expect(lastOnOpenChange).toBeTruthy()
33+
})
34+
}
35+
36+
describe("CheckpointSaved popover visibility", () => {
37+
// Timers are controlled per-test to avoid interfering with i18n init
38+
const baseProps = {
39+
ts: 123,
40+
commitHash: "abc123",
41+
currentHash: "zzz999",
42+
checkpoint: { from: "prev123", to: "abc123" } as Record<string, unknown>,
43+
}
44+
45+
it("shows menu while popover is open and hides when closed", async () => {
46+
const { getByTestId } = render(<CheckpointSaved {...baseProps} />)
47+
48+
const getMenu = () => getByTestId("checkpoint-menu-container") as HTMLElement
49+
50+
// Initially hidden (relies on group-hover)
51+
expect(getMenu()).toBeTruthy()
52+
expect(getMenu().className).toContain("hidden")
53+
54+
// Open via captured handler
55+
await waitForOpenHandler()
56+
lastOnOpenChange?.(true)
57+
58+
await waitFor(() => {
59+
expect(getMenu().className).toContain("block")
60+
expect(getMenu().className).not.toContain("hidden")
61+
})
62+
63+
// Close via captured handler — menu remains visible briefly, then hides
64+
lastOnOpenChange?.(false)
65+
66+
await waitFor(() => {
67+
expect(getMenu().className).toContain("block")
68+
})
69+
70+
await waitFor(() => {
71+
expect(getMenu().className).toContain("hidden")
72+
})
73+
})
74+
75+
it("resets confirm state when popover closes", async () => {
76+
const { getByTestId } = render(<CheckpointSaved {...baseProps} />)
77+
78+
// Open the popover
79+
await waitForOpenHandler()
80+
lastOnOpenChange?.(true)
81+
82+
// Enter confirm state
83+
const restoreFilesAndTaskBtn = await waitFor(() => getByTestId("restore-files-and-task-btn"))
84+
await userEvent.click(restoreFilesAndTaskBtn)
85+
86+
// Confirm warning should be visible
87+
expect(getByTestId("checkpoint-confirm-warning")).toBeTruthy()
88+
89+
// Close popover -> confirm state should reset
90+
lastOnOpenChange?.(false)
91+
92+
// Reopen
93+
lastOnOpenChange?.(true)
94+
95+
// Confirm warning should be gone after reopening
96+
await waitFor(() => {
97+
expect(screen.queryByTestId("checkpoint-confirm-warning")).toBeNull()
98+
})
99+
})
100+
101+
it("closes popover after preview and after confirm restore", async () => {
102+
const { getByTestId } = render(<CheckpointSaved {...baseProps} />)
103+
104+
const popoverRoot = () => getByTestId("popover-root")
105+
const menuContainer = () => getByTestId("checkpoint-menu-container")
106+
107+
// Open
108+
await waitForOpenHandler()
109+
lastOnOpenChange?.(true)
110+
await waitFor(() => {
111+
expect(popoverRoot().getAttribute("data-open")).toBe("true")
112+
expect(menuContainer().className).toContain("block")
113+
})
114+
115+
// Click preview -> popover closes; menu remains briefly visible, then hides
116+
await userEvent.click(getByTestId("restore-files-btn"))
117+
await waitFor(() => {
118+
expect(popoverRoot().getAttribute("data-open")).toBe("false")
119+
expect(menuContainer().className).toContain("block")
120+
})
121+
await waitFor(() => {
122+
expect(menuContainer().className).toContain("hidden")
123+
})
124+
125+
// Reopen
126+
lastOnOpenChange?.(true)
127+
await waitFor(() => {
128+
expect(popoverRoot().getAttribute("data-open")).toBe("true")
129+
})
130+
131+
// Enter confirm and confirm restore -> popover closes; menu then hides
132+
await userEvent.click(getByTestId("restore-files-and-task-btn"))
133+
await userEvent.click(getByTestId("confirm-restore-btn"))
134+
await waitFor(() => {
135+
expect(popoverRoot().getAttribute("data-open")).toBe("false")
136+
})
137+
await waitFor(() => {
138+
expect(menuContainer().className).toContain("hidden")
139+
})
140+
})
141+
})

0 commit comments

Comments
 (0)