Fix avatar and confirm page loading infinitely in offline#88120
Fix avatar and confirm page loading infinitely in offline#88120jnowakow wants to merge 4 commits intoExpensify:mainfrom
Conversation
|
@stitesExpensify Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 407053d2e8
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| originalAmount: shouldCopyOriginalAmount ? transaction.originalAmount : null, | ||
| originalCurrency: shouldCopyOriginalCurrency ? transaction.originalCurrency : null, |
There was a problem hiding this comment.
Restore original FX fields on move failure
The optimistic merge now force-clears originalAmount and originalCurrency, but the corresponding failure rollback for this transaction still restores only reportID, comment, and converted-amount state. If CHANGE_TRANSACTIONS_REPORT fails (offline/server error), these optimistic nulls persist and the transaction loses its prior original-currency metadata, which can leave incorrect values in UI/search until a full refetch.
Useful? React with 👍 / 👎.
| ...transaction, | ||
| reportID, | ||
| comment, | ||
| originalAmount: shouldCopyOriginalAmount ? transaction.originalAmount : null, |
There was a problem hiding this comment.
❌ CONSISTENCY-6 (docs)
The optimistic data now explicitly overrides originalAmount and originalCurrency (setting them to null when they match amount/currency), but the failureData block at lines 1081-1090 does not restore these fields. If the API call fails, originalAmount and originalCurrency will remain null instead of being reverted to their original values from transaction.originalAmount and transaction.originalCurrency.
Every field explicitly overridden in optimisticData should have a corresponding restoration in failureData. Add the original values to the failure block:
failureData.push({
onyxMethod: Onyx.METHOD.MERGE,
key: `${ONYXKEYS.COLLECTION.TRANSACTION}${transaction.transactionID}`,
value: {
reportID: transaction.reportID,
comment: transaction.comment,
originalAmount: transaction.originalAmount,
originalCurrency: transaction.originalCurrency,
...(shouldClearAmount && {pendingAction: transaction.pendingAction ?? null}),
...(shouldClearAmount && {convertedAmount: transaction.convertedAmount}),
},
});Reviewed at: 407053d | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
|
@rojiphil are you available to review this one? |
|
Yes. Available. Will come to this one soon. |
|
Looking into this one now. |
|
@jnowakow For consistency, let us reset the values of 88120-web-chrome-001.mp4 |
Explanation of Change
Bug 1 — Avatar opens "Not here" page
Optimistically created action missed
actorIDwhich should be equal to current user.Bug 2 — Confirm page loads infinitely
Search always return
originalCurrencyfor transactions even if it is not set. (Auth code). It was copied to transaction object when transaction was undeleted even if it shouldn't be there. The proper fix it to modify backend code but this PR provides hot fix (not copyingoriginalCurrencywhen it is equal tocurrency.originalCurrencyis used to display loading page:App/src/pages/iou/request/step/IOURequestStepConfirmation.tsx
Line 1319 in 47c4779
Fixed Issues
$ #88034
PROPOSAL:
Tests
Offline tests
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
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
avatar-and-submit.mov