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

Payment Sep 25 [$1000] Workspaces - Visual flickering when navigating back to workspace list from a WS #24341

Closed
1 of 6 tasks
lanitochka17 opened this issue Aug 9, 2023 · 78 comments
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Engineering External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review Weekly KSv2

Comments

@lanitochka17
Copy link

lanitochka17 commented Aug 9, 2023

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:

  1. Open the staging web link
  2. Go to Settings > Workspaces
  3. Select a workspace to open WS settings
  4. Go back to workspace list by tapping on the back button

Expected Result:

No visual issues / screen should slide smoothly

Actual Result:

Visual flickering on the button "New workspace" when going back to workspace list

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.52.1

Reproducible in staging?: Yes

Reproducible in production?: Yes

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

Bug6159271_RPReplay_Final1691599332__1_.mp4

Expensify/Expensify Issue URL:

Issue reported by: Applause - Internal Team

Slack conversation:

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01cbc57cc70a4d58dc
  • Upwork Job ID: 1689518567778115584
  • Last Price Increase: 2023-08-24
@lanitochka17 lanitochka17 added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Aug 9, 2023
@melvin-bot
Copy link

melvin-bot bot commented Aug 9, 2023

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

@melvin-bot
Copy link

melvin-bot bot commented Aug 9, 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

@sophiepintoraetz
Copy link
Contributor

I'm able to reproduce this and I think this is a design polish (but necessary)

RPReplay_Final1691647496.MP4

@sophiepintoraetz sophiepintoraetz added the External Added to denote the issue can be worked on by a contributor label Aug 10, 2023
@melvin-bot melvin-bot bot changed the title Workspaces - Visual flickering when navigating back to workspace list from a WS [$1000] Workspaces - Visual flickering when navigating back to workspace list from a WS Aug 10, 2023
@melvin-bot
Copy link

melvin-bot bot commented Aug 10, 2023

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

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Aug 10, 2023
@melvin-bot
Copy link

melvin-bot bot commented Aug 10, 2023

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

@melvin-bot
Copy link

melvin-bot bot commented Aug 10, 2023

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

@swakeert
Copy link

Proposal

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

On ios/ Safari, when navigating back to the Workspace list from a particular workspaces' setting, there is a visual flickering and occasional movement of "New Workspace" text.

What is the root cause of that problem?

On moving between the Workspace list and Workspace Setting page, there is a change in the safe area background. When the Workspace setting page is unmounted, it is causing a slightly delayed re-render of the footer and safe area which is causing the 'flickering' effect.

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

We need to inspect the state and find the root cause of the re-render. And ensure it does not perform the re-render when coming back from the workspace setting page.

What alternative solutions did you explore? (Optional)

I checked the transition effects on the footer and headerPageLayout and its child components to see what could have caused the re-render, but did not find anything.

@Thanos30
Copy link
Contributor

I personally can't replicate it in my local environment, so that makes it hard to test. I found this which might be useful:

react-navigation/react-navigation#9530

@melvin-bot melvin-bot bot added the Overdue label Aug 14, 2023
@sophiepintoraetz
Copy link
Contributor

@allroundexperts - any thoughts on the proposal here?

@melvin-bot melvin-bot bot removed the Overdue label Aug 14, 2023
@allroundexperts
Copy link
Contributor

@swakeert Thanks for your proposal. Your RCA is making sense but your solution seems to be too abstract. Can you propose a solution that I can implement and test?

@allroundexperts
Copy link
Contributor

Still looking for more proposals 👀

@Thanos30
Copy link
Contributor

@allroundexperts any idea why this isn't reproducable on my localhost?

@swakeert
Copy link

I'll need to spend some time with the code and fiddle around. Once I have the answer, it would probably be a few lines of code anyway. Not sure what more I can suggest without actually solving the problem.

@allroundexperts
Copy link
Contributor

@allroundexperts any idea why this isn't reproducable on my localhost?

I'm not sure really. Does it happen on staging for you?

@allroundexperts
Copy link
Contributor

I'll need to spend some time with the code and fiddle around. Once I have the answer, it would probably be a few lines of code anyway. Not sure what more I can suggest without actually solving the problem.

I think with an issue like this, its actually fine to be more concrete with your proposed solution.

@Thanos30
Copy link
Contributor

Thanos30 commented Aug 14, 2023

It happens on both live websites, it doesn't happen when running locally.

@swakeert
Copy link

@allroundexperts in that case I'll try to fix it locally and raise a PR or send a code diff. But since I'm not officially assigned this task yet, I'll have to keep it lower on my personal priority. :)

@allroundexperts
Copy link
Contributor

@allroundexperts in that case I'll try to fix it locally and raise a PR or send a code diff. But since I'm not officially assigned this task yet, I'll have to keep it lower on my personal priority. :)

There isn't any need to raise a PR before being assigned. It would be fine if you just update your proposal pointing out the exact change that we need to make in order to fix this.

@Thanos30
Copy link
Contributor

@swakeert were you able to reproduce the issue locally on your environment by any chance? Without that being possible I couldn't provide a solution.

@allroundexperts
Copy link
Contributor

It happens on both live websites, it doesn't happen when running locally.

I would assume that something is off with your setup. I'm able to reproduce this on my simulator.

@Thanos30
Copy link
Contributor

@allroundexperts Got it, thanks, I will re-check

@sophiepintoraetz
Copy link
Contributor

@studentofcoding - I don't have to accept it until payment is due - don't worry!

@studentofcoding
Copy link
Contributor

Leave this here for a context of merging within 3 days (or on 1 Sept) but it got delayed as Merge Freeze

#26172 (comment)
Screenshot 2023-09-12 at 22 07 16

@studentofcoding
Copy link
Contributor

Hey guys, just checked the Upwork page, and the job is already closed, even though I haven't yet received the Offer.

Screenshot 2023-09-13 at 16 13 28

Can anybody share what happened? @dangrous @sophiepintoraetz

Thanks

@sophiepintoraetz
Copy link
Contributor

sophiepintoraetz commented Sep 13, 2023

That happens when the job posting is too old but I can still accept proposals (edit) I'll need to look into this.

@studentofcoding
Copy link
Contributor

That happens when the job posting is too old but I can still accept proposals (edit) I'll need to look into this.

Noted, thanks for speedy follow-up @sophiepintoraetz

@studentofcoding
Copy link
Contributor

Follow up

Hey guys, just following up on this issue, after the last deployment on Staging (v1.3.70-2) Smart App Banner is finally showing on WS details!

Staging (v1.3.70-2)

staging.mp4

But a weird & interesting quirk is happening on Production (v1.3.69-2) as the Smart App Banner didn't show

Production (v1.3.69-2

production.mp4

This behavior is really interesting, as we have the same structure for /workspace/* URL on apple-app-site-association & the only differences are on the updated /teachersunite/*that being updated by Internal PR here

Staging Production
staging production

My hunch is this behavior is happening because the Production still didn't have the last updated change from Staging, which is the change from internal PR below (that basically change /save-the-world/* into /teachersunite/* that make the Smart App Banner didn't behave as expected

What do you think of this guy's @dangrous @allroundexperts? Just want to follow this up, so if there's another step needed from my side I'll gladly add it before the final Production build :)

Also, could I ask for your insight @aswin-s ? thank you!

@aswin-s
Copy link
Contributor

aswin-s commented Sep 15, 2023

@studentofcoding Everything looks good to me. We'll just have to wait for v1.3.70-2 to be deployed to Prod. Smart banner is shown only when the installed binary supports the url prefix of the current web page and the corresponding entry is added to apple-app-site-association.

@studentofcoding
Copy link
Contributor

@studentofcoding Everything looks good to me. We'll just have to wait for v1.3.70-2 to be deployed to Prod. Smart banner is shown only when the installed binary supports the url prefix of the current web page and the corresponding entry is added to apple-app-site-association.

Hey @aswin-s, thank you for your confirmation & sharing the detailed process!

@studentofcoding
Copy link
Contributor

Hey guys, I saw that @MelvinBot, didn't mention any automatic process on merging to main or staging nor regression period, Is this normal? Thanks!

cc: @dangrous @sophiepintoraetz

@dangrous
Copy link
Contributor

He's been acting up lately, haha. This is merged, on staging, not quite on prod, but almost there!

@studentofcoding
Copy link
Contributor

🤣 Thanks @dangrous!

In the mean time, do I need to re-applied to Upwork @sophiepintoraetz? Thanks

@studentofcoding
Copy link
Contributor

studentofcoding commented Sep 18, 2023

@studentofcoding Everything looks good to me. We'll just have to wait for v1.3.70-2 to be deployed to Prod. Smart banner is shown only when the installed binary supports the url prefix of the current web page and the corresponding entry is added to apple-app-site-association.

Hey @aswin-s, we have v1.3.70-8 on Production but Smart Banner still not showing. Do we miss anything or any config that might interfer with it?

This is interesting as on Staging it's the Smart Banner are already showing on v1.3.70-2

cc @dangrous

image

trim.986FDAAF-ED80-4872-B587-400861442DED.MOV

@aswin-s
Copy link
Contributor

aswin-s commented Sep 18, 2023

@studentofcoding Seems to be working for me?

trim.205493BA-8832-4024-8C28-B8DE913CCEF6.MOV

@studentofcoding
Copy link
Contributor

@studentofcoding Seems to be working for me?

trim.205493BA-8832-4024-8C28-B8DE913CCEF6.MOV

Thanks for your confirmation @aswin-s !

It's finally working on my end too after remove the cache and safari data and (re-install the App)

trim.3769EFB4-9E40-4350-BB3D-2F0A22F2C556.MOV

@dangrous
Copy link
Contributor

Just doing Melvin's job for him (🙄) this was deployed to production 18 hours ago, so we can handle payment (assuming no regressions) September 25th 6:13PM GMT+8!

@sophiepintoraetz sophiepintoraetz changed the title [$1000] Workspaces - Visual flickering when navigating back to workspace list from a WS Payment Sep 25 [$1000] Workspaces - Visual flickering when navigating back to workspace list from a WS Sep 19, 2023
@studentofcoding
Copy link
Contributor

Just doing Melvin's job for him (🙄) this was deployed to production 18 hours ago, so we can handle payment (assuming no regressions) September 25th 6:13PM GMT+8!

What a lazy Melvin! 😂

Thanks for this @dangrous !

@sophiepintoraetz
Copy link
Contributor

@studentofcoding - here's the new job for you to apply to. It would help if you referenced your Upworks profile or GH handle so we can identify you and hire you directly.

@allroundexperts - I think you can raise a ND request for $1000 (potentially including the bonus if there are no regressions that arise).

@studentofcoding
Copy link
Contributor

@studentofcoding - here's the new job for you to apply to. It would help if you referenced your Upworks profile or GH handle so we can identify you and hire you directly.

@allroundexperts - I think you can raise a ND request for $1000 (potentially including the bonus if there are no regressions that arise).

Sure, the Upwork and GH handle are sended also within the Proposal @sophiepintoraetz

@allroundexperts
Copy link
Contributor

Hi Sophie!
I'll do that on Sep 25 so that it does not block my current payment.

@sophiepintoraetz
Copy link
Contributor

so that it does not block my current payment.

Oh interesting, why would it do that??

@allroundexperts
Copy link
Contributor

Oh interesting, why would it do that??

My current IOU report contains a lot of entries right now. The report can be paid only if ALL the entries are approved for payment. If one of the entry is on hold, the rest of the entries couldn't be paid as well. Hope that makes sense 😄

@studentofcoding
Copy link
Contributor

@studentofcoding - here's the new job for you to apply to. It would help if you referenced your Upworks profile or GH handle so we can identify you and hire you directly.

@allroundexperts - I think you can raise a ND request for $1000 (potentially including the bonus if there are no regressions that arise).

Anyway, I registered the GH and Upwork profile on another Issue a week ago too @sophiepintoraetz. But if it's still needed here, here are the details:

Contributor details
Your Expensify account email: yonathanevan@gmail.com
Upwork Profile Link: https://www.upwork.com/freelancers/~01ca6ccb72fa9cc678

On another note, I think Upwork Job is still not accepted, thanks!

@melvin-bot
Copy link

melvin-bot bot commented Sep 24, 2023

⚠️ Missing/invalid email or upwork profile link. Please make sure you add both your Expensify email and Upwork profile link in the format specified.

1 similar comment
@melvin-bot
Copy link

melvin-bot bot commented Sep 24, 2023

⚠️ Missing/invalid email or upwork profile link. Please make sure you add both your Expensify email and Upwork profile link in the format specified.

@studentofcoding
Copy link
Contributor

Leave this here for a context of merging within 3 days (or on 1 Sept) but it got delayed as Merge Freeze

#26172 (comment) Screenshot 2023-09-12 at 22 07 16

Looking forward for the Job acceptance on Upwork and Payment @sophiepintoraetz

Also, Share the context above, as it's on 50% Urgency bonuses

cc: @dangrous @allroundexperts

@sophiepintoraetz
Copy link
Contributor

Payouts due:

Issue Reporter: N/A (Applause)
Contributor: $1500 @studentofcoding
Contributor+: $1500 @allroundexperts

Eligible for 50% #urgency bonus? Y

@JmillsExpensify
Copy link

$1,500 payment approved for @allroundexperts based on BZ summary.

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. Engineering External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review Weekly KSv2
Projects
None yet
Development

No branches or pull requests

9 participants