Skip to content

Conversation

@nikhagra-akamai
Copy link
Contributor

@nikhagra-akamai nikhagra-akamai commented Jul 22, 2025

Description 📝

Updated Alert Reusable component based on the latest mockup

https://www.figma.com/design/uvdOz9Xs0RVdMRJMCIgS9H/ACLP---Contextual-views?node-id=13721-11625&t=fXEAT02sleQcIKkQ-0

Changes 🔄

List any change(s) relevant to the reviewer.

  1. Hide manage alert button if it is used in create mode i.e. entityId is undefined
  2. Filters alerts with scope region based on the regionId passed
  3. Show confirmationDialog on save button click only if previously legacy alerts were present which is controlled by isLegacyAlertAvailable prop
  4. Updated prop description for onToggleAlert to be used in both create and edit mode to serve different use cases

💡 NOTE: Same handler can be used by the service owner to track unsaved alert toggles and subsequently show confirmation dialog box while switching with unsaved changes.

Target release date 🗓️

12th August

Preview 📷

Include a screenshot or screen recording of the change.

🔒 Use the Mask Sensitive Data setting for security.

💡 Use <video src="" /> tag when including recordings in table.

Before After
Screenshot 2025-07-22 at 5 11 19 PM Screenshot 2025-07-22 at 5 07 35 PM
Screenshot 2025-07-22 at 5 13 22 PM Screenshot 2025-07-22 at 5 13 49 PM

How to test 🧪

  1. Switch to mock user
  2. Add with required properties to see ACLP alerts
  3. Playaround with props to visualize the 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


  • I have read and considered all applicable items listed above.

As an Author, before moving this PR from Draft to Open, I confirmed ✅

  • All tests and CI checks are passing
  • TypeScript compilation succeeded without errors
  • Code passes all linting rules

@nikhagra-akamai nikhagra-akamai requested a review from a team as a code owner July 22, 2025 11:46
@nikhagra-akamai nikhagra-akamai requested review from cpathipa and harsh-akamai and removed request for a team July 22, 2025 11:46
<Box display="flex" justifyContent="space-between">
<Box alignItems="center" display="flex" gap={0.5}>
<Typography variant="h2">Alerts</Typography>
<BetaChip />
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion(non-blocking): Variable chip for different phases
As a separate effort, we may want to conditionally render either the beta or new feature chip based on LD flag. Maybe we can move/re-use this util throughout the app

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

great point, beta chip will vary based on the phase. Will update it

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to update the flag in LD to integrate this, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've updated it to show beta chip according to aclpBetaServices

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

non-blocking: I believe we want to swap the Beta chip with the New feature chip for the GA rollout. We may also need to update the LD flag structure for alerts (and possibly other features) to allow hiding the Beta chip even when the feature flag is enabled.

something like this:

{
  ...
  alerts: {
    enabled: boolean;
    beta: boolean;
    new: boolean;
  },
  ...
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is not finalized yet to show new feature chip. Will take it later. Currently it'll just toggle to show or hide beta chip

@jaalah-akamai jaalah-akamai added the Add'tl Approval Needed Waiting on another approval! label Jul 22, 2025
@nikhagra-akamai
Copy link
Contributor Author

@pmakode-akamai pls review & Provide second approval

…ra-akamai/manager into alert/contextual_view_enhancement
Copy link
Contributor

@pmakode-akamai pmakode-akamai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we pass isLegacyAlertAvailable={!isLinodeAclpSubscribed} to the AclpReusabledComponent here at /LinodesDetail/LinodeAlerts/LinodeAlerts.tsx#L45 in this PR itself, or should we handle it in a separate PR?

edit: Addressed in d33d48b

@pmakode-akamai pmakode-akamai added the ACLP Integration CI (Cloud Interfaces) Support for CC (Core Compute) CloudPulse Integration label Jul 23, 2025
Copy link
Contributor

@pmakode-akamai pmakode-akamai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue (blocking): Now that we have some beta alerts enabled by default, I noticed that in the create flow, service owners don’t have access to the default enabled beta alerts state until they toggle one of the alert options (for eg., updatedAlerts from handleToggleAlert), I'm wondering how we can ensure these alerts are enabled in the default state without requiring user interaction

Screen.Recording.2025-07-23.at.3.04.04.PM.mov

@nikhagra-akamai
Copy link
Contributor Author

issue (blocking): Now that we have some beta alerts enabled by default, I noticed that in the create flow, service owners don’t have access to the default enabled beta alerts state until they toggle one of the alert options (for eg., updatedAlerts from handleToggleAlert), I'm wondering how we can ensure these alerts are enabled in the default state without requiring user interaction

@pmakode-akamai I've updated the logic to handle initial state

Screen.Recording.2025-07-23.at.5.02.56.PM.mov

@linode-gh-bot
Copy link
Collaborator

Cloud Manager UI test results

🎉 690 passing tests on test run #8 ↗︎

❌ Failing✅ Passing↪️ Skipped🕐 Duration
0 Failing690 Passing4 Skipped124m 32s

Copy link
Contributor

@pmakode-akamai pmakode-akamai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good ✅

Thanks for making the changes and fixing the initial state logic!

@github-project-automation github-project-automation bot moved this from Review to Approved in Cloud Manager Jul 23, 2025
@pmakode-akamai pmakode-akamai added Approved Multiple approvals and ready to merge! and removed Add'tl Approval Needed Waiting on another approval! Ready for Review labels Jul 23, 2025
@nikhagra-akamai nikhagra-akamai merged commit ea95d5b into linode:develop Jul 23, 2025
35 checks passed
@github-project-automation github-project-automation bot moved this from Approved to Merged in Cloud Manager Jul 23, 2025
@nikhagra-akamai
Copy link
Contributor Author

Missed to add the changeset, hence added in this PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ACLP Integration CI (Cloud Interfaces) Support for CC (Core Compute) CloudPulse Integration Approved Multiple approvals and ready to merge! Cloud Pulse - Alerting Cloud Pulse

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

6 participants