[HOLD for payment 2023-07-05] [$1000] Chat - App crashes when edit comment clicked for mark down message

[lanitochka17]

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

---

Issue found when executing PR #20336

Action Performed:

1. Login with any account
2. Open a report
3. Send a markdown message e.g google.com
4. Clicking on reply in thread of this message
5. Verify that thread title display as the text of the message
6. Edit the message

Expected Result:

Verify that thread title still display as the text of the message instead of the markdown of the message

Actual Result:

App crashes

Workaround:

Unknown

Platforms:

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

• Android / native
• Android / Chrome
• iOS / native
• iOS / Safari
• MacOS / Chrome / Safari
• MacOS / Desktop

Version Number: 1.3.28.3

Reproducible in staging?: Yes

Reproducible in production?: Yes

If this was caught during regression testing, add the test name, ID and link from TestRail:

Email or phone of affected tester (no customers):

Logs: https://stackoverflow.com/c/expensify/questions/4856

Notes/Photos/Videos: Any additional supporting documentation

app.crash.20336.MP4

Expensify/Expensify Issue URL:

Issue reported by: Applause - Internal Team

Slack conversation:

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~0115b4eacb891784a9
• Upwork Job ID: 1669642999913762816
• Last Price Increase: 2023-06-16

[melvin-bot]

Triggered auto assignment to @dylanexpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

[melvin-bot]

Bug0 Triage Checklist (Main S/O)

• This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
• This bug is not a duplicate report (check E/App issues and #expensify-bugs)
  • If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
• This bug is reproducible using the reproduction steps in the OP. S/O
  • If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
  • If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
• This issue is filled out as thoroughly and clearly as possible
  • Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
• I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~0115b4eacb891784a9

[melvin-bot]

Current assignee @dylanexpensify is eligible for the External assigner, not assigning anyone new.

[melvin-bot]

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

[dukenv0307]

Proposal

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

App crashes when editing message after replying to thread and go back.

What is the root cause of that problem?

It's here

App/src/libs/ReportScrollManager/index.native.js

Line 13 in bd0e843

flatListRef.current.scrollToIndex(index);

where the flatListRef.current is null but we call scrollToIndex on it so it crashes.

The deeper root cause is that we're setting the flatListRef here https://github.com/Expensify/App/blob/bd0e843621dda8ad1fb31595f0f923a9d9447cf8/src/pages/home/report/ReportActionsList.js#LL155C25-L155C25 when the InvertedFlatList renders. So when we go back from the thread to the main report:

1. The InvertedFlatList of the thread unmounts
2. The InvertedFlatList of the main report mounts

1 actually happens after 2 so the flatListRef is set to null.

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

1. We need to checkflatListRef.current is not falsy before calling scrollToIndex on it.

if (!flatListRef.current) {
    return;
}

Same for the .scrollToOffset below it and the corresponding file for web.

2. Above is to prevent the crash. But we also need to fix the deeper cause. We can either:

• Store an object of flatListRefs instead of just 1. Like:

{
   [reportID A]: refA,
   [reportID B]: refB
}

so that when InvertedFlatList of report A unmounts, it will only set the flatListRefs[reportID A] to null. Meanwhile the flatListRefs[reportID B] is still correct and calling scrollToIndex on it works well.

What alternative solutions did you explore? (Optional)

Another way to do 2 is to only set the value of flatListRef only if the ref of the target element is not null. So when the report unmounts we'll not set the flatListRef to null.

[mountiny]

@dukenv0307 ere you bale to find the PR which introduced this?

[dukenv0307]

@mountiny I haven't been able to find the PR that causes the regression. But it's not from this PR.

[mountiny]

@fedirjh let us know what you think of the proposasl, thanks

[fedirjh]

@mountiny I think this is a regression from #20296 , will try to revert and test it.

[fedirjh]

Ok Confirmed , reverting #20296 doesn’t fixes the issue, this issue is tricky, need more investigation.

[mountiny]

Thanks, what about the proposal of @dukenv0307 then?

[bernhardoj]

Proposal

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

Crash when edit a message after coming back from another chat

What is the root cause of that problem?

It has the same root cause with this issue #20513 I worked on previously (here is the proposal).

So, we have a global ref for the message list.

App/src/libs/ReportScrollManager/index.js

Lines 3 to 5 in bd0e843

	// This ref is created using React.createRef here because this function is used by a component that doesn't have access
	// to the original ref.
	const flatListRef = React.createRef();

and is set in ReportActionsList

App/src/pages/home/report/ReportActionsList.js

Lines 152 to 155 in bd0e843

	<Animated.View style={[animatedStyles, styles.flex1]}>
	<InvertedFlatList
	accessibilityLabel={props.translate('sidebarScreen.listOfChatMessages')}
	ref={ReportScrollManager.flatListRef}

When we close a chat, that ref will become null. It's not a regression from #20296 but from the new navigation. This doesn't happen before because report screen is always mounted.

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

I would suggest to use the same solution.

1. Use a context to share the ref. In [HOLD for payment 2023-06-28] [$1000] mweb - Chat - Reaction list does not appear after navigating to chat from member detail #20513, we added a context specifically for reaction list only, but I think we can start to move the context to ReportScreen. The context will contains for both reaction list ref and flat list ref.
2. Modify ReportScrollManager. Remove the global ref and modify the function to accept a ref instead.
   

   App/src/libs/ReportScrollManager/index.js

   Line 14 in bd0e843

   function scrollToIndex(index, isEditing) {

   
   

   App/src/libs/ReportScrollManager/index.js

   Line 26 in bd0e843

   function scrollToBottom() {

ReportScrollManager is also being used in MoneyRequestModal too which is a separate page from ReportScreen.

App/src/pages/iou/MoneyRequestModal.js

Lines 398 to 405 in bd0e843

	onConfirm={(selectedParticipants) => {
	createTransaction(selectedParticipants);
	ReportScrollManager.scrollToBottom();
	}}
	onSendMoney={(paymentMethodType) => {
	sendMoney(paymentMethodType);
	ReportScrollManager.scrollToBottom();
	}}

It will scroll to bottom when we do a money request. As we are removing the global ref, we need a way to have the same behavior with the local ref. To achieve this, we can use the same approach of scrolling to bottom when we add a new message. In ReportActionsView, we have a listener that will scroll to bottom when a new action is added from the current user:

App/src/pages/home/report/ReportActionsView.js

Lines 102 to 111 in bd0e843

	this.unsubscribeFromNewActionEvent = Report.subscribeToNewActionEvent(this.props.report.reportID, (isFromCurrentUser, newActionID) => {
	const isNewMarkerReportActionIDSet = !_.isEmpty(this.state.newMarkerReportActionID);
	// If a new comment is added and it's from the current user scroll to the bottom otherwise leave the user positioned where
	// they are now in the list.
	if (isFromCurrentUser) {
	ReportScrollManager.scrollToBottom();
	// If the current user sends a new message in the chat we clear the new marker since they have "read" the report
	this.setState({newMarkerReportActionID: ''});

When we add a new action, we will trigger the listener here

App/src/libs/actions/Report.js

Lines 299 to 305 in bd0e843

	// Notify the ReportActionsView that a new comment has arrived
	if (reportID === newActionSubscriber.reportID) {
	const isFromCurrentUser = lastAction.actorAccountID === currentUserAccountID;
	newActionSubscriber.callback(isFromCurrentUser, lastAction.reportActionID);
	}
	}

So, here is what we need to do:

1. Move the logic to a function (notifyNewAction) and export it
2. Use the new function in IOU requestMoney, sendMoneyElsewhere, etc.

This issue could potentially happens to all global ref.

[melvin-bot]

Looks like something related to react-navigation may have been mentioned in this issue discussion.

As a reminder, please make sure that all proposals are not workarounds and that any and all attempt to fix the issue holistically have been made before proceeding with a solution. Proposals to change our DeprecatedCustomActions.js files should not be accepted.

Feel free to drop a note in #expensify-open-source with any questions.

[fedirjh]

We need to check flatListRef.current is not falsy before calling scrollToIndex on it.

@mountiny This solution will prevent the crash , we can implement it and keep this issue open for more investigation.

Other thing that I noticed. LEt's say we have an edit composer open in a chat Report/ thread. If we navigate from LHN to the chat report, It will automatically scroll to the open edit composer , however if we navigate from child thread to the parent report , nothing will happen.

From LHN to Chat Report / Thread	From thread to Chat Report
Simulator.Screen.Recording.-.iPhone.14.-.2023-06-16.at.13.57.11.mp4	Simulator.Screen.Recording.-.iPhone.14.-.2023-06-16.at.13.57.31.mp4

[fedirjh]

@bernhardoj That’s an interesting proposal 👍🏼

[mountiny]

that would be a separate issue

[fedirjh]

@bernhardoj Is it possible that we use hooks + context for your solution ?

[bernhardoj]

@fedirjh sorry, which part exactly do you wondering whether it's possible to use hooks or not?

[fedirjh]

@bernhardoj The ReportScrollManager, can’t we migrate it a custom hook that exports ref + methods ? something similar to , I think that would works ?

function useReportScrollManager(props) {
    const flatListRef = useContext(ReportListRefContext);  

   const scrollToIndex = (index) => {
    flatListRef.current.scrollToIndex(index);
  };

  const scrollToBottom = () => {
    if (!flatListRef.current) {
      return;
    }

    flatListRef.current.scrollToOffset({ animated: false, offset: 0 });
  };

  return [flatListRef, scrollToIndex, scrollToBottom];
}

[bernhardoj]

I guess that would work, we will create 2 useReportScrollManager, index.js and index.native.js.

[melvin-bot]

Reviewing label has been removed, please complete the "BugZero Checklist".

[melvin-bot]

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.3.33-4 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• Fix crash when edit a message after coming back from a child report #21227

If no regressions arise, payment will be issued on 2023-07-05. 🎊

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

• External issue reporter
• Contributor that fixed the issue
• Contributor+ that helped on the issue and/or PR

As a reminder, here are the bonuses/penalties that should be applied for any External issue:

• Merged PR within 3 business days of assignment - 50% bonus
• Merged PR more than 9 business days after assignment - 50% penalty

[melvin-bot]

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

• [@fedirjh] The PR that introduced the bug has been identified. Link to the PR:
• [@fedirjh] 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:
• [@fedirjh] 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:
• [@fedirjh] Determine if we should create a regression test for this bug.
• [@fedirjh] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
• [@dylanexpensify] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[dylanexpensify]

Nice! Will get payments today, @fedirjh will you work on list?

[fedirjh]

• Link to the PR: This is a bug from the new navigation refactor.
• Link to discussion: I don’t think there’s any way to catch this type of bug sooner. The refactor has touched multiple parts including this one. we should have a regression test for this case.
• Determine if we should create a regression test for this bug. ✅

---

Regression Test Proposal

1. Open any chat
2. Send a new message
3. Press Reply in thread on the new message
4. Send a message in the thread
5. Go back to main chat
6. Edit the message
7. Verify the message enters edit mode and no crash

Do we agree 👍 or 👎

[dylanexpensify]

@fedirjh @bernhardoj mind applying here??

[bernhardoj]

Applied

[dylanexpensify]

ty @bernhardoj! Will pay out today!

[dylanexpensify]

@bernhardoj offer sent!

[dylanexpensify]

@fedirjh you're paid via ND, right?

[fedirjh]

@dylanexpensify Nope , I think that I already applied to the upwork job.

[dylanexpensify]

Sorry y'all missed the window before job closed, @fedirjh mind applying here

[fedirjh]

@dylanexpensify Thank you! Applied.

[dylanexpensify]

Offfer sent!

[dylanexpensify]

payment sent, thanks @fedirjh!!

[dylanexpensify]

done!