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

App crashes when attempting to open an #admins room #44583

Closed
1 of 6 tasks
m-natarajan opened this issue Jun 28, 2024 · 29 comments · Fixed by #44708
Closed
1 of 6 tasks

App crashes when attempting to open an #admins room #44583

m-natarajan opened this issue Jun 28, 2024 · 29 comments · Fixed by #44708
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. DeployBlockerCash This issue or pull request should block deployment Engineering Reviewing Has a PR in review Weekly KSv2

Comments

@m-natarajan
Copy link

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!


Version Number: 9.0.3-1
Reproducible in staging?: Yes
Reproducible in production?: No
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: @francoisl
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1719528213578329

Action Performed:

  1. Create a workspace
  2. Open the workspace's settings page and change anything that would cause a system message in the admins room
    For example, create, delete, rename or disable a category ; add tags, etc.
  3. Try to open the admins room for that workspace

Expected Result:

Workspace Admin room opens

Actual Result:

App crashes when opening admin room

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

Screen.Recording.2024-06-27.at.3.41.34.PM.mov
Recording.263.mp4
Recording.262.mp4

View all open jobs on GitHub

@m-natarajan m-natarajan added DeployBlockerCash This issue or pull request should block deployment Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. DeployBlocker Indicates it should block deploying the API labels Jun 28, 2024
Copy link

melvin-bot bot commented Jun 28, 2024

Triggered auto assignment to @CortneyOfstad (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

Copy link

melvin-bot bot commented Jun 28, 2024

Triggered auto assignment to @robertjchen (DeployBlockerCash), see https://stackoverflowteams.com/c/expensify/questions/9980/ for more details.

@github-actions github-actions bot added Engineering Hourly KSv2 and removed Daily KSv2 labels Jun 28, 2024
Copy link
Contributor

👋 Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open `StagingDeployCash` deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

  1. Identify the pull request that introduced this issue and revert it.
  2. Find someone who can quickly fix the issue.
  3. Fix the issue yourself.

@neonbhai
Copy link
Contributor

Fixed on latest main:

Screen.20Recording.202024-06-28.20at.205.mp4

@robertjchen
Copy link
Contributor

got it, thanks!

@mountiny
Copy link
Contributor

@neonbhai Sorry to confirm, you mentioned its fixed on main, but is it fixed on staging?

@mountiny mountiny reopened this Jun 28, 2024
@mountiny mountiny removed the DeployBlocker Indicates it should block deploying the API label Jun 28, 2024
@neonbhai
Copy link
Contributor

@mountiny hi, cannot reproduce on staging:

Screen.Recording.2024-06-28.at.2.30.36.PM.mov

@m-natarajan
Copy link
Author

@mountiny we are still able to reproduce this bug and clicking on the WS right after creating the WS also crashes the app.

Recording.265.mp4

@mountiny
Copy link
Contributor

@neonbhai can you try in offline? maybe its issue with optimistic data

@neonbhai
Copy link
Contributor

I can reproduce the crash when creating a workspace -> changing name -> switching to it from workspace switcher:

Screen.Recording.2024-06-28.at.5.01.28.PM.mov

The console error is for maximum update depth exceeded. Ref link

Screenshot 2024-06-28 at 5 25 48 PM

Crash doesn't reproduce for me for the system message steps (both online and offline):

Attempt 1 (offline)
Screen.Recording.2024-06-28.at.5.05.38.PM.mov

Aside: We don't set an optimistic system message for toggling of category/tags

Attempt 2 (online)
Screen.Recording.2024-06-28.at.5.05.38.PM.mov

cc: @mountiny

@melvin-bot melvin-bot bot added the Overdue label Jul 1, 2024
@robertjchen
Copy link
Contributor

Was this a regression from a recent change? 👀

@melvin-bot melvin-bot bot removed the Overdue label Jul 1, 2024
@jasperhuangg
Copy link
Contributor

@neonbhai Does the stack trace indicate which component is exceeding the max update depth?

@neonbhai
Copy link
Contributor

neonbhai commented Jul 1, 2024

Looks like its coming from the ReportScreen component:

Screenshot 2024-07-02 at 2 02 22 AM

@jasperhuangg
Copy link
Contributor

At first glance this seems suspicious: #44412

@jasperhuangg
Copy link
Contributor

#44412 is not the cause :/

@jasperhuangg
Copy link
Contributor

jasperhuangg commented Jul 1, 2024

The issue has either something to do with:

  • the optimistic data set when we call UpdateWorkspaceGeneralSettings
  • Something about how the workspace switcher works
  • Something about how the ReportScreen works

@jasperhuangg
Copy link
Contributor

I can't seem to reproduce this on desktop

@jasperhuangg
Copy link
Contributor

jasperhuangg commented Jul 1, 2024

I think this is the cause #44559

Namely the code that calls getSnapshot here cc @roryabraham

Need to set USE_REACT_STRICT_MODE flag to false in CONFIG.ts. Can reproduce this consistently with my account on staging with the following steps:

  1. Sign into any account
  2. Create a new workspace.
  3. Rename that workspace.
  4. In the workspace list, click the three dots and click "Go to #admins room".

@robertjchen
Copy link
Contributor

@jasperhuangg Awesome investigation! 🙇

@jasperhuangg
Copy link
Contributor

@robertjchen @roryabraham Still looking into this, it's either caused by #44412 or #44559, or a combination of the two 😆 I'm trying every possible combination now.

@jasperhuangg
Copy link
Contributor

Okay yep confirmed it's caused by #44412, reverting that fixes things for me locally

@jasperhuangg
Copy link
Contributor

Looks like @roryabraham is OOO today so in the interest of pushing deploy forward I'm gonna go ahead and revert that PR

@jasperhuangg
Copy link
Contributor

Screenshot 2024-07-01 at 4 21 24 PM

btw @roryabraham here's the error

@roryabraham
Copy link
Contributor

roryabraham commented Jul 1, 2024

Reverting that PR will cause another regression @jasperhuangg

edit: sorry, was thinking of another PR. This one should be fine to revert

@roryabraham
Copy link
Contributor

@jasperhuangg can you try this PR and see if it solves the problem?

#44681

@roryabraham
Copy link
Contributor

If it does, you could actually take it a step further and just remove the useLastAccessedReportID hook and put findLastAccessedReport in a useEffect directly

@neonbhai
Copy link
Contributor

neonbhai commented Jul 1, 2024

Came across very similar warning to #44583 (comment) while investigating this on native device, asking to memoize getSnapshot call in useOnyx.ts

Image Screenshot 2024-07-01 at 1 44 12 AM

@melvin-bot melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Hourly KSv2 labels Jul 1, 2024
@jasperhuangg jasperhuangg reopened this Jul 1, 2024
@jasperhuangg
Copy link
Contributor

jasperhuangg commented Jul 1, 2024

Ah shoot @roryabraham jumped the gun and missed your messages, my bad! The CP is already under way and it looks like that PR you linked is still an early draft–do you mind if we try to get it out in tomorrow's deploy? I'd feel better if we had some more time to test everything thoroughly without any time pressure today.

@jasperhuangg
Copy link
Contributor

Can't reproduce this anymore on staging

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. DeployBlockerCash This issue or pull request should block deployment Engineering Reviewing Has a PR in review Weekly KSv2
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants