Count non-ASCII comment characters by their encoded length #14752
Conversation
melvin-bot
bot
requested review from
mollfpr and
stitesExpensify
and removed request for
a team
|
@stitesExpensify @mollfpr One of you needs to copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
redstar504
force-pushed
the
Brayden-CommentLength
branch
from
cde58f1
to
e8ae6f4
Compare
|
@redstar504 Could you resolve the unsigned commit issue? Thanks! |
|
@mollfpr The commit is signed. The action failed due to another issue, and returned a 500 error. Any ideas? |
|
I manually re-ran the job and it passed 😄 |
|
Thanks @stitesExpensify. I had a feeling that would do it. |
|
Missing test movie added for the desktop platform. Ready for review! |
There was a problem hiding this comment.
To achieve the optimization, I have placed the call to the length calculator inside the CommentLength component itself. This makes it easy to memoize the function and cache the result between renders until comment text has affirmatively changed.
@redstar504 Thanks for the optimization concern! I feel like refactoring the ExceededCommentLength is not necessary for this PR, and it needs time too to review the solution for the optimization issue. I'll that we focus and keep the PR as the proposal we accepted. What do you think @stitesExpensify?
|
Thanks for the preliminary review @mollfpr! I was unaware of the rule surrounding hooks. The technique I used establishes some groundwork for expanding on the comment length calculation. We can build upon it to fix the related issue #14268. To solve the related issue, we could simply debounce the calculation until there is a break in typing. The break gives us the opportunity to perform the more expensive markdownToHtml conversion before passing it to the calculation used in this PR, providing us with the total HTML length. Conducting a markdown conversion on every keypress would be too expensive. With the addition of debouncing we could eliminate any potential delays introduced while actively typing. We also want to memoize it so it does not occur more than once, since the composer is being re-rendered multiple times during key events. Isolating all of this behavior to it's own component also makes it trivial to reuse in the edit composer. A couple of hooks may be justified here due to the simplicity and cost savings. Thoughts @stitesExpensify? |
|
Like @mollfpr said, we are not using hooks at all unfortunately.
How expensive is it? Is it noticeable while typing? |
|
@stitesExpensify I haven't done much testing for that issue, but I would not recommend it. There are a lot of conversion steps in the markdown transformer. We can discuss an optimization later when we address it. I will revert the hook usage for this problem and submit the simplified patch so we can get this closed out first. |
redstar504
force-pushed
the
Brayden-CommentLength
branch
2 times, most recently
from
dd33803
to
ac8f83b
Compare
redstar504
force-pushed
the
Brayden-CommentLength
branch
from
ac8f83b
to
b418a11
Compare
|
@mollfpr @stitesExpensify I have replaced this PR's commit with the agreed patch from the issue thread. The hook-based optimization is available on a reference branch if we decide to revisit the underlying concept as part of the other issue. I have verified it continues to function on each platform as expected, and it's ready for review @mollfpr. |
redstar504
requested review from
mollfpr
and removed request for
stitesExpensify
|
@stitesExpensify can you re-assign yourself to this PR? I clicked request review and for some reason it removed you. |
|
@redstar504 Let's add a test case where sending a message/emojis near the limit is not failing. |
|
So far, I don't experience any performance issues or regressions. It may not notice, but I guess re-renders of ReportActionCompose need to be addressed as an improvement in another issue. |
Reviewer Checklist
Screenshots/VideosWeb14752.Web.movMobile Web - Chrome14752.mWeb-Chrome.mp4Mobile Web - Safari14752.mWeb-Safari.mp4Desktop14752.Desktop.moviOS14752.iOS.mp4Android14752.Android.mp4 |
|
Good to go @stitesExpensify! |
|
🚀 Deployed to staging by https://github.com/stitesExpensify in version: 1.2.67-6 🚀
|
|
Thanks @stitesExpensify! |
|
🚀 Deployed to production by https://github.com/mountiny in version: 1.2.67-7 🚀
|
Details
This PR now only contains the patch from the issue thread.
The composer optimization described below will be deferred until we determine that such an optimization may be necessary as part of the related issue.
The suggested optimization for reference:
Due to re-renders of ReportActionCompose, we are presently counting comment length several times per keypress. As result, we should limit invocations of the calculation to once per change of the comment text.
To achieve the optimization, we can place the call to the length calculator inside the CommentLength component itself. This makes it easy to memoize the function and cache the result between renders until comment text has affirmatively changed.
This can be followed up by using useEffect to call a handler on the parent component if the length has crossed the limit threshold. This inverts control and updates the UI indicators/warnings in the parent component from the CommentLength child component.
Since we are abstracting the logic, we can reuse the same behaviour in ReportActionItemMessageEdit without having to duplicate the optimization.
Fixed Issues
$ #13988
PROPOSAL: #13988 (comment)
Tests
Offline tests
QA Steps
1251 emojis should not be sendable ❌
1250 emojis should be sendable ✔️
2501 copyright symbols should not be sendable ❌
2500 copyright symbols should be sendable ✔️
228 higher-byte emojis should not be sendable ❌
227 higher-byte emojis should be sendable ✔️
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*files and using the translation methodWaiting for Copylabel for a copy review on the original GH to get the correct copy.STYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)/** comment above it */thisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)Avataris modified, I verified thatAvataris working as expected in all cases)ScrollViewcomponent to make it scrollable when more elements are added to the page.Screenshots/Videos
Web
safari.test.1.webm
Mobile Web - Chrome
android-chrome.webm
Mobile Web - Safari
ios.safari.test.webm
Desktop
mactest.webm
iOS
ios.test.result.webm
Android
android.test.webm