Update several transactions expense in the chat to work as expected if no optimistic thread report was created #68536
Conversation
…t wasn't created before
…ns-expense-inbox # Conflicts: # src/pages/home/ReportScreen.tsx
…ns-expense-inbox # Conflicts: # src/pages/home/ReportScreen.tsx
|
@hungvu193 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] |
|
I feel a bit under the weather today. Hopefully I can review it before EOD. |
|
Note: This issue is also on main, the report is empty after resolving duplicates. Screen.Recording.2025-08-21.at.00.16.03.mov |
hungvu193
left a comment
hungvu193
left a comment
There was a problem hiding this comment.
Took a round of testing, I've found a few bugs but I couldn't find a consistent way to reproduce them. I'll take a look in the morning,.
|
|
||
| const isReportUnread = ({lastReadTime = '', lastVisibleActionCreated = '', lastMentionedTime = ''}: Report): boolean => | ||
| lastReadTime < lastVisibleActionCreated || lastReadTime < (lastMentionedTime ?? ''); | ||
| lastReadTime <= lastVisibleActionCreated || lastReadTime < (lastMentionedTime ?? ''); |
There was a problem hiding this comment.
What's the purpose of this change 🤔 Looks like it doesn't relate to our current PR
There was a problem hiding this comment.
Right now, if a user puts an expense on Hold without opening it, we:
- create the transaction thread report
- create a report action - comment with Hold reason
They are created at the same time. But since the user hasn't opened the thread report, they should see the unread bubble (the same way as it's in the prod)
It doesn't happen to me on main / in the branch after merging main! But I've found another bug related to duplicates resolution (related to my updates!):
BUG.mp4I'll try to fix it! |
I also saw it yesterday but I couldn't reproduce it consistently :/ |
|
I've spend prettty much time on this bug investigations, and it feels like it's fixed by bumping Onyx version to |
Do we have a plan or an existing PR to upgrade onyx? I recently reviewed Onyx bump version 132. |
|
I think we are waiting for |
|
Let me double check 👀 |
|
All checks passed 😄 |
|
@roryabraham Can you please take a look at this one? |
src/components/MoneyRequestReportView/MoneyRequestReportTransactionList.tsx
Show resolved
Hide resolved
| } | ||
|
|
||
| const backTo = Navigation.getActiveRoute(); | ||
| const reportIDToNavigate = iouAction?.childReportID ?? generateReportID(); |
There was a problem hiding this comment.
Is this correct?
| const reportIDToNavigate = iouAction?.childReportID ?? generateReportID(); | |
| // Optimistically generate a reportID for the transaction thread if one does not yet exist. | |
| // The report itself will be created after we perform the navigation | |
| const reportIDToNavigate = iouAction?.childReportID ?? generateReportID(); |
Also, does it make sense to "search" for a report that doesn't yet exist? I'm just finding that it's unintuitive to include an optimistic reportID in a URL param for a report that hasn't yet been created. Maybe a better alternative would be search/view/optimistic or search/view/create-report or something like that?
There was a problem hiding this comment.
I think it can be confusing for the user to see search/view/create-report url here
I'm thinking about changing the approach a little to remove extra params from the route. I want to create a utility method that will create the thread report and only then navigate to it. This way we can escape extra params passing in the route, wdyt?
There was a problem hiding this comment.
That sounds like a good improvement.
| if (!targetTransactionThreadReportID && targetTransaction?.reportID) { | ||
| // If the report was already loaded before, but there are still no transaction thread report info, it means it hasn't been created yet. | ||
| // So we should create it. | ||
| if (targetTransactionReportMetadata?.hasOnceLoadedReportActions) { |
There was a problem hiding this comment.
I'm finding this useEffect really hard to follow. It seems like it's doing multiple things when running a few times in a row, which generally is a bad pattern. We should split up the effect into multiple effects, each of which do exactly one thing.
Plus, I don't think there should be multiple codepaths that lead to calling openReport. It's a very expensive API call to make, and ideally just one codepath should execute that API call.
There was a problem hiding this comment.
I'm finding this useEffect really hard to follow. It seems like it's doing multiple things when running a few times in a row, which generally is a bad pattern. We should split up the effect into multiple effects, each of which do exactly one thing.
Okay, I'll give it a try!
Plus, I don't think there should be multiple codepaths that lead to calling openReport. It's a very expensive API call to make, and ideally just one codepath should execute that API call.
I'm not 100% sure it will be possible in this case, cause there already was an additional call:
App/src/pages/TransactionMerge/DetailsReviewPage.tsx
Lines 78 to 90 in 441d232
There was a problem hiding this comment.
I've tried to clean it up a little in 1957994
| if (transactionID && currentUserEmail) { | ||
| openReport(reportIDFromRoute, '', [currentUserEmail], undefined, moneyRequestReportActionID, false, [], undefined, transactionID); | ||
| if (transactionID && currentUserEmail && !report) { | ||
| const iouReport = getReportOrDraftReport(iouReportID); |
There was a problem hiding this comment.
We shouldn't be adding new code usages that rely under the hood on Onyx.connect, because it's deprecated. I suppose NAB and we can handle the migration separately if it's going to be very large in scope
There was a problem hiding this comment.
Let leave it as is for now, there is a chance this part will be removed if we won't pass additional params in the route
There was a problem hiding this comment.
Hi! This will be cleaned up soon and we'll completely remove the usage of OpenReport here. The API will send the relevant report when opening the merge transaction page. I'll create an issue today.
There was a problem hiding this comment.
@youssef-lr can you link the issue when you create it?
…ns-expense-inbox # Conflicts: # src/components/MoneyRequestReportView/MoneyRequestReportTransactionList.tsx
|
I've resolved conflicts and removed this check: Lines 29 to 31 in 189b876 It was added in this PR. I guess it was added because the beta affected testing before we implemented an additional toggle to control thread creation. Note: the Jest test failing on other PRs as well, and it's not related to the changes, implemented in this one. |
|
Created #69487 for the flaky test |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
My WIP diff for Full diffdiff --git a/src/pages/TransactionMerge/DetailsReviewPage.tsx b/src/pages/TransactionMerge/DetailsReviewPage.tsx
index 1a99f386c6f..dddf0c39ed4 100644
--- a/src/pages/TransactionMerge/DetailsReviewPage.tsx
+++ b/src/pages/TransactionMerge/DetailsReviewPage.tsx
@@ -1,4 +1,4 @@
-import React, {useCallback, useEffect, useState} from 'react';
+import React, {useCallback, useEffect, useMemo, useState} from 'react';
import {View} from 'react-native';
import FullPageNotFoundView from '@components/BlockingViews/FullPageNotFoundView';
import Button from '@components/Button';
@@ -14,6 +14,7 @@ import useOnyx from '@hooks/useOnyx';
import useThemeStyles from '@hooks/useThemeStyles';
import {setMergeTransactionKey} from '@libs/actions/MergeTransaction';
import {convertToDisplayString} from '@libs/CurrencyUtils';
+import memoize from '@libs/memoize';
import {
getMergeableDataAndConflictFields,
getMergeFieldTranslationKey,
@@ -76,25 +77,17 @@ function DetailsReviewPage({route}: DetailsReviewPageProps) {
const [hasErrors, setHasErrors] = useState<Partial<Record<MergeFieldKey, boolean>>>({});
const [diffFields, setDiffFields] = useState<MergeFieldKey[]>([]);
- const [isCheckingDataBeforeGoNext, setIsCheckingDataBeforeGoNext] = useState<boolean>(false);
+ const [isLoadingConfirmationData, setIsLoadingConfirmationData] = useState<boolean>(false);
- useEffect(() => {
- if (!transactionID || !targetTransaction || !sourceTransaction) {
- return;
- }
-
- const {conflictFields, mergeableData} = getMergeableDataAndConflictFields(targetTransaction, sourceTransaction);
-
- setMergeTransactionKey(transactionID, mergeableData);
- setDiffFields(conflictFields as MergeFieldKey[]);
- }, [targetTransaction, sourceTransaction, transactionID]);
+ // Memoize openReport to prevent duplicate calls with the same parameters
+ const memoizedOpenReport = useMemo(() => memoize(openReport), []);
+ // Optimisitcally creates a new transaction thread report with optimistic data
const createTargetTransactionThreadReport = useCallback(() => {
const transactionThreadReportID = generateReportID();
const iouReport = getReportOrDraftReport(targetTransaction?.reportID);
const optimisticTransactionThread = buildTransactionThread(iouActionForTargetTransaction, iouReport, undefined, transactionThreadReportID);
-
- openReport(
+ memoizedOpenReport(
transactionThreadReportID,
undefined,
currentUserEmail ? [currentUserEmail] : [],
@@ -105,51 +98,74 @@ function DetailsReviewPage({route}: DetailsReviewPageProps) {
undefined,
targetTransaction?.transactionID,
);
- }, [currentUserEmail, iouActionForTargetTransaction, targetTransaction?.reportID, targetTransaction?.transactionID]);
+ }, [currentUserEmail, iouActionForTargetTransaction, targetTransaction?.reportID, targetTransaction?.transactionID, memoizedOpenReport]);
+ // Initialize merge data and identify conflicting fields between transactions
useEffect(() => {
- if (!isCheckingDataBeforeGoNext) {
+ if (!transactionID || !targetTransaction || !sourceTransaction) {
return;
}
- // When user selects a card transaction to merge, that card transaction becomes the target transaction.
- // The App may not have the transaction thread report loaded for card transactions, so we need to trigger
- // OpenReport to ensure the transaction thread report is available for confirmation page
- if (!targetTransactionThreadReportID && targetTransaction?.reportID) {
- // If the report was already loaded before, but there are still no transaction thread report info, it means it hasn't been created yet.
- // So we should create it.
- if (hasOnceLoadedTransactionThreadReportActions) {
- createTargetTransactionThreadReport();
- setIsCheckingDataBeforeGoNext(false);
- Navigation.navigate(ROUTES.MERGE_TRANSACTION_CONFIRMATION_PAGE.getRoute(transactionID, Navigation.getActiveRoute()));
- return;
- }
- return openReport(targetTransaction.reportID);
+ const {conflictFields, mergeableData} = getMergeableDataAndConflictFields(targetTransaction, sourceTransaction);
+
+ setMergeTransactionKey(transactionID, mergeableData);
+ setDiffFields(conflictFields as MergeFieldKey[]);
+ }, [targetTransaction, sourceTransaction, transactionID]);
+
+ // Load main transaction report when thread report doesn't exist and report actions haven't loaded
+ useEffect(() => {
+ if (!isLoadingConfirmationData) {
+ return;
}
- if (targetTransactionThreadReportID && !targetTransactionThreadReport) {
- return openReport(targetTransactionThreadReportID);
+
+ if (!targetTransactionThreadReportID && targetTransaction?.reportID && !hasOnceLoadedTransactionThreadReportActions) {
+ memoizedOpenReport(targetTransaction.reportID);
}
- // We need to wait for report to be loaded completely, avoid still optimistic loading
- if (!targetTransactionThreadReport?.reportID) {
+ }, [isLoadingConfirmationData, targetTransactionThreadReportID, targetTransaction?.reportID, hasOnceLoadedTransactionThreadReportActions, memoizedOpenReport]);
+
+ // Create new thread report when main report is loaded but no thread exists, then navigate
+ useEffect(() => {
+ if (!isLoadingConfirmationData) {
return;
}
- Navigation.navigate(ROUTES.MERGE_TRANSACTION_CONFIRMATION_PAGE.getRoute(transactionID, Navigation.getActiveRoute()));
- setIsCheckingDataBeforeGoNext(false);
+ if (!targetTransactionThreadReportID && targetTransaction?.reportID && hasOnceLoadedTransactionThreadReportActions) {
+ createTargetTransactionThreadReport();
+ setIsLoadingConfirmationData(false);
+ Navigation.navigate(ROUTES.MERGE_TRANSACTION_CONFIRMATION_PAGE.getRoute(transactionID, Navigation.getActiveRoute()));
+ }
}, [
- isCheckingDataBeforeGoNext,
+ isLoadingConfirmationData,
targetTransactionThreadReportID,
targetTransaction?.reportID,
- targetTransactionThreadReport,
- transactionID,
hasOnceLoadedTransactionThreadReportActions,
- iouActionForTargetTransaction,
- currentUserEmail,
- targetTransaction?.transactionID,
createTargetTransactionThreadReport,
+ transactionID,
]);
- // Handle selection
+ // Load existing thread report data when we have the ID but not the report
+ useEffect(() => {
+ if (!isLoadingConfirmationData) {
+ return;
+ }
+
+ if (targetTransactionThreadReportID && !targetTransactionThreadReport) {
+ memoizedOpenReport(targetTransactionThreadReportID);
+ }
+ }, [isLoadingConfirmationData, targetTransactionThreadReportID, targetTransactionThreadReport, memoizedOpenReport]);
+
+ // Navigate to confirmation page when thread report is fully loaded
+ useEffect(() => {
+ if (!isLoadingConfirmationData) {
+ return;
+ }
+
+ if (targetTransactionThreadReport?.reportID) {
+ Navigation.navigate(ROUTES.MERGE_TRANSACTION_CONFIRMATION_PAGE.getRoute(transactionID, Navigation.getActiveRoute()));
+ setIsLoadingConfirmationData(false);
+ }
+ }, [isLoadingConfirmationData, targetTransactionThreadReport?.reportID, transactionID]);
+
const handleSelect = (field: MergeFieldKey, value: MergeValue) => {
// Clear error if it has
setHasErrors((prev) => {
@@ -163,7 +179,6 @@ function DetailsReviewPage({route}: DetailsReviewPageProps) {
});
};
- // Handle continue
const handleContinue = () => {
if (!mergeTransaction) {
return;
@@ -180,7 +195,8 @@ function DetailsReviewPage({route}: DetailsReviewPageProps) {
setHasErrors(newHasErrors);
if (isEmptyObject(newHasErrors)) {
- setIsCheckingDataBeforeGoNext(true);
+ // Start the data preparation phase for navigation
+ setIsLoadingConfirmationData(true);
}
};
@@ -273,7 +289,7 @@ function DetailsReviewPage({route}: DetailsReviewPageProps) {
text={translate('common.continue')}
onPress={handleContinue}
isDisabled={!isEmptyObject(hasErrors)}
- isLoading={isCheckingDataBeforeGoNext}
+ isLoading={isLoadingConfirmationData}
pressOnEnter
/>
</FixedFooter> |
|
🚀 Deployed to staging by https://github.com/roryabraham in version: 9.2.1-0 🚀
|
|
🚀 Deployed to production by https://github.com/arosiclair in version: 9.2.1-20 🚀
|
7 participants
Explanation of Change
For the several transactions expenses opened from the inbox:
Fixed Issues
$ #67886
PROPOSAL: N/A
Tests
noOptimisticTransactionThreadsbeta.Block transaction thread report creationtoggle (you can also turn it on in the troubleshoot modal CMD + D)Regressions to test over:
#67589
#67588
❗ Note:
There are two known issues:
Offline tests
Same as in the Tests section.
QA Steps
Same as in the Tests section.
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))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.mp4
Android: mWeb Chrome
android_web.mp4
iOS: Native
ios.mp4
iOS: mWeb Safari
ios_web.mp4
MacOS: Chrome / Safari
web.mp4
MacOS: Desktop
desktop1.mp4