Add group settings functionality and enhance group details screen #128
Conversation
WalkthroughThis update refactors API calls to use a centralized client handling authentication internally, removing explicit token parameters. It adds new group management API functions and a dedicated group settings screen. Navigation and UI components for group details and avatars are enhanced, enabling admins to manage group info, members, and settings with improved avatar rendering and seamless navigation. Backend logic now enforces checks for unsettled balances before allowing member removal or leaving groups, with corresponding tests added. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant HomeScreen
participant GroupDetailsScreen
participant GroupSettingsScreen
participant API
participant BackendService
User->>HomeScreen: Selects a group
HomeScreen->>GroupDetailsScreen: Navigates with groupId
GroupDetailsScreen->>User: Displays group details & member list
User->>GroupDetailsScreen: Taps settings icon
GroupDetailsScreen->>GroupSettingsScreen: Navigates with groupId
GroupSettingsScreen->>API: Fetch group details & members
API-->>GroupSettingsScreen: Returns group and member data
GroupSettingsScreen->>User: Displays group info, members, settings
User->>GroupSettingsScreen: Edits group info / removes member / leaves group
GroupSettingsScreen->>API: Sends update/delete/leave requests
API->>BackendService: Validates requests, checks unsettled balances
BackendService-->>API: Returns success or error (e.g., unsettled balances)
API-->>GroupSettingsScreen: Returns success/failure
GroupSettingsScreen->>User: Updates UI and shows confirmation or error
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 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). (1)
✨ 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 #128 +/- ##
==========================================
+ Coverage 71.86% 72.35% +0.48%
==========================================
Files 17 17
Lines 1667 1682 +15
Branches 153 155 +2
==========================================
+ Hits 1198 1217 +19
+ Misses 416 411 -5
- Partials 53 54 +1
🚀 New features to boost your workflow:
|
|
🚀 Expo preview is ready!
|
- Updated import statements to use consistent formatting across GroupDetailsScreen, GroupSettingsScreen, HomeScreen, and JoinGroupScreen. - Changed string literals to use double quotes for consistency. - Simplified API calls by removing unnecessary token parameters in group-related API functions. - Enhanced error handling and user feedback with clearer alert messages. - Improved layout and styling for better user experience in various components. - Refactored settlement status calculations and rendering logic for clarity.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
frontend/screens/GroupDetailsScreen.js (1)
230-241: Style block duplicates unsupportedgapSame concern as above – swap
gapfor explicit spacing.frontend/screens/GroupSettingsScreen.js (1)
207-213:gapnot portable across RN versionsSame styling issue – replace with flexbox spacing utilities for reliability.
🧹 Nitpick comments (3)
frontend/api/groups.js (1)
94-141: Reduce header duplication & validateupdatespayloadEvery new helper repeats the same
Authorizationheader block. Extract a smallauthConfig(token)helper (or reuse the centralapiClientwith an interceptor) to DRY this file and cut maintenance overhead.
Also make sure theupdatesobject passed toupdateGroupis whitelisted on the server-side; for safety you may want to pre-filter unexpected keys on the client as well.frontend/screens/GroupDetailsScreen.js (1)
41-47: Header callback will re-create on every render
navigation.setOptions({...})is insideuseEffectthat depends on[token, groupId], but the callback you pass (headerRight) is re-created each render, forcing React Navigation to re-compute options. Wrap the callback inuseCallbackor move the wholesetOptionsintouseLayoutEffectwith an empty dependency array to avoid unnecessary work.frontend/screens/HomeScreen.js (1)
142-153: Avoid passing full image URL asgroupIconparamWhen
isImageis true you pass the URL string asgroupIcon. Down-stream screens expecting an emoji/letter might attempt to render that inAvatar.Text. Pass a dedicatediconLabelparam (letter/emoji) and a separateimageUrlto keep semantics clear.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
frontend/api/groups.js(1 hunks)frontend/navigation/GroupsStackNavigator.js(2 hunks)frontend/screens/GroupDetailsScreen.js(4 hunks)frontend/screens/GroupSettingsScreen.js(1 hunks)frontend/screens/HomeScreen.js(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
frontend/navigation/GroupsStackNavigator.js (1)
frontend/screens/GroupSettingsScreen.js (1)
GroupSettingsScreen(16-258)
frontend/api/groups.js (1)
frontend/api/auth.js (2)
apiClient(5-10)token(20-26)
⏰ 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). (2)
- GitHub Check: bundle-analysis
- GitHub Check: preview
🔇 Additional comments (2)
frontend/navigation/GroupsStackNavigator.js (1)
3-5: Route addition looks goodImporting the screen and registering the
"GroupSettings"route is straightforward and matches the rest of the stack. No issues spotted.Also applies to: 17-17
frontend/screens/GroupSettingsScreen.js (1)
26-30: Missing dependency:user?._idwon’t update
useMemodepends onuser?._idbut the optional-chain expression is evaluated immediately, so React will treat it as a constant value. Depend onuser(oruser && user._id) instead to keep admin status in sync if context changes.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
frontend/screens/JoinGroupScreen.js (1)
8-10: Drop unusedtokenfrom context destructuring
const { token } = useContext(AuthContext);is now dead code and will trigger ESLint‘sno-unused-vars. Remove the destructuring (or extract only the values you actually use) to keep the component clean.frontend/context/AuthContext.js (1)
45-53: Register the token-update listener only onceBecause the effect depends on
tokenandrefresh, a fresh listener is assigned on every token change, replacing the previous one mid-stream. UseuseCallback+ empty-deps (or move listener assignment outside a hook) so the reference is stable and you avoid missing rapid consecutive refresh events.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
frontend/api/auth.js(1 hunks)frontend/api/client.js(1 hunks)frontend/api/groups.js(1 hunks)frontend/context/AuthContext.js(4 hunks)frontend/screens/AddExpenseScreen.js(6 hunks)frontend/screens/FriendsScreen.js(4 hunks)frontend/screens/GroupDetailsScreen.js(9 hunks)frontend/screens/GroupSettingsScreen.js(1 hunks)frontend/screens/HomeScreen.js(9 hunks)frontend/screens/JoinGroupScreen.js(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- frontend/screens/GroupSettingsScreen.js
- frontend/screens/AddExpenseScreen.js
🚧 Files skipped from review as they are similar to previous changes (3)
- frontend/screens/HomeScreen.js
- frontend/screens/GroupDetailsScreen.js
- frontend/api/groups.js
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-07-26T09:41:01.332Z
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/auth/**/*.py : JWT authentication with refresh token rotation must be implemented in the backend.
Applied to files:
frontend/context/AuthContext.js
🧬 Code Graph Analysis (3)
frontend/screens/JoinGroupScreen.js (2)
frontend/context/AuthContext.js (2)
AuthContext(10-10)AuthContext(10-10)frontend/api/groups.js (2)
joinGroup(23-24)joinGroup(23-24)
frontend/context/AuthContext.js (3)
frontend/api/auth.js (2)
refresh(15-17)refresh(15-17)frontend/api/client.js (8)
setAuthTokens(15-18)setAuthTokens(15-18)setTokenUpdateListener(11-13)setTokenUpdateListener(11-13)accessToken(5-5)refreshToken(6-6)clearAuthTokens(20-23)clearAuthTokens(20-23)backend/app/auth/routes.py (1)
refresh_token(172-212)
frontend/api/client.js (1)
backend/app/auth/routes.py (1)
refresh_token(172-212)
🪛 Biome (2.1.2)
frontend/api/client.js
[error] 66-66: Unsafe usage of 'return'.
'return' in 'finally' overwrites the control flow statements inside 'try' and 'catch'.
(lint/correctness/noUnsafeFinally)
🔇 Additional comments (1)
frontend/screens/FriendsScreen.js (1)
26-39: Looks good – API call correctly updated to new client
No issues spotted in the fetch/transform block; logic matches the newgetFriendsBalancesignature.
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
backend/app/groups/service.py (1)
75-78: Inconsistent member user image key: 'avatar' vs 'imageUrl'Elsewhere you use
imageUrl, but here the fallback usesavatar. This breaks consumer expectations and UI.Apply this diff:
- "avatar": None, + "imageUrl": None,
🧹 Nitpick comments (2)
backend/tests/groups/test_groups_service.py (2)
191-219: Good coverage for removal-blocked path; flatten nested context managers (SIM117)Use a single with-statement for multiple contexts.
Apply this diff:
- with patch.object(self.service, "get_db", return_value=mock_db): - with pytest.raises(HTTPException) as exc: + with patch.object(self.service, "get_db", return_value=mock_db), \ + pytest.raises(HTTPException) as exc: await self.service.remove_member(group_id, member_id, admin_id)
250-277: Solid leave-blocked test; also flatten nested context managers (SIM117)Same nit as above; combine contexts.
Apply this diff:
- with patch.object(self.service, "get_db", return_value=mock_db): - with pytest.raises(HTTPException) as exc: + with patch.object(self.service, "get_db", return_value=mock_db), \ + pytest.raises(HTTPException) as exc: await self.service.leave_group(group_id, user_id)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
backend/app/groups/service.py(2 hunks)backend/tests/groups/test_groups_service.py(1 hunks)frontend/screens/GroupSettingsScreen.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/screens/GroupSettingsScreen.js
🧰 Additional context used
🪛 Ruff (0.12.2)
backend/tests/groups/test_groups_service.py
213-214: Use a single with statement with multiple contexts instead of nested with statements
(SIM117)
271-272: Use a single with statement with multiple contexts instead of nested with statements
(SIM117)
🔇 Additional comments (2)
backend/tests/groups/test_groups_service.py (2)
220-249: LGTM: happy-path remove_member when settled is covered wellTest logic and mocks look correct; asserts validate the intended success path.
278-306: LGTM: happy-path leave when settled is coveredMocks and assertions reflect the intended behavior.
✅ Actions performedComments resolved. |
3 similar comments
✅ Actions performedComments resolved. |
✅ Actions performedComments resolved. |
✅ Actions performedComments resolved. |
…te group icon handling
… update error handling
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
backend/app/groups/service.py (1)
447-469: Same unsettled-check duplicated – centralise logicReplicate fix from leave_group: use
raise … from eand move the existence query into a shared helper to cut duplication and keep behaviour consistent.
🧹 Nitpick comments (5)
frontend/screens/EditProfileScreen.js (3)
14-14: Trim user input before validation and submissionAvoid accepting whitespace-only names and send a normalized value to the API.
- if (!name) { + if (!name?.trim()) { Alert.alert("Error", "Name cannot be empty."); return; } @@ - const response = await updateUser({ name }); + const response = await updateUser({ name: name.trim() });Also applies to: 19-19
24-25: Surface server error messages and improve logging contextBubble up API-provided messages when available and log the meaningful detail.
- console.error("Failed to update profile:", error); - Alert.alert("Error", "Failed to update profile."); + console.error( + "Failed to update profile:", + error?.response?.data ?? error?.message ?? error + ); + Alert.alert( + "Error", + error?.response?.data?.message ?? error?.message ?? "Failed to update profile." + );
34-35: Prevent navigation while submitting to avoid state updates on unmounted componentDisable the back action during submission to reduce UI churn and potential setState-on-unmounted warnings.
- <Appbar.BackAction onPress={() => navigation.goBack()} /> + <Appbar.BackAction + onPress={() => { + if (!isSubmitting) navigation.goBack(); + }} + disabled={isSubmitting} + />backend/app/groups/service.py (1)
315-337: Preserve traceback & DRY the unsettled-checkCatch only
Exceptionbut re-raise without context. Useraise HTTPException(... ) from eto keep the original stack (Ruff B904).
Also, this exact block is duplicated inremove_member; extract into a private helper_has_pending_settlementsto avoid copy-paste.backend/tests/groups/test_groups_service.py (1)
216-219: Flatten nestedwithblocks for readabilityCombine patching & exception assertions in a single
withto silence Ruff SIM117 and reduce indentation:with patch.object(self.service, "get_db", return_value=mock_db), \ pytest.raises(HTTPException) as exc: await self.service.remove_member(group_id, member_id, admin_id)Also applies to: 274-277
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
backend/app/groups/service.py(2 hunks)backend/tests/groups/test_groups_service.py(3 hunks)frontend/api/auth.js(1 hunks)frontend/api/client.js(1 hunks)frontend/app.json(1 hunks)frontend/package.json(1 hunks)frontend/screens/EditProfileScreen.js(1 hunks)frontend/screens/GroupDetailsScreen.js(6 hunks)frontend/screens/GroupSettingsScreen.js(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- frontend/package.json
🚧 Files skipped from review as they are similar to previous changes (3)
- frontend/api/auth.js
- frontend/screens/GroupDetailsScreen.js
- frontend/api/client.js
🧰 Additional context used
📓 Path-based instructions (3)
backend/**/*.py
📄 CodeRabbit Inference Engine (.github/copilot-instructions.md)
Backend code must be implemented using FastAPI with Python 3.9+ and organized under the /backend/ directory.
Files:
backend/app/groups/service.pybackend/tests/groups/test_groups_service.py
backend/app/{auth,user,groups,expenses}/**/*.py
📄 CodeRabbit Inference Engine (.github/copilot-instructions.md)
backend/app/{auth,user,groups,expenses}/**/*.py: Backend services should be modular, with authentication and user registration in app/auth/, user profile management in app/user/, group management in app/groups/, and expense tracking in app/expenses/.
When adding a new API endpoint, add the route to the appropriate service router file in the backend.
Files:
backend/app/groups/service.py
backend/tests/**/*.py
📄 CodeRabbit Inference Engine (.github/copilot-instructions.md)
Backend tests must be placed in the /backend/tests/ directory and run using pytest.
Files:
backend/tests/groups/test_groups_service.py
🧠 Learnings (2)
📓 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 ui-poc/pages/Groups.py : API calls from the frontend should use retry logic as demonstrated in Groups.py.
📚 Learning: 2025-07-26T09:41:01.332Z
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.
Applied to files:
backend/app/groups/service.py
🧬 Code Graph Analysis (1)
backend/tests/groups/test_groups_service.py (4)
backend/tests/conftest.py (1)
mock_db(65-96)backend/app/groups/service.py (2)
remove_member(409-474)leave_group(286-342)backend/app/groups/routes.py (1)
leave_group(100-107)backend/tests/groups/test_groups_routes.py (1)
test_leave_group(234-247)
🪛 Ruff (0.12.2)
backend/app/groups/service.py
329-332: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
461-464: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
backend/tests/groups/test_groups_service.py
216-217: Use a single with statement with multiple contexts instead of nested with statements
(SIM117)
274-275: Use a single with statement with multiple contexts instead of nested with statements
(SIM117)
🪛 GitHub Check: codecov/patch
backend/app/groups/service.py
[warning] 325-325: backend/app/groups/service.py#L325
Added line #L325 was not covered by tests
[warning] 457-457: backend/app/groups/service.py#L457
Added line #L457 was not covered by tests
⏰ 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). (2)
- GitHub Check: bundle-analysis
- GitHub Check: preview
🔇 Additional comments (3)
frontend/screens/EditProfileScreen.js (3)
19-19: Tokenless update via centralized client looks goodCalling updateUser without a token aligns with the centralized apiClient approach. Cleaner and reduces prop/threading of auth state.
9-9: Good defaulting of editable stateInitializing from user?.name with a safe fallback is appropriate.
19-19: Verified: updateUser is wired to apiClient with retry logic
TheupdateUsercall in EditProfileScreen uses the sharedapiClient(frontend/api/auth.js:11), andapiClientis configured in frontend/api/client.js with:
- A request interceptor to inject auth headers (line 34)
- A response interceptor that automatically refreshes expired tokens and retries the original request (lines 72–89)
No further action needed.
…n account and friends screens
…nsistent handling
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
frontend/screens/AccountScreen.js (1)
24-28: Extract repeated avatar-selection logic into a shared utilityThis regex-based branching now exists in several screens (Account, Friends, Home, Edit Profile). Centralising it (e.g.
getAvatarSource(imageUrl, fallbackLabel)) eliminates copy-paste, keeps behaviour consistent, and makes future fixes (e.g. more URI schemes) one-liner updates.frontend/screens/FriendsScreen.js (1)
95-105: Regex check may still yieldhasImagebutimageUri === nullIf the string starts with an unsupported scheme (e.g.
file://),hasImageis truthy yet noimageUriis produced, but the UI falls back onAvatar.Textonly whenimageUriis falsy. Consider merging the two checks or simply derivingimageUrifirst and branching on that variable.frontend/screens/EditProfileScreen.js (1)
9-12: Remove unusedtokenvariable
tokenis extracted from context but never used, leading to lint noise.frontend/context/AuthContext.js (1)
52-58: Clean up the token-update listener inuseEffect
setTokenUpdateListeneris invoked on every render wheretokenorrefreshchanges, potentially stacking multiple listeners if the helper doesn’t replace the previous one. Return a cleanup function:useEffect(() => { const unsub = setTokenUpdateListener(({ accessToken, refreshToken }) => { if (accessToken && accessToken !== token) setToken(accessToken); if (refreshToken && refreshToken !== refresh) setRefresh(refreshToken); }); return () => unsub?.(); }, [token, refresh]);Prevents memory leaks and duplicate state updates.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
frontend/api/auth.js(1 hunks)frontend/context/AuthContext.js(4 hunks)frontend/screens/AccountScreen.js(3 hunks)frontend/screens/EditProfileScreen.js(3 hunks)frontend/screens/FriendsScreen.js(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/api/auth.js
🧰 Additional context used
🧠 Learnings (2)
📓 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 ui-poc/pages/Groups.py : API calls from the frontend should use retry logic as demonstrated in Groups.py.
📚 Learning: 2025-07-26T09:41:01.332Z
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/auth/**/*.py : JWT authentication with refresh token rotation must be implemented in the backend.
Applied to files:
frontend/context/AuthContext.js
⏰ 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: preview
…r improved UI experience
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
frontend/screens/FriendsScreen.js (3)
19-27: Fetch both APIs in parallel to cut wait time
getFriendsBalance()andgetGroups()are awaited one after the other, adding unnecessary latency. Kick them off together:-const friendsResponse = await getFriendsBalance(); -const friendsData = friendsResponse.data.friendsBalance || []; -const groupsResponse = await getGroups(); -const groups = groupsResponse?.data?.groups || []; +const [friendsResponse, groupsResponse] = await Promise.all([ + getFriendsBalance(), + getGroups(), +]); +const friendsData = friendsResponse.data.friendsBalance || []; +const groups = groupsResponse?.data?.groups || [];Small win, especially on mobile networks.
62-71: Factor out image-URI normalisationThe identical base64/URL detection logic appears twice. Extract to a helper (e.g.,
normalizeImageUri(src)) to avoid duplication and keep the component lean.Also applies to: 100-107
15-53: Guard against state updates after unmountIf the user navigates away before the fetch resolves,
setIsLoading/setFriendswill run on an unmounted component. Add anisActiveflag orAbortControllercleanup inside the effect’s return to prevent noisy warnings.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/app/expenses/service.py(1 hunks)frontend/screens/FriendsScreen.js(4 hunks)
✅ Files skipped from review due to trivial changes (1)
- backend/app/expenses/service.py
🧰 Additional context used
🧠 Learnings (2)
📓 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 ui-poc/pages/Groups.py : API calls from the frontend should use retry logic as demonstrated in Groups.py.
📚 Learning: 2025-07-26T09:41:01.332Z
Learnt from: CR
PR: Devasy23/splitwiser#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-07-26T09:41:01.332Z
Learning: Applies to ui-poc/pages/Groups.py : API calls from the frontend should use retry logic as demonstrated in Groups.py.
Applied to files:
frontend/screens/FriendsScreen.js
⏰ 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: bundle-analysis
… formatted currency
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
frontend/screens/FriendsScreen.js (1)
36-47: Nice defensive fallback on breakdown arrayUsing (friend.breakdown || []) prevents crashes. Good fix.
🧹 Nitpick comments (6)
frontend/utils/currency.js (1)
4-7: Optional: Use Intl.NumberFormat and make currency/locale configurableImproves thousands separators, negative formatting, and future-proofs for multi-currency users while keeping current API intact.
Add an Intl-based formatter alongside current function:
+export const formatCurrencyIntl = ( + amount, + { currency = "INR", locale = "en-IN", minimumFractionDigits = 2, maximumFractionDigits = 2 } = {} +) => { + const num = Number(amount); + if (!Number.isFinite(num)) return new Intl.NumberFormat(locale, { style: "currency", currency, minimumFractionDigits, maximumFractionDigits }).format(0); + return new Intl.NumberFormat(locale, { style: "currency", currency, minimumFractionDigits, maximumFractionDigits }).format(num); +};And export it:
export default { getCurrencySymbol, formatCurrency, + formatCurrencyIntl, };Also applies to: 8-13
frontend/screens/FriendsScreen.js (5)
105-111: Handle zero group balances consistently ("Settled up" + gray)Align group-level display with friend-level: show "Settled up" and gray color when balance is 0.
- const groupBalanceColor = group.balance < 0 ? "red" : "green"; - const groupBalanceText = - group.balance < 0 - ? `You owe ${formatCurrency(Math.abs(group.balance))}` - : `Owes you ${formatCurrency(group.balance)}`; + const isZero = group.balance === 0; + const groupBalanceColor = isZero ? "gray" : group.balance < 0 ? "red" : "green"; + const groupBalanceText = isZero + ? "Settled up" + : group.balance < 0 + ? `You owe ${formatCurrency(Math.abs(group.balance))}` + : `Owes you ${formatCurrency(group.balance)}`;
70-82: Deduplicate image URI detection with a small helperSame logic is implemented twice for friend and group avatars. Extract it to a helper to reduce bugs and keep behavior consistent.
Replace the blocks with:
- let imageUri = null; - if (hasImage) { - // If it's a raw base64 string without prefix, add a default MIME prefix - if ( - /^data:image/.test(item.imageUrl) || - /^https?:\/\//.test(item.imageUrl) - ) { - imageUri = item.imageUrl; - } else if (/^[A-Za-z0-9+/=]+$/.test(item.imageUrl.substring(0, 50))) { - imageUri = `data:image/jpeg;base64,${item.imageUrl}`; - } - } + const imageUri = toImageUri(item.imageUrl);- let groupImageUri = null; - if (group.imageUrl) { - if ( - /^data:image/.test(group.imageUrl) || - /^https?:\/\//.test(group.imageUrl) - ) { - groupImageUri = group.imageUrl; - } else if ( - /^[A-Za-z0-9+/=]+$/.test(group.imageUrl.substring(0, 50)) - ) { - groupImageUri = `data:image/jpeg;base64,${group.imageUrl}`; - } - } + const groupImageUri = toImageUri(group.imageUrl);Add this helper (outside the selected ranges, e.g., above renderFriend):
const toImageUri = (src) => { if (!src) return null; if (/^data:image/.test(src) || /^https?:\/\//.test(src)) return src; const head = src.slice(0, 50); if (/^[A-Za-z0-9+/=]+$/.test(head)) return `data:image/jpeg;base64,${src}`; return null; };Also applies to: 110-123
64-68: Prefer theme colors over raw "red"/"green" for accessibility and themingUse react-native-paper theme colors so it adapts to light/dark themes and improves contrast.
Example:
-import { - Appbar, - Avatar, - Divider, - IconButton, - List, - Text, -} from "react-native-paper"; +import { + Appbar, + Avatar, + Divider, + IconButton, + List, + Text, + useTheme, +} from "react-native-paper";const FriendsScreen = () => { + const { colors } = useTheme(); ... - const balanceColor = item.netBalance < 0 ? "red" : "green"; + const balanceColor = item.netBalance < 0 ? colors.error : colors.primary;Apply similar for groupBalanceColor.
233-233: Ensure FlatList keys are stringsMinor nit: coerce id to string to avoid warnings when ids are numeric.
- keyExtractor={(item) => item.id} + keyExtractor={(item) => String(item.id)}
247-251: Remove unused style (loaderContainer) or repurposeloaderContainer seems unused after skeleton adoption; dead code.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
frontend/screens/AccountScreen.js(3 hunks)frontend/screens/FriendsScreen.js(4 hunks)frontend/screens/HomeScreen.js(9 hunks)frontend/utils/currency.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/screens/AccountScreen.js
- frontend/screens/HomeScreen.js
🧰 Additional context used
🧠 Learnings (2)
📓 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 ui-poc/pages/Groups.py : API calls from the frontend should use retry logic as demonstrated in Groups.py.
📚 Learning: 2025-07-26T09:41:01.332Z
Learnt from: CR
PR: Devasy23/splitwiser#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-07-26T09:41:01.332Z
Learning: Applies to ui-poc/pages/Groups.py : API calls from the frontend should use retry logic as demonstrated in Groups.py.
Applied to files:
frontend/screens/FriendsScreen.js
⏰ 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: preview
🔇 Additional comments (2)
frontend/screens/FriendsScreen.js (2)
27-34: Fetch friends and groups truly in parallel (AI summary says concurrent; code is sequential)Do both requests concurrently to reduce total wait time.
- // Fetch friends balance + groups concurrently for group icons - const friendsResponse = await getFriendsBalance(); - const friendsData = friendsResponse.data.friendsBalance || []; - const groupsResponse = await getGroups(); - const groups = groupsResponse?.data?.groups || []; + // Fetch friends balance + groups concurrently for group icons + const [friendsResponse, groupsResponse] = await Promise.all([ + getFriendsBalance(), + getGroups(), + ]); + const friendsData = friendsResponse?.data?.friendsBalance || []; + const groups = groupsResponse?.data?.groups || [];Likely an incorrect or invalid review comment.
193-206: Shimmer skeleton is well-implementedSmooth looped animation with proper cleanup; good UX while fetching.
…prove image MIME type handling in profile updates
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
frontend/screens/EditProfileScreen.js (1)
24-31: Resolved: Use actual MIME type for data URL (matches prior bot suggestion).Good fix deriving MIME dynamically and falling back safely. This addresses the earlier comment about hardcoded "image/jpeg".
🧹 Nitpick comments (6)
frontend/screens/EditProfileScreen.js (6)
11-11: Update state shape comment to reflect mimeType.The comment is stale. It should include mimeType to match the actual state shape.
- const [pickedImage, setPickedImage] = useState(null); // { uri, base64 } + const [pickedImage, setPickedImage] = useState(null); // { uri, base64, mimeType }
15-17: Trim name before validating and sending to API.Prevents whitespace-only names and keeps payload normalized.
- const handleUpdateProfile = async () => { - if (!name) { + const handleUpdateProfile = async () => { + const trimmedName = name.trim(); + if (!trimmedName) { Alert.alert("Error", "Name cannot be empty."); return; } setIsSubmitting(true); try { - const updates = { name }; + const updates = { name: trimmedName };Also applies to: 21-21
45-55: Handle iOS “limited” photo access and use granted boolean.Expo returns a granted boolean and may set accessPrivileges to "limited" on iOS. Treat both as allowed.
- const { status } = await ImagePicker.requestMediaLibraryPermissionsAsync(); - if (status !== "granted") { + const { status, granted, accessPrivileges } = + await ImagePicker.requestMediaLibraryPermissionsAsync(); + const allowed = granted || status === "granted" || accessPrivileges === "limited"; + if (!allowed) { Alert.alert( "Permission required", "We need media library permission to select an image." ); return; }
62-80: Improve MIME inference for HEIC/HEIF and a few common formats.Many iOS photos are HEIC/HEIF. Add those mappings to avoid mislabeling as JPEG.
- let mimeType = asset.mimeType || asset.type; // expo sometimes supplies type like 'image' + let mimeType = asset.mimeType || asset.type; // expo sometimes supplies type like 'image' if (mimeType && !/image\//.test(mimeType)) { // if it's just 'image', normalize - if (mimeType === 'image') mimeType = 'image/jpeg'; + if (mimeType === "image") mimeType = "image/jpeg"; } if (!mimeType || !/image\//.test(mimeType)) { // Attempt to infer from file extension as a lightweight fallback const ext = (asset.uri || "").split(".").pop()?.toLowerCase(); - if (ext === "png") mimeType = "image/png"; + if (ext === "png") mimeType = "image/png"; + else if (ext === "heic") mimeType = "image/heic"; + else if (ext === "heif") mimeType = "image/heif"; else if (ext === "webp") mimeType = "image/webp"; else if (ext === "gif") mimeType = "image/gif"; else if (ext === "jpg" || ext === "jpeg") mimeType = "image/jpeg"; else mimeType = "image/jpeg"; // safe default }
45-81: Avoid duplication: extract a shared image-picking helper.This pickImage logic overlaps with GroupSettingsScreen (frontend/screens/GroupSettingsScreen.js lines 121-143). Consider a small utility (e.g., frontend/utils/imagePicking.js) that:
- Requests permission (handling “limited”).
- Launches picker with consistent options.
- Normalizes/infer mimeType as done here.
- Returns { uri, base64, mimeType }.
Reduces drift and centralizes future fixes (e.g., HEIC support).
56-61: Optional: downscale before base64 to avoid large payloads.High-res images can bloat requests and hit 413s. Consider resizing to a sensible bound (e.g., 512–1024 px) with expo-image-manipulator.
Add import:
+import * as ImageManipulator from "expo-image-manipulator";Inside pickImage, after selecting asset:
// Example: bound longest side to 768, keep aspect ratio const bound = 768; const { width = bound, height = bound } = asset; const scale = Math.min(bound / Math.max(width, height), 1); const target = scale < 1 ? { width: Math.round(width * scale), height: Math.round(height * scale) } : undefined; let processed = asset; if (target) { const manipulated = await ImageManipulator.manipulateAsync( asset.uri, [{ resize: target }], { compress: 0.8, base64: true } // keep original format; RN will honor mime when constructing data URL later ); processed = { ...asset, uri: manipulated.uri, base64: manipulated.base64 }; }Then use processed.uri and processed.base64 for setPickedImage and MIME inference.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
frontend/api/client.js(1 hunks)frontend/screens/EditProfileScreen.js(3 hunks)frontend/utils/currency.js(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- frontend/utils/currency.js
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/api/client.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 ui-poc/pages/Groups.py : API calls from the frontend should use retry logic as demonstrated in Groups.py.
🧬 Code Graph Analysis (1)
frontend/screens/EditProfileScreen.js (3)
frontend/screens/GroupSettingsScreen.js (1)
pickImage(122-144)frontend/context/AuthContext.js (5)
AuthContext(10-10)AuthContext(10-10)user(13-13)updateUserInContext(165-173)updatedUser(103-105)frontend/api/auth.js (2)
updateUser(11-11)updateUser(11-11)
⏰ 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: preview
🔇 Additional comments (1)
frontend/screens/EditProfileScreen.js (1)
33-35: No additional retry logic needed for updateUserThe
updateUsercall infrontend/api/auth.jsuses the sharedapiClientfromfrontend/api/client.js, which already includes:
- Automatic token refresh on 401 responses
- Exponential-backoff retries (up to 3 attempts) for network errors and 5xx responses
All update calls therefore benefit from the standardized retry and interceptor behavior.
✅ Actions performedComments resolved. |
3 similar comments
✅ Actions performedComments resolved. |
✅ Actions performedComments resolved. |
✅ Actions performedComments resolved. |
…ances and exception handling
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
backend/tests/groups/test_groups_service.py (7)
191-222: Combine nested contexts (SIM117) and assert no writes on blocked removalFlatten the nested with blocks and assert DB write is not attempted when blocked. Also assert the settlements lookup happened.
- with patch.object(self.service, "get_db", return_value=mock_db): - with pytest.raises(HTTPException) as exc: - await self.service.remove_member(group_id, member_id, admin_id) + with patch.object(self.service, "get_db", return_value=mock_db), pytest.raises(HTTPException) as exc: + await self.service.remove_member(group_id, member_id, admin_id) assert exc.value.status_code == 400 assert "unsettled balances" in str(exc.value.detail) + settlements.find_one.assert_called_once() + groups.update_one.assert_not_called()
223-252: Stabilize mocks and strengthen success assertions
- Use return_value instead of a single-item side_effect to avoid StopIteration if the service calls find_one more than once.
- Assert the expected DB interactions on the happy path.
- groups.find_one.side_effect = [ - { - "_id": ObjectId(group_id), - "members": [ - {"userId": admin_id, "role": "admin"}, - {"userId": member_id, "role": "member"}, - ], - } - ] + groups.find_one.return_value = { + "_id": ObjectId(group_id), + "members": [ + {"userId": admin_id, "role": "admin"}, + {"userId": member_id, "role": "member"}, + ], + } settlements.find_one.return_value = None # No pending settlements groups.update_one.return_value = MagicMock(modified_count=1) with patch.object(self.service, "get_db", return_value=mock_db): ok = await self.service.remove_member(group_id, member_id, admin_id) assert ok is True + settlements.find_one.assert_called_once() + groups.update_one.assert_called_once()
253-280: Combine nested contexts (SIM117) and assert no writes on blocked leaveFlatten the nested with blocks and assert no write occurs when leave is blocked. Also verify settlement lookup.
- with patch.object(self.service, "get_db", return_value=mock_db): - with pytest.raises(HTTPException) as exc: - await self.service.leave_group(group_id, user_id) + with patch.object(self.service, "get_db", return_value=mock_db), pytest.raises(HTTPException) as exc: + await self.service.leave_group(group_id, user_id) assert exc.value.status_code == 400 assert "unsettled balances" in str(exc.value.detail) + settlements.find_one.assert_called_once() + groups.update_one.assert_not_called()
281-309: Strengthen success-path assertionsOn allowed leave, assert lookups and writes occur exactly once.
with patch.object(self.service, "get_db", return_value=mock_db): ok = await self.service.leave_group(group_id, user_id) assert ok is True + settlements.find_one.assert_called_once() + groups.update_one.assert_called_once()
528-566: Minor: verify settlements lookup performed in this happy-path testThis improves intent coverage with minimal cost.
with patch.object(self.service, "get_db", return_value=mock_db): result = await self.service.leave_group( "642f1e4a9b3c2d1f6a1b2c3d", "user456" ) assert result is True + mock_settlements.find_one.assert_called_once() + mock_collection.update_one.assert_called_once()
603-673: Apply SIM117 and assert no writes on blocked/exceptional leave paths
- Flatten nested contexts per Ruff (SIM117).
- Ensure no write occurs when leave is blocked or when lookup fails.
- with patch.object(self.service, "get_db", return_value=mock_db): - with pytest.raises(HTTPException) as exc: - await self.service.leave_group(str(group["_id"]), "member1") + with patch.object(self.service, "get_db", return_value=mock_db), pytest.raises(HTTPException) as exc: + await self.service.leave_group(str(group["_id"]), "member1") assert exc.value.status_code == 400 assert "Cannot leave group with unsettled balances" in exc.value.detail + settlements.find_one.assert_called_once() + groups.update_one.assert_not_called()- with patch.object(self.service, "get_db", return_value=mock_db): - with pytest.raises(HTTPException) as exc: - await self.service.leave_group(str(group["_id"]), "member1") + with patch.object(self.service, "get_db", return_value=mock_db), pytest.raises(HTTPException) as exc: + await self.service.leave_group(str(group["_id"]), "member1") assert exc.value.status_code == 503 assert "Unable to verify unsettled balances" in exc.value.detail + settlements.find_one.assert_called_once() + groups.update_one.assert_not_called()
675-743: Apply SIM117 and assert no writes on blocked/exceptional removal paths
- Flatten nested contexts per Ruff (SIM117).
- Ensure no write occurs when removal is blocked or lookup fails.
- with patch.object(self.service, "get_db", return_value=mock_db): - with pytest.raises(HTTPException) as exc: - await self.service.remove_member(str(group["_id"]), "member1", "admin1") + with patch.object(self.service, "get_db", return_value=mock_db), pytest.raises(HTTPException) as exc: + await self.service.remove_member(str(group["_id"]), "member1", "admin1") assert exc.value.status_code == 400 assert "Cannot remove member with unsettled balances" in exc.value.detail + settlements.find_one.assert_called_once() + groups.update_one.assert_not_called()- with patch.object(self.service, "get_db", return_value=mock_db): - with pytest.raises(HTTPException) as exc: - await self.service.remove_member(str(group["_id"]), "member1", "admin1") + with patch.object(self.service, "get_db", return_value=mock_db), pytest.raises(HTTPException) as exc: + await self.service.remove_member(str(group["_id"]), "member1", "admin1") assert exc.value.status_code == 503 assert "Unable to verify unsettled balances" in exc.value.detail + settlements.find_one.assert_called_once() + groups.update_one.assert_not_called()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/tests/groups/test_groups_service.py(4 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
backend/**/*.py
📄 CodeRabbit Inference Engine (.github/copilot-instructions.md)
Backend code must be implemented using FastAPI with Python 3.9+ and organized under the /backend/ directory.
Files:
backend/tests/groups/test_groups_service.py
backend/tests/**/*.py
📄 CodeRabbit Inference Engine (.github/copilot-instructions.md)
Backend tests must be placed in the /backend/tests/ directory and run using pytest.
Files:
backend/tests/groups/test_groups_service.py
🧠 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 ui-poc/pages/Groups.py : API calls from the frontend should use retry logic as demonstrated in Groups.py.
🪛 Ruff (0.12.2)
backend/tests/groups/test_groups_service.py
216-217: Use a single with statement with multiple contexts instead of nested with statements
(SIM117)
274-275: Use a single with statement with multiple contexts instead of nested with statements
(SIM117)
632-633: Use a single with statement with multiple contexts instead of nested with statements
(SIM117)
667-668: Use a single with statement with multiple contexts instead of nested with statements
(SIM117)
702-703: Use a single with statement with multiple contexts instead of nested with statements
(SIM117)
737-738: Use a single with statement with multiple contexts instead of nested with statements
(SIM117)
⏰ 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: bundle-analysis
Introduce group settings features, including new APIs for managing groups and a dedicated settings screen. Enhance the group details screen with improved member display and navigation to the settings.
Summary by CodeRabbit
New Features
Improvements
Authentication
Bug Fixes
Chores