CU-868frkzum FlatList to FlashList change, updated LiveKit, fixes for… #172
Conversation
WalkthroughAdopts Shopify FlashList across UI, updates mocks and Jest ignore rules, bumps dependencies, adds testIDs to lists, tightens role-assignment save filtering (exclude empty/whitespace RoleId/UserId), refines GPS save logic (only populate when both lat/long missing, add AltitudeAccuracy, guard async refresh), and adds two CI env secrets. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant RolesUI as Roles (Modal/BottomSheet)
participant RolesStore as Roles Store
participant API as Backend
User->>RolesUI: Tap "Save"
RolesUI->>RolesUI: Build pendingAssignments, trim values
RolesUI->>RolesUI: Filter entries where RoleId && UserId are non-empty
RolesUI->>RolesStore: assignRoles({ UnitId, Roles: filtered })
RolesStore->>API: POST /roles/assign
API-->>RolesStore: 200 / error
RolesStore-->>RolesUI: Resolve / Reject
RolesUI-->>User: Close or show error
sequenceDiagram
autonumber
actor User
participant StatusUI as Status BottomSheet
participant StatusStore as Status Store
participant Location as Location Store
participant Core as Core Store
participant API as Backend
User->>StatusUI: Submit status
StatusUI->>StatusStore: saveUnitStatus(input)
StatusStore->>StatusStore: If Latitude AND Longitude both missing/empty
StatusStore->>Location: Read latest location
alt location available
StatusStore->>StatusStore: Populate Latitude,Longitude,Accuracy,Altitude,Speed,Heading,AltitudeAccuracy('')
else no location
StatusStore->>StatusStore: Set GPS fields to ''
end
StatusStore->>Core: setActiveUnitWithFetch(...)
Core-->>StatusStore: Promise | undefined
StatusStore->>StatusStore: If thenable, attach .catch(handler)
StatusStore->>API: POST /status/save
API-->>StatusStore: 200/Error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
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. Tip 🧪 Early access (models): enabledWe are currently testing Sonnet 4.5 code review models, which should lead to better review quality. However, this model may result in higher noise levels in the review comments. Please disable the early access features if the noise level causes any inconvenience. Note:
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/components/roles/roles-bottom-sheet.tsx (1)
82-104: Unassigning a role is now impossibleWhen a user clears an assignment,
handleAssignUserdrops the pending entry (because the userId is falsy) and the mapper here falls back to the existingunitRoleAssignments, so we keep sending the previousUserId. Even if we forced a blankuserId, this filter strips it out. The result is that clearing a role no longer reaches the API, so assignments can only ever be added or reassigned—never removed.Please preserve the “remove assignment” path by carrying an explicit pending removal through
pendingAssignmentsand allowingUserId === ''for those roles instead of filtering them out wholesale.const handleAssignUser = React.useCallback((roleId: string, userId?: string) => { setPendingAssignments((current) => { const filtered = current.filter((a) => a.roleId !== roleId); - // Only add to pending assignments if a user is actually selected - if (userId && userId.trim() !== '') { - return [...filtered, { roleId, userId }]; - } - // If no user selected (unassigned), just return filtered array - return filtered; + const normalizedUserId = (userId ?? '').trim(); + return [...filtered, { roleId, userId: normalizedUserId }]; }); }, []); @@ - const allUnitRoles = filteredRoles - .map((role) => { + const allUnitRoles = filteredRoles + .map((role) => { const pendingAssignment = pendingAssignments.find((a) => a.roleId === role.UnitRoleId); const currentAssignment = unitRoleAssignments.find((a) => a.UnitRoleId === role.UnitRoleId && a.UnitId === activeUnit.UnitId); - const assignedUserId = pendingAssignment?.userId || currentAssignment?.UserId || ''; + const hasPendingUpdate = pendingAssignment !== undefined; + const assignedUserId = hasPendingUpdate + ? pendingAssignment?.userId ?? '' + : currentAssignment?.UserId ?? ''; return { RoleId: role.UnitRoleId, UserId: assignedUserId, Name: '', }; }) - .filter((role) => { - // Only include roles that have valid RoleId and assigned UserId - return role.RoleId && role.RoleId.trim() !== '' && role.UserId && role.UserId.trim() !== ''; - }); + .filter((role) => { + if (!role.RoleId || role.RoleId.trim() === '') { + return false; + } + if (role.UserId && role.UserId.trim() !== '') { + return true; + } + const hadExistingAssignment = unitRoleAssignments.some( + (a) => a.UnitRoleId === role.RoleId && a.UnitId === activeUnit.UnitId + ); + return hadExistingAssignment; + });src/components/calls/call-images-modal.tsx (1)
387-407: Provide FlashList with anestimatedItemSize(or layout override).FlashList requires either
estimatedItemSizeoroverrideItemLayoutto compute offsets correctly. Without it, you’ll get runtime warnings andscrollToIndexwill keep throwing, which means the Previous/Next buttons will silently fail (we’re just logging the caught error right now). Please setestimatedItemSize={width}for this horizontal pager (or override the layout) so navigation remains reliable.
🧹 Nitpick comments (3)
src/app/onboarding.tsx (1)
5-68: Avoidanyon the FlashList refSwitching to FlashList shouldn’t force us to abandon type safety—using
anyhere hides real API issues. Keep the ref strongly typed so we retain autocomplete and compile-time checks. As per coding guidelines.-import { Dimensions, Image } from 'react-native'; +import { Dimensions, Image } from 'react-native'; +import type { FlashList } from '@shopify/flash-list'; … -const flatListRef = useRef<any>(null); // FlashList ref type +const flatListRef = useRef<FlashList<OnboardingItemProps> | null>(null);__mocks__/@shopify/flash-list.ts (1)
5-7: Tighten the mock’s typing to avoidany.Stubbing with
props: any, ref: anydefeats the type-safety guarantees we rely on, and the compiler will no longer catch accidental misuse of FlashList-only props during test runs. Please mirror the real signature by forwarding a generically typedFlatListProps<T>/FlatList<T>ref (you can still loosen it with an index signature for FlashList extras) so the mock stays transparent to TypeScript.src/components/calls/call-images-modal.tsx (1)
62-62: Keep the ref strongly typed.Switching the gallery list to FlashList shouldn’t force us back to
useRef<any>. Please type this asuseRef<FlashList<CallFileResultData> | null>(import the type from@shopify/flash-list) so IDE hints and compile-time checks keep working.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (26)
.github/workflows/react-native-cicd.yml(1 hunks)__mocks__/@shopify/flash-list.ts(1 hunks)docs/empty-role-id-fix.md(1 hunks)docs/gps-coordinate-duplication-fix.md(1 hunks)jest.config.js(1 hunks)package.json(2 hunks)src/app/(app)/calls.tsx(1 hunks)src/app/(app)/contacts.tsx(1 hunks)src/app/(app)/notes.tsx(2 hunks)src/app/(app)/protocols.tsx(2 hunks)src/app/onboarding.tsx(2 hunks)src/components/calls/call-images-modal.tsx(2 hunks)src/components/notifications/NotificationInbox.tsx(2 hunks)src/components/roles/__tests__/roles-bottom-sheet.test.tsx(1 hunks)src/components/roles/__tests__/roles-modal.test.tsx(1 hunks)src/components/roles/roles-bottom-sheet.tsx(1 hunks)src/components/roles/roles-modal.tsx(1 hunks)src/components/status/__tests__/gps-coordinate-duplication-fix.test.tsx(1 hunks)src/components/status/__tests__/location-update-validation.test.tsx(1 hunks)src/components/status/__tests__/status-gps-integration-working.test.tsx(1 hunks)src/components/status/__tests__/status-gps-integration.test.tsx(1 hunks)src/components/status/status-bottom-sheet.tsx(1 hunks)src/components/ui/actionsheet/index.tsx(2 hunks)src/components/ui/flat-list/index.tsx(1 hunks)src/components/ui/select/select-actionsheet.tsx(2 hunks)src/stores/status/store.ts(3 hunks)
🧰 Additional context used
📓 Path-based instructions (9)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursorrules)
**/*.{ts,tsx}: Write concise, type-safe TypeScript code
Use camelCase for variable and function names (e.g., isFetchingData, handleUserInput)
Use TypeScript for all components, favoring interfaces for props and state
Avoid using any; strive for precise types
Use Expo SecureStore for sensitive data
Use zustand for state management
Use react-query for data fetching and caching
Use react-i18next for internationalization
Use react-native-mmkv for local storage
Use axios for API requests
**/*.{ts,tsx}: Write concise, type-safe TypeScript code
Use camelCase for variable and function names
Use TypeScript for all components, favoring interfaces for props and state
Avoid using any; strive for precise types
Use React Navigation for handling navigation and deep linking
Handle errors gracefully and provide user feedback
Use Expo's SecureStore for sensitive data
Use zustand for state management
Use react-query for data fetching
Use react-i18next for internationalization
Use react-native-mmkv for local storage
Use axios for API requests
Files:
src/components/roles/roles-bottom-sheet.tsxsrc/app/(app)/contacts.tsxsrc/app/onboarding.tsx__mocks__/@shopify/flash-list.tssrc/components/roles/__tests__/roles-modal.test.tsxsrc/components/ui/select/select-actionsheet.tsxsrc/components/ui/actionsheet/index.tsxsrc/components/notifications/NotificationInbox.tsxsrc/components/roles/roles-modal.tsxsrc/components/status/status-bottom-sheet.tsxsrc/components/calls/call-images-modal.tsxsrc/app/(app)/calls.tsxsrc/components/roles/__tests__/roles-bottom-sheet.test.tsxsrc/stores/status/store.tssrc/app/(app)/notes.tsxsrc/components/status/__tests__/location-update-validation.test.tsxsrc/components/status/__tests__/status-gps-integration.test.tsxsrc/components/ui/flat-list/index.tsxsrc/app/(app)/protocols.tsxsrc/components/status/__tests__/gps-coordinate-duplication-fix.test.tsxsrc/components/status/__tests__/status-gps-integration-working.test.tsx
**/*.tsx
📄 CodeRabbit inference engine (.cursorrules)
**/*.tsx: Use functional components and hooks over class components
Use PascalCase for React component names (e.g., UserProfile, ChatScreen)
Utilize React.FC for defining functional components with props
Minimize useEffect, useState, and heavy computations inside render
Use React.memo() for components with static props to prevent unnecessary re-renders
Optimize FlatList with removeClippedSubviews, maxToRenderPerBatch, windowSize
Use getItemLayout for FlatList when items have consistent size
Avoid anonymous functions in renderItem or event handlers to prevent re-renders
Use gluestack-ui for styling; if no component exists in src/components/ui, style via StyleSheet.create or styled-components
Optimize images using react-native-fast-image
Use React Navigation for navigation and deep linking with best practices
Wrap all user-facing text in t() from react-i18next
Use react-hook-form for form handling
Use @rnmapbox/maps for maps or vehicle navigation
Use lucide-react-native for icons directly in markup; do not use the gluestack-ui icon component
Use the conditional operator (?:) for conditional rendering instead of &&
**/*.tsx: Use functional components and hooks over class components
Use PascalCase for component names
Utilize React.FC for defining functional components with props
Minimize useEffect/useState and heavy computations inside render
Use React.memo for components with static props to prevent unnecessary re-renders
Optimize FlatList with removeClippedSubviews, maxToRenderPerBatch, and windowSize
Use getItemLayout for FlatList when items have consistent size
Avoid anonymous functions in renderItem or event handlers
Optimize image handling using react-native-fast-image
Wrap all user-facing text in t() from react-i18next
Ensure support for dark mode and light mode
Ensure the app is accessible following WCAG for mobile
Use react-hook-form for form handling
Use @rnmapbox/maps for maps and navigation
Use lucide-react-native for icons directly in markup; do not us...
Files:
src/components/roles/roles-bottom-sheet.tsxsrc/app/(app)/contacts.tsxsrc/app/onboarding.tsxsrc/components/roles/__tests__/roles-modal.test.tsxsrc/components/ui/select/select-actionsheet.tsxsrc/components/ui/actionsheet/index.tsxsrc/components/notifications/NotificationInbox.tsxsrc/components/roles/roles-modal.tsxsrc/components/status/status-bottom-sheet.tsxsrc/components/calls/call-images-modal.tsxsrc/app/(app)/calls.tsxsrc/components/roles/__tests__/roles-bottom-sheet.test.tsxsrc/app/(app)/notes.tsxsrc/components/status/__tests__/location-update-validation.test.tsxsrc/components/status/__tests__/status-gps-integration.test.tsxsrc/components/ui/flat-list/index.tsxsrc/app/(app)/protocols.tsxsrc/components/status/__tests__/gps-coordinate-duplication-fix.test.tsxsrc/components/status/__tests__/status-gps-integration-working.test.tsx
**/*
📄 CodeRabbit inference engine (.cursorrules)
Directory and file names should be lowercase and hyphenated (e.g., user-profile, chat-screen)
Files:
src/components/roles/roles-bottom-sheet.tsxsrc/app/(app)/contacts.tsxsrc/app/onboarding.tsxdocs/gps-coordinate-duplication-fix.md__mocks__/@shopify/flash-list.tssrc/components/roles/__tests__/roles-modal.test.tsxdocs/empty-role-id-fix.mdpackage.jsonsrc/components/ui/select/select-actionsheet.tsxjest.config.jssrc/components/ui/actionsheet/index.tsxsrc/components/notifications/NotificationInbox.tsxsrc/components/roles/roles-modal.tsxsrc/components/status/status-bottom-sheet.tsxsrc/components/calls/call-images-modal.tsxsrc/app/(app)/calls.tsxsrc/components/roles/__tests__/roles-bottom-sheet.test.tsxsrc/stores/status/store.tssrc/app/(app)/notes.tsxsrc/components/status/__tests__/location-update-validation.test.tsxsrc/components/status/__tests__/status-gps-integration.test.tsxsrc/components/ui/flat-list/index.tsxsrc/app/(app)/protocols.tsxsrc/components/status/__tests__/gps-coordinate-duplication-fix.test.tsxsrc/components/status/__tests__/status-gps-integration-working.test.tsx
{components/ui/**/*.{ts,tsx},**/*.tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use gluestack-ui consistently; if no component exists in components/ui, style via StyleSheet.create or styled-components
Files:
src/components/roles/roles-bottom-sheet.tsxsrc/app/(app)/contacts.tsxsrc/app/onboarding.tsxsrc/components/roles/__tests__/roles-modal.test.tsxsrc/components/ui/select/select-actionsheet.tsxsrc/components/ui/actionsheet/index.tsxsrc/components/notifications/NotificationInbox.tsxsrc/components/roles/roles-modal.tsxsrc/components/status/status-bottom-sheet.tsxsrc/components/calls/call-images-modal.tsxsrc/app/(app)/calls.tsxsrc/components/roles/__tests__/roles-bottom-sheet.test.tsxsrc/app/(app)/notes.tsxsrc/components/status/__tests__/location-update-validation.test.tsxsrc/components/status/__tests__/status-gps-integration.test.tsxsrc/components/ui/flat-list/index.tsxsrc/app/(app)/protocols.tsxsrc/components/status/__tests__/gps-coordinate-duplication-fix.test.tsxsrc/components/status/__tests__/status-gps-integration-working.test.tsx
**/*.{test.ts,test.tsx,spec.ts,spec.tsx}
📄 CodeRabbit inference engine (.cursorrules)
Create and use Jest tests to validate all generated components
Files:
src/components/roles/__tests__/roles-modal.test.tsxsrc/components/roles/__tests__/roles-bottom-sheet.test.tsxsrc/components/status/__tests__/location-update-validation.test.tsxsrc/components/status/__tests__/status-gps-integration.test.tsxsrc/components/status/__tests__/gps-coordinate-duplication-fix.test.tsxsrc/components/status/__tests__/status-gps-integration-working.test.tsx
**/__tests__/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursorrules)
Generate tests for all components, services, and logic; ensure tests run without errors
Files:
src/components/roles/__tests__/roles-modal.test.tsxsrc/components/roles/__tests__/roles-bottom-sheet.test.tsxsrc/components/status/__tests__/location-update-validation.test.tsxsrc/components/status/__tests__/status-gps-integration.test.tsxsrc/components/status/__tests__/gps-coordinate-duplication-fix.test.tsxsrc/components/status/__tests__/status-gps-integration-working.test.tsx
**/*.{test,spec}.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Create and use Jest tests for all generated components
Files:
src/components/roles/__tests__/roles-modal.test.tsxsrc/components/roles/__tests__/roles-bottom-sheet.test.tsxsrc/components/status/__tests__/location-update-validation.test.tsxsrc/components/status/__tests__/status-gps-integration.test.tsxsrc/components/status/__tests__/gps-coordinate-duplication-fix.test.tsxsrc/components/status/__tests__/status-gps-integration-working.test.tsx
src/components/ui/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursorrules)
Build and maintain shared Gluestack UI wrappers in src/components/ui
Files:
src/components/ui/select/select-actionsheet.tsxsrc/components/ui/actionsheet/index.tsxsrc/components/ui/flat-list/index.tsx
jest.config.@(js|ts)
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Provide Jest configuration
Files:
jest.config.js
🧠 Learnings (12)
📚 Learning: 2025-08-12T03:34:25.992Z
Learnt from: CR
PR: Resgrid/Unit#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-12T03:34:25.992Z
Learning: Applies to **/*.tsx : Optimize FlatList with removeClippedSubviews, maxToRenderPerBatch, and windowSize
Applied to files:
src/app/(app)/contacts.tsxsrc/app/onboarding.tsx__mocks__/@shopify/flash-list.tssrc/components/ui/select/select-actionsheet.tsxsrc/components/ui/actionsheet/index.tsxsrc/components/calls/call-images-modal.tsxsrc/app/(app)/notes.tsxsrc/components/ui/flat-list/index.tsxsrc/app/(app)/protocols.tsx
📚 Learning: 2025-08-12T03:33:40.238Z
Learnt from: CR
PR: Resgrid/Unit#0
File: .cursorrules:0-0
Timestamp: 2025-08-12T03:33:40.238Z
Learning: Applies to **/*.tsx : Optimize FlatList with removeClippedSubviews, maxToRenderPerBatch, windowSize
Applied to files:
src/app/(app)/contacts.tsxsrc/app/onboarding.tsx__mocks__/@shopify/flash-list.tssrc/components/ui/select/select-actionsheet.tsxsrc/components/ui/actionsheet/index.tsxsrc/components/calls/call-images-modal.tsxsrc/app/(app)/notes.tsxsrc/components/ui/flat-list/index.tsxsrc/app/(app)/protocols.tsx
📚 Learning: 2025-08-12T03:34:25.992Z
Learnt from: CR
PR: Resgrid/Unit#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-12T03:34:25.992Z
Learning: Applies to **/*.tsx : Use lucide-react-native for icons directly in markup; do not use gluestack-ui icon component
Applied to files:
src/app/(app)/contacts.tsxsrc/app/onboarding.tsxsrc/components/ui/actionsheet/index.tsxsrc/components/notifications/NotificationInbox.tsxsrc/components/calls/call-images-modal.tsxsrc/app/(app)/notes.tsxsrc/app/(app)/protocols.tsx
📚 Learning: 2025-08-12T03:33:40.238Z
Learnt from: CR
PR: Resgrid/Unit#0
File: .cursorrules:0-0
Timestamp: 2025-08-12T03:33:40.238Z
Learning: Applies to **/*.tsx : Use lucide-react-native for icons directly in markup; do not use the gluestack-ui icon component
Applied to files:
src/app/(app)/contacts.tsxsrc/app/onboarding.tsxsrc/components/ui/actionsheet/index.tsxsrc/components/notifications/NotificationInbox.tsxsrc/components/calls/call-images-modal.tsxsrc/app/(app)/notes.tsxsrc/app/(app)/protocols.tsx
📚 Learning: 2025-08-12T03:33:40.238Z
Learnt from: CR
PR: Resgrid/Unit#0
File: .cursorrules:0-0
Timestamp: 2025-08-12T03:33:40.238Z
Learning: Applies to **/*.tsx : Use getItemLayout for FlatList when items have consistent size
Applied to files:
src/app/onboarding.tsxsrc/components/calls/call-images-modal.tsx
📚 Learning: 2025-08-12T03:34:25.992Z
Learnt from: CR
PR: Resgrid/Unit#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-12T03:34:25.992Z
Learning: Applies to **/*.tsx : Use rnmapbox/maps for maps and navigation
Applied to files:
src/app/onboarding.tsx
📚 Learning: 2025-08-12T03:33:40.238Z
Learnt from: CR
PR: Resgrid/Unit#0
File: .cursorrules:0-0
Timestamp: 2025-08-12T03:33:40.238Z
Learning: Applies to **/*.{test.ts,test.tsx,spec.ts,spec.tsx} : Create and use Jest tests to validate all generated components
Applied to files:
src/components/roles/__tests__/roles-modal.test.tsxjest.config.js
📚 Learning: 2025-08-12T03:34:25.992Z
Learnt from: CR
PR: Resgrid/Unit#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-12T03:34:25.992Z
Learning: Applies to **/*.{test,spec}.{ts,tsx} : Create and use Jest tests for all generated components
Applied to files:
src/components/roles/__tests__/roles-modal.test.tsxjest.config.js
📚 Learning: 2025-08-12T03:34:25.992Z
Learnt from: CR
PR: Resgrid/Unit#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-12T03:34:25.992Z
Learning: Applies to {components/ui/**/*.{ts,tsx},**/*.tsx} : Use gluestack-ui consistently; if no component exists in components/ui, style via StyleSheet.create or styled-components
Applied to files:
src/components/ui/select/select-actionsheet.tsxsrc/components/ui/actionsheet/index.tsx
📚 Learning: 2025-08-12T03:34:25.992Z
Learnt from: CR
PR: Resgrid/Unit#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-12T03:34:25.992Z
Learning: Applies to jest.config.@(js|ts) : Provide Jest configuration
Applied to files:
jest.config.js
📚 Learning: 2025-08-12T03:34:25.992Z
Learnt from: CR
PR: Resgrid/Unit#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-12T03:34:25.992Z
Learning: Applies to **/*.tsx : Optimize image handling using react-native-fast-image
Applied to files:
src/components/calls/call-images-modal.tsx
📚 Learning: 2025-08-12T03:33:40.238Z
Learnt from: CR
PR: Resgrid/Unit#0
File: .cursorrules:0-0
Timestamp: 2025-08-12T03:33:40.238Z
Learning: Applies to **/*.tsx : Optimize images using react-native-fast-image
Applied to files:
src/components/calls/call-images-modal.tsx
🧬 Code graph analysis (8)
__mocks__/@shopify/flash-list.ts (1)
src/components/ui/flat-list/index.tsx (1)
FlashList(2-2)
src/components/roles/__tests__/roles-modal.test.tsx (3)
src/stores/app/core-store.ts (1)
useCoreStore(48-263)src/stores/roles/store.ts (1)
useRolesStore(25-90)src/components/roles/roles-modal.tsx (1)
RolesModal(23-146)
src/components/ui/select/select-actionsheet.tsx (2)
__mocks__/@shopify/flash-list.ts (1)
FlashList(5-7)src/components/ui/flat-list/index.tsx (1)
FlashList(2-2)
src/components/ui/actionsheet/index.tsx (2)
__mocks__/@shopify/flash-list.ts (1)
FlashList(5-7)src/components/ui/flat-list/index.tsx (1)
FlashList(2-2)
src/components/roles/roles-modal.tsx (1)
src/stores/roles/store.ts (1)
useRolesStore(25-90)
src/stores/status/store.ts (2)
src/stores/app/location-store.ts (1)
useLocationStore(22-52)src/lib/logging/index.tsx (2)
error(74-76)logger(80-80)
src/components/status/__tests__/location-update-validation.test.tsx (2)
src/stores/app/location-store.ts (1)
useLocationStore(22-52)src/stores/status/store.ts (1)
useStatusesStore(117-238)
src/components/status/__tests__/gps-coordinate-duplication-fix.test.tsx (2)
src/stores/app/location-store.ts (1)
useLocationStore(22-52)src/stores/status/store.ts (1)
useStatusesStore(117-238)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test
🔇 Additional comments (3)
src/app/(app)/calls.tsx (1)
72-73: Appreciate the extra test coverage hook.The new
testIDmakes it much easier to target this list in our E2E and component tests. 👍jest.config.js (1)
11-12: Good call letting Jest transpile FlashList.Adding
@shopify/flash-listto the allowlist keeps the v2 sources compatible with our Jest environment.src/app/(app)/contacts.tsx (1)
4-13: Thanks for routing this screen through the shared FlashList wrapper.Keeps the list implementation consistent across the app while we migrate away from the core FlatList.
| // CORRECT - Uses AND logic | ||
| if ((!input.Latitude && !input.Longitude) || (input.Latitude === '' && input.Longitude === '')) { | ||
| const locationState = useLocationStore.getState(); | ||
|
|
||
| if (locationState.latitude !== null && locationState.longitude !== null) { | ||
| input.Latitude = locationState.latitude.toString(); | ||
| input.Longitude = locationState.longitude.toString(); | ||
| input.Accuracy = locationState.accuracy?.toString() || ''; | ||
| input.Altitude = locationState.altitude?.toString() || ''; | ||
| input.AltitudeAccuracy = ''; // Added missing field | ||
| input.Speed = locationState.speed?.toString() || ''; | ||
| input.Heading = locationState.heading?.toString() || ''; | ||
| } |
There was a problem hiding this comment.
Clarify AltitudeAccuracy handling
The text says we now populate AltitudeAccuracy, but the sample still assigns an empty string. That doesn’t reflect a real population step and contradicts the earlier description/tests. Please update the doc (and snippet) to show how the field is actually sourced—e.g., using locationState.altitudeAccuracy?.toString() || ''—so the guidance matches the fix.
🤖 Prompt for AI Agents
In docs/gps-coordinate-duplication-fix.md around lines 61 to 73, the sample
assigns input.AltitudeAccuracy = '' which contradicts the text that says we
populate AltitudeAccuracy; update the snippet to source the value from
locationState by replacing the empty-string assignment with code that sets
input.AltitudeAccuracy to locationState.altitudeAccuracy?.toString() || '' so
the example matches the described behavior and tests.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
src/components/notifications/NotificationInbox.tsx (1)
310-310: Verify the estimatedItemSize value matches actual item heights.The
estimatedItemSize={80}prop is essential for FlashList performance, but the actual item height may vary due to:
- Selection mode adds a checkbox indicator (lines 190-194)
- Action buttons vary by notification type (lines 203-214)
- Text content wrapping may affect height
FlashList performs best when
estimatedItemSizeclosely matches the average item height. Consider measuring actual rendered item heights to ensure this value is accurate.Optionally, if items have distinctly different heights in selection mode vs normal mode, consider adding
getItemTypeto help FlashList optimize layout:getItemType={(item, index) => { return isSelectionMode ? 'selection' : 'normal'; }}src/components/roles/__tests__/roles-bottom-sheet.test.tsx (2)
345-464: Consider testing the actual component instead of duplicating logic.This test validates the filtering behavior by creating a
TestComponentthat replicates the save logic fromRolesBottomSheet. While this confirms the filtering algorithm works, it doesn't verify that the actual component implements this logic correctly.Consider refactoring to:
- Render the actual
RolesBottomSheetcomponent- Mock
RoleAssignmentItemto expose user-selection controls- Simulate user interactions (selecting/clearing users)
- Verify the
assignRolescallThis would provide true integration coverage rather than unit-testing the algorithm in isolation.
Example approach:
-const TestComponent = () => { - // ... duplicate component logic ... -}; -render(<TestComponent />); +// Mock RoleAssignmentItem to expose controls +jest.mock('../role-assignment-item', () => ({ + RoleAssignmentItem: ({ role, onAssignUser }: any) => { + const { TouchableOpacity, Text } = require('react-native'); + return ( + <TouchableOpacity + testID={`assign-${role.UnitRoleId}`} + onPress={() => onAssignUser('user1')} + > + <Text>{role.Name}</Text> + </TouchableOpacity> + ); + }, +})); + +render(<RolesBottomSheet isOpen={true} onClose={mockOnClose} />); +// Interact with actual component and verify behavior
546-567: Consider demonstrating this fix through component behavior.This test validates that removing the
UnitIdfilter from the assignment lookup allows finding assignments regardless ofUnitIdmismatch. While clear, it's testing plain JavaScriptfind()logic rather than the component's behavior.Consider either:
- Inline verification: Move this into a comment or inline assertion within a component test
- Component-level test: Render
RolesBottomSheetwith mismatchedUnitIdvalues and verify the assignment displays correctlyExample component-level approach:
it('should display role assignments even when UnitId does not match', () => { const assignmentsWithDifferentUnitId: ActiveUnitRoleResultData[] = [ { UnitRoleId: 'role1', UnitId: 'different-unit', // Mismatch UserId: 'user1', Name: 'Captain', FullName: 'John Doe', UpdatedOn: new Date().toISOString(), }, ]; mockUseRolesStore.mockReturnValue({ roles: mockRoles, unitRoleAssignments: assignmentsWithDifferentUnitId, users: mockUsers, isLoading: false, error: null, fetchRolesForUnit: mockFetchRolesForUnit, fetchUsers: mockFetchUsers, assignRoles: mockAssignRoles, } as any); render(<RolesBottomSheet isOpen={true} onClose={mockOnClose} />); // Verify the assignment is displayed despite UnitId mismatch expect(screen.getByText(/John Doe/)).toBeTruthy(); });src/components/roles/roles-modal.tsx (1)
56-95: Unassignments now reach the backend.The
allUnitRolesderivation correctly allows roles with an emptyUserIdto pass through the filter (Lines 73-76), resolving the regression from the past review where unassignments were blocked.Note: The current implementation sends all roles for the unit (including those that were never assigned) rather than only changed roles. The past review suggested a more granular approach (checking
hadExistingAssignmentto exclude no-op unassignments), which would reduce payload size. Consider that optimization if performance becomes a concern.Optional: More efficient payload (only send changed roles)
The past review comment suggested a more complex filter that only includes roles that either:
- Have a non-empty
UserId(assignments), or- Had an existing assignment that is being cleared (true unassignments)
This would prevent sending no-op unassignments for roles that were never assigned. Apply this diff if you want to optimize the payload:
.filter((role) => { - // Only filter out entries lacking a RoleId - allow empty UserId for unassignments - return role.RoleId && role.RoleId.trim() !== ''; + if (!role.RoleId || role.RoleId.trim() === '') { + return false; + } + if (role.UserId && role.UserId.trim() !== '') { + return true; + } + const hadExistingAssignment = unitRoleAssignments.some( + (assignment) => assignment.UnitRoleId === role.RoleId && assignment.UnitId === activeUnit.UnitId + ); + return hadExistingAssignment; });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (10)
package.json(7 hunks)src/components/calls/call-images-modal.tsx(3 hunks)src/components/notifications/NotificationInbox.tsx(2 hunks)src/components/roles/__tests__/role-assignment-item.test.tsx(1 hunks)src/components/roles/__tests__/roles-bottom-sheet.test.tsx(1 hunks)src/components/roles/__tests__/roles-modal.test.tsx(1 hunks)src/components/roles/roles-bottom-sheet.tsx(2 hunks)src/components/roles/roles-modal.tsx(4 hunks)src/components/status/status-bottom-sheet.tsx(1 hunks)src/stores/roles/store.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/status/status-bottom-sheet.tsx
🧰 Additional context used
📓 Path-based instructions (7)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursorrules)
**/*.{ts,tsx}: Write concise, type-safe TypeScript code
Use camelCase for variable and function names (e.g., isFetchingData, handleUserInput)
Use TypeScript for all components, favoring interfaces for props and state
Avoid using any; strive for precise types
Use Expo SecureStore for sensitive data
Use zustand for state management
Use react-query for data fetching and caching
Use react-i18next for internationalization
Use react-native-mmkv for local storage
Use axios for API requests
**/*.{ts,tsx}: Write concise, type-safe TypeScript code
Use camelCase for variable and function names
Use TypeScript for all components, favoring interfaces for props and state
Avoid using any; strive for precise types
Use React Navigation for handling navigation and deep linking
Handle errors gracefully and provide user feedback
Use Expo's SecureStore for sensitive data
Use zustand for state management
Use react-query for data fetching
Use react-i18next for internationalization
Use react-native-mmkv for local storage
Use axios for API requests
Files:
src/components/calls/call-images-modal.tsxsrc/components/notifications/NotificationInbox.tsxsrc/components/roles/roles-modal.tsxsrc/components/roles/roles-bottom-sheet.tsxsrc/stores/roles/store.tssrc/components/roles/__tests__/roles-bottom-sheet.test.tsxsrc/components/roles/__tests__/role-assignment-item.test.tsxsrc/components/roles/__tests__/roles-modal.test.tsx
**/*.tsx
📄 CodeRabbit inference engine (.cursorrules)
**/*.tsx: Use functional components and hooks over class components
Use PascalCase for React component names (e.g., UserProfile, ChatScreen)
Utilize React.FC for defining functional components with props
Minimize useEffect, useState, and heavy computations inside render
Use React.memo() for components with static props to prevent unnecessary re-renders
Optimize FlatList with removeClippedSubviews, maxToRenderPerBatch, windowSize
Use getItemLayout for FlatList when items have consistent size
Avoid anonymous functions in renderItem or event handlers to prevent re-renders
Use gluestack-ui for styling; if no component exists in src/components/ui, style via StyleSheet.create or styled-components
Optimize images using react-native-fast-image
Use React Navigation for navigation and deep linking with best practices
Wrap all user-facing text in t() from react-i18next
Use react-hook-form for form handling
Use @rnmapbox/maps for maps or vehicle navigation
Use lucide-react-native for icons directly in markup; do not use the gluestack-ui icon component
Use the conditional operator (?:) for conditional rendering instead of &&
**/*.tsx: Use functional components and hooks over class components
Use PascalCase for component names
Utilize React.FC for defining functional components with props
Minimize useEffect/useState and heavy computations inside render
Use React.memo for components with static props to prevent unnecessary re-renders
Optimize FlatList with removeClippedSubviews, maxToRenderPerBatch, and windowSize
Use getItemLayout for FlatList when items have consistent size
Avoid anonymous functions in renderItem or event handlers
Optimize image handling using react-native-fast-image
Wrap all user-facing text in t() from react-i18next
Ensure support for dark mode and light mode
Ensure the app is accessible following WCAG for mobile
Use react-hook-form for form handling
Use @rnmapbox/maps for maps and navigation
Use lucide-react-native for icons directly in markup; do not us...
Files:
src/components/calls/call-images-modal.tsxsrc/components/notifications/NotificationInbox.tsxsrc/components/roles/roles-modal.tsxsrc/components/roles/roles-bottom-sheet.tsxsrc/components/roles/__tests__/roles-bottom-sheet.test.tsxsrc/components/roles/__tests__/role-assignment-item.test.tsxsrc/components/roles/__tests__/roles-modal.test.tsx
**/*
📄 CodeRabbit inference engine (.cursorrules)
Directory and file names should be lowercase and hyphenated (e.g., user-profile, chat-screen)
Files:
src/components/calls/call-images-modal.tsxsrc/components/notifications/NotificationInbox.tsxsrc/components/roles/roles-modal.tsxsrc/components/roles/roles-bottom-sheet.tsxsrc/stores/roles/store.tssrc/components/roles/__tests__/roles-bottom-sheet.test.tsxsrc/components/roles/__tests__/role-assignment-item.test.tsxsrc/components/roles/__tests__/roles-modal.test.tsxpackage.json
{components/ui/**/*.{ts,tsx},**/*.tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use gluestack-ui consistently; if no component exists in components/ui, style via StyleSheet.create or styled-components
Files:
src/components/calls/call-images-modal.tsxsrc/components/notifications/NotificationInbox.tsxsrc/components/roles/roles-modal.tsxsrc/components/roles/roles-bottom-sheet.tsxsrc/components/roles/__tests__/roles-bottom-sheet.test.tsxsrc/components/roles/__tests__/role-assignment-item.test.tsxsrc/components/roles/__tests__/roles-modal.test.tsx
**/*.{test.ts,test.tsx,spec.ts,spec.tsx}
📄 CodeRabbit inference engine (.cursorrules)
Create and use Jest tests to validate all generated components
Files:
src/components/roles/__tests__/roles-bottom-sheet.test.tsxsrc/components/roles/__tests__/role-assignment-item.test.tsxsrc/components/roles/__tests__/roles-modal.test.tsx
**/__tests__/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursorrules)
Generate tests for all components, services, and logic; ensure tests run without errors
Files:
src/components/roles/__tests__/roles-bottom-sheet.test.tsxsrc/components/roles/__tests__/role-assignment-item.test.tsxsrc/components/roles/__tests__/roles-modal.test.tsx
**/*.{test,spec}.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Create and use Jest tests for all generated components
Files:
src/components/roles/__tests__/roles-bottom-sheet.test.tsxsrc/components/roles/__tests__/role-assignment-item.test.tsxsrc/components/roles/__tests__/roles-modal.test.tsx
🧠 Learnings (9)
📚 Learning: 2025-08-12T03:34:25.992Z
Learnt from: CR
PR: Resgrid/Unit#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-12T03:34:25.992Z
Learning: Applies to **/*.tsx : Optimize image handling using react-native-fast-image
Applied to files:
src/components/calls/call-images-modal.tsx
📚 Learning: 2025-08-12T03:34:25.992Z
Learnt from: CR
PR: Resgrid/Unit#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-12T03:34:25.992Z
Learning: Applies to **/*.tsx : Optimize FlatList with removeClippedSubviews, maxToRenderPerBatch, and windowSize
Applied to files:
src/components/calls/call-images-modal.tsxsrc/components/notifications/NotificationInbox.tsx
📚 Learning: 2025-08-12T03:33:40.238Z
Learnt from: CR
PR: Resgrid/Unit#0
File: .cursorrules:0-0
Timestamp: 2025-08-12T03:33:40.238Z
Learning: Applies to **/*.tsx : Optimize FlatList with removeClippedSubviews, maxToRenderPerBatch, windowSize
Applied to files:
src/components/calls/call-images-modal.tsxsrc/components/notifications/NotificationInbox.tsx
📚 Learning: 2025-08-12T03:33:40.238Z
Learnt from: CR
PR: Resgrid/Unit#0
File: .cursorrules:0-0
Timestamp: 2025-08-12T03:33:40.238Z
Learning: Applies to **/*.tsx : Optimize images using react-native-fast-image
Applied to files:
src/components/calls/call-images-modal.tsx
📚 Learning: 2025-08-12T03:34:25.992Z
Learnt from: CR
PR: Resgrid/Unit#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-12T03:34:25.992Z
Learning: Applies to **/*.tsx : Use lucide-react-native for icons directly in markup; do not use gluestack-ui icon component
Applied to files:
src/components/calls/call-images-modal.tsxsrc/components/notifications/NotificationInbox.tsx
📚 Learning: 2025-08-12T03:33:40.238Z
Learnt from: CR
PR: Resgrid/Unit#0
File: .cursorrules:0-0
Timestamp: 2025-08-12T03:33:40.238Z
Learning: Applies to **/*.tsx : Use lucide-react-native for icons directly in markup; do not use the gluestack-ui icon component
Applied to files:
src/components/calls/call-images-modal.tsxsrc/components/notifications/NotificationInbox.tsx
📚 Learning: 2025-08-12T03:33:40.238Z
Learnt from: CR
PR: Resgrid/Unit#0
File: .cursorrules:0-0
Timestamp: 2025-08-12T03:33:40.238Z
Learning: Applies to **/*.tsx : Use getItemLayout for FlatList when items have consistent size
Applied to files:
src/components/calls/call-images-modal.tsxsrc/components/notifications/NotificationInbox.tsx
📚 Learning: 2025-08-12T03:33:40.238Z
Learnt from: CR
PR: Resgrid/Unit#0
File: .cursorrules:0-0
Timestamp: 2025-08-12T03:33:40.238Z
Learning: Applies to **/*.{test.ts,test.tsx,spec.ts,spec.tsx} : Create and use Jest tests to validate all generated components
Applied to files:
src/components/roles/__tests__/role-assignment-item.test.tsxsrc/components/roles/__tests__/roles-modal.test.tsx
📚 Learning: 2025-08-12T03:34:25.992Z
Learnt from: CR
PR: Resgrid/Unit#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-12T03:34:25.992Z
Learning: Applies to **/*.{test,spec}.{ts,tsx} : Create and use Jest tests for all generated components
Applied to files:
src/components/roles/__tests__/roles-modal.test.tsx
🧬 Code graph analysis (3)
src/components/roles/roles-modal.tsx (2)
src/stores/roles/store.ts (1)
useRolesStore(25-90)src/components/roles/role-assignment-item.tsx (1)
RoleAssignmentItem(20-51)
src/components/roles/__tests__/role-assignment-item.test.tsx (1)
src/components/roles/role-assignment-item.tsx (1)
RoleAssignmentItem(20-51)
src/components/roles/__tests__/roles-modal.test.tsx (3)
src/stores/app/core-store.ts (1)
useCoreStore(48-263)src/stores/roles/store.ts (1)
useRolesStore(25-90)src/components/roles/roles-modal.tsx (1)
RolesModal(23-157)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test
🔇 Additional comments (25)
src/components/calls/call-images-modal.tsx (2)
8-8: LGTM! Import migration aligns with FlashList adoption.The import change correctly reflects the migration from React Native's FlatList to the local FlashList-based component.
Also applies to: 13-13
386-408: Verify FlashList performance meets requirements.The migration from React Native FlatList to FlashList replaces multiple RN-specific optimization props (getItemLayout, removeClippedSubviews, maxToRenderPerBatch, windowSize) with
estimatedItemSize. While FlashList uses a different optimization approach (view recycling), please verify:
- The horizontal scrolling performance meets requirements for image galleries
- Consider adding
drawDistanceif you notice rendering issues at the edges- Since items have consistent size (full width), the estimatedItemSize={width} is appropriate
Based on learnings: The coding guidelines emphasize FlatList optimizations (removeClippedSubviews, maxToRenderPerBatch, windowSize, getItemLayout), though FlashList achieves similar performance through different mechanisms.
src/components/notifications/NotificationInbox.tsx (2)
301-301: Good addition of testID for testing.Adding
testID="notifications-list"improves testability of the notification list component.
10-10: Wrapper verified: correctly exports FlashList.The wrapper at
src/components/ui/flat-list/index.tsxre-exports Shopify's FlashList as FlatList. The implementation includes all required props (data, renderItem, estimatedItemSize) and follows FlashList best practices.src/stores/roles/store.ts (1)
59-59: LGTM! Consistent loading state management.Explicitly clearing
isLoadingwhen updatingunitRoleAssignmentsensures consistency with other async methods in the store and prevents stale loading states in the UI.src/components/roles/__tests__/roles-bottom-sheet.test.tsx (1)
466-544: Same concern: test duplicates component logic.This test validates whitespace filtering by replicating the component's save logic. Apply the same refactoring suggestion from the previous test to verify the actual
RolesBottomSheetimplementation.src/components/roles/roles-bottom-sheet.tsx (2)
146-146: Consistent with save logic, same verification needed.The removal of
UnitIdfilter here matches the change in the save logic (line 85). This ensures the component displays and saves assignments using the same lookup strategy.However, the same verification for
UnitRoleIduniqueness applies here. IfUnitRoleIdis not globally unique, the component could display assignments from incorrect units.
81-97: Removal of UnitId filter is correct.The removal of the
UnitIdfilter on lines 85 and 146 is intentional and correct. TheunitRoleAssignmentsarray is fetched viagetRoleAssignmentsForUnit(unitId)(line 38), which returns assignments for a specific unit. However, the API can return assignments with mismatched or missingUnitIdfields, makingUnitRoleIdthe reliable identifier. The test at lines 560-567 inroles-bottom-sheet.test.tsxexplicitly validates this scenario with the comment "The old logic would fail to find this assignment due to UnitId mismatch."Additionally,
filteredRoles(line 73) already filters byactiveUnit.UnitId, ensuring only the active unit's roles are processed.Note:
roles-modal.tsxstill uses theUnitIdfilter (line 64), creating an inconsistency. Consider removing it there as well for consistency.Optional: Minor optimization.
Consider filtering
filteredRolesupfront to exclude roles without validUnitRoleId:const allUnitRoles = filteredRoles + .filter((role) => role.UnitRoleId && role.UnitRoleId.trim() !== '') .map((role) => { const pendingAssignment = pendingAssignments.find((a) => a.roleId === role.UnitRoleId); const currentAssignment = unitRoleAssignments.find((a) => a.UnitRoleId === role.UnitRoleId); const assignedUserId = pendingAssignment?.userId || currentAssignment?.UserId || ''; return { RoleId: role.UnitRoleId, UserId: assignedUserId, Name: '', }; }) .filter((role) => { - // Only include roles that have valid RoleId and assigned UserId - return role.RoleId && role.RoleId.trim() !== '' && role.UserId && role.UserId.trim() !== ''; + // Only include roles that have assigned users + return role.UserId && role.UserId.trim() !== ''; });src/components/roles/roles-modal.tsx (5)
41-47: handleAssignUser now correctly tracks unassignments.The callback properly preserves
undefinedor emptyuserIdvalues inpendingAssignments, enabling unassignments to be tracked. The past review concern about blocking unassignments is partially addressed here.
49-51: LGTM: Unit-scoped role filtering.Memoization correctly filters roles by the active unit and prevents unnecessary recalculations.
53-53: LGTM: Save enablement logic.The
hasChangesflag correctly tracks whether there are pending modifications.
115-139: LGTM: Rendering correctly resolves pending and persisted assignments.The rendering logic properly merges
unitRoleAssignmentsandpendingAssignmentsto compute the current state for each role, ensuring thatRoleAssignmentItemreceives accurate data for filtering and display.
149-149: LGTM: Save button correctly disabled when no changes.The button is properly disabled during loading or when there are no pending changes.
src/components/roles/__tests__/role-assignment-item.test.tsx (3)
9-71: LGTM: Test mocks are appropriate for the component.The mocks correctly simulate i18n and UI components for rendering tests. The
Selectmock is simplified but sufficient for validating rendering and filtering logic.
73-127: LGTM: Test data is complete and realistic.Mock role and users have all required fields properly populated for comprehensive testing.
133-234: LGTM: Comprehensive test coverage for RoleAssignmentItem.The test suite correctly validates:
- Role name rendering
- Placeholder display
- Assigned user display
- User filtering logic (excludes users assigned to other roles, includes users assigned to the same role)
- Unassigned option display
All assertions align with the component's expected behavior.
src/components/roles/__tests__/roles-modal.test.tsx (4)
1-61: LGTM: Comprehensive mocks for unit testing RolesModal.All dependencies (stores, UI components, i18n, logger) are properly mocked to isolate the component under test.
69-137: LGTM: Complete and realistic test data.Mock data includes all required fields for unit, roles, users, and assignments, enabling comprehensive testing of the modal's behavior.
139-168: LGTM: Proper test setup with isolated mocks.The
beforeEachhook correctly resets mocks and initializes store states for each test, ensuring test isolation.
170-251: LGTM: Thorough test coverage for RolesModal core functionality.The test suite correctly validates:
- Rendering when open/closed
- Lifecycle methods (fetchRolesForUnit, fetchUsers) invoked on open
- Role assignment items rendered
- Error state display
- Graceful handling of missing active unit
- Unit-scoped role filtering
All assertions align with the component's expected behavior.
package.json (5)
100-100: Verify axios behavior after security upgrade.The axios update from ~1.7.5 to ~1.12.0 addresses two security vulnerabilities:
- CVE-2025-27152 (SSRF/credential leakage via absolute URLs, fixed in 1.8.2)
- DoS via data: URI (fixed in 1.12.0 by enforcing
maxContentLengthfor data: URLs)No breaking changes are documented in the v1.12.0 release notes, but test your axios usage (especially if using data: URIs or custom adapters) to ensure the security fixes don't affect your application's behavior.
244-246: Security fix confirmed - form-data resolution is correct.The resolution pins
form-datato version 4.0.4, which addresses a CRITICAL vulnerability (CVE-2025-7783) affecting versions >= 4.0.0 and < 4.0.4. The vulnerability involves an unsafe random function for choosing multipart boundaries, enabling potential HTTP parameter pollution attacks. Version 4.0.4 is the patched release and the latest stable version.This resolution ensures all transitive dependencies use the secure version.
146-146: React Native 0.79.6: patch bump contains bug fixes.The update from 0.79.5 to 0.79.6 includes fixes for Legacy Architecture crash/freeze on reload, Modal rendering issues, and TurboModule crashes on 32-bit Android, along with minor enterprise repository support additions. This is a standard patch release with no breaking changes—safe to proceed.
87-89: Verify LiveKit native integration after major WebRTC engine upgrade.The LiveKit packages are compatible with each other, but @livekit/react-native-webrtc jumped from M125 to M137 WebRTC engine, introducing breaking changes:
- Native ABI, codec support (M137 adds HEVC/H.265), and platform behavior changes
- iOS/Podspec and native dependency changes required; black-screen issues reported with M125 builds
- Native module rebuild needed after upgrade
Additionally, switching from tilde (~) to caret (^) ranges permits wider patch-level updates that could introduce instability.
Action required:
- Rebuild native modules (
npx expo prebuild --clean)- Verify iOS Podspec and Android Gradle configurations
- Test video/audio functionality thoroughly on both platforms
- Watch for black-screen or codec-related regressions
Also applies to: 135-135
97-97: FlashList 1.7.6 is appropriate—no concerns.Version 1.7.6 was published March 19, 2025, making it a recent release, and no security vulnerabilities are reported for recent 1.x releases. This version avoids the new-architecture requirement that v2 introduces, which aligns with your React Native 0.79.6 setup running the old architecture. The choice to use 1.7.6 resolves the critical blocker from the previous review.
| const [imageErrors, setImageErrors] = useState<Set<string>>(new Set()); | ||
| const [fullScreenImage, setFullScreenImage] = useState<{ source: any; name?: string } | null>(null); | ||
| const flatListRef = useRef<FlatList>(null); | ||
| const flatListRef = useRef<any>(null); // FlashList ref type |
There was a problem hiding this comment.
Replace any with proper FlashList ref type.
The ref type has been weakened from a specific FlatList type to any, which loses compile-time type safety for methods like scrollToIndex. Import the proper ref type from @shopify/flash-list or the local wrapper.
As per coding guidelines, avoid any and strive for precise types. Apply this diff:
+import type { FlashList } from '@shopify/flash-list';
+
- const flatListRef = useRef<any>(null); // FlashList ref type
+ const flatListRef = useRef<FlashList<CallFileResultData>>(null);Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/components/calls/call-images-modal.tsx around line 62, the ref is typed
as useRef<any>(null) which weakens type safety and prevents proper typing of
FlashList instance methods; replace any with the appropriate FlashList ref type
(import the FlashList or FlashListRef type from '@shopify/flash-list' or your
local wrapper) and declare the ref as useRef<FlashList<T> | null>(null) or
useRef<FlashListRef<T> | null>(null) using the correct item type T for this list
so methods like scrollToIndex are correctly typed; update the import and the
useRef generic accordingly and adjust any call sites if needed to handle the
possibly null ref.
| describe('Empty RoleId prevention', () => { | ||
| it('should filter out roles with empty or whitespace RoleId and UserId', () => { | ||
| const { fireEvent } = require('@testing-library/react-native'); | ||
|
|
||
| render(<RolesModal isOpen={true} onClose={mockOnClose} />); | ||
|
|
||
| // The component should render without errors | ||
| expect(screen.getByText('Unit Role Assignments')).toBeTruthy(); | ||
| expect(screen.getByText('Save')).toBeTruthy(); | ||
| }); | ||
|
|
||
| it('should handle save with empty assignments gracefully', async () => { | ||
| const { fireEvent } = require('@testing-library/react-native'); | ||
|
|
||
| // Mock the save to resolve successfully even with empty roles | ||
| mockAssignRoles.mockResolvedValueOnce({}); | ||
|
|
||
| render(<RolesModal isOpen={true} onClose={mockOnClose} />); | ||
|
|
||
| // Try to save - should not throw error even if no pending assignments | ||
| const saveButton = screen.getByText('Save'); | ||
| fireEvent.press(saveButton); | ||
|
|
||
| // Component should still be functional | ||
| expect(screen.getByText('Unit Role Assignments')).toBeTruthy(); | ||
| }); | ||
|
|
||
| it('should allow unassignments by including roles with valid RoleId but empty UserId', () => { | ||
| // Test the filter logic that should allow unassignments | ||
| const testRoles = [ | ||
| { RoleId: 'role-1', UserId: 'user-1', Name: '' }, // Valid assignment | ||
| { RoleId: 'role-2', UserId: '', Name: '' }, // Valid unassignment - should pass through | ||
| { RoleId: '', UserId: 'user-3', Name: '' }, // Invalid - no RoleId, should be filtered out | ||
| { RoleId: ' ', UserId: 'user-4', Name: '' }, // Invalid - whitespace RoleId, should be filtered out | ||
| ]; | ||
|
|
||
| const filteredRoles = testRoles.filter((role) => { | ||
| // Only filter out entries lacking a RoleId - allow empty UserId for unassignments | ||
| return role.RoleId && role.RoleId.trim() !== ''; | ||
| }); | ||
|
|
||
| expect(filteredRoles).toHaveLength(2); | ||
| expect(filteredRoles[0]).toEqual({ RoleId: 'role-1', UserId: 'user-1', Name: '' }); | ||
| expect(filteredRoles[1]).toEqual({ RoleId: 'role-2', UserId: '', Name: '' }); // Unassignment should be included | ||
| }); | ||
|
|
||
| it('should track pending removals and assignments properly', () => { | ||
| render(<RolesModal isOpen={true} onClose={mockOnClose} />); | ||
|
|
||
| // The component should track pending assignments including removals (empty UserId) | ||
| // This ensures that unassignments reach the assignRoles API call | ||
| expect(screen.getByText('Unit Role Assignments')).toBeTruthy(); | ||
| }); | ||
|
|
||
| it('should find role assignments without UnitId filter', () => { | ||
| // Test that demonstrates the fix - assignments should be found without the UnitId filter | ||
| const testRoleAssignments = [ | ||
| { | ||
| UnitRoleId: 'role1', | ||
| UnitId: '', // UnitId might be empty or different in the API response | ||
| Name: 'Captain', | ||
| UserId: 'user1', | ||
| FullName: 'John Doe', | ||
| UpdatedOn: new Date().toISOString(), | ||
| }, | ||
| ]; | ||
|
|
||
| // The old logic would fail to find this assignment due to UnitId mismatch | ||
| const assignmentWithUnitIdFilter = testRoleAssignments.find((a) => a.UnitRoleId === 'role1' && a.UnitId === 'unit1'); | ||
| expect(assignmentWithUnitIdFilter).toBeUndefined(); | ||
|
|
||
| // The new logic should find this assignment | ||
| const assignmentWithoutUnitIdFilter = testRoleAssignments.find((a) => a.UnitRoleId === 'role1'); | ||
| expect(assignmentWithoutUnitIdFilter).toBeDefined(); | ||
| expect(assignmentWithoutUnitIdFilter?.UserId).toBe('user1'); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Inconsistency: Test expectation doesn't match component implementation.
The test at lines 307-328 documents that role assignments should be found without a UnitId filter, and the comment states "The old logic would fail to find this assignment due to UnitId mismatch" and "The new logic should find this assignment."
However, examining src/components/roles/roles-modal.tsx at line 64, the component still filters by UnitId:
const currentAssignment = unitRoleAssignments.find((a) => a.UnitRoleId === role.UnitRoleId && a.UnitId === activeUnit.UnitId);This creates two possibilities:
- The test is incorrect: It documents a change that wasn't actually made to the component.
- The component is incomplete: The intended fix (removing the
UnitIdfilter fromcurrentAssignmentlookup) was not applied.
If the UnitId filter is causing issues (e.g., backend returns assignments with empty or different UnitId), the component should be updated to match the test's expectation:
- const currentAssignment = unitRoleAssignments.find((a) => a.UnitRoleId === role.UnitRoleId && a.UnitId === activeUnit.UnitId);
+ const currentAssignment = unitRoleAssignments.find((a) => a.UnitRoleId === role.UnitRoleId);Otherwise, if the UnitId filter is correct, remove or update the test to reflect the actual implementation.
Additional note on test 3 (lines 280-297):
This test is a unit test of the filter logic (pure function test) rather than a component integration test. While it clearly documents the expected filtering behavior, consider moving it to a separate utility function test file if the logic is extracted, or convert it to a component integration test that exercises the filter through user interactions.
|
Approve |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
… location.
Summary by CodeRabbit