Skip to content

Consolidate DecisionModals into a global component #80285

Open
2 of 4 issues completed
Open

Description

@roryabraham

Following up on #68799 with another common modal type.

Problem

When every component mounts its own DecisionModal in the component tree, it creates component bloat, memory overhead from multiple modal instances, and repetitive boilerplate code that makes the application harder to maintain.

Solution

Lift a single DecisionModal instance up in the component tree so that it can be shared across the app. Child components can use context to show and hide the shared DecisionModal with whatever props they want to pass to it. Internally, the state of the DecisionModal is a stack (allowing for multi-step confirmations, one of our UI requirements), and that state is managed through React Context.

Activity

  1. added
    BugSomething is broken. Auto assigns a BugZero manager.
    on Jan 22, 2026
  2. melvin-bot commented on Jan 22, 2026

    @melvin-bot

    Triggered auto assignment to @JmillsExpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

  3. roryabraham commented on Jan 23, 2026

    @roryabraham
    ContributorAuthor

    Going to try taking the quicker/cheaper path here and do this refactoring in one go, rather than in many incremental PRs as we have done for ConfirmModal, relying on Applause for pre-merge regression testing. I understand it makes the single PR riskier, but:

    • we haven't really seen issues rolling this change out
    • it seemed to work fine with my local smoke tests
    • if this works out it's much faster and cheaper
  4. roryabraham commented on Jan 26, 2026

    @roryabraham
    ContributorAuthor

    QA ongoing - one issue reported but it seems like expected behavior

  5. 29 remaining items

  6. roryabraham commented on Sep 16, 2026

    @roryabraham
    ContributorAuthor

    @ZhenjaHorbach is actively finishing up ConfirmModal migrations in #100076, then will start on this.

  7. ZhenjaHorbach commented on Oct 5, 2026

    @ZhenjaHorbach
    Contributor

    @MelvinBot
    Analyze all DecisionModals and check which modals can be migrated

  8. MelvinBot commented on Oct 5, 2026

    @MelvinBot
    Contributor

    DecisionModal migration audit

    The global infrastructure already exists (useDecisionModal, DecisionModalWrapper, useHoldMenuModal). There are 18 inline DecisionModals left: 16 <DecisionModal> elements plus 2 <ProcessMoneyReportHoldMenu> call sites. You can migrate 16 of them now. The 2 domain "Close account" modals need a wrapper change first.

    Easy: migrate now (10)

    None of these has a first option or an onModalHide, and all have static text. Each one becomes showDecisionModal({title, prompt, secondOptionText}), and you can delete the isVisible state.

    Modal Location
    Download failed WorkspaceMembersPage.tsx:829
    Download failed WorkspacePerDiemPage.tsx:414
    Download failed WorkspaceCategoriesPage.tsx:712
    Download failed WorkspaceTagsPage.tsx:824
    Offline DynamicWorkspaceExpensifyCardDetailsPage.tsx:505
    Offline SearchTransactionsChangeReport.tsx:380
    Invalid file BaseImportOnyxState.tsx:56. Visibility comes from props, so call showDecisionModal in the .catch of both index.tsx and index.native.tsx
    Download failed / Offline SelectionToolbar/index.tsx:309-326 (2 modals). This also fixes a bug: on wide layouts the toolbar unmounts after a CSV export starts, so a later failure modal never appears
    Statement PDF: one feed at a time SearchBulkActionsButton.tsx:311

    Needs care: migrate with small extra work (6)

    Modal Location What to watch
    Download failed ImportTagsOptionsPage.tsx:298 It opens from inside a global ConfirmModal. Keep the close(() => showDecisionModal(...)) wrapper so the ConfirmModal closes first, instead of reappearing after "OK"
    Offline WorkspaceCompanyCardsPage.tsx:153 useAssignCard takes a state setter. Change it to an onOfflineAssign callback, and update tests/unit/hooks/useAssignCard.test.ts
    Offline / Download failed SearchBulkActionsButton.tsx:277-294 (2 modals) State lives in useSearchBulkActions (about 20 setter sites) and is asserted in 4 test files. Pass an explicit prompt at each download-failed site. Today emptyReportsCount can be stale and show the wrong message
    Hold menu (pay) ProcessMoneyReportHoldMenu.tsx:79, used by SelectionToolbar and ReportPreviewHoldMenu This is an older, pay-only copy of HoldMenuModalWrapper. Replace both callers with useHoldMenuModal().showHoldMenu, as MoneyReportHeaderSelectionDropdown already does. Then delete ProcessMoneyReportHoldMenu, ReportPreviewHoldMenu, and the preview hold-menu context

    Blocked: needs a wrapper change first (2)

    Modal Location
    Close account (force / safe) DomainMemberDetailsPage.tsx:242, DomainMembersPage.tsx:359

    Today, dismissing these modals with the backdrop, Escape, or Back does nothing. The wrapper maps dismiss to the second option (DecisionModalWrapper.tsx:47). That makes dismiss look the same as "Safe close". After a naive migration, dismissing would open the close-account confirmation. Cancelling it reopens the decision modal (DomainMemberDetailsPage.tsx:107-110), so the user would be stuck in a loop.

    Fix: have the wrapper return a distinct action on dismiss, for example a new ModalActions.DISMISS. Then migrate these pages as an async flow that awaits the decision and then the confirmation.

    Cross-cutting notes
    • Shared helper: 9 of these modals are the same "Offline" or "Download failed" alert. The same bodies are already duplicated in useExportActions.ts:114-128 and MoneyReportHeaderModals.tsx:71-85. A small hook that returns showOfflineModal and showDownloadFailedModal would remove those copies.
    • Behavior change: useDecisionModal defaults to shouldHandleNavigationBack: true, so browser Back will close these alerts. None of the inline modals set this today. It matches the modals that are already migrated.
    • Memo deps: several triggers sit inside useMemo menu-item arrays. Add showDecisionModal to their deps.
    • Cleanup: most files can also drop their isSmallScreenWidth / useResponsiveLayout usage, which only existed for the modal.
    • Not verified at runtime: the SelectionToolbar and SearchBulkActions "modal never appears after unmount" bugs come from reading the code only.

    Next Steps: Reply with @MelvinBot <your feedback> to refine this, or @MelvinBot implement the Easy group to open a draft PR.


    view run

  9. ZhenjaHorbach commented on Oct 5, 2026

    @ZhenjaHorbach
    Contributor

    @MelvinBot
    Can you create 4 sub-issues for migrating these modals?
    With approximately the same migration complexity

  10. MelvinBot commented on Oct 5, 2026

    @MelvinBot
    Contributor

    I created 4 sub-issues and linked them to this issue. They cover all 18 modals and are split so each is about the same amount of work:

    Sub-issue Modals Main effort
    [Batch 1] Refactor DecisionModal usage to useDecisionModal in simple alert modals (8 modals) 8 Many files, each a simple one-button alert. Adds a shared "Offline" / "Download failed" helper
    [Batch 2] Refactor DecisionModal usage to useDecisionModal in Search bulk actions and SelectionToolbar alerts (5 modals) 5 Large useSearchBulkActions hook and 5 test files
    [Batch 3] Replace ProcessMoneyReportHoldMenu with useHoldMenuModal in SelectionToolbar and report preview (2 modals) 2 Moves two hold-menu callers to useHoldMenuModal, then deletes the old component and the preview context
    [Batch 4] Add dismiss action to DecisionModalWrapper and migrate domain close-account and company cards modals (3 modals) 3 Adds a dismiss result to DecisionModalWrapper and changes the useAssignCard API

    All four are assigned to ZhenjaHorbach and roryabraham with the Daily label, the same as the ConfirmModal batches. Batch 4 must add the wrapper's dismiss action before it migrates the domain pages.


    view run

  11. roryabraham commented on Oct 20, 2026

    @roryabraham
    ContributorAuthor

    @MelvinBot what is the status of this issue and its sub-issues?

  12. MelvinBot commented on Oct 20, 2026

    @MelvinBot
    Contributor

    One of the 4 sub-issues is done, one is in review, and two have not started. The parent issue stays open until all 18 modals are migrated.

    Sub-issue Status
    [$175] [Batch 1] Refactor DecisionModal usage to useDecisionModal in simple alert modals (8 modals) In review. Migrate batch 1 one-button alerts from DecisionModal to useDecisionModal has ZhenjaHorbach's approval, but your approval was dismissed. The PR has merge conflicts again.
    [Due for payment 2026-10-23] [$175] [Batch 2] Refactor DecisionModal usage to useDecisionModal in Search bulk actions and SelectionToolbar alerts (5 modals) Done. Use useDecisionModal for Search bulk actions and SelectionToolbar alerts merged on Oct 14 and reached production in 9.5.12-1. Payment is due Oct 23. ZhenjaHorbach asked to close it.
    [Batch 3] Replace ProcessMoneyReportHoldMenu with useHoldMenuModal in SelectionToolbar and report preview (2 modals) Not started. No PR yet.
    [Batch 4] Add dismiss action to DecisionModalWrapper and migrate domain close-account and company cards modals (3 modals) Not started. No PR yet.

    To unblock Batch 1, fix the conflicts and get your re-approval.


    view run

  13. ZhenjaHorbach commented on Nov 5, 2026

    @ZhenjaHorbach
    Contributor

    @MelvinBot
    Can we remove DecisionModal component now?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Labels

BugSomething is broken. Auto assigns a BugZero manager.MonthlyKSv2

Type

No type

Projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions