Conversation
Test Results2 tests 2 ✅ 7s ⏱️ Results for commit 94e3136. ♻️ This comment has been updated with latest results. |
SummarySummary
CoverageAHKFlowApp.API - 63.3%
AHKFlowApp.Application - 98.1%
AHKFlowApp.Domain - 100%
AHKFlowApp.Infrastructure - 100%
AHKFlowApp.TestUtilities - 57.5%
AHKFlowApp.UI.Blazor - 79.8%
Per-assembly thresholds: Domain line≥85% br≥70% · Application line≥85% br≥45% · Infrastructure line≥70% br≥50% · API line≥57% br≥50% · UI.Blazor line≥65% br≥28% |
s205109
commented
Apr 28, 2026
Owner
Author
s205109
left a comment
s205109
left a comment
There was a problem hiding this comment.
Build & tests: Builds cleanly (
Contributor
There was a problem hiding this comment.
Pull request overview
Restores and hardens the local Entra ID authentication setup flow for the Blazor frontend by improving configuration validation, enhancing the sign-in failure UX, and making the Entra setup script more robust/consistent.
Changes:
- Added a centralized
AuthConfigurationValidator(used at startup) plus unit tests for missing/placeholder config scenarios. - Improved the auth failure UI (
Authentication.razor) with dev-only guidance for common local Entra setup issues. - Updated
setup-entra-app.ps1to create/verify the service principal and wait for Entra/Graph changes to become visible; documentation updated accordingly.
Reviewed changes
Copilot reviewed 11 out of 12 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/AHKFlowApp.UI.Blazor.Tests/Shared/LoginDisplayTests.cs | Adds bUnit tests verifying LoginDisplay navigates to login/logout routes based on auth state. |
| tests/AHKFlowApp.UI.Blazor.Tests/Auth/AuthConfigurationValidatorTests.cs | Adds unit tests for MSAL configuration validation behavior. |
| src/Frontend/AHKFlowApp.UI.Blazor/Shared/LoginDisplay.razor | Minor formatting; keeps login/logout navigation behavior. |
| src/Frontend/AHKFlowApp.UI.Blazor/Program.cs | Replaces inline required-key checks with AuthConfigurationValidator.ValidateForMsal. |
| src/Frontend/AHKFlowApp.UI.Blazor/Pages/Authentication.razor | Adds a custom LogInFailed UI with dev-only setup guidance and a “Back to home” action. |
| src/Frontend/AHKFlowApp.UI.Blazor/Auth/AuthConfigurationValidator.cs | New validator for required MSAL configuration + placeholder detection. |
| src/Frontend/AHKFlowApp.UI.Blazor/AHKFlowApp.UI.Blazor.csproj | Exposes internals to the test assembly via InternalsVisibleTo. |
| scripts/setup-entra-app.ps1 | Adds service principal creation + retry/wait logic for eventual consistency. |
| docs/deployment/entra-setup.md | Updates setup documentation to reflect additional wiring/wait behavior and AADSTS500011 guidance. |
| docs/architecture/authentication.md | Adds clarification about Microsoft-hosted error pages vs SPA-rendered errors. |
| README.md | Adds first-time Entra setup guidance and AADSTS500011 troubleshooting note. |
| Directory.Packages.props | Formatting-only change. |
- ConvertFrom-JsonSafe wrapper avoids ConvertFrom-Json throw on empty/non-JSON az output (failed sp lookups, transient Graph reads in retry loops). - AuthConfigurationValidator also rejects placeholder in AzureAd:DefaultScope. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Owner
Author
|
Reviewed the 5 Copilot comments — all valid, all addressed in cbbf352:
Additional issues found while reviewing
None are blockers — happy to land them in a follow-up if you'd rather keep this PR scoped to the auth-restore fix. |
- Unify config key: test-auth path now reads ApiHttpClient:BaseAddress (was ApiBaseUrl); update appsettings.Local.json + appsettings.E2E.json. - Add test for disabled logout in test-auth mode; use IAsyncLifetime to avoid sync-dispose throw on MudBlazor PopoverService. - Bump SP propagation wait to 90s (18x5s) in setup-entra-app.ps1. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Owner
Author
|
@copilot can you fix the test errors in the last run? |
…iBaseUrl Commit 8da70b7 changed Program.cs (test-auth branch) to read ApiHttpClient:BaseAddress instead of ApiBaseUrl, but SpaHost.cs still injected ApiBaseUrl into the intercepted appsettings responses. The base appsettings.json has ApiHttpClient:BaseAddress=http://localhost:5600, so E2E tests sent all API calls to localhost:5600 (unreachable in CI) rather than the local test proxy, causing td.draft-row timeouts. Agent-Logs-Url: https://github.com/s205109/AHKFlowApp/sessions/2ed7e782-fe49-415e-974a-0b3bcc50d9ba Co-authored-by: s205109 <4550557+s205109@users.noreply.github.com>
Contributor
Fixed in 94e3136. The root cause was a config key mismatch introduced in the previous commit ( |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Brief description of changes.
Checklist
dotnet test)dotnet build)