ruddarr / app

A native iOS companion app for Radarr and Sonarr instances.
https://apps.apple.com/app/ruddarr/id6476240130
MIT License
117 stars 6 forks source link

Fix `MediaCalendar` crash by marking it `@MainActor` #341

Closed pteasima closed 1 month ago

pteasima commented 1 month ago

There's not much to test here, it still builds with this one line change and in theory should help.

Again, I recommend we do these things throughout the codebase as one larger refactoring task.

pteasima commented 1 month ago

@tillkruss Im getting linted 🤦‍♂️😅

pteasima commented 1 month ago

Does it make sense to make flag most models as @MainActor?

I think so. Here's my iMessage from today on this very topic: "This one probably has quite an easy fix [...], but it also reveals a larger issue where maybe we just never synchronized anything we might wanna just mark anything we touch from views as @MainActor. Im making you aware since this would be a larger refactoring effort after which we may want to reassess and see whether many crashes are gone."

So all models and also most or all view methods (afaik we can't quite mark the whole views as mainactor, because hasn't properly some things like @Environment so it won't let you. But most view-related things should indeed be @MainActor. I never fully got into the habit of doing this because most my codebases use a lot of @State and very few @Observable, but in Ruddarr I'd do it as much as possible. If in some case its not possible, at least it surfaces a potential issue.

pteasima commented 1 month ago

But I should probably make it clear that some of the logic is ok to run off the main thread. So in theory its just the properties and not all the methods that need to be @MainActor. But i'd recommend to just play it safe, mark the type @MainActor and solve any performance issues that arise by nonisolated. I expect very few performance issues. Most of the expensive stuff like networking, json decoding probably happens in other async methods outside of these models and those are still free to run on any thread.

tillkruss commented 1 month ago

So all the models do HTTP requests, as long as we can mark the model as main actor without the HTTP requests blocking the entire UI we're good.