-
-
Notifications
You must be signed in to change notification settings - Fork 254
Improve 401, 403 and 404 status code handling in bit Boilerplate (#11433) #11434
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
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 Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughRefactors 40X status code handling by converting a private middleware configurator into a WebApplication extension method and updating its call site. Adjusts the status code pipeline to avoid invoking Next for specific scenarios. Separately, sets the builder ContentRootPath to AppContext.BaseDirectory unconditionally. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Client
participant WA as WebApplication
participant SCP as StatusCodePages Middleware
participant H as Handle40XStatusCodes (custom)
C->>WA: HTTP request
WA->>SCP: Pass through pipeline
alt 401/403/404
SCP->>H: Invoke custom handler
note right of H: New behavior short-circuits<br/>without calling Next
H-->>C: Return 40X response
else Other status / pass-through
SCP-->>WA: Continue pipeline
WA-->>C: Response
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this 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 handling of 401, 403, and 404 status codes in the bit Boilerplate template by removing unnecessary conditional comments and refactoring the status code handling logic.
- Removes conditional compilation comments from WebApplication configuration
- Renames the status code handling method for better clarity
- Simplifies the middleware logic by removing an unnecessary else block
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| Program.cs | Removes conditional compilation comments around ContentRootPath setting |
| Program.Middlewares.cs | Renames status code handler method and simplifies middleware logic |
...mplates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Web/Program.Middlewares.cs
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Web/Program.Middlewares.cs (1)
183-198: Consider performance optimization for path checking.The middleware performs string operations on every request, including those that don't need status code handling. Consider caching the path check results or moving these checks inside the StatusCodePages handler for better performance.
- app.Use(async (context, next) => - { - if (context.Request.Path.HasValue) - { - if (context.Request.Path.Value.Contains(PageUrls.NotFound, StringComparison.InvariantCultureIgnoreCase)) - { - context.Response.StatusCode = (int)HttpStatusCode.NotFound; - } - if (context.Request.Path.Value.Contains(PageUrls.NotAuthorized, StringComparison.InvariantCultureIgnoreCase)) - { - context.Response.StatusCode = context.Request.Query["isForbidden"].FirstOrDefault() is "true" ? (int)HttpStatusCode.Forbidden : (int)HttpStatusCode.Unauthorized; - } - } - - await next.Invoke(context); - }); + app.Use(async (context, next) => + { + var path = context.Request.Path.Value; + if (!string.IsNullOrEmpty(path)) + { + if (path.Contains(PageUrls.NotFound, StringComparison.InvariantCultureIgnoreCase)) + { + context.Response.StatusCode = (int)HttpStatusCode.NotFound; + } + else if (path.Contains(PageUrls.NotAuthorized, StringComparison.InvariantCultureIgnoreCase)) + { + context.Response.StatusCode = context.Request.Query["isForbidden"].FirstOrDefault() is "true" ? (int)HttpStatusCode.Forbidden : (int)HttpStatusCode.Unauthorized; + } + } + + await next.Invoke(context); + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (2)
src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Web/Program.Middlewares.cs(2 hunks)src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Web/Program.cs(0 hunks)
💤 Files with no reviewable changes (1)
- src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Web/Program.cs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build and test
🔇 Additional comments (4)
src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Web/Program.Middlewares.cs (4)
57-57: LGTM: Clean API improvement with extension method pattern.The change from direct method call to extension method call improves the API design by making the middleware registration feel more natural in the pipeline chain.
181-181: Good refactor to extension method pattern.Converting the private method to an extension method improves the API design and makes it more consistent with other middleware registration patterns in ASP.NET Core.
206-220: No action required — redirects are scoped to Blazor components or unmatched requests.401/403 redirect only runs when the endpoint's metadata contains ComponentTypeMetadata (Blazor) and 404 redirect only when GetEndpoint() is null, so mapped API controllers (api/... via MapControllers) are not affected.
200-222: Verified — no else/Next removed; other status codes unaffected.Inspected src/Templates/Boilerplate/.../Program.Middlewares.cs and other UseStatusCodePages instances: HandleAsync only redirects 401/403 (Blazor endpoints) and 404 (no endpoint); there is no removed else or Next() invocation — other status codes continue to flow to the normal pipeline.
closes #11433
Summary by CodeRabbit
Bug Fixes
Refactor