Skip to content

Fix the UI for approving chained commands #6623

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Aug 3, 2025
Merged

Fix the UI for approving chained commands #6623

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion webview-ui/src/components/chat/CommandExecution.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -192,7 +192,6 @@ export const CommandExecution = ({ executionId, text, icon, title }: CommandExec
</div>
{command && command.trim() && (
<CommandPatternSelector
command={command}
patterns={commandPatterns}
allowedCommands={allowedCommands}
deniedCommands={deniedCommands}
Expand Down
11 changes: 2 additions & 9 deletions webview-ui/src/components/chat/CommandPatternSelector.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@ interface CommandPattern {
}

interface CommandPatternSelectorProps {
command: string
patterns: CommandPattern[]
allowedCommands: string[]
deniedCommands: string[]
Expand All @@ -20,7 +19,6 @@ interface CommandPatternSelectorProps {
}

export const CommandPatternSelector: React.FC<CommandPatternSelectorProps> = ({
command,
patterns,
allowedCommands,
deniedCommands,
Expand All @@ -37,13 +35,8 @@ export const CommandPatternSelector: React.FC<CommandPatternSelectorProps> = ({

// Create a combined list with full command first, then patterns
const allPatterns = useMemo(() => {
// Trim the command to ensure consistency with extracted patterns
const trimmedCommand = command.trim()
const fullCommandPattern: CommandPattern = { pattern: trimmedCommand }

// Create a set to track unique patterns we've already seen
const seenPatterns = new Set<string>()
seenPatterns.add(trimmedCommand) // Add the trimmed full command first

// Filter out any patterns that are duplicates or are the same as the full command
const uniquePatterns = patterns.filter((p) => {
Expand All @@ -54,8 +47,8 @@ export const CommandPatternSelector: React.FC<CommandPatternSelectorProps> = ({
return true
})

return [fullCommandPattern, ...uniquePatterns]
}, [command, patterns])
return uniquePatterns
}, [patterns])

const getPatternStatus = (pattern: string): "allowed" | "denied" | "none" => {
if (allowedCommands.includes(pattern)) return "allowed"
Expand Down
52 changes: 30 additions & 22 deletions webview-ui/src/components/chat/__tests__/CommandExecution.spec.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,11 +22,13 @@ vi.mock("../../common/CodeBlock", () => ({
}))

vi.mock("../CommandPatternSelector", () => ({
CommandPatternSelector: ({ command, onAllowPatternChange, onDenyPatternChange }: any) => (
CommandPatternSelector: ({ patterns, onAllowPatternChange, onDenyPatternChange }: any) => (
<div data-testid="command-pattern-selector">
<span>{command}</span>
<button onClick={() => onAllowPatternChange(command)}>Allow {command}</button>
<button onClick={() => onDenyPatternChange(command)}>Deny {command}</button>
{patterns.map((pattern: any, index: number) => (
<span key={index}>{pattern.pattern}</span>
))}
<button onClick={() => onAllowPatternChange(patterns[0]?.pattern)}>Allow</button>
<button onClick={() => onDenyPatternChange(patterns[0]?.pattern)}>Deny</button>
</div>
),
}))
Expand Down Expand Up @@ -104,7 +106,7 @@ describe("CommandExecution", () => {
</ExtensionStateWrapper>,
)

const allowButton = screen.getByText("Allow git push")
const allowButton = screen.getByText("Allow")
fireEvent.click(allowButton)

expect(mockExtensionState.setAllowedCommands).toHaveBeenCalledWith(["npm", "git push"])
Expand All @@ -120,7 +122,7 @@ describe("CommandExecution", () => {
</ExtensionStateWrapper>,
)

const denyButton = screen.getByText("Deny docker run")
const denyButton = screen.getByText("Deny")
fireEvent.click(denyButton)

expect(mockExtensionState.setAllowedCommands).toHaveBeenCalledWith(["npm"])
Expand All @@ -143,7 +145,7 @@ describe("CommandExecution", () => {
</ExtensionStateContext.Provider>,
)

const allowButton = screen.getByText("Allow npm test")
const allowButton = screen.getByText("Allow")
fireEvent.click(allowButton)

// "npm test" is already in allowedCommands, so it should be removed
Expand All @@ -167,7 +169,7 @@ describe("CommandExecution", () => {
</ExtensionStateContext.Provider>,
)

const denyButton = screen.getByText("Deny rm -rf")
const denyButton = screen.getByText("Deny")
fireEvent.click(denyButton)

// "rm -rf" is already in deniedCommands, so it should be removed
Expand Down Expand Up @@ -223,7 +225,8 @@ Suggested patterns: npm, npm install, npm run`

const selector = screen.getByTestId("command-pattern-selector")
expect(selector).toBeInTheDocument()
expect(selector).toHaveTextContent("ls -la | grep test")
// Should show one of the individual commands from the pipe
expect(selector.textContent).toMatch(/ls -la|grep test/)
})

it("should handle commands with && operator", () => {
Expand All @@ -235,7 +238,8 @@ Suggested patterns: npm, npm install, npm run`

const selector = screen.getByTestId("command-pattern-selector")
expect(selector).toBeInTheDocument()
expect(selector).toHaveTextContent("npm install && npm test")
// Should show one of the individual commands from the && chain
expect(selector.textContent).toMatch(/npm install|npm test|npm/)
})

it("should not show pattern selector for empty commands", () => {
Expand Down Expand Up @@ -301,7 +305,7 @@ Output here`
</ExtensionStateContext.Provider>,
)

const allowButton = screen.getByText("Allow rm file.txt")
const allowButton = screen.getByText("Allow")
fireEvent.click(allowButton)

// "rm file.txt" should be removed from denied and added to allowed
Expand All @@ -321,7 +325,8 @@ Output here`

const selector = screen.getByTestId("command-pattern-selector")
expect(selector).toBeInTheDocument()
expect(selector).toHaveTextContent("npm install && npm test || echo 'failed'")
// Should show one of the individual commands from the complex chain
expect(selector.textContent).toMatch(/npm install|npm test|echo|npm/)
})

it("should handle commands with output", () => {
Expand Down Expand Up @@ -356,7 +361,8 @@ Other output here`

const selector = screen.getByTestId("command-pattern-selector")
expect(selector).toBeInTheDocument()
expect(selector).toHaveTextContent("echo $(whoami) && git status")
// Should show one of the individual commands
expect(selector.textContent).toMatch(/echo|whoami|git status|git/)
})

it("should handle commands with backtick subshells", () => {
Expand All @@ -368,7 +374,8 @@ Other output here`

const selector = screen.getByTestId("command-pattern-selector")
expect(selector).toBeInTheDocument()
expect(selector).toHaveTextContent("git commit -m `date`")
// Should show one of the individual commands
expect(selector.textContent).toMatch(/git commit|date|git/)
})

it("should handle commands with special characters", () => {
Expand All @@ -380,7 +387,8 @@ Other output here`

const selector = screen.getByTestId("command-pattern-selector")
expect(selector).toBeInTheDocument()
expect(selector).toHaveTextContent("cd ~/projects && npm start")
// Should show one of the individual commands
expect(selector.textContent).toMatch(/cd ~\/projects|npm start|cd|npm/)
})

it("should handle commands with mixed content including output", () => {
Expand Down Expand Up @@ -421,7 +429,7 @@ Running tests...
)

// Click to allow "git push origin main"
const allowButton = screen.getByText("Allow git push origin main")
const allowButton = screen.getByText("Allow")
fireEvent.click(allowButton)

// Should add to allowed and remove from denied
Expand All @@ -442,10 +450,10 @@ Running tests...
// Should still render the command
expect(screen.getByTestId("code-block")).toHaveTextContent("echo 'test with unclosed quote")

// Should show pattern selector with the full command
// Should show pattern selector with a command pattern
const selector = screen.getByTestId("command-pattern-selector")
expect(selector).toBeInTheDocument()
expect(selector).toHaveTextContent("echo 'test with unclosed quote")
expect(selector.textContent).toMatch(/echo/)
})

it("should handle empty or whitespace-only commands", () => {
Expand Down Expand Up @@ -525,8 +533,8 @@ Output:
const selector = screen.getByTestId("command-pattern-selector")
expect(selector).toBeInTheDocument()

// Should show the full command in the selector
expect(selector).toHaveTextContent("wc -l *.go *.java")
// Should show a command pattern
expect(selector.textContent).toMatch(/wc/)

// The output should still be displayed in the code block
expect(codeBlocks.length).toBeGreaterThan(1)
Expand All @@ -548,8 +556,8 @@ Output:
const selector = screen.getByTestId("command-pattern-selector")
expect(selector).toBeInTheDocument()

// Should show the full command in the selector
expect(selector).toHaveTextContent("wc -l *.go *.java")
// Should show a command pattern
expect(selector.textContent).toMatch(/wc/)

// The output should still be displayed in the code block
const codeBlocks = screen.getAllByTestId("code-block")
Expand Down
41 changes: 21 additions & 20 deletions webview-ui/src/components/chat/__tests__/CommandPatternSelector.spec.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,8 @@ const TestWrapper = ({ children }: { children: React.ReactNode }) => <TooltipPro

describe("CommandPatternSelector", () => {
const defaultProps = {
command: "npm install express",
patterns: [
{ pattern: "npm install express", description: "Full command" },
{ pattern: "npm install", description: "Install npm packages" },
{ pattern: "npm *", description: "Any npm command" },
],
Expand All @@ -51,7 +51,7 @@ describe("CommandPatternSelector", () => {
expect(screen.getByText("chat:commandExecution.manageCommands")).toBeInTheDocument()
})

it("should show full command as first pattern when expanded", () => {
it("should show patterns when expanded", () => {
render(
<TestWrapper>
<CommandPatternSelector {...defaultProps} />
Expand All @@ -62,8 +62,9 @@ describe("CommandPatternSelector", () => {
const expandButton = screen.getByRole("button")
fireEvent.click(expandButton)

// Check that the full command is shown
// Check that the patterns are shown
expect(screen.getByText("npm install express")).toBeInTheDocument()
expect(screen.getByText("- Full command")).toBeInTheDocument()
})

it("should show extracted patterns when expanded", () => {
Expand Down Expand Up @@ -95,9 +96,9 @@ describe("CommandPatternSelector", () => {
const expandButton = screen.getByRole("button")
fireEvent.click(expandButton)

// Click on the full command pattern
const fullCommandDiv = screen.getByText("npm install express").closest("div")
fireEvent.click(fullCommandDiv!)
// Click on a pattern
const patternDiv = screen.getByText("npm install express").closest("div")
fireEvent.click(patternDiv!)

// An input should appear
const input = screen.getByDisplayValue("npm install express") as HTMLInputElement
Expand Down Expand Up @@ -168,9 +169,9 @@ describe("CommandPatternSelector", () => {
const expandButton = screen.getByRole("button")
fireEvent.click(expandButton)

// Find the full command pattern row and click allow
const fullCommandPattern = screen.getByText("npm install express").closest(".ml-5")
const allowButton = fullCommandPattern?.querySelector('button[aria-label*="addToAllowed"]')
// Find a pattern row and click allow
const patternRow = screen.getByText("npm install express").closest(".ml-5")
const allowButton = patternRow?.querySelector('button[aria-label*="addToAllowed"]')
fireEvent.click(allowButton!)

// Check that the callback was called with the pattern
Expand All @@ -194,9 +195,9 @@ describe("CommandPatternSelector", () => {
const expandButton = screen.getByRole("button")
fireEvent.click(expandButton)

// Find the full command pattern row and click deny
const fullCommandPattern = screen.getByText("npm install express").closest(".ml-5")
const denyButton = fullCommandPattern?.querySelector('button[aria-label*="addToDenied"]')
// Find a pattern row and click deny
const patternRow = screen.getByText("npm install express").closest(".ml-5")
const denyButton = patternRow?.querySelector('button[aria-label*="addToDenied"]')
fireEvent.click(denyButton!)

// Check that the callback was called with the pattern
Expand All @@ -220,11 +221,11 @@ describe("CommandPatternSelector", () => {
const expandButton = screen.getByRole("button")
fireEvent.click(expandButton)

// Click on the full command pattern to edit
const fullCommandDiv = screen.getByText("npm install express").closest("div")
fireEvent.click(fullCommandDiv!)
// Click on a pattern to edit
const patternDiv = screen.getByText("npm install express").closest("div")
fireEvent.click(patternDiv!)

// Edit the command
// Edit the pattern
const input = screen.getByDisplayValue("npm install express") as HTMLInputElement
fireEvent.change(input, { target: { value: "npm install react" } })

Expand Down Expand Up @@ -254,11 +255,11 @@ describe("CommandPatternSelector", () => {
const expandButton = screen.getByRole("button")
fireEvent.click(expandButton)

// Click on the full command pattern to edit
const fullCommandDiv = screen.getByText("npm install express").closest("div")
fireEvent.click(fullCommandDiv!)
// Click on a pattern to edit
const patternDiv = screen.getByText("npm install express").closest("div")
fireEvent.click(patternDiv!)

// Edit the command
// Edit the pattern
const input = screen.getByDisplayValue("npm install express") as HTMLInputElement
fireEvent.change(input, { target: { value: "npm install react" } })

Expand Down
Loading