enhancement: auto focus always allow action from tool approval dialog and add req parameters (#5836)

* enhancement: auto focus always allow action from tool approval dialog

* chore: error handling tools parameters

* chore: update test button focus cases
This commit is contained in:
Faisal Amir 2025-07-22 12:17:53 +07:00 committed by GitHub
parent 78df0a20ec
commit 25952f293c
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
18 changed files with 147 additions and 64 deletions

View File

@ -1,6 +1,8 @@
import React from 'react'
import { render, screen, fireEvent } from '@testing-library/react' import { render, screen, fireEvent } from '@testing-library/react'
import { describe, it, expect, vi } from 'vitest' import { describe, it, expect, vi } from 'vitest'
import userEvent from '@testing-library/user-event' import userEvent from '@testing-library/user-event'
import '@testing-library/jest-dom'
import { Button } from '../button' import { Button } from '../button'
describe('Button', () => { describe('Button', () => {
@ -15,14 +17,22 @@ describe('Button', () => {
render(<Button>Default Button</Button>) render(<Button>Default Button</Button>)
const button = screen.getByRole('button') const button = screen.getByRole('button')
expect(button).toHaveClass('bg-primary', 'text-primary-fg', 'hover:bg-primary/90') expect(button).toHaveClass(
'bg-primary',
'text-primary-fg',
'hover:bg-primary/90'
)
}) })
it('applies destructive variant classes', () => { it('applies destructive variant classes', () => {
render(<Button variant="destructive">Destructive Button</Button>) render(<Button variant="destructive">Destructive Button</Button>)
const button = screen.getByRole('button') const button = screen.getByRole('button')
expect(button).toHaveClass('bg-destructive', 'text-destructive-fg', 'hover:bg-destructive/90') expect(button).toHaveClass(
'bg-destructive',
'text-destructive-fg',
'hover:bg-destructive/90'
)
}) })
it('applies link variant classes', () => { it('applies link variant classes', () => {
@ -76,14 +86,21 @@ describe('Button', () => {
const button = screen.getByRole('button') const button = screen.getByRole('button')
expect(button).toBeDisabled() expect(button).toBeDisabled()
expect(button).toHaveClass('disabled:pointer-events-none', 'disabled:opacity-50') expect(button).toHaveClass(
'disabled:pointer-events-none',
'disabled:opacity-50'
)
}) })
it('does not trigger click when disabled', async () => { it('does not trigger click when disabled', async () => {
const handleClick = vi.fn() const handleClick = vi.fn()
const user = userEvent.setup() const user = userEvent.setup()
render(<Button disabled onClick={handleClick}>Disabled Button</Button>) render(
<Button disabled onClick={handleClick}>
Disabled Button
</Button>
)
await user.click(screen.getByRole('button')) await user.click(screen.getByRole('button'))
@ -106,7 +123,11 @@ describe('Button', () => {
}) })
it('accepts custom props', () => { it('accepts custom props', () => {
render(<Button data-testid="custom-button" type="submit">Custom Button</Button>) render(
<Button data-testid="custom-button" type="submit">
Custom Button
</Button>
)
const button = screen.getByTestId('custom-button') const button = screen.getByTestId('custom-button')
expect(button).toHaveAttribute('type', 'submit') expect(button).toHaveAttribute('type', 'submit')
@ -125,7 +146,11 @@ describe('Button', () => {
}) })
it('combines variant and size classes correctly', () => { it('combines variant and size classes correctly', () => {
render(<Button variant="destructive" size="lg">Large Destructive Button</Button>) render(
<Button variant="destructive" size="lg">
Large Destructive Button
</Button>
)
const button = screen.getByRole('button') const button = screen.getByRole('button')
expect(button).toHaveClass('bg-destructive', 'text-destructive-fg') // destructive variant expect(button).toHaveClass('bg-destructive', 'text-destructive-fg') // destructive variant
@ -140,16 +165,22 @@ describe('Button', () => {
const button = screen.getByRole('button') const button = screen.getByRole('button')
fireEvent.keyDown(button, { key: 'Enter' }) fireEvent.keyDown(button, { key: 'Enter' })
expect(handleKeyDown).toHaveBeenCalledWith(expect.objectContaining({ expect(handleKeyDown).toHaveBeenCalledWith(
key: 'Enter' expect.objectContaining({
})) key: 'Enter',
})
)
}) })
it('supports focus events', () => { it('supports focus events', () => {
const handleFocus = vi.fn() const handleFocus = vi.fn()
const handleBlur = vi.fn() const handleBlur = vi.fn()
render(<Button onFocus={handleFocus} onBlur={handleBlur}>Focus Button</Button>) render(
<Button onFocus={handleFocus} onBlur={handleBlur}>
Focus Button
</Button>
)
const button = screen.getByRole('button') const button = screen.getByRole('button')
fireEvent.focus(button) fireEvent.focus(button)
@ -163,6 +194,10 @@ describe('Button', () => {
render(<Button>Focus Button</Button>) render(<Button>Focus Button</Button>)
const button = screen.getByRole('button') const button = screen.getByRole('button')
expect(button).toHaveClass('focus-visible:border-ring', 'focus-visible:ring-ring/50') expect(button).toHaveClass(
'focus-visible:border-primary',
'focus-visible:ring-2',
'focus-visible:ring-primary/60'
)
}) })
}) })

View File

@ -5,13 +5,18 @@ import { cva, type VariantProps } from 'class-variance-authority'
import { cn } from '@/lib/utils' import { cn } from '@/lib/utils'
const buttonVariants = cva( const buttonVariants = cva(
"inline-flex items-center justify-center gap-2 whitespace-nowrap rounded-md text-sm font-medium transition-all disabled:pointer-events-none disabled:opacity-50 [&_svg]:pointer-events-none [&_svg:not([class*='size-'])]:size-4 shrink-0 [&_svg]:shrink-0 outline-none focus-visible:border-ring focus-visible:ring-ring/50 focus-visible:ring-[0px] aria-invalid:ring-destructive/20 dark:aria-invalid:ring-destructive/40 aria-invalid:border-destructive cursor-pointer focus:outline-none", cn(
"inline-flex items-center justify-center gap-2 whitespace-nowrap rounded-md text-sm font-medium transition-all disabled:pointer-events-none disabled:opacity-50 [&_svg]:pointer-events-none [&_svg:not([class*='size-'])]:size-4 shrink-0 [&_svg]:shrink-0 outline-none aria-invalid:ring-destructive/60 aria-invalid:border-destructive cursor-pointer",
'focus:border-accent focus:ring-2 focus:ring-accent/50 focus:accent-[0px]',
'focus-visible:border-accent focus-visible:ring-2 focus-visible:ring-accent/50 focus-visible:accent-[0px]'
),
{ {
variants: { variants: {
variant: { variant: {
default: 'bg-primary text-primary-fg shadow-xs hover:bg-primary/90', default:
'bg-primary text-primary-fg shadow-xs hover:bg-primary/90 focus-visible:ring-primary/60 focus:ring-primary/60 focus:border-primary focus-visible:border-primary',
destructive: destructive:
'bg-destructive shadow-xs hover:bg-destructive/90 focus-visible:ring-destructive/20 dark:focus-visible:ring-destructive/40 text-destructive-fg', 'bg-destructive shadow-xs hover:bg-destructive/90 focus-visible:ring-destructive/60 text-destructive-fg focus:border-destructive focus:ring-destructive/60',
link: 'underline-offset-4 hover:no-underline', link: 'underline-offset-4 hover:no-underline',
}, },
size: { size: {

View File

@ -104,13 +104,13 @@ const EditDialog = ({
</TooltipContent> </TooltipContent>
</Tooltip> </Tooltip>
</DialogTrigger> </DialogTrigger>
<DialogContent className="w-3/4 h-3/4"> <DialogContent className="w-3/4">
<DialogHeader> <DialogHeader>
<DialogTitle>{t('common:dialogs.editMessage.title')}</DialogTitle> <DialogTitle>{t('common:dialogs.editMessage.title')}</DialogTitle>
<Textarea <Textarea
value={draft} value={draft}
onChange={(e) => setDraft(e.target.value)} onChange={(e) => setDraft(e.target.value)}
className="mt-2 resize-none h-full w-full" className="mt-2 resize-none w-full"
onKeyDown={(e) => { onKeyDown={(e) => {
// Prevent key from being captured by parent components // Prevent key from being captured by parent components
e.stopPropagation() e.stopPropagation()

View File

@ -19,7 +19,7 @@ export default function ToolApproval() {
return null return null
} }
const { toolName, onApprove, onDeny } = modalProps const { toolName, toolParameters, onApprove, onDeny } = modalProps
const handleAllowOnce = () => { const handleAllowOnce = () => {
onApprove(true) // true = allow once only onApprove(true) // true = allow once only
@ -58,7 +58,20 @@ export default function ToolApproval() {
</div> </div>
</DialogHeader> </DialogHeader>
<div className="bg-main-view-fg/8 p-2 border border-main-view-fg/5 rounded-lg"> {toolParameters && Object.keys(toolParameters).length > 0 && (
<div className="bg-main-view-fg/4 p-2 border border-main-view-fg/5 rounded-lg">
<h4 className="text-sm font-medium text-main-view-fg mb-2">
{t('tools:toolApproval.parameters')}
</h4>
<div className="bg-main-view-fg/6 rounded-md p-2 text-sm font-mono border border-main-view-fg/5">
<pre className="text-main-view-fg/80 whitespace-pre-wrap break-words">
{JSON.stringify(toolParameters, null, 2)}
</pre>
</div>
</div>
)}
<div className="bg-main-view-fg/1 p-2 border border-main-view-fg/5 rounded-lg">
<p className="text-sm text-main-view-fg/70 leading-relaxed"> <p className="text-sm text-main-view-fg/70 leading-relaxed">
{t('tools:toolApproval.securityNotice')} {t('tools:toolApproval.securityNotice')}
</p> </p>
@ -80,7 +93,7 @@ export default function ToolApproval() {
> >
{t('tools:toolApproval.allowOnce')} {t('tools:toolApproval.allowOnce')}
</Button> </Button>
<Button variant="default" onClick={handleAllow}> <Button variant="default" onClick={handleAllow} autoFocus>
{t('tools:toolApproval.alwaysAllow')} {t('tools:toolApproval.alwaysAllow')}
</Button> </Button>
</div> </div>

View File

@ -5,6 +5,7 @@ import { localStorageKey } from '@/constants/localStorage'
export type ToolApprovalModalProps = { export type ToolApprovalModalProps = {
toolName: string toolName: string
threadId: string threadId: string
toolParameters?: object
onApprove: (allowOnce: boolean) => void onApprove: (allowOnce: boolean) => void
onDeny: () => void onDeny: () => void
} }
@ -21,7 +22,7 @@ type ToolApprovalState = {
// Actions // Actions
approveToolForThread: (threadId: string, toolName: string) => void approveToolForThread: (threadId: string, toolName: string) => void
isToolApproved: (threadId: string, toolName: string) => boolean isToolApproved: (threadId: string, toolName: string) => boolean
showApprovalModal: (toolName: string, threadId: string) => Promise<boolean> showApprovalModal: (toolName: string, threadId: string, toolParameters?: object) => Promise<boolean>
closeModal: () => void closeModal: () => void
setModalOpen: (open: boolean) => void setModalOpen: (open: boolean) => void
setAllowAllMCPPermissions: (allow: boolean) => void setAllowAllMCPPermissions: (allow: boolean) => void
@ -52,7 +53,7 @@ export const useToolApproval = create<ToolApprovalState>()(
return state.approvedTools[threadId]?.includes(toolName) || false return state.approvedTools[threadId]?.includes(toolName) || false
}, },
showApprovalModal: (toolName: string, threadId: string) => { showApprovalModal: (toolName: string, threadId: string, toolParameters?: object) => {
return new Promise<boolean>((resolve) => { return new Promise<boolean>((resolve) => {
// Check if tool is already approved for this thread // Check if tool is already approved for this thread
const state = get() const state = get()
@ -66,6 +67,7 @@ export const useToolApproval = create<ToolApprovalState>()(
modalProps: { modalProps: {
toolName, toolName,
threadId, threadId,
toolParameters,
onApprove: (allowOnce: boolean) => { onApprove: (allowOnce: boolean) => {
if (!allowOnce) { if (!allowOnce) {
// If not "allow once", add to approved tools for this thread // If not "allow once", add to approved tools for this thread

View File

@ -329,7 +329,11 @@ export const postMessageProcessing = async (
message: ThreadMessage, message: ThreadMessage,
abortController: AbortController, abortController: AbortController,
approvedTools: Record<string, string[]> = {}, approvedTools: Record<string, string[]> = {},
showModal?: (toolName: string, threadId: string) => Promise<boolean>, showModal?: (
toolName: string,
threadId: string,
toolParameters?: object
) => Promise<boolean>,
allowAllMCPPermissions: boolean = false allowAllMCPPermissions: boolean = false
) => { ) => {
// Handle completed tool calls // Handle completed tool calls
@ -358,11 +362,23 @@ export const postMessageProcessing = async (
} }
// Check if tool is approved or show modal for approval // Check if tool is approved or show modal for approval
let toolParameters = {}
if (toolCall.function.arguments.length) {
try {
toolParameters = JSON.parse(toolCall.function.arguments)
} catch (error) {
console.error('Failed to parse tool arguments:', error)
}
}
const approved = const approved =
allowAllMCPPermissions || allowAllMCPPermissions ||
approvedTools[message.thread_id]?.includes(toolCall.function.name) || approvedTools[message.thread_id]?.includes(toolCall.function.name) ||
(showModal (showModal
? await showModal(toolCall.function.name, message.thread_id) ? await showModal(
toolCall.function.name,
message.thread_id,
toolParameters
)
: true) : true)
let result = approved let result = approved

View File

@ -7,5 +7,6 @@
"alwaysAllow": "Immer erlauben", "alwaysAllow": "Immer erlauben",
"permissions": "Berechtigungen", "permissions": "Berechtigungen",
"approve": "Genehmigen", "approve": "Genehmigen",
"reject": "Ablehnen" "reject": "Ablehnen",
"parameters": "Werkzeug-Parameter"
} }

View File

@ -5,6 +5,7 @@
"securityNotice": "Dieses Werkzeug möchte eine Aktion ausführen. Bitte überprüfen und genehmigen oder Ablehnen.", "securityNotice": "Dieses Werkzeug möchte eine Aktion ausführen. Bitte überprüfen und genehmigen oder Ablehnen.",
"deny": "Ablehnen", "deny": "Ablehnen",
"allowOnce": "Einmal erlauben", "allowOnce": "Einmal erlauben",
"alwaysAllow": "Immer erlauben" "alwaysAllow": "Immer erlauben",
"parameters": "Werkzeug-Parameter"
} }
} }

View File

@ -7,5 +7,6 @@
"alwaysAllow": "Always Allow", "alwaysAllow": "Always Allow",
"permissions": "Permissions", "permissions": "Permissions",
"approve": "Approve", "approve": "Approve",
"reject": "Reject" "reject": "Reject",
"parameters": "Tool Parameters"
} }

View File

@ -5,6 +5,7 @@
"securityNotice": "Malicious tools or conversation content could potentially trick the assistant into attempting harmful actions. Review each tool call carefully before approving.", "securityNotice": "Malicious tools or conversation content could potentially trick the assistant into attempting harmful actions. Review each tool call carefully before approving.",
"deny": "Deny", "deny": "Deny",
"allowOnce": "Allow Once", "allowOnce": "Allow Once",
"alwaysAllow": "Always Allow" "alwaysAllow": "Always Allow",
"parameters": "Tool Parameters"
} }
} }

View File

@ -7,5 +7,6 @@
"alwaysAllow": "Selalu Izinkan", "alwaysAllow": "Selalu Izinkan",
"permissions": "Izin", "permissions": "Izin",
"approve": "Setujui", "approve": "Setujui",
"reject": "Tolak" "reject": "Tolak",
"parameters": "Parameter Alat"
} }

View File

@ -5,6 +5,7 @@
"deny": "Tolak", "deny": "Tolak",
"allowOnce": "Izinkan Sekali", "allowOnce": "Izinkan Sekali",
"alwaysAllow": "Selalu Izinkan", "alwaysAllow": "Selalu Izinkan",
"description": "Asisten ingin menggunakan <strong>{{toolName}}</strong>" "description": "Asisten ingin menggunakan <strong>{{toolName}}</strong>",
"parameters": "Parameter Alat"
} }
} }

View File

@ -7,5 +7,6 @@
"alwaysAllow": "Luôn cho phép", "alwaysAllow": "Luôn cho phép",
"permissions": "Quyền", "permissions": "Quyền",
"approve": "Phê duyệt", "approve": "Phê duyệt",
"reject": "Từ chối" "reject": "Từ chối",
"parameters": "Tham số công cụ"
} }

View File

@ -5,6 +5,7 @@
"deny": "Từ chối", "deny": "Từ chối",
"allowOnce": "Cho phép một lần", "allowOnce": "Cho phép một lần",
"alwaysAllow": "Luôn cho phép", "alwaysAllow": "Luôn cho phép",
"description": "Trợ lý muốn sử dụng <strong>{{toolName}}</strong>" "description": "Trợ lý muốn sử dụng <strong>{{toolName}}</strong>",
"parameters": "Tham số công cụ"
} }
} }

View File

@ -7,5 +7,6 @@
"alwaysAllow": "始终允许", "alwaysAllow": "始终允许",
"permissions": "权限", "permissions": "权限",
"approve": "批准", "approve": "批准",
"reject": "拒绝" "reject": "拒绝",
"parameters": "工具参数"
} }

View File

@ -5,6 +5,7 @@
"deny": "拒绝", "deny": "拒绝",
"allowOnce": "允许一次", "allowOnce": "允许一次",
"alwaysAllow": "始终允许", "alwaysAllow": "始终允许",
"description": "助手想要使用 <strong>{{toolName}}</strong>" "description": "助手想要使用 <strong>{{toolName}}</strong>",
"parameters": "工具参数"
} }
} }

View File

@ -7,5 +7,6 @@
"alwaysAllow": "一律允許", "alwaysAllow": "一律允許",
"permissions": "權限", "permissions": "權限",
"approve": "核准", "approve": "核准",
"reject": "拒絕" "reject": "拒絕",
"parameters": "工具參數"
} }

View File

@ -5,6 +5,7 @@
"deny": "拒絕", "deny": "拒絕",
"allowOnce": "允許一次", "allowOnce": "允許一次",
"alwaysAllow": "一律允許", "alwaysAllow": "一律允許",
"description": "助理想要使用 <strong>{{toolName}}</strong>" "description": "助理想要使用 <strong>{{toolName}}</strong>",
"parameters": "工具參數"
} }
} }