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

Performance: limit Sentry tracking, fix NFT hooks #5819

Merged
merged 12 commits into from
Jun 7, 2024

Conversation

christianbaroni
Copy link
Member

@christianbaroni christianbaroni commented Jun 5, 2024

Fixes APP-1555
Fixes APP-1566

What changed (plus any additional context for devs)

  • Limits Sentry tracking to crash reporting, startup time tracking, and explicit error reporting
    • Not fully clear why but Sentry is badly hurting app performance — limiting it to tracking crashes and startup times solves the problem
  • Fixes issues causing NFT data to be excessively recreated — some more improvements to be made but the big problems are resolved
    • Had to bump the nftsQueryKey persisterVersion because the structure of data (from the useQuery hook) is now different with the NFT data transformation moved into the useQuery.
    • The structure of the data returned by the useLegacyNFTs wrapper hook is unchanged and fully backwards compatible — ideally we later remove the wrapper hook and migrate to the new NFT types.
  • Removes non-critical ENS-related hooks that were making a significant number of network requests
  • Removes unused code in App.js and in the WalletScreen component

Also:

  • Isolates the dapps data from UI components in the browser, because the dapps object is quite heavy and it's unnecessary for it to cause re-renders in search

Screen recordings / screenshots

What to test

@jinchung jinchung added performance performance related improvements swaps v2 labels Jun 5, 2024
@@ -72,8 +72,7 @@ const Logger = {
console.log(...args); // eslint-disable-line no-console
}
if (args.length === 1 && typeof args[0] === 'string') {
// @ts-expect-error ts-migrate(2769) FIXME: No overload matches this call.
sentryUtils.addInfoBreadcrumb.apply(null, args);
sentryUtils.addInfoBreadcrumb.apply(null, [args[0]]);
Copy link
Contributor

Choose a reason for hiding this comment

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

nice

Copy link
Member

@brunobar79 brunobar79 left a comment

Choose a reason for hiding this comment

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

LGTM!

src/hooks/useCollectible.ts Show resolved Hide resolved
Copy link
Member

@jinchung jinchung left a comment

Choose a reason for hiding this comment

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

Comments inline

src/screens/WalletScreen/index.tsx Show resolved Hide resolved
src/screens/WalletScreen/index.tsx Show resolved Hide resolved
src/screens/WalletScreen/index.tsx Show resolved Hide resolved
src/resources/nfts/index.ts Outdated Show resolved Hide resolved
src/resources/nfts/index.ts Show resolved Hide resolved
src/resources/nfts/index.ts Show resolved Hide resolved
Copy link
Member

@jinchung jinchung left a comment

Choose a reason for hiding this comment

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

🌮

@christianbaroni christianbaroni merged commit 6642231 into develop Jun 7, 2024
6 checks passed
@christianbaroni christianbaroni deleted the @christian/performance branch June 7, 2024 15:32
BrodyHughes added a commit that referenced this pull request Jun 11, 2024
…ap-v2-e2e

* 'develop' of github.com:rainbow-me/rainbow:
  Fix weird issues with rounding  (#5822)
  Fix provider recursion bug, provider function types (#5829)
  bump braces (#5831)
  Add entry point for other networks (#5824)
  remove info icon on max transaction fee row as we don't have that in spec (#5827)
  [e2e] Add Malicious Dapp interaction test (#5764)
  reorg ens related calls (#5828)
  Performance: limit Sentry tracking, fix NFT hooks (#5819)
  fix wrong chainname (#5823)
  Disable flashbots toggle on appropriate chains (#5812)
  add type to SearchAsset type and add into type possibilities (#5820)
  fix fasterimage border radius android (#5816)
  more Safemath fixes (#5818)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance performance related improvements swaps v2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants