feat: static & live location sharing (draft)#36746
feat: static & live location sharing (draft)#36746yiweigao0226 wants to merge 22 commits intoRocketChat:developfrom
Conversation
|
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
|
|
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (1)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including You can disable this status message by setting the WalkthroughThis PR introduces a comprehensive live location sharing feature across client and server. It adds map provider abstraction using OpenStreetMap, geolocation utilities with caching and retry logic, new React components for static and live map views, API endpoints for location lifecycle management, server-side cleanup for expired sessions, MongoDB indexes for efficiency, and extensive localization support. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant Client as Client UI
participant Geolocation as Geolocation API
participant Cache as Position Cache
participant API as API Server
participant DB as Database
User->>Client: Click "Share Location"
Client->>Geolocation: Request permission
alt Permission Granted
Client->>Cache: Check cached position
alt Cache Valid (30s)
Cache-->>Client: Return cached position
else Cache Expired/Empty
Client->>Geolocation: getCurrentPosition (quick read)
alt Quick Read Success
Geolocation-->>Client: Position (coarse quantized)
Client->>Cache: Store position
else Quick Read Fails (transient)
Client->>Geolocation: watchPosition (20s retry)
Geolocation-->>Client: Position update
Client->>Geolocation: clearWatch
Client->>Cache: Store position
else Transient Fails
Client->>Geolocation: getCurrentPosition (high accuracy)
Geolocation-->>Client: Final Position
Client->>Cache: Store position
end
end
Client->>Client: Show map preview
User->>Client: Confirm share
Client->>API: POST /api/v1/liveLocation.start
API->>DB: Create live-location attachment & message
API-->>Client: Message with map link & coordinates
Client->>User: Display in chat
else Permission Denied
Client->>User: Show error modal
end
sequenceDiagram
participant Client as Client
participant API as API Server
participant DB as Database
participant Other as Other Users
Client->>API: POST /api/v1/liveLocation.start
Note over API: Validates rid, msgId, durationSec<br/>Checks room access & membership<br/>Creates/reuses active session
API->>DB: Create/Update message with<br/>live-location attachment
API->>Other: Emit notifyOnMessageChange
API-->>Client: Return live-location data
loop Every 3s throttle (or less)
Client->>Client: Get new coordinates from watchPosition
Client->>API: POST /api/v1/liveLocation.update
Note over API: Throttle check (3s)<br/>Validate coords
API->>DB: Update attachment.coords & lastUpdateAt
API->>Other: Emit notifyOnMessageChange
end
Client->>API: POST /api/v1/liveLocation.stop
Note over API: Deactivate attachment<br/>Record stoppedAt & finalCoords
API->>DB: Update message
API->>Other: Emit notifyOnMessageChange + room event
API-->>Client: Success
sequenceDiagram
participant Startup as Startup
participant Timer as Cleanup Timer
participant DB as Database
participant Messages as Message Store
Startup->>Timer: Initialize cleanup interval
loop Every CLEANUP_INTERVAL_MS
Timer->>DB: Query for stale live-location attachments
Note over DB: Find where:<br/>- type = 'live-location'<br/>- isActive = true<br/>- lastUpdateAt < staleThreshold<br/> OR expiresAt < now
DB-->>Timer: Matching message IDs
Timer->>DB: updateMany with arrayFilters
Note over DB: Set attachments[].isActive = false<br/>Set attachments[].stoppedAt = now
Timer->>Messages: Broadcast changes for each message
Timer->>Timer: Log any failures
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/meteor/client/components/message/content/location/MapView.tsx (1)
30-31: Add OSM attribution for legal complianceOpenStreetMap usage requires attribution. Display a small attribution line below the image.
- return <MapViewImage linkUrl={linkUrl} imageUrl={imageUrl} />; + return ( + <> + <MapViewImage linkUrl={linkUrl} imageUrl={imageUrl} /> + <div style={{ fontSize: 10, color: 'var(--rcx-color-secondary-info)', marginTop: 4 }}> + © OpenStreetMap contributors + </div> + </> + );
🧹 Nitpick comments (19)
apps/meteor/client/views/room/ShareLocation/mapProvider.ts (2)
44-46: Align zoom defaults between static image and deep linkStatic default is 15 while the OSM link hardcodes 16. Pick one to avoid confusing users with different zoom levels across preview vs. deep link. I suggest defaulting both to 16.
- const zoom = opts?.zoom ?? 15; + const zoom = opts?.zoom ?? 16; ... - return `https://www.openstreetmap.org/?mlat=${lat}&mlon=${lng}#map=16/${lat}/${lng}`; + return `https://www.openstreetmap.org/?mlat=${lat}&mlon=${lng}#map=${16}/${lat}/${lng}`;Also applies to: 50-53
20-36: Avoid exposing client keys; prefer server-side proxy or strict key restrictionsThese URLs embed API keys directly in the client. If you must expose them, ensure strict domain referrer restrictions and usage quotas. Ideally, proxy static-map requests server-side to hide keys and enable caching.
Also applies to: 38-57
apps/meteor/client/views/room/ShareLocation/getGeolocationPosition.ts (1)
116-125: JSON-serializing GeolocationPosition is not guaranteed portable; store a plain subsetSome browsers return objects with non‑enumerables/getters. Safer to persist a plain serializable subset (coords + timestamp), and reconstruct a position-like object on read.
-function cache(position: GeolocationPosition) { +function cache(position: GeolocationPosition) { try { - sessionStorage.setItem( - CACHE_KEY, - JSON.stringify({ timestamp: Date.now(), position }), - ); + const { latitude, longitude, accuracy, altitude, altitudeAccuracy, heading, speed } = position.coords; + sessionStorage.setItem( + CACHE_KEY, + JSON.stringify({ + timestamp: Date.now(), + coords: { latitude, longitude, accuracy, altitude, altitudeAccuracy, heading, speed }, + }), + ); } catch { // ignore } } -function readCached(): - | { timestamp: number; position: GeolocationPosition } - | null { +function readCached(): + | { timestamp: number; position: GeolocationPosition } + | null { try { const raw = sessionStorage.getItem(CACHE_KEY); if (!raw) return null; - return JSON.parse(raw); + const parsed = JSON.parse(raw) as { + timestamp: number; + coords: GeolocationCoordinates; + }; + // Reconstruct a position-like object sufficient for downstream usage + const position = { + coords: parsed.coords, + timestamp: parsed.timestamp, + } as GeolocationPosition; + return { timestamp: parsed.timestamp, position }; } catch { return null; } }Also applies to: 127-137
apps/meteor/client/components/message/content/location/MapView.tsx (1)
22-29: Redundant null-check on linkUrl
linkUrlis always a non-empty string here. You can drop theif (!linkUrl)guard..gitpod.yml (1)
7-9: Speed up installs and ensure lockfile fidelityConsider using frozen lockfile and leveraging Gitpod cache layers.
-tasks: - - init: yarn install && yarn run build +tasks: + - init: yarn install --frozen-lockfile && yarn run build command: yarn run devapps/meteor/client/views/room/composer/messageBox/MessageBoxActionsToolbar/hooks/useShareLocationAction.tsx (1)
17-21: Update provider-dependent logic: stop tying availability to Google key only
canGetGeolocationstill depends onMapView_GMapsAPIKey. If OSM/LocationIQ is selected, the check should consider that key as well, or not require a key if you’re okay with link-only sharing.- const canGetGeolocation = isMapViewEnabled && isGeolocationCurrentPositionSupported && googleMapsApiKey && googleMapsApiKey.length; + const locationIQKey = useSetting('MapView_LocationIQ_APIKey', ''); + const canGetGeolocation = + isMapViewEnabled && + isGeolocationCurrentPositionSupported && + (Boolean(googleMapsApiKey && googleMapsApiKey.length) || Boolean(locationIQKey && locationIQKey.length));apps/meteor/client/views/room/ShareLocation/ShareLocationModal.tsx (4)
35-41: Guard localStorage access and handle SSR/blocked storage.Accessing localStorage unguarded can throw in Safari private mode/blocked storage or SSR. Wrap in try/catch and default safely.
- const [provider, setProvider] = useState<MapProviderName>(() => { - const saved = localStorage.getItem('mapProvider') as MapProviderName | null; - return saved ?? 'openstreetmap'; - }); + const [provider, setProvider] = useState<MapProviderName>(() => { + try { + const saved = typeof window !== 'undefined' ? (window.localStorage.getItem('mapProvider') as MapProviderName | null) : null; + return saved ?? 'openstreetmap'; + } catch { + return 'openstreetmap'; + } + }); ... - useEffect(() => { - localStorage.setItem('mapProvider', provider); - }, [provider]); + useEffect(() => { + try { + window.localStorage.setItem('mapProvider', provider); + } catch { + /* noop */ + } + }, [provider]);
99-136: Localize user-facing strings in the Provider stage.Hardcoded English strings bypass i18n and break l10n. Wrap title, button labels, and help text with
t().- title="Choose map provider" + title={t('Choose_map_provider')} ... - confirmText="Continue" + confirmText={t('Continue')} ... - OpenStreetMap + {t('OpenStreetMap')} ... - Google + {t('Google')} ... - <div style={{ fontSize: 12, color: '#666' }}> - You can change this later. Your choice is saved for live sharing too. - </div> + <div style={{ fontSize: 12, color: '#666' }}> + {t('Map_provider_choice_persist_help')} + </div>
142-156: Localize action labels in the Choose stage.“Live Location”, “Current Location”, and the helper sentence should use
t().- cancelText="Live Location" + cancelText={t('Live_Location')} ... - confirmText="Current Location" + confirmText={t('Current_Location')} ... - Choose to share your current location once or start live location sharing. + {t('Choose_static_or_live_location_help')}
277-279: Localize provider label.Wrap “Provider: …” with
t()for consistency.- Provider: {provider === 'google' ? 'Google Maps' : 'OpenStreetMap'} + {t('Provider')}: {provider === 'google' ? t('Google_Maps') : t('OpenStreetMap')}apps/meteor/client/views/room/ShareLocation/MapView.tsx (3)
3-3: Use a direct relative import path.
'../ShareLocation/mapProvider'from within the same directory is odd. Prefer'./mapProvider'for clarity.-import type { MapProviderName, MapProvider } from '../ShareLocation/mapProvider'; +import type { MapProviderName, MapProvider } from './mapProvider';
51-59: Reduce bandwidth and referrer leakage on external images.Add
loading="lazy"and a conservativereferrerPolicyto avoid sending chat URLs as referrers to third parties.- <img + <img src={staticUrl} alt="Map preview" - style={{ display: 'block', width: '100%', height: 'auto' }} + loading="lazy" + referrerPolicy="no-referrer" + style={{ display: 'block', width: '100%', height: 'auto' }} />
64-85: Localize fallback strings.“Map preview unavailable” should use i18n.
- aria-label="Map preview unavailable" - title="Map preview unavailable" + aria-label={/* t('Map_preview_unavailable') */ 'Map preview unavailable'} + title={/* t('Map_preview_unavailable') */ 'Map preview unavailable'}Note: If this component is used outside translation context, consider passing the label as a prop.
apps/meteor/client/views/room/ShareLocation/useLiveLocationStopListener.ts (1)
74-85: Global window pollution for control hooks.Exporting
stopLiveLocationSharingonwindowis fragile. Consider event emitters or a context instead.apps/meteor/client/views/room/ShareLocation/LiveLocationModal.tsx (3)
367-380: i18n: localize UI strings across the live modal.Most labels (“Start Live Sharing”, “Stop Sharing”, status texts, errors, tooltips, titles) are hardcoded. Wrap with
t()for localization.Also applies to: 522-649
184-196: High update/battery overhead risk.You start geolocation watch, plus a polling heartbeat in the service, plus an additional sharing interval here. This stacks timers and GPS reads, increasing battery drain and bandwidth (static maps with cache-busting). Prefer one source (watchPosition) with
shouldPushUpdategating.Also applies to: 306-313, 355-362
369-380: Avoid cache-busting on every render unless necessary.Appending
ts=Date.now()disables caching. If the preview only needs to update on position change, memoize and reuse the same URL between updates.Also applies to: 589-600
apps/meteor/client/views/room/ShareLocation/liveLocationService.ts (2)
25-70: Dev mock overrides global geolocation; gate more safely.Overwriting
navigator.geolocation.*globally can surprise other code and leak into tests if env is mis-configured. Prefer a test-only mock pluggable into the service, or guard behind an explicit feature flag and avoid mutating globals.- private setupDevelopmentMock() { - if (process.env.NODE_ENV === 'development') { + private setupDevelopmentMock() { + if (process.env.NODE_ENV === 'development' && (window as any).__RC_ENABLE_GEO_MOCK__) { // ... } }
137-169: Duplicate GPS reads (watch + polling + sharing interval).Service already sets up both a watch and a poll heartbeat; adding a separate sharing interval doubles work. Consolidate to watchPosition + throttled updates via
shouldPushUpdate.Also applies to: 171-192
📜 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 (10)
.gitpod.yml(1 hunks)apps/meteor/client/components/message/content/location/MapView.tsx(1 hunks)apps/meteor/client/views/room/ShareLocation/LiveLocationModal.tsx(1 hunks)apps/meteor/client/views/room/ShareLocation/MapView.tsx(1 hunks)apps/meteor/client/views/room/ShareLocation/ShareLocationModal.tsx(1 hunks)apps/meteor/client/views/room/ShareLocation/getGeolocationPosition.ts(1 hunks)apps/meteor/client/views/room/ShareLocation/liveLocationService.ts(1 hunks)apps/meteor/client/views/room/ShareLocation/mapProvider.ts(1 hunks)apps/meteor/client/views/room/ShareLocation/useLiveLocationStopListener.ts(1 hunks)apps/meteor/client/views/room/composer/messageBox/MessageBoxActionsToolbar/hooks/useShareLocationAction.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
apps/meteor/client/views/room/ShareLocation/ShareLocationModal.tsx (2)
apps/meteor/client/views/room/ShareLocation/mapProvider.ts (3)
MapProviderName(3-3)MapProvider(5-13)createMapProvider(60-66)apps/meteor/client/views/room/ShareLocation/getGeolocationPosition.ts (1)
getGeolocationPosition(7-73)
apps/meteor/client/views/room/ShareLocation/useLiveLocationStopListener.ts (2)
packages/ui-contexts/src/index.ts (1)
useEndpoint(32-32)apps/meteor/client/views/room/ShareLocation/liveLocationService.ts (1)
LiveLocationService(10-250)
apps/meteor/client/views/room/ShareLocation/MapView.tsx (1)
apps/meteor/client/views/room/ShareLocation/mapProvider.ts (2)
MapProviderName(3-3)MapProvider(5-13)
🪛 Gitleaks (8.28.0)
apps/meteor/client/views/room/ShareLocation/ShareLocationModal.tsx
[high] 45-45: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 44-44: Uncovered a GCP API key, which could lead to unauthorized access to Google Cloud services and data breaches.
(gcp-api-key)
apps/meteor/client/views/room/ShareLocation/useLiveLocationStopListener.ts
[high] 34-34: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
apps/meteor/client/components/message/content/location/MapView.tsx
[high] 14-14: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
apps/meteor/client/views/room/ShareLocation/LiveLocationModal.tsx
[high] 53-53: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 52-52: Uncovered a GCP API key, which could lead to unauthorized access to Google Cloud services and data breaches.
(gcp-api-key)
🔇 Additional comments (1)
apps/meteor/client/views/room/ShareLocation/getGeolocationPosition.ts (1)
105-114: Transient error detection looks goodCovers POSITION_UNAVAILABLE and common Apple CoreLocation messages. LGTM.
If you see other device-specific transient strings in telemetry, consider adding them here or gating via a feature flag.
| const locationIQKey = 'pk.898e468814facdcffda869b42260a2f0'; | ||
|
|
||
| const linkUrl = `https://maps.google.com/maps?daddr=${latitude},${longitude}`; | ||
| const linkUrl = `https://www.openstreetmap.org/?mlat=${latitude}&mlon=${longitude}#map=16/${latitude}/${longitude}`; | ||
|
|
||
| const imageUrl = useAsyncImage( | ||
| googleMapsApiKey | ||
| ? `https://maps.googleapis.com/maps/api/staticmap?zoom=14&size=250x250&markers=color:gray%7Clabel:%7C${latitude},${longitude}&key=${googleMapsApiKey}` | ||
| : undefined, | ||
| `https://maps.locationiq.com/v2/staticmap?key=${locationIQKey}¢er=${latitude},${longitude}&zoom=16&size=250x250&markers=icon:small-red-cutout|${latitude},${longitude}`, | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Hardcoded LocationIQ API key committed to repo (critical secret leak)
This is flagged by gitleaks. Keys must not be hardcoded. Read from settings and rotate the leaked key immediately.
- const locationIQKey = 'pk.898e468814facdcffda869b42260a2f0';
+ const locationIQKey = (useSetting('MapView_LocationIQ_APIKey', '') as string) || '';
- const linkUrl = `https://www.openstreetmap.org/?mlat=${latitude}&mlon=${longitude}#map=16/${latitude}/${longitude}`;
+ const linkUrl = `https://www.openstreetmap.org/?mlat=${latitude}&mlon=${longitude}#map=16/${latitude}/${longitude}`;
- const imageUrl = useAsyncImage(
- `https://maps.locationiq.com/v2/staticmap?key=${locationIQKey}¢er=${latitude},${longitude}&zoom=16&size=250x250&markers=icon:small-red-cutout|${latitude},${longitude}`,
- );
+ const staticUrl = `https://maps.locationiq.com/v2/staticmap?key=${encodeURIComponent(
+ locationIQKey,
+ )}¢er=${latitude},${longitude}&zoom=16&size=250x250&markers=icon:small-red-cutout|${latitude},${longitude}`;
+ const imageUrl = useAsyncImage(staticUrl);🏁 Script executed:
#!/bin/bash
# Find any other hardcoded LocationIQ/Google keys in the repo
rg -nP -C2 'locationiq.*key=|maps\.googleapis\.com\/maps\/api\/staticmap\?'
rg -nP -C2 'pk\.[A-Za-z0-9]{20,}'Length of output: 4984
Extract API keys to configuration and remove all hardcoded secrets
- This PR only updates MapView.tsx, but hardcoded LocationIQ (“pk.898e4688…”) and Google Maps (“AIzaSyBeNJS…”) keys also appear in:
• apps/meteor/client/views/room/ShareLocation/mapProvider.ts
• apps/meteor/client/views/room/ShareLocation/liveLocationService.ts
• apps/meteor/client/views/room/ShareLocation/useLiveLocationStopListener.ts
• apps/meteor/client/views/room/ShareLocation/ShareLocationModal.tsx
• apps/meteor/client/views/room/ShareLocation/LiveLocationModal.tsx - Move all API keys to secure settings or environment variables (e.g. via useSetting), update URL builders to consume them, then rotate the leaked keys immediately.
🧰 Tools
🪛 Gitleaks (8.28.0)
[high] 14-14: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🤖 Prompt for AI Agents
In apps/meteor/client/components/message/content/location/MapView.tsx around
lines 14 to 20, a LocationIQ API key is hardcoded (pk.898e4688...) and static
URLs are built with it; similarly other files listed in the comment contain
hardcoded LocationIQ and Google keys. Replace hardcoded keys by reading them
from secure configuration/environment (e.g. useSetting or process.env), update
all URL builders to accept the key via that setting, and remove any plaintext
secrets from source; then ensure the leaked keys are rotated immediately and
secrets are referenced consistently across the other files:
apps/meteor/client/views/room/ShareLocation/mapProvider.ts,
liveLocationService.ts, useLiveLocationStopListener.ts, ShareLocationModal.tsx,
and LiveLocationModal.tsx.
There was a problem hiding this comment.
Replace user input api keys with MapLibre
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
MapLibre GL JS implementation guide
💡 Result:
Quick MapLibre GL JS implementation guide — minimal steps, best practices, and links.
- Install or include the library
- npm: npm install maplibre-gl; import in JS/TS and import CSS. [1]
- CDN: include UNPKG JS and CSS in . [1]
- Basic map initialization
- Example:
import maplibregl from 'maplibre-gl';
import 'maplibre-gl/dist/maplibre-gl.css';
const map = new maplibregl.Map({
container: 'map', // element id
style: 'https://demotiles.maplibre.org/globe.json',
center: [lng, lat],
zoom: 2
});
[1]
- Styles & tile sources
- Use MapLibre-compatible style JSON (Mapbox Style Spec). You can host your own vector/raster tiles (MapTiler, self-hosted tileserver, etc.) or use public demo tiles. No Mapbox token required unless using Mapbox services. [1][2]
- Controls, markers, popups, events
- Add built-in controls: map.addControl(new maplibregl.NavigationControl()); create markers/popups with Marker/Popup classes; listen to map events (click, move, load). [1]
- Layers & data
- Add GeoJSON or vector sources programmatically: map.addSource(...) and map.addLayer(...). Use style expressions and paint/layout properties as in the Mapbox Style Spec. [1]
- Bundlers, TypeScript & compatibility
- MapLibre is distributed as an ES module; import in bundlers. TypeScript definitions are included. For migration from mapbox-gl, replace mapboxgl → maplibregl and update imports/CSS. v1 branch remains backward-compatible with Mapbox GL JS v1. [2][3]
- CSP & web worker notes
- For strict CSP environments use the CSP bundle (maplibre-gl-csp.js / -csp-worker.js) and set worker URL via maplibregl.setWorkerUrl(...). Required CSP directives: worker-src blob:; child-src blob:; img-src data: blob:. [1]
- Performance & debugging tips
- Use vector tiles and appropriate tile size/zoom ranges; cluster large GeoJSON sets (geojson source clustering). Use style-level filters and zoom-based layer visibility. Profile WebGL and reduce expensive paint operations. [1]
- Resources
- Official docs & quickstart (API, examples, CSP, CDN). [1]
- Mapbox→MapLibre migration guidance. [2]
- MapLibre GL JS GitHub (source, issues, releases, examples). [3]
References
[1] MapLibre GL JS docs — Quickstart, API, CSP, CDN. (maplibre.org)
[2] MapLibre Mapbox migration guide (notes on replacing mapbox-gl). (maplibre.org)
[3] maplibre/maplibre-gl-js GitHub repository. (github.com)
If you want, I can: produce a ready-to-run HTML example, show how to load custom vector tiles, or give a migration diff from mapbox-gl to maplibre-gl for your codebase. Which would help most?
Citations:
- 1: https://maplibre.org/maplibre-gl-js/docs/?utm_source=openai
- 2: https://maplibre.org/maplibre-gl-js/docs/guides/mapbox-migration-guide/?utm_source=openai
- 3: https://github.com/maplibre/maplibre-gl-js?utm_source=openai
@yiweigao0226 Great choice! MapLibre GL JS is open-source and doesn't require API keys for basic tile rendering, which completely eliminates the security issue and simplifies configuration.
Implementation approach
1. Install MapLibre
npm install maplibre-gl2. Create a MapLibre wrapper component for interactive maps
Replace the static image approach with an interactive map component:
// apps/meteor/client/components/message/content/location/MapLibreView.tsx
import { useEffect, useRef } from 'react';
import maplibregl from 'maplibre-gl';
import 'maplibre-gl/dist/maplibre-gl.css';
type MapLibreViewProps = {
latitude: number;
longitude: number;
zoom?: number;
interactive?: boolean;
};
export const MapLibreView = ({
latitude,
longitude,
zoom = 16,
interactive = true
}: MapLibreViewProps) => {
const mapContainer = useRef<HTMLDivElement>(null);
const map = useRef<maplibregl.Map | null>(null);
useEffect(() => {
if (!mapContainer.current) return;
map.current = new maplibregl.Map({
container: mapContainer.current,
style: 'https://demotiles.maplibre.org/style.json', // or use OpenStreetMap tiles
center: [longitude, latitude],
zoom,
interactive
});
new maplibregl.Marker()
.setLngLat([longitude, latitude])
.addTo(map.current);
return () => map.current?.remove();
}, [latitude, longitude, zoom, interactive]);
return <div ref={mapContainer} style={{ width: '100%', height: '250px' }} />;
};3. For static message previews, export map canvas to image
MapLibre can render maps that you can export as images using canvas methods after the map loads.
4. Update these files:
- MapView.tsx → Replace with MapLibreView component
- mapProvider.ts → Simplify to only return OSM URLs for external links
- ShareLocationModal.tsx & LiveLocationModal.tsx → Use MapLibre for preview
- liveLocationService.ts & useLiveLocationStopListener.ts → Remove hardcoded keys, generate OSM links only
5. Tile source options:
- Use MapLibre-compatible style JSON with free OpenStreetMap tiles
- No external API keys needed for basic OSM tiles
- Can self-host tiles if needed for performance/customization
This approach gives you interactive maps without any API key management, better UX, and resolves the security concern completely. Would you like me to help draft the complete refactored components?
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the current state of MapView.tsx to see if it still has the hardcoded key
cat apps/meteor/client/components/message/content/location/MapView.tsx
Length of output: 937
🏁 Script executed:
#!/bin/bash
# Search for any hardcoded LocationIQ or Google Maps API keys in the codebase
rg -n "pk\.[A-Za-z0-9]{30,}" --type ts --type tsx
rg -n "AIzaSy[A-Za-z0-9_-]{33}" --type ts --type tsx
rg -n "locationiq.*key\s*=" -i --type ts --type tsx
Length of output: 83
🏁 Script executed:
#!/bin/bash
# Check if MapLibre is being used in the MapView component
rg -n "maplibre" apps/meteor/client/components/message/content/location/
Length of output: 0
🏁 Script executed:
#!/bin/bash
# List all files in the message content location directory to understand the structure
fd . apps/meteor/client/components/message/content/location/ --type f
Length of output: 291
🏁 Script executed:
#!/bin/bash
# Check the ShareLocation MapView.tsx to see how it differs
cat apps/meteor/client/views/room/ShareLocation/MapView.tsx
Length of output: 2057
🏁 Script executed:
#!/bin/bash
# Search for any hardcoded API keys across all TypeScript/TSX files
rg -n "pk\.[A-Za-z0-9]{30,}"
rg -n "AIzaSy[A-Za-z0-9_-]{33}"
Length of output: 0
🏁 Script executed:
#!/bin/bash
# Check if there are any LocationIQ references in the codebase
rg -n "locationiq" -i
Length of output: 0
🏁 Script executed:
#!/bin/bash
# Look at the git diff for this specific file to understand what's being changed
git diff HEAD -- apps/meteor/client/components/message/content/location/MapView.tsx
Length of output: 0
🏁 Script executed:
#!/bin/bash
# Verify the mapProvider implementation has no hardcoded keys
cat apps/meteor/client/views/room/ShareLocation/mapProvider.ts | head -50
Length of output: 1569
🏁 Script executed:
#!/bin/bash
# Check what the git status shows for this file
git status apps/meteor/client/components/message/content/location/MapView.tsx
Length of output: 64
@yiweigao0226 Excellent work! 🎉 I can confirm your MapLibre implementation is complete and secure:
✅ Verified clean implementation:
- No hardcoded API keys in the codebase
mapProvider.tsuses OpenStreetMap's free static map service (no API keys required)- New location sharing components (ShareLocationModal, LiveLocationModal, MapLibreMap) use MapLibre correctly
- OSMProvider generates URLs via
staticmap.openstreetmap.fr- completely free and secure
However, regarding this specific diff:
This diff proposes adding a hardcoded LocationIQ key (pk.898e468814facdcffda869b42260a2f0) to the old message attachment renderer at apps/meteor/client/components/message/content/location/MapView.tsx. This change should not be applied as it would introduce the security vulnerability we discussed.
For the old MapView component, you have two options:
- Keep current approach: Leave it using Google Maps via
useSetting('MapView_GMapsAPIKey', '')(current secure implementation) - Migrate to MapLibre/OSM: Update it to use your new
mapProviderpattern like the ShareLocation components
Recommendation: Option 2 would provide consistency across your codebase and eliminate the need for Google Maps API keys entirely. You could reuse your OSMProvider for both new location sharing AND rendering existing location message attachments.
Would you like me to provide a refactored version of the old MapView component to use your new MapLibre-based architecture?
🧬 Code Graph Analysis Results
packages/ui-contexts/src/index.ts
<start_line>69</start_line>
<end_line>69</end_line>
useSetting
apps/meteor/client/views/room/ShareLocation/MapView.tsx
<start_line>7</start_line>
<end_line>16</end_line>
export type MapViewProps = {
latitude: number;
longitude: number;
zoom?: number;
width?: number;
height?: number;
provider?: MapProviderName;
mapInstance?: IMapProvider;
showAttribution?: boolean;
};
🧠 Learnings used
Learnt from: yiweigao0226
PR: RocketChat/Rocket.Chat#36746
File: apps/meteor/client/views/room/ShareLocation/LiveLocationModal.tsx:0-0
Timestamp: 2025-10-24T08:36:09.247Z
Learning: The location sharing feature in apps/meteor/client/views/room/ShareLocation/ uses MapLibre GL JS for map rendering instead of Google Maps or LocationIQ, eliminating the need for API keys in client code.
Learnt from: yiweigao0226
PR: RocketChat/Rocket.Chat#36746
File: apps/meteor/client/views/room/ShareLocation/ShareLocationModal.tsx:0-0
Timestamp: 2025-10-24T08:37:21.101Z
Learning: The location sharing feature is migrating from Google Maps and LocationIQ to MapLibre GL JS to eliminate hardcoded API keys in client code, addressing security concerns about key exposure in the client bundle and message attachments.
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
| const allowGeolocation = true; | ||
| // const allowGeolocation = room && canGetGeolocation && !isRoomFederated(room); | ||
|
|
There was a problem hiding this comment.
Re-enable proper gating; don’t unconditionally enable Share Location
Hardcoding allowGeolocation = true will expose the action in unsupported contexts (no geolocation, federated rooms, no provider configured). Gate on support and provider config.
- const allowGeolocation = true;
- // const allowGeolocation = room && canGetGeolocation && !isRoomFederated(room);
+ const locationIQKey = useSetting('MapView_LocationIQ_APIKey', '');
+ const anyProviderConfigured = Boolean((googleMapsApiKey && googleMapsApiKey.length) || (locationIQKey && locationIQKey.length));
+ const allowGeolocation = isMapViewEnabled && isGeolocationCurrentPositionSupported && anyProviderConfigured && !isRoomFederated(room);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const allowGeolocation = true; | |
| // const allowGeolocation = room && canGetGeolocation && !isRoomFederated(room); | |
| const locationIQKey = useSetting('MapView_LocationIQ_APIKey', ''); | |
| const anyProviderConfigured = Boolean( | |
| (googleMapsApiKey && googleMapsApiKey.length) || | |
| (locationIQKey && locationIQKey.length), | |
| ); | |
| const allowGeolocation = | |
| isMapViewEnabled && | |
| isGeolocationCurrentPositionSupported && | |
| anyProviderConfigured && | |
| !isRoomFederated(room); |
🤖 Prompt for AI Agents
In
apps/meteor/client/views/room/composer/messageBox/MessageBoxActionsToolbar/hooks/useShareLocationAction.tsx
around lines 24-26, revert the hardcoded allowGeolocation and reintroduce proper
gating: compute allowGeolocation only when room exists, the client supports
geolocation (e.g. navigator.geolocation or canGetGeolocation), the room is not
federated (isRoomFederated(room) === false), and a Share Location provider is
configured/enabled; replace the true literal with the combined boolean
expression (room && canGetGeolocation && !isRoomFederated(room) &&
isShareLocationProviderConfigured) and ensure you import or call whatever
function/flag indicates provider configuration so the action only appears when
all these checks pass.
There was a problem hiding this comment.
@yiweigao0226, thank you for the improvements to the geolocation caching! Switching from sessionStorage to in-memory caching with reduced coordinate precision and adding the toJSON method sounds like a good refinement to the getGeolocationPosition implementation.
However, in this specific file (useShareLocationAction.tsx), the concern is about the hardcoded allowGeolocation = true on line 24. This bypasses all safety checks and will expose the Share Location action even when:
- Geolocation API is not supported
- The room is federated (where location sharing may not work)
- No map provider is configured
Do you plan to restore the gating logic here, or is there a specific reason you want to keep it unconditionally enabled? If you're still working on the provider configuration checks, I can help update this to properly gate based on whether LocationIQ or Google Maps keys are configured.
🧠 Learnings used
Learnt from: yiweigao0226
PR: RocketChat/Rocket.Chat#36746
File: apps/meteor/client/views/room/ShareLocation/ShareLocationModal.tsx:0-0
Timestamp: 2025-10-24T08:37:21.088Z
Learning: The location sharing feature is migrating from Google Maps and LocationIQ to MapLibre GL JS to eliminate hardcoded API keys in client code, addressing security concerns about key exposure in the client bundle and message attachments.
Learnt from: yiweigao0226
PR: RocketChat/Rocket.Chat#36746
File: apps/meteor/client/views/room/ShareLocation/LiveLocationModal.tsx:0-0
Timestamp: 2025-10-24T08:36:09.223Z
Learning: The location sharing feature in apps/meteor/client/views/room/ShareLocation/ uses MapLibre GL JS for map rendering instead of Google Maps or LocationIQ, eliminating the need for API keys in client code.
There was a problem hiding this comment.
Changes were made by adding the checking:
const isMapViewEnabled = useSetting('MapView_Enabled') === true;
const isGeolocationCurrentPositionSupported = Boolean(navigator.geolocation?.getCurrentPosition);
// OSM-based map preview does not require a Google Maps API key
const canGetGeolocation = isMapViewEnabled && isGeolocationCurrentPositionSupported;
There was a problem hiding this comment.
@yiweigao0226, I can see you've added the proper checks for isMapViewEnabled and isGeolocationCurrentPositionSupported, and combined them into canGetGeolocation - that's great!
However, the issue is that on line 24, allowGeolocation is still hardcoded to true, which means these checks aren't actually being used:
const allowGeolocation = true; // ← This bypasses all checks
// const allowGeolocation = room && canGetGeolocation && !isRoomFederated(room);You should uncomment the original line to actually use the canGetGeolocation variable you defined:
const allowGeolocation = room && canGetGeolocation && !isRoomFederated(room);This will ensure the Share Location action is only enabled when:
- The room exists
MapView_Enabledis true- Browser supports geolocation
- The room is not federated
The checks you added are perfect for the OSM-based approach (no API key needed), but they need to be connected to allowGeolocation to take effect.
🧠 Learnings used
Learnt from: yiweigao0226
PR: RocketChat/Rocket.Chat#36746
File: apps/meteor/client/views/room/ShareLocation/LiveLocationModal.tsx:0-0
Timestamp: 2025-10-24T08:36:09.247Z
Learning: The location sharing feature in apps/meteor/client/views/room/ShareLocation/ uses MapLibre GL JS for map rendering instead of Google Maps or LocationIQ, eliminating the need for API keys in client code.
Learnt from: yiweigao0226
PR: RocketChat/Rocket.Chat#36746
File: apps/meteor/client/views/room/ShareLocation/ShareLocationModal.tsx:0-0
Timestamp: 2025-10-24T08:37:21.101Z
Learning: The location sharing feature is migrating from Google Maps and LocationIQ to MapLibre GL JS to eliminate hardcoded API keys in client code, addressing security concerns about key exposure in the client bundle and message attachments.
There was a problem hiding this comment.
@yiweigao0226, I can see you've added the proper checks for isMapViewEnabled and isGeolocationCurrentPositionSupported, and combined them into canGetGeolocation - that's great!
However, the issue is that on line 24, allowGeolocation is still hardcoded to true, which means these checks aren't actually being used:
const allowGeolocation = true; // ← This bypasses all checks
// const allowGeolocation = room && canGetGeolocation && !isRoomFederated(room);You should uncomment the original line to actually use the canGetGeolocation variable you defined:
const allowGeolocation = room && canGetGeolocation && !isRoomFederated(room);This will ensure the Share Location action is only enabled when:
- The room exists
MapView_Enabledis true- Browser supports geolocation
- The room is not federated
The checks you added are perfect for the OSM-based approach (no API key needed), but they need to be connected to allowGeolocation to take effect.
🧠 Learnings used
Learnt from: yiweigao0226
PR: RocketChat/Rocket.Chat#36746
File: apps/meteor/client/views/room/ShareLocation/LiveLocationModal.tsx:0-0
Timestamp: 2025-10-24T08:36:09.247Z
Learning: The location sharing feature in apps/meteor/client/views/room/ShareLocation/ uses MapLibre GL JS for map rendering instead of Google Maps or LocationIQ, eliminating the need for API keys in client code.
Learnt from: yiweigao0226
PR: RocketChat/Rocket.Chat#36746
File: apps/meteor/client/views/room/ShareLocation/ShareLocationModal.tsx:0-0
Timestamp: 2025-10-24T08:37:21.101Z
Learning: The location sharing feature is migrating from Google Maps and LocationIQ to MapLibre GL JS to eliminate hardcoded API keys in client code, addressing security concerns about key exposure in the client bundle and message attachments.
There was a problem hiding this comment.
This change was already made
const isMapViewEnabled = useSetting('MapView_Enabled') === true;
const isGeolocationCurrentPositionSupported = Boolean(navigator.geolocation?.getCurrentPosition);
// OSM-based map preview does not require a Google Maps API key
const canGetGeolocation = isMapViewEnabled && isGeolocationCurrentPositionSupported;
const handleShareLocation = () => setModal(<ShareLocationModal rid={room._id} tmid={tmid} onClose={() => setModal(null)} />);
const allowGeolocation = room && canGetGeolocation && !isRoomFederated(room);
There was a problem hiding this comment.
@yiweigao0226, you're absolutely right - I can see in the current file content that the fix has been applied! 🎉
The proper gating is now in place:
const allowGeolocation = room && canGetGeolocation && !isRoomFederated(room);This correctly ensures the Share Location action is only enabled when:
- ✅ The room exists
- ✅
MapView_Enabledsetting is true - ✅ Browser supports geolocation API
- ✅ The room is not federated
Since you're using MapLibre GL JS with OpenStreetMap, no API key checks are needed (as you correctly noted in the comment). The implementation looks good now!
🧠 Learnings used
Learnt from: yiweigao0226
PR: RocketChat/Rocket.Chat#36746
File: apps/meteor/client/views/room/ShareLocation/LiveLocationModal.tsx:0-0
Timestamp: 2025-10-24T08:36:09.247Z
Learning: The location sharing feature in apps/meteor/client/views/room/ShareLocation/ uses MapLibre GL JS for map rendering instead of Google Maps or LocationIQ, eliminating the need for API keys in client code.
Learnt from: yiweigao0226
PR: RocketChat/Rocket.Chat#36746
File: apps/meteor/client/views/room/ShareLocation/ShareLocationModal.tsx:0-0
Timestamp: 2025-10-24T08:37:21.101Z
Learning: The location sharing feature is migrating from Google Maps and LocationIQ to MapLibre GL JS to eliminate hardcoded API keys in client code, addressing security concerns about key exposure in the client bundle and message attachments.
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
apps/meteor/client/views/room/ShareLocation/getGeolocationPosition.ts
Outdated
Show resolved
Hide resolved
apps/meteor/client/views/room/ShareLocation/LiveLocationModal.tsx
Outdated
Show resolved
Hide resolved
apps/meteor/client/views/room/ShareLocation/LiveLocationModal.tsx
Outdated
Show resolved
Hide resolved
apps/meteor/client/views/room/ShareLocation/ShareLocationModal.tsx
Outdated
Show resolved
Hide resolved
apps/meteor/client/views/room/ShareLocation/ShareLocationModal.tsx
Outdated
Show resolved
Hide resolved
apps/meteor/client/views/room/ShareLocation/ShareLocationModal.tsx
Outdated
Show resolved
Hide resolved
apps/meteor/client/views/room/ShareLocation/useLiveLocationStopListener.ts
Outdated
Show resolved
Hide resolved
| const service = new LiveLocationService({ | ||
| locationIQKey: 'pk.898e468814facdcffda869b42260a2f0', // TODO: move to config | ||
| updateInterval: 10000, | ||
| minMoveMeters: 5 | ||
| }); |
There was a problem hiding this comment.
Remove hardcoded LocationIQ key; inject config.
Creating a service with an inline API key repeats the secret leak and forks config from the main flow.
- const service = new LiveLocationService({
- locationIQKey: 'pk.898e468814facdcffda869b42260a2f0', // TODO: move to config
+ const service = new LiveLocationService({
+ locationIQKey: '', // TODO: inject from secure app settings or reuse existing provider instanceFollow-up: pass a provider/service instance into this hook or read from a secure config source; do not persist key-bearing URLs in updates.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const service = new LiveLocationService({ | |
| locationIQKey: 'pk.898e468814facdcffda869b42260a2f0', // TODO: move to config | |
| updateInterval: 10000, | |
| minMoveMeters: 5 | |
| }); | |
| const service = new LiveLocationService({ | |
| locationIQKey: '', // TODO: inject from secure app settings or reuse existing provider instance | |
| updateInterval: 10000, | |
| minMoveMeters: 5 | |
| }); |
🧰 Tools
🪛 Gitleaks (8.28.0)
[high] 34-34: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🤖 Prompt for AI Agents
In apps/meteor/client/views/room/ShareLocation/useLiveLocationStopListener.ts
around lines 33-37 the LiveLocationService is instantiated with a hardcoded
LocationIQ API key; replace this by injecting the key or a preconfigured service
instance instead of embedding secrets in source. Update the hook to accept
either a LiveLocationService (or provider) as a parameter or read the key from a
secure config (Meteor.settings, process.env, or a centralized config module) and
pass it into the service constructor; ensure you remove the literal 'pk.898e...'
string and do not persist key-bearing URLs in state or updates.
There was a problem hiding this comment.
This file has been deleted
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if the file exists
fd -t f "useLiveLocationStopListener.ts" apps/meteor/client/views/room/ShareLocation/
Length of output: 74
🏁 Script executed:
#!/bin/bash
# Check the content of the file, particularly around the API key section
cat apps/meteor/client/views/room/ShareLocation/useLiveLocationStopListener.ts | head -40
Length of output: 1469
@yiweigao0226, the file apps/meteor/client/views/room/ShareLocation/useLiveLocationStopListener.ts still exists in the current PR branch. I can see it contains the hardcoded LocationIQ API key on line 32:
locationIQKey: 'pk.898e468814facdcffda869b42260a2f0', // TODO: move to configIf you've deleted this file locally, please make sure to:
- Commit the deletion:
git add apps/meteor/client/views/room/ShareLocation/useLiveLocationStopListener.ts - Push the changes to the PR branch
Alternatively, if you're refactoring rather than deleting, please ensure the hardcoded API key is removed and replaced with a secure configuration approach as suggested in the original review.
There was a problem hiding this comment.
This file has been deleted
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if the file exists in the current branch
if [ -f "apps/meteor/client/views/room/ShareLocation/useLiveLocationStopListener.ts" ]; then
echo "=== FILE EXISTS ==="
echo "File size:"
wc -l apps/meteor/client/views/room/ShareLocation/useLiveLocationStopListener.ts
echo ""
echo "First 40 lines:"
head -40 apps/meteor/client/views/room/ShareLocation/useLiveLocationStopListener.ts
else
echo "=== FILE DOES NOT EXIST ==="
fi
echo ""
echo "=== GIT STATUS ==="
git status --short | grep -i "useLiveLocationStopListener" || echo "No changes to useLiveLocationStopListener files in git status"
echo ""
echo "=== RECENT GIT LOG FOR THIS FILE ==="
git log --oneline --all -10 -- "apps/meteor/client/views/room/ShareLocation/useLiveLocationStopListener.ts" || echo "No commits found for this file"
echo ""
echo "=== CHECK IF FILE WAS DELETED IN RECENT COMMITS ==="
git log --diff-filter=D --summary --all -5 -- "*useLiveLocationStopListener*" || echo "No deletion commits found"
Length of output: 636
@yiweigao0226, you're absolutely right! I can now confirm that the file apps/meteor/client/views/room/ShareLocation/useLiveLocationStopListener.ts has been deleted in commit 1b969b0442 (Oct 25, 2025).
I apologize for the confusion in my previous response - it was based on outdated information. The deletion of this file successfully addresses the security concern about the hardcoded LocationIQ API key. Thank you for confirming and for removing the file! 🎉
🐰✨
/)/)
( •.•)
/>🗑️ File deleted, security issue resolved!
🧠 Learnings used
Learnt from: yiweigao0226
PR: RocketChat/Rocket.Chat#36746
File: apps/meteor/client/views/room/ShareLocation/LiveLocationModal.tsx:0-0
Timestamp: 2025-10-24T08:36:09.247Z
Learning: The location sharing feature in apps/meteor/client/views/room/ShareLocation/ uses MapLibre GL JS for map rendering instead of Google Maps or LocationIQ, eliminating the need for API keys in client code.
Learnt from: yiweigao0226
PR: RocketChat/Rocket.Chat#36746
File: apps/meteor/client/views/room/ShareLocation/ShareLocationModal.tsx:0-0
Timestamp: 2025-10-24T08:37:21.101Z
Learning: The location sharing feature is migrating from Google Maps and LocationIQ to MapLibre GL JS to eliminate hardcoded API keys in client code, addressing security concerns about key exposure in the client bundle and message attachments.
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
There was a problem hiding this comment.
Actionable comments posted: 2
📜 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 (7)
apps/meteor/app/live-location/server/index.ts(1 hunks)apps/meteor/app/live-location/server/methods/getLiveLocation.ts(1 hunks)apps/meteor/app/live-location/server/methods/startLiveLocation.ts(1 hunks)apps/meteor/app/live-location/server/methods/stopLiveLocation.ts(1 hunks)apps/meteor/app/live-location/server/methods/updateLiveLocation.ts(1 hunks)apps/meteor/app/live-location/server/startup/live-location.ts(1 hunks)apps/meteor/server/main.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
apps/meteor/app/live-location/server/methods/startLiveLocation.ts (6)
packages/core-typings/src/IUser.ts (1)
IUser(186-255)apps/meteor/app/live-location/server/methods/getLiveLocation.ts (2)
rid(11-63)userId(68-70)apps/meteor/app/live-location/server/methods/stopLiveLocation.ts (2)
rid(14-69)userId(74-76)apps/meteor/app/live-location/server/methods/updateLiveLocation.ts (2)
rid(16-81)userId(86-88)packages/core-typings/src/IRoom.ts (1)
IRoom(21-95)apps/meteor/app/lib/server/lib/notifyListener.ts (1)
notifyOnMessageChange(507-513)
apps/meteor/app/live-location/server/methods/stopLiveLocation.ts (4)
apps/meteor/app/live-location/server/methods/getLiveLocation.ts (2)
rid(11-63)userId(68-70)apps/meteor/app/live-location/server/methods/startLiveLocation.ts (2)
rid(22-109)userId(114-116)apps/meteor/app/live-location/server/methods/updateLiveLocation.ts (2)
rid(16-81)userId(86-88)apps/meteor/app/lib/server/lib/notifyListener.ts (1)
notifyOnMessageChange(507-513)
apps/meteor/app/live-location/server/methods/updateLiveLocation.ts (4)
apps/meteor/app/live-location/server/methods/getLiveLocation.ts (2)
rid(11-63)userId(68-70)apps/meteor/app/live-location/server/methods/startLiveLocation.ts (2)
rid(22-109)userId(114-116)apps/meteor/app/live-location/server/methods/stopLiveLocation.ts (2)
rid(14-69)userId(74-76)apps/meteor/app/lib/server/lib/notifyListener.ts (1)
notifyOnMessageChange(507-513)
apps/meteor/app/live-location/server/methods/getLiveLocation.ts (3)
apps/meteor/app/live-location/server/methods/startLiveLocation.ts (2)
rid(22-109)userId(114-116)apps/meteor/app/live-location/server/methods/stopLiveLocation.ts (2)
rid(14-69)userId(74-76)apps/meteor/app/live-location/server/methods/updateLiveLocation.ts (2)
rid(16-81)userId(86-88)
| const msg = { | ||
| rid, | ||
| ts: now, | ||
| u: { | ||
| _id: uid, | ||
| username: user.username, | ||
| name: (user as any).name || user.username | ||
| }, | ||
| attachments: [ | ||
| { | ||
| type: 'live-location', | ||
| live: { | ||
| isActive: true, | ||
| ownerId: uid, | ||
| startedAt: now, | ||
| lastUpdateAt: now, | ||
| expiresAt, | ||
| coords: opts.initial || null, | ||
| version: 1, | ||
| }, | ||
| }, | ||
| ], | ||
| } as any; | ||
|
|
||
| try { | ||
| const result = await Messages.insertOne(msg); | ||
|
|
||
| const createdMsg = await Messages.findOneById(result.insertedId); | ||
| if (createdMsg) { | ||
| void notifyOnMessageChange({ | ||
| id: createdMsg._id, | ||
| data: createdMsg, | ||
| }); | ||
| } | ||
|
|
||
| return { msgId: result.insertedId }; |
There was a problem hiding this comment.
Do not bypass the message service when creating the live-location message.
Writing directly with Messages.insertOne skips the normal message pipeline (room msgs/lm, subscription unread counters, mentions, notifier hooks, webhooks, etc.), so rooms will not reflect this share in unread counts or last-message summaries. Reuse the standard message insertion flow (e.g., go through the same helper we use for regular/static location messages) so all side effects fire before broadcasting the new doc.
🤖 Prompt for AI Agents
In apps/meteor/app/live-location/server/methods/startLiveLocation.ts around
lines 70–105, the code currently writes the live-location message directly with
Messages.insertOne and then manually notifies, which bypasses the standard
message pipeline (room lm/msgs, unread counters, mentions, hooks, webhooks,
etc.). Replace the direct insert with the project's central message-insertion
helper used for regular/static location messages (i.e., call the same
service/function that creates messages so all side effects run), pass the
constructed msg object into that helper, capture the created message/result it
returns, and return its id; remove the direct insertOne/findOneById and the
manual notifyOnMessageChange call so the normal hooks/notifications handle
broadcasting and counters.
There was a problem hiding this comment.
This file has been deleted to avoid using Meteor.method
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if the file startLiveLocation.ts exists in the repository
fd -t f "startLiveLocation.ts" apps/meteor/app/live-location/
Length of output: 65
🏁 Script executed:
#!/bin/bash
# Check if the file has content and see its current state
wc -l apps/meteor/app/live-location/server/methods/startLiveLocation.ts
head -20 apps/meteor/app/live-location/server/methods/startLiveLocation.ts
Length of output: 751
@yiweigao0226, thank you for the update! If the file has been deleted, then my concern about the message pipeline bypass is no longer applicable. I appreciate you addressing this.
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (2)
apps/meteor/app/live-location/server/methods/stopLiveLocation.ts (1)
8-8: Type definition duplicated across files.The
Coordstype is duplicated here and instartLiveLocation.tsandupdateLiveLocation.ts. See the comment inupdateLiveLocation.tsfor the refactoring suggestion.apps/meteor/app/live-location/server/methods/startLiveLocation.ts (1)
10-10: Type definition duplicated across files.The
Coordstype is duplicated here and inupdateLiveLocation.tsandstopLiveLocation.ts. See the comment inupdateLiveLocation.tsfor the refactoring suggestion.
🧹 Nitpick comments (10)
apps/meteor/app/live-location/server/methods/getLiveLocation.ts (2)
44-49: Redundant attachment lookup.Line 44 searches
msg.attachmentsfor alive-locationattachment, but the query at lines 28-36 already uses$elemMatchto ensure at least one such attachment exists. If the query succeeded,msg.attachmentsis guaranteed to contain a matching attachment, making this.find()call redundant.Apply this diff to simplify:
-const attachment = msg.attachments?.find((att: any) => att.type === 'live-location') as any; -if (!attachment) { - throw new Meteor.Error('error-live-location-not-found', 'Live location attachment not found', { - method: 'liveLocation.get', - }); -} +const attachment = msg.attachments?.[0] as any;
50-62: Type safety: unvalidated access toattachment.livefields.The returned object references deeply nested fields (e.g.,
attachment.live?.coords,attachment.live?.ownerId) without runtime validation. If the attachment structure is malformed or missing expected fields (e.g.,attachment.liveisnullorundefined), the method returnsundefinedfor those fields. This is safe but inconsistent with the earlier error handling (lines 38-42, 44-49) which throws explicit errors for missing data.Consider adding a validation step to ensure
attachment.liveis present and has the expected shape before constructing the return object, or document thatundefinedis an acceptable return value for missing fields.apps/meteor/app/live-location/server/methods/updateLiveLocation.ts (2)
52-56: Redundant Date construction.Line 54 constructs a
Datefromlastvianew Date(last), butlastis already typed asDate | undefined(line 52). Iflastexists, it's already aDateinstance, so the redundant construction is unnecessary. This occurs because the value is retrieved from MongoDB, which may return a date as an ISO string or a Date object depending on the driver configuration.Apply this diff to ensure consistent Date handling:
-if (last && now.getTime() - new Date(last).getTime() < MIN_INTERVAL_MS) { +if (last && now.getTime() - (last instanceof Date ? last : new Date(last)).getTime() < MIN_INTERVAL_MS) {Or, more simply, trust that MongoDB dates are already Date objects:
-if (last && now.getTime() - new Date(last).getTime() < MIN_INTERVAL_MS) { +if (last && now.getTime() - last.getTime() < MIN_INTERVAL_MS) {
84-94: Rate limiter inconsistency.The rate limiter allows 12 calls per 60 seconds (line 92), which averages to one call every 5 seconds. However, the code enforces
MIN_INTERVAL_MS = 3000(3 seconds) at line 10. This creates an inconsistency: a client could theoretically send updates every 3 seconds and hit the rate limit after 36 seconds (12 calls × 3s = 36s).Align the rate limiter with the minimum interval. If updates should occur every 3 seconds, allow 20 calls per 60 seconds (60s ÷ 3s = 20):
DDPRateLimiter.addRule( { userId(userId: string) { return !!userId; }, type: 'method', name: 'liveLocation.update', }, - 12, + 20, 60000 );Alternatively, if 12 calls per 60s is the intended limit, increase
MIN_INTERVAL_MSto 5000.apps/meteor/app/live-location/server/methods/startLiveLocation.ts (5)
12-16: Unnecessary module augmentation.The
declare module 'meteor/meteor'block at lines 12-16 augments the Meteor namespace to add a type forMeteor.user(), but this method is never called in the file. The code usesMeteor.userId()(line 26) andMeteor.users.findOneAsync()(line 62), making this augmentation unused.Remove the unused module augmentation:
-declare module 'meteor/meteor' { - namespace Meteor { - function user(): IUser | null; - } -} -
31-34: Unused room fetch.Lines 31-34 fetch the room and throw an error if not found, but the room object itself is never used afterward. The access check at line 40 (
canAccessRoomIdAsync) already verifies the room exists and the user has access. This makes the room fetch redundant.Remove the unused room fetch:
-const room = await Rooms.findOneById<IRoom>(rid); -if (!room) { - throw new Meteor.Error('error-room-not-found', 'Room not found', { method: 'liveLocation.start' }); -} -The
canAccessRoomIdAsynccheck at line 40 will handle the room-not-found case.
62-68: Potential race condition: user deletion between auth check and fetch.After verifying
uidat line 26, the code fetches the user at lines 62-64. If the user is deleted between these two operations (e.g., by an admin), the method will throwerror-invalid-user. While the window is small, this creates a potential race condition.Consider handling this scenario more gracefully by falling back to a default username if the user fetch fails:
const user = await Meteor.users.findOneAsync({ _id: uid }, { projection: { username: 1, name: 1 } }); if (!user) { - throw new Meteor.Error('error-invalid-user', 'User not found', { method: 'liveLocation.start' }); + // User was deleted after auth check - use fallback + user = { username: 'deleted-user', name: 'Deleted User' }; }Alternatively, if throwing an error is preferred, document that this is expected behavior for deleted users.
70-92: Type safety bypassed withas anycast.Line 92 casts the entire message object
as any, bypassing TypeScript's type checking. This could mask issues where the message structure doesn't match the expected schema. The cast is likely necessary because the message doesn't fully conform to theIMessagetype, but it would be safer to define a proper type for live-location messages.Consider defining a type for the message structure:
type LiveLocationMessage = { rid: string; ts: Date; u: { _id: string; username?: string; name?: string }; attachments: Array<{ type: 'live-location'; live: { isActive: boolean; ownerId: string; startedAt: Date; lastUpdateAt: Date; expiresAt?: Date; coords: Coords | null; version: number; }; }>; };Then replace
as anywithas LiveLocationMessage:-} as any; +} as LiveLocationMessage;
94-108: Incomplete error handling in try-catch block.The try-catch block at lines 94-108 only wraps the insertion and notification logic, but if
Messages.findOneById(line 97) ornotifyOnMessageChange(line 99) fails, the error won't be caught and the method may throw an unhandled exception.Expand the error handling to cover all operations:
try { const result = await Messages.insertOne(msg); const createdMsg = await Messages.findOneById(result.insertedId); if (createdMsg) { void notifyOnMessageChange({ id: createdMsg._id, data: createdMsg, }); } return { msgId: result.insertedId }; -} catch (insertError) { - throw new Meteor.Error('error-message-creation-failed', 'Failed to create live location message', { method: 'liveLocation.start' }); +} catch (error) { + throw new Meteor.Error('error-message-creation-failed', 'Failed to create live location message', { + method: 'liveLocation.start', + cause: error + }); }Alternatively, consider whether notification failures should prevent returning the
msgId. If notification is non-critical, handle it separately.apps/meteor/app/live-location/server/startup/live-location.ts (1)
39-54: Document single-attachment guarantee for live-location messages
startLiveLocationalways creates the message’sattachmentsarray with exactly one element at index 0; add a comment inlive-location.ts(and related methods) clarifying this assumption to justify the hard-codedattachments.0.
📜 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 (7)
apps/meteor/app/live-location/server/index.ts(1 hunks)apps/meteor/app/live-location/server/methods/getLiveLocation.ts(1 hunks)apps/meteor/app/live-location/server/methods/startLiveLocation.ts(1 hunks)apps/meteor/app/live-location/server/methods/stopLiveLocation.ts(1 hunks)apps/meteor/app/live-location/server/methods/updateLiveLocation.ts(1 hunks)apps/meteor/app/live-location/server/startup/live-location.ts(1 hunks)apps/meteor/server/main.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
apps/meteor/app/live-location/server/methods/updateLiveLocation.ts (4)
apps/meteor/app/live-location/server/methods/getLiveLocation.ts (2)
rid(11-63)userId(68-70)apps/meteor/app/live-location/server/methods/startLiveLocation.ts (2)
rid(22-109)userId(114-116)apps/meteor/app/live-location/server/methods/stopLiveLocation.ts (2)
rid(14-69)userId(74-76)apps/meteor/app/lib/server/lib/notifyListener.ts (1)
notifyOnMessageChange(507-513)
apps/meteor/app/live-location/server/methods/startLiveLocation.ts (6)
packages/core-typings/src/IUser.ts (1)
IUser(186-255)apps/meteor/app/live-location/server/methods/getLiveLocation.ts (2)
rid(11-63)userId(68-70)apps/meteor/app/live-location/server/methods/stopLiveLocation.ts (2)
rid(14-69)userId(74-76)apps/meteor/app/live-location/server/methods/updateLiveLocation.ts (2)
rid(16-81)userId(86-88)packages/core-typings/src/IRoom.ts (1)
IRoom(21-95)apps/meteor/app/lib/server/lib/notifyListener.ts (1)
notifyOnMessageChange(507-513)
apps/meteor/app/live-location/server/methods/stopLiveLocation.ts (4)
apps/meteor/app/live-location/server/methods/getLiveLocation.ts (2)
rid(11-63)userId(68-70)apps/meteor/app/live-location/server/methods/startLiveLocation.ts (2)
rid(22-109)userId(114-116)apps/meteor/app/live-location/server/methods/updateLiveLocation.ts (2)
rid(16-81)userId(86-88)apps/meteor/app/lib/server/lib/notifyListener.ts (1)
notifyOnMessageChange(507-513)
apps/meteor/app/live-location/server/methods/getLiveLocation.ts (3)
apps/meteor/app/live-location/server/methods/startLiveLocation.ts (2)
rid(22-109)userId(114-116)apps/meteor/app/live-location/server/methods/stopLiveLocation.ts (2)
rid(14-69)userId(74-76)apps/meteor/app/live-location/server/methods/updateLiveLocation.ts (2)
rid(16-81)userId(86-88)
🔇 Additional comments (4)
apps/meteor/app/live-location/server/methods/getLiveLocation.ts (1)
28-42: Query doesn't filter by message owner.The query at lines 28-36 locates any message with a
live-locationattachment in the room, but doesn't restrict it to messages owned by the requesting user ('u._id': uidis missing). This allows any room member to retrieve any user's live location data. While this may be intentional for a "viewer" use case, it creates a potential privacy concern if the design assumes only the owner can fetch their own live location state.Verify whether non-owners should be able to retrieve live location data. If this is by design, consider documenting the access policy. If ownership restriction is intended, apply this diff:
const msg = await Messages.findOne({ _id: msgId, rid, + 'u._id': uid, attachments: { $elemMatch: { type: 'live-location', }, }, });apps/meteor/server/main.ts (1)
20-20: LGTM!The import placement is appropriate, and the side-effect import pattern is correct for registering the live-location server methods and startup routines.
apps/meteor/app/live-location/server/index.ts (1)
1-5: LGTM!The side-effect import pattern correctly wires up the live-location server methods and startup logic. Import order is logical.
apps/meteor/app/live-location/server/startup/live-location.ts (1)
18-26: TTL index semantics verified:ensureIndexcorrectly forwardsexpireAfterSeconds: 0tocreateIndex;expiresAtis assigned wheneveropts.durationSecis truthy; thepartialFilterExpressionexcludes documents lacking a date.
| */ | ||
| async 'liveLocation.start'(rid: string, opts: { durationSec?: number; initial?: Coords } = {}) { | ||
| check(rid, String); | ||
| check(opts, Match.ObjectIncluding({ durationSec: Match.Optional(Number), initial: Match.Optional(Object) })); |
There was a problem hiding this comment.
Weak validation for opts.initial coordinates.
Line 24 validates opts with Match.Optional(Object) for the initial field, which only checks that it's an object but doesn't validate its structure. A malformed initial object (e.g., { lat: "invalid", lng: null }) will be accepted and stored, potentially breaking downstream consumers.
Apply this diff to add proper validation:
-check(opts, Match.ObjectIncluding({ durationSec: Match.Optional(Number), initial: Match.Optional(Object) }));
+check(opts, Match.ObjectIncluding({
+ durationSec: Match.Optional(Number),
+ initial: Match.Optional({
+ lat: Number,
+ lng: Number,
+ acc: Match.Optional(Number),
+ }),
+}));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| check(opts, Match.ObjectIncluding({ durationSec: Match.Optional(Number), initial: Match.Optional(Object) })); | |
| check(opts, Match.ObjectIncluding({ | |
| durationSec: Match.Optional(Number), | |
| initial: Match.Optional({ | |
| lat: Number, | |
| lng: Number, | |
| acc: Match.Optional(Number), | |
| }), | |
| })); |
🤖 Prompt for AI Agents
In apps/meteor/app/live-location/server/methods/startLiveLocation.ts around line
24, the check currently uses Match.Optional(Object) for opts.initial which
permits malformed coordinate objects; replace that with a stricter
Match.ObjectIncluding shape that requires numeric lat and lng (and optionally
numeric accuracy and timestamp) so only properly typed coordinates are accepted
(e.g. Match.Optional(Match.ObjectIncluding({ lat: Number, lng: Number, accuracy:
Match.Optional(Number), timestamp: Match.Optional(Number) }))). Ensure the
updated check call enforces this shape for opts.initial.
There was a problem hiding this comment.
This file has been deleted to avoid using Meteor.method
There was a problem hiding this comment.
Rate Limit Exceeded
@yiweigao0226 have exceeded the limit for the number of chat messages per hour. Please wait 0 minutes and 33 seconds before sending another message.
| async 'liveLocation.stop'(rid: string, msgId: string, finalCoords?: Coords) { | ||
| check(rid, String); | ||
| check(msgId, String); |
There was a problem hiding this comment.
Missing validation for optional finalCoords parameter.
Line 16 validates only rid and msgId, but not finalCoords. If a malformed finalCoords object is provided (e.g., { lat: "invalid", lng: null }), it will be stored in the database without validation, potentially breaking downstream consumers.
Apply this diff to add validation:
check(rid, String);
check(msgId, String);
+if (finalCoords !== undefined) {
+ check(finalCoords, {
+ lat: Number,
+ lng: Number,
+ acc: Match.Optional(Number),
+ });
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async 'liveLocation.stop'(rid: string, msgId: string, finalCoords?: Coords) { | |
| check(rid, String); | |
| check(msgId, String); | |
| async 'liveLocation.stop'(rid: string, msgId: string, finalCoords?: Coords) { | |
| check(rid, String); | |
| check(msgId, String); | |
| if (finalCoords !== undefined) { | |
| check(finalCoords, { | |
| lat: Number, | |
| lng: Number, | |
| acc: Match.Optional(Number), | |
| }); | |
| } |
🤖 Prompt for AI Agents
In apps/meteor/app/live-location/server/methods/stopLiveLocation.ts around lines
14–16, the optional finalCoords parameter is not validated; add validation so
when finalCoords is provided it matches the expected Coords shape (e.g., an
object with numeric lat and lng), using check with Match.Optional and
Match.Where/Match.ObjectIncluding or a shape check to ensure lat and lng are
Numbers (and validate any additional fields you expect), and reject or throw if
the structure/types are invalid before persisting to the database.
There was a problem hiding this comment.
This file has been deleted to avoid using Meteor.method
There was a problem hiding this comment.
@yiweigao0226, understood! Since the file has been deleted, the validation concern is no longer applicable. Thank you for the clarification.
| const modifier: any = { | ||
| $set: { | ||
| 'attachments.0.live.isActive': false, | ||
| 'attachments.0.live.stoppedAt': new Date(), | ||
| }, | ||
| }; | ||
|
|
||
| if (finalCoords) { | ||
| modifier.$set['attachments.0.live.coords'] = finalCoords; | ||
| } |
There was a problem hiding this comment.
Hard-coded array index assumes single attachment.
Lines 46-47 and 52 update attachments.0.live.*, assuming the live-location attachment is always at index 0. This is the same issue flagged in updateLiveLocation.ts. The selector at lines 32-42 uses $elemMatch to verify the attachment exists, but the update blindly modifies attachments.0, which could corrupt unrelated attachments if the live-location attachment is not first.
Apply the same positional operator fix as suggested in updateLiveLocation.ts:
const modifier: any = {
$set: {
- 'attachments.0.live.isActive': false,
- 'attachments.0.live.stoppedAt': new Date(),
+ 'attachments.$.live.isActive': false,
+ 'attachments.$.live.stoppedAt': new Date(),
},
};
if (finalCoords) {
- modifier.$set['attachments.0.live.coords'] = finalCoords;
+ modifier.$set['attachments.$.live.coords'] = finalCoords;
}Note: When using the positional operator $, MongoDB requires the query to include the array field in a way that identifies which element matched. The current selector with $elemMatch should work with $.
🤖 Prompt for AI Agents
In apps/meteor/app/live-location/server/methods/stopLiveLocation.ts around lines
44 to 53, the update currently hard-codes attachments.0.* which assumes the
live-location attachment is at index 0; replace those paths to use the
positional operator (attachments.$.live.*) so the update targets the array
element matched by the selector ($elemMatch already present in the query).
Update both the $set keys ('attachments.0.live.isActive',
'attachments.0.live.stoppedAt', and conditional 'attachments.0.live.coords') to
use 'attachments.$.live.isActive', 'attachments.$.live.stoppedAt', and
'attachments.$.live.coords' respectively, leaving the selector unchanged so
MongoDB can bind $ to the matched element.
There was a problem hiding this comment.
This file has been deleted to avoid using Meteor.method
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if the stopLiveLocation.ts file exists in the current branch
if [ -f "apps/meteor/app/live-location/server/methods/stopLiveLocation.ts" ]; then
echo "File exists"
cat apps/meteor/app/live-location/server/methods/stopLiveLocation.ts
else
echo "File does not exist - has been deleted"
fi
# Also check what files exist in the live-location methods directory
echo "---"
echo "Files in live-location methods directory:"
fd . apps/meteor/app/live-location/server/methods/ 2>/dev/null || echo "Directory may not exist"
# Check what alternative approach might be in place
echo "---"
echo "Checking for alternative implementations:"
rg -n "liveLocation.stop" --type=ts -g '!*.test.*' -C2
Length of output: 4724
@yiweigao0226, thanks for the update! I can see you're moving away from the Meteor.method pattern for this functionality.
However, I noticed the file apps/meteor/app/live-location/server/methods/stopLiveLocation.ts still appears to exist in the current branch with the Meteor.methods implementation. Could you clarify:
- Are you planning to delete this file in an upcoming commit?
- Or have you already pushed the deletion and I'm looking at an outdated state?
If you're migrating to a different approach (e.g., REST API endpoints), I'd be happy to review that implementation instead. The hard-coded array index issue would be moot once the file is removed. 👍
| import { DDPRateLimiter } from 'meteor/ddp-rate-limiter'; | ||
| import { notifyOnMessageChange } from '../../../lib/server/lib/notifyListener'; | ||
|
|
||
| type Coords = { lat: number; lng: number; acc?: number }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Type definition duplicated across files.
The Coords type is defined identically in startLiveLocation.ts (line 10) and stopLiveLocation.ts (line 8). Consider extracting this type to a shared types file to reduce duplication and ensure consistency.
Create a shared types file (e.g., apps/meteor/app/live-location/server/types.ts) and export the Coords type:
export type Coords = { lat: number; lng: number; acc?: number };Then import it in all three files:
+import type { Coords } from '../types';
-type Coords = { lat: number; lng: number; acc?: number };🤖 Prompt for AI Agents
In apps/meteor/app/live-location/server/methods/updateLiveLocation.ts around
line 8 the Coords type is duplicated across startLiveLocation.ts and
stopLiveLocation.ts; extract the type to a new shared file
apps/meteor/app/live-location/server/types.ts exporting export type Coords = {
lat: number; lng: number; acc?: number }; then remove the local type
declarations and import { Coords } from './types' in updateLiveLocation.ts,
startLiveLocation.ts and stopLiveLocation.ts so all three files use the single
shared type.
There was a problem hiding this comment.
This file has been deleted to avoid using Meteor.method
There was a problem hiding this comment.
Rate Limit Exceeded
@yiweigao0226 have exceeded the limit for the number of chat messages per hour. Please wait 0 minutes and 46 seconds before sending another message.
| async 'liveLocation.update'(rid: string, msgId: string, coords: Coords) { | ||
| check(rid, String); | ||
| check(msgId, String); | ||
| check(coords, Object); |
There was a problem hiding this comment.
Weak validation for coords parameter.
Line 19 uses check(coords, Object), which only verifies that coords is an object but doesn't validate its structure. A malformed coords object (e.g., missing lat or lng, or with non-numeric values) will be accepted and stored in the database, potentially breaking downstream consumers.
Apply this diff to add proper validation:
-check(coords, Object);
+check(coords, {
+ lat: Number,
+ lng: Number,
+ acc: Match.Optional(Number),
+});🤖 Prompt for AI Agents
In apps/meteor/app/live-location/server/methods/updateLiveLocation.ts around
line 19, the call check(coords, Object) only ensures coords is an object;
replace it with a stricter shape check (e.g., check(coords, { lat: Number, lng:
Number, accuracy: Match.Optional(Number), heading: Match.Optional(Number) }))
and additionally validate value ranges (lat between -90 and 90, lng between -180
and 180) and that numeric fields are finite; if validation fails, throw a
Meteor.Error with a clear message. Ensure you import Match from 'meteor/check'
if not already imported.
There was a problem hiding this comment.
This file has been deleted to avoid using Meteor.method
There was a problem hiding this comment.
Rate Limit Exceeded
@yiweigao0226 have exceeded the limit for the number of chat messages per hour. Please wait 0 minutes and 39 seconds before sending another message.
| const res = await Messages.updateOne( | ||
| { _id: msgId }, | ||
| { | ||
| $set: { | ||
| 'attachments.0.live.coords': coords, | ||
| 'attachments.0.live.lastUpdateAt': updateTime, | ||
| }, | ||
| }, | ||
| ); |
There was a problem hiding this comment.
Hard-coded array index assumes single attachment.
Lines 63-64 update attachments.0.live.coords and attachments.0.live.lastUpdateAt, assuming the live-location attachment is always at index 0. This is fragile if a message contains multiple attachments or if the live-location attachment is not the first one. While the query at lines 34-44 uses $elemMatch to find the matching attachment, the update blindly modifies attachments.0.
This could corrupt unrelated attachments if the live-location attachment is not at index 0. To fix this safely, you need to use a positional operator ($) or update all matching elements. However, MongoDB's positional operator in update operations requires the query to also use the positional filter.
The safest approach is to ensure the query result guarantees the attachment structure, or use a more robust update strategy. For now, verify that all live-location messages have exactly one attachment at index 0, or refactor to use the positional operator:
const res = await Messages.updateOne(
- { _id: msgId },
+ {
+ _id: msgId,
+ 'attachments.type': 'live-location',
+ 'attachments.live.isActive': true,
+ },
{
$set: {
- 'attachments.0.live.coords': coords,
- 'attachments.0.live.lastUpdateAt': updateTime,
+ 'attachments.$.live.coords': coords,
+ 'attachments.$.live.lastUpdateAt': updateTime,
},
},
);Note: The positional operator $ updates the first element that matched the query's $elemMatch condition.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const res = await Messages.updateOne( | |
| { _id: msgId }, | |
| { | |
| $set: { | |
| 'attachments.0.live.coords': coords, | |
| 'attachments.0.live.lastUpdateAt': updateTime, | |
| }, | |
| }, | |
| ); | |
| const res = await Messages.updateOne( | |
| { | |
| _id: msgId, | |
| 'attachments.type': 'live-location', | |
| 'attachments.live.isActive': true, | |
| }, | |
| { | |
| $set: { | |
| 'attachments.$.live.coords': coords, | |
| 'attachments.$.live.lastUpdateAt': updateTime, | |
| }, | |
| }, | |
| ); |
🤖 Prompt for AI Agents
In apps/meteor/app/live-location/server/methods/updateLiveLocation.ts around
lines 59-67, the update is mutating attachments.0 which assumes the
live-location attachment is at index 0; instead modify the update to target the
matching attachment using MongoDB positional targeting (either the positional
operator "$" by including the same $elemMatch predicate in the query so the
matched element is updated as attachments.$.live.*) or use arrayFilters to match
the attachment by a unique field (e.g., type or id) and update
attachments.$[att].live.*; adjust the query/update pair accordingly so only the
attachment that matched the original $elemMatch is updated (ensure arrayFilters
or $elemMatch are present in the update call).
There was a problem hiding this comment.
This file has been deleted to avoid using Meteor.method
There was a problem hiding this comment.
Rate Limit Exceeded
@yiweigao0226 have exceeded the limit for the number of chat messages per hour. Please wait 0 minutes and 31 seconds before sending another message.
| async function ensureIndex(collection: any, keys: any, options: any = {}) { | ||
| try { | ||
| await collection.col.createIndex(keys, options); | ||
| } catch (e) { | ||
| // ignore errors | ||
| } | ||
| } |
There was a problem hiding this comment.
Silent error swallowing in ensureIndex helper.
The ensureIndex function at lines 7-13 catches all errors and ignores them (line 10-12). While this allows startup to proceed even if index creation fails, it completely hides potential issues such as:
- Conflicting index definitions
- Insufficient permissions
- Database connectivity problems
- Index name collisions
Silent failures make debugging difficult and could lead to performance issues if critical indexes aren't created.
Apply this diff to at least log errors:
async function ensureIndex(collection: any, keys: any, options: any = {}) {
try {
await collection.col.createIndex(keys, options);
} catch (e) {
- // ignore errors
+ console.error('Failed to create index:', { keys, options, error: e });
}
}Or better yet, throw critical errors:
async function ensureIndex(collection: any, keys: any, options: any = {}) {
try {
await collection.col.createIndex(keys, options);
} catch (e: any) {
- // ignore errors
+ // Ignore "index already exists" errors, but throw others
+ if (e.code !== 85 && e.code !== 86) {
+ throw e;
+ }
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async function ensureIndex(collection: any, keys: any, options: any = {}) { | |
| try { | |
| await collection.col.createIndex(keys, options); | |
| } catch (e) { | |
| // ignore errors | |
| } | |
| } | |
| async function ensureIndex(collection: any, keys: any, options: any = {}) { | |
| try { | |
| await collection.col.createIndex(keys, options); | |
| } catch (e) { | |
| console.error('Failed to create index:', { keys, options, error: e }); | |
| } | |
| } |
🤖 Prompt for AI Agents
In apps/meteor/app/live-location/server/startup/live-location.ts around lines
7-13, the ensureIndex helper currently swallows all errors; change the catch to
log the error (including collection name, index keys and options) and then
rethrow it so critical index creation failures surface during startup. Use the
project logger if available (or console.error) to record a clear message and
error object, then throw the caught error to prevent silent failures.
There was a problem hiding this comment.
@yiweigao0226 Great! Thanks for addressing the error handling in the ensureIndex helper. 🎉
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/meteor/client/components/message/content/location/MapView.tsx (1)
7-19: Add provider abstraction to MapView.tsx
The component hardcodes LocationIQ URLs and doesn’t import or use anymapProvider.tsabstractions (MapProviderName,IMapProvider,getStaticMapUrl(),getAttribution). Create or updateapps/meteor/client/components/message/content/location/mapProvider.tsand refactor MapView to leverage it.
♻️ Duplicate comments (19)
apps/meteor/client/components/message/content/location/MapView.tsx (1)
13-19: Hardcoded API key still present—previous review's critical issue unresolved.The LocationIQ API key remains hardcoded despite being flagged in the previous review as a critical security issue. This key must be moved to secure configuration immediately and the leaked key rotated.
As recommended in the previous review, apply this diff:
- const locationIQKey = 'pk.898e468814facdcffda869b42260a2f0'; + const locationIQKey = (useSetting('MapView_LocationIQ_APIKey', '') as string) || ''; const linkUrl = `https://www.openstreetmap.org/?mlat=${latitude}&mlon=${longitude}#map=16/${latitude}/${longitude}`; - const imageUrl = useAsyncImage( - `https://maps.locationiq.com/v2/staticmap?key=${locationIQKey}¢er=${latitude},${longitude}&zoom=16&size=250x250&markers=icon:small-red-cutout|${latitude},${longitude}`, - ); + const staticUrl = `https://maps.locationiq.com/v2/staticmap?key=${encodeURIComponent( + locationIQKey, + )}¢er=${latitude},${longitude}&zoom=16&size=250x250&markers=icon:small-red-cutout|${latitude},${longitude}`; + const imageUrl = useAsyncImage(staticUrl);Additionally, ensure you import
useSettingfrom the appropriate package:import { useSetting } from '@rocket.chat/ui-contexts';Important: Rotate the exposed key immediately after applying this fix, as it has been committed to the repository.
apps/meteor/app/live-location/server/methods/stopLiveLocation.ts (2)
15-54: ValidatefinalCoordsbefore persisting.Malformatted coordinates (e.g., strings) still slide through and get saved to the live-location attachment, breaking downstream consumers. Please add a
checkclause that enforces numericlat/lng(and optional numericacc) whenfinalCoordsis supplied.check(rid, String); check(msgId, String); + if (finalCoords !== undefined) { + check( + finalCoords, + Match.ObjectIncluding({ + lat: Number, + lng: Number, + acc: Match.Optional(Number), + }), + ); + }
45-54: Stop updatingattachments.0—use the positional operator.The selector already finds the matching live-location attachment via
$elemMatch, but the modifier still hard-codes index0. If the live-location attachment is not the first entry, you’ll flip some other attachment to inactive. Switch those$setpaths toattachments.$.live.*(or array filters) so MongoDB updates the element that matched the query.const modifier: any = { $set: { - 'attachments.0.live.isActive': false, - 'attachments.0.live.stoppedAt': new Date(), + 'attachments.$.live.isActive': false, + 'attachments.$.live.stoppedAt': new Date(), }, }; if (finalCoords) { - modifier.$set['attachments.0.live.coords'] = finalCoords; + modifier.$set['attachments.$.live.coords'] = finalCoords; }apps/meteor/app/live-location/server/methods/startLiveLocation.ts (2)
23-68: Tighten validation foropts.initial.
${Match.Optional(Object)}lets any shape through (including{ lat: '0', lng: null }). Enforce numericlat/lng(and optional numericacc) so we never persist bad coordinates.- check(opts, Match.ObjectIncluding({ durationSec: Match.Optional(Number), initial: Match.Optional(Object) })); + check( + opts, + Match.ObjectIncluding({ + durationSec: Match.Optional(Number), + initial: Match.Optional( + Match.ObjectIncluding({ + lat: Number, + lng: Number, + acc: Match.Optional(Number), + }), + ), + }), + );
74-112: Don’t bypass the message pipeline.Creating the live-location message with
Messages.insertOneskips the normal message service (room counters, notifiers, webhooks, Apps, etc.), so shares won’t behave like real messages. Please route this through the standard helper we use for regular/static location messages (e.g.,insertMessage/sendMessage) and let that API return the created document instead of manualinsertOne+notifyOnMessageChange.apps/meteor/app/live-location/server/startup/live-location.ts (3)
6-12: Surface index-creation failures.
ensureIndexstill swallows every error, masking permission or definition problems. At least log (preferably rethrow) anything other than “index already exists” so ops can react when the TTL/index creation fails.async function ensureIndex(collection: any, keys: any, options: any = {}) { try { await collection.col.createIndex(keys, options); } catch (e: any) { - // ignore errors + if (e?.code !== 85 && e?.code !== 86) { + // 85/86 = duplicate index definitions in Mongo + throw e; + } } }
35-51: Add error handling around the cleanup loop.An exception from
Messages.updateMany(network hiccup, write conflict) will currently vanish inside the interval. Wrap the body intry/catchand log the failure so we know cleanup stalled.
35-51: Broadcast cleanup changes to clients.When the interval flips
isActivetofalse, nobody is notified—rooms continue showing the session as live until a reload. Capture the affected message ids (e.g., prefetch them) and callnotifyOnMessageChangeafter the update.apps/meteor/app/live-location/server/methods/updateLiveLocation.ts (3)
9-20: ShareCoordstype instead of redefining locally.This file still declares
type Coords = { lat: number; lng: number; acc?: number }, duplicating the definitions in start/stop handlers. Extract the type into a shared module (e.g.../types) and import it here to keep the shape consistent.
20-21: Tighten runtime validation ofcoords.
check(coords, Object)accepts anything object-like, so payloads missinglat/lngor carrying strings slip through and end up in the DB. Validate the shape explicitly (lat/lng numbers, optional acc) and reject invalid ranges.-import { check } from 'meteor/check'; +import { check, Match } from 'meteor/check'; … - check(coords, Object); + check(coords, { + lat: Number, + lng: Number, + acc: Match.Optional(Number), + }); + + if (!Number.isFinite(coords.lat) || coords.lat < -90 || coords.lat > 90) { + throw new Meteor.Error('error-invalid-coords', 'Latitude out of range', { method: 'liveLocation.update' }); + } + + if (!Number.isFinite(coords.lng) || coords.lng < -180 || coords.lng > 180) { + throw new Meteor.Error('error-invalid-coords', 'Longitude out of range', { method: 'liveLocation.update' }); + }Based on learnings
60-68: Use positional update instead of hard-codingattachments.0.The selector’s
$elemMatchdoesn’t guarantee the live-location attachment sits at index 0. Updatingattachments.0.*can corrupt unrelated attachments. Reuse the$elemMatchcriteria in the update and target the matched element with the$positional operator (or arrayFilters).- const res = await Messages.updateOne( - { _id: msgId }, + const res = await Messages.updateOne( + { + _id: msgId, + 'attachments.type': 'live-location', + 'attachments.live.isActive': true, + }, { $set: { - 'attachments.0.live.coords': coords, - 'attachments.0.live.lastUpdateAt': updateTime, + 'attachments.$.live.coords': coords, + 'attachments.$.live.lastUpdateAt': updateTime, }, }, );apps/meteor/client/views/room/ShareLocation/useLiveLocationStopListener.ts (2)
31-38: Remove hard-coded LocationIQ key from client bundle.The literal LocationIQ token is exposed to every browser and propagated into attachment URLs, allowing anyone to reuse or exhaust the key. Load keys from a secure server-side source (or require OSM-only static maps) and never persist them in messages.
29-49: Avoid duplicating stop-update logic.This hook rebuilds the final attachment and calls
/v1/chat.update, whileLiveLocationModal.stopSharingperforms the same update. The two competing writes race and spam history. Pick one place to own the final update (prefer the hook) and let the other just delegate to it.apps/meteor/client/views/room/ShareLocation/LiveLocationModal.tsx (3)
52-63: Strip Google/LocationIQ API keys from client code.Hardcoding
googleMapsApiKey/locationIQKeyleaks credentials to every client and into chat history. Move these keys to secured server-side config (or disable providers needing keys) and fetch them at runtime without exposing secrets. Rotate the leaked keys immediately.
219-233: Do not persist key-bearing static map URLs in attachments.
createLiveLocationAttachmentbuilds a LocationIQ image URL containing the API key, which then gets stored, forwarded and logged. Until you serve static maps through a keyless proxy (or choose a provider without keys), omitimage_url/image_typefor key-requiring providers.- image_url: staticMapUrl, - image_type: 'image/png', + ...(map.name === 'openstreetmap' + ? { image_url: staticMapUrl, image_type: 'image/png' } + : {}),
319-337: Remove duplicate “stop sharing” update block.This component already calls
stopLiveLocationSharing, which updates the message. The precedingif (messageIdRef.current && position)block issues another/v1/chat.update, producing double edits. Delete the local update and rely on the shared stop handler.- if (messageIdRef.current && position) { - try { - const finalAttachment = createLiveLocationAttachment(position, false); - await updateMessage({ - roomId: rid, - msgId: messageIdRef.current, - text: '📍 Live location sharing stopped', - attachments: [finalAttachment], - customFields: {}, - } as any); - } catch (err) { - console.error('[Stop sharing update error]', err); - } - } - await stopLiveLocationSharing(rid, messageIdRef.current || undefined, position || undefined);apps/meteor/client/views/room/ShareLocation/ShareLocationModal.tsx (2)
42-54: Remove hard-coded map provider keys from the modal.Embedding the Google and LocationIQ keys exposes them to all clients. Load keys from secure config (or disable providers needing them) and rotate the leaked credentials.
229-241: Avoid persisting static map URLs that contain API keys.
map.getStaticMapUrlreturns a LocationIQ URL with the client key; storing it inimage_urlleaks the key in message history. Skip the static image unless you proxy it server-side without a key in the URL.- const staticMapUrl = map.getStaticMapUrl(latitude, longitude, { zoom: 17, width: 512, height: 512 }); … - image_url: staticMapUrl, - image_type: 'image/png', + const staticMapUrl = + map.name === 'openstreetmap' + ? map.getStaticMapUrl(latitude, longitude, { zoom: 17, width: 512, height: 512 }) + : undefined; … - image_url: staticMapUrl, - image_type: 'image/png', + ...(staticMapUrl ? { image_url: staticMapUrl, image_type: 'image/png' } : {}),apps/meteor/client/views/room/ShareLocation/liveLocationService.ts (1)
79-122: Do not embed LocationIQ keys in generated URLs.
generateMapUrls/createLocationAttachmentbakelocationIQKeyinto the static map URL, so every message attachment leaks the credential. Serve static maps via a server proxy (no key in the client URL) or limit to provider flows that require no key.- const staticMapUrl = `https://maps.locationiq.com/v2/staticmap?key=${this.config.locationIQKey}¢er=${latitude},${longitude}&zoom=17&size=512x512&markers=icon:small-red-cutout|${latitude},${longitude}`; - return { mapsLink, staticMapUrl }; + const staticMapUrl = this.config.locationIQKey + ? undefined + : `https://maps.locationiq.com/v2/staticmap?center=${latitude},${longitude}&zoom=17&size=512x512&markers=icon:small-red-cutout|${latitude},${longitude}`; + return { mapsLink, staticMapUrl };
🧹 Nitpick comments (1)
apps/meteor/client/components/message/content/location/MapView.tsx (1)
15-18: Consider URL-encoding dynamic parameters.While
latitudeandlongitudeare typed as numbers and likely safe, consistently encoding all dynamic values in URLs is a good security practice, especially for the API key.- const linkUrl = `https://www.openstreetmap.org/?mlat=${latitude}&mlon=${longitude}#map=16/${latitude}/${longitude}`; + const linkUrl = `https://www.openstreetmap.org/?mlat=${encodeURIComponent(latitude)}&mlon=${encodeURIComponent(longitude)}#map=16/${encodeURIComponent(latitude)}/${encodeURIComponent(longitude)}`;
📜 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 (18)
apps/meteor/app/live-location/server/index.ts(1 hunks)apps/meteor/app/live-location/server/methods/getLiveLocation.ts(1 hunks)apps/meteor/app/live-location/server/methods/startLiveLocation.ts(1 hunks)apps/meteor/app/live-location/server/methods/stopLiveLocation.ts(1 hunks)apps/meteor/app/live-location/server/methods/updateLiveLocation.ts(1 hunks)apps/meteor/app/live-location/server/startup/live-location.ts(1 hunks)apps/meteor/client/components/message/content/location/MapView.tsx(1 hunks)apps/meteor/client/views/room/ShareLocation/LiveLocationModal.tsx(1 hunks)apps/meteor/client/views/room/ShareLocation/MapView.tsx(1 hunks)apps/meteor/client/views/room/ShareLocation/ShareLocationModal.tsx(1 hunks)apps/meteor/client/views/room/ShareLocation/getGeolocationPosition.ts(1 hunks)apps/meteor/client/views/room/ShareLocation/liveLocationService.ts(1 hunks)apps/meteor/client/views/room/ShareLocation/mapProvider.ts(1 hunks)apps/meteor/client/views/room/ShareLocation/useLiveLocationStopListener.ts(1 hunks)apps/meteor/client/views/room/composer/messageBox/MessageBoxActionsToolbar/hooks/useShareLocationAction.tsx(2 hunks)apps/meteor/packages/meteor-user-presence/.npm/package/.gitignore(1 hunks)apps/meteor/packages/meteor-user-presence/.npm/package/README(1 hunks)apps/meteor/packages/meteor-user-presence/.npm/package/npm-shrinkwrap.json(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- apps/meteor/packages/meteor-user-presence/.npm/package/README
- apps/meteor/packages/meteor-user-presence/.npm/package/npm-shrinkwrap.json
- apps/meteor/packages/meteor-user-presence/.npm/package/.gitignore
🚧 Files skipped from review as they are similar to previous changes (4)
- apps/meteor/client/views/room/ShareLocation/mapProvider.ts
- apps/meteor/client/views/room/ShareLocation/MapView.tsx
- apps/meteor/app/live-location/server/methods/getLiveLocation.ts
- apps/meteor/client/views/room/composer/messageBox/MessageBoxActionsToolbar/hooks/useShareLocationAction.tsx
🧰 Additional context used
🧬 Code graph analysis (6)
apps/meteor/client/views/room/ShareLocation/LiveLocationModal.tsx (3)
apps/meteor/client/views/room/ShareLocation/liveLocationService.ts (4)
LocationState(2-2)LiveLocationService(10-244)startSharing(165-186)stopSharing(188-195)apps/meteor/client/views/room/ShareLocation/useLiveLocationStopListener.ts (1)
useLiveLocationStopListener(7-94)apps/meteor/client/views/room/ShareLocation/mapProvider.ts (3)
MapProviderName(3-3)IMapProvider(5-17)createMapProvider(71-74)
apps/meteor/client/views/room/ShareLocation/ShareLocationModal.tsx (3)
packages/core-typings/src/IMessage/IMessage.ts (1)
IMessage(138-239)apps/meteor/client/views/room/ShareLocation/mapProvider.ts (3)
MapProviderName(3-3)IMapProvider(5-17)createMapProvider(71-74)apps/meteor/client/views/room/ShareLocation/getGeolocationPosition.ts (1)
getGeolocationPosition(7-73)
apps/meteor/app/live-location/server/methods/stopLiveLocation.ts (4)
apps/meteor/app/live-location/server/methods/getLiveLocation.ts (2)
rid(12-64)userId(69-71)apps/meteor/app/live-location/server/methods/startLiveLocation.ts (2)
rid(23-113)userId(118-120)apps/meteor/app/live-location/server/methods/updateLiveLocation.ts (2)
rid(17-82)userId(87-89)apps/meteor/app/lib/server/lib/notifyListener.ts (1)
notifyOnMessageChange(507-513)
apps/meteor/app/live-location/server/methods/startLiveLocation.ts (6)
packages/core-typings/src/IUser.ts (1)
IUser(186-255)apps/meteor/app/live-location/server/methods/getLiveLocation.ts (2)
rid(12-64)userId(69-71)apps/meteor/app/live-location/server/methods/stopLiveLocation.ts (2)
rid(15-70)userId(75-77)apps/meteor/app/live-location/server/methods/updateLiveLocation.ts (2)
rid(17-82)userId(87-89)packages/core-typings/src/IRoom.ts (1)
IRoom(21-95)apps/meteor/app/lib/server/lib/notifyListener.ts (1)
notifyOnMessageChange(507-513)
apps/meteor/app/live-location/server/methods/updateLiveLocation.ts (4)
apps/meteor/app/live-location/server/methods/getLiveLocation.ts (2)
rid(12-64)userId(69-71)apps/meteor/app/live-location/server/methods/startLiveLocation.ts (2)
rid(23-113)userId(118-120)apps/meteor/app/live-location/server/methods/stopLiveLocation.ts (2)
rid(15-70)userId(75-77)apps/meteor/app/lib/server/lib/notifyListener.ts (1)
notifyOnMessageChange(507-513)
apps/meteor/client/views/room/ShareLocation/useLiveLocationStopListener.ts (1)
apps/meteor/client/views/room/ShareLocation/liveLocationService.ts (1)
LiveLocationService(10-244)
🪛 Gitleaks (8.28.0)
apps/meteor/client/views/room/ShareLocation/LiveLocationModal.tsx
[high] 53-53: Uncovered a GCP API key, which could lead to unauthorized access to Google Cloud services and data breaches.
(gcp-api-key)
[high] 54-54: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
apps/meteor/client/views/room/ShareLocation/useLiveLocationStopListener.ts
[high] 32-32: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
apps/meteor/client/views/room/ShareLocation/getGeolocationPosition.ts
Outdated
Show resolved
Hide resolved
apps/meteor/client/views/room/ShareLocation/LiveLocationModal.tsx
Outdated
Show resolved
Hide resolved
apps/meteor/client/views/room/ShareLocation/ShareLocationModal.tsx
Outdated
Show resolved
Hide resolved
|
|
||
| success({ | ||
| coords: { | ||
| latitude: lat, |
Check failure
Code scanning / CodeQL
Insecure randomness High
| success({ | ||
| coords: { | ||
| latitude: lat, | ||
| longitude: lon, |
Check failure
Code scanning / CodeQL
Insecure randomness High
There was a problem hiding this comment.
Hey, there's some things that should be addressed here:
- Meteor methods should not be introduced. You'll need to provide that functionality as new endpoints - check
apps/meteor/app/api/server/v1for examples. - There are several unaddressed comments by code rabbit ai. You'll need to either leave a reply explaining why the AI comment is invalid, or address the changes in the code.
- There seem to be hard coded API keys in multiple locations. These should likely be a setting, correct?
- There are several hard coded strings in the UI. All text shown to users needs to be introduced using i18n so it can be translated.
- There are several
as anytype assertions in the code. This is generally considered a bad practice. Please make the assertions to a narrow type instead. - There are no automated tests covering the feature. Make sure to introduce testing.
- There are checks in the CI that are not passing. Make sure all these are green.
Let me know if you need further clarification
| /** | ||
| * Get live location data for a message | ||
| */ | ||
| async 'liveLocation.get'(rid: string, msgId: string) { |
There was a problem hiding this comment.
Should not create new methods, use rest APIs only
There was a problem hiding this comment.
Changes were made since this file was deleted by avoid using Meteor.methods
| _id: msgId, | ||
| rid, | ||
| attachments: { | ||
| $elemMatch: { | ||
| type: 'live-location', | ||
| }, | ||
| }, |
There was a problem hiding this comment.
Changes were made since this file was deleted by avoid using Meteor.methods
| }); | ||
| } | ||
|
|
||
| const attachment = msg.attachments?.find((att: any) => att.type === 'live-location') as any; |
There was a problem hiding this comment.
Maybe it should use the message type and not the attachment for defining the type
There was a problem hiding this comment.
Changes were made since this file was deleted by avoid using Meteor.methods
| await ensureIndex(Messages, { 'attachments.0.type': 1, 'attachments.0.live.isActive': 1 }); | ||
| await ensureIndex(Messages, { 'rid': 1, 'u._id': 1, 'attachments.0.type': 1, 'attachments.0.live.isActive': 1 }); | ||
| await ensureIndex( | ||
| Messages, | ||
| { 'attachments.0.live.expiresAt': 1 }, | ||
| { | ||
| expireAfterSeconds: 0, | ||
| partialFilterExpression: { 'attachments.0.type': 'live-location', 'attachments.0.live.expiresAt': { $type: 'date' } }, | ||
| name: 'liveLocation_expiresAt_TTL', | ||
| }, | ||
| ); | ||
| await ensureIndex( | ||
| Messages, | ||
| { 'attachments.0.live.lastUpdateAt': 1 }, | ||
| { | ||
| partialFilterExpression: { 'attachments.0.type': 'live-location' }, | ||
| name: 'liveLocation_lastUpdateAt_idx', | ||
| }, | ||
| ); |
There was a problem hiding this comment.
Wrong place to create indexes, should be in the model
There was a problem hiding this comment.
Changes were made by creating the indexes in packages/models/src/models/Messages.ts
|
|
||
| const MapView = ({ latitude, longitude }: MapViewProps) => { | ||
| const googleMapsApiKey = useSetting('MapView_GMapsAPIKey', ''); | ||
| const locationIQKey = 'pk.898e468814facdcffda869b42260a2f0'; |
| @@ -0,0 +1 @@ | |||
| node_modules | |||
There was a problem hiding this comment.
Why this change in this package?
There was a problem hiding this comment.
This file is from the develop branch.
| @@ -0,0 +1,10 @@ | |||
| { | |||
There was a problem hiding this comment.
Why this change in this package?
There was a problem hiding this comment.
This file is from the develop branch
| @@ -0,0 +1,7 @@ | |||
| This directory and the files immediately inside it are automatically generated | |||
There was a problem hiding this comment.
Why this change in this package?
There was a problem hiding this comment.
This file is from the develop branch
| "npm:stack-trace": "npm:stack-trace@0.0.10", | ||
| "npm:stack-trace@0.0.10": "npm:stack-trace@0.0.10", | ||
| "npm:uuid@8.3.2": "npm:uuid@8.3.2" | ||
| "version": "5", |
There was a problem hiding this comment.
Why this change in this package?
.gitpod.yml
Outdated
| @@ -0,0 +1,11 @@ | |||
| # This configuration file was automatically generated by Gitpod. | |||
There was a problem hiding this comment.
From a security point of view, this feature should be location service agnostic - we should not hardcode OpenStreetMap nor Google Maps. Additionally, it should be optional and enabled via a setting in the administration panel. Lots of workspace administrators may not be comfortable with having a location sharing feature enabled in their servers.
Additionally, there are some API keys that are hardcoded in the PR that should be removed. Each workspace administrator should be able to provide their own API keys via a setting or the like.
By ensuring that we make it optional and location service agnostic, we're able to offer users the flexibility of enabling or not this feature in their respective workspaces.
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/meteor/client/views/room/composer/messageBox/MessageBoxActionsToolbar/hooks/useShareLocationAction.tsx (1)
4-5: Use the app’s i18n context, not react-i18next directlyImporting useTranslation from react-i18next bypasses Rocket.Chat’s ui-contexts provider. Switch to @rocket.chat/ui-contexts for consistency and to avoid missing provider/runtime issues.
Apply this diff:
-import { useSetting, useSetModal } from '@rocket.chat/ui-contexts'; -import { useTranslation } from 'react-i18next'; +import { useSetting, useSetModal, useTranslation } from '@rocket.chat/ui-contexts';Also applies to: 14-15
🧹 Nitpick comments (24)
apps/meteor/client/startup/startup.ts (1)
7-7: Single-source MapLibre CSS import — keep it centralized.Importing CSS here is good; just ensure no other files import the same CSS to avoid duplicate styles and larger bundles. If this is the designated global import point, add a short comment to discourage re-imports elsewhere.
apps/meteor/client/views/room/ShareLocation/useBrowserLiveLocation.spec.tsx (1)
61-68: Good lifecycle coverage on geolocation watch/cleanup.Asserts coordinate updates and
clearWatch(99)on unmount — solid. Consider also asserting thatwatchPositionwas called once to guard regressions in the hook.expect(clearWatch).toHaveBeenCalledWith(99); + // Optional: ensure watch was registered exactly once + // (relies on the mock being a jest.fn) + // expect((navigator.geolocation as any).watchPosition).toHaveBeenCalledTimes(1);apps/meteor/client/views/room/ShareLocation/getGeolocationPosition.spec.ts (1)
101-117: Fallback-to-watch path looks robust; add a small assertion on clearWatch coupling.You already verify
clearWatch(1). Optionally also assert thatwatchPositionwas invoked before success to couple the expectations.await new Promise((r) => setTimeout(r, 0)); + expect(geo.watchPosition).toHaveBeenCalledTimes(1); geo.succeedWatch(11.1111, 22.2222);apps/meteor/client/views/room/ShareLocation/ShareLocationModal.spec.tsx (2)
89-123: End-to-end static flow validation is solid; add payload shape checks for attachment/link if feasible.You assert message text, rid/tmid, and close behavior. If the modal now emits a structured attachment (static location card), consider asserting that the payload includes the expected attachment with link/title from the map provider to catch formatting regressions.
72-88: Live-location disabled path: add a guard that the endpoint isn’t called.Since live is disabled here, assert
sendMessageSpywas not called to prevent accidental sends in the future.fireEvent.click(screen.getByText('Ok')); expect(screen.getByText('Current_Location')).toBeInTheDocument(); + expect(sendMessageSpy).not.toHaveBeenCalled();apps/meteor/client/views/room/ShareLocation/mapProvider.spec.ts (1)
10-16: Add tests for defaults and numeric formatting
- Also assert default args behavior: no opts passed should yield zoom 15 and 600x320 in the static URL.
- Include a case with negative coords and more decimals to ensure string formatting is stable (no unexpected precision drift).
Also applies to: 18-22, 24-27
apps/meteor/client/views/room/composer/messageBox/MessageBoxActionsToolbar/hooks/useShareLocationAction.tsx (1)
18-21: Remove redundant room check; consider a simpler geolocation capability probe
- room is already validated (throws if falsy), so
room &&in allowGeolocation is redundant.- Optional:
Boolean(navigator.geolocation)is sufficient; checkinggetCurrentPositionspecifically is not necessary.Apply this diff:
-const isGeolocationCurrentPositionSupported = Boolean(navigator.geolocation?.getCurrentPosition); -// OSM-based map preview does not require a Google Maps API key -const canGetGeolocation = isMapViewEnabled && isGeolocationCurrentPositionSupported; +const isGeolocationSupported = typeof navigator !== 'undefined' && !!navigator.geolocation; +// OSM-based map preview does not require an API key (MapLibre/OSM) +const canGetGeolocation = isMapViewEnabled && isGeolocationSupported; @@ -const allowGeolocation = room && canGetGeolocation && !isRoomFederated(room); +const allowGeolocation = canGetGeolocation && !isRoomFederated(room);Based on learnings.
Also applies to: 23-31
apps/meteor/client/views/room/ShareLocation/useBrowserLiveLocation.ts (2)
22-29: Clarify throttle semantics; avoid side effects in “shouldPublish”
shouldPublish()mutates state (updates lastSentRef) which can be surprising. Prefer a two-step API or rename to reflect the side effect.Option A (split check/commit):
-export function useBrowserLiveLocation(throttleMs = 10000) { +export function useBrowserLiveLocation(throttleMs = 10000) { @@ - function shouldPublish() { + function canPublish() { const now = Date.now(); - if (now - lastSentRef.current >= throttleMs) { - lastSentRef.current = now; - return true; - } - return false; + return now - lastSentRef.current >= throttleMs; } + + function markPublished() { + lastSentRef.current = Date.now(); + } @@ - return { coord, shouldPublish }; + return { coord, canPublish, markPublished }; }Option B (keep name but document the side effect).
Also applies to: 31-32
6-20: Minor: surface geolocation errors to the callerCurrently errors are swallowed. Consider returning an error state (e.g., set to a ref or separate error state) so the UI can inform users when permission is denied or unavailable.
apps/meteor/client/views/room/ShareLocation/getGeolocationPosition.ts (1)
11-16: Guard sessionStorage cleanup more defensively (SSR/priv. modes)Top‑level cleanup is fine, but add explicit guards to avoid ReferenceErrors in stricter runtimes.
Apply this diff:
-try { - sessionStorage.removeItem('lastGeoPosition'); -} catch { - // ignore sessionStorage unavailability (SSR, privacy mode, etc.) - void 0; -} +try { + if (typeof window !== 'undefined' && 'sessionStorage' in window) { + window.sessionStorage.removeItem('lastGeoPosition'); + } +} catch { + // ignore sessionStorage unavailability (SSR, privacy mode, etc.) +}apps/meteor/client/views/room/ShareLocation/MapLibreMap.tsx (3)
101-107: Cap trail length to prevent unbounded memory growthLive sessions may append indefinitely. Cap to a reasonable N to avoid leaks.
markerRef.current.setLngLat([LON, LAT]); - trailCoordsRef.current.push([LON, LAT]); + trailCoordsRef.current.push([LON, LAT]); + if (trailCoordsRef.current.length > 1000) { + trailCoordsRef.current.shift(); + }Also applies to: 108-121
47-51: Use built-in AttributionControl instead of a manual overlayMapLibre can render attribution from source metadata; avoids duplicated UI and keeps compliance consistent.
- map.addControl(new maplibregl.NavigationControl({ showCompass: false }), 'top-right'); + map.addControl(new maplibregl.NavigationControl({ showCompass: false }), 'top-right'); + map.addControl(new maplibregl.AttributionControl({ compact: true }), 'bottom-left'); ... - <div - style={{ - position: 'absolute', - left: 8, - bottom: 6, - fontSize: 12, - background: 'rgba(255,255,255,0.8)', - padding: '2px 6px', - borderRadius: 4, - }} - > - © OpenStreetMap contributors - </div>Also applies to: 133-146
31-39: Make tile source configurable; avoid hard-coding tile.openstreetmap.orgPublic OSM tiles aren't for heavy production use. Expose tiles URL via props so deployments can point to a self-hosted/proxy or a commercial provider, keeping you within OSM tile usage policy.
type Props = { lat: number; lon: number; zoom?: number; height?: number | string; liveCoords?: { lon: number; lat: number } | null; + tilesUrl?: string; visible?: boolean; }; -export default function MapLibreMap({ lat, lon, zoom = 15, height = 360, liveCoords, visible = true }: Props) { +export default function MapLibreMap({ + lat, + lon, + zoom = 15, + height = 360, + liveCoords, + visible = true, + tilesUrl = 'https://tile.openstreetmap.org/{z}/{x}/{y}.png', +}: Props) {Then use
[tilesUrl]instead of the hard-coded URL in the sources config at line 33.apps/meteor/client/views/room/ShareLocation/MapView.tsx (3)
50-52: Localize aria-label and title in the fallback placeholderKeep accessibility strings translatable.
- aria-label='Map preview unavailable' - title='Map preview unavailable' + aria-label={t('Map_Preview_Unavailable')} + title={t('Map_Preview_Unavailable')}
7-16: Remove or use the unusedproviderprop
provider?: MapProviderNameis declared but unused; either remove it or use it to buildmapInstance.
63-72: Optional: hint browser decoding priority for faster renderingHarmless perf win for large images.
<img src={src} alt={t('Map_Preview_Alt')} width={width} height={height} style={{ width, height, borderRadius: 8, border: '1px solid #e0e0e0', display: 'block' }} onError={() => setErrored(true)} loading='lazy' + decoding='async' + fetchPriority='low' referrerPolicy='no-referrer' />apps/meteor/client/views/room/ShareLocation/mapProvider.ts (2)
28-34: Clamp zoom and size to safe bounds to avoid oversized requestsPrevents accidental huge images and aligns with common provider limits.
getStaticMapUrl(lat: number, lng: number, opts?: { zoom?: number; width?: number; height?: number }): string { - const zoom = opts?.zoom ?? 15; - const width = opts?.width ?? 600; - const height = opts?.height ?? 320; + const zoom = Math.max(0, Math.min(19, opts?.zoom ?? 15)); + const width = Math.max(64, Math.min(1024, opts?.width ?? 600)); + const height = Math.max(64, Math.min(1024, opts?.height ?? 320)); return `https://staticmap.openstreetmap.fr/staticmap.php?center=${lat},${lng}&zoom=${zoom}&size=${width}x${height}&markers=${lat},${lng},red-pushpin`; }
33-34: Avoid relying on community staticmap.openstreetmap.fr for productionThis service is community-run and rate-limited; it may be unreliable at scale. Prefer a self-hosted static map renderer, a proxy with caching, or a commercial tiles/static service. Also add CDN caching headers if proxied.
apps/meteor/client/views/room/ShareLocation/ShareLocationModal.tsx (2)
36-37: Avoid hard-coding provider; allow configurationHard-coding 'openstreetmap' makes it harder to swap providers or styles. Consider wiring this to a setting or passing a provider instance from the caller.
- const map: IMapProvider = useMemo(() => createMapProvider('openstreetmap', {}), []); + const map: IMapProvider = useMemo(() => createMapProvider('openstreetmap', {}), []); + // TODO: read provider from settings/feature flag and pass style/options as needed
211-217: Optional: use static MapView for preview to reduce tile loadInteractive MapLibre in the modal is heavier than necessary for a confirm step. A static preview (MapView) lowers network/CPU and aligns with attachment previews.
apps/meteor/app/live-location/server/startup/live-location.ts (2)
70-77: Narrow the arrayFilter to only stale/expired attachments.As written, any active live-location attachment in the matched messages will be deactivated, not just the stale/expired ones. Include the same stale predicates in the arrayFilter to avoid deactivating non-stale attachments when a message contains multiple live-location entries.
-const options: UpdateOptions = { - arrayFilters: [ - { - 'liveAtt.type': 'live-location', - 'liveAtt.live.isActive': true, - }, - ], -}; +const options: UpdateOptions = { + arrayFilters: [ + { + 'liveAtt.type': 'live-location', + 'liveAtt.live.isActive': true, + $or: [ + { 'liveAtt.live.lastUpdateAt': { $lt: staleBefore } }, + { 'liveAtt.live.expiresAt': { $lte: now } }, + ], + }, + ], +};Also applies to: 63-77
91-99: Bound broadcast concurrency for large batches.If many IDs are cleaned at once, a single Promise.allSettled can spike CPU/GC. Consider batching (e.g., 50–100) to smooth load; preserves ordering neutrality.
-const results = await Promise.allSettled(ids.map((id) => broadcastMessageUpdate(id))); +const chunk = <T,>(arr: T[], n: number) => Array.from({ length: Math.ceil(arr.length / n) }, (_, i) => arr.slice(i * n, i * n + n)); +let rejected = 0; +for (const batch of chunk(ids, 100)) { + const res = await Promise.allSettled(batch.map((id) => broadcastMessageUpdate(id))); + rejected += res.filter((r) => r.status === 'rejected').length; +} +if (rejected) { + console.error(`[LiveLocationCleanup] broadcastMessageUpdate(): ${rejected} failures of ${ids.length}`); +}apps/meteor/app/api/server/v1/liveLocation.ts (2)
46-51: Remove Meteor interface augmentation here.This module-level augmentation of Meteor.user() is unnecessary and risks clashing with global typings. Prefer relying on this.userId provided by the API context.
-declare module 'meteor/meteor' { - // eslint-disable-next-line @typescript-eslint/naming-convention - interface Meteor { - user(): IUser | null; - } -}
158-160: Consider API rate limiting at the route level.You already throttle by MIN_INTERVAL_MS per message, but adding a per-user route limiter helps protect the server. Suggest adding rateLimiterOptions to update (and possibly start/stop) with sane defaults.
Please confirm the preferred pattern in this codebase; if supported, something like:
-API.v1.addRoute( - 'liveLocation.update', - { authRequired: true }, +API.v1.addRoute( + 'liveLocation.update', + { authRequired: true, rateLimiterOptions: { numRequests: 60, intervalTime: 60_000 } },
📜 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 (18)
apps/meteor/app/api/server/index.ts(1 hunks)apps/meteor/app/api/server/v1/liveLocation.ts(1 hunks)apps/meteor/app/live-location/server/index.ts(1 hunks)apps/meteor/app/live-location/server/startup/live-location.ts(1 hunks)apps/meteor/client/startup/startup.ts(1 hunks)apps/meteor/client/views/room/ShareLocation/MapLibreMap.tsx(1 hunks)apps/meteor/client/views/room/ShareLocation/MapView.spec.tsx(1 hunks)apps/meteor/client/views/room/ShareLocation/MapView.tsx(1 hunks)apps/meteor/client/views/room/ShareLocation/ShareLocationModal.spec.tsx(1 hunks)apps/meteor/client/views/room/ShareLocation/ShareLocationModal.tsx(1 hunks)apps/meteor/client/views/room/ShareLocation/getGeolocationPosition.spec.ts(1 hunks)apps/meteor/client/views/room/ShareLocation/getGeolocationPosition.ts(1 hunks)apps/meteor/client/views/room/ShareLocation/mapProvider.spec.ts(1 hunks)apps/meteor/client/views/room/ShareLocation/mapProvider.ts(1 hunks)apps/meteor/client/views/room/ShareLocation/useBrowserLiveLocation.spec.tsx(1 hunks)apps/meteor/client/views/room/ShareLocation/useBrowserLiveLocation.ts(1 hunks)apps/meteor/client/views/room/composer/messageBox/MessageBoxActionsToolbar/hooks/useShareLocationAction.tsx(1 hunks)apps/meteor/package.json(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: yiweigao0226
PR: RocketChat/Rocket.Chat#36746
File: apps/meteor/client/views/room/ShareLocation/ShareLocationModal.tsx:0-0
Timestamp: 2025-10-24T08:37:21.101Z
Learning: The location sharing feature is migrating from Google Maps and LocationIQ to MapLibre GL JS to eliminate hardcoded API keys in client code, addressing security concerns about key exposure in the client bundle and message attachments.
Learnt from: yiweigao0226
PR: RocketChat/Rocket.Chat#36746
File: apps/meteor/client/views/room/ShareLocation/LiveLocationModal.tsx:0-0
Timestamp: 2025-10-24T08:36:09.247Z
Learning: The location sharing feature in apps/meteor/client/views/room/ShareLocation/ uses MapLibre GL JS for map rendering instead of Google Maps or LocationIQ, eliminating the need for API keys in client code.
📚 Learning: 2025-10-24T08:36:09.247Z
Learnt from: yiweigao0226
PR: RocketChat/Rocket.Chat#36746
File: apps/meteor/client/views/room/ShareLocation/LiveLocationModal.tsx:0-0
Timestamp: 2025-10-24T08:36:09.247Z
Learning: The location sharing feature in apps/meteor/client/views/room/ShareLocation/ uses MapLibre GL JS for map rendering instead of Google Maps or LocationIQ, eliminating the need for API keys in client code.
Applied to files:
apps/meteor/client/views/room/composer/messageBox/MessageBoxActionsToolbar/hooks/useShareLocationAction.tsxapps/meteor/client/views/room/ShareLocation/MapLibreMap.tsxapps/meteor/client/views/room/ShareLocation/ShareLocationModal.tsxapps/meteor/client/views/room/ShareLocation/mapProvider.ts
📚 Learning: 2025-10-24T08:37:21.101Z
Learnt from: yiweigao0226
PR: RocketChat/Rocket.Chat#36746
File: apps/meteor/client/views/room/ShareLocation/ShareLocationModal.tsx:0-0
Timestamp: 2025-10-24T08:37:21.101Z
Learning: The location sharing feature is migrating from Google Maps and LocationIQ to MapLibre GL JS to eliminate hardcoded API keys in client code, addressing security concerns about key exposure in the client bundle and message attachments.
Applied to files:
apps/meteor/client/views/room/composer/messageBox/MessageBoxActionsToolbar/hooks/useShareLocationAction.tsxapps/meteor/client/views/room/ShareLocation/ShareLocationModal.tsxapps/meteor/client/views/room/ShareLocation/mapProvider.ts
🧬 Code graph analysis (8)
apps/meteor/app/live-location/server/startup/live-location.ts (2)
apps/meteor/app/lib/server/lib/notifyListener.ts (1)
notifyOnMessageChange(507-513)packages/core-typings/src/IMessage/IMessage.ts (1)
IMessage(138-239)
apps/meteor/app/api/server/v1/liveLocation.ts (3)
packages/core-typings/src/IUser.ts (1)
IUser(186-255)packages/core-typings/src/IRoom.ts (1)
IRoom(21-95)apps/meteor/app/lib/server/lib/notifyListener.ts (1)
notifyOnMessageChange(507-513)
apps/meteor/client/views/room/ShareLocation/ShareLocationModal.spec.tsx (1)
apps/meteor/client/views/room/ShareLocation/getGeolocationPosition.ts (1)
getGeolocationPosition(45-114)
apps/meteor/client/views/room/ShareLocation/getGeolocationPosition.spec.ts (1)
apps/meteor/client/views/room/ShareLocation/getGeolocationPosition.ts (1)
getGeolocationPosition(45-114)
apps/meteor/client/views/room/ShareLocation/useBrowserLiveLocation.spec.tsx (1)
apps/meteor/client/views/room/ShareLocation/useBrowserLiveLocation.ts (1)
useBrowserLiveLocation(3-32)
apps/meteor/client/views/room/ShareLocation/ShareLocationModal.tsx (3)
apps/meteor/client/views/room/ShareLocation/mapProvider.ts (2)
IMapProvider(9-14)createMapProvider(46-52)apps/meteor/client/views/room/ShareLocation/getGeolocationPosition.ts (1)
getGeolocationPosition(45-114)apps/meteor/client/views/room/ShareLocation/MapLibreMap.tsx (1)
MapLibreMap(16-148)
apps/meteor/client/views/room/ShareLocation/MapView.tsx (1)
apps/meteor/client/views/room/ShareLocation/mapProvider.ts (2)
MapProviderName(3-3)IMapProvider(9-14)
apps/meteor/client/views/room/ShareLocation/mapProvider.spec.ts (1)
apps/meteor/client/views/room/ShareLocation/mapProvider.ts (2)
createMapProvider(46-52)OSMProvider(16-44)
🪛 Biome (2.1.2)
apps/meteor/client/views/room/ShareLocation/MapLibreMap.tsx
[error] 3-3: Do not shadow the global "Map" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
🔇 Additional comments (9)
apps/meteor/client/views/room/ShareLocation/getGeolocationPosition.spec.ts (1)
67-72: Nice negative-path coverage for API-unavailable case.Deleting
navigator.geolocationand asserting the thrown message aligns with the module’s guard (typeof window/'geolocation' in navigator). Looks good.apps/meteor/client/views/room/ShareLocation/ShareLocationModal.spec.tsx (1)
61-66: Good test harness with QueryClient; disable retries to keep tests deterministic.The provider wrapper and retry disabled setting are appropriate for modal flows that depend on immediate state.
apps/meteor/package.json (1)
395-395: MapLibre GL worker configuration not yet implemented — requires CSP verification.The codebase currently imports MapLibre without explicit worker configuration (
workerClass). The CSS import atapps/meteor/client/startup/startup.ts:7is present and not duplicated. However, MapLibre will default to using a blob:-based worker, which may violate strict CSP policies commonly enforced in Rocket.Chat environments.Before merging:
- Verify your CSP policy allows
worker-src blob:and tile/image sources used by your style provider- If strict CSP is enforced without blob: support, configure MapLibre with
workerClassto use an inline or CSP-compatible worker- Confirm tile domains in your style configuration are whitelisted in CSP and proxy settings
apps/meteor/client/views/room/ShareLocation/mapProvider.spec.ts (1)
3-8: LGTM: default provider coverage is solidAssertions for instance type and provider name look good.
apps/meteor/client/views/room/ShareLocation/MapView.spec.tsx (1)
13-23: Strong, focused tests; good coverage of fallbacks and attributionCases for no mapInstance, image render with attribution, hiding attribution, and error fallback look good.
Also applies to: 25-38, 40-48, 50-62
apps/meteor/client/views/room/ShareLocation/getGeolocationPosition.ts (1)
45-114: Acquisition flow and in‑memory cache look goodMulti‑stage get (quick, watch fallback, final high‑accuracy), coarse quantization (3 dp), no persisted PII. Solid improvements over sessionStorage. Consider exporting MAX_CACHE_AGE_MS/COARSE_DP for testability if needed.
apps/meteor/client/views/room/composer/messageBox/MessageBoxActionsToolbar/hooks/useShareLocationAction.tsx (1)
17-23: Review assertion contradicted by codebase stateThe verification search found hundreds of remaining imports of
useTranslationfromreact-i18nextthroughout client code, contradicting the review comment's request to verify their absence. The file under review itself importsuseTranslationat line 5, though the code snippet (lines 17–23) shows no direct usage of the hook.Key findings:
- Multiple packages retain these imports:
packages/livechat/,packages/web-ui-registration/,packages/ui-voip/,packages/ui-client/,apps/meteor/client/, and many othersuseShareLocationAction.tsximports the hook but doesn't appear to call it in the shown linesClarification needed: Is the import in this file intentional (perhaps used elsewhere in the full file), or should it be removed as part of a broader migration?
apps/meteor/app/api/server/index.ts (1)
24-24: Correct placement of live-location API import.Loaded before OpenAPI generation; endpoints will be included. LGTM.
apps/meteor/app/live-location/server/index.ts (1)
1-1: Side-effect import is fine.This wiring is sufficient to start the cleanup job on server startup. LGTM.
apps/meteor/client/views/room/ShareLocation/ShareLocationModal.tsx
Outdated
Show resolved
Hide resolved
apps/meteor/client/views/room/ShareLocation/useBrowserLiveLocation.spec.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (2)
apps/meteor/app/api/server/v1/liveLocation.ts (2)
302-327: Good fix: arrayFilters used for stop; avoids attachments[0] pitfalls.Selector is scoped to
_id,rid,u._idand usesattachments.$[liveAtt]withisActive: true. This closes the wrong-attachment/TOCTOU issue.
228-233: Enforce throttle atomically in Mongo; don’t read attachments[0] in app code.Current logic reads
attachments?.[0].live.lastUpdateAtand compares in app, which is (a) racy under concurrency and (b) may read the wrong attachment. Push the throttle predicate intoarrayFiltersand remove the app-side time check.Apply:
- const last: Date | undefined = (msg.attachments?.[0] as ILiveLocationAttachment)?.live?.lastUpdateAt; - const now = new Date(); - if (last && now.getTime() - new Date(last).getTime() < MIN_INTERVAL_MS) { - return API.v1.success({ ignored: true, reason: 'too-soon' }); - } - - const updateTime = new Date(); + const now = new Date(); + const threshold = new Date(now.getTime() - MIN_INTERVAL_MS); const res = await Messages.updateOne( { _id: msgId, rid, 'u._id': uid }, { $set: { 'attachments.$[liveAtt].live.coords': coords, - 'attachments.$[liveAtt].live.lastUpdateAt': updateTime, + 'attachments.$[liveAtt].live.lastUpdateAt': now, }, }, { arrayFilters: [ { 'liveAtt.type': 'live-location', 'liveAtt.live.isActive': true, + $or: [ + { 'liveAtt.live.lastUpdateAt': { $lt: threshold } }, + { 'liveAtt.live.lastUpdateAt': { $exists: false } }, + ], }, ], } as any, ); - if (res.modifiedCount > 0) { + if (res.modifiedCount === 0) { + return API.v1.success({ ignored: true, reason: 'too-soon-or-not-active' }); + } + { const updatedMsg = await Messages.findOneById(msgId); if (updatedMsg) { void notifyOnMessageChange({ id: updatedMsg._id, data: updatedMsg, }); } }Also applies to: 235-251
🧹 Nitpick comments (4)
apps/meteor/client/views/room/ShareLocation/MapLibreMap.tsx (2)
33-33: Consider using the mapProvider abstraction for tile URL.The OSM tile URL is hardcoded. Based on the AI summary mentioning
mapProvider.tswithOSMProviderand attribution helpers, consider using that abstraction for consistency and easier provider switching.
133-145: Attribution could leverage mapProvider abstraction.The attribution overlay is hardcoded. The
mapProvider.tsmodule mentioned in the AI summary provides attribution methods that could centralize this concern and ensure consistency across static and live maps.apps/meteor/app/api/server/v1/liveLocation.ts (2)
12-21: Make isValidCoords a proper type guard and simplify truthiness check.Small TS polish; explicit return type helps callers and narrows types.
-const isValidCoords = (c?: { lat: number; lon: number }) => - c && - typeof c.lat === 'number' && +const isValidCoords = (c?: unknown): c is { lat: number; lon: number } => + !!c && + typeof (c as any).lat === 'number' && Number.isFinite(c.lat) && c.lat >= -90 && c.lat <= 90 && - typeof c.lon === 'number' && + typeof (c as any).lon === 'number' && Number.isFinite(c.lon) && c.lon >= -180 && c.lon <= 180;
58-63: Move Meteor typings augmentation to a central d.ts.Local augmentation can conflict/duplicate across files. Prefer a single
apps/meteor/types/meteor-user.d.ts(or existing types package).
📜 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)
apps/meteor/app/api/server/v1/liveLocation.ts(1 hunks)apps/meteor/client/views/room/ShareLocation/MapLibreMap.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: yiweigao0226
PR: RocketChat/Rocket.Chat#36746
File: apps/meteor/client/views/room/ShareLocation/ShareLocationModal.tsx:0-0
Timestamp: 2025-10-24T08:37:21.101Z
Learning: The location sharing feature is migrating from Google Maps and LocationIQ to MapLibre GL JS to eliminate hardcoded API keys in client code, addressing security concerns about key exposure in the client bundle and message attachments.
Learnt from: yiweigao0226
PR: RocketChat/Rocket.Chat#36746
File: apps/meteor/client/views/room/ShareLocation/LiveLocationModal.tsx:0-0
Timestamp: 2025-10-24T08:36:09.247Z
Learning: The location sharing feature in apps/meteor/client/views/room/ShareLocation/ uses MapLibre GL JS for map rendering instead of Google Maps or LocationIQ, eliminating the need for API keys in client code.
📚 Learning: 2025-10-24T08:36:09.247Z
Learnt from: yiweigao0226
PR: RocketChat/Rocket.Chat#36746
File: apps/meteor/client/views/room/ShareLocation/LiveLocationModal.tsx:0-0
Timestamp: 2025-10-24T08:36:09.247Z
Learning: The location sharing feature in apps/meteor/client/views/room/ShareLocation/ uses MapLibre GL JS for map rendering instead of Google Maps or LocationIQ, eliminating the need for API keys in client code.
Applied to files:
apps/meteor/client/views/room/ShareLocation/MapLibreMap.tsx
🧬 Code graph analysis (1)
apps/meteor/app/api/server/v1/liveLocation.ts (3)
packages/core-typings/src/IUser.ts (1)
IUser(186-255)packages/core-typings/src/IRoom.ts (1)
IRoom(21-95)apps/meteor/app/lib/server/lib/notifyListener.ts (1)
notifyOnMessageChange(507-513)
🔇 Additional comments (2)
apps/meteor/client/views/room/ShareLocation/MapLibreMap.tsx (2)
3-3: Map shadowing issue resolved.The import now uses the alias
MLMapto avoid shadowing the globalMap, addressing the previous review concern.
87-92: LGTM!This effect correctly updates the map center and marker position when coordinates change, without requiring map re-initialization.
| const existing = await Messages.findOne({ | ||
| rid, | ||
| 'u._id': uid, | ||
| 'attachments': { | ||
| $elemMatch: { | ||
| 'type': 'live-location', | ||
| 'live.isActive': true, | ||
| }, | ||
| }, | ||
| }); | ||
|
|
||
| if (existing) { | ||
| return API.v1.success({ msgId: existing._id }); | ||
| } |
There was a problem hiding this comment.
Close “double start” race; ensure single active live-location per user/room.
Two concurrent starts can both miss existing and insert duplicates. Add a partial unique index or make start atomic.
Option A (preferred): partial unique index (migration/startup):
await Messages.col.createIndex(
{ rid: 1, 'u._id': 1, 'attachments.type': 1, 'attachments.live.isActive': 1 },
{
name: 'uniq_active_live_location_per_user_room',
unique: true,
partialFilterExpression: {
'attachments.type': 'live-location',
'attachments.live.isActive': true,
},
},
);Option B: atomic upsert (still recommend A as the guard):
- const existing = await Messages.findOne({ rid, 'u._id': uid, 'attachments': { $elemMatch: { 'type': 'live-location', 'live.isActive': true } } });
- if (existing) {
- return API.v1.success({ msgId: existing._id });
- }
+ const existing = await Messages.findOne({ rid, 'u._id': uid, 'attachments': { $elemMatch: { type: 'live-location', 'live.isActive': true } } });
+ if (existing) return API.v1.success({ msgId: existing._id });
...
- const result = await Messages.insertOne(msg);
+ const result = await Messages.insertOne(msg); // guarded by unique index aboveAlso applies to: 159-173
🤖 Prompt for AI Agents
In apps/meteor/app/api/server/v1/liveLocation.ts around lines 106-119 (and
similarly at 159-173), two concurrent "start" requests can both miss the
existing query and insert duplicate active live-location attachments; add a
durable guard by adding a partial unique index at app startup/migration on {
rid, 'u._id', 'attachments.type', 'attachments.live.isActive' } with a name like
'uniq_active_live_location_per_user_room' and a partialFilterExpression that
only applies when attachments.type==='live-location' and
attachments.live.isActive===true, and also make the start operation atomic
(e.g., perform an upsert or findOneAndUpdate that only inserts when no active
attachment exists) so that even without the index concurrent requests cannot
create duplicates; apply the same changes for the code at lines 159-173.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the current state of the liveLocation.ts file
cat apps/meteor/app/api/server/v1/liveLocation.ts
Length of output: 10743
🏁 Script executed:
#!/bin/bash
# Check for any unique index creation related to live-location in startup files
rg -n "createIndex.*live.*location" --type=ts -i
Length of output: 0
🏁 Script executed:
#!/bin/bash
# Check for any index creation with partialFilterExpression
rg -n "partialFilterExpression" --type=ts -A5 -B5
Length of output: 15226
🏁 Script executed:
#!/bin/bash
# Look for any migration or startup files that might create indexes for Messages collection
fd -e ts -e js | xargs rg -l "Messages.*createIndex|createIndex.*Messages" | head -20
Length of output: 478
🏁 Script executed:
#!/bin/bash
# Check the full modelIndexes method in Messages.ts to see all indexes
cat packages/models/src/models/Messages.ts | grep -A 100 "protected modelIndexes"
Length of output: 3058
🏁 Script executed:
#!/bin/bash
# Look specifically for unique indexes in Messages model
rg -n "unique.*true" packages/models/src/models/Messages.ts -A2 -B2
Length of output: 0
🏁 Script executed:
#!/bin/bash
# Check if there's a unique index for the live-location scenario
rg -n "'rid'.*'u._id'.*'attachments'" packages/models/src/models/Messages.ts -A3 -B3
Length of output: 0
🏁 Script executed:
#!/bin/bash
# Check the exact line 68 in Messages.ts to see the index definition
sed -n '67,69p' packages/models/src/models/Messages.ts
Length of output: 176
🏁 Script executed:
#!/bin/bash
# Search for any unique constraint related to live-location in the entire codebase
rg -n "unique.*true" --type=ts -C5 | rg -A5 -B5 "live-location|liveLocation"
Length of output: 0
🏁 Script executed:
#!/bin/bash
# Check if there are any database migrations that add a unique index
fd migration -e ts | xargs rg -l "live.location|liveLocation" 2>/dev/null
Length of output: 0
@yiweigao0226 Thank you for adding the error handling! I can see you've added a try-catch block (lines 159-173 in liveLocation.ts) to handle duplicate key errors. However, the fix is incomplete:
What's currently in place:
- ✅ Error handling for code 11000 (duplicate key error)
- ✅ Retry logic to find existing message on duplicate
What's still missing:
- ❌ The unique index in the database
The problem:
Without a unique index at the database level, MongoDB will never throw error code 11000 for duplicate live-location messages. Your error handling code will never be triggered, so concurrent requests can still create duplicates.
To complete the fix:
Add this unique index in packages/models/src/models/Messages.ts around line 68:
{ key: { 'rid': 1, 'u._id': 1, 'attachments.0.type': 1, 'attachments.0.live.isActive': 1 } },
// Add the unique constraint:
{
key: { 'rid': 1, 'u._id': 1, 'attachments.0.type': 1, 'attachments.0.live.isActive': 1 },
unique: true,
partialFilterExpression: {
'attachments.0.type': 'live-location',
'attachments.0.live.isActive': true,
},
name: 'uniq_active_live_location_per_user_room',
},This will:
- Prevent duplicate active live-location messages at the database level
- Trigger the error code 11000 that your error handling is designed to catch
- Ensure your retry logic in the catch block actually works
Without this index, the race condition remains unresolved.
🧠 Learnings used
Learnt from: yiweigao0226
PR: RocketChat/Rocket.Chat#36746
File: apps/meteor/client/views/room/ShareLocation/LiveLocationModal.tsx:0-0
Timestamp: 2025-10-24T08:36:09.247Z
Learning: The location sharing feature in apps/meteor/client/views/room/ShareLocation/ uses MapLibre GL JS for map rendering instead of Google Maps or LocationIQ, eliminating the need for API keys in client code.
Learnt from: yiweigao0226
PR: RocketChat/Rocket.Chat#36746
File: apps/meteor/client/views/room/ShareLocation/ShareLocationModal.tsx:0-0
Timestamp: 2025-10-24T08:37:21.101Z
Learning: The location sharing feature is migrating from Google Maps and LocationIQ to MapLibre GL JS to eliminate hardcoded API keys in client code, addressing security concerns about key exposure in the client bundle and message attachments.
| useEffect(() => { | ||
| if (!liveCoords || !mapRef.current || !markerRef.current) return; | ||
| const { lon: LON, lat: LAT } = liveCoords; | ||
|
|
||
| markerRef.current.setLngLat([LON, LAT]); | ||
| trailCoordsRef.current.push([LON, LAT]); | ||
|
|
||
| if (trailRef.current) { | ||
| const data: FeatureCollection<LineString> = { | ||
| type: 'FeatureCollection', | ||
| features: [ | ||
| { | ||
| type: 'Feature', | ||
| properties: {}, | ||
| geometry: { type: 'LineString', coordinates: trailCoordsRef.current }, | ||
| }, | ||
| ], | ||
| }; | ||
| trailRef.current.setData(data); | ||
| } | ||
| }, [liveCoords]); |
There was a problem hiding this comment.
Implement trail reset mechanism to prevent unbounded memory growth.
The trail coordinates accumulate indefinitely via trailCoordsRef.current.push([LON, LAT]) without any size limit or reset mechanism during the component's lifetime. In long-running live location sessions, this could lead to significant memory consumption and performance degradation.
Consider adding:
- A size limit (e.g., keep only the last N coordinates)
- A prop to reset the trail when starting a new session
- Time-based cleanup (remove coordinates older than X minutes)
Example implementation for size-limited trail:
markerRef.current.setLngLat([LON, LAT]);
- trailCoordsRef.current.push([LON, LAT]);
+ const maxTrailLength = 1000;
+ trailCoordsRef.current.push([LON, LAT]);
+ if (trailCoordsRef.current.length > maxTrailLength) {
+ trailCoordsRef.current.shift();
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| useEffect(() => { | |
| if (!liveCoords || !mapRef.current || !markerRef.current) return; | |
| const { lon: LON, lat: LAT } = liveCoords; | |
| markerRef.current.setLngLat([LON, LAT]); | |
| trailCoordsRef.current.push([LON, LAT]); | |
| if (trailRef.current) { | |
| const data: FeatureCollection<LineString> = { | |
| type: 'FeatureCollection', | |
| features: [ | |
| { | |
| type: 'Feature', | |
| properties: {}, | |
| geometry: { type: 'LineString', coordinates: trailCoordsRef.current }, | |
| }, | |
| ], | |
| }; | |
| trailRef.current.setData(data); | |
| } | |
| }, [liveCoords]); | |
| useEffect(() => { | |
| if (!liveCoords || !mapRef.current || !markerRef.current) return; | |
| const { lon: LON, lat: LAT } = liveCoords; | |
| markerRef.current.setLngLat([LON, LAT]); | |
| const maxTrailLength = 1000; | |
| trailCoordsRef.current.push([LON, LAT]); | |
| if (trailCoordsRef.current.length > maxTrailLength) { | |
| trailCoordsRef.current.shift(); | |
| } | |
| if (trailRef.current) { | |
| const data: FeatureCollection<LineString> = { | |
| type: 'FeatureCollection', | |
| features: [ | |
| { | |
| type: 'Feature', | |
| properties: {}, | |
| geometry: { type: 'LineString', coordinates: trailCoordsRef.current }, | |
| }, | |
| ], | |
| }; | |
| trailRef.current.setData(data); | |
| } | |
| }, [liveCoords]); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the current implementation of the trail update logic in MapLibreMap.tsx
cat apps/meteor/client/views/room/ShareLocation/MapLibreMap.tsx | sed -n '99,120p'
Length of output: 564
🏁 Script executed:
#!/bin/bash
# Get the complete trail update effect to see the implementation
cat apps/meteor/client/views/room/ShareLocation/MapLibreMap.tsx | sed -n '115,145p'
Length of output: 755
@yiweigao0226, excellent! I can confirm you've implemented the trail size limit exactly as suggested. The trail now maintains a maximum of 1000 coordinates, preventing unbounded memory growth during long-running live location sessions. This ensures stable performance and memory usage. Great work! 🎉
🧠 Learnings used
Learnt from: yiweigao0226
PR: RocketChat/Rocket.Chat#36746
File: apps/meteor/client/views/room/ShareLocation/LiveLocationModal.tsx:0-0
Timestamp: 2025-10-24T08:36:09.247Z
Learning: The location sharing feature in apps/meteor/client/views/room/ShareLocation/ uses MapLibre GL JS for map rendering instead of Google Maps or LocationIQ, eliminating the need for API keys in client code.
Learnt from: yiweigao0226
PR: RocketChat/Rocket.Chat#36746
File: apps/meteor/client/views/room/ShareLocation/ShareLocationModal.tsx:0-0
Timestamp: 2025-10-24T08:37:21.101Z
Learning: The location sharing feature is migrating from Google Maps and LocationIQ to MapLibre GL JS to eliminate hardcoded API keys in client code, addressing security concerns about key exposure in the client bundle and message attachments.
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
There was a problem hiding this comment.
Actionable comments posted: 2
📜 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)
apps/meteor/client/views/room/ShareLocation/ShareLocationModal.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: yiweigao0226
PR: RocketChat/Rocket.Chat#36746
File: apps/meteor/client/views/room/ShareLocation/ShareLocationModal.tsx:0-0
Timestamp: 2025-10-24T08:37:21.101Z
Learning: The location sharing feature is migrating from Google Maps and LocationIQ to MapLibre GL JS to eliminate hardcoded API keys in client code, addressing security concerns about key exposure in the client bundle and message attachments.
Learnt from: yiweigao0226
PR: RocketChat/Rocket.Chat#36746
File: apps/meteor/client/views/room/ShareLocation/LiveLocationModal.tsx:0-0
Timestamp: 2025-10-24T08:36:09.247Z
Learning: The location sharing feature in apps/meteor/client/views/room/ShareLocation/ uses MapLibre GL JS for map rendering instead of Google Maps or LocationIQ, eliminating the need for API keys in client code.
📚 Learning: 2025-10-24T08:37:21.101Z
Learnt from: yiweigao0226
PR: RocketChat/Rocket.Chat#36746
File: apps/meteor/client/views/room/ShareLocation/ShareLocationModal.tsx:0-0
Timestamp: 2025-10-24T08:37:21.101Z
Learning: The location sharing feature is migrating from Google Maps and LocationIQ to MapLibre GL JS to eliminate hardcoded API keys in client code, addressing security concerns about key exposure in the client bundle and message attachments.
Applied to files:
apps/meteor/client/views/room/ShareLocation/ShareLocationModal.tsx
📚 Learning: 2025-10-24T08:36:09.247Z
Learnt from: yiweigao0226
PR: RocketChat/Rocket.Chat#36746
File: apps/meteor/client/views/room/ShareLocation/LiveLocationModal.tsx:0-0
Timestamp: 2025-10-24T08:36:09.247Z
Learning: The location sharing feature in apps/meteor/client/views/room/ShareLocation/ uses MapLibre GL JS for map rendering instead of Google Maps or LocationIQ, eliminating the need for API keys in client code.
Applied to files:
apps/meteor/client/views/room/ShareLocation/ShareLocationModal.tsx
🧬 Code graph analysis (1)
apps/meteor/client/views/room/ShareLocation/ShareLocationModal.tsx (4)
packages/core-typings/src/IMessage/IMessage.ts (1)
IMessage(138-239)apps/meteor/client/views/room/ShareLocation/mapProvider.ts (2)
IMapProvider(9-14)createMapProvider(46-52)apps/meteor/client/views/room/ShareLocation/getGeolocationPosition.ts (1)
getGeolocationPosition(45-114)apps/meteor/client/views/room/ShareLocation/MapLibreMap.tsx (1)
MapLibreMap(16-148)
apps/meteor/client/views/room/ShareLocation/ShareLocationModal.tsx
Outdated
Show resolved
Hide resolved
apps/meteor/client/views/room/ShareLocation/ShareLocationModal.tsx
Outdated
Show resolved
Hide resolved
julio-rocketchat
added
the
do not merge
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
packages/i18n/src/locales/en.i18n.json (3)
3254-3256: Improve map preview alt text for accessibility.Alt text should convey purpose and context. Consider parameterizing to include a label (e.g., place name) and keep an explicit fallback when it’s unavailable.
- "Map_Preview_Alt": "Map preview", - "Map_Preview_Unavailable": "Map preview unavailable", + "Map_Preview_Alt": "Map preview of {{label}}", + "Map_Preview_Unavailable": "Map preview unavailable"If a label isn’t available, pass a generic like “shared location”. Based on learnings.
3956-3956: Now I need to search for wheremapProvider.tsis and how it's actually used, and where the i18n translationOSM_Attributionis actually consumed.Based on my verification through web searches and code examination:
Make provider/ODbL compliance flexible through template-based attribution
OpenMapTiles requires attribution "© MapTiler © OpenStreetMap contributors", while the current implementation only shows "© OpenStreetMap contributors". The suggested template approach with
{{providerAttribution}}placeholder allows supporting multiple tile providers and their specific ODbL requirements.The review comment identifies a valid compliance concern. The current hardcoded attribution in
MapLibreMap.tsxline 35 would need to be updated alongside the i18n changes to fully implement this suggestion—replacing the static attribution string with a configurable template that accepts provider-specific values.The suggested changes to the i18n file align with OpenMapTiles attribution requirements and would enable flexibility for future provider changes or upgrades to tile services that have additional licensing obligations.
2376-2376: Microcopy polish—optional UX improvements recommended.The i18n keys are properly wired and the error recovery flow (Unable_To_Fetch_Current_Location + Try_again + Tips_Improve_Location_Accuracy_*) is already implemented in ShareLocationModal.tsx (lines 162–173). These suggested changes follow standard UX patterns:
Getting_Your_Location: Add ellipsis for progress states.Live_Location_Disabled_Body: Use contraction for tone consistency.Share_Location_Choice_Description: Simplify phrasing for clarity.- "Getting_Your_Location": "Getting your location", + "Getting_Your_Location": "Getting your location…", - "Live_Location_Disabled_Body": "Live location sharing is not available in this build", + "Live_Location_Disabled_Body": "Live location sharing isn't available in this build", - "Share_Location_Choice_Description": "Choose to share your current location once or start live location sharing" + "Share_Location_Choice_Description": "Share your current location once, or start live location sharing"
📜 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 (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (2)
apps/meteor/package.json(1 hunks)packages/i18n/src/locales/en.i18n.json(10 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/meteor/package.json
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: yiweigao0226
PR: RocketChat/Rocket.Chat#36746
File: apps/meteor/client/views/room/ShareLocation/ShareLocationModal.tsx:0-0
Timestamp: 2025-10-24T08:37:21.101Z
Learning: The location sharing feature is migrating from Google Maps and LocationIQ to MapLibre GL JS to eliminate hardcoded API keys in client code, addressing security concerns about key exposure in the client bundle and message attachments.
Learnt from: yiweigao0226
PR: RocketChat/Rocket.Chat#36746
File: apps/meteor/client/views/room/ShareLocation/LiveLocationModal.tsx:0-0
Timestamp: 2025-10-24T08:36:09.247Z
Learning: The location sharing feature in apps/meteor/client/views/room/ShareLocation/ uses MapLibre GL JS for map rendering instead of Google Maps or LocationIQ, eliminating the need for API keys in client code.
📚 Learning: 2025-10-24T08:37:21.101Z
Learnt from: yiweigao0226
PR: RocketChat/Rocket.Chat#36746
File: apps/meteor/client/views/room/ShareLocation/ShareLocationModal.tsx:0-0
Timestamp: 2025-10-24T08:37:21.101Z
Learning: The location sharing feature is migrating from Google Maps and LocationIQ to MapLibre GL JS to eliminate hardcoded API keys in client code, addressing security concerns about key exposure in the client bundle and message attachments.
Applied to files:
packages/i18n/src/locales/en.i18n.json
| "Current_Chats": "Current Chats", | ||
| "Current_File": "Current File", | ||
| "Current_Import_Operation": "Current Import Operation", | ||
| "Current_Location": "Current Location", |
There was a problem hiding this comment.
🧩 Analysis chain
Avoid duplicate semantics: consider reusing “My location” or “Location”.
You added Current_Location, but similar keys exist: My_location and Location. To keep copy consistent and reduce churn across locales, prefer one canonical label (e.g., reuse My_location or Location) unless the UX needs a distinct string. If distinct, clarify the nuance in the key name (e.g., Location_Current_Label).
Also applies to: 3573-3573, 3174-3174
🏁 Script executed:
#!/bin/bash
# Find usages of potentially overlapping keys to assess consolidation impact
rg -n --glob '!**/node_modules/**' -C2 '"Current_Location"|"My_location"|"Location"' packages appsLength of output: 36320
🏁 Script executed:
#!/bin/bash
# Check if Current_Location exists in any other locale file
rg -l 'Current_Location' packages/i18n/src/locales/Length of output: 38
🏁 Script executed:
#!/bin/bash
# Search for usage of these keys in TypeScript/JavaScript to understand context
rg -n --type=ts --type=tsx --type=js --type=jsx 'Current_Location|My_location|Location' packages apps --max-count=5 -g '!**/node_modules/**' | head -50Length of output: 27
🏁 Script executed:
#!/bin/bash
# Simplified search for Current_Location usage in code (no type flags)
rg -n 'Current_Location' packages apps -g '!**/node_modules/**' -g '!*.json'Length of output: 863
🏁 Script executed:
#!/bin/bash
# Also check if My_location is used in code for comparison
rg -n 'My_location' packages apps -g '!**/node_modules/**' -g '!*.json' | head -20Length of output: 0
Add Current_Location translations to all locale files to complete i18n consistency.
Current_Location is used in the share location feature (ShareLocationModal) but exists only in en.i18n.json. Add this key to all ~50 other locale files (following the same line patterns as My_location and other location-related strings) to avoid breaking the localization setup. The original concern about duplication with My_location is not applicable—My_location is unused in the codebase, and Current_Location serves a distinct feature context.
Affected locales: az, fa, zh-TW, zh, vi-VN, zh-HK, bg, uk, tr, ta-IN, sv, sq, sl-SI, sk-SK, sr, ro, ru, pt, pt-BR, pl, ms-MY, nn, mn, lt, ku, lo, lv, km, ko, nb, it, id, ka-GE, hr, nl, he, fr, eo, es, fi, el, de-AT, de-IN, da, cy, hu, cs, ja, af, ar, ca, de, be-BY, bs, th-TH, hi-IN (and any others in the locales directory).
🤖 Prompt for AI Agents
In packages/i18n/src/locales/en.i18n.json around line 1505, the
"Current_Location" key exists only in English; add the "Current_Location" key to
every other locale file listed (az, fa, zh-TW, zh, vi-VN, zh-HK, bg, uk, tr,
ta-IN, sv, sq, sl-SI, sk-SK, sr, ro, ru, pt, pt-BR, pl, ms-MY, nn, mn, lt, ku,
lo, lv, km, ko, nb, it, id, ka-GE, hr, nl, he, fr, eo, es, fi, el, de-AT, de-IN,
da, cy, hu, cs, ja, af, ar, ca, de, be-BY, bs, th-TH, hi-IN, and any others in
packages/i18n/src/locales), placing it near other location-related keys (e.g.,
My_location) and ensuring valid JSON (commas and ordering). For each locale, add
"Current_Location": "<localized string>" using the appropriate translation if
available, otherwise fallback to the English "Current Location"; save and run a
JSON lint to confirm no syntax errors.
There was a problem hiding this comment.
Is there any files that I can run to update all ~50 other locale files?
There was a problem hiding this comment.
@yiweigao0226 Short answer: you don’t need to touch all ~50 locale files for this PR. The i18n layer falls back to English if a key is missing in a locale, so adding it to en.i18n.json is sufficient and translators will pick it up later. See the Internationalization guide note about English fallback. (developer.rocket.chat)
If you still want to backfill a placeholder into every locale now (so they all explicitly contain Current_Location), here’s a small helper you can run from the repo root. It reads the English string and injects it into each locale file only if the key is missing.
#!/usr/bin/env bash
set -euo pipefail
BASE="packages/i18n/src/locales"
KEY="Current_Location"
# Grab the English value (e.g., "Current Location") from en.i18n.json
VAL="$(jq -r --arg k "$KEY" '.[$k]' "$BASE/en.i18n.json")"
if [[ -z "$VAL" || "$VAL" == "null" ]]; then
echo "Error: key \"$KEY\" not found in $BASE/en.i18n.json" >&2
exit 1
fi
# Iterate over all locale files except English
find "$BASE" -name '*.i18n.json' ! -name 'en.i18n.json' | while read -r f; do
if jq -e --arg k "$KEY" 'has($k)' "$f" > /dev/null; then
echo "ok - $(basename "$f") already has $KEY"
else
tmp="${f}.tmp"
# Add the missing key with the English fallback value
jq --arg k "$KEY" --arg v "$VAL" '. + {($k): $v}' "$f" > "$tmp" && mv "$tmp" "$f"
echo "add - $(basename "$f") -> $KEY"
fi
done
# Optional: verify JSON remains valid
echo
echo "Verifying JSON syntax..."
find "$BASE" -name '*.i18n.json' -print0 | xargs -0 -n1 jq -e >/dev/null && echo "All good ✅"Notes:
- This is safe to run repeatedly; it only adds the key when missing.
- If you prefer not to backfill, doing nothing is fine—the UI will show the English string until translations land via the normal localization workflow.
🧠 Learnings used
Learnt from: yiweigao0226
Repo: RocketChat/Rocket.Chat PR: 36746
File: apps/meteor/client/views/room/ShareLocation/ShareLocationModal.tsx:0-0
Timestamp: 2025-10-24T08:37:21.101Z
Learning: The location sharing feature is migrating from Google Maps and LocationIQ to MapLibre GL JS to eliminate hardcoded API keys in client code, addressing security concerns about key exposure in the client bundle and message attachments.
Learnt from: yiweigao0226
Repo: RocketChat/Rocket.Chat PR: 36746
File: apps/meteor/client/views/room/ShareLocation/LiveLocationModal.tsx:0-0
Timestamp: 2025-10-24T08:36:09.247Z
Learning: The location sharing feature in apps/meteor/client/views/room/ShareLocation/ uses MapLibre GL JS for map rendering instead of Google Maps or LocationIQ, eliminating the need for API keys in client code.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
apps/meteor/app/api/server/v1/liveLocation.ts (1)
245-249: Major: Throttle check has TOCTOU race - move predicate to arrayFilters.The throttle enforcement is vulnerable to time-of-check-to-time-of-use races:
- Two concurrent updates read
msgat lines 229-243- Both pass the
lastUpdateAtcheck at lines 245-249- Both execute
updateOneat lines 252-268- Result: multiple updates within the 3-second window
Fix by moving the throttle check into the arrayFilters predicate:
-const last: Date | undefined = (msg.attachments?.[0] as ILiveLocationAttachment)?.live?.lastUpdateAt; -const now = new Date(); -if (last && now.getTime() - new Date(last).getTime() < MIN_INTERVAL_MS) { - return API.v1.success({ ignored: true, reason: 'too-soon' }); -} - -const updateTime = new Date(); +const now = new Date(); +const threshold = new Date(now.getTime() - MIN_INTERVAL_MS); + const res = await Messages.updateOne( { _id: msgId, rid, 'u._id': uid }, { $set: { 'attachments.$[liveAtt].live.coords': coords, - 'attachments.$[liveAtt].live.lastUpdateAt': updateTime, + 'attachments.$[liveAtt].live.lastUpdateAt': now, }, }, { arrayFilters: [ { 'liveAtt.type': 'live-location', 'liveAtt.live.isActive': true, + $or: [ + { 'liveAtt.live.lastUpdateAt': { $lt: threshold } }, + { 'liveAtt.live.lastUpdateAt': { $exists: false } }, + ], }, ], } as any, ); +if (res.modifiedCount === 0) { + return API.v1.success({ ignored: true, reason: 'too-soon-or-not-active' }); +} + -if (res.modifiedCount > 0) { +{ const updatedMsg = await Messages.findOneById(msgId); if (updatedMsg) { void notifyOnMessageChange({This makes the throttle check atomic at the database level.
packages/i18n/src/locales/en.i18n.json (1)
1505-1505: Ensure locale coverage for new location/OSM strings and avoid duplicate semantics.
- Add these keys to all locale files to prevent fallback/missing-string issues.
- Consider consolidating “Current_Location” vs existing “My_location” to a single canonical label if UX doesn’t require both.
Based on learnings.
Run to verify presence across locales and spot unused duplicates:
#!/bin/bash # Check all locales contain the new keys keys='Current_Location|Getting_Your_Location|Live_Location|Live_Location_Disabled_Body|Map_Preview_Alt|Map_Preview_Unavailable|OSM_Attribution|Share_Location_Choice_Description|Tips_Improve_Location_Accuracy_Title|Tips_Improve_Location_Accuracy_Bullet_1|Tips_Improve_Location_Accuracy_Bullet_2|Tips_Improve_Location_Accuracy_Bullet_3|Tips_Improve_Location_Accuracy_Bullet_4|Try_again|Unable_To_Fetch_Current_Location|Using_OpenStreetMap_Label|View_on_OpenStreetMap' rg -nP "^(?:\s*)\"(?:$keys)\"\s*:" packages/i18n/src/locales/*.json | awk -F: '{print $1}' | sort | uniq -c | sort -nr echo echo "Potential duplicate semantics:" rg -nP '"(My_location|Current_Location)"\s*:' packages/apps apps packages -g '!**/node_modules/**' -g '!*.json' -C2Also applies to: 2376-2376, 3158-3160, 3254-3256, 3956-3956, 4818-4818, 5251-5255, 5333-5333, 5386-5386, 5640-5640, 5694-5694
🧹 Nitpick comments (8)
apps/meteor/app/api/server/v1/liveLocation.ts (1)
289-289: Optional: Use consistent route context type.For consistency with the other endpoints, consider typing the context:
- async post(this: any) { + async post(this: IAPIRouteContext) {apps/meteor/client/views/room/ShareLocation/MapLibreMap.tsx (1)
97-115: Inconsistent indentation in visibility resize effect.Lines 98-115 use spaces for indentation while the rest of the file uses tabs. This appears to be a formatting inconsistency.
Ensure consistent tab-based indentation throughout the effect:
useEffect(() => { - const map = mapRef.current; - if (!map || !visible) return; + const map = mapRef.current; + if (!map || !visible) return; - let r1 = 0; - let r2 = 0; + let r1 = 0; + let r2 = 0; - // wait for visibility/layout to settle, then resize - r1 = requestAnimationFrame(() => { - r2 = requestAnimationFrame(() => { - map.resize(); - }); - }); + // wait for visibility/layout to settle, then resize + r1 = requestAnimationFrame(() => { + r2 = requestAnimationFrame(() => { + map.resize(); + }); + }); - return () => { - if (r1) cancelAnimationFrame(r1); - if (r2) cancelAnimationFrame(r2); - }; - }, [visible]); + return () => { + if (r1) cancelAnimationFrame(r1); + if (r2) cancelAnimationFrame(r2); + }; + }, [visible]);packages/i18n/src/locales/en.i18n.json (3)
5333-5333: Reuse existing “Retry” instead of introducing “Try_again”.Avoid near-duplicate keys for the same CTA.
- "Try_again": "Try again",Update call sites to use the existing "Retry".
3956-3956: OSM attribution: include required copyright link(s).Text is fine, but ensure the UI wraps it with a link to OpenStreetMap copyright (and license, if shown) when rendered.
If UI supports i18n with markup (e.g., <1> tags), consider:
- Keep OSM_Attribution as plain text for minimal UIs.
- Optionally add OSM_Attribution_HTML and use it where links are allowed.
Also applies to: 5694-5694
3254-3256: Slightly clearer alt text for screen readers (optional).“Map preview” can be more descriptive.
- "Map_Preview_Alt": "Map preview", + "Map_Preview_Alt": "Map preview of location", "Map_Preview_Unavailable": "Map preview unavailable",apps/meteor/client/views/room/ShareLocation/ShareLocationModal.tsx (2)
65-74: Consider logging or notifying on geolocation errors.The catch block at Line 70 silently swallows all errors from
getGeolocationPosition(). While the subsequent permission state update may handle denied cases in the UI, other errors (e.g., timeout, unavailable API) won't provide user feedback.Consider adding minimal error logging or a toast notification:
} catch (e) { + console.warn('Geolocation request failed:', e); const state = await getGeolocationPermission(); queryClient.setQueryData(['geolocationPermission'], state); }
220-220: State machine fallback could use null for clarity.The empty fragment
<></>at Line 220 serves as a fallback for the state machine. While the state transitions appear to cover all cases (choose/live/static paths), returningnullwould be more idiomatic for a "render nothing" fallback in React.- return <></>; + return null;apps/meteor/client/views/room/ShareLocation/getGeolocationPosition.ts (1)
87-101: Consider logging the swallowed transient-retry error.Lines 97-100 silently discard the error from the transient retry attempt with
void e;. While this allows the flow to proceed to the final high-accuracy attempt (which is reasonable), it makes debugging harder if the transient retry consistently fails.Add minimal logging to aid troubleshooting:
} catch (e) { // ignore transient failure; a final high-accuracy attempt will follow - void e; + console.debug('Transient retry failed, proceeding to high-accuracy attempt:', e); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
apps/meteor/app/api/server/v1/liveLocation.ts(1 hunks)apps/meteor/client/views/room/ShareLocation/MapLibreMap.tsx(1 hunks)apps/meteor/client/views/room/ShareLocation/ShareLocationModal.tsx(1 hunks)apps/meteor/client/views/room/ShareLocation/getGeolocationPosition.ts(1 hunks)apps/meteor/client/views/room/ShareLocation/useBrowserLiveLocation.spec.tsx(1 hunks)packages/i18n/src/locales/en.i18n.json(11 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/meteor/client/views/room/ShareLocation/useBrowserLiveLocation.spec.tsx
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: yiweigao0226
PR: RocketChat/Rocket.Chat#36746
File: apps/meteor/client/views/room/ShareLocation/ShareLocationModal.tsx:0-0
Timestamp: 2025-10-24T08:37:21.101Z
Learning: The location sharing feature is migrating from Google Maps and LocationIQ to MapLibre GL JS to eliminate hardcoded API keys in client code, addressing security concerns about key exposure in the client bundle and message attachments.
Learnt from: yiweigao0226
PR: RocketChat/Rocket.Chat#36746
File: apps/meteor/client/views/room/ShareLocation/LiveLocationModal.tsx:0-0
Timestamp: 2025-10-24T08:36:09.247Z
Learning: The location sharing feature in apps/meteor/client/views/room/ShareLocation/ uses MapLibre GL JS for map rendering instead of Google Maps or LocationIQ, eliminating the need for API keys in client code.
📚 Learning: 2025-10-24T08:36:09.247Z
Learnt from: yiweigao0226
PR: RocketChat/Rocket.Chat#36746
File: apps/meteor/client/views/room/ShareLocation/LiveLocationModal.tsx:0-0
Timestamp: 2025-10-24T08:36:09.247Z
Learning: The location sharing feature in apps/meteor/client/views/room/ShareLocation/ uses MapLibre GL JS for map rendering instead of Google Maps or LocationIQ, eliminating the need for API keys in client code.
Applied to files:
apps/meteor/client/views/room/ShareLocation/ShareLocationModal.tsxapps/meteor/app/api/server/v1/liveLocation.tsapps/meteor/client/views/room/ShareLocation/MapLibreMap.tsxapps/meteor/client/views/room/ShareLocation/getGeolocationPosition.ts
📚 Learning: 2025-10-24T08:37:21.101Z
Learnt from: yiweigao0226
PR: RocketChat/Rocket.Chat#36746
File: apps/meteor/client/views/room/ShareLocation/ShareLocationModal.tsx:0-0
Timestamp: 2025-10-24T08:37:21.101Z
Learning: The location sharing feature is migrating from Google Maps and LocationIQ to MapLibre GL JS to eliminate hardcoded API keys in client code, addressing security concerns about key exposure in the client bundle and message attachments.
Applied to files:
apps/meteor/client/views/room/ShareLocation/ShareLocationModal.tsxapps/meteor/app/api/server/v1/liveLocation.tsapps/meteor/client/views/room/ShareLocation/MapLibreMap.tsxpackages/i18n/src/locales/en.i18n.jsonapps/meteor/client/views/room/ShareLocation/getGeolocationPosition.ts
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
PR: RocketChat/Rocket.Chat#0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.{ts,tsx} : Avoid using page.locator(); prefer semantic locators like page.getByRole, page.getByLabel, page.getByText, and page.getByTitle
Applied to files:
apps/meteor/client/views/room/ShareLocation/ShareLocationModal.tsx
📚 Learning: 2025-09-23T19:22:59.217Z
Learnt from: dougfabris
PR: RocketChat/Rocket.Chat#36987
File: apps/meteor/tests/e2e/page-objects/fragments/room-toolbar.ts:10-20
Timestamp: 2025-09-23T19:22:59.217Z
Learning: In Playwright e2e tests, prefer stable selectors like data-qa-id attributes over localized text in getByRole() or getByText() calls to prevent test failures when UI language changes. Test translations separately by validating actual text content after ensuring UI interactions work with stable selectors.
Applied to files:
apps/meteor/client/views/room/ShareLocation/ShareLocationModal.tsx
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
PR: RocketChat/Rocket.Chat#0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Ensure tests run reliably in parallel without shared state conflicts
Applied to files:
apps/meteor/app/api/server/v1/liveLocation.ts
🧬 Code graph analysis (2)
apps/meteor/client/views/room/ShareLocation/ShareLocationModal.tsx (3)
apps/meteor/client/views/room/ShareLocation/mapProvider.ts (2)
IMapProvider(9-14)createMapProvider(46-52)apps/meteor/client/views/room/ShareLocation/getGeolocationPosition.ts (1)
getGeolocationPosition(45-114)apps/meteor/client/views/room/ShareLocation/MapLibreMap.tsx (1)
MapLibreMap(16-168)
apps/meteor/app/api/server/v1/liveLocation.ts (3)
packages/core-typings/src/IUser.ts (1)
IUser(186-255)packages/core-typings/src/IRoom.ts (1)
IRoom(21-95)apps/meteor/app/lib/server/lib/notifyListener.ts (1)
notifyOnMessageChange(507-513)
🪛 Biome (2.1.2)
apps/meteor/client/views/room/ShareLocation/MapLibreMap.tsx
[error] 85-87: This code will never be reached ...
... because this statement will return from the function beforehand
(lint/correctness/noUnreachable)
🔇 Additional comments (8)
apps/meteor/app/api/server/v1/liveLocation.ts (2)
285-367: LGTM - stop endpoint properly uses arrayFilters.The stop endpoint correctly addresses previous review concerns by:
- Using
arrayFiltersto target the specific live-location attachment (lines 337-343)- Including
ridandu._idin the selector (lines 318-322)- Making the update conditional on
isActive: true(line 340)- Properly handling the optional
finalCoordsparameter (lines 329-331)This prevents updating the wrong attachment and closes TOCTOU gaps.
369-434: LGTM - get endpoint looks solid.The retrieval endpoint properly:
- Validates all required parameters and authentication
- Enforces room access and membership checks
- Safely finds the live-location attachment using type matching
- Returns a comprehensive payload with all relevant fields
apps/meteor/client/views/room/ShareLocation/MapLibreMap.tsx (1)
117-141: LGTM!The live coordinates effect correctly updates the marker position and maintains a bounded trail with a maximum of 1000 coordinates, preventing memory growth issues. The GeoJSON update logic is clean and efficient.
apps/meteor/client/views/room/ShareLocation/getGeolocationPosition.ts (5)
3-3: LGTM: AnyErr type extension is appropriate.The
AnyErrtype extendsGeolocationPositionErrorwith an optionalmessageproperty to handle variations in browser implementations. This is a pragmatic approach to type safety for error handling.
11-16: LGTM: Safe migration cleanup for old sessionStorage.The try-catch wrapped
sessionStorage.removeItemat Lines 11-16 safely migrates away from the old sessionStorage-based caching (which had PII concerns). The catch block correctly handles environments where sessionStorage is unavailable (SSR, privacy modes, etc.).
22-43: LGTM: Coarse position with preserved timestamp.The
toCoarsePositionfunction correctly:
- Quantizes coordinates to 3 decimal places (~111m precision) for privacy
- Preserves the original
src.timestamp(Line 34) to maintain measurement fidelity- Includes a
toJSONmethod (Lines 35-41) to ensure proper serializationThis addresses previous PII and serialization concerns.
45-114: LGTM: Well-structured multi-stage geolocation strategy.The staged fallback approach is resilient:
- Stage 0: Serves fresh in-memory cache (30s validity)
- Stage 1: Quick low-accuracy read (8s timeout, lets CoreLocation settle)
- Stage 2: Transient fallback via
watchPositionwith additional retry- Stage 3: Final high-accuracy attempt
The flow correctly bails on
PERMISSION_DENIEDand handles Apple's transientkCLErrorLocationUnknownerrors.
116-144: LGTM: watchOnce cleanup logic is correct.The
watchOncehelper properly cleans up both the watch ID and timeout in all code paths (success, error, and timeout). The timeout fallback at Line 128 (options.timeout ?? 20000) is defensive coding, even though callers already specify timeout.
| return () => { | ||
| resizeObsRef.current?.disconnect(); | ||
| window.removeEventListener('resize', onWinResize); | ||
| map.remove(); | ||
| mapRef.current = null; | ||
| markerRef.current = null; | ||
| trailRef.current = null; | ||
| trailCoordsRef.current = []; | ||
| }; | ||
| map.on('error', (e) => { | ||
| console.error('MapLibre error:', e); | ||
| }); |
There was a problem hiding this comment.
Critical: Error handler is unreachable after return statement.
Lines 85-87 register the map.on('error', ...) handler after the cleanup function's return statement, making this code unreachable. Error handling is currently non-functional.
Apply this diff to move the error handler before the cleanup return:
markerRef.current = new maplibregl.Marker({ color: '#1976d2' }).setLngLat([lon, lat]).addTo(map);
+ map.on('error', (e) => {
+ console.error('MapLibre error:', e);
+ });
+
map.on('load', () => {Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Biome (2.1.2)
[error] 85-87: This code will never be reached ...
... because this statement will return from the function beforehand
(lint/correctness/noUnreachable)
🤖 Prompt for AI Agents
In apps/meteor/client/views/room/ShareLocation/MapLibreMap.tsx around lines 76
to 87, the map.on('error', ...) handler is placed after the function's return
making it unreachable; move the map.on('error', (e) => ...) call so it is
registered before the cleanup return and, inside the returned cleanup function,
remove that listener (e.g., map.off('error', handler)) along with the existing
cleanup steps to ensure the error handler is active during the component
lifecycle and properly removed on unmount.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the current state of MapLibreMap.tsx to verify the fix
cat apps/meteor/client/views/room/ShareLocation/MapLibreMap.tsx
Length of output: 4258
@yiweigao0226 Perfect! I've verified the fix and can confirm that the error handler is now properly positioned. The map.on('error', ...) handler has been moved to lines 52-54, before the return statement, making it reachable and functional during the component's lifecycle.
The error handler will now correctly catch and log any MapLibre errors. The cleanup is also handled appropriately since map.remove() tears down the map instance and removes all associated event listeners.
Great work fixing this critical issue! 🎉
🧠 Learnings used
Learnt from: yiweigao0226
PR: RocketChat/Rocket.Chat#36746
File: apps/meteor/client/views/room/ShareLocation/LiveLocationModal.tsx:0-0
Timestamp: 2025-10-24T08:36:09.247Z
Learning: The location sharing feature in apps/meteor/client/views/room/ShareLocation/ uses MapLibre GL JS for map rendering instead of Google Maps or LocationIQ, eliminating the need for API keys in client code.
Learnt from: yiweigao0226
PR: RocketChat/Rocket.Chat#36746
File: apps/meteor/client/views/room/ShareLocation/ShareLocationModal.tsx:0-0
Timestamp: 2025-10-24T08:37:21.101Z
Learning: The location sharing feature is migrating from Google Maps and LocationIQ to MapLibre GL JS to eliminate hardcoded API keys in client code, addressing security concerns about key exposure in the client bundle and message attachments.
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
For the location service agnostic feature, due to time constraints, I’ve included only the default OSM provider for now. However, I plan to add a settings preference later that will allow users to select their preferred map provider. The settings functionality has already been implemented on the mobile side. |
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 ignored due to path filters (1)
packages/apps-engine/deno-runtime/deno.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
apps/meteor/app/api/server/v1/liveLocation.ts(1 hunks)apps/meteor/client/views/room/ShareLocation/MapLibreMap.tsx(1 hunks)apps/meteor/client/views/room/ShareLocation/useBrowserLiveLocation.spec.tsx(1 hunks)packages/models/src/models/Messages.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/meteor/client/views/room/ShareLocation/useBrowserLiveLocation.spec.tsx
- apps/meteor/client/views/room/ShareLocation/MapLibreMap.tsx
- apps/meteor/app/api/server/v1/liveLocation.ts
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: yiweigao0226
PR: RocketChat/Rocket.Chat#36746
File: apps/meteor/client/views/room/ShareLocation/ShareLocationModal.tsx:0-0
Timestamp: 2025-10-24T08:37:21.101Z
Learning: The location sharing feature is migrating from Google Maps and LocationIQ to MapLibre GL JS to eliminate hardcoded API keys in client code, addressing security concerns about key exposure in the client bundle and message attachments.
Learnt from: yiweigao0226
PR: RocketChat/Rocket.Chat#36746
File: apps/meteor/client/views/room/ShareLocation/LiveLocationModal.tsx:0-0
Timestamp: 2025-10-24T08:36:09.247Z
Learning: The location sharing feature in apps/meteor/client/views/room/ShareLocation/ uses MapLibre GL JS for map rendering instead of Google Maps or LocationIQ, eliminating the need for API keys in client code.
| key: { 'attachments.0.live.expiresAt': 1 }, | ||
| expireAfterSeconds: 0, | ||
| partialFilterExpression: { | ||
| 'attachments.0.type': 'live-location', | ||
| 'attachments.0.live.expiresAt': { $type: 'date' }, | ||
| }, | ||
| name: 'liveLocation_expiresAt_TTL', | ||
| }, |
There was a problem hiding this comment.
TTL index only expires live locations if they stay at attachments[0]
The TTL relies on 'attachments.0.live.expiresAt', but any live-location message that gains a different attachment ahead of it (or simply stores the live attachment past index 0) will no longer match the partial filter. Those documents never enter the TTL index, so their real-time location data will persist indefinitely—exactly the privacy risk this cleanup was meant to prevent. MongoDB TTL indexes can safely target array subdocuments directly; they use the earliest date in the array when expiring entries.(mongodb.com) Please anchor the TTL to the $elemMatch element instead of the first array slot, for example:
- {
- key: { 'attachments.0.live.expiresAt': 1 },
- expireAfterSeconds: 0,
- partialFilterExpression: {
- 'attachments.0.type': 'live-location',
- 'attachments.0.live.expiresAt': { $type: 'date' },
- },
- name: 'liveLocation_expiresAt_TTL',
- },
+ {
+ key: { 'attachments.live.expiresAt': 1 },
+ expireAfterSeconds: 0,
+ partialFilterExpression: {
+ attachments: {
+ $elemMatch: {
+ type: 'live-location',
+ 'live.expiresAt': { $type: 'date' },
+ },
+ },
+ },
+ name: 'liveLocation_expiresAt_TTL',
+ },That way the TTL remains effective regardless of attachment ordering, and the cleanup guarantees stay intact.
🤖 Prompt for AI Agents
In packages/models/src/models/Messages.ts around lines 83–90, the TTL index
currently targets 'attachments.0.live.expiresAt' so only documents where the
live attachment is at index 0 are indexed; change the index to target the array
element instead and use a partialFilterExpression with $elemMatch so any
attachment with type 'live-location' and a date expiresAt is included.
Specifically, set the key to the array path for the nested date (e.g.
'attachments.live.expiresAt'), keep expireAfterSeconds: 0, and replace the
partialFilterExpression with an $elemMatch on attachments: { $elemMatch: { type:
'live-location', 'live.expiresAt': { $type: 'date' } } } (and optionally update
the index name) so TTL applies regardless of attachment ordering.
There was a problem hiding this comment.
Oops, something went wrong! Please try again later. 🐰 💔
I understand. We won't be able to approve it - from a security perspective - before these changes are implemented. |
julio-rocketchat
left a comment
julio-rocketchat
left a comment
There was a problem hiding this comment.
(Just adding this comment to request changes and note that to be merged into develop there are security and privacy concerns that need to be addressed first. The PR can still be merged into a feature branch if you feel like it, but the security team is not comfortable in approving the PR as is and letting it be merge into the develp branch)
6 participants
Proposed changes (including videos or screenshots)
Issue(s)
Steps to test or reproduce
Further comments
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Chores