Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.52k stars 2.87k forks source link

[$250] Initial switch to chat is jarring #48762

Open m-natarajan opened 1 month ago

m-natarajan commented 1 month ago

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.29-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: @flodnv Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1725455595449419

Action Performed:

Maybe pre-req: Switch to focus mode

  1. Sign out and back in
  2. Use the chat switcher to go to a chat with many messages
  3. Observe jarring transition between empty state and full message view

    Expected Result:

    The transition should be smoother, perhaps with a loading skeleton or something.

    Actual Result:

    It feels like the product blinks between screens

    Workaround:

    Unknown

    Platforms:

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

    • [x] Android: Native
    • [ ] Android: mWeb Chrome
    • [ ] iOS: Native
    • [ ] iOS: mWeb Safari
    • [ ] MacOS: Chrome / Safari
    • [x] MacOS: Desktop

Screenshots/Videos

https://github.com/user-attachments/assets/2aa471d0-5bd7-4a7e-a06e-8e8046f08971

https://github.com/user-attachments/assets/faa85060-386b-42e9-aba1-95dd12e138fa

Add any screenshot/video evidence

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~021833309825524574048
  • Upwork Job ID: 1833309825524574048
  • Last Price Increase: 2024-09-10
Issue OwnerCurrent Issue Owner: @Ollyws
melvin-bot[bot] commented 1 month ago

Triggered auto assignment to @mallenexpensify (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.

flodnv commented 1 month ago

I also experienced this bug on Android Native today.

melvin-bot[bot] commented 1 month ago

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

melvin-bot[bot] commented 1 month ago

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

mallenexpensify commented 1 month ago

I don't think I was able to reproduce on Desktop, @Ollyws can you attempt reproduction and post your findings? Thx

Ollyws commented 1 month ago

I don't see any issue here, the skeleton is loading correctly for me.

flodnv commented 1 month ago

Did you try in focus mode?

What else can I do to help diagnose this? I encounter it several times a week...

puneetlath commented 1 month ago

I think there's not really a bug here, but @flodnv is proposing that we make this UI transition smoother somehow, right?

muttmuure commented 1 month ago

This seems to be the same issue with how the optimistic chat load is jarring. I think what Flo is seeing is:

  1. Switch Chats
  2. You see an optimistic chat first
  3. Then you see the loading skeleton
  4. Then you see the full chat history
muttmuure commented 1 month ago

That is weird, and inconsistent. I thought we established the correct pattern here:

https://github.com/Expensify/App/issues/43791

And the expected behavior is here:

https://expensify.slack.com/archives/C05LX9D6E07/p1718644857351619?thread_ts=1718365848.007159&cid=C05LX9D6E07

puneetlath commented 1 month ago

I thought we established the correct pattern here: https://github.com/Expensify/App/issues/43791

That is the case when you are clicking on a report in the LHN that you already have locally. Whereas the "jarring" effect being referred to here is when you don't already have the chat with someone locally. So instead what is happening is that you are:

And the expected behavior is here: https://expensify.slack.com/archives/C05LX9D6E07/p1718644857351619?thread_ts=1718365848.007159&cid=C05LX9D6E07

I don't think this is really possible in the scenario described above because the client doesn't know that there is an existing report until the API responds. If we were to disallow chatting until the API responds, then you'd never be able to start a chat with someone offline if you don't already have a chat with them locally.

muttmuure commented 1 month ago

Got it, yeah that makes sense.

So I think I'm again landing on what I asked a few days back:

image

It sounds like @flodnv agrees, we could propose some ways to make the transition "less blinky"

flodnv commented 1 month ago

I think at the very least we should show the newly loaded messages gently appearing at the bottom of the current screen, instead of making it feel like the screen is completely removed and added back.

muttmuure commented 1 month ago

that's a good idea, it would be cool if we didn't render the full report window component, just added the messages as they are loaded

puneetlath commented 1 month ago

Yeah I like that idea too. @muttmuure this seems like a good one for an agency.

melvin-bot[bot] commented 1 month ago

Triggered auto assignment to @isabelastisser (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.

mallenexpensify commented 1 month ago

@isabelastisser , I'm out til Tuesday, can you keep 👀 on this plz? Just posted in #expensify-callstack looking for a volunteer too.

muttmuure commented 1 month ago

@perunt and @chrispader are going to take a look at this

chrispader commented 1 month ago

I'm going to look into this either tmrw or the day after!

perunt commented 1 month ago

I'm starting on this task and want to detail a few points here.

I think at the very least we should show the newly loaded messages gently appearing at the bottom of the current screen, instead of making it feel like the screen is completely removed and added back.

So initially we can have multiple states: a) Chat was just created and it's empty b) Chat already exists and it's empty or has a few messages so you can see the background picture c) Chat already exists and it has content more than the height of the screen and you can't see the background picture

Gently adding new messages would work only for 'a' and 'b' cases. With 'Gently adding', I understand you mean something with animation, right? In that case, the background image that's showing only at the beginning of the chat would create another 'jumpy' effect when a lot of messages are added.

For now, we don't have such a pattern as adding messages with animation. I think this should be discussed with @shawnborton as it would change the general perception of the app. Don't get me wrong, I'm not against this, but it definitely needs to be agreed upon with the design team.

I have a few ideas and would like to hear your thoughts on this:

  1. If you look closer, you would see that when we see the skeleton, it shows even in the top bar where you can see info about the user. I would suggest keeping that space without skeleton as we already have data about it. It would decrease the 'jumpy' effect a bit.

  2. If we don't have any messages inside, then show skeleton from the very beginning until the report is loaded (as suggested by @muttmuure). image

@muttmuure @flodnv @puneetlath

melvin-bot[bot] commented 1 month ago

@Ollyws, @chrispader, @mallenexpensify, @isabelastisser Whoops! This issue is 2 days overdue. Let's get this updated quick!

isabelastisser commented 1 month ago

Not overdue!

melvin-bot[bot] commented 1 month ago

@Ollyws @chrispader @mallenexpensify @isabelastisser 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!

puneetlath commented 1 month ago

So initially we can have multiple states: a) Chat was just created and it's empty b) Chat already exists and it's empty or has a few messages so you can see the background picture c) Chat already exists and it has content more than the height of the screen and you can't see the background picture

@perunt I don't know if these states actually apply to the case in question here. The "jumpy" scenario happens when:

  1. be in focus mode
  2. have DM'd with userA before
  3. not have the userA DM in your LHN
  4. sign out and sign in
  5. then try to start a chat with them

You will experience the "jump" because we will first create an empty optimistic chat with that user, but when the back-end returns the existing chat, we redirect to it.

perunt commented 1 month ago

I just mentioned a more detailed scenario about what chat you can land on when opening a chat through search (and you don't have that chat in the LHN)

that's a good idea, it would be cool if we didn't render the full report window component, just added the messages as they are loaded

My main concern was that if we open such a chat, we show the user an empty page like this: image Then, we upload new data and add new messages to the bottom. If we have a lot of messages, the user will see somewhat random (from their perspective) scrolling. This would look a bit weird.

So, my idea was to start with a small optimization. Take a look at this screenshot: Screenshot 2024-09-23 at 16 29 38 What we do here:

  1. Show the user an empty page with proper info in the header and composer.
  2. Show a skeleton in the area where messages should appear. Show a skeleton in the header and remove the composer.
  3. Bring back the header with proper info and bring back the composer.
  4. End state when we render all information.

My suggestion was that if we show some information in the header, we shouldn't replace it with a skeleton, as this info (user name) is actual information (look at the second picture). If we have anything to show(user name + avatar), we can show it immediately and not replace it with a skeleton when fetching the chat messages. Also, on the first screen, we show the composer, then remove it, and then show it again. We could maybe keep the composer or render a skeleton for the composer that looks like a composer. This will reduce the flickering effect a lot.

Also, the second idea was to use the approach suggested by @muttmuure here: https://github.com/Expensify/App/issues/48762#issuecomment-2343344892, which says to show a skeleton instead of the empty page in case we don't have any content to show.

puneetlath commented 1 month ago

Ahh I see. Thanks for the explanation. Ok I like the ideas of keeping the header and composer as-is during the transition for sure. Maybe we could start with just that and see how it feels before deciding whether to do to more?

Also, it looks like the API response that tells you that there is a pre-existing report doesn't also return the data for the pre-existing report. So we make another OpenReport call to get that data after re-routing. Would it be easier to improve the front-end experience if we returned the data for the pre-existing report in the first API call? As though you had called OpenReport with the correct reportID in the first place.

melvin-bot[bot] commented 1 month ago

@Ollyws, @chrispader, @mallenexpensify, @isabelastisser 6 days overdue. This is scarier than being forced to listen to Vogon poetry!

perunt commented 1 month ago

Sure, I'll start with keeping the header and composer in their place and with the same info.

It happens because we use preexistingReportID here, as we don't have much info about this report. I would like us to include the reportID in the LHN data list, but I understand that it might make the search a bit slower

puneetlath commented 1 month ago

We can return more data about that pre-existing report in the OpenReport response. Should we do that?

perunt commented 1 month ago

No need for it, thanks. I found the issue. Now it looks like this:

https://github.com/user-attachments/assets/e10ec3fa-4eb1-4bb8-a413-7a7d32111654

puneetlath commented 1 month ago

Wow, nice!!

melvin-bot[bot] commented 1 month ago

Triggered auto assignment to @dubielzyk-expensify (Design), see these Stack Overflow questions for more details.

flodnv commented 1 month ago

That's much better! I wonder how we can make it feel more smooth though 🤔 Any ideas @dubielzyk-expensify ?

dubielzyk-expensify commented 1 month ago

If I'm understanding correctly, I think the problem is that it shows the beginning of chat -> skeleton loader -> fully loaded chat.

By removing even one of these it'd be a lot smoother. So going from beginning of chat -> fully loaded or directly from skeleton loader -> fully loaded chat (though this one might not be what we want because every chat might feel like it's loading).

The other thing we could do is a very quick fade between the skeleton loader and fully loaded content:

https://github.com/user-attachments/assets/4a24fb6c-043b-4d61-b740-aef0de10717c

cc @Expensify/design

dannymcclain commented 1 month ago

I love the idea of a quick fade. And I agree that removing one of the "steps" would likely help, but maybe that option isn't on the table for this.

Ollyws commented 4 weeks ago

What's the concensus here? From a design perspective are we happy to go forward with @perunt's most recent changes for now, or should we further refine the transition?

dannymcclain commented 3 weeks ago

I think the proposal is definitely an improvement, but I still wonder why we show the "beginning of chat" at all? Seems like showing that is causing an unnecessary flash of content to me. Why not just show Header + composer with skeletons in between > correct messages / beginning of chat (i.e. properly loaded data). Why do we have to start with the beginning of chat always even if we don't know if that's the correct thing to display? cc @dubielzyk-expensify

puneetlath commented 3 weeks ago

I think we should go forward with @perunt current changes while we continue to discuss further improvements.

@dannymcclain I think that would mean that we show the skeleton permanently if you're offline. The current behavior allows for offline sending (by naively assuming it's a new chat if it doesn't have the data locally).

dannymcclain commented 3 weeks ago

Oh gotcha! I see. Seems like there's more things to consider than I originally thought 😅

Down to implement @perunt's changes now and continue to think of improvements like you said.

puneetlath commented 3 weeks ago

Maybe we could do what you say if we detect that you are online. But only go with the naive approach of showing the empty chat if you are offline.

dubielzyk-expensify commented 3 weeks ago

Yeah I actually like doing some sort of detection cause I think the video above is quite jarring and perhaps it's fair to optimize it for the online usecase.

perunt commented 2 weeks ago

So, where were we, guys? Should I add something, or is it good to go?

muttmuure commented 1 week ago

I would like us to keep working on this

muttmuure commented 1 week ago

@perunt is your change ready for review?

puneetlath commented 1 week ago

I think we should start with the current changes shown here: https://github.com/Expensify/App/issues/48762#issuecomment-2374558089. That'll be a good improvement. And then we can continue to discuss further improvements from there.

perunt commented 1 week ago

I think we should start with the current changes shown here: https://github.com/Expensify/App/issues/48762#issuecomment-2374558089. That'll be a good improvement. And then we can continue to discuss further improvements from there.

These changes have been implemented here https://github.com/Expensify/App/pull/49731 and this PR is ready for review

puneetlath commented 1 week ago

@Ollyws shall we go ahead and take that forward?

muttmuure commented 1 week ago

@perunt please could you take a look at the merge conflicts?

melvin-bot[bot] commented 5 days ago

This issue has not been updated in over 15 days. @Ollyws, @chrispader, @mallenexpensify, @isabelastisser, @dubielzyk-expensify 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!

mallenexpensify commented 4 days ago

@perunt 👀 on the merge conflicts plz