[No QA] Add reasonAttributes to ActivityIndicator usage sites#84727
Conversation
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c382002a69
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const hasFailedOnfido = userWallet?.hasFailedOnfido ?? false; | ||
| const hasEligibleActiveAdmin = hasEligibleActiveAdminFromWorkspaces(allPolicies, currentUserLogin, paymentMethod?.selectedPaymentMethod?.bankAccountID?.toString()); | ||
| const paidGroupPolicy = Object.values(allPolicies ?? {}).find(isPaidGroupPolicy); | ||
| const walletLoadingReasonAttributes: SkeletonSpanReasonAttributes = {context: 'WalletPage', shouldShowLoadingSpinner}; |
There was a problem hiding this comment.
Memoize spinner reason attributes before passing props
This object is recreated on every render, so ActivityIndicator receives a new reasonAttributes reference each time. In useSkeletonSpan (src/libs/telemetry/useSkeletonSpan.ts), the [reasonAttributes, spanId] effect treats that as a change and calls span.addEvent(...) on every re-render while the spinner is mounted, which can generate noisy telemetry and unnecessary overhead during loading states (especially on WalletPage, which re-renders from Onyx/network updates). Please memoize this object (e.g., useMemo keyed by shouldShowLoadingSpinner) so span attribute updates are emitted only when values actually change.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
I think it makes sense. @sosek108 what do you think?
|
I can review this PR if needed. |
|
JmillsExpensify
left a comment
There was a problem hiding this comment.
No product review required.
Reviewer Checklist
|
|
@codex review |
@sosek108 Just confirming the scope of this PR: should we only add |
|
Codex Review: Something went wrong. Try again later by commenting “@codex review”. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@huult yes, only mentioned files. In the end I'll do double check if we missed anything |
|
Thanks for confirming! |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚧 @youssef-lr has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/youssef-lr in version: 9.3.38-0 🚀
|
|
🚀 Deployed to production by https://github.com/cristipaval in version: 9.3.38-4 🚀
|

Explanation of Change
Added
reasonAttributes(withSkeletonSpanReasonAttributes) to allActivityIndicatorusage sites so that Sentry telemetry spans can capture the loading context for each site. This makes it possible to identify which components are responsible for long or infinite loading states in production.Files updated:
AddPlaidBankAccount— tracksplaidData.isLoadingAddToWalletButton/index.native— tracksisLoadingPlaidLink— tracksready/isPlaidLoadedReimbursementAccount/.../BankAccountDetails— trackscorpayFields.isLoadingReimbursementAccount/.../Confirmation— trackscorpayFields.isLoadingPersonalCardDetailsHeaderMenu— tracksisLoadingLastUpdatedWalletPage— tracksshouldShowLoadingSpinnerCopyCodesPage— tracksaccount.isLoadingSubscriptionPlanCard— tracks!privateSubscription(data not yet loaded)Fixed Issues
$ #83394
PROPOSAL:
Tests
Offline tests
N/A
QA Steps
Same as tests
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand 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: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari