Conversation
📝 WalkthroughWalkthroughAdds geo fields and service-radius to AllowedCity (DB, DTOs, migration); integrates geocoding/Nominatim search and PATCH/search endpoints; converts location handlers to Result-based flows; standardizes Result response envelopes; centralizes Keycloak role/permission mapping and adds dev auth debug middleware; many endpoints shifted to permission-based auth. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Endpoint as Locations API Endpoint
participant Handler as CreateAllowedCityHandler
participant Geo as GeocodingService
participant Repo as AllowedCityRepository
Client->>Endpoint: POST /admin/allowed-cities {city,state,lat?,lon?,radius?}
Endpoint->>Handler: Send CreateAllowedCityCommand
Handler->>Repo: ExistsAsync(city,state)?
alt exists
Repo-->>Handler: true
Handler-->>Endpoint: Result.Failure (Conflict)
Endpoint-->>Client: 409 Conflict (Result failure)
else
Handler->>Geo: GetCoordinatesAsync(city,state) [if coords missing]
Geo-->>Handler: GeoPoint or null
Handler->>Repo: AddAsync(AllowedCity with coords,radius)
Repo-->>Handler: created id
Handler-->>Endpoint: Result.Success(id)
Endpoint-->>Client: 200 OK (Result success)
end
sequenceDiagram
participant Client as Client
participant API as API
participant Inspect as InspectAuthMiddleware
participant Auth as JWT/AuthSubsystem
participant Logger as Logger
Client->>API: HTTP request (with Authorization)
API->>Inspect: InvokeAsync (dev-only)
Inspect->>Auth: read claims, roles, permissions
Auth-->>Inspect: claims data
Inspect->>API: register OnStarting to add debug headers
API->>Logger: optional diagnostic logs
API-->>Client: Response + debug headers (X-Debug-User, X-Debug-Roles, X-Debug-Permissions...)
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/Modules/Providers/Application/Handlers/Queries/GetProvidersByCityQueryHandler.cs (1)
35-38: Localize the failure message to Portuguese (user-facing response).The failure string is returned to the caller and should be in Portuguese per the language policy. Please translate it (e.g., “Ocorreu um erro ao recuperar os prestadores”).
src/Modules/Providers/Application/Handlers/Queries/GetProvidersByVerificationStatusQueryHandler.cs (1)
28-28: Localize user-facing error message to Portuguese.
Result.Failurereturns a user-facing string; per repo language policy it should be Portuguese. Suggested update below. Based on learnings, logs stay English, user-facing messages should be Portuguese.🔧 Proposed fix
- return Result<IReadOnlyList<ProviderDto>>.Failure("An error occurred while retrieving providers"); + return Result<IReadOnlyList<ProviderDto>>.Failure("Ocorreu um erro ao recuperar os prestadores");src/Modules/ServiceCatalogs/API/Endpoints/Service/GetServicesByCategoryEndpoint.cs (1)
18-34: Add permission requirement for consistency with other read endpoints.
GetServicesByCategoryEndpointlacks theServiceCatalogsReadpermission requirement thatGetAllServicesEndpointhas. WhileGetServiceByIdEndpointalso lacks this permission, the unfiltered list endpoint (GetAllServicesEndpoint) does require it. Consider whetherGetServicesByCategoryEndpointshould also requireServiceCatalogsReadfor consistent permission enforcement across read operations, or if the difference is intentional based on your authorization design.
🤖 Fix all issues with AI agents
In `@src/Bootstrapper/MeAjudaAi.ApiService/Extensions/SecurityExtensions.cs`:
- Line 310: The code currently appends raw exception messages to a response
header (context.Response.Headers.Append(...)) which leaks sensitive info and
uses a hardcoded header name; add a constant in AuthConstants.Headers (e.g.,
public const string DebugAuthFailure = "X-Debug-Auth-Failure") and change the
header emission in SecurityExtensions (where context.Response.Headers.Append is
called) to only run in non-production environments (use IHostEnvironment or
env.IsDevelopment()/IsStaging() as appropriate) and avoid including the full
exception text — include a short sanitized message or exception type instead
(e.g., ex.GetType().Name or a truncated/sanitized message) when adding the
header, and use the new AuthConstants.Headers.DebugAuthFailure constant for the
header name.
In `@src/Modules/Users/Application/Handlers/Commands/DeleteUserCommandHandler.cs`:
- Around line 7-8: Remove the duplicate ValidationMessages import by deleting
the using directive for MeAjudaAi.Shared.Utilities.Constants and keeping only
MeAjudaAi.Contracts.Utilities.Constants in DeleteUserCommandHandler.cs so the
ValidationMessages symbol is unambiguous; ensure the file retains the using
MeAjudaAi.Contracts.Utilities.Constants line and compile to confirm no other
references to the Shared namespace remain.
In
`@src/Modules/Users/Application/Handlers/Queries/GetUserByUsernameQueryHandler.cs`:
- Around line 6-7: Remove the redundant using directive for
MeAjudaAi.Shared.Utilities.Constants at the top of GetUserByUsernameQueryHandler
(the file/class GetUserByUsernameQueryHandler.cs); keep the existing using
MeAjudaAi.Contracts.Utilities.Constants for ValidationMessages and ensure no
other symbols from the Shared namespace are referenced—simply delete the unused
"using MeAjudaAi.Shared.Utilities.Constants;" line.
In
`@src/Modules/Users/Tests/Integration/GetUserByUsernameQueryIntegrationTests.cs`:
- Around line 8-9: Remove the redundant using directive referencing
MeAjudaAi.Shared.Utilities.Constants in GetUserByUsernameQueryIntegrationTests
(the import on line with "using MeAjudaAi.Shared.Utilities.Constants"); keep the
existing using MeAjudaAi.Contracts.Utilities.Constants which defines
ValidationMessages, so delete the unused Shared import and run the build/tests
to ensure no other references to that namespace remain.
In
`@src/Modules/Users/Tests/Unit/Application/Queries/GetUserByUsernameQueryHandlerTests.cs`:
- Around line 7-8: The file imports the same ValidationMessages namespace from
two places which is redundant and can cause ambiguity; remove the duplicate by
deleting the using for MeAjudaAi.Shared.Utilities.Constants and keep only using
MeAjudaAi.Contracts.Utilities.Constants so references to ValidationMessages
resolve unambiguously.
In `@src/Shared/Authorization/Keycloak/KeycloakPermissionResolver.cs`:
- Around line 370-382: Add unit tests in KeycloakPermissionResolverTests.cs to
assert that the new roles map to the correct permissions: create tests that call
KeycloakPermissionResolver.Resolve (or the relevant mapping method) with
"meajudaai-catalog-manager" and assert the returned permissions include
EPermission.ServiceCatalogsRead and EPermission.ServiceCatalogsManage, and
another test for "meajudaai-location-manager" asserting
EPermission.LocationsRead and EPermission.LocationsManage; also update the test
fixture/setup that seeds Keycloak roles (or the realm configuration used by
tests) to include these two roles so the resolver tests run against the expected
role set.
In `@src/Web/MeAjudaAi.Web.Admin/App.razor`:
- Around line 85-106: The log messages in DiagnoseOidcConfiguration are in
Portuguese; update all Logger calls in that method (Logger.LogInformation,
Logger.LogWarning, Logger.LogError) to English strings while preserving the same
semantics and structured parameters (e.g., keep "{Error}" placeholder and
include exception 'ex' in LogError). Specifically, translate the startup info,
the "detected problems" warning and the per-error warning entries, and the error
message in the catch block; leave method flow,
OidcDebugService.CheckKeycloakHealthAsync usage, and
healthCheck.GetDiagnosticSummary() unchanged.
In `@src/Web/MeAjudaAi.Web.Admin/Authorization/PolicyNames.cs`:
- Around line 32-36: Update the XML doc for the LocationsManagerPolicy constant
to reflect that the policy requires a role, not a permission: change the comment
text to indicate it "Requer role 'locations-manager'" (or similar phrasing
consistent with other policy docs) so the documentation for
PolicyNames.LocationsManagerPolicy matches the actual policy setup that calls
RequireRole("locations-manager") in Program.cs.
In `@src/Web/MeAjudaAi.Web.Admin/Services/OidcDebugService.cs`:
- Around line 6-9: This file contains mojibake in XML comments—save the source
file as UTF-8 and update the garbled Portuguese text in the OidcDebugService XML
docs and other commented strings (look for class/summary comments and comments
near methods referenced in this file such as OidcDebugService and any public
methods/properties) to proper Portuguese (e.g. replace "Servi�o para
diagnosticar problemas com OIDC e Keycloak. Fornece informa��es detalhadas sobre
a configura��o de autentica��o." with "Serviço para diagnosticar problemas com
OIDC e Keycloak. Fornece informações detalhadas sobre a configuração de
autenticação."). Ensure all occurrences at the regions flagged (around the
summary block and the other comment blocks) are corrected and encoded as UTF-8
so the accents render properly.
- Around line 45-114: Update all logger messages in OidcDebugService (calls to
_logger.LogInformation, _logger.LogDebug, _logger.LogWarning, _logger.LogError)
to English and remove emoji/malformed glyphs; e.g., replace "?? Iniciando
diagnóstico de Keycloak" with "Starting Keycloak diagnostics", " Well-Known
Error" messages and "?? Timeout..." with plain English equivalents, and change
final messages ("? Diagnóstico de Keycloak concluído", " ? Erro durante
diagnóstico de Keycloak") to "Keycloak diagnostics completed" and "Error during
Keycloak diagnostics". Keep existing structured logging placeholders and
variables (wellKnownUrl, tokenUrl, authUrl, _clientConfig.Keycloak.*,
_navigationManager.Uri, result) intact and do not change exception logging or
the result assignments.
- Around line 141-177: Translate all user-facing strings in GetDiagnosticSummary
to Portuguese (pt-BR) and replace the corrupted "????????..." border with a
clean UTF-8 box-drawing or simple ASCII border; update the list entries that
reference properties (Timestamp, Authority, ClientId, Scopes,
PostLogoutRedirectUri, WellKnownEndpointStatus, TokenEndpointUrl,
AuthorizationEndpointUrl, CurrentUrl, AppOrigin, IsHealthy, Errors) to
Portuguese labels (e.g., "Relatório de Diagnóstico Keycloak", "Data/Hora",
"Configuração", "Endpoints", "Informações do Cliente", "Status", "Erros") and
format the healthy/issue status text accordingly (e.g., "Saudável" / "Problemas
detectados"); ensure the final string join and error enumeration
(Errors.Select(...)) remain intact and that the new border characters are valid
UTF-8 or plain ASCII so the admin UI displays correctly.
🧹 Nitpick comments (19)
src/Modules/Providers/Application/Handlers/Queries/GetProviderByUserIdQueryHandler.cs (1)
44-44: Consider localizing the error message to Portuguese.If the
Result.Failuremessage is surfaced to API consumers or the frontend, it should be in Portuguese per the established language policy. Consider changing"Error getting provider"to a Portuguese equivalent such as"Erro ao buscar prestador".Based on learnings, user-facing API responses should be in Portuguese.
src/Bootstrapper/MeAjudaAi.ApiService/Endpoints/ConfigurationEndpoints.cs (1)
44-57: Harden Keycloak authority normalization/validation.
IfKeycloak:Authorityis provided with trailing slashes orKeycloak:BaseUrlis whitespace, the resulting authority can be malformed. Consider trimming/validating consistently.♻️ Suggested hardening
- var keycloakAuthority = configuration["Keycloak:Authority"]; + var keycloakAuthority = configuration["Keycloak:Authority"]?.TrimEnd('/'); if (string.IsNullOrWhiteSpace(keycloakAuthority)) { // Construir Authority a partir de BaseUrl e Realm - var keycloakBaseUrl = configuration["Keycloak:BaseUrl"] - ?? throw new InvalidOperationException("Keycloak:BaseUrl ou Keycloak:Authority deve estar configurado"); + var keycloakBaseUrl = configuration["Keycloak:BaseUrl"]; + if (string.IsNullOrWhiteSpace(keycloakBaseUrl)) + throw new InvalidOperationException("Keycloak:BaseUrl ou Keycloak:Authority deve estar configurado"); + keycloakBaseUrl = keycloakBaseUrl.TrimEnd('/'); - var keycloakRealm = configuration["Keycloak:Realm"] - ?? "meajudaai"; // Valor padrão + var keycloakRealm = configuration["Keycloak:Realm"]; + if (string.IsNullOrWhiteSpace(keycloakRealm)) + keycloakRealm = "meajudaai"; // Valor padrão + keycloakRealm = keycloakRealm.Trim('/'); - keycloakAuthority = $"{keycloakBaseUrl.TrimEnd('/')}/realms/{keycloakRealm}"; + keycloakAuthority = $"{keycloakBaseUrl}/realms/{keycloakRealm}"; }src/Modules/ServiceCatalogs/Application/Mappings/ServiceCatalogsMappingExtensions.cs (1)
3-4: Prefer a single constants namespace here.With both
MeAjudaAi.Shared.Utilities.ConstantsandMeAjudaAi.Contracts.Utilities.Constantsin scope,ValidationMessagescan become ambiguous or resolve to legacy values. If everything moved to Contracts, drop the Shared import (or fully qualify). Based on learnings, keep enums/constants aligned with Contracts.♻️ Suggested cleanup
-using MeAjudaAi.Shared.Utilities.Constants; using MeAjudaAi.Contracts.Utilities.Constants;src/Modules/Users/Tests/Unit/Application/Services/UsersModuleApiTests.cs (1)
6-8: Prefer Contracts constants only to avoid ambiguity.If
ValidationMessagesexists in both namespaces, references may become ambiguous or bind to legacy constants. Consider removing the Shared import once Contracts is the source of truth. Based on learnings, keep constants in Contracts.♻️ Suggested cleanup
-using MeAjudaAi.Shared.Utilities.Constants; using MeAjudaAi.Contracts.Utilities.Constants;src/Modules/Users/Application/Validators/UpdateUserProfileRequestValidator.cs (1)
3-5: Prefer a single constants namespace.Having both Shared and Contracts constants in scope can cause ambiguous or legacy resolution for
ValidationMessages/ValidationConstants. If Contracts is the target, drop the Shared import (or fully qualify). Based on learnings, prefer Contracts for constants.♻️ Suggested cleanup
-using MeAjudaAi.Shared.Utilities.Constants; using MeAjudaAi.Contracts.Utilities.Constants;src/Modules/ServiceCatalogs/Tests/Unit/Application/Mappings/ServiceCatalogsMappingExtensionsTests.cs (1)
5-6: Prefer Contracts constants only.If
ValidationMessagesexists in both namespaces, the reference can become ambiguous or resolve to legacy values. Consider removing the Shared import once Contracts is authoritative. Based on learnings, keep constants aligned with Contracts.♻️ Suggested cleanup
-using MeAjudaAi.Shared.Utilities.Constants; using MeAjudaAi.Contracts.Utilities.Constants;src/Modules/Users/Application/Validators/CreateUserCommandValidator.cs (1)
3-5: Prefer a single constants namespace.Keeping both Shared and Contracts constants in scope can cause ambiguous or legacy resolution of
ValidationMessages/ValidationConstants. If Contracts is the target, drop the Shared import (or fully qualify). Based on learnings, prefer Contracts.♻️ Suggested cleanup
-using MeAjudaAi.Shared.Utilities.Constants; using MeAjudaAi.Contracts.Utilities.Constants;src/Modules/Users/Tests/Unit/Application/Validators/UpdateUserProfileRequestValidatorTests.cs (1)
4-6: Prefer Contracts constants only to avoid ambiguity.If
ValidationMessagesexists in both namespaces, references may become ambiguous or bind to legacy values. Consider removing the Shared import once Contracts is the source of truth. Based on learnings, prefer Contracts constants.♻️ Suggested cleanup
-using MeAjudaAi.Shared.Utilities.Constants; using MeAjudaAi.Contracts.Utilities.Constants;src/Web/MeAjudaAi.Web.Admin/Services/OidcDebugService.cs (1)
51-56: Prefer injected HttpClient (or factory) over per-call instantiation.
In Blazor, repeated new HttpClient can lead to suboptimal resource usage. Consider DI-injecting HttpClient (or IHttpClientFactory on server) and reuse it.♻️ Suggested DI refactor
public class OidcDebugService { private readonly ClientConfiguration _clientConfig; private readonly ILogger<OidcDebugService> _logger; private readonly NavigationManager _navigationManager; + private readonly HttpClient _httpClient; @@ public OidcDebugService( ClientConfiguration clientConfig, ILogger<OidcDebugService> logger, - NavigationManager navigationManager) + NavigationManager navigationManager, + HttpClient httpClient) { _clientConfig = clientConfig; _logger = logger; _navigationManager = navigationManager; + _httpClient = httpClient; } @@ - using var client = new HttpClient { Timeout = TimeSpan.FromSeconds(10) }; + _httpClient.Timeout = TimeSpan.FromSeconds(10); @@ - var response = await client.GetAsync(wellKnownUrl); + var response = await _httpClient.GetAsync(wellKnownUrl);src/Modules/ServiceCatalogs/Tests/Unit/Application/ModuleApi/ServiceCatalogsModuleApiTests.cs (1)
7-9: Prefer Contracts constants only to avoid ambiguity.
With both namespaces imported,ValidationMessagescan become ambiguous if it exists in both. Please remove the Shared constants import and rely on Contracts. Based on learnings, prioritize Contracts constants.♻️ Suggested cleanup
-using MeAjudaAi.Shared.Utilities.Constants; using MeAjudaAi.Contracts.Utilities.Constants;src/Modules/ServiceCatalogs/Application/ModuleApi/ServiceCatalogsModuleApi.cs (1)
5-7: Make ValidationMessages source explicit (prefer Contracts).
IfValidationMessagesexists in both namespaces, this becomes ambiguous; even if not, keeping both imports obscures intent. Recommend removing the Shared import or aliasing Contracts explicitly. Based on learnings, prefer Contracts constants.♻️ Suggested explicit alias
-using MeAjudaAi.Shared.Utilities.Constants; -using MeAjudaAi.Contracts.Utilities.Constants; +using ValidationMessages = MeAjudaAi.Contracts.Utilities.Constants.ValidationMessages;src/Modules/Locations/API/Endpoints/LocationsAdmin/GetAllAllowedCitiesEndpoint.cs (1)
11-12: Missing blank line between using directive and namespace declaration.The using directive on line 11 lacks a blank line separator before the namespace declaration on line 12, which differs from the file's existing style (lines 1-10 have proper spacing).
Suggested fix
using MeAjudaAi.Contracts.Models; + namespace MeAjudaAi.Modules.Locations.API.Endpoints.LocationsAdmin;src/Modules/Locations/API/Endpoints/LocationsAdmin/CreateAllowedCityEndpoint.cs (1)
12-13: Missing blank line between using directive and namespace declaration.Same formatting inconsistency as in
GetAllAllowedCitiesEndpoint.cs. Add a blank line for consistency.Suggested fix
using MeAjudaAi.Contracts.Models; + namespace MeAjudaAi.Modules.Locations.API.Endpoints.LocationsAdmin;src/Modules/Locations/API/Endpoints/LocationsAdmin/GetAllowedCityByIdEndpoint.cs (1)
11-12: Missing blank line between using directive and namespace declaration.Consistent with other endpoint files in this PR - add a blank line separator.
Suggested fix
using MeAjudaAi.Contracts.Models; + namespace MeAjudaAi.Modules.Locations.API.Endpoints.LocationsAdmin;src/Web/MeAjudaAi.Web.Admin/Program.cs (3)
131-131: Remove commented-out code or add a TODO explaining why it's kept.The commented line
// builder.Services.AddPermissionBasedAuthorization(builder.Configuration);should either be removed if obsolete or accompanied by a TODO/FIXME explaining when it will be re-enabled.
154-157: CreateRoleNames.LocationsManagerconstant and use it for consistency.The
LocationsManagerPolicyuses a hardcoded"locations-manager"string while all other role-based policies useRoleNamesconstants. Definepublic const string LocationsManager = "locations-manager";in theRoleNamesclass and replace the hardcoded string in line 157.Note: The same issue exists in
CatalogManagerPolicy(line 149), which also uses a hardcoded string despiteRoleNames.CatalogManageralready being defined.
148-149: Replace hardcoded string withRoleNames.CatalogManagerconstant for consistency.Line 149 uses a hardcoded
"catalog-manager"string while other policies useRoleNames.*constants (RoleNames.Admin,RoleNames.ProviderManager,RoleNames.DocumentReviewer). TheRoleNames.CatalogManagerconstant exists and should be used here.Change:
policy.RequireRole("catalog-manager", RoleNames.Admin)to:
policy.RequireRole(RoleNames.CatalogManager, RoleNames.Admin)src/Modules/Locations/API/Endpoints/LocationsModuleEndpoints.cs (1)
38-41: Stale comment references removed authorization method.Line 39 comment still mentions "RequireAdmin" but the code now uses
RequirePermission. Update the comment to reflect the new permission-based authorization.📝 Suggested comment update
// Usa o sistema unificado de versionamento via BaseEndpoint - // RequireAdmin aplicado no grupo garante que todos endpoints são protegidos por padrão + // RequirePermission aplicado no grupo garante que todos endpoints são protegidos por padrão var endpoints = BaseEndpoint.CreateVersionedGroup(app, "admin/allowed-cities", "Allowed Cities") .RequirePermission(EPermission.LocationsManage);src/Web/MeAjudaAi.Web.Admin/App.razor (1)
77-79: Consider limiting OIDC diagnostics to development environment.Running OIDC health checks on every app initialization adds startup latency in production. Consider gating this behind an environment check or configuration flag.
♻️ Suggested conditional execution
Inject
IWebAssemblyHostEnvironmentand conditionally run diagnostics:`@inject` IJSRuntime JSRuntime `@inject` OidcDebugService OidcDebugService +@inject IWebAssemblyHostEnvironment HostEnvironment `@implements` IDisposable// Realizar diagnóstico de OIDC/Keycloak - await DiagnoseOidcConfiguration(); + if (HostEnvironment.IsDevelopment()) + { + await DiagnoseOidcConfiguration(); + }
src/Bootstrapper/MeAjudaAi.ApiService/Extensions/SecurityExtensions.cs
Outdated
Show resolved
Hide resolved
src/Modules/Users/Application/Handlers/Commands/DeleteUserCommandHandler.cs
Show resolved
Hide resolved
src/Modules/Users/Application/Handlers/Queries/GetUserByUsernameQueryHandler.cs
Show resolved
Hide resolved
src/Modules/Users/Tests/Integration/GetUserByUsernameQueryIntegrationTests.cs
Show resolved
Hide resolved
src/Modules/Users/Tests/Unit/Application/Queries/GetUserByUsernameQueryHandlerTests.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@src/Bootstrapper/MeAjudaAi.ApiService/Extensions/SecurityExtensions.cs`:
- Line 321: The file references AuthConstants in SecurityExtensions.cs (used in
the context.Response.Headers.Append call) but is missing its namespace import;
add the using directive for MeAjudaAi.Shared.Utilities.Constants to the file's
using/import section so AuthConstants resolves correctly.
In
`@src/Bootstrapper/MeAjudaAi.ApiService/Extensions/ServiceCollectionExtensions.cs`:
- Around line 168-170: The InspectAuthMiddleware is currently added
unconditionally via app.UseMiddleware<InspectAuthMiddleware>(), which can leak
sensitive auth headers; change ServiceCollectionExtensions to gate the
middleware registration behind environment or configuration by checking
IHostEnvironment (e.g., env.IsDevelopment() or !env.IsProduction()) or a config
flag (e.g., IConfiguration.GetValue<bool>("EnableInspectAuthMiddleware")) and
only call app.UseMiddleware<InspectAuthMiddleware>() when the check passes;
update the method that contains the app.UseMiddleware call to accept or resolve
the environment/config and add the conditional guard around the
InspectAuthMiddleware registration.
In `@src/Bootstrapper/MeAjudaAi.ApiService/Middleware/InspectAuthMiddleware.cs`:
- Around line 21-38: No trecho do método InvokeAsync em InspectAuthMiddleware.cs
existem comentários em inglês; converta todos esses comentários inline para
Português mantendo o mesmo sentido e contexto (por exemplo: "Wait,
AuthenticationMiddleware runs before this..." -> equivalente em Português),
garantindo que as observações sobre ordem de execução, uso de OnStarting e
restrições de adicionar headers permaneçam claras; edite os comentários dentro
do método InvokeAsync e quaisquer comentários relacionados a _next, HttpContext
e fluxo de autenticação/autorização para Português sem alterar a lógica do
código.
🧹 Nitpick comments (6)
src/Modules/Providers/Application/Handlers/Queries/GetProvidersByStateQueryHandler.cs (1)
31-35: Correct localization, but inconsistent with line 23.The Portuguese error message aligns with the language policy (user-facing API responses in Portuguese). However, line 23 still returns an English message
"State parameter is required". Consider updating it for consistency.Based on learnings, user-facing validation messages and API responses should be in Portuguese.
♻️ Suggested fix for consistency
if (string.IsNullOrWhiteSpace(query.State)) { logger.LogWarning("Invalid state parameter: state cannot be null or empty"); - return Result<IReadOnlyList<ProviderDto>>.Failure("State parameter is required"); + return Result<IReadOnlyList<ProviderDto>>.Failure("O parâmetro de estado é obrigatório"); }src/Modules/Users/Application/Validators/UpdateUserProfileRequestValidator.cs (1)
43-46: Optional: Consider translating inline comments to Portuguese.Per the codebase language policy, inline comments should be written in Portuguese while logs remain in English. These comments are pre-existing, so this is not blocking, but you may want to address it in a follow-up for consistency.
Based on learnings about the language policy.
src/Web/MeAjudaAi.Web.Admin/Program.cs (2)
131-131: Consider removing commented-out code.Commented-out code adds noise and can become stale. If
AddPermissionBasedAuthorizationis no longer needed, remove this line entirely. If it's temporarily disabled for debugging or a phased migration, add a TODO comment explaining when it should be restored or removed.♻️ Suggested fix
-// builder.Services.AddPermissionBasedAuthorization(builder.Configuration); // Autorização com políticas baseadas em roles
154-158: Policy registration is correct; consider reordering for consistency.The
LocationsManagerPolicyis correctly implemented requiring either "locations-manager" or "admin" roles, matching the pattern of other manager policies.Minor nit: Other manager policies (Provider, Document, Catalog) are grouped before
ViewerPolicy. Consider movingLocationsManagerPolicybeforeViewerPolicy(lines 151-153) for consistency.♻️ Optional reorder for consistency
// Política de Gerente de Catálogo - requer "catalog-manager" ou "admin" options.AddPolicy(PolicyNames.CatalogManagerPolicy, policy => policy.RequireRole(RoleNames.CatalogManager, RoleNames.Admin)); + // Política de Gerente de Localidades - requer "locations-manager" ou "admin" + options.AddPolicy(PolicyNames.LocationsManagerPolicy, policy => + policy.RequireRole(RoleNames.LocationsManager, RoleNames.Admin)); + // Política de Visualizador - qualquer usuário autenticado options.AddPolicy(PolicyNames.ViewerPolicy, policy => policy.RequireAuthenticatedUser()); - - // Política de Gerente de Localidades - requer "locations-manager" ou "admin" - options.AddPolicy(PolicyNames.LocationsManagerPolicy, policy => - policy.RequireRole(RoleNames.LocationsManager, RoleNames.Admin)); });src/Modules/Providers/Application/Handlers/Queries/GetProviderByIdQueryHandler.cs (1)
43-46: Localize all user-facing errors consistently (and centralize messages).The new Portuguese failure text aligns with the language policy, but the earlier
Error.NotFound("Provider not found")remains English. Consider moving these to shared validation/error message constants (e.g., in shared contracts) to keep localization consistent across handlers. Based on learnings, ...src/Bootstrapper/MeAjudaAi.ApiService/Extensions/SecurityExtensions.cs (1)
310-323: Comments should be in Portuguese per codebase language policy.Based on learnings, all in-code comments should be written in Portuguese while log messages remain in English. The comments on lines 312 and 316 are in English.
♻️ Proposed fix
- // Only emit debug header in non-production environments + // Emite header de debug apenas em ambientes não-produção if (!env.IsProduction()) { var sanitizedMessage = $"{context.Exception.GetType().Name}: {context.Exception.Message.Length} chars"; - // Or better: truncated message to avoid leaking too much internal state, but helpful for debugging + // Mensagem completa apenas em desenvolvimento para facilitar debugging if (env.IsDevelopment())
src/Bootstrapper/MeAjudaAi.ApiService/Extensions/ServiceCollectionExtensions.cs
Outdated
Show resolved
Hide resolved
src/Bootstrapper/MeAjudaAi.ApiService/Middleware/InspectAuthMiddleware.cs
Outdated
Show resolved
Hide resolved
…cation, CORS policy, and configuration validation, alongside a new authentication inspection middleware and a query handler for providers.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/Bootstrapper/MeAjudaAi.ApiService/Middleware/InspectAuthMiddleware.cs`:
- Around line 45-79: InspectAuthMiddleware's InvokeAsync currently writes
sensitive auth info into response headers unconditionally; restrict this to
non-production environments by injecting or retrieving the hosting environment
(IWebHostEnvironment or IHostEnvironment) inside
InspectAuthMiddleware/InvokeAsync and gate the context.Response.OnStarting
registration behind an environment check (e.g., only when !env.IsProduction()).
Ensure both the authenticated and unauthenticated branches check the environment
before adding headers so X-Debug-* headers are never emitted in production.
♻️ Duplicate comments (1)
src/Bootstrapper/MeAjudaAi.ApiService/Middleware/InspectAuthMiddleware.cs (1)
22-44: Translate inline comments to Portuguese per repo language policy.Multiple English comments exist throughout the file (lines 25-30, 35-39, 43-44). Per the codebase language policy, comments and XML documentation should be in Portuguese while logs remain in English. Based on learnings, keep comments in Portuguese.
🧹 Nitpick comments (2)
src/Bootstrapper/MeAjudaAi.ApiService/Middleware/InspectAuthMiddleware.cs (2)
11-41: Remove or complete dead code:DebugAuthMiddlewareis unused.This class has injected dependencies (
_logger) that are never used and theInvokeAsyncmethod only calls_next(context)without performing any actual work. The extensive comments suggest this was exploratory code that was superseded byInspectAuthMiddleware.Consider removing this class entirely to avoid confusion and dead code in the codebase.
♻️ Proposed removal
-/// <summary> -/// Middleware temporário para diagnóstico de problemas de autorização. -/// Adiciona headers com roles e permissões do usuário atual. -/// </summary> -public class DebugAuthMiddleware -{ - private readonly RequestDelegate _next; - private readonly ILogger<DebugAuthMiddleware> _logger; - - public DebugAuthMiddleware(RequestDelegate next, ILogger<DebugAuthMiddleware> logger) - { - _next = next; - _logger = logger; - } - - public async Task InvokeAsync(HttpContext context) - { - // Executa o pipeline primeiro (para garantir que autenticação aconteceu) - // Wait, AuthenticationMiddleware runs before this if we register it correctly. - // We want to run logic AFTER Authentication. - - // Pass to next middleware first? No, we want to set headers BEFORE response starts. - // But Identity is set BY AuthenticationMiddleware which must run BEFORE this. - // So: Authentication -> DebugAuthMiddleware -> Authorization -> Endpoint - - await _next(context); - - // Headers só podem ser adicionados se a resposta não começou - // Mas se a autorização falhar (403), o response já pode ter começado? - // Authorization middleware writes 403. - // So we need to use OnStarting. - - // BETTER: Logic inside Invoke, before Next, accessing User. - } -} - -// Simplified approach: Middleware that inspects context.User immediately. -// Assumption: registered after UseAuthentication.
55-75: Consider centralizing debug header names inAuthConstants.Headers.For consistency with the pattern used in
SecurityExtensions.cs(which referencesAuthConstants.Headers.DebugAuthFailure), consider adding these debug header names as constants inAuthConstants.Headers. This aligns with the learnings preference for reusingShared.Contractsfor enums/constants.// In AuthConstants.Headers: public const string DebugUser = "X-Debug-User"; public const string DebugRoles = "X-Debug-Roles"; public const string DebugPermissionsCount = "X-Debug-Permissions-Count"; public const string DebugPermissions = "X-Debug-Permissions"; public const string DebugAuthStatus = "X-Debug-Auth-Status";
src/Bootstrapper/MeAjudaAi.ApiService/Middleware/InspectAuthMiddleware.cs
Outdated
Show resolved
Hide resolved
…onProvider - Register KeycloakPermissionResolver in SecurityExtensions.AddAuthorizationPolicies - Add IConfiguration parameter to AddAuthorizationPolicies method - Update Keycloak realm roles to match expected role names in KeycloakPermissionResolver - Assign all admin roles to admin.portal user in realm configuration - Fix unit tests to pass IConfiguration parameter - Add reimport-realm scripts (.ps1 and .sh) for easy realm reconfiguration - Restrict InspectAuthMiddleware debug headers to non-production environments - Centralize debug header constants in AuthConstants.Headers - Translate comments to Portuguese per repo language policy This fixes the issue where users were getting 403 Forbidden errors because the KeycloakPermissionProvider was not being registered in the DI container, resulting in zero permissions being resolved.
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
infrastructure/keycloak/realms/meajudaai-realm.dev.json (1)
222-252: Avoid committing default user passwords.
Line 224–251 embeds known passwords in the realm export. Even withtemporary: true, this is risky if reused or accidentally promoted. Prefer generating passwords at import time or setting them via a post-import step (from env/secret store), leaving credentials out of the committed JSON.
🤖 Fix all issues with AI agents
In `@infrastructure/keycloak/realms/meajudaai-realm.dev.json`:
- Around line 184-199: The exported realm contains a hardcoded confidential
client secret for the clientId "meajudaai-api-service" (the "MeAjudaAi API
Service Account") — remove the "secret": "meajudaai-api-secret-dev-2024" entry
from the client JSON so no secret is committed; instead generate or set the
client secret at import/runtime via Keycloak admin API or CI secrets. Update the
client block in the realm JSON to omit the "secret" field (leave
serviceAccountsEnabled and protocol unchanged) and document in deployment notes
to provision the secret securely (via admin-cli post-import or environment/CI)
referencing the clientId "meajudaai-api-service".
- Around line 39-88: Create a centralized set of role constants (e.g., a static
class RoleConstants or update RoleNames) that defines every meajudaai-* role
string present in the Keycloak JSON and replace all hardcoded string literals in
KeycloakPermissionResolver (method(s) around the role checks), UserRoles,
RoleNames, and Program authorization policy registrations to reference these
constants; remove legacy role definitions and any policy/usage referencing
"provider-manager" and "document-reviewer" from UserRoles, RoleNames, and
Program so code and Keycloak realm roles are consistent.
In `@infrastructure/keycloak/reimport-realm.ps1`:
- Around line 69-70: The script uses Get-Content to read $REALM_FILE into
$realmContent without verifying the file exists; add a Test-Path check for the
variable REALM_FILE before calling Get-Content and if it does not exist emit a
clear error and exit the script (so that the code path using $realmContent in
this script stops cleanly). Locate the Get-Content usage that assigns
$realmContent and wrap it with a guard using Test-Path $REALM_FILE, calling
Write-Error/Write-Host (or equivalent logger) with a descriptive message and
terminating the script (e.g., exit) when the file is missing.
- Around line 55-61: The catch block handling deletion errors currently logs
non-404 failures and continues; change it to fail fast by emitting an error and
terminating the script: in the catch for the deletion call (where
$_.Exception.Response.StatusCode is checked) replace the Write-Host for non-404
with a Write-Error (including the detailed exception message and status) and
then call exit 1 (or rethrow the exception) so the script stops instead of
proceeding to an import that will likely fail; update the catch around the
delete realm logic to perform this fail-fast behavior.
- Around line 81-88: The script currently prints secrets and passwords via
multiple Write-Host calls (the block printing "Client Secret" and user
passwords); remove those Write-Host lines or guard them behind an explicit
opt-in boolean parameter (e.g. a new $ShowSecrets / -ShowSecrets switch that
defaults to false) and only emit the secret/password output when that parameter
is true; update any call sites or help text to document the new switch so
secrets are never printed by default.
- Around line 17-22: The tokenBody currently hardcodes admin credentials
(username/password/client_id); change it to obtain credentials securely
instead—read username and client_id from environment variables (e.g.,
$env:KEYCLOAK_ADMIN_USER and $env:KEYCLOAK_CLIENT_ID) and read the password via
a secure prompt (Read-Host -AsSecureString converted to plain text only when
needed) or from a protected secret store, and fail early with a clear error if
required values are missing; update the tokenBody construction to use those
variables instead of literal strings.
In `@src/Shared/Authorization/Keycloak/KeycloakPermissionProvider.cs`:
- Around line 31-38: The code calls
keycloakResolver.ResolvePermissionsAsync(...) and then uses permissions.Count
which will NRE if null; update KeycloakPermissionProvider to guard against null
by assigning the result to a local (e.g., permissions = await
keycloakResolver.ResolvePermissionsAsync(...) ?? Enumerable.Empty<Permission>()
or a new List<Permission>() depending on the method's return type), use that
local in logger.LogDebug (MaskUserId(userId) remains), and return the non-null
collection so ResolvePermissionsAsync and logger.LogDebug never observe a null
permissions reference.
- Around line 29-47: The catch block in the method that calls
keycloakResolver.ResolvePermissionsAsync should not swallow cancellation: update
the exception handling in KeycloakPermissionProvider (the method using
ResolvePermissionsAsync and MaskUserId) so that OperationCanceledException (and
derived cancellation exceptions) are rethrown instead of being logged and
returning Array.Empty<EPermission>(); specifically, detect or catch
OperationCanceledException and throw it (or if catching Exception, rethrow when
ex is OperationCanceledException) so cancellation propagates to callers while
still logging or handling non-cancellation exceptions as before.
- Around line 15-16: The Keycloak provider's ModuleName property currently
returns ModuleNames.Users which prevents its cross-module permissions from being
used; change the KeycloakPermissionProvider.ModuleName to indicate it applies to
all modules (e.g., a new ModuleNames.All or empty/wildcard value), then update
the filtering in KeycloakPermissionResolver.GetUserPermissionsByModuleAsync so
it includes providers whose ModuleName equals the requested module OR whose
ModuleName is the all/wildcard value; verify
MapKeycloakRoleToPermissions_AdminRole still maps Keycloak roles to permissions
across Providers, ServiceCatalogs, Locations, etc.
🧹 Nitpick comments (1)
infrastructure/keycloak/reimport-realm.ps1 (1)
4-6: Make script parameters configurable.
Consider reading$KEYCLOAK_URL,$REALM_NAME, and$REALM_FILEfrom env vars or script parameters so the script can be reused across environments without edits.
src/Shared/Authorization/Keycloak/KeycloakPermissionProvider.cs
Outdated
Show resolved
Hide resolved
src/Shared/Authorization/Keycloak/KeycloakPermissionProvider.cs
Outdated
Show resolved
Hide resolved
- Update GetProviderByUserIdQueryHandlerTests to expect 'Erro ao buscar prestador' - Update GetProviderByIdQueryHandlerTests to expect 'Erro ao buscar prestador' - Update GetProvidersByStateQueryHandlerTests to expect 'obrigatório' and 'erro ao recuperar' - Update GetProvidersByVerificationStatusQueryHandlerTests to expect 'erro ao recuperar' - Update GetProvidersByTypeQueryHandlerTests to expect 'erro ao recuperar' - Update GetProvidersByCityQueryHandlerTests to expect 'erro ao recuperar' All tests now pass (12/12 succeeded)
Security Improvements: - Remove hardcoded client secret from meajudaai-realm.dev.json - Remove hardcoded user passwords from realm configuration - Rewrite reimport-realm.ps1 with secure credential handling: * Environment variable support (KEYCLOAK_ADMIN_USER, KEYCLOAK_CLIENT_ID, etc.) * Secure password prompt using Read-Host -AsSecureString * -ShowSecrets switch parameter (default: false) * Fail-fast error handling with proper exit codes * Clear post-import instructions for manual credential setup Code Quality: - Create RoleConstants.cs with centralized Keycloak role definitions - Update KeycloakPermissionResolver to use RoleConstants instead of hardcoded strings - Eliminate magic strings for improved maintainability Authorization Provider Fixes: - Add null guard in KeycloakPermissionProvider (use null-coalescing operator) - Fix cancellation handling: rethrow OperationCanceledException - Update ModuleName property to return wildcard '*' for cross-module permissions - Add XML documentation explaining cross-module permission support All changes improve security posture, code maintainability, and authorization robustness.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@src/Modules/Providers/Tests/Unit/Application/Handlers/Queries/GetProviderByIdQueryHandlerTests.cs`:
- Line 98: The test suite GetProviderByIdQueryHandlerTests and the handler
should use Portuguese for user-facing "not found" messages: update the handler
method (GetProviderByIdQueryHandler.Handle or wherever Error.NotFound("Provider
not found") is returned) to return a Portuguese message such as "Prestador não
encontrado", then update the unit test expectation (the assertion that currently
looks for "Provider not found") to assert a Portuguese substring like "Prestador
não encontrado" so both the code path and tests match localization.
🧹 Nitpick comments (4)
src/Shared/Authorization/Keycloak/KeycloakPermissionResolver.cs (1)
307-371: Consider pre-computing lowercased role constants.Each switch arm calls
ToLowerInvariant()onRoleConstants.*at runtime. While the performance impact is minimal, you could simplify by either:
- Defining the constants in lowercase in
RoleConstants.cs(if Keycloak role names are always lowercase), or- Using
StringComparison.OrdinalIgnoreCasewith a different pattern.Current approach works correctly but involves redundant string allocations on each call.
src/Shared/Utilities/Constants/RoleConstants.cs (1)
1-35: Well-structured centralized role constants.Good approach centralizing Keycloak role strings to avoid duplication and ensure consistency. The XML documentation correctly uses Portuguese per the codebase language policy.
Minor: Inline comments (e.g.,
// System/Admin roles) are in English. Per the established language policy from learnings, comments should be in Portuguese while logs remain in English. Consider translating these section comments.Based on learnings,
Shared.Contractsis the preferred location for shared enums/constants to keep Web aligned with backend. Consider whether this class should be moved there for consistency.src/Shared/Authorization/Keycloak/KeycloakPermissionProvider.cs (2)
2-2: Unused import.
MeAjudaAi.Shared.Utilities.Constantsis imported but no symbols from this namespace are used in this file.🧹 Proposed fix
using MeAjudaAi.Shared.Authorization.Core; -using MeAjudaAi.Shared.Utilities.Constants; using Microsoft.Extensions.Logging;
62-74: DuplicateMaskUserIdimplementation.This method is identical to
KeycloakPermissionResolver.MaskUserId(lines 57-66). Consider extracting it to a shared utility class to avoid duplication and ensure consistent masking behavior.♻️ Suggested approach
Create a shared utility, for example in
MeAjudaAi.Shared.Utilities:public static class PiiMaskingHelper { public static string MaskUserId(string userId) { if (string.IsNullOrWhiteSpace(userId)) return "[EMPTY]"; if (userId.Length <= 6) return $"{userId[0]}***{userId[^1]}"; return $"{userId[..3]}***{userId[^3..]}"; } }Then reference it from both
KeycloakPermissionResolverandKeycloakPermissionProvider.
...odules/Providers/Tests/Unit/Application/Handlers/Queries/GetProviderByIdQueryHandlerTests.cs
Show resolved
Hide resolved
…uery handler for providers.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/Shared/Utilities/PiiMaskingHelper.cs`:
- Around line 13-16: The MaskUserId method currently accepts a non-nullable
string but explicitly handles null/whitespace; change the parameter type from
string to string? in the MaskUserId signature to reflect nullable input, update
any nullable annotations/imports if needed, and fix any call sites that rely on
the previous non-nullable contract (adjusting or adding null checks/casts as
required) so the project compiles cleanly while keeping the existing
null/whitespace handling inside MaskUserId.
In
`@tests/MeAjudaAi.Architecture.Tests/Authorization/PermissionArchitectureTests.cs`:
- Around line 151-156: The assertion currently uses parts[0].All(...) and
parts[1].All(...) which returns true on empty strings, allowing empty or
hyphen-only segments; update the checks in PermissionArchitectureTests to first
ensure parts[0] and parts[1] are non-empty and contain at least one letter
(e.g., ensure parts[i].Length > 0 and parts[i].Any(char.IsLetter)), and also
reject strings that consist only of '-' or that have leading/trailing hyphens if
desired; modify the Assert.True calls referencing parts[0] and parts[1] to
combine these non-empty/has-letter checks with the existing char.IsLower/== '-'
validation so empty or hyphen-only segments fail the test.
tests/MeAjudaAi.Architecture.Tests/Authorization/PermissionArchitectureTests.cs
Outdated
Show resolved
Hide resolved
…or message Updated test assertion to expect 'erro ao recuperar' instead of 'error occurred' to match the Portuguese error messages returned by the handler.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@src/Modules/Providers/Tests/Unit/Application/Handlers/Queries/GetProvidersByIdsQueryHandlerTests.cs`:
- Line 148: Create a centralized constant for the duplicated error text (e.g.,
ValidationMessages.Providers.ErrorRetrievingProviders = "Ocorreu um erro ao
recuperar os prestadores"), then update all five query handlers
(GetProvidersByIdsQueryHandler, GetProvidersByCityQueryHandler,
GetProvidersByStateQueryHandler, GetProvidersByTypeQueryHandler,
GetProvidersByVerificationStatusQueryHandler) to use that constant when
producing the error message, and update the corresponding unit tests (e.g.,
GetProvidersByIdsQueryHandlerTests.cs and the other four tests) to assert
against ValidationMessages.Providers.ErrorRetrievingProviders instead of the
hard-coded string.
...ules/Providers/Tests/Unit/Application/Handlers/Queries/GetProvidersByIdsQueryHandlerTests.cs
Outdated
Show resolved
Hide resolved
…a new validation messages constant file.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@tests/MeAjudaAi.Shared.Tests/Unit/Authorization/PermissionTests.cs`:
- Line 127: The test AllPermissions_ShouldHaveValidModuleNames fails because
GetModule() on the new LocationsRead and LocationsManage permissions yields
"locations" but the validModules array declared in PermissionTests (var
validModules = new[] { "system", "users", "providers", "orders", "reports",
"admin", "service-catalogs" };) does not include "locations"; fix by adding
"locations" to that validModules array so it contains "locations" alongside the
other module names.
🧹 Nitpick comments (3)
src/Modules/Providers/Application/Handlers/Queries/GetProvidersByStateQueryHandler.cs (1)
24-24: Consider centralizing this validation message as well.For consistency with the error message on line 35 and the broader refactoring effort, consider adding a constant like
ValidationMessages.Providers.StateParameterRequiredand using it here instead of the inline string.♻️ Suggested approach
In
src/Contracts/Utilities/Constants/ValidationMessages.cs:public static class Providers { public const string ErrorRetrievingProviders = "Ocorreu um erro ao recuperar os prestadores"; + public const string StateParameterRequired = "O parâmetro de estado é obrigatório"; }Then in this file:
- return Result<IReadOnlyList<ProviderDto>>.Failure("O parâmetro de estado é obrigatório"); + return Result<IReadOnlyList<ProviderDto>>.Failure(ValidationMessages.Providers.StateParameterRequired);src/Modules/Providers/Tests/Unit/Application/Handlers/Queries/GetProvidersByStateQueryHandlerTests.cs (1)
68-68: Consider using a centralized constant for the "required" validation message.These assertions use partial string matching (
.Contain("obrigatório")), while line 156 uses the exact constantValidationMessages.Providers.ErrorRetrievingProviders. For consistency, consider adding a constant for the state required message (e.g.,ValidationMessages.Providers.StateRequired) and asserting against it directly.This would improve maintainability if the message wording changes and align with the centralization approach used elsewhere in this PR. Based on learnings, prefer reuse of Contracts constants.
Also applies to: 87-87, 106-106
src/Contracts/Utilities/Constants/ValidationMessages.cs (1)
96-103: Approve the new Providers class; minor punctuation inconsistency.The new
Providersclass follows the established pattern and the Portuguese message is correct per the language policy. However,ErrorRetrievingProviderslacks a trailing period while other messages in this file consistently end with one (e.g.,"O email é obrigatório.").Optional: Add trailing period for consistency
public static class Providers { - public const string ErrorRetrievingProviders = "Ocorreu um erro ao recuperar os prestadores"; + public const string ErrorRetrievingProviders = "Ocorreu um erro ao recuperar os prestadores."; }
tests/MeAjudaAi.Shared.Tests/Unit/Authorization/PermissionTests.cs
Outdated
Show resolved
Hide resolved
… tests and validation messages, and add `PermissionTests` for authorization.
…oviding a configurable environment with mock services and dedicated test databases.
…agnostic integration tests.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@tests/MeAjudaAi.Shared.Tests/TestInfrastructure/Handlers/BaseTestAuthenticationHandler.cs`:
- Around line 68-73: Replace the two new English inline comments above the
permission claims in BaseTestAuthenticationHandler.cs with Portuguese versions:
change “Add all admin permissions for ServiceCatalogs” to “Adicionar todas as
permissões administrativas para Catálogos de Serviço” and change “Add all admin
permissions for Locations” to “Adicionar todas as permissões administrativas
para Localizações”; update the comments immediately preceding the claims.Add
calls that use AuthConstants.Claims.Permission and
EPermission.ServiceCatalogs*/EPermission.Locations* so the wording matches the
repo policy.
tests/MeAjudaAi.Shared.Tests/TestInfrastructure/Handlers/BaseTestAuthenticationHandler.cs
Outdated
Show resolved
Hide resolved
…t API endpoints, along with a new admin dashboard page.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Modules/Providers/API/Endpoints/ProviderAdmin/GetProvidersByCityEndpoint.cs (1)
21-63:⚠️ Potential issue | 🟡 MinorUpdate remarks: endpoint is no longer “consulta pública.”
WithRequirePermission(EPermission.ProvidersRead), the XML remarks should reflect authorized access.✍️ Suggested update
-/// filtrados por cidade específica. Utiliza arquitetura CQRS e permite -/// consulta pública para facilitar descoberta de serviços. +/// filtrados por cidade específica. Utiliza arquitetura CQRS e permite +/// consulta autorizada para facilitar descoberta de serviços.
🤖 Fix all issues with AI agents
In
`@src/Modules/Providers/API/Endpoints/ProviderAdmin/GetProvidersByVerificationStatusEndpoint.cs`:
- Around line 105-109: Translate the inline comment following the throw in the
catch block that handles OperationCanceledException inside
GetProvidersByVerificationStatus to Portuguese, keeping the code behavior
unchanged and preserving the single-line comment syntax (//); leave the
logger.LogInformation message in English as-is and only replace the comment text
"Propagate cancellation to pipeline" with an appropriate Portuguese translation
(e.g., "Propagar cancelamento para o pipeline") next to the throw statement.
In
`@src/Modules/Providers/API/Endpoints/ProviderServices/AddServiceToProviderEndpoint.cs`:
- Line 48: The authorization change removed the context-aware SelfOrAdmin policy
and replaced it with a role-only permission check, blocking regular providers
from updating their own provider record; revert or adjust this by restoring the
context-aware policy usage on the endpoint (use
.RequireAuthorization("SelfOrAdmin") instead of
.RequirePermission(EPermission.ProvidersUpdate)) or, if you prefer
permission-based control, update the role-to-permission mapping so the Provider
role is granted EPermission.ProvidersUpdate; locate the endpoint configuration
where .RequirePermission(EPermission.ProvidersUpdate) is applied
(AddServiceToProviderEndpoint) and either swap it back to
.RequireAuthorization("SelfOrAdmin") or add EPermission.ProvidersUpdate to the
Provider role in the authorization/role mapping.
🧹 Nitpick comments (2)
src/Modules/Providers/API/Endpoints/ProviderAdmin/UpdateProviderProfileEndpoint.cs (1)
54-54: Documentation may be inconsistent with new permission model.Line 54 states "🔒 Controle de acesso: próprio prestador ou administrador" (self or admin), but the endpoint now requires
EPermission.ProvidersUpdate. If providers aren't granted this permission by default, the description should be updated to reflect the actual access control.Consider updating the description to match the permission-based model, e.g., "🔒 Controle de acesso: usuários com permissão ProvidersUpdate".
📝 Suggested description update
- - 🔒 Controle de acesso: próprio prestador ou administrador + - 🔒 Controle de acesso: usuários com permissão ProvidersUpdateAlso applies to: 68-68
src/Modules/Providers/API/Endpoints/ProviderAdmin/RemoveDocumentEndpoint.cs (1)
52-52: Documentation inconsistency with permission model.Similar to UpdateProviderProfileEndpoint, line 52 states "🔒 Controle de acesso: próprio prestador ou administrador" but the endpoint now requires
EPermission.ProvidersUpdate. Consider updating the description to reflect the permission-based access control.📝 Suggested description update
- - 🔒 Controle de acesso: próprio prestador ou administrador + - 🔒 Controle de acesso: usuários com permissão ProvidersUpdateAlso applies to: 74-74
src/Modules/Providers/API/Endpoints/ProviderAdmin/GetProvidersByVerificationStatusEndpoint.cs
Outdated
Show resolved
Hide resolved
src/Modules/Providers/API/Endpoints/ProviderServices/AddServiceToProviderEndpoint.cs
Outdated
Show resolved
Hide resolved
…t endpoints, a development data seeder, a provider DTO, and an admin UI page for providers.
…agement, and core infrastructure components.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
src/Modules/SearchProviders/Application/ModuleApi/SearchProvidersModuleApi.cs (1)
66-71:⚠️ Potential issue | 🟠 MajorCancellation exceptions are now swallowed and logged as errors.
The removal of the explicit
OperationCanceledExceptioncatch block changes the behavior:
- Cancellation is no longer propagated — callers cannot distinguish between a genuine availability failure and a cancelled operation.
- Logged as Error instead of Debug — cancellations will pollute error logs, making real issues harder to spot.
- Cooperative cancellation semantics broken — during graceful shutdown or request timeouts, the caller should see
OperationCanceledException, notfalse.Consider restoring the explicit handling so cancellations propagate correctly.
🔧 Proposed fix to restore OperationCanceledException handling
return testResult.IsSuccess; } - + catch (OperationCanceledException) + { + logger.LogDebug("SearchProviders availability check was cancelled"); + throw; + } catch (Exception ex) { logger.LogError(ex, "Error checking SearchProviders module availability"); return false; }src/Modules/Users/Application/ModuleApi/UsersModuleApi.cs (2)
74-79:⚠️ Potential issue | 🟡 MinorCancellation logged at Error level after removing explicit handler.
With
OperationCanceledExceptionno longer caught explicitly, cancellation during shutdown or timeout will be logged viaLogError(line 77). Cancellation is expected control flow, not an error, and logging it at Error level pollutes error metrics and complicates diagnostics.Consider filtering cancellation from error logging:
🛠️ Proposed fix
- catch (Exception ex) + catch (OperationCanceledException) + { + logger.LogDebug("Users module availability check was cancelled"); + return false; + } + catch (Exception ex) { logger.LogError(ex, "Error checking Users module availability"); return false; }
112-117:⚠️ Potential issue | 🟡 MinorSame cancellation concern applies here.
OperationCanceledExceptionwill now be logged as "Basic operations test failed" (Warning level), which is misleading—the test was cancelled, not failed. For consistency with the suggested fix above and clearer diagnostics:🛠️ Proposed fix
- catch (Exception ex) + catch (OperationCanceledException) + { + logger.LogDebug("Basic operations test was cancelled for Users module"); + return false; + } + catch (Exception ex) { logger.LogWarning(ex, "Basic operations test failed for Users module"); return false; }src/Modules/Providers/Application/ModuleApi/ProvidersModuleApi.cs (1)
82-87:⚠️ Potential issue | 🟠 MajorSwallowing
OperationCanceledExceptionmasks cancellation semantics.The removal of dedicated
OperationCanceledExceptionhandling causes cancellation to be logged as an error and returnfalseinstead of propagating. This can interfere with graceful shutdown, request timeouts, and caller expectations — they won't know the operation was cancelled vs. actually failed.Consider restoring explicit handling or filtering it from the generic catch:
🛡️ Proposed fix to propagate cancellation
logger.LogDebug("Providers module is available and healthy"); return true; } - - catch (Exception ex) + catch (OperationCanceledException) + { + throw; // Let cancellation propagate + } + catch (Exception ex) { logger.LogError(ex, "Error checking Providers module availability"); return false; }Apply the same pattern to
CanExecuteBasicOperationsAsync:return false; } - - catch (Exception ex) + catch (OperationCanceledException) + { + throw; + } + catch (Exception ex) { logger.LogWarning(ex, "Basic operations test failed for Providers module"); return false; }Also applies to: 110-115
src/Modules/Documents/Application/Handlers/UploadDocumentCommandHandler.cs (1)
136-141:⚠️ Potential issue | 🟡 MinorTranslate user-facing error message to Portuguese.
The error message on line 140 should be in Portuguese per the codebase language policy. User-facing messages and API responses shown to the frontend must be in Portuguese, while logs should remain in English (line 139 is correct).
- throw new InvalidOperationException("Failed to upload document. Please try again later.", ex); + throw new InvalidOperationException("Falha ao enviar documento. Por favor, tente novamente mais tarde.", ex);src/Modules/Documents/Application/ModuleApi/DocumentsModuleApi.cs (2)
151-166:⚠️ Potential issue | 🟡 MinorTranslate XML documentation to Portuguese.
These XML docs are in English; policy requires Portuguese for comments and XML summaries.
✍️ Proposed translation
- /// <summary> - /// Gets the status of a document. - /// </summary> - /// <param name="documentId">Document ID</param> - /// <param name="cancellationToken">Cancellation token</param> - /// <returns>Document status DTO or null if not found</returns> - /// <remarks> - /// <para><strong>UpdatedAt Semantics:</strong></para> - /// <para>Uses VerifiedAt ?? UploadedAt where VerifiedAt represents the timestamp of the last - /// status change (verification or rejection). The domain model sets VerifiedAt when documents - /// are verified OR rejected. For documents still in Uploaded/PendingVerification status, - /// falls back to UploadedAt.</para> - /// <para><strong>Note:</strong> RejectedAt is NOT used in the fallback chain because the domain - /// already populates VerifiedAt for rejected documents, making VerifiedAt the authoritative - /// timestamp for all terminal states (Verified/Rejected).</para> - /// </remarks> + /// <summary> + /// Obtém o status de um documento. + /// </summary> + /// <param name="documentId">ID do documento</param> + /// <param name="cancellationToken">Token de cancelamento</param> + /// <returns>DTO de status do documento ou null se não encontrado</returns> + /// <remarks> + /// <para><strong>Semântica de UpdatedAt:</strong></para> + /// <para>Usa VerifiedAt ?? UploadedAt, onde VerifiedAt representa o timestamp da última mudança + /// de status (verificação ou rejeição). O modelo de domínio define VerifiedAt quando documentos + /// são verificados OU rejeitados. Para documentos ainda em Uploaded/PendingVerification, + /// usa UploadedAt como fallback.</para> + /// <para><strong>Nota:</strong> RejectedAt NÃO é usado na cadeia de fallback porque o domínio + /// já popula VerifiedAt para documentos rejeitados, tornando VerifiedAt o timestamp + /// autoritativo para todos os estados terminais (Verified/Rejected).</para> + /// </remarks>Based on learnings: "Adopt a language policy across the codebase: 1) All logs must be in English, 2) Comments and XML summaries (///,
, , etc.) must be in Portuguese, 3) User-facing validation messages and API responses shown to the frontend must be in Portuguese."
45-83:⚠️ Potential issue | 🟠 MajorPreserve cancellation semantics instead of treating cancellation as failure.
All methods accepting
cancellationTokencatch genericExceptionwithout distinguishingOperationCanceledException. When the token is canceled, the exception is logged as an error and converted to failure/false, which masks cancellation semantics and inflates error telemetry. Each method should explicitly handle cancellation and either rethrow or return without error logging.🛠️ Suggested pattern (apply to all methods with a cancellationToken)
try { logger.LogDebug("Checking Documents module availability"); ... } + catch (OperationCanceledException) when (cancellationToken.IsCancellationRequested) + { + logger.LogDebug("Documents module availability check canceled"); + throw; + } catch (Exception ex) { logger.LogError(ex, "Error checking Documents module availability"); return false; }Applies to: IsAvailableAsync, CanExecuteBasicOperationsAsync, GetDocumentByIdAsync, GetProviderDocumentsAsync, GetDocumentStatusAsync, HasVerifiedDocumentsAsync, HasRequiredDocumentsAsync, GetDocumentStatusCountAsync, HasPendingDocumentsAsync, HasRejectedDocumentsAsync
🤖 Fix all issues with AI agents
In `@src/Shared/Exceptions/GlobalExceptionHandler.cs`:
- Around line 17-20: The unwrap logic in GlobalExceptionHandler that checks for
wrappers (InvalidOperationException,
System.Reflection.TargetInvocationException, AggregateException) currently only
unwraps to a set of exceptions that excludes NotFoundException, causing wrapped
404s to be treated as 500s; update the conditional that inspects
exception.InnerException (in the unwrap branch inside GlobalExceptionHandler) to
also include NotFoundException in the list (alongside ValidationException,
BadRequestException, UnprocessableEntityException, ArgumentException,
DomainException) so wrapped NotFoundException instances are unwrapped and
handled with their correct 404 response.
🧹 Nitpick comments (4)
.editorconfig (1)
12-31: Consider scoping the global suppressions or documenting rationale per rule.Disabling many Sonar rules in the global
*.cssection can mask real issues across the entire codebase. If these are needed only for specific areas (e.g., migrations, generated, or legacy code), scope them to those paths or add explicit justification comments for each rule.src/Modules/Providers/Application/ModuleApi/ProvidersModuleApi.cs (1)
140-151: Avoid double enumeration ofIEnumerable.
providerIds.Count()on line 142 andToList()on line 144 each enumerate the input. If the caller passes a deferred sequence (e.g., a LINQ query), it will be evaluated twice.♻️ Suggested refactor
public async Task<Result<IReadOnlyList<ModuleProviderBasicDto>>> GetProvidersBasicInfoAsync(IEnumerable<Guid> providerIds, CancellationToken cancellationToken = default) { - logger.LogDebug("Getting basic provider info for {Count} provider IDs", providerIds.Count()); - - var result = await getProvidersByIdsHandler.HandleAsync(new GetProvidersByIdsQuery(providerIds.ToList()), cancellationToken); + var idList = providerIds.ToList(); + logger.LogDebug("Getting basic provider info for {Count} provider IDs", idList.Count); + + var result = await getProvidersByIdsHandler.HandleAsync(new GetProvidersByIdsQuery(idList), cancellationToken);src/Shared/Seeding/DevelopmentDataSeeder.cs (1)
151-154: Comment should be in Portuguese per project language policy.The log message is correctly in English, but the inline comment should be in Portuguese for consistency with the rest of the file. Based on learnings, comments and XML documentation should be in Portuguese while logs must be in English.
Proposed fix
- // Always seed providers (uses ON CONFLICT DO NOTHING) + // Sempre semeia providers (usa ON CONFLICT DO NOTHING)src/Modules/Locations/Application/Handlers/UpdateAllowedCityHandler.cs (1)
25-39: Consider usingValidationMessagesconstants for consistency.The error messages are hardcoded, while the endpoint maps errors by status code to
ValidationMessages.Locations.AllowedCityNotFoundandValidationMessages.Locations.DuplicateCity. Using the constants directly in the handler would improve maintainability if these messages are ever logged or inspected.♻️ Suggested change
if (allowedCity == null) { - return Result.Failure(Error.NotFound("Cidade permitida não encontrada")); + return Result.Failure(Error.NotFound(ValidationMessages.Locations.AllowedCityNotFound)); } // ... if (existing is not null && existing.Id != command.Id) { - return Result.Failure(Error.Conflict($"Cidade '{command.CityName}-{command.StateSigla}' já cadastrada")); + return Result.Failure(Error.Conflict(ValidationMessages.Locations.DuplicateCity)); }Add the import:
using static MeAjudaAi.Modules.Locations.ValidationMessages;
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…d Providers, and add administrative UI for managing allowed cities.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/Modules/Documents/Application/ModuleApi/DocumentsModuleApi.cs (2)
207-248:⚠️ Potential issue | 🟡 MinorTranslate remaining English comments/XML docs to Portuguese.
The PERFORMANCE NOTE block and helper method summaries (plus thecancellationTokenparam description) are still in English—please localize them to Portuguese to match the repository language policy.Based on learnings, adopt a language policy across the codebase: 1) All logs must be in English, 2) Comments and XML summaries (///,
, , etc.) must be in Portuguese, 3) User-facing validation messages and API responses shown to the frontend must be in Portuguese.
Also applies to: 275-279
119-137:⚠️ Potential issue | 🟠 MajorAdd OperationCanceledException handling to Result-returning methods.
The genericcatch (Exception)swallowsOperationCanceledException, converting cancellations into failure Results and error logs rather than propagating the cancellation. This prevents upstream cancellation from short-circuiting and masks request timeouts. Your codebase already demonstrates the correct pattern inIsAvailableAsyncandCanExecuteBasicOperationsAsync. Apply this pattern to all Result-returning methods: add a filteredcatch (OperationCanceledException) when (cancellationToken.IsCancellationRequested) { throw; }before the generic catch.Methods requiring this fix:
GetDocumentByIdAsync,GetProviderDocumentsAsync,GetDocumentStatusAsync,HasVerifiedDocumentsAsync,HasRequiredDocumentsAsync,GetDocumentStatusCountAsync,HasPendingDocumentsAsync, andHasRejectedDocumentsAsync.Pattern (already implemented in IsAvailableAsync, line 77-81)
- catch (Exception ex) + catch (OperationCanceledException) when (cancellationToken.IsCancellationRequested) + { + throw; + } + catch (Exception ex) { logger.LogError(ex, "..."); return Result<T>.Failure("..."); }
🤖 Fix all issues with AI agents
In `@src/Modules/Documents/Application/Handlers/UploadDocumentCommandHandler.cs`:
- Around line 136-141: The catch-all in UploadDocumentCommandHandler's method is
wrapping intentional UnauthorizedAccessException and ArgumentException into
InvalidOperationException; change the error handling so known exceptions are not
swallowed—either narrow the try scope to only the risky calls so the explicitly
thrown UnauthorizedAccessException and ArgumentException occur outside the try,
or in the catch (Exception ex) check and rethrow when ex is
UnauthorizedAccessException or ArgumentException (e.g., throw;), logging only
unexpected exceptions and preserving original messages and types for
GlobalExceptionHandler to map to proper HTTP statuses. Ensure references: the
catch (Exception ex) block in UploadDocumentCommandHandler and the places where
UnauthorizedAccessException/ArgumentException are thrown remain intact.
In `@src/Shared/Seeding/DevelopmentDataSeeder.cs`:
- Around line 440-441: The comment "We use simple SQL injection for development
seeder brevity" is misleading; update the comment in the Insert Document
(Primary) section of DevelopmentDataSeeder (the seeding code that uses EF Core
parameter placeholders like {0}, {1}) to say you are using simple/raw SQL or EF
Core parameterized/raw SQL for brevity and that EF Core parameters are safer and
prevent SQL injection; keep the rest of the logic unchanged and ensure the
comment accurately reflects that the code uses parameterized queries rather than
describing a vulnerability.
- Around line 384-409: The third provider entry uses Guid.NewGuid() for Id and
UserId which breaks idempotent seeding; add two stable GUID constants (e.g.,
private static readonly Guid Provider3UserId and Provider3Id) near the other
provider constants in the DevelopmentDataSeeder class and replace the
Guid.NewGuid() occurrences in the third provider object with those constants so
subsequent seed runs use the same user_id and id values.
🧹 Nitpick comments (1)
src/Web/MeAjudaAi.Web.Admin/Components/Dialogs/EditAllowedCityDialog.razor (1)
35-57: Consider making the latitude/longitude row responsive.Without wrapping, the two fields can overflow on narrow screens. Add a responsive flex class or wrap behavior.
Proposed tweak
- <div class="d-flex gap-4 mb-3"> + <div class="d-flex flex-column flex-sm-row gap-4 mb-3">
src/Modules/Documents/Application/Handlers/UploadDocumentCommandHandler.cs
Show resolved
Hide resolved
…der, and an admin dialog for editing allowed cities.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/Shared/Seeding/DevelopmentDataSeeder.cs`:
- Around line 24-30: The seeder currently generates new document IDs each run
causing duplicate rows; change the seeding to use stable, pre-defined GUIDs
(e.g., Provider1Id, Provider2Id, Provider3Id and their corresponding
Provider1UserId/Provider2UserId/Provider3UserId) when creating provider
documents instead of calling UuidGenerator.NewId(), and ensure the insert/update
logic uses those stable IDs so the ON CONFLICT (provider_id, id) DO NOTHING
behavior becomes idempotent; apply the same fix to the other seeding block
referenced (the entries around the later provider seeding, the section noted as
lines 445-452).
- Around line 442-444: The comment block above the SQL insert in
DevelopmentDataSeeder.cs is in English; translate the three English comment
lines ("Insert Document (Primary)", "Note: We use parameterized SQL with EF Core
to ensure safety and prevent SQL injection.", and "provider_id and id are
composite primary key") into Portuguese and replace the existing comments in
that location so comments and XML summaries follow the project's Portuguese
policy while leaving code and identifiers unchanged.
🧹 Nitpick comments (1)
src/Modules/Locations/Application/Handlers/UpdateAllowedCityHandler.cs (1)
51-55: Consider documenting the coordinate threshold constant.The
0.0001threshold for detecting "missing" coordinates (~11 meters at the equator) is reasonable for Brazilian cities, but extracting it to a named constant would improve readability and maintainability.💡 Optional: Extract threshold to a named constant
+private const double CoordinateZeroThreshold = 0.0001; + public async Task<Result> HandleAsync(UpdateAllowedCityCommand command, CancellationToken cancellationToken = default) { // ... - var existingCoordsMissing = Math.Abs(allowedCity.Latitude) < 0.0001 && Math.Abs(allowedCity.Longitude) < 0.0001; - var commandCoordsMissing = Math.Abs(lat) < 0.0001 && Math.Abs(lon) < 0.0001; + var existingCoordsMissing = Math.Abs(allowedCity.Latitude) < CoordinateZeroThreshold && Math.Abs(allowedCity.Longitude) < CoordinateZeroThreshold; + var commandCoordsMissing = Math.Abs(lat) < CoordinateZeroThreshold && Math.Abs(lon) < CoordinateZeroThreshold;
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/Shared/Seeding/DevelopmentDataSeeder.cs`:
- Around line 424-425: Translate the inline English comment "// Insert Provider"
to Portuguese per repo policy; locate the comment immediately above the
ExecuteSqlRawAsync call in DevelopmentDataSeeder.cs and replace it with a
Portuguese comment such as "// Inserir Provedor" (or another appropriate
Portuguese term for "provider") ensuring the rest of the code is unchanged.
…evelopment and test data, including categories, locations, and providers.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/Modules/Documents/Application/ModuleApi/DocumentsModuleApi.cs`:
- Around line 133-141: There are two identical filtered catch blocks for
OperationCanceledException in DocumentsModuleApi.cs which makes the second
unreachable; remove the duplicate so only a single "catch
(OperationCanceledException) when (cancellationToken.IsCancellationRequested)"
remains and ensures it rethrows to propagate cancellation to the caller
(preserve the existing throw; behavior), keeping only one filtered catch in the
method.
🧹 Nitpick comments (1)
src/Modules/Documents/Tests/Unit/Application/UploadDocumentCommandHandlerTests.cs (1)
576-576: Prefer a shared constant for the localized error text.Hard-coding the full Portuguese message makes the test brittle if localization changes. If a shared constant (e.g.,
ValidationMessages) exists for this message, assert against that instead.
src/Modules/Documents/Application/ModuleApi/DocumentsModuleApi.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…nit tests for document upload functionality.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/Modules/Documents/Application/ModuleApi/DocumentsModuleApi.cs`:
- Around line 133-136: No bloco catch que captura OperationCanceledException em
DocumentsModuleApi.cs (condição cancellationToken.IsCancellationRequested),
substitua o termo em inglês "caller" no comentário por uma palavra em português
— por exemplo, altere "// Propagar cancelamento para o caller" para "// Propagar
cancelamento para o chamador" (ou "// Propagar cancelamento para o invocador")
para manter o comentário totalmente em Português.
…h a development data seeder and module provider DTO.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@src/Shared/Seeding/DevelopmentDataSeeder.cs`:
- Around line 450-459: The seed inserts into providers.document using
ExecuteSqlRawAsync but never uses the per-provider DocumentId fields
(provider.DocumentId) and the comment incorrectly claims the composite PK is
(provider_id, id) while the ON CONFLICT uses (provider_id, document_type);
either remove the unused DocumentId properties and all assignments (e.g.,
Provider.*.DocumentId) and update the comment to reflect the actual composite
key (provider_id, document_type), or include provider.DocumentId in the
INSERT/VALUES and change the ON CONFLICT clause/comment to match the true
schema; locate the ExecuteSqlRawAsync call, the provider.DocumentId assignments,
and the surrounding comment in DevelopmentDataSeeder.cs to apply the chosen fix.
In `@src/Web/MeAjudaAi.Web.Admin/Pages/Dashboard.razor`:
- Line 237: The inline comment "// TEMPORARY: Charts disabled for debugging" in
Dashboard.razor is misleading because charts are processed and displayed; remove
or update that comment to reflect the actual behavior (e.g., delete the line or
change to "Charts enabled — debugging comment removed") so future maintainers
aren't confused; locate the comment text in the Dashboard.razor component and
update it accordingly.
- Around line 296-299: The branch that handles numeric type keys parses typeKey
and returns ProviderType.ToDisplayName(typeValue) but doesn't guard against a
null result; update the numeric branch (the int.TryParse block that uses typeKey
and typeValue and calls ProviderType.ToDisplayName) to return a non-null
fallback by applying the same null-coalescing fallback used elsewhere (e.g., ??
"Desconhecido") so unknown/ unmapped integer values yield "Desconhecido" instead
of null.
🧹 Nitpick comments (6)
src/Web/MeAjudaAi.Web.Admin/Pages/Documents.razor (1)
37-37: Localization changes look good, but inconsistent terminology remains.The updates from "Provider" to "Provedor" are correct per the Portuguese user-facing text policy. However, the file still contains "Provider" in other locations:
- Line 30:
"Documentos por Provider"- Line 52:
"Provider ID:"- Line 119:
"Nenhum documento encontrado para este provider"Consider updating these for consistency across the page.
Also applies to: 45-45, 133-133
src/Web/MeAjudaAi.Web.Admin/Pages/Providers.razor (4)
118-118: Move@injectdirective to the top with other injections.This injection is placed between the markup and
@codeblock, while other@injectdirectives are grouped at lines 9-13. Relocating it improves readability and follows Blazor conventions.♻️ Suggested relocation
Move to the directive block at the top:
`@inject` ILogger<Providers> Logger `@inject` IPermissionService PermissionService +@inject NavigationManager Navigation `@implements` IDisposableThen remove line 118.
128-140: UseQueryHelpers.ParseQueryfor robust query string parsing.Manual parsing doesn't handle URL-encoded values or edge cases reliably. The built-in utility provides safer parsing.
♻️ Suggested fix using QueryHelpers
Add the using directive at the top:
`@using` Microsoft.AspNetCore.WebUtilitiesThen replace the manual parsing:
- var uri = Navigation.ToAbsoluteUri(Navigation.Uri); - var query = uri.Query.TrimStart('?'); - - if (!string.IsNullOrEmpty(query)) - { - var queryParams = query.Split('&'); - if (queryParams.Any(q => q.Equals("filter=pending", StringComparison.OrdinalIgnoreCase))) - { - _showOnlyPending = true; - } - } + var uri = Navigation.ToAbsoluteUri(Navigation.Uri); + var queryParams = QueryHelpers.ParseQuery(uri.Query); + + if (queryParams.TryGetValue("filter", out var filterValue) && + filterValue.ToString().Equals("pending", StringComparison.OrdinalIgnoreCase)) + { + _showOnlyPending = true; + }
147-155: Consider subscribing to state changes instead of filtering on every render.
UpdateFilteredProviders()is called on every re-render after the first. While memoization reduces the cost, this still incurs overhead on each render cycle. Consider using Fluxor'sSubscribeToActionor reacting toProvidersStatechanges more selectively.
200-208: Cache theRowStyleFuncdelegate to avoid unnecessary re-renders.The expression-bodied property creates a new delegate instance on each access, which may cause MudDataGrid to detect a "changed" parameter and re-render unnecessarily.
♻️ Suggested fix: use a readonly field or method
Option 1: Readonly field (preferred)
private bool _showOnlyPending = false; +private readonly Func<ModuleProviderDto, int, string> _rowStyleFunc = (x, _) => + x.VerificationStatus == VerificationStatus.Pending.ToString() + ? "background-color: var(--mud-palette-warning-hover); font-weight: 500;" + : "";Then in the markup:
-RowStyleFunc="@RowStyleFunc" +RowStyleFunc="@_rowStyleFunc"Option 2: Private method
-private Func<ModuleProviderDto, int, string> RowStyleFunc => (x, i) => -{ - if (x.VerificationStatus == VerificationStatus.Pending.ToString()) - return "background-color: var(--mud-palette-warning-hover); font-weight: 500;"; - - return ""; -}; +private string GetRowStyle(ModuleProviderDto provider, int index) => + provider.VerificationStatus == VerificationStatus.Pending.ToString() + ? "background-color: var(--mud-palette-warning-hover); font-weight: 500;" + : "";Then use
RowStyleFunc="@GetRowStyle"in the markup.src/Web/MeAjudaAi.Web.Admin/Pages/Dashboard.razor (1)
265-270: Consider usingProviderTypeOrderIntsfor consistent semantic ordering.
ProviderTypeOrderIntsis defined at line 183-186 but unused. The status chart usesStatusOrderIntsfor semantic ordering while types use alphabetical sort, creating inconsistency.Optional: Apply same ordering pattern as status chart
// Provider Type Chart Data var typeGroups = providers .Where(p => !string.IsNullOrEmpty(p.ProviderType)) .GroupBy(p => p.ProviderType) - .OrderBy(g => g.Key) // Simple alphabetical sort for now, or assume string matches Enum names + .OrderBy(g => { + if (int.TryParse(g.Key, out int typeValue)) + { + var index = Array.IndexOf(ProviderTypeOrderInts, typeValue); + return index >= 0 ? index : int.MaxValue; + } + return int.MaxValue; + }) .ToList();
…ts management, including new documentation and development data seeding.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
docs/roadmap.md (2)
4004-4008:⚠️ Potential issue | 🟡 MinorFix the corrupted heading glyph.
The replacement character breaks the heading rendering.✏️ Suggested change
-#### � Resultado Esperado Sprint 8 +#### ✅ Resultado Esperado Sprint 8
4043-4064:⚠️ Potential issue | 🟡 MinorClean up the migrations code block (contains unrelated prose).
The code sample is currently broken and will render incorrectly.✏️ Suggested change
```csharp private static void EnsureDatabaseMigrations(WebApplication app) { - Keycloak client automation script (setup em 1 comando) - **DAY 1** -- ✅ 0 analyzer warnings no Admin Portal (S2094, S2953, S2933, MUD0002 resolvidos) -- ✅ 30-40 testes bUnit (10 → 30+, +200% cobertura) - -**Timeline**: -- **Dia 1** (17 Jan): Keycloak automation script - **CRITICAL PATH** -- **Semana 1** (17-24 Jan): Customer App Home + Busca + Warnings fix -- **Semana 2** (24-31 Jan): Customer App Perfil + Mobile + Testes -- **Semana 3** (31 Jan): PolishingVariable("APPLY_MIGRATIONS"); + var applyMigrations = Environment.GetEnvironmentVariable("APPLY_MIGRATIONS"); if (!string.IsNullOrEmpty(applyMigrations) && bool.TryParse(applyMigrations, out var shouldApply) && !shouldApply) { logger?.LogInformation("Migrações automáticas desabilitadas via APPLY_MIGRATIONS=false"); return; } // Aplicar migrations normalmente context.Database.Migrate(); }</details> </blockquote></details> </blockquote></details>🤖 Fix all issues with AI agents
In `@docs/admin-portal/architecture.md`: - Around line 39-48: Update the code fence in the architecture.md feature-tree to specify a language (e.g., replace the opening ``` with ```text) so markdownlint passes and rendering is clearer; apply this to the block that shows the tree containing ProvidersState.cs, ProvidersActions.cs, ProvidersReducers.cs, ProvidersEffects.cs, and ProvidersSelectors.cs so the tree is treated as plain text by the renderer. In `@docs/admin-portal/overview.md`: - Around line 62-99: The fenced directory-tree code block lacks a language tag; update the opening fence for the block that begins with "src/Web/MeAjudaAi.Web.Admin/" from ``` to ```text so the directory tree is marked as plain text (satisfying MD040); keep the block content unchanged and only modify the opening fence.🧹 Nitpick comments (3)
src/Web/MeAjudaAi.Web.Admin/Pages/Dashboard.razor (2)
110-116: @key bindings will force chart re-creation on every data update.Using the array reference as
@keymeans a new key on everyUpdateChartData()call, causing full chart destruction and re-creation. If this is intentional for forcing MudBlazor chart re-renders, it's fine. If you want smoother updates, consider using a version counter or stable identifier instead.Also applies to: 140-146
183-186:ProviderTypeOrderIntsappears unused.The array at lines 183-186 is defined but never referenced—the type grouping at line 268 uses alphabetical sorting instead. Consider either:
- Removing the unused array, or
- Applying semantic ordering similar to status (using
ProviderTypeOrderInts) if a specific order is desired.Option: Remove unused array
private static readonly int[] StatusOrderInts = { (int)VerificationStatus.Pending, (int)VerificationStatus.Verified, (int)VerificationStatus.Rejected }; - private static readonly int[] ProviderTypeOrderInts = { - (int)ProviderType.Individual, - (int)ProviderType.Company - };Also applies to: 264-269
src/Web/MeAjudaAi.Web.Admin/Pages/Providers.razor (1)
124-139: Consider adding a using directive for cleaner code.The query parameter parsing logic is correct. For improved readability, consider adding a
@using Microsoft.AspNetCore.WebUtilitiesdirective at the top of the file instead of using the fully qualified name inline.♻️ Optional refactor
Add at the top with other using statements:
`@using` Microsoft.AspNetCore.WebUtilitiesThen simplify the parsing:
- var queryParams = Microsoft.AspNetCore.WebUtilities.QueryHelpers.ParseQuery(uri.Query); + var queryParams = QueryHelpers.ParseQuery(uri.Query);
… and add a new Providers page.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@Directory.Packages.props`:
- Line 15: The PackageVersion entry for HtmlSanitizer pins an invalid NuGet
version ("9.0.890"); update the <PackageVersion Include="HtmlSanitizer"
Version="9.0.890" /> entry to a released version (e.g., Version="9.0.892" or
Version="9.0.889") so package restore succeeds—locate the PackageVersion element
with Include="HtmlSanitizer" and change the Version attribute accordingly.
In `@docs/admin-portal/architecture.md`:
- Around line 162-164: The example uses a Portuguese operation label in the call
to _errorHandler.HandleApiError ("carregar provedores"), which could flow into
logs/telemetry; change that label to an English operation description (e.g.,
"load providers") to comply with the English-only logging policy, or if it's
intended as a user-facing message instead of a log label, clarify that in the
example and leave user-facing text documented as such; update the string passed
to _errorHandler.HandleApiError and any related calls (also review the value
propagated into _snackbar.Add and LoadProvidersFailureAction) to ensure
consistency with the chosen intent.
In `@docs/admin-portal/overview.md`:
- Around line 19-22: Update the Frontend docs entry that currently reads
"MudBlazor 8.0" to the actual pinned version "MudBlazor 8.15.0" so it matches
the project's Directory.Packages.props; edit the "Frontend" section in
overview.md and replace the MudBlazor version string accordingly.
🧹 Nitpick comments (1)
docs/admin-portal/overview.md (1)
88-92: Clarify how constants align with Shared.Contracts to avoid duplication.The
Constants/section reads like module-specific constants live in Web.Admin. If these are shared across backend/UI, consider pointing to Shared.Contracts (or add a note that only UI-specific constants live here).Based on learnings: “For frigini/MeAjudaAi Web reviews, prefer commit-by-commit (newest-first) with concise verdicts and concrete follow-ups; prioritize reuse of Shared.Contracts for enums/constants to keep Web aligned with backend/shared code.”
…ncluding overview and architecture, and introduce new package lock files.
📊 Code Coverage ReportCoverage: 90.25% (extracted from Cobertura (Aggregated Direct)) 📈 Coverage Details
📋 Coverage Analysis
🎯 Quality Gates
📁 Artifacts
This comment is updated automatically on each push to track coverage trends. |
1 participant
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Documentation
Tests