-
Notifications
You must be signed in to change notification settings - Fork 3.6k
Description
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:
- Configuration Overload: Three props (
isMultiScanEnabled,isStartingScan,setIsMultiScanEnabled) control behavior modes, making the component hard to reason about - Scattered Mobile Web Checks:
isMobile()is called ~10 times throughoutindex.tsxinstead of once at the top level - Incorrect Abstractions: Using
isSmallScreenWidthas a proxy for drag-and-drop capability - React Compiler Blockers: 2 suppressed
exhaustive-depsESLint rules prevent React Compiler adoption - Massive Duplication: ~2200 lines total across
index.tsxandindex.native.tsxwith 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
- Immediate:
shouldReuseInitialTransactionunit tests - directly addresses the bug - 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
- Later (Phase 2+):
useReceiptScanhook 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 (
useOnyxcalls) - 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 IOURequestStepScanRefactored: 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:
- React unmounts the old
IOURequestStepScaninstance - A fresh instance mounts with all state reset to initial values
- The
useReceiptScanhook re-initializes, derivingisMultiScanEnabledfrom currentoptimisticTransactions - All mount effects run fresh
This eliminates the need for:
isMultiScanEnabledpropsetIsMultiScanEnabledpropisStartingScanprop (can be derived from route/context)- Any imperative reset logic
Migration Notes
- This is a large refactor that should be done incrementally
- Start with Phase 1 (tests) to establish a safety net before refactoring
- Each phase can be a separate PR to reduce review burden
- Unit tests for
shouldReuseInitialTransactionare the immediate regression guard for [PAY][$250] Multi scan - After replacing receipt, other receipt disappears, or confirm page becomes broken #79324
Key Files to Modify
- src/pages/iou/request/step/IOURequestStepScan/index.tsx - Main web implementation
- src/pages/iou/request/step/IOURequestStepScan/index.native.tsx - Native implementation
- src/pages/iou/request/step/IOURequestStepScan/types.ts - Remove
isStartingScan,setIsMultiScanEnabledprops - src/pages/iou/request/IOURequestStartPage.tsx - Remove multi-scan state management
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
Type
Projects
Status