CU-868f7hkrj Minor bug fixes #161
Conversation
WalkthroughAdds pull-to-refresh that bypasses contacts cache, introduces getSystemConfig API and supporting models, adjusts imports, enhances contact notes rendering and Settings bottom-sheet, pins react-native-callkeep to a specific commit, and adds/updates tests and documentation. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant ContactsScreen
participant ContactsStore
participant ContactsAPI
participant CacheManager
participant Server
User->>ContactsScreen: Pull to refresh
ContactsScreen->>ContactsStore: fetchContacts(true)
ContactsStore->>ContactsAPI: getAllContacts(true)
ContactsAPI->>CacheManager: remove('/Contacts/GetAllContacts')
ContactsAPI->>Server: GET /Contacts/GetAllContacts
Server-->>ContactsAPI: 200 data
ContactsAPI-->>ContactsStore: data
ContactsStore-->>ContactsScreen: update state
ContactsScreen-->>User: refreshed list
sequenceDiagram
participant Caller
participant ConfigAPI
participant CachedEndpoint as Cached /Config/GetSystemConfig
participant Server
Caller->>ConfigAPI: getSystemConfig()
ConfigAPI->>CachedEndpoint: GET (ttl=1 day, enabled=false)
CachedEndpoint->>Server: Request
Server-->>CachedEndpoint: GetSystemConfigResult
CachedEndpoint-->>ConfigAPI: response.data
ConfigAPI-->>Caller: GetSystemConfigResult
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
✨ 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: 6
🧹 Nitpick comments (17)
package.json (1)
148-148: Pinned commit is valid; consider tagging a durable release
- Verified that commit 957193d0716f1c2dfdc18e627cbff0f8a0800971 exists and is accessible upstream.
- No existing tags reference this commit.
- For long-term stability, you may fork the repo, tag this commit (e.g.
v1.0.0), and update the dependency to point at your tagged release.Dependency remains:
- "react-native-callkeep": "github:Irfanwani/react-native-callkeep#957193d0716f1c2dfdc18e627cbff0f8a0800971" + "react-native-callkeep": "github:<your-fork>/react-native-callkeep#v1.0.0"src/components/settings/server-url-bottom-sheet.tsx (3)
5-5: Use the UI-layer ScrollView for consistency with gluestack-uiPer our guidelines, prefer the UI wrapper over raw RN when available to keep styling/behavior consistent.
-import { Platform, ScrollView } from 'react-native'; +import { Platform } from 'react-native'; +import { ScrollView } from '../ui';Also applies to: 76-76
94-106: Improve UX: submit on keyboard “Done”Trigger form submission on the keyboard’s “Done” to reduce taps. No behavior change otherwise.
<InputField value={value} onChangeText={onChange} placeholder={t('settings.enter_server_url')} autoCapitalize="none" autoCorrect={false} keyboardType="url" textContentType="URL" returnKeyType="done" autoFocus={false} blurOnSubmit={true} + onSubmitEditing={handleSubmit(onFormSubmit)} />
49-66: Normalize the URL before saving (trim/trailing slash)Users may enter trailing slashes or spaces. Normalize to avoid double slashes like //api/v4.
Example change (outside the shown hunk):
const onFormSubmit = async (data: ServerUrlForm) => { try { setIsLoading(true); const base = data.url.trim().replace(/\/+$/, ''); // remove trailing slashes await setUrl(`${base}/api/${Env.API_VERSION}`); logger.info({ message: 'Server URL updated successfully', context: { url: base } }); onClose(); } catch (error) { logger.error({ message: 'Failed to update server URL', context: { error } }); } finally { setIsLoading(false); } };src/components/settings/__tests__/server-url-bottom-sheet-simple.test.tsx (2)
116-123: Assert the newly added input props (textContentType, returnKeyType, blurOnSubmit)These props are part of the recent changes; add expectations to lock behavior.
it('renders input field with correct keyboard properties', () => { render(<ServerUrlBottomSheet {...defaultProps} />); const inputField = screen.getByTestId('input-field'); expect(inputField.props.autoCapitalize).toBe('none'); expect(inputField.props.autoCorrect).toBe(false); expect(inputField.props.keyboardType).toBe('url'); + expect(inputField.props.textContentType).toBe('URL'); + expect(inputField.props.returnKeyType).toBe('done'); + expect(inputField.props.blurOnSubmit).toBe(true); });
6-21: Reduce over-mocking of RN Settings unless required by this testThese deep RN Settings mocks add maintenance overhead and can mask real issues. If not needed for this specific test, drop them.
Try removing these blocks locally; if tests still pass, delete them to simplify.
src/app/(app)/__tests__/contacts.test.tsx (1)
292-294: Initial fetch assertion is fine; consider making intent explicit.Using toHaveBeenCalledWith() is valid to assert no args. Optionally also assert not called with true to make the intent unmistakable.
Suggested addition:
expect(mockFetchContacts).toHaveBeenCalledTimes(1); expect(mockFetchContacts).toHaveBeenCalledWith(); // No force refresh on initial load +expect(mockFetchContacts).not.toHaveBeenCalledWith(true);src/stores/contacts/__tests__/store.test.ts (1)
5-5: Remove unused cacheManager mock/import/alias.These are not referenced; keeping them adds noise and may trigger noUnusedLocals rules.
Apply this diff:
-import { cacheManager } from '@/lib/cache/cache-manager'; @@ -jest.mock('@/lib/cache/cache-manager'); @@ -const mockCacheManager = cacheManager as jest.Mocked<typeof cacheManager>;Also applies to: 16-16, 20-20
src/api/config/index.ts (3)
7-14: Fix grammar in cache TTL commentsNit: “1 days” → “1 day” in both comments.
Apply this diff:
- ttl: 60 * 1000 * 1440, // Cache for 1 days + ttl: 60 * 1000 * 1440, // Cache for 1 day- ttl: 60 * 1000 * 1440, // Cache for 1 days + ttl: 60 * 1000 * 1440, // Cache for 1 day
16-26: Add explicit return types for stronger type safetyBeing explicit here improves readability and prevents accidental type drift.
Apply this diff:
-export const getConfig = async (key: string) => { +export const getConfig = async (key: string): Promise<GetConfigResult> => { const response = await getConfigApi.get<GetConfigResult>({ key: encodeURIComponent(key), }); return response.data; }; -export const getSystemConfig = async () => { - const response = await getSystemConfigApi.get<GetSystemConfigResult>(); - return response.data; -}; +export const getSystemConfig = async (): Promise<GetSystemConfigResult> => { + const response = await getSystemConfigApi.get<GetSystemConfigResult>(); + return response.data; +};
23-26: Consider adding a small unit test for getSystemConfig()A focused test that stubs getSystemConfigApi.get and asserts the endpoint path, call, and typed return would lock this in.
I can scaffold a Jest test that mocks ../common/cached-client, verifies the GET call to /Config/GetSystemConfig, and asserts the typed data flow. Want me to push that?
src/app/(app)/__tests__/contacts-pull-to-refresh.integration.test.tsx (3)
1-4: Use act and assert the refresh actually triggers fetchContacts(true)Import act so we can simulate the RefreshControl’s onRefresh and verify the force-refresh path unequivocally.
Apply this diff:
-import { render, waitFor } from '@testing-library/react-native'; +import { render, act } from '@testing-library/react-native';
107-133: Strengthen the first test by invoking onRefresh and asserting argumentsRight now it only checks initial call and presence of RefreshControl. Trigger onRefresh to prove fetchContacts(true) is used.
Apply this diff:
- it('should properly configure pull-to-refresh with force cache refresh', async () => { - const mockFetchContacts = jest.fn(); + it('should properly configure pull-to-refresh with force cache refresh', async () => { + const mockFetchContacts = jest.fn().mockResolvedValue(undefined); @@ - // Verify that the contacts list has refresh control - const flatList = getByTestId('contacts-list'); - expect(flatList).toBeTruthy(); - expect(flatList.props.refreshControl).toBeTruthy(); - - // The handleRefresh function should be properly configured to call fetchContacts with true - // This is tested indirectly through the component structure and our unit tests - expect(mockFetchContacts).toHaveBeenCalledWith(); // Initial load without force refresh + // Verify that the contacts list has a refresh control we can use + const flatList = getByTestId('contacts-list'); + expect(flatList).toBeTruthy(); + const refreshControl = flatList.props.refreshControl; + expect(refreshControl).toBeTruthy(); + + // Trigger pull-to-refresh and assert force refresh is used + await act(async () => { + await refreshControl.props.onRefresh(); + }); + expect(mockFetchContacts).toHaveBeenCalledTimes(2); + expect(mockFetchContacts).toHaveBeenNthCalledWith(1); + expect(mockFetchContacts).toHaveBeenNthCalledWith(2, true); });
149-162: Optionally, execute onRefresh here too to assert the force-refresh call pathThis turns the descriptive comment into an executable assertion.
Apply this diff:
// Verify refresh control is configured expect(refreshControl).toBeTruthy(); expect(refreshControl.props.refreshing).toBe(false); expect(typeof refreshControl.props.onRefresh).toBe('function'); - // The handleRefresh function implementation includes: - // 1. setRefreshing(true) - // 2. await fetchContacts(true) - with force refresh - // 3. setRefreshing(false) - // This ensures proper state management during refresh + // Trigger a refresh and ensure force refresh is used + await act(async () => { + await refreshControl.props.onRefresh(); + }); + expect(mockFetchContacts).toHaveBeenCalledWith(true);src/components/contacts/contact-notes-list.tsx (3)
45-48: Localize the fallback string and improve HTML detectionAvoid hard-coded English and make the HTML detection more robust so plain text like "2 < 3" isn't misclassified as HTML.
Apply this diff:
- // Fallback display for empty or plain text notes - const isPlainText = !note.Note || !note.Note.includes('<'); - const noteContent = note.Note || '(No content)'; + // Fallback display for empty or plain text notes + const hasHtmlTags = /<\/?[a-z][\s\S]*>/i.test(note.Note ?? ''); + const isPlainText = !hasHtmlTags; + const noteContent = note.Note?.trim() || t('contacts.noContent');
87-87: Avoid nested scrolling: support dynamic WebView heightFixed height makes long notes awkward to read and creates nested scroll areas. Auto-size the WebView to its content.
Apply this diff:
- style={styles.webView} + style={[styles.webView, { height: webViewHeight }]}Add minimal auto-height plumbing in ContactNoteCard:
// state const [webViewHeight, setWebViewHeight] = React.useState<number>(200); // in the WebView props injectedJavaScript={` (function() { var height = Math.max( document.documentElement.scrollHeight || 0, document.body.scrollHeight || 0 ); window.ReactNativeWebView.postMessage(String(height)); })(); true; `} onMessage={(e) => { const h = Number(e.nativeEvent.data); if (!Number.isNaN(h)) { // clamp to avoid pathological sizes setWebViewHeight(Math.min(Math.max(h, 200), 1000)); } }}
288-292: Replace fixed height with minHeight to play well with dynamic sizingIf you adopt auto-height, avoid hard-coding height in the StyleSheet. Keep a minimum for short content.
Apply this diff:
webView: { - height: 200, // Fixed height with scroll capability + // Height is controlled dynamically via state; keep a minimum for short content + minHeight: 200, backgroundColor: 'transparent', width: '100%', },
📜 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 (16)
docs/contacts-pull-to-refresh-implementation.md(1 hunks)package.json(1 hunks)src/api/config/index.ts(2 hunks)src/api/contacts/contacts.ts(2 hunks)src/app/(app)/__tests__/contacts-pull-to-refresh.integration.test.tsx(1 hunks)src/app/(app)/__tests__/contacts.test.tsx(1 hunks)src/app/(app)/contacts.tsx(1 hunks)src/components/contacts/contact-notes-list.tsx(5 hunks)src/components/settings/__tests__/server-url-bottom-sheet-simple.test.tsx(1 hunks)src/components/settings/server-url-bottom-sheet.tsx(2 hunks)src/models/v4/configs/getSystemConfigResult.ts(1 hunks)src/models/v4/configs/getSystemConfigResultData.ts(1 hunks)src/stores/app/__tests__/core-store.test.ts(2 hunks)src/stores/app/core-store.ts(1 hunks)src/stores/contacts/__tests__/store.test.ts(3 hunks)src/stores/contacts/store.ts(2 hunks)
🧰 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/app/(app)/contacts.tsxsrc/models/v4/configs/getSystemConfigResult.tssrc/components/settings/__tests__/server-url-bottom-sheet-simple.test.tsxsrc/stores/app/core-store.tssrc/models/v4/configs/getSystemConfigResultData.tssrc/api/contacts/contacts.tssrc/app/(app)/__tests__/contacts.test.tsxsrc/stores/contacts/__tests__/store.test.tssrc/api/config/index.tssrc/app/(app)/__tests__/contacts-pull-to-refresh.integration.test.tsxsrc/stores/app/__tests__/core-store.test.tssrc/components/settings/server-url-bottom-sheet.tsxsrc/stores/contacts/store.tssrc/components/contacts/contact-notes-list.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/(app)/contacts.tsxsrc/components/settings/__tests__/server-url-bottom-sheet-simple.test.tsxsrc/app/(app)/__tests__/contacts.test.tsxsrc/app/(app)/__tests__/contacts-pull-to-refresh.integration.test.tsxsrc/components/settings/server-url-bottom-sheet.tsxsrc/components/contacts/contact-notes-list.tsx
**/*
📄 CodeRabbit Inference Engine (.cursorrules)
Directory and file names should be lowercase and hyphenated (e.g., user-profile, chat-screen)
Files:
src/app/(app)/contacts.tsxsrc/models/v4/configs/getSystemConfigResult.tssrc/components/settings/__tests__/server-url-bottom-sheet-simple.test.tsxsrc/stores/app/core-store.tssrc/models/v4/configs/getSystemConfigResultData.tssrc/api/contacts/contacts.tssrc/app/(app)/__tests__/contacts.test.tsxsrc/stores/contacts/__tests__/store.test.tssrc/api/config/index.tssrc/app/(app)/__tests__/contacts-pull-to-refresh.integration.test.tsxsrc/stores/app/__tests__/core-store.test.tssrc/components/settings/server-url-bottom-sheet.tsxsrc/stores/contacts/store.tspackage.jsondocs/contacts-pull-to-refresh-implementation.mdsrc/components/contacts/contact-notes-list.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/(app)/contacts.tsxsrc/components/settings/__tests__/server-url-bottom-sheet-simple.test.tsxsrc/app/(app)/__tests__/contacts.test.tsxsrc/app/(app)/__tests__/contacts-pull-to-refresh.integration.test.tsxsrc/components/settings/server-url-bottom-sheet.tsxsrc/components/contacts/contact-notes-list.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/settings/__tests__/server-url-bottom-sheet-simple.test.tsxsrc/app/(app)/__tests__/contacts.test.tsxsrc/stores/contacts/__tests__/store.test.tssrc/app/(app)/__tests__/contacts-pull-to-refresh.integration.test.tsxsrc/stores/app/__tests__/core-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/components/settings/__tests__/server-url-bottom-sheet-simple.test.tsxsrc/app/(app)/__tests__/contacts.test.tsxsrc/stores/contacts/__tests__/store.test.tssrc/app/(app)/__tests__/contacts-pull-to-refresh.integration.test.tsxsrc/stores/app/__tests__/core-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/components/settings/__tests__/server-url-bottom-sheet-simple.test.tsxsrc/app/(app)/__tests__/contacts.test.tsxsrc/stores/contacts/__tests__/store.test.tssrc/app/(app)/__tests__/contacts-pull-to-refresh.integration.test.tsxsrc/stores/app/__tests__/core-store.test.ts
🧠 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/components/settings/__tests__/server-url-bottom-sheet-simple.test.tsxsrc/stores/app/__tests__/core-store.test.ts
📚 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 jest.config.@(js|ts) : Provide Jest configuration
Applied to files:
src/stores/app/__tests__/core-store.test.ts
📚 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/stores/app/__tests__/core-store.test.ts
🧬 Code Graph Analysis (6)
src/models/v4/configs/getSystemConfigResult.ts (1)
src/models/v4/configs/getSystemConfigResultData.ts (1)
GetSystemConfigResultData(1-3)
src/components/settings/__tests__/server-url-bottom-sheet-simple.test.tsx (1)
src/components/settings/server-url-bottom-sheet.tsx (1)
ServerUrlBottomSheet(30-134)
src/app/(app)/__tests__/contacts.test.tsx (2)
src/stores/contacts/store.ts (1)
useContactsStore(25-76)src/app/(app)/contacts.tsx (1)
Contacts(17-98)
src/stores/contacts/__tests__/store.test.ts (2)
src/api/contacts/contactNotes.ts (1)
getContactNotes(7-12)src/stores/contacts/store.ts (1)
useContactsStore(25-76)
src/api/config/index.ts (2)
src/models/v4/configs/getConfigResult.ts (1)
GetConfigResult(4-6)src/models/v4/configs/getSystemConfigResult.ts (1)
GetSystemConfigResult(4-6)
src/components/settings/server-url-bottom-sheet.tsx (3)
src/components/ui/index.tsx (1)
ScrollView(18-18)src/components/ui/input/index.tsx (2)
Input(174-174)InputField(174-174)src/components/ui/button/index.tsx (3)
Button(334-334)ButtonText(334-334)ButtonSpinner(334-334)
🪛 LanguageTool
docs/contacts-pull-to-refresh-implementation.md
[grammar] ~8-~8: There might be a mistake here.
Context: ...ncement (src/api/contacts/contacts.ts) - Added cache invalidation support: Modi...
(QB_NEW_EN)
[grammar] ~25-~25: There might be a mistake here.
Context: ...ncement (src/stores/contacts/store.ts) - Updated interface: Modified `fetchCont...
(QB_NEW_EN)
[grammar] ~49-~49: There might be a mistake here.
Context: ...hancement (src/app/(app)/contacts.tsx) - Pull-to-refresh improvement: Updated `...
(QB_NEW_EN)
[grammar] ~64-~64: There might be a mistake here.
Context: ...tores/contacts/tests/store.test.ts`) - Force refresh testing: Added test to v...
(QB_NEW_EN)
[grammar] ~69-~69: There might be a mistake here.
Context: .../app/(app)/tests/contacts.test.tsx`) - Refresh functionality: Added test to v...
(QB_NEW_EN)
[grammar] ~74-~74: There might be a mistake here.
Context: ...s-pull-to-refresh.integration.test.tsx`) - End-to-end verification: Created compr...
(QB_NEW_EN)
[grammar] ~81-~81: There might be a mistake here.
Context: ...res ### ✅ Pull-to-Refresh Functionality - Already implemented: The contacts page...
(QB_NEW_EN)
[grammar] ~86-~86: There might be a mistake here.
Context: ...emains unchanged ### ✅ Cache Management - Smart caching: Normal loads use cache ...
(QB_NEW_EN)
[grammar] ~91-~91: There might be a mistake here.
Context: ...ing fresh requests ### ✅ Loading States - Initial load: Shows full loading scree...
(QB_NEW_EN)
[grammar] ~96-~96: There might be a mistake here.
Context: ...es for both scenarios ### ✅ Type Safety - TypeScript support: Full type safety m...
(QB_NEW_EN)
[grammar] ~97-~97: There might be a mistake here.
Context: ...maintained throughout the implementation - Interface consistency: Proper interfac...
(QB_NEW_EN)
[grammar] ~98-~98: There might be a mistake here.
Context: ...rface definitions for all new parameters - Backward compatibility: Existing code ...
(QB_NEW_EN)
[grammar] ~103-~103: There might be a mistake here.
Context: ...tacts Store Tests: ✅ All Passing (15/15) - Initial state management - Fetch contact...
(QB_NEW_EN)
[grammar] ~104-~104: There might be a mistake here.
Context: ...ssing (15/15) - Initial state management - Fetch contacts (default and force refres...
(QB_NEW_EN)
[grammar] ~105-~105: There might be a mistake here.
Context: ...tch contacts (default and force refresh) - Error handling - Loading states - Contac...
(QB_NEW_EN)
[grammar] ~106-~106: There might be a mistake here.
Context: ...ault and force refresh) - Error handling - Loading states - Contact notes functiona...
(QB_NEW_EN)
[grammar] ~107-~107: There might be a mistake here.
Context: ...fresh) - Error handling - Loading states - Contact notes functionality - Search and...
(QB_NEW_EN)
[grammar] ~108-~108: There might be a mistake here.
Context: ...ing states - Contact notes functionality - Search and selection features ### Conta...
(QB_NEW_EN)
[grammar] ~111-~111: There might be a mistake here.
Context: ...ntacts Page Tests: ✅ All Passing (11/11) - Component rendering - Loading states - S...
(QB_NEW_EN)
[grammar] ~112-~112: There might be a mistake here.
Context: ...ll Passing (11/11) - Component rendering - Loading states - Search functionality - ...
(QB_NEW_EN)
[grammar] ~113-~113: There might be a mistake here.
Context: ...) - Component rendering - Loading states - Search functionality - Contact selection...
(QB_NEW_EN)
[grammar] ~114-~114: There might be a mistake here.
Context: ... - Loading states - Search functionality - Contact selection - Pull-to-refresh conf...
(QB_NEW_EN)
[grammar] ~115-~115: There might be a mistake here.
Context: ...Search functionality - Contact selection - Pull-to-refresh configuration - Force re...
(QB_NEW_EN)
[grammar] ~116-~116: There might be a mistake here.
Context: ...election - Pull-to-refresh configuration - Force refresh parameter verification ##...
(QB_NEW_EN)
[grammar] ~119-~119: There might be a mistake here.
Context: ...# Integration Tests: ✅ All Passing (3/3) - Pull-to-refresh configuration - Refresh ...
(QB_NEW_EN)
[grammar] ~120-~120: There might be a mistake here.
Context: ...ng (3/3) - Pull-to-refresh configuration - Refresh state management - Loading state...
(QB_NEW_EN)
[grammar] ~121-~121: There might be a mistake here.
Context: ...configuration - Refresh state management - Loading state differentiation ### All C...
(QB_NEW_EN)
[grammar] ~124-~124: There might be a mistake here.
Context: ...act-Related Tests: ✅ All Passing (66/66) - ContactCard component tests - ContactDet...
(QB_NEW_EN)
[grammar] ~125-~125: There might be a mistake here.
Context: ...ng (66/66) - ContactCard component tests - ContactDetailsSheet component tests - Co...
(QB_NEW_EN)
[grammar] ~126-~126: There might be a mistake here.
Context: ...ts - ContactDetailsSheet component tests - ContactNotesList component tests - Store...
(QB_NEW_EN)
[grammar] ~127-~127: There might be a mistake here.
Context: ...tests - ContactNotesList component tests - Store integration tests - Page component...
(QB_NEW_EN)
[grammar] ~128-~128: There might be a mistake here.
Context: ...omponent tests - Store integration tests - Page component tests ## Implementation ...
(QB_NEW_EN)
[grammar] ~133-~133: There might be a mistake here.
Context: ...plementation Notes ### Caching Strategy - Default behavior: Uses cached data for...
(QB_NEW_EN)
[grammar] ~139-~139: There might be a mistake here.
Context: ... regular navigation ### User Experience - Seamless transition: No breaking chang...
(QB_NEW_EN)
[grammar] ~140-~140: There might be a mistake here.
Context: ...eaking changes to existing functionality - Visual feedback: Pull-to-refresh indic...
(QB_NEW_EN)
[grammar] ~141-~141: There might be a mistake here.
Context: ...to-refresh indicator shows refresh state - Error handling: Graceful error handlin...
(QB_NEW_EN)
[grammar] ~142-~142: There might be a mistake here.
Context: ...error handling during refresh operations - Data freshness: Guarantees fresh data ...
(QB_NEW_EN)
[grammar] ~145-~145: There might be a mistake here.
Context: ...explicitly requests it ### Code Quality - Clean implementation: Minimal changes ...
(QB_NEW_EN)
[grammar] ~146-~146: There might be a mistake here.
Context: ...n**: Minimal changes with maximum impact - Test coverage: Comprehensive test cove...
(QB_NEW_EN)
[grammar] ~147-~147: There might be a mistake here.
Context: ...sive test coverage for new functionality - Type safety: Full TypeScript support m...
(QB_NEW_EN)
[grammar] ~148-~148: There might be a mistake here.
Context: ...ty**: Full TypeScript support maintained - Documentation: Clear code comments and...
(QB_NEW_EN)
🔇 Additional comments (12)
src/api/contacts/contacts.ts (1)
22-27: Remove redundantawaitand add explicit return typeCache invalidation via
cacheManager.remove()is synchronous (returnsvoid), so awaiting it has no effect. However, specifying an explicit return type improves type safety.• File:
src/api/contacts/contacts.ts
• Function:getAllContactsSuggested diff:
-export const getAllContacts = async (forceRefresh: boolean = false) => { +export const getAllContacts = async (forceRefresh: boolean = false): Promise<ContactsResult> => { if (forceRefresh) { // Clear cache before making the request - cacheManager.remove('/Contacts/GetAllContacts'); + cacheManager.remove('/Contacts/GetAllContacts'); }Optional: extract the cache key string (
'/Contacts/GetAllContacts') into a shared constant to prevent drift between calls toremove()and yourcreateCachedApiEndpoint.Likely an incorrect or invalid review comment.
src/components/settings/server-url-bottom-sheet.tsx (1)
69-69: ConfirmsnapPointsProp Support in UIActionsheetI’ve checked our
UIActionsheetimplementation insrc/components/ui/actionsheet/index.tsxand don’t see any explicit handling or forwarding of asnapPointsprop. Please verify that thecreateActionsheetcomponent from@gluestack-ui/actionsheetactually accepts and usessnapPoints(check its TypeScript definitions or documentation). If it doesn’t, you’ll need to:
- Extend our wrapper to forward
snapPointsto the underlying native component- Or remove the prop from
server-url-bottom-sheet.tsxto avoid silent failuresLocation:
src/components/settings/server-url-bottom-sheet.tsx, line 69:
<Actionsheet isOpen={isOpen} onClose={onClose} snapPoints={[80]}>src/stores/app/core-store.ts (1)
6-6: LGTM: import consolidated to the config barrelUsing the barrel export improves ergonomics and keeps API boundaries clean. No functional changes to fetchConfig flow.
src/stores/app/__tests__/core-store.test.ts (2)
64-65: Barrel import alignment LGTM.Importing getConfig from '@/api/config' matches the mock and the updated app store implementation.
5-7: Mock path update validated — no stale imports foundI ran the ripgrep scan for any remaining
@/api/config/configimports or mocks and found none.src/stores/contacts/__tests__/store.test.ts (1)
123-127: Store fetch tests cover both default and force-refresh paths.Good assertions: default path calls getAllContacts(false) and force path calls getAllContacts(true), with state updates verified.
Also applies to: 129-140
src/stores/contacts/store.ts (2)
18-18: Public API updated to support optional force refresh — LGTM.Signature aligns with component usage and tests.
35-39: Flag propagation to API is correct; loading and error handling preserved.This ensures UI can bypass cache on pull-to-refresh while maintaining existing state semantics.
src/models/v4/configs/getSystemConfigResult.ts (1)
4-6: Model looks consistent and correctMatches the existing v4 config result pattern (extends BaseV4Request, Data initialized). No issues spotted.
src/models/v4/configs/getSystemConfigResultData.ts (1)
1-12: LGTM: Data shapes align with API needsSimple DTOs with sensible defaults; consistent with other v4 model patterns (PascalCase props).
docs/contacts-pull-to-refresh-implementation.md (1)
1-156: Documentation is comprehensive and aligned with the implementationClear overview, rationale, and test coverage summary. Nicely ties API/store/component layers together.
src/components/contacts/contact-notes-list.tsx (1)
5-6: Named import for WebView looks correctSwitching to the named export from react-native-webview aligns with current typings and common usage.
| </head> | ||
| <body>${noteContent}</body> | ||
| </html> |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Sanitize HTML before injecting it into WebView
Raw server-provided HTML is injected directly. Even with JS disabled, unsanitized HTML can still cause rendering quirks; with JS enabled, it’s a full XSS in the WebView context. Sanitize before injection.
Confirm whether backend already sanitizes these notes. If not, sanitize on the client or server.
Apply this diff:
- <body>${noteContent}</body>
+ <body>${sanitizedHtml}</body>Add a sanitization step in ContactNoteCard:
// Add near the top of ContactNoteCard
import xss from 'xss';
// Inside ContactNoteCard:
const sanitizedHtml = React.useMemo(() => xss(noteContent), [noteContent]);Note: xss is a small, DOM-less sanitizer suitable for React Native. Alternatively, sanitize on the server to avoid adding a dependency.
🏁 Script executed:
#!/bin/bash
# Show context where `noteContent` is injected into the WebView template
rg -n -C3 'noteContent' -g 'src/components/contacts/contact-notes-list.tsx' || true
# Search for any existing HTML-sanitization logic in the frontend
rg -n 'sanitize' -g 'src' || true
rg -n 'xss' -g 'src' || trueLength of output: 2198
Sanitize HTML before injecting into the WebView
We didn’t find any existing sanitization in the frontend, and raw noteContent is being dropped directly into the <body>…</body> of the WebView’s HTML. Even if JavaScript is disabled, malformed or malicious markup can still break layout or be abused once JS is re-enabled.
Please apply the following mandatory refactor:
-
Confirm whether your backend cleans or strips dangerous HTML from
note.Note. If it does not, add sanitization on the server or—preferably for defense-in-depth—on the client as well. -
In
src/components/contacts/contact-notes-list.tsx, replace the direct injection ofnoteContentwith a sanitized variable. For example:- <body>${noteContent}</body> + <body>${sanitizedHtml}</body>
-
Near the top of
ContactNoteCard, add:import xss from 'xss'; // … const sanitizedHtml = React.useMemo(() => xss(noteContent), [noteContent]);
-
Ensure your
WebViewprops disable JavaScript if you don’t need it (javaScriptEnabled={false}), but do not rely on that alone for safety.
By sanitizing here (or on the server), you prevent any embedded scripts or malformed tags from causing XSS or layout issues.
🤖 Prompt for AI Agents
In src/components/contacts/contact-notes-list.tsx around lines 175-177,
noteContent is being injected raw into the WebView HTML body which risks XSS or
layout breakage; confirm whether the backend already strips dangerous HTML and
if not (or for defense-in-depth) sanitize on the client by importing a sanitizer
(e.g., xss), creating a memoized sanitizedHtml = React.useMemo(() =>
xss(noteContent), [noteContent]), and use sanitizedHtml in the generated HTML
instead of noteContent; additionally ensure the WebView props explicitly disable
JavaScript (javaScriptEnabled={false}) unless needed.
src/components/settings/__tests__/server-url-bottom-sheet-simple.test.tsx
Show resolved
Hide resolved
|
Approve |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/app/(app)/contacts.tsx (1)
37-41: Spinner reset fixed with try/finally — nice follow-through.This addresses the earlier feedback about the refresh spinner getting stuck on errors. Looks good.
src/components/contacts/contact-notes-list.tsx (1)
45-48: Sanitize HTML before injecting; internationalize the plain-text fallbackRaw note HTML is still injected into WebView. Even with JS off, unsanitized markup is risky (and JS may be toggled later). Also, the literal “(No content)” should be localized.
Apply these diffs:
- // Fallback display for empty or plain text notes - const isPlainText = !note.Note || !note.Note.includes('<'); - const noteContent = note.Note || '(No content)'; + // Fallback display for empty or plain text notes + const isPlainText = !note.Note || !/<[^>]+>/.test(note.Note); + const noteContent = note.Note || t('contacts.noContent'); + // Sanitize HTML defensively before injecting into WebView + const sanitizedHtml = React.useMemo(() => xss(noteContent), [noteContent]);- <body>${noteContent}</body> + <body>${sanitizedHtml}</body>Add this import (outside the shown range):
import xss from 'xss';If you prefer server-side sanitization, keep the client-side sanitizer for defense-in-depth. I can wire this end-to-end (including adding the i18n key) if helpful.
Also applies to: 196-197
🧹 Nitpick comments (11)
src/app/(app)/contacts.tsx (4)
35-42: Optionally capture and instrument refresh errors (analytics/toast).Consider catching the error to track failures (and optionally notify the user). This keeps the UX transparent and helps with diagnostics.
const handleRefresh = React.useCallback(async () => { setRefreshing(true); try { - await fetchContacts(true); // Force refresh to bypass cache - } finally { + await fetchContacts(true); // Force refresh to bypass cache + } catch (err) { + trackEvent('contacts_refresh_failed', { + message: err instanceof Error ? err.message : String(err), + }); + // TODO: Optionally show a toast/snackbar here + } finally { setRefreshing(false); } - }, [fetchContacts]); + }, [fetchContacts, trackEvent]);
84-92: FlatList perf: avoid inline renderItem and add virtualization props.Inline renderItem creates a new function per render and can trigger unnecessary re-renders. Also, enabling virtualization props improves scroll performance on large lists.
<FlatList testID="contacts-list" data={filteredContacts} - keyExtractor={(item) => item.ContactId} - renderItem={({ item }) => <ContactCard contact={item} onPress={selectContact} />} + keyExtractor={keyExtractor} + renderItem={renderItem} showsVerticalScrollIndicator={false} contentContainerStyle={{ paddingBottom: 100 }} + removeClippedSubviews + maxToRenderPerBatch={10} + windowSize={7} refreshControl={<RefreshControl refreshing={refreshing} onRefresh={handleRefresh} />} />Place this near the other hooks (above the return):
// Add to imports (top of file): // import { ListRenderItem } from 'react-native'; type Contact = { ContactId: string | number; FirstName?: string; LastName?: string; Email?: string; CompanyName?: string; OtherName?: string; }; const keyExtractor = React.useCallback((item: Contact) => String(item.ContactId), []); const renderItem = React.useCallback<ListRenderItem<Contact>>( ({ item }) => <ContactCard contact={item} onPress={selectContact} />, [selectContact] );
44-56: Normalize search query once; reuse to reduce repeated trim/compute.Small readability/perf cleanup and consistent whitespace handling.
const filteredContacts = React.useMemo(() => { - if (!searchQuery.trim()) return contacts; - - const query = searchQuery.toLowerCase(); + const normalizedQuery = searchQuery.trim().toLowerCase(); + if (!normalizedQuery) return contacts; return contacts.filter( (contact) => - (contact.FirstName?.toLowerCase().includes(query) ?? false) || - (contact.LastName?.toLowerCase().includes(query) ?? false) || - (contact.Email?.toLowerCase().includes(query) ?? false) || - (contact.CompanyName?.toLowerCase().includes(query) ?? false) || - (contact.OtherName?.toLowerCase().includes(query) ?? false) + (contact.FirstName?.toLowerCase().includes(normalizedQuery) ?? false) || + (contact.LastName?.toLowerCase().includes(normalizedQuery) ?? false) || + (contact.Email?.toLowerCase().includes(normalizedQuery) ?? false) || + (contact.CompanyName?.toLowerCase().includes(normalizedQuery) ?? false) || + (contact.OtherName?.toLowerCase().includes(normalizedQuery) ?? false) ); }, [contacts, searchQuery]);
27-33: Analytics effect fires on every search change; consider firing “rendered” once.If the intent is to track only the initial render, gate the call so it runs once. You can separately track search changes if desired.
- // Track when contacts view is rendered - React.useEffect(() => { - trackEvent('contacts_view_rendered', { - contactsCount: contacts.length, - hasSearchQuery: searchQuery.length > 0, - }); - }, [trackEvent, contacts.length, searchQuery]); + // Track when contacts view is first rendered (once) + const renderedRef = React.useRef(false); + React.useEffect(() => { + if (!renderedRef.current) { + renderedRef.current = true; + trackEvent('contacts_view_rendered', { + contactsCount: contacts.length, + }); + } + }, [trackEvent, contacts.length]); + + // Optionally, track search state changes separately + // React.useEffect(() => { + // trackEvent('contacts_search_changed', { hasSearchQuery: searchQuery.length > 0 }); + // }, [trackEvent, searchQuery]);src/components/settings/__tests__/server-url-bottom-sheet-simple.test.tsx (4)
32-40: Make react-hook-form handleSubmit behave like real APIReturning a no-op function today is fine, but aligning with RHF’s API will future-proof “Save” button interaction tests.
Apply this diff:
-jest.mock('react-hook-form', () => ({ - useForm: () => ({ - control: {}, - handleSubmit: jest.fn(), - setValue: jest.fn(), - formState: { errors: {} }, - }), - Controller: ({ render }: any) => render({ field: { onChange: jest.fn(), value: '' } }), -})); +jest.mock('react-hook-form', () => ({ + useForm: () => ({ + control: {}, + // Return a function that invokes the provided callback with minimal form data + handleSubmit: (fn: any) => () => fn({ url: '' }), + setValue: jest.fn(), + formState: { errors: {} }, + }), + Controller: ({ render }: any) => render({ field: { onChange: jest.fn(), value: '' } }), +}));
52-67: Use real RN host components in mocks instead of string element typesUsing 'View'/'Text'/'TextInput' strings works in many cases, but referencing the actual RN components reduces renderer edge cases and keeps prop behavior consistent.
Apply this diff:
const createMockUIComponent = (displayName: string) => ({ children, testID, ...props }: any) => { const React = require('react'); - return React.createElement('View', { testID: testID || displayName.toLowerCase(), ...props }, children); + const { View } = require('react-native'); + return React.createElement(View, { testID: testID || displayName.toLowerCase(), ...props }, children); }; const createMockTextComponent = (displayName: string) => ({ children, testID, ...props }: any) => { const React = require('react'); - return React.createElement('Text', { testID: testID || displayName.toLowerCase(), ...props }, children); + const { Text } = require('react-native'); + return React.createElement(Text, { testID: testID || displayName.toLowerCase(), ...props }, children); }; const createMockInputComponent = ({ testID, ...props }: any) => { const React = require('react'); - return React.createElement('TextInput', { testID: testID || 'input-field', ...props }); + const { TextInput } = require('react-native'); + return React.createElement(TextInput, { testID: testID || 'input-field', ...props }); };
68-75: Forward props in Actionsheet mock to preserve handlers and attributesThis keeps onClose and other props available for future interaction tests while keeping the conditional rendering behavior.
Apply this diff:
-jest.mock('../../ui/actionsheet', () => ({ - Actionsheet: ({ children, isOpen }: any) => isOpen ? createMockUIComponent('Actionsheet')({ children }) : null, +jest.mock('../../ui/actionsheet', () => ({ + Actionsheet: ({ isOpen, ...props }: any) => + isOpen ? createMockUIComponent('Actionsheet')({ ...props }) : null, ActionsheetBackdrop: createMockUIComponent('ActionsheetBackdrop'), ActionsheetContent: createMockUIComponent('ActionsheetContent'), ActionsheetDragIndicator: createMockUIComponent('ActionsheetDragIndicator'), ActionsheetDragIndicatorWrapper: createMockUIComponent('ActionsheetDragIndicatorWrapper'), }));
116-123: Expand assertions to cover newly added input props (prevents regressions)The component sets textContentType, returnKeyType, and blurOnSubmit. Verifying them will better align tests with the UI contract.
Apply this diff:
const inputField = screen.getByTestId('input-field'); expect(inputField.props.autoCapitalize).toBe('none'); expect(inputField.props.autoCorrect).toBe(false); expect(inputField.props.keyboardType).toBe('url'); + expect(inputField.props.textContentType).toBe('URL'); + expect(inputField.props.returnKeyType).toBe('done'); + expect(inputField.props.blurOnSubmit).toBe(true);src/components/contacts/contact-notes-list.tsx (3)
294-301: Prefer FlatList for rendering note cards to enable virtualizationFor a potentially unbounded list, FlatList avoids mounting all WebViews at once and aligns with performance guidelines.
Apply this diff:
- return ( - <VStack space="md" className="flex-1 p-4"> - {sortedNotes.map((note) => ( - <ContactNoteCard key={note.ContactNoteId} note={note} /> - ))} - </VStack> - ); + return ( + <FlatList + contentContainerStyle={{ padding: 16 }} + data={sortedNotes} + keyExtractor={(item) => String(item.ContactNoteId)} + renderItem={({ item }) => <ContactNoteCard note={item} />} + initialNumToRender={5} + windowSize={7} + maxToRenderPerBatch={10} + updateCellsBatchingPeriod={50} + removeClippedSubviews + /> + );And add this import (outside the shown range):
import { FlatList } from 'react-native';
35-35: Remove unused backgroundColor constantThis variable isn’t used; drop it to keep things tidy (or apply it where intended).
- const backgroundColor = colorScheme === 'dark' ? '#374151' : '#F9FAFB';
30-31: Use a single source of truth for expiration logic and displayContactNoteResultData exposes both ExpiresOnUtc (Date) and ExpiresOn (string ISO). Today you use ExpiresOnUtc to compute
isExpiredbut fall back to formatDate(note.ExpiresOn) for the UI, which can lead to subtle timezone or parsing mismatches. To eliminate any confusion, reference the same field in both places—preferably the already-parsed ExpiresOnUtc.• In src/components/contacts/contact-notes-list.tsx:
– Replace thenote.ExpiresOnguard and display withnote.ExpiresOnUtc.
– Update the conditional and formatting at lines 210–215 to use formatDate(note.ExpiresOnUtc).Example diff:
- ) : note.ExpiresOn ? ( + ) : note.ExpiresOnUtc ? ( <HStack space="xs" className="items-center"> <ClockIcon size={14} color="#6b7280" /> - <Text className="text-xs text-gray-500 dark:text-gray-400"> - {t('contacts.expires')}: {formatDate(note.ExpiresOn)} + <Text className="text-xs text-gray-500 dark:text-gray-400"> + {t('contacts.expires')}: {formatDate(note.ExpiresOnUtc)} </Text> </HStack>Also ensure the
note.ExpiresOnUtcguard replaces any other fallbacks in this file (e.g. at lines 30–31 and 210–215). If the UX requires a localized display string, you can still derive it solely fromExpiresOnUtc—this keeps logic and presentation aligned.
📜 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)
src/api/config/index.ts(2 hunks)src/app/(app)/__tests__/contacts.test.tsx(2 hunks)src/app/(app)/contacts.tsx(1 hunks)src/components/contacts/contact-notes-list.tsx(5 hunks)src/components/settings/__tests__/server-url-bottom-sheet-simple.test.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/api/config/index.ts
- src/app/(app)/tests/contacts.test.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/app/(app)/contacts.tsxsrc/components/settings/__tests__/server-url-bottom-sheet-simple.test.tsxsrc/components/contacts/contact-notes-list.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/(app)/contacts.tsxsrc/components/settings/__tests__/server-url-bottom-sheet-simple.test.tsxsrc/components/contacts/contact-notes-list.tsx
**/*
📄 CodeRabbit Inference Engine (.cursorrules)
Directory and file names should be lowercase and hyphenated (e.g., user-profile, chat-screen)
Files:
src/app/(app)/contacts.tsxsrc/components/settings/__tests__/server-url-bottom-sheet-simple.test.tsxsrc/components/contacts/contact-notes-list.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/(app)/contacts.tsxsrc/components/settings/__tests__/server-url-bottom-sheet-simple.test.tsxsrc/components/contacts/contact-notes-list.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/settings/__tests__/server-url-bottom-sheet-simple.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/settings/__tests__/server-url-bottom-sheet-simple.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/settings/__tests__/server-url-bottom-sheet-simple.test.tsx
🧠 Learnings (2)
📚 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/components/settings/__tests__/server-url-bottom-sheet-simple.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 **/*.tsx : Use React Navigation for navigation and deep linking with best practices
Applied to files:
src/components/contacts/contact-notes-list.tsx
🧬 Code Graph Analysis (1)
src/components/settings/__tests__/server-url-bottom-sheet-simple.test.tsx (1)
src/components/settings/server-url-bottom-sheet.tsx (1)
ServerUrlBottomSheet(30-134)
🔇 Additional comments (4)
src/components/settings/__tests__/server-url-bottom-sheet-simple.test.tsx (3)
23-30: Partial mock of react-native is correct and avoids brittle full-module stubsThis addresses prior feedback by preserving real exports while overriding only Platform.OS. Good call.
106-114: Visibility tests are focused and stableAsserting presence/absence via the Actionsheet testID is straightforward and resilient given the current mock setup.
15-21: server-url-bottom-sheet-simple.test.tsx: reconsider RN Settings mocksI ran a code search and found that these deep-internal Settings mocks only live in this one test file—no other TS/TSX in the repo imports
react-native/Libraries/Settings/Settings.iosorNativeSettingsManager. If your tests still pass without them, it’s safer to drop these mocks to reduce future maintenance. Otherwise, please leave them in place with a brief comment explaining which warning or error they silence.• File needing attention:
- src/components/settings/tests/server-url-bottom-sheet-simple.test.tsx
• Lines to review/remove:- jest.mock('react-native/Libraries/Settings/Settings.ios', () => ({})); - jest.mock('react-native/Libraries/Settings/NativeSettingsManager', () => ({ - getConstants: () => ({}), - get: jest.fn(), - set: jest.fn(), - }));• Next steps:
- Comment out or delete these mocks and re-run the test suite.
- If you encounter failures or warnings, restore them with a short comment (e.g. “Silences RN Settings warning in iOS”).
- Otherwise, commit the removal to simplify the test.
src/components/contacts/contact-notes-list.tsx (1)
96-98: Good hardening: JS and DOM storage disabled in WebViewDisabling JavaScript and DOM storage by default is the right default posture for semi/untrusted content.
| originWhitelist={['about:']} | ||
| scrollEnabled={true} | ||
| showsVerticalScrollIndicator={true} | ||
| showsHorizontalScrollIndicator={false} | ||
| androidLayerType="software" | ||
| // Security: Disable JavaScript and DOM storage by default | ||
| // Only re-enable for pre-sanitized, trusted content that requires it | ||
| javaScriptEnabled={false} | ||
| domStorageEnabled={false} | ||
| startInLoadingState={false} | ||
| mixedContentMode="compatibility" | ||
| // Security: Handle navigation to prevent in-WebView navigation and open external links safely | ||
| onShouldStartLoadWithRequest={(request) => { | ||
| // Allow initial load of our HTML content | ||
| if (request.url.startsWith('about:') || request.url.startsWith('data:')) { | ||
| return true; | ||
| } | ||
|
|
||
| // For any external links, open in system browser instead | ||
| Linking.openURL(request.url); | ||
| return false; | ||
| }} | ||
| onNavigationStateChange={(navState) => { | ||
| // Additional protection: if navigation occurs to external URL, open in system browser | ||
| if (navState.url && !navState.url.startsWith('about:') && !navState.url.startsWith('data:')) { | ||
| Linking.openURL(navState.url); | ||
| } | ||
| }} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Tighten WebView origin policy and external link handling; remove redundant navigation handler
- Use about:blank precisely (not about:) to match the inline HTML load.
- Drop mixedContentMode="compatibility" (unnecessary risk surface for local content).
- Gate external URLs to http(s)/tel/mailto, and guard Linking.openURL errors.
- Rely on onShouldStartLoadWithRequest; remove onNavigationStateChange to avoid duplicate opens. Also set setSupportMultipleWindows={false} to prevent target=_blank popups.
Apply this diff:
- originWhitelist={['about:']}
+ originWhitelist={['about:blank']}
scrollEnabled={true}
showsVerticalScrollIndicator={true}
showsHorizontalScrollIndicator={false}
androidLayerType="software"
// Security: Disable JavaScript and DOM storage by default
// Only re-enable for pre-sanitized, trusted content that requires it
javaScriptEnabled={false}
domStorageEnabled={false}
startInLoadingState={false}
- mixedContentMode="compatibility"
+ setSupportMultipleWindows={false}
// Security: Handle navigation to prevent in-WebView navigation and open external links safely
- onShouldStartLoadWithRequest={(request) => {
- // Allow initial load of our HTML content
- if (request.url.startsWith('about:') || request.url.startsWith('data:')) {
- return true;
- }
-
- // For any external links, open in system browser instead
- Linking.openURL(request.url);
- return false;
- }}
- onNavigationStateChange={(navState) => {
- // Additional protection: if navigation occurs to external URL, open in system browser
- if (navState.url && !navState.url.startsWith('about:') && !navState.url.startsWith('data:')) {
- Linking.openURL(navState.url);
- }
- }}
+ onShouldStartLoadWithRequest={(request) => {
+ const url = request?.url ?? '';
+ // Allow initial load and data URIs
+ if (url.startsWith('about:') || url.startsWith('data:')) return true;
+ // Only allow safe external schemes and open in system handlers
+ const isHttp = url.startsWith('http://') || url.startsWith('https://');
+ const isTel = url.startsWith('tel:');
+ const isMail = url.startsWith('mailto:');
+ if (isHttp || isTel || isMail) {
+ Linking.openURL(url).catch(() => {});
+ }
+ return false;
+ }}📝 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.
| originWhitelist={['about:']} | |
| scrollEnabled={true} | |
| showsVerticalScrollIndicator={true} | |
| showsHorizontalScrollIndicator={false} | |
| androidLayerType="software" | |
| // Security: Disable JavaScript and DOM storage by default | |
| // Only re-enable for pre-sanitized, trusted content that requires it | |
| javaScriptEnabled={false} | |
| domStorageEnabled={false} | |
| startInLoadingState={false} | |
| mixedContentMode="compatibility" | |
| // Security: Handle navigation to prevent in-WebView navigation and open external links safely | |
| onShouldStartLoadWithRequest={(request) => { | |
| // Allow initial load of our HTML content | |
| if (request.url.startsWith('about:') || request.url.startsWith('data:')) { | |
| return true; | |
| } | |
| // For any external links, open in system browser instead | |
| Linking.openURL(request.url); | |
| return false; | |
| }} | |
| onNavigationStateChange={(navState) => { | |
| // Additional protection: if navigation occurs to external URL, open in system browser | |
| if (navState.url && !navState.url.startsWith('about:') && !navState.url.startsWith('data:')) { | |
| Linking.openURL(navState.url); | |
| } | |
| }} | |
| originWhitelist={['about:blank']} | |
| scrollEnabled={true} | |
| showsVerticalScrollIndicator={true} | |
| showsHorizontalScrollIndicator={false} | |
| androidLayerType="software" | |
| // Security: Disable JavaScript and DOM storage by default | |
| // Only re-enable for pre-sanitized, trusted content that requires it | |
| javaScriptEnabled={false} | |
| domStorageEnabled={false} | |
| startInLoadingState={false} | |
| setSupportMultipleWindows={false} | |
| // Security: Handle navigation to prevent in-WebView navigation and open external links safely | |
| onShouldStartLoadWithRequest={(request) => { | |
| const url = request?.url ?? ''; | |
| // Allow initial load and data URIs | |
| if (url.startsWith('about:') || url.startsWith('data:')) { | |
| return true; | |
| } | |
| // Only allow safe external schemes and open in system handlers | |
| const isHttp = url.startsWith('http://') || url.startsWith('https://'); | |
| const isTel = url.startsWith('tel:'); | |
| const isMail = url.startsWith('mailto:'); | |
| if (isHttp || isTel || isMail) { | |
| Linking.openURL(url).catch(() => {}); | |
| } | |
| return false; | |
| }} |
🤖 Prompt for AI Agents
In src/components/contacts/contact-notes-list.tsx around lines 89 to 116,
tighten the WebView security by changing the origin whitelist to 'about:blank'
(to match the inline HTML), remove the mixedContentMode prop, add
setSupportMultipleWindows={false} to disable popups, gate external link handling
in onShouldStartLoadWithRequest to only allow http(s), tel, and mailto schemes
(reject everything else), and wrap Linking.openURL with a safe check/guard
(e.g., Linking.canOpenURL or try/catch) to avoid unhandled errors; also remove
the onNavigationStateChange handler to prevent duplicate external opens and rely
solely on onShouldStartLoadWithRequest.
Summary by CodeRabbit
New Features
UI/UX
Documentation
Tests
Chores