[HOLD ON #33725] [$500] Compose-Sending message page flickers & sending link page shows loading for a second

izarutskaya commented 10 months 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: 1.3.92-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: Applause-Internal Team Slack conversation: @

Action Performed:

Launch app
Tap on a report
Send any message
Send any url

Expected Result:

Sending message page must not flicker & sending link page must not show loading for a second also.

Actual Result:

Sending message page flickers & sending link page shows loading for a second

Workaround:

Unknown

Platforms:

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

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

Screenshots/Videos

Android: Native

https://github.com/Expensify/App/assets/115492554/5f34d223-8b10-4562-8794-79fae203a261

Android: mWeb Chrome

iOS: Native

iOS: mWeb Safari

MacOS: Chrome / Safari

MacOS: Desktop

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~01e22aad449d431bed
Upwork Job ID: 1717861795790860288
Last Price Increase: 2023-11-10
Automatic offers:

0xmiroslav | Reviewer | 27654846

melvin-bot[bot] commented 10 months ago

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

melvin-bot[bot] commented 10 months ago

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

melvin-bot[bot] commented 10 months ago

Bug0 Triage Checklist (Main S/O)

[ ] This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
[ ] This bug is not a duplicate report (check E/App issues and #expensify-bugs)
- If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
[ ] This bug is reproducible using the reproduction steps in the OP. S/O
- If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
- If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
[ ] This issue is filled out as thoroughly and clearly as possible
- Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
[ ] I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

melvin-bot[bot] commented 10 months ago

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

tienifr commented 10 months ago

Proposal

Please re-state the problem that we are trying to solve in this issue.

Sending link page shows loading for a second

What is the root cause of that problem?

We'll show the loading in the bottom of the screen whenever we're loading new messages, when we scroll to the bottom of the screen, it will trigger onStartReached which will trigger the loading of newest chats.

The problem is when we send a new message to the chat ourselves, we'll also trigger scrolling to the bottom here, which will also trigger the loading. That's why we see the loading after sending the message, and the flicker.

What changes do you think we should make in order to solve the problem?

We should not load new chats when the scroll to bottom happens due to automatic scrolling (when we send a new message to the chat ourselves).

We need to set a variable/ref like isAutomaticScrollToBottom to true before we trigger scrolling to bottom, in loadNewerChats, if we see isAutomaticScrollToBottom is true, we'll set it to false, and early return (do not load the newest chats).

What alternative solutions did you explore? (Optional)

NA

melvin-bot[bot] commented 10 months ago

@bfitzexpensify, @0xmiroslav Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

bfitzexpensify commented 10 months ago

How does the proposal in https://github.com/Expensify/App/issues/30495#issuecomment-1783728777 look @0xmiroslav?

melvin-bot[bot] commented 10 months ago

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

0xmiros commented 10 months ago

@tienifr are you still able to reproduce?

tienifr commented 10 months ago

Yes, I can still reproduce

perunt commented 10 months ago

@tienifr, you had mentioned this as a copy of another issue. Here are my thoughts on that. https://github.com/Expensify/App/issues/30729#issuecomment-1794372264

melvin-bot[bot] commented 10 months ago

@bfitzexpensify, @0xmiroslav Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

bfitzexpensify commented 10 months ago

@perunt just to clarify, you see these as two separate issues requiring different solutions, correct? Your comment seems to suggest that @tienifr's solution here wouldn't fully solve the issue reported in https://github.com/Expensify/App/issues/30729

perunt commented 10 months ago

This could be resolved with one PR. The main issue is that onStartReached is automatically called each time we scroll to the bottom (to the newest message). This automatic scroll is triggered because if you scroll slightly and then send a message, the sent message should come into focus – that's the current behavior. Right now, this automatic scrolling is managed by autoscrollToTopThreshold (that's why removing reportScrollManager.scrollToBottom() doesn't broke anything). However, I'm not sure if this will work well with the upcoming Comment Linking feature. Therefore, I would suggest preventing onStartReached from being called during automatic scrolls to the end, instead of disabling the automatic scroll altogether

melvin-bot[bot] commented 10 months ago

@bfitzexpensify @0xmiroslav 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!

melvin-bot[bot] commented 10 months ago

@bfitzexpensify @0xmiroslav 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!

melvin-bot[bot] commented 10 months ago

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

melvin-bot[bot] commented 10 months ago

@bfitzexpensify, @0xmiroslav Huh... This is 4 days overdue. Who can take care of this?

melvin-bot[bot] commented 10 months ago

@bfitzexpensify, @0xmiroslav Eep! 4 days overdue now. Issues have feelings too...

bfitzexpensify commented 10 months ago

OK, given that we'll need a unique solution for this problem, how does the proposal in https://github.com/Expensify/App/issues/30495#issuecomment-1783728777 look @0xmiroslav?

roryabraham commented 10 months ago

Signing on for this as it's a dupe of https://github.com/Expensify/App/issues/30729

melvin-bot[bot] commented 10 months ago

📣 @0xmiroslav 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app!

Offer link Upwork job

melvin-bot[bot] commented 10 months ago

@bfitzexpensify, @roryabraham, @perunt, @0xmiroslav 6 days overdue. This is scarier than being forced to listen to Vogon poetry!

0xmiros commented 10 months ago

Not overdue

bfitzexpensify commented 10 months ago

@roryabraham you mentioned this is a dupe of https://github.com/Expensify/App/issues/30729, which has more progress. Should we keep both of these open?

tienifr commented 10 months ago

Therefore, I would suggest preventing onStartReached from being called during automatic scrolls to the end, instead of disabling the automatic scroll altogether

@perunt Just want to confirm your mean here. My solution does not prevent disabling the automatic scroll, I just prevent loadNewerChats when isAutomaticScrollToBottom is true. Did you understand my solution like that?

melvin-bot[bot] commented 10 months ago

@bfitzexpensify @roryabraham @perunt @0xmiroslav this issue is now 4 weeks old and preventing us from maintaining WAQ, can you:

Decide whether any proposals currently meet our guidelines and can be approved as-is today
If no proposals meet that standard, please take this issue internal and treat it as one of your highest priorities
If you have any questions, don't hesitate to start a discussion in #expensify-open-source

Thanks!

melvin-bot[bot] commented 10 months ago

Current assignee @0xmiroslav is eligible for the Internal assigner, not assigning anyone new.

melvin-bot[bot] commented 9 months ago

@bfitzexpensify, @roryabraham, @perunt, @0xmiroslav Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

melvin-bot[bot] commented 9 months ago

This issue has not been updated in over 15 days. @bfitzexpensify, @roryabraham, @perunt, @0xmiroslav 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!

roryabraham commented 8 months ago

This should be fixed now, requesting retest: https://expensify.slack.com/archives/C9YU7BX5M/p1703721450532629

mvtglobally commented 8 months ago

Not repro on Galaxy, but seem to be still repro on Pixel 6/ Android 14

https://github.com/Expensify/App/assets/43995119/de9ce021-a93e-4219-8c81-15fb9d53ffb7

https://github.com/Expensify/App/assets/43995119/57fe8722-829c-462c-91a4-539318bcb4e0

roryabraham commented 8 months ago

So we are no longer seeing a flickering loading spinner, which (I believe) was a contributor to this problem, but it seems there's some other problem as well. Maybe something causing a remount of the chat list, which flickers only on lower-end devices.

roryabraham commented 8 months ago

I'm not sure about the best path forward here, but I'm tempted to suggest that we just sit on this problem for now, and look to a hopefully not-too-distant future when we migrate the main chat list from FlatList to FlashList

bfitzexpensify commented 8 months ago

Cool, putting this on hold for https://github.com/Expensify/App/issues/33725

roryabraham commented 8 months ago

Still on HOLD

roryabraham commented 8 months ago

Making this a monthly in the meantime

melvin-bot[bot] commented 8 months ago

Triggered auto assignment to Contributor Plus for review of internal employee PR - @hoangzinh (Internal)

mallenexpensify commented 8 months ago

@hoangzinh reassigning, please take over as C+. If you don't have bandwidth, unassign yourself. Thanks

roryabraham commented 6 months ago

still on HOLD

bfitzexpensify commented 5 months ago

Still held

roryabraham commented 4 months ago

Still held

bfitzexpensify commented 3 months ago

Still held.

bfitzexpensify commented 2 months ago

Still on hold for https://github.com/Expensify/App/issues/33725

bfitzexpensify commented 1 month ago

Remains held

roryabraham commented 1 week ago

still on HOLD

Expensify / App