Open lanitochka17 opened 1 month ago
Triggered auto assignment to @RachCHopkins (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.
@RachCHopkins FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors
Fine on web/desktop.
Same on iOS native app. Not just android/web, my guess is that it's anything with a mobile aspect ratio.
Job added to Upwork: https://www.upwork.com/jobs/~021838383119719075523
Triggered auto assignment to Contributor-plus team member for initial proposal review - @paultsimura (External
)
Edited by proposal-police: This proposal was edited at 2024-09-27 08:30:08 UTC.
When a whisper is received in the chat for the current user the screen is not scrolled down and the whisper is not visible until the user decides to scroll down.
The cause of the issue seems to be an intented behaviour in the logic of the scrolling for new actions. The current logic scrolls the user to the bottom if the action is from the current user, all other cases no scrolling is done. This logic is in ReportActionsList.tsx
in this code block:
const scrollToBottomForCurrentUserAction = useCallback(
(isFromCurrentUser: boolean) => {
// If a new comment is added and it's from the current user scroll to the bottom otherwise leave the user positioned where
// they are now in the list.
if (!isFromCurrentUser) {
return;
}
if (!hasNewestReportActionRef.current) {
Navigation.navigate(ROUTES.REPORT_WITH_ID.getRoute(report.reportID));
return;
}
InteractionManager.runAfterInteractions(() => reportScrollManager.scrollToBottom());
},
[reportScrollManager, report.reportID],
);
We have a few options depending on how the scrolling is intented to behave;
!isFromCurrentUser
check.isFromCurrentUser
check would be replaced by the following block: const newestAction = sortedVisibleReportActions[0];
const whisperedTo = ReportActionsUtils.getWhisperedTo(newestAction);
const transactionsWithReceipts = ReportUtils.getTransactionsWithReceipts(report.reportID);
const isWhisper = whisperedTo.length > 0 && transactionsWithReceipts.length === 0;
const isWhisperOnlyVisibleByUser = isWhisper && ReportUtils.isCurrentUserTheOnlyParticipant(whisperedTo);
const shouldScrollDown = isFromCurrentUser || isWhisperOnlyVisibleByUser;
if (!shouldScrollDown) {
return;
}
We need to change the approach to check the latest action in the chat and check if that is a whisper for the current user or not by changing this existing useEffect to this:
useEffect(() => {
const isWhisperForUser = isWhisperVisibleOnlyToUser(sortedVisibleReportActions[0])
const shouldScrollForWhisper = isWhisperForUser && hasNewestReportAction;
if (
(scrollingVerticalOffset.current < AUTOSCROLL_TO_TOP_THRESHOLD &&
previousLastIndex.current !== lastActionIndex &&
reportActionSize.current > sortedVisibleReportActions.length &&
hasNewestReportAction) || shouldScrollForWhisper
) {
reportScrollManager.scrollToBottom();
}
previousLastIndex.current = lastActionIndex;
reportActionSize.current = sortedVisibleReportActions.length;
}, [lastActionIndex, sortedVisibleReportActions, reportScrollManager, hasNewestReportAction, linkedReportActionID]);
In addition we need to fix how the hasNewestReportAction
is calculated by replacing hasNewestReportAction = sortedVisibleReportActions[0]?.created === report.lastVisibleActionCreated
with:
hasNewestReportAction = sortedVisibleReportActions[0]?.created >= report.lastVisibleActionCreated
. Up to this point we had assumed that the sortedVisibleReportActions[0]?.created is always equal to report.lastVisibleActionCreated but for some reason the optimistic data are a bit behind the actual BE.
@klajdipaja is correct β this looks intentional at the moment. However, it seems reasonable to also enable auto-scroll on the whispers as they are system messages addressed directly to the user and often require some action/attention.
πππ C+ reviewed to summon an Engineer for decision.
Triggered auto assignment to @blimpich, see https://stackoverflow.com/c/expensify/questions/7972 for more details.
π£ @paultsimura π An offer has been automatically sent to your Upwork account for the Reviewer role π Thanks for contributing to the Expensify app!
π£ @klajdipaja π 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 π
Agreed, lets got with solution 2 outlined in the proposal.
If possible, can we reach out the engineers who implemented this code originally to make sure they're aware of the changes we're making?
@marcaaron @Julesssss I see you were involved in https://github.com/Expensify/App/pull/536, where this comment appeared for the first time:
If a new comment is added and it's from the current user scroll to the bottom
Just a heads-up: we plan to add auto-scroll to the bottom when the user receives a whisper message. The reasoning: these are system messages addressed directly to the user and often require some action/attention. It will help draw the user's attention because currently you barely see that the new actionable message was received:
https://github.com/user-attachments/assets/e4f6ea0f-59ce-443a-b6dd-2af5b8c7662e
@paultsimura While testing my changes in android emulator I found out that there's an issue with my proposal. When the internet or device is really slow I saw that the scrolling does not happen.
The root cause of that is that the scrollToBottomForCurrentUserAction
is called as a callback after the API.write for the action that the user has executed but if the internet is slow the whisper message has not arrived yet.
We need to change the approach to check the latest action in the chat and check if that is a whisper for the current user or not by changing this existing useEffect to this:
useEffect(() => {
const isWhisperForUser = isWhisperVisibleOnlyToUser(sortedVisibleReportActions[0])
const shouldScrollForWhisper = isWhisperForUser && hasNewestReportAction;
if (
(scrollingVerticalOffset.current < AUTOSCROLL_TO_TOP_THRESHOLD &&
previousLastIndex.current !== lastActionIndex &&
reportActionSize.current > sortedVisibleReportActions.length &&
hasNewestReportAction) || shouldScrollForWhisper
) {
reportScrollManager.scrollToBottom();
}
previousLastIndex.current = lastActionIndex;
reportActionSize.current = sortedVisibleReportActions.length;
}, [lastActionIndex, sortedVisibleReportActions, reportScrollManager, hasNewestReportAction, linkedReportActionID]);
This is testing really well, you can check this out in this branch and let me know what you think
@klajdipaja please make a draft PR and tag me there. It's easier to work with
I've played with these ACTIONABLE_WHISPER
messages a little, and they look quite buggy. I will report separate bugs, and we may want to hold this one for them.
Duplicating the bugs here for clarity:
Bug 1:
@{user}
syntax;Expected: The report is scrolled to the bottom so the whisper action becomes visible. Actual: The report is not scrolled.
Bug 2:
@{user}
syntax;@{user}
mentionExpected: The "New messages" floating button should not appear as there is no new message Actual: The "New messages" floating button appears
Expected: The report's last message in LHN should remain @{user}
Actual: The report's last message in LHN changes to Expensify: Heads up...
https://github.com/user-attachments/assets/57d820e6-ce54-4c85-968a-b5bd533bd09c
@paultsimura did you check those with the draft PR #49699 I created? I have a feeling it will solve both issues but haven't had the time to double check
@klajdipaja no it doesn't fix those. From what I see, is doesn't fix the current one either.
@paultsimura Just to give you an update.
I did a lot of debugging with this and found out that there's an issue with the hasNewestReportAction
that I think it's rooted in the optimistic data but so far have not been able to exactly confirm this as that part of the codebase is masive.
I logged in the console the report.lastVisibleActionCreated
and I noticed that it is sometimes off by a few nanoseconds after the comment has been submitted and later when the opimistic data is updated it goes to the correct value. This means that the action.created
in this cases is bigger then the report.lastVisibleActionCreated
which is the change that I added into my draft branch and now it seems to be working as expected.
I replaced in my PR hasNewestReportAction = sortedVisibleReportActions[0]?.created === report.lastVisibleActionCreated
with:
hasNewestReportAction = sortedVisibleReportActions[0]?.created >= report.lastVisibleActionCreated
I have a recording where you can see the scroll to bottom working and the logs of the dates that I have placed within my useEffect but it's bigger then the 10MB limit github has. How did you get such a long recording within that size limit?
@klajdipaja the corresponding GH is here: https://github.com/Expensify/App/issues/49825 Please post a formal proposal for a solution there if you have one.
Updated proposal: https://github.com/Expensify/App/issues/49599#issuecomment-2370882042 Now I have included option 3 that is implemented in the Draft branch and covers the issue in this ticket
Regarding the slight difference of the lastVisibleActionCreated time can we have a BE engineer feedback how they set that time on their side? I found out the different value comes from the BE and it's also different to the clientCreatedTime which I was assuming would be what they use on their end to set the last:
I'm attaching request and response screenshots so that you can see the difference in the values. Request: Response:
Looking at the backend we mainly use clientCreatedTime
to determine whether or not the user has "read" the comment. I think lastVisibleActionCreated
corresponds to when the report action was created in the DB.
@klajdipaja I think the discrepancy comes from the fact that:
TL;DR: It looks like you're comparing the timestamps of 2 different report actions.
@blimpich let's hold for https://github.com/Expensify/App/issues/49825 and https://github.com/Expensify/App/issues/49824 π
@paultsimura, @blimpich, @klajdipaja, @RachCHopkins Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!
Holding
Changing to Weekly, since we're waiting on other issues.
Looks like we have PRs on the boil for both linked issues.
Not Overdue
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: 9.0.39.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
Action Performed:
Expected Result:
After mentioning another user that is not part of the chat, a whisper with options should be received, and the chat should be autoscrolled to the bottom
Actual Result:
When the whisper is received, after mentioning an user that is not part of the room, the chat is not autoscrolled to the bottom
Workaround:
Unknown
Platforms:
Which of our officially supported platforms is this issue occurring on?
Screenshots/Videos
Add any screenshot/video evidence
https://github.com/user-attachments/assets/931f56f7-bbad-42ba-9a1a-705d221436ff
View all open jobs on GitHub
Upwork Automation - Do Not Edit
Issue Owner
Current Issue Owner: @paultsimura