diff --git a/src/components/MoneyRequestReportView/MoneyRequestReportActionsList.tsx b/src/components/MoneyRequestReportView/MoneyRequestReportActionsList.tsx index 41abe12d930f..4cb006546532 100644 --- a/src/components/MoneyRequestReportView/MoneyRequestReportActionsList.tsx +++ b/src/components/MoneyRequestReportView/MoneyRequestReportActionsList.tsx @@ -6,7 +6,6 @@ import {useIsReportLoadPending} from '@hooks/useInFlightRequests'; import useIsReportActionsLoaded from '@hooks/useIsReportActionsLoaded'; import useLoadReportActions from '@hooks/useLoadReportActions'; import useLocalize from '@hooks/useLocalize'; -import useMarkOpenReportEndOnSkeleton from '@hooks/useMarkOpenReportEndOnSkeleton'; import useNetworkWithOfflineStatus from '@hooks/useNetworkWithOfflineStatus'; import useNewTransactions from '@hooks/useNewTransactions'; import useOnyx from '@hooks/useOnyx'; @@ -213,9 +212,6 @@ function MoneyRequestReportActionsList({onLayout}: MoneyRequestReportListProps) return filteredActions.slice().reverse(); }, [reportActions, isOffline, canPerformWriteAction, reportTransactionIDs, shouldShowHarvestCreatedAction, visibleReportActionsData, reportID]); - const shouldShowOpenReportLoadingSkeleton = isInitialReportLoadPending && visibleReportActions.length === 0; - useMarkOpenReportEndOnSkeleton(report, shouldShowOpenReportLoadingSkeleton); - const lastAction = visibleReportActions.at(-1); const {scrollOffsetRef} = useActionListContext(); @@ -726,14 +722,14 @@ function MoneyRequestReportActionsList({onLayout}: MoneyRequestReportListProps) * Runs when the FlatList finishes laying out */ const recordTimeToMeasureItemLayout = useCallback(() => { - if (didLayout.current || !report) { + if (didLayout.current || !reportIDFromRoute) { return; } didLayout.current = true; - markOpenReportEnd(report, {warm: !shouldShowOpenReportLoadingSkeleton}); - }, [report, shouldShowOpenReportLoadingSkeleton]); + markOpenReportEnd(reportIDFromRoute, report, {warm: true}); + }, [reportIDFromRoute, report]); const isReportEmpty = isEmpty(visibleReportActions) && isEmpty(transactions) && !isInitialReportLoadPending; const showEmptyState = isReportEmpty; diff --git a/src/components/MoneyRequestReportView/MoneyRequestReportView.tsx b/src/components/MoneyRequestReportView/MoneyRequestReportView.tsx index b1cd8f0dea9c..14789b18d9e6 100644 --- a/src/components/MoneyRequestReportView/MoneyRequestReportView.tsx +++ b/src/components/MoneyRequestReportView/MoneyRequestReportView.tsx @@ -56,6 +56,9 @@ type MoneyRequestReportViewProps = { /** The report */ report: OnyxEntry; + /** Report ID from the route, known before the report itself loads */ + reportIDFromRoute: string | undefined; + /** Loading state for report */ reportLoadingState: OnyxEntry; @@ -110,7 +113,7 @@ function InitialLoadingSkeleton({styles, onLayout}: {styles: ThemeStyles; onLayo ); } -function MoneyRequestReportView({report, reportLoadingState, shouldDisplayReportFooter, backToRoute, onLayout}: MoneyRequestReportViewProps) { +function MoneyRequestReportView({report, reportIDFromRoute, reportLoadingState, shouldDisplayReportFooter, backToRoute, onLayout}: MoneyRequestReportViewProps) { const styles = useThemeStyles(); const {isOffline} = useNetwork(); @@ -205,13 +208,16 @@ function MoneyRequestReportView({report, reportLoadingState, shouldDisplayReport }; }, [reportID]); - useMarkOpenReportEndOnSkeleton(report, shouldShowOpenReportLoadingSkeleton); + const shouldShowEmptyActionsSkeleton = reportActions.length === 0; + const shouldShowAppLoadSkeleton = !!report && isAppLoadPending; + // These skeletons render before the report lands in Onyx, so the mark uses the route id. + useMarkOpenReportEndOnSkeleton(reportIDFromRoute, shouldShowOpenReportLoadingSkeleton || shouldShowEmptyActionsSkeleton || shouldShowAppLoadSkeleton); if (shouldShowOpenReportLoadingSkeleton) { return ; } - if (reportActions.length === 0) { + if (shouldShowEmptyActionsSkeleton) { return ; } @@ -219,7 +225,7 @@ function MoneyRequestReportView({report, reportLoadingState, shouldDisplayReport return; } - if (isAppLoadPending) { + if (shouldShowAppLoadSkeleton) { return ( diff --git a/src/hooks/useMarkOpenReportEndOnSkeleton.ts b/src/hooks/useMarkOpenReportEndOnSkeleton.ts index 6b6d058b4554..9cbd0a1546c0 100644 --- a/src/hooks/useMarkOpenReportEndOnSkeleton.ts +++ b/src/hooks/useMarkOpenReportEndOnSkeleton.ts @@ -1,24 +1,26 @@ +import getNonEmptyStringOnyxID from '@libs/getNonEmptyStringOnyxID'; import markOpenReportEnd from '@libs/telemetry/markOpenReportEnd'; -import type * as OnyxTypes from '@src/types/onyx'; - -import type {OnyxEntry} from 'react-native-onyx'; +import ONYXKEYS from '@src/ONYXKEYS'; import {useEffect} from 'react'; +import useOnyx from './useOnyx'; + /** - * Closes the open-report performance span as a cold open (`warm: false`) the moment a cold-load skeleton - * appears. Shared by every report surface that gates its own skeleton (the chat-list guard, the route - * orchestrator, and the money-request views): the open-report span must still close even though the list - * body — which would otherwise own this mark once mounted — isn't mounted while the skeleton shows. + * Closes the open-report span as a cold open (`warm: false`) while a skeleton shows. Call it from a component + * mounted exactly while the skeleton is on screen. The list body that would otherwise close the span isn't + * mounted yet. */ -function useMarkOpenReportEndOnSkeleton(report: OnyxEntry, isSkeletonVisible: boolean) { +function useMarkOpenReportEndOnSkeleton(reportID: string | undefined, isSkeletonVisible = true) { + const [report] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT}${getNonEmptyStringOnyxID(reportID)}`); + useEffect(() => { - if (!isSkeletonVisible || !report) { + if (!isSkeletonVisible || !reportID) { return; } - markOpenReportEnd(report, {warm: false}); - }, [report, isSkeletonVisible]); + markOpenReportEnd(reportID, report, {warm: false}); + }, [reportID, report, isSkeletonVisible]); } export default useMarkOpenReportEndOnSkeleton; diff --git a/src/libs/telemetry/markOpenReportEnd.ts b/src/libs/telemetry/markOpenReportEnd.ts index cc04a1115836..40e60b40305a 100644 --- a/src/libs/telemetry/markOpenReportEnd.ts +++ b/src/libs/telemetry/markOpenReportEnd.ts @@ -4,6 +4,7 @@ import CONST from '@src/CONST'; import type * as OnyxTypes from '@src/types/onyx'; import type {SpanAttributes} from '@sentry/core'; +import type {OnyxEntry} from 'react-native-onyx'; import {endSpanWithAttributes} from './activeSpans'; @@ -12,22 +13,22 @@ type MarkOpenReportEndOptions = { }; /** - * Mark all 'open_report*' telemetry spans as finished. + * Mark all 'open_report*' telemetry spans as finished. Keyed by `reportID` so it still ends when the report + * hasn't loaded. The report-shape attributes are then left off. */ -function markOpenReportEnd(report: OnyxTypes.Report, options: MarkOpenReportEndOptions = {}) { - const {reportID, type, chatType} = report; - - const isTransactionThread = isReportTransactionThread(report); - const isOneTransactionThread = isOneTransactionReport(report); - +function markOpenReportEnd(reportID: string, report: OnyxEntry, options: MarkOpenReportEndOptions = {}) { const spanId = `${CONST.TELEMETRY.SPAN_OPEN_REPORT}_${reportID}`; - const attributes: SpanAttributes = { - [CONST.TELEMETRY.ATTRIBUTE_IS_TRANSACTION_THREAD]: isTransactionThread, - [CONST.TELEMETRY.ATTRIBUTE_IS_ONE_TRANSACTION_REPORT]: isOneTransactionThread, - [CONST.TELEMETRY.ATTRIBUTE_REPORT_TYPE]: type, - [CONST.TELEMETRY.ATTRIBUTE_CHAT_TYPE]: chatType, - }; + const attributes: SpanAttributes = {}; + + // Cold opens close this span before the report reaches Onyx. Reading the shape off a missing report would + // report `false` for an unknown, so leave the attributes off and let the consumer read absence as unknown. + if (report) { + attributes[CONST.TELEMETRY.ATTRIBUTE_IS_TRANSACTION_THREAD] = isReportTransactionThread(report); + attributes[CONST.TELEMETRY.ATTRIBUTE_IS_ONE_TRANSACTION_REPORT] = isOneTransactionReport(report); + attributes[CONST.TELEMETRY.ATTRIBUTE_REPORT_TYPE] = report.type; + attributes[CONST.TELEMETRY.ATTRIBUTE_CHAT_TYPE] = report.chatType; + } if (options.warm !== undefined) { attributes[CONST.TELEMETRY.ATTRIBUTE_IS_WARM] = options.warm; diff --git a/src/pages/Search/SearchMoneyRequestReportPage.tsx b/src/pages/Search/SearchMoneyRequestReportPage.tsx index 7185d3b24894..d0cf7042b5a5 100644 --- a/src/pages/Search/SearchMoneyRequestReportPage.tsx +++ b/src/pages/Search/SearchMoneyRequestReportPage.tsx @@ -409,6 +409,7 @@ function SearchMoneyRequestReportPage({route}: SearchMoneyRequestPageProps) { } > diff --git a/src/pages/inbox/report/ReportActionsList.tsx b/src/pages/inbox/report/ReportActionsList.tsx index e1146363055a..61b2456cfc7d 100644 --- a/src/pages/inbox/report/ReportActionsList.tsx +++ b/src/pages/inbox/report/ReportActionsList.tsx @@ -430,9 +430,7 @@ function ReportActionsListContent({reportID, onLayout}: ReportActionsListContent didLayout.current = true; - if (report) { - markOpenReportEnd(report, {warm: true}); - } + markOpenReportEnd(reportID, report, {warm: true}); }; // The guard only mounts this content when the report is loaded, so this is effectively unreachable. diff --git a/src/pages/inbox/report/ReportActionsListContext.tsx b/src/pages/inbox/report/ReportActionsListContext.tsx index 5e3777c395a5..0e5e876212ae 100644 --- a/src/pages/inbox/report/ReportActionsListContext.tsx +++ b/src/pages/inbox/report/ReportActionsListContext.tsx @@ -120,7 +120,6 @@ function computeReportActionsSkeletonState(readinessSignals: ReportActionsReadin return { shouldShowLoadingSkeleton, shouldShowDerivedTimingSkeleton, - shouldShowInitialSkeleton, }; } diff --git a/src/pages/inbox/report/ReportActionsLoadingSkeleton.tsx b/src/pages/inbox/report/ReportActionsLoadingSkeleton.tsx index e692a041b327..2fc926b894d0 100644 --- a/src/pages/inbox/report/ReportActionsLoadingSkeleton.tsx +++ b/src/pages/inbox/report/ReportActionsLoadingSkeleton.tsx @@ -2,6 +2,7 @@ import ReportActionsSkeletonView from '@components/ReportActionsSkeletonView'; import useCancelSendMessageSpanOnSkeleton from '@hooks/useCancelSendMessageSpanOnSkeleton'; import type {SkeletonName} from '@hooks/useCancelSendMessageSpanOnSkeleton'; +import useMarkOpenReportEndOnSkeleton from '@hooks/useMarkOpenReportEndOnSkeleton'; import React from 'react'; @@ -14,14 +15,19 @@ type ReportActionsLoadingSkeletonProps = { /** Whether the skeleton rows animate */ shouldAnimate?: boolean; + + /** Whether this skeleton closes the open-report span as a cold open. Off when it isn't waiting on report data */ + shouldMarkOpenReportEnd?: boolean; }; /** - * Report-actions loading skeleton. Mounted only while the skeleton shows, so it hosts the hook that cancels - * the otherwise never-ending send-message span (tagged with `skeletonName`). + * Report-actions loading skeleton. Hosts the skeleton-phase span marks: cancelling the never-ending + * send-message span, and closing the open-report span as cold. Mounted here means visible, while a parent's + * copy of the skeleton condition can drift from what actually renders. */ -function ReportActionsLoadingSkeleton({reportID, skeletonName, shouldAnimate = true}: ReportActionsLoadingSkeletonProps) { +function ReportActionsLoadingSkeleton({reportID, skeletonName, shouldAnimate = true, shouldMarkOpenReportEnd = true}: ReportActionsLoadingSkeletonProps) { useCancelSendMessageSpanOnSkeleton(reportID, skeletonName); + useMarkOpenReportEndOnSkeleton(reportID, shouldMarkOpenReportEnd); return ; } diff --git a/src/pages/inbox/report/ReportActionsSkeletonGuard.tsx b/src/pages/inbox/report/ReportActionsSkeletonGuard.tsx index 649f18c839c4..b65a2e66dcaa 100644 --- a/src/pages/inbox/report/ReportActionsSkeletonGuard.tsx +++ b/src/pages/inbox/report/ReportActionsSkeletonGuard.tsx @@ -1,7 +1,6 @@ import useBackfillWhenNoVisibleActions from '@hooks/useBackfillWhenNoVisibleActions'; import useCopySelectionHelper from '@hooks/useCopySelectionHelper'; import {useIsReportLoadPending} from '@hooks/useInFlightRequests'; -import useMarkOpenReportEndOnSkeleton from '@hooks/useMarkOpenReportEndOnSkeleton'; import usePendingConciergeResponse from '@hooks/usePendingConciergeResponse'; import useReportActionsListModel from '@hooks/useReportActionsListModel'; import useStartConciergeSession from '@hooks/useStartConciergeSession'; @@ -34,10 +33,9 @@ type ReportActionsSkeletonGuardProps = { function ReportActionsSkeletonGuard({reportID, children}: ReportActionsSkeletonGuardProps) { const isReportLoadPending = useIsReportLoadPending(reportID); const {readinessSignals, state, actions} = useReportActionsListModel(reportID, isReportLoadPending); - const {shouldShowLoadingSkeleton, shouldShowDerivedTimingSkeleton, shouldShowInitialSkeleton} = computeReportActionsSkeletonState(readinessSignals); + const {shouldShowLoadingSkeleton, shouldShowDerivedTimingSkeleton} = computeReportActionsSkeletonState(readinessSignals); const { - report, isConciergeMainDM, oldestUnreadReportAction, hasOnceLoadedReportActions, @@ -63,8 +61,6 @@ function ReportActionsSkeletonGuard({reportID, children}: ReportActionsSkeletonG hasCachedReportActions, }); - useMarkOpenReportEndOnSkeleton(report, shouldShowInitialSkeleton); - useBackfillWhenNoVisibleActions({ reportID, isMissingReportActions, diff --git a/tests/ui/MoneyRequestReportViewTest.tsx b/tests/ui/MoneyRequestReportViewTest.tsx index e240c1ff319d..f0dc787734b5 100644 --- a/tests/ui/MoneyRequestReportViewTest.tsx +++ b/tests/ui/MoneyRequestReportViewTest.tsx @@ -117,6 +117,7 @@ const renderMoneyRequestReportView = (onLayout: (event: LayoutChangeEvent) => vo render( { renderReportActionsList(); expect(screen.getByTestId('ReportActionsSkeletonView')).toBeTruthy(); - expect(mockMarkOpenReportEnd).toHaveBeenCalledWith(mockReport, {warm: false}); + expect(mockMarkOpenReportEnd).toHaveBeenCalledWith(mockReport.reportID, mockReport, {warm: false}); expect(mockUseIsReportLoadPending).toHaveBeenCalledWith(mockReport.reportID); }); @@ -575,10 +575,9 @@ describe('ReportActionsList (body)', () => { renderReportActionsList(); - // The guard owns this mark now (it used to live in the body); it must still fire while the - // initial skeleton shows, otherwise the open-report span regresses. + // Must fire while the skeleton shows or the open-report span regresses. expect(screen.getByTestId('ReportActionsSkeletonView')).toBeTruthy(); - expect(mockMarkOpenReportEnd).toHaveBeenCalledWith(mockReport, {warm: false}); + expect(mockMarkOpenReportEnd).toHaveBeenCalledWith(mockReport.reportID, mockReport, {warm: false}); }); it('does not fire the warm:false mark once content is visible', () => { @@ -589,7 +588,7 @@ describe('ReportActionsList (body)', () => { renderReportActionsList(); expect(screen.queryByTestId('ReportActionsSkeletonView')).toBeNull(); - expect(mockMarkOpenReportEnd).not.toHaveBeenCalledWith(mockReport, {warm: false}); + expect(mockMarkOpenReportEnd).not.toHaveBeenCalledWith(mockReport.reportID, mockReport, {warm: false}); }); }); diff --git a/tests/ui/ReportActionsTest.tsx b/tests/ui/ReportActionsTest.tsx index 965e63f42f9a..18f7f63e2496 100644 --- a/tests/ui/ReportActionsTest.tsx +++ b/tests/ui/ReportActionsTest.tsx @@ -199,7 +199,7 @@ describe('ReportActions (orchestrator)', () => { expect(screen.getByTestId('ReportActionsSkeletonView')).toBeTruthy(); expect(mockReportActionsListBody).not.toHaveBeenCalled(); - expect(mockMarkOpenReportEnd).toHaveBeenCalledWith(mockReport, {warm: false}); + expect(mockMarkOpenReportEnd).toHaveBeenCalledWith(REPORT_ID, mockReport, {warm: false}); }); it('mounts the body (not the orchestrator app-load skeleton) for a Concierge report during app load', () => { diff --git a/tests/unit/ReportActionsListContextTest.ts b/tests/unit/ReportActionsListContextTest.ts index 7459e7584111..6bfa0ddbbd1f 100644 --- a/tests/unit/ReportActionsListContextTest.ts +++ b/tests/unit/ReportActionsListContextTest.ts @@ -58,27 +58,27 @@ function createReadinessSignals(overrides: Partial { - it('shows the initial skeleton for a pending OpenReport matching this report', () => { + it('shows the loading skeleton for a pending OpenReport matching this report', () => { const state = computeReportActionsSkeletonState( createReadinessSignals({ isReportLoadPending: true, }), ); - expect(state.shouldShowInitialSkeleton).toBe(true); + expect(state.shouldShowLoadingSkeleton).toBe(true); }); - it('does not show the initial skeleton when no OpenReport matching this report is pending', () => { + it('does not show the loading skeleton when no OpenReport matching this report is pending', () => { const state = computeReportActionsSkeletonState( createReadinessSignals({ isReportLoadPending: false, }), ); - expect(state.shouldShowInitialSkeleton).toBe(false); + expect(state.shouldShowLoadingSkeleton).toBe(false); }); - it('does not show the initial skeleton for a pending OpenReport while offline', () => { + it('does not show the loading skeleton for a pending OpenReport while offline', () => { const state = computeReportActionsSkeletonState( createReadinessSignals({ isOffline: true, @@ -86,17 +86,17 @@ describe('computeReportActionsSkeletonState', () => { }), ); - expect(state.shouldShowInitialSkeleton).toBe(false); + expect(state.shouldShowLoadingSkeleton).toBe(false); }); - it('releases the initial skeleton after a terminal OpenReport failure', () => { + it('releases the loading skeleton after a terminal OpenReport failure', () => { const state = computeReportActionsSkeletonState( createReadinessSignals({ isReportLoadPending: false, }), ); - expect(state.shouldShowInitialSkeleton).toBe(false); + expect(state.shouldShowLoadingSkeleton).toBe(false); }); it('releases the unread initial load when no report load is pending', () => { @@ -196,7 +196,7 @@ describe('computeReportActionsSkeletonState', () => { }), ); - expect(state.shouldShowInitialSkeleton).toBe(false); + expect(state.shouldShowLoadingSkeleton).toBe(false); }); it('preserves the linked message skeleton when the report load is pending', () => { @@ -210,6 +210,6 @@ describe('computeReportActionsSkeletonState', () => { }), ); - expect(state.shouldShowInitialSkeleton).toBe(true); + expect(state.shouldShowLoadingSkeleton).toBe(true); }); });