chore: migrate from expo-av to expo-audio and expo-video#6719
chore: migrate from expo-av to expo-audio and expo-video#6719Rohit3523 wants to merge 233 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughMigrates audio and video usage from expo-av to expo-audio and expo-video across recording, playback, VoIP, ringer, previews, tests, and package dependencies, and updates seek and status handling to the new APIs. ChangesExpo media migration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Recording is not working... idk why... |
…t.Chat.ReactNative into react-native-81
Rohit3523
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
Rohit3523
had a problem deploying
to
experimental_android_build
GitHub Actions
Error
Rohit3523
had a problem deploying
to
official_android_build
GitHub Actions
Error
Rohit3523
had a problem deploying
to
official_ios_build
GitHub Actions
Error
Rohit3523
had a problem deploying
to
experimental_ios_build
GitHub Actions
Error
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/lib/services/voip/playCallEndedSound.test.ts (1)
9-14: ⚡ Quick winConsider adding a comment clarifying the mock instance strategy.
The mock setup creates a shared
mockPlayer(line 14), but individual tests useactualPlayerfrommockCreateAudioPlayer.mock.results[0].valueto verify behavior. While this is the correct approach, a comment explaining thatmockPlayeris the template instance (provided by jest.setup.js) and tests should useactualPlayerfrom mock results would improve clarity for future maintainers.📝 Suggested clarification comment
// Access the global mock from jest.setup.js - it provides a consistent mock player +// Note: Individual tests should use `mockCreateAudioPlayer.mock.results[n].value` +// to access the actual player instance returned by each call, not this shared template. // eslint-disable-next-line `@typescript-eslint/no-var-requires` const expoAudio = require('expo-audio');🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/lib/services/voip/playCallEndedSound.test.ts` around lines 9 - 14, Add a short clarifying comment above the mock setup explaining that expoAudio.createAudioPlayer (mockCreateAudioPlayer) returns a template mock instance (mockPlayer) provided by jest.setup.js and that tests should reference the concrete instance created during the test run via mockCreateAudioPlayer.mock.results[0].value (often named actualPlayer) when asserting behavior; mention both symbols mockPlayer and mockCreateAudioPlayer.mock.results to guide future maintainers.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/lib/services/voip/playCallEndedSound.ts`:
- Line 1: The code calls player?.release() which is not part of expo-audio
v0.4.9; update the cleanup to call player?.remove() instead (replace both
occurrences of player?.release() in the playCallEndedSound implementation), and
ensure any playbackStatusUpdate listeners are removed if you added them (refer
to createAudioPlayer and the local player variable and its addListener usage) so
resources are cleaned with player.remove().
---
Nitpick comments:
In `@app/lib/services/voip/playCallEndedSound.test.ts`:
- Around line 9-14: Add a short clarifying comment above the mock setup
explaining that expoAudio.createAudioPlayer (mockCreateAudioPlayer) returns a
template mock instance (mockPlayer) provided by jest.setup.js and that tests
should reference the concrete instance created during the test run via
mockCreateAudioPlayer.mock.results[0].value (often named actualPlayer) when
asserting behavior; mention both symbols mockPlayer and
mockCreateAudioPlayer.mock.results to guide future maintainers.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 82d96c33-6c4b-4cea-9a28-7da554bbd6f9
📒 Files selected for processing (4)
app/containers/MessageComposer/components/Buttons/MicOrSendButton.tsxapp/containers/MessageComposer/components/RecordAudio/RecordAudio.tsxapp/lib/services/voip/playCallEndedSound.test.tsapp/lib/services/voip/playCallEndedSound.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- app/containers/MessageComposer/components/RecordAudio/RecordAudio.tsx
- app/containers/MessageComposer/components/Buttons/MicOrSendButton.tsx
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: ESLint and Test / run-eslint-and-test
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{js,ts,jsx,tsx}: Use descriptive names for functions, variables, and classes that clearly convey their purpose
Write comments that explain the 'why' behind code decisions, not the 'what'
Keep functions small and focused on a single responsibility
Use const by default, let when reassignment is needed, and avoid var
Prefer async/await over .then() chains for handling asynchronous operations
Use explicit error handling with try/catch blocks for async operations
Avoid deeply nested code; refactor complex logic into helper functions
Files:
app/lib/services/voip/playCallEndedSound.tsapp/lib/services/voip/playCallEndedSound.test.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use TypeScript for type safety; add explicit type annotations to function parameters and return types
Prefer interfaces over type aliases for defining object shapes in TypeScript
Use enums for sets of related constants rather than magic strings or numbers
Files:
app/lib/services/voip/playCallEndedSound.tsapp/lib/services/voip/playCallEndedSound.test.ts
**/*.{ts,tsx,js,jsx,json}
📄 CodeRabbit inference engine (CLAUDE.md)
Use Prettier formatting with tabs, single quotes, 130 character width, no trailing commas, avoid arrow parens, and bracket same line
Files:
app/lib/services/voip/playCallEndedSound.tsapp/lib/services/voip/playCallEndedSound.test.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use ESLint with
@rocket.chat/eslint-configbase including React, React Native, TypeScript, and Jest plugins
Files:
app/lib/services/voip/playCallEndedSound.tsapp/lib/services/voip/playCallEndedSound.test.ts
app/lib/services/voip/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Implement VoIP WebRTC peer-to-peer audio calls with Zustand stores (not Redux) in 'app/lib/services/voip/' with native CallKit (iOS) and Telecom (Android) integration
Files:
app/lib/services/voip/playCallEndedSound.tsapp/lib/services/voip/playCallEndedSound.test.ts
🧠 Learnings (1)
📚 Learning: 2026-04-30T17:07:51.020Z
Learnt from: diegolmello
Repo: RocketChat/Rocket.Chat.ReactNative PR: 7274
File: app/lib/services/voip/MediaCallEvents.ts:0-0
Timestamp: 2026-04-30T17:07:51.020Z
Learning: In this Rocket.Chat React Native codebase, the ESLint rule `no-void: error` is enforced. When you see a promise returned from an async call that is not awaited (a “floating promise”), do not silence it with the `void somePromise()` pattern. Instead, handle the promise explicitly by attaching `.catch(...)` (or otherwise awaiting/handling the error) so unhandled-rejection risks are addressed in a way that satisfies the existing ESLint configuration.
Applied to files:
app/lib/services/voip/playCallEndedSound.tsapp/lib/services/voip/playCallEndedSound.test.ts
🔇 Additional comments (6)
app/lib/services/voip/playCallEndedSound.ts (3)
53-57: Watchdog capturescurrentPlayercorrectly before releasing.The watchdog pattern is implemented safely: it captures the player reference (line 54) before clearing the module state (line 55), then releases the captured instance (line 56). This ensures that even if
currentPlayeris set to null byreleaseLock(), the original player is still released.However, this comment is contingent on resolving the listener closure issue flagged separately. If the listener uses value-capture (as suggested), the watchdog should use the same pattern for consistency.
14-21:releaseLock()correctly resets all module state.The function now clears
currentPlayer(line 16) in addition to the flag and timer, ensuring the module is fully reset for the next invocation.
43-49: 🏗️ Heavy liftRace condition analysis: This claim is incorrect. The
isPlayingguard at lines 34-36 prevents concurrent players. When a listener fires,releaseLock()at line 46 atomically sets bothisPlaying = falseandcurrentPlayer = nullbefore exiting. This blocks any newplayCallEndedSound()invocation from proceeding past the guard until the current player fully completes. The scenario described (Player A's listener firing while Player B exists) cannot occur because Player B cannot be created whileisPlayingis true. Additionally, line 45 already captures the player into a local variable, which is a reference to the player instance at that moment—even ifcurrentPlayerchanged later (which it cannot due to the guard), the local variable would remain unchanged. The code is safe as written.> Likely an incorrect or invalid review comment.app/lib/services/voip/playCallEndedSound.test.ts (3)
37-50: Well-structured test for didJustFinish → release flow.The test correctly:
- Creates a player via the function under test
- Retrieves the actual player instance from
mockCreateAudioPlayer.mock.results[0].value- Extracts the registered listener callback
- Invokes the callback with the expected status
- Asserts release was called
This pattern (using
actualPlayerfrom mock results rather than the sharedmockPlayer) ensures the test verifies the exact instance created by the code, which is the right approach.
95-118: Watchdog test correctly uses fake timers and verifies lock release.The test properly:
- Wraps fake timers in try/finally to ensure cleanup
- Verifies the player is not released before the timeout
- Advances timers by exactly
WATCHDOG_MS(5000ms)- Confirms the watchdog releases the player
- Validates that a subsequent invocation is allowed (lock was cleared)
120-137: Excellent test verifying watchdog is cleared by natural completion.This test ensures that when the listener fires
didJustFinish, the watchdog timer is properly cleared and does not cause a double-release. The assertion thatactualPlayer.releaseis called exactly once (line 133) after advancing timers well past the watchdog threshold confirms the timer was successfully cancelled.
| @@ -1,17 +1,19 @@ | |||
| import { Audio } from 'expo-av'; | |||
| import { createAudioPlayer } from 'expo-audio'; | |||
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
expo-audio 0.4.9 createAudioPlayer API documentation and AudioPlayer methods
💡 Result:
expo-audio 0.4.9: createAudioPlayer and AudioPlayer API/methods 1) createAudioPlayer API - Import: import { createAudioPlayer } from 'expo-audio'; [1][2] - Signature (from Expo package source): - createAudioPlayer(source?: AudioSource | string | number | null, options?: AudioPlayerOptions): AudioPlayer [3] - Purpose: creates an AudioPlayer instance that “doesn't release automatically” (so you must manage lifecycle yourself). [1][2] - Lifecycle note: Expo’s docs say that when using createAudioPlayer directly (instead of the useAudioPlayer hook), it’s your responsibility to call release() when done, otherwise you risk memory leaks. [1][2] - Options fields used by the implementation: - updateInterval (default 500) - downloadFirst (default false) - keepAudioSessionActive (default false) - preferredForwardBufferDuration (default 0) [3] - If downloadFirst is true and source is provided, Expo resolves/downloads the source and then calls player.replace(resolved); on failure it logs a warning and falls back to player.replace(fallback). [3] 2) AudioPlayer methods (imperative API) Expo’s docs list the following AudioPlayer methods: [1][2] - clearLockScreenControls() [1][2] - pause() [1][2] - play() [1][2] - remove() [1][2] - replace(source) [1][2] - seekTo(seconds, toleranceMillisBefore, toleranceMillisAfter) [1][2] - setActiveForLockScreen(active, metadata, options) [1][2] - setPlaybackRate(rate, pitchCorrectionQuality) [1][2] - updateLockScreenMetadata(metadata) [1][2] Note: Expo also has useAudioPlayer / useAudioPlayerStatus in the same docs page; those manage lifecycle automatically. [1][2] Sources: [1] https://docs.expo.dev/versions/v54.0.0/sdk/audio/ [2] https://docs.expo.dev/versions/latest/sdk/audio/ [3] https://github.com/expo/expo/blob/073ace8d/packages/expo-audio/src/ExpoAudio.ts
Citations:
- 1: https://docs.expo.dev/versions/v54.0.0/sdk/audio
- 2: https://docs.expo.dev/versions/latest/sdk/audio/
- 3: https://expo-expo.mintlify.app/sdk/audio
🏁 Script executed:
find . -name "package.json" -type f | head -20Repository: RocketChat/Rocket.Chat.ReactNative
Length of output: 1746
🏁 Script executed:
cat app/lib/services/voip/playCallEndedSound.tsRepository: RocketChat/Rocket.Chat.ReactNative
Length of output: 2062
🏁 Script executed:
grep -A 5 '"expo-audio"' ./package.jsonRepository: RocketChat/Rocket.Chat.ReactNative
Length of output: 261
🏁 Script executed:
npm info expo-audio@0.4.9 | head -50Repository: RocketChat/Rocket.Chat.ReactNative
Length of output: 1168
🏁 Script executed:
# Extract and examine the expo-audio 0.4.9 tarball
cd /tmp && npm pack expo-audio@0.4.9 2>/dev/null && tar -tzf expo-audio-0.4.9.tgz | grep -E '(index|Audio)' | head -20Repository: RocketChat/Rocket.Chat.ReactNative
Length of output: 930
🏁 Script executed:
cd /tmp && tar -xzf expo-audio-0.4.9.tgz && cat package/build/ExpoAudio.js | head -200Repository: RocketChat/Rocket.Chat.ReactNative
Length of output: 4474
🏁 Script executed:
cat /tmp/package/build/index.jsRepository: RocketChat/Rocket.Chat.ReactNative
Length of output: 300
🏁 Script executed:
cat /tmp/package/build/AudioModule.types.d.ts | head -150Repository: RocketChat/Rocket.Chat.ReactNative
Length of output: 4615
🏁 Script executed:
cat /tmp/package/build/Audio.types.d.ts | head -200Repository: RocketChat/Rocket.Chat.ReactNative
Length of output: 5135
🏁 Script executed:
grep -i "release\|remove" /tmp/package/build/AudioModule.types.d.tsRepository: RocketChat/Rocket.Chat.ReactNative
Length of output: 157
Use player.remove() instead of player.release().
The expo-audio v0.4.9 AudioPlayer API provides remove(): void to clean up resources, not release(). The code calls player?.release() on lines 49 and 61, which will fail at runtime. The verified API methods are:
createAudioPlayer(require(...))✓player.addListener('playbackStatusUpdate', callback)✓status.isLoadedandstatus.didJustFinish✓player.play()✓player.remove()(notrelease())
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/lib/services/voip/playCallEndedSound.ts` at line 1, The code calls
player?.release() which is not part of expo-audio v0.4.9; update the cleanup to
call player?.remove() instead (replace both occurrences of player?.release() in
the playCallEndedSound implementation), and ensure any playbackStatusUpdate
listeners are removed if you added them (refer to createAudioPlayer and the
local player variable and its addListener usage) so resources are cleaned with
player.remove().
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/views/AttachmentView.tsx (1)
187-187:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFloating promise:
FileSystem.deleteAsyncresult is not handled.This async call returns a promise that is neither awaited nor has error handling. Per the codebase's
no-void: errorESLint rule, floating promises must be handled explicitly with.catch().🔧 Proposed fix
- FileSystem.deleteAsync(file, { idempotent: true }); + FileSystem.deleteAsync(file, { idempotent: true }).catch(() => { + // cleanup failure is non-critical; file will be cleaned up by OS eventually + });Based on learnings: "In this Rocket.Chat React Native codebase, the ESLint rule
no-void: erroris enforced. When you see a promise returned from an async call that is not awaited, handle the promise explicitly by attaching.catch(...)so unhandled-rejection risks are addressed."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/views/AttachmentView.tsx` at line 187, The call to FileSystem.deleteAsync(file, { idempotent: true }) is a floating promise and must be handled; update the code in AttachmentView to either await the call if inside an async function or (per project lint rule) attach a .catch(...) to the returned promise to handle errors, e.g., call FileSystem.deleteAsync(...).catch(err => {/* log via processLogger/console or swallow with a comment */}); ensure you reference the FileSystem.deleteAsync invocation in AttachmentView and use the existing logging utility (or console.error) inside the catch handler.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@app/views/AttachmentView.tsx`:
- Line 187: The call to FileSystem.deleteAsync(file, { idempotent: true }) is a
floating promise and must be handled; update the code in AttachmentView to
either await the call if inside an async function or (per project lint rule)
attach a .catch(...) to the returned promise to handle errors, e.g., call
FileSystem.deleteAsync(...).catch(err => {/* log via processLogger/console or
swallow with a comment */}); ensure you reference the FileSystem.deleteAsync
invocation in AttachmentView and use the existing logging utility (or
console.error) inside the catch handler.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8f6e8f60-7e9e-423d-93a6-ab5937194e3f
📒 Files selected for processing (3)
app/containers/MessageComposer/components/Buttons/MicOrSendButton.tsxapp/views/AttachmentView.tsxapp/views/ShareView/Preview.tsx
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: ESLint and Test / run-eslint-and-test
- GitHub Check: format
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{js,ts,jsx,tsx}: Use descriptive names for functions, variables, and classes that clearly convey their purpose
Write comments that explain the 'why' behind code decisions, not the 'what'
Keep functions small and focused on a single responsibility
Use const by default, let when reassignment is needed, and avoid var
Prefer async/await over .then() chains for handling asynchronous operations
Use explicit error handling with try/catch blocks for async operations
Avoid deeply nested code; refactor complex logic into helper functions
Files:
app/views/ShareView/Preview.tsxapp/views/AttachmentView.tsxapp/containers/MessageComposer/components/Buttons/MicOrSendButton.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use TypeScript for type safety; add explicit type annotations to function parameters and return types
Prefer interfaces over type aliases for defining object shapes in TypeScript
Use enums for sets of related constants rather than magic strings or numbersUse TypeScript with strict mode and baseUrl set to app/ for import resolution
Files:
app/views/ShareView/Preview.tsxapp/views/AttachmentView.tsxapp/containers/MessageComposer/components/Buttons/MicOrSendButton.tsx
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx,js,jsx}: Use Prettier with tabs, single quotes, 130 char width, no trailing commas, arrow parens avoid, bracket same line
Use@rocket.chat/eslint-configbase with React, React Native, TypeScript, Jest plugins
Files:
app/views/ShareView/Preview.tsxapp/views/AttachmentView.tsxapp/containers/MessageComposer/components/Buttons/MicOrSendButton.tsx
app/views/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
View components (70+ screen components) should be placed in app/views/ directory
Files:
app/views/ShareView/Preview.tsxapp/views/AttachmentView.tsx
app/containers/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Reusable UI components should be placed in app/containers/ directory
Files:
app/containers/MessageComposer/components/Buttons/MicOrSendButton.tsx
🧠 Learnings (1)
📚 Learning: 2026-04-30T17:07:51.020Z
Learnt from: diegolmello
Repo: RocketChat/Rocket.Chat.ReactNative PR: 7274
File: app/lib/services/voip/MediaCallEvents.ts:0-0
Timestamp: 2026-04-30T17:07:51.020Z
Learning: In this Rocket.Chat React Native codebase, the ESLint rule `no-void: error` is enforced. When you see a promise returned from an async call that is not awaited (a “floating promise”), do not silence it with the `void somePromise()` pattern. Instead, handle the promise explicitly by attaching `.catch(...)` (or otherwise awaiting/handling the error) so unhandled-rejection risks are addressed in a way that satisfies the existing ESLint configuration.
Applied to files:
app/views/ShareView/Preview.tsxapp/views/AttachmentView.tsxapp/containers/MessageComposer/components/Buttons/MicOrSendButton.tsx
🔇 Additional comments (8)
app/containers/MessageComposer/components/Buttons/MicOrSendButton.tsx (1)
11-11: LGTM!Also applies to: 20-20, 55-55
app/views/AttachmentView.tsx (5)
49-58: Guard against repeatederrorevents firing duplicate alerts andgoBackcalls.The
statusChangeevent can fire multiple times witherrorstatus (e.g., transient → terminal error). Without a guard, this handler will stack multipleAlert.alertdialogs and callnavigation.goBack()repeatedly.♻️ Suggested guard using a ref
+ const hasHandledErrorRef = React.useRef(false); + useEventListener(player, 'statusChange', ({ status }) => { if (status === 'readyToPlay') { setLoading(false); - } else if (status === 'error') { + } else if (status === 'error' && !hasHandledErrorRef.current) { + hasHandledErrorRef.current = true; setLoading(false); Alert.alert(I18n.t('Error'), I18n.t('There_was_an_error_while_action', { action: I18n.t('playing_video') })); navigation.goBack(); } });
55-55: Theplaying_videotranslation key does not exist.The key is missing from all translation JSON files. Users will see the literal string
playing_videoin the error alert. Use an existing key likeError_play_videoor addplaying_videoto all locale files.
97-106: LGTM!
120-123: LGTM!Also applies to: 134-150, 152-194, 196-213
217-223: LGTM!app/views/ShareView/Preview.tsx (2)
19-25: LGTM!
83-97: LGTM!Also applies to: 101-110
Rohit3523
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
diegolmello
left a comment
diegolmello
left a comment
There was a problem hiding this comment.
Besides this, make sure to resolve code rabbit
| const durationSeconds = data.duration ?? 0; | ||
| duration.value = durationSeconds > 0 ? durationSeconds : 0; | ||
| const currentSecond = data.currentTime ?? 0; |
| const permissionToUpload = useCanUploadFile(rid); | ||
| useKeepAwake(); | ||
|
|
||
| async function doRecording() { |
There was a problem hiding this comment.
Any reason to rename and move away from the effect?
| const player = useAudioPlayer(RINGER_SOUND_FILES[ringer]); | ||
|
|
||
| useEffect(() => { | ||
| const loadAndPlay = async () => { |
There was a problem hiding this comment.
Any reason to remove the conditional loading?
| bitRate: Audio.RecordingOptionsPresets.LOW_QUALITY.android.bitRate | ||
| outputFormat: 'aac_adts', | ||
| audioEncoder: 'aac', | ||
| sampleRate: 44100 |
There was a problem hiding this comment.
This magic number is wrong. They probably have defaults, no?
| audiosRendered.delete(audioKey); | ||
| } | ||
|
|
||
| class AudioManagerClass { |
There was a problem hiding this comment.
Why did you refactor this? What's the benefit?
You also refactored, but didn't add any tests.
| import { useTheme } from '../theme'; | ||
| import { LOCAL_DOCUMENT_DIRECTORY, getFilename } from '../lib/methods/handleMediaDownload'; | ||
|
|
||
| const VideoContent = ({ |
There was a problem hiding this comment.
Different file with stories and tests
Rohit3523
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
Rohit3523
requested a deployment
to
approve_e2e_testing
GitHub Actions
Waiting
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/containers/MessageComposer/components/RecordAudio/RecordAudio.tsx (1)
92-105: 🎯 Functional Correctness | 🟠 MajorCapture a non-null recording URI before switching to review.
audioRecorder.uriandrecorderState.urlare nullable here, butsendAudiostill force-castsaudioRecorder.uriand the review UI falls back to''forAudioPlayer. Store a confirmed URI afterstop()and reuse that value for playback and upload, or bail out if no file path is available.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/containers/MessageComposer/components/RecordAudio/RecordAudio.tsx` around lines 92 - 105, The recording URI handling in RecordAudio is unsafe because goReview and sendAudio rely on nullable audioRecorder.uri/recorderState.url. Update the RecordAudio component to capture and store a confirmed non-null URI immediately after audioRecorder.stop() succeeds, then reuse that stored URI for both AudioPlayer playback and getInfoAsync/upload in sendAudio; if no URI is available, exit early instead of force-casting or falling back to an empty string.
🧹 Nitpick comments (1)
app/containers/MessageComposer/components/RecordAudio/RecordAudio.tsx (1)
47-47: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd explicit return types to the async handlers.
These local async functions should declare
Promise<void>to match the TypeScript guideline for explicit return types.Proposed fix
- async function doRecording() { + async function doRecording(): Promise<void> { ... - const cancelRecording = async () => { + const cancelRecording = async (): Promise<void> => { ... - const goReview = async () => { + const goReview = async (): Promise<void> => { ... - const sendAudio = async () => { + const sendAudio = async (): Promise<void> => {Also applies to: 82-82, 92-92, 101-101
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/containers/MessageComposer/components/RecordAudio/RecordAudio.tsx` at line 47, Add explicit Promise<void> return types to the local async handlers in RecordAudio, including doRecording and the other async functions referenced in this diff, so they match the TypeScript guideline for explicit return types. Update each async function signature to declare its return type directly on the function/method definition, keeping the existing behavior unchanged.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/containers/MessageComposer/components/RecordAudio/RecordAudio.tsx`:
- Line 38: The RecordAudio component has an unused retry ref that triggers
ESLint. Remove numberOfTriesRef from RecordAudio.tsx and clean up any related
dead code or imports so the component only keeps refs and state that are
actually read.
---
Outside diff comments:
In `@app/containers/MessageComposer/components/RecordAudio/RecordAudio.tsx`:
- Around line 92-105: The recording URI handling in RecordAudio is unsafe
because goReview and sendAudio rely on nullable
audioRecorder.uri/recorderState.url. Update the RecordAudio component to capture
and store a confirmed non-null URI immediately after audioRecorder.stop()
succeeds, then reuse that stored URI for both AudioPlayer playback and
getInfoAsync/upload in sendAudio; if no URI is available, exit early instead of
force-casting or falling back to an empty string.
---
Nitpick comments:
In `@app/containers/MessageComposer/components/RecordAudio/RecordAudio.tsx`:
- Line 47: Add explicit Promise<void> return types to the local async handlers
in RecordAudio, including doRecording and the other async functions referenced
in this diff, so they match the TypeScript guideline for explicit return types.
Update each async function signature to declare its return type directly on the
function/method definition, keeping the existing behavior unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5aa6be69-04d9-44d5-9796-e2957fc68bfc
📒 Files selected for processing (6)
app/containers/AudioPlayer/Seek.tsxapp/containers/AudioPlayer/index.tsxapp/containers/MessageComposer/components/Buttons/MicOrSendButton.tsxapp/containers/MessageComposer/components/RecordAudio/Duration.tsxapp/containers/MessageComposer/components/RecordAudio/RecordAudio.tsxapp/containers/Ringer/index.tsx
💤 Files with no reviewable changes (1)
- app/containers/AudioPlayer/Seek.tsx
🚧 Files skipped from review as they are similar to previous changes (4)
- app/containers/MessageComposer/components/RecordAudio/Duration.tsx
- app/containers/MessageComposer/components/Buttons/MicOrSendButton.tsx
- app/containers/Ringer/index.tsx
- app/containers/AudioPlayer/index.tsx
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: ESLint and Test / run-eslint-and-test
- GitHub Check: format
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{js,ts,jsx,tsx}: Use descriptive names for functions, variables, and classes that clearly convey their purpose
Write comments that explain the 'why' behind code decisions, not the 'what'
Keep functions small and focused on a single responsibility
Use const by default, let when reassignment is needed, and avoid var
Prefer async/await over .then() chains for handling asynchronous operations
Use explicit error handling with try/catch blocks for async operations
Avoid deeply nested code; refactor complex logic into helper functions
Files:
app/containers/MessageComposer/components/RecordAudio/RecordAudio.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use TypeScript for type safety; add explicit type annotations to function parameters and return types
Prefer interfaces over type aliases for defining object shapes in TypeScript
Use enums for sets of related constants rather than magic strings or numbersUse TypeScript with strict mode enabled
Files:
app/containers/MessageComposer/components/RecordAudio/RecordAudio.tsx
**/*.{js,jsx,ts,tsx,json}
📄 CodeRabbit inference engine (CLAUDE.md)
Use Prettier formatting with tabs, single quotes, 130 character line width, no trailing commas, and avoid arrow function parentheses
Files:
app/containers/MessageComposer/components/RecordAudio/RecordAudio.tsx
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Enforce ESLint rules from
@rocket.chat/eslint-configwith React, React Native, TypeScript, and Jest plugins
Files:
app/containers/MessageComposer/components/RecordAudio/RecordAudio.tsx
app/containers/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Place reusable UI components in 'app/containers/' directory
Files:
app/containers/MessageComposer/components/RecordAudio/RecordAudio.tsx
🧠 Learnings (2)
📚 Learning: 2026-04-30T17:07:51.020Z
Learnt from: diegolmello
Repo: RocketChat/Rocket.Chat.ReactNative PR: 7274
File: app/lib/services/voip/MediaCallEvents.ts:0-0
Timestamp: 2026-04-30T17:07:51.020Z
Learning: In this Rocket.Chat React Native codebase, the ESLint rule `no-void: error` is enforced. When you see a promise returned from an async call that is not awaited (a “floating promise”), do not silence it with the `void somePromise()` pattern. Instead, handle the promise explicitly by attaching `.catch(...)` (or otherwise awaiting/handling the error) so unhandled-rejection risks are addressed in a way that satisfies the existing ESLint configuration.
Applied to files:
app/containers/MessageComposer/components/RecordAudio/RecordAudio.tsx
📚 Learning: 2026-06-25T18:37:44.793Z
Learnt from: diegolmello
Repo: RocketChat/Rocket.Chat.ReactNative PR: 7434
File: app/views/ScreenLockConfigView.tsx:101-141
Timestamp: 2026-06-25T18:37:44.793Z
Learning: In the Rocket.Chat React Native codebase, do not treat passing an `async` function directly to an event prop in React/React Native UI components (e.g., `onPress={async () => ...}` in TSX) as a “floating promises” CI-blocking lint issue—this repo does not enable the ESLint `no-floating-promises` rule (while `no-void` is enforced). Only raise robustness follow-ups when there are genuinely unhandled promise paths (e.g., fire-and-forget calls like `save()` that return a Promise that is neither awaited nor handled), and prefer making sure failure paths are explicitly handled/reported rather than blocking on lint-style floating-promise concerns.
Applied to files:
app/containers/MessageComposer/components/RecordAudio/RecordAudio.tsx
🪛 ESLint
app/containers/MessageComposer/components/RecordAudio/RecordAudio.tsx
[error] 38-38: 'numberOfTriesRef' is assigned a value but never used.
(@typescript-eslint/no-unused-vars)
🔇 Additional comments (1)
app/containers/MessageComposer/components/RecordAudio/RecordAudio.tsx (1)
2-2: LGTM!Also applies to: 34-36, 39-39, 63-80
| const recorderState = useAudioRecorderState(audioRecorder); | ||
|
|
||
| const durationRef = useRef<IDurationRef>({} as IDurationRef); | ||
| const numberOfTriesRef = useRef(0); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔴 Critical | ⚡ Quick win
Remove the unused retry ref to unblock ESLint.
numberOfTriesRef is never read, and static analysis flags this as @typescript-eslint/no-unused-vars.
Proposed fix
- const numberOfTriesRef = useRef(0);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const numberOfTriesRef = useRef(0); |
🧰 Tools
🪛 ESLint
[error] 38-38: 'numberOfTriesRef' is assigned a value but never used.
(@typescript-eslint/no-unused-vars)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/containers/MessageComposer/components/RecordAudio/RecordAudio.tsx` at
line 38, The RecordAudio component has an unused retry ref that triggers ESLint.
Remove numberOfTriesRef from RecordAudio.tsx and clean up any related dead code
or imports so the component only keeps refs and state that are actually read.
Source: Linters/SAST tools
3 participants
Proposed changes
Depends on: #6875
Issue(s)
https://rocketchat.atlassian.net/browse/CORE-2035
How to test or reproduce
Screenshots
Types of changes
Checklist
Further comments
Summary by CodeRabbit
Refactor
Bug Fixes
Chores