Conversation
| state = .fetched(.byNumber(total: newTotalNumber)) | ||
| tableNode.deleteRows(at: [IndexPath(row: index, section: 0)], with: .left) | ||
| do { | ||
| try ObjcException.catch { |
There was a problem hiding this comment.
Can you please explain how is this fix related to the crash fix?
There was a problem hiding this comment.
It looks like there is an issue related to the index of the row that we try to update
There was a problem hiding this comment.
As @tomholub said, we want to avoid having crash here. Even if issue is still here, we should not crash.
There was a problem hiding this comment.
Can we have an issue to solve the root cause of the crash?
There was a problem hiding this comment.
absolutely. Let's solve it in this PR as well.
There was a problem hiding this comment.
Since all UI is async with Texture, I guess that using message index is unsafe. We should move to message ID, as this is the only way to know that message we want to delete is the same message.
There was a problem hiding this comment.
Would suggest to investigate root cause of the issue.
Also if there was only one message before moving to trash we need to update whole state of the controller.
| tableNode.deleteRows(at: [IndexPath(row: index, section: 0)], with: .left) | ||
| do { | ||
| try ObjcException.catch { | ||
| self.tableNode.deleteRows(at: [IndexPath(row: index, section: 0)], with: .left) |
There was a problem hiding this comment.
Data source for table node should be updated with correct amount of inboxInput then reload should be performed.
There was a problem hiding this comment.
Data source is updated above:
inboxInput.remove(at: index)
We could reload table node with reloadData(), but crash means that we are in invalid state. We could move from index to some kind of message ID.
But I guess that this PR is more about handling Objc exceptions in Swift.
There was a problem hiding this comment.
While originally it was indeed primaryli about handling of ObjC exceptions, that was in part because we didn't know any better.
If the actual root cause of this exception can be fixed, that is even better and more appropriate. Both should be done. We didn't know that, so we just focused on handling, but Anton has experience with Texture specifically to notice that this is something we should be able to fix directly as well.
There was a problem hiding this comment.
I think it's something more specific than just updating data source, as this crash happens very rarely.
Implementing ObjC exception handling should help us to find root cause of this issue.
May be switching from index to message id (as @ivan-ushakov proposed) will fix this crash, but we can't be 100% sure as currently we're not able to reproduce this issue.
There was a problem hiding this comment.
If the goal is to collect more information about the crash, I suppose we should have it show a modal similar to other crash scenarios? Then some user may take a screenshot and send it to us for debugging.
As currently, it will just be logged then forgotten.
sosnovsky
left a comment
sosnovsky
left a comment
There was a problem hiding this comment.
I think it's a good solution for now - app won't crash if this issue occurs again and users will get alert with error message.
|
@Kharchevskyi are you agree with current approach? We will try to track down the cause of the crash with that alerts and also prevent unhandled exception to crash app. |
|
@ivan-ushakov sure, no objections 👍 |
4 participants
close #1076
Tests (delete all except exactly one):
To be filled by reviewers
I have reviewed that this PR... (tick whichever items you personally focused on during this review):