chore: Split compile response persistence responsibilities#1546
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughCompileSessionResultService is removed and its logic is split into CompileResponseFactory, CompileSessionResultStore, and CompileResultRecordingContext. CompileController and CompileUseCase now use the new types, and tests were renamed and expanded for the new split. ChangesCompile result service split
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Packages/src/Editor/FirstPartyTools/Compile/CompileResponseFactory.cs (1)
29-38: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMissing test coverage for the non-force-recompile indeterminate branch.
This branch (triggered e.g. from
HandleCompileStoppedWithoutFinishEventresults withIsIndeterminate=trueandforceRecompile=false) isn't exercised by any test inCompileResponseFactoryTests.cs. The only indeterminate-flagged test case (CreateResponse_WhenForceCompileIsUnknown_ExplainsNullDetails) actually short-circuits intoCreateForceCompileResultsinceforceRecompile: true, never reaching this code path.✅ Suggested additional test
[Test] public void CreateResponse_WhenNonForceCompileIsIndeterminate_ExplainsNullDetails() { CompileResult result = new CompileResult( success: null, errorCount: 0, warningCount: 0, completedAt: DateTime.Now, messages: Array.Empty<CompilerMessage>(), errors: Array.Empty<CompilerMessage>(), warnings: Array.Empty<CompilerMessage>(), isIndeterminate: true, message: null); CompileResponse response = CompileResponseFactory.CreateResponse(result, forceRecompile: false); Assert.That(response.Errors, Is.Null); Assert.That(response.Warnings, Is.Null); Assert.That(response.Message, Is.EqualTo("Compilation status is unknown. Use get-logs to inspect the compiler output.")); }🤖 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 `@Packages/src/Editor/FirstPartyTools/Compile/CompileResponseFactory.cs` around lines 29 - 38, Add test coverage for the `CompileResponseFactory.CreateResponse` indeterminate path when `forceRecompile` is false, since the existing `CreateResponse_WhenForceCompileIsUnknown_ExplainsNullDetails` test only exercises `CreateForceCompileResult`. Add a new test in `CompileResponseFactoryTests` that builds a `CompileResult` with `IsIndeterminate = true` and `forceRecompile: false`, then verifies the returned `CompileResponse` has null `Errors` and `Warnings` and the fallback status message. Use the existing `CompileResponseFactory.CreateResponse` and `CompileResult` symbols to target the non-force-recompile branch directly.
🤖 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.
Nitpick comments:
In `@Packages/src/Editor/FirstPartyTools/Compile/CompileResponseFactory.cs`:
- Around line 29-38: Add test coverage for the
`CompileResponseFactory.CreateResponse` indeterminate path when `forceRecompile`
is false, since the existing
`CreateResponse_WhenForceCompileIsUnknown_ExplainsNullDetails` test only
exercises `CreateForceCompileResult`. Add a new test in
`CompileResponseFactoryTests` that builds a `CompileResult` with
`IsIndeterminate = true` and `forceRecompile: false`, then verifies the returned
`CompileResponse` has null `Errors` and `Warnings` and the fallback status
message. Use the existing `CompileResponseFactory.CreateResponse` and
`CompileResult` symbols to target the non-force-recompile branch directly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1fdef517-1f93-43f7-a17d-c8a95733d24e
⛔ Files ignored due to path filters (5)
Assets/Tests/Editor/CompileResponseFactoryTests.cs.metais excluded by none and included by noneAssets/Tests/Editor/CompileSessionResultStoreTests.cs.metais excluded by none and included by nonePackages/src/Editor/FirstPartyTools/Compile/CompileResponseFactory.cs.metais excluded by none and included by nonePackages/src/Editor/FirstPartyTools/Compile/CompileResultRecordingContext.cs.metais excluded by none and included by nonePackages/src/Editor/FirstPartyTools/Compile/CompileSessionResultStore.cs.metais excluded by none and included by none
📒 Files selected for processing (9)
Assets/Tests/Editor/CompileResponseFactoryTests.csAssets/Tests/Editor/CompileSessionResultStoreTests.csPackages/src/Editor/FirstPartyTools/Compile/CompileController.csPackages/src/Editor/FirstPartyTools/Compile/CompileResponseFactory.csPackages/src/Editor/FirstPartyTools/Compile/CompileResultFactory.csPackages/src/Editor/FirstPartyTools/Compile/CompileResultRecordingContext.csPackages/src/Editor/FirstPartyTools/Compile/CompileSessionResultService.csPackages/src/Editor/FirstPartyTools/Compile/CompileSessionResultStore.csPackages/src/Editor/FirstPartyTools/Compile/CompileUseCase.cs
💤 Files with no reviewable changes (1)
- Packages/src/Editor/FirstPartyTools/Compile/CompileSessionResultService.cs
Owner
Author
|
Deferring the CodeRabbit indeterminate-response coverage nitpick to the planned follow-up PR. The suggestion is valid coverage, but this PR is intentionally move-only with test reference swaps/assertions preserved. The follow-up that pins indeterminate and validation response shapes before unifying them will include the IsIndeterminate=true and forceRecompile=false CreateResponse shape. |
There was a problem hiding this comment.
1 issue found and verified against the latest diff
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="Packages/src/Editor/FirstPartyTools/Compile/CompileResultRecordingContext.cs">
<violation number="1" location="Packages/src/Editor/FirstPartyTools/Compile/CompileResultRecordingContext.cs:30">
P2: Direct callers of CompilationExecutionService.ExecuteCompilationAsync can persist compile status with an unsafe RequestId, bypassing the safety contract CompileUseCase applies before storage. Consider keeping CompileResultRecordingContext.Create aligned with CompileRequestIdRules.IsSafe so malformed IDs do not corrupt the SessionState request index or scoped keys.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| { | ||
| Debug.Assert(request != null, "request must not be null"); | ||
|
|
||
| if (!request.WaitForDomainReload || string.IsNullOrWhiteSpace(request.RequestId)) |
There was a problem hiding this comment.
P2: Direct callers of CompilationExecutionService.ExecuteCompilationAsync can persist compile status with an unsafe RequestId, bypassing the safety contract CompileUseCase applies before storage. Consider keeping CompileResultRecordingContext.Create aligned with CompileRequestIdRules.IsSafe so malformed IDs do not corrupt the SessionState request index or scoped keys.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Packages/src/Editor/FirstPartyTools/Compile/CompileResultRecordingContext.cs, line 30:
<comment>Direct callers of CompilationExecutionService.ExecuteCompilationAsync can persist compile status with an unsafe RequestId, bypassing the safety contract CompileUseCase applies before storage. Consider keeping CompileResultRecordingContext.Create aligned with CompileRequestIdRules.IsSafe so malformed IDs do not corrupt the SessionState request index or scoped keys.</comment>
<file context>
@@ -0,0 +1,41 @@
+ {
+ Debug.Assert(request != null, "request must not be null");
+
+ if (!request.WaitForDomainReload || string.IsNullOrWhiteSpace(request.RequestId))
+ {
+ return Disabled();
</file context>
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
User Impact
Changes
Verification