refactor(FR-3474): extract shared Service Configuration/Health Check/Pre-Start Actions form components - #8603
Merged
Merged
Conversation
Contributor
agatha197
force-pushed
the
fix/FR-3474-preset-field-naming
branch
from
August 6, 2026 05:08
fee2b76 to
da96298
Compare
github-actions
Bot
added
frontend
refactor
area:lib
Contributor
Coverage Report for root-coverage
File CoverageNo changed files found. |
Contributor
Coverage Report for backend-ai-ui-coverage (./packages/backend.ai-ui)
File CoverageNo changed files found. |
agatha197
force-pushed
the
fix/FR-3474-preset-field-naming
branch
from
August 7, 2026 04:43
a9eebdc to
67f6df8
Compare
agatha197
added a commit
that referenced
this pull request
Aug 7, 2026
…Command toggle #8603 removed the Basic/Advanced Segmented toggle — Execution (Shell/Exec) + Shell are now always visible instead of hidden behind "Advanced". Update the add-revision and admin-preset specs to match: - Drop every `getByText('Advanced', ...).click()` step; assert Execution/ Shell are visible and Shell is pre-filled with /bin/bash immediately after the form mounts, with no toggle in between. - Rename tests whose titles referenced "Advanced mode"/"Advanced Shell mode". - Merge "submits the raw command verbatim" and "submits shell = /bin/bash when Advanced Shell mode is left unchanged" into one test — both scenarios are now identical (nothing left to distinguish them without the toggle). - Update stale helper-text assertions to the new CommandShellHelper / CommandExecHelper i18n copy. - add-revision-runtime-defaults.spec.ts needed no changes — its "Advanced" references are the unrelated Preset↔Custom toggle and the Model Definition File Path's "Advanced Settings" collapse panel.
agatha197
added a commit
that referenced
this pull request
Aug 10, 2026
4-lens review (reuse/simplification/efficiency/altitude) across the whole main...HEAD stack found consistent hotspots in this branch's own files: - AdminDeploymentPresetReviewSummary.tsx duplicated a ~110-line Descriptions block (Shell/Command/Port/Health Check fields/Pre-Start Actions) verbatim across its two capability-gated render sites. Extracted to a single renderServiceConfigSummaryFields() called from both. - The `readsVfolderConfigFiles` fallback (`?? name === 'custom'`) was re-derived inline in 3 places in this branch's files, each with its own copy of the "never `?? false`" warning comment. Extracted to resolvesReadsVfolderConfigFiles() in modelServiceCommand.ts. - AdminDeploymentPresetSettingPageContent.tsx's Service Configuration render site re-fetched runtimeVariantId via a Form.Item dependencies render-prop and re-ran the same .find() already computed once at the top of the component for the legacy-nesting logic — now reuses that value directly, dropping the redundant Form.Item wrapper and lookup. - AdminDeploymentPresetModelConfigItem.tsx repeated the 4-segment ['modelDefinition', 'models', 0, ...] prefix 13 times (several wrapped across 6-7 lines) after the earlier Form.List removal — added a modelField(...path) helper. - The Pre-Start Actions JSON.parse-with-fallback and the supportsNullableModelDefinition ? (v || null) : (v ?? fallback) ternary were each repeated in buildModelDefinitionInput — extracted preStartActionsToInput() and a local nullableStringField() helper. Net: -334/+199 lines across the 5 files touched. Out of scope for this pass: the same readsVfolderConfigFiles pattern and a duplicate Pre-Start Actions JSON.parse also appear in DeploymentAddRevisionModal.tsx, but that file's changes belong to a lower stack layer (fr-3205 / fix/FR-3474-preset-field-naming #8603), not this branch's own commits — fixing it here would violate the stacked-PR convention of making changes on the branch that owns them. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Extracts duplicated model-service configuration controls into shared form components used by revision and preset workflows.
Changes:
- Adds shared Service Configuration, Health Check, and Pre-Start Actions components.
- Unifies form field names and submission mappings.
- Removes Basic/Advanced mode and updates command guidance across locales.
Reviewed changes
Copilot reviewed 32 out of 32 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
react/src/components/ModelServiceFormItems/ServiceConfigurationFormItems.tsx |
Adds shared command and port fields. |
react/src/components/ModelServiceFormItems/ModelServiceHealthCheckFormItems.tsx |
Adds shared health-check fields. |
react/src/components/ModelServiceFormItems/PreStartActionsFormList.tsx |
Adds shared pre-start action list. |
react/src/components/DeploymentAddRevisionModal.tsx |
Adopts shared fields and renamed values. |
react/src/components/AdminDeploymentPresetSettingPageContent.tsx |
Adopts shared nested form fields. |
react/src/pages/AdminDeploymentPresetSettingPage.tsx |
Updates preset submission mapping. |
react/src/components/AdminDeploymentPresetFormTypes.ts |
Aligns shared form value types. |
react/src/components/AdminDeploymentPresetReviewSummary.tsx |
Reorders and updates service review details. |
react/src/components/AdminDeploymentPresetModelConfigItem.tsx |
Adjusts model configuration layout. |
react/src/helper/modelServiceCommand.ts |
Simplifies execution-mode handling. |
react/src/helper/modelServiceCommand.test.ts |
Updates command-mode tests. |
resources/i18n/en.json |
Updates English command guidance. |
resources/i18n/de.json |
Updates German command guidance. |
resources/i18n/el.json |
Updates Greek command guidance. |
resources/i18n/es.json |
Updates Spanish command guidance. |
resources/i18n/fi.json |
Updates Finnish command guidance. |
resources/i18n/fr.json |
Updates French command guidance. |
resources/i18n/id.json |
Updates Indonesian command guidance. |
resources/i18n/it.json |
Updates Italian command guidance. |
resources/i18n/ja.json |
Updates Japanese command guidance. |
resources/i18n/ko.json |
Updates Korean command guidance. |
resources/i18n/mn.json |
Updates Mongolian command guidance. |
resources/i18n/ms.json |
Updates Malay command guidance. |
resources/i18n/pl.json |
Updates Polish command guidance. |
resources/i18n/pt.json |
Updates Portuguese command guidance. |
resources/i18n/pt-BR.json |
Updates Brazilian Portuguese guidance. |
resources/i18n/ru.json |
Updates Russian command guidance. |
resources/i18n/th.json |
Updates Thai command guidance. |
resources/i18n/tr.json |
Updates Turkish command guidance. |
resources/i18n/vi.json |
Updates Vietnamese command guidance. |
resources/i18n/zh-CN.json |
Updates Simplified Chinese guidance. |
resources/i18n/zh-TW.json |
Updates Traditional Chinese guidance. |
Suppressed comments (1)
react/src/components/ModelServiceFormItems/ServiceConfigurationFormItems.tsx:41
- This removes the Basic/Advanced control from both forms, but the PR description and linked issue define the extracted Service Configuration as including that Segmented control. This is a product-behavior change rather than a behavior-preserving extraction; either restore the control or update the PR scope/acceptance criteria and stacked tests to document the redesign.
// No Basic/Advanced toggle: Execution (Shell/Exec) + Shell are always shown
// together with Command/Port — team feedback (devops sync, 2026-08-07) was
// that hiding this behind an "Advanced" switch made the always-shell-wrapped
// Basic mode look like it didn't run through a shell at all, which was the
// actual source of confusion. Showing the real execution mode up front is
// more explicit, at the cost of one more always-visible field.
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 32 out of 32 changed files in this pull request and generated no new comments.
Suppressed comments (3)
react/src/components/ModelServiceFormItems/ServiceConfigurationFormItems.tsx:20
- The shared API removes a caller-specific validation difference: the preset form previously required both Start Command and Port, while the revision modal kept them optional, and the linked #8333 coverage still states that preset requirement. This component now applies only a whitespace rule to the command and no required rule to the port, so extracting it silently weakens preset validation. Please expose caller-configurable required rules/props and enable them for the preset call site only.
export interface ServiceConfigurationFormItemsProps {
namePrefix: Array<string | number>;
/** Per-field placeholder text, grouped to match ModelServiceHealthCheckFormItems. */
placeholders?: Partial<{
command: string;
react/src/components/ModelServiceFormItems/ServiceConfigurationFormItems.tsx:40
- This is a user-visible redesign rather than the extraction described by the PR and linked issue: both explicitly say the shared section retains the Basic/Advanced Segmented control, but this removes it and always exposes Execution/Shell. Either restore the scoped behavior or update the PR requirements and verification to explicitly cover this additional UX change.
// No Basic/Advanced toggle: Execution (Shell/Exec) + Shell are always shown
// together with Command/Port — team feedback (devops sync, 2026-08-07) was
// that hiding this behind an "Advanced" switch made the always-shell-wrapped
// Basic mode look like it didn't run through a shell at all, which was the
// actual source of confusion. Showing the real execution mode up front is
react/src/components/ModelServiceFormItems/ServiceConfigurationFormItems.tsx:110
- With the revision modal's
namePrefix={[]}, this leaf name changes its generated DOM id fromcommandShelltoshell. That directly contradicts the PR description's rationale that the revision modal's#command*ids are intentionally preserved for #8333. Please either preserve the modal's established field names through an explicit mapping or correct the PR/dependent test documentation and ensure the locator update lands atomically.
<Form.Item
name={[...namePrefix, 'shell']}
label={t('modelService.Shell')}
agatha197
added a commit
that referenced
this pull request
Aug 11, 2026
- AdminDeploymentPresetFormTypes.ts: the ModelServiceFormValue comment still said port/startCommand were UI-required and described an abandoned plan to keep DeploymentAddRevisionModal.tsx's old commandShell/commandPort names stable. Neither matches current behavior: both fields are optional per BA-6613 (confirmed in ServiceConfigurationFormItems.tsx — no `required` rule on either), and DeploymentAddRevisionModal.tsx's fields were in fact renamed to the shorter unified names, with fr-3344's e2e suite updated to match (`#shell`/`#port`, not `#commandShell`/ `#commandPort`). Rewrote the comment to describe what actually happens. - modelServiceCommand.ts: resolveCommandShell()'s doc said `execution` is omitted "e.g. the preset form, which has no Exec mode" — stale, since the shared ServiceConfigurationFormItems now exposes Exec mode to both callers. Reworded to the real omission case (managers old enough that supportsCommandShell is false). No behavior change — both fixes are comment/doc corrections. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
agatha197
added a commit
that referenced
this pull request
Aug 11, 2026
4-lens review (reuse/simplification/efficiency/altitude) across the whole main...HEAD stack found consistent hotspots in this branch's own files: - AdminDeploymentPresetReviewSummary.tsx duplicated a ~110-line Descriptions block (Shell/Command/Port/Health Check fields/Pre-Start Actions) verbatim across its two capability-gated render sites. Extracted to a single renderServiceConfigSummaryFields() called from both. - The `readsVfolderConfigFiles` fallback (`?? name === 'custom'`) was re-derived inline in 3 places in this branch's files, each with its own copy of the "never `?? false`" warning comment. Extracted to resolvesReadsVfolderConfigFiles() in modelServiceCommand.ts. - AdminDeploymentPresetSettingPageContent.tsx's Service Configuration render site re-fetched runtimeVariantId via a Form.Item dependencies render-prop and re-ran the same .find() already computed once at the top of the component for the legacy-nesting logic — now reuses that value directly, dropping the redundant Form.Item wrapper and lookup. - AdminDeploymentPresetModelConfigItem.tsx repeated the 4-segment ['modelDefinition', 'models', 0, ...] prefix 13 times (several wrapped across 6-7 lines) after the earlier Form.List removal — added a modelField(...path) helper. - The Pre-Start Actions JSON.parse-with-fallback and the supportsNullableModelDefinition ? (v || null) : (v ?? fallback) ternary were each repeated in buildModelDefinitionInput — extracted preStartActionsToInput() and a local nullableStringField() helper. Net: -334/+199 lines across the 5 files touched. Out of scope for this pass: the same readsVfolderConfigFiles pattern and a duplicate Pre-Start Actions JSON.parse also appear in DeploymentAddRevisionModal.tsx, but that file's changes belong to a lower stack layer (fr-3205 / fix/FR-3474-preset-field-naming #8603), not this branch's own commits — fixing it here would violate the stacked-PR convention of making changes on the branch that owns them. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
agatha197
force-pushed
the
fix/FR-3474-preset-field-naming
branch
from
August 11, 2026 01:17
90c65f2 to
f633979
Compare
agatha197
force-pushed
the
fix/FR-3474-preset-field-naming
branch
from
August 11, 2026 07:43
f633979 to
07e94f6
Compare
agatha197
added a commit
that referenced
this pull request
Aug 11, 2026
…Command toggle #8603 removed the Basic/Advanced Segmented toggle — Execution (Shell/Exec) + Shell are now always visible instead of hidden behind "Advanced". Update the add-revision and admin-preset specs to match: - Drop every `getByText('Advanced', ...).click()` step; assert Execution/ Shell are visible and Shell is pre-filled with /bin/bash immediately after the form mounts, with no toggle in between. - Rename tests whose titles referenced "Advanced mode"/"Advanced Shell mode". - Merge "submits the raw command verbatim" and "submits shell = /bin/bash when Advanced Shell mode is left unchanged" into one test — both scenarios are now identical (nothing left to distinguish them without the toggle). - Update stale helper-text assertions to the new CommandShellHelper / CommandExecHelper i18n copy. - add-revision-runtime-defaults.spec.ts needed no changes — its "Advanced" references are the unrelated Preset↔Custom toggle and the Model Definition File Path's "Advanced Settings" collapse panel.
agatha197
added a commit
that referenced
this pull request
Aug 11, 2026
4-lens review (reuse/simplification/efficiency/altitude) across the whole main...HEAD stack found consistent hotspots in this branch's own files: - AdminDeploymentPresetReviewSummary.tsx duplicated a ~110-line Descriptions block (Shell/Command/Port/Health Check fields/Pre-Start Actions) verbatim across its two capability-gated render sites. Extracted to a single renderServiceConfigSummaryFields() called from both. - The `readsVfolderConfigFiles` fallback (`?? name === 'custom'`) was re-derived inline in 3 places in this branch's files, each with its own copy of the "never `?? false`" warning comment. Extracted to resolvesReadsVfolderConfigFiles() in modelServiceCommand.ts. - AdminDeploymentPresetSettingPageContent.tsx's Service Configuration render site re-fetched runtimeVariantId via a Form.Item dependencies render-prop and re-ran the same .find() already computed once at the top of the component for the legacy-nesting logic — now reuses that value directly, dropping the redundant Form.Item wrapper and lookup. - AdminDeploymentPresetModelConfigItem.tsx repeated the 4-segment ['modelDefinition', 'models', 0, ...] prefix 13 times (several wrapped across 6-7 lines) after the earlier Form.List removal — added a modelField(...path) helper. - The Pre-Start Actions JSON.parse-with-fallback and the supportsNullableModelDefinition ? (v || null) : (v ?? fallback) ternary were each repeated in buildModelDefinitionInput — extracted preStartActionsToInput() and a local nullableStringField() helper. Net: -334/+199 lines across the 5 files touched. Out of scope for this pass: the same readsVfolderConfigFiles pattern and a duplicate Pre-Start Actions JSON.parse also appear in DeploymentAddRevisionModal.tsx, but that file's changes belong to a lower stack layer (fr-3205 / fix/FR-3474-preset-field-naming #8603), not this branch's own commits — fixing it here would violate the stacked-PR convention of making changes on the branch that owns them. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
agatha197
added a commit
that referenced
this pull request
Aug 14, 2026
…fig docs Two comments contradicted what the shared Service Configuration form actually does, both flagged in review on #8603: - `AdminDeploymentPresetFormTypes.ts` still described `port` / `startCommand` as "required at the UI level" citing AdminDeploymentPresetModelConfigItem. Neither carries a `required` rule on either form (BA-6613): the backend defaults `shell`, each page's submit-mapping falls back to a default port, and the `required` prop in that component is a visual-only hint on name/modelPath, not these fields. - `resolveCommandShell`'s doc gave "the preset form, which has no Exec mode" as the example of an omitted `execution`. The preset form reaches Exec through the shared component now, so the example is generalized to a legacy manager that renders no Execution control. Comments only — no behaviour change. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
agatha197
force-pushed
the
fix/FR-3474-preset-field-naming
branch
from
August 14, 2026 02:34
a2f4a18 to
e143af1
Compare
…Pre-Start Actions form components DeploymentAddRevisionModal.tsx and AdminDeploymentPresetSettingPageContent.tsx each inlined a near-identical block of Form.Items for Service Configuration (Basic/Advanced + Execution + Shell + Command + Port), Health Check, and Pre-Start Actions. Extract all three into react/src/components/ModelServiceFormItems/, parameterized by a namePrefix so each form's own field-path shape (flat vs nested under modelDefinition.models[0].service) still works. To make namePrefix a pure array-prepend with no per-field name mapping, the preset form's leaf field names (shell, port, enableHealthCheck, nested healthCheck.*, commandAdvanced/commandExecution) were unified with the revision modal's naming instead of the reverse, and the "command" prefix itself was dropped from both forms since it turned out to be a copy-paste artifact, not a deliberate convention. Based directly on fr-3205; e2e test updates for the renamed fields (and other e2e work) are handled separately in the PR stacked on top of this one. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ealth-check placeholders - Share ModelHealthCheckFormValue/PreStartActionFormValue types between DeploymentAddRevisionModal and AdminDeploymentPresetFormTypes instead of duplicating inline shapes. - Add ServiceFormNamePrefix type shared by all three ModelServiceFormItems components instead of repeating Array<string | number> inline. - Normalize ServiceConfigurationFormItems' placeholder props into a grouped `placeholders` object, matching ModelServiceHealthCheckFormItems' shape. - Restore the 3 health-check placeholders (interval, maxWaitTime, expectedStatusCode) dropped during the original extraction. # Conflicts: # react/src/components/DeploymentAddRevisionModal.tsx
…nFormItems, drop ServiceFormNamePrefix type file
- ServiceConfigurationFormItems now calls baiClient.supports('model-service-command-string')
itself instead of taking it as a caller-supplied prop. Both callers already
computed the exact same value identically, so the prop only threaded a
value through without avoiding any real duplication; AdminDeploymentPresetSettingPageContent.tsx's
copy becomes dead code and is removed along with its now-unused
useSuspendedBackendaiClient import. DeploymentAddRevisionModal.tsx keeps
its own copy since it still needs it for submit-mapping.
- Drop ModelServiceFormItems/types.ts: a single-line type alias doesn't
warrant its own file, and the sibling SessionFormItems/ folder has no such
precedent. Inline Array<string | number> in each of the 3 components'
props instead.
… default BA-6613 relaxes PresetModelServiceConfigInput's command/port validation to match the Add-Revision modal: `command` is already optional/null-default and `shell` defaults to `/bin/bash` on the backend; only the WebUI's preset form still required Start Command and Port. Drop the now caller-invariant commandRules/portRules props from ServiceConfigurationFormItems (both callers want the same optional rules) and hardcode `[{ whitespace: true }]` on Start Command, no rule on Port — matching DeploymentAddRevisionModal.tsx's existing behavior exactly. Also prefill EMPTY_MODEL_SEED.service.shell with DEFAULT_MODEL_SERVICE_SHELL so a newly-added model's Shell field starts pre-filled with /bin/bash when Advanced/Shell mode is revealed, matching the Add-Revision modal's Custom form initialValues. Port stays hard-required (Int!) in the current schema, but buildModelDefinitionInput already falls back to `service.port ?? 8000` unconditionally, so relaxing the form's validation doesn't risk sending an invalid mutation.
…ell on Exec switch The Basic Info Review card listed fields in the order they were originally added rather than the order they're entered: Image before Runtime Params/ Service Config/Health Check/Pre-Start Actions (input has Image last), Port before Command/Shell (input types Shell/Command before Port), and Pre-Start Actions before Health Check (input has Health Check first). Reorder the Descriptions.Items to mirror AdminDeploymentPresetSettingPageContent.tsx's actual field sequence. Same fix for the model metadata sub-card: License is entered right after Version but was rendered last. Also: switching Execution from Shell to Exec left the (now-hidden) Shell input's value in the antd form store — resolveCommandShell() already discards it at submit (Exec always sends shell: null), but the Review page reads the raw field value directly, so it kept showing the stale shell choice. Reset shell to DEFAULT_MODEL_SERVICE_SHELL on the Exec switch in both AdminDeploymentPresetSettingPageContent.tsx and DeploymentAddRevisionModal.tsx's onValuesChange, mirroring the existing Advanced->Basic reset that already guards against the same class of bug. # Conflicts: # react/src/components/AdminDeploymentPresetReviewSummary.tsx
…ution reset form.setFieldsValue() replaces nested object/array values wholesale rather than deep-merging. modelDefinition.models is a plain array field (no Form.List), so the Advanced->Basic and Exec-switch resets — which only passed a partial `models[0]` object — wiped out name, modelPath, port, startCommand, and every other sibling field on every reset. Reproduced manually: clicking the Exec radio cleared all typed values and bounced the form back to Basic mode. Fix: read the current models[0] value via getFieldValue, merge the reset fields into a complete copy, and write that back — never a partial object.
… resetting on Exec switch Drop the Exec-switch reset added in baa33d2 in favor of a simpler fix: the Review page's stale-Shell-after-switching-to-Exec bug doesn't need the underlying form value cleared at all — resolveCommandShell() already ignores it at submit time regardless of what's stored. Gate PresetReviewSummary's Shell Descriptions.Item on `execution !== 'exec'` instead, matching what's actually submitted. This also means a user who round-trips Exec -> Shell gets their own previously-typed shell back rather than being force-reset to the default every time. Keep the Advanced -> Basic reset (still needed: re-opening Advanced later should not resurface a stale custom shell as if it were still selected), and its earlier setFieldsValue wipe-bug fix.
…ps and helper text Rewords 5 modelService i18n keys (CommandAdvancedModeTooltip, ExecutionTooltip, CommandShellHelper, CommandExecHelper, ShellTooltip) to spell out the actual execution mechanism (bash -c "..." vs. shlex-split argv with no shell) instead of just stating which operators work, and to note that a custom shell must exist in the image or the service fails to start. Follow-up from the devops sync thread where teammates asked when to use Exec vs Shell mode and what breaks if a chosen shell isn't present in the image.
Devops sync feedback (2026-08-07): hiding Basic mode's implicit shell-wrapped execution behind an "Advanced" switch was the actual source of confusion — teammates didn't realize Basic mode ran the command through a shell at all. Drop the toggle entirely and always show what used to be the "Advanced" content (Execution radio + conditional Shell field) alongside Command/Port. - modelServiceCommand.ts: drop `advanced` from CommandModeState / deriveCommandModeState / resolveCommandShell (Shell mode's default shell is now always sent explicitly, never omitted for an implicit "Basic" state). Trim COMMAND_SHELL_OPTIONS's AutoComplete suggestions to just /bin/bash — the only shell the backend actually assumes is present in the image; suggesting others invites a launch failure if that shell binary is missing (per Bo Keum Kim's Teams comment). - ServiceConfigurationFormItems.tsx: remove the Basic/Advanced Segmented toggle from the Collapse header; Execution + Shell render unconditionally whenever the manager supports the command/shell API. - AdminDeploymentPresetSettingPageContent.tsx / DeploymentAddRevisionModal.tsx: drop the now-moot Advanced→Basic onValuesChange reset (nothing left to reset without a Basic state) and the `advanced` field from form types / initialValues / submit-mapping. - AdminDeploymentPresetSettingPage.tsx: drop `advanced` from the resolveCommandShell() call in buildModelDefinitionInput.
…ip for the merged mode Removes the now-unused CommandAdvancedModeTooltip key (described a toggle that no longer exists) and rewords ExecutionTooltip across all 22 locales to drop the "same as Basic mode" comparison, keeping the guidance on when to actually reach for Exec mode.
- modelServiceCommand.ts: COMMAND_SHELL_OPTIONS is surfaced as the Shell input's placeholder now, not an AutoComplete dropdown. - AdminDeploymentPresetFormTypes.ts: validation lives in the self-hosted form-engine, not antd. - Restore the why-comments for port min=2 (backend gt=1 exclusive) and expectedStatusCode min=101 (backend gt=100) that were dropped during the ModelServiceFormItems extraction.
…fig docs Two comments contradicted what the shared Service Configuration form actually does, both flagged in review on #8603: - `AdminDeploymentPresetFormTypes.ts` still described `port` / `startCommand` as "required at the UI level" citing AdminDeploymentPresetModelConfigItem. Neither carries a `required` rule on either form (BA-6613): the backend defaults `shell`, each page's submit-mapping falls back to a default port, and the `required` prop in that component is a visual-only hint on name/modelPath, not these fields. - `resolveCommandShell`'s doc gave "the preset form, which has no Exec mode" as the example of an omitted `execution`. The preset form reaches Exec through the shared component now, so the example is generalized to a legacy manager that renders no Execution control. Comments only — no behaviour change. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ation and Pre-Start Actions antd's bordered Collapse panel used to supply the bottom padding these sections ended with, so their last elements carried marginBottom: 0. The flat Astryx Collapsible has no such padding, leaving the Port field and the Add Pre-Start Action button flush against whatever section follows. Give both an explicit token.marginMD bottom gap.
…e Collapsible trigger
…n gap ServiceConfigurationFormItems now owns its section-end margin, so the per-page wrapper margins on top of it (marginMD in the revision modal, marginLG on the preset page) doubled the gap before Enable Health Check to ~50px. Drop the wrapper margins and let the component's own gap stand.
…Service Configuration section
yomybaby
approved these changes
Aug 21, 2026
yomybaby
left a comment
yomybaby
left a comment
Member
There was a problem hiding this comment.
LGTM.
The namePrefix array-prepend is the right shape for this: no per-field mapping table inside the shared components, so the extraction stays honest about what it costs (both forms must share leaf field names) instead of hiding it. Equally good is what was deliberately not extracted — each page keeps its own GraphQL submit-mapping, because the differences between ModelServiceConfigInput and PresetModelServiceConfigInput are real rather than incidental. The header comment stating that ownership boundary explicitly is what should keep this from drifting apart again, and #8622 does respect it (it adds portTooltipExtra as a prop rather than reaching for baiClient.supports() inside the shared component).
Removing the Basic/Advanced toggle that #8170 had just introduced is the right call, and the reason recorded in the code — hiding the mode made the always-shell-wrapped Basic mode look like it did not use a shell at all — is worth having written down. Same for trimming COMMAND_SHELL_OPTIONS to the one shell the backend actually assumes.
Three non-blocking notes, all cosmetic:
-
react/src/components/DeploymentAddRevisionModal.tsx:949— the comment still says "Reconstruct the command string and Basic/Advanced + Execution + Shell UI state", but this PR removedCommandModeState.advanced.git grepfor theadvancedidentifier finds nothing left inreact/srcore2e. The two other "No Basic/Advanced toggle" mentions (ServiceConfigurationFormItems.tsx:42,AdminDeploymentPresetSettingPageContent.tsx:738) are deliberate explanations and read fine. -
general.Basicis now orphaned in 21 locale files. #8170 added it for the Segmented control; this PR removed both call sites (DeploymentAddRevisionModal.tsx:302,AdminDeploymentPresetSettingPageContent.tsx:951) and there are no remaining uses. Harmless if you want to keep it as a generic word ingeneral, but it was introduced and orphaned inside the same stack. -
packages/backend.ai-client/src/client.ts:978— "an omittedshellis null, which disables shell wrapping and turns the Basic mode into Exec". Basic mode is gone as of this PR. I initially read this as invalidating the gate's rationale and went back to check: it does not.AdminDeploymentPresetSettingPage.tsx:107still sendsshell: undefinedwhen the command is empty, so the omitted-shell path is live and the 26.8.0 gate is still justified. Only the wording is stale.
One thing I looked at and decided is fine: the form field is named startCommand but holds the raw command string that is usually submitted as command. AdminDeploymentPresetFormTypes.ts says so directly, and renaming it would break #8333's DOM-id locators a second time. Leaving it alone is the better trade.
Merged
5 tasks
Brings the stack's parent (and through it the latest main) onto this branch so CI runs against current code. No conflicts; merge only.
This was referenced Aug 21, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Resolves #8602 (FR-3474)
Stacked on #8170 (FR-3205).
Summary
DeploymentAddRevisionModal.tsx(Add Revision modal, Custom mode) andAdminDeploymentPresetSettingPageContent.tsx(Admin Deployment Preset form)each inlined a near-byte-identical block of
antdForm.Items for threeconcerns: Service Configuration (Basic/Advanced Segmented + Execution radio +
Shell AutoComplete + Command input + Port), Health Check (checkbox + 6 detail
fields), and Pre-Start Actions (dynamic action/args list). Built in parallel
during FR-3205/FR-3342, the JSX was copy-pasted rather than shared and was
starting to diverge (e.g. a Shell placeholder present in the preset form but
missing in the revision modal — now fixed as a side effect).
Extracted the shared JSX into
react/src/components/ModelServiceFormItems/(
ServiceConfigurationFormItems,ModelServiceHealthCheckFormItems,PreStartActionsFormList), each parameterized by anamePrefixarray propso it works for both the revision modal's flat form (
namePrefix: []) andthe preset form's nested one (
namePrefix: ['modelDefinition', 'models', 0, 'service']). Each page's own GraphQL submit-mapping stays fully separate —the two mutations (
ModelServiceConfigInputvsPresetModelServiceConfigInput)have real, intentional differences (nullability, health-check
enablesemantics) that don't belong in shared UI code.
Why the preset form's field names changed, not the revision modal's
For
namePrefixto be a pure array-prepend (no per-field name mapping insidethe shared components), both forms need identical leaf field names.
Two separate changes happened here, and an earlier draft of this section
conflated them:
the revision modal's structure. It had no e2e coverage at the time, while
PR test(FR-3344): add E2E coverage for custom deployment add-revision (FR-3205 + FR-3342) #8333 (FR-3344, stacked on top of this one) locates several revision
modal fields by their antd-derived DOM id, so the preset form was the
cheaper side to move.
commandprefix was then dropped from both forms(
commandAdvanced/commandExecution/commandShell/commandPort→advanced/execution/shell/port). It turned out to be a copy-pasteartifact with no structural meaning, not a deliberate convention. This
does change the revision modal's ids, and test(FR-3344): add E2E coverage for custom deployment add-revision (FR-3205 + FR-3342) #8333's locator updates for it
ship in that PR — verified: no
commandShell/commandPort/commandExecutionreference remains anywhere in the stack, ine2e/orreact/src.PR #8333's own locator updates for this rename (and its new Admin Deployment
Preset e2e coverage) live in that PR, stacked on top of this one.
Verification
bash scripts/verify.sh— PASS (Relay/Lint/Format/TypeScript/Terminology).mode) and Admin Deployment Preset create/edit flows.
PR's rename is exactly what that suite exercises.
Astryx rebase note (2026-08-11)
Rebased onto the Astryx-migrated main (#8626). The extracted
ModelServiceFormItems/*shared components were re-expressed in Astryx idiom (form-engine +AstryxForm*+Collapsible); prop shapes, validation rules, and i18n keys unchanged.modelServiceCommand.ts+ tests are byte-identical to the pre-rebase branch. Stale antd-era comments were reworded and the why-comments for the port/status-code minimums restored. Verified:scripts/verify.shALL PASS, react vitest 1174 passed, BUI vitest 583 passed.