fix: [DI-29172] - Dependent API error handling in Edit Alert#13379
fix: [DI-29172] - Dependent API error handling in Edit Alert#13379santoshp210-akamai wants to merge 11 commits intolinode:developfrom
Conversation
santoshp210-akamai
requested review from
pmakode-akamai and
venkymano-akamai
pmakode-akamai
left a comment
pmakode-akamai
left a comment
There was a problem hiding this comment.
I see that a Notice appears when blocking related API requests and the Submit button becomes disabled in that case.
However, shouldn't the Submit button be enabled only when there are no dependent API errors and the form has unsaved changes?
There was a problem hiding this comment.
Pull request overview
Implements form-level handling for dependent API failures in CloudPulse Edit Alert, surfacing a warning and preventing submission when required data cannot be loaded.
Changes:
- Adds a
hasAPIErrorform field and wires it into dependent-call components to signal API failures. - Displays a warning
Noticeand disables submit whenhasAPIErroris true. - Adjusts resources/regions UI loading/error behavior and updates related tests/changeset.
Reviewed changes
Copilot reviewed 25 out of 25 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/manager/src/features/CloudPulse/Alerts/EditAlert/EditAlertDefinition.tsx | Watches hasAPIError, shows warning notice, disables submit on API error. |
| packages/manager/src/features/CloudPulse/Alerts/CreateAlert/utilities.ts | Omits hasAPIError from payload builds (create/edit). |
| packages/manager/src/features/CloudPulse/Alerts/CreateAlert/types.ts | Adds hasAPIError to form type. |
| packages/manager/src/features/CloudPulse/Alerts/CreateAlert/schemas.ts | Adds schema support for hasAPIError. |
| packages/manager/src/features/CloudPulse/Alerts/CreateAlert/Resources/CloudPulseModifyAlertResources.tsx | Passes an error callback down to resource selector. |
| packages/manager/src/features/CloudPulse/Alerts/CreateAlert/Regions/CloudPulseModifyAlertRegions.tsx | Passes an error callback down to regions selector. |
| packages/manager/src/features/CloudPulse/Alerts/CreateAlert/NotificationChannels/AddChannelListing.tsx | Sets hasAPIError based on notification channels query error. |
| packages/manager/src/features/CloudPulse/Alerts/CreateAlert/Criteria/MetricCriteria.tsx | Sets hasAPIError based on metric definition query error. |
| packages/manager/src/features/CloudPulse/Alerts/CreateAlert/Criteria/DimensionFilterValue/useCleanupStaleValues.ts | Skips stale-value cleanup when dependent API is in error state. |
| packages/manager/src/features/CloudPulse/Alerts/CreateAlert/Criteria/DimensionFilterValue/constants.ts | Adds handleError callback to dimension filter autocomplete props. |
| packages/manager/src/features/CloudPulse/Alerts/CreateAlert/Criteria/DimensionFilterValue/ValueFieldRenderer.tsx | Threads handleError into renderer pipeline. |
| packages/manager/src/features/CloudPulse/Alerts/CreateAlert/Criteria/DimensionFilterValue/ValueFieldRenderer.test.tsx | Updates test props to include handleError. |
| packages/manager/src/features/CloudPulse/Alerts/CreateAlert/Criteria/DimensionFilterValue/ObjectStorageDimensionFilterAutocomplete.tsx | Detects dependent API errors and reports via handleError. |
| packages/manager/src/features/CloudPulse/Alerts/CreateAlert/Criteria/DimensionFilterValue/ObjectStorageDimensionFilterAutocomplete.test.tsx | Updates test props to include handleError. |
| packages/manager/src/features/CloudPulse/Alerts/CreateAlert/Criteria/DimensionFilterValue/FirewallDimensionFilterAutocomplete.tsx | Detects dependent API errors and reports via handleError. |
| packages/manager/src/features/CloudPulse/Alerts/CreateAlert/Criteria/DimensionFilterValue/FirewallDimensionFilterAutocomplete.test.tsx | Updates test props to include handleError. |
| packages/manager/src/features/CloudPulse/Alerts/CreateAlert/Criteria/DimensionFilterValue/DimensionFilterAutocomplete.test.tsx | Updates test props to include handleError. |
| packages/manager/src/features/CloudPulse/Alerts/CreateAlert/Criteria/DimensionFilterValue/BlockStorageDimensionFilterAutocomplete.tsx | Detects dependent API errors, reports via handleError, and prevents stale cleanup on error. |
| packages/manager/src/features/CloudPulse/Alerts/CreateAlert/Criteria/DimensionFilterValue/BlockStorageDimensionFilterAutocomplete.test.tsx | Updates test props to include handleError. |
| packages/manager/src/features/CloudPulse/Alerts/CreateAlert/Criteria/DimensionFilterField.tsx | Captures child API errors and sets form-level hasAPIError. |
| packages/manager/src/features/CloudPulse/Alerts/AlertsResources/DisplayAlertResources.tsx | Avoids showing empty state when table data has loading error. |
| packages/manager/src/features/CloudPulse/Alerts/AlertsResources/AlertsResources.tsx | Reports resource/region API errors upward and changes loading UI to hide content via display: none. |
| packages/manager/src/features/CloudPulse/Alerts/AlertsResources/AlertsResources.test.tsx | Updates loading-state visibility assertions. |
| packages/manager/src/features/CloudPulse/Alerts/AlertRegions/AlertRegions.tsx | Reports region/resource API errors upward via setError. |
| packages/manager/.changeset/pr-13379-fixed-1770618987460.md | Adds changeset entry describing the fix. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| React.useEffect(() => { | ||
| setValue('hasAPIError', notificationChannelsError); | ||
| }, [setValue, notificationChannelsError]); |
There was a problem hiding this comment.
hasAPIError is being overwritten by a single dependency’s error state. If multiple dependent APIs can error, a later effect that sets false will clear the global flag even while other sections are still failing. Consider tracking per-section error flags (e.g., apiErrors.notificationChannels, apiErrors.metricDefinitions, etc.) and deriving hasAPIError = Object.values(apiErrors).some(Boolean), or otherwise aggregating errors so the form only clears the global flag when all dependent APIs are healthy.
|
|
||
| export const alertDefinitionFormSchema = createAlertDefinitionSchema.concat( | ||
| object({ | ||
| hasAPIError: mixed<boolean>().optional(), |
There was a problem hiding this comment.
mixed<boolean>() doesn’t strictly validate boolean inputs in the way boolean() does (it can allow coercions/values you may not intend). Prefer using boolean().optional() (or mixed().oneOf([true, false]).optional() if you explicitly want that behavior) to keep the schema consistent and stricter.
| React.useEffect(() => { | ||
| const hasError = isResourcesError || isRegionsError; | ||
| if (setError) { | ||
| setError(hasError); | ||
| } | ||
| }, [setError, isResourcesError, isRegionsError]); |
There was a problem hiding this comment.
This callback reports only this component’s error state upward. If multiple children call setError/handleError independently, the last writer wins and can incorrectly clear the parent’s global error flag while another dependent API is still failing. Update the parent/consumer contract to support aggregation (e.g., setError(sourceKey, hasError) or setApiErrors((prev) => ({...prev, [sourceKey]: hasError}))), then compute the final hasAPIError from the aggregate.
| /** | ||
| * Callback triggered when a dependent API has an error. | ||
| */ | ||
| handleError?: (hasError: boolean) => void; |
There was a problem hiding this comment.
The error callback is named inconsistently across the diff (handleError in dimension filter components vs setError in regions/resources). Consider standardizing on a single convention (e.g., onApiErrorChange or onDependentApiErrorChange) to make intent and directionality consistent and reduce confusion for component consumers.
| handleError?: (hasError: boolean) => void; | |
| onDependentApiErrorChange?: (hasError: boolean) => void; |
| React.useEffect(() => { | ||
| const hasError = isError || isRegionsError; | ||
| if (handleError) { | ||
| handleError(hasError); | ||
| } | ||
| }, [isError, isRegionsError, handleError]); |
There was a problem hiding this comment.
New behavior is introduced to notify the parent via handleError, but the updated tests only pass a mock function without asserting it’s called with the correct values. Add unit tests that mock useObjectStorageFetchOptions / useRegionsQuery into error and non-error states and assert handleError(true) and handleError(false) are emitted appropriately.
| expect(queryByText(searchPlaceholder)).not.toBeVisible(); | ||
| expect(queryByText(regionPlaceholder)).not.toBeVisible(); |
There was a problem hiding this comment.
toBeVisible() expects an HTMLElement; if queryByText(...) returns null, this assertion will error and make the test brittle. If the intent is to assert the element exists but is hidden, use getByText(...) with .not.toBeVisible(). If the intent is to assert it is not rendered, keep .not.toBeInTheDocument().
| expect(queryByText(searchPlaceholder)).not.toBeVisible(); | |
| expect(queryByText(regionPlaceholder)).not.toBeVisible(); | |
| expect(queryByText(searchPlaceholder)).not.toBeInTheDocument(); | |
| expect(queryByText(regionPlaceholder)).not.toBeInTheDocument(); |
@pmakode-akamai , The overall enablement behavior (i.e., not tying it to unsaved changes) follows the original UX decision from the initial design discussions, so we’ve kept that consistent and only added the blocking condition for dependent API errors. If we think the button should also depend on unsaved changes, we can loop in the UX team and confirm the behaviour. |
Ideally, the behavior should also depend on unsaved changes, as that's the pattern we've been following across the app and aligns with common UX practices. Since we're already introducing disabled-state logic for the Submit button in this PR (based on API errors), it might be worth confirming whether unsaved changes should also be included in the enablement logic. We can loop in UX to clarify and address it here if possible, or handle it in a follow-up PR if needed. |
There was a problem hiding this comment.
-
I see the Notice when the related API requests are blocked - that looks good ✅
-
The enablement of the Submit button based on unsaved changes should also be reviewed, either in this PR or in a follow-up ❗️
- This is important, as keeping the Submit always enabled and disabling it only when a related API fails doesn't seem ideal. In its current state, a user could trigger a Submit action before the dependent APIs have finished loading or before errors are surfaced, which could lead to unintended or redundant requests.
-
Also, there are merge conflicts that need to be resolved ❗️
|
@pmakode-akamai , I have resolved the conflicts. For the behaviour of enabling the Submit on unsaved changes, we will review with UX and take it as part of a separate PR. So for now we can proceed with this PR |
pmakode-akamai
left a comment
pmakode-akamai
left a comment
There was a problem hiding this comment.
Approving this so that there are no blockers, but we still need to revisit the Submit button in the edit flow.
Apart from that, I think you still need to look into this comment: #13379 (comment)
@pmakode-akamai , We have changed the AlertResources component to fix one of our edge-cases of infinite refetching for regions API call, The #13379 (comment) comment now fails as the filters and text is present in the Document, so we have to resort to asserting the visibility |
Cloud Manager UI test results🔺 1 failing test on test run #11 ↗︎
Details
TroubleshootingUse this command to re-run the failing tests: pnpm cy:run -s "cypress/e2e/core/objectStorage/object-storage.e2e.spec.ts" |
|||||||||||||||||
4 participants
Description 📝
This PR implements error handling for dependent API failures in the CloudPulse Edit Alert feature
Changes 🔄
-Added hasAPIError boolean field to track API errors across the alert form
-Implemented error detection in components that make API calls (notification channels, metric criteria, dimension filters)
-Display warning notice and disable submit button when API errors occur
Scope 🚢
Upon production release, changes in this PR will be visible to:
Target release date 🗓️
Please specify a release date (and environment, if applicable) to guarantee timely review of this PR. If exact date is not known, please approximate and update it as needed.
Preview 📷
Include a screenshot
<img src="" />or video<video src="" />of the change.🔒 Use the Mask Sensitive Data setting for security.
💡 For changes requiring multiple steps to validate, prefer a video for clarity.
edit-form-before.mp4
edit-form-after.mp4
How to test 🧪
Prerequisites
(How to setup test environment)
Reproduction steps
(How to reproduce the issue, if applicable)
Verification steps
(How to verify changes)
Author Checklists
As an Author, to speed up the review process, I considered 🤔
👀 Doing a self review
❔ Our contribution guidelines
🤏 Splitting feature into small PRs
➕ Adding a changeset
🧪 Providing/improving test coverage
🔐 Removing all sensitive information from the code and PR description
🚩 Using a feature flag to protect the release
👣 Providing comprehensive reproduction steps
📑 Providing or updating our documentation
🕛 Scheduling a pair reviewing session
📱 Providing mobile support
♿ Providing accessibility support
As an Author, before moving this PR from Draft to Open, I confirmed ✅