Performance: chat input #25758
Performance: chat input #25758
Conversation
…y-app-fork into perf/composer-investigations
…ction' of github.com:margelo/expensify-app-fork into perf/composer-investigations
…ction' of github.com:margelo/expensify-app-fork into perf/composer-investigations
…y-app-fork into refactor-report-action-compose
…ction' into refactor-report-action-compose
This reverts commit eb9baf8.
This reverts commit 9739c08.
move actionButtonRef
…actor-report-action-compose
…y-app-fork into refactor-report-action-compose
…ction' of github.com:margelo/expensify-app-fork into refactor-report-action-compose
|
🚀 Deployed to production by https://github.com/luacmartins in version: 1.3.58-5 🚀
|
|
🚀 Deployed to staging by https://github.com/mountiny in version: 1.3.59-0 🚀
|
|
🚀 Deployed to production by https://github.com/luacmartins in version: 1.3.59-5 🚀
|
|
|
||
| const reportRecipient = personalDetails[participantsWithoutExpensifyAccountIDs[0]]; | ||
| const shouldUseFocusedColor = !isBlockedFromConcierge && !disabled && isFocused; | ||
| const isFullSizeComposerAvailable = isFullComposerAvailable && !_.isEmpty(value); |
| maxLines={maxComposerLines} | ||
| onFocus={onFocus} | ||
| onBlur={onBlur} | ||
| onClick={setShouldBlockSuggestionCalc} |
There was a problem hiding this comment.
Is this intended? Previously we used to set shouldBlockEmojiCalc and shouldBlockMentionCalc to false but now this being set to true.
There was a problem hiding this comment.
I dont think there should be change in the behaviour here, not intentionally
| [isKeyboardShown, isSmallScreenWidth, parentReportActions, report, reportActions, reportID, submitForm, suggestionsRef, valueRef], | ||
| ); | ||
|
|
||
| const onSelectionChange = useCallback( |
There was a problem hiding this comment.
Coming from #26534
We should've ensured to open the suggestion list only when the composer is focused, to prevent unexpected behavior.
| const {translate} = useLocalize(); | ||
|
|
||
| const Tap = Gesture.Tap() | ||
| .enabled() |
There was a problem hiding this comment.
Enabled condition was not preserved !(isCommentEmpty || isBlockedFromConcierge || disabled || hasExceededMaxCommentLength) and caused regression #27779
| * @param {String} reportID | ||
| * @param {String} comment | ||
| */ | ||
| const debouncedSaveReportComment = _.debounce((reportID, comment) => { |
There was a problem hiding this comment.
This caused a regression of showing draft icon - more information here - #27362 (comment)
| } | ||
|
|
||
| const valueAfterTheCursor = value.substring(selectionEnd); | ||
| const indexOfFirstWhitespaceCharOrEmojiAfterTheCursor = valueAfterTheCursor.search(CONST.REGEX.NEW_LINE_OR_WHITE_SPACE_OR_EMOJI); |
There was a problem hiding this comment.
This regex NEW_LINE_OR_WHITE_SPACE_OR_EMOJI never existed which caused #28754
|
As talked already, this caused super minor style regression: |
|
It seems like it caused this regression as well: #30921 (discrepancy between create and edit mode in terms of shown max message length limit error). |
|
|
||
| const [highlightedEmojiIndex, setHighlightedEmojiIndex] = useArrowKeyFocusManager({ | ||
| isActive: isEmojiSuggestionsMenuVisible, | ||
| maxIndex: SuggestionsUtils.getMaxArrowIndex(suggestionValues.suggestedEmojis.length, isAutoSuggestionPickerLarge), |
There was a problem hiding this comment.
👋 Coming from #33613
SuggestionsUtils.getMaxArrowIndex did return visible list length (instead of true list length), which meant that you couldn't scroll past the visible items using a keyboard
We resolved this with
maxIndex: suggestionValues.suggestedMentions.length - 1
| useEffect(() => { | ||
| // Value state does not have the same value as comment props when the comment gets changed from another tab. | ||
| // In this case, we should synchronize the value between tabs. | ||
| const shouldSyncComment = prevCommentProp !== comment && value !== comment; |
There was a problem hiding this comment.
this caused a race where some characters will be skipped when typing on the same interval - more info #37356
| (text, shouldAddTrailSpace = true) => { | ||
| const updatedText = shouldAddTrailSpace ? `${text} ` : text; | ||
| const selectionSpaceLength = shouldAddTrailSpace ? CONST.SPACE_LENGTH : 0; | ||
| updateComment(ComposerUtils.insertText(commentRef.current, selection, updatedText)); |
There was a problem hiding this comment.
This line caused this issue:
#41778
More detail about the RCA here:
#41778 (comment)
Details
This PR improves the performance of the
ReportActionComposecomponent greatly.The main improvement is that we moved state down into separated components. In the
ReportActionComposecomponent we render a lot of items, such as the send button, emoji button, a menu for attachments, attachment picker popups, the composer itself, comment exceeding notice, mention suggestions, emoji suggestions, etc. When we entered one letter all the top level state was updated multiple times, leading to a lot of re-renders of all the mentioned components.So, to achieve a better performance we needed to break out more separate components. This also cleaned up the
ReportActionComposecomponent, it went down from1362lines to410lines, thus becoming a lot easier to read and maintain.However, this made the PR a bit larger than anticipated. The following gives an overview of the changes to help review this PR.
Note
I'd recommend first looking at the code itself and understanding its structure, and then checking the specific diffs of this PR.
The
ReportActionComposewas moved into its own folder undersrc/pages/home/report/ReportActionCompose. Here are all new split out components placed:SendButton.js: The JSX of the send button was moved into its own component for cleanupSuggestions.js: Before the mention and emoji suggestions were part of the main composer, and also their state was placed there. The composer itself doesn't have to re-render when we change the suggestion state, so it was moved into its own component.SuggestionMention.js: Before the mention suggestion were re-rendering when e.g. the emoji suggestions were changing. Now they are decoupled into their own places.SuggestionEmoji.js: Separate implementation for emoji suggestionsAttachmentPickerWithMenuItems.js: This contains the options we show when pressing the + in the composer. The options need access to the attachment picker, so they are grouped together. They also had state that's unnecessary to other parts of the composer.ComposerWithSuggestions.js: Here lives the actual<Composer />and itsvalueandselectionstate. Only the<Composer />and the<Suggestions />need to re-render when any of the two states are changing, hence they are grouped together.SilentCommentUpdater.js: This components subscribes to the actual draft comment, and fires a side effect for certain conditions. For the other components we don't need the draft comment, so it makes no sense re-rendering these. That's why it was split out into its own component for performance.Another note: Initially I wanted to make separate PRs for each split out component, but: the components were so entangled with each other, that it was only possible to make a full clean up 🍝😅 .
Fixed Issues
$ #25763
Tests
From
mainbranchmain.mov
From this branch
improved.mov
Offline tests
n/a
QA Steps
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)myBool && <MyComponent />.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.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Web
Screen.Recording.2023-08-23.at.12.15.57.mov
Mobile Web - Chrome
Screen.Recording.2023-08-23.at.12.15.24.mov
Mobile Web - Safari
Simulator.Screen.Recording.-.iPhone.14.Pro.-.2023-08-23.at.12.20.42.mp4
Desktop
improved.mov
iOS
Simulator.Screen.Recording.-.iPhone.14.Pro.-.2023-08-23.at.12.26.56.mp4
Android
Screen.Recording.2023-08-23.at.12.12.12.mov