[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 https://github.com/Expensify/App/pull/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
• [x] 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

https://github.com/Expensify/App/assets/78819774/d4abc7cb-2977-4e6b-823b-268c7bf41f43

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[bot]]

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

[melvin-bot[bot]]

Bug0 Triage Checklist (Main S/O)

• [x] This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
• [x] 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
• [x] 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.
• [x] 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.
• [x] I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

[melvin-bot[bot]]

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

[melvin-bot[bot]]

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

[melvin-bot[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 https://github.com/Expensify/App/blob/bd0e843621dda8ad1fb31595f0f923a9d9447cf8/src/libs/ReportScrollManager/index.native.js#L13 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:

     {
     }

     so that when InvertedFlatList of report A unmounts, it will only set the flatListRefsreportID A to null. Meanwhile the flatListRefsreportID 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 https://github.com/Expensify/App/pull/20296 , will try to revert and test it.

[fedirjh]

Ok Confirmed , reverting https://github.com/Expensify/App/pull/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 https://github.com/Expensify/App/issues/20513 I worked on previously (here is the proposal).

So, we have a global ref for the message list. https://github.com/Expensify/App/blob/bd0e843621dda8ad1fb31595f0f923a9d9447cf8/src/libs/ReportScrollManager/index.js#L3-L5 and is set in ReportActionsList https://github.com/Expensify/App/blob/bd0e843621dda8ad1fb31595f0f923a9d9447cf8/src/pages/home/report/ReportActionsList.js#L152-L155

When we close a chat, that ref will become null. It's not a regression from https://github.com/Expensify/App/pull/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 https://github.com/Expensify/App/issues/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. https://github.com/Expensify/App/blob/bd0e843621dda8ad1fb31595f0f923a9d9447cf8/src/libs/ReportScrollManager/index.js#L14 https://github.com/Expensify/App/blob/bd0e843621dda8ad1fb31595f0f923a9d9447cf8/src/libs/ReportScrollManager/index.js#L26

ReportScrollManager is also being used in MoneyRequestModal too which is a separate page from ReportScreen. https://github.com/Expensify/App/blob/bd0e843621dda8ad1fb31595f0f923a9d9447cf8/src/pages/iou/MoneyRequestModal.js#L398-L405

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: https://github.com/Expensify/App/blob/bd0e843621dda8ad1fb31595f0f923a9d9447cf8/src/pages/home/report/ReportActionsView.js#L102-L111

When we add a new action, we will trigger the listener here https://github.com/Expensify/App/blob/bd0e843621dda8ad1fb31595f0f923a9d9447cf8/src/libs/actions/Report.js#L299-L305

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[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

[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.

[fedirjh]

When we close a chat, that ref will become null.

@bernhardoj can you elaborate more on the root cause ? When navigate form child report to parent report , we close a chat and we open another chat , so on the second chat the ref should be reset again unless if something prevent that ?

[bernhardoj]

The first chat is never unmounted, so the ref stays null when going back to the first chat.

1. Open app. ref: undefined
2. Open first chat, page and list is mounted ref: list 1
3. Open second chat, page and list is mounted ref: list 2
4. Close second chat, list is unmounted ref: null

EDIT: Btw, do you think we should handle other global ref too?

[fedirjh]

So for first chat it should be re-rendered for the ref to be reset. I am suspecting something , the ref is created outside of the react component lifecycle and that's probably why it doesn’t reset automatically. I think your solution worked because we moved the ref inside the react component lifecycle.

Btw, do you think we should handle other global ref too?

We should handle all cases of ReportScrollManager

[bernhardoj]

So for first chat it should be re-rendered for the ref to be reset

I don't think ref will re-reset when the component is re-rendered, except when we use a callback ref with inline function. https://legacy.reactjs.org/docs/refs-and-the-dom.html#caveats-with-callback-refs

We should handle all cases of ReportScrollManager

I mean the other global ref like emoji picker

[fedirjh]

I mean the other global ref like emoji picker

I think we need to create a tracker issue for that and treat each ref case separately.

@bernhardoj Just one final question , did you tested your proposal on all platforms ?

[bernhardoj]

Yes, tested on all platforms.

Btw, there are still some class components that need the changes, so I don't think we can use hooks for now?

[dylanexpensify]

@fedirjh curious for your thoughts on this ^ when you get a chance today! 🙇‍♂️

[fedirjh]

Yes, tested on all platforms.

That's perfect

Btw, there are still some class components that need the changes, so I don't think we can use hooks for now?

@bernhardoj I suggest using hooks with functional components and updating the class components to include a comment regarding the use of hooks during the refactor. This comment will serve as a deprecation notice, ensuring that whoever performs the refactor will use the new hook. What are your thoughts on this approach ?

[bernhardoj]

Hmm, I honestly don't like leaving a comment reminding people to migrate it to hook 😅.

[bernhardoj]

Sooo, what should we do here? 😄. I guess we need another vote to push this forward? My previous comment is just a personal preference, so I'm up to any decision.

[fedirjh]

@bernhardoj I think we have another option that we can explore , What about using a HOC instead of a hook ? I think that will works for both class an functional component.

[bernhardoj]

Yep, that's what I thought too, but then I remember this comment that prefer hook over HOC, so If I need to choose, I would choose the hook option.

[fedirjh]

That's good, let’s proceed with the hook implementation.

[fedirjh]

@bernhardoj's proposal looks good to me.

🎀 👀 🎀 C+ reviewed

[melvin-bot[bot]]

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

[amyevans]

there are still some class components that need the changes

Can you list out which components these are? Is it just <ReportActionsView />? Perhaps we can comment on the migration GH tracking issue(s) for added reminder/visibility.

+1 to preferring hooks over HOC though.

[melvin-bot[bot]]

📣 @bernhardoj You have been assigned to this job by @amyevans! Please apply to this job in Upwork 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 📖

[bernhardoj]

Yes, it's just ReportActionsView. I thought we need the hook in ReportScreen too, but I forget we only define the ref and context there, so no hook is needed there.

[bernhardoj]

PR is ready for review.

Perhaps we can comment on the migration GH tracking issue(s) for added reminder/visibility.

Should I add a comment there now or after the PR is merged?

[amyevans]

After the PR is merged sounds best

[melvin-bot[bot]]

Based on my calculations, the pull request did not get merged within 3 working days of assignment. Please, check out my computations here:

• when @bernhardoj got assigned: 2023-06-21 14:32:02 Z
• when the PR got merged: 2023-06-26 15:53:09 UTC
• days elapsed: 3

On to the next one 🚀

[bernhardoj]

After the PR is merged sounds best

Added the hook migration comment here https://github.com/Expensify/App/issues/16267#issuecomment-1608797896

[melvin-bot[bot]]

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

[melvin-bot[bot]]

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

• https://github.com/Expensify/App/pull/21227

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

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[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]

• [x] Link to the PR: This is a bug from the new navigation refactor.
• [x] 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.
• [x] 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