Skip to content

Enable/disable sign in button #201

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 7 commits into from
Mar 24, 2025
Merged

Conversation

lauren-ciha
Copy link
Member

Before:

  • When a user clicks on the sign in button and signs in, the button remains enabled even as github.com redirects back to the extension. This means the user could initiate multiple requests with multiple redirect options, which is not a good user experience

After:

  • Now, when a user clicks on the sign in button and signs in, the sign in button is disabled when github.com redirects back.
  • Errors in the OAuth redirection flow are now displayed to the user

Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This pull request improves the sign in user experience by ensuring the sign in button is correctly disabled during OAuth redirections and by surfacing OAuth-related errors to the user.

  • Introduces a new OAuth redirection event in the developer ID interfaces and provider.
  • Updates UI components (SignInForm, SignOutForm, and SignInPage) to reflect the button state and to wire up related events.
  • Enhances timeout handling in the OAuth flow and adds corresponding tests.

Reviewed Changes

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

Show a summary per file
File Description
GitHubExtension.Test/DeveloperIdTests/OAuthRequestTest.cs Test for handling OAuth timeouts via delayed token creation.
GitHubExtension/DeveloperId/IDeveloperIdProvider.cs Added OAuthRedirected event to signal redirection outcomes.
GitHubExtension/Controls/Forms/SignInForm.cs Updated sign in logic to control button state with OAuth event handling.
GitHubExtension/DeveloperId/DeveloperIdProvider.cs Modifies OAuth redirection handling by invoking the OAuthRedirected event upon exceptions.
GitHubExtension/DeveloperId/OAuthRequest.cs Implements timeout handling for the OAuth token exchange using Task.WhenAny.
GitHubExtension/Controls/Pages/SignInPage.cs Subscribes to property change events to update page content.
GitHubExtension/Controls/Forms/SignOutForm.cs Added button enabled substitution in the sign out template.
Files not reviewed (1)
  • GitHubExtension/Controls/Templates/AuthTemplate.json: Language not supported
Comments suppressed due to low confidence (1)

GitHubExtension/DeveloperId/DeveloperIdProvider.cs:190

  • Using .Wait() on an asynchronous method can lead to potential deadlocks or blocking issues. Consider refactoring this to use async/await throughout the method.
oAuthRequest.CompleteOAuthAsync(authorizationResponse).Wait();

guimafelipe
guimafelipe previously approved these changes Mar 24, 2025
guimafelipe
guimafelipe previously approved these changes Mar 24, 2025
Copy link
Contributor

@Copilot Copilot AI left a comment

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 improves the user experience during OAuth sign in by disabling the sign in button after a successful sign in and displaying error messages on a failed OAuth redirection.

  • Tests now simulate and validate OAuth request timeouts.
  • A new OAuthRedirected event is added to propagate error or state changes.
  • The SignInForm is updated to manage the button's enabled state based on the OAuth flow outcome.

Reviewed Changes

Copilot reviewed 8 out of 9 changed files in this pull request and generated no comments.

Show a summary per file
File Description
GitHubExtension.Test/DeveloperIdTests/OAuthRequestTest.cs Test to validate timeout handling in OAuthRequest
GitHubExtension.Test/Controls/SignInFormTest.cs Added tests to ensure proper error handling and button state during OAuth redirection
GitHubExtension/DeveloperId/IDeveloperIdProvider.cs Added OAuthRedirected event in the provider interface
GitHubExtension/Controls/Forms/SignInForm.cs Updated button state logic and event subscriptions to control sign in button status
GitHubExtension/DeveloperId/DeveloperIdProvider.cs Updated HandleOauthRedirection with event notifications for OAuth and error handling
GitHubExtension/DeveloperId/OAuthRequest.cs Updated timeout logic for obtaining the OAuth access token
GitHubExtension/Controls/Pages/SignInPage.cs Subscribed to PropChanged event to update the sign in page on state changes
GitHubExtension/Controls/Forms/SignOutForm.cs Updated template substitutions for sign out button
Files not reviewed (1)
  • GitHubExtension/Controls/Templates/AuthTemplate.json: Language not supported
Comments suppressed due to low confidence (2)

GitHubExtension/DeveloperId/DeveloperIdProvider.cs:190

  • Calling .Wait() on an asynchronous method here can potentially cause a deadlock, especially on UI threads; consider using async/await all the way to avoid such issues.
oAuthRequest.CompleteOAuthAsync(authorizationResponse).Wait();

GitHubExtension.Test/Controls/SignInFormTest.cs:32

  • [nitpick] There appears to be a duplicate setup for mockGitHubClient.Oauth which may lead to confusion and maintenance overhead; consider consolidating this setup into a single occurrence.
mockGitHubClient.Setup(client => client.Oauth).Returns(mockOauthClient.Object);

Copy link
Contributor

@Copilot Copilot AI left a comment

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 improves the OAuth sign‐in experience by disabling the sign in button after a successful sign in and displaying errors when the OAuth flow fails. Key changes include adding timeout handling in OAuth requests, introducing an OAuthRedirected event, and updating sign in and sign out UI behavior.

Reviewed Changes

Copilot reviewed 8 out of 9 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
GitHubExtension.Test/DeveloperIdTests/OAuthRequestTest.cs Introduces a test for OAuth timeout handling
GitHubExtension.Test/Controls/SignInFormTest.cs Adds a test for handling OAuth redirection errors with duplicate mock setups
GitHubExtension/DeveloperId/IDeveloperIdProvider.cs Adds the OAuthRedirected event to notify about redirection outcomes
GitHubExtension/Controls/Forms/SignInForm.cs Implements button state updates in response to OAuth events and sign out actions
GitHubExtension/DeveloperId/OAuthRequest.cs Implements a timeout for token exchange in the OAuth flow
GitHubExtension/DeveloperId/DeveloperIdProvider.cs Updates HandleOauthRedirection to invoke OAuthRedirected and handles exceptions
GitHubExtension/Controls/Pages/SignInPage.cs Wires an event handler to update the sign in page when properties change
GitHubExtension/Controls/Forms/SignOutForm.cs Sets a hardcoded enabled state for the sign out button
Files not reviewed (1)
  • GitHubExtension/Controls/Templates/AuthTemplate.json: Language not supported

@lauren-ciha lauren-ciha merged commit 5cf40d7 into main Mar 24, 2025
3 checks passed
@lauren-ciha lauren-ciha deleted the user/laurenciha/oauth-timeout branch March 24, 2025 23:15
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.

2 participants