CU-868ex18rd Fixing unit tests bug fixes #59
Conversation
WalkthroughAdds a release-notes input and consolidates release notes generation in CI. Introduces extensive Jest mocks and removes many analytics-heavy tests. Refactors multiple storage modules to use a shared storage instance and adds new storage helpers. Adjusts analytics flows (safer tracking, hashing) and routing/coordinate handling. Reworks offline queue processing, push notification registration, and several service internals. Changes
Sequence Diagram(s)sequenceDiagram
actor Dev as Developer
participant GH as GitHub Actions
participant Git as Repo
participant PR as PR Body
Dev->>GH: workflow_dispatch (release_notes?)
GH->>GH: Determine NOTES source
alt release_notes provided
GH->>GH: NOTES = input.release_notes
else PR body includes "## Release Notes"
GH->>PR: Extract section until next header
PR-->>GH: NOTES
else
GH->>Git: Read last 5 commits
Git-->>GH: Commit messages
GH->>GH: NOTES = joined messages
end
GH->>GH: Validate NOTES non-empty
alt NOTES empty
GH-->>Dev: Fail job
else
GH->>GH: Write RELEASE_NOTES.md (header + NOTES)
end
sequenceDiagram
participant App
participant PNS as PushNotificationService
participant Notif as Expo Notifications
participant Backend as registerDevice API
App->>PNS: registerForPushNotifications(userId, dept)
PNS->>Notif: requestPermissions/getExpoPushToken
Notif-->>PNS: token
PNS->>PNS: setup listeners, Android channels
PNS->>Backend: registerDevice { UserId, Token, DeviceUuid, Prefix }
Backend-->>PNS: ok
PNS-->>App: token or null
sequenceDiagram
participant App
participant OQP as offlineQueueProcessor
participant Store as Shared Storage
participant Net as NetInfo
participant API as savePersonnelStatus
App->>OQP: addPersonnelStatusToQueue(payload)
OQP->>Store: enqueue item
Net-->>OQP: isInternetReachable = true (event)
OQP->>Store: read queue
loop items
OQP->>API: savePersonnelStatus(payload)
alt success
API-->>OQP: 200
OQP->>Store: remove item
else failure
API-->>OQP: error
OQP->>OQP: schedule retry (backoff)
OQP->>Store: update retries
end
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
🪧 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: 22
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (8)
src/lib/storage/index.tsx (1)
18-24: Enforce a single MMKV instance to maintain stable hook subscriptionsAvoid reassigning the
storagevariable after creating the fallback—anyuseMMKV*hooks or event listeners bound to the original instance will not automatically migrate to a new one, leading to missed updates or non-deterministic behavior.Key areas to address:
- In src/lib/storage/index.tsx (lines 18–24), remove the ad-hoc fallback assignment of
new MMKV({ id: 'ResgridUnit' })and instead initializestorageonce with the secure instance.- In src/lib/storage/secure-storage.ts, refactor
getGeneralStorage()(around line 223) so it returns or configures the pre-existing fallback instance rather than instantiating a freshMMKVeach time.- Guard against concurrent initializations by adding a simple flag in
initializeStorage()(e.g.,if (storageInitialized) return;) and resetting it on error to allow safe retries.- Apply the same pattern to other modules (
units-filter.ts,realtime-geolocation.ts, etc.) to ensure they all share the identicalMMKVobject.Suggested patch in src/lib/storage/index.tsx:
-export let storage: MMKV; -let storageInitialized = false; - -// Synchronous storage fallback initialization -storage = new MMKV({ id: 'ResgridUnit' }); - -// Initialize secure storage -initializeStorage().catch(console.error); - -const initializeStorage = async () => { - if (!storageInitialized) { - storage = await getGeneralStorage(); - storageInitialized = true; - } -}; +export let storage: MMKV = new MMKV({ id: 'ResgridUnit' }); +let storageInitialized = false; + +// Async upgrade to secure storage without swapping out the instance +const initializeStorage = async () => { + if (storageInitialized) return; + storageInitialized = true; + try { + const secureInstance = await getGeneralStorage(); + // Optionally reconfigure `storage` here instead of replacing the object: + // e.g. Object.assign(storage, secureInstance) + } catch (e) { + storageInitialized = false; + throw e; + } +}; + +initializeStorage().catch((error) => { + console.error('Failed to initialize secure storage:', error); +});These changes ensure that all hooks and listeners stay connected to a single, stable
MMKVinstance throughout the app lifecycle.__mocks__/expo-audio.ts (1)
1-19: Ensure the manual mock is registered and (optionally) add a default exportYour tests import named functions from
expo-audio, but there’s nojest.mock('expo-audio')anywhere in the repo—so the manual mock in__mocks__/expo-audio.tsisn’t being applied. Please:
- Add an explicit mock registration in your Jest setup (e.g.,
jest-setup.tsorjest.config.js):// jest-setup.ts jest.mock('expo-audio');- (Optional) Extend the mock to include a default export, so future code using
import Audio from 'expo-audio'also works without test breakage. In__mocks__/expo-audio.ts, append:// ...existing mocks +// Default export to support `import Audio from 'expo-audio'` +export default { + getRecordingPermissionsAsync, + requestRecordingPermissionsAsync, +};src/services/bluetooth-audio.service.ts (1)
890-890: Verify audioService availability before method callsThe code calls methods on
audioServicewithout checking if they exist. With the current empty object fallback, this could cause runtime errors.Add safety checks before calling audioService methods:
// Play connected device sound - await audioService.playConnectedDeviceSound(); + if (audioService?.playConnectedDeviceSound) { + await audioService.playConnectedDeviceSound(); + }Apply similar checks for all audioService method calls at lines 1449, 1451, 1484, and 1486.
Also applies to: 1449-1449, 1451-1451, 1484-1484, 1486-1486
src/services/location.ts (1)
25-32: Validate numeric string conversions for location dataConverting numeric values to strings without validation could lead to issues with special numeric values (
NaN,Infinity). The fallback to'0'for accuracy values might not be appropriate for all use cases.- locationInput.Latitude = location.coords.latitude.toString(); - locationInput.Longitude = location.coords.longitude.toString(); - locationInput.Accuracy = location.coords.accuracy?.toString() || '0'; - locationInput.Altitude = location.coords.altitude?.toString() || '0'; - locationInput.AltitudeAccuracy = location.coords.altitudeAccuracy?.toString() || '0'; - locationInput.Speed = location.coords.speed?.toString() || '0'; - locationInput.Heading = location.coords.heading?.toString() || '0'; + // Validate and convert numeric values safely + locationInput.Latitude = isFinite(location.coords.latitude) ? location.coords.latitude.toString() : '0'; + locationInput.Longitude = isFinite(location.coords.longitude) ? location.coords.longitude.toString() : '0'; + locationInput.Accuracy = (location.coords.accuracy != null && isFinite(location.coords.accuracy)) ? location.coords.accuracy.toString() : null; + locationInput.Altitude = (location.coords.altitude != null && isFinite(location.coords.altitude)) ? location.coords.altitude.toString() : null; + locationInput.AltitudeAccuracy = (location.coords.altitudeAccuracy != null && isFinite(location.coords.altitudeAccuracy)) ? location.coords.altitudeAccuracy.toString() : null; + locationInput.Speed = (location.coords.speed != null && isFinite(location.coords.speed)) ? location.coords.speed.toString() : null; + locationInput.Heading = (location.coords.heading != null && isFinite(location.coords.heading)) ? location.coords.heading.toString() : null;src/services/push-notification.ts (1)
145-201: Clarify registerForPushNotifications parameter mappingIt looks like registerForPushNotifications is defined to take a unitId but the React hook invokes it with the userId, and that value is sent as UnitId in the payload to registerUnitDevice. The API model PushRegistrationUnitInput only includes a UnitId field, so passing a user’s ID here will likely misassociate the device.
Please address this mismatch by either:
- Renaming the first parameter of registerForPushNotifications (and the corresponding payload field) to something like subscriberId if it’s truly intended for user-based registration, or
- Updating the hook to pass the actual unit’s UnitId (if the endpoint is indeed for registering devices on units rather than users), and renaming the local variable userId to unitId for clarity.
Locations to update:
- src/services/push-notification.ts around the registerUnitDevice call (lines ~194–197) where the payload is built with UnitId: unitId.
- src/services/push-notification.ts in the hook (lines ~283–291) where you call
.registerForPushNotifications(userId, rights.DepartmentCode)—either swap in the correct context ID or rename parameters to reflect the true identifier.src/components/calendar/__tests__/calendar-item-details-sheet.test.tsx (1)
41-47: Fix non-RN host elements in mocks to prevent renderer crashesReturning DOM tags ('div', 'span', 'h1') and string component names ('Loading', 'Badge') can break react-native renderer. Replace with RN primitives.
Apply these diffs:
-jest.mock('@/components/common/loading', () => ({ - Loading: 'Loading', -})); +jest.mock('@/components/common/loading', () => { + const React = require('react'); + const { View } = require('react-native'); + return { Loading: ({ children }: any) => <View testID="loading">{children}</View> }; +}); -jest.mock('@/components/ui/badge', () => ({ - Badge: 'Badge', -})); +jest.mock('@/components/ui/badge', () => { + const React = require('react'); + const { View, Text } = require('react-native'); + return { Badge: ({ children }: any) => <View><Text>{children}</Text></View> }; +});-jest.mock('@/components/ui/heading', () => ({ - Heading: ({ children }: any) => <h1>{children}</h1>, -})); +jest.mock('@/components/ui/heading', () => { + const React = require('react'); + const { Text } = require('react-native'); + return { Heading: ({ children }: any) => <Text accessibilityRole="header">{children}</Text> }; +}); -jest.mock('@/components/ui/hstack', () => ({ - HStack: ({ children }: any) => <div>{children}</div>, -})); +jest.mock('@/components/ui/hstack', () => { + const React = require('react'); + const { View } = require('react-native'); + return { HStack: ({ children }: any) => <View>{children}</View> }; +});-jest.mock('@/components/ui/text', () => ({ - Text: ({ children }: any) => <span>{children}</span>, -})); +jest.mock('@/components/ui/text', () => { + const React = require('react'); + const { Text } = require('react-native'); + return { Text: ({ children }: any) => <Text>{children}</Text> }; +}); -jest.mock('@/components/ui/vstack', () => ({ - VStack: ({ children }: any) => <div>{children}</div>, -})); +jest.mock('@/components/ui/vstack', () => { + const React = require('react'); + const { View } = require('react-native'); + return { VStack: ({ children }: any) => <View>{children}</View> }; +});Also applies to: 71-77, 97-103
src/components/notes/__tests__/note-details-sheet-basic.test.tsx (1)
59-63: Avoid string host components for icons in RN testsReturning string component names can break RN renderer. Swap to functional mocks that render null or a View.
Apply this diff:
-jest.mock('lucide-react-native', () => ({ - Calendar: 'Calendar', - Tag: 'Tag', - X: 'X', -})); +jest.mock('lucide-react-native', () => { + const React = require('react'); + const { View } = require('react-native'); + const NullIcon = () => null; + return { + Calendar: NullIcon, + Tag: NullIcon, + X: NullIcon, + }; +});src/app/__tests__/onboarding.test.tsx (1)
413-442: Strengthen assertion for missing slide dataCurrent test passes even if no analytics call is made. Assert slideTitle falls back to 'Unknown' per implementation.
-// Should handle the case where slide data doesn't exist -const call = mockTrackEvent.mock.calls.find(call => call[0] === 'onboarding_slide_changed'); -if (call) { - const analyticsData = call[1]; - expect(analyticsData.slideTitle).toBeDefined(); -} +const call = mockTrackEvent.mock.calls.find(call => call[0] === 'onboarding_slide_changed'); +expect(call).toBeTruthy(); +const analyticsData = call![1]; +expect(analyticsData.slideTitle).toBe('Unknown');
__mocks__/react-native-ble-plx.ts
Outdated
| // Mock device creation helper | ||
| export const createMockDevice = (id: string, name: string | null = 'Mock Device', options: Partial<Device> = {}): Device => ({ | ||
| id, | ||
| name, | ||
| rssi: -60, | ||
| serviceUUIDs: ['0000110A-0000-1000-8000-00805F9B34FB'], | ||
| async isConnected() { | ||
| return true; | ||
| }, | ||
| async discoverAllServicesAndCharacteristics() { | ||
| return this; | ||
| }, | ||
| async services() { | ||
| return [ | ||
| { | ||
| uuid: '0000110A-0000-1000-8000-00805F9B34FB', | ||
| async characteristics() { | ||
| return [ | ||
| { | ||
| uuid: '0000FE59-0000-1000-8000-00805F9B34FB', | ||
| isNotifiable: true, | ||
| monitor: (callback) => { | ||
| // Simulate button press after a delay | ||
| setTimeout(() => { | ||
| const mockChar = { | ||
| uuid: '0000FE59-0000-1000-8000-00805F9B34FB', | ||
| isNotifiable: true, | ||
| value: Buffer.from([0x01]).toString('base64'), // Mute button | ||
| monitor: () => ({ remove: () => {} }), | ||
| }; | ||
| callback(null, mockChar); | ||
| }, 1000); | ||
| return { remove: () => {} }; | ||
| }, | ||
| }, | ||
| ]; | ||
| }, | ||
| }, | ||
| ]; | ||
| }, | ||
| onDisconnected(callback) { | ||
| return { remove: () => {} }; | ||
| }, | ||
| async cancelConnection() {}, | ||
| ...options, | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Timers aren’t cleaned up; Subscription.remove should cancel pending callbacks.
Both startDeviceScan and Characteristic.monitor schedule setTimeout callbacks without a way to cancel them, which can cause test flakiness and leaks. Wire timer cancellation into remove()/stopDeviceScan().
Apply this diff:
+import { Buffer } from 'buffer';
@@
export class BleManager {
private static mockState: State = State.PoweredOn;
private static mockDevices: Device[] = [];
- private static stateListener: ((state: State) => void) | null = null;
+ private static stateListeners = new Set<(state: State) => void>();
+ private static scanTimer: ReturnType<typeof setTimeout> | null = null;
@@
static setMockState(state: State) {
this.mockState = state;
- if (this.stateListener) {
- this.stateListener(state);
- }
+ this.stateListeners.forEach((l) => l(state));
}
@@
- onStateChange(listener: (state: State) => void, emitCurrentValue: boolean = false): Subscription {
- BleManager.stateListener = listener;
+ onStateChange(listener: (state: State) => void, emitCurrentValue: boolean = false): Subscription {
+ BleManager.stateListeners.add(listener);
if (emitCurrentValue) {
- listener(BleManager.mockState);
+ listener(BleManager.mockState);
}
return {
remove: () => {
- BleManager.stateListener = null;
+ BleManager.stateListeners.delete(listener);
},
};
}
@@
- startDeviceScan(uuids: string[] | null, options: any, listener: (error: BleError | null, device: Device | null) => void): Subscription {
+ startDeviceScan(
+ uuids: string[] | null,
+ options: Record<string, unknown> | undefined,
+ listener: (error: BleError | null, device: Device | null) => void
+ ): Subscription {
// Simulate finding devices
- setTimeout(() => {
+ BleManager.scanTimer = setTimeout(() => {
BleManager.mockDevices.forEach((device) => {
listener(null, device);
});
}, 100);
return {
- remove: () => {},
+ remove: () => {
+ if (BleManager.scanTimer) {
+ clearTimeout(BleManager.scanTimer);
+ BleManager.scanTimer = null;
+ }
+ },
};
}
@@
- stopDeviceScan(): void {}
+ stopDeviceScan(): void {
+ if (BleManager.scanTimer) {
+ clearTimeout(BleManager.scanTimer);
+ BleManager.scanTimer = null;
+ }
+ }
@@
export const createMockDevice = (id: string, name: string | null = 'Mock Device', options: Partial<Device> = {}): Device => ({
@@
- monitor: (callback) => {
+ monitor: (callback) => {
// Simulate button press after a delay
- setTimeout(() => {
+ const timeout = setTimeout(() => {
const mockChar = {
uuid: '0000FE59-0000-1000-8000-00805F9B34FB',
isNotifiable: true,
value: Buffer.from([0x01]).toString('base64'), // Mute button
- monitor: () => ({ remove: () => {} }),
+ monitor: () => ({ remove: () => {} }),
};
callback(null, mockChar);
}, 1000);
- return { remove: () => {} };
+ return { remove: () => clearTimeout(timeout) };
},This also adds a Buffer import for TS type safety across environments.
CLAUDE.md
Outdated
| - **Types**: TypeScript with strict mode, explicit return types on functions | ||
| - **Components**: Use functional components with React FC typing | ||
| - **State Management**: Use Zustand for global state management, React hooks for component state | ||
| - **Styling**: NativeWind (Tailwind CSS for React Native) |
There was a problem hiding this comment.
Inconsistent styling guidance: NativeWind vs gluestack-ui. Align to a single approach.
The "Code Style Guidelines" prescribe NativeWind (Tailwind), while "Best Practices" prescribe gluestack-ui (and fallbacks). This sends conflicting direction to contributors and will cause inconsistent UI. Given the repository-wide guidance (and retrieved learnings) emphasize gluestack-ui-first with StyleSheet/styled-components fallback, remove the NativeWind directive here and keep gluestack-ui as the single source of truth.
Apply this diff to reconcile:
- - **Styling**: NativeWind (Tailwind CSS for React Native)
+ - **Styling**: Prefer gluestack-ui components from `components/ui`. If a component does not exist, style via `StyleSheet.create()` or `styled-components`. Avoid mixing styling systems within the same feature.Note: This is consistent with prior org guidance to prefer gluestack-ui across .tsx files.
Also applies to: 45-47
🤖 Prompt for AI Agents
In CLAUDE.md around lines 26 and 45-47, remove the NativeWind (Tailwind CSS for
React Native) styling directive and replace it with a single consistent rule:
prefer gluestack-ui-first for component styling and, where gluestack-ui is
unavailable, fall back to React Native StyleSheet or styled-components; update
the "Code Style Guidelines" section to match the "Best Practices" guidance so
the document prescribes gluestack-ui as the single source of truth for .tsx
files and remove any conflicting references to NativeWind.
CLAUDE.md
Outdated
| - **Paths**: Use absolute imports with `@/` alias (`import X from '@/components/Y'`) | ||
| - **Organization**: Organize files by feature, grouping related components, hooks, and styles. |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Absolute-import alias requires config in tsconfig and Jest.
The @/ alias needs to be reflected in tsconfig.json (paths/baseUrl) and in Jest’s moduleNameMapper to avoid broken imports in tests.
Run this to verify configuration exists:
If missing, I can generate the minimal tsconfig and Jest mappings.
Also applies to: 59-70
🏁 Script executed:
#!/bin/bash
# Verify tsconfig alias and jest mapping are set for '@/'
rg -nP '"baseUrl"\s*:\s*"' tsconfig*.json -C2
rg -nP '"paths"\s*:\s*{\s*"@/\*"' tsconfig*.json -C3
fd -a jest.config* | xargs -I{} rg -nP 'moduleNameMapper.*"^@/(.*)$"' {}Length of output: 314
🏁 Script executed:
#!/usr/bin/env bash
set -e
echo "🔍 Listing Jest config files..."
fd --type f -g 'jest.config.*' || echo "No jest.config.* files found"
echo
echo "🔍 Searching for moduleNameMapper in Jest config files..."
for file in $(fd --type f -g 'jest.config.*'); do
echo "—in $file:"
rg -nP 'moduleNameMapper' -C2 "$file" || echo " ↳ No moduleNameMapper found in $file"
done
echo
echo "🔍 Inspecting package.json for Jest settings..."
if [ -f package.json ]; then
echo "—Looking for \"jest\" section:"
rg -nP '"jest"\s*:' -C5 package.json || echo " ↳ No \"jest\" field in package.json"
echo
echo "—Looking for moduleNameMapper inside package.json:"
rg -nP 'moduleNameMapper' -C2 package.json || echo " ↳ No moduleNameMapper in package.json"
else
echo "package.json not found"
fiLength of output: 1523
Add Jest mapping for @/ alias
The @/* alias is already configured in tsconfig.json ("baseUrl": "." and "paths": { "@/*": ["./src/*"] }), but Jest’s moduleNameMapper in jest.config.js only handles CSS modules. You need to add the following mapping so imports like import X from '@/components/Y' resolve in tests.
• File to update:
- jest.config.js (around the existing moduleNameMapper)
• Diff suggestion:
moduleNameMapper: {
- '\\.(css|less|scss|sass)$': 'identity-obj-proxy',
+ '^@/(.*)$': '<rootDir>/src/$1',
+ '\\.(css|less|scss|sass)$': 'identity-obj-proxy',
},This ensures that Jest can resolve @/ imports to the src/ directory.
📝 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.
| - **Paths**: Use absolute imports with `@/` alias (`import X from '@/components/Y'`) | |
| - **Organization**: Organize files by feature, grouping related components, hooks, and styles. | |
| moduleNameMapper: { | |
| '^@/(.*)$': '<rootDir>/src/$1', | |
| '\\.(css|less|scss|sass)$': 'identity-obj-proxy', | |
| }, |
🤖 Prompt for AI Agents
In CLAUDE.md around lines 28 to 29, note that Jest cannot resolve the `@/`
TypeScript path alias used in imports; update jest.config.js to add a
moduleNameMapper entry mapping '^@/(.*)$' to '<rootDir>/src/$1' (alongside the
existing CSS module mappings) so Jest resolves `@/` imports during tests; update
and run tests to confirm resolution.
src/stores/app/core-store.ts
Outdated
| // Currently active unit ID for location service | ||
| activeUnitId: string | null; | ||
| activeStatuses: StatusesResultData[] | null; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
New activeUnitId state added—consider adding an explicit setter and persistence migration notes.
Adding the field is fine. To avoid ad-hoc mutations and to keep API symmetry with other setters, expose setActiveUnitId(unitId: string | null). Also consider whether any consumers depend on previous shape; while Zustand’s merge usually preserves defaults, call out that no migration is needed.
I can provide a small patch to add the setter if you confirm the intended update flow (e.g., called by location service on unit switch).
Quick scan for usages/missing setter:
🏁 Script executed:
#!/bin/bash
# Find reads/writes of activeUnitId and any prior names (e.g., activeUnit)
rg -nP -C2 '\bactiveUnitId\b|\bactiveUnit\b' --type=ts --type=tsxLength of output: 95
🏁 Script executed:
#!/bin/bash
# Locate the core-store file
find . -type f -name core-store.ts
# Print the first 200 lines of core-store.ts
sed -n '1,200p' src/stores/app/core-store.ts
# Search for any reads/writes of activeUnitId or legacy activeUnit across TS/TSX
echo "Searching for activeUnitId usage:"
rg -nP -C2 '\bactiveUnitId\b|\bactiveUnit\b' -g '*.ts' -g '*.tsx'Length of output: 25359
Implement setActiveUnitId and update unit selection flow
Currently, activeUnitId in the core store is never updated, so services/location.ts always sees null and skips the location API call. To fix this critical breakage:
• Expose a setter in CoreState and implement it in core-store.ts.
• Call this setter wherever the user switches units (e.g., in your roles store’s init or in the unit-selection component).
• No persistence migration is required—Zustand’s persist merges new fields with defaults and will initialize activeUnitId to null when absent.
Bullet-point summary:
- Add to
CoreStateinterface:
•setActiveUnitId: (unitId: string | null) => void; - In the
persistconfig foruseCoreStore, implement:
•setActiveUnitId: (unitId) => set({ activeUnitId: unitId }), - After a unit is selected (e.g., in your roles store or selection component), invoke:
•useCoreStore.getState().setActiveUnitId(selectedUnitId);
Example diff snippet:
--- a/src/stores/app/core-store.ts
+++ b/src/stores/app/core-store.ts
@@ interface CoreState {
activeUnitId: string | null;
activeStatuses: StatusesResultData[] | null;
+ /** Set the currently active unit for location tracking */
+ setActiveUnitId: (unitId: string | null) => void;
@@ export const useCoreStore = create<CoreState>()(
persist(
(set, get) => ({
activeUnitId: null,
+ // New setter for activeUnitId
+ setActiveUnitId: (unitId) => set({ activeUnitId: unitId }),
activeCallId: null,
// ...
})
)
);Then, in your unit-selection flow (for example in src/stores/roles/store.ts or wherever a unit is chosen):
import { useCoreStore } from '@/stores/app/core-store';
// After fetching or selecting a unit
useCoreStore.getState().setActiveUnitId(chosenUnit.UnitId);🤖 Prompt for AI Agents
In src/stores/app/core-store.ts around lines 23 to 25, activeUnitId is never
updated so location service always sees null; add a setter to CoreState and
implement it in the persist config: add setActiveUnitId: (unitId: string | null)
=> void to the interface and implement setActiveUnitId: (unitId) => set({
activeUnitId: unitId }) in the useCoreStore persist initializer, then call
useCoreStore.getState().setActiveUnitId(selectedUnitId) wherever the user
switches units (e.g., roles store init or unit-selection component).
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 (3)
src/services/push-notification.ts (1)
185-192: Do not log raw push tokensPush tokens are sensitive and can be abused. Avoid emitting the full token at info level.
Minimal, safe change:
logger.info({ message: 'Push notification token obtained', context: { - token: this.pushToken, userId, platform: Platform.OS, }, });If you need traceability, log only a suffix:
- token: this.pushToken, + tokenSuffix: this.pushToken.slice(-6),src/lib/storage/units-filter.ts (1)
10-18: Load path should ensure storage readinessIf you keep using
storage.getString, add an awaitable guard at the top:export const loadUnitsFilterOptions = async (): Promise<string[]> => { try { + // await ensureStorageReady(); // if exported from index.tsx const filterOptions = storage.getString(UNITS_FILTER_OPTIONS); const parsedOptions = filterOptions ? JSON.parse(filterOptions) : [];Without this, early app calls can throw during cold start.
src/lib/storage/index.tsx (1)
1-74: Action Required: EnsurestorageIs Initialized Before Any Synchronous AccessWe’ve identified numerous modules that import and use
storagesynchronously at startup, which may execute before the asynchronous initialization insrc/lib/storage/index.tsxcompletes. To prevent runtime errors or unexpectedundefinedbehavior, each of these consumers must be guarded or deferred until afterstorageis fully initialized.Files and locations requiring attention:
- Hooks:
src/lib/hooks/use-selected-theme.tsx(line 34:storage.getString(SELECTED_THEME))src/lib/hooks/use-keep-alive.tsx(line 40:storage.getBoolean(KEEP_ALIVE_ENABLED))src/lib/hooks/use-realtime-geolocation.ts(import & anystorage.get*/storage.set)src/lib/hooks/use-location-tracking.ts(import & anystorage.*)src/lib/hooks/use-background-geolocation.ts(import & anystorage.*)- Cache:
src/lib/cache/cache-manager.ts(lines 38, 43, 52, 61, 68:storage.getString/storage.set/storage.delete)- Storage utilities:
src/lib/storage/personnel-filter.ts(lines 13:storage.getString, 34:storage.set, 52:storage.delete)src/lib/storage/realtime-geolocation.ts(lines 12:storage.getBoolean, 31:storage.set)src/lib/storage/background-geolocation.ts(lines 18:storage.getBoolean, 37:storage.set)src/lib/storage/units-filter.ts(lines 11:storage.getString, 32:storage.set, 50:storage.delete)src/lib/storage/offline-queue-storage.ts(lines 57, 89, 122:this.storage.set/.getString/.delete)- Services:
src/services/offline-queue-processor.ts(lines 40, 56, 75:storage.getString/storage.set)Next steps:
- Add a guard or promise to defer these calls until after
initializeStorage()completes (e.g., check a ready flag or await initialization).- Alternatively, wrap each synchronous access in a helper that waits for
storageto be ready.- Ensure test environment fallbacks remain unaffected.
🧹 Nitpick comments (48)
src/components/units/__tests__/units-filter-sheet-basic.test.tsx (5)
21-23: Make i18n mock resilient: support defaultValue and key fallbackCurrent t signature ignores options/defaultValue and can return undefined. Return options.defaultValue if present, otherwise the key.
Apply this diff:
-jest.mock('react-i18next', () => ({ - useTranslation: () => ({ t: (key: string, def: string) => def }), -})); +jest.mock('react-i18next', () => ({ + useTranslation: () => ({ + t: (key: string, opts?: { defaultValue?: string }) => + (opts && opts.defaultValue) ?? key, + }), +}));
3-7: Future‑proof icon mocks to cover any icon importExplicitly listing icons is brittle. Use a Proxy so any named icon yields a null component.
Apply this diff:
-jest.mock('lucide-react-native', () => ({ - Check: () => null, - Filter: () => null, - X: () => null, -})); +jest.mock('lucide-react-native', () => + new Proxy( + {}, + { + get: () => () => null, + } + ) +);
27-29: Cover both alias and relative loading pathsIf the component ever imports Loading via a relative path instead of the alias, the mock won’t match. Add a sibling mock to reduce flakiness across refactors.
Apply this diff:
jest.mock('@/components/common/loading', () => ({ Loading: () => null, })); +jest.mock('../../common/loading', () => ({ + Loading: () => null, +}));
30-45: Avoid any in TS: type children as ReactNode across UI passthroughsTests should also be type-safe. Replace any with ReactNode in the passthrough component mocks.
Apply these diffs:
+import type { ReactNode } from 'react';-jest.mock('../../ui/actionsheet', () => ({ - Actionsheet: ({ children }: { children: any }) => children, +jest.mock('../../ui/actionsheet', () => ({ + Actionsheet: ({ children }: { children: ReactNode }) => children, ActionsheetBackdrop: () => null, - ActionsheetContent: ({ children }: { children: any }) => children, + ActionsheetContent: ({ children }: { children: ReactNode }) => children, ActionsheetDragIndicator: () => null, - ActionsheetDragIndicatorWrapper: ({ children }: { children: any }) => children, + ActionsheetDragIndicatorWrapper: ({ children }: { children: ReactNode }) => children, })); -jest.mock('../../ui/badge', () => ({ Badge: ({ children }: { children: any }) => children })); -jest.mock('../../ui/box', () => ({ Box: ({ children }: { children: any }) => children })); -jest.mock('../../ui/button', () => ({ Button: ({ children }: { children: any }) => children })); +jest.mock('../../ui/badge', () => ({ Badge: ({ children }: { children: ReactNode }) => children })); +jest.mock('../../ui/box', () => ({ Box: ({ children }: { children: ReactNode }) => children })); +jest.mock('../../ui/button', () => ({ Button: ({ children }: { children: ReactNode }) => children })); jest.mock('../../ui/checkbox', () => ({ Checkbox: () => null })); -jest.mock('../../ui/heading', () => ({ Heading: ({ children }: { children: any }) => children })); -jest.mock('../../ui/hstack', () => ({ HStack: ({ children }: { children: any }) => children })); -jest.mock('../../ui/pressable', () => ({ Pressable: ({ children }: { children?: any }) => children })); -jest.mock('../../ui/text', () => ({ Text: ({ children }: { children?: any }) => children })); -jest.mock('../../ui/vstack', () => ({ VStack: ({ children }: { children?: any }) => children })); +jest.mock('../../ui/heading', () => ({ Heading: ({ children }: { children: ReactNode }) => children })); +jest.mock('../../ui/hstack', () => ({ HStack: ({ children }: { children: ReactNode }) => children })); +jest.mock('../../ui/pressable', () => ({ Pressable: ({ children }: { children?: ReactNode }) => children })); +jest.mock('../../ui/text', () => ({ Text: ({ children }: { children?: ReactNode }) => children })); +jest.mock('../../ui/vstack', () => ({ VStack: ({ children }: { children?: ReactNode }) => children }));
1-1: Minor: add ReactNode import to support stricter child typingsEnables replacing any with ReactNode in the mocks without introducing implicit any.
Apply this diff:
-import { describe, expect, it, jest } from '@jest/globals'; +import { describe, expect, it, jest } from '@jest/globals'; +import type { ReactNode } from 'react';src/services/__tests__/offline-queue-processor.test.ts (2)
24-26: Avoid asserting on instanceof; it forces prototype mutation in the implementationThe
instanceof OfflineQueueProcessorassertion couples the test to a prototype hack and obscures the real contract (methods/behavior). Prefer verifying the public API surface.Apply this diff:
- it('should be an instance of OfflineQueueProcessor', () => { - expect(offlineQueueProcessor).toBeInstanceOf(OfflineQueueProcessor); - }); + it('should expose the OfflineQueueProcessor public API', () => { + expect(typeof offlineQueueProcessor.addPersonnelStatusToQueue).toBe('function'); + expect(typeof offlineQueueProcessor.startProcessing).toBe('function'); + expect(typeof offlineQueueProcessor.startBackgroundProcessing).toBe('function'); + expect(typeof offlineQueueProcessor.cleanup).toBe('function'); + });
28-31: Test currently bakes in stub-only behavior; make it resilient across environmentsExpecting an empty string ties the test to the stub. In production, the method returns a non-empty id.
Consider one of:
- Make the assertion environment-aware.
- Or, better, assert contract not value shape.
Example:
- it('should return empty string for addPersonnelStatusToQueue', () => { - const input = new SavePersonStatusInput(); - const result = offlineQueueProcessor.addPersonnelStatusToQueue(input); - expect(result).toBe(''); - }); + it('addPersonnelStatusToQueue should return a string id without throwing', () => { + const input = new SavePersonStatusInput(); + const result = offlineQueueProcessor.addPersonnelStatusToQueue(input); + expect(typeof result).toBe('string'); + });If you want to verify stub behavior specifically, set NODE_ENV to 'test' and assert
result === ''in a dedicated test.src/services/offline-queue-processor.ts (6)
64-68: Fire-and-forget enqueue risks losing items on crash; consider making it async or durably synchronous
addPersonnelStatusToQueuereturns before persistence completes (enqueueis async). If the app crashes, the item may be lost.Two options:
- Preferred: make the API async and await persistence.
- addPersonnelStatusToQueue(status: SavePersonStatusInput): string { - const id = `${Date.now()}-${Math.random()}`; - this.enqueue({ id, type: 'personnelStatus', payload: status, retries: 0 }); - return id; - } + async addPersonnelStatusToQueue(status: SavePersonStatusInput): Promise<string> { + const id = this.generateId(); + await this.enqueue({ id, type: 'personnelStatus', payload: status, retries: 0 }); + return id; + }
- If the signature must remain sync, document the at-least-once semantics and surface a callback/event when the write completes so callers can reflect persistence status in the UI.
Also consider switching the id to a UUID for stability (see separate note).
Also applies to: 70-76
65-66: Use UUIDs instead of Date.now() + Math.random() for id collisions and readabilityThis combination has a small but real collision risk and hinders traceability.
Apply this diff (requires
uuidorreact-native-uuid):+import { v4 as uuid } from 'uuid'; @@ - const id = `${Date.now()}-${Math.random()}`; + const id = uuid();If adding deps is undesirable, at least clamp
Math.random()to a fixed precision to reduce accidental scientific-notation stringification.
49-52: Backoff inside the loop blocks the entire queue; schedule next run insteadAwaiting the backoff for each failing item serializes all retries and keeps
processinglocked. Prefer recordingretriesand re-queueing immediately, then scheduling a singlesetTimeoutto callprocessQueue()again after the minimum backoff across remaining items.Sketch:
- item.retries++; - const backoff = Math.min(2 ** item.retries * 1000, 30000); - await new Promise((resolve) => setTimeout(resolve, backoff)); - remaining.push(item); + item.retries++; + remaining.push(item); + minNextDelayMs = Math.min(minNextDelayMs, Math.min(2 ** item.retries * 1000, 30000));Then after writing the merged queue:
if (merged.length > 0 && Number.isFinite(minNextDelayMs)) { setTimeout(() => { void this.processQueue(); }, minNextDelayMs); }This keeps the queue responsive and avoids long blocking sleeps.
125-136: Prototype mutation to satisfy instanceof is brittle; prefer a stable interface
Object.setPrototypeOf(instance, OfflineQueueProcessor.prototype)is a footgun—hard to reason about and can breakinstanceofchecks for Real/Stub. Expose a stable interface and drop the mutation; adjust tests accordingly.Apply this diff:
export class OfflineQueueProcessor extends ProcessorClass { /** * Returns the singleton instance of the processor. */ - static getInstance(): RealOfflineQueueProcessor | StubOfflineQueueProcessor { - const instance = ProcessorClass.getInstance(); - // Ensure the instance is recognized as OfflineQueueProcessor - Object.setPrototypeOf(instance, OfflineQueueProcessor.prototype); - return instance; - } + static getInstance(): RealOfflineQueueProcessor | StubOfflineQueueProcessor { + return ProcessorClass.getInstance(); + } }And update the test to assert API shape (see test comment).
91-113: Unify return contract across environments for predictabilityThe stub returns
''while the real impl returns a non-empty id. This divergence complicates callers and tests. Consider returning a synthetic id from the stub too (e.g.,debug-<uuid>), and logging the stub usage.Apply this diff:
addPersonnelStatusToQueue(status: SavePersonStatusInput): string { if (process.env.NODE_ENV === 'production') { logger.error({ message: 'Stub offline queue used in production' }); throw new Error('OfflineQueueProcessor stub used in production'); } - return ''; + return 'debug-' + Date.now().toString(36); }
70-76: Nit: remove ineffective awaits on synchronous MMKV.set
MMKV#setis synchronous. Usingawaithere is harmless but misleading and may hide accidental Promise assumptions.Apply this diff:
- await storage.set(this.storageKey, JSON.stringify(items)); + storage.set(this.storageKey, JSON.stringify(items)); @@ - return Promise.resolve(); + return Promise.resolve();(Leaving the explicit
Promise.resolve()in the stub is fine for clarity.)Also applies to: 100-106
src/services/__tests__/audio.service.test.ts (2)
69-71: Use enum constants from expo-av instead of string literals; verify service sets Android interruption modeGood call adding interruptionModeAndroid. To make the assertion resilient to mock differences across files (jest-setup vs local), prefer the enum exports over raw strings and ensure the service always passes the Android mode (even on iOS) as your expectation requires.
Suggested changes:
- import { Audio, InterruptionModeIOS } from 'expo-av'; + import { Audio, InterruptionModeIOS, InterruptionModeAndroid } from 'expo-av'; it('should set audio mode correctly', () => { expect(mockAudioSetAudioModeAsync).toHaveBeenCalledWith({ allowsRecordingIOS: true, staysActiveInBackground: true, playsInSilentModeIOS: true, shouldDuckAndroid: true, playThroughEarpieceAndroid: true, - interruptionModeIOS: 'doNotMix', - interruptionModeAndroid: 'duckOthers', + interruptionModeIOS: InterruptionModeIOS.DoNotMix, + interruptionModeAndroid: InterruptionModeAndroid.DuckOthers, }); });If the underlying audio.service sets interruptionModeAndroid conditionally by platform, this test will fail on iOS; confirm the intended cross-platform behavior.
Also applies to: 115-124
17-42: Reduce duplicate module mocks to avoid drift with jest-setupThis test re-mocks expo-asset and expo-av even though global mocks are provided in jest-setup.ts. Keeping both can lead to divergence (e.g., enum values in jest-setup are numeric while this file uses strings). Consider relying on the centralized mocks unless a test requires specific behavior, in which case document the differences here.
src/app/(app)/home/personnel.tsx (2)
80-85: Replace && with ?: for conditional rendering per repo guidelinesThe repo guidelines prefer the ternary operator over && in TSX.
Apply:
- {selectedFilters.length > 0 && ( - <Badge size="sm" variant="solid" className="bg-blue-500"> - <Text className="text-xs text-white">{selectedFilters.length}</Text> - </Badge> - )} + {selectedFilters.length > 0 ? ( + <Badge size="sm" variant="solid" className="bg-blue-500"> + <Text className="text-xs text-white">{selectedFilters.length}</Text> + </Badge> + ) : null}
93-99: Avoid inline renderItem; memoize to reduce re-renders on large listsInline lambdas recreate every render and can hurt FlatList perf.
Apply:
- renderItem={({ item }) => <PersonnelCard personnel={item} onPress={selectPersonnel} />} + renderItem={renderPersonnelItem}And add above the return:
const renderPersonnelItem = React.useCallback( ({ item }: { item: (typeof filteredPersonnel)[number] }) => ( <PersonnelCard personnel={item} onPress={selectPersonnel} /> ), [selectPersonnel] );Optionally consider FlatList perf props where applicable: removeClippedSubviews, maxToRenderPerBatch, windowSize.
src/services/location.ts (4)
18-26: Harden numeric guard: prefer Number.isFinite and avoid coercion edge casesGlobal isFinite coerces values; Number.isFinite is stricter and safer for your types. Also explicitly guard non-number inputs.
Apply:
-const safeNumericString = (value: number | null | undefined, field: string): string => { - // Treat null, undefined, NaN, and Infinity as invalid - if (value == null || !isFinite(value)) { +const safeNumericString = (value: number | null | undefined, field: string): string => { + // Treat null, undefined, NaN, and Infinity as invalid + if (typeof value !== 'number' || !Number.isFinite(value)) { logger.warn({ message: `Invalid ${field} value: ${value}, defaulting to '0'` }); return '0'; } return value.toString(); };
5-13: Remove unused imports to keep builds green under noUnusedLocals and reduce bundle sizesetPersonLocation, SavePersonnelLocationInput, and useAuthStore aren’t used anymore after the unit-based switch.
Apply:
-import { setPersonLocation } from '@/api/personnel/personnelLocation'; import { setUnitLocation } from '@/api/units/unitLocation'; -import { useAuthStore } from '@/lib/auth'; @@ -import { SavePersonnelLocationInput } from '@/models/v4/personnelLocation/savePersonnelLocationInput'; import { SaveUnitLocationInput } from '@/models/v4/unitLocation/saveUnitLocationInput';If noUnusedLocals is enabled (common in TS configs), these will fail CI until removed.
184-195: Async watchPosition handler: consider simple in-flight guard to avoid backpressureIf updates arrive faster than the network, awaiting each call can cause a backlog. A minimal in-flight guard prevents piling up requests.
Example:
let foregroundInFlight = false; this.locationSubscription = await Location.watchPositionAsync( { accuracy: Location.Accuracy.Balanced, timeInterval: 15000, distanceInterval: 10 }, async (location) => { if (foregroundInFlight) return; foregroundInFlight = true; try { logger.info({ message: 'Foreground location update received', context: { latitude: location.coords.latitude, longitude: location.coords.longitude, heading: location.coords.heading } }); useLocationStore.getState().setLocation(location); await sendLocationToAPI(location); } finally { foregroundInFlight = false; } } );
219-230: Avoid dual background channels (TaskManager + watchPositionAsync) to prevent duplicate sends and battery drainYou’re registering a TaskManager background task and also starting a background watchPositionAsync subscription. This can double-send updates when the app is backgrounded.
Simplify to one background mechanism (recommended: TaskManager). Use watchPositionAsync for foreground only, and gate background watch removal behind AppState and TaskManager registration state. I can provide a refactor if you want to consolidate.
jest-setup.ts (2)
43-47: Partially mock @react-navigation/native to avoid breaking other exports; return cleanup when providedStubbing the entire module can break tests importing other symbols. Prefer a partial mock that preserves real exports and returns any cleanup function from the effect.
Apply:
-jest.mock('@react-navigation/native', () => ({ - useFocusEffect: (callback: () => void) => callback(), -})); +jest.mock('@react-navigation/native', () => { + const actual = jest.requireActual('@react-navigation/native'); + return { + ...actual, + useFocusEffect: (effect: () => void | (() => void)) => { + const cleanup = effect(); + return typeof cleanup === 'function' ? cleanup : undefined; + }, + }; +});
195-204: Unify InterruptionMode values with test expectations to reduce cross-file driftIn jest-setup, enums are numeric (0) but some tests assert string values (e.g., 'duckOthers'). Aligning on strings improves consistency and readability across the suite.
Apply:
jest.mock('expo-av', () => ({ Audio: { setAudioModeAsync: jest.fn().mockResolvedValue(undefined), Sound: { createAsync: jest.fn().mockResolvedValue({ sound: { setPositionAsync: jest.fn(), playAsync: jest.fn(), unloadAsync: jest.fn() } }), }, }, - InterruptionModeAndroid: { DuckOthers: 0 }, - InterruptionModeIOS: { DoNotMix: 0 }, + InterruptionModeAndroid: { DuckOthers: 'duckOthers' }, + InterruptionModeIOS: { DoNotMix: 'doNotMix' }, }));If any test relies on numeric constants, keep both forms or adapt those tests accordingly.
src/services/push-notification.ts (4)
178-184: Redundant platform branch and unnecessary cast
getDevicePushTokenAsync()returns a string indataon both platforms. The conditional and cast add noise.Apply:
- // The token format depends on the platform - const token = Platform.OS === 'ios' ? devicePushToken.data : devicePushToken.data; - - this.pushToken = token as string; + // The token is a string on both platforms + const token = devicePushToken.data; + this.pushToken = token;
229-229: Nit: update copy to reflect user-based flow (optional)The test notification body still says “Resgrid Unit”. Consider adjusting to avoid confusion in UX and tests.
Proposed:
- body: 'This is a test notification from Resgrid Unit', + body: 'This is a test notification from Resgrid',
194-201: PushRegistrationInput contract verified; refactor Platform magic numbersI’ve confirmed that
PushRegistrationInputis defined insrc/models/v4/device/pushRegistrationInput.tswith exactly these PascalCase properties:
- UserId: string
- Token: string
- Platform: number
- DeviceUuid: string
- Prefix: string
And that
Platformis intended to be 1 for iOS and 2 for Android, matching the backend contract.To improve readability and reduce “magic number” risk, consider extracting that mapping into a local constant or enum:
- await registerDevice({ - UserId: userId, - Token: this.pushToken, - Platform: Platform.OS === 'ios' ? 1 : 2, - DeviceUuid: getDeviceUuid() || '', - Prefix: departmentCode, - }); + const platform = Platform.OS === 'ios' ? 1 : 2; + await registerDevice({ + UserId: userId, + Token: this.pushToken, + Platform: platform, + DeviceUuid: getDeviceUuid() || '', + Prefix: departmentCode, + });Locations to address:
- src/services/push-notification.ts (lines 194–201): extract the
Platform.OS→ number mapping into a named constant or enum for clarity.Optional enhancement:
- Add a lightweight retry (e.g., 2–3 attempts with exponential backoff) around
registerDeviceto better handle transient failures per the repo’s offline/queueing guidelines.
6-6: Remove the unusedregisterUnitDeviceimportThe import of
registerUnitDeviceon line 6 ofsrc/services/push-notification.tsisn’t referenced anywhere in that file (only its definition in the API and a mock in tests) and should be removed to eliminate dead code.Locations to update:
src/services/push-notification.tsline 6Suggested diff:
- import { registerDevice, registerUnitDevice } from '@/api/devices/push'; + import { registerDevice } from '@/api/devices/push';src/lib/storage/secure-storage.ts (5)
171-185: Web path: comment vs. behavior and unused encrypted wrapper
- The comment says “disable persistence on web for PII safety,” but the code still creates an
MMKVinstance forofflineQueueStorage. If the intent is ephemeral, either skip initializing it or back it with a non-persistent in-memory store.WebEncryptedStorageis constructed but never wired intoMMKVor exported as the primary storage for web. Ifreact-native-mmkvon web isn’t guaranteed in your environment, this will silently bypass the encryption wrapper.Consider one of:
- Use
WebEncryptedStoragedirectly for web consumers; or- Ensure your
MMKVweb implementation is configured to use the encrypted wrapper; or- Make
offlineQueueStorageephemeral on web.Minimal change to align code with the comment:
- offlineQueueStorage = new MMKV({ - id: 'ResgridOfflineQueue', - }); + // Disable persistence for the offline queue on web + offlineQueueStorage = undefined;If other modules expect a defined storage for the offline queue, expose a no-op/in-memory shim instead.
14-33: Randomness on mobile: ensurecrypto.getRandomValuesis available and typedRequiring
react-native-get-random-valuesinsidegenerateSecureKeymay work at runtime but can still trip TypeScript or bundling in some setups. Prefer importing once at module scope so the polyfill is guaranteed before use:+// Ensures global.crypto.getRandomValues exists in RN +import 'react-native-get-random-values';Then you can safely call
globalThis.crypto.getRandomValues(array)and keep types happy.Also consider guarding if
globalThis.crypto?.getRandomValuesis unavailable to avoid rare crashes in niche environments.
271-281: Key rotation may desynchronize consumers holding the old instanceReassigning
generalStorageandofflineQueueStorageto newMMKVinstances means any modules that cached or injected the old instance (e.g.,storageexported fromindex.tsx) keep pointing at stale storages. Either:
- Emit an event and let dependents refresh references; or
- Centralize all storage access through a delegating wrapper that swaps the underlying instance transparently; or
- Restart the app after rotation.
I recommend a delegating wrapper for
storage(seeindex.tsxcomment) so rotation is seamless.
92-116: Security fallback silently stores plaintext on failuresOn web,
encrypt()failure falls back to returning plaintext, andset()falls back to storing plaintext in localStorage. That’s a pragmatic fallback but it weakens guarantees silently. At minimum, add a prominent log or a feature flag to disallow plaintext fallback in production builds.Optionally, gate by environment:
- return data; // Fallback to unencrypted data + if (process.env.NODE_ENV === 'production') { + throw new Error('Encryption failed in production'); + } + return data; // dev/test fallbackAnd in
set(), bail instead of storing plaintext in production.Also applies to: 118-146
246-251: Surface init failure from module-load initializerThe catch logs at module load are good, but consider retrying with backoff or exposing an
ensureStorageReady()that callers can await if startup order changes. This complements the Promise guard above and keeps consumers in control.src/lib/storage/index.tsx (5)
42-45: getItem should tolerate parse errors and undefinedstorageduring early startupWith the above fallback, undefined
storageis resolved. Consider also catchingJSON.parseerrors to avoid throwing on corrupted values: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) return null; + try { + return JSON.parse(value) as T; + } catch { + return null; + } }
47-53: Remove unnecessary async or return a Promise consistentlyThese functions are sync wrappers over MMKV but marked
async. Either removeasyncor returnPromise<void>intentionally to keep a consistent async API. If you keep them async, document that they never reject.-export async function setItem<T>(key: string, value: T) { +export function setItem<T>(key: string, value: T) { storage.set(key, JSON.stringify(value)); } -export async function removeItem(key: string) { +export function removeItem(key: string) { storage.delete(key); }
55-66: zustand storage depends onstoragebeing ready synchronouslyZustand’s
StateStorageis synchronous; it cannot await initialization. The in-memory shim change above is necessary sozustandStorageis safe to use at app boot. Without it, persisters may crash or drop state.
68-74:useIsFirstTimemay crash early; avoid passing an unready instanceIf
storagewas undefined, passing it here would crash. With the in-memory shim change, it’s mitigated. Still, consider omitting the instance to use the library default (if configured) or guard via a small hook that returns[true]until storage is ready.Minimal improvement:
- const [isFirstTime, setIsFirstTime] = useMMKVBoolean(IS_FIRST_TIME, storage); + const [isFirstTime, setIsFirstTime] = useMMKVBoolean(IS_FIRST_TIME, storage); if (isFirstTime === undefined || isFirstTime === null || isFirstTime === true) { return [true, setIsFirstTime] as const; }If you adopt the in-memory shim, this is fine. Otherwise, you must gate rendering of components using this hook until storage is initialized.
1-38: Optional: export an explicit readiness helper for feature modulesConsider exporting
ensureStorageReady(): Promise<void>so modules can await readiness during their own async flows (e.g., initial data loads), even though the shim prevents crashes:// near initializeStorage export const ensureStorageReady = async () => { // no-op if real storage already swapped in (e.g., by tracking a symbol/brand) // or run initializeStorage again (idempotent) };I can wire this up if desired.
src/lib/storage/units-filter.ts (2)
2-2: Guard against uninitialized storage or import the typed helpersDirectly using
storage.getStringrelies onstoragebeing initialized. Givenindex.tsxnow initializes asynchronously, either:
- Await readiness at the start of each async function; or
- Use the exported typed helpers
getItem/setItem/removeItemwhich can centralize readiness and JSON handling.Example change:
-import { storage } from './index'; +import { storage, getItem, setItem } from './index';And then use
getItem<string[]>('UNITS_FILTER_OPTIONS')andsetItem('UNITS_FILTER_OPTIONS', options).
30-42: Save path can be centralized and validatedConsider using
setItemto centralize JSON/stringify and future-proof storage migrations:- storage.set(UNITS_FILTER_OPTIONS, JSON.stringify(options)); + // Optionally validate `options` is an array of strings here + storage.set(UNITS_FILTER_OPTIONS, JSON.stringify(options));Or:
- storage.set(UNITS_FILTER_OPTIONS, JSON.stringify(options)); + setItem(UNITS_FILTER_OPTIONS, options);src/lib/storage/realtime-geolocation.ts (2)
2-2: Same init concern: avoid directstorageaccess before readyAs with units filter, please ensure storage readiness or switch to helpers from
index.tsx.
30-32: Wrap save in try/catch for parity and resilienceOther storage modules log on failure; mirror that behavior here:
-export const saveRealtimeGeolocationState = (enabled: boolean): void => { - storage.set(REALTIME_GEOLOCATION_ENABLED, enabled); -}; +export const saveRealtimeGeolocationState = (enabled: boolean): void => { + try { + storage.set(REALTIME_GEOLOCATION_ENABLED, enabled); + } catch (error) { + logger.error({ + message: 'Failed to save realtime geolocation state', + context: { error, enabled }, + }); + } +};src/lib/storage/personnel-filter.ts (2)
2-2: Same note: centralize access or ensure readinessDirect
storageaccess assumes it’s initialized. PrefergetItem/setItem/removeItemor await readiness in async functions.
32-44: Consider typed helpers and input validationLeverage centralized helpers and validate
optionsbeing a string array before persisting to prevent corrupt data:- storage.set(PERSONNEL_FILTER_OPTIONS, JSON.stringify(options)); + // assert(Array.isArray(options) && options.every(o => typeof o === 'string')); + storage.set(PERSONNEL_FILTER_OPTIONS, JSON.stringify(options));src/lib/storage/background-geolocation.ts (5)
2-2: Confirm shared storage import path and file extension mismatch in comments.Importing from './index' looks right, but the inline comments below reference index.tsx. If the shared storage actually lives in index.ts, align the comments to avoid confusion. If it is index.tsx, consider importing from './index.tsx' explicitly if your tooling doesn’t resolve it automatically.
6-7: Remove misleading “Use secure storage” note (this module uses MMKV).This setting (enable/disable flag) isn’t sensitive and is stored via MMKV through the shared storage. The comment suggests SecureStore; that’s misleading here. Either remove the comment or clarify which data goes to SecureStore vs MMKV in a central place.
-// Use secure storage +// Persist non-sensitive background geolocation preference via shared MMKV storage
8-11: Duplicate and noisy comments; tighten to a single, accurate line.Two identical “Using shared storage instance…” comments add noise and reference index.tsx specifically. Keep one line and keep it extension-agnostic.
-// Using shared storage instance from index.tsx - -// Using shared storage instance from index.tsx +// Uses the shared storage instance from ./index
36-38: Add write error handling and return a status to callers.Persist failures are rare but silent here. Wrap in try/catch, log, and return a boolean so callers can react (retry, show toast, etc.). If you prefer not to change the signature, still add try/catch and logging.
-export const saveBackgroundGeolocationState = (enabled: boolean): void => { - storage.set(BACKGROUND_GEOLOCATION_ENABLED, enabled); -}; +export const saveBackgroundGeolocationState = (enabled: boolean): boolean => { + try { + storage.set(BACKGROUND_GEOLOCATION_ENABLED, enabled); + logger.info({ + message: 'Background geolocation state saved', + context: { enabled }, + }); + return true; + } catch (error) { + logger.error({ + message: 'Failed to save background geolocation state', + context: { error }, + }); + return false; + } +};
16-31: KeeploadBackgroundGeolocationStateasync; optional logging refactorBased on the call‐site analysis, this function’s promise signature must remain intact:
• src/services/location.ts:158
this.isBackgroundGeolocationEnabled = await loadBackgroundGeolocationState();• src/app/_layout.tsx:132
loadBackgroundGeolocationState() .then(() => { … });Removing the
async/Promise would break both theawaitand.then(...)usage. Since the overhead of anasyncfunction here is negligible, I recommend keeping it as-is, but improving consistency in your logs by ensuring you never logundefined:export const loadBackgroundGeolocationState = async (): Promise<boolean> => { try { - const backgroundGeolocationEnabled = storage.getBoolean(BACKGROUND_GEOLOCATION_ENABLED); - logger.info({ - message: 'Background geolocation state loaded on startup', - context: { enabled: backgroundGeolocationEnabled }, - }); - return backgroundGeolocationEnabled ?? false; + const raw = storage.getBoolean(BACKGROUND_GEOLOCATION_ENABLED); + const enabled = raw ?? false; + logger.info({ + message: 'Background geolocation state loaded on startup', + context: { enabled }, + }); + return enabled; } catch (error) { logger.error({ message: 'Failed to load background geolocation state on startup', context: { error }, }); return false; } };This change aligns the logged value with the returned value and avoids any ambiguity between
undefinedandfalse.
📜 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 (19)
__mocks__/expo-audio.ts(1 hunks)__mocks__/react-native-ble-manager.ts(1 hunks)jest-setup.ts(2 hunks)src/app/(app)/home/personnel.tsx(2 hunks)src/components/units/__tests__/units-filter-sheet-basic.test.tsx(1 hunks)src/lib/storage/background-geolocation.ts(1 hunks)src/lib/storage/index.tsx(1 hunks)src/lib/storage/personnel-filter.ts(1 hunks)src/lib/storage/realtime-geolocation.ts(1 hunks)src/lib/storage/secure-storage.ts(2 hunks)src/lib/storage/units-filter.ts(1 hunks)src/services/__tests__/audio.service.test.ts(1 hunks)src/services/__tests__/location.test.ts(1 hunks)src/services/__tests__/offline-queue-processor.test.ts(1 hunks)src/services/bluetooth-audio.service.ts(4 hunks)src/services/location.ts(5 hunks)src/services/offline-queue-processor.ts(1 hunks)src/services/push-notification.ts(6 hunks)src/stores/app/core-store.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- src/stores/app/core-store.ts
- src/services/bluetooth-audio.service.ts
- mocks/expo-audio.ts
- src/services/tests/location.test.ts
- mocks/react-native-ble-manager.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
Use TypeScript for all components and favor interfaces for props and state
Avoid using any; use precise types
Use React Navigation for navigation and deep linking following best practices
Handle errors gracefully and provide user feedback
Implement proper offline support (caching, queueing, retries)
Use Expo SecureStore for sensitive data storage
Use zustand for state management
Use react-hook-form for form handling
Use react-query for data fetching and caching
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
Ensure support for dark mode and light mode
Handle errors gracefully and provide user feedback
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/secure-storage.tssrc/services/__tests__/audio.service.test.tssrc/lib/storage/realtime-geolocation.tssrc/lib/storage/units-filter.tssrc/components/units/__tests__/units-filter-sheet-basic.test.tsxsrc/lib/storage/background-geolocation.tssrc/lib/storage/index.tsxsrc/lib/storage/personnel-filter.tssrc/services/location.tssrc/app/(app)/home/personnel.tsxsrc/services/__tests__/offline-queue-processor.test.tssrc/services/offline-queue-processor.tsjest-setup.tssrc/services/push-notification.ts
src/**
📄 CodeRabbit inference engine (.cursorrules)
src/**: Organize files by feature, grouping related components, hooks, and styles
Directory and file names should be lowercase and hyphenated (e.g., user-profile)
Files:
src/lib/storage/secure-storage.tssrc/services/__tests__/audio.service.test.tssrc/lib/storage/realtime-geolocation.tssrc/lib/storage/units-filter.tssrc/components/units/__tests__/units-filter-sheet-basic.test.tsxsrc/lib/storage/background-geolocation.tssrc/lib/storage/index.tsxsrc/lib/storage/personnel-filter.tssrc/services/location.tssrc/app/(app)/home/personnel.tsxsrc/services/__tests__/offline-queue-processor.test.tssrc/services/offline-queue-processor.tssrc/services/push-notification.ts
**/*.{test,spec}.{ts,tsx}
📄 CodeRabbit inference engine (.cursorrules)
**/*.{test,spec}.{ts,tsx}: Create Jest tests for all generated components, services, and logic
Ensure tests run without errors and fix failing tests
Files:
src/services/__tests__/audio.service.test.tssrc/components/units/__tests__/units-filter-sheet-basic.test.tsxsrc/services/__tests__/offline-queue-processor.test.ts
src/**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
src/**/*.test.{ts,tsx}: Create and use Jest tests to validate all generated components
Generate tests for all components, services, and logic; ensure tests run without errors
Files:
src/services/__tests__/audio.service.test.tssrc/components/units/__tests__/units-filter-sheet-basic.test.tsxsrc/services/__tests__/offline-queue-processor.test.ts
**/*.tsx
📄 CodeRabbit inference engine (.cursorrules)
**/*.tsx: Use functional components and React hooks instead of class components
Use PascalCase for React component names
Use React.FC for defining functional components with props
Minimize useEffect/useState usage and avoid heavy computations during render
Use React.memo for components with static props to prevent unnecessary re-renders
Optimize FlatList with removeClippedSubviews, maxToRenderPerBatch, and windowSize
Provide getItemLayout to FlatList when items have consistent size
Avoid anonymous functions in renderItem or event handlers; define callbacks with useCallback or outside render
Use gluestack-ui for styling where available from components/ui; otherwise, style via StyleSheet.create or styled-components
Ensure responsive design across screen sizes and orientations
Use react-native-fast-image for image handling instead of the default Image where appropriate
Wrap all user-facing text in t() from react-i18next for translations
Support dark mode and light mode in UI components
Use @rnmapbox/maps for maps or navigation features
Use lucide-react-native for icons directly; do not use the gluestack-ui icon component
Use conditional rendering with the ternary operator (?:) instead of &&
**/*.tsx: Use functional components and hooks over class components
Ensure components are modular, reusable, and maintainable
Ensure all components are mobile-friendly, responsive, and support both iOS and Android
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 to prevent re-renders
Ensure responsive design for different screen sizes and orientations
Optimize image handling using rea...
Files:
src/components/units/__tests__/units-filter-sheet-basic.test.tsxsrc/lib/storage/index.tsxsrc/app/(app)/home/personnel.tsx
🧠 Learnings (10)
📚 Learning: 2025-08-21T02:47:24.070Z
Learnt from: CR
PR: Resgrid/Responder#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-21T02:47:24.070Z
Learning: Applies to **/*.{ts,tsx} : Use react-native-mmkv for local storage
Applied to files:
src/lib/storage/realtime-geolocation.tssrc/lib/storage/units-filter.tssrc/lib/storage/background-geolocation.tssrc/lib/storage/index.tsxsrc/lib/storage/personnel-filter.ts
📚 Learning: 2025-08-21T02:46:42.705Z
Learnt from: CR
PR: Resgrid/Responder#0
File: .cursorrules:0-0
Timestamp: 2025-08-21T02:46:42.705Z
Learning: Applies to **/*.{ts,tsx} : Use Expo SecureStore for sensitive data storage
Applied to files:
src/lib/storage/realtime-geolocation.tssrc/lib/storage/units-filter.tssrc/lib/storage/background-geolocation.tssrc/lib/storage/index.tsxsrc/lib/storage/personnel-filter.ts
📚 Learning: 2025-08-21T02:47:24.070Z
Learnt from: CR
PR: Resgrid/Responder#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-21T02:47:24.070Z
Learning: Use Expo's SecureStore for sensitive data
Applied to files:
src/lib/storage/realtime-geolocation.tssrc/lib/storage/background-geolocation.tssrc/lib/storage/index.tsxsrc/lib/storage/personnel-filter.ts
📚 Learning: 2025-08-21T02:47:24.070Z
Learnt from: CR
PR: Resgrid/Responder#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-21T02:47:24.070Z
Learning: Applies to src/**/*.test.{ts,tsx} : Create and use Jest tests to validate all generated components
Applied to files:
src/components/units/__tests__/units-filter-sheet-basic.test.tsx
📚 Learning: 2025-08-21T02:46:42.705Z
Learnt from: CR
PR: Resgrid/Responder#0
File: .cursorrules:0-0
Timestamp: 2025-08-21T02:46:42.705Z
Learning: Applies to **/*.{test,spec}.{ts,tsx} : Create Jest tests for all generated components, services, and logic
Applied to files:
src/components/units/__tests__/units-filter-sheet-basic.test.tsx
📚 Learning: 2025-08-21T02:46:42.705Z
Learnt from: CR
PR: Resgrid/Responder#0
File: .cursorrules:0-0
Timestamp: 2025-08-21T02:46:42.705Z
Learning: Applies to **/*.tsx : Use lucide-react-native for icons directly; do not use the gluestack-ui icon component
Applied to files:
src/app/(app)/home/personnel.tsx
📚 Learning: 2025-08-21T02:47:24.070Z
Learnt from: CR
PR: Resgrid/Responder#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-21T02:47:24.070Z
Learning: Applies to **/*.tsx : Use lucide-react-native for icons directly in markup; do not use gluestack-ui icon component
Applied to files:
src/app/(app)/home/personnel.tsx
📚 Learning: 2025-08-21T02:46:42.705Z
Learnt from: CR
PR: Resgrid/Responder#0
File: .cursorrules:0-0
Timestamp: 2025-08-21T02:46:42.705Z
Learning: Applies to **/*.{ts,tsx} : Use React Navigation for navigation and deep linking following best practices
Applied to files:
src/app/(app)/home/personnel.tsx
📚 Learning: 2025-08-21T02:46:42.705Z
Learnt from: CR
PR: Resgrid/Responder#0
File: .cursorrules:0-0
Timestamp: 2025-08-21T02:46:42.705Z
Learning: Applies to **/*.tsx : Use rnmapbox/maps for maps or navigation features
Applied to files:
src/app/(app)/home/personnel.tsx
📚 Learning: 2025-08-21T02:46:42.705Z
Learnt from: CR
PR: Resgrid/Responder#0
File: .cursorrules:0-0
Timestamp: 2025-08-21T02:46:42.705Z
Learning: Applies to **/*.{ts,tsx} : Implement proper offline support (caching, queueing, retries)
Applied to files:
src/services/offline-queue-processor.ts
🧬 Code graph analysis (5)
src/lib/storage/index.tsx (1)
src/lib/storage/secure-storage.ts (1)
getGeneralStorage(227-232)
src/services/location.ts (1)
src/stores/app/core-store.ts (1)
useCoreStore(48-222)
src/services/__tests__/offline-queue-processor.test.ts (1)
src/services/offline-queue-processor.ts (2)
offlineQueueProcessor(139-139)OfflineQueueProcessor(127-137)
src/services/offline-queue-processor.ts (4)
__mocks__/@react-native-community/netinfo.js (1)
NetInfo(3-10)src/lib/storage/index.tsx (1)
storage(8-8)src/lib/storage/secure-storage.ts (1)
getOfflineQueueStorage(234-239)src/api/personnel/personnelStatuses.ts (1)
savePersonnelStatus(19-28)
src/services/push-notification.ts (1)
src/api/devices/push.ts (1)
registerDevice(17-22)
⏰ 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). (2)
- GitHub Check: test
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (12)
src/components/units/__tests__/units-filter-sheet-basic.test.tsx (2)
8-10: LGTM: analytics mock is minimal and effectiveSimple, side-effect-free stub of useAnalytics with jest.fn is appropriate for an importability smoke test.
11-20: Mock shape and import path validation – no action requiredThe
useUnitsStoremock insrc/components/units/__tests__/units-filter-sheet-basic.test.tsxmatches the exact fields destructured byUnitsFilterSheet(filterOptions,selectedFilters,isFilterSheetOpen,isLoadingFilters,closeFilterSheet,toggleFilter) and uses the correct import path (@/stores/units/store). No additional selectors are used in the component, so the test remains stable as written.src/app/(app)/home/personnel.tsx (1)
29-35: Analytics on mount instead of on focus — confirm the product intent to avoid under/over-countingSwitching from useFocusEffect to a mount-only useEffect changes event semantics. Users revisiting the screen without unmounting won’t trigger another personnel_viewed. If the metric should count each focus/visit, keep useFocusEffect or manually subscribe to focus events.
If the mount-only behavior is intended, all good. Otherwise, I can refactor to react-navigation focus events with a stable callback.
jest-setup.ts (1)
34-36: LGTM: Centralized expo-audio mockGood move delegating to mocks/expo-audio.ts; this keeps audio-related tests consistent.
src/services/push-notification.ts (3)
93-99: Good call registering listeners before channel setupRegistering listeners first reduces the risk of missing early events on cold start. Channel setup remains awaited so initialization logs won’t race ahead. LGTM.
189-191: LGTM on logging context switch to userIdSwitching from unit to user in the logging context fits the new contract.
145-145: No outstanding call sites or documentation for registerForPushNotificationsI confirmed that the only invocations of
registerForPushNotificationsreside withinsrc/services/push-notification.ts(declaration at line 145 and call at line 271) and there are no Markdown docs referencing it. All externalunitIdusages belong to unrelated APIs. No further updates are needed.src/lib/storage/index.tsx (1)
10-21: Good: test-only in-memory storage to stabilize unit testsThe test fallback using a simple Map-backed MMKV-like object is a sensible approach to keep tests deterministic.
src/lib/storage/realtime-geolocation.ts (1)
10-18: Verify storage readiness before useI don’t see any existing “ensureStorageReady” (or equivalent) helper in src/lib/storage, and without it calling storage.getBoolean immediately on startup could potentially throw if the underlying storage hasn’t been initialized. Please:
• Confirm how
storageis instantiated in src/lib/storage/index.ts (or wherever it lives) and whether getBoolean is safe to call synchronously on app launch.
• If initialization is asynchronous, add (or implement) a storage-ready check—e.g. anensureStorageReady()that youawaitbefore anystorage.get*calls.Once you’ve verified storage’s readiness guarantees (or added the helper), you can safely default to
falsewith the existing nullish coalescing.src/lib/storage/personnel-filter.ts (1)
11-20: Ignore guard pattern suggestion: noensureStorageReady()exists
- There’s no exported
ensureStorageReady()(or equivalent) in the storage modules—initialization is kicked off insrc/lib/storage/index.tsxvia the unexportedinitializeStorage().- Other filter loaders (e.g.
loadUnitsFilterOptions) use the same direct-call pattern, so adding a non-existent guard here would be inconsistent.If storage readiness truly needs enforcing, a shared helper must first be defined and exported. Otherwise, you can disregard this suggested change.
Likely an incorrect or invalid review comment.
src/lib/storage/background-geolocation.ts (2)
43-45: LGTM: explicit accessor for the storage key.Clear, testable, and avoids key duplication elsewhere.
1-45: ✔ No circular import detectedAfter inspection,
src/lib/storage/index.tsxdoes not import frombackground-geolocation.ts, so theimport { storage } from './index'inbackground-geolocation.tsdoes not introduce a cycle.
| jest.mock('react-native', () => ({ | ||
| SectionList: () => null, | ||
| })); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Don’t override the entire react-native module; merge with the real mock instead
Fully replacing react-native risks breaking imports like Platform, StyleSheet, View, etc., and can mask regressions. Extend the preset’s real mock and only override SectionList.
Apply this diff:
-jest.mock('react-native', () => ({
- SectionList: () => null,
-}));
+jest.mock('react-native', () => {
+ const RN = jest.requireActual('react-native');
+ return { ...RN, SectionList: () => null };
+});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| jest.mock('react-native', () => ({ | |
| SectionList: () => null, | |
| })); | |
| jest.mock('react-native', () => { | |
| const RN = jest.requireActual('react-native'); | |
| return { ...RN, SectionList: () => null }; | |
| }); |
🤖 Prompt for AI Agents
In src/components/units/__tests__/units-filter-sheet-basic.test.tsx around lines
24 to 26, the test currently fully replaces the react-native module which can
break other imports; change the jest.mock to merge with the real react-native
mock by calling jest.requireActual('react-native') (or jest.requireMock as
appropriate) and spread its properties, then override only SectionList to return
null so Platform, StyleSheet, View, etc. remain intact.
| // Initialize storage asynchronously | ||
| let storageInitialized = false; | ||
| const initializeStorage = async () => { | ||
| if (storageInitialized) return; | ||
| storageInitialized = true; | ||
| try { | ||
| storage = await getGeneralStorage(); | ||
| } catch (error) { | ||
| storageInitialized = false; | ||
| throw error; | ||
| } | ||
| }; | ||
| // Initialize secure storage | ||
| initializeStorage().catch((error) => { | ||
| console.error('Failed to initialize secure storage:', error); | ||
| }); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Race: storage can be undefined when consumers call it; make it always non-null synchronously
Outside tests, storage remains uninitialized until the async initializeStorage() completes. Call sites (e.g., useIsFirstTime, zustandStorage, filters) access storage synchronously and can crash.
Make storage point to an in-memory shim immediately, then atomically swap in the secure MMKV once ready. Optionally migrate in-memory keys into the real store.
Proposed refactor:
export let storage: MMKV;
-// In test environment, provide an in-memory storage fallback so tests don't break before async init
-if (process.env.NODE_ENV === 'test') {
- const memoryMap = new Map<string, string>();
- storage = {
- getString: (key: string) => memoryMap.get(key) as string | undefined,
- set: (key: string, value: string) => {
- memoryMap.set(key, value);
- },
- delete: (key: string) => {
- memoryMap.delete(key);
- },
- } as unknown as MMKV;
-} else {
+// In-memory shim used in tests and as a startup fallback in non-test
+const createMemoryStorage = () => {
+ const memoryMap = new Map<string, string>();
+ return {
+ getString: (key: string) => memoryMap.get(key) as string | undefined,
+ set: (key: string, value: string) => {
+ memoryMap.set(key, value);
+ },
+ delete: (key: string) => {
+ memoryMap.delete(key);
+ },
+ // Optional: minimally support getAllKeys if you plan to migrate
+ getAllKeys: () => Array.from(memoryMap.keys()),
+ } as unknown as MMKV;
+};
+
+if (process.env.NODE_ENV === 'test') {
+ storage = createMemoryStorage();
+} else {
+ // Provide a synchronous, non-null default immediately
+ storage = createMemoryStorage();
// Initialize storage asynchronously
let storageInitialized = false;
const initializeStorage = async () => {
if (storageInitialized) return;
storageInitialized = true;
try {
- storage = await getGeneralStorage();
+ const real = await getGeneralStorage();
+ // Optional migration from in-memory shim -> real store
+ if ((storage as any).getAllKeys) {
+ for (const k of (storage as any).getAllKeys()) {
+ const v = storage.getString(k);
+ if (v !== undefined) real.set(k, v);
+ }
+ }
+ storage = real;
} catch (error) {
storageInitialized = false;
throw error;
}
};
// Initialize secure storage
initializeStorage().catch((error) => {
console.error('Failed to initialize secure storage:', error);
});
}This guarantees storage is callable at all times and prevents crashes in hooks, zustand, and feature modules.
📝 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.
| // Initialize storage asynchronously | |
| let storageInitialized = false; | |
| const initializeStorage = async () => { | |
| if (storageInitialized) return; | |
| storageInitialized = true; | |
| try { | |
| storage = await getGeneralStorage(); | |
| } catch (error) { | |
| storageInitialized = false; | |
| throw error; | |
| } | |
| }; | |
| // Initialize secure storage | |
| initializeStorage().catch((error) => { | |
| console.error('Failed to initialize secure storage:', error); | |
| }); | |
| } | |
| export let storage: MMKV; | |
| // In-memory shim used in tests and as a startup fallback in non-test | |
| const createMemoryStorage = () => { | |
| const memoryMap = new Map<string, string>(); | |
| return { | |
| getString: (key: string) => memoryMap.get(key) as string | undefined, | |
| set: (key: string, value: string) => { | |
| memoryMap.set(key, value); | |
| }, | |
| delete: (key: string) => { | |
| memoryMap.delete(key); | |
| }, | |
| // Optional: minimally support getAllKeys if you plan to migrate | |
| getAllKeys: () => Array.from(memoryMap.keys()), | |
| } as unknown as MMKV; | |
| }; | |
| if (process.env.NODE_ENV === 'test') { | |
| storage = createMemoryStorage(); | |
| } else { | |
| // Provide a synchronous, non-null default immediately | |
| storage = createMemoryStorage(); | |
| // Initialize storage asynchronously | |
| let storageInitialized = false; | |
| const initializeStorage = async () => { | |
| if (storageInitialized) return; | |
| storageInitialized = true; | |
| try { | |
| const real = await getGeneralStorage(); | |
| // Optional migration from in-memory shim -> real store | |
| if ((storage as any).getAllKeys) { | |
| for (const k of (storage as any).getAllKeys()) { | |
| const v = storage.getString(k); | |
| if (v !== undefined) real.set(k, v); | |
| } | |
| } | |
| storage = real; | |
| } catch (error) { | |
| storageInitialized = false; | |
| throw error; | |
| } | |
| }; | |
| // Initialize secure storage | |
| initializeStorage().catch((error) => { | |
| console.error('Failed to initialize secure storage:', error); | |
| }); | |
| } |
🤖 Prompt for AI Agents
In src/lib/storage/index.tsx around lines 22 to 38, storage can be undefined
until the async initializeStorage completes which allows synchronous callers to
crash; to fix, immediately assign storage to an in-memory shim implementation
that matches the storage API, start initializeStorage() to create the real
secure MMKV store, and when that promise resolves atomically replace the
in-memory shim with the real store (while preserving method references or using
a proxy to avoid racey reads). During the swap, migrate any keys from the
in-memory shim into the real store, keep storageInitialized flags for readiness
only (not for null-checks), and ensure initializeStorage() catches and logs
errors without reverting storage to undefined.
| let storageInitializing = false; | ||
|
|
||
| const initializeStorage = async (): Promise<void> => { | ||
| if (storageInitializing) return; | ||
| storageInitializing = true; | ||
| try { |
There was a problem hiding this comment.
Fix init guard: current boolean guard causes race and use-before-init
initializeStorage() returns early when storageInitializing is true without awaiting the in-flight work. Callers like getGeneralStorage() then proceed to return generalStorage!, which can still be undefined. This is a real race in app startup and tests.
Replace the boolean guard with an in-flight Promise and await it. Also clear the guard in a finally block. Example refactor:
-let storageInitializing = false;
+let storageInitPromise: Promise<void> | null = null;
-const initializeStorage = async (): Promise<void> => {
- if (storageInitializing) return;
- storageInitializing = true;
+const initializeStorage = async (): Promise<void> => {
+ if (storageInitPromise) return storageInitPromise;
+ storageInitPromise = (async () => {
+ try {
+ if (Platform.OS === 'web') {
+ // existing web init...
+ } else {
+ // existing mobile init...
+ }
+ } catch (error) {
+ logger.error({
+ message: 'Failed to initialize secure storage, falling back to basic storage',
+ context: { error },
+ });
+ generalStorage = new MMKV({ id: 'ResgridUnit' });
+ offlineQueueStorage = new MMKV({ id: 'ResgridOfflineQueue' });
+ }
+ })();
+ try {
+ await storageInitPromise;
+ } finally {
+ storageInitPromise = null;
+ }
};
- storageInitializing = false;This ensures all concurrent callers await the same init and never observe an uninitialized generalStorage.
Also applies to: 222-223
| jest.mock('@/lib/storage/secure-storage', () => ({ | ||
| getOfflineQueueStorage: jest.fn().mockResolvedValue({ | ||
| getString: jest.fn().mockReturnValue('[]'), | ||
| set: jest.fn().mockResolvedValue(undefined), | ||
| }), | ||
| })); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Align the storage mock with MMKV’s synchronous API to avoid masking issues
react-native-mmkv’s set is synchronous; returning a Promise here can hide await/no-await mistakes in production code.
Apply this diff to better mirror MMKV:
jest.mock('@/lib/storage/secure-storage', () => ({
getOfflineQueueStorage: jest.fn().mockResolvedValue({
- getString: jest.fn().mockReturnValue('[]'),
- set: jest.fn().mockResolvedValue(undefined),
+ getString: jest.fn(() => '[]'),
+ set: jest.fn(), // sync like MMKV
}),
}));📝 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.mock('@/lib/storage/secure-storage', () => ({ | |
| getOfflineQueueStorage: jest.fn().mockResolvedValue({ | |
| getString: jest.fn().mockReturnValue('[]'), | |
| set: jest.fn().mockResolvedValue(undefined), | |
| }), | |
| })); | |
| jest.mock('@/lib/storage/secure-storage', () => ({ | |
| getOfflineQueueStorage: jest.fn().mockResolvedValue({ | |
| getString: jest.fn(() => '[]'), | |
| set: jest.fn(), // sync like MMKV | |
| }), | |
| })); |
🤖 Prompt for AI Agents
In src/services/__tests__/offline-queue-processor.test.ts around lines 7-12, the
test mocks storage.set as asynchronous (mockResolvedValue) which misrepresents
react-native-mmkv’s synchronous API; update the storage mock so set is
synchronous (e.g., use jest.fn().mockReturnValue(undefined) or
jest.fn().mockImplementation(() => undefined)) while keeping
getOfflineQueueStorage as mockResolvedValue if that factory is async, so tests
mirror production behavior and avoid hiding await/no-await bugs.
| const unitId = useCoreStore.getState().activeUnitId; | ||
| if (!unitId) { | ||
| logger.warn({ message: 'No active unit selected, skipping location API call' }); | ||
| return; | ||
| } | ||
|
|
||
| const locationInput = new SavePersonnelLocationInput(); | ||
| locationInput.UserId = userId; | ||
| const locationInput = new SaveUnitLocationInput(); | ||
| locationInput.UnitId = unitId; | ||
| locationInput.Timestamp = new Date(location.timestamp).toISOString(); | ||
| locationInput.Latitude = location.coords.latitude.toString(); | ||
| locationInput.Longitude = location.coords.longitude.toString(); | ||
| locationInput.Accuracy = location.coords.accuracy?.toString() || '0'; | ||
| locationInput.Altitude = location.coords.altitude?.toString() || '0'; | ||
| locationInput.AltitudeAccuracy = location.coords.altitudeAccuracy?.toString() || '0'; | ||
| locationInput.Speed = location.coords.speed?.toString() || '0'; | ||
| locationInput.Heading = location.coords.heading?.toString() || '0'; | ||
|
|
||
| const result = await setPersonLocation(locationInput); | ||
|
|
||
| locationInput.Accuracy = safeNumericString(location.coords.accuracy, 'accuracy'); | ||
| locationInput.Altitude = safeNumericString(location.coords.altitude, 'altitude'); | ||
| locationInput.AltitudeAccuracy = safeNumericString(location.coords.altitudeAccuracy, 'altitudeAccuracy'); | ||
| locationInput.Speed = safeNumericString(location.coords.speed, 'speed'); | ||
| locationInput.Heading = safeNumericString(location.coords.heading, 'heading'); | ||
| const result = await setUnitLocation(locationInput); |
There was a problem hiding this comment.
Latitude/Longitude bypass safeNumericString; potential 'NaN'/'Infinity' payloads
Latitude/Longitude are toString()’d directly; if they’re NaN/Infinity you’ll send invalid strings to the API. Apply the same guard you added for the other fields.
Apply:
- locationInput.Latitude = location.coords.latitude.toString();
- locationInput.Longitude = location.coords.longitude.toString();
+ locationInput.Latitude = safeNumericString(location.coords.latitude, 'latitude');
+ locationInput.Longitude = safeNumericString(location.coords.longitude, 'longitude');📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const unitId = useCoreStore.getState().activeUnitId; | |
| if (!unitId) { | |
| logger.warn({ message: 'No active unit selected, skipping location API call' }); | |
| return; | |
| } | |
| const locationInput = new SavePersonnelLocationInput(); | |
| locationInput.UserId = userId; | |
| const locationInput = new SaveUnitLocationInput(); | |
| locationInput.UnitId = unitId; | |
| locationInput.Timestamp = new Date(location.timestamp).toISOString(); | |
| locationInput.Latitude = location.coords.latitude.toString(); | |
| locationInput.Longitude = location.coords.longitude.toString(); | |
| locationInput.Accuracy = location.coords.accuracy?.toString() || '0'; | |
| locationInput.Altitude = location.coords.altitude?.toString() || '0'; | |
| locationInput.AltitudeAccuracy = location.coords.altitudeAccuracy?.toString() || '0'; | |
| locationInput.Speed = location.coords.speed?.toString() || '0'; | |
| locationInput.Heading = location.coords.heading?.toString() || '0'; | |
| const result = await setPersonLocation(locationInput); | |
| locationInput.Accuracy = safeNumericString(location.coords.accuracy, 'accuracy'); | |
| locationInput.Altitude = safeNumericString(location.coords.altitude, 'altitude'); | |
| locationInput.AltitudeAccuracy = safeNumericString(location.coords.altitudeAccuracy, 'altitudeAccuracy'); | |
| locationInput.Speed = safeNumericString(location.coords.speed, 'speed'); | |
| locationInput.Heading = safeNumericString(location.coords.heading, 'heading'); | |
| const result = await setUnitLocation(locationInput); | |
| const unitId = useCoreStore.getState().activeUnitId; | |
| if (!unitId) { | |
| logger.warn({ message: 'No active unit selected, skipping location API call' }); | |
| return; | |
| } | |
| const locationInput = new SaveUnitLocationInput(); | |
| locationInput.UnitId = unitId; | |
| locationInput.Timestamp = new Date(location.timestamp).toISOString(); | |
| locationInput.Latitude = safeNumericString(location.coords.latitude, 'latitude'); | |
| locationInput.Longitude = safeNumericString(location.coords.longitude, 'longitude'); | |
| locationInput.Accuracy = safeNumericString(location.coords.accuracy, 'accuracy'); | |
| locationInput.Altitude = safeNumericString(location.coords.altitude, 'altitude'); | |
| locationInput.AltitudeAccuracy = safeNumericString(location.coords.altitudeAccuracy, 'altitudeAccuracy'); | |
| locationInput.Speed = safeNumericString(location.coords.speed, 'speed'); | |
| locationInput.Heading = safeNumericString(location.coords.heading, 'heading'); | |
| const result = await setUnitLocation(locationInput); |
🤖 Prompt for AI Agents
In src/services/location.ts around lines 31 to 46, Latitude and Longitude are
being sent as location.coords.latitude.toString() and
location.coords.longitude.toString(), which can produce 'NaN'/'Infinity'
payloads; replace those direct toString() calls with the same safeNumericString
wrapper used for other numeric fields (e.g.,
safeNumericString(location.coords.latitude, 'latitude') and
safeNumericString(location.coords.longitude, 'longitude')) so invalid numeric
values are normalized before sending to the API.
| NetInfo.addEventListener((state) => { | ||
| if (state.isInternetReachable) { | ||
| this.processQueue(); | ||
| } | ||
| }); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Leak: store NetInfo unsubscribe and release it in cleanup()
The listener is never removed. Even with a singleton, we should release the subscription during app teardown.
Apply this diff:
class RealOfflineQueueProcessor {
private static instance: RealOfflineQueueProcessor | null = null;
private processing = false;
private storageKey = 'offline_queue';
+ private unsubscribe?: () => void;
private constructor() {
- NetInfo.addEventListener((state) => {
+ this.unsubscribe = NetInfo.addEventListener((state) => {
if (state.isInternetReachable) {
- this.processQueue();
+ void this.processQueue();
}
});
}
@@
cleanup(): void {
- // no-op
+ if (this.unsubscribe) {
+ this.unsubscribe();
+ this.unsubscribe = undefined;
+ }
}Also applies to: 78-80
🤖 Prompt for AI Agents
In src/services/offline-queue-processor.ts around lines 21-26 (and also apply
the same change for lines 78-80), the NetInfo.addEventListener subscription is
not stored or removed, causing a leak; update the class to store the
subscription returned by NetInfo.addEventListener (e.g., this.netInfoUnsubscribe
or this.netInfoSubscription) when creating the listener and then call and clear
that stored unsubscribe function inside cleanup() (or equivalent teardown) so
the listener is removed during app shutdown; apply the same pattern for the
other listener at lines 78-80.
| async processQueue(): Promise<void> { | ||
| if (this.processing) return; | ||
| this.processing = true; | ||
| try { | ||
| // Task is already defined at module level, just verify it's registered | ||
| const isTaskRegistered = await TaskManager.isTaskRegisteredAsync(QUEUE_PROCESSOR_TASK_NAME); | ||
| if (!isTaskRegistered) { | ||
| logger.warn({ | ||
| message: 'Background task not registered, this should not happen as it is defined at module level', | ||
| }); | ||
| } else { | ||
| logger.info({ | ||
| message: 'Background queue processor task is registered and ready', | ||
| }); | ||
| } | ||
|
|
||
| // Stop foreground processing | ||
| if (this.processingInterval) { | ||
| clearInterval(this.processingInterval); | ||
| this.processingInterval = null; | ||
| const storage = await getOfflineQueueStorage(); | ||
| const raw = storage.getString(this.storageKey); | ||
| const items: QueueItem[] = raw ? JSON.parse(raw) : []; | ||
| const remaining: QueueItem[] = []; | ||
| for (const item of items) { | ||
| try { | ||
| if (item.type === 'personnelStatus') { | ||
| await savePersonnelStatus(item.payload); | ||
| } | ||
| } catch (error) { | ||
| item.retries++; | ||
| const backoff = Math.min(2 ** item.retries * 1000, 30000); | ||
| await new Promise((resolve) => setTimeout(resolve, backoff)); | ||
| remaining.push(item); | ||
| logger.warn({ message: 'Retrying offline queue item', context: { id: item.id, error } }); | ||
| } | ||
| } | ||
|
|
||
| logger.info({ | ||
| message: 'Started background queue processing', | ||
| }); | ||
| await storage.set(this.storageKey, JSON.stringify(remaining)); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Data-loss race: concurrent enqueue vs. processQueue overwrites new items
processQueue() reads the queue once and later writes remaining back. Any items enqueued during processing are lost because the final set() overwrites them.
Merge newly enqueued items before writing, keyed by id:
async processQueue(): Promise<void> {
if (this.processing) return;
this.processing = true;
try {
const storage = await getOfflineQueueStorage();
- const raw = storage.getString(this.storageKey);
- const items: QueueItem[] = raw ? JSON.parse(raw) : [];
+ const raw = storage.getString(this.storageKey);
+ const items: QueueItem[] = raw ? JSON.parse(raw) : [];
+ const seen = new Set(items.map(i => i.id));
const remaining: QueueItem[] = [];
for (const item of items) {
try {
if (item.type === 'personnelStatus') {
await savePersonnelStatus(item.payload);
}
} catch (error) {
item.retries++;
const backoff = Math.min(2 ** item.retries * 1000, 30000);
await new Promise((resolve) => setTimeout(resolve, backoff));
remaining.push(item);
logger.warn({ message: 'Retrying offline queue item', context: { id: item.id, error } });
}
}
- await storage.set(this.storageKey, JSON.stringify(remaining));
+ // Merge in items enqueued while we were processing to avoid overwriting them
+ const latestRaw = storage.getString(this.storageKey);
+ const latestItems: QueueItem[] = latestRaw ? JSON.parse(latestRaw) : [];
+ const newItems = latestItems.filter(i => !seen.has(i.id));
+ const merged = [...newItems, ...remaining];
+ storage.set(this.storageKey, JSON.stringify(merged));Follow-up:
- Add a unit test that enqueues during processing and verifies items are not lost. I can draft it if useful.
Also applies to: 70-76
| const storage = await getOfflineQueueStorage(); | ||
| const raw = storage.getString(this.storageKey); | ||
| const items: QueueItem[] = raw ? JSON.parse(raw) : []; | ||
| const remaining: QueueItem[] = []; | ||
| for (const item of items) { | ||
| try { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Harden JSON parsing; corrupted storage currently aborts processing
A malformed JSON string will throw and halt processing. Fail-safe to an empty queue and log.
Apply this diff:
- const raw = storage.getString(this.storageKey);
- const items: QueueItem[] = raw ? JSON.parse(raw) : [];
+ const raw = storage.getString(this.storageKey);
+ let items: QueueItem[] = [];
+ if (raw) {
+ try {
+ items = JSON.parse(raw);
+ } catch (e) {
+ logger.warn({ message: 'Offline queue corrupted; resetting', context: { error: e } });
+ items = [];
+ }
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const storage = await getOfflineQueueStorage(); | |
| const raw = storage.getString(this.storageKey); | |
| const items: QueueItem[] = raw ? JSON.parse(raw) : []; | |
| const remaining: QueueItem[] = []; | |
| for (const item of items) { | |
| try { | |
| const storage = await getOfflineQueueStorage(); | |
| const raw = storage.getString(this.storageKey); | |
| let items: QueueItem[] = []; | |
| if (raw) { | |
| try { | |
| items = JSON.parse(raw); | |
| } catch (e) { | |
| logger.warn({ message: 'Offline queue corrupted; resetting', context: { error: e } }); | |
| items = []; | |
| } | |
| } | |
| const remaining: QueueItem[] = []; | |
| for (const item of items) { | |
| try { |
🤖 Prompt for AI Agents
In src/services/offline-queue-processor.ts around lines 39 to 44,
JSON.parse(raw) can throw on corrupted storage which aborts processing; wrap the
parse in a try/catch, and on error log the storageKey and error, set items to an
empty array as a fail-safe, and optionally reset the stored value to '[]' (or
remove the key) to repair corrupted state so processing continues without
throwing.
| const userId = useAuthStore((state) => state.userId); | ||
| const rights = securityStore((state) => state.rights); | ||
| const previousUnitIdRef = useRef<string | null>(null); | ||
| const previousUserIdRef = useRef<string | null>(null); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Missed re-registration scenarios and false “success” caching
The hook only keys re-registration on userId. If DepartmentCode changes for the same user, or if registration fails (returns null), previousUserIdRef still advances and blocks future attempts. Track both identifiers and only update on success.
Apply:
- const previousUserIdRef = useRef<string | null>(null);
+ const previousRegistrationRef = useRef<{ userId: string | null; departmentCode: string | null }>({
+ userId: null,
+ departmentCode: null,
+ });- // Only register if we have an active user ID and it's different from the previous one
- if (rights && userId && userId !== previousUserIdRef.current) {
+ // Only register if we have an active user/department combo different from the previous one
+ if (rights?.DepartmentCode && userId && (
+ userId !== previousRegistrationRef.current.userId ||
+ rights.DepartmentCode !== previousRegistrationRef.current.departmentCode
+ )) {
pushNotificationService
.registerForPushNotifications(userId, rights.DepartmentCode)
.then((token) => {
if (token) {
logger.info({
message: 'Successfully registered for push notifications',
- context: { userId: userId },
+ context: { userId },
});
+ // Advance only on success
+ previousRegistrationRef.current = { userId, departmentCode: rights.DepartmentCode };
}
})- previousUserIdRef.current = userId;
+ // moved into success branch aboveAlso applies to: 268-269, 287-287
🤖 Prompt for AI Agents
In src/services/push-notification.ts around lines 265-287, the hook currently
only tracks previousUserIdRef and advances it even when registration fails or
when DepartmentCode changes, which prevents re-registration and causes false
success caching; change the logic to track both previousUserIdRef and
previousDepartmentCodeRef (or a single combined key) and only assign/update
these refs after a successful registration (i.e., when the registration result
is non-null/valid), and update all related checks and early-returns at lines
~268-269 and ~287 to use the combined identifier or both refs so re-registration
occurs when either userId or DepartmentCode changes and failed attempts don’t
block retries.
|
Approve |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Summary by CodeRabbit
Bug Fixes
Refactor
Chores