Skip to content

Update BrowserSignin initializer to not throw unnecessarily - #251

Merged
AlexNachbaur merged 4 commits into
masterfrom
BrowserSigninInitializer
Sep 11, 2025
Merged

Update BrowserSignin initializer to not throw unnecessarily#251
AlexNachbaur merged 4 commits into
masterfrom
BrowserSigninInitializer

Conversation

@AlexNachbaur

Copy link
Copy Markdown
Contributor

The BrowserSignin initializer was only throwing because the AuthorizationCodeFlow's initializer being used was throwing if the redirectUri was nil. However, BrowserSignin was accepting a non-optional URL for that same value. As a result, AuthorizationCodeFlow would never throw.

Instead of using try! to prevent this (since the initializer may change in the future), I rearranged the code to utilize initializers that would prevent needing to use throws.

Copilot AI review requested due to automatic review settings September 11, 2025 17:48

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR refactors the BrowserSignin initializer to remove unnecessary throwing behavior by directly using non-throwing AuthorizationCodeFlow and SessionLogoutFlow initializers instead of relying on OAuth2Client's throwing initializer.

  • Replaces throwing BrowserSignin convenience initializers with non-throwing versions
  • Updates AuthorizationCodeFlow convenience initializers to accept an optional logoutRedirectUri parameter
  • Modifies test code to use the new non-throwing initialization patterns

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
Sources/BrowserSignin/BrowserSignin.swift Removes throws from convenience initializers and restructures initialization to use AuthorizationCodeFlow and SessionLogoutFlow directly
Sources/OAuth2Auth/Authentication/AuthorizationCodeFlow.swift Adds logoutRedirectUri parameter to convenience initializers
Tests/BrowserSigninTests/BrowserSigninInitializerTests.swift Updates test to use non-throwing initializer
Tests/BrowserSigninTests/BrowserSigninFlowTests.swift Refactors tests to manually create flows instead of using BrowserSignin convenience initializer
Sources/AuthFoundation/JWT/Protocols/Claim.swift Adds Sendable, Hashable, Equatable conformance to IsClaim protocol

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Comment thread Sources/BrowserSignin/BrowserSignin.swift

@IldarAbdullin-okta IldarAbdullin-okta left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@AlexNachbaur
AlexNachbaur merged commit 408b899 into master Sep 11, 2025
34 of 37 checks passed
@AlexNachbaur
AlexNachbaur deleted the BrowserSigninInitializer branch September 11, 2025 23:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants