[OPIK-7042] [FE] feat: Optimization Studio new-run sidebar#7291
Conversation
…leanup First slice (verified, typecheck + lint clean): - Fix the Equals metric default to read reference_key from DEFAULT_EQUALS_METRIC_CONFIGS (was wrongly pulling the JSON-schema validator constant; harmless today since both are "", but incorrect). - Widen StudioLlmModel.model off the legacy PROVIDER_MODEL_TYPE enum to `string` (model ids come from the dynamic registry); drop the now- redundant cast in the v2 studio-config converter. Sidebar conversion + god-hook split + dataset-picker pagination follow on this branch. Part of OPIK_6528. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Convert the new-run wizard from the full-page /optimizations/new route into a right ResizableSidePanel opened over the runs list (Figma 548:27872): - NewRunSidebar hosts the form logic + 2-column body with a "New optimization run" top bar and an Optimize/Cancel footer; mounted only while open. - The list opens it via `?new` (+ `?template`/`?rerun` to pre-fill); the studio-templates row, empty state and the single-run header now set those params instead of navigating to /new. - Remove the /optimizations/new route + its guard usage; delete the orphaned OptimizationsNewPage / OptimizationsNewHeader. Routing/open-close behavior is not runtime-verified (no app run); typecheck, lint and the optimization test suites are green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… arms - Replace the silent 1000-dataset list (used only to resolve the selected dataset's name on submit) with a single useDatasetById; the picker itself was already the searchable DatasetSelectBox. - Remove the dead commented-out Evolutionary optimizer params schema (Evolutionary isn't offered in OPTIMIZER_OPTIONS). - Add the Code metric to OPTIMIZATION_METRIC_OPTIONS (it already has a full schema arm, config component and submit handling, just wasn't selectable). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Break useOptimizationsNewFormHandlers (~305 lines) into cohesive hooks under formHandlers/: useOptimizerFormHandlers, useMetricFormHandlers, useModelFormHandlers and useSubmitOptimization. The main hook (~108 lines) now just composes them plus the watches, selected-dataset lookup, and the dataset/name handlers. Also drop the dead code the sidebar conversion left behind: the unused handleCancel and the breadcrumb wiring for the removed /optimizations/new route. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…nput alignment & variable validation
- Render the form during demo-dataset prep instead of a full-panel spinner; the item-source field shows a local "Preparing dataset..." state and submit stays disabled until ready.
- Align the Name input to 32px (dimension="sm") to match the right-rail controls and Figma.
- Block submit when {{variables}} in the prompt or G-Eval metric aren't columns in the selected item source, with an inline reason (prevents runs that fail at evaluation time).
- Metric settings / config-field cleanup (field cards, GEval & reference-key fields, dataset-variable hints).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
![baz-reviewer[bot]](https://avatars.githubusercontent.com/in/933528?s=60&v=4){kind=small}
| <div className="flex w-72 flex-col gap-6"> | ||
| <div className="space-y-4"> | ||
| <div className="space-y-2"> | ||
| <div className="flex items-center gap-2"> | ||
| <Label htmlFor="reference_key" className="text-sm"> | ||
| Reference key | ||
| </Label> | ||
| <ExplainerIcon | ||
| {...EXPLAINERS_MAP[EXPLAINER_ID.metric_reference_key]} | ||
| /> | ||
| </div> | ||
| <Label htmlFor="reference_key" className="text-sm"> | ||
| Reference key | ||
| </Label> | ||
| <Input | ||
| id="reference_key" | ||
| placeholder="e.g., answer or $.scores[?(@.name=='Useful')].value" |
There was a problem hiding this comment.
Duplicated reference-key input block
Equals, JsonSchema, Levenshtein, and NumericalSimilarity each inline the same reference-key input + dataset-variable hint block, so styling, tooltip, or dataset-variable changes have to be kept in sync across four files — should we extract a shared ReferenceKeyField component with props for placeholder/value and metric-specific defaults?
Want Baz to fix this for you? Activate Fixer
There was a problem hiding this comment.
Commit fc32aad addressed this comment by extracting the repeated reference-key label/input/dataset-variable hint into a shared ReferenceKeyField component and switching all four metric configs to use it. This centralizes placeholder and hint behavior so styling and dataset-variable changes only need to be updated in one place.
| hasMissingVariables | ||
| } | ||
| > | ||
| {isSubmitting && <Loader2 className="mr-2 size-4 animate-spin" />} |
There was a problem hiding this comment.
Spinner animates SVG directly
Loader2 gets className="mr-2 size-4 animate-spin" directly, so the Lucide <svg> is animated in place and can do extra repaint work — should we wrap it in a span and move animate-spin there, as .agents/skills/opik-frontend/performance.md requires?
Want Baz to fix this for you? Activate Fixer You can also update your AI coding guidelines based on this comment by apply pr to [branch name]
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
apps/opik-frontend/src/v2/pages/OptimizationsPage/OptimizationsNewPage/OptimizationsNewPageContent.tsx
around lines 94-96 inside the `Optimize prompt` Button rendering, the `Loader2` lucide
icon has `className="mr-2 size-4 animate-spin"`, which applies `animate-spin` directly
to the underlying `<svg>`. Refactor so `animate-spin` is applied to a non-SVG wrapper
element (e.g., a `span` around the icon) and keep only sizing/spacing classes on the
wrapper or the icon without `animate-spin`. Ensure the visual spacing and spinner
behavior remain the same.
There was a problem hiding this comment.
Commit 2865dff addressed this comment by moving animate-spin off the Lucide Loader2 SVG and onto a wrapping <span>. The icon now keeps only its sizing class, preserving the spinner behavior while avoiding direct SVG animation.
| const datasetNameValue = selectedDataset?.name || ""; | ||
|
|
||
| if (!datasetNameValue) return; |
There was a problem hiding this comment.
Silent no-op on unresolved dataset
handleSubmit can return early while useDatasetById is still loading or has failed, because datasetNameValue falls back to "" from selectedDataset?.name and the button isn't disabled on dataset lookup state, so a valid form leaves Optimize prompt doing nothing until the fetch resolves — could we block submit until the query settles or surface the lookup error?
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/OptimizationsPage/OptimizationsNewPage/formHandlers/useSubmitOptimization.ts
around lines 39-41 inside `handleSubmit`, don’t silently `return` when
`selectedDataset?.name` is empty. Instead, treat “dataset not ready / failed” as an
error: either (a) add an explicit `datasetReady`/`datasetLoading`/`datasetError` input
to this hook and block submission until it’s ready (and/or throw a user-facing error),
or (b) set form-level error and avoid calling the mutation/navigation until the dataset
fetch has succeeded. Then ensure the UI disable logic in OptimizationsNewPageContent.tsx
(the button handler around the checks at lines 87-92) includes the dataset lookup
settlement state, so the “Optimize prompt” button is disabled while the dataset
query is loading and shows an error if the lookup fails.
There was a problem hiding this comment.
Commit 2865dff addressed this comment by disabling “Optimize prompt” while the selected dataset lookup is loading or has errored, and by surfacing an inline error message when the lookup fails. That prevents the previous silent no-op during unresolved dataset fetches, even though the submit handler still guards against a missing dataset name.
| const missingDatasetVariables = useMemo(() => { | ||
| if (datasetVariables.length === 0) return []; | ||
|
|
There was a problem hiding this comment.
False missing-column submit block
missingDatasetVariables is built from datasetVariables, and useDatasetSamplePreview fills that list from Object.keys(datasetSample) where datasetSample is just datasetItemsData.content[0].data, so one sample row is treated as the schema and {{variable}} references for optional columns get flagged missing when they're absent there, which keeps Optimize prompt disabled even though the dataset includes those columns — should we validate against datasetItemsData.columns instead?
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/OptimizationsPage/OptimizationsNewPage/useOptimizationsNewFormHandlers.ts
around lines 50-76, the `missingDatasetVariables` logic currently treats
`datasetVariables` as the authoritative list of available `{{mustache}}` tags, but
`datasetVariables` comes from `useDatasetSamplePreview` (based on only the first sample
row). Refactor so the “allowed variables” set is derived from the dataset’s
schema-backed column list (the `datasetItemsData.columns` you already fetch alongside
items) rather than `Object.keys(datasetSample)`. Concretely, update
`useDatasetSamplePreview` to return `datasetColumns` (or rename `datasetVariables`)
based on `datasetItemsData.columns`, then in `useOptimizationsNewFormHandlers` compute
`missingDatasetVariables` by comparing against that schema list (and keep the current
“skip until loaded/empty” behavior so it doesn’t block during loading).
There was a problem hiding this comment.
Commit 2865dff addressed this comment by changing useDatasetSamplePreview to derive datasetVariables from datasetItemsData.columns instead of Object.keys(datasetSample). missingDatasetVariables still compares against datasetVariables, but that list is now schema-backed, so optional columns no longer get flagged missing just because they were absent in the first sample row.
| const ALGORITHM_ICON_MAP: Partial<Record<OPTIMIZER_TYPE, IconConfig>> = { | ||
| [OPTIMIZER_TYPE.GEPA]: { icon: Dna, color: "#12A594" }, | ||
| [OPTIMIZER_TYPE.HIERARCHICAL_REFLECTIVE]: { icon: Network, color: "#6E56CF" }, | ||
| [OPTIMIZER_TYPE.EVOLUTIONARY]: { icon: GitBranch, color: "#30A46C" }, | ||
| }; |
There was a problem hiding this comment.
Sidebar icons ignore theme tokens
ALGORITHM_ICON_MAP and METRIC_ICON_MAP hard-code hex colors, and renderIconLabel inlines them instead of using the theme-variable path from apps/opik-frontend/src/main.scss, so the optimization sidebar icons won't pick up dark-mode or brand palette updates — should we switch these to token-backed classes/variables, as .agents/skills/opik-frontend/ui-components.md suggests?
Want Baz to fix this for you? Activate Fixer You can also update your AI coding guidelines based on this comment by apply pr to [branch name]
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
apps/opik-frontend/src/v2/pages/OptimizationsPage/OptimizationsNewPage/OptimizationsNewConfigSidebar.tsx
around lines 47-72, `ALGORITHM_ICON_MAP`, `METRIC_ICON_MAP`, and `renderIconLabel`
hard-code hex colors and apply them via inline `style={{ color: ... }}`, which bypasses
the theme variables/class system (e.g., `--color-*` / `--chart-*`) and breaks
dark-mode/brand palette updates. Refactor by removing the hex `color` values from both
icon maps and instead reference theme-backed tokens: either use named CSS
variable-backed colors (e.g., set icon color to `var(--color-... )` / `var(--chart-...
)`) or convert `color` to a Tailwind/class-based token and pass it via `className` so it
follows the existing theme pipeline. If the needed tokens don’t exist yet, add/extend
named CSS variables in the shared theme stylesheet (main.scss or the appropriate theme
file) and then reference those tokens from this component.
There was a problem hiding this comment.
Commit b8e85d7 addressed this comment by replacing the inline hex icon colors with CSS custom properties in main.scss and updating ALGORITHM_ICON_MAP/METRIC_ICON_MAP to reference those theme-backed variables. This moves the sidebar icons onto the shared theme token path so they can follow palette updates.
| playgroundRoute.addChildren([playgroundIndexRoute]), | ||
| optimizationsRoute.addChildren([ | ||
| optimizationsListRoute, | ||
| optimizationsNewRoute.addChildren([optimizationsNewIndexRoute]), | ||
| optimizationCompareRedirectRoute, | ||
| optimizationBaseRoute.addChildren([optimizationRoute, trialRoute]), | ||
| ]), |
There was a problem hiding this comment.
Legacy wizard links no longer work
/optimizations/new no longer aliases to .../optimizations?new=true, so old deep links and bookmarks can fall through the remaining /optimizations/* compat route and get consumed by /$optimizationId instead of opening the new-run sidebar — should we add the redirect back?
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/router.tsx around lines 653-658 where
`optimizationsRoute.addChildren([...])` is defined, reintroduce an alias/redirect for
the legacy path `.../optimizations/new` that maps to `.../optimizations?new=true` (and
ensure it handles the index case previously covered by the removed
`optimizationsNewRoute`/`optimizationsNewIndexRoute`). Add the redirect as a child route
under `optimizationsRoute` so it is matched before any catch-all/details routes (e.g.,
the `/$optimizationId` detail route) can consume the URL. Also verify around lines
420-437 that the deleted route definitions for `/new` were not replaced elsewhere; if
they were not, restore the redirect using the same routing helpers already used in this
file for compare/redirect routes.
There was a problem hiding this comment.
Commit fc32aad addressed this comment by reintroducing a legacy /optimizations/new child route that redirects to the optimizations list with ?new=true open. It is registered before the /$optimizationId route, so old deep links no longer get consumed by the detail route.
| showIcon | ||
| ? (option) => ( | ||
| <div className="flex min-w-0 items-center gap-2"> | ||
| <Database className="size-4 shrink-0 text-[#F4B400]" /> |
There was a problem hiding this comment.
Dataset icon bypasses theme palette
The Database icon uses text-[#F4B400] instead of the existing --chart-yellow token, so it bypasses theme updates and brand changes — should we switch it to var(--chart-yellow) or a named theme class, as .agents/skills/opik-frontend/ui-components.md suggests?
Want Baz to fix this for you? Activate Fixer You can also update your AI coding guidelines based on this comment by apply pr to [branch name]
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
apps/opik-frontend/src/v2/pages-shared/experiments/DatasetSelectBox/DatasetSelectBox.tsx
around lines 65-74 (the `renderTitle` branch that renders the `Database` icon), remove
the hard-coded color class `text-[#F4B400]`. Refactor the icon styling to use the
existing theme/design token defined in main.scss (i.e., reference `--chart-yellow` /
var(--chart-yellow) or an existing named theme utility/class that maps to it) so the
color updates with future brand/theme changes. Ensure the new styling still matches the
Figma gold color and passes the UI guideline
`.agents/skills/opik-frontend/ui-components.md`.
There was a problem hiding this comment.
Commit 2865dff addressed this comment by replacing the hard-coded text-[#F4B400] color with text-[color:var(--chart-yellow)] on the Database icon. This makes the icon follow the shared theme token instead of bypassing palette updates.
- Variable check uses the dataset's server-side column schema (datasetItemsData.columns) instead of one sampled row, so optional columns absent from row 1 no longer falsely block submit (Baz: false missing-column block). - Submit is disabled while the selected-dataset lookup is loading and surfaces an error if it fails, closing the silent no-op window (Baz: silent no-op on unresolved dataset). - Spinner: wrap Loader2 in an animate-spin span so the SVG isn't animated in place (perf guideline). - Dataset icon uses the --chart-yellow theme token instead of a hard-coded hex. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
| (messages ?? []).forEach((message) => | ||
| collect( | ||
| typeof message.content === "string" ? message.content : undefined, | ||
| ), |
There was a problem hiding this comment.
Structured prompt vars bypass submit gate
missingDatasetVariables only inspects string message.content, so LLMMessage.content arrays lose their {{...}} tags and hasMissingVariables can miss required dataset fields, letting OptimizationsNewPageContent submit prompts that still reference them — should we flatten structured parts with the existing content-to-text helper before safelyGetPromptVariables?
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/OptimizationsPage/OptimizationsNewPage/useOptimizationsNewFormHandlers.ts
around lines 59-62 inside the logic that builds `missingDatasetVariables` / `referenced`
from `messages` (the loop that calls `safelyGetPromptVariables(text)`), fix the bug
where only string `message.content` is inspected and array-based content loses `{{...}}`
tags. Refactor so that for each `LLMMessage`, you first convert/flatten `content`
(string or Array<...>) into a single text string using the existing
“content-to-text” helper used elsewhere, then run `safelyGetPromptVariables` on that
normalized text. This ensures `hasMissingVariables` correctly blocks submit when tags
appear inside text parts of structured content arrays.
There was a problem hiding this comment.
Commit fc32aad addressed this comment by flattening each message’s structured content with extractMessageContent(message.content) before passing it to safelyGetPromptVariables. This lets {{...}} tags inside multipart/array message content be detected, so the missing-variable submit gate can no longer be bypassed in that case.
| const datasetVariables = useMemo( | ||
| () => (datasetItemsData?.columns ?? []).map((column) => column.name), | ||
| [datasetItemsData?.columns], |
There was a problem hiding this comment.
Loading columns skips validation
datasetVariables falls back to [] while /datasets/{id}/items is still loading, so useOptimizationsNewFormHandlers treats datasetVariables.length === 0 as “skip validation” and OptimizationsNewPageContent lets submit through once useDatasetById finishes, which means users can bypass the missing-variable guard before the columns arrive — should we expose a loading state for dataset columns and disable submit until it resolves?
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/OptimizationsPage/OptimizationsNewPage/useDatasetSamplePreview.ts
around lines 36-38 inside the `useDatasetSamplePreview` hook, `datasetVariables` is
computed as `datasetItemsData?.columns ?? []`, which turns the pending state of
`/datasets/{id}/items` into an empty array. Refactor this hook to also return a
`datasetVariablesLoading` (and optionally `datasetVariablesError`) derived from
`useDatasetItemsList`’s status, and change the logic so submit/validation is not
treated as “no variables” while columns are still loading (e.g., return
`null`/undefined or a loading flag rather than `[]`). Then update
`OptimizationsNewPageContent` / `useOptimizationsNewFormHandlers` to block submit and/or
missing-variable guard until `datasetVariablesLoading` is false, matching the
dataset-by-id loading/error behavior.
There was a problem hiding this comment.
Commit fc32aad addressed this comment by adding areColumnsLoading to useDatasetSamplePreview and propagating it into useOptimizationsNewFormHandlers. Submit is now gated while either the dataset or its columns are still loading, preventing the temporary empty-array state from bypassing the missing-variable check.
- Flatten structured (array) message content via extractMessageContent before pulling {{vars}}, so prompts with multipart parts are still checked by the missing-variable gate.
- Gate submit while the dataset's columns are still loading (areColumnsLoading), so the missing-variable check can't be bypassed in the window before columns arrive.
- Restore legacy /optimizations/new -> /optimizations?new=true redirect so old deep links open the new-run sidebar instead of hitting the /$optimizationId detail route.
- Extract a shared ReferenceKeyField used by the Equals/JSON Schema/Levenshtein/Numerical Similarity metric configs (was duplicated 4x).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…kens Replaces hard-coded hex on the algorithm/metric icon maps with named --optimizer-icon-* / --metric-icon-* CSS variables in main.scss (matching the existing --template-icon-* / --chart-* convention), so the icons follow the theme system instead of bypassing it. Colors are unchanged. Addresses the last Baz review comment. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ce-key field
- Validation is now lazy (mode: onSubmit + reValidateMode: onChange): RHF field errors and the {{variable}} mismatch no longer appear while editing, switching metrics, or before a dataset is picked. The 'Optimize prompt' button stays clickable and reveals what's missing on click (still won't submit with errors / missing variables).
- Reference-key input aligned to 32px (dimension="sm") to match the Algorithm/Item source/Metric controls.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
| // Validation feedback is lazy: errors (RHF field errors + the variable | ||
| // mismatch) only appear once the user has tried to submit, not while they're | ||
| // still editing / switching metrics / before a dataset is picked. | ||
| const [submitAttempted, setSubmitAttempted] = useState(false); | ||
|
|
||
| <div className="flex flex-col gap-6 xl:flex-row"> | ||
| const onSubmit = useCallback(() => { | ||
| setSubmitAttempted(true); | ||
| if (hasMissingVariables) { | ||
| // Surface RHF field errors alongside the mismatch, but don't submit. | ||
| void form.trigger(); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Lazy gate misses field errors
submitAttempted only gates the missing-variable banner, so OptimizationsNewConfigSidebar still renders RHF FormMessage errors immediately and switching item sources can show Reference key is required before the first Optimize prompt click — should we stop validating these changes eagerly or hide field errors until submit has been attempted?
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/OptimizationsPage/OptimizationsNewPage/OptimizationsNewPageContent.tsx
around lines 45-58, the `submitAttempted` state is set in `onSubmit` but is not used to
gate field-level RHF errors. Update this by either (a) passing `submitAttempted` down to
`OptimizationsNewConfigSidebar` and using it to suppress rendering of RHF
`FormMessage`/`form.formState.errors.metricParams` until submit is attempted, or (b)
adjusting the dataset/metric change path that clears `reference_key` to avoid eager
validation (remove/avoid `setValue(..., { shouldValidate: true })` for intermediate
switches). Ensure the missing-variable banner logic and the click handler remain
consistent so that errors only appear after the first “Optimize prompt” click, not
immediately on source changes.
There was a problem hiding this comment.
Commit eb84c94 addressed this comment by removing the eager shouldValidate: true calls from the dataset/metric change paths, so switching sources no longer immediately triggers RHF field errors. It also switched submit tracking to RHF isSubmitted and wrapped the button with form.handleSubmit(...), keeping errors hidden until a submit attempt occurs.
There was a problem hiding this comment.
Addressed in eb84c94. The eager validation was coming from the change handlers, not the banner: dataset/metric/optimizer/prompt setValue calls forced shouldValidate: true, so RHF re-ran the resolver on every change despite mode: "onSubmit", surfacing field FormMessage errors while editing. Dropped shouldValidate from all of them (kept shouldDirty). Now field errors respect mode: "onSubmit" and only appear once form.handleSubmit runs — same trigger as the missing-variable banner, which is now gated on form.formState.isSubmitted. So switching item sources no longer flashes "Reference key is required" before the first submit.
| <div className="flex items-center gap-2"> | ||
| <Button | ||
| onClick={onSubmit} | ||
| disabled={ | ||
| isSubmitting || | ||
| isPreparingDataset || | ||
| isDatasetLoading || | ||
| isDatasetError | ||
| } |
There was a problem hiding this comment.
Enter-key submit is broken
This block bypasses the shadcn/RHF form pattern from .agents/skills/opik-frontend/forms.md: the primary action is Button onClick={onSubmit} and disabled follows useSubmitOptimization's isSubmitting, so Enter in the inputs can't start an optimization run and RHF never controls the busy state — should we wrap it in <Form {...form}>/<form onSubmit={form.handleSubmit(...) }> and drive disabled from form.formState.isSubmitting?
Want Baz to fix this for you? Activate Fixer You can also update your AI coding guidelines based on this comment by apply pr to [branch name]
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
apps/opik-frontend/src/v2/pages/OptimizationsPage/OptimizationsNewPage/OptimizationsNewPageContent.tsx
around lines 107-115, the primary action is wired as a Button onClick={onSubmit} and the
busy/disabled state relies on the hook’s custom isSubmitting, which bypasses the
shadcn/RHF required submit lifecycle and prevents Enter-key submission. Refactor this
section to use the standard shadcn/RHF pattern: wrap the content in <Form {...form}> and
add a real <form onSubmit={form.handleSubmit(handleSubmit)}>, then make the primary
Button a submit button (type="submit") and set disabled using
form.formState.isSubmitting. Update the submitAttempted logic to trigger validation
feedback inside the onSubmit handler (via form.trigger / form.handleSubmit) rather than
relying on an onClick-only flow.
There was a problem hiding this comment.
Partly addressed in eb84c94, with one intentional deviation.
Adopted: submit and busy state now flow through RHF. The primary action is onClick={form.handleSubmit(onValid)} (RHF validates the schema first, then runs onValid), and disabled/Starting... are driven by form.formState.isSubmitting — useSubmitOptimization no longer owns its own isSubmitting state or calls trigger().
Deviation — no <form onSubmit> wrapper / Enter-to-submit: this panel is a rich multi-field editor with several nested interactive controls that render plain <button>s without type="button" (prompt library menu, model-settings popover, metric-config popovers, the message-list add/remove controls). Wrapping the whole thing in a <form> would make Enter in any text input submit the run, and make any of those nested buttons act as an implicit submit — a worse UX and a real footgun. For a heavyweight "start a run" action (not a quick login-style form), keeping submit an explicit button click is deliberate. Happy to revisit if we first audit every nested control for type="button", but that's a broader change across the shared LLM components than this PR should carry.
There was a problem hiding this comment.
Thanks, that makes sense — the RHF-driven submit flow addresses the core concern, and the explicit-click choice here is a reasonable constraint given the nested controls. I’ll save this to memory once the PR is merged.
There was a problem hiding this comment.
Commit c6fa3cb addressed this comment by routing the primary action through RHF’s handleSubmit, so validation and submission now flow through the form lifecycle instead of a custom onClick path. It also moved the busy-state handling onto RHF form state. The <form> wrapper was intentionally not added, so Enter-to-submit remains unsupported.
… RHF Address Baz round-2 findings on the new-run sidebar form: - #10 Lazy gate missed RHF field errors: change handlers still forced `shouldValidate: true`, so field errors surfaced eagerly while editing despite `mode: "onSubmit"`. Dropped it everywhere (dataset/metric/ optimizer/prompt/demo-dataset setValue) so field errors and the variable-mismatch footer both surface only after a submit attempt. - #11 Submit + busy state now flow through `form.handleSubmit(onValid)` and `form.formState.isSubmitting`/`isSubmitted`, instead of a manual `trigger()` + local `isSubmitting` state and a `submitAttempted` flag. `useSubmitOptimization` no longer re-validates or owns a busy flag. Enter-key submit is intentionally not wired: the form is a rich multi-field panel with nested untyped buttons (prompt library menu, metric-config popovers, model settings), so a `<form>` wrapper would make Enter/stray buttons submit. Submit stays an explicit button click. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
RHF's handleSubmit re-throws when the submit handler rejects, and the create mutation uses mutateAsync (rejects on error). The mutation already surfaces API errors via its onError toast, so the re-thrown rejection was only producing an unhandled promise rejection in the console. Catch it at the onClick call site; RHF has already reset isSubmitting before re-throwing. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Adopt the shadcn/RHF form pattern from forms.md instead of calling
form.handleSubmit from an onClick handler:
- Wrap the new-run content in <form onSubmit={form.handleSubmit(onValid)}>,
submit button type="submit", cancel type="button". Enter now submits.
- A <button> without an explicit type inside a <form> defaults to submit,
so add type="button" to the non-submit buttons that render inside this
form's DOM: LoadedPromptDisplay (detach), LLMPromptMessageActions (save),
LLMPromptMessage (show more/less), AlgorithmConfigs (use prompt model),
DatasetVariablesHint (variable chips). Pure correctness fixes — these
should never submit any form. Buttons in portaled Dialog/Dropdown content
are outside the form DOM and were left as-is.
RHF still owns validation and busy state (form.formState.isSubmitting /
isSubmitted); the submit handler swallows the settled rejection since the
create mutation already toasts API errors via its onError.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
| // Native form submit (submit button / Enter). RHF's handleSubmit re-throws | ||
| // when the submit handler rejects; the create mutation already toasts API | ||
| // errors via its `onError`, so swallow the settled rejection to avoid an | ||
| // unhandled promise rejection. RHF has already reset `isSubmitting` by the | ||
| // time it re-throws, so state stays consistent. | ||
| const handleFormSubmit = useCallback( | ||
| (event: React.FormEvent) => { | ||
| void form | ||
| .handleSubmit(onValid)(event) | ||
| .catch(() => {}); |
There was a problem hiding this comment.
Uncovered submit-path regression
OptimizationsNewPageContent now uses <form onSubmit> plus .catch(() => {}), so Enter-key submits and the error path could regress without coverage — could we add a regression test in OptimizationsNewPageContent/NewRunSidebar that submits via Enter and asserts useOptimizationCreateMutation.onError still runs, as AGENTS.md and .agents/skills/opik-frontend/testing.md require?
Want Baz to fix this for you? Activate Fixer You can also update your AI coding guidelines based on this comment by apply pr to [branch name]
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
apps/opik-frontend/src/v2/pages/OptimizationsPage/OptimizationsNewPage/OptimizationsNewPageContent.tsx
around lines 60-69 (the new `handleFormSubmit` callback that wraps
`form.handleSubmit(onValid)` with a `.catch(() => {})`), add a regression test to cover
the Enter-key submission path. Create or update the relevant test for
`OptimizationsNewPageContent` (and/or `NewRunSidebar` if it owns the submit control) to
render the component, fill the required form fields, trigger a submit via pressing Enter
on an input inside the form, and assert that the create mutation’s `onError` is
reached (e.g., by mocking `useOptimizationCreateMutation` to throw/reject and verifying
the mocked `onError` handler or the resulting error toast/UI is shown). Ensure the test
specifically verifies the swallow-catch behavior does not prevent the mutation error
handling from executing.
…ic from new-run options Removes the "Custom code" (METRIC_TYPE.CODE) entry from OPTIMIZATION_METRIC_OPTIONS so it is no longer selectable in the new-run sidebar. The backend exec path exists (OPIK-3932 / #4817) but was never exercised live here and has FE validation / error-surfacing gaps, so exposing it now risks silent "Initialized" hangs on bad code. OPIK-7042's scope also explicitly excludes the code metric from options. Re-exposing it (with pre-submit syntax validation, dataset-column checks, and surfaced run errors) is tracked in OPIK-7172. The CODE type, schema arm, and CodeMetricConfigs component are left in place for that follow-up. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…nd simplify prompt helper Delete comments that restate the code, keeping only ticket/Figma refs and genuine gotchas; trim the rest to their essential rationale. Also rewrite safelyGetPromptVariables to reuse getPromptMustacheTags and drop its dead `!== "."` filter. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…o constants Static ALGORITHM_ICON_MAP / METRIC_ICON_MAP lookup tables don't belong in a component file — move them (and the IconConfig type) into constants/optimizations next to the existing option lists so they're reusable. Also folds in the new-run sidebar shell/form split. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
| export const safelyGetPromptVariables = (template: string): string[] => { | ||
| try { | ||
| return getPromptMustacheTags(template) | ||
| .map((name) => name.split(".")[0]) | ||
| .filter((name) => name.length > 0); |
There was a problem hiding this comment.
Untested prompt parsing branches
src/lib/prompt.test.ts doesn’t cover safelyGetPromptVariables() or the new missingDatasetVariables branch, so regressions in dotted tags, empty input, or malformed templates could slip through — should we add unit coverage here, as .agents/skills/opik-frontend/testing.md suggests?
Want Baz to fix this for you? Activate Fixer You can also update your AI coding guidelines based on this comment by apply pr to [branch name]
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
apps/opik-frontend/src/lib/prompt.ts around lines 28-32 (the logic that normalizes
mustache tag names via `name.split(".")[0]` before the `missingDatasetVariables` check),
add unit tests that lock in this dotted-tag behavior and edge cases. Extend
apps/opik-frontend/src/lib/prompt.test.ts with cases for: empty template/input, dotted
tags (e.g., `{{dataset.foo}}` should be treated as `dataset`), malformed templates
causing mustache.parse to throw (should return false/handled value), and a focused
assertion that the `missingDatasetVariables` branch triggers (or not) based on the
normalized variables. Follow the existing test patterns in that file and ensure both the
parsing/normalization function output and the downstream missingDatasetVariables
decision are covered.
|
🔄 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. |
…ric title above box
The new-run form wrapped its whole body in a <form>, so every native <button>
inside (dropdowns, settings, prompt actions) defaulted to type="submit" and
submitted on click. Move the body outside a standalone <form id> and link only
the "Optimize prompt" button via form={id}, so nothing else can submit it.
Also move the "Metric settings" heading above its card (out of the box header)
to match the Figma.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
awkoy
added
test-environment
…faults, compact selectors Address code-review findings on the new optimization run sidebar: - Algorithm model represents 'follow prompt model' as unset (picker shows 'Same as prompt · <model>'). Stop force-seeding/syncing it to the prompt model on change and on rerun, which discarded explicit choices and could submit a stale/unavailable model behind an 'inherited' label. - 'Use prompt model' now clears the explicit algorithm model and works even when no prompt model is selected. - Merge metric defaults on load so required params the saved config omitted (e.g. EQUALS case_sensitive) are present — otherwise Zod rejects a field with no inline error and Create silently no-ops. - Extract shared MetricParamErrors type (was copy-pasted across 7 files). - Compact model dropdown rows (h-8) + small search; align explainer icon sizes; shorten G-Eval placeholders. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
| <div className="flex items-center justify-between gap-2"> | ||
| <div className="flex items-center gap-1"> | ||
| <Label htmlFor="verbose" className="cursor-pointer text-sm"> | ||
| Verbose | ||
| </Label> |
There was a problem hiding this comment.
Repeated optimizer toggle markup
The Label + ExplainerIcon + Switch row is duplicated across the optimizer configs, so any spacing, tooltip, switch-size, or focus-behavior tweak needs to be repeated in each block — should we extract a shared OptimizerToggleField for the common id/label/explainerId/checked/onToggle props?
Want Baz to fix this for you? Activate Fixer
…ne separately) Remove this branch's only ui/select.tsx change (the SelectItem 'size' compact variant) and its two usages in the new-run sidebar. The select sizing work is being done separately, and this was the sole conflict with main's SelectItem refactor — reverting it lets select.tsx merge cleanly from main. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Resolve the single OptimizationModelSelect conflict by dropping the withoutCheck prop (removed on main) and keeping the compact h-8 model rows. main's SelectItem now indicates selection via a checked background instead of the left checkmark. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
⏱️ pre-commit per-hook timing
⏭️ 39 skipped (no matching files changed)
|
awkoy
added
test-environment
|
🔄 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. |
|
🌙 Nightly cleanup: The test environment for this PR ( |
…un-sidebar-local # Conflicts: # apps/opik-frontend/src/v2/pages/OptimizationsPage/OptimizationsPage.tsx
awkoy
added
test-environment
|
🔄 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. |
…ithm model clear New-run sidebar: - Prompt messages: surface empty-content errors per message (red border + inline "Message is required") instead of one general banner — per-index superRefine emits issues at [index, "content"] and validationErrors is passed through to LLMPromptMessages. - Algorithm model: replace the "Use prompt model" link with an inline x clear button on the field, and simplify the placeholder to "Same as prompt". Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
awkoy
added
test-environment
|
🔄 Test environment deployment process has started Phase 1: Deploying base version You can monitor the progress here. |
| const optimizerParams = { | ||
| ...baseOptimizerParams, | ||
| model: | ||
| savedOptimizerModel && availableModels.includes(savedOptimizerModel) | ||
| ? savedOptimizerModel | ||
| : undefined, |
There was a problem hiding this comment.
Stale optimizer params leak on inherit
When the saved optimizer model drops out of availableModels, optimizerParams.model is cleared but baseOptimizerParams.model_parameters stays set, so convertFormDataToStudioConfig() forwards stale optimizer params and the backend applies them to the fallback prompt model instead of dropping them — should we clear model_parameters here too, like the AlgorithmConfigs.tsx clear path?
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/optimizations/OptimizationConfigForm/schema.ts
around lines 232-237, in the logic that handles a previously-saved optimizer model no
longer present in availableModels (currently clearing only optimizerParams.model),
update it to also clear/drop the corresponding model_parameters from the params object
that gets forwarded by convertFormDataToStudioConfig. Specifically, when you detect the
saved optimizer model isn’t in availableModels and you switch to the prompt/default
model, remove baseOptimizerParams.model_parameters (or optimizerParams.model_parameters
if that’s where it lives) so stale parameters aren’t applied to the inherited model.
Match the behavior of the UI clear path in AlgorithmConfigs.tsx, and ensure the
generated request no longer includes those parameters in this scenario.
| const validationErrors = get(formState.errors, ["messages"]) as | ||
| | MessageValidationError[] | ||
| | undefined; | ||
|
|
||
| return ( |
There was a problem hiding this comment.
Root messages error disappears
messages can still be empty when a loaded prompt/template contains [], so validationErrors?.[idx]?.content?.message has no card to attach to in LLMPromptMessages and Zod's .min(1, "At least one message is required") error disappears — should we keep a root-level banner for messages or render the array error above the list?
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/OptimizationsPage/OptimizationsNewPage/OptimizationsNewPromptSection.tsx
around lines 264-298, in the `FormField` render for `name="messages"`, you removed the
root `<FormMessage />` and now only forward per-message `validationErrors` to
`LLMPromptMessages`. This causes the Zod array-level error (e.g., `.min(1, "At least one
message is required")`) to be invisible when `messages` is empty because there are no
cards to annotate. Refactor to always surface the root error for `messages` (either
re-add a `<FormMessage />` for this field, or render a small banner using
`formState.errors.messages` above `LLMPromptMessages` when `fieldMessages.length ===
0`). Ensure the banner/error uses the correct `formState.errors.messages` shape
(array-level vs per-index) so the user can see why an empty template fails validation.
|
✅ 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. |
|
🌙 Nightly cleanup: The test environment for this PR ( |
3 participants
Details
Reworks the Optimization Studio "new run" flow into a right-hand sidebar over the runs list (replacing the former full-page
/optimizations/newroute) and hardens the form. The form is split into per-section hooks, resolves the selected dataset's name on demand (instead of scanning a capped list), and cleans up the metric/schema arms. Most recent additions:dimension="sm") to match the right-rail controls and Figma.{{variables}}in the prompt or the G-Eval metric aren't columns in the selected item source (e.g. after switching datasets a template's{{question}}/{{expected_behavior}}no longer match). Prevents runs that would fail at evaluation time.Change checklist
Issues
Related but NOT resolved here: OPIK_7159 — surface optimizer run failures (and reconcile lost/never-consumed jobs) instead of leaving a run stuck on "Initialized". Tracked separately.
AI-WATERMARK
AI-WATERMARK: yes
tscandeslintpass. The variable-validation block has not yet been exercised end-to-end in a running environment (see Testing).Testing
npx tsc --project tsconfig.json --noEmit— passes.npx eslint --max-warnings=0on the changed new-run files — passes.opik-redis-masterservice is missing incometml-development), so end-to-end studio runs could not be triggered — see OPIK_7159. Suggested manual pass once a working env is available: open the demo template, switch the item source to a dataset with different columns, and confirm submit is blocked with the inline reason.Documentation
N/A — internal UI behavior; no documentation changes.
🤖 Generated with Claude Code