Conversation
|
Warning Review limit reached
Next review available in: 17 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (48)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
This comment has been minimized.
This comment has been minimized.
| // Create axios instance with default config | ||
| const axiosInstance: AxiosInstance = axios.create({ | ||
| baseURL: getBaseApiUrl(), | ||
| timeout: 15000, |
There was a problem hiding this comment.
The magic number 15000 used for the axios timeout obscures intent and complicates future tuning. Extract a shared constant such as DEFAULT_REQUEST_TIMEOUT_MS = 15000 to allow consistent reuse across the codebase.
Kody rule violation: Replace magic numbers with named constants
Prompt for LLM
File src/api/common/client.tsx:
Line 11:
The magic number `15000` used for the axios timeout obscures intent and complicates future tuning. Extract a shared constant such as `DEFAULT_REQUEST_TIMEOUT_MS = 15000` to allow consistent reuse across the codebase.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| </InputSlot> | ||
| <InputField testID="incidents-search" placeholder={t('incidents.search')} value={searchQuery} onChangeText={setSearchQuery} returnKeyType="search" /> | ||
| {searchQuery ? ( | ||
| <Pressable className="h-full items-center justify-center pr-3" onPress={() => setSearchQuery('')} accessibilityLabel={t('common.clear')} accessibilityRole="button" testID="incidents-search-clear" hitSlop={8}> |
There was a problem hiding this comment.
Inline arrow functions in JSX props create new functions on every render, causing unnecessary performance overhead. Move the function definition outside the render method to comply with the 'Avoid using .bind() or arrow functions in JSX props' rule.
Prompt for LLM
File src/app/(app)/incidents.tsx:
Line 93:
Inline arrow functions in JSX props create new functions on every render, causing unnecessary performance overhead. Move the function definition outside the render method to comply with the 'Avoid using .bind() or arrow functions in JSX props' rule.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| import { useMapsStore } from '@/stores/maps/store'; | ||
|
|
||
| export default function CustomMapViewer() { | ||
| export default function CustomMapViewer(): React.JSX.Element { |
There was a problem hiding this comment.
The CustomMapViewer component uses a default export, reducing clarity and maintainability during refactoring. Change to a named export like export function CustomMapViewer(): React.JSX.Element to improve autocomplete and rename operations.
Kody rule violation: Avoid default exports
Prompt for LLM
File src/app/(app)/maps/custom/[id].tsx:
Line 20:
The `CustomMapViewer` component uses a default export, reducing clarity and maintainability during refactoring. Change to a named export like `export function CustomMapViewer(): React.JSX.Element` to improve autocomplete and rename operations.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| return; | ||
| } | ||
|
|
||
| const first = incidentPins[0]; |
There was a problem hiding this comment.
Direct array indexing without a bounds check risks returning undefined on empty arrays. Add an explicit length guard, such as const first = incidentPins.length > 0 ? incidentPins[0] : null, before dereferencing incidentPins.
Kody rule violation: Check query results before accessing indices
Prompt for LLM
File src/app/command-map/[callId].tsx:
Line 150:
Direct array indexing without a bounds check risks returning undefined on empty arrays. Add an explicit length guard, such as `const first = incidentPins.length > 0 ? incidentPins[0] : null`, before dereferencing `incidentPins`.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| */ | ||
| let inFlightRefresh: Promise<AuthResponse> | null = null; | ||
|
|
||
| export const refreshTokenSingleFlight = (refreshToken: string): Promise<AuthResponse> => { |
There was a problem hiding this comment.
The JSDoc block for the refreshTokenSingleFlight function (lines 92-97) lacks a @returns {Promise<AuthResponse>} tag and rejection condition documentation. Move the JSDoc directly above the function declaration and add the missing @returns and rejection details.
Kody rule violation: Document async/Promise behavior and errors
Prompt for LLM
File src/lib/auth/api.tsx:
Line 100:
The JSDoc block for the `refreshTokenSingleFlight` function (lines 92-97) lacks a `@returns {Promise<AuthResponse>}` tag and rejection condition documentation. Move the JSDoc directly above the function declaration and add the missing `@returns` and rejection details.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| let cacheItem: CacheItem<T>; | ||
| try { | ||
| cacheItem = JSON.parse(cached); | ||
| } catch { |
There was a problem hiding this comment.
The catch block silently discards JSON parse failures without logging the exception, making cache corruption impossible to diagnose. Capture the error and log it with structured context using logger.error('cache parse failed', { key, err }) before returning null.
Kody rule violation: Avoid empty catch blocks
Prompt for LLM
File src/lib/cache/cache-manager.ts:
Line 52:
The catch block silently discards JSON parse failures without logging the exception, making cache corruption impossible to diagnose. Capture the error and log it with structured context using `logger.error('cache parse failed', { key, err })` before returning null.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| export const getMapsHeaderState = (pathname: string): MapsHeaderState => { | ||
| const normalizedPathname = pathname.replace(/\/+$/, '') || '/'; | ||
|
|
||
| if (normalizedPathname === '/maps') { |
There was a problem hiding this comment.
The inline string literal '/maps' bypasses route centralization requirements. Extract the path into a named constant such as const MAPS_BASE_ROUTE = '/maps' to prevent typos and improve maintainability.
Kody rule violation: Centralize string constants
Prompt for LLM
File src/lib/maps-route.ts:
Line 11:
The inline string literal `'/maps'` bypasses route centralization requirements. Extract the path into a named constant such as `const MAPS_BASE_ROUTE = '/maps'` to prevent typos and improve maintainability.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| @@ -0,0 +1,43 @@ | |||
| export type MapsHeaderTitleKey = 'maps.custom_maps' | 'maps.indoor_maps' | 'maps.search_maps' | 'maps.title'; | |||
There was a problem hiding this comment.
The hand-enumerated MapsHeaderTitleKey type risks diverging from runtime string values. Derive the type from a const tuple using const MAPS_HEADER_TITLE_KEYS = [...] as const and typeof MAPS_HEADER_TITLE_KEYS[number].
Kody rule violation: Derive TypeScript types from validation schemas
Prompt for LLM
File src/lib/maps-route.ts:
Line 1:
The hand-enumerated `MapsHeaderTitleKey` type risks diverging from runtime string values. Derive the type from a const tuple using `const MAPS_HEADER_TITLE_KEYS = [...] as const` and `typeof MAPS_HEADER_TITLE_KEYS[number]`.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| }, | ||
| }); | ||
| useLocationStore.getState().setLocation(location); | ||
| sendLocationToAPI(location); // Send to API for background updates |
There was a problem hiding this comment.
The fire-and-forget sendLocationToAPI(location) call inside the watchPosition callback leaves Promise rejections unhandled. Append a .catch() handler or wrap an await call in try/catch to properly capture and log network errors.
Kody rule violation: Handle async operations with proper error handling
Prompt for LLM
File src/services/location.ts:
Line 286:
The fire-and-forget `sendLocationToAPI(location)` call inside the `watchPosition` callback leaves Promise rejections unhandled. Append a `.catch()` handler or wrap an `await` call in `try/catch` to properly capture and log network errors.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| // never fire for action presses from the killed state. | ||
| if (Platform.OS !== 'web') { | ||
| notifee.onBackgroundEvent(async ({ type, detail }) => { | ||
| if (type === EventType.ACTION_PRESS && detail.pressAction?.id === 'check-in') { |
There was a problem hiding this comment.
The raw string literal 'check-in' used for the press-action identifier is inconsistent and fragile. Define a shared constant like const PRESS_ACTION_CHECK_IN = 'check-in' or an enum to align with existing centralized identifiers.
Kody rule violation: Use enums instead of magic strings
Prompt for LLM
File src/services/push-notification.ts:
Line 443:
The raw string literal `'check-in'` used for the press-action identifier is inconsistent and fragile. Define a shared constant like `const PRESS_ACTION_CHECK_IN = 'check-in'` or an enum to align with existing centralized identifiers.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| logger.error({ | ||
| message: `Error in SignalR event listener for event: ${event}`, | ||
| context: { error }, | ||
| }); |
There was a problem hiding this comment.
The error log embeds the event name inside the message string instead of as a structured field. Add the event and hub as explicit structured fields like op: 'signalr.emit', event to ensure logs remain searchable and filterable.
Kody rule violation: Include error context in structured logs
Prompt for LLM
File src/services/signalr.service.ts:
Line 646 to 649:
The error log embeds the event name inside the message string instead of as a structured field. Add the event and hub as explicit structured fields like `op: 'signalr.emit', event` to ensure logs remain searchable and filterable.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| room.on(RoomEvent.Disconnected, () => { | ||
| logger.warn({ | ||
| message: 'LiveKit room disconnected (server kick or unrecoverable network loss)', | ||
| context: { roomName: roomInfo.Name }, | ||
| }); | ||
| // Only clean up if this room is still the active one (avoids racing a manual disconnect) | ||
| if (get().currentRoom === room) { | ||
| Promise.resolve(get().disconnectFromRoom()).catch((cleanupError: unknown) => { | ||
| logger.error({ | ||
| message: 'Failed to clean up after LiveKit room disconnect', | ||
| context: { error: cleanupError }, | ||
| }); | ||
| }); | ||
| } | ||
| }); |
There was a problem hiding this comment.
The RoomEvent.Disconnected handler (line 575-589) fires re-entrant cleanup during a manual disconnect because currentRoom is only set to null at the end of disconnectFromRoom() (line 850). Clear currentRoom or set an isDisconnecting flag before calling currentRoom.disconnect() (line 790) to prevent concurrent async cleanup chains and duplicated sounds.
// In disconnectFromRoom(), clear currentRoom before disconnecting:
// const { currentRoom } = get();
// if (currentRoom) {
// set({ currentRoom: null }); // clear FIRST so the Disconnected handler's guard skips re-entrant cleanup
// try { await currentRoom.disconnect(); } catch ...
// ... rest of cleanup references the local `currentRoom` variablePrompt for LLM
File src/stores/app/livekit-store.ts:
Line 575 to 589:
The `RoomEvent.Disconnected` handler (line 575-589) fires re-entrant cleanup during a manual disconnect because `currentRoom` is only set to null at the end of `disconnectFromRoom()` (line 850). Clear `currentRoom` or set an `isDisconnecting` flag before calling `currentRoom.disconnect()` (line 790) to prevent concurrent async cleanup chains and duplicated sounds.
Suggested Code:
// In disconnectFromRoom(), clear currentRoom before disconnecting:
// const { currentRoom } = get();
// if (currentRoom) {
// set({ currentRoom: null }); // clear FIRST so the Disconnected handler's guard skips re-entrant cleanup
// try { await currentRoom.disconnect(); } catch ...
// ... rest of cleanup references the local `currentRoom` variable
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
Kody Review CompleteGreat news! 🎉 Keep up the excellent work! 🚀 Kody Guide: Usage and ConfigurationInteracting with Kody
Current Kody ConfigurationReview OptionsThe following review options are enabled or disabled:
|
2 participants
PR Description
Overview
This PR implements RIC-T40 Fixes, a collection of stability, robustness, and feature improvements across the Resgrid IC mobile application.
Key Changes
🔐 Authentication Stability
refreshTokenSingleFlightwrapper to ensure concurrent refresh requests (from the axios 401 interceptor and proactive refresh timer) share a single in-flight request, preventing the rotating refresh token from being consumed multiple times.expires_inhandling: Added fallback to 3600 seconds whenexpires_inis missing or undefined, preventing crashes during login, SSO login, and token refresh.🗺️ Maps Route Restructuring
maps-route.tshelper to dynamically set the correct header (drawer menu on landing, back button on child routes) and title based on the current pathname._layout.tsxfor Maps with aStacknavigator supporting index, search, custom maps, and indoor maps screens.🔍 Incidents Search
📍 Command Map Camera
🛡️ Robustness & Null-Safety Fixes
MapMakerInfos ?? []) in map rendering and SignalR update handler.isValidbefore formatting, falling back to raw string or empty value.📡 SignalR Improvements
📱 Push Notifications
notifee.onBackgroundEventregistration to module scope so notification action presses work when the app is killed (headless JS task).📞 LiveKit
ReconnectingandDisconnectedevent handlers with automatic cleanup on unexpected disconnects.disconnectFromRoomresilient — cleanup continues even if the disconnect call or sound playback fails.📍 Location Service
🔧 Other
isConnectingin a permanent state.