feat: [M3-10178] - Add Unsaved Changes modal for Legacy Alerts on Linode Details page
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
LinodeSettingsAlertsPaneltoAlertsPaneland moved it, along withAlertsSection, toLinodeDetail/LinodeAlerts/*to align with the right folder structure within theLinodeDetailflow feature - Updated legacy alerts header to use
Alertsinstead ofDefault Alertsbased on the latest UX mocks - Fixed random eslint issues
Target release date ποΈ
N/A
Preview π·
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.alertsfeature flag checked βοΈ - Navigate to the Linode Landing page, locate either
aclp-supported-region-linode-1oraclp-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
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.
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.
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 β
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 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.
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 π
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 Failing | 669 Passing | 4 Skipped | 129m 19s |
Details
| Failing Tests | ||
|---|---|---|
| Spec | Test | |
| :x: | object-storage-objects-multicluster.spec.ts | Cloud 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"
merging - object-storage-objects-multicluster.spec.ts test failure is flaky and unrelated
@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!
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