status-im / status-mobile

a free (libre) open source, mobile OS for Ethereum
https://status.app
Mozilla Public License 2.0
3.88k stars 984 forks source link

Inconsistent maximum character counts for messages on desktop and mobile platforms #21007

Open Horupa-Olena opened 1 month ago

Horupa-Olena commented 1 month ago

Bug Report

Reproduction

  1. On the desktop version of the app, attempt to send a message longer than 2000 characters.
  2. On the mobile version of the app, attempt to send a message longer than 4096 characters.

Expected behavior

The maximum character count for messages should be consistent across all platforms, either 2000 or 4096 characters.

Actual behavior

Additional Information

ilmotta commented 2 weeks ago

@xAlisher @benjthayer do you know what is the correct requirement for the artificial max number of characters per message?

benjthayer commented 2 weeks ago

@xAlisher @benjthayer do you know what is the correct requirement for the artificial max number of characters per message?

I'm not sure if there is anything driving the char limit on desktop other than alignment with Discord - which is also 2000.

From a design / UX perspective for the author/reader there is no hard requirement to limit the character count given we implement scrolling in the composer and an expand button to reveal the full extent of longer messages when they are sent.

The only thing from a design perspective that we would want to ensure is that if a user sends an overly long message (imagine they copy and paste an entire novel!) that when the user hits the expand button, they aren't caught in an endless scroll. That could be a potential spam vector.

@jrainville @igor-sirotin do you have any context of any technical aspect driving the 2000 char limit or was alignment to Discord the main driver?

@ilmotta do you know what the driver for the 4096 char limit on mobile was?

jrainville commented 2 weeks ago

do you have any context of any technical aspect driving the 2000 char limit or was alignment to Discord the main driver?

I don't remember clearly. I know it was chosen back when Simon was the designer. 2000 might have been chosen to align with Discord like you said, or because it looked ok when pressing the "Expand" button. Though, it's still possible to create an obnoxious scrolling situation by creating a message that is like 1\n2\n3\n..., so it would be one line of character that would be super long.

There is obviously a limit on the size of messages on the Waku side, but we now have a mechanism on status-go that chunks messages, so we can in theory send messages of any size. Anyway, text messages should never reach that limit compared to images.

ilmotta commented 2 weeks ago

@benjthayer @jrainville I found a pretty old commit that first introduced the limit, here's the PR https://github.com/status-im/status-mobile/pull/11112

Unfortunately, the explanation as to why we chose 4096 was in a repository in status-im that doesn't exist anymore https://github.com/status-im/security-reports/issues/22#issuecomment-674998679, but now we know it was related to security problem.

Maybe @vkjr or @cammellos who were in that PR could still remember why 4096 characters? It's a stretch, but maybe you guys recall something.

cammellos commented 2 weeks ago

@benjthayer @jrainville I found a pretty old commit that first introduced the limit, here's the PR #11112

Unfortunately, the explanation as to why we chose 4096 was in a repository in status-im that doesn't exist anymore status-im/security-reports#22 (comment), but now we know it was related to security problem.

Maybe @vkjr or @cammellos who were in that PR could still remember why 4096 characters? It's a stretch, but maybe you guys recall something.

Large messages can choke the renderer, since we fetch the last 20 messages, if all those messages are 2000 characters, the renderer will slow down considerably (we used to hide partially messages longer than x)

vkjr commented 1 week ago

@ilmotta, no, unfortunately I don't remember why this exact size(