App icon indicating copy to clipboard operation
App copied to clipboard

[$250] iOS - App crashes when pasting large text to the composer and backgrounding the app

Open IuliiaHerets opened this issue 1 year ago • 38 comments

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.50-5 Reproducible in staging?: Y Reproducible in production?: Y If this was caught on HybridApp, is this reproducible on New Expensify Standalone?: Y Issue was found when executing this PR: https://github.com/Expensify/App/pull/50487 Issue reported by: Applause Internal Team

Action Performed:

  1. Navigate to https://www.lipsum.com/
  2. Generate text with 10 000 words
  3. Copy the generated text and the title and quotes under in at the top of the page
  4. Open the app
  5. Log in with a Gmail account
  6. Open any 1:1 DM
  7. Long tap inside the composer
  8. Tap on "Paste"
  9. Background the app
  10. Foreground the app
  11. Try to interact with the app
  12. Repeat steps 9-11 until it crashes

Expected Result:

If the pasted text is longer than 10,000 characters, we should trim the text to 10,000 characters.

We can use a similar pattern to how we indicate the character limit in the Workspace name input field (but it would say Character limit reached (10,000/10,000) instead, or something similar)

Actual Result:

When pasting large text to the composer and backgrounding the app a few times, we attempt to paste the full text string, and the app crashes.

Workaround:

Unknown

Platforms:

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

Screenshots/Videos

https://github.com/user-attachments/assets/bd7c2898-c673-4922-84c2-4d66cad52101

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~021859632203774875578
  • Upwork Job ID: 1859632203774875578
  • Last Price Increase: 2024-12-05
  • Automatic offers:
    • wildan-m | Contributor | 105221106

IuliiaHerets avatar Oct 17 '24 20:10 IuliiaHerets

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

melvin-bot[bot] avatar Oct 17 '24 20:10 melvin-bot[bot]

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

IuliiaHerets avatar Oct 17 '24 20:10 IuliiaHerets

Is this a separate bug from https://github.com/Expensify/App/issues/48888? Or is it a regression from that issue's PR?

If we don't allow users to send messages over 10k characters, why would we allow them to paste over 10k characters in the composer? Why don't we just cut off everything over 10k?

sakluger avatar Oct 17 '24 20:10 sakluger

cc @anmurali @jasperhuangg @Pujan92 curious for your thoughts on the expected behavior here since you all were involved in https://github.com/Expensify/App/issues/48888.

sakluger avatar Oct 17 '24 20:10 sakluger

@sakluger Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

melvin-bot[bot] avatar Oct 21 '24 18:10 melvin-bot[bot]

I posted to Slack (https://expensify.slack.com/archives/C05LX9D6E07/p1729615084337139) asking for feedback on the expected behavior.

sakluger avatar Oct 22 '24 16:10 sakluger

@sakluger I can't reproduce the crash. Also, this isn't a regression from the other issue.

https://github.com/user-attachments/assets/a5b2307f-e52e-42a8-ba2b-f56d7386bfae

Pujan92 avatar Oct 22 '24 17:10 Pujan92

This feels like a major edge case, especially given that @Pujan92 can't reproduce at all.

I'm going to close the issue for now. @IuliiaHerets - if you're still able to reproduce it, feel free to reopen. At that point, if we can consistently reproduce, then we'll likely change the behavior to auto-clip the text to 10k max characters.

sakluger avatar Oct 22 '24 21:10 sakluger

Issue is still reproducible on the latest build 9.0.63-3

https://github.com/user-attachments/assets/a9d9668e-caf0-46a6-946a-8531d1e91193

lanitochka17 avatar Nov 18 '24 18:11 lanitochka17

@lanitochka17 thanks for sharing the new video. Just to confirm, did you use the exact same reproduction steps, or did you modify the steps in any way?

sakluger avatar Nov 19 '24 22:11 sakluger

I updated the expected behavior in the OP to indicate that we should trim to 10,000 characters if the user tries pasting more than that.

sakluger avatar Nov 21 '24 16:11 sakluger

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

melvin-bot[bot] avatar Nov 21 '24 16:11 melvin-bot[bot]

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

melvin-bot[bot] avatar Nov 21 '24 16:11 melvin-bot[bot]

@sakluger The tester used the same steps.

lanitochka17 avatar Nov 22 '24 14:11 lanitochka17

Edited by proposal-police: This proposal was edited at 2024-12-06 06:16:13 UTC.

Proposal

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

iOS - App crashes when pasting large text to the composer and backgrounding the app

What is the root cause of that problem?

New feature if it's follow this expected behavior https://github.com/Expensify/App/issues/51059#issuecomment-2491680722

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

We can trim the comment to CONST.MAX_COMMENT_LENGTH

in handlePastedHTML and handlePastePlainText at useHtmlPaste


    const handlePastedHTML = useCallback(
        (html: string) => {
            // Trim the HTML content if it exceeds the max length
            const trimmedHtml = html.length > CONST.MAX_COMMENT_LENGTH 
                ? html.substring(0, CONST.MAX_COMMENT_LENGTH) 
                : html;

            paste(Parser.htmlToMarkdown(trimmedHtml));
        },
        [paste],
    );

    const handlePastePlainText = useCallback(
        (event: ClipboardEvent) => {
            const plainText = event.clipboardData?.getData('text/plain');
            // Trim the text content if it exceeds the max length
            const trimmedText = plainText && plainText.length > CONST.MAX_COMMENT_LENGTH
                ? plainText.substring(0, CONST.MAX_COMMENT_LENGTH)
                : plainText;

            if (trimmedText) {
                paste(trimmedText);
            }
        },
        [paste],

Then modify useHandleExceedMaxCommentLength to also return isEqualToMaxCommentLength and commentLength

src/hooks/useHandleExceedMaxCommentLength.ts

const useHandleExceedMaxCommentLength = () => {
    const [hasExceededMaxCommentLength, setHasExceededMaxCommentLength] = useState(false);
    const [isEqualToMaxCommentLength, setIsEqualToMaxCommentLength] = useState(false);
    const [commentLength, setCommentLength] = useState(0);

    const handleValueChange = useCallback(
        (value: string, parsingDetails?: ParsingDetails) => {
            const length = ReportUtils.getCommentLength(value, parsingDetails);
            const isMaxLength = length === CONST.MAX_COMMENT_LENGTH;
            const hasExceeded = length > CONST.MAX_COMMENT_LENGTH;

            setCommentLength(length);

            if (isMaxLength !== isEqualToMaxCommentLength) {
                setIsEqualToMaxCommentLength(isMaxLength);
            }

            if (hasExceeded !== hasExceededMaxCommentLength) {
                setHasExceededMaxCommentLength(hasExceeded);
            }
        },
        [hasExceededMaxCommentLength, isEqualToMaxCommentLength],
    );
    const validateCommentMaxLength = useMemo(() => debounce(handleValueChange, 1500, {leading: true}), [handleValueChange]);

    return {hasExceededMaxCommentLength, validateCommentMaxLength, isEqualToMaxCommentLength, commentLength};
};

We separate isEqualToMaxCommentLength and hasExceededMaxCommentLength because we'll show the validation message but not disabling the button if the length is equal to maxlength.

Adjust ReportActionCompose and ReportActionItemMessageEdit to show the ExceededCommentLength component if it meet this condition : (isEqualToMaxCommentLength || hasExceededMaxCommentLength)

Modify ExceededCommentLength to accept commentLength props so we can have 10.000/10.000 validation message format


type ExceededCommentLengthProps = {
    commentLength: number;
};

function ExceededCommentLength({commentLength}: ExceededCommentLengthProps) {
    const styles = useThemeStyles();
    const {numberFormat, translate} = useLocalize();

	 function ExceededCommentLength() {
            style={[styles.textMicro, styles.textDanger, styles.chatItemComposeSecondaryRow, styles.mlAuto, styles.pl2]}
            numberOfLines={1}
        >
            {translate('composer.commentExceededMaxLength', { formattedLength: numberFormat(commentLength), formattedMaxLength: numberFormat(CONST.MAX_COMMENT_LENGTH) })}

Adjust the message accordingly

Branch for this solution

What alternative solutions did you explore? (Optional)

Trim to 10.000 will not make user aware that the text trimmed, we can trim to CONST.MAX_COMMENT_LENGTH + 1 or more to make user see the error and also prevent potential crash, or create new warning/info text if we trim the text so the user will notice

wildan-m avatar Nov 23 '24 03:11 wildan-m

I think what @wildan-m proposed does make sense. Is trimming an option here @sakluger?

allroundexperts avatar Nov 24 '24 22:11 allroundexperts

@allroundexperts I commented here updated the OP to clarify that we do want to trim to 10k characters if the user tries pasting more.

We should indicate in the UI that the text was trimmed. In the OP, I said We can use a similar pattern to how we indicate the character limit in the Workspace name input field (but it would say Character limit reached (10,000/10,000) instead, or something similar). Here's what the UI looks like in the Workspace name input field:

image

@wildan-m can we add that Character limit reached (10,000/10,000) validation error in the proposal? The validation error should only appear if the limit is reached.

sakluger avatar Nov 25 '24 16:11 sakluger

@sakluger I think we already have that

image

but we should insert 10.001 character or more to show that did you mean we should also count the current length? (1323/10.000)

or we can show a pop up?

image

wildan-m avatar Nov 25 '24 20:11 wildan-m

Hey @wildan-m, thanks for adding that screenshot, I didn't realize we already had the length validation error for this field.

I'm not sure if adding 0.1 characters is a good idea, I'll let @allroundexperts comment on that. Alternatively, we could just show the validation when character length > 9999, right?

The pop up warning is not a bad idea either, but let's stick to trimming for now.

sakluger avatar Nov 26 '24 00:11 sakluger

@sakluger show validation at 9999 might make user think they've reached the 10.000, but 1 char left. We might need a separate message, or maybe consult with the design team?

wildan-m avatar Nov 26 '24 01:11 wildan-m

@sakluger, @allroundexperts Whoops! This issue is 2 days overdue. Let's get this updated quick!

melvin-bot[bot] avatar Nov 28 '24 09:11 melvin-bot[bot]

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

melvin-bot[bot] avatar Nov 28 '24 16:11 melvin-bot[bot]

@sakluger, @allroundexperts 6 days overdue. This is scarier than being forced to listen to Vogon poetry!

melvin-bot[bot] avatar Dec 02 '24 09:12 melvin-bot[bot]

@wildan-m I meant that the validation would check for > 9999 characters, not >= 9999 characters. So 9999 wouldn't trigger it, but 10,000 would.

@allroundexperts what do you think is best?

sakluger avatar Dec 02 '24 17:12 sakluger

I'd agree. Let's show the message when 10,000 characters are reached.

allroundexperts avatar Dec 02 '24 21:12 allroundexperts

@sakluger @allroundexperts do we need to change the validation text?

While the current validation text might lead the user to believe that their message exceeds 10,000 characters, causing them to delete a character in order to avoid the validation message.

wildan-m avatar Dec 02 '24 22:12 wildan-m

Sure, let's change the validation text to:

Maximum character limit reached (10,000/10,000)

sakluger avatar Dec 04 '24 23:12 sakluger

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

melvin-bot[bot] avatar Dec 05 '24 16:12 melvin-bot[bot]

Proposal Updated

  • Add branch link
  • Add adjustment for 10.000/10.000 validation message format based on this comment

wildan-m avatar Dec 06 '24 06:12 wildan-m

@sakluger, @allroundexperts Whoops! This issue is 2 days overdue. Let's get this updated quick!

melvin-bot[bot] avatar Dec 06 '24 09:12 melvin-bot[bot]