Skip to content
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

[hold for payment 2024-05-29][$250] Chat icons visible before input field renders #41397

Closed
1 of 6 tasks
Julesssss opened this issue May 1, 2024 · 32 comments
Closed
1 of 6 tasks
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review Weekly KSv2

Comments

@Julesssss
Copy link
Contributor

Julesssss commented May 1, 2024

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!


Version Number: 1.4.67-4

Action Performed:

  • Kill the app
  • Open the app
  • Navigate to a chat

Expected Result:

  • Buttons and input field should become visible at the same time

Actual Result:

  • Icons are visible before the input field renders
  • They jump from left side to right which looks bad

Platforms:

Which of our officially supported platforms is this issue occurring on?

  • Android: Native
  • Android: mWeb Chrome
  • iOS: Native UNTESTED
  • iOS: mWeb Safari
  • MacOS: Chrome / Safari
  • MacOS: Desktop

Screenshots/Videos

Screenshot 2024-05-01 at 09 36 13

InputButtons.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~012097aedd79eaa599
  • Upwork Job ID: 1785589469415067648
  • Last Price Increase: 2024-05-01
  • Automatic offers:
    • ikevin127 | Reviewer | 0
    • bernhardoj | Contributor | 0
@Julesssss Julesssss added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels May 1, 2024
@Julesssss Julesssss self-assigned this May 1, 2024
Copy link

melvin-bot bot commented May 1, 2024

Triggered auto assignment to @joekaufmanexpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

@Julesssss Julesssss added the External Added to denote the issue can be worked on by a contributor label May 1, 2024
Copy link

melvin-bot bot commented May 1, 2024

Job added to Upwork: https://www.upwork.com/jobs/~012097aedd79eaa599

@melvin-bot melvin-bot bot changed the title Chat icons visible before input field renders [$250] Chat icons visible before input field renders May 1, 2024
@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label May 1, 2024
Copy link

melvin-bot bot commented May 1, 2024

Triggered auto assignment to Contributor-plus team member for initial proposal review - @ikevin127 (External)

@ShridharGoel
Copy link
Contributor

ShridharGoel commented May 1, 2024

Proposal

Please re-state the problem that we are trying to solve in this issue.

The emoji and send icons become visible before the input field and then jump when the input field is shown.

What is the root cause of that problem?

This happens because the + icon and composer are inside AttachmentModal. This leads to an unnecessary increase in component nesting, while the emoji icon and send icon are outside the AttachmentModal so they display earlier.

<AttachmentModal
headerTitle={translate('reportActionCompose.sendAttachment')}
onConfirm={addAttachment}
onModalShow={() => setIsAttachmentPreviewActive(true)}
onModalHide={onAttachmentPreviewClose}
>
{({displayFileInModal}) => (
<>
<AttachmentPickerWithMenuItems
displayFileInModal={displayFileInModal}
reportID={reportID}
report={report}
reportParticipantIDs={reportParticipantIDs}
isFullComposerAvailable={isFullComposerAvailable}
isComposerFullSize={isComposerFullSize}
isBlockedFromConcierge={isBlockedFromConcierge}
disabled={!!disabled}
setMenuVisibility={setMenuVisibility}
isMenuVisible={isMenuVisible}
onTriggerAttachmentPicker={onTriggerAttachmentPicker}
raiseIsScrollLikelyLayoutTriggered={raiseIsScrollLikelyLayoutTriggered}
onCanceledAttachmentPicker={() => {
isNextModalWillOpenRef.current = false;
restoreKeyboardState();
}}
onMenuClosed={restoreKeyboardState}
onAddActionPressed={onAddActionPressed}
onItemSelected={onItemSelected}
actionButtonRef={actionButtonRef}
/>
<ComposerWithSuggestions
ref={composerRef}
animatedRef={animatedRef}
suggestionsRef={suggestionsRef}
isNextModalWillOpenRef={isNextModalWillOpenRef}
isScrollLikelyLayoutTriggered={isScrollLikelyLayoutTriggered}
raiseIsScrollLikelyLayoutTriggered={raiseIsScrollLikelyLayoutTriggered}
reportID={reportID}
policyID={report?.policyID ?? ''}
parentReportID={report?.parentReportID}
parentReportActionID={report?.parentReportActionID}
includeChronos={ReportUtils.chatIncludesChronos(report)}
isGroupPolicyReport={isGroupPolicyReport}
isEmptyChat={isEmptyChat}
lastReportAction={lastReportAction}
isMenuVisible={isMenuVisible}
inputPlaceholder={inputPlaceholder}
isComposerFullSize={isComposerFullSize}
displayFileInModal={displayFileInModal}
textInputShouldClear={textInputShouldClear}
setTextInputShouldClear={setTextInputShouldClear}
isBlockedFromConcierge={isBlockedFromConcierge}
disabled={!!disabled}
isFullComposerAvailable={isFullComposerAvailable}
setIsFullComposerAvailable={setIsFullComposerAvailable}
setIsCommentEmpty={setIsCommentEmpty}
handleSendMessage={handleSendMessage}
shouldShowComposeInput={shouldShowComposeInput}
onFocus={onFocus}
onBlur={onBlur}
measureParentContainer={measureContainer}
listHeight={listHeight}
onValueChange={(value) => {
if (value.length === 0 && isComposerFullSize) {
Report.setIsComposerFullSize(reportID, false);
}
validateCommentMaxLength(value, {reportID});
}}
/>
<ReportDropUI
onDrop={(event: DragEvent) => {
if (isAttachmentPreviewActive) {
return;
}
const data = event.dataTransfer?.items[0];
displayFileInModal(data as unknown as FileObject);
}}
/>
</>
)}
</AttachmentModal>

What changes do you think we should make in order to solve the problem?

Move the below code inside AttachmentModal, so the emoji icon and send icon also work as children of AttachmentModal.

{DeviceCapabilities.canUseTouchScreen() && isMediumScreenWidth ? null : (
    <EmojiPickerButton
        isDisabled={isBlockedFromConcierge || disabled}
        onModalHide={focus}
        //  @ts-expect-error TODO: Remove this once EmojiPickerButton (https://github.com/Expensify/App/issues/25155) is migrated to TypeScript.
        onEmojiSelected={(...args) => composerRef.current?.replaceSelectionWithText(...args)}
        emojiPickerID={report?.reportID}
        shiftVertical={emojiShiftVertical}
    />
)}
<SendButton
    isDisabled={isSendDisabled}
    handleSendMessage={handleSendMessage}
/>

@ikevin127
Copy link
Contributor

♻️ Reviewing proposal.

@ikevin127
Copy link
Contributor

@ShridharGoel Thanks for your proposal !

Your RCA is correct in the sense that some elements are rendered as children of AttachmentModal therefore they become visible with delay. Regarding this being necessary or not: I think that it is since these components are using children props like displayFileInModal passed from AttachmentModal which are required for: AttachmentPickerWithMenuItems, ComposerWithSuggestions and ReportDropUI to work as expected (as children of AttachmentModal).

When it comes to the proposed solution, because AttachmentModal is a complex component used in other parts of the app, I am reserved when it comes to the addition of the attachment prop and additional logic mentioned since adding an additional state variable would increase the render count which we want to avoid, except when absolutely necessary.

Tip

Instead of moving the mentioned AttachmentModal children components outside of the AttachmentModal, I'd direct my attention towards the components that are outside of the AttachmentModal and try to find a solution that would make it such that all of the components become visible at the same time, as mentioned in the expected result.

@c3024
Copy link
Contributor

c3024 commented May 2, 2024

Proposal

Please re-state the problem that we are trying to solve in this issue.

Chat icons appear to jump from left to right because the attachment picker button and the composer are loaded with delay

What is the root cause of that problem?

Composer and AttachmentPicker are inside the AttachmentModal so the icons are loaded quicker than the composer and attachment picker.

What changes do you think we should make in order to solve the problem?

I think the jumping of icons from left to right is what causing the bad look. We can position them at the right from the beginning by adding styles.justifyContentEnd here


Instead of the above, we can add the justifyContentEnd to chatItemComposeBox here

It does not break anything because this style is used only from composer and edit composer.

What alternative solutions did you explore? (Optional)

@ShridharGoel
Copy link
Contributor

Proposal

Updated based on @ikevin127's comment.

@bernhardoj
Copy link
Contributor

bernhardoj commented May 2, 2024

Proposal

Please re-state the problem that we are trying to solve in this issue.

The add action button and the input itself is missing when opening a chat for the first time after killing the app.

What is the root cause of that problem?

The add button and the input/composer are wrapped with an AttachmentModal component.

<AttachmentModal
headerTitle={translate('reportActionCompose.sendAttachment')}
onConfirm={addAttachment}
onModalShow={() => setIsAttachmentPreviewActive(true)}
onModalHide={onAttachmentPreviewClose}
>
{({displayFileInModal}) => (
<>
<AttachmentPickerWithMenuItems
displayFileInModal={displayFileInModal}
reportID={reportID}
report={report}
reportParticipantIDs={reportParticipantIDs}
isFullComposerAvailable={isFullComposerAvailable}
isComposerFullSize={isComposerFullSize}
isBlockedFromConcierge={isBlockedFromConcierge}
disabled={!!disabled}
setMenuVisibility={setMenuVisibility}
isMenuVisible={isMenuVisible}
onTriggerAttachmentPicker={onTriggerAttachmentPicker}
raiseIsScrollLikelyLayoutTriggered={raiseIsScrollLikelyLayoutTriggered}
onCanceledAttachmentPicker={() => {
isNextModalWillOpenRef.current = false;
restoreKeyboardState();
}}
onMenuClosed={restoreKeyboardState}
onAddActionPressed={onAddActionPressed}
onItemSelected={onItemSelected}
actionButtonRef={actionButtonRef}
/>
<ComposerWithSuggestions
ref={composerRef}
animatedRef={animatedRef}
suggestionsRef={suggestionsRef}
isNextModalWillOpenRef={isNextModalWillOpenRef}
isScrollLikelyLayoutTriggered={isScrollLikelyLayoutTriggered}
raiseIsScrollLikelyLayoutTriggered={raiseIsScrollLikelyLayoutTriggered}
reportID={reportID}
policyID={report?.policyID ?? ''}
parentReportID={report?.parentReportID}
parentReportActionID={report?.parentReportActionID}
includeChronos={ReportUtils.chatIncludesChronos(report)}
isGroupPolicyReport={isGroupPolicyReport}
isEmptyChat={isEmptyChat}
lastReportAction={lastReportAction}
isMenuVisible={isMenuVisible}
inputPlaceholder={inputPlaceholder}
isComposerFullSize={isComposerFullSize}
displayFileInModal={displayFileInModal}
textInputShouldClear={textInputShouldClear}
setTextInputShouldClear={setTextInputShouldClear}
isBlockedFromConcierge={isBlockedFromConcierge}
disabled={!!disabled}
isFullComposerAvailable={isFullComposerAvailable}
setIsFullComposerAvailable={setIsFullComposerAvailable}
setIsCommentEmpty={setIsCommentEmpty}
handleSendMessage={handleSendMessage}
shouldShowComposeInput={shouldShowComposeInput}
onFocus={onFocus}
onBlur={onBlur}
measureParentContainer={measureContainer}
listHeight={listHeight}
onValueChange={(value) => {
if (value.length === 0 && isComposerFullSize) {
Report.setIsComposerFullSize(reportID, false);
}
validateCommentMaxLength(value, {reportID});
}}
/>

AttachmentModal connects to several onyx data,

export default withOnyx<AttachmentModalProps, AttachmentModalOnyxProps>({
transaction: {
key: ({report}) => {
const parentReportAction = ReportActionsUtils.getReportAction(report?.parentReportID ?? '', report?.parentReportActionID ?? '');
const transactionID = parentReportAction?.actionName === CONST.REPORT.ACTIONS.TYPE.IOU ? parentReportAction?.originalMessage.IOUTransactionID ?? '0' : '0';
return `${ONYXKEYS.COLLECTION.TRANSACTION}${transactionID}`;
},
initWithStoredValues: false,
},
parentReport: {
key: ({report}) => `${ONYXKEYS.COLLECTION.REPORT}${report ? report.parentReportID : '0'}`,
},
policy: {
key: ({report}) => `${ONYXKEYS.COLLECTION.POLICY}${report ? report.policyID : '0'}`,
},
parentReportActions: {
key: ({report}) => `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${report ? report.parentReportID : '0'}`,
canEvict: false,
},
})(memo(AttachmentModal));

as we (probably) know, withOnyx will render null if the data is not all ready yet.

In our case, both parentReport and policy are not available yet.
Screenshot 2024-05-02 at 22 19 44

Also, if we open a non-policy chat, the add (+) button is still shown in delay because in AttachmentPickerWithMenuItems, we connect to policy data.

export default withOnyx<AttachmentPickerWithMenuItemsProps, AttachmentPickerWithMenuItemsOnyxProps>({
policy: {
key: ({report}) => `${ONYXKEYS.COLLECTION.POLICY}${report?.policyID}`,
},
})(AttachmentPickerWithMenuItems);

What changes do you think we should make in order to solve the problem?

Reading through the code, both parentReport and policy actually in AttachmentModal aren't being used anymore.

const threeDotsMenuItems = useMemo(() => {
if (!isReceiptAttachment || !parentReport || !parentReportActions) {
return [];
}

}, [isReceiptAttachment, parentReport, parentReportActions, policy, transaction, file, sourceState, iouType]);

The above code shows the only usage of both data.

Both data are first added in #28373 to check whether the user can edit the receipt or not, but the condition (canEditReceipt) is now "transformed" into a prop, so it's now controlled by the parent component. So, it's best to remove both parentReport and policy.

For AttachmentPickerWithMenuItems, set the initial value of the policy to an empty object.
initialValue: {} as OnyxTypes.Policy

What alternative solutions did you explore? (Optional)

Set the initial value for both data in AttachmentModal as an empty object

OR

Replace useOnyx with withOnyx

@ikevin127
Copy link
Contributor

♻️ Reviewing proposals.

@ikevin127
Copy link
Contributor

@c3024 Thanks for your proposal.

RCA:

Composer and AttachmentPicker are inside the AttachmentModal so the icons are loaded quicker than the composer and attachment picker.

Your RCA is partially correct, except the other way around. The components inside the AttachmentModal appear slower to load then the ones outside it.

Regarding your solution: while it solves the positioning of the components outside of the AttachmentModal by keeping them at the right of the container instead of having them move from left to right, it still does not fix the issue as the icons inside the AttachmentModal are still appearing with delay (see attached video).

Android: Native (OnePlus 7T - Android 14)
proposal-2.mp4

Note

If you feel like I missed something from your proposal, please let me know and if the argument is solid I can reconsider assignment.

@ikevin127
Copy link
Contributor

@bernhardoj Thanks for your proposal.

Part of RCA:

as we (probably) know, withOnyx will render null if the data is not all ready yet.

In our case, both parentReport and policy are not available yet.

Your RCA is correct when it comes to the withOnyx rendering null if all imported keys data is not ready yet. I would accept this RCA as correct, if your proposed solution would actually fix the issue.

Regarding your solution: I tested it and there's still an issue with the + (attachment picker component) in the sense that it still appears with delay and there's a weird UI glitch (see video below), therefore your combination of RCA + solution does not fix our issue as per the expected result.

Android: Native (OnePlus 7T - Android 14)
proposal-3.mp4

Note

If you feel like I missed something from your proposal, please let me know and if the argument is solid I can reconsider assignment.

@ikevin127
Copy link
Contributor

ikevin127 commented May 2, 2024

Given @ShridharGoel's updated proposal, I'd say the root cause was identified and the updated solution fixes our issue (see video below) as per the expected result which states: "Buttons and input field should become visible at the same time".

Android: Native (OnePlus 7T - Android 14)
proposal-1.mp4

🎀👀🎀 C+ reviewed

Copy link

melvin-bot bot commented May 2, 2024

Current assignee @Julesssss is eligible for the choreEngineerContributorManagement assigner, not assigning anyone new.

@bernhardoj
Copy link
Contributor

bernhardoj commented May 3, 2024

Hmm, I don't see the + action button shown with delay.

Screen.Recording.2024-05-03.at.13.58.18.mov

The AttachmentPickerWithMenuItems is also connected with a policy data, but it's available when I log it,

export default withOnyx<AttachmentPickerWithMenuItemsProps, AttachmentPickerWithMenuItemsOnyxProps>({
policy: {
key: ({report}) => `${ONYXKEYS.COLLECTION.POLICY}${report?.policyID}`,
},
})(AttachmentPickerWithMenuItems);

Can you test adding an initial value ({}) to it?

@ikevin127

EDIT: Ah, I can reproduce it if I open a non-related policy chat, so the onyx key will be policy_undefined. The initial value solution will fix it. Updated my proposal to include that.

@Julesssss
Copy link
Contributor Author

Thanks all, I'll let Kevin take another look at @bernhardoj's updated proposal before assigning.

@bernhardoj
Copy link
Contributor

false alarm, just linking to a similar issue

@Expensify Expensify deleted a comment from melvin-bot bot May 3, 2024
@ikevin127
Copy link
Contributor

Thanks all, I'll let Kevin take another look at @bernhardoj's updated proposal before assigning.

Given @bernhardoj's updated proposal, the RCA is correct and makes sense now! The updated solution targets the root cause of the issue and fixes it more efficiently then the previously selected proposal (see videos below).

Android: Native
android.mp4
iOS: Native
ios.MP4

Reasoning:

The updated proposal completely removes the bottleneck which delayed the display of the child components within AttachmentModal and AttachmentPickerWithMenuItems component -> this renders all chat icons at the same time and a lot faster than the previously selected proposal.

Why ?
The previously selected proposal's solution worked in the sense that it would display all chat icons at the same time but it would do this by delaying all the icons display, instead of targeting the root cause which is that components wrapped with withOnyx don't render until all imported onyx keys data is ready.

🎀👀🎀 C+ reviewed

Copy link

melvin-bot bot commented May 3, 2024

Current assignee @Julesssss is eligible for the choreEngineerContributorManagement assigner, not assigning anyone new.

@joekaufmanexpensify
Copy link
Contributor

waiting for sign off on proposal

@melvin-bot melvin-bot bot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label May 7, 2024
Copy link

melvin-bot bot commented May 7, 2024

📣 @ikevin127 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job

Copy link

melvin-bot bot commented May 7, 2024

📣 @bernhardoj 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job
Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Keep in mind: Code of Conduct | Contributing 📖

@melvin-bot melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Daily KSv2 labels May 7, 2024
@bernhardoj
Copy link
Contributor

PR is ready

cc: @ikevin127

@joekaufmanexpensify
Copy link
Contributor

PR merged today!

@joekaufmanexpensify
Copy link
Contributor

On staging

@ikevin127
Copy link
Contributor

⚠️ Automation failed: This was deployed to production today here, therefore should be [HOLD for Payment 2024-05-29] and have Awaiting Payment label.

cc @joekaufmanexpensify @Julesssss

@joekaufmanexpensify
Copy link
Contributor

Sounds good! We need to issue the following payments tomorrow:

@joekaufmanexpensify joekaufmanexpensify changed the title [$250] Chat icons visible before input field renders [hold for payment 2024-05-29][$250] Chat icons visible before input field renders May 28, 2024
@joekaufmanexpensify
Copy link
Contributor

All set to issue payment

@joekaufmanexpensify
Copy link
Contributor

@bernhardoj $250 sent and contract ended!

@joekaufmanexpensify
Copy link
Contributor

@ikevin127 $250 sent and contract ended!

@joekaufmanexpensify
Copy link
Contributor

Upwork job closed.

@joekaufmanexpensify
Copy link
Contributor

All set, thanks everyone!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something is broken. Auto assigns a BugZero manager. External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review Weekly KSv2
Projects
None yet
Development

No branches or pull requests

6 participants