Skip to content

Conversation

msanatan
Copy link
Contributor

@msanatan msanatan commented Oct 19, 2025

We now notify users that there's a new version available in app. Of course, not all users are on Discord, and not all Discord users read their messages. This should help keep users on the most up-to-date version

Any users who are referencing the old package at UnityMcpBridge will now get a notice if they udpate

Summary by CodeRabbit

  • New Features

    • Added package update checking against the latest release on GitHub with daily caching and a clear-cache option.
    • Version label now highlights available updates (orange indicator and tooltip with details).
    • Added one-time migration dialog and a persistent migration banner linking to the migration guide.
  • Tests

    • Added comprehensive tests for version comparison, update-check behavior, cache handling, asset-store detection, and cache clearing.

@msanatan msanatan self-assigned this Oct 19, 2025
Copy link
Contributor

coderabbitai bot commented Oct 19, 2025

Walkthrough

Adds a package update service API and implementation that checks GitHub package.json for newer versions (cached daily via EditorPrefs), exposes it via the service locator, updates the editor UI when updates exist, adds tests, and introduces a one-time migration dialog/banner in the legacy editor window.

Changes

Cohort / File(s) Summary
Package Update Service API
MCPForUnity/Editor/Services/IPackageUpdateService.cs, MCPForUnity/Editor/Services/IPackageUpdateService.cs.meta
New public interface IPackageUpdateService with CheckForUpdate, IsNewerVersion, IsGitInstallation, and ClearCache; meta file added.
Package Update Service Implementation
MCPForUnity/Editor/Services/PackageUpdateService.cs, MCPForUnity/Editor/Services/PackageUpdateService.cs.meta
New PackageUpdateService implements IPackageUpdateService: daily EditorPrefs cache, fetches latest version from GitHub package.json, semantic version compare, Asset Store detection, cache clearing; meta file added.
Service Locator
MCPForUnity/Editor/Services/MCPServiceLocator.cs
Exposes new public static IPackageUpdateService Updates with lazy init, registers implementations, and disposes/resets the service.
Editor UI (new window)
MCPForUnity/Editor/Windows/MCPForUnityEditorWindowNew.cs
Adds UpdateVersionLabel() invoked during UI init to query Updates service and adjust version label styling/tooltip when an update is available.
Legacy Editor UI migration
UnityMcpBridge/Editor/Windows/MCPForUnityEditorWindow.cs
Adds one-time migration flow: ShowMigrationDialogIfNeeded() called from OnEnable() and DrawMigrationWarningBanner() called from OnGUI().
Unit tests
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Services/PackageUpdateServiceTests.cs, .../PackageUpdateServiceTests.cs.meta
New NUnit edit-mode tests covering IsNewerVersion scenarios, cache behavior, update detection, ClearCache, and Asset Store/non-Git handling; meta file added.

Sequence Diagram(s)

sequenceDiagram
    participant UI as Editor UI
    participant Locator as MCPServiceLocator
    participant Service as PackageUpdateService
    participant Cache as EditorPrefs
    participant GitHub as GitHub (package.json)

    UI->>Locator: MCPServiceLocator.Updates.CheckForUpdate(currentVersion)
    Locator->>Service: forward CheckForUpdate
    Service->>Cache: Read last-check timestamp & cached latestVersion
    alt cache recent (<1 day)
        Cache-->>Service: cached latestVersion
        Service->>Service: IsNewerVersion(cached, current)
        Service-->>UI: UpdateCheckResult (from cache)
    else cache missing/expired
        Service->>Service: IsGitInstallation()
        alt Asset Store installation
            Service-->>UI: UpdateCheckResult (skipped - Asset Store)
        else Git installation
            Service->>GitHub: Fetch package.json
            alt fetch success
                GitHub-->>Service: latestVersion
                Service->>Cache: store latestVersion & timestamp
                Service->>Service: IsNewerVersion(latest, current)
                Service-->>UI: UpdateCheckResult (fresh)
            else fetch failure
                Service-->>UI: UpdateCheckResult (failed, may use cached)
            end
        end
    end

    alt UpdateAvailable
        UI->>UI: show arrow, orange color, tooltip with Git URL
    else No update
        UI->>UI: show normal version label
    end
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

Suggested reviewers

  • dsarno
  • Scriptwonder

Poem

🐰 I hopped to fetch the latest line,

A cached carrot warmed by time.
An orange arrow points the way —
“New MCP!” it seems to say.
Tests snug, migration in a loop, hooray, I’ll hop away!

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The PR title "Notify users when there's a new version" directly and accurately reflects the main objective of the changeset. The modifications establish a new update-checking service infrastructure (IPackageUpdateService and PackageUpdateService) that queries GitHub for newer versions, integrates it into the service locator, and displays update notifications in the editor UI with visual indicators when a newer version is detected. The title clearly conveys the core user-facing feature being introduced without unnecessary detail or vagueness, and appropriately prioritizes the primary change from a user perspective.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

🧹 Nitpick comments (3)
MCPForUnity/Editor/Windows/MCPForUnityEditorWindowNew.cs (1)

837-857: Consider extracting the Git URL to a constant.

The Git URL on line 850 is hardcoded in the tooltip. Consider extracting this to a constant or deriving it from package.json to improve maintainability.

+        private const string GitRepositoryUrl = "https://github.com/CoplayDev/unity-mcp.git?path=/MCPForUnity";
+
         private void UpdateVersionLabel()
         {
             string currentVersion = AssetPathUtility.GetPackageVersion();
             versionLabel.text = $"v{currentVersion}";

             // Check for updates using the service
             var updateCheck = MCPServiceLocator.Updates.CheckForUpdate(currentVersion);

             if (updateCheck.UpdateAvailable && !string.IsNullOrEmpty(updateCheck.LatestVersion))
             {
                 // Update available - enhance the label
                 versionLabel.text = $"\u2191 v{currentVersion} (Update available: v{updateCheck.LatestVersion})";
                 versionLabel.style.color = new Color(1f, 0.7f, 0f); // Orange
-                versionLabel.tooltip = $"Version {updateCheck.LatestVersion} is available. Update via Package Manager.\n\nGit URL: https://github.com/CoplayDev/unity-mcp.git?path=/MCPForUnity";
+                versionLabel.tooltip = $"Version {updateCheck.LatestVersion} is available. Update via Package Manager.\n\nGit URL: {GitRepositoryUrl}";
             }
             else
             {
                 versionLabel.style.color = StyleKeyword.Null; // Default color
                 versionLabel.tooltip = $"Current version: {currentVersion}";
             }
         }
MCPForUnity/Editor/Services/PackageUpdateService.cs (1)

129-159: Consider async network call for better editor responsiveness.

The synchronous WebClient.DownloadString (line 145) blocks the editor thread. While the daily cache mitigates this, consider using async/await with HttpClient for a more responsive UI, especially on slower networks.

Note: WebClient is deprecated in newer .NET versions. If Unity's .NET version supports it, consider migrating to HttpClient with async/await pattern for better performance and future compatibility.

TestProjects/UnityMCPTests/Assets/Tests/EditMode/Services/PackageUpdateServiceTests.cs (1)

1-251: Consider adding tests for IsGitInstallation and refactoring for better testability.

The test suite has good coverage for version comparison and cache behavior, but is missing coverage for:

  1. IsGitInstallation method: This method determines whether the package is installed via Git or Asset Store, which affects the update check behavior. This is testable without network calls.

  2. Asset Store installation path: The early-return behavior in CheckForUpdate when IsGitInstallation() returns false is not tested.

  3. Network dependency: Tests at lines 182-197 and 228-249 are non-deterministic because they depend on actual network state.

Recommended additions:

Add tests for IsGitInstallation:

[Test]
public void IsGitInstallation_ReturnsTrue_ForPackagesPath()
{
    // This test requires mocking AssetPathUtility.GetMcpPackageRootPath()
    // or making IsGitInstallation testable via dependency injection
}

[Test]
public void CheckForUpdate_SkipsCheck_ForAssetStoreInstallation()
{
    // Test that Asset Store installations return appropriate message
}

Consider dependency injection for better testability:

Refactor PackageUpdateService to accept an injectable version fetcher interface, which would allow you to:

  • Mock network calls for deterministic tests
  • Test the full CheckForUpdate flow including fresh checks
  • Eliminate non-deterministic test behavior
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 3503378 and 4df0345.

📒 Files selected for processing (9)
  • MCPForUnity/Editor/Services/IPackageUpdateService.cs (1 hunks)
  • MCPForUnity/Editor/Services/IPackageUpdateService.cs.meta (1 hunks)
  • MCPForUnity/Editor/Services/MCPServiceLocator.cs (3 hunks)
  • MCPForUnity/Editor/Services/PackageUpdateService.cs (1 hunks)
  • MCPForUnity/Editor/Services/PackageUpdateService.cs.meta (1 hunks)
  • MCPForUnity/Editor/Windows/MCPForUnityEditorWindowNew.cs (2 hunks)
  • TestProjects/UnityMCPTests/Assets/Tests/EditMode/Services/PackageUpdateServiceTests.cs (1 hunks)
  • TestProjects/UnityMCPTests/Assets/Tests/EditMode/Services/PackageUpdateServiceTests.cs.meta (1 hunks)
  • UnityMcpBridge/Editor/Windows/MCPForUnityEditorWindow.cs (3 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
MCPForUnity/Editor/Services/IPackageUpdateService.cs (1)
MCPForUnity/Editor/Services/PackageUpdateService.cs (4)
  • IsNewerVersion (74-100)
  • IsGitInstallation (103-117)
  • ClearCache (120-124)
  • UpdateCheckResult (19-71)
MCPForUnity/Editor/Services/MCPServiceLocator.cs (1)
MCPForUnity/Editor/Services/PackageUpdateService.cs (1)
  • PackageUpdateService (12-160)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Services/PackageUpdateServiceTests.cs (2)
MCPForUnity/Editor/Services/PackageUpdateService.cs (3)
  • PackageUpdateService (12-160)
  • IsNewerVersion (74-100)
  • ClearCache (120-124)
MCPForUnity/Editor/Services/IPackageUpdateService.cs (2)
  • IsNewerVersion (21-21)
  • ClearCache (32-32)
MCPForUnity/Editor/Services/PackageUpdateService.cs (2)
MCPForUnity/Editor/Services/IPackageUpdateService.cs (5)
  • UpdateCheckResult (13-13)
  • UpdateCheckResult (38-59)
  • IsNewerVersion (21-21)
  • IsGitInstallation (27-27)
  • ClearCache (32-32)
MCPForUnity/Editor/Helpers/AssetPathUtility.cs (1)
  • GetMcpPackageRootPath (39-78)
MCPForUnity/Editor/Windows/MCPForUnityEditorWindowNew.cs (2)
MCPForUnity/Editor/Helpers/AssetPathUtility.cs (2)
  • AssetPathUtility (13-161)
  • GetPackageVersion (142-160)
MCPForUnity/Editor/Services/MCPServiceLocator.cs (1)
  • MCPServiceLocator (8-70)
🔇 Additional comments (9)
UnityMcpBridge/Editor/Windows/MCPForUnityEditorWindow.cs (2)

1582-1610: Migration dialog implementation looks good.

The one-time dialog with three options provides a good UX for legacy users. The "Remind Me Later" option intentionally doesn't set the flag, ensuring users see the dialog again until they take action.


1612-1639: Non-dismissible banner provides appropriate persistent notice.

The always-visible migration warning ensures legacy users are aware of the need to migrate, complementing the one-time dialog.

MCPForUnity/Editor/Services/MCPServiceLocator.cs (1)

16-24: Service locator integration follows established patterns.

The new Updates service is properly integrated with lazy initialization, registration support, and disposal handling consistent with other services in the locator.

Also applies to: 45-46, 60-60, 68-68

MCPForUnity/Editor/Services/IPackageUpdateService.cs (1)

1-60: Well-designed interface with clear documentation.

The IPackageUpdateService interface and UpdateCheckResult class provide a clean API surface with comprehensive XML documentation.

MCPForUnity/Editor/Services/PackageUpdateService.cs (2)

19-71: Update check implementation with appropriate caching.

The daily cache prevents excessive network requests, and the logic correctly handles Asset Store installations. The synchronous network call (line 48) may briefly block the editor UI, but given the daily cache, this should occur infrequently.


103-117: Installation detection and cache management work correctly.

IsGitInstallation properly distinguishes between Package Manager and Asset Store installations. ClearCache correctly removes both cache keys.

Also applies to: 120-124

TestProjects/UnityMCPTests/Assets/Tests/EditMode/Services/PackageUpdateServiceTests.cs (3)

42-114: Excellent version comparison test coverage!

These tests comprehensively cover the semantic version comparison logic including major/minor/patch comparisons, prefix handling (v/V), mixed prefixes, double-digit versions, and invalid format handling. The test names are descriptive and assertions include helpful messages.


116-180: Well-structured cache behavior tests.

These tests effectively verify the caching logic by setting up EditorPrefs with today's date and various version scenarios. They properly assert both the cache hit behavior and the correct UpdateAvailable determination based on version comparisons.


199-226: Good cache clearing test coverage.

These tests properly verify both the happy path (clearing existing cache) and the edge case (clearing non-existent cache doesn't throw). The verification steps are thorough and correct.

Comment on lines +74 to +100
public bool IsNewerVersion(string version1, string version2)
{
try
{
// Remove any "v" prefix
version1 = version1.TrimStart('v', 'V');
version2 = version2.TrimStart('v', 'V');

var version1Parts = version1.Split('.');
var version2Parts = version2.Split('.');

for (int i = 0; i < Math.Min(version1Parts.Length, version2Parts.Length); i++)
{
if (int.TryParse(version1Parts[i], out int v1Num) &&
int.TryParse(version2Parts[i], out int v2Num))
{
if (v1Num > v2Num) return true;
if (v1Num < v2Num) return false;
}
}
return false;
}
catch
{
return false;
}
}
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Fix version comparison logic for differing part counts.

IsNewerVersion has a bug when comparing versions with different numbers of parts. Currently, IsNewerVersion("1.0.1", "1.0") returns false, but it should return true since 1.0.1 is newer than 1.0.

Apply this diff to handle versions with different part counts:

 public bool IsNewerVersion(string version1, string version2)
 {
     try
     {
         // Remove any "v" prefix
         version1 = version1.TrimStart('v', 'V');
         version2 = version2.TrimStart('v', 'V');

         var version1Parts = version1.Split('.');
         var version2Parts = version2.Split('.');

-        for (int i = 0; i < Math.Min(version1Parts.Length, version2Parts.Length); i++)
+        int maxLength = Math.Max(version1Parts.Length, version2Parts.Length);
+        
+        for (int i = 0; i < maxLength; i++)
         {
-            if (int.TryParse(version1Parts[i], out int v1Num) &&
-                int.TryParse(version2Parts[i], out int v2Num))
+            int v1Num = 0;
+            int v2Num = 0;
+            
+            if (i < version1Parts.Length)
+                int.TryParse(version1Parts[i], out v1Num);
+            if (i < version2Parts.Length)
+                int.TryParse(version2Parts[i], out v2Num);
+            
+            if (v1Num > v2Num) return true;
+            if (v1Num < v2Num) return false;
-            {
-                if (v1Num > v2Num) return true;
-                if (v1Num < v2Num) return false;
-            }
         }
         return false;
     }
     catch
     {
         return false;
     }
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
public bool IsNewerVersion(string version1, string version2)
{
try
{
// Remove any "v" prefix
version1 = version1.TrimStart('v', 'V');
version2 = version2.TrimStart('v', 'V');
var version1Parts = version1.Split('.');
var version2Parts = version2.Split('.');
for (int i = 0; i < Math.Min(version1Parts.Length, version2Parts.Length); i++)
{
if (int.TryParse(version1Parts[i], out int v1Num) &&
int.TryParse(version2Parts[i], out int v2Num))
{
if (v1Num > v2Num) return true;
if (v1Num < v2Num) return false;
}
}
return false;
}
catch
{
return false;
}
}
public bool IsNewerVersion(string version1, string version2)
{
try
{
// Remove any "v" prefix
version1 = version1.TrimStart('v', 'V');
version2 = version2.TrimStart('v', 'V');
var version1Parts = version1.Split('.');
var version2Parts = version2.Split('.');
int maxLength = Math.Max(version1Parts.Length, version2Parts.Length);
for (int i = 0; i < maxLength; i++)
{
int v1Num = 0;
int v2Num = 0;
if (i < version1Parts.Length)
int.TryParse(version1Parts[i], out v1Num);
if (i < version2Parts.Length)
int.TryParse(version2Parts[i], out v2Num);
if (v1Num > v2Num) return true;
if (v1Num < v2Num) return false;
}
return false;
}
catch
{
return false;
}
}
🤖 Prompt for AI Agents
In MCPForUnity/Editor/Services/PackageUpdateService.cs around lines 74 to 100,
the version comparison only compares shared parts and returns false for "1.0.1"
vs "1.0"; modify the logic so after the loop you inspect the remaining parts of
the longer version: parse each remaining segment as int (treat non-numeric or
missing as 0) and if any remaining segment in version1 > 0 return true, if any
remaining segment in version2 > 0 return false, otherwise fall back to false;
preserve the existing try/catch and trimming of 'v' prefixes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This package only uses semantic versions, bump-version.yml (not in PR) updates versions in a standard way

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Find and examine the bump-version.yml file
fd -t f "bump-version.yml" --exec cat {}

Length of output: 0


🏁 Script executed:

#!/bin/bash
# Check the package.json to see the version format
fd -t f "package.json" --exec cat {}

Length of output: 2261


🏁 Script executed:

#!/bin/bash
# Search for where GetPackageVersion is implemented to understand version retrieval
rg -nP -C5 "GetPackageVersion"

Length of output: 4114


🏁 Script executed:

#!/bin/bash
# Check what version comparison test cases exist
rg -nP -A2 "IsNewerVersion" TestProjects/UnityMCPTests/Assets/Tests/EditMode/Services/PackageUpdateServiceTests.cs | head -80

Length of output: 2635


🏁 Script executed:

#!/bin/bash
# Look for .github/workflows directory and bump-version file
fd -t f "bump" -e yml -e yaml

Length of output: 0


@msanatan, thanks for the clarification! You're absolutely right—if the package strictly uses semantic versioning (major.minor.patch) and bump-version.yml ensures versions are always in that format, then the edge case CodeRabbit flagged (comparing versions with different part counts like "1.0.1" vs "1.0") won't occur in practice.

Looking at your test suite, all the test cases confirm this: they all use complete three-part versions ("2.0.0", "1.2.0", "1.0.2", etc.), and the package.json shows versions like "4.1.1" and "6.1.0".

Since IsNewerVersion is primarily used internally to compare package versions that are always semantic versioned, the current implementation should work fine for your use case. The bug would only surface if someone called the method with inconsistent version formats, which doesn't happen in your workflow.

Feel free to disregard CodeRabbit's suggestion if you're confident in your versioning process! 👍

Comment on lines +11 to +12
private const string TestLastCheckDateKey = "MCPForUnity.LastUpdateCheck";
private const string TestCachedVersionKey = "MCPForUnity.LatestKnownVersion";
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Hardcoded cache keys duplicate private implementation details.

These string literals duplicate private constants from PackageUpdateService. If the service changes its EditorPrefs keys, these tests will silently pass while testing the wrong keys.

Consider one of these approaches:

Option 1: Extract keys to a shared constant class

Create a shared constants file:

namespace MCPForUnity.Editor.Services
{
    internal static class PackageUpdateConstants
    {
        internal const string LastCheckDateKey = "MCPForUnity.LastUpdateCheck";
        internal const string CachedVersionKey = "MCPForUnity.LatestKnownVersion";
    }
}

Then reference these in both the service and tests:

-private const string TestLastCheckDateKey = "MCPForUnity.LastUpdateCheck";
-private const string TestCachedVersionKey = "MCPForUnity.LatestKnownVersion";
+private const string TestLastCheckDateKey = PackageUpdateConstants.LastCheckDateKey;
+private const string TestCachedVersionKey = PackageUpdateConstants.CachedVersionKey;

Option 2: Use reflection to access the private constants

-private const string TestLastCheckDateKey = "MCPForUnity.LastUpdateCheck";
-private const string TestCachedVersionKey = "MCPForUnity.LatestKnownVersion";
+private static readonly string TestLastCheckDateKey = 
+    typeof(PackageUpdateService).GetField("LastCheckDateKey", BindingFlags.NonPublic | BindingFlags.Static)
+        .GetValue(null) as string;
+private static readonly string TestCachedVersionKey = 
+    typeof(PackageUpdateService).GetField("CachedVersionKey", BindingFlags.NonPublic | BindingFlags.Static)
+        .GetValue(null) as string;
🤖 Prompt for AI Agents
In
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Services/PackageUpdateServiceTests.cs
around lines 11-12, the test hardcodes EditorPrefs key strings that duplicate
private constants in PackageUpdateService; change tests to use the actual keys
instead of duplicating them by either (preferred) moving the keys into a shared
internal constants class (e.g.,
MCPForUnity.Editor.Services.PackageUpdateConstants with LastCheckDateKey and
CachedVersionKey) and referencing that constant from both the service and tests,
or (alternative) read the private constant values via reflection in the test so
the keys remain single-sourced; update using statements/namespace references as
needed and remove the hardcoded literals.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch but low enough risk

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@msanatan, understood! I appreciate you considering the trade-off. If the keys ever do change in the future and the tests need updating, the duplication will be easy enough to spot. Feel free to reach out if you'd like to revisit this later.

    /)_/)
   (•_• )
   />📝 Fair enough!

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

♻️ Duplicate comments (1)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Services/PackageUpdateServiceTests.cs (1)

11-12: Hardcoded cache keys still duplicate private implementation details.

This issue from the previous review remains unaddressed. The test constants duplicate private constants from PackageUpdateService, creating a maintenance risk if the service changes its EditorPrefs keys.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 4df0345 and 455dbdf.

📒 Files selected for processing (1)
  • TestProjects/UnityMCPTests/Assets/Tests/EditMode/Services/PackageUpdateServiceTests.cs (1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Services/PackageUpdateServiceTests.cs (2)
MCPForUnity/Editor/Services/PackageUpdateService.cs (3)
  • PackageUpdateService (12-160)
  • IsNewerVersion (74-100)
  • ClearCache (120-124)
MCPForUnity/Editor/Services/IPackageUpdateService.cs (2)
  • IsNewerVersion (21-21)
  • ClearCache (32-32)
🔇 Additional comments (3)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Services/PackageUpdateServiceTests.cs (3)

14-40: LGTM!

Setup and teardown logic correctly ensures clean EditorPrefs state before and after each test.


42-114: LGTM!

Version comparison tests provide comprehensive coverage including edge cases (prefixes, multi-digit versions, invalid formats) with clear assertions.


182-209: LGTM!

Cache clearing tests properly verify removal of both cache keys and graceful handling when no cache exists.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (2)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Services/PackageUpdateServiceTests.cs (2)

182-210: Consider making the expired cache test deterministic.

The test correctly verifies that expired cache triggers a fresh fetch, but it branches on network availability, making it non-deterministic. Additionally, the assertion at line 200 assumes the fetched version differs from "4.0.0", which could cause false failures if that's the actual GitHub version.

Consider refactoring PackageUpdateService to accept an injectable HTTP client or version fetcher, enabling you to mock network responses in tests:

// In test:
var mockFetcher = new Mock<IVersionFetcher>();
mockFetcher.Setup(f => f.FetchLatestVersion()).Returns("5.1.0");
var service = new PackageUpdateService(mockFetcher.Object);

// Arrange: expired cache
string yesterday = DateTime.Now.AddDays(-1).ToString("yyyy-MM-dd");
EditorPrefs.SetString(TestLastCheckDateKey, yesterday);
EditorPrefs.SetString(TestCachedVersionKey, "4.0.0");

// Act
var result = service.CheckForUpdate("5.0.0");

// Assert
Assert.IsTrue(result.CheckSucceeded);
Assert.AreEqual("5.1.0", result.LatestVersion, "Should fetch fresh version, not use expired cache");
mockFetcher.Verify(f => f.FetchLatestVersion(), Times.Once);

This would provide deterministic, reliable verification that expired cache is bypassed.


232-259: ClearCache tests verify key deletion correctly.

The tests properly validate that cache keys are removed and that the operation is safe when no cache exists.

Consider adding a behavioral test to verify that after ClearCache(), the next CheckForUpdate() performs a fresh fetch rather than using stale data:

[Test]
public void CheckForUpdate_PerformsFreshFetch_AfterCacheCleared()
{
    // Arrange: Set up valid cache
    EditorPrefs.SetString(TestLastCheckDateKey, DateTime.Now.ToString("yyyy-MM-dd"));
    EditorPrefs.SetString(TestCachedVersionKey, "1.0.0");
    
    // Verify cached version is used
    var cachedResult = _service.CheckForUpdate("1.0.0");
    Assert.AreEqual("1.0.0", cachedResult.LatestVersion);
    
    // Act: Clear cache
    _service.ClearCache();
    
    // Next check should fetch fresh (or fail if offline, but not use old cache)
    var freshResult = _service.CheckForUpdate("1.0.0");
    
    // Assert: If check succeeded, cache should be repopulated with today's date
    if (freshResult.CheckSucceeded)
    {
        Assert.AreEqual(DateTime.Now.ToString("yyyy-MM-dd"), 
            EditorPrefs.GetString(TestLastCheckDateKey, ""));
    }
}

This would complement the existing tests by verifying the end-to-end behavior.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 455dbdf and cff607a.

📒 Files selected for processing (1)
  • TestProjects/UnityMCPTests/Assets/Tests/EditMode/Services/PackageUpdateServiceTests.cs (1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Services/PackageUpdateServiceTests.cs (2)
MCPForUnity/Editor/Services/PackageUpdateService.cs (5)
  • PackageUpdateService (12-160)
  • IsNewerVersion (74-100)
  • ClearCache (120-124)
  • UpdateCheckResult (19-71)
  • IsGitInstallation (103-117)
MCPForUnity/Editor/Services/IPackageUpdateService.cs (5)
  • IsNewerVersion (21-21)
  • ClearCache (32-32)
  • UpdateCheckResult (13-13)
  • UpdateCheckResult (38-59)
  • IsGitInstallation (27-27)
🔇 Additional comments (4)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Services/PackageUpdateServiceTests.cs (4)

42-114: Comprehensive version comparison test coverage.

The IsNewerVersion test suite thoroughly covers major/minor/patch comparisons, version prefixes, equality checks, and edge cases including invalid formats and multi-digit version numbers.


116-180: Solid cache behavior validation.

The CheckForUpdate tests effectively verify that valid cache is used correctly and that the UpdateAvailable flag is computed accurately across different version scenarios.


212-230: Asset Store installation behavior properly tested.

The test correctly validates that Asset Store installations receive the appropriate message and don't attempt GitHub updates. Using MockAssetStorePackageUpdateService is a clean approach to isolate this scenario.


262-294: Clean mock implementation for Asset Store testing.

The mock service provides a focused implementation for testing Asset Store installation behavior, with appropriate comments explaining minimal implementations of unused interface methods.

@msanatan msanatan merged commit 673456b into CoplayDev:main Oct 19, 2025
1 check passed
@msanatan msanatan deleted the add-version-update-notice branch October 19, 2025 00:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant