CU-868fdadum Expo update for And16k, bug fixes and tweaks. #166
Conversation
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
src/components/settings/unit-selection-bottom-sheet.tsx (1)
60-67: Add isLoading:false on success in roles store
- In fetchRolesForUnit (src/stores/roles/store.ts:55–63), after
set({ unitRoleAssignments: unitRoles.Data })callset({ isLoading: false }).- In assignRoles (src/stores/roles/store.ts:82–89), after
await setRoleAssignmentsForUnit(data)callset({ isLoading: false })..github/workflows/react-native-cicd.yml (1)
279-318: Release-notes input undefined + version mismatch (10 vs 7) — fix to prevent lint/build surprises
- actionlint: github.event.inputs.release_notes isn’t defined for workflow_dispatch. Add the input, or drop the reference.
- Header says “Version 10.” but tags/names remain “7.”. Align to a single major to avoid confusing releases.
Apply within this hunk to align the GitHub Release metadata with “10”:
- name: 📦 Create Release if: ${{ matrix.platform == 'android' && (github.event.inputs.buildType == 'all' || github.event_name == 'push' || github.event.inputs.buildType == 'prod-apk') }} uses: ncipollo/release-action@v1 with: - tag: '7.${{ github.run_number }}' + tag: '10.${{ github.run_number }}' commit: ${{ github.sha }} makeLatest: true allowUpdates: true - name: '7.${{ github.run_number }}' + name: '10.${{ github.run_number }}' artifacts: './ResgridUnit-prod.apk' bodyFile: 'RELEASE_NOTES.md'Also add the missing input (outside the changed hunk) so actionlint passes:
# under: on.workflow_dispatch.inputs release_notes: description: 'Optional release notes override' type: string required: falseIf “10” is the new stream, update the package.json version bump step similarly (currently hardcoded to 7) for consistency. I can draft that patch if you want it in this PR.
src/components/status/status-bottom-sheet.tsx (1)
66-73: Tighten types: avoidanyin getStatusProperty.This defeats type safety on
selectedStatus.Apply:
- const getStatusProperty = React.useCallback( - <T extends keyof CustomStatusResultData>(prop: T, defaultValue: CustomStatusResultData[T]): CustomStatusResultData[T] => { - if (!selectedStatus) return defaultValue; - return (selectedStatus as any)[prop] ?? defaultValue; - }, - [selectedStatus] - ); + const getStatusProperty = React.useCallback( + <T extends keyof CustomStatusResultData>(prop: T, defaultValue: CustomStatusResultData[T]): CustomStatusResultData[T] => { + return selectedStatus ? (selectedStatus[prop] ?? defaultValue) : defaultValue; + }, + [selectedStatus] + );package.json (1)
105-105: Align Expo SDK and React Native versions
The project currently declares"expo": "~52.0.46", "react-native": "0.77.3",and excludes
"react-native@~0.76.6"underexpo.install.exclude, causing an SDK 52 vs RN 0.77 mismatch.
- Either pin React Native (and related libraries) to Expo SDK 52’s supported versions (e.g.
"react-native": "~0.76.6") and remove any conflicting excludes.- Or upgrade Expo to SDK 53+ and align all React Native–dependent libs to RN 0.77.x.
src/services/location.ts (1)
129-144: Don’t prompt for background permission when only foreground is required.You still call
requestBackgroundPermissionsAsync(), which can prompt users unnecessarily (especially on iOS).Apply:
- const { status: foregroundStatus } = await Location.requestForegroundPermissionsAsync(); - const { status: backgroundStatus } = await Location.requestBackgroundPermissionsAsync(); + const { status: foregroundStatus } = await Location.requestForegroundPermissionsAsync(); logger.info({ message: 'Location permissions requested', context: { foregroundStatus, - backgroundStatus, }, }); - return foregroundStatus === 'granted'; + return foregroundStatus === 'granted';Background permission can be checked later with
getBackgroundPermissionsAsync()where needed (you already do this below).src/services/__tests__/signalr.service.test.ts (1)
498-535: Replacejest.runAllTicks()calls with a microtask-flush helper
Modern Jest (27+) uses “modern” fake timers by default, which don’t supportrunAllTicks(); instead, explicitly flush pending microtasks (jest-archive-august-2023.netlify.app, medium.com).
Add at the top of your test file:const flushPromises = () => Promise.resolve();Then update each occurrence:
- await jest.runAllTicks(); + await flushPromises();in src/services/tests/signalr.service.test.ts at lines 525, 558, 616. Run the SignalR tests under the default (modern) fake-timer implementation (Jest 29.7.0) to verify no regressions.
🧹 Nitpick comments (47)
src/components/settings/__tests__/server-url-bottom-sheet-simple.test.tsx (1)
15-20: Use forwardRef and avoid requiring react-native in the ScrollView mock.Forwarding refs better matches real ScrollView behavior and reduces surprises if the component attaches a ref. Also, using the host string 'View' avoids loading react-native during mocking.
-jest.mock('react-native/Libraries/Components/ScrollView/ScrollView', () => { - const React = require('react'); - const { View } = require('react-native'); - return ({ children, ...props }: any) => React.createElement(View, props, children); -}); +jest.mock('react-native/Libraries/Components/ScrollView/ScrollView', () => { + const React = require('react'); + return React.forwardRef(({ children, ...props }: any, ref: any) => + React.createElement('View', { ref, ...props }, children) + ); +});src/components/settings/__tests__/unit-selection-bottom-sheet-simple.test.tsx (2)
380-400: Remove unused console.error spy or mock logger instead.The component logs via logger, not console.error. The spy doesn’t intercept anything and adds noise.
- const consoleError = jest.spyOn(console, 'error').mockImplementation(() => { }); @@ - consoleError.mockRestore(); + // If you want to silence logs, mock '@/lib/logging' instead.
90-110: ActionsheetItem mock is fine; add a testID pass-through for clarity.To make queries explicit and resilient, forward an optional testID prop.
- ActionsheetItem: ({ children, onPress, disabled }: any) => { + ActionsheetItem: ({ children, onPress, disabled, testID }: any) => { const React = require('react'); const handlePress = disabled ? undefined : onPress; return React.createElement( - 'TouchableOpacity', - { onPress: handlePress, testID: 'actionsheet-item', disabled }, + 'TouchableOpacity', + { onPress: handlePress, testID: testID || 'actionsheet-item', disabled }, children ); },src/components/settings/__tests__/unit-selection-bottom-sheet.test.tsx (3)
454-474: Same note as in “simple” suite: console.error spy is redundant.The component logs via logger; remove or mock logger.
- const consoleError = jest.spyOn(console, 'error').mockImplementation(() => { }); @@ - consoleError.mockRestore();
476-486: Memoization test is weak; consider measuring renders.Asserting text counts doesn’t prove memoization. If desired, spy on render counts with a jest.fn child or React.Profiler.
90-114: Small usability tweak in mocks: make actionsheet items actually pressable.Currently using a View with onPress. Using TouchableOpacity (like in the simple suite) or Pressable improves fidelity.
- ActionsheetItem: ({ children, onPress, disabled, ...props }: any) => { + ActionsheetItem: ({ children, onPress, disabled, ...props }: any) => { const React = require('react'); - return React.createElement( - 'View', + return React.createElement( + 'TouchableOpacity', { onPress: disabled ? undefined : onPress, testID: props.testID || 'actionsheet-item', accessibilityState: { disabled }, }, children ); },src/components/settings/unit-selection-bottom-sheet.tsx (4)
45-51: Good close-while-loading guard.Prevents accidental dismissal mid-action. Consider also disabling the Actionsheet backdrop close if the lib exposes it.
52-77: Skip redundant work when selecting the already-active unit.Avoid network churn by short-circuiting if the picked unit is already active; optional UX: just close.
const handleUnitSelection = React.useCallback( async (unit: UnitResultData) => { if (isLoading) { return; } + if (unit.UnitId === activeUnit?.UnitId) { + handleClose(); + return; + } try { setIsLoading(true); await setActiveUnit(unit.UnitId); await useRolesStore.getState().fetchRolesForUnit(unit.UnitId); logger.info({ message: 'Active unit updated successfully', context: { unitId: unit.UnitId }, }); handleClose(); } catch (error) { logger.error({ message: 'Failed to update active unit', context: { error }, }); } finally { setIsLoading(false); } }, - [setActiveUnit, handleClose, isLoading] + [setActiveUnit, handleClose, isLoading, activeUnit?.UnitId] );
106-135: Consider FlatList for scalable lists.If units can grow, FlatList aligns with project guidelines (virtualization, getItemLayout, batching). Not urgent if the list stays small.
116-127: Selection styling likely won’t trigger with data-[checked] variant.Unless ActionsheetItem sets data-checked, this class won’t apply. Explicitly apply a selected style when active.
- <ActionsheetItem ... className={activeUnit?.UnitId === unit.UnitId ? 'data-[checked=true]:bg-background-100' : ''}> + <ActionsheetItem + ... + className={activeUnit?.UnitId === unit.UnitId ? 'bg-background-100' : ''} + >tsconfig.json (1)
33-33: Include types directory — good; consider scoping to declarations only.If types/ contains ambient/global declarations, prefer including only .d.ts to avoid compiling runtime .ts here. Otherwise, keep as-is.
Apply if appropriate:
- "include": ["src/**/*.ts", "src/**/*.tsx", ".expo/types/**/*.ts", "expo-env.d.ts", "nativewind-env.d.ts", "__mocks__/**/*.ts", "app.config.ts", "jest-setup.ts", "__tests__/**/*.ts", "__tests__/**/*.tsx", "types/**/*.ts"] + "include": ["src/**/*.ts", "src/**/*.tsx", ".expo/types/**/*.ts", "expo-env.d.ts", "nativewind-env.d.ts", "__mocks__/**/*.ts", "app.config.ts", "jest-setup.ts", "__tests__/**/*.ts", "__tests__/**/*.tsx", "types/**/*.d.ts"]app.config.ts (1)
116-116: Gate critical alerts by environment to match entitlements.You already gate the iOS entitlements to production/internal; align the permission flag to avoid requesting unsupported options in other envs.
- allowCriticalAlerts: true, + allowCriticalAlerts: Env.APP_ENV === 'production' || Env.APP_ENV === 'internal',Also ensure your Apple team has the Critical Alerts entitlement approved for the bundle ID before releasing.
.github/dependabot.yml (1)
10-10: Setting open-pull-requests-limit: 0 disables Dependabot PRs. Confirm intent.If the goal is to pause noise temporarily, consider package grouping or ignore rules instead.
src/stores/status/__tests__/store.test.ts (1)
312-316: Test updated to expect re-throw — OK; consider asserting the errorOptional: assert the thrown error to make intent explicit.
- try { - await result.current.saveUnitStatus(input); - } catch (error) { - // Expected to throw now since we re-throw critical errors - } + await expect(result.current.saveUnitStatus(input)).rejects.toThrow('Critical error')src/components/status/status-bottom-sheet.tsx (4)
504-505: Remove no-op color ternary.
colorScheme === 'dark' ? '#fff' : '#fff'always yields#fff.Apply:
- <ArrowRight size={14} color={colorScheme === 'dark' ? '#fff' : '#fff'} /> + <ArrowRight size={14} color="#fff" />Also applies to: 614-615
188-193: Drop UTC→GMT string replace.
toUTCString()already returns “GMT”;.replace('UTC','GMT')is unnecessary and brittle.- input.TimestampUtc = locationDate.toUTCString().replace('UTC', 'GMT'); + input.TimestampUtc = locationDate.toUTCString();
460-499: Use a virtualized list for large datasets.
ScrollViewrendering full lists (statuses/calls/stations) can hurt perf and memory. Prefer FlashList (already in deps) or FlatList.If using FlashList:
- <ScrollView className="max-h-[400px]"> - <VStack space="sm"> - {activeStatuses?.Statuses?.map(...)} - </VStack> - </ScrollView> + <FlashList + estimatedItemSize={56} + data={activeStatuses?.Statuses ?? []} + keyExtractor={(s) => String(s.Id)} + renderItem={({ item: status }) => (/* item UI */)} + style={{ maxHeight: 400 }} + />Also apply similar refactor for calls/stations.
Also applies to: 544-607
456-509: Align conditional rendering with project guidelines.Guidelines specify using the conditional operator
?:instead of&&. Several blocks here use&&.Example:
- {currentStep === 'select-status' && (<VStack>...</VStack>)} + {currentStep === 'select-status' ? (<VStack>...</VStack>) : null}Also applies to: 510-618, 638-676
src/hooks/use-map-signalr-updates.ts (3)
20-28: Abort vs. concurrency: unreachable abort path.Because
isUpdatingshort-circuits, the “cancel previous request” branch never runs for in-flight calls. Decide between:
- Keep single-flight (keep
isUpdating) and only support unmount aborts, or- Allow superseding updates by aborting the in-flight request and starting a new one.
If preferring supersede-latest:
- if (isUpdating.current) { ... return; } + if (isUpdating.current) { + abortController.current?.abort(); + }
8-10: Export debounce constant for reuse in tests/config.- const DEBOUNCE_DELAY = 1000; + export const DEBOUNCE_DELAY = 1000;
14-15: Timer type: prefer ReturnType for RN/web compat.- const debounceTimer = useRef<NodeJS.Timeout | null>(null); + const debounceTimer = useRef<ReturnType<typeof setTimeout> | null>(null);Also applies to: 90-120
src/hooks/__tests__/use-map-signalr-updates.test.ts (2)
221-255: Test name vs. assertion mismatch; also not asserting real cancellation.“cancel previous request” test only counts controllers; it never asserts
abort()nor that the request received a signal.
- Assert
mockAbortwas called when a new update arrives.- Spy on
getMapDataAndMarkersto verify it was called with anAbortSignal.
Example:- expect(abortControllerCount).toBe(1); + expect(mockAbort).toHaveBeenCalled(); + expect(getMapDataAndMarkers).toHaveBeenCalledWith(expect.any(AbortSignal));
354-378: Stabilize timer usage and cleanup.Prefer
jest.advanceTimersByTime(DEBOUNCE_DELAY)overrunAllTimers()to avoid unintended timer flush; also restoreAbortControllerinafterEachto reduce test coupling.package.json (2)
96-96: Avoid RC for Mapbox in production unless necessary.
@rnmapbox/maps@10.1.42-rc.0may introduce instability. Prefer the latest stable unless the RC fixes a blocker you’ve patched for.
36-36: postinstall-postinstall is unused.If you intend to guarantee patch-package runs after all installs, add the recommended script:
"scripts": { + "prepare": "is-ci || postinstall-postinstall", "postinstall": "patch-package", }Or remove
postinstall-postinstallfrom devDependencies.Also applies to: 205-206
src/services/location.ts (2)
166-174: Localize foreground-service notification strings.User-facing strings should be wrapped with
t().Example:
- notificationTitle: 'Location Tracking', - notificationBody: 'Tracking your location in the background', + notificationTitle: t('location.notification_title'), + notificationBody: t('location.notification_body'),Ensure keys exist in translations.
Also applies to: 287-289
206-207: Await API calls or handle promise rejection in watchers.
sendLocationToAPI(location)is fire-and-forget; unhandled rejections could be swallowed in some environments.- sendLocationToAPI(location); + void sendLocationToAPI(location);At minimum, mark as intentionally unawaited.
Also applies to: 245-246
src/services/__tests__/location-foreground-permissions.test.ts (2)
160-166: Avoidas anyfor permission statusesPrefer precise typing for the mocked
statusfields to keep tests type-safe.Apply:
- mockLocation.requestForegroundPermissionsAsync.mockResolvedValue({ - status: 'granted' as any, + mockLocation.requestForegroundPermissionsAsync.mockResolvedValue({ + status: 'granted' as Location.PermissionStatus, expires: 'never', granted: true, canAskAgain: true, }); @@ - mockLocation.requestBackgroundPermissionsAsync.mockResolvedValue({ - status: 'denied' as any, + mockLocation.requestBackgroundPermissionsAsync.mockResolvedValue({ + status: 'denied' as Location.PermissionStatus, expires: 'never', granted: false, canAskAgain: true, }); @@ - mockLocation.getBackgroundPermissionsAsync.mockResolvedValue({ - status: 'denied' as any, + mockLocation.getBackgroundPermissionsAsync.mockResolvedValue({ + status: 'denied' as Location.PermissionStatus, expires: 'never', granted: false, canAskAgain: true, });Also applies to: 167-173, 174-180
214-215: Nit: simpler assertionFor "does not throw", simply
await locationService.startLocationUpdates();is clearer and avoids a redundant matcher.- await expect(locationService.startLocationUpdates()).resolves.not.toThrow(); + await locationService.startLocationUpdates();src/services/__tests__/location.test.ts (3)
42-60: Type safety for background permission mocksUse concrete union type instead of
as anyfor permission statuses.- const mockGetBackgroundPermissions = jest.fn(); + const mockGetBackgroundPermissions = jest.fn(); @@ - mockLocation.getBackgroundPermissionsAsync.mockResolvedValue({ - status: 'granted' as any, + mockLocation.getBackgroundPermissionsAsync.mockResolvedValue({ + status: 'granted' as Location.PermissionStatus, expires: 'never', granted: true, canAskAgain: true, });Also applies to: 165-171
1957-1964: Wait for async submit to avoid flakiness
handleSubmitawaits an asyncsaveUnitStatus. UsewaitForto avoid timing races.- fireEvent.press(nextButton); - expect(mockSaveUnitStatus).toHaveBeenCalled(); + fireEvent.press(nextButton); + await waitFor(() => expect(mockSaveUnitStatus).toHaveBeenCalled());
1738-1756: Brittle DOM hierarchy checksSeveral assertions rely on
.parent?.parentandclassNameto infer “selected” state. Consider testIDs or explicit accessibility states for more resilient tests.Also applies to: 1782-1803
src/components/bluetooth/bluetooth-audio-modal.tsx (4)
45-53: Stop scanning on close/unmount to conserve batteryWhen the sheet closes or the component unmounts, ensure scanning is stopped.
useEffect(() => { // Auto-start scanning when modal opens and Bluetooth is ready if (isOpen && bluetoothState === State.PoweredOn && !isScanning && !connectedDevice) { handleStartScan().catch((error) => { logger.error({ message: 'Failed to start scan', context: { error }, }); }); } - }, [isOpen, bluetoothState, isScanning, connectedDevice, handleStartScan]); + // Stop scan if modal closes + if (!isOpen && isScanning) { + bluetoothAudioService.stopScanning(); + } + return () => { + bluetoothAudioService.stopScanning(); + }; + }, [isOpen, bluetoothState, isScanning, connectedDevice, handleStartScan]);
247-286: Avoid inline lambdas in lists to reduce re-rendersCreate stable handlers for device actions instead of
onPress={() => handleConnectDevice(device)}.- const renderDeviceList = () => { + const handleConnectById = React.useCallback(async (id: string) => { + if (isConnecting) return; + try { + await bluetoothAudioService.connectToDevice(id); + } catch (error) { + logger.error({ message: 'Failed to connect to device', context: { error, deviceId: id } }); + } + }, [isConnecting]); + + const renderDeviceList = () => { @@ - {!device.isConnected ? ( - <Button onPress={() => handleConnectDevice(device)} size="sm" isDisabled={isConnecting}> + {!device.isConnected ? ( + <Button onPress={() => handleConnectById(device.id)} size="sm" isDisabled={isConnecting}>Also applies to: 277-281
309-314: Badge text should be localized“LiveKit Active” should use
t(), per guidelines.- <Text className="ml-1 text-xs">LiveKit Active</Text> + <Text className="ml-1 text-xs">{t('bluetooth.livekit_active', { defaultValue: 'LiveKit Active' })}</Text>
335-335: Memoize the modal componentWrap with
React.memoto avoid unnecessary re-renders when parent passes stable props.-export default BluetoothAudioModal; +export default React.memo(BluetoothAudioModal);src/components/status/__tests__/status-bottom-sheet.test.tsx (2)
600-606: PreferwaitForfor async state assertionsWhere actions trigger async submissions or store updates, wrap expectations in
waitForto avoid flakiness.- const submitButton = screen.getByRole('button', { name: /submit/i }); - expect(submitButton.props.accessibilityState.disabled).toBe(true); + const submitButton = screen.getByRole('button', { name: /submit/i }); + await waitFor(() => expect(submitButton.props.accessibilityState.disabled).toBe(true));Apply similarly to other submit/next flows.
Also applies to: 622-626, 1959-1964
986-988: Brittle.parent?.parentand className checksThese assertions are sensitive to layout changes. Consider using testIDs (e.g.,
testID="destination-none-selected") or explicit accessibility states to verify selection.Also applies to: 1016-1018, 1046-1048, 1657-1663, 1713-1716
src/services/__tests__/signalr.service.enhanced.test.ts (5)
35-37: Reduce mock variable shadowing; simplify auth store stubbing.Two different
mockRefreshAccessTokensymbols (factory-local vs test-local) are confusing. Rename the test-local one and reset viamockReset().-const mockGetState = (useAuthStore as any).getState; -const mockRefreshAccessToken = jest.fn().mockResolvedValue(undefined); +const mockGetState = (useAuthStore as any).getState; +const refreshAccessTokenMock = jest.fn().mockResolvedValue(undefined); @@ -// Reset refresh token mock -mockRefreshAccessToken.mockClear(); -mockRefreshAccessToken.mockResolvedValue(undefined); +// Reset refresh token mock +refreshAccessTokenMock.mockReset(); +refreshAccessTokenMock.mockResolvedValue(undefined); @@ -// Mock auth store -mockGetState.mockReturnValue({ - accessToken: 'mock-token', - refreshAccessToken: mockRefreshAccessToken, -}); +// Mock auth store +mockGetState.mockReturnValue({ + accessToken: 'mock-token', + refreshAccessToken: refreshAccessTokenMock, +});Also applies to: 72-81
62-71: Type the mocks to avoidas anyand strengthen assertions.Use
jest.Mocked<HubConnection>/jest.Mocked<HubConnectionBuilder>for better type safety and IDE help.- let mockConnection: jest.Mocked<HubConnection>; - let mockBuilderInstance: jest.Mocked<HubConnectionBuilder>; + let mockConnection: jest.Mocked<HubConnection>; + let mockBuilderInstance: jest.Mocked<HubConnectionBuilder>;And construct with explicit casts on each field instead of
as any, or wrap withas unknown as jest.Mocked<...>once at the end to avoid leakingany.
118-142: Broaden assertions to verify builder config.Since URL/token handling is critical, assert
withUrlis called once with expected args (sanitized), not just thatbuild/startran.Example:
expect(mockBuilderInstance.withUrl).toHaveBeenCalledTimes(1); expect(mockBuilderInstance.withAutomaticReconnect).toHaveBeenCalledWith([0, 2000, 5000, 10000, 30000]); expect(mockBuilderInstance.configureLogging).toHaveBeenCalledWith(LogLevel.Information);
245-276: Prefer spying over overridingMap.has.Overwriting
connections.hasmutates the instance shape. Usejest.spyOn(connectionsMap, 'has').-const originalHas = connectionsMap.has; -connectionsMap.has = jest.fn().mockReturnValue(true); +const hasSpy = jest.spyOn(connectionsMap, 'has').mockReturnValue(true); @@ -// Restore original method -connectionsMap.has = originalHas; +hasSpy.mockRestore();
211-223: Ensure timers are always restored.Use an
afterEach(() => jest.useRealTimers())to prevent cross-test leakage instead of restoring inside individual tests.Add:
afterEach(() => { jest.useRealTimers(); });Also applies to: 254-276
src/services/signalr.service.ts (3)
222-243: DRY up connection code paths.
connectToHubWithEventingUrlandconnectToHubduplicate connection setup and handlers. Extract a helper to build the connection and to register handlers.Outline:
private buildConnection(url: string, tokenFactory?: () => string): HubConnectionBuilderprivate wireConnectionEvents(connection: HubConnection, name: string, methods: string[])Also applies to: 245-322
445-468: Makeinvokegeneric to return typed results.Current signature discards return types from hub methods.
- public async invoke(hubName: string, method: string, data: unknown): Promise<void> { + public async invoke<T = unknown>(hubName: string, method: string, data: unknown): Promise<T | void> { @@ - return await connection.invoke(method, data); + return await connection.invoke<T>(method, data);Call sites can then specify
<T>when expecting a value.
470-486: Consider makingresetInstanceasync to guarantee cleanup before reuse.Tests may race with pending
disconnectAll(). Returning aPromise<void>enablesawait SignalRService.resetInstance()in tests.- public static resetInstance(): void { + public static async resetInstance(): Promise<void> { - if (SignalRService.instance) { - // Disconnect all connections before resetting - SignalRService.instance.disconnectAll().catch((error) => { - logger.error({ message: 'Error disconnecting all hubs during instance reset', context: { error } }); - }); - } + if (SignalRService.instance) { + try { + await SignalRService.instance.disconnectAll(); + } catch (error) { + logger.error({ message: 'Error disconnecting all hubs during instance reset', context: { error } }); + } + } SignalRService.instance = null; - SignalRService.isCreating = false; logger.debug({ message: 'SignalR service singleton instance reset' }); }docs/signalr-service-refactoring.md (1)
65-72: Minor grammar and style nits for readability.Tighten phrasing (e.g., “thread safety” → “re-entrancy safety” for JS), hyphenate consistently (“clean-up” → “cleanup”). No functional impact.
If you want, I can submit a copy edit PR for this page.
Also applies to: 118-126
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (2)
.DS_Storeis excluded by!**/.DS_Storeyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (30)
.github/dependabot.yml(1 hunks).github/workflows/react-native-cicd.yml(1 hunks)app.config.ts(1 hunks)docs/signalr-service-refactoring.md(1 hunks)expo-env.d.ts(1 hunks)package.json(5 hunks)src/components/audio-stream/audio-stream-bottom-sheet.tsx(0 hunks)src/components/bluetooth/bluetooth-audio-modal.tsx(13 hunks)src/components/settings/__tests__/server-url-bottom-sheet-simple.test.tsx(1 hunks)src/components/settings/__tests__/unit-selection-bottom-sheet-simple.test.tsx(1 hunks)src/components/settings/__tests__/unit-selection-bottom-sheet.test.tsx(1 hunks)src/components/settings/unit-selection-bottom-sheet.tsx(4 hunks)src/components/status/__tests__/status-bottom-sheet.test.tsx(5 hunks)src/components/status/status-bottom-sheet.tsx(6 hunks)src/hooks/__tests__/use-map-signalr-updates.test.ts(1 hunks)src/hooks/use-map-signalr-updates.ts(1 hunks)src/hooks/use-status-signalr-updates.ts(0 hunks)src/services/__tests__/location-foreground-permissions.test.ts(1 hunks)src/services/__tests__/location.test.ts(12 hunks)src/services/__tests__/signalr.service.enhanced.test.ts(1 hunks)src/services/__tests__/signalr.service.test.ts(6 hunks)src/services/location.ts(4 hunks)src/services/signalr.service.ts(8 hunks)src/stores/status/__tests__/store.test.ts(1 hunks)src/stores/status/store.ts(2 hunks)src/translations/ar.json(1 hunks)src/translations/en.json(1 hunks)src/translations/es.json(1 hunks)src/utils/b01-inrico-debug.ts(0 hunks)tsconfig.json(1 hunks)
💤 Files with no reviewable changes (3)
- src/hooks/use-status-signalr-updates.ts
- src/components/audio-stream/audio-stream-bottom-sheet.tsx
- src/utils/b01-inrico-debug.ts
🧰 Additional context used
📓 Path-based instructions (10)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursorrules)
**/*.{ts,tsx}: Write concise, type-safe TypeScript code
Use camelCase for variable and function names (e.g., isFetchingData, handleUserInput)
Use TypeScript for all components, favoring interfaces for props and state
Avoid using any; strive for precise types
Use Expo SecureStore for sensitive data
Use zustand for state management
Use react-query for data fetching and caching
Use react-i18next for internationalization
Use react-native-mmkv for local storage
Use axios for API requests
**/*.{ts,tsx}: Write concise, type-safe TypeScript code
Use camelCase for variable and function names
Use TypeScript for all components, favoring interfaces for props and state
Avoid using any; strive for precise types
Use React Navigation for handling navigation and deep linking
Handle errors gracefully and provide user feedback
Use Expo's SecureStore for sensitive data
Use zustand for state management
Use react-query for data fetching
Use react-i18next for internationalization
Use react-native-mmkv for local storage
Use axios for API requests
Files:
src/components/settings/__tests__/unit-selection-bottom-sheet-simple.test.tsxsrc/components/settings/__tests__/unit-selection-bottom-sheet.test.tsxapp.config.tssrc/services/__tests__/location-foreground-permissions.test.tssrc/components/settings/__tests__/server-url-bottom-sheet-simple.test.tsxsrc/stores/status/__tests__/store.test.tsexpo-env.d.tssrc/services/__tests__/signalr.service.enhanced.test.tssrc/hooks/use-map-signalr-updates.tssrc/services/location.tssrc/components/status/__tests__/status-bottom-sheet.test.tsxsrc/stores/status/store.tssrc/components/status/status-bottom-sheet.tsxsrc/hooks/__tests__/use-map-signalr-updates.test.tssrc/components/bluetooth/bluetooth-audio-modal.tsxsrc/services/__tests__/location.test.tssrc/components/settings/unit-selection-bottom-sheet.tsxsrc/services/__tests__/signalr.service.test.tssrc/services/signalr.service.ts
**/*.tsx
📄 CodeRabbit inference engine (.cursorrules)
**/*.tsx: Use functional components and hooks over class components
Use PascalCase for React component names (e.g., UserProfile, ChatScreen)
Utilize React.FC for defining functional components with props
Minimize useEffect, useState, and heavy computations inside render
Use React.memo() for components with static props to prevent unnecessary re-renders
Optimize FlatList with removeClippedSubviews, maxToRenderPerBatch, windowSize
Use getItemLayout for FlatList when items have consistent size
Avoid anonymous functions in renderItem or event handlers to prevent re-renders
Use gluestack-ui for styling; if no component exists in src/components/ui, style via StyleSheet.create or styled-components
Optimize images using react-native-fast-image
Use React Navigation for navigation and deep linking with best practices
Wrap all user-facing text in t() from react-i18next
Use react-hook-form for form handling
Use @rnmapbox/maps for maps or vehicle navigation
Use lucide-react-native for icons directly in markup; do not use the gluestack-ui icon component
Use the conditional operator (?:) for conditional rendering instead of &&
**/*.tsx: Use functional components and hooks over class components
Use PascalCase for component names
Utilize React.FC for defining functional components with props
Minimize useEffect/useState and heavy computations inside render
Use React.memo for components with static props to prevent unnecessary re-renders
Optimize FlatList with removeClippedSubviews, maxToRenderPerBatch, and windowSize
Use getItemLayout for FlatList when items have consistent size
Avoid anonymous functions in renderItem or event handlers
Optimize image handling using react-native-fast-image
Wrap all user-facing text in t() from react-i18next
Ensure support for dark mode and light mode
Ensure the app is accessible following WCAG for mobile
Use react-hook-form for form handling
Use @rnmapbox/maps for maps and navigation
Use lucide-react-native for icons directly in markup; do not us...
Files:
src/components/settings/__tests__/unit-selection-bottom-sheet-simple.test.tsxsrc/components/settings/__tests__/unit-selection-bottom-sheet.test.tsxsrc/components/settings/__tests__/server-url-bottom-sheet-simple.test.tsxsrc/components/status/__tests__/status-bottom-sheet.test.tsxsrc/components/status/status-bottom-sheet.tsxsrc/components/bluetooth/bluetooth-audio-modal.tsxsrc/components/settings/unit-selection-bottom-sheet.tsx
**/*
📄 CodeRabbit inference engine (.cursorrules)
Directory and file names should be lowercase and hyphenated (e.g., user-profile, chat-screen)
Files:
src/components/settings/__tests__/unit-selection-bottom-sheet-simple.test.tsxsrc/components/settings/__tests__/unit-selection-bottom-sheet.test.tsxapp.config.tssrc/services/__tests__/location-foreground-permissions.test.tssrc/components/settings/__tests__/server-url-bottom-sheet-simple.test.tsxsrc/stores/status/__tests__/store.test.tssrc/translations/es.jsonexpo-env.d.tstsconfig.jsonsrc/translations/ar.jsonsrc/services/__tests__/signalr.service.enhanced.test.tssrc/hooks/use-map-signalr-updates.tssrc/services/location.tssrc/translations/en.jsonsrc/components/status/__tests__/status-bottom-sheet.test.tsxsrc/stores/status/store.tspackage.jsonsrc/components/status/status-bottom-sheet.tsxsrc/hooks/__tests__/use-map-signalr-updates.test.tssrc/components/bluetooth/bluetooth-audio-modal.tsxsrc/services/__tests__/location.test.tsdocs/signalr-service-refactoring.mdsrc/components/settings/unit-selection-bottom-sheet.tsxsrc/services/__tests__/signalr.service.test.tssrc/services/signalr.service.ts
**/*.{test.ts,test.tsx,spec.ts,spec.tsx}
📄 CodeRabbit inference engine (.cursorrules)
Create and use Jest tests to validate all generated components
Files:
src/components/settings/__tests__/unit-selection-bottom-sheet-simple.test.tsxsrc/components/settings/__tests__/unit-selection-bottom-sheet.test.tsxsrc/services/__tests__/location-foreground-permissions.test.tssrc/components/settings/__tests__/server-url-bottom-sheet-simple.test.tsxsrc/stores/status/__tests__/store.test.tssrc/services/__tests__/signalr.service.enhanced.test.tssrc/components/status/__tests__/status-bottom-sheet.test.tsxsrc/hooks/__tests__/use-map-signalr-updates.test.tssrc/services/__tests__/location.test.tssrc/services/__tests__/signalr.service.test.ts
**/__tests__/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursorrules)
Generate tests for all components, services, and logic; ensure tests run without errors
Files:
src/components/settings/__tests__/unit-selection-bottom-sheet-simple.test.tsxsrc/components/settings/__tests__/unit-selection-bottom-sheet.test.tsxsrc/services/__tests__/location-foreground-permissions.test.tssrc/components/settings/__tests__/server-url-bottom-sheet-simple.test.tsxsrc/stores/status/__tests__/store.test.tssrc/services/__tests__/signalr.service.enhanced.test.tssrc/components/status/__tests__/status-bottom-sheet.test.tsxsrc/hooks/__tests__/use-map-signalr-updates.test.tssrc/services/__tests__/location.test.tssrc/services/__tests__/signalr.service.test.ts
{components/ui/**/*.{ts,tsx},**/*.tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use gluestack-ui consistently; if no component exists in components/ui, style via StyleSheet.create or styled-components
Files:
src/components/settings/__tests__/unit-selection-bottom-sheet-simple.test.tsxsrc/components/settings/__tests__/unit-selection-bottom-sheet.test.tsxsrc/components/settings/__tests__/server-url-bottom-sheet-simple.test.tsxsrc/components/status/__tests__/status-bottom-sheet.test.tsxsrc/components/status/status-bottom-sheet.tsxsrc/components/bluetooth/bluetooth-audio-modal.tsxsrc/components/settings/unit-selection-bottom-sheet.tsx
**/*.{test,spec}.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Create and use Jest tests for all generated components
Files:
src/components/settings/__tests__/unit-selection-bottom-sheet-simple.test.tsxsrc/components/settings/__tests__/unit-selection-bottom-sheet.test.tsxsrc/services/__tests__/location-foreground-permissions.test.tssrc/components/settings/__tests__/server-url-bottom-sheet-simple.test.tsxsrc/stores/status/__tests__/store.test.tssrc/services/__tests__/signalr.service.enhanced.test.tssrc/components/status/__tests__/status-bottom-sheet.test.tsxsrc/hooks/__tests__/use-map-signalr-updates.test.tssrc/services/__tests__/location.test.tssrc/services/__tests__/signalr.service.test.ts
src/translations/**
📄 CodeRabbit inference engine (.cursorrules)
Store translation dictionaries in src/translations
Files:
src/translations/es.jsonsrc/translations/ar.jsonsrc/translations/en.json
src/translations/**/*.{json,ts}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Store translation dictionaries in src/translations
Files:
src/translations/es.jsonsrc/translations/ar.jsonsrc/translations/en.json
tsconfig.json
📄 CodeRabbit inference engine (.cursorrules)
Enable strict typing in tsconfig.json
Files:
tsconfig.json
🧠 Learnings (9)
📚 Learning: 2025-08-12T03:33:40.238Z
Learnt from: CR
PR: Resgrid/Unit#0
File: .cursorrules:0-0
Timestamp: 2025-08-12T03:33:40.238Z
Learning: Applies to **/*.{test.ts,test.tsx,spec.ts,spec.tsx} : Create and use Jest tests to validate all generated components
Applied to files:
src/components/settings/__tests__/unit-selection-bottom-sheet-simple.test.tsxsrc/components/settings/__tests__/unit-selection-bottom-sheet.test.tsxsrc/components/settings/__tests__/server-url-bottom-sheet-simple.test.tsxtsconfig.json
📚 Learning: 2025-08-12T03:34:25.992Z
Learnt from: CR
PR: Resgrid/Unit#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-12T03:34:25.992Z
Learning: Applies to **/*.{test,spec}.{ts,tsx} : Create and use Jest tests for all generated components
Applied to files:
src/components/settings/__tests__/unit-selection-bottom-sheet-simple.test.tsxsrc/components/settings/__tests__/server-url-bottom-sheet-simple.test.tsxtsconfig.json
📚 Learning: 2025-08-12T03:33:40.238Z
Learnt from: CR
PR: Resgrid/Unit#0
File: .cursorrules:0-0
Timestamp: 2025-08-12T03:33:40.238Z
Learning: Applies to **/__tests__/**/*.{ts,tsx} : Generate tests for all components, services, and logic; ensure tests run without errors
Applied to files:
src/components/settings/__tests__/unit-selection-bottom-sheet-simple.test.tsx
📚 Learning: 2025-08-12T03:34:25.992Z
Learnt from: CR
PR: Resgrid/Unit#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-12T03:34:25.992Z
Learning: Applies to **/*.{ts,tsx} : Use TypeScript for all components, favoring interfaces for props and state
Applied to files:
tsconfig.json
📚 Learning: 2025-08-12T03:33:40.238Z
Learnt from: CR
PR: Resgrid/Unit#0
File: .cursorrules:0-0
Timestamp: 2025-08-12T03:33:40.238Z
Learning: Applies to tsconfig.json : Enable strict typing in tsconfig.json
Applied to files:
tsconfig.json
📚 Learning: 2025-08-12T03:34:25.992Z
Learnt from: CR
PR: Resgrid/Unit#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-12T03:34:25.992Z
Learning: Applies to **/*.tsx : Use rnmapbox/maps for maps and navigation
Applied to files:
package.json
📚 Learning: 2025-08-12T03:33:40.238Z
Learnt from: CR
PR: Resgrid/Unit#0
File: .cursorrules:0-0
Timestamp: 2025-08-12T03:33:40.238Z
Learning: Applies to **/*.tsx : Use rnmapbox/maps for maps or vehicle navigation
Applied to files:
package.json
📚 Learning: 2025-08-12T03:34:25.992Z
Learnt from: CR
PR: Resgrid/Unit#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-12T03:34:25.992Z
Learning: Applies to **/*.tsx : Use lucide-react-native for icons directly in markup; do not use gluestack-ui icon component
Applied to files:
src/components/bluetooth/bluetooth-audio-modal.tsx
📚 Learning: 2025-08-12T03:33:40.238Z
Learnt from: CR
PR: Resgrid/Unit#0
File: .cursorrules:0-0
Timestamp: 2025-08-12T03:33:40.238Z
Learning: Applies to **/*.tsx : Use lucide-react-native for icons directly in markup; do not use the gluestack-ui icon component
Applied to files:
src/components/bluetooth/bluetooth-audio-modal.tsx
🧬 Code graph analysis (15)
src/components/settings/__tests__/unit-selection-bottom-sheet-simple.test.tsx (4)
src/stores/app/core-store.ts (1)
useCoreStore(48-263)src/stores/units/store.ts (1)
useUnitsStore(16-31)src/stores/roles/store.ts (1)
useRolesStore(25-90)src/components/settings/unit-selection-bottom-sheet.tsx (1)
UnitSelectionBottomSheet(27-147)
src/components/settings/__tests__/unit-selection-bottom-sheet.test.tsx (4)
src/stores/app/core-store.ts (1)
useCoreStore(48-263)src/stores/units/store.ts (1)
useUnitsStore(16-31)src/stores/roles/store.ts (1)
useRolesStore(25-90)src/components/settings/unit-selection-bottom-sheet.tsx (1)
UnitSelectionBottomSheet(27-147)
src/services/__tests__/location-foreground-permissions.test.ts (4)
src/services/location.ts (1)
locationService(340-340)src/api/units/unitLocation.ts (1)
setUnitLocation(9-14)src/lib/logging/index.tsx (1)
logger(80-80)src/lib/storage/background-geolocation.ts (1)
loadBackgroundGeolocationState(15-30)
src/services/__tests__/signalr.service.enhanced.test.ts (2)
src/lib/logging/index.tsx (1)
logger(80-80)src/services/signalr.service.ts (1)
SignalRHubConnectConfig(13-18)
src/hooks/use-map-signalr-updates.ts (3)
src/stores/signalr/signalr-store.ts (1)
useSignalRStore(25-208)src/lib/logging/index.tsx (2)
logger(80-80)error(74-76)src/api/mapping/mapping.ts (1)
getMapDataAndMarkers(10-13)
src/services/location.ts (2)
src/lib/storage/background-geolocation.ts (1)
loadBackgroundGeolocationState(15-30)src/lib/logging/index.tsx (1)
logger(80-80)
src/components/status/__tests__/status-bottom-sheet.test.tsx (1)
src/components/status/status-bottom-sheet.tsx (1)
StatusBottomSheet(26-681)
src/stores/status/store.ts (2)
src/lib/logging/index.tsx (2)
logger(80-80)error(74-76)src/stores/app/core-store.ts (1)
useCoreStore(48-263)
src/components/status/status-bottom-sheet.tsx (2)
src/components/ui/button/index.tsx (2)
Button(334-334)ButtonText(334-334)src/components/ui/index.tsx (1)
ScrollView(18-18)
src/hooks/__tests__/use-map-signalr-updates.test.ts (4)
src/api/mapping/mapping.ts (1)
getMapDataAndMarkers(10-13)src/lib/logging/index.tsx (2)
logger(80-80)error(74-76)src/stores/signalr/signalr-store.ts (1)
useSignalRStore(25-208)src/hooks/use-map-signalr-updates.ts (1)
useMapSignalRUpdates(11-136)
src/components/bluetooth/bluetooth-audio-modal.tsx (3)
src/services/bluetooth-audio.service.ts (1)
bluetoothAudioService(1667-1667)src/lib/logging/index.tsx (1)
error(74-76)src/stores/app/bluetooth-audio-store.ts (1)
BluetoothAudioDevice(17-25)
src/services/__tests__/location.test.ts (1)
src/services/location.ts (1)
locationService(340-340)
src/components/settings/unit-selection-bottom-sheet.tsx (3)
src/stores/units/store.ts (1)
useUnitsStore(16-31)src/stores/app/core-store.ts (1)
useCoreStore(48-263)src/lib/logging/index.tsx (2)
error(74-76)logger(80-80)
src/services/__tests__/signalr.service.test.ts (1)
src/services/signalr.service.ts (1)
signalRService(512-512)
src/services/signalr.service.ts (1)
src/lib/logging/index.tsx (2)
logger(80-80)error(74-76)
🪛 actionlint (1.7.7)
.github/workflows/react-native-cicd.yml
282-282: property "release_notes" is not defined in object type {buildtype: string; platform: string}
(expression)
🪛 LanguageTool
docs/signalr-service-refactoring.md
[grammar] ~19-~19: There might be a mistake here.
Context: ...ts`) #### Thread-Safe Singleton Pattern - Added proper singleton instance manageme...
(QB_NEW_EN)
[grammar] ~24-~24: There might be a mistake here.
Context: ...tiple instances #### Connection Locking - Added connectionLocks Map to prevent c...
(QB_NEW_EN)
[grammar] ~29-~29: There might be a mistake here.
Context: ...ctions #### Improved Reconnection Logic - Enhanced handleConnectionClose() with ...
(QB_NEW_EN)
[grammar] ~35-~35: There might be a mistake here.
Context: ...during delay #### Better Error Handling - Enhanced logging for all connection stat...
(QB_NEW_EN)
[grammar] ~36-~36: There might be a mistake here.
Context: ...hanced logging for all connection states - Improved error context in log messages -...
(QB_NEW_EN)
[grammar] ~37-~37: There might be a mistake here.
Context: ...- Improved error context in log messages - Added proper cleanup on connection failu...
(QB_NEW_EN)
[grammar] ~46-~46: There might be a mistake here.
Context: ...date events #### Concurrency Prevention - Added isUpdating ref to prevent multip...
(QB_NEW_EN)
[grammar] ~50-~50: There might be a mistake here.
Context: ...ive at a time #### Request Cancellation - Added AbortController support to cance...
(QB_NEW_EN)
[grammar] ~55-~55: There might be a mistake here.
Context: ...ontrollers #### Enhanced Error Handling - Added special handling for AbortError ...
(QB_NEW_EN)
[grammar] ~60-~60: There might be a mistake here.
Context: ...recovery mechanisms #### Proper Cleanup - Added cleanup for debounce timers on unm...
(QB_NEW_EN)
[grammar] ~75-~75: There might be a mistake here.
Context: ...leaks ## Testing ### New Test Coverage - Comprehensive test suite for `useMapSign...
(QB_NEW_EN)
[grammar] ~76-~76: There might be a mistake here.
Context: ...r useMapSignalRUpdates hook (14 tests) - Tests for debouncing, concurrency preven...
(QB_NEW_EN)
[grammar] ~77-~77: There might be a mistake here.
Context: ... prevention, error handling, and cleanup - Tests for AbortController integration - ...
(QB_NEW_EN)
[grammar] ~78-~78: There might be a mistake here.
Context: ... - Tests for AbortController integration - Tests for edge cases and error scenarios...
(QB_NEW_EN)
[grammar] ~81-~81: There might be a mistake here.
Context: ...rios ### Enhanced SignalR Service Tests - Added tests for singleton behavior - Add...
(QB_NEW_EN)
[grammar] ~89-~89: There might be a mistake here.
Context: ...c ## Configuration ### Debounce Timing - Default debounce delay: 1000ms (configur...
(QB_NEW_EN)
[grammar] ~90-~90: There might be a mistake here.
Context: ...figurable via DEBOUNCE_DELAY constant) - Can be adjusted based on performance req...
(QB_NEW_EN)
[grammar] ~93-~93: There might be a mistake here.
Context: ... requirements ### Reconnection Settings - Max reconnection attempts: 5 (unchanged)...
(QB_NEW_EN)
[grammar] ~94-~94: There might be a mistake here.
Context: ...Max reconnection attempts: 5 (unchanged) - Reconnection interval: 5000ms (unchanged...
(QB_NEW_EN)
[grammar] ~95-~95: There might be a mistake here.
Context: ...econnection interval: 5000ms (unchanged) - Enhanced with better cleanup and state m...
(QB_NEW_EN)
[grammar] ~101-~101: There might be a mistake here.
Context: ...s are backward compatible: - Public API of SignalR service remains unchanged - Map...
(QB_NEW_EN)
[grammar] ~120-~120: There might be a mistake here.
Context: ...ary API calls, better request management 2. Better User Experience: Faster map upd...
(QB_NEW_EN)
[grammar] ~121-~121: There might be a mistake here.
Context: ... Faster map updates, reduced server load 3. Enhanced Reliability: Better error han...
(QB_NEW_EN)
[grammar] ~122-~122: There might be a mistake here.
Context: ...handling, improved connection management 4. Memory Efficiency: Proper cleanup prev...
(QB_NEW_EN)
[grammar] ~123-~123: There might be a mistake here.
Context: ...**: Proper cleanup prevents memory leaks 5. Thread Safety: Singleton pattern preve...
(QB_NEW_EN)
[grammar] ~124-~124: There might be a mistake here.
Context: ...ngleton pattern prevents race conditions 6. Testability: Comprehensive test covera...
(QB_NEW_EN)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test
| #### Thread-Safe Singleton Pattern | ||
| - Added proper singleton instance management with race condition protection | ||
| - Added `resetInstance()` method for testing purposes | ||
| - Improved singleton creation with polling mechanism to prevent multiple instances | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Align docs with the singleton implementation (remove “polling” language).
Code should not use a blocking polling loop for singleton creation. Update this section to describe simple lazy initialization.
🧰 Tools
🪛 LanguageTool
[grammar] ~19-~19: There might be a mistake here.
Context: ...ts`) #### Thread-Safe Singleton Pattern - Added proper singleton instance manageme...
(QB_NEW_EN)
🤖 Prompt for AI Agents
In docs/signalr-service-refactoring.md around lines 19 to 23, the section
currently says the singleton uses a "polling mechanism" to prevent multiple
instances; change the wording to describe simple lazy initialization instead:
remove any mention of polling or blocking loops, state that the singleton uses a
thread-safe lazy initialization (e.g., double-checked locking or equivalent
lock/volatile pattern) to prevent race conditions, keep mention of
resetInstance() for testing, and clarify that instance creation is non-blocking
and coordinated via a lock rather than by polling.
| ## Testing | ||
|
|
||
| ### New Test Coverage | ||
| - Comprehensive test suite for `useMapSignalRUpdates` hook (14 tests) | ||
| - Tests for debouncing, concurrency prevention, error handling, and cleanup | ||
| - Tests for AbortController integration | ||
| - Tests for edge cases and error scenarios | ||
|
|
||
| ### Enhanced SignalR Service Tests | ||
| - Added tests for singleton behavior | ||
| - Added tests for connection locking | ||
| - Enhanced existing test coverage | ||
| - Added tests for improved reconnection logic | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify referenced tests and exports exist (avoid stale docs).
Docs mention the hook tests and DEBOUNCE_DELAY. Please confirm files/exports are present.
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Checking hook file and export..."
fd use-map-signalr-updates.ts src | sed -n '1p' | xargs -I{} rg -n "export const DEBOUNCE_DELAY" {}
echo "Checking hook tests..."
fd -e ts -e tsx "__tests__" src/hooks | xargs -I{} rg -n "describe\\(.*useMapSignalRUpdates" {} -n -C1 || true
echo "Checking enhanced service tests..."
fd -e ts "signalr.service.enhanced.test.ts" src/services/__tests__ -x echo {}Length of output: 198
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "1. Locate hook file containing useMapSignalRUpdates:"
rg -l "function useMapSignalRUpdates" -t ts || true
echo "2. Verify DEBOUNCE_DELAY export in source:"
rg -n "export const DEBOUNCE_DELAY" -t ts || true
echo "3. List all hook test files:"
find src/hooks -type f \( -name "*.test.ts" -o -name "*.spec.ts" -o -name "*.test.tsx" -o -name "*.spec.tsx" \) -print || true
echo "4. Search hook tests for useMapSignalRUpdates:"
rg -n "useMapSignalRUpdates" -C1 --glob "src/hooks/**/*.{test,spec}.{ts,tsx}" || true
echo "5. List all service test files:"
find src/services -type f \( -name "*.test.ts" -o -name "*.spec.ts" \) -print || true
echo "6. Search service tests for SignalR keywords (singleton, reconnect, connection):"
rg -nE "singleton|reconnect|connection" --glob "src/services/**/*.{test,spec}.ts" || trueLength of output: 6618
Remove stale DEBOUNCE_DELAY reference or add its export.
- No
export const DEBOUNCE_DELAYfound insrc/hooks/use-map-signalr-updates.ts—either export it or remove it from the docs. - Hook tests (
src/hooks/__tests__/use-map-signalr-updates.test.ts) and enhanced service tests (src/services/__tests__/signalr.service.enhanced.test.ts) are present.
🧰 Tools
🪛 LanguageTool
[grammar] ~75-~75: There might be a mistake here.
Context: ...leaks ## Testing ### New Test Coverage - Comprehensive test suite for `useMapSign...
(QB_NEW_EN)
[grammar] ~76-~76: There might be a mistake here.
Context: ...r useMapSignalRUpdates hook (14 tests) - Tests for debouncing, concurrency preven...
(QB_NEW_EN)
[grammar] ~77-~77: There might be a mistake here.
Context: ... prevention, error handling, and cleanup - Tests for AbortController integration - ...
(QB_NEW_EN)
[grammar] ~78-~78: There might be a mistake here.
Context: ... - Tests for AbortController integration - Tests for edge cases and error scenarios...
(QB_NEW_EN)
[grammar] ~81-~81: There might be a mistake here.
Context: ...rios ### Enhanced SignalR Service Tests - Added tests for singleton behavior - Add...
(QB_NEW_EN)
🤖 Prompt for AI Agents
In docs/signalr-service-refactoring.md around lines 73 to 86, the documentation
references DEBOUNCE_DELAY which is not exported from
src/hooks/use-map-signalr-updates.ts; either remove the stale DEBOUNCE_DELAY
mention from the docs or export it from the hook file. To fix: if tests rely on
the constant, add a named export in src/hooks/use-map-signalr-updates.ts (export
const DEBOUNCE_DELAY = <current value>) and run tests; otherwise, delete the
DEBOUNCE_DELAY bullet from the docs and any README/test references to keep docs
and code consistent.
src/components/settings/__tests__/unit-selection-bottom-sheet-simple.test.tsx
Outdated
Show resolved
Hide resolved
src/components/settings/__tests__/unit-selection-bottom-sheet.test.tsx
Outdated
Show resolved
Hide resolved
src/components/settings/__tests__/unit-selection-bottom-sheet.test.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/services/location.ts (1)
196-215: Make foreground subscription idempotent; prevent duplicate watchPosition subscriptionsIf startLocationUpdates is called more than once, a prior subscription is not removed, causing duplicate callbacks.
- // Start foreground updates - this.locationSubscription = await Location.watchPositionAsync( + // Start foreground updates + if (this.locationSubscription) { + await this.locationSubscription.remove(); + this.locationSubscription = null; + } + this.locationSubscription = await Location.watchPositionAsync( { accuracy: Location.Accuracy.Balanced, timeInterval: 15000, distanceInterval: 10, }, (location) => { logger.info({ message: 'Foreground location update received', context: { latitude: location.coords.latitude, longitude: location.coords.longitude, heading: location.coords.heading, }, }); useLocationStore.getState().setLocation(location); - sendLocationToAPI(location); // Send to API for foreground updates + void sendLocationToAPI(location); // fire-and-forget; errors are handled internally } );src/components/settings/unit-selection-bottom-sheet.tsx (1)
52-77: Make selection idempotent; add user feedback on failure
- Use the ref guard to ensure a single in-flight selection.
- Consider surfacing an alert/toast on error per “Handle errors gracefully” guideline.
Apply this diff:
const handleUnitSelection = React.useCallback( async (unit: UnitResultData) => { - if (isLoading) { - return; - } + if (isSelectingRef.current) return; + isSelectingRef.current = true; try { setIsLoading(true); await setActiveUnit(unit.UnitId); await useRolesStore.getState().fetchRolesForUnit(unit.UnitId); logger.info({ message: 'Active unit updated successfully', context: { unitId: unit.UnitId }, }); handleClose(); } catch (error) { logger.error({ message: 'Failed to update active unit', context: { error }, }); + // TODO: Surface feedback to the user (toast or Alert + i18n) } finally { setIsLoading(false); + isSelectingRef.current = false; } }, [setActiveUnit, handleClose, isLoading] );If you want, I can wire a Toast/Alert with i18n keys.
src/services/signalr.service.ts (2)
107-112: Do not rewrite “+” to “%20” in query strings.
URLSearchParams.toString()legitimately encodes spaces as+. Blind replacement corrupts literal plus signs.- if (queryParams.toString()) { - // Manually encode to ensure spaces are encoded as %20 instead of + - const queryString = queryParams.toString().replace(/\+/g, '%20'); - fullUrl = `${fullUrl}?${queryString}`; - } + const qs = queryParams.toString(); + if (qs) { + fullUrl = `${fullUrl}?${qs}`; + }
296-346: Bug: scheduled reconnect ignores later explicit disconnects.
hubConfigis captured beforesetTimeout. IfdisconnectFromHubruns and removes config, the callback still reconnects. Re-check config inside the timeout and bail if removed.- const hubConfig = this.hubConfigs.get(hubName); - if (hubConfig) { + const hubConfig = this.hubConfigs.get(hubName); + if (hubConfig) { logger.info({ message: `Scheduling reconnection attempt ${currentAttempts}/${this.MAX_RECONNECT_ATTEMPTS} for hub: ${hubName}`, }); setTimeout(async () => { try { + // Honor explicit disconnects: if config was removed, abort + const cfg = this.hubConfigs.get(hubName); + if (!cfg) { + logger.debug({ + message: `Reconnection cancelled for hub: ${hubName} (config removed)`, + }); + return; + } // Check if connection was re-established during the delay if (this.connections.has(hubName)) { logger.debug({ message: `Hub ${hubName} is already connected, skipping reconnection attempt`, }); return; } @@ - await this.connectToHubWithEventingUrl(hubConfig); + await this.connectToHubWithEventingUrl(cfg);
♻️ Duplicate comments (2)
src/services/__tests__/signalr.service.enhanced.test.ts (1)
33-48: Good fix: stable logger mock provided.This addresses the prior flakiness risk where logger methods could be undefined in tests.
src/services/signalr.service.ts (1)
33-46: LGTM: simplified singleton without busy-wait.Removes the JS-thread blocking guard noted previously.
🧹 Nitpick comments (32)
src/components/status/status-bottom-sheet.tsx (3)
506-510: Remove redundant ternary in icon color
colorScheme === 'dark' ? '#fff' : '#fff'is identical on both branches.- <ArrowRight size={14} color={colorScheme === 'dark' ? '#fff' : '#fff'} /> + <ArrowRight size={14} color="#fff" />Optional: if your Button supports sizes, prefer size props over manual padding for consistency:
- <Button onPress={handleNext} isDisabled={!canProceedFromCurrentStep()} className="bg-blue-600 px-4 py-2"> + <Button onPress={handleNext} isDisabled={!canProceedFromCurrentStep()} size="sm" className="bg-blue-600">
616-620: Same nit: simplify icon color, consider size propDuplicate of the earlier button: remove the no-op ternary and prefer size prop if available.
- <Button onPress={handleNext} isDisabled={!canProceedFromCurrentStep()} className="bg-blue-600 px-4 py-2"> + <Button onPress={handleNext} isDisabled={!canProceedFromCurrentStep()} size="sm" className="bg-blue-600"> - <ArrowRight size={14} color={colorScheme === 'dark' ? '#fff' : '#fff'} /> + <ArrowRight size={14} color="#fff" />
670-678: Minor polish: icon color ternary and tap target
- The ArrowLeft color ternary returns the same color.
- With
px-3, ensure the overall button still meets a 44pt min tap target; using a size prop is safer.- <Button variant="outline" onPress={handlePrevious} className="px-3" isDisabled={isSubmitting}> - <ArrowLeft size={14} color={colorScheme === 'dark' ? '#737373' : '#737373'} /> + <Button variant="outline" size="sm" onPress={handlePrevious} className="px-3" isDisabled={isSubmitting}> + <ArrowLeft size={14} color="#737373" />And similarly for the submit button:
- <Button onPress={handleSubmit} isDisabled={!canProceedFromCurrentStep() || isSubmitting} className="bg-blue-600 px-3"> + <Button onPress={handleSubmit} isDisabled={!canProceedFromCurrentStep() || isSubmitting} size="sm" className="bg-blue-600">src/services/location.ts (5)
129-137: Type the background permission status explicitlyUse Location.PermissionStatus for backgroundStatus to avoid widening to string and to keep strong typing.
- let backgroundStatus = 'undetermined'; + let backgroundStatus: Location.PermissionStatus = 'undetermined';
173-181: iOS UX: surface the system indicator for background trackingConsider enabling the iOS indicator to meet platform expectations and reduce confusion when tracking runs in background.
await Location.startLocationUpdatesAsync(LOCATION_TASK_NAME, { accuracy: Location.Accuracy.Balanced, timeInterval: 15000, distanceInterval: 10, foregroundService: { notificationTitle: 'Location Tracking', notificationBody: 'Tracking your location in the background', }, + showsBackgroundLocationIndicator: true, // iOS });
196-224: Skip foreground watch when background task is active to avoid duplicatesIf shouldEnableBackground is true (TaskManager task registered), you can rely on that stream even while foregrounded and skip watchPosition entirely.
- // Start foreground updates - this.locationSubscription = await Location.watchPositionAsync( + // Start foreground updates only if not relying on TaskManager background task + if (!shouldEnableBackground) { + this.locationSubscription = await Location.watchPositionAsync( { accuracy: Location.Accuracy.Balanced, timeInterval: 15000, distanceInterval: 10, }, (location) => { logger.info({ message: 'Foreground location update received', context: { latitude: location.coords.latitude, longitude: location.coords.longitude, heading: location.coords.heading, }, }); useLocationStore.getState().setLocation(location); - sendLocationToAPI(location); // Send to API for foreground updates + void sendLocationToAPI(location); // Send to API for foreground updates } - ); + ); + } else { + logger.info({ message: 'Using TaskManager background task for updates; skipping foreground watch' }); + }Note: This will require aligning tests that currently expect a foreground watch even when background is enabled. -->
270-286: Honor “canAskAgain” when requesting background permissionsIf the OS disallows further prompts (canAskAgain: false), consider surfacing a user-facing action to open Settings instead of silently returning.
- const { status: backgroundStatus } = await Location.requestBackgroundPermissionsAsync(); - const hasBackgroundPermissions = backgroundStatus === 'granted'; + const { status: backgroundStatus, canAskAgain } = await Location.requestBackgroundPermissionsAsync(); + const hasBackgroundPermissions = backgroundStatus === 'granted'; + if (!hasBackgroundPermissions && canAskAgain === false) { + logger.warn({ + message: 'Background permissions permanently denied; prompt user to enable in Settings', + context: { backgroundStatus }, + }); + }
303-306: AppState nuance: consider “inactive” on iOS transitionsOn iOS, apps can briefly be inactive before background; if you intend to start background updates in that window, include it.
- if (AppState.currentState === 'background') { + if (AppState.currentState === 'background' || AppState.currentState === 'inactive') { await this.startBackgroundUpdates(); }src/services/__tests__/location-foreground-permissions.test.ts (1)
237-256: This test doesn’t exercise production code; consider removing or rewritingThe “old incorrect behavior” test asserts a local mock function, not the service logic. It adds noise without preventing regressions.
- it('should log the exact error from user logs when permission check was wrong', async () => { - // ...local mock demonstration... - }); + // Consider removing this test, or rewrite to assert requestPermissions() does not require background permission.src/services/__tests__/location.test.ts (3)
353-377: Background task registration path is validatedConsider adding an assertion that foreground watchPosition isn’t started when the background task is active to prevent double streams (after applying the suggested service change).
590-603: Enabling background tracking with granted permissions is coveredAdd a companion test to assert we don’t start watchPosition when relying on TaskManager (post-fix).
678-751: Comprehensive foreground-only mode suiteGreat coverage. After the service change to avoid duplicate streams, consider adding an assertion that only one update source fires per location (store/API called once).
src/components/settings/unit-selection-bottom-sheet.tsx (8)
82-82: Use design tokens instead of hard-coded colorsAlign Actionsheet background to gluestack tokens to keep themes consistent.
Apply this diff (adjust tokens to your theme):
- <ActionsheetContent className="bg-white dark:bg-gray-900"> + <ActionsheetContent className="bg-background-0 dark:bg-background-950">
93-105: Follow conditional rendering guideline (use ?:, not &&)Replace the && pattern with a ternary for consistency.
Apply this diff:
- {activeUnit && ( + {activeUnit ? ( <Box className="rounded-lg border border-outline-200 bg-background-50 p-3"> <VStack space="xs"> <Text size="sm" className="font-medium text-typography-900"> {t('settings.current_unit')} </Text> <Text size="sm" className="text-typography-600"> {activeUnit.Name} </Text> </VStack> </Box> - )} + ) : null}
108-141: Consider FlatList for scalability and to avoid mapping with inline handlersIf units can grow, FlatList is preferable to ScrollView. It also gives getItemLayout, windowing, etc. Additionally, avoid inline lambdas in render to prevent needless re-renders.
Example (outline only):
// outside render const renderItem = React.useCallback(({ item }: { item: UnitResultData }) => ( <UnitListItem unit={item} isSelected={activeUnit?.UnitId === item.UnitId} onSelect={handleUnitSelection} disabled={isLoading} /> ), [activeUnit?.UnitId, handleUnitSelection, isLoading]); // in JSX <FlatList data={units} keyExtractor={(u) => u.UnitId} renderItem={renderItem} removeClippedSubviews maxToRenderPerBatch={20} windowSize={5} />I can provide a concrete diff if you decide to switch.
120-121: Active-row highlight class is a no-op as writtenUsing data-[checked=true]:… without setting the data attribute won’t apply. Apply the background unconditionally when selected.
Apply this diff:
- className={activeUnit?.UnitId === unit.UnitId ? 'data-[checked=true]:bg-background-100' : ''} + className={activeUnit?.UnitId === unit.UnitId ? 'bg-background-100' : undefined}
30-30: Surface fetch error state with retryDestructure error from the units store and show an error UI with a retry action instead of falling through to “no units available.”
Apply this diff:
- const { units, fetchUnits, isLoading: isLoadingUnits } = useUnitsStore(); + const { units, fetchUnits, isLoading: isLoadingUnits, error: unitsError } = useUnitsStore();- {isLoadingUnits ? ( + {isLoadingUnits ? ( <Center className="py-8"> <Spinner size="large" /> </Center> - ) : units.length > 0 ? ( + ) : unitsError ? ( + <VStack space="sm" className="py-8 px-2"> + <Text className="text-center text-typography-500">{t('settings.units_fetch_error')}</Text> + <Button onPress={fetchUnits} variant="solid" className="self-center"> + <ButtonText>{t('common.retry')}</ButtonText> + </Button> + </VStack> + ) : units.length > 0 ? ( <VStack space="sm">I can add the i18n keys if they’re not present.
Also applies to: 109-140
124-129: Localize the unit type label if it’s user-facingIf unit.Type is an enum (e.g., “Engine”, “Ambulance”), map it through i18n for consistency.
Example:
<ActionsheetItemText size="sm" className="text-typography-500"> {t(`units.type.${unit.Type}`, { defaultValue: unit.Type })} </ActionsheetItemText>
27-27: Align with project guideline: use React.FC for componentsRefactor to define as React.FC and memoize the component reference.
Apply this diff:
-export const UnitSelectionBottomSheet = React.memo<UnitSelectionBottomSheetProps>(({ isOpen, onClose }) => { +const UnitSelectionBottomSheetComponent: React.FC<UnitSelectionBottomSheetProps> = ({ isOpen, onClose }) => {-}); - -UnitSelectionBottomSheet.displayName = 'UnitSelectionBottomSheet'; +}; +export const UnitSelectionBottomSheet = React.memo(UnitSelectionBottomSheetComponent); +UnitSelectionBottomSheet.displayName = 'UnitSelectionBottomSheet';Also applies to: 153-156
116-121: Avoid inline onPress lambdas within list renderingExtract a memoized UnitListItem component and pass a stable onSelect(unitId) handler to minimize re-renders.
I can provide a concrete diff introducing a memoized UnitListItem if you want to adopt this.
src/hooks/__tests__/use-map-signalr-updates.test.ts (3)
251-286: Test name doesn’t match behavior; not asserting cancellation.The case “should cancel previous request when new update comes in” never triggers a second timestamp or verifies
abort()—it only checks that an AbortController was created. Either rename or assert actual cancellation.Apply:
-it('should cancel previous request when new update comes in', async () => { +it('should create an AbortController per request', async () => {If you intend to verify cancellation, simulate a second timestamp and assert
mockAbortwas called, e.g.:// after first render + runAllTimers() rerender(() => { mockUseSignalRStore.mockReturnValue(timestamp1 + 1); return useMapSignalRUpdates(mockOnMarkersUpdate); }); jest.runAllTimers(); expect(mockAbort).toHaveBeenCalled();
64-66: Restore real timers after suite.Avoid leaking fake timers across other suites.
afterEach(() => { jest.runOnlyPendingTimers(); }); + +afterAll(() => { + jest.useRealTimers(); +});
89-95: Make signal assertion less brittle.
expect.objectContaining({ aborted: false })depends on internal shape. Prefer asserting that a signal-like object is passed, then check.abortedis boolean.Example:
const signalArg = mockGetMapDataAndMarkers.mock.calls[0][0]; expect(typeof signalArg?.aborted).toBe('boolean');Also applies to: 123-129, 185-193, 211-219, 237-245, 305-311, 323-327, 345-358, 439-447
src/app/(app)/index.tsx (1)
193-193: Solid unmount-safety via AbortController; prefer axios.isCancel for robustness.String-matching “canceled” is brittle across axios versions.
+import axios from 'axios'; ... - if (error instanceof Error && (error.name === 'AbortError' || error.message === 'canceled')) { + if (error instanceof Error && (error.name === 'AbortError' || axios.isCancel?.(error))) {Also applies to: 197-214, 220-224
src/api/mapping/mapping.ts (1)
15-23: Minor: avoid encoding numeric query param; optional alias for typo’d name.
encodeURIComponent(type)is unnecessary for numbers. Also consider exporting an aliasgetMapLayersto avoid the “May” typo without breaking imports.- type: encodeURIComponent(type), + type,Optional alias:
export const getMapLayers = getMayLayers;src/hooks/use-map-signalr-updates.ts (2)
8-10: Export the debounce constant to avoid test duplication.Prevents magic numbers in tests/assertions.
-const DEBOUNCE_DELAY = 1000; +export const DEBOUNCE_DELAY = 1000;
46-53: Redundant aborted check inside try.Aborted axios/fetch calls reject; the explicit post-await
signal.abortedcheck is unnecessary. Keeping only the catch-path handling simplifies flow.- // Check if request was aborted - if (abortController.current?.signal.aborted) { - logger.debug({ - message: 'Map markers request was aborted', - context: { timestamp: lastUpdateTimestamp }, - }); - return; - }Also applies to: 71-86
src/services/__tests__/signalr.service.enhanced.test.ts (3)
32-32: Optional: provide an explicit @microsoft/signalr mock factory for clarity.Auto-mocking works, but an explicit factory avoids surprises with constants/enums and keeps types stable.
-jest.mock('@microsoft/signalr'); +jest.mock('@microsoft/signalr', () => { + return { + HubConnectionBuilder: jest.fn(), + LogLevel: { Information: 1 }, + }; +});
60-96: Guard against leaked fake timers between tests.If a test fails before calling
useRealTimers(), later tests may inherit fake timers. Add a global safety in teardown.beforeEach(() => { jest.clearAllMocks(); @@ mockGetState.mockReturnValue({ accessToken: 'mock-token', refreshAccessToken: mockRefreshAccessToken, }); }); + + afterEach(() => { + jest.useRealTimers(); + });
1-1: Remove unused import.
LogLevelisn’t used in this test file.-import { HubConnection, HubConnectionBuilder, LogLevel } from '@microsoft/signalr'; +import { HubConnection, HubConnectionBuilder } from '@microsoft/signalr';src/services/signalr.service.ts (2)
156-170: Lower log level for payloads to avoid noisy/PII logs.Consider logging received data at
debuginstead ofinfo.- connection.on(method, (data) => { - logger.info({ + connection.on(method, (data) => { + logger.debug({ message: `Received ${method} message from hub: ${config.name}`, context: { method, data }, }); this.handleMessage(config.name, method, data); });
436-451: Make resetInstance await disconnects to avoid stray tasks in tests.Returning a promise prevents overlap with new instances in fast test setups.
- public static resetInstance(): void { + public static async resetInstance(): Promise<void> { if (SignalRService.instance) { - // Disconnect all connections before resetting - SignalRService.instance.disconnectAll().catch((error) => { - logger.error({ - message: 'Error disconnecting all hubs during instance reset', - context: { error }, - }); - }); + try { + await SignalRService.instance.disconnectAll(); + } catch (error) { + logger.error({ + message: 'Error disconnecting all hubs during instance reset', + context: { error }, + }); + } } SignalRService.instance = null; logger.debug({ message: 'SignalR service singleton instance reset', }); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (14)
src/api/common/client.tsx(1 hunks)src/api/mapping/mapping.ts(1 hunks)src/app/(app)/index.tsx(1 hunks)src/components/settings/__tests__/unit-selection-bottom-sheet-simple.test.tsx(1 hunks)src/components/settings/__tests__/unit-selection-bottom-sheet.test.tsx(1 hunks)src/components/settings/unit-selection-bottom-sheet.tsx(4 hunks)src/components/status/status-bottom-sheet.tsx(9 hunks)src/hooks/__tests__/use-map-signalr-updates.test.ts(1 hunks)src/hooks/use-map-signalr-updates.ts(1 hunks)src/services/__tests__/location-foreground-permissions.test.ts(1 hunks)src/services/__tests__/location.test.ts(12 hunks)src/services/__tests__/signalr.service.enhanced.test.ts(1 hunks)src/services/location.ts(3 hunks)src/services/signalr.service.ts(8 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/components/settings/tests/unit-selection-bottom-sheet-simple.test.tsx
- src/components/settings/tests/unit-selection-bottom-sheet.test.tsx
🧰 Additional context used
📓 Path-based instructions (7)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursorrules)
**/*.{ts,tsx}: Write concise, type-safe TypeScript code
Use camelCase for variable and function names (e.g., isFetchingData, handleUserInput)
Use TypeScript for all components, favoring interfaces for props and state
Avoid using any; strive for precise types
Use Expo SecureStore for sensitive data
Use zustand for state management
Use react-query for data fetching and caching
Use react-i18next for internationalization
Use react-native-mmkv for local storage
Use axios for API requests
**/*.{ts,tsx}: Write concise, type-safe TypeScript code
Use camelCase for variable and function names
Use TypeScript for all components, favoring interfaces for props and state
Avoid using any; strive for precise types
Use React Navigation for handling navigation and deep linking
Handle errors gracefully and provide user feedback
Use Expo's SecureStore for sensitive data
Use zustand for state management
Use react-query for data fetching
Use react-i18next for internationalization
Use react-native-mmkv for local storage
Use axios for API requests
Files:
src/api/common/client.tsxsrc/api/mapping/mapping.tssrc/app/(app)/index.tsxsrc/services/__tests__/location-foreground-permissions.test.tssrc/services/__tests__/signalr.service.enhanced.test.tssrc/components/settings/unit-selection-bottom-sheet.tsxsrc/hooks/use-map-signalr-updates.tssrc/services/__tests__/location.test.tssrc/services/location.tssrc/components/status/status-bottom-sheet.tsxsrc/services/signalr.service.tssrc/hooks/__tests__/use-map-signalr-updates.test.ts
**/*.tsx
📄 CodeRabbit inference engine (.cursorrules)
**/*.tsx: Use functional components and hooks over class components
Use PascalCase for React component names (e.g., UserProfile, ChatScreen)
Utilize React.FC for defining functional components with props
Minimize useEffect, useState, and heavy computations inside render
Use React.memo() for components with static props to prevent unnecessary re-renders
Optimize FlatList with removeClippedSubviews, maxToRenderPerBatch, windowSize
Use getItemLayout for FlatList when items have consistent size
Avoid anonymous functions in renderItem or event handlers to prevent re-renders
Use gluestack-ui for styling; if no component exists in src/components/ui, style via StyleSheet.create or styled-components
Optimize images using react-native-fast-image
Use React Navigation for navigation and deep linking with best practices
Wrap all user-facing text in t() from react-i18next
Use react-hook-form for form handling
Use @rnmapbox/maps for maps or vehicle navigation
Use lucide-react-native for icons directly in markup; do not use the gluestack-ui icon component
Use the conditional operator (?:) for conditional rendering instead of &&
**/*.tsx: Use functional components and hooks over class components
Use PascalCase for component names
Utilize React.FC for defining functional components with props
Minimize useEffect/useState and heavy computations inside render
Use React.memo for components with static props to prevent unnecessary re-renders
Optimize FlatList with removeClippedSubviews, maxToRenderPerBatch, and windowSize
Use getItemLayout for FlatList when items have consistent size
Avoid anonymous functions in renderItem or event handlers
Optimize image handling using react-native-fast-image
Wrap all user-facing text in t() from react-i18next
Ensure support for dark mode and light mode
Ensure the app is accessible following WCAG for mobile
Use react-hook-form for form handling
Use @rnmapbox/maps for maps and navigation
Use lucide-react-native for icons directly in markup; do not us...
Files:
src/api/common/client.tsxsrc/app/(app)/index.tsxsrc/components/settings/unit-selection-bottom-sheet.tsxsrc/components/status/status-bottom-sheet.tsx
**/*
📄 CodeRabbit inference engine (.cursorrules)
Directory and file names should be lowercase and hyphenated (e.g., user-profile, chat-screen)
Files:
src/api/common/client.tsxsrc/api/mapping/mapping.tssrc/app/(app)/index.tsxsrc/services/__tests__/location-foreground-permissions.test.tssrc/services/__tests__/signalr.service.enhanced.test.tssrc/components/settings/unit-selection-bottom-sheet.tsxsrc/hooks/use-map-signalr-updates.tssrc/services/__tests__/location.test.tssrc/services/location.tssrc/components/status/status-bottom-sheet.tsxsrc/services/signalr.service.tssrc/hooks/__tests__/use-map-signalr-updates.test.ts
{components/ui/**/*.{ts,tsx},**/*.tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use gluestack-ui consistently; if no component exists in components/ui, style via StyleSheet.create or styled-components
Files:
src/api/common/client.tsxsrc/app/(app)/index.tsxsrc/components/settings/unit-selection-bottom-sheet.tsxsrc/components/status/status-bottom-sheet.tsx
**/*.{test.ts,test.tsx,spec.ts,spec.tsx}
📄 CodeRabbit inference engine (.cursorrules)
Create and use Jest tests to validate all generated components
Files:
src/services/__tests__/location-foreground-permissions.test.tssrc/services/__tests__/signalr.service.enhanced.test.tssrc/services/__tests__/location.test.tssrc/hooks/__tests__/use-map-signalr-updates.test.ts
**/__tests__/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursorrules)
Generate tests for all components, services, and logic; ensure tests run without errors
Files:
src/services/__tests__/location-foreground-permissions.test.tssrc/services/__tests__/signalr.service.enhanced.test.tssrc/services/__tests__/location.test.tssrc/hooks/__tests__/use-map-signalr-updates.test.ts
**/*.{test,spec}.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Create and use Jest tests for all generated components
Files:
src/services/__tests__/location-foreground-permissions.test.tssrc/services/__tests__/signalr.service.enhanced.test.tssrc/services/__tests__/location.test.tssrc/hooks/__tests__/use-map-signalr-updates.test.ts
🧬 Code graph analysis (10)
src/app/(app)/index.tsx (2)
src/api/mapping/mapping.ts (1)
getMapDataAndMarkers(10-13)src/lib/logging/index.tsx (2)
error(74-76)logger(80-80)
src/services/__tests__/location-foreground-permissions.test.ts (4)
src/services/location.ts (1)
locationService(347-347)src/api/units/unitLocation.ts (1)
setUnitLocation(9-14)src/lib/logging/index.tsx (1)
logger(80-80)src/lib/storage/background-geolocation.ts (1)
loadBackgroundGeolocationState(15-30)
src/services/__tests__/signalr.service.enhanced.test.ts (2)
src/lib/logging/index.tsx (1)
logger(80-80)src/services/signalr.service.ts (1)
SignalRHubConnectConfig(13-18)
src/components/settings/unit-selection-bottom-sheet.tsx (2)
src/stores/units/store.ts (1)
useUnitsStore(16-31)src/lib/logging/index.tsx (2)
error(74-76)logger(80-80)
src/hooks/use-map-signalr-updates.ts (2)
src/stores/signalr/signalr-store.ts (1)
useSignalRStore(25-208)src/api/mapping/mapping.ts (1)
getMapDataAndMarkers(10-13)
src/services/__tests__/location.test.ts (1)
src/services/location.ts (1)
locationService(347-347)
src/services/location.ts (2)
src/lib/logging/index.tsx (1)
logger(80-80)src/lib/storage/background-geolocation.ts (1)
loadBackgroundGeolocationState(15-30)
src/components/status/status-bottom-sheet.tsx (2)
src/components/ui/button/index.tsx (2)
Button(334-334)ButtonText(334-334)src/components/ui/index.tsx (1)
ScrollView(18-18)
src/services/signalr.service.ts (1)
src/lib/logging/index.tsx (2)
logger(80-80)error(74-76)
src/hooks/__tests__/use-map-signalr-updates.test.ts (4)
src/api/mapping/mapping.ts (1)
getMapDataAndMarkers(10-13)src/lib/logging/index.tsx (2)
logger(80-80)error(74-76)src/stores/signalr/signalr-store.ts (1)
useSignalRStore(25-208)src/hooks/use-map-signalr-updates.ts (1)
useMapSignalRUpdates(11-145)
🔇 Additional comments (33)
src/components/status/status-bottom-sheet.tsx (7)
68-73: Accessor refactor looks goodNarrowing
getStatusPropertyto just 'Detail' | 'Note' simplifies usage and intent. No issues spotted.
75-79: Confirm server/API type for status Type
getStatusId()returns a string. Please confirmSaveUnitStatusInput.Typeis meant to be a string. If the API expects a number, coerce before assignment.If needed:
-const getStatusId = React.useCallback((): string => { +const getStatusId = React.useCallback((): string => { if (!selectedStatus) return '0'; return selectedStatus.Id.toString(); }, [selectedStatus]);And at assignment (see Line 174 comment) convert to number:
- input.Type = getStatusId(); + input.Type = Number(getStatusId());
174-174: Validateinput.Typetype matches backend contractYou’re now passing a string. If the API expects a number, convert; otherwise this is fine.
- input.Type = getStatusId(); + // If string is correct, keep as-is. + // If number is required: + // input.Type = Number(getStatusId());
238-238: Good: dependency array updatedIncluding
getStatusIdin thehandleSubmitdependencies is correct.
442-442: Snap point increase to 90%—sanity-check on small screens90% can crowd content and keyboard. Please verify on small devices and with keyboard visible that content remains reachable/scrollable and not obscured by the action bar.
549-549: Dynamic ScrollView height: verify no overlap with action buttonsThe reduced max height for detailLevel 3 is sensible. Please confirm long lists don’t get hidden behind the bottom action row; add bottom padding if needed.
- <ScrollView className={detailLevel === 3 ? 'max-h-[200px]' : 'max-h-[300px]'}> + <ScrollView + className={detailLevel === 3 ? 'max-h-[200px]' : 'max-h-[300px]'} + contentContainerStyle={{ paddingBottom: 12 }} +>
636-639: Submit button (skip-destination path) update looks fineCompact styling plus proper disabled state and spinner are correct.
src/services/location.ts (1)
169-186: Prevent duplicate location updates by avoiding simultaneous TaskManager and watchPosition streamsWhen background geolocation is enabled,
Location.startLocationUpdatesAsyncemits in both foreground and background. Running the watch-position subscription alongside the TaskManager task will produce duplicate events, increasing battery and network usage.• In
startBackgroundUpdates(), return early ifawait TaskManager.isTaskRegisteredAsync(LOCATION_TASK_NAME)is true.
• Optionally, disable the foregroundwatchPositionsubscription whenshouldEnableBackgroundis true.Add a test to confirm only one API call per location event when both streams could overlap.
src/services/__tests__/location-foreground-permissions.test.ts (4)
203-236: Good regression coverage for foreground-only permissionsValidates that requestPermissions defaults to foreground-only and that tracking starts without background. Looks solid.
257-287: Nice: warns and runs foreground-only when setting enabled but OS denies backgroundAccurately matches the new service behavior and log context.
289-317: End-to-end check of foreground update path is effectiveConfirms store update, API shape, and logging. Good coverage.
381-389: Covers the “no background task when denied” pathPrevents accidental registration under denied state. Good.
src/services/__tests__/location.test.ts (7)
42-53: Adding getBackgroundPermissionsAsync to the mock unblocks new flowsThis aligns the testbed with the service changes. Looks good.
203-211: Default to foreground-only request is asserted correctlyMatches the new API contract of requestPermissions(requestBackground?: boolean).
244-256: Logs for foreground-only requests are asserted preciselyKeeps observability guarantees under test. Good.
301-320: Foreground updates proceed even when background is deniedGood negative-path coverage and logging context checks.
322-341: Warns when setting enabled but background deniedAccurately codifies the degraded-mode behavior. Good.
378-390: Denied background permissions prevent task registrationSolid guardrail test.
638-645: Active-state guard validatedPrevents accidental background updates while app is active. Nice.
src/components/settings/unit-selection-bottom-sheet.tsx (5)
27-31: Good call memoizing the sheet componentReact.memo with a typed props generic and an explicit displayName improves render stability and debug-ability. Looks solid.
33-44: Confirm fetch policy: only load units when the sheet opens and list is emptyIf units can change between openings, consider revalidating on open (or SWR-style refresh) instead of relying on length === 0. Otherwise, stale lists may persist session-long.
Would you like me to scan the repo for other places that refresh units and report back to ensure coverage?
45-51: Close guard looks correctPreventing close while a selection is in-flight avoids UX glitches. Nice.
80-80: snapPoints semantics confirmed
snapPointsfor gluestack-ui’s Actionsheet accepts percentages (0–100); using[85]correctly sets the sheet to 85% of screen height. No code changes needed.
131-132: Replace className prop on lucide-react-native icons with color prop
lucide-react-native icons ignore className; use the color prop instead. For example:- {activeUnit?.UnitId === unit.UnitId && <Check size={20} className="text-primary-600" />} + {activeUnit?.UnitId === unit.UnitId && <Check size={20} color={/* primary-600 token or hex */} />}No
useTokenhelper was found—please import your theme’s primary-600 token (or implement a hook to retrieve it at runtime) and pass it tocolor.src/hooks/__tests__/use-map-signalr-updates.test.ts (1)
131-172: Concurrent-update test will fail once coalescing is fixed.With a proper “queue next update” behavior (see hook comment), this test should expect a second call after the first resolves, not a permanent skip. Update expectations accordingly.
src/api/common/client.tsx (1)
123-127: AbortSignal plumbed correctly through client methods. LGTM.src/api/mapping/mapping.ts (1)
10-13: Pass-through of AbortSignal looks correct.src/services/signalr.service.ts (6)
29-29: Per-hub connection locking is a solid concurrency control.Prevents duplicate connects and races; lock cleanup in
finallyis correct.
49-69: Connect gate: good serialization of concurrent connects.Awaiting the in-flight promise and deleting the lock in
finallyavoids leaks and thundering herd.
188-209: Connect gate (direct URL): consistent and correct.Mirrors the eventing-URL path; good symmetry.
374-389: Disconnect waits on in-flight connects—nice touch.Prevents stop/start races and logs ignored errors appropriately.
411-434: Clarify behavior when invoking without an active connection.Currently a no-op return; consider throwing a descriptive error so callers can react, or confirm that silent failure is desired.
- const connection = this.connections.get(hubName); - if (connection) { - try { - return await connection.invoke(method, data); - } catch (error) { - logger.error({ - message: `Error invoking method ${method} from hub: ${hubName}`, - context: { error }, - }); - throw error; - } - } + const connection = this.connections.get(hubName); + if (!connection) { + const err = new Error(`No active connection for hub: ${hubName}`); + logger.error({ message: 'Invoke failed', context: { hubName, method, error: err } }); + throw err; + } + try { + return await connection.invoke(method, data); + } catch (error) { + logger.error({ + message: `Error invoking method ${method} from hub: ${hubName}`, + context: { error }, + }); + throw error; + }
478-479: Explicitly exporting the class is helpful.Keeps the singleton default flow while enabling controlled instantiation in tests.
src/hooks/use-map-signalr-updates.ts
Outdated
| const fetchAndUpdateMarkers = useCallback(async () => { | ||
| // Prevent concurrent API calls | ||
| if (isUpdating.current) { | ||
| logger.debug({ | ||
| message: 'Map markers update already in progress, skipping', | ||
| context: { timestamp: lastUpdateTimestamp }, | ||
| }); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Dropped updates while a fetch is in flight — coalesce or queue next update.
If a new timestamp arrives during an in-flight fetch, the debounced call executes while isUpdating is true and returns early; no retry is scheduled after the first fetch completes. Latest updates can be lost until another SignalR event occurs.
Apply:
- const debounceTimer = useRef<NodeJS.Timeout | null>(null);
+ const debounceTimer = useRef<ReturnType<typeof setTimeout> | null>(null);
const abortController = useRef<AbortController | null>(null);
+ const hasPendingUpdate = useRef<boolean>(false);
@@
- if (isUpdating.current) {
- logger.debug({
+ if (isUpdating.current) {
+ hasPendingUpdate.current = true;
+ logger.debug({
message: 'Map markers update already in progress, skipping',
context: { timestamp: lastUpdateTimestamp },
});
return;
}
@@
} finally {
isUpdating.current = false;
abortController.current = null;
+ // If an update came in while we were fetching, process it now.
+ if (hasPendingUpdate.current || lastProcessedTimestamp.current !== lastUpdateTimestamp) {
+ hasPendingUpdate.current = false;
+ // Schedule on next tick to avoid deep recursion and respect latest timestamp.
+ setTimeout(() => { void fetchAndUpdateMarkers(); }, 0);
+ }
}Note: Update the concurrent-update test to expect a second call after the first resolves.
Also applies to: 93-96
🤖 Prompt for AI Agents
In src/hooks/use-map-signalr-updates.ts around lines 19 to 27 the
fetchAndUpdateMarkers callback returns early when isUpdating.current is true,
which drops any incoming timestamp updates during an in-flight fetch; change the
logic to record/queue the latest requested timestamp while a fetch is in
progress (e.g., set a pendingTimestamp variable or flag with the newest
timestamp) and, when the current fetch completes, check for a pendingTimestamp
and immediately trigger another fetch for that latest timestamp (clear the
pending state once scheduled); apply the same fix at lines 93-96 and update the
concurrent-update test to expect a second fetch call after the first resolves.
️✅ There are no secrets present in this pull request anymore.If these secrets were true positive and are still valid, we highly recommend you to revoke them. 🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request. |
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (1)
src/components/settings/unit-selection-bottom-sheet.tsx (1)
34-34: Reentrancy guard closes the double-tap race — niceThe ref-based guard is the right fix here and pairs well with the disabled state.
🧹 Nitpick comments (22)
src/translations/es.json (2)
551-551: Use sentence case for consistency with other settings labels.Change “Unidad Actual” → “Unidad actual”.
Apply:
- "current_unit": "Unidad Actual", + "current_unit": "Unidad actual",
597-600: Align style with existing ES strings (“con éxito”, formal register).Other success messages use “con éxito” and formal “usted”. Update both lines.
Apply:
- "unit_selected_successfully": "{{unitName}} seleccionada exitosamente", + "unit_selected_successfully": "{{unitName}} seleccionada con éxito", @@ - "unit_selection_failed": "Error al seleccionar la unidad. Inténtalo de nuevo.", + "unit_selection_failed": "Error al seleccionar la unidad. Por favor, inténtelo de nuevo.",src/lib/storage/index.tsx (4)
18-21: Harden JSON parsing to avoid crashes on corrupt entries.
JSON.parsecan throw; return null on malformed JSON to keep callers safe.export function getItem<T>(key: string): T | null { - const value = storage.getString(key); - return value ? JSON.parse(value) : null; + const value = storage.getString(key); + if (value == null) return null; + try { + return JSON.parse(value) as T; + } catch { + return null; + } }
31-41: Align zustand storage methods to void returns.
StateStorage.setItem/removeItemreturn void; no need to return MMKV results.export const zustandStorage: StateStorage = { setItem: (name, value) => { - return storage.set(name, value); + storage.set(name, value); }, getItem: (name) => { const value = storage.getString(name); return value ?? null; }, removeItem: (name) => { - return storage.delete(name); + storage.delete(name); }, };
46-49: Simplify first-run logic.Same behavior with less branching.
- if (isFirstTime === undefined || isFirstTime === null || isFirstTime === true) { - return [true, setIsFirstTime] as const; - } - return [isFirstTime, setIsFirstTime] as const; + return [isFirstTime !== false, setIsFirstTime] as const;
1-1: File extension: use .ts (no JSX present).Rename to
src/lib/storage/index.tsfor clarity and tooling consistency.src/components/settings/unit-selection-bottom-sheet.tsx (8)
28-28: Make props typing explicit in the memoized componentAnnotate the destructured params to lock the props type at the callsite and improve editor inference.
-export const UnitSelectionBottomSheet = React.memo<UnitSelectionBottomSheetProps>(({ isOpen, onClose }) => { +export const UnitSelectionBottomSheet = React.memo<UnitSelectionBottomSheetProps>(({ isOpen, onClose }: UnitSelectionBottomSheetProps) => {
48-54: Non-cancellable close during workBlocking close while
isLoading || isProcessingRef.currentis okay, but consider allowing cancel to dismiss UI without interrupting work to reduce perceived latency.
72-104: Tighten the try/catch/finallyYou can drop the outer try/catch and rely on the inner try/catch/finally; set the ref before awaiting to cover all paths.
- try { - isProcessingRef.current = true; - setIsLoading(true); - let hasError = false; - try { + isProcessingRef.current = true; + setIsLoading(true); + let hasError = false; + try { await setActiveUnit(unit.UnitId); await useRolesStore.getState().fetchRolesForUnit(unit.UnitId); ... - } catch (error) { + } catch (error) { hasError = true; ... - } finally { + } finally { setIsLoading(false); isProcessingRef.current = false; if (!hasError) { handleClose(); } - } - } catch (outerError) { - // This should not happen, but just in case - setIsLoading(false); - isProcessingRef.current = false; - } + }
127-139: Follow project guideline: prefer ternary over && for conditional renderingReplace logical-AND renders with ternaries.
- {activeUnit && ( + {activeUnit ? ( <Box className="rounded-lg border border-outline-200 bg-background-50 p-3"> ... </Box> - )} + ) : null}- {activeUnit?.UnitId === unit.UnitId && <Check size={20} className="text-primary-600" />} + {activeUnit?.UnitId === unit.UnitId ? <Check size={20} className="text-primary-600" /> : null}Also applies to: 165-166
150-156: Avoid inline handlers in list items; hoist to a stable callbackInline closures in large lists trigger extra renders. Hoist or memoize a handler factory.
- onPress={() => handleUnitSelection(unit)} + onPress={React.useMemo(() => () => handleUnitSelection(unit), [unit, handleUnitSelection])}If the list can grow, prefer FlatList (see next comment).
140-174: Use FlatList for better perf and memory on variable/long unit listsScrollView renders all items; FlatList virtualizes and aligns with your guidelines (windowSize, maxToRenderPerBatch, removeClippedSubviews).
-import { ScrollView } from 'react-native'; +import { FlatList } from 'react-native';- <ScrollView className="flex-1" showsVerticalScrollIndicator={false} testID="scroll-view"> - {isLoadingUnits ? ( - <Center className="py-8"> - <Spinner size="large" /> - </Center> - ) : units.length > 0 ? ( - <VStack space="sm"> - {units.map((unit) => ( - <ActionsheetItem - key={unit.UnitId} - onPress={() => handleUnitSelection(unit)} - disabled={isLoading} - className={activeUnit?.UnitId === unit.UnitId ? 'data-[checked=true]:bg-background-100' : ''} - testID={`unit-item-${unit.UnitId}`} - > - <VStack className="flex-1"> - <ActionsheetItemText size="md" className={activeUnit?.UnitId === unit.UnitId ? 'font-medium' : 'font-normal'}> - {unit.Name} - </ActionsheetItemText> - <ActionsheetItemText size="sm" className="text-typography-500"> - {unit.Type} - </ActionsheetItemText> - </VStack> - {activeUnit?.UnitId === unit.UnitId ? <Check size={20} className="text-primary-600" /> : null} - </ActionsheetItem> - ))} - </VStack> - ) : ( - <Center className="py-8"> - <Text className="text-center text-typography-500">{t('settings.no_units_available')}</Text> - </Center> - )} - </ScrollView> + <FlatList + className="flex-1" + data={units} + keyExtractor={(u) => u.UnitId} + renderItem={({ item }) => ( + <ActionsheetItem + onPress={React.useMemo(() => () => handleUnitSelection(item), [item, handleUnitSelection])} + disabled={isLoading || activeUnit?.UnitId === item.UnitId} + className={activeUnit?.UnitId === item.UnitId ? 'bg-background-100' : ''} + testID={`unit-item-${item.UnitId}`} + > + <VStack className="flex-1"> + <ActionsheetItemText size="md" className={activeUnit?.UnitId === item.UnitId ? 'font-medium' : 'font-normal'}> + {item.Name} + </ActionsheetItemText> + <ActionsheetItemText size="sm" className="text-typography-500"> + {item.Type} + </ActionsheetItemText> + </VStack> + {activeUnit?.UnitId === item.UnitId ? <Check size={20} className="text-primary-600" /> : null} + </ActionsheetItem> + )} + ListEmptyComponent={ + isLoadingUnits ? ( + <Center className="py-8"> + <Spinner size="large" /> + </Center> + ) : ( + <Center className="py-8"> + <Text className="text-center text-typography-500">{t('settings.no_units_available')}</Text> + </Center> + ) + } + removeClippedSubviews + maxToRenderPerBatch={16} + windowSize={7} + testID="units-flat-list" + />Also applies to: 4-4
154-155: Fix non-effective class and disable already-selected item
data-[checked=true]:bg-*won’t apply here; set the background directly and disable presses for the selected unit.- disabled={isLoading} - className={activeUnit?.UnitId === unit.UnitId ? 'data-[checked=true]:bg-background-100' : ''} + disabled={isLoading || activeUnit?.UnitId === unit.UnitId} + className={activeUnit?.UnitId === unit.UnitId ? 'bg-background-100' : ''}
162-163: Consider localizing unit type if it’s an enumIf
unit.Typeis an enum/code, map to a human label via i18n (e.g., t('units.type.engine')).src/services/location.ts (4)
196-217: Serialize/throttle foreground API sends to avoid uncontrolled concurrency.
sendLocationToAPIis fire-and-forget; frequent updates can pile up. Make the callback async and gate with an in-flight flag.Apply this diff to the foreground subscription callback:
- (location) => { + async (location) => { logger.info({ message: 'Foreground location update received', context: { latitude: location.coords.latitude, longitude: location.coords.longitude, heading: location.coords.heading, }, }); useLocationStore.getState().setLocation(location); - sendLocationToAPI(location); // Send to API for foreground updates + if (this.fgSendInFlight) { + logger.debug({ message: 'Dropping foreground update: send in flight' }); + return; + } + this.fgSendInFlight = true; + try { + await sendLocationToAPI(location); // serialized send + } finally { + this.fgSendInFlight = false; + } }Add this field to the class (near other private fields):
private fgSendInFlight = false;
205-212: Reduce PII verbosity: lower log level for per-update lat/long.Consider
debughere to avoid spamming production logs with precise coordinates.Apply this diff:
- logger.info({ + logger.debug({ message: 'Foreground location update received',
279-291: Ensure foreground permission is granted before requesting background (iOS UX).On iOS, requesting background without granted foreground can auto-deny or show confusing prompts. Guard with a foreground check.
Apply this diff:
if (enabled) { - // Request background permissions when enabling background geolocation + // Ensure foreground permission is granted first (especially important on iOS) + const { status: fgStatus } = await Location.getForegroundPermissionsAsync(); + if (fgStatus !== 'granted') { + const { status: newFgStatus } = await Location.requestForegroundPermissionsAsync(); + if (newFgStatus !== 'granted') { + logger.warn({ + message: 'Cannot enable background geolocation: foreground permissions not granted', + context: { foregroundStatus: newFgStatus }, + }); + return; + } + } + // Request background permissions when enabling background geolocation const { status: backgroundStatus } = await Location.requestBackgroundPermissionsAsync(); const hasBackgroundPermissions = backgroundStatus === 'granted'; if (!hasBackgroundPermissions) { logger.warn({ message: 'Cannot enable background geolocation: background permissions not granted', context: { backgroundStatus }, }); return; }
129-150: EnsurerequestPermissionsricher return shape is backward-compatible and update call-sites
- Hundreds of call-sites (stores, services, components, tests, docs) still treat the return value as a boolean; if you change the signature to return a status object, update these sites or preserve the original boolean return for compatibility.
src/hooks/use-map-signalr-updates.ts (1)
151-163: Consider extracting cleanup logic to a dedicated function.The cleanup logic is duplicated between two useEffect hooks. Consider extracting it for better maintainability.
+ const cleanup = useCallback(() => { + // Clear debounce timer + if (debounceTimer.current) { + clearTimeout(debounceTimer.current); + debounceTimer.current = null; + } + + // Abort any ongoing request + if (abortController.current) { + abortController.current.abort(); + abortController.current = null; + } + }, []); // Cleanup on unmount useEffect(() => { - return () => { - // Clear debounce timer - if (debounceTimer.current) { - clearTimeout(debounceTimer.current); - } - - // Abort any ongoing request - if (abortController.current) { - abortController.current.abort(); - } - }; + return cleanup; - }, []); + }, [cleanup]);And update the other useEffect's cleanup:
// Cleanup function return () => { - if (debounceTimer.current) { - clearTimeout(debounceTimer.current); - debounceTimer.current = null; - } + // Only clear the timer here, not the abort controller + if (debounceTimer.current) { + clearTimeout(debounceTimer.current); + debounceTimer.current = null; + } };src/services/__tests__/signalr.service.test.ts (1)
499-535: Consider adding assertions for the reconnection interval.The test advances timers by 5000ms but doesn't verify that this matches the RECONNECT_INTERVAL constant from the service.
it('should attempt reconnection on connection close', async () => { + const EXPECTED_RECONNECT_INTERVAL = 5000; // Should match SignalRService.RECONNECT_INTERVAL + // Use fake timers to control setTimeout behavior jest.useFakeTimers(); // ... existing code ... // Trigger connection close onCloseCallback(); - // Advance timers by the exact reconnect interval (5000ms) - jest.advanceTimersByTime(5000); + // Advance timers by the exact reconnect interval + jest.advanceTimersByTime(EXPECTED_RECONNECT_INTERVAL);src/services/signalr.service.ts (2)
380-396: Consider adding timeout for waiting on connection locks during disconnect.While waiting for an ongoing connection to complete before disconnecting is good practice, consider adding a timeout to prevent indefinite waiting in edge cases.
public async disconnectFromHub(hubName: string): Promise<void> { // Wait for any ongoing connection attempt to complete const existingLock = this.connectionLocks.get(hubName); if (existingLock) { logger.info({ message: `Waiting for ongoing connection to hub ${hubName} to complete before disconnecting`, }); try { - await existingLock; + // Add a timeout to prevent indefinite waiting + await Promise.race([ + existingLock, + new Promise((_, reject) => setTimeout(() => reject(new Error('Timeout waiting for connection')), 10000)) + ]); } catch (error) { // Ignore connection errors when we're trying to disconnect logger.debug({ - message: `Connection attempt failed while waiting to disconnect from hub ${hubName}`, + message: `Connection attempt failed or timed out while waiting to disconnect from hub ${hubName}`, context: { error }, }); } }
29-29: Consider using a more specific type for connection locks.The generic
Promise<void>type for connection locks could be more descriptive.- private connectionLocks: Map<string, Promise<void>> = new Map(); + // Map of hub names to their ongoing connection promises + private connectionLocks: Map<string, Promise<void>> = new Map();Or consider creating a type alias:
type ConnectionLock = Promise<void>; private connectionLocks: Map<string, ConnectionLock> = new Map();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (13)
src/components/settings/__tests__/unit-selection-bottom-sheet-simple.test.tsx(1 hunks)src/components/settings/__tests__/unit-selection-bottom-sheet.test.tsx(1 hunks)src/components/settings/unit-selection-bottom-sheet.tsx(2 hunks)src/hooks/__tests__/use-map-signalr-updates.test.ts(1 hunks)src/hooks/use-map-signalr-updates.ts(1 hunks)src/lib/storage/index.tsx(1 hunks)src/services/__tests__/signalr.service.enhanced.test.ts(1 hunks)src/services/__tests__/signalr.service.test.ts(8 hunks)src/services/location.ts(2 hunks)src/services/signalr.service.ts(9 hunks)src/translations/ar.json(2 hunks)src/translations/en.json(2 hunks)src/translations/es.json(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/components/settings/tests/unit-selection-bottom-sheet.test.tsx
- src/translations/en.json
- src/components/settings/tests/unit-selection-bottom-sheet-simple.test.tsx
🧰 Additional context used
📓 Path-based instructions (9)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursorrules)
**/*.{ts,tsx}: Write concise, type-safe TypeScript code
Use camelCase for variable and function names (e.g., isFetchingData, handleUserInput)
Use TypeScript for all components, favoring interfaces for props and state
Avoid using any; strive for precise types
Use Expo SecureStore for sensitive data
Use zustand for state management
Use react-query for data fetching and caching
Use react-i18next for internationalization
Use react-native-mmkv for local storage
Use axios for API requests
**/*.{ts,tsx}: Write concise, type-safe TypeScript code
Use camelCase for variable and function names
Use TypeScript for all components, favoring interfaces for props and state
Avoid using any; strive for precise types
Use React Navigation for handling navigation and deep linking
Handle errors gracefully and provide user feedback
Use Expo's SecureStore for sensitive data
Use zustand for state management
Use react-query for data fetching
Use react-i18next for internationalization
Use react-native-mmkv for local storage
Use axios for API requests
Files:
src/lib/storage/index.tsxsrc/hooks/use-map-signalr-updates.tssrc/services/signalr.service.tssrc/hooks/__tests__/use-map-signalr-updates.test.tssrc/services/location.tssrc/services/__tests__/signalr.service.test.tssrc/services/__tests__/signalr.service.enhanced.test.tssrc/components/settings/unit-selection-bottom-sheet.tsx
**/*.tsx
📄 CodeRabbit inference engine (.cursorrules)
**/*.tsx: Use functional components and hooks over class components
Use PascalCase for React component names (e.g., UserProfile, ChatScreen)
Utilize React.FC for defining functional components with props
Minimize useEffect, useState, and heavy computations inside render
Use React.memo() for components with static props to prevent unnecessary re-renders
Optimize FlatList with removeClippedSubviews, maxToRenderPerBatch, windowSize
Use getItemLayout for FlatList when items have consistent size
Avoid anonymous functions in renderItem or event handlers to prevent re-renders
Use gluestack-ui for styling; if no component exists in src/components/ui, style via StyleSheet.create or styled-components
Optimize images using react-native-fast-image
Use React Navigation for navigation and deep linking with best practices
Wrap all user-facing text in t() from react-i18next
Use react-hook-form for form handling
Use @rnmapbox/maps for maps or vehicle navigation
Use lucide-react-native for icons directly in markup; do not use the gluestack-ui icon component
Use the conditional operator (?:) for conditional rendering instead of &&
**/*.tsx: Use functional components and hooks over class components
Use PascalCase for component names
Utilize React.FC for defining functional components with props
Minimize useEffect/useState and heavy computations inside render
Use React.memo for components with static props to prevent unnecessary re-renders
Optimize FlatList with removeClippedSubviews, maxToRenderPerBatch, and windowSize
Use getItemLayout for FlatList when items have consistent size
Avoid anonymous functions in renderItem or event handlers
Optimize image handling using react-native-fast-image
Wrap all user-facing text in t() from react-i18next
Ensure support for dark mode and light mode
Ensure the app is accessible following WCAG for mobile
Use react-hook-form for form handling
Use @rnmapbox/maps for maps and navigation
Use lucide-react-native for icons directly in markup; do not us...
Files:
src/lib/storage/index.tsxsrc/components/settings/unit-selection-bottom-sheet.tsx
**/*
📄 CodeRabbit inference engine (.cursorrules)
Directory and file names should be lowercase and hyphenated (e.g., user-profile, chat-screen)
Files:
src/lib/storage/index.tsxsrc/translations/es.jsonsrc/translations/ar.jsonsrc/hooks/use-map-signalr-updates.tssrc/services/signalr.service.tssrc/hooks/__tests__/use-map-signalr-updates.test.tssrc/services/location.tssrc/services/__tests__/signalr.service.test.tssrc/services/__tests__/signalr.service.enhanced.test.tssrc/components/settings/unit-selection-bottom-sheet.tsx
{components/ui/**/*.{ts,tsx},**/*.tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use gluestack-ui consistently; if no component exists in components/ui, style via StyleSheet.create or styled-components
Files:
src/lib/storage/index.tsxsrc/components/settings/unit-selection-bottom-sheet.tsx
src/translations/**
📄 CodeRabbit inference engine (.cursorrules)
Store translation dictionaries in src/translations
Files:
src/translations/es.jsonsrc/translations/ar.json
src/translations/**/*.{json,ts}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Store translation dictionaries in src/translations
Files:
src/translations/es.jsonsrc/translations/ar.json
**/*.{test.ts,test.tsx,spec.ts,spec.tsx}
📄 CodeRabbit inference engine (.cursorrules)
Create and use Jest tests to validate all generated components
Files:
src/hooks/__tests__/use-map-signalr-updates.test.tssrc/services/__tests__/signalr.service.test.tssrc/services/__tests__/signalr.service.enhanced.test.ts
**/__tests__/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursorrules)
Generate tests for all components, services, and logic; ensure tests run without errors
Files:
src/hooks/__tests__/use-map-signalr-updates.test.tssrc/services/__tests__/signalr.service.test.tssrc/services/__tests__/signalr.service.enhanced.test.ts
**/*.{test,spec}.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Create and use Jest tests for all generated components
Files:
src/hooks/__tests__/use-map-signalr-updates.test.tssrc/services/__tests__/signalr.service.test.tssrc/services/__tests__/signalr.service.enhanced.test.ts
🧠 Learnings (1)
📚 Learning: 2025-08-12T03:34:25.992Z
Learnt from: CR
PR: Resgrid/Unit#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-12T03:34:25.992Z
Learning: Applies to {components/ui/**/*.{ts,tsx},**/*.tsx} : Use gluestack-ui consistently; if no component exists in components/ui, style via StyleSheet.create or styled-components
Applied to files:
src/components/settings/unit-selection-bottom-sheet.tsx
🧬 Code graph analysis (7)
src/hooks/use-map-signalr-updates.ts (3)
src/stores/signalr/signalr-store.ts (1)
useSignalRStore(25-208)src/lib/logging/index.tsx (2)
logger(80-80)error(74-76)src/api/mapping/mapping.ts (1)
getMapDataAndMarkers(10-13)
src/services/signalr.service.ts (1)
src/lib/logging/index.tsx (2)
logger(80-80)error(74-76)
src/hooks/__tests__/use-map-signalr-updates.test.ts (4)
src/api/mapping/mapping.ts (1)
getMapDataAndMarkers(10-13)src/lib/logging/index.tsx (2)
logger(80-80)error(74-76)src/stores/signalr/signalr-store.ts (1)
useSignalRStore(25-208)src/hooks/use-map-signalr-updates.ts (1)
useMapSignalRUpdates(11-164)
src/services/location.ts (3)
src/lib/logging/index.tsx (1)
logger(80-80)src/lib/storage/background-geolocation.ts (1)
loadBackgroundGeolocationState(15-30)src/stores/app/location-store.ts (1)
useLocationStore(22-52)
src/services/__tests__/signalr.service.test.ts (1)
src/services/signalr.service.ts (1)
signalRService(484-484)
src/services/__tests__/signalr.service.enhanced.test.ts (2)
src/lib/logging/index.tsx (1)
logger(80-80)src/services/signalr.service.ts (1)
SignalRHubConnectConfig(13-18)
src/components/settings/unit-selection-bottom-sheet.tsx (3)
src/stores/units/store.ts (1)
useUnitsStore(16-31)src/stores/app/core-store.ts (1)
useCoreStore(48-263)src/stores/roles/store.ts (1)
useRolesStore(25-90)
🪛 Gitleaks (8.27.2)
src/lib/storage/index.tsx
13-13: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test
🔇 Additional comments (16)
src/translations/ar.json (3)
551-551: LGTM: Adds settings.current_unit correctly.Key name, placement, and Arabic phrasing look good.
597-600: LGTM: Success and failure messages read well and mirror EN keys.Interpolation token {{unitName}} is correct and consistent.
551-552: Cross-locale keys and placeholder consistency verified
current_unit, english, unit_selected_successfully, and unit_selection_failed exist in all locale files; {{unitName}} placeholder is used consistently.src/components/settings/unit-selection-bottom-sheet.tsx (4)
36-46: Fetch-on-open guard is soundConditioning on
isOpen && units.length === 0avoids redundant network calls and jitter. LGTM.
57-71: Early-return on same unit is good UXShort-circuiting and closing without extra work is correct.
189-189: displayName is set — good for DevToolsThis helps debugging and snapshots.
86-96: Verify i18n keys exist across all locale JSON files
Confirm that the following keys are defined in every locale you ship to prevent runtime fallbacks:
- settings.unit_selected_successfully
- settings.unit_selection_failed
src/services/location.ts (2)
225-230: Nice: contextual startup log for foreground updates.Including
backgroundEnabled,backgroundPermissions, and the persisted setting is helpful for triaging field issues.
169-186: Register-once logic is correct.Guarding
startLocationUpdatesAsyncwithisTaskRegisteredAsyncprevents duplicate background registrations.src/hooks/__tests__/use-map-signalr-updates.test.ts (1)
1-520: Well-structured and comprehensive test suite.The test suite effectively covers all the major functionality of the
useMapSignalRUpdateshook, including debouncing, concurrency, error handling, and lifecycle management. The test organization is clear, and the use of fake timers for testing debounced behavior is appropriate.src/hooks/use-map-signalr-updates.ts (1)
24-32: Good implementation of the queueing mechanism.The concurrency control logic properly queues the latest timestamp when an update is already in progress. This prevents redundant API calls while ensuring the latest data is fetched after the current operation completes.
src/services/__tests__/signalr.service.enhanced.test.ts (2)
293-345: Excellent test coverage for the edge case of explicit disconnect after scheduled reconnect.This test properly verifies that scheduled reconnection attempts are cancelled when a hub is explicitly disconnected. The test appropriately manipulates internal state and verifies the correct debug logging occurs.
132-207: Well-designed tests for connection locking behavior.The connection locking tests effectively verify that concurrent connections to the same hub are properly serialized. The tests cover multiple scenarios including connection, disconnection, and method invocation.
src/services/__tests__/signalr.service.test.ts (1)
206-208: Good handling of URLSearchParams encoding.The test correctly expects URLSearchParams to use
+for spaces, which is the standard encoding behavior for query parameters.src/services/signalr.service.ts (2)
33-46: Good implementation of the singleton pattern.The simplified singleton implementation is thread-safe in JavaScript's single-threaded environment and properly logs instance creation.
48-69: Excellent implementation of connection locking.The connection locking mechanism effectively prevents concurrent connection attempts to the same hub. The use of try/finally ensures proper cleanup of the lock.
| jest.runOnlyPendingTimers(); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use proper test lifecycle cleanup.
Running pending timers in afterEach could affect test isolation if a test fails or times out. Consider using jest.clearAllTimers() instead to avoid running potentially problematic code.
afterEach(() => {
- jest.runOnlyPendingTimers();
+ jest.clearAllTimers();
+ jest.useRealTimers();
});📝 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.
| jest.runOnlyPendingTimers(); | |
| }); | |
| afterEach(() => { | |
| jest.clearAllTimers(); | |
| jest.useRealTimers(); | |
| }); |
🤖 Prompt for AI Agents
In src/hooks/__tests__/use-map-signalr-updates.test.ts around lines 65-66, the
afterEach currently calls jest.runOnlyPendingTimers(), which can execute
leftover timers from a failing or timed-out test and break isolation; replace
that call with jest.clearAllTimers() (and ensure any fake timer setup uses
jest.useFakeTimers() where needed) so pending timers are cleared instead of run,
preserving test isolation and preventing unexpected side effects.
| global.AbortController = jest.fn().mockImplementation(() => { | ||
| abortControllerCount++; | ||
| return { | ||
| signal: { aborted: false }, | ||
| abort: mockAbort, | ||
| }; | ||
| }) as any; |
There was a problem hiding this comment.
Mock AbortController properly to avoid test pollution.
The test replaces the global AbortController but doesn't properly simulate all the behaviors, particularly the signal.aborted property. This could lead to inconsistent test results.
Apply this fix to properly mock the AbortController:
- global.AbortController = jest.fn().mockImplementation(() => {
- abortControllerCount++;
- return {
- signal: { aborted: false },
- abort: mockAbort,
- };
- }) as any;
+ global.AbortController = jest.fn().mockImplementation(() => {
+ abortControllerCount++;
+ let aborted = false;
+ const signal = {
+ get aborted() { return aborted; },
+ addEventListener: jest.fn(),
+ removeEventListener: jest.fn(),
+ dispatchEvent: jest.fn(),
+ };
+ return {
+ signal,
+ abort: jest.fn(() => {
+ aborted = true;
+ mockAbort();
+ }),
+ };
+ }) as any;📝 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.
| global.AbortController = jest.fn().mockImplementation(() => { | |
| abortControllerCount++; | |
| return { | |
| signal: { aborted: false }, | |
| abort: mockAbort, | |
| }; | |
| }) as any; | |
| global.AbortController = jest.fn().mockImplementation(() => { | |
| abortControllerCount++; | |
| let aborted = false; | |
| const signal = { | |
| get aborted() { return aborted; }, | |
| addEventListener: jest.fn(), | |
| removeEventListener: jest.fn(), | |
| dispatchEvent: jest.fn(), | |
| }; | |
| return { | |
| signal, | |
| abort: jest.fn(() => { | |
| aborted = true; | |
| mockAbort(); | |
| }), | |
| }; | |
| }) as any; |
| storage = new MMKV({ | ||
| id: 'ResgridUnit', | ||
| encryptionKey: 'hunter2', | ||
| encryptionKey: '9f066882-5c07-47a4-9bf3-783074b590d5', | ||
| }); |
There was a problem hiding this comment.
💡 Verification agent
❓ Verification inconclusive
Hard-coded MMKV encryptionKey (secret leak) — rotate and move to per-device SecureStore key.
A plaintext key committed to the repo defeats encryption (all installs share it) and triggers secret scanners. Rotate/invalidate the leaked key and refactor to derive a per-device key stored in Expo SecureStore before MMKV init. Also confirm no PII is stored unencrypted on web.
Apply this minimal stop-gap to remove the leaked key (prevents further exposure, but removes at-rest encryption until proper init is added):
storage = new MMKV({
id: 'ResgridUnit',
- encryptionKey: '9f066882-5c07-47a4-9bf3-783074b590d5',
});Recommended follow-up (outside the selected lines): initialize MMKV after fetching/storing a per-device key.
// Add these imports at top-level
import * as SecureStore from 'expo-secure-store';
import * as Random from 'expo-random';
const MMKV_KEY = 'mmkvEncryptionKey';
async function getOrCreateMMKVKey(): Promise<string> {
const existing = await SecureStore.getItemAsync(MMKV_KEY);
if (existing) return existing;
const bytes = await Random.getRandomBytesAsync(32);
const hex = Array.from(bytes).map(b => b.toString(16).padStart(2, '0')).join('');
await SecureStore.setItemAsync(MMKV_KEY, hex);
return hex;
}
// Replace the synchronous top-level init with:
export let storage: MMKV;
export async function initStorage() {
if (Platform.OS === 'web') {
storage = new MMKV({ id: 'ResgridUnit' });
} else {
const encryptionKey = await getOrCreateMMKVKey();
storage = new MMKV({ id: 'ResgridUnit', encryptionKey });
}
}Then call await initStorage() before any storage usage (e.g., in App bootstrap).
Rotate leaked MMKV key and move to SecureStore
Hard-coded encryptionKey in src/lib/storage/index.tsx:11-14 exposes a static secret; remove it, rotate the leaked key, and derive/store a per-device key in Expo SecureStore before initializing MMKV. Confirm the web build doesn’t persist PII unencrypted.
storage = new MMKV({
id: 'ResgridUnit',
- encryptionKey: '9f066882-5c07-47a4-9bf3-783074b590d5',
});Use expo-secure-store + expo-random to generate, persist, and retrieve the encryption key at runtime.
Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Gitleaks (8.27.2)
13-13: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🤖 Prompt for AI Agents
In src/lib/storage/index.tsx around lines 11 to 14 the MMKV encryptionKey is
hard-coded and must be removed and rotated; replace the static secret by
generating a per-device key at runtime using expo-random and persisting it with
expo-secure-store (on app startup: attempt to read key from SecureStore, if
missing generate a new secure random key, store it, then pass it into MMKV
initialization), delete the hard-coded value from source, and ensure any
deployed secret is rotated out of history/CI; for web builds do not persist PII
unencrypted — either use a web-safe encrypted storage mechanism or fall back to
non-persistent in-memory storage with a clear warning/log and ensure MMKV only
initializes when a secure key is present.
| async requestPermissions(requestBackground = false): Promise<boolean> { | ||
| const { status: foregroundStatus } = await Location.requestForegroundPermissionsAsync(); | ||
| const { status: backgroundStatus } = await Location.requestBackgroundPermissionsAsync(); | ||
|
|
||
| let backgroundStatus = 'undetermined'; | ||
| if (requestBackground) { | ||
| const result = await Location.requestBackgroundPermissionsAsync(); | ||
| backgroundStatus = result.status; | ||
| } | ||
|
|
||
| logger.info({ | ||
| message: 'Location permissions requested', | ||
| context: { | ||
| foregroundStatus, | ||
| backgroundStatus, | ||
| backgroundStatus: requestBackground ? backgroundStatus : 'not requested', | ||
| backgroundRequested: requestBackground, | ||
| }, | ||
| }); | ||
|
|
||
| return foregroundStatus === 'granted' && backgroundStatus === 'granted'; | ||
| // Only require foreground permissions for basic functionality | ||
| // Background permissions are optional and will be handled separately | ||
| return foregroundStatus === 'granted'; | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Platform entitlements check (iOS/Android) recommended.
Verify required descriptors exist (InfoPlist strings, UIBackgroundModes=location, Android ACCESS_BACKGROUND_LOCATION, foreground service type).
🏁 Script executed:
#!/bin/bash
# iOS (app.json/app.config.* or Info.plist in native)
rg -nP -C2 'UIBackgroundModes|NSLocationAlwaysAndWhenInUseUsageDescription|NSLocationWhenInUseUsageDescription' app.json app.config.ts ios Info.plist 2>/dev/null || true
# Android manifest and permissions
rg -nP -C2 'ACCESS_BACKGROUND_LOCATION|FOREGROUND_SERVICE|FOREGROUND_SERVICE_LOCATION|ACCESS_FINE_LOCATION|ACCESS_COARSE_LOCATION' android/app/src/main/AndroidManifest.xml app.json app.config.ts 2>/dev/null || true
# Ensure the background task name is unique
rg -nP --type=ts --type=tsx -C2 "defineTask\\([^,]*['\"]location-updates['\"]" Length of output: 1267
Add missing iOS and Android location entitlements
- iOS (app.config.ts / Info.plist): add NSLocationWhenInUseUsageDescription and NSLocationAlwaysAndWhenInUseUsageDescription, and include 'location' in UIBackgroundModes.
- Android (AndroidManifest.xml / app.config.ts): add android.permission.ACCESS_FINE_LOCATION, ACCESS_COARSE_LOCATION and ACCESS_BACKGROUND_LOCATION, and set foregroundServiceType="location".
🤖 Prompt for AI Agents
In src/services/location.ts around lines 129 to 150, the app requests location
(foreground and optional background) but the native entitlements are missing;
update iOS and Android config files: for iOS (app.config.ts / Info.plist) add
NSLocationWhenInUseUsageDescription and
NSLocationAlwaysAndWhenInUseUsageDescription with appropriate user-facing
description strings, and add 'location' to UIBackgroundModes; for Android
(AndroidManifest.xml / app.config.ts) add
android.permission.ACCESS_FINE_LOCATION,
android.permission.ACCESS_COARSE_LOCATION and
android.permission.ACCESS_BACKGROUND_LOCATION to the manifest and ensure any
foreground service uses foregroundServiceType="location". Ensure these entries
are present in the correct platform config files and that the user-facing iOS
strings explain why location is needed.
src/services/signalr.service.ts
Outdated
| // Remove the connection from our maps to allow fresh connection | ||
| this.connections.delete(hubName); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Potential race condition when deleting connection before reconnect.
Deleting the connection from the map before attempting to reconnect could cause issues if another operation checks for the connection during this window.
Consider only deleting after a failed reconnection attempt:
- // Remove the connection from our maps to allow fresh connection
- this.connections.delete(hubName);
-
await this.connectToHubWithEventingUrl(currentHubConfig);
logger.info({
message: `Successfully reconnected to hub: ${hubName} after ${currentAttempts} attempts`,
});
} catch (error) {
+ // Remove the failed connection from our maps
+ this.connections.delete(hubName);
+
logger.error({
message: `Failed to refresh token or reconnect to hub: ${hubName}`,
context: { error, attempts: currentAttempts, maxAttempts: this.MAX_RECONNECT_ATTEMPTS },
});📝 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.
| // Remove the connection from our maps to allow fresh connection | |
| this.connections.delete(hubName); | |
| try { | |
| // (moved deletion into catch block — no longer removed preemptively) | |
| await this.connectToHubWithEventingUrl(currentHubConfig); | |
| logger.info({ | |
| message: `Successfully reconnected to hub: ${hubName} after ${currentAttempts} attempts`, | |
| }); | |
| } catch (error) { | |
| // Remove the failed connection from our maps | |
| this.connections.delete(hubName); | |
| logger.error({ | |
| message: `Failed to refresh token or reconnect to hub: ${hubName}`, | |
| context: { error, attempts: currentAttempts, maxAttempts: this.MAX_RECONNECT_ATTEMPTS }, | |
| }); | |
| } |
🤖 Prompt for AI Agents
In src/services/signalr.service.ts around lines 336-337, the code deletes the
connection from the map before attempting a reconnect, which can cause a race if
other code checks the map during the reconnect window; change the logic to
attempt the reconnection first (or set a "reconnecting" flag/state for that hub)
and only call this.connections.delete(hubName) after the reconnection attempts
have definitively failed, or clear the flag on success—ensure concurrent checks
respect the flag/state so they don't treat the hub as fully absent during
in-progress reconnects.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
src/services/location.ts (2)
233-273: Duplicate background streams prevented but past issue persists.The new check for
isTaskRegisteredbefore creating awatchPositionAsyncsubscription addresses the duplicate streams issue from the previous review. However, this is a partial fix - the same logic should be applied to the foreground updates instartLocationUpdates()to ensure complete consistency.The logic correctly prevents creating a
watchPositionAsyncsubscription when the OS-managed background task is already registered, which avoids the duplicate background streams issue identified in the previous review.
129-343: Verify platform entitlements are configured for background location.Based on the past review comments, the required iOS and Android location entitlements may still be missing. The enhanced background location functionality requires proper platform configuration.
Please verify that the following entitlements have been added since the previous review:
#!/bin/bash # Description: Verify iOS and Android location entitlements are properly configured # Check iOS configuration (app.config.ts, app.json, or Info.plist) echo "=== iOS Configuration Check ===" fd -e ts -e json -e plist | xargs rg -l "NSLocationWhenInUseUsageDescription|NSLocationAlwaysAndWhenInUseUsageDescription|UIBackgroundModes" | head -5 | while read file; do echo "File: $file" rg -A2 -B2 "NSLocationWhenInUseUsageDescription|NSLocationAlwaysAndWhenInUseUsageDescription|UIBackgroundModes.*location" "$file" 2>/dev/null || echo " No iOS location strings found" done echo -e "\n=== Android Configuration Check ===" # Check Android configuration (AndroidManifest.xml, app.config.ts, or app.json) fd -e xml -e ts -e json | xargs rg -l "ACCESS_FINE_LOCATION|ACCESS_COARSE_LOCATION|ACCESS_BACKGROUND_LOCATION|FOREGROUND_SERVICE_LOCATION" | head -5 | while read file; do echo "File: $file" rg -A2 -B2 "ACCESS_FINE_LOCATION|ACCESS_COARSE_LOCATION|ACCESS_BACKGROUND_LOCATION|foregroundServiceType.*location" "$file" 2>/dev/null || echo " No Android location permissions found" done echo -e "\n=== Background Task Name Uniqueness Check ===" # Verify background task name uniqueness rg -n "defineTask.*location-updates" --type ts
🧹 Nitpick comments (5)
src/services/__tests__/signalr.service.test.ts (1)
487-491: Accessing private emit method breaks TS visibility; cast for test or expose a helper.Directly indexing a private member may fail TS type-checking. Either cast or provide a public test helper.
- signalRService['emit']('testEvent', { test: 'data' }); + (signalRService as any).emit('testEvent', { test: 'data' });Or add a public testing-only helper on the service guarded by NODE_ENV.
src/services/signalr.service.ts (4)
63-84: Avoid “await lock then return” dead-end; retry connect if the prior attempt failed.When a connection is in-flight you
awaitits promise and return unconditionally. If that attempt failed, callers can get stuck without a second attempt.- if (existingLock) { + if (existingLock) { logger.info({ message: `Connection to hub ${config.name} is already in progress, waiting...`, }); - await existingLock; - return; + try { + await existingLock; + } catch { + // ignore – we'll attempt below + } + if (this.connections.has(config.name) || this.reconnectingHubs.has(config.name)) { + return; + } }Mirror the same pattern in
connectToHubfor the legacy path.
208-229: Apply the same post-lock recheck to legacy connectToHub.Prevents missed retries if the first attempt fails under the lock.
- if (existingLock) { + if (existingLock) { logger.info({ message: `Connection to hub ${config.name} is already in progress, waiting...`, }); - await existingLock; - return; + try { + await existingLock; + } catch { + // ignore – we'll attempt below + } + if (this.connections.has(config.name) || this.reconnectingHubs.has(config.name)) { + return; + } }
349-377: Reconnecting flag timing.After the above fix, the reconnecting flag is set earlier. Remove this later add to avoid duplicate intent and keep logs accurate.
- // Set reconnecting flag to indicate this hub is in the process of reconnecting - this.reconnectingHubs.add(hubName);
505-520: Make resetInstance async or await the cleanup to reduce races in tests.
disconnectAll()is async but not awaited before nulling the instance. In tests this can cause interleaving with the next instance’s lifecycle.- public static resetInstance(): void { + public static async resetInstance(): Promise<void> { if (SignalRService.instance) { - SignalRService.instance.disconnectAll().catch((error) => { + try { + await SignalRService.instance.disconnectAll(); + } catch (error) { logger.error({ message: 'Error disconnecting all hubs during instance reset', context: { error }, }); - }); + } } SignalRService.instance = null; logger.debug({ message: 'SignalR service singleton instance reset', }); }Update call sites in tests to
await SignalRService.resetInstance().
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
src/hooks/__tests__/use-map-signalr-updates.test.ts(1 hunks)src/services/__tests__/signalr.service.test.ts(11 hunks)src/services/location.ts(4 hunks)src/services/signalr.service.ts(10 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/hooks/tests/use-map-signalr-updates.test.ts
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursorrules)
**/*.{ts,tsx}: Write concise, type-safe TypeScript code
Use camelCase for variable and function names (e.g., isFetchingData, handleUserInput)
Use TypeScript for all components, favoring interfaces for props and state
Avoid using any; strive for precise types
Use Expo SecureStore for sensitive data
Use zustand for state management
Use react-query for data fetching and caching
Use react-i18next for internationalization
Use react-native-mmkv for local storage
Use axios for API requests
**/*.{ts,tsx}: Write concise, type-safe TypeScript code
Use camelCase for variable and function names
Use TypeScript for all components, favoring interfaces for props and state
Avoid using any; strive for precise types
Use React Navigation for handling navigation and deep linking
Handle errors gracefully and provide user feedback
Use Expo's SecureStore for sensitive data
Use zustand for state management
Use react-query for data fetching
Use react-i18next for internationalization
Use react-native-mmkv for local storage
Use axios for API requests
Files:
src/services/location.tssrc/services/signalr.service.tssrc/services/__tests__/signalr.service.test.ts
**/*
📄 CodeRabbit inference engine (.cursorrules)
Directory and file names should be lowercase and hyphenated (e.g., user-profile, chat-screen)
Files:
src/services/location.tssrc/services/signalr.service.tssrc/services/__tests__/signalr.service.test.ts
**/*.{test.ts,test.tsx,spec.ts,spec.tsx}
📄 CodeRabbit inference engine (.cursorrules)
Create and use Jest tests to validate all generated components
Files:
src/services/__tests__/signalr.service.test.ts
**/__tests__/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursorrules)
Generate tests for all components, services, and logic; ensure tests run without errors
Files:
src/services/__tests__/signalr.service.test.ts
**/*.{test,spec}.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Create and use Jest tests for all generated components
Files:
src/services/__tests__/signalr.service.test.ts
🧬 Code graph analysis (3)
src/services/location.ts (3)
src/lib/logging/index.tsx (1)
logger(80-80)src/lib/storage/background-geolocation.ts (1)
loadBackgroundGeolocationState(15-30)src/stores/app/location-store.ts (1)
useLocationStore(22-52)
src/services/signalr.service.ts (1)
src/lib/logging/index.tsx (2)
logger(80-80)error(74-76)
src/services/__tests__/signalr.service.test.ts (1)
src/services/signalr.service.ts (2)
signalRService(546-546)SignalRHubConnectConfig(13-18)
🔇 Additional comments (12)
src/services/__tests__/signalr.service.test.ts (4)
494-540: Nice coverage for availability/reconnecting flags.Good assertions around
isHubAvailable/isHubReconnectingand duplicate-connect suppression.
208-213: URLSearchParams encoding expectation looks correct.Plus for spaces and percent-encoding for reserved characters are asserted properly.
231-236: Bearer token encoding case covered well.Solid edge-case test to prevent accidental header-based auth on geo hub.
757-787: State reset in nested describe is thorough.Good isolation: clearing internal maps, mocks, and tokens avoids flaky tests.
src/services/signalr.service.ts (4)
49-55: Helper APIs look good.
isHubAvailableandisHubReconnectingare simple and useful for UX gating.Also applies to: 56-62
129-132: Good: querystring assembly avoids leaking tokens in logs.Merging existing params with
URLSearchParamsis correct and you sanitizeaccess_tokenin logs.
432-474: Graceful disconnect waits on in-flight connects.Solid pattern to avoid tearing down during an in-progress connect; also clears reconnecting flags.
29-31: Per-hub locks and reconnecting guard are the right primitives.The combination of
connectionLocksandreconnectingHubsneatly prevents duplicate connects and racy invocations.src/services/location.ts (4)
129-150: Parameter added to requestPermissions method.The new optional parameter
requestBackgroundallows selective background permission requests, which is a good approach for conditional permission handling.
152-194: Enhanced permission handling and background task registration logic.The refactored flow properly separates background settings from permissions and implements proper fallback to foreground-only mode when background permissions are denied. The logging provides good context for debugging permission states.
196-231: Good idempotent foreground subscription handling.The guard against duplicate subscriptions prevents resource leaks and provides clear logging when attempting to create duplicate subscriptions.
286-343: Background geolocation setting update with proper permission validation.The enhanced logic properly validates background permissions before enabling background tracking and includes comprehensive checks to prevent duplicate task registration. The additional validation in lines 321-331 ensures consistency by checking for existing OS-managed tasks before creating watch subscriptions.
| await jest.runAllTicks(); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace non-standard jest.runAllTicks with supported timer/microtask flushes.
jest.runAllTicks() isn’t available in modern Jest. Use microtask flush + timer utilities to avoid brittle tests.
- jest.advanceTimersByTime(5000);
- await jest.runAllTicks();
+ await jest.advanceTimersByTimeAsync(5000);
+ // Flush pending microtasks
+ await Promise.resolve();If advanceTimersByTimeAsync isn’t available in your Jest version, keep advanceTimersByTime(5000) and follow it with await Promise.resolve().
Also applies to: 669-670, 703-704, 737-739, 816-817, 849-861, 907-909
🤖 Prompt for AI Agents
In src/services/__tests__/signalr.service.test.ts around lines 635-636 (and
similarly at 669-670, 703-704, 737-739, 816-817, 849-861, 907-909), replace the
non-standard jest.runAllTicks() calls with a microtask/timer-safe flush: if your
Jest supports jest.advanceTimersByTimeAsync use that to advance timers and await
it; otherwise keep any existing jest.advanceTimersByTime(5000) calls but
immediately follow them with await Promise.resolve() to flush microtasks (or
simply replace jest.runAllTicks() with await Promise.resolve() where no timers
need advancing). Ensure each replacement is awaited so microtasks are flushed
and tests become stable.
| (signalRService as any).connections.delete(mockConfig.name); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Stop manually deleting connections in tests; service should handle stale entries.
Tests delete (signalRService as any).connections.delete(...) before triggering reconnection. After fixing the service to ignore stale/closed connections, these deletions become unnecessary and can mask regressions.
- // Remove the connection to simulate it being closed
- (signalRService as any).connections.delete(mockConfig.name);
+ // No manual deletion needed; service should detect closed/stale connections and proceed to reconnectAlso applies to: 843-844, 851-852, 898-899
🤖 Prompt for AI Agents
In src/services/__tests__/signalr.service.test.ts around lines 807-808 (also
apply same change at 843-844, 851-852, 898-899): tests currently manually delete
entries via (signalRService as any).connections.delete(...); remove these manual
deletions so tests exercise the service reconnection/stale-connection handling;
update the tests to trigger reconnection and assert the service ignores or
cleans up stale/closed connections itself (e.g., by asserting the reconnection
succeeds or that the connection map contains the new/expected entry) rather than
preemptively deleting entries.
|
Approve |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (8)
src/services/__tests__/signalr.service.enhanced.test.ts (4)
204-207: Stabilize log assertions to avoid brittleness.Exact object matching can be fragile if logging adds fields. Prefer partial match.
-expect(mockLogger.info).toHaveBeenCalledWith({ - message: `Connection to hub ${mockConfig.name} is already in progress, waiting...`, -}); +expect(mockLogger.info).toHaveBeenCalledWith( + expect.objectContaining({ + message: `Connection to hub ${mockConfig.name} is already in progress, waiting...`, + }) +);
271-277: Helper to set mock connection state.Direct descriptor manipulation is fine, but a tiny helper improves clarity and reuse across tests.
-Object.defineProperty(mockConnection, 'state', { - value: HubConnectionState.Connected, - writable: true, -}); +const setState = (s: number) => + Object.defineProperty(mockConnection, 'state', { value: s, writable: true }); +setState(HubConnectionState.Connected);
340-347: Unnecessary manual restoration of internal map.
resetInstance()in beforeEach already tears down state. Restoringconnectionshere is redundant.-// Restore original connections map and timers -connectionsMap.clear(); -originalConnectionsMap.forEach((value, key) => { - connectionsMap.set(key, value); -}); +// No-op: instance is reset in beforeEach
50-52: Avoid shadowed mock names for readability.
mockRefreshAccessTokenis declared both inside the module factory and in the test. Rename the test-level one to avoid confusion.-const mockGetState = (useAuthStore as any).getState; -const mockRefreshAccessToken = jest.fn().mockResolvedValue(undefined); +const authGetStateMock = (useAuthStore as any).getState; +const refreshAccessTokenMock = jest.fn().mockResolvedValue(undefined); @@ -// Mock auth store -mockGetState.mockReturnValue({ +// Mock auth store +authGetStateMock.mockReturnValue({ accessToken: 'mock-token', - refreshAccessToken: mockRefreshAccessToken, + refreshAccessToken: refreshAccessTokenMock, });Also applies to: 93-97
src/services/signalr.service.ts (4)
29-33: Optional: track and cancel scheduled reconnect timers on disconnect to free resources faster.Currently, timers rely on config checks to no-op. Storing timer IDs allows proactive cancellation on explicit disconnect or when max attempts are reached.
private hubConfigs: Map<string, SignalRHubConnectConfig> = new Map(); - private connectionLocks: Map<string, Promise<void>> = new Map(); + private connectionLocks: Map<string, Promise<void>> = new Map(); private reconnectingHubs: Set<string> = new Set(); + private reconnectTimers: Map<string, ReturnType<typeof setTimeout>> = new Map(); @@ - setTimeout(async () => { + const t = setTimeout(async () => { try { + this.reconnectTimers.delete(hubName); // ... } catch (error) { // ... } - }, this.RECONNECT_INTERVAL); + }, this.RECONNECT_INTERVAL); + this.reconnectTimers.set(hubName, t); @@ logger.error({ message: `Max reconnection attempts (${this.MAX_RECONNECT_ATTEMPTS}) reached for hub: ${hubName}`, }); // Clean up resources for this failed connection this.connections.delete(hubName); this.reconnectAttempts.delete(hubName); this.hubConfigs.delete(hubName); this.reconnectingHubs.delete(hubName); + const timer = this.reconnectTimers.get(hubName); + if (timer) { + clearTimeout(timer); + this.reconnectTimers.delete(hubName); + } @@ const connection = this.connections.get(hubName); if (connection) { try { await connection.stop(); this.connections.delete(hubName); this.reconnectAttempts.delete(hubName); this.hubConfigs.delete(hubName); this.reconnectingHubs.delete(hubName); + const timer = this.reconnectTimers.get(hubName); + if (timer) { + clearTimeout(timer); + this.reconnectTimers.delete(hubName); + } logger.info({ message: `Disconnected from hub: ${hubName}`, });If you want, I can add a focused test to assert timers are cleared on explicit disconnect.
Also applies to: 323-341, 415-424, 454-477
49-55: Add a direct “isHubConnected” helper.You have
isHubAvailableandisHubReconnecting. A connected-state check helps callers avoid reading internal maps./** * Check if a hub is currently reconnecting */ public isHubReconnecting(hubName: string): boolean { return this.reconnectingHubs.has(hubName); } + + /** + * Check if a hub is actively connected + */ + public isHubConnected(hubName: string): boolean { + const conn = this.connections.get(hubName); + return !!conn && conn.state === HubConnectionState.Connected; + }
111-138: Sanitize logs consistently.You already redact the token in
fullUrl. Consider omittingconfigor logging only non-sensitive fields to reduce log noise/PII risk.
550-552: Be aware of side effects from exporting a singleton at module import.
export const signalRService = ...creates/logs the instance on import. If cold-start time/log noise matters, prefer lazy access viagetInstance().
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
src/services/__tests__/signalr.service.enhanced.test.ts(1 hunks)src/services/signalr.service.ts(11 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursorrules)
**/*.{ts,tsx}: Write concise, type-safe TypeScript code
Use camelCase for variable and function names (e.g., isFetchingData, handleUserInput)
Use TypeScript for all components, favoring interfaces for props and state
Avoid using any; strive for precise types
Use Expo SecureStore for sensitive data
Use zustand for state management
Use react-query for data fetching and caching
Use react-i18next for internationalization
Use react-native-mmkv for local storage
Use axios for API requests
**/*.{ts,tsx}: Write concise, type-safe TypeScript code
Use camelCase for variable and function names
Use TypeScript for all components, favoring interfaces for props and state
Avoid using any; strive for precise types
Use React Navigation for handling navigation and deep linking
Handle errors gracefully and provide user feedback
Use Expo's SecureStore for sensitive data
Use zustand for state management
Use react-query for data fetching
Use react-i18next for internationalization
Use react-native-mmkv for local storage
Use axios for API requests
Files:
src/services/__tests__/signalr.service.enhanced.test.tssrc/services/signalr.service.ts
**/*
📄 CodeRabbit inference engine (.cursorrules)
Directory and file names should be lowercase and hyphenated (e.g., user-profile, chat-screen)
Files:
src/services/__tests__/signalr.service.enhanced.test.tssrc/services/signalr.service.ts
**/*.{test.ts,test.tsx,spec.ts,spec.tsx}
📄 CodeRabbit inference engine (.cursorrules)
Create and use Jest tests to validate all generated components
Files:
src/services/__tests__/signalr.service.enhanced.test.ts
**/__tests__/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursorrules)
Generate tests for all components, services, and logic; ensure tests run without errors
Files:
src/services/__tests__/signalr.service.enhanced.test.ts
**/*.{test,spec}.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Create and use Jest tests for all generated components
Files:
src/services/__tests__/signalr.service.enhanced.test.ts
🧬 Code graph analysis (2)
src/services/__tests__/signalr.service.enhanced.test.ts (2)
src/lib/logging/index.tsx (1)
logger(80-80)src/services/signalr.service.ts (1)
SignalRHubConnectConfig(13-18)
src/services/signalr.service.ts (1)
src/lib/logging/index.tsx (2)
logger(80-80)error(74-76)
🔇 Additional comments (4)
src/services/signalr.service.ts (4)
329-347: Good: explicit checks for removed config and live connection.Skipping when config is gone and when the connection is already Connected avoids spurious reconnects.
349-381: Reconnect flow sequencing is solid after removing the self-blocking guard.Token refresh first, then clean stale entry, then reconnect via the public path, and finally clear the flag.
436-478: Graceful disconnect while a connect is in-flight looks correct.Waiting on the per-hub lock prevents stop/start races; cleanup clears all relevant maps/flags.
480-506: Clear errors on invoke when reconnecting or not connected.Good UX: explicit messages and awaited lock before invoking.
| // Mock the dependencies | ||
| jest.mock('@microsoft/signalr'); | ||
| jest.mock('@/lib/logging', () => ({ |
There was a problem hiding this comment.
Fix: provide a concrete mock for @microsoft/signalr (enum + builder).
jest.mock('@microsoft/signalr') without a factory can make HubConnectionState undefined and break checks/assignments (e.g., Lines 75, 271–276). Also ensures LogLevel.Information exists for the service under test.
Apply:
-jest.mock('@microsoft/signalr');
+jest.mock('@microsoft/signalr', () => {
+ const HubConnectionState = {
+ Disconnected: 0,
+ Connecting: 1,
+ Connected: 2,
+ Disconnecting: 3,
+ Reconnecting: 4,
+ };
+ const LogLevel = { Trace: 0, Debug: 1, Information: 2, Warning: 3, Error: 4, Critical: 5, None: 6 };
+
+ class MockHubConnectionBuilder {
+ withUrl = jest.fn().mockReturnThis();
+ withAutomaticReconnect = jest.fn().mockReturnThis();
+ configureLogging = jest.fn().mockReturnThis();
+ build = jest.fn(); // test overrides to return mockConnection
+ }
+
+ return {
+ __esModule: true,
+ HubConnectionBuilder: jest.fn(() => new MockHubConnectionBuilder()),
+ HubConnectionState,
+ LogLevel,
+ };
+});📝 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.
| // Mock the dependencies | |
| jest.mock('@microsoft/signalr'); | |
| jest.mock('@/lib/logging', () => ({ | |
| // Mock the dependencies | |
| jest.mock('@microsoft/signalr', () => { | |
| const HubConnectionState = { | |
| Disconnected: 0, | |
| Connecting: 1, | |
| Connected: 2, | |
| Disconnecting: 3, | |
| Reconnecting: 4, | |
| }; | |
| const LogLevel = { | |
| Trace: 0, | |
| Debug: 1, | |
| Information: 2, | |
| Warning: 3, | |
| Error: 4, | |
| Critical: 5, | |
| None: 6, | |
| }; | |
| class MockHubConnectionBuilder { | |
| withUrl = jest.fn().mockReturnThis(); | |
| withAutomaticReconnect = jest.fn().mockReturnThis(); | |
| configureLogging = jest.fn().mockReturnThis(); | |
| build = jest.fn(); // test overrides to return mockConnection | |
| } | |
| return { | |
| __esModule: true, | |
| HubConnectionBuilder: jest.fn(() => new MockHubConnectionBuilder()), | |
| HubConnectionState, | |
| LogLevel, | |
| }; | |
| }); | |
| jest.mock('@/lib/logging', () => ({ | |
| logger: { | |
| info: jest.fn(), | |
| debug: jest.fn(), | |
| warn: jest.fn(), | |
| error: jest.fn(), | |
| }, | |
| })); |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/services/signalr.service.ts (1)
386-477: Fix reconnection loop: only one attempt runs; schedule subsequent attempts until MAX.After a failed attempt inside the setTimeout, no new close event is emitted, so you never reach MAX_RECONNECT_ATTEMPTS. Schedule the next attempt explicitly.
- // Don't immediately retry; let the next connection close event trigger another attempt - // This prevents rapid retry loops that could overwhelm the server + // Schedule another attempt (bounded by MAX_RECONNECT_ATTEMPTS). No new close event will fire here. + const current = this.reconnectAttempts.get(hubName) ?? 0; + if (current < this.MAX_RECONNECT_ATTEMPTS) { + setTimeout(() => this.handleConnectionClose(hubName), this.RECONNECT_INTERVAL); + }
♻️ Duplicate comments (2)
src/services/__tests__/signalr.service.test.ts (2)
636-638: Replace deprecated jest.runAllTicks with supported flush.- jest.advanceTimersByTime(5000); - await jest.runAllTicks(); + await jest.advanceTimersByTimeAsync?.(5000) ?? jest.advanceTimersByTime(5000); + await Promise.resolve();Apply similarly at 670-672, 704-706, 738-739, 818-819, 851-852, 909-910.
629-631: Stop manually deleting connections in tests; exercise stale-connection handling.- // Remove the connection to simulate it being closed - (signalRService as any).connections.delete(mockConfig.name); + // No manual deletion; let onclose/reconnect logic handle stale entries.Also applies to: 664-665, 698-699, 731-733, 809-810, 845-846, 853-854, 900-901
🧹 Nitpick comments (7)
src/services/signalr.service.ts (4)
418-445: Remove redundant delete; stale entry already cleared above.connections.delete(hubName) is executed twice in this flow.
- // Remove the connection from our maps to allow fresh connection - // This is now safe because we have the reconnecting flag set - this.connections.delete(hubName);
549-576: Preserve return values from hub invocations (make invoke generic).Current signature Promise discards results from server methods.
- public async invoke(hubName: string, method: string, data: unknown): Promise<void> { + public async invoke<T = unknown>(hubName: string, method: string, data: unknown): Promise<T> { @@ - if (connection) { - try { - return await connection.invoke(method, data); + if (connection) { + try { + return await connection.invoke<T>(method, data);
333-345: Optionally reflect reconnecting state during built-in auto-reconnect.Set RECONNECTING on onreconnecting; clear on onreconnected for more accurate isHubAvailable/isHubReconnecting.
connection.onreconnecting((error) => { logger.warn({ message: `Reconnecting to hub: ${config.name}`, context: { error }, }); + this.setHubState(config.name, HubConnectingState.RECONNECTING); }); @@ connection.onreconnected((connectionId) => { logger.info({ message: `Reconnected to hub: ${config.name}`, context: { connectionId }, }); this.reconnectAttempts.set(config.name, 0); + this.setHubState(config.name, HubConnectingState.IDLE); });Also applies to: 206-218
578-593: Make resetInstance await disconnections to avoid test races.Returning Promise ensures the next getInstance isn’t interleaving with pending disconnects.
- public static resetInstance(): void { + public static async resetInstance(): Promise<void> { if (SignalRService.instance) { - // Disconnect all connections before resetting - SignalRService.instance.disconnectAll().catch((error) => { - logger.error({ - message: 'Error disconnecting all hubs during instance reset', - context: { error }, - }); - }); + try { + await SignalRService.instance.disconnectAll(); + } catch (error) { + logger.error({ message: 'Error disconnecting all hubs during instance reset', context: { error } }); + } } SignalRService.instance = null; logger.debug({ message: 'SignalR service singleton instance reset', }); }src/services/__tests__/signalr.service.reconnect-fix.test.ts (1)
55-63: Minor: avoid any in tests when possible.Where feasible, prefer typed helpers over casting to any to access internals; or expose minimal test hooks.
docs/signalr-reconnect-self-blocking-fix.md (2)
5-14: Tighten wording and punctuation.-2. This could lead to scenarios where: +2) This led to scenarios where: @@ - - The manual connection would be rejected - - If the reconnection failed, the hub would be stuck in reconnecting state - - Future manual connection attempts would continue to be blocked + - The manual connection was rejected. + - If the reconnection failed, the hub could become stuck in a reconnecting state. + - Future manual connection attempts continued to be blocked.
45-56: Minor grammar/formatting nits in lists.-**Direct Connections (`_connectToHubInternal` and `_connectToHubWithEventingUrlInternal`):** +Direct connections (`_connectToHubInternal` and `_connectToHubWithEventingUrlInternal`): @@ -**Automatic Reconnections:** +Automatic reconnections:
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
docs/signalr-reconnect-self-blocking-fix.md(1 hunks)src/services/__tests__/signalr.service.reconnect-fix.test.ts(1 hunks)src/services/__tests__/signalr.service.test.ts(12 hunks)src/services/signalr.service.ts(13 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursorrules)
**/*.{ts,tsx}: Write concise, type-safe TypeScript code
Use camelCase for variable and function names (e.g., isFetchingData, handleUserInput)
Use TypeScript for all components, favoring interfaces for props and state
Avoid using any; strive for precise types
Use Expo SecureStore for sensitive data
Use zustand for state management
Use react-query for data fetching and caching
Use react-i18next for internationalization
Use react-native-mmkv for local storage
Use axios for API requests
**/*.{ts,tsx}: Write concise, type-safe TypeScript code
Use camelCase for variable and function names
Use TypeScript for all components, favoring interfaces for props and state
Avoid using any; strive for precise types
Use React Navigation for handling navigation and deep linking
Handle errors gracefully and provide user feedback
Use Expo's SecureStore for sensitive data
Use zustand for state management
Use react-query for data fetching
Use react-i18next for internationalization
Use react-native-mmkv for local storage
Use axios for API requests
Files:
src/services/__tests__/signalr.service.reconnect-fix.test.tssrc/services/signalr.service.tssrc/services/__tests__/signalr.service.test.ts
**/*
📄 CodeRabbit inference engine (.cursorrules)
Directory and file names should be lowercase and hyphenated (e.g., user-profile, chat-screen)
Files:
src/services/__tests__/signalr.service.reconnect-fix.test.tsdocs/signalr-reconnect-self-blocking-fix.mdsrc/services/signalr.service.tssrc/services/__tests__/signalr.service.test.ts
**/*.{test.ts,test.tsx,spec.ts,spec.tsx}
📄 CodeRabbit inference engine (.cursorrules)
Create and use Jest tests to validate all generated components
Files:
src/services/__tests__/signalr.service.reconnect-fix.test.tssrc/services/__tests__/signalr.service.test.ts
**/__tests__/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursorrules)
Generate tests for all components, services, and logic; ensure tests run without errors
Files:
src/services/__tests__/signalr.service.reconnect-fix.test.tssrc/services/__tests__/signalr.service.test.ts
**/*.{test,spec}.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Create and use Jest tests for all generated components
Files:
src/services/__tests__/signalr.service.reconnect-fix.test.tssrc/services/__tests__/signalr.service.test.ts
🧬 Code graph analysis (3)
src/services/__tests__/signalr.service.reconnect-fix.test.ts (2)
src/services/signalr.service.ts (2)
signalRService(619-619)SignalRService(620-620)src/lib/logging/index.tsx (1)
logger(80-80)
src/services/signalr.service.ts (1)
src/lib/logging/index.tsx (2)
logger(80-80)error(74-76)
src/services/__tests__/signalr.service.test.ts (1)
src/services/signalr.service.ts (2)
signalRService(619-619)SignalRHubConnectConfig(13-18)
🪛 LanguageTool
docs/signalr-reconnect-self-blocking-fix.md
[grammar] ~8-~8: There might be a mistake here.
Context: ...d 2. This could lead to scenarios where: - A reconnection attempt was in progress ...
(QB_NEW_EN)
[grammar] ~12-~12: There might be a mistake here.
Context: ...nnection failed, the hub would be stuck in reconnecting state - Future manual c...
(QB_NEW_EN)
[grammar] ~17-~17: There might be a mistake here.
Context: ...le reconnectingHubs Set to track both: - Automatic reconnection attempts - Direc...
(QB_NEW_EN)
[grammar] ~18-~18: There might be a mistake here.
Context: ... both: - Automatic reconnection attempts - Direct connection attempts This caused ...
(QB_NEW_EN)
[grammar] ~21-~21: There might be a mistake here.
Context: ...block direct connections when hubs were in reconnecting state. ## Solution Imple...
(QB_NEW_EN)
[grammar] ~45-~45: There might be a mistake here.
Context: ..._connectToHubWithEventingUrlInternal):** - Only block duplicate direct connections ...
(QB_NEW_EN)
[grammar] ~47-~47: There might be a mistake here.
Context: ... state) - Allow direct connections even when hub is in RECONNECTING state - Log ...
(QB_NEW_EN)
[grammar] ~48-~48: There might be a mistake here.
Context: ... - Log reconnecting state but proceed with connection attempt - Set state to `DIRE...
(QB_NEW_EN)
[grammar] ~49-~49: There might be a mistake here.
Context: ...empt - Set state to DIRECT_CONNECTING during connection attempt - Clean up state on ...
(QB_NEW_EN)
[grammar] ~50-~50: There might be a mistake here.
Context: ...TING` during connection attempt - Clean up state on both success and failure **Au...
(QB_NEW_EN)
[grammar] ~52-~52: There might be a mistake here.
Context: ... and failure Automatic Reconnections: - Set state to RECONNECTING during recon...
(QB_NEW_EN)
🔇 Additional comments (3)
src/services/signalr.service.ts (1)
259-286: Legacy connect path doesn’t reconnect; confirm intent.handleConnectionClose relies on hubConfigs (set only by connectToHubWithEventingUrl). Legacy connectToHub connections won’t be retried.
Do you want legacy hubs to auto-reconnect? If yes, mirror hubConfigs storage and a legacy reconnect path.
Also applies to: 386-494
src/services/__tests__/signalr.service.reconnect-fix.test.ts (1)
95-126: LGTM: direct-connecting vs reconnecting semantics are well-covered.src/services/__tests__/signalr.service.test.ts (1)
495-542: Align isHubAvailable/isHubReconnecting assertions with new state machine.The expectations look good; once you set onreconnecting/ onreconnected state (service change), add assertions accordingly.
Summary by CodeRabbit