chore: expo audio by cimigree · Pull Request #1660 · digidem/comapeo-mobile

cimigree · 2026-01-20T16:46:04Z

Migration from expo-av to expo-audio

stacked on #1655
stacked on #1651

It doesn't really have to be stacked -- just thought it made sense to deal with this after the expo upgrade.
Closes #1652
Some of the differences, aside from hook and method names:

No permission hooks - manual state management required
Time units: seconds (requires conversion to milliseconds for existing hooks)
No auto-reset - must call seekTo(0) manually for replay
Need prepareToRecordAsync() call before record()

Also, to make the React Compiler happy, I had to refactor the try/ catch blocks:

Minimized creating/ changing values inside catch statements
Removed finally blocks (not supported by React Compiler)
Eliminated throw statements in favor of navigation to error screen

awana-lockfile-bot · 2026-01-20T16:46:20Z

`package-lock.json` changes

Click to toggle table visibility

Name	Status	Previous	Current
`expo-audio`		-	1.1.1
`expo-av`		16.0.8	-

socket-security · 2026-01-20T16:58:16Z

Warning

Review the following alerts detected in dependencies.

According to your organization's Security Policy, it is recommended to resolve "Warn" alerts. Learn more about Socket for GitHub.

Action	Severity	Alert (click "▶" to expand/collapse)
Warn		Obfuscated code: npm `@browserstack/ai-sdk-node` is 100.0% likely obfuscated Confidence: 1.00 Location: Package overview From: package-lock.json → `npm/@wdio/browserstack-service@9.21.0` → `npm/@browserstack/ai-sdk-node@1.5.17` ℹ Read more on: This package \| This alert \| What is obfuscated code? Next steps: Take a moment to review the security alert above. Review the linked package source code to understand the potential risk. Ensure the package is not malicious before proceeding. If you're unsure how to proceed, reach out to your security team or ask the Socket team for help at `support@socket.dev`. Suggestion: Packages should not obfuscate their code. Consider not using packages with obfuscated code. Mark the package as acceptable risk. To ignore this alert only in this pull request, reply with the comment `@SocketSecurity ignore npm/@browserstack/ai-sdk-node@1.5.17`. You can also ignore all packages with `@SocketSecurity ignore-all`. To ignore an alert for all future pull requests, use Socket's Dashboard to change the triage state of this alert.
Warn		Obfuscated code: npm `@browserstack/ai-sdk-node` is 98.0% likely obfuscated Confidence: 0.98 Location: Package overview From: package-lock.json → `npm/@wdio/browserstack-service@9.21.0` → `npm/@browserstack/ai-sdk-node@1.5.17` ℹ Read more on: This package \| This alert \| What is obfuscated code? Next steps: Take a moment to review the security alert above. Review the linked package source code to understand the potential risk. Ensure the package is not malicious before proceeding. If you're unsure how to proceed, reach out to your security team or ask the Socket team for help at `support@socket.dev`. Suggestion: Packages should not obfuscate their code. Consider not using packages with obfuscated code. Mark the package as acceptable risk. To ignore this alert only in this pull request, reply with the comment `@SocketSecurity ignore npm/@browserstack/ai-sdk-node@1.5.17`. You can also ignore all packages with `@SocketSecurity ignore-all`. To ignore an alert for all future pull requests, use Socket's Dashboard to change the triage state of this alert.
Warn		Obfuscated code: npm `@react-native/debugger-frontend` is 96.0% likely obfuscated Confidence: 0.96 Location: Package overview From: package-lock.json → `npm/@react-native/debugger-frontend@0.81.5` ℹ Read more on: This package \| This alert \| What is obfuscated code? Next steps: Take a moment to review the security alert above. Review the linked package source code to understand the potential risk. Ensure the package is not malicious before proceeding. If you're unsure how to proceed, reach out to your security team or ask the Socket team for help at `support@socket.dev`. Suggestion: Packages should not obfuscate their code. Consider not using packages with obfuscated code. Mark the package as acceptable risk. To ignore this alert only in this pull request, reply with the comment `@SocketSecurity ignore npm/@react-native/debugger-frontend@0.81.5`. You can also ignore all packages with `@SocketSecurity ignore-all`. To ignore an alert for all future pull requests, use Socket's Dashboard to change the triage state of this alert.

View full report

…po 53. Other changes as needed.

ErikSin

This is a good first pass, but I think the advantage of the new API is that it simplifies state management and we have not taken full advantage of that here.

The main thing is that we do not need to manually set states related to the recording/audio file (aka timing, whether its playing, whether its recording etc). That is now all exposed in the expo hooks. Both of the hooks in the comapeo code base are now just state syncing and creating states that are already available via the expo hooks.

Let me know if you want to cowork on this together!

ErikSin · 2026-01-26T22:17:52Z

-  const [status, setStatus] = useState<Audio.RecordingStatus | null>(null);
+  const recorder = useAudioRecorder(RECORDING_OPTIONS);
+  const status = useAudioRecorderState(recorder, 100);
+  const [isRecording, setIsRecording] = useState(false);


could we just use status instead of [isRecording, setIsRecording]? It seems like we are manually setting a state here, but that state is already accessible via the status

Yes. Done, 7672fd7

ErikSin · 2026-01-26T22:25:48Z

  const [isPlaying, setPlaying] = useState(false);
-  const [duration, setDuration] = useState<number>(0);
  const [currentPosition, setCurrentPosition] = useState(0);
+  const lastPositionRef = useRef(0);


I don't think we need [isPlaying, setPlaying], [currentPosition, setCurrentPosition], or lastPositionRef.

The new Api has all of these properties exposed in the status, and I believe those are all stateful (because they are exposed via a hook)

Im actually wondering if we even need this hook anymore. Everything is now exposed via the 2 hooks, and if we get rid of all these other useStates, and useRefs, were left with a useEffect at the top level (that is also no longer needed since we don't need to sync any states), and a start and stop function, which essentially just hit play and stop...

I removed the hook. I am not crazy about that. I feel like we now have duplicated code. Granted there wasn't too much in that hook after I simplified the state, but there was something, and now it is in UI files twice.
aba77b6
0d48e32

@cimigree If you think it is better to keep it as a hook, Im open to it. I was suggesting it with the minimal context of doing the code review. In otherwords, I trust your judgment on this and my opinion is never the final say.

Ok I reverted back to a hook.

ErikSin · 2026-01-26T22:38:58Z

+  React.useEffect(() => {
+    getRecordingPermissionsAsync().then(setAudioPermission);
+  }, []);
+


we should do this check before navigating here, and then if there is no permission navigate to the permission screen, otherwise navigate here

It actually works exactly the same without the permission in there. It already is a Nav prop on the bottom sheet so I didn't think it was necessary as everything worked just fine without adding another nav prop.

socket-security · 2026-01-27T23:11:56Z

Review the following changes in direct dependencies. Learn more about Socket for GitHub.

Diff	Package	Supply Chain Security	Vulnerability	Quality	Maintenance	License
	npm/@osm_borders/maritime_10000m@1.1.0
	npm/@formatjs/cli@6.8.2
	npm/@types/lodash.isequal@4.5.8
	npm/@react-native/typescript-config@0.76.9
	npm/@react-native/metro-babel-transformer@0.76.9
	npm/@types/lint-staged@13.3.0
	npm/@types/react-native-zeroconf@0.13.1
	npm/@comapeo/nodejs-mobile-react-native@18.20.4-2
	npm/@types/react-native-indicators@0.16.6
	npm/@react-native-documents/picker@11.0.0
	npm/@react-native/metro-config@0.79.5
	npm/@tanstack/eslint-plugin-query@5.91.2
	npm/@types/semver@7.7.1
	npm/@react-navigation/native-stack@7.3.21
	npm/@react-navigation/native@7.1.20
	npm/@mapeo/mock-data@5.0.0
	npm/@react-navigation/bottom-tabs@7.8.5
	npm/@comapeo/cloud@0.3.0
	npm/@formatjs/intl-pluralrules@5.4.6
	npm/@react-navigation/stack@7.6.4
	npm/@types/jest@30.0.0
	npm/@formatjs/intl-getcanonicallocales@2.5.6
	npm/@babel/preset-env@7.28.5
	npm/@types/mocha@10.0.10
	npm/@formatjs/intl-locale@4.2.13
	npm/@react-native-vector-icons/octicons@20.4.0
	npm/@types/react@19.2.7
	npm/@babel/runtime@7.27.1 ⏵ 7.28.4
	npm/@react-native-vector-icons/fontisto@12.4.0
	npm/@types/utm@1.1.4
	npm/@formatjs/intl-relativetimeformat@11.4.13
	npm/@comapeo/core-react@7.2.0
See 34 more rows in the dashboard

View full report

ErikSin

Good job on the second pass, the code feel much cleaner now!

The main blocking comment is that your hooks are not agnostic and rely on the navigation context. I think both the hooks should not be dealing with navigating to the error bottom sheet and should be dealt with by the consuming screen.

I also noticed alot of checks for the existence of things. Your doing early throws and ternaries for objects that are never undefined/null (according to the type definitions), so i dont think they are necessary. I could be wrong, so let me know if there is some context that I am missing.

Lastly, the nature of the hooks have changed. They are both just forwarding other hooks, which is not a problem, and it does make the code simpler since they are being reused. BUT if we are doing that, I think we need to memoize the values that are being exposed OR expose the entire value. It is just misleading to expose only certain parts of the status when any update to the status is going to cause re-renders regardless.

ErikSin · 2026-01-29T19:23:37Z

+    if (!recorder) {
+      navigate('ErrorBottomSheet');
+      return;
+    }


I don't think this check is necessary. If useAudioRecorder is not returning the recorder we have a bigger issue with expo...

ErikSin · 2026-01-29T19:25:59Z

-          stat => setStatus(stat),
-        );
-        setRecordingInstance(recording);
+        await recorder.prepareToRecordAsync();


this second try that re-attempts to prepare the recording I think is also unnecessary. If its failing once, trying again right after shouldn't make a difference and i think it just complicates the code

ErikSin · 2026-01-29T19:30:59Z

+    if (!recorder || status.isRecording === false) {
+      navigate('ErrorBottomSheet');
+      return;
+    }


Is there a worry that the recorder won't be instantiated? I think the check for a recorder and then throwing an error is not necessary, it feels like its just complicating the code.

I also don't think we need to check status.isRecording === false. if we just call recorder.stop(); its already in a try catch. So it should either do nothing, or if does throw an error, the catch will cause the error bottom sheet to open, so no point it checking it twice.

ErikSin · 2026-01-29T19:40:12Z

I don't think the hook should be responsible for navigation to the error bottom sheet. It should be agnostic, and only deal with recorder states. And then the screen, which has access to the navigation should be catching the errors and should be in charge of those navigations. IT also will make the stack trace clearer and we will know what screen is causing the error

ErikSin · 2026-01-29T19:43:38Z

  const startPlayback = useCallback(async () => {
-    if (!recordedSoundRef.current || isPlaying) return;
+    if (!player || status.playing) return;


i don't think we need to check for the existence of the player here

ErikSin · 2026-01-29T19:45:34Z


  const stopPlayback = useCallback(async () => {
-    if (!recordedSoundRef.current || !isPlaying) return;
+    if (!player || !status.playing) return;


same comment as above, i don't think we need check for the existence of the player

ErikSin · 2026-01-29T20:01:08Z

+    duration: status.duration ? status.duration * 1000 : 0,
+    isPlaying: status.playing,
+    currentPosition: status.currentTime ? status.currentTime * 1000 : 0,


the ternarary checks I don't think are necessary, duration and currentTime are never undefined or null.

Also, these are not memoized so any component consuming these hooks will be rerendered if the status is updated in any way, even if that status update does not update duration, playing, or current time. Which is not necessarily a problem (its a design flaw of react, not us), but it makes me question whether we need to only expose just these 3 things. It we return the entire status, its really clear that the component is consuming the entire status and we are being explicit that renders are effected by the status as a whole.

I'm open to alternatives if you feel strongly about, but IMO it just feels simpler and easier to expose the entire status object.

ErikSin · 2026-01-29T20:08:35Z

the useCallbacks are now unnecessary. Since the status is at the top level, the consuming components will be re-render called any time the status is updated. So memoizing these functions will not stop an unnecessary re-renders because it is going to happen anyways

Secondly, same comment about the error bottom sheet. I dont think the hook should be responsible for the navigation, the consuming component should be responsible for it.

ErikSin · 2026-01-29T22:06:16Z

+    player,
+    status,
+    duration: status.duration * 1000,
+    isPlaying: status.playing,
+    currentPosition: status.currentTime * 1000,
    startPlayback,


I think there was a misunderstanding in my comment. I was trying to say It is misleading that the duration, isPlaying, and currentPosition are there own return items. As a hook, it leads me to believe that if I use any of these 3 things, that they will statefully update the consuming component only when those 3 things change.

So i am suggesting that we should get rid of those 3 completely and ONLY return status. That way there is no ambiguity that consuming component will update with EVERY change in status.

Also if you want to return the player, then i don't think we need to expose startPayback() and stopPlayback(). But if we do that, I don't think we should be using this hook at all...

ErikSin

Everything looks pretty good now, but i am getting an error thrown when trying to set permissions.

I wanted to check in about the removal of the useAudioPlayback hook. I'm a little confused because when I originally suggested removing it, you mentioned wanting to keep it. Later in review, I proposed that if we were going to expose the recorder, keeping the hook might not make sense—and I was hoping we could discuss the trade-offs together before deciding.

I realize my suggestion may have come across as a recommendation rather than an invitation to talk it through. My intent was to open a conversation so we could align on the architecture direction, especially since we had different initial takes. I'd love to find a smoother way for us to navigate these kinds of decisions when we have differing views—something I know I have talked to you about before.

Do you have suggestion of how we can make these architectural conversations more collaborative going forward?

ErikSin · 2026-02-02T19:34:42Z

@@ -40,7 +39,6 @@ export function AudioRecording({
  const {startRecording, stopRecording, status} = useAudioRecording();
  const timeElapsed = status?.durationMillis || 0;


status is no longer optional, so can change to const timeElapsed = status.durationMillis

ErikSin · 2026-02-02T19:42:20Z

      }
+    }

+    try {


is 2 try/catches necessary here?

ErikSin · 2026-02-02T19:43:58Z

+        player.play();
+      }
+    } catch {
+      navigation.navigate('ErrorBottomSheet');


We should pass the error to sentry as well

ErikSin · 2026-02-02T19:44:31Z

+        console.error('Failed to start recording:', error);
+        navigation.replace('ErrorBottomSheet');


we can get rid of the console, and pass the error to sentry

ErikSin · 2026-02-02T20:00:54Z

Something is happening thats causing an error to be thrown. When i grant my permission from this page, its attempting to navigate to the the audio record page and then an error is being thrown. My guess is that the permission is being set but the app hasn't registered that is set yet. The error that is getting thrown is:

ERROR Failed to start recording: [Error: Call to function 'AudioRecorder.record' has been rejected. → Caused by: The 1st argument cannot be cast to type expo.modules.audio.AudioRecorder (received class java.lang.Integer) → Caused by: Cannot use shared object that was already released]

9b0676c & ebcc82c

These changes were the only way I could get this to work. The double try... I think that is why that was there. I also had to take the error handling out of the screen in this situation. If I did two tries, I think I could leave the error handling in the screen, but I would need to have a catch in the hook, which I know you didn't want.
I know it's not what you had in mind. I tried a lot of things. A lot of things.
If you have other ideas, would you mind just taking over the PR?

Hmm, I am not sure when I said that I didn't want a catch in a hook. Is there a comment that your specifically referring to, or was it something i said in a meeting? Regardless, I think that was a misunderstanding, because I think it is perfectly normal to have a catch in a hook.

Also, the screen should handle navigation to error bottom, but the hook can handle the actual error and just pass it to the screen. Which it was what your doing right now, no? I think I don't fully understand what you mean when you say "I also had to take the error handling out of the screen in this situation".

Ok I am sure I misunderstood. I think what you didn't want was navigation. My mistake.
The issue here is that with the startRecording function in src/frontend/screens/Audio/AudioRecording/useAudioRecording.ts, there is always an error with enabling permissions. The recorder just isn't ready as soon as we arrive from granting permissions. The second try fixed the issue in the old code. In this pass of this code, I changed it to use a specific if/ then statement. But either way (two tries or the if/ then) an error comes from the hook. And when an error comes from the hook, if I have a try/ catch in that first useEffect in the AudioRecording/index.tsx file, then an error will be thrown and we will navigate to the error bottom sheet.
If I don't have a try catch in there, then it all works.
Is it ok to not have a try catch in that useEffect at the top of src/frontend/screens/Audio/AudioRecording/index.tsx?

So i updated expo-audio to the latest and it fixed all these problems. no need for a 2 try/catches. And now we can put the try catch back into the useEffect at the top of src/frontend/screens/Audio/AudioRecording/index.tsx

@ErikSin What is the latest? I tried 1.1.1 and I got the same thing when I removed the extra if/ then.

screen-20260205-234826.mp4

yup 1.1.1. Works on both my emulator and physical device

Screen.Recording.2026-02-05.at.3.26.14.PM.mov

in the hook:

const startRecording = useCallback(async () => { await recorder.prepareToRecordAsync(); recorder.record(); }, [recorder]);

in the screen:

React.useEffect(() => { const start = async () => { await startRecording(); setIsLoading(false); }; try { start(); } catch { navigation.navigate('ErrorBottomSheet'); } }, [startRecording, navigation]);

Ok thanks. I had the try catch wrong. This worked and is now pushed up.

…try catch to screen.

ErikSin

i think everything is addressed now, thanks!

ErikSin · 2026-02-09T17:37:37Z

+      start();
+    } catch {
+      navigation.navigate('ErrorBottomSheet');
+    }


Sorry, my example that i gave was just simplified to show that it was working. We should still catch the error and pass it to sentry

We should also do a navigation.replace() here so the user is navigated back to the observation create screen when they close the error bottom sheet.

cimigree requested a review from ErikSin January 20, 2026 18:28

cimigree changed the title ~~Chore/expo audio~~ chore: expo audio Jan 21, 2026

cimigree added 3 commits January 26, 2026 14:57

Converts usage of expo av to the api for expo audio.

5ad0ab5

Fixes for react compiler.

54b57b9

Oops forgot to commit the json file

9c16bb7

cimigree force-pushed the chore/expo-audio branch from 0e1943a to 9c16bb7 Compare January 26, 2026 19:59

Changes to different version of expo audio that is compatible with ex…

7589c8f

…po 53. Other changes as needed.

ErikSin requested changes Jan 26, 2026

View reviewed changes

cimigree added 3 commits January 27, 2026 11:25

Merge branch 'develop' into chore/expo-audio

fea4c05

Simplifies permissions for audio. Takes state syncing out of our hooks.

7672fd7

Simplifies code in hook for playback.

40ef024

cimigree force-pushed the chore/expo-audio branch from 0d48e32 to 40ef024 Compare January 27, 2026 23:11

cimigree requested a review from ErikSin January 27, 2026 23:13

Merges with develop

7ab8104

ErikSin requested changes Jan 29, 2026

View reviewed changes

Simplifies audio recording code.

eef64cd

ErikSin reviewed Jan 29, 2026

View reviewed changes

Removes playbook hook again.

5c8c8d8

ErikSin requested changes Feb 2, 2026

View reviewed changes

cimigree added 2 commits February 2, 2026 16:31

Code works to record from permissions without error.

9b0676c

Takes unneeded error catching out of hook.

ebcc82c

cimigree requested a review from ErikSin February 3, 2026 03:10

cimigree added 5 commits February 3, 2026 11:51

Merge branch 'develop' into chore/expo-audio

45e94e6

Moves from generic try catch to specific if then statement

b150da4

Merges with develop

fa1b07d

Updates expo audio version. Removes check for working recorder. Adds …

6bfccb1

…try catch to screen.

Adjusts try catch statement

a1a6472

ErikSin reviewed Feb 9, 2026

View reviewed changes

Fixes to error handling and navigation.

058faea

cimigree requested a review from ErikSin February 10, 2026 16:57

ErikSin approved these changes Feb 10, 2026

View reviewed changes

cimigree merged commit b197710 into develop Feb 10, 2026
12 of 13 checks passed

cimigree deleted the chore/expo-audio branch February 10, 2026 18:09

		@@ -40,7 +39,6 @@ export function AudioRecording({
		const {startRecording, stopRecording, status} = useAudioRecording();
		const timeElapsed = status?.durationMillis \|\| 0;

                     }
+                  }
+                  try {

		console.error('Failed to start recording:', error);
		navigation.replace('ErrorBottomSheet');

Uh oh!

Conversation

cimigree commented Jan 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

awana-lockfile-bot Bot commented Jan 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

package-lock.json changes

Uh oh!

socket-security Bot commented Jan 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ErikSin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cimigree Jan 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

socket-security Bot commented Jan 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ErikSin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ErikSin left a comment

Choose a reason for hiding this comment

Uh oh!

cimigree commented Jan 20, 2026 •

edited

Loading

awana-lockfile-bot Bot commented Jan 20, 2026 •

edited

Loading

`package-lock.json` changes

socket-security Bot commented Jan 20, 2026 •

edited

Loading

cimigree Jan 27, 2026 •

edited

Loading

socket-security Bot commented Jan 27, 2026 •

edited

Loading