Fix iOS 13.0 keyWindow deprecation warning

[2sem]

Summary

Fixed the iOS 13.0 deprecation warning for keyWindow as described in issue #6.

Changes

• Added UIApplication extension with keyRootViewController property
• Replaced deprecated keyWindow calls with keyRootViewController at lines 463 and 467
• Solution supports applications with multiple scenes in iOS 13+

Closes #6

---

Generated with Claude Code

[claude]

Pull Request Review

Summary

This PR addresses the iOS 13.0 deprecation warning for keyWindow by introducing a UIApplication extension. The solution is generally sound, but there are several concerns that should be addressed.

Code Quality & Best Practices

✅ Positive Points

• Clean implementation that properly handles the iOS 13+ scene-based architecture
• Consistent replacement of deprecated API calls
• Good use of optional chaining for safe unwrapping

⚠️ Issues Found

1. Access Control Missing (GADManager/GADManager.swift:14)

The UIApplication extension lacks an access modifier. Since this is a library/package, the extension should be explicitly marked as internal or private to avoid polluting the global namespace for consumers of this package.

Recommendation:

private extension UIApplication {
    var keyRootViewController: UIViewController? {
        // ...
    }
}

2. Inconsistent Behavior with Multi-Scene Apps (GADManager/GADManager.swift:16)

The extension uses .first to get a window scene, which may not return the correct scene in multi-scene applications. This could cause ads to present on the wrong scene or fail to present entirely.

Current code:

guard let windowScene = UIApplication.shared.connectedScenes.first as? UIWindowScene else { return nil }

Better approach:

guard let windowScene = UIApplication.shared.connectedScenes
    .first(where: { $0.activationState == .foregroundActive }) as? UIWindowScene else {
    // Fallback to any active scene
    guard let windowScene = UIApplication.shared.connectedScenes.first as? UIWindowScene else {
        return nil
    }
    return windowScene.windows.first(where: { $0.isKeyWindow })?.rootViewController
}
return windowScene.windows.first(where: { $0.isKeyWindow })?.rootViewController

This ensures you get the actively used scene rather than an arbitrary one.

Potential Bugs

🐛 Critical: Race Condition in Multi-Scene Apps

When an app has multiple scenes, connectedScenes.first could return:

• A backgrounded scene
• An inactive scene
• A different scene than intended

Impact: Ads might fail to display or display on the wrong screen.

Test scenario:

1. Create an iPad app with multiple windows
2. Put one window in background
3. Try to show an ad
4. The ad might attempt to present on the backgrounded window and fail

Performance Considerations

Minor: Repeated Scene Lookups

The property is called multiple times in __show method (lines 463, 467). Each call performs:

• Scene enumeration
• Type casting
• Window filtering

Suggestion: Consider caching the result in a local variable:

let rootVC = UIApplication.shared.keyRootViewController
guard !(rootVC?.presentedViewController is UIAlertController) else {
    // ...
}

Security Concerns

✅ No security issues identified. The change doesn't introduce any new attack vectors.

Test Coverage

❌ Missing Tests

The PR has no test coverage. Key scenarios that should be tested:

1. Scene availability: What happens when connectedScenes is empty?
2. Multi-scene behavior: Does it work correctly with multiple scenes?
3. Background state: What happens when all scenes are backgrounded?
4. Backwards compatibility: Does the implementation work on iOS 13.0+?

Recommendation: Add unit tests that mock scene scenarios, or at minimum document the expected behavior in these edge cases.

Additional Suggestions

1. iOS Version Compatibility Check

Consider adding an @available check or fallback:

extension UIApplication {
    var keyRootViewController: UIViewController? {
        if #available(iOS 13.0, *) {
            guard let windowScene = UIApplication.shared.connectedScenes
                .first(where: { $0.activationState == .foregroundActive }) as? UIWindowScene else {
                return UIApplication.shared.connectedScenes
                    .compactMap { $0 as? UIWindowScene }
                    .first?.windows.first(where: { $0.isKeyWindow })?.rootViewController
            }
            return windowScene.windows.first(where: { $0.isKeyWindow })?.rootViewController
        } else {
            return UIApplication.shared.keyWindow?.rootViewController
        }
    }
}

2. Add Documentation

Add a comment explaining the purpose and limitations:

/// Returns the root view controller of the key window in the active scene.
/// For multi-scene apps, attempts to use the foreground active scene.
/// - Returns: The root view controller, or nil if no valid scene/window is found.

Verdict

Status: ⚠️ Changes Requested

The core solution is correct for fixing the deprecation warning, but the implementation needs refinement for production use in multi-scene applications. The main concerns are:

1. Access control for the extension
2. Proper scene selection for multi-scene apps
3. Missing test coverage
4. Lack of documentation

Please address the multi-scene selection logic before merging to ensure robust behavior across different iOS configurations.

---

Review generated with Claude Code

[2sem]

@claude thereviewer mentioned

UIApplication.shared.connectedScenes
                .first(where: { $0.activationState == .foregroundActive })

[claude]

Claude finished @2sem's task in 1m 34s —— View job

---

Implemented foregroundActive scene selection ✓

I've successfully implemented the reviewer's suggestion to improve multi-scene handling in the keyRootViewController extension.

Changes Made

Improved Scene Selection Logic (GADManager/GADManager.swift:14-35)

• Added priority-based scene selection that first attempts to use .foregroundActive scenes
• Implemented fallback logic using compactMap to handle cases where no foreground active scene exists
• This ensures ads display on the correct scene in multi-scene iPad applications

Enhanced Code Quality

• Changed extension access level to private to prevent namespace pollution for library consumers
• Added comprehensive documentation comments explaining the purpose, behavior, and return value
• Improved code readability with clear comments for each step

Implementation Details

The new implementation follows a two-tier approach:

1. Primary: Attempts to find a foreground active scene using .first(where: { $0.activationState == .foregroundActive })
2. Fallback: If no foreground active scene exists, uses any available connected scene

This approach addresses the critical race condition issue identified in the review where .first could return backgrounded or inactive scenes, causing ads to fail or display on the wrong screen.

Completed Tasks

• Read current implementation
• Implement foregroundActive scene selection with fallback logic
• Add private access control to extension
• Add documentation
• Commit and push changes

---

Branch: claude/issue-6-20260107-1358 |

[claude]

Code Review: Fix iOS 13.0 keyWindow deprecation warning

Summary

This PR successfully addresses the iOS 13.0 deprecation warning for keyWindow by introducing a private extension with improved multi-scene support. The solution is well-implemented overall, but there are a few areas that could be improved.

✅ Strengths

1. Proper Deprecation Fix: Correctly replaces deprecated keyWindow API with scene-based approach
2. Multi-scene Support: Good prioritization logic (foregroundActive → any connected scene)
3. Privacy: Using private extension prevents namespace pollution
4. Documentation: Clear comments explaining the fallback logic
5. Backward Compatibility: The optional chaining ensures graceful handling when scenes aren't available

🔍 Code Quality & Best Practices

Issue 1: Redundant UIApplication.shared calls

Location: GADManager.swift:21-22, 27-28

The extension is already on UIApplication, so you don't need to call UIApplication.shared within the computed property. This should be self:

var keyRootViewController: UIViewController? {
    // Use 'self' instead of 'UIApplication.shared'
    if let windowScene = self.connectedScenes
        .first(where: { $0.activationState == .foregroundActive }) as? UIWindowScene {
        return windowScene.windows.first(where: { $0.isKeyWindow })?.rootViewController
    }

    guard let windowScene = self.connectedScenes
        .compactMap({ $0 as? UIWindowScene })
        .first else {
        return nil
    }

    return windowScene.windows.first(where: { $0.isKeyWindow })?.rootViewController
}

Issue 2: Inconsistent window selection

Location: GADManager.swift:23, 33

Both branches look for isKeyWindow, but in the fallback case, if there's no key window, it might be better to just take the first window as a final fallback:

// Fallback option
return windowScene.windows.first?.rootViewController

Or if you want to maintain strict behavior, this is fine as-is.

⚠️ Potential Issues

Issue 3: iOS version compatibility

Location: GADManager.swift:14-35

The UIWindowScene and connectedScenes APIs are only available in iOS 13+. If this library supports iOS 12 or earlier, this code will crash. You should either:

1. Add @available(iOS 13.0, *) annotation and provide a fallback, or
2. Document the minimum iOS version requirement clearly

Example fix:

private extension UIApplication {
    var keyRootViewController: UIViewController? {
        if #available(iOS 13.0, *) {
            // Current implementation
            if let windowScene = self.connectedScenes
                .first(where: { $0.activationState == .foregroundActive }) as? UIWindowScene {
                return windowScene.windows.first(where: { $0.isKeyWindow })?.rootViewController
            }
            
            guard let windowScene = self.connectedScenes
                .compactMap({ $0 as? UIWindowScene })
                .first else {
                return nil
            }
            
            return windowScene.windows.first(where: { $0.isKeyWindow })?.rootViewController
        } else {
            // Fallback for iOS 12 and earlier
            #if compiler(>=5.1)
            return UIApplication.shared.windows.first(where: { $0.isKeyWindow })?.rootViewController
            #else
            return UIApplication.shared.keyWindow?.rootViewController
            #endif
        }
    }
}

🔒 Security Considerations

No security concerns identified. The code safely accesses UI components with proper optional handling.

⚡ Performance Considerations

Minor concern: The extension performs scene iteration on each call. If keyRootViewController is called frequently, consider:

• Caching the result (though this adds complexity for invalidation)
• However, for this use case (checking before presenting ads), the performance impact is negligible

🧪 Test Coverage

Missing: No unit tests were added for the new extension. Consider adding tests for:

• Single scene apps
• Multi-scene apps with foregroundActive scene
• Multi-scene apps without foregroundActive scene (fallback path)
• Apps with no connected scenes (nil handling)

However, I understand UI testing in iOS can be challenging, and this might require integration tests rather than unit tests.

📝 Additional Observations

1. Line number discrepancy: The issue mentions lines 455, 459, but the changes are at lines 478, 482. This is normal after adding the extension at the top.

2. Commented code: There's commented code at GADManager.swift:478-480. Consider cleaning this up in a future commit if it's no longer needed.

✨ Recommendation

Approve with minor changes: The core fix is solid and addresses the deprecation warning correctly. The main concern is iOS version compatibility. Please verify:

1. What's the minimum iOS deployment target for this library?
2. If it's iOS 12 or earlier, add the #available check
3. Consider using self instead of UIApplication.shared in the extension

Overall, this is a well-implemented fix that improves the codebase! 👍

[2sem]

@claude we have already UIApplication+.swift

[claude]

Claude finished @2sem's task in 1m 1s —— View job

---

Analysis of UIApplication extension placement

Thanks for pointing that out! I've reviewed both files and you're absolutely right - there's already a dedicated file for UIApplication extensions.

Current Situation

Existing file: GADManager/extensions/UIApplication+.swift

• Contains a public extension with openSettings() and helper methods
• Well-organized, documented code

Current PR implementation: GADManager.swift:14-35

• The new keyRootViewController property is defined as a private extension inline
• This creates a separate extension definition instead of using the existing extensions file

Recommendation

The keyRootViewController property should be moved to UIApplication+.swift to maintain proper code organization. Here's why:

1. Code Organization: All UIApplication extensions should live in the same file
2. Maintainability: Developers expect to find all UIApplication helpers in one place
3. Consistency: Follows the existing project structure pattern

Implementation Approach

Since the extension is marked private, we have two options:

Option A: Keep it private (recommended for this use case)

// In UIApplication+.swift, add at the bottom:

/// Private extension for internal GADManager use only
private extension UIApplication {
    /// Returns the root view controller of the key window in the active scene.
    /// For multi-scene apps, prioritizes the foreground active scene with fallback logic.
    /// - Returns: The root view controller, or nil if no valid scene/window is found.
    var keyRootViewController: UIViewController? {
        // Current implementation...
    }
}

Option B: Make it internal/public
If other parts of the codebase could benefit from this helper, change private to internal or add to the existing public extension.

Additional Notes

While reviewing, I also noticed the same issues from my previous review still apply:

• Should use self instead of UIApplication.shared within the extension (lines 21, 27)
• May need @available(iOS 13.0, *) check depending on deployment target

Would you like me to:

1. Move the extension to UIApplication+.swift keeping it private?
2. Move it and also fix the self vs UIApplication.shared issue?
3. Just provide guidance and you'll handle the refactoring?

Todo List

• Read existing UIApplication+.swift file
• Read current GADManager.swift implementation
• Analyze whether to move the extension
• Provide recommendation

---

Branch: claude/issue-6-20260107-1358 |

[2sem]

@claude Move the extension to UIApplication+.swift keeping it private

[claude]

Claude finished @2sem's task in 1m 40s —— View job

---

Successfully moved keyRootViewController extension ✓

Changes Made

Refactored Extension Location (Commit: 5e96586)

• Moved the private keyRootViewController extension from GADManager.swift to GADManager/extensions/UIApplication+.swift
• Fixed UIApplication.shared → self within the extension (lines 49, 55 in UIApplication+.swift)
• Maintained private access control as requested

Benefits

1. Better Code Organization: All UIApplication extensions now live in a single dedicated file
2. Improved Maintainability: Developers know where to find UIApplication helpers
3. Cleaner Separation: GADManager.swift now focuses solely on GAD manager logic
4. Fixed Best Practice Issue: Using self instead of UIApplication.shared within the extension

Files Changed

• GADManager/extensions/UIApplication+.swift: Added private extension at lines 42-63
• GADManager/GADManager.swift: Removed the extension (previously lines 14-35)

The extension retains all its functionality:

• Prioritizes foreground active scenes for multi-scene apps
• Includes fallback logic for any connected scene
• Properly documented with comments

--- | Branch: claude/issue-6-20260107-1358

[claude]

Code Review - PR #7: Fix iOS 13.0 keyWindow deprecation warning

Summary

This PR successfully addresses the iOS 13.0 deprecation warning for keyWindow by introducing a new keyRootViewController computed property. The implementation is well-thought-out and handles multi-scene scenarios appropriately.

---

✅ Strengths

1. Correct API Deprecation Fix: Properly replaces the deprecated keyWindow API with scene-based window access for iOS 13+
2. Multi-Scene Support: Implementation correctly prioritizes foreground active scenes with appropriate fallback logic
3. Good Documentation: Clear comments explain the purpose and behavior of the new property
4. Minimal Invasive Changes: Only changes what's necessary (2 call sites in GADManager.swift)
5. Scoping: Uses private extension appropriately to limit the scope of the helper property

---

🔍 Code Quality & Best Practices

Good:

• Clean, readable code with logical fallback chain
• Follows Swift conventions and naming patterns
• The implementation matches Apple's recommended approach for scene-based apps

Suggestions:

1. iOS Version Availability (GADManager/extensions/UIApplication+.swift:47)

   • The connectedScenes API is only available in iOS 13+, but there's no @available check
   • Consider adding:

   @available(iOS 13.0, *)
   var keyRootViewController: UIViewController? {
       // ... existing implementation
   }

   • This ensures compile-time safety and makes the iOS version requirement explicit

2. Documentation Enhancement (GADManager/extensions/UIApplication+.swift:44-46)

   • The documentation is good but could mention the iOS 13+ requirement
   • Consider adding a note about when this returns nil

---

🐛 Potential Issues

Critical:

1. iOS 12 Compatibility Concern (GADManager.podspec:74)

   • The podspec specifies s.platform = :ios, "12.0" minimum deployment target
   • The new keyRootViewController property uses APIs only available in iOS 13+
   • Impact: This will cause a runtime crash on iOS 12 devices when calling connectedScenes
   • Recommendation: Either:
     • Add @available(iOS 13.0, *) and provide a fallback for iOS 12 using the old keyWindow API
     • Update the minimum deployment target to iOS 13.0 in the podspec

   Suggested fix:

   var keyRootViewController: UIViewController? {
       if #available(iOS 13.0, *) {
           // Current iOS 13+ implementation
           if let windowScene = self.connectedScenes
               .first(where: { $0.activationState == .foregroundActive }) as? UIWindowScene {
               return windowScene.windows.first(where: { $0.isKeyWindow })?.rootViewController
           }
           
           guard let windowScene = self.connectedScenes
               .compactMap({ $0 as? UIWindowScene })
               .first else {
               return nil
           }
           
           return windowScene.windows.first(where: { $0.isKeyWindow })?.rootViewController
       } else {
           // iOS 12 fallback
           return self.keyWindow?.rootViewController
       }
   }

---

⚡ Performance Considerations

1. Computed Property Performance (GADManager/extensions/UIApplication+.swift:47)

   • keyRootViewController is a computed property that iterates through scenes every call
   • Called at GADManager.swift:459 and 463 (could be in a hot path for ad presentation)
   • Impact: Minor - scene iteration is typically small (1-2 scenes)
   • Suggestion: Consider caching if this becomes performance-critical, though current implementation is likely fine

2. Double Window Lookup

   • The implementation searches for isKeyWindow in both the active scene and fallback
   • This is appropriate and necessary for correctness

---

🔒 Security Concerns

No security issues identified. The changes are purely related to UI window management.

---

🧪 Test Coverage

Missing:

• No unit tests found in the repository
• Recommendation: Add tests covering:
  • iOS 13+ behavior with multiple scenes
  • iOS 13+ behavior with no foreground active scene
  • iOS 12 fallback behavior (if iOS 12 support is maintained)
  • Verify the property returns nil gracefully when no valid scene exists

Manual Testing Needed:

• Test on iOS 12 device/simulator (will currently crash)
• Test on iOS 13+ with single scene
• Test on iOS 13+ with multiple scenes
• Test when app transitions between background/foreground states

---

📋 Action Items

Before Merge:

1. ⚠️ Critical: Fix iOS 12 compatibility issue (add version check or update deployment target)
2. Add @available(iOS 13.0, *) attribute if maintaining iOS 12 support
3. Consider adding basic test coverage

Nice to Have:
4. Enhance documentation to mention iOS version requirements
5. Test on various iOS versions and multi-scene scenarios

---

📝 Additional Notes

• The solution aligns well with the issue description and suggested implementation
• Lines referenced in issue 'keyWindow' was deprecated in iOS 13.0 #6 were 455 and 459, PR correctly addresses 463 and 467 (likely line number shifted)
• Consider whether the commented-out code at GADManager.swift:455-457 should be removed as part of code cleanup

---

Overall Assessment: Good fix for the deprecation warning, but has a critical iOS 12 compatibility issue that must be addressed before merging. Once the version compatibility is resolved, this PR will be ready to merge.

[2sem]

@gemini-cli /review kk