-
Notifications
You must be signed in to change notification settings - Fork 3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Feat: Display unread marker for messages that were received while offline #49480
Feat: Display unread marker for messages that were received while offline #49480
Conversation
This is ready for review! :) |
@roryabraham @chrispader Do you think we should work on the marker dismissal here? I can only dismiss if I enter another conversation and come back: Screen.Recording.2024-09-21.at.16.45.52.mov |
@roryabraham Can you confirm something please? Should this new feat of ours only work on the web or other platforms as well? Thank you. |
If so, i think this should be handled in a separate issue. The same behavior is the case for receiving messages in a report when it's not active. (also the case on Screen.Recording.2024-09-23.at.12.52.33.mov |
I understand, I also agree that taking this to another issue would be more appropriate. @chrispader I'm having problems with the marker, I'm "playing around" with two conversations and the behavior is still not consistent in my tests: Screen.Recording.2024-09-23.at.12.19.21.movI'll come back here tomorrow and test more, I'll clean everything up here, maybe it's something with my local. But, in your tests, did everything work correctly? Thank you. |
One thing that makes me wonder: Why did the right user receive the message from the left one before throttling was disabled (before it went back online)? That's weird. I can see that the users own messages are als recognized as "new messages". This shouldn't happen. I'm going to look into that! Other than that, i had no problems with the unread marker while testing |
Reviewer Checklist
Screenshots/VideosAndroid: Native49480_android_native.movAndroid: mWeb Chrome49480_android_web.moviOS: Native49480_ios_native.moviOS: mWeb SafariMacOS: Chrome / Safari49480_web_chorme.movMacOS: Desktop49480_web_desktop.mov |
@chrispader Indeed, sorry, ignore that report, it was an issue with my local. I tested the changes in all platforms, it seems fine so far. Did you have a chance to check "I can see that the users own messages are als recognized as "new messages". This shouldn't happen. I'm going to look into that!"? Thank you. |
Just fixed the issue 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I retested it and it looks good to me:
Screen.Recording.2024-09-26.at.15.14.09.mov
@chrispader Do you know why the "Performance Tests error" in the PR? Thanks.
No idea tbh :/ I don't think it's actually related to the changes. Had the same issue with a few other PRs |
I don't think we should introduce this feature at this time. |
I don't see any reason why this would be web-only. So all platforms 🙂 |
I've approved the PR but the performance tests are failing. |
SolutionThis logic has no control of whether the message has (actually) been created/sent while the sender was offline. Only messages received while the own device was offline will be "marked as unread" (green line). Therefore, we have two options on how to handle this in the UI. Approach 1Stick with the current implementation and only mark messages as unread, that have been received while the current user was offline. In case both users are offline while sending message, the user who is back online first will not see the "urnead marker". Approach 2Also mark messages as unread, that have been created while the sender was offline. For this, we will need to add an additional prop like I don't favor this approach though, since this would also mean, that all (online) users will receive "unread markers" (green lines) if some other user sends a message while offline. This might be very disturbing in groups especially. Curious about your thoughts cc @brunovjk @tylerkaraszewski |
Thanks for the detailed explanation, @chrispader. Apologies for any unnecessary extra work on this. I believe this is primarily an edge case and not reflective of typical user behavior, though I can't confirm that definitively (we'd need @tylerkaraszewski's input on that). From my perspective, we can proceed as is to avoid regressions. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, the changes seem solid and well thought out. I agree with proceeding, in my tests everything works well:
Android: Native
49480_android_native.mov
Android: mWeb Chrome
49480_android_web.mov
iOS: Native
49480_ios_native.mov
iOS: mWeb Safari
49480_ios_web.mov
MacOS: Chrome / Safari
49480_web_chrome.mov
MacOS: Desktop
49480_web_desktop.mov
I've read the scenario above and particularly point 9 is of interest:
This says that user A receives a message while online and thus the green line is not updated. I think that's fine, I don't think user A needs to care if user B was online or not when they initially tried to send it. When the message is actually able to be sent, user A is online, and the message arrives immediately. I think that's fine. That lets us go with the current solution, correct? |
Correct, technically the message is received while already back online. (Only) if both devices come back online at the same time, it might be perceived as weird, but i suppose no user will ever use two devices at the same time and text himself.
Yes, that sounds good! |
Great, we just need to resolve the conflicts to merge? Thanks. |
Resolved conflicts! |
Can we address the failing unit test? |
I'm not sure what is causing, if is indeed our PR, I will investigate and back here if I find something |
Sorry my bad, while restructuring the new checks in the changes from Tests should now succeed, @brunovjk @tylerkaraszewski |
Retesting |
Just tested it and still works on my machine :) |
Indeed, everything looks very good to me: Screen.Recording.2024-10-30.at.10.06.35.mov@tylerkaraszewski all yours. |
✋ 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/tylerkaraszewski in version: 9.0.57-0 🚀
|
🚀 Deployed to production by https://github.com/luacmartins in version: 9.0.57-10 🚀
|
@@ -1806,10 +1807,38 @@ function getReportActionsLength() { | |||
return Object.keys(allReportActions ?? {}).length; | |||
} | |||
|
|||
function wasActionCreatedWhileOffline(action: ReportAction, isOffline: boolean, lastOfflineAt: Date | undefined, lastOnlineAt: Date | undefined, locale: Locale): boolean { | |||
// The user was never online. | |||
if (!lastOnlineAt) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const wasMessageReceivedWhileOffline = useCallback( | ||
(message: OnyxTypes.ReportAction) => | ||
!ReportActionsUtils.wasActionTakenByCurrentUser(message) && | ||
ReportActionsUtils.wasActionCreatedWhileOffline(message, isOffline, lastOfflineAt.current, lastOnlineAt.current, preferredLocale), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Coming from #55560, We missed a case here, when we get an optimistic message from Concierge offline, we fixed it by returning false if the message is optimistic
Proposal: #55560 (comment)
PR: #56628
Details
Fixed Issues
$ #44007
PROPOSAL:
Tests
Offline tests
The description in "Tests" is already covering offline behaviour.
QA Steps
Same as in Tests.
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label and/or tagged@Expensify/design
so the design team can review the changes.ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
Screen.Recording.2024-09-21.at.20.49.28.mov
MacOS: Desktop
MacOS Chrome (Arc) screen recording attached.