Skip to content

[$175] [Batch 6] Refactor ConfirmModal usage to useConfirmModal in GPS trip modals (5 modals) #100156

Closed

Description

@MelvinBot

Part of #100076.

Problem

ConfirmModal is deprecated. These five call sites are the GPS trip modals on native. Three are straightforward user-triggered confirms in GPSButtons; the other two are Onyx-driven and are the trickiest instances in the whole migration, because their visibility is derived state and they read GPS data that keeps changing while the modal is open.

Modals in this batch

.../IOURequestStepDistanceGPS/GPSButtons/index.tsx

Instance Notes
:181 Zero-distance acknowledgement. Direct analogue of the already-migrated DiscardGPSTripButton.tsx:33 in this same feature.
:189 "Location required" — shown via the child flow's onDeny when permission is permanently denied. No other modal is open at that moment, so there's no stacking conflict.
:207 Location services disabled. checkSettingsAndPermissions returns before starting the permissions flow, so it can never coexist with the child's modals.

Onyx-driven controllers

Instance Notes
src/components/GPSInProgressModal/index.native.tsx:27 Triggered from a non-React action (libs/actions/HybridApp/index.ts:109), where hooks can't be used — so keep the Onyx flag as the cross-boundary trigger and make this a null-rendering controller. Nothing external flips the flag off, so no auto-hide is needed.
src/components/GPSTripStateChecker/index.native.tsx:145 Hardest instance in this batch — see below.

GPSTripStateChecker specifics

Its isVisible is a four-term derived expression at :146 (showContinueTripModal && isTracking && !isTripFromDifferentUser && splashScreenState === HIDDEN). Three of the four are external state that can flip while the modal is open, and today the modal auto-hides for free. After migration you need an effect that calls closeModal() when any condition goes false.

Two more traps:

  • onCancel is destructive, not a dismissal — onCancel={onViewTrip} at :152 calls stopGpsTrip. The wrapper collapses cancel and backdrop press into a single CLOSE, and the default shouldHandleNavigationBack: true would additionally route Android hardware-back into "stop the trip". Pass shouldHandleNavigationBack: false.
  • tests/unit/components/GPSTripStateCheckerTest.tsx asserts against the page-local render tree and will need updating, since the global modal renders at app root.

Stale-closure risk (both controllers)

getGpsPoints(gpsDraftDetails) currently reads the current render's Onyx value. GPS points keep accumulating into GPS_DRAFT_DETAILS while tracking, so a .then() closure capturing gpsDraftDetails at show time would submit a truncated trip. Read gpsDraftDetails and isOffline through refs (or Onyx.connectWithoutView) inside the continuation. Affects both GPSInProgressModal:22 and GPSTripStateChecker:140.

The controller pattern

For the two Onyx-driven modals, convert to a null-rendering controller calling showConfirmModal() from a useEffect behind a useRef guard. The idiom already exists at PriorityModeController.tsx:63.

Shared gotchas

Apply to every batch — full write-up in this analysis.

  1. Delete onBackdropPress, never forward it. It spreads to BaseModal, which prefers it over onClose, so handleCancel never runs and the promise never resolves.
  2. shouldHandleNavigationBack silently flips undefined → true (useConfirmModal.ts:17).
  3. closeModal() is not id-scoped — it pops whatever is on top (ModalContext.tsx:111) and bug-bot-alerts on an empty stack. Directly relevant to the GPSTripStateChecker auto-hide effect.
  4. Props become a snapshot taken at show time, not a live binding.
  5. Native-only surface, so the wrapper's global ENTER shortcut is inert here.

Acceptance criteria

  • All five instances use useConfirmModal(); no ConfirmModal import remains in these files.
  • A trip resumed after app restart still submits the full set of GPS points, not a truncated set.
  • The continue-trip modal still disappears on its own when tracking stops, the trip changes owner, or the splash screen is showing.
  • Hardware-back no longer stops the trip.
  • Existing relevant tests pass; GPSTripStateCheckerTest.tsx updated.
Upwork Automation - Do Not Edit
Issue OwnerCurrent Issue Owner: @ZhenjaHorbach

Activity

  1. ZhenjaHorbach commented on Sep 14, 2026

    @ZhenjaHorbach
    Contributor

    @MelvinBot
    Create a PR to migrate modals from the issue description

  2. roryabraham commented on Sep 22, 2026

    @roryabraham
    Contributor

    seems like Melvin timed out on this one

  3. ZhenjaHorbach commented on Sep 23, 2026

    @ZhenjaHorbach
    Contributor

    @MelvinBot
    Create a PR to migrate modals from the issue description

  4. MelvinBot commented on Sep 23, 2026

    @MelvinBot
    ContributorAuthor

    🤖 Draft PR created: [Batch 6] Migrate GPS trip ConfirmModal usages to useConfirmModal (Expensify/App#102013)

    Please mark it as "Ready for review" when it is ready.

    All five instances are migrated and no ConfirmModal import remains in the three files — the lint ratchet dropped exactly the 5 matching no-deprecated/ConfirmModal entries (1 + 1 + 3) from config/eslint/eslint.seatbelt.tsv, which is the mechanical confirmation.

    How each trap in the description was handled

    • Stale closures (both controllers). gpsDraftDetails and isOffline are read through refs inside the .then() continuation, so a trip answered after more points accumulated submits the full set rather than the show-time set.
    • GPSTripStateChecker auto-hide. The prompt is pushed under an id and an effect closes it with closeModalByID('gpsContinueTrip') when tracking stops, the trip changes owner, or the splash screen returns. I used closeModalByID rather than the closeModal() the description suggested, precisely because of gotcha Add Expensify eslint #3 — closeModal() pops whatever is on top, which would take an unrelated modal down if one had been opened over this one. The same idiom is already in useScreenShareRequestPrompt.ts.
    • Destructive cancel. shouldHandleNavigationBack: false is passed explicitly, so hardware back no longer stops the trip. A separate useRef flag distinguishes an auto-close from a real answer — without it the auto-hide would resolve with CLOSE, which means "stop the trip" here.
    • onBackdropPress was never present on these instances, so there was nothing to drop.

    Tests

    GPSTripStateCheckerTest.tsx now mocks useConfirmModal with the existing tests/utils/mockUseConfirmModal helper, since the prompt no longer renders in the page-local tree. The three original Onyx assertions are unchanged, plus four new cases: nothing is pushed behind the splash screen; shouldHandleNavigationBack is false; the entry is closed by id without stopping the trip when tracking stops; and a user-stopped trip submits every point recorded up to the moment they answered. That last one fails against a show-time read, so it is a real guard on the stale-closure fix rather than a restatement of it.

    Checks run
    Check Result
    npm run lint-changed ✅ pass
    npm run typecheck ✅ pass
    npm run spell-changed ✅ pass (4 files, 0 issues)
    npm run react-compiler-compliance-check check <3 changed src files> ✅ pass
    npm test -- --testPathPattern "GPS|IOURequestStepDistance" ✅ 13 suites, 111 tests pass
    npm test -- tests/unit/OnyxDrivenGlobalModalsTest.tsx tests/unit/ModalContextCloseByIDTest.tsx ✅ pass

    Prettier has no standalone npm script in this repo; formatting is enforced through ESLint, which passed. I ran react-compiler-compliance-check in check mode against the three changed source files because check-changed could not resolve its base ref in this environment.

    One thing I could not do: these are native-only surfaces and browser control was unavailable in this run, so nothing here is verified on a device. The Tests and QA sections of the PR are left as TODOs for you — the migration needs a pass on Android and/or iOS, particularly the continue-trip prompt's auto-hide and the hardware-back behavior.

  5. changed the title [-][Batch 6] Refactor ConfirmModal usage to useConfirmModal in GPS trip modals (5 modals)[/-] [+][$175] [Batch 6] Refactor ConfirmModal usage to useConfirmModal in GPS trip modals (5 modals)[/+] on Oct 1, 2026
  6. melvin-bot commented on Oct 1, 2026

    @melvin-bot
  7. jponikarchuk commented on Oct 5, 2026

    @jponikarchuk

    Deploy Blocker #102961 was identified to be related to the PR #102013 that attempted to fix this issue.

  8. added and removed
    ReviewingHas a PR in review
    on Oct 5, 2026
  9. changed the title [-][$175] [Batch 6] Refactor ConfirmModal usage to useConfirmModal in GPS trip modals (5 modals)[/-] [+][Due for payment 2026-10-12] [$175] [Batch 6] Refactor ConfirmModal usage to useConfirmModal in GPS trip modals (5 modals)[/+] on Oct 5, 2026
  10. melvin-bot commented on Oct 5, 2026

    @melvin-bot

    @ZhenjaHorbach

    The solution for this issue has been 🚀 deployed to production 🚀 in version 9.5.2-4 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

    If no regressions arise, payment will be issued on 2026-10-12. 🎊

    The following checklist (instructions) will need to be completed before the issue can be closed. Please copy/paste the Contributor+ Checklist from here into a new comment on this GH and complete it. If you have the K2 extension, you can simply click: [this button]. If no checklist is needed for this issue, you can click: [no checklist button]

  11. added
    Skip PaymentSkips all the payment process steps for an issue. Use it for fixed-price projects.
    on Oct 6, 2026
  12. changed the title [-][Due for payment 2026-10-12] [$175] [Batch 6] Refactor ConfirmModal usage to useConfirmModal in GPS trip modals (5 modals)[/-] [+][$175] [Batch 6] Refactor ConfirmModal usage to useConfirmModal in GPS trip modals (5 modals)[/+] on Oct 6, 2026
  13. mallenexpensify commented on Oct 6, 2026

    @mallenexpensify
    Contributor

    Please comment and tag me if/when payment is due, thx

  14. ZhenjaHorbach commented on Oct 7, 2026

    @ZhenjaHorbach
    Contributor

    Deploy Blocker #102961 was identified to be related to the PR #102013 that attempted to fix this issue.

    It's a separate issue
    Not a regression

  15. ZhenjaHorbach commented on Oct 7, 2026

    @ZhenjaHorbach
    Contributor

    @roryabraham @mallenexpensify
    And let's close this issue!

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

Metadata

Metadata

Labels

DailyKSv2Skip PaymentSkips all the payment process steps for an issue. Use it for fixed-price projects.

Type

No type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions