tech-stories: [M3-10023] - The final boss: Linodes Reroute#12363
tech-stories: [M3-10023] - The final boss: Linodes Reroute#12363abailly-akamai merged 23 commits intolinode:developfrom
Conversation
ed7044f to
757ec00
Compare
| const regionQuery = `®ionID=${linode.region}`; | ||
| const typeQuery = linode.type ? `&typeID=${linode.type}` : ''; | ||
| return `/linodes/create?linodeID=${linode.id}${regionQuery}&type=Clone+Linode${typeQuery}`; | ||
| return `/linodes/create?linodeID=${linode.id}${regionQuery}&type=Clone%20Linode${typeQuery}`; |
There was a problem hiding this comment.
the new router is more strict about params encoding/decoding, which is good!
| }); | ||
|
|
||
| // Confirm can navigate to Linode after success | ||
| cy.visitWithLogin(`/linodes/${mockLinode.id}`); |
There was a problem hiding this comment.
I think something was amiss with this last step. Not sure how it passed before
There was a problem hiding this comment.
Opening up the upgrade interface dialog used to just append /upgrade-interfaces to the end of the route, so if you opened the dialog on /linodes/$linodeID, closing it would take you back there
with the new routing updates, I think correct behavior for this test would be to confirm the url ends with linodes/mockLinode.id/configurations if you don't click the navigate to network settings button
| subtype: params.subtype ?? undefined, | ||
| type: params.type ?? undefined, | ||
| }), | ||
| }); |
There was a problem hiding this comment.
this could be trimmed and optimized, but this touches so many files using this util i wanted to keep things scoped. I'll make a ticket to make this util more aligned with the new router's API
coliu-akamai
left a comment
There was a problem hiding this comment.
so awesome to see this pr up - thanks for all your hard work on this, Alban! 🎉
breaking up my review of this into multiple passes to make sure I look over stuff as carefully as possible. Currently reviewed:
✅ linode landing - order by, pagination and list/grid view (I've been using msw for pagination/order by - could you remind me if there's a shared account with a large # of Linodes we could use?)
✅ Linode create - tabs look good
✅ linode interfaces - upgrade dialog
left a comment regarding linode interfaces - detail drawer
| }); | ||
|
|
||
| // Confirm can navigate to Linode after success | ||
| cy.visitWithLogin(`/linodes/${mockLinode.id}`); |
There was a problem hiding this comment.
Opening up the upgrade interface dialog used to just append /upgrade-interfaces to the end of the route, so if you opened the dialog on /linodes/$linodeID, closing it would take you back there
with the new routing updates, I think correct behavior for this test would be to confirm the url ends with linodes/mockLinode.id/configurations if you don't click the navigate to network settings button
| @@ -0,0 +1,9 @@ | |||
| import { createLazyRoute } from '@tanstack/react-router'; | |||
There was a problem hiding this comment.
qq, are we no longer putting lazy routes in their own file in the routes package?
There was a problem hiding this comment.
good eyes! Correct, @bnussman-akamai found that it created issues with lazy loading. There will be follow up tickets to change this in other features
| navigate({ | ||
| to: '/linodes/$linodeId/configurations/upgrade-interfaces', | ||
| params: { linodeId }, | ||
| }); |
There was a problem hiding this comment.
thank you!! 🚀 🚀
(discovered while testing that this takes care of a bug when trying to open up the upgrade dialog in Linode Landing, summary view)
...er/src/features/Linodes/LinodesDetail/LinodeNetworking/LinodeInterfaces/LinodeInterfaces.tsx
Show resolved
Hide resolved
packages/manager/src/features/Linodes/LinodeCreate/utilities.ts
Outdated
Show resolved
Hide resolved
packages/manager/src/features/Linodes/LinodeCreate/Details/PlacementGroupPanel.test.tsx
Show resolved
Hide resolved
coliu-akamai
left a comment
There was a problem hiding this comment.
Thanks Alban! went through rest of Linode pages, things are looking good from what I've seen! (one small thing for /linodes/id/storage page below)
✅ /linodes/$linodeId
✅ /linodes/$linodeId/clone
✅ /linodes/$linodeId/clone/configs
✅ /linodes/$linodeId/clone/disks
✅ /linodes/$linodeId/analytics
✅ /linodes/$linodeId/networking
✅ /linodes/$linodeId/networking/interfaces
✅ /linodes/$linodeId/networking/interfaces/$interfaceId
✅ /linodes/$linodeId/storage (see below)
✅ /linodes/$linodeId/configurations
✅ /linodes/$linodeId/configurations/upgrade-interfaces
✅ /linodes/$linodeId/backup
✅ /linodes/$linodeId/activity
✅ /linodes/$linodeId/settings
✅ /linodes/$linodeId/alerts
✅ /linodes/$linodeId/metrics
✅ links to other entities like firewalls/vpcs/lke
✅ submit redirects
When on /linodes/id/storage, I noticed that sorting the Volumes table affects the disk table (and sorting the Disk table affects the arrows for the Volumes table) - see video
| prod | this branch |
|---|---|
prod-order-by-storage.mov |
this-branch-order-by-storage.mov |
Cloud Manager UI test results🎉 666 passing tests on test run #22 ↗︎
|

Description 📝
Rerouting the last feature: Linodes
As much as I tried to keep a reasonable scope & diff size for the routing refactor, it's hard to break things down and Linodes is a big feature.
This PR does not remove (yet) react router dom from teh codebase, just reroute this particular chunk. A LOT of the diff is also due to extra mocking on our unit tests.
Changes 🔄
Preview 📷
There should be no visual or functional regression as a result of this PR
How to test 🧪
Verification steps
Landing
Create Route
Detail Routes
👉 Test all paths and confirm
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 ✅