Conversation
This comment has been minimized.
This comment has been minimized.
|
Warning Review limit reached
Next review available in: 32 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe release-note extraction script now replaces standalone ChangesiOS microphone permission flow
Release note normalization
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant App
participant LiveKitStore
participant ExpoAudio
participant WebRTC
App->>LiveKitStore: requestPermissions()
LiveKitStore->>ExpoAudio: getRecordingPermissionsAsync()
ExpoAudio-->>LiveKitStore: microphone permission status
LiveKitStore-->>App: continue without interactive iOS request
WebRTC->>WebRTC: prompt when publishing microphone track
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@scripts/extract-release-notes.sh`:
- Around line 49-50: Update the notes substitution in the release-notes script
to replace standalone “PR” using a BSD/GNU-compatible boundary expression rather
than sed’s \b extension. Preserve matches at the beginning and end of lines and
avoid replacing “PR” within larger words.
In `@src/stores/app/__tests__/livekit-store.test.ts`:
- Around line 263-268: Replace the unsafe as any cast in the permission-response
mocks used by the tests around mockGetRecordingPermissionsAsync with a correctly
typed shared response object or typed helper such as isGranted. Reuse that typed
shape in both permission tests while preserving their existing
granted-permission behavior.
In `@src/stores/app/livekit-store.ts`:
- Around line 407-414: Update both requestPermissions and
ensureMicrophonePermission in src/stores/app/livekit-store.ts at lines 407-414
and 965-972 to return false when microphone permission is ungranted and
canAskAgain is false, while retaining true for promptable ungranted states.
Update the iOS permission test in src/stores/app/__tests__/livekit-store.test.ts
at lines 278-296 to expect false for the permanently denied state.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 86d157e1-0563-47a3-9f68-daad7ccefda7
📒 Files selected for processing (3)
scripts/extract-release-notes.shsrc/stores/app/__tests__/livekit-store.test.tssrc/stores/app/livekit-store.ts
| # Replace standalone "PR" with "Release" for release notes text | ||
| notes="$(printf '%s\n' "$notes" | sed -E 's/\bPR\b/Release/g')" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/sh
set -eu
input='PR PRs XPR PR.'
output="$(printf '%s\n' "$input" | sed -E 's/\bPR\b/Release/g')"
printf 'sed output: %s\n' "$output"Repository: Resgrid/Unit
Length of output: 187
🏁 Script executed:
#!/bin/sh
set -eu
printf 'sed implementation/version:\n'
sed --version 2>&1 || true
sed -E 'x' /dev/null 2>&1 | sed -n '1p' || true
printf '\nCurrent sed behavior for word boundaries:\n'
input='PR PRs XPR PR'
output="$(printf '%s\n' "$input" | sed -E 's/\bPR\b/Release/g')"
printf '%s\n' "$output"
printf '\nGNU sed version string:\n'
sed --version 2>&1 | sed -n '1p'Repository: Resgrid/Unit
Length of output: 903
🏁 Script executed:
#!/bin/sh
set -eu
echo "tracked workflow/tooling files:"
git ls-files | rg '(^\.github/workflows/|scripts/|README\.md|package\.json|justfile|\.circleci|\.github/)' | head -200
echo
echo "search for extract-release-notes usage:"
rg -n "extract-release-notes|extract-release-notes\.sh|RELEASE.*NOTES|release.*notes" .github scripts README.md package.json 2>/dev/null | head -200
echo
echo "script excerpt:"
cat -n scripts/extract-release-notes.sh | sed -n '1,120p'Repository: Resgrid/Unit
Length of output: 8225
🏁 Script executed:
#!/bin/sh
set -eu
echo "Workflow file size:"
wc -l .github/workflows/react-native-cicd.yml
echo
echo "Relevant workflow slices around script invocations:"
sed -n '300,385p' .github/workflows/react-native-cicd.yml
echo "---"
sed -n '625,700p' .github/workflows/react-native-cicd.yml
echo
echo "Runner labels declared in repository:"
rg -n "runs-on:\s*\\$\\{\\{.*runner|runs-on:|ubuntu|macos|windows|self-hosted" .github/workflows/react-native-cicd.yml package.json README.mdRepository: Resgrid/Unit
Length of output: 7251
Use a portable word-boundary replacement.
This script runs on both Ubuntu and macOS runners, and the macOS runner uses BSD sed, where \b is not guaranteed to match “PR” at string boundaries. Replace the GNU-specific sed -E 's/\bPR\b/Release/g' with a portable boundary expression before publishing release notes.
Proposed portable replacement
- notes="$(printf '%s\n' "$notes" | sed -E 's/\bPR\b/Release/g')"
+ notes="$(printf '%s\n' "$notes" | sed -E 's/(^|[^[:alnum:]_])PR([^[:alnum:]_]|$)/\1Release\2/g')"📝 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.
| # Replace standalone "PR" with "Release" for release notes text | |
| notes="$(printf '%s\n' "$notes" | sed -E 's/\bPR\b/Release/g')" | |
| # Replace standalone "PR" with "Release" for release notes text | |
| notes="$(printf '%s\n' "$notes" | sed -E 's/(^|[^[:alnum:]_])PR([^[:alnum:]_]|$)/\1Release\2/g')" |
🤖 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 `@scripts/extract-release-notes.sh` around lines 49 - 50, Update the notes
substitution in the release-notes script to replace standalone “PR” using a
BSD/GNU-compatible boundary expression rather than sed’s \b extension. Preserve
matches at the beginning and end of lines and avoid replacing “PR” within larger
words.
| mockGetRecordingPermissionsAsync.mockResolvedValueOnce({ | ||
| granted: false, | ||
| canAskAgain: true, | ||
| expires: 'never', | ||
| status: 'undetermined', | ||
| } as any); | ||
|
|
||
| // Mock permission request - granted | ||
| mockRequestRecordingPermissionsAsync.mockResolvedValueOnce({ | ||
| granted: true, | ||
| canAskAgain: true, | ||
| expires: 'never', | ||
| status: 'granted', | ||
| } as any); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n -g '!node_modules/**' '"typescript"|typescript' package.json tsconfig*.json 2>/dev/null || trueRepository: Resgrid/Unit
Length of output: 388
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate file =="
fd -a 'livekit-store.test.ts' . || true
echo "== relevant lines =="
if [ -f src/stores/app/__tests__/livekit-store.test.ts ]; then
nl -ba src/stores/app/__tests__/livekit-store.test.ts | sed -n '240,300p'
fi
echo "== imports around file =="
if [ -f src/stores/app/__tests__/livekit-store.test.ts ]; then
nl -ba src/stores/app/__tests__/livekit-store.test.ts | sed -n '1,80p'
fi
echo "== usages of getRecordingPermissionsAsync in tests/source =="
rg -n "getRecordingPermissionsAsync|mockGetRecordingPermissionsAsync|MockedFn|As any|as any" src -g '!node_modules/**' || trueRepository: Resgrid/Unit
Length of output: 295
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="src/stores/app/__tests__/livekit-store.test.ts"
echo "== imports header =="
awk 'NR>=1 && NR<=90 {printf "%6d %s\n", NR, $0}' "$file"
echo "== relevant lines =="
awk 'NR>=240 && NR<=300 {printf "%6d %s\n", NR, $0}' "$file"
echo "== imports/permissions usages =="
rg -n "getRecordingPermissionsAsync|mockGetRecordingPermissionsAsync|mockGetCameraPermissionsAsync|as any" "$file" || trueRepository: Resgrid/Unit
Length of output: 7024
Replace the unsafe permission-response casts.
as any bypasses the mocked API contract in both permission tests. Use one typed response object, e.g. an isGranted helper, or infer the response shape without casting to any.
🤖 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 `@src/stores/app/__tests__/livekit-store.test.ts` around lines 263 - 268,
Replace the unsafe as any cast in the permission-response mocks used by the
tests around mockGetRecordingPermissionsAsync with a correctly typed shared
response object or typed helper such as isGranted. Reuse that typed shape in
both permission tests while preserving their existing granted-permission
behavior.
Source: Coding guidelines
| if (!micPermission.granted) { | ||
| const result = await requestRecordingPermissionsAsync(); | ||
| if (!result.granted) { | ||
| logger.error({ | ||
| message: 'Microphone permission not granted', | ||
| context: { platform: Platform.OS }, | ||
| }); | ||
| return false; | ||
| } | ||
| logger.info({ | ||
| message: 'Microphone permission not yet granted - WebRTC will prompt on publish', | ||
| context: { platform: Platform.OS }, | ||
| }); | ||
| } | ||
|
|
||
| logger.info({ | ||
| message: 'Microphone permission granted successfully', | ||
| context: { platform: Platform.OS }, | ||
| }); | ||
| return true; |
There was a problem hiding this comment.
Silent WebRTC publish failure occurs because requestPermissions returns true unconditionally on iOS (line 414), causing connectToRoom (line 489) to bypass the 'Open Settings' alert and abort logic (lines 495-506) when microphone permission is permanently denied (canAskAgain: false, status: 'denied'). Return false for permanently denied permissions to restore denied-permission guidance while preserving the deadlock fix that avoids requestRecordingPermissionsAsync.
const micPermission = await getRecordingPermissionsAsync();
if (!micPermission.granted) {
if (micPermission.canAskAgain) {
// Undetermined — WebRTC will trigger the native prompt on publish
logger.info({
message: 'Microphone permission not yet granted - WebRTC will prompt on publish',
context: { platform: Platform.OS },
});
return true;
}
// Permanently denied — iOS will not re-prompt; direct user to Settings
logger.error({
message: 'Microphone permission denied',
context: { platform: Platform.OS },
});
return false;
}
return true;Prompt for LLM
File src/stores/app/livekit-store.ts:
Line 407 to 414:
Silent WebRTC publish failure occurs because `requestPermissions` returns `true` unconditionally on iOS (line 414), causing `connectToRoom` (line 489) to bypass the 'Open Settings' alert and abort logic (lines 495-506) when microphone permission is permanently denied (`canAskAgain: false`, `status: 'denied'`). Return `false` for permanently denied permissions to restore denied-permission guidance while preserving the deadlock fix that avoids `requestRecordingPermissionsAsync`.
Suggested Code:
const micPermission = await getRecordingPermissionsAsync();
if (!micPermission.granted) {
if (micPermission.canAskAgain) {
// Undetermined — WebRTC will trigger the native prompt on publish
logger.info({
message: 'Microphone permission not yet granted - WebRTC will prompt on publish',
context: { platform: Platform.OS },
});
return true;
}
// Permanently denied — iOS will not re-prompt; direct user to Settings
logger.error({
message: 'Microphone permission denied',
context: { platform: Platform.OS },
});
return false;
}
return true;
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
Code Review Completed! 🔥The code review was successfully completed based on your current configurations. Kody Guide: Usage and ConfigurationInteracting with Kody
Current Kody ConfigurationReview OptionsThe following review options are enabled or disabled:
|
| if (!micPermission.canAskAgain) { | ||
| logger.warn({ | ||
| message: 'Microphone permission permanently denied', | ||
| context: { platform: Platform.OS }, | ||
| }); | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Duplicated permission-permanently-denied check-and-log blocks at lines 408–414 and 974–980 in src/stores/app/livekit-store.ts force error-prone, lock-step maintenance for any changes to the message, log level, or return value. Extract a helper such as isMicPermissionPermanentlyDenied(permission) or handlePermanentlyDenied(platform) to encapsulate the canAskAgain check, warning log, and boolean return.
Kody rule violation: Extract duplicated logic into functions
Prompt for LLM
File src/stores/app/livekit-store.ts:
Line 408 to 414:
Duplicated permission-permanently-denied check-and-log blocks at lines 408–414 and 974–980 in `src/stores/app/livekit-store.ts` force error-prone, lock-step maintenance for any changes to the message, log level, or return value. Extract a helper such as `isMicPermissionPermanentlyDenied(permission)` or `handlePermanentlyDenied(platform)` to encapsulate the `canAskAgain` check, warning log, and boolean return.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
|
Approve |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
2 participants
Description
This PR fixes an iOS voice/audio issue that caused the app to freeze when requesting microphone permissions.
The Problem
On iOS, calling
expo-audio'srequestRecordingPermissionsAsync()activatesAVAudioSessionin record mode. This deadlocks againstexpo-av's already-active audio session, causing the app to hang.The Fix
The
livekit-storeno longer callsrequestRecordingPermissionsAsyncon iOS. Instead, it performs only a non-activating permission status check viagetRecordingPermissionsAsync(). When microphone permission hasn't been granted yet, the store returnstrueand allows WebRTC to trigger the native iOS microphone prompt itself when an audio track is published. This change was applied to both the permission request flow and the pre-voice-UI permission check.The corresponding unit tests were updated to verify that
requestRecordingPermissionsAsyncis never invoked on iOS, regardless of current permission status.Additionally, a minor release notes script change replaces standalone "PR" text with "Release" in generated release notes.