[$500] Compose - The send button is inactive after deleting text with more than 10,000 characters

[lanitochka17]

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: 1.4.34-0 Reproducible in staging?: Y Reproducible in production?: Y If this was caught during regression testing, add the test name, ID and link from TestRail: https://expensify.testrail.io/index.php?/tests/view/4257780 Email or phone of affected tester (no customers): Logs: https://stackoverflow.com/c/expensify/questions/4856 Expensify/Expensify Issue URL: Issue reported by: Applause - Internal Team Slack conversation:

Action Performed:

Prerequisites: You may use this site to copy the text https://www.lipsum.com/feed/html

1. Open https://staging.new.expensify.com/
2. Log in under your HT account
3. Navigate to any conversation
4. Quickly paste 5-6 times text of more than 3000 characters into Compose Box
5. Press Enter until the error about exceeding the limit of 10000 characters appears
6. Highlight the inserted text
7. Enter any message

Expected Result:

The send message button should become active after deleting text containing more than 10,000 characters

Actual Result:

The send message button is inactive after deleting text with more than 10,000 characters

Workaround:

Unknown

Platforms:

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

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

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/Expensify/App/assets/78819774/354a15c2-5c19-45ac-b137-6f0038d19dea

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~018a5065c30f70c29b
• Upwork Job ID: 1752871278488870912
• Last Price Increase: 2024-02-01

[melvin-bot[bot]]

Job added to Upwork: https://www.upwork.com/jobs/~018a5065c30f70c29b

[melvin-bot[bot]]

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

[melvin-bot[bot]]

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

[lanitochka17]

We think that this bug might be related to #vip-vsp CC @quinthar

[dukenv0307]

Proposal

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

The send message button is inactive after deleting text with more than 10,000 characters

What is the root cause of that problem?

When we submit the comment, we'll set the isCommentEmpty to true here without checking if the comment is valid. However the max length validation error subsequently happens here, so it early returns doesn't actually set the comment to empty.

The isCommentEmpty remaining true causes the Send button to be disabled due to this logic.

Regarding the red border and error message shown to the user, it was done in a debounce here to optimize for performance, so the error message shows up a bit later and it's expected.

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

In here, we need to validate the comment (including max length validation) synchronously and early return if there's validation error. We can do that in many ways, for example by:

1. Extract the comment validation logic to here

const isCommentValid = useCallback((comment) => {
    const trimmedComment = comment.trim();
    const commentLength = ReportUtils.getCommentLength(trimmedComment);

    // Don't submit empty comments or comments that exceed the character limit
    if (!commentLength || commentLength > CONST.MAX_COMMENT_LENGTH) {
        return false;
    }

    return true;
}, []);

And use that common method to validate in prepareCommentAndResetComposer 2. Then here, expose that method so it can be used in ReportActionCompose

isCommentValid: () => isCommentValid(commentRef.current),

3. In here synchronously validate the comment length using the above method, and any other comment validation if applicable. If the validation fails, early return.

if (!composerRef.current.isCommentValid()) {
    return;
}

4. [Optional] When the input exceeds max length and we click on Send button quickly, it will take a bit for the user to realize what goes wrong due to this debounce for optimization, we can improve it a bit to make the error shows immediately after clicking Send, by exposing the setHasExceededMaxCommentLength in the useHandleExceedMaxCommentLength hook, and then in the check in 3, if the input exceeds max length, setHasExceededMaxCommentLength(true) immediately.

5. [Optional] As a clean up, we can consider removing the validation logic here since we already validate earlier in 2

Unrelated but in useHandleExceedMaxCommentLength we're not trimming the comment before calculating the length like other places, we might want to trim there to make it consistent.

What alternative solutions did you explore? (Optional)

Move this logic to here instead, so it will only update isCommentEmpty to true after validation and it actually clears the comment.

Another approach is to move the early return logic here to here. Just a side not but this prevent default should happen after we validate, not before.

[jeremy-croff]

Proposal

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

We run into an issue where the use cannot send a message because the input thinks it's disabled

What is the root cause of that problem?

A performance optimization was implemented here: https://github.com/Expensify/App/blob/dc3c39609700909cedea4655bad13900d6305e69/src/pages/home/report/ReportActionCompose/ComposerWithSuggestions/ComposerWithSuggestions.js#L268 That prevents the setIsCommentEmpty callback from updating unnecessarily if it didn't change. It uses the actual input text data to confirm if it's empty or not.

One level higher, we are stuck in an use case where a state variable isCommentEmpty is true here: https://github.com/Expensify/App/blob/dc3c39609700909cedea4655bad13900d6305e69/src/pages/home/report/ReportActionCompose/ReportActionCompose.js#L341 Even when there's text in the input, and when you keep on typing, it keeps failing the downstream check that determines if the setIsCommentEmpty should be called. Since it now only calls when the actual input goes from empty to filled, or filled to empty. Because there was always text in the input, it notices no change and does not call setIsCommentEmpty(false) to update the parent state and remove the disabled send button.

The root cause is that we use a debounced method for max length check: https://github.com/Expensify/App/blob/dc3c39609700909cedea4655bad13900d6305e69/src/hooks/useHandleExceedMaxCommentLength.ts#L22 and it allows us to hit "send" before we actually finished validating the max length. Which improperlys sets the local isCommentEmpty state as empty here https://github.com/Expensify/App/blob/dc3c39609700909cedea4655bad13900d6305e69/src/pages/home/report/ReportActionCompose/ReportActionCompose.js#L351 Conflicting with the real check down stream, that is not able to update it isCommentEmpty back to false because it's not detecting the change because of the performance optimization logic it has.

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

We can remove the performance optimization, so that it will re-update the empty state on text change.

Alternatively, if we want to keep the performance optimization, we can pass the isEmpty state as a prop to ComposerWithSuggestions, and update it only if the current state is different from the prop.

What alternative solutions did you explore? (Optional)

[dukenv0307]

Proposal updated to clarify the RCA and add an alternative solution

Move this logic to here instead, so it will only update isCommentEmpty to true after validation and it actually clears the comment.

[melvin-bot[bot]]

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

[allroundexperts]

Still reviewing the proposals.

[allroundexperts]

Reviewing again now!

[allroundexperts]

Thanks for your proposal @jeremy-croff. I don't think we should remove the optimisation in place. The alternate solution that you've suggested doesn't seem ideal to me as well since we will be having a useless render cycle ie isCommentEmpty set as true, and then from the child, being set as false.

I like @dukenv0307's proposal of making the check inside handleSendMessage rely on the length of the message synchronously. That would prevent us from removing any optimisation. Let's go with them.

🎀 👀 🎀 C+ reviewed

[melvin-bot[bot]]

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

[jeremy-croff]

Thanks for your proposal @jeremy-croff. I don't think we should remove the optimisation in place. The alternate solution that you've suggested doesn't seem ideal to me as well since we will be having a useless render cycle ie isCommentEmpty set as true, and then from the child, being set as false.

I like @dukenv0307's proposal of making the check inside handleSendMessage rely on the length of the message synchronously. That would prevent us from removing any optimisation. Let's go with them.

🎀 👀 🎀 C+ reviewed

Good analysis!

[mallenexpensify]

@youssef-lr this one slipped through the cracks. I didn't add it to a project and I'm unsure of the value of implementing the fix, it's seems rare that posts with 10k added/removed will happen. What do you think?

@dukenv0307 will you proposed fix help in other instances outside of just add/remove 10k characters?

[youssef-lr]

@mallenexpensify yeah I'm with you on this, I don't think this is worth fixing right now, especially that there is easily a workaround, for example in the recording below I managed to easily send another comment after removing the 10k text.

https://github.com/Expensify/App/assets/9680864/1d8de98e-64fc-4598-925b-9d01719f9795

[dukenv0307]

@mallenexpensify yeah I'm with you on this, I don't think this is worth fixing right now, especially that there is easily a workaround, for example in the recording below I managed to easily send another comment after removing the 10k text.

@youssef-lr Just checking if you pressed Send/press Enter to send after filling in the long 10000+ characters paragraph? If you did, subsequently you won't be able to use the Composer at all. In my testing, the Composer is pretty much unusable after following the steps to reproduce, I cannot send via Enter, the button to send is disabled, ... although there's text in the Composer.

@dukenv0307 will you proposed fix help in other instances outside of just add/remove 10k characters?

@mallenexpensify Yes, there's also another related bug due to the same root cause:

1. Expand the composer to full size
2. Enter 10000+ characters paragraph and press Enter
3. The composer size will flicker (it will reduce to smaller size, no longer full size)

I'm unsure of the value of implementing the fix, it's seems rare that posts with 10k added/removed will happen.

@mallenexpensify If the outcome if the bug happens is negligile (like just a style is off or something unexpected shows, ...), I would've agreed with you.

But in this case the Composer (which is the core feature in the app) will become totally unusable, blocking the user. Also the solution is straight forward and won't take much effort, I think we should fix it.

IMO (Rare occurence x big impact when occuring) = worth fixing (Rare occurence x low impact when occuring) = not worth

[jeremy-croff]

@mallenexpensify yeah I'm with you on this, I don't think this is worth fixing right now, especially that there is easily a workaround, for example in the recording below I managed to easily send another comment after removing the 10k text.

Screen.Recording.2024-02-07.at.23.11.41.mov

It seems this recording doesn't reproduce the bug mention in OP. The bug occurs if you pasted 10k+ text, hit send immediately, then clear it and type before the text limit debounce happens. Then you cannot send your text input anymore. The only way to get out of that state is to clear your message and start over. It's edge case for sure, but is a clear bug that's identified and fixable, even if it just improves one customers experience down the line. If we stack ranked it, it would be low priority for sure because of the difficulty to get in this state.

[mallenexpensify]

Thanks for the context @dukenv0307 and @jeremy-croff . @allroundexperts , can you please review and chime in?

Expand the composer to full size

@dukenv0307 can you... err... expand on this? Or outline reproducible steps? Curious how frequently this might happen

IMO (Rare occurence x big impact when occuring) = worth fixing (Rare occurence x low impact when occuring) = not worth

@dukenv0307 I like this framing.

[allroundexperts]

@mallenexpensify In my opinion, if something is clearly faulty in the code (like the case here), we should fix it regardless if its small or large. Although, I'd agree that someone would get into this state rarely.

[mallenexpensify]

Thanks @allroundexperts , We started the contributor program by fixing EVERY bug we could find, now we're focusing on bugs that we can assign to our roadmap projects. For this one, the likely project would be #vip-vsb, and with so many issues already in/on that project, I'm unsure the fix here would be viewed as valuable enough to add.

[dukenv0307]

@dukenv0307 can you... err... expand on this? Or outline reproducible steps? Curious how frequently this might happen

@mallenexpensify

Here're the steps to reproduce that additional bug with the same root cause:

1. Write some text in the Composer
2. Expand the Composer to full size
3. Paste a paragraph with more than 10,000 characters
4. Click Send

We'll see the Composer height flickers and becomes a non-full-size composer.

Here's the video of the same:

https://github.com/Expensify/App/assets/129500732/ee3ce3a5-b127-44b1-9225-12877c80f060

@dukenv0307 I like this framing.

The one mentioned in the OP is high severity if it occurs, since the user won't be able to use the Composer at all.

[mallenexpensify]

Thanks @dukenv0307 , I'm pausing for a sec, trying to think of the next best step here, more in a day or two.

[mvtglobally]

Issue not reproducible during KI retests. (First week)

[mallenexpensify]

I'm going to close, for now. I just can't see users pasting in 10k+ characters intentionally, then deleting, is going to happen often, if ever.