Refactor FXIOS-13944 [Swift 6] Isolate new state and subsequently a lot of redux to the main thread #30301
Conversation
Cramsden
requested review from
OrlaM,
ih-codes,
lmarceau,
mattreaganmozilla and
yoanarios
💪 Quality guardian5 tests files modified. You're a champion of test coverage! 🚀 🥇 Perfect PR sizeSmaller PRs are easier to review. Thanks for making life easy for reviewers! ✨ 💬 Description craftsmanGreat PR description! Reviewers salute you 🫡 ❌ Per-file test coverage gateThe following changed file(s) are below 35.0% coverage:
Client.app: Coverage: 37.06
Generated by 🚫 Danger Swift against ce303de |
There was a problem hiding this comment.
Seems reasonable to me at first glance. I honestly expected way more issues/changes, haha.
I commented a few spots where I think the State reducer files could avoid isolation if they used the passed-in state instead of direct store access. Seems odd we would bypass the reducer state input to access store directly anyway.
|
|
||
| weak var coordinatorDelegate: ContextMenuCoordinator? | ||
|
|
||
| @MainActor |
There was a problem hiding this comment.
ContextMenuState feels a bit weird that it contains all these actions for dispatching to the store, probably some responsibilities that should eventually be separated out. Unrelated to this work though.
|
|
||
| // MARK: - Navigation Toolbar Actions | ||
| @MainActor | ||
| private static func navigationActions( |
There was a problem hiding this comment.
Would it be better for us to instead pass in the current state and not need to access this on the main actor because of this line right below? Then I don't think we'd need any @MainActors in this file?
guard let toolbarState = store.state.screenState(ToolbarState.self, for: .toolbar, window: action.windowUUID)
There was a problem hiding this comment.
I looked closer at this, and the problem is that this reducer also needs the parent ToolbarState to calculate the next state, which feels a bit weird to me. But that's why it accesses the store. 🤔
What are everyone's thoughts on a reducer needing to access the global store to get a higher level state in order to make decisions? In other words, one reducer can't render the next state with just its previous state + an action. The reducer also needs to access the global state, so the next state is really a function of the previous state + global current state + an action.
In this specific example, the reducer for NavigationBarState (a child property of ToolbarState) needs to access the ToolbarState through the global store to help decide whether the user can navigate forward/backward with the nav buttons, etc..
firefox-ios/Client/Frontend/Browser/Toolbars/Redux/AddressBarState.swift
Show resolved
Hide resolved
| /// Stores your entire app state in the form of a single data structure. | ||
| /// This state can only be modified by dispatching Actions to the store. | ||
| /// Whenever the state of the store changes, the store will notify all store subscriber. | ||
| public final class Store<State: StateType & Sendable>: DefaultDispatchStore { |
There was a problem hiding this comment.
Should we just annotate the entire store @MainActor if we're doing so in AppState.swift anyway? 🤔
|
This pull request has conflicts when rebasing. Could you fix it @Cramsden? 🙏 |
| } | ||
|
|
||
| @objc | ||
| private nonisolated func didTapReload() { |
There was a problem hiding this comment.
This is marked nonisolated but it's called from a UIButton tap. In other places we did not mark those UI button @objc selector methods as nonisolated, and we call directly store.dispatch. It's assumed since the click is from a button and it's in the UI, then it's always the main thread.
We can totally change that, but I just want us to have consistency.
There was a problem hiding this comment.
I personally would vote to always mark @objc methods nonisolated... I think there's less room for programmer error then, like if they decide to call such a method from a notification later as well or something. You'd have to check every caller to know if the usage is "safe" to assume main actor isolation. 👀
...ent/Frontend/Settings/HomepageSettings/WallpaperSettings/v1/WallpaperSettingsViewModel.swift
Outdated
Show resolved
Hide resolved
ih-codes
force-pushed
the
cr/FXIOS-13944_isolate-new-state-to-main-actor
branch
from
f958b6e to
a0e36e3
Compare
|
I've rebased and fixed the conflicts for you! I think I'll also try to address mine and @lmarceau's PR feedback on your branch if that's ok (just revert my commit(s) if you disagree with the changes). But I thought I'd help out to get this one over the finish line since you're out sick @Cramsden and it's a big one I'd like to see merged in soon. |
|
Ok, I made 2 small changes for some of the PR feedback in a commit. I resolved those conversations and left the rest open for you @Cramsden! |
📜 Tickets
Jira ticket
💡 Description
MainActorMainActorSubscriptionWrapperto theMainActorMainActordispatchLegacythat were not already isolated to theMainActorand move them todispatch🎥 Demos
Demo
📝 Checklist