Replace expo-av with expo-audio - #80624
Conversation
|
|
Codecov Report❌ Looks like you've decreased code coverage for some files. Please write tests to increase, or at least maintain, the existing level of code coverage. See our documentation here for how to interpret this table.
|
|
🚧 @roryabraham has triggered a test Expensify/App build. You can view the workflow run here. |
This comment has been minimized.
This comment has been minimized.
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
on it now |
|
@blazejkustra Can you check if it works on an Android native device? I didn't receive the message on the first device after sending it from the second device. It only appears when I open the chat Screen.Recording.2026-01-29.at.15.40.09.mov |
I tested on a physical device and it worked well, I'll try again today and let you know |
|
@roryabraham Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
Weird, I'm not receiving new messages unless I click on the chat. Could you check if it works for you on main @dukenv0307? |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 202ccd4645
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
@blazejkustra I tried to build the app on main branch but still couldn't receive the new message until I opened the chat |
|
Weird, I tested it two days ago and I was able to receive new messages. It must be a recent regression on main 😢 |
|
🚧 @mjasikowski has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
@dukenv0307 let us know in slack how the testing is going! |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppI couldn't verify it for now because I didn't receive the push notification Android: mWeb ChromeiOS: HybridAppI couldn't verify it for now because I didn't receive the push notification iOS: mWeb SafariMacOS: Chrome / SafariScreen.Recording.2026-01-29.at.15.33.28.1.mov |
|
@mountiny All yours! |
roryabraham
left a comment
There was a problem hiding this comment.
only NAB comments, we can address in a follow-up if we want
|
|
||
| // Sound assets must be required at compile time | ||
| const SOUND_ASSETS: Record<ValueOf<typeof SOUNDS>, AudioSource> = { | ||
| [SOUNDS.DONE]: require('@assets/sounds/done.mp3') as AudioSource, |
There was a problem hiding this comment.
NAB follow-up - should we lazy-load these assets?
| [SOUNDS.RECEIVE]: require('@assets/sounds/receive.mp3') as AudioSource, | ||
| }; | ||
|
|
||
| setAudioModeAsync({playsInSilentMode: false, shouldPlayInBackground: true}); |
There was a problem hiding this comment.
NAB: we are calling this async function, but we don't await it finishing before calling playSound. I imagine this could potentially expose us to some race condition where we try and play a sound before these settings finish setting.
So maybe I'd capture the promise returned by this and await it in playSound. Or just call the sync version of this function, which I assume is available.
There was a problem hiding this comment.
Also, should we create a wrapper to ensure it's only called once?
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/roryabraham in version: 9.3.11-19 🚀
|
|
This PR failing because of the issue |
^ Verified on prod, so I'll check this off |
|
🚀 Deployed to production by https://github.com/Julesssss in version: 9.3.12-1 🚀
|
Explanation of Change
Fixed Issues
$ #80742
PROPOSAL: N/A
MOBILE-EXPENSIFY: https://github.com/Expensify/Mobile-Expensify/pull/13831
Tests
For these tests it is recommended to use a secondary device to send the messages to the main testing device.
Offline tests
N/A
QA Steps
Verify all test cases for native android and ios apps
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
untitled.webm
iOS: Native
Simulator.Screen.Recording.-.iPhone.SE.3rd.generation.-.2026-01-29.at.17.05.47.mov