Dismiss presented vcs when subscribe route is detected

[danielebogo]

📘 Part of: General Bug

Fixes PCIOS-337

This PR fixes the presentation issue when a subscribe URL is tapped from within a webview in our Safari VC, displayed from an episode detail or Full screen player.

Our JLRoutes was trying to present the loading above an already presented view controller, as all the hierarchy wasn't correctly dismissed.

To test

• Run this branch
• Visit a NYT podcast like The Daily
• Open an episode detail and scroll the description till the end, when you see _You can also subscribe to your favorite....`
• Tap the URL next to it (ping me if you don't find it)
• Sign in with the credential shared in the issue
• When you see the composition with all podcasts cover tap the + to expand the podcasts app list (see screenshot below)

• Select Pocket Casts
• Select one of the podcast in the list and tap the + blue button
• Confirm Safari and the episode detail are dismissed and the loading appears
• Confirm after the loading is completed, the selected podcast detail is pushed
• Do the same test from other entry points, also using the deep link I shared in the last message in the issue (ping me if you don't find it)

Checklist

• I have considered if this change warrants user-facing release notes and have added them to CHANGELOG.md if necessary.
• I have considered adding unit tests for my changes.
• I have updated (or requested that someone edit) the spreadsheet to reflect any new or changed analytics.

[dangermattic]

	1 Warning
⚠️	This PR is assigned to the milestone 8.1 ❄️. The due date for this milestone has already passed. Please assign it to a milestone with a later deadline or check whether the release for this milestone has already been finished.

Generated by 🚫 Danger

[SergioEstevao]

Tested the following scenarios:

• Opening NYT private feeds url on Episode detail and Using Web Player inside the app
• Opening NYT private feeds url on Episode detail and Using External Safari
• Using the direct link in Safari

All worked correctly and opened the correct podcast after loading.

Code looks good.

[leandroalonso]

@claude review

[claude]

Claude finished @leandroalonso's task —— View job

---

Code Review Complete ✅

• Check for repository-specific guidelines (CLAUDE.md)
• Read and analyze the changed files
• Review code quality and best practices
• Check for security concerns
• Evaluate performance implications
• Assess testing coverage
• Verify documentation
• Provide comprehensive feedback

---

Summary

This PR successfully addresses PCIOS-337 by fixing a presentation issue when subscribing to a podcast from a URL within a Safari view controller. The changes are well-targeted and minimal, which is excellent. The core fix modifies SceneHelper.rootViewController() to optionally bypass the top-most presented view controller, allowing the subscribe route handler to properly dismiss any presented views before showing the loading alert.

✅ Positive Aspects

1. Minimal and focused changes: Only 14 additions and 5 deletions across 2 files - exactly what's needed to fix the issue
2. Backward compatible: The includeTopMost parameter defaults to true, preserving existing behavior for all other call sites
3. Clear intent: The inline comment at AppDelegate+UrlHandling.swift:174 clearly explains why includeTopMost: false is needed
4. Consistent pattern: The dismiss pattern rootController.dismiss(animated: false, completion: nil) matches other similar usages in the same file (lines 52, 274)
5. Well-tested: The PR description includes comprehensive testing instructions covering multiple entry points

🔍 Review Feedback

Code Quality: Good ✅

Strengths:

• Clean Swift code following the existing codebase conventions
• Appropriate use of guard statements
• Good variable naming (rootController vs controller)

Minor Suggestions:

1. Consider renaming the parameter (AppDelegate+UrlHandling.swift:174 and SceneHelper.swift:26)

   The parameter name includeTopMost could be clearer. Consider:

   • skipPresentedViewControllers: Bool = false (defaults to false, set to true to skip)
   • includePresentedViewController: Bool = true (more explicit about what's included)

   Current usage reads a bit awkwardly: "don't include top most" → includeTopMost: false

   Alternative that might be clearer:

   class func rootViewController(skipPresentedViewController: Bool = false) -> UIViewController? {
       let appScene = connectedScene()?.windows.first(where: { $0.rootViewController is MainTabBarController })
       let rootVC = appScene?.rootViewController
       if skipPresentedViewController {
           return rootVC
       }
       return rootVC?.topMostPresentedViewController ?? rootVC
   }

2. Comment placement (AppDelegate+UrlHandling.swift:174)

   The inline comment is helpful but could be more concise. Swift convention often places longer comments above the line:

   guard
       let strongSelf = self,
       // Get the main root controller without considering presented views
       let rootController = SceneHelper.rootViewController(includeTopMost: false),
       let subscribeUrl = (parameters[JLRouteURLKey] as? URL)?.absoluteString
   else {
       return false
   }

Security: No Issues ✅

• No new security vulnerabilities introduced
• The existing URL validation and error handling remain unchanged
• The dismiss behavior doesn't create any security concerns

Performance: No Issues ✅

• The changes have negligible performance impact
• The optional parameter allows the function to short-circuit when includeTopMost: false
• No blocking operations or heavy computations added

Testing: Adequate ⚠️

Observations:

• ✅ Manual testing instructions are very comprehensive
• ✅ The PR checklist indicates unit tests were considered
• ⚠️ No unit tests exist for URL routing/handling in the codebase (searched for JLRoutes in test files, found none)
• ⚠️ No unit tests added for SceneHelper.rootViewController() parameter

Recommendation:
While the existing pattern in this codebase doesn't include tests for routing logic, this would be a good opportunity to add integration tests for critical user flows like the subscribe route. However, given that:

1. The existing codebase has no tests for this area
2. The change is minimal and well-tested manually
3. Adding test infrastructure would be a larger effort

I don't consider this a blocker for this PR, but it would be valuable to track as future work.

Documentation: Good ✅

• ✅ CHANGELOG updated (implied by PR checklist being checked)
• ✅ Inline comment explains the rationale
• ✅ PR description provides clear context and testing steps
• ✅ Linear issue reference included

🎯 Specific Line-by-Line Notes

podcasts/SceneHelper.swift:26-32

class func rootViewController(includeTopMost: Bool = true) -> UIViewController? {
    let appScene = connectedScene()?.windows.first(where: { $0.rootViewController is MainTabBarController })
    let rootVC = appScene?.rootViewController
    if includeTopMost {
        return rootVC?.topMostPresentedViewController ?? rootVC
    }
    return rootVC
}

Observation: The logic is correct, but consider the inverted logic alternative mentioned above for clarity. The current implementation works perfectly but reads slightly awkwardly with the double negative.

podcasts/AppDelegate+UrlHandling.swift:171-178

JLRoutes.global().addRoute("/subscribe/*") { [weak self] parameters -> Bool in
    guard
        let strongSelf = self,
        let rootController = SceneHelper.rootViewController(includeTopMost: false),
        let subscribeUrl = (parameters[JLRouteURLKey] as? URL)?.absoluteString
    else {
        return false
    }

Observation: Perfect use of the new parameter. The variable rename from controller to rootController makes the intent clearer throughout the function.

podcasts/AppDelegate+UrlHandling.swift:188-189

rootController.dismiss(animated: false, completion: nil)
strongSelf.progressDialog?.showAlert(rootController, hasProgress: false, completion: {

Observation: Consistent usage of rootController throughout. The dismiss-then-present pattern is correct and matches other similar flows in the file.

🏁 Conclusion

This is a well-executed bug fix that addresses the root cause of the presentation issue. The solution is elegant, minimal, and maintains backward compatibility. The code quality is good, and while there are minor suggestions for improved clarity, none are blockers.

Recommendation: ✅ Approve with minor suggestions

The suggestions around parameter naming are purely for readability and can be addressed in this PR or left as-is since the current implementation is functionally correct and follows existing patterns.

Great work on the fix! 🎉

---