Skip to content

refactor: enhance email service#62

Merged
foxminchan merged 1 commit into
mainfrom
chore/organize-code
Jun 16, 2025
Merged

refactor: enhance email service#62
foxminchan merged 1 commit into
mainfrom
chore/organize-code

Conversation

@foxminchan

@foxminchan foxminchan commented Jun 16, 2025 •
edited
Loading

Copy link
Copy Markdown
Owner

Pull Request Description

This pull request refactors components of the email service and related infrastructure while aligning configuration and logging behavior across tests, state management, and exception handling.

  • Replaces EmailOptions with MailKitSettings in tests for improved consistency with production email configuration.
  • Introduces GlobalLogBuffer dependency and flush calls in ConversationState, MessageBuffer, ChatStreaming, and exception handlers to enhance logging reliability.
  • Updates namespaces and minor configuration settings across various Aspire projects, including adjustments in health check and testing extensions.

Checklist

  • My code follows the DDD principles
  • I've used the latest C# features and idioms
  • I've added/updated unit tests for business logic
  • Service boundaries are maintained (no cross-service direct dependencies)
  • Repository pattern is used for data access
  • Domain Events are used for cross-service communication
  • I've registered new services in AppHost project (if applicable)
  • I've updated both entities and DTOs (if applicable)

Summary by CodeRabbit

  • New Features

    • Introduced enhanced MailKit email sender with improved configuration, health checks, and telemetry integration.
    • Added environment-aware email outbox service for flexible email delivery in different environments.
    • Added ChatContext service to unify conversation state and cancellation management.
    • Improved error handling by explicitly flushing log buffers on exceptions across various components.

  • Refactor

    • Replaced legacy email sender and configuration types (EmailOptions, SendGridOptions, JobOptions) with new settings classes (MailKitSettings, SendGridSettings, JobSettings).
    • Updated namespaces and class names for improved clarity and organization.
    • Streamlined service registration and configuration for email and job services.
    • Refactored chat streaming to use primary constructor syntax and consolidated dependencies.
    • Added detailed XML documentation to Azure infrastructure provisioning and local development extension classes.

  • Bug Fixes

    • Enhanced reliability of email and job workers by ensuring logs are flushed immediately upon errors.
    • Fixed cancellation token usage in email cleanup worker.

  • Tests

    • Updated unit tests to align with new configuration types and dependency injection changes.
    • Removed obsolete tests related to deprecated configuration classes.
    • Simplified and cleaned up test data fakers and related helpers.

  • Chores

    • Removed unused package references and global using directives to reduce project dependencies and improve maintainability.
    • Deleted legacy email sender implementations, related factories, and extensions.

Copilot AI review requested due to automatic review settings June 16, 2025 11:37
@netlify

netlify Bot commented Jun 16, 2025 •
edited
Loading

Copy link
Copy Markdown

✅ Deploy Preview for bookwormdev canceled.

Name Link
🔨 Latest commit 5406e19
🔍 Latest deploy log https://app.netlify.com/projects/bookwormdev/deploys/68501780be26530008fc6f53

@coderabbitai

coderabbitai Bot commented Jun 16, 2025 •
edited
Loading

Copy link
Copy Markdown

Walkthrough

This update restructures and modernizes the notification service's email infrastructure. It replaces the legacy EmailOptions and related domain settings with new MailKitSettings and SendGridSettings classes, introducing improved configuration, validation, and dependency injection patterns. The MailKit and SendGrid sender implementations are refactored, with new factories, health checks, and extension methods for streamlined service registration and telemetry. The outbox sender is renamed and its registration updated. Several obsolete files and tests are removed, and all usages of the old settings are migrated to the new pattern. Logging is enhanced by explicitly flushing buffers on error paths.

Changes

Files / Groups Change Summary
Directory.Packages.props, .../BookWorm.Notification.csproj Removed AspNetCore.HealthChecks.Network package references.
src/Aspire/BookWorm.AppHost/Extensions/** Updated namespaces for extension classes (AI, Infrastructure, Network, Security, Testing); minor logic changes in K6Extensions.
src/Aspire/BookWorm.AppHost/GlobalUsings.cs Added global usings for new extension namespaces.
src/Aspire/BookWorm.ServiceDefaults/Extensions.cs Added blank lines for readability; no logic changes.
src/BuildingBlocks/BookWorm.Chassis/Exceptions/** Exception handlers updated to accept log buffer dependencies and flush logs on error.
src/Services/Chat/BookWorm.Chat/Infrastructure/ChatStreaming/ChatStreaming.cs Refactored to primary constructor syntax; injected GlobalLogBuffer and flush logs on streaming errors.
src/Services/Chat/BookWorm.Chat/Infrastructure/ConversationState/** Added GlobalLogBuffer injection; flush logs on errors in message buffering and Redis conversation state.
src/Services/Chat/BookWorm.Chat/Infrastructure/ChatStreaming/ChatContext.cs Added new ChatContext class encapsulating conversation state and cancellation management.
src/Services/Chat/BookWorm.Chat/Infrastructure/ChatStreaming/Extensions.cs Registered ChatContext singleton; simplified IMcpClient service registration.
src/Services/Notification/BookWorm.Notification.UnitTests/Consumers/** Tests updated to use MailKitSettings instead of EmailOptions; adjusted DI registrations.
src/Services/Notification/BookWorm.Notification.UnitTests/Domain/** Deleted tests for EmailOptions, JobOptions, and SendGridOptions.
src/Services/Notification/BookWorm.Notification.UnitTests/Fakers/FakerExtensions.cs Removed generators for invalid/empty config values; simplified and refactored remaining methods.
src/Services/Notification/BookWorm.Notification.UnitTests/Fakers/TestDataFakers.cs Removed unused fakers, switched to MailKitSettings; updated argument order in CancelOrderCommand faker.
src/Services/Notification/BookWorm.Notification.UnitTests/Workers/** Worker tests updated to inject and mock GlobalLogBuffer.
src/Services/Notification/BookWorm.Notification/Domain/Builders/OrderMimeMessageBuilder.cs Changed parameter from EmailOptions to MailKitSettings for sender info.
src/Services/Notification/BookWorm.Notification/Domain/Settings/EmailOptions.cs Deleted: Old email configuration class removed.
src/Services/Notification/BookWorm.Notification/Extensions/Extensions.cs Updated service registration: switched to new MailKit/SendGrid registration methods and outbox registration.
src/Services/Notification/BookWorm.Notification/GlobalUsings.cs Removed global usings for deleted/renamed namespaces.
src/Services/Notification/BookWorm.Notification/Infrastructure/Job/Extensions.cs Switched job configuration from JobOptions to JobSettings.
src/Services/Notification/BookWorm.Notification/Infrastructure/Job/JobSettings.cs Renamed from JobOptions, moved namespace, adjusted visibility.
src/Services/Notification/BookWorm.Notification/Infrastructure/Senders/Extensions/** Deleted: Old sender registration and endpoint extensions removed.
src/Services/Notification/BookWorm.Notification/Infrastructure/Senders/Factories/** Deleted: Old SMTP client factory and object pool policy removed.
src/Services/Notification/BookWorm.Notification/Infrastructure/Senders/MailKit/** New: Introduced MailKitSettings, MailKitClientFactory, MailKitSender, health check, and registration extensions.
src/Services/Notification/BookWorm.Notification/Infrastructure/Senders/Outbox/EmailOutboxService.cs Renamed from OutboxSender, updated namespace and references.
src/Services/Notification/BookWorm.Notification/Infrastructure/Senders/Outbox/OutboxExtensions.cs New: Added extension to register the email outbox service.
src/Services/Notification/BookWorm.Notification/Infrastructure/Senders/Providers/MailKitSender.cs Deleted: Old MailKit sender implementation removed.
src/Services/Notification/BookWorm.Notification/Infrastructure/Senders/SendGrid/SendGridExtensions.cs New: Added SendGrid client registration extension.
src/Services/Notification/BookWorm.Notification/Infrastructure/Senders/SendGrid/SendGridSender.cs Updated: Switched to SendGridSettings, simplified logic, added log flushing on error.
src/Services/Notification/BookWorm.Notification/Infrastructure/Senders/SendGrid/SendGridSettings.cs Renamed from SendGridOptions, moved namespace, adjusted visibility.
src/Services/Notification/BookWorm.Notification/Infrastructure/Senders/TelemetryTags.cs Deleted: Telemetry tag constants removed.
src/Services/Notification/BookWorm.Notification/IntegrationEvents/EventHandlers/** Updated handlers to use MailKitSettings instead of EmailOptions.
src/Services/Notification/BookWorm.Notification/Workers/CleanUpSentEmailWorker.cs Added GlobalLogBuffer dependency; flush logs on error; added cancellation token to delete call.
src/Services/Notification/BookWorm.Notification/Workers/ResendErrorEmailWorker.cs Updated to use MailKitSettings, added GlobalLogBuffer dependency, flush logs on errors.

Sequence Diagram(s)

sequenceDiagram
    participant App as IHostApplicationBuilder
    participant DI as Dependency Injection
    participant MailKitExt as MailKitExtensions
    participant MailKitFactory as MailKitClientFactory
    participant MailKitSender as MailKitSender
    participant Smtp as SMTP Server

    App->>MailKitExt: AddMailKitClient(connectionName)
    MailKitExt->>DI: Register MailKitSettings, MailKitClientFactory, MailKitSender
    MailKitSender->>MailKitFactory: GetSmtpClientAsync()
    MailKitFactory->>Smtp: Connect & Authenticate
    MailKitSender->>Smtp: SendAsync(MimeMessage)
    Note right of MailKitSender: On error, log and flush buffer
Loading 
sequenceDiagram
    participant App as IHostApplicationBuilder
    participant DI as Dependency Injection
    participant OutboxExt as OutboxExtensions
    participant Outbox as EmailOutboxService
    participant Sender as ISender (MailKit/SendGrid)

    App->>OutboxExt: AddEmailOutbox()
    OutboxExt->>DI: Register EmailOutboxService as ISender
    Outbox->>Sender: SendAsync(MimeMessage)
    Sender->>...: (MailKitSender or SendGridSender logic)
Loading

Poem

In the meadow of code, where the emails do hop,
The old options have vanished—no more need to mop!
With MailKit and SendGrid, our messages fly,
Through factories and health checks, they leap to the sky.
Logs now flush quickly when errors appear,
A rabbit’s delight—our code’s crystal clear!
🐇💌

✨ Finishing Touches
  • 📝 Generate Docstrings
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Explain this complex logic.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai explain this code block.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and explain its main purpose.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copilot AI reviewed Jun 16, 2025
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull Request Overview

This pull request refactors components of the email service and related infrastructure while aligning configuration and logging behavior across tests, state management, and exception handling.

  • Replaces EmailOptions with MailKitSettings in tests for improved consistency with production email configuration.
  • Introduces GlobalLogBuffer dependency and flush calls in ConversationState, MessageBuffer, ChatStreaming, and exception handlers to enhance logging reliability.
  • Updates namespaces and minor configuration settings across various Aspire projects, including adjustments in health check and testing extensions.

Reviewed Changes

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

Show a summary per file
File Description
src/Services/Notification/BookWorm.Notification.UnitTests/Consumers/CancelOrderConsumerTests.cs Replaces EmailOptions with MailKitSettings and updates DI registration.
src/Services/Chat/BookWorm.Chat/Infrastructure/ConversationState/RedisConversationState.cs Adds GlobalLogBuffer dependency and flushes log buffer on dispose and error handling.
src/Services/Chat/BookWorm.Chat/Infrastructure/ConversationState/MessageBuffer.cs Introduces GlobalLogBuffer and adjusts the return statement in CoalesceFragments.
src/Services/Chat/BookWorm.Chat/Infrastructure/ChatStreaming/ChatStreaming.cs Incorporates GlobalLogBuffer with flush calls after processing to streamline logging.
src/BuildingBlocks/BookWorm.Chassis/Exceptions/* Passes a log buffer to various exception handlers and flushes after logging errors.
src/Aspire/BookWorm.AppHost/Extensions/* Updates namespace organization and minor adjustments in health checks and testing extensions.
Directory.Packages.props Removes the AspNetCore.HealthChecks.Network package.
Comments suppressed due to low confidence (3)

src/Services/Notification/BookWorm.Notification.UnitTests/Consumers/CancelOrderConsumerTests.cs:41

  • Ensure that replacing EmailOptions with MailKitSettings in the test setup is consistent with the production configuration of the email service.
_mailKitSettings = new() { From = "[email protected]" };

Directory.Packages.props:73

  • Confirm that the removal of the AspNetCore.HealthChecks.Network package does not adversely affect any health check functionality within the services.
<PackageVersion Include="AspNetCore.HealthChecks.Network" Version="$(HealthChecksVersion)" />

src/Aspire/BookWorm.AppHost/Extensions/Testing/K6Extensions.cs:45

  • [nitpick] Verify that using the string literal "true" for the K6WebDashboard environment variable meets downstream requirements, replacing the previous ToString() conversion.
.WithEnvironment(K6WebDashboard, "true")

Comment thread src/Services/Chat/BookWorm.Chat/Infrastructure/ConversationState/MessageBuffer.cs
@foxminchan foxminchan self-assigned this Jun 16, 2025
coderabbitai[bot]
coderabbitai Bot reviewed Jun 16, 2025
View reviewed changes

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 19

🔭 Outside diff range comments (1)
src/Services/Notification/BookWorm.Notification/Infrastructure/Senders/Outbox/EmailOutboxService.cs (1)

19-24: Gracefully handle plain-text emails

mailMessage.HtmlBody can be null when only a TextBody is supplied. Persisting null in the outbox will later break HTML renderers in the admin portal.

-    mailMessage.HtmlBody
+    mailMessage.HtmlBody ?? mailMessage.TextBody

Optionally persist both fields so that downstream consumers can decide which flavour to display.

♻️ Duplicate comments (2)
src/Aspire/BookWorm.AppHost/Extensions/Infrastructure/AzureExtensions.Provision.cs (1)

99-115: Same tag duplication comment applies here
See earlier note; consider centralising tag logic for the PostgreSQL path as well.

src/Services/Notification/BookWorm.Notification/Infrastructure/Senders/SendGrid/SendGridSender.cs (1)

37-47: Preserve cancellation semantics as in MailKitSender

Same concern as above – wrap OperationCanceledException separately to avoid masking cancellations.

- if (response.StatusCode is not (HttpStatusCode.OK or HttpStatusCode.Accepted))
+ if (response.StatusCode is not (HttpStatusCode.OK or HttpStatusCode.Accepted))

(Apply the same try/catch refactor suggested for MailKitSender.)

🧹 Nitpick comments (31)
src/Aspire/BookWorm.ServiceDefaults/Extensions.cs (1)

145-145: Unnecessary extra blank lines introduced

The added blank lines break the existing one-blank-line separation style used throughout the file. Consider reverting to a single blank line after each variable block to keep formatting consistent.

Also applies to: 154-154

src/Services/Notification/BookWorm.Notification/Infrastructure/Job/Extensions.cs (1)

42-44: Variable naming drift

jobOptions no longer reflects the type; consider renaming to jobSettings for clarity and to match the class rename.

src/Services/Notification/BookWorm.Notification/Infrastructure/Job/JobSettings.cs (1)

10-23: Possible missing using System.ComponentModel.DataAnnotations

Attributes such as [Required] and [RegularExpression] are defined in System.ComponentModel.DataAnnotations.
If a global-using isn’t already present, this file will fail to compile.

src/Aspire/BookWorm.AppHost/Extensions/Infrastructure/AzureExtensions.Provision.cs (1)

50-55: Tag-adding logic is duplicated across resources

Identical Environment / Projects tags are set in every provisioning block.
Extract a small helper (e.g., AddStandardTags(this ITaggable res, IDistributedApplicationBuilder app)) to keep these sections DRY and lower maintenance overhead.

src/Aspire/BookWorm.AppHost/Extensions/Network/EndpointExtensions.cs (1)

11-15: Doc-comment sentence is truncated

/// This ensures that ends abruptly. Complete or remove to avoid stale documentation.

src/Aspire/BookWorm.AppHost/Extensions/Testing/K6Extensions.cs (1)

45-45: Explicit "true" string is fine—consider constant reuse.

Hard-coding "true" is clearer than true.ToString(), but you might expose a small helper/constant to avoid magic strings if the flag is reused elsewhere.

src/Services/Notification/BookWorm.Notification/Workers/CleanUpSentEmailWorker.cs (1)

8-10: Prefer injecting via an abstraction rather than the concrete GlobalLogBuffer

GlobalLogBuffer is being injected directly. If a lightweight façade (e.g. ILogBuffer) were introduced you could:

  • keep production code tied to the concrete buffer,
  • but still mock the interface in tests,
  • and avoid leaking a 3rd-party type through your public surface.

This minor change improves decoupling and future-proofs the class should the underlying buffering implementation change.

src/Services/Notification/BookWorm.Notification/IntegrationEvents/EventHandlers/CancelOrderCommandHandler.cs (1)

30-33: Minor: avoid the using alias for a single type

The alias

using MailKitSettings = ...

adds indirection with little benefit—MailKitSettings is already an unambiguous name across the solution. Dropping the alias reduces noise in stack traces and IDE “go to definition”.

src/Services/Notification/BookWorm.Notification/Domain/Builders/OrderMimeMessageBuilder.cs (1)

29-32: Validate settings before constructing the From address

mailKitSettings.Name / From are used directly; if configuration is missing the resulting MimeMessage contains an empty address and the SMTP transaction will fail late. Quick guard:

 public OrderMimeMessageBuilder WithFrom(MailKitSettings mailKitSettings)
 {
-    From = new(mailKitSettings.Name, mailKitSettings.From);
+    if (string.IsNullOrWhiteSpace(mailKitSettings.From))
+        throw new ArgumentException("Sender address is not configured", nameof(mailKitSettings));
+
+    From = new(mailKitSettings.Name, mailKitSettings.From);
     return this;
 }

Failing fast here produces a clear exception and surfaces misconfiguration during testing rather than at runtime.

src/Services/Notification/BookWorm.Notification.UnitTests/Workers/CleanUpSentEmailWorkerTests.cs (1)

23-24: Add an assertion that logBuffer.Flush() is invoked on failure

The new dependency is mocked but never verified. A simple expectation strengthens the test and protects the behaviour introduced in the PR:

logBufferMock
    .Verify(x => x.Flush(), Times.Once);

Place it in GivenException_WhenCleaningUp_ThenShouldLogError.

Also applies to: 39-40

src/BuildingBlocks/BookWorm.Chassis/Exceptions/ValidationExceptionHandler.cs (1)

10-13: Inject abstraction, not concrete buffer

PerRequestLogBuffer is a concrete type. Depending directly on it couples the handler to the Microsoft-specific implementation and complicates testing/mocking. If possible, depend on the ILogBuffer (or similar abstraction exposed by Microsoft.Extensions.Diagnostics.Buffering) to respect the Dependency-Inversion Principle.

src/Services/Notification/BookWorm.Notification/IntegrationEvents/EventHandlers/PlaceOrderCommandHandler.cs (1)

1-1: Remove aliasing—prefer ordinary using

The file-level alias MailKitSettings = …MailKitSettings; adds unnecessary cognitive load; the fully-qualified name is already concise. Unless you alias multiple conflicting types, drop the alias for clarity.

src/Services/Notification/BookWorm.Notification/IntegrationEvents/EventHandlers/CompleteOrderCommandHandler.cs (1)

1-1: Same aliasing concern as in PlaceOrderCommandHandler

Drop the using-alias unless a real name clash exists.

src/BuildingBlocks/BookWorm.Chassis/Exceptions/NotFoundExceptionHandler.cs (1)

21-24: Depend on abstraction rather than concrete buffer

Same concern as in ValidationExceptionHandler—inject an interface if available.

src/BuildingBlocks/BookWorm.Chassis/Exceptions/GlobalExceptionHandler.cs (1)

9-12: Inject abstraction for easier testing

GlobalLogBuffer is concrete; prefer an abstraction to decouple.

src/Services/Notification/BookWorm.Notification/Infrastructure/Senders/Outbox/EmailOutboxService.cs (1)

5-5: Prefer static readonly for immutable constants

_partitionKey never changes after construction and is identical for every instance.
Making it static readonly avoids recomputing the same string for each service instance and communicates intent.

-private readonly string _partitionKey = nameof(Domain.Models.Outbox).ToLowerInvariant();
+private static readonly string _partitionKey =
+    nameof(Domain.Models.Outbox).ToLowerInvariant();
src/Services/Notification/BookWorm.Notification.UnitTests/Workers/ResendErrorEmailWorkerTests.cs (1)

43-48: Mock GlobalLogBuffer more realistically

GlobalLogBuffer.Flush() is invoked inside the worker.
Consider configuring the mock to verify that Flush() is called at least once in the failure paths to keep the test meaningful.

logBufferMock.Setup(b => b.Flush());
src/Services/Chat/BookWorm.Chat/Infrastructure/ChatStreaming/ChatStreaming.cs (1)

164-165: Flush success path too?

You flush the global log buffer on both exception branches but not after the normal completion of streaming.
If the intention is to guarantee log persistence after every streamed message, add a flush after SaveAssistantMessageToDatabase succeeds.

await SaveAssistantMessageToDatabase(...);
_logBuffer.Flush();

Ignore if log batching is preferred for the happy path.

src/Services/Notification/BookWorm.Notification.UnitTests/Consumers/CancelOrderConsumerTests.cs (1)

41-42: Minor: include required fields in settings

MailKitSettings usually requires host, port and credentials. Supplying only From may hide mis-configuration in tests.
Populate minimal valid values or use a faker to keep the tests realistic.

src/Services/Notification/BookWorm.Notification/Workers/ResendErrorEmailWorker.cs (1)

10-12: Prefer the options pattern for settings injection

Injecting MailKitSettings as a concrete type works, but wiring it through IOptions<MailKitSettings> (or IOptionsSnapshot<>) provides consistency with the rest of the ASP .NET configuration story and enables validation & reload-on-change for free.

-public sealed class ResendErrorEmailWorker(
-    ILogger<ResendErrorEmailWorker> logger,
-    MailKitSettings mailKitSettings,
+public sealed class ResendErrorEmailWorker(
+    ILogger<ResendErrorEmailWorker> logger,
+    IOptions<MailKitSettings> mailKitSettings,
src/Services/Notification/BookWorm.Notification.UnitTests/Consumers/PlaceOrderConsumerTests.cs (1)

33-45: Nit: rename test faker for clarity

TestDataFakers.EmailOptions now returns MailKitSettings, which can confuse future readers.

-public static readonly Faker<MailKitSettings> EmailOptions = new Faker<MailKitSettings>()
+public static readonly Faker<MailKitSettings> MailKitSettings = new Faker<MailKitSettings>()
src/Services/Notification/BookWorm.Notification/Infrastructure/Senders/MailKit/MailKitHealthCheck.cs (1)

3-19: Health-check implementation solid, tiny enhancement possible

Fetching an SMTP client is enough for a basic probe, but you could improve granularity by returning Degraded when the client is retrieved but not connected/authenticated (check client.IsConnected). Not blocking the PR.

src/Services/Notification/BookWorm.Notification/Infrastructure/Senders/Outbox/OutboxExtensions.cs (1)

21-30: Underlying senders must be registered before the outbox

AddEmailOutbox resolves MailKitSender / SendGridSender directly. If neither AddMailKitClient nor AddSendGridClient has been invoked earlier, the container build will fail at runtime.

Document the ordering or defensively call the required registrations here to make the extension self-contained.

src/Services/Notification/BookWorm.Notification/Infrastructure/Senders/MailKit/MailKitClientFactory.cs (1)

8-12: Implement IAsyncDisposable or call DisconnectAsync()

SmtpClient.Dispose() does not send QUIT; prefer DisconnectAsync(true) to gracefully close the connection. Providing IAsyncDisposable enables callers to await it during shutdown.

src/Services/Notification/BookWorm.Notification.UnitTests/Fakers/TestDataFakers.cs (1)

9-11: Rename variable to match type

EmailOptions still appears as the field name although the type is MailKitSettings, which is misleading.

-public static readonly Faker<MailKitSettings> EmailOptions = …
+public static readonly Faker<MailKitSettings> MailKitSettingsFaker = …
src/Services/Notification/BookWorm.Notification/Infrastructure/Senders/MailKit/MailKitSettings.cs (2)

27-39: Error messages reference the value, not the key

The interpolated string uses {connectionString} inside the “ConnectionStrings:” placeholder, yielding confusing output (it prints the whole connection string). Capture the key name separately or adjust wording.

-It should be provided in 'ConnectionStrings:{connectionString}'
+It should be provided in the 'ConnectionStrings' section or the
+'{ConfigurationSection}:Endpoint' key

59-67: Use Uri.TryCreate result variable

You already stored uri above; re-using the out var hides potential misuse. Minor readability fix.

- if (Uri.TryCreate(endpoint.ToString(), UriKind.Absolute, out uri) is false)
+ if (Uri.TryCreate(endpoint.ToString(), UriKind.Absolute, out var parsed) is false)
  …
- Endpoint = uri;
+ Endpoint = parsed;
src/Services/Notification/BookWorm.Notification/Infrastructure/Senders/MailKit/MailKitExtensions.cs (1)

61-84: Repeated AddOpenTelemetry() may override prior configuration

Chaining AddOpenTelemetry() twice can drop earlier tracing/metric configuration set by other libraries. Prefer configuring both tracing and metrics in one builder or using ConfigureOpenTelemetry pattern.

src/Services/Notification/BookWorm.Notification.UnitTests/Fakers/FakerExtensions.cs (1)

15-28: Consider null-forgiving operator removal

null! suppresses nullable warnings but hides potential regressions. For tests, explicitly pass null and annotate constructor parameter as nullable where appropriate.

src/Services/Notification/BookWorm.Notification/Infrastructure/Senders/SendGrid/SendGridSettings.cs (2)

11-13: [Key] attribute is misleading on a settings DTO

[Key] is an Entity Framework/Data-Annotations hint for primary keys; this class is never persisted.
Keeping it here conveys the wrong semantics and adds an unnecessary dependency on EF‐Core packages.

-[Key]
 public string ApiKey { get; set; } = string.Empty;

15-20: Prefer the built-in [EmailAddress] attribute to a custom regex

[EmailAddress] from System.ComponentModel.DataAnnotations already validates most RFC-compliant addresses and is
maintained by the BCL. The custom regex is harder to read, duplicate maintenance burden, and could reject future-valid
addresses (e.g., new single-character TLDs).

-[RegularExpression(
-    @"^[a-zA-Z0-9._%+-]+@(?!.*\.\.)[a-zA-Z0-9.-]+\.[a-zA-Z]{2,}$",
-    ErrorMessage = "Invalid email format"
-)]
+[EmailAddress]
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 8618443 and 752648d.

📒 Files selected for processing (59)
  • Directory.Packages.props (0 hunks)
  • src/Aspire/BookWorm.AppHost/Extensions/AI/ModelExtensions.cs (1 hunks)
  • src/Aspire/BookWorm.AppHost/Extensions/Infrastructure/AzureExtensions.Provision.cs (5 hunks)
  • src/Aspire/BookWorm.AppHost/Extensions/Infrastructure/AzureExtensions.Run.cs (1 hunks)
  • src/Aspire/BookWorm.AppHost/Extensions/Infrastructure/DashboardExtensions.cs (1 hunks)
  • src/Aspire/BookWorm.AppHost/Extensions/Infrastructure/RedisResourceExtensions.cs (1 hunks)
  • src/Aspire/BookWorm.AppHost/Extensions/Network/EmailExtensions.cs (1 hunks)
  • src/Aspire/BookWorm.AppHost/Extensions/Network/EndpointExtensions.cs (1 hunks)
  • src/Aspire/BookWorm.AppHost/Extensions/Network/LaunchProfileExtensions.cs (1 hunks)
  • src/Aspire/BookWorm.AppHost/Extensions/Security/KeycloakExtensions.cs (1 hunks)
  • src/Aspire/BookWorm.AppHost/Extensions/Testing/K6Extensions.cs (2 hunks)
  • src/Aspire/BookWorm.AppHost/GlobalUsings.cs (1 hunks)
  • src/Aspire/BookWorm.ServiceDefaults/Extensions.cs (2 hunks)
  • src/BuildingBlocks/BookWorm.Chassis/Exceptions/GlobalExceptionHandler.cs (2 hunks)
  • src/BuildingBlocks/BookWorm.Chassis/Exceptions/NotFoundExceptionHandler.cs (3 hunks)
  • src/BuildingBlocks/BookWorm.Chassis/Exceptions/ValidationExceptionHandler.cs (2 hunks)
  • src/Services/Chat/BookWorm.Chat/Infrastructure/ChatStreaming/ChatStreaming.cs (6 hunks)
  • src/Services/Chat/BookWorm.Chat/Infrastructure/ConversationState/MessageBuffer.cs (5 hunks)
  • src/Services/Chat/BookWorm.Chat/Infrastructure/ConversationState/RedisConversationState.cs (6 hunks)
  • src/Services/Notification/BookWorm.Notification.UnitTests/Consumers/CancelOrderConsumerTests.cs (6 hunks)
  • src/Services/Notification/BookWorm.Notification.UnitTests/Consumers/CompleteOrderConsumerTests.cs (7 hunks)
  • src/Services/Notification/BookWorm.Notification.UnitTests/Consumers/PlaceOrderConsumerTests.cs (6 hunks)
  • src/Services/Notification/BookWorm.Notification.UnitTests/Domain/EmailOptionsTests.cs (0 hunks)
  • src/Services/Notification/BookWorm.Notification.UnitTests/Domain/JobOptionsTests.cs (0 hunks)
  • src/Services/Notification/BookWorm.Notification.UnitTests/Domain/SendGridOptionsTests.cs (0 hunks)
  • src/Services/Notification/BookWorm.Notification.UnitTests/Fakers/FakerExtensions.cs (1 hunks)
  • src/Services/Notification/BookWorm.Notification.UnitTests/Fakers/TestDataFakers.cs (1 hunks)
  • src/Services/Notification/BookWorm.Notification.UnitTests/Workers/CleanUpSentEmailWorkerTests.cs (3 hunks)
  • src/Services/Notification/BookWorm.Notification.UnitTests/Workers/ResendErrorEmailWorkerTests.cs (3 hunks)
  • src/Services/Notification/BookWorm.Notification/BookWorm.Notification.csproj (0 hunks)
  • src/Services/Notification/BookWorm.Notification/Domain/Builders/OrderMimeMessageBuilder.cs (2 hunks)
  • src/Services/Notification/BookWorm.Notification/Domain/Settings/EmailOptions.cs (0 hunks)
  • src/Services/Notification/BookWorm.Notification/Extensions/Extensions.cs (2 hunks)
  • src/Services/Notification/BookWorm.Notification/GlobalUsings.cs (0 hunks)
  • src/Services/Notification/BookWorm.Notification/Infrastructure/Job/Extensions.cs (1 hunks)
  • src/Services/Notification/BookWorm.Notification/Infrastructure/Job/JobSettings.cs (1 hunks)
  • src/Services/Notification/BookWorm.Notification/Infrastructure/Senders/Extensions/Extensions.cs (0 hunks)
  • src/Services/Notification/BookWorm.Notification/Infrastructure/Senders/Extensions/MailPitEndpointExtensions.cs (0 hunks)
  • src/Services/Notification/BookWorm.Notification/Infrastructure/Senders/Extensions/SenderWithTelemetryExtensions.cs (0 hunks)
  • src/Services/Notification/BookWorm.Notification/Infrastructure/Senders/Factories/ISmtpClientFactory.cs (0 hunks)
  • src/Services/Notification/BookWorm.Notification/Infrastructure/Senders/Factories/SmtpClientFactory.cs (0 hunks)
  • src/Services/Notification/BookWorm.Notification/Infrastructure/Senders/Factories/SmtpClientFactoryObjectPoolPolicy.cs (0 hunks)
  • src/Services/Notification/BookWorm.Notification/Infrastructure/Senders/MailKit/MailKitClientFactory.cs (1 hunks)
  • src/Services/Notification/BookWorm.Notification/Infrastructure/Senders/MailKit/MailKitExtensions.cs (1 hunks)
  • src/Services/Notification/BookWorm.Notification/Infrastructure/Senders/MailKit/MailKitHealthCheck.cs (1 hunks)
  • src/Services/Notification/BookWorm.Notification/Infrastructure/Senders/MailKit/MailKitSender.cs (1 hunks)
  • src/Services/Notification/BookWorm.Notification/Infrastructure/Senders/MailKit/MailKitSettings.cs (1 hunks)
  • src/Services/Notification/BookWorm.Notification/Infrastructure/Senders/Outbox/EmailOutboxService.cs (2 hunks)
  • src/Services/Notification/BookWorm.Notification/Infrastructure/Senders/Outbox/OutboxExtensions.cs (1 hunks)
  • src/Services/Notification/BookWorm.Notification/Infrastructure/Senders/Providers/MailKitSender.cs (0 hunks)
  • src/Services/Notification/BookWorm.Notification/Infrastructure/Senders/SendGrid/SendGridExtensions.cs (1 hunks)
  • src/Services/Notification/BookWorm.Notification/Infrastructure/Senders/SendGrid/SendGridSender.cs (2 hunks)
  • src/Services/Notification/BookWorm.Notification/Infrastructure/Senders/SendGrid/SendGridSettings.cs (1 hunks)
  • src/Services/Notification/BookWorm.Notification/Infrastructure/Senders/TelemetryTags.cs (0 hunks)
  • src/Services/Notification/BookWorm.Notification/IntegrationEvents/EventHandlers/CancelOrderCommandHandler.cs (2 hunks)
  • src/Services/Notification/BookWorm.Notification/IntegrationEvents/EventHandlers/CompleteOrderCommandHandler.cs (2 hunks)
  • src/Services/Notification/BookWorm.Notification/IntegrationEvents/EventHandlers/PlaceOrderCommandHandler.cs (2 hunks)
  • src/Services/Notification/BookWorm.Notification/Workers/CleanUpSentEmailWorker.cs (2 hunks)
  • src/Services/Notification/BookWorm.Notification/Workers/ResendErrorEmailWorker.cs (4 hunks)
💤 Files with no reviewable changes (15)
  • src/Services/Notification/BookWorm.Notification/BookWorm.Notification.csproj
  • Directory.Packages.props
  • src/Services/Notification/BookWorm.Notification/Infrastructure/Senders/Factories/ISmtpClientFactory.cs
  • src/Services/Notification/BookWorm.Notification/GlobalUsings.cs
  • src/Services/Notification/BookWorm.Notification/Infrastructure/Senders/Factories/SmtpClientFactoryObjectPoolPolicy.cs
  • src/Services/Notification/BookWorm.Notification/Infrastructure/Senders/Extensions/MailPitEndpointExtensions.cs
  • src/Services/Notification/BookWorm.Notification/Infrastructure/Senders/Factories/SmtpClientFactory.cs
  • src/Services/Notification/BookWorm.Notification/Infrastructure/Senders/Extensions/SenderWithTelemetryExtensions.cs
  • src/Services/Notification/BookWorm.Notification.UnitTests/Domain/JobOptionsTests.cs
  • src/Services/Notification/BookWorm.Notification/Infrastructure/Senders/TelemetryTags.cs
  • src/Services/Notification/BookWorm.Notification/Infrastructure/Senders/Providers/MailKitSender.cs
  • src/Services/Notification/BookWorm.Notification.UnitTests/Domain/SendGridOptionsTests.cs
  • src/Services/Notification/BookWorm.Notification.UnitTests/Domain/EmailOptionsTests.cs
  • src/Services/Notification/BookWorm.Notification/Domain/Settings/EmailOptions.cs
  • src/Services/Notification/BookWorm.Notification/Infrastructure/Senders/Extensions/Extensions.cs
🧰 Additional context used
🧬 Code Graph Analysis (8)
src/Services/Chat/BookWorm.Chat/Infrastructure/ChatStreaming/ChatStreaming.cs (2)
src/Services/Chat/BookWorm.Chat/Extensions/Extensions.cs (1)
  • Extensions (3-60)
src/Services/Chat/BookWorm.Chat/Infrastructure/ChatStreaming/Extensions.cs (1)
  • Extensions (3-51)
src/Services/Notification/BookWorm.Notification/IntegrationEvents/EventHandlers/CompleteOrderCommandHandler.cs (2)
src/Services/Notification/BookWorm.Notification/IntegrationEvents/EventHandlers/CancelOrderCommandHandler.cs (1)
  • AsyncApi (5-40)
src/Services/Notification/BookWorm.Notification/IntegrationEvents/EventHandlers/PlaceOrderCommandHandler.cs (1)
  • AsyncApi (5-40)
src/Services/Notification/BookWorm.Notification/Extensions/Extensions.cs (4)
src/Services/Notification/BookWorm.Notification/Infrastructure/Senders/MailKit/MailKitExtensions.cs (1)
  • AddMailKitClient (18-30)
src/BuildingBlocks/BookWorm.Constants/Aspire/Components.cs (1)
  • Components (3-43)
src/Services/Notification/BookWorm.Notification/Infrastructure/Senders/SendGrid/SendGridExtensions.cs (1)
  • AddSendGridClient (12-15)
src/Services/Notification/BookWorm.Notification/Infrastructure/Senders/Outbox/OutboxExtensions.cs (1)
  • AddEmailOutbox (17-31)
src/Services/Notification/BookWorm.Notification.UnitTests/Consumers/PlaceOrderConsumerTests.cs (2)
src/Services/Notification/BookWorm.Notification.UnitTests/Fakers/TestDataFakers.cs (1)
  • TestDataFakers (7-27)
src/Services/Notification/BookWorm.Notification.UnitTests/Fakers/FakerExtensions.cs (1)
  • FakerExtensions (6-44)
src/Services/Notification/BookWorm.Notification/Infrastructure/Senders/Outbox/OutboxExtensions.cs (4)
src/Services/Notification/BookWorm.Notification/Infrastructure/Table/Extensions.cs (1)
  • AddTableService (7-14)
src/Services/Notification/BookWorm.Notification/Infrastructure/Senders/MailKit/MailKitSender.cs (1)
  • MailKitSender (6-38)
src/Services/Notification/BookWorm.Notification/Infrastructure/Senders/SendGrid/SendGridSender.cs (1)
  • SendGridSender (8-51)
src/Services/Notification/BookWorm.Notification/Infrastructure/Senders/Outbox/EmailOutboxService.cs (1)
  • EmailOutboxService (3-34)
src/Services/Notification/BookWorm.Notification/Infrastructure/Senders/MailKit/MailKitSettings.cs (1)
src/Services/Notification/BookWorm.Notification/Infrastructure/Senders/SendGrid/SendGridSettings.cs (1)
  • OptionsValidator (6-25)
src/Services/Notification/BookWorm.Notification/Infrastructure/Job/JobSettings.cs (2)
src/Services/Notification/BookWorm.Notification/Infrastructure/Senders/MailKit/MailKitSettings.cs (1)
  • OptionsValidator (6-80)
src/Services/Notification/BookWorm.Notification/Infrastructure/Senders/SendGrid/SendGridSettings.cs (1)
  • OptionsValidator (6-25)
src/Services/Notification/BookWorm.Notification.UnitTests/Fakers/TestDataFakers.cs (1)
src/Services/Notification/BookWorm.Notification.UnitTests/Fakers/FakerExtensions.cs (3)
  • Outbox (8-13)
  • PlaceOrderCommand (15-28)
  • PlaceOrderCommand (30-43)
⏰ Context from checks skipped due to timeout of 90000ms (5)
  • GitHub Check: Analyzers
  • GitHub Check: Style
  • GitHub Check: Security Scan
  • GitHub Check: Build and analyze
  • GitHub Check: Security Scan
🔇 Additional comments (27)
src/Services/Notification/BookWorm.Notification/Infrastructure/Job/JobSettings.cs (1)

5-9: Access modifier change may limit re-use

ConfigurationSection was changed from public to internal.
If other projects (e.g. tests, out-of-process tools, or central configuration helpers) need this constant, they will no longer compile. Verify that the constant isn’t referenced outside BookWorm.Notification before committing to the restriction.

src/Aspire/BookWorm.AppHost/Extensions/Network/LaunchProfileExtensions.cs (1)

1-1: Consistent namespace relocation
The namespace has been correctly updated to BookWorm.AppHost.Extensions.Network, aligning with the folder structure and other network-related extensions. No functional impact.

src/Aspire/BookWorm.AppHost/Extensions/Infrastructure/RedisResourceExtensions.cs (1)

6-6: Namespace aligned with infrastructure domain
The change to BookWorm.AppHost.Extensions.Infrastructure accurately clusters Redis resource extensions with other infrastructure-related extensions. No further edits needed.

src/Aspire/BookWorm.AppHost/Extensions/Security/KeycloakExtensions.cs (1)

1-1: Security namespace introduced
Moving KeycloakExtensions into BookWorm.AppHost.Extensions.Security cleanly separates security concerns. Implementation remains untouched.

src/Aspire/BookWorm.AppHost/Extensions/Infrastructure/DashboardExtensions.cs (1)

1-1: Infrastructure namespace reorganization
The DashboardExtensions class now resides under BookWorm.AppHost.Extensions.Infrastructure, matching its role in telemetry setup. No behavior changes.

src/Aspire/BookWorm.AppHost/Extensions/Infrastructure/AzureExtensions.Run.cs (2)

1-1: Namespace updated for Azure run-time extensions
The namespace switch to BookWorm.AppHost.Extensions.Infrastructure is consistent with other Azure-related infrastructure extensions.

3-29: Enhanced XML documentation
The added <summary> and <remarks> blocks provide comprehensive guidance on the development-time emulator support, improving discoverability and maintainability.

src/Aspire/BookWorm.AppHost/Extensions/Infrastructure/AzureExtensions.Provision.cs (1)

71-76: Verify PublicNetworkAccess API surface

SignalRService.PublicNetworkAccess is typed as an enum (PublicNetworkAccess.Enabled/Disabled) in recent SDKs.
The literal string "Enabled" may produce a compile-time error when the SDK is upgraded.

#!/bin/bash
# Check the property type in the generated SDK
rg --context 2 $'class SignalRService' | rg -n PublicNetworkAccess
src/Aspire/BookWorm.AppHost/Extensions/Network/EmailExtensions.cs (1)

1-1: Namespace move looks good

Pure namespace relocation; no functional impact detected.

src/Aspire/BookWorm.AppHost/Extensions/AI/ModelExtensions.cs (1)

1-1: Namespace update acknowledged

No behavioural change; implementation remains intact.

src/Aspire/BookWorm.AppHost/GlobalUsings.cs (1)

5-9: Check for type collisions with new global usings

Adding five broad namespaces increases the chance of ambiguous type names (e.g., multiple Extensions classes). Monitor for CS0104 warnings during build.

src/Aspire/BookWorm.AppHost/Extensions/Testing/K6Extensions.cs (2)

3-3: Namespace move looks correct—verify callers/imports.

The namespace now matches the physical folder (Extensions/Testing). Please run a quick IDE-wide search or build to catch any leftover using BookWorm.AppHost.Extensions; directives referencing the old location.

52-53: Ensure WithK6OtlpEnvironment() extension is referenced.

Nice addition for OTLP, but double-check that the extension is included via a referenced package/namespace in this compilation unit; otherwise the fluent chain will fail to resolve.

src/Services/Notification/BookWorm.Notification/IntegrationEvents/EventHandlers/PlaceOrderCommandHandler.cs (1)

32-33: Pass the cancellation token to SendAsync

sender.SendAsync already receives context.CancellationToken, but the preceding fluent builder (WithFrom/WithBody) does not honour it. If any of those steps allocate resources (e.g. render Razor views) they should be cancellable. Consider extending the builder API or at least documenting that it is safe to block.

src/Services/Notification/BookWorm.Notification/IntegrationEvents/EventHandlers/CompleteOrderCommandHandler.cs (1)

32-33: Cancellation token propagation

Builder chain (WithFrom, etc.) still ignores context.CancellationToken. Propagate or expose async alternatives.

src/Services/Chat/BookWorm.Chat/Infrastructure/ConversationState/MessageBuffer.cs (1)

169-170: Potential double flushing

_logBuffer.Flush() is called only on exception paths.
If successful flushes are important to capture before process shutdown, consider flushing in the finally block as well (or rely on the built-in periodic flush interval).
No change needed if the current behaviour is intentional.

src/Services/Notification/BookWorm.Notification.UnitTests/Consumers/CompleteOrderConsumerTests.cs (1)

21-48: Configuration object injection looks good

The switch from EmailOptions to MailKitSettings is clean and the DI registration via AddSingleton ensures all collaborators receive the same instance. No functional or test-isolation issues spotted.

Also applies to: 78-110, 145-189

src/Services/Notification/BookWorm.Notification/Workers/ResendErrorEmailWorker.cs (2)

29-33: GlobalLogBuffer.Flush() is synchronous – consider async overload

Flushing the buffer inside an async job path can cause thread-pool starvation if the flush operation is heavy. GlobalLogBuffer exposes FlushAsync(); using it inside the catch blocks would keep the method fully asynchronous.

-            logBuffer.Flush();
+            await logBuffer.FlushAsync(cancellationToken);

[ suggest_optional_refactor ]

Also applies to: 74-77

60-66: Verify the “From” address completeness

WithFrom(mailKitSettings) will typically use both From and Name. ResendErrorEmailWorker passes only the settings coming from configuration; double-check that Name is also configured, otherwise you may send messages with an empty display name which some mail providers treat as spam.

src/Services/Notification/BookWorm.Notification/Extensions/Extensions.cs (1)

37-47: Service-registration sequence LGTM

The new AddMailKitClient / AddSendGridClient APIs plus AddEmailOutbox are in the correct order — the concrete senders are registered before the outbox decorator. Nothing else to flag.

src/Services/Notification/BookWorm.Notification.UnitTests/Consumers/PlaceOrderConsumerTests.cs (1)

85-112: New faker helpers integrate nicely

The switch to WithNullEmail / WithEmptyEmailAddress helpers simplifies the test intent; good improvement.

src/Services/Notification/BookWorm.Notification/Infrastructure/Senders/SendGrid/SendGridExtensions.cs (1)

24-24: Configure<T> overload likely mis-used

services.Configure<SendGridSettings>(configurationSection); passes only the section name, not the IConfigurationSection. Unless you have a custom extension, the typical call is:

services.Configure<SendGridSettings>(
    builder.Configuration.GetSection(configurationSection));

Double-check the overload to ensure the options are actually bound.

src/Services/Notification/BookWorm.Notification/Infrastructure/Senders/MailKit/MailKitSender.cs (1)

19-25: nameof(Notification) may not resolve – verify pipeline key

Notification is a namespace segment, not a type. nameof accepts namespace names only from C# 11 onward. If you’re compiling with an older language version this will fail. Consider hard-coding the key or moving it to a const string:

private const string PipelineName = "Notification";
...
var pipeline = provider.GetPipeline(PipelineName);
src/Services/Chat/BookWorm.Chat/Infrastructure/ConversationState/RedisConversationState.cs (1)

154-168: Good practice – explicit log buffer flush on dispose

Explicitly flushing _logBuffer in the disposal path ensures no buffered entries are lost. Nice catch.

src/Services/Notification/BookWorm.Notification.UnitTests/Fakers/TestDataFakers.cs (1)

17-24: Constructor argument order drift – verify correctness

The new instantiation order inserts the second Guid before Email. Confirm that PlaceOrderCommand’s signature really changed; otherwise tests will silently generate invalid commands.

src/Services/Notification/BookWorm.Notification/Infrastructure/Senders/SendGrid/SendGridSettings.cs (2)

9-9: Changing ConfigurationSection to internal may break external registration code

ConfigurationSection was previously public. Any code outside the BookWorm.Notification assembly that relied on
SendGridSettings.ConfigurationSection (e.g., in AppHost or test projects that bind options) will now fail to compile.
Double-check all callers and, if the constant really shouldn’t leak, expose a helper in the new registration extension so
consumers don’t need to hard-code the string.

1-24: Verify that System.ComponentModel.DataAnnotations is in the global-usings set

All of the attributes used (Required, MaxLength, etc.) come from System.ComponentModel.DataAnnotations.
If no global using exists, add the explicit import to avoid build errors in IDEs that don’t honour the SDK’s implicit-using list.

+using System.ComponentModel.DataAnnotations;

Comment thread src/Services/Notification/BookWorm.Notification/Infrastructure/Job/Extensions.cs
Comment thread src/Aspire/BookWorm.AppHost/Extensions/Infrastructure/AzureExtensions.Provision.cs Outdated
Comment thread src/Aspire/BookWorm.AppHost/Extensions/Infrastructure/AzureExtensions.Provision.cs
Comment thread src/Services/Notification/BookWorm.Notification/Workers/CleanUpSentEmailWorker.cs Outdated
Comment thread ...ification/BookWorm.Notification/IntegrationEvents/EventHandlers/CancelOrderCommandHandler.cs
Comment thread ...es/Notification/BookWorm.Notification/Infrastructure/Senders/MailKit/MailKitClientFactory.cs
Comment thread ...es/Notification/BookWorm.Notification/Infrastructure/Senders/MailKit/MailKitClientFactory.cs Outdated
Comment thread ...ervices/Notification/BookWorm.Notification/Infrastructure/Senders/MailKit/MailKitSettings.cs Outdated
Comment thread ...vices/Notification/BookWorm.Notification/Infrastructure/Senders/MailKit/MailKitExtensions.cs
Comment thread ...vices/Notification/BookWorm.Notification/Infrastructure/Senders/MailKit/MailKitExtensions.cs
@github-actions

github-actions Bot commented Jun 16, 2025 •
edited
Loading

Copy link
Copy Markdown

Test Results

913 tests   - 33   913 ✅  - 33   5m 21s ⏱️ + 3m 15s
  8 suites ± 0     0 💤 ± 0 
  8 files   ± 0     0 ❌ ± 0 

Results for commit 5406e19. ± Comparison against base commit 8618443.

This pull request removes 33 tests. 
BookWorm.Notification.UnitTests.Domain.EmailOptionsTests ‑ GivenCustomName_WhenSettingName_ThenShouldSetCorrectly
BookWorm.Notification.UnitTests.Domain.EmailOptionsTests ‑ GivenDefaultValues_WhenCreatingEmailOptions_ThenShouldHaveDefaultName
BookWorm.Notification.UnitTests.Domain.EmailOptionsTests ‑ GivenInvalidEmail_WhenValidatingOptions_ThenShouldThrowValidationException(  )
BookWorm.Notification.UnitTests.Domain.EmailOptionsTests ‑ GivenInvalidEmail_WhenValidatingOptions_ThenShouldThrowValidationException()
BookWorm.Notification.UnitTests.Domain.EmailOptionsTests ‑ GivenInvalidEmail_WhenValidatingOptions_ThenShouldThrowValidationException(invalid-email)
BookWorm.Notification.UnitTests.Domain.EmailOptionsTests ‑ GivenInvalidEmail_WhenValidatingOptions_ThenShouldThrowValidationException(null)
BookWorm.Notification.UnitTests.Domain.EmailOptionsTests ‑ GivenValidEmail_WhenSettingFromAddress_ThenShouldSetCorrectly
BookWorm.Notification.UnitTests.Domain.EmailOptionsTests ‑ GivenValidEmail_WhenValidatingOptions_ThenShouldSucceed
BookWorm.Notification.UnitTests.Domain.JobOptionsTests ‑ GivenDefaultValues_WhenCreatingJobOptions_ThenShouldHaveDefaultCronExpressions
BookWorm.Notification.UnitTests.Domain.JobOptionsTests ‑ GivenInvalidCronExpression_WhenValidatingOptions_ThenShouldThrowValidationException(  )
…

♻️ This comment has been updated with latest results.

@foxminchan
foxminchan force-pushed the chore/organize-code branch from 752648d to 1b80b3a Compare June 16, 2025 12:25
coderabbitai[bot]
coderabbitai Bot reviewed Jun 16, 2025
View reviewed changes

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 4

🔭 Outside diff range comments (1)
src/Services/Chat/BookWorm.Chat/Infrastructure/ChatStreaming/Extensions.cs (1)

35-57: IMcpClient is no longer resolvable – missing generic overload

Dropping the generic overload registers the instance with service-type object (or its concrete runtime type), so IMcpClient injections will fail at runtime:

-services.AddSingleton(_ =>
+services.AddSingleton<IMcpClient>(_ =>
 {
     …
     return mcpClient;
 });

Without the explicit service-type, any class that requests IMcpClient (e.g. ChatStreaming) will receive nothing and trigger InvalidOperationException: Unable to resolve service for type 'IMcpClient' ….

♻️ Duplicate comments (2)
src/Services/Notification/BookWorm.Notification/Infrastructure/Senders/MailKit/MailKitExtensions.cs (2)

43-43: Building a second root ServiceProvider is still present
Previous feedback about avoiding services.BuildServiceProvider() has not been addressed. This creates a parallel container, duplicates singletons and leaks disposables.
Use the options pattern or access configuration directly (see prior suggestion).

55-55: MailKitClientFactory should be Singleton, not Scoped

A scoped registration spawns a factory (and SMTP client) per request, defeating the “one client per application” goal.
Switch to AddSingleton.

- services.AddScoped(_ => new MailKitClientFactory(settings));
+ services.AddSingleton(_ => new MailKitClientFactory(settings));
🧹 Nitpick comments (4)
src/Aspire/BookWorm.AppHost/Extensions/Infrastructure/AzureExtensions.Provision.cs (2)

9-23: XML documentation no longer matches the code — update the narrative

The remarks still talk about “Premium ZRS + Hot access tier”, “Premium_P1 SKU with 10 capacity units”, etc., while the implementation now provisions StandardLrs, Free_F1, and no AccessTier. The mismatch will mislead maintainers and reviewers.

-///         - Storage accounts with Premium ZRS and Hot access tier
-///         - SignalR services with Premium_P1 SKU and 10 capacity units
+///         - Storage accounts with Standard LRS
+///         - SignalR services with Free_F1 SKU

Adjust the rest of the bullet list accordingly (PostgreSQL tier/capacity, Redis, etc.).

49-54: Duplicate tagging logic: extract a helper to improve DRYness

The same Tags.Add(nameof(Environment)… / Tags.Add(nameof(Projects)… block appears four times. A small extension method keeps the intent clear and future-proof:

-resource.Tags.Add(nameof(Environment), builder.ApplicationBuilder.Environment.EnvironmentName);
-resource.Tags.Add(nameof(Projects), nameof(BookWorm));
+resource.ApplyStandardTags(builder.ApplicationBuilder.Environment.EnvironmentName);
// elsewhere
internal static void ApplyStandardTags(this ITaggedResource resource, string environment)
{
    resource.Tags.Add(nameof(Environment), environment);
    resource.Tags.Add(nameof(Projects), nameof(BookWorm));
}

Also applies to: 75-80, 108-113, 167-172

src/Services/Chat/BookWorm.Chat/Infrastructure/ChatStreaming/ChatStreaming.cs (1)

135-136: Flush on success path as well for symmetrical logging

logBuffer.Flush() is invoked only on error/cancel branches. If everything succeeds, buffered logs may remain unflushed until an unrelated trigger occurs. Adding a flush in the success path (immediately after persisting the assistant message) keeps behaviour consistent.

src/Services/Notification/BookWorm.Notification/Infrastructure/Senders/MailKit/MailKitClientFactory.cs (1)

20-34: Reconnect also when authentication is lost

IsConnected does not guarantee the session is still authenticated (many servers drop auth after idling).
Consider checking !_client.IsAuthenticated together with !_client.IsConnected so callers never receive a live-but-unauthenticated client.

-            if (_client is null || !_client.IsConnected)
+            if (_client is null || !_client.IsConnected || !_client.IsAuthenticated)
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 752648d and 1b80b3a.

📒 Files selected for processing (61)
  • Directory.Packages.props (0 hunks)
  • src/Aspire/BookWorm.AppHost/Extensions/AI/ModelExtensions.cs (1 hunks)
  • src/Aspire/BookWorm.AppHost/Extensions/Infrastructure/AzureExtensions.Provision.cs (5 hunks)
  • src/Aspire/BookWorm.AppHost/Extensions/Infrastructure/AzureExtensions.Run.cs (1 hunks)
  • src/Aspire/BookWorm.AppHost/Extensions/Infrastructure/DashboardExtensions.cs (1 hunks)
  • src/Aspire/BookWorm.AppHost/Extensions/Infrastructure/RedisResourceExtensions.cs (1 hunks)
  • src/Aspire/BookWorm.AppHost/Extensions/Network/EmailExtensions.cs (1 hunks)
  • src/Aspire/BookWorm.AppHost/Extensions/Network/EndpointExtensions.cs (1 hunks)
  • src/Aspire/BookWorm.AppHost/Extensions/Network/LaunchProfileExtensions.cs (1 hunks)
  • src/Aspire/BookWorm.AppHost/Extensions/Security/KeycloakExtensions.cs (1 hunks)
  • src/Aspire/BookWorm.AppHost/Extensions/Testing/K6Extensions.cs (2 hunks)
  • src/Aspire/BookWorm.AppHost/GlobalUsings.cs (1 hunks)
  • src/Aspire/BookWorm.ServiceDefaults/Extensions.cs (2 hunks)
  • src/BuildingBlocks/BookWorm.Chassis/Exceptions/GlobalExceptionHandler.cs (2 hunks)
  • src/BuildingBlocks/BookWorm.Chassis/Exceptions/NotFoundExceptionHandler.cs (3 hunks)
  • src/BuildingBlocks/BookWorm.Chassis/Exceptions/ValidationExceptionHandler.cs (2 hunks)
  • src/Services/Chat/BookWorm.Chat/Infrastructure/ChatStreaming/ChatContext.cs (1 hunks)
  • src/Services/Chat/BookWorm.Chat/Infrastructure/ChatStreaming/ChatStreaming.cs (13 hunks)
  • src/Services/Chat/BookWorm.Chat/Infrastructure/ChatStreaming/Extensions.cs (2 hunks)
  • src/Services/Chat/BookWorm.Chat/Infrastructure/ConversationState/MessageBuffer.cs (5 hunks)
  • src/Services/Chat/BookWorm.Chat/Infrastructure/ConversationState/RedisConversationState.cs (6 hunks)
  • src/Services/Notification/BookWorm.Notification.UnitTests/Consumers/CancelOrderConsumerTests.cs (6 hunks)
  • src/Services/Notification/BookWorm.Notification.UnitTests/Consumers/CompleteOrderConsumerTests.cs (7 hunks)
  • src/Services/Notification/BookWorm.Notification.UnitTests/Consumers/PlaceOrderConsumerTests.cs (6 hunks)
  • src/Services/Notification/BookWorm.Notification.UnitTests/Domain/EmailOptionsTests.cs (0 hunks)
  • src/Services/Notification/BookWorm.Notification.UnitTests/Domain/JobOptionsTests.cs (0 hunks)
  • src/Services/Notification/BookWorm.Notification.UnitTests/Domain/SendGridOptionsTests.cs (0 hunks)
  • src/Services/Notification/BookWorm.Notification.UnitTests/Fakers/FakerExtensions.cs (1 hunks)
  • src/Services/Notification/BookWorm.Notification.UnitTests/Fakers/TestDataFakers.cs (1 hunks)
  • src/Services/Notification/BookWorm.Notification.UnitTests/Workers/CleanUpSentEmailWorkerTests.cs (3 hunks)
  • src/Services/Notification/BookWorm.Notification.UnitTests/Workers/ResendErrorEmailWorkerTests.cs (3 hunks)
  • src/Services/Notification/BookWorm.Notification/BookWorm.Notification.csproj (0 hunks)
  • src/Services/Notification/BookWorm.Notification/Domain/Builders/OrderMimeMessageBuilder.cs (2 hunks)
  • src/Services/Notification/BookWorm.Notification/Domain/Settings/EmailOptions.cs (0 hunks)
  • src/Services/Notification/BookWorm.Notification/Extensions/Extensions.cs (2 hunks)
  • src/Services/Notification/BookWorm.Notification/GlobalUsings.cs (0 hunks)
  • src/Services/Notification/BookWorm.Notification/Infrastructure/Job/Extensions.cs (1 hunks)
  • src/Services/Notification/BookWorm.Notification/Infrastructure/Job/JobSettings.cs (1 hunks)
  • src/Services/Notification/BookWorm.Notification/Infrastructure/Senders/Extensions/Extensions.cs (0 hunks)
  • src/Services/Notification/BookWorm.Notification/Infrastructure/Senders/Extensions/MailPitEndpointExtensions.cs (0 hunks)
  • src/Services/Notification/BookWorm.Notification/Infrastructure/Senders/Extensions/SenderWithTelemetryExtensions.cs (0 hunks)
  • src/Services/Notification/BookWorm.Notification/Infrastructure/Senders/Factories/ISmtpClientFactory.cs (0 hunks)
  • src/Services/Notification/BookWorm.Notification/Infrastructure/Senders/Factories/SmtpClientFactory.cs (0 hunks)
  • src/Services/Notification/BookWorm.Notification/Infrastructure/Senders/Factories/SmtpClientFactoryObjectPoolPolicy.cs (0 hunks)
  • src/Services/Notification/BookWorm.Notification/Infrastructure/Senders/MailKit/MailKitClientFactory.cs (1 hunks)
  • src/Services/Notification/BookWorm.Notification/Infrastructure/Senders/MailKit/MailKitExtensions.cs (1 hunks)
  • src/Services/Notification/BookWorm.Notification/Infrastructure/Senders/MailKit/MailKitHealthCheck.cs (1 hunks)
  • src/Services/Notification/BookWorm.Notification/Infrastructure/Senders/MailKit/MailKitSender.cs (1 hunks)
  • src/Services/Notification/BookWorm.Notification/Infrastructure/Senders/MailKit/MailKitSettings.cs (1 hunks)
  • src/Services/Notification/BookWorm.Notification/Infrastructure/Senders/Outbox/EmailOutboxService.cs (2 hunks)
  • src/Services/Notification/BookWorm.Notification/Infrastructure/Senders/Outbox/OutboxExtensions.cs (1 hunks)
  • src/Services/Notification/BookWorm.Notification/Infrastructure/Senders/Providers/MailKitSender.cs (0 hunks)
  • src/Services/Notification/BookWorm.Notification/Infrastructure/Senders/SendGrid/SendGridExtensions.cs (1 hunks)
  • src/Services/Notification/BookWorm.Notification/Infrastructure/Senders/SendGrid/SendGridSender.cs (2 hunks)
  • src/Services/Notification/BookWorm.Notification/Infrastructure/Senders/SendGrid/SendGridSettings.cs (1 hunks)
  • src/Services/Notification/BookWorm.Notification/Infrastructure/Senders/TelemetryTags.cs (0 hunks)
  • src/Services/Notification/BookWorm.Notification/IntegrationEvents/EventHandlers/CancelOrderCommandHandler.cs (2 hunks)
  • src/Services/Notification/BookWorm.Notification/IntegrationEvents/EventHandlers/CompleteOrderCommandHandler.cs (2 hunks)
  • src/Services/Notification/BookWorm.Notification/IntegrationEvents/EventHandlers/PlaceOrderCommandHandler.cs (2 hunks)
  • src/Services/Notification/BookWorm.Notification/Workers/CleanUpSentEmailWorker.cs (3 hunks)
  • src/Services/Notification/BookWorm.Notification/Workers/ResendErrorEmailWorker.cs (4 hunks)
💤 Files with no reviewable changes (15)
  • src/Services/Notification/BookWorm.Notification/BookWorm.Notification.csproj
  • Directory.Packages.props
  • src/Services/Notification/BookWorm.Notification/GlobalUsings.cs
  • src/Services/Notification/BookWorm.Notification/Infrastructure/Senders/Factories/ISmtpClientFactory.cs
  • src/Services/Notification/BookWorm.Notification/Infrastructure/Senders/Factories/SmtpClientFactory.cs
  • src/Services/Notification/BookWorm.Notification.UnitTests/Domain/JobOptionsTests.cs
  • src/Services/Notification/BookWorm.Notification/Infrastructure/Senders/Extensions/MailPitEndpointExtensions.cs
  • src/Services/Notification/BookWorm.Notification/Infrastructure/Senders/Extensions/SenderWithTelemetryExtensions.cs
  • src/Services/Notification/BookWorm.Notification/Infrastructure/Senders/Factories/SmtpClientFactoryObjectPoolPolicy.cs
  • src/Services/Notification/BookWorm.Notification/Infrastructure/Senders/TelemetryTags.cs
  • src/Services/Notification/BookWorm.Notification.UnitTests/Domain/EmailOptionsTests.cs
  • src/Services/Notification/BookWorm.Notification.UnitTests/Domain/SendGridOptionsTests.cs
  • src/Services/Notification/BookWorm.Notification/Domain/Settings/EmailOptions.cs
  • src/Services/Notification/BookWorm.Notification/Infrastructure/Senders/Providers/MailKitSender.cs
  • src/Services/Notification/BookWorm.Notification/Infrastructure/Senders/Extensions/Extensions.cs
✅ Files skipped from review due to trivial changes (2)
  • src/Aspire/BookWorm.ServiceDefaults/Extensions.cs
  • src/Services/Chat/BookWorm.Chat/Infrastructure/ChatStreaming/ChatContext.cs
🚧 Files skipped from review as they are similar to previous changes (39)
  • src/Aspire/BookWorm.AppHost/Extensions/Network/EndpointExtensions.cs
  • src/Aspire/BookWorm.AppHost/Extensions/Network/LaunchProfileExtensions.cs
  • src/Aspire/BookWorm.AppHost/Extensions/Network/EmailExtensions.cs
  • src/Aspire/BookWorm.AppHost/Extensions/Infrastructure/DashboardExtensions.cs
  • src/Aspire/BookWorm.AppHost/Extensions/Security/KeycloakExtensions.cs
  • src/Aspire/BookWorm.AppHost/Extensions/Infrastructure/RedisResourceExtensions.cs
  • src/Aspire/BookWorm.AppHost/Extensions/AI/ModelExtensions.cs
  • src/Services/Notification/BookWorm.Notification/Domain/Builders/OrderMimeMessageBuilder.cs
  • src/Services/Notification/BookWorm.Notification/Infrastructure/Senders/SendGrid/SendGridSettings.cs
  • src/Services/Notification/BookWorm.Notification/Infrastructure/Job/Extensions.cs
  • src/BuildingBlocks/BookWorm.Chassis/Exceptions/NotFoundExceptionHandler.cs
  • src/Services/Notification/BookWorm.Notification/Workers/CleanUpSentEmailWorker.cs
  • src/Aspire/BookWorm.AppHost/GlobalUsings.cs
  • src/BuildingBlocks/BookWorm.Chassis/Exceptions/ValidationExceptionHandler.cs
  • src/Services/Notification/BookWorm.Notification/IntegrationEvents/EventHandlers/CompleteOrderCommandHandler.cs
  • src/BuildingBlocks/BookWorm.Chassis/Exceptions/GlobalExceptionHandler.cs
  • src/Services/Notification/BookWorm.Notification/Infrastructure/Job/JobSettings.cs
  • src/Services/Notification/BookWorm.Notification.UnitTests/Workers/CleanUpSentEmailWorkerTests.cs
  • src/Services/Notification/BookWorm.Notification/Infrastructure/Senders/Outbox/EmailOutboxService.cs
  • src/Services/Notification/BookWorm.Notification.UnitTests/Workers/ResendErrorEmailWorkerTests.cs
  • src/Services/Notification/BookWorm.Notification/IntegrationEvents/EventHandlers/CancelOrderCommandHandler.cs
  • src/Aspire/BookWorm.AppHost/Extensions/Testing/K6Extensions.cs
  • src/Services/Chat/BookWorm.Chat/Infrastructure/ConversationState/MessageBuffer.cs
  • src/Services/Notification/BookWorm.Notification.UnitTests/Consumers/CancelOrderConsumerTests.cs
  • src/Services/Notification/BookWorm.Notification/Workers/ResendErrorEmailWorker.cs
  • src/Services/Notification/BookWorm.Notification/Infrastructure/Senders/SendGrid/SendGridExtensions.cs
  • src/Services/Notification/BookWorm.Notification.UnitTests/Consumers/CompleteOrderConsumerTests.cs
  • src/Services/Notification/BookWorm.Notification/Infrastructure/Senders/MailKit/MailKitHealthCheck.cs
  • src/Services/Notification/BookWorm.Notification/IntegrationEvents/EventHandlers/PlaceOrderCommandHandler.cs
  • src/Services/Notification/BookWorm.Notification.UnitTests/Consumers/PlaceOrderConsumerTests.cs
  • src/Services/Notification/BookWorm.Notification/Infrastructure/Senders/SendGrid/SendGridSender.cs
  • src/Services/Notification/BookWorm.Notification/Infrastructure/Senders/Outbox/OutboxExtensions.cs
  • src/Services/Chat/BookWorm.Chat/Infrastructure/ConversationState/RedisConversationState.cs
  • src/Services/Notification/BookWorm.Notification/Infrastructure/Senders/MailKit/MailKitSender.cs
  • src/Services/Notification/BookWorm.Notification/Infrastructure/Senders/MailKit/MailKitSettings.cs
  • src/Services/Notification/BookWorm.Notification.UnitTests/Fakers/TestDataFakers.cs
  • src/Aspire/BookWorm.AppHost/Extensions/Infrastructure/AzureExtensions.Run.cs
  • src/Services/Notification/BookWorm.Notification.UnitTests/Fakers/FakerExtensions.cs
  • src/Services/Notification/BookWorm.Notification/Extensions/Extensions.cs
🧰 Additional context used
🧬 Code Graph Analysis (2)
src/Services/Notification/BookWorm.Notification/Infrastructure/Senders/MailKit/MailKitClientFactory.cs (2)
src/Services/Notification/BookWorm.Notification/Infrastructure/Senders/MailKit/MailKitHealthCheck.cs (1)
  • Task (5-19)
src/Services/Notification/BookWorm.Notification/Infrastructure/Senders/MailKit/MailKitSender.cs (1)
  • Task (13-40)
src/Services/Notification/BookWorm.Notification/Infrastructure/Senders/MailKit/MailKitExtensions.cs (5)
src/Aspire/BookWorm.ServiceDefaults/Configuration/ConfigurationExtensions.cs (1)
  • GetRequiredConnectionString (25-37)
src/Services/Notification/BookWorm.Notification/Infrastructure/Senders/MailKit/MailKitSettings.cs (1)
  • ParseConnectionString (27-89)
src/Services/Notification/BookWorm.Notification/Infrastructure/Senders/MailKit/MailKitClientFactory.cs (1)
  • MailKitClientFactory (3-52)
src/Services/Notification/BookWorm.Notification/Infrastructure/Senders/MailKit/MailKitHealthCheck.cs (1)
  • MailKitHealthCheck (3-20)
src/BuildingBlocks/BookWorm.Constants/Aspire/Components.cs (1)
  • Components (3-43)
⏰ Context from checks skipped due to timeout of 90000ms (7)
  • GitHub Check: Analyzers
  • GitHub Check: Style
  • GitHub Check: Security Scan
  • GitHub Check: Security Scan
  • GitHub Check: Style
  • GitHub Check: Analyzers
  • GitHub Check: Build and analyze
🔇 Additional comments (3)
src/Aspire/BookWorm.AppHost/Extensions/Infrastructure/AzureExtensions.Provision.cs (1)

45-48: Good catch switching to .Single()

Using Single() removes the silent-failure path previously discussed and aligns with the stricter provisioning style. No further concerns here.

src/Services/Chat/BookWorm.Chat/Infrastructure/ChatStreaming/Extensions.cs (1)

11-17: Registration of ChatContext looks good

The composite object is resolved purely from existing singleton services and is itself registered as a singleton – perfect fit.

src/Services/Chat/BookWorm.Chat/Infrastructure/ChatStreaming/ChatStreaming.cs (1)

170-172: Cancellation token disposal race

await chatContext.CancellationManager.CancelAsync(assistantReplyId) runs after the final fragment is published. If any consumer awaits that fragment before CancelAsync completes, they may observe an already-cancelled token. Typically you’d cancel before signalling completion to downstream consumers. Please double-check the intended order.

Comment thread src/Aspire/BookWorm.AppHost/Extensions/Infrastructure/AzureExtensions.Provision.cs
Comment thread src/Services/Chat/BookWorm.Chat/Infrastructure/ChatStreaming/ChatStreaming.cs
Comment thread ...es/Notification/BookWorm.Notification/Infrastructure/Senders/MailKit/MailKitClientFactory.cs
Comment thread ...vices/Notification/BookWorm.Notification/Infrastructure/Senders/MailKit/MailKitExtensions.cs
@foxminchan
foxminchan force-pushed the chore/organize-code branch from 1b80b3a to 5406e19 Compare June 16, 2025 13:09
coderabbitai[bot]
coderabbitai Bot reviewed Jun 16, 2025
View reviewed changes

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 0

♻️ Duplicate comments (2)
src/Services/Notification/BookWorm.Notification/Infrastructure/Senders/MailKit/MailKitExtensions.cs (2)

41-43: Compile error & duplicated-root-provider pattern persist

services.Configure<MailKitSettings>(sectionName); targets a non-existent overload, and the subsequent BuildServiceProvider() creates a second root container – both problems were flagged in the last review.

- services.Configure<MailKitSettings>(sectionName);
-
- var settings = services.BuildServiceProvider().GetRequiredService<MailKitSettings>();
+var settings = builder.Configuration                // avoid extra root provider
+    .GetSection(sectionName)
+    .Get<MailKitSettings>() ?? new();

This binds the section correctly and removes the extra service provider.

55-56: MailKitClientFactory should be a singleton

Creating the factory per scope recreates the underlying SMTP client, defeating connection reuse and increasing resource consumption. Register it once:

- services.AddScoped(_ => new MailKitClientFactory(settings));
+ services.AddSingleton(_ => new MailKitClientFactory(settings));
🧹 Nitpick comments (1)
src/Services/Notification/BookWorm.Notification/Infrastructure/Senders/MailKit/MailKitExtensions.cs (1)

1-3: Unused using directive

using MailKit; is never referenced in this file and can be removed to keep the compilation unit tidy.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 1b80b3a and 5406e19.

📒 Files selected for processing (61)
  • Directory.Packages.props (0 hunks)
  • src/Aspire/BookWorm.AppHost/Extensions/AI/ModelExtensions.cs (1 hunks)
  • src/Aspire/BookWorm.AppHost/Extensions/Infrastructure/AzureExtensions.Provision.cs (5 hunks)
  • src/Aspire/BookWorm.AppHost/Extensions/Infrastructure/AzureExtensions.Run.cs (1 hunks)
  • src/Aspire/BookWorm.AppHost/Extensions/Infrastructure/DashboardExtensions.cs (1 hunks)
  • src/Aspire/BookWorm.AppHost/Extensions/Infrastructure/RedisResourceExtensions.cs (1 hunks)
  • src/Aspire/BookWorm.AppHost/Extensions/Network/EmailExtensions.cs (1 hunks)
  • src/Aspire/BookWorm.AppHost/Extensions/Network/EndpointExtensions.cs (1 hunks)
  • src/Aspire/BookWorm.AppHost/Extensions/Network/LaunchProfileExtensions.cs (1 hunks)
  • src/Aspire/BookWorm.AppHost/Extensions/Security/KeycloakExtensions.cs (1 hunks)
  • src/Aspire/BookWorm.AppHost/Extensions/Testing/K6Extensions.cs (2 hunks)
  • src/Aspire/BookWorm.AppHost/GlobalUsings.cs (1 hunks)
  • src/Aspire/BookWorm.ServiceDefaults/Extensions.cs (2 hunks)
  • src/BuildingBlocks/BookWorm.Chassis/Exceptions/GlobalExceptionHandler.cs (2 hunks)
  • src/BuildingBlocks/BookWorm.Chassis/Exceptions/NotFoundExceptionHandler.cs (3 hunks)
  • src/BuildingBlocks/BookWorm.Chassis/Exceptions/ValidationExceptionHandler.cs (2 hunks)
  • src/Services/Chat/BookWorm.Chat/Infrastructure/ChatStreaming/ChatContext.cs (1 hunks)
  • src/Services/Chat/BookWorm.Chat/Infrastructure/ChatStreaming/ChatStreaming.cs (13 hunks)
  • src/Services/Chat/BookWorm.Chat/Infrastructure/ChatStreaming/Extensions.cs (2 hunks)
  • src/Services/Chat/BookWorm.Chat/Infrastructure/ConversationState/MessageBuffer.cs (5 hunks)
  • src/Services/Chat/BookWorm.Chat/Infrastructure/ConversationState/RedisConversationState.cs (6 hunks)
  • src/Services/Notification/BookWorm.Notification.UnitTests/Consumers/CancelOrderConsumerTests.cs (6 hunks)
  • src/Services/Notification/BookWorm.Notification.UnitTests/Consumers/CompleteOrderConsumerTests.cs (7 hunks)
  • src/Services/Notification/BookWorm.Notification.UnitTests/Consumers/PlaceOrderConsumerTests.cs (6 hunks)
  • src/Services/Notification/BookWorm.Notification.UnitTests/Domain/EmailOptionsTests.cs (0 hunks)
  • src/Services/Notification/BookWorm.Notification.UnitTests/Domain/JobOptionsTests.cs (0 hunks)
  • src/Services/Notification/BookWorm.Notification.UnitTests/Domain/SendGridOptionsTests.cs (0 hunks)
  • src/Services/Notification/BookWorm.Notification.UnitTests/Fakers/FakerExtensions.cs (1 hunks)
  • src/Services/Notification/BookWorm.Notification.UnitTests/Fakers/TestDataFakers.cs (1 hunks)
  • src/Services/Notification/BookWorm.Notification.UnitTests/Workers/CleanUpSentEmailWorkerTests.cs (3 hunks)
  • src/Services/Notification/BookWorm.Notification.UnitTests/Workers/ResendErrorEmailWorkerTests.cs (3 hunks)
  • src/Services/Notification/BookWorm.Notification/BookWorm.Notification.csproj (0 hunks)
  • src/Services/Notification/BookWorm.Notification/Domain/Builders/OrderMimeMessageBuilder.cs (2 hunks)
  • src/Services/Notification/BookWorm.Notification/Domain/Settings/EmailOptions.cs (0 hunks)
  • src/Services/Notification/BookWorm.Notification/Extensions/Extensions.cs (2 hunks)
  • src/Services/Notification/BookWorm.Notification/GlobalUsings.cs (0 hunks)
  • src/Services/Notification/BookWorm.Notification/Infrastructure/Job/Extensions.cs (1 hunks)
  • src/Services/Notification/BookWorm.Notification/Infrastructure/Job/JobSettings.cs (1 hunks)
  • src/Services/Notification/BookWorm.Notification/Infrastructure/Senders/Extensions/Extensions.cs (0 hunks)
  • src/Services/Notification/BookWorm.Notification/Infrastructure/Senders/Extensions/MailPitEndpointExtensions.cs (0 hunks)
  • src/Services/Notification/BookWorm.Notification/Infrastructure/Senders/Extensions/SenderWithTelemetryExtensions.cs (0 hunks)
  • src/Services/Notification/BookWorm.Notification/Infrastructure/Senders/Factories/ISmtpClientFactory.cs (0 hunks)
  • src/Services/Notification/BookWorm.Notification/Infrastructure/Senders/Factories/SmtpClientFactory.cs (0 hunks)
  • src/Services/Notification/BookWorm.Notification/Infrastructure/Senders/Factories/SmtpClientFactoryObjectPoolPolicy.cs (0 hunks)
  • src/Services/Notification/BookWorm.Notification/Infrastructure/Senders/MailKit/MailKitClientFactory.cs (1 hunks)
  • src/Services/Notification/BookWorm.Notification/Infrastructure/Senders/MailKit/MailKitExtensions.cs (1 hunks)
  • src/Services/Notification/BookWorm.Notification/Infrastructure/Senders/MailKit/MailKitHealthCheck.cs (1 hunks)
  • src/Services/Notification/BookWorm.Notification/Infrastructure/Senders/MailKit/MailKitSender.cs (1 hunks)
  • src/Services/Notification/BookWorm.Notification/Infrastructure/Senders/MailKit/MailKitSettings.cs (1 hunks)
  • src/Services/Notification/BookWorm.Notification/Infrastructure/Senders/Outbox/EmailOutboxService.cs (2 hunks)
  • src/Services/Notification/BookWorm.Notification/Infrastructure/Senders/Outbox/OutboxExtensions.cs (1 hunks)
  • src/Services/Notification/BookWorm.Notification/Infrastructure/Senders/Providers/MailKitSender.cs (0 hunks)
  • src/Services/Notification/BookWorm.Notification/Infrastructure/Senders/SendGrid/SendGridExtensions.cs (1 hunks)
  • src/Services/Notification/BookWorm.Notification/Infrastructure/Senders/SendGrid/SendGridSender.cs (2 hunks)
  • src/Services/Notification/BookWorm.Notification/Infrastructure/Senders/SendGrid/SendGridSettings.cs (1 hunks)
  • src/Services/Notification/BookWorm.Notification/Infrastructure/Senders/TelemetryTags.cs (0 hunks)
  • src/Services/Notification/BookWorm.Notification/IntegrationEvents/EventHandlers/CancelOrderCommandHandler.cs (2 hunks)
  • src/Services/Notification/BookWorm.Notification/IntegrationEvents/EventHandlers/CompleteOrderCommandHandler.cs (2 hunks)
  • src/Services/Notification/BookWorm.Notification/IntegrationEvents/EventHandlers/PlaceOrderCommandHandler.cs (2 hunks)
  • src/Services/Notification/BookWorm.Notification/Workers/CleanUpSentEmailWorker.cs (3 hunks)
  • src/Services/Notification/BookWorm.Notification/Workers/ResendErrorEmailWorker.cs (4 hunks)
💤 Files with no reviewable changes (15)
  • src/Services/Notification/BookWorm.Notification/BookWorm.Notification.csproj
  • src/Services/Notification/BookWorm.Notification.UnitTests/Domain/JobOptionsTests.cs
  • Directory.Packages.props
  • src/Services/Notification/BookWorm.Notification/Infrastructure/Senders/TelemetryTags.cs
  • src/Services/Notification/BookWorm.Notification/GlobalUsings.cs
  • src/Services/Notification/BookWorm.Notification/Infrastructure/Senders/Factories/ISmtpClientFactory.cs
  • src/Services/Notification/BookWorm.Notification/Infrastructure/Senders/Factories/SmtpClientFactory.cs
  • src/Services/Notification/BookWorm.Notification/Infrastructure/Senders/Extensions/SenderWithTelemetryExtensions.cs
  • src/Services/Notification/BookWorm.Notification/Infrastructure/Senders/Factories/SmtpClientFactoryObjectPoolPolicy.cs
  • src/Services/Notification/BookWorm.Notification/Infrastructure/Senders/Extensions/MailPitEndpointExtensions.cs
  • src/Services/Notification/BookWorm.Notification/Infrastructure/Senders/Providers/MailKitSender.cs
  • src/Services/Notification/BookWorm.Notification/Domain/Settings/EmailOptions.cs
  • src/Services/Notification/BookWorm.Notification.UnitTests/Domain/EmailOptionsTests.cs
  • src/Services/Notification/BookWorm.Notification.UnitTests/Domain/SendGridOptionsTests.cs
  • src/Services/Notification/BookWorm.Notification/Infrastructure/Senders/Extensions/Extensions.cs
✅ Files skipped from review due to trivial changes (1)
  • src/Aspire/BookWorm.ServiceDefaults/Extensions.cs
🚧 Files skipped from review as they are similar to previous changes (44)
  • src/Services/Notification/BookWorm.Notification/Infrastructure/Senders/SendGrid/SendGridSettings.cs
  • src/Aspire/BookWorm.AppHost/Extensions/Infrastructure/DashboardExtensions.cs
  • src/Aspire/BookWorm.AppHost/Extensions/Network/LaunchProfileExtensions.cs
  • src/Aspire/BookWorm.AppHost/Extensions/Infrastructure/RedisResourceExtensions.cs
  • src/Aspire/BookWorm.AppHost/Extensions/Network/EmailExtensions.cs
  • src/Aspire/BookWorm.AppHost/GlobalUsings.cs
  • src/Services/Notification/BookWorm.Notification/Infrastructure/Senders/MailKit/MailKitClientFactory.cs
  • src/Aspire/BookWorm.AppHost/Extensions/AI/ModelExtensions.cs
  • src/Services/Notification/BookWorm.Notification.UnitTests/Workers/CleanUpSentEmailWorkerTests.cs
  • src/Aspire/BookWorm.AppHost/Extensions/Security/KeycloakExtensions.cs
  • src/Services/Notification/BookWorm.Notification/Infrastructure/Senders/Outbox/EmailOutboxService.cs
  • src/Aspire/BookWorm.AppHost/Extensions/Network/EndpointExtensions.cs
  • src/Services/Chat/BookWorm.Chat/Infrastructure/ChatStreaming/Extensions.cs
  • src/BuildingBlocks/BookWorm.Chassis/Exceptions/GlobalExceptionHandler.cs
  • src/Services/Notification/BookWorm.Notification/Workers/CleanUpSentEmailWorker.cs
  • src/Services/Notification/BookWorm.Notification/IntegrationEvents/EventHandlers/PlaceOrderCommandHandler.cs
  • src/BuildingBlocks/BookWorm.Chassis/Exceptions/ValidationExceptionHandler.cs
  • src/Services/Notification/BookWorm.Notification/IntegrationEvents/EventHandlers/CompleteOrderCommandHandler.cs
  • src/Services/Chat/BookWorm.Chat/Infrastructure/ChatStreaming/ChatContext.cs
  • src/Services/Notification/BookWorm.Notification/Domain/Builders/OrderMimeMessageBuilder.cs
  • src/Services/Notification/BookWorm.Notification/Infrastructure/Job/Extensions.cs
  • src/Aspire/BookWorm.AppHost/Extensions/Testing/K6Extensions.cs
  • src/Services/Chat/BookWorm.Chat/Infrastructure/ConversationState/MessageBuffer.cs
  • src/Services/Notification/BookWorm.Notification/Extensions/Extensions.cs
  • src/Services/Notification/BookWorm.Notification/Workers/ResendErrorEmailWorker.cs
  • src/Services/Notification/BookWorm.Notification.UnitTests/Consumers/CompleteOrderConsumerTests.cs
  • src/Services/Notification/BookWorm.Notification/IntegrationEvents/EventHandlers/CancelOrderCommandHandler.cs
  • src/Services/Notification/BookWorm.Notification/Infrastructure/Job/JobSettings.cs
  • src/BuildingBlocks/BookWorm.Chassis/Exceptions/NotFoundExceptionHandler.cs
  • src/Services/Notification/BookWorm.Notification.UnitTests/Workers/ResendErrorEmailWorkerTests.cs
  • src/Services/Notification/BookWorm.Notification/Infrastructure/Senders/SendGrid/SendGridExtensions.cs
  • src/Services/Notification/BookWorm.Notification.UnitTests/Consumers/CancelOrderConsumerTests.cs
  • src/Services/Notification/BookWorm.Notification.UnitTests/Consumers/PlaceOrderConsumerTests.cs
  • src/Services/Notification/BookWorm.Notification/Infrastructure/Senders/MailKit/MailKitHealthCheck.cs
  • src/Aspire/BookWorm.AppHost/Extensions/Infrastructure/AzureExtensions.Run.cs
  • src/Services/Notification/BookWorm.Notification/Infrastructure/Senders/MailKit/MailKitSender.cs
  • src/Services/Chat/BookWorm.Chat/Infrastructure/ConversationState/RedisConversationState.cs
  • src/Services/Notification/BookWorm.Notification/Infrastructure/Senders/SendGrid/SendGridSender.cs
  • src/Services/Notification/BookWorm.Notification.UnitTests/Fakers/TestDataFakers.cs
  • src/Services/Chat/BookWorm.Chat/Infrastructure/ChatStreaming/ChatStreaming.cs
  • src/Services/Notification/BookWorm.Notification/Infrastructure/Senders/Outbox/OutboxExtensions.cs
  • src/Aspire/BookWorm.AppHost/Extensions/Infrastructure/AzureExtensions.Provision.cs
  • src/Services/Notification/BookWorm.Notification/Infrastructure/Senders/MailKit/MailKitSettings.cs
  • src/Services/Notification/BookWorm.Notification.UnitTests/Fakers/FakerExtensions.cs
⏰ Context from checks skipped due to timeout of 90000ms (7)
  • GitHub Check: Style
  • GitHub Check: Analyzers
  • GitHub Check: Security Scan
  • GitHub Check: Style
  • GitHub Check: Analyzers
  • GitHub Check: Security Scan
  • GitHub Check: Build and analyze

@sonarqubecloud

sonarqubecloud Bot commented Jun 16, 2025

Copy link
Copy Markdown

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
100.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

@foxminchan
foxminchan merged commit 71bae9a into main Jun 16, 2025
27 checks passed
@foxminchan
foxminchan deleted the chore/organize-code branch June 16, 2025 13:18
@coderabbitai coderabbitai Bot mentioned this pull request Jul 2, 2025
Merged
8 tasks
This was referenced Sep 27, 2025
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot Copilot left review comments
+1 more reviewer
@coderabbitai coderabbitai[bot] coderabbitai[bot] left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

@foxminchan foxminchan

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@foxminchan