Repository navigation
Conversation
random-settings.tsx was one 1537-line component holding four field
groups, 17 form watches and six inline opt-in cards. It is now 234 lines
and 2 watches: one component per concern (SLO targets, load/stages, seed,
distribution, shared prefix, multi-turn), each taking no props and
reading form / action / intl from its own hooks. Editing one card no
longer re-renders the whole form.
SectionCard replaces the bordered card that was inlined six times. `open`
gates both the body and the header gap; passing `onOpenChange` renders
the standard toggle, and a discriminated union keeps that mutually
exclusive with `extra` — four cards put a registered Form.Item there, and
silently dropping one would unregister a field.
Because the gated Form.Items unmount, those cards must derive their open
state with `Form.useWatch(name, { form, preserve: true })`. Plain
useWatch reads getFieldsValue(), which carries REGISTERED fields only, so
it returns undefined while the card is closed — the card could then never
open from prefilled data, and the toggle handler would overwrite it.
Drops dead code found on the way: the sloFieldsFromTargets writes in
writeSloTargets and handleProfileChange (the 9 flat slo_*_ms fields have
no Form.Item, so they never reached the request — handleModalOk expands
slo_targets instead), `currentData` from the form context, and three
unreferenced styles. Raw `display: flex` wrappers became `Flex`.
One behaviour change: the Data Distribution card's expanded state no
longer survives switching the dataset away from Random and back while
all six fields are empty, since the toggle unmounts with the card.
Submitted values are unaffected.
There was a problem hiding this comment.
Code Review
This pull request refactors the benchmark form by modularizing its sections into dedicated card components (such as SectionCard, DistributionCard, LoadSettingsForm, MultiTurnCard, SeedCard, SharedPrefixCard, and SloTargetsForm), which improves maintainability and readability. Feedback is provided regarding Ant Design form validation: when adding new items to the prefix buckets or load stages, calling form.setFieldValue silently clears existing validation errors. It is recommended to trigger revalidation immediately after adding these items to ensure the validation state remains accurate.
There was a problem hiding this comment.
When adding a new prefix bucket, form.setFieldValue is called, which silently wipes any existing validation errors on the prefix_buckets field in Ant Design. If the field was previously invalid (e.g., after a failed submission), the error message will disappear even though the newly added item is still invalid (since prefix_tokens is undefined).
Please call revalidatePrefix() inside handlePrefixAdd to ensure the validation state remains accurate.
const handlePrefixAdd = () => {
form.setFieldValue('prefix_buckets', [
...(prefixBuckets || []),
{ prefix_count: 1, bucket_weight: 100 }
]);
revalidatePrefix();
};
| const handleStageAdd = () => { | ||
| form.setFieldValue('stages', [...(stages || []), { rate: undefined }]); | ||
| }; |
There was a problem hiding this comment.
Similar to the prefix buckets, calling form.setFieldValue when adding a new stage silently clears any existing validation errors on the stages field. If the stages were previously invalid, the error message will be wiped even though the new stage is still invalid (since rate is undefined).
Please call revalidateStages() inside handleStageAdd to keep the validation state in sync.
const handleStageAdd = () => {
form.setFieldValue('stages', [...(stages || []), { rate: undefined }]);
revalidateStages();
};
setFieldValue resolves to setFields([{ …, errors: [], warnings: [] }]), so
every write wipes the field's error. The delete/change handlers already
re-validated; the add handlers did not, so clicking "Add" after a failed
submit dropped the message while making the list more invalid. Turning on
Shared Prefix had the same hole — it seeds a bucket with no prefix_tokens,
exactly what its validator rejects. Prefix buckets, stages and SLO targets
now each write through one revalidating helper, so the omission can't recur.
`turns` was clamped to 64 only in the stepper, so typing straight into the
box submitted any number. Both bounds are TURNS_MIN / TURNS_MAX now and the
input carries `max`.
handleProfileChange derived auto_tune as `config.auto_tune ?? true` while
load-settings renders every named preset as auto-tune regardless. They agree
today — a named preset is a ramp by definition — but one arriving with
auto_tune=false would have submitted a manual run with no stages, since
those sit in an unmounted Form.Item. Both sides derive it the same way now,
off the same ProfileValueMap constant.
Also drops the FilterBar `marginTop` prop that is not in FilterBarProps,
which clears the last type error on the page.
There was a problem hiding this comment.
🟡 Changes recommended
There is at least one confirmed functional issue (SLO add-row behavior when all combinations are exhausted) plus a type-level API contract gap in SectionCard that should be tightened before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Refactors the benchmark “Random settings” form by splitting the previous large random-settings.tsx into smaller, concern-focused subcomponents (dataset shaping cards, load settings, SLO targets) and introducing a shared SectionCard wrapper to standardize opt-in card UX.
Changes:
- Split random-settings into dedicated form sections/cards (seed, distribution, shared prefix, multi-turn, load settings, SLO targets).
- Introduced
SectionCardto replace repeated bordered opt-in card markup and enforce mutual exclusivity between built-in toggle vs custom header controls. - Simplified benchmark form context by removing
currentDatafrom the context payload and relying on preserved form store reads for gated fields.
File summaries
| File | Description |
|---|---|
| src/pages/benchmark/index.tsx | Minor FilterBar prop cleanup. |
| src/pages/benchmark/forms/random-settings.tsx | Replaced the monolithic component with composed sub-forms/cards and SectionCard usage. |
| src/pages/benchmark/forms/dataset.tsx | Aligns profile/auto-tune derivation logic and migrates layout wrappers to Flex; stops passing datasetList as prop to random settings. |
| src/pages/benchmark/forms/index.tsx | Stops providing currentData via form context (keeps prefill logic in this file). |
| src/pages/benchmark/forms/slo-targets.tsx | New: dedicated SLO targets editor using a controlled MetadataList. |
| src/pages/benchmark/forms/shared-prefix-card.tsx | New: opt-in shared prefix bucket list card using SectionCard. |
| src/pages/benchmark/forms/seed-card.tsx | New: seed configuration card (random vs pinned) using SectionCard. |
| src/pages/benchmark/forms/multi-turn-card.tsx | New: multi-turn opt-in card with stepper controls. |
| src/pages/benchmark/forms/load-settings.tsx | New: load type + auto/manual stages configuration split into its own form section. |
| src/pages/benchmark/forms/distribution-card.tsx | New: opt-in distribution inputs + charts, with preserved watches for gated fields. |
| src/pages/benchmark/config/index.ts | Adds TURNS_MIN/TURNS_MAX and updates SLO mapping documentation. |
| src/pages/benchmark/config/form-context.ts | Removes currentData from the shared form context type. |
| src/pages/benchmark/components/section-card.tsx | New: standardized opt-in card component with type-level mutual exclusivity for header controls. |
Review details
- Files reviewed: 13/13 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| type SectionCardHeaderProps = | ||
| | { | ||
| /** Opt-in card: renders the standard small Switch bound to `open`. */ | ||
| onOpenChange: (open: boolean) => void; | ||
| /** Disables the built-in Switch. */ | ||
| disabled?: boolean; | ||
| extra?: never; | ||
| } | ||
| | { | ||
| onOpenChange?: never; | ||
| disabled?: never; | ||
| /** Header's right-hand control: a form-bound switch, Segmented, Tag… */ | ||
| extra?: React.ReactNode; | ||
| }; |
| const handleSloAdd = () => { | ||
| // Default the new row to the first (metric, agg) pair still free. | ||
| const taken = new Set(sloTargets.map((t) => sloTargetKey(t.metric, t.agg))); | ||
| const free = SLO_METRICS.flatMap((m) => | ||
| SLO_AGGS.map((a) => ({ metric: m.value, agg: a.value })) | ||
| ).find((c) => !taken.has(sloTargetKey(c.metric, c.agg))); | ||
| writeSloTargets([...sloTargets, { ...(free || {}), value: null }]); | ||
| }; |
There was a problem hiding this comment.
🟡 Changes recommended
The PR metadata describes a benchmark refactor, but the code changes shown are in the login form, so the PR scope/description needs to be corrected or split for accurate review.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 1
- Review effort level: Lite
There was a problem hiding this comment.
🔵 Needs a closer look
It introduces at least one confirmed UI dead-end (SLO add can create an unusable row) and a type-safety gap in SectionCard props that can allow a non-functional toggle state.
Review details
Suppressed comments (2)
src/pages/benchmark/forms/slo-targets.tsx:78
- handleSloAdd can create an unusable extra row when all (metric, agg) combinations are already taken:
freebecomes undefined, the new row has no selectable options (they’ll all be disabled), and the user can’t recover except deleting the row.
const free = SLO_METRICS.flatMap((m) =>
SLO_AGGS.map((a) => ({ metric: m.value, agg: a.value }))
).find((c) => !taken.has(sloTargetKey(c.metric, c.agg)));
writeSloTargets([...sloTargets, { ...(free || {}), value: null }]);
src/pages/benchmark/components/section-card.tsx:82
- SectionCard’s JSDoc says
openis always controlled (no defaultValue), but the current prop types allow omittingopenwhile passingonOpenChange. In that casebodyOpenfalls back to!!children, so an opt-in card with children will render permanently open with a non-functional toggle. Makingopenrequired whenonOpenChangeis present prevents this class of bug at compile time.
type SectionCardProps = SectionCardBaseProps & SectionCardHeaderProps;
- Files reviewed: 13/13 changed files
- Comments generated: 0 new
- Review effort level: Lite

random-settings.tsx was one 1537-line component holding four field groups, 17 form watches and six inline opt-in cards. It is now 234 lines and 2 watches: one component per concern (SLO targets, load/stages, seed, distribution, shared prefix, multi-turn), each taking no props and reading form / action / intl from its own hooks. Editing one card no longer re-renders the whole form.
SectionCard replaces the bordered card that was inlined six times.
opengates both the body and the header gap; passingonOpenChangerenders the standard toggle, and a discriminated union keeps that mutually exclusive withextra— four cards put a registered Form.Item there, and silently dropping one would unregister a field.Because the gated Form.Items unmount, those cards must derive their open state with
Form.useWatch(name, { form, preserve: true }). Plain useWatch reads getFieldsValue(), which carries REGISTERED fields only, so it returns undefined while the card is closed — the card could then never open from prefilled data, and the toggle handler would overwrite it.Drops dead code found on the way: the sloFieldsFromTargets writes in writeSloTargets and handleProfileChange (the 9 flat slo_*_ms fields have no Form.Item, so they never reached the request — handleModalOk expands slo_targets instead),
currentDatafrom the form context, and three unreferenced styles. Rawdisplay: flexwrappers becameFlex.One behaviour change: the Data Distribution card's expanded state no longer survives switching the dataset away from Random and back while all six fields are empty, since the toggle unmounts with the card. Submitted values are unaffected.