App: [HOLD for payment 2023-02-15] [$4000] Error message appears when sending 15000 emojis
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:
- navigate to any chat
- send 15000 emoji to the chat
- Compose with 15000 characters and add one more emoji
Expected Result:
Emojis should be sent since it’s exactly 15000 and no error message should be. Also after 15000 characters adding an emoji should show the count as 15001
Actual Result:
getting an error Auth CreeateReportAction returned an error
In step 3 shows the count as 15002
Workaround:
unknown
Platforms:
Which of our officially supported platforms is this issue occurring on?
- Android / native
- Android / Chrome
- iOS / native
- iOS / Safari
- MacOS / Chrome / Safari
- MacOS / Desktop
Version Number: 1.2.48-2
Reproducible in staging?: y
Reproducible in production?: y
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos:
Expensify/Expensify Issue URL: Issue reported by: @jayeshmangwani Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1672427549244909
Upwork Automation - Do Not Edit
- Upwork Job URL: https://www.upwork.com/jobs/~01cb872390d8ccc3cf
- Upwork Job ID: 1611228769941909504
- Last Price Increase: 2023-01-25
About this issue
- Original URL
- State: closed
- Created a year ago
- Comments: 202 (179 by maintainers)
Thank you both for your clarification and honesty. In that case, @redstar504 will receive the full payout
@stitesExpensify To provide some clarity, I first submitted the working solution using regex. Shortly after @s77rt provided a refactor of it using a for-loop.
After reviewing, @mollfpr suggested that if we went with the for-loop, the reward should be split (ie. I would get the job, but implement the loop in the PR).
Since we have agreed on the regex approach, we are using both my idea and my implementation of it. Splitting the reward would not make sense. Thanks for being transparent about that @s77rt.
@mollfpr @jayeshmangwani will you apply here?
Sorry for the delay, I agree that we don’t need to add a new checklist @mollfpr
Regression Test Proposal
1251 Emojis:
Additional Testing Steps:
🤨
👻
😶🌫️
👩👩👦👦
Do we agree 👍 or 👎
BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:
@stitesExpensify I think we need to clarify this before continuing with the working PR 😅
Honestly the optimizations are cool, but I think the regex is the cleanest solution. A difference of 2ms isn’t worth “dirtying” the code IMO, so let’s just stick to regex.
Thanks to you all for putting in work on this! I also agree that we will pay out this issue as @mollfpr described here
If you want a true optimization, we could just run the modified character length function when the normal length exceeds
MAX_COMMENT_LENGTH / 6
. Although for the sake of clean code, it’s really not necessary since the performance impact is practically non-existent.Thank you, @redstar504 and @s77rt, for bringing up the conversation.
Both of you have significant proposals, but I agree @redstar504 is the first to come up with the concept.
I test both of your proposals regarding the performance issue, and there’s a significant distance.
I agree, but we can discuss this with the others for a better opinion. Changing the
COMMENT_MAX_LENGTH
will affect the edit message, and I believeUpdateComment
has a different limit exceed. See this comment. Except we treat both of them differently.@stitesExpensify I’m inclined with @redstar504 proposal and combine with @s77rt logic to count the correct comment length due to performance issue.
Just my opinion; I’m not okay with this. Since this issue has long conversations and many discussions, we can split the bounty between @redstar504 and @s77rt. @redstar504 will be the one who opens the PR and have the opportunity to get the bonus timeline. cc @adelekennedy
@mollfpr Figured out why you could send those on staging now. The recent commit https://github.com/Expensify/App/commit/58b9c9d080446be16b9b8354cfc48069c73ab1ec removed clientID from the buildOptimisticAddCommentReportAction.
Since the 60kb validation on the backend actually takes place on the message object generated through this action, this increased the possible comment size by a small margin.
So rather than a limit of 59959 we could now get away with a maximum possible comment size of 59988 bytes before hitting the auth error. To accurately demonstrate this, the
COMMENT_MAX_LENGTH
from my proposal should be updated to 59988.It also appears due to a recent change, the error message is not displayed on the UI but it fails silently. I am only able to see that the comment below is failing on staging by viewing the XHR requests in Chrome dev tools.
You should hit the error on the staging with a minimum of 9999 copyright symbols:
Thanks for the update @s77rt! I’ll review it in the morning.
Opened the discussion @mollfpr
https://expensify.slack.com/archives/C01GTK53T8Q/p1674729288444489
Proposal 2
Details
Since we got more info about the server and how it stores the messages which is apparently as @redstar504 mentioned we can count the length of the message to be the same way as it’s calculated on the server.
My proposal is different as we don’t really care about the encoded message, we only care about the length of that encoded message, so I optimised the code to get the length without encoding the message.
You will see a confusing part where I just multiply the length of
lengthCorrectionOfNonASCII
by 5 😅. Well in short that’s as the variable name states a correction. Where did the 5 came from? Pretty straightforward, if any char is outside the ASCII range it will be encoded in a literal 6 chars (\uXXXX
) where in javascript it will be counted as 1. so the diff is 5. Example: If you run'😄'.length
you will get 2 but in the BE it’s actually 12 bytes. so we take into account the 2 and since the emoji is not in the ascii range and it’s made of two chars, we add 2 * 5 bytes (10), totally to 12 bytes.Keeping the current
MAX_COMMENT_LENGTH
value at 15000 we will be able to send 1250 emojis of 😄 (15000 / 12). If we increase the value to 59959 (and if it’s a permitted value) then we would be able to send 4996 emojis of 😄 (59959 / 12). Where 12 is the length of encoded string representing that emoji which is:\uD83D\uDE04
Benchmarks Since this is going to have a great impact on the performance it’s important to take that into account, from the above proposals I only took the latest proposal from @redstar504 to compare with. (@getusha is using an external library and I think we don’t want that for now)
I have made 3 tests:
Results:
Source code: https://gist.github.com/s77rt/f170b5a12cd75133fd7aee5cf1c53ec0
Sure @mollfpr i will open the discussion in short
Also if we can change
MAX_COMMENT_LENGTH
that means with my current proposal we can send more than3700
@getusha This also confirms the 59959 byte limit for the comment string itself. The additional JSON in the message field make up the 41 additional characters to get to 60kb.
@mollfpr There is no way to allow 15,000 emojis in a single message by modifying the front-end only. The best we can do is to limit the length of comments on the front-end to the amount the back-end will allow.
Refer to my earlier comment: https://github.com/Expensify/App/issues/13988#issuecomment-1379850576
We could also remove any emojis coming after the first 9990 characters of the string to keep within the 60kb validation.
https://github.com/Expensify/App/blob/main/src/libs/ReportUtils.js#L649-L653
Still waiting for internal engineers to hop into the Slack thread.
I think
Intl.Segmenter
is only available for web. It’s still a new thing, firefox does not support that either.Ahh yes, I’ll review the proposals later tonight. Thanks for the ping!
For anyone whos looking for 15,000 of 😁 https://docs.google.com/document/d/1P4JZEIB3KSFCUNw0F1oEVwST8RCPpwR49gYRz7J0Ds8/edit?usp=sharing
For 15,000 text characters https://github.com/Expensify/App/issues/13988#issuecomment-1377436914
@getusha No problem 👍
@mollfpr it’s not 2 characters it is two set of characters
that’s why it is returning 2
It would be handy to get some insight into how the BE calculates the length & what’s the exact limit for the messages & how it is handled , etc.
@priyeshshah11 got it from here https://expensify.slack.com/archives/C049HHMV9SM/p1672427549244909 the bug report
@mollfpr here we go
Just to confirm, that also means that there aren’t actually 15k emojis right? It’s
U+1F928
(7 characters) so in this case it would be15000/7 = 2142
so 2142 emojis would put us at 14999 and we shouldn’t be able to add another emoji?@adelekennedy I think we are looking for external approach?
@getusha yes! I asked a clarifying question here though I accidentally added ‘external’ too soon!