Develop #214
Develop #214
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. 📝 WalkthroughWalkthroughConverted many Zustand store consumers to selector-based access, added extensive web shims and platform guards for native services, memoized UI context objects, refactored map/web rendering and Electron asset loading, updated build/runtime environment vars, and migrated tests to selector-aware mocks. Changes
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
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. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (15)
src/components/ui/shared-tabs.tsx (1)
17-20:⚠️ Potential issue | 🟠 MajorGlobal singleton store will cause conflicts between multiple
SharedTabsinstances.
useTabStoreis created once at module scope, so everySharedTabsthat receives anonChangeprop shares the sameactiveIndex. If two tab groups coexist (e.g., on the same screen or across mounted screens), selecting a tab in one will switch the other.Additionally,
initialIndex(line 57) only seedslocalActiveIndex— the global store always starts at0, soinitialIndexis silently ignored whenonChangeis provided.Consider either:
- Lifting tab state entirely to the parent (controlled component pattern — remove the store, accept
activeIndex+onChangeas props), or- Using local
useStateexclusively (the store adds no value here since it isn't consumed outside this component).Option 1 is the cleanest for a shared UI component:
Proposed refactor (controlled + uncontrolled pattern)
-// Tab state management with zustand -interface TabState { - activeIndex: number; - setActiveIndex: (index: number) => void; -} - -const useTabStore = create<TabState>((set) => ({ - activeIndex: 0, - setActiveIndex: (index) => set({ activeIndex: index }), -}));interface SharedTabsProps { tabs: TabItem[]; + activeIndex?: number; // controlled mode initialIndex?: number; scrollable?: boolean; ... }- const [localActiveIndex, setLocalActiveIndex] = useState(initialIndex); - const activeIndex = useTabStore((s) => s.activeIndex); - const setActiveIndex = useTabStore((s) => s.setActiveIndex); + const [localActiveIndex, setLocalActiveIndex] = useState(initialIndex); + const currentIndex = activeIndex ?? localActiveIndex;Also applies to: 58-59
src/services/push-notification.ts (1)
465-472:⚠️ Potential issue | 🟡 MinorPush token logged in plaintext.
The FCM push token is logged at
infolevel. While useful for debugging, push tokens are semi-sensitive — they can be used to send notifications to a specific device. Consider logging only a truncated portion or moving this todebuglevel so it's excluded from production logs (where severity is'warn').src/components/settings/audio-device-selection.tsx (1)
47-47:⚠️ Potential issue | 🟡 MinorHardcoded English string bypasses i18n.
The fallback
' Device'suffix is not wrapped int(), so it won't be translated. As per coding guidelines, all user-facing text should uset()fromreact-i18next.Proposed fix
- return deviceType.charAt(0).toUpperCase() + deviceType.slice(1) + ' Device'; + return t('settings.audio_device_selection.generic_device', { type: deviceType.charAt(0).toUpperCase() + deviceType.slice(1) });src/components/settings/bluetooth-device-selection-bottom-sheet.tsx (1)
258-283:⚠️ Potential issue | 🟡 MinorHardcoded user-facing strings should be wrapped in
t().Lines 258, 281, and 283 contain hardcoded English text (
"System Audio","AirPods, Car, Wired Headset") that should uset()for internationalization. As per coding guidelines, all user-facing text must be wrapped int()fromreact-i18next.♻️ Suggested fix
- await setPreferredDevice({ id: 'system-audio', name: 'System Audio' }); + await setPreferredDevice({ id: 'system-audio', name: t('bluetooth.system_audio') });- <Text className={`font-medium ${preferredDevice?.id === 'system-audio' ? 'text-primary-700 dark:text-primary-300' : 'text-neutral-900 dark:text-neutral-100'}`}> - System Audio - </Text> - <Text className="text-xs text-neutral-500">AirPods, Car, Wired Headset</Text> + <Text className={`font-medium ${preferredDevice?.id === 'system-audio' ? 'text-primary-700 dark:text-primary-300' : 'text-neutral-900 dark:text-neutral-100'}`}> + {t('bluetooth.system_audio')} + </Text> + <Text className="text-xs text-neutral-500">{t('bluetooth.system_audio_description')}</Text>src/components/roles/roles-modal.tsx (1)
60-80:⚠️ Potential issue | 🟠 MajorBug: Unassignment logic is broken —
||operator falls back to the current assignment.On line 69,
pendingAssignment?.userId || currentAssignment?.UserId || ''uses||, so when a pending unassignment setsuserIdtoundefined(line 49), the expression falls through tocurrentAssignment?.UserId, effectively ignoring the unassignment.Compare with
roles-bottom-sheet.tsxwhich handles this differently by excluding entries without auserIdfrompendingAssignmentsentirely (lines 67–71) and filtering out roles withoutUserIdin the save (line 100).To fix, use nullish coalescing (
??) or check for the existence of a pending entry separately:Proposed fix
- const assignedUserId = pendingAssignment?.userId || currentAssignment?.UserId || ''; + const assignedUserId = pendingAssignment + ? (pendingAssignment.userId ?? '') + : (currentAssignment?.UserId ?? '');src/stores/calls/store.ts (1)
32-48:⚠️ Potential issue | 🔴 CriticalThe new
isLoadingguard combined with missing error handling permanently locks the store on failure.The
initmethod has notry/catch. If any of the three API calls (lines 38–40) fails,isLoadingremainstrueandisInitializedremainsfalse. The new guard at line 34 checksget().isLoading, so any subsequentinit()call will return early — the store is permanently stuck in a loading state with no way to recover.Before this PR, a failed init would still leave
isLoading: true, but re-mounting could re-enterinit. The new guard makes this a hard lock.Proposed fix: wrap in try/catch like the roles store
init: async () => { // Prevent re-initialization during tree remounts if (get().isInitialized || get().isLoading) { return; } set({ isLoading: true, error: null }); + try { const callsResponse = await getCalls(); const callPrioritiesResponse = await getCallPriorities(); const callTypesResponse = await getCallTypes(); set({ calls: Array.isArray(callsResponse.Data) ? callsResponse.Data : [], callPriorities: Array.isArray(callPrioritiesResponse.Data) ? callPrioritiesResponse.Data : [], callTypes: Array.isArray(callTypesResponse.Data) ? callTypesResponse.Data : [], isLoading: false, isInitialized: true, lastFetchedAt: Date.now(), }); + } catch (error) { + set({ error: 'Failed to initialize calls', isLoading: false }); + } },src/app/call/new/index.tsx (2)
2-2:⚠️ Potential issue | 🟠 MajorRemove unused
@testing-library/react-nativeimport from production code.
renderis imported but never used in this file. Including a test-only library in a production component inflates the bundle and may cause issues on platforms where the library isn't available.Proposed fix
-import { render } from '@testing-library/react-native';
204-204:⚠️ Potential issue | 🟡 MinorRemove
console.logthat dumps the full form data in production.This logs the entire call creation payload including address, coordinates, contact info, and dispatch details. Consider removing it or gating behind
__DEV__.Proposed fix
- console.log('Creating new call with data:', data);src/components/calls/call-images-modal.tsx (1)
426-426:⚠️ Potential issue | 🟠 Major
Math.random()inkeyExtractorbreaks React's reconciliation.When an item lacks an
Id, the random fallback produces a new key every render, causing the FlatList to remount items and lose scroll position. Use theindexparameter or a stable fallback instead.Proposed fix
- keyExtractor={(item) => item?.Id || `image-${Math.random()}`} + keyExtractor={(item, index) => item?.Id || `image-fallback-${index}`}src/app/call/[id]/edit.tsx (1)
215-215:⚠️ Potential issue | 🟡 MinorRemove
console.logbefore merging.Line 215 has a
console.logthat should be replaced withlogger.debug(or removed) to stay consistent with the structured logging used elsewhere in the codebase.Proposed fix
- console.log('Updating call with data:', data); + logger.debug({ message: 'Updating call with data', context: { data } });src/services/bluetooth-audio.service.ts (1)
852-853:⚠️ Potential issue | 🟡 MinorDuplicate assignment:
this.isInitialized = falseappears twice.Line 852 and 853 are identical. One should likely be removed — or the second was intended to reset a different flag.
🐛 Proposed fix
// Reset initialization flags this.isInitialized = false; - this.isInitialized = false; this.hasAttemptedPreferredDeviceConnection = false;src/components/maps/map-view.web.tsx (1)
286-293:⚠️ Potential issue | 🟠 Major
getBoundingClientRect()on an empty container will return zero dimensions.When
anchoris an{x, y}object, this code callscontainer.getBoundingClientRect()immediately after creating the marker. At this point, the container has no children rendered yet (children are rendered in a separate effect at Lines 325-329), sorect.widthandrect.heightwill both be0, making the offset calculation always[0, 0].Consider moving the offset calculation into the children-rendering effect, after
root.render(...)completes, or deferring it withrequestAnimationFrame.🐛 Sketch of a fix — defer offset calculation
+ // Recalculate anchor offset after children are rendered + useEffect(() => { + if ( + markerRef.current && + containerRef.current && + typeof anchor === 'object' && anchor !== null && 'x' in anchor && 'y' in anchor + ) { + // Use rAF to ensure layout has settled after React render + const frameId = requestAnimationFrame(() => { + const rect = containerRef.current?.getBoundingClientRect(); + if (rect && rect.width > 0 && rect.height > 0) { + const xOffset = (anchor.x - 0.5) * rect.width; + const yOffset = (anchor.y - 0.5) * rect.height; + markerRef.current?.setOffset([xOffset, yOffset]); + } + }); + return () => cancelAnimationFrame(frameId); + } + }, [children, anchor]);And remove the inline offset calculation from Lines 288-293.
src/features/livekit-call/components/LiveKitCallModal.tsx (1)
88-152:⚠️ Potential issue | 🟡 MinorHardcoded English strings should be wrapped in
t()for i18n.Multiple user-facing strings are not wrapped in
t(): "Connecting to", "Connection Error", "Try Again", "Connected", "You are in room:", "Your ID:", "Mute", "Unmute", "Leave Call", "Join a Voice Call", "Select a room to join:", and the "Join" button text. As per coding guidelines, all user-facing text must uset()fromreact-i18nextwith dictionary files insrc/translations.src/app/(app)/_layout.tsx (1)
104-160:⚠️ Potential issue | 🟡 MinorInitialization failure dismisses loading overlay without user feedback.
setIsInitComplete(true)in thefinallyblock (line 158) means the loading overlay is removed even when initialization fails. WhilehasInitialized.currentstaysfalseallowing retry, the user sees the full app shell without any indication that data loading failed or a way to retry.Consider showing an error state or retry prompt instead of silently proceeding.
src/stores/app/livekit-store.ts (1)
558-601:⚠️ Potential issue | 🟡 MinorFix
disconnectFromRoomreturn type: interface declaresvoidbut implementation isasync.The interface at line 161 declares
disconnectFromRoom: () => void, but the implementation at line 558 isasync, returning a Promise. This creates a type contract violation where callers likesrc/services/app-reset.service.ts:215correctlyawaitthe function, while UI components inLiveKitCallModal.tsxandlivekit-bottom-sheet.tsxcall it without awaiting. Although internal try-catch blocks handle errors, callers that don't await may experience unhandled rejections if errors occur. The interface should reflect the async implementation.🔧 Suggested type fix
// Room operations connectToRoom: (roomInfo: DepartmentVoiceChannelResultData, token: string) => Promise<void>; - disconnectFromRoom: () => void; + disconnectFromRoom: () => Promise<void>;
🤖 Fix all issues with AI agents
In `@electron/main.js`:
- Around line 183-201: The protocol handler accepts a user-controlled pathname
which can contain ".." and escape distPath; fix by resolving the final path with
path.resolve(distPath, decodeURIComponent(url.pathname)) and verify the resolved
path starts with the resolved distPath (e.g., const resolvedDist =
path.resolve(distPath)) before serving, falling back to index.html on mismatch;
also import pathToFileURL from 'url' and replace the net.fetch('file://' +
filePath) call with net.fetch(pathToFileURL(filePath).toString()) for correct
cross-platform file URLs in the protocol.handle callback.
In `@scripts/patch-nativewind.py`:
- Around line 42-74: The script currently does a full-file overwrite of
useColorScheme.js by writing patched_hook to use_color_scheme_path which can
silently clobber upstream changes; instead change Patch 2 to perform a targeted
replacement (e.g., locate and replace only the problematic block within
useColorScheme.js) or add a file-hash/version gate before writing: read the
existing useColorScheme.js, verify its hash or a known marker and only apply a
precise replace of the specific function/body (referencing useColorScheme,
prevEffect, cleanupEffect, and colorScheme.get/set/toggle) or abort with a clear
error if the file differs so you don't overwrite unexpected upstream updates.
- Around line 12-15: The current calls to content.replace(...) in
scripts/patch-nativewind.py can silently no-op if the target string changes;
after performing each substitution (e.g., the darkModeFlag change where you
replace the literal 'const darkModeFlag =
stylesheet_1.StyleSheet.getFlag("darkMode");'), assert the replacement actually
occurred by comparing the new string to the original and raise/exit with a clear
error if unchanged; apply this guard pattern to every replace() call in the
script (including the other calls you noted around the other patches) so each
substitution is verified and the script fails loudly when a patch didn't apply.
- Around line 4-7: The hardcoded absolute path should be replaced by a path
derived from the script location: compute a project_root (e.g., based on
os.path.dirname(__file__) and os.path.join(.., '..' as needed)) and then build
color_scheme_path using os.path.join(project_root, 'node_modules',
'react-native-css-interop', 'dist', 'runtime', 'web', 'color-scheme.js'); make
the same change for use_color_scheme_path and api_path so all three
(color_scheme_path, use_color_scheme_path, api_path) are constructed relative to
the script file or an optional CLI argument for root.
In `@src/app/call/`[id].tsx:
- Line 28: The import securityStore is used like a React hook but lacks the
conventional "use" prefix and triggers react-hooks rules; update the import to
alias securityStore as useSecurityStore (so it's consistent with other hooks
like useCallDetailStore/useCoreStore), then replace calls to securityStore(...)
with useSecurityStore(...) such as where const canUserCreateCalls is derived,
ensuring the hook naming convention is followed across the file.
In `@src/components/sidebar/sidebar-content.tsx`:
- Around line 66-88: The mapping over activeStatuses.Statuses can crash because
invertColor(status.BColor, true) throws for null/invalid hex; update the
rendering in sidebar-content.tsx to compute a safe text color before passing it
to ButtonText: either add a small validator (or reuse an existing isValidHex
helper in src/lib/utils.ts) and if status.BColor is missing/invalid fall back to
a default color (e.g., '#000000' or computed via a safeInvert fallback), or wrap
the invertColor call in try/catch and use the fallback on error; ensure you only
change the color computation (the Button/Text props and setIsOpen(status) usage
remain the same so the UI and action are preserved.
- Line 38: Replace the hardcoded unitName string with a translated value: change
the SidebarUnitCard prop unitName="No Unit" to use t(...) (e.g.,
unitName={t('common.no_unit')} or the appropriate key), ensuring the t function
from react-i18next is in scope; update the unitName prop usage in the
SidebarUnitCard component invocation so all user-facing text is wrapped with
t().
In `@src/components/status/status-bottom-sheet.tsx`:
- Line 8: Remove the unused import "useShallow" from 'zustand/react/shallow' in
the StatusBottomSheet component file; locate the import line that includes
useShallow (the one at the top of src/components/status/status-bottom-sheet.tsx)
and delete only that identifier so the file no longer imports an unused
symbol—no other code changes are needed.
In `@src/components/ui/skeleton/index.tsx`:
- Around line 24-64: customTimingFunction (created via Easing.bezier(0.4, 0,
0.6, 1)) is recreated every render which causes the useEffect that controls the
Animated.loop to restart continuously; fix by memoizing or hoisting that easing
function so its reference is stable (e.g., move Easing.bezier(...) outside the
component or wrap it in useMemo) and then remove the unstable
customTimingFunction from the effect dependencies (or keep the stable reference
there) so the animation lifecycle in the useEffect (which references pulseAnim,
animationDuration, isLoaded, isWeb, animRef) only runs when meaningful inputs
change.
In `@src/lib/countly-shim.web.ts`:
- Line 18: The initWithConfig function currently types its parameter as any;
change its signature to use the existing CountlyConfig type (preserving
optionality if needed) so the parameter becomes (_config?: CountlyConfig) and
update any internal references to rely on the typed shape; refer to the
initWithConfig method and the CountlyConfig type in this file when making the
change.
In `@src/lib/native-module-shims.web.ts`:
- Around line 188-258: The Countly shim duplicate should be removed and
re-exported from the existing module to avoid divergence: replace the local
Countly object with a direct re-export of the canonical export from the existing
countly-shim.web.ts so the exported symbol Countly (and associated types like
CountlyConfig and method initWithConfig) come from that single source of truth;
update any references in this file to export/import the same named export rather
than redefining functions like init, start, getCurrentDeviceId, etc., ensuring
TypeScript types align with CountlyConfig and initWithConfig from the original
module.
In `@src/services/notification-sound.service.ts`:
- Around line 49-54: performInitialization returns early on web leaving sound
properties null which causes playNotificationSound to emit repeated logger.warn
messages; update playNotificationSound (and/or its helper methods) to check
Platform.OS === 'web' and return silently when on web (or suppress the
"Notification sound not loaded" warning when Platform.OS === 'web'), referencing
the performInitialization and playNotificationSound functions and the
logger.warn call so the guard is added before any null-check warning is logged.
In `@src/stores/app/loading-store.ts`:
- Around line 47-48: The hook currently subscribes to the entire loadingStates
map via useLoadingStore((s) => s.loadingStates) causing all consumers to
re-render; change the selector to pick the specific primitive for the requested
key (e.g., useLoadingStore((s) => s.loadingStates[key] ?? false)) and assign
that directly to loading so only updates to that key trigger re-renders (update
the selector call in the useLoading / loading-store code that references
loadingStates and key).
- Line 52: toggleLoading currently closes over the stale loading value via
useCallback, causing rapid consecutive calls to cancel each other; fix by moving
the toggle logic into the store or by reading the fresh value from the store
before updating: add a store action (e.g., toggleLoading) that uses set(state =>
({ loadingStates: { ...state.loadingStates, [key]: !state.loadingStates[key]
}})) and replace the hook's useCallback toggleLoading (which calls
setLoading(key, !loading)) with a call to the new store.toggleLoading(key) (or,
if not adding a store action, have the hook call setLoading with a function that
reads the current store value via get() to compute the negation) so updates use
the latest state rather than the render-time captured loading.
🧹 Nitpick comments (36)
src/components/push-notification/push-notification-modal.tsx (2)
83-83: Avoid{...({} as any)}— this suppresses type errors withany.This empty-object-cast-to-
anyspread is a type workaround that hides the actual type mismatch between theModalcomponent's props and what's being passed. If this is needed to work around a gluestack-ui typing issue, consider a more targeted suppression (e.g.,@ts-expect-errorwith a comment explaining why) so future type fixes aren't silently swallowed.Suggested alternative
- <Modal isOpen={isOpen} onClose={handleClose} size="md" {...({} as any)}> + {/* `@ts-expect-error` gluestack-ui Modal type mismatch — remove when types are fixed */} + <Modal isOpen={isOpen} onClose={handleClose} size="md">As per coding guidelines: "Avoid using
any; strive for precise types".
14-29: Consider wrappingNotificationIconinReact.memo.Since this component receives a simple
typeprop and renders pure output, memoizing it prevents unnecessary re-renders when the parent re-renders for unrelated reasons.-const NotificationIcon = ({ type }: { type: NotificationType }) => { +const NotificationIcon = React.memo(({ type }: { type: NotificationType }) => { // ... -}; +});As per coding guidelines: "Use
React.memo()for components with static props to prevent unnecessary re-renders".src/components/sidebar/unit-sidebar.tsx (2)
21-21: Consider wrapping inReact.memoto avoid parent-triggered re-renders.The component's props (
unitName,unitType,unitGroup,bgColor) are likely stable strings from the parent. Wrapping inReact.memowould skip re-renders caused by parent updates when these props haven't changed.
98-139:callButton,mapLockButton, andaudioStreamButtonstyles are identical — consider consolidating.Same for their
*Activecounterparts. A single shared style (e.g.,iconButton/iconButtonActive) would reduce duplication.src/components/ui/gluestack-ui-provider/index.web.tsx (2)
53-60:MediaQueryList.addListener/removeListenerare deprecated.These were deprecated in favor of
addEventListener/removeEventListener. While this appears to be pre-existing code, consider updating since this is a web-specific file.Suggested fix
- media.addListener(handleMediaQuery); + media.addEventListener('change', handleMediaQuery); - return () => media.removeListener(handleMediaQuery); + return () => media.removeEventListener('change', handleMediaQuery);
62-75: Consider addingcssVariablesWithModeto the dependency array.Line 70 references
cssVariablesWithModebut it's not listed in the deps at Line 75. Since the value is memoized with[]it won't cause extra runs, but including it satisfies the exhaustive-deps lint rule and makes the dependency explicit.src/components/ui/shared-tabs.tsx (2)
143-143: Anonymous arrow functions inonPressinside map loops.Each render creates new function references per tab, which can cause unnecessary re-renders of
Pressablechildren. Consider extracting a memoizedTabItemsub-component that receivesindexandonPressas props.As per coding guidelines: "Avoid anonymous functions in renderItem or event handlers to prevent re-renders."
Also applies to: 161-161
120-135:StyleSheet.createcalled inside a render-path function.
getContainerStylecallsStyleSheet.createon every render.StyleSheet.createis meant to be called once (typically at module scope) to register styles. Move the style computation touseMemoor use plain style objects if they need to be dynamic.patches/@gluestack-ui+nativewind-utils+1.0.28.patch (2)
11-23:shallowEqualis duplicated four times across the patched files.Since this is a
patch-packagepatch where each file must be self-contained, extracting to a shared module would require patching an additional file (e.g.,utils). Consider patching the library's existing utils module to exportshallowEqualonce, then importing it in each file — this would reduce drift if the logic ever needs updating.Also applies to: 65-77, 126-138, 184-196
34-44: Ref writes during render are technically unsafe under React concurrent features.Mutating
mergedRef.current,prevParentRef.current, andprevContextRef.currentduring the render phase can lead to torn reads if React discards and retries a render (e.g., withuseTransitionor Suspense). The idiomatic alternative isuseMemo:const mergedValue = React.useMemo( () => ({ ...parentContextValues, [scope]: context }), // only recompute when values actually change: [scope, context, parentContextValues] );However, this still creates a new object when
parentContextValuesis a new reference (which is the original problem). Because the realistic impact here is limited to an unnecessary re-render rather than data corruption, and this is a third-party library patch for performance, the current approach is pragmatic. Just be aware of the trade-off if concurrent features are enabled.Also applies to: 97-107
src/stores/app/loading-store.ts (1)
54-62: TheuseMemowrapper adds complexity without meaningful benefit here.The returned object is lightweight (a boolean + three stable callbacks). Wrapping it in
useMemosaves one shallow object allocation per render but introduces its own overhead and dependency array to maintain. This is optional — feel free to keep it if you prefer the pattern for consistency across the codebase.src/app/_layout.tsx (1)
57-57: Minor: remove stale commented-out code.The
//!isRunningInExpoGo()fragment is now dead. Consider removing it to keep the line clean.Suggested cleanup
- enableNativeFramesTracking: Platform.OS !== 'web', //!isRunningInExpoGo(), // Tracks slow and frozen frames in the application + enableNativeFramesTracking: Platform.OS !== 'web', // Tracks slow and frozen frames in the applicationscripts/patch-nativewind.py (1)
9-11: No error handling if the target files don't exist.If
node_moduleshasn't been installed yet or the package version doesn't include these files, the script will crash with an unhelpfulFileNotFoundError. A pre-check with a clear message would improve DX.Proposed guard
+for path in [color_scheme_path, use_color_scheme_path, api_path]: + if not os.path.isfile(path): + print(f"⚠️ Target not found, skipping patch: {path}") + sys.exit(1) + with open(color_scheme_path, 'r') as f: content = f.read()src/components/ui/drawer/index.tsx (1)
13-14:Dimensions.get('window')is captured once at module load time — won't react to resize/rotation.
screenWidthandscreenHeightare computed at import time. On web (and on rotation on native), the drawer slide distances will use stale values. Consider usinguseWindowDimensions()insideDrawerContentinstead if responsive behavior is needed.src/lib/logging/index.tsx (1)
1-10: Consider reusingisWebfromsrc/lib/platform.ts.
src/lib/platform.tsalready exportsisWebwith the samePlatform.OS === 'web'check. Reusing it would reduce duplication, unless there's a circular dependency concern with importing from platform into the logging module.src/components/bluetooth/bluetooth-audio-modal.tsx (1)
287-287: Anonymous function inonPressinside a mapped list.
onPress={() => handleConnectDevice(device)}creates a new closure on every render for each device item, which can cause unnecessary re-renders. Consider extracting a memoized device-item component that receivesdeviceandonConnectas props. As per coding guidelines, "Avoid anonymous functions in renderItem or event handlers to prevent re-renders."src/components/contacts/contact-notes-list.tsx (1)
239-241: Per-field selectors look good; consider a more targetedcontactNotesselector.The migration is correct. However, selecting the entire
contactNotesobject means this component re-renders whenever any contact's notes change. SincecontactIdis available as a prop, you could narrow the subscription:- const contactNotes = useContactsStore((s) => s.contactNotes); + const notes = useContactsStore((s) => s.contactNotes[contactId] || []);This would also simplify downstream usage (removing line 249). That said, the current approach works fine and matches the broader refactor pattern.
nginx.conf (1)
41-43: CSP uses'unsafe-eval'— verify this is required by the app bundle.
'unsafe-eval'inscript-srcsignificantly weakens XSS protection. This is sometimes needed by bundlers or libraries (e.g., some Expo Web builds), but if the app can run without it, removing it would harden the policy. The comment on Lines 41-42 mentions thatdocker-entrypoint.shcan tighten the policy — consider doing so for production.Also,
connect-src 'self' https: wss:permits connections to any origin, which effectively disables connect-src restrictions. If the set of backend/API origins is known, narrowing this would be preferable.src/stores/security/store.ts (2)
23-29:JSON.stringifyfor equality check — acceptable here but has caveats.This works for plain data objects, but be aware that property order differences or
undefinedvalues can cause false negatives. For a small rights object this is pragmatic. If the response shape ever includes dates or complex types, consider a dedicated shallow-compare utility instead.
46-58:isUserGroupAdmincreates a new closure on every call.Since
useSecurityStorereturns a newisUserGroupAdminfunction reference each timerightschanges (or even on every render if the parent re-renders), any consumer that includes it in a dependency array will see unnecessary re-triggers. Consider memoizing it or documenting that consumers should call it inline only.src/services/signalr.service.ts (1)
616-618:removeAllListenersis a clean addition for listener cleanup.This complements the pre-connection cleanup pattern in the signalR store. Consider also cleaning up in
disconnectFromHubto prevent stale listeners from accumulating if a hub is disconnected without explicit cleanup.src/components/calls/call-files-modal.tsx (1)
64-74: AnalyticsuseEffectdepends on the fullcallFilesarray reference.Unlike other tracking effects in this PR that depend on
.length, this one includescallFilesdirectly. SincefetchCallFilescreates a new array on each fetch, this effect re-fires every time files are loaded—even if the content hasn't changed. Consider usingcallFiles?.lengthif you only need the count.Proposed fix
- }, [isOpen, trackEvent, callId, callFiles, isLoadingFiles, errorFiles]); + }, [isOpen, trackEvent, callId, callFiles?.length, isLoadingFiles, errorFiles]);docker/docker-entrypoint.sh (1)
25-25:ANDROID_VERSION_CODEis hardcoded to1without an env-var override.Every other field follows the
${UNIT_X:-default}pattern for runtime configurability. If this is intentional for web-only deployments, consider adding a brief comment. Otherwise, make it configurable:Proposed fix (if configurability is desired)
- ANDROID_VERSION_CODE: 1, + ANDROID_VERSION_CODE: ${UNIT_ANDROID_VERSION_CODE:-1},And add to the Dockerfile
ENVblock:+ UNIT_ANDROID_VERSION_CODE=1 \src/components/maps/map-pins.tsx (1)
16-29: Good memoization pattern for per-pin rendering.One minor note: the
keyprop onMapbox.MarkerView(Line 23) is redundant sinceMapPinitself already receives akeyat the call site (Line 35), and there's only oneMarkerViewperMapPin. Ifidis sufficient for Mapbox's internal tracking, thekeycan be dropped from theMarkerView.src/services/location.ts (1)
382-401: Unnecessary branching andas anycast instopLocationUpdates.Since the web shim at line 182 is already cast to
Location.LocationSubscription, callingawait this.locationSubscription.remove()works for both web (syncremove—awaiton a non-Promise resolves immediately) and native. The explicit web/native branching andas anycast can be removed. As per coding guidelines,anyshould be avoided.♻️ Simplify by removing the platform branch
async stopLocationUpdates(): Promise<void> { if (this.locationSubscription) { - if (isWeb) { - // On web the subscription is our own shim wrapping clearWatch - (this.locationSubscription as any).remove(); - } else { - await this.locationSubscription.remove(); - } + await this.locationSubscription.remove(); this.locationSubscription = null; } - if (!isWeb) { - await this.stopBackgroundUpdates(); + if (!isWeb) { + await this.stopBackgroundUpdates(); - // Check if task is registered before stopping - const isTaskRegistered = await TaskManager.isTaskRegisteredAsync(LOCATION_TASK_NAME); - if (isTaskRegistered) { - await Location.stopLocationUpdatesAsync(LOCATION_TASK_NAME); - } + // Check if task is registered before stopping + const isTaskRegistered = await TaskManager.isTaskRegisteredAsync(LOCATION_TASK_NAME); + if (isTaskRegistered) { + await Location.stopLocationUpdatesAsync(LOCATION_TASK_NAME); + } }src/stores/signalr/signalr-store.ts (2)
54-57: Good: Pre-connection listener cleanup prevents handler accumulation.This correctly removes stale listeners before re-registering, preventing duplicate handlers across reconnections.
However, Biome flags the
forEachcallback becauseremoveAllListenersreturns a value. Use a block body to suppress the lint warning.🧹 Suppress Biome lint warning
- updateEvents.forEach((event) => signalRService.removeAllListeners(event)); + updateEvents.forEach((event) => { signalRService.removeAllListeners(event); });
143-145: SameforEachreturn-value lint issue as with update events.🧹 Suppress Biome lint warning
- geoEvents.forEach((event) => signalRService.removeAllListeners(event)); + geoEvents.forEach((event) => { signalRService.removeAllListeners(event); });src/services/bluetooth-audio.service.ts (1)
66-73: Inconsistent web guard: usesPlatform.OS === 'web'instead ofthis.isWeb.All other web guards in this class use the
this.isWebfield (Line 49). This one checksPlatform.OS === 'web'directly, which is functionally equivalent but inconsistent.♻️ Proposed fix
- if (Platform.OS === 'web') { + if (this.isWeb) {src/components/status/status-bottom-sheet.tsx (1)
70-76: Consider batching location store selectors withuseShallow.Seven individual selectors on
useLocationStoremeans seven independent subscriptions. Since the location store sets all these properties in a singleset()call viasetLocation, each update will trigger up to seven re-renders of this component (one per property change notification). SinceuseShallowis already imported (though currently unused), you could batch these into a single selector:♻️ Proposed alternative using useShallow
- const latitude = useLocationStore((state) => state.latitude); - const longitude = useLocationStore((state) => state.longitude); - const heading = useLocationStore((state) => state.heading); - const accuracy = useLocationStore((state) => state.accuracy); - const speed = useLocationStore((state) => state.speed); - const altitude = useLocationStore((state) => state.altitude); - const timestamp = useLocationStore((state) => state.timestamp); + const { latitude, longitude, heading, accuracy, speed, altitude, timestamp } = useLocationStore( + useShallow((state) => ({ + latitude: state.latitude, + longitude: state.longitude, + heading: state.heading, + accuracy: state.accuracy, + speed: state.speed, + altitude: state.altitude, + timestamp: state.timestamp, + })) + );src/components/maps/map-view.web.tsx (1)
323-329: Comment says "layout effect" but this is a regularuseEffect.The comment on Line 324 says "Using a layout effect" but the code uses
useEffect, notuseLayoutEffect. For rendering into an external DOM node (mapbox-gl marker),useEffectis generally fine, but the comment should be corrected to avoid confusion.📝 Proposed fix
- // Render children into the marker's React root whenever children identity changes. - // Using a layout effect with [children] dep so it only fires when children actually change. + // Render children into the marker's React root whenever children identity changes. useEffect(() => {src/stores/app/livekit-store.ts (1)
412-437: Mute sync condition is correct but hard to follow — consider inverting for clarity.The condition
isMicrophoneEnabled === mutedis logically correct (it detects a mismatch between mic state and CallKeep muted state), but the inline comments spanning 8 lines suggest it's confusing. A simpler expression would improve readability:♻️ Suggested simplification
- if (room.localParticipant.isMicrophoneEnabled === muted) { - // If CallKeep says "muted" (true), and Mic is enabled (true), we need to disable mic. - // If CallKeep says "unmuted" (false), and Mic is disabled (false), we need to enable mic. - // Wait, logic check: - // isMicrophoneEnabled = true means NOT MUTED. - // muted = true means MUTED. - // So if isMicrophoneEnabled (true) and muted (true) -> mismatch, we must mute. - // if isMicrophoneEnabled (false) and muted (false) -> mismatch, we must unmute. - - // Actually effectively: setMicrophoneEnabled(!muted) - await room.localParticipant.setMicrophoneEnabled(!muted); + // Sync mic state: mic enabled means "not muted", so they're out of sync + // when isMicrophoneEnabled equals the muted flag. + const isOutOfSync = room.localParticipant.isMicrophoneEnabled === muted; + if (isOutOfSync) { + await room.localParticipant.setMicrophoneEnabled(!muted);src/app/(app)/index.tsx (1)
388-389: Prefer ternary? :over&&for conditional rendering.Per coding guidelines, use the conditional operator
? :instead of&&for conditional rendering. This applies here and at line 399 (locationHeading !== null && ...).As per coding guidelines: "Use ternary operator
? :for conditional rendering instead of&&".♻️ Example for the outer conditional
- {locationLatitude && locationLongitude && ( - <Mapbox.PointAnnotation ...> + {locationLatitude && locationLongitude ? ( + <Mapbox.PointAnnotation ...> ... </Mapbox.PointAnnotation> - )} + ) : null}src/app/(app)/_layout.tsx (3)
300-305: Use lucide-react-native icons directly instead of the gluestack-uiIconwrapper.These icon renderers use
<Icon as={Map} stroke={color} ...>which wraps lucide icons in the gluestack-uiIconcomponent. As per coding guidelines: "Use lucide-react-native for icons directly in markup; do not use the gluestack-ui icon component."♻️ Example fix for one icon
- const mapIcon = useCallback(({ color }: { color: string }) => <Icon as={Map} stroke={color} className="text-primary-500 dark:text-primary-400" />, []); + const mapIcon = useCallback(({ color }: { color: string }) => <Map size={24} color={color} />, []);
243-260: Ref mutation during render — consider moving to an effect or memoization.Updating
lastNovuConfig.currentdirectly during render (lines 253–260) is a side effect that React's concurrent mode documentation advises against. While it's practically safe here because the value is derived deterministically from state, it could cause subtle issues with concurrent features in React 19.♻️ Alternative: derive via useMemo
- const lastNovuConfig = useRef<{...} | null>(null); - if (novuReady) { - lastNovuConfig.current = { ... }; - } + const novuConfig = useMemo(() => { + if (!novuReady) return null; + return { + subscriberId: `${rights?.DepartmentCode}_Unit_${activeUnitId}`, + applicationIdentifier: config!.NovuApplicationId, + backendUrl: config!.NovuBackendApiUrl, + socketUrl: config!.NovuSocketUrl, + }; + }, [novuReady, rights?.DepartmentCode, activeUnitId, config]); + + // Cache last known-good config so provider stays mounted during transient nulls + const lastNovuConfig = useRef(novuConfig); + if (novuConfig) lastNovuConfig.current = novuConfig;Note: The ref update still happens during render in this refactor, but the
useMemomakes the derivation explicit and cacheable. If full purity is needed, move the ref update into auseEffect.
456-456: Prefer ternary? :over&&for conditional rendering.As per coding guidelines: "Use ternary operator
? :for conditional rendering instead of&&".♻️ Suggested fix
- {isInitComplete && novuReady && <NotificationInbox isOpen={isNotificationsOpen} onClose={handleCloseNotifications} />} + {isInitComplete && novuReady ? <NotificationInbox isOpen={isNotificationsOpen} onClose={handleCloseNotifications} /> : null}metro.config.js (1)
62-78: Document zustand version dependency for path-based module resolution.The hard-coded CJS paths assume zustand 4.5.5's export structure (e.g.,
node_modules/zustand/index.js). While this version does publish CJS at these paths via conditional exports, future versions (e.g., zustand 5.x) could reorganize their exports and silently break this resolver. Add a comment noting the supported zustand version constraint or consider validating these paths exist at build time to catch version mismatches early.
| import { useLocationStore } from '@/stores/app/location-store'; | ||
| import { useCallDetailStore } from '@/stores/calls/detail-store'; | ||
| import { useSecurityStore } from '@/stores/security/store'; | ||
| import { securityStore } from '@/stores/security/store'; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the store definition
cat src/stores/security/store.ts | head -30Repository: Resgrid/Unit
Length of output: 1064
🏁 Script executed:
# Check how other stores are defined and used
fd -t f "store.ts" src/stores --exec head -20 {} +Repository: Resgrid/Unit
Length of output: 18582
🏁 Script executed:
# Look for eslint configurations
fd -t f "eslint" . -o fd -t f ".eslintrc*" .Repository: Resgrid/Unit
Length of output: 179
🏁 Script executed:
# Check how securityStore is imported and used across the codebase
rg "securityStore" --type=ts --type=tsx -B2 -A2Repository: Resgrid/Unit
Length of output: 83
🏁 Script executed:
# Check for other store imports with 'use' prefix
rg "import.*use.*Store.*from.*stores" --type=ts --type=tsxRepository: Resgrid/Unit
Length of output: 83
🏁 Script executed:
cat src/stores/security/store.tsRepository: Resgrid/Unit
Length of output: 1958
🏁 Script executed:
# Check the specific import in the file being reviewed
grep -n "import.*securityStore\|import.*useSecurityStore" src/app/call/\[id\].tsxRepository: Resgrid/Unit
Length of output: 115
🏁 Script executed:
# Verify if useSecurityStore is exported and how it's used elsewhere
rg "useSecurityStore|securityStore" src/stores/security/store.ts -A2 -B2Repository: Resgrid/Unit
Length of output: 338
🏁 Script executed:
# Check signalr-store to see how it imports both
cat src/stores/signalr/signalr-store.ts | grep -A5 "securityStore\|useSecurityStore"Repository: Resgrid/Unit
Length of output: 550
🏁 Script executed:
# Check how securityStore is used in [id].tsx around line 61
sed -n '50,75p' src/app/call/\[id\].tsxRepository: Resgrid/Unit
Length of output: 1566
securityStore should follow React hook naming conventions.
Line 61 calls securityStore as a React hook, but it lacks the use prefix that is consistent with other store hooks in this file (useCallDetailStore, useCoreStore, useStatusBottomSheetStore, useToastStore). This inconsistency will trigger react-hooks/rules-of-hooks ESLint warnings.
Import the store with a use prefix alias:
import { securityStore as useSecurityStore } from '@/stores/security/store';Then use it as:
const canUserCreateCalls = useSecurityStore((state) => state.rights?.CanCreateCalls);This aligns with the codebase convention of prefixing store hooks with use.
🤖 Prompt for AI Agents
In `@src/app/call/`[id].tsx at line 28, The import securityStore is used like a
React hook but lacks the conventional "use" prefix and triggers react-hooks
rules; update the import to alias securityStore as useSecurityStore (so it's
consistent with other hooks like useCallDetailStore/useCoreStore), then replace
calls to securityStore(...) with useSecurityStore(...) such as where const
canUserCreateCalls is derived, ensuring the hook naming convention is followed
across the file.
| <VStack space="md" className="w-full flex-1 p-2"> | ||
| {/* First row - Two cards side by side */} | ||
| <HStack space="md"> | ||
| <SidebarUnitCard unitName="No Unit" unitType="" unitGroup={t('common.no_unit_selected')} bgColor="bg-background-50" /> |
There was a problem hiding this comment.
Hardcoded "No Unit" string should be wrapped in t().
All user-facing text must use t() for internationalization. As per coding guidelines: "Ensure all text is wrapped in t() from react-i18next for translations."
Proposed fix
- <SidebarUnitCard unitName="No Unit" unitType="" unitGroup={t('common.no_unit_selected')} bgColor="bg-background-50" />
+ <SidebarUnitCard unitName={t('common.no_unit')} unitType="" unitGroup={t('common.no_unit_selected')} bgColor="bg-background-50" />📝 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.
| <SidebarUnitCard unitName="No Unit" unitType="" unitGroup={t('common.no_unit_selected')} bgColor="bg-background-50" /> | |
| <SidebarUnitCard unitName={t('common.no_unit')} unitType="" unitGroup={t('common.no_unit_selected')} bgColor="bg-background-50" /> |
🤖 Prompt for AI Agents
In `@src/components/sidebar/sidebar-content.tsx` at line 38, Replace the hardcoded
unitName string with a translated value: change the SidebarUnitCard prop
unitName="No Unit" to use t(...) (e.g., unitName={t('common.no_unit')} or the
appropriate key), ensuring the t function from react-i18next is in scope; update
the unitName prop usage in the SidebarUnitCard component invocation so all
user-facing text is wrapped with t().
| const Countly = { | ||
| CountlyConfig, | ||
| init: async () => {}, | ||
| initWithConfig: async (_config?: any) => {}, |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Avoid any — use CountlyConfig type.
CountlyConfig is defined in the same file; use it instead of any.
- initWithConfig: async (_config?: any) => {},
+ initWithConfig: async (_config?: CountlyConfig) => {},As per coding guidelines, "Avoid using any; strive for precise types".
📝 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.
| initWithConfig: async (_config?: any) => {}, | |
| initWithConfig: async (_config?: CountlyConfig) => {}, |
🤖 Prompt for AI Agents
In `@src/lib/countly-shim.web.ts` at line 18, The initWithConfig function
currently types its parameter as any; change its signature to use the existing
CountlyConfig type (preserving optionality if needed) so the parameter becomes
(_config?: CountlyConfig) and update any internal references to rely on the
typed shape; refer to the initWithConfig method and the CountlyConfig type in
this file when making the change.
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (19)
src/components/status/__tests__/status-gps-integration-working.test.tsx (1)
1-95:⚠️ Potential issue | 🟡 MinorCorrupted block comment — likely a merge or encoding artifact.
Lines 1–95 contain garbled, overlapping text fragments (e.g.,
expect.objec expect(mockSaveUni expect(mockSaveUnitStatus)...). While it's wrapped in a/* */comment so it won't break at runtime, this is clearly corrupted content rather than intentional documentation. It should be replaced with a clean descriptive comment or removed entirely.src/components/settings/__tests__/unit-selection-bottom-sheet.test.tsx (3)
210-217:⚠️ Potential issue | 🟡 MinorToast store mock always invokes
selector(state)without a type guard.Unlike the other store mocks that check
typeof selector === 'function', the toast store mock unconditionally callsselector(state). If the component (or any future code path) ever callsuseToastStore()without a selector, this will throw aTypeError: selector is not a function.🛡️ Proposed fix
mockUseToastStore.mockImplementation((selector: any) => { const state = { showToast: mockShowToast, toasts: [], removeToast: jest.fn(), }; - return selector(state); + return typeof selector === 'function' ? selector(state) : state; });
331-338:⚠️ Potential issue | 🟡 MinorSame inconsistency in
beforeEachtoast mock — apply the same guard.This is the second instance of the toast store mock that unconditionally invokes
selector(state).🛡️ Proposed fix
mockUseToastStore.mockImplementation((selector: any) => { const state = { showToast: mockShowToast, toasts: [], removeToast: jest.fn(), }; - return selector(state); + return typeof selector === 'function' ? selector(state) : state; });
538-541:⚠️ Potential issue | 🟡 MinorTiming-dependent assertion may cause flaky tests.
await new Promise(resolve => setTimeout(resolve, 50))followed by a synchronous assertion is inherently racy — CI under load could make this pass or fail nondeterministically. Consider wrapping the expectation inwaitForinstead, or restructuring the test to avoid relying on wall-clock timing.src/components/roles/__tests__/roles-bottom-sheet.test.tsx (2)
434-496:⚠️ Potential issue | 🟠 MajorTests duplicate component internals instead of exercising the actual component.
TestComponentre-implements the save/filtering logic fromRolesBottomSheet. This means the test validates a copy of the logic — if the real component's filter changes (or regresses), these tests still pass. Instead, render<RolesBottomSheet>, interact with its Save button viafireEvent, and assert onmockAssignRolescalls. That way you're testing the actual code path.The same concern applies to the whitespace-filtering test at Lines 550–584.
24-29:⚠️ Potential issue | 🟡 MinorMock uses
<div>instead of a React Native element.In a React Native testing environment,
<div>is not a valid host component. UseViewfromreact-nativeto avoid potential warnings or failures depending on the test renderer configuration.Proposed fix
jest.mock('@/components/ui/bottom-sheet', () => ({ - CustomBottomSheet: ({ children, isOpen }: any) => { + CustomBottomSheet: ({ children, isOpen }: { children: React.ReactNode; isOpen: boolean }) => { + const { View } = require('react-native'); if (!isOpen) return null; - return <div>{children}</div>; + return <View>{children}</View>; }, }));src/components/roles/__tests__/roles-modal.test.tsx (1)
283-291:⚠️ Potential issue | 🟡 MinorUnused
fireEventimport.
fireEventis required on line 284 but never used in this test case. Remove it to avoid confusion.Proposed fix
it('should filter out roles with empty or whitespace RoleId and UserId', () => { - const { fireEvent } = require('@testing-library/react-native'); - render(<RolesModal isOpen={true} onClose={mockOnClose} />);src/components/status/status-bottom-sheet.tsx (2)
538-538:⚠️ Potential issue | 🟡 MinorRedundant ternary — both branches produce the same value.
colorScheme === 'dark' ? '#fff' : '#fff'always evaluates to'#fff'. Same pattern on Line 648 and Line 658 with'#737373'. Simplify to a plain string.Proposed fix
- <ArrowRight size={14} color={colorScheme === 'dark' ? '#fff' : '#fff'} /> + <ArrowRight size={14} color="#fff" />Apply the same simplification at lines 648, 658, and 724.
499-501:⚠️ Potential issue | 🟡 MinorHard-coded
#fffffffallback ignores dark mode.When
status.BColoris falsy and the item is not selected, the background is forced to white (#ffffff), which will look wrong in dark mode. Consider using a theme-aware fallback (e.g.,colorScheme === 'dark' ? '#1f2937' : '#ffffff') or move the fallback to a className-based style so NativeWind's dark mode applies.src/app/(app)/_layout.tsx (1)
149-159:⚠️ Potential issue | 🟠 Major
isInitCompleteis set totrueeven when initialization fails.When
initializeAppthrows,hasInitializedis correctly reset tofalse, butisInitCompleteis unconditionally set totruein thefinallyblock. This removes the loading overlay and renders the drawer/sidebar/notification inbox against potentially uninitialized stores, which could cause runtime errors or a broken UI state.Consider setting
isInitCompletetotrueonly on success, or introducing a separate error state to show a retry UI.Proposed fix
} catch (error) { logger.error({ message: 'Failed to initialize app', context: { error }, }); // Reset initialization state on error so it can be retried hasInitialized.current = false; } finally { isInitializing.current = false; - setIsInitComplete(true); } + + // Only reveal the full UI after successful init + setIsInitComplete(true);src/components/status/__tests__/status-gps-integration.test.tsx (1)
59-62:⚠️ Potential issue | 🟡 MinorDebug
console.logleft in test setup.Line 60 logs every
useLocationStorecall during test execution. This adds noise to test output and appears to be a leftover debugging artifact.Proposed fix
mockUseLocationStore.mockImplementation(() => { - console.log('useLocationStore called, returning:', mockLocationStore); return mockLocationStore; });src/components/status/__tests__/status-gps-debug.test.tsx (1)
64-186:⚠️ Potential issue | 🟡 MinorThis entire file appears to be a debugging/diagnostic test, not a production test.
The test contains 10+
console.logcalls (lines 151–183), an unconditionalscreen.debug()(line 174), and its sole purpose is to verify a Submit button renders. All of this diagnostic output will pollute CI logs on every run. If the underlying issue has been resolved, consider removing this file entirely; otherwise, strip the debug statements and convert it into a proper focused test.src/components/livekit/livekit-bottom-sheet.tsx (1)
58-73:⚠️ Potential issue | 🟠 MajorTelemetry effect fires on every dependency change, not just on open.
The dependency array includes
isConnected,isConnecting,currentView,isMuted,isTalking, etc. This means thelivekit_bottom_sheet_openedevent will be re-emitted every time any of these values change while the sheet is visible — not just when the sheet first opens. This can flood analytics with duplicate or misleading events.Consider tracking only when
isBottomSheetVisibletransitions totrue, using a ref to detect the transition:Proposed fix
+ const wasVisibleRef = useRef(false); + useEffect(() => { - if (isBottomSheetVisible) { + if (isBottomSheetVisible && !wasVisibleRef.current) { trackEvent('livekit_bottom_sheet_opened', { availableRoomsCount: availableRooms.length, isConnected: isConnected, isConnecting: isConnecting, currentView: currentView, hasCurrentRoom: !!currentRoomInfo, currentRoomName: currentRoomInfo?.Name || 'none', isMuted: isMuted, isTalking: isTalking, hasBluetoothMicrophone: selectedAudioDevices?.microphone?.type === 'bluetooth', hasBluetoothSpeaker: selectedAudioDevices?.speaker?.type === 'bluetooth', }); } + wasVisibleRef.current = isBottomSheetVisible; }, [isBottomSheetVisible, trackEvent, availableRooms.length, isConnected, isConnecting, currentView, currentRoomInfo, isMuted, isTalking, selectedAudioDevices?.microphone?.type, selectedAudioDevices?.speaker?.type]);src/stores/app/livekit-store.ts (1)
246-336:⚠️ Potential issue | 🟡 MinorHardcoded English strings in Alert dialogs violate i18n guidelines.
Lines 306-308, 315, and 324 contain user-facing text that is not wrapped in
t()fromreact-i18next. As per coding guidelines: "Wrap all user-facing text int()fromreact-i18next".Proposed fix (example for one Alert)
- Alert.alert( - 'Voice Permissions Required', - 'Phone state permission was permanently denied. Voice functionality may not work correctly. Please open Settings and grant the Phone permission for this app.', - [ - { text: 'Cancel', style: 'cancel' }, - { text: 'Open Settings', onPress: () => Linking.openSettings() }, - ] - ); + Alert.alert( + t('permissions.voice_permissions_required'), + t('permissions.phone_state_permanently_denied'), + [ + { text: t('common.cancel'), style: 'cancel' }, + { text: t('permissions.open_settings'), onPress: () => Linking.openSettings() }, + ] + );Apply the same pattern to the other two Alert calls on Lines 315 and 324.
src/components/livekit/__tests__/livekit-bottom-sheet.test.tsx (1)
568-581:⚠️ Potential issue | 🟡 MinorThis test validates the mock, not the component's behavior.
The test directly calls
audioService.playConnectionSound()and asserts it was called — it doesn't verify thatLiveKitBottomSheetinvokes the audio service under the right conditions. This test provides false confidence; consider replacing it with a test that triggers a user action (e.g., mute toggle) and asserts the component calls the expected audio service method.src/components/maps/map-view.web.tsx (1)
294-300:⚠️ Potential issue | 🟠 MajorAnchor offset calculation reads zero-sized container.
At this point in the effect,
createRoothas been called butroot.render(children)hasn't executed yet (it runs in the separate effect on Line 332). The container has no content andgetBoundingClientRect()will return{ width: 0, height: 0 }, so the offset will always be[0, 0]regardless of theanchorprop.To fix this, the offset needs to be computed after children are rendered into the container. One approach is to defer the offset calculation into the children-rendering effect (or use a
ResizeObserver/requestAnimationFrameafter the first render).Suggested approach: compute offset after children render
Move the offset logic into the children-rendering effect (lines 332–336) or a dedicated post-render callback:
// Render children into the marker's React root whenever children identity changes. useEffect(() => { if (containerRootRef.current && children) { containerRootRef.current.render(<>{children}</>); + + // Recalculate anchor offset after children are rendered + if (typeof anchor === 'object' && anchor !== null && 'x' in anchor && 'y' in anchor && markerRef.current && containerRef.current) { + requestAnimationFrame(() => { + const rect = containerRef.current?.getBoundingClientRect(); + if (rect && markerRef.current) { + const xOffset = (anchor.x - 0.5) * rect.width; + const yOffset = (anchor.y - 0.5) * rect.height; + markerRef.current.setOffset([xOffset, yOffset]); + } + }); + } } - }, [children]); + }, [children, anchor]);src/features/livekit-call/store/__tests__/useLiveKitCallStore.test.ts (1)
507-554:⚠️ Potential issue | 🟡 MinorDuplicate nested
describe('Error Handling')and duplicate test case.There's a
describe('Error Handling')at line 507 containing anotherdescribe('Error Handling')at line 508, andit('should handle basic error state management')appears twice (lines 530–541 and 543–553) with identical logic. This looks like a copy-paste mistake.🐛 Proposed fix — remove the duplicate nesting
describe('Error Handling', () => { - describe('Error Handling', () => { it('should handle room initialization errors', async () => { // ...existing test... }); it('should handle basic error state management', async () => { // ...existing test... }); - }); - - it('should handle basic error state management', async () => { - const { result } = renderHook(() => useLiveKitCallStore()); - - // Test basic error clearing functionality since token fetching isn't implemented - act(() => { - // Set an error state and then clear it - result.current.actions._clearError(); - }); - - expect(result.current.error).toBeNull(); - }); });src/app/(app)/__tests__/calls.test.tsx (1)
263-273:⚠️ Potential issue | 🟡 MinorMultiple tests contain only
expect(true).toBe(true)— they validate nothing.Tests at lines 263, 310, 322, 333, 347, 360, and 373 all rely on
expect(true).toBe(true)as their primary assertion. These pass unconditionally and don't verify any component behavior. While the comments explain query limitations with HTML mocks, these are effectively dead tests.Consider either:
- Querying the rendered output to make real assertions (e.g.,
screen.queryByTestId,toJSON()snapshots)- Marking them as
.todotests until proper assertions can be addedsrc/app/(app)/index.tsx (1)
392-411:⚠️ Potential issue | 🟠 Major
&&with numericlocationLatitude/locationLongitudecan crash React NativeIn React Native,
{0 && <View />}attempts to render0as a bare text node, which throws "Text strings must be rendered within a <Text> component". SincelocationLatitudeandlocationLongitudeare numbers, if either is exactly0the short-circuit evaluates to0and RN crashes.This affects:
- Line 392:
{locationLatitude && locationLongitude && (<Mapbox.PointAnnotation …>…</Mapbox.PointAnnotation>)}- Line 314:
showRecenterButtonresolves tolocationLongitude(a number) when all preceding terms are truthy, then used in{showRecenterButton && (…)}at line 416.Per coding guidelines, prefer the ternary operator (
? :) for conditional rendering instead of&&.Proposed fix — use explicit boolean coercion or null checks
- const showRecenterButton = !isMapLocked && hasUserMovedMap && locationLatitude && locationLongitude; + const showRecenterButton = !isMapLocked && hasUserMovedMap && locationLatitude != null && locationLongitude != null;- {locationLatitude && locationLongitude && ( - <Mapbox.PointAnnotation id="userLocation" coordinate={[locationLongitude, locationLatitude]} anchor={{ x: 0.5, y: 0.5 }}> + {locationLatitude != null && locationLongitude != null ? ( + <Mapbox.PointAnnotation id="userLocation" coordinate={[locationLongitude, locationLatitude]} anchor={{ x: 0.5, y: 0.5 }}> ... - </Mapbox.PointAnnotation> - )} + </Mapbox.PointAnnotation> + ) : null}Apply the same
!= nullpattern to other guard expressions on lines 81, 137, 169, and 259.As per coding guidelines: "Use ternary operator
? :for conditional rendering instead of&&".Also applies to: 314-314
🤖 Fix all issues with AI agents
In `@electron/main.js`:
- Around line 185-212: The handler using path.resolve(distPath,
decodeURIComponent(url.pathname)) is wrong because an absolute pathname resets
the base so every request escapes distPath; change to first join the base and
request (use path.join(distPath, decodeURIComponent(url.pathname))) then
canonicalize (path.resolve or path.normalize) into resolvedPath, keep the same
security check against resolvedDist (and path.sep) and the fallback to
index.html; update references in protocol.handle('app') where resolvedPath,
filePath, distPath, resolvedDist, and url.pathname are used.
In `@src/app/`(app)/index.tsx:
- Around line 219-236: The Animated.loop started inside the useEffect (using
pulseAnim) never gets stopped, so capture the loop animation in a variable
(e.g., loopAnim = Animated.loop(...)) before calling .start(), and return a
cleanup function from the useEffect that calls loopAnim.stop() to halt the
animation when the component (MapContent) unmounts; keep the Platform.OS !==
'web' guard and the same Animated.sequence/timing configuration but ensure you
stop the same loopAnim instance in the cleanup.
In `@src/components/maps/map-view.web.tsx`:
- Around line 318-320: The dependency array for the effect that recreates the
marker is missing the "anchor" dependency referenced in the comment; update the
effect in map-view.web.tsx (the closure that uses map, id, and
markerOptions.anchor) to include anchor in the dependency array so the marker is
recreated when anchor changes, or if you intend not to trigger on anchor changes
then update the comment to remove "anchor" — modify the effect surrounding map
and id (the one that creates/updates the marker) to either add anchor to [map,
id] or correct the comment to match the actual dependencies.
- Around line 330-336: Update the misleading comment and ensure the marker is
cleared when children becomes falsy: change the comment that currently claims
"Using a layout effect" to accurately say "Using useEffect" (or similar) and
modify the effect that uses useEffect so that it always calls
containerRootRef.current.render(...) — rendering children when present and
rendering an empty fragment (or null-equivalent) when children is falsy — so
stale content is cleared; target the useEffect block around
containerRootRef.current.render(...) and the children variable in
map-view.web.tsx.
- Around line 9-10: Remove the unnecessary TypeScript suppression above the
import of createRoot: delete the "// `@ts-ignore` - react-dom/client types may not
be available" comment so the import line "import { createRoot } from
'react-dom/client';" uses the built-in React 19 type definitions without
silencing the compiler.
In `@src/components/push-notification/__tests__/push-notification-modal.test.tsx`:
- Line 111: The test mocks duplicate store objects in each mockImplementation
causing potential drift; instead create a single shared mock store object and
reuse it in the mock for usePushNotificationModalStore (and the other spots) so
both branches return the exact same object; implement a small helper (e.g.,
sharedMockStore or getMockedStore) and update the mockImplementation calls to
return that shared object when selector is a function or as the fallback,
referencing usePushNotificationModalStore, mockImplementation, and mockStore to
locate where to change.
In `@src/components/settings/bluetooth-device-selection-bottom-sheet.tsx`:
- Around line 280-281: The two hardcoded user-facing strings inside the Text
components (the label that checks preferredDevice?.id === 'system-audio' and the
subtitle "AirPods, Car, Wired Headset") must be wrapped with the i18n translator
t(); update the Text nodes in bluetooth-device-selection-bottom-sheet.tsx so the
primary label uses t('System Audio') (or an appropriate key) where
preferredDevice?.id === 'system-audio' is checked, and the secondary description
uses t('AirPods, Car, Wired Headset') (or a chosen translation key), ensuring
you import and use the t function from react-i18next in that component.
In `@src/components/sidebar/__tests__/roles-sidebar.test.tsx`:
- Line 243: The mock for useCoreStore is passing newUnit directly so
selector(state => state.activeUnit) receives a UnitResultData and returns
undefined; update the mockUseCoreStore implementation used in this test to pass
an object with activeUnit: newUnit (i.e.,
mockUseCoreStore.mockImplementation((selector: any) => typeof selector ===
'function' ? selector({ activeUnit: newUnit }) : { activeUnit: newUnit })),
ensuring the selector in the component (useCoreStore((state) =>
state.activeUnit)) sees the intended activeUnit value and the test exercises the
intended branch.
In `@src/components/sidebar/__tests__/unit-sidebar.test.tsx`:
- Line 59: The mock for useCoreStore is calling selector(...) unconditionally
which will throw if the hook is used without a selector; update
mockUseCoreStore.mockImplementation to mirror the other store mocks by checking
typeof selector === 'function' and returning selector({ activeUnit: null } as
any) only when it's a function, otherwise return the state object ({ activeUnit:
null } as any); reference mockUseCoreStore and the selector guard pattern used
elsewhere in this test file.
In `@src/services/location.ts`:
- Around line 156-185: The web branch silently no-ops when 'geolocation' is
missing and lacks a success log; update the isWeb path in start/subscribe logic
to (1) emit a logger.warn (or logger.error) when 'geolocation' is not present so
callers can diagnose unsupported environments, and (2) after successfully
creating the watch with navigator.geolocation.watchPosition and assigning
this.locationSubscription, call logger.info with a message like "Foreground
location updates started" (matching the native path) so successful subscription
is observable; use the existing this.locationSubscription, watchId, and logger
symbols to implement these logs.
- Line 13: The file defines a duplicate const isWeb (const isWeb = Platform.OS
=== 'web') even though isWeb is already exported; remove the local declaration
and instead import the shared exported symbol (isWeb) from the module that
provides it, replace usages to reference the imported isWeb, and remove any
now-unused Platform import to avoid duplication and dead code.
| // @ts-ignore - react-dom/client types may not be available | ||
| import { createRoot } from 'react-dom/client'; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check package.json for `@types/react-dom` version and React version
fd "package.json" -t f --max-depth 2 -x cat {} | jq '{react: .dependencies.react // .devDependencies.react, "react-dom": .dependencies["react-dom"] // .devDependencies["react-dom"], "@types/react-dom": .dependencies["@types/react-dom"] // .devDependencies["@types/react-dom"], "@types/react": .dependencies["@types/react"] // .devDependencies["@types/react"]}'Repository: Resgrid/Unit
Length of output: 2308
🏁 Script executed:
#!/bin/bash
# Read the actual file around lines 9-10
head -20 src/components/maps/map-view.web.tsxRepository: Resgrid/Unit
Length of output: 743
🏁 Script executed:
#!/bin/bash
# Check if tsconfig.json exists and examine type checking settings
fd "tsconfig.json" -t f --max-depth 2 -x cat {}Repository: Resgrid/Unit
Length of output: 1146
🏁 Script executed:
#!/bin/bash
# Read package.json and extract dependency versions
cat package.json | jq '.dependencies, .devDependencies' | grep -E '(react|@types/react)'Repository: Resgrid/Unit
Length of output: 1826
🏁 Script executed:
#!/bin/bash
# Also try extracting specific versions more directly
cat package.json | jq -r '.dependencies["react-dom"], .devDependencies["react-dom"], .dependencies["@types/react-dom"], .devDependencies["@types/react-dom"]'Repository: Resgrid/Unit
Length of output: 77
Remove unnecessary @ts-ignore for react-dom/client.
React 19.0.0 includes bundled TypeScript definitions for react-dom/client and does not require the @types/react-dom package. The @ts-ignore suppression can be safely removed.
🤖 Prompt for AI Agents
In `@src/components/maps/map-view.web.tsx` around lines 9 - 10, Remove the
unnecessary TypeScript suppression above the import of createRoot: delete the
"// `@ts-ignore` - react-dom/client types may not be available" comment so the
import line "import { createRoot } from 'react-dom/client';" uses the built-in
React 19 type definitions without silencing the compiler.
| // Only recreate marker when map, id, or anchor change — NOT children | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, [map, coordinate, id, children, anchor, onSelected, title]); | ||
| }, [map, id]); |
There was a problem hiding this comment.
Comment says anchor is a dependency but the array omits it.
The comment on Line 318 states "Only recreate marker when map, id, or anchor change" but the dependency array is [map, id]. If anchor type changes from object to string (or vice versa), the marker won't be recreated and the markerOptions.anchor won't be updated. Either add anchor to the dependency array or correct the comment.
Proposed fix
- // Only recreate marker when map, id, or anchor change — NOT children
// eslint-disable-next-line react-hooks/exhaustive-deps
- }, [map, id]);
+ // Only recreate marker when map, id, or anchor change — NOT children
+ // eslint-disable-next-line react-hooks/exhaustive-deps
+ }, [map, id, anchor]);📝 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.
| // Only recreate marker when map, id, or anchor change — NOT children | |
| // eslint-disable-next-line react-hooks/exhaustive-deps | |
| }, [map, coordinate, id, children, anchor, onSelected, title]); | |
| }, [map, id]); | |
| // Only recreate marker when map, id, or anchor change — NOT children | |
| // eslint-disable-next-line react-hooks/exhaustive-deps | |
| }, [map, id, anchor]); |
🤖 Prompt for AI Agents
In `@src/components/maps/map-view.web.tsx` around lines 318 - 320, The dependency
array for the effect that recreates the marker is missing the "anchor"
dependency referenced in the comment; update the effect in map-view.web.tsx (the
closure that uses map, id, and markerOptions.anchor) to include anchor in the
dependency array so the marker is recreated when anchor changes, or if you
intend not to trigger on anchor changes then update the comment to remove
"anchor" — modify the effect surrounding map and id (the one that
creates/updates the marker) to either add anchor to [map, id] or correct the
comment to match the actual dependencies.
| // Render children into the marker's React root whenever children identity changes. | ||
| // Using a layout effect with [children] dep so it only fires when children actually change. | ||
| useEffect(() => { | ||
| if (containerRootRef.current && children) { | ||
| containerRootRef.current.render(<>{children}</>); | ||
| } | ||
| }, [children]); |
There was a problem hiding this comment.
Two minor concerns with the children-rendering effect.
-
Misleading comment: Line 331 says "Using a layout effect" but the code uses
useEffect, notuseLayoutEffect. Update the comment to match. -
Stale children when
childrenbecomes falsy: The guardif (... && children)means that if children transitions from a valid element tonull/undefined, the last rendered content will remain visible in the marker. Consider rendering an empty fragment when children is falsy to clear the marker content.
Proposed fix
- // Render children into the marker's React root whenever children identity changes.
- // Using a layout effect with [children] dep so it only fires when children actually change.
+ // Render children into the marker's React root whenever children change.
useEffect(() => {
- if (containerRootRef.current && children) {
- containerRootRef.current.render(<>{children}</>);
+ if (containerRootRef.current) {
+ containerRootRef.current.render(<>{children}</>);
}
}, [children]);🤖 Prompt for AI Agents
In `@src/components/maps/map-view.web.tsx` around lines 330 - 336, Update the
misleading comment and ensure the marker is cleared when children becomes falsy:
change the comment that currently claims "Using a layout effect" to accurately
say "Using useEffect" (or similar) and modify the effect that uses useEffect so
that it always calls containerRootRef.current.render(...) — rendering children
when present and rendering an empty fragment (or null-equivalent) when children
is falsy — so stale content is cleared; target the useEffect block around
containerRootRef.current.render(...) and the children variable in
map-view.web.tsx.
src/components/settings/bluetooth-device-selection-bottom-sheet.tsx
Outdated
Show resolved
Hide resolved
| }; | ||
|
|
||
| mockUseCoreStore.mockReturnValue(newUnit); | ||
| mockUseCoreStore.mockImplementation((selector: any) => typeof selector === 'function' ? selector(newUnit) : newUnit); |
There was a problem hiding this comment.
Bug: mock state shape inconsistent with selector — test passes for the wrong reason.
The component uses useCoreStore((state) => state.activeUnit), so the selector expects an object with an activeUnit property. Here newUnit is passed directly (a UnitResultData), so selector(newUnit) evaluates to newUnit.activeUnit → undefined. The test expects 0/0 Active which happens to match the "no active unit" path, not the "different unit with no assignments" path it intends to verify.
Every other mockUseCoreStore call in this file wraps the value correctly (e.g., line 89, 100).
🐛 Proposed fix
- mockUseCoreStore.mockImplementation((selector: any) => typeof selector === 'function' ? selector(newUnit) : newUnit);
+ mockUseCoreStore.mockImplementation((selector: any) => typeof selector === 'function' ? selector({ activeUnit: newUnit }) : { activeUnit: newUnit });📝 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.
| mockUseCoreStore.mockImplementation((selector: any) => typeof selector === 'function' ? selector(newUnit) : newUnit); | |
| mockUseCoreStore.mockImplementation((selector: any) => typeof selector === 'function' ? selector({ activeUnit: newUnit }) : { activeUnit: newUnit }); |
🤖 Prompt for AI Agents
In `@src/components/sidebar/__tests__/roles-sidebar.test.tsx` at line 243, The
mock for useCoreStore is passing newUnit directly so selector(state =>
state.activeUnit) receives a UnitResultData and returns undefined; update the
mockUseCoreStore implementation used in this test to pass an object with
activeUnit: newUnit (i.e., mockUseCoreStore.mockImplementation((selector: any)
=> typeof selector === 'function' ? selector({ activeUnit: newUnit }) : {
activeUnit: newUnit })), ensuring the selector in the component
(useCoreStore((state) => state.activeUnit)) sees the intended activeUnit value
and the test exercises the intended branch.
| @@ -57,14 +57,23 @@ describe('SidebarUnitCard', () => { | |||
|
|
|||
| // Reset to default mocks | |||
| mockUseCoreStore.mockImplementation((selector) => selector({ activeUnit: null } as any)); | |||
There was a problem hiding this comment.
Inconsistent mock pattern: useCoreStore lacks the non-selector fallback.
Line 59 always calls selector(...) without checking if selector is a function. The other three stores in this same beforeEach (lines 60–80) use the typeof selector === 'function' guard. If useCoreStore is ever called without a selector in the component under test, this will throw a TypeError.
♻️ Suggested fix for consistency
- mockUseCoreStore.mockImplementation((selector) => selector({ activeUnit: null } as any));
+ mockUseCoreStore.mockImplementation((selector: any) => typeof selector === 'function' ? selector({ activeUnit: null } as any) : { activeUnit: null });📝 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.
| mockUseCoreStore.mockImplementation((selector) => selector({ activeUnit: null } as any)); | |
| mockUseCoreStore.mockImplementation((selector: any) => typeof selector === 'function' ? selector({ activeUnit: null } as any) : { activeUnit: null }); |
🤖 Prompt for AI Agents
In `@src/components/sidebar/__tests__/unit-sidebar.test.tsx` at line 59, The mock
for useCoreStore is calling selector(...) unconditionally which will throw if
the hook is used without a selector; update mockUseCoreStore.mockImplementation
to mirror the other store mocks by checking typeof selector === 'function' and
returning selector({ activeUnit: null } as any) only when it's a function,
otherwise return the state object ({ activeUnit: null } as any); reference
mockUseCoreStore and the selector guard pattern used elsewhere in this test
file.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/app/`(app)/index.tsx:
- Around line 397-398: The conditional guards using logical && (e.g., the
Mapbox.PointAnnotation render guarded by "locationLatitude && locationLongitude
&& (...)") incorrectly treat 0 as falsy and can render "0"; replace these with
nullish checks and the conditional operator so each guard becomes
"(locationLatitude ?? locationLongitude) ? <...> : null" (specifically update
the Mapbox.PointAnnotation that references locationLatitude/locationLongitude
and apply the same pattern to the other guards at the same sites mentioned),
ensuring you check each coordinate with the nullish coalescing or explicit
null/undefined checks and use ? : to return the JSX or null.
In `@src/services/location.ts`:
- Around line 155-191: In startLocationUpdates(), update the web geolocation
error callback (the watcher created via navigator.geolocation.watchPosition) to
treat fatal permission errors by removing/clearing the stale subscription and
clearing this.locationSubscription so future startLocationUpdates() calls can
re-subscribe; specifically ensure the subscription wrapper’s remove() (which
calls navigator.geolocation.clearWatch) is invoked (or call clearWatch with the
captured watchId) and then set this.locationSubscription = undefined/null inside
the error handler, while keeping the existing logger call.
🧹 Nitpick comments (3)
electron/main.js (1)
200-209: Minor:catchswallows allstatSyncerrors, not justENOENT.Permission errors or other filesystem issues would silently fall back to
index.htmlrather than surfacing the real problem. This is unlikely in practice for a local packaged app, so it's a nit.💡 Optional: narrow the catch
} catch (err) { - // File not found - serve index.html for client-side routing - filePath = path.join(resolvedDist, 'index.html'); + if (err.code === 'ENOENT') { + // File not found - serve index.html for client-side routing + filePath = path.join(resolvedDist, 'index.html'); + } else { + // Unexpected error - log and fall back + console.error('Protocol handler error:', err); + filePath = path.join(resolvedDist, 'index.html'); + } }src/services/location.ts (1)
388-407: Unnecessaryas anycast and redundant platform branch instopLocationUpdates.The web shim (line 187) is already cast to
Location.LocationSubscription, sothis.locationSubscription.remove()compiles withoutas any. Moreover,await-ing the synchronous web shim'sremove()is harmless (resolves immediately), so the entireisWeb/elsesplit is unnecessary.This also violates the coding guideline: "Avoid using
any; strive for precise types."Suggested simplification
async stopLocationUpdates(): Promise<void> { if (this.locationSubscription) { - if (isWeb) { - // On web the subscription is our own shim wrapping clearWatch - (this.locationSubscription as any).remove(); - } else { - await this.locationSubscription.remove(); - } + await this.locationSubscription.remove(); this.locationSubscription = null; }As per coding guidelines: "Avoid using
any; strive for precise types."src/app/(app)/index.tsx (1)
403-412: Same falsy-zero issue withlocationHeading— and uses&&conditional rendering.
locationHeading !== null && locationHeading !== undefinedis a proper nullish check here ✓ — but this JSX conditional still uses&&instead of the ternary operator per coding guidelines. This is a minor nit compared to the latitude/longitude issue above.Suggested ternary pattern
- {locationHeading !== null && locationHeading !== undefined && ( - <View - style={[ - styles.directionIndicator, - { - transform: [{ rotate: `${locationHeading}deg` }], - }, - ]} - /> - )} + {locationHeading != null ? ( + <View + style={[ + styles.directionIndicator, + { + transform: [{ rotate: `${locationHeading}deg` }], + }, + ]} + /> + ) : null}As per coding guidelines: "Use the conditional operator (?:) for conditional rendering instead of &&".
| async startLocationUpdates(): Promise<void> { | ||
| // On web, use a lightweight browser geolocation watcher instead of expo-location/TaskManager | ||
| if (isWeb) { | ||
| if (!('geolocation' in navigator)) { | ||
| logger.warn({ message: 'Geolocation API not available in this browser' }); | ||
| return; | ||
| } | ||
|
|
||
| if (!this.locationSubscription) { | ||
| const watchId = navigator.geolocation.watchPosition( | ||
| (pos) => { | ||
| const loc: Location.LocationObject = { | ||
| coords: { | ||
| latitude: pos.coords.latitude, | ||
| longitude: pos.coords.longitude, | ||
| altitude: pos.coords.altitude ?? 0, | ||
| accuracy: pos.coords.accuracy ?? 0, | ||
| altitudeAccuracy: pos.coords.altitudeAccuracy ?? 0, | ||
| heading: pos.coords.heading ?? 0, | ||
| speed: pos.coords.speed ?? 0, | ||
| }, | ||
| timestamp: pos.timestamp, | ||
| }; | ||
| useLocationStore.getState().setLocation(loc); | ||
| sendLocationToAPI(loc); | ||
| }, | ||
| (err) => { | ||
| logger.warn({ message: 'Web geolocation error', context: { code: err.code, msg: err.message } }); | ||
| }, | ||
| { enableHighAccuracy: false, maximumAge: 15000, timeout: 30000 } | ||
| ); | ||
| // Store a compatible subscription object | ||
| this.locationSubscription = { remove: () => navigator.geolocation.clearWatch(watchId) } as unknown as Location.LocationSubscription; | ||
| logger.info({ message: 'Foreground location updates started' }); | ||
| } | ||
| return; | ||
| } |
There was a problem hiding this comment.
Web geolocation path looks solid — previous observability concerns addressed.
The web branch now logs a warning when navigator.geolocation is unavailable (line 159) and logs success on subscription (line 188), resolving both prior review concerns.
One minor edge case: if the browser revokes geolocation permission after the watcher is established, the error callback (lines 181-183) logs the failure but leaves this.locationSubscription set. Subsequent calls to startLocationUpdates would see the stale subscription and skip re-subscribing (line 163 guard), silently stopping updates.
Suggested handling for fatal geolocation errors
(err) => {
- logger.warn({ message: 'Web geolocation error', context: { code: err.code, msg: err.message } });
+ logger.warn({ message: 'Web geolocation error', context: { code: err.code, msg: err.message } });
+ // PERMISSION_DENIED — clear the subscription so a future call can retry
+ if (err.code === err.PERMISSION_DENIED) {
+ if (this.locationSubscription) {
+ (this.locationSubscription as { remove: () => void }).remove();
+ this.locationSubscription = null;
+ }
+ }
},🤖 Prompt for AI Agents
In `@src/services/location.ts` around lines 155 - 191, In startLocationUpdates(),
update the web geolocation error callback (the watcher created via
navigator.geolocation.watchPosition) to treat fatal permission errors by
removing/clearing the stale subscription and clearing this.locationSubscription
so future startLocationUpdates() calls can re-subscribe; specifically ensure the
subscription wrapper’s remove() (which calls navigator.geolocation.clearWatch)
is invoked (or call clearWatch with the captured watchId) and then set
this.locationSubscription = undefined/null inside the error handler, while
keeping the existing logger call.
|
Approve |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
1 participant
Summary by CodeRabbit
New Features
Improvements
Configuration