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

lanitochka17 · 2024-08-20T19:26:26Z

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: 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): [email protected]
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?

Screenshots/Videos

Add any screenshot/video evidence

Bug6577380_1724181250000.Screen_Recording_20240821_030954_One_UI_Home.mp4

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 · 2024-08-20T19:26:36Z

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 · 2024-08-20T19:26:54Z

@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 · 2024-08-20T19:27:12Z

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

Krishna2323 · 2024-08-20T19:43:43Z

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.

App/src/components/HTMLEngineProvider/HTMLRenderers/PreRenderer.tsx

Lines 42 to 44 in dd32073

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

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 · 2024-08-21T07:15:33Z

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:

App/src/components/HTMLEngineProvider/HTMLRenderers/PreRenderer.tsx

Line 29 in dd32073

function PreRenderer({TDefaultRenderer, onPressIn, onPressOut, onLongPress, ...defaultRendererProps}: PreRendererProps) {

When long pressing on it, we will call:

App/src/components/HTMLEngineProvider/HTMLRenderers/PreRenderer.tsx

Line 43 in dd32073

    
           showContextMenuForReport(event, anchor, report?.reportID ?? '-1', action, checkIfContextMenuActive, ReportUtils.isArchivedRoom(report, reportNameValuePairs))

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:

App/src/components/HTMLEngineProvider/HTMLRenderers/PreRenderer.tsx

Lines 42 to 44 in dd32073

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

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:

[email protected]

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:

App/src/components/HTMLEngineProvider/HTMLRenderers/PreRenderer.tsx

Lines 42 to 44 in dd32073

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

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 · 2024-08-21T07:18:35Z

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 · 2024-08-24T17:25:15Z

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

melvin-bot · 2024-08-24T17:25:19Z

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

hoangzinh · 2024-08-26T16:41:16Z

Thanks for proposals, everyone. Will review today

hoangzinh · 2024-08-27T02:17:02Z

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

Screen.Recording.2024-08-27.at.09.05.20.mov

@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 #47743 (comment)~~

Updated: changed my suggestion base on this #47743 (comment)

🎀👀🎀 C+ reviewed~

melvin-bot · 2024-08-27T02:17:13Z

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

daledah · 2024-08-27T02:53:57Z

@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:

Screen.Recording.2024-08-27.at.12.22.42.mov

daledah · 2024-08-27T03:16:24Z

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

daledah · 2024-08-27T05:25:41Z

Added evidence in comment.

hoangzinh · 2024-08-27T08:24:47Z

@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 · 2024-08-27T08:25:23Z

Ah you can also check my recording here for reference #47743 (comment)

daledah · 2024-08-27T08:30:43Z

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

Screen.Recording.2024-08-27.at.15.29.20.mov

FYI, we already have a logic to show the context menu actions on long press:

App/src/pages/home/report/ReportActionItem.tsx

Line 347 in d5cfecf

ReportActionContextMenu.showContextMenu(

hoangzinh · 2024-08-27T10:43:56Z

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

Screen.Recording.2024-08-27.at.17.41.34.mov

daledah · 2024-08-27T11:18:11Z

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.

Screen.Recording.2024-08-27.at.18.06.10.mov

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

daledah · 2024-08-31T10:15:12Z

@hoangzinh PR is ready.

kevinksullivan · 2024-09-12T19:06:54Z

PR still in review. Looping in another BZ member as I am going OOO

melvin-bot · 2024-09-12T19:07:14Z

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

isabelastisser · 2024-09-17T19:08:16Z

No updates yet.

melvin-bot · 2024-09-20T18:06:02Z

@hoangzinh, @lakchote, @kevinksullivan, @isabelastisser, @daledah Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

lakchote · 2024-09-23T09:15:28Z

Discussion is still happening in the PR.

melvin-bot · 2024-09-24T18:06:30Z

@hoangzinh, @lakchote, @kevinksullivan, @isabelastisser, @daledah 6 days overdue. This is scarier than being forced to listen to Vogon poetry!

hoangzinh · 2024-09-25T02:06:51Z

PR has been deployed to staging. Awaiting for testing

hoangzinh · 2024-09-25T02:08:19Z

@lakchote @isabelastisser can you help to add the "Weekly" label back for this issue? It seems to be reverted to "Daily"

lakchote · 2024-09-25T08:03:59Z

@lakchote @isabelastisser can you help to add the "Weekly" label back for this issue? It seems to be reverted to "Daily"

Done.

hoangzinh · 2024-10-18T03:54:23Z

BugZero Checklist:

The PR that introduced the bug has been identified. Link to the PR: Nope, I think it's just a new edge case that we haven't covered.
The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment: N/A
A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion: N/A
Determine if we should create a regression test for this bug: 🚫 It's just an edge case UI issue. We don't need a regression test for it.

hoangzinh · 2024-10-18T03:54:45Z

cc @isabelastisser this issue is ready for payment

isabelastisser · 2024-10-22T18:24:53Z

@hoangzinh, I processed your payment in Upwork.
@daledah, please share your Upwork profile, thanks!

daledah · 2024-10-29T17:55:41Z

@isabelastisser Here you go https://www.upwork.com/freelancers/~0138d999529f34d33f

isabelastisser · 2024-11-05T19:57:02Z

@daledah, thanks! I sent you the offer in Upwork now. All set!

daledah · 2024-11-06T04:16:33Z

@isabelastisser Many thanks, offer accepted.

lanitochka17 added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Aug 20, 2024

melvin-bot bot assigned kevinksullivan Aug 20, 2024

melvin-bot bot added the Overdue label Aug 23, 2024

kevinksullivan added the External Added to denote the issue can be worked on by a contributor label Aug 24, 2024

melvin-bot bot changed the title ~~Split - App crashes when tapping Copy to clipboard on code block context menu~~ [$250] Split - App crashes when tapping Copy to clipboard on code block context menu Aug 24, 2024

melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Aug 24, 2024

melvin-bot bot assigned hoangzinh Aug 24, 2024

melvin-bot bot removed the Overdue label Aug 24, 2024

kevinksullivan added this to [#whatsnext] #vip-vsb Aug 24, 2024

melvin-bot bot assigned lakchote Aug 27, 2024

melvin-bot bot added Weekly KSv2 and removed Daily KSv2 labels Aug 31, 2024

kevinksullivan removed their assignment Sep 12, 2024

kevinksullivan added Bug Something is broken. Auto assigns a BugZero manager. and removed Bug Something is broken. Auto assigns a BugZero manager. labels Sep 12, 2024

melvin-bot bot assigned isabelastisser Sep 12, 2024

melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Sep 12, 2024

kevinksullivan self-assigned this Sep 12, 2024

lakchote added Weekly KSv2 and removed Daily KSv2 labels Sep 25, 2024

nathanmetcalf unassigned kevinksullivan Sep 26, 2024

mallenexpensify removed this from [#whatsnext] #vip-vsb Oct 17, 2024

isabelastisser closed this as completed Nov 5, 2024

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

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

Comments

lanitochka17 commented Aug 20, 2024 • edited by melvin-bot bot Loading

Action Performed:

Expected Result:

Actual Result:

Workaround:

Platforms:

Screenshots/Videos

melvin-bot bot commented Aug 20, 2024

lanitochka17 commented Aug 20, 2024

lanitochka17 commented Aug 20, 2024

Krishna2323 commented Aug 20, 2024 • edited by github-actions bot Loading

Proposal

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

What is the root cause of that problem?

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

What alternative solutions did you explore? (Optional)

Result

daledah commented Aug 21, 2024

Proposal

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

What is the root cause of that problem?

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

What alternative solutions did you explore? (Optional)

wildan-m commented Aug 21, 2024

Proposal

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

What is the root cause of that problem?

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

What alternative solutions did you explore? (Optional)

melvin-bot bot commented Aug 24, 2024

melvin-bot bot commented Aug 24, 2024

hoangzinh commented Aug 26, 2024

hoangzinh commented Aug 27, 2024 • edited Loading

melvin-bot bot commented Aug 27, 2024

daledah commented Aug 27, 2024 • edited Loading

daledah commented Aug 27, 2024

daledah commented Aug 27, 2024

hoangzinh commented Aug 27, 2024

hoangzinh commented Aug 27, 2024

daledah commented Aug 27, 2024 • edited Loading

hoangzinh commented Aug 27, 2024

daledah commented Aug 27, 2024 • edited Loading

daledah commented Aug 31, 2024

kevinksullivan commented Sep 12, 2024

melvin-bot bot commented Sep 12, 2024

isabelastisser commented Sep 17, 2024

melvin-bot bot commented Sep 20, 2024

lakchote commented Sep 23, 2024

melvin-bot bot commented Sep 24, 2024

hoangzinh commented Sep 25, 2024

hoangzinh commented Sep 25, 2024

lakchote commented Sep 25, 2024

hoangzinh commented Oct 18, 2024

BugZero Checklist:

hoangzinh commented Oct 18, 2024

isabelastisser commented Oct 22, 2024

daledah commented Oct 29, 2024

isabelastisser commented Nov 5, 2024

daledah commented Nov 6, 2024

lanitochka17 commented Aug 20, 2024 •

edited by melvin-bot bot

Loading

Krishna2323 commented Aug 20, 2024 •

edited by github-actions bot

Loading

What alternative solutions did you explore? (Optional)  

hoangzinh commented Aug 27, 2024 •

edited

Loading

daledah commented Aug 27, 2024 •

edited

Loading

daledah commented Aug 27, 2024 •

edited

Loading

daledah commented Aug 27, 2024 •

edited

Loading