fix: offline merge visibility for track distance expenses#77862
fix: offline merge visibility for track distance expenses#77862Bar-Maz wants to merge 44 commits intoExpensify:mainfrom
Conversation
|
All contributors have signed the CLA ✍️ ✅ |
ab9d95a to
f5aea4e
Compare
Codecov Report❌ Looks like you've decreased code coverage for some files. Please write tests to increase, or at least maintain, the existing level of code coverage. See our documentation here for how to interpret this table.
|
f5aea4e to
d179e80
Compare
package-lock.json
Outdated
| "electron-builder": "26.0.19", | ||
| "eslint": "^9.36.0", | ||
| "eslint-config-airbnb-typescript": "^18.0.0", | ||
| "eslint-config-expensify": "^2.0.101", |
| const shouldShowLoadingPlaceholder = useMemo(() => { | ||
| if (isOffline) { | ||
| return false; | ||
| } | ||
| if (Array.isArray(eligibleTransactions)) { | ||
| return false; | ||
| } | ||
| return true; | ||
| }, [isOffline, eligibleTransactions]); |
There was a problem hiding this comment.
Will there be any benefit from this instead just
const shouldShowLoadingPlaceholder = !isOffline && !Array.isArray(eligibleTransactions);
| const reportTransactionIDs = useMemo(() => { | ||
| const allTransactions = getAllNonDeletedTransactions(reportTransactions, allReportActions ?? []); | ||
| return allTransactions.map((transaction) => transaction.transactionID); | ||
| }, [reportTransactions, allReportActions]); |
There was a problem hiding this comment.
I don't see a point for creating temporary allTransactions here, I guess it could be a big one. If its the same in result, maybe lets use previous one?
| ), | ||
| [reportActions, isOffline, canPerformWriteAction, reportTransactionIDs, policy], | ||
| ); | ||
| const [allTransactions] = useOnyx(ONYXKEYS.COLLECTION.TRANSACTION, {canBeMissing: true}); |
There was a problem hiding this comment.
I'm bit worried about this allTransactions and visibleReportActions. Let's check how it performs with heavy account. I think there could be performance drop if we use it in dep array with such logic like this one
| }, [canShowHeader, retryLoadNewerChatsError]); | ||
|
|
||
| const shouldShowSkeleton = isOffline && !sortedVisibleReportActions.some((action) => action.actionName === CONST.REPORT.ACTIONS.TYPE.CREATED); | ||
| const shouldShowSkeleton = isOffline && sortedVisibleReportActions.length === 0; |
There was a problem hiding this comment.
Is this part of the solution? The previous condition looks like a solution to a specific use case.
|
|
||
| const shouldShowSkeleton = isOffline && !sortedVisibleReportActions.some((action) => action.actionName === CONST.REPORT.ACTIONS.TYPE.CREATED); | ||
| const shouldShowSkeleton = isOffline && sortedVisibleReportActions.length === 0; | ||
|
|
| [oldIOUAction.reportActionID]: null, | ||
| }, | ||
| }); | ||
|
|
| const isIOUReportExpense = isExpenseReport(iouReport); | ||
|
|
||
| if (isIOUReportExpense) { | ||
| const expenseReportTransactions = Object.values(allTransactions ?? {}).filter((transaction) => { |
There was a problem hiding this comment.
There is allTransactions on every filter iteration reportActions.filter((reportAction) and it looks quite complex. Maybe it would be better to create some utility function for it or even few of them (with tests ideally if possible)? I think it will be hard to test and maintain in this shape
| ); | ||
|
|
||
| if (eligibleTransactions?.length === 0) { | ||
| const shouldShowLoadingPlaceholder = useMemo(() => { |
There was a problem hiding this comment.
we should avoid using useMemo - compiler would mark this as an error
9a62f9a to
7a6ae71
Compare
|
Hey @Bar-Maz, please make sure to resolve all of the checks failures! |
|
I have read the CLA Document and I hereby sign the CLA |
ad232c3 to
d11d715
Compare
…ode review feedback
…line expense merge
src/libs/actions/Report.ts
Outdated
| const report = allReports?.[`${ONYXKEYS.COLLECTION.REPORT}${reportID}`]; | ||
| const reportExists = !!report; | ||
|
|
||
| if (isOffline && hasReportActions && reportExists) { |
There was a problem hiding this comment.
why do we need this change? If we want to create an optimistic report, it would be better if we can do it in Transaction_Merge API.
There was a problem hiding this comment.
I've moved the optimistic report creation logic from openReport to mergeTransactionRequest in MergeTransaction.ts.
src/libs/actions/Report.ts
Outdated
| }; | ||
| const isOffline = NetworkStore.isOffline(); | ||
| const hasReportActions = reportActionsExist(reportID); | ||
| const report = allReports?.[`${ONYXKEYS.COLLECTION.REPORT}${reportID}`]; |
There was a problem hiding this comment.
Actually, we want to avoid using allReports here. We already had many issues to remove it, like #66411
There was a problem hiding this comment.
I've modified this function so it no longer adds any new reliance on allReports.
…ode-distance-merge
|
@Bar-Maz Some lints are failing |
| } | ||
| const reportID = mergeTransaction.reportID; | ||
|
|
||
| // Ensure expense report exists in Onyx before merge (required by getUpdateMoneyRequestParams) |
There was a problem hiding this comment.
Can you please share the steps to reproduce this issue? I think expenseReport appears in the confirmation page.
There was a problem hiding this comment.
After testing, I found that the expense report is always present in Onyx at this point. I was worried that it might not be fully loaded if we, for example, just logged in. This part turned out to be unnecessary, so I removed it.
| import Parser from './Parser'; | ||
| import {arePersonalDetailsMissing, getEffectiveDisplayName, getPersonalDetailByEmail, getPersonalDetailsByIDs} from './PersonalDetailsUtils'; | ||
| import {getPolicy, isPolicyAdmin as isPolicyAdminPolicyUtils} from './PolicyUtils'; | ||
| // eslint-disable-next-line import/no-cycle |
There was a problem hiding this comment.
Let's add the explanation when we want to disable lint rule
There was a problem hiding this comment.
Done! I'll also explore the possibility of getting rid of these cycles in a separate proposal.
There was a problem hiding this comment.
Can you try @libs/ReportUtils? We used before without any lint error so I think we can keep the same
|
Testing now |
If we select the details of SelfDM, the workspace expense will show infinite loading Screen.Recording.2026-01-22.at.11.01.44.mov |
heyjennahay
left a comment
There was a problem hiding this comment.
Product review not required
|
Hi, @dukenv0307 However, while working on this issue, I noticed that after the offline merge, the workspace and chat don’t seem to update properly on the current main branch. In fact, most of the changes in this PR are addressing that behavior. I can fix this so that chat updates are reflected correctly as well, based on the work already done here. After consulting with @VickyStash, I have a clearer idea of a few improvements we could make to the logic. Do we want to correct this behavior? If so, what’s the preferred approach? Should I continue in this PR, or open a new one? I’ve attached a video demonstrating the current state and the mentioned issue. offline_merge_main.mp4 |
|
Is it expected to show the loading indicator here?
Screen.Recording.2026-01-27.at.09.22.20.mov |
I think we should do it in another PR because this one is quite big. Can you please update the tests/expectations to cover the following case
|
I agree that this PR is quite big. However, the original issue (the skeleton loader after merge) already seems to be resolved on What remains (and what most of this PR is addressing) is the inconsistent chat/workspace update behavior after offline merge, which can still be observed on If we don’t want to extend the scope of this issue, we could close it as resolved and open a new issue specifically for the offline-merge chat/workspace update behavior, and continue the work there based on the changes already made in this PR. |
@Bar-Maz I don't think we should add more changes to this PR. Ideally, we can list the improvements in this PR and make the other improvements in separate PRs. cc @VickyStash for more thoughts |
I agree that this PR is already quite big and complex.
@Bar-Maz does the PR fixes this issue right now (without any furthur adjustments?). Does it fix it in a way so optimistic data is aligned with the data received from the BE later? If any adjustments are needed, I think it's maybe better to have a new PR. I think it worth to check:
Cause I see there are some PRs, that are applying fixes in the same area, does any of it fixes the issue you mention? |
|
Any updates @Bar-Maz ? |
|
Hi, I'll close it as I'm preparing a separate PR that solves the issue |
Explanation of Change
Fixed an issue where track distance expenses were not visible in expense reports after merging transactions while offline, causing skeleton loaders and empty reports.
Root Cause:
The transaction filtering logic for expense reports excluded all transactions with pending actions, even when offline. After merging a distance expense offline, the transaction had
pendingAction: ADD, which caused it to be filtered out, resulting in skeleton loaders and empty reports.Solution:
ADDpending action when offline (they're excluded when online)ReportActionsViewto use the new filtering logic for expense reportsResult:
Merged expenses are now immediately visible in expense reports when offline, without skeleton loaders or empty reports.
Fixed Issues
$ #75267
PROPOSAL:
Tests
Offline tests
QA Steps
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.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari