Skip to content

Commit 1e119d6

Browse files
NaccOllNaccOll
committed
fix: popover show error when CheckpointMenu has multiple popover
1 parent 7d5a21c commit 1e119d6

File tree

3 files changed

+93
-81
lines changed

3 files changed

+93
-81
lines changed

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

Lines changed: 82 additions & 68 deletions
Original file line numberDiff line numberDiff line change
@@ -15,35 +15,45 @@ type CheckpointMenuBaseProps = {
1515
checkpoint: Checkpoint
1616
}
1717
type CheckpointMenuControlledProps = {
18-
open: boolean
1918
onOpenChange: (open: boolean) => void
2019
}
2120
type CheckpointMenuUncontrolledProps = {
22-
open?: undefined
2321
onOpenChange?: undefined
2422
}
2523
type CheckpointMenuProps = CheckpointMenuBaseProps & (CheckpointMenuControlledProps | CheckpointMenuUncontrolledProps)
2624

27-
export const CheckpointMenu = ({
28-
ts,
29-
commitHash,
30-
currentHash,
31-
checkpoint,
32-
open,
33-
onOpenChange,
34-
}: CheckpointMenuProps) => {
25+
export const CheckpointMenu = ({ ts, commitHash, currentHash, checkpoint, onOpenChange }: CheckpointMenuProps) => {
3526
const { t } = useTranslation()
36-
const [internalOpen, setInternalOpen] = useState(false)
37-
const [isConfirming, setIsConfirming] = useState(false)
38-
const [isDiffOpen, setIsDiffOpen] = useState(false)
27+
const [internalRestoreOpen, setInternalRestoreOpen] = useState(false)
28+
const [restoreConfirming, setRestoreIsConfirming] = useState(false)
29+
const [internalMoreOpen, setInternalMoreOpen] = useState(false)
3930
const portalContainer = useRooPortal("roo-portal")
4031

4132
const isCurrent = currentHash === commitHash
4233

4334
const previousCommitHash = checkpoint?.from
4435

45-
const isOpen = open ?? internalOpen
46-
const setOpen = onOpenChange ?? setInternalOpen
36+
const restoreOpen = internalRestoreOpen
37+
const moreOpen = internalMoreOpen
38+
const setRestoreOpen = useCallback(
39+
(open: boolean) => {
40+
setInternalRestoreOpen(open)
41+
if (onOpenChange) {
42+
onOpenChange(open)
43+
}
44+
},
45+
[onOpenChange],
46+
)
47+
48+
const setMoreOpen = useCallback(
49+
(open: boolean) => {
50+
setInternalMoreOpen(open)
51+
if (onOpenChange) {
52+
onOpenChange(open)
53+
}
54+
},
55+
[onOpenChange],
56+
)
4757

4858
const onCheckpointDiff = useCallback(() => {
4959
vscode.postMessage({
@@ -68,22 +78,22 @@ export const CheckpointMenu = ({
6878

6979
const onPreview = useCallback(() => {
7080
vscode.postMessage({ type: "checkpointRestore", payload: { ts, commitHash, mode: "preview" } })
71-
setOpen(false)
72-
}, [ts, commitHash, setOpen])
81+
setRestoreOpen(false)
82+
}, [ts, commitHash, setRestoreOpen])
7383

7484
const onRestore = useCallback(() => {
7585
vscode.postMessage({ type: "checkpointRestore", payload: { ts, commitHash, mode: "restore" } })
76-
setOpen(false)
77-
}, [ts, commitHash, setOpen])
86+
setRestoreOpen(false)
87+
}, [ts, commitHash, setRestoreOpen])
7888

7989
const handleOpenChange = useCallback(
8090
(open: boolean) => {
81-
setOpen(open)
91+
setRestoreOpen(open)
8292
if (!open) {
83-
setIsConfirming(false)
93+
setRestoreIsConfirming(false)
8494
}
8595
},
86-
[setOpen],
96+
[setRestoreOpen],
8797
)
8898

8999
return (
@@ -93,7 +103,13 @@ export const CheckpointMenu = ({
93103
<span className="codicon codicon-diff-single" />
94104
</Button>
95105
</StandardTooltip>
96-
<Popover open={isOpen} onOpenChange={handleOpenChange}>
106+
<Popover
107+
open={restoreOpen}
108+
onOpenChange={(open) => {
109+
handleOpenChange(open)
110+
setRestoreIsConfirming(false)
111+
}}
112+
data-testid="restore-popover">
97113
<StandardTooltip content={t("chat:checkpoint.menu.restore")}>
98114
<PopoverTrigger asChild>
99115
<Button variant="ghost" size="icon" aria-label={t("chat:checkpoint.menu.restore")}>
@@ -113,54 +129,52 @@ export const CheckpointMenu = ({
113129
</div>
114130
</div>
115131
)}
116-
{!isCurrent && (
132+
<div className="flex flex-col gap-1 group hover:text-foreground">
117133
<div className="flex flex-col gap-1 group hover:text-foreground">
118-
<div className="flex flex-col gap-1 group hover:text-foreground">
119-
{!isConfirming ? (
134+
{!restoreConfirming ? (
135+
<Button
136+
variant="secondary"
137+
onClick={() => setRestoreIsConfirming(true)}
138+
data-testid="restore-files-and-task-btn">
139+
{t("chat:checkpoint.menu.restoreFilesAndTask")}
140+
</Button>
141+
) : (
142+
<>
120143
<Button
121-
variant="secondary"
122-
onClick={() => setIsConfirming(true)}
123-
data-testid="restore-files-and-task-btn">
124-
{t("chat:checkpoint.menu.restoreFilesAndTask")}
144+
variant="default"
145+
onClick={onRestore}
146+
className="grow"
147+
data-testid="confirm-restore-btn">
148+
<div className="flex flex-row gap-1">
149+
<CheckIcon />
150+
<div>{t("chat:checkpoint.menu.confirm")}</div>
151+
</div>
125152
</Button>
126-
) : (
127-
<>
128-
<Button
129-
variant="default"
130-
onClick={onRestore}
131-
className="grow"
132-
data-testid="confirm-restore-btn">
133-
<div className="flex flex-row gap-1">
134-
<CheckIcon />
135-
<div>{t("chat:checkpoint.menu.confirm")}</div>
136-
</div>
137-
</Button>
138-
<Button variant="secondary" onClick={() => setIsConfirming(false)}>
139-
<div className="flex flex-row gap-1">
140-
<Cross2Icon />
141-
<div>{t("chat:checkpoint.menu.cancel")}</div>
142-
</div>
143-
</Button>
144-
</>
145-
)}
146-
{isConfirming ? (
147-
<div
148-
data-testid="checkpoint-confirm-warning"
149-
className="text-destructive font-bold">
150-
{t("chat:checkpoint.menu.cannotUndo")}
151-
</div>
152-
) : (
153-
<div className="text-muted transition-colors group-hover:text-foreground">
154-
{t("chat:checkpoint.menu.restoreFilesAndTaskDescription")}
155-
</div>
156-
)}
157-
</div>
153+
<Button variant="secondary" onClick={() => setRestoreIsConfirming(false)}>
154+
<div className="flex flex-row gap-1">
155+
<Cross2Icon />
156+
<div>{t("chat:checkpoint.menu.cancel")}</div>
157+
</div>
158+
</Button>
159+
</>
160+
)}
161+
{restoreConfirming ? (
162+
<div
163+
data-testid="checkpoint-confirm-warning"
164+
className="text-destructive font-bold">
165+
{t("chat:checkpoint.menu.cannotUndo")}
166+
</div>
167+
) : (
168+
<div className="text-muted transition-colors group-hover:text-foreground">
169+
{t("chat:checkpoint.menu.restoreFilesAndTaskDescription")}
170+
</div>
171+
)}
158172
</div>
159-
)}
173+
</div>
160174
</div>
161175
</PopoverContent>
162176
</Popover>
163-
<Popover open={isDiffOpen} onOpenChange={(open) => setIsDiffOpen(open)}>
177+
<Popover open={moreOpen} onOpenChange={(open) => setMoreOpen(open)} data-testid="more-popover">
164178
<StandardTooltip content={t("chat:task.seeMore")}>
165179
<PopoverTrigger asChild>
166180
<Button variant="ghost" size="icon" aria-label={t("chat:checkpoint.menu.more")}>
@@ -171,19 +185,19 @@ export const CheckpointMenu = ({
171185
<PopoverContent align="end" container={portalContainer}>
172186
<div className="flex flex-col gap-2">
173187
<Button
174-
variant="ghost"
188+
variant="secondary"
175189
onClick={() => {
176190
onDiffFromInit()
177-
setIsDiffOpen(false)
191+
setMoreOpen(false)
178192
}}>
179193
<span className="codicon codicon-versions mr-2" />
180194
{t("chat:checkpoint.menu.viewDiffFromInit")}
181195
</Button>
182196
<Button
183-
variant="ghost"
197+
variant="secondary"
184198
onClick={() => {
185199
onDiffWithCurrent()
186-
setIsDiffOpen(false)
200+
setMoreOpen(false)
187201
}}>
188202
<span className="codicon codicon-diff mr-2" />
189203
{t("chat:checkpoint.menu.viewDiffWithCurrent")}

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

Lines changed: 1 addition & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -84,12 +84,7 @@ export const CheckpointSaved = ({ checkpoint, ...props }: CheckpointSavedProps)
8484
<div
8585
data-testid="checkpoint-menu-container"
8686
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-
/>
87+
<CheckpointMenu {...props} checkpoint={metadata} onOpenChange={handlePopoverOpenChange} />
9388
</div>
9489
</div>
9590
)

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

Lines changed: 10 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,14 +1,20 @@
11
// npx vitest run src/components/chat/checkpoints/__tests__/CheckpointSaved.spec.tsx
22

3+
// Capture onOpenChange from Popover to control open/close in tests
4+
let lastOnOpenChange: ((open: boolean) => void) | undefined
5+
36
vi.mock("@/components/ui", () => {
47
// Minimal UI primitives to ensure deterministic behavior in tests
58
return {
69
Button: ({ children, ...rest }: any) => <button {...rest}>{children}</button>,
710
StandardTooltip: ({ children }: any) => <>{children}</>,
8-
Popover: ({ children, onOpenChange, open }: any) => {
9-
lastOnOpenChange = onOpenChange
11+
Popover: (props: any) => {
12+
const { children, onOpenChange, open, ...rest } = props
13+
if (rest["data-testid"] === "restore-popover") {
14+
lastOnOpenChange = onOpenChange
15+
}
1016
return (
11-
<div data-testid="popover-root" data-open={open}>
17+
<div data-testid={rest["data-testid"]} data-open={open}>
1218
{children}
1319
</div>
1420
)
@@ -23,9 +29,6 @@ import React from "react"
2329
import userEvent from "@testing-library/user-event"
2430
import { CheckpointSaved } from "../CheckpointSaved"
2531

26-
// Capture onOpenChange from Popover to control open/close in tests
27-
let lastOnOpenChange: ((open: boolean) => void) | undefined
28-
2932
const waitForOpenHandler = async () => {
3033
await waitFor(() => {
3134
// ensure Popover mock captured the onOpenChange handler before using it
@@ -101,7 +104,7 @@ describe("CheckpointSaved popover visibility", () => {
101104
it("closes popover after preview and after confirm restore", async () => {
102105
const { getByTestId } = render(<CheckpointSaved {...baseProps} />)
103106

104-
const popoverRoot = () => getByTestId("popover-root")
107+
const popoverRoot = () => getByTestId("restore-popover")
105108
const menuContainer = () => getByTestId("checkpoint-menu-container")
106109

107110
// Open

0 commit comments

Comments
 (0)