-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
perf: Create custom spans for account overview tabs #28086
Changes from 29 commits
8ecf0b2
9924b50
a4c46b7
394d487
39cc6f2
656dfdb
9f90b84
04d0694
ccc6a34
e9b9aef
c53f8bb
b9fa09e
b79a95e
b5046fd
694e73d
c0f9811
21bbcf6
4fa73c1
a5c2ae2
6bbef8e
26b2031
f49728b
0d914ec
9c33083
cfcc492
df17187
f3c989b
3790fc4
2718cc4
7eae322
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,20 @@ | ||
import { TraceName } from '../lib/trace'; | ||
import { MetaMetricsEventName } from './metametrics'; | ||
|
||
export enum AccountOverviewTabKey { | ||
Tokens = 'tokens', | ||
Nfts = 'nfts', | ||
Activity = 'activity', | ||
} | ||
|
||
export const ACCOUNT_OVERVIEW_TAB_KEY_TO_METAMETRICS_EVENT_NAME_MAP = { | ||
[AccountOverviewTabKey.Tokens]: MetaMetricsEventName.TokenScreenOpened, | ||
[AccountOverviewTabKey.Nfts]: MetaMetricsEventName.NftScreenOpened, | ||
[AccountOverviewTabKey.Activity]: MetaMetricsEventName.ActivityScreenOpened, | ||
} as const; | ||
|
||
export const ACCOUNT_OVERVIEW_TAB_KEY_TO_TRACE_NAME_MAP = { | ||
[AccountOverviewTabKey.Tokens]: TraceName.AccountOverviewAssetListTab, | ||
[AccountOverviewTabKey.Nfts]: TraceName.AccountOverviewNftsTab, | ||
[AccountOverviewTabKey.Activity]: TraceName.AccountOverviewActivityTab, | ||
} as const; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -39,6 +39,7 @@ import { | |
} from '../../../../../../shared/constants/metametrics'; | ||
import { getCurrentLocale } from '../../../../../ducks/locale/locale'; | ||
import Spinner from '../../../../ui/spinner'; | ||
import { endTrace, TraceName } from '../../../../../../shared/lib/trace'; | ||
|
||
export default function NftsTab() { | ||
const useNftDetection = useSelector(getUseNftDetection); | ||
|
@@ -84,6 +85,7 @@ export default function NftsTab() { | |
referrer: ORIGIN_METAMASK, | ||
}, | ||
}); | ||
endTrace({ name: TraceName.AccountOverviewNftsTab }); | ||
}, [ | ||
nftsLoading, | ||
showNftBanner, | ||
|
@@ -93,6 +95,12 @@ export default function NftsTab() { | |
currentLocale, | ||
]); | ||
|
||
useEffect(() => { | ||
if (!nftsLoading && !nftsStillFetchingIndication) { | ||
endTrace({ name: TraceName.AccountOverviewNftsTab }); | ||
} | ||
}, [nftsLoading, nftsStillFetchingIndication]); | ||
|
||
Comment on lines
+97
to
+102
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. From https://react.dev/learn/you-might-not-need-an-effect#recap
This comment was marked as outdated.
Sorry, something went wrong. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Applied, but with some questions: Wouldn't this be a case where I'm not sure if introducing impure logic to the top-level scope of a react component is in keeping with the article's recommendations. This usage also doesn't seem to fall under any of the example cases discussed in the article (no computations, no altering state in this or any relative component, no event handlers, etc.). That said, removing the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've gone back to using
I tried removing them, but ran into issues where the spans were not behaving as intended e.g. FCP lasting longer than FMP, FMP lasting longer than fully loaded. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I reviewed the react docs in more detail, and it seems to indicate that
Edit: Upon discussion..
|
||
if (!hasAnyNfts && nftsStillFetchingIndication) { | ||
return ( | ||
<Box className="nfts-tab__loading"> | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,8 +1,10 @@ | ||
import { AccountOverviewTabKey } from '../../../../shared/constants/app-state'; | ||
|
||
export type AccountOverviewCommonProps = { | ||
onTabClick: (tabName: string) => void; | ||
setBasicFunctionalityModalOpen: () => void; | ||
///: BEGIN:ONLY_INCLUDE_IF(build-main) | ||
onSupportLinkClick: () => void; | ||
///: END:ONLY_INCLUDE_IF | ||
defaultHomeActiveTabName: string; | ||
defaultHomeActiveTabName: AccountOverviewTabKey | null; | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@HowardBraham Had to remove this line as it always triggers when
nftsLoading
is set to false, even if thedetectNfts
background request is still pending. I'll use this to implementTraceName.AccountOverviewNftsTabFCP
in the PR for additional spans.I added screenshots in the PR description Results section showing that the NftsTab span now captures both performance issues targeted by this ticket.