manager icon indicating copy to clipboard operation
manager copied to clipboard

feat: [M3-10178] - Add Unsaved Changes modal for Legacy Alerts on Linode Details page

Open pmakode-akamai opened this issue 7 months ago β€’ 1 comments

Description πŸ“

Add "Unsaved Changes" Modal for Legacy Alerts on Linode Details Page and a bit of refactoring ♻️

As part of the ACLP integration, we need to add an "Unsaved Changes" modal to the Legacy Alerts section on the Linode Details page. This should show up if a user tries to switch tabs or navigate away without saving their changes, to help prevent accidental data loss and improve the overall user experience.

[!Note] This is a global change and is not controlled by a feature flag.

Changes πŸ”„

  • Added "Unsaved Changes" modal for Legacy Alerts on Linode Details Page
  • Renamed LinodeSettingsAlertsPanel to AlertsPanel and moved it, along with AlertsSection, to LinodeDetail/LinodeAlerts/* to align with the right folder structure within the LinodeDetail flow feature
  • Updated legacy alerts header to use Alerts instead of Default Alerts based on the latest UX mocks
  • Fixed random eslint issues

Target release date πŸ—“οΈ

N/A

Preview πŸ“·

Screenshot 2025-06-16 at 7 40 05β€―PM

How to test πŸ§ͺ

Verification steps

Normal Flow:

  • Navigate to the Linode Details page -> Alerts tab
  • Confirm that "Alerts" appears as the header for the Alerts section
  • Make changes and attempt to switch tabs or navigate away --> verify that the "Unsaved changes" modal is displayed
  • Ensure all default alert settings are accessible and continue to function as they did before

Aclp Integration Flow:

  • Prerequisite: Enable MSW and set the aclpBetaServices.alerts feature flag checked β˜‘οΈ
  • Navigate to the Linode Landing page, locate either aclp-supported-region-linode-1 or aclp-supported-region-linode-2, and go to the Alerts tab in the Linode Details page
  • Make changes in Legacy Alerts and attempt to switch tabs or navigate away -- verify that the "Unsaved changes" modal is displayed
  • Confirm that "Alerts" is shown as the header for the legacy Alerts tab
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

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

As an Author, before moving this PR from Draft to Open, I confirmed βœ…

  • [x] All unit tests are passing
  • [x] TypeScript compilation succeeded without errors
  • [x] Code passes all linting rules

pmakode-akamai avatar Jun 16 '25 14:06 pmakode-akamai

Something weird was happening with Github approvals last week.

In any case, @pmakode-akamai - it looks like there are a couple of merge conflicts on this branch that will need to be resolved.

mjac0bs avatar Jun 23 '25 18:06 mjac0bs

I think I'm missing some context here because I'm wondering: why are we adding this functionality now to a legacy component?

@mjac0bs I suppose it's mainly to avoid an inconsistent user experience. Since we're introducing the unsaved changes modal for the beta, if we only add it in ACLP-supported regions, users in legacy (both supported and non-supported regions) won't see it. That would make the experience feel uneven. This modal is meant to be a general feature, so including it in legacy helps keep things consistent across regions and modes.

pmakode-akamai avatar Jun 30 '25 06:06 pmakode-akamai

After pulling the latest changes from develop, the <Prompt /> component (Unsaved Changes Modal) is no longer working as expected. This issue may be due to recent changes related to tanstack. Marking this PR as "Do Not Merge" for now while we investigate. cc: @mjac0bs @dwiley-akamai @abailly-akamai

Edit: Removed <Prompt /> component (seems to be deprecated now) and used tanstack useBlocker hook to handle Unsaved Changes modal βœ…

pmakode-akamai avatar Jun 30 '25 10:06 pmakode-akamai

Things aren't quite behaving the same as the firewall page, though, which does show the modal when the user navigates away from the page.

@mjac0bs Looks like the modal for the firewall page works when the user navigates away, not because of the useBlocker hook, but because Prompt is still being used (especially the || isModalOpen condition in the open prop added here). Although the features seem routed to Tanstack, I'm curious why side nav routes are being treated as non-Tanstack routes.

I think if the useBlocker hook is defined inside a component deeply nested within the linodesRouteTree, it might not be able to capture navigations away from that tree. When navigating out of the Linodes route subtree (e.g., to the Images route subtree), the blocker unmounts since it's tied to that subtree. If possible, we might need to move useBlocker higher in the component tree to keep it active during cross-tree transitions. Otherwise, we may have to stick with Prompt for now. I'll see if there's any other option though.

pmakode-akamai avatar Jul 01 '25 08:07 pmakode-akamai

@pmakode-akamai thanks for working on this.

We will need to remove the reliance on Prompt since it is a react-router-dom util that will be removed shortly, however it is ok to keep it until the last cleanup. useBlocker is indeed the right replacement as you found out, but does not work 100% yet - see explanation below πŸ‘‡

I'm curious why side nav routes are being treated as non-Tanstack routes

I believe it is because the main <Link /> is still the react-router-dom util, therefore despite all features using tanstack router we are still managing two history caches (Legacy Link is used everywhere still). We're close to retiring it which should resolve these quirks! I am aware it is not ideal, but this was the best strategy to progressively being able to reroute such a large application.

I believe you have it right in this PR, and you still need the Prompt until Link is coupled with Tanstack router.

abailly-akamai avatar Jul 01 '25 09:07 abailly-akamai

I believe it is because the main <Link /> is still the react-router-dom util, therefore despite all features using tanstack router we are still managing two history caches (Legacy Link is used everywhere still). We're close to retiring it which should resolve these quirks! I am aware it is not ideal, but this was the best strategy to progressively being able to reroute such a large application.

I believe you have it right in this PR, and you still need the Prompt until Link is coupled with Tanstack router.

Ah, got it -- that makes sense. Thanks for the context, @abailly-akamai! I'll keep the Prompt in place for now πŸ‘

pmakode-akamai avatar Jul 01 '25 11:07 pmakode-akamai

Cloud Manager UI test results

:small_red_triangle: 1 failing test on test run #13 β†—οΈŽ

:x: Failing:white_check_mark: Passing:arrow_right_hook: Skipped:clock1: Duration
1 Failing669 Passing4 Skipped129m 19s

Details

Failing Tests
SpecTest
:x:object-storage-objects-multicluster.spec.tsCloud Manager Cypress Tests→Object Storage Multicluster objects » Object Storage Multicluster objects

Troubleshooting

Use this command to re-run the failing tests:

pnpm cy:run -s "cypress/e2e/core/objectStorageMulticluster/object-storage-objects-multicluster.spec.ts"

linode-gh-bot avatar Jul 02 '25 07:07 linode-gh-bot

merging - object-storage-objects-multicluster.spec.ts test failure is flaky and unrelated

pmakode-akamai avatar Jul 02 '25 07:07 pmakode-akamai

@pmakode-akamai out of curiosity, is there enough e2e coverage for this once we finalize the routing migration and remove the <Prompt />? If not I would strongly encourage to add this to existing cypress tests. thanks!

abailly-akamai avatar Jul 02 '25 08:07 abailly-akamai

is there enough e2e coverage for this once we finalize the routing migration and remove the <Prompt />? If not I would strongly encourage to add this to existing cypress tests. thanks!

@abailly-akamai I don't think we currently have e2e coverage for this modal or for the other instances of the Unsaved Changes modal in CM. The recent changes related to Tanstack also seem to have impacted the Upload Image feature, specifically the Unsaved Changes modal's <Prompt />, which isn't working as expected (since useBlocker isn't being used there). This went undetected. I agree that we should add e2e coverage for the modal related to this PR. I'll create a coverage ticket for this, along with any related tickets if necessary.

edit: created related tickets: M3-10275, M3-10276, M3-10277

pmakode-akamai avatar Jul 02 '25 08:07 pmakode-akamai