Fix the UI for approving chained commands (#6623)

Author: mrubens

webview-ui/src/components/chat/CommandExecution.tsx

@@ -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}

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

@@ -11,7 +11,6 @@ interface CommandPattern {
 }
 
 interface CommandPatternSelectorProps {
-	command: string
 	patterns: CommandPattern[]
 	allowedCommands: string[]
 	deniedCommands: string[]
@@ -20,7 +19,6 @@ interface CommandPatternSelectorProps {
 }
 
 export const CommandPatternSelector: React.FC<CommandPatternSelectorProps> = ({
-	command,
 	patterns,
 	allowedCommands,
 	deniedCommands,
@@ -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) => {
@@ -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"

webview-ui/src/components/chat/__tests__/CommandExecution.spec.tsx

@@ -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>
 	),
 }))
@@ -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"])
@@ -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"])
@@ -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
@@ -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
@@ -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", () => {
@@ -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", () => {
@@ -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
@@ -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", () => {
@@ -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", () => {
@@ -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", () => {
@@ -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", () => {
@@ -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
@@ -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", () => {
@@ -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)
@@ -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")

webview-ui/src/components/chat/__tests__/CommandPatternSelector.spec.tsx

@@ -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" },
 		],
@@ -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} />
@@ -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", () => {
@@ -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
@@ -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
@@ -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
@@ -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" } })
 
@@ -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" } })