[$250] Refactor ImportSpreadsheetConfirmModal

[roryabraham]

Problem

ImportSpreadsheetConfirmModal fails to compile with React Compiler, because it's not following React best practices, as described in this slack comment:

• Removing manual memoization causes it to break
• bastion components that only return null are a code smell. There are some legitimate uses for these (i.e: you want to conditionally subscribe to some data, you can conditionally render a component, but not conditionally call a hook). But that's the exception to the rule. If we're using a bastion component just for a useEffect, it should probably be a hook
• ImportSpreadsheetConfirmModal is basically monkey-patching the old ConfirmModal pattern with the new global modal pattern under the hood
• Rather than controlling an imperative API (showConfirmModal) via useEffect, it's simpler (and more efficient) to just call it in the user event handler (i.e: button click) that ultimately causes that effect to run. This best practice is documented clearly in the React docs here

Solution

I took a stab at a holistic refactor, covering only one usage. In my opinion it ends up both simpler and more efficient. It does use API.makeRequestWithSideEffects, but this flow doesn't write any data optimistically, and the intention is to wait for a response then show a confirmation modal. Plus this flow is already blocked when offline. So API.makeRequestWithSideEffects is actually the better choice in this case.

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~022053928240587573729
• Upwork Job ID: 2053928240587573729
• Last Price Increase: 2026-05-11

Issue Owner

Current Issue Owner: @KJ21-ENG

[roryabraham]

Marking this external to get a C and C+ to help take the holistic refactor POC over the finish line.

[KJ21-ENG]

Replaces my earlier proposal. The root cause is unchanged, but the prior body undercounted the legacy consumers and used a few stale command/file names. Verified current main has nine <ImportSpreadsheetConfirmModal> page call sites, including ImportedMultiLevelTagsPage, and the company-card flow also needs to preserve the pending-message copy currently driven by shouldShowPendingMessage.

Proposal

Please re-state the problem that we are trying to solve in this issue.

ImportSpreadsheetConfirmModal does not compile under React Compiler because it is a null-rendering component whose only purpose is to run an effect. That effect watches ONYXKEYS.IMPORTED_SPREADSHEET modal flags and then calls the imperative showConfirmModal API.

The refactor should remove that bastion component and stop using Onyx as a one-shot "open the modal now" bus. The import result modal should be shown from the import button handler after the import action resolves, using the hook pattern from the #89298 POC.

What is the root cause of that problem?

ImportSpreadsheetConfirmModal reads importFinalModal and shouldFinalModalBeOpened from ONYXKEYS.IMPORTED_SPREADSHEET, then calls showConfirmModal inside useEffect. The component returns null, and the effect suppresses react-hooks/exhaustive-deps because callback dependencies are unstable.

App/src/components/ImportSpreadsheetConfirmModal.tsx

Lines 22 to 55 in 3c5eab5

	function ImportSpreadsheetConfirmModal({isVisible, closeImportPageAndModal, onModalHide, shouldHandleNavigationBack = true}: ImportSpreadsheetConfirmModalProps) {
	const {translate} = useLocalize();
	const {showConfirmModal} = useConfirmModal();
	const [spreadsheet] = useOnyx(ONYXKEYS.IMPORTED_SPREADSHEET);
	const titleText = spreadsheet?.importFinalModal?.titleKey ? translate(spreadsheet.importFinalModal.titleKey) : '';
	const promptText = spreadsheet?.importFinalModal?.promptKey
	? translate(spreadsheet.importFinalModal.promptKey, spreadsheet.importFinalModal.promptKeyParams as TranslationParameters<typeof spreadsheet.importFinalModal.promptKey>[0])
	: '';
	const pendingText = spreadsheet?.shouldShowPendingMessage ? translate('spreadsheet.importCompanyCardTransactionsPendingMessage') : '';
	const fullPromptText = pendingText ? `${promptText} ${pendingText}` : promptText;
	useEffect(() => {
	if (!isVisible || !titleText || !promptText || !spreadsheet?.importFinalModal) {
	return;
	}
	showConfirmModal({
	id: 'import-spreadsheet-confirm-modal',
	title: titleText,
	prompt: fullPromptText,
	confirmText: translate('common.buttonConfirm'),
	shouldShowCancelButton: false,
	shouldHandleNavigationBack,
	onModalHide,
	}).then(() => {
	closeImportPageAndModal();
	});
	// We don't need the callbacks as dependencies as they are unstable
	// references that cause an infinite re-render loop.
	// eslint-disable-next-line react-hooks/exhaustive-deps
	}, [isVisible, titleText, fullPromptText, spreadsheet?.importFinalModal, shouldHandleNavigationBack]);
	return null;

Those Onyx fields are defined as part of the imported-spreadsheet model even though they are transient UI side-effect data, not persisted spreadsheet state.

App/src/types/onyx/ImportedSpreadsheet.ts

Lines 51 to 58 in 3c5eab5

	/** Whether final modal should be opened */
	shouldFinalModalBeOpened: boolean;
	/** Whether to show a pending message in the final modal informing that cards and transactions may take time to appear */
	shouldShowPendingMessage?: boolean;
	/** Texts to display depending on request success/failure */
	importFinalModal: ImportFinalModalUnion;

The action layer writes the modal-open fields after import commands. For example, categories writes shouldFinalModalBeOpened and importFinalModal in successData / failureData, and tags does the same for both single-level and multi-level imports.

App/src/libs/actions/Policy/Category.ts

Lines 310 to 344 in 3c5eab5

	function updateImportSpreadsheetData({added, updated}: {added: number; updated: number}) {
	const onyxData: OnyxData<typeof ONYXKEYS.IMPORTED_SPREADSHEET> = {
	successData: [
	{
	onyxMethod: Onyx.METHOD.MERGE,
	key: ONYXKEYS.IMPORTED_SPREADSHEET,
	value: {
	shouldFinalModalBeOpened: true,
	importFinalModal: {
	titleKey: 'spreadsheet.importSuccessfulTitle',
	promptKey: 'spreadsheet.importCategoriesSuccessfulDescription',
	promptKeyParams: {
	added,
	updated,
	},
	},
	},
	},
	],
	failureData: [
	{
	onyxMethod: Onyx.METHOD.MERGE,
	key: ONYXKEYS.IMPORTED_SPREADSHEET,
	value: {
	shouldFinalModalBeOpened: true,
	importFinalModal: {
	titleKey: 'spreadsheet.importFailedTitle',
	promptKey: 'spreadsheet.importFailedDescription',
	},
	},
	},
	],
	};
	return onyxData;

App/src/libs/actions/Policy/Tag.ts

Lines 96 to 128 in 3c5eab5

	function updateImportSpreadsheetData(tagsLength: number): OnyxData<typeof ONYXKEYS.IMPORTED_SPREADSHEET> {
	const onyxData: OnyxData<typeof ONYXKEYS.IMPORTED_SPREADSHEET> = {
	successData: [
	{
	onyxMethod: Onyx.METHOD.MERGE,
	key: ONYXKEYS.IMPORTED_SPREADSHEET,
	value: {
	shouldFinalModalBeOpened: true,
	importFinalModal: {
	titleKey: 'spreadsheet.importSuccessfulTitle',
	promptKey: 'spreadsheet.importTagsSuccessfulDescription',
	promptKeyParams: {
	tags: tagsLength,
	},
	},
	},
	},
	],
	failureData: [
	{
	onyxMethod: Onyx.METHOD.MERGE,
	key: ONYXKEYS.IMPORTED_SPREADSHEET,
	value: {
	shouldFinalModalBeOpened: true,
	importFinalModal: {
	titleKey: 'spreadsheet.importFailedTitle',
	promptKey: 'spreadsheet.importFailedDescription',
	},
	},
	},
	],
	};

App/src/libs/actions/Policy/Tag.ts

Lines 885 to 925 in 3c5eab5

	const onyxData: OnyxData<typeof ONYXKEYS.COLLECTION.POLICY | typeof ONYXKEYS.IMPORTED_SPREADSHEET> = {
	successData: [
	{
	onyxMethod: Onyx.METHOD.MERGE,
	key: `${ONYXKEYS.COLLECTION.POLICY}${policyID}`,
	value: {
	hasMultipleTagLists: true,
	},
	},
	{
	onyxMethod: Onyx.METHOD.MERGE,
	key: ONYXKEYS.IMPORTED_SPREADSHEET,
	value: {
	shouldFinalModalBeOpened: true,
	importFinalModal: {
	titleKey: 'spreadsheet.importSuccessfulTitle',
	promptKey: 'spreadsheet.importMultiLevelTagsSuccessfulDescription',
	},
	},
	},
	],
	failureData: [
	{
	onyxMethod: Onyx.METHOD.MERGE,
	key: `${ONYXKEYS.COLLECTION.POLICY}${policyID}`,
	value: {
	hasMultipleTagLists: false,
	},
	},
	{
	onyxMethod: Onyx.METHOD.MERGE,
	key: ONYXKEYS.IMPORTED_SPREADSHEET,
	value: {
	shouldFinalModalBeOpened: true,
	importFinalModal: {
	titleKey: 'spreadsheet.importFailedTitle',
	promptKey: 'spreadsheet.importFailedDescription',
	},
	},
	},
	],

The same pattern is consumed by nine current page call sites: ImportedCategoriesPage, ImportedMembersPage, ImportedMembersConfirmationPage, ImportedTagsPage, ImportedMultiLevelTagsPage, ImportMultiLevelTagsSettingsPage, ImportedPerDiemPage, CompanyCardsImportedPage, and TransactionsImportedPage.

App/src/pages/workspace/tags/ImportedMultiLevelTagsPage.tsx

Lines 31 to 75 in 3c5eab5

	const importTags = useCallback(() => {
	setIsImportingTags(true);
	importMultiLevelTags(policyID, spreadsheet);
	}, [spreadsheet, policyID]);
	if (!spreadsheet && isLoadingOnyxValue(spreadsheetMetadata)) {
	return;
	}
	const spreadsheetColumns = spreadsheet?.data;
	if (!spreadsheetColumns) {
	return <NotFoundPage />;
	}
	const closeImportPageAndModal = () => {
	setIsClosing(true);
	setIsImportingTags(false);
	Navigation.goBack(ROUTES.WORKSPACE_TAGS.getRoute(policyID));
	};
	return (
	<ScreenWrapper
	testID="ImportedMultiLevelTagsPage"
	enableEdgeToEdgeBottomSafeAreaPadding
	shouldShowOfflineIndicatorInWideScreen
	>
	<HeaderWithBackButton
	title={translate('workspace.tags.importTags')}
	onBackButtonPress={() => Navigation.goBack(ROUTES.WORKSPACE_MULTI_LEVEL_TAGS_IMPORT_SETTINGS.getRoute(policyID))}
	/>
	<ImportSpreadsheetColumns
	spreadsheetColumns={spreadsheetColumns}
	columnNames={columnNames}
	importFunction={importTags}
	isButtonLoading={isImportingTags}
	learnMoreLink={CONST.IMPORT_SPREADSHEET.MULTI_LEVEL_TAGS_ARTICLE_LINK}
	shouldShowColumnHeader={false}
	shouldShowDropdownMenu={false}
	customHeaderText={translate('workspace.tags.importMultiLevelTagsSupportingText')}
	/>
	<ImportSpreadsheetConfirmModal
	isVisible={spreadsheet?.shouldFinalModalBeOpened}
	closeImportPageAndModal={closeImportPageAndModal}
	/>

So the trigger is a user pressing Import, the mechanism is action-layer modal data being written to Onyx, and the impact is a separate effect-only component re-emitting that data through an imperative modal API. React Compiler correctly rejects the pattern because the side effect belongs in the event handler that started the import, not in a render-driven effect.

What changes do you think we should make in order to solve the problem?

Add or reuse src/hooks/useImportSpreadsheetConfirmModal.ts from the #89298 POC. The hook should wrap useConfirmModal and expose an imperative function that accepts an ImportFinalModal result, translates its title/prompt, preserves the existing import-spreadsheet-confirm-modal id, and supports the existing onModalHide / shouldHandleNavigationBack behavior. Include the company-card pending-message copy as explicit return data, not as shouldShowPendingMessage on Onyx.

For each import action, stop writing the final modal payload into ONYXKEYS.IMPORTED_SPREADSHEET. Instead, return the modal payload to the caller. For online-only imports that need the server response, move the command from WRITE_COMMANDS to SIDE_EFFECT_REQUEST_COMMANDS and call API.makeRequestWithSideEffects with the existing lint justification from #89298. Where an action already has real optimistic/failure data, such as importCSVCompanyCards and importTransactionsFromCSV, preserve those real Onyx updates and remove only the modal-open fields from Onyx.

App/src/libs/actions/CompanyCards.ts

Lines 1154 to 1190 in 3c5eab5

	const successData: Array<OnyxUpdate<typeof ONYXKEYS.IMPORTED_SPREADSHEET>> = [
	{
	onyxMethod: Onyx.METHOD.MERGE,
	key: ONYXKEYS.IMPORTED_SPREADSHEET,
	value: {
	shouldFinalModalBeOpened: true,
	shouldShowPendingMessage: true,
	importFinalModal: {
	titleKey: 'spreadsheet.importSuccessfulTitle',
	promptKey: 'spreadsheet.importCompanyCardTransactionsSuccessfulDescription',
	promptKeyParams: {
	transactions: transactionsCount,
	},
	},
	},
	},
	];
	const failureData: Array<OnyxUpdate<typeof ONYXKEYS.COLLECTION.SHARED_NVP_PRIVATE_DOMAIN_MEMBER | typeof ONYXKEYS.COLLECTION.LAST_SELECTED_FEED | typeof ONYXKEYS.IMPORTED_SPREADSHEET>> =
	[
	{
	onyxMethod: Onyx.METHOD.MERGE,
	key: `${ONYXKEYS.COLLECTION.LAST_SELECTED_FEED}${policyID}`,
	value: lastSelectedFeed ?? null,
	},
	{
	onyxMethod: Onyx.METHOD.MERGE,
	key: ONYXKEYS.IMPORTED_SPREADSHEET,
	value: {
	shouldFinalModalBeOpened: true,
	importFinalModal: {
	titleKey: 'spreadsheet.importFailedTitle',
	promptKey: 'spreadsheet.importFailedDescription',
	},
	},
	},
	];

App/src/libs/actions/ImportTransactions.ts

Lines 408 to 457 in 3c5eab5

	optimisticData.push({
	onyxMethod: Onyx.METHOD.MERGE,
	key: ONYXKEYS.IMPORTED_SPREADSHEET,
	value: {
	shouldFinalModalBeOpened: true,
	importFinalModal: {
	titleKey: 'spreadsheet.importSuccessfulTitle' as const,
	promptKey: 'spreadsheet.importTransactionsSuccessfulDescription' as const,
	promptKeyParams: {transactions: transactionList.length},
	},
	},
	});
	// Only add card cleanup to failure data if we created a new card
	if (!isAddingToExistingCard) {
	failureData.push({
	onyxMethod: Onyx.METHOD.MERGE,
	key: ONYXKEYS.CARD_LIST,
	value: {
	[cardID]: null,
	},
	});
	}
	failureData.push({
	onyxMethod: Onyx.METHOD.MERGE,
	key: ONYXKEYS.IMPORTED_SPREADSHEET,
	value: {
	shouldFinalModalBeOpened: true,
	importFinalModal: {
	titleKey: 'spreadsheet.importFailedTitle' as const,
	promptKey: 'spreadsheet.importFailedDescription' as const,
	promptKeyParams: undefined,
	},
	},
	});
	// Restore the previous saved layout on failure, or null if none existed
	failureData.push({
	onyxMethod: Onyx.METHOD.MERGE,
	key: ONYXKEYS.NVP_SAVED_CSV_COLUMN_LAYOUT_LIST,
	value: {
	[cardID]: previouslySavedLayout ?? null,
	},
	});
	API.write(WRITE_COMMANDS.IMPORT_CSV_TRANSACTIONS, params, {
	optimisticData,
	failureData,
	});

Update every page import handler so it owns the flow directly: validate columns, set the local loading state, await importAction(...), await showImportSpreadsheetConfirmModal(result), then run the page's existing cleanup/navigation. Remove the <ImportSpreadsheetConfirmModal> JSX from all nine consumers.

Scope boundaries:

• Files touched: src/libs/API/types.ts - move the relevant import commands currently under WRITE_COMMANDS at lines 14, 16, and 175-179 to SIDE_EFFECT_REQUEST_COMMANDS where the migrated action needs a direct response.
• Files touched: src/hooks/useImportSpreadsheetConfirmModal.ts - add/reuse the hook from [POC] Refactor ImportSpreadsheetConfirmModal into a hook (Members flow) #89298, including the optional pending-message suffix for company-card imports.
• Files touched: src/libs/actions/Policy/Member.ts - updateImportSpreadsheetData / importPolicyMembers at lines 146-181 and 1004-1051.
• Files touched: src/libs/actions/Policy/Category.ts - updateImportSpreadsheetData / importPolicyCategories at lines 310-344 and 953-969.
• Files touched: src/libs/actions/Policy/Tag.ts - updateImportSpreadsheetData, importPolicyTags, and importMultiLevelTags at lines 96-128, 247-258, and 880-925.
• Files touched: src/libs/actions/Policy/PerDiem.ts - updateImportSpreadsheetData / importPerDiemRates at lines 128-174.
• Files touched: src/libs/actions/CompanyCards.ts - importCSVCompanyCards at lines 1112-1231, preserving lastSelectedFeed, feed settings rollback, and the pending-message text.
• Files touched: src/libs/actions/ImportTransactions.ts - importTransactionsFromCSV at lines 316-457, preserving transaction/card/layout optimistic and failure data.
• Files touched: the nine page consumers: ImportedCategoriesPage.tsx lines 122-206, ImportedMembersPage.tsx lines 77-252, ImportedMembersConfirmationPage.tsx lines 94-222, ImportedTagsPage.tsx lines 88-158, ImportedMultiLevelTagsPage.tsx lines 31-75, ImportMultiLevelTagsSettingsPage.tsx lines 152-170, ImportedPerDiemPage.tsx lines 107-176, CompanyCardsImportedPage.tsx lines 103-185, and TransactionsImportedPage.tsx lines 97-154.
• Files touched: src/types/onyx/ImportedSpreadsheet.ts - export ImportFinalModal, then remove shouldFinalModalBeOpened, importFinalModal, and shouldShowPendingMessage after all consumers are migrated.
• Files touched: delete src/components/ImportSpreadsheetConfirmModal.tsx after the last consumer is gone.
• Platforms: all platforms, because this is shared React/TypeScript.
• Code paths not touched: ImportSpreadsheetColumns.tsx column-mapping UI, the durable spreadsheet fields on ONYXKEYS.IMPORTED_SPREADSHEET (data, columns, containsHeader, import settings), and the global useConfirmModal implementation.
• API/backend changes: none; the client keeps the same API commands and request params, only the client-side request type and modal-result plumbing change where needed.

What alternative solutions did you explore? (Optional)

Keeping ImportSpreadsheetConfirmModal and trying to make the effect Compiler-compliant is not enough. The issue is not just the dependency array; the component is still a bastion that fires an imperative modal API from an effect.

Converting the component into a hook that still subscribes to Onyx and fires from useEffect is also not enough. That hides the bastion, but keeps Onyx as a one-shot message bus.

Only shipping the Members-flow POC from #89298 is a valid first PR, but not a final state. It leaves the legacy component and modal-open Onyx fields alive for categories, tags, multi-level tags, per diem, company cards, and transactions.

The later human proposals are directionally aligned with this root cause. The missing pieces I am explicitly carrying forward here are the full nine consumer call sites, the actual current command/file names (IMPORT_CSV_COMPANY_CARDS, IMPORT_CSV_TRANSACTIONS, ImportTransactions.ts), and preservation of the company-card pending-message copy while removing shouldShowPendingMessage from Onyx.

[melvin-bot]

Triggered auto assignment to Contributor-plus team member for initial proposal review - @ZhenjaHorbach (External)

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~022053928240587573729