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

Chat - After specific formatted message is sent, new messages appears above it #9438

Closed
kbecciv opened this issue Jun 14, 2022 · 18 comments
Closed
Assignees
Labels
Engineering Improvement Item broken or needs improvement. Internal Requires API changes or must be handled by Expensify staff Monthly KSv2

Comments

@kbecciv
Copy link

kbecciv commented Jun 14, 2022

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!


Action Performed:

  1. Go to URL https://staging.new.expensify.com/
    and login with credentials
  2. Open any chat and copy/paste specific formatted text from the "Additional Environment Info" field
  3. Try to send another message
  4. Repeat steps 2-3 if needed

Additional information:

Architecture


_
Architectural diagram
Windows 95 was designed to be maximally compatible with existing MS-DOSand 16-bit Windows programs and device drivers while offering a more stable and better performing system.[12]%5B%5B13%5D%5D(https://wiki2.org/en/Windows_95%23cite_note-w95-archi-13
) The Windows 95 architecture is an evolution of Windows for Workgroups' 386 enhanced mode.Configuration Manager (CONFIGMG)Responsible for implementing Plug and Play functionality; monitoring hardware configuration changes; detecting devices using bus enumerators_; and allocating I/O ports, IRQs, DMA channels and memory in a conflict-free fashion.[14]Installable File System Manager (Input/Output Subsystem)Coordinates access to supported file systems. Windows 95 initially shipped with support for FAT12, FAT16, the VFAT extension, ISO 9660 (CDFS), Joliet and network redirectors, with later releases supporting FAT32.[15]

Expected Result:

All sent messages should be displayed in the correct order

Actual Result:

Once specific formatted message is sent - all new sent messages appears below that message

Workaround:

Unknown

Platform:

Where is this issue occurring?

  • Web
  • iOS
  • Android
  • Desktop App
  • Mobile Web

Version Number: 1.1.76.2

Reproducible in staging?: Yes

Reproducible in production?: Yes

Email or phone of affected tester (no customers): yuriy.turbo+ch11@gmail.com/Aa111111

Logs: https://stackoverflow.com/c/expensify/questions/4856

Notes/Photos/Videos: Any additional supporting documentation

Bug5608473_Screen_Recording_2022-06-14_at_01.58.35.mp4

Expensify/Expensify Issue URL:

Issue reported by: Applause - Internal Team

Slack conversation:

View all open jobs on GitHub

@melvin-bot
Copy link

melvin-bot bot commented Jun 14, 2022

Triggered auto assignment to @thienlnam (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

@thienlnam
Copy link
Contributor

Can easily reproduce on web sending the text snippet on production.

This only affects the person sending the message not the person receiving it.

I'm not seeing any indication the message didn't send successfully, checked the logs and nothing really stood out - the last message shown in the LHN is the correct message

Screen Shot 2022-06-14 at 9 21 46 AM

I'm also noticing that in the Onyx data, the stuck messages have indexes that are much higher than the other messages. This is likely why they keep getting pushed to the top. However, if you refresh the onyx data for that chat then it works as expected.

@thienlnam
Copy link
Contributor

It seems to be stuck as an optimisticID, and does not refresh correctly with the correct reportActionID once it is sent

@thienlnam
Copy link
Contributor

I am pretty sure this can be external, would start investigation here

@thienlnam thienlnam added the External Added to denote the issue can be worked on by a contributor label Jun 14, 2022
@melvin-bot
Copy link

melvin-bot bot commented Jun 14, 2022

Triggered auto assignment to @NicMendonca (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

@NicMendonca
Copy link
Contributor

@melvin-bot melvin-bot bot added Weekly KSv2 and removed Daily KSv2 labels Jun 14, 2022
@melvin-bot
Copy link

melvin-bot bot commented Jun 14, 2022

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

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Jun 14, 2022
@melvin-bot
Copy link

melvin-bot bot commented Jun 14, 2022

Current assignee @thienlnam is eligible for the Exported assigner, not assigning anyone new.

@melvin-bot melvin-bot bot changed the title Chat - After specific formatted message is sent, new messages appears above it [$250] Chat - After specific formatted message is sent, new messages appears above it Jun 14, 2022
@allroundexperts
Copy link
Contributor

Proposal

Issue

There are two reasons which are causing this issue:

  1. Network.post request in Report_AddComment function returns promise that never resolves. The network tab shows the request to be successful with 200 code but the promise never resolves. This happens for requests that go into the sequential queue. This causes the following code to never run as the promise never resolves:
    https://github.com/Expensify/App/blob/main/src/libs/actions/Report.js#L1012

  2. The API is not pushing the comment back to the client App. This can be clearly seen here:
    https://user-images.githubusercontent.com/30054992/174401466-a0fd1c0c-59ed-436f-8897-2b173ebe0012.mp4
    The websocket message does come back if the comment is plain text. But for the markdown text, the websocket does not return the comment back.

Solution

The easiest solution is to fix the WS API so that it returns the comment for markdown types. If that is not possible (It should be really), then we can refactor the Network.Post request so that the promise resolves even in the sequential queue. IMO, the second suggestion is a separate fix which should have a separate bug.

@melvin-bot melvin-bot bot added Daily KSv2 Overdue and removed Weekly KSv2 labels Jun 17, 2022
@NicMendonca
Copy link
Contributor

@mananjadhav bump on the proposal review 👆

@melvin-bot melvin-bot bot removed the Overdue label Jun 20, 2022
@mananjadhav
Copy link
Collaborator

mananjadhav commented Jun 20, 2022

I am going to be reviewing did review @allroundexperts's comments today. But I can't find any specific proposal here which describe what changes will be made and where.

This causes the following code to never run as the promise never resolves

This is the same function used for non-formatted text as well. Are you saying the issue exists for any type of comment? As per the GH body, it seems it is a problem only for formatted text.

easiest solution is to fix the WS API so that it returns the comment for markdown types

Are you suggesting that for the formatted text the API doesn't return any response? Even after success? and is that true only for formatted text? And what is WS API? Web Sockets? I'll have to check if we're using Websockets to post data.

@allroundexperts
Copy link
Contributor

@mananjadhav Yes. WS is websockets. For other comments, the issue does not exist because the websocket response is being sent correctly.

@allroundexperts
Copy link
Contributor

@mananjadhav As confirmed in this chat:
https://expensify.slack.com/archives/C01GTK53T8Q/p1655499310036769
We need to not worry about the promise not resolving. Instead we need to fix the websocket api call that is failing.

@marcaaron
Copy link
Contributor

I would vote to take this internal. I'm not sure there is anything we can do in the client to investigate why the push data is failing to send...

https://www.expensify.com/_devportal/tools/logSearch/#query=request_id:(%2271e735df3f68f4c4-SJC%22)+AND+timestamp:[2022-06-20T19:21:22.807Z+TO+2022-06-20T21:21:22.807Z]&index=logs_expensify-017295

@marcaaron marcaaron added Internal Requires API changes or must be handled by Expensify staff Improvement Item broken or needs improvement. and removed Exported 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 Jun 20, 2022
@marcaaron marcaaron changed the title [$250] Chat - After specific formatted message is sent, new messages appears above it Chat - After specific formatted message is sent, new messages appears above it Jun 20, 2022
@melvin-bot
Copy link

melvin-bot bot commented Jun 24, 2022

Whoops! This issue is 2 days overdue. Let's get this updated quick!

@melvin-bot melvin-bot bot added the Overdue label Jun 24, 2022
@thienlnam thienlnam added Weekly KSv2 and removed Daily KSv2 labels Jun 24, 2022
@melvin-bot melvin-bot bot removed the Overdue label Jun 24, 2022
@mvtglobally
Copy link

potentially related issue #9370

@melvin-bot melvin-bot bot added Monthly KSv2 and removed Weekly KSv2 labels Jul 18, 2022
@melvin-bot
Copy link

melvin-bot bot commented Jul 18, 2022

This issue has not been updated in over 15 days. 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!

@tgolen tgolen self-assigned this Aug 17, 2022
@tgolen
Copy link
Contributor

tgolen commented Aug 17, 2022

I am no longer able to reproduce this. You can see here how my messages sent after I pasted in the formatted message are showing at the bottom, where I would expect them to. (I pasted the comment after "2" and before "3").

image

@tgolen tgolen closed this as completed Aug 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Engineering Improvement Item broken or needs improvement. Internal Requires API changes or must be handled by Expensify staff Monthly KSv2
Projects
None yet
Development

No branches or pull requests

8 participants