feat: (Codecov) add updateRepository function to update local storage and url query params for repo selection

[adrian-codecov]

This PR adds the ability to store repository data to the URL query parameters as well as local storage and reflux.

For the Codecov to Sentry project, we're tasked with adding a series of components, some of which would benefit from saving fields into local storage and url params. Those selectors are seen in this image below, from left to right: Codecov organization (different from what Sentry calls an organization), a repository, a branch and a time range - this PR focuses on the adding repository functionality exclusively.

Main Features

• Add updateRepository function in actionCreators, which calls the updateRepository reflux store function and updates + persists the repository in local storage and the url.
• Add updateRepository function in pageFiltersStore, which mainly updates the state w/ the repository selection (see notes on absence of desync).
• Adjust logic in several places, such as parse.tsx, persistence.tsx, pageFilters.tsx, to also evaluate for a repository.
• Add repository in several types as needed. Some types accept the repository field to be undefined as otherwise I'd affect other non-related Codecov services would have to declare the repository when updating environments, projects or dates. I can adjust this if needed.
• Adds/adjusts tests to include repository key when appropriate.

Update

• After the first pass, I had to adjust a ton of tests that relied on the PageFilter store state, as my changes involves adding a new variable to the state. To make this less painful for next time, I moved all the tests signalled by TS to use a PageFilter fixture. Hence there's a big number of tests changed, but hopefully next time we add a variable to the filter, we only need to change the fixture instead.

Notes

• Any feedback is welcome, specially since I'm touching a part of the code that will affect non-Codecov functionality with the new repository key being part of localstorage data.
• I'll have another PR with a UI component soon that can help for manual testing for this deployment.
• There's some things I'm not doing as I'm unfamiliar with the implications but I'm happy to address those if necessary. Off the top of my head, these include:
  • Adding the desync logic in case the URL params and store data is different.
  • Pinned filters: my understanding is there was an old way of storing things and a new way (pinning), so I added repository to that list. Please correct me if that's mistaken.

Thanks!

[nicholas-codecov]

everything looks good to me

[ryan953]

My initial thinking is that: it seems good to re-use the url and localstorage stuff that powers the global pageFilters stuff.

However adding in optional params to the top-level code interface seems like it'll be annoying over time. You'll probably want a unique version of pageFilters that will capture and require org+repo+branch+time. So i'd really suggest taking a look at splitting things up and planning for something that has strong types, but can still share some methods under the hood.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

✅ All tests successful. No failed tests found.

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #89818      +/-   ##
==========================================
- Coverage   83.07%   83.07%   -0.01%     
==========================================
  Files       10261    10261              
  Lines      579556   579549       -7     
  Branches    22622    22622              
==========================================
- Hits       481491   481484       -7     
  Misses      97622    97622              
  Partials      443      443              

[adrian-codecov]

Thanks for the pointers and advice @ryan953 and @malwilley!

Proceeded to make separate but similar types for the Codecov filters, allowing to reuse most of the functionality and keep things separate. I did create some methods exclusive to Codecov where relevant, as well as rename some types like SentryPageFilterUpdate to then create an overarching type like type PageFiltersUpdate = SentryPageFilterUpdate & CodecovPageFiltersUpdate - let me know if this is okay.

No sentry hook or file was affected, which is good w/ the type separation, just had to adjust a bunch of tests as the PageFilters' state will inevitably change with the addition of the repository key. As suggested per @malwilley, adjusted all of those tests to use a page filter fixture so next time we make a change to these we only have to change the fixture rather than the whole test.

Please let me know how it looks!

[malwilley]

It feels wrong to me that we separate codecov and sentry selections in these functions, yet the underlying state is a combination of the two. But the only other option I can think of is creating a completely separate store to track the codecov selection which is probably prohibitively difficult.

Not requesting changes based on this, just noting my thoughts about it. Tagging the frontend github team to see if anyone else has good ideas.

[adrian-codecov]

Yeah it's not the most elegant solution but it's the one that caused less friction.

I tried adding an extra param to the existing initialize function, but that affected a bunch of other tests - it's an option but I'm not the biggest fan. Luckily this function is only used once, so it'd only affect one code file there.

Alternatively, I had fused the newSelection param to expect type SentryFilters + CodecovFilters, and inside the initialize fn create a helper that split that object into 'sentrySelection' and 'codecovSelection', but I wasn't too convinced with this one either.

Open to ideas!

[JonasBa]

I agree here with @malwilley, this feels wrong.

I tried adding an extra param to the existing initialize function, but that affected a bunch of other tests - it's an option but I'm not the biggest fan. Luckily this function is only used once, so it'd only affect one code file there.

Understood, but that is somewhat expected, as you are touching some code that is used very often at Sentry. Updating a ton of tests isn't great, and I get that it's tedious, but it's a lot easier than maintaining two different implementations under the hood.

[adrianviquez]

Thanks for the pointers @malwilley and @scttcper, ready for another pass!

(I changed my GH account to my personal one so you'll see my recent commits are by a different user)

[JonasBa]

This feels like a bad move for the codebase for numerous reasons. I get that it's easy to add yet another flow, but this isn't the right solution. If Codecov is being merged into Sentry, we shouldn't need a separate code flow at the page filter level.

I would like to understand the issues of the considered alternatives (documenting them would go a long way here). You mentioned adding a query param breaks a lot of tests, which is somewhat expected, and while annoying, isn't particularly hard to fix. Miantaining an entirely separate flow and implementation for Codecov is something that is going to linger in the codebase forever, and I'd much rather we find a good implementation from the start and really integrate codecov into sentry as a first class product.

I am willing to help and dedicate some time to getting this properly integrated into our current pagefilters if that helps.

[JonasBa]

Why do we need a distinction of Codecov vs Sentry? If we are merging Codecov to Sentry, then this should not exist, and it should just be Sentry going forward.

I'm not a fan of this approach, as I'm fairly sure it will linger around until someone ultimately needs to address it (at which point, we'll have a lot of the code relying on this and it'll be a massive PITA to update)

[JonasBa]

I agree here with @malwilley, this feels wrong.

I tried adding an extra param to the existing initialize function, but that affected a bunch of other tests - it's an option but I'm not the biggest fan. Luckily this function is only used once, so it'd only affect one code file there.

Understood, but that is somewhat expected, as you are touching some code that is used very often at Sentry. Updating a ton of tests isn't great, and I get that it's tedious, but it's a lot easier than maintaining two different implementations under the hood.

[adrianviquez]

closing in favor of #90693