Closed lanitochka17 closed 3 months ago
Triggered auto assignment to @puneetlath (Bug
), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.
@puneetlath Whoops! This issue is 2 days overdue. Let's get this updated quick!
@lanitochka17 is this happening on specific platforms? Or all?
@puneetlath only Android
@puneetlath Whoops! This issue is 2 days overdue. Let's get this updated quick!
Job added to Upwork: https://www.upwork.com/jobs/~01cba2ec575ed8acd2
Triggered auto assignment to Contributor-plus team member for initial proposal review - @hoangzinh (External
)
IOU –Expense chat doesn't scroll to bottom when send message or money request
Currently we are using DateUtils library to create the optimistic values, for example when we send a new message the report created and lastVisibleActionCreated are not using the same instance/values of the DateUtils. we are using it two times which cause the difference between them causing hasNewestReportAction to be false and this makes the auto scroll not works.
We should modify every method related to build optimistic when sending a new comment, split expense, submit expense, etc.
To fix the new comment sending, the created
date is assigned using same function used for lastVisibleActionCreated
DateUtils.getDBTimeWithSkew
(in our scenarios without a offset), this is the part of the code:
To add a comment we use the addAction
method:
https://github.com/Expensify/App/blob/9f512746783f2a9db6c2e8e74416605113fc7ab2/src/libs/actions/Report.ts#L439-L448
But this never going to match because the lastVisibleActionCreated
is created in different function in different time, so we should modify the buildOptimisticAddCommentReportAction to accept the currentTime
and use it to specify the created prop like:
created: currentTime ?? DateUtils.getDBTimeWithSkew(Date.now() + createdOffset),
To send a split expense we should modify createSplitsAndOnyxData to use the same currentTime like:
const currentTime = DateUtils.getDBTime();
splitChatReport.lastReadTime = currentTime;
splitChatReport.lastMessageText = splitIOUReportAction.message?.[0]?.text;
splitChatReport.lastMessageHtml = splitIOUReportAction.message?.[0]?.html;
splitChatReport.lastActorAccountID = currentUserAccountID;
splitChatReport.lastVisibleActionCreated = currentTime;
splitIOUReportAction.created = currentTime;
(For the last one it might be better to modify ReportUtils.buildOptimisticIOUReportAction
and getOrCreateOptimisticSplitChatReport
to accept currentTime
as a new parameter, but currently there are other mutations for splitChatReport.)
To submit an expense we should modify buildOptimisticReportPreview
to accept a new parameter containing the created time:
function buildOptimisticReportPreview(
.
.
.
created = DateUtils.getDBTime(),
This way we can send the create date from getMoneyRequestInformation passing the same currentTime
like:
const currentTime = DateUtils.getDBTime();
.
.
.
let chatReport = !isEmptyObject(parentChatReport) && parentChatReport?.reportID ? parentChatReport : null;
**if (chatReport) {
chatReport.lastVisibleActionCreated = currentTime;
}**
.
.
.
if (reportPreviewAction) {
reportPreviewAction = ReportUtils.updateReportPreview(iouReport, reportPreviewAction, false, comment, optimisticTransaction);
} else {
reportPreviewAction = ReportUtils.buildOptimisticReportPreview(chatReport, iouReport, comment, optimisticTransaction, '', **currentTime**);
@hoangzinh thoughts on the proposals so far?
but we got the issue when we set the FlatList without this prop, the Flatlist component do not detects the change of this if we add this after a render.
@samilabud I don't think your RC is correct. Can you check why user just sent a new message but shouldEnableAutoScrollToTopThreshold is false, lead to "we set the FlatList without this prop" as you said above?
but we got the issue when we set the FlatList without this prop, the Flatlist component do not detects the change of this if we add this after a render.
@samilabud I don't think your RC is correct. Can you check why user just sent a new message but shouldEnableAutoScrollToTopThreshold is false, lead to "we set the FlatList without this prop" as you said above?
Hi, when the reportActionsView load, set a variable to determinate if there are a new message, when the user sent a new message that value is false and later we send that false into the next statement:
const shouldEnableAutoScroll = hasNewestReportAction && (!reportActionID || !isNavigatingToLinkedMessage);
Because of this false value, the FlatList going to take that behavior not matter what we are sending as a message.
Actually, your solution doesn't work in offline mode
https://github.com/Expensify/App/assets/9639873/422fec46-3827-4758-a1e1-30df80723da6
when the user sent a new message that value is false
@samilabud If we put a log there, we can see that value would be true after seconds. Can you find a reason for it?
Actually, your solution doesn't work in offline mode
Screen.Recording.2024-05-03.at.18.19.58.mp4
when the user sent a new message that value is false
@samilabud If we put a log there, we can see that value would be true after seconds. Can you find a reason for it?
Hey @hoangzinh, as far I can see to determinate if we has a new message we are comparing the last time we synchronized with the server (lastVisibleActionCreated
) vs the current time of the last message we sent (reportActions[0]?.created
). This cause the next behavior:
If we are online
reportActions[0]?.created
match withlastVisibleActionCreated
so we got true (for shouldEnableAutoScroll
).lastVisibleActionCreated
remains unchanged and reportActions[0]?.created
change to the current time, this makes that shouldEnableAutoScroll
switch to false. (it seems the logic behind is to wait for the server response to do the automatic scroll down)shouldEnableAutoScroll
) because reportActions[0]?.created
match withlastVisibleActionCreated
match again.If we are offline
shouldEnableAutoScroll
remains false because reportActions[0]?.created
is changing to the current time but lastVisibleActionCreated
keeps as the same until we get online again.As I said in before it seems the logic behind is to wait for the server response to do the automatic scroll down, so maybe this is the expected behavior, except for the error which was reported here.
Please let me know if I should verify deeper or if you have any other suggestion.
@puneetlath @hoangzinh this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!
- When we send a message the lastVisibleActionCreated remains unchanged and reportActions[0]?.created change to the current time
@samilabud I don't think it's correct. Can you check again? I think the auto scroll behavior in chat should work for both online and offline mode.
- When we send a message the lastVisibleActionCreated remains unchanged and reportActions[0]?.created change to the current time
@samilabud I don't think it's correct. Can you check again? I think the auto scroll behavior in chat should work for both online and offline mode.
Yes it is a bit weird and I have checked it again, below a video with the output of the debug log:
https://github.com/Expensify/App/assets/5216036/e88a7d3f-95bc-40e4-9659-b63dd6a84a8b
As you can see the hasNewestReportAction variable when is offline remains false, which means shouldEnableAutoScroll going to be false:
const shouldEnableAutoScroll = hasNewestReportAction && (!reportActionID || !isNavigatingToLinkedMessage);
BTW I have also tested with my mobile phone, the behavior is the same when is offline.
@samilabud I agreed that at the moment, hasNewestReportAction
is falsy
in a short period time then become truthy
. I believe it's a root cause and a bug that we should fix it.
Btw, can we step back a little bit and answer my question here https://github.com/Expensify/App/issues/40594#issuecomment-2094219765?
- When we send a message the lastVisibleActionCreated remains unchanged and reportActions[0]?.created change to the current time
Can you log lastVisibleActionCreated
when it's in offline mode to confirm what you said is correct? Thanks
Hi @hoangzinh, thanks for the suggestion, I realized the difference between lastVisibleActionCreated
and reportActions[0]?.created
is 2.262 seconds before we are synchronized with the server, that is the why we got the false, below the video requested:
https://github.com/Expensify/App/assets/5216036/8998e4eb-aefa-45a7-bcb2-5c9a56032002
So instead of lastVisibleActionCreated
remained unchanged is that it is delayed (when offline and also when we send a message online).
When online the lastVisibleActionCreated
is 0.263 seconds apart and makes hasNewestReportAction
false:
After synchronized (hasNewestReportAction
is true):
As far I can see this difference comes from the optimistic value of lastVisibleActionCreated:
Anything else please let me know 🙏🏼
As far I can see this difference comes from the optimistic value of lastVisibleActionCreated:
@hoangzinh based of this last comment, I have updated my proposal , please see the alternative part.
@samilabud Because both reportAction and report are all optimistic updated, is it possible to make reportActions[0]?.created === report.lastVisibleActionCreated
with no delay?
@samilabud Because both reportAction and report are all optimistic updated, is it possible to make
reportActions[0]?.created === report.lastVisibleActionCreated
with no delay?
I'm checking, but today the expensify services are down, maybe tomorrow I can give you an answer.
📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸
is it possible to make
reportActions[0]?.created === report.lastVisibleActionCreated
with no delay?
Of course, we should modify every method related to build optimistic when sending a new comment, split expense, submit expense, etc.
To send a new comment the created
date is assigned using same function used for lastVisibleActionCreated
DateUtils.getDBTimeWithSkew
(in our scenarios without a offset), this is the part of the code:
To add a comment we use the addAction
method:
https://github.com/Expensify/App/blob/9f512746783f2a9db6c2e8e74416605113fc7ab2/src/libs/actions/Report.ts#L439-L448
But this never going to match because the lastVisibleActionCreated
is created in different render stage of the ReportActions, so we should modify the buildOptimisticAddCommentReportAction to accept the currentTime
and use it to specify the created prop like:
created: currentTime ?? DateUtils.getDBTimeWithSkew(Date.now() + createdOffset),
To send a split expense we should modify createSplitsAndOnyxData to use the same currentTime like:
const currentTime = DateUtils.getDBTime();
splitChatReport.lastReadTime = currentTime;
splitChatReport.lastMessageText = splitIOUReportAction.message?.[0]?.text;
splitChatReport.lastMessageHtml = splitIOUReportAction.message?.[0]?.html;
splitChatReport.lastActorAccountID = currentUserAccountID;
splitChatReport.lastVisibleActionCreated = currentTime;
splitIOUReportAction.created = currentTime;
(For the last one it might be better to modify ReportUtils.buildOptimisticIOUReportAction
and getOrCreateOptimisticSplitChatReport
to accept currentTime
as a new parameter, but currently there are other mutations for splitChatReport.)
Test:
https://github.com/Expensify/App/assets/5216036/3bb3ee75-20aa-447b-8438-8b18d4ae1c85
@hoangzinh please let me know if this idea makes sense to you 🙏🏼.
Hi @samilabud just wanna double check because when I saw your recording above, after you create a money request, it doesn't auto scroll to bottom, does it?
Hi @samilabud just wanna double check because when I saw your recording above, after you create a money request, it doesn't auto scroll to bottom, does it?
Oh it seems like I did the scroll down with the mouse 😄😳, but yes it was automatically.
can you update your proposal with the root cause & solution based on that? Thanks
@samilabud we're getting close to the solution. Btw, I have feedbacks
But this never going to match because the lastVisibleActionCreated is created in different render stage of the ReportActions
What do you mean with "different render stage of the ReportAction"? Is it updated in a function, how render stage of ReportActions affect it? I think it should be something like because DateUtils.getDBTime() is called 2 times so it's different.
we're getting close to the solution
Nice and good news!, thanks for the feedbacks, thanks to them I have learned a lot about how Onyx and Expensify works.
What do you mean with "different render stage of the ReportAction"?
I mean the different calls in different times of each function, I have changed my words in the proposal, please check it out.
2. Can you update your "What is the root cause of that problem?
Root cause updated, thank you!
(I removed the alternative option and left only the one we are working with. )
@samilabud proposal https://github.com/Expensify/App/issues/40594#issuecomment-2086170714 looks good to me.
🎀👀🎀 C+ reviewed
Current assignee @puneetlath is eligible for the choreEngineerContributorManagement assigner, not assigning anyone new.
Hi everyone, I have created the PR, but still waiting for the offer.
Issue not reproducible during KI retests. (First week)
📣 @hoangzinh 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app!
📣 @samilabud 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!
Offer link Upwork job Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑💻 Keep in mind: Code of Conduct | Contributing 📖
FYI @samilabud the C+ (in this case @hoangzinh) provides the recommendation on who to hire, but you aren't actually hired until an Expensify internal engineer (in this case me) agrees.
FYI @samilabud the C+ (in this case @hoangzinh) provides the recommendation on who to hire, but you aren't actually hired until an Expensify internal engineer (in this case me) agrees.
oh ok, good to know, thanks!
This issue has not been updated in over 15 days. @puneetlath, @samilabud, @hoangzinh eroding to Monthly issue.
P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!
What's the status of the PR @samilabud @hoangzinh?
What's the status of the PR @samilabud @hoangzinh?
Ready to review since: https://github.com/Expensify/App/issues/40594#issuecomment-2106497524
Oh no my bad, was reviewed and waiting for: https://github.com/Expensify/App/pull/42030#issuecomment-2121084466
From what I can see you've been hired in Upwork. I'm more asking about the status of the PR.
about the status of the PR.
The PR is ready, and the review was done but my reviewer was asking about if we can merge this in https://github.com/Expensify/App/pull/42030#issuecomment-2113336521.
I don't pretty sure about the next steps, sorry.
@allroundexperts where do you think we go from here?
I think we can put this issue on hold for https://github.com/Expensify/App/issues/40724
@puneetlath the holding PR has been merged. It appears that our issue has been resolved. Can we ask QA to test this issue again?
https://github.com/Expensify/App/assets/9639873/247556f2-238e-4167-8a05-582099f2394b
Ok @hoangzinh so is any payment needed here? Or was this ultimately solved by a different PR?
If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!
Version Number: 1.4.63-0 Reproducible in staging?: Y Reproducible in production?: Y If this was caught during regression testing, add the test name, ID and link from TestRail: N/A Issue reported by: Applause - Internal Team
Issue found when executing PR https://github.com/Expensify/App/pull/39685
Action Performed:
Expected Result:
Expense chat scroll to bottom when send a message and create second money request
Actual Result:
Expense chat doesn't scroll to bottom when send a message and create second money request
Workaround:
Unknown
Platforms:
Which of our officially supported platforms is this issue occurring on?
Screenshots/Videos
Add any screenshot/video evidence
https://github.com/Expensify/App/assets/78819774/49293dbd-48de-4313-9051-0a49d2798fb8
View all open jobs on GitHub
Upwork Automation - Do Not Edit