Conversation
This comment has been minimized.
This comment has been minimized.
|
Important Review skippedToo many files! This PR contains 151 files, which is 51 over the limit of 100. To get a review, narrow the scope: Upgrade to a paid plan to raise the limit. This review couldn't start because sufficient usage credits or metered capacity aren't available. Add credits or update usage-based reviews in the billing tab, then retry. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (151)
You can disable this status message by setting the 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 |
| @@ -0,0 +1,5 @@ | |||
| export default { | |||
There was a problem hiding this comment.
Default export in postcss.config.mjs violates Rule [72] favoring named exports for clarity and safer refactoring. Convert to a named constant export, verifying PostCSS loader compatibility before converting.
Kody rule violation: Avoid default exports
Prompt for LLM
File postcss.config.mjs:
Line 1:
Default export in `postcss.config.mjs` violates Rule [72] favoring named exports for clarity and safer refactoring. Convert to a named constant export, verifying PostCSS loader compatibility before converting.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| <Mapbox.PointAnnotation id="userLocation" coordinate={[locationLongitude, locationLatitude]} anchor={{ x: 0.5, y: 0.5 }}> | ||
| <Animated.View style={[styles.markerContainer, Platform.OS === 'web' ? styles.markerPulseWeb : { transform: [{ scale: pulseAnim }] }]}> | ||
| <View style={[styles.markerOuterRing, Platform.OS === 'web' && styles.markerOuterRingPulseWeb]} /> | ||
| <View style={styles.markerOuterRing} className={Platform.OS === 'web' ? 'pulse-ring' : undefined} /> |
There was a problem hiding this comment.
Global .pulse-ring CSS class violates Rule [16] requiring component-scoped styling for all non-top-level components. Move the animation into a CSS Module or BEM-style class name scoped to the component.
Kody rule violation: Use component-scoped styling
Prompt for LLM
File src/app/(app)/index.tsx:
Line 573:
Global `.pulse-ring` CSS class violates Rule [16] requiring component-scoped styling for all non-top-level components. Move the animation into a CSS Module or BEM-style class name scoped to the component.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| })); | ||
|
|
||
| jest.mock('nativewind', () => ({ | ||
| styled: jest.fn((Component: any) => Component), |
There was a problem hiding this comment.
Parameter Component uses PascalCase, violating Rule [7] requiring camelCase for locals and parameters. Rename to component.
Kody rule violation: Use proper naming conventions
Prompt for LLM
File src/app/call/new/__tests__/what3words.test.tsx:
Line 118:
Parameter `Component` uses PascalCase, violating Rule [7] requiring camelCase for locals and parameters. Rename to `component`.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| const isActive = segment.value === activeFilter; | ||
| return ( | ||
| <Pressable key={segment.value} onPress={() => handleFilterChange(segment.value)} className={`flex-1 items-center rounded-md px-3 py-2 ${isActive ? 'bg-white shadow-sm dark:bg-gray-600' : ''}`}> | ||
| <Pressable key={segment.value} onPress={() => handleFilterChange(segment.value)} className={`flex-1 items-center rounded-md px-3 py-2 ${isActive ? 'bg-white shadow-xs dark:bg-gray-600' : ''}`}> |
There was a problem hiding this comment.
Inline arrow function in the onPress prop of Pressable creates a new function on every render, degrading 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/maps/search.tsx:
Line 176:
Inline arrow function in the `onPress` prop of `Pressable` creates a new function on every render, degrading 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.
| } catch { | ||
| return this; | ||
| } |
There was a problem hiding this comment.
Swallowed exception in the _updateProjection catch block returns this without logging, violating Rule [29]. Log the caught error with structured context (operation name, error message) before returning the fallback value so failures are traceable.
Kody rule violation: Avoid empty catch blocks
Prompt for LLM
File src/components/maps/map-view.web.tsx:
Line 349 to 351:
Swallowed exception in the `_updateProjection` catch block returns `this` without logging, violating Rule [29]. Log the caught error with structured context (operation name, error message) before returning the fallback value so failures are traceable.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
|
|
||
| const cancelButton = screen.getByText('common.cancel').parent; | ||
| fireEvent.press(cancelButton); | ||
| fireEvent.press(cancelButton!); |
There was a problem hiding this comment.
Non-null assertion (!) on cancelButton suppresses the compiler instead of handling the nullable .parent result per Rule [2]. Add an explicit null guard with a clear error message or use optional chaining with an early return.
Kody rule violation: Add null checks before accessing properties
Prompt for LLM
File src/components/settings/__tests__/login-info-bottom-sheet-simple.test.tsx:
Line 219:
Non-null assertion (`!`) on `cancelButton` suppresses the compiler instead of handling the nullable `.parent` result per Rule [2]. Add an explicit null guard with a clear error message or use optional chaining with an early return.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
|
|
||
| const cancelButton = screen.getByText('common.cancel').parent; | ||
| fireEvent.press(cancelButton); | ||
| fireEvent.press(cancelButton!); |
There was a problem hiding this comment.
Potentially-null cancelButton derived from .parent is dereferenced via fireEvent.press with only a non-null assertion, violating Rule [26]. Add an explicit null check (e.g., expect(cancelButton).not.toBeNull()) or branch on the null case deterministically before the call.
Kody rule violation: Add null checks to prevent NullReferenceException
Prompt for LLM
File src/components/settings/__tests__/login-info-bottom-sheet-simple.test.tsx:
Line 219:
Potentially-null `cancelButton` derived from `.parent` is dereferenced via `fireEvent.press` with only a non-null assertion, violating Rule [26]. Add an explicit null check (e.g., `expect(cancelButton).not.toBeNull()`) or branch on the null case deterministically before the call.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| const ActionsheetFlatList = React.forwardRef<React.ComponentRef<typeof UIActionsheet.FlatList>, IActionsheetFlatListProps>(function ActionsheetFlatList({ className, style, ...props }, ref) { | ||
| return ( | ||
| <View className={actionsheetFlatListStyle({ class: className })} style={style}> | ||
| <UIActionsheet.FlatList ref={ref} estimatedItemSize={94} {...props} /> | ||
| <UIActionsheet.FlatList ref={ref} {...props} /> | ||
| </View> | ||
| ); |
There was a problem hiding this comment.
Missing estimatedItemSize on UIActionsheet.FlatList (a @shopify/flash-list) triggers dynamic item measurement, causing extra layout passes and scroll jank. Restore estimatedItemSize={94} or accept it as a forwarded prop.
<UIActionsheet.FlatList ref={ref} estimatedItemSize={94} {...props} />Prompt for LLM
File src/components/ui/actionsheet/index.tsx:
Line 362 to 367:
Missing `estimatedItemSize` on `UIActionsheet.FlatList` (a `@shopify/flash-list`) triggers dynamic item measurement, causing extra layout passes and scroll jank. Restore `estimatedItemSize={94}` or accept it as a forwarded prop.
Suggested Code:
<UIActionsheet.FlatList ref={ref} estimatedItemSize={94} {...props} />
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
|
|
||
| type IAnimatedPressableProps = React.ComponentProps<typeof Pressable> & MotionComponentProps<typeof Pressable, ViewStyle, unknown, unknown, unknown>; | ||
|
|
||
| const AnimatedPressable = createMotionAnimatedComponent(Pressable) as React.ComponentType<IAnimatedPressableProps>; |
There was a problem hiding this comment.
Unsafe type cast on createMotionAnimatedComponent(Pressable) bypasses compile-time type safety. Use the as operator with proper type guards or pattern matching to ensure null safety before usage.
Kody rule violation: Use safe type casting with as operator
Prompt for LLM
File src/components/ui/actionsheet/index.tsx:
Line 23:
Unsafe type cast on `createMotionAnimatedComponent(Pressable)` bypasses compile-time type safety. Use the `as` operator with proper type guards or pattern matching to ensure null safety before usage.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| // drives the class-based `dark:` variant (see @custom-variant in global.css). | ||
| const osScheme = useColorScheme(); | ||
| const colorSchemeName: 'light' | 'dark' = mode === 'system' ? (osScheme === 'dark' ? 'dark' : 'light') : mode === 'dark' ? 'dark' : 'light'; | ||
| const resolvedScheme: 'light' | 'dark' = mode === 'system' ? (osScheme === 'dark' ? 'dark' : 'light') : mode; |
There was a problem hiding this comment.
Repeated inline string literals 'light', 'dark', 'system' and the inline 'light' | 'dark' type duplicate the canonical ModeType union, risking drift. Derive from a single source (e.g., const MODES = ['light','dark','system'] as const) and reference those constants everywhere.
Kody rule violation: Use enums instead of magic strings
Prompt for LLM
File src/components/ui/gluestack-ui-provider/index.tsx:
Line 14:
Repeated inline string literals `'light'`, `'dark'`, `'system'` and the inline `'light' | 'dark'` type duplicate the canonical `ModeType` union, risking drift. Derive from a single source (e.g., `const MODES = ['light','dark','system'] as const`) and reference those constants everywhere.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| duration: 100, | ||
| }} | ||
| })} | ||
| exiting={FadeOut.duration(150)} |
There was a problem hiding this comment.
Inline magic number 150 duplicates the value on line 119 and invites drift if one changes without the other. Extract a shared named constant such as MENU_ANIM_DURATION = 150 and use it for both entering and exiting durations.
Kody rule violation: Replace magic numbers with named constants
Prompt for LLM
File src/components/ui/menu/index.tsx:
Line 123:
Inline magic number `150` duplicates the value on line 119 and invites drift if one changes without the other. Extract a shared named constant such as `MENU_ANIM_DURATION = 150` and use it for both entering and exiting durations.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| // react-native-web ships no Appearance.setColorScheme — drive the <html> | ||
| // class directly (same mechanism as GluestackUIProvider's script.ts). | ||
| if (typeof document !== 'undefined') { | ||
| const resolved = t === 'system' ? (window.matchMedia('(prefers-color-scheme: dark)').matches ? 'dark' : 'light') : t; |
There was a problem hiding this comment.
Hardcoded 'dark' and 'light' literals repeated across files violate Rule [6] requiring shared string literals to be centralized. Extract named constants or reuse existing ColorSchemeType members.
Kody rule violation: Centralize string constants
Prompt for LLM
File src/lib/hooks/use-selected-theme.tsx:
Line 15:
Hardcoded `'dark'` and `'light'` literals repeated across files violate Rule [6] requiring shared string literals to be centralized. Extract named constants or reuse existing `ColorSchemeType` members.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| @@ -14,9 +14,21 @@ export const statusDetailAllowsCalls = (detail: number): boolean => { | |||
| }; | |||
|
|
|||
| export const statusDetailAllowsStations = (detail: number): boolean => { | |||
There was a problem hiding this comment.
Unverified destination-type fallback in statusDetailAllowsStations and statusDetailAllowsPois permits handleSubmit to submit a RespondingToType the backend's Detail flag never authorized, risking rejection or silent misrouting. Confirm the backend accepts every RespondingToType the fallback now permits, or gate the fallback behind an explicit opt-in config/feature flag rather than applying it unconditionally to every department.
// Verify the backend honors these destinations for the legacy Detail values
// before applying the fallback unconditionally; otherwise restrict to departments
// that opt in.
export const statusDetailAllowsStations = (detail: number, legacyFallback = false): boolean => {
if (detail === CustomStateDetailTypes.Stations || detail === CustomStateDetailTypes.CallsAndStations || detail === CustomStateDetailTypes.StationsAndPois || detail === CustomStateDetailTypes.CallsStationsAndPois) {
return true;
}
return legacyFallback ? statusDetailAllowsCalls(detail) : false;
};Prompt for LLM
File src/models/v4/customStatuses/customStateDetailTypes.ts:
Line 16:
Unverified destination-type fallback in `statusDetailAllowsStations` and `statusDetailAllowsPois` permits `handleSubmit` to submit a `RespondingToType` the backend's `Detail` flag never authorized, risking rejection or silent misrouting. Confirm the backend accepts every `RespondingToType` the fallback now permits, or gate the fallback behind an explicit opt-in config/feature flag rather than applying it unconditionally to every department.
Suggested Code:
// Verify the backend honors these destinations for the legacy Detail values
// before applying the fallback unconditionally; otherwise restrict to departments
// that opt in.
export const statusDetailAllowsStations = (detail: number, legacyFallback = false): boolean => {
if (detail === CustomStateDetailTypes.Stations || detail === CustomStateDetailTypes.CallsAndStations || detail === CustomStateDetailTypes.StationsAndPois || detail === CustomStateDetailTypes.CallsStationsAndPois) {
return true;
}
return legacyFallback ? statusDetailAllowsCalls(detail) : false;
};
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| const isNoStationSentinel = (station: GroupResultData): boolean => { | ||
| const id = station.GroupId?.trim() ?? ''; | ||
| const name = station.Name?.trim().toLowerCase() ?? ''; | ||
| return id === '' || id === '0' || name === 'no station'; | ||
| }; |
There was a problem hiding this comment.
Duplicated normalize-and-compare logic in isNoStationSentinel mirrors isNoCallSentinel, risking drift when sentinel rules change. Extract a shared isSentinel(id, name, sentinelName) helper and call it from both functions.
Kody rule violation: Extract duplicated logic into functions
Prompt for LLM
File src/stores/status/store.ts:
Line 69 to 73:
Duplicated normalize-and-compare logic in `isNoStationSentinel` mirrors `isNoCallSentinel`, risking drift when sentinel rules change. Extract a shared `isSentinel(id, name, sentinelName)` helper and call it from both functions.
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:
|
| // Verified safe against the backend: SaveUnitStatus (UnitStatusController) | ||
| // validates the destination via IsValidDestinationAsync (entity exists in the | ||
| // same department) and never checks the status Detail flag, and | ||
| // GetSetUnitStatusData (DispatchController) serves these stations/pois as the |
There was a problem hiding this comment.
Unsafe type casting violates team rules. Use the as operator or pattern matching for safe casts and guard null results before usage.
Kody rule violation: Use safe type casting with as operator
Prompt for LLM
File src/models/v4/customStatuses/customStateDetailTypes.ts:
Line 26:
Unsafe type casting violates team rules. Use the `as` operator or pattern matching for safe casts and guard null results before usage.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| return normalizedId === '' || normalizedId === '0' || normalizedName === sentinelName; | ||
| }; | ||
|
|
||
| const isNoCallSentinel = (call: CallResultData): boolean => isSentinel(call.CallId, call.Name, 'no call'); |
There was a problem hiding this comment.
Scattered string literal for the sentinel name 'no call' duplicates values, violating Rule [6]. Centralize sentinel names into a constant object, such as const SENTINEL_NAMES = { NoCall: 'no call', NoStation: 'no station' } as const;, and pass SENTINEL_NAMES.NoCall.
Kody rule violation: Centralize string constants
Prompt for LLM
File src/stores/status/store.ts:
Line 67:
Scattered string literal for the sentinel name 'no call' duplicates values, violating Rule [6]. Centralize sentinel names into a constant object, such as `const SENTINEL_NAMES = { NoCall: 'no call', NoStation: 'no station' } as const;`, and pass `SENTINEL_NAMES.NoCall`.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| return normalizedId === '' || normalizedId === '0' || normalizedName === sentinelName; | ||
| }; | ||
|
|
||
| const isNoCallSentinel = (call: CallResultData): boolean => isSentinel(call.CallId, call.Name, 'no call'); |
There was a problem hiding this comment.
Magic string for the finite sentinel-name set 'no call' risks typos and reduces discoverability, violating Rule [8]. Define a const tuple like const SENTINEL_NAMES = ['no call', 'no station'] as const; with type SentinelName = typeof SENTINEL_NAMES[number]; and reference the named member.
Kody rule violation: Use enums instead of magic strings
Prompt for LLM
File src/stores/status/store.ts:
Line 67:
Magic string for the finite sentinel-name set 'no call' risks typos and reduces discoverability, violating Rule [8]. Define a const tuple like `const SENTINEL_NAMES = ['no call', 'no station'] as const;` with `type SentinelName = typeof SENTINEL_NAMES[number];` and reference the named member.
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}
2 participants
PR Description: RU-T52 Expo 56, Livekit 2.10 and Gluestack v5 Update
Summary
This PR upgrades the Resgrid Unit app to Expo SDK 56, LiveKit 2.10, and Gluestack UI v5 / NativeWind v5 / Tailwind v4, and includes several bug fixes and compatibility improvements that were required to complete the migration.
1. Framework & Platform Upgrades
targetSdkVersionto 36, removes the manualnewArchEnabledflag (now default), and addsexpo-image,expo-sharing, andexpo-status-barplugins.react-native-worklets/jest/resolver.js) andEXPO_PUBLIC_USE_RN_FETCHenv var set for test runtime.2. Gluestack UI v5 + NativeWind v5 + Tailwind v4 Migration
This is the largest portion of the change set — every Gluestack UI component wrapper (
src/components/ui/*) was rewritten:Import path consolidation
@gluestack-ui/<component>packages → unified@gluestack-ui/core/<component>/creator@gluestack-ui/nativewind-utils/*→@gluestack-ui/utils/nativewind-utilsPrimitiveIconboilerplate removed; components now use the coreUIIconwrapped with NativeWind'sstyled().Styling bridge
cssInterop(Component, …)calls replaced withstyled(Component, { className: 'style' }).withStates/withStyleContextAndStates(platform-conditional) removed in favor of a singlewithStyleContextpath.metro.config.jsswitched towithNativewind;postcss.config.mjsadded with@tailwindcss/postcss.Tailwind v4
global.cssrewritten from@tailwinddirectives to@import+@themeblocks, with all Gluestack design tokens expressed as CSS custom properties for light/dark (viaprefers-color-schemeand.dark/.lightclass overrides).tailwind.config.jsdropped in favor of inline@themedeclarations.shadow-sm→shadow-xs,rounded-sm→rounded-xs.Animation library swap
@legendapp/motion(Motion.View,AnimatePresence,initial/animate/exit) replaced withreact-native-reanimated(entering/exitingwithFadeIn,ZoomIn,SlideIn*, etc.) in Modal, AlertDialog, Drawer, Menu, Popover, Toast, and Actionsheet.Theme handling
GluestackUIProvidersimplified — no longer injects JS config objects; instead toggles alight/darkclassName and usesAppearance.setColorScheme.useColorSchemeis now imported fromnativewind(tracks manual overrides on web), anduse-selected-themedrives appearance viaAppearance.setColorScheme(native) and<html>class manipulation (web).@custom-variant darkwired up sodark:variants work with the class-driven approach.3. Navigation Consolidation
useFocusEffect,useIsFocused,DarkTheme,DefaultTheme, andThemeProviderare now imported fromexpo-routerinstead of@react-navigation/native.navigationRef(createNavigationContainerRef) export was removed.FocusAwareStatusBarsimplified for SDK 56 edge-to-edge (only visibility APIs remain) and no longer requires aNavigationContextguard.4. Bug Fixes & Improvements
MapViewnow forcesprojection: 'mercator'at construction, onload, onstyle.load, and patches_updateProjectionto swallow errors.StaticMapnow sizes the static image request to the actual viewport width (capped at 1280) soautoviewport pins stay centered in landscape.statusDetailAllowsStations/statusDetailAllowsPoisnow fall back tostatusDetailAllowsCallsso legacy departments using olderDetailvalues still surface destination options.patches/@gluestack-ui+core+5.0.15.patch) swaps the static window-height calculation foruseWindowDimensionsso the actionsheet reacts to orientation changes.toDomPropsutility strips RN-only props (testID,numberOfLines, accessibility props, responder handlers, etc.) and mapstestID→data-testidfor all web (*.web.tsx) UI wrappers, eliminating React "unknown prop" warnings.NotificationDetailandNotificationInboxnow read the color scheme viaAppearance.getColorScheme()instead of the removednativewind/colorScheme.get().animationNamestyle (rejected by react-native-web) replaced with a.pulse-ringCSS class inglobal.css.gap-*classes directly, removing the previouscssInterop-based nativeStyleToProp mapping.5. Test Updates
All test files were updated to match the new architecture:
nativewindmocks now includestyledandvars.@react-navigation/nativetoexpo-router.TurboModuleRegistry,Text,Modal, etc.) added where needed for the new Gluestack creator imports.test-utilsno longer wraps renders in aNavigationContainer.Risk areas to verify: all UI components (modals, drawers, actionsheets, menus, popovers, toasts), dark/light theme switching on both native and web, and the web map interaction after the projection fix.