Skip to content
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

[$1000] Jump to last unread chat when opening a report #35011

Open
6 tasks done
kbecciv opened this issue Jan 23, 2024 · 83 comments
Open
6 tasks done

[$1000] Jump to last unread chat when opening a report #35011

kbecciv opened this issue Jan 23, 2024 · 83 comments
Assignees
Labels
Internal Requires API changes or must be handled by Expensify staff NewFeature Something to build that is a new item. Weekly KSv2

Comments

@kbecciv
Copy link

kbecciv commented Jan 23, 2024

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.28.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:
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Expensify/Expensify Issue URL:
Issue reported by: @MonilBhavsar
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1705558628672569

Action Performed:

  1. A is participant of a chat or room
  2. As another user send some good number of messages to chat report, so new messages can be scrolled up and down
  3. As user A, open the chat report

Expected Result:

The chat report view opens from where the messages are unread and user can scroll down

Actual Result:

The chat report view is scrolled down to the last message and user needs to scroll up to find last read message

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

  • Android: Native
  • Android: mWeb Chrome
  • iOS: Native
  • iOS: mWeb Safari
  • MacOS: Chrome / Safari
  • MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

Recording.5931.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01fc144b2f2f5df388
  • Upwork Job ID: 1749920553887232000
  • Last Price Increase: 2024-06-11
  • Automatic offers:
    • ikevin127 | Contributor | 0
    • ishpaul777 | Contributor | 102670752
Issue OwnerCurrent Issue Owner: @roryabraham
@kbecciv kbecciv added Daily KSv2 Needs Reproduction Reproducible steps needed Bug Something is broken. Auto assigns a BugZero manager. labels Jan 23, 2024
Copy link

melvin-bot bot commented Jan 23, 2024

Triggered auto assignment to @trjExpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

@kbecciv kbecciv added External Added to denote the issue can be worked on by a contributor and removed Needs Reproduction Reproducible steps needed labels Jan 23, 2024
@melvin-bot melvin-bot bot changed the title Chat - When opening a chat with lots of unread messages user needs to scroll up to find last read message [$500] Chat - When opening a chat with lots of unread messages user needs to scroll up to find last read message Jan 23, 2024
Copy link

melvin-bot bot commented Jan 23, 2024

Job added to Upwork: https://www.upwork.com/jobs/~01fc144b2f2f5df388

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Jan 23, 2024
Copy link

melvin-bot bot commented Jan 23, 2024

Triggered auto assignment to Contributor-plus team member for initial proposal review - @mollfpr (External)

@jeremy-croff
Copy link
Contributor

This PR #28793 specifically makes it scrolled to the bottom, seems like NAB

@trjExpensify
Copy link
Contributor

trjExpensify commented Jan 24, 2024

I'm a bit confused by the bug report @MonilBhavsar, so asking about it here. I thought this was something we'd implement via comment linking.

@roryabraham roryabraham changed the title [$500] Chat - When opening a chat with lots of unread messages user needs to scroll up to find last read message [HOLD #30269] [$500] Chat - When opening a chat with lots of unread messages user needs to scroll up to find last read message Jan 24, 2024
@roryabraham
Copy link
Contributor

HOLD for comment linking: #30269

@roryabraham roryabraham added NewFeature Something to build that is a new item. and removed Help Wanted Apply this label when an issue is open to proposals by contributors Bug Something is broken. Auto assigns a BugZero manager. labels Jan 24, 2024
Copy link

melvin-bot bot commented Jan 24, 2024

Current assignee @trjExpensify is eligible for the NewFeature assigner, not assigning anyone new.

@melvin-bot melvin-bot bot added Weekly KSv2 and removed Daily KSv2 labels Jan 24, 2024
@melvin-bot melvin-bot bot added the Overdue label Feb 2, 2024
@trjExpensify
Copy link
Contributor

No change, still on hold Melv!

@melvin-bot melvin-bot bot removed the Overdue label Feb 5, 2024
@melvin-bot melvin-bot bot added the Overdue label Feb 13, 2024
@trjExpensify
Copy link
Contributor

Samesies, Melv!

@melvin-bot melvin-bot bot removed the Overdue label Feb 14, 2024
@melvin-bot melvin-bot bot added the Overdue label Feb 23, 2024
@trjExpensify
Copy link
Contributor

Still on hold, comment linking got held for the deploy freeze last week.

@trjExpensify
Copy link
Contributor

Taking the issue off hold.

@ishpaul777
Copy link
Contributor

we are discussing this on slack

@roryabraham roryabraham added Internal Requires API changes or must be handled by Expensify staff and removed External Added to denote the issue can be worked on by a contributor Help Wanted Apply this label when an issue is open to proposals by contributors labels Jul 18, 2024
@roryabraham
Copy link
Contributor

My suggestion for next steps:

  • Right now, when OpenReport is called w/o any specific reportActionID as an anchor, we essentially return the 50 newest visible actions on the report. We'll want to update that such that it looks at the clientLastReadTime (already passed by App) and use that to determine the anchor action. It will then return 25 messages before and 25 messages after that anchor, and the client will render those
  • Then we already have some logic to fetch newer actions as the user scrolls down

I'll need to test carefully to make sure that making the back-end changes won't break NewDot without any changes. If it won't then we're good here. If it will, some additional steps and a more careful rollout might be needed.

@trjExpensify
Copy link
Contributor

What's the latest here?

@melvin-bot melvin-bot bot removed the Overdue label Jul 30, 2024
@melvin-bot melvin-bot bot added the Overdue label Aug 7, 2024
@ishpaul777
Copy link
Contributor

any updates here? @roryabraham

@melvin-bot melvin-bot bot removed the Overdue label Aug 7, 2024
@melvin-bot melvin-bot bot added the Overdue label Aug 16, 2024
@roryabraham
Copy link
Contributor

Was looking into this, chats are fetched in OpenReport from Web-E here

@melvin-bot melvin-bot bot removed the Overdue label Aug 23, 2024
@roryabraham
Copy link
Contributor

roryabraham commented Aug 27, 2024

ok, I tried just making the change to pass the clientLastReadTime as the anchor action, but it didn't work. Seems like NewDot changes will be needed. The new message indicator appeared below the message I marked as unread, and it looked like we only returned actions before that point, not before and after.

On top of that, the main issue was that the reportActionsPages_ Onyx key was populated assuming that we were at the end of the chat. In addition, the IOU actions seem to have been fetched and delivered to the front-end separately.

@roryabraham
Copy link
Contributor

So one thing I'm seeing is that this code is problematic. Because we don't have access to the lastReadReportActionID. (only the lastReadTime, which will tend to be more reliable, because what if the lastReadReportActionID were deleted while you were offline or something?).

Basically, that code was built with the (then-correct) assumption that initially opening a report without a cursor action would mean that you'll get the latest actions. However, this issue seeks to change that. To correctly track pagination with this change, we need to return some additional pagination metadata. Fortunately, I've already got an issue for that here. So I'm going to put this on HOLD for that

@roryabraham roryabraham changed the title [$1000] Chat - When opening a chat with lots of unread messages user needs to scroll up to find last read message [HOLD #41153][$1000] Jump to last unread chat when opening a report Aug 27, 2024
@melvin-bot melvin-bot bot added the Overdue label Sep 4, 2024
@roryabraham
Copy link
Contributor

still working on #41153

@melvin-bot melvin-bot bot removed the Overdue label Sep 4, 2024
@melvin-bot melvin-bot bot added the Overdue label Sep 13, 2024
@ishpaul777
Copy link
Contributor

Not overdue #41153 is WIP

@trjExpensify
Copy link
Contributor

Yep, agreed!

@roryabraham
Copy link
Contributor

All the PRs for the back-end of #41153 have been merged and should go to prod on Wednesday 2024-09-18

@melvin-bot melvin-bot bot removed the Overdue label Sep 16, 2024
@roryabraham roryabraham changed the title [HOLD #41153][$1000] Jump to last unread chat when opening a report [$1000] Jump to last unread chat when opening a report Sep 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Internal Requires API changes or must be handled by Expensify staff NewFeature Something to build that is a new item. Weekly KSv2
Projects
Status: Todo
Development

No branches or pull requests

10 participants