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

[MM-58074] Calls: Fix crashing app when starting/joining call #7926

Merged
merged 1 commit into from
May 1, 2024

Conversation

cpoile
Copy link
Member

@cpoile cpoile commented Apr 30, 2024

Summary

  • The recent bump to Android 34 (Push react native to 0.73.6 #7863) broke calls, this fixes it. Android 34 requires more specific permissions and some new code for foreground services (we're using that to make the call continue while the app is in the background).
  • Totally understandable and unanticipated consequence of updating things. But it highlights a problem we have with mobile -- no calls e2e test infra. Until we get that, I wonder if we could request manual testing from @DHaussermann or myself on any react-native or Android SDK bumps like that, to make sure Calls doesn't break?
  • Related, I reverted the react-native-webrtc upgrade from that PR as well -- we don't want to upgrade that without thorough testing. It's a fast-moving package and often breaks things. If possible, could XYZ update that when needed? (bc of the manual testing required)
  • cc @larkox for visibility, you can review if you'd like.
  • Thanks everyone!

Ticket Link

Checklist

  • Added or updated unit tests (required for all new features)
  • Has UI changes
  • Includes text changes and localization file updates
  • Have tested against the 5 core themes to ensure consistency between them.

Device Information

  • Android: 13, Pixel 6
  • Android: 13, Galaxy Tab s7+
  • iOS: 16.5.1, iPhone 14

Release Note

NONE

@cpoile cpoile added the 2: Dev Review Requires review by a core commiter label Apr 30, 2024
@cpoile cpoile requested review from streamer45 and enahum April 30, 2024 22:28
Copy link
Contributor

@streamer45 streamer45 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for taking care of this. 100% agree on the need for e2e tests.

@amyblais amyblais added this to the v2.17.0 milestone Apr 30, 2024
Copy link
Contributor

@enahum enahum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

E2E are available for the mobile app, but I guess you'll have to do some work to add a way to test calls.

As for the webrtc package, I guess that is fine, but we need to be carefull not to fall that far behind

@cpoile cpoile added 4: Reviews Complete All reviewers have approved the pull request and removed 2: Dev Review Requires review by a core commiter labels May 1, 2024
@cpoile cpoile merged commit c009013 into main May 1, 2024
13 checks passed
@cpoile cpoile deleted the MM-58074-mobile-calls-crash branch May 1, 2024 15:15
@larkox
Copy link
Contributor

larkox commented May 3, 2024

Sorry here! But at least didn't get to production! 😅

@yasserfaraazkhan is finishing getting back the e2e tests for mobile back and running (they have been out of commission for a while). When that is done, we may be able to secure some time to look into adding e2e to calls (but not sure how that would look).

Also, agree with enahum about not falling too far behind. Nevertheless, in future version bumps I will make sure to add you as a reviewer, just in case.

cyrusjc pushed a commit to cyrusjc/mattermost-mobile that referenced this pull request May 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4: Reviews Complete All reviewers have approved the pull request release-note-none
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants