tech-story: [M3-9052] - Migrate PlacementGroups to Tanstack router #11474

coliu-akamai · 2025-01-03T20:35:36Z

Description 📝

Adds tanstack routing to placement groups + associated cleanup

Changes 🔄

routing consolidations in routes/PlacementGroups
use existing patterns for orderby, pagination, and getting dialog data
fix tests
removed route for /placement-groups/$id/linodes, as this was an extra route that only showed up after closing a drawer while on a PG's details page
for the Linodes table in placement groups, had to remove ordering by status as this is not supported by the API (now using API for order by)

Preview 📷

Behavior should be relatively unchanged

How to test 🧪

Confirm navigation & routed drawers at /placement-groups
- Confirm pagination
- Confirm sorting
- confirm searching
Confirm navigation & drawers at /placement-groups/{id}
- confirm sorting
- confirm searching

Pagination testing:

for /placement-groups, I used MSW + crud operations (seeded ~130 pgs)
for /placement-groups/{id}, I'm having a few issues - see comment below

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 unit tests are passing
TypeScript compilation succeeded without errors
Code passes all linting rules

packages/manager/src/features/PlacementGroups/PlacementGroupsUnassignModal.tsx

...ures/PlacementGroups/PlacementGroupsDetail/PlacementGroupsLinodes/PlacementGroupsLinodes.tsx

packages/manager/.eslintrc.cjs

coliu-akamai · 2025-01-13T16:00:29Z

packages/manager/cypress/e2e/core/linodes/resize-linode.spec.ts

@@ -155,7 +155,7 @@ describe('resize linode', () => {
    });
  });

-  it.only('resizes a linode by decreasing size', () => {


whoops didn't catch this earlier in its respective pr 😅

coliu-akamai · 2025-01-13T16:02:42Z

packages/manager/src/features/PlacementGroups/PlacementGroupsAssignLinodesDrawer.test.tsx

          label: 'PG-1',
+          placement_group_type: 'anti_affinity:local',


this and a bunch of other test files may just be eslint fixes. Otherwise, they'll have changes to use renderWithThemeAndRouter/other tanstack changes

coliu-akamai · 2025-01-13T16:03:26Z

packages/manager/src/features/PlacementGroups/PlacementGroupsCreateDrawer.tsx

@@ -11,6 +11,7 @@ import { createPlacementGroupSchema } from '@linode/validation';
 import { useFormik } from 'formik';
 import { useSnackbar } from 'notistack';
 import * as React from 'react';
+// eslint-disable-next-line no-restricted-imports


since PlacementGroupsCreatedDrawer is used in LinodesCreate, we need to keep react-router-dom here (for now)

coliu-akamai · 2025-01-13T16:08:10Z

packages/manager/src/features/PlacementGroups/PlacementGroupsDeleteModal.tsx

@@ -105,28 +99,6 @@ export const PlacementGroupsDeleteModal = (props: Props) => {
    return null;
  }

-  if (!assignedLinodes) {


for here and PlacementGroupsUnassignModal, now that we fetch the values in the parent component via useDialogData, and pass isFetching here, we can remove this existing check - a loading state will get rendered if the data is fetching

coliu-akamai · 2025-01-13T16:10:04Z

packages/manager/src/features/PlacementGroups/PlacementGroupsDetail/PlacementGroupsDetail.tsx


  const {
    data: placementGroup,
    error: placementGroupError,
    isLoading,
  } = usePlacementGroupQuery(placementGroupId);
-  const { data: linodes, isFetching: isFetchingLinodes } = useAllLinodesQuery(


this and related logic was moved down to a child component, PlacementGroupsLinodes, to avoid excessive prop drilling

coliu-akamai · 2025-01-13T16:23:29Z

...PlacementGroups/PlacementGroupsDetail/PlacementGroupsLinodes/PlacementGroupsLinodesTable.tsx

-    <OrderBy data={linodes} order="asc" orderBy={orderLinodeKey}>
-      {({ data: orderedData, handleOrderChange, order, orderBy }) => (
-        <Paginate data={orderedData}>
-          {({


rather than OrderBy, we're now using useOrderV2 - see PlacementGroupsLinodes

paginate - tbd, running into some issues

coliu-akamai · 2025-01-13T16:24:02Z

...PlacementGroups/PlacementGroupsDetail/PlacementGroupsLinodes/PlacementGroupsLinodesTable.tsx

  const orderLinodeKey = 'label';
-  const orderStatusKey = 'status';


see Changes section in PR main comment - removed status option bc it's not supported by the API

coliu-akamai · 2025-01-13T16:28:11Z

packages/manager/src/routes/placementGroups/index.ts

+      action,
+    }),
+  },
+  path: 'linodes/$action',


since assigning a Linode and unassigning it use slightly different routes, updating to use params.action === action pattern wasn't as straightforward as placementGroupAction - I ended up making placementGroupsUnassignRoute be a child of this placementGroupLinodesActionBaseRoute route

coliu-akamai · 2025-01-13T19:59:51Z

...ures/PlacementGroups/PlacementGroupsDetail/PlacementGroupsLinodes/PlacementGroupsLinodes.tsx

+    ...(query && { label: { '+contains': query } }),
+  };
+
+  const { data: linodes, isFetching: isFetchingLinodes } = useAllLinodesQuery(


pagination for the linodes table: having trouble testing with mock testing - it looks like filtering isn't applied, so all linodes are being returned, not just the ones that match lines 72-73 above. (also, repeat linodes are being returned/the same query is firing several times?, so I need to do more investigation with this)

I do think that pagination with actual data should work, (if it gets to that point - for actual data, most PGs I see can only have up to 5, maybe 10 linodes, so I haven't yet seen the footer) bc the actual queries are returning filtered and paginated data. However, bc I'm having trouble confirming, I don't want to say this with 100% confidence...

If it's better, I can switch this back to the original code which uses the Paginate hoc, but would need to do more work to get routing working for pagination then

I think this is ok. Seems like an edge for mock data only so i am not worried about this.

...ures/PlacementGroups/PlacementGroupsDetail/PlacementGroupsLinodes/PlacementGroupsLinodes.tsx

bnussman-akamai · 2025-01-13T20:35:25Z

...ures/PlacementGroups/PlacementGroupsDetail/PlacementGroupsLinodes/PlacementGroupsLinodes.tsx

-  >();
+  const { isLinodeReadOnly, placementGroup, region } = props;
+  const navigate = useNavigate();
+  const params = useParams({ strict: false });


Out of curiosity, is there any realistic way we can avoid using strict: false so often? It feels wrong, but does the job

We can only avoid strict: false for routes that don't have mounted routed content. So for pages containing modals/drawers/tabs we need to have it - it is by design (and also because of the way our application was composed)

getting closer revert rest of createDrawer router changes maybe a bit closer?? fix landing state prevent query from firing when NaN trying to add routing for search and order by pg linodes table

…k Router

coliu-akamai · 2025-01-22T15:59:54Z

@abailly-akamai for the notice issue, this is what I'm seeing - I just rebased with develop which has this pr that fixed a lot of notices 🚀

Looking into the extra reload thing - wondering if isFetching is suddenly changing

update - changing deleteModal to use isLoading instead + each unassign Linode button has its own loading state

linode-gh-bot · 2025-01-22T20:55:53Z

Cloud Manager UI test results

🔺 1 failing test on test run #36 ↗︎

❌ Failing	✅ Passing	↪️ Skipped	🕐 Duration
`1 Failing`	`476 Passing`	`2 Skipped`	`108m 0s`

Details

Failing Tests
	Spec	Test
❌	`resize-linode.spec.ts`	resize linode » resizes a linode by increasing size: warm migration

Troubleshooting

Use this command to re-run the failing tests:

yarn cy:run -s "cypress/e2e/core/linodes/resize-linode.spec.ts"

coliu-akamai added Work in Progress Routing Refactor labels Jan 3, 2025

coliu-akamai self-assigned this Jan 3, 2025

coliu-akamai changed the title ~~tech-story: [M3-9052] - DRAFT! Migrate PlacementGroups to Tanstack router~~ tech-story: [M3-9052] - DRAFT Migrate PlacementGroups to Tanstack router Jan 3, 2025

coliu-akamai force-pushed the m3-9052 branch 4 times, most recently from d4590c6 to 18984b0 Compare January 7, 2025 15:10

linode deleted a comment from linode-gh-bot Jan 7, 2025

coliu-akamai force-pushed the m3-9052 branch 3 times, most recently from 0797511 to 610ed4b Compare January 10, 2025 15:18

abailly-akamai reviewed Jan 10, 2025

View reviewed changes

packages/manager/src/features/PlacementGroups/PlacementGroupsUnassignModal.tsx Outdated Show resolved Hide resolved

...ures/PlacementGroups/PlacementGroupsDetail/PlacementGroupsLinodes/PlacementGroupsLinodes.tsx Outdated Show resolved Hide resolved

coliu-akamai changed the title ~~tech-story: [M3-9052] - DRAFT Migrate PlacementGroups to Tanstack router~~ tech-story: [M3-9052] - Migrate PlacementGroups to Tanstack router Jan 10, 2025

coliu-akamai removed the Work in Progress label Jan 10, 2025

coliu-akamai commented Jan 13, 2025

View reviewed changes

coliu-akamai added the Work in Progress label Jan 13, 2025

coliu-akamai commented Jan 13, 2025

View reviewed changes

coliu-akamai removed the Work in Progress label Jan 13, 2025

coliu-akamai marked this pull request as ready for review January 13, 2025 20:02

coliu-akamai requested review from a team as code owners January 13, 2025 20:02

coliu-akamai requested review from AzureLatte, bnussman-akamai and carrillo-erik and removed request for a team January 13, 2025 20:02

coliu-akamai added the Ready for Review label Jan 13, 2025

bnussman-akamai reviewed Jan 13, 2025

View reviewed changes

coliu-akamai force-pushed the m3-9052 branch from 3ebaed1 to 8210c8e Compare January 14, 2025 17:46

coliu-akamai and others added 16 commits January 22, 2025 10:39

step 5 (after this gets a bit finnicky i think)

f65aa95

step 6 initial changes - 23f, 29p

241757b

steps 6-7 ish

78d6fb9

switch PG create back to react router dom (used in LinodeCreate as well)

2173cc5

getting closer revert rest of createDrawer router changes maybe a bit closer?? fix landing state prevent query from firing when NaN trying to add routing for search and order by pg linodes table

drawer/modal for pg landing

44383ca

feeling a lot better about this updated order by pg linodes table

d1861de

pg linodes table - pagination and dialog data

59f10d1

update failing e2e

d8ab6d2

more unit tests passing than ever before :)

d42ae55

testing pls send help

f79e0b2

test cleanup

75e054c

some cleanup pt1 + address initial feedback

a5dc853

Added changeset: Refactor routing for Placement Groups to use Tanstac…

6503788

…k Router

fix e2e delete test

a899f07

infer search types @bnussman-akamai

7056f88

fix duplicate import missed during rebase

712f2b4

coliu-akamai force-pushed the m3-9052 branch from fadd822 to 712f2b4 Compare January 22, 2025 15:40

abailly-akamai and others added 2 commits January 22, 2025 14:45

fix issue with delete modal rendering

2b8495f

remove extra loading after unassigning a linode

e0fca27

coliu-akamai added Add'tl Approval Needed Waiting on another approval! and removed Ready for Review labels Jan 22, 2025

bnussman-akamai approved these changes Jan 23, 2025

View reviewed changes

bnussman-akamai added Approved Multiple approvals and ready to merge! and removed Add'tl Approval Needed Waiting on another approval! labels Jan 23, 2025

coliu-akamai merged commit 20b4b73 into linode:develop Jan 23, 2025
22 of 23 checks passed

coliu-akamai deleted the m3-9052 branch January 23, 2025 13:28

hkhalil-akamai mentioned this pull request Feb 5, 2025

Release v1.136.0 - release → staging #11623

Merged

dwiley-akamai mentioned this pull request Feb 11, 2025

Release v1.136.0 - staging → master #11645

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tech-story: [M3-9052] - Migrate PlacementGroups to Tanstack router #11474

tech-story: [M3-9052] - Migrate PlacementGroups to Tanstack router #11474

coliu-akamai commented Jan 3, 2025 •

edited

Loading

coliu-akamai Jan 13, 2025

coliu-akamai Jan 13, 2025

coliu-akamai Jan 13, 2025

coliu-akamai Jan 13, 2025

abailly-akamai Jan 21, 2025

coliu-akamai Jan 13, 2025

coliu-akamai Jan 13, 2025 •

edited

Loading

coliu-akamai Jan 13, 2025

coliu-akamai Jan 13, 2025 •

edited

Loading

coliu-akamai Jan 13, 2025 •

edited

Loading

abailly-akamai Jan 21, 2025

bnussman-akamai Jan 13, 2025

abailly-akamai Jan 13, 2025 •

edited

Loading

coliu-akamai commented Jan 22, 2025 •

edited

Loading

linode-gh-bot commented Jan 22, 2025

		const orderLinodeKey = 'label';
		const orderStatusKey = 'status';

tech-story: [M3-9052] - Migrate PlacementGroups to Tanstack router #11474

tech-story: [M3-9052] - Migrate PlacementGroups to Tanstack router #11474

Conversation

coliu-akamai commented Jan 3, 2025 • edited Loading

Description 📝

Changes 🔄

Preview 📷

How to test 🧪

As an Author, to speed up the review process, I considered 🤔

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coliu-akamai Jan 13, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coliu-akamai Jan 13, 2025 • edited Loading

Choose a reason for hiding this comment

coliu-akamai Jan 13, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abailly-akamai Jan 13, 2025 • edited Loading

Choose a reason for hiding this comment

coliu-akamai commented Jan 22, 2025 • edited Loading

linode-gh-bot commented Jan 22, 2025

Cloud Manager UI test results

🔺 1 failing test on test run #36 ↗︎

Details

Troubleshooting

coliu-akamai commented Jan 3, 2025 •

edited

Loading

coliu-akamai Jan 13, 2025 •

edited

Loading

coliu-akamai Jan 13, 2025 •

edited

Loading

coliu-akamai Jan 13, 2025 •

edited

Loading

abailly-akamai Jan 13, 2025 •

edited

Loading

coliu-akamai commented Jan 22, 2025 •

edited

Loading