Repository navigation
chore: Collapse the Compile tool's redundant intermediate DTO layer - #1514
Conversation
The Compile pipeline used to carry a redundant intermediate DTO layer (IUnityCliLoopCompilationService, UnityCliLoopCompileRequest, UnityCliLoopCompileIssue, UnityCliLoopCompileResult) that only performed field-for-field copies between the wire-facing CompileSchema / CompileResponse and the internal CompileResult. The intermediate types had no external consumers and added mapping code without changing behavior. Route CompileSchema and CompileResponse directly through CompileUseCase, CompilationExecutionService, CompileController, and CompileSessionResultService, and delete the intermediate types. The session-state JSON that StoreCompileResult writes stays byte-identical because CompileResponse has the same declaration order, nullability, and PascalCase property names as the old UnityCliLoopCompileResult, and both flow through the same JsonRpcResponseSerializer settings without a contract resolver rename.
|
Warning Review limit reached
Next review available in: 45 seconds Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR migrates the compilation feature's request/response types from legacy ChangesCompile Response Type Migration
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant CompileTool
participant CompileUseCase
participant CompileController
participant CompileSessionResultService
participant SessionState
CompileTool->>CompileUseCase: CompileAsync(CompileSchema, ct)
CompileUseCase->>CompileController: ExecuteCompilationAsync(CompileSchema, ct)
CompileController->>CompileSessionResultService: CreateCompileResult(compilerMessages)
CompileSessionResultService-->>CompileController: CompileResponse
CompileController->>CompileSessionResultService: StoreCompileResult(CompileResponse)
CompileSessionResultService->>SessionState: persist serialized CompileResponse JSON
CompileController-->>CompileUseCase: compilation completed
CompileUseCase-->>CompileTool: CompileResponse
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Use the existing CompileIssue constructor instead of the CreateIssue helper and object initializers, because the constructor only became available once the intermediate DTO (which had no constructor) was replaced by CompileIssue. Extend the PascalCase pinning test to cover all seven stored properties, since CompileResponse now doubles as the SessionState payload parsed by the CLI and no dedicated storage DTO guards that shape anymore.
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
Packages/src/Editor/FirstPartyTools/Compile/CompileUseCase.cs (1)
145-162: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRedundant helper duplicates the
CompileResponseconstructor.
CreateCompileResult(bool?, int?, int?, CompileIssue[], CompileIssue[], string)has the exact same parameter order/types as the existingCompileResponse(bool?, int?, int?, CompileIssue[], CompileIssue[], string message = null)constructor. This is the same kind of redundant mapping this PR is removing elsewhere (the commit history already dropped the legacyCreateIssuehelper for the same reason).♻️ Proposed cleanup: drop the wrapper, call the constructor directly
- CompileResponse response = CreateCompileResult( - false, - 1, - 0, - new[] { new CompileIssue(preparation.ErrorMessage, "", 0) }, - Array.Empty<CompileIssue>(), - null); + CompileResponse response = new( + false, + 1, + 0, + new[] { new CompileIssue(preparation.ErrorMessage, "", 0) }, + Array.Empty<CompileIssue>());(apply the same substitution at the two other call sites, lines 96-102 and 120-126)
- private static CompileResponse CreateCompileResult( - bool? success, - int? errorCount, - int? warningCount, - CompileIssue[] errors, - CompileIssue[] warnings, - string message) - { - return new CompileResponse - { - Success = success, - ErrorCount = errorCount, - WarningCount = warningCount, - Errors = errors, - Warnings = warnings, - Message = 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 `@Packages/src/Editor/FirstPartyTools/Compile/CompileUseCase.cs` around lines 145 - 162, Remove the redundant CreateCompileResult helper in CompileUseCase and construct CompileResponse directly instead, since it duplicates the CompileResponse(bool?, int?, int?, CompileIssue[], CompileIssue[], string) constructor exactly. Update the call sites in CompileUseCase that currently use CreateCompileResult so they instantiate CompileResponse inline with the same arguments, matching the cleanup already done for CreateIssue.Packages/src/Editor/FirstPartyTools/Compile/CompileSessionResultService.cs (1)
20-53: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSame constructor-vs-initializer redundancy as
CompileUseCase.CreateCompileResult.The
CompileResponseobject initializers here (and inCreateForceCompileResult, lines 101-113) populate the exact same fields theCompileResponse(bool?, int?, int?, CompileIssue[], CompileIssue[], string)constructor already accepts. Not a functional issue, but worth consolidating with the constructor since this file is the canonical place doing that mapping now that the DTO layer is gone.🤖 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/CompileSessionResultService.cs` around lines 20 - 53, The CompileResponse construction in CreateCompileResult (and the matching path in CreateForceCompileResult) is duplicating the same field mapping already covered by the CompileResponse(bool?, int?, int?, CompileIssue[], CompileIssue[], string) constructor. Replace the object initializers with the constructor call so CompileSessionResultService becomes the single canonical mapping point, and keep the existing logic for indeterminate results, force-recompile handling, and AddMissingTestFrameworkReferenceHint intact.
🤖 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/CompileSessionResultService.cs`:
- Around line 20-53: The CompileResponse construction in CreateCompileResult
(and the matching path in CreateForceCompileResult) is duplicating the same
field mapping already covered by the CompileResponse(bool?, int?, int?,
CompileIssue[], CompileIssue[], string) constructor. Replace the object
initializers with the constructor call so CompileSessionResultService becomes
the single canonical mapping point, and keep the existing logic for
indeterminate results, force-recompile handling, and
AddMissingTestFrameworkReferenceHint intact.
In `@Packages/src/Editor/FirstPartyTools/Compile/CompileUseCase.cs`:
- Around line 145-162: Remove the redundant CreateCompileResult helper in
CompileUseCase and construct CompileResponse directly instead, since it
duplicates the CompileResponse(bool?, int?, int?, CompileIssue[],
CompileIssue[], string) constructor exactly. Update the call sites in
CompileUseCase that currently use CreateCompileResult so they instantiate
CompileResponse inline with the same arguments, matching the cleanup already
done for CreateIssue.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f047a388-8f72-4858-a12e-c7d6b5d1334f
⛔ Files ignored due to path filters (1)
Packages/src/Editor/FirstPartyTools/Compile/UnityCliLoopCompileTypes.cs.metais excluded by none and included by none
📒 Files selected for processing (7)
Assets/Tests/Editor/CompileSessionResultServiceTests.csPackages/src/Editor/FirstPartyTools/Compile/CompilationExecutionService.csPackages/src/Editor/FirstPartyTools/Compile/CompileController.csPackages/src/Editor/FirstPartyTools/Compile/CompileSessionResultService.csPackages/src/Editor/FirstPartyTools/Compile/CompileTool.csPackages/src/Editor/FirstPartyTools/Compile/CompileUseCase.csPackages/src/Editor/FirstPartyTools/Compile/UnityCliLoopCompileTypes.cs
💤 Files with no reviewable changes (1)
- Packages/src/Editor/FirstPartyTools/Compile/UnityCliLoopCompileTypes.cs
CodeRabbit flagged that the CreateCompileResult helper and the remaining object initializers duplicate the CompileResponse constructor, mirroring the CreateIssue cleanup already applied. Named arguments keep the adjacent nullable counts and issue arrays from being swapped silently.
1 participant
Summary
IUnityCliLoopCompilationService,UnityCliLoopCompileRequest,UnityCliLoopCompileIssue,UnityCliLoopCompileResult), which only performed field-by-field copies between the wire DTOs and the compile pipeline.User Impact
get-compile-statusresult JSON stay byte-identical.Changes
CompileUseCase,CompilationExecutionService,CompileController,CompileSessionResultService) now usesCompileSchema/CompileResponse/CompileIssuedirectly.CompileToolno longer needs its ToRequest/ToResponse/ToCompileIssues mapping helpers; it passes the schema through and returns the use case result.UnityCliLoopCompileTypes.cs(the whole intermediate layer) is deleted. Repo-wide grep confirms no remaining references, including non-C# files.Wire compatibility
CompileSessionResultService.StoreCompileResultserializes the stored result into Editor SessionState, and that JSON is returned verbatim to the CLI viaget-compile-status.CompileResponseextends an empty base class and declares the same 7 properties in the same order and nullability as the deletedUnityCliLoopCompileResult, andJsonRpcResponseSerializer.Settingsuses no contract resolver, so the stored JSON shape is unchanged (asserted byStoreCompileResult_WhenResultIsPersisted_UsesPascalCaseJson).Verification
dist/darwin-arm64/uloop compile→ 0 errors, 0 warnings.dist/darwin-arm64/uloop run-tests --filter-type regex --filter-value "Compile.*Tests"→ 48 passed, 0 failed, 0 skipped.