refactor: move third-party migration rules to domain#1540
Merged
Conversation
Contributor
|
Warning Review limit reached
Next review available in: 43 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 selected for processing (1)
📝 WalkthroughWalkthroughThis PR relocates the ThirdPartyToolMigration rule set into the Domain namespace, adds new Domain helper and data types, removes duplicated Infrastructure implementations, and updates remaining Infrastructure call sites and tests to reference the Domain types. ChangesDomain Namespace Migration
Estimated code review effort: 5 (Critical) | ~120 minutes 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 |
Keep the moved rule surface limited to Infrastructure-forced public APIs and remove residual orphan using directives while preserving the Domain DTO alias needed by AsmdefRules.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
Packages/src/Editor/Domain/ThirdPartyToolMigrationAliasRules.cs (1)
87-156: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract shared combine logic to remove 5x duplication.
Each
GetCombined*Aliasesmethod repeats the identicalConcat(...).Distinct(StringComparer.Ordinal).ToArray()pattern. Consider factoring a private helper.♻️ Proposed refactor
+ private static string[] CombineAliases(IEnumerable<string> sourceAliases, string[] additionalAliases) + { + return sourceAliases.Concat(additionalAliases).Distinct(StringComparer.Ordinal).ToArray(); + } + public static string[] GetCombinedLegacyNamespaceAliases( string source, string[] legacyAssemblyAliases) { Debug.Assert(source != null, "source must not be null"); Debug.Assert(legacyAssemblyAliases != null, "legacyAssemblyAliases must not be null"); - return GetLegacyNamespaceAliases(source) - .Concat(legacyAssemblyAliases) - .Distinct(StringComparer.Ordinal) - .ToArray(); + return CombineAliases(GetLegacyNamespaceAliases(source), legacyAssemblyAliases); }Apply the equivalent simplification to the other four
GetCombined*Aliasesmethods.🤖 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/Domain/ThirdPartyToolMigrationAliasRules.cs` around lines 87 - 156, The five GetCombined*Aliases methods in ThirdPartyToolMigrationAliasRules all repeat the same Concat/Distinct/ToArray combination logic, so factor that shared behavior into a private helper and have GetCombinedLegacyNamespaceAliases, GetCombinedLegacyToolInfoTypeAliases, GetCombinedCurrentApplicationNamespaceAliases, GetCombinedCurrentDomainNamespaceAliases, and GetCombinedCurrentFirstPartyToolsNamespaceAliases call it with their specific source aliases and arrays. Keep the existing null assertions and StringComparer.Ordinal de-duplication semantics unchanged while removing the duplicated implementation.Packages/src/Editor/Domain/ThirdPartyToolMigrationRuleCatalog.cs (1)
226-230: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReference the
LegacyNamespaceconstant instead of a hardcoded literal.
LegacyNamespaceAliasRegexspells outio\.github\.hatayama\.uLoopMCPliterally, while every sibling pattern in this catalog (LegacyNamespacePattern,LegacyGlobalUsingRegex,LegacyNamespaceUsingRegex, etc.) derives fromRegex.Escape(LegacyNamespace). IfLegacyNamespaceis ever updated, this pattern will silently keep matching the old value.♻️ Proposed fix
public static readonly Regex LegacyNamespaceAliasRegex = new( @"\busing\s+(?<alias>[A-Za-z_][A-Za-z0-9_]*)\s*=\s*(?:global::)?" + - @"io\.github\.hatayama\.uLoopMCP\s*;", + $@"{Regex.Escape(LegacyNamespace)}\s*;", RegexOptions.Compiled);🤖 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/Domain/ThirdPartyToolMigrationRuleCatalog.cs` around lines 226 - 230, The LegacyNamespaceAliasRegex pattern hardcodes the old namespace instead of using the shared LegacyNamespace constant. Update the regex construction in ThirdPartyToolMigrationRuleCatalog so this alias pattern is built from Regex.Escape(LegacyNamespace), matching the approach used by LegacyNamespacePattern, LegacyGlobalUsingRegex, and LegacyNamespaceUsingRegex. Keep the alias capture logic intact, but replace the literal namespace text with the constant-derived expression so future namespace changes propagate automatically.
🤖 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/Domain/ThirdPartyToolMigrationAliasRules.cs`:
- Around line 87-156: The five GetCombined*Aliases methods in
ThirdPartyToolMigrationAliasRules all repeat the same Concat/Distinct/ToArray
combination logic, so factor that shared behavior into a private helper and have
GetCombinedLegacyNamespaceAliases, GetCombinedLegacyToolInfoTypeAliases,
GetCombinedCurrentApplicationNamespaceAliases,
GetCombinedCurrentDomainNamespaceAliases, and
GetCombinedCurrentFirstPartyToolsNamespaceAliases call it with their specific
source aliases and arrays. Keep the existing null assertions and
StringComparer.Ordinal de-duplication semantics unchanged while removing the
duplicated implementation.
In `@Packages/src/Editor/Domain/ThirdPartyToolMigrationRuleCatalog.cs`:
- Around line 226-230: The LegacyNamespaceAliasRegex pattern hardcodes the old
namespace instead of using the shared LegacyNamespace constant. Update the regex
construction in ThirdPartyToolMigrationRuleCatalog so this alias pattern is
built from Regex.Escape(LegacyNamespace), matching the approach used by
LegacyNamespacePattern, LegacyGlobalUsingRegex, and LegacyNamespaceUsingRegex.
Keep the alias capture logic intact, but replace the literal namespace text with
the constant-derived expression so future namespace changes propagate
automatically.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 089c05d6-22ce-4a2c-abb0-e7bc132bf507
⛔ Files ignored due to path filters (35)
Packages/src/Editor/Domain/ThirdPartyToolMigrationAliasRules.cs.metais excluded by none and included by nonePackages/src/Editor/Domain/ThirdPartyToolMigrationApiDetectionRules.cs.metais excluded by none and included by nonePackages/src/Editor/Domain/ThirdPartyToolMigrationApplicationDetectionRules.cs.metais excluded by none and included by nonePackages/src/Editor/Domain/ThirdPartyToolMigrationArgumentRules.cs.metais excluded by none and included by nonePackages/src/Editor/Domain/ThirdPartyToolMigrationAttributeRules.cs.metais excluded by none and included by nonePackages/src/Editor/Domain/ThirdPartyToolMigrationCSharpRules.cs.metais excluded by none and included by nonePackages/src/Editor/Domain/ThirdPartyToolMigrationCodeTextDetectionRules.cs.metais excluded by none and included by nonePackages/src/Editor/Domain/ThirdPartyToolMigrationCodeTextMaskBuilder.cs.metais excluded by none and included by nonePackages/src/Editor/Domain/ThirdPartyToolMigrationCodeTextMaskInterpolationRules.cs.metais excluded by none and included by nonePackages/src/Editor/Domain/ThirdPartyToolMigrationConstructorArgumentRules.cs.metais excluded by none and included by nonePackages/src/Editor/Domain/ThirdPartyToolMigrationDelayRules.cs.metais excluded by none and included by nonePackages/src/Editor/Domain/ThirdPartyToolMigrationDomainDetectionRules.cs.metais excluded by none and included by nonePackages/src/Editor/Domain/ThirdPartyToolMigrationEditorDelayRules.cs.metais excluded by none and included by nonePackages/src/Editor/Domain/ThirdPartyToolMigrationInterpolatedStringRules.cs.metais excluded by none and included by nonePackages/src/Editor/Domain/ThirdPartyToolMigrationMetadataConstructorRules.cs.metais excluded by none and included by nonePackages/src/Editor/Domain/ThirdPartyToolMigrationParsingRules.cs.metais excluded by none and included by nonePackages/src/Editor/Domain/ThirdPartyToolMigrationRegexRewriteRules.cs.metais excluded by none and included by nonePackages/src/Editor/Domain/ThirdPartyToolMigrationRuleCatalog.cs.metais excluded by none and included by nonePackages/src/Editor/Domain/ThirdPartyToolMigrationRuleData.cs.metais excluded by none and included by nonePackages/src/Editor/Domain/ThirdPartyToolMigrationScreenshotArgumentRules.cs.metais excluded by none and included by nonePackages/src/Editor/Domain/ThirdPartyToolMigrationScreenshotDeconstructionRules.cs.metais excluded by none and included by nonePackages/src/Editor/Domain/ThirdPartyToolMigrationScreenshotDetectionRules.cs.metais excluded by none and included by nonePackages/src/Editor/Domain/ThirdPartyToolMigrationScreenshotRules.cs.metais excluded by none and included by nonePackages/src/Editor/Domain/ThirdPartyToolMigrationTimingArgumentRules.cs.metais excluded by none and included by nonePackages/src/Editor/Domain/ThirdPartyToolMigrationTimingCallerRules.cs.metais excluded by none and included by nonePackages/src/Editor/Domain/ThirdPartyToolMigrationTimingCleanupRules.cs.metais excluded by none and included by nonePackages/src/Editor/Domain/ThirdPartyToolMigrationTimingDeclarationRules.cs.metais excluded by none and included by nonePackages/src/Editor/Domain/ThirdPartyToolMigrationTimingInvocationRules.cs.metais excluded by none and included by nonePackages/src/Editor/Domain/ThirdPartyToolMigrationTimingMethodBodyRules.cs.metais excluded by none and included by nonePackages/src/Editor/Domain/ThirdPartyToolMigrationTimingMethodDeclarationRules.cs.metais excluded by none and included by nonePackages/src/Editor/Domain/ThirdPartyToolMigrationTimingTypeNameRules.cs.metais excluded by none and included by nonePackages/src/Editor/Domain/ThirdPartyToolMigrationTimingTypeResolutionRules.cs.metais excluded by none and included by nonePackages/src/Editor/Domain/ThirdPartyToolMigrationTimingTypeScopeRules.cs.metais excluded by none and included by nonePackages/src/Editor/Domain/ThirdPartyToolMigrationToolContractDetectionRules.cs.metais excluded by none and included by nonePackages/src/Editor/Domain/ThirdPartyToolMigrationTypeReplacementRules.cs.metais excluded by none and included by none
📒 Files selected for processing (49)
Assets/Tests/Editor/ThirdPartyToolMigrationRulesTests.csPackages/src/Editor/Domain/ThirdPartyToolMigrationAliasRules.csPackages/src/Editor/Domain/ThirdPartyToolMigrationApiDetectionRules.csPackages/src/Editor/Domain/ThirdPartyToolMigrationApplicationDetectionRules.csPackages/src/Editor/Domain/ThirdPartyToolMigrationArgumentRules.csPackages/src/Editor/Domain/ThirdPartyToolMigrationAttributeRules.csPackages/src/Editor/Domain/ThirdPartyToolMigrationCSharpRules.csPackages/src/Editor/Domain/ThirdPartyToolMigrationCodeTextDetectionRules.csPackages/src/Editor/Domain/ThirdPartyToolMigrationCodeTextMaskBuilder.csPackages/src/Editor/Domain/ThirdPartyToolMigrationCodeTextMaskInterpolationRules.csPackages/src/Editor/Domain/ThirdPartyToolMigrationConstructorArgumentRules.csPackages/src/Editor/Domain/ThirdPartyToolMigrationDelayRules.csPackages/src/Editor/Domain/ThirdPartyToolMigrationDomainDetectionRules.csPackages/src/Editor/Domain/ThirdPartyToolMigrationEditorDelayRules.csPackages/src/Editor/Domain/ThirdPartyToolMigrationInterpolatedStringRules.csPackages/src/Editor/Domain/ThirdPartyToolMigrationMetadataConstructorRules.csPackages/src/Editor/Domain/ThirdPartyToolMigrationParsingRules.csPackages/src/Editor/Domain/ThirdPartyToolMigrationRegexRewriteRules.csPackages/src/Editor/Domain/ThirdPartyToolMigrationRuleCatalog.csPackages/src/Editor/Domain/ThirdPartyToolMigrationRuleData.csPackages/src/Editor/Domain/ThirdPartyToolMigrationScreenshotArgumentRules.csPackages/src/Editor/Domain/ThirdPartyToolMigrationScreenshotDeconstructionRules.csPackages/src/Editor/Domain/ThirdPartyToolMigrationScreenshotDetectionRules.csPackages/src/Editor/Domain/ThirdPartyToolMigrationScreenshotRules.csPackages/src/Editor/Domain/ThirdPartyToolMigrationTimingArgumentRules.csPackages/src/Editor/Domain/ThirdPartyToolMigrationTimingCallerRules.csPackages/src/Editor/Domain/ThirdPartyToolMigrationTimingCleanupRules.csPackages/src/Editor/Domain/ThirdPartyToolMigrationTimingDeclarationRules.csPackages/src/Editor/Domain/ThirdPartyToolMigrationTimingInvocationRules.csPackages/src/Editor/Domain/ThirdPartyToolMigrationTimingMethodBodyRules.csPackages/src/Editor/Domain/ThirdPartyToolMigrationTimingMethodDeclarationRules.csPackages/src/Editor/Domain/ThirdPartyToolMigrationTimingTypeNameRules.csPackages/src/Editor/Domain/ThirdPartyToolMigrationTimingTypeResolutionRules.csPackages/src/Editor/Domain/ThirdPartyToolMigrationTimingTypeScopeRules.csPackages/src/Editor/Domain/ThirdPartyToolMigrationToolContractDetectionRules.csPackages/src/Editor/Domain/ThirdPartyToolMigrationTypeReplacementRules.csPackages/src/Editor/Infrastructure/ThirdPartyToolMigration/ThirdPartyToolMigrationAliasRules.csPackages/src/Editor/Infrastructure/ThirdPartyToolMigration/ThirdPartyToolMigrationAsmdefMigrationRequirementResolver.csPackages/src/Editor/Infrastructure/ThirdPartyToolMigration/ThirdPartyToolMigrationAsmdefRules.csPackages/src/Editor/Infrastructure/ThirdPartyToolMigration/ThirdPartyToolMigrationAssemblyUsageScanState.csPackages/src/Editor/Infrastructure/ThirdPartyToolMigration/ThirdPartyToolMigrationConstructorArgumentRules.csPackages/src/Editor/Infrastructure/ThirdPartyToolMigration/ThirdPartyToolMigrationCrossFileTimingMigrationPlanner.csPackages/src/Editor/Infrastructure/ThirdPartyToolMigration/ThirdPartyToolMigrationDetectionRules.csPackages/src/Editor/Infrastructure/ThirdPartyToolMigration/ThirdPartyToolMigrationFastAssemblyRequirementCollector.csPackages/src/Editor/Infrastructure/ThirdPartyToolMigration/ThirdPartyToolMigrationFastSourceTargetDetector.csPackages/src/Editor/Infrastructure/ThirdPartyToolMigration/ThirdPartyToolMigrationPlanBuilder.csPackages/src/Editor/Infrastructure/ThirdPartyToolMigration/ThirdPartyToolMigrationPreflightScanner.csPackages/src/Editor/Infrastructure/ThirdPartyToolMigration/ThirdPartyToolMigrationRules.csPackages/src/Editor/Infrastructure/ThirdPartyToolMigration/ThirdPartyToolMigrationToolContractDetectionRules.cs
💤 Files with no reviewable changes (3)
- Packages/src/Editor/Infrastructure/ThirdPartyToolMigration/ThirdPartyToolMigrationAliasRules.cs
- Packages/src/Editor/Infrastructure/ThirdPartyToolMigration/ThirdPartyToolMigrationConstructorArgumentRules.cs
- Packages/src/Editor/Infrastructure/ThirdPartyToolMigration/ThirdPartyToolMigrationToolContractDetectionRules.cs
There was a problem hiding this comment.
1 issue found and verified against the latest diff
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Remove the fallback that masked null content in the migrated source DTO so programmer mistakes remain visible outside debug assertions.
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
Notes
Verification