Improve isReportMessageAttachment performance #45748
Conversation
melvin-bot
bot
requested review from
marcochavezf
and removed request for
a team
|
@marcochavezf Please 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] |
| const regex = new RegExp(` ${CONST.ATTACHMENT_SOURCE_ATTRIBUTE}="(.*)"`, 'i'); | ||
| return (message.text === CONST.ATTACHMENT_MESSAGE_TEXT || !!Str.isVideo(message.text)) && (!!message.html.match(regex) || message.html === CONST.ATTACHMENT_UPLOADING_MESSAGE_HTML); | ||
| const isAttachmentMessageText = message.text === CONST.ATTACHMENT_MESSAGE_TEXT; | ||
| const isVideoText = Str.isVideo(message.text); |
There was a problem hiding this comment.
can we somehow save calculating isVideo? Looking from the traces it costs a lot to run it, maybe there are cheaper checks here that can bail out earlier?
I'd say simply moving hasAttachmentHtml to the 1st position can vastly limit this time as it also has to evaluate to true.
IMO we can continue with isVideo only after this check goes through. What are your thoughts on this?
There was a problem hiding this comment.
Your suggestion makes sense. By moving the hasAttachmentHtml check first, we can avoid running the costly isVideo check unless absolutely necessary.
There was a problem hiding this comment.
right, so:
hasAttachmentHtmlcan go first,- then we can check for
isAttachmentMessageTextand as a last resort calculateisVideoTextas a fallback for it.
There was a problem hiding this comment.
Changed, isVideo no longer appears in the trace
| return message.text === CONST.ATTACHMENT_MESSAGE_TEXT && message.translationKey === CONST.TRANSLATION_KEYS.ATTACHMENT; | ||
| } | ||
|
|
||
| const hasAttachmentHtml = attachmentRegex.test(message.html) || message.html === CONST.ATTACHMENT_UPLOADING_MESSAGE_HTML; |
There was a problem hiding this comment.
these are being executed left to right - I think you can also swap them around to avoid running a regexp check when a simple strict equality check on message.html === CONST.ATTACHMENT_UPLOADING_MESSAGE_HTML; will be enough.
There was a problem hiding this comment.
that way we can mostly rely on cheap equality checks, and only run a regexp.test() or isVideo() as fallbacks.
There was a problem hiding this comment.
approving it already, this is a minor change
| return message.text === CONST.ATTACHMENT_MESSAGE_TEXT && message.translationKey === CONST.TRANSLATION_KEYS.ATTACHMENT; | ||
| } | ||
|
|
||
| const hasAttachmentHtml = attachmentRegex.test(message.html) || message.html === CONST.ATTACHMENT_UPLOADING_MESSAGE_HTML; |
There was a problem hiding this comment.
approving it already, this is a minor change
Reviewer Checklist
Screenshots/VideosAndroid: Native2024-07-19.20.12.52.movAndroid: mWeb Chrome2024-07-19.20.09.39.moviOS: Native2024-07-19.20.16.33.moviOS: mWeb Safari2024-07-19.20.20.35.movMacOS: Chrome / Safari2024-07-19.19.40.21.movMacOS: Desktop2024-07-19.19.44.39.mov |
| @@ -2,6 +2,8 @@ import {Str} from 'expensify-common'; | |||
| import CONST from '@src/CONST'; | |||
| import type {Message} from '@src/types/onyx/ReportAction'; | |||
|
|
|||
| const attachmentRegex = new RegExp(` ${CONST.ATTACHMENT_SOURCE_ATTRIBUTE}="(.*)"`, 'i'); | |||
There was a problem hiding this comment.
I have a small question
Is a space at the beginning of RegExp required?
After testing, I noticed that RegExp works the same
There was a problem hiding this comment.
Oh
Just noticed
The same RegExp was before
|
Attachments work well ! 2024-07-19.20.18.48.movBut since this bug is also reproduced on the main LGTM |
|
🎯 @ZhenjaHorbach, thanks for reviewing and testing this PR! 🎉 An E/App issue has been created to issue payment here: #45793. |
| @@ -2,6 +2,8 @@ import {Str} from 'expensify-common'; | |||
| import CONST from '@src/CONST'; | |||
| import type {Message} from '@src/types/onyx/ReportAction'; | |||
|
|
|||
| const attachmentRegex = new RegExp(` ${CONST.ATTACHMENT_SOURCE_ATTRIBUTE}="(.*)"`, 'i'); | |||
|
Stepped in to get this improvement into the next staging release |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/mountiny in version: 9.0.10-2 🚀
|
|
🚀 Cherry-picked to staging by https://github.com/mountiny in version: 9.0.10-3 🚀
@Expensify/applauseleads please QA this PR and check it off on the deploy checklist if it passes. |
|
🚀 Deployed to production by https://github.com/mountiny in version: 9.0.10-7 🚀
|
|
🚀 Deployed to production by https://github.com/francoisl in version: 9.0.11-5 🚀
|
Details
This PR improves the performance of the
isReportMessageAttachmentmethod, which is part of thegetOrderedReportIDsoptimisation.Before:
After:
Fixed Issues
$
PROPOSAL:
Tests
Offline tests
QA Steps
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)myBool && <MyComponent />.src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop