Skip to content

[WIP] [Performance] Fix inbox pre-insert and search dismiss + refactor submit orchestrator #93713

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.

Sign up for GitHub

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

Draft
JakubKorytko wants to merge 1 commit into
base: main
Choose a base branch
from
+119 −70
Draft

[WIP] [Performance] Fix inbox pre-insert and search dismiss + refactor submit orchestrator #93713

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
161 changes: 101 additions & 60 deletions src/pages/iou/request/step/confirmation/SubmitExpenseOrchestrator.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import DateUtils from '@libs/DateUtils';
import {cancelDeferredWrite, flushDeferredWrite, reserveDeferredWriteChannel} from '@libs/deferredLayoutWrite';
import getIsNarrowLayout from '@libs/getIsNarrowLayout';
import Log from '@libs/Log';
import getTopmostReportParams from '@libs/Navigation/helpers/getTopmostReportParams';
import isReportOpenInRHP from '@libs/Navigation/helpers/isReportOpenInRHP';
import isReportOpenInSuperWideRHP from '@libs/Navigation/helpers/isReportOpenInSuperWideRHP';
import isReportTopmostSplitNavigator from '@libs/Navigation/helpers/isReportTopmostSplitNavigator';
Expand Down Expand Up @@ -140,11 +141,15 @@ function SubmitExpenseOrchestrator({
return () => clearTimeout(confirmingSafetyTimeout.current);
}, [isConfirming]);

// Unified from both prop (isFromGlobalCreate) and transaction flags because
// the transaction flags are the source of truth — the prop is derived from
// the same transaction at mount time. Either source being true is sufficient
// for correct handler selection (e.g. SEARCH_DISMISS) and telemetry.
const isFromGlobalCreateFromTransaction = !!(isFromGlobalCreateOnTransaction || isFromFloatingActionButtonOnTransaction);
const isFromGlobalCreateForNavigation = !!(isFromGlobalCreate || isFromGlobalCreateFromTransaction);

const startSubmitSpans = () => {
const hasReceiptFiles = Object.values(receiptFiles).some((receipt) => !!receipt);
// Re-derive from transaction inside the callback so telemetry captures the value
// at submission time, not at render time (transaction is mutable Onyx state).
const isFromGlobalCreateForTelemetry = !!(isFromGlobalCreateOnTransaction || isFromFloatingActionButtonOnTransaction);
const scenario = getSubmitExpenseScenario({
iouType,
isDistanceRequest,
Expand All @@ -153,15 +158,15 @@ function SubmitExpenseOrchestrator({
isCategorizingTrackExpense,
isSharingTrackExpense,
isPerDiemRequest,
isFromGlobalCreate: isFromGlobalCreateForTelemetry,
isFromGlobalCreate: isFromGlobalCreateForNavigation,
hasReceiptFiles,
});

startTracking({
scenario,
iouType,
requestType: requestType ?? 'unknown',
isFromGlobalCreate: isFromGlobalCreateForTelemetry,
isFromGlobalCreate: isFromGlobalCreateForNavigation,
hasReceipt: hasReceiptFiles,
});
};
Expand All @@ -174,7 +179,7 @@ function SubmitExpenseOrchestrator({
return {
isPreInserted,
isReportPreInserted: isPreInserted && Navigation.getPreInsertedFullscreenRouteName() === NAVIGATORS.REPORTS_SPLIT_NAVIGATOR,
isFromGlobalCreate,
isFromGlobalCreate: isFromGlobalCreateForNavigation,
canDismissFromSearch,
navigatesToDestinationReport: iouType === CONST.IOU.TYPE.SPLIT || iouType === CONST.IOU.TYPE.TRACK,
destinationReportID,
Expand All @@ -188,7 +193,7 @@ function SubmitExpenseOrchestrator({
// Fast-path handlers defer createTransaction until after the dismiss animation completes
// via dismissModal's afterTransition callback (backed by TransitionTracker). This prevents
// heavy optimistic Onyx writes from blocking the JS thread during the RHP slide-out animation.
const handleSearchPreInsert = () => {
const handleSearchPreInsert = (locationPermissionGranted = false) => {
setFastPath(CONST.TELEMETRY.FAST_PATH_HANDLER.SEARCH_PRE_INSERT, CONST.TELEMETRY.SUBMIT_OPTIMIZATION.PRE_INSERT, CONST.TELEMETRY.SUBMIT_OPTIMIZATION.DISMISS_FIRST);
setPendingSubmitFollowUpAction(CONST.TELEMETRY.SUBMIT_FOLLOW_UP_ACTION.NAVIGATE_TO_SEARCH);
Navigation.clearFullscreenPreInsertedFlag();
Expand All @@ -198,32 +203,51 @@ function SubmitExpenseOrchestrator({
// shouldHandleNavigation defaults to true here (other fast paths pass false). The Search screen was
// pre-inserted before the modal opened, so the nav stack is already correct and createTransaction's
// post-create cleanup (navigateAfterExpenseCreate) finishes the flow.
createTransaction();
createTransaction(locationPermissionGranted);
setIsConfirming(false);
},
});
};

const handleReportPreInsert = () => {
const dismissAfterEnsuringDestinationReportIsPreInserted = (reportID: string | undefined, afterTransition: () => void) => {
if (!reportID) {
Navigation.dismissModal({afterTransition});
return;
}

// Only trust the pre-inserted report if the Reports tab is actually focused.
// getTopmostReportParams can find a matching report in an unfocused/stale
// Reports tab (e.g. user opened the report earlier, returned to Inbox, then
// submitted). Dismissing in that case would reveal Inbox, not the report.
if (isReportTopmostSplitNavigator()) {
const preInsertedReportID = getTopmostReportParams(navigationRef.getRootState())?.reportID;
if (preInsertedReportID === reportID) {
Navigation.dismissModal({afterTransition});
return;
}
}

Navigation.revealRouteBeforeDismissingModal(ROUTES.REPORT_WITH_ID.getRoute(reportID), {afterTransition});
};

const handleReportPreInsert = (locationPermissionGranted = false) => {
setFastPath(CONST.TELEMETRY.FAST_PATH_HANDLER.REPORT_PRE_INSERT, CONST.TELEMETRY.SUBMIT_OPTIMIZATION.PRE_INSERT, CONST.TELEMETRY.SUBMIT_OPTIMIZATION.DISMISS_FIRST);
Navigation.clearFullscreenPreInsertedFlag();
setPendingSubmitFollowUpAction(CONST.TELEMETRY.SUBMIT_FOLLOW_UP_ACTION.DISMISS_MODAL_AND_OPEN_REPORT, destinationReportID);
reserveDeferredWriteChannel(CONST.DEFERRED_LAYOUT_WRITE_KEYS.DISMISS_MODAL, {destinationReportID});
Navigation.dismissModal({
afterTransition: () => {
createTransaction(false, false);
setIsConfirming(false);
},
dismissAfterEnsuringDestinationReportIsPreInserted(destinationReportID, () => {
createTransaction(locationPermissionGranted, false);
setIsConfirming(false);
});
};

const handleDismissModalFastPath = () => {
const handleDismissModalFastPath = (locationPermissionGranted = false) => {
setFastPath(CONST.TELEMETRY.FAST_PATH_HANDLER.DISMISS_MODAL, CONST.TELEMETRY.SUBMIT_OPTIMIZATION.DISMISS_FIRST);
const shouldPreserveSearchWithPlaceholder = (iouType === CONST.IOU.TYPE.SPLIT || iouType === CONST.IOU.TYPE.TRACK) && isSearchTopmostFullScreenRoute();
reserveDeferredWriteChannel(shouldPreserveSearchWithPlaceholder ? CONST.DEFERRED_LAYOUT_WRITE_KEYS.SEARCH : CONST.DEFERRED_LAYOUT_WRITE_KEYS.DISMISS_MODAL, {destinationReportID});

const runAfterDismiss = () => {
createTransaction(false, false);
createTransaction(locationPermissionGranted, false);
setIsConfirming(false);
};

Expand All @@ -241,35 +265,56 @@ function SubmitExpenseOrchestrator({
// Wide: always the handler
// Narrow: only runs if the user submitted before the pre-insert timer (300ms)
// elapsed - SEARCH_PRE_INSERT is the primary narrow handler.
const handleSearchDismiss = () => {
const handleSearchDismiss = (locationPermissionGranted = false) => {
setFastPath(CONST.TELEMETRY.FAST_PATH_HANDLER.SEARCH_DISMISS, CONST.TELEMETRY.SUBMIT_OPTIMIZATION.DISMISS_FIRST);
const searchType = iouType === CONST.IOU.TYPE.INVOICE ? CONST.SEARCH.DATA_TYPES.INVOICE : CONST.SEARCH.DATA_TYPES.EXPENSE;
const isSameType = getCurrentSearchQueryJSON()?.type === searchType;
const isNarrow = getIsNarrowLayout();
setPendingSubmitFollowUpAction(isSameType ? CONST.TELEMETRY.SUBMIT_FOLLOW_UP_ACTION.DISMISS_MODAL_ONLY : CONST.TELEMETRY.SUBMIT_FOLLOW_UP_ACTION.NAVIGATE_TO_SEARCH);
reserveDeferredWriteChannel(CONST.DEFERRED_LAYOUT_WRITE_KEYS.SEARCH);

const runAfterDismiss = () => {
createTransaction(false, false);
createTransaction(locationPermissionGranted, false);
setIsConfirming(false);
};

if (!isSameType && !getIsNarrowLayout()) {
const runAfterSearchDismissRecovery = (afterRecovery?: () => void) => {
const finish = () => {
runAfterDismiss();
afterRecovery?.();
};

if (navigationRef.getRootState()?.routes?.at(-1)?.name !== NAVIGATORS.RIGHT_MODAL_NAVIGATOR) {
finish();
return;
}

Log.info('[SubmitExpenseOrchestrator] Search dismiss recovery: RHP still on top after first dismiss, dismissing again');
Navigation.dismissModal({
afterTransition: finish,
});
};

if (!isSameType && !isNarrow) {
dismissWideToNewSearchType(searchType, runAfterDismiss);
return;
}

Navigation.dismissModal({
afterTransition: () => {
runAfterDismiss();
// Narrow fallback: pre-insert timer didn't fire, navigate after dismiss.
if (!isSameType) {
runAfterSearchDismissRecovery(() => {
// Narrow fallback: pre-insert timer didn't fire, navigate after dismiss.
if (isSameType) {
return;
}

Navigation.navigate(ROUTES.SEARCH_ROOT.getRoute({query: buildCannedSearchQuery({type: searchType})}), {forceReplace: true});
}
});
},
});
};

const handleDismissToReport = () => {
const handleDismissToReport = (locationPermissionGranted = false) => {
if (!destinationReportID) {
// Tracking already started in onSubmit; just override the fast path label.
Log.warn('[SubmitExpenseOrchestrator] handleDismissToReport reached without destinationReportID - falling back to default submit');
Expand All @@ -281,7 +326,7 @@ function SubmitExpenseOrchestrator({
// is intentionally the same approach used in handleDefaultSubmit so
// this fallback behaves identically to the standard submit path.
requestAnimationFrame(() => {
createTransaction();
createTransaction(locationPermissionGranted);
requestAnimationFrame(() => {
setIsConfirming(false);
});
Expand All @@ -294,25 +339,25 @@ function SubmitExpenseOrchestrator({

Navigation.revealRouteBeforeDismissingModal(ROUTES.REPORT_WITH_ID.getRoute(destinationReportID), {
afterTransition: () => {
createTransaction(false, false);
createTransaction(locationPermissionGranted, false);
setIsConfirming(false);
},
});
};

// A global-create submit off the inbox lands on Search — reserve the channel so the optimistic write defers behind the skeleton.
const reserveSearchChannelIfGlobalCreate = () => {
if (!isFromGlobalCreate || isReportTopmostSplitNavigator()) {
if (!isFromGlobalCreateForNavigation || isReportTopmostSplitNavigator()) {
return;
}
reserveDeferredWriteChannel(CONST.DEFERRED_LAYOUT_WRITE_KEYS.SEARCH);
};

const handleDefaultSubmit = () => {
const handleDefaultSubmit = (locationPermissionGranted = false) => {
setFastPath(CONST.TELEMETRY.FAST_PATH_HANDLER.DEFAULT);
reserveSearchChannelIfGlobalCreate();
requestAnimationFrame(() => {
createTransaction();
createTransaction(locationPermissionGranted);
requestAnimationFrame(() => {
setIsConfirming(false);
});
Expand All @@ -323,7 +368,7 @@ function SubmitExpenseOrchestrator({
// When the destination report is empty we reserve a DISMISS_MODAL deferred-write channel
// so that MoneyRequestReportActionsList can show a loading skeleton instead of the
// "no expenses" empty state while the dismiss animation plays.
const handleReportInRHPDismiss = () => {
const handleReportInRHPDismiss = (locationPermissionGranted = false) => {
setFastPath(CONST.TELEMETRY.FAST_PATH_HANDLER.REPORT_IN_RHP_DISMISS, CONST.TELEMETRY.SUBMIT_OPTIMIZATION.DISMISS_FIRST);
const rootState = navigationRef.getRootState();

Expand All @@ -341,7 +386,7 @@ function SubmitExpenseOrchestrator({
if (isDestinationEmpty) {
flushDeferredWrite(CONST.DEFERRED_LAYOUT_WRITE_KEYS.DISMISS_MODAL);
}
createTransaction(false, false);
createTransaction(locationPermissionGranted, false);
setIsConfirming(false);
};

Expand All @@ -359,7 +404,26 @@ function SubmitExpenseOrchestrator({
if (isDestinationEmpty) {
cancelDeferredWrite(CONST.DEFERRED_LAYOUT_WRITE_KEYS.DISMISS_MODAL);
}
handleDefaultSubmit();
handleDefaultSubmit(locationPermissionGranted);
};

const dispatchSubmitHandler = (locationPermissionGranted = false) => {
startSubmitSpans();
const rootState = navigationRef.getRootState();
const snapshot = buildNavigationSnapshot(rootState);
const handler = getSubmitHandler(snapshot);

const handlers: Record<SubmitHandler, () => void> = {
[SUBMIT_HANDLER.SEARCH_PRE_INSERT]: () => handleSearchPreInsert(locationPermissionGranted),
[SUBMIT_HANDLER.REPORT_PRE_INSERT]: () => handleReportPreInsert(locationPermissionGranted),
[SUBMIT_HANDLER.DISMISS_MODAL]: () => handleDismissModalFastPath(locationPermissionGranted),
[SUBMIT_HANDLER.DISMISS_TO_REPORT]: () => handleDismissToReport(locationPermissionGranted),
[SUBMIT_HANDLER.REPORT_IN_RHP_DISMISS]: () => handleReportInRHPDismiss(locationPermissionGranted),
[SUBMIT_HANDLER.SEARCH_DISMISS]: () => handleSearchDismiss(locationPermissionGranted),
[SUBMIT_HANDLER.DEFAULT]: () => handleDefaultSubmit(locationPermissionGranted),
};

handlers[handler]();
};

// Not wrapped in useCallback: MoneyRequestConfirmationList is React.memo-wrapped, but this
Expand All @@ -381,22 +445,7 @@ function SubmitExpenseOrchestrator({
}
}

startSubmitSpans();

const rootState = navigationRef.getRootState();
const handler = getSubmitHandler(buildNavigationSnapshot(rootState));

const handlers: Record<SubmitHandler, () => void> = {
[SUBMIT_HANDLER.SEARCH_PRE_INSERT]: handleSearchPreInsert,
[SUBMIT_HANDLER.REPORT_PRE_INSERT]: handleReportPreInsert,
[SUBMIT_HANDLER.DISMISS_MODAL]: handleDismissModalFastPath,
[SUBMIT_HANDLER.DISMISS_TO_REPORT]: handleDismissToReport,
[SUBMIT_HANDLER.REPORT_IN_RHP_DISMISS]: handleReportInRHPDismiss,
[SUBMIT_HANDLER.SEARCH_DISMISS]: handleSearchDismiss,
[SUBMIT_HANDLER.DEFAULT]: handleDefaultSubmit,
};

handlers[handler]();
dispatchSubmitHandler();
};

return (
Expand All @@ -407,24 +456,16 @@ function SubmitExpenseOrchestrator({
resetPermissionFlow={() => {
setStartLocationPermissionFlow(false);
}}
// onGrant/onDeny fire before the permission modal finishes closing.
// On iOS, navigating immediately would break the modal close animation.
onGrant={() => {
startSubmitSpans();
setFastPath(CONST.TELEMETRY.FAST_PATH_HANDLER.DEFAULT);
reserveSearchChannelIfGlobalCreate();
navigateAfterInteraction(() => {
createTransaction(true);
});
navigateAfterInteraction(() => dispatchSubmitHandler(true));
}}
onDeny={(wasUserInitiated) => {
startSubmitSpans();
setFastPath(CONST.TELEMETRY.FAST_PATH_HANDLER.DEFAULT);
if (wasUserInitiated) {
updateLastLocationPermissionPrompt();
}
reserveSearchChannelIfGlobalCreate();
navigateAfterInteraction(() => {
createTransaction(false);
});
navigateAfterInteraction(() => dispatchSubmitHandler(false));
}}
onInitialGetLocationCompleted={() => {
setIsConfirming(false);
Expand Down
14 changes: 9 additions & 5 deletions tests/ui/TimeExpenseConfirmationTest.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,11 +63,15 @@ jest.mock('@components/ProductTrainingContext', () => ({
jest.mock('@src/hooks/useResponsiveLayout');

jest.mock('@libs/Navigation/navigationRef', () => ({
getCurrentRoute: jest.fn(() => ({
name: 'Money_Request_Step_Confirmation',
params: {},
})),
getState: jest.fn(() => ({})),
__esModule: true,
default: {
getCurrentRoute: jest.fn(() => ({
name: 'Money_Request_Step_Confirmation',
params: {},
})),
getState: jest.fn(() => ({})),
getRootState: jest.fn(() => ({routes: []})),
},
}));

jest.mock('@libs/Navigation/Navigation', () => {
Expand Down
14 changes: 9 additions & 5 deletions tests/ui/components/IOURequestStepConfirmationPageTest.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,11 +89,15 @@ jest.mock('@libs/getCurrentPosition');
jest.mock('@libs/getIsNarrowLayout', () => jest.fn(() => false));

jest.mock('@libs/Navigation/navigationRef', () => ({
getCurrentRoute: jest.fn(() => ({
name: 'Money_Request_Step_Confirmation',
params: {},
})),
getState: jest.fn(() => ({})),
__esModule: true,
default: {
getCurrentRoute: jest.fn(() => ({
name: 'Money_Request_Step_Confirmation',
params: {},
})),
getState: jest.fn(() => ({})),
getRootState: jest.fn(() => ({routes: []})),
},
}));

jest.mock('@libs/Navigation/Navigation', () => {
Expand Down
Loading