[jules] enhance: Add swipe-to-delete for expenses with undo #280
Conversation
- Installed `react-native-gesture-handler` and `react-native-reanimated`. - Configured babel and App root for gesture handling. - Created `SwipeableExpenseRow` component with theme support. - Updated `GroupDetailsScreen` to support swipe-to-delete. - Implemented optimistic UI updates and "Undo" snackbar. - Added unmount cleanup to ensure pending deletions are committed. - Updated API to include `deleteExpense`. - Updated tracking files (`todo.md`, `changelog.md`, `knowledge.md`). Co-authored-by: Devasy23 <110348311+Devasy23@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.
|
WalkthroughThis PR implements a swipe-to-delete feature with undo functionality for expenses in the mobile app. It adds the required gesture handler dependencies, wraps the app with Changes
Possibly related PRs
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: 6
🤖 Fix all issues with AI agents
In `@mobile/components/SwipeableExpenseRow.js`:
- Around line 28-31: The onPress handler currently calls onDelete() unguarded
which will throw if onDelete is undefined; update the onPress in the
SwipeableExpenseRow component to first close the swipeable via
swipeableRef.current?.close() and then check that the onDelete prop is a
function (e.g., typeof onDelete === 'function') before invoking it so the
deletion call is skipped safely when the callback is missing.
- Around line 53-61: Remove the redundant backgroundColor from the StyleSheet
definition for rightActionContainer: delete the backgroundColor: '#ff5252' entry
in styles.rightActionContainer so the inline prop-controlled color ({
backgroundColor: deleteColor || "#ff5252" } in SwipeableExpenseRow) is the
single source of truth; update styles.rightActionContainer accordingly and
ensure no other parts reference the removed key.
In `@mobile/screens/GroupDetailsScreen.js`:
- Around line 111-119: The unmount cleanup silently swallows delete errors for
deleteExpense called in the useEffect cleanup (deletedExpenseIdRef, groupId), so
persist the pending deletion ID to AsyncStorage before attempting the async
delete on unmount and implement retry logic on the next mount: on unmount save
deletedExpenseIdRef.current (e.g., under a key like pendingDelete:<groupId>),
attempt deleteExpense in the cleanup and if it fails leave the ID in
AsyncStorage and surface a user-facing notification (toast/alert) about the
failed delete; on component mount check AsyncStorage for
pendingDelete:<groupId>, retry deleteExpense, remove the key on success, and
handle/remove stale IDs appropriately to avoid infinite retries.
- Around line 70-78: handleDeleteExpense currently fire-and-forgets
commitDeletion(deletedExpenseId), causing an earlier delete to remain in-flight
while UI shows a new snackbar; make handleDeleteExpense async and await
commitDeletion(deletedExpenseId) when deletedExpenseId is set (i.e., if
(deletedExpenseId) await commitDeletion(deletedExpenseId)); then proceed to
setDeletedExpenseId(expenseId), update deletedExpenseIdRef.current and show the
snackbar and Haptics; if you intentionally want concurrency instead, add a
concise comment above the commitDeletion call documenting the trade-off so
future maintainers understand the choice.
- Around line 97-109: The onDismissSnackbar callback reads the stale
deletedExpenseId state and can still call commitDeletion after onUndo; change
onDismissSnackbar to read deletedExpenseIdRef.current (the synchronous ref
cleared by onUndo) instead of the deletedExpenseId state, and only call
commitDeletion when deletedExpenseIdRef.current is non-null; keep the existing
setDeletedExpenseId(null) in onUndo but rely on deletedExpenseIdRef.current for
the synchronous check in onDismissSnackbar (reference: onUndo,
onDismissSnackbar, deletedExpenseIdRef, commitDeletion).
- Line 279: The FlatList is re-rendering because data={expenses.filter((e) =>
e._id !== deletedExpenseId)} creates a new array each render; fix by memoizing
the filtered list with useMemo: create a const filteredExpenses = useMemo(() =>
expenses.filter((e) => e._id !== deletedExpenseId), [expenses,
deletedExpenseId]) and pass filteredExpenses to FlatList's data prop instead of
the inline filter; ensure useMemo is imported from React and references the
existing expenses and deletedExpenseId variables.
| onPress={() => { | ||
| swipeableRef.current?.close(); | ||
| onDelete(); | ||
| }} |
There was a problem hiding this comment.
Add a guard for missing onDelete callback.
If onDelete is not provided (or is undefined), pressing the delete button will throw a TypeError. Consider a defensive check.
🛡️ Proposed fix
onPress={() => {
swipeableRef.current?.close();
- onDelete();
+ onDelete?.();
}}📝 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.
| onPress={() => { | |
| swipeableRef.current?.close(); | |
| onDelete(); | |
| }} | |
| onPress={() => { | |
| swipeableRef.current?.close(); | |
| onDelete?.(); | |
| }} |
🤖 Prompt for AI Agents
In `@mobile/components/SwipeableExpenseRow.js` around lines 28 - 31, The onPress
handler currently calls onDelete() unguarded which will throw if onDelete is
undefined; update the onPress in the SwipeableExpenseRow component to first
close the swipeable via swipeableRef.current?.close() and then check that the
onDelete prop is a function (e.g., typeof onDelete === 'function') before
invoking it so the deletion call is skipped safely when the callback is missing.
| const styles = StyleSheet.create({ | ||
| rightActionContainer: { | ||
| width: 80, | ||
| justifyContent: 'center', | ||
| alignItems: 'center', | ||
| backgroundColor: '#ff5252', | ||
| borderTopRightRadius: 12, | ||
| borderBottomRightRadius: 12, | ||
| }, |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Redundant backgroundColor in StyleSheet.
Line 58 sets backgroundColor: '#ff5252' in styles.rightActionContainer, but it's always overridden by the inline style on Line 20 ({ backgroundColor: deleteColor || "#ff5252" }). Remove it from the StyleSheet to avoid confusion.
♻️ Proposed fix
rightActionContainer: {
width: 80,
justifyContent: 'center',
alignItems: 'center',
- backgroundColor: '#ff5252',
borderTopRightRadius: 12,
borderBottomRightRadius: 12,
},📝 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 styles = StyleSheet.create({ | |
| rightActionContainer: { | |
| width: 80, | |
| justifyContent: 'center', | |
| alignItems: 'center', | |
| backgroundColor: '#ff5252', | |
| borderTopRightRadius: 12, | |
| borderBottomRightRadius: 12, | |
| }, | |
| const styles = StyleSheet.create({ | |
| rightActionContainer: { | |
| width: 80, | |
| justifyContent: 'center', | |
| alignItems: 'center', | |
| borderTopRightRadius: 12, | |
| borderBottomRightRadius: 12, | |
| }, |
🤖 Prompt for AI Agents
In `@mobile/components/SwipeableExpenseRow.js` around lines 53 - 61, Remove the
redundant backgroundColor from the StyleSheet definition for
rightActionContainer: delete the backgroundColor: '#ff5252' entry in
styles.rightActionContainer so the inline prop-controlled color ({
backgroundColor: deleteColor || "#ff5252" } in SwipeableExpenseRow) is the
single source of truth; update styles.rightActionContainer accordingly and
ensure no other parts reference the removed key.
| const handleDeleteExpense = (expenseId) => { | ||
| if (deletedExpenseId) { | ||
| commitDeletion(deletedExpenseId); | ||
| } | ||
| setDeletedExpenseId(expenseId); | ||
| deletedExpenseIdRef.current = expenseId; | ||
| setSnackbarVisible(true); | ||
| Haptics.notificationAsync(Haptics.NotificationFeedbackType.Success); | ||
| }; |
There was a problem hiding this comment.
Rapid successive deletes: previous deletion committed without await; errors silently lost to caller.
When a second swipe-delete arrives while one is pending, commitDeletion(deletedExpenseId) is fire-and-forget (no await). commitDeletion has its own try/catch, so this won't crash, but the user sees the snackbar for the new deletion while the old one may still be in-flight. If the old deletion fails, the alert will pop up seemingly out of context.
Consider awaiting or, at minimum, adding a brief comment documenting this intentional design choice so future maintainers understand the trade-off.
🤖 Prompt for AI Agents
In `@mobile/screens/GroupDetailsScreen.js` around lines 70 - 78,
handleDeleteExpense currently fire-and-forgets commitDeletion(deletedExpenseId),
causing an earlier delete to remain in-flight while UI shows a new snackbar;
make handleDeleteExpense async and await commitDeletion(deletedExpenseId) when
deletedExpenseId is set (i.e., if (deletedExpenseId) await
commitDeletion(deletedExpenseId)); then proceed to
setDeletedExpenseId(expenseId), update deletedExpenseIdRef.current and show the
snackbar and Haptics; if you intentionally want concurrency instead, add a
concise comment above the commitDeletion call documenting the trade-off so
future maintainers understand the choice.
| const onUndo = () => { | ||
| setDeletedExpenseId(null); | ||
| deletedExpenseIdRef.current = null; | ||
| setSnackbarVisible(false); | ||
| }; | ||
|
|
||
| const onDismissSnackbar = () => { | ||
| setSnackbarVisible(false); | ||
| if (deletedExpenseId) { | ||
| commitDeletion(deletedExpenseId); | ||
| setDeletedExpenseId(null); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Bug: Undo is ineffective — onDismissSnackbar reads stale closure state after onUndo runs.
When the user presses "Undo", onUndo calls setDeletedExpenseId(null), but the state update hasn't re-rendered yet. The Snackbar then fires onDismissSnackbar, which still sees the old deletedExpenseId from the same render closure and proceeds to call commitDeletion — defeating the undo.
Use deletedExpenseIdRef.current (which is synchronously nulled in onUndo) instead of the state variable:
🐛 Proposed fix
const onDismissSnackbar = () => {
setSnackbarVisible(false);
- if (deletedExpenseId) {
- commitDeletion(deletedExpenseId);
+ if (deletedExpenseIdRef.current) {
+ commitDeletion(deletedExpenseIdRef.current);
setDeletedExpenseId(null);
}
};🤖 Prompt for AI Agents
In `@mobile/screens/GroupDetailsScreen.js` around lines 97 - 109, The
onDismissSnackbar callback reads the stale deletedExpenseId state and can still
call commitDeletion after onUndo; change onDismissSnackbar to read
deletedExpenseIdRef.current (the synchronous ref cleared by onUndo) instead of
the deletedExpenseId state, and only call commitDeletion when
deletedExpenseIdRef.current is non-null; keep the existing
setDeletedExpenseId(null) in onUndo but rely on deletedExpenseIdRef.current for
the synchronous check in onDismissSnackbar (reference: onUndo,
onDismissSnackbar, deletedExpenseIdRef, commitDeletion).
| useEffect(() => { | ||
| return () => { | ||
| if (deletedExpenseIdRef.current) { | ||
| deleteExpense(groupId, deletedExpenseIdRef.current).catch((err) => | ||
| console.error("Unmount delete failed", err) | ||
| ); | ||
| } | ||
| }; | ||
| }, [groupId]); |
There was a problem hiding this comment.
Unmount cleanup silently discards server errors.
The .catch only logs to console. If the delete fails (e.g., network error, 403), the user will never know the expense wasn't actually deleted. Consider persisting the pending deletion ID (e.g., AsyncStorage) so it can be retried on next mount, or at minimum note this as a known limitation.
🤖 Prompt for AI Agents
In `@mobile/screens/GroupDetailsScreen.js` around lines 111 - 119, The unmount
cleanup silently swallows delete errors for deleteExpense called in the
useEffect cleanup (deletedExpenseIdRef, groupId), so persist the pending
deletion ID to AsyncStorage before attempting the async delete on unmount and
implement retry logic on the next mount: on unmount save
deletedExpenseIdRef.current (e.g., under a key like pendingDelete:<groupId>),
attempt deleteExpense in the cleanup and if it fails leave the ID in
AsyncStorage and surface a user-facing notification (toast/alert) about the
failed delete; on component mount check AsyncStorage for
pendingDelete:<groupId>, retry deleteExpense, remove the key on success, and
handle/remove stale IDs appropriately to avoid infinite retries.
| <FlatList | ||
| style={styles.contentContainer} | ||
| data={expenses} | ||
| data={expenses.filter((e) => e._id !== deletedExpenseId)} |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Memoize the filtered expense list to avoid unnecessary FlatList re-renders.
expenses.filter(...) creates a new array reference on every render, forcing FlatList to diff the entire list each time. Wrap it in useMemo:
♻️ Proposed refactor
+import { useContext, useEffect, useMemo, useRef, useState } from "react";
...
- data={expenses.filter((e) => e._id !== deletedExpenseId)}
+ data={filteredExpenses}Add before the return:
const filteredExpenses = useMemo(
() => expenses.filter((e) => e._id !== deletedExpenseId),
[expenses, deletedExpenseId]
);📝 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.
| data={expenses.filter((e) => e._id !== deletedExpenseId)} | |
| import { useContext, useEffect, useMemo, useRef, useState } from "react"; | |
| // ... other imports ... | |
| export default function GroupDetailsScreen() { | |
| // ... other state and hooks ... | |
| const filteredExpenses = useMemo( | |
| () => expenses.filter((e) => e._id !== deletedExpenseId), | |
| [expenses, deletedExpenseId] | |
| ); | |
| return ( | |
| // ... other JSX ... | |
| <FlatList | |
| data={filteredExpenses} | |
| // ... other props ... | |
| /> | |
| // ... rest of component ... | |
| ); | |
| } |
🤖 Prompt for AI Agents
In `@mobile/screens/GroupDetailsScreen.js` at line 279, The FlatList is
re-rendering because data={expenses.filter((e) => e._id !== deletedExpenseId)}
creates a new array each render; fix by memoizing the filtered list with
useMemo: create a const filteredExpenses = useMemo(() => expenses.filter((e) =>
e._id !== deletedExpenseId), [expenses, deletedExpenseId]) and pass
filteredExpenses to FlatList's data prop instead of the inline filter; ensure
useMemo is imported from React and references the existing expenses and
deletedExpenseId variables.
1 participant
Implemented swipe-to-delete functionality for expenses in
GroupDetailsScreen.react-native-gesture-handlerandreact-native-reanimated.SwipeableExpenseRowcomponent.useRefanduseEffectcleanup.PR created automatically by Jules for task 3953291382767595427 started by @Devasy23
Summary by CodeRabbit
Release Notes