[HOLD for payment 2023-06-29] [$1000] Console error when selecting a user to assign a task to

[kavimuru]

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. Open a chat
2. Go to "Assign Task" section
3. Enter the task title and click on the "Next" button
4. Then click on "Confirm task"
5. Click on "To" and select a user

Expected Result:

A console error should not appear

Actual Result:

Console error when selecting a user to assign a task to

Workaround:

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

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.19-2
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

2023-05-25.00-49-45.mp4

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

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01ca7b76593b61b50d
• Upwork Job ID: 1663406805379948544
• Last Price Increase: 2023-06-13

[melvin-bot]

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

[situchan]

Proposal

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

Console error shows after selecting assignee on task assignee selector page

What is the root cause of that problem?

This came from lack knowledge of lifecycle in functional component.
In TaskAssigneeSelectorModal, there's search_render measurement.
Timer should start on component mount and should end only on OptionsSelector onLayout callback or component unmount.
In current logic, there are more cases of start/end timer: whenever props or search value are changed because of [updateOptions] dependency.

App/src/pages/tasks/TaskAssigneeSelectorModal.js

Lines 110 to 114 in d70b746

	return () => {
	Timing.end(CONST.TIMING.SEARCH_RENDER);
	Performance.markEnd(CONST.TIMING.SEARCH_RENDER);
	};
	}, [updateOptions]);

So finally these are call orders in Timing.js:

• end
• start
• end
• start
• getEnvironment promise return
• getEnvironment promise return (error happens here because key is removed by this line)
• start
• end
• getEnvironment promise return

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

As this measurement is unintentionally (confirmed here) added in this page, we can safely remove this effect and onLayout callback.

Also remove same in TaskShareDestinationSelectorModal.

What alternative solutions did you explore? (Optional)

Early return if key doesn't exist here

[bfitzexpensify]

Reproduced

[melvin-bot]

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

[melvin-bot]

Current assignee @bfitzexpensify is eligible for the External assigner, not assigning anyone new.

[melvin-bot]

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

[melvin-bot]

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

[bernhardoj]

Proposal

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

Console error when we select an assignee.

What is the root cause of that problem?

The error comes from Timing.end where we are trying to access timestampData[eventName].

App/src/libs/actions/Timing.js

Lines 31 to 36 in 3ce8e82

	if (!timestampData[eventName]) {
	return;
	}
	Environment.getEnvironment().then((envName) => {
	const {startTime, shouldUseFirebase} = timestampData[eventName];

Notice that we already have a guard to check if timestampData[eventName] exist or not. However, we access it inside the promise, which means something must have deleted it before the promise resolve. We can easily move the check inside the promise, but that doesn't really solve the root cause, but rather just "hiding" the issue. The only way timestampData[eventName] is deleted through Timing.end, so we can guess that there are one calls to Timing.end that already delete the data while the other call's promise just completed.

We have multiple calls of Timing because we call it inside the useEffect cleanup function which will be called every time the dependency array changes or the component unmount.
https://github.com/Expensify/App/blob/3ce8e8212c8c9dcc4a7767a1c256555624ad6202/src/pages/tasks/TaskAssigneeSelectorModal.js#L104C22-L114

Notice the dependency array is updateOptions which is a function wrapped with useCallback that have a dependency array of props and searchValue. This means, every props changes, we will call Timing.end.

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

As the timing measure is to measure how long the search options to render, I don't think it makes sense to call Timing.end in a useEffect cleanup function. We want to measure how long the search to render, not how long the component unmounts or how long any props change. So,

1. Remove Timing and Performance start/end from useEffect cleanup function.
2. Call Timing and Performance start at top of the function. Use ref to call it once.

const isMeasuringStarted = useRef(false);
if (!isMeasuringStarted.current) 
    isMeasuringStarted.current = true
    call timing and performance start

The above changes are enough to solve this issue, but I see another thing that we can improve, read more below:

read more

After applying my changes above, you will notice that we call `updateOptions` in `useEffect` and `onChangeText`. This means, we are calling `updateOptions` twice every time we type something. Also, calling `updateOptions` in `onChangeText` is wrong because we need to wait for the state changes before calling `updateOptions` (like a `setState` callback). What we need to do instead is replace `updateOptions` to a plain `useEffect` that will run every time the props.reports, personalDetails, betas, or searchValue are changed.

This will improve the code and prevent unnecessary search logic on every props changes.

Let me know if we agree to do this.

As TaskAssigneeSelectorModal and TaskShareDestinationSelectorModal has a same Timing (and search) logic, we can apply the suggestion on both pages.

[mallenexpensify]

Since this already is External and has proposals, I put the below on hold pending this issue since it appears to be a dupe

• Assignee- Console error displayed on adding a assignee #19462

[bfitzexpensify]

@eVoloshchak couple of proposals ready for your review.

[melvin-bot]

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

[bfitzexpensify]

Hey @greg-schroeder - I'm ooo this week so assigning a BZ buddy. Current status: we have a couple of proposals for @eVoloshchak to review. Don't expect you'll need to do much here before I get back next week but appreciate you keeping an eye on it.

[situchan]

@arosiclair @eVoloshchak I suggest to hold this issue a bit as we cannot test assign task flow without fixing #20807. So I cannot add screenshots.
The PR to fix that issue is being actively reviewed. I keep watching that and will post here after merged.
Btw, here's draft PR for code review: #20840

[youssef-lr]

@situchan here's the fix in this line, you can add it locally in order to test the flow but without committing it, just so you guys don't get blocked working on this while waiting for this to be merged.

[situchan]

Thanks. That PR is merged but now not able to login (https://expensify.slack.com/archives/C01GTK53T8Q/p1686889937230319) so it's another blocker for testing.

[situchan]

@eVoloshchak PR is ready

[situchan]

@eVoloshchak kindly bump for review

[melvin-bot]

Based on my calculations, the pull request did not get merged within 3 working days of assignment. Please, check out my computations here:

• when @situchan got assigned: 2023-06-14 15:20:14 Z
• when the PR got merged: 2023-06-19 19:55:58 UTC
• days elapsed: 4

On to the next one 🚀

[bfitzexpensify]

Just noting here that I'm going to consider this eligible for the 3-day merging bonus, since it missed by just a few hours and was a little hamstrung with testing because of the log-in issues.

[melvin-bot]

Reviewing label has been removed, please complete the "BugZero Checklist".

[melvin-bot]

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.3.30-5 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• remove wrong performance measurement in task assignee selector, share destination pages #20840

If no regressions arise, payment will be issued on 2023-06-29. 🎊

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

• External issue reporter
• Contributor that fixed the issue
• Contributor+ that helped on the issue and/or PR

As a reminder, here are the bonuses/penalties that should be applied for any External issue:

• Merged PR within 3 business days of assignment - 50% bonus
• Merged PR more than 9 business days after assignment - 50% penalty

[melvin-bot]

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:

• [@eVoloshchak] The PR that introduced the bug has been identified. Link to the PR:
• [@eVoloshchak] 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:
• [@eVoloshchak] 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:
• [@eVoloshchak] Determine if we should create a regression test for this bug.
• [@eVoloshchak] 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.
• [@bfitzexpensify] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[bfitzexpensify]

Offers sent out @ayazhussain79 @situchan @eVoloshchak

[eVoloshchak]

• The PR that introduced the bug has been identified. Link to the PR: Create Task FrontEnd Changes #17992

• 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: [HOLD for payment 2023-06-29] [$1000] Console error when selecting a user to assign a task to #19772 (comment)

• 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: We already have a checklist item for precisely this

• Determine if we should create a regression test for this bug. This is a warning, doesn't affect the user experience, I think regression test isn't needed here

[eVoloshchak]

@bfitzexpensify, could you hold my payment till tomorrow, please?
I'll try to request the payment using NewDot instead of Upwork, need some additional setting up to do

[bfitzexpensify]

Sure!

[melvin-bot]

@arosiclair, @eVoloshchak, @bfitzexpensify, @situchan Whoops! This issue is 2 days overdue. Let's get this updated quick!

[eVoloshchak]

Not overdue, waiting for the first payment via NewDot to clear, after that will request a payment for this

[eVoloshchak]

This has been paid out🎉

[bfitzexpensify]

Awesome! Let's close this out.