Conversation
There was a problem hiding this comment.
Code Review
This pull request fixes an issue with updating app details and refactors the getAppDetails method in AppProvider. The UI change in update_app.dart correctly removes a fixed-height SizedBox that could cause layout overflows. The logic change in app_provider.dart improves the behavior of fetching app details. I've identified a potential crash scenario due to unhandled exceptions during data parsing from the server response and suggested a fix to make the application more robust.
| if (appData != null) { | ||
| var freshApp = App.fromJson(appData); | ||
| var oldApp = apps.where((element) => element.id == id).firstOrNull; | ||
| if (oldApp == null) { | ||
| return null; | ||
| if (oldApp != null) { | ||
| var idx = apps.indexOf(oldApp); | ||
| apps[idx] = freshApp; | ||
| notifyListeners(); | ||
| } | ||
| var idx = apps.indexOf(oldApp); | ||
| apps[idx] = App.fromJson(app); | ||
| notifyListeners(); | ||
| return apps[idx]; | ||
| return freshApp; | ||
| } |
There was a problem hiding this comment.
The App.fromJson constructor could throw an exception if the appData from the server is malformed (e.g., missing required fields). This would cause an unhandled exception and potentially crash the app. It's safer to wrap the parsing and state update logic in a try-catch block to handle potential data parsing errors gracefully.
Additionally, I've optimized the logic to find and update the app in the local list by using indexWhere instead of where followed by indexOf, which is more efficient as it avoids a second iteration over the list.
| if (appData != null) { | |
| var freshApp = App.fromJson(appData); | |
| var oldApp = apps.where((element) => element.id == id).firstOrNull; | |
| if (oldApp == null) { | |
| return null; | |
| if (oldApp != null) { | |
| var idx = apps.indexOf(oldApp); | |
| apps[idx] = freshApp; | |
| notifyListeners(); | |
| } | |
| var idx = apps.indexOf(oldApp); | |
| apps[idx] = App.fromJson(app); | |
| notifyListeners(); | |
| return apps[idx]; | |
| return freshApp; | |
| } | |
| if (appData != null) { | |
| try { | |
| var freshApp = App.fromJson(appData); | |
| final idx = apps.indexWhere((element) => element.id == id); | |
| if (idx != -1) { | |
| apps[idx] = freshApp; | |
| notifyListeners(); | |
| } | |
| return freshApp; | |
| } catch (e) { | |
| Logger.debug('Failed to parse app details for id: $id, error: $e'); | |
| return null; | |
| } | |
| } |
No description provided.