Skip to content

Commit 17857f6

Browse files
bburdabburda
committed
fix: address PR review feedback from Copilot
- Add JSON validation before operation invoke with error display - Fix UUID regex to match standard format with hyphens - Add encodeURIComponent for goalId in API URL parameters - Return empty array instead of raw string on array parse error - Add accessibility attributes to editable parameter div (role, tabIndex, keyboard) - Add proper id/htmlFor and focus styling to auto-refresh checkbox - Replace native button with Button component for History toggle
1 parent d01595a commit 17857f6

5 files changed

Lines changed: 58 additions & 25 deletions

File tree

‎src/components/ActionStatusPanel.tsx‎

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -137,13 +137,18 @@ export function ActionStatusPanel({ componentId, operationName, goalId }: Action
137137

138138
<div className="flex items-center gap-2">
139139
{/* Auto-refresh checkbox */}
140-
<label className="flex items-center gap-1.5 text-xs text-muted-foreground cursor-pointer">
140+
<label
141+
htmlFor={`auto-refresh-${goalId}`}
142+
className="flex items-center gap-1.5 text-xs text-muted-foreground cursor-pointer"
143+
>
141144
<input
145+
id={`auto-refresh-${goalId}`}
142146
type="checkbox"
143147
checked={autoRefreshGoals}
144148
onChange={(e) => setAutoRefreshGoals(e.target.checked)}
145-
className="rounded border-muted-foreground"
149+
className="rounded border-muted-foreground focus-visible:ring-2 focus-visible:ring-offset-2 focus-visible:ring-primary"
146150
disabled={isTerminal}
151+
aria-label="Auto-refresh action status"
147152
/>
148153
Auto-refresh
149154
</label>

‎src/components/ConfigurationPanel.tsx‎

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -183,9 +183,18 @@ function ParameterRow({
183183
<div
184184
className={`px-3 py-1.5 rounded border text-sm font-mono truncate ${param.read_only
185185
? 'bg-muted text-muted-foreground cursor-not-allowed'
186-
: 'bg-background cursor-pointer hover:border-primary'
186+
: 'bg-background cursor-pointer hover:border-primary focus:outline-none focus:ring-2 focus:ring-primary focus:ring-offset-2'
187187
}`}
188188
onClick={startEditing}
189+
onKeyDown={(e) => {
190+
if (!param.read_only && (e.key === 'Enter' || e.key === ' ')) {
191+
e.preventDefault();
192+
startEditing();
193+
}
194+
}}
195+
role={param.read_only ? undefined : 'button'}
196+
tabIndex={param.read_only ? undefined : 0}
197+
aria-label={param.read_only ? `${param.name}: ${formatValue(param.value, param.type)} (read-only)` : `Edit ${param.name}`}
189198
title={param.read_only ? 'Read-only parameter' : 'Click to edit'}
190199
>
191200
{formatValue(param.value, param.type)}
@@ -248,7 +257,8 @@ function parseValue(input: string, type: ParameterType): unknown {
248257
try {
249258
return JSON.parse(input);
250259
} catch {
251-
return input;
260+
// Return empty array instead of invalid string to prevent type mismatch
261+
return [];
252262
}
253263
default:
254264
return input;

‎src/components/OperationResponse.tsx‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -30,11 +30,11 @@ function ValueDisplay({ value, depth = 0 }: { value: unknown; depth?: number })
3030
if (value === '') {
3131
return <span className="text-muted-foreground italic">(empty)</span>;
3232
}
33-
// Check if it's a UUID-like string
34-
if (/^[a-f0-9]{32}$/i.test(value)) {
33+
// Check if it's a UUID-like string (standard format with hyphens)
34+
if (/^[a-f0-9]{8}-[a-f0-9]{4}-[a-f0-9]{4}-[a-f0-9]{4}-[a-f0-9]{12}$/i.test(value)) {
3535
return (
3636
<code className="bg-muted px-1.5 py-0.5 rounded text-xs font-mono">
37-
{value.slice(0, 8)}...{value.slice(-8)}
37+
{value.slice(0, 8)}...{value.slice(-12)}
3838
</code>
3939
);
4040
}

‎src/components/OperationsPanel.tsx‎

Lines changed: 33 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -138,17 +138,24 @@ function OperationRow({
138138
}
139139
}, []);
140140

141+
// Track JSON validation error
142+
const [jsonError, setJsonError] = useState<string | null>(null);
143+
141144
const handleInvoke = useCallback(async () => {
145+
// Validate JSON before invoking
146+
let payload: unknown;
147+
try {
148+
payload = JSON.parse(requestBody);
149+
setJsonError(null);
150+
} catch (e) {
151+
const errorMsg = e instanceof Error ? e.message : 'Invalid JSON';
152+
setJsonError(errorMsg);
153+
return; // Don't invoke with invalid JSON
154+
}
155+
142156
setIsInvoking(true);
143157

144158
try {
145-
let payload: unknown;
146-
try {
147-
payload = JSON.parse(requestBody);
148-
} catch {
149-
payload = {};
150-
}
151-
152159
// Build request based on operation kind
153160
const request = operation.kind === 'service'
154161
? { type: operation.type, request: payload }
@@ -274,10 +281,19 @@ function OperationRow({
274281
<div className="space-y-2">
275282
<Textarea
276283
value={requestBody}
277-
onChange={(e) => handleJsonChange(e.target.value)}
284+
onChange={(e) => {
285+
handleJsonChange(e.target.value);
286+
setJsonError(null); // Clear error on change
287+
}}
278288
placeholder="{}"
279-
className="font-mono text-sm min-h-[80px]"
289+
className={`font-mono text-sm min-h-[80px] ${jsonError ? 'border-destructive' : ''}`}
280290
/>
291+
{jsonError && (
292+
<div className="flex items-center gap-2 text-xs text-destructive">
293+
<AlertCircle className="w-3 h-3" />
294+
Invalid JSON: {jsonError}
295+
</div>
296+
)}
281297
{/* Invoke button below textarea */}
282298
<Button
283299
variant="default"
@@ -333,18 +349,20 @@ function OperationRow({
333349
{history.length > 0 && (
334350
<div className="space-y-2">
335351
<div className="flex items-center justify-between">
336-
<button
352+
<Button
353+
variant="ghost"
354+
size="sm"
337355
onClick={() => setShowHistory(!showHistory)}
338-
className="flex items-center gap-1.5 text-xs font-medium text-muted-foreground hover:text-foreground transition-colors"
356+
className="h-6 px-2 text-xs font-medium text-muted-foreground hover:text-foreground"
339357
>
340-
<History className="w-3.5 h-3.5" />
358+
<History className="w-3.5 h-3.5 mr-1.5" />
341359
History ({history.length})
342360
{showHistory ? (
343-
<ChevronUp className="w-3 h-3" />
361+
<ChevronUp className="w-3 h-3 ml-1" />
344362
) : (
345-
<ChevronDown className="w-3 h-3" />
363+
<ChevronDown className="w-3 h-3 ml-1" />
346364
)}
347-
</button>
365+
</Button>
348366
{showHistory && (
349367
<Button
350368
variant="ghost"

‎src/lib/sovd-api.ts‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -590,7 +590,7 @@ export class SovdApiClient {
590590
goalId?: string
591591
): Promise<ActionGoalStatus> {
592592
const url = goalId
593-
? this.getUrl(`components/${componentId}/operations/${encodeURIComponent(operationName)}/status?goal_id=${goalId}`)
593+
? this.getUrl(`components/${componentId}/operations/${encodeURIComponent(operationName)}/status?goal_id=${encodeURIComponent(goalId)}`)
594594
: this.getUrl(`components/${componentId}/operations/${encodeURIComponent(operationName)}/status`);
595595

596596
const response = await fetchWithTimeout(url, {
@@ -643,7 +643,7 @@ export class SovdApiClient {
643643
goalId: string
644644
): Promise<ActionGoalResult> {
645645
const response = await fetchWithTimeout(
646-
this.getUrl(`components/${componentId}/operations/${encodeURIComponent(operationName)}/result?goal_id=${goalId}`),
646+
this.getUrl(`components/${componentId}/operations/${encodeURIComponent(operationName)}/result?goal_id=${encodeURIComponent(goalId)}`),
647647
{
648648
method: 'GET',
649649
headers: { 'Accept': 'application/json' },
@@ -670,7 +670,7 @@ export class SovdApiClient {
670670
goalId: string
671671
): Promise<ActionCancelResponse> {
672672
const response = await fetchWithTimeout(
673-
this.getUrl(`components/${componentId}/operations/${encodeURIComponent(operationName)}?goal_id=${goalId}`),
673+
this.getUrl(`components/${componentId}/operations/${encodeURIComponent(operationName)}?goal_id=${encodeURIComponent(goalId)}`),
674674
{
675675
method: 'DELETE',
676676
headers: { 'Accept': 'application/json' },

0 commit comments

Comments
 (0)