CU-868f4q2qr Adding server switch to login form, eventing url change. #155
Conversation
|
Task linked: CU-868f4q2qr Fix issues with Call Images |
|
| GitGuardian id | GitGuardian status | Secret | Commit | Filename | |
|---|---|---|---|---|---|
| 20215389 | Triggered | Bearer Token | 83bb851 | src/services/tests/signalr.service.test.ts | View secret |
🛠 Guidelines to remediate hardcoded secrets
- Understand the implications of revoking this secret by investigating where it is used in your code.
- Replace and store your secret safely. Learn here the best practices.
- Revoke and rotate this secret.
- If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.
To avoid such incidents in the future consider
- following these best practices for managing and storing secrets including API keys and other credentials
- install secret detection on pre-commit to catch secret before it leaves your machine and ease remediation.
🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.
WalkthroughAdds Android PR-body-driven release steps to CI, an iOS icon, removes CHANNEL_API_URL from public env, expands GetConfigResultData with eight fields, makes CoreStore fetch config during init, implements EventingUrl-based SignalR connections with reconnection, adds Login server-URL UI and many tests/mocks. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as Caller
participant Store as SignalR Store
participant Core as Core Store
participant Svc as SignalR Service
participant Hub as SignalR Hub
UI->>Store: connectUpdateHub()
Store->>Core: getState().config.EventingUrl
alt EventingUrl missing
Store-->>UI: set error, log, abort
else EventingUrl present
Store->>Svc: connectToHubWithEventingUrl({ name, eventingUrl, hubName, methods })
Svc->>Svc: build full hub URL (append hubName)
alt hubName == REALTIME_GEO_HUB_NAME
Svc->>Hub: start with access_token in URL
else
Svc->>Hub: start with access token via header/tokenFactory
end
Hub-->>Svc: connected
Svc-->>Store: mark connected
end
Hub-->>Svc: onclose
Svc->>Svc: refresh token -> reconnect using stored hubConfig (delay/backoff)
sequenceDiagram
participant App as App
participant Core as CoreStore
participant API as getConfig API
App->>Core: init()
Core->>Core: fetchConfig()
Core->>API: getConfig()
alt success
API-->>Core: config data
Core-->>App: continue init
else error
API-->>Core: error
Core-->>App: throw/reject "Failed to fetch config"
end
sequenceDiagram
participant User as User
participant Login as Login Screen
participant Sheet as ServerUrlBottomSheet
User->>Login: tap "Server URL"
Login->>Sheet: open (isOpen=true)
User->>Sheet: close
Sheet->>Login: onClose()
Login->>Sheet: isOpen=false
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (20)
.github/workflows/react-native-cicd.yml (1)
279-299: Consider a dedicated release job to aggregate artifacts from both platformsCurrent approach limits artifacts to Android and couples release creation to one matrix leg. A separate job can download both artifacts and publish a single release consistently.
Example pattern:
create-release: needs: build-and-deploy if: ${{ github.event_name != 'pull_request' }} # or your desired trigger runs-on: ubuntu-latest steps: - uses: actions/download-artifact@v4 with: name: app-builds-android path: dist/android - uses: actions/download-artifact@v4 with: name: app-builds-ios path: dist/ios - name: 📦 Create GitHub Release uses: ncipollo/release-action@v1 with: tag: "7.${{ github.run_number }}" commit: ${{ github.sha }} makeLatest: true allowUpdates: true name: "7.${{ github.run_number }}" artifacts: | dist/android/ResgridUnit-prod.apk dist/android/ResgridUnit-prod.aab dist/ios/ResgridUnit-ios-adhoc.ipa dist/ios/ResgridUnit-ios-prod.ipa bodyFile: "RELEASE_NOTES.md"jest-setup.ts (1)
159-176: Consider using a more complete zod mock implementation.While the current mock works for basic cases, it doesn't handle many zod features like
email(),optional(),nullable(),refine(), or schema composition. This could lead to tests passing when they shouldn't if the actual validation logic relies on these features.Consider enhancing the mock to support more zod features:
jest.mock('zod', () => ({ z: { object: jest.fn(() => ({ parse: jest.fn((data) => data), safeParse: jest.fn((data) => ({ success: true, data })), + shape: {}, + partial: jest.fn(() => ({ + parse: jest.fn((data) => data), + safeParse: jest.fn((data) => ({ success: true, data })), + })), })), string: jest.fn(() => ({ min: jest.fn(() => ({ parse: jest.fn((data) => data), safeParse: jest.fn((data) => ({ success: true, data })), })), + email: jest.fn(() => ({ + parse: jest.fn((data) => data), + safeParse: jest.fn((data) => ({ success: true, data })), + })), + optional: jest.fn(() => ({ + parse: jest.fn((data) => data), + safeParse: jest.fn((data) => ({ success: true, data })), + })), parse: jest.fn((data) => data), safeParse: jest.fn((data) => ({ success: true, data })), })), + number: jest.fn(() => ({ + parse: jest.fn((data) => data), + safeParse: jest.fn((data) => ({ success: true, data })), + })), + boolean: jest.fn(() => ({ + parse: jest.fn((data) => data), + safeParse: jest.fn((data) => ({ success: true, data })), + })), + infer: jest.fn(), }, __esModule: true, }));src/models/v4/configs/getConfigResultData.ts (2)
4-4: Consider making EventingUrl a required field with validation.The
EventingUrlfield is crucial for SignalR connectivity. Having it default to an empty string could lead to runtime failures if not properly configured.Consider adding runtime validation or making this field required to fail early:
- public EventingUrl: string = ''; + public EventingUrl: string; // Make required + + constructor(data?: Partial<GetConfigResultData>) { + // Validate EventingUrl is provided + if (!data?.EventingUrl) { + throw new Error('EventingUrl is required for SignalR connectivity'); + } + Object.assign(this, data); + }
10-13: Consider using more descriptive names for location tracking parameters.The parameter names could be clearer about their purpose and units.
Consider renaming for clarity:
- public PersonnelLocationStaleSeconds: number = 300; - public UnitLocationStaleSeconds: number = 300; - public PersonnelLocationMinMeters: number = 15; - public UnitLocationMinMeters: number = 15; + public PersonnelLocationStaleTimeoutSeconds: number = 300; + public UnitLocationStaleTimeoutSeconds: number = 300; + public PersonnelLocationMinDistanceMeters: number = 15; + public UnitLocationMinDistanceMeters: number = 15;src/services/signalr.service.ts (1)
67-71: Consider extracting the token parameter logic for maintainability.The special handling for the geolocation hub could be made more maintainable by extracting it into a separate method.
Consider extracting this logic:
+ private buildHubUrl(eventingUrl: string, hubName: string, token: string): string { + const normalizedEventingUrl = eventingUrl.endsWith('/') ? eventingUrl : `${eventingUrl}/`; + let fullUrl = `${normalizedEventingUrl}${hubName}`; + + // For geolocation hub, add token as URL parameter instead of header + const isGeolocationHub = hubName === Env.REALTIME_GEO_HUB_NAME; + if (isGeolocationHub) { + const separator = fullUrl.includes('?') ? '&' : '?'; + fullUrl = `${fullUrl}${separator}access_token=${encodeURIComponent(token)}`; + } + + return fullUrl; + } public async connectToHubWithEventingUrl(config: SignalRHubConnectConfig): Promise<void> { // ... validation code ... - // Ensure EventingUrl has a trailing slash - const normalizedEventingUrl = config.eventingUrl.endsWith('/') ? config.eventingUrl : `${config.eventingUrl}/`; - - let fullUrl = `${normalizedEventingUrl}${config.hubName}`; - - // For geolocation hub, add token as URL parameter instead of header - const isGeolocationHub = config.hubName === Env.REALTIME_GEO_HUB_NAME; - if (isGeolocationHub) { - const separator = fullUrl.includes('?') ? '&' : '?'; - fullUrl = `${fullUrl}${separator}access_token=${encodeURIComponent(token)}`; - } + const fullUrl = this.buildHubUrl(config.eventingUrl, config.hubName, token); + const isGeolocationHub = config.hubName === Env.REALTIME_GEO_HUB_NAME;src/app/call/new/__tests__/address-search.test.ts (1)
151-169: Consistent fixture shape for “no key” path.Mirrors the added fields in mockConfig, ensuring both presence/absence scenarios stay in sync with the model. Looks good.
To reduce duplication across tests (address/plus-code/coordinates/what3words), consider a small test factory:
// e.g., src/test-utils/config.ts import { type GetConfigResultData } from '@/models/v4/configs/getConfigResultData'; export const createConfig = (overrides?: Partial<GetConfigResultData>): GetConfigResultData => ({ GoogleMapsKey: '', W3WKey: '', EventingUrl: '', LoggingKey: '', MapUrl: '', MapAttribution: '', OpenWeatherApiKey: '', DirectionsMapKey: '', PersonnelLocationStaleSeconds: 300, UnitLocationStaleSeconds: 300, PersonnelLocationMinMeters: 15, UnitLocationMinMeters: 15, NovuBackendApiUrl: '', NovuSocketUrl: '', NovuApplicationId: '', AnalyticsApiKey: '', AnalyticsHost: '', ...overrides, });Then in tests:
const mockConfig = createConfig({ GoogleMapsKey: 'test-api-key' });src/app/call/new/__tests__/what3words.test.tsx (1)
10-28: Updated config fixture fields align with GetConfigResultData expansion.EventingUrl, DirectionsMapKey, PersonnelLocationStaleSeconds, UnitLocationStaleSeconds, PersonnelLocationMinMeters, UnitLocationMinMeters, AnalyticsApiKey, AnalyticsHost are correctly included with expected defaults. Good parity with other test suites.
Consider reusing a shared test factory for config objects (see suggestion in address-search tests) to DRY up fixtures across the suite.
src/app/login/index.tsx (1)
63-63: Wiring Server URL button to open the bottom sheet looks good; consider stabilizing the handler.Passing an inline arrow function is fine here; to avoid re-renders you could wrap it with useCallback.
Example:
// Add to imports import React, { useEffect, useState, useCallback } from 'react'; // Inside component: const handleServerUrlPress = useCallback(() => setShowServerUrl(true), []); // Usage: <LoginForm onSubmit={onSubmit} isLoading={status === 'loading'} error={error ?? undefined} onServerUrlPress={handleServerUrlPress} />src/app/call/new/__tests__/plus-code-search.test.ts (1)
124-142: Consistent field additions for the “missing key” config.Good to see both positive and negative config paths sharing the same shape.
Same DRY suggestion as in address-search: extract a small
createConfigfactory for reuse across tests.src/stores/signalr/signalr-store.ts (4)
54-61: Connection via EventingUrl is in place; verify name vs hubName semantics and handler idempotency.
- Double-check that
nameandhubNameshould be identical. Ifnameis a local alias andhubNameis the server hub identifier, equality is fine only if they’re meant to be identical. Otherwise, pass distinct values accordingly.- Ensure event handlers aren’t registered multiple times across reconnect attempts. If
connectToHubWithEventingUrlordisconnectFromHubdoesn’t clear handlers, consider removing existing listeners before re-registering.If
signalRService.offis available, remove prior handlers before adding:// Before registering handlers (example names) signalRService.off('personnelStatusUpdated'); signalRService.off('personnelStaffingUpdated'); signalRService.off('unitStatusUpdated'); signalRService.off('callsUpdated'); signalRService.off('callAdded'); signalRService.off('callClosed'); signalRService.off('onConnected');Alternatively, guard with a local “isConnecting” flag to avoid concurrent connect calls adding duplicate listeners.
62-62: Specify radix in parseInt to avoid edge cases.Apply this diff:
- await signalRService.invoke(Env.CHANNEL_HUB_NAME, 'connect', parseInt(securityStore.getState().rights?.DepartmentId ?? '0')); + await signalRService.invoke( + Env.CHANNEL_HUB_NAME, + 'connect', + parseInt(securityStore.getState().rights?.DepartmentId ?? '0', 10) + );
120-127: Clarify error message context.This catch pertains to the update hub specifically; naming it helps when scanning logs.
- message: 'Failed to connect to SignalR hubs', + message: 'Failed to connect to update SignalR hub',
187-194: Clarify geolocation hub error message.- message: 'Failed to connect to SignalR hubs', + message: 'Failed to connect to geolocation SignalR hub',src/stores/app/__tests__/core-store.test.ts (2)
182-206: Add more comprehensive test coverage for fetchConfig.While the success path test covers the basic scenario, consider adding tests for:
- Config data with missing fields
- Config data with invalid types
- Verifying that all expected fields are correctly stored
Consider adding these additional test cases:
+ it('should handle config with missing fields gracefully', async () => { + mockGetActiveUnitId.mockReturnValue(null); + mockGetActiveCallId.mockReturnValue(null); + + const partialConfigData = { + EventingUrl: 'https://eventing.example.com/', + // Missing other fields + } as GetConfigResultData; + + mockGetConfig.mockResolvedValue({ + Data: partialConfigData, + } as any); + + const { result } = renderHook(() => useCoreStore()); + + await act(async () => { + await result.current.init(); + }); + + expect(result.current.config).toEqual(partialConfigData); + expect(result.current.isInitialized).toBe(true); + });
145-156: Improve test reliability by avoiding mock clearing mid-test.Clearing all mocks in the middle of a test (Line 146) could lead to unexpected behavior if other mocks are being used. Consider using more specific mock clearing or resetting.
- // Clear mock to verify second call doesn't happen - jest.clearAllMocks(); + // Reset specific mock to verify second call doesn't happen + mockGetConfig.mockClear();src/stores/app/core-store.ts (1)
86-92: Consider more specific error handling for config fetch failures.The current implementation throws a generic error when config fetch fails. Consider preserving the original error for better debugging.
- // If config fetch failed, don't continue initialization - if (get().error) { - throw new Error('Config fetch failed, cannot continue initialization'); - } + // If config fetch failed, don't continue initialization + const currentError = get().error; + if (currentError) { + throw new Error(`Config fetch failed: ${currentError}`); + }src/services/__tests__/signalr.service.test.ts (1)
474-499: Consider usingwaitForinstead of manually advancing timers.While the test correctly validates reconnection behavior, using
waitForfrom testing-library would make the test more maintainable and less brittle to timing changes.- // Use fake timers for setTimeout - jest.useFakeTimers(); - - // Trigger connection close - onCloseCallback(); - - // Fast-forward time to trigger reconnection - jest.advanceTimersByTime(5000); - - // Should have called connectToHubWithEventingUrl for reconnection - expect(connectSpy).toHaveBeenCalledWith(mockConfig); - - jest.useRealTimers(); + // Trigger connection close + onCloseCallback(); + + // Wait for reconnection attempt + await waitFor(() => { + expect(connectSpy).toHaveBeenCalledWith(mockConfig); + }, { timeout: 6000 });src/app/login/__tests__/login-form.test.tsx (3)
6-61: Consider extracting the mock LoginForm to a shared test utility.The mock LoginForm implementation is quite substantial and might be useful for other test files. Consider moving it to a shared test utilities file for better maintainability and reusability.
Would you like me to help create a shared test utilities file for form component mocks that could be reused across multiple test files?
10-10: Fix the empty destructuring pattern in function parameters.The mock function has an empty destructuring pattern
{ }in the default value foronSubmit.- const MockLoginForm = ({ onSubmit = () => { }, isLoading = false, error = undefined, onServerUrlPress }: any) => { + const MockLoginForm = ({ onSubmit = () => {}, isLoading = false, error = undefined, onServerUrlPress }: any) => {
287-292: Simplify the conditional check for parent element.The conditional check for the parent element can be simplified since
fireEvent.presswill handle undefined gracefully.- const serverUrlButton = screen.getByText('Server URL').parent; - if (serverUrlButton) { - fireEvent.press(serverUrlButton); - expect(onServerUrlPress).toHaveBeenCalledTimes(1); - } + const serverUrlButton = screen.getByText('Server URL'); + fireEvent.press(serverUrlButton.parent!); + expect(onServerUrlPress).toHaveBeenCalledTimes(1);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (3)
.DS_Storeis excluded by!**/.DS_Storeassets/ios-icon.pngis excluded by!**/*.pngassets/splash-icon.pngis excluded by!**/*.png
📒 Files selected for processing (19)
.github/workflows/react-native-cicd.yml(1 hunks)app.config.ts(1 hunks)env.js(0 hunks)jest-setup.ts(1 hunks)src/app/call/new/__tests__/address-search.test.ts(2 hunks)src/app/call/new/__tests__/coordinates-search.test.tsx(2 hunks)src/app/call/new/__tests__/plus-code-search.test.ts(2 hunks)src/app/call/new/__tests__/what3words.test.tsx(1 hunks)src/app/login/__tests__/index.test.tsx(1 hunks)src/app/login/__tests__/login-form.test.tsx(1 hunks)src/app/login/index.tsx(4 hunks)src/app/login/login-form.tsx(2 hunks)src/models/v4/configs/getConfigResultData.ts(1 hunks)src/services/__tests__/signalr.service.test.ts(1 hunks)src/services/signalr.service.ts(5 hunks)src/stores/app/__tests__/core-store.test.ts(4 hunks)src/stores/app/core-store.ts(2 hunks)src/stores/signalr/__tests__/signalr-store.test.ts(1 hunks)src/stores/signalr/signalr-store.ts(2 hunks)
💤 Files with no reviewable changes (1)
- env.js
🧰 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:
app.config.tssrc/app/call/new/__tests__/address-search.test.tssrc/stores/app/core-store.tsjest-setup.tssrc/models/v4/configs/getConfigResultData.tssrc/app/call/new/__tests__/what3words.test.tsxsrc/services/__tests__/signalr.service.test.tssrc/app/call/new/__tests__/plus-code-search.test.tssrc/stores/signalr/signalr-store.tssrc/stores/signalr/__tests__/signalr-store.test.tssrc/app/login/__tests__/index.test.tsxsrc/stores/app/__tests__/core-store.test.tssrc/app/login/login-form.tsxsrc/app/login/index.tsxsrc/services/signalr.service.tssrc/app/login/__tests__/login-form.test.tsxsrc/app/call/new/__tests__/coordinates-search.test.tsx
**/*
📄 CodeRabbit Inference Engine (.cursorrules)
Directory and file names should be lowercase and hyphenated (e.g., user-profile, chat-screen)
Files:
app.config.tssrc/app/call/new/__tests__/address-search.test.tssrc/stores/app/core-store.tsjest-setup.tssrc/models/v4/configs/getConfigResultData.tssrc/app/call/new/__tests__/what3words.test.tsxsrc/services/__tests__/signalr.service.test.tssrc/app/call/new/__tests__/plus-code-search.test.tssrc/stores/signalr/signalr-store.tssrc/stores/signalr/__tests__/signalr-store.test.tssrc/app/login/__tests__/index.test.tsxsrc/stores/app/__tests__/core-store.test.tssrc/app/login/login-form.tsxsrc/app/login/index.tsxsrc/services/signalr.service.tssrc/app/login/__tests__/login-form.test.tsxsrc/app/call/new/__tests__/coordinates-search.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/app/call/new/__tests__/address-search.test.tssrc/app/call/new/__tests__/what3words.test.tsxsrc/services/__tests__/signalr.service.test.tssrc/app/call/new/__tests__/plus-code-search.test.tssrc/stores/signalr/__tests__/signalr-store.test.tssrc/app/login/__tests__/index.test.tsxsrc/stores/app/__tests__/core-store.test.tssrc/app/login/__tests__/login-form.test.tsxsrc/app/call/new/__tests__/coordinates-search.test.tsx
**/__tests__/**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (.cursorrules)
Generate tests for all components, services, and logic; ensure tests run without errors
Files:
src/app/call/new/__tests__/address-search.test.tssrc/app/call/new/__tests__/what3words.test.tsxsrc/services/__tests__/signalr.service.test.tssrc/app/call/new/__tests__/plus-code-search.test.tssrc/stores/signalr/__tests__/signalr-store.test.tssrc/app/login/__tests__/index.test.tsxsrc/stores/app/__tests__/core-store.test.tssrc/app/login/__tests__/login-form.test.tsxsrc/app/call/new/__tests__/coordinates-search.test.tsx
**/*.{test,spec}.{ts,tsx}
📄 CodeRabbit Inference Engine (.github/copilot-instructions.md)
Create and use Jest tests for all generated components
Files:
src/app/call/new/__tests__/address-search.test.tssrc/app/call/new/__tests__/what3words.test.tsxsrc/services/__tests__/signalr.service.test.tssrc/app/call/new/__tests__/plus-code-search.test.tssrc/stores/signalr/__tests__/signalr-store.test.tssrc/app/login/__tests__/index.test.tsxsrc/stores/app/__tests__/core-store.test.tssrc/app/login/__tests__/login-form.test.tsxsrc/app/call/new/__tests__/coordinates-search.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/app/call/new/__tests__/what3words.test.tsxsrc/app/login/__tests__/index.test.tsxsrc/app/login/login-form.tsxsrc/app/login/index.tsxsrc/app/login/__tests__/login-form.test.tsxsrc/app/call/new/__tests__/coordinates-search.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/app/call/new/__tests__/what3words.test.tsxsrc/app/login/__tests__/index.test.tsxsrc/app/login/login-form.tsxsrc/app/login/index.tsxsrc/app/login/__tests__/login-form.test.tsxsrc/app/call/new/__tests__/coordinates-search.test.tsx
🧠 Learnings (3)
📚 Learning: 2025-08-12T03:33:40.227Z
Learnt from: CR
PR: Resgrid/Unit#0
File: .cursorrules:0-0
Timestamp: 2025-08-12T03:33:40.227Z
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/app/login/__tests__/index.test.tsxsrc/app/login/__tests__/login-form.test.tsx
📚 Learning: 2025-08-12T03:33:40.227Z
Learnt from: CR
PR: Resgrid/Unit#0
File: .cursorrules:0-0
Timestamp: 2025-08-12T03:33:40.227Z
Learning: Applies to **/__tests__/**/*.{ts,tsx} : Generate tests for all components, services, and logic; ensure tests run without errors
Applied to files:
src/app/login/__tests__/index.test.tsx
📚 Learning: 2025-08-12T03:34:25.963Z
Learnt from: CR
PR: Resgrid/Unit#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-12T03:34:25.963Z
Learning: Applies to **/*.{test,spec}.{ts,tsx} : Create and use Jest tests for all generated components
Applied to files:
src/app/login/__tests__/index.test.tsx
🧬 Code Graph Analysis (8)
src/stores/app/core-store.ts (1)
src/lib/logging/index.tsx (2)
error(74-76)logger(80-80)
src/services/__tests__/signalr.service.test.ts (2)
src/lib/logging/index.tsx (2)
logger(80-80)error(74-76)src/services/signalr.service.ts (3)
signalRService(317-317)SignalRHubConnectConfig(13-18)SignalRHubConfig(7-11)
src/app/login/__tests__/index.test.tsx (3)
src/components/ui/index.tsx (2)
View(18-18)TouchableOpacity(18-18)src/lib/logging/index.tsx (1)
error(74-76)src/app/login/index.tsx (1)
Login(17-98)
src/stores/app/__tests__/core-store.test.ts (3)
src/lib/storage/app.tsx (2)
getActiveUnitId(24-30)getActiveCallId(35-41)src/api/config/config.ts (1)
getConfig(10-15)src/models/v4/configs/getConfigResultData.ts (1)
GetConfigResultData(1-19)
src/app/login/login-form.tsx (1)
src/components/ui/button/index.tsx (2)
Button(334-334)ButtonText(334-334)
src/app/login/index.tsx (3)
src/app/login/login-form.tsx (1)
LoginForm(45-184)src/lib/logging/index.tsx (1)
error(74-76)src/components/settings/server-url-bottom-sheet.tsx (1)
ServerUrlBottomSheet(29-120)
src/services/signalr.service.ts (2)
src/lib/logging/index.tsx (2)
logger(80-80)error(74-76)env.js (1)
Env(166-166)
src/app/login/__tests__/login-form.test.tsx (2)
src/components/ui/index.tsx (3)
View(18-18)TouchableOpacity(18-18)ActivityIndicator(18-18)src/app/login/login-form.tsx (1)
LoginForm(45-184)
🪛 actionlint (1.7.7)
.github/workflows/react-native-cicd.yml
280-280: "github.event.pull_request.body" is potentially untrusted. avoid using it directly in inline scripts. instead, pass it through an environment variable. see https://docs.github.com/en/actions/security-for-github-actions/security-guides/security-hardening-for-github-actions for more details
(expression)
⏰ 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 (21)
app.config.ts (1)
41-41: iOS-specific icon addition looks good.The addition of a separate iOS icon configuration is appropriate for platform-specific requirements.
src/app/login/login-form.tsx (2)
19-34: Good practice extracting schema creation into a factory function.Making the schema creation testable through a factory function is a good testing practice.
176-180: Server URL button implementation looks good.The conditional rendering and use of the translation key for the button text follows the codebase conventions properly.
src/services/signalr.service.ts (3)
13-18: Good addition of the eventing-based configuration interface.The new
SignalRHubConnectConfiginterface properly encapsulates the eventing URL configuration requirements.
62-62: Good URL normalization practice.Ensuring the EventingUrl has a trailing slash prevents URL concatenation issues.
79-79: Good practice storing configuration for reconnection.Storing the hub configuration enables robust reconnection with the same settings.
src/app/call/new/__tests__/address-search.test.ts (1)
102-121: Config fixture extended correctly; defaults align with the updated model.The added fields (EventingUrl, DirectionsMapKey, PersonnelLocationStaleSeconds, UnitLocationStaleSeconds, PersonnelLocationMinMeters, UnitLocationMinMeters, AnalyticsApiKey, AnalyticsHost) on mockConfig match the expanded GetConfigResultData shape and expected default values. Good consistency.
src/app/login/index.tsx (2)
19-19: State for bottom sheet visibility is well-scoped.Local state
showServerUrlis appropriate for the modal’s open/close lifecycle.
95-95: Bottom sheet integration is correct and closes cleanly.
onClose={() => setShowServerUrl(false)}properly resets local state. Nice and contained.src/app/call/new/__tests__/plus-code-search.test.ts (1)
76-95: Config fixture extended with new fields — matches model defaults.The added EventingUrl/DirectionsMapKey/…/AnalyticsHost fields and their default values look correct.
src/stores/signalr/signalr-store.ts (3)
41-53: Solid guard for EventingUrl presence; clearer failure mode.Fetching EventingUrl from core store config and short-circuiting with a logged error is a good move and prevents malformed connections.
150-162: Good: geolocation hub also guarded by EventingUrl presence.Consistent error handling with the update hub path. Looks solid.
164-169: EventingUrl-based geolocation connection: verify alias vs hub identifiers.As with the update hub, confirm
nameandhubNameare intended to be equal for geolocation. If not, pass appropriate distinct values.src/stores/app/core-store.ts (1)
244-255: Good error handling pattern in fetchConfig.The implementation correctly:
- Resets the error state on success (Line 247)
- Sets a descriptive error message (Line 249)
- Logs the error with context (Lines 250-253)
- Re-throws the error for upstream handling (Line 254)
This allows callers to handle the error appropriately while maintaining proper error state in the store.
src/app/login/__tests__/index.test.tsx (2)
72-93: Clean mock implementation for LoginForm.The mock properly exposes test IDs for all interactive elements and correctly handles the optional
onServerUrlPressprop. This enables deterministic testing of the login flow and server URL functionality.
127-241: Comprehensive test coverage for the Login component.The test suite thoroughly covers:
- Rendering of UI components
- Server URL bottom sheet open/close flow
- Error modal display
- Authentication redirect
- Analytics tracking
- Form submission
All tests properly use
waitForfor async operations and appropriately mock dependencies.src/app/call/new/__tests__/coordinates-search.test.tsx (1)
116-131: Config fixtures correctly updated with new fields.Both
mockConfigandconfigWithoutKeyhave been properly updated to include all the new fields fromGetConfigResultData. The default values match those in the model definition.Also applies to: 260-275
src/stores/signalr/__tests__/signalr-store.test.ts (2)
152-196: Good test coverage for EventingUrl validation.The tests properly verify that:
- Missing EventingUrl prevents connection attempts
- Appropriate error messages are set and logged
- The SignalR service is not called when preconditions aren't met
197-212: Proper error handling test for connection failures.The test correctly validates that connection errors are properly caught, stored in the error state, and logged with appropriate context.
src/services/__tests__/signalr.service.test.ts (2)
72-279: LGTM! Comprehensive test coverage for the new EventingUrl-based connection flow.The tests thoroughly validate the new
connectToHubWithEventingUrlfunctionality, including:
- Successful connection setup with proper URL construction
- Method registration and event handler setup
- Error handling for missing tokens and EventingUrl
- Special handling for geolocation hub authentication
- URL encoding for tokens with special characters
- Edge cases with trailing slashes and existing query parameters
172-190: LGTM! Excellent coverage of URL encoding edge cases.The test properly validates that tokens with special characters are correctly encoded for the geolocation hub URL parameter, which is critical for preventing authentication failures.
| it('should handle URL with existing query parameters for geolocation hub', async () => { | ||
| const geoConfig: SignalRHubConnectConfig = { | ||
| name: 'geoHub', | ||
| eventingUrl: 'https://api.example.com/path?existing=param', | ||
| hubName: 'geolocationHub', // This should match REALTIME_GEO_HUB_NAME from env | ||
| methods: ['onPersonnelLocationUpdated'], | ||
| }; | ||
|
|
||
| await signalRService.connectToHubWithEventingUrl(geoConfig); | ||
|
|
||
| // Should append the token parameter correctly | ||
| expect(mockBuilderInstance.withUrl).toHaveBeenCalledWith( | ||
| 'https://api.example.com/path?existing=param/geolocationHub&access_token=mock-token', | ||
| {} | ||
| ); | ||
| }); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify the expected URL construction for existing query parameters.
The test expects the URL to be constructed as https://api.example.com/path?existing=param/geolocationHub&access_token=mock-token (line 224), which appears incorrect. When EventingUrl already contains query parameters, the hub name should be appended to the path before the query string, not after.
The expected URL should likely be:
- 'https://api.example.com/path?existing=param/geolocationHub&access_token=mock-token',
+ 'https://api.example.com/path/geolocationHub?existing=param&access_token=mock-token',🏁 Script executed:
#!/bin/bash
# Check how the SignalRService constructs URLs with existing query parameters
ast-grep --pattern 'connectToHubWithEventingUrl' src/services/signalr.service.ts -A 50 | head -100Length of output: 98
🏁 Script executed:
#!/bin/bash
# Locate the connectToHubWithEventingUrl method in the service files
rg -nP "connectToHubWithEventingUrl" -C 5 src/services
# Inspect how the URL is constructed within that method
rg -nP "withUrl" -C 5 src/services/signalr.service.tsLength of output: 26959
🏁 Script executed:
#!/bin/bash
# Locate where fullUrl is defined and constructed
rg -nP "fullUrl" -C10 src/services/signalr.service.tsLength of output: 1642
Fix URL construction for existing query parameters.
The current implementation appends the hub name into the query string (treating the trailing slash as part of the query) rather than inserting it into the path before the query. We should split out the query parameters, append the hub name to the path, then reassemble the query string.
Locations to update:
src/services/signalr.service.ts(lines 61–71): AdjustnormalizedEventingUrl/fullUrllogic.src/services/__tests__/signalr.service.test.ts(lines 212–227): Update the expected URL.
Suggested diff:
--- a/src/services/signalr.service.ts
+++ b/src/services/signalr.service.ts
@@ 61,8c61,13
- // Ensure EventingUrl has a trailing slash
- const normalizedEventingUrl = config.eventingUrl.endsWith('/') ? config.eventingUrl : `${config.eventingUrl}/`;
-
- let fullUrl = `${normalizedEventingUrl}${config.hubName}`;
+ // Split base path and existing query parameters
+ const [baseUrl, existingQuery] = config.eventingUrl.split('?');
+ const normalizedPath = baseUrl.endsWith('/') ? baseUrl : `${baseUrl}/`;
+ // Build URL: always insert hubName into path, then reattach query
+ let fullUrl = existingQuery
+ ? `${normalizedPath}${config.hubName}?${existingQuery}`
+ : `${normalizedPath}${config.hubName}`;
// For geolocation hub, add token as URL parameter instead of header
const isGeolocationHub = config.hubName === Env.REALTIME_GEO_HUB_NAME;
@@ 67,7c72,7
- if (isGeolocationHub) {
+ if (isGeolocationHub) {
const separator = fullUrl.includes('?') ? '&' : '?';
fullUrl = `${fullUrl}${separator}access_token=${encodeURIComponent(token)}`;
}--- a/src/services/__tests__/signalr.service.test.ts
+++ b/src/services/__tests__/signalr.service.test.ts
@@ it('should handle URL with existing query parameters for geolocation hub', async () => {
- expect(mockBuilderInstance.withUrl).toHaveBeenCalledWith(
- 'https://api.example.com/path?existing=param/geolocationHub&access_token=mock-token',
- {}
- );
+ expect(mockBuilderInstance.withUrl).toHaveBeenCalledWith(
+ 'https://api.example.com/path/geolocationHub?existing=param&access_token=mock-token',
+ {}
+ );📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| it('should handle URL with existing query parameters for geolocation hub', async () => { | |
| const geoConfig: SignalRHubConnectConfig = { | |
| name: 'geoHub', | |
| eventingUrl: 'https://api.example.com/path?existing=param', | |
| hubName: 'geolocationHub', // This should match REALTIME_GEO_HUB_NAME from env | |
| methods: ['onPersonnelLocationUpdated'], | |
| }; | |
| await signalRService.connectToHubWithEventingUrl(geoConfig); | |
| // Should append the token parameter correctly | |
| expect(mockBuilderInstance.withUrl).toHaveBeenCalledWith( | |
| 'https://api.example.com/path?existing=param/geolocationHub&access_token=mock-token', | |
| {} | |
| ); | |
| }); | |
| it('should handle URL with existing query parameters for geolocation hub', async () => { | |
| const geoConfig: SignalRHubConnectConfig = { | |
| name: 'geoHub', | |
| eventingUrl: 'https://api.example.com/path?existing=param', | |
| hubName: 'geolocationHub', // This should match REALTIME_GEO_HUB_NAME from env | |
| methods: ['onPersonnelLocationUpdated'], | |
| }; | |
| await signalRService.connectToHubWithEventingUrl(geoConfig); | |
| // Should append the token parameter correctly | |
| expect(mockBuilderInstance.withUrl).toHaveBeenCalledWith( | |
| 'https://api.example.com/path/geolocationHub?existing=param&access_token=mock-token', | |
| {} | |
| ); | |
| }); |
🤖 Prompt for AI Agents
In src/services/signalr.service.ts (lines ~61–71) and the test
src/services/__tests__/signalr.service.test.ts (lines 212–227), the code
currently inserts the hub name into the query string when the eventingUrl
already contains query parameters; instead, parse the incoming eventingUrl into
path and query components, append the hub name to the path (ensuring a single
slash), then reassemble the query string and finally add access_token as an
additional query parameter (merging with existing ones); update the test
expectation to assert the rebuilt URL where the hub segment is in the path and
access_token is appended to the query.
| const hubConfig = this.hubConfigs.get(hubName); | ||
| if (hubConfig) { | ||
| setTimeout(() => { | ||
| this.connectToHubWithEventingUrl(hubConfig); | ||
| }, this.RECONNECT_INTERVAL); | ||
| } else { | ||
| logger.error({ | ||
| message: `No stored config found for hub: ${hubName}`, | ||
| }); | ||
| }, this.RECONNECT_INTERVAL); | ||
| } |
There was a problem hiding this comment.
Potential issue with reconnection after authentication token expiry.
The reconnection logic uses the stored configuration but doesn't refresh the authentication token. If the connection closes due to token expiry, the reconnection will fail with the same expired token.
The token should be refreshed before reconnection attempts:
private handleConnectionClose(hubName: string): void {
const attempts = this.reconnectAttempts.get(hubName) || 0;
if (attempts < this.MAX_RECONNECT_ATTEMPTS) {
this.reconnectAttempts.set(hubName, attempts + 1);
const hubConfig = this.hubConfigs.get(hubName);
if (hubConfig) {
setTimeout(() => {
+ // Ensure we have a fresh token before reconnecting
+ const token = useAuthStore.getState().accessToken;
+ if (!token) {
+ logger.error({
+ message: `No authentication token available for reconnection to hub: ${hubName}`,
+ });
+ return;
+ }
this.connectToHubWithEventingUrl(hubConfig);
}, this.RECONNECT_INTERVAL);
} else {
logger.error({
message: `No stored config found for hub: ${hubName}`,
});
}
} else {Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/services/signalr.service.ts around lines 231 to 240, the reconnection
branch uses the stored hubConfig as-is which can contain an expired auth token;
before calling connectToHubWithEventingUrl on reconnect, refresh or re-acquire a
current authentication token (or regenerate the auth headers) and update
hubConfig with the new token, then proceed with connectToHubWithEventingUrl; if
token refresh fails, log the failure and avoid attempting the reconnect until a
valid token is obtained.
| // Mock all dependencies before importing anything | ||
| jest.mock('@/services/signalr.service', () => { | ||
| const mockInstance = { | ||
| connectToHubWithEventingUrl: jest.fn().mockResolvedValue(undefined), | ||
| disconnectFromHub: jest.fn().mockResolvedValue(undefined), | ||
| invoke: jest.fn().mockResolvedValue(undefined), | ||
| on: jest.fn(), | ||
| connectToHub: jest.fn().mockResolvedValue(undefined), | ||
| disconnectAll: jest.fn().mockResolvedValue(undefined), | ||
| }; | ||
| return { | ||
| signalRService: mockInstance, | ||
| default: mockInstance, | ||
| }; | ||
| }); | ||
|
|
||
| jest.mock('@/stores/app/core-store', () => ({ | ||
| useCoreStore: { | ||
| getState: jest.fn(() => ({ | ||
| config: { | ||
| EventingUrl: 'https://eventing.example.com/', | ||
| }, | ||
| })), | ||
| subscribe: jest.fn(), | ||
| setState: jest.fn(), | ||
| destroy: jest.fn(), | ||
| }, | ||
| })); | ||
|
|
||
| jest.mock('../../app/core-store', () => { | ||
| return { | ||
| useCoreStore: { | ||
| getState: jest.fn(() => ({ | ||
| config: { | ||
| EventingUrl: 'https://eventing.example.com/', | ||
| }, | ||
| })), | ||
| subscribe: jest.fn(), | ||
| setState: jest.fn(), | ||
| destroy: jest.fn(), | ||
| }, | ||
| }; | ||
| }); | ||
|
|
||
| jest.mock('@/stores/security/store', () => ({ | ||
| securityStore: mockSecurityStore, | ||
| })); | ||
|
|
||
| jest.mock('../../security/store', () => ({ | ||
| securityStore: mockSecurityStore, | ||
| useSecurityStore: mockSecurityStore, | ||
| })); | ||
|
|
||
| jest.mock('@/lib/logging', () => ({ | ||
| logger: { | ||
| info: jest.fn(), | ||
| error: jest.fn(), | ||
| warn: jest.fn(), | ||
| debug: jest.fn(), | ||
| trace: jest.fn(), | ||
| fatal: jest.fn(), | ||
| }, | ||
| })); | ||
|
|
||
| jest.mock('@/lib/env', () => ({ | ||
| Env: { | ||
| CHANNEL_HUB_NAME: 'eventingHub', | ||
| REALTIME_GEO_HUB_NAME: 'geolocationHub', | ||
| }, | ||
| })); | ||
|
|
||
| jest.mock('@/lib', () => ({ | ||
| useAuthStore: { | ||
| getState: jest.fn(() => ({ | ||
| accessToken: 'mock-token', | ||
| })), | ||
| }, | ||
| })); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider consolidating duplicate mock definitions.
There are duplicate mock definitions for useCoreStore (lines 35-46 and 48-61) and securityStore (lines 63-65 and 67-70). This could lead to confusion and maintenance issues.
Remove the duplicate mock definitions:
-jest.mock('../../app/core-store', () => {
- return {
- useCoreStore: {
- getState: jest.fn(() => ({
- config: {
- EventingUrl: 'https://eventing.example.com/',
- },
- })),
- subscribe: jest.fn(),
- setState: jest.fn(),
- destroy: jest.fn(),
- },
- };
-});
-jest.mock('../../security/store', () => ({
- securityStore: mockSecurityStore,
- useSecurityStore: mockSecurityStore,
-}));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Mock all dependencies before importing anything | |
| jest.mock('@/services/signalr.service', () => { | |
| const mockInstance = { | |
| connectToHubWithEventingUrl: jest.fn().mockResolvedValue(undefined), | |
| disconnectFromHub: jest.fn().mockResolvedValue(undefined), | |
| invoke: jest.fn().mockResolvedValue(undefined), | |
| on: jest.fn(), | |
| connectToHub: jest.fn().mockResolvedValue(undefined), | |
| disconnectAll: jest.fn().mockResolvedValue(undefined), | |
| }; | |
| return { | |
| signalRService: mockInstance, | |
| default: mockInstance, | |
| }; | |
| }); | |
| jest.mock('@/stores/app/core-store', () => ({ | |
| useCoreStore: { | |
| getState: jest.fn(() => ({ | |
| config: { | |
| EventingUrl: 'https://eventing.example.com/', | |
| }, | |
| })), | |
| subscribe: jest.fn(), | |
| setState: jest.fn(), | |
| destroy: jest.fn(), | |
| }, | |
| })); | |
| jest.mock('../../app/core-store', () => { | |
| return { | |
| useCoreStore: { | |
| getState: jest.fn(() => ({ | |
| config: { | |
| EventingUrl: 'https://eventing.example.com/', | |
| }, | |
| })), | |
| subscribe: jest.fn(), | |
| setState: jest.fn(), | |
| destroy: jest.fn(), | |
| }, | |
| }; | |
| }); | |
| jest.mock('@/stores/security/store', () => ({ | |
| securityStore: mockSecurityStore, | |
| })); | |
| jest.mock('../../security/store', () => ({ | |
| securityStore: mockSecurityStore, | |
| useSecurityStore: mockSecurityStore, | |
| })); | |
| jest.mock('@/lib/logging', () => ({ | |
| logger: { | |
| info: jest.fn(), | |
| error: jest.fn(), | |
| warn: jest.fn(), | |
| debug: jest.fn(), | |
| trace: jest.fn(), | |
| fatal: jest.fn(), | |
| }, | |
| })); | |
| jest.mock('@/lib/env', () => ({ | |
| Env: { | |
| CHANNEL_HUB_NAME: 'eventingHub', | |
| REALTIME_GEO_HUB_NAME: 'geolocationHub', | |
| }, | |
| })); | |
| jest.mock('@/lib', () => ({ | |
| useAuthStore: { | |
| getState: jest.fn(() => ({ | |
| accessToken: 'mock-token', | |
| })), | |
| }, | |
| })); | |
| // Mock all dependencies before importing anything | |
| jest.mock('@/services/signalr.service', () => { | |
| const mockInstance = { | |
| connectToHubWithEventingUrl: jest.fn().mockResolvedValue(undefined), | |
| disconnectFromHub: jest.fn().mockResolvedValue(undefined), | |
| invoke: jest.fn().mockResolvedValue(undefined), | |
| on: jest.fn(), | |
| connectToHub: jest.fn().mockResolvedValue(undefined), | |
| disconnectAll: jest.fn().mockResolvedValue(undefined), | |
| }; | |
| return { | |
| signalRService: mockInstance, | |
| default: mockInstance, | |
| }; | |
| }); | |
| jest.mock('@/stores/app/core-store', () => ({ | |
| useCoreStore: { | |
| getState: jest.fn(() => ({ | |
| config: { | |
| EventingUrl: 'https://eventing.example.com/', | |
| }, | |
| })), | |
| subscribe: jest.fn(), | |
| setState: jest.fn(), | |
| destroy: jest.fn(), | |
| }, | |
| })); | |
| jest.mock('@/stores/security/store', () => ({ | |
| securityStore: mockSecurityStore, | |
| })); | |
| jest.mock('@/lib/logging', () => ({ | |
| logger: { | |
| info: jest.fn(), | |
| error: jest.fn(), | |
| warn: jest.fn(), | |
| debug: jest.fn(), | |
| trace: jest.fn(), | |
| fatal: jest.fn(), | |
| }, | |
| })); | |
| jest.mock('@/lib/env', () => ({ | |
| Env: { | |
| CHANNEL_HUB_NAME: 'eventingHub', | |
| REALTIME_GEO_HUB_NAME: 'geolocationHub', | |
| }, | |
| })); | |
| jest.mock('@/lib', () => ({ | |
| useAuthStore: { | |
| getState: jest.fn(() => ({ | |
| accessToken: 'mock-token', | |
| })), | |
| }, | |
| })); |
🤖 Prompt for AI Agents
In src/stores/signalr/__tests__/signalr-store.test.ts around lines 19 to 96,
there are duplicate mock definitions for useCoreStore and securityStore;
consolidate them by keeping a single mock implementation for each module and
remove the duplicate blocks, export that single mock for both import paths used
in the file (e.g., the alias and relative path) or move the mock into a shared
variable and reuse it in both jest.mock calls, ensuring the mock includes
getState, subscribe, setState, destroy (for useCoreStore) and the same
mockSecurityStore reference for securityStore so tests still run and all imports
receive the same mocked instance.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/stores/signalr/__tests__/signalr-store.test.ts (1)
34-50: Remove duplicate mock definitions for better maintainability.There are two
jest.mockcalls for the security store module (lines 52-54 and 56-59). This duplication could lead to confusion and maintenance issues. The second mock for../../security/storeappears to be unnecessary since both imports likely resolve to the same module.Remove the duplicate mock:
jest.mock('@/stores/security/store', () => ({ securityStore: mockSecurityStore, })); - -jest.mock('../../security/store', () => ({ - securityStore: mockSecurityStore, - useSecurityStore: mockSecurityStore, -}));If both import paths are actually needed, consolidate them into a single mock registration that handles both paths.
Also applies to: 52-59
src/services/__tests__/signalr.service.test.ts (1)
233-248: Fix URL construction for existing query parameters.The test expects the URL to be constructed incorrectly when EventingUrl already contains query parameters. The hub name should be appended to the path before the query string, not within it.
The expected URL on line 245 should be:
- 'https://api.example.com/path/geolocationHub?existing=param&access_token=mock-token', + 'https://api.example.com/path/geolocationHub?existing=param&access_token=mock-token',Wait, the expected URL actually looks correct. Let me verify the implementation...
#!/bin/bash # Check how the SignalRService constructs URLs with existing query parameters rg -nP "const url = new URL" -A20 src/services/signalr.service.ts
🧹 Nitpick comments (1)
.github/workflows/react-native-cicd.yml (1)
296-304: LGTM: Release notes file generation is sound and idempotentScoped to the Android leg and falls back cleanly when RELEASE_NOTES is empty. No issues.
If you’d like the header to match the GitHub Release title, consider echoing “Resgrid Unit 7.${{ github.run_number }}” (or similar) instead of “Version …”, purely for consistency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
.github/workflows/react-native-cicd.yml(1 hunks)src/app/login/__tests__/login-form.test.tsx(1 hunks)src/services/__tests__/signalr.service.test.ts(1 hunks)src/services/signalr.service.ts(5 hunks)src/stores/signalr/__tests__/signalr-store.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/app/login/tests/login-form.test.tsx
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{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/services/__tests__/signalr.service.test.tssrc/stores/signalr/__tests__/signalr-store.test.tssrc/services/signalr.service.ts
**/*
📄 CodeRabbit Inference Engine (.cursorrules)
Directory and file names should be lowercase and hyphenated (e.g., user-profile, chat-screen)
Files:
src/services/__tests__/signalr.service.test.tssrc/stores/signalr/__tests__/signalr-store.test.tssrc/services/signalr.service.ts
**/*.{test.ts,test.tsx,spec.ts,spec.tsx}
📄 CodeRabbit Inference Engine (.cursorrules)
Create and use Jest tests to validate all generated components
Files:
src/services/__tests__/signalr.service.test.tssrc/stores/signalr/__tests__/signalr-store.test.ts
**/__tests__/**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (.cursorrules)
Generate tests for all components, services, and logic; ensure tests run without errors
Files:
src/services/__tests__/signalr.service.test.tssrc/stores/signalr/__tests__/signalr-store.test.ts
**/*.{test,spec}.{ts,tsx}
📄 CodeRabbit Inference Engine (.github/copilot-instructions.md)
Create and use Jest tests for all generated components
Files:
src/services/__tests__/signalr.service.test.tssrc/stores/signalr/__tests__/signalr-store.test.ts
🧬 Code Graph Analysis (3)
src/services/__tests__/signalr.service.test.ts (2)
src/lib/logging/index.tsx (2)
logger(80-80)error(74-76)src/services/signalr.service.ts (3)
signalRService(359-359)SignalRHubConnectConfig(13-18)SignalRHubConfig(7-11)
src/stores/signalr/__tests__/signalr-store.test.ts (3)
src/services/signalr.service.ts (1)
signalRService(359-359)src/stores/signalr/signalr-store.ts (1)
useSignalRStore(25-208)src/lib/logging/index.tsx (1)
logger(80-80)
src/services/signalr.service.ts (2)
src/lib/logging/index.tsx (2)
logger(80-80)error(74-76)env.js (1)
Env(166-166)
⏰ 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 (9)
src/stores/signalr/__tests__/signalr-store.test.ts (3)
140-163: Test coverage looks comprehensive for missing EventingUrl scenario.The test properly validates that:
- No connection attempt is made when EventingUrl is missing
- Error state is set with appropriate message
- Error is logged with the correct message
183-198: Good error handling test coverage.The test effectively validates error propagation and logging when connection fails. The error context is properly included in the log message.
569-607: Excellent test coverage for token refresh failure scenario.The test thoroughly validates that:
- Token refresh is attempted during reconnection
- Connection is not attempted when token refresh fails
- Error is properly logged with context
- The spy correctly verifies that
connectToHubWithEventingUrlis not calledThis aligns well with the implementation in
signalr.service.tsthat handles token refresh before reconnection.src/services/__tests__/signalr.service.test.ts (3)
210-231: Comprehensive test coverage for URL encoding special characters.The test properly validates that complex access tokens with special characters are correctly URL-encoded when used as query parameters for the geolocation hub. This is crucial for security token handling.
495-526: Well-structured reconnection test with proper timer handling.The test effectively validates the reconnection flow:
- Uses fake timers to control async timing
- Verifies token refresh before reconnection
- Confirms reconnection attempt with stored config
- Properly restores timers and spies
569-607: Robust test for token refresh failure during reconnection.The test thoroughly validates error handling when token refresh fails:
- Mocks token refresh to fail
- Verifies no reconnection attempt is made
- Confirms error is logged with appropriate context
- Uses spy to ensure
connectToHubWithEventingUrlis not calledThis aligns well with the service implementation's token refresh logic.
src/services/signalr.service.ts (3)
61-84: URL construction correctly handles existing query parameters.The implementation properly:
- Parses the EventingUrl using the URL API
- Appends the hub name to the path (not the query)
- Preserves existing query parameters
- Adds the access_token for geolocation hubs
- Manually encodes spaces as %20 instead of +
This matches the test expectations and handles all edge cases correctly.
247-277: Excellent implementation of token refresh during reconnection.The reconnection logic properly addresses the authentication token expiry issue:
- Refreshes the token before attempting reconnection
- Validates token availability after refresh
- Logs the refresh process for debugging
- Handles refresh failures gracefully without attempting reconnection
- Preserves the original config for reconnection
This implementation effectively prevents reconnection attempts with expired tokens.
86-89: Good security practice: Token redaction in logs.The implementation properly redacts the access token in log messages for geolocation hubs using regex replacement. This prevents sensitive token exposure in logs.
| - name: 📋 Extract Release Notes from PR Body | ||
| if: ${{ github.event_name == 'pull_request' && matrix.platform == 'android' }} | ||
| env: | ||
| PR_BODY: ${{ github.event.pull_request.body }} | ||
| run: | | ||
| set -eo pipefail | ||
| # Grab lines after "## Release Notes" until the next header | ||
| RELEASE_NOTES="$(printf '%s\n' "$PR_BODY" \ | ||
| | awk 'f && /^## /{f=0} /^## Release Notes/{f=1; next} f')" | ||
| # Use a unique delimiter to write multiline into GITHUB_ENV | ||
| delimiter="EOF_$(date +%s)_$RANDOM" | ||
| { | ||
| echo "RELEASE_NOTES<<$delimiter" | ||
| printf '%s\n' "${RELEASE_NOTES:-No release notes provided.}" | ||
| echo "$delimiter" | ||
| } >> "$GITHUB_ENV" | ||
There was a problem hiding this comment.
Release notes extraction step will never run due to job-level event gating
The build-and-deploy job only runs on push/workflow_dispatch, but this step is gated to pull_request events. As a result, RELEASE_NOTES is never populated from the PR body, and downstream steps will always use the fallback.
Fix by removing the event gate and restricting to the android matrix leg. Also make the header match case-insensitive and more whitespace-tolerant.
- - name: 📋 Extract Release Notes from PR Body
- if: ${{ github.event_name == 'pull_request' && matrix.platform == 'android' }}
+ - name: 📋 Extract Release Notes from PR Body
+ if: ${{ matrix.platform == 'android' }}
env:
PR_BODY: ${{ github.event.pull_request.body }}
run: |
- set -eo pipefail
- # Grab lines after "## Release Notes" until the next header
- RELEASE_NOTES="$(printf '%s\n' "$PR_BODY" \
- | awk 'f && /^## /{f=0} /^## Release Notes/{f=1; next} f')"
+ set -eo pipefail
+ # Grab lines after "## Release Notes" (case-insensitive) until the next "## " header
+ RELEASE_NOTES="$(printf '%s\n' "$PR_BODY" \
+ | awk 'BEGIN{IGNORECASE=1} f && /^##[[:space:]]/{f=0} /^##[[:space:]]*Release Notes[[:space:]]*$/ {f=1; next} f')"
# Use a unique delimiter to write multiline into GITHUB_ENV
delimiter="EOF_$(date +%s)_$RANDOM"
{
echo "RELEASE_NOTES<<$delimiter"
printf '%s\n' "${RELEASE_NOTES:-No release notes provided.}"
echo "$delimiter"
} >> "$GITHUB_ENV"If you truly need PR-body-driven notes on push (post-merge), consider fetching the merged PR’s body via the GitHub API (gh or curl) in push/workflow_dispatch runs.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - name: 📋 Extract Release Notes from PR Body | |
| if: ${{ github.event_name == 'pull_request' && matrix.platform == 'android' }} | |
| env: | |
| PR_BODY: ${{ github.event.pull_request.body }} | |
| run: | | |
| set -eo pipefail | |
| # Grab lines after "## Release Notes" until the next header | |
| RELEASE_NOTES="$(printf '%s\n' "$PR_BODY" \ | |
| | awk 'f && /^## /{f=0} /^## Release Notes/{f=1; next} f')" | |
| # Use a unique delimiter to write multiline into GITHUB_ENV | |
| delimiter="EOF_$(date +%s)_$RANDOM" | |
| { | |
| echo "RELEASE_NOTES<<$delimiter" | |
| printf '%s\n' "${RELEASE_NOTES:-No release notes provided.}" | |
| echo "$delimiter" | |
| } >> "$GITHUB_ENV" | |
| - name: 📋 Extract Release Notes from PR Body | |
| if: ${{ matrix.platform == 'android' }} | |
| env: | |
| PR_BODY: ${{ github.event.pull_request.body }} | |
| run: | | |
| - set -eo pipefail | |
| - # Grab lines after "## Release Notes" until the next header | |
| - RELEASE_NOTES="$(printf '%s\n' "$PR_BODY" \ | |
| set -eo pipefail | |
| # Grab lines after "## Release Notes" (case-insensitive) until the next "## " header | |
| RELEASE_NOTES="$(printf '%s\n' "$PR_BODY" \ | |
| | awk 'BEGIN{IGNORECASE=1} f && /^##[[:space:]]/{f=0} /^##[[:space:]]*Release Notes[[:space:]]*$/ {f=1; next} f')" | |
| # Use a unique delimiter to write multiline into GITHUB_ENV | |
| delimiter="EOF_$(date +%s)_$RANDOM" | |
| { | |
| echo "RELEASE_NOTES<<$delimiter" | |
| printf '%s\n' "${RELEASE_NOTES:-No release notes provided.}" | |
| echo "$delimiter" | |
| } >> "$GITHUB_ENV" |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
.github/workflows/react-native-cicd.yml (1)
279-295: Make Release Notes extraction resilient on push/workflow_dispatch by fetching the PR bodyCurrently this job never runs on pull_request (job-level if), so PR_BODY is empty on push/workflow_dispatch and the notes always fall back. Fetch the associated PR body via the GitHub API when not a PR event. Also make the header match case-insensitive and whitespace-tolerant to be more robust.
Apply:
- name: 📋 Extract Release Notes from PR Body - if: ${{ matrix.platform == 'android' }} + if: ${{ matrix.platform == 'android' }} env: PR_BODY: ${{ github.event.pull_request.body }} + GITHUB_TOKEN: ${{ github.token }} run: | - set -eo pipefail - # Grab lines after "## Release Notes" until the next header - RELEASE_NOTES="$(printf '%s\n' "$PR_BODY" \ - | awk 'f && /^## /{f=0} /^## Release Notes/{f=1; next} f')" + set -euo pipefail + BODY="${PR_BODY:-}" + # On push/workflow_dispatch, pull_request.body is unavailable; try to resolve the PR for this commit + if [ -z "$BODY" ]; then + PRS_JSON="$(curl -sS -H "Authorization: Bearer $GITHUB_TOKEN" -H "Accept: application/vnd.github+json" \ + "https://api.github.com/repos/${{ github.repository }}/commits/${{ github.sha }}/pulls")" + PR_NUMBER="$(printf '%s' "$PRS_JSON" | jq -r '.[0].number // empty')" + if [ -n "$PR_NUMBER" ]; then + BODY="$(curl -sS -H "Authorization: Bearer $GITHUB_TOKEN" -H "Accept: application/vnd.github+json" \ + "https://api.github.com/repos/${{ github.repository }}/pulls/$PR_NUMBER" | jq -r '.body // empty')" + fi + fi + # Grab lines after "## Release Notes" (case-insensitive) until the next "## " header + RELEASE_NOTES="$(printf '%s\n' "$BODY" \ + | awk 'BEGIN{IGNORECASE=1} f && /^##[[:space:]]/{f=0} /^##[[:space:]]*Release Notes[[:space:]]*$/ {f=1; next} f')" # Use a unique delimiter to write multiline into GITHUB_ENV delimiter="EOF_$(date +%s)_$RANDOM" { echo "RELEASE_NOTES<<$delimiter" printf '%s\n' "${RELEASE_NOTES:-No release notes provided.}" echo "$delimiter" } >> "$GITHUB_ENV"Verification:
- On push/workflow_dispatch: step resolves the PR for the commit and populates RELEASE_NOTES when available.
- On pull_request (if you ever enable this job for PRs): it uses the provided PR_BODY safely (no shell expansion).
🧹 Nitpick comments (1)
.github/workflows/react-native-cicd.yml (1)
305-315: Optional: Avoid overwriting body on reruns; keep release notes stableOn reruns with allowUpdates: true, you may want to keep the existing body intact. Add omitBodyDuringUpdate: true to prevent overwriting notes.
uses: ncipollo/release-action@v1 with: tag: "7.${{ github.run_number }}" commit: ${{ github.sha }} makeLatest: true allowUpdates: true + omitBodyDuringUpdate: true name: "7.${{ github.run_number }}" artifacts: "./ResgridUnit-prod.apk" bodyFile: "RELEASE_NOTES.md"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
.github/workflows/react-native-cicd.yml(1 hunks)
⏰ 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 (1)
.github/workflows/react-native-cicd.yml (1)
296-304: LGTM: Scoped release notes file avoids mutating CHANGELOGGood move to generate a per-run RELEASE_NOTES.md with a fallback. This keeps artifacts clean and avoids repo mutations during CI.
|
Approve |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Summary by CodeRabbit
New Features
Refactor
Chores
Tests