Mobile: Add Pull-to-Refresh with Haptics #268
Conversation
Co-authored-by: Devasy <110348311+Devasy@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
✅ Deploy Preview for split-but-wiser canceled.
|
WalkthroughImplements mobile pull-to-refresh functionality with haptic feedback across HomeScreen, FriendsScreen, and GroupDetailsScreen. Refactors data fetching with optional loading indicators to enable silent updates. Adds expo-haptics dependency and updates project documentation and tests to reflect completed work. Changes
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@mobile/screens/GroupDetailsScreen.js`:
- Around line 57-62: The onRefresh handler can get stuck if Haptics.impactAsync
rejects; update the onRefresh function so you call setIsRefreshing(true) then
attempt Haptics.impactAsync but suppress any haptics errors (use .catch or
try/catch) and ensure fetchData(false) is always awaited and
setIsRefreshing(false) is executed in a finally block; reference the onRefresh
function, Haptics.impactAsync, fetchData, and setIsRefreshing when making this
change.
| const onRefresh = async () => { | ||
| setIsRefreshing(true); | ||
| await Haptics.impactAsync(Haptics.ImpactFeedbackStyle.Light); | ||
| await fetchData(false); | ||
| setIsRefreshing(false); | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
wc -l mobile/screens/GroupDetailsScreen.jsRepository: Devasy/splitwiser
Length of output: 101
🏁 Script executed:
sed -n '50,70p' mobile/screens/GroupDetailsScreen.jsRepository: Devasy/splitwiser
Length of output: 661
🏁 Script executed:
sed -n '1,30p' mobile/screens/GroupDetailsScreen.jsRepository: Devasy/splitwiser
Length of output: 1025
🏁 Script executed:
rg -n "onRefresh" mobile/screens/GroupDetailsScreen.jsRepository: Devasy/splitwiser
Length of output: 135
🏁 Script executed:
rg -n "Haptics" mobile/screens/ -A 2 -B 2Repository: Devasy/splitwiser
Length of output: 2227
🏁 Script executed:
sed -n '40,55p' mobile/screens/GroupDetailsScreen.jsRepository: Devasy/splitwiser
Length of output: 671
🏁 Script executed:
web_search
expo-haptics impactAsync error handling rejection behaviorRepository: Devasy/splitwiser
Length of output: 162
🌐 Web query:
expo-haptics impactAsync can it reject throw error
💡 Result:
Yes. expo-haptics’s Haptics.impactAsync() returns a Promise<void> and it can reject (throw) in at least these cases:
- API unavailable on the current platform/runtime: the JS wrapper explicitly checks for the native method and throws
UnavailabilityErrorif it doesn’t exist, which rejects the promise. (In current source it checksExpoHaptics.impactAsyncand throws if missing.) [2] - Native call fails: after the availability check it
awaits the native module call, so any native-side error would also reject the promise. [2]
The official docs only state that it “fulfills once native haptics functionality is triggered” and don’t enumerate errors, but it is still a Promise and (per the implementation) can reject as above. [1][2]
Practical usage: wrap it in try/catch if you need to be safe on all platforms/builds. [1][2]
Sources: Expo docs for impactAsync() [1], expo-haptics implementation (Haptics.ts) [2].
Harden refresh flow against haptics failures.
Haptics.impactAsync can reject if haptics is unavailable on the platform or if the native call fails. If it rejects, fetchData is never called and isRefreshing remains true, leaving the UI in a stuck state. Wrap in try/finally and use .catch() to suppress haptics errors so the refresh flow always completes.
✅ Suggested fix
const onRefresh = async () => {
setIsRefreshing(true);
- await Haptics.impactAsync(Haptics.ImpactFeedbackStyle.Light);
- await fetchData(false);
- setIsRefreshing(false);
+ try {
+ await Haptics.impactAsync(Haptics.ImpactFeedbackStyle.Light).catch(() => {});
+ await fetchData(false);
+ } finally {
+ setIsRefreshing(false);
+ }
};🤖 Prompt for AI Agents
In `@mobile/screens/GroupDetailsScreen.js` around lines 57 - 62, The onRefresh
handler can get stuck if Haptics.impactAsync rejects; update the onRefresh
function so you call setIsRefreshing(true) then attempt Haptics.impactAsync but
suppress any haptics errors (use .catch or try/catch) and ensure
fetchData(false) is always awaited and setIsRefreshing(false) is executed in a
finally block; reference the onRefresh function, Haptics.impactAsync, fetchData,
and setIsRefreshing when making this change.
Co-authored-by: Devasy <110348311+Devasy@users.noreply.github.com>
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/tests/expenses/test_expense_service.py (1)
743-751: Fix assertion to correctly access the pipeline stage.This is the cause of the pipeline failure.
call_args_listitems "are two-tuples of (positional args, keyword args)", socall_args_list[0][0]is the tuple of positional arguments, andcall_args_list[0][0][0]is the actual pipeline list. The current assertion checks if"$match"is in the pipeline list (which contains dicts), not in the first pipeline stage dict.🐛 Proposed fix
# The FIRST aggregate call is the filtered one aggregate_call_args = mock_db.expenses.aggregate.call_args_list[0][0] - assert "$match" in aggregate_call_args[0] - match_query = aggregate_call_args[0]["$match"] + pipeline = aggregate_call_args[0] + assert "$match" in pipeline[0] + match_query = pipeline[0]["$match"] assert "createdAt" in match_query assert match_query["createdAt"]["$gte"] == from_date assert match_query["createdAt"]["$lte"] == to_date assert "tags" in match_query assert match_query["tags"]["$in"] == tags
🤖 Fix all issues with AI agents
In `@backend/tests/expenses/test_expense_service.py`:
- Around line 1984-1985: Replace the broad pytest.raises(Exception) in the test
around expense_service.get_friends_balance_summary(user_id_str) with a specific
assertion: either use pytest.raises with the exact exception class the service
raises (e.g., HTTPException) and assert expected attributes (like status_code)
on exc_info.value, or keep pytest.raises but add a match= regex to validate the
error message; update the test to reference
expense_service.get_friends_balance_summary and assert the specific error
characteristics rather than catching all Exceptions.
| with pytest.raises(Exception): # The service doesn't catch all exceptions | ||
| await expense_service.get_friends_balance_summary(user_id_str) |
There was a problem hiding this comment.
Use a more specific exception or add a match parameter.
Using pytest.raises(Exception) is too broad and can mask unrelated errors. Either catch the specific exception type the service raises or add a match parameter to narrow the scope.
🛡️ Proposed fix
- with pytest.raises(Exception): # The service doesn't catch all exceptions
+ with pytest.raises(Exception, match="Calculation failed"):
await expense_service.get_friends_balance_summary(user_id_str)Or better, if the service wraps this in a specific exception:
with pytest.raises(HTTPException) as exc_info:
await expense_service.get_friends_balance_summary(user_id_str)
assert exc_info.value.status_code == 500📝 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.
| with pytest.raises(Exception): # The service doesn't catch all exceptions | |
| await expense_service.get_friends_balance_summary(user_id_str) | |
| with pytest.raises(Exception, match="Calculation failed"): | |
| await expense_service.get_friends_balance_summary(user_id_str) |
🧰 Tools
🪛 Ruff (0.14.14)
1984-1984: Do not assert blind exception: Exception
(B017)
1984-1984: pytest.raises(Exception) is too broad, set the match parameter or use a more specific exception
(PT011)
🤖 Prompt for AI Agents
In `@backend/tests/expenses/test_expense_service.py` around lines 1984 - 1985,
Replace the broad pytest.raises(Exception) in the test around
expense_service.get_friends_balance_summary(user_id_str) with a specific
assertion: either use pytest.raises with the exact exception class the service
raises (e.g., HTTPException) and assert expected attributes (like status_code)
on exc_info.value, or keep pytest.raises but add a match= regex to validate the
error message; update the test to reference
expense_service.get_friends_balance_summary and assert the specific error
characteristics rather than catching all Exceptions.
Co-authored-by: Devasy <110348311+Devasy@users.noreply.github.com>
for more information, see https://pre-commit.ci
vrajpatelll
deleted the
jules-mobile-pull-to-refresh-14011509928752649065
branch
Implemented pull-to-refresh functionality with haptic feedback on HomeScreen, GroupDetailsScreen, and FriendsScreen.
expo-hapticsdependency.RefreshControlwith theme-aware styling.isLoadingfromisRefreshing).PR created automatically by Jules for task 14011509928752649065 started by @Devasy23
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.