LEGLINK-788: Improve Generated Patient Storage and Uploaded Patient Processing. - #1798
Open
nvmLantana wants to merge 6 commits into
Open
LEGLINK-788: Improve Generated Patient Storage and Uploaded Patient Processing.#1798nvmLantana wants to merge 6 commits into
nvmLantana wants to merge 6 commits into
Conversation
…e process uploaded patient files to avoid timeouts.
Contributor
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe PR adds durable generated-patient template caching and version tracking, external imported-bundle resolution, asynchronous patient replacement with status polling, and related persistence and runtime updates. ChangesGenerated template caching
Imported bundle execution
Asynchronous patient replacement
Runtime and controller maintenance
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant RunExecutor
participant FhirGenerationPipeline
participant MongoGeneratedPatientTemplateCache
participant GeneratedTemplateCacheVersionStore
participant MongoSnapshotStore
RunExecutor->>FhirGenerationPipeline: Generate and upload with cache
FhirGenerationPipeline->>MongoGeneratedPatientTemplateCache: Get or store template
FhirGenerationPipeline-->>RunExecutor: Return generated template keys
RunExecutor->>GeneratedTemplateCacheVersionStore: Bind keys to run version
RunExecutor->>MongoSnapshotStore: Persist cache metadata
sequenceDiagram
participant ScenarioEditor
participant ScenariosController
participant PatientReplacementManager
participant FhirDataLoader
ScenarioEditor->>ScenariosController: Start replacement
ScenariosController->>PatientReplacementManager: Queue purge and replay
PatientReplacementManager->>FhirDataLoader: Expunge and verify deletion
PatientReplacementManager-->>ScenariosController: Return operation status URL
ScenarioEditor->>ScenariosController: Poll status
ScenariosController-->>ScenarioEditor: Return progress and outcome
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
![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: 16
🧹 Nitpick comments (11)
DotNet/Automation/Generation/FhirGenerationPipeline.cs (2)
809-823: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the self-referencing assembly name from the dependency list.
assemblyis theAutomationassembly itself.GetReferencedAssemblies()never returns the declaring assembly, so the"Automation"entry never matches and contributes nothing. The pipeline assembly identity is already covered byGetAssemblyIdentityHash(assembly)on Line 826.♻️ Proposed cleanup
var dependencyNames = new[] { - "Automation", "Hl7.Fhir.Base", "Hl7.Fhir.Support" };🤖 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 `@DotNet/Automation/Generation/FhirGenerationPipeline.cs` around lines 809 - 823, Remove the "Automation" entry from the dependencyNames array in ComputeGeneratorDependencyFingerprint; retain only the externally referenced dependency names, since the pipeline assembly is already included through GetAssemblyIdentityHash(assembly).
336-369: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd XUnit tests for the cache-hit and cache-miss branches.
This new
if/elsechanges how patient entries are produced. Add focused unit tests for both branches with a mockedIGeneratedPatientTemplateCache(Moq): one test that asserts a store call on a miss, and one test that asserts the cached template run tag is materialized to the current run tag on a hit. Do not use network calls in these tests.Based on learnings from the path instructions: "If/Else or Switch/Case blocks are introduced or modified — ensure each branch has a corresponding unit test" and "No network activity (HTTP calls, sockets, etc.) should appear in unit tests. Recommend using mocks (via Moq) for any external communication."
🤖 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 `@DotNet/Automation/Generation/FhirGenerationPipeline.cs` around lines 336 - 369, Add focused XUnit tests covering both branches around the generated-template cache flow, using a Moq mock of IGeneratedPatientTemplateCache and no network calls. Verify the cache-miss test invokes StoreAsync, and verify the cache-hit test replaces the cached template run tag with the current run tag in the materialized bundle output.Source: Path instructions
DotNet/Automation.UI/Services/Persistence/MongoGeneratedPatientTemplateCache.cs (1)
81-90: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winReduce the per-store round trips to blob storage.
StoreAsyncruns once per generated patient. Each call issuesCreateIfNotExistsAsync,ExistsAsync,UploadAsync, andSetHttpHeadersAsync. Two of these are avoidable:
- Create the container once (lazily, or in the same place the imported-bundle content store creates it) instead of on every store.
- Set the content type in the upload call instead of a second
SetHttpHeadersAsyncrequest.♻️ Proposed change for the upload call
- using var stream = new MemoryStream(bytes, writable: false); - await blob.UploadAsync(stream, overwrite: true, cancellationToken: ct); - await blob.SetHttpHeadersAsync(new BlobHttpHeaders { ContentType = "application/json" }, cancellationToken: ct); + using var stream = new MemoryStream(bytes, writable: false); + await blob.UploadAsync( + stream, + new BlobUploadOptions { HttpHeaders = new BlobHttpHeaders { ContentType = "application/json" } }, + cancellationToken: ct);🤖 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 `@DotNet/Automation.UI/Services/Persistence/MongoGeneratedPatientTemplateCache.cs` around lines 81 - 90, Update StoreAsync to avoid per-call container creation by ensuring _container is created once through lazy initialization or the existing imported-bundle container setup. Configure the JSON content type in BlobUploadOptions during UploadAsync, then remove the separate SetHttpHeadersAsync request while preserving the existing existence check and cancellation behavior.DotNet/Automation.UI/Services/AutomationRunManager.cs (1)
28-66: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider grouping the
RunExecutordependencies.The constructor now takes 14 parameters and hand-builds
RunExecutorfrom 12 of them. Each new execution dependency requires a change in two places. RegisterRunExecutorin the container and inject it, or pass a single options/dependencies record.🤖 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 `@DotNet/Automation.UI/Services/AutomationRunManager.cs` around lines 28 - 66, Refactor AutomationRunManager to avoid manually constructing RunExecutor with its execution dependencies. Register RunExecutor in the dependency-injection container and inject it into the AutomationRunManager constructor, then retain the injected instance; alternatively, introduce a single dependencies/options record and pass that record to RunExecutor.DotNet/Automation.UI/Services/Persistence/GeneratedTemplateCacheVersionStore.cs (1)
97-106: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
BuildScenarioKeycan split one scenario across two keys.The key falls back from
scenarioIdto a trimmedscenarioName, then to a template-set hash. A scenario that is run once by ID and once by name produces two independent version sequences for the same logical scenario. The name branch is also case-sensitive and free-form.If that is intentional, add a short comment that states the precedence and its consequence.
🤖 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 `@DotNet/Automation.UI/Services/Persistence/GeneratedTemplateCacheVersionStore.cs` around lines 97 - 106, Clarify the intended key precedence in BuildScenarioKey by adding a short comment explaining that scenarioId takes priority over scenarioName and templateKeys, and that using different identity inputs creates separate version sequences. Preserve the current case-sensitive, trimmed-name behavior unless the implementation is being changed intentionally.DotNet/ServiceTests/UnitTests/AutomationUI/ImportedBundleBlobMigrationServiceTests.cs (2)
13-13: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRename the test class and file to match the renamed service.
The service is now
PatientBundleExternalizationMigrationService, but the class is stillImportedBundleBlobMigrationServiceTestsand the file is stillImportedBundleBlobMigrationServiceTests.cs. Rename both toPatientBundleExternalizationMigrationServiceTestsso the test type tracks the type under test.🤖 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 `@DotNet/ServiceTests/UnitTests/AutomationUI/ImportedBundleBlobMigrationServiceTests.cs` at line 13, Rename the test file and the ImportedBundleBlobMigrationServiceTests class to PatientBundleExternalizationMigrationServiceTests, keeping the existing test contents unchanged so the test type matches the renamed service.
191-212: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd branch coverage for the new externalization logic.
ExternalizeBundleJsonAsyncintroduces new branches inPatientBundleExternalizationMigrationService: blank input, rootJsonArrayshape, rootJsonObjectwithimportedPatientBundles, unsupported shape, per-entry blankbundleJson, and the zero-migrated result. None of these have tests.
CreateServicealready mocksIMongoCollectionandIImportedBundleContentStorewith Moq, so the shape-detection branches can be covered with small focused tests and no network activity.Do you want me to generate these XUnit tests?
Based on path instructions: "If/Else or Switch/Case blocks are introduced or modified — ensure each branch has a corresponding unit test" and "Large unit tests should be avoided; keeping unit tests small and focused on targeted business logic".
🤖 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 `@DotNet/ServiceTests/UnitTests/AutomationUI/ImportedBundleBlobMigrationServiceTests.cs` around lines 191 - 212, Add focused xUnit tests for each branch introduced by ExternalizeBundleJsonAsync in PatientBundleExternalizationMigrationService: blank input, root JsonArray, root JsonObject containing importedPatientBundles, unsupported root shape, blank per-entry bundleJson, and the zero-migrated result. Reuse CreateService and its existing Moq dependencies, keep tests small, and avoid network activity by mocking the content store and database interactions as needed.Source: Path instructions
DotNet/Automation.UI/Services/Persistence/ImportedBundleExecutionResolver.cs (1)
26-48: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueConfirm the resolver stays within Cosmos DB for MongoDB RU limits for large scenarios.
ResolveAsyncissues oneFindwith$inover all bundle ids, then one blob read per bundle in a sequential loop. The query shape is compatible with Cosmos DB for MongoDB RU. The sequential blob reads add latency proportional to the bundle count on the run path.Consider bounded parallelism for
_contentStore.ReadAsyncwhen a scenario carries many imported bundles.🤖 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 `@DotNet/Automation.UI/Services/Persistence/ImportedBundleExecutionResolver.cs` around lines 26 - 48, Update ResolveAsync’s content-loading loop to read imported bundle content with bounded parallelism instead of sequentially awaiting each _contentStore.ReadAsync call. Preserve the existing validation errors, cancellation token usage, contentById population, and single MongoDB $in query; limit concurrent reads to avoid unbounded load.Source: Coding guidelines
DotNet/Automation.UI/Services/RunExecutor.cs (2)
362-379: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winMake the cache-version binding best-effort.
BindRunAsyncruns after generation and upload complete. It writes traceability metadata only. If the store throws, the exception propagates to the handler at line 894, the run is markedFailed, and the completed generation work is discarded.This file already treats non-essential persistence as best-effort. Lines 664-672 and 678-686 wrap snapshot writes in
try/catchand emit a warning. Apply the same pattern here.♻️ Proposed refactor to isolate the binding failure
- var cacheBinding = await _generatedTemplateVersionStore.BindRunAsync( - state.RunId, - state.ScenarioId, - state.RunNameOverride, - pipelineResult.GeneratedTemplateKeys, - state.RunCancellation.Token); - if (cacheBinding != null) - { - lock (state.Sync) - { - state.GeneratedTemplateCacheVersionId = cacheBinding.VersionId; - state.GeneratedTemplateCacheVersionNumber = cacheBinding.VersionNumber; - state.GeneratedTemplateCacheScenarioKey = cacheBinding.ScenarioKey; - state.GeneratedTemplateSetHash = cacheBinding.TemplateSetHash; - } - - output.WriteLine($"[cache-version] Bound run to {cacheBinding.ScenarioKey} v{cacheBinding.VersionNumber} ({cacheBinding.VersionId})."); - } + try + { + var cacheBinding = await _generatedTemplateVersionStore.BindRunAsync( + state.RunId, + state.ScenarioId, + state.RunNameOverride, + pipelineResult.GeneratedTemplateKeys, + state.RunCancellation.Token); + if (cacheBinding != null) + { + lock (state.Sync) + { + state.GeneratedTemplateCacheVersionId = cacheBinding.VersionId; + state.GeneratedTemplateCacheVersionNumber = cacheBinding.VersionNumber; + state.GeneratedTemplateCacheScenarioKey = cacheBinding.ScenarioKey; + state.GeneratedTemplateSetHash = cacheBinding.TemplateSetHash; + } + + output.WriteLine($"[cache-version] Bound run to {cacheBinding.ScenarioKey} v{cacheBinding.VersionNumber} ({cacheBinding.VersionId})."); + } + } + catch (Exception bindEx) when (bindEx is not OperationCanceledException) + { + output.WriteLine($"[WARN] Failed to bind generated-template cache version: {bindEx.Message}"); + }🤖 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 `@DotNet/Automation.UI/Services/RunExecutor.cs` around lines 362 - 379, Wrap the BindRunAsync call and its cache metadata updates in a try/catch so binding failures do not propagate to the run handler or change a completed run to Failed. Preserve successful state assignments and output, and emit a warning through the existing logging pattern used for non-essential snapshot persistence.
368-379: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd XUnit coverage for both branches of the cache-binding condition.
Line 368 introduces a new conditional. Add small XUnit tests that cover a non-null
cacheBinding(state fields are populated) and a nullcacheBinding(state fields stay unset). MockGeneratedTemplateCacheVersionStorewith Moq so no network activity occurs.Do you want me to generate these tests?
Based on path instructions: "If/Else or Switch/Case blocks are introduced or modified — ensure each branch has a corresponding unit test" and "No network activity (HTTP calls, sockets, etc.) should appear in unit tests. Recommend using mocks (via Moq) for any external communication."
🤖 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 `@DotNet/Automation.UI/Services/RunExecutor.cs` around lines 368 - 379, Add focused XUnit coverage for the cacheBinding conditional in the relevant RunExecutor tests: mock GeneratedTemplateCacheVersionStore with Moq, then verify the non-null branch populates all GeneratedTemplateCache* state fields and the null branch leaves them unset. Keep the tests isolated from network or other external activity.Source: Path instructions
DotNet/Automation.UI/Services/PatientReplacementManager.cs (1)
41-44: 🚀 Performance & Scalability | 🔵 Trivial | ⚖️ Poor tradeoffConsider sharing a
FhirDataLoaderfactory instead of constructing one per operation.
ExecuteAsyncbuilds a newFhirDataLoaderfor every replacement.FhirDataLoadercreates its ownRestClientand authenticates in its constructor (FhirDataLoader.cslines 35-44).ScenariosControllerrepeats the same construction at lines 431 and 644. Each instance holds a separate connection pool.A shared factory or a typed client registration would reuse connections and centralize the authentication configuration. This matches the repository guideline that prefers typed
HttpClientclients withHttpClientFactory. Treat this as a follow-up, becauseFhirDataLoaderuses RestSharp rather thanHttpClienttoday.As per coding guidelines: "Prefer typed HttpClient clients with HttpClientFactory and enable header propagation (e.g., Authorization)".
🤖 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 `@DotNet/Automation.UI/Services/PatientReplacementManager.cs` around lines 41 - 44, The FhirDataLoader is constructed per operation, preventing reuse of its RestClient and duplicating authentication setup. Introduce a shared FhirDataLoader factory or typed client registration, then update ExecuteAsync and the construction sites in ScenariosController to obtain instances through it while centralizing FHIR configuration and preserving Authorization header propagation.Source: Coding guidelines
🤖 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 `@DotNet/Automation.UI/Controllers/ScenariosController.cs`:
- Around line 360-362: Update the operation start call in the controller to pass
the trimmed patient identifier, matching the identifier used for
resourcesToDelete and BuildReplayBundles. Keep patientIdForLog limited to
sanitized logging purposes and do not use it as the operational id passed to
Start.
In `@DotNet/Automation.UI/Models/IdRequest.cs`:
- Around line 3-6: Update IdRequest validation and its consumers to reject
invalid ModelState or Guid.Empty identifiers with HTTP 400 before invoking any
store or persistence calls. Add shared validation around IdRequest so all
request handlers use the same checks, while preserving normal processing for
valid IDs.
In `@DotNet/Automation.UI/Program.cs`:
- Line 313: Update PatientReplacementManager to expire completed and failed
operations instead of retaining them for the process lifetime. Add a
terminal-state timestamp and periodic cleanup, or use an equivalent
bounded-cache strategy, while preserving active-operation tracking and status
retrieval until expiry.
In
`@DotNet/Automation.UI/Services/Persistence/GeneratedTemplateCacheVersionStore.cs`:
- Around line 59-77: Update MongoIndexManager to register
automation_generated_template_versions with the required indexes: a composite
{ScenarioKey: 1, VersionNumber: -1} index supporting the filtered descending
queries, and a {ScenarioKey: 1, TemplateSetHash: 1} index supporting the version
lookup. Ensure these indexes are provisioned for the new collection alongside
its existing index configuration.
- Around line 74-94: Add a unique MongoDB index for the ScenarioKey and
TemplateSetHash fields during the GeneratedTemplateCacheVersionStore
initialization/setup. Update BindRunAsync to catch duplicate-key insertion
failures, reread the existing matching document, and return its binding so
concurrent runs share the first writer’s version; retain the existing
latest-version allocation for new template hashes.
In
`@DotNet/Automation.UI/Services/Persistence/ImportedBundleExecutionResolver.cs`:
- Around line 50-62: Update ResolveAsync to accept inputs without
UploadedBundleId when their inline BundleJson is present. Remove the
unconditional unresolved-input failure and resolve each input by preferring
contentById for a valid uploaded-bundle reference, otherwise using the input’s
inline BundleJson; throw only when both are unavailable, while preserving the
existing missing-content error context.
In
`@DotNet/Automation.UI/Services/Persistence/MongoGeneratedPatientTemplateCache.cs`:
- Around line 42-45: Update the blob-root construction in
MongoGeneratedPatientTemplateCache so generated templates never derive their
prefix from ImportedBundleBlobStorageSettings.BlobRoot. Use a dedicated
template-root setting, or the container root, and retain the
generated-patient-templates suffix without nesting under
automation/imported-bundles.
- Around line 48-102: Update GeneratedPatientTemplateCache.GetAsync and
StoreAsync to inject and use a logger, catching non-cancellation exceptions from
MongoDB, blob, and JSON operations; GetAsync must log and return null, while
StoreAsync must log and return without throwing. Preserve cancellation
propagation by rethrowing cancellation exceptions, and keep StoreAsync’s
existing argument validation exceptions outside the catch scope.
In `@DotNet/Automation.UI/Services/Persistence/MongoSnapshotStore.cs`:
- Around line 97-101: Update UpsertRunSummaryAsync so the four
generated-template cache fields—GeneratedTemplateCacheVersionId,
GeneratedTemplateCacheVersionNumber, GeneratedTemplateCacheScenarioKey, and
GeneratedTemplateSetHash—are preserved when an incoming summary does not provide
them, while still allowing explicitly deleted fields to remain deleted. Ensure
fresh DashboardSeedService summaries cannot overwrite existing cache bindings
with empty values, and retain the existing behavior for populated cache fields.
In
`@DotNet/Automation.UI/Services/Persistence/PatientBundleExternalizationMigrationService.cs`:
- Around line 68-106: Update MigrateEmbeddedPayloadsAsync to process both
scenario and run-input documents in MigrationBatchSize batches instead of
calling ToListAsync without limits. Project only the fields used by each pass,
and apply the existing InterBatchPause between batches, reusing the batching
pattern from MigrateInlinePayloadsToAbsAsync while preserving the current
updates and migration counts.
- Around line 113-124: Update ExternalizeBundleJsonAsync to catch JsonNode.Parse
failures and treat malformed JSON as unprocessable by returning the original
json with no bundles and zero externalized items, matching
RemapUploadedBundleIdsInJson’s skip behavior. Ensure malformed payloads do not
propagate exceptions into StartAsync.
In `@DotNet/Automation.UI/Services/RunSnapshotOrchestrator.cs`:
- Around line 254-271: Update DisposeUnregisteredPollerAsync to catch unexpected
exceptions from await task in addition to OperationCanceledException, log the
fault through the existing service logger, and preserve the finally block so
scope and cts are always disposed.
In `@DotNet/Automation.UI/Views/Shared/_ScenarioEditorModal.cshtml`:
- Around line 610-612: Update the importedUploadStatus span in the scenario
editor modal to include role="status" and aria-live="polite", preserving its
existing content and behavior so setImportedUploadStatus updates are announced
to assistive technology.
- Around line 602-624: Update waitForPatientReplacement to enforce a finite
polling deadline or attempt limit, throwing a clear timeout error when the
replacement does not reach a terminal status. Ensure the upload handler’s
existing error flow can surface that timeout and release the busy state, and
stop further polling when the scenario editor modal is closed by checking the
modal’s active/open state before each request and delay.
In `@DotNet/Automation/FhirDataLoader.cs`:
- Around line 106-135: Add one propagated operation deadline across
DotNet/Automation/FhirDataLoader.cs lines 106-135,
DotNet/Automation.UI/Services/PatientReplacementManager.cs lines 13-25, and
DotNet/Automation.UI/Views/Shared/_ScenarioEditorModal.cshtml lines 602-624:
update WaitForPatientDeletionAsync to accept a timeout, use a
bounded/increasing-delay loop like WaitForServerAsync, and throw
TimeoutException; have PatientReplacementManager.Start create the timeout CTS,
pass its token through ExecuteAsync, DeleteResourcesWithExpungeAsync, and
WaitForPatientDeletionAsync, translate OperationCanceledException to
operation.Fail, and evict completed operations after a retention period; add a
maximum and cancellation-on-close to the scenario editor’s polling loop so it
stops with a timeout instead of running forever.
In `@DotNet/Automation/Generation/FhirGenerationPipeline.cs`:
- Around line 878-884: Update ReplaceRunTag and the cache path around
GenerateAndUploadAsync to validate run tags before template replacement. Add or
reuse IsSafeRunTagForTemplateCache to accept only the expected generated run-tag
shape, and skip caching by setting generatedTemplateCache to null when
ids.RunTag is unsafe; preserve replacement behavior for validated tags.
---
Nitpick comments:
In `@DotNet/Automation.UI/Services/AutomationRunManager.cs`:
- Around line 28-66: Refactor AutomationRunManager to avoid manually
constructing RunExecutor with its execution dependencies. Register RunExecutor
in the dependency-injection container and inject it into the
AutomationRunManager constructor, then retain the injected instance;
alternatively, introduce a single dependencies/options record and pass that
record to RunExecutor.
In `@DotNet/Automation.UI/Services/PatientReplacementManager.cs`:
- Around line 41-44: The FhirDataLoader is constructed per operation, preventing
reuse of its RestClient and duplicating authentication setup. Introduce a shared
FhirDataLoader factory or typed client registration, then update ExecuteAsync
and the construction sites in ScenariosController to obtain instances through it
while centralizing FHIR configuration and preserving Authorization header
propagation.
In
`@DotNet/Automation.UI/Services/Persistence/GeneratedTemplateCacheVersionStore.cs`:
- Around line 97-106: Clarify the intended key precedence in BuildScenarioKey by
adding a short comment explaining that scenarioId takes priority over
scenarioName and templateKeys, and that using different identity inputs creates
separate version sequences. Preserve the current case-sensitive, trimmed-name
behavior unless the implementation is being changed intentionally.
In
`@DotNet/Automation.UI/Services/Persistence/ImportedBundleExecutionResolver.cs`:
- Around line 26-48: Update ResolveAsync’s content-loading loop to read imported
bundle content with bounded parallelism instead of sequentially awaiting each
_contentStore.ReadAsync call. Preserve the existing validation errors,
cancellation token usage, contentById population, and single MongoDB $in query;
limit concurrent reads to avoid unbounded load.
In
`@DotNet/Automation.UI/Services/Persistence/MongoGeneratedPatientTemplateCache.cs`:
- Around line 81-90: Update StoreAsync to avoid per-call container creation by
ensuring _container is created once through lazy initialization or the existing
imported-bundle container setup. Configure the JSON content type in
BlobUploadOptions during UploadAsync, then remove the separate
SetHttpHeadersAsync request while preserving the existing existence check and
cancellation behavior.
In `@DotNet/Automation.UI/Services/RunExecutor.cs`:
- Around line 362-379: Wrap the BindRunAsync call and its cache metadata updates
in a try/catch so binding failures do not propagate to the run handler or change
a completed run to Failed. Preserve successful state assignments and output, and
emit a warning through the existing logging pattern used for non-essential
snapshot persistence.
- Around line 368-379: Add focused XUnit coverage for the cacheBinding
conditional in the relevant RunExecutor tests: mock
GeneratedTemplateCacheVersionStore with Moq, then verify the non-null branch
populates all GeneratedTemplateCache* state fields and the null branch leaves
them unset. Keep the tests isolated from network or other external activity.
In `@DotNet/Automation/Generation/FhirGenerationPipeline.cs`:
- Around line 809-823: Remove the "Automation" entry from the dependencyNames
array in ComputeGeneratorDependencyFingerprint; retain only the externally
referenced dependency names, since the pipeline assembly is already included
through GetAssemblyIdentityHash(assembly).
- Around line 336-369: Add focused XUnit tests covering both branches around the
generated-template cache flow, using a Moq mock of
IGeneratedPatientTemplateCache and no network calls. Verify the cache-miss test
invokes StoreAsync, and verify the cache-hit test replaces the cached template
run tag with the current run tag in the materialized bundle output.
In
`@DotNet/ServiceTests/UnitTests/AutomationUI/ImportedBundleBlobMigrationServiceTests.cs`:
- Line 13: Rename the test file and the ImportedBundleBlobMigrationServiceTests
class to PatientBundleExternalizationMigrationServiceTests, keeping the existing
test contents unchanged so the test type matches the renamed service.
- Around line 191-212: Add focused xUnit tests for each branch introduced by
ExternalizeBundleJsonAsync in PatientBundleExternalizationMigrationService:
blank input, root JsonArray, root JsonObject containing importedPatientBundles,
unsupported root shape, blank per-entry bundleJson, and the zero-migrated
result. Reuse CreateService and its existing Moq dependencies, keep tests small,
and avoid network activity by mocking the content store and database
interactions as needed.
🪄 Autofix
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 Plus
Run ID: 1ff1c8cd-8b2f-4265-a1a4-ef797edf44eb
📒 Files selected for processing (36)
.github/copilot-instructions.mdDotNet/Automation.Link/Models/AutomationRunSummary.csDotNet/Automation.UI/Automation.UI.csprojDotNet/Automation.UI/Controllers/NormalizationsController.csDotNet/Automation.UI/Controllers/OrganizationResourceMapsController.csDotNet/Automation.UI/Controllers/QueryPlansController.csDotNet/Automation.UI/Controllers/RunsController.csDotNet/Automation.UI/Controllers/ScenariosController.csDotNet/Automation.UI/Models/IdRequest.csDotNet/Automation.UI/Program.csDotNet/Automation.UI/Services/ApiHealth/ApiHealthExecutionRunManager.csDotNet/Automation.UI/Services/ApiHealth/TestSuites/IServiceTestSuite.csDotNet/Automation.UI/Services/AutomationRunManager.csDotNet/Automation.UI/Services/MutableRunState.csDotNet/Automation.UI/Services/PatientReplacementManager.csDotNet/Automation.UI/Services/Persistence/ApiHealthRunDocument.csDotNet/Automation.UI/Services/Persistence/GeneratedTemplateCacheVersionStore.csDotNet/Automation.UI/Services/Persistence/ImportedBundleExecutionResolver.csDotNet/Automation.UI/Services/Persistence/MongoApiHealthRunStore.csDotNet/Automation.UI/Services/Persistence/MongoDocuments.csDotNet/Automation.UI/Services/Persistence/MongoGeneratedPatientTemplateCache.csDotNet/Automation.UI/Services/Persistence/MongoScenarioStore.csDotNet/Automation.UI/Services/Persistence/MongoSnapshotStore.csDotNet/Automation.UI/Services/Persistence/PatientBundleExternalizationMigrationService.csDotNet/Automation.UI/Services/Persistence/TestScenarioDocument.csDotNet/Automation.UI/Services/QueryPlanTemplateSeedService.csDotNet/Automation.UI/Services/RunExecutor.csDotNet/Automation.UI/Services/RunHub.csDotNet/Automation.UI/Services/RunSnapshotOrchestrator.csDotNet/Automation.UI/Views/Shared/_ScenarioEditorModal.cshtmlDotNet/Automation/FhirDataLoader.csDotNet/Automation/Generation/FhirBundleGenerator.csDotNet/Automation/Generation/FhirGenerationPipeline.csDotNet/Automation/Generation/IGeneratedPatientTemplateCache.csDotNet/Automation/Generation/ImportedPatientInput.csDotNet/ServiceTests/UnitTests/AutomationUI/ImportedBundleBlobMigrationServiceTests.cs
💤 Files with no reviewable changes (9)
- DotNet/Automation.UI/Services/Persistence/ApiHealthRunDocument.cs
- DotNet/Automation.UI/Services/Persistence/MongoApiHealthRunStore.cs
- DotNet/Automation.UI/Controllers/OrganizationResourceMapsController.cs
- DotNet/Automation.UI/Controllers/QueryPlansController.cs
- DotNet/Automation.UI/Controllers/NormalizationsController.cs
- DotNet/Automation.UI/Automation.UI.csproj
- DotNet/Automation.UI/Services/ApiHealth/TestSuites/IServiceTestSuite.cs
- DotNet/Automation.UI/Services/RunHub.cs
- DotNet/Automation.UI/Services/QueryPlanTemplateSeedService.cs
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.
🛠️ Description of Changes
Generated patient bundles are now stored in ABS to avoid cosmosdb size limits
Generated patient bundles now use a caching/versioning system to reduce repetitive bundle saves (Each scenario will maintain a versioned cache of the generated data that is then uniquified per run, and the run is tied to that cache version).
Made Expunging patients and uploading their data to the fhir server for large patient bundle uploads when creating a scenario a background process that is monitored to avoid timeouts.
🧪 Testing Performed
Please describe the testing that was performed on the changes included in this PR.
🧑🔬 Unit Testing
📓 Documentation Updated
Please update any relevant sections in the project documentation that were impacted by the changes in the PR.
Summary by CodeRabbit