Conversation
|
No actionable comments were generated in the recent review. 🎉 📝 WalkthroughWalkthroughAdds platform-specific Mapbox adapters, gates blob file saving to web-only, introduces ensureMicrophonePermission with timeout protections and updated connect flow in the LiveKit store, removes pre-warm phone permission from the bottom sheet, and updates tests and map initialization behavior accordingly. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as Sidebar (UI)
participant Store as LiveKit Store
participant Perm as Permissions API
participant Audio as AudioSession
participant Room as Room.connect
UI->>Store: ensureMicrophonePermission()
Store->>Perm: check/request microphone permission (platform-specific)
alt permission granted
Perm-->>Store: true
Store-->>UI: true
UI->>UI: open LiveKit bottom sheet
UI->>Store: connectToRoom()
Store->>Perm: verify permissions (withTimeout)
Store->>Audio: startAudioSession (withTimeout)
Audio-->>Store: started
Store->>Room: connect (withTimeout)
Room-->>Store: connected
Store-->>UI: connected
else permission denied / never_ask_again / error
Perm-->>Store: false / error
Store-->>UI: false (do not open modal, log/alert as needed)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/stores/app/__tests__/livekit-store.test.ts (1)
118-124:⚠️ Potential issue | 🟡 Minor
loggermock is missingwarn— will causeTypeErrorif any tested path callslogger.warn.The store uses
logger.warnin multiple places (e.g.,ensureMicrophonePermissiondenied path,requestAndroidPhonePermissionstimeout). While current tests don't hit those paths, adding a new test that does will fail cryptically.Proposed fix
jest.mock('../../../lib/logging', () => ({ logger: { info: jest.fn(), error: jest.fn(), debug: jest.fn(), + warn: jest.fn(), }, }));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/stores/app/__tests__/livekit-store.test.ts` around lines 118 - 124, The mocked logger in the tests is missing the warn method which will cause a TypeError if code paths like ensureMicrophonePermission or requestAndroidPhonePermissions call logger.warn; update the jest.mock for logger to include warn: jest.fn() so the mock implements logger.warn (keeping other methods info/error/debug intact) to prevent failures when those paths are exercised.
🧹 Nitpick comments (6)
src/components/sidebar/__tests__/unit-sidebar.test.tsx (1)
155-179: Sameawait fireEvent.pressissue — usewaitForinstead.Same concern as noted in
unit-sidebar-simplified.test.tsx:fireEvent.pressis synchronous, soawaiton it doesn't wait for the asynchandleOpenLiveKit. UsewaitForto properly assert after the async handler settles.Proposed fix
+import { render, screen, fireEvent, waitFor } from '@testing-library/react-native'; ... it('handles call button press', async () => { ... const callButton = screen.getByTestId('call-button'); - await fireEvent.press(callButton); + fireEvent.press(callButton); - expect(mockEnsureMicrophonePermission).toHaveBeenCalled(); - expect(mockSetIsBottomSheetVisible).toHaveBeenCalledWith(true); + await waitFor(() => { + expect(mockEnsureMicrophonePermission).toHaveBeenCalled(); + expect(mockSetIsBottomSheetVisible).toHaveBeenCalledWith(true); + }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/sidebar/__tests__/unit-sidebar.test.tsx` around lines 155 - 179, The test currently awaits fireEvent.press (which is synchronous) so assertions may run before the async handler completes; change the test to call fireEvent.press(callButton) without await and then wrap the assertions in a waitFor to wait for the async handleOpenLiveKit flow to settle — specifically ensure mockEnsureMicrophonePermission and mockSetIsBottomSheetVisible(true) are asserted inside waitFor; reference the test's mock functions mockEnsureMicrophonePermission and mockSetIsBottomSheetVisible and the call button obtained via screen.getByTestId('call-button').src/components/sidebar/__tests__/unit-sidebar-simplified.test.tsx (1)
174-182:await fireEvent.press(...)does not actually await the async event handler.
fireEvent.pressis synchronous and returnsvoid. Theawaitonly awaitsundefined, which is a no-op. The test passes becausemockEnsureMicrophonePermissionresolves via a microtask that happens to flush before the assertion, but this is fragile. UsingwaitForfrom@testing-library/react-nativeis the idiomatic approach:Proposed fix
+import { render, screen, fireEvent, waitFor } from '@testing-library/react-native'; ... it('opens LiveKit when call button is pressed', async () => { render(<SidebarUnitCard {...defaultProps} />); const callButton = screen.getByTestId('call-button'); - await fireEvent.press(callButton); + fireEvent.press(callButton); - expect(mockEnsureMicrophonePermission).toHaveBeenCalled(); - expect(mockSetIsBottomSheetVisible).toHaveBeenCalledWith(true); + await waitFor(() => { + expect(mockEnsureMicrophonePermission).toHaveBeenCalled(); + expect(mockSetIsBottomSheetVisible).toHaveBeenCalledWith(true); + }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/sidebar/__tests__/unit-sidebar-simplified.test.tsx` around lines 174 - 182, The test incorrectly uses await on the synchronous fireEvent.press; update the test for SidebarUnitCard to call fireEvent.press(callButton) without awaiting and then use waitFor from `@testing-library/react-native` to assert side effects: wait for mockEnsureMicrophonePermission to have been called and for mockSetIsBottomSheetVisible to have been calledWith(true). This ensures the async handlers complete before assertions and stabilizes the test.src/components/sidebar/unit-sidebar.tsx (1)
39-45: Permission denial result is intentionally ignored, but an unhandled rejection could occur if the promise rejects unexpectedly.The
await ensureMicrophonePermission()doesn't check the return value, so the bottom sheet opens even if permission is denied. Per the store comments, this is by design. However, if the promise were to reject (rather than returningfalse), the error would be unhandled. While the current implementation's try/catch should prevent this, a defensive wrapper would be safer:Proposed defensive wrapper
const handleOpenLiveKit = async () => { - await ensureMicrophonePermission(); + try { + await ensureMicrophonePermission(); + } catch { + // Permission check failed unexpectedly; still open the voice UI + // so the user can see available rooms. + } setIsBottomSheetVisible(true); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/sidebar/unit-sidebar.tsx` around lines 39 - 45, Wrap the ensureMicrophonePermission() call in a defensive try/catch inside handleOpenLiveKit so any rejection is swallowed (log optionally) but does not create an unhandled promise rejection; keep the existing behavior of always calling setIsBottomSheetVisible(true) regardless of the permission result. Update the handleOpenLiveKit function to await ensureMicrophonePermission() inside try { await ensureMicrophonePermission(); } catch (err) { /* ignore or process/log err */ } then call setIsBottomSheetVisible(true).src/stores/app/livekit-store.ts (1)
368-427:requestPermissionsandensureMicrophonePermissioncontain near-identical logic.Both methods perform the same check-then-request pattern for Android (
PermissionsAndroid.check→PermissionsAndroid.request) and iOS (getRecordingPermissionsAsync→requestRecordingPermissionsAsync). The only differences are log messages and severity levels (errorvswarn).Consider extracting the shared platform permission logic into a private helper (e.g.,
checkOrRequestMicPermission) and having both methods delegate to it. This would reduce ~50 lines of duplication and ensure future permission-flow changes are applied consistently.Also applies to: 892-943
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/stores/app/livekit-store.ts` around lines 368 - 427, requestPermissions and ensureMicrophonePermission duplicate the same platform permission flow; extract that shared logic into a new private helper (e.g., checkOrRequestMicPermission) that implements the Android path (PermissionsAndroid.check → PermissionsAndroid.request) and the iOS path (getRecordingPermissionsAsync → requestRecordingPermissionsAsync) and returns a boolean success plus optional detail. Update requestPermissions and ensureMicrophonePermission to call checkOrRequestMicPermission and handle only the differing logging/severity and any call-site specifics (e.g., phone-state handling in requestPermissions), or accept an optional parameter to control log severity/context, so all permission behavior is centralized and duplication removed.src/stores/app/__tests__/livekit-store.test.ts (1)
153-168: No tests for the newensureMicrophonePermissionstore action.The PR introduces
ensureMicrophonePermissionas a key new public API on the LiveKit store, but this test file only coversrequestPermissions. Consider adding tests for:
- Android: permission already granted (short-circuit)
- Android: permission granted after request
- Android: permission denied
- iOS: equivalent paths
- Error handling path
These would exercise the same platform-specific logic but through the new entry point, ensuring both code paths remain correct.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/stores/app/__tests__/livekit-store.test.ts` around lines 153 - 168, Add unit tests for the new public store action ensureMicrophonePermission to mirror the existing requestPermissions coverage: write tests covering Android when permission is already granted (mockPermissionsAndroidCheck returns GRANTED and ensureMicrophonePermission short-circuits), Android when permission is granted after calling request (mockPermissionsAndroidCheck returns DENIED then mockPermissionsAndroidRequest returns GRANTED), Android when request is denied (mockPermissionsAndroidRequest returns DENIED), iOS success/failure paths (override Platform.OS to 'ios' and assert the iOS-specific branch), and an error-handling test that simulates the underlying permission check/request throwing an error to assert the action propagates or handles it; reuse/reset mockPermissionsAndroidCheck, mockPermissionsAndroidRequest and useLiveKitStore.setState in beforeEach as in the existing tests and assert expected store state changes and return values from ensureMicrophonePermission.src/components/maps/mapbox.native.ts (1)
31-47:MapboxExportsre-readsMapbox.*instead of the already-defined named consts.The web sibling (
mapbox.web.ts) uses shorthand references to its named consts insideMapboxExports(e.g.,MapView,notMapView: MapboxWeb.MapView). This file does the opposite, duplicating allMapbox.*reads. Using the already-defined consts would be more consistent and DRY.♻️ Proposed refactor
const MapboxExports = { - MapView: Mapbox.MapView, - Camera: Mapbox.Camera, - PointAnnotation: Mapbox.PointAnnotation, - UserLocation: Mapbox.UserLocation, - MarkerView: Mapbox.MarkerView, - ShapeSource: Mapbox.ShapeSource, - SymbolLayer: Mapbox.SymbolLayer, - CircleLayer: Mapbox.CircleLayer, - LineLayer: Mapbox.LineLayer, - FillLayer: Mapbox.FillLayer, - Images: Mapbox.Images, - Callout: Mapbox.Callout, - StyleURL: Mapbox.StyleURL, - UserTrackingMode: Mapbox.UserTrackingMode, - setAccessToken: Mapbox.setAccessToken, + MapView, + Camera, + PointAnnotation, + UserLocation, + MarkerView, + ShapeSource, + SymbolLayer, + CircleLayer, + LineLayer, + FillLayer, + Images, + Callout, + StyleURL, + UserTrackingMode, + setAccessToken, };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/maps/mapbox.native.ts` around lines 31 - 47, MapboxExports is repeatedly reading properties off Mapbox (Mapbox.MapView, Mapbox.Camera, etc.) instead of using the already-defined local constants (e.g., MapView, Camera, PointAnnotation, UserLocation, MarkerView, ShapeSource, SymbolLayer, CircleLayer, LineLayer, FillLayer, Images, Callout, StyleURL, UserTrackingMode, setAccessToken). Fix by replacing each Mapbox.* entry with the corresponding local identifier (use shorthand property names like MapView, Camera, etc.) inside the MapboxExports object so it mirrors mapbox.web.ts and avoids redundant Mapbox.* property accesses.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/api/calls/callFiles.ts`:
- Around line 81-95: The saveBlobAsFile function currently returns void and
silently no-ops on native, so change its signature to return a boolean
indicating success; in saveBlobAsFile(blob, fileName) return true after
completing the web flow (window.URL.createObjectURL, link.click,
revokeObjectURL) and return false on the native branch (Platform.OS !== 'web')
so callers can detect unsupported platforms; update the function's TypeScript
signature accordingly and ensure any future callers check the boolean result to
show appropriate UI or fallback to expo-file-system/expo-sharing.
In `@src/components/maps/static-map.tsx`:
- Line 48: The spread of web-only props initialCenter and initialZoom into the
MapView in StaticMap (the JSX line using Platform.OS === 'web') causes a
TypeScript error because the native MapView type doesn't include those props;
remove that conditional spread from src/components/maps/static-map.tsx and
instead implement web-specific initialization inside map-view.web.tsx by reading
the Camera component's centerCoordinate and zoomLevel (or via a ref/context) to
set the initial camera on first render; update map-view.web.tsx to consume
Camera props (centerCoordinate, zoomLevel) and apply them to the underlying web
MapView initialization so StaticMap's JSX remains platform-agnostic and
type-safe.
---
Outside diff comments:
In `@src/stores/app/__tests__/livekit-store.test.ts`:
- Around line 118-124: The mocked logger in the tests is missing the warn method
which will cause a TypeError if code paths like ensureMicrophonePermission or
requestAndroidPhonePermissions call logger.warn; update the jest.mock for logger
to include warn: jest.fn() so the mock implements logger.warn (keeping other
methods info/error/debug intact) to prevent failures when those paths are
exercised.
---
Nitpick comments:
In `@src/components/maps/mapbox.native.ts`:
- Around line 31-47: MapboxExports is repeatedly reading properties off Mapbox
(Mapbox.MapView, Mapbox.Camera, etc.) instead of using the already-defined local
constants (e.g., MapView, Camera, PointAnnotation, UserLocation, MarkerView,
ShapeSource, SymbolLayer, CircleLayer, LineLayer, FillLayer, Images, Callout,
StyleURL, UserTrackingMode, setAccessToken). Fix by replacing each Mapbox.*
entry with the corresponding local identifier (use shorthand property names like
MapView, Camera, etc.) inside the MapboxExports object so it mirrors
mapbox.web.ts and avoids redundant Mapbox.* property accesses.
In `@src/components/sidebar/__tests__/unit-sidebar-simplified.test.tsx`:
- Around line 174-182: The test incorrectly uses await on the synchronous
fireEvent.press; update the test for SidebarUnitCard to call
fireEvent.press(callButton) without awaiting and then use waitFor from
`@testing-library/react-native` to assert side effects: wait for
mockEnsureMicrophonePermission to have been called and for
mockSetIsBottomSheetVisible to have been calledWith(true). This ensures the
async handlers complete before assertions and stabilizes the test.
In `@src/components/sidebar/__tests__/unit-sidebar.test.tsx`:
- Around line 155-179: The test currently awaits fireEvent.press (which is
synchronous) so assertions may run before the async handler completes; change
the test to call fireEvent.press(callButton) without await and then wrap the
assertions in a waitFor to wait for the async handleOpenLiveKit flow to settle —
specifically ensure mockEnsureMicrophonePermission and
mockSetIsBottomSheetVisible(true) are asserted inside waitFor; reference the
test's mock functions mockEnsureMicrophonePermission and
mockSetIsBottomSheetVisible and the call button obtained via
screen.getByTestId('call-button').
In `@src/components/sidebar/unit-sidebar.tsx`:
- Around line 39-45: Wrap the ensureMicrophonePermission() call in a defensive
try/catch inside handleOpenLiveKit so any rejection is swallowed (log
optionally) but does not create an unhandled promise rejection; keep the
existing behavior of always calling setIsBottomSheetVisible(true) regardless of
the permission result. Update the handleOpenLiveKit function to await
ensureMicrophonePermission() inside try { await ensureMicrophonePermission(); }
catch (err) { /* ignore or process/log err */ } then call
setIsBottomSheetVisible(true).
In `@src/stores/app/__tests__/livekit-store.test.ts`:
- Around line 153-168: Add unit tests for the new public store action
ensureMicrophonePermission to mirror the existing requestPermissions coverage:
write tests covering Android when permission is already granted
(mockPermissionsAndroidCheck returns GRANTED and ensureMicrophonePermission
short-circuits), Android when permission is granted after calling request
(mockPermissionsAndroidCheck returns DENIED then mockPermissionsAndroidRequest
returns GRANTED), Android when request is denied (mockPermissionsAndroidRequest
returns DENIED), iOS success/failure paths (override Platform.OS to 'ios' and
assert the iOS-specific branch), and an error-handling test that simulates the
underlying permission check/request throwing an error to assert the action
propagates or handles it; reuse/reset mockPermissionsAndroidCheck,
mockPermissionsAndroidRequest and useLiveKitStore.setState in beforeEach as in
the existing tests and assert expected store state changes and return values
from ensureMicrophonePermission.
In `@src/stores/app/livekit-store.ts`:
- Around line 368-427: requestPermissions and ensureMicrophonePermission
duplicate the same platform permission flow; extract that shared logic into a
new private helper (e.g., checkOrRequestMicPermission) that implements the
Android path (PermissionsAndroid.check → PermissionsAndroid.request) and the iOS
path (getRecordingPermissionsAsync → requestRecordingPermissionsAsync) and
returns a boolean success plus optional detail. Update requestPermissions and
ensureMicrophonePermission to call checkOrRequestMicPermission and handle only
the differing logging/severity and any call-site specifics (e.g., phone-state
handling in requestPermissions), or accept an optional parameter to control log
severity/context, so all permission behavior is centralized and duplication
removed.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/api/calls/callFiles.ts (1)
81-99:saveBlobAsFilenow correctly signals callers — previous review concern resolved.The boolean return type and
Platform.OS !== 'web'guard are implemented cleanly. One optional hardening: callinglink.click()on an element that hasn't been appended to the document can silently fail in older Safari / some iOS WebView environments. The safer cross-browser pattern is:🔧 Optional: attach link to DOM before clicking
const url = window.URL.createObjectURL(blob); const link = document.createElement('a'); link.href = url; link.download = fileName; - link.click(); + document.body.appendChild(link); + link.click(); + document.body.removeChild(link); // Clean up window.URL.revokeObjectURL(url);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/calls/callFiles.ts` around lines 81 - 99, saveBlobAsFile currently calls link.click() without attaching the anchor to the DOM which can fail on older Safari / some iOS WebViews; update the saveBlobAsFile function to append the created anchor element to document.body before calling link.click() and then remove it afterwards (ensure window.URL.revokeObjectURL(url) still runs during cleanup), so the code path around link (the link variable and its click invocation inside saveBlobAsFile) works more reliably across browsers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/maps/map-view.web.tsx`:
- Around line 384-392: The initial programmatic camera set uses map.jumpTo but
omits the eventData and optional heading/pitch, causing the moveend handler
(which checks e._programmatic) to misclassify it as a user action and trigger
onCameraChanged with isUserInteraction: true; update the initial map.jumpTo call
(the one using centerCoordinate and zoomLevel) to include the same eventData
object used by other programmatic moves ({ _programmatic: true }) and include
the destructured heading and pitch props so the first render matches subsequent
flyTo/easeTo calls, ensuring moveend sees _programmatic and prevents false user
interaction reporting.
---
Nitpick comments:
In `@src/api/calls/callFiles.ts`:
- Around line 81-99: saveBlobAsFile currently calls link.click() without
attaching the anchor to the DOM which can fail on older Safari / some iOS
WebViews; update the saveBlobAsFile function to append the created anchor
element to document.body before calling link.click() and then remove it
afterwards (ensure window.URL.revokeObjectURL(url) still runs during cleanup),
so the code path around link (the link variable and its click invocation inside
saveBlobAsFile) works more reliably across browsers.
Member
Author
|
Approve |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes
Behavior Changes
Tests