Conversation
️✅ There are no secrets present in this pull request anymore.If these secrets were true positive and are still valid, we highly recommend you to revoke them. 🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request. |
📝 WalkthroughWalkthroughThe PR hardens authentication, external URL handling, logging, caching, SAML callbacks, realtime reconnection, offline queuing, app teardown, map rendering, timers, and list performance. It also adds or updates tests for these behaviors. ChangesApplication reliability and security
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant ApiClient
participant AuthStore
participant RefreshLock
participant API
ApiClient->>AuthStore: handle 401 response
AuthStore->>RefreshLock: refreshTokenSingleFlight
RefreshLock->>API: refresh token request
API-->>RefreshLock: AuthResponse
RefreshLock-->>AuthStore: shared refresh result
AuthStore-->>ApiClient: refreshed access token or failure
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
This comment has been minimized.
This comment has been minimized.
| mainWindow.webContents.setWindowOpenHandler(({ url }) => { | ||
| require('electron').shell.openExternal(url); | ||
| if (/^https?:\/\//i.test(url)) { | ||
| require('electron').shell.openExternal(url); |
There was a problem hiding this comment.
Unguarded external OS call — shell.openExternal(url) is invoked without a try/catch wrapper, leaving missing-browser and permission-denied failures unhandled. Wrap the call in try/catch with context and log the url and err on failure (e.g., try { await require('electron').shell.openExternal(url); } catch (err) { console.error('openExternal failed', { url, err }); }).
Kody rule violation: Add try-catch blocks for external calls
Prompt for LLM
File electron/main.js:
Line 83:
Unguarded external OS call — shell.openExternal(url) is invoked without a try/catch wrapper, leaving missing-browser and permission-denied failures unhandled. Wrap the call in try/catch with context and log the url and err on failure (e.g., `try { await require('electron').shell.openExternal(url); } catch (err) { console.error('openExternal failed', { url, err }); }`).
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| logger.warn({ | ||
| message: 'Request failed after token refresh attempt', | ||
| context: { error: refreshError }, | ||
| }); |
There was a problem hiding this comment.
Token-refresh failure log lacks the operation name and correlation identifiers required by Rule [3] for cross-log/SIEM correlation. Add structured fields for the operation and a correlation identifier, e.g., logger.warn({ message: 'Request failed after token refresh attempt', operation: 'token_refresh', trace_id: originalRequest.headers?.['x-trace-id'], context: { error: refreshError } });.
Kody rule violation: Include error context in structured logs
Prompt for LLM
File src/api/common/client.tsx:
Line 114 to 117:
Token-refresh failure log lacks the operation name and correlation identifiers required by Rule [3] for cross-log/SIEM correlation. Add structured fields for the operation and a correlation identifier, e.g., `logger.warn({ message: 'Request failed after token refresh attempt', operation: 'token_refresh', trace_id: originalRequest.headers?.['x-trace-id'], context: { error: refreshError } });`.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| }); | ||
| // The notes list is cached for 2 days — invalidate so the new/updated note | ||
| // is visible immediately instead of after cache expiry. | ||
| cacheManager.remove('/Notes/GetAllNotes'); |
There was a problem hiding this comment.
Inline cache key literal — '/Notes/GetAllNotes' is duplicated knowledge shared with the cache write site, so a typo in either location silently breaks cache invalidation. Define a shared constant (e.g., const NOTES_CACHE_KEY = '/Notes/GetAllNotes' in a routes/keys constants file) and reference it both here and wherever the notes list is cached.
Kody rule violation: Centralize string constants
Prompt for LLM
File src/api/notes/notes.ts:
Line 68:
Inline cache key literal — '/Notes/GetAllNotes' is duplicated knowledge shared with the cache write site, so a typo in either location silently breaks cache invalidation. Define a shared constant (e.g., `const NOTES_CACHE_KEY = '/Notes/GetAllNotes'` in a routes/keys constants file) and reference it both here and wherever the notes list is cached.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
|
|
||
| const renderItem = useCallback( | ||
| ({ item }: { item: CallResultData }) => ( | ||
| <Pressable onPress={() => router.push(`/call/${item.CallId}`)}> |
There was a problem hiding this comment.
Inline arrow function in the onPress JSX prop creates a new function on every render, impacting performance. Move the function definition outside the render method.
Kody rule violation: Avoid using .bind() or arrow functions in JSX props
Prompt for LLM
File src/app/(app)/calls.tsx:
Line 84:
Inline arrow function in the onPress JSX prop creates a new function on every render, impacting performance. Move the function definition outside the render method.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| }, [protocols, searchQuery]); | ||
|
|
||
| const renderProtocol = React.useCallback(({ item }: { item: (typeof filteredProtocols)[number] }) => <ProtocolCard protocol={item} onPress={handleProtocolPress} />, [handleProtocolPress]); | ||
| const protocolKeyExtractor = React.useCallback((item: (typeof filteredProtocols)[number], index: number) => item.ProtocolId || `protocol-${index}`, []); |
There was a problem hiding this comment.
Unstable list key — the key extractor falls back to a template literal embedding the array index when ProtocolId is absent, causing incorrect React reconciliation on reorder/insert/delete. Use a guaranteed unique identifier from the data or combine a stable field with another distinguishing property rather than the positional index.
Kody rule violation: Avoid array indexes as keys in React lists
Prompt for LLM
File src/app/(app)/protocols.tsx:
Line 62:
Unstable list key — the key extractor falls back to a template literal embedding the array index when ProtocolId is absent, causing incorrect React reconciliation on reorder/insert/delete. Use a guaranteed unique identifier from the data or combine a stable field with another distinguishing property rather than the positional index.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| case FeedFormat.YouTubeLive: | ||
| case FeedFormat.Embed: | ||
| return safeWebUrl ? ( | ||
| <WebView source={{ uri: safeWebUrl }} style={styles.video} allowsInlineMediaPlayback mediaPlaybackRequiresUserAction={false} javaScriptEnabled originWhitelist={['https://*', 'http://*']} /> |
There was a problem hiding this comment.
Duplicated JSX — the safeWebUrl ? <WebView …> : <Text …> block repeats across the YouTubeLive/Embed, MJPEG/Other, and default switch cases with only minor prop differences, inviting copy-paste drift such as a missing originWhitelist. Extract a helper like renderSecureWebView({ javaScriptEnabled, mediaPlaybackRequiresUserAction }) that renders the WebView with shared props plus originWhitelist or falls back to the Text.
Kody rule violation: Extract duplicated logic into functions
Prompt for LLM
File src/components/call-video-feeds/video-player-modal.tsx:
Line 48:
Duplicated JSX — the `safeWebUrl ? <WebView …> : <Text …>` block repeats across the YouTubeLive/Embed, MJPEG/Other, and default switch cases with only minor prop differences, inviting copy-paste drift such as a missing originWhitelist. Extract a helper like `renderSecureWebView({ javaScriptEnabled, mediaPlaybackRequiresUserAction })` that renders the WebView with shared props plus originWhitelist or falls back to the Text.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| } | ||
| const query = searchQuery.toLowerCase(); | ||
| return { | ||
| users: data.users.filter((user) => user.Name.toLowerCase().includes(query)), |
There was a problem hiding this comment.
Null pointer dereference — user.Name is accessed and .toLowerCase() called without a null guard, throwing TypeError and crashing the modal when Name is undefined/null. Use optional chaining with a fallback (user?.Name?.toLowerCase()?.includes(query)) or pre-filter with user.Name != null.
Kody rule violation: Add null checks before accessing properties
Prompt for LLM
File src/components/calls/dispatch-selection-modal.tsx:
Line 51:
Null pointer dereference — user.Name is accessed and .toLowerCase() called without a null guard, throwing TypeError and crashing the modal when Name is undefined/null. Use optional chaining with a fallback (`user?.Name?.toLowerCase()?.includes(query)`) or pre-filter with `user.Name != null`.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| useEffect(() => { | ||
| const interval = setInterval(() => { | ||
| // Shared ticker: all visible timer cards run on ONE interval. | ||
| return subscribeToSharedTicker(() => { |
There was a problem hiding this comment.
Unhandled subscription error — subscribeToSharedTicker is invoked with only a tick callback and no onError/onFailure path, violating Rule [4]'s requirement that subscription/listener APIs always provide an error handler. Pass an error callback (e.g., subscribeToSharedTicker(onTick, { onError: (e) => logger.error('ticker failed', { err: e }) })) or document why errors are impossible for this ticker.
Kody rule violation: Provide error handlers to subscription/listener APIs
Prompt for LLM
File src/components/check-in-timers/check-in-timer-card.tsx:
Line 34:
Unhandled subscription error — subscribeToSharedTicker is invoked with only a tick callback and no onError/onFailure path, violating Rule [4]'s requirement that subscription/listener APIs always provide an error handler. Pass an error callback (e.g., `subscribeToSharedTicker(onTick, { onError: (e) => logger.error('ticker failed', { err: e }) })`) or document why errors are impossible for this ticker.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| return null; | ||
| } | ||
|
|
||
| const styleId = colorScheme === 'dark' ? 'mapbox/dark-v11' : 'mapbox/streets-v12'; |
There was a problem hiding this comment.
Inline magic strings — the Mapbox style identifiers 'mapbox/dark-v11' and 'mapbox/streets-v12' are duplicated string literals where a typo silently breaks the map. Define a const map such as const MAP_STYLES = { dark: 'mapbox/dark-v11', street: 'mapbox/streets-v12' } as const; and reference it by key.
Kody rule violation: Use enums instead of magic strings
Prompt for LLM
File src/components/maps/static-map.tsx:
Line 36:
Inline magic strings — the Mapbox style identifiers 'mapbox/dark-v11' and 'mapbox/streets-v12' are duplicated string literals where a typo silently breaks the map. Define a const map such as `const MAP_STYLES = { dark: 'mapbox/dark-v11', street: 'mapbox/streets-v12' } as const;` and reference it by key.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| {/* Show user location if requested */} | ||
| {showUserLocation && <Mapbox.UserLocation visible={true} showsUserHeadingIndicator={true} />} | ||
| </Mapbox.MapView> | ||
| <Image source={{ uri: imageUrl }} style={StyleSheet.flatten([styles.map, { height }])} resizeMode="cover" accessibilityLabel={address || 'Map'} /> |
There was a problem hiding this comment.
Hardcoded user-visible string — the fallback accessibility label 'Map' is untranslated despite useTranslation() being available (line 28) and t() used elsewhere (line 55), breaking i18n consistency for screen-reader users. Replace the literal with a translation key, e.g. accessibilityLabel={address || t('map.label')}.
Kody rule violation: Internationalize user-facing text with next-intl or next-i18next
Prompt for LLM
File src/components/maps/static-map.tsx:
Line 63:
Hardcoded user-visible string — the fallback accessibility label 'Map' is untranslated despite useTranslation() being available (line 28) and t() used elsewhere (line 55), breaking i18n consistency for screen-reader users. Replace the literal with a translation key, e.g. `accessibilityLabel={address || t('map.label')}`.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| const URL_PATTERN = /^https?:\/\/.+/; | ||
| // Plain HTTP would send the OAuth2 password grant and bearer tokens in | ||
| // cleartext — only allow it in dev builds (local testing against localhost). | ||
| const URL_PATTERN = __DEV__ ? /^https?:\/\/.+/ : /^https:\/\/.+/; |
There was a problem hiding this comment.
Plaintext HTTP exposure — the dev URL pattern permits arbitrary HTTP endpoints, violating Rule [61] and risking OAuth2 password-grant and bearer token exposure over HTTP since the regex does not restrict dev HTTP to localhost. Require HTTPS in all builds, or limit any dev-only HTTP exception to localhost/127.0.0.1 and enforce TLS 1.2+ on the server.
Kody rule violation: Enforce TLS 1.2+ and HSTS on all external endpoints
Prompt for LLM
File src/components/settings/server-url-bottom-sheet.tsx:
Line 30:
Plaintext HTTP exposure — the dev URL pattern permits arbitrary HTTP endpoints, violating Rule [61] and risking OAuth2 password-grant and bearer token exposure over HTTP since the regex does not restrict dev HTTP to localhost. Require HTTPS in all builds, or limit any dev-only HTTP exception to localhost/127.0.0.1 and enforce TLS 1.2+ on the server.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| // genuine axios connectivity failure (no response received). Plain Errors are | ||
| // treated as server rejections and rethrown instead of queued. | ||
| const createNetworkError = (): Error => { | ||
| const error = new Error('Network Error') as any; |
There was a problem hiding this comment.
Type safety escape hatch — casting the Error to any to attach axios-specific properties defeats TypeScript's compiler checks, allowing typos on isAxiosError, code, and request to go undetected. Define a minimal interface (e.g., interface NetworkError extends Error { isAxiosError: true; code: string; request: unknown }) and cast to that, or use Object.assign(new Error('Network Error'), { isAxiosError: true, code: 'ERR_NETWORK', request: {} }).
Kody rule violation: Use safe type casting with as operator
Prompt for LLM
File src/components/status/__tests__/status-gps-integration-working.test.tsx:
Line 141:
Type safety escape hatch — casting the Error to `any` to attach axios-specific properties defeats TypeScript's compiler checks, allowing typos on isAxiosError, code, and request to go undetected. Define a minimal interface (e.g., `interface NetworkError extends Error { isAxiosError: true; code: string; request: unknown }`) and cast to that, or use `Object.assign(new Error('Network Error'), { isAxiosError: true, code: 'ERR_NETWORK', request: {} })`.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| return null; | ||
| } | ||
|
|
||
| const pendingRelayState = getItem<string>(SAML_RELAY_STATE_KEY); |
There was a problem hiding this comment.
Unhandled Promise from getItem() — the call omits await while sibling setItem (line 49) and removeItem (line 89) are awaited, leaving pendingRelayState holding a Promise rather than the stored nonce string, so the comparison relayState !== pendingRelayState on line 83 is always true and every legitimate callback is rejected; any underlying storage rejection is also unhandled. Change to const pendingRelayState = await getItem<string>(SAML_RELAY_STATE_KEY); so the value resolves and any rejection propagates to the caller's try/catch.
Kody rule violation: Handle async operations with proper error handling
Prompt for LLM
File src/hooks/use-saml-login.ts:
Line 76:
Unhandled Promise from getItem() — the call omits await while sibling setItem (line 49) and removeItem (line 89) are awaited, leaving pendingRelayState holding a Promise rather than the stored nonce string, so the comparison `relayState !== pendingRelayState` on line 83 is always true and every legitimate callback is rejected; any underlying storage rejection is also unhandled. Change to `const pendingRelayState = await getItem<string>(SAML_RELAY_STATE_KEY);` so the value resolves and any rejection propagates to the caller's try/catch.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| * Returns the SAMLResponse when valid, null otherwise. The nonce is consumed | ||
| * on use so a captured callback URL cannot be replayed. | ||
| */ | ||
| async function validateSamlCallback(url: string): Promise<string | null> { |
There was a problem hiding this comment.
Incomplete JSDoc — validateSamlCallback returns Promise<string | null> but its block (lines 61–65) lacks the formal @returns {Promise<string | null>} and @throws tags required by Rule [23]. Add proper JSDoc annotations documenting the resolve value (e.g., @returns {Promise<string | null>} The SAMLResponse string, or null if the callback is invalid.) and noting that storage read/remove failures can cause rejection.
Kody rule violation: Document async/Promise behavior and errors
Prompt for LLM
File src/hooks/use-saml-login.ts:
Line 66:
Incomplete JSDoc — validateSamlCallback returns Promise<string | null> but its block (lines 61–65) lacks the formal @returns {Promise<string | null>} and @throws tags required by Rule [23]. Add proper JSDoc annotations documenting the resolve value (e.g., `@returns {Promise<string | null>} The SAMLResponse string, or null if the callback is invalid.`) and noting that storage read/remove failures can cause rejection.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| const cacheItem: CacheItem<T> = JSON.parse(cached); | ||
| let cacheItem: CacheItem<T>; | ||
| try { | ||
| cacheItem = JSON.parse(cached); |
There was a problem hiding this comment.
Missing shape validation — JSON.parse is wrapped in try/catch for SyntaxError but the parsed object's structure is never verified before accessing cacheItem.timestamp and cacheItem.expiresIn on line 68, so valid JSON like {} yields undefined properties, makes isExpired compute NaN, and treats the entry as non-expired while serving malformed data. After parsing, validate structure: if (!cacheItem || typeof cacheItem.timestamp !== 'number' || typeof cacheItem.expiresIn !== 'number') { storage.delete(key); return null; }.
Kody rule violation: Always validate JSON parsing
Prompt for LLM
File src/lib/cache/cache-manager.ts:
Line 60:
Missing shape validation — JSON.parse is wrapped in try/catch for SyntaxError but the parsed object's structure is never verified before accessing cacheItem.timestamp and cacheItem.expiresIn on line 68, so valid JSON like `{}` yields undefined properties, makes isExpired compute NaN, and treats the entry as non-expired while serving malformed data. After parsing, validate structure: `if (!cacheItem || typeof cacheItem.timestamp !== 'number' || typeof cacheItem.expiresIn !== 'number') { storage.delete(key); return null; }`.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| } catch { | ||
| // A throwing listener must not kill the shared ticker for everyone else. | ||
| } |
There was a problem hiding this comment.
Silently swallowed exceptions — the catch block consumes every listener error with no logging or rethrow, violating Rule [30]'s requirement that exceptions be logged with context or handled explicitly, making misbehaving-subscriber debugging impossible. Log with structured context (e.g., logger.error('shared-ticker listener threw', { err })) or expose an error callback in the subscription API so subscribers can decide how to handle their own failures.
Kody rule violation: Avoid empty catch blocks
Prompt for LLM
File src/lib/shared-ticker.ts:
Line 16 to 18:
Silently swallowed exceptions — the catch block consumes every listener error with no logging or rethrow, violating Rule [30]'s requirement that exceptions be logged with context or handled explicitly, making misbehaving-subscriber debugging impossible. Log with structured context (e.g., logger.error('shared-ticker listener threw', { err })) or expose an error callback in the subscription API so subscribers can decide how to handle their own failures.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| message: `Scheduling reconnection attempt ${currentAttempts}/${this.MAX_RECONNECT_ATTEMPTS} for hub: ${hubName} in ${delay}ms`, | ||
| }); | ||
|
|
||
| setTimeout(async () => { |
There was a problem hiding this comment.
Leaked timer — setTimeout is invoked without storing its timer ID, leaving no deterministic cleanup path to cancel a pending reconnect timer as required by Rule [52], risking orphaned callbacks after teardown or explicit disconnect. Store the timer ID (e.g., this.reconnectTimers.set(hubName, setTimeout(...))) and clearTimeout it in the disconnect/teardown path.
Kody rule violation: Clear timers on teardown/unmount
Prompt for LLM
File src/services/signalr.service.ts:
Line 434:
Leaked timer — setTimeout is invoked without storing its timer ID, leaving no deterministic cleanup path to cancel a pending reconnect timer as required by Rule [52], risking orphaned callbacks after teardown or explicit disconnect. Store the timer ID (e.g., `this.reconnectTimers.set(hubName, setTimeout(...))`) and clearTimeout it in the disconnect/teardown path.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| }); | ||
| }); | ||
|
|
||
| room.on(RoomEvent.Disconnected, () => { |
There was a problem hiding this comment.
Spurious RoomEvent.Disconnected handler invocation during intentional room switches — connectToRoom() awaits currentRoom.disconnect() (line 516) without first clearing isConnected, so the handler's if (!get().isConnected) return guard passes and runs the full unexpected-drop reset, firing callKeepService.endCall(), notifee.stopForegroundService(), and set({ currentRoom: null, isConnected: false }). Set isConnected: false before disconnecting the old room, matching the pattern already used in disconnectFromRoom (line 824).
// Disconnect from current room if connected
if (currentRoom) {
logger.debug({ message: 'connectToRoom: disconnecting existing room' });
// Mark disconnected FIRST so the RoomEvent.Disconnected handler treats
// this as intentional (same pattern as disconnectFromRoom).
set({ isConnected: false });
await currentRoom.disconnect();
}Prompt for LLM
File src/stores/app/livekit-store.ts:
Line 582:
Spurious RoomEvent.Disconnected handler invocation during intentional room switches — connectToRoom() awaits currentRoom.disconnect() (line 516) without first clearing isConnected, so the handler's `if (!get().isConnected) return` guard passes and runs the full unexpected-drop reset, firing callKeepService.endCall(), notifee.stopForegroundService(), and `set({ currentRoom: null, isConnected: false })`. Set `isConnected: false` before disconnecting the old room, matching the pattern already used in disconnectFromRoom (line 824).
Suggested Code:
// Disconnect from current room if connected
if (currentRoom) {
logger.debug({ message: 'connectToRoom: disconnecting existing room' });
// Mark disconnected FIRST so the RoomEvent.Disconnected handler treats
// this as intentional (same pattern as disconnectFromRoom).
set({ isConnected: false });
await currentRoom.disconnect();
}
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| clearTimeout(existingTimeoutId); | ||
| } | ||
| // Retry after 30 seconds for transient errors | ||
| const retryTimeoutId = setTimeout(() => get().refreshAccessToken(), 30000); |
There was a problem hiding this comment.
Magic number — the literal 30000 inlined in setTimeout obscures intent and is inconsistent with the existing refreshDelayMs convention used for the proactive-refresh delay. Extract a named constant such as const REFRESH_RETRY_DELAY_MS = 30_000; (or reuse a shared time-constant utility) at the top of the module or in a constants file and use it in the setTimeout call.
Kody rule violation: Replace magic numbers with named constants
Prompt for LLM
File src/stores/auth/store.tsx:
Line 253:
Magic number — the literal 30000 inlined in setTimeout obscures intent and is inconsistent with the existing refreshDelayMs convention used for the proactive-refresh delay. Extract a named constant such as `const REFRESH_RETRY_DELAY_MS = 30_000;` (or reuse a shared time-constant utility) at the top of the module or in a constants file and use it in the setTimeout call.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
Code Review Completed! 🔥The code review was successfully completed based on your current configurations. Kody Guide: Usage and ConfigurationInteracting with Kody
Current Kody ConfigurationReview OptionsThe following review options are enabled or disabled:
|
|
|
||
| beforeEach(() => { | ||
| jest.clearAllMocks(); | ||
| mockDispatchStore.data.users[0].Name = 'John Doe'; |
There was a problem hiding this comment.
Unguarded property access mockDispatchStore.data.users[0].Name risks a TypeError if data or users is null/undefined at runtime. Use optional chaining or assert the mock structure beforehand.
Kody rule violation: Add null checks before accessing properties
Prompt for LLM
File src/components/calls/__tests__/dispatch-selection-modal.test.tsx:
Line 175:
Unguarded property access `mockDispatchStore.data.users[0].Name` risks a TypeError if `data` or `users` is null/undefined at runtime. Use optional chaining or assert the mock structure beforehand.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| }); | ||
|
|
||
| it.each([undefined, null])('should not crash while filtering when a user name is %s', (missingName) => { | ||
| mockDispatchStore.data.users[0].Name = missingName as unknown as string; |
There was a problem hiding this comment.
Double cast missingName as unknown as string bypasses TypeScript type safety by forcing an explicitly undefined or null value to type string. Widen the mock type (e.g., Name?: string | null) and assign directly without the unsafe cast.
Kody rule violation: Use safe type casting with as operator
Prompt for LLM
File src/components/calls/__tests__/dispatch-selection-modal.test.tsx:
Line 232:
Double cast `missingName as unknown as string` bypasses TypeScript type safety by forcing an explicitly undefined or null value to type `string`. Widen the mock type (e.g., `Name?: string | null`) and assign directly without the unsafe cast.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| return null; | ||
| } | ||
|
|
||
| const pendingRelayState = await getItem<string>(SAML_RELAY_STATE_KEY); |
There was a problem hiding this comment.
Unhandled promise rejection risk: await getItem(...) lacks a surrounding try/catch or .catch, and a storage read can reject due to quota, serialization, or transport errors. Wrap the call in try/catch and handle the failure explicitly, or ensure the enclosing function already guards this call.
Kody rule violation: Handle async operations with proper error handling
Prompt for LLM
File src/hooks/use-saml-login.ts:
Line 76:
Unhandled promise rejection risk: `await getItem(...)` lacks a surrounding try/catch or `.catch`, and a storage read can reject due to quota, serialization, or transport errors. Wrap the call in try/catch and handle the failure explicitly, or ensure the enclosing function already guards this call.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| listeners.add(listener); | ||
|
|
||
| if (!interval) { | ||
| interval = setInterval(tick, 1000); |
There was a problem hiding this comment.
Magic number 1000 obscures the tick interval's intent and is error-prone if the duration needs to change. Extract a named constant such as const TICK_INTERVAL_MS = 1000; at the top of the module.
Kody rule violation: Replace magic numbers with named constants
Prompt for LLM
File src/lib/shared-ticker.ts:
Line 36:
Magic number `1000` obscures the tick interval's intent and is error-prone if the duration needs to change. Extract a named constant such as `const TICK_INTERVAL_MS = 1000;` at the top of the module.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| const service = SignalRService.getInstance(); | ||
|
|
||
| await service.connectToHubWithEventingUrl(mockConfig); | ||
| const onCloseCallback = mockConnection.onclose.mock.calls[0][0]; |
There was a problem hiding this comment.
Accessing mock.calls[0][0] without verifying the array is non-empty throws a cryptic TypeError if onclose was never invoked. Add a length assertion or use optional chaining before indexing.
Kody rule violation: Check query results before accessing indices
Prompt for LLM
File src/services/__tests__/signalr.service.enhanced.test.ts:
Line 348:
Accessing `mock.calls[0][0]` without verifying the array is non-empty throws a cryptic TypeError if `onclose` was never invoked. Add a length assertion or use optional chaining before indexing.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
|
Approve |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/app/login/sso.tsx (1)
98-113: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winHandle failed callback validation and login.
validateSamlCallback()performs storage I/O andssoLogin()is awaited; either rejection becomes an unhandled promise rejection here. Wrap the handler body intry/catch, log the failure, and resetisSsoLoadingbefore showing the error modal.As per coding guidelines, “All async operations must have proper try/catch with logging.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/login/sso.tsx` around lines 98 - 113, Wrap the async URL handler registered by Linking.addEventListener in try/catch so rejections from validateSamlCallback and ssoLogin are handled. In the catch block, log the failure, reset isSsoLoading, and show the error modal; preserve the existing invalid-callback handling and successful ssoLogin flow.Source: Coding guidelines
src/lib/logging/index.tsx (2)
159-174: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winSentry never receives a real exception anymore —
err instanceof Errorcan no longer be true.
sanitizeLogContextnow converts realErrorinstances into plain{name, message, stack}objects (line 52-54) and axios-like errors into plain summary objects (line 37-46) beforesanitized.erroris checked at line 168. Since both paths strip theErrorprototype,err instanceof Erroris alwaysfalseafter this change, soSentry.captureExceptionis now dead code for every call site that logscontext: { error }— every error report falls through toSentry.captureMessage, losing stack traces and exception-based grouping in Sentry across the app.🐛 Capture the raw error, not the sanitized copy
public error(entry: LogEntry): void { this.log('error', entry); if (!isJest) { + 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 } }); + if (rawError instanceof Error) { + Sentry.captureException(rawError, { extra: { message: entry.message, ...sanitized } }); } else { Sentry.captureMessage(entry.message, { level: 'error', extra: sanitized }); } } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/logging/index.tsx` around lines 159 - 174, Update the error handling in Logger.error so Sentry.captureException receives the raw Error from entry.context before sanitizeLogContext converts it; use the sanitized context only for Sentry metadata and retain captureMessage for non-Error values.
48-60: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winArrays bypass sanitization entirely.
The recursion guard excludes arrays (
!Array.isArray(value)), sosanitizeValuenever processes array elements — any array containing objects with sensitive fields (tokens, emails, credentials) is logged/reported completely unredacted.🔒 Recurse into arrays too
const sanitizeValue = (key: string, value: unknown, depth: number): unknown => { if (isSensitiveKey(key)) return '[REDACTED]'; if (isAxiosErrorLike(value)) return summarizeAxiosError(value); if (value instanceof Error) { return { name: value.name, message: value.message, stack: value.stack }; } - if (depth > 0 && value !== null && typeof value === 'object' && !Array.isArray(value)) { + if (depth > 0 && Array.isArray(value)) { + return value.map((item) => sanitizeValue(key, item, depth - 1)); + } + if (depth > 0 && value !== null && typeof value === 'object') { return sanitizeObject(value as Record<string, unknown>, depth - 1); } if (typeof value === 'function' || typeof value === 'symbol') return String(value); return value; };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/logging/index.tsx` around lines 48 - 60, Update sanitizeValue to recurse into arrays when depth > 0, sanitizing each element through the existing sanitization logic so nested sensitive fields are redacted. Preserve the current object recursion, depth limit, and primitive/function/symbol handling.src/stores/signalr/signalr-store.ts (1)
281-303: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winRestore the geolocation connected flag on reconnect.
connectGeolocationHubonly setsisGeolocationHubConnectedviaonGeolocationConnect, but the SignalR service emits__hubReconnected:${name}after automatic reconnects. Subscribe to the hub-scoped reconnected lifecycle event and clear it on disconnect so the flag can recover without relying on a callback replay.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/stores/signalr/signalr-store.ts` around lines 281 - 303, Update connectGeolocationHub to subscribe to the hub-scoped __hubReconnected:${Env.REALTIME_GEO_HUB_NAME} lifecycle event and set isGeolocationHubConnected to true when it fires. Keep the existing geoHubDisconnected listener setting the flag to false, and register the reconnect listener alongside the other geolocation hub listeners.
🧹 Nitpick comments (13)
src/lib/utils.ts (1)
12-29: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winNumeric epoch input silently produces
nullinparseApiUtcDate.For
typeof input === 'number',String(input)+ appended'Z'(e.g."1690000000000Z") is not parseable bynew Date(), so a valid epoch timestamp would incorrectly resolve tonull/'Unknown'instead of the correct date. Current callers only pass strings, so this isn't hit today, but the function'sunknownsignature invites future numeric callers to hit this silently.🛠️ Proposed fix: handle numeric input directly
if (input instanceof Date) { return Number.isNaN(input.getTime()) ? null : input; } + + if (typeof input === 'number') { + return Number.isNaN(input) || !Number.isFinite(input) ? null : new Date(input); + } let value = String(input).trim();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/utils.ts` around lines 12 - 29, Update parseApiUtcDate to handle numeric input before converting values to strings: treat finite numbers as epoch timestamps and construct the Date directly from the number, while returning null for invalid numeric values. Preserve the existing Date and string parsing behavior.src/stores/signalr/signalr-store.ts (2)
49-50: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winType
lastUpdateTimestampsby event key.
Record<string, number>loses theUpdateHubEventunion you just introduced, so typos in consumer lookups (e.g.state.lastUpdateTimestamps[event]insrc/hooks/use-map-signalr-updates.tsLine 24) compile silently.Partial<Record<UpdateHubEvent, number>>keeps the?? 0fallbacks working while catching bad keys.♻️ Proposed change
/** Per-event timestamps — consumers should subscribe to the events they care about. */ - lastUpdateTimestamps: Record<string, number>; + lastUpdateTimestamps: Partial<Record<UpdateHubEvent, number>>;The reconnect resync at Line 153 would then use the same type:
- const timestamps: Record<string, number> = {}; + const timestamps: Partial<Record<UpdateHubEvent, number>> = {};🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/stores/signalr/signalr-store.ts` around lines 49 - 50, Update the lastUpdateTimestamps property in the signalr store state to use Partial<Record<UpdateHubEvent, number>> instead of Record<string, number>, preserving undefined values for existing ?? 0 fallbacks and propagating the same event-keyed type through the reconnect resync logic.
54-56: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
lastGeolocationMessage/lastGeolocationTimestampare now dead state.With the no-op listeners at Lines 298-299 nothing ever writes these fields; they stay
null/0and are only cleared again indisconnectGeolocationHub. Keeping them inSignalRStateadvertises a contract the store no longer fulfills. Either drop them from the interface and the twosetcalls, or mark them@deprecatedlike the update-hub aggregates at Lines 45-48.Also applies to: 293-299
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/stores/signalr/signalr-store.ts` around lines 54 - 56, Remove lastGeolocationMessage and lastGeolocationTimestamp from SignalRState, along with their corresponding set calls in the geolocation listener/disconnect logic around disconnectGeolocationHub; keep isGeolocationHubConnected and the no-op listeners unchanged.src/hooks/use-saml-login.ts (1)
44-59: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valuePending nonce has no expiry or abandonment cleanup.
If the user backs out of the browser,
saml_pending_relay_statestays in storage forever and is accepted by a later callback. Not exploitable (an attacker still needs the exact UUID), but storing{ nonce, createdAt }and rejecting entries older than a few minutes invalidateSamlCallbacktightens the window.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/hooks/use-saml-login.ts` around lines 44 - 59, Update the SAML relay-state flow around startSamlLogin and validateSamlCallback to persist the nonce together with a creation timestamp, reject and clear stored entries older than a few minutes during callback validation, and remove the pending entry when the browser flow is abandoned or otherwise cannot complete.src/hooks/use-status-signalr-updates.ts (1)
35-56: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueTrailing-only debounce can starve under a sustained event stream.
The timer resets on every new message, so status events arriving continuously less than
DEBOUNCE_DELAYapart would never trigger a refresh. Filtering onmessage.UnitId === activeUnitIdmakes that unlikely in practice, but a max-wait (fire if the first pending update is older than, say, 5s) would guarantee progress.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/hooks/use-status-signalr-updates.ts` around lines 35 - 56, Update the debounce logic in the SignalR status-update handler so continuous events cannot indefinitely postpone refreshActiveUnitStatus. Preserve the trailing debounce while adding a maximum wait from the first pending update (for example, 5 seconds), triggering the refresh when that limit is reached and resetting the max-wait state after execution.src/stores/signalr/__tests__/signalr-store.test.ts (1)
332-335: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFlushing the fire-and-forget reconnect handler via
setTimeout(0)is fragile.The
__hubReconnectedhandler isvoid (async () => …)()in the store; a single macrotask tick happens to flush it today, but adding anotherawaitinside the handler would silently break this test. Consider having the handler expose its promise, or drain with a small loop of microtask flushes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/stores/signalr/__tests__/signalr-store.test.ts` around lines 332 - 335, Replace the single setTimeout(0) wait in the __hubReconnected:eventingHub test with a reliable drain of the fire-and-forget handler’s asynchronous work, preferably by exposing and awaiting its promise; otherwise use a bounded loop of microtask flushes. Keep the existing reconnect assertions and handler behavior unchanged.src/hooks/__tests__/use-map-signalr-updates.test.ts (1)
20-29: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a negative case for non-map-relevant events.
The whole point of the new selector is that events like
weatherAlertReceivedmust not trigger a marker refetch, but every test only seedscallsUpdated. A test that setslastUpdateTimestamps: { weatherAlertReceived: t }and assertsgetMapDataAndMarkersis never called would lock in the filtering behavior.♻️ Suggested helper + test
const stateWithUpdate = (timestamp: number) => ({ lastUpdateTimestamps: timestamp > 0 ? { callsUpdated: timestamp } : {}, }); +const mockStoreWithEvent = (event: string, timestamp: number) => { + const state = { lastUpdateTimestamps: { [event]: timestamp } }; + mockUseSignalRStore.mockImplementation((selector: any) => (typeof selector === 'function' ? selector(state) : state)); +};🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/hooks/__tests__/use-map-signalr-updates.test.ts` around lines 20 - 29, Add a negative test for the selector used by useMapSignalRUpdates that seeds lastUpdateTimestamps with weatherAlertReceived instead of callsUpdated, then verifies getMapDataAndMarkers is never called. Extend or reuse stateWithUpdate/mockStoreWithTimestamp only as needed to represent this non-map-relevant event while preserving existing map-update tests.src/stores/app/__tests__/livekit-store-room-switch.test.ts (1)
1-32: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePlace native-module mocks above the imports.
jest.mockcalls are hoisted so behavior is correct, but the repo convention is mocks first.As per coding guidelines: "Mock native modules at the top of test files before imports".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/stores/app/__tests__/livekit-store-room-switch.test.ts` around lines 1 - 32, Move the native-module jest.mock declarations for `@livekit/react-native`, `@livekit/react-native-webrtc`, and `@notifee/react-native` above all imports in the test file, keeping their existing mock implementations unchanged.Source: Coding guidelines
src/services/offline-event-manager.service.ts (1)
233-241: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueRename
MAX_CONCURRENT_EVENTS— it is now a per-batch cap, and it bounds drain rate.With sequential processing the constant no longer describes concurrency, and 3 events per 10s tick means a large offline backlog drains slowly (100 queued events ≈ 5.5 minutes of connectivity). Consider
MAX_EVENTS_PER_BATCHand a larger batch size, since ordering is now guaranteed by the awaited loop rather than by the cap.♻️ Proposed rename
- const eventsToProcess = [...pendingEvents].sort((a, b) => a.createdAt - b.createdAt).slice(0, this.MAX_CONCURRENT_EVENTS); + const eventsToProcess = [...pendingEvents].sort((a, b) => a.createdAt - b.createdAt).slice(0, this.MAX_EVENTS_PER_BATCH);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/services/offline-event-manager.service.ts` around lines 233 - 241, Rename MAX_CONCURRENT_EVENTS to a per-batch limit such as MAX_EVENTS_PER_BATCH throughout OfflineEventManagerService, including the slice used to build eventsToProcess. Increase the batch limit so sequential processing drains backlogs faster, while preserving creation-order sorting and the awaited processEvent loop.src/services/__tests__/signalr.service.enhanced.test.ts (1)
317-317: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTests assert on private internals via
as any.
reconnectTimers/hubConfigsare private; these assertions break on any internal rename and bypass the "noany" rule. Consider asserting observable behavior instead (e.g. noconnectToHubWithEventingUrl/refreshAccessTokencall after advancing timers, which lines 329/361 already cover), or expose a small test-only accessor.As per coding guidelines: "Never use
anytype; use precise types and interfaces with TypeScript strict mode enabled".Also applies to: 344-362
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/services/__tests__/signalr.service.enhanced.test.ts` at line 317, Replace the `(service as any).reconnectTimers` and `(service as any).hubConfigs` assertions in the reconnect timer tests with observable behavior, reusing the existing `connectToHubWithEventingUrl` and `refreshAccessToken` expectations after advancing timers. Remove all `any` casts while preserving coverage of the timer and configuration behavior.Source: Coding guidelines
src/services/__tests__/location.test.ts (1)
631-645: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTests mutate shared fixtures without teardown, leaking state into later tests in the same file. Both cases overwrite module-level mock state inside a test body and rely on the test running to completion (or not at all) to restore it; a failing assertion leaves the fixture corrupted for every subsequent test.
src/services/__tests__/location.test.ts#L631-L645: move themockCoreStoreState.activeUnitId = 'unit-123'restore out of the test body and into anafterEach(or re-assign it in the sharedbeforeEach).src/services/__tests__/signalr.service.test.ts#L946-L984: resetmockRefreshAccessToken's implementation andmockGetState's return value inbeforeEach/afterEachso theaccessToken: nullstate does not persist past this test.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/services/__tests__/location.test.ts` around lines 631 - 645, The tests leak shared mock state when assertions fail. In src/services/__tests__/location.test.ts lines 631-645, move restoration of mockCoreStoreState.activeUnitId to the shared beforeEach or afterEach, removing reliance on the test body. In src/services/__tests__/signalr.service.test.ts lines 946-984, reset mockRefreshAccessToken’s implementation and mockGetState’s return value in beforeEach or afterEach so the accessToken: null setup is isolated to that test.src/stores/check-in-timers/store.ts (1)
114-133: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider triggering an immediate refetch when returning to foreground.
Backgrounding the poll is a good battery fix, but after resuming the app can show up to
intervalMs(30s) of stale check-in timer data until the next scheduled tick. Subscribing toAppStatechanges and callingfetchTimerStatuses(callId)immediately on transition to'active'(mirroring the pattern inlocation.ts) would tighten that window for this safety-relevant countdown.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/stores/check-in-timers/store.ts` around lines 114 - 133, Update startPolling to subscribe to AppState changes and immediately call fetchTimerStatuses(callId) when transitioning to the active state, matching the existing location.ts pattern. Store or clean up the listener alongside the polling interval so repeated startPolling calls do not create duplicate foreground handlers.src/stores/status/store.ts (1)
244-321: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winServer-rejected status saves are now logged (and reported to Sentry) twice.
The inner catch logs the rejection and rethrows when
!isNetworkError(error); that rethrow is caught again by the outer try/catch, which logs the same error a second time and re-sets the identical error state before rethrowing again. Sincelogger.error()auto-reports to Sentry, every server-rejected status save now produces two Sentry events for one failure. Consider letting the inner handler set state without throwing (return instead), or having the outer catch skip re-logging when the error already has its state set, so each real failure is reported once.As per coding guidelines,
**/*.{ts,tsx}: "logger.error()automatically reports to Sentry; sensitive keys are redacted from context" — duplicatelogger.error()calls for one failure double the Sentry noise for this path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/stores/status/store.ts` around lines 244 - 321, Prevent server-rejected status saves from being logged twice: in the non-network-error branch of the inner handler, preserve the existing logger.error call and error state update, then return without rethrowing into the outer catch. Keep network failures on the existing offline-queue path and preserve the outer catch for errors not already handled.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/app/_layout.tsx`:
- Around line 4-6: Move the side-effect import for the app reset registration to
the side-effect import group in the module’s import section, keeping it before
all value imports. Preserve the existing registration behavior and ordering
required by simple-import-sort.
In `@src/app/call/__tests__/`[id].test.tsx:
- Around line 1362-1374: Update the test render in
src/app/call/__tests__/[id].test.tsx at lines 1362-1374 to destructure unmount
from render and call it after the assertions. Apply the same cleanup in
src/components/calls/__tests__/dispatch-selection-modal.test.tsx at lines
235-237 by destructuring unmount and invoking it after assertions.
- Around line 285-290: Replace the any type in the mocked FullScreenMap
component with a small props interface containing the required isOpen field and
an appropriate type for forwarded props, keeping the existing mock rendering
behavior unchanged.
In `@src/app/routes/stop/`[id].tsx:
- Around line 218-222: Normalize PlannedArrival and PlannedDeparture to an
explicit UTC timestamp contract before passing them to safeFormatDate, and
update the corresponding raw rendering in stop-card.tsx and new Date() usage in
routes/start.tsx to use the same parsing. Preserve the existing display behavior
while ensuring timezone-less values are not interpreted as local time.
In `@src/components/check-in-timers/__tests__/check-in-timer-card.test.tsx`:
- Around line 4-8: Reorder the imports in the check-in timer card test so the
relative value import of CheckInTimerCard appears before the type-only
CheckInTimerStatusResultData import, preserving the existing side-effect,
external, and alias import groups.
In `@src/components/maps/static-map.tsx`:
- Around line 31-34: Update the coordinate validation in the imageUrl useMemo
callback to accept valid zero-valued latitude and longitude coordinates. Replace
the truthiness check with finite-number validation, returning null only when
either coordinate is not finite.
In `@src/components/status/__tests__/status-bottom-sheet.test.tsx`:
- Around line 200-203: Replace the any-typed mockStore in the location-store
Jest mock with a minimal location-state interface and a generic selector
function type, and type getState() to return that state. Apply the same precise
typing to the related mock setup around the additional referenced section,
preserving selector and getState contract checks without using any.
In `@src/components/status/__tests__/status-gps-integration.test.tsx`:
- Around line 46-55: Replace the any-cast network-error fixtures in
createNetworkError and its counterpart in
src/components/status/__tests__/status-gps-integration.test.tsx (lines 46-55)
and src/components/status/__tests__/status-gps-integration-working.test.tsx
(lines 137-146) with a minimal precisely typed Axios-like error interface or
typed Object.assign, preserving the isAxiosError, code, and request properties.
In `@src/hooks/__tests__/use-saml-login.test.ts`:
- Around line 176-192: Update the storage mocks in the handleDeepLink tests to
match the synchronous getItem contract: replace the rejected-promise mock with a
synchronous mockImplementation that throws for the read-failure case, and
replace the resolved-value mock at the other referenced test with the
appropriate synchronous mockReturnValue. Keep the existing assertions and hook
behavior unchanged.
In `@src/lib/logging/index.tsx`:
- Around line 126-137: Update the private log method to sanitize the merged
logging context before passing it to this.logger[level]. Reuse the existing
sanitizeLogContext helper, ensuring globalContext, context, operation, and
trace_id are all covered while preserving the current severity filtering and
timestamp behavior.
In `@src/services/push-notification.ts`:
- Around line 477-502: Extend unregisterFromPushNotifications to invalidate the
device’s push token server-side during logout, adding or calling the backend
unregister-device operation before or alongside the existing local cleanup.
Ensure the request identifies the current device/token and completes or safely
handles failure without skipping local state clearing, then update the method’s
comment to reflect the implemented behavior.
In `@src/services/signalr.service.ts`:
- Around line 453-535: Update the reconnect callback around reconnectTimers and
connectToHubWithEventingUrl to invalidate in-flight reconnects when
disconnectFromHub, disconnectAll, or resetInstance tears down a hub. Add a
per-hub generation/epoch (or equivalent disconnected marker), capture it when
scheduling the callback, and verify it after refreshAccessToken and before
reconnecting or applying success state; abort when the generation changed or hub
configuration was removed. Ensure teardown increments or marks the generation so
the callback cannot recreate configuration or reopen a connection.
- Around line 445-446: Update the reconnect delay calculation in the SignalR
service to use true exponential backoff: base RECONNECT_INTERVAL multiplied by 2
to the power of currentAttempts minus one, still capped at 60000 milliseconds.
Keep the existing attempt sequence and cap behavior consistent with the comment.
In `@src/stores/app/__tests__/livekit-store-room-switch.test.ts`:
- Around line 86-89: Update the test setup in livekit-store-room-switch.test.ts
so the stopForegroundService assertion exercises the Android-specific branch by
setting Platform.OS to "android" for that case. Preserve the existing
intentional-switch guard coverage and ensure the assertion would fail if
foreground-service teardown were incorrectly triggered.
In `@src/stores/app/livekit-store.ts`:
- Around line 585-592: Update the RoomEvent.Disconnected handler to verify that
the event’s room is still the store’s active/current room before resetting call
state. Keep the existing isConnected guard, but add a room-identity check using
the room instance captured by this handler and the store’s current room
reference, returning early for stale rooms so currentRoom, CallKeep, and
foreground-service state remain intact.
In `@src/stores/auth/store.tsx`:
- Around line 339-350: Update the auth store rehydration flow around partialize
and refreshAccessToken so a persisted, non-expired accessToken immediately
restores status to signedIn, using refreshTokenExpiresOn for expiry validation.
Set status to loading only when rehydration has a refresh token without a usable
access token, while preserving the existing refresh retry behavior for transient
failures.
In `@src/stores/signalr/__tests__/zz-dbg.test.ts`:
- Around line 1-48: Delete the scratch test artifact zz-dbg.test.ts entirely;
its console logging, duplicated mocks, and assertion-free department-join
scenario are already covered by signalr-store.test.ts.
---
Outside diff comments:
In `@src/app/login/sso.tsx`:
- Around line 98-113: Wrap the async URL handler registered by
Linking.addEventListener in try/catch so rejections from validateSamlCallback
and ssoLogin are handled. In the catch block, log the failure, reset
isSsoLoading, and show the error modal; preserve the existing invalid-callback
handling and successful ssoLogin flow.
In `@src/lib/logging/index.tsx`:
- Around line 159-174: Update the error handling in Logger.error so
Sentry.captureException receives the raw Error from entry.context before
sanitizeLogContext converts it; use the sanitized context only for Sentry
metadata and retain captureMessage for non-Error values.
- Around line 48-60: Update sanitizeValue to recurse into arrays when depth > 0,
sanitizing each element through the existing sanitization logic so nested
sensitive fields are redacted. Preserve the current object recursion, depth
limit, and primitive/function/symbol handling.
In `@src/stores/signalr/signalr-store.ts`:
- Around line 281-303: Update connectGeolocationHub to subscribe to the
hub-scoped __hubReconnected:${Env.REALTIME_GEO_HUB_NAME} lifecycle event and set
isGeolocationHubConnected to true when it fires. Keep the existing
geoHubDisconnected listener setting the flag to false, and register the
reconnect listener alongside the other geolocation hub listeners.
---
Nitpick comments:
In `@src/hooks/__tests__/use-map-signalr-updates.test.ts`:
- Around line 20-29: Add a negative test for the selector used by
useMapSignalRUpdates that seeds lastUpdateTimestamps with weatherAlertReceived
instead of callsUpdated, then verifies getMapDataAndMarkers is never called.
Extend or reuse stateWithUpdate/mockStoreWithTimestamp only as needed to
represent this non-map-relevant event while preserving existing map-update
tests.
In `@src/hooks/use-saml-login.ts`:
- Around line 44-59: Update the SAML relay-state flow around startSamlLogin and
validateSamlCallback to persist the nonce together with a creation timestamp,
reject and clear stored entries older than a few minutes during callback
validation, and remove the pending entry when the browser flow is abandoned or
otherwise cannot complete.
In `@src/hooks/use-status-signalr-updates.ts`:
- Around line 35-56: Update the debounce logic in the SignalR status-update
handler so continuous events cannot indefinitely postpone
refreshActiveUnitStatus. Preserve the trailing debounce while adding a maximum
wait from the first pending update (for example, 5 seconds), triggering the
refresh when that limit is reached and resetting the max-wait state after
execution.
In `@src/lib/utils.ts`:
- Around line 12-29: Update parseApiUtcDate to handle numeric input before
converting values to strings: treat finite numbers as epoch timestamps and
construct the Date directly from the number, while returning null for invalid
numeric values. Preserve the existing Date and string parsing behavior.
In `@src/services/__tests__/location.test.ts`:
- Around line 631-645: The tests leak shared mock state when assertions fail. In
src/services/__tests__/location.test.ts lines 631-645, move restoration of
mockCoreStoreState.activeUnitId to the shared beforeEach or afterEach, removing
reliance on the test body. In src/services/__tests__/signalr.service.test.ts
lines 946-984, reset mockRefreshAccessToken’s implementation and mockGetState’s
return value in beforeEach or afterEach so the accessToken: null setup is
isolated to that test.
In `@src/services/__tests__/signalr.service.enhanced.test.ts`:
- Line 317: Replace the `(service as any).reconnectTimers` and `(service as
any).hubConfigs` assertions in the reconnect timer tests with observable
behavior, reusing the existing `connectToHubWithEventingUrl` and
`refreshAccessToken` expectations after advancing timers. Remove all `any` casts
while preserving coverage of the timer and configuration behavior.
In `@src/services/offline-event-manager.service.ts`:
- Around line 233-241: Rename MAX_CONCURRENT_EVENTS to a per-batch limit such as
MAX_EVENTS_PER_BATCH throughout OfflineEventManagerService, including the slice
used to build eventsToProcess. Increase the batch limit so sequential processing
drains backlogs faster, while preserving creation-order sorting and the awaited
processEvent loop.
In `@src/stores/app/__tests__/livekit-store-room-switch.test.ts`:
- Around line 1-32: Move the native-module jest.mock declarations for
`@livekit/react-native`, `@livekit/react-native-webrtc`, and `@notifee/react-native`
above all imports in the test file, keeping their existing mock implementations
unchanged.
In `@src/stores/check-in-timers/store.ts`:
- Around line 114-133: Update startPolling to subscribe to AppState changes and
immediately call fetchTimerStatuses(callId) when transitioning to the active
state, matching the existing location.ts pattern. Store or clean up the listener
alongside the polling interval so repeated startPolling calls do not create
duplicate foreground handlers.
In `@src/stores/signalr/__tests__/signalr-store.test.ts`:
- Around line 332-335: Replace the single setTimeout(0) wait in the
__hubReconnected:eventingHub test with a reliable drain of the fire-and-forget
handler’s asynchronous work, preferably by exposing and awaiting its promise;
otherwise use a bounded loop of microtask flushes. Keep the existing reconnect
assertions and handler behavior unchanged.
In `@src/stores/signalr/signalr-store.ts`:
- Around line 49-50: Update the lastUpdateTimestamps property in the signalr
store state to use Partial<Record<UpdateHubEvent, number>> instead of
Record<string, number>, preserving undefined values for existing ?? 0 fallbacks
and propagating the same event-keyed type through the reconnect resync logic.
- Around line 54-56: Remove lastGeolocationMessage and lastGeolocationTimestamp
from SignalRState, along with their corresponding set calls in the geolocation
listener/disconnect logic around disconnectGeolocationHub; keep
isGeolocationHubConnected and the no-op listeners unchanged.
In `@src/stores/status/store.ts`:
- Around line 244-321: Prevent server-rejected status saves from being logged
twice: in the non-network-error branch of the inner handler, preserve the
existing logger.error call and error state update, then return without
rethrowing into the outer catch. Keep network failures on the existing
offline-queue path and preserve the outer catch for errors not already handled.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 6b03adec-cb8f-4ba0-a539-2c2502423d7a
📒 Files selected for processing (88)
app.config.tselectron/main.jssrc/api/common/__tests__/client.test.tssrc/api/common/client.tsxsrc/api/notes/notes.tssrc/app/(app)/__tests__/index.test.tsxsrc/app/(app)/__tests__/protocols.test.tsxsrc/app/(app)/_layout.tsxsrc/app/(app)/calls.tsxsrc/app/(app)/contacts.tsxsrc/app/(app)/index.tsxsrc/app/(app)/notes.tsxsrc/app/(app)/protocols.tsxsrc/app/(app)/settings.tsxsrc/app/(app)/weather-alerts.tsxsrc/app/_layout.tsxsrc/app/call/[id].tsxsrc/app/call/[id]/edit.tsxsrc/app/call/__tests__/[id].test.tsxsrc/app/call/new/index.tsxsrc/app/login/sso.tsxsrc/app/routes/active.tsxsrc/app/routes/stop/[id].tsxsrc/components/call-video-feeds/video-player-modal.tsxsrc/components/calls/__tests__/dispatch-selection-modal.test.tsxsrc/components/calls/call-card.tsxsrc/components/calls/call-images-modal.tsxsrc/components/calls/dispatch-selection-modal.tsxsrc/components/check-in-timers/__tests__/check-in-timer-card.test.tsxsrc/components/check-in-timers/check-in-timer-card.tsxsrc/components/contacts/contact-card.tsxsrc/components/maps/__tests__/full-screen-map.test.tsxsrc/components/maps/full-screen-map.tsxsrc/components/maps/static-map.tsxsrc/components/notes/note-card.tsxsrc/components/settings/server-url-bottom-sheet.tsxsrc/components/status/__tests__/status-bottom-sheet.test.tsxsrc/components/status/__tests__/status-gps-integration-working.test.tsxsrc/components/status/__tests__/status-gps-integration.test.tsxsrc/components/status/status-bottom-sheet.tsxsrc/components/ui/html-renderer/index.web.tsxsrc/components/ui/utils.tsxsrc/features/livekit-call/store/useLiveKitCallStore.tssrc/hooks/__tests__/use-map-signalr-updates.test.tssrc/hooks/__tests__/use-saml-login.test.tssrc/hooks/__tests__/use-status-signalr-updates.test.tsxsrc/hooks/use-map-signalr-updates.tssrc/hooks/use-saml-login.tssrc/hooks/use-status-signalr-updates.tssrc/lib/__tests__/protocol-utils.test.tssrc/lib/__tests__/shared-ticker.test.tssrc/lib/auth/api.tsxsrc/lib/auth/refresh-lock.tssrc/lib/auth/session-cleanup.tssrc/lib/auth/types.tsxsrc/lib/cache/__tests__/cache-manager.test.tssrc/lib/cache/cache-manager.tssrc/lib/logging/index.tsxsrc/lib/logging/types.tsxsrc/lib/protocol-utils.tssrc/lib/shared-ticker.tssrc/lib/storage/index.tsxsrc/lib/utils.tssrc/services/__tests__/app-reset.service.test.tssrc/services/__tests__/location-foreground-permissions.test.tssrc/services/__tests__/location.test.tssrc/services/__tests__/signalr.service.enhanced.test.tssrc/services/__tests__/signalr.service.test.tssrc/services/app-reset.service.tssrc/services/check-in-notification.service.tssrc/services/location.tssrc/services/offline-event-manager.service.tssrc/services/push-notification.tssrc/services/signalr.service.tssrc/stores/app/__tests__/livekit-store-room-switch.test.tssrc/stores/app/core-store.tssrc/stores/app/livekit-store.tssrc/stores/app/server-url-store.tssrc/stores/auth/store.tsxsrc/stores/calls/detail-store.tssrc/stores/calls/store.tssrc/stores/check-in-timers/store.tssrc/stores/offline-queue/store.tssrc/stores/signalr/__tests__/signalr-store.test.tssrc/stores/signalr/__tests__/zz-dbg.test.tssrc/stores/signalr/signalr-store.tssrc/stores/status/__tests__/store.test.tssrc/stores/status/store.ts
💤 Files with no reviewable changes (3)
- src/app/call/new/index.tsx
- src/components/ui/utils.tsx
- src/app/call/[id]/edit.tsx
| // Side-effect import: registers the full app-data wipe as the session-cleanup | ||
| // handler for every logout path (manual, forced 401, refresh rejection). | ||
| import '@/services/app-reset.service'; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Place the side-effect import in the side-effect import group.
Keep this registration import before value imports so the required import ordering remains lint-compliant. As per coding guidelines, ESLint enforces simple-import-sort with grouping order: side-effect imports, external packages, internal aliases, relative imports, type imports.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/app/_layout.tsx` around lines 4 - 6, Move the side-effect import for the
app reset registration to the side-effect import group in the module’s import
section, keeping it before all value imports. Preserve the existing registration
behavior and ordering required by simple-import-sort.
Source: Coding guidelines
| jest.mock('@/components/maps/full-screen-map', () => ({ | ||
| FullScreenMap: ({ isOpen, ...props }: any) => { | ||
| const React = require('react'); | ||
| return isOpen ? React.createElement('full-screen-map', { ...props, testID: 'full-screen-call-map' }) : null; | ||
| }, | ||
| })); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
rg -n --type tsx '\bany\b' 'src/app/call/__tests__/[id].test.tsx'Repository: Resgrid/Unit
Length of output: 178
🏁 Script executed:
#!/bin/bash
set -euo to
# Locate and inspect the relevant test file and occurrences of mock any.
if [ -f 'src/app/call/__tests__/[id].test.tsx' ]; then
echo "== file exists =="
wc -l 'src/app/call/__tests__/[id].test.tsx'
echo
echo "== relevant lines =="
sed -n '260,305p' 'src/app/call/__tests__/[id].test.tsx' | nl -ba -v260
echo
echo "== any occurrences =="
grep -n '\bany\b' 'src/app/call/__tests__/[id].test.tsx' || true
else
echo "src/app/call/__tests__/[id].test.tsx not found"
echo "candidate files:"
fd -a '\[id\]\.test\.tsx' . || true
fiRepository: Resgrid/Unit
Length of output: 193
🏁 Script executed:
#!/bin/bash
set -eu # without the invalid `o` from `to`
# Locate and inspect the relevant test file and occurrences of explicit any.
if [ -f 'src/app/call/__tests__/[id].test.tsx' ]; then
echo "== file exists =="
wc -l 'src/app/call/__tests__/[id].test.tsx'
echo
echo "== relevant lines =="
sed -n '260,305p' 'src/app/call/__tests__/[id].test.tsx' | nl -ba -v260
echo
echo "== explicit any occurrences =="
grep -n '\bany\b' 'src/app/call/__tests__/[id].test.tsx' || true
else
echo "src/app/call/__tests__/[id].test.tsx not found"
echo "candidate files:"
fd -a '\[id\]\.test\.tsx' . || true
fiRepository: Resgrid/Unit
Length of output: 273
🏁 Script executed:
#!/bin/bash
set -eu
if [ -f 'src/app/call/__tests__/[id].test.tsx' ]; then
wc -l 'src/app/call/__tests__/[id].test.tsx'
cat -n 'src/app/call/__tests__/[id].test.tsx' | sed -n '260,305p'
echo "-- explicit any occurrences --"
grep -n '\bany\b' 'src/app/call/__tests__/[id].test.tsx' || true
else
echo "src/app/call/__tests__/[id].test.tsx not found"
fiRepository: Resgrid/Unit
Length of output: 5227
Replace the any mock props type.
Use a small FullScreenMap props interface instead of any so the test stays aligned with the component contract.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/app/call/__tests__/`[id].test.tsx around lines 285 - 290, Replace the any
type in the mocked FullScreenMap component with a small props interface
containing the required isOpen field and an appropriate type for forwarded
props, keeping the existing mock rendering behavior unchanged.
Source: Coding guidelines
| const { getByTestId, queryByTestId } = render(<CallDetail />); | ||
|
|
||
| expect(queryByTestId('full-screen-call-map')).toBeNull(); | ||
| fireEvent.press(await waitFor(() => getByTestId('call-detail-static-map'))); | ||
|
|
||
| const fullScreenMap = getByTestId('full-screen-call-map'); | ||
| expect(fullScreenMap.props).toMatchObject({ | ||
| latitude: 40.7128, | ||
| longitude: -74.006, | ||
| title: 'Test Call', | ||
| address: '123 Main St', | ||
| }); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Unmount rendered trees in the added tests.
src/app/call/__tests__/[id].test.tsx#L1362-L1374: destructureunmountfromrender()and call it after assertions.src/components/calls/__tests__/dispatch-selection-modal.test.tsx#L235-L237: destructureunmountfromrender()and call it after assertions.
As per coding guidelines, “Always call unmount() in tests to clean up.”
📍 Affects 2 files
src/app/call/__tests__/[id].test.tsx#L1362-L1374(this comment)src/components/calls/__tests__/dispatch-selection-modal.test.tsx#L235-L237
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/app/call/__tests__/`[id].test.tsx around lines 1362 - 1374, Update the
test render in src/app/call/__tests__/[id].test.tsx at lines 1362-1374 to
destructure unmount from render and call it after the assertions. Apply the same
cleanup in src/components/calls/__tests__/dispatch-selection-modal.test.tsx at
lines 235-237 by destructuring unmount and invoking it after assertions.
Source: Coding guidelines
| <Text className="text-sm font-medium">{safeFormatDate(stop.PlannedArrival, 'MMM d, h:mm a')}</Text> | ||
| </VStack> | ||
| <VStack> | ||
| <Text className="text-xs text-typography-500">{t('routes.planned_departure')}</Text> | ||
| <Text className="text-sm font-medium">{stop.PlannedDeparture ? format(new Date(stop.PlannedDeparture), 'MMM d, h:mm a') : '--'}</Text> | ||
| <Text className="text-sm font-medium">{safeFormatDate(stop.PlannedDeparture, 'MMM d, h:mm a')}</Text> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
fd -i 'routeInstanceStopResultData.ts' src/models
rg -n -C2 '\bPlanned(Arrival|Departure)\b' srcRepository: Resgrid/Unit
Length of output: 3085
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## Candidate date utility files"
fd -i 'date|time|format|date.*util' src | sed -n '1,120p'
echo
echo "## safeFormatDate and related date utility definitions"
rg -n -C3 'function safeFormatDate|const safeFormatDate|parseApiUtcDate|formatDateTime|format.*Date|safeDate' src
echo
echo "## Planned timestamp formatting usage"
rg -n -C3 'PlannedArrival|PlannedDeparture|parseApiUtcDate' src/app src/components src/lib src/services src/models
echo
echo "## stop detail relevant section"
sed -n '180,235p' src/app/routes/stop/[id].tsx
echo
echo "## stop-card relevant section"
sed -n '55,75p' src/components/routes/stop-card.tsxRepository: Resgrid/Unit
Length of output: 40740
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## src/lib/utils.ts parseApiUtcDate implementation"
sed -n '1,55p' src/lib/utils.ts
echo
echo "## src/lib/utils.ts formatDateForDisplay implementation"
sed -n '155,255p' src/lib/utils.ts
echo
echo "## src/lib/utils.ts parseWeatherAlertDate implementation"
sed -n '395,430p' src/lib/utils.ts
echo
echo "## src/lib/utils.ts safeFormatDate implementation"
sed -n '305,355p' src/lib/utils.ts
echo
echo "## src/app/call/[id].tsx imports around safeFormatDate"
sed -n '25,35p' src/app/call/[id].tsx
echo
echo "## src/app/routes/start.tsx imports and formatDateTime implementation"
sed -n '1,35p' src/app/routes/start.tsx
sed -n '21,38p' src/app/routes/start.tsx
echo
echo "## Standalone JS behavior for timezone-less ISO strings"
node - <<'JS'
const input = '2026-07-01T12:30:00';
console.log({
native_timestamp: new Date(input).getTime(),
naive_treated_as_localish_date: new Date(input).toString(),
});
console.log('native local ISO for naive input:', new Date input.toISOString());
console.log('fixed UTC parse for naive input:', new Date(input + 'Z').toISOString());
JSRepository: Resgrid/Unit
Length of output: 11698
🏁 Script executed:
#!/bin/bash
set -euo pipefail
node - <<'JS'
const naive = '2026-07-01T12:30:00';
const utc = '2026-07-01T12:30:00Z';
console.log({
naive_timestamp: new Date(naive).getTime(),
naive_iso: new Date(naive).toISOString(),
naive_locale: new Date(naive).toLocaleString(),
utc_timestamp: new Date(utc).getTime(),
utc_iso: new Date(utc).toISOString(),
utc_locale: new Date(utc).toLocaleString(),
});
console.log('same timestamp naive==utc?', new Date(naive).getTime() === new Date(utc).getTime());
JSRepository: Resgrid/Unit
Length of output: 411
Sanitize the planned routes timestamp fields before formatting.
These values are typed as plain strings, while PlannedArrival/PlannedDeparture are also rendered raw in stop-card.tsx and via new Date() in routes/start.tsx; that leaves the contract ambiguous and can parse timezone-less values as local time. Add a typed UTC designator or validate the contract, and apply the same parsing in stop-card.tsx and routes/start.tsx.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/app/routes/stop/`[id].tsx around lines 218 - 222, Normalize
PlannedArrival and PlannedDeparture to an explicit UTC timestamp contract before
passing them to safeFormatDate, and update the corresponding raw rendering in
stop-card.tsx and new Date() usage in routes/start.tsx to use the same parsing.
Preserve the existing display behavior while ensuring timezone-less values are
not interpreted as local time.
| import { logger } from '@/lib/logging'; | ||
| import { subscribeToSharedTicker } from '@/lib/shared-ticker'; | ||
| import type { CheckInTimerStatusResultData } from '@/models/v4/checkIn/checkInTimerStatusResultData'; | ||
|
|
||
| import { CheckInTimerCard } from '../check-in-timer-card'; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Move the type-only import after relative value imports.
The type import on Line 6 precedes the relative component import on Line 8. As per coding guidelines, ESLint enforces simple-import-sort with grouping order: side-effect imports, external packages, internal aliases, relative imports, type imports.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/check-in-timers/__tests__/check-in-timer-card.test.tsx` around
lines 4 - 8, Reorder the imports in the check-in timer card test so the relative
value import of CheckInTimerCard appears before the type-only
CheckInTimerStatusResultData import, preserving the existing side-effect,
external, and alias import groups.
Source: Coding guidelines
| const reconnectTimer = setTimeout(async () => { | ||
| if (this.reconnectTimers.get(hubName) === reconnectTimer) { | ||
| this.reconnectTimers.delete(hubName); | ||
| } | ||
|
|
||
| try { | ||
| // Check if the hub config was removed (e.g., by explicit disconnect) | ||
| const currentHubConfig = this.hubConfigs.get(hubName); | ||
| if (!currentHubConfig) { | ||
| logger.debug({ | ||
| message: `Hub ${hubName} config was removed, skipping reconnection attempt`, | ||
| }); | ||
| return; | ||
| } | ||
|
|
||
| // If a live connection exists, skip; if it's stale/closed, drop it | ||
| const existingConn = this.connections.get(hubName); | ||
| if (existingConn && existingConn.state === HubConnectionState.Connected) { | ||
| logger.debug({ | ||
| message: `Hub ${hubName} is already connected, skipping reconnection attempt`, | ||
| }); | ||
| this.reconnectAttempts.set(hubName, 0); | ||
| return; | ||
| } | ||
|
|
||
| // Mark as reconnecting and remove stale entry (if any) to allow a fresh connect | ||
| this.setHubState(hubName, HubConnectingState.RECONNECTING); | ||
| if (existingConn) { | ||
| this.connections.delete(hubName); | ||
| } | ||
|
|
||
| // Refresh authentication token before reconnecting | ||
| logger.info({ | ||
| message: `Refreshing authentication token before reconnecting to hub: ${hubName}`, | ||
| }); | ||
|
|
||
| await useAuthStore.getState().refreshAccessToken(); | ||
|
|
||
| // Verify we have a valid token after refresh — if the session is gone | ||
| // (refresh rejected), stop reconnecting instead of retrying forever. | ||
| const token = useAuthStore.getState().accessToken; | ||
| if (!token) { | ||
| logger.warn({ | ||
| message: `No valid authentication token after refresh, aborting reconnect for hub: ${hubName}`, | ||
| }); | ||
| this.setHubState(hubName, HubConnectingState.IDLE); | ||
| this.reconnectAttempts.delete(hubName); | ||
| this.hubConfigs.delete(hubName); | ||
| return; | ||
| } | ||
|
|
||
| logger.info({ | ||
| message: `Token refreshed successfully, attempting to reconnect to hub: ${hubName} (attempt ${currentAttempts}/${this.MAX_RECONNECT_ATTEMPTS})`, | ||
| }); | ||
|
|
||
| this.connections.delete(hubName); | ||
|
|
||
| await this.connectToHubWithEventingUrl(currentHubConfig); | ||
|
|
||
| // Clear reconnecting state on successful reconnection | ||
| this.setHubState(hubName, HubConnectingState.IDLE); | ||
| this.reconnectAttempts.set(hubName, 0); | ||
|
|
||
| logger.info({ | ||
| message: `Successfully reconnected to hub: ${hubName} after ${currentAttempts} attempts`, | ||
| }); | ||
| } catch (error) { | ||
| // Attempt failed — the old connection object is gone, so no further | ||
| // onclose event will fire. We MUST reschedule here or the hub stays | ||
| // dead until the next app background/resume cycle. | ||
| this.setHubState(hubName, HubConnectingState.IDLE); | ||
|
|
||
| logger.error({ | ||
| message: `Reconnection attempt ${currentAttempts}/${this.MAX_RECONNECT_ATTEMPTS} failed for hub: ${hubName}`, | ||
| context: { error }, | ||
| }); | ||
|
|
||
| // Reschedule (attempt count was already incremented when this attempt | ||
| // was scheduled); handleConnectionClose enforces the max-attempt cap. | ||
| this.handleConnectionClose(hubName); | ||
| } | ||
| }, delay); | ||
| this.reconnectTimers.set(hubName, reconnectTimer); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
An already-running reconnect callback can resurrect a hub after an explicit disconnect.
clearReconnectTimer only cancels a pending setTimeout. Once the callback body starts, it reads hubConfigs at Line 460 and then awaits refreshAccessToken() (Line 489) and connectToHubWithEventingUrl() (Line 510). If disconnectFromHub/disconnectAll runs during those awaits, it deletes hubConfigs/connections only after stop() resolves, so the in-flight callback re-registers the config and re-opens the connection — a hub that stays connected after teardown (and, in resetInstance/logout paths, keeps a socket alive under a stale session).
Suggest a per-hub generation counter (or a disconnectedHubs epoch set) checked after each await before proceeding.
🛡️ Sketch
+ // Re-check after the async gap — an explicit disconnect may have run.
+ if (!this.hubConfigs.has(hubName)) {
+ logger.debug({ message: `Hub ${hubName} disconnected during reconnect, aborting` });
+ this.setHubState(hubName, HubConnectingState.IDLE);
+ return;
+ }
logger.info({
message: `Token refreshed successfully, attempting to reconnect to hub: ${hubName} (attempt ${currentAttempts}/${this.MAX_RECONNECT_ATTEMPTS})`,
});🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/services/signalr.service.ts` around lines 453 - 535, Update the reconnect
callback around reconnectTimers and connectToHubWithEventingUrl to invalidate
in-flight reconnects when disconnectFromHub, disconnectAll, or resetInstance
tears down a hub. Add a per-hub generation/epoch (or equivalent disconnected
marker), capture it when scheduling the callback, and verify it after
refreshAccessToken and before reconnecting or applying success state; abort when
the generation changed or hub configuration was removed. Ensure teardown
increments or marks the generation so the callback cannot recreate configuration
or reopen a connection.
| Platform: { | ||
| OS: 'web', | ||
| }, | ||
| })); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
stopForegroundService assertion is vacuous under Platform.OS: 'web'.
The unexpected-drop handler only calls notifee.stopForegroundService() inside an android branch, so Line 243 would pass even if the intentional-switch guard regressed. Set Platform.OS: 'android' (or add a second case) so this assertion actually covers the foreground-service teardown path.
Also applies to: 243-243
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/stores/app/__tests__/livekit-store-room-switch.test.ts` around lines 86 -
89, Update the test setup in livekit-store-room-switch.test.ts so the
stopForegroundService assertion exercises the Android-specific branch by setting
Platform.OS to "android" for that case. Preserve the existing intentional-switch
guard coverage and ensure the assertion would fail if foreground-service
teardown were incorrectly triggered.
| room.on(RoomEvent.Disconnected, () => { | ||
| // Fired on network drop AND on intentional room.disconnect(). The | ||
| // intentional path sets isConnected=false BEFORE disconnecting, so a | ||
| // truthy value here means an unexpected drop — reset state so the UI | ||
| // never gets stuck showing "in call" with a dead room. | ||
| if (!get().isConnected) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Guard on room identity, not just isConnected.
This handler is registered per-room but only checks the global isConnected flag. A late Disconnected from a previously torn-down room (LiveKit can emit it after disconnect() resolves) will observe isConnected === true for the new room and wipe currentRoom/CallKeep/foreground-service state for a live call. Add an identity check.
🛡️ Proposed fix
room.on(RoomEvent.Disconnected, () => {
- if (!get().isConnected) {
+ // Ignore events from a room that is no longer the active one.
+ if (get().currentRoom !== room || !get().isConnected) {
return;
}📝 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.
| room.on(RoomEvent.Disconnected, () => { | |
| // Fired on network drop AND on intentional room.disconnect(). The | |
| // intentional path sets isConnected=false BEFORE disconnecting, so a | |
| // truthy value here means an unexpected drop — reset state so the UI | |
| // never gets stuck showing "in call" with a dead room. | |
| if (!get().isConnected) { | |
| return; | |
| } | |
| room.on(RoomEvent.Disconnected, () => { | |
| // Fired on network drop AND on intentional room.disconnect(). The | |
| // intentional path sets isConnected=false BEFORE disconnecting, so a | |
| // truthy value here means an unexpected drop — reset state so the UI | |
| // never gets stuck showing "in call" with a dead room. | |
| // Ignore events from a room that is no longer the active one. | |
| if (get().currentRoom !== room || !get().isConnected) { | |
| return; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/stores/app/livekit-store.ts` around lines 585 - 592, Update the
RoomEvent.Disconnected handler to verify that the event’s room is still the
store’s active/current room before resetting call state. Keep the existing
isConnected guard, but add a room-identity check using the room instance
captured by this handler and the store’s current room reference, returning early
for stale rooms so currentRoom, CallKeep, and foreground-service state remain
intact.
| // Only persist what is needed to restore a session. Transient fields — | ||
| // status, error, refreshTimeoutId — must never be persisted: an app kill | ||
| // mid-login used to rehydrate `status: 'loading'` with no way out, | ||
| // permanently locking the user behind a spinner. | ||
| partialize: (state) => ({ | ||
| accessToken: state.accessToken, | ||
| refreshToken: state.refreshToken, | ||
| refreshTokenExpiresOn: state.refreshTokenExpiresOn, | ||
| profile: state.profile, | ||
| userId: state.userId, | ||
| isFirstTime: state.isFirstTime, | ||
| }), |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Offline cold start can never reach signedIn.
status is no longer persisted, so after rehydration the only path to signedIn is a successful refreshAccessToken(). Line 377 sets status: 'loading' and, on a transient failure (offline — the common case for this app), Line 253 just retries every 30s and leaves status: 'loading' forever. Since isAuthenticated() requires status === 'signedIn', a responder who launches the app without connectivity is locked out of cached data, which is a regression from persisting status.
Consider optimistically setting status: 'signedIn' on rehydration when a non-expired accessToken exists (using the persisted refreshTokenExpiresOn), and reserving loading for the case where only a refresh token is present.
Also applies to: 363-383
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/stores/auth/store.tsx` around lines 339 - 350, Update the auth store
rehydration flow around partialize and refreshAccessToken so a persisted,
non-expired accessToken immediately restores status to signedIn, using
refreshTokenExpiresOn for expiry validation. Set status to loading only when
rehydration has a refresh token without a usable access token, while preserving
the existing refresh retry behavior for transient failures.
| import { act, renderHook } from '@testing-library/react-native'; | ||
|
|
||
| const mockCoreStoreGetState = jest.fn(() => ({ config: { EventingUrl: 'https://eventing.example.com/' } })); | ||
| const mockSecurityStore = { getState: jest.fn(() => ({ rights: { DepartmentId: '123' } })) }; | ||
|
|
||
| jest.mock('@/services/signalr.service', () => { | ||
| const mockInstance = { | ||
| connectToHubWithEventingUrl: jest.fn().mockResolvedValue(undefined), | ||
| disconnectFromHub: jest.fn().mockResolvedValue(undefined), | ||
| invoke: jest.fn().mockResolvedValue(undefined), | ||
| on: jest.fn(), | ||
| removeAllListeners: jest.fn(), | ||
| }; | ||
| class MockSignalRService { | ||
| static readonly HUB_DISCONNECTED_EVENT = '__hubDisconnected'; | ||
| static readonly HUB_RECONNECTING_EVENT = '__hubReconnecting'; | ||
| static readonly HUB_RECONNECTED_EVENT = '__hubReconnected'; | ||
| } | ||
| return { SignalRService: MockSignalRService, signalRService: mockInstance, default: mockInstance }; | ||
| }); | ||
| jest.mock('../../app/core-store', () => { | ||
| const mockStore: any = () => mockCoreStoreGetState(); | ||
| mockStore.getState = () => mockCoreStoreGetState(); | ||
| return { useCoreStore: mockStore }; | ||
| }); | ||
| jest.mock('../../security/store', () => { | ||
| console.log('RELATIVE security mock factory ran'); | ||
| return { securityStore: mockSecurityStore, useSecurityStore: mockSecurityStore }; | ||
| }); | ||
| jest.mock('@/stores/security/store', () => { | ||
| console.log('ALIAS security mock factory ran'); | ||
| return { securityStore: mockSecurityStore }; | ||
| }); | ||
| jest.mock('@/lib/logging', () => ({ logger: { info: jest.fn(), error: jest.fn(), warn: jest.fn(), debug: jest.fn(), trace: jest.fn(), fatal: jest.fn() } })); | ||
| jest.mock('@/lib/env', () => ({ Env: { CHANNEL_HUB_NAME: 'eventingHub', REALTIME_GEO_HUB_NAME: 'geolocationHub' } })); | ||
| jest.mock('@/lib', () => ({ useAuthStore: { getState: jest.fn(() => ({ accessToken: 'mock-token' })) } })); | ||
|
|
||
| import { useSignalRStore } from '../signalr-store'; | ||
| import { signalRService } from '@/services/signalr.service'; | ||
|
|
||
| it('dbg join', async () => { | ||
| const { result } = renderHook(() => useSignalRStore()); | ||
| await act(async () => { | ||
| await result.current.connectUpdateHub(); | ||
| }); | ||
| console.log('error:', result.current.error); | ||
| console.log('invoke calls:', (signalRService.invoke as jest.Mock).mock.calls); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Delete this debug artifact.
zz-dbg.test.ts is a scratch file: it has no assertions (so it can never fail), prints via console.log at Lines 27, 31, 46 and 47, duplicates mock setup and the department-join scenario already covered by src/stores/signalr/__tests__/signalr-store.test.ts (Lines 221-229), and the zz-dbg name does not describe anything under test. It only adds CI time and maintenance noise.
As per coding guidelines, "Never use console.log in production code — always use logger from @/lib/logging" and "Use lowercase-hyphenated naming for files and directories".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/stores/signalr/__tests__/zz-dbg.test.ts` around lines 1 - 48, Delete the
scratch test artifact zz-dbg.test.ts entirely; its console logging, duplicated
mocks, and assertion-free department-join scenario are already covered by
signalr-store.test.ts.
Source: Coding guidelines
2 participants
Summary
This PR delivers a comprehensive set of security hardening, bug fixes, and performance optimizations for the Resgrid Unit app (RU-T50).
Security Fixes
accessTokenFactory.http(s)URLs only, blocking dangerous schemes (file:,javascript:, custom protocols).allow-same-originfrom the iframe sandbox so server-supplied HTML runs on an opaque origin with no access to app storage/cookies. Added URL scheme validation on link clicks.http(s)before loading; JavaScript is disabled for MJPEG/other formats that don't require it.READ_MEDIA_IMAGESandREAD_MEDIA_VIDEOpermissions by switching to the system photo picker (selected-item-only access).Bug Fixes
parseApiUtcDate()and crash-safesafeFormatDate().PROCESSINGstatus after an app kill are recovered toPENDINGon rehydration; duplicate NetInfo listeners are prevented.RoomEvent.Disconnectedhandling to reset call state and tear down CallKeep/foreground service when the room drops unexpectedly.Performance Improvements
React.memocards.Summary by CodeRabbit