-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
[Awaiting bugzero checklist and c+ newdot payment][$2000] Empty screen shows for a brief moment in change task assignee modal #23563
Comments
Triggered auto assignment to @kadiealexander ( |
Bug0 Triage Checklist (Main S/O)
|
ProposalPlease re-state the problem that we are trying to solve in this issue.Empty screen shows for a brief moment in change task assignee modal What is the root cause of that problem?This problem is due to a combination of two factors:
App/src/pages/tasks/TaskAssigneeSelectorModal.js Lines 99 to 105 in 8b23658
What changes do you think we should make in order to solve the problem?We should perform two changes:
The same problem seems to be present in Fixed result
chrome_LBiMPNYSaq.mp4What alternative solutions did you explore? (Optional)N/A |
Reproduced: RPReplay_Final1690358026.MP4 |
Job added to Upwork: https://www.upwork.com/jobs/~0163bd3cb559f30da0 |
Current assignee @kadiealexander is eligible for the External assigner, not assigning anyone new. |
Triggered auto assignment to Contributor-plus team member for initial proposal review - @sobitneupane ( |
ProposalPlease re-state the problem that we are trying to solve in this issue.Empty screen shows for a brief moment in change task assignee modal What is the root cause of that problem?When the transition ends we render the options but we have to wait some times (~200ms) to get data, that why we can see the empty page for a brief moment
App/src/pages/tasks/TaskAssigneeSelectorModal.js Lines 99 to 105 in 23a8a34
What changes do you think we should make in order to solve the problem?When we're getting data we should show the skeleton In order to do that I introduce the new variable
and here
Note There're other places (TaskShareDestinationSelectorModal, SearchPage, ...) face this problem so we should to apply the above logic too ResultScreen.Recording.2023-07-26.at.15.54.27.mov |
Proposal This is because state of skeleton is becoming false before the list contacts fetched from API completely. Its state should be false immediately when response arrives from API. Like this
About me: Portfolio: Feel free to reach me out. Best Regards, Contributor details |
📣 @meesum1214! 📣
|
Contributor details |
|
Thanks for your proposal @samh-nl.
If we do so,
I believe @samh-nl Whenever you update your proposal, please follow the contributing guideline and use the following format:
|
Thanks for the proposal @tienifr.
Can you please explain what problem @tienifr Can you also help me understand why the issue is reproducible here but not in Assignee selection in Create task flow? |
Concern 1: Concern 2: didScreenTransitionEnd It's original purpose was to block autofocus until the animation has finished, as autofocus causes a drop in FPS during animation. Perhaps delaying loading the data results in a smoother animation on mobile, I am not sure at this point.
Are we sure that it is not reproducible there? It uses the same component and I see the same behavior. |
In App/src/pages/tasks/TaskAssigneeSelectorModal.js Lines 91 to 96 in 975146f
We call several setState functions. React sends the state update operation (performed by the function returned by useState) to a queue to be processed. So the execution time of these setState functions may depend on the previous one in the queue (come from previous screen) or other aspects. I tried to calculate the total execution time of these setState functions in /new-task/assignee, and I realized that it's around 150ms at the first time, so we don't see the blank screen. But in /r/:reportID/assignee it's over 800ms that why we can see the empty screen shows for brief moment. I also tried to calculate the execution time in /new-task/assignee in many different search values and they were so different. Screen.Recording.2023-07-28.at.13.13.05.mp4That why we should has isLoading value to check when the function get finished. I'm so appreciate if someone can give me the more detail reason why this difference. Thanks a lot |
Thanks for the analysis. There is still some black area which we are unaware of on the Root Cause. Let's try to dig deeper. |
@sobitneupane, @kadiealexander Uh oh! This issue is overdue by 2 days. Don't forget to update your issues! |
Not overdue, will consider bumping the price tomorrow if no additional proposals come through. |
📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸 |
@tienifr @sobitneupane @Beamanator @kadiealexander Thank you for everyone considering me as well. is there I am also eligible to get this bonus amount with Contributor? because I also have the solution #23563 (comment) for this RCA. |
@kadiealexander @Beamanator any updates |
@kadiealexander This was deployed to production 2 weeks ago and is ready for payment. |
Regression Test Proposal
Do we agree 👍 or 👎 |
Requested payment on newDot. |
@kadiealexander Can you please provide a payment summary? |
This comment was marked as outdated.
This comment was marked as outdated.
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:
|
@kadiealexander could you able to check this
#23563 (comment) I have the correct RCA and solution but I was not selected but @tienifr has the same solution only the earlier one so he selected it. |
@pradeepmdk we don't have a precedence for this, let me ask internally. |
@kadiealexander Thank you. I need some clarification on selecting the proposal. where i am missing in selecting my proposal here? and due to #23563 (comment) being unclear of ROOT case only we are getting more proposals here |
I actually didn't pick the proposals, @sobitneupane could you please take a look at @pradeepmdk's question above? |
#23563 (comment) I agree that @tienifr posted the solution first. I posted after #23563 (comment) and increased the bounty. but now I only have both RCA and solution first. 😢 |
As we mentioned above #23563 (comment) and #23563 (comment). I also believe we should give the bounty for @pradeepmdk because he found the correct RCA cc @kadiealexander @sobitneupane |
Yup. We have agreed on awarding bounty to @pradeepmdk . I am not sure what is the right amount but I vaguely remember paying 25% in such cases. cc: @kadiealexander |
Let me know what I am missing in selecting my proposal here still confusing here #23563 (comment) As per this Root cause is unclear so we are getting more proposals and increasing the bountry. As per this, I should be treated equally(including bouns) right? |
any update |
We are discussing this internally @pradeepmdk, thanks for your patience. |
@kadiealexander thanks for the update. |
@pradeepmdk our team has reviewed the situation and we feel it's fair to award you 25% of the contributor bounty for your assistance in determining the root cause of this bug. As you were not materially involved in proposing or implementing the chosen solution, we do not agree that the full 100% would be fair here. Thanks again for your patience while we discussed this. Please apply here and we will get this paid out to you. |
@sobitneupane please don't forget about the first three points in the checklist here. Thanks! |
Resurfacing this and updating it since it got buried. Payouts due:
Eligible for 50% #urgency bonus? Yes Upwork job is here. |
@kadiealexander Thank you
I proposed the solution here #23563 (comment) for chosen one. |
@pradeepmdk thank you for your additional information, however our decision here is final. I have sent you a contract in Upwork for the payment, please accept and I will ensure you are paid quickly. 😊 |
@kadiealexander I agree your decision is final. but it is still weird here because for no reason my RCA and solution were ignored. but no one has given me a solution as to why the proposal was not selected here. |
For clarity, @sobitneupane explained here why we preferred @tienifr's solution, even though your RCA was valuable here. I appreciate this was a competitive issue with lots of similar proposals, but ultimately our team reserves the right to choose the solution we believe is best. |
$3,000 payment for @sobitneupane approved based on BZ summary. |
If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!
Action Performed:
Expected Result:
Contacts list shows immediately after skeleton
Actual Result:
Empty list shows for a brief moment before contacts list shows
Note: this doesn't happen in new task flow
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?
Version Number: 1.3.45-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
bug2.mov
YFCI4302.1.MP4
Expensify/Expensify Issue URL:
Issue reported by: @situchan
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1690261404929219
View all open jobs on GitHub
Upwork Automation - Do Not Edit
The text was updated successfully, but these errors were encountered: