feat(FR-3468): add per-field revert-to-loaded-revision icon in DeploymentAddRevisionModal - #9520
feat(FR-3468): add per-field revert-to-loaded-revision icon in DeploymentAddRevisionModal#9520yomybaby wants to merge 3 commits into
Conversation
…mentAddRevisionModal The Custom form of the Add-revision modal could only be reset wholesale (`resetFields`), so undoing one accidental edit meant losing every other edit made alongside it. Snapshot the values the Custom form was last loaded with from a revision (the deployment's current revision via "Load current revision", or the source revision the modal was opened from) and render a small ghost Undo2 icon next to each field whose value has since diverged from that snapshot. Clicking it reverts only that field. The icon is hidden while the field still matches, and never appears at all on a form that was never loaded from a revision. The baseline is taken from the DERIVED form values that `applyRevisionToCustomForm` writes, not from the raw GraphQL revision: the prefill applies non-round-trippable transforms (resource slots, shmem units, health-check enable handling), so comparing against the raw revision would light up fields the user never touched. Scope is the fields this modal renders itself — model folder, mount destination, subpath, runtime variant and model definition path. The shared form-item components (resource allocation, image environment, health check) own their own layout and are left alone. Whole-form reset is unchanged, and Preset mode has no baseline (switching to it discards the Custom edits). The visibility check subscribes with `Form.useWatch` rather than a `dependencies` render prop: the engine notifies dependency watchers on user input but not on a programmatic `setFieldValue`, so a `dependencies`-driven button would have survived its own click. Claude-Session: https://claude.ai/code/session_011RFmSEBxxxvCXyquSPtqVJ
Coverage Report for react-coverage (./react)
File Coverage
|
||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment.
🟡 Changes recommended
Reverting modelFolderId can leave an invalid stale modelSubpath.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds per-field controls for reverting selected Custom form fields to their loaded revision values.
Changes:
- Tracks revision baselines and conditionally displays revert buttons.
- Covers five modal-owned fields with focused tests.
- Adds localized tooltip keys across supported locales.
File summaries
| File | Description |
|---|---|
resources/i18n/zh-TW.json |
Registers the revert tooltip key. |
resources/i18n/zh-CN.json |
Registers the revert tooltip key. |
resources/i18n/vi.json |
Registers the revert tooltip key. |
resources/i18n/tr.json |
Registers the revert tooltip key. |
resources/i18n/ru.json |
Registers the revert tooltip key. |
resources/i18n/pt.json |
Registers the revert tooltip key. |
resources/i18n/pt-BR.json |
Registers the revert tooltip key. |
resources/i18n/pl.json |
Registers the revert tooltip key. |
resources/i18n/ms.json |
Registers the revert tooltip key. |
resources/i18n/mn.json |
Registers the revert tooltip key. |
resources/i18n/ko.json |
Adds Korean tooltip text. |
resources/i18n/ja.json |
Registers the revert tooltip key. |
resources/i18n/it.json |
Registers the revert tooltip key. |
resources/i18n/id.json |
Registers the revert tooltip key. |
resources/i18n/fr.json |
Registers the revert tooltip key. |
resources/i18n/fi.json |
Registers the revert tooltip key. |
resources/i18n/es.json |
Registers the revert tooltip key. |
resources/i18n/en.json |
Adds English tooltip text. |
resources/i18n/el.json |
Registers the revert tooltip key. |
resources/i18n/de.json |
Registers the revert tooltip key. |
react/src/components/DeploymentAddRevisionModal.tsx |
Implements baseline tracking and per-field reverts; folder reversion must also clear a stale subpath. |
react/src/components/DeploymentAddRevisionModal.test.tsx |
Tests revert visibility and single-field restoration. |
Review details
- Files reviewed: 22/22 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.
Reverting `modelFolderId` through the per-field revert button calls `setFieldValue`, which bypasses `BAIVFolderSelect.onChange` and its subpath cleanup, so a subpath picked under the edited folder could be submitted against the loaded revision's folder. Restore the loaded revision's own subpath alongside the folder, mirroring the existing programmatic folder-change cleanup. Claude-Session: https://claude.ai/code/session_011RFmSEBxxxvCXyquSPtqVJ
There was a problem hiding this comment.
🟡 Changes recommended
Accessibility associations and coupled folder/subpath regression coverage must be fixed before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (6)
Previously missed (3) — in code that hasn't changed since the last review.
react/src/components/DeploymentAddRevisionModal.tsx:2466
- Moving the field
nameto the nestednoStyleitem leaves this outer label without a generatedhtmlFor, so clicking the visible label no longer focuses the mount-destination input. Preserve the field name on the layout item as well, matching the repository's established nested-item pattern.
This issue also appears on line 2483 of the same file.
react/src/components/DeploymentAddRevisionModal.tsx:2518
- Moving the field
nameto the nestednoStyleitem leaves the visible label unassociated with the selector. It also leaves the outer warning container without theruntimeVariantId_helpid referenced by the control'saria-describedby; retain the name on this layout item so both associations remain valid.
react/src/components/DeploymentAddRevisionModal.tsx:2683 - Moving the field
nameto the nestednoStyleitem leaves this outer label without a generatedhtmlForand leaves its validation message without the id referenced by the input'saria-describedby. Retain the name on the layout item so the label and validation text stay associated with the input.
react/src/components/DeploymentAddRevisionModal.tsx:2504
- When the selected folder differs from the loaded revision, this button can restore the old folder's subpath into the new folder. Changing
modelFolderIddeliberately clearsmodelSubpath, so the subpath baseline is only valid while the folder itself still matches; hide this revert action until then (and cover this sequence in a regression test).
<RevertToRevisionValueButton
name="modelSubpath"
baseline={revisionBaseline}
form={customForm}
/>
react/src/components/DeploymentAddRevisionModal.tsx:2487
- Moving the field
nameto the nestednoStyleitem leaves this outer label without a generatedhtmlFor, so clicking the visible label no longer focuses the subpath picker. Preserve the field name on the layout item as well, matching the repository's established nested-item pattern.
<BAIFormItem
label={t('modelService.Subpath')}
tooltip={t('modelService.SubpathTooltip')}
style={{ flex: 1 }}
>
react/src/components/DeploymentAddRevisionModal.tsx:324
- This new user-facing tooltip resolves to the literal
__NOT_TRANSLATED__in every managed locale except English and Korean because those locale files contain a real value rather than a missing key, so the English fallback is not used. Provide translations for the other 18 locale entries before release.
label={t('deployment.RevertToLoadedRevisionValue')}
tooltip={t('deployment.RevertToLoadedRevisionValue')}
- Files reviewed: 22/22 changed files
- Comments generated: 1
- Review effort level: Balanced
…he tooltip Copilot pass 2: - The subpath's own revert would restore the loaded revision's path into a folder it does not belong to whenever the folder itself has diverged, so it is withheld until the folder matches the loaded revision again; the folder's revert restores the pair together. - Regression spec covers the coupled sequence: pick another folder (its onChange clears the subpath), confirm only the folder offers a revert, then revert it and assert both baseline values come back. - `make i18n` seeded `deployment.RevertToLoadedRevisionValue` as `__NOT_TRANSLATED__` in the 18 non-en/ko locales, which is what users of those locales would have seen in the tooltip. Translated, following the `LoadCurrentRevision` idiom that keeps "Revision" as the term. Claude-Session: https://claude.ai/code/session_011RFmSEBxxxvCXyquSPtqVJ
|
Copilot pass 2 left three findings in the review body only ("suppressed comments"), with no thread to answer. Recording what happened to each: Acted on
Left open for a human decision
verify.sh: |
Resolves #8587 (FR-3468)
What changed
The Custom form of
DeploymentAddRevisionModalcould only be reset wholesale (resetFields), so undoing one accidental edit meant losing every other edit made alongside it.This PR snapshots the values the Custom form was last loaded with from a revision — the deployment's current revision via Load current revision, or the source revision the modal was opened from — and renders a small ghost
Undo2icon next to each field whose value has since diverged from that snapshot. Clicking it reverts only that field.Design decisions
applyRevisionToCustomFormapplies non-round-trippable transforms (resource slots → cpu/mem/accelerator, shmem units, health-checkenablehandling). Comparing against the raw revision would light the icon up on fields the user never touched, so the snapshot is taken from the object that is actually pushed into the form.ResourceAllocationFormItems,ImageEnvironmentSelectFormItems,ModelServiceHealthCheckFormItems, …) own their own layout and are used by other screens, so they are left untouched. The issue explicitly leaves the exact list to implementation.IconButtonin aBAIFlexrow, the same shape the model-folder selector already uses — rather than injecting a button into the<label>.Form.useWatch, not adependenciesrender prop. The form engine notifies dependency watchers on user input but not on a programmaticsetFieldValue, so adependencies-driven button would have survived its own click (caught by the test below before it shipped).undefinedare normalized to the same thing for the comparison: clearing a text input that the revision did not set is not a divergence.Tests
react/src/components/DeploymentAddRevisionModal.test.tsx— two new specs: no revert affordance is rendered while every field still matches the loaded revision, and editing one field then clicking its revert icon restores exactly that value and withdraws the icon.pnpm --filter backend-ai-webui-react exec vitest run src/components/DeploymentAddRevisionModal.test.tsx→ 5 passed (5).make i18nfor the new key across the locale files.Verification
bash scripts/verify.sh(Relay, Lint, Format, TypeScript, agent mappings, terminology):Review notes
Checklist: (if applicable)
https://claude.ai/code/session_011RFmSEBxxxvCXyquSPtqVJ