refactor(benchmark): split the random settings form by concern by hibig · Pull Request #1382 · gpustack/gpustack-ui · GitHub
Skip to content

refactor(benchmark): split the random settings form by concern - #1382

Open
hibig wants to merge 2 commits into
mainfrom
dev
Open

hibig wants to merge 2 commits into
mainfrom
dev

Conversation

@hibig

@hibig hibig commented Sep 3, 2026

Copy link
Copy Markdown
Collaborator

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.

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.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +61 to +66

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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();
  };

Comment on lines +67 to +69
const handleStageAdd = () => {
form.setFieldValue('stages', [...(stages || []), { rate: undefined }]);
};

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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.
Copilot AI lite review requested due to automatic review settings September 4, 2026 04:37

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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 SectionCard to 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 currentData from 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.

Comment on lines +67 to +80
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;
};
Comment on lines +72 to +79
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 }]);
};
Copilot AI review requested due to automatic review settings September 9, 2026 04:14

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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

Comment on lines 238 to 242
Copilot AI review requested due to automatic review settings September 9, 2026 05:28

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 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: free becomes 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 open is always controlled (no defaultValue), but the current prop types allow omitting open while passing onOpenChange. In that case bodyOpen falls back to !!children, so an opt-in card with children will render permanently open with a non-functional toggle. Making open required when onOpenChange is 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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants