[$250] Split - App crashes when tapping Copy to clipboard on code block context menu

lanitochka17 commented 3 weeks ago

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

Version Number: 9.0.22-7 Reproducible in staging?: Y Reproducible in production?: Y If this was caught during regression testing, add the test name, ID and link from TestRail: N/A Email or phone of affected tester (no customers): applausetester+kh050806@applause.expensifail.com Issue reported by: Applause - Internal Team

Action Performed:

Launch New Expensify app
Go to group chat
Tap + > Spli expense > Manual
Create a split manual expense with code block as the description
Tap on the split preview
Long press on the code block
Tap Copy to clipboard

Expected Result:

App will not crash

Actual Result:

App crashes when tapping Copy to clipboard on code block context menu in split menu

Workaround:

Unknown

Platforms:

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

[x] Android: Native
[ ] Android: mWeb Chrome
[x] iOS: Native
[ ] iOS: mWeb Safari
[ ] MacOS: Chrome / Safari
[ ] MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/user-attachments/assets/d4c3042e-a823-4a11-bc8e-0c11771791ad

logs (2).txt

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~01a78f1cc82b94611b
Upwork Job ID: 1827396760466325645
Last Price Increase: 2024-08-24
Automatic offers:

hoangzinh | Reviewer | 103731517

Issue Owner

Current Issue Owner: @hoangzinh

melvin-bot[bot] commented 3 weeks ago

Triggered auto assignment to @kevinksullivan (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.

lanitochka17 commented 3 weeks ago

@kevinksullivan FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors

lanitochka17 commented 3 weeks ago

We think that this bug might be related to #vip-split

Krishna2323 commented 3 weeks ago

Edited by proposal-police: This proposal was edited at 2024-08-20 19:46:28 UTC.

Proposal

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

Split - App crashes when tapping Copy to clipboard on code block context menu

What is the root cause of that problem?

Context menu is not disabled when action and report are undefined. https://github.com/Expensify/App/blob/dd320739ebcd9491c62d25c56ed14d3890c161c0/src/components/HTMLEngineProvider/HTMLRenderers/PreRenderer.tsx#L42-L44

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

We should not call showContextMenuForReport when report or action is undefined. The check can be added before calling showContextMenuForReport or here.
We should also check other html renderers with same bug.

What alternative solutions did you explore? (Optional)  

When report or action is not present, we can render TDefaultRenderer without PressableWithoutFeedback.

        <View style={isLast ? styles.mt2 : styles.mv2}>
            <ShowContextMenuContext.Consumer>
                {({anchor, report, reportNameValuePairs, action, checkIfContextMenuActive}) =>
                    report && action ? (
                        <PressableWithoutFeedback
                            onPress={onPressIn ?? (() => {})}
                            onPressIn={onPressIn}
                            onPressOut={onPressOut}
                            onLongPress={(event) => {
                                if (!report || !action) {
                                    return;
                                }
                                showContextMenuForReport(event, anchor, report?.reportID ?? '-1', action, checkIfContextMenuActive, ReportUtils.isArchivedRoom(report, reportNameValuePairs));
                            }}
                            shouldUseHapticsOnLongPress
                            role={CONST.ROLE.PRESENTATION}
                            accessibilityLabel={translate('accessibilityHints.prestyledText')}
                        >
                            <View>
                                {/* eslint-disable-next-line react/jsx-props-no-spreading */}
                                <TDefaultRenderer {...defaultRendererProps} />
                            </View>
                        </PressableWithoutFeedback>
                    ) : (
                        <View>
                            {/* eslint-disable-next-line react/jsx-props-no-spreading */}
                            <TDefaultRenderer {...defaultRendererProps} />
                        </View>
                    )
                }
            </ShowContextMenuContext.Consumer>
        </View>

Result

daledah commented 3 weeks ago

Proposal

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

A long press on the description field show the context menu actions.

What is the root cause of that problem?

The code block is displayed as the pre tag: https://github.com/Expensify/App/blob/dd320739ebcd9491c62d25c56ed14d3890c161c0/src/components/HTMLEngineProvider/HTMLRenderers/PreRenderer.tsx#L29
When long pressing on it, we will call: https://github.com/Expensify/App/blob/dd320739ebcd9491c62d25c56ed14d3890c161c0/src/components/HTMLEngineProvider/HTMLRenderers/PreRenderer.tsx#L43
There is the case the report or action data is undefined, because there is no ShowContextMenuContext.Provider wrapped in it, this issue for example.
As a result, call any context menu action, such as "Copy to clipboard" will throw the error "Cannot read properties of null (reading 'actionName')".

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

There are 4 types of context menu:
```
CONTEXT_MENU_TYPES: {
    LINK: 'LINK',
    REPORT_ACTION: 'REPORT_ACTION',
    EMAIL: 'EMAIL',
    REPORT: 'REPORT',
},
```
we must not allow user to open context menu type REPORT_ACTION by long pressing on the description field. Because they only can open context menu type REPORT_ACTION by long pressing on the report action.
Additionally, the onLongPress in: https://github.com/Expensify/App/blob/dd320739ebcd9491c62d25c56ed14d3890c161c0/src/components/HTMLEngineProvider/HTMLRenderers/PreRenderer.tsx#L42-L44

was initially added in this PR to ensure that long-pressing on a link or email triggers context menu type LINK or EMAIL. However, this is redundant because if an email or link is wrapped in a code block like:

abcdef@gmail.com

or

https://dev.new.expensify.com:8082/

They are displayed as code, not as an active email or link. Therefore, long-pressing on them should not trigger the context menu type LINK or EMAIL.

So the solution is just need to remove the above onLongPress.

What alternative solutions did you explore? (Optional)

In ShowContextMenuContext, we can introduce a new value in its context value, named canOpenContextMenu and its default value is false.

So this: https://github.com/Expensify/App/blob/dd320739ebcd9491c62d25c56ed14d3890c161c0/src/components/HTMLEngineProvider/HTMLRenderers/PreRenderer.tsx#L42-L44 will be:

                    onLongPress={(event) => {
                        if (!canOpenContextMenu) {
                            return;
                        }
                        showContextMenuForReport(event, anchor, report?.reportID ?? '-1', action, checkIfContextMenuActive, ReportUtils.isArchivedRoom(report, reportNameValuePairs));
                    }}

and just pass canOpenContextMenu: true in where in need through the ShowContextMenuContext.provider.

wildan-m commented 3 weeks ago

Proposal

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

Context menu shown in code block in menu item

What is the root cause of that problem?

The context menu shouldn't be shown in codeblock in menu item

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

If codeblocks are unnecessary in the menu item description, we can remove the shouldParseTitle parameter from MenuItemWithTopDescription.

src/components/ReportActionItem/MoneyRequestView.tsx (or other similar parts)

                <OfflineWithFeedback pendingAction={getPendingFieldAction('comment')}>
                    <MenuItemWithTopDescription
                        description={translate('common.description')}
                        shouldParseTitle
                        title={updatedTransactionDescription ?? transactionDescription}

What alternative solutions did you explore? (Optional)

N/A

melvin-bot[bot] commented 2 weeks ago

Job added to Upwork: https://www.upwork.com/jobs/~01a78f1cc82b94611b

melvin-bot[bot] commented 2 weeks ago

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

hoangzinh commented 2 weeks ago

Thanks for proposals, everyone. Will review today

hoangzinh commented 2 weeks ago

Thanks for your patience, everyone. After reviewing, here is my feedbacks

@Krishna2323's proposal: Can you add more details on your RCA next time? It's too short to understand the root cause. But your solution is most viable to me.
@daledah's proposal: Love your RCA, it's clear and helpful to me. But your main solution will cause a regression bug when long press on mobile devices, it doesn't open the context menu. Your alternative solution looks promising, but it will cause passing unnecessary context value. Because I think if report or action is undefined (like @Krishna2323 described in his proposal), it means it's not on a report action.

Click here to view recording

https://github.com/user-attachments/assets/522f5d18-98c8-4490-8210-8d08c85de5f6

@wildan-m's proposal: I don't think your solution is our expectation.

~Based on the above, I think we can go with @Krishna2323's proposal https://github.com/Expensify/App/issues/47743#issuecomment-2299635822~

Updated: changed my suggestion base on this https://github.com/Expensify/App/issues/47743#issuecomment-2312562668

🎀👀🎀 C+ reviewed~

melvin-bot[bot] commented 2 weeks ago

Triggered auto assignment to @lakchote, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

daledah commented 2 weeks ago

@hoangzinh

when long press on mobile devices, it doesn't open the context menu

I think it is expected, long press on the money request view's description should not display the context menu action. For example, In the video below, we still can open the context menu action in the confirmation page:

https://github.com/user-attachments/assets/4a280706-07d2-46fe-b802-52a2e448806d

daledah commented 2 weeks ago

cc @lakchote, do you have any thoughts on my comment above?

daledah commented 2 weeks ago

Added evidence in comment.

hoangzinh commented 2 weeks ago

@daledah I mean with your solution, on the report page, it won't open the context menu when user long presses on the codeblock messages

Step to reproduce on native apps:

Go to any reports
Send a codeblock message
Long press on that message
Expect: the context menu will be opened.

hoangzinh commented 2 weeks ago

Ah you can also check my recording here for reference https://github.com/Expensify/App/issues/47743#issuecomment-2311437663

daledah commented 2 weeks ago

Thanks @hoangzinh . I followed your test steps and It still works well with my changes:

https://github.com/user-attachments/assets/da14ad9a-bf31-4fd3-a519-5c098defba64

FYI, we already have a logic to show the context menu actions on long press: https://github.com/Expensify/App/blob/d5cfecfc53a2abf926a1dd9be8bc72c0b373ff81/src/pages/home/report/ReportActionItem.tsx#L347

hoangzinh commented 2 weeks ago

Can you go back and revisit the report again? When I test, I have to do the same to reproduce the issue if you've already opened the report page

Click here to view recording

https://github.com/user-attachments/assets/d7717831-bb6f-4677-a821-bff02c2c568d

daledah commented 2 weeks ago

Can you go back and revisit the report again?

I was only able to test on the web due to an issue with running the iOS app. I noticed that if I long press on a container area outside the code block, the context menu opens. However, if I long press directly on the code block, the context menu does not open.

https://github.com/user-attachments/assets/461919ac-d23e-435b-8209-d361164df194

I my alternative solution can fix two bugs because it fixed the RCA:

I mean with your solution, on the report page, it won't open the context menu when user long presses on the codeblock messages

and

we still can open the context menu action in the confirmation page

hoangzinh commented 2 weeks ago

@daledah Yeah. That's why I mentioned in my feedback comment here https://github.com/Expensify/App/issues/47743#issuecomment-2311437663. Your alternative solution looks promise and explicit, but I do more like @Krishna2323's solution in this case.

daledah commented 2 weeks ago

The root cause of this issue is that we're not preventing users from opening the context menu in the money request view, right? If we apply this solution, it won't address the root cause but rather serve as a workaround. Consequently, the bug where users can still open the context menu on the confirmation page (and there are a lot of similar bugs where we use money request view) will persist.

daledah commented 2 weeks ago

@lakchote @kevinksullivan

Please help confirm in the video below, we can long press on the description to open the context menu, is it a expected or bug:

https://github.com/user-attachments/assets/4a280706-07d2-46fe-b802-52a2e448806d

Krishna2323 commented 2 weeks ago

The root cause of this issue is that we're not preventing users from opening the context menu in the money request view, right? If we apply this https://github.com/Expensify/App/issues/47743#issuecomment-2299635822, it won't address the root cause but rather serve as a workaround. Consequently, the bug where users can still open the context menu on the confirmation page (and there are a lot of similar bugs where we use money request view) will persist.

~I don't agree with this, the context menu should only be opened for report actions and my proposal solves that correctly.~

Sorry, I misunderstood what @daledah was trying to say.

daledah commented 2 weeks ago

@hoangzinh Can you help confirm @Krishna2323 's comment above?

Below is my test result:

https://github.com/user-attachments/assets/47325b3a-76da-4e05-b00e-60c689e8bf30

hoangzinh commented 2 weeks ago

Oh wait, you're right @daledah. It doesn't work in case of expense

hoangzinh commented 2 weeks ago

Hi @lakchote please hold on 2nd review. I changed my selected proposal. I will review proposals again.

Krishna2323 commented 2 weeks ago

I don't agree with this, the context menu should only be opened for report actions and my proposal solves that correctly.

Sorry for that, I misunderstood @daledah's concern. IMO @daledah's alternative solution is the best option.

hoangzinh commented 2 weeks ago

Yep, @daledah's alternative solution is the best approach so far.

Link to proposal https://github.com/Expensify/App/issues/47743#issuecomment-2301309551

🎀👀🎀 C+ reviewed

cc @lakchote all yours

melvin-bot[bot] commented 2 weeks ago

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

lakchote commented 2 weeks ago

I do agree with you, let's go with @daledah's alternative solution.

Thanks for reviewing.

melvin-bot[bot] commented 2 weeks ago

📣 @hoangzinh 🎉 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

melvin-bot[bot] commented 2 weeks ago

📣 @daledah You have been assigned to this job! Please apply to the Upwork job and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻 Once you apply to this job, your Upwork ID will be stored and you will be automatically hired for future jobs! Keep in mind: Code of Conduct | Contributing 📖

daledah commented 1 week ago

@hoangzinh PR is ready.

Expensify / App