chore: handle goals saving errors#628
Conversation
revu-bot
left a comment
There was a problem hiding this comment.
Summary
This PR adds comprehensive error handling for goals saving operations, distinguishing between local storage errors and API synchronization failures. The implementation provides appropriate user feedback for different failure scenarios.
Key Changes
- ✅ Added try-catch blocks to handle errors during goal saving
- ✅ Separated local storage operations from API synchronization
- ✅ Implemented user-friendly error messages with Alert dialogs
⚠️ 3 Important Issues Found requiring attention
Issues by Severity
| Severity | Count | Category |
|---|---|---|
| IMPORTANT | 2 | Error handling logic |
| MINOR | 1 | Type safety |
The error handling strategy is sound, but there are issues with the API error detection logic and potential race conditions that need to be addressed.
| // Gestion de la synchronisation API séparément | ||
| let apiError = false; | ||
| try { | ||
| const apiResult = await updateApiReminer(goal); | ||
| apiError = !apiResult || apiResult.ok === false || apiResult.error; | ||
| } catch (apiErr) { |
There was a problem hiding this comment.
[IMPORTANT] Incorrect API error detection logic
Why it matters:
- The condition
!apiResultwill be true whenapiResult = { ok: true }, incorrectly flagging success as an error - This causes false positives where successful API calls trigger the "not synchronized" warning
- Users will see unnecessary warning messages even when everything works correctly
Root cause:
The logic checks !apiResult which is falsy for valid success objects. The function updateApiReminer always returns an object, never null/undefined.
Proposed fix:
| // Gestion de la synchronisation API séparément | |
| let apiError = false; | |
| try { | |
| const apiResult = await updateApiReminer(goal); | |
| apiError = !apiResult || apiResult.ok === false || apiResult.error; | |
| } catch (apiErr) { | |
| let apiError = false; | |
| try { | |
| const apiResult = await updateApiReminer(goal); | |
| apiError = apiResult?.ok === false || !!apiResult?.error; | |
| } catch (apiErr) { |
This correctly identifies errors only when ok is explicitly false or when an error property exists.
| // Gestion de la synchronisation API séparément | |
| let apiError = false; | |
| try { | |
| const apiResult = await updateApiReminer(goal); | |
| apiError = !apiResult || apiResult.ok === false || apiResult.error; | |
| } catch (apiErr) { | |
| // Gestion de la synchronisation API séparément | |
| let apiError = false; | |
| try { | |
| const apiResult = await updateApiReminer(goal); | |
| apiError = apiResult?.ok === false || !!apiResult?.error; | |
| } catch (apiErr) { |
| // Feedback utilisateur approprié | ||
| if (hasApiError) { | ||
| Alert.alert( | ||
| "Objectifs sauvegardés", | ||
| "Vos objectifs ont été sauvegardés localement, mais les rappels n'ont pas pu être synchronisés. Ils seront synchronisés automatiquement lors de votre prochaine connexion.", | ||
| [{ text: "OK", onPress: () => navigation.goBack() }] | ||
| ); | ||
| } else { | ||
| navigation.goBack(); | ||
| } |
There was a problem hiding this comment.
[IMPORTANT] Race condition with navigation after Alert dismissal
Why it matters:
- The Alert's
onPresscallback navigates back, but the code also callsnavigation.goBack()immediately after for the success case - If the user dismisses the alert quickly, there could be a double navigation attempt
- The loading state is reset in
finally, but navigation happens asynchronously via Alert callback
Proposed fix:
Move the navigation call outside the Alert to ensure consistent behavior:
| // Feedback utilisateur approprié | |
| if (hasApiError) { | |
| Alert.alert( | |
| "Objectifs sauvegardés", | |
| "Vos objectifs ont été sauvegardés localement, mais les rappels n'ont pas pu être synchronisés. Ils seront synchronisés automatiquement lors de votre prochaine connexion.", | |
| [{ text: "OK", onPress: () => navigation.goBack() }] | |
| ); | |
| } else { | |
| navigation.goBack(); | |
| } | |
| // Feedback utilisateur approprié | |
| if (hasApiError) { | |
| Alert.alert( | |
| "Objectifs sauvegardés", | |
| "Vos objectifs ont été sauvegardés localement, mais les rappels n'ont pas pu être synchronisés. Ils seront synchronisés automatiquement lors de votre prochaine connexion.", | |
| [{ text: "OK" }] | |
| ); | |
| } | |
| // Navigate back after showing feedback | |
| navigation.goBack(); |
This ensures navigation happens consistently regardless of the alert, and the alert is just informational.
| // Feedback utilisateur approprié | |
| if (hasApiError) { | |
| Alert.alert( | |
| "Objectifs sauvegardés", | |
| "Vos objectifs ont été sauvegardés localement, mais les rappels n'ont pas pu être synchronisés. Ils seront synchronisés automatiquement lors de votre prochaine connexion.", | |
| [{ text: "OK", onPress: () => navigation.goBack() }] | |
| ); | |
| } else { | |
| navigation.goBack(); | |
| } | |
| // Feedback utilisateur approprié | |
| if (hasApiError) { | |
| Alert.alert( | |
| "Objectifs sauvegardés", | |
| "Vos objectifs ont été sauvegardés localement, mais les rappels n'ont pas pu être synchronisés. Ils seront synchronisés automatiquement lors de votre prochaine connexion.", | |
| [{ text: "OK" }] | |
| ); | |
| } | |
| // Navigate back after showing feedback | |
| navigation.goBack(); |
Co-authored-by: Revu <dnum.fabrique@sg.social.gouv.fr>
|
|
🎉 This PR is included in version 1.126.2 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |



No description provided.