Conversation
`DeploymentRevisionPreset` exists since 26.4.2 but its `modelDefinition` field only since 26.4.4, and neither fragment on the preset type gated it, so a 26.4.2 / 26.4.3 manager rejected the whole preset query instead of the health-check section quietly hiding. The v26.9.0 risk digest surfaced this as PresetModelConfig.name / .service, PresetModelDefinition.models and PresetModelServiceConfig.healthCheck used without @SInCE in DeploymentPresetDetailModal.tsx. Those types are the 26.9.0 renames of ModelDefinition / ModelConfig / ModelServiceConfig with identical field names, so 26.9.0 is not the boundary; gating them there would hide data that 26.4.4 - 26.8.x managers do return. Annotating the `modelDefinition` ancestor at 26.4.4 closes the real gap and satisfies the digest rule for every nested selection. Both consumers already read `modelDefinition` with optional chaining. Resolves #9406 (FR-3844) Co-Authored-By: Claude Fable 5.1 <[email protected]> Claude-Session: https://claude.ai/code/session_01DH9zan75dZ8t5X5TjHdsP3
Contributor
Coverage Report for react-coverage (./react)
File Coverage
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
…ports() The @SInCE annotation only strips the selection; the preset form still rendered the Model Definition card on 26.4.2 / 26.4.3 managers and sent `modelDefinition` in the create/update input, which those managers reject ("Added in 26.4.4"). Input fields cannot carry @SInCE, so this adds a `preset-model-definition` feature flag (26.4.4) and uses it to: - omit the `modelDefinition` key from both preset mutations, - hide the Model Definition card in the preset form, - make the detail modal's service-configuration section depend on support explicitly rather than on the stripped field being empty. Co-Authored-By: Claude Fable 5.1 <[email protected]> Claude-Session: https://claude.ai/code/session_01DH9zan75dZ8t5X5TjHdsP3
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Unsupported model-definition data can still appear during review after URL state restoration.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds v26.4.4 capability gating for deployment preset model definitions.
Changes:
- Adds the
preset-model-definitioncapability. - Gates related queries, mutation inputs, and UI.
- Regenerates affected Relay artifacts.
File summaries
| File | Review |
|---|---|
react/src/pages/AdminDeploymentPresetSettingPage.tsx |
Omits unsupported mutation input. Nit: Missing pre-26.4.4 E2E coverage. |
react/src/components/DeploymentPresetDetailModal.tsx |
Gates model-definition details. |
react/src/components/AdminDeploymentPresetSettingPageContent.tsx |
Gates the form card. Moderate: URL-restored state can still display unsupported review data. |
react/src/__generated__/VFolderDeployModalQuery.graphql.ts |
Regenerates Relay query. |
react/src/__generated__/ModelCardDrawerQuery.graphql.ts |
Regenerates Relay query. |
react/src/__generated__/DeploymentPresetDetailModalFragment.graphql.ts |
Regenerates Relay fragment. |
react/src/__generated__/DeploymentAddRevisionModalPresetDetailQuery.graphql.ts |
Regenerates Relay query. |
react/src/__generated__/DeploymentAddRevisionModalCardDetailQuery.graphql.ts |
Regenerates Relay query. |
react/src/__generated__/AdminDeploymentPresetSettingPageUpdateMutation.graphql.ts |
Regenerates Relay mutation. |
react/src/__generated__/AdminDeploymentPresetSettingPagePresetQuery.graphql.ts |
Regenerates Relay query. |
react/src/__generated__/AdminDeploymentPresetSettingPageCreateMutation.graphql.ts |
Regenerates Relay mutation. |
react/src/__generated__/AdminDeploymentPresetSettingPageContent_preset.graphql.ts |
Regenerates Relay fragment. |
packages/backend.ai-client/src/client.ts |
Adds the v26.4.4 capability flag. |
Review details
Files not reviewed (9)
- react/src/generated/AdminDeploymentPresetSettingPageContent_preset.graphql.ts: Generated file
- react/src/generated/AdminDeploymentPresetSettingPageCreateMutation.graphql.ts: Generated file
- react/src/generated/AdminDeploymentPresetSettingPagePresetQuery.graphql.ts: Generated file
- react/src/generated/AdminDeploymentPresetSettingPageUpdateMutation.graphql.ts: Generated file
- react/src/generated/DeploymentAddRevisionModalCardDetailQuery.graphql.ts: Generated file
- react/src/generated/DeploymentAddRevisionModalPresetDetailQuery.graphql.ts: Generated file
- react/src/generated/DeploymentPresetDetailModalFragment.graphql.ts: Generated file
- react/src/generated/ModelCardDrawerQuery.graphql.ts: Generated file
- react/src/generated/VFolderDeployModalQuery.graphql.ts: Generated file
Suppressed comments (1)
react/src/pages/AdminDeploymentPresetSettingPage.tsx:223
- The new compatibility branch is not covered by the existing preset E2E scenarios: both flag installers leave
preset-model-definitionto the test manager's normal value, so it remains true and neither the card-hidden path nor omission from create/update inputs is asserted. Add a pre-26.4.4 capability override and verify that the card is absent and the captured mutation input has nomodelDefinitionkey; this guards the exact regression this change addresses.
// `modelDefinition` exists on the preset input since 26.4.4; older managers
// reject the key, so it is left out of the mutation entirely.
const supportsModelDefinition = baiClient.supports('preset-model-definition');
- Files reviewed: 4/13 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
Create mode syncs form values into the URL, so a restored link can carry `modelDefinition.enabled` on a manager that predates preset model definitions. The submit already omits the field; the review summary now hides the rows too so what is shown matches what is saved. Co-Authored-By: Claude Fable 5.1 <[email protected]> Claude-Session: https://claude.ai/code/session_01DH9zan75dZ8t5X5TjHdsP3
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The only unresolved finding is a non-blocking test-coverage nit.
Review details
Files not reviewed (9)
- react/src/generated/AdminDeploymentPresetSettingPageContent_preset.graphql.ts: Generated file
- react/src/generated/AdminDeploymentPresetSettingPageCreateMutation.graphql.ts: Generated file
- react/src/generated/AdminDeploymentPresetSettingPagePresetQuery.graphql.ts: Generated file
- react/src/generated/AdminDeploymentPresetSettingPageUpdateMutation.graphql.ts: Generated file
- react/src/generated/DeploymentAddRevisionModalCardDetailQuery.graphql.ts: Generated file
- react/src/generated/DeploymentAddRevisionModalPresetDetailQuery.graphql.ts: Generated file
- react/src/generated/DeploymentPresetDetailModalFragment.graphql.ts: Generated file
- react/src/generated/ModelCardDrawerQuery.graphql.ts: Generated file
- react/src/generated/VFolderDeployModalQuery.graphql.ts: Generated file
Suppressed comments (1)
react/src/pages/AdminDeploymentPresetSettingPage.tsx:223
- The new pre-26.4.4 compatibility path is not exercised by the listed Vitest tests, although
e2e/serving/admin-preset-service-config.spec.tsalready tests this page's capability-controlled rendering and mutation payloads. Add an unsupported-manager scenario that forcespreset-model-definitionoff and verifies the card/review data stay hidden andmodelDefinitionis absent from the submitted input; otherwise the core FR-3844 behavior can regress while verification still passes.
// `modelDefinition` exists on the preset input since 26.4.4; older managers
// reject the key, so it is left out of the mutation entirely.
const supportsModelDefinition = baiClient.supports('preset-model-definition');
- Files reviewed: 5/14 changed files
- Comments generated: 0 new
- Review effort level: Balanced
Contributor
Author
|
Dev server on box |
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 #9406 (FR-3844)
Why
The v26.9.0 release risk digest (FR-3663) flagged
PresetModelConfig.name/.service,PresetModelDefinition.modelsandPresetModelServiceConfig.healthCheckas used without@sinceinDeploymentPresetDetailModal.tsx.Those three types are the 26.9.0 (BA-7210 / FR-3481) renames of
ModelDefinition/ModelConfig/ModelServiceConfig; every field name the WebUI selects exists on both generations, so 26.9.0 is not the version boundary for the query. Gating those fields at 26.9.0 would hide preset health-check data that 26.4.4 – 26.8.x managers do return.The real gap is one level up:
DeploymentRevisionPresetexists since 26.4.2, but itsmodelDefinitionfield (query and input) only since 26.4.4, and nothing on the preset surfaces gated it:modelDefinitionungated, so a 26.4.2 / 26.4.3 manager rejected the whole preset query (the FR-3673 failure class);modelDefinitionin the create / update input, which those managers reject. Input fields cannot carry@since, so only asupports()guard can cover that side.What
Query gating (
@since)DeploymentPresetDetailModalFragment(also reused byDeploymentAddRevisionModalPresetDetailQuery) andAdminDeploymentPresetSettingPageContent_preset:modelDefinition @since(version: "26.4.4"). Relay artifacts regenerated.Render / input gating (
supports())client.ts: newpreset-model-definitionfeature flag, set at 26.4.4 next tomodel-mount-subpath.AdminDeploymentPresetSettingPage.tsx: themodelDefinitionkey is omitted from both preset mutations when unsupported (same...(flag && { key })pattern assubpath).AdminDeploymentPresetSettingPageContent.tsx: the Model Definition card is not rendered when unsupported. On those managerspreset-model-config-typeis also false, so the Service Configuration / Health Check / Pre-Start Actions items (nested in that card for legacy managers) disappear with it; the review step already keys off the switch, so it shows nothing extra.DeploymentPresetDetailModal.tsx: the service-configuration section depends on the flag explicitly instead of on the stripped field happening to be empty.AdminDeploymentPresetReviewSummary.tsx: the review-step Model Definition rows are gated on the same flag, so a create-mode URL restored withmodelDefinition.enabledon an unsupported manager does not show data the submit omits (Copilot pass 1).A gated ancestor covers every nested selection under the digest's rule, so the four flagged fields no longer appear.
Dev server
http://fr-3844-pr9408-version-control.sujin.10-82-0-159.sslip.io/admin/deployments?tab=deployment-presets (dev VPN, via dev-gw; login form pre-fills the shared test backend and account)
How to check:
modelDefinitionomitted from the mutation) needs an older manager or asupports('preset-model-definition')override; it is not reachable on the shared backend — see FR-3845 for the E2E that will cover it.Verification
bash scripts/verify.sh→=== ALL PASS ===vitest run src/components/DeploymentAddRevisionModal.test.tsx src/form-engine/formEngineAcceptance.test.tsx→ 34 passednode scripts/release-risk-report.mjs --from v26.8.1 --to <ref> --json, version-gating gaps mentioning the preset types:origin/main: 4 (PresetModelConfig.name,PresetModelConfig.service,PresetModelDefinition.models,PresetModelServiceConfig.healthCheck, all inDeploymentPresetDetailModal.tsx)Not covered here:
descriptionandimageIdon the same preset input are also "Added in 26.4.4" and remain unguarded, as before this PR.🤖 Generated with Claude Code
https://claude.ai/code/session_01DH9zan75dZ8t5X5TjHdsP3