Skip to content
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

feat(insights): ensure all links within domain views, stay within domain views #77804

Closed

Conversation

DominikB2014
Copy link
Contributor

@DominikB2014 DominikB2014 commented Sep 19, 2024

Work for #77572

This ensures any links within a module that is within a domain view, stays within that domain view.
(this means you can now go to the http domain summary, resource summary, etc)

This is only necessary to support existing insights, and domain view work, at the same time. Something to keep in mind, this is all likely to change at somepoint when we remove the flag and do the release, i was trying to be "non-intrusive" as possible with the existing stuff so it works well in two places

This was quite tricky to do, so to make it support both at the same time, i'm using the pathname rather then query params in order to keep track of what page to render. This is because all modules use subpaths to get to their respective summary pages. This all handled in a new component called ModuleRenderer which looks at the current url to render the appropriate module page/summary page.

There are other approaches to this problem,

  1. Create a wrapper component for each module, the wrapper would have all the domain view controls (overview, web vitals, http, assets). The issue I found with this approach, is that it seemed like a lot of duplication. And we'll to handle the case where a module fits under many domain views.

@github-actions github-actions bot added the Scope: Frontend Automatically applied to PRs that change frontend components label Sep 19, 2024
Copy link

codecov bot commented Sep 19, 2024

Codecov Report

Attention: Patch coverage is 51.28205% with 19 lines in your changes missing coverage. Please review.

✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
static/app/views/insights/pages/moduleRenderer.tsx 15.38% 11 Missing ⚠️
...ts/browser/resources/views/resourceSummaryPage.tsx 0.00% 5 Missing ⚠️
static/app/views/insights/pages/useFilters.tsx 78.57% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master   #77804      +/-   ##
==========================================
- Coverage   78.09%   78.08%   -0.01%     
==========================================
  Files        6979     6983       +4     
  Lines      309716   309826     +110     
  Branches    50696    50714      +18     
==========================================
+ Hits       241862   241919      +57     
- Misses      61377    61430      +53     
  Partials     6477     6477              

[ModuleName.OTHER]: () => null,
};

const ModuleSubpages: Partial<Record<ModuleName, React.ComponentType>> = {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Todo, fill this in

@0Calories
Copy link
Member

0Calories commented Sep 23, 2024

This is fine if it's just a temporary change and to help you debug, but I'm not quite following the rationale for having a structure like this. It's understandably more tedious, but in my opinion a much cleaner approach, if we were to stick to the pattern of specifying individual routes.

The approach you have here to automatically determine the URL based on the module is smart, but it's a pattern that isn't followed anywhere else in our codebase and has the potential to be difficult to build upon and restricts us if we need to refactor in the future.

For the sake of organization, we should prioritize separation of concerns rather than minimizing some code duplication. Here's what I suggest:

  • Make separate PRs for each new domain landing page
  • Use a query param to determine which tab is selected, aka the module to display in the current domain
  • Define separate routes for the subpages, like span summary pages for the individual domains
  • You can still greatly reduce code duplication by using a pattern like Composition & Specialization. You can use Composition to define the general structure of the domain landing pages and then Specialization to implement the specific structures for each landing page

Following a design pattern for structuring these pages will greatly improve code readability, discovery, and will be easier to build upon or refactor in the future if needed

@DominikB2014
Copy link
Contributor Author

Closing in favour of another approach

@github-actions github-actions bot locked and limited conversation to collaborators Oct 9, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Scope: Frontend Automatically applied to PRs that change frontend components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants