[HOLD for payment 2023-12-26] [$500] No error message when editing a chat to go past character limit

[m-natarajan]

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.3.95-5
Reproducible in staging?: y
Reproducible in production?: y
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
Expensify/Expensify Issue URL:
Issue reported by: @grgia
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1699281873727459

Action Performed:

1. Open app
2. Go to a report and enter > 10K characters
3. Observe the error message
4. Edit a message and enter >10k characters

Expected Result:

Error message should appear

Actual Result:

No error message and could not send the message

Workaround:

unknown

Platforms:

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

• Android: Native
• Android: mWeb Chrome
• iOS: Native
• iOS: mWeb Safari
• MacOS: Chrome / Safari
• MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

Recording.206.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~019dbff50a5a453b18
• Upwork Job ID: 1721581708178677760
• Last Price Increase: 2023-11-13
• Automatic offers:
• DylanDylann | Contributor | 27770738

Issue Owner

Current Issue Owner: @burczu

[melvin-bot]

Triggered auto assignment to @abekkala (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/~019dbff50a5a453b18

[melvin-bot]

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

[mpiniarski]

Proposal

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

When editing a message, length validation does not work.

What is the root cause of that problem?

Passing reportId to ExceededCommentLength.js in ReportActionItemMessageEdit.js

causes it to be tied to an incorrect value from Onyx store (inExceededCommentLength.js):

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

We don't need to pass reportId for ReportActionItemMessageEdit.js, as we are passing comment property.
.

What alternative solutions did you explore? (Optional)

[melvin-bot]

📣 @mpiniarski! 📣
Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork.
Please follow these steps:

1. Make sure you've read and understood the contributing guidelines.
2. Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
3. Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
4. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details.
   
   Format:

Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>

[erquhart]

Proposal

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

The 10,000 character max error message does not show when editing an existing message.

What is the root cause of that problem?

The ExceededCommentLength component checks the onyx store directly for the comment value. It is currently set up to check for the draft of a new comment, but not for the draft of an existing comment, which uses a different Onyx key.

This component was connected to Onyx to improve chat input performance:
https://github.com/Expensify/App/pull/25758/files#diff-6322282b36a11b64417bc4327aa7245c139a36643ce59f4cf32583d20164023e

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

ExceededCommentLength being tied to the Onyx store isn't ideal, but to retain the chat input performance gains that this achieves, we can allow consumers of the component to provide the draft key for Onyx:

// ReportActionCompose.js (new draft)
<ExceededCommentLength
    reportID={reportID}
    draftKey={ONYXKEYS.COLLECTION.REPORT_DRAFT_COMMENT}
    onExceededMaxCommentLength={setExceededMaxCommentLength}
/>

// ReportActionItemMessageEdit.js (edit draft)
<ExceededCommentLength
    reportID={reportID}
    draftKey={ONYXKEYS.COLLECTION.REPORT_ACTIONS_DRAFTS}
    onExceededMaxCommentLength={setExceededMaxCommentLength}
/>

This allows the component to be used with any comment, and allows the consumer to specify where to retrieve it. This requires limited changes to the ExceededCommentLength component and no changes to functionality.

Before:

export default withOnyx({
    comment: {
        key: ({reportID}) => `${ONYXKEYS.COLLECTION.REPORT_DRAFT_COMMENT}${reportID}`,
        initialValue: '',
    },
})(ExceededCommentLength);

After:

export default withOnyx({
    comment: {
        key: ({draftKey, reportID}) => `${draftKey}${reportID}`,
        initialValue: '',
    },
})(ExceededCommentLength);

This approach allows ExceededCommentLength to continue using onyx to retrieve the comment for performance purposes, rather than just passing the comment in as a prop, which would effectively undermine #25758.

Tested locally.

What alternative solutions did you explore? (Optional)

Making ExceededCommentLength a dumb component that just receives the comment through props and spits out a message. This is undesirable because it reverts some of the effect of #25758.

[bernhardoj]

Proposal

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

There is no exceeded length error message on the edit composer.

What is the root cause of that problem?

In edit composer component, we pass both comment and reportID props to the ExceededCommentLength component.

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

Lines 464 to 468 in ee0fb61

	<ExceededCommentLength
	comment={draft}
	reportID={props.reportID}
	onExceededMaxCommentLength={(hasExceeded) => setHasExceededMaxCommentLength(hasExceeded)}
	/>

reportID is used to get the draft text of the main composer, so when we use it on an edit composer, we always get an empty string (ofc if the main composer is empty), overriding the custom comment. Previously, ExceededCommentLength only accepts a comment props, but after a performance improvement PR (#25758), we started to accept reportID for the main composer so we don't need to subscribe to the main composer draft onyx on ReportActionCompose component.

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

1. Remove the onyx subscription from ExceededCommentLength
2. To get the comment from the onyx, create a new component called MainExceededCommentLength that will be used for the main composer only. This component will be the wrapper for the onyx connection.

const MainExceededCommentLength = (props) => {
    return <ExceededCommentLength comment={props.comment} onExceededMaxCommentLength={props.onExceededMaxCommentLength}/>
}

export default withOnyx({
    comment: {
        key: ({reportID}) => `${ONYXKEYS.COLLECTION.REPORT_DRAFT_COMMENT}${reportID}`,
        initialValue: '',
    },
})(MainExceededCommentLength);

[erquhart]

Proposal

Updated

• Recognized that the ExceededCommentLength component was connected to Onyx for performance reasons
• Changed recommendation to have consumers pass in the onyx key for the comment draft rather than hardcoding the key
• Allows minimal change, no new components need to be created
• Allows flexibility as new consumers can specify the key without needing to update logic in ExceededCommentLength.

[tienifr]

Proposal

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

No error message and could not send the message

What is the root cause of that problem?

We're passing reportID here as a param of ExceededCommentLength, and we're connecting to the report comment in ExceededCommentLength, so it will always yield the wrong (empty) draft because the ExceededCommentLength in message edit is supposed to get the report action comment draft instead.

Earlier ExceededCommentLength was refactored to that it would contain within itself the logic to fetch report comment, for performance purpose, but we didn't do the same in the message edit case, we still use the comment prop for message edit.

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

We should remove comment prop, accept another optional prop reportActionID in ExceededCommentLength, and in here, if both reportActionID and reportID exists, we'll retrieve the comment from REPORT_ACTIONS_DRAFTS by reportActionID and reportID instead.

That will make sure the ExceededCommentLength works well for both main composer and edit composer, and it will also improve the performance by reduce the unnecessary re-rendering of ExceededCommentLength in edit composer (similar to what we achieved when moving the Onyx call inside ExceededCommentLength earlier by introducing the reportID prop).

Detail Implementation:

1. Use reportActionID instead of comment here

reportActionID={props.action.reportActionID}

2. Change the key here based on reportActionID and reportID

        key: ({reportID, reportActionID}) => reportActionID && reportID? `${ONYXKEYS.COLLECTION.REPORT_ACTIONS_DRAFTS}${reportID}`:`${ONYXKEYS.COLLECTION.REPORT_DRAFT_COMMENT}${reportID}`,

3. Get the comment from props

    const comment = props.reportActionID && props.reportID? get(props,['comment', props.reportActionID],''): get(props, 'comment','')

4. Change props.comment to comment (the one we get above)

What alternative solutions did you explore? (Optional)

NA

Result

Screen.Recording.2023-11-15.at.15.25.10.mp4

[abekkala]

@burczu can you review the proposals?

[burczu]

@abekkala I'm sorry, I've missed that one - I'll review proposals soon.

[melvin-bot]

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

[burczu]

@erquhart Could you please be more specific how the ExceedCommentLength component should be changed? How do you want to use this new draftKey property in case of composing and editing?

[burczu]

@tienifr I've tried your solution but it does not work for me. Could you be more specific how do you pass reportActionID to the component and where?

[DylanDylann]

Proposal

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

• No error message when editing a chat to go past character limit

What is the root cause of that problem?

• ExceededCommentLength uses the comment prop to decide if it will display error or not (when comment.length > 10K, it will display error). This comment is from ONYXKEYS.COLLECTION.REPORT_DRAFT_COMMENT
• And ExceededCommentLength uses in ReportActionItemMessageEdit as well, so when the main composer has < 10K characters, edit composer has > 10K characters, the edit composer will have no error.

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

• Rather than getting the comment from ONYXKEYS.COLLECTION.REPORT_DRAFT_COMMENT, we can just pass the comment param to ExceededCommentLength from its parent component. This can fix this issue, also reducing the delay time in updating max length error (currently, when user types more than 10K, it will delay 1500ms before displaying error).
• We can do it by these below steps:

1. Remove subscribing ONYXKEYS.COLLECTION.REPORT_DRAFT_COMMENT
   

   App/src/components/ExceededCommentLength.js

   Lines 63 to 68 in 2c7062b

   	export default withOnyx({
   	comment: {
   	key: ({reportID}) => `${ONYXKEYS.COLLECTION.REPORT_DRAFT_COMMENT}${reportID}`,
   	initialValue: '',
   	},
   	})(ExceededCommentLength);

2. With ReportActionItemMessageEdit, we do not need to do anything because we already had the below logic:
   

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

   Line 468 in 2c7062b

   comment={draft}

3. With ReportActionComposer, updating
   

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

   Line 454 in 358c3bf

   <ExceededCommentLength

   
   to

<ExceededCommentLength comment={value} ... />                 

with value from:

App/src/pages/home/report/ReportActionCompose/ComposerWithSuggestions.js

Line 112 in 347df0c

const [value, setValue] = useState(() => {

What alternative solutions did you explore? (Optional)

• To reduce the number of re-render times of component ExceededCommentLength: Rather than passing the comment param to ExceededCommentLength from its parent component like the above solution, we can just pass the param called isExceedMaxLength to ExceededCommentLength from its parent component. So the ExceededCommentLength just re-renders when the isExceedMaxLength update its value from true to false and Vice versa

[erquhart]

@burczu I went ahead and added a before/after to my proposal to clarify.

[tienifr]

@burczu Just add the detail implementation and result in my proposal. Can you please help check again?

[burczu]

Thanks for your updates @erquhart and @tienifr, I'll get back to your proposals soon.

@DylanDylann So, your solution actually rolls back the changes introduced in this performance PR: #25758 - are you sure it actually improves the performance?

[DylanDylann]

@burczu

• Before, we need to display exactly the current input length, for example 12000/10000, so we need to get the comment from Onyx - that will be updated with 1000ms delay to increase the performance. If we do not get the comment from Onyx, the ExceededCommentLength will re-render once the input change, affect to the performace.

• But now, we just need to know whether the input is greater than the MAX_COMMENT_LENGTH or not, so my proposal suggested that, we just need to pass the isExceedMaxLength to ExceededCommentLength from its parent. So the ExceededCommentLength just re-render once the isExceedMaxLength update from "true" to "false" or vice versa.

• Besides fixing this bug, it also reduce the delay time when updating the error (Currently, updating error has 1500ms delay)

[melvin-bot]

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

[abekkala]

@laurenreidexpensify

STATUS:

I'm going ooo but this one is at the finish line and waiting payment on Dec 26.
Then, if regression test is needed a GH will need to be created for that.

• Issue reported by: @grgia (no payment 😉)
• Fix: @DylanDylann [$500, if no regressions] Offer link
• PR Review: @burczu [$500, if no regressions] please complete checklist.

[laurenreidexpensify]

Just a heads up I am OOO until 28 Dec, @DylanDylann @burczu so this will only be paid then, we have a skeleton team working next week so a few payments may be a little bit late

[melvin-bot]

@tgolen, @burczu, @abekkala, @laurenreidexpensify, @DylanDylann Whoops! This issue is 2 days overdue. Let's get this updated quick!

[laurenreidexpensify]

Payment Summary:

Fix: @DylanDylann $500 paid in Upwork
PR Review: @burczu N/A - expert contributor (callstack)

@burczu please complete checklist steps thanks

[melvin-bot]

@tgolen, @burczu, @abekkala, @laurenreidexpensify, @DylanDylann Whoops! This issue is 2 days overdue. Let's get this updated quick!

[melvin-bot]

@tgolen, @burczu, @abekkala, @laurenreidexpensify, @DylanDylann Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[laurenreidexpensify]

@burczu bump to complete checklist thanks

[abekkala]

@burczu can you please complete the checklist so this issue can be closed. thanks!

[melvin-bot]

@tgolen, @burczu, @abekkala, @laurenreidexpensify, @DylanDylann Huh... This is 4 days overdue. Who can take care of this?

[abekkala]

LAST TASK: waiting on @burczu to complete checklist, then this issue can be closed.

[burczu]

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:

• [@burczu] The PR that introduced the bug has been identified. Link to the PR:

#25758

• [@burczu] 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:

#25758 (comment)

• [@burczu] 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:

Not found any.

• [@burczu] Determine if we should create a regression test for this bug.

I think it may be worth checking if we have such test for creating the report message and update it with the check for editing report message as well.

• [@burczu] 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.

I'll post the regression test proposal in a separate message.

• [@abekkala] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[burczu]

Regression test proposal

1. Open the app.
2. Go to a report and enter > 10K characters in the message box.
3. Check if the error message appears.
4. Update the message to have less than 10k characters and post it.
5. Edit this message and again enter >10k characters.
6. Check if the error message appears.

Do we agree 👍 or 👎