-
Notifications
You must be signed in to change notification settings - Fork 5
Develop #80
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughRemoved client Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant PTTInterface as PTT Interface
participant usePTT as usePTT Hook
participant LiveKitStore as LiveKit Store
participant Platform as Platform Services
User->>PTTInterface: Select channel / Connect
PTTInterface->>usePTT: connect(channel)
activate usePTT
usePTT->>LiveKitStore: connectToRoom(selectedChannel)
activate LiveKitStore
LiveKitStore->>Platform: start foreground/session (Android/iOS)
Platform-->>LiveKitStore: service started
LiveKitStore-->>usePTT: connection established
deactivate LiveKitStore
usePTT-->>PTTInterface: isConnected, currentChannel
deactivate usePTT
User->>PTTInterface: Press PTT
PTTInterface->>usePTT: startTransmitting()
usePTT->>LiveKitStore: enable mic / publish audio
usePTT-->>PTTInterface: isTransmitting = true
User->>PTTInterface: Release PTT
PTTInterface->>usePTT: stopTransmitting()
usePTT->>LiveKitStore: disable mic / stop publish
usePTT-->>PTTInterface: isTransmitting = false
User->>PTTInterface: Disconnect
PTTInterface->>usePTT: disconnect()
usePTT->>LiveKitStore: disconnectFromRoom()
LiveKitStore->>Platform: stop foreground/service
LiveKitStore-->>usePTT: disconnected
usePTT-->>PTTInterface: isConnected = false
sequenceDiagram
participant User
participant NotesPanel as Notes Panel
participant AddNoteSheet as AddNoteBottomSheet
participant API as Note API
participant ActivityLog as Activity Log
User->>NotesPanel: Click "Add Note"
NotesPanel->>AddNoteSheet: open (isOpen = true)
activate AddNoteSheet
AddNoteSheet->>API: getNoteCategories()
API-->>AddNoteSheet: categories
AddNoteSheet->>User: show form
User->>AddNoteSheet: Submit
AddNoteSheet->>API: saveNote(data)
API-->>AddNoteSheet: saved note
AddNoteSheet->>ActivityLog: log note created
AddNoteSheet-->>NotesPanel: onNoteAdded()
NotesPanel->>API: refresh notes
AddNoteSheet->>User: close sheet
deactivate AddNoteSheet
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In `@package.json`:
- Line 12: The "web" npm script currently omits the EXPO_NO_DOTENV=1 prefix,
causing inconsistent dotenv behavior versus other scripts; update the "web"
script (named "web") to include EXPO_NO_DOTENV=1 like the other scripts, or if
the difference is intentional, add an inline comment near the "web" script
explaining why it should load .env (or not) to make the intent explicit and keep
behavior consistent across "start", "prebuild", "android", "ios", build scripts
and "web".
In `@src/app/`(app)/home.tsx:
- Around line 192-201: The availability logic is inconsistent: availableUnits
treats a falsy u.CurrentStatus as available while availablePersonnel requires an
explicit status match; decide and align them—either (A) if missing/null status
should count as available, change the personnel filter (referencing personnel
and p.Status in availablePersonnel) to also treat falsy as available and add a
clarifying comment, or (B) if missing status should not count as available,
remove the !u.CurrentStatus branch from the units filter (referencing units and
u.CurrentStatus in availableUnits) so both filters only count explicit
'available' or 'standing by' statuses and add a brief comment documenting the
chosen semantics.
In `@src/components/dispatch-console/add-note-bottom-sheet.tsx`:
- Around line 85-112: The submit handler onFormSubmit currently only logs errors
but gives no user feedback; add a saveError state (e.g., via useState<string |
null>) and clear it when submission starts (in onFormSubmit before calling
saveNote), then set a user-friendly message into saveError inside the catch
block (using the caught error to enrich the message) and trigger a visible UI
feedback (either render the saveError inline in the Add Note bottom sheet or
call a toast/alert helper) so users see the failure; ensure you still reset
setIsLoading(false) in finally and clear saveError on successful save before
calling onNoteAdded and onClose.
In `@src/components/sidebar/side-menu.tsx`:
- Around line 2-15: The imported symbol Map from lucide-react-native shadows the
global Map constructor; change the import to an aliased name (e.g., import { Map
as MapIcon } from 'lucide-react-native') and then replace all usages of the Map
component in this file (e.g., JSX where <Map ...> is used) with the new alias
(<MapIcon ...>) so the global Map remains unshadowed and icon references still
work.
In `@src/hooks/use-ptt.ts`:
- Around line 231-276: The hook currently defines startAndroidForegroundService
and stopAndroidForegroundService which duplicate foreground service lifecycle
already managed by connectToRoom and disconnectFromRoom in livekit-store.ts;
remove the duplicate management by deleting calls to
startAndroidForegroundService and stopAndroidForegroundService from the hook's
connect/disconnect flow (or remove these two functions entirely from use-ptt.ts)
so only the livekit-store.ts implementations handle Android foreground service
start/stop, ensuring you keep connectToRoom and disconnectFromRoom as the single
source of truth.
In `@src/stores/app/livekit-store.ts`:
- Around line 209-241: The store currently duplicates Android foreground service
logic and defines an unnecessary inner function; consolidate foreground service
management in livekit-store.ts by removing the duplicate start/stop logic from
use-ptt.ts and exposing a single startForegroundService/stopForegroundService
API on the store (or a clearly named methods like startAndroidForegroundService
and stopAndroidForegroundService) that the hook calls; simplify the
implementation in livekit-store.ts by eliminating the nested
startForegroundService function and directly call
notifee.registerForegroundService and notifee.displayNotification with a single
consistent android.channelId (pick one: 'ptt-channel' or 'notif') and keep
logger.debug/error handling around registration and display. Ensure
connectToRoom() no longer starts a separate foreground service and that stop
logic in the store stops the notification/service to avoid duplicate
notifications.
🧹 Nitpick comments (20)
src/components/sidebar/__tests__/side-menu.test.tsx (2)
115-128: Duplicate test assertions.This test is named "should display menu items with lucide icons" but only verifies label text, making it identical to the "should render all menu items" test at lines 31-43. Consider either removing this test or adding actual icon verification (e.g., via
testIDon icon wrappers).
130-142: Theme tests only verify rendering, not styles.These tests confirm the component renders with each
colorSchemevalue but don't verify actual styling differences. This is acceptable as a smoke test given React Native styling is difficult to assert, but consider adding a comment noting the limitation.src/components/sidebar/side-menu.tsx (1)
92-95: Consider typing routes to avoidas any.The
as anyassertion bypasses expo-router's route type checking. If typed routes are configured, consider using the proper route type; otherwise, this is an acceptable workaround.src/app/onboarding.tsx (1)
24-40: LGTM! Consider extracting to translation files.The title update to "Resgrid Dispatch" aligns well with the app branding. However, note that these onboarding strings are hardcoded while the rest of the app uses i18n via
useTranslation(). Consider moving these to translation files for consistency, especially since the AI summary indicates translation updates are being made acrossen.json,es.json, andar.jsonin this PR.,
src/components/dispatch-console/active-calls-panel.tsx (2)
3-3: Unused import.
useColorSchemeis imported but not used in this component. Remove it to keep imports clean.🔧 Suggested fix
-import { useColorScheme } from 'nativewind';
152-168: Remove debug logging before merging.These
console.logstatements appear to be debug artifacts. Consider removing them or replacing with a conditional debug logger to avoid polluting production logs.🔧 Suggested fix
- // Log calls state for debugging - useEffect(() => { - console.log('[ActiveCallsPanel] Calls updated:', { - totalCalls: calls.length, - callStates: calls.map((c) => ({ id: c.CallId, name: c.Name, state: c.State })), - }); - }, [calls]); - const activeCalls = useMemo(() => { // Filter for active or open calls using utility function let filtered = calls.filter((c) => isCallActive(c.State)); - console.log('[ActiveCallsPanel] Active calls filtered:', { - total: calls.length, - active: filtered.length, - filteredCalls: filtered.map((c) => ({ id: c.CallId, name: c.Name, state: c.State })), - }); - // Apply search filtersrc/components/dispatch-console/units-panel.tsx (1)
2-2: Unused import:useColorSchemeThe
useColorSchemehook is imported but never used in this component. Remove to keep imports clean.🧹 Suggested fix
-import { useColorScheme } from 'nativewind';src/hooks/__tests__/use-ptt.test.ts (1)
465-489: Consider testing callback invocation in AppState changeThe AppState tests verify listener setup/cleanup, but don't test the actual callback behavior (e.g., logging when coming to foreground, setting
wasConnectedBeforeBackgroundwhen going to background). Consider adding a test that invokes the captured listener callback.💡 Example test for callback behavior
it('should track connection state when going to background', () => { let capturedCallback: ((state: AppStateStatus) => void) | null = null; const mockRemove = jest.fn(); const addEventListenerSpy = jest .spyOn(require('react-native').AppState, 'addEventListener') .mockImplementation((_, callback) => { capturedCallback = callback; return { remove: mockRemove }; }); mockUseLiveKitStore.mockReturnValue({ isConnected: true, // ... other required fields }); renderHook(() => usePTT()); // Simulate going to background capturedCallback?.('background'); // Can verify logging was called or internal state changed addEventListenerSpy.mockRestore(); });src/hooks/use-ptt.ts (1)
151-155: Suppressed exhaustive-deps warning should includefetchVoiceSettingsThe eslint-disable comment suppresses the warning for missing
fetchVoiceSettingsin the dependency array. SincefetchVoiceSettingscomes from a Zustand store, it's stable and won't change, so this is acceptable. Consider adding a comment explaining why this is safe.src/components/dispatch-console/activity-log-panel.tsx (1)
2-2: Unused import:useColorSchemeThe
useColorSchemehook is imported but never used in this component. Remove to keep imports clean.🧹 Suggested fix
-import { useColorScheme } from 'nativewind';src/components/dispatch-console/notes-panel.tsx (1)
2-2: Unused import.
useColorSchemeis imported but never used in this file.🧹 Remove unused import
-import { useColorScheme } from 'nativewind';src/lib/hooks/use-selected-theme.tsx (1)
36-38: Web platform branch logs but doesn't apply the dark theme.The log says "defaulting to dark mode" but no
colorScheme.set('dark')call follows. Meanwhile, the native path (lines 46-47) explicitly setscolorScheme.set('dark')when no theme is found.The web-specific file (
use-selected-theme.web.tsx) handles this by callingcolorScheme.set('dark')in its ownloadSelectedTheme. Since the web build will use.web.tsxvia bundler resolution, this native file's web branch is likely dead code—but the inconsistency is confusing.Consider removing the web early-return here entirely (it's unreachable on web builds) or adding the
colorScheme.set('dark')call to keep the code self-documenting.src/components/dispatch-console/__tests__/add-note-bottom-sheet.test.tsx (1)
193-227: Test coverage is minimal.The current tests verify basic rendering and category fetching. Consider adding tests for:
- Form submission success and error paths
- Close button behavior (especially when
isLoadingis true)- Form validation errors display
These can be added incrementally.
src/components/dispatch-console/add-note-bottom-sheet.tsx (2)
141-146: Redundant validation rule.
minLength: 1is redundant whenrequiredis already specified—requiredensures the field is non-empty.🧹 Simplify validation
rules={{ required: t('form.required'), - minLength: { - value: 1, - message: t('form.required'), - }, }}
199-204: Same redundant validation as the title field.🧹 Simplify validation
rules={{ required: t('form.required'), - minLength: { - value: 1, - message: t('form.required'), - }, }}src/components/dispatch-console/stats-header.tsx (1)
88-88: Minor: Redundant static backgroundColor in styles.The
dividerstyle at Line 107 sets a staticbackgroundColor: '#d1d5db', but it's always overridden inline at Line 88. Consider removing the static value or adding a comment for clarity.♻️ Optional cleanup
const styles = StyleSheet.create({ iconContainer: { width: 32, height: 32, borderRadius: 8, alignItems: 'center', justifyContent: 'center', }, divider: { width: 1, height: 24, - backgroundColor: '#d1d5db', + // backgroundColor set dynamically based on color scheme }, });Also applies to: 104-108
src/app/(app)/_layout.tsx (1)
399-408: Hardcoded string should use i18n translation.The "Close Menu" text at Line 404 is hardcoded in English. Use the translation function for consistency with the rest of the app.
♻️ Proposed fix
- <TouchableOpacity onPress={() => setIsOpen(false)} style={[layoutStyles.closeButton, webTheme.closeButton]}> - <RNText style={[layoutStyles.closeButtonText, webTheme.closeButtonText]}>Close Menu</RNText> + <TouchableOpacity onPress={() => setIsOpen(false)} style={[layoutStyles.closeButton, webTheme.closeButton]}> + <RNText style={[layoutStyles.closeButtonText, webTheme.closeButtonText]}>{t('common.close')}</RNText>src/components/dispatch-console/ptt-interface.tsx (1)
85-88: Remove duplicate voice settings initialization.The
usePTThook already callsrefreshVoiceSettings()on mount via its internaluseEffectwith an empty dependency array (line 153 inuse-ptt.ts). This additionaluseEffectin the component causes the same initialization to run twice, resulting in duplicate API calls.♻️ Proposed fix
- // Fetch voice settings on mount - useEffect(() => { - refreshVoiceSettings(); - }, [refreshVoiceSettings]);src/app/(app)/home.tsx (2)
291-299: Consider distinct translations for action and description.Both
actionanddescriptionfields in the activity log entry use similar translation keys (note_createdandnote_added), which may result in redundant or confusing log entries like "Note Created - Note Added".Suggested improvement
const handleNoteAdded = () => { fetchNotes(); addActivityLogEntry({ type: 'system', action: t('dispatch.note_created'), - description: t('dispatch.note_added'), + description: t('dispatch.new_note_saved_successfully'), }); };
604-607: Minor optimization: memoize containerStyle.The
containerStylearray is recreated on every render. WhileStyleSheet.flattenis cheap, memoizing this could avoid unnecessary allocations:Suggested optimization
- const containerStyle = colorScheme === 'dark' ? [styles.container, styles.containerDark] : [styles.container, styles.containerLight]; + const containerStyle = useMemo( + () => (colorScheme === 'dark' ? [styles.container, styles.containerDark] : [styles.container, styles.containerLight]), + [colorScheme] + );
| "android": "cross-env EXPO_NO_DOTENV=1 expo run:android", | ||
| "ios": "cross-env EXPO_NO_DOTENV=1 expo run:ios --device", | ||
| "web": "cross-env EXPO_NO_DOTENV=1 expo start --web", | ||
| "web": "cross-env expo start --web --clear", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Inconsistent EXPO_NO_DOTENV=1 usage across scripts.
The web script no longer sets EXPO_NO_DOTENV=1, while all other scripts (start, prebuild, android, ios, and build scripts on lines 8-11 and 28-35) consistently use it. This means .env files will be auto-loaded for web but not for other platforms, potentially causing different environment variable behavior between web and native development.
Was this intentional? If so, consider adding a comment explaining why web differs. Otherwise, restore consistency:
Suggested fix to restore consistency
- "web": "cross-env expo start --web --clear",
+ "web": "cross-env EXPO_NO_DOTENV=1 expo start --web --clear",📝 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.
| "web": "cross-env expo start --web --clear", | |
| "web": "cross-env EXPO_NO_DOTENV=1 expo start --web --clear", |
🤖 Prompt for AI Agents
In `@package.json` at line 12, The "web" npm script currently omits the
EXPO_NO_DOTENV=1 prefix, causing inconsistent dotenv behavior versus other
scripts; update the "web" script (named "web") to include EXPO_NO_DOTENV=1 like
the other scripts, or if the difference is intentional, add an inline comment
near the "web" script explaining why it should load .env (or not) to make the
intent explicit and keep behavior consistent across "start", "prebuild",
"android", "ios", build scripts and "web".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/stores/app/livekit-store.ts (1)
159-216: Stop the existing foreground service when reconnecting to avoid duplicates.
connectToRoom()disconnects an existing room but doesn’t stop the foreground service before starting a new one. This can leave multiple foreground notifications/services active when reconnecting.🩹 Proposed fix
- // Disconnect from current room if connected - if (currentRoom) { - currentRoom.disconnect(); - } + // Disconnect from current room if connected + if (currentRoom) { + currentRoom.disconnect(); + if (Platform.OS === 'android') { + await get().stopAndroidForegroundService(); + } + }
🤖 Fix all issues with AI agents
In `@src/components/dispatch-console/add-note-bottom-sheet.tsx`:
- Around line 62-67: When the sheet closes the useEffect that currently calls
reset() should also clear the save error state so previous errors don't persist;
update the effect that depends on isOpen and reset to also call the save error
setter (e.g., setSaveError(null) or clearSaveError()) when !isOpen, ensuring
both reset() and the saveError clear happen together in the same block.
In `@src/hooks/use-ptt.ts`:
- Around line 187-191: The CallKeep mute callback currently only calls
setIsMuted and doesn't change the LiveKit microphone; update the callback passed
to callKeepService.setMuteStateCallback to call setMutedInternal(muted) (or
callMutedHandler that syncs mic state) instead of just setIsMuted so the actual
microphone is enabled/disabled; keep any user-interaction sync logic but ensure
setMutedInternal is invoked inside the async callback (e.g., replace
setIsMuted(muted) with await setMutedInternal(muted) or call it directly) to
guarantee the LiveKit mic state matches the UI mute state.
In `@src/stores/app/livekit-store.ts`:
- Around line 89-90: handleLeaveRoom in LiveKitCallModal currently calls
actions.disconnectFromRoom() without awaiting it, causing a race where onClose()
runs before the disconnect completes; make handleLeaveRoom an async function,
await actions.disconnectFromRoom() and only call onClose() after the await so
the disconnect finishes before closing the modal (update the handleLeaveRoom
declaration and any callers to handle the async function accordingly,
referencing handleLeaveRoom, actions.disconnectFromRoom, and onClose).
- Around line 306-342: The startAndroidForegroundService function uses
notifee.displayNotification with channelId 'ptt-channel' but never creates that
channel; before calling notifee.displayNotification (inside
startAndroidForegroundService or during initialization) call
notifee.createChannel with id 'ptt-channel' and appropriate settings so the
notification will be shown on Android 8+; update startAndroidForegroundService
to ensure notifee.createChannel(...) completes (await) prior to the
notifee.displayNotification(...) call.
In `@src/translations/en.json`:
- Around line 692-697: The translation keys for voice channels (voice_disabled,
select_channel, select_channel_description, change_channel_warning,
default_channel, no_channels_available) were added to en.json but are missing
from ar.json and es.json; update both src/translations/ar.json and
src/translations/es.json to include these exact keys with appropriate Arabic and
Spanish translations (or placeholders if needed) so the UI doesn't fall back to
English, ensuring the keys match exactly the identifiers from en.json and
preserving JSON formatting and ordering consistent with the surrounding entries.
🧹 Nitpick comments (4)
src/components/sidebar/side-menu.tsx (4)
32-50: Hardcoded labels should use i18n translations.The menu labels are hardcoded English strings, but the codebase uses
useTranslationfor internationalization (as seen insettings.tsx). This inconsistency will cause localization issues.♻️ Suggested approach
+import { useTranslation } from 'react-i18next'; // Inside the component: +const { t } = useTranslation(); // Convert static menuItems to a function or move inside component: -const menuItems: MenuItem[] = [ - { id: 'home', label: 'Home', icon: Home, route: '/' }, +const getMenuItems = (t: TFunction): MenuItem[] => [ + { id: 'home', label: t('menu.home'), icon: Home, route: '/' }, // ... etc ];
65-67: Unused color definitions:buttonBgandbuttonText.These colors are defined but never used in this component. Consider removing them to reduce noise, or add a comment if they're reserved for future use.
♻️ Suggested fix
childBg: '#ffffff', // white for child items - buttonBg: '#2563eb', // blue-600 (matching panel buttons) - buttonText: '#ffffff', }, dark: { // ... childBg: '#111827', // gray-900 for child items - buttonBg: '#2563eb', // blue-600 (matching panel buttons) - buttonText: '#ffffff', },Also applies to: 79-81
92-95: Consider typed routes instead ofas anycast.The
route as anycast bypasses expo-router's type safety. If you have typed routes configured, consider using them; otherwise, this works but loses compile-time route validation.♻️ Alternative using Href type
+import { useRouter, Href } from 'expo-router'; - const handleNavigation = (route: string) => { + const handleNavigation = (route: Href) => { onNavigate?.(); - router.push(route as any); + router.push(route); };Then update
MenuItem.routeto beHref<string>or keep asstringif routes aren't fully typed in your project.
152-156: testID placement may be misleading.The
testID="side-menu-scroll-view"is on the outerView, not the actualScrollView. This could confuse tests expecting scroll behavior on this element.♻️ Suggested fix
- <View style={[styles.container, { backgroundColor: theme.background }]} testID="side-menu-scroll-view"> + <View style={[styles.container, { backgroundColor: theme.background }]} testID="side-menu-container"> <View style={[styles.header, { backgroundColor: theme.headerBg, borderBottomColor: theme.headerBorder }]}> <Text style={[styles.headerText, { color: theme.headerText }]}>Menu</Text> </View> - <ScrollView style={styles.scrollView} showsVerticalScrollIndicator={false}> + <ScrollView style={styles.scrollView} showsVerticalScrollIndicator={false} testID="side-menu-scroll-view">
| "voice_disabled": "Voice disabled", | ||
| "select_channel": "Select Channel", | ||
| "select_channel_description": "Choose a voice channel to connect to", | ||
| "change_channel_warning": "Selecting a new channel will disconnect from the current one", | ||
| "default_channel": "Default", | ||
| "no_channels_available": "No voice channels available", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing translations for voice channel keys in ar.json and es.json.
These new voice channel-related keys (voice_disabled, select_channel, select_channel_description, change_channel_warning, default_channel, no_channels_available) are only present in en.json. The Arabic and Spanish translation files are missing these keys, which will cause the UI to fall back to English for non-English users.
Run the following script to confirm the missing keys:
#!/bin/bash
# Description: Check if voice channel keys exist in ar.json and es.json
echo "Checking ar.json for voice channel keys:"
rg -c '"voice_disabled"|"select_channel"|"select_channel_description"|"change_channel_warning"|"default_channel"|"no_channels_available"' src/translations/ar.json || echo "No matches found in ar.json"
echo ""
echo "Checking es.json for voice channel keys:"
rg -c '"voice_disabled"|"select_channel"|"select_channel_description"|"change_channel_warning"|"default_channel"|"no_channels_available"' src/translations/es.json || echo "No matches found in es.json"🤖 Prompt for AI Agents
In `@src/translations/en.json` around lines 692 - 697, The translation keys for
voice channels (voice_disabled, select_channel, select_channel_description,
change_channel_warning, default_channel, no_channels_available) were added to
en.json but are missing from ar.json and es.json; update both
src/translations/ar.json and src/translations/es.json to include these exact
keys with appropriate Arabic and Spanish translations (or placeholders if
needed) so the UI doesn't fall back to English, ensuring the keys match exactly
the identifiers from en.json and preserving JSON formatting and ordering
consistent with the surrounding entries.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@src/app/`(app)/_layout.tsx:
- Around line 369-415: The JSX block defining webTheme and the content variable
(including RNView, SideMenu, CreateDrawerMenuButton, and layoutStyles usage) has
Prettier formatting issues (indentation/trailing spaces); run the project
formatter (npx prettier --write) or your editor's format command on this file to
fix whitespace/indentation, ensure closing JSX tags and array style props (e.g.,
style={[layoutStyles.navBar, { paddingTop: insets.top }, webTheme.navBar]}) are
properly indented, and remove any trailing spaces so the web layout block
compiles cleanly.
- Around line 10-17: The import block in _layout.tsx is failing Prettier because
it spreads multiple named imports across lines; collapse the multi-line import
of ActivityIndicator, Platform, StyleSheet, Text as RNText, TouchableOpacity,
and View as RNView into a single-line import from 'react-native' (or run the
project's Prettier/format command) so the import statement matches the project's
formatting rules.
In `@src/app/`(app)/home.tsx:
- Around line 607-610: The expression assigning containerStyle via useMemo needs
Prettier-consistent formatting; reformat the useMemo callback that returns the
array based on colorScheme so it matches Prettier output (split the conditional
and array elements across lines as Prettier expects) and then run
Prettier/format on the file—update the code referencing containerStyle, useMemo,
colorScheme, and styles.container/styles.containerDark/styles.containerLight
accordingly so CI no longer flags formatting.
In `@src/components/sidebar/side-menu.tsx`:
- Around line 1-18: The import statements at the top of side-menu.tsx are not
sorted per the project's simple-import-sort rules, causing ESLint failures; run
the ESLint autofix (npx eslint --fix src/components/sidebar/side-menu.tsx) or
manually reorder imports into the project's groups (builtin/node, external
packages like react and react-native, scoped/external icons such as
lucide-react-native, hooks like useTranslation and useRouter, then
local/internal imports) so symbols like Href, useRouter, useTranslation, React,
useState, and the lucide icons are in the correct grouped and alphabetical order
to satisfy simple-import-sort.
In `@src/hooks/use-ptt.ts`:
- Around line 177-240: The hook uses setMutedInternal in startCallKeepSession's
dependency array before setMutedInternal is declared, causing a TDZ error; fix
by moving the setMutedInternal useCallback above startCallKeepSession (and
before any other CallKeep-related callbacks that reference it), ensuring
setMutedInternal is declared prior to startCallKeepSession and
endCallKeepSession, and keep existing references to callKeepService,
currentRoom, and setIsMuted unchanged.
♻️ Duplicate comments (1)
src/translations/en.json (1)
490-503: Add menu translations to other locales.
menu.*keys were added here, but the Spanish file in this PR doesn’t include them, so non‑English users will fall back to English. Please add matching keys in other locales.
🧹 Nitpick comments (1)
src/app/(app)/_layout.tsx (1)
580-587: Consider removing unusedbackdropstyle.The
backdropstyle is defined but doesn't appear to be used in the current implementation. If it's not needed, consider removing it to keep the codebase clean.
src/app/(app)/home.tsx
Outdated
| const containerStyle = useMemo( | ||
| () => (colorScheme === 'dark' ? [styles.container, styles.containerDark] : [styles.container, styles.containerLight]), | ||
| [colorScheme] | ||
| ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Prettier warning in containerStyle useMemo.
CI is flagging formatting here—reformat to match Prettier’s output.
🔧 Suggested Prettier-aligned change
- const containerStyle = useMemo(
- () => (colorScheme === 'dark' ? [styles.container, styles.containerDark] : [styles.container, styles.containerLight]),
- [colorScheme]
- );
+ const containerStyle = useMemo(() => (colorScheme === 'dark' ? [styles.container, styles.containerDark] : [styles.container, styles.containerLight]), [colorScheme]);📝 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 containerStyle = useMemo( | |
| () => (colorScheme === 'dark' ? [styles.container, styles.containerDark] : [styles.container, styles.containerLight]), | |
| [colorScheme] | |
| ); | |
| const containerStyle = useMemo(() => (colorScheme === 'dark' ? [styles.container, styles.containerDark] : [styles.container, styles.containerLight]), [colorScheme]); |
🧰 Tools
🪛 GitHub Actions: React Native CI/CD
[warning] 607-607: Prettier: Replace ⏎····()·=>·(colorScheme·===·'dark'·?·[styles.container,·styles.containerDark]·:·[styles.container,·styles.containerLight]),⏎····[colorScheme]⎏·· with ()·=>·(colorScheme·===·'dark'·?·[styles.container,·styles.containerDark]·:·[styles.container,·styles.containerLight]),·[colorScheme]
🤖 Prompt for AI Agents
In `@src/app/`(app)/home.tsx around lines 607 - 610, The expression assigning
containerStyle via useMemo needs Prettier-consistent formatting; reformat the
useMemo callback that returns the array based on colorScheme so it matches
Prettier output (split the conditional and array elements across lines as
Prettier expects) and then run Prettier/format on the file—update the code
referencing containerStyle, useMemo, colorScheme, and
styles.container/styles.containerDark/styles.containerLight accordingly so CI no
longer flags formatting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@src/app/`(app)/_layout.tsx:
- Around line 472-481: The web menu button in CreateDrawerMenuButton lacks an
accessibility label; update the TouchableOpacity returned when Platform.OS ===
'web' to include an accessibilityLabel (e.g., "Open navigation menu") and set
accessibilityRole="button" (and accessible={true}) so screen readers announce
its intent; locate the TouchableOpacity in CreateDrawerMenuButton (the element
with testID="drawer-menu-button" and onPress={() => setIsOpen(true)}) and add
these props while preserving existing styles and behavior.
In `@src/components/sidebar/side-menu.tsx`:
- Around line 20-38: The menu item defined in getMenuItems with id 'new-call'
currently uses the non-existent route '/new-call' causing 404s; update that
item's route property to '/call/new' so it matches the expo-router path for the
page (look for the object with id 'new-call' inside getMenuItems and replace
route: '/new-call' with route: '/call/new').
In `@src/hooks/use-ptt.ts`:
- Around line 248-261: The refreshVoiceSettings function leaves a stale error
when a subsequent refresh succeeds; after a successful await
fetchVoiceSettings() call, clear the error state and notify the error handler by
calling setError(null) and onErrorRef.current?.(null) (or the equivalent reset)
before logging success so previous error messages are removed; keep the catch
block unchanged to set errors on failure.
| const CreateDrawerMenuButton = ({ setIsOpen, colorScheme }: CreateDrawerMenuButtonProps) => { | ||
| // Use React Native primitives on web to avoid infinite render loops from gluestack-ui/lucide | ||
| if (Platform.OS === 'web') { | ||
| const isDark = colorScheme === 'dark'; | ||
| return ( | ||
| <TouchableOpacity onPress={() => setIsOpen(true)} testID="drawer-menu-button" style={layoutStyles.menuButton}> | ||
| <RNText style={[layoutStyles.menuIcon, { color: isDark ? '#f9fafb' : '#030712' }]}>☰</RNText> | ||
| </TouchableOpacity> | ||
| ); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add an accessibility label for the web menu button.
Right now the button is only an icon; screen readers won’t announce intent.
🛠️ Proposed fix
- return (
- <TouchableOpacity onPress={() => setIsOpen(true)} testID="drawer-menu-button" style={layoutStyles.menuButton}>
+ return (
+ <TouchableOpacity
+ onPress={() => setIsOpen(true)}
+ testID="drawer-menu-button"
+ accessibilityLabel="Open menu"
+ accessibilityRole="button"
+ style={layoutStyles.menuButton}
+ >
<RNText style={[layoutStyles.menuIcon, { color: isDark ? '#f9fafb' : '#030712' }]}>☰</RNText>
</TouchableOpacity>
);📝 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 CreateDrawerMenuButton = ({ setIsOpen, colorScheme }: CreateDrawerMenuButtonProps) => { | |
| // Use React Native primitives on web to avoid infinite render loops from gluestack-ui/lucide | |
| if (Platform.OS === 'web') { | |
| const isDark = colorScheme === 'dark'; | |
| return ( | |
| <TouchableOpacity onPress={() => setIsOpen(true)} testID="drawer-menu-button" style={layoutStyles.menuButton}> | |
| <RNText style={[layoutStyles.menuIcon, { color: isDark ? '#f9fafb' : '#030712' }]}>☰</RNText> | |
| </TouchableOpacity> | |
| ); | |
| } | |
| const CreateDrawerMenuButton = ({ setIsOpen, colorScheme }: CreateDrawerMenuButtonProps) => { | |
| // Use React Native primitives on web to avoid infinite render loops from gluestack-ui/lucide | |
| if (Platform.OS === 'web') { | |
| const isDark = colorScheme === 'dark'; | |
| return ( | |
| <TouchableOpacity | |
| onPress={() => setIsOpen(true)} | |
| testID="drawer-menu-button" | |
| accessibilityLabel="Open menu" | |
| accessibilityRole="button" | |
| style={layoutStyles.menuButton} | |
| > | |
| <RNText style={[layoutStyles.menuIcon, { color: isDark ? '#f9fafb' : '#030712' }]}>☰</RNText> | |
| </TouchableOpacity> | |
| ); | |
| } |
🤖 Prompt for AI Agents
In `@src/app/`(app)/_layout.tsx around lines 472 - 481, The web menu button in
CreateDrawerMenuButton lacks an accessibility label; update the TouchableOpacity
returned when Platform.OS === 'web' to include an accessibilityLabel (e.g.,
"Open navigation menu") and set accessibilityRole="button" (and
accessible={true}) so screen readers announce its intent; locate the
TouchableOpacity in CreateDrawerMenuButton (the element with
testID="drawer-menu-button" and onPress={() => setIsOpen(true)}) and add these
props while preserving existing styles and behavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@src/components/maps/full-screen-location-picker.web.tsx`:
- Around line 47-64: The reverseGeocode function currently parses
response.json() without checking HTTP status; update reverseGeocode to check
response.ok after the fetch (the call using Env.MAPBOX_PUBKEY) and handle
non-2xx responses by logging the status and body (or message), setting
setAddress(undefined) and skipping JSON parsing or throwing to be caught by the
existing catch block; ensure setIsReverseGeocoding(false) still runs in finally
and keep existing state updates (setAddress, setIsReverseGeocoding) in their
current places.
- Line 12: The MAPBOX_GL_CSS_URL constant currently pins the CSS to v3.1.2;
update MAPBOX_GL_CSS_URL to use
'https://api.mapbox.com/mapbox-gl-js/v3.15.0/mapbox-gl.css' so the CSS version
matches the installed mapbox-gl v3.15.0, and make the same change in the other
map components (Map, UnifiedMapView, LocationPicker, StaticMap) that hardcode
the CSS URL so all map controls/popups render correctly.
In `@src/components/maps/location-picker.web.tsx`:
- Around line 88-139: The map never initializes when currentLocation is
initially null because the map container div isn't rendered, causing the
useEffect that creates the Mapbox instance (useEffect that references
map.current and mapContainer.current) to return early; always render the DOM
node referenced by mapContainer (so mapContainer.current is non-null) regardless
of currentLocation and move any UI overlay logic (the "no location" UI) outside
or on top of that div; ensure the existing initializer in the useEffect creates
map.current and marker.current as before and keep getUserLocation() call, so the
effect can run and later updates to currentLocation just update marker.current
via the existing on('click') handler or setCurrentLocation logic.
In `@src/hooks/use-ptt.ts`:
- Around line 248-263: In refreshVoiceSettings (the useCallback defined as
refreshVoiceSettings), remove the success-path invocation
onErrorRef.current?.(null) since onError is typed as (error: string) => void;
instead only call onErrorRef.current when an actual error occurs; keep
setError(null) on success but do not pass null to onErrorRef.current and leave
the catch block unchanged (it should continue to call onErrorRef.current with
the error message).
In `@src/translations/en.json`:
- Around line 500-513: Add the missing "menu" translation block to both ar.json
and es.json by copying the "menu" object from en.json and translating (or at
minimum providing placeholders) for the keys: "calls", "calls_list", "contacts",
"home", "map", "menu", "messages", "new_call", "personnel", "protocols",
"settings", and "units"; ensure the JSON structure matches en.json (same key
names and nesting) so the UI no longer falls back to English.
♻️ Duplicate comments (1)
src/translations/ar.json (1)
725-730: Voice channel translations added - addresses previous review feedback.The Arabic translations for voice channel keys are now properly included. The translations appear accurate and grammatically correct.
🧹 Nitpick comments (7)
src/components/sidebar/side-menu.tsx (2)
67-75: Consider memoizingmenuItemsto avoid recreation on each render.
getMenuItems(t)creates a new array and objects on every render. For a small menu this is negligible, but memoizing prevents unnecessary work.♻️ Optional optimization
function SideMenu({ onNavigate, colorScheme: propColorScheme }: SideMenuProps): React.JSX.Element { const router = useRouter(); const { t } = useTranslation(); const [expandedItems, setExpandedItems] = useState<Set<string>>(new Set()); - const menuItems = getMenuItems(t); + const menuItems = React.useMemo(() => getMenuItems(t), [t]);
94-120: Consider adding accessibility labels for screen reader support.The
Pressableelements lackaccessibilityLabelandaccessibilityRoleprops, which would improve the experience for users relying on assistive technologies.♿ Optional accessibility enhancement
<Pressable + accessibilityRole="button" + accessibilityLabel={hasChildren ? `${item.label}, expandable` : item.label} + accessibilityState={{ expanded: hasChildren ? isExpanded : undefined }} onPress={() => { if (hasChildren) { toggleExpanded(item.id);src/hooks/use-ptt.ts (1)
54-66: Unused variablevoipServerWebsocketSslAddress.The variable
voipServerWebsocketSslAddressis destructured from the store but never used in this hook. The connection usestargetChannel.Tokendirectly. Consider removing it to reduce noise.♻️ Suggested fix
const { isConnected: storeConnected, isConnecting: storeConnecting, currentRoom, currentRoomInfo, isVoiceEnabled, - voipServerWebsocketSslAddress, availableRooms, fetchVoiceSettings, connectToRoom, disconnectFromRoom, } = useLiveKitStore();src/components/maps/full-screen-location-picker.web.tsx (2)
1-1: Unused import:MapPinIcon
MapPinIconis imported but never used in this component.Suggested fix
-import { MapPinIcon, XIcon } from 'lucide-react-native'; +import { XIcon } from 'lucide-react-native';
141-152: Duplicated dragend handler logicThe
dragendhandler logic is duplicated for the initial marker (lines 142-152) and the marker created on click (lines 172-182). Consider extracting to a shared helper.Suggested refactor
+ const handleMarkerDragEnd = useCallback(() => { + const lngLat = marker.current?.getLngLat(); + if (lngLat) { + const newLocation = { + latitude: lngLat.lat, + longitude: lngLat.lng, + }; + setCurrentLocation(newLocation); + reverseGeocode(newLocation.latitude, newLocation.longitude); + } + }, [reverseGeocode]);Then use
marker.current.on('dragend', handleMarkerDragEnd);in both places.Also applies to: 172-182
src/components/maps/location-picker.web.tsx (2)
11-11: CSS version mismatch (same as full-screen variant)The CSS URL references v3.1.2 while the package is v3.15.0. Consider aligning versions or extracting the CSS URL to a shared constant.
1-217: Consider extracting shared Mapbox logicThis component shares significant code with
full-screen-location-picker.web.tsx:
- CSS injection (lines 34-42)
- Geolocation logic (lines 44-86)
- Map initialization pattern (lines 88-139)
- Marker creation/update
Consider extracting into shared hooks like
useMapboxCss(),useGeolocation(), oruseMapboxMap()to reduce duplication and ensure consistent behavior.
| import { Env } from '@/lib/env'; | ||
|
|
||
| // Mapbox GL CSS needs to be injected for web | ||
| const MAPBOX_GL_CSS_URL = 'https://api.mapbox.com/mapbox-gl-js/v3.1.2/mapbox-gl.css'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
🏁 Script executed:
# Find package.json and check mapbox-gl version
find . -name "package.json" -o -name "package-lock.json" -o -name "yarn.lock" | head -5Repository: Resgrid/Dispatch
Length of output: 282
🏁 Script executed:
# Search for mapbox-gl version in package files
rg "mapbox-gl" --type json -A 2 -B 2 | head -50Repository: Resgrid/Dispatch
Length of output: 275
🏁 Script executed:
# Check if there are other CSS URL references in the codebase
rg "mapbox-gl.*css" --type ts --type tsx -B 2 -A 2Repository: Resgrid/Dispatch
Length of output: 87
🏁 Script executed:
# Search for all mapbox-gl version mentions
rg "v3\." -t ts -t tsx -t json | grep -i mapbox | head -20Repository: Resgrid/Dispatch
Length of output: 87
🏁 Script executed:
# Search for mapbox-gl CSS references with proper syntax
rg "mapbox-gl.*css" --type-add 'web:*.{ts,tsx,jsx,js}' -t web -B 2 -A 2Repository: Resgrid/Dispatch
Length of output: 4467
🏁 Script executed:
# Check the specific file mentioned in the review
cat -n src/components/maps/full-screen-location-picker.web.tsx | head -30Repository: Resgrid/Dispatch
Length of output: 1473
🏁 Script executed:
# Search for all version references to mapbox-gl
rg "mapbox" src/ -i -B 1 -A 1Repository: Resgrid/Dispatch
Length of output: 29765
🌐 Web query:
Mapbox GL JS CSS version compatibility matching package version best practice
💡 Result:
Short best-practice checklist for Mapbox GL JS + CSS version compatibility
- Always use the CSS file that matches the exact Mapbox GL JS release you’re using (same version number). The Mapbox docs show JS+CSS are distributed together and the CSS is required for proper map controls/popups. [1][2].
- Install or import the CSS from the same source as the JS (CDN pair or the same npm package path e.g. import "mapbox-gl/dist/mapbox-gl.css" or the CDN URL with the same vX.Y.Z). This avoids runtime layout/style mismatches. [1][2].
- Lock versions in package.json (or your CDN URLs) and update both JS and CSS together in one change; use exact versions or a lockfile to prevent drifting. (See Mapbox migration guidance when upgrading between major versions.) [1][4].
- When upgrading between major versions, read the Mapbox changelog/migration guide for any class/markup/style changes that affect plugins or custom CSS; test UI elements (controls, popups, markers) after upgrade. [1][4].
- If you use forks/alternatives (MapLibre), import that package’s CSS (maplibre‑gl.css) rather than Mapbox’s — class names and CSS details may differ. MapLibre’s migration docs call out replacing the CSS link when switching. [3].
- For frameworks (react-map-gl, plugins): ensure ancillary type packages or companion libs match the maplib version you use (e.g.,
@types/mapbox-glshould match mapbox-gl v1/v2 if used). Check the framework’s upgrade notes. [5].
References
- Mapbox GL JS docs — CDN usage and CSS required. [1][2].
- Mapbox GL JS migration/changelog guidance. [4].
- MapLibre migration guide (CSS vs Mapbox differences). [3].
- react-map-gl upgrade notes on matching maplib/mapbox versions. [5].
Update Mapbox GL CSS version to match installed package (v3.15.0)
The CSS URL hardcodes v3.1.2, but mapbox-gl v3.15.0 is installed. Official Mapbox documentation requires the CSS to match the JavaScript version exactly to ensure proper rendering of map controls and popups. Update the CSS URL and the same hardcoded version in the other map components (src/app/(app)/map.web.tsx, src/components/maps/unified-map-view.web.tsx, src/components/maps/location-picker.web.tsx, and src/components/maps/static-map.web.tsx).
🤖 Prompt for AI Agents
In `@src/components/maps/full-screen-location-picker.web.tsx` at line 12, The
MAPBOX_GL_CSS_URL constant currently pins the CSS to v3.1.2; update
MAPBOX_GL_CSS_URL to use
'https://api.mapbox.com/mapbox-gl-js/v3.15.0/mapbox-gl.css' so the CSS version
matches the installed mapbox-gl v3.15.0, and make the same change in the other
map components (Map, UnifiedMapView, LocationPicker, StaticMap) that hardcode
the CSS URL so all map controls/popups render correctly.
| "menu": { | ||
| "calls": "Calls", | ||
| "calls_list": "Calls List", | ||
| "contacts": "Contacts", | ||
| "home": "Home", | ||
| "map": "Map", | ||
| "menu": "Menu", | ||
| "messages": "Messages", | ||
| "new_call": "New Call", | ||
| "personnel": "Personnel", | ||
| "protocols": "Protocols", | ||
| "settings": "Settings", | ||
| "units": "Units" | ||
| }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Verify if menu section exists in ar.json and es.json
echo "Checking for 'menu' section in ar.json:"
rg -n '"menu":\s*\{' src/translations/ar.json || echo "No 'menu' section found in ar.json"
echo ""
echo "Checking for 'menu' section in es.json:"
rg -n '"menu":\s*\{' src/translations/es.json || echo "No 'menu' section found in es.json"
echo ""
echo "Confirming 'menu' section exists in en.json:"
rg -n '"menu":\s*\{' src/translations/en.json || echo "No 'menu' section found in en.json"Repository: Resgrid/Dispatch
Length of output: 272
Add missing menu section to ar.json and es.json.
The new menu section with navigation keys (calls, calls_list, contacts, home, map, menu, messages, new_call, personnel, protocols, settings, units) is only present in en.json (lines 500-513). Both ar.json and es.json are missing this entire section, which will cause the UI to fall back to English for non-English users.
🤖 Prompt for AI Agents
In `@src/translations/en.json` around lines 500 - 513, Add the missing "menu"
translation block to both ar.json and es.json by copying the "menu" object from
en.json and translating (or at minimum providing placeholders) for the keys:
"calls", "calls_list", "contacts", "home", "map", "menu", "messages",
"new_call", "personnel", "protocols", "settings", and "units"; ensure the JSON
structure matches en.json (same key names and nesting) so the UI no longer falls
back to English.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/components/maps/full-screen-location-picker.web.tsx`:
- Around line 218-259: The early return when isLoading removes the map <div> and
detaches the Mapbox element (mapContainer / map.current) so the useEffect
initialization (which guards with if (map.current) return) never reattaches;
instead always render the map container div (the ref mapContainer) so Mapbox
stays mounted and move the loading UI into an overlay that sits on top
(absolute/fixed within styles.container) when isLoading is true; remove the
early return branch that returns only the loading view and keep existing
functions/refs (mapContainer, map.current, useEffect) intact so the map instance
remains bound while the overlay shows loading.
- Around line 74-101: getUserLocation currently only updates an existing marker
(marker.current) so when initialLocation is null no marker is created; update
getUserLocation to mirror the map click handler by creating a new marker when
marker.current is falsy: instantiate marker.current = new mapboxgl.Marker({/*
same options as click handler */}).setLngLat([newLocation.longitude,
newLocation.latitude]).addTo(map.current), attach the same dragend handler (if
used), and ensure setCurrentLocation(newLocation),
reverseGeocode(newLocation.latitude, newLocation.longitude) and
setIsLoading(false) are called as before; keep all map movement logic using
map.current.flyTo unchanged.
♻️ Duplicate comments (1)
src/components/maps/location-picker.web.tsx (1)
147-174: Keep the map container mounted during loading to avoid a blank mapWhen
isLoadingis true, the component returns early and unmounts the map<div>. The Mapbox instance remains bound to the old DOM node and won’t reinitialize, so the map can stay blank after loading completes. Keep the map container always mounted and overlay the loading UI instead. Please verify by running the web picker, clicking “Get my location,” and confirming the map remains visible after loading.🛠️ Suggested fix (keep map mounted + overlay loading)
- if (isLoading) { - return ( - <View style={StyleSheet.flatten([styles.container, { height }])}> - <View style={styles.loadingContainer}> - <Text style={styles.loadingText}>{t('common.loading')}</Text> - </View> - </View> - ); - } - return ( <View style={StyleSheet.flatten([styles.container, { height }])}> <div ref={mapContainer} style={{ height: '100%', width: '100%' }} /> - {!currentLocation ? ( + {isLoading && ( + <View style={[styles.loadingContainer, StyleSheet.absoluteFill]}> + <Text style={styles.loadingText}>{t('common.loading')}</Text> + </View> + )} + {!isLoading && !currentLocation ? ( <View style={styles.noLocationContainer}> <Text style={styles.noLocationText}>{t('common.no_location')}</Text> <Pressable onPress={getUserLocation}> <Text style={styles.getLocationText}>{t('common.get_my_location')}</Text> </Pressable> </View> ) : null} <View style={styles.buttonContainer}> - <Button onPress={handleConfirmLocation} disabled={!currentLocation}> + <Button onPress={handleConfirmLocation} disabled={!currentLocation || isLoading}> <ButtonText>{t('common.confirm_location')}</ButtonText> </Button> </View> </View> );
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
Approve |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR is approved.
Summary by CodeRabbit
New Features
Improvements
Chores
Tests
✏️ Tip: You can customize this high-level summary in your review settings.