Skip to content

[Due for payment 2026-03-24] [$1000] Clean up IOURequestStepScan #79929

@roryabraham

Description

@roryabraham

Coming from #79324 (comment). I chatted with cursor a bit and came up with the outline of a plan to improve this component.


IOURequestStepScan Component Refactoring Plan

Problem Summary

The IOURequestStepScan component has accumulated significant technical debt:

  1. Configuration Overload: Three props (isMultiScanEnabled, isStartingScan, setIsMultiScanEnabled) control behavior modes, making the component hard to reason about
  2. Scattered Mobile Web Checks: isMobile() is called ~10 times throughout index.tsx instead of once at the top level
  3. Incorrect Abstractions: Using isSmallScreenWidth as a proxy for drag-and-drop capability
  4. React Compiler Blockers: 2 suppressed exhaustive-deps ESLint rules prevent React Compiler adoption
  5. Massive Duplication: ~2200 lines total across index.tsx and index.native.tsx with significant shared business logic

Refactoring Strategy

Phase 1: Add Automated Test Coverage (Safety Net)

Add tests first to prevent regressions during refactoring and validate the current behavior.

Unit Tests for Pure Functions

File: tests/unit/TransactionUtilsTest.ts (extend existing)

Add tests for shouldReuseInitialTransaction - this function is central to the bug in issue #79324:

describe('shouldReuseInitialTransaction', () => {
    test('returns false when initialTransaction is null', () => {
        expect(shouldReuseInitialTransaction(null, true, 0, false, [])).toBe(false);
    });

    test('returns true for single file upload (shouldAcceptMultipleFiles=false)', () => {
        const transaction = createRandomTransaction(1);
        expect(shouldReuseInitialTransaction(transaction, false, 0, false, [])).toBe(true);
    });

    test('returns false for index > 0 in multi-file upload', () => {
        const transaction = createRandomTransaction(1);
        expect(shouldReuseInitialTransaction(transaction, true, 1, false, [])).toBe(false);
    });

    test('returns true for first file when multi-scan disabled and no existing receipt', () => {
        const transaction = createRandomTransaction(1);
        transaction.receipt = undefined;
        expect(shouldReuseInitialTransaction(transaction, true, 0, false, [transaction])).toBe(true);
    });

    test('returns false for first file when multi-scan enabled and receipt already exists', () => {
        const transaction = createRandomTransaction(1);
        transaction.receipt = { source: 'file://receipt.png' };
        expect(shouldReuseInitialTransaction(transaction, true, 0, true, [transaction])).toBe(false);
    });
});

UI Tests for Multi-Scan Flow

File: tests/ui/IOURequestStepScanTest.tsx (new)

Test the critical user flows using @testing-library/react-native:

describe('IOURequestStepScan', () => {
    beforeAll(() => {
        Onyx.init({ keys: ONYXKEYS });
    });

    // Mock react-native-vision-camera and react-webcam
    jest.mock('react-native-vision-camera', () => ({ useCameraDevice: jest.fn() }));

    test('replacing receipt in multi-scan does not clear other drafts', async () => {
        // Given: Two draft transactions exist (simulating multi-scan)
        await Onyx.merge(`${ONYXKEYS.COLLECTION.TRANSACTION_DRAFT}tx1`, {
            ...createRandomTransaction(1),
            receipt: { source: 'file://receipt1.png' },
        });
        await Onyx.merge(`${ONYXKEYS.COLLECTION.TRANSACTION_DRAFT}tx2`, {
            ...createRandomTransaction(2),
            receipt: { source: 'file://receipt2.png' },
        });

        // When: User replaces first receipt (via backTo flow)
        render(<IOURequestStepScan route={{ params: { backTo: '/some/route', ... } }} />);

        // Simulate file selection...

        // Then: Second draft transaction should still exist
        const tx2 = await getOnyxValue(`${ONYXKEYS.COLLECTION.TRANSACTION_DRAFT}tx2`);
        expect(tx2).toBeDefined();
        expect(tx2?.receipt?.source).toBe('file://receipt2.png');
    });

    test('multi-scan mode preserves receipts when adding new ones', async () => {
        // Test that adding receipts in multi-scan mode doesn't clear existing ones
    });
});

Test Coverage Priority

  1. Immediate: shouldReuseInitialTransaction unit tests - directly addresses the bug
  2. High Priority: UI test for "replacing receipt doesn't clear other drafts" - regression test for [PAY][$250] Multi scan - After replacing receipt, other receipt disappears, or confirm page becomes broken #79324
  3. Later (Phase 2+): useReceiptScan hook tests after extraction

Phase 2: Extract Shared Business Logic into a Custom Hook

Create a new hook useReceiptScan that encapsulates all the shared logic between web and native:

File: src/pages/iou/request/step/IOURequestStepScan/useReceiptScan.ts

This hook should contain:

  • All Onyx subscriptions (useOnyx calls)
  • Transaction management logic (transactions, optimisticTransactions)
  • Navigation callbacks (navigateToConfirmationStep, navigateToConfirmationPage, navigateBack)
  • Receipt processing logic (createTransaction, submitReceipts)
  • Multi-scan state management (move state here instead of parent component)
  • Location permission flow state
// Signature (approximate)
function useReceiptScan(params: {
    reportID: string;
    initialTransactionID: string;
    iouType: IOUType;
    action: IOUAction;
    backTo?: string;
    backToReport?: boolean;
    report: OnyxEntry<Report>;
    initialTransaction: OnyxEntry<Transaction>;
}) {
    // ... all shared state and callbacks
    return {
        // State
        isMultiScanEnabled,
        setIsMultiScanEnabled,
        receiptFiles,
        setReceiptFiles,
        shouldSkipConfirmation,
        transactions,
        // ... other state
        
        // Callbacks
        navigateToConfirmationStep,
        submitReceipts,
        updateScanAndNavigate,
        toggleMultiScan,
        // ... other callbacks
    };
}

Phase 3: Consolidate isMobile() Checks and Fix Capability Detection

The isMobile() function from @libs/Browser is the correct approach for distinguishing mobile web from desktop web at runtime (per CROSS-PLATFORM.md). This cannot be done with file extensions since both are "web" at build time.

However, the current implementation has issues:

Issue A: Scattered checks - isMobile() is called ~10 times throughout index.tsx:

  • Lines 193, 280, 935, 997, 1029, 1059, 1061, 1121, 1123, 1124

Refactored approach: Call isMobile() once at the top level and use composition to render either a mobile camera view or desktop upload view:

const isMobileWeb = isMobile();

return (
    <StepScreenDragAndDropWrapper ...>
        {isMobileWeb ? (
            <MobileCameraView ... />
        ) : (
            <DesktopUploadView ... />
        )}
    </StepScreenDragAndDropWrapper>
);

Issue B: Wrong abstraction for drag-and-drop - The code uses isSmallScreenWidth as a proxy:

// we need to use isSmallScreenWidth instead of shouldUseNarrowLayout because drag and drop is not supported on mobile
// eslint-disable-next-line rulesdir/prefer-shouldUseNarrowLayout-instead-of-isSmallScreenWidth
const {isSmallScreenWidth} = useResponsiveLayout();

This conflates screen size with capability. Instead, create a proper capability abstraction using platform-specific files:

// useDragAndDropSupport/index.ts (web - real implementation)
import {isMobile} from '@libs/Browser';

function useDragAndDropSupport() {
    // Drag and drop works well on desktop web only
    // Mobile web browsers technically support it but the UX is poor
    return !isMobile();
}

export default useDragAndDropSupport;
// useDragAndDropSupport/index.native.ts (native - no-op, always false)
function useDragAndDropSupport() {
    // Native apps don't support drag-and-drop for file upload
    return false;
}

export default useDragAndDropSupport;

This follows the platform pattern from CROSS-PLATFORM.md: index.ts contains the real web implementation, index.native.ts provides the native override (a no-op in this case). This makes the intent explicit and removes the misleading use of isSmallScreenWidth.

Phase 4: Fix ESLint Suppressions for React Compiler Compatibility

Issue 1 (line 276-277): Mount-only effect that checks if scan files can be read

useEffect(() => {
    // ... scan file validation logic
    // eslint-disable-next-line react-hooks/exhaustive-deps
}, []);

Fix: Extract to a proper initialization hook with explicit dependency tracking:

const hasInitialized = useRef(false);
useEffect(() => {
    if (hasInitialized.current) return;
    hasInitialized.current = true;
    // ... validation logic
}, [setIsMultiScanEnabled, transactions]);

Issue 2 (line 301-302): Camera permission effect that only runs on mount/tab focus

useEffect(() => {
    if (!isMobile() || !isTabActive) {
        // ...
    }
    // eslint-disable-next-line react-hooks/exhaustive-deps
}, [isTabActive]);

Fix: Include all dependencies and move the mobile check outside the effect body:

// Since this effect only applies to mobile web, it belongs in MobileCameraView
// (after Phase 4 composition refactor). For now, include all deps:
useEffect(() => {
    if (!isMobileWeb || !isTabActive) {
        setVideoConstraints(undefined);
        return;
    }
    // permission query logic...
}, [isMobileWeb, isTabActive, requestCameraPermission]);

Note: After Phase 4, this effect will live in MobileCameraView.tsx where isMobileWeb is implicitly true, eliminating the need for the check entirely.

Phase 5: Separate Concerns via Composition

Instead of a single monolithic component controlled by flags, split into focused components:

IOURequestStepScan/
├── index.tsx                    # Web: Chooses between Desktop/Mobile views
├── index.native.tsx             # Native: Camera view
├── useReceiptScan.ts            # Shared business logic hook
├── components/
│   ├── DesktopUploadView.tsx    # Desktop file upload UI (web only)
│   └── MobileCameraView.tsx     # Mobile camera UI (web only)
├── useDragAndDropSupport/
│   ├── index.ts                 # Web: real impl using isMobile()
│   └── index.native.ts          # Native: no-op (returns false)
├── ReceiptPreviews/             # Existing (no changes needed)
├── CameraPermission/            # Existing (good pattern - native vs web)
├── LocationPermission/          # Existing (good pattern - native vs web)
└── types.ts                     # Simplified - remove isStartingScan

Phase 6: Move Multi-Scan State to Hook and Use key for Reset

Remove the prop drilling pattern where IOURequestStartPage manages multi-scan state.

Current (IOURequestStartPage.tsx:91):

const [isMultiScanEnabled, setIsMultiScanEnabled] = useState((optimisticTransactions ?? []).length > 1);
// ... passed as props to IOURequestStepScan

Refactored: The useReceiptScan hook manages state internally. To reset state when tabs change, use React's key prop to force a remount - this is the idiomatic declarative pattern:

// In IOURequestStartPage - use selectedTab as the key
<TopTab.Screen name={CONST.TAB_REQUEST.SCAN}>
    {() => (
        <TabScreenWithFocusTrapWrapper>
            <IOURequestStepScan
                key={selectedTab}  // Forces remount when tab changes
                route={route}
                navigation={navigation}
                // No more isMultiScanEnabled/setIsMultiScanEnabled props needed
            />
        </TabScreenWithFocusTrapWrapper>
    )}
</TopTab.Screen>

When selectedTab changes:

  1. React unmounts the old IOURequestStepScan instance
  2. A fresh instance mounts with all state reset to initial values
  3. The useReceiptScan hook re-initializes, deriving isMultiScanEnabled from current optimisticTransactions
  4. All mount effects run fresh

This eliminates the need for:

  • isMultiScanEnabled prop
  • setIsMultiScanEnabled prop
  • isStartingScan prop (can be derived from route/context)
  • Any imperative reset logic

Migration Notes

Key Files to Modify

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~022014513414347273472
  • Upwork Job ID: 2014513414347273472
  • Last Price Increase: 2026-03-15
  • Automatic offers:
    • samranahm | Contributor | 110213564

Metadata

Metadata

Labels

Awaiting PaymentAuto-added when associated PR is deployed to productionBugSomething is broken. Auto assigns a BugZero manager.ExternalAdded to denote the issue can be worked on by a contributorReviewingHas a PR in reviewWeeklyKSv2

Type

No type

Projects

Status

HIGH

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions