[No QA][Sentry] Track infinite skeletons #76299
Conversation
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
melvin-bot
bot
requested review from
JmillsExpensify and
abdulrahuman5196
and removed request for
a team
|
@abdulrahuman5196 Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
|
Lint CI is failing because of these errors in skeleton components: changes are not related to my script and errors are triggered because I've modified these components. Should I resolve these problems? |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| import {endSpan, startSpan} from './activeSpans'; | ||
|
|
||
| function useSkeletonSpan(component: string) { | ||
| const reactId = useId(); |
There was a problem hiding this comment.
Potential memory leak: Using useId() in combination with the component name creates a unique span ID for every instance of the component. If skeletons are frequently mounted/unmounted, this could create a large number of span entries in Sentry.
Consider:
- Adding a comment explaining this is intentional for tracking individual skeleton instances
- OR using just the component name if you only need to track whether any instance of the skeleton is visible (not individual instances)
| name: component, | ||
| }); | ||
|
|
||
| return () => endSpan(spanId); |
There was a problem hiding this comment.
Missing JSDoc: This hook lacks documentation. According to the PR checklist and STYLE.md guidelines, hooks should have JSDoc comments explaining:
- What the hook does
- Parameters and their purpose
- Expected behavior
| return () => endSpan(spanId); | |
| /** | |
| * Hook to track skeleton component lifecycle for Sentry telemetry. | |
| * Creates a span when the skeleton mounts and ends it when unmounted. | |
| * This helps identify "infinite skeleton" issues where loading states don't resolve. | |
| * | |
| * @param component - The name of the skeleton component being tracked | |
| */ | |
| function useSkeletonSpan(component: string) { |
| function useSkeletonSpan(component: string) { | ||
| const reactId = useId(); | ||
|
|
||
| useEffect(() => { |
There was a problem hiding this comment.
Missing dependency array optimization: The component parameter is passed as a dependency to useEffect, but it's expected to be a constant string literal. If the component name changes (which shouldn't happen), this will restart spans unnecessarily.
Consider adding a comment or using as const to indicate this should be a constant:
function useSkeletonSpan(component: string) {
const reactId = useId();
useEffect(() => {
// Component name should be a constant string literal for this component
const spanId = `${CONST.TELEMETRY.SPAN_SKELETON}_${component}_${reactId}`;
| * This file contains logic related to tracking skeleton across the application. | ||
| */ | ||
| import {useEffect, useId} from 'react'; | ||
| import CONST from '@src/CONST'; |
There was a problem hiding this comment.
Type safety: The component parameter should be typed more strictly. Consider creating a union type of all valid skeleton component names in CONST.ts to prevent typos and ensure consistency.
Example:
// In CONST.ts
SKELETON_COMPONENTS: {
AVATAR: 'AvatarSkeleton',
ACCOUNT_SWITCHER: 'AccountSwitcherSkeletonView',
// ... etc
} as const
// In useSkeletonSpan.ts
type SkeletonComponentName = ValueOf<typeof CONST.SKELETON_COMPONENTS>;
function useSkeletonSpan(component: SkeletonComponentName) {This would make the usage in components type-safe: useSkeletonSpan(CONST.SKELETON_COMPONENTS.AVATAR)
1 participant
Explanation of Change
This PR introduces new hook that contains common logic used in components related to skeletons. Its goal is to send spans to Sentry containing information about how long did component exist in the tree.
Fixed Issues
$ #74138
PROPOSAL:
Tests
Offline tests
N/A
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
N/A
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
android.mov
Android: mWeb Chrome
iOS: Native
ios.mov
iOS: mWeb Safari
MacOS: Chrome / Safari
web.mov
MacOS: Desktop