Migrate dashboard endpoints to Immediate.Apis - #84
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe dashboard now registers services before application build and maps generated API endpoints afterward. It adds typed dashboard routes, validation problem details, mutation handling, telemetry links, and dashboard and batch SSE streams. ChangesDashboard API
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant DashboardApiEndpoints
participant IJobStorage
participant JobMutations
Client->>DashboardApiEndpoints: Send dashboard request
DashboardApiEndpoints->>IJobStorage: Query dashboard data
DashboardApiEndpoints->>JobMutations: Apply requested mutation
IJobStorage-->>DashboardApiEndpoints: Return data or missing-resource result
JobMutations-->>DashboardApiEndpoints: Return mutation result
DashboardApiEndpoints-->>Client: Return typed response or problem details
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
2996c18 to
4c06ba3
Compare
dcbb6ab to
5e6c14c
Compare
Coverage Report for CI Build 30663166872Coverage decreased (-0.3%) to 83.799%Details
Uncovered Changes
Coverage Regressions1 previously-covered line in 1 file lost coverage.
Coverage Stats
💛 - Coveralls |
5e6c14c to
ce6f738
Compare
ce6f738 to
ce641c2
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
src/Immediate.Jobs.Dashboard/ImmediateJobsDashboardServiceCollectionExtensions.cs (1)
19-27: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the options registration idempotent.
AddSingleton(options)appends a new registration on every call. If an application callsAddImmediateJobsDashboardtwice, for example once in a shared extension and once inProgram.cs, the last registration wins and every telemetry link and authorization policy configured by the first call is discarded silently. UseTryAddSingletonand applyconfigureto the already-registered instance, or throw on a second registration.♻️ Proposed change
- var options = new ImmediateJobsDashboardOptions(); - configure?.Invoke(options); - options.Validate(); - - _ = services.AddSingleton(options); + var options = (ImmediateJobsDashboardOptions?)services + .FirstOrDefault(d => d.ServiceType == typeof(ImmediateJobsDashboardOptions))?.ImplementationInstance + ?? new ImmediateJobsDashboardOptions(); + configure?.Invoke(options); + options.Validate(); + + services.TryAddSingleton(options);This requires
using Microsoft.Extensions.DependencyInjection.Extensions;.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Immediate.Jobs.Dashboard/ImmediateJobsDashboardServiceCollectionExtensions.cs` around lines 19 - 27, Make options registration idempotent in AddImmediateJobsDashboard by using TryAddSingleton with the existing options instance and importing Microsoft.Extensions.DependencyInjection.Extensions; ensure repeated calls preserve the initially registered configuration rather than silently replacing it.src/Immediate.Jobs.Dashboard/DashboardApiEndpoints.cs (2)
703-716: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
MutateJobAsyncandMutateBatchAsyncare identical.Both delegate to
MutateAsync(operation, includeConflict: true). Two names for one behavior invite divergence later. KeepMutateRecurringAsyncand replace the other two with a singleMutateAsync(operation, includeConflict: true)call at each site, or expose one method with the flag.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Immediate.Jobs.Dashboard/DashboardApiEndpoints.cs` around lines 703 - 716, Remove the redundant MutateJobAsync and MutateBatchAsync wrappers, and update their callers to invoke MutateAsync with includeConflict: true directly. Preserve MutateRecurringAsync with includeConflict: false and retain the existing behavior at each call site.
758-777: 🚀 Performance & Scalability | 🔵 Trivial | 🏗️ Heavy liftConsider the storage cost of the dashboard SSE loop.
Each connected client polls independently. Every iteration issues one monitoring snapshot query, one 100-row job query, and one 100-row batch query. At the default 2-second
UpdateInterval, ten open dashboard tabs generate about fifteen storage queries per second against the jobs database. Consider a shared background snapshot that all SSE connections read, or a per-process cache keyed by interval, so the query rate stays independent of client count.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Immediate.Jobs.Dashboard/DashboardApiEndpoints.cs` around lines 758 - 777, Reduce storage load in the dashboard SSE loop around GetMonitoringSnapshotAsync, QueryJobsAsync, and QueryBatchesAsync by introducing a shared background snapshot or per-process cache keyed by the update interval. Have each connected client read the cached DashboardState instead of issuing storage queries independently, while preserving the existing SSE serialization, event format, flushing, cancellation, and interval behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/Immediate.Jobs.Dashboard/DashboardApiEndpoints.cs`:
- Around line 409-414: Add [FromRoute] to the BatchId property in
DeleteDashboardBatch.Command and the JobId property in
DeleteDashboardJob.Command, alongside their existing [NotEmpty] attributes, so
DELETE route values bind correctly; also add functional coverage for DELETE
/api/batches/{id} and DELETE /api/jobs/{id}.
In
`@src/Immediate.Jobs.Dashboard/ImmediateJobsDashboardEndpointRouteBuilderExtensions.cs`:
- Around line 45-46: Prevent map-time configuration from mutating the shared
options singleton in the endpoint route-mapping flow around
configure?.Invoke(options) and options.Validate(). Prefer cloning the registered
options for each mapped dashboard group before applying configure and
validating, so repeated mappings do not duplicate telemetry links and active
requests retain their original settings.
---
Nitpick comments:
In `@src/Immediate.Jobs.Dashboard/DashboardApiEndpoints.cs`:
- Around line 703-716: Remove the redundant MutateJobAsync and MutateBatchAsync
wrappers, and update their callers to invoke MutateAsync with includeConflict:
true directly. Preserve MutateRecurringAsync with includeConflict: false and
retain the existing behavior at each call site.
- Around line 758-777: Reduce storage load in the dashboard SSE loop around
GetMonitoringSnapshotAsync, QueryJobsAsync, and QueryBatchesAsync by introducing
a shared background snapshot or per-process cache keyed by the update interval.
Have each connected client read the cached DashboardState instead of issuing
storage queries independently, while preserving the existing SSE serialization,
event format, flushing, cancellation, and interval behavior.
In
`@src/Immediate.Jobs.Dashboard/ImmediateJobsDashboardServiceCollectionExtensions.cs`:
- Around line 19-27: Make options registration idempotent in
AddImmediateJobsDashboard by using TryAddSingleton with the existing options
instance and importing Microsoft.Extensions.DependencyInjection.Extensions;
ensure repeated calls preserve the initially registered configuration rather
than silently replacing it.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 3ba8eaeb-f7d1-494c-a671-84ef26bcd2ff
📒 Files selected for processing (13)
Directory.Packages.propsdocs/monitoring-api.mdreadme.mdsamples/Aspire/Api/Program.cssamples/Basic/Program.cssrc/Immediate.Jobs.Dashboard/DashboardApiEndpoints.cssrc/Immediate.Jobs.Dashboard/DashboardValidationFilter.cssrc/Immediate.Jobs.Dashboard/Immediate.Jobs.Dashboard.csprojsrc/Immediate.Jobs.Dashboard/ImmediateJobsDashboardEndpointRouteBuilderExtensions.cssrc/Immediate.Jobs.Dashboard/ImmediateJobsDashboardOptions.cssrc/Immediate.Jobs.Dashboard/ImmediateJobsDashboardServiceCollectionExtensions.cstests/Immediate.Jobs.FunctionalTests/GeneratedJobTests.cstests/Immediate.Jobs.FunctionalTests/Packages/DashboardPackageTests.cs
| [Validate] | ||
| internal sealed partial record Command : IValidationTarget<Command> | ||
| { | ||
| [NotEmpty] | ||
| public required string BatchId { get; init; } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Both DELETE commands omit [FromRoute] on their route property. Every other mutation command in this file declares [FromRoute] (lines 388, 477, 520, 574, 598). For non-GET routes, Immediate.Apis binds the request record from the request body unless a member declares its binding source, and a DELETE request carries no body, so the route value never reaches the command.
src/Immediate.Jobs.Dashboard/DashboardApiEndpoints.cs#L409-L414: add[FromRoute]above[NotEmpty]onDeleteDashboardBatch.Command.BatchId.src/Immediate.Jobs.Dashboard/DashboardApiEndpoints.cs#L496-L501: add[FromRoute]above[NotEmpty]onDeleteDashboardJob.Command.JobId.
Add functional test coverage for DELETE /api/jobs/{id} and DELETE /api/batches/{id}, because both paths are currently uncovered according to the reported patch coverage.
📍 Affects 1 file
src/Immediate.Jobs.Dashboard/DashboardApiEndpoints.cs#L409-L414(this comment)src/Immediate.Jobs.Dashboard/DashboardApiEndpoints.cs#L496-L501
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/Immediate.Jobs.Dashboard/DashboardApiEndpoints.cs` around lines 409 -
414, Add [FromRoute] to the BatchId property in DeleteDashboardBatch.Command and
the JobId property in DeleteDashboardJob.Command, alongside their existing
[NotEmpty] attributes, so DELETE route values bind correctly; also add
functional coverage for DELETE /api/batches/{id} and DELETE /api/jobs/{id}.
| configure?.Invoke(options); | ||
| if (options.UpdateInterval <= TimeSpan.Zero) | ||
| throw new ArgumentOutOfRangeException(nameof(configure), "The dashboard update interval must be positive."); | ||
| options.Validate(); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Map-time configure mutates the shared options singleton.
options is the singleton instance registered by AddImmediateJobsDashboard. This call mutates it after the container is built. Two consequences follow:
- If an application maps the dashboard at two prefixes,
AddTelemetryLinkappends the same links again, so every job-detail page shows duplicated links. - The mutation is visible to any request already resolving the singleton, including active SSE streams that read
options.UpdateInterval.
Consider treating the map-time configure as deprecated and directing configuration to AddImmediateJobsDashboard, or clone the options instance for the group being mapped.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@src/Immediate.Jobs.Dashboard/ImmediateJobsDashboardEndpointRouteBuilderExtensions.cs`
around lines 45 - 46, Prevent map-time configuration from mutating the shared
options singleton in the endpoint route-mapping flow around
configure?.Invoke(options) and options.Validate(). Prefer cloning the registered
options for each mapped dashboard group before applying configure and
validating, so repeated mappings do not duplicate telemetry links and active
requests retain their original settings.
3 participants
Stack
feature/job-retention)Summary
AddImmediateJobsDashboardregistration while preserving the existing prefix, authorization, SPA, JSON, and SSE behaviorVerification
dotnet build Immediate.Jobs.slnx -c Release -p:TreatWarningsAsErrors=true -v:minimaldotnet test tests/Immediate.Jobs.FunctionalTests/Immediate.Jobs.FunctionalTests.csproj -c Release --no-restore -p:TreatWarningsAsErrors=true -v:minimal(748 passed across net8.0, net9.0, net10.0, and net11.0)dotnet pack src/Immediate.Jobs.Dashboard/Immediate.Jobs.Dashboard.csproj -c Release --no-buildgit diff --checkSummary by CodeRabbit
New Features
Bug Fixes
400 Bad Requestresponses.Documentation