feat(FR-854): add a reset action for image resource limits - #9487
Draft
yomybaby wants to merge 2 commits into
Draft
feat(FR-854): add a reset action for image resource limits#9487yomybaby wants to merge 2 commits into
yomybaby wants to merge 2 commits into
Conversation
The manager has exposed `clear_image_custom_resource_limit` since 25.6.0, but nothing in the WebUI called it: once an admin set custom minimum resource limits on an image there was no way back to the values the image itself declares. Add a "Reset to Default" action to the footer of ManageImageResourceLimitModal. Resetting is reversible - the admin can retype the limits - so it uses the anchored BAIPopconfirm tier rather than a typed-confirmation modal. The mutation is keyed on the image canonical string, not the node id, so the action is disabled when any part of that key is missing, and hidden entirely on managers older than 25.6.0. Its payload carries only `image_node`, so failures are read off the GraphQL errors instead of an ok/msg pair; the selection set returns `resource_limits` so Relay patches the row in the normalized store. Claude-Session: https://claude.ai/code/session_011RFmSEBxxxvCXyquSPtqVJ
github-actions
Bot
added
area:ux
Contributor
Coverage Report for react-coverage (./react)
File Coverage
|
||||||||||||||||||||||||||||||||||||||
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The clear and save mutations can overlap, making the resulting resource limits response-order dependent.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds a version-gated action for restoring an image’s default resource limits.
Changes:
- Adds the
clear_image_custom_resource_limitmutation and confirmation UI. - Adds localized labels and success messages.
- Adds Relay artifacts and tests for canonical keys and version gating.
File summaries
| File | Description |
|---|---|
react/src/components/ManageImageResourceLimitModal.tsx |
Adds reset mutation and UI action. |
react/src/components/ManageImageResourceLimitModal.test.tsx |
Tests reset requests and version gating. |
react/src/__generated__/ManageImageResourceLimitModalClearMutation.graphql.ts |
Generated clear-mutation artifact. |
react/src/__generated__/ManageImageResourceLimitModalTestQuery.graphql.ts |
Generated test-query artifact. |
resources/i18n/en.json |
Adds English reset strings. |
resources/i18n/ko.json |
Adds Korean reset strings. |
resources/i18n/de.json |
Adds reset placeholders. |
resources/i18n/el.json |
Adds reset placeholders. |
resources/i18n/es.json |
Adds reset placeholders. |
resources/i18n/fi.json |
Adds reset placeholders. |
resources/i18n/fr.json |
Adds reset placeholders. |
resources/i18n/id.json |
Adds reset placeholders. |
resources/i18n/it.json |
Adds reset placeholders. |
resources/i18n/ja.json |
Adds reset placeholders. |
resources/i18n/mn.json |
Adds reset placeholders. |
resources/i18n/ms.json |
Adds reset placeholders. |
resources/i18n/pl.json |
Adds reset placeholders. |
resources/i18n/pt.json |
Adds reset placeholders. |
resources/i18n/pt-BR.json |
Adds reset placeholders. |
resources/i18n/ru.json |
Adds reset placeholders. |
resources/i18n/tr.json |
Adds reset placeholders. |
resources/i18n/vi.json |
Adds reset placeholders. |
resources/i18n/zh-CN.json |
Adds reset placeholders. |
resources/i18n/zh-TW.json |
Adds reset placeholders. |
Review details
Files not reviewed (2)
- react/src/generated/ManageImageResourceLimitModalClearMutation.graphql.ts: Generated file
- react/src/generated/ManageImageResourceLimitModalTestQuery.graphql.ts: Generated file
- Files reviewed: 22/24 changed files
- Comments generated: 3
- Review effort level: Balanced
Comment on lines
+133
to
+134
| message.success(t('environment.DescImageResourceLimitReset')); | ||
| onRequestClose(true); |
Guard the Save button while the clear mutation is in flight. The reset trigger already refused to start while `modify_image` was pending, but the mirror was missing: `modify_image` could be started from Save while `clear_image_custom_resource_limit` was still settling, and the two writes race over the same `resource_limits`. Cover the canonical-key guard the PR flagged as safety-critical: reset is disabled when `registry`, `tag`, or both `name`/`namespace` are absent, and the key falls back to `namespace` when the deprecated `name` is null. vitest: 6 tests pass. scripts/verify.sh: === ALL PASS === Claude-Session: https://claude.ai/code/session_011RFmSEBxxxvCXyquSPtqVJ
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The mutation is invalid against supported pre-26.1 managers, and pending resets can close a subsequently opened modal.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Files not reviewed (2)
- react/src/generated/ManageImageResourceLimitModalClearMutation.graphql.ts: Generated file
- react/src/generated/ManageImageResourceLimitModalTestQuery.graphql.ts: Generated file
Suppressed comments (1)
react/src/components/ManageImageResourceLimitModal.tsx:134
- This successful reset already returns
image_node { id resource_limits }, so Relay patches the existing row, butonRequestClose(true)makesImageListcallupdateFetchKey()and requery the entire list. Preserve truthful success reporting while distinguishing reset from the legacymodify_imagepath at the caller, and refetch only for the latter.
onRequestClose(true);
- Files reviewed: 22/24 changed files
- Comments generated: 2
- Review effort level: Balanced
| useMutation<ManageImageResourceLimitModalClearMutation>(graphql` | ||
| mutation ManageImageResourceLimitModalClearMutation( | ||
| $imageCanonical: String! | ||
| $architecture: String |
Comment on lines
+221
to
+224
| isDisabled={ | ||
| !canResetResourceLimit || isInFlightModifyImageInput | ||
| } | ||
| isLoading={isInFlightClearResourceLimit} |
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 #3523 (FR-854)
clear_image_custom_resource_limithas been on the manager since 25.6.0, but nothing in the WebUI ever called it. Once an admin set custom minimum resource limits on an image there was no way back to the values the image itself declares — the only image resource-limit UI,ManageImageResourceLimitModal, could only write new limits throughmodify_image.This adds a Reset to Default action to that modal's footer.
Design decisions
BAIPopconfirm(.claude/rules/destructive-confirmation.md), notBAIDeleteConfirmModal.isManagerVersionCompatibleWith('25.6.0')is false. Permission-wise nothing extra is needed:ClearImageCustomResourceLimitandModifyImageare bothSUPERADMIN-only on the manager, so the reset action has exactly the same reach as the Save button next to it.clear_image_custom_resource_limit_by_idfield indata/schema.graphql, so the legacy canonical-keyed mutation is the only option. The canonical string is built from the same expression the save path already uses, and the button is disabled when any part of that key (registry/name‖namespace/tag) is missing rather than sending a malformednull/null:null.errorsarray.ClearImageCustomResourceLimitPayloadreturns onlyimage_node— there is nook/msgpair like the siblingmodify_imagepath has.image_node { id resource_limits { … } }, which is exactly what both the list query and the modal fragment read, so Relay patches the normalized record.onRequestClose(true)is still called so the caller's existing handling (ImageList'supdateFetchKey(), which the legacymodify_imagepath genuinely needs) stays truthful.New i18n keys:
environment.ResetImageResourceLimit,environment.DescResetImageResourceLimit,environment.DescImageResourceLimitReset(en + ko written by hand, the other 20 locales carry the__NOT_TRANSLATED__placeholdersmake i18nemits).Tests
react/src/components/ManageImageResourceLimitModal.test.tsx(vitest +relay-test-utils): asserts the clear mutation goes out with the canonical key and architecture of the edited image, and that the action is absent on a manager older than 25.6.0.pnpm exec vitest run src/components/ManageImageResourceLimitModal.test.tsx src/components/ImageList.test.tsx src/pages/EnvironmentPage.test.tsx→ 3 files, 21 tests passed.pnpm run relay— generated artifacts committed.Verification
bash scripts/verify.sh:Review notes
isManagerVersionCompatibleWith) and confirm the footer collapses back to the plain Cancel/Save pair with no leftover gap.image_canonicalis the only handle the mutation takes, so an image whoseregistry/tagis null must not be resettable — that is whatcanResetResourceLimitprotects.Checklist: (if applicable)
react/src/components/ManageImageResourceLimitModal.test.tsxhttps://claude.ai/code/session_011RFmSEBxxxvCXyquSPtqVJ