Skip to content
This repository has been archived by the owner on Nov 30, 2022. It is now read-only.

Fix crash when muting account #245

Merged
merged 2 commits into from Nov 27, 2022
Merged

Fix crash when muting account #245

merged 2 commits into from Nov 27, 2022

Conversation

vollkorntomate
Copy link
Contributor

@vollkorntomate vollkorntomate commented Nov 25, 2022

Summary

This is a fix/workaround for the crash that happened sometimes after muting a profile.

The root cause seems to be the fact that cached posts are deleted from the database upon muting, which results in lastUpdate containing no items anymore, which leads to an index-out-of-bounds error when trying to access it.

Other Information

Fixes #48.

@alexbbrown
Copy link

alexbbrown commented Nov 25, 2022

I like that this addresses the crash and it is concise.

But I'm worried. I find it odd to check for an out of bounds error in this way, because it suggests that the caller is using an out-of-date indexpath that's older than the value it's indexing. If that's possible in general, it suggests that the indexpath could be often be pointing at elements which have moved, which can lead to other, non crashing UI bugs.

Did you discover why this method is being called with an out of date indexpath, and if that could be addressed and avoided elsewhere.

My worry is that this change basically says: yes app state can be wrong or inconsistent but we shouldn't crash. Personally I have generally taken the opposite approach in my code; invalid or inconsistent app state should cause a crash.

@vollkorntomate
Copy link
Contributor Author

Hey Alex!
I see your point about the app being in an invalid state. There is definitively a bigger problem lying beneath, but I don't know where exactly and it would probably require much larger change. It seems to have something to do with the TableViewController and the way it dequeues reusable cells, since the indexPath is coming from UITableViewDiffableDataSource (in TableViewDataSource.init(..)).
But I have to admit I'm not experienced enough with this, so a deeper dive into the issue is something for the future (or someone else if anyone has an idea and is willing to provide a fix).

@jzzocc
Copy link
Member

jzzocc commented Nov 27, 2022

There's definitely work to be done in making updates from the local DB more deterministic in this situation that I don't have time for right now, but this mitigates a very poor user experience in the moment, thanks.

@jzzocc jzzocc merged commit 8692b8c into metabolist:develop Nov 27, 2022
@vollkorntomate vollkorntomate deleted the fix-48 branch November 28, 2022 13:45
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Crash when muting profile you follow
3 participants