Skip to content

Adrian/add storage solution #90693

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

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from
Draft

Conversation

adrianviquez
Copy link
Contributor

@adrianviquez adrianviquez commented Apr 30, 2025

This PR adds a context to the Codecov product to hold state and functionality to save/get data to/from the url and local storage. This context is used in the CodecovProvider that will be the main wrapper for all things Codecov. This is the preferred approach over this pr.

Notes

  • When I name things "Codecov" here, I'm referring it as a product and not as the company. So thinking of it as a "feature" or "product" such as "replay" or "issues". It might be unnecessary for some cases (like the persistence/url), but I think it makes sense for the provider/context pieces. Open to thoughts here!
  • CodecovProvider: this is the main code that will act as a wrapper for everything in the Codecov product. It instantiates and initializes the CodecovContext component, as well as sets functions to manipulate the context.
  • CodecovContext: the context itself. Has some helper methods, one of note being initializeCodecovContext. This will initialize the context with data from the url or local storage.
  • Persistence.tsx and url.tsx these have helper methods to interact with url and local storage. More specifics in the files below. I'm currently missing a function to set data into the URL. I'm also open to changing the "Codecov"-ness of these methods as they shouldn't necessarily be Codecov specific.

Thanks!

@github-actions github-actions bot added the Scope: Frontend Automatically applied to PRs that change frontend components label Apr 30, 2025
* 2) Attempts to parse the data.
* 3) Validates and returns the data.
*/
export function getCodecovDataFromLocalStorage(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

You'll see both methods here are get/setCodecovData... . As stated in the description, I'm thinking of Codecov here more as a product, so the idea with these helper methods is to help choose the correct key and decode the data that's needed according to what this method expects.

Thinking it through though, I think we could make it more generic and be something like this

export function getDataFromLocalStorage<T>(
  localStorageKey: string,
): T | null {
  const value = localStorage.getItem(localStorageKey);

  if (!value) {
    return null;
  }

  let decoded: T;

  try {
    decoded = JSON.parse(value);
  } catch (err) {
    captureException(err);
    return null;
  }

  return {...decoded}
}

And elevate the key to the caller. And do something similar for the setter. Wdyt?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I actually went back and did this, so new fn follows this. Also did that for setting data to local storage and query data from the url

return maybe;
}

export function getCodecovParamsFromQuery(query: Location['query']) {
Copy link
Contributor Author

@adrianviquez adrianviquez Apr 30, 2025

Choose a reason for hiding this comment

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

Similar to the comment above, there's probably a way to generically request fields from the query param and validate those, having a harder time thinking about that one as each field probably has a different validation based on it's data type, but I suppose we could do some validation based on the type you're expecting, so making a mapping that says something like "repository": string, "branches": string[], "date": datetime, and have methods that do like getString, getArray, etc.

Edit: addressed this here

@adrianviquez adrianviquez requested a review from JonasBa April 30, 2025 20:09
const CODECOV_PRODUCT = 'codecov';

// Types
type CodecovObjectInLocalStorage = {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Currently redundant; could extend from the type in the Context component if this eventually grows or simply reuse it. Leaning towards reusing the original instead of extending.

export function getCodecovDataFromLocalStorage(
orgSlug: string
): CodecovObjectInLocalStorage | null {
// 1)
Copy link
Contributor Author

@adrianviquez adrianviquez Apr 30, 2025

Choose a reason for hiding this comment

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

Trying this approach to comment functionality, not sure how I feel but wanted opinions.

Edit: Got rid of it, already talked myself out of it 😂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Scope: Frontend Automatically applied to PRs that change frontend components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant