fix(clusters): show advanced settings save action on payload changes - #2916
Open
rmnbrd wants to merge 1 commit into
Open
fix(clusters): show advanced settings save action on payload changes#2916rmnbrd wants to merge 1 commit into
rmnbrd wants to merge 1 commit into
Conversation
- Compare normalized settings payloads instead of raw form dirtiness - Preserve sticky toaster visibility during exit animations
|
View your CI Pipeline Execution ↗ for commit fd428d2
💡 Verify your cache is correct by running tasks in a sandbox. Read docs ↗ ☁️ Nx Cloud last updated this comment at |
Contributor
Author
|
@@cubic-dev-ai Can you review this PR? |
Contributor
@rmnbrd I have started the AI code review. It will take a few minutes to complete. |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
3 issues found across 7 files
Confidence score: 2/5
buildClusterAdvancedSettingsPayload.tscan omit unchanged fields whose names contain.when another setting is edited, causing saved configuration to lose existing values; retain the flat key unless a nested value for the same path exists.cluster-advanced-settings-feature.tsxcompares a raw form baseline with a payload that substitutes defaults, so editing an unrelated field can incorrectly show the save action; normalize the baseline and current payload consistently before comparison.cluster-advanced-settings-feature.spec.tsxdoes not cover the normalized-payload behavior being changed, leaving the field-omission and false-dirty-state regressions undetected; add assertions for both cases.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="libs/domains/clusters/feature/src/lib/cluster-advanced-settings/cluster-advanced-settings-feature.tsx">
<violation number="1" location="libs/domains/clusters/feature/src/lib/cluster-advanced-settings/cluster-advanced-settings-feature.tsx:30">
P2: When a loaded setting is empty or null and its default is non-empty, editing any other field incorrectly makes the save action appear because the baseline is raw while the form payload substitutes the default. Initialize `formBaseline` through `buildClusterAdvancedSettingsPayload` and update it when `defaultAdvancedSettings` becomes available.</violation>
</file>
<file name="libs/domains/clusters/feature/src/lib/cluster-advanced-settings/build-cluster-advanced-settings-payload.ts">
<violation number="1" location="libs/domains/clusters/feature/src/lib/cluster-advanced-settings/build-cluster-advanced-settings-payload.ts:12">
P1: When another setting is edited, this deletion drops unchanged fields whose names contain `.` from the payload. Retain the flat key unless a nested value for the same path exists, so saving one setting does not omit the others.</violation>
</file>
<file name="libs/domains/clusters/feature/src/lib/cluster-advanced-settings/cluster-advanced-settings-feature.spec.tsx">
<violation number="1" location="libs/domains/clusters/feature/src/lib/cluster-advanced-settings/cluster-advanced-settings-feature.spec.tsx:36">
P2: This test does not exercise the behavior the PR fixes, so it won't catch the regression. The new logic compares the normalized `buildClusterAdvancedSettingsPayload(formValues, ...)` against `formBaseline` instead of raw `formState.isDirty`; the regression is that payload-equivalent (formatting-only) edits wrongly trigger the save banner. This test only edits the value from `1` to `2`, which is a genuine payload change, so the banner shows under both the old `isDirty` logic and the new logic — it passes against the pre-fix code and validates nothing new. Add a case that types a payload-equivalent value (e.g. whitespace or equivalent JSON formatting) and asserts the toaster stays `hidden`, plus a case asserting the toaster stays visible during the exit-delay transition that this PR also touches.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
|
||
| Object.keys(dataWithoutFlatKeys).forEach((key) => { | ||
| if (key.includes('.')) { | ||
| delete dataWithoutFlatKeys[key] |
Contributor
There was a problem hiding this comment.
P1: When another setting is edited, this deletion drops unchanged fields whose names contain . from the payload. Retain the flat key unless a nested value for the same path exists, so saving one setting does not omit the others.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At libs/domains/clusters/feature/src/lib/cluster-advanced-settings/build-cluster-advanced-settings-payload.ts, line 12:
<comment>When another setting is edited, this deletion drops unchanged fields whose names contain `.` from the payload. Retain the flat key unless a nested value for the same path exists, so saving one setting does not omit the others.</comment>
<file context>
@@ -0,0 +1,34 @@
+
+ Object.keys(dataWithoutFlatKeys).forEach((key) => {
+ if (key.includes('.')) {
+ delete dataWithoutFlatKeys[key]
+ }
+ })
</file context>
Suggested change
| delete dataWithoutFlatKeys[key] | |
| if ( | |
| key.split('.').reduce<unknown>((value, part) => { | |
| return value !== null && typeof value === 'object' && part in value | |
| ? (value as Record<string, unknown>)[part] | |
| : undefined | |
| }, dataWithoutFlatKeys) !== undefined | |
| ) { | |
| delete dataWithoutFlatKeys[key] | |
| } |
| useEffect(() => { | ||
| if (clusterAdvancedSettings) { | ||
| methods.reset(initFormValues(keys, clusterAdvancedSettings)) | ||
| setFormBaseline(clusterAdvancedSettings) |
Contributor
There was a problem hiding this comment.
P2: When a loaded setting is empty or null and its default is non-empty, editing any other field incorrectly makes the save action appear because the baseline is raw while the form payload substitutes the default. Initialize formBaseline through buildClusterAdvancedSettingsPayload and update it when defaultAdvancedSettings becomes available.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At libs/domains/clusters/feature/src/lib/cluster-advanced-settings/cluster-advanced-settings-feature.tsx, line 30:
<comment>When a loaded setting is empty or null and its default is non-empty, editing any other field incorrectly makes the save action appear because the baseline is raw while the form payload substitutes the default. Initialize `formBaseline` through `buildClusterAdvancedSettingsPayload` and update it when `defaultAdvancedSettings` becomes available.</comment>
<file context>
@@ -19,57 +19,33 @@ export function ClusterAdvancedSettingsFeature() {
useEffect(() => {
if (clusterAdvancedSettings) {
methods.reset(initFormValues(keys, clusterAdvancedSettings))
+ setFormBaseline(clusterAdvancedSettings)
}
}, [clusterAdvancedSettings, keys, methods])
</file context>
| mockUseEditClusterAdvancedSettings.mockReturnValue({ mutateAsync: jest.fn() }) | ||
| }) | ||
|
|
||
| it('should keep the save banner hidden until the payload changes after the form reset', async () => { |
Contributor
There was a problem hiding this comment.
P2: This test does not exercise the behavior the PR fixes, so it won't catch the regression. The new logic compares the normalized buildClusterAdvancedSettingsPayload(formValues, ...) against formBaseline instead of raw formState.isDirty; the regression is that payload-equivalent (formatting-only) edits wrongly trigger the save banner. This test only edits the value from 1 to 2, which is a genuine payload change, so the banner shows under both the old isDirty logic and the new logic — it passes against the pre-fix code and validates nothing new. Add a case that types a payload-equivalent value (e.g. whitespace or equivalent JSON formatting) and asserts the toaster stays hidden, plus a case asserting the toaster stays visible during the exit-delay transition that this PR also touches.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At libs/domains/clusters/feature/src/lib/cluster-advanced-settings/cluster-advanced-settings-feature.spec.tsx, line 36:
<comment>This test does not exercise the behavior the PR fixes, so it won't catch the regression. The new logic compares the normalized `buildClusterAdvancedSettingsPayload(formValues, ...)` against `formBaseline` instead of raw `formState.isDirty`; the regression is that payload-equivalent (formatting-only) edits wrongly trigger the save banner. This test only edits the value from `1` to `2`, which is a genuine payload change, so the banner shows under both the old `isDirty` logic and the new logic — it passes against the pre-fix code and validates nothing new. Add a case that types a payload-equivalent value (e.g. whitespace or equivalent JSON formatting) and asserts the toaster stays `hidden`, plus a case asserting the toaster stays visible during the exit-delay transition that this PR also touches.</comment>
<file context>
@@ -0,0 +1,48 @@
+ mockUseEditClusterAdvancedSettings.mockReturnValue({ mutateAsync: jest.fn() })
+ })
+
+ it('should keep the save banner hidden until the payload changes after the form reset', async () => {
+ const { userEvent } = renderWithProviders(<ClusterAdvancedSettingsFeature />)
+
</file context>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR improves the “Advanced settings” save banner behavior by baselining and comparing a normalized advanced-settings payload (instead of relying on raw react-hook-form dirtiness), and by making the sticky toaster resilient to rapid hide/show transitions during its exit delay.
Changes:
- Add a normalized payload builder and compare normalized payloads against a stored baseline to decide when the save banner should be shown.
- Persist/update the “current payload baseline” after initial load and after a successful save.
- Fix sticky toaster exit-delay behavior by clearing the pending hide timeout when visibility returns to
true, and add regression tests for both payload equivalence and toaster transitions.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| libs/shared/ui/src/lib/components/sticky-action-form-toaster/sticky-action-form-toaster.tsx | Clears the exit-delay timeout via useEffect cleanup so the toaster stays visible if re-shown before the delay ends. |
| libs/shared/ui/src/lib/components/sticky-action-form-toaster/sticky-action-form-toaster.spec.tsx | Adds a fake-timers regression test to cover the exit-delay visibility transition. |
| libs/domains/clusters/feature/src/lib/cluster-advanced-settings/cluster-advanced-settings.tsx | Switches banner visibility to a normalized-payload comparison against a provided baseline instead of formState.isDirty. |
| libs/domains/clusters/feature/src/lib/cluster-advanced-settings/cluster-advanced-settings.spec.tsx | Updates/adds tests to cover payload-equivalent edits (e.g. 1 vs 1.0) and rapid payload changes. |
| libs/domains/clusters/feature/src/lib/cluster-advanced-settings/cluster-advanced-settings-feature.tsx | Tracks and updates formBaseline on load and on successful save, and uses the shared payload builder on submit. |
| libs/domains/clusters/feature/src/lib/cluster-advanced-settings/cluster-advanced-settings-feature.spec.tsx | Adds coverage ensuring the banner remains hidden after reset until the payload actually changes. |
| libs/domains/clusters/feature/src/lib/cluster-advanced-settings/build-cluster-advanced-settings-payload.ts | Extracts payload construction/parsing into a reusable helper shared by submit + payload comparison logic. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+50
to
+56
| const hasPayloadChanged = | ||
| formState.isDirty && | ||
| formBaseline !== undefined && | ||
| !equal( | ||
| buildClusterAdvancedSettingsPayload(formValues as Record<string, unknown>, defaultAdvancedSettings), | ||
| formBaseline | ||
| ) |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## fix-deploy-build-timer #2916 +/- ##
==========================================================
+ Coverage 48.52% 48.66% +0.14%
==========================================================
Files 1304 1218 -86
Lines 28128 26490 -1638
Branches 8220 7906 -314
==========================================================
- Hits 13648 12891 -757
+ Misses 12186 11439 -747
+ Partials 2294 2160 -134
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
3 existing issues remain and 1 new issue found across 7 files
Confidence score: 2/5
build-cluster-advanced-settings-payload.tscan drop untouched dotted settings when saving a different field, causing the submitted payload to omit existing configuration; preserve literal keys while allowing nested values to overwrite them.cluster-advanced-settings-feature.tsxcan show a save toaster when an empty loaded value has a non-empty default even though the normalized payload is unchanged; build the comparison baseline withbuildClusterAdvancedSettingsPayload.cluster-advanced-settings-feature.spec.tsxdoes not cover the normalized-payload comparison, leaving the primary regression insufficiently guarded; add a case for an untouched dotted setting and an empty value with a default.cluster-advanced-settings.tsxwatches the entire form and reparses every field on each keystroke, which may cause avoidable table-wide re-renders; narrow the watched state or memoize the payload computation.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="libs/domains/clusters/feature/src/lib/cluster-advanced-settings/cluster-advanced-settings.tsx">
<violation number="1" location="libs/domains/clusters/feature/src/lib/cluster-advanced-settings/cluster-advanced-settings.tsx:40">
P3: Watching the entire form with `useWatch({ control })` re-renders the component and recomputes `buildClusterAdvancedSettingsPayload` (parsing every field) on every keystroke, re-rendering the whole settings table for each edit. For tables with many settings this is avoidable work; limit the watch to the fields used in the comparison or memoize the payload derivation keyed on the watched values.</violation>
</file>
Requires human review: Auto-approval blocked by 3 unresolved issues from previous reviews.
Re-trigger cubic
| formBaseline, | ||
| }: ClusterAdvancedSettingsProps) { | ||
| const { control, formState, reset } = useFormContext<{ [key: string]: string }>() | ||
| const formValues = useWatch({ control }) |
Contributor
There was a problem hiding this comment.
P3: Watching the entire form with useWatch({ control }) re-renders the component and recomputes buildClusterAdvancedSettingsPayload (parsing every field) on every keystroke, re-rendering the whole settings table for each edit. For tables with many settings this is avoidable work; limit the watch to the fields used in the comparison or memoize the payload derivation keyed on the watched values.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At libs/domains/clusters/feature/src/lib/cluster-advanced-settings/cluster-advanced-settings.tsx, line 40:
<comment>Watching the entire form with `useWatch({ control })` re-renders the component and recomputes `buildClusterAdvancedSettingsPayload` (parsing every field) on every keystroke, re-rendering the whole settings table for each edit. For tables with many settings this is avoidable work; limit the watch to the fields used in the comparison or memoize the payload derivation keyed on the watched values.</comment>
<file context>
@@ -32,8 +34,10 @@ export function ClusterAdvancedSettings({
+ formBaseline,
}: ClusterAdvancedSettingsProps) {
const { control, formState, reset } = useFormContext<{ [key: string]: string }>()
+ const formValues = useWatch({ control })
const [showOverriddenOnly, toggleShowOverriddenOnly] = useState(false)
</file context>
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.
Summary
Issue: Fixed the Advanced Settings save banner to reliably show up only when the payload changes.
Jira ticket : QOV-2209
Screenshots / Recordings
Not applicable.
Testing
yarn testoryarn test -u(if you need to regenerate snapshots)yarn formatyarn lintPR Checklist
.cursor/rules)feat(service): add new Terraform service) - required for semantic-release