Conversation
📝 WalkthroughWalkthroughThis PR introduces Jest mocks for Mapbox GL and CSS styles, refactors platform-specific Mapbox component imports from dynamic requires to direct static imports, adds Android phone permission pre-warming in LiveKit connection flows, updates the Sidebar component to accept an onClose prop, and converts UserTrackingMode to a TypeScript enum with modified string values. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/livekit/livekit-bottom-sheet.tsx (1)
257-264:⚠️ Potential issue | 🟠 MajorDuplicate connecting state: the
ActivityIndicatorinrenderRoomSelectis unreachable dead code.
getViewContent()(line 258) checksisConnectingfirst and returns a plain text "connecting" view before theswitchstatement can ever callrenderRoomSelect(). Therefore, the newActivityIndicator-based loading state at lines 170–175 insiderenderRoomSelectwill never render.You likely want to remove one of the two. The
renderRoomSelectversion withActivityIndicatorlooks like the intended UX — consider removing the early return ingetViewContent:Suggested fix
const getViewContent = () => { - if (isConnecting) { - return ( - <View style={styles.content} testID="connecting-view"> - <Text className="text-center text-lg font-medium">{t('livekit.connecting')}</Text> - </View> - ); - } - switch (currentView) { case BottomSheetView.ROOM_SELECT: return renderRoomSelect();Also applies to: 168-175
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/livekit/livekit-bottom-sheet.tsx` around lines 257 - 264, The early-return in getViewContent() that checks isConnecting causes renderRoomSelect()'s ActivityIndicator to be dead code; remove the isConnecting branch from getViewContent() (or at minimum stop returning the plain text connecting view there) so the switch/case can call renderRoomSelect() and render its ActivityIndicator, ensuring the connecting state is handled only inside renderRoomSelect(); update any testID or accessibility labels accordingly (refer to getViewContent, renderRoomSelect, isConnecting, and the ActivityIndicator block).
🧹 Nitpick comments (4)
src/components/sidebar/sidebar-content.tsx (1)
33-36:onCloseis invoked synchronously before navigation — consider whether this is the intended UX
onClose?.()(which resolves tosetIsOpen(false)) schedules a state update and triggers the drawer's close animation asynchronously, whilerouter.push('/settings')fires immediately afterward in the same call frame. The navigation will start before the drawer finishes closing, which can produce a visual overlap of the sliding-close animation and the route transition.If this is acceptable, no change is needed. If a fully-closed drawer is desired before navigation begins, consider awaiting the animation:
💡 Optional: delay navigation until after close
- const handleNavigateToSettings = () => { - onClose?.(); - router.push('/settings'); - }; + const handleNavigateToSettings = () => { + if (onClose) { + onClose(); + // Allow the drawer close animation to complete before navigating + setTimeout(() => router.push('/settings'), 300); + } else { + router.push('/settings'); + } + };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/sidebar/sidebar-content.tsx` around lines 33 - 36, handleNavigateToSettings currently calls onClose?.() (which flips setIsOpen(false)) and immediately calls router.push('/settings'), causing navigation to start before the drawer finish-close animation; update the flow so navigation happens after the drawer has fully closed by either: (A) changing onClose to return a Promise that resolves when the close animation completes and awaiting onClose() inside handleNavigateToSettings before calling router.push, or (B) adding a small timed delay in handleNavigateToSettings (matching the drawer animation duration) before calling router.push; reference the handleNavigateToSettings function and the onClose/ router.push calls when implementing the change.src/stores/app/livekit-store.ts (1)
353-355: Nit: therequestPermissionscomment referencesconnectToRoombut it's actually called from the bottom sheet'srequestAndroidPhonePermissions.The comment says "called at the top of
connectToRoom" butrequestAndroidPhonePermissionsis fire-and-forget from the bottom sheet, not from withinconnectToRoom. Consider:- // Microphone only — phone-state permissions are handled separately by - // `requestAndroidPhonePermissions`, called at the top of `connectToRoom`. + // Microphone only — phone-state permissions are handled separately by + // `requestAndroidPhonePermissions`, called when the bottom sheet opens.🤖 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 353 - 355, Update the comment above requestPermissions to correctly state where phone-state permissions are requested: change the reference from "called at the top of connectToRoom" to indicate that phone-state permissions are handled separately by requestAndroidPhonePermissions, which is invoked from the bottom sheet (not from connectToRoom); keep the note that requestPermissions only requests microphone permission and mention the actual caller requestAndroidPhonePermissions to avoid future confusion.src/services/bluetooth-audio.service.ts (1)
10-22: Misleading comment: saysimport()but usesrequire().The comments on lines 13 and 19 say "Using import() for lazy loading" but the actual code uses synchronous
require(). While both achieve lazy loading for circular dependency breaking, the comment should match the implementation.Also, the
anyreturn types defeat type safety at every call site. Consider at minimum returning a typed store interface, even if loosely.Suggested comment fix
// Lazy getters to avoid circular dependencies with livekit-store and useLiveKitCallStore // eslint-disable-next-line `@typescript-eslint/no-explicit-any` const getLiveKitCallStore = (): any => { - // Using import() for lazy loading to avoid circular dependencies + // Using require() for lazy loading to avoid circular dependencies // eslint-disable-next-line `@typescript-eslint/no-require-imports` return require('@/features/livekit-call/store/useLiveKitCallStore').useLiveKitCallStore; }; // eslint-disable-next-line `@typescript-eslint/no-explicit-any` const getLiveKitStore = (): any => { - // Using import() for lazy loading to avoid circular dependencies + // Using require() for lazy loading to avoid circular dependencies // eslint-disable-next-line `@typescript-eslint/no-require-imports` return require('@/stores/app/livekit-store').useLiveKitStore; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/bluetooth-audio.service.ts` around lines 10 - 22, The comments falsely state "Using import() for lazy loading" while the functions getLiveKitCallStore and getLiveKitStore use synchronous require(); either update the comments to mention require() or change the implementation to use dynamic import() for real ES module lazy loading, and replace the loose ": any" return types with the actual store return types exported by the modules (e.g., the type returned by useLiveKitCallStore and useLiveKitStore) by importing those types with "import type" so callers retain type safety.src/components/maps/mapbox.ts (1)
38-55: Use ES shorthand property syntax in theMapboxaggregator.All ten explicit key-value pairs where
key === valueidentifier can be collapsed to shorthand, which is more idiomatic TypeScript/JavaScript.♻️ Proposed refactor
const Mapbox = { ...MapboxImpl, - MapView: MapView, - Camera: Camera, - PointAnnotation: PointAnnotation, - UserLocation: UserLocation, - MarkerView: MarkerView, - ShapeSource: ShapeSource, - SymbolLayer: SymbolLayer, - CircleLayer: CircleLayer, - LineLayer: LineLayer, - FillLayer: FillLayer, - Images: Images, - Callout: Callout, - StyleURL: StyleURL, - UserTrackingMode: UserTrackingMode, - setAccessToken: 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.ts` around lines 38 - 55, The Mapbox object literal uses redundant explicit key-value pairs where the key equals the identifier; replace entries like "MapView: MapView, Camera: Camera, PointAnnotation: PointAnnotation, UserLocation: UserLocation, MarkerView: MarkerView, ShapeSource: ShapeSource, SymbolLayer: SymbolLayer, CircleLayer: CircleLayer, LineLayer: LineLayer, FillLayer: FillLayer, Images: Images, Callout: Callout, StyleURL: StyleURL, UserTrackingMode: UserTrackingMode, setAccessToken: setAccessToken" with ES shorthand property syntax (e.g., MapView, Camera, PointAnnotation, etc.) while keeping the spread "...MapboxImpl" unchanged so the exported Mapbox object is more concise and idiomatic.
🤖 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 31-35: The test mock for the UserTrackingMode enum is using
outdated string values; update the mock object in the test (the UserTrackingMode
entry in index.test.tsx) to match the new enum strings by replacing Follow:
'follow' -> 'normal', FollowWithHeading: 'followWithHeading' -> 'compass', and
FollowWithCourse: 'followWithCourse' -> 'course' so the mock aligns with the
UserTrackingMode enum in map-view.web.tsx.
---
Outside diff comments:
In `@src/components/livekit/livekit-bottom-sheet.tsx`:
- Around line 257-264: The early-return in getViewContent() that checks
isConnecting causes renderRoomSelect()'s ActivityIndicator to be dead code;
remove the isConnecting branch from getViewContent() (or at minimum stop
returning the plain text connecting view there) so the switch/case can call
renderRoomSelect() and render its ActivityIndicator, ensuring the connecting
state is handled only inside renderRoomSelect(); update any testID or
accessibility labels accordingly (refer to getViewContent, renderRoomSelect,
isConnecting, and the ActivityIndicator block).
---
Nitpick comments:
In `@src/components/maps/mapbox.ts`:
- Around line 38-55: The Mapbox object literal uses redundant explicit key-value
pairs where the key equals the identifier; replace entries like "MapView:
MapView, Camera: Camera, PointAnnotation: PointAnnotation, UserLocation:
UserLocation, MarkerView: MarkerView, ShapeSource: ShapeSource, SymbolLayer:
SymbolLayer, CircleLayer: CircleLayer, LineLayer: LineLayer, FillLayer:
FillLayer, Images: Images, Callout: Callout, StyleURL: StyleURL,
UserTrackingMode: UserTrackingMode, setAccessToken: setAccessToken" with ES
shorthand property syntax (e.g., MapView, Camera, PointAnnotation, etc.) while
keeping the spread "...MapboxImpl" unchanged so the exported Mapbox object is
more concise and idiomatic.
In `@src/components/sidebar/sidebar-content.tsx`:
- Around line 33-36: handleNavigateToSettings currently calls onClose?.() (which
flips setIsOpen(false)) and immediately calls router.push('/settings'), causing
navigation to start before the drawer finish-close animation; update the flow so
navigation happens after the drawer has fully closed by either: (A) changing
onClose to return a Promise that resolves when the close animation completes and
awaiting onClose() inside handleNavigateToSettings before calling router.push,
or (B) adding a small timed delay in handleNavigateToSettings (matching the
drawer animation duration) before calling router.push; reference the
handleNavigateToSettings function and the onClose/ router.push calls when
implementing the change.
In `@src/services/bluetooth-audio.service.ts`:
- Around line 10-22: The comments falsely state "Using import() for lazy
loading" while the functions getLiveKitCallStore and getLiveKitStore use
synchronous require(); either update the comments to mention require() or change
the implementation to use dynamic import() for real ES module lazy loading, and
replace the loose ": any" return types with the actual store return types
exported by the modules (e.g., the type returned by useLiveKitCallStore and
useLiveKitStore) by importing those types with "import type" so callers retain
type safety.
In `@src/stores/app/livekit-store.ts`:
- Around line 353-355: Update the comment above requestPermissions to correctly
state where phone-state permissions are requested: change the reference from
"called at the top of connectToRoom" to indicate that phone-state permissions
are handled separately by requestAndroidPhonePermissions, which is invoked from
the bottom sheet (not from connectToRoom); keep the note that requestPermissions
only requests microphone permission and mention the actual caller
requestAndroidPhonePermissions to avoid future confusion.
Comment on lines
+31
to
+35
| export enum UserTrackingMode { | ||
| Follow = 'normal', | ||
| FollowWithHeading = 'compass', | ||
| FollowWithCourse = 'course', | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how UserTrackingMode is used across the codebase
echo "=== UserTrackingMode usage ==="
rg -n 'UserTrackingMode' --type=ts --type=tsx -C2
echo ""
echo "=== Direct string comparisons with old values ==="
rg -n "'follow'|'followWithHeading'|'followWithCourse'" --type=ts --type=tsx
echo ""
echo "=== Direct string comparisons with new values ==="
rg -n "'normal'|'compass'|'course'" --type=ts --type=tsx -C1
echo ""
echo "=== Native map-view implementation ==="
fd 'map-view.native' --exec cat {}Repository: Resgrid/Unit
Length of output: 1389
🏁 Script executed:
#!/bin/bash
# Check map-view web implementation
echo "=== map-view.web.tsx content ==="
cat src/components/maps/map-view.web.tsx
echo ""
echo "=== All references to UserTrackingMode (broader search) ==="
rg 'UserTrackingMode' -C2
echo ""
echo "=== Check mapbox.ts aggregator ==="
cat src/components/maps/mapbox.ts
echo ""
echo "=== Check how Mapbox UserTrackingMode is used (any reference) ==="
rg 'followUserMode|tracking' -C2Repository: Resgrid/Unit
Length of output: 48693
Update test mocks to match new UserTrackingMode enum values.
The enum values changed from 'follow'/'followWithHeading'/'followWithCourse' to 'normal'/'compass'/'course' to align with mapbox-gl. The test file at src/app/(app)/__tests__/index.test.tsx contains a mock with outdated values and needs updating:
UserTrackingMode: {
Follow: 'follow', // Should be 'normal'
FollowWithHeading: 'followWithHeading', // Should be 'compass'
FollowWithCourse: 'followWithCourse', // Should be 'course'
}Production code safely accesses the enum via property names (Mapbox.UserTrackingMode.FollowWithHeading), so no runtime breakage occurs, but the mock should remain consistent with the actual implementation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/maps/map-view.web.tsx` around lines 31 - 35, The test mock for
the UserTrackingMode enum is using outdated string values; update the mock
object in the test (the UserTrackingMode entry in index.test.tsx) to match the
new enum strings by replacing Follow: 'follow' -> 'normal', FollowWithHeading:
'followWithHeading' -> 'compass', and FollowWithCourse: 'followWithCourse' ->
'course' so the mock aligns with the UserTrackingMode enum in map-view.web.tsx.
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
Tests