Remove long-unused Iterable deep links handling #24487
Conversation
Generated by 🚫 Danger |
|
| App Name | WordPress | |
| Configuration | Release-Alpha | |
| Build Number | 27446 | |
| Version | PR #24487 | |
| Bundle ID | org.wordpress.alpha | |
| Commit | 6f001c4 | |
| Installation URL | 57a9ihl8o0q4g |
|
| App Name | Jetpack | |
| Configuration | Release-Alpha | |
| Build Number | 27446 | |
| Version | PR #24487 | |
| Bundle ID | com.jetpack.alpha | |
| Commit | 6f001c4 | |
| Installation URL | 297t1rs49nrao |
| self.selectedIndex = WPTab.notifications.rawValue | ||
| completion?(self.notificationsViewController!) | ||
| // UITabBarController.selectedIndex must be used from main thread only. | ||
| DispatchQueue.main.async { [weak self] in | ||
| guard let self else { return } | ||
|
|
||
| self.selectedIndex = WPTab.notifications.rawValue | ||
| completion?(self.notificationsViewController!) | ||
| } |
There was a problem hiding this comment.
Noticed this when running Jetpack in the Simulator and opening the app simulating a user deep link via xcrun simctl openurl booted https://wordpress.com/notifications
I considered moving the main thread dispatch earlier on, e.g. in the deep link handling
diff --git a/WordPress/Classes/System/WordPressAppDelegate.swift b/WordPress/Classes/System/WordPressAppDelegate.swift
index 8167daf7b1..6b9a57642a 100644
--- a/WordPress/Classes/System/WordPressAppDelegate.swift
+++ b/WordPress/Classes/System/WordPressAppDelegate.swift
@@ -457,7 +457,9 @@ extension WordPressAppDelegate {
}
trackDeepLink(for: url) { url in
- UniversalLinkRouter.shared.handle(url: url)
+ DispatchQueue.main.async {
+ UniversalLinkRouter.shared.handle(url: url)
+ }
}
}but I thought it best to be precise and keep the main thread dispatching as close to the code requiring it as possible.
However, forcing the deep link handling to run on the main thread might be a good blanket approach if we fear that other deep link might implicitly require the main thread.
There was a problem hiding this comment.
I'd suggest adding DispatchQueue.main.async in trackDeepLink as you originally wanted and adding wpAssert(Thread.isMainThread) in showNotificationsTab. In the future, we'll try and use @MainActor as much as we can.
Adding an async in showNotificationsTab is too risky because it has many callers, and some of them might rely on it executing synchronously.
mokagio
force-pushed
the
mokagio/links-update
branch
from
292bb06 to
6f001c4
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR removes legacy deep link handling code related to Iterable and ensures that notification tab transitions occur on the main thread.
- Removed the unused Iterable deep link check and related methods.
- Updated notifications tab presentation to guarantee UI updates occur on the main thread.
Reviewed Changes
Copilot reviewed 2 out of 8 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| WordPress/Classes/System/WordPressAppDelegate.swift | Removed Iterable-specific code and simplified deep link tracking logic. |
| WordPress/Classes/System/Root View/WPTabBarController+RootViewPresenter.swift | Ensured notifications tab selection is executed on the main thread with DispatchQueue.main. |
Files not reviewed (6)
- Sources/Reader/Reader.entitlements: Language not supported
- WordPress/Jetpack/JetpackDebug.entitlements: Language not supported
- WordPress/Jetpack/JetpackRelease-Alpha.entitlements: Language not supported
- WordPress/Jetpack/JetpackRelease.entitlements: Language not supported
- WordPress/WordPress-Alpha.entitlements: Language not supported
- WordPress/WordPress.entitlements: Language not supported
Comments suppressed due to low confidence (1)
WordPress/Classes/System/WordPressAppDelegate.swift:477
- The URLSession data task does not call the completion handler if response?.url is nil, which could lead to unexpected behavior. Consider adding a fallback call to the completion handler (or an error callback) when the response URL is unavailable.
guard let url = response?.url else { return }
|
| guard let url = response?.url else { | ||
| return | ||
| } | ||
|
|
||
| completion(url) |
There was a problem hiding this comment.
Copilot observed:
The URLSession data task does not call the completion handler if response?.url is nil, which could lead to unexpected behavior. Consider adding a fallback call to the completion handler (or an error callback) when the response URL is unavailable.
That's a good point actually. I don't think it's in scope for this PR, but we might want to log a ticket to look into it. Unless... how can we get to the point where a deep link has a response without URL? Is this a legacy from the Objective-C NSURLResponse implementation?
There was a problem hiding this comment.
If you never expect it to happen, let's add wpAssertionFailure.
See pbMoDN-fd-p2#comment-7311 , with thanks to @crazytonyli for noticing this.
I'm still waiting for confirmation from the marketing folks that the unused assumption is correct, but I'm fairly confident it is, so in the meantime I went ahead and opened this quick PR.
For reference, Iterable support was introduced in #14212
This PR relates to https://linear.app/a8c/issue/CMM-260/configure-reader-target
I tested this using the same deep link URL suggested in #14212 from Jetpack.
Notice two warnings in the console that we might want to look into later:
<UINavigationController: 0x3222d8c00>from detached view controller<WordPress.NotificationsViewController: 0x32383ea00>is not supported, and may result in incorrect safe area insets and a corrupt root presentation. Make sure<WordPress.NotificationsViewController: 0x32383ea00>is in the view controller hierarchy before presenting from it. Will become a hard exception in a future release.$s9WordPress0aB11AppDelegateC29applicationDidEnterBackgroundyySo13UIApplicationCFTo"), was created over 30 seconds ago. In applications running in the background, this creates a risk of termination. Remember to callUIApplication.endBackgroundTask(_:)for your task in a timely manner to avoid this.