Refactor FXIOS-12796 [Swift 6 Migration] Notification Center addObserver closure fixes #28062
Conversation
… for rust code change requirement.
… then use assumeIsolated for main actor isolated code.
…trivial types Sendable to fix some sendable warnings while I'm at it.
ih-codes
force-pushed
the
ih/FXIOS-12796-addObserver-fixes-1
branch
from
0384b04 to
db989f4
Compare
…separate PR to tackle.
Client.app: Coverage: 35.23
ShareTo.appex: Coverage: 27.28
libStorage.a: Coverage: 54.2
Generated by 🚫 Danger Swift against ec0703f |
| ) { notification in | ||
| if let newState = notification.userInfo?["newState"] as? ConstellationState { | ||
| guard Thread.isMainThread else { | ||
| assertionFailure("This must be called main thread") |
There was a problem hiding this comment.
Maybe I should add a fatal log here too for Sentry?
There was a problem hiding this comment.
And maybe fallback implementation. I can't decide. This is so messy!
There was a problem hiding this comment.
We can have a fatal log, but if we're very sure this is main thread only, I don't think fallback is necessary? We can monitor the beta closely for fatal logs in the next release, and we'll get quickly warned if any of those fatal logs is getting spammed/called
| // private func observe(_ notification: Notification.Name, | ||
| // with closure: @escaping ((Notification) -> Void)) -> NSObjectProtocol? { | ||
| // return NotificationCenter.default.addObserver(forName: notification, object: nil, queue: .main, using: closure) | ||
| // } |
There was a problem hiding this comment.
Do we still need this if it's commented?
| forName: Notification.Name.constellationStateUpdate, | ||
| object: nil, | ||
| queue: .main | ||
| ) { [weak self ] _ in |
There was a problem hiding this comment.
nit
| ) { [weak self ] _ in | |
| ) { [weak self] _ in |
| ) { notification in | ||
| if let newState = notification.userInfo?["newState"] as? ConstellationState { | ||
| guard Thread.isMainThread else { | ||
| assertionFailure("This must be called main thread") |
There was a problem hiding this comment.
We can have a fatal log, but if we're very sure this is main thread only, I don't think fallback is necessary? We can monitor the beta closely for fatal logs in the next release, and we'll get quickly warned if any of those fatal logs is getting spammed/called
📜 Tickets
Jira ticket
Github issue
💡 Description
We can be safer about our
.mainqueue closure execution blocks and make sure we're in a main actor isolated context, fixing warnings.Example warning:
@Cramsden, I just repeated the same pattern we discussed elsewhere on a previous PR.
Note: Because
Notificationcan never beSendable, I opted to remove the utility method in SensitiveViewController.There are still a few warnings related to non-Sendable types in these closures that should be tackled in a separate PR (some of the changes were starting to spiral so I backed off).
cc @Cramsden @lmarceau
📝 Checklist
@Mergifyio backport release/v120)