chore: Use direct schemas in remaining mapped tools#1599
Conversation
|
Warning Review limit reached
Next review available in: 23 minutes 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 ignored due to path filters (2)
📒 Files selected for processing (18)
📝 WalkthroughWalkthroughThis PR removes legacy UnityCliLoop-prefixed service interfaces and request/result DTOs (GetHierarchy, FindGameObjects, Screenshot, RecordInput, ReplayInput) and updates tools, use cases, an exporter, and tests to construct/consume the corresponding schema/response contract types directly, eliminating intermediate mapping helpers. ChangesSchema/Response contract migration
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Tool as FeatureTool
participant UseCase as FeatureUseCase
Tool->>UseCase: ExecuteAsync(Schema, ct)
UseCase-->>Tool: Response
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 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Assets/Tests/Editor/UnityCliLoopToolExecutionServiceTests.cs (1)
100-114: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThis test is now tautological and its name is stale.
After removing the
ToResponsemapping, this test constructs aScreenshotResponse, setsTimedOut/Message/Screenshots, and then asserts those same values back—so it now only exercisesScreenshotResponse's auto-properties andScreenshotCount, not any production behavior. The method nameScreenshotTool_ToResponse_WhenCaptureTimedOut_PreservesTimeoutDetailsalso references a method that no longer exists.Consider redirecting it to exercise the real timeout path (e.g.
ScreenshotUseCase.CreateTimedOutResult/ a rendering-timeout capture) so the "preserves timeout details" guarantee is actually verified, and rename accordingly.🤖 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 `@Assets/Tests/Editor/UnityCliLoopToolExecutionServiceTests.cs` around lines 100 - 114, The test is tautological and the name is stale because it no longer exercises any production mapping logic. Update the test in UnityCliLoopToolExecutionServiceTests so it drives the real timeout path via ScreenshotUseCase.CreateTimedOutResult or the relevant rendering-timeout flow instead of manually constructing ScreenshotResponse, then assert the resulting response preserves TimedOut, Message, and ScreenshotCount. Rename the test to match the actual behavior being verified and remove the reference to the removed ToResponse method.
🤖 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 `@Assets/Tests/Editor/UnityCliLoopToolExecutionServiceTests.cs`:
- Around line 100-114: The test is tautological and the name is stale because it
no longer exercises any production mapping logic. Update the test in
UnityCliLoopToolExecutionServiceTests so it drives the real timeout path via
ScreenshotUseCase.CreateTimedOutResult or the relevant rendering-timeout flow
instead of manually constructing ScreenshotResponse, then assert the resulting
response preserves TimedOut, Message, and ScreenshotCount. Rename the test to
match the actual behavior being verified and remove the reference to the removed
ToResponse method.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2b78124c-b362-4680-a858-358a0df8ad94
⛔ Files ignored due to path filters (2)
Packages/src/Editor/FirstPartyTools/Common/InputRecording/UnityCliLoopInputRecordingTypes.cs.metais excluded by none and included by nonePackages/src/Editor/FirstPartyTools/GetHierarchy/UnityCliLoopHierarchyTypes.cs.metais excluded by none and included by none
📒 Files selected for processing (18)
Assets/Tests/Editor/InputActionValidationTests.csAssets/Tests/Editor/OnionAssemblyDependencyTests.csAssets/Tests/Editor/UnityCliLoopToolExecutionServiceTests.csPackages/src/Editor/FirstPartyTools/Common/InputRecording/UnityCliLoopInputRecordingTypes.csPackages/src/Editor/FirstPartyTools/FindGameObjects/Application/FindGameObjectsResultExporter.csPackages/src/Editor/FirstPartyTools/FindGameObjects/FindGameObjectsTool.csPackages/src/Editor/FirstPartyTools/FindGameObjects/FindGameObjectsUseCase.csPackages/src/Editor/FirstPartyTools/FindGameObjects/UnityCliLoopGameObjectSearchTypes.csPackages/src/Editor/FirstPartyTools/GetHierarchy/GetHierarchyTool.csPackages/src/Editor/FirstPartyTools/GetHierarchy/GetHierarchyUseCase.csPackages/src/Editor/FirstPartyTools/GetHierarchy/UnityCliLoopHierarchyTypes.csPackages/src/Editor/FirstPartyTools/RecordInput/RecordInputTool.csPackages/src/Editor/FirstPartyTools/RecordInput/RecordInputUseCase.csPackages/src/Editor/FirstPartyTools/ReplayInput/ReplayInputTool.csPackages/src/Editor/FirstPartyTools/ReplayInput/ReplayInputUseCase.csPackages/src/Editor/FirstPartyTools/Screenshot/ScreenshotTool.csPackages/src/Editor/FirstPartyTools/Screenshot/ScreenshotUseCase.csPackages/src/Editor/ToolContracts/UnityCliLoopScreenshotTypes.cs
💤 Files with no reviewable changes (4)
- Packages/src/Editor/FirstPartyTools/Common/InputRecording/UnityCliLoopInputRecordingTypes.cs
- Packages/src/Editor/FirstPartyTools/GetHierarchy/UnityCliLoopHierarchyTypes.cs
- Packages/src/Editor/FirstPartyTools/FindGameObjects/UnityCliLoopGameObjectSearchTypes.cs
- Packages/src/Editor/ToolContracts/UnityCliLoopScreenshotTypes.cs
Remove the intermediate request/result mapping layer from the remaining mapped bundled tools so they follow the same schema-to-response flow as the rest of the tool set. The public response shapes stay unchanged while unused single-implementation service and DTO types are removed.
c71c3a1 to
ab886ee
Compare
Summary
User Impact
Changes
Protocol
Verification