feat: static & live location sharing (draft) #6694
Conversation
Signed-off-by: yiwei gao <[email protected]>
Signed-off-by: yiwei gao <[email protected]>
Signed-off-by: yiwei gao <[email protected]>
|
yiwei gao seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
WalkthroughAdds location sharing and live-tracking: map provider preferences and per-user API keys, static map previews and deep links, single-shot and foreground tracking, global live-location state and status bar, new modals and preference screens, Android/iOS location permissions, and an attachments parameter added to sendMessage. Also removes StatusBar from root render and updates project config. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor U as User
participant A as ActionsButton
participant P as Permissions
participant S as staticLocation
participant N as Navigation
participant M as LocationPreviewModal
participant SM as sendMessage
participant R as Server
U->>A: Tap "Share Location"
A->>P: Check/request location permission
P-->>A: Granted / Denied
alt Granted
A->>S: getCurrentPositionOnce()
S-->>A: Coords
A->>N: Navigate to LocationPreviewModal(params)
U->>M: Confirm Share
M->>SM: sendMessage(..., attachments?)
SM->>R: Post message
R-->>SM: OK
SM-->>M: Success
else Denied
A-->>U: Show permission error alert
end
sequenceDiagram
autonumber
actor U as User
participant A as ActionsButton
participant N as Navigation
participant LM as LiveLocationPreviewModal
participant T as LiveLocationTracker
participant SB as LiveLocationStatusBar
participant SM as sendMessage
participant H as handleLiveLocationUrl
U->>A: Start Live Location
A->>N: Open LiveLocationPreviewModal(params)
LM->>T: startTracking(callback)
T-->>LM: location updates
LM->>SB: emit active status
SB-->>U: Banner shown
U->>LM: Share -> LM->>SM: send start live message
SB->>T: Tap stop -> stopTracking()
SB->>SM: send stop message
SM-->>SB: OK
H-->>N: Incoming deep link handled by handleLiveLocationUrl
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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: 19
🧹 Nitpick comments (22)
ios/.ruby-version (1)
1-1: Update .ruby-version to Ruby 3.2.9Ruby 3.2.2 has no reported vulnerabilities, but upgrading to the latest 3.2.x patch (3.2.9) ensures you benefit from all bug fixes and performance improvements.
app/views/LocationShare/services/staticLocation.ts (4)
35-35: Redundant nullish coalescing.Line 35 uses
accuracy ?? undefined, but ifaccuracyis alreadynullorundefined, the result isundefinedanyway. This is redundant.Simplify to:
-accuracy: accuracy ?? undefined, +accuracy,
39-41: Silent error swallowing in fast path.The try-catch block silently swallows errors from
getLastKnownPositionAsyncwithout logging. This makes debugging difficult if the fast path fails unexpectedly.Add minimal logging:
} catch (e) { - // Use fresh location if cached fails + // Use fresh location if cached fails (expected in some cases) + log('getLastKnownPositionAsync failed, falling back to fresh location', e); }
55-55: Same redundant nullish coalescing.Line 55 has the same redundant pattern as line 35.
-accuracy: accuracy ?? undefined, +accuracy,
58-60: Redundant error re-throw.The catch block on lines 58-60 catches and immediately re-throws the error without any transformation or logging. This adds no value.
Remove the try-catch wrapper around the entire function body, or add meaningful error handling:
- } catch (error) { - throw error; - }app/views/LocationPreferencesView/ListPicker.tsx (2)
37-37: Redundant ternary - simplify initialization.The initialization
value ? OPTIONS.find(option => option.value === value) : OPTIONS[0]is redundant because thefindwill returnundefinedif no match is found, and theuseEffecton lines 39-42 will immediately correct it toOPTIONS[0].Simplify:
-const [option, setOption] = useState(value ? OPTIONS.find(option => option.value === value) : OPTIONS[0]); +const [option, setOption] = useState<typeof OPTIONS[0]>(OPTIONS.find(option => option.value === value) || OPTIONS[0]);
16-25: Hardcoded options - consider making configurable.The
OPTIONSarray is hardcoded with only two providers. If the app needs to support additional map providers in the future or customize the available providers per deployment, this will require code changes.Consider accepting options as a prop or importing from a shared configuration:
interface IListPickerProps { title: string; value: MapProviderName; onChangeValue: (value: MapProviderName) => void; testID?: string; options?: { label: string; value: MapProviderName }[]; } const ListPicker = ({ title, value, onChangeValue, testID, options = DEFAULT_OPTIONS }: IListPickerProps) => { // ...app/views/LocationPreferencesView/ApiKeyModal.tsx (3)
48-48: secureTextEntry hides API keys - consider trade-offs.Using
secureTextEntryfor API keys prevents users from verifying what they typed. API keys are typically long alphanumeric strings that are error-prone to enter blind. Unlike passwords (which users memorize), API keys are usually copied from external sources.Consider adding a "show/hide" toggle to let users verify their input:
const [isSecure, setIsSecure] = useState(true); // In TextInput secureTextEntry={isSecure} // Add toggle button <TouchableOpacity onPress={() => setIsSecure(!isSecure)}> <CustomIcon name={isSecure ? 'eye' : 'eye-off'} /> </TouchableOpacity>
25-27: Missing input validation on save.The
handleSavefunction only trims whitespace but doesn't validate whether the API key format is reasonable (e.g., minimum length, character set). Users could save empty strings or invalid keys.Add basic validation:
const handleSave = () => { const trimmed = value.trim(); if (trimmed.length === 0) { // Show error or disable save return; } onSave(trimmed); };Consider validating against known API key patterns for Google Maps (starts with certain prefixes, specific length) if feasible.
76-76: Hardcoded overlay color - use theme.Line 76 hardcodes
backgroundColor: 'rgba(0, 0, 0, 0.5)'instead of using the theme's backdrop color. This is inconsistent with the theming system.Based on the relevant code snippet showing
colors.backdropColorandcolors.backdropOpacityexist in the theme:-backgroundColor: 'rgba(0, 0, 0, 0.5)', +backgroundColor: colors.backdropColor, +opacity: colors.backdropOpacity,Or apply the backdrop styling via the theme's predefined overlay values.
app/views/LocationShare/services/handleLiveLocationUrl.ts (2)
22-29: Log or handle AsyncStorage failures in saveEndedSet.The empty catch block silently swallows storage errors. While non-critical for this feature, consider logging the error for debugging purposes or telemetry.
Apply this diff to add minimal error logging:
} catch (e) { - + console.warn('[handleLiveLocationUrl] Failed to persist ended IDs:', e); }
46-84: Consider consistent return values for handleLiveLocationUrl.The function returns
trueat line 72 when navigating on cold start, but implicitly returnsundefinedin other execution paths (lines 60, 77). For better consistency, consider either:
- Making the return type explicit (
Promise<boolean>orPromise<void>)- Returning a boolean in all paths to indicate whether the URL was handled
Otherwise, the logic is sound: proper validation, user-friendly alerts for ended sessions, and correct routing for active vs. cold-start scenarios.
Apply this diff to make returns consistent:
- if (liveLocationId && (await isLiveLocationEnded(liveLocationId))) { + if (liveLocationId && (await isLiveLocationEnded(liveLocationId))) { Alert.alert(I18n.t('Live_Location_Ended_Title'), I18n.t('Live_Location_Ended_Message'), [{ text: I18n.t('OK') }]); - return; + return false; } if (!isLiveLocationActive()) { // Handle cold start - navigate to live location viewer Navigation.navigate('LiveLocationPreviewModal', { provider, ...(rid ? { rid } : {}), ...(tmid ? { tmid } : {}), liveLocationId, isTracking: false }); return true; } const params = getCurrentLiveParams(); if (params?.liveLocationId && liveLocationId && params.liveLocationId !== liveLocationId) { - return; + return false; } reopenLiveLocationModal(); + return true; } catch (e) { // Invalid URL format + return false; }app/views/LocationShare/services/liveLocation.ts (3)
1-2: Minor: Add empty line between import groups.ESLint flagged the missing empty line between external and internal imports. While minor, following the linting rule improves consistency.
Apply this diff:
import * as Location from 'expo-location'; + import { MapProviderName } from './mapProviders';
121-141: Consider omitting empty rid/tmid parameters from the URL.Lines 131-132 set
ridandtmidto empty strings when not provided. This results in URL query strings likerid=&tmid=which are unnecessary. Consider conditionally adding parameters only when they have values.Apply this diff to omit empty parameters:
- const params = new URLSearchParams({ - liveLocationId, - rid: rid || '', - tmid: tmid || '', - provider, - action: 'reopen' - }); + const params = new URLSearchParams({ + liveLocationId, + provider, + action: 'reopen' + }); + if (rid) params.set('rid', rid); + if (tmid) params.set('tmid', tmid);
143-149: Unused parameters in createLiveLocationStopMessage.The function accepts
_liveLocationId,_provider, and_lastCoordsbut doesn't use them. If these are reserved for future enhancement (e.g., showing final coordinates or session details in the stop message), consider adding a comment to clarify intent. Otherwise, they could be removed to reduce confusion.app/i18n/locales/en.json (1)
322-322: Standardize capitalization of "API key" in user-facing strings.There's an inconsistency in the capitalization of "API key":
- Line 322: "Enter your OpenStreetMap API key" (lowercase "key")
- Line 629: "OpenStreetMap API Key" (uppercase "Key")
For consistency across the UI, standardize to one form (typically lowercase "key" is more common in modern UX writing).
Apply this diff to standardize:
- "OSM_API_Key": "OpenStreetMap API Key", + "OSM_API_Key": "OpenStreetMap API key",Similarly for Google Maps:
- "Google_Maps_API_Key": "Google Maps API Key", + "Google_Maps_API_Key": "Google Maps API key",Also applies to: 629-629
app/views/LocationShare/LocationPreviewModal.tsx (1)
13-13: Consider removing duplicate Coords type definition.The
Coordstype is already defined inapp/views/LocationShare/services/mapProviders.ts(line 11) andapp/views/LocationShare/services/staticLocation.ts(line 2). Consider importing it instead to maintain a single source of truth.-type Coords = { latitude: number; longitude: number; accuracy?: number; timestamp?: number }; +import { Coords } from './services/staticLocation';app/views/LocationShare/LiveLocationStatusBar.tsx (2)
128-144: Consider safe area insets for the status bar positioning.The status bar uses absolute positioning with
top: 0, which may overlap with the device's status bar or notch on iOS/Android. Consider usingSafeAreaVieworuseSafeAreaInsetsto respect device safe areas.+import { useSafeAreaInsets } from 'react-native-safe-area-context'; + export default function LiveLocationStatusBar({ onPress }: Props) { + const insets = useSafeAreaInsets(); // ... rest of component return ( - <View style={styles.container}> + <View style={[styles.container, { top: insets.top }]}>
52-66: Clarify deeplink listener comment
In app/views/LocationShare/LiveLocationStatusBar.tsx (line 52), replace “works around openLink.ts” with a note that this hook registers a separate listener for incoming rocketchat://live-location URLs—bypassing the centralized deep‐link parser in app/index.tsx (which doesn’t recognize live-location links)—so the component can handle live-location links directly.app/views/LocationShare/services/mapProviders.ts (3)
55-75: Consider using template literal for query string.The function correctly builds Google Static Maps URLs. However, using template literals instead of string concatenation would be more idiomatic and readable.
Apply this diff:
- const qp = - `center=${latitude},${longitude}` + - `&zoom=${z}` + - `&size=${width}x${height}` + - `&scale=${sc}` + - `&maptype=${enc(type)}` + - `&markers=color:${enc(color)}|${latitude},${longitude}` + - (googleApiKey ? `&key=${enc(googleApiKey)}` : ''); + const qp = `center=${latitude},${longitude}&zoom=${z}&size=${width}x${height}&scale=${sc}&maptype=${enc(type)}&markers=color:${enc(color)}|${latitude},${longitude}${googleApiKey ? `&key=${enc(googleApiKey)}` : ''}`;Or for better readability across multiple lines:
- const qp = - `center=${latitude},${longitude}` + - `&zoom=${z}` + - `&size=${width}x${height}` + - `&scale=${sc}` + - `&maptype=${enc(type)}` + - `&markers=color:${enc(color)}|${latitude},${longitude}` + - (googleApiKey ? `&key=${enc(googleApiKey)}` : ''); + const qp = [ + `center=${latitude},${longitude}`, + `zoom=${z}`, + `size=${width}x${height}`, + `scale=${sc}`, + `maptype=${enc(type)}`, + `markers=color:${enc(color)}|${latitude},${longitude}`, + googleApiKey ? `key=${enc(googleApiKey)}` : '' + ].filter(Boolean).join('&');
77-93: Consider using template literal for query string.Similar to
buildGoogleStaticUrl, using a template literal or array join would improve readability.Apply this diff:
- const qp = - (osmApiKey ? `key=${enc(osmApiKey)}` : '') + - `¢er=${latitude},${longitude}` + - `&zoom=${z}` + - `&size=${width}x${height}` + - `&markers=${enc(marker)}`; + const qp = [ + osmApiKey ? `key=${enc(osmApiKey)}` : '', + `center=${latitude},${longitude}`, + `zoom=${z}`, + `size=${width}x${height}`, + `markers=${enc(marker)}` + ].filter(Boolean).join('&');
119-191: Consider removingasyncfrom non-awaiting paths (optional).The deep link logic is well-structured and platform-aware. The
mapsDeepLinkfunction isasyncto supportiosGoogleLink, which awaitsLinking.canOpenURL. While ESLint flags "no await" for some code paths, keeping the functionasyncensures a consistent API and allows for future async operations without breaking changes.If you prefer to address the linter warning, you can refactor to return a
Promiseexplicitly:export function mapsDeepLink(provider: AnyProvider, coords: Coords): Promise<string> { if (Platform.OS === 'ios') { switch (provider) { case 'google': return iosGoogleLink(coords); case 'osm': return Promise.resolve(iosOsmLink(coords)); case 'apple': return Promise.resolve(iosAppleLink(coords)); default: return Promise.resolve(iosAppleLink(coords)); } } // ANDROID switch (provider) { case 'google': case 'apple': return Promise.resolve(androidGoogleLikeLink(coords)); case 'osm': return Promise.resolve(androidOsmLink(coords)); default: return Promise.resolve(androidGoogleLikeLink(coords)); } }However, the current
asyncsignature is acceptable for API consistency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (2)
ios/Podfile.lockis excluded by!**/*.lockyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (28)
android/app/src/main/AndroidManifest.xml(1 hunks)app.json(1 hunks)app/containers/MessageComposer/components/Buttons/ActionsButton.tsx(3 hunks)app/i18n/locales/en.json(17 hunks)app/index.tsx(0 hunks)app/lib/constants/keys.ts(1 hunks)app/lib/methods/sendMessage.ts(4 hunks)app/stacks/InsideStack.tsx(4 hunks)app/stacks/MasterDetailStack/index.tsx(2 hunks)app/stacks/MasterDetailStack/types.ts(1 hunks)app/stacks/types.ts(2 hunks)app/views/LocationPreferencesView/ApiKeyModal.tsx(1 hunks)app/views/LocationPreferencesView/ListPicker.tsx(1 hunks)app/views/LocationPreferencesView/index.tsx(1 hunks)app/views/LocationShare/LiveLocationPreviewModal.tsx(1 hunks)app/views/LocationShare/LiveLocationStatusBar.tsx(1 hunks)app/views/LocationShare/LocationPreviewModal.tsx(1 hunks)app/views/LocationShare/services/handleLiveLocationUrl.ts(1 hunks)app/views/LocationShare/services/liveLocation.ts(1 hunks)app/views/LocationShare/services/mapProviders.ts(1 hunks)app/views/LocationShare/services/staticLocation.ts(1 hunks)app/views/RoomView/index.tsx(4 hunks)app/views/UserPreferencesView/index.tsx(1 hunks)ios/.ruby-version(1 hunks)ios/NotificationService/Info.plist(1 hunks)ios/RocketChatRN.xcodeproj/project.pbxproj(4 hunks)ios/RocketChatRN/Info.plist(4 hunks)package.json(2 hunks)
💤 Files with no reviewable changes (1)
- app/index.tsx
🧰 Additional context used
🧬 Code graph analysis (13)
app/stacks/InsideStack.tsx (2)
app/views/LocationShare/LocationPreviewModal.tsx (1)
LocationPreviewModal(24-136)app/views/LocationShare/LiveLocationPreviewModal.tsx (1)
LiveLocationPreviewModal(72-365)
app/views/LocationPreferencesView/index.tsx (7)
app/stacks/types.ts (1)
ProfileStackParamList(190-203)app/stacks/MasterDetailStack/types.ts (1)
MasterDetailInsideStackParamList(213-232)app/theme.tsx (1)
useTheme(29-29)app/lib/hooks/useAppSelector.ts (1)
useAppSelector(6-6)app/lib/methods/userPreferences.ts (1)
useUserPreferences(10-10)app/views/LocationShare/services/mapProviders.ts (1)
MapProviderName(7-7)app/lib/constants/keys.ts (4)
MAP_PROVIDER_PREFERENCE_KEY(24-24)MAP_PROVIDER_DEFAULT(27-27)GOOGLE_MAPS_API_KEY_PREFERENCE_KEY(25-25)OSM_API_KEY_PREFERENCE_KEY(26-26)
app/views/LocationShare/services/staticLocation.ts (1)
app/views/LocationShare/services/mapProviders.ts (1)
Coords(12-12)
app/views/LocationPreferencesView/ApiKeyModal.tsx (2)
app/theme.tsx (1)
useTheme(29-29)app/lib/constants/colors.ts (1)
colors(280-302)
app/views/LocationShare/services/liveLocation.ts (1)
app/views/LocationShare/services/mapProviders.ts (1)
MapProviderName(7-7)
app/views/LocationShare/services/mapProviders.ts (1)
app/views/LocationShare/services/staticLocation.ts (1)
Coords(3-3)
app/views/LocationShare/LiveLocationStatusBar.tsx (4)
app/lib/hooks/useAppSelector.ts (1)
useAppSelector(6-6)app/selectors/login.ts (1)
getUserSelector(20-20)app/views/LocationShare/LiveLocationPreviewModal.tsx (6)
isLiveLocationActive(367-369)addStatusChangeListener(55-57)removeStatusChangeListener(58-60)reopenLiveLocationModal(371-379)getCurrentLiveParams(52-54)stopGlobalLiveLocation(381-416)app/views/LocationShare/services/handleLiveLocationUrl.ts (2)
isLiveMessageLink(42-44)handleLiveLocationUrl(46-84)
app/views/LocationPreferencesView/ListPicker.tsx (3)
app/views/LocationShare/services/mapProviders.ts (1)
MapProviderName(7-7)app/containers/ActionSheet/Provider.tsx (2)
useActionSheet(39-39)TActionSheetOptionsItem(7-15)app/theme.tsx (1)
useTheme(29-29)
app/views/RoomView/index.tsx (1)
app/views/LocationShare/LiveLocationStatusBar.tsx (1)
LiveLocationStatusBar(27-125)
app/containers/MessageComposer/components/Buttons/ActionsButton.tsx (7)
app/lib/hooks/useAppSelector.ts (1)
useAppSelector(6-6)app/lib/methods/userPreferences.ts (1)
useUserPreferences(10-10)app/views/LocationShare/services/mapProviders.ts (1)
MapProviderName(7-7)app/lib/constants/keys.ts (4)
MAP_PROVIDER_PREFERENCE_KEY(24-24)MAP_PROVIDER_DEFAULT(27-27)GOOGLE_MAPS_API_KEY_PREFERENCE_KEY(25-25)OSM_API_KEY_PREFERENCE_KEY(26-26)app/lib/methods/helpers/info.ts (1)
showErrorAlert(5-6)app/views/LocationShare/services/staticLocation.ts (1)
getCurrentPositionOnce(24-61)app/containers/ActionSheet/Provider.tsx (1)
TActionSheetOptionsItem(7-15)
app/views/LocationShare/services/handleLiveLocationUrl.ts (1)
app/views/LocationShare/LiveLocationPreviewModal.tsx (3)
isLiveLocationActive(367-369)getCurrentLiveParams(52-54)reopenLiveLocationModal(371-379)
app/views/LocationShare/LocationPreviewModal.tsx (5)
app/views/LocationShare/services/staticLocation.ts (1)
Coords(3-3)app/views/LocationShare/services/mapProviders.ts (5)
Coords(12-12)MapProviderName(7-7)staticMapUrl(98-113)mapsDeepLink(165-191)providerLabel(196-196)app/lib/hooks/useAppSelector.ts (1)
useAppSelector(6-6)app/selectors/login.ts (1)
getUserSelector(20-20)app/lib/methods/sendMessage.ts (1)
sendMessage(87-241)
app/views/LocationShare/LiveLocationPreviewModal.tsx (5)
app/views/LocationShare/services/mapProviders.ts (4)
MapProviderName(7-7)staticMapUrl(98-113)mapsDeepLink(165-191)providerLabel(196-196)app/views/LocationShare/services/liveLocation.ts (5)
LiveLocationTracker(14-115)LiveLocationState(4-12)generateLiveLocationId(117-119)createLiveLocationMessage(121-141)createLiveLocationStopMessage(143-149)app/lib/hooks/useAppSelector.ts (1)
useAppSelector(6-6)app/selectors/login.ts (1)
getUserSelector(20-20)app/views/LocationShare/services/handleLiveLocationUrl.ts (1)
markLiveLocationAsEnded(31-35)
🪛 Biome (2.1.2)
app/views/LocationShare/services/mapProviders.ts
[error] 110-110: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
🪛 ESLint
app/views/LocationShare/services/liveLocation.ts
[error] 1-1: There should be at least one empty line between import groups
(import/order)
app/views/LocationShare/services/mapProviders.ts
[error] 66-72: Unexpected string concatenation.
(prefer-template)
[error] 86-90: Unexpected string concatenation.
(prefer-template)
[error] 165-165: Async function 'mapsDeepLink' has no 'await' expression.
(require-await)
app/views/LocationShare/LiveLocationStatusBar.tsx
[error] 2-12: There should be at least one empty line between import groups
(import/order)
[error] 10-10: 'Platform' is defined but never used.
(@typescript-eslint/no-unused-vars)
[error] 11-11: 'StatusBar' is defined but never used.
(@typescript-eslint/no-unused-vars)
app/views/RoomView/index.tsx
[error] 1498-1498: 'loading' is assigned a value but never used.
(@typescript-eslint/no-unused-vars)
app/containers/MessageComposer/components/Buttons/ActionsButton.tsx
[error] 12-12: react-native import should occur before import of ../../../../lib/database/services/Subscription
(import/order)
[error] 13-13: There should be at least one empty line between import groups
(import/order)
[error] 13-13: expo-location import should occur before import of ../../../../lib/database/services/Subscription
(import/order)
app/views/LocationShare/services/handleLiveLocationUrl.ts
[error] 26-28: Empty block statement.
(no-empty)
app/views/LocationShare/LocationPreviewModal.tsx
[error] 9-9: There should be at least one empty line between import groups
(import/order)
[error] 10-10: There should be at least one empty line between import groups
(import/order)
[error] 10-10: react-redux import should occur before import of ../../lib/navigation/appNavigation
(import/order)
[error] 40-40: 'token' is assigned a value but never used.
(@typescript-eslint/no-unused-vars)
app/views/LocationShare/LiveLocationPreviewModal.tsx
[error] 3-3: There should be at least one empty line between import groups
(import/order)
[error] 4-4: There should be no empty line within import group
(import/order)
[error] 18-18: There should be at least one empty line between import groups
(import/order)
[error] 19-19: react-redux import should occur before import of ../../i18n
(import/order)
[error] 239-239: Expected 'undefined' and instead saw 'void'.
(no-void)
[error] 395-395: Empty block statement.
(no-empty)
🔇 Additional comments (53)
ios/RocketChatRN.xcodeproj/project.pbxproj (1)
1885-1885: Confirm whether the quote removal is intentional.The changes remove surrounding quotation marks from path variables in build phase inputPaths (lines 1885, 1971) and HEADER_SEARCH_PATHS (lines 2701, 2778). Both quoted and unquoted forms are valid in Xcode project files, so this has no functional impact. However, it's worth confirming whether this was an intentional style change or an automatic modification by Xcode when the project file was saved.
Also applies to: 1971-1971, 2701-2701, 2778-2778
ios/NotificationService/Info.plist (1)
25-26: LGTM! Key promoted to top-level.The
IS_OFFICIALkey has been correctly moved from a nested position to the top level of the plist dictionary, improving the file structure.package.json (2)
77-107: Clarify the need for two location libraries.Both
expo-location(line 77) andreact-native-geolocation-service(line 107) are added. These libraries serve similar purposes. Please clarify:
- Why both are needed (e.g., platform-specific fallback, feature coverage)?
- Whether they might conflict in location permission handling or API usage?
Having redundant location libraries can increase bundle size and create maintenance overhead.
77-77: Confirm expo-location compatibility and acknowledge security audit
expo-location@^19.0.7has no known security advisories. (security.snyk.io)- Ensure v19.x aligns with your Expo SDK 53 (React Native 0.79.4) modules.
app/stacks/MasterDetailStack/types.ts (1)
196-196: LGTM! Navigation type correctly added.The
LocationPreferencesViewentry is properly added toModalStackParamListwith no required parameters, following the established pattern for similar settings screens.ios/RocketChatRN/Info.plist (3)
60-60: LGTM! Google Maps scheme added.The
comgooglemapsscheme is correctly added toLSApplicationQueriesSchemes, allowing the app to detect and open Google Maps.
93-96: LGTM! Clear and appropriate location usage descriptions.The location usage descriptions clearly explain:
- When-in-use: Location shared only on explicit user action
- Always/background: Purpose of background access and user control
These descriptions comply with App Store guidelines and respect user privacy.
112-112: LGTM! Background location mode added.The
locationbackground mode is correctly added toUIBackgroundModes, enabling live location sharing while the app is in the background. This aligns with the usage descriptions provided on lines 93-96.app.json (1)
3-3: expo-web-browser dependency already declaredexpo-web-browser is listed in package.json (version 14.1.5), so no further action is required.
app/views/LocationPreferencesView/ApiKeyModal.tsx (1)
49-49: autoFocus may not work reliably on Android.The
autoFocusprop on TextInput is known to have inconsistent behavior on Android, especially in modals. Users may need to manually tap the input.Test the autoFocus behavior on both iOS and Android devices. If it doesn't work reliably on Android, consider using a ref with a setTimeout to focus programmatically:
const inputRef = useRef<TextInput>(null); useEffect(() => { if (visible) { setTimeout(() => inputRef.current?.focus(), 100); } }, [visible]); // In TextInput ref={inputRef}app/views/LocationShare/services/staticLocation.ts (1)
8-22: Race condition in timeout cleanup.The
withTimeouthelper has a potential issue: if the promise settles exactly as the timeout fires, bothresolve/rejectand the timeout'srejectcould execute. While Promise semantics guarantee only the first settlement wins, theclearTimeoutcall on line 15 and 18 may execute after the timeout callback on line 11 has already fired, leaving an uncleared timeout in edge cases.Consider a more robust pattern using
Promise.race:-function withTimeout<T>(p: Promise<T>, ms: number): Promise<T> { - return new Promise((resolve, reject) => { - const t = setTimeout(() => { - reject(new Error('Location request timed out')); - }, ms); - - p.then(v => { - clearTimeout(t); - resolve(v); - }).catch(e => { - clearTimeout(t); - reject(e); - }); - }); -} +function withTimeout<T>(p: Promise<T>, ms: number): Promise<T> { + return Promise.race([ + p, + new Promise<T>((_, reject) => + setTimeout(() => reject(new Error('Location request timed out')), ms) + ) + ]); +}Likely an incorrect or invalid review comment.
app/lib/methods/sendMessage.ts (1)
104-111: I encountered issues running the prior scripts; please verify manually thatgetRoomInstancereturns anEncryptionRoominstance whoseencryptmethod correctly processes the fullIMessage, including itsattachmentsfield. Ensure that attachments are encrypted or passed through as intended to avoid data loss.app/lib/constants/keys.ts (1)
24-27: LGTM! Well-structured constants for map provider preferences.The new constants follow the existing naming conventions and provide a sensible default of OpenStreetMap. The keys are clearly named for their purposes: storing user preferences, API keys, and defining the default provider.
app/views/LocationShare/services/handleLiveLocationUrl.ts (3)
1-9: LGTM! Clean imports and well-named storage key.The module-level cache pattern for
endedIdsis appropriate for avoiding repeated AsyncStorage reads.
31-40: LGTM! Clean and straightforward API.The async functions properly handle the lazy-loaded set and provide a clear interface for tracking ended live locations.
42-44: LGTM! Appropriate URL validation.The case-insensitive regex correctly identifies live location deep links.
app/views/LocationShare/services/liveLocation.ts (6)
4-12: LGTM! Well-structured state type.The
LiveLocationStatetype clearly captures the essential location data with an appropriate optional accuracy field and activity flag.
14-27: LGTM! Clean encapsulation and safe callback handling.The class structure with private fields and optional-chaining for the callback is well-designed.
29-92: Verify the buffered update strategy meets UX requirements.The implementation uses a buffered update pattern: position changes from
watchPositionAsyncupdatecurrentStateinternally but do not immediately emit (line 69-79), while a separate 10-secondsetIntervalemits the current state. This means location updates on the UI can be delayed up to 10 seconds even if the user moves significantly.Tradeoffs:
- Pro: Reduces network traffic and battery usage by limiting message frequency.
- Con: Users may perceive stale location data for up to 10 seconds, which could be confusing during live tracking.
Consider whether this delay is acceptable for the live location sharing UX, or if position changes should trigger immediate emissions (perhaps with a debounce/throttle to balance freshness and efficiency).
Additional notes:
- Battery-conscious settings (Balanced accuracy, 5m distance interval) are well-chosen.
- Permission and service checks are thorough and correct.
94-110: LGTM! Thorough cleanup on stop.The method correctly removes the subscription, clears the interval, and emits a final inactive state while preserving the last-received timestamp.
112-114: LGTM! Simple and correct getter.
117-119: LGTM! Sufficient ID generation for session tracking.The combination of timestamp and random string provides adequate uniqueness for non-security-critical session IDs.
app/i18n/locales/en.json (2)
51-53: LGTM! Comprehensive i18n coverage for location sharing features.The new localization keys provide thorough coverage for:
- Map provider configuration (Google Maps & OpenStreetMap)
- API key management with clear security messaging
- Location sharing workflows (static & live)
- User-facing prompts and status indicators
The keys follow existing naming conventions and provide clear, actionable text for users.
Also applies to: 157-157, 183-184, 211-211, 317-322, 380-383, 392-392, 452-457, 476-478, 489-490, 594-594, 608-608, 657-657, 664-664, 815-817, 843-847
628-628: Ignore the “OS” renaming suggestion.The “OS” key is exclusively used in Livechat.tsx to label the user’s Operating System (
roomUser.os), so there’s no ambiguity with OpenStreetMap and no change is needed.Likely an incorrect or invalid review comment.
app/views/RoomView/index.tsx (1)
23-23: LGTM! Clean integration of location status bar.The additions integrate the new live location status feature into RoomView:
StatusBarandLiveLocationStatusBarare rendered at the top of the SafeAreaViewLiveLocationStatusBarmanages its own visibility based on global live location state- The component placement is appropriate for a persistent status indicator
Based on the retrieved code snippet,
LiveLocationStatusBarhandles its own press events and state management, making this a clean, minimal integration.Also applies to: 106-106, 1539-1540
app/views/UserPreferencesView/index.tsx (1)
83-89: LGTM! Location preferences item properly integrated.The new list item follows the established pattern:
- Uses localized title with existing i18n key
- Navigates via the existing
navigateToScreenhelper- Includes proper accessibility attributes (testID, showActionIndicator)
- Separator placement is consistent with other items
The positioning after Notifications makes logical sense for user preferences organization.
app/stacks/InsideStack.tsx (3)
6-6: LGTM! Clean imports for location features.The new imports are well-organized:
I18nimport added to support localized modal titles- Three new views imported for location preferences and previews
- Import placement follows existing file organization
Also applies to: 79-81
172-172: LGTM! LocationPreferencesView added to profile stack.The screen is correctly added to ProfileStackNavigator, making it accessible from the user preferences flow. This is consistent with the addition in MasterDetailStack and the navigation from UserPreferencesView.
326-335: LGTM! Modal screens properly configured for location sharing.Both modal screens are well-configured:
LocationPreviewModalandLiveLocationPreviewModaluse modal presentation- Titles are properly localized using
I18n.t- Positioning after ModalBlockView is logical
- Modal presentation mode is appropriate for these transient location-sharing flows
Based on the provided code snippets, these modals handle:
- Static location preview with map image and share functionality
- Live location tracking with real-time updates and session management
The configuration aligns with the expected use cases.
app/stacks/types.ts (2)
194-194: LGTM: LocationPreferencesView added to ProfileStack.The addition of LocationPreferencesView to the profile navigation stack is appropriate for user-level location preferences.
298-314: LGTM: Modal type definitions are well-structured.Both LocationPreviewModal and LiveLocationPreviewModal have appropriate type definitions with:
- Room/thread context (rid, tmid)
- Provider flexibility (Google Maps/OSM)
- Optional API keys for customization
- LiveLocationPreviewModal correctly includes ownership tracking fields
app/views/LocationShare/LocationPreviewModal.tsx (2)
73-91: Verify location attachment support in sendMessage.The
sendMessagefunction is called without theattachmentsparameter (line 84), but the function signature fromapp/lib/methods/sendMessage.tsshows it accepts an optionalattachmentsarray. For proper location message handling, consider whether the location data should be passed as a structured attachment rather than just markdown text.If location data should be stored as a structured attachment for better parsing/display in message lists, apply this pattern:
const onShare = async () => { try { safeSet(() => setSubmitting(true)); const deep = await mapsDeepLink(provider, coords); const providerName = providerLabel(provider); const message = `📍 **Location** [🗺️ Open in ${providerName}](${deep})`; - await sendMessage(rid, message, tmid, { id, username }, false); + const attachments = [{ + type: 'location', + latitude: coords.latitude, + longitude: coords.longitude, + accuracy: coords.accuracy, + timestamp: coords.timestamp + }]; + await sendMessage(rid, message, tmid, { id, username }, false, attachments); Navigation.back();This would enable:
- Better message list previews (show map thumbnails)
- Structured querying of location messages
- Consistent location data format
110-121: LGTM: Image caching and error handling.Good use of expo-image with disk caching, transition animation, and blurhash placeholder to prevent flicker on Android. The error handler is appropriately minimal since the UI gracefully handles missing images.
app/views/LocationPreferencesView/index.tsx (3)
34-70: LGTM: Clean per-user storage pattern with proper state management.The implementation correctly:
- Uses per-user storage keys with userId suffix to isolate preferences
- Provides sensible defaults (OSM as default provider, empty strings for API keys)
- Implements clean modal handlers with save/cancel flows
- Separates concerns between provider selection and API key management
72-113: LGTM: Proper use of translation keys in List components.The code correctly passes translation keys (e.g.,
'Map_Provider','API_Keys') to List components, which handle I18n.t() internally. The conditional rendering of "Configured" vs "Not_configured" status is appropriate.
32-39: Guard against missinguserId
Ensurestate.login.user.idis always a non-empty string before constructing preference keys—add a loading state, fallback, or early return ifuserIdisn’t populated to prevent shared or colliding storage keys.app/views/LocationShare/LiveLocationStatusBar.tsx (2)
86-105: Verify ownership determination logic covers all scenarios.The ownership check (line 92) uses:
params.ownerName === usernameORparams.isTracking. This logic should be verified:
- What if
ownerNameis undefined butisTrackingis true? (Viewer mistakenly treated as owner)- What if both are undefined/false? (Correct: not owner)
- Should
isTrackingalone determine ownership, or isownerNamecomparison the source of truth?Review the ownership model in the live location flow to ensure:
- Only the session creator can stop sharing (sends "Ended" message)
- Viewers can only dismiss their local UI
- The fallback to
isTrackingis intentional and documentedBased on learnings: The params structure should clearly indicate owner vs viewer role.
44-79: LGTM: Robust state management and animation lifecycle.The component properly:
- Uses mounted guard to prevent state updates after unmount
- Subscribes/unsubscribes to global status changes
- Manages animation lifecycle tied to active state
- Implements smooth pulse animation with native driver
app/containers/MessageComposer/components/Buttons/ActionsButton.tsx (6)
37-41: LGTM! User-scoped preferences.The user-scoped preference keys ensure that each user's map provider and API key settings are isolated, which is appropriate for multi-account scenarios.
43-60: Good sheet serialization pattern.The
openSheetSafelyhelper effectively prevents overlapping sheet transitions with a busy flag and deferred execution. The fixed 350ms delay is reasonable for most animations.
73-115: LGTM! Solid permission and location flow.The function correctly handles:
- Platform-specific permission requests
- Location retrieval with timeout handling (via
getCurrentPositionOnce)- Provider-specific API key inclusion
- Navigation with master-detail awareness
- Error handling with user-friendly messages
163-183: LGTM! Clear mode selection flow.The mode sheet provides a clear choice between static and live location sharing, with safe sheet transitions for each option.
192-254: LGTM! Consistent sheet-close-then-action pattern.All action handlers now properly hide the sheet and defer actions via
InteractionManager, ensuring smooth UI transitions and preventing blocking animations.
257-274: LGTM! API key validation prevents broken flows.The Share Location action properly validates that required API keys are configured before allowing the user to proceed, providing clear error messaging when keys are missing.
app/views/LocationShare/services/mapProviders.ts (4)
7-23: LGTM! Well-defined types.The type definitions are clear and comprehensive, with provider-specific options appropriately separated (e.g.,
googleApiKeyvsosmApiKey,mapTypefor Google only).
28-36: LGTM! Sensible defaults and clear provider URLs.The constants provide reasonable defaults for map rendering, and the comment documenting LocationIQ for OSM static maps is helpful.
41-50: LGTM! Clean helper functions.The
parseSizeandenchelpers are simple, defensive, and reusable.
196-198: LGTM! Correct provider labels and attributions.The labels are user-friendly, and the OSM attribution complies with OpenStreetMap licensing requirements.
app/views/LocationShare/LiveLocationPreviewModal.tsx (5)
21-70: LGTM! Global tracker pattern for persistent sessions.The global singleton tracker and status listener system are well-designed to support:
- Keeping live location running when the modal is minimized
- Notifying the status bar and other components of state changes
- Safe listener execution with error isolation (lines 64-68)
72-109: LGTM! Defensive state management.The component setup includes:
- Mounted guard (
safeSet) to prevent state updates after unmount- Map image prefetch for improved UX
- Shallow equality check on Redux selectors to minimize re-renders
These patterns prevent common React Native pitfalls.
111-127: LGTM! Safe location update handler.The handler safely updates state, generates provider-specific map URLs, and notifies listeners. All state updates use the
safeSetguard.
129-153: LGTM! Tracker initialization with reuse logic.The effect correctly:
- Reuses
globalTrackerwhen reopening an active session- Creates a new tracker for initial shares
- Sets up dual callbacks to support both local UI updates and global state
- Handles tracking failures with user-friendly alerts
418-527: LGTM! Comprehensive styles.The styles provide a polished UI with:
- Clear visual hierarchy
- Appropriate spacing and shadows
- Consistent color scheme
- Blurhash placeholder for smooth image loading
| <uses-permission android:name="android.permission.FOREGROUND_SERVICE" /> | ||
| <uses-permission android:name="android.permission.FOREGROUND_SERVICE_LOCATION" /> |
There was a problem hiding this comment.
FOREGROUND_SERVICE_LOCATION requires service declaration in manifest.
The FOREGROUND_SERVICE_LOCATION permission requires a corresponding <service> declaration with android:foregroundServiceType="location" in the manifest. Without this declaration, the permission is ineffective and location access from a foreground service will fail.
Add a service declaration if live location tracking uses a foreground service:
<application>
...
<service
android:name=".LocationService"
android:foregroundServiceType="location"
android:exported="false" />
...
</application>If the app uses Expo or a library that handles this automatically, verify the service is properly configured.
#!/bin/bash
# Description: Check for service declarations and location service implementation
echo "Checking for service declarations in AndroidManifest..."
rg -n 'android:foregroundServiceType="location"' android/
echo "Checking for location service implementation..."
fd -e java -e kt LocationService android/app/src/🤖 Prompt for AI Agents
In android/app/src/main/AndroidManifest.xml around lines 27-28, the
FOREGROUND_SERVICE_LOCATION permission is declared but there's no corresponding
<service> entry with android:foregroundServiceType="location", so foreground
location access will fail; add a service declaration inside the <application>
(e.g., a LocationService with android:foregroundServiceType="location" and
android:exported=\"false\") or ensure your Expo/library-provided service is
configured to expose that attribute, then verify by searching the manifest for
android:foregroundServiceType="location" and checking your LocationService
implementation files.
| <!-- Optional: only if you truly need background updates on Android 10+ --> | ||
| <uses-permission android:name="android.permission.ACCESS_BACKGROUND_LOCATION" /> |
There was a problem hiding this comment.
🧩 Analysis chain
Background location permission requires strong justification.
The ACCESS_BACKGROUND_LOCATION permission is highly sensitive and triggers additional user prompts on Android 10+. Google Play requires apps using this permission to justify its necessity and may require special declaration forms.
Verify that:
- The PR truly requires background location access (the PR description mentions "sessions continue when modal is minimized" but doesn't clearly state background tracking is needed when the app is not visible).
- If background access is only needed for foreground services (while the app is visible but the modal is minimized), the
FOREGROUND_SERVICE_LOCATIONpermission alone may suffice. - The app's privacy policy is updated to explain background location usage.
- The runtime permission flow properly explains why background access is requested (required by Play Store policies).
Consider whether this permission can be removed if the feature only needs location while the app is in the foreground or in a foreground service state.
🏁 Script executed:
#!/bin/bash
# Description: Check if background location subscription is actually used in the codebase
echo "Searching for background location configuration or task registration..."
rg -n --type typescript --type javascript -C3 'Background|TaskManager|startLocationUpdates.*background' app/Length of output: 106
🏁 Script executed:
#!/bin/bash
echo "Searching for background location usage in codebase..."
rg -n --ignore-case -C3 'backgroundLocation' -g '*.ts' -g '*.tsx' -g '*.js' -g '*.jsx' .
rg -n --ignore-case -C3 'startLocationUpdates' .
rg -n --ignore-case -C3 'registerTask' .
rg -n --ignore-case -C3 'TaskManager' .Length of output: 54
🏁 Script executed:
#!/bin/bash
sed -n '1,200p' android/app/src/main/AndroidManifest.xmlLength of output: 5160
Remove unused ACCESS_BACKGROUND_LOCATION permission
No background‐location tasks (e.g. via TaskManager or startLocationUpdates) are registered anywhere in the codebase—drop <uses-permission android:name="android.permission.ACCESS_BACKGROUND_LOCATION"/> unless you’re actually implementing background updates.
🤖 Prompt for AI Agents
In android/app/src/main/AndroidManifest.xml around lines 29-30, remove the
unused ACCESS_BACKGROUND_LOCATION permission entry; since the codebase does not
register any background location tasks, delete the <uses-permission
android:name="android.permission.ACCESS_BACKGROUND_LOCATION" /> line from the
manifest (or guard it behind a build flag only when background updates are
implemented) to eliminate unnecessary permission requests.
| "expo": { | ||
| "extra": { | ||
| "GOOGLE_MAPS_API_KEY": "", | ||
| "OSM_API_KEY": "" | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Ensure runtime handling of empty API keys.
The GOOGLE_MAPS_API_KEY and OSM_API_KEY are initialized as empty strings, which is appropriate for an open-source repository. However, verify that the application gracefully handles missing API keys at runtime (e.g., shows a configuration prompt, disables features, or provides clear error messages).
🏁 Script executed:
#!/bin/bash
# Description: Check how the app handles empty API keys in map provider logic
rg -nP --type=ts --type=tsx -C3 'GOOGLE_MAPS_API_KEY|OSM_API_KEY' -g '!node_modules' -g '!*.json'Length of output: 27
🏁 Script executed:
#!/bin/bash
# Search for API key references across code
rg -n 'GOOGLE_MAPS_API_KEY|OSM_API_KEY' --glob '**/*.{ts,tsx,js,jsx}' --glob '!node_modules/**'Length of output: 1264
🏁 Script executed:
#!/bin/bash
rg -n "import .* from 'expo-constants'" -C3 --glob "**/*.{ts,tsx,js,jsx}" --glob "!node_modules/**"Length of output: 0
Implement runtime loading and graceful fallback for map API keys
I didn’t find any code consuming the GOOGLE_MAPS_API_KEY/OSM_API_KEY values from your app.json extras. Import Constants from expo-constants, read Constants.expoConfig.extra?.GOOGLE_MAPS_API_KEY (or Constants.manifest.extra for SDK < 48), merge with your useUserPreferences values, and disable map features or prompt for key entry when both are empty.
app/containers/MessageComposer/components/Buttons/ActionsButton.tsx
Outdated
Show resolved
Hide resolved
| const openLivePreview = async (provider: MapProviderName) => { | ||
| try { | ||
| if (!rid) { | ||
| showErrorAlert(I18n.t('Room_not_available'), I18n.t('Oops')); | ||
| return; | ||
| } | ||
|
|
||
| if (Platform.OS === 'android') { | ||
| const res = await PermissionsAndroid.requestMultiple([ | ||
| PermissionsAndroid.PERMISSIONS.ACCESS_FINE_LOCATION, | ||
| PermissionsAndroid.PERMISSIONS.ACCESS_COARSE_LOCATION | ||
| ]); | ||
| const fine = res[PermissionsAndroid.PERMISSIONS.ACCESS_FINE_LOCATION]; | ||
| const coarse = res[PermissionsAndroid.PERMISSIONS.ACCESS_COARSE_LOCATION]; | ||
| if (fine !== PermissionsAndroid.RESULTS.GRANTED && coarse !== PermissionsAndroid.RESULTS.GRANTED) { | ||
| throw new Error(I18n.t('Permission_denied')); | ||
| } | ||
| } else { | ||
| const { status } = await Location.requestForegroundPermissionsAsync(); | ||
| if (status !== 'granted') { | ||
| throw new Error(I18n.t('Location_permission_required')); | ||
| } | ||
| } | ||
|
|
||
| const params = { | ||
| rid, | ||
| tmid, | ||
| provider, | ||
| googleKey: provider === 'google' ? googleApiKey : undefined, | ||
| osmKey: provider === 'osm' ? osmApiKey : undefined | ||
| }; | ||
|
|
||
| // Defer navigation until after sheets/animations are done | ||
| InteractionManager.runAfterInteractions(() => { | ||
| // @ts-ignore | ||
| if (isMasterDetail) { | ||
| Navigation.navigate('ModalStackNavigator', { screen: 'LiveLocationPreviewModal', params }); | ||
| } else { | ||
| Navigation.navigate('LiveLocationPreviewModal', params); | ||
| } | ||
| }); | ||
| } catch (e: any) { | ||
| showErrorAlert(e?.message || I18n.t('Could_not_get_location'), I18n.t('Oops')); | ||
| } | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove @ts-ignore by updating navigation types.
The live location preview flow is well-structured, but the @ts-ignore on line 151 indicates missing type definitions for LiveLocationPreviewModal in the navigation param list.
Ensure that the navigation types (e.g., in app/stacks/types.ts or app/stacks/MasterDetailStack/types.ts) include:
LiveLocationPreviewModal: {
rid: string;
tmid?: string;
provider: MapProviderName;
googleKey?: string;
osmKey?: string;
liveLocationId?: string;
ownerName?: string;
isTracking?: boolean;
};This will eliminate the need for @ts-ignore and provide type safety.
| @@ -0,0 +1,61 @@ | |||
| import * as Location from 'expo-location'; | |||
|
|
|||
| export type Coords = { latitude: number; longitude: number; accuracy?: number; timestamp?: number }; | |||
There was a problem hiding this comment.
Duplicate type definition - consolidate to avoid drift.
The Coords type is defined here and also in mapProviders.ts (line 11). The definitions differ: this version includes optional accuracy and timestamp fields, while mapProviders.ts only has latitude and longitude.
Consolidate to a single shared type definition to prevent drift and inconsistencies. Consider defining the complete type in a shared location (e.g., a types file or in mapProviders.ts) and importing it here.
-export type Coords = { latitude: number; longitude: number; accuracy?: number; timestamp?: number };
+import { Coords } from './mapProviders';Then update mapProviders.ts to include the optional fields:
export type Coords = {
latitude: number;
longitude: number;
accuracy?: number;
timestamp?: number
};🤖 Prompt for AI Agents
In app/views/LocationShare/services/staticLocation.ts around line 3: there's a
duplicate Coords type here that differs from the one in mapProviders.ts;
consolidate by moving the complete Coords definition (latitude, longitude,
optional accuracy and timestamp) into a single shared file (or into
mapProviders.ts), update mapProviders.ts to export the full type with optional
fields, then replace the local type in this file with an import of that shared
Coords type and remove the duplicate definition to prevent drift.
| <key>NSExceptionDomains</key> | ||
| <dict> | ||
| <key>maps.locationiq.com</key> | ||
| <dict> | ||
| <key>NSIncludesSubdomains</key> | ||
| <true/> | ||
| <key>NSExceptionAllowsInsecureHTTPLoads</key> | ||
| <true/> | ||
| <key>NSExceptionRequiresForwardSecrecy</key> | ||
| <false/> | ||
| </dict> | ||
| </dict> |
There was a problem hiding this comment.
🧩 Analysis chain
Security concern: Insecure HTTP loads allowed for maps.locationiq.com.
The configuration allows insecure HTTP loads (NSExceptionAllowsInsecureHTTPLoads = true) and disables forward secrecy for maps.locationiq.com. This weakens transport security and exposes location-related traffic to potential interception.
Verify whether LocationIQ's API supports HTTPS endpoints. If HTTPS is available, remove this exception and use secure connections.
🌐 Web query:
Does LocationIQ maps API support HTTPS endpoints?
💡 Result:
Yes — LocationIQ APIs and map tile/style endpoints use HTTPS (for example: https://us1.locationiq.com/v1/... and https://tiles.locationiq.com/...). [1][2]
Sources:
- LocationIQ API reference (endpoints shown with https). [1]
- LocationIQ homepage / Maps docs (tiles/style URLs use https). [2]
Remove NSExceptionAllowsInsecureHTTPLoads for maps.locationiq.com in Info.plist
LocationIQ supports HTTPS endpoints; drop the insecure‐HTTP exception (and the domain exception block if no other overrides are needed) to enforce secure connections.
🤖 Prompt for AI Agents
In ios/RocketChatRN/Info.plist around lines 70 to 81, the
NSExceptionAllowsInsecureHTTPLoads entry for maps.locationiq.com should be
removed to enforce HTTPS; either delete the NSExceptionAllowsInsecureHTTPLoads
key/value under the maps.locationiq.com dict, or if there are no other
domain-specific overrides needed, remove the entire maps.locationiq.com domain
dict from NSExceptionDomains so the app will use default secure ATS behavior.
yiweigao0226
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
yiweigao0226
requested a deployment
to
approve_e2e_testing
GitHub Actions
Waiting
* [FIX] Border style * [FIX] Refactoring * [FIX] fix color of border * Undo Co-authored-by: Aroo <[email protected]> Co-authored-by: Diego Mello <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 8
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
app/containers/message/Components/LiveLocationCard.tsx(1 hunks)app/containers/message/Content.tsx(2 hunks)app/views/LocationShare/LiveLocationPreviewModal.tsx(1 hunks)app/views/LocationShare/services/handleLiveLocationUrl.ts(1 hunks)app/views/LocationShare/services/liveLocation.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
app/containers/message/Components/LiveLocationCard.tsx (8)
app/theme.tsx (1)
useTheme(29-29)app/lib/hooks/useAppSelector.ts (1)
useAppSelector(6-6)app/lib/methods/userPreferences.ts (1)
useUserPreferences(10-10)app/views/LocationShare/services/mapProviders.ts (1)
MapProviderName(7-7)app/lib/constants/keys.ts (4)
MAP_PROVIDER_PREFERENCE_KEY(24-24)MAP_PROVIDER_DEFAULT(27-27)GOOGLE_MAPS_API_KEY_PREFERENCE_KEY(25-25)OSM_API_KEY_PREFERENCE_KEY(26-26)app/views/LocationShare/LiveLocationPreviewModal.tsx (5)
getCurrentLiveParams(58-60)isLiveLocationActive(470-472)addStatusChangeListener(61-63)removeStatusChangeListener(64-66)reopenLiveLocationModal(474-482)app/views/LocationShare/services/handleLiveLocationUrl.ts (3)
isLiveLocationEnded(62-65)addLiveLocationEndedListener(14-16)removeLiveLocationEndedListener(18-20)app/lib/constants/colors.ts (1)
themes(304-304)
app/views/LocationShare/services/handleLiveLocationUrl.ts (1)
app/views/LocationShare/LiveLocationPreviewModal.tsx (3)
isLiveLocationActive(470-472)getCurrentLiveParams(58-60)reopenLiveLocationModal(474-482)
app/views/LocationShare/services/liveLocation.ts (1)
app/views/LocationShare/services/mapProviders.ts (1)
MapProviderName(7-7)
app/containers/message/Content.tsx (3)
app/containers/message/interfaces.ts (1)
IMessageContent(46-68)app/definitions/IMessage.ts (1)
IUserMessage(23-27)app/views/LocationShare/services/handleLiveLocationUrl.ts (1)
markLiveLocationAsEnded(52-60)
app/views/LocationShare/LiveLocationPreviewModal.tsx (5)
app/views/LocationShare/services/mapProviders.ts (4)
MapProviderName(7-7)staticMapUrl(98-113)mapsDeepLink(165-191)providerLabel(196-196)app/views/LocationShare/services/liveLocation.ts (5)
LiveLocationTracker(14-115)LiveLocationState(4-12)generateLiveLocationId(117-119)createLiveLocationMessage(121-141)createLiveLocationStopMessage(143-149)app/lib/hooks/useAppSelector.ts (1)
useAppSelector(6-6)app/selectors/login.ts (1)
getUserSelector(20-20)app/views/LocationShare/services/handleLiveLocationUrl.ts (4)
isLiveLocationEnded(62-65)addLiveLocationEndedListener(14-16)removeLiveLocationEndedListener(18-20)markLiveLocationAsEnded(52-60)
🪛 ESLint
app/containers/message/Components/LiveLocationCard.tsx
[error] 2-2: There should be at least one empty line between import groups
(import/order)
[error] 4-4: '/home/jailuser/git/app/lib/constants/index.ts' imported multiple times.
(import/no-duplicates)
[error] 25-25: '/home/jailuser/git/app/lib/constants/index.ts' imported multiple times.
(import/no-duplicates)
app/views/LocationShare/services/handleLiveLocationUrl.ts
[error] 47-49: Empty block statement.
(no-empty)
app/views/LocationShare/services/liveLocation.ts
[error] 1-1: There should be at least one empty line between import groups
(import/order)
app/containers/message/Content.tsx
[error] 52-52: 'messageTs' is assigned a value but never used.
(@typescript-eslint/no-unused-vars)
app/views/LocationShare/LiveLocationPreviewModal.tsx
[error] 3-3: There should be at least one empty line between import groups
(import/order)
[error] 4-4: There should be no empty line within import group
(import/order)
[error] 23-23: There should be at least one empty line between import groups
(import/order)
[error] 24-24: react-redux import should occur before import of ../../i18n
(import/order)
[error] 326-326: Expected 'undefined' and instead saw 'void'.
(no-void)
[error] 437-463: Do not nest ternary expressions.
(no-nested-ternary)
[error] 498-498: Empty block statement.
(no-empty)
| const staleCheck = setInterval(() => { | ||
| if (isMessageTooOld(messageTimestamp) && cardIsActive) { | ||
| setCardIsActive(false); | ||
| } | ||
| }, 5 * 60 * 1000); |
There was a problem hiding this comment.
Stale interval sees outdated cardIsActive.
cardIsActive in this interval is frozen to the value from the render that created the effect, so if the card becomes active later (e.g., status-change listener toggles it on) the stale-check never runs. Use a ref or include the state in the effect dependencies and read the latest value before toggling.
🤖 Prompt for AI Agents
In app/containers/message/Components/LiveLocationCard.tsx around lines 170 to
174, the setInterval callback closes over the stale cardIsActive value from the
render that created the effect, so later changes to cardIsActive are ignored;
fix by ensuring the interval reads the latest value — either replace
cardIsActive usage with a mutable ref (create a cardIsActiveRef, update it in
state setter and read cardIsActiveRef.current inside the interval) or include
cardIsActive in the effect dependency array and clear/recreate the interval when
it changes so the callback always has the current state; also ensure you clear
the interval on unmount to avoid leaks.
app/containers/message/Content.tsx
Outdated
| // Derive a timestamp safely without changing IMessageContent | ||
| const anyProps = props as any; | ||
| const messageTs: string | Date | number | undefined = | ||
| ts ?? | ||
| anyProps?.ts ?? | ||
| anyProps?._updatedAt ?? | ||
| anyProps?.updatedAt ?? | ||
| undefined; | ||
|
|
||
| if (isLiveLocationMessage && props.msg) { | ||
| content = ( | ||
| <LiveLocationCard | ||
| msg={props.msg} | ||
| isActive={true} | ||
| author={props.author} | ||
| /> | ||
| ); | ||
| } else if (isLiveLocationStopMessage && props.msg) { |
There was a problem hiding this comment.
Pass the timestamp to LiveLocationCard.
You compute messageTs but never use it, so ESLint flags the variable and cards never receive their timestamp to age out stale sessions. Forward messageTimestamp={messageTs} (after null checks) and drop the eslint warning.
🧰 Tools
🪛 ESLint
[error] 52-52: 'messageTs' is assigned a value but never used.
(@typescript-eslint/no-unused-vars)
🤖 Prompt for AI Agents
In app/containers/message/Content.tsx around lines 50 to 67, you compute
messageTs but never pass it to LiveLocationCard; forward the derived timestamp
by adding a messageTimestamp={messageTs} prop to the LiveLocationCard (ensure
you pass only non-undefined values or allow undefined if the component accepts
it), remove or replace any eslint-unused-variable warning (either use the
variable as described or prefix with /* eslint-disable-next-line no-unused-vars
*/ if absolutely needed), and keep existing null checks so stale live-location
sessions can age out correctly.
| <List.Info info='Map_Provider_Info' /> | ||
| </List.Section> | ||
|
|
||
| <List.Section title='API_Keys'> |
There was a problem hiding this comment.
Asking all users to login on Google Console to get API Keys is not a good approach. We should always try to use native APIs.
package.json
Outdated
| @@ -103,6 +104,7 @@ | |||
| "react-native-easy-grid": "0.2.2", | |||
| "react-native-easy-toast": "2.3.0", | |||
| "react-native-file-viewer": "2.1.4", | |||
| "react-native-geolocation-service": "^5.3.1", | |||
There was a problem hiding this comment.
It doesn't look like you're using this anymore.
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
app/containers/message/Components/LiveLocationCard.tsx (1)
170-175: Fix stalecardIsActivecapture in the interval.Line 172 references
cardIsActivecaptured from the render that created the effect, so once the state flips (e.g., listener marks the card active) the interval still sees the old value and never deactivates the card when the message ages out. Result: ended sessions can stay “Active” indefinitely. Use a ref (or recreate the interval whencardIsActivechanges) so the callback reads the latest state before deciding.const LiveLocationCard: React.FC<LiveLocationCardProps> = ({ msg, isActive = true, messageTimestamp, author, onPress }) => { const { theme } = useTheme(); const [cardIsActive, setCardIsActive] = useState(isActive); + const cardIsActiveRef = useRef(cardIsActive); + + useEffect(() => { + cardIsActiveRef.current = cardIsActive; + }, [cardIsActive]); @@ - const staleCheck = setInterval(() => { - if (isMessageTooOld(messageTimestamp) && cardIsActive) { + const staleCheck = setInterval(() => { + if (isMessageTooOld(messageTimestamp) && cardIsActiveRef.current) { setCardIsActive(false); } }, 5 * 60 * 1000);
🧹 Nitpick comments (6)
app/views/LocationShare/services/liveLocation.ts (5)
88-99: Add error logging for debugging and monitoring.The error is caught but not logged, making it difficult to diagnose failures when the server API is unavailable. Consider logging the error before falling back to non-server mode.
Apply this diff to add error logging:
} catch (error: any) { + console.error('[LiveLocation] Failed to start server-side tracking:', error); this.useServerApi = false;
151-158: Log error before stopping tracker.When live-location-not-found errors occur, logging them would help diagnose issues with session management and provide visibility into why tracking was stopped.
Apply this diff:
} catch (error: any) { if (error?.error === 'error-live-location-not-found' || error?.message?.includes('live-location-not-found')) { + console.warn('[LiveLocation] Session not found on server, stopping tracker:', error); this.useServerApi = false;
193-203: Add error logging for stop failures.Failures during stop operations should be logged to help diagnose cleanup issues and potential resource leaks.
Apply this diff:
} catch (error) { - // Failed to stop on server + console.error('[LiveLocation] Failed to stop on server:', error); } } else { // Send stop message for fallback mode const stopMessage = this.createLiveLocationMessage(this.currentState.coords, 'stop'); try { await sendMessage(this.rid, stopMessage, this.tmid, this.user, false); } catch (error) { - // Failed to send stop message + console.error('[LiveLocation] Failed to send stop message:', error); }
257-277: Remove or utilize unused parameters.Parameters
_serverUrl,rid, andtmidare unused increateLiveLocationMessage. Theridandtmidparameters are included in the deep-link query but not used in the message body. If they're truly unnecessary, remove them; otherwise, document their purpose or use them meaningfully.If these parameters are for future use or external consistency, add a comment explaining why they're present but unused. Otherwise, remove them:
export function createLiveLocationMessage( liveLocationId: string, provider: MapProviderName, coords: { latitude: number; longitude: number }, - _serverUrl: string, - rid?: string, - tmid?: string ): string { const params = new URLSearchParams({ liveLocationId, - rid: rid || '', - tmid: tmid || '', provider, action: 'reopen' });Or, if they're intentional for the deep-link routing, add a comment:
export function createLiveLocationMessage( liveLocationId: string, provider: MapProviderName, coords: { latitude: number; longitude: number }, _serverUrl: string, // Reserved for future server URL customization rid?: string, // Room ID for deep-link routing tmid?: string // Thread message ID for deep-link routing ): string {
279-285: Remove unused parameters from stop message helper.The
_providerand_lastCoordsparameters are prefixed with underscore but never used. If they're not needed, remove them to simplify the function signature.Apply this diff:
export function createLiveLocationStopMessage( - liveLocationId: string, - _provider: MapProviderName, - _lastCoords: { latitude: number; longitude: number } + liveLocationId: string ): string { return `📍 **Live Location Ended** (ID: ${liveLocationId})`; }app/containers/message/Components/Attachments/Attachments.tsx (1)
100-115: Consider extending IAttachment type for live-location fields.The type assertion
(file as any)at lines 100 and 105 suggests that theIAttachmentinterface doesn't include thelivefield or other live-location-specific properties. Consider extending the interface to include these fields for better type safety.In
app/definitions/IAttachment.ts, add live-location fields:export interface IAttachment { // ... existing fields ... // Live location specific fields live?: { id: string; ownerId: string; ownerUsername: string; isActive: boolean; coords: { lat: number; lng: number; acc?: number; }; lastUpdateAt: Date; // ... other live location fields }; }Then remove the type assertions in this file:
- if (file.type === 'live-location' && (file as any).live) { + if (file.type === 'live-location' && file.live) { return ( <LiveLocationAttachment key={`live-location-${index}`} - attachment={file as any} + attachment={file}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (21)
app/containers/MessageComposer/components/Buttons/ActionsButton.tsx(4 hunks)app/containers/message/Components/Attachments/Attachments.tsx(2 hunks)app/containers/message/Components/Attachments/LiveLocationAttachment.tsx(1 hunks)app/containers/message/Components/LiveLocationCard.tsx(1 hunks)app/containers/message/Content.tsx(1 hunks)app/containers/message/Message.tsx(3 hunks)app/containers/message/interfaces.ts(1 hunks)app/index.tsx(0 hunks)app/stacks/InsideStack.tsx(4 hunks)app/stacks/types.ts(2 hunks)app/views/LocationShare/LiveLocationPreviewModal.tsx(1 hunks)app/views/LocationShare/LiveLocationStatusBar.tsx(1 hunks)app/views/LocationShare/LiveLocationViewerModal.tsx(1 hunks)app/views/LocationShare/LocationPreviewModal.tsx(1 hunks)app/views/LocationShare/services/handleLiveLocationUrl.ts(1 hunks)app/views/LocationShare/services/liveLocation.ts(1 hunks)app/views/LocationShare/services/liveLocationApi.ts(1 hunks)app/views/LocationShare/services/mapProviders.ts(1 hunks)app/views/LocationShare/services/staticLocation.ts(1 hunks)app/views/RoomView/index.tsx(10 hunks)package.json(1 hunks)
💤 Files with no reviewable changes (1)
- app/index.tsx
🚧 Files skipped from review as they are similar to previous changes (3)
- app/containers/message/Content.tsx
- app/views/LocationShare/services/staticLocation.ts
- app/views/LocationShare/services/handleLiveLocationUrl.ts
🧰 Additional context used
🧬 Code graph analysis (13)
app/views/LocationShare/LiveLocationStatusBar.tsx (4)
app/lib/hooks/useAppSelector.ts (1)
useAppSelector(6-6)app/selectors/login.ts (1)
getUserSelector(20-20)app/views/LocationShare/LiveLocationPreviewModal.tsx (6)
isLiveLocationMinimized(58-60)addMinimizedStatusListener(62-64)removeMinimizedStatusListener(66-68)reopenLiveLocationModal(544-557)getCurrentLiveParams(54-56)stopGlobalLiveLocation(559-595)app/views/LocationShare/services/handleLiveLocationUrl.ts (2)
isLiveMessageLink(67-69)handleLiveLocationUrl(71-110)
app/containers/message/Components/LiveLocationCard.tsx (8)
app/theme.tsx (1)
useTheme(29-29)app/lib/hooks/useAppSelector.ts (1)
useAppSelector(6-6)app/lib/methods/userPreferences.ts (1)
useUserPreferences(10-10)app/views/LocationShare/services/mapProviders.ts (1)
MapProviderName(7-7)app/lib/constants/keys.ts (4)
MAP_PROVIDER_PREFERENCE_KEY(24-24)MAP_PROVIDER_DEFAULT(27-27)GOOGLE_MAPS_API_KEY_PREFERENCE_KEY(25-25)OSM_API_KEY_PREFERENCE_KEY(26-26)app/views/LocationShare/LiveLocationPreviewModal.tsx (5)
getCurrentLiveParams(54-56)isLiveLocationActive(540-542)addStatusChangeListener(80-82)removeStatusChangeListener(83-85)reopenLiveLocationModal(544-557)app/views/LocationShare/services/handleLiveLocationUrl.ts (3)
isLiveLocationEnded(62-65)addLiveLocationEndedListener(14-16)removeLiveLocationEndedListener(18-20)app/lib/constants/colors.ts (1)
themes(304-304)
app/containers/MessageComposer/components/Buttons/ActionsButton.tsx (7)
app/lib/hooks/useAppSelector.ts (1)
useAppSelector(6-6)app/lib/methods/userPreferences.ts (1)
useUserPreferences(10-10)app/views/LocationShare/services/mapProviders.ts (1)
MapProviderName(7-7)app/lib/constants/keys.ts (4)
MAP_PROVIDER_PREFERENCE_KEY(24-24)MAP_PROVIDER_DEFAULT(27-27)GOOGLE_MAPS_API_KEY_PREFERENCE_KEY(25-25)OSM_API_KEY_PREFERENCE_KEY(26-26)app/lib/methods/helpers/info.ts (1)
showErrorAlert(5-6)app/views/LocationShare/services/staticLocation.ts (1)
getCurrentPositionOnce(24-59)app/containers/ActionSheet/Provider.tsx (1)
TActionSheetOptionsItem(7-15)
app/containers/message/Message.tsx (1)
app/containers/message/Components/Attachments/components.ts (1)
Attachments(4-4)
app/containers/message/Components/Attachments/LiveLocationAttachment.tsx (4)
app/definitions/IAttachment.ts (1)
IAttachment(9-48)app/theme.tsx (1)
useTheme(29-29)app/lib/hooks/useAppSelector.ts (1)
useAppSelector(6-6)app/views/LocationShare/LiveLocationPreviewModal.tsx (2)
reopenLiveLocationModal(544-557)isLiveLocationActive(540-542)
app/containers/message/Components/Attachments/Attachments.tsx (2)
app/definitions/IAttachment.ts (1)
IAttachment(9-48)app/containers/message/interfaces.ts (1)
IMessageAttachments(10-20)
app/views/LocationShare/LiveLocationViewerModal.tsx (5)
app/lib/hooks/useAppSelector.ts (1)
useAppSelector(6-6)app/lib/methods/userPreferences.ts (1)
useUserPreferences(10-10)app/views/LocationShare/services/mapProviders.ts (4)
MapProviderName(7-7)staticMapUrl(88-103)mapsDeepLink(143-168)providerLabel(173-173)app/lib/constants/keys.ts (4)
MAP_PROVIDER_PREFERENCE_KEY(24-24)MAP_PROVIDER_DEFAULT(27-27)GOOGLE_MAPS_API_KEY_PREFERENCE_KEY(25-25)OSM_API_KEY_PREFERENCE_KEY(26-26)app/views/LocationShare/services/liveLocationApi.ts (2)
LiveLocationApi(45-89)serverToMobileCoords(105-111)
app/stacks/InsideStack.tsx (2)
app/views/LocationShare/LocationPreviewModal.tsx (1)
LocationPreviewModal(24-135)app/views/LocationShare/LiveLocationPreviewModal.tsx (1)
LiveLocationPreviewModal(97-538)
app/views/LocationShare/LocationPreviewModal.tsx (5)
app/views/LocationShare/services/mapProviders.ts (5)
Coords(12-12)MapProviderName(7-7)staticMapUrl(88-103)mapsDeepLink(143-168)providerLabel(173-173)app/views/LocationShare/services/staticLocation.ts (1)
Coords(3-3)app/lib/hooks/useAppSelector.ts (1)
useAppSelector(6-6)app/selectors/login.ts (1)
getUserSelector(20-20)app/lib/methods/sendMessage.ts (1)
sendMessage(87-241)
app/views/LocationShare/services/mapProviders.ts (1)
app/views/LocationShare/services/staticLocation.ts (1)
Coords(3-3)
app/views/LocationShare/LiveLocationPreviewModal.tsx (5)
app/views/LocationShare/services/mapProviders.ts (4)
MapProviderName(7-7)staticMapUrl(88-103)mapsDeepLink(143-168)providerLabel(173-173)app/views/LocationShare/services/liveLocation.ts (2)
LiveLocationTracker(18-251)LiveLocationState(7-16)app/lib/hooks/useAppSelector.ts (1)
useAppSelector(6-6)app/selectors/login.ts (1)
getUserSelector(20-20)app/views/LocationShare/services/handleLiveLocationUrl.ts (4)
isLiveLocationEnded(62-65)addLiveLocationEndedListener(14-16)removeLiveLocationEndedListener(18-20)markLiveLocationAsEnded(52-60)
app/views/LocationShare/services/liveLocation.ts (2)
app/views/LocationShare/services/liveLocationApi.ts (2)
LiveLocationApi(45-89)mobileToServerCoords(94-100)app/views/LocationShare/services/mapProviders.ts (1)
MapProviderName(7-7)
app/views/RoomView/index.tsx (2)
app/lib/services/restApi.ts (5)
getUserInfo(1056-1059)editMessage(955-959)setReaction(322-324)joinRoom(272-278)toggleFollowMessage(774-780)app/views/LocationShare/LiveLocationStatusBar.tsx (1)
LiveLocationStatusBar(26-124)
🪛 Biome (2.1.2)
app/views/LocationShare/services/mapProviders.ts
[error] 147-147: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
🔇 Additional comments (8)
package.json (1)
77-77: LGTM: Clean dependency addition for location features.The
expo-locationpackage is correctly added and aligns with the PR's location-sharing functionality.app/containers/message/interfaces.ts (1)
18-19: LGTM: Context fields enable proper attachment linking.Adding
messageIdandroomIdtoIMessageAttachmentsallows LiveLocationAttachment and other components to navigate correctly while maintaining backward compatibility.app/views/LocationShare/services/liveLocationApi.ts (1)
1-111: LGTM: Clean API wrapper with good documentation.The
LiveLocationApiclass provides a well-structured interface for server-side live location operations. The JSDoc comments clearly document parameters and return types, and the coordinate conversion utilities (mobileToServerCoordsandserverToMobileCoords) maintain clean separation between mobile and server coordinate formats.app/containers/message/Message.tsx (1)
42-42: LGTM: Consistent prop propagation across rendering branches.The addition of
messageIdandroomIdto all threeAttachmentsusages (preview, default content, and pinned-info branch) ensures LiveLocationAttachment and other components receive the necessary context regardless of the rendering path.Also applies to: 90-90, 177-177
app/views/RoomView/index.tsx (2)
12-19: LGTM: Clean import consolidation and component addition.Replacing indirect
Services.*calls with direct imports fromlib/services/restApiimproves clarity and reduces indirection. The addition ofLiveLocationStatusBarintegrates cleanly with the existing SafeAreaView structure.Also applies to: 57-57
1538-1538: LGTM: LiveLocationStatusBar properly integrated.The status bar is correctly placed within SafeAreaView and will display above the room content when live location sharing is active.
app/containers/message/Components/Attachments/Attachments.tsx (1)
17-17: LGTM: removeQuote correctly handles live-location attachments.Including
file?.type === 'live-location'in theremoveQuotefilter ensures live-location attachments are treated as non-quote content and rendered appropriately.app/stacks/InsideStack.tsx (1)
79-82: LGTM: Clean navigation structure for location features.The addition of
LocationPreferencesViewto the ProfileStack and the three location modals (LocationPreviewModal,LiveLocationPreviewModal,LiveLocationViewerModal) to the InsideStack follows existing navigation patterns. The use of modal presentation and localized titles viaI18n.t()is consistent with the rest of the codebase.Also applies to: 173-173, 327-341
| console.log('🔍 [Attachments] Processing attachments:', attachments?.length || 0); | ||
| if (attachments) { | ||
| attachments.forEach((att, idx) => { | ||
| console.log(`🔍 [Attachments] Attachment ${idx}:`, { | ||
| type: att.type, | ||
| hasLive: !!(att as any).live, | ||
| keys: Object.keys(att) | ||
| }); | ||
| }); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove debug console.log statements before merging.
Multiple console.log statements are present throughout the file for debugging attachment processing. These should be removed to avoid cluttering production logs and potentially exposing sensitive attachment metadata.
Apply this diff to remove all debug logs:
- console.log('🔍 [Attachments] Processing attachments:', attachments?.length || 0);
if (attachments) {
- attachments.forEach((att, idx) => {
- console.log(`🔍 [Attachments] Attachment ${idx}:`, {
- type: att.type,
- hasLive: !!(att as any).live,
- keys: Object.keys(att)
- });
- });
}
-
const nonQuoteAttachments = attachments?.filter(removeQuote);
- console.log('🔍 [Attachments] Non-quote attachments:', nonQuoteAttachments?.length || 0);
-
if (!nonQuoteAttachments || nonQuoteAttachments.length === 0) {
- console.log('🔍 [Attachments] No attachments to render');
return null;
}And around line 99:
- // Handle live location attachments
- console.log('🔍 [Attachments] Checking file:', { type: file.type, hasLive: !!(file as any).live });
if (file.type === 'live-location' && (file as any).live) {
- console.log('✅ [Attachments] Rendering LiveLocationAttachment!');
return (Also applies to: 36-36, 39-39, 99-99, 101-101
🤖 Prompt for AI Agents
In app/containers/message/Components/Attachments/Attachments.tsx around lines
23-32 (and also at lines 36, 39, 99, 101), remove the debug console.log
statements that print attachment processing and metadata; replace them with
no-op or, if runtime telemetry is required, use the existing project logger
(e.g., logger.debug(...)) consistent with app logging conventions; ensure no
other console.log calls remain in this file before merging.
| useEffect(() => { | ||
| if (!isTracking && liveLocationId) { | ||
| isLiveLocationEnded(liveLocationId).then(ended => { | ||
| if (ended) { | ||
| const inactiveState = { | ||
| coords: { | ||
| latitude: 37.78583, | ||
| longitude: -122.40642, | ||
| accuracy: 5 | ||
| }, | ||
| timestamp: Date.now() - (5 * 60 * 1000), // 5 minutes ago to show it's old | ||
| isActive: false | ||
| }; | ||
| handleLocationUpdate(inactiveState); | ||
| } else { | ||
| const activeState = { | ||
| coords: { | ||
| latitude: 37.78583, | ||
| longitude: -122.40642, | ||
| accuracy: 5 | ||
| }, | ||
| timestamp: Date.now(), | ||
| isActive: true | ||
| }; | ||
| handleLocationUpdate(activeState); | ||
| } | ||
| }); | ||
|
|
||
| const handleLiveLocationEnded = (endedLocationId: string) => { | ||
| if (endedLocationId === liveLocationId) { | ||
| const inactiveState = { | ||
| coords: { | ||
| latitude: 37.78583, | ||
| longitude: -122.40642, | ||
| accuracy: 5 | ||
| }, | ||
| timestamp: Date.now() - (5 * 60 * 1000), | ||
| isActive: false | ||
| }; | ||
| handleLocationUpdate(inactiveState); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Live viewer never loads real coordinates
When isTracking is false, the modal sets hard-coded coordinates (37.78583/-122.40642) for both active and ended states and never starts the intended polling logic (viewerUpdateIntervalRef). As a result, anyone opening a live-location message while not the owner always sees a San Francisco placeholder instead of the actual shared position—live viewing is effectively broken.
Please hook this branch into the real data source (e.g., fetch/poll via LiveLocationApi.get or subscribe to live updates) and update viewerUpdateIntervalRef accordingly so viewers receive actual coordinates.
🤖 Prompt for AI Agents
In app/views/LocationShare/LiveLocationPreviewModal.tsx around lines 170–211,
the effect uses hard-coded coords instead of fetching real live-location data
and never starts the viewerUpdateIntervalRef polling; replace the placeholder
branches with a call to the real data source (e.g., await
LiveLocationApi.get(liveLocationId) or subscribe to live updates) and call
handleLocationUpdate with the returned coords/timestamp/isActive; then start a
polling/subscription that stores its handle in viewerUpdateIntervalRef (e.g.,
setInterval ID or unsubscribe function) so updates are applied periodically, and
add proper cleanup (clearInterval or unsubscribe) in the effect cleanup; also
update the handleLiveLocationEnded callback to fetch/update the latest state
from the API instead of using hard-coded coordinates.
| <View style={styles.mapPlaceholder}> | ||
| <Text style={[styles.loadingText, { fontSize: 16, fontWeight: 'bold', marginBottom: 8 }]}>📠Map Preview</Text> | ||
| {locationState?.coords && ( | ||
| <Text style={[styles.loadingText, { fontSize: 14, marginBottom: 8 }]}> | ||
| {locationState.coords.latitude.toFixed(5)}, {locationState.coords.longitude.toFixed(5)} | ||
| </Text> | ||
| )} | ||
| <Text style={[styles.loadingText, { fontSize: 12, fontStyle: 'italic', textAlign: 'center' }]}> | ||
| Map preview unavailable{'\n'}Tap "Open in Maps" below to view location | ||
| </Text> |
There was a problem hiding this comment.
Fix the garbled map placeholder text
The placeholder currently renders the mojibake string ðŸ“� Map Preview; users will see gibberish. Replace it with the intended character (e.g., 📍 Map Preview) so the label displays correctly.
🤖 Prompt for AI Agents
In app/views/LocationShare/LiveLocationPreviewModal.tsx around lines 515 to 524,
the map placeholder header currently contains a mojibake string ("� Map
Preview"); replace that literal with the intended text, e.g. "📍 Map Preview",
ensuring the file is saved with UTF-8 encoding so the emoji renders correctly
across platforms.
| private createLiveLocationMessage( | ||
| coords: { latitude: number; longitude: number; accuracy?: number }, | ||
| type: 'start' | 'stop' | ||
| ): string { | ||
| const params = new URLSearchParams({ | ||
| liveLocationId: this.liveLocationId, | ||
| rid: this.rid, | ||
| tmid: this.tmid || '', | ||
| provider: 'osm' as MapProviderName, | ||
| action: 'reopen' | ||
| }); | ||
| const appDeepLink = `rocketchat://live-location?${params.toString()}`; | ||
|
|
||
| if (type === 'start') { | ||
| return `📍 **Live Location Start** | ||
| [🔴 View Live Location](${appDeepLink}) | ||
| Coordinates: ${coords.latitude.toFixed(6)}, ${coords.longitude.toFixed(6)}`; | ||
| } | ||
| return `📍 **Live Location Ended** (ID: ${this.liveLocationId})`; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove hardcoded provider and use instance configuration.
Line 237 hardcodes 'osm' as MapProviderName instead of using the provider from the instance or allowing it to be configured. This limits flexibility and doesn't align with the broader map provider preference system introduced in the PR.
Consider one of these approaches:
- Add a
providerfield to the LiveLocationTracker constructor and use it here - Accept provider as a parameter to
createLiveLocationMessage - Use the default from user preferences
For example, if adding to constructor:
private liveLocationId: string;
+ private provider: MapProviderName;
constructor(
rid: string,
tmid: string | undefined,
user: { id: string; username: string },
onUpdate: (state: LiveLocationState) => void,
- durationSec?: number
+ durationSec?: number,
+ provider: MapProviderName = 'google'
) {
this.rid = rid;
this.tmid = tmid;
this.user = user;
this.onLocationUpdate = onUpdate;
this.durationSec = durationSec;
+ this.provider = provider;
this.liveLocationId = `live_${Date.now()}_${Math.random().toString(36).slice(2, 11)}`;
}Then use this.provider at line 237 instead of hardcoded 'osm'.
Committable suggestion skipped: line range outside the PR's diff.
| case 'google': | ||
| const url = await iosGoogleLink(coords); | ||
| return url; | ||
| case 'osm': | ||
| return iosOsmLink(coords); | ||
| case 'apple': | ||
| return iosAppleLink(coords); | ||
| default: | ||
| return iosAppleLink(coords); | ||
| } |
There was a problem hiding this comment.
Wrap the iOS Google case in a block
Declaring const url directly inside the case 'google' branch violates the no-switch-declarations rule, so CI will fail. Wrap that branch in its own block:
- case 'google':
- const url = await iosGoogleLink(coords);
- return url;
+ case 'google': {
+ const url = await iosGoogleLink(coords);
+ return url;
+ }As per static analysis hints
📝 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.
| case 'google': | |
| const url = await iosGoogleLink(coords); | |
| return url; | |
| case 'osm': | |
| return iosOsmLink(coords); | |
| case 'apple': | |
| return iosAppleLink(coords); | |
| default: | |
| return iosAppleLink(coords); | |
| } | |
| case 'google': { | |
| const url = await iosGoogleLink(coords); | |
| return url; | |
| } | |
| case 'osm': | |
| return iosOsmLink(coords); | |
| case 'apple': | |
| return iosAppleLink(coords); | |
| default: | |
| return iosAppleLink(coords); | |
| } |
🧰 Tools
🪛 Biome (2.1.2)
[error] 147-147: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
🤖 Prompt for AI Agents
In app/views/LocationShare/services/mapProviders.ts around lines 146 to 155, the
`case 'google'` branch declares `const url` directly in a switch which violates
the no-switch-declarations rule; wrap the contents of the `case 'google'` branch
in its own block (add `{ ... }` after the case and close before the next case)
so the `const url = await iosGoogleLink(coords); return url;` is scoped inside
the block and CI static analysis will pass.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
app/containers/MessageComposer/components/Buttons/ActionsButton.tsx(4 hunks)app/containers/message/Components/Attachments/Attachments.tsx(2 hunks)app/containers/message/Message.tsx(3 hunks)app/i18n/locales/en.json(17 hunks)app/index.tsx(0 hunks)app/lib/constants/keys.ts(1 hunks)
💤 Files with no reviewable changes (1)
- app/index.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- app/containers/message/Components/Attachments/Attachments.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
app/containers/MessageComposer/components/Buttons/ActionsButton.tsx (7)
app/lib/hooks/useAppSelector.ts (1)
useAppSelector(6-6)app/lib/methods/userPreferences.ts (1)
useUserPreferences(10-10)app/views/LocationShare/services/mapProviders.ts (1)
MapProviderName(7-7)app/lib/constants/keys.ts (4)
MAP_PROVIDER_PREFERENCE_KEY(25-25)MAP_PROVIDER_DEFAULT(28-28)GOOGLE_MAPS_API_KEY_PREFERENCE_KEY(26-26)OSM_API_KEY_PREFERENCE_KEY(27-27)app/lib/methods/helpers/info.ts (1)
showErrorAlert(5-6)app/views/LocationShare/services/staticLocation.ts (1)
getCurrentPositionOnce(24-59)app/containers/ActionSheet/Provider.tsx (1)
TActionSheetOptionsItem(7-17)
🪛 ESLint
app/containers/MessageComposer/components/Buttons/ActionsButton.tsx
[error] 18-18: Missing file extension for "../../../../lib/methods"
(import/extensions)
[error] 24-24: Missing file extension for "../../../../lib/constants"
(import/extensions)
🔇 Additional comments (7)
app/lib/constants/keys.ts (1)
25-28: LGTM!The new constants follow the existing naming conventions and integrate well with the codebase. The default value
'osm'correctly matches theMapProviderNametype defined inapp/views/LocationShare/services/mapProviders.ts.app/containers/MessageComposer/components/Buttons/ActionsButton.tsx (5)
46-63: Well-designed helper for preventing sheet overlap.The
openSheetSafelyfunction effectively prevents race conditions during action sheet transitions using a ref-based lock and deferred execution. This pattern ensures a smooth UX when chaining multiple sheet interactions.
76-118: Permission handling and error management look solid.The function correctly requests location permissions for both Android and iOS, handles permission denials gracefully, and uses
InteractionManagerto defer navigation until animations complete. Error messages are properly localized.
120-164: Live location flow is well-structured.The implementation properly requests both FINE and COARSE location permissions on Android for live tracking. The
@ts-ignoreon line 154 has already been flagged in previous reviews for missing navigation type definitions.
166-186: Clean mode selection implementation.The function provides a clear choice between static and live location sharing, properly using
openSheetSafelyto serialize the sheet transitions.
260-277: API key validation ensures a better user experience.The code validates that the required API key is configured before allowing location sharing and provides a helpful error message directing users to configure their API key in settings. The provider-specific key checking logic is correct.
app/i18n/locales/en.json (1)
56-58: Comprehensive localization coverage for the location sharing feature.The new localization keys are well-organized, follow existing naming conventions, and provide clear messaging for users. The keys cover API key management, map provider selection, location permissions, and live location tracking.
Note: When this feature is finalized, ensure translations are added to other locale files to maintain multilingual support.
Also applies to: 162-162, 188-189, 216-216, 322-322, 326-327, 385-387, 397-397, 457-461, 480-482, 494-495, 599-599, 613-613, 631-631, 634-635, 652-652, 662-662, 669-669, 820-820, 822-822, 848-852
| <Quote {...props} /> | ||
| <Content {...props} /> | ||
| <Attachments {...props} /> | ||
| <Attachments {...props} messageId={props.id} roomId={props.rid} /> |
There was a problem hiding this comment.
🧩 Analysis chain
LGTM! Verify prop types downstream.
The propagation of messageId and roomId to the Attachments component is consistent across all three render branches (preview, default, and pinned message info). This enables live-location attachments to render with the necessary context.
Confirm that:
- The
Attachmentscomponent's prop types correctly acceptmessageIdandroomId props.idandprops.ridare always defined (though existing usage at line 174 suggests they are)- The downstream
LiveLocationAttachmentproperly handles these props
Also applies to: 90-90, 164-164
🏁 Script executed:
#!/bin/bash
# Description: Verify Attachments component accepts the new props and check prop types
# Check Attachments component definition and prop types
ast-grep --pattern $'interface $_ {
$$$
messageId$_
$$$
}'
# Check if LiveLocationAttachment uses these props
rg -n --type=typescript --type=typescriptreact -A 5 'messageId.*roomId|roomId.*messageId'Length of output: 34
🏁 Script executed:
#!/bin/bash
# Locate AttachmentsProps interface/type
rg -nP 'interface\s+AttachmentsProps' -A5 --glob '*.tsx'
rg -nP 'type\s+AttachmentsProps' -A5 --glob '*.tsx'
# Locate Attachments component definition or export
rg -nP 'export\s+(?:const|function|class)\s+Attachments' -C3 --glob '*.tsx'
# Locate LiveLocationAttachment usage to see prop handling
rg -n 'LiveLocationAttachment' -C3 --glob '*.tsx'Length of output: 3690
Fix LiveLocationAttachment prop forwarding
LiveLocationAttachment’s interface only accepts roomId and Attachments.tsx isn’t passing messageId or roomId through.
- Add
messageId?: stringtoILiveLocationAttachment. - In
Attachments.tsx, pass bothmessageId={messageId}androomId={roomId}from the parent props into<LiveLocationAttachment …/>.
🤖 Prompt for AI Agents
In app/containers/message/Message.tsx around line 42: LiveLocationAttachment's
props are not being forwarded from Attachments.tsx; update the
LiveLocationAttachment interface to include messageId?: string (where
ILiveLocationAttachment is declared) and modify Attachments.tsx to forward both
messageId={messageId} and roomId={roomId} from the parent props into the
<LiveLocationAttachment …/> element so the component receives both identifiers.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (4)
app/containers/MessageComposer/components/Buttons/ActionsButton.tsx (1)
152-160: Remove @ts-ignore by updating navigation typesAdd LiveLocationPreviewModal route to your navigation types so this call is typed; avoid suppressions.
Suggested param type (adjust to your stack typings):
LiveLocationPreviewModal: { rid: string; tmid?: string; provider: MapProviderName; googleKey?: string; osmKey?: string; isTracking?: boolean; liveLocationId?: string; };app/containers/message/Components/LiveLocationCard.tsx (1)
170-176: Interval uses stale cardIsActive valueThe setInterval callback closes over an outdated state value; use a ref or include state in deps and recreate interval.
-const staleCheck = setInterval(() => { - if (isMessageTooOld(messageTimestamp) && cardIsActive) { - setCardIsActive(false); - } -}, 5 * 60 * 1000); +const activeRef = useRef(cardIsActive); +useEffect(() => { activeRef.current = cardIsActive; }, [cardIsActive]); +const staleCheck = setInterval(() => { + if (isMessageTooOld(messageTimestamp) && activeRef.current) { + setCardIsActive(false); + } +}, 5 * 60 * 1000);app/views/LocationShare/LiveLocationPreviewModal.tsx (2)
516-516: Fix the corrupted emoji in map placeholder.The placeholder text contains mojibake (
ðŸ") instead of a proper emoji or plain text. This was previously flagged but remains unfixed.Apply this diff:
-<Text style={[styles.loadingText, { fontSize: 16, fontWeight: 'bold', marginBottom: 8 }]}>ðŸ" Map Preview</Text> +<Text style={[styles.loadingText, { fontSize: 16, fontWeight: 'bold', marginBottom: 8 }]}>📍 Map Preview</Text>Ensure the file is saved with UTF-8 encoding so the emoji renders correctly.
170-219: Live viewers still see hard-coded San Francisco coordinates instead of real data.This issue was flagged in a previous review but remains unfixed. When
!isTracking && liveLocationIdis true (viewer mode), the code sets placeholder coordinates (37.78583, -122.40642) and never fetches or polls the actual shared location from the server. Viewers opening a "View Live Location" link will always see San Francisco, making the feature non-functional.Replace the hard-coded branches with real data fetching:
useEffect(() => { if (!isTracking && liveLocationId) { + let pollInterval: ReturnType<typeof setInterval> | null = null; + + const fetchLiveLocation = async () => { + try { + const data = await LiveLocationApi.get(liveLocationId); + const state: LiveLocationState = { + coords: { + latitude: data.coords.latitude, + longitude: data.coords.longitude, + accuracy: data.coords.accuracy + }, + timestamp: data.timestamp, + isActive: data.isActive + }; + handleLocationUpdate(state); + } catch (error) { + // Handle fetch error + } + }; + + // Initial fetch + fetchLiveLocation(); + + // Poll every 10 seconds for updates + pollInterval = setInterval(fetchLiveLocation, 10000); + viewerUpdateIntervalRef.current = pollInterval; + - isLiveLocationEnded(liveLocationId).then(ended => { - if (ended) { - const inactiveState = { - coords: { - latitude: 37.78583, - longitude: -122.40642, - accuracy: 5 - }, - timestamp: Date.now() - (5 * 60 * 1000), - isActive: false - }; - handleLocationUpdate(inactiveState); - } else { - const activeState = { - coords: { - latitude: 37.78583, - longitude: -122.40642, - accuracy: 5 - }, - timestamp: Date.now(), - isActive: true - }; - handleLocationUpdate(activeState); - } - }); const handleLiveLocationEnded = (endedLocationId: string) => { if (endedLocationId === liveLocationId) { - const inactiveState = { - coords: { - latitude: 37.78583, - longitude: -122.40642, - accuracy: 5 - }, - timestamp: Date.now() - (5 * 60 * 1000), - isActive: false - }; - handleLocationUpdate(inactiveState); + // Fetch final state from server + fetchLiveLocation(); } }; addLiveLocationEndedListener(handleLiveLocationEnded); return () => { removeLiveLocationEndedListener(handleLiveLocationEnded); + if (pollInterval) { + clearInterval(pollInterval); + } }; }Note: This assumes
LiveLocationApi.getexists inapp/views/LocationShare/services/liveLocationApi.ts. If not, you'll need to implement it to fetch live location data from the server.
🧹 Nitpick comments (15)
app/containers/message/Components/Attachments/LiveLocationAttachment.tsx (4)
30-39: Avoid destructuring unused props to prevent lint noiseDrop unused props instead of aliasing with underscores. This keeps the signature clean and avoids relying on underscore-ignore config.
Apply:
-const LiveLocationAttachment: React.FC<ILiveLocationAttachment> = ({ - attachment, - getCustomEmoji: _getCustomEmoji, - showAttachment: _showAttachment, - style: _style, - isReply: _isReply, - author: _author, - messageId, - roomId -}) => { +const LiveLocationAttachment: React.FC<ILiveLocationAttachment> = ({ attachment, messageId, roomId }) => {
61-67: Localize alert stringsReplace hard-coded Alert copy with I18n keys to match app localization.
-Alert.alert( - 'Cannot View Live Location', - 'You cannot view others\' live locations while sharing your own. Please stop sharing your location first.', - [{ text: 'OK' }] -); +Alert.alert( + I18n.t('Live_Location_Cannot_View_Title'), + I18n.t('Live_Location_Cannot_View_Message'), + [{ text: I18n.t('OK') }] +);
84-85: Use theme tokens instead of hard-coded colorsReplace '#27ae60' and '#e74c3c' with theme-provided success/danger colors to support theming and accessibility.
- borderLeftColor: live.isActive ? '#27ae60' : '#e74c3c' + borderLeftColor: live.isActive ? colors.tintSuccess : colors.tintDanger- color: live.isActive ? '#27ae60' : '#e74c3c' + color: live.isActive ? colors.tintSuccess : colors.tintDangerAlso applies to: 108-109
126-131: Consider allowing navigation for ended sessionsCurrently disabled when ended. Consider still navigating to the viewer (read-only) so users can see last known point.
app/containers/MessageComposer/components/Buttons/ActionsButton.tsx (1)
260-276: Missing API key UXWhen keys are missing, offer a quick path to settings (Location Preferences) instead of just alerting.
- showErrorAlert( - I18n.t('API_key_required', { provider: mapProvider === 'google' ? 'Google Maps' : 'OpenStreetMap' }), - I18n.t('Please_configure_API_key_in_settings') - ); - return; + showErrorAlert( + I18n.t('API_key_required', { provider: mapProvider === 'google' ? 'Google Maps' : 'OpenStreetMap' }), + I18n.t('Please_configure_API_key_in_settings'), + () => Navigation.navigate('LocationPreferencesView') + ); + return;app/views/LocationShare/LocationPreviewModal.tsx (3)
92-135: Localize UI strings and use theme tokensReplace hard-coded strings ('Share Location', 'Cancel', 'Sharing...', 'Accuracy') with I18n.t(...) and swap raw colors in styles for theme tokens for consistency and dark mode.
139-171: Avoid hard-coded colors in stylesUse theme colors (e.g., themes[theme].surfaceLight, font colors, primary) instead of hex codes to align with app theming.
61-68: Localize error alertsUse I18n for Alert titles/messages ('Error', 'Could not open maps application', 'Oops', 'Could_not_send_message').
Also applies to: 85-89
app/containers/message/Components/LiveLocationCard.tsx (3)
215-216: Use default provider constant, not hard-coded 'google'Prefer MAP_PROVIDER_DEFAULT for consistency.
-const finalProvider = viewerMapProvider || ownerProvider || 'google'; +const finalProvider = viewerMapProvider || ownerProvider || MAP_PROVIDER_DEFAULT;
52-98: Remove or gate console logsThese logs are verbose; guard behind a dev flag or remove for production.
186-206: Localize user-facing strings and colorsUse I18n for alerts/text ('Live Location Ended', 'Live Location Active', etc.) and theme colors instead of hex codes for status/labels.
Also applies to: 257-265
app/views/LocationShare/LiveLocationViewerModal.tsx (2)
295-317: Map URL fallback may fail without keysFalling back to Google without a key often returns 403. If neither provider key is available, skip staticMapUrl and show the placeholder only; rely on “Open in Maps”.
-} else { - const mapResult = staticMapUrl('google', mobileCoords, { size: '350x220' }); - mapUrl = mapResult.url; -} +} else { + // No valid key — keep placeholder instead of calling provider without credentials + mapUrl = ''; +}
428-545: Localize UI strings and theme colorsReplace hard-coded UI strings ('Loading live location...', 'Close', titles, etc.) with I18n.t, and swap hex colors in styles for theme tokens to support dark mode.
app/views/LocationShare/LiveLocationPreviewModal.tsx (2)
247-272: Add timeout to getCurrentPositionAsync for reliability.The location fetch has no timeout and could hang indefinitely if location services are slow or malfunctioning, blocking the preview modal from loading.
Wrap the call in a timeout:
} else { - Location.getCurrentPositionAsync({ - accuracy: Location.Accuracy.High - }).then((location: Location.LocationObject) => { - const previewState: LiveLocationState = { - coords: { - latitude: location.coords.latitude, - longitude: location.coords.longitude, - accuracy: location.coords.accuracy ?? undefined - }, - timestamp: Date.now(), - isActive: false - }; - handleLocationUpdate(previewState); - }).catch((_error: any) => { + const timeoutPromise = new Promise<never>((_, reject) => + setTimeout(() => reject(new Error('Location request timed out')), 10000) + ); + + Promise.race([ + Location.getCurrentPositionAsync({ + accuracy: Location.Accuracy.High + }), + timeoutPromise + ]).then((location: Location.LocationObject) => { + const previewState: LiveLocationState = { + coords: { + latitude: location.coords.latitude, + longitude: location.coords.longitude, + accuracy: location.coords.accuracy ?? undefined + }, + timestamp: Date.now(), + isActive: false + }; + handleLocationUpdate(previewState); + }).catch((_error: any) => { const defaultState: LiveLocationState = { coords: { latitude: 37.78583, longitude: -122.40642, accuracy: 5 }, timestamp: Date.now(), isActive: false }; handleLocationUpdate(defaultState); }); }
435-435: Simplify the double negative for clarity.The condition
!(!isTracking && liveLocationId)is logically correct but harder to parse than necessary.Consider refactoring to eliminate the double negative:
- if (isOwner() && !(!isTracking && liveLocationId)) { + const isViewerMode = !isTracking && liveLocationId; + if (isOwner() && !isViewerMode) { return ( <TouchableOpacity onPress={onStopSharing} style={[styles.btn, styles.btnDanger]}> <Text style={[styles.btnText, styles.btnTextDanger]}>Stop Sharing</Text> </TouchableOpacity> ); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (9)
app/containers/MessageComposer/components/Buttons/ActionsButton.tsx(4 hunks)app/containers/message/Components/Attachments/LiveLocationAttachment.tsx(1 hunks)app/containers/message/Components/LiveLocationCard.tsx(1 hunks)app/containers/message/Content.tsx(1 hunks)app/views/LocationPreferencesView/index.tsx(1 hunks)app/views/LocationShare/LiveLocationPreviewModal.tsx(1 hunks)app/views/LocationShare/LiveLocationStatusBar.tsx(1 hunks)app/views/LocationShare/LiveLocationViewerModal.tsx(1 hunks)app/views/LocationShare/LocationPreviewModal.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- app/views/LocationShare/LiveLocationStatusBar.tsx
- app/containers/message/Content.tsx
- app/views/LocationPreferencesView/index.tsx
🧰 Additional context used
🧬 Code graph analysis (6)
app/containers/MessageComposer/components/Buttons/ActionsButton.tsx (7)
app/lib/hooks/useAppSelector.ts (1)
useAppSelector(6-6)app/lib/methods/userPreferences.ts (1)
useUserPreferences(10-10)app/views/LocationShare/services/mapProviders.ts (1)
MapProviderName(7-7)app/lib/constants/keys.ts (4)
MAP_PROVIDER_PREFERENCE_KEY(25-25)MAP_PROVIDER_DEFAULT(28-28)GOOGLE_MAPS_API_KEY_PREFERENCE_KEY(26-26)OSM_API_KEY_PREFERENCE_KEY(27-27)app/lib/methods/helpers/info.ts (1)
showErrorAlert(5-6)app/views/LocationShare/services/staticLocation.ts (1)
getCurrentPositionOnce(24-59)app/containers/ActionSheet/Provider.tsx (1)
TActionSheetOptionsItem(7-17)
app/containers/message/Components/Attachments/LiveLocationAttachment.tsx (4)
app/definitions/IAttachment.ts (1)
IAttachment(9-48)app/theme.tsx (1)
useTheme(29-29)app/lib/hooks/useAppSelector.ts (1)
useAppSelector(6-6)app/views/LocationShare/LiveLocationPreviewModal.tsx (2)
reopenLiveLocationModal(544-557)isLiveLocationActive(540-542)
app/views/LocationShare/LiveLocationViewerModal.tsx (5)
app/lib/hooks/useAppSelector.ts (1)
useAppSelector(6-6)app/lib/methods/userPreferences.ts (1)
useUserPreferences(10-10)app/views/LocationShare/services/mapProviders.ts (4)
MapProviderName(7-7)staticMapUrl(88-103)mapsDeepLink(143-168)providerLabel(173-173)app/lib/constants/keys.ts (4)
MAP_PROVIDER_PREFERENCE_KEY(25-25)MAP_PROVIDER_DEFAULT(28-28)GOOGLE_MAPS_API_KEY_PREFERENCE_KEY(26-26)OSM_API_KEY_PREFERENCE_KEY(27-27)app/views/LocationShare/services/liveLocationApi.ts (2)
LiveLocationApi(45-89)serverToMobileCoords(105-111)
app/containers/message/Components/LiveLocationCard.tsx (8)
app/theme.tsx (1)
useTheme(29-29)app/lib/hooks/useAppSelector.ts (1)
useAppSelector(6-6)app/lib/methods/userPreferences.ts (1)
useUserPreferences(10-10)app/views/LocationShare/services/mapProviders.ts (1)
MapProviderName(7-7)app/lib/constants/keys.ts (4)
MAP_PROVIDER_PREFERENCE_KEY(25-25)MAP_PROVIDER_DEFAULT(28-28)GOOGLE_MAPS_API_KEY_PREFERENCE_KEY(26-26)OSM_API_KEY_PREFERENCE_KEY(27-27)app/views/LocationShare/LiveLocationPreviewModal.tsx (5)
getCurrentLiveParams(54-56)isLiveLocationActive(540-542)addStatusChangeListener(80-82)removeStatusChangeListener(83-85)reopenLiveLocationModal(544-557)app/views/LocationShare/services/handleLiveLocationUrl.ts (3)
isLiveLocationEnded(62-65)addLiveLocationEndedListener(14-16)removeLiveLocationEndedListener(18-20)app/lib/constants/colors.ts (1)
themes(304-304)
app/views/LocationShare/LocationPreviewModal.tsx (4)
app/views/LocationShare/services/mapProviders.ts (5)
Coords(12-12)MapProviderName(7-7)staticMapUrl(88-103)mapsDeepLink(143-168)providerLabel(173-173)app/lib/hooks/useAppSelector.ts (1)
useAppSelector(6-6)app/selectors/login.ts (1)
getUserSelector(20-20)app/lib/methods/sendMessage.ts (1)
sendMessage(88-242)
app/views/LocationShare/LiveLocationPreviewModal.tsx (5)
app/views/LocationShare/services/mapProviders.ts (4)
MapProviderName(7-7)staticMapUrl(88-103)mapsDeepLink(143-168)providerLabel(173-173)app/views/LocationShare/services/liveLocation.ts (2)
LiveLocationTracker(18-251)LiveLocationState(7-16)app/lib/hooks/useAppSelector.ts (1)
useAppSelector(6-6)app/selectors/login.ts (1)
getUserSelector(20-20)app/views/LocationShare/services/handleLiveLocationUrl.ts (4)
isLiveLocationEnded(62-65)addLiveLocationEndedListener(14-16)removeLiveLocationEndedListener(18-20)markLiveLocationAsEnded(52-60)
🔇 Additional comments (2)
app/containers/message/Components/Attachments/LiveLocationAttachment.tsx (1)
70-74: Verify screen registration and paramsEnsure 'LiveLocationViewerModal' is registered in stacks for both master-detail and standard flows, and expects { rid, msgId } params.
app/views/LocationShare/LiveLocationViewerModal.tsx (1)
333-347: Ensure interval cleanup on all pathsCleanup looks good; keep stopPeriodicUpdates idempotent and confirm no duplicate intervals when toggling isActive quickly.
| Navigation.navigate('LiveLocationPreviewModal', { | ||
| rid: rid!, | ||
| tmid, | ||
| provider: finalProvider, | ||
| googleKey: finalProvider === 'google' ? viewerGoogleApiKey : undefined, | ||
| osmKey: finalProvider === 'osm' ? viewerOsmApiKey : undefined, | ||
| liveLocationId: liveLocationId!, | ||
| isTracking: false, | ||
| ownerName: author?.name || author?.username || 'Other User' | ||
| }); | ||
| } else { |
There was a problem hiding this comment.
Wrong screen/params for viewing others’ live location
This navigates to LiveLocationPreviewModal with preview params. Viewer flow should open LiveLocationViewerModal and pass msgId (from the deep link) with rid.
Apply:
- Navigation.navigate('LiveLocationPreviewModal', {
- rid: rid!,
- tmid,
- provider: finalProvider,
- googleKey: finalProvider === 'google' ? viewerGoogleApiKey : undefined,
- osmKey: finalProvider === 'osm' ? viewerOsmApiKey : undefined,
- liveLocationId: liveLocationId!,
- isTracking: false,
- ownerName: author?.name || author?.username || 'Other User'
- });
+ const msgId = params.get('msgId');
+ if (!rid || !(msgId || liveLocationId)) {
+ Alert.alert(I18n.t('Error'), I18n.t('Could_not_open_live_location'));
+ return;
+ }
+ Navigation.navigate('LiveLocationViewerModal', {
+ rid,
+ msgId: msgId || (liveLocationId as string)
+ });📝 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.
| Navigation.navigate('LiveLocationPreviewModal', { | |
| rid: rid!, | |
| tmid, | |
| provider: finalProvider, | |
| googleKey: finalProvider === 'google' ? viewerGoogleApiKey : undefined, | |
| osmKey: finalProvider === 'osm' ? viewerOsmApiKey : undefined, | |
| liveLocationId: liveLocationId!, | |
| isTracking: false, | |
| ownerName: author?.name || author?.username || 'Other User' | |
| }); | |
| } else { | |
| const msgId = params.get('msgId'); | |
| if (!rid || !(msgId || liveLocationId)) { | |
| Alert.alert(I18n.t('Error'), I18n.t('Could_not_open_live_location')); | |
| return; | |
| } | |
| Navigation.navigate('LiveLocationViewerModal', { | |
| rid, | |
| msgId: msgId || (liveLocationId as string) | |
| }); | |
| } else { |
🤖 Prompt for AI Agents
In app/containers/message/Components/LiveLocationCard.tsx around lines 217 to
227, the code incorrectly navigates to LiveLocationPreviewModal with preview
parameters when viewing another user's live location; update the navigation to
open LiveLocationViewerModal instead and pass rid plus the deep-link msgId (use
msgId from the current scope) as parameters, removing preview-only params like
provider/googleKey/osmKey/liveLocationId/isTracking/ownerName so the viewer flow
receives rid and msgId only.
| } from '../../../../lib/constants/keys'; | ||
|
|
||
| export const ActionsButton = () => { | ||
| 'use memo'; |
There was a problem hiding this comment.
Remove stray directive literal
"'use memo';" is a no-op directive string and confusing. Remove it.
- 'use memo';
+ // no-op📝 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.
| 'use memo'; | |
| // no-op |
🤖 Prompt for AI Agents
In app/containers/MessageComposer/components/Buttons/ActionsButton.tsx around
line 27, there's a stray directive literal "'use memo';" which is a no-op and
confusing; simply remove that line from the file so the source contains no
unnecessary directive string.
| handleLocationUpdate(defaultState); | ||
| }); | ||
| } | ||
| }, [provider, googleKey, osmKey, isTracking, liveLocationId]); |
There was a problem hiding this comment.
Add missing dependencies to useEffect.
The effect uses id, username, rid, and tmid but doesn't include them in the dependency array. This can cause stale closures where the effect uses outdated values when these props change.
Apply this diff:
- }, [provider, googleKey, osmKey, isTracking, liveLocationId]);
+ }, [provider, googleKey, osmKey, isTracking, liveLocationId, id, username, rid, tmid]);📝 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.
| }, [provider, googleKey, osmKey, isTracking, liveLocationId]); | |
| }, [provider, googleKey, osmKey, isTracking, liveLocationId, id, username, rid, tmid]); |
🤖 Prompt for AI Agents
In app/views/LocationShare/LiveLocationPreviewModal.tsx around line 273, the
useEffect dependency array is missing values used inside the effect (id,
username, rid, tmid); update the dependency array to include id, username, rid,
and tmid in addition to the existing provider, googleKey, osmKey, isTracking,
and liveLocationId so the effect re-runs when any of those props change and
avoids stale closures.
| if (params?.liveLocationId) { | ||
| try { | ||
| await markLiveLocationAsEnded(params.liveLocationId); | ||
| } catch { | ||
| // Failed to mark live location as ended | ||
| } | ||
| } | ||
|
|
||
| // No need to send manual stop message - server handles this via LiveLocationApi.stop | ||
| if (params?.liveLocationId) { | ||
| try { | ||
| await markLiveLocationAsEnded(params.liveLocationId); | ||
| } catch (e) { | ||
| // best-effort cleanup | ||
| } | ||
| } |
There was a problem hiding this comment.
Remove duplicate markLiveLocationAsEnded call.
Lines 569-575 and 578-584 both call markLiveLocationAsEnded(params.liveLocationId). This is redundant and appears to be a copy-paste error.
Apply this diff to remove the duplicate:
if (params?.liveLocationId) {
try {
await markLiveLocationAsEnded(params.liveLocationId);
} catch {
// Failed to mark live location as ended
}
}
- // No need to send manual stop message - server handles this via LiveLocationApi.stop
- if (params?.liveLocationId) {
- try {
- await markLiveLocationAsEnded(params.liveLocationId);
- } catch (e) {
- // best-effort cleanup
- }
- }
} finally {📝 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.
| if (params?.liveLocationId) { | |
| try { | |
| await markLiveLocationAsEnded(params.liveLocationId); | |
| } catch { | |
| // Failed to mark live location as ended | |
| } | |
| } | |
| // No need to send manual stop message - server handles this via LiveLocationApi.stop | |
| if (params?.liveLocationId) { | |
| try { | |
| await markLiveLocationAsEnded(params.liveLocationId); | |
| } catch (e) { | |
| // best-effort cleanup | |
| } | |
| } | |
| if (params?.liveLocationId) { | |
| try { | |
| await markLiveLocationAsEnded(params.liveLocationId); | |
| } catch { | |
| // Failed to mark live location as ended | |
| } | |
| } | |
| } finally { |
🤖 Prompt for AI Agents
In app/views/LocationShare/LiveLocationPreviewModal.tsx around lines 569 to 584,
there is a duplicated call to markLiveLocationAsEnded(params.liveLocationId);
remove one of the duplicate blocks so the function is only called once; keep a
single try/catch that handles errors (preserving whichever catch includes error
variable if you need it) and delete the extra redundant try/catch to avoid
calling the same cleanup twice.
3 participants
Description
This PR introduces live location sharing to Rocket.Chat React Native, allowing users to share and track real-time locations in chat. It supports both one-time static sharing and continuous live updates every 10 seconds. Users can minimize the modal while keeping tracking active, with a persistent status bar indicating the session. The design clearly separates owners, who can start or stop sharing, from viewers, who can only view. The feature also includes deep linking, session persistence across minimization, and a polished, intuitive UI.
Proposed changes
Issue(s)
How to test or reproduce
Screenshots
Types of changes
Checklist
Further comments
Summary by CodeRabbit
New Features
Chores
Localization