Skip to content

refactor: [M3-9723] - Managed Tanstack routing #11994

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 20 commits into from
Apr 18, 2025

Conversation

abailly-akamai
Copy link
Contributor

@abailly-akamai abailly-akamai commented Apr 8, 2025

Description 📝

This PR migrates our routing tooling in the Managed feature to Tanstack router.
It also cleans up the code to adhere to our latest standards.

⚠️ The diff is a little large because of Managed being an older feature that has not been touched very much recently and presented challenges in the way the code was composed. To that effect I updated some components to be closer to our standards in order to cleanly implement the routing patterns.

Changes 🔄

  • Implements new routing tools
  • Cleans up old coding patterns (barrel files, exports, function composition, missing queries)
  • Update unit and e2e tests

Preview 📷

There should not be any visual or functional regression with the feature. Only difference is the introduction of loading patterns in some edit/delete drawer/modals

How to test 🧪

Verification steps

  • Navigate to /managed and for each tab, confirm:
    • no routing regression
    • no visual regression
    • all actions (edit, delete, details etc) opening drawers and modals
    • table sorting
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

@abailly-akamai abailly-akamai self-assigned this Apr 8, 2025
@abailly-akamai abailly-akamai force-pushed the M3-9723 branch 2 times, most recently from a5e3ca0 to a9cf99d Compare April 9, 2025 14:41
});

return groups;
};
Copy link
Contributor Author

Choose a reason for hiding this comment

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

just moved that util but it is unchanged

enabled,
refetchInterval: 20000,
});

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The queries added here were apparently never needed cause we were fetching ALL then filtering to get a single record. As with other routing refactors and our new standards, we now fetch an individual record when performing an action in a drawer, modal etc

@abailly-akamai abailly-akamai marked this pull request as ready for review April 10, 2025 14:12
@abailly-akamai abailly-akamai requested review from a team as code owners April 10, 2025 14:12
@abailly-akamai abailly-akamai requested review from jdamore-linode, mjac0bs and carrillo-erik and removed request for a team April 10, 2025 14:12
Copy link
Contributor

@jdamore-linode jdamore-linode left a comment

Choose a reason for hiding this comment

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

Nice! Thank you @abailly-akamai!

Copy link
Contributor

@mjac0bs mjac0bs left a comment

Choose a reason for hiding this comment

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

Thanks for the patience on this one and sorry it took me longer than I liked to get to. And as always, thanks for your work on this + your thoroughness in doing them.

Confirmed:

  • Routes are routing as expected for create, edit, and delete actions
    • Successful CRUD operations for SSH Access, Contacts, and Credentials
    • Didn't test Monitor CRUD operations since we don't have support for that in the MSW, but did confirm the routes update as expected when interacting with the actions
    • 🐛 Noted the undefined label in the header of the HistoryDrawer
  • Sorting and filtering tables works for all tables
  • Backwards and forwards browser navigation
  • Visual parity with prod

Comment on lines +22 to +37
<MaskableText isToggleable text={contact.name} />
</TableCell>
<Hidden mdDown>
<TableCell>
<MaskableText text={contact.group ?? ''} isToggleable />
<MaskableText isToggleable text={contact.group ?? ''} />
</TableCell>
</Hidden>
<TableCell>
<MaskableText text={contact.email} isToggleable />
<MaskableText isToggleable text={contact.email} />
</TableCell>
<Hidden xsDown>
<TableCell>
<MaskableText text={contact.phone.primary ?? ''} isToggleable />
<MaskableText isToggleable text={contact.phone.primary ?? ''} />
</TableCell>
<TableCell>
<MaskableText text={contact.phone.secondary ?? ''} isToggleable />
<MaskableText isToggleable text={contact.phone.secondary ?? ''} />
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the clean up. 🧹

Copy link
Contributor

Choose a reason for hiding this comment

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

Minor: some linter warnings in this file

@mjac0bs mjac0bs added the Approved Multiple approvals and ready to merge! label Apr 17, 2025
Copy link
Contributor

@carrillo-erik carrillo-erik left a comment

Choose a reason for hiding this comment

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

Very nice work. It took me longer than anticipated to approve because I taking the time to understand the changes. Verified all actions within modals and drawers. No regressions with routing.

@linode-gh-bot
Copy link
Collaborator

Cloud Manager UI test results

🎉 559 passing tests on test run #16 ↗︎

❌ Failing✅ Passing↪️ Skipped🕐 Duration
0 Failing559 Passing5 Skipped115m 4s

@abailly-akamai abailly-akamai merged commit 8751678 into linode:develop Apr 18, 2025
35 checks passed
rodonnel-akamai pushed a commit to rodonnel-akamai/manager that referenced this pull request Apr 23, 2025
* Initial commit: landing routing

* monitors

* remove barrel files

* save progress

* Wrap up monitors

* SSH Access

* Credentials

* Wrapping up UI

* fix units

* e2e fixes

* fix after rebase

* fix remainin e2e failure

* eslint fix

* eslint fix

* linting fixes

* Added changeset: Managed Tanstack routing

* feedback @mjac0bs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Approved Multiple approvals and ready to merge! Routing Refactor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants