[OPIK-5385]: create eval suites improvement;#6054
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
...rontend/src/v2/pages-shared/datasets/AddEvaluationSuiteSidebar/AddEvaluationSuiteSidebar.tsx
Fixed
Show fixed
Hide fixed
![baz-reviewer[bot]](https://avatars.githubusercontent.com/in/933528?s=60&v=4){kind=small}
...rontend/src/v2/pages-shared/datasets/AddEvaluationSuiteSidebar/AddEvaluationSuiteSidebar.tsx
Outdated
Show resolved
Hide resolved
...rontend/src/v2/pages-shared/datasets/AddEvaluationSuiteSidebar/AddEvaluationSuiteSidebar.tsx
Outdated
Show resolved
Hide resolved
| <ConfirmDialog | ||
| open={confirmOpen} | ||
| setOpen={setConfirmOpen} | ||
| onCancel={submitHandler} | ||
| title="File can't be uploaded" | ||
| description="This file cannot be uploaded because it does not pass validation. If you continue, the evaluation suite will be created without any items. You can add items manually later, or go back and upload a valid file." | ||
| cancelText="Create empty evaluation suite" | ||
| confirmText="Go back" | ||
| /> |
There was a problem hiding this comment.
ConfirmDialog is duplicated in AddEditEvaluationSuiteDialog, should we extract a CsvUploadValidationConfirm component that takes typeLabel, onCancel, and setOpen?
Finding type: Code Dedup and Conventions | Severity: 🟢 Low
Want Baz to fix this for you? Activate Fixer
There was a problem hiding this comment.
I'd leave it. The ConfirmDialog usage is just 8 lines of props in each place, and the two instances have slightly different description text
|
🔄 Test environment deployment process has started Phase 1: Deploying base version You can monitor the progress here. |
|
✅ Test environment is now available! To configure additional Environment variables for your environment, run [Deploy Opik AdHoc Environment workflow] (https://github.com/comet-ml/comet-deployment/actions/workflows/deploy_opik_adhoc_env.yaml) Access Information
The deployment has completed successfully and the version has been verified. |
aadereiko
force-pushed
the
sashaa/OPIK-5385/create-eval-suites-improvement
branch
from
b9a50b0 to
5ebdaaa
Compare
| (error: AxiosError, action: "create" | "update") => { | ||
| const statusCode = get(error, ["response", "status"]); | ||
| const errorMessage = | ||
| get(error, ["response", "data", "message"]) || | ||
| get(error, ["response", "data", "errors", 0]) || | ||
| get(error, ["message"]); |
There was a problem hiding this comment.
Can we replace response.data.errors[0] with response.data.errors.join('; ') and fall back to message in handleMutationError?
response.data.errors[0] => response.data.errors.join('; ') + fallback to message
Finding type: Provide comprehensive error messages | Severity: 🟢 Low
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents:
Before applying, verify this suggestion against the current code. In
apps/opik-frontend/src/v2/pages-shared/datasets/AddEditEvaluationSuiteDialog/useEvaluationSuiteForm.ts
around lines 282 to 287, the handleMutationError function only reads the first error
string (response.data.errors[0]) and so the toast shows only one validation issue.
Change the error extraction so it first checks for response.data.errors being an array
and, if present, joins all strings with '; ' (e.g. errors.join('; ')); if that array is
not present, fall back to response.data.message, then to error.message, and finally to a
generic message. Use that aggregated string as the description in the toast and keep the
existing Conflict handling unchanged.
There was a problem hiding this comment.
It's ok, we use it this way everywhere else in the platform
There was a problem hiding this comment.
Got it, thanks for explaining the existing pattern—I'll keep this convention in mind and save it to memory after the PR is merged.
| <Separator className="mb-4" /> | ||
| <div className="mb-4"> | ||
| <h3 className="comet-body-s-accented">Evaluation criteria</h3> | ||
| <p className="comet-body-xs text-light-slate"> | ||
| Define the conditions required for the evaluation to pass | ||
| </p> | ||
| </div> | ||
| <div className="pt-1.5"> | ||
| <AssertionsField | ||
| editableAssertions={assertions} | ||
| onChangeEditable={(index, value) => { | ||
| setAssertions((prev) => { | ||
| const next = [...prev]; | ||
| next[index] = value; | ||
| return next; | ||
| }); | ||
| }} | ||
| onRemoveEditable={(index) => { | ||
| setAssertions((prev) => | ||
| prev.filter((_, i) => i !== index), | ||
| ); | ||
| }} | ||
| onAdd={() => setAssertions((prev) => [...prev, ""])} | ||
| placeholder="e.g. Response should be factually accurate and cite sources" | ||
| /> | ||
| <div className="mb-4 flex gap-4"> | ||
| <div className="flex flex-1 flex-col gap-1"> | ||
| <Label | ||
| htmlFor="runsPerItem" | ||
| className="comet-body-s-accented" |
There was a problem hiding this comment.
Evaluation criteria and Global assertions blocks duplicate across AddEvaluationSuiteSidebar and EvaluationSuiteItemForm, should we extract a shared EvaluationCriteriaForm component accepting runsInput, thresholdInput, assertions, and callbacks?
Finding types: Code Dedup and Conventions centralize shared logic | Severity: 🟠 Medium
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents:
Before applying, verify this suggestion against the current code. In
apps/opik-frontend/src/v2/pages-shared/datasets/AddEditEvaluationSuiteDialog/AddEditEvaluationSuiteDialog.tsx
around lines 133-214, the "Evaluation criteria" / "Global assertions" block duplicates
UI and state-management logic that also appears in AddEvaluationSuiteSidebar.tsx (lines
~223-381) and EvaluationSuiteItemForm.tsx (lines ~165-260). Extract this repeated JSX
and its related validation/inputs into a single shared component (e.g.
EvaluationCriteriaForm) that accepts props for runsInput, thresholdInput, assertions,
setAssertions, isEdit, and any label/placeholder text and returns the same UI and
callbacks. Replace the duplicated blocks in all three files with the new component and
update imports; ensure existing behavior (including onChange handlers, add/remove
assertion logic, and any accessibility ids) and styling are preserved and tests compile.
There was a problem hiding this comment.
It's ok, one of them is used in forms, the other is just simply used, so no need for a shared component;
There was a problem hiding this comment.
Got it, that makes sense given the different usage contexts—I'll keep this in mind and save it to memory once the PR is merged.
|
🌙 Nightly cleanup: The test environment for this PR ( |
|
🌙 Nightly cleanup: The test environment for this PR ( |
4 similar comments
|
🌙 Nightly cleanup: The test environment for this PR ( |
|
🌙 Nightly cleanup: The test environment for this PR ( |
|
🌙 Nightly cleanup: The test environment for this PR ( |
|
🌙 Nightly cleanup: The test environment for this PR ( |
aadereiko
force-pushed
the
sashaa/OPIK-5385/create-eval-suites-improvement
branch
from
5ebdaaa to
0c9c15b
Compare
| <div className="flex shrink-0 items-center justify-end gap-2 border-t px-6 py-4"> | ||
| <Button | ||
| variant="outline" | ||
| size="sm" | ||
| onClick={() => { | ||
| if (isDirty) { | ||
| requestConfirm(onClose); | ||
| } else { | ||
| onClose(); | ||
| } | ||
| }} | ||
| > |
There was a problem hiding this comment.
The relocated Save button is outside the form and can't access form.formState.isSubmitting, should we expose isSubmitting from EvaluationSuiteItemFormContainer and wire it into the footer button to disable/show a spinner per .agents/skills/opik-frontend/forms.md?
Finding type: AI Coding Guidelines | Severity: 🔴 High
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents:
Before applying, verify this suggestion against the current code. In
apps/opik-frontend/src/v1/pages/EvaluationSuiteItemsPage/EvaluationSuiteItemPanel/AddEvaluationSuiteItemPanel.tsx
around lines 128-149, the Save button was moved outside the form so it cannot observe
form.formState.isSubmitting and remains enabled during submission. Modify
EvaluationSuiteItemFormContainer to accept a new callback prop (e.g.,
onSubmittingChange: (isSubmitting: boolean) => void) and have the form container call
that callback whenever the internal form's isSubmitting changes. Then in this file, wire
that prop to capture isSubmitting into state and use it to disable the footer Save
button and show a loading/spinner state while submitting. Ensure the callback is called
on mount with the initial isSubmitting and on every submit lifecycle change so the
button reliably reflects the form state.
4 participants
Details
Summary
AddEditEvaluationSuiteDialogfrom ~600 lines into a hook + UI component splitMulti-step sidebar wizard
The new
AddEvaluationSuiteSidebarguides users through 3 steps:The edit flow still uses the existing
AddEditEvaluationSuiteDialogvia row actions.Shared CsvUploadDialog
The CSV upload loading overlay was duplicated in
v1/AddEditEvaluationSuiteDialog,v2/AddEditEvaluationSuiteDialog, and the new sidebar. Extracted intoCsvUploadDialogand reused in v2 dialog + sidebar.AddEditEvaluationSuiteDialog refactor
Split into:
useEvaluationSuiteForm.ts— state, mutations, validation, file handlingAddEditEvaluationSuiteDialog.tsx— pure UI (~250 lines)ResizableSidePanel enhancement
Added
closeButtonPositionprop ("left"|"right", defaults to"left") to support placing the close button on the right side of the header.Files changed
shared/ResizableSidePanel/ResizableSidePanel.tsx— addedcloseButtonPositionpropEvaluationSuitesPage/EvaluationSuitesPage.tsx— uses sidebar for createAddEditEvaluationSuiteDialog/AddEditEvaluationSuiteDialog.tsx— refactored, uses hook + shared CSV dialogAddEditEvaluationSuiteDialog/useEvaluationSuiteForm.ts— new, extracted form logicAddEvaluationSuiteSidebar/AddEvaluationSuiteSidebar.tsx— new, multi-step wizardCsvUploadDialog/CsvUploadDialog.tsx— new, shared upload modalChange checklist
Issues
AI-WATERMARK
Testing
npm run lintnpm run typecheckDocumentation