[No QA] fix: stop empty LHN collection subs and scope provider logging#91066
Conversation
Remove collection-level Onyx subscriptions from LHNEmptyState and isolate the caught-up log in a renderless component. Remove the provider logging effect that ran after every render and replace it with a scoped transition logger on the inbox sidebar. Co-authored-by: Cursor <cursoragent@cursor.com>
| reportsCount: Object.keys(reports ?? {}).length, | ||
| policyCount: Object.keys(policy ?? {}).length, | ||
| personalDetailsCount: Object.keys(personalDetails ?? {}).length, |
There was a problem hiding this comment.
I think we can remove it now, I dont recall having issues with the wohoo page recently
There was a problem hiding this comment.
Great. Another approach I can think of is to create a new derived value to store the length of the collection
There was a problem hiding this comment.
Thanks! Since @mountiny confirmed the counts aren't valuable, I'd rather not add a derived value just to log them, that would reintroduce a subscription for a single log line. Keeping the simpler Log.info('Woohoo!...') without payload
| const {orderedReports} = useSidebarOrderedReportsState(); | ||
| const orderedReportsLength = orderedReports.length; | ||
| const prevOrderedReportsLength = usePrevious(orderedReportsLength); | ||
| const firstRender = useRef(true); | ||
|
|
||
| useEffect(() => { | ||
| if (orderedReportsLength > 0 && prevOrderedReportsLength === 0) { | ||
| Log.info('[useSidebarOrderedReports] Ordered reports went from empty to non-empty', false); | ||
| } | ||
|
|
||
| if (orderedReportsLength === 0 && (prevOrderedReportsLength ?? 0) > 0) { | ||
| Log.info('[useSidebarOrderedReports] Ordered reports went from non-empty to empty', false); | ||
| } | ||
|
|
||
| if (firstRender.current && orderedReportsLength === 0) { | ||
| Log.info('[useSidebarOrderedReports] Ordered reports initialized empty', false); | ||
| } | ||
|
|
||
| firstRender.current = false; | ||
| }, [orderedReportsLength, prevOrderedReportsLength]); |
There was a problem hiding this comment.
I see that we only care about the number of reports here. So I think sorting them is unnecessary. should we make the number of report into the derived value
There was a problem hiding this comment.
No extra sort cost here useSidebarOrderedReportsState() is just a React context read, and the provider's fn runs once regardless of whether this logger is mounted. Adding a dedicated derived value for the count would actually be more expensive imo. it would need to replicate the full LHN filtering plus a new Onyx subscription, just to feed a transition log
There was a problem hiding this comment.
@BartekObudzinski The log data may have been added intentionally. Please confirm if it's okay to remove it.
There was a problem hiding this comment.
Yes, intentional. The original effect logged parsedDeps, changedDeps, and per-component perf metrics but generating that payload required subscribing to chatReports, policies, transactions, transactionViolations, reportNameValuePairs, and reportAttributes, plus running deepEqual over all of them in a useEffect that ran every render. That's exactly the cost this PR is removing.
There was a problem hiding this comment.
@BartekObudzinski These data logs were added in #69443 and #65923. Could you recheck to see if it's safe to remove now?
There was a problem hiding this comment.
both source issues are closed and this PR had already removed the diagnostic payload that made the logs useful. Dropped both loggers entirely.
There was a problem hiding this comment.
@mountiny Could you confirm again that these data logs (currentDeps, previousDeps, perfRef) are fine to remove?
There was a problem hiding this comment.
Yeah I think that its ok to remove now
|
@DylanDylann 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] |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / Safari |
|
🚧 @mountiny 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! 🧪🧪
|
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/mountiny in version: 9.3.82-0 🚀
Bundle Size Analysis (Sentry): |
|
I reviewed the changes in this PR. It removes internal logging subscriptions and utility functions from No help site changes are required. This PR does not affect any user-facing behavior, features, UI, or settings. @BartekObudzinski, please review the linked help site PR and confirm it reflects the current behavior. Then mark the linked help site PR |
|
🚀 Deployed to production by https://github.com/mountiny in version: 9.3.82-3 🚀
|
5 participants
Explanation of Change
LHNEmptyStatewas subscribing to the full report, policy, and personal details collections only to log counts while the empty inbox was visible. That caused unnecessary re-renders during cold start when data was still streaming in.This PR removes those subscriptions from the empty-state UI, moves the caught-up log into a renderless
LHNCaughtUpLogger, and removes theuseSidebarOrderedReportsprovider logging effect that ran after every render. Transition logging is now handled by a scopedSidebarOrderedReportsTransitionLoggeron the inbox sidebar, which only runs when the ordered report list transitions between empty and non-empty.This is Plan A toward improving inbox performance (#77175): it reduces wasted render work on the empty LHN path without changing populated-list behavior.
Fixed Issues
$ #91272
PROPOSAL:
Tests
Offline tests
N/A
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
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