Skip to content

Commit b7986c0

Browse files
hannesrudolphhannesrudolph
committed
fix: address PR review comments and improve command permission UI
- Add comprehensive test coverage for removing command patterns - Fix hardcoded strings with proper i18n translations - Improve tooltips with clearer descriptions and settings link - Update button tooltips to say 'Add to allowed list' etc - Fix failing tests after UI changes
1 parent e98c404 commit b7986c0

File tree

4 files changed

+356
-50
lines changed

4 files changed

+356
-50
lines changed

‎src/core/webview/__tests__/webviewMessageHandler.allowDeny.spec.ts‎

Lines changed: 250 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,250 @@
1+
import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"
2+
import * as vscode from "vscode"
3+
import { webviewMessageHandler } from "../webviewMessageHandler"
4+
import { ClineProvider } from "../ClineProvider"
5+
import { Package } from "../../../shared/package"
6+
7+
// Mock vscode module
8+
vi.mock("vscode", () => ({
9+
window: {
10+
showInformationMessage: vi.fn(),
11+
showErrorMessage: vi.fn(),
12+
},
13+
workspace: {
14+
getConfiguration: vi.fn(),
15+
},
16+
ConfigurationTarget: {
17+
Global: 1,
18+
},
19+
}))
20+
21+
// Mock Package
22+
vi.mock("../../../shared/package", () => ({
23+
Package: {
24+
name: "roo-code",
25+
},
26+
}))
27+
28+
describe("webviewMessageHandler - allowedCommands and deniedCommands", () => {
29+
let mockProvider: any
30+
let mockContextProxy: any
31+
let mockConfigUpdate: any
32+
33+
beforeEach(() => {
34+
vi.clearAllMocks()
35+
36+
// Setup mock for workspace configuration
37+
mockConfigUpdate = vi.fn().mockResolvedValue(undefined)
38+
vi.mocked(vscode.workspace.getConfiguration).mockReturnValue({
39+
update: mockConfigUpdate,
40+
} as any)
41+
42+
// Create mock context proxy
43+
mockContextProxy = {
44+
getValue: vi.fn(),
45+
setValue: vi.fn(),
46+
}
47+
48+
// Create mock provider
49+
mockProvider = {
50+
contextProxy: mockContextProxy,
51+
postStateToWebview: vi.fn().mockResolvedValue(undefined),
52+
log: vi.fn(),
53+
} as any
54+
})
55+
56+
afterEach(() => {
57+
vi.restoreAllMocks()
58+
})
59+
60+
describe("allowedCommands", () => {
61+
it("should update the allowed commands list", async () => {
62+
// Create message with new allowed commands
63+
const message = {
64+
type: "allowedCommands",
65+
commands: ["npm test", "git status", "npm run build"],
66+
}
67+
68+
// Call handler
69+
await webviewMessageHandler(mockProvider, message as any)
70+
71+
// Verify the commands were updated
72+
expect(mockContextProxy.setValue).toHaveBeenCalledWith("allowedCommands", [
73+
"npm test",
74+
"git status",
75+
"npm run build",
76+
])
77+
78+
// Verify workspace settings were updated
79+
expect(vscode.workspace.getConfiguration).toHaveBeenCalledWith("roo-code")
80+
expect(mockConfigUpdate).toHaveBeenCalledWith(
81+
"allowedCommands",
82+
["npm test", "git status", "npm run build"],
83+
vscode.ConfigurationTarget.Global,
84+
)
85+
86+
// Note: The actual implementation doesn't call postStateToWebview for these messages
87+
})
88+
89+
it("should handle removing patterns from allowed commands", async () => {
90+
// Setup initial state
91+
mockContextProxy.getValue.mockReturnValue(["npm test", "git status", "npm run build"])
92+
93+
// Create message that removes "git status"
94+
const message = {
95+
type: "allowedCommands",
96+
commands: ["npm test", "npm run build"],
97+
}
98+
99+
// Call handler
100+
await webviewMessageHandler(mockProvider, message as any)
101+
102+
// Verify the pattern was removed
103+
expect(mockContextProxy.setValue).toHaveBeenCalledWith("allowedCommands", ["npm test", "npm run build"])
104+
105+
// Verify workspace settings were updated
106+
expect(mockConfigUpdate).toHaveBeenCalledWith(
107+
"allowedCommands",
108+
["npm test", "npm run build"],
109+
vscode.ConfigurationTarget.Global,
110+
)
111+
})
112+
113+
it("should handle empty allowed commands list", async () => {
114+
// Create message with empty commands
115+
const message = {
116+
type: "allowedCommands",
117+
commands: [],
118+
}
119+
120+
// Call handler
121+
await webviewMessageHandler(mockProvider, message as any)
122+
123+
// Verify the commands were cleared
124+
expect(mockContextProxy.setValue).toHaveBeenCalledWith("allowedCommands", [])
125+
126+
// Verify workspace settings were updated
127+
expect(mockConfigUpdate).toHaveBeenCalledWith("allowedCommands", [], vscode.ConfigurationTarget.Global)
128+
})
129+
130+
it("should filter out invalid commands", async () => {
131+
// Create message with some invalid commands
132+
const message = {
133+
type: "allowedCommands",
134+
commands: ["npm test", "", " ", null, undefined, 123, "git status"],
135+
}
136+
137+
// Call handler
138+
await webviewMessageHandler(mockProvider, message as any)
139+
140+
// Verify only valid commands were kept
141+
expect(mockContextProxy.setValue).toHaveBeenCalledWith("allowedCommands", ["npm test", "git status"])
142+
})
143+
})
144+
145+
describe("deniedCommands", () => {
146+
it("should update the denied commands list", async () => {
147+
// Create message with new denied commands
148+
const message = {
149+
type: "deniedCommands",
150+
commands: ["rm -rf", "sudo", "chmod 777"],
151+
}
152+
153+
// Call handler
154+
await webviewMessageHandler(mockProvider, message as any)
155+
156+
// Verify the commands were updated
157+
expect(mockContextProxy.setValue).toHaveBeenCalledWith("deniedCommands", ["rm -rf", "sudo", "chmod 777"])
158+
159+
// Verify workspace settings were updated
160+
expect(vscode.workspace.getConfiguration).toHaveBeenCalledWith("roo-code")
161+
expect(mockConfigUpdate).toHaveBeenCalledWith(
162+
"deniedCommands",
163+
["rm -rf", "sudo", "chmod 777"],
164+
vscode.ConfigurationTarget.Global,
165+
)
166+
167+
// Note: The actual implementation doesn't call postStateToWebview for these messages
168+
})
169+
170+
it("should handle removing patterns from denied commands", async () => {
171+
// Setup initial state
172+
mockContextProxy.getValue.mockReturnValue(["rm -rf", "sudo", "chmod 777"])
173+
174+
// Create message that removes "sudo"
175+
const message = {
176+
type: "deniedCommands",
177+
commands: ["rm -rf", "chmod 777"],
178+
}
179+
180+
// Call handler
181+
await webviewMessageHandler(mockProvider, message as any)
182+
183+
// Verify the pattern was removed
184+
expect(mockContextProxy.setValue).toHaveBeenCalledWith("deniedCommands", ["rm -rf", "chmod 777"])
185+
186+
// Verify workspace settings were updated
187+
expect(mockConfigUpdate).toHaveBeenCalledWith(
188+
"deniedCommands",
189+
["rm -rf", "chmod 777"],
190+
vscode.ConfigurationTarget.Global,
191+
)
192+
})
193+
194+
it("should handle empty denied commands list", async () => {
195+
// Create message with empty commands
196+
const message = {
197+
type: "deniedCommands",
198+
commands: [],
199+
}
200+
201+
// Call handler
202+
await webviewMessageHandler(mockProvider, message as any)
203+
204+
// Verify the commands were cleared
205+
expect(mockContextProxy.setValue).toHaveBeenCalledWith("deniedCommands", [])
206+
207+
// Verify workspace settings were updated
208+
expect(mockConfigUpdate).toHaveBeenCalledWith("deniedCommands", [], vscode.ConfigurationTarget.Global)
209+
})
210+
211+
it("should filter out invalid commands", async () => {
212+
// Create message with some invalid commands
213+
const message = {
214+
type: "deniedCommands",
215+
commands: ["rm -rf", "", " ", null, undefined, false, "sudo"],
216+
}
217+
218+
// Call handler
219+
await webviewMessageHandler(mockProvider, message as any)
220+
221+
// Verify only valid commands were kept
222+
expect(mockContextProxy.setValue).toHaveBeenCalledWith("deniedCommands", ["rm -rf", "sudo"])
223+
})
224+
})
225+
226+
describe("interaction between allowed and denied commands", () => {
227+
it("should handle switching a command from allowed to denied", async () => {
228+
// First, set up allowed commands
229+
await webviewMessageHandler(mockProvider, {
230+
type: "allowedCommands",
231+
commands: ["npm test", "git status"],
232+
} as any)
233+
234+
// Then move "git status" to denied
235+
await webviewMessageHandler(mockProvider, {
236+
type: "allowedCommands",
237+
commands: ["npm test"],
238+
} as any)
239+
240+
await webviewMessageHandler(mockProvider, {
241+
type: "deniedCommands",
242+
commands: ["git status"],
243+
} as any)
244+
245+
// Verify both lists were updated correctly
246+
expect(mockContextProxy.setValue).toHaveBeenCalledWith("allowedCommands", ["npm test"])
247+
expect(mockContextProxy.setValue).toHaveBeenCalledWith("deniedCommands", ["git status"])
248+
})
249+
})
250+
})

‎webview-ui/src/components/chat/CommandPatternSelector.tsx‎

Lines changed: 37 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,10 @@
11
import { useState } from "react"
22
import { ChevronDown, Check, X } from "lucide-react"
3+
import { Trans } from "react-i18next"
34
import { useAppTranslation } from "@src/i18n/TranslationContext"
45
import { cn } from "@src/lib/utils"
56
import { StandardTooltip } from "@src/components/ui/standard-tooltip"
7+
import { VSCodeLink } from "@vscode/webview-ui-toolkit/react"
68

79
interface CommandPattern {
810
pattern: string
@@ -62,7 +64,31 @@ export const CommandPatternSelector = ({
6264
/>
6365
<span className="font-medium">{t("chat:commandExecution.manageCommands")}</span>
6466
{isExpanded && (
65-
<StandardTooltip content={t("chat:commandExecution.commandManagementDescription")}>
67+
<StandardTooltip
68+
content={
69+
<Trans
70+
i18nKey="chat:commandExecution.commandManagementDescription"
71+
components={{
72+
settingsLink: (
73+
<VSCodeLink
74+
href="#"
75+
onClick={(e) => {
76+
e.preventDefault()
77+
window.postMessage(
78+
{
79+
type: "action",
80+
action: "settingsButtonClicked",
81+
values: { section: "autoApprove" },
82+
},
83+
"*",
84+
)
85+
}}
86+
className="inline"
87+
/>
88+
),
89+
}}
90+
/>
91+
}>
6692
<i
6793
className="codicon codicon-info text-vscode-descriptionForeground ml-1"
6894
style={{ fontSize: "12px" }}
@@ -86,7 +112,11 @@ export const CommandPatternSelector = ({
86112
</div>
87113
<div className="flex items-center gap-1">
88114
<StandardTooltip
89-
content={status === "allowed" ? "Remove from allowed" : "Add to allowed"}>
115+
content={
116+
status === "allowed"
117+
? t("chat:commandExecution.removeFromAllowed")
118+
: t("chat:commandExecution.addToAllowed")
119+
}>
90120
<button
91121
onClick={() => handleAllowClick(item.pattern)}
92122
className={cn(
@@ -104,7 +134,11 @@ export const CommandPatternSelector = ({
104134
</button>
105135
</StandardTooltip>
106136
<StandardTooltip
107-
content={status === "denied" ? "Remove from denied" : "Add to denied"}>
137+
content={
138+
status === "denied"
139+
? t("chat:commandExecution.removeFromDenied")
140+
: t("chat:commandExecution.addToDenied")
141+
}>
108142
<button
109143
onClick={() => handleDenyClick(item.pattern)}
110144
className={cn(

0 commit comments

Comments
 (0)