Implement friends balance retrieval and enhance HomeScreen actions #114
Conversation
…n; enhance balance calculation logic
…actions; remove redundant buttons
❌ Deploy Preview for splitwizer failed. Why did it fail? →
|
WalkthroughThe changes introduce a new API function to fetch friends' balance data and update the FriendsScreen to use this endpoint, simplifying balance calculations and improving UI feedback. Expense payer logic is updated across relevant files to prioritize a new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant FriendsScreen
participant API
participant Backend
User->>FriendsScreen: Open Friends Screen
FriendsScreen->>API: getFriendsBalance(token)
API->>Backend: GET /users/me/friends-balance
Backend-->>API: Friends balance data
API-->>FriendsScreen: Friends balance data
FriendsScreen->>User: Render friends balances with tooltips
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15 minutes Suggested labels
Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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). (4)
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 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 comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #114 +/- ##
=======================================
Coverage 71.86% 71.86%
=======================================
Files 17 17
Lines 1667 1667
Branches 153 153
=======================================
Hits 1198 1198
Misses 416 416
Partials 53 53
🚀 New features to boost your workflow:
|
|
🚀 Expo preview is ready!
|
|
Note Unit test generation is an Early Access feature. Expect some limitations and changes as we gather feedback and continue to improve it. Generating unit tests... This may take up to 20 minutes. |
…and screen components
|
Creating a local commit for the generated unit tests... The unit tests have been committed to the current branch. Commit ID: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (6)
frontend/package.json (1)
32-32: Consider updating @babel/core to a more recent versionThe specified version
^7.20.0is from 2022. Consider updating to^7.25.0or later for better compatibility and bug fixes.- "@babel/core": "^7.20.0", + "@babel/core": "^7.25.0",frontend/api/groups.test.js (2)
39-39: Avoid using require() in ES6 modulesThe code uses
require('./groups')which is inconsistent with ES6 module syntax used elsewhere. Consider using dynamic import or restructuring the test.- // Import the module to trigger axios.create - require('./groups'); + // Import the module to trigger axios.create + jest.isolateModules(() => { + require('./groups'); + });
807-812: Consider reducing performance test timeout thresholdA 5-second timeout for processing 100 groups with 50 members and 100 expenses each seems excessive for a calculation function. Consider a more aggressive target.
- expect(endTime - startTime).toBeLessThan(5000); // Should complete within 5 seconds + expect(endTime - startTime).toBeLessThan(1000); // Should complete within 1 secondfrontend/screens/HomeScreen.test.js (1)
809-829: Test for memory leaks with auth context changesThe lifecycle test checks functionality but doesn't verify that old subscriptions/effects are cleaned up properly.
Add a test to verify cleanup:
it('should cleanup effects when component unmounts', async () => { const { unmount } = renderWithAuthContext(); await waitFor(() => { expect(getGroups).toHaveBeenCalledTimes(1); }); unmount(); // Verify no additional API calls are made after unmount await act(async () => { await new Promise(resolve => setTimeout(resolve, 100)); }); expect(getGroups).toHaveBeenCalledTimes(1); });frontend/screens/GroupDetailsScreen.test.js (2)
104-104: Redundant mockClear() call
Alert.alertis already defined asjest.fn()in the React Native mock (line 21), which creates a fresh mock function. ThemockClear()call here is redundant sincejest.clearAllMocks()on line 103 already clears all mocks.beforeEach(() => { jest.clearAllMocks(); - Alert.alert.mockClear(); });
798-800: Use more specific variable declarationConsider using a more specific initial value or type for better code clarity.
it('should update loading state correctly during data fetch', async () => { - let resolvePromises; + let resolvePromises = null; const promises = new Promise(resolve => { resolvePromises = resolve; });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
frontend/api/groups.test.js(1 hunks)frontend/babel.config.js(1 hunks)frontend/jest.setup.js(1 hunks)frontend/package.json(2 hunks)frontend/screens/FriendsScreen.test.js(1 hunks)frontend/screens/GroupDetailsScreen.test.js(1 hunks)frontend/screens/HomeScreen.js(1 hunks)frontend/screens/HomeScreen.test.js(1 hunks)frontend/utils/balanceCalculator.test.js(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- frontend/babel.config.js
- frontend/jest.setup.js
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/screens/HomeScreen.js
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
PR: Devasy23/splitwiser#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-07-26T09:41:01.332Z
Learning: Applies to backend/app/expenses/**/*.py : Settlements must track debt resolution between users in the expense tracking logic.
Learnt from: CR
PR: Devasy23/splitwiser#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-07-26T09:41:01.332Z
Learning: Applies to backend/app/expenses/**/*.py : Support for different expense split types (equal, unequal, percentage-based) must be implemented in the expense tracking logic.
🧬 Code Graph Analysis (1)
frontend/screens/GroupDetailsScreen.test.js (3)
frontend/screens/FriendsScreen.test.js (3)
RN(19-19)mockAuthContext(39-42)renderWithContext(80-86)frontend/screens/HomeScreen.test.js (1)
mockNavigation(22-24)frontend/screens/GroupDetailsScreen.js (2)
GroupDetailsScreen(7-188)settlements(12-12)
🔇 Additional comments (2)
frontend/utils/balanceCalculator.test.js (1)
693-696: Parsed amounts are already validatedUpon inspection of
frontend/utils/balanceCalculator.js, both split amounts and group balances are explicitly guarded against invalid parsing:
- Around line 96–99,
splitAmountis parsed withparseFloatand immediately filtered byisNaN(splitAmount)andsplitAmount <= 0.- Around line 145–150, group balances are parsed and then filtered using
!isNaN(group.balance).No additional validation is needed for string-to-number parsing in these sections—please disregard the original suggestion.
Likely an incorrect or invalid review comment.
frontend/screens/GroupDetailsScreen.test.js (1)
1-1078: Excellent comprehensive test coverage!This test file provides thorough coverage of the GroupDetailsScreen component with well-organized test suites covering:
- Initial loading and setup
- Data fetching with various API response scenarios
- Currency formatting
- Member name resolution
- Expense rendering with different payment scenarios
- Settlement summary calculations
- Empty states and edge cases
- Navigation functionality
- State management and performance
The testing patterns follow best practices with proper mocking, context providers, and async handling.
frontend/api/groups.test.js
Outdated
| data: { | ||
| totalOwed: 0, | ||
| totalOwing: 0, | ||
| netBalance: 0 | ||
| } | ||
| }; | ||
| mockApiClient.get.mockResolvedValue(mockResponse); | ||
|
|
||
| const result = await getUserBalanceSummary(mockToken); | ||
|
|
There was a problem hiding this comment.
Console spy not properly restored in some test paths
The console.warn spy is created but only restored in the happy path. If the test fails before line 579, the spy won't be restored.
it('should log warnings for invalid input', () => {
const consoleSpy = jest.spyOn(console, 'warn').mockImplementation(() => {});
- calculateFriendBalances(null, 'currentUser');
- expect(consoleSpy).toHaveBeenCalledWith('Invalid input to calculateFriendBalances:', { groupsWithDetails: null, currentUserId: 'currentUser' });
-
- calculateFriendBalances([], null);
- expect(consoleSpy).toHaveBeenCalledWith('Invalid input to calculateFriendBalances:', { groupsWithDetails: [], currentUserId: null });
-
- consoleSpy.mockRestore();
+ try {
+ calculateFriendBalances(null, 'currentUser');
+ expect(consoleSpy).toHaveBeenCalledWith('Invalid input to calculateFriendBalances:', { groupsWithDetails: null, currentUserId: 'currentUser' });
+
+ calculateFriendBalances([], null);
+ expect(consoleSpy).toHaveBeenCalledWith('Invalid input to calculateFriendBalances:', { groupsWithDetails: [], currentUserId: null });
+ } finally {
+ consoleSpy.mockRestore();
+ }
});Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In frontend/api/groups.test.js around lines 571 to 580, the console.warn spy is
created but only restored in the successful test path, risking it not being
restored if the test fails earlier. To fix this, ensure the console.warn spy is
restored in a finally block or use afterEach/afterAll hooks to restore the spy
regardless of test outcome, guaranteeing cleanup in all test paths.
frontend/package.json
Outdated
| "private": true, | ||
| "jest": { | ||
| "preset": "react-native", | ||
| "testEnvironment": "jsdom", |
There was a problem hiding this comment.
Incorrect test environment for React Native
React Native tests typically run in a Node environment, not jsdom. The jsdom environment is for web-based React applications.
- "testEnvironment": "jsdom",
+ "testEnvironment": "node",📝 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.
| "testEnvironment": "jsdom", | |
| "testEnvironment": "node", |
🤖 Prompt for AI Agents
In frontend/package.json at line 40, the testEnvironment is set to "jsdom",
which is incorrect for React Native tests. Change the testEnvironment value from
"jsdom" to "node" to properly configure the testing environment for React
Native.
frontend/package.json
Outdated
| "collectCoverageFrom": [ | ||
| "api/**/*.{js,jsx}", | ||
| "!api/**/*.test.{js,jsx}", | ||
| "!**/node_modules/**" | ||
| ] |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Expand test coverage collection to include screens and utils
Currently, coverage is only collected from the api directory. Consider including other important directories like screens, utils, and context.
"collectCoverageFrom": [
"api/**/*.{js,jsx}",
+ "screens/**/*.{js,jsx}",
+ "utils/**/*.{js,jsx}",
+ "context/**/*.{js,jsx}",
"!api/**/*.test.{js,jsx}",
+ "!screens/**/*.test.{js,jsx}",
+ "!utils/**/*.test.{js,jsx}",
+ "!context/**/*.test.{js,jsx}",
"!**/node_modules/**"
]📝 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.
| "collectCoverageFrom": [ | |
| "api/**/*.{js,jsx}", | |
| "!api/**/*.test.{js,jsx}", | |
| "!**/node_modules/**" | |
| ] | |
| "collectCoverageFrom": [ | |
| "api/**/*.{js,jsx}", | |
| "screens/**/*.{js,jsx}", | |
| "utils/**/*.{js,jsx}", | |
| "context/**/*.{js,jsx}", | |
| "!api/**/*.test.{js,jsx}", | |
| "!screens/**/*.test.{js,jsx}", | |
| "!utils/**/*.test.{js,jsx}", | |
| "!context/**/*.test.{js,jsx}", | |
| "!**/node_modules/**" | |
| ] |
🤖 Prompt for AI Agents
In frontend/package.json around lines 56 to 60, the test coverage collection is
limited to the api directory. Update the collectCoverageFrom array to also
include files from the screens, utils, and context directories by adding
appropriate glob patterns for these directories, while continuing to exclude
test files and node_modules as before.
| const originalConsoleError = console.error; | ||
| beforeAll(() => { | ||
| console.error = jest.fn(); | ||
| }); | ||
|
|
||
| afterAll(() => { | ||
| console.error = originalConsoleError; | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Console error mock not properly restored on test failure
The console.error is mocked but restoration depends on all tests passing. Use Jest's built-in console mocking instead.
-// Mock console.error to avoid noise in tests
-const originalConsoleError = console.error;
-beforeAll(() => {
- console.error = jest.fn();
-});
-
-afterAll(() => {
- console.error = originalConsoleError;
-});
+// Mock console.error to avoid noise in tests
+beforeEach(() => {
+ jest.spyOn(console, 'error').mockImplementation(() => {});
+});
+
+afterEach(() => {
+ console.error.mockRestore();
+});📝 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.
| const originalConsoleError = console.error; | |
| beforeAll(() => { | |
| console.error = jest.fn(); | |
| }); | |
| afterAll(() => { | |
| console.error = originalConsoleError; | |
| }); | |
| // Mock console.error to avoid noise in tests | |
| beforeEach(() => { | |
| jest.spyOn(console, 'error').mockImplementation(() => {}); | |
| }); | |
| afterEach(() => { | |
| console.error.mockRestore(); | |
| }); |
🤖 Prompt for AI Agents
In frontend/screens/FriendsScreen.test.js around lines 29 to 36, the
console.error mock is manually overridden and restored, which can fail if tests
error out before restoration. Replace this manual mocking with Jest's built-in
console mocking utilities such as jest.spyOn(console,
'error').mockImplementation() in beforeAll and restore it properly in afterAll
or use jest.restoreAllMocks() to ensure console.error is always restored even if
tests fail.
| const iconButtons = UNSAFE_getByType('IconButton'); | ||
| fireEvent.press(iconButtons); |
There was a problem hiding this comment.
Avoid using UNSAFE_getByType in tests
UNSAFE_getByType is marked as unsafe and may break in future versions. Consider using more stable query methods.
- // Find the IconButton by type since it may not have accessible role
- const iconButtons = UNSAFE_getByType('IconButton');
- fireEvent.press(iconButtons);
+ // Find the close button by its testID or accessible role
+ const closeButton = getByTestId('tooltip-close-button') || getByLabelText('Close tooltip');
+ fireEvent.press(closeButton);Note: This change requires adding a testID or accessibilityLabel to the IconButton component in the actual FriendsScreen implementation.
Also applies to: 482-483
🤖 Prompt for AI Agents
In frontend/screens/FriendsScreen.test.js around lines 126-127 and also lines
482-483, replace the use of UNSAFE_getByType('IconButton') with a more stable
query method like getByTestId or getByAccessibilityLabel. To do this, first add
a testID or accessibilityLabel prop to the IconButton component in the
FriendsScreen source code, then update the test to query IconButton using that
identifier and trigger the press event on the found element.
frontend/screens/HomeScreen.test.js
Outdated
| })); | ||
|
|
||
| // Mock Alert | ||
| jest.spyOn(Alert, 'alert').mockImplementation(() => {}); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Alert mock should be properly restored
The Alert.alert is mocked with spyOn but never restored, which could affect other tests.
-// Mock Alert
-jest.spyOn(Alert, 'alert').mockImplementation(() => {});
+// Mock Alert will be set up in beforeEach
// In the describe block:
beforeEach(() => {
jest.clearAllMocks();
- Alert.alert.mockClear();
+ jest.spyOn(Alert, 'alert').mockImplementation(() => {});
mockNavigation.navigate.mockClear();
mockConsoleError.mockClear();
});
+
+afterEach(() => {
+ Alert.alert.mockRestore();
+});Also applies to: 44-44
🤖 Prompt for AI Agents
In frontend/screens/HomeScreen.test.js at lines 16 and 44, the Alert.alert
method is mocked using jest.spyOn but the mock is never restored, which can
cause side effects in other tests. To fix this, add code to restore the original
Alert.alert implementation after each test, typically by calling mockRestore()
in an afterEach or afterAll block to ensure the mock does not persist beyond the
intended test scope.
frontend/screens/HomeScreen.test.js
Outdated
| getOptimizedSettlements.mockImplementation(() => new Promise(resolve => | ||
| setTimeout(() => resolve({ data: { optimizedSettlements: [] } }), 100) | ||
| )); |
There was a problem hiding this comment.
setTimeout without cleanup can cause test flakiness
The setTimeout in the mock implementation should be cleaned up to prevent test interference.
// Delay the settlement response to simulate loading
- getOptimizedSettlements.mockImplementation(() => new Promise(resolve =>
- setTimeout(() => resolve({ data: { optimizedSettlements: [] } }), 100)
- ));
+ let timeoutId;
+ getOptimizedSettlements.mockImplementation(() => new Promise(resolve => {
+ timeoutId = setTimeout(() => resolve({ data: { optimizedSettlements: [] } }), 100);
+ }));
+
+ // Add cleanup in afterEach
+ afterEach(() => {
+ if (timeoutId) clearTimeout(timeoutId);
+ });Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In frontend/screens/HomeScreen.test.js around lines 296 to 298, the mock
implementation of getOptimizedSettlements uses setTimeout without cleanup, which
can cause test flakiness. Refactor the mock to use jest's fake timers or return
a resolved Promise immediately to avoid asynchronous delays. Alternatively,
ensure the timeout is cleared after the test to prevent interference with other
tests.
frontend/screens/HomeScreen.test.js
Outdated
| // Button should be disabled and show loading | ||
| const createButton = getByText('Create'); | ||
| expect(createButton.props.disabled).toBeTruthy(); | ||
|
|
There was a problem hiding this comment.
Testing disabled state on text element instead of button
The test is checking the disabled prop on a Text element returned by getByText, but buttons don't work this way in React Native.
// Button should be disabled and show loading
- const createButton = getByText('Create');
- expect(createButton.props.disabled).toBeTruthy();
+ // Find the actual button component, not the text
+ const createButton = getByTestId('create-button');
+ expect(createButton.props.disabled).toBeTruthy();Note: This requires adding a testID="create-button" to the actual button component in HomeScreen.
📝 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.
| // Button should be disabled and show loading | |
| const createButton = getByText('Create'); | |
| expect(createButton.props.disabled).toBeTruthy(); | |
| // Button should be disabled and show loading | |
| + // Find the actual button component, not the text | |
| - const createButton = getByText('Create'); | |
| + const createButton = getByTestId('create-button'); | |
| expect(createButton.props.disabled).toBeTruthy(); |
🤖 Prompt for AI Agents
In frontend/screens/HomeScreen.test.js around lines 502 to 505, the test
incorrectly checks the disabled prop on a Text element instead of the actual
Button component. To fix this, add a testID="create-button" prop to the Button
component in HomeScreen, then update the test to use
getByTestId('create-button') to select the button and check its disabled prop
correctly.
| const maliciousObj = { '__proto__': { polluted: 'value' } }; | ||
| expect(safeGet(maliciousObj, '__proto__.polluted')).toBe('value'); | ||
| expect(safeGet({}, 'polluted', 'safe')).toBe('safe'); // Should not be polluted | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Test doesn't verify prototype pollution protection
The test checks that safeGet handles __proto__ keys but doesn't verify that the prototype pollution attempt is actually prevented in the implementation.
Add a more comprehensive test:
it('should handle prototype pollution attempts', () => {
const maliciousObj = { '__proto__': { polluted: 'value' } };
expect(safeGet(maliciousObj, '__proto__.polluted')).toBe('value');
- expect(safeGet({}, 'polluted', 'safe')).toBe('safe'); // Should not be polluted
+
+ // Verify prototype pollution is prevented
+ const testObj = {};
+ const beforeProto = Object.getPrototypeOf(testObj);
+ safeGet(maliciousObj, '__proto__.polluted');
+ const afterProto = Object.getPrototypeOf(testObj);
+ expect(beforeProto).toBe(afterProto);
+ expect(testObj.polluted).toBeUndefined();
});📝 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.
| const maliciousObj = { '__proto__': { polluted: 'value' } }; | |
| expect(safeGet(maliciousObj, '__proto__.polluted')).toBe('value'); | |
| expect(safeGet({}, 'polluted', 'safe')).toBe('safe'); // Should not be polluted | |
| }); | |
| it('should handle prototype pollution attempts', () => { | |
| const maliciousObj = { '__proto__': { polluted: 'value' } }; | |
| expect(safeGet(maliciousObj, '__proto__.polluted')).toBe('value'); | |
| // Verify prototype pollution is prevented | |
| const testObj = {}; | |
| const beforeProto = Object.getPrototypeOf(testObj); | |
| safeGet(maliciousObj, '__proto__.polluted'); | |
| const afterProto = Object.getPrototypeOf(testObj); | |
| expect(beforeProto).toBe(afterProto); | |
| expect(testObj.polluted).toBeUndefined(); | |
| }); |
🤖 Prompt for AI Agents
In frontend/utils/balanceCalculator.test.js around lines 784 to 787, the test
for safeGet checks access to __proto__ keys but does not confirm that prototype
pollution is prevented. Enhance the test by verifying that the prototype of an
object is not polluted after using safeGet with __proto__ keys, ensuring the
implementation properly blocks prototype pollution attempts.
… utils, and screen components" This reverts commit df45073.
3 participants
Retrieve and display friends' balances in the FriendsScreen, improving balance calculation logic. Update HomeScreen header to streamline group creation and joining actions by removing redundant buttons.
Summary by CodeRabbit
New Features
Improvements
Bug Fixes