Fix scan-error modal not displayed when retry fails#85042
Fix scan-error modal not displayed when retry fails#85042
Conversation
Two issues prevented the scan-error modal from appearing on retry failure: 1. The retry flow in handleFileRetry called dismissError() which triggers cleanUpMoneyRequest() for pending ADD transactions. This destructively deletes the transaction/report and navigates the user away before the retry action can complete and surface errors. 2. The useConfirmModal hook defaults shouldHandleNavigationBack to true, which can cause unwanted navigation when the error modal is dismissed. Changes: - Replace dismissError() with Transaction.clearError() in handleFileRetry for retry cases, preserving the transaction context so the user stays on the expense details page - Add transactionID to ReceiptError type and thread it through from getReceiptError callers - Add shouldHandleNavigationBack: false to showConfirmModal in DotIndicatorMessage to match the old inline ConfirmModal behavior Co-authored-by: DylanDylann <DylanDylann@users.noreply.github.com>
…id-values lint rule
|
Fixed the failing |
|
The only failing check is CLA / CLA, and it failed due to a transient DNS resolution error — not a code issue: This is an infrastructure flake (GitHub API was temporarily unreachable from the CI runner). All other checks are passing or still pending. Fix: Re-run the failed CLA workflow. I wasn't able to trigger a re-run due to permissions — please re-run it manually from the Actions tab. |
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
@MelvinBot Please review again the expectation: Expected Result: Actual Result: |
|
I investigated the failing checks — both failures are infrastructure-related and not caused by code changes in this PR. 1. CLA / CLA — DNS resolution failure on the CI runner: 2. build (New Expensify Dev, DebugDevelopment, false) — All build steps completed successfully, but the job itself failed at the "Complete job" stage (runner-level issue after running for 46 minutes). This is a runner/infrastructure timeout, not a build error. Action needed: Please re-run the failed checks — they should pass on retry. |
|
Thanks for flagging this, Problem 1: The confirm modal is only triggered on file reconstruction failureThe // src/libs/ReceiptUploadRetryHandler/index.ts:9-19
fetch(message.source)
.then((res) => res.blob())
.then((blob) => {
// File reconstruction succeeded → handleFileRetry is called (no modal)
handleFileRetry(message, reconstructedFile, dismissError, setShouldShowErrorModal);
})
.catch(() => {
// File reconstruction failed → modal is shown
setShouldShowErrorModal(true);
});With "Simulate failing network requests" enabled, only API calls through The Problem 2: Retry creates a brand new transactionInside const optimisticTransactionID = existingTransactionID ?? NumberUtils.rand64();Since the retry params don't include Result: the user sees the error disappear (cleared on the old transaction) but never sees the new error (it's on a different transaction they're not viewing). Problem 3: Navigation side effects during retry
What needs to change
I'll work on updating this PR with the additional fixes. |
When retrying a failed scan expense, the retry flow was creating a brand new transaction (with a new transactionID) instead of reusing the original. This caused the retry failure error to appear on a different transaction than the one the user was viewing, making the error invisible. Changes: - Pass existingTransactionDraft/existingTransaction with the original transactionID during retry so requestMoney/trackExpense reuse the same transaction instead of generating a new one - Set shouldHandleNavigation: false during retry to prevent navigation side effects - In trackExpense, fall back to existingTransaction.transactionID when computing existingTransactionID Co-authored-by: DylanDylann <DylanDylann@users.noreply.github.com>
|
Pushed a fix in dc7244e that addresses the core issues: What changed:
Expected behavior after this fix:
Note on the confirm modal: The confirm modal (generic error + "OK" button) is specifically designed for when the local file reconstruction fails (e.g., the receipt photo was deleted from the device). With "Simulate failing network requests", only API calls fail — local file access still works. So the expected behavior is the receipt error re-appearing with the "Retry"/"Download" links, not the confirm modal. |
The previous commit changed trackExpense to reuse existingTransaction.transactionID for retries, but Duplicate.ts had a hardcoded placeholder transactionID: '1' in its existingTransaction objects. This caused duplicated transactions to get the same ID as the original. Replace the placeholder with NumberUtils.rand64() to generate proper unique IDs. Co-authored-by: DylanDylann <DylanDylann@users.noreply.github.com>
|
Fixed the failing check: The Root cause: The previous commit ( Fix: Replaced the hardcoded |
Explanation of Change
When a user creates a scan expense with "Simulate failing network requests" enabled and taps "Retry" after the error appears, the confirm modal does not appear and the app redirects to the workspace room.
Two issues caused this:
Destructive dismiss during retry:
handleFileRetrycalleddismissError()before dispatching the retry action. For pending ADD transactions,dismissError()triggerscleanUpMoneyRequest()which deletes the entire transaction/report and navigates the user away — before the retry can complete and surface errors. This is now replaced withTransaction.clearError()which only clears the error from the transaction without destroying it, keeping the user on the expense details page.Modal navigation back: The
useConfirmModalhook defaultsshouldHandleNavigationBacktotrue. When the error modal was shown viashowConfirmModalinDotIndicatorMessage, dismissing it could trigger unwanted navigation. This is now explicitly set tofalseto match the original inlineConfirmModalbehavior.To support the non-destructive retry,
transactionIDis now threaded through theReceiptErrortype sohandleFileRetrycan target the specific transaction for error clearing.Fixed Issues
$ #83537
PROPOSAL: #83537 (comment)
Tests
Offline tests
QA Steps
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand 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.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari