Skip to content

Commit 51059b9

Browse files
daniel-lxsdaniel-lxs
committed
feat(checkpoints): enhance popover behavior and visibility logic in CheckpointSaved component
1 parent c606003 commit 51059b9

File tree

3 files changed

+195
-68
lines changed

3 files changed

+195
-68
lines changed

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

Lines changed: 56 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -8,14 +8,21 @@ 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
16-
open?: boolean
17-
onOpenChange?: (open: boolean) => void
1816
}
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)
1926

2027
export const CheckpointMenu = ({
2128
ts,
@@ -57,7 +64,9 @@ export const CheckpointMenu = ({
5764
const handleOpenChange = useCallback(
5865
(open: boolean) => {
5966
setOpen(open)
60-
setIsConfirming(false)
67+
if (!open) {
68+
setIsConfirming(false)
69+
}
6170
},
6271
[setOpen],
6372
)
@@ -72,7 +81,7 @@ export const CheckpointMenu = ({
7281
<Popover open={isOpen} onOpenChange={handleOpenChange}>
7382
<StandardTooltip content={t("chat:checkpoint.menu.restore")}>
7483
<PopoverTrigger asChild>
75-
<Button variant="ghost" size="icon">
84+
<Button variant="ghost" size="icon" aria-label={t("chat:checkpoint.menu.restore")}>
7685
<span className="codicon codicon-history" />
7786
</Button>
7887
</PopoverTrigger>
@@ -81,47 +90,58 @@ export const CheckpointMenu = ({
8190
<div className="flex flex-col gap-2">
8291
{!isCurrent && (
8392
<div className="flex flex-col gap-1 group hover:text-foreground">
84-
<Button variant="secondary" onClick={onPreview}>
93+
<Button variant="secondary" onClick={onPreview} data-testid="restore-files-btn">
8594
{t("chat:checkpoint.menu.restoreFiles")}
8695
</Button>
8796
<div className="text-muted transition-colors group-hover:text-foreground">
8897
{t("chat:checkpoint.menu.restoreFilesDescription")}
8998
</div>
9099
</div>
91100
)}
92-
<div className="flex flex-col gap-1 group hover:text-foreground">
101+
{!isCurrent && (
93102
<div className="flex flex-col gap-1 group hover:text-foreground">
94-
{!isConfirming ? (
95-
<Button variant="secondary" onClick={() => setIsConfirming(true)}>
96-
{t("chat:checkpoint.menu.restoreFilesAndTask")}
97-
</Button>
98-
) : (
99-
<>
100-
<Button variant="default" onClick={onRestore} className="grow">
101-
<div className="flex flex-row gap-1">
102-
<CheckIcon />
103-
<div>{t("chat:checkpoint.menu.confirm")}</div>
104-
</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")}
105110
</Button>
106-
<Button variant="secondary" onClick={() => setIsConfirming(false)}>
107-
<div className="flex flex-row gap-1">
108-
<Cross2Icon />
109-
<div>{t("chat:checkpoint.menu.cancel")}</div>
110-
</div>
111-
</Button>
112-
</>
113-
)}
114-
{isConfirming ? (
115-
<div className="text-destructive font-bold">
116-
{t("chat:checkpoint.menu.cannotUndo")}
117-
</div>
118-
) : (
119-
<div className="text-muted transition-colors group-hover:text-foreground">
120-
{t("chat:checkpoint.menu.restoreFilesAndTaskDescription")}
121-
</div>
122-
)}
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>
123143
</div>
124-
</div>
144+
)}
125145
</div>
126146
</PopoverContent>
127147
</Popover>

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

Lines changed: 41 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import { useMemo, useState } from "react"
1+
import { useMemo, useRef, useState, useEffect } from "react"
22
import { useTranslation } from "react-i18next"
33
import { cn } from "@/lib/utils"
44

@@ -17,6 +17,36 @@ export const CheckpointSaved = ({ checkpoint, ...props }: CheckpointSavedProps)
1717
const { t } = useTranslation()
1818
const isCurrent = props.currentHash === props.commitHash
1919
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
2050

2151
const metadata = useMemo(() => {
2252
if (!checkpoint) {
@@ -50,9 +80,16 @@ export const CheckpointSaved = ({ checkpoint, ...props }: CheckpointSavedProps)
5080
"linear-gradient(90deg, rgba(0, 188, 255, .65), rgba(0, 188, 255, .65) 80%, rgba(0, 188, 255, 0) 99%)",
5181
}}></span>
5282

53-
{/* Keep menu visible while popover is open; otherwise rely on group-hover */}
54-
<div className={cn("h-4 -mt-2", isPopoverOpen ? "block" : "hidden group-hover:block")}>
55-
<CheckpointMenu {...props} checkpoint={metadata} open={isPopoverOpen} onOpenChange={setIsPopoverOpen} />
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+
/>
5693
</div>
5794
</div>
5895
)

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

Lines changed: 98 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -1,42 +1,40 @@
11
// npx vitest run src/components/chat/checkpoints/__tests__/CheckpointSaved.spec.tsx
22

3-
import { render, waitFor } from "@/utils/test-utils"
4-
import React from "react"
5-
import { CheckpointSaved } from "../CheckpointSaved"
6-
7-
// Capture onOpenChange from Popover to control open/close in tests
8-
let lastOnOpenChange: ((open: boolean) => void) | undefined
9-
10-
vi.mock("@/components/ui", async () => {
11-
const actual = await vi.importActual<any>("@/components/ui")
3+
vi.mock("@/components/ui", () => {
4+
// Minimal UI primitives to ensure deterministic behavior in tests
125
return {
13-
...actual,
14-
Popover: ({
15-
children,
16-
onOpenChange,
17-
open,
18-
}: {
19-
children: React.ReactNode
20-
open?: boolean
21-
onOpenChange?: (open: boolean) => void
22-
}) => {
6+
Button: ({ children, ...rest }: any) => <button {...rest}>{children}</button>,
7+
StandardTooltip: ({ children }: any) => <>{children}</>,
8+
Popover: ({ children, onOpenChange, open }: any) => {
239
lastOnOpenChange = onOpenChange
2410
return (
2511
<div data-testid="popover-root" data-open={open}>
2612
{children}
2713
</div>
2814
)
2915
},
30-
PopoverTrigger: ({ children }: { children: React.ReactNode }) => (
31-
<div data-testid="popover-trigger">{children}</div>
32-
),
33-
PopoverContent: ({ children }: { children: React.ReactNode }) => (
34-
<div data-testid="popover-content">{children}</div>
35-
),
16+
PopoverTrigger: ({ children }: any) => <div data-testid="popover-trigger">{children}</div>,
17+
PopoverContent: ({ children }: any) => <div data-testid="popover-content">{children}</div>,
3618
}
3719
})
3820

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+
3936
describe("CheckpointSaved popover visibility", () => {
37+
// Timers are controlled per-test to avoid interfering with i18n init
4038
const baseProps = {
4139
ts: 123,
4240
commitHash: "abc123",
@@ -45,27 +43,99 @@ describe("CheckpointSaved popover visibility", () => {
4543
}
4644

4745
it("shows menu while popover is open and hides when closed", async () => {
48-
const { container } = render(<CheckpointSaved {...baseProps} />)
46+
const { getByTestId } = render(<CheckpointSaved {...baseProps} />)
4947

50-
const getMenu = () => container.querySelector("div.h-4.-mt-2") as HTMLElement
48+
const getMenu = () => getByTestId("checkpoint-menu-container") as HTMLElement
5149

5250
// Initially hidden (relies on group-hover)
5351
expect(getMenu()).toBeTruthy()
5452
expect(getMenu().className).toContain("hidden")
5553

5654
// Open via captured handler
55+
await waitForOpenHandler()
5756
lastOnOpenChange?.(true)
5857

5958
await waitFor(() => {
6059
expect(getMenu().className).toContain("block")
6160
expect(getMenu().className).not.toContain("hidden")
6261
})
6362

64-
// Close via captured handler
63+
// Close via captured handler — menu remains visible briefly, then hides
6564
lastOnOpenChange?.(false)
6665

66+
await waitFor(() => {
67+
expect(getMenu().className).toContain("block")
68+
})
69+
6770
await waitFor(() => {
6871
expect(getMenu().className).toContain("hidden")
6972
})
7073
})
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+
})
71141
})

0 commit comments

Comments
 (0)