Android - Crash when editing a message

[isagoico]

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

---

Action Performed:

1. Launch the app
2. Log in with any account
3. Search for any user
4. Find you message in the conversation
5. Click on Edit
6. Write longer message

Expected Result:

Able to edit a message

Actual Result:

App crashes after editing the message

Workaround:

Can the user still use Expensify without this being fixed? Have you informed them of the workaround?

Platform:

Where is this issue occurring?

Web
iOS
Android ✔️
Desktop App
Mobile Web

Version Number: 1.0.69-0

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

Notes/Photos/Videos: Any additional supporting documentation

IMPORTANT NOTE: This was only reproducible constantly for 1 tester on a Samsung Galaxy S21/11. Attaching crash logs.

Crash logs

Bug5114780_Screen_Recording_20210615-160124_Expensifycash.mp4

Expensify/Expensify Issue URL: https://github.com/Expensify/Expensify/issues/167571

View all open jobs on Upwork

Upwork job: https://www.upwork.com/jobs/~01b31c0c7bf061f50b

[MelvinBot]

Triggered auto assignment to @Jag96 (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

[Jag96]

I can see this crash in crashlytics.

The error here is

Fatal Exception: java.lang.IndexOutOfBoundsException setSpan (107 ... 107) ends beyond length 106

android.text.SpannableStringBuilder.checkRange (SpannableStringBuilder.java:1335)
android.text.SpannableStringBuilder.setSpan (SpannableStringBuilder.java:694)
android.text.SpannableStringBuilder.setSpan (SpannableStringBuilder.java:686)
android.text.Selection.setSelection (Selection.java:94)
android.text.Selection.setSelection (Selection.java:78)
android.widget.EditText.setSelection (EditText.java:152)
com.facebook.react.views.textinput.ReactEditText.setSelection (ReactEditText.java:340)

It looks like this react-native issue could be related: facebook/react-native#29063. I haven't been able to reproduce on my iPhone or on the Android emulator, so there may be more to this. cc @parasharrajat since this is partially related to the discussion going on in #3434 (comment).

All that being said, this is an External issue so applying the External label.

[MelvinBot]

Triggered auto assignment to @NicMendonca (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

[MelvinBot]

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

[NicMendonca]

Posted to Upwork: https://www.upwork.com/jobs/~01b31c0c7bf061f50b

[aliabbasmalik8]

@Jag96 I tried this solution and it's not fixed that issue

PROPOSAL 1

How can we produce it?
type a little bit slowly and wait.

Why did this issue happen?
as we know that setState takes little time to update the state, so before updating the state we saved this.state.draft so after updating props, it's rerendered with the previous state draft, and in this case, selection have start and end position greater than text length

How to fix it?
We need to update the below-mentioned functions.

https://github.com/Expensify/Expensify.cash/blob/5394fc8c55f860e98d926076cbac8221fea790b7/src/pages/home/report/ReportActionItemMessageEdit.js#L85

https://github.com/Expensify/Expensify.cash/blob/5394fc8c55f860e98d926076cbac8221fea790b7/src/pages/home/report/ReportActionItemMessageEdit.js#L65

like this

updateDraft(newDraft) {
    this.textInput.setNativeProps({text: newDraft});
    this.setState({draft: newDraft});
    this.debouncedSaveDraft(newDraft);
}

debouncedSaveDraft(newDraft) {
    saveReportActionDraft(this.props.reportID, this.props.action.reportActionID, newDraft);
}

PROPOSAL 2

@Jag96 you are right selection has some issues but I tested well and issue produced on IOS and yes we can fix it by removing the selection prop for Android as we already not using this prop on IOS. and after that, I also need to handle some more work that relates to the emoji position on android and can do that under one PR.

Thanks

[Jag96]

I posted on #3691 that I was unable to reproduce an issue w/ the same error message on 1.0.73-0, since I'm unable to reproduce this on my emulator @isagoico can we get a retest of this issue for the tester on the Samsung Galaxy S21/11 on the latest E.cash version? Just to confirm it hasn't been fixed by that version

[NikkiWines]

@isagoico just bumping @Jag96's comment above about retesting this issue, in case you missed it!

[isagoico]

My bad, we had retested this and forgot to update here. Issue is still ocurring to the same same tester on the same device latest build.

[NikkiWines]

@isagoico is this only reproducible for this one tester on the one device? Or can we confirm that it can be reproduced on multiple android devices? Either way though, since it's reproducible on at least one device, do we want to accept the above proposal for this one @Jag96?

[isagoico]

Yep! so far it has been reproducible only on 1 device for 1 tester.

[Jag96]

@NikkiWines I haven't had a chance to review the proposals, @aliabbasmalik8 is Proposal 1 preferred over Proposal 2? For Proposal 2 can you expand on and after that, I also need to handle some more work that relates to the emoji position on android.

[aliabbasmalik8]

@Jag96

@aliabbasmalik8 is Proposal 1 preferred over Proposal 2?

YES

and after that, I also need to handle some more work that relates to the emoji position on android

Proposal 2 approaches will break the selection position on android in ReportActionCompose component so also handle that issues but I prefer solution 1.

Thanks

[Jag96]

Asked internally to get another set of eyes on the proposal to make sure I'm not missing any potential side effects.

[roryabraham]

So to be clear, you're saying that the problem occurs because the trim function is being used on the draft message, and is causing the selection state to get out-of-sync with the draft message, such that the selection is attempting to index beyond the bounds of the draft message?

And the only change in Proposal 1 is to not trim the draft before updating the state? If so, it seems reasonable. But I do wonder if a better solution would be to fix this upstream in React Native...

Also, do we know the context for when + why the selection prop was disabled on iOS?

[parasharrajat]

One question. If selection prop is disabled on IOS, then how do we manage cursor position there?

[aliabbasmalik8]

@roryabraham for proposal 1, I passed newDraft directly to debouncedSaveDraft function and called this function saveReportActionDraft with newDraft value.

updateDraft(newDraft) {
    this.textInput.setNativeProps({text: newDraft});
    this.setState({draft: newDraft});
    this.debouncedSaveDraft(newDraft);
}
debouncedSaveDraft(newDraft) {
    saveReportActionDraft(this.props.reportID, this.props.action.reportActionID, newDraft);
}

the purpose of doing this is setState takes some time to update the state so when we saved value at the backend before updating the state then the previous value saved and this.props. draftMessage have the previous message and the selection is attempting to index beyond the bounds of the draft message

https://github.com/Expensify/Expensify.cash/blob/5394fc8c55f860e98d926076cbac8221fea790b7/src/pages/home/report/ReportActionItemMessageEdit.js#L122

We can also achieve this by using this way

updateDraft(newDraft) {
    this.textInput.setNativeProps({text: newDraft});
    this.setState({draft: newDraft}, () => {
         this.debouncedSaveDraft(); // called function in callback when state update successfully
    }
);
}

debouncedSaveDraft() {
    saveReportActionDraft(this.props.reportID, this.props.action.reportActionID, this.state.draft);
}

Also, do we know the context for when + why the selection prop was disabled on iOS?

Selection Props have a serious problem with Texinput on IOS and it's disabled from the very start.
https://github.com/facebook/react-native/issues/29063

https://github.com/Expensify/Expensify.cash/blob/c5707430dff9ecfb631012bb0ba25357c7f367e5/src/components/TextInputFocusable/index.ios.js#L72

One question. If selection prop is disabled on IOS, then how do we manage the cursor position there?

As you know that we are using this prop onSelectionChange to save selection position and used it when we add emojis in mid of the message but the selection prop particular used to handle cursor issue on android and web because after adding emojis cursor not update the position in web and android but this issue did not occur in IOS.

[roryabraham]

I'm sorry @aliabbasmalik8, I am having a lot of difficulty understanding your last comment. First, can you help me understand the race condition you're describing, and why not trimming the newDraft fixes it? Maybe with an example?

[aliabbasmalik8]

@roryabraham Now it can be resolved by updating this function updateDraft

https://github.com/Expensify/Expensify.cash/blob/57f557cd31198a631305b617c374f2418eb5e9fa/src/pages/home/report/ReportActionItemMessageEdit.js#L69-L74

like this

updateDraft(newDraft) {
    this.textInput.setNativeProps({text: newDraft});
    this.setState({draft: newDraft});
    this.debouncedSaveDraft(newDraft);
}

and 2nd point already resolved by someone else by update code from defaultValue={this.props.draftMessage} to defaultValue={this.state.draft} at here
https://github.com/Expensify/Expensify.cash/blob/57f557cd31198a631305b617c374f2418eb5e9fa/src/pages/home/report/ReportActionItemMessageEdit.js#L126

Thanks

[NicMendonca]

@roryabraham bumping @aliabbasmalik8's proposal ☝️

[Jag96]

@aliabbasmalik8 I have the same question that @roryabraham had previously, why does not trimming the newDraft fix the issue here?

[aliabbasmalik8]

@Jag96 right now not trimming will fix the issue.
I mentioned here #3599 (comment)

and the first point(that mentioned in the first proposal) is already resolved.

[Jag96]

@aliabbasmalik8 can you provide an example to explain why not trimming fixes the issue? I don't understand what the actual issue is and how not trimming the value fixes it.

[aliabbasmalik8]

@Jag96
we used selection in TextInputFocusable props so when we update the message then selection(cursor position) also update and if we use space at the end of the message then trim will remove that space so selection(cursor position) goes beyond the message length so it'll create this issue

https://github.com/Expensify/Expensify.cash/blob/57f557cd31198a631305b617c374f2418eb5e9fa/src/pages/home/report/ReportActionItemMessageEdit.js#L121-L136

[Jag96]

@aliabbasmalik8 thanks for the explanation, that sounds reasonable to me. @roryabraham is out this week, so @NikkiWines if you don't have any other questions I think this is good to move forward!

[NikkiWines]

No further questions from me. @NicMendonca you're good to hire @aliabbasmalik8 for this job.

[NicMendonca]

@aliabbasmalik8 can you please apply for the job in Upwork so we can start the contract? Thanks!

[aliabbasmalik8]

@NicMendonca applied for the JOB.
Here is my PROPOSAL: https://www.upwork.com/ab/proposals/1417930058476466177
Thanks

[NicMendonca]

@aliabbasmalik8 hired! Feel free to spin up the PR now. Thanks!

[arielgreen]

Paid.