Publish application layer as a NuGet#41
Merged
kyurkchyan merged 13 commits intomasterfrom Apr 8, 2026
Merged
Conversation
Contributor
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughAdds batched DLQ peek and batch-diagnostic commands/handlers/DTOs, optional App Insights fallback, DI rename and caller updates, NuGet/SourceLink packaging metadata, expanded CI packaging/publish steps, CLI/output redirect guard, paginated mock behavior, and extensive unit and integration tests. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Mediator as Mediator/Handler
participant Factory as ServiceBusClientFactory
participant SBAdmin as ServiceBus Admin
participant SB as Azure Service Bus
rect rgba(200,200,255,0.5)
Client->>Mediator: Send(PeekDlqBatchCommand)
end
Mediator->>Factory: CreateClient(namespace)
Factory-->>Mediator: IServiceBusClient
Mediator->>SBAdmin: Get runtime properties (queue/subscription)
SBAdmin-->>Mediator: Dead-letter count
Mediator->>SB: Create DLQ receiver
loop until batch full / consecutive empty peeks / cancellation
Mediator->>SB: PeekMessagesAsync(max=100, fromSequence?)
SB-->>Mediator: Messages[]
Mediator->>Mediator: Extract OperationId, dedupe, track sequence, wrap-around guard
end
Mediator-->>Client: Result(PeekDlqBatchResult)
sequenceDiagram
participant Client
participant Mediator as Mediator/Handler
participant AppInsights as IAppInsightsService
rect rgba(200,255,200,0.5)
Client->>Mediator: Send(DiagnoseBatchCommand)
end
Mediator->>Mediator: Return empty if no operations
Mediator->>AppInsights: Initialize(AppInsightsResourceId)
Mediator->>AppInsights: DiagnoseBatchAsync(list of (OperationId, EnqueuedTime))
AppInsights-->>Mediator: Dictionary<OperationId, DiagnosticResult>
Mediator->>Mediator: Correlate and enrich DiagnosticResult with MessageId/Subject/DeadLetterReason
Mediator-->>Client: Result<IReadOnlyList<DiagnosticResult>>
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
SummarySummary
CoverageServiceBusToolset.Application - 64.4%
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
src/ServiceBusToolset.Application/DeadLetters/PeekDlq/PeekDlqBatchCommandHandler.cs (1)
128-132: Consider catching specific exceptions instead of barecatch.The broad
catchsilently swallows all exceptions, including unexpected ones likeNullReferenceExceptionorOutOfMemoryException. While the comment explains the intent, catching specific Azure SDK exceptions (e.g.,RequestFailedException) would be more precise.♻️ Proposed refinement
} - catch + catch (Azure.RequestFailedException) { // Admin API may not be available in all environments (e.g., emulator, unit tests) return 0; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ServiceBusToolset.Application/DeadLetters/PeekDlq/PeekDlqBatchCommandHandler.cs` around lines 128 - 132, Replace the bare catch in PeekDlqBatchCommandHandler (the catch block that currently returns 0) with specific exception handlers: catch Azure.RequestFailedException (or the SDK's equivalent) and return 0 as intended for environments where the Admin API is unavailable, and separately catch OperationCanceledException/TaskCanceledException if cancellation is expected; do not swallow critical exceptions—allow fatal exceptions (e.g., OutOfMemoryException, NullReferenceException) to propagate by not catching them or by rethrowing them. Ensure the new handlers preserve the existing comment about emulator/unit tests and log or include the exception message if helpful.tests/ServiceBusToolset.Integration.Tests/DeadLetters/PeekDlqBatchIntegrationShould.cs (1)
108-108: Rename test method to match the repository test naming pattern.
ReturnTotalDeadLetterCountdoes not follow[Action]_When[Condition]. ConsiderReturnTotalDeadLetterCount_WhenDlqHasMessages.As per coding guidelines, test methods in
tests/**should follow[Action]_When[Condition]naming.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/ServiceBusToolset.Integration.Tests/DeadLetters/PeekDlqBatchIntegrationShould.cs` at line 108, Rename the test method ReturnTotalDeadLetterCount to follow the repository naming pattern; change its name to ReturnTotalDeadLetterCount_WhenDlqHasMessages and update any references (e.g., attributes or usages) to the new method name so the test framework and any callers still find it; ensure the method signature and async modifier remain unchanged (method: ReturnTotalDeadLetterCount -> ReturnTotalDeadLetterCount_WhenDlqHasMessages).tests/ServiceBusToolset.Application.Tests/DeadLetters/PeekDlq/PeekDlqBatchCommandHandlerShould.cs (1)
35-35: Standardize non-conforming test method names.These methods don’t match the required
[Action]_When[Condition]pattern:
ExtractOperationIdFromDiagnosticIdDeduplicateOperationIdsReturnLastSequenceNumberPreserveEnqueuedTimeFillBatchAcrossMultipleSubBatchesPlease rename them to the project pattern for consistency and discoverability.
As per coding guidelines, test methods in
tests/**should follow[Action]_When[Condition]naming.Also applies to: 85-85, 113-113, 139-139, 216-216
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/ServiceBusToolset.Application.Tests/DeadLetters/PeekDlq/PeekDlqBatchCommandHandlerShould.cs` at line 35, Rename the non-conforming test methods to the project's [Action]_When[Condition] pattern: change ExtractOperationIdFromDiagnosticId to ExtractOperationId_WhenDiagnosticIdIsProvided, DeduplicateOperationIds to DeduplicateOperationIds_WhenDuplicatesExist, ReturnLastSequenceNumber to ReturnLastSequenceNumber_WhenBatchCompleted, PreserveEnqueuedTime to PreserveEnqueuedTime_WhenMessagesAreEnqueued, and FillBatchAcrossMultipleSubBatches to FillBatchAcrossMultipleSubBatches_WhenSubBatchesAreRequired; update the method names everywhere they’re referenced (test runner, any [Fact]/[Theory] attributes or helper calls) so the tests compile and keep the original behavior and assertions unchanged..github/workflows/stable-release.yml (1)
66-74: Stable release workflow does not publish symbol packages (.snupkg) to NuGet.The Application project is configured with
<IncludeSymbols>true</IncludeSymbols>and<SymbolPackageFormat>snupkg</SymbolPackageFormat>, causing the pack step (lines 71–74) to generate.snupkgfiles. However, the NuGet push command on line 98 only pushes*.nupkgfiles, so symbols are never published.Suggested fix
- name: Push to NuGet env: NUGET_API_KEY: ${{ secrets.NUGET_API_KEY }} run: | if [ -z "$NUGET_API_KEY" ]; then echo "::error::NUGET_API_KEY secret is required for stable releases" exit 1 fi dotnet nuget push ./artifacts/*.nupkg --api-key "$NUGET_API_KEY" --source https://api.nuget.org/v3/index.json --skip-duplicate + dotnet nuget push ./artifacts/*.snupkg --api-key "$NUGET_API_KEY" --source https://api.nuget.org/v3/index.json --skip-duplicate🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/stable-release.yml around lines 66 - 74, The workflow packs the Application project (step "Pack Application", src/ServiceBusToolset.Application) which produces .snupkg symbol packages, but the NuGet push step only pushes *.nupkg; update the push step to also publish .snupkg files (e.g. change the push artifacts pattern to include ./artifacts/*.snupkg or explicitly push both ./artifacts/*.nupkg and ./artifacts/*.snupkg) so symbol packages generated by the Application pack step are uploaded to NuGet..github/workflows/alpha-release.yml (1)
127-130: Publish symbol packages, not only.nupkgfiles.The project is configured to produce
.snupkgfiles (seeIncludeSymbolsandSymbolPackageFormatin the csproj), but the NuGet push step only targets./artifacts/*.nupkg, leaving symbol packages unpublished.Suggested workflow patch
- name: Push to NuGet env: NUGET_API_KEY: ${{ secrets.NUGET_API_KEY }} run: | if [ -z "$NUGET_API_KEY" ]; then echo "::warning::NUGET_API_KEY secret not set, skipping publish" exit 0 fi dotnet nuget push ./artifacts/*.nupkg --api-key "$NUGET_API_KEY" --source https://api.nuget.org/v3/index.json --skip-duplicate + dotnet nuget push ./artifacts/*.snupkg --api-key "$NUGET_API_KEY" --source https://api.nuget.org/v3/index.json --skip-duplicate🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/alpha-release.yml around lines 127 - 130, The NuGet publish step only pushes ./artifacts/*.nupkg so generated symbol packages (.snupkg) are never published; update the publish/push job that runs after the "Pack Application" step to include symbol packages as well (e.g., change the artifact glob from ./artifacts/*.nupkg to include ./artifacts/*.snupkg or run a second push for ./artifacts/*.snupkg), ensuring the workflow pushes both .nupkg and .snupkg (and retains any API key/credential usage already present).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/ServiceBusToolset.Application/DeadLetters/DiagnoseDlq/DiagnoseBatchCommandHandler.cs`:
- Line 32: The ToDictionary call in DiagnoseBatchCommandHandler (var
operationsById = command.Operations.ToDictionary(op => op.OperationId)) can
throw ArgumentException on duplicate OperationId values; update this to handle
duplicates explicitly by deduplicating (e.g., use DistinctBy(op =>
op.OperationId) or GroupBy(op => op.OperationId).Select(g => g.First())) before
building the dictionary, or validate and throw a clearer exception with context
if duplicates are not allowed, and add a brief comment explaining the chosen
approach.
---
Nitpick comments:
In @.github/workflows/alpha-release.yml:
- Around line 127-130: The NuGet publish step only pushes ./artifacts/*.nupkg so
generated symbol packages (.snupkg) are never published; update the publish/push
job that runs after the "Pack Application" step to include symbol packages as
well (e.g., change the artifact glob from ./artifacts/*.nupkg to include
./artifacts/*.snupkg or run a second push for ./artifacts/*.snupkg), ensuring
the workflow pushes both .nupkg and .snupkg (and retains any API key/credential
usage already present).
In @.github/workflows/stable-release.yml:
- Around line 66-74: The workflow packs the Application project (step "Pack
Application", src/ServiceBusToolset.Application) which produces .snupkg symbol
packages, but the NuGet push step only pushes *.nupkg; update the push step to
also publish .snupkg files (e.g. change the push artifacts pattern to include
./artifacts/*.snupkg or explicitly push both ./artifacts/*.nupkg and
./artifacts/*.snupkg) so symbol packages generated by the Application pack step
are uploaded to NuGet.
In
`@src/ServiceBusToolset.Application/DeadLetters/PeekDlq/PeekDlqBatchCommandHandler.cs`:
- Around line 128-132: Replace the bare catch in PeekDlqBatchCommandHandler (the
catch block that currently returns 0) with specific exception handlers: catch
Azure.RequestFailedException (or the SDK's equivalent) and return 0 as intended
for environments where the Admin API is unavailable, and separately catch
OperationCanceledException/TaskCanceledException if cancellation is expected; do
not swallow critical exceptions—allow fatal exceptions (e.g.,
OutOfMemoryException, NullReferenceException) to propagate by not catching them
or by rethrowing them. Ensure the new handlers preserve the existing comment
about emulator/unit tests and log or include the exception message if helpful.
In
`@tests/ServiceBusToolset.Application.Tests/DeadLetters/PeekDlq/PeekDlqBatchCommandHandlerShould.cs`:
- Line 35: Rename the non-conforming test methods to the project's
[Action]_When[Condition] pattern: change ExtractOperationIdFromDiagnosticId to
ExtractOperationId_WhenDiagnosticIdIsProvided, DeduplicateOperationIds to
DeduplicateOperationIds_WhenDuplicatesExist, ReturnLastSequenceNumber to
ReturnLastSequenceNumber_WhenBatchCompleted, PreserveEnqueuedTime to
PreserveEnqueuedTime_WhenMessagesAreEnqueued, and
FillBatchAcrossMultipleSubBatches to
FillBatchAcrossMultipleSubBatches_WhenSubBatchesAreRequired; update the method
names everywhere they’re referenced (test runner, any [Fact]/[Theory] attributes
or helper calls) so the tests compile and keep the original behavior and
assertions unchanged.
In
`@tests/ServiceBusToolset.Integration.Tests/DeadLetters/PeekDlqBatchIntegrationShould.cs`:
- Line 108: Rename the test method ReturnTotalDeadLetterCount to follow the
repository naming pattern; change its name to
ReturnTotalDeadLetterCount_WhenDlqHasMessages and update any references (e.g.,
attributes or usages) to the new method name so the test framework and any
callers still find it; ensure the method signature and async modifier remain
unchanged (method: ReturnTotalDeadLetterCount ->
ReturnTotalDeadLetterCount_WhenDlqHasMessages).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1772e8cd-d720-4f49-a9ec-dd35619de3a1
📒 Files selected for processing (13)
.github/workflows/alpha-release.yml.github/workflows/stable-release.ymlsrc/ServiceBusToolset.Application/DeadLetters/DiagnoseDlq/DiagnoseBatchCommand.cssrc/ServiceBusToolset.Application/DeadLetters/DiagnoseDlq/DiagnoseBatchCommandHandler.cssrc/ServiceBusToolset.Application/DeadLetters/PeekDlq/PeekDlqBatchCommand.cssrc/ServiceBusToolset.Application/DeadLetters/PeekDlq/PeekDlqBatchCommandHandler.cssrc/ServiceBusToolset.Application/DeadLetters/PeekDlq/PeekDlqBatchResult.cssrc/ServiceBusToolset.Application/ServiceBusToolset.Application.csprojtests/ServiceBusToolset.Application.Tests/Common/Mocks/MockServiceBusClientFactory.cstests/ServiceBusToolset.Application.Tests/DeadLetters/DiagnoseDlq/DiagnoseBatchCommandHandlerShould.cstests/ServiceBusToolset.Application.Tests/DeadLetters/PeekDlq/PeekDlqBatchCommandHandlerShould.cstests/ServiceBusToolset.Integration.Tests/DeadLetters/DiagnoseBatchIntegrationShould.cstests/ServiceBusToolset.Integration.Tests/DeadLetters/PeekDlqBatchIntegrationShould.cs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/ServiceBusToolset.Application/DeadLetters/DiagnoseDlq/DiagnoseBatchCommandHandler.cs (1)
22-34: Deduplicate before calling App Insights to avoid redundant batch items.At Line 22, duplicates are still sent to
DiagnoseBatchAsync; dedupe is only applied later for enrichment lookup. Consider buildingoperationsByIdfirst and deriving the outbound list from it.♻️ Suggested refactor
- var operations = command.Operations - .Select(op => (op.OperationId, op.EnqueuedTime)) - .ToList(); + // Deduplicate by OperationId — take the first occurrence if duplicates exist + var operationsById = command.Operations + .DistinctBy(op => op.OperationId) + .ToDictionary(op => op.OperationId); + + var operations = operationsById.Values + .Select(op => (op.OperationId, op.EnqueuedTime)) + .ToList(); var diagnosticResults = await appInsightsService.DiagnoseBatchAsync(operations, null, cancellationToken); var results = new List<DiagnosticResult>(); - // Deduplicate by OperationId — take the first occurrence if duplicates exist - var operationsById = command.Operations - .DistinctBy(op => op.OperationId) - .ToDictionary(op => op.OperationId);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ServiceBusToolset.Application/DeadLetters/DiagnoseDlq/DiagnoseBatchCommandHandler.cs` around lines 22 - 34, The code currently builds `operations` from `command.Operations` and sends duplicates to `appInsightsService.DiagnoseBatchAsync`; instead, deduplicate first by `OperationId` using the same logic used later (e.g., create `operationsById = command.Operations.DistinctBy(op => op.OperationId).ToDictionary(op => op.OperationId)`), then derive the outbound list (tuple of `(OperationId, EnqueuedTime)`) from `operationsById.Values` and pass that deduplicated list into `appInsightsService.DiagnoseBatchAsync` (keeping the `cancellationToken`), and finally continue using `operationsById` for enrichment and building `results/DiagnosticResult`.tests/ServiceBusToolset.Application.Tests/DeadLetters/PeekDlq/PeekDlqBatchCommandHandlerShould.cs (1)
241-262: Consider adding test coverage forFromSequenceNumberparameter.The
CreateCommandhelper acceptsfromSequenceNumber, but no test exercises this pagination scenario. A test verifying that messages are peeked starting from the specified sequence number would improve confidence in the pagination logic.💡 Example test for FromSequenceNumber
[Fact] public async Task StartPeekingFromSequenceNumber_WhenFromSequenceNumberProvided() { var messages = new[] { ServiceBusReceivedMessageBuilder.Create() .WithMessageId("msg-1") .WithDiagnosticId("abc123def456abc123def456abc12345") .WithSequenceNumber(100) .Build(), ServiceBusReceivedMessageBuilder.Create() .WithMessageId("msg-2") .WithDiagnosticId("def456abc123def456abc123def45678") .WithSequenceNumber(101) .Build() }; _mockFactory.WithMessagesToReturn(messages); // Start from sequence 50 - should still get all messages since mock doesn't filter var command = CreateCommand(batchSize: 500, fromSequenceNumber: 50); var result = await _handler.Handle(command, CancellationToken.None); result.IsSuccess.ShouldBeTrue(); result.Value.Messages.Count.ShouldBe(2); result.Value.LastSequenceNumber.ShouldBe(101); }Note: The current mock ignores
fromSequenceNumber(per context snippet 2), so this would primarily verify the handler's flow rather than actual sequence filtering. Consider updating the mock to honorfromSequenceNumberfor more realistic behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/ServiceBusToolset.Application.Tests/DeadLetters/PeekDlq/PeekDlqBatchCommandHandlerShould.cs` around lines 241 - 262, Add a unit test exercising the fromSequenceNumber pagination path by using the existing CreateCommand helper to construct a PeekDlqBatchCommand with a non-null fromSequenceNumber, arrange messages via _mockFactory.WithMessagesToReturn (create ServiceBusReceivedMessage instances with specific SequenceNumber values using ServiceBusReceivedMessageBuilder), call _handler.Handle(command, CancellationToken.None), and assert the returned result.IsSuccess, result.Value.Messages.Count and result.Value.LastSequenceNumber reflect the expected start/offset behavior; if you want stricter validation also update the mock factory to honor the fromSequenceNumber filter so the test verifies actual filtering logic rather than only the handler flow.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/ServiceBusToolset.Application/DeadLetters/PeekDlq/PeekDlqBatchCommandHandler.cs`:
- Around line 118-145: Update GetDeadLetterCountAsync to accept and propagate a
CancellationToken and pass it into the Azure admin API calls: modify the method
signature of GetDeadLetterCountAsync(IServiceBusClientFactory clientFactory,
string fullyQualifiedNamespace, EntityTarget target, CancellationToken
cancellationToken), and then call
adminClient.GetQueueRuntimePropertiesAsync(target.Queue!, cancellationToken) and
adminClient.GetSubscriptionRuntimePropertiesAsync(target.Topic!,
target.Subscription!, cancellationToken); also remove the
OperationCanceledException catch or keep it as needed, and update all call sites
to supply the appropriate CancellationToken when invoking
GetDeadLetterCountAsync.
In
`@tests/ServiceBusToolset.Application.Tests/DeadLetters/DiagnoseDlq/DiagnoseBatchCommandHandlerShould.cs`:
- Line 33: Rename the test methods to follow the [Action]_When[Condition]
convention: change the method named InitializeAppInsightsService to a
descriptive name like
InitializeAppInsightsService_WhenCalled_RegistersAppInsights (or similar) and
similarly rename the three other test methods at the indicated locations (lines
53, 102, 153) to match [Action]_When[Condition]; update each method declaration
(e.g., the async Task method signatures such as InitializeAppInsightsService)
and any references or test attributes to use the new names so the tests compile
and follow the coding guideline.
---
Nitpick comments:
In
`@src/ServiceBusToolset.Application/DeadLetters/DiagnoseDlq/DiagnoseBatchCommandHandler.cs`:
- Around line 22-34: The code currently builds `operations` from
`command.Operations` and sends duplicates to
`appInsightsService.DiagnoseBatchAsync`; instead, deduplicate first by
`OperationId` using the same logic used later (e.g., create `operationsById =
command.Operations.DistinctBy(op => op.OperationId).ToDictionary(op =>
op.OperationId)`), then derive the outbound list (tuple of `(OperationId,
EnqueuedTime)`) from `operationsById.Values` and pass that deduplicated list
into `appInsightsService.DiagnoseBatchAsync` (keeping the `cancellationToken`),
and finally continue using `operationsById` for enrichment and building
`results/DiagnosticResult`.
In
`@tests/ServiceBusToolset.Application.Tests/DeadLetters/PeekDlq/PeekDlqBatchCommandHandlerShould.cs`:
- Around line 241-262: Add a unit test exercising the fromSequenceNumber
pagination path by using the existing CreateCommand helper to construct a
PeekDlqBatchCommand with a non-null fromSequenceNumber, arrange messages via
_mockFactory.WithMessagesToReturn (create ServiceBusReceivedMessage instances
with specific SequenceNumber values using ServiceBusReceivedMessageBuilder),
call _handler.Handle(command, CancellationToken.None), and assert the returned
result.IsSuccess, result.Value.Messages.Count and
result.Value.LastSequenceNumber reflect the expected start/offset behavior; if
you want stricter validation also update the mock factory to honor the
fromSequenceNumber filter so the test verifies actual filtering logic rather
than only the handler flow.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b1363d8b-5dba-42ea-9a39-abf2453dd3c5
📒 Files selected for processing (9)
.github/workflows/alpha-release.yml.github/workflows/stable-release.ymlsrc/ServiceBusToolset.Application/DeadLetters/DiagnoseDlq/DiagnoseBatchCommand.cssrc/ServiceBusToolset.Application/DeadLetters/DiagnoseDlq/DiagnoseBatchCommandHandler.cssrc/ServiceBusToolset.Application/DeadLetters/PeekDlq/PeekDlqBatchCommandHandler.cstests/ServiceBusToolset.Application.Tests/DeadLetters/DiagnoseDlq/DiagnoseBatchCommandHandlerShould.cstests/ServiceBusToolset.Application.Tests/DeadLetters/PeekDlq/PeekDlqBatchCommandHandlerShould.cstests/ServiceBusToolset.Integration.Tests/DeadLetters/DiagnoseBatchIntegrationShould.cstests/ServiceBusToolset.Integration.Tests/DeadLetters/PeekDlqBatchIntegrationShould.cs
✅ Files skipped from review due to trivial changes (1)
- src/ServiceBusToolset.Application/DeadLetters/DiagnoseDlq/DiagnoseBatchCommand.cs
🚧 Files skipped from review as they are similar to previous changes (4)
- .github/workflows/stable-release.yml
- .github/workflows/alpha-release.yml
- tests/ServiceBusToolset.Integration.Tests/DeadLetters/DiagnoseBatchIntegrationShould.cs
- tests/ServiceBusToolset.Integration.Tests/DeadLetters/PeekDlqBatchIntegrationShould.cs
Comment on lines
+118
to
+145
| private static async Task<long> GetDeadLetterCountAsync( | ||
| IServiceBusClientFactory clientFactory, | ||
| string fullyQualifiedNamespace, | ||
| EntityTarget target) | ||
| { | ||
| try | ||
| { | ||
| var adminClient = clientFactory.CreateAdministrationClient(fullyQualifiedNamespace); | ||
|
|
||
| if (target.IsQueueMode) | ||
| { | ||
| var props = await adminClient.GetQueueRuntimePropertiesAsync(target.Queue!); | ||
| return props.Value.DeadLetterMessageCount; | ||
| } | ||
|
|
||
| var subProps = await adminClient.GetSubscriptionRuntimePropertiesAsync(target.Topic!, target.Subscription!); | ||
| return subProps.Value.DeadLetterMessageCount; | ||
| } | ||
| catch (Azure.RequestFailedException) | ||
| { | ||
| // Admin API may not be available in all environments (e.g., emulator, unit tests) | ||
| return 0; | ||
| } | ||
| catch (OperationCanceledException) | ||
| { | ||
| return 0; | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Missing CancellationToken propagation to admin API calls.
The admin API methods accept a CancellationToken, but this method neither accepts one nor passes it to the underlying calls. While the OperationCanceledException catch handles post-hoc cancellation, the calls themselves will run to completion even if the parent operation is cancelled.
🛠️ Proposed fix to propagate cancellation token
private static async Task<long> GetDeadLetterCountAsync(
IServiceBusClientFactory clientFactory,
string fullyQualifiedNamespace,
- EntityTarget target)
+ EntityTarget target,
+ CancellationToken cancellationToken)
{
try
{
var adminClient = clientFactory.CreateAdministrationClient(fullyQualifiedNamespace);
if (target.IsQueueMode)
{
- var props = await adminClient.GetQueueRuntimePropertiesAsync(target.Queue!);
+ var props = await adminClient.GetQueueRuntimePropertiesAsync(target.Queue!, cancellationToken);
return props.Value.DeadLetterMessageCount;
}
- var subProps = await adminClient.GetSubscriptionRuntimePropertiesAsync(target.Topic!, target.Subscription!);
+ var subProps = await adminClient.GetSubscriptionRuntimePropertiesAsync(target.Topic!, target.Subscription!, cancellationToken);
return subProps.Value.DeadLetterMessageCount;
}And update the call site at line 25:
var totalDeadLetterCount = command.KnownDeadLetterCount
- ?? await GetDeadLetterCountAsync(clientFactory, command.FullyQualifiedNamespace, command.Target);
+ ?? await GetDeadLetterCountAsync(clientFactory, command.FullyQualifiedNamespace, command.Target, cancellationToken);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/ServiceBusToolset.Application/DeadLetters/PeekDlq/PeekDlqBatchCommandHandler.cs`
around lines 118 - 145, Update GetDeadLetterCountAsync to accept and propagate a
CancellationToken and pass it into the Azure admin API calls: modify the method
signature of GetDeadLetterCountAsync(IServiceBusClientFactory clientFactory,
string fullyQualifiedNamespace, EntityTarget target, CancellationToken
cancellationToken), and then call
adminClient.GetQueueRuntimePropertiesAsync(target.Queue!, cancellationToken) and
adminClient.GetSubscriptionRuntimePropertiesAsync(target.Topic!,
target.Subscription!, cancellationToken); also remove the
OperationCanceledException catch or keep it as needed, and update all call sites
to supply the appropriate CancellationToken when invoking
GetDeadLetterCountAsync.
| } | ||
|
|
||
| [Fact] | ||
| public async Task InitializeAppInsightsService() |
Contributor
There was a problem hiding this comment.
Rename test methods to match the required [Action]_When[Condition] format.
Line 33, Line 53, Line 102, and Line 153 currently use names outside the required pattern.
✏️ Suggested renames
- public async Task InitializeAppInsightsService()
+ public async Task InitializeAppInsightsService_WhenOperationsExist()
- public async Task ReturnDiagnosticResults_WithEnrichedMessageInfo()
+ public async Task ReturnDiagnosticResults_WhenMessageInfoNeedsEnrichment()
- public async Task HandleMultipleOperations()
+ public async Task ReturnDiagnosticResults_WhenMultipleOperationsProvided()
- public async Task PassCorrectOperationsToAppInsights()
+ public async Task PassCorrectOperationsToAppInsights_WhenHandlingCommand()As per coding guidelines tests/**: Test methods follow [Action]_When[Condition] naming.
Also applies to: 53-53, 102-102, 153-153
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@tests/ServiceBusToolset.Application.Tests/DeadLetters/DiagnoseDlq/DiagnoseBatchCommandHandlerShould.cs`
at line 33, Rename the test methods to follow the [Action]_When[Condition]
convention: change the method named InitializeAppInsightsService to a
descriptive name like
InitializeAppInsightsService_WhenCalled_RegistersAppInsights (or similar) and
similarly rename the three other test methods at the indicated locations (lines
53, 102, 153) to match [Action]_When[Condition]; update each method declaration
(e.g., the async Task method signatures such as InitializeAppInsightsService)
and any references or test attributes to use the new names so the tests compile
and follow the coding guideline.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/ServiceBusToolset.Application.Tests/DeadLetters/PeekDlq/PeekDlqBatchCommandHandlerShould.cs (1)
215-260: Add a unit test for the explicitFromSequenceNumberpath.Nothing in this suite currently asserts the first-peek branch in
PeekDlqBatchCommandHandlerat Lines 44-52 or the wrap-around break at Lines 61-66. One focused test usingCreateCommand(fromSequenceNumber: ...)would lock down that paging contract.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/ServiceBusToolset.Application.Tests/DeadLetters/PeekDlq/PeekDlqBatchCommandHandlerShould.cs` around lines 215 - 260, Add a focused unit test that exercises the explicit FromSequenceNumber path by calling CreateCommand(fromSequenceNumber: <someSequence>) and invoking PeekDlqBatchCommandHandler.Handle so the handler hits the first-peek branch in PeekDlqBatchCommandHandler and exercises the wrap-around break logic; the test should configure the mock factory (_mockFactory.WithMessagesToReturn or WithNoMessages) to return paged messages around the provided sequence number and assert that the resulting result.Value.PeekedInBatch, result.Value.Messages (content and count), result.Value.HasMoreMessages and result.Value.TotalDeadLetterCount reflect starting from the provided sequence number and that the handler breaks out correctly (no infinite loop and correct paging behavior).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/ServiceBusToolset.Application/DeadLetters/PeekDlq/PeekDlqBatchCommandHandler.cs`:
- Around line 36-38: The loop in PeekDlqBatchCommandHandler.Handle currently
treats cancellation as normal exhaustion because it only breaks the loop on
cancellation; update the handler so that after the loop you check
cancellationToken.IsCancellationRequested and, if set, short-circuit with an
appropriate canceled response (e.g., throw OperationCanceledException or return
a Result/Response indicating cancellation) instead of falling through to the
success return paths that return empty/partial data (the success-returning
blocks currently used around the "empty/partial" returns); ensure both success
return sites that return empty results are guarded by this cancellation check so
cancellation is distinguishable from "no more messages."
- Around line 107-109: The hasMore calculation in PeekDlqBatchCommandHandler.cs
(using allMessages, command.BatchSize and emptyBatches) can return true on a
full first page even when there are no more dead letters; change hasMore to use
the resolved total dead-letter count (the variable assigned at Line 25, e.g.,
totalDeadLetterCount/resolvedCount) together with the current offset/skip to
determine if more rows exist—i.e., compute hasMore by checking
totalDeadLetterCount > (currentOffset + allMessages.Count) and still require
emptyBatches < EmptyBatchThreshold, instead of relying solely on
allMessages.Count >= command.BatchSize.
---
Nitpick comments:
In
`@tests/ServiceBusToolset.Application.Tests/DeadLetters/PeekDlq/PeekDlqBatchCommandHandlerShould.cs`:
- Around line 215-260: Add a focused unit test that exercises the explicit
FromSequenceNumber path by calling CreateCommand(fromSequenceNumber:
<someSequence>) and invoking PeekDlqBatchCommandHandler.Handle so the handler
hits the first-peek branch in PeekDlqBatchCommandHandler and exercises the
wrap-around break logic; the test should configure the mock factory
(_mockFactory.WithMessagesToReturn or WithNoMessages) to return paged messages
around the provided sequence number and assert that the resulting
result.Value.PeekedInBatch, result.Value.Messages (content and count),
result.Value.HasMoreMessages and result.Value.TotalDeadLetterCount reflect
starting from the provided sequence number and that the handler breaks out
correctly (no infinite loop and correct paging behavior).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 275a95d1-9f66-480e-8942-4dda5c88db14
📒 Files selected for processing (2)
src/ServiceBusToolset.Application/DeadLetters/PeekDlq/PeekDlqBatchCommandHandler.cstests/ServiceBusToolset.Application.Tests/DeadLetters/PeekDlq/PeekDlqBatchCommandHandlerShould.cs
Comment on lines
+36
to
+38
| while (allMessages.Count < command.BatchSize && | ||
| emptyBatches < EmptyBatchThreshold && | ||
| !cancellationToken.IsCancellationRequested) |
Contributor
There was a problem hiding this comment.
Don't treat cancellation as normal exhaustion.
The IsCancellationRequested guard at Line 38 only exits the loop, so a canceled request falls through to the success returns at Lines 73-80 or Lines 111-116 with empty/partial data. That makes cancellation indistinguishable from “no more messages”.
🛠️ Suggested change
- while (allMessages.Count < command.BatchSize &&
- emptyBatches < EmptyBatchThreshold &&
- !cancellationToken.IsCancellationRequested)
+ while (allMessages.Count < command.BatchSize &&
+ emptyBatches < EmptyBatchThreshold)
{
+ cancellationToken.ThrowIfCancellationRequested();
var remaining = command.BatchSize - allMessages.Count;
var subBatchSize = Math.Min(PeekSubBatchSize, remaining);Also applies to: 73-80, 111-116
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/ServiceBusToolset.Application/DeadLetters/PeekDlq/PeekDlqBatchCommandHandler.cs`
around lines 36 - 38, The loop in PeekDlqBatchCommandHandler.Handle currently
treats cancellation as normal exhaustion because it only breaks the loop on
cancellation; update the handler so that after the loop you check
cancellationToken.IsCancellationRequested and, if set, short-circuit with an
appropriate canceled response (e.g., throw OperationCanceledException or return
a Result/Response indicating cancellation) instead of falling through to the
success return paths that return empty/partial data (the success-returning
blocks currently used around the "empty/partial" returns); ensure both success
return sites that return empty results are guarded by this cancellation check so
cancellation is distinguishable from "no more messages."
Comment on lines
+107
to
+109
| var lastSequenceNumber = allMessages[^1].SequenceNumber; | ||
| // We have more messages if we filled the batch (didn't run out early) | ||
| var hasMore = allMessages.Count >= command.BatchSize && emptyBatches < EmptyBatchThreshold; |
Contributor
There was a problem hiding this comment.
HasMoreMessages can be a false positive on a full first page.
Line 109 ignores the resolved count from Line 25. If the first page returns exactly BatchSize messages and totalDeadLetterCount is the same value, this still reports HasMoreMessages = true, which sends the caller to a guaranteed-empty next page.
🧭 Suggested change
- var hasMore = allMessages.Count >= command.BatchSize && emptyBatches < EmptyBatchThreshold;
+ var hasMore = command.FromSequenceNumber is null && totalDeadLetterCount > 0
+ ? allMessages.Count < totalDeadLetterCount
+ : allMessages.Count >= command.BatchSize && emptyBatches < EmptyBatchThreshold;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| var lastSequenceNumber = allMessages[^1].SequenceNumber; | |
| // We have more messages if we filled the batch (didn't run out early) | |
| var hasMore = allMessages.Count >= command.BatchSize && emptyBatches < EmptyBatchThreshold; | |
| var lastSequenceNumber = allMessages[^1].SequenceNumber; | |
| // We have more messages if we filled the batch (didn't run out early) | |
| var hasMore = command.FromSequenceNumber is null && totalDeadLetterCount > 0 | |
| ? allMessages.Count < totalDeadLetterCount | |
| : allMessages.Count >= command.BatchSize && emptyBatches < EmptyBatchThreshold; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/ServiceBusToolset.Application/DeadLetters/PeekDlq/PeekDlqBatchCommandHandler.cs`
around lines 107 - 109, The hasMore calculation in PeekDlqBatchCommandHandler.cs
(using allMessages, command.BatchSize and emptyBatches) can return true on a
full first page even when there are no more dead letters; change hasMore to use
the resolved total dead-letter count (the variable assigned at Line 25, e.g.,
totalDeadLetterCount/resolvedCount) together with the current offset/skip to
determine if more rows exist—i.e., compute hasMore by checking
totalDeadLetterCount > (currentOffset + allMessages.Count) and still require
emptyBatches < EmptyBatchThreshold, instead of relying solely on
allMessages.Count >= command.BatchSize.
Moved the Initialize call ahead of PeekAsync so it fires even when the queue turns out to be empty, preserving the fail-fast behaviour and restoring the existing tests. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Tests
Chores
Refactor