diff --git a/app.config.ts b/app.config.ts index 15049635..6d47b26c 100644 --- a/app.config.ts +++ b/app.config.ts @@ -82,7 +82,13 @@ export default ({ config }: ConfigContext): ExpoConfig => ({ foregroundImage: './assets/adaptive-icon.png', backgroundColor: '#2484c4', }, - softwareKeyboardLayoutMode: 'pan', + // 'pan' makes Android scroll the window under the IME on its own, which fights + // react-native-keyboard-controller. Its hooks flip the activity to adjustResize on + // mount and call setDefaultMode() on unmount, restoring whatever this value is — so + // with 'pan' any closing sheet or modal drops the app back into pan mode and inputs + // end up under the keyboard. Edge-to-edge means the OS no longer resizes for us + // either, so 'resize' leaves keyboard avoidance entirely to the library. + softwareKeyboardLayoutMode: 'resize', package: Env.PACKAGE, googleServicesFile: 'google-services.json', // Register the ResgridUnit:// deep-link scheme so OIDC / SAML callbacks are routed back here @@ -139,7 +145,10 @@ export default ({ config }: ConfigContext): ExpoConfig => ({ [ '@rnmapbox/maps', { - RNMapboxMapsVersion: '11.16.2', + // Keep in step with the `mapbox` field of the installed @rnmapbox/maps — the JS + // bindings are generated against a specific native SDK, and pinning an older one + // makes style props the bindings emit (symbolZOffset and friends) trap natively. + RNMapboxMapsVersion: '11.23.1', }, ], [ diff --git a/package.json b/package.json index f17667d9..3956b6f7 100644 --- a/package.json +++ b/package.json @@ -71,7 +71,7 @@ "@notifee/react-native": "9.1.8", "@novu/react-native": "3.11.0", "@react-native-community/netinfo": "12.0.1", - "@rnmapbox/maps": "10.2.10", + "@rnmapbox/maps": "10.3.5", "@semantic-release/git": "10.0.1", "@sentry/react-native": "~8.20.0", "@shopify/flash-list": "2.0.2", diff --git a/patches/@rnmapbox+maps+10.3.5.patch b/patches/@rnmapbox+maps+10.3.5.patch new file mode 100644 index 00000000..eec9fd3e --- /dev/null +++ b/patches/@rnmapbox+maps+10.3.5.patch @@ -0,0 +1,60 @@ +diff --git a/node_modules/@rnmapbox/maps/android/src/main/mapbox-v11-compat/v11/com/rnmapbox/rnmbx/v11compat/Location.kt b/node_modules/@rnmapbox/maps/android/src/main/mapbox-v11-compat/v11/com/rnmapbox/rnmbx/v11compat/Location.kt +index 8cda49e..05cbf87 100644 +--- a/node_modules/@rnmapbox/maps/android/src/main/mapbox-v11-compat/v11/com/rnmapbox/rnmbx/v11compat/Location.kt ++++ b/node_modules/@rnmapbox/maps/android/src/main/mapbox-v11-compat/v11/com/rnmapbox/rnmbx/v11compat/Location.kt +@@ -73,17 +73,50 @@ fun LocationEngine.requestLocationUpdatesV11(callback: LocationEngineCallback, l + } + } + val observer = LocationObserverAdapter(callback) ++ // Publish into `observers` *before* registering with the provider, and pin the provider we ++ // register on. A concurrent `removeLocationUpdates` (same two threads described below) that ++ // lands between the two steps would otherwise never see this observer and would leave it ++ // registered — feeding location updates to a callback the caller already tore down, on a ++ // provider instance the request refresh above may have replaced. Registration itself stays ++ // outside the monitor: no Mapbox SDK call is ever made while holding it. ++ val provider = locationProvider ++ synchronized(observers) { ++ observers.add(observer) ++ } + if (looper != null) { +- locationProvider.addLocationObserver(observer, looper) ++ provider.addLocationObserver(observer, looper) + } else { +- locationProvider.addLocationObserver(observer) ++ provider.addLocationObserver(observer) ++ } ++ // Gone from the list means a removal ran while we were registering, and its ++ // `removeLocationObserver` hit a provider that did not know this observer yet. Undo here. ++ val canceled = synchronized(observers) { !observers.contains(observer) } ++ if (canceled) { ++ provider.removeLocationObserver(observer) + } +- observers.add(observer) + } + ++// `observers` is reached from more than one thread: LocationManager.enable() runs on the ++// main thread when the activity resumes, and again on the React native-modules thread when ++// RNMBXLocationModule.start()/setMinDisplacement() land. Unsynchronized, the two overlap ++// inside Kotlin's `removeAll { }` (filterInPlace), whose trailing `removeAt(readIndex)` ++// walks indices captured before the other thread shrank the list — IndexOutOfBoundsException ++// "Index 0 out of bounds for length 0", crashing the app on foreground. ++// ++// Guard both mutations, and match on identity via a plain collection so `filterInPlace` ++// is not involved at all. `removeLocationObserver` runs outside the lock: it calls into the ++// Mapbox SDK and must not be holding our monitor while it does. ++// ++// Dropping an observer from the list is also the cancel signal for a registration still ++// in flight on the other thread — see `requestLocationUpdatesV11`, which re-checks ++// membership after registering and unregisters itself if it was pulled out meanwhile. + fun LocationEngine.removeLocationUpdates(callback: LocationEngineCallback) { +- observers.filter { it.callback == callback }.forEach { locationProvider.removeLocationObserver(it) } +- observers.removeAll { it.callback == callback } ++ val stale = synchronized(observers) { ++ val matched = observers.filter { it.callback == callback } ++ observers.removeAll(matched.toSet()) ++ matched ++ } ++ stale.forEach { locationProvider.removeLocationObserver(it) } + } + + fun LocationEngine.getLastLocation(callback: LocationEngineCallback) { diff --git a/patches/react-native-webview+13.16.1.patch b/patches/react-native-webview+13.16.1.patch new file mode 100644 index 00000000..1e734643 --- /dev/null +++ b/patches/react-native-webview+13.16.1.patch @@ -0,0 +1,71 @@ +diff --git a/node_modules/react-native-webview/apple/RNCWebViewDecisionManager.m b/node_modules/react-native-webview/apple/RNCWebViewDecisionManager.m +index aa90548..0c99c73 100644 +--- a/node_modules/react-native-webview/apple/RNCWebViewDecisionManager.m ++++ b/node_modules/react-native-webview/apple/RNCWebViewDecisionManager.m +@@ -1,7 +1,15 @@ + #import "RNCWebViewDecisionManager.h" + +- +- ++/** ++ * Thread-safe singleton that manages navigation decision handlers for WKWebView. ++ * ++ * This class bridges async navigation decisions between: ++ * - WKWebView delegate (main thread) - stores decision handlers ++ * - React Native bridge (background thread) - resolves decisions from JS ++ * ++ * All public methods use @synchronized for thread safety since they access ++ * shared state (nextLockIdentifier and decisionHandlers) from different threads. ++ */ + @implementation RNCWebViewDecisionManager + + @synthesize nextLockIdentifier; +@@ -16,22 +24,39 @@ + return lockManager; + } + ++/** ++ * Stores a decision handler and returns a unique identifier. ++ * Called from the main thread (WKNavigationDelegate). ++ * @synchronized ensures atomic increment + insertion. ++ */ + - (int)setDecisionHandler:(DecisionBlock)decisionHandler { +- int lockIdentifier = self.nextLockIdentifier++; +- +- [self.decisionHandlers setObject:decisionHandler forKey:@(lockIdentifier)]; +- return lockIdentifier; ++ @synchronized (self) { ++ int lockIdentifier = self.nextLockIdentifier++; ++ [self.decisionHandlers setObject:decisionHandler forKey:@(lockIdentifier)]; ++ return lockIdentifier; ++ } + } + ++/** ++ * Resolves a pending navigation decision. ++ * Called from the RN bridge thread (background) when JS responds. ++ * ++ * The handler is invoked OUTSIDE the @synchronized block to prevent deadlocks, ++ * since the handler dispatches to the main queue and could potentially ++ * trigger another navigation that re-enters this class. ++ */ + - (void) setResult:(BOOL)shouldStart + forLockIdentifier:(int)lockIdentifier { +- DecisionBlock handler = [self.decisionHandlers objectForKey:@(lockIdentifier)]; +- if (handler == nil) { +- RCTLogWarn(@"Lock not found"); +- return; ++ DecisionBlock handler; ++ @synchronized (self) { ++ handler = [self.decisionHandlers objectForKey:@(lockIdentifier)]; ++ if (handler == nil) { ++ RCTLogWarn(@"Lock not found"); ++ return; ++ } ++ [self.decisionHandlers removeObjectForKey:@(lockIdentifier)]; + } + handler(shouldStart); +- [self.decisionHandlers removeObjectForKey:@(lockIdentifier)]; + } + + - (id)init { diff --git a/src/components/__tests__/react-native-webview-patch.test.ts b/src/components/__tests__/react-native-webview-patch.test.ts new file mode 100644 index 00000000..6bdd45bf --- /dev/null +++ b/src/components/__tests__/react-native-webview-patch.test.ts @@ -0,0 +1,32 @@ +import { readFileSync } from 'fs'; +import { join } from 'path'; + +/** + * Guards `patches/react-native-webview+13.16.1.patch`. + * + * `RNCWebViewDecisionManager` is a process-wide singleton whose `decisionHandlers` + * dictionary is written from the main thread (`WKNavigationDelegate` storing a handler) + * and read from a bridge worker thread (`shouldStartLoadWithLockIdentifier` resolving one + * from JS). Unsynchronized, a lookup could probe buckets while the dictionary rehashed and + * send `isEqual:` to a freed key — `EXC_BAD_ACCESS ... KERN_INVALID_ADDRESS`, which crashed + * the app when a WebView screen was torn down mid-navigation. + * + * Upstream fixed this in 14.0.1, but Expo SDK 56 pins react-native-webview to exactly + * 13.16.1, so the fix is backported verbatim rather than taken by upgrade. + * Losing the patch — to a `yarn install`, or to an Expo bump that lands a version still + * carrying the race — brings the crash straight back. + */ +describe('react-native-webview decision manager patch', () => { + const source = readFileSync(join(process.cwd(), 'node_modules/react-native-webview/apple/RNCWebViewDecisionManager.m'), 'utf8'); + + it('serialises access to the shared decision handler map', () => { + expect(source).toContain('@synchronized (self)'); + // Both entry points must hold the lock, not just the reader. + expect(source.match(/@synchronized \(self\)/g)).toHaveLength(2); + }); + + it('invokes the handler outside the lock so a re-entrant navigation cannot deadlock', () => { + // The entry is removed inside the lock, then the closing brace, then the call. + expect(source).toMatch(/removeObjectForKey:@\(lockIdentifier\)\];\s*\}\s*handler\(shouldStart\);/); + }); +}); diff --git a/src/components/calls/dispatch-selection-modal.tsx b/src/components/calls/dispatch-selection-modal.tsx index a3129889..1149cb6d 100644 --- a/src/components/calls/dispatch-selection-modal.tsx +++ b/src/components/calls/dispatch-selection-modal.tsx @@ -128,7 +128,7 @@ export const DispatchSelectionModal: React.FC = ({ - {selection.everyone && } + {selection.everyone ? : null} {t('calls.everyone')} @@ -153,7 +153,7 @@ export const DispatchSelectionModal: React.FC = ({ selection.users.includes(user.Id) ? 'border-blue-500 bg-blue-500' : colorScheme === 'dark' ? 'border-neutral-600' : 'border-neutral-300' }`} > - {selection.users.includes(user.Id) && } + {selection.users.includes(user.Id) ? : null} {user.Name} @@ -180,7 +180,7 @@ export const DispatchSelectionModal: React.FC = ({ selection.groups.includes(group.Id) ? 'border-blue-500 bg-blue-500' : colorScheme === 'dark' ? 'border-neutral-600' : 'border-neutral-300' }`} > - {selection.groups.includes(group.Id) && } + {selection.groups.includes(group.Id) ? : null} {group.Name} @@ -207,7 +207,7 @@ export const DispatchSelectionModal: React.FC = ({ selection.roles.includes(role.Id) ? 'border-blue-500 bg-blue-500' : colorScheme === 'dark' ? 'border-neutral-600' : 'border-neutral-300' }`} > - {selection.roles.includes(role.Id) && } + {selection.roles.includes(role.Id) ? : null} {role.Name} @@ -234,7 +234,7 @@ export const DispatchSelectionModal: React.FC = ({ selection.units.includes(unit.Id) ? 'border-blue-500 bg-blue-500' : colorScheme === 'dark' ? 'border-neutral-600' : 'border-neutral-300' }`} > - {selection.units.includes(unit.Id) && } + {selection.units.includes(unit.Id) ? : null} {unit.Name} diff --git a/src/components/chat/new-conversation-sheet.tsx b/src/components/chat/new-conversation-sheet.tsx index 80c7e1e9..69e33622 100644 --- a/src/components/chat/new-conversation-sheet.tsx +++ b/src/components/chat/new-conversation-sheet.tsx @@ -19,6 +19,7 @@ import { VStack } from '@/components/ui/vstack'; import { logger } from '@/lib/logging'; import { getAvatarUrl } from '@/lib/utils'; import { type RecipientsResultData } from '@/models/v4/messages/recipientsResultData'; +import useAuthStore from '@/stores/auth/store'; import { useToastStore } from '@/stores/toast/store'; interface NewConversationSheetProps { @@ -38,8 +39,14 @@ function isPersonRecipient(recipient: RecipientsResultData): boolean { return type === 'personnel' || type === 'person' || type === 'user' || type === 'p' || type === ''; } +/** The server rejects self-DMs, so the current user never belongs in the picker. */ +function isSelfRecipient(recipient: RecipientsResultData, currentUserId: string | null): boolean { + return !!currentUserId && recipientUserId(recipient).toLowerCase() === currentUserId.toLowerCase(); +} + export function NewConversationSheet({ isOpen, onClose, mode, onCreated }: NewConversationSheetProps) { const { t } = useTranslation(); + const currentUserId = useAuthStore((s) => s.userId); const [recipients, setRecipients] = useState([]); const [loading, setLoading] = useState(false); const [loadError, setLoadError] = useState(false); @@ -59,7 +66,7 @@ export function NewConversationSheet({ isOpen, onClose, mode, onCreated }: NewCo getRecipients(true, false) .then((result) => { if (cancelled) return; - setRecipients((result.Data ?? []).filter(isPersonRecipient)); + setRecipients((result.Data ?? []).filter((r) => isPersonRecipient(r) && !isSelfRecipient(r, currentUserId))); }) .catch((error) => { if (cancelled) return; @@ -73,7 +80,7 @@ export function NewConversationSheet({ isOpen, onClose, mode, onCreated }: NewCo return () => { cancelled = true; }; - }, [isOpen]); + }, [isOpen, currentUserId]); const filtered = useMemo(() => { const q = query.trim().toLowerCase(); diff --git a/src/components/maps/__tests__/rnmapbox-version-floor.test.ts b/src/components/maps/__tests__/rnmapbox-version-floor.test.ts new file mode 100644 index 00000000..078aea9a --- /dev/null +++ b/src/components/maps/__tests__/rnmapbox-version-floor.test.ts @@ -0,0 +1,51 @@ +import mapboxPackage from '@rnmapbox/maps/package.json'; +import { readFileSync } from 'fs'; +import { join } from 'path'; + +describe('@rnmapbox/maps version floor', () => { + /** + * 10.2.x `AnimatedPoint` assigned `this._listeners = {}` in its constructor. It extends + * React Native's `AnimatedWithChildren`, and `AnimatedNode` owns that field — on RN 0.85 + * it is a `Map`, so the plain object broke `AnimatedNode.__callListeners`, which calls + * `this._listeners.forEach(...)`. + * + * `Mapbox.UserLocation` defaults to `animated`, so every location update on a screen with + * a map ran `AnimatedPoint.timing().start()` and the first frame threw + * `TypeError: undefined is not a function`, taking the app down (seen in Responder, + * reproduced on the call detail screen). 10.3.0 guards the assignment; dropping below that + * brings the crash straight back. + */ + it('is at least 10.3.0, where the AnimatedPoint listener clobber was fixed', () => { + const [major, minor] = mapboxPackage.version.split('.').map(Number); + + expect(major).toBeGreaterThanOrEqual(10); + expect(major > 10 || minor >= 3).toBe(true); + }); + + /** + * The JS bindings are generated against a specific native SDK. Pinning an older one in the + * Expo plugin leaves style props the bindings emit (`symbolZOffset`) unimplemented + * natively, which traps in `RNMBXStyle.symbolLayer` on iOS. + */ + it('pins the same native Mapbox SDK the installed bindings target', () => { + const appConfig = readFileSync(join(process.cwd(), 'app.config.ts'), 'utf8'); + const pinned = /RNMapboxMapsVersion:\s*'([^']+)'/.exec(appConfig)?.[1]; + + expect(pinned).toBe(mapboxPackage.mapbox.android); + }); + + /** + * Guards `patches/@rnmapbox+maps+10.3.5.patch`. Upstream's `LocationEngine.observers` is a + * plain list mutated from both the main thread (activity resume) and the React + * native-modules thread (`RNMBXLocationModule.start`). The overlap lands inside Kotlin's + * `removeAll { }` and throws `IndexOutOfBoundsException: Index 0 out of bounds for + * length 0`, killing the app as it foregrounds. A `yarn install` + * that drops the patch brings the crash back, so assert on the installed source. + */ + it('keeps the LocationEngine observer list guarded against concurrent mutation', () => { + const locationKt = readFileSync(join(process.cwd(), 'node_modules/@rnmapbox/maps/android/src/main/mapbox-v11-compat/v11/com/rnmapbox/rnmbx/v11compat/Location.kt'), 'utf8'); + + expect(locationKt).toContain('synchronized(observers)'); + expect(locationKt).not.toMatch(/observers\.removeAll\s*\{/); + }); +}); diff --git a/src/lib/logging/__tests__/index.test.ts b/src/lib/logging/__tests__/index.test.ts new file mode 100644 index 00000000..8e26642f --- /dev/null +++ b/src/lib/logging/__tests__/index.test.ts @@ -0,0 +1,194 @@ +// The logging module short-circuits all Sentry reporting when JEST_WORKER_ID is +// set, so the module is loaded below with that variable temporarily removed. + +const mockCaptureException = jest.fn(); +const mockCaptureMessage = jest.fn(); + +jest.mock('@sentry/react-native', () => ({ + captureException: (...args: unknown[]) => mockCaptureException(...args), + captureMessage: (...args: unknown[]) => mockCaptureMessage(...args), +})); + +// Replace the console transport so exercising the real (non-Jest) code path +// doesn't print every test's log line. +const mockTransportLog = jest.fn(); +jest.mock('react-native-logs', () => ({ + consoleTransport: jest.fn(), + logger: { + createLogger: () => ({ + debug: mockTransportLog, + info: mockTransportLog, + warn: mockTransportLog, + error: mockTransportLog, + }), + }, +})); + +jest.mock('react-native', () => ({ + Platform: { OS: 'ios' }, +})); + +import type { Logger } from '../types'; + +let logger: Logger; +let sanitizeLogContext: (context: Record | undefined) => Record; + +beforeAll(() => { + const workerId = process.env.JEST_WORKER_ID; + delete process.env.JEST_WORKER_ID; + jest.isolateModules(() => { + const loggingModule = require('../index'); + logger = loggingModule.logger; + sanitizeLogContext = loggingModule.sanitizeLogContext; + }); + process.env.JEST_WORKER_ID = workerId; +}); + +beforeEach(() => { + jest.clearAllMocks(); +}); + +describe('LogService#error Sentry reporting', () => { + it('reports an Error as an exception so Sentry groups by the real stack', () => { + const error = new Error('Config fetch failed'); + error.stack = 'Error: Config fetch failed\n at fetchConfig (core-store.ts:307:31)'; + + logger.error({ message: 'Failed to initialize app', context: { error } }); + + expect(mockCaptureMessage).not.toHaveBeenCalled(); + expect(mockCaptureException).toHaveBeenCalledTimes(1); + + const [captured] = mockCaptureException.mock.calls[0]; + expect(captured).toBeInstanceOf(Error); + expect(captured.name).toBe('Error'); + expect(captured.message).toBe('Config fetch failed'); + expect(captured.stack).toBe(error.stack); + }); + + it('preserves a custom error name so the issue title is the real exception type', () => { + const error = new Error('Network Error'); + error.name = 'AxiosError'; + + logger.error({ message: 'Failed to send location to API', context: { error } }); + + expect(mockCaptureException.mock.calls[0][0].name).toBe('AxiosError'); + }); + + it('does not forward the original error object to Sentry', () => { + // captureException serializes an error's own enumerable properties into the + // event; an AxiosError carries config.data with urlencoded credentials. + const axiosError = Object.assign(new Error('Request failed with status code 400'), { + name: 'AxiosError', + isAxiosError: true, + code: 'ERR_BAD_REQUEST', + config: { method: 'post', url: '/Connect/token', data: 'password=hunter2&grant_type=password' }, + response: { status: 400 }, + }); + + logger.error({ message: 'Failed to send location to API', context: { error: axiosError } }); + + const [captured] = mockCaptureException.mock.calls[0]; + expect(captured).not.toBe(axiosError); + expect(captured).not.toHaveProperty('config'); + expect(captured).not.toHaveProperty('response'); + expect(JSON.stringify(captured)).not.toContain('hunter2'); + }); + + it('still attaches the sanitized context as extra data', () => { + const axiosError = Object.assign(new Error('Request failed with status code 400'), { + name: 'AxiosError', + isAxiosError: true, + code: 'ERR_BAD_REQUEST', + config: { method: 'post', url: '/UnitLocation/SetUnitLocation?key=abc' }, + response: { status: 400 }, + }); + + logger.error({ message: 'Failed to send location to API', context: { error: axiosError, unitId: 'unit-123' } }); + + const [, options] = mockCaptureException.mock.calls[0]; + expect(options.extra).toEqual( + expect.objectContaining({ + message: 'Failed to send location to API', + unitId: 'unit-123', + error: expect.objectContaining({ + name: 'AxiosError', + code: 'ERR_BAD_REQUEST', + status: 400, + url: '/UnitLocation/SetUnitLocation', + isAxiosError: true, + }), + }) + ); + }); + + it('falls back to captureMessage when the context holds no Error', () => { + logger.error({ message: 'Token refresh rejected by server', context: { error: 'invalid_grant' } }); + + expect(mockCaptureException).not.toHaveBeenCalled(); + expect(mockCaptureMessage).toHaveBeenCalledWith('Token refresh rejected by server', { level: 'error', extra: { error: 'invalid_grant' } }); + }); + + it('falls back to captureMessage when there is no context at all', () => { + logger.error({ message: 'Something went wrong' }); + + expect(mockCaptureException).not.toHaveBeenCalled(); + expect(mockCaptureMessage).toHaveBeenCalledWith('Something went wrong', { level: 'error', extra: {} }); + }); + + it('includes operation and trace_id in the reported extra data', () => { + logger.error({ message: 'Request failed', operation: 'token_refresh', trace_id: 'abc123' }); + + expect(mockCaptureMessage).toHaveBeenCalledWith('Request failed', { level: 'error', extra: { operation: 'token_refresh', trace_id: 'abc123' } }); + }); + + it('does not report warn, info or debug to Sentry', () => { + logger.warn({ message: 'Failed to send location to API', context: { error: new Error('Network Error') } }); + logger.info({ message: 'Location successfully sent to API' }); + logger.debug({ message: 'Skipping location API call' }); + + expect(mockCaptureException).not.toHaveBeenCalled(); + expect(mockCaptureMessage).not.toHaveBeenCalled(); + }); +}); + +describe('sanitizeLogContext', () => { + it('redacts sensitive keys', () => { + expect(sanitizeLogContext({ accessToken: 'abc', refresh_token: 'def', unitId: 'unit-123' })).toEqual({ + accessToken: '[REDACTED]', + refresh_token: '[REDACTED]', + unitId: 'unit-123', + }); + }); + + it('reduces an axios error to a summary without the request body', () => { + const axiosError = Object.assign(new Error('Request failed with status code 401'), { + name: 'AxiosError', + isAxiosError: true, + code: 'ERR_BAD_REQUEST', + config: { method: 'post', url: '/Connect/token?x=1', baseURL: 'https://api.resgrid.com/api/v4', data: 'password=hunter2' }, + response: { status: 401 }, + }); + + expect(sanitizeLogContext({ error: axiosError })).toEqual({ + error: { + name: 'AxiosError', + message: 'Request failed with status code 401', + code: 'ERR_BAD_REQUEST', + status: 401, + method: 'post', + url: '/Connect/token', + baseURL: 'https://api.resgrid.com/api/v4', + isAxiosError: true, + }, + }); + }); + + it('expands a plain Error, whose properties are non-enumerable', () => { + const error = new Error('boom'); + error.stack = 'Error: boom\n at somewhere'; + + expect(sanitizeLogContext({ error })).toEqual({ + error: { name: 'Error', message: 'boom', stack: 'Error: boom\n at somewhere' }, + }); + }); +}); diff --git a/src/lib/logging/index.tsx b/src/lib/logging/index.tsx index 881210ff..60e0459e 100644 --- a/src/lib/logging/index.tsx +++ b/src/lib/logging/index.tsx @@ -72,6 +72,23 @@ export const sanitizeLogContext = (context: LogContext | undefined): LogContext return sanitizeObject(context as Record, 2); }; +/** + * Rebuilds a bare Error carrying only name/message/stack so Sentry groups the + * issue by the real exception and shows the originating frames. + * + * The original is never handed to Sentry directly: `captureException` serializes + * an error's own enumerable properties into the event, which for an AxiosError + * means `config.data` — the urlencoded password/token bodies `summarizeAxiosError` + * exists to strip. Copying three fields keeps the grouping win without the leak. + */ +const toCaptureableError = (value: unknown): Error | null => { + if (!(value instanceof Error)) return null; + const stripped = new Error(value.message); + stripped.name = value.name; + stripped.stack = value.stack; + return stripped; +}; + // On web, async: true wraps every log call in setTimeout which — combined with // Sentry's setTimeout instrumentation — creates unbounded memory growth. // Setting async: false on web prevents this. Severity stays 'debug' in dev @@ -159,14 +176,20 @@ class LogService { public error(entry: LogEntry): void { this.log('error', entry); if (!isJest) { + // Read the error BEFORE sanitizing: sanitizeLogContext replaces every + // Error with a plain object, so testing the sanitized value for + // `instanceof Error` never matched and every logger.error was reported as + // a message fingerprinted on LogService#error — no exception type and no + // originating stack on any issue in the project. + const rawError = entry.context?.error; const sanitized = sanitizeLogContext({ ...entry.context, ...(entry.operation ? { operation: entry.operation } : {}), ...(entry.trace_id ? { trace_id: entry.trace_id } : {}), }); - const err = sanitized.error; - if (err instanceof Error) { - Sentry.captureException(err, { extra: { message: entry.message, ...sanitized } }); + const captureable = toCaptureableError(rawError); + if (captureable) { + Sentry.captureException(captureable, { extra: { message: entry.message, ...sanitized } }); } else { Sentry.captureMessage(entry.message, { level: 'error', extra: sanitized }); } diff --git a/src/services/__tests__/location.test.ts b/src/services/__tests__/location.test.ts index e4bd9ca9..9349537b 100644 --- a/src/services/__tests__/location.test.ts +++ b/src/services/__tests__/location.test.ts @@ -7,6 +7,7 @@ jest.mock('@/lib/hooks/use-background-geolocation', () => ({ })); jest.mock('@/lib/logging', () => ({ logger: { + debug: jest.fn(), info: jest.fn(), warn: jest.fn(), error: jest.fn(), @@ -664,6 +665,176 @@ describe('LocationService', () => { }); }); + describe('Unavailable sensor values', () => { + // iOS reports -1 for course, speed and the accuracy fields when the value + // is unavailable, which a stationary unit does on every fix. + const locationWithSentinels: Location.LocationObject = { + coords: { + latitude: 52.08197889841628, + longitude: -4.68186865536404, + altitude: -3.5, + accuracy: -1, + altitudeAccuracy: -1, + heading: -1, + speed: -1, + }, + timestamp: 1_755_176_389_981, + }; + + it('sends 0 instead of the iOS -1 sentinel values', async () => { + await locationService.startLocationUpdates(); + const locationCallback = mockLocation.watchPositionAsync.mock.calls[0][1] as Function; + await locationCallback(locationWithSentinels); + + expect(mockSetUnitLocation).toHaveBeenCalledWith( + expect.objectContaining({ + Accuracy: '0', + AltitudeAccuracy: '0', + Speed: '0', + Heading: '0', + }) + ); + }); + + it('preserves a legitimately negative altitude', async () => { + await locationService.startLocationUpdates(); + const locationCallback = mockLocation.watchPositionAsync.mock.calls[0][1] as Function; + await locationCallback(locationWithSentinels); + + expect(mockSetUnitLocation).toHaveBeenCalledWith(expect.objectContaining({ Altitude: '-3.5' })); + }); + + it('does not queue the -1 sentinels for offline replay', async () => { + mockSetUnitLocation.mockRejectedValue(new Error('Network Error')); + mockIsNetworkError.mockReturnValue(true); + + await locationService.startLocationUpdates(); + const locationCallback = mockLocation.watchPositionAsync.mock.calls[0][1] as Function; + await locationCallback(locationWithSentinels); + + expect(mockQueueLocationUpdateEvent).toHaveBeenCalledWith('unit-123', locationWithSentinels.coords.latitude, locationWithSentinels.coords.longitude, undefined, undefined, undefined); + }); + }); + + describe('Server rejection backoff', () => { + // Unit IDs here are deliberately distinct from 'unit-123': the backoff + // state lives in the module, and a rejection recorded for one unit must + // not leak into the other tests in this file. + const createAxiosError = (status: number, data: unknown = { Message: 'Invalid location' }) => + Object.assign(new Error(`Request failed with status code ${status}`), { + isAxiosError: true, + response: { status, data }, + }); + + let nowSpy: jest.SpyInstance; + let now = 1_700_000_000_000; + + beforeEach(() => { + now = 1_700_000_000_000; + nowSpy = jest.spyOn(Date, 'now').mockImplementation(() => now); + }); + + afterEach(() => { + nowSpy.mockRestore(); + mockCoreStoreState.activeUnitId = 'unit-123'; + }); + + const sendLocation = async (): Promise => { + await locationService.startLocationUpdates(); + const locationCallback = mockLocation.watchPositionAsync.mock.calls.at(-1)![1] as Function; + await locationCallback(mockLocationObject); + }; + + it('logs the response status and body so the rejection can be diagnosed', async () => { + mockCoreStoreState.activeUnitId = 'unit-reject-status'; + mockSetUnitLocation.mockRejectedValue(createAxiosError(400, { Message: 'Heading out of range' })); + + await sendLocation(); + + expect(mockLogger.warn).toHaveBeenCalledWith({ + message: 'Failed to send location to API', + context: { + error: 'Request failed with status code 400', + status: 400, + response: { Message: 'Heading out of range' }, + latitude: mockLocationObject.coords.latitude, + longitude: mockLocationObject.coords.longitude, + }, + }); + }); + + it('stops sending for the backoff window after a 4xx and resumes once it elapses', async () => { + mockCoreStoreState.activeUnitId = 'unit-reject-backoff'; + mockSetUnitLocation.mockRejectedValue(createAxiosError(400)); + + await sendLocation(); + expect(mockSetUnitLocation).toHaveBeenCalledTimes(1); + + // Well inside the 30s window — the next fix must not hit the API. + now += 10_000; + await sendLocation(); + expect(mockSetUnitLocation).toHaveBeenCalledTimes(1); + expect(mockLogger.debug).toHaveBeenCalledWith(expect.objectContaining({ message: 'Skipping location API call while backing off after server rejection' })); + + now += 25_000; + await sendLocation(); + expect(mockSetUnitLocation).toHaveBeenCalledTimes(2); + }); + + it('escalates the backoff on repeated rejections', async () => { + mockCoreStoreState.activeUnitId = 'unit-reject-escalate'; + mockSetUnitLocation.mockRejectedValue(createAxiosError(400)); + + await sendLocation(); + expect(mockLogger.warn).toHaveBeenCalledWith(expect.objectContaining({ message: 'Backing off location updates after server rejection', context: expect.objectContaining({ backoffMs: 30_000 }) })); + + now += 31_000; + await sendLocation(); + expect(mockLogger.warn).toHaveBeenCalledWith(expect.objectContaining({ message: 'Backing off location updates after server rejection', context: expect.objectContaining({ backoffMs: 60_000 }) })); + }); + + it('clears the backoff when the active unit changes', async () => { + mockCoreStoreState.activeUnitId = 'unit-reject-switch-a'; + mockSetUnitLocation.mockRejectedValue(createAxiosError(400)); + + await sendLocation(); + expect(mockSetUnitLocation).toHaveBeenCalledTimes(1); + + // Same instant, different unit: the previous rejection says nothing + // about this one. + mockCoreStoreState.activeUnitId = 'unit-reject-switch-b'; + await sendLocation(); + expect(mockSetUnitLocation).toHaveBeenCalledTimes(2); + }); + + it('clears the backoff after a successful send', async () => { + mockCoreStoreState.activeUnitId = 'unit-reject-recover'; + mockSetUnitLocation.mockRejectedValue(createAxiosError(400)); + await sendLocation(); + + now += 31_000; + mockSetUnitLocation.mockResolvedValue(mockApiResponse); + await sendLocation(); + + mockLogger.warn.mockClear(); + mockSetUnitLocation.mockRejectedValue(createAxiosError(400)); + await sendLocation(); + + // Counter restarted, so this is a first rejection again, not a third. + expect(mockLogger.warn).toHaveBeenCalledWith(expect.objectContaining({ message: 'Backing off location updates after server rejection', context: expect.objectContaining({ consecutiveRejections: 1, backoffMs: 30_000 }) })); + }); + + it('does not back off on server errors, which are transient', async () => { + mockCoreStoreState.activeUnitId = 'unit-reject-5xx'; + mockSetUnitLocation.mockRejectedValue(createAxiosError(503)); + + await sendLocation(); + await sendLocation(); + + expect(mockSetUnitLocation).toHaveBeenCalledTimes(2); + }); + }); + describe('Background Geolocation Setting Updates', () => { it('should enable background tracking and register task when permissions are granted', async () => { await locationService.updateBackgroundGeolocationSetting(true); diff --git a/src/services/__tests__/push-notification-hook.test.ts b/src/services/__tests__/push-notification-hook.test.ts new file mode 100644 index 00000000..4f58dfbb --- /dev/null +++ b/src/services/__tests__/push-notification-hook.test.ts @@ -0,0 +1,257 @@ +// Tests for the usePushNotifications hook. Kept in its own file because the +// hook needs @testing-library/react-native, while push-notification.test.ts +// exercises the service through a bare-module import. + +jest.mock('react-native', () => ({ + Platform: { + OS: 'ios', + select: jest.fn((obj: any) => obj.ios ?? obj.default), + }, +})); + +jest.mock('expo-device', () => ({ + isDevice: true, + deviceName: 'Test Device', + osName: 'iOS', + osVersion: '15.0', +})); + +jest.mock('expo-notifications', () => ({ + setNotificationHandler: jest.fn(), + addNotificationReceivedListener: jest.fn(() => ({ remove: jest.fn() })), + addNotificationResponseReceivedListener: jest.fn(() => ({ remove: jest.fn() })), + getLastNotificationResponseAsync: jest.fn(() => Promise.resolve(null)), + getPermissionsAsync: jest.fn(() => Promise.resolve({ status: 'granted' })), + requestPermissionsAsync: jest.fn(() => Promise.resolve({ status: 'granted' })), + getDevicePushTokenAsync: jest.fn(() => Promise.resolve({ data: 'test-device-token' })), + AndroidImportance: { MAX: 5, HIGH: 4, DEFAULT: 3 }, + AndroidNotificationVisibility: { PUBLIC: 1 }, +})); + +jest.mock('@notifee/react-native', () => ({ + __esModule: true, + default: { + createChannel: jest.fn(() => Promise.resolve()), + deleteChannel: jest.fn(() => Promise.resolve()), + setNotificationCategories: jest.fn(() => Promise.resolve()), + requestPermission: jest.fn(() => Promise.resolve({ authorizationStatus: 1 })), + displayNotification: jest.fn(() => Promise.resolve('notification-id')), + onForegroundEvent: jest.fn(() => jest.fn()), + onBackgroundEvent: jest.fn(), + }, + AndroidImportance: { HIGH: 4, DEFAULT: 3 }, + AndroidVisibility: { PUBLIC: 1 }, + AuthorizationStatus: { AUTHORIZED: 1, DENIED: 2 }, + EventType: { PRESS: 1, ACTION_PRESS: 2 }, +})); + +jest.mock('@/lib/navigation', () => ({ + routerPushWithRetry: jest.fn().mockResolvedValue(undefined), +})); + +jest.mock('@/lib/logging', () => ({ + logger: { + info: jest.fn(), + error: jest.fn(), + warn: jest.fn(), + debug: jest.fn(), + }, +})); + +jest.mock('@/lib/storage/app', () => ({ + getDeviceUuid: jest.fn(() => 'test-device-uuid'), +})); + +jest.mock('@/lib/storage/notification-prefs', () => ({ + getModernNotificationSoundsEnabled: jest.fn(() => true), + getAppliedNotificationSoundMode: jest.fn(() => undefined), + setAppliedNotificationSoundMode: jest.fn(), +})); + +jest.mock('@/api/devices/push', () => ({ + registerUnitDevice: jest.fn(), +})); + +jest.mock('@/stores/push-notification/store', () => ({ + usePushNotificationModalStore: { + getState: jest.fn(() => ({ showNotificationModal: jest.fn() })), + }, +})); + +jest.mock('@/stores/check-in-timers/store', () => ({ + useCheckInTimerStore: { + getState: jest.fn(() => ({ performCheckIn: jest.fn() })), + }, +})); + +jest.mock('@/stores/app/location-store', () => ({ + useLocationStore: { + getState: jest.fn(() => ({ latitude: null, longitude: null })), + }, +})); + +// The three stores the hook actually gates on. Each keeps mutable state plus a +// __setState helper so a test can model hydration order (persisted unit/rights +// available before auth has settled). +jest.mock('@/stores/app/core-store', () => { + const state: any = { activeUnitId: 'test-unit' }; + return { + useCoreStore: Object.assign((selector: any) => (selector ? selector(state) : state), { + __setState: (next: any) => Object.assign(state, next), + }), + }; +}); + +jest.mock('@/stores/security/store', () => { + const state: any = { rights: { DepartmentCode: 'TEST' } }; + return { + securityStore: Object.assign((selector: any) => (selector ? selector(state) : state), { + __setState: (next: any) => Object.assign(state, next), + }), + }; +}); + +jest.mock('@/stores/auth/store', () => { + const state: any = { status: 'signedIn', accessToken: 'test-access-token' }; + const store: any = (selector: any) => (selector ? selector(state) : state); + store.getState = () => state; + store.__setState = (next: any) => Object.assign(state, next); + return { __esModule: true, default: store }; +}); + +import { renderHook, waitFor } from '@testing-library/react-native'; + +import { logger } from '@/lib/logging'; +import { useCoreStore } from '@/stores/app/core-store'; +import useAuthStore from '@/stores/auth/store'; +import { securityStore } from '@/stores/security/store'; + +import { pushNotificationService, usePushNotifications } from '../push-notification'; + +const setAuthState = (next: { status?: string; accessToken?: string | null }) => (useAuthStore as unknown as { __setState: (n: unknown) => void }).__setState(next); +const setCoreState = (next: { activeUnitId?: string | null }) => (useCoreStore as unknown as { __setState: (n: unknown) => void }).__setState(next); +const setSecurityState = (next: { rights: unknown }) => (securityStore as unknown as { __setState: (n: unknown) => void }).__setState(next); + +describe('usePushNotifications', () => { + let registerSpy: jest.SpyInstance; + + beforeEach(() => { + jest.clearAllMocks(); + setAuthState({ status: 'signedIn', accessToken: 'test-access-token' }); + setCoreState({ activeUnitId: 'test-unit' }); + setSecurityState({ rights: { DepartmentCode: 'TEST' } }); + registerSpy = jest.spyOn(pushNotificationService, 'registerForPushNotifications').mockResolvedValue('test-device-token'); + }); + + afterEach(() => { + registerSpy.mockRestore(); + }); + + it('registers when the user is signed in and a unit is active', async () => { + const { unmount } = renderHook(() => usePushNotifications()); + + await waitFor(() => { + expect(registerSpy).toHaveBeenCalledWith('test-unit', 'TEST'); + }); + + unmount(); + }); + + it('does not register while auth has not settled, even with a persisted unit and rights', async () => { + // Cold background start: MMKV-persisted unit/rights hydrate immediately + // while the auth store is still 'idle'. Registering here would send a + // stale token, 401, and force a refresh that can log the user out. + setAuthState({ status: 'idle' }); + + const { unmount } = renderHook(() => usePushNotifications()); + + await waitFor(() => { + expect(registerSpy).not.toHaveBeenCalled(); + }); + + unmount(); + }); + + it('does not register when signed in but no access token is present', async () => { + setAuthState({ status: 'signedIn', accessToken: null }); + + const { unmount } = renderHook(() => usePushNotifications()); + + await waitFor(() => { + expect(registerSpy).not.toHaveBeenCalled(); + }); + + unmount(); + }); + + it('registers once auth transitions to signed in', async () => { + setAuthState({ status: 'loading', accessToken: null }); + + const { rerender, unmount } = renderHook(() => usePushNotifications()); + + expect(registerSpy).not.toHaveBeenCalled(); + + setAuthState({ status: 'signedIn', accessToken: 'test-access-token' }); + rerender({}); + + await waitFor(() => { + expect(registerSpy).toHaveBeenCalledWith('test-unit', 'TEST'); + }); + + unmount(); + }); + + it('does not register again for the same unit after a successful registration', async () => { + const { rerender, unmount } = renderHook(() => usePushNotifications()); + + await waitFor(() => { + expect(registerSpy).toHaveBeenCalledTimes(1); + }); + + // New rights object identity re-runs the effect; the unit is unchanged and + // already registered, so no second call. + setSecurityState({ rights: { DepartmentCode: 'TEST' } }); + rerender({}); + + await waitFor(() => { + expect(logger.info).toHaveBeenCalledWith(expect.objectContaining({ message: 'Successfully registered for push notifications' })); + }); + expect(registerSpy).toHaveBeenCalledTimes(1); + + unmount(); + }); + + it('retries on the next effect run when registration failed', async () => { + // registerForPushNotifications swallows its errors and resolves null, so a + // transient failure must not mark the unit as registered — otherwise push + // stays dead for the rest of the app session. + registerSpy.mockResolvedValueOnce(null); + + const { rerender, unmount } = renderHook(() => usePushNotifications()); + + await waitFor(() => { + expect(registerSpy).toHaveBeenCalledTimes(1); + }); + + setSecurityState({ rights: { DepartmentCode: 'TEST' } }); + rerender({}); + + await waitFor(() => { + expect(registerSpy).toHaveBeenCalledTimes(2); + }); + + unmount(); + }); + + it('logs an error when the registration promise rejects', async () => { + registerSpy.mockRejectedValueOnce(new Error('boom')); + + const { unmount } = renderHook(() => usePushNotifications()); + + await waitFor(() => { + expect(logger.error).toHaveBeenCalledWith(expect.objectContaining({ message: 'Error in push notification registration hook' })); + }); + + unmount(); + }); +}); diff --git a/src/services/__tests__/push-notification.test.ts b/src/services/__tests__/push-notification.test.ts index b68b8d90..4ff64863 100644 --- a/src/services/__tests__/push-notification.test.ts +++ b/src/services/__tests__/push-notification.test.ts @@ -78,6 +78,15 @@ jest.mock('@/stores/app/location-store', () => ({ }, })); +// The hook gates registration on auth state; the service tests never render it, +// but the module-level import still has to resolve without the real store. +jest.mock('@/stores/auth/store', () => { + const state = { status: 'signedIn', accessToken: 'test-access-token' }; + const store: any = (selector: any) => (selector ? selector(state) : state); + store.getState = () => state; + return { __esModule: true, default: store }; +}); + // Mock expo-notifications (the push transport) const mockReceivedRemove = jest.fn(); const mockResponseRemove = jest.fn(); diff --git a/src/services/location.ts b/src/services/location.ts index 1b58565a..3019ec40 100644 --- a/src/services/location.ts +++ b/src/services/location.ts @@ -1,3 +1,4 @@ +import axios from 'axios'; import * as Location from 'expo-location'; import * as TaskManager from 'expo-task-manager'; import { AppState, type AppStateStatus } from 'react-native'; @@ -15,6 +16,31 @@ import { isNetworkError } from '@/utils/network'; const LOCATION_TASK_NAME = 'location-updates'; +// A 4xx from SetUnitLocation is deterministic: the same unit sending the same +// shape of payload will be rejected again on the next fix. Foreground updates +// arrive every 15s, so without a backoff a single rejected unit produces a +// failed request — and a log line — indefinitely. +const REJECTION_BACKOFF_BASE_MS = 30 * 1000; +const REJECTION_BACKOFF_MAX_MS = 15 * 60 * 1000; + +let rejectedUnitId: string | null = null; +let consecutiveRejections = 0; +let nextAttemptAtMs = 0; + +const resetRejectionBackoff = (): void => { + rejectedUnitId = null; + consecutiveRejections = 0; + nextAttemptAtMs = 0; +}; + +/** + * iOS reports -1 for course, speed and the accuracy fields when the value is + * unavailable — a stationary unit (parked at the station, screen off) reports + * it on every single fix. Returns undefined for those sentinels so they are + * never sent to the API or queued for offline replay. + */ +const nonNegativeOrUndefined = (value: number | null | undefined): number | undefined => (typeof value === 'number' && Number.isFinite(value) && value >= 0 ? value : undefined); + // Helper function to send location to API const sendLocationToAPI = async (location: Location.LocationObject): Promise => { const { activeUnitId } = useCoreStore.getState(); @@ -26,19 +52,39 @@ const sendLocationToAPI = async (location: Location.LocationObject): Promise Date.now()) { + logger.debug({ + message: 'Skipping location API call while backing off after server rejection', + context: { unitId: activeUnitId, consecutiveRejections, nextAttemptAtMs }, + }); + return; + } + + const { latitude, longitude, altitude, accuracy, altitudeAccuracy, speed, heading } = location.coords; + const locationInput = new SaveUnitLocationInput(); locationInput.UnitId = activeUnitId; 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'; + locationInput.Latitude = latitude.toString(); + locationInput.Longitude = longitude.toString(); + locationInput.Accuracy = nonNegativeOrUndefined(accuracy)?.toString() ?? '0'; + // Altitude is legitimately negative below sea level, so only non-finite + // values are replaced. + locationInput.Altitude = typeof altitude === 'number' && Number.isFinite(altitude) ? altitude.toString() : '0'; + locationInput.AltitudeAccuracy = nonNegativeOrUndefined(altitudeAccuracy)?.toString() ?? '0'; + locationInput.Speed = nonNegativeOrUndefined(speed)?.toString() ?? '0'; + locationInput.Heading = nonNegativeOrUndefined(heading)?.toString() ?? '0'; const result = await setUnitLocation(locationInput); + resetRejectionBackoff(); + logger.info({ message: 'Location successfully sent to API', context: { @@ -49,15 +95,33 @@ const sendLocationToAPI = async (location: Location.LocationObject): Promise= 400 && status < 500 && activeUnitId) { + rejectedUnitId = activeUnitId; + consecutiveRejections += 1; + const backoffMs = Math.min(REJECTION_BACKOFF_BASE_MS * 2 ** (consecutiveRejections - 1), REJECTION_BACKOFF_MAX_MS); + nextAttemptAtMs = Date.now() + backoffMs; + + logger.warn({ + message: 'Backing off location updates after server rejection', + context: { unitId: activeUnitId, status, consecutiveRejections, backoffMs }, + }); + } + // Queue the position for offline replay on genuine network failures so the // unit's location on the server does not silently go stale. Server // rejections (4xx) are NOT queued — replaying them would fail forever. @@ -67,9 +131,9 @@ const sendLocationToAPI = async (location: Location.LocationObject): Promise { const activeUnitId = useCoreStore((state) => state.activeUnitId); const rights = securityStore((state) => state.rights); + const authStatus = useAuthStore((state) => state.status); const previousUnitIdRef = useRef(null); useEffect(() => { // Push notifications are native-only; skip on web if (Platform.OS === 'web') return; + // Don't register until auth has settled. On a cold start (especially a + // background wake) the persisted activeUnitId and rights hydrate before + // token validity is known, so registering here sends a request with a + // stale token: the 401 forces a refresh and, if the refresh token has + // expired, logs the user out. + if (authStatus !== 'signedIn' || !useAuthStore.getState().accessToken) return; + // Only register if we have an active unit ID and it's different from the previous one if (rights && activeUnitId && activeUnitId !== previousUnitIdRef.current) { pushNotificationService .registerForPushNotifications(activeUnitId, rights.DepartmentCode) .then((token) => { if (token) { + // Only mark the unit as registered on success. Marking it + // unconditionally meant a single transient failure (401 during + // hydration, offline, 5xx) permanently disabled push for the rest + // of the app session, since the ref then matched forever. + previousUnitIdRef.current = activeUnitId; logger.info({ message: 'Successfully registered for push notifications', context: { unitId: activeUnitId }, @@ -570,15 +584,13 @@ export const usePushNotifications = () => { context: { error }, }); }); - - previousUnitIdRef.current = activeUnitId; } // Cleanup function return () => { // No need to clean up here as the service handles its own cleanup }; - }, [activeUnitId, rights]); + }, [activeUnitId, rights, authStatus]); return { pushToken: pushNotificationService.getPushToken(), diff --git a/src/stores/app/__tests__/location-store.test.ts b/src/stores/app/__tests__/location-store.test.ts index e7e060ad..7e1e981d 100644 --- a/src/stores/app/__tests__/location-store.test.ts +++ b/src/stores/app/__tests__/location-store.test.ts @@ -164,4 +164,64 @@ describe('useLocationStore', () => { expect(typeof result.current.setBackgroundEnabled).toBe('function'); expect(typeof result.current.setMapLocked).toBe('function'); }); + + // iOS ignores watchPositionAsync's timeInterval, so a stationary device delivers the same + // fix many times a second. Notifying subscribers on each one drove React into "Maximum + // update depth exceeded". + describe('repeat fixes', () => { + const buildLocation = (overrides: { latitude?: number; timestamp?: number } = {}) => ({ + coords: { + latitude: overrides.latitude ?? 40.7128, + longitude: -74.006, + heading: 180, + accuracy: 5, + speed: 0, + altitude: 10, + altitudeAccuracy: 3, + }, + timestamp: overrides.timestamp ?? 1_700_000_000_000, + }); + + it('does not notify subscribers when the fix carries nothing new', () => { + const listener = jest.fn(); + useLocationStore.getState().setLocation(buildLocation()); + + const unsubscribe = useLocationStore.subscribe(listener); + useLocationStore.getState().setLocation(buildLocation({ timestamp: 1_700_000_005_000 })); + unsubscribe(); + + expect(listener).not.toHaveBeenCalled(); + expect(useLocationStore.getState().latitude).toBe(40.7128); + }); + + // persist's wrapped `set` writes storage after every call it sees, so the duplicate check + // has to happen before `set` is reached — otherwise a stationary device re-serializes and + // re-writes MMKV at the same many-times-a-second rate. + it('does not touch persisted storage when the fix carries nothing new', () => { + const { zustandStorage } = require('@/lib/storage'); + useLocationStore.getState().setLocation(buildLocation()); + + const setItemSpy = jest.spyOn(zustandStorage, 'setItem'); + + useLocationStore.getState().setLocation(buildLocation({ timestamp: 1_700_000_005_000 })); + expect(setItemSpy).not.toHaveBeenCalled(); + + useLocationStore.getState().setLocation(buildLocation({ latitude: 40.73 })); + expect(setItemSpy).toHaveBeenCalledTimes(1); + + setItemSpy.mockRestore(); + }); + + it('still notifies subscribers when the device actually moves', () => { + const listener = jest.fn(); + useLocationStore.getState().setLocation(buildLocation()); + + const unsubscribe = useLocationStore.subscribe(listener); + useLocationStore.getState().setLocation(buildLocation({ latitude: 40.72 })); + unsubscribe(); + + expect(listener).toHaveBeenCalledTimes(1); + expect(useLocationStore.getState().latitude).toBe(40.72); + }); + }); }); diff --git a/src/stores/app/core-store.ts b/src/stores/app/core-store.ts index 62d5b61b..dc61688b 100644 --- a/src/stores/app/core-store.ts +++ b/src/stores/app/core-store.ts @@ -85,14 +85,12 @@ export const useCoreStore = create()( set({ isLoading: true, isInitializing: true, error: null }); try { - // Fetch config first before anything else - this is critical for SignalR connections + // Fetch config first before anything else - this is critical for SignalR connections. + // fetchConfig rethrows, so a failure aborts init here with the original + // error intact — callers upstream classify it with isNetworkError, which + // a synthetic replacement Error would defeat. await get().fetchConfig(); - // If config fetch failed, don't continue initialization - if (get().error) { - throw new Error('Config fetch failed, cannot continue initialization'); - } - const activeUnitId = getActiveUnitId(); const activeCallId = getActiveCallId(); diff --git a/src/stores/app/location-store.ts b/src/stores/app/location-store.ts index 31b793d4..6b403854 100644 --- a/src/stores/app/location-store.ts +++ b/src/stores/app/location-store.ts @@ -21,7 +21,7 @@ export interface LocationState { export const useLocationStore = create()( persist( - (set) => ({ + (set, get) => ({ latitude: null, longitude: null, heading: null, @@ -31,16 +31,28 @@ export const useLocationStore = create()( timestamp: null, isBackgroundEnabled: false, isMapLocked: false, - setLocation: (location) => - set({ - latitude: location.coords.latitude, - longitude: location.coords.longitude, - heading: location.coords.heading, - accuracy: location.coords.accuracy, - speed: location.coords.speed, - altitude: location.coords.altitude, - timestamp: location.timestamp, - }), + // iOS ignores `timeInterval` on watchPositionAsync, so a stationary device still + // delivers fixes many times a second. Writing every one of them notified every + // subscriber at that rate and React eventually gave up with "Maximum update depth + // exceeded". Bail out when the fix carries nothing new. + // + // The comparison runs against `get()` *before* `set`, not inside the updater: persist + // hands the creator a wrapped `set` that calls its storage write after every + // invocation, whether or not the updater changed anything. Deduping inside the updater + // silences the subscribers but still serializes and writes MMKV on each repeat fix. + // + // `timestamp` is deliberately left out of the comparison: it changes on every fix and + // has no subscriber, so including it would defeat the guard. + setLocation: (location) => { + const { latitude, longitude, heading, accuracy, speed, altitude } = location.coords; + const state = get(); + + if (state.latitude === latitude && state.longitude === longitude && state.heading === heading && state.accuracy === accuracy && state.speed === speed && state.altitude === altitude) { + return; + } + + set({ latitude, longitude, heading, accuracy, speed, altitude, timestamp: location.timestamp }); + }, setBackgroundEnabled: (enabled) => set({ isBackgroundEnabled: enabled }), setMapLocked: (locked) => set({ isMapLocked: locked }), }), diff --git a/yarn.lock b/yarn.lock index e298fcb5..a56ded59 100644 --- a/yarn.lock +++ b/yarn.lock @@ -3885,10 +3885,10 @@ "@react-aria/switch" "^3.8.0" "@react-spectrum/switch" "^3.7.0" -"@rnmapbox/maps@10.2.10": - version "10.2.10" - resolved "https://registry.yarnpkg.com/@rnmapbox/maps/-/maps-10.2.10.tgz#5ba121ffd1017e5a5550e9d4829f53701e87ce87" - integrity sha512-OfjW0rHp5bUWfzBo5fZ7qdKwAzGoocXYTsSssSPVMxZ2Y7axuhcbmsO5bV6gg+BJs5RwEsghzwTIoGydBNUClA== +"@rnmapbox/maps@10.3.5": + version "10.3.5" + resolved "https://registry.yarnpkg.com/@rnmapbox/maps/-/maps-10.3.5.tgz#bbc57d910654d79d31f37122d4cb23e48c94bc52" + integrity sha512-ShQppktl1H4HHPp/aninsEVK7+tsHmN48ULxpB1rfmx7W3WogB21Q1NAEw7CzxERnLZveijyutszJG2y3EBn2Q== dependencies: "@turf/along" "6.5.0" "@turf/distance" "6.5.0"