Markdown - Large code blocks are not displayed until user refreshes the page

[isagoico]

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!

---

Action Performed:

1. Log in to e.cash as account A
2. Navigate to a conversation with account B
3. Open a incognito tab and log in to account B
4. Send a large code block to account A
5. Navigate back to account A and check the conversation with account B

Expected Result:

Message with large code block is displayed

Actual Result:

Message is not displayed. User needs to reload the page sothe message is displayed.

Workaround:

User needs to refresh the page.

Platform:

Where is this issue occurring?

Web ✔️
iOS
Android
Desktop App
Mobile Web

Version Number: 1.0.74-0

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

Notes/Photos/Videos: Any additional supporting documentation

Grabando.193.mp4

Expensify/Expensify Issue URL:

View all open jobs on Upwork

---

From @MonilBhavsar https://expensify.slack.com/archives/C01GTK53T8Q/p1624957028202800

Large code blocks are not rendered until page is refreshed on web and app is restarted on desktop.

[MelvinBot]

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

[HorusGoul]

Things that I've found:

• The issue can be reproduced locally.
• It affects all kinds of large messages, not only code blocks.
• It's related to the 10kB limit imposed by Pusher. More info on that: https://support.pusher.com/hc/en-us/articles/360019115473-What-is-the-message-size-limit-when-publishing-a-message-in-Channels-
• This code should prevent this, but it doesn't:
  https://github.com/Expensify/Web-Expensify/blob/ae58ce1f1e0f20eab21a72ff5e3669830a72e544/lib/Push/Service.php#L119-L129
• The problem I see with that code is that we're measuring the payload size and not taking into account the modifications that Pusher's library applies to it. Inside the pusher-php-server code, we can find lines that encode or encrypt the payload, and these operations definitely change the size of the message, and if it exceeds the 10kB limit, the push message won't be sent.
  https://github.com/pusher/pusher-http-php/blob/v4.1.4/src/Pusher.php#L526-L535

Solutions to solve this problem:

a. We could implement one of Pusher's suggestions.
b. Add client-side validation to avoid messages from exceeding X number of characters, so we can be sure that it will fit in a Pusher notification.
c. Split large messages into multiple messages like Telegram does it. We could implement this on the client.

I'd implement (b) or (c) as preventive measures to avoid messages from getting lost during a conversation and then work in (a) until we are sure it works with any size. Once (a) is ready, we could remove (b) or (c) as an improvement. I think N7 could be great because the improvement is aligned with the need to send "long important messages" related to the business.

Also, if you try to send a text with a size of 3.5kB, it won't be received in the other end because the payload size exceeds the 10kB limit. What do we include in it to make it almost 3 times bigger? Is all of that necessary?

Invoking the @Expensify/engineering team: What solution would you pick?

[tgolen]

I believe we have support already for (c). If you look here, all of that code should be able to handle multiple "chunks" of a single pusher message that is sent over multiple requests. I don't exactly remember how that code works, but I think @johnmlee101 was the one that wrote and implemented most of it, so he could hopefully provide some more context for us.

[HorusGoul]

Cool! If there's already support in the app for this, the only thing left would be sending the messages in chunks from the server to the Pusher API.

[johnmlee101]

Yeah! For new versions of the apps we should be able to handle pusher requests (at least the ones used for Inbox Calls). There might be some API changes required to make sure this flow works (as we don't want to disrupt the mobile app for older version support), but should be straightforward.

[HorusGoul]

Should we open a new issue related to the API changes? We could also close this one as we don't need to change anything on the client-side.

[marcaaron]

There might be some API changes required to make sure this flow works (as we don't want to disrupt the mobile app for older version support), but should be straightforward.

To clarify, we are currently not sending report comments in chunks and will start doing so? I'm curious how we can do this without breaking existing mobile clients (Expensify not Expensify.cash) who are not set up to process chunks (most important maybe being Concierge in the current app). Pusher implementation here is not set up for chunks (as the implementation predates the chunking change).

It's hard to tell which client you are sending a message to or whether they can support chunking so the clearest way to achieve this is to send the events to a channel that we know can support the chunking. Maybe there's a better way since that will result in a ton of potentially unnecessary events?

Maybe this is less of a concern as I'm not too sure what happens when a mobile client tries to handle a message that is chunked. Maybe we handle it gracefully and the message just doesn't appear? If so, then it's less of a concern.

[tgolen]

I don't think the old mobile app uses pusher at all, does it?

…

On Thu, Jul 1, 2021 at 12:33 PM Marc Glasser ***@***.***> wrote: There might be some API changes required to make sure this flow works (as we don't want to disrupt the mobile app for older version support), but should be straightforward. To clarify, we are currently not sending report comments in chunks and will start doing so? I'm curious how we can do this without breaking existing mobile clients (Expensify not Expensify.cash) who are not set up to process chunks (most important maybe being Concierge in the current app). Pusher implementation here is not set up for chunks <https://github.com/Expensify/Mobile-Expensify/blob/b733fc9fc66b69c9c558a821aa0eb9f4bc5c4d49/app/managers/PusherManager.js#L179-L184> (as the implementation predates the chunking change). It's hard to tell which client you are sending a message to or whether they can support chunking so the clearest way to achieve this is to send the events to a channel that we know can support the chunking. Maybe there's a better way since that will result in a ton of potentially unnecessary events? Maybe this is less of a concern as I'm not too sure what happens when a mobile client tries to handle a message that is chunked. Maybe we handle it gracefully and the message just doesn't appear? If so, then it's less of a concern. — You are receiving this because you are on a team that was mentioned. Reply to this email directly, view it on GitHub <#3797 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAJMAB3KJQMUQ2TCCY6KX4TTVSYJFANCNFSM47QYVOWQ> .

[marcaaron]

Linked the code where it is used above. But yes, realtime report comments, Concierge widget, etc

[HorusGoul]

Going to return this to the pool as it's not a priority right now.

[MelvinBot]

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!

[pecanoro]

Is this still an issue? I was going to apply the export label but maybe we can close it

[flodnv]

@pecanoro I sent you a 1200 lines / 62000 characters code block and it worked for me without any refresh. Did it work for you? FWIW the app froze for like 5 seconds after hitting "send" but that's another problem...

[pecanoro]

Nope, I didn't get it hahaaha

[pecanoro]

Adding external then!

[kevinksullivan]

Removing myself as an assignee if this is just internal and in search of a technical volunteer.

[danieldoglas]

Hey guys, does anyone know if the order of events is guaranteed from Pusher? I mean, if I have 10 events, am I going to receive the last one as last for sure?

If that's not the case, we may also need to change the implementation at App too.

[danieldoglas]

@johnmlee101 , I saw the implementation client->client.

One way to make it send in chunks only to users using the new app would be to filter by reportComment and only start with Push_Channel::CHANNEL_NAME_CONCIERGE channels. Since we only have channels P2P on the new app, that would not send the chunked pushs to old.e.

Would that be bad?

[johnmlee101]

Can you elaborate what parts of the code you would want to change for this to work? I'm not sure if I get the question 😓

[danieldoglas]

I was thinking of composing this if here: https://github.com/Expensify/Web-Expensify/blob/f87face9d5963e18603e5380765d588680ab741e/lib/Push/Service.php#L122

If the data is not from concierge (name uses prefix) and has > 10kb I'll break it to chunks and send the push. Else it will follow the same rules it has today.

[danieldoglas]

Ok, just saw that the channel_name for concierge in new.E is also using private-report[...], so probably that won't work. I'm checking if I have any property that indicate that that specific chat is a concierge chat or not.

[tgolen]

does anyone know if the order of events is guaranteed from Pusher?

I think you can definitely assume it WON'T come in order. It has to go from our PHP to Pusher's server to our client, and there is a lot of room there for things to get out of order.

[danieldoglas]

Will stop this for now to change to a Daily reported bug

[danieldoglas]

Back on it.

[danieldoglas]

Ok, the code is now implemented from PHP. The messages are arriving at Pusher.
I'm getting a strange behaviour and am investigating now. If I send the chunks of message together, even if they are < 10240, the event is listed on Pusher as succeeded, but for some reason they are not being listed in my websocket at new.E.

Will continue to work on this.

[danieldoglas]

Ok, found it and it's now working.

@johnmlee101 , I'll create a new eventType called "chunked-reportComment" and "chunked-repportCommentEdit". Since we were never sending the push notifications when it was bigger than 10240, this would be the situation when we would use this format and apps that are prepared to receive that kind of event will work with it. If I use the same handle that we use today (reportComment), I'll need to change the current format of the push, which will break the current app.

Do you see any other alternative here?

[MelvinBot]

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

[johnmlee101]

Hmm, for backwards compatibility I think the prefix makes sense to me. Can't think of another solution at the moment?

[MelvinBot]

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

[MelvinBot]

@danieldoglas Eep! 4 days overdue now. Issues have feelings too...

[danieldoglas]

Backend approved. Waiting for deploy to test new.E on prod and send that PR.