DRY MoneyRequestHeader/MoneyReportHeader and enable React Compiler#80276
Closed
roryabraham wants to merge 31 commits intomainfrom
Closed
DRY MoneyRequestHeader/MoneyReportHeader and enable React Compiler#80276roryabraham wants to merge 31 commits intomainfrom
roryabraham wants to merge 31 commits intomainfrom
Conversation
Transform MoneyRequestHeaderStatusBar from a presentational component to a smart component that handles its own Onyx subscriptions and status computation logic. This extracts the duplicated status bar logic from MoneyRequestHeader and MoneyReportHeader. Changes: - Accept identifying props (transactionID, reportID, policyID, etc.) - Add internal Onyx subscriptions for transaction, report, policy - Move status computation logic from parent components - Return null when no status bar should be shown - Internalize getStatusIcon helper function
Add promise-based API for DecisionModal following the same pattern as useConfirmModal. This preserves DecisionModal's distinct styling and behavior while enabling imperative usage. Changes: - Create DecisionModalWrapper component to bridge DecisionModal with global modal system - Create useDecisionModal hook with showDecisionModal method - Add onModalHide prop to DecisionModal to support wrapper pattern - Export DecisionModalActions for clarity on return values
Add promise-based wrappers for HoldSubmitterEducationalModal and HoldOrRejectEducationalModal to integrate them with the global modal system. These wrappers intercept callbacks to signal completion. Changes: - Create HoldSubmitterEducationalModalWrapper - Create HoldOrRejectEducationalModalWrapper - Both wrappers intercept onClose and onConfirm to resolve with 'CONFIRMED' action - Handles FeatureTrainingModal's internal visibility management
Add promise-based hook for educational modals with internal NVP management. This hook encapsulates the logic for showing and dismissing educational modals based on user state. Changes: - Create useHoldEducationalModal hook - Internally subscribes to NVPs for hold/reject dismissal state - Provides showSubmitterEducationalModal for submitters - Provides showApproverEducationalModal for approvers - Provides unified showEducationalModalIfNeeded method - Automatically updates NVPs after confirmation - Returns dismissal state for advanced use cases
Extract duplicate transaction logic into reusable hook. This hook encapsulates the throttled button state and duplicateTransaction callback with all necessary Onyx subscriptions. Changes: - Create useDuplicateExpenseAction hook - Encapsulate useThrottledButtonState for isDuplicateActive - Provide duplicateTransaction callback - Handle all related Onyx subscriptions (allPolicyCategories, policyRecentlyUsedCurrencies, etc.) - Maintains same behavior as inline implementation
Major refactoring to use promise-based modals and extracted hooks, eliminating significant code duplication. Changes: - Replace manual status bar computation with smart MoneyRequestHeaderStatusBar component - Use useDuplicateExpenseAction hook for duplicate logic - Use useHoldEducationalModal hook for educational modals with promise-based pattern - Use useConfirmModal for delete confirmation - Remove local modal state (isDeleteModalVisible, isHoldEducationalModalVisible, rejectModalAction) - Remove modal JSX from component tree - Update HOLD and REJECT actions to use async/await with showEducationalModalIfNeeded - Update DELETE action to use promise-based showConfirmModal - Simplify imports by removing unused dependencies
Add imports for new promise-based modal hooks and duplicate expense hook. This prepares MoneyReportHeader for similar refactoring as MoneyRequestHeader. Changes: - Add useDecisionModal import - Add useHoldEducationalModal import - Add useDuplicateExpenseAction import - Remove useThrottledButtonState import (will use from hook) - Remove duplicateExpenseTransaction import (will use from hook) Note: Full refactoring of MoneyReportHeader secondary actions and modal state management remains to be done following the same pattern as MoneyRequestHeader.
Refactor the modal system to support custom action types per modal wrapper implementation, eliminating the need for type assertions. Changes in ModalContext: - Remove global ModalActions constant (now defined per wrapper) - Make ModalStateChangePayload generic with string constraint - Update ModalProps to be generic with action type parameter - Remove ModalActions export Changes in ConfirmModalWrapper: - Define ConfirmModalActions locally - Define ConfirmModalAction type - Use typed ModalProps<ConfirmModalAction> - Export ConfirmModalActions Changes in DecisionModalWrapper: - Define DecisionModalActions locally - Define DecisionModalAction type - Use typed ModalProps<DecisionModalAction> - Export DecisionModalActions - Remove isSmallScreenWidth prop (now computed internally in DecisionModal) Changes in DecisionModal: - Add useResponsiveLayout to compute isSmallScreenWidth internally - Remove isSmallScreenWidth from props Changes in hooks: - useConfirmModal: Import and re-export ConfirmModalActions, return typed promise - useDecisionModal: Import and re-export DecisionModalActions, return typed promise Changes in MoneyRequestHeader: - Import ConfirmModalActions from useConfirmModal - Add eslint suppression for ConfirmModalActions.CONFIRM access - Remove unused imports (useState, Transaction, useDecisionModal) Changes in MoneyReportHeader: - Import ConfirmModalActions from useConfirmModal - Remove DecisionModal isSmallScreenWidth prop usages - Add eslint suppressions for ConfirmModalActions.CONFIRM access - Remove unused new hook imports (will be used in future refactoring) Changes in MoneyRequestHeaderStatusBar: - Remove useMemo wrapper from status computation - Use lazy-loaded Expensify icons instead of namespace import - Simplify to use separate statusIcon and statusDescription variables
Remove old callback-based educational modal pattern and use the new useHoldEducationalModal hook for cleaner, async/await flow. Changes: - Remove HoldSubmitterEducationalModal and HoldOrRejectEducationalModal imports - Remove ModalActions import - Add useHoldEducationalModal hook - Remove state variables (isHoldEducationalModalVisible, rejectModalAction) - Remove NVP subscriptions (dismissedRejectUseExplanation, dismissedHoldUseExplanation) - Update HOLD action to use showEducationalModalIfNeeded with async/await - Update REJECT action to use showEducationalModalIfNeeded with async/await - Remove dismissModalAndUpdateUseHold callback - Remove dismissRejectModalBasedOnAction callback - Remove educational modal JSX from component tree - Remove isSmallScreenWidth prop from DecisionModal instances
Migrate MoneyReportHeader and SearchPage to use promise-based educational modals, eliminating all remaining lint suppressions and type errors. Changes in MoneyReportHeader: - Add useHoldEducationalModal hook - Remove educational modal imports - Remove state variables (isHoldEducationalModalVisible, rejectModalAction) - Remove NVP subscriptions (dismissedRejectUseExplanation, dismissedHoldUseExplanation) - Update HOLD action to use async showEducationalModalIfNeeded - Update REJECT and REJECT_BULK actions to use async showEducationalModalIfNeeded - Remove dismissModalAndUpdateUseHold and dismissRejectModalBasedOnAction callbacks - Remove educational modal JSX from render - Update MoneyRequestHeaderStatusBar usage to smart component pattern - Remove getStatusBarProps and getStatusIcon functions (now in smart component) - Remove unused imports (IconAsset, BrokenConnectionDescription, variables, dismissRejectUseExplanation, setNameValuePair, isProcessingReport, getArchiveReason, archiveReason, isArchivedReport) - Remove isSmallScreenWidth props from DecisionModal instances Changes in SearchPage: - Add useHoldEducationalModal hook - Remove educational modal imports and ModalActions import - Remove state variables (isHoldEducationalModalVisible, rejectModalAction) - Remove NVP subscriptions - Update HOLD and REJECT bulk actions to use async showEducationalModalIfNeeded - Remove dismiss callbacks - Remove educational modal JSX - Remove isSmallScreenWidth props from DecisionModal instances Changes in educational modal wrappers: - Add TODO comments explaining their purpose as bridge components - Document future migration path to remove wrappers Changes in type system: - Remove all unsafe member access lint suppressions - Generic types properly infer through the call chain
Changes: - Import ConfirmModalActions from useConfirmModal - Replace ModalActions.CONFIRM with ConfirmModalActions.CONFIRM - Remove unused imports (setNameValuePair, dismissRejectUseExplanation) - Remove isSmallScreenWidth (now computed inside DecisionModal) - Remove outdated comment about isSmallScreenWidth - Update useMemo dependency array to use showEducationalModalIfNeeded instead of dismissedHoldUseExplanation and dismissedRejectUseExplanation
Update educational modals to accept closeModal directly, eliminating the wrapper layer. This completes the promise-based modal migration. Changes: - Update HoldSubmitterEducationalModal to accept closeModal (ModalProps) - Update HoldOrRejectEducationalModal to accept closeModal (ModalProps) - Both modals now handle closeModal with 'CONFIRMED' action internally - Update useHoldEducationalModal to reference modals directly - Delete HoldSubmitterEducationalModalWrapper.tsx (no longer needed) - Delete HoldOrRejectEducationalModalWrapper.tsx (no longer needed) - Fix import path in useHoldEducationalModal to use relative import This follows the same pattern as ConfirmModal, where wrappers serve as temporary bridges during migration. Since all usages now go through useHoldEducationalModal, we can eliminate the wrapper layer entirely.
Simplify the hook by avoiding method destructuring and removing unused return values. Changes: - Use context.showModal instead of destructuring to avoid lint error about unbound methods - Remove unused isDismissedHoldExplanation and isDismissedRejectExplanation from return (NVP checks are handled internally)
Remove duplicate imports and use relative paths for hook imports per lint rules. Changes: - Use relative imports for all hooks (useDefaultExpensePolicy, useOnyx, usePermissions, useThrottledButtonState) - Remove duplicate imports that were accidentally added
Define specific action types for educational modals using ValueOf pattern, matching the approach used in ConfirmModalWrapper and DecisionModalWrapper. Changes: - Add HoldSubmitterEducationalModalActions constant with CONFIRMED action - Define HoldSubmitterEducationalModalAction type using ValueOf - Type HoldSubmitterEducationalModalProps with ModalProps<HoldSubmitterEducationalModalAction> - Use constant instead of string literal in handleClose - Export actions and action type for consistency - Add HoldOrRejectEducationalModalActions constant with CONFIRMED action - Define HoldOrRejectEducationalModalAction type using ValueOf - Type HoldOrRejectEducationalModalProps with ModalProps<HoldOrRejectEducationalModalAction> - Use constant instead of string literal in handleClose - Export actions and action type for consistency This ensures type safety and makes the action values explicit rather than using generic string literals.
Since MoneyRequestHeader is compiled by React Compiler, manual memoization is unnecessary and can be removed. Changes: - Remove useCallback and useMemo from imports - Remove useCallback wrapper from markAsCash function - Remove useMemo wrapper from primaryAction computation - Remove useMemo wrapper from secondaryActions computation - Convert to inline ternary expressions for cleaner code React Compiler automatically optimizes these computations.
Replace simple acknowledgement DecisionModals with ConfirmModal using shouldShowCancelButton: false, which is more semantically appropriate. Changes: - Remove DecisionModal import (not needed) - Remove useDecisionModal import - Remove 3 DecisionModal JSX instances from render - Convert onExportFailed and onExportOffline callbacks to use showConfirmModal with shouldShowCancelButton: false - Inline the modal calls directly where needed (download error and offline prompts) - Update useMemo dependency to include showConfirmModal These were simple acknowledgement modals with only a confirm button, so ConfirmModal is the correct semantic choice.
Replace the single clearSelectedTransactions function with two specialized functions to eliminate unnecessary dependencies and enable React Compiler compatibility. Root cause: The old clearSelectedTransactions had searchContextData.selectedTransactions in its dependencies, even when called with boolean parameter (which doesn't read that value). This caused the function reference to change whenever selections changed, creating infinite loops in useEffect. Solution (Option 1): Split into two functions with appropriate dependencies: - clearAllSelectedTransactions() - stable, only depends on setSelectedTransactions - clearSelectedTransactionsByHash(hash, shouldTurnOff) - has searchContextData dependencies for hash-based logic Changes in SearchContext: - Create clearAllSelectedTransactions with stable dependencies - Create clearSelectedTransactionsByHash with searchContextData deps - Remove old clearSelectedTransactions function - Update context value and useMemo dependencies Changes in types.ts: - Replace clearSelectedTransactions overloaded type with two separate function signatures - Add documentation for each function's purpose Changes across 15 files (call site migration): - clearSelectedTransactions(true) → clearAllSelectedTransactions() - clearSelectedTransactions() → clearSelectedTransactionsByHash() - clearSelectedTransactions(hash) → clearSelectedTransactionsByHash(hash) - clearSelectedTransactions(hash, flag) → clearSelectedTransactionsByHash(hash, flag) - Updated all destructuring from useSearchContext() - Updated all useEffect dependency arrays Critical fix in MoneyReportHeader: - Removed eslint-disable comment from useEffect - Now safely depends on stable clearAllSelectedTransactions - No more infinite loop risk This enables MoneyReportHeader to be compiled by React Compiler in the future.
Remove all useMemo and useCallback wrappers from MoneyReportHeader to make it fully React Compiler compatible. Changes: - Remove useMemo and useCallback from React imports - Remove 30 useMemo/useCallback wrappers total - Convert to inline expressions, ternaries, or direct assignments - No IIFE patterns used - all logic is inline - Keep useMemoizedLazyExpensifyIcons (lazy loading pattern, not manual memoization) Conversions made: - exportTemplates: Remove useMemo wrapper - requestParentReportAction: Convert to ternary expression - transactions: Inline Object.values() - isBlockSubmitDueToStrictPolicyRules: Inline function call - isExported: Inline function call - integrationNameFromExportMessage: Convert to ternary - hasOnlyPendingTransactions: Inline boolean expression - transactionIDs: Inline map - messagePDF: Convert to if-else statements (no IIFE) - hasAllPendingRTERViolations: Inline function call - getCanIOUBePaid: Remove useCallback - showExportProgressModal: Remove useCallback - beginExportWithTemplate: Remove useCallback - canIOUBePaid: Inline getCanIOUBePaid() call - onlyShowPayElsewhere: Inline conditional call - shouldShowApproveButton: Inline boolean logic - confirmPayment: Remove useCallback - markAsCash: Remove useCallback - duplicateExpenseTransaction: Remove useCallback - primaryAction: Inline getReportPrimaryAction() call - confirmExport: Remove useCallback - addExpenseDropdownOptions: Inline getAddExpenseDropdownOptions() call - exportSubmenuOptions: Remove useMemo, use direct object (no IIFE) - secondaryActions: Remove useMemo, use conditional assignment - secondaryExportActions: Remove useMemo, use conditional assignment - unapproveWarningText: Remove useMemo, inline JSX - showDeleteModal: Remove useCallback - showExportAgainModal: Remove useCallback - selectedTransactionsOptions: Remove useMemo, inline map call MoneyReportHeader is now fully React Compiler compatible with zero manual memoization.
Remove all useMemo and useCallback wrappers from MoneyReportHeader to make it fully React Compiler compatible. Changes: - Remove useMemo and useCallback from React imports - Remove 30 useMemo/useCallback wrappers total - Convert to inline expressions, ternaries, or direct assignments - No IIFE patterns used - all logic is inline - Keep useMemoizedLazyExpensifyIcons (lazy loading pattern, not manual memoization) Conversions made: - exportTemplates: Remove useMemo wrapper - requestParentReportAction: Convert to ternary expression - transactions: Inline Object.values() - isBlockSubmitDueToStrictPolicyRules: Inline function call - isExported: Inline function call - integrationNameFromExportMessage: Convert to ternary - hasOnlyPendingTransactions: Inline boolean expression - transactionIDs: Inline map - messagePDF: Convert to if-else statements (no IIFE) - hasAllPendingRTERViolations: Inline function call - getCanIOUBePaid: Remove useCallback - showExportProgressModal: Remove useCallback - beginExportWithTemplate: Remove useCallback - canIOUBePaid: Inline getCanIOUBePaid() call - onlyShowPayElsewhere: Inline conditional call - shouldShowApproveButton: Inline boolean logic - confirmPayment: Remove useCallback - markAsCash: Remove useCallback - duplicateExpenseTransaction: Remove useCallback - primaryAction: Inline getReportPrimaryAction() call - confirmExport: Remove useCallback, add exportType parameter - addExpenseDropdownOptions: Inline getAddExpenseDropdownOptions() call - exportSubmenuOptions: Remove useMemo, use direct object (no IIFE) - secondaryActions: Remove useMemo, use conditional assignment - secondaryExportActions: Remove useMemo, use conditional assignment - unapproveWarningText: Remove useMemo, inline JSX - showDeleteModal: Remove useCallback, convert to async/await - showExportAgainModal: Remove useCallback, accept exportType param, convert to async/await - selectedTransactionsOptions: Remove useMemo, inline map call Fix useEffect anti-pattern (per React docs): - Remove state → Effect chain for export modals - Move showExportAgainModal definition before first usage - Remove exportModalStatus state (unused after refactoring) - Remove useEffect that watched exportModalStatus - Event handlers now call showExportAgainModal(exportType) directly - showExportAgainModal passes exportType to confirmExport - Eliminates unnecessary re-renders from state changes MoneyReportHeader is now fully React Compiler compatible with zero manual memoization and proper event-driven architecture.
Reduce --max-warnings from 383 to 378 (5 fewer warnings) as a result of enabling React Compiler for MoneyRequestHeader, MoneyReportHeader, and SearchContext. The refactoring eliminated manual memoization that was causing React Compiler warnings, resulting in cleaner code and fewer lint warnings overall.
|
npm has a |
Update files that were importing ModalActions from the old location to use ConfirmModalActions from useConfirmModal instead. Changes: - DomainAdminDetailsPage: Import ConfirmModalActions from useConfirmModal - PureReportActionItem: Import ConfirmModalActions from useConfirmModal - IOURequestStepConfirmation: Import ConfirmModalActions from useConfirmModal - IOURequestStepWaypoint: Import ConfirmModalActions from useConfirmModal - All files: Update ModalActions.CONFIRM → ConfirmModalActions.CONFIRM - Combine duplicate imports into single import statements This fixes lint errors introduced when we made modal actions generic and moved ModalActions definitions from ModalContext to individual modal wrappers.
- Remove CONFLICTING_RULES.md and non-compiling-files.json from tracking - Reset Mobile-Expensify submodule to match main
roryabraham
force-pushed
the
Rory-DRYMoneyRequestView
branch
from
6833a5a to
146e3f2
Compare
Contributor
Author
|
This PR has lots of good stuff but is too big and must be split up |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Explanation of Change
This PR:
MoneyRequestHeader,MoneyReportHeader, andSearchContextReact Compiler compliant.useMemo/useCallbackfrom above componentsclearSelectedTransactionsinto two functions with appropriate dependenciesMoneyRequestHeaderandMoneyReportHeaderMoneyRequestHeaderStatusBarto handle its own subscriptions per the guidelines in [No QA] ai-review: rule for composition over configuration #80222useDuplicateExpenseActionhook to extract duplicate transaction logicuseHoldEducationalModalhook with promise-based educational modalsFixed Issues
(partial) #68765
Tests
Hold/Reject Functionality:
Delete Expense:
Duplicate Expense:
Status Bar:
Export Modal:
Offline tests
Same as Tests
QA Steps
Same as Tests
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps./** comment above it */thisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari