Skip to content

Commit 25952f2

Browse files
urmauururmauur
authored
✨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
1 parent 78df0a2 commit 25952f2

File tree

18 files changed

+147
-64
lines changed

18 files changed

+147
-64
lines changed

‎web-app/src/components/ui/__tests__/button.test.tsx

Lines changed: 75 additions & 40 deletions
Original file line numberDiff line numberDiff line change
@@ -1,113 +1,134 @@
1+
import React from 'react'
12
import { render, screen, fireEvent } from '@testing-library/react'
23
import { describe, it, expect, vi } from 'vitest'
34
import userEvent from '@testing-library/user-event'
5+
import '@testing-library/jest-dom'
46
import { Button } from '../button'
57

68
describe('Button', () => {
79
it('renders button with children', () => {
810
render(<Button>Click me</Button>)
9-
11+
1012
expect(screen.getByRole('button')).toBeInTheDocument()
1113
expect(screen.getByText('Click me')).toBeInTheDocument()
1214
})
1315

1416
it('applies default variant classes', () => {
1517
render(<Button>Default Button</Button>)
16-
18+
1719
const button = screen.getByRole('button')
18-
expect(button).toHaveClass('bg-primary', 'text-primary-fg', 'hover:bg-primary/90')
20+
expect(button).toHaveClass(
21+
'bg-primary',
22+
'text-primary-fg',
23+
'hover:bg-primary/90'
24+
)
1925
})
2026

2127
it('applies destructive variant classes', () => {
2228
render(<Button variant="destructive">Destructive Button</Button>)
23-
29+
2430
const button = screen.getByRole('button')
25-
expect(button).toHaveClass('bg-destructive', 'text-destructive-fg', 'hover:bg-destructive/90')
31+
expect(button).toHaveClass(
32+
'bg-destructive',
33+
'text-destructive-fg',
34+
'hover:bg-destructive/90'
35+
)
2636
})
2737

2838
it('applies link variant classes', () => {
2939
render(<Button variant="link">Link Button</Button>)
30-
40+
3141
const button = screen.getByRole('button')
3242
expect(button).toHaveClass('underline-offset-4', 'hover:no-underline')
3343
})
3444

3545
it('applies default size classes', () => {
3646
render(<Button>Default Size</Button>)
37-
47+
3848
const button = screen.getByRole('button')
3949
expect(button).toHaveClass('h-7', 'px-3', 'py-2')
4050
})
4151

4252
it('applies small size classes', () => {
4353
render(<Button size="sm">Small Button</Button>)
44-
54+
4555
const button = screen.getByRole('button')
4656
expect(button).toHaveClass('h-6', 'px-2')
4757
})
4858

4959
it('applies large size classes', () => {
5060
render(<Button size="lg">Large Button</Button>)
51-
61+
5262
const button = screen.getByRole('button')
5363
expect(button).toHaveClass('h-9', 'rounded-md', 'px-4')
5464
})
5565

5666
it('applies icon size classes', () => {
5767
render(<Button size="icon">Icon</Button>)
58-
68+
5969
const button = screen.getByRole('button')
6070
expect(button).toHaveClass('size-8')
6171
})
6272

6373
it('handles click events', async () => {
6474
const handleClick = vi.fn()
6575
const user = userEvent.setup()
66-
76+
6777
render(<Button onClick={handleClick}>Click me</Button>)
68-
78+
6979
await user.click(screen.getByRole('button'))
70-
80+
7181
expect(handleClick).toHaveBeenCalledTimes(1)
7282
})
7383

7484
it('can be disabled', () => {
7585
render(<Button disabled>Disabled Button</Button>)
76-
86+
7787
const button = screen.getByRole('button')
7888
expect(button).toBeDisabled()
79-
expect(button).toHaveClass('disabled:pointer-events-none', 'disabled:opacity-50')
89+
expect(button).toHaveClass(
90+
'disabled:pointer-events-none',
91+
'disabled:opacity-50'
92+
)
8093
})
8194

8295
it('does not trigger click when disabled', async () => {
8396
const handleClick = vi.fn()
8497
const user = userEvent.setup()
85-
86-
render(<Button disabled onClick={handleClick}>Disabled Button</Button>)
87-
98+
99+
render(
100+
<Button disabled onClick={handleClick}>
101+
Disabled Button
102+
</Button>
103+
)
104+
88105
await user.click(screen.getByRole('button'))
89-
106+
90107
expect(handleClick).not.toHaveBeenCalled()
91108
})
92109

93110
it('forwards ref correctly', () => {
94111
const ref = vi.fn()
95-
112+
96113
render(<Button ref={ref}>Button with ref</Button>)
97-
114+
98115
expect(ref).toHaveBeenCalledWith(expect.any(HTMLButtonElement))
99116
})
100117

101118
it('accepts custom className', () => {
102119
render(<Button className="custom-class">Custom Button</Button>)
103-
120+
104121
const button = screen.getByRole('button')
105122
expect(button).toHaveClass('custom-class')
106123
})
107124

108125
it('accepts custom props', () => {
109-
render(<Button data-testid="custom-button" type="submit">Custom Button</Button>)
110-
126+
render(
127+
<Button data-testid="custom-button" type="submit">
128+
Custom Button
129+
</Button>
130+
)
131+
111132
const button = screen.getByTestId('custom-button')
112133
expect(button).toHaveAttribute('type', 'submit')
113134
})
@@ -118,51 +139,65 @@ describe('Button', () => {
118139
<a href="/test">Link Button</a>
119140
</Button>
120141
)
121-
142+
122143
const link = screen.getByRole('link')
123144
expect(link).toHaveAttribute('href', '/test')
124145
expect(link).toHaveClass('bg-primary', 'text-primary-fg') // Should inherit button classes
125146
})
126147

127148
it('combines variant and size classes correctly', () => {
128-
render(<Button variant="destructive" size="lg">Large Destructive Button</Button>)
129-
149+
render(
150+
<Button variant="destructive" size="lg">
151+
Large Destructive Button
152+
</Button>
153+
)
154+
130155
const button = screen.getByRole('button')
131156
expect(button).toHaveClass('bg-destructive', 'text-destructive-fg') // destructive variant
132157
expect(button).toHaveClass('h-9', 'rounded-md', 'px-4') // large size
133158
})
134159

135160
it('handles keyboard events', () => {
136161
const handleKeyDown = vi.fn()
137-
162+
138163
render(<Button onKeyDown={handleKeyDown}>Keyboard Button</Button>)
139-
164+
140165
const button = screen.getByRole('button')
141166
fireEvent.keyDown(button, { key: 'Enter' })
142-
143-
expect(handleKeyDown).toHaveBeenCalledWith(expect.objectContaining({
144-
key: 'Enter'
145-
}))
167+
168+
expect(handleKeyDown).toHaveBeenCalledWith(
169+
expect.objectContaining({
170+
key: 'Enter',
171+
})
172+
)
146173
})
147174

148175
it('supports focus events', () => {
149176
const handleFocus = vi.fn()
150177
const handleBlur = vi.fn()
151-
152-
render(<Button onFocus={handleFocus} onBlur={handleBlur}>Focus Button</Button>)
153-
178+
179+
render(
180+
<Button onFocus={handleFocus} onBlur={handleBlur}>
181+
Focus Button
182+
</Button>
183+
)
184+
154185
const button = screen.getByRole('button')
155186
fireEvent.focus(button)
156187
fireEvent.blur(button)
157-
188+
158189
expect(handleFocus).toHaveBeenCalledTimes(1)
159190
expect(handleBlur).toHaveBeenCalledTimes(1)
160191
})
161192

162193
it('applies focus-visible styling', () => {
163194
render(<Button>Focus Button</Button>)
164-
195+
165196
const button = screen.getByRole('button')
166-
expect(button).toHaveClass('focus-visible:border-ring', 'focus-visible:ring-ring/50')
197+
expect(button).toHaveClass(
198+
'focus-visible:border-primary',
199+
'focus-visible:ring-2',
200+
'focus-visible:ring-primary/60'
201+
)
167202
})
168-
})
203+
})

‎web-app/src/components/ui/button.tsx

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -5,13 +5,18 @@ import { cva, type VariantProps } from 'class-variance-authority'
55
import { cn } from '@/lib/utils'
66

77
const buttonVariants = cva(
8-
"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",
8+
cn(
9+
"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",
10+
'focus:border-accent focus:ring-2 focus:ring-accent/50 focus:accent-[0px]',
11+
'focus-visible:border-accent focus-visible:ring-2 focus-visible:ring-accent/50 focus-visible:accent-[0px]'
12+
),
913
{
1014
variants: {
1115
variant: {
12-
default: 'bg-primary text-primary-fg shadow-xs hover:bg-primary/90',
16+
default:
17+
'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',
1318
destructive:
14-
'bg-destructive shadow-xs hover:bg-destructive/90 focus-visible:ring-destructive/20 dark:focus-visible:ring-destructive/40 text-destructive-fg',
19+
'bg-destructive shadow-xs hover:bg-destructive/90 focus-visible:ring-destructive/60 text-destructive-fg focus:border-destructive focus:ring-destructive/60',
1520
link: 'underline-offset-4 hover:no-underline',
1621
},
1722
size: {

‎web-app/src/containers/ThreadContent.tsx

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -104,13 +104,13 @@ const EditDialog = ({
104104
</TooltipContent>
105105
</Tooltip>
106106
</DialogTrigger>
107-
<DialogContent className="w-3/4 h-3/4">
107+
<DialogContent className="w-3/4">
108108
<DialogHeader>
109109
<DialogTitle>{t('common:dialogs.editMessage.title')}</DialogTitle>
110110
<Textarea
111111
value={draft}
112112
onChange={(e) => setDraft(e.target.value)}
113-
className="mt-2 resize-none h-full w-full"
113+
className="mt-2 resize-none w-full"
114114
onKeyDown={(e) => {
115115
// Prevent key from being captured by parent components
116116
e.stopPropagation()

‎web-app/src/containers/dialogs/ToolApproval.tsx

Lines changed: 16 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,7 @@ export default function ToolApproval() {
1919
return null
2020
}
2121

22-
const { toolName, onApprove, onDeny } = modalProps
22+
const { toolName, toolParameters, onApprove, onDeny } = modalProps
2323

2424
const handleAllowOnce = () => {
2525
onApprove(true) // true = allow once only
@@ -58,7 +58,20 @@ export default function ToolApproval() {
5858
</div>
5959
</DialogHeader>
6060

61-
<div className="bg-main-view-fg/8 p-2 border border-main-view-fg/5 rounded-lg">
61+
{toolParameters && Object.keys(toolParameters).length > 0 && (
62+
<div className="bg-main-view-fg/4 p-2 border border-main-view-fg/5 rounded-lg">
63+
<h4 className="text-sm font-medium text-main-view-fg mb-2">
64+
{t('tools:toolApproval.parameters')}
65+
</h4>
66+
<div className="bg-main-view-fg/6 rounded-md p-2 text-sm font-mono border border-main-view-fg/5">
67+
<pre className="text-main-view-fg/80 whitespace-pre-wrap break-words">
68+
{JSON.stringify(toolParameters, null, 2)}
69+
</pre>
70+
</div>
71+
</div>
72+
)}
73+
74+
<div className="bg-main-view-fg/1 p-2 border border-main-view-fg/5 rounded-lg">
6275
<p className="text-sm text-main-view-fg/70 leading-relaxed">
6376
{t('tools:toolApproval.securityNotice')}
6477
</p>
@@ -80,7 +93,7 @@ export default function ToolApproval() {
8093
>
8194
{t('tools:toolApproval.allowOnce')}
8295
</Button>
83-
<Button variant="default" onClick={handleAllow}>
96+
<Button variant="default" onClick={handleAllow} autoFocus>
8497
{t('tools:toolApproval.alwaysAllow')}
8598
</Button>
8699
</div>

‎web-app/src/hooks/useToolApproval.ts

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ import { localStorageKey } from '@/constants/localStorage'
55
export type ToolApprovalModalProps = {
66
toolName: string
77
threadId: string
8+
toolParameters?: object
89
onApprove: (allowOnce: boolean) => void
910
onDeny: () => void
1011
}
@@ -21,7 +22,7 @@ type ToolApprovalState = {
2122
// Actions
2223
approveToolForThread: (threadId: string, toolName: string) => void
2324
isToolApproved: (threadId: string, toolName: string) => boolean
24-
showApprovalModal: (toolName: string, threadId: string) => Promise<boolean>
25+
showApprovalModal: (toolName: string, threadId: string, toolParameters?: object) => Promise<boolean>
2526
closeModal: () => void
2627
setModalOpen: (open: boolean) => void
2728
setAllowAllMCPPermissions: (allow: boolean) => void
@@ -52,7 +53,7 @@ export const useToolApproval = create<ToolApprovalState>()(
5253
return state.approvedTools[threadId]?.includes(toolName) || false
5354
},
5455

55-
showApprovalModal: (toolName: string, threadId: string) => {
56+
showApprovalModal: (toolName: string, threadId: string, toolParameters?: object) => {
5657
return new Promise<boolean>((resolve) => {
5758
// Check if tool is already approved for this thread
5859
const state = get()
@@ -66,6 +67,7 @@ export const useToolApproval = create<ToolApprovalState>()(
6667
modalProps: {
6768
toolName,
6869
threadId,
70+
toolParameters,
6971
onApprove: (allowOnce: boolean) => {
7072
if (!allowOnce) {
7173
// If not "allow once", add to approved tools for this thread

‎web-app/src/lib/completion.ts

Lines changed: 18 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -329,7 +329,11 @@ export const postMessageProcessing = async (
329329
message: ThreadMessage,
330330
abortController: AbortController,
331331
approvedTools: Record<string, string[]> = {},
332-
showModal?: (toolName: string, threadId: string) => Promise<boolean>,
332+
showModal?: (
333+
toolName: string,
334+
threadId: string,
335+
toolParameters?: object
336+
) => Promise<boolean>,
333337
allowAllMCPPermissions: boolean = false
334338
) => {
335339
// Handle completed tool calls
@@ -358,11 +362,23 @@ export const postMessageProcessing = async (
358362
}
359363

360364
// Check if tool is approved or show modal for approval
365+
let toolParameters = {}
366+
if (toolCall.function.arguments.length) {
367+
try {
368+
toolParameters = JSON.parse(toolCall.function.arguments)
369+
} catch (error) {
370+
console.error('Failed to parse tool arguments:', error)
371+
}
372+
}
361373
const approved =
362374
allowAllMCPPermissions ||
363375
approvedTools[message.thread_id]?.includes(toolCall.function.name) ||
364376
(showModal
365-
? await showModal(toolCall.function.name, message.thread_id)
377+
? await showModal(
378+
toolCall.function.name,
379+
message.thread_id,
380+
toolParameters
381+
)
366382
: true)
367383

368384
let result = approved

‎web-app/src/locales/de-DE/tool-approval.json

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,5 +7,6 @@
77
"alwaysAllow": "Immer erlauben",
88
"permissions": "Berechtigungen",
99
"approve": "Genehmigen",
10-
"reject": "Ablehnen"
10+
"reject": "Ablehnen",
11+
"parameters": "Werkzeug-Parameter"
1112
}

0 commit comments

Comments
 (0)