OAWebView Init

[codefiesta]

Testing SwiftUI can be relatively painful as it usually requires XCUIApplication or abstracting out bits of internal view logic into ViewModels which just offers a layer of abstraction.

Currently the OAWebView is initialized with an empty constructor and has the OAuth object injected with

@Environment(\.oauth)
var oauth: OAuth

If we change the OAWebView to be initialized with an OAuth object instead of using the @Environment injection, we should be able to greatly improve the test coverage because this will allow tests to call the OAWebViewCoordinator methods without using a .default injected OAuth object.

public init(oauth: OAuth) { 
    self.oauth = oauth
}

[hstdt]

How about this way?

@MainActor
public class OAWebViewCoordinator: NSObject {

    let webView: OAWebView?

    /// The oauth reference.
    let oauth: OAuth

    /// Initializer
    /// - Parameter webView: the webview that is being coordinated.
    init(_ webView: OAWebView) {
        self.webView = webView
        self.oauth = webView.oauth
        super.init()
    }

    /// Initializer for testing
    /// - Parameter oauth: the oauth reference.
    init(_ oauth: OAuth) {
        self.webView = nil
        self.oauth = oauth
        super.init()
    }
    ...
}

[codefiesta]

Yea, for sure. That could work as well. Another issue I'm looking at right now is how clients initialize the OAWebView in their applications and how to make this is as simple as possible for consumers of the Kit.

As of right now it's fairly simple to inject via the @Environment which will automatically get picked up by the OAWebView ...

struct OAuthApp: App {

    /// Build the scene body
    var body: some Scene {

        WindowGroup {
            ContentView()
        }
        
        WindowGroup(id: "oauth") {
            OAWebView()
        }
    }
} 

But, if the OAuth gets set via constructor, clients would need to do something like this ...

struct OAuthApp: App {

    let oauth: OAuth = .init(.module)

    /// Build the scene body
    var body: some Scene {

        WindowGroup {
            ContentView()
                 .environment(oauth)
        }
        
        WindowGroup(id: "oauth") {
            OAWebView(oauth: oauth)
        }
    }
} 

Ease of use vs flexibility + testability? I dunno, what do you think @hstdt?

[codefiesta]

I crafted up a couple of sample PRs to help myself fully understand the implications of changing the init signatures for the the OAWebView and OAWebViewCoordinator structs with some of the Pros and Cons:

OAWebView.init(oauth: OAuth):

This change alters the public signature of the OAWebView to specify an OAuth object to use.
See: #68

✅ Pros:

1. More explicitly specifies the OAuth object to use for clients.
2. Provides a single entry point and code path for both testing and production use.
3. Slightly higher test coverage - 89.3%

❌ Cons:

1. Clients will absolutely need to update the way they initialize their OAWebView.

OAWebViewCoordinator.init(oauth: OAuth):

This change alters the internal signature of the OAWebViewCoordinator to specify an OAuth object to use to allow for testing.
See: #69

✅ Pros:

1. No downstream changes for clients. Simply adds a new init that is called specifically for testing.
2. No init changes - clients can simply call OAWebView().environment(\.oauth, oauth) if they need to explicitly set the oauth to use.

❌ Cons:

1. Slightly lower test coverage: 88.5%
2. Adds a code path specifically created for testing purposes.

Food for Thought

I dunno, the test coverage differences seems negligible. I suppose it depends on how clients actually init an OAuth object and use it inside their apps. If they have a custom way of initializing an OAuth object then they would almost certainly need to either do one of the following right?:

OAWebView(oauth: oauth)

OR

OAWebView()
    .environment(\.oauth, oauth)

🤷‍♂️

[hstdt]

I wanted to raise a potential security concern when logging into same issuer across multiple window scenes simultaneously, as currently, the same key is used for Keychain storage.

The reason I bring this up is that by extending this scenario, we might be conflating two distinct use cases, leading to some awkwardness in the current implementation.

For most users, OAuth as a singleton is sufficient, and the current approach—using an environment key with default global value with the same Keychain storage—works well.

However, for advanced users who need multiple OAuth instances (e.g., maintaining simultaneous logged-in states, multi-scene apps, or different modules within the same scene handling separate tasks, or your example of package library let oauth: OAuth = .init(.module)) which required data isolating as a third party dependency), we could introduce a dedicated type like UniqueOAuthor whatever it calles.

This could be initialized with a uniqueId (e.g., UniqueOAuth(uniqueId: uuid.uuidString)), allowing Keychain to store multiple entries per service based on this ID. Users could then manage instances freely without interference. Additionally, UniqueOAuth wouldn’t be accessible via the environment’s default values, preventing accidental misuse or bugs.

Following this approach would mean having OAWebView provide two distinct initialization methods:

1. A default init (no parameters) for standard
2. A parameterized init for advanced multi-instance cases

WindowGroup(id: "oauth") {
    OAWebView()
}

let oauth = UniqueOAuth(uniqueId: uuid)
WindowGroup(id: uuid) {
    OAWebView(oauth: oauth)
}

PS: I'm not sure UniqueOAuth should be provided by OAuthKit, an OAuthProtocol might be enough. Further details would depend on your vision for this project. This is more of a conceptual direction at this stage rather than a fully-baked proposal.

[codefiesta]

So if I'm understanding this use case correctly, is the problem here that you could have 2 windows overwriting the same Keychain value (using the provider id)?

[codefiesta]

Curious, have you tried using different applicationTag values when initializing the OAuth objects? I'm wondering if that would solve the issue you are talking about?

let tag = "com.unique.id"
let options: [OAuth.Option: Sendable] = [.applicationTag: tag, .autoRefresh: true]
let oauth: OAuth = .init(.module, options: options)

[hstdt]

Oh I missed it! You're right, applicationTag is exactly the uniqueId I want.

So, in multi-instance cases, IMO OAWebView(oauth: oauth) will prevent potential bugs as I said above.

UniqueOAuth wouldn’t be accessible via the environment’s default values, preventing accidental misuse or bugs.

@main
struct OAuthSampleApp: App {
    static let id1 = UUID().uuidString
    static let id2 = UUID().uuidString
    @State var oauth1: OAuth = .init(.main, options: [.applicationTag: Self.id1])
    @State var oauth2: OAuth = .init(.main, options: [.applicationTag: Self.id2])

    var body: some Scene {

        WindowGroup {
            VStack {
                ContentView()
                    .environment(\.oauth, oauth1)
                
                ContentView()
                    .environment(\.oauth, oauth2)
            }
        }

        #if !os(tvOS)
        WindowGroup(id: Self.id1) {
            OAWebView()
                .environment(\.oauth, oauth1) // If I missed this one, OAWebView will read the default OAuth(.main)
            
            OAWebView(oauth: oauth1) // better for prevent potential bugs
        }
        
        WindowGroup(id: Self.id2) {
            ...
        }
        #endif
    }
}

furthermore, if OAuth conform to Codable

private func openWebView() {
    #if !os(tvOS)
    openWindow(id: "oauth", value: oauth) // if cannot conform to codable, openWindow(id: "oauth", value: oauth.applicationTag)
    #endif
}

WindowGroup(id: "oauth", for: OAuth.self) { oauth in 
    OAWebView(oauth: oauth)
}

[codefiesta]

Oh I missed it! You're right, applicationTag is exactly the uniqueId I want.

Awesome. Good to hear.

OAWebView(oauth: oauth1) // better for prevent potential bugs

Yea, I realized this during testing as well. The @Environment is convenient for nested sub views that need to access the oauth object, but it can quickly introduce bugs and confusion of how it's currently set and accessed inside the OAWebView. I think the implicit setting of it inside the OAWebView(oauth: OAuth) constructor will lead to a whole lot less headaches and confusion for developers.

I think I'll push this into the 1.2.0 milestone.

[codefiesta]

Change merged into milestone 1.2.0