Improve Boilerplate social sign-in (#9620) #9621
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe pull request introduces modifications to the social sign-in functionality across multiple platforms. The changes primarily focus on enhancing the local HTTP server handling for social authentication, with updates to the Changes
Assessment against linked issues
Sequence DiagramsequenceDiagram
participant Client
participant LocalHttpServer
participant IdentityController
participant SocialProvider
Client->>LocalHttpServer: UseLocalHttpServerForSocialSignIn()
LocalHttpServer-->>Client: Return platform-specific result
Client->>IdentityController: GetSocialSignInUri
IdentityController->>SocialProvider: Request Authorization
SocialProvider-->>Client: Redirect with Authorization Code
Client->>LocalHttpServer: Start(CancellationToken)
LocalHttpServer-->>Client: Return Local Port
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Services/NoopLocalHttpServer.cs (1)
5-5: Consider preserving no-op behavior or clarifying intent.Currently, the
Startmethod throws aNotImplementedException. If the intended behavior is to do nothing (as implied by this class’ name), returning a sentinel value (e.g.,-1) or logging a warning might be more appropriate. If throwing is intentional, consider clarifying this is a placeholder for future implementation.src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Maui/Services/MauiLocalHttpServer.cs (2)
4-4: Unused import.
System.Net.Socketsmight not be considered an unused import if you’re explicitly callingTcpListener. However, confirm whether all references in this scope are necessary. If not, consider removing it for clarity.
91-98: Clarify intent ofUseLocalHttpServerForSocialSignIn.Returning
AppPlatform.IsAndroid is falsecould be confusing. This negative check might reduce readability. Consider renaming or inverting the logic to explicitly communicate your intent, e.g.return !AppPlatform.IsAndroid;.- return AppPlatform.IsAndroid is false; + return !AppPlatform.IsAndroid;src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Maui/App.xaml.cs (1)
79-81: Keep documentation minimal but helpful.Including download links for specific Android WebView versions can be useful during development. However, consider referencing official documentation or a more general solution for maintaining compatibility to avoid link rot over time.
Also applies to: 87-89
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Components/Pages/Identity/SignIn/SignInPage.razor.cs(1 hunks)src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Components/Pages/Identity/SignUp/SignUpPage.razor.cs(1 hunks)src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Services/Contracts/ILocalHttpServer.cs(1 hunks)src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Services/NoopLocalHttpServer.cs(1 hunks)src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Maui/App.xaml.cs(5 hunks)src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Maui/MauiProgram.Services.cs(1 hunks)src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Maui/Services/MauiLocalHttpServer.cs(2 hunks)src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Web/wwwroot/.well-known/assetlinks.json(2 hunks)src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Windows/Services/WindowsLocalHttpServer.cs(1 hunks)
🔇 Additional comments (10)
src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Services/NoopLocalHttpServer.cs (1)
7-10: Document potential implications of returning false.Returning
falsefromUseLocalHttpServerForSocialSignIn()indicates that no local HTTP server is used for social sign-in. Ensure that all calling code handles this scenario (e.g., it sets the port to -1 and handles that gracefully). The logic seems consistent, but confirm there's no accidental requirement for a real server.src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Services/Contracts/ILocalHttpServer.cs (1)
6-17: Excellent documentation of multi-platform sign-in flow.The newly added documentation provides a clear breakdown of how different platforms handle local HTTP servers and universal deep links. This helps maintainers and users of this interface understand its usage. Great job!
src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Windows/Services/WindowsLocalHttpServer.cs (1)
71-75: Platform alignment confirmed.Returning
trueinUseLocalHttpServerForSocialSignIn()aligns with Windows supporting a local HTTP server for social sign-in. This is consistent with the documentation in the interface comments. No concerns here.src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Components/Pages/Identity/SignUp/SignUpPage.razor.cs (1)
78-78: Ensure consistent exception handling.While you handle
KnownExceptioninside thetry...catch, theStartmethod inNoopLocalHttpServerthrows aNotImplementedException. If the “no-op” server is used unexpectedly here, it might propagate an unhandled exception. Confirm usage is limited to the correct platform or add handling for unexpected exceptions (like a fallback path).src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Maui/MauiProgram.Services.cs (1)
42-42: Ensure platform compatibility forILocalHttpServer.Registering
MauiLocalHttpServeras a singleton service for all platforms might raise compatibility issues where certain platforms do not support the local server. If this is intentional, confirm thatMauiLocalHttpServerincludes fallback or no-op behavior for unsupported platforms (e.g., Android).src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Maui/App.xaml.cs (3)
5-5: New logging import is good.This import is necessary for the newly introduced logging usage. No issues identified.
19-19: Injecting and storingILogger<App>is correct.The logger injection is properly implemented. Logging app lifecycle events with a typed logger enriches diagnostic data. Be sure to keep log messages minimal and relevant to avoid log inundation.
Also applies to: 29-29, 34-34
98-98: Good caution about unsupported WebView versions.The warning log is prudent. Confirm that the overall user flow remains coherent after displaying the alert and redirecting users to update. Use a more specific severity level (e.g.,
logger.LogError) if outdated WebView versions break major functionality.src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Components/Pages/Identity/SignIn/SignInPage.razor.cs (1)
99-99: Validate port usage fallback.Passing
-1for the port whenUseLocalHttpServerForSocialSignIn()isfalsemay require additional handling downstream. Confirm that the route calculation or other sign-in flows gracefully handle a negative port, to avoid confusion or runtime errors.src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Web/wwwroot/.well-known/assetlinks.json (1)
24-25: Verify the SHA256 fingerprint configurationBoth Android apps (AdminPanel.Template and Todo.Template) are using the same SHA256 fingerprint. Please verify if this is intentional or if each app should have its own unique fingerprint.
Run the following script to check for other occurrences of this fingerprint in the codebase:
Also applies to: 19-21
...rplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Web/wwwroot/.well-known/assetlinks.json
Outdated
Show resolved
Hide resolved
ysmoradi
deleted the
9620-boilerplate-project-template-social-sign-in-needs-improvements
branch
closes #9620
Summary by CodeRabbit
New Features
Improvements
Configuration Changes