-
Notifications
You must be signed in to change notification settings - Fork 171
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
Fix crashes for extremely long messages with no line breaks #2163
Fix crashes for extremely long messages with no line breaks #2163
Conversation
I mentioned it in the PR description, but I'll add it here too for more visibility:
|
📱 Scan the QR code below to install the build (arm64 only) for this PR. |
Quality Gate passedKudos, no new issues were introduced! 0 New issues |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #2163 +/- ##
===========================================
+ Coverage 66.95% 67.03% +0.08%
===========================================
Files 1375 1375
Lines 34236 34188 -48
Branches 7546 7449 -97
===========================================
- Hits 22922 22918 -4
+ Misses 7675 7653 -22
+ Partials 3639 3617 -22 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
|
||
companion object { | ||
// Max characters to display in the summary message. This works around https://github.com/element-hq/element-x-android/issues/2105 | ||
private const val MAX_SAFE_LENGTH = 500 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
512 would have been nicer to me 🤓
@bmarty thanks for the approval!
Any ideas on this? Should we discuss it tomorrow and iterate on this PR once we've decided how to handle these messages in the timeline? |
Yes, let's discuss this IRL. |
We'll discuss the problem with the timeline with the product team and come up with a different solution for it. We're merging this in the meantime. |
Type of change
Content
Motivation and context
Fixes #2105 .
This can affect the rendering of the timeline too, but I'm not sure about how to handle this. Should we hard truncate them at N length? Should we look for the first
\n
char and if it's > N truncate it them? Should we add some suffix to the end of the message to make sure users understand it's been truncated?Screenshots / GIFs
Tests
DefaultRoomLastMessageFormatter.kt
, replace one of the results with somebuildString { ... }
that ends up having several thousand characters without any line breaks.If it doesn't crash, the substring is doing its job and the issue is fixed. To be sure you can remove the trailing
.take(MAX_SAFE_LENGTH)
expression.Tested devices
Checklist