Skip to content

Commit 06f2c12

Browse files
committed
fix(webui): address PR review feedback for eval copy functionality
- Add useCallback wrappers for handleSaveEvalName and handleCopyEval to prevent unnecessary re-renders - Add error toast notifications for failed copy/rename operations - Fix Date.now() inefficiency in Eval.copy() batch loop by calling once per batch instead of per result - Replace deprecated onKeyPress with onKeyDown in ConfirmEvalNameDialog - Fix logging consistency to use 'id' variable instead of 'req.params.id' in error handler
1 parent 0b9ba7c commit 06f2c12

File tree

4 files changed

+64
-39
lines changed

4 files changed

+64
-39
lines changed

src/app/src/pages/eval/components/ConfirmEvalNameDialog.tsx

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -86,7 +86,7 @@ export const ConfirmEvalNameDialog = ({
8686
}
8787
};
8888

89-
const handleKeyPress = (event: React.KeyboardEvent) => {
89+
const handleKeyDown = (event: React.KeyboardEvent) => {
9090
if (event.key === 'Enter' && !event.shiftKey) {
9191
event.preventDefault();
9292
handleConfirm();
@@ -120,7 +120,7 @@ export const ConfirmEvalNameDialog = ({
120120
label={label}
121121
value={name}
122122
onChange={(e) => setName(e.target.value)}
123-
onKeyPress={handleKeyPress}
123+
onKeyDown={handleKeyDown}
124124
margin="normal"
125125
error={!!error}
126126
helperText={

src/app/src/pages/eval/components/ResultsView.tsx

Lines changed: 58 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -388,48 +388,72 @@ export default function ResultsView({
388388
[updateColumnVisibility],
389389
);
390390

391-
const handleSaveEvalName = async (newName: string) => {
392-
invariant(config, 'Config must be loaded before updating its description');
393-
const newConfig = { ...config, description: newName };
394-
395-
const response = await callApi(`/eval/${evalId}`, {
396-
method: 'PATCH',
397-
headers: {
398-
'Content-Type': 'application/json',
399-
},
400-
body: JSON.stringify({ config: newConfig }),
401-
});
391+
const handleSaveEvalName = React.useCallback(
392+
async (newName: string) => {
393+
try {
394+
invariant(config, 'Config must be loaded before updating its description');
395+
const newConfig = { ...config, description: newName };
402396

403-
if (!response.ok) {
404-
throw new Error('Failed to update eval name');
405-
}
397+
const response = await callApi(`/eval/${evalId}`, {
398+
method: 'PATCH',
399+
headers: {
400+
'Content-Type': 'application/json',
401+
},
402+
body: JSON.stringify({ config: newConfig }),
403+
});
406404

407-
setConfig(newConfig);
408-
};
405+
if (!response.ok) {
406+
throw new Error('Failed to update eval name');
407+
}
408+
409+
setConfig(newConfig);
410+
} catch (error) {
411+
console.error('Failed to update eval name:', error);
412+
showToast(
413+
`Failed to update eval name: ${error instanceof Error ? error.message : 'Unknown error'}`,
414+
'error',
415+
);
416+
throw error;
417+
}
418+
},
419+
[config, evalId, setConfig, showToast],
420+
);
409421

410-
const handleCopyEval = async (description: string) => {
411-
invariant(evalId, 'Eval ID must be set before copying');
422+
const handleCopyEval = React.useCallback(
423+
async (description: string) => {
424+
try {
425+
invariant(evalId, 'Eval ID must be set before copying');
412426

413-
const response = await callApi(`/eval/${evalId}/copy`, {
414-
method: 'POST',
415-
headers: {
416-
'Content-Type': 'application/json',
417-
},
418-
body: JSON.stringify({ description }),
419-
});
427+
const response = await callApi(`/eval/${evalId}/copy`, {
428+
method: 'POST',
429+
headers: {
430+
'Content-Type': 'application/json',
431+
},
432+
body: JSON.stringify({ description }),
433+
});
420434

421-
if (!response.ok) {
422-
throw new Error('Failed to copy evaluation');
423-
}
435+
if (!response.ok) {
436+
throw new Error('Failed to copy evaluation');
437+
}
424438

425-
const { id: newEvalId, distinctTestCount } = await response.json();
439+
const { id: newEvalId, distinctTestCount } = await response.json();
426440

427-
// Open in new tab (Google Docs pattern)
428-
window.open(`/eval/${newEvalId}`, '_blank');
441+
// Open in new tab (Google Docs pattern)
442+
window.open(`/eval/${newEvalId}`, '_blank');
429443

430-
// Show success toast
431-
showToast(`Copied ${distinctTestCount.toLocaleString()} results successfully`, 'success');
432-
};
444+
// Show success toast
445+
showToast(`Copied ${distinctTestCount.toLocaleString()} results successfully`, 'success');
446+
} catch (error) {
447+
console.error('Failed to copy evaluation:', error);
448+
showToast(
449+
`Failed to copy evaluation: ${error instanceof Error ? error.message : 'Unknown error'}`,
450+
'error',
451+
);
452+
throw error;
453+
}
454+
},
455+
[evalId, showToast],
456+
);
433457

434458
const handleDeleteEvalClick = async () => {
435459
if (window.confirm('Are you sure you want to delete this evaluation?')) {

src/models/eval.ts

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1069,12 +1069,13 @@ export default class Eval {
10691069
}
10701070

10711071
// Map to new eval with new IDs and timestamps
1072+
const now = Date.now();
10721073
const copiedResults = batch.map((result) => ({
10731074
...result,
10741075
id: randomUUID(),
10751076
evalId: newEvalId,
1076-
createdAt: Date.now(),
1077-
updatedAt: Date.now(),
1077+
createdAt: now,
1078+
updatedAt: now,
10781079
}));
10791080

10801081
// Insert batch

src/server/routes/eval.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -648,7 +648,7 @@ evalRouter.post('/:id/copy', async (req: Request, res: Response): Promise<void>
648648

649649
logger.error('Failed to copy eval', {
650650
error,
651-
evalId: req.params.id,
651+
evalId: id,
652652
});
653653
res.status(500).json({ error: 'Failed to copy evaluation' });
654654
}

0 commit comments

Comments
 (0)