Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[$500] Web - Error doesn't disappear for a while after deleting entire max exceed message. #27698

Closed
1 of 6 tasks
kbecciv opened this issue Sep 18, 2023 · 68 comments
Closed
1 of 6 tasks
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Engineering Internal Requires API changes or must be handled by Expensify staff

Comments

@kbecciv
Copy link

kbecciv commented Sep 18, 2023

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!


Action Performed:

  1. Go to any report.
  2. Paste a message that has over 15,000 characters. Notice now the Composer has red border with the error below.
  3. Delete entire message.
  4. Notice that Composer still has error for a while after deleting message.

Expected Result:

Error should disappear right after user deleted the message.

Actual Result:

Composer still has error for a while after deleting message.

Workaround:

Unknown

Platforms:

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

  • Android / native
  • Android / Chrome
  • iOS / native
  • iOS / Safari
  • MacOS / Chrome / Safari
  • MacOS / Desktop

Version Number: 1.3.71.4
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
Notes/Photos/Videos: Any additional supporting documentation

Screen.Recording.2023-09-17.at.14.50.21.mov
Recording.4576.mp4

Expensify/Expensify Issue URL:
Issue reported by: @hungvu193
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1694937014673629

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01bef70e1146af3c78
  • Upwork Job ID: 1713723076177219584
  • Last Price Increase: 2023-10-16
@kbecciv kbecciv added External Added to denote the issue can be worked on by a contributor Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Sep 18, 2023
@melvin-bot melvin-bot bot changed the title Web - Error doesn't disappear for a while after deleting entire max exceed message. [$500] Web - Error doesn't disappear for a while after deleting entire max exceed message. Sep 18, 2023
@melvin-bot
Copy link

melvin-bot bot commented Sep 18, 2023

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

@melvin-bot
Copy link

melvin-bot bot commented Sep 18, 2023

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

@melvin-bot
Copy link

melvin-bot bot commented Sep 18, 2023

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 melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Sep 18, 2023
@melvin-bot
Copy link

melvin-bot bot commented Sep 18, 2023

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

@melvin-bot
Copy link

melvin-bot bot commented Sep 18, 2023

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

@DylanDylann
Copy link
Contributor

DylanDylann commented Sep 18, 2023

Proposal

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

  • Error doesn't disappear for a while after deleting entire max exceed message.

What is the root cause of that problem?

  • Currently, the border error is display based on hasExceededMaxCommentLength
  • hasExceededMaxCommentLength is set by debounce function updateCommentLength(comment,onExceededMaxCommentLength) - delay 500ms and the comment params is get from the ONYXKEYS.COLLECTION.REPORT_DRAFT_COMMENT that is also updated by the debounce function (1000ms)
    const updateCommentLength = useMemo(
    () =>
    debounce((comment, onExceededMaxCommentLength) => {
    const newCommentLength = ReportUtils.getCommentLength(comment);
    setCommentLength(newCommentLength);
    onExceededMaxCommentLength(newCommentLength > CONST.MAX_COMMENT_LENGTH);
    }, CONST.TIMING.COMMENT_LENGTH_DEBOUNCE_TIME),
    [],
    );
    useEffect(() => {
    updateCommentLength(props.comment, props.onExceededMaxCommentLength);
    }, [props.comment, props.onExceededMaxCommentLength, updateCommentLength]);
  • It leads to after clearing all the input, it will delay for a while after removing the border error

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

  • These are 2 methods to reduce the delay time when update the border error:
  1. (Reduce 1000ms delay) In here
    debounce((comment, onExceededMaxCommentLength) => {
    we can get the comment param from the value state
    const [value, setValue] = useState(() => getDraftComment(reportID) || '');
    rather than the ONYXKEYS.COLLECTION.REPORT_DRAFT_COMMENT
    comment: {
    key: ({reportID}) => `${ONYXKEYS.COLLECTION.REPORT_DRAFT_COMMENT}${reportID}`,
    because the ONYXKEYS.COLLECTION.REPORT_DRAFT_COMMENT will be updated by the debounce function.
  2. (Reduce 500ms): Call the updateCommentLength(comment,onExceededMaxCommentLength) without delay.
    const updateCommentLength = useMemo(
    () =>
    debounce((comment, onExceededMaxCommentLength) => {
    const newCommentLength = ReportUtils.getCommentLength(comment);
    setCommentLength(newCommentLength);
    onExceededMaxCommentLength(newCommentLength > CONST.MAX_COMMENT_LENGTH);
    }, CONST.TIMING.COMMENT_LENGTH_DEBOUNCE_TIME),
    [],
    );

    Also we can remove the commentLenght state here because will don't need to display exact comment length (like "12370/10000"), just need to know whether it is greater than 10.000 or not
    const [commentLength, setCommentLength] = useState(0);
    . So the updateCommentLength function just call onExceededMaxCommentLength(newCommentLength > CONST.MAX_COMMENT_LENGTH), not affect the performance
  • So combining these two method will reduce 1500ms delay time when update the border error

What alternative solutions did you explore? (Optional)

  • NA

Result

Screencast.from.19-09-2023.00.20.01.webm

@laurenJ7892
Copy link

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

Error doesn't disappear for a while after deleting entire comment when it has exceeded the maximum message

What is the root cause of that problem?

The debouncer applies the delay when the comment length is being changed. When it goes to a length of 0, it is still waiting COMMENT_LENGTH_DEBOUNCE_TIME to show that.

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

Conditionally apply the debouncer if comment value is not null. It still keeps existing functionality debouncer intact when the user is typing but it will allow on a sudden delete of everything to not apply it.

const updateCommentLength = useMemo(() => {
const updateLength = (comment) => {
const newCommentLength = ReportUtils.getCommentLength(comment);
setCommentLength(newCommentLength);
onExceededMaxCommentLength(newCommentLength > CONST.MAX_COMMENT_LENGTH);
};

if (comment == '') {
    // If the comment is empty, update immediately without delay
    updateLength(comment);
    return null; 
} else {
    // If the comment is not empty, apply debounce
    return debounce(() => updateLength(comment), CONST.TIMING.COMMENT_LENGTH_DEBOUNCE_TIME);
}

}, [comment, onExceededMaxCommentLength]);

What alternative solutions did you explore? (Optional)

  1. Remove the debounce entirely and adopt useCallback to update comment length directly:

const updateCommentLength = useCallback((comment, onExceededMaxCommentLength) => {
const newCommentLength = ReportUtils.getCommentLength(comment);
setCommentLength(newCommentLength);
onExceededMaxCommentLength(newCommentLength > CONST.MAX_COMMENT_LENGTH);
}, [ ]);

  1. Examine other areas in the code base and see if it is suitable to reduce the debounce constant.

Contributor details
Your Expensify account email: [email protected]
Upwork Profile Link: https://www.upwork.com/freelancers/~010c554754482f6227

@melvin-bot
Copy link

melvin-bot bot commented Sep 19, 2023

✅ Contributor details stored successfully. Thank you for contributing to Expensify!

@kevin-williams
Copy link

kevin-williams commented Sep 20, 2023

Proposal

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

Error doesn't disappear for a while after deleting entire comment when it has exceeded the maximum message

What is the root cause of that problem?

Two problems add up to a 1.5 sec delay:

  1. The ExceedCommentLength checker is getting the debounced value after the report draft is saved, so there is a 1 second delay.
  2. The debounce should only happen on setting the error, but you have the 500ms delay on the debounce of the value going back to good again.

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

Three changes:

  1. update ComposerWithSuggestions.js to add an onChangeText prop which gets fired as the comments get updated.

and then in the updateComment function, add a call to the onChangeText prop (if passed). Also add onChangeText to prop types and document.

setValue(newComment);
onChangeText && onChangeText(newComment)
  1. ReportActionCompose - add
    const [comment, setComment] = useState('');

Then change the comment param to just reference the comment variable above.

<ExceededCommentLength
           comment={comment}
          onExceededMaxCommentLength={setExceededMaxCommentLength}
/>

And to ComposerWithSuggestions add the property

onChangeText={setComment}
  1. ExceededCommentLength - update the use effect to short circuit the debounce when the error is fixed, but keep the debounce when checking for an error.
 useEffect(() => {
        const propsCommentLength = ReportUtils.getCommentLength(props.comment);
        // Short circuit if the error is removed
        if (propsCommentLength <= CONST.MAX_COMMENT_LENGTH) {
            setCommentLength(propsCommentLength);
            props.onExceededMaxCommentLength(false);
        } else {
            // Debounce the change when checking for an error
            updateCommentLength(props.comment, props.onExceededMaxCommentLength);
        }
    }, [props.comment, props.onExceededMaxCommentLength, updateCommentLength]);

This code is currently working in my local repo and the red border and line count exceeded message are removed immediately.

It also works when editing a comment, not just composing a new one.

@melvin-bot melvin-bot bot added the Overdue label Sep 20, 2023
@jliexpensify
Copy link
Contributor

Bumping @sobitneupane for reviews!

@melvin-bot melvin-bot bot added Overdue and removed Overdue labels Sep 21, 2023
@sobitneupane
Copy link
Contributor

Thanks for your proposal @DylanDylann

We can get the comment param from the value state inside the component rather than the ONYXKEYS.COLLECTION.REPORT_DRAFT_COMMENT

Where are you suggesting to make the change? Will it affect if we have existing draft comment?

also we can call the updateCommentLength(comment,onExceededMaxCommentLength) without delay

I don't think we can do that. This change was made to improve performance and was intended.

@melvin-bot melvin-bot bot removed the Overdue label Sep 25, 2023
@sobitneupane
Copy link
Contributor

If we fail to identify a straightforward and direct solution, I believe it may be appropriate to consider closing the issue

@DylanDylann
Copy link
Contributor

DylanDylann commented Sep 25, 2023

@sobitneupane

Where are you suggesting to make the change? Will it affect if we have existing draft comment?

  • Currently, we get the comment param from the draft and the draft value is updated with the delay 1000ms:
    export default withOnyx({
    comment: {
    key: ({reportID}) => `${ONYXKEYS.COLLECTION.REPORT_DRAFT_COMMENT}${reportID}`,
    },
    })(ExceededCommentLength);
  • So we can get the comment param from the value state inside the component directly. With this change, the delay time is decreased 1000ms

@DylanDylann
Copy link
Contributor

DylanDylann commented Sep 25, 2023

@sobitneupane

I don't think we can do that. This change was made to improve performance and was intended.

  • Now with the change here fix: ensure processing of markup in comment #24319, we do not need the commentLength state, just need to know the input `s length is > 10.000 or not, so we can remove [commentLength, setCommentLength]
  • The logic
    function ExceededCommentLength(props) {
    const [commentLength, setCommentLength] = useState(0);
    const updateCommentLength = useMemo(
    () =>
    debounce((comment, onExceededMaxCommentLength) => {
    const newCommentLength = ReportUtils.getCommentLength(comment);
    setCommentLength(newCommentLength);
    onExceededMaxCommentLength(newCommentLength > CONST.MAX_COMMENT_LENGTH);
    }, CONST.TIMING.COMMENT_LENGTH_DEBOUNCE_TIME),
    [],
    );
    useEffect(() => {
    updateCommentLength(props.comment, props.onExceededMaxCommentLength);
    }, [props.comment, props.onExceededMaxCommentLength, updateCommentLength]);
    if (commentLength <= CONST.MAX_COMMENT_LENGTH) {
    return null;
    }
    can become:
    const updateCommentLength = useMemo(
        () =>
            debounce((comment, onExceededMaxCommentLength) => {
                const newCommentLength = ReportUtils.getCommentLength(comment);
          
                onExceededMaxCommentLength(newCommentLength > CONST.MAX_COMMENT_LENGTH);
            }, CONST.TIMING.COMMENT_LENGTH_DEBOUNCE_TIME),
        [],
    );

    useEffect(() => {
        updateCommentLength(props.comment, props.onExceededMaxCommentLength);
    }, [props.comment, props.onExceededMaxCommentLength, updateCommentLength]);

    if (!props.hasExceededMaxCommentLength) {
        return null;
    }
  • In the above change, the hasExceededMaxCommentLength state only changes when the input`s length exceeds 10.000 so it does not affect the performance. With this change, the delay time is decreased 500ms

@kevin-williams
Copy link

@sobitneupane

My solution does not change any of the delays on saving the report in onyx or in setting the error, but it does remove the error immediately by short circuiting the debounce check and directly checking the changed form value rather than the debounced saved report value.

@melvin-bot melvin-bot bot added the Overdue label Sep 27, 2023
@jliexpensify
Copy link
Contributor

Bump @sobitneupane to address comments

@jliexpensify
Copy link
Contributor

Bumping @Ollyws !

@melvin-bot melvin-bot bot added Overdue and removed Overdue labels Oct 22, 2023
@Ollyws
Copy link
Contributor

Ollyws commented Oct 25, 2023

Still considering this one...

@melvin-bot melvin-bot bot added Overdue and removed Overdue labels Oct 25, 2023
@Ollyws
Copy link
Contributor

Ollyws commented Oct 30, 2023

From what I can see it doesn't seem like there is any easy way we can remove the debounce.
Given the low impact of this issue I think closing it was the correct decision.

@melvin-bot melvin-bot bot removed the Overdue label Oct 30, 2023
@DylanDylann
Copy link
Contributor

DylanDylann commented Oct 30, 2023

@Ollyws my proposal has two options to remove the debounce: One option can reduce 1000ms and another is 500ms. Combining both can reduce all the debounce. I am very confident that we can go with the 1st option, and just need to consider whether we use 2nd option as well

@DylanDylann
Copy link
Contributor

@Ollyws you can take a look at my proposal and feel free to ask more question about it. Thanks

@Ollyws
Copy link
Contributor

Ollyws commented Oct 30, 2023

As far as I can see the only part of your proposal that could be implemented would be method 1, but there were these unresolved comments about that:

@DylanDylann Thanks for the update. We need to implement the solution not only in ReportActionCompose but also in ReportActionItemMessageEdit which doesn't make use of ComposerWithSuggestions.

I agree with

I don't think we can go with the 2nd option as I have already mentioned that it will deteriorate the performance of Composer.

The debounce was added in #15501 for performance reasons and I don't think it's worth decreasing the performance of the composer, the most used component in the app in order to increase the speed of this very edge case scenario.

@DylanDylann
Copy link
Contributor

DylanDylann commented Oct 30, 2023

@Ollyws

As far as I can see the only part of your proposal that could be implemented would be method 1, but there were these unresolved comments about that:
@DylanDylann Thanks for the update. We need to implement the solution not only in ReportActionCompose but also in ReportActionItemMessageEdit which doesn't make use of ComposerWithSuggestions.

I just checked the current edit message`s logic, the behavior is slightly different from the message composer logic: The message composer error is updated with ~1500 ms delay, but the message edit is only ~ 500 ms.

As the RCA mentioned in my proposal, there are two factors led to the delay (with the message composer):
hasExceededMaxCommentLength is set by debounce function updateCommentLength(comment,onExceededMaxCommentLength) - delay 500ms and the comment params is get from the ONYXKEYS.COLLECTION.REPORT_DRAFT_COMMENT that is also updated by the debounce function (1000ms)

And the message edit only encounter the case hasExceededMaxCommentLength is set by debounce function updateCommentLength(comment,onExceededMaxCommentLength) - delay 500ms

@Ollyws
Copy link
Contributor

Ollyws commented Oct 30, 2023

Ah sorry I missed that response.
As far as I can see on staging the exceeded comment length message isn't working atall for the edit composer.
It only appears if you exceed the comment in the main composer, I would guess because we're getting the value from the draft.

@DylanDylann
Copy link
Contributor

DylanDylann commented Oct 30, 2023

@Ollyws yeah. You are correct. So in suggestion is:

  1. Go with my first option to reduce 1000ms.
  2. Fix the issue that you `ve mentioned:

I would guess because we're getting the value from the draft.

RCA:

  • In here:
    comment: {
    key: ({reportID}) => `${ONYXKEYS.COLLECTION.REPORT_DRAFT_COMMENT}${reportID}`,
    },

    we get the comment from draft, but use in both edit composer and main composer, that leads to the "max exceed length" does not work when editing comment, and if we are editing comment and switch to type a > 10.000 characters message in main composer, the max exceed length exist in both main composer and edit composer.

Solution:

  • Add an param called isEditing to ExceededCommentLength component, and get the proper message based on it

@melvin-bot melvin-bot bot added the Overdue label Nov 1, 2023
@Ollyws
Copy link
Contributor

Ollyws commented Nov 1, 2023

@DylanDylann How do you plan to get the value variable to the ExceededCommentLength component, given it's in ReportActionCompose and not ComposerWithSuggestions?

@melvin-bot melvin-bot bot removed the Overdue label Nov 1, 2023
@DylanDylann
Copy link
Contributor

DylanDylann commented Nov 1, 2023

@Ollyws yeah, it is my plan

@Ollyws
Copy link
Contributor

Ollyws commented Nov 1, 2023

But how exactly?

@DylanDylann
Copy link
Contributor

DylanDylann commented Nov 1, 2023

Yeah. I still go with this solution because it is very straightforward

@Ollyws
Copy link
Contributor

Ollyws commented Nov 1, 2023

@DylanDylann I'm asking how will you get the value variable from ComposerWithSuggestions to ReportActionCompose?
You're planning to move the whole thing into ReportActionCompose and pass it to ComposerWithSuggestions?

@DylanDylann
Copy link
Contributor

@Ollyws Sorry for misunderstood your question

@melvin-bot melvin-bot bot added the Overdue label Nov 6, 2023
@DylanDylann
Copy link
Contributor

@Ollyws Please help review my solution here when you have a chance #27698 (comment)

@Ollyws
Copy link
Contributor

Ollyws commented Nov 6, 2023

I think given that there is another issue #30921 open for the edit message character limit not working, and that while some proposals can mitigate this slightly it's essentially impossible to fix completely without removing the debounce, so along with my previous reasoning I vote to close this.

@melvin-bot melvin-bot bot removed the Overdue label Nov 6, 2023
@DylanDylann
Copy link
Contributor

@jliexpensify What do you think about this one #27698 (comment)?

@jliexpensify
Copy link
Contributor

@DylanDylann in all honesty, I'm going to defer to @Ollyws here - my main role is to help troubleshoot and manage the issue, and as the C+, Olly should be finding suitable proposals. Given that he's already mentioned that it's an edge case and this issue should be closed a few times already, I'm inclined to lean towards this as well - also this statement indicates we won't really be solving the issue?

it's essentially impossible to fix completely without removing the debounce

If Olly did think it was an actual issue, my understanding is that he would have chosen a proposal and asked for an internal engineering review. Going to close this one out!

@DylanDylann
Copy link
Contributor

Many thanks @jliexpensify and @Ollyws

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Engineering Internal Requires API changes or must be handled by Expensify staff
Projects
None yet
Development

No branches or pull requests

10 participants