feat: static & live location sharing (draft) #6694
feat: static & live location sharing (draft) #6694yiweigao0226 wants to merge 46 commits intoRocketChat:developfrom
Conversation
Signed-off-by: yiwei gao <yiweigao@MacBook-Pro-4.local>
Signed-off-by: yiwei gao <yiweigao@MacBook-Pro-4.local>
Signed-off-by: yiwei gao <yiweigao0226@gmail.com>
|
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 full location features: permissions and foreground service on Android/iOS, map provider utilities, static/current/live location acquisition and live-tracking services, REST endpoints, UI screens/components for sharing/viewing locations, message/attachment integrations, localization, and navigation/type updates. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant UI as Composer UI
participant P as Permissions
participant Loc as StaticLocation
participant Map as MapProvider
participant Msg as Messaging / REST
U->>UI: Tap "Share Location"
UI->>P: Request location permission
alt permission granted
P-->>UI: granted
UI->>Loc: getCurrentPositionOnce()
Loc-->>UI: coords
UI->>Map: build staticMapUrl / preview
Map-->>UI: preview image
U->>UI: Confirm share
UI->>Msg: sendMessage(..., attachments)
Msg->>Msg: call liveLocation.start (if live)
Msg-->>UI: success
else permission denied
P-->>UI: denied
UI-->>U: show permission error
end
sequenceDiagram
participant Owner as Sender
participant Tracker as LiveLocationTracker
participant Pos as watchPositionAsync
participant API as LiveLocation API
participant Viewer as Receiver
Owner->>Tracker: startTracking()
Tracker->>API: liveLocation.start(rid, initial)
API-->>Tracker: msgId (active)
Tracker->>Pos: subscribe to position updates
loop every 10s
Pos-->>API: liveLocation.update(msgId, coords)
API-->>Tracker: ok
end
Viewer->>API: liveLocation.get(msgId)
API-->>Viewer: current coords / status
Owner->>Tracker: stopTracking()
Tracker->>API: liveLocation.stop(msgId, finalCoords)
API-->>Tracker: stopped
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Areas needing extra attention:
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (1)
🔇 Additional comments (2)
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
app.json
Outdated
| "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.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| @@ -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.
yiweigao0226
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
yiweigao0226
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Failure
* [FIX] Border style * [FIX] Refactoring * [FIX] fix color of border * Undo Co-authored-by: Aroo <azhaubassar@gmail.com> Co-authored-by: Diego Mello <diegolmello@gmail.com>
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)
diegolmello
left a comment
diegolmello
left a comment
There was a problem hiding this comment.
I'm missing recordings and a better documentation on PR description.
| <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
|
Most of Rohit’s comments have been addressed, and I’ve left replies for the ones that are still pending. |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
app/containers/message/Message.tsx (1)
43-43: Remove redundant prop forwarding.Since
{...props}already spreads all props includingidandrid, explicitly passingid={props.id} rid={props.rid}is redundant.Apply this diff:
-<Attachments {...props} id={props.id} rid={props.rid} /> +<Attachments {...props} />Also applies to lines 94 and 168.
📜 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 (3)
app/containers/message/Components/Attachments/Reply.tsx(2 hunks)app/containers/message/Message.tsx(4 hunks)app/views/LocationShare/services/liveLocationApi.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
app/views/LocationShare/services/liveLocationApi.ts (1)
app/lib/services/restApi.ts (4)
liveLocationStart(1110-1115)liveLocationUpdate(1117-1118)liveLocationStop(1120-1124)liveLocationGet(1126-1126)
app/containers/message/Message.tsx (1)
app/containers/message/Components/CurrentLocationCard.tsx (1)
isCurrentLocationMessage(63-65)
🔇 Additional comments (6)
app/containers/message/Components/Attachments/Reply.tsx (2)
213-213: LGTM! Context destructuring extended correctly.Adding
ridto the destructured MessageContext values is consistent with the existing pattern and necessary for forwarding room context to attachments.
256-263: LGTM! Prop forwarding enables attachment context.Forwarding
idandridfrom MessageContext to the Attachments component provides the necessary message and room identifiers for live location attachments to function correctly.app/containers/message/Message.tsx (2)
19-19: LGTM! Import added for location sharing feature.The import of
CurrentLocationCardandisCurrentLocationMessageis correctly structured and necessary for the new location message rendering logic.
84-95: Conditional rendering logic is sound.The implementation correctly:
- Detects location messages via
isCurrentLocationMessage- Suppresses
Contentto avoid showing raw coordinates whenCurrentLocationCardrenders them- Suppresses
Urlsto prevent coordinate text from being parsed as URLs- Renders
CurrentLocationCardfor all messages (component returnsnullfor non-location messages per the implementation)The conditional rendering addresses the question from the past reviewer and is necessary to prevent duplicate display of coordinates.
app/views/LocationShare/services/liveLocationApi.ts (2)
4-41: LGTM! Type definitions are clear and consistent.The type definitions are well-structured and provide good type safety for the API surface. The indentation issue from the previous review appears to be resolved.
97-107: LGTM! Coordinate conversion is correct.The function correctly maps from mobile format (
latitude,longitude,accuracy) to server format (lat,lon,acc). The previous issue withlngvslonhas been resolved.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
app/views/LocationShare/LiveLocationViewerModal.tsx (1)
285-287: Hardcoded 'osm' breaks provider selection across preview, cache, and attribution.The component computes
providerfrom user preferences (line 228) but ignores it in three places: the static map URL (line 286), the cache key prefix (line 314), and the attribution text (line 525). Users who select Google Maps will see OSM tiles with incorrect attribution and cache collisions.Apply this diff to honor the selected provider:
if (mobileCoords) { - const mapResult = staticMapUrl('osm', mobileCoords, { zoom: 15 }); + const mapResult = staticMapUrl(provider as MapProviderName, mobileCoords, { zoom: 15 }); setMapImageUrl(mapResult.url); }const cacheKey = useMemo(() => { const lat = liveLocationData?.coords?.latitude; const lon = liveLocationData?.coords?.longitude; - return lat && lon ? `osm-${lat.toFixed(5)}-${lon.toFixed(5)}-z15-v2` : undefined; - }, [liveLocationData?.coords?.latitude, liveLocationData?.coords?.longitude]); + return lat && lon ? `${provider}-${lat.toFixed(5)}-${lon.toFixed(5)}-z15-v2` : undefined; + }, [liveLocationData?.coords?.latitude, liveLocationData?.coords?.longitude, provider]);- <Text style={[styles.attribution, { color: colors.fontSecondaryInfo }]}>{providerAttribution('osm')}</Text> + <Text style={[styles.attribution, { color: colors.fontSecondaryInfo }]}>{providerAttribution(provider as MapProviderName)}</Text>Also applies to: 311-315, 525-525
📜 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 (2)
app/views/LocationShare/LiveLocationViewerModal.tsx(1 hunks)app/views/LocationShare/services/liveLocationApi.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
app/views/LocationShare/LiveLocationViewerModal.tsx (7)
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 (5)
MapProviderName(5-5)staticMapUrl(76-91)mapsDeepLink(125-151)providerLabel(153-153)providerAttribution(154-154)app/lib/constants/keys.ts (2)
MAP_PROVIDER_PREFERENCE_KEY(25-25)MAP_PROVIDER_DEFAULT(28-28)app/views/LocationShare/services/liveLocationApi.ts (2)
LiveLocationApi(43-98)serverToMobileCoords(112-125)app/views/LocationShare/services/handleLiveLocationUrl.ts (2)
addLiveLocationEndedListener(13-15)removeLiveLocationEndedListener(17-19)
app/views/LocationShare/services/liveLocationApi.ts (1)
app/lib/services/restApi.ts (4)
liveLocationStart(1110-1115)liveLocationUpdate(1117-1118)liveLocationStop(1120-1124)liveLocationGet(1126-1126)
| stoppedAt?: string; | ||
| coords: { lat: number; lon: number }; | ||
| expiresAt?: string; | ||
| version?: number; |
There was a problem hiding this comment.
Validation and type mismatch for version field.
Line 81 validates that version is a required number and throws if missing, yet line 40 declares version?: number (optional) in LiveLocationGetResponse. Since the validation guarantees version will always be a number when get() succeeds, the type should reflect that by removing the ?.
Apply this diff to align the type with runtime behavior:
export type LiveLocationGetResponse = {
messageId: string;
ownerId: string;
ownerUsername?: string;
ownerName?: string;
isActive: boolean;
startedAt: string;
lastUpdateAt: string;
stoppedAt?: string;
coords: { lat: number; lon: number };
expiresAt?: string;
- version?: number;
+ version: number;
};Alternatively, if the server can legitimately omit version, remove it from the validation check on line 81 and keep the type optional.
Also applies to: 81-82, 95-95
🤖 Prompt for AI Agents
In app/views/LocationShare/services/liveLocationApi.ts around lines 40, 81-82
and 95, the type for LiveLocationGetResponse declares version as optional but
runtime validation at line 81 requires version to be present; change the
TypeScript declaration at line 40 from version?: number to version: number (and
update any related types/usages at line 95 accordingly) so the static type
matches the runtime guarantee; alternatively, if the server can omit version,
remove the required check at line 81-82 instead—pick one approach and make the
type and validation consistent.
| attachments, | ||
| timeFormat, | ||
| showAttachment, | ||
| style: _style, |
There was a problem hiding this comment.
I can't find the use of style in this component
| showAttachment, | ||
| style: _style, | ||
| getCustomEmoji, | ||
| isReply: _isReply, |
There was a problem hiding this comment.
I can't find the use of _isReply
| <Quote {...props} /> | ||
| <Content {...props} /> | ||
| <Attachments {...props} /> | ||
| <Attachments {...props} id={props.id} rid={props.rid} /> |
There was a problem hiding this comment.
Do not pass id and rid as seperate prop
| {!isLocation ? <Content {...props} /> : null} | ||
| {/* Render a card for "current location" messages; returns null otherwise */} | ||
| <CurrentLocationCard msg={props.msg as string} /> | ||
| <Attachments {...props} id={props.id} rid={props.rid} /> |
There was a problem hiding this comment.
Do not pass id and rid as separate prop
| {props.isInfo && props.type === 'message_pinned' ? ( | ||
| <View pointerEvents='none'> | ||
| <Attachments {...props} /> | ||
| <Attachments {...props} id={props.id} rid={props.rid} /> |
There was a problem hiding this comment.
Do not pass id and rid as separate prop
app/sagas/rooms.js
Outdated
| }) | ||
| ), | ||
| ...messagesToUpdate.map((message) => { | ||
| ...messagesToUpdate.map(message => { |
There was a problem hiding this comment.
Unrelated to this PR, Revert this
app/sagas/rooms.js
Outdated
| }), | ||
| ...messagesToCreate.map(message => | ||
| messagesCollection.prepareCreate((m) => { | ||
| messagesCollection.prepareCreate(m => { |
There was a problem hiding this comment.
Unrelated to this PR, Revert this
app/sagas/rooms.js
Outdated
| } | ||
| }), | ||
| ...subsToDelete.map((subscription) => { | ||
| ...subsToDelete.map(subscription => { |
There was a problem hiding this comment.
Unrelated to this PR, Revert this
app/sagas/rooms.js
Outdated
| }) | ||
| ), | ||
| ...subsToUpdate.map((subscription) => { | ||
| ...subsToUpdate.map(subscription => { |
There was a problem hiding this comment.
Unrelated to this PR, Revert this
app/sagas/rooms.js
Outdated
| const allRecords = [ | ||
| ...subsToCreate.map(subscription => | ||
| subCollection.prepareCreate((s) => { | ||
| subCollection.prepareCreate(s => { |
There was a problem hiding this comment.
Unrelated to this PR, Revert this
|
Also use |
I ran the command to format the code, but it unintentionally modified some unrelated files. |
|
Try to take a pull from develop, i think that will resolve the issue |
It is already up to date. I can run the yarn prettier-lint and manually revert the files that has been changed. Does this work? |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
app/views/LocationShare/services/liveLocationApi.ts (1)
40-40: Type and runtime validation mismatch forversion.Line 40 declares
version?: numberas optional, but line 81 validates thatversionmust be a number and throws if missing. Since the runtime guaranteesversionwill always be present whenget()succeeds, the type should reflect that.Apply this diff to align the type with runtime behavior:
export type LiveLocationGetResponse = { messageId: string; ownerId: string; ownerUsername?: string; ownerName?: string; isActive: boolean; startedAt: string; lastUpdateAt: string; stoppedAt?: string; coords: { lat: number; lon: number }; expiresAt?: string; - version?: number; + version: number; };Alternatively, if the server can legitimately omit
version, remove it from the validation check on line 81.app/views/LocationShare/LiveLocationPreviewModal.tsx (3)
266-270: Hardcoded provider in cache key ignores user selection.The cache key is hardcoded to
'osm'(line 269), so users who choose Google Maps will see OSM tiles and incorrect cache entries. This creates a mismatch between the selected provider and the displayed map.Apply this diff to respect the chosen provider:
const cacheKey = useMemo(() => { const lat = locationState?.coords?.latitude; const lon = locationState?.coords?.longitude; - return lat && lon ? `osm-${lat.toFixed(5)}-${lon.toFixed(5)}-z15-v2` : undefined; - }, [locationState?.coords?.latitude, locationState?.coords?.longitude]); + return lat && lon ? `${provider}-${lat.toFixed(5)}-${lon.toFixed(5)}-z15-v2` : undefined; + }, [locationState?.coords?.latitude, locationState?.coords?.longitude, provider]);
274-286: Hardcoded provider in map URL generation ignores user selection.Line 279 hardcodes
'osm'in thestaticMapUrlcall, so the preview always shows OSM tiles regardless of the user's chosen provider. This creates a mismatch between the selected provider and the displayed preview.Apply this diff:
(state: LiveLocationState) => { safeSet(() => setLocationState(state)); if (state.coords) { - const { url } = staticMapUrl('osm', state.coords, { zoom: 15 }); + const { url } = staticMapUrl(provider, state.coords, { zoom: 15 }); safeSet(() => setMapImageUrl(url)); } emitStatusChange(); }, [safeSet] );And update the dependency array to include
provider:- }, [safeSet]); + }, [safeSet, provider]);
731-731: Hardcoded OSM attribution shown for all providers.Line 731 hardcodes
'osm'in theproviderAttributioncall, so OSM attribution is always displayed even when Google Maps is selected. This is both incorrect and a branding issue.Apply this diff:
- <Text style={[styles.attribution, { color: colors.fontSecondaryInfo }]}>{providerAttribution('osm')}</Text> + <Text style={[styles.attribution, { color: colors.fontSecondaryInfo }]}>{providerAttribution(provider)}</Text>app/containers/MessageComposer/components/Buttons/ActionsButton.tsx (1)
149-149: Remove @ts-ignore by adding navigation type definitions.The
@ts-ignoreon line 149 indicates missing type definitions forLiveLocationPreviewModalin the navigation param list. Add the route definition to your navigation types (likely inapp/stacks/types.ts):LiveLocationPreviewModal: { rid: string; tmid?: string; provider: MapProviderName; };This will eliminate the need for
@ts-ignoreand provide type safety for navigation.
📜 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/MessageComposer/components/Buttons/ActionsButton.tsx(4 hunks)app/containers/message/Components/Attachments/Attachments.tsx(2 hunks)app/definitions/IAttachment.ts(1 hunks)app/views/LocationShare/LiveLocationPreviewModal.tsx(1 hunks)app/views/LocationShare/services/liveLocationApi.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
app/containers/message/Components/Attachments/Attachments.tsx (2)
app/definitions/IAttachment.ts (1)
IAttachment(9-58)app/containers/message/interfaces.ts (1)
IMessageAttachments(18-28)
app/views/LocationShare/services/liveLocationApi.ts (1)
app/lib/services/restApi.ts (4)
liveLocationStart(1110-1115)liveLocationUpdate(1117-1118)liveLocationStop(1120-1124)liveLocationGet(1126-1126)
app/containers/MessageComposer/components/Buttons/ActionsButton.tsx (8)
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(5-5)app/lib/constants/keys.ts (2)
MAP_PROVIDER_PREFERENCE_KEY(25-25)MAP_PROVIDER_DEFAULT(28-28)app/lib/methods/helpers/info.ts (1)
showErrorAlert(5-6)app/views/LocationShare/services/staticLocation.ts (1)
getCurrentPositionOnce(24-59)app/views/LocationShare/LiveLocationPreviewModal.tsx (2)
isLiveLocationActive(745-747)reopenLiveLocationModal(749-761)app/containers/ActionSheet/Provider.tsx (1)
TActionSheetOptionsItem(7-17)
app/views/LocationShare/LiveLocationPreviewModal.tsx (7)
app/views/LocationShare/services/mapProviders.ts (5)
MapProviderName(5-5)staticMapUrl(76-91)mapsDeepLink(125-151)providerLabel(153-153)providerAttribution(154-154)app/views/LocationShare/services/liveLocation.ts (2)
LiveLocationTracker(19-277)LiveLocationState(8-17)app/theme.tsx (1)
useTheme(29-29)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(60-63)markLiveLocationAsEnded(51-58)addLiveLocationEndedListener(13-15)removeLiveLocationEndedListener(17-19)app/views/LocationShare/services/liveLocationApi.ts (2)
LiveLocationApi(43-98)serverToMobileCoords(112-125)
🔇 Additional comments (8)
app/views/LocationShare/services/liveLocationApi.ts (2)
43-98: LGTM!The API wrapper methods are well-structured with consistent error handling, proper type safety, and thorough validation. The coordinate transformation logic correctly maps between client and server representations.
100-125: LGTM!The coordinate conversion utilities are correctly implemented with proper validation. The explicit check for missing
lonprevents silent failures that could result from defaulting to a real coordinate value.app/views/LocationShare/LiveLocationPreviewModal.tsx (2)
34-98: LGTM!The global state management and listener pattern are appropriate for maintaining live location session state across navigation. Error handling in the emit functions prevents listener failures from affecting other listeners.
745-791: LGTM!The exported API functions provide a clean interface for managing live location sessions from external components. The cleanup logic in
stopGlobalLiveLocationproperly handles all state in afinallyblock to ensure consistency.app/containers/MessageComposer/components/Buttons/ActionsButton.tsx (1)
182-260: LGTM!The action handlers have been consistently refactored to close the action sheet and defer navigation/operations via
InteractionManager. This ensures proper UI sequencing and prevents race conditions with sheet transitions. The new location sharing flow integrates cleanly with the existing pattern.app/containers/message/Components/Attachments/Attachments.tsx (3)
16-22: LGTM!The
removeQuotepredicate correctly includes live-location attachments alongside other non-quote attachment types using consistent optional chaining.
25-25: LGTM!The component signature correctly destructures
idandridfrom props, which are needed for theLiveLocationAttachmentcomponent to function properly.
77-79: LGTM!The live-location attachment rendering is correctly implemented with proper type checking and props forwarding. The integration follows the existing pattern for other attachment types.
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 (1)
app/definitions/IAttachment.ts(1 hunks)
| function extractCoordsFromMessage(msg?: string | null): Coords | null { | ||
| if (!msg) return null; | ||
|
|
||
| // geo:lat,lon | ||
| let m = msg.match(/geo:([+-]?\d+\.?\d*),([+-]?\d+\.?\d*)/i); | ||
| if (m) return { latitude: Number(m[1]), longitude: Number(m[2]) }; | ||
| // geo:0,0?q=lat,lon (allow %2C) | ||
| m = msg.match(/geo:[^\s?]*\?q=([+-]?\d+\.?\d*)(?:,|%2C)([+-]?\d+\.?\d*)/i); | ||
| if (m) return { latitude: Number(m[1]), longitude: Number(m[2]) }; | ||
|
|
||
| // Apple Maps: ?ll=lat,lon or ?q=lat,lon | ||
| m = msg.match(/maps\.apple\.com\/(?:\?[^\s]*?)(?:[?&]ll=)([+-]?\d+\.?\d*),([+-]?\d+\.?\d*)/i); | ||
| if (m) return { latitude: Number(m[1]), longitude: Number(m[2]) }; | ||
| m = msg.match(/maps\.apple\.com\/(?:\?[^\s]*?)(?:[?&]q=)([+-]?\d+\.?\d*),([+-]?\d+\.?\d*)/i); | ||
| if (m) return { latitude: Number(m[1]), longitude: Number(m[2]) }; | ||
|
|
||
| // Google Maps: comgooglemaps://?q=lat,lon or ...?center=lat,lon (allow %2C) | ||
| m = msg.match(/comgooglemaps:\/\/\?(?:[^\s)]*?)(?:[?&])(?:q|center)=([+-]?\d+\.?\d*)(?:,|%2C)([+-]?\d+\.?\d*)/i); | ||
| if (m) return { latitude: Number(m[1]), longitude: Number(m[2]) }; | ||
| // https://www.google.com/maps/... with query|q|center | ||
| m = msg.match(/google\.com\/maps\/(?:[^\s)]*?)(?:[?&](?:query|q|center)=)([+-]?\d+\.?\d*)(?:,|%2C)([+-]?\d+\.?\d*)/i); | ||
| if (m) return { latitude: Number(m[1]), longitude: Number(m[2]) }; | ||
| // https://www.google.com/maps/@lat,lon,zoom | ||
| m = msg.match(/google\.com\/maps\/(?:[^\s)]*?)@([+-]?\d+\.?\d*),([+-]?\d+\.?\d*)/i); | ||
| if (m) return { latitude: Number(m[1]), longitude: Number(m[2]) }; | ||
| // https://maps.google.com/?q=lat,lon (allow %2C) | ||
| m = msg.match(/maps\.google\.com\/(?:[^\s)]*?)(?:[?&](?:q|query|center)=)([+-]?\d+\.?\d*)(?:,|%2C)([+-]?\d+\.?\d*)/i); | ||
| if (m) return { latitude: Number(m[1]), longitude: Number(m[2]) }; | ||
|
|
||
| // OpenStreetMap: ?mlat=lat&mlon=lon or #map=z/lat/lon | ||
| m = msg.match(/openstreetmap\.org\/(?:[^\s]*?)(?:[?&]mlat=)([+-]?\d+\.?\d*)(?:[^\s]*?)(?:[?&]mlon=|&mlon=)([+-]?\d+\.?\d*)/i); | ||
| if (m) return { latitude: Number(m[1]), longitude: Number(m[2]) }; | ||
| m = msg.match(/openstreetmap\.org\/[^\s#]*#map=\d+\/([+-]?\d+\.?\d*)\/([+-]?\d+\.?\d*)/i); | ||
| if (m) return { latitude: Number(m[1]), longitude: Number(m[2]) }; | ||
|
|
||
| return null; | ||
| } |
There was a problem hiding this comment.
Can you write some jest test for this function like passing some url of google maps, apple maps, open street map and check if it return the expected return value or not. Test will help us to make sure that this function is working as expected in the future.
|
I am trying my best to review this PR but it's really laggy in all the browsers which is taking time. |
Thank you for taking the time to review this PR! The lag might be caused by the large number of comments. I’m considering creating another PR with the same code on a temporary branch based on this one. Each time I update the code, I’ll update both branches. For this PR, you only need to finish the final review before merge. Would that approach work better for you to review, or should we keep everything in a single PR for consistency? |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
app/containers/message/Components/LiveLocationCard.tsx (3)
30-39: Remove unused extraction logic.The memo extracts
liveLocationIdbut then simply returnsisActivewithout using it. Either complete the intended logic or simplify to just returnisActive.Apply this diff:
- const initialActiveState = useMemo(() => { - const linkMatch = msg?.match(/rocketchat:\/\/live-location\?([^)]+)/); - if (!linkMatch) return isActive; - - const params = new URLSearchParams(linkMatch[1]); - const liveLocationId = params.get('liveLocationId'); - if (!liveLocationId) return isActive; - - return isActive; - }, [msg, isActive]); + const initialActiveState = isActive;
48-87: Memoize timestamp validation function.
isMessageTooOldis recreated on every render but is used in effects. Wrap it withuseCallbackto avoid unnecessary recreations.Apply this diff:
- const isMessageTooOld = ( + const isMessageTooOld = useCallback(( timestamp?: string | Date | number | { $date?: number; valueOf?: () => number; getTime?: () => number } - ) => { + ) => { if (timestamp == null) { return false; } // ... rest of function - }; + }, []);
113-113: Remove empty comment line.Line 113 contains an empty comment. Remove it to clean up the code.
Apply this diff:
const THIRTY_MINUTES_MS = 30 * 60 * 1000; const ageMs = now - messageTime; - if (ageMs > THIRTY_MINUTES_MS) {app/containers/message/Components/CurrentLocationCard.tsx (2)
24-27: Consider importing Coords type from mapProviders.The
Coordstype is already exported fromapp/views/LocationShare/services/mapProviders.ts(with optionalaccuracyandtimestampfields). Redefining it here creates duplication and potential inconsistency.Apply this diff to import and use the existing type:
import { mapsDeepLink, staticMapUrl, providerAttribution, - providerLabel + providerLabel, + type Coords } from '../../../views/LocationShare/services/mapProviders'; import type { MapProviderName } from '../../../views/LocationShare/services/mapProviders'; -type Coords = { - latitude: number; - longitude: number; -};
243-243: Consider using theme color for ripple effect.The
android_ripplecolor is hardcoded asrgba(0,0,0,0.08). While ripple effects typically use semi-transparent overlays, consider using a theme-based color for consistency.For example:
android_ripple={{ color: colors.strokeLight }}Or create a dedicated ripple color in the theme if one doesn't exist.
📜 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 (4)
app/containers/message/Components/CurrentLocationCard.test.ts(1 hunks)app/containers/message/Components/CurrentLocationCard.tsx(1 hunks)app/containers/message/Components/LiveLocationCard.test.tsx(1 hunks)app/containers/message/Components/LiveLocationCard.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
app/containers/message/Components/LiveLocationCard.tsx (3)
app/theme.tsx (1)
useTheme(29-29)app/views/LocationShare/LiveLocationPreviewModal.tsx (5)
getCurrentLiveParams(57-59)isLiveLocationActive(745-747)addStatusChangeListener(82-84)removeStatusChangeListener(85-87)reopenLiveLocationModal(749-761)app/views/LocationShare/services/handleLiveLocationUrl.ts (3)
isLiveLocationEnded(60-63)addLiveLocationEndedListener(13-15)removeLiveLocationEndedListener(17-19)
app/containers/message/Components/CurrentLocationCard.tsx (5)
app/views/LocationShare/services/mapProviders.ts (6)
Coords(10-10)MapProviderName(5-5)staticMapUrl(76-91)providerLabel(153-153)mapsDeepLink(125-151)providerAttribution(154-154)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/lib/constants/keys.ts (2)
MAP_PROVIDER_PREFERENCE_KEY(25-25)MAP_PROVIDER_DEFAULT(28-28)
app/containers/message/Components/CurrentLocationCard.test.ts (1)
app/containers/message/Components/CurrentLocationCard.tsx (1)
extractCoordsFromMessage(50-114)
🪛 Biome (2.1.2)
app/containers/message/Components/CurrentLocationCard.tsx
[error] 67-67: The regular expression includes this negated empty character class.
Negated empty character classes match anything.
If you want to match against [, escape it [.
Otherwise, remove the character class or fill it.
(lint/correctness/noEmptyCharacterClassInRegex)
[error] 73-73: The regular expression includes this negated empty character class.
Negated empty character classes match anything.
If you want to match against [, escape it [.
Otherwise, remove the character class or fill it.
(lint/correctness/noEmptyCharacterClassInRegex)
[error] 79-79: The regular expression includes this negated empty character class.
Negated empty character classes match anything.
If you want to match against [, escape it [.
Otherwise, remove the character class or fill it.
(lint/correctness/noEmptyCharacterClassInRegex)
[error] 92-92: The regular expression includes this negated empty character class.
Negated empty character classes match anything.
If you want to match against [, escape it [.
Otherwise, remove the character class or fill it.
(lint/correctness/noEmptyCharacterClassInRegex)
🔇 Additional comments (12)
app/containers/message/Components/LiveLocationCard.test.tsx (2)
1-88: LGTM: Comprehensive test infrastructure.The mocks properly simulate stateful behavior of live location services, and the helper functions provide consistent test data generation. The setup and cleanup logic is appropriate.
90-243: LGTM: Thorough test coverage.The test suite comprehensively validates active/inactive states, navigation flows, event handling, session synchronization, and edge cases. The async testing with act and flush helpers is properly implemented.
app/containers/message/Components/LiveLocationCard.tsx (3)
1-25: LGTM: Clean imports and interface.The imports are well-organized and the props interface is clear with appropriate optional fields.
195-229: LGTM: Proper theme integration.All colors are correctly sourced from the theme hook, ensuring consistent theming across light/dark modes. The conditional rendering and styling logic is clean.
232-291: LGTM: Clean and well-organized styles.The StyleSheet contains appropriate styles for the card layout and pulse animation effect.
app/containers/message/Components/CurrentLocationCard.test.ts (1)
1-169: Excellent test coverage!The test suite comprehensively validates URL parsing across all supported map providers and formats. Edge cases like URL encoding, parameter reordering, whitespace handling, and signed coordinates are well covered.
app/containers/message/Components/CurrentLocationCard.tsx (6)
30-48: LGTM!The utility functions are well-implemented with proper error handling and clear single responsibilities. The use of
Number.isFiniteprevents NaN and Infinity values, and the try-catch inparsePairsafely handles malformed URI components.
116-118: LGTM!Clean helper function with clear single responsibility.
121-180: LGTM!Constants and styles are correctly defined outside the component, following React best practices. All color values use theme colors rather than hardcoded values.
192-195: Static map always uses OpenStreetMap.The static map preview is hardcoded to use OSM (line 194), regardless of the user's map provider preference. This is likely intentional since Google Maps Static API requires an API key, but it means users who prefer Google Maps will see an OSM preview and then open Google Maps when tapping the link. Consider documenting this design decision or ensuring it's acceptable to the team.
Is this behavior intentional? If so, consider adding a comment explaining why the static map provider differs from the deep link provider.
207-223: LGTM!Error handling is comprehensive and appropriate. Haptics and deep linking are properly wrapped in try-catch blocks, and
canOpenURLis checked before attempting to open the URL, addressing previous review feedback. Silent failures are correct for these optional enhancements.
227-276: Well-structured component implementation!The rendering logic is clean and well-organized:
- Proper use of accessibility props for both the map and button
- Effective image caching with custom cache keys
- Responsive pressed states for user feedback
- Consistent use of i18n for all user-facing text
- Theme colors applied throughout (except for the optional ripple improvement noted separately)
| if (lat !== undefined && lon !== undefined) return { latitude: lat, longitude: lon }; | ||
| } | ||
|
|
||
| m = s.match(/maps\.apple\.com\/[^]*?[?&]ll=([^&]+)/i); |
There was a problem hiding this comment.
Address static analysis warnings about [^] pattern.
The regex pattern [^] (negated empty character class) is flagged by Biome. While technically valid in JavaScript (matches any character including newlines), it's considered a code smell. Consider replacing with [\s\S] for better clarity and to satisfy the linter.
Apply this diff to address all four occurrences:
- m = s.match(/maps\.apple\.com\/[^]*?[?&]ll=([^&]+)/i);
+ m = s.match(/maps\.apple\.com\/[\s\S]*?[?&]ll=([^&]+)/i);
- m = s.match(/maps\.apple\.com\/[^]*?[?&]q=([^&]+)/i);
+ m = s.match(/maps\.apple\.com\/[\s\S]*?[?&]q=([^&]+)/i);
- m = s.match(/google\.com\/maps\/[^]*?[?&](?:q|query|center)=([^&]+)/i);
+ m = s.match(/google\.com\/maps\/[\s\S]*?[?&](?:q|query|center)=([^&]+)/i);
- m = s.match(/comgooglemaps:\/\/[^]*?[?&](?:q|center)=([^&]+)/i);
+ m = s.match(/comgooglemaps:\/\/[\s\S]*?[?&](?:q|center)=([^&]+)/i);Also applies to: 73-73, 79-79, 92-92
🧰 Tools
🪛 Biome (2.1.2)
[error] 67-67: The regular expression includes this negated empty character class.
Negated empty character classes match anything.
If you want to match against [, escape it [.
Otherwise, remove the character class or fill it.
(lint/correctness/noEmptyCharacterClassInRegex)
🤖 Prompt for AI Agents
In app/containers/message/Components/CurrentLocationCard.tsx around lines 67,
73, 79 and 92, replace the `[^]` usage in the regexes (e.g.
`maps\.apple\.com\/[^]*?[?&]ll=...`) with `[\s\S]` to avoid the Biome
static-analysis warning; update each match expression to use `[\s\S]*?` (or
`[\s\S]`-based equivalent) so the regex still matches any character including
newlines while satisfying the linter.
| const rid = params.get('rid'); | ||
| const msgId = params.get('msgId'); | ||
| if (!rid || !(msgId || liveLocationId)) { | ||
| Alert.alert(I18n.t('Error'), I18n.t('Could_not_open_live_location')); |
There was a problem hiding this comment.
Use specific translation key instead of generic 'Error'.
Lines 182 and 190 use I18n.t('Error') which was flagged in past reviews as an invalid translation key. Use a more specific key that exists in the localization file.
Based on past review comments.
Apply this diff:
if (!rid || !(msgId || liveLocationId)) {
- Alert.alert(I18n.t('Error'), I18n.t('Could_not_open_live_location'));
+ Alert.alert(I18n.t('Could_not_open_live_location'), I18n.t('Please_try_again'));
return;
}
// ...
} else {
- Alert.alert(I18n.t('Error'), I18n.t('Could_not_open_live_location'));
+ Alert.alert(I18n.t('Could_not_open_live_location'), I18n.t('Please_try_again'));
}Also applies to: 190-190
🤖 Prompt for AI Agents
In app/containers/message/Components/LiveLocationCard.tsx around lines 182 and
190, replace the generic I18n.t('Error') title with a specific, existing
translation key from the locale file (for example use a LiveLocation-specific
title key such as I18n.t('LiveLocation.errorTitle') or the exact key used for
live-location errors in your i18n files); update both Alert.alert calls to pass
that specific key so the alert title resolves correctly.
|
|
Hey @yiweigao0226 , I am going to review the pull request in the noon or evening (your local time), right now I am trying to wrap up few things 😅, sorry for the delay 😅. |
No worries, take your time. |
| <Quote {...props} /> | ||
| <Content {...props} /> | ||
| <Attachments {...props} /> | ||
| <Attachments {...props} id={props.id} rid={props.rid} /> |
There was a problem hiding this comment.
Do not pass id and rid here as a separate prop. We are passing whole props to Attachments component so will be able to access it from there
| {!isLocation ? <Content {...props} /> : null} | ||
| {/* Render a card for "current location" messages; returns null otherwise */} | ||
| <CurrentLocationCard msg={props.msg as string} /> | ||
| <Attachments {...props} id={props.id} rid={props.rid} /> |
There was a problem hiding this comment.
Do not pass id and rid here as a separate prop. We are passing whole props to Attachments component so will be able to access it from there
| {props.isInfo && props.type === 'message_pinned' ? ( | ||
| <View pointerEvents='none'> | ||
| <Attachments {...props} /> | ||
| <Attachments {...props} id={props.id} rid={props.rid} /> |
There was a problem hiding this comment.
Do not pass id and rid here as a separate prop. We are passing whole props to Attachments component so will be able to access it from there
| ); | ||
| } | ||
|
|
||
| /* eslint-disable react-native/no-unused-styles */ |
|
Also can you try to add this in the storybook for the UI testing |
4 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
Settings
Localization
Chores