Fix LHN display issue for money request via scan#25906
Fix LHN display issue for money request via scan#25906luacmartins merged 30 commits intoExpensify:mainfrom
Conversation
|
@thesahindia 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] |
|
Taking this one over! |
Reviewer Checklist
Screenshots/VideosWebScreen.Recording.2023-09-05.at.10.09.39.PM.movMobile Web - ChromeScreen.Recording.2023-09-05.at.10.19.54.PM.movMobile Web - SafariScreen.Recording.2023-09-05.at.10.22.14.PM.movDesktopScreen.Recording.2023-09-05.at.10.27.45.PM.moviOSScreen.Recording.2023-09-05.at.10.24.47.PM.movAndroidScreen.Recording.2023-09-05.at.10.23.58.PM.mov |
|
Hi @ygshbht! I'm still seeing Screen.Recording.2023-08-29.at.4.58.34.AM.movCan you please check / confirm? |
|
@allroundexperts can you share a screenshot or video of the issue? |
My bad. I have updated the screen recording in my original comment. |
|
Does that need to be fixed too? As you can see its the same in the videos i have shared. There's the main chat where you request the money from and there's the all IOU requests chat. I was under the impression that only the IOU requests chat's title needs to be changed as that was mentioned in the issue and not the subtitle. Changing the subtitle will require a slightly different approach. We can certainly discuss further if that is an issue too and needs to be changed |
|
@luacmartins Can you confirm if this is something we need to fix as part of this ticket? |
|
Yes, we should fix that |
|
@allroundexperts Updated 2023-08-30.18-09-29.mp4 |
|
Still not fixed @ygshbht. Screen.Recording.2023-08-31.at.5.25.29.PM.mov |
|
I'm sure I did. But I'll try again. |
|
No, I don't think it shows being scanned on that screen too. Let me see |
|
@allroundexperts In this case since "Being scaneed" is not going to show because it is not a reportAction, what do you want to show in the LHN in case there is a message? "Being scanned" or the message? Is the situation here same like the other two? |
|
If you see the screenshot below, the receipt scan is still in progress. But the LHN shows a |
|
@allroundexperts In this case, I think the |
|
I was offline 😄 |
|
@allroundexperts Oh. that makes it unclear why it would change at one place but not at another. Would you still mind checking with the Onyx data in the Application tab or any other way? Because i too have experienced |
|
I will and let you know. Does it not happen at your end? |
…od via function argument for display logic.
|
@allroundexperts Updated. I've have kept the original approach of using function argument here #25906 (comment). Let me know if you'd like to change |
src/libs/ReportUtils.js
Outdated
| /** | ||
| * Gets all sorted transactions on an IOU report with a receipt and whose pending action is not delete | ||
| * | ||
| * @param {Object|null} iouReportID | ||
| * @returns {[Object]} | ||
| */ | ||
| function getSortedTransactionsWithReceipts(iouReportID) { | ||
| const reportActions = ReportActionsUtils.getAllReportActions(iouReportID); | ||
| const sortedReportActions = ReportActionsUtils.getSortedReportActionsForDisplay(reportActions); | ||
|
|
||
| return _.reduce( | ||
| sortedReportActions, | ||
| (transactions, action) => { | ||
| if (ReportActionsUtils.isMoneyRequestAction(action)) { | ||
| const transaction = TransactionUtils.getLinkedTransaction(action); | ||
| if (TransactionUtils.hasReceipt(transaction)) { | ||
| transactions.push(transaction); | ||
| } | ||
| } | ||
| return transactions; | ||
| }, | ||
| [], | ||
| ); | ||
| } | ||
|
|
There was a problem hiding this comment.
Why is this needed? Why can't we just use the logic we used in the getReportPreviewMessage function?
There was a problem hiding this comment.
You could say its legacy. Currently its used here but we can remove it and just use the last reportAction like getReportPreviewMessage. In such case the dependency array of optionItem will change whenever new reportAction is added but it does so anyway
There was a problem hiding this comment.
That would be better. We should try to add as less of changes as possible. The reason is that there's a lot of edge cases dependent on this code and doing such a change would just introduce regressions (which I want to avoid).
|
@allroundexperts Updated |
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, [fullReport.reportID, receiptTransactions, reportActions]); | ||
|
|
||
| const memoizedLastTransaction = useDeepCompareMemo(lastTransaction); |
There was a problem hiding this comment.
Why are we re-memoizing the already memoed transaction?
There was a problem hiding this comment.
The useDeepCompareMemo does the deepEqal isEqual check. So even if the transaction has not changed but the reference is (you get a new object but with the same data), this will prevent rerendering
There was a problem hiding this comment.
If you always get the same object in situations where the transaction has not changed, then we dont have to use useDeepCompareMemo. We may remove it since optionItem memo does deepEqual check in optionItem. The only benefit of using useDeepCompareMemo is that this line won't have to be executed
`const item = SidebarUtils.getOptionData(fullReport, reportActions, personalDetails, preferredLocale, policy);
`
There was a problem hiding this comment.
Do you think a re-render is more expensive or a deep comparison of two objects?
There was a problem hiding this comment.
I think its an overkill to use it
There was a problem hiding this comment.
I can agree. And its not like you cant add these optimizations when there are performance issues
There was a problem hiding this comment.
If there is a performance issue, I think we can add it after discussion with a wider group. Let's remove that file for now and if needed, we can add it again.
There was a problem hiding this comment.
Awesome, Thanks!
Can you please re-test this and update the screen recordings again since we did quite some refactoring to the original PR?
| const lastReportAction = _.first(sortedReportActions); | ||
| return TransactionUtils.getLinkedTransaction(lastReportAction); | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, [fullReport.reportID, receiptTransactions, reportActions]); |
There was a problem hiding this comment.
Why is receiptTransactions is needed as a dependency?
There was a problem hiding this comment.
So that when the status of transaction updates (from scanning to scanned), the LHN is updated too
There was a problem hiding this comment.
Did you mean to ask this in this context? #25906 (comment)
There was a problem hiding this comment.
I think it depends on the size of the object you are deep comparing and the component that has to be re-rendered. If the deep comparison object is small, like in this case, I think it's a negligible performance issue.
There was a problem hiding this comment.
Sorry yes. Let's take this there!
|
Thanks @ygshbht! Testing this again now. |
Co-authored-by: Carlos Martins <luacmartins@gmail.com>
Co-authored-by: Carlos Martins <luacmartins@gmail.com>
allroundexperts
left a comment
allroundexperts
left a comment
There was a problem hiding this comment.
Looks good to me as well!
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/luacmartins in version: 1.3.65-0 🚀
|
|
🚀 Deployed to production by https://github.com/mountiny in version: 1.3.65-7 🚀
|
|
🚀 Deployed to staging by https://github.com/luacmartins in version: 1.3.66-0 🚀
|
|
🚀 Deployed to production by https://github.com/Beamanator in version: 1.3.66-3 🚀
|
| // In some scenarios, a transaction might be created after reportActions have been modified. | ||
| // This can lead to situations where `lastTransaction` doesn't update and retains the previous value. | ||
| // However, performance overhead of this is minimized by using memos inside the component. | ||
| receiptTransactions: {key: ONYXKEYS.COLLECTION.TRANSACTION}, |
There was a problem hiding this comment.
I'm not sure that it was fully realized, but this means that every option in the LHN is connected to the entire transaction collection. That means that when any transaction is updated, every single option in the LHN is re-rendered. That seems like it's bad for performance.
There was a problem hiding this comment.
@tgolen Yes, that results in additional renders. However, I don't perceive it as a significant performance issue as transaction updates are infrequent and memoization is implemented within the component. But indeed, there's room for improvement. What specific optimizations do you propose?
There was a problem hiding this comment.
I initally tried accessing only the transcation attached to last reportAction but that doesn't work as I explain in the comment
There was a problem hiding this comment.
OK, I don't really fully understand that comment. Some of the questions I have are:
- "In some scenarios" what scenarios exactly?
- Where does
lastTransactioncome from and why doesn't it update?
It seems like if the root problem for that was fixed, then there wouldn't be a need for this workaround.
There was a problem hiding this comment.
There was a problem hiding this comment.
getTransaction(lastReportAction);
OK, so it's getting the transactionID like this transactionID = reportAction.originalMessage?.IOUTransactionID ?? ''; which is fine.
The transcaction seems to be created sometime after the transactionID of reportAction is generated. So you don't have access to transcation when the reportAction gets the transactionID
I don't think that matters. As long as you have the transactionID, you can connect to Onyx with it and you'll get any updates to the object on that key.
I imagine the code should look more like this:
receiptTransaction: {
key: ({ reportActions }) => {
const lastReportAction = _.last(reportActions);
if (!lastReportAction || !lastReportAction.name || lastReportAction.name !== CONST.REPORT.ACTIONS.TYPE.IOU || !lastReportAction.originalMessage || !lastReportAction.originalMessage.IOUTransactionID) {
return `${ONYXKEYS.COLLECTION.TRANSACTION}0`; // This will always return `null` for these cases
}
return `${ONYXKEYS.COLLECTION.TRANSACTION}${lastReportAction.originalMessage.IOUTransactionID}`;
}
}
There was a problem hiding this comment.
@tgolen What you say makes sense. But I'm not sure how Onyx works in detail. From what i remember this approach didn't work for me. If it does for you, the change can be implemented. If you test, please also check if the LHN is updated when the status of transaction is completed (i.e to scanned or scan failed). If i recall correclty, many times, the lastTranscation that i had was actually the second last one
There was a problem hiding this comment.
There isn't anything magical about how Onyx works (I know because I wrote a lot of it and there are many many engineers better than me out there).
So, I think it's just running with something like what I've posted and debugging it until it works. Or also, maybe you uncover some bugs, which is great too! We all learn through this. That's better than leaving a workaround like this in the code which could have a big performance impact.
When it's all said and done though, it sounds like we're having a discussion about completely refactoring this component to not have any withOnyx() bindings, but I don't know how far away that is or when it is coming.
There was a problem hiding this comment.
You are right. Honestly, I'm very curious to know what the actual issue is here. My apologies if it appears to be a workaround. I did not see it as such; it was merely the best I could do and provided the explanation for the same
6 participants
Details
Fixed Issues
$ #25778
PROPOSAL: #25778 (comment)
Tests
Scan in progressOffline tests
Same as Tests
QA Steps
Same as tests
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)myBool && <MyComponent />.src/languages/*files and using the translation methodWaiting for Copylabel for a copy review on the original GH to get the correct copy.STYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)/** comment above it */thisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)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
Web
desktop.web.mp4
Mobile Web - Chrome
android_chrome_speedup_720p.mp4
Mobile Web - Safari
safari_speedup_720p.mp4
Desktop
desktop.3.mp4
iOS
ios_speedup_720p.mp4
Android
android_trimmed_720p.mp4