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
Code monitors: truncate long Slack blocks #50083
Conversation
a6d67dc
to
70d4d9d
Compare
// We limit the bytes to ensure we don't hit Slack's max block size of 3000 | ||
// characters. To be conservative, we truncate to 2500 bytes. We also limit the |
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.
q: Is it possible that multi-byte character (eg. emojis or CJK character) will be split up by using this truncation method? If yes, is there a way we can prevent this?
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.
We don't actually truncate the text by byte. We always truncate entire lines -- if a line will push us over the 2500 limit, then we truncate it and all subsequent lines. This seemed simplest and also produces a clean output. I now see this comment is not clear, I can update it :)
For context, we could use a rune/ character limit instead, but bytes seems safest so we don't count on the Slack API's meaning of "character".
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.
Got it, makes sense! So if the first line was already 2500+ characters, no preview will be sent?
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.
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.
Awesome, thanks for looking into this, it all sounds very reasonable.
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.
No problem, I had all the same questions when I was developing the fix!
Slack requires that text blocks be no longer than 3000 characters. We already limit the number of lines in the match blocks, but if the lines are long then the block can still exceed the character limit. With this change, we also truncate lines if they will push the block over the character limit. Fixes #49979.
Slack requires that text blocks be no longer than 3000 characters. We already limit the number of lines in the match blocks, but if the lines are long then the block can still exceed the character limit. With this change, we also truncate lines if they will push the block over the character limit.
Fixes #49979.
Test plan
New unit test for character-based truncation.
Also tested manually -- reproduced the error, then verified this change resolves it.